adam/open-swe
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/open-swe.git synced 2026-09-30 06:53:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

feat: surface out-of-diff findings in a collapsed dropdown (#1427)

Browse source 
* 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 <details> 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>
This commit is contained in:
Johannes du Plessis 2026-06-05 10:41:48 -07:00 • committed by GitHub
parent d2b3cb01c0
commit f64ab2bcd7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 216 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
agent/reviewer.py
View file

@ -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

3
agent/reviewer_findings.py
View file

@ -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",

45
agent/reviewer_publish.py
View file

@ -230,18 +230,59 @@ def review_summary_marker(pr_number: int) -> str:
return f"<!-- open-swe-reviewer pr={pr_number} -->"
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 ``<details>`` 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"<details>\n<summary>🔍 {count} out-of-diff {noun}</summary>\n\n"
"These relate to code outside this PR's changed lines.\n\n"
+ "\n".join(items)
+ "\n</details>"
)
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))

19
agent/tools/add_finding.py
View file

@ -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"] = (

57
agent/tools/publish_review.py
View file

@ -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:

79
tests/test_reviewer_publish.py
View file

@ -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 "<!-- open-swe-reviewer pr=99 -->" 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 "<details>" in body
assert "1 out-of-diff finding</summary>" 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</summary>" in body
assert "<!-- open-swe-reviewer pr=7 -->" 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

35
tests/test_reviewer_tools.py
View file

@ -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: