diff --git a/agent/reviewer.py b/agent/reviewer.py index acc99807..db0ec05d 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -94,6 +94,10 @@ Tools: `add_finding`, `update_finding`, `list_findings`, `publish_review`, `resolve_finding_thread`, `reply_to_finding_thread`. Call `publish_review` once at the end. +If `publish_review` returns `unresolvable_findings`, do NOT retry with the +same args — call `update_finding(status="resolved")` on those ids, or fix +their file/line via `update_finding`, then call `publish_review` again. + Re-review: for each open finding, `update_finding(id, status="resolved")` if fixed, `update_finding` with new fields + `note` if changed, otherwise do nothing. Add net-new findings with `add_finding`. diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 53879782..d98757b8 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -149,7 +149,34 @@ async def post_pull_request_review( e.response.status_code, body, ) - return {"_error": f"HTTP {e.response.status_code}: {body}"} + # GitHub returns 422 with errors like "Path could not be resolved" + # or "Line could not be resolved" when an inline comment's anchor + # is not part of the PR diff. Surface that as a structured signal + # so the tool layer can prune the offending findings and retry + # once, instead of the agent retrying with byte-identical args. + error_kind: str | None = None + raw_errors: list[Any] = [] + if e.response.status_code == 422: + try: + parsed = e.response.json() + if isinstance(parsed, dict): + candidate = parsed.get("errors", []) + if isinstance(candidate, list): + raw_errors = candidate + except Exception: # noqa: BLE001 — body may not be JSON + raw_errors = [] + if any( + isinstance(err, str) + and ("Path could not be resolved" in err or "Line could not be resolved" in err) + for err in raw_errors + ): + error_kind = "unresolved_anchor" + return { + "_error": f"HTTP {e.response.status_code}: {body}", + "_error_kind": error_kind, + "_raw_errors": raw_errors, + "_status": e.response.status_code, + } except httpx.HTTPError as e: logger.exception("Failed to POST PR review for %s/%s#%s", owner, repo, pr_number) return {"_error": f"{type(e).__name__}: {e}"} diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 4118b241..a06b0f8c 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -3,10 +3,13 @@ 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_findings import ( Severity, filter_findings_for_publish, @@ -35,6 +38,8 @@ from ..utils.github_token import ( ) from ..utils.slack import post_slack_thread_reply +logger = logging.getLogger(__name__) + def publish_review( severity_threshold: str = "medium", @@ -241,6 +246,68 @@ async def _publish_review_async( inline_comments=inline_comments, token=token, ) + # If GitHub rejected the batch because one or more inline comments anchor + # to a file/line that's not in the PR diff, drop just those findings and + # retry once. Returning the bare 422 to the agent only invites it to + # retry publish_review with byte-identical args until findings drain. + unresolvable_findings: list[str] = [] + if ( + isinstance(review_response, dict) + and review_response.get("_error_kind") == "unresolved_anchor" + ): + valid_with_payload, dropped_ids = await _filter_against_pr_diff( + eligible_with_payload, + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + ) + if dropped_ids and valid_with_payload: + retry_inline = [p for _, p in valid_with_payload] + retry_body = render_review_body(pr_number=pr_number, surfaced_count=len(retry_inline)) + retry_response = await post_pull_request_review( + owner=owner, + repo=repo, + pr_number=pr_number, + head_sha=head_sha, + body=retry_body, + inline_comments=retry_inline, + token=token, + ) + if isinstance(retry_response, dict) and "_error" not in retry_response: + review_response = retry_response + inline_comments = retry_inline + eligible_with_payload = valid_with_payload + unresolvable_findings = dropped_ids + else: + retry_error = ( + retry_response.get("_error", "unknown error") + if isinstance(retry_response, dict) + else "no response" + ) + return { + "success": False, + "error": f"Failed to POST PR review: {retry_error}", + "unresolvable_findings": dropped_ids, + "hint": ( + "Call update_finding(status='resolved') on these ids " + "or fix their file/line before retrying." + ), + } + else: + # Either nothing to drop (no diff_line_set available, so we can't + # tell which findings are bad) or everything would be dropped. + # Either way, do not retry — surface the structural signal so the + # agent stops retrying with the same args. + return { + "success": False, + "error": f"Failed to POST PR review: {review_response['_error']}", + "unresolvable_findings": dropped_ids, + "hint": ( + "Call update_finding(status='resolved') on these ids " + "or fix their file/line before retrying." + ), + } if isinstance(review_response, dict) and "_error" in review_response: return { "success": False, @@ -303,13 +370,106 @@ async def _publish_review_async( await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) - return { + result: dict[str, Any] = { "success": True, "review_id": review_id, "surfaced_count": len(inline_comments), "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), "resolved_thread_count": resolved_thread_count, } + if unresolvable_findings: + result["unresolvable_findings"] = unresolvable_findings + result["hint"] = ( + "Some findings had anchors not in the PR diff; " + "call update_finding to fix or resolve them." + ) + return result + + +async def _resolve_diff_line_set( + *, + owner: str, + repo: str, + pr_number: int, + token: str, +) -> dict[str, set[int]] | None: + """Return the new-side line set for the PR diff, fetching it if needed. + + Reviewer runs clear ``configurable['diff_line_set']`` before the agent + starts (so ``add_finding`` trusts the agent's anchors), which means the + publish-time retry path can't rely on it being populated. Fetch the PR's + unified diff from the GitHub REST API and recompute the line set on the + fly. Returns ``None`` if the fetch fails — caller treats that as "we + can't tell which finding is bad, don't retry blindly". + """ + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + cached = configurable.get("diff_line_set") if isinstance(configurable, dict) else None + 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, + ) + return None + return compute_diff_line_set(response.text) + + +async def _filter_against_pr_diff( + eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], + *, + owner: str, + repo: str, + pr_number: int, + token: str, +) -> tuple[list[tuple[dict[str, Any], dict[str, Any]]], list[str]]: + """Drop findings whose path/line range is not in the current PR diff. + + Returns ``(valid_with_payload, dropped_finding_ids)``. When the diff + cannot be resolved (fetch failed and no cached set), we return everything + unchanged and an empty drop list — the caller will then surface the + original error rather than retry blindly. + """ + diff_line_set = await _resolve_diff_line_set( + owner=owner, repo=repo, pr_number=pr_number, token=token + ) + if diff_line_set is None: + return list(eligible_with_payload), [] + + valid: list[tuple[dict[str, Any], dict[str, Any]]] = [] + dropped: list[str] = [] + for finding, payload in eligible_with_payload: + path = payload.get("path") + # Prefer the finding's recorded range; fall back to the payload line. + start_line = finding.get("start_line") + end_line = finding.get("end_line") + if end_line is None: + payload_line = payload.get("line") + if isinstance(payload_line, int): + 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): + valid.append((finding, payload)) + else: + finding_id = finding.get("id") + if isinstance(finding_id, str): + dropped.append(finding_id) + return valid, dropped async def _maybe_post_slack_completion_reply( diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index b2c1a5e9..bf8d15ef 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -671,3 +671,412 @@ async def test_reply_to_review_comment_posts_reply_payload() -> None: args = client_cm.post.await_args assert args.args[0] == "https://api.github.com/repos/o/r/pulls/7/comments/123/replies" assert args.kwargs["json"] == {"body": "Thanks for the context."} + + +@pytest.mark.asyncio +async def test_post_pull_request_review_tags_unresolved_anchor_on_422() -> None: + """A GitHub 422 with 'Path could not be resolved' must be tagged as + ``unresolved_anchor`` and carry the raw errors so the tool layer can act + on it (drop offending findings + retry) instead of bubbling an opaque + error string that the agent will only retry with identical args.""" + import httpx + + response = MagicMock() + response.status_code = 422 + response.text = '{"errors":["Path could not be resolved"]}' + response.json.return_value = {"errors": ["Path could not be resolved"]} + response.raise_for_status.side_effect = httpx.HTTPStatusError( + "Unprocessable Entity", + request=MagicMock(), + response=response, + ) + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.post = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + result = await post_pull_request_review( + owner="o", + repo="r", + pr_number=1, + head_sha="sha", + body="b", + inline_comments=[{"path": "missing.py", "line": 1, "side": "RIGHT", "body": "x"}], + token="t", + ) + + assert isinstance(result, dict) + assert result.get("_error_kind") == "unresolved_anchor" + assert result.get("_status") == 422 + assert result.get("_raw_errors") == ["Path could not be resolved"] + assert "HTTP 422" in result.get("_error", "") + + +@pytest.mark.asyncio +async def test_post_pull_request_review_tags_unresolved_anchor_on_line_error() -> None: + """A 'Line could not be resolved' 422 must also be tagged as + ``unresolved_anchor`` so a line that's not in the diff is treated the same + way as a path that's not in the diff.""" + import httpx + + response = MagicMock() + response.status_code = 422 + response.text = '{"errors":["Line could not be resolved"]}' + response.json.return_value = {"errors": ["Line could not be resolved"]} + response.raise_for_status.side_effect = httpx.HTTPStatusError( + "Unprocessable Entity", + request=MagicMock(), + response=response, + ) + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.post = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + result = await post_pull_request_review( + owner="o", + repo="r", + pr_number=1, + head_sha="sha", + body="b", + inline_comments=[], + token="t", + ) + + assert isinstance(result, dict) + assert result.get("_error_kind") == "unresolved_anchor" + + +@pytest.mark.asyncio +async def test_post_pull_request_review_does_not_tag_unrelated_422() -> None: + """A 422 whose errors don't match the anchor patterns must NOT be tagged + as ``unresolved_anchor`` — the retry path is only safe for known + per-comment anchor failures.""" + import httpx + + response = MagicMock() + response.status_code = 422 + response.text = '{"errors":["something else"]}' + response.json.return_value = {"errors": ["something else"]} + response.raise_for_status.side_effect = httpx.HTTPStatusError( + "Unprocessable Entity", + request=MagicMock(), + response=response, + ) + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.post = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + result = await post_pull_request_review( + owner="o", + repo="r", + pr_number=1, + head_sha="sha", + body="b", + inline_comments=[], + token="t", + ) + + assert isinstance(result, dict) + assert result.get("_error_kind") is None + assert result.get("_raw_errors") == ["something else"] + + +@pytest.mark.asyncio +async def test_publish_review_drops_unresolvable_findings_and_retries_once() -> None: + """When GitHub rejects the batch with an ``unresolved_anchor`` 422, the + tool must filter the bad findings against the PR diff_line_set, re-POST + with only the valid ones, return ``success=True``, and report the dropped + finding ids via ``unresolvable_findings`` plus a corrective hint.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _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), + ] + # 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}} + + first_response = { + "_error": "HTTP 422: ...", + "_error_kind": "unresolved_anchor", + "_raw_errors": ["Path could not be resolved"], + "_status": 422, + } + retry_response = {"id": 7777} + post_review = AsyncMock(side_effect=[first_response, retry_response]) + fetch_comments = AsyncMock(return_value=[]) + set_metadata = AsyncMock() + + with ( + patch( + "agent.tools.publish_review.get_config", + return_value={ + "configurable": { + "thread_id": "tid", + "diff_line_set": diff_line_set, + }, + }, + ), + 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.fetch_review_comments", fetch_comments), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch( + "agent.tools.publish_review._store_thread_ids_on_findings", + new_callable=AsyncMock, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + assert post_review.await_count == 2 + # Retry must contain only the in-diff finding. + retry_inline = post_review.await_args_list[1].kwargs["inline_comments"] + assert {c["path"] for c in retry_inline} == {"in_diff.py"} + assert result["success"] is True + assert result["review_id"] == 7777 + assert result["surfaced_count"] == 1 + assert result["unresolvable_findings"] == ["f_bad"] + assert "update_finding" in result["hint"] + + +@pytest.mark.asyncio +async def test_publish_review_reports_unresolvable_when_retry_still_fails() -> None: + """If even the filtered retry fails, the tool surfaces + ``success=False`` plus the offending finding ids and a hint — it must + NOT collapse into the opaque retry-with-same-args loop.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _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}} + + first_response = { + "_error": "HTTP 422: ...", + "_error_kind": "unresolved_anchor", + "_raw_errors": ["Path could not be resolved"], + "_status": 422, + } + retry_response = {"_error": "HTTP 500: boom"} + post_review = AsyncMock(side_effect=[first_response, retry_response]) + + with ( + patch( + "agent.tools.publish_review.get_config", + return_value={ + "configurable": { + "thread_id": "tid", + "diff_line_set": diff_line_set, + }, + }, + ), + 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", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + assert result["success"] is False + assert result["unresolvable_findings"] == ["f_bad"] + assert "update_finding" in result["hint"] + + +@pytest.mark.asyncio +async def test_publish_review_does_not_retry_when_no_findings_can_be_dropped() -> None: + """When the unresolved_anchor 422 fires but the diff_line_set rules out + no findings (e.g., diff data unavailable), the tool must NOT retry — it + must surface the structured error so the agent stops looping.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _f(id="f_only", severity="high", file="in_diff.py", start_line=10, end_line=10), + ] + # No cached diff_line_set, and the on-demand fetch fails — no way to tell + # which finding is bad. + first_response = { + "_error": "HTTP 422: ...", + "_error_kind": "unresolved_anchor", + "_raw_errors": ["Path could not be resolved"], + "_status": 422, + } + post_review = AsyncMock(return_value=first_response) + + with ( + patch( + "agent.tools.publish_review.get_config", + return_value={"configurable": {"thread_id": "tid"}}, + ), + 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_diff_line_set", + new_callable=AsyncMock, + return_value=None, + ), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + # Only one attempt — never retry blindly. + assert post_review.await_count == 1 + assert result["success"] is False + assert result["unresolvable_findings"] == [] + assert "update_finding" in result["hint"] + + +@pytest.mark.asyncio +async def test_publish_review_fetches_pr_diff_when_diff_line_set_missing() -> None: + """Reviewer runs clear ``diff_line_set`` from config before the agent + starts, so the publish-time retry path must fall back to fetching the + PR's unified diff on demand and recomputing the line set — otherwise no + finding is ever droppable and the retry surfaces empty + ``unresolvable_findings`` for the reachable production case.""" + from agent.tools.publish_review import _publish_review_async + + findings = [ + _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), + ] + first_response = { + "_error": "HTTP 422: ...", + "_error_kind": "unresolved_anchor", + "_raw_errors": ["Path could not be resolved"], + "_status": 422, + } + retry_response = {"id": 9999} + post_review = AsyncMock(side_effect=[first_response, retry_response]) + + 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" + ) + + 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", + return_value={"configurable": {"thread_id": "tid"}}, + ), + 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.httpx.AsyncClient", _FakeClient), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch( + "agent.tools.publish_review._store_thread_ids_on_findings", + new_callable=AsyncMock, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + assert post_review.await_count == 2 + retry_inline = post_review.await_args_list[1].kwargs["inline_comments"] + assert {c["path"] for c in retry_inline} == {"in_diff.py"} + assert result["success"] is True + assert result["unresolvable_findings"] == ["f_bad"]