From 7e107fdf5c6568ce40819bcfa7df54a86c2483bb Mon Sep 17 00:00:00 2001 From: tzhouam Date: Tue, 29 Sep 2026 16:16:01 +0800 Subject: [PATCH] Stop publishing [resolved] lines; file residuals as their own comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A [resolved] findings line was re-filed as a P2 comment whenever its prose matched a residual keyword. The bare "but " marker, and plain "residual:", also matched lines that decline their point, so vllm-omni#6968 published "... (author follow-up) — not re-raised." as a finding. The coverage editor received the same lines as raw material and could do the same. A [resolved] line is now only the review's record. The reviewer prompt asks for a residual that needs action to be filed as its own comment with disposition publish, a file:line and the [resolved] line as evidence, which is the structured selection #154 introduced. The prose promotion is removed and the coverage editor no longer sees [resolved] lines. Tests use the verbatim #6968 line (never published) and the measured pr4977 residual filed as a publish comment (published). Fixes #155 Signed-off-by: tzhouam Co-Authored-By: Claude Opus 5.5 --- doc/architecture/SPEC/engine/steps/review.md | 7 +- .../engine/steps/review/prompts.py | 7 +- .../engine/steps/review/refinement.py | 72 ++----------------- test/test_agent_ensemble.py | 60 ++++++++++------ test/test_review_render.py | 32 +++++---- 5 files changed, 76 insertions(+), 102 deletions(-) diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index 84cee2a1..9897772d 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,11 @@ agent、评审结果的有界精炼、评测调优过的 prompt 数据和确定 「Checked, no defect found」注记：保留证据，去掉诉求，不带优先级。 在此之前取舍只活在模型的 `summary` 散文里，而发布读的是列表，于是 summary 说要丢弃的请求照样行内发出（#141）。 +- **`[resolved]` 是记录，不是诉求**：findings 里的 `[resolved]` 行从不变成评论。 + 残留问题若需要贡献者处理，由评审者自己另发一条 `publish` 评论（file:line、 + 要做的改动、`evidence` 引用该 `[resolved]` 行）；覆盖补充（coverage editor） + 的输入也剔除 `[resolved]` 行。此前按关键词从散文里“提拔”残留，会把明确 + 写着不再追究的行发成 P2（#155）。 - **`review_verdict` 是发布的 state 字段，不是散文**：由 `_review_verdict(review_comments, pr_state)` 计算（**与渲染器同一个 helper，绝不第二份校准规则**），随 `review_text`/`review_summary`/ diff --git a/src/infermatrix_copilot/engine/steps/review/prompts.py b/src/infermatrix_copilot/engine/steps/review/prompts.py index fbc7922a..0bdb970b 100644 --- a/src/infermatrix_copilot/engine/steps/review/prompts.py +++ b/src/infermatrix_copilot/engine/steps/review/prompts.py @@ -47,7 +47,12 @@ stating the residual you checked (what the fix does NOT cover): a fix that landed \ mid-PR usually narrows rather than closes, and reviewing its residue is the work. On \ amended/post-review heads these confirmations are most of what a reader checks the \ -review against; silence about a resolved concern reads as never having looked. \ +review against; silence about a resolved concern reads as never having looked. The \ +`[resolved]` line is the RECORD, never the ask, and is never published: when the \ +residual needs the contributor to act, ALSO file it as its own comment \ +(`disposition: publish`, file:line of the uncovered path, the change or test to \ +add, the `[resolved]` line quoted in `evidence`); a residual left only in a \ +findings line is not raised. \ CAUTION on absence claims: verify "X was removed/is absent" with `diff_stat`, but \ never render a bare "X is absent from the diff" assertion — on amended PRs the \ reader's thread may reference an earlier revision, and an absolute absence claim \ diff --git a/src/infermatrix_copilot/engine/steps/review/refinement.py b/src/infermatrix_copilot/engine/steps/review/refinement.py index 77181e0c..b1735e4a 100644 --- a/src/infermatrix_copilot/engine/steps/review/refinement.py +++ b/src/infermatrix_copilot/engine/steps/review/refinement.py @@ -13,69 +13,7 @@ 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 +from .utils import _SEVERITY_ORDER async def _promote_uncovered(ctx: StepContext, output: dict, @@ -91,7 +29,12 @@ async def _promote_uncovered(ctx: StepContext, output: dict, 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] + # `[resolved]` lines are the review's record of a concern it closed, not + # raw material: a residual that needs action is filed by the reviewer as + # its own `publish` comment (prompts.py), and mining the record here + # re-published lines that explicitly decline their point (#155). + findings = [str(x) for x in (output.get("findings") or []) + if not str(x).lstrip().lower().startswith("[resolved]")][: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 [] @@ -516,7 +459,6 @@ async def refine_review( 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/test/test_agent_ensemble.py b/test/test_agent_ensemble.py index cd246ba0..3a86845f 100644 --- a/test/test_agent_ensemble.py +++ b/test/test_agent_ensemble.py @@ -1159,28 +1159,44 @@ def test_outcome_blocked_distinguishes_dead_seat_from_quiet_seat(): "_tools_used": ["grep"]}) -def test_resolved_residual_becomes_a_comment(settings, trace, tmp_path): - """A `[resolved]` line that states a RESIDUAL is promoted into a scored - 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.refinement import ( - _promote_resolved_residuals, - ) - out = _promote_resolved_residuals( - _ctx(settings, trace, tmp_path), - {"review_comments": [], - "findings": [ - "[resolved] prior concern 'guard runs after the collective': " - "fixed at pipe.py:120 `validate(x)`. Residual: the batch path " - "at pipe.py:340 still calls it inside the rank-0 branch.", - "[resolved] prior concern 'missing pin': fixed at req.txt:3.", - "[sweep] read 40 files, nothing else of note"]}) - kept = out["review_comments"] - assert len(kept) == 1, "only the residual-bearing line promotes" - assert kept[0]["file"] == "pipe.py" and kept[0]["line"] == 120 - assert "Residual" in kept[0]["comment"] - assert next(trace.events("review_resolved_promoted"))["added"] == 1 +# vllm-project/vllm-omni#6968, verbatim: the `[resolved]` line that the old +# prose promotion published as a P2 even though it says it is not re-raised. +_DECLINED_RESOLVED = ( + "[resolved] TI2I silent guidance-mode flip on co-batch: request-unique key " + "(`_boogu_batch_compatibility_key` → (\"boogu_image\", \"ti2i\", request_id)) " + "+ RuntimeError fail-closed at pipeline_boogu_image.py:667; residual: TI2I " + "stays batch=1 until RequestBatchSamplingParamsKey adds " + "`guidance_scale_2_provided` (author follow-up) — not re-raised.") + + +def test_resolved_line_is_never_mined_into_a_comment(settings, trace, tmp_path): + """#155: a `[resolved]` findings line is the review's record, not an ask. + The prose promotion that turned residual-sounding lines into comments is + gone, and the coverage editor no longer receives `[resolved]` lines as raw + material, so the #6968 line cannot become a published finding.""" + from types import SimpleNamespace + + from infermatrix_copilot.engine.steps.review import refinement + + assert not hasattr(refinement, "_promote_resolved_residuals") + seen: list[str] = [] + + class _LLM: + available = True + + def create(self, *, messages, **_kw): + seen.append(messages[0]["content"]) + return SimpleNamespace(text='{"additions": []}') + + ctx = _ctx(settings, trace, tmp_path, llm=_LLM()) + out = asyncio.run(refinement._promote_uncovered( + ctx, {"review_comments": [], + "findings": [_DECLINED_RESOLVED, + "[validated] pipeline_boogu_image.py:690 image_gs " + "forced to 1.0 without a reference"]}, {})) + assert out["review_comments"] == [] + assert seen and "TI2I silent guidance-mode flip" not in seen[0] + assert "image_gs forced to 1.0" in seen[0] def test_empty_final_lens_is_retried_not_discarded(settings, trace, tmp_path): diff --git a/test/test_review_render.py b/test/test_review_render.py index 7a99b7b9..bec3d935 100644 --- a/test/test_review_render.py +++ b/test/test_review_render.py @@ -156,19 +156,25 @@ def test_findings_with_no_file_do_not_invent_a_broken_reference(): assert "`PR description`" in md and "`general`" in md -def test_promoted_residuals_are_deduped_against_each_other(): - """`covered` only held the pre-existing comments, so N resolved lines - about one residual promoted N times - pr4977 shipped four near-identical - PR-description-staleness comments, saturating the cap of 4.""" - import asyncio - from types import SimpleNamespace - 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), - "review_comments": []}) - assert len(out["review_comments"]) == 1 - assert asyncio is not None # keep the import honest for the linter +def test_residual_filed_as_a_publish_comment_is_published(): + """#155: a genuine residual still ships, as the reviewer's own comment. + The pr4977 residual (`_TRC`: the PR description still advertises the + dropped trust_remote_code kwarg) arrives as ONE `publish` comment quoting + its `[resolved]` line; the `[resolved]` lines themselves stay records.""" + from infermatrix_copilot.engine.steps.review.utils import ( + finalize_review_dispositions) + residual = {"file": "PR description", "line": None, "severity": "minor", + "disposition": "publish", + "comment": "The PR description still advertises the " + "trust_remote_code kwarg that 9947f414 dropped for " + "kernels 0.13.x compat; update it.", + "evidence": _TRC[3]} + record = {"file": "flash_attn_hub.py", "line": 34, "severity": "minor", + "disposition": "resolved", "comment": _TRC[1], "evidence": _TRC[1]} + published, withheld = finalize_review_dispositions( + {"review_comments": [residual, record], "findings": list(_TRC)}) + assert [c["comment"] for c in published] == [residual["comment"]] + assert [c["comment"] for c in withheld] == [_TRC[1]] def test_skill_curation_queue_stays_out_of_the_deliverable():