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(