From d4e47689ab4ad57cc06d1b2c08e40e41bde1d7ff Mon Sep 17 00:00:00 2001 From: tzhouam Date: Sat, 26 Sep 2026 06:35:12 +0800 Subject: [PATCH] Separate PR review refinement from step orchestration --- doc/architecture/SPEC/engine/steps/review.md | 10 +- .../engine/steps/review/__init__.py | 5 +- .../engine/steps/review/refinement.py | 522 +++++++++++++++++ .../engine/steps/review/steps.py | 523 +----------------- test/test_agent_ensemble.py | 2 +- test/test_review_render.py | 2 +- 6 files changed, 538 insertions(+), 526 deletions(-) create mode 100644 src/infermatrix_copilot/engine/steps/review/refinement.py diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index 1f720f47d..84cee2a17 100644 --- a/doc/architecture/SPEC/engine/steps/review.md +++ b/doc/architecture/SPEC/engine/steps/review.md @@ -2,7 +2,7 @@ -`patch gate + PR review + quality（8 个源文件） · step 库（评审） · refactor-status: ok` +`patch gate + PR review + quality（9 个源文件） · step 库（评审） · refactor-status: ok` ## 职责 条件式 patch 门 + PR 评审 agent step + 有界的 PR review-readiness 质量 step， @@ -11,7 +11,7 @@ Pre-push patch 门必须审查显式 base 到当前 HEAD 的已提交变更； tracked checkout、不可解析的 ref 或超过 reviewer 容量的 diff 都阻塞推送。 通过后记录批准的 base/head，后续推送必须验证当前 HEAD 仍匹配。 它曾是一个 341 行的模块；现在将提交范围的 mutation gate、只读 PR 评审 -agent、评测调优过的 prompt 数据和确定性 helper 分开。 +agent、评审结果的有界精炼、评测调优过的 prompt 数据和确定性 helper 分开。 ## 包内布局（一个文件一个关注点） - `__init__.py` —— import `patch_gate`、`quality` 和 `steps` 以触发各自的 @@ -23,8 +23,10 @@ agent、评测调优过的 prompt 数据和确定性 helper 分开。 - `patch_gate.py` —— `review.patch_gate`（validation/read）：捕获已提交的 base/head 范围、计算触发条件、审查精确 diff 并记录批准的 head；不依赖 PR review agent 的 prompt/render 控制流。 -- `steps.py` —— `agent.review_diff`（agent/read）及其评审覆盖、二轮检查、 - 评论核验控制流；不批准或推送 mutation。 +- `steps.py` —— `agent.review_diff`（agent/read）：冻结 evidence pack、 + 选择评审深度、调用评审 passes 并组装最终结果；不批准或推送 mutation。 +- `refinement.py` —— 对 draft 结果进行覆盖补充、定向第二轮、逐评论核验； + 接受 handler 提供的 frozen diff、工具和 trace，不注册 step 或发布评论。 - `anchor.py` —— 基于代码片段的评论锚定（2026-08 新增）。 - `repo_tools.py` —— 只读的变更考古工具组（2026-08 新增）。 - `quality.py` —— `agent.assess_pr_quality`：一次 tool-less、只读模型调用， diff --git a/src/infermatrix_copilot/engine/steps/review/__init__.py b/src/infermatrix_copilot/engine/steps/review/__init__.py index 4e019f082..f25a20583 100644 --- a/src/infermatrix_copilot/engine/steps/review/__init__.py +++ b/src/infermatrix_copilot/engine/steps/review/__init__.py @@ -3,8 +3,9 @@ Importing this package imports `patch_gate`, `quality`, and `steps` for their `@step` registration side effects. The committed-range mutation gate -lives apart from the read-only review agent and its prompt/render helpers. The public contract below (spec: `engine/steps/review`) is re-exported -so existing `from ..steps.review import X` importers are unchanged. +lives apart from the read-only review agent; the agent's bounded refinement +passes live in `refinement`. The public contract below (spec: +`engine/steps/review`) is re-exported for existing importers. """ from __future__ import annotations diff --git a/src/infermatrix_copilot/engine/steps/review/refinement.py b/src/infermatrix_copilot/engine/steps/review/refinement.py new file mode 100644 index 000000000..77181e0c3 --- /dev/null +++ b/src/infermatrix_copilot/engine/steps/review/refinement.py @@ -0,0 +1,522 @@ +"""Bounded refinement of a draft PR review before publication. + +Coverage promotion, the targeted second round, and per-comment verification +share the same draft result but never own step registration or GitHub effects. +The review handler supplies a frozen diff, tools, and tracing context. +""" + +from __future__ import annotations + +import asyncio +import json +import re + +from ....llm import parse_json_reply +from ...step import StepContext +from .utils import _SEVERITY_ORDER, _same_finding + + +_RESIDUAL_MARKERS = ("residual", "not covered", "does not cover", "still ", + "remains ", "left unfixed", "but ") + + +def _promote_resolved_residuals(ctx: StepContext, output: dict) -> dict: + """Turn every `[resolved]` findings line that states a RESIDUAL into a + review comment. + + Measured across two holdouts: the ground truth on merged/amended heads is + ~70% "a reviewer raised X, the fix landed — what does it still not + cover?", and the passes DO produce that reasoning (the `[resolved]` + contract in prompts.py asks for exactly it). But `[resolved]` is a + FINDINGS line, and findings render into the unscored 'Validated' block — + so the arm's best answers to the dominant question class were routed away + from the only channel a reader (or judge) scores. This promotion is + grounded by construction: it re-files a line the run already wrote and + verified, so it cannot invent a claim, and it sits outside the + coverage-promotion cap because it is not a discretionary addition.""" + findings = [str(f) for f in (output.get("findings") or [])] + comments = list(output.get("review_comments") or []) + covered = {(str(c.get("file") or ""), str(c.get("comment") or "")[:60]) + for c in comments} + added: list[dict] = [] + for line in findings: + low = line.lstrip().lower() + if not low.startswith("[resolved]"): + continue + body = line.split("]", 1)[-1].strip() + if not any(m in body.lower() for m in _RESIDUAL_MARKERS): + continue # a bare confirmation is not a finding + m = re.search(r"([\w./\-]+\.\w+):(\d+)", body) + file_, line_no = (m.group(1), int(m.group(2))) if m else ("", None) + if (file_, body[:60]) in covered: + continue + # ...and against EACH OTHER. `covered` only holds the pre-existing + # comments, so N resolved lines about one residual promoted N times: + # pr4977 shipped four near-identical "the PR description still claims + # trust_remote_code" comments (the cap of 4, saturated) out of five + # `[resolved]` lines stating that one residual, and the judge docked + # it for "4 nearly-identical inline comments restating the same + # PR-description staleness point, hurting signal density". Promotion + # is where that duplication is cheapest to stop — before these + # compete for the comment budget against distinct findings. + if any(_same_finding(body, prev["comment"]) for prev in added): + continue + added.append({"file": file_, "line": line_no, "severity": "minor", + "comment": body, + "evidence": line.strip(), + "corroborated_by": ["resolved-residual"]}) + if not added: + ctx.trace.record("review_resolved_promoted", step="agent.review_diff", + added=0, resolved_lines=sum( + 1 for f in findings + if f.lstrip().lower().startswith("[resolved]"))) + return output + out = dict(output) + out["review_comments"] = comments + added[:4] + ctx.trace.record("review_resolved_promoted", step="agent.review_diff", + added=len(added[:4])) + return out + + +async def _promote_uncovered(ctx: StepContext, output: dict, + spec: dict) -> dict: + """Coverage-promotion pass: one tool-less LLM call that promotes + maintainer-relevant concerns ALREADY PRESENT in the run's own findings/ + validated/blockers lines into review comments. + + Train forensics showed the recall loss is often not investigation but + conversion: the ground-truth concern sat verbatim in a lens's findings or + the run's blockers ("the VoxCPM2 regression is a separate mechanism", + "NPU lane is red", "diff evidence was truncated") and never became a + comment. Promotion is grounded by construction — the call may only cite + material from the lines it is shown, never new claims — so it raises + recall without the speculation cost of widening the lens budget.""" + findings = [str(x) for x in (output.get("findings") or [])][:60] + blockers = [str(x) for x in (output.get("blockers") or [])][:10] + assumptions = [str(x) for x in (output.get("assumptions") or [])][:10] + comments = output.get("review_comments") or [] + if not (findings or blockers) or ctx.llm is None \ + or not getattr(ctx.llm, "available", False): + ctx.trace.record("review_coverage_skipped", + step="agent.review_diff", + reason="no findings" if not (findings or blockers) + else "no llm") + return output + system = ( + "You are the coverage editor for a PR review. You receive the " + "review's KEPT COMMENTS plus the reviewers' raw FINDINGS/BLOCKERS/" + "ASSUMPTIONS lines. Identify up to 3 maintainer-relevant concerns " + "that appear in the raw lines but are NOT covered by any kept " + "comment, and promote each into a comment object. Prioritize these " + "classes: blast-radius of a changed default/shared value, benchmark " + "evidence for perf/capacity changes, test integrity (a test that " + "cannot fail or is never selected), dependency-version " + "compatibility, resource lifecycle on abort paths, scope explicitly " + "left unfixed (linked-issue remainder), red CI on the head, " + "machinery duplicating a named existing helper (sibling contrast), " + "a refuted PR-body claim, and — MOST OFTEN MISSED — GUARANTEE-GAP " + "RESIDUE hiding in the [validated]/[claim-verified]/[sweep] lines: " + "any verified fact that records a WEAKER guarantee than the PR " + "needs (a mock that authors the very value the test asserts, a " + "single-variant validation of a multi-variant feature, a gate " + "relocated to a slower lane, 'proves route propagation only', " + "'covered by weekly only') is a FINDING wearing a validation " + "stamp — promote it as the pointed question or ask it implies. " + "Rules: " + "(1) ONLY promote what the raw lines already state — no new claims, " + "no re-investigation; quote the source line in `evidence`. (2) A " + "promoted line KEEPS its directive force: a concern naming a " + "concrete defect, missing update, or duplicated machinery is " + "phrased as the change to make (name both files and the helper), " + "NEVER as a 'could you confirm…?' question; only a validation with " + "a genuinely unresolved residual becomes a scoping ask. (3) " + "severity: minor unless the raw line " + "demonstrates breakage (then major). (4) If everything relevant is " + "already covered, return no additions. Reply with exactly one JSON " + 'object: {"additions": [{"file": str, "line": int, "severity": ' + '"major"|"minor", "comment": str, "evidence": str}]}') + prompt = ( + "## KEPT COMMENTS\n" + + json.dumps([{k: c.get(k) for k in ("file", "line", "severity", + "comment")} + for c in comments], ensure_ascii=False, indent=1) + + "\n\n## RAW FINDINGS\n" + "\n".join(f"- {x}" for x in findings) + + ("\n\n## BLOCKERS\n" + "\n".join(f"- {x}" for x in blockers) + if blockers else "") + + ("\n\n## ASSUMPTIONS\n" + "\n".join(f"- {x}" for x in assumptions) + if assumptions else "")) + try: + _tt = ctx.settings.tier_target(spec.get("mode", "eco")) + llm = (ctx.llm.for_target(_tt) + if hasattr(ctx.llm, "for_target") else ctx.llm) + reply = await asyncio.to_thread( + llm.create, system=system, + messages=[{"role": "user", "content": prompt}], + model=ctx.settings.review_promotion_model or _tt.model, + role="reducer", + max_tokens=max(4096, ctx.settings.llm_max_tokens)) + obj = parse_json_reply(reply.text or "") + except Exception as exc: # never fail the review over the extra pass — + ctx.trace.record("review_coverage_skipped", # but say WHY, loudly + step="agent.review_diff", + reason=f"{type(exc).__name__}: {exc}"[:200]) + return output + additions = (obj or {}).get("additions") if isinstance(obj, dict) else None + kept: list[dict] = [] + for a in (additions or [])[:3]: + if not isinstance(a, dict) or not a.get("comment") \ + or not a.get("evidence"): + continue + # mechanical near-dup guard vs kept comments: the prompt's "NOT + # covered by any kept comment" rule was observed re-adding a + # merged-away duplicate (wave-3 pr6049 — the reducer collapsed two + # candidates, promotion re-added the merged-away variant, judges + # penalized the pair). Nearby anchor alone is not duplication (a + # linked-issue remainder often anchors beside the defect), so the + # guard also requires substantial text overlap. + def _words(c): + return set(str(c.get("comment") or "").lower().split()) + aw = _words(a) + if aw and any( + str(a.get("file") or "") == str(c.get("file") or "") + and isinstance(a.get("line"), int) + and isinstance(c.get("line"), int) + and abs(a["line"] - c["line"]) <= 8 + and len(aw & _words(c)) >= max(4, len(aw) // 2) + for c in comments): + continue + kept.append({"file": str(a.get("file") or "?"), + "line": a.get("line"), + "severity": str(a.get("severity") or "minor").lower(), + "comment": str(a["comment"]), + "evidence": str(a["evidence"]), + # ordering tag: promoted items carry a protected-class + # concern; under the comment budget they rank ahead of + # uncorroborated same-severity items (stripped at cap) + "corroborated_by": ["coverage"]}) + if kept: + output = dict(output) + output["review_comments"] = list(comments) + kept + ctx.trace.record("review_coverage_promoted", + step="agent.review_diff", added=len(kept)) + else: + ctx.trace.record("review_coverage_skipped", + step="agent.review_diff", + reason="model returned no valid additions", + parsed=obj is not None, + reply_head=str(reply.text or "")[:200]) + return output + + +def _uncovered_hunks(diff: str, output: dict) -> list[str]: + """Hunk clusters with no comment anchored near them and no findings line + citing a nearby file:line — the second round's coverage seed, at the + granularity GT actually has. v14's file-level seed measured too coarse on + the wave-3 gate: one comment anywhere in a file marked the whole file + covered, while the human GT this is judged against is per-hunk inline + comments (the losses concentrated on multi-hunk files whose comments + clustered on one region). Test files included: an uncovered test hunk is + where test-integrity findings hide. Returns `path:start` entries.""" + import re as _re + + regions: dict[str, list[int]] = {} + current = None + for line in str(diff or "").splitlines(): + if line.startswith("+++ b/"): + current = line[6:] + elif line.startswith("@@") and current: + m = _re.search(r"\+(\d+)", line) + if m: + regions.setdefault(current, []).append(int(m.group(1))) + comments = output.get("review_comments") or [] + cited: dict[str, list[int]] = {} + for f in output.get("findings") or []: + # Only CLAIM/RESOLVED anchors count as coverage. Blanket + # [sweep]/[validated] stamps do not: wave-3 forensics found them to + # be false negatives on exactly the GT zones ("TP group build … + # verified" where the crash was), and their density suppressed the + # second round on every hunk it could have rescued (pr5678: skipped + # with four missed-GT hunks "covered" by self-issued stamps). + low = str(f).lstrip().lower() + if not low.startswith(("[claim-", "[resolved]")): + continue + for base, num in _re.findall(r"([\w.\-]+\.\w+):(\d+)", str(f)): + cited.setdefault(base, []).append(int(num)) + out: list[str] = [] + for path, starts in regions.items(): + base = path.rsplit("/", 1)[-1] + file_comments = [c.get("line") for c in comments + if str(c.get("file") or "").endswith(base) + and isinstance(c.get("line"), int)] + file_cited = cited.get(base, []) + has_any_comment = any(str(c.get("file") or "").endswith(base) + for c in comments) + for s in starts: + near = any(abs(n - s) <= 40 for n in file_comments + file_cited) + # a file-level comment (no line) covers a single-hunk file only + if not near and not (has_any_comment and len(starts) == 1): + out.append(f"{path}:{s}") + return out + + +async def _second_round(ctx: StepContext, output: dict, common: dict, + diff: str) -> dict: + """Coverage-driven second investigation round (RFC q3): one bounded pass + seeded by the run's own coverage holes, instead of a bigger fixed pass + count. Additions face the same verify pass as first-round comments.""" + from ...agent_runtime import run_agent_step + + if not ctx.settings.review_second_round: + return output + # 8 seeds, not 14: a 14-seed round-2 was measured producing finals too + # large to coerce on the wave-4 big items (both 14-seed runs failed, + # both sub-10-seed runs succeeded) — fewer, deeper hunk visits beat a + # sweep that never files + uncovered = _uncovered_hunks(diff, output)[:8] + claims_unchecked = "[claim-" not in " ".join( + str(f) for f in (output.get("findings") or [])) + if len(uncovered) < ctx.settings.review_second_round_min_files \ + and not claims_unchecked: + ctx.trace.record("review_second_round", step="agent.review_diff", + skipped="no coverage holes", uncovered=0) + return output + kept = [{k: c.get(k) for k in ("file", "line", "severity", "comment")} + for c in (output.get("review_comments") or [])] + guidance = ( + "You are the SECOND-ROUND reviewer. The first round produced the " + "KEPT COMMENTS in your evidence; your job is ONLY its coverage " + "holes — do not re-litigate or duplicate what is already covered.\n" + + (("These diff hunks (file:start-line) have NO comment and NO " + "recorded verification near them. Review each AT MAINTAINER " + "INLINE GRANULARITY: page to the hunk in situ, and either raise " + "one specific localized comment (the concrete ask a human " + "reviewer would leave on that hunk) or record a one-line " + "[validated]/[resolved] findings entry naming file:line — " + "silence on a changed hunk is not an option:\n" + + "\n".join(f"- {p}" for p in uncovered) + "\n") + if uncovered else "") + + (("The PR body's checkable claims were never verified (no " + "[claim-*] findings line exists). Build the claim ledger now: " + "verify or refute each checkable body claim (checklist 13).\n") + if claims_unchecked else "") + + "Every comment needs verbatim-quoted evidence with file:line. " + "Budget discipline: reserve your last round for the output " + "contract.") + # per-file diff slices for the seed files: on big PRs the shared pr_diff + # evidence is CAPPED and the tail files' hunks never reach any pass — + # wave-3 pr5691's round-2 read the right 10 files but held the same + # truncated diff blob, so the beyond-cap hunks stayed unreviewable. The + # slices come from the UNCAPPED diff text. + seed_paths = {p.rsplit(":", 1)[0] for p in uncovered} + slices: list[str] = [] + for chunk in re.split(r"(?m)^(?=diff --git )", str(diff or "")): + m = re.search(r"^\+\+\+ b/(.+)$", chunk, re.M) + if m and m.group(1) in seed_paths: + slices.append(chunk) + hunk_evidence = "".join(slices)[:100_000] + from ...agent_runtime.ensemble import ( + lens_backend_member, + outcome_blocked as _seat_produced_nothing, + ) + + member = lens_backend_member(ctx.settings, "round2") + routing = ({"harness_member": member, "model_override": member.model} + if member is not None else {}) + round_kwargs = {**common, "step_name": "agent.review_diff#round2", + "guidance": common["guidance"] + "\n\n## SECOND ROUND\n" + + guidance, + "evidence": {**common["evidence"], + "uncovered_hunk_diffs": hunk_evidence, + "kept_comments": json.dumps( + kept, ensure_ascii=False)}} + result, extra = await run_agent_step( + ctx, **round_kwargs, + max_iters=ctx.settings.review_second_round_max_iters, **routing) + if routing and _seat_produced_nothing(result, extra): + # A routed second round has no retry of its own, so a transport + # failure on that backend silently deleted the whole coverage pass: + # measured 2026-08-16, a Fable-5 quota exhaustion left 16 of 20 + # holdout items reporting "8 uncovered hunks, 0 added" — the pass + # that exists to close coverage holes contributed nothing, invisibly. + # Fall back to the run's own backend and say so. + ctx.trace.record("capability_gap", capability="review.routed_seat", + step="agent.review_diff#round2", + effect="routed second round produced no output; " + "retrying on the run's default backend") + result, extra = await run_agent_step( + ctx, **{**round_kwargs, + "step_name": "agent.review_diff#round2/fallback"}, + max_iters=ctx.settings.review_second_round_max_iters) + if not result.ok and not (extra or {}).get("review_comments"): + ctx.trace.record("review_second_round", step="agent.review_diff", + uncovered=len(uncovered), added_comments=0, + added_findings=0, failed=True) + return output + new_comments = [] + for c in (extra or {}).get("review_comments") or []: + if not isinstance(c, dict) or not c.get("comment"): + continue + # near-dup guard: same file + line within 8 of an existing comment + dup = any(str(c.get("file") or "") == str(e.get("file") or "") + and isinstance(c.get("line"), int) + and isinstance(e.get("line"), int) + and abs(c["line"] - e["line"]) <= 8 + for e in (output.get("review_comments") or [])) + if not dup: + new_comments.append(c) + new_findings = [str(f) for f in (extra or {}).get("findings") or [] + if str(f).lstrip().lower().startswith( + ("[validated]", "[resolved]", "[claim-", "[sweep]", + "[upstream-verify]"))] + out2 = dict(output) + if new_comments: + out2["review_comments"] = list( + output.get("review_comments") or []) + new_comments + if new_findings: + out2["findings"] = list(output.get("findings") or []) + new_findings + ctx.trace.record("review_second_round", step="agent.review_diff", + uncovered=len(uncovered), + claims_unchecked=claims_unchecked, + added_comments=len(new_comments), + added_findings=len(new_findings)) + return out2 + + +_VERIFY_GUIDANCE = """You verify ONE draft review comment against the PR-time tree. + +In order, with the minimum tool calls (your budget is small): +1. Anchor: does the cited file:line (or quoted snippet) exist as the comment + claims? Read that region of the file. +2. Claim: is the asserted problem true of the code you just read? If the + comment asserts consumers/callers/tests elsewhere, grep for them and read + the one that decides the claim. +3. Severity: major requires a real defect in the changed code or a required + update the diff lacks; "consider adding X" polish is minor at most. + +Verdicts: +- confirmed: you READ the code that makes the claim true. You MUST return + `evidence` as SELF-CONTAINED PROOF a reader with no repo access can check: + the decisive code line(s) QUOTED VERBATIM with their file:line, e.g. + 'serving_speech.py:3711 `extra_args["tts_local_seed"] = seed` — set for + every model, no qwen3_tts gate'. A narrative like "read the file, claim + holds" is NOT proof and scores as speculation downstream. When the + decisive code lies OUTSIDE the diff (a consumer, a sibling platform, a CI + lane rule), say so explicitly in the evidence — 'unchanged by this diff, + present in the PR-time tree: `quote`' — a diff-only reader + must see why the quoted line is not in the diff, or the finding reads as + fabrication. Optionally + return a tightened `comment` (sharper wording, exact file/line) — keep + the substance, never soften a confirmed defect. +- refuted: the code CONTRADICTS the claim (misread, already handled, wrong + file). Refuted is an evidence conclusion, never a budget one. +- unverifiable: you could not decide within budget. + +Confirming from plausibility alone is the one failure this pass exists to +prevent — when in doubt between confirmed and unverifiable, unverifiable.""" + + +async def _verify_comments(ctx: StepContext, output: dict, + common: dict) -> dict: + """Per-comment agentic verification (val-gate lesson: with recall at + parity the arm lost on per-comment grounding). Each draft comment gets + one small tool-loop that must anchor and re-derive the claim on the + PR-time tree: refuted comments drop, unverifiable ones demote one + severity step, confirmed ones may be tightened in wording/position. A + verification-step failure keeps the comment unchanged — this pass may + only improve precision, never silently delete recall.""" + from ...agent_runtime import run_agent_step + + comments = list(output.get("review_comments") or []) + if not comments or not ctx.settings.review_verify_comments: + return output + sem = asyncio.Semaphore(ctx.settings.review_verify_concurrency) + diff_ev = str((common.get("evidence") or {}).get("pr_diff") or "") + + async def _one(i: int, c: dict): + async with sem: + # pr_diff leads the evidence pack so the big block is byte- + # identical across the fan-out and rides the provider cache + probe = {k: v for k, v in c.items() if k != "corroborated_by"} + result, out = await run_agent_step( + ctx, step_name=f"agent.verify_comment#{i}", + purpose="Verify one draft review comment against the " + "PR-time tree.", + guidance=_VERIFY_GUIDANCE, + expected="verdict confirmed|refuted|unverifiable, with " + "optional comment/line/severity corrections", + evidence={"pr_diff": diff_ev, + "draft_comment": json.dumps( + probe, ensure_ascii=False)}, + output_extension={ + "verdict": "confirmed|refuted|unverifiable", + "comment": "optional tightened rewrite", + "line": "optional corrected line number", + "severity": "optional corrected severity", + "evidence": "confirmed only: verbatim-quoted decisive " + "code line(s) with file:line — proof a " + "repo-less reader can check"}, + extra_tools=common.get("extra_tools"), + max_iters=ctx.settings.review_verify_max_iters) + return i, result, (out or {}) + + results = await asyncio.gather(*(_one(i, c) + for i, c in enumerate(comments))) + demote = {"blocker": "major", "major": "minor", "minor": "nit"} + kept: list[dict] = [] + n_drop = n_demote = 0 + for i, result, out in sorted(results, key=lambda r: r[0]): + c = dict(comments[i]) + verdict = str(out.get("verdict") or "").lower() + if not result.ok or verdict not in ("confirmed", "refuted", + "unverifiable"): + kept.append(c) # fail-open: never delete on pass failure + continue + if verdict == "refuted": + n_drop += 1 + continue + if verdict == "confirmed": + c["_verified"] = True # budget/overflow: a confirmed comment's + # tail placement is evidence, not noise — see the overflow gate + if out.get("comment"): + c["comment"] = str(out["comment"]) + if isinstance(out.get("line"), int): + c["line"] = out["line"] + sev = str(out.get("severity") or "").lower() + if sev in _SEVERITY_ORDER: + c["severity"] = sev + if out.get("evidence"): + # replace the drafting-stage narrative with the verifier's + # quoted proof: the review is judged by readers with no repo + # access, and a claim whose evidence they can check from the + # quote alone is the difference between "grounded" and + # "speculative" (precision sat at .55 across five configs + # until the rendered evidence became self-proving) + c["evidence"] = str(out["evidence"]) + else: # unverifiable: keep, one step down + s = str(c.get("severity", "minor")).lower() + c["severity"] = demote.get(s, "nit") + n_demote += 1 + kept.append(c) + ctx.trace.record("review_comments_verified", step="agent.review_diff", + total=len(comments), dropped=n_drop, demoted=n_demote) + out2 = dict(output) + out2["review_comments"] = kept + return out2 + + +async def refine_review( + ctx: StepContext, output: dict, *, common: dict, diff: str, + spec: dict, depth: str, +) -> dict: + """Run the bounded post-draft passes in their evidence-preserving order. + + A successful non-light draft first re-files its own verified residuals, + then mines uncovered findings and hunks. Every depth, including light, + verifies the resulting comments before the handler applies its final + disposition and comment budget. + """ + if depth != "light": + output = _promote_resolved_residuals(ctx, output) + output = await _promote_uncovered(ctx, output, spec) + output = await _second_round(ctx, output, common, diff) + return await _verify_comments(ctx, output, common) diff --git a/src/infermatrix_copilot/engine/steps/review/steps.py b/src/infermatrix_copilot/engine/steps/review/steps.py index 7f80cd930..0d3df6cfa 100644 --- a/src/infermatrix_copilot/engine/steps/review/steps.py +++ b/src/infermatrix_copilot/engine/steps/review/steps.py @@ -18,8 +18,6 @@ from dataclasses import replace from pathlib import Path -from ....llm import parse_json_reply - from ....review.planner import DEFAULT_STANDARD_LENSES, DEPTHS, plan_review from ...step import FailureKind, StepContext, StepResult from .._common import gh_read_tools as _gh_read_tools @@ -27,6 +25,7 @@ from .._common import step from .anchor import resolve_review_comments from .repo_tools import review_repo_tools +from .refinement import refine_review from .prompts import ( _REVIEW_DEEP_PASSES, _REVIEW_DOCS_PASS, @@ -42,7 +41,6 @@ _review_verdict, disposition_records, finalize_review_dispositions, - _same_finding, _sweep_targets, ) @@ -96,494 +94,6 @@ def _consumer_sweep(repo: str | None, diff: str) -> str: "is where breakage hides:\n" + "\n".join(out)) -_RESIDUAL_MARKERS = ("residual", "not covered", "does not cover", "still ", - "remains ", "left unfixed", "but ") - - -def _promote_resolved_residuals(ctx: StepContext, output: dict) -> dict: - """Turn every `[resolved]` findings line that states a RESIDUAL into a - review comment. - - Measured across two holdouts: the ground truth on merged/amended heads is - ~70% "a reviewer raised X, the fix landed — what does it still not - cover?", and the passes DO produce that reasoning (the `[resolved]` - contract in prompts.py asks for exactly it). But `[resolved]` is a - FINDINGS line, and findings render into the unscored 'Validated' block — - so the arm's best answers to the dominant question class were routed away - from the only channel a reader (or judge) scores. This promotion is - grounded by construction: it re-files a line the run already wrote and - verified, so it cannot invent a claim, and it sits outside the - coverage-promotion cap because it is not a discretionary addition.""" - findings = [str(f) for f in (output.get("findings") or [])] - comments = list(output.get("review_comments") or []) - covered = {(str(c.get("file") or ""), str(c.get("comment") or "")[:60]) - for c in comments} - added: list[dict] = [] - for line in findings: - low = line.lstrip().lower() - if not low.startswith("[resolved]"): - continue - body = line.split("]", 1)[-1].strip() - if not any(m in body.lower() for m in _RESIDUAL_MARKERS): - continue # a bare confirmation is not a finding - m = re.search(r"([\w./\-]+\.\w+):(\d+)", body) - file_, line_no = (m.group(1), int(m.group(2))) if m else ("", None) - if (file_, body[:60]) in covered: - continue - # ...and against EACH OTHER. `covered` only holds the pre-existing - # comments, so N resolved lines about one residual promoted N times: - # pr4977 shipped four near-identical "the PR description still claims - # trust_remote_code" comments (the cap of 4, saturated) out of five - # `[resolved]` lines stating that one residual, and the judge docked - # it for "4 nearly-identical inline comments restating the same - # PR-description staleness point, hurting signal density". Promotion - # is where that duplication is cheapest to stop — before these - # compete for the comment budget against distinct findings. - if any(_same_finding(body, prev["comment"]) for prev in added): - continue - added.append({"file": file_, "line": line_no, "severity": "minor", - "comment": body, - "evidence": line.strip(), - "corroborated_by": ["resolved-residual"]}) - if not added: - ctx.trace.record("review_resolved_promoted", step="agent.review_diff", - added=0, resolved_lines=sum( - 1 for f in findings - if f.lstrip().lower().startswith("[resolved]"))) - return output - out = dict(output) - out["review_comments"] = comments + added[:4] - ctx.trace.record("review_resolved_promoted", step="agent.review_diff", - added=len(added[:4])) - return out - - -async def _promote_uncovered(ctx: StepContext, output: dict, - spec: dict) -> dict: - """Coverage-promotion pass: one tool-less LLM call that promotes - maintainer-relevant concerns ALREADY PRESENT in the run's own findings/ - validated/blockers lines into review comments. - - Train forensics showed the recall loss is often not investigation but - conversion: the ground-truth concern sat verbatim in a lens's findings or - the run's blockers ("the VoxCPM2 regression is a separate mechanism", - "NPU lane is red", "diff evidence was truncated") and never became a - comment. Promotion is grounded by construction — the call may only cite - material from the lines it is shown, never new claims — so it raises - recall without the speculation cost of widening the lens budget.""" - findings = [str(x) for x in (output.get("findings") or [])][:60] - blockers = [str(x) for x in (output.get("blockers") or [])][:10] - assumptions = [str(x) for x in (output.get("assumptions") or [])][:10] - comments = output.get("review_comments") or [] - if not (findings or blockers) or ctx.llm is None \ - or not getattr(ctx.llm, "available", False): - ctx.trace.record("review_coverage_skipped", - step="agent.review_diff", - reason="no findings" if not (findings or blockers) - else "no llm") - return output - system = ( - "You are the coverage editor for a PR review. You receive the " - "review's KEPT COMMENTS plus the reviewers' raw FINDINGS/BLOCKERS/" - "ASSUMPTIONS lines. Identify up to 3 maintainer-relevant concerns " - "that appear in the raw lines but are NOT covered by any kept " - "comment, and promote each into a comment object. Prioritize these " - "classes: blast-radius of a changed default/shared value, benchmark " - "evidence for perf/capacity changes, test integrity (a test that " - "cannot fail or is never selected), dependency-version " - "compatibility, resource lifecycle on abort paths, scope explicitly " - "left unfixed (linked-issue remainder), red CI on the head, " - "machinery duplicating a named existing helper (sibling contrast), " - "a refuted PR-body claim, and — MOST OFTEN MISSED — GUARANTEE-GAP " - "RESIDUE hiding in the [validated]/[claim-verified]/[sweep] lines: " - "any verified fact that records a WEAKER guarantee than the PR " - "needs (a mock that authors the very value the test asserts, a " - "single-variant validation of a multi-variant feature, a gate " - "relocated to a slower lane, 'proves route propagation only', " - "'covered by weekly only') is a FINDING wearing a validation " - "stamp — promote it as the pointed question or ask it implies. " - "Rules: " - "(1) ONLY promote what the raw lines already state — no new claims, " - "no re-investigation; quote the source line in `evidence`. (2) A " - "promoted line KEEPS its directive force: a concern naming a " - "concrete defect, missing update, or duplicated machinery is " - "phrased as the change to make (name both files and the helper), " - "NEVER as a 'could you confirm…?' question; only a validation with " - "a genuinely unresolved residual becomes a scoping ask. (3) " - "severity: minor unless the raw line " - "demonstrates breakage (then major). (4) If everything relevant is " - "already covered, return no additions. Reply with exactly one JSON " - 'object: {"additions": [{"file": str, "line": int, "severity": ' - '"major"|"minor", "comment": str, "evidence": str}]}') - prompt = ( - "## KEPT COMMENTS\n" - + json.dumps([{k: c.get(k) for k in ("file", "line", "severity", - "comment")} - for c in comments], ensure_ascii=False, indent=1) - + "\n\n## RAW FINDINGS\n" + "\n".join(f"- {x}" for x in findings) - + ("\n\n## BLOCKERS\n" + "\n".join(f"- {x}" for x in blockers) - if blockers else "") - + ("\n\n## ASSUMPTIONS\n" + "\n".join(f"- {x}" for x in assumptions) - if assumptions else "")) - try: - _tt = ctx.settings.tier_target(spec.get("mode", "eco")) - llm = (ctx.llm.for_target(_tt) - if hasattr(ctx.llm, "for_target") else ctx.llm) - reply = await asyncio.to_thread( - llm.create, system=system, - messages=[{"role": "user", "content": prompt}], - model=ctx.settings.review_promotion_model or _tt.model, - role="reducer", - max_tokens=max(4096, ctx.settings.llm_max_tokens)) - obj = parse_json_reply(reply.text or "") - except Exception as exc: # never fail the review over the extra pass — - ctx.trace.record("review_coverage_skipped", # but say WHY, loudly - step="agent.review_diff", - reason=f"{type(exc).__name__}: {exc}"[:200]) - return output - additions = (obj or {}).get("additions") if isinstance(obj, dict) else None - kept: list[dict] = [] - for a in (additions or [])[:3]: - if not isinstance(a, dict) or not a.get("comment") \ - or not a.get("evidence"): - continue - # mechanical near-dup guard vs kept comments: the prompt's "NOT - # covered by any kept comment" rule was observed re-adding a - # merged-away duplicate (wave-3 pr6049 — the reducer collapsed two - # candidates, promotion re-added the merged-away variant, judges - # penalized the pair). Nearby anchor alone is not duplication (a - # linked-issue remainder often anchors beside the defect), so the - # guard also requires substantial text overlap. - def _words(c): - return set(str(c.get("comment") or "").lower().split()) - aw = _words(a) - if aw and any( - str(a.get("file") or "") == str(c.get("file") or "") - and isinstance(a.get("line"), int) - and isinstance(c.get("line"), int) - and abs(a["line"] - c["line"]) <= 8 - and len(aw & _words(c)) >= max(4, len(aw) // 2) - for c in comments): - continue - kept.append({"file": str(a.get("file") or "?"), - "line": a.get("line"), - "severity": str(a.get("severity") or "minor").lower(), - "comment": str(a["comment"]), - "evidence": str(a["evidence"]), - # ordering tag: promoted items carry a protected-class - # concern; under the comment budget they rank ahead of - # uncorroborated same-severity items (stripped at cap) - "corroborated_by": ["coverage"]}) - if kept: - output = dict(output) - output["review_comments"] = list(comments) + kept - ctx.trace.record("review_coverage_promoted", - step="agent.review_diff", added=len(kept)) - else: - ctx.trace.record("review_coverage_skipped", - step="agent.review_diff", - reason="model returned no valid additions", - parsed=obj is not None, - reply_head=str(reply.text or "")[:200]) - return output - - -def _uncovered_hunks(diff: str, output: dict) -> list[str]: - """Hunk clusters with no comment anchored near them and no findings line - citing a nearby file:line — the second round's coverage seed, at the - granularity GT actually has. v14's file-level seed measured too coarse on - the wave-3 gate: one comment anywhere in a file marked the whole file - covered, while the human GT this is judged against is per-hunk inline - comments (the losses concentrated on multi-hunk files whose comments - clustered on one region). Test files included: an uncovered test hunk is - where test-integrity findings hide. Returns `path:start` entries.""" - import re as _re - - regions: dict[str, list[int]] = {} - current = None - for line in str(diff or "").splitlines(): - if line.startswith("+++ b/"): - current = line[6:] - elif line.startswith("@@") and current: - m = _re.search(r"\+(\d+)", line) - if m: - regions.setdefault(current, []).append(int(m.group(1))) - comments = output.get("review_comments") or [] - cited: dict[str, list[int]] = {} - for f in output.get("findings") or []: - # Only CLAIM/RESOLVED anchors count as coverage. Blanket - # [sweep]/[validated] stamps do not: wave-3 forensics found them to - # be false negatives on exactly the GT zones ("TP group build … - # verified" where the crash was), and their density suppressed the - # second round on every hunk it could have rescued (pr5678: skipped - # with four missed-GT hunks "covered" by self-issued stamps). - low = str(f).lstrip().lower() - if not low.startswith(("[claim-", "[resolved]")): - continue - for base, num in _re.findall(r"([\w.\-]+\.\w+):(\d+)", str(f)): - cited.setdefault(base, []).append(int(num)) - out: list[str] = [] - for path, starts in regions.items(): - base = path.rsplit("/", 1)[-1] - file_comments = [c.get("line") for c in comments - if str(c.get("file") or "").endswith(base) - and isinstance(c.get("line"), int)] - file_cited = cited.get(base, []) - has_any_comment = any(str(c.get("file") or "").endswith(base) - for c in comments) - for s in starts: - near = any(abs(n - s) <= 40 for n in file_comments + file_cited) - # a file-level comment (no line) covers a single-hunk file only - if not near and not (has_any_comment and len(starts) == 1): - out.append(f"{path}:{s}") - return out - - -async def _second_round(ctx: StepContext, output: dict, common: dict, - diff: str) -> dict: - """Coverage-driven second investigation round (RFC q3): one bounded pass - seeded by the run's own coverage holes, instead of a bigger fixed pass - count. Additions face the same verify pass as first-round comments.""" - from ...agent_runtime import run_agent_step - - if not ctx.settings.review_second_round: - return output - # 8 seeds, not 14: a 14-seed round-2 was measured producing finals too - # large to coerce on the wave-4 big items (both 14-seed runs failed, - # both sub-10-seed runs succeeded) — fewer, deeper hunk visits beat a - # sweep that never files - uncovered = _uncovered_hunks(diff, output)[:8] - claims_unchecked = "[claim-" not in " ".join( - str(f) for f in (output.get("findings") or [])) - if len(uncovered) < ctx.settings.review_second_round_min_files \ - and not claims_unchecked: - ctx.trace.record("review_second_round", step="agent.review_diff", - skipped="no coverage holes", uncovered=0) - return output - kept = [{k: c.get(k) for k in ("file", "line", "severity", "comment")} - for c in (output.get("review_comments") or [])] - guidance = ( - "You are the SECOND-ROUND reviewer. The first round produced the " - "KEPT COMMENTS in your evidence; your job is ONLY its coverage " - "holes — do not re-litigate or duplicate what is already covered.\n" - + (("These diff hunks (file:start-line) have NO comment and NO " - "recorded verification near them. Review each AT MAINTAINER " - "INLINE GRANULARITY: page to the hunk in situ, and either raise " - "one specific localized comment (the concrete ask a human " - "reviewer would leave on that hunk) or record a one-line " - "[validated]/[resolved] findings entry naming file:line — " - "silence on a changed hunk is not an option:\n" - + "\n".join(f"- {p}" for p in uncovered) + "\n") - if uncovered else "") - + (("The PR body's checkable claims were never verified (no " - "[claim-*] findings line exists). Build the claim ledger now: " - "verify or refute each checkable body claim (checklist 13).\n") - if claims_unchecked else "") - + "Every comment needs verbatim-quoted evidence with file:line. " - "Budget discipline: reserve your last round for the output " - "contract.") - # per-file diff slices for the seed files: on big PRs the shared pr_diff - # evidence is CAPPED and the tail files' hunks never reach any pass — - # wave-3 pr5691's round-2 read the right 10 files but held the same - # truncated diff blob, so the beyond-cap hunks stayed unreviewable. The - # slices come from the UNCAPPED diff text. - seed_paths = {p.rsplit(":", 1)[0] for p in uncovered} - slices: list[str] = [] - for chunk in re.split(r"(?m)^(?=diff --git )", str(diff or "")): - m = re.search(r"^\+\+\+ b/(.+)$", chunk, re.M) - if m and m.group(1) in seed_paths: - slices.append(chunk) - hunk_evidence = "".join(slices)[:100_000] - from ...agent_runtime.ensemble import ( - lens_backend_member, - outcome_blocked as _seat_produced_nothing, - ) - - member = lens_backend_member(ctx.settings, "round2") - routing = ({"harness_member": member, "model_override": member.model} - if member is not None else {}) - round_kwargs = {**common, "step_name": "agent.review_diff#round2", - "guidance": common["guidance"] + "\n\n## SECOND ROUND\n" - + guidance, - "evidence": {**common["evidence"], - "uncovered_hunk_diffs": hunk_evidence, - "kept_comments": json.dumps( - kept, ensure_ascii=False)}} - result, extra = await run_agent_step( - ctx, **round_kwargs, - max_iters=ctx.settings.review_second_round_max_iters, **routing) - if routing and _seat_produced_nothing(result, extra): - # A routed second round has no retry of its own, so a transport - # failure on that backend silently deleted the whole coverage pass: - # measured 2026-08-16, a Fable-5 quota exhaustion left 16 of 20 - # holdout items reporting "8 uncovered hunks, 0 added" — the pass - # that exists to close coverage holes contributed nothing, invisibly. - # Fall back to the run's own backend and say so. - ctx.trace.record("capability_gap", capability="review.routed_seat", - step="agent.review_diff#round2", - effect="routed second round produced no output; " - "retrying on the run's default backend") - result, extra = await run_agent_step( - ctx, **{**round_kwargs, - "step_name": "agent.review_diff#round2/fallback"}, - max_iters=ctx.settings.review_second_round_max_iters) - if not result.ok and not (extra or {}).get("review_comments"): - ctx.trace.record("review_second_round", step="agent.review_diff", - uncovered=len(uncovered), added_comments=0, - added_findings=0, failed=True) - return output - new_comments = [] - for c in (extra or {}).get("review_comments") or []: - if not isinstance(c, dict) or not c.get("comment"): - continue - # near-dup guard: same file + line within 8 of an existing comment - dup = any(str(c.get("file") or "") == str(e.get("file") or "") - and isinstance(c.get("line"), int) - and isinstance(e.get("line"), int) - and abs(c["line"] - e["line"]) <= 8 - for e in (output.get("review_comments") or [])) - if not dup: - new_comments.append(c) - new_findings = [str(f) for f in (extra or {}).get("findings") or [] - if str(f).lstrip().lower().startswith( - ("[validated]", "[resolved]", "[claim-", "[sweep]", - "[upstream-verify]"))] - out2 = dict(output) - if new_comments: - out2["review_comments"] = list( - output.get("review_comments") or []) + new_comments - if new_findings: - out2["findings"] = list(output.get("findings") or []) + new_findings - ctx.trace.record("review_second_round", step="agent.review_diff", - uncovered=len(uncovered), - claims_unchecked=claims_unchecked, - added_comments=len(new_comments), - added_findings=len(new_findings)) - return out2 - - -_VERIFY_GUIDANCE = """You verify ONE draft review comment against the PR-time tree. - -In order, with the minimum tool calls (your budget is small): -1. Anchor: does the cited file:line (or quoted snippet) exist as the comment - claims? Read that region of the file. -2. Claim: is the asserted problem true of the code you just read? If the - comment asserts consumers/callers/tests elsewhere, grep for them and read - the one that decides the claim. -3. Severity: major requires a real defect in the changed code or a required - update the diff lacks; "consider adding X" polish is minor at most. - -Verdicts: -- confirmed: you READ the code that makes the claim true. You MUST return - `evidence` as SELF-CONTAINED PROOF a reader with no repo access can check: - the decisive code line(s) QUOTED VERBATIM with their file:line, e.g. - 'serving_speech.py:3711 `extra_args["tts_local_seed"] = seed` — set for - every model, no qwen3_tts gate'. A narrative like "read the file, claim - holds" is NOT proof and scores as speculation downstream. When the - decisive code lies OUTSIDE the diff (a consumer, a sibling platform, a CI - lane rule), say so explicitly in the evidence — 'unchanged by this diff, - present in the PR-time tree: `quote`' — a diff-only reader - must see why the quoted line is not in the diff, or the finding reads as - fabrication. Optionally - return a tightened `comment` (sharper wording, exact file/line) — keep - the substance, never soften a confirmed defect. -- refuted: the code CONTRADICTS the claim (misread, already handled, wrong - file). Refuted is an evidence conclusion, never a budget one. -- unverifiable: you could not decide within budget. - -Confirming from plausibility alone is the one failure this pass exists to -prevent — when in doubt between confirmed and unverifiable, unverifiable.""" - - -async def _verify_comments(ctx: StepContext, output: dict, - common: dict) -> dict: - """Per-comment agentic verification (val-gate lesson: with recall at - parity the arm lost on per-comment grounding). Each draft comment gets - one small tool-loop that must anchor and re-derive the claim on the - PR-time tree: refuted comments drop, unverifiable ones demote one - severity step, confirmed ones may be tightened in wording/position. A - verification-step failure keeps the comment unchanged — this pass may - only improve precision, never silently delete recall.""" - from ...agent_runtime import run_agent_step - - comments = list(output.get("review_comments") or []) - if not comments or not ctx.settings.review_verify_comments: - return output - sem = asyncio.Semaphore(ctx.settings.review_verify_concurrency) - diff_ev = str((common.get("evidence") or {}).get("pr_diff") or "") - - async def _one(i: int, c: dict): - async with sem: - # pr_diff leads the evidence pack so the big block is byte- - # identical across the fan-out and rides the provider cache - probe = {k: v for k, v in c.items() if k != "corroborated_by"} - result, out = await run_agent_step( - ctx, step_name=f"agent.verify_comment#{i}", - purpose="Verify one draft review comment against the " - "PR-time tree.", - guidance=_VERIFY_GUIDANCE, - expected="verdict confirmed|refuted|unverifiable, with " - "optional comment/line/severity corrections", - evidence={"pr_diff": diff_ev, - "draft_comment": json.dumps( - probe, ensure_ascii=False)}, - output_extension={ - "verdict": "confirmed|refuted|unverifiable", - "comment": "optional tightened rewrite", - "line": "optional corrected line number", - "severity": "optional corrected severity", - "evidence": "confirmed only: verbatim-quoted decisive " - "code line(s) with file:line — proof a " - "repo-less reader can check"}, - extra_tools=common.get("extra_tools"), - max_iters=ctx.settings.review_verify_max_iters) - return i, result, (out or {}) - - results = await asyncio.gather(*(_one(i, c) - for i, c in enumerate(comments))) - demote = {"blocker": "major", "major": "minor", "minor": "nit"} - kept: list[dict] = [] - n_drop = n_demote = 0 - for i, result, out in sorted(results, key=lambda r: r[0]): - c = dict(comments[i]) - verdict = str(out.get("verdict") or "").lower() - if not result.ok or verdict not in ("confirmed", "refuted", - "unverifiable"): - kept.append(c) # fail-open: never delete on pass failure - continue - if verdict == "refuted": - n_drop += 1 - continue - if verdict == "confirmed": - c["_verified"] = True # budget/overflow: a confirmed comment's - # tail placement is evidence, not noise — see the overflow gate - if out.get("comment"): - c["comment"] = str(out["comment"]) - if isinstance(out.get("line"), int): - c["line"] = out["line"] - sev = str(out.get("severity") or "").lower() - if sev in _SEVERITY_ORDER: - c["severity"] = sev - if out.get("evidence"): - # replace the drafting-stage narrative with the verifier's - # quoted proof: the review is judged by readers with no repo - # access, and a claim whose evidence they can check from the - # quote alone is the difference between "grounded" and - # "speculative" (precision sat at .55 across five configs - # until the rendered evidence became self-proving) - c["evidence"] = str(out["evidence"]) - else: # unverifiable: keep, one step down - s = str(c.get("severity", "minor")).lower() - c["severity"] = demote.get(s, "nit") - n_demote += 1 - kept.append(c) - ctx.trace.record("review_comments_verified", step="agent.review_diff", - total=len(comments), dropped=n_drop, demoted=n_demote) - out2 = dict(output) - out2["review_comments"] = kept - return out2 - - @step("agent.review_diff", "agent", "read", "Evidence-grounded two-stage review: tool-loop investigation draft, " "then verify-and-rewrite editor pass.") @@ -768,34 +278,11 @@ async def _review_diff(ctx: StepContext) -> StepResult: f"{result.summary}", outputs=result.outputs, changed_files=result.changed_files) - if plan is not None and plan.depth != "light" and result.ok: - # coverage promotion mines the run's own findings/blockers for - # uncovered protected-class concerns; runs AFTER the salvage (an - # ensemble that self-reported needs_review still carries findings - # worth mining — gating on the pre-salvage status silently disabled - # the pass on exactly the GT-rich items it exists for) and BEFORE - # the verify pass, so promoted items face the same scrutiny - # resolved-residual promotion FIRST: it re-files reasoning the run - # already verified, so it should be visible to the coverage editor - # (which is told not to duplicate kept comments) rather than compete - # with it for the cap of 3 - output = _promote_resolved_residuals(ctx, output) - output = await _promote_uncovered(ctx, output, spec) - # coverage-driven second round (RFC q3): changed files nobody wrote a - # line about seed ONE bounded extra pass — BEFORE the verify pass, so - # second-round comments face the same scrutiny as first-round ones - output = await _second_round(ctx, output, common, str(diff)) if plan is not None and result.ok: - # per-comment agentic verification: every surviving draft comment is - # checked against the PR-time tree by a small tool-loop before the - # budget — the measured loss driver after the recall fixes was - # per-comment grounding (val: arm precision .54-.56 vs baseline - # .65 with recall at parity), which packaging cannot buy. LIGHT - # depth verifies too (v14 train probe: the thin items where light - # runs were exactly where unverified marginal comments lost on - # precision — a wrong sibling-doc ask a verify probe would have - # refuted shipped unchecked); on ≤3 comments the pass costs cents - output = await _verify_comments(ctx, output, common) + output = await refine_review( + ctx, output, common=common, diff=str(diff), spec=spec, + depth=plan.depth, + ) # deterministic comment budget: severity-ordered, corroboration-aware, # capped at 8 — the low-signal tail goes first (reducers ignored a # prompted cap; the cap is a product budget, so it applies to every diff --git a/test/test_agent_ensemble.py b/test/test_agent_ensemble.py index f7dc377ec..cd246ba04 100644 --- a/test/test_agent_ensemble.py +++ b/test/test_agent_ensemble.py @@ -1164,7 +1164,7 @@ def test_resolved_residual_becomes_a_comment(settings, trace, tmp_path): comment. Ground truth on merged/amended heads is ~70% "the fix landed — what does it still not cover?", and the passes produce exactly that reasoning; it was rendering into the unscored Validated block.""" - from infermatrix_copilot.engine.steps.review.steps import ( + from infermatrix_copilot.engine.steps.review.refinement import ( _promote_resolved_residuals, ) out = _promote_resolved_residuals( diff --git a/test/test_review_render.py b/test/test_review_render.py index 1545ad89e..7a99b7b9b 100644 --- a/test/test_review_render.py +++ b/test/test_review_render.py @@ -162,7 +162,7 @@ def test_promoted_residuals_are_deduped_against_each_other(): PR-description-staleness comments, saturating the cap of 4.""" import asyncio from types import SimpleNamespace - from infermatrix_copilot.engine.steps.review.steps import ( + from infermatrix_copilot.engine.steps.review.refinement import ( _promote_resolved_residuals) ctx = SimpleNamespace(trace=SimpleNamespace(record=lambda *a, **k: None)) out = _promote_resolved_residuals(ctx, {"findings": list(_TRC),