From f64ab2bcd725687b5a998ce95a6bbae59376e9eb Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 5 Jun 2026 10:41:48 -0700 Subject: [PATCH] feat: surface out-of-diff findings in a collapsed dropdown (#1427) * fix: stop reviewer retrying out-of-diff findings add_finding rejects findings anchored outside the PR diff, but the agent retried the same finding 2-3x with adjacent line ranges before giving up, burning a model turn each. Add a reviewer-prompt recovery block telling the agent the rejection is authoritative (drop or re-anchor to a + line, don't retry adjacent), and enrich the rejection payload with nearby in-diff line ranges for the file so a single re-anchor needs no guessing. * feat: surface out-of-diff findings in a collapsed dropdown Instead of rejecting findings anchored outside the PR diff, accept them (marked in_diff=false) and surface them in a collapsed
section of the review summary, Devin-style. Inline comments stay reserved for in-diff findings; out-of-diff are severity-gated and capped the same way. Re-review normally suppresses the empty summary, but now makes an exception when there are new out-of-diff findings to surface. Surfaced out-of-diff findings carry a github_review_id so they aren't reposted on later pushes. Supersedes the earlier 'drop/re-anchor out-of-diff' prompt guidance. --------- Co-authored-by: open-swe[bot] <215916821+open-swe[bot]@users.noreply.github.com> --- agent/reviewer.py | 7 +++ agent/reviewer_findings.py | 3 ++ agent/reviewer_publish.py | 45 ++++++++++++++++++- agent/tools/add_finding.py | 19 ++++---- agent/tools/publish_review.py | 57 +++++++++++++++++++++--- tests/test_reviewer_publish.py | 79 +++++++++++++++++++++++++++++++++- tests/test_reviewer_tools.py | 35 ++++++++++----- 7 files changed, 216 insertions(+), 29 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index 79beb9dc..50ed144c 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -100,6 +100,13 @@ 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). + 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 new fields + `note` if changed, otherwise do nothing. Add net-new findings with diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 05b7fd1d..f4c317f2 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -89,6 +89,7 @@ class Finding(TypedDict, total=False): start_line: int | None end_line: int | None side: DiffSide + in_diff: bool description: str suggestion: str | None status: FindingStatus @@ -183,6 +184,7 @@ def new_finding( suggestion: str | None = None, diff_hunk: str | None = None, finding_id: str | None = None, + in_diff: bool = True, ) -> Finding: """Construct a fully-populated ``Finding`` ready to persist.""" resolved_id = finding_id or new_finding_id() @@ -212,6 +214,7 @@ def new_finding( "start_line": start_line, "end_line": end_line, "side": side, + "in_diff": in_diff, "description": description, "suggestion": suggestion, "status": "open", diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 56d4b47e..90070f49 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -230,18 +230,59 @@ def review_summary_marker(pr_number: int) -> str: return f"" -def render_review_body(*, pr_number: int, surfaced_count: int, trace_url: str | None = None) -> str: +def render_out_of_diff_section(findings: list[Finding]) -> str: + """Render findings anchored outside the PR diff as a collapsed dropdown. + + These can't be posted as inline comments (GitHub rejects off-diff lines), so + they live in the review summary body inside a ``
`` block — visible + on demand without adding noise to the changed-line review. + """ + count = len(findings) + noun = "finding" if count == 1 else "findings" + items: list[str] = [] + for f in findings: + title, detail = _split_title_and_detail( + (f.get("description") or "").strip(), f.get("title") + ) + location = f.get("file") or "?" + line_ref = _format_line_reference(f.get("start_line"), f.get("end_line")) + if line_ref: + location += f" {line_ref.strip('*()')}".replace("Refers to ", "") + item = f"- {_severity_emoji(f.get('severity') or 'medium')} **{title}** — `{location}`" + if detail: + item += f"\n {detail}" + items.append(item) + return ( + f"
\n🔍 {count} out-of-diff {noun}\n\n" + "These relate to code outside this PR's changed lines.\n\n" + + "\n".join(items) + + "\n
" + ) + + +def render_review_body( + *, + pr_number: int, + surfaced_count: int, + trace_url: str | None = None, + out_of_diff_findings: list[Finding] | None = None, +) -> str: """Compose the top-level review body.""" - if surfaced_count == 0: + out_of_diff_findings = out_of_diff_findings or [] + if surfaced_count == 0 and not out_of_diff_findings: headline = ( "## ✅ Open SWE Review: No issues found\n\n" "Open SWE reviewed this PR and found no potential bugs to report." ) + elif surfaced_count == 0: + headline = "**Open SWE Review** found no issues in the changed lines." else: issue_word = "issue" if surfaced_count == 1 else "issues" headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}." parts = [headline] + if out_of_diff_findings: + parts.append(render_out_of_diff_section(out_of_diff_findings)) if trace_url: parts.append(f"[View Open SWE trace]({trace_url})") parts.append(review_summary_marker(pr_number)) diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index ba08880b..90a58d59 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -115,16 +115,9 @@ def add_finding( diff_line_set = configurable.get("diff_line_set") if isinstance(configurable, dict) else None diff_text = configurable.get("diff_text", "") if isinstance(configurable, dict) else "" - if isinstance(diff_line_set, dict) and not is_range_in_diff( + 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) - ): - return { - "success": False, - "error": ( - f"Finding range {file}:{start_line}-{end_line} is not part of the PR diff. " - "Only review changes the PR introduces; do not flag pre-existing code." - ), - } + ) diff_hunk: str | None = None if isinstance(diff_text, str) and diff_text: @@ -150,10 +143,18 @@ def add_finding( side=_cast_side(side), suggestion=clipped_suggestion, diff_hunk=diff_hunk, + in_diff=in_diff, ) asyncio.run(append_finding(thread_id, finding)) 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 69cf8dda..852d7264 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -166,8 +166,15 @@ 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"] + 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( - unpublished_findings, + 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, ) @@ -184,7 +191,10 @@ async def _publish_review_eval_dry_run_async( "dry_run": True, "review_id": None, "surfaced_count": len(inline_comments), - "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), + "out_of_diff_count": len(eligible_out_of_diff), + "hidden_count": max( + len(open_unpublished) - len(inline_comments) - len(eligible_out_of_diff), 0 + ), "resolved_thread_count": 0, } @@ -229,8 +239,21 @@ 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_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( - unpublished_findings, severity_threshold=severity_threshold, cap=cap + 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]] = [] @@ -252,9 +275,15 @@ 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 ( - is_re_review - or await open_swe_review_exists(owner=owner, repo=repo, pr_number=pr_number, token=token) + if ( + not inline_comments + and not eligible_out_of_diff + and ( + is_re_review + or await open_swe_review_exists( + owner=owner, repo=repo, pr_number=pr_number, token=token + ) + ) ): resolved_thread_count = await _resolve_threads_for_resolved_findings( owner=owner, @@ -277,6 +306,7 @@ async def _publish_review_async( pr_number=pr_number, surfaced_count=len(inline_comments), trace_url=review_trace_url, + out_of_diff_findings=eligible_out_of_diff, ) review_response = await post_pull_request_review( @@ -310,6 +340,7 @@ async def _publish_review_async( pr_number=pr_number, surfaced_count=len(retry_inline), trace_url=review_trace_url, + out_of_diff_findings=eligible_out_of_diff, ) retry_response = await post_pull_request_review( owner=owner, @@ -368,6 +399,15 @@ 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: + # Mark surfaced out-of-diff findings so re-review doesn't repost them. + await _store_review_id_on_findings( + thread_id=thread_id, + findings=findings, + eligible_with_payload=[(dict(f), {}) for f in eligible_out_of_diff], + review_id=review_id, + ) + if review_id is not None and inline_comments: await _store_review_id_on_findings( thread_id=thread_id, @@ -435,7 +475,10 @@ async def _publish_review_async( "success": True, "review_id": review_id, "surfaced_count": len(inline_comments), - "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), + "out_of_diff_count": len(eligible_out_of_diff), + "hidden_count": max( + len(open_unpublished) - len(inline_comments) - len(eligible_out_of_diff), 0 + ), "resolved_thread_count": resolved_thread_count, } if unresolvable_findings: diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index de26da33..74f93655 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -206,7 +206,33 @@ def test_render_review_body_no_findings_message() -> None: body = render_review_body(pr_number=99, surfaced_count=0) assert "## ✅ Open SWE Review: No issues found" in body assert "Open SWE reviewed this PR and found no potential bugs to report." in body - assert "" in body + + +def test_render_review_body_with_only_out_of_diff_findings() -> None: + body = render_review_body( + pr_number=7, + surfaced_count=0, + out_of_diff_findings=[ + _f(title="Caller passes stale arg", description="boom", file="x/caller.py") + ], + ) + assert "No issues found" not in body + assert "found no issues in the changed lines" in body + assert "
" in body + assert "1 out-of-diff finding" in body + assert "**Caller passes stale arg**" in body + assert "`x/caller.py" in body + + +def test_render_review_body_combines_inline_and_out_of_diff() -> None: + body = render_review_body( + pr_number=7, + surfaced_count=2, + out_of_diff_findings=[_f(title="A"), _f(title="B")], + ) + assert "found 2 potential issues." in body + assert "2 out-of-diff findings" in body + assert "" in body def test_render_review_body_includes_trace_link_when_provided() -> None: @@ -539,6 +565,57 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> assert result["skipped_empty_re_review"] is True +@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.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _f( + id="f_ood", + file="caller.py", + in_diff=False, + first_seen_sha="newsha", + github_review_comment_id=None, + github_review_id=None, + ) + ] + post_review = AsyncMock(return_value={"id": 555}) + + with ( + 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._resolve_threads_for_resolved_findings", + AsyncMock(return_value=0), + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", 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)), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + 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"] == [] + assert result["success"] is True + assert result["surfaced_count"] == 0 + assert result["out_of_diff_count"] == 1 + assert "skipped_empty_re_review" not in result + + @pytest.mark.asyncio async def test_publish_review_skips_duplicate_empty_summary_when_open_swe_already_reviewed() -> ( None diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index e0d74cf0..558f4e79 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -88,8 +88,17 @@ def test_add_finding_rejects_empty_title() -> None: assert "title" in result["error"].lower() -def test_add_finding_rejects_out_of_diff_lines() -> None: - with patch("agent.tools.add_finding.get_config", return_value=_config()): +def test_add_finding_accepts_out_of_diff_lines_marked_not_in_diff() -> None: + captured: list[Any] = [] + + async def fake_append(_thread_id: str, finding: Any) -> None: + captured.append(finding) + + with ( + patch("agent.tools.add_finding.get_config", return_value=_config()), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.add_finding.append_finding", side_effect=fake_append), + ): result = add_finding( severity="high", confidence="high", @@ -100,8 +109,10 @@ def test_add_finding_rejects_out_of_diff_lines() -> None: start_line=99, end_line=99, ) - assert result["success"] is False - assert "not part of the PR diff" in result["error"] + assert result["success"] is True + assert result["in_diff"] is False + assert "out-of-diff section" in result["note"] + assert captured[0]["in_diff"] is False def test_add_finding_accepts_left_side_anchor_on_old_line() -> None: @@ -139,9 +150,9 @@ def test_add_finding_accepts_left_side_anchor_on_old_line() -> None: assert result["success"] is True -def test_add_finding_rejects_left_anchor_outside_old_side_set() -> None: - """A LEFT anchor on a line that's not in the old-side hunk must be - rejected — same guard, just on the correct side.""" +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.""" config = { "configurable": { "thread_id": "tid-1", @@ -153,7 +164,11 @@ def test_add_finding_rejects_left_anchor_outside_old_side_set() -> None: }, "metadata": {}, } - with patch("agent.tools.add_finding.get_config", return_value=config): + with ( + patch("agent.tools.add_finding.get_config", return_value=config), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.add_finding.append_finding", new_callable=AsyncMock), + ): result = add_finding( severity="high", confidence="high", @@ -165,8 +180,8 @@ def test_add_finding_rejects_left_anchor_outside_old_side_set() -> None: end_line=99, side="LEFT", ) - assert result["success"] is False - assert "not part of the PR diff" in result["error"] + assert result["success"] is True + assert result["in_diff"] is False def test_add_finding_rejects_invalid_confidence() -> None: