From 0143de8e1e2ee42ffe8f2605fd7146f563e01cc0 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Fri, 2 Oct 2026 19:16:26 +0800 Subject: [PATCH 1/2] Fix post-reduction review actionability and final summary consistency --- .../engine/agent_runtime/ensemble.py | 9 +++ .../engine/steps/review/refinement.py | 52 ++++++++++--- .../engine/steps/review/steps.py | 21 ++++- .../engine/steps/review/utils.py | 3 +- test/test_agent_ensemble.py | 76 +++++++++++++++++++ 5 files changed, 145 insertions(+), 16 deletions(-) diff --git a/src/infermatrix_copilot/engine/agent_runtime/ensemble.py b/src/infermatrix_copilot/engine/agent_runtime/ensemble.py index 906605cd6..44fe7c5e6 100644 --- a/src/infermatrix_copilot/engine/agent_runtime/ensemble.py +++ b/src/infermatrix_copilot/engine/agent_runtime/ensemble.py @@ -603,6 +603,15 @@ def _ok(obj) -> bool: kept[i]["severity"] = str(v["severity"]) merged["summary"] = str(reduced.get("summary") or merged.get("summary") or "") + if merge_key == "review_comments": + # Later coverage passes need the selection decisions too. Raw + # findings alone can otherwise resurrect a concern the reducer + # already established was harmless or outside this change. + merged["_rejected_review_comments"] = [ + {"candidate": candidates[i], "reason": str(v.get("why") or "")} + for i, v in by_i.items() + if str(v.get("action", "")).lower() == "drop" + ] else: merged["summary"] = (f"ensemble of {len(samples)} samples " "(merge reduction failed; unverified union)") diff --git a/src/infermatrix_copilot/engine/steps/review/refinement.py b/src/infermatrix_copilot/engine/steps/review/refinement.py index b1735e4a1..9af2b8832 100644 --- a/src/infermatrix_copilot/engine/steps/review/refinement.py +++ b/src/infermatrix_copilot/engine/steps/review/refinement.py @@ -66,6 +66,11 @@ async def _promote_uncovered(ctx: StepContext, output: dict, "'covered by weekly only') is a FINDING wearing a validation " "stamp — promote it as the pointed question or ask it implies. " "Rules: " + "A true code fact is not automatically a defect. Require a concrete " + "current trigger and adverse effect or an unmet requirement. Do not " + "promote harmless unused configuration or hypothetical future code " + "changes. Respect REJECTED COMMENTS and their reasons; reopen one " + "only with new evidence that defeats the rejection. " "(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 " @@ -84,6 +89,9 @@ async def _promote_uncovered(ctx: StepContext, output: dict, "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## REJECTED COMMENTS\n" + + json.dumps(output.get("_rejected_review_comments") or [], + ensure_ascii=False) + ("\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) @@ -229,6 +237,11 @@ async def _second_round(ctx: StepContext, output: dict, common: dict, "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" + "Respect rejected_comments and their reasons. Reopen a rejected " + "concern only with new evidence defeating that reason. A true code " + "fact needs a current trigger and adverse effect or an unmet " + "requirement to become a finding; harmless unused values and " + "hypothetical future changes are not defects.\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 " @@ -271,7 +284,10 @@ async def _second_round(ctx: StepContext, output: dict, common: dict, "evidence": {**common["evidence"], "uncovered_hunk_diffs": hunk_evidence, "kept_comments": json.dumps( - kept, ensure_ascii=False)}} + kept, ensure_ascii=False), + "rejected_comments": json.dumps( + output.get("_rejected_review_comments") + or [], ensure_ascii=False)}} result, extra = await run_agent_step( ctx, **round_kwargs, max_iters=ctx.settings.review_second_round_max_iters, **routing) @@ -333,13 +349,20 @@ async def _second_round(ctx: StepContext, output: dict, common: dict, 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 +3. Actionability: establish a concrete trigger on this PR head and its + adverse effect, or a specific unmet requirement. A code fact being true + does not establish a defect. Unused optional configuration without an + affected consumer, and a problem contingent on hypothetical future code + changes, are not actionable. Check rejected_comments before reopening + a concern; identify new evidence that defeats its rejection. +4. 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. + the current trigger and adverse effect (or unmet requirement), together + with 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 @@ -352,6 +375,9 @@ async def _second_round(ctx: StepContext, output: dict, common: dict, 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. +- not_actionable: the cited facts may be true, but no current defect or + unmet requirement is demonstrated. Explain why the proposed ask is not + needed on this head. This verdict drops the comment. - unverifiable: you could not decide within budget. Confirming from plausibility alone is the one failure this pass exists to @@ -363,7 +389,7 @@ async def _verify_comments(ctx: StepContext, output: 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 + PR-time tree: refuted and non-actionable 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.""" @@ -385,19 +411,23 @@ async def _one(i: int, c: dict): purpose="Verify one draft review comment against the " "PR-time tree.", guidance=_VERIFY_GUIDANCE, - expected="verdict confirmed|refuted|unverifiable, with " + expected="verdict confirmed|refuted|not_actionable|unverifiable, with " "optional comment/line/severity corrections", evidence={"pr_diff": diff_ev, "draft_comment": json.dumps( - probe, ensure_ascii=False)}, + probe, ensure_ascii=False), + "rejected_comments": json.dumps( + output.get("_rejected_review_comments") or [], + ensure_ascii=False)}, output_extension={ - "verdict": "confirmed|refuted|unverifiable", + "verdict": "confirmed|refuted|not_actionable|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"}, + "code line(s) with file:line, current trigger " + "and adverse effect or unmet requirement — " + "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 {}) @@ -411,10 +441,10 @@ async def _one(i: int, c: dict): c = dict(comments[i]) verdict = str(out.get("verdict") or "").lower() if not result.ok or verdict not in ("confirmed", "refuted", - "unverifiable"): + "not_actionable", "unverifiable"): kept.append(c) # fail-open: never delete on pass failure continue - if verdict == "refuted": + if verdict in ("refuted", "not_actionable"): n_drop += 1 continue if verdict == "confirmed": diff --git a/src/infermatrix_copilot/engine/steps/review/steps.py b/src/infermatrix_copilot/engine/steps/review/steps.py index 6feb44e27..2b8afc5d8 100644 --- a/src/infermatrix_copilot/engine/steps/review/steps.py +++ b/src/infermatrix_copilot/engine/steps/review/steps.py @@ -295,6 +295,10 @@ async def _review_diff(ctx: StepContext) -> StepResult: f"{result.summary}", outputs=result.outputs, changed_files=result.changed_files) + draft_selection = [ + (c.get("file"), c.get("line"), c.get("comment"), c.get("severity")) + for c in output.get("review_comments") or [] + ] if plan is not None and result.ok: output = await refine_review( ctx, output, common=common, diff=str(diff), spec=spec, @@ -320,10 +324,6 @@ async def _review_diff(ctx: StepContext) -> StepResult: published, withheld = finalize_review_dispositions(output) # at most three untested-function comments; the rest are named, not lost published, ut_dropped = cap_gap_comments(published, ut_report) - if ut_dropped: - output["summary"] = (str(output.get("summary") or "").rstrip() - + "\n\nAlso without a unit test: " - + ", ".join(f"`{n}`" for n in ut_dropped) + ".").lstrip() output["review_comments"] = published output["_withheld_findings"] = withheld comments = sorted(output.get("review_comments") or [], @@ -333,6 +333,19 @@ async def _review_diff(ctx: StepContext) -> StepResult: for c in comments: c.pop("corroborated_by", None) output["review_comments"] = comments[:8] + final_selection = [ + (c.get("file"), c.get("line"), c.get("comment"), c.get("severity")) + for c in output["review_comments"] + ] + if sorted(draft_selection, key=repr) != sorted(final_selection, key=repr): + # The reducer's prose describes its draft selection. After promotion, + # verification or a budget cut it may contradict the published set. + # Let the renderer derive a truthful summary from the final comments. + output["summary"] = "" + if ut_dropped: + output["summary"] = (str(output.get("summary") or "").rstrip() + + "\n\nAlso without a unit test: " + + ", ".join(f"`{n}`" for n in ut_dropped) + ".").lstrip() # Recorded AFTER the cut: a candidate the review chose to publish but the # budget dropped is `over_budget`, never `excluded`. A consumer checking # that no withheld finding was published must not trip over a healthy diff --git a/src/infermatrix_copilot/engine/steps/review/utils.py b/src/infermatrix_copilot/engine/steps/review/utils.py index 13258a292..0f606fa59 100644 --- a/src/infermatrix_copilot/engine/steps/review/utils.py +++ b/src/infermatrix_copilot/engine/steps/review/utils.py @@ -636,7 +636,8 @@ def _render_review_md(output: dict, pr_state: str = "") -> str: entry += f"\n\nProposed change (not a ready patch): {suggestion}" lines.append(entry) parts = _review_summary_parts(output) - parts.append("\n\n".join(lines) if lines else output.get("summary", "No findings.")) + parts.append("\n\n".join(lines) if lines else + (output.get("summary") or "No actionable findings.")) overflow = output.get("_review_overflow") or [] if overflow: # findings the comment budget cut — one line each, so a real (often diff --git a/test/test_agent_ensemble.py b/test/test_agent_ensemble.py index 3a86845fd..37028c419 100644 --- a/test/test_agent_ensemble.py +++ b/test/test_agent_ensemble.py @@ -438,6 +438,82 @@ def test_coverage_promotion_converts_findings_to_comments(settings, trace, assert len(kept) == 2 assert "remains unfixed" in kept[1]["comment"] assert next(trace.events("review_coverage_promoted"))["added"] == 1 + assert "2 actionable finding(s)." in result.outputs["review_summary"] + assert "merged" not in result.outputs["review_summary"] + assert not llm._replies + + +def test_rejected_facts_stay_available_and_non_actionable_additions_drop( + settings, trace, tmp_path, git_repo): + """A true unused-key observation must not become a published defect + merely because coverage revisits it after the reducer rejected it.""" + settings.review_ensemble = True + settings.review_depth = "full" + settings.review_second_round = True + settings.review_verify_comments = True + settings.review_verify_concurrency = 1 + harmless = {"file": "config.py", "line": 10, "severity": "minor", + "comment": "optional key also reaches an unused consumer", + "evidence": "config.py:10 `allowed |= {'model_config'}`"} + future = {"file": "config.py", "line": 40, "severity": "minor", + "comment": "validator may reject this if its fields change later", + "evidence": "config.py:40 `fields = current_fields()`"} + reason = "No current affected consumer; forwarding is harmless" + llm = ScriptedLLM([ + contract(review_comments=[harmless], + findings=["[validated] optional key is forwarded"]), + *[contract(review_comments=[])] * (len(_REVIEW_LENSES) - 1), + verdicts_reply({"i": 0, "action": "drop", "why": reason}, + summary="No actionable findings."), + Reply(blocks=[Block(type="text", text=json.dumps( + {"additions": [harmless]}))]), + contract(review_comments=[future]), + contract(verdict="not_actionable", evidence=reason), + contract(verdict="not_actionable", + evidence="current_fields excludes the key on this head"), + ]) + state = {"diff_text": "diff --git a/config.py b/config.py\n" + "+++ b/config.py\n@@ -1,0 +1,1 @@\n+allowed = set()", + "task_spec": {"pr": 9}, "repo_path": str(git_repo)} + result = asyncio.run(register_builtin_steps(StepRegistry()).get( + "agent.review_diff").handler( + _ctx(settings, trace, tmp_path, state, llm=llm))) + assert result.ok + assert result.outputs["review_comments"] == [] + assert result.outputs["review_summary"].endswith("**Verdict:** APPROVE") + assert "optional key also reaches" not in result.outputs["review_text"] + assert next(trace.events("review_comments_verified"))["dropped"] == 2 + # The decision survives every boundary, including the second round and + # independent verifiers, instead of only living in a reducer archive. + for call in llm.calls[-4:]: + assert reason in json.dumps(call["messages"]) + assert not llm._replies + + +def test_verification_removal_clears_stale_reducer_summary( + settings, trace, tmp_path, git_repo): + settings.review_ensemble = True + settings.review_depth = "full" + settings.review_verify_comments = True + llm = ScriptedLLM([ + contract(review_comments=[{"file": "config.py", "line": 1, + "severity": "major", "comment": "broken", + "evidence": "config.py:1"}]), + *[contract(review_comments=[])] * (len(_REVIEW_LENSES) - 1), + verdicts_reply({"i": 0, "action": "keep"}, + summary="One blocking defect remains."), + contract(verdict="refuted"), + ]) + state = {"diff_text": "diff --git a/config.py b/config.py\n+x = 1", + "task_spec": {"pr": 9}, "repo_path": str(git_repo)} + result = asyncio.run(register_builtin_steps(StepRegistry()).get( + "agent.review_diff").handler( + _ctx(settings, trace, tmp_path, state, llm=llm))) + assert result.ok + assert result.outputs["review_comments"] == [] + for field in ("review_text", "review_summary"): + assert "blocking defect remains" not in result.outputs[field] + assert "No actionable findings." in result.outputs[field] assert not llm._replies From dfc423e40d1a28b084f4f6131f1df56893ab30f8 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Fri, 2 Oct 2026 19:23:06 +0800 Subject: [PATCH 2/2] Document review refinement selection and actionability invariants --- doc/architecture/SPEC/engine/steps/review.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index b1ee0b60c..f06c2104e 100644 --- a/doc/architecture/SPEC/engine/steps/review.md +++ b/doc/architecture/SPEC/engine/steps/review.md @@ -1,6 +1,6 @@ # engine/steps/review/ —— 规范 - + `patch gate + PR review + quality（9 个源文件） · step 库（评审） · refactor-status: ok` @@ -49,6 +49,15 @@ agent、评审结果的有界精炼、评测调优过的 prompt 数据和确定 「Checked, no defect found」注记：保留证据，去掉诉求，不带优先级。 在此之前取舍只活在模型的 `summary` 散文里，而发布读的是列表，于是 summary 说要丢弃的请求照样行内发出（#141）。 +- **终局摘要不能复用过期取舍**：覆盖补充、第二轮、核验、disposition 或评论预算 + 改变评论的成员、主张、位置或严重度后，清除 reducer 的 draft summary， + 由最终可发布集合生成摘要；无发现时正文明确写 `No actionable findings.`。 +- **事实成立不等于需要改动**：覆盖补充、第二轮和逐评论核验都携带 ensemble + reducer 的拒绝候选及原因（`_rejected_review_comments`）；重新提出同一关切 + 必须有推翻原原因的新证据。核验须指出当前 head 的具体触发条件及坏结果， + 或明确未满足的要求。无影响的未使用可选值、依赖未来假设代码变更的问题 + 属于 `not_actionable`，与 `refuted` 一样丢弃；无法核验仍保留并降级， + 调用失败仍保留原评论。 - **`[resolved]` 是记录，不是诉求**：findings 里的 `[resolved]` 行从不变成评论。 残留问题若需要贡献者处理，由评审者自己另发一条 `publish` 评论（file:line、 要做的改动、`evidence` 引用该 `[resolved]` 行）；覆盖补充（coverage editor）