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

feat: disable out-of-diff reviewer findings (#1516)

Browse source 
* 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] <open-swe@users.noreply.github.com>

* 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] <open-swe@users.noreply.github.com>

---------

Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
Johannes du Plessis 2026-06-12 10:19:30 -07:00 • committed by GitHub
parent 73b3ba7930
commit 258b5b4034
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 66 additions and 90 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
agent/reviewer.py
View file

@ -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 <base_sha>:path` vs
`git show <head_sha>: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.

24
agent/tools/add_finding.py
View file

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

65
agent/tools/publish_review.py
View file

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

23
tests/test_reviewer_publish.py
View file

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

16
tests/test_reviewer_tools.py
View file

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