From fa147ce256acdfb13acd6d90c26cc628541332af Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 27 May 2026 10:03:33 -0700 Subject: [PATCH] fix: fetch PR diff via GitHub API to re-enable add_finding validation (#1339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- agent/reviewer.py | 35 +++++++-- agent/reviewer_diff.py | 77 ++++++++++++++---- agent/tools/add_finding.py | 2 +- agent/tools/publish_review.py | 32 ++------ tests/test_reviewer.py | 138 +++++++++++++++++++++++++++++++++ tests/test_reviewer_diff.py | 25 +++++- tests/test_reviewer_publish.py | 26 ++----- tests/test_reviewer_tools.py | 67 +++++++++++++++- 8 files changed, 332 insertions(+), 70 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index db0ec05d..944c8056 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -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 ( diff --git a/agent/reviewer_diff.py b/agent/reviewer_diff.py index 81399dc7..d9e5e2ea 100644 --- a/agent/reviewer_diff.py +++ b/agent/reviewer_diff.py @@ -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( diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index 64abd97b..4bd30ca3 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -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, diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index a06b0f8c..8b158376 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -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") diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 85e3a352..e3ca2661 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -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 diff --git a/tests/test_reviewer_diff.py b/tests/test_reviewer_diff.py index 81ba838b..d3a897b2 100644 --- a/tests/test_reviewer_diff.py +++ b/tests/test_reviewer_diff.py @@ -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 diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index bf8d15ef..03360eca 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -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", diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 770b0dc1..7df2dd28 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -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(