diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index f1254873..564ddf08 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -127,7 +127,8 @@ async def publish_review( ``"approve_with_open_findings"``, ``"request_changes_without_open_findings"``, ``"self_review"``, ``"head_moved"`` — the reviewed commit is no longer the PR head — - ``"author_unknown"``, or ``"github_state_mismatch"``). + ``"head_check_failed"``, ``"author_unknown"``, or + ``"github_state_mismatch"``). """ if severity_threshold not in {"low", "medium", "high", "critical"}: return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"} @@ -595,8 +596,9 @@ async def _publish_review_async( ) if verdict_attempted and not verdict_submitted and verdict_ignored_reason is None: verdict_ignored_reason = "github_state_mismatch" + body_update_failed = False if verdict_attempted and isinstance(review_id, int) and recorded_state: - await update_pull_request_review_body( + body_updated = await update_pull_request_review_body( owner=owner, repo=repo, pr_number=pr_number, @@ -606,11 +608,13 @@ async def _publish_review_async( recorded_state=recorded_state, verdict=verdict, verdict_requester=verdict_requester, + verdict_authorization=verdict_authorization, verdict_submitted=verdict_submitted, verdict_ignored_reason=verdict_ignored_reason, ), token=token, ) + body_update_failed = body_updated is not True await _reconcile_last_verdict( thread_id=thread_id, owner=owner, @@ -718,6 +722,12 @@ async def _publish_review_async( result["verdict_submitted"] = False result["verdict_ignored"] = True result["verdict_ignored_reason"] = verdict_ignored_reason + if body_update_failed: + result["body_update_failed"] = True + result["body_update_message"] = ( + "GitHub recorded the review state, but updating the review body failed. " + "The original body remains neutral and does not claim a verdict." + ) if unresolvable_findings: result["unresolvable_findings"] = unresolvable_findings result["hint"] = ( @@ -806,7 +816,10 @@ async def _post_review_guarded( pr_number=pr_number, token=token, ) - if live_head_sha is not None and live_head_sha != reviewed_head_sha: + if live_head_sha is None: + submitted_verdict = None + ignored_reason = "head_check_failed" + elif live_head_sha != reviewed_head_sha: submitted_verdict = None ignored_reason = "head_moved" post_head_sha = live_head_sha @@ -815,6 +828,7 @@ async def _post_review_guarded( decorated_body = _decorate_review_body( body, verdict=submitted_verdict, + verdict_authorization=verdict_authorization, verdict_requester=verdict_requester, verdict_ignored_reason=ignored_reason, ) @@ -876,13 +890,19 @@ def _decorate_review_body( body: str, *, verdict: str | None, + verdict_authorization: str, verdict_requester: str, verdict_ignored_reason: str | None, ) -> str: """Append verdict attribution / downgrade context to the review body.""" if verdict is not None: - requester = f"@{verdict_requester}" if verdict_requester else "the requester" - return f"{body}\n\nVerdict (`{verdict}`) requested by {requester}." + if verdict_authorization == "consistent": + return ( + f"{body}\n\nAutomatic verdict evaluation (`{verdict}`) pending " + "based on authoritative finding state." + ) + requester = f"@{verdict_requester}" if verdict_requester else "an explicit human request" + return f"{body}\n\nVerdict (`{verdict}`) pending for {requester}." if verdict_ignored_reason: reason = verdict_ignored_reason.replace("_", " ") return f"{body}\n\n> Verdict withheld ({reason}). Published as a comment review." @@ -895,11 +915,17 @@ def _decorate_recorded_review_body( recorded_state: str, verdict: str | None, verdict_requester: str, + verdict_authorization: str, verdict_submitted: bool, verdict_ignored_reason: str | None, ) -> str: if verdict_submitted and verdict is not None: - requester = f"@{verdict_requester}" if verdict_requester else "the requester" + if verdict_authorization == "consistent": + return ( + f"{body}\n\nReview outcome: **{recorded_state}**. " + f"Automatic verdict (`{verdict}`) recorded based on authoritative finding state." + ) + requester = f"@{verdict_requester}" if verdict_requester else "an explicit human request" return ( f"{body}\n\nReview outcome: **{recorded_state}**. " f"Verdict (`{verdict}`) recorded for {requester}." diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index bb308543..6c9109c7 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -2569,7 +2569,10 @@ def _verdict_publish_patches( "agent.tools.publish_review.fetch_pull_request_head_sha", AsyncMock(return_value="sha"), ), - patch("agent.tools.publish_review.update_pull_request_review_body", AsyncMock()), + patch( + "agent.tools.publish_review.update_pull_request_review_body", + AsyncMock(return_value=True), + ), patch( "agent.tools.publish_review.dismiss_pull_request_review", dismiss or AsyncMock(return_value=True), @@ -2608,7 +2611,7 @@ async def test_publish_async_approve_with_zero_findings_bypasses_empty_skip() -> assert "skipped_empty_re_review" not in result assert post_review.await_args.kwargs["event"] == "APPROVE" body = post_review.await_args.kwargs["body"] - assert "Verdict (`approve`) requested by @amoussa1229." in body + assert "Verdict (`approve`) pending for @amoussa1229." in body async def test_publish_async_consistent_approve_clean_posts_approve() -> None: @@ -2639,7 +2642,8 @@ async def test_publish_async_consistent_approve_clean_posts_approve() -> None: assert result["verdict_event"] == "APPROVE" assert post_review.await_args.kwargs["event"] == "APPROVE" body = post_review.await_args.kwargs["body"] - assert "Verdict (`approve`) requested by the requester." in body + assert "Automatic verdict evaluation (`approve`) pending" in body + assert "requester" not in body async def test_publish_async_consistent_approve_with_open_findings_downgrades() -> None: @@ -3060,6 +3064,9 @@ async def test_publish_async_verdict_authorization_matrix( assert post_review.await_args.kwargs["event"] == expected_event assert result.get("verdict_submitted") is should_submit assert f"Review outcome: **{recorded_state}**" in update_body.await_args.kwargs["body"] + if authorization == "consistent" and should_submit: + assert "Automatic verdict" in update_body.await_args.kwargs["body"] + assert "requester" not in update_body.await_args.kwargs["body"] if not should_submit: expected_reason = "verdict_not_requested" if authorization == "consistent": @@ -3072,6 +3079,103 @@ async def test_publish_async_verdict_authorization_matrix( assert "Published as a comment review" in post_review.await_args.kwargs["body"] +async def test_publish_async_body_update_failure_keeps_initial_body_neutral() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 4002, "state": "APPROVED"}) + update_body = AsyncMock(return_value=False) + with ExitStack() as stack: + for patcher in _verdict_publish_patches(findings=[], post_review=post_review): + stack.enter_context(patcher) + stack.enter_context( + patch("agent.tools.publish_review.update_pull_request_review_body", update_body) + ) + 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_authorization="consistent", + ) + + initial_body = post_review.await_args.kwargs["body"] + assert "recorded" not in initial_body.lower() + assert "submitted" not in initial_body.lower() + assert "review outcome" not in initial_body.lower() + assert result["verdict_submitted"] is True + assert result["body_update_failed"] is True + assert "original body remains neutral" in result["body_update_message"] + + +async def test_publish_async_unauthorized_verdict_posts_withheld_empty_re_review() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 4003, "state": "COMMENTED"}) + with ExitStack() as stack: + for patcher in _verdict_publish_patches(findings=[], post_review=post_review): + stack.enter_context(patcher) + 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=True, + verdict="approve", + verdict_authorization="none", + ) + + assert "skipped_empty_re_review" not in result + assert result["verdict_ignored_reason"] == "verdict_not_requested" + assert post_review.await_args.kwargs["event"] == "COMMENT" + assert "Verdict withheld (verdict not requested)" in post_review.await_args.kwargs["body"] + + +async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None: + from contextlib import ExitStack + + from agent.tools.publish_review import _publish_review_async + + post_review = AsyncMock(return_value={"id": 4004, "state": "COMMENTED"}) + with ExitStack() as stack: + for patcher in _verdict_publish_patches(findings=[], post_review=post_review): + stack.enter_context(patcher) + stack.enter_context( + patch( + "agent.tools.publish_review.fetch_pull_request_head_sha", + AsyncMock(return_value=None), + ) + ) + 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_authorization="consistent", + ) + + assert result["verdict_submitted"] is False + assert result["verdict_ignored_reason"] == "head_check_failed" + assert post_review.await_args.kwargs["event"] == "COMMENT" + assert "Verdict withheld (head check failed)" in post_review.await_args.kwargs["body"] + + async def test_publish_async_rechecks_live_head_immediately_before_verdict_post() -> None: from contextlib import ExitStack @@ -3101,7 +3205,7 @@ async def test_publish_async_rechecks_live_head_immediately_before_verdict_post( assert lock_state["held"] is True order.append("post") assert kwargs["event"] == "COMMENT" - return {"id": 4002, "state": "COMMENTED"} + return {"id": 4005, "state": "COMMENTED"} post = AsyncMock(side_effect=post_review) with ExitStack() as stack: