diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 806e17c7..abc8ed0a 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -221,6 +221,15 @@ def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None: return payload +def review_summary_marker(pr_number: int) -> str: + """The hidden marker embedded in every Open SWE review summary body. + + Used both to stamp the summary (``render_review_body``) and to detect + (``open_swe_review_exists``) whether Open SWE has already reviewed a PR. + """ + return f"" + + def render_review_body(*, pr_number: int, surfaced_count: int, trace_url: str | None = None) -> str: """Compose the top-level review body.""" if surfaced_count == 0: @@ -235,10 +244,57 @@ def render_review_body(*, pr_number: int, surfaced_count: int, trace_url: str | parts = [headline] if trace_url: parts.append(f"[View Open SWE trace]({trace_url})") - parts.append(f"") + parts.append(review_summary_marker(pr_number)) return "\n\n".join(parts) +async def open_swe_review_exists( + *, + owner: str, + repo: str, + pr_number: int, + token: str, +) -> bool: + """Return True if Open SWE has already posted a review summary on this PR. + + Detected via the ``review_summary_marker`` that ``render_review_body`` + embeds in every Open SWE review body. The reviewer uses this to avoid + posting a duplicate "No issues found" summary when the ``re_review`` config + flag is stale — a push that lands mid-run is delivered as a queued message + into the still-running first-review run, whose configurable still says + ``re_review=False``, so the empty-review guard can't trust that flag alone. + + On any API failure this returns False (fail open): the only consequence is + a possible duplicate summary, never a suppressed first review. + """ + marker = review_summary_marker(pr_number) + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/pulls/{pr_number}/reviews" + headers = _github_headers(token) + params: dict[str, Any] = {"per_page": 100, "page": 1} + async with httpx.AsyncClient() as client: + while True: + try: + response = await client.get(url, headers=headers, params=params, timeout=30) + response.raise_for_status() + except httpx.HTTPError: + logger.exception( + "Failed to list PR reviews for %s/%s#%s", + owner, + repo, + pr_number, + ) + return False + data = response.json() + if not isinstance(data, list) or not data: + return False + for review in data: + if isinstance(review, dict) and marker in (review.get("body") or ""): + return True + if len(data) < 100: # noqa: PLR2004 + return False + params["page"] += 1 + + async def post_pull_request_review( *, owner: str, diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 23c78bb0..c465f95f 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -27,6 +27,7 @@ from ..reviewer_publish import ( fetch_pr_review_threads, fetch_review_comments, fetch_review_thread_id_for_comment, + open_swe_review_exists, parse_review_comment_marker, post_pull_request_review, render_inline_comment_payload, @@ -235,12 +236,20 @@ async def _publish_review_async( inline_comments.append(payload) eligible_with_payload.append((dict(finding), payload)) - # On re-review with nothing new to surface, skip the "no issues found" - # comment — the user already saw the previous findings, and posting - # another summary on every push is noise. 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 is_re_review and not inline_comments: + # With nothing new to surface, skip the "no issues found" summary if Open + # SWE has already reviewed this PR — the user already saw the previous + # result, and posting another summary on every push is noise. We can't rely + # on the static re_review flag alone: a push that lands mid-run is delivered + # as a queued message into the still-running first-review run, whose + # configurable still says re_review=False, so that path would post a + # duplicate "No issues found". Key off the actual PR state (an existing Open + # 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 ( + is_re_review + or await open_swe_review_exists(owner=owner, repo=repo, pr_number=pr_number, token=token) + ): resolved_thread_count = await _resolve_threads_for_resolved_findings( owner=owner, repo=repo, diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 21c2916b..72297874 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -11,6 +11,7 @@ import pytest from agent.reviewer_findings import Finding, new_finding from agent.reviewer_publish import ( fetch_pr_review_threads, + open_swe_review_exists, parse_review_comment_marker, post_pull_request_review, render_inline_comment_body, @@ -19,6 +20,7 @@ from agent.reviewer_publish import ( render_review_body, reply_to_review_comment, resolve_review_thread, + review_summary_marker, ) @@ -42,6 +44,7 @@ def _isolate_publish_review_pr_state() -> Iterator[None]: with ( patch("agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[])), patch("agent.tools.publish_review.replace_findings", AsyncMock()), + patch("agent.tools.publish_review.open_swe_review_exists", AsyncMock(return_value=False)), ): yield @@ -512,6 +515,135 @@ async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> assert result["skipped_empty_re_review"] is True +@pytest.mark.asyncio +async def test_publish_review_skips_duplicate_empty_summary_when_open_swe_already_reviewed() -> ( + None +): + """A push landing mid-run is queued into the still-running first-review run, + whose configurable still says re_review=False. With nothing to surface, the + empty-review guard must key off the existing Open SWE review summary on the + PR (not the stale flag) so it does not post a duplicate "No issues found".""" + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock() + set_metadata = AsyncMock() + resolve_threads = AsyncMock(return_value=0) + review_exists = AsyncMock(return_value=True) + slack_reply = AsyncMock() + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[])), + patch("agent.tools.publish_review.open_swe_review_exists", review_exists), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch("agent.tools.publish_review._resolve_threads_for_resolved_findings", resolve_threads), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch("agent.tools.publish_review._maybe_post_slack_completion_reply", slack_reply), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + ) + + post_review.assert_not_called() + review_exists.assert_awaited_once() + resolve_threads.assert_awaited_once() + slack_reply.assert_not_called() + assert result["success"] is True + assert result["review_id"] is None + assert result["surfaced_count"] == 0 + assert result["skipped_empty_re_review"] is True + set_metadata.assert_awaited_once_with("tid", last_reviewed_sha="newsha") + + +@pytest.mark.asyncio +async def test_publish_review_skips_review_existence_check_on_re_review() -> None: + """When re_review is already True we know a prior review exists, so the + empty-review guard must short-circuit without an extra reviews API call.""" + from agent.tools.publish_review import _publish_review_async + + review_exists = AsyncMock(return_value=True) + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[])), + patch("agent.tools.publish_review.open_swe_review_exists", review_exists), + patch("agent.tools.publish_review.post_pull_request_review", AsyncMock()) as 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="newsha", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=True, + ) + + review_exists.assert_not_called() + post_review.assert_not_called() + assert result["skipped_empty_re_review"] is True + + +@pytest.mark.asyncio +async def test_open_swe_review_exists_detects_summary_marker() -> None: + response = MagicMock() + response.json.return_value = [ + {"id": 1, "body": "some human review"}, + {"id": 2, "body": f"## ✅ Open SWE Review\n\n{review_summary_marker(7)}"}, + ] + response.raise_for_status.return_value = None + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.get = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t") + assert exists is True + + +@pytest.mark.asyncio +async def test_open_swe_review_exists_false_without_marker() -> None: + response = MagicMock() + response.json.return_value = [{"id": 1, "body": "looks good to me"}] + response.raise_for_status.return_value = None + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.get = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t") + assert exists is False + + +@pytest.mark.asyncio +async def test_open_swe_review_exists_fails_open_on_http_error() -> None: + import httpx + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.get = AsyncMock(side_effect=httpx.HTTPError("boom")) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t") + assert exists is False + + @pytest.mark.asyncio async def test_re_review_backfills_existing_marker_and_skips_duplicate_post() -> None: from agent.tools.publish_review import _publish_review_async