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

fix: fetch PR diff via GitHub API to re-enable add_finding validation (#1339)

Browse source 
* reviewer: fetch PR diff via GitHub API to re-enable add_finding validation

The previous hotfix in reviewer.py set diff_line_set=None because the
sandbox-based diff prep was sometimes producing empty diffs. That made
every bad anchor a publish-time 422 instead of a creation-time
rejection — the agent burned tokens producing unanchorable findings,
and we had to add a publish-time retry safety net (#1338) to clean up.

Fetch the PR's unified diff via the GitHub REST API at reviewer
startup and populate diff_text + diff_line_set so add_finding can
reject bad anchors immediately. The API path is reliable and is the
same diff GitHub validates against when posting inline review
comments. If the fetch fails, fall back to the previous behavior
(validation disabled, publish-time retry handles it).

Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so
both reviewer.py and publish_review.py share one implementation
instead of two copies.

* reviewer: make diff_line_set validation side-aware

compute_diff_line_set previously returned only new-side line numbers,
so re-enabling add_finding's validation would wrongly reject findings
with side=LEFT (deleted-line bugs whose only anchor is an old-side
line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}}
instead, and have is_range_in_diff select the matching side from the
finding's recorded side. add_finding and publish_review's retry
filter both pass the finding's side through.
This commit is contained in:
Johannes du Plessis 2026-05-27 10:03:33 -07:00 • committed by GitHub
parent 58b1d52fee
commit fa147ce256
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 332 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
agent/reviewer.py
View file

@ -39,6 +39,7 @@ from .middleware import (
SlackAssistantStatusMiddleware,
ToolErrorMiddleware,
)
from .reviewer_diff import compute_diff_line_set, fetch_pr_diff
from .reviewer_findings import (
list_findings as list_findings_async,
)
@ -534,13 +535,33 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
last_reviewed_sha = str(config["configurable"].get("last_reviewed_sha", "") or "")
is_re_review = bool(config["configurable"].get("re_review"))
# Hotfix: prep was producing empty diffs for some PRs and the agent
# silently published "no issues found". The agent now fetches the diff
# itself via `gh pr diff` (or `gh api ...compare...` on re-review).
# `add_finding`'s in-diff line-range validation is skipped when no
# diff_line_set is set in config — we trust the agent's anchors.
config["configurable"]["diff_text"] = ""
config["configurable"]["diff_line_set"] = None
# Fetch the PR's unified diff from the GitHub API and populate
# diff_text + diff_line_set so add_finding can reject bad anchors at
# creation time (instead of letting them fail at publish_review with a
# 422 the agent then has to clean up). The API path is reliable — the
# previous sandbox-based prep was sometimes producing empty diffs,
# which is what forced the earlier hotfix. If the fetch fails, leave
# the validation disabled so the run isn't blocked entirely.
pr_diff_text = ""
pr_diff_line_set: dict[str, set[int]] | None = None
if (
pr_number is not None
and isinstance(pr_number, int)
and repo_owner
and repo_name
and github_token
):
fetched_diff = await fetch_pr_diff(
owner=repo_owner,
repo=repo_name,
pr_number=pr_number,
token=github_token,
)
if fetched_diff is not None:
pr_diff_text = fetched_diff
pr_diff_line_set = compute_diff_line_set(fetched_diff)
config["configurable"]["diff_text"] = pr_diff_text
config["configurable"]["diff_line_set"] = pr_diff_line_set
existing_threads_block = ""
if (

77
agent/reviewer_diff.py
View file

@ -18,11 +18,13 @@ import asyncio
import logging
import re
from dataclasses import dataclass
from typing import TYPE_CHECKING
from typing import TYPE_CHECKING, Literal
if TYPE_CHECKING:
from deepagents.backends.protocol import SandboxBackendProtocol
DiffSide = Literal["LEFT", "RIGHT"]
logger = logging.getLogger(__name__)
@ -122,19 +124,24 @@ def parse_unified_diff(diff_text: str) -> list[FileDiff]:
return files
def compute_diff_line_set(diff_text: str) -> dict[str, set[int]]:
"""Return ``{file: {line, ...}}`` for the new-side lines covered by the diff.
def compute_diff_line_set(diff_text: str) -> dict[str, dict[str, set[int]]]:
"""Return ``{file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}}`` for the
lines covered by the diff.
A ``Finding`` whose ``(file, start_line..end_line)`` range falls outside
this set cannot be rendered as an inline GitHub review comment, so
``add_finding`` rejects it.
Inline GitHub review comments anchor either to a new-side line (``side =
RIGHT``, the default — additions and context) or to an old-side line
(``side = LEFT`` — deletions). ``add_finding`` and ``publish_review``
validate a finding's ``(file, start_line..end_line, side)`` against the
matching set so deleted-line bugs aren't wrongly rejected.
"""
out: dict[str, set[int]] = {}
out: dict[str, dict[str, set[int]]] = {}
for file_diff in parse_unified_diff(diff_text):
lines = out.setdefault(file_diff.file, set())
sides = out.setdefault(file_diff.file, {"RIGHT": set(), "LEFT": set()})
for hunk in file_diff.hunks:
for line in range(hunk.new_start, hunk.new_end + 1):
lines.add(line)
sides["RIGHT"].add(line)
for line in range(hunk.old_start, hunk.old_end + 1):
sides["LEFT"].add(line)
return out
@ -164,23 +171,63 @@ def extract_diff_hunk(
def is_range_in_diff(
line_set: dict[str, set[int]],
line_set: dict[str, dict[str, set[int]]],
file: str,
start_line: int | None,
end_line: int | None,
side: DiffSide = "RIGHT",
) -> bool:
"""Return True if every line in ``start_line..end_line`` is in the diff.
"""Return True if every line in ``start_line..end_line`` is on the given
side of the diff for ``file``.
File-level findings (both None) are always allowed.
File-level findings (both None) are always allowed. ``side`` selects
new-side lines (``RIGHT``, the default — additions/context) or old-side
lines (``LEFT`` — deletions). Pass the finding's recorded ``side``.
"""
if start_line is None and end_line is None:
return True
if start_line is None or end_line is None:
return False
file_lines = line_set.get(file)
if not file_lines:
file_sides = line_set.get(file)
if not file_sides:
return False
return all(line in file_lines for line in range(start_line, end_line + 1))
side_lines = file_sides.get(side)
if not side_lines:
return False
return all(line in side_lines for line in range(start_line, end_line + 1))
async def fetch_pr_diff(
*,
owner: str,
repo: str,
pr_number: int,
token: str,
timeout: float = 30.0,
) -> str | None:
"""Fetch the PR's unified diff (base..head) from the GitHub REST API.
Returns ``None`` if the request fails. This is the same diff GitHub
validates against when posting inline review comments, so it's the right
source for ``add_finding``'s in-diff anchor validation and for
``publish_review``'s 422 retry filter.
"""
import httpx
headers = {
"Accept": "application/vnd.github.diff",
"Authorization": f"Bearer {token}",
"X-GitHub-Api-Version": "2022-11-28",
}
url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
try:
async with httpx.AsyncClient() as client:
response = await client.get(url, headers=headers, timeout=timeout)
response.raise_for_status()
except httpx.HTTPError:
logger.exception("Failed to fetch PR diff for %s/%s#%s", owner, repo, pr_number)
return None
return response.text
async def compute_diff_in_sandbox(

2
agent/tools/add_finding.py
View file

@ -105,7 +105,7 @@ def add_finding(
diff_text = configurable.get("diff_text", "") if isinstance(configurable, dict) else ""
if isinstance(diff_line_set, dict) and not is_range_in_diff(
diff_line_set, file, start_line, end_line
diff_line_set, file, start_line, end_line, side=_cast_side(side)
):
return {
"success": False,

32
agent/tools/publish_review.py
View file

@ -3,13 +3,11 @@
from __future__ import annotations
import asyncio
import logging
from typing import Any
import httpx
from langgraph.config import get_config
from ..reviewer_diff import compute_diff_line_set, is_range_in_diff
from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff
from ..reviewer_findings import (
Severity,
filter_findings_for_publish,
@ -38,8 +36,6 @@ from ..utils.github_token import (
)
from ..utils.slack import post_slack_thread_reply
logger = logging.getLogger(__name__)
def publish_review(
severity_threshold: str = "medium",
@ -408,25 +404,10 @@ async def _resolve_diff_line_set(
if isinstance(cached, dict):
return cached
headers = {
"Accept": "application/vnd.github.diff",
"Authorization": f"Bearer {token}",
"X-GitHub-Api-Version": "2022-11-28",
}
url = f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}"
try:
async with httpx.AsyncClient() as client:
response = await client.get(url, headers=headers, timeout=30.0)
response.raise_for_status()
except httpx.HTTPError:
logger.exception(
"Failed to fetch PR diff for %s/%s#%s during publish_review retry",
owner,
repo,
pr_number,
)
diff_text = await fetch_pr_diff(owner=owner, repo=repo, pr_number=pr_number, token=token)
if diff_text is None:
return None
return compute_diff_line_set(response.text)
return compute_diff_line_set(diff_text)
async def _filter_against_pr_diff(
@ -463,7 +444,10 @@ async def _filter_against_pr_diff(
end_line = payload_line
if start_line is None:
start_line = payload_line
if isinstance(path, str) and is_range_in_diff(diff_line_set, path, start_line, end_line):
side = finding.get("side") if finding.get("side") in {"LEFT", "RIGHT"} else "RIGHT"
if isinstance(path, str) and is_range_in_diff(
diff_line_set, path, start_line, end_line, side=side
):
valid.append((finding, payload))
else:
finding_id = finding.get("id")

138
tests/test_reviewer.py
View file

@ -685,3 +685,141 @@ async def test_reviewer_continues_when_thread_fetch_raises() -> None:
# The reviewer must still produce a usable prompt even if the thread
# fetch fails; the first-review user-message context should still appear.
assert "## Pull request to review" in captured["system_prompt"]
@pytest.mark.asyncio
async def test_reviewer_populates_diff_line_set_from_github_api() -> None:
"""The reviewer must fetch the PR's unified diff via the GitHub API and
populate ``configurable['diff_line_set']`` + ``diff_text`` so
``add_finding`` can reject anchors not in the PR diff at creation time.
Without this, bad anchors only fail at publish_review with a 422."""
config: RunnableConfig = {
"configurable": {
"__is_for_execution__": True,
"thread_id": "reviewer-thread-id",
"source": "github",
"repo": {"owner": "acme", "name": "repo"},
"pr_number": 42,
"pr_url": "https://github.com/acme/repo/pull/42",
"base_sha": "base",
"head_sha": "head",
},
"metadata": {},
}
pr_diff = (
"diff --git a/in_diff.py b/in_diff.py\n"
"--- a/in_diff.py\n"
"+++ b/in_diff.py\n"
"@@ -1,1 +10,1 @@\n"
"+touched\n"
)
def fake_create_deep_agent(*, system_prompt: str, **kwargs: object) -> _DummyAgent:
return _DummyAgent()
with (
patch(
"agent.reviewer.get_github_token_from_thread",
new_callable=AsyncMock,
return_value=("gh-token", "encrypted-token", None),
),
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
patch(
"agent.reviewer.ensure_sandbox_for_thread",
new_callable=AsyncMock,
return_value=MagicMock(),
),
patch(
"agent.reviewer.aresolve_sandbox_work_dir",
new_callable=AsyncMock,
return_value="/workspace",
),
patch(
"agent.reviewer.fetch_agents_md",
new_callable=AsyncMock,
return_value=None,
),
patch(
"agent.reviewer.fetch_pr_review_threads",
new_callable=AsyncMock,
return_value=[],
),
patch(
"agent.reviewer.fetch_pr_diff",
new_callable=AsyncMock,
return_value=pr_diff,
) as mock_fetch_diff,
patch("agent.reviewer.make_model", return_value=MagicMock()),
patch("agent.reviewer.create_deep_agent", side_effect=fake_create_deep_agent),
):
await reviewer.get_reviewer_agent(config)
mock_fetch_diff.assert_awaited_once_with(
owner="acme", repo="repo", pr_number=42, token="gh-token"
)
assert config["configurable"]["diff_text"] == pr_diff
assert config["configurable"]["diff_line_set"] == {"in_diff.py": {"RIGHT": {10}, "LEFT": {1}}}
@pytest.mark.asyncio
async def test_reviewer_leaves_validation_disabled_when_diff_fetch_fails() -> None:
"""If the GitHub diff fetch fails, the reviewer must not block the run —
fall back to ``diff_line_set=None`` so ``add_finding`` skips validation
and the publish-time retry safety net handles anything bad."""
config: RunnableConfig = {
"configurable": {
"__is_for_execution__": True,
"thread_id": "reviewer-thread-id",
"source": "github",
"repo": {"owner": "acme", "name": "repo"},
"pr_number": 42,
"pr_url": "https://github.com/acme/repo/pull/42",
"base_sha": "base",
"head_sha": "head",
},
"metadata": {},
}
def fake_create_deep_agent(*, system_prompt: str, **kwargs: object) -> _DummyAgent:
return _DummyAgent()
with (
patch(
"agent.reviewer.get_github_token_from_thread",
new_callable=AsyncMock,
return_value=("gh-token", "encrypted-token", None),
),
patch("agent.reviewer.resolve_github_token", new_callable=AsyncMock),
patch(
"agent.reviewer.ensure_sandbox_for_thread",
new_callable=AsyncMock,
return_value=MagicMock(),
),
patch(
"agent.reviewer.aresolve_sandbox_work_dir",
new_callable=AsyncMock,
return_value="/workspace",
),
patch(
"agent.reviewer.fetch_agents_md",
new_callable=AsyncMock,
return_value=None,
),
patch(
"agent.reviewer.fetch_pr_review_threads",
new_callable=AsyncMock,
return_value=[],
),
patch(
"agent.reviewer.fetch_pr_diff",
new_callable=AsyncMock,
return_value=None,
),
patch("agent.reviewer.make_model", return_value=MagicMock()),
patch("agent.reviewer.create_deep_agent", side_effect=fake_create_deep_agent),
):
await reviewer.get_reviewer_agent(config)
assert config["configurable"]["diff_text"] == ""
assert config["configurable"]["diff_line_set"] is None

25
tests/test_reviewer_diff.py
View file

@ -44,8 +44,16 @@ def test_parse_unified_diff_extracts_hunks_per_file() -> None:
def test_compute_diff_line_set_covers_each_hunks_new_lines() -> None:
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
assert line_set["foo.py"] == {10, 11, 12, 13}
assert line_set["bar.py"] == {1, 2, 3, 51, 52, 53, 54}
assert line_set["foo.py"]["RIGHT"] == {10, 11, 12, 13}
assert line_set["bar.py"]["RIGHT"] == {1, 2, 3, 51, 52, 53, 54}
def test_compute_diff_line_set_also_covers_old_side_lines() -> None:
"""LEFT-side findings anchor to deleted/old-side lines; the line set
must expose those so add_finding doesn't wrongly reject them."""
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
assert line_set["foo.py"]["LEFT"] == {10, 11, 12}
assert line_set["bar.py"]["LEFT"] == {1, 2, 50, 51, 52}
def test_is_range_in_diff_for_inline_and_file_level() -> None:
@ -56,6 +64,19 @@ def test_is_range_in_diff_for_inline_and_file_level() -> None:
assert is_range_in_diff(line_set, "foo.py", None, None) is True
def test_is_range_in_diff_left_side_accepts_old_line_numbers() -> None:
"""A finding with side=LEFT must validate against the OLD-side line set,
not the new-side. The new-side hunk for foo.py is +10..+13; the old-side
is -10..-12. Asserting against the wrong side would falsely reject a
valid deleted-line finding."""
line_set = compute_diff_line_set(_TWO_FILE_DIFF)
assert is_range_in_diff(line_set, "foo.py", 12, 12, side="LEFT") is True
# And the same line on RIGHT side is also in-diff (it's context).
assert is_range_in_diff(line_set, "foo.py", 12, 12, side="RIGHT") is True
# A LEFT anchor on a line that doesn't exist on the old side must be rejected.
assert is_range_in_diff(line_set, "foo.py", 13, 13, side="LEFT") is False
def test_extract_diff_hunk_returns_overlapping_hunk_body() -> None:
hunk = extract_diff_hunk(_TWO_FILE_DIFF, "bar.py", 51, 52)
assert hunk is not None

26
tests/test_reviewer_publish.py
View file

@ -800,7 +800,7 @@ async def test_publish_review_drops_unresolvable_findings_and_retries_once() ->
]
# The PR diff only covers in_diff.py:10. f_bad anchors to a file/line not
# in the diff, so it must be dropped on retry.
diff_line_set = {"in_diff.py": {10}}
diff_line_set = {"in_diff.py": {"RIGHT": {10}, "LEFT": set()}}
first_response = {
"_error": "HTTP 422: ...",
@ -878,7 +878,7 @@ async def test_publish_review_reports_unresolvable_when_retry_still_fails() -> N
_f(id="f_good", severity="high", file="in_diff.py", start_line=10, end_line=10),
_f(id="f_bad", severity="high", file="not_in_diff.py", start_line=99, end_line=99),
]
diff_line_set = {"in_diff.py": {10}}
diff_line_set = {"in_diff.py": {"RIGHT": {10}, "LEFT": set()}}
first_response = {
"_error": "HTTP 422: ...",
@ -1019,23 +1019,6 @@ async def test_publish_review_fetches_pr_diff_when_diff_line_set_missing() -> No
"+touched\n"
)
http_response = MagicMock()
http_response.text = pr_diff
http_response.raise_for_status = MagicMock()
class _FakeClient:
def __init__(self, *_: Any, **__: Any) -> None:
return None
async def __aenter__(self) -> _FakeClient:
return self
async def __aexit__(self, *_: Any) -> None:
return None
async def get(self, *_: Any, **__: Any) -> Any:
return http_response
with (
patch(
"agent.tools.publish_review.get_config",
@ -1047,7 +1030,10 @@ async def test_publish_review_fetches_pr_diff_when_diff_line_set_missing() -> No
AsyncMock(return_value=findings),
),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.httpx.AsyncClient", _FakeClient),
patch(
"agent.tools.publish_review.fetch_pr_diff",
AsyncMock(return_value=pr_diff),
),
patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])),
patch(
"agent.tools.publish_review._resolve_threads_for_resolved_findings",

67
tests/test_reviewer_tools.py
View file

@ -16,7 +16,9 @@ def _config(**configurable_overrides: Any) -> dict[str, Any]:
"thread_id": "tid-1",
"head_sha": "sha-head",
"diff_text": "",
"diff_line_set": {"foo.py": list(range(10, 41))},
"diff_line_set": {
"foo.py": {"RIGHT": set(range(10, 41)), "LEFT": set()},
},
},
"metadata": {},
}
@ -54,6 +56,69 @@ def test_add_finding_rejects_out_of_diff_lines() -> None:
assert "not part of the PR diff" in result["error"]
def test_add_finding_accepts_left_side_anchor_on_old_line() -> None:
"""A finding on a deleted (LEFT-side) line must validate against the
old-side line set, not the new-side. With only RIGHT lines in 10..40,
a LEFT anchor at the same number should still pass when the line is in
the old-side set."""
config = {
"configurable": {
"thread_id": "tid-1",
"head_sha": "sha-head",
"diff_text": "",
"diff_line_set": {
"foo.py": {"RIGHT": {10, 11, 12}, "LEFT": {50, 51}},
},
},
"metadata": {},
}
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",
category="correctness",
file="foo.py",
description="deleted call to releaseResources()",
start_line=51,
end_line=51,
side="LEFT",
)
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."""
config = {
"configurable": {
"thread_id": "tid-1",
"head_sha": "sha-head",
"diff_text": "",
"diff_line_set": {
"foo.py": {"RIGHT": {10, 11, 12}, "LEFT": {50, 51}},
},
},
"metadata": {},
}
with patch("agent.tools.add_finding.get_config", return_value=config):
result = add_finding(
severity="high",
confidence="high",
category="correctness",
file="foo.py",
description="d",
start_line=99,
end_line=99,
side="LEFT",
)
assert result["success"] is False
assert "not part of the PR diff" in result["error"]
def test_add_finding_rejects_invalid_confidence() -> None:
with patch("agent.tools.add_finding.get_config", return_value=_config()):
result = add_finding(