diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index a0b3d592..8e363507 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -63,6 +63,9 @@ from ..utils.tracing import REVIEW_TRACING_PROJECT logger = logging.getLogger(__name__) _VERDICT_EVENTS = {"approve": "APPROVE", "request_changes": "REQUEST_CHANGES"} +# Map the GitHub review event we POST to the review "state" GitHub reports back +# on the created review object, so we can confirm the verdict actually landed. +_EVENT_TO_STATE = {"APPROVE": "APPROVED", "REQUEST_CHANGES": "CHANGES_REQUESTED"} async def publish_review( @@ -113,9 +116,11 @@ async def publish_review( GitHub Review was created. When ``verdict`` was passed, the result also carries - ``verdict_submitted`` (a non-COMMENT review state was actually posted) - or ``verdict_ignored`` + ``verdict_ignored_reason`` - (``"verdict_not_requested"`` or ``"self_review"``). + ``verdict_submitted`` (GitHub confirmed the requested APPROVE/ + REQUEST_CHANGES state) or ``verdict_ignored`` + + ``verdict_ignored_reason`` (``"verdict_not_requested"``, + ``"self_review"``, ``"head_moved"`` — the reviewed commit is no longer + the PR head — or ``"author_unknown"``). """ if severity_threshold not in {"low", "medium", "high", "critical"}: return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"} @@ -308,26 +313,64 @@ async def _publish_review_async( ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() verdict_ignored_reason: str | None = None - if verdict is not None: - pr_author = _pr_author_from_thread(await get_thread_metadata(thread_id)) - if pr_author in INTERNAL_BOT_LOGINS: - logger.info( - "publish_review verdict %r downgraded to comment: PR %s/%s#%s was " - "authored by internal bot %r (self-review)", - verdict, - owner, - repo, - pr_number, - pr_author, - ) - verdict = None - verdict_ignored_reason = "self_review" - event = _VERDICT_EVENTS.get(verdict or "", "COMMENT") + reviewed_head_sha = head_sha # The run config's head_sha is frozen at run creation; a push that arrived # mid-run updated the live head in thread metadata. Prefer that so the # review anchors to (and last_reviewed_sha advances to) the commit actually # reviewed, not the stale one this run was created for. head_sha = await resolve_review_head_sha(thread_id, {"head_sha": head_sha}) + + if verdict is not None: + metadata = await get_thread_metadata(thread_id) + # A verdict is merge-affecting (a real APPROVE/REQUEST_CHANGES), so it + # must reflect exactly the commit the agent reviewed. If a push landed + # mid-run and moved the head, the resolved head no longer matches what + # this run examined — downgrade to a comment rather than stamp an + # approval onto unreviewed code (and let the push's own re-review submit + # a fresh verdict). A plain comment publish still safely retargets. + if reviewed_head_sha and head_sha and head_sha != reviewed_head_sha: + logger.info( + "publish_review verdict %r downgraded to comment: head moved %s -> %s " + "mid-run for %s/%s#%s", + verdict, + reviewed_head_sha, + head_sha, + owner, + repo, + pr_number, + ) + verdict = None + verdict_ignored_reason = "head_moved" + else: + pr_author = _pr_author_from_thread(metadata) + bot_logins = {login.casefold() for login in INTERNAL_BOT_LOGINS} + if not pr_author: + # Fail closed: a verdict on a PR whose author we cannot confirm + # might be a self-review on an Open SWE PR. Downgrade rather than + # risk approving our own work. + logger.info( + "publish_review verdict %r downgraded to comment: PR author " + "unknown for %s/%s#%s", + verdict, + owner, + repo, + pr_number, + ) + verdict = None + verdict_ignored_reason = "author_unknown" + elif pr_author.casefold() in bot_logins: + logger.info( + "publish_review verdict %r downgraded to comment: PR %s/%s#%s was " + "authored by internal bot %r (self-review)", + verdict, + owner, + repo, + pr_number, + pr_author, + ) + verdict = None + verdict_ignored_reason = "self_review" + event = _VERDICT_EVENTS.get(verdict or "", "COMMENT") review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override) review_ui_url = dashboard_review_url(owner, repo, pr_number) findings = await _backfill_findings_from_pr_threads( @@ -518,6 +561,49 @@ async def _publish_review_async( "or fix their file/line before retrying." ), } + elif event != "COMMENT": + # A verdict is pending but every inline comment anchors outside the + # diff. GitHub accepts a bodied review with zero inline comments, so + # post the authorized verdict rather than dropping it — the verdict + # must land even when the findings can't be anchored. + verdict_only_body = _decorate_review_body( + render_review_body( + pr_number=pr_number, + surfaced_count=0, + trace_url=review_trace_url, + ui_url=review_ui_url, + additional_findings_count=additional_findings_count, + ), + verdict=verdict, + verdict_requester=verdict_requester, + verdict_ignored_reason=verdict_ignored_reason, + ) + verdict_only_response = await post_pull_request_review( + owner=owner, + repo=repo, + pr_number=pr_number, + head_sha=head_sha, + body=verdict_only_body, + inline_comments=[], + token=token, + event=event, + ) + if isinstance(verdict_only_response, dict) and "_error" not in verdict_only_response: + review_response = verdict_only_response + inline_comments = [] + eligible_with_payload = [] + unresolvable_findings = dropped_ids + else: + verdict_error = ( + verdict_only_response.get("_error", "unknown error") + if isinstance(verdict_only_response, dict) + else "no response" + ) + return { + "success": False, + "error": f"Failed to POST PR review: {verdict_error}", + "unresolvable_findings": dropped_ids, + } else: # Either nothing to drop (no diff_line_set available, so we can't # tell which findings are bad) or everything would be dropped. @@ -546,7 +632,19 @@ async def _publish_review_async( } review_id = review_response.get("id") if isinstance(review_response, dict) else None - verdict_submitted = event != "COMMENT" and review_id is not None + # Trust GitHub's recorded review state, not the event we asked for: GitHub + # can accept the POST (returning an id) yet land the review as COMMENTED + # (e.g. a same-identity re-approval). Only claim a verdict when the returned + # state actually matches. When the response omits state, fall back to the + # requested event so a valid submission isn't under-reported. + returned_state = review_response.get("state") if isinstance(review_response, dict) else None + expected_state = _EVENT_TO_STATE.get(event) + if expected_state is None: + verdict_submitted = False + elif isinstance(returned_state, str) and returned_state: + verdict_submitted = returned_state.upper() == expected_state and review_id is not None + else: + verdict_submitted = review_id is not None await _reconcile_last_verdict( thread_id=thread_id, owner=owner, diff --git a/agent/utils/prompt_data.py b/agent/utils/prompt_data.py index cfc4a2d0..f6c607c4 100644 --- a/agent/utils/prompt_data.py +++ b/agent/utils/prompt_data.py @@ -18,6 +18,7 @@ DATA_BLOCK_WRAPPER_TAGS = ( "body", "pr_overview", "title", + "finding_reply", "requester_instructions", ) _CLOSING_TAG_RE = re.compile( diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index d506f590..cce5100c 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -2469,13 +2469,21 @@ async def test_publish_review_forwards_authorized_verdict() -> None: assert "verdict_ignored" not in result +# Sentinel so a test can pass an explicit empty-metadata dict (to exercise the +# author-unknown fail-closed path) distinctly from "not specified" (which +# defaults to a normal non-bot author so verdicts are honored). +_UNSET_METADATA: dict[str, Any] = {"__unset__": True} + + def _verdict_publish_patches( *, findings: list[Finding], post_review: AsyncMock, - thread_metadata: dict[str, Any] | None = None, + thread_metadata: dict[str, Any] | None = _UNSET_METADATA, dismiss: AsyncMock | None = None, ) -> list[Any]: + if thread_metadata is _UNSET_METADATA: + thread_metadata = {"pr": {"author": "external-contributor"}} return [ 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)), @@ -2656,3 +2664,171 @@ async def test_publish_async_comment_publish_does_not_dismiss_without_findings() ) dismiss.assert_not_awaited() + + +async def test_publish_async_verdict_downgraded_when_head_moves_mid_run() -> None: + """A mid-run push that moves the head must downgrade a verdict to a comment + rather than stamp an approval on an unreviewed commit.""" + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 2001, "state": "COMMENTED"}) + with ExitStack() as stack: + for p in _verdict_publish_patches(findings=[], post_review=post_review): + stack.enter_context(p) + # resolved head differs from the reviewed (config) head → drift. + stack.enter_context( + patch( + "agent.tools.publish_review.resolve_review_head_sha", + AsyncMock(return_value="newhead"), + ) + ) + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="reviewedhead", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + verdict="approve", + verdict_requester="amoussa1229", + ) + + assert result["verdict_ignored"] is True + assert result["verdict_ignored_reason"] == "head_moved" + assert post_review.await_args.kwargs["event"] == "COMMENT" + + +async def test_publish_async_verdict_fails_closed_on_unknown_author() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 2002, "state": "COMMENTED"}) + with ExitStack() as stack: + # No pr author in metadata → cannot confirm it isn't a bot self-review. + for p in _verdict_publish_patches(findings=[], post_review=post_review, thread_metadata={}): + stack.enter_context(p) + 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, + verdict="approve", + verdict_requester="amoussa1229", + ) + + assert result["verdict_ignored"] is True + assert result["verdict_ignored_reason"] == "author_unknown" + assert post_review.await_args.kwargs["event"] == "COMMENT" + + +async def test_publish_async_self_review_guard_is_case_insensitive() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 2003, "state": "COMMENTED"}) + with ExitStack() as stack: + for p in _verdict_publish_patches( + findings=[], + post_review=post_review, + thread_metadata={"pr": {"author": "Seahaven-OpenSWE[bot]"}}, + ): + stack.enter_context(p) + 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, + verdict="approve", + verdict_requester="amoussa1229", + ) + + assert result["verdict_ignored_reason"] == "self_review" + assert post_review.await_args.kwargs["event"] == "COMMENT" + + +async def test_publish_async_verdict_lands_when_all_findings_out_of_diff() -> None: + """When every finding anchors outside the diff, an authorized verdict must + still post as a bodied review with zero inline comments.""" + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)] + # First POST 422s on the anchor; the verdict-only retry succeeds. + post_review = AsyncMock( + side_effect=[ + {"_error": "HTTP 422", "_error_kind": "unresolved_anchor", "_status": 422}, + {"id": 2004, "state": "CHANGES_REQUESTED"}, + ] + ) + with ExitStack() as stack: + for p in _verdict_publish_patches(findings=findings, post_review=post_review): + stack.enter_context(p) + # Force _filter_against_pr_diff to drop everything (no valid comments). + stack.enter_context( + patch( + "agent.tools.publish_review._filter_against_pr_diff", + AsyncMock(return_value=([], ["f_1"])), + ) + ) + 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, + verdict="request_changes", + verdict_requester="amoussa1229", + ) + + assert result["success"] is True + assert result["verdict_submitted"] is True + assert result["verdict_event"] == "REQUEST_CHANGES" + # Second (retry) call posts the verdict with no inline comments. + assert post_review.await_args.kwargs["event"] == "REQUEST_CHANGES" + assert post_review.await_args.kwargs["inline_comments"] == [] + + +async def test_publish_async_verdict_not_submitted_when_github_coerces_state() -> None: + """GitHub can accept the POST but land the review as COMMENTED; the result + must not claim a verdict in that case.""" + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 2005, "state": "COMMENTED"}) + with ExitStack() as stack: + for p in _verdict_publish_patches(findings=[], post_review=post_review): + stack.enter_context(p) + 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, + verdict="approve", + verdict_requester="amoussa1229", + ) + + assert result["success"] is True + assert result.get("verdict_submitted") is not True