From 258b5b4034405b72bb11180324cc0aad19547fd5 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 12 Jun 2026 10:19:30 -0700 Subject: [PATCH] feat: disable out-of-diff reviewer findings (#1516) * feat: disable out-of-diff reviewer findings PR reviews were surfacing findings about code outside the PR's changed lines, which read as random/off-topic noise. Reject out-of-diff findings at add_finding and stop surfacing them in publish_review so only findings anchored to changed lines reach the PR. Co-authored-by: open-swe[bot] * fix: align reviewer bar with out-of-diff rejection The filing bar still permitted proven regressions in files absent from the diff, but add_finding now rejects those, so a concrete regression could be silently dropped after a failed tool call. Update the bar and the "Do NOT file" list so the prompt only directs in-diff findings. Co-authored-by: open-swe[bot] --------- Co-authored-by: open-swe[bot] --- agent/reviewer.py | 28 +++++++-------- agent/tools/add_finding.py | 24 +++++++------ agent/tools/publish_review.py | 65 +++++++++------------------------- tests/test_reviewer_publish.py | 23 +++++++----- tests/test_reviewer_tools.py | 16 ++++----- 5 files changed, 66 insertions(+), 90 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index dfec50be..536bde8a 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -105,12 +105,10 @@ If `publish_review` returns `unresolvable_findings`, do NOT retry with the same args — call `update_finding(status="resolved", note="...")` on those ids, or fix their file/line via `update_finding`, then call `publish_review` again. -If `add_finding` returns `in_diff: false`, the finding was accepted but anchored -outside the PR diff (e.g. a caller broken by a changed signature, in a file the -PR doesn't touch). It will be surfaced in a collapsed "out-of-diff findings" -section of the review summary instead of as an inline comment. This is expected — -do NOT re-anchor, retry, or drop it. Only file such findings when they clear the -bar below (a proven regression, not speculation about pre-existing code). +Out-of-diff findings are disabled. `add_finding` rejects any finding whose +`start_line..end_line` is not part of the PR diff (returns `success: false` with +`in_diff: false`). Do NOT re-anchor or retry — only file findings anchored to a +line this PR actually changed. Re-review: for each open finding, `update_finding(id, status="resolved", note="...")` if fixed (include a brief explanation of the fix in `note`), `update_finding` with @@ -135,11 +133,12 @@ question or a short clarification is needed after pushback. 1. You can anchor it to a specific changed line and quote that line. 2. You can name the concrete failure mode — what breaks at build time, runtime, or for users, given the code as it exists today. -3. **Diff-anchor:** the finding's file appears in the PR diff hunk, OR you - proved a regression via `git show :path` vs - `git show :path` on a callsite of a symbol whose signature - changed in the diff. Do not file bugs in unrelated files or subsystems - based on inference alone. +3. **Diff-anchor:** the finding anchors to a specific line inside the PR diff + hunk. `add_finding` rejects any finding whose lines are not part of the + diff. A signature change can still cause a regression at an unchanged + callsite, but you can only file it when the affected line is itself in the + diff — do not file bugs in files or lines absent from the diff, and do not + file based on inference about unrelated files or subsystems. # Do NOT file @@ -172,9 +171,10 @@ question or a short clarification is needed after pushback. - **Scope-policing / architectural critique.** No "this PR doesn't achieve its stated goal", "the design should be different". - **Pre-existing issues** not introduced by this diff. -- **Out-of-diff / wrong-subsystem speculation.** Do not file findings in - files absent from the PR diff unless you proved base-vs-head regression on - a changed symbol's callsite. +- **Out-of-diff findings.** `add_finding` rejects any finding whose lines are + not part of the PR diff. Do not file findings in files or lines absent from + the diff — even a proven base-vs-head regression at an unchanged callsite + cannot be filed. - **Same-bug fan-out.** If the same defect appears in N files, file ONE finding that lists all sites in `description`. Not N findings. diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index 8a8fc63a..1701bcb0 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -51,9 +51,10 @@ def add_finding( ``start_line..end_line``. **In-diff only:** ``start_line..end_line`` must be inside the PR diff. - File-level findings (both ``start_line`` and ``end_line`` None) are - accepted but won't render as inline GitHub comments — only use when the - issue truly isn't anchored to a line. + Findings anchored to lines outside the diff are rejected (out-of-diff + findings are disabled). File-level findings (both ``start_line`` and + ``end_line`` None) are accepted but won't render as inline GitHub + comments — only use when the issue truly isn't anchored to a line. Args: severity: One of ``low``, ``medium``, ``high``, ``critical``. @@ -120,6 +121,16 @@ def add_finding( in_diff = not isinstance(diff_line_set, dict) or is_range_in_diff( diff_line_set, file, start_line, end_line, side=_cast_side(side) ) + if not in_diff: + return { + "success": False, + "in_diff": False, + "error": ( + "Out-of-diff findings are disabled. This finding's lines are not " + "part of this PR's diff. Only file findings anchored to a line the " + "PR changed. Do not re-anchor or retry." + ), + } diff_hunk: str | None = None if isinstance(diff_text, str) and diff_text: @@ -156,13 +167,6 @@ def add_finding( except ReviewerThreadMissingError as exc: return thread_missing_tool_result(exc) result: dict[str, Any] = {"success": True, "finding_id": finding["id"]} - if not in_diff: - result["in_diff"] = False - result["note"] = ( - "Anchored outside the PR diff. This will be surfaced in the collapsed " - "out-of-diff section of the review summary, not as an inline comment. " - "Do not re-anchor or retry." - ) if suggestion_dropped: result["suggestion_dropped"] = True result["warning"] = ( diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index e6de1ebf..78682317 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -79,7 +79,7 @@ def publish_review( Returns: Dictionary with ``success``, ``review_id``, ``surfaced_count``, ``hidden_count``, ``resolved_thread_count``, and sometimes - ``out_of_diff_count``, ``unresolvable_findings``, plus the flags below. + ``unresolvable_findings``, plus the flags below. ``success: true`` alone does NOT mean a GitHub Review was posted — check the flags: @@ -193,18 +193,13 @@ async def _publish_review_eval_dry_run_async( findings = await list_findings_async(thread_id) unpublished_findings = [f for f in findings if not _has_publication_identity(f)] open_unpublished = [f for f in unpublished_findings if f.get("status", "open") == "open"] + # Out-of-diff findings are disabled: only in-diff findings are surfaced. in_diff_unpublished = [f for f in unpublished_findings if f.get("in_diff", True)] - out_of_diff_unpublished = [f for f in unpublished_findings if not f.get("in_diff", True)] eligible = filter_findings_for_publish( in_diff_unpublished, severity_threshold=severity_threshold, cap=cap, ) - eligible_out_of_diff = filter_findings_for_publish( - out_of_diff_unpublished, - severity_threshold=severity_threshold, - cap=cap, - ) inline_comments = [ payload for finding in eligible @@ -218,10 +213,7 @@ async def _publish_review_eval_dry_run_async( "dry_run": True, "review_id": None, "surfaced_count": len(inline_comments), - "out_of_diff_count": len(eligible_out_of_diff), - "hidden_count": max( - len(open_unpublished) - len(inline_comments) - len(eligible_out_of_diff), 0 - ), + "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), "resolved_thread_count": 0, } @@ -267,22 +259,13 @@ async def _publish_review_async( f for f in unpublished_findings if f.get("first_seen_sha") == head_sha ] open_unpublished = [f for f in unpublished_findings if f.get("status", "open") == "open"] - # In-diff findings become inline comments. Out-of-diff findings can't anchor - # to an inline comment (GitHub rejects off-diff lines), so they're surfaced - # in a collapsed dropdown in the review body. Already-surfaced out-of-diff - # findings carry a github_review_id, so they're not re-posted on re-review. + # In-diff findings become inline comments. Out-of-diff findings are disabled: + # they are never surfaced on the PR (any legacy in-state ones are treated as + # hidden). in_diff_unpublished = [f for f in unpublished_findings if f.get("in_diff", True)] - out_of_diff_unpublished = [ - f - for f in unpublished_findings - if not f.get("in_diff", True) and not isinstance(f.get("github_review_id"), int) - ] eligible = filter_findings_for_publish( in_diff_unpublished, severity_threshold=severity_threshold, cap=cap ) - eligible_out_of_diff = filter_findings_for_publish( - out_of_diff_unpublished, severity_threshold=severity_threshold, cap=cap - ) inline_comments: list[dict[str, Any]] = [] eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]] = [] @@ -303,17 +286,13 @@ async def _publish_review_async( # SWE review summary) instead. Still resolve threads for findings that just # moved to resolved, and advance last_reviewed_sha so subsequent pushes # don't redo the same diff. - if ( - not inline_comments - and not eligible_out_of_diff - and await _open_swe_already_reviewed( - thread_id=thread_id, - owner=owner, - repo=repo, - pr_number=pr_number, - token=token, - is_re_review=is_re_review, - ) + if not inline_comments and await _open_swe_already_reviewed( + thread_id=thread_id, + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + is_re_review=is_re_review, ): resolved_thread_count = await _resolve_threads_for_resolved_findings( owner=owner, @@ -348,7 +327,6 @@ async def _publish_review_async( surfaced_count=len(inline_comments), trace_url=review_trace_url, ui_url=review_ui_url, - out_of_diff_findings=eligible_out_of_diff, ) review_response = await post_pull_request_review( @@ -383,7 +361,6 @@ async def _publish_review_async( surfaced_count=len(retry_inline), trace_url=review_trace_url, ui_url=review_ui_url, - out_of_diff_findings=eligible_out_of_diff, ) retry_response = await post_pull_request_review( owner=owner, @@ -442,7 +419,7 @@ async def _publish_review_async( } review_id = review_response.get("id") if isinstance(review_response, dict) else None - if review_id is not None and (eligible_out_of_diff or inline_comments): + if review_id is not None and inline_comments: # Record the GitHub review id AND inline comment ids in a single # findings write. Previously these were three separate read-replace # cycles (out-of-diff review id, inline review id, comment ids); each @@ -466,7 +443,6 @@ async def _publish_review_async( await _record_review_publication( thread_id=thread_id, review_id=review_id, - out_of_diff_findings=eligible_out_of_diff, inline_with_payload=eligible_with_payload, comment_records=comment_records, langgraph_run_id=langgraph_run_id, @@ -510,9 +486,7 @@ async def _publish_review_async( await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) await clear_review_started_comment(thread_id=thread_id, owner=owner, repo=repo, token=token) - conclusion, check_title, check_summary = review_check_conclusion( - len(inline_comments) + len(eligible_out_of_diff) - ) + conclusion, check_title, check_summary = review_check_conclusion(len(inline_comments)) await settle_review_check_run( thread_id=thread_id, owner=owner, @@ -527,10 +501,7 @@ async def _publish_review_async( "success": True, "review_id": review_id, "surfaced_count": len(inline_comments), - "out_of_diff_count": len(eligible_out_of_diff), - "hidden_count": max( - len(open_unpublished) - len(inline_comments) - len(eligible_out_of_diff), 0 - ), + "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), "resolved_thread_count": resolved_thread_count, } if unresolvable_findings: @@ -736,7 +707,6 @@ async def _record_review_publication( *, thread_id: str, review_id: int, - out_of_diff_findings: list[Finding], inline_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], comment_records: list[dict[str, Any]], langgraph_run_id: str | None, @@ -749,9 +719,6 @@ async def _record_review_publication( GitHub returned for it in the same record. """ review_finding_ids = { - finding.get("id") for finding in out_of_diff_findings if isinstance(finding.get("id"), str) - } - review_finding_ids |= { finding.get("id") for finding, _payload in inline_with_payload if isinstance(finding.get("id"), str) diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 2a227ba1..5fe296ca 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -669,10 +669,10 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> @pytest.mark.asyncio -async def test_publish_review_surfaces_out_of_diff_finding_on_re_review() -> None: - """A new out-of-diff finding has no inline comment, but the re-review - empty-summary suppression must not swallow it — it should post a summary - review carrying the collapsed out-of-diff dropdown.""" +async def test_publish_review_does_not_surface_out_of_diff_finding() -> None: + """Out-of-diff findings are disabled: a finding anchored outside the diff is + never surfaced on the PR. On a re-review with nothing else to post, it is + treated as an empty re-review.""" from agent.tools.publish_review import _publish_review_async findings = [ @@ -691,11 +691,17 @@ async def test_publish_review_surfaces_out_of_diff_finding_on_re_review() -> Non patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)), patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch( + "agent.tools.publish_review._open_swe_already_reviewed", + AsyncMock(return_value=True), + ), patch( "agent.tools.publish_review._resolve_threads_for_resolved_findings", AsyncMock(return_value=0), ), patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()), + patch("agent.tools.publish_review.clear_review_started_comment", AsyncMock()), + patch("agent.tools.publish_review.settle_review_check_run", AsyncMock()), patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()), patch("agent.tools.publish_review._resolve_review_trace_url", AsyncMock(return_value=None)), ): @@ -710,13 +716,12 @@ async def test_publish_review_surfaces_out_of_diff_finding_on_re_review() -> Non is_re_review=True, ) - post_review.assert_awaited_once() - assert "out-of-diff" in post_review.await_args.kwargs["body"] - assert post_review.await_args.kwargs["inline_comments"] == [] + post_review.assert_not_called() assert result["success"] is True + assert result["review_id"] is None assert result["surfaced_count"] == 0 - assert result["out_of_diff_count"] == 1 - assert "skipped_empty_re_review" not in result + assert "out_of_diff_count" not in result + assert result["skipped_empty_re_review"] is True @pytest.mark.asyncio diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 6f173bee..17babd42 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -88,7 +88,7 @@ def test_add_finding_rejects_empty_title() -> None: assert "title" in result["error"].lower() -def test_add_finding_accepts_out_of_diff_lines_marked_not_in_diff() -> None: +def test_add_finding_rejects_out_of_diff_lines() -> None: captured: list[Any] = [] async def fake_append(_thread_id: str, finding: Any) -> None: @@ -109,10 +109,10 @@ def test_add_finding_accepts_out_of_diff_lines_marked_not_in_diff() -> None: start_line=99, end_line=99, ) - assert result["success"] is True + assert result["success"] is False assert result["in_diff"] is False - assert "out-of-diff section" in result["note"] - assert captured[0]["in_diff"] is False + assert "disabled" in result["error"].lower() + assert captured == [] def test_add_finding_accepts_left_side_anchor_on_old_line() -> None: @@ -150,9 +150,9 @@ def test_add_finding_accepts_left_side_anchor_on_old_line() -> None: assert result["success"] is True -def test_add_finding_left_anchor_outside_old_side_set_marked_not_in_diff() -> None: - """A LEFT anchor on a line that's not in the old-side hunk is accepted but - marked out-of-diff — same guard, just on the correct side.""" +def test_add_finding_left_anchor_outside_old_side_set_rejected() -> None: + """A LEFT anchor on a line that's not in the old-side hunk is rejected — + out-of-diff findings are disabled, validated on the correct side.""" config = { "configurable": { "thread_id": "tid-1", @@ -180,7 +180,7 @@ def test_add_finding_left_anchor_outside_old_side_set_marked_not_in_diff() -> No end_line=99, side="LEFT", ) - assert result["success"] is True + assert result["success"] is False assert result["in_diff"] is False