From 4fea5f21c57edaa713afee6bbf3b8743d77c3f19 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Fri, 31 Jul 2026 13:34:29 -0400 Subject: [PATCH] fix(reviewer): enforce blocking review checks --- agent/tools/publish_review.py | 4 -- agent/utils/github_checks.py | 39 ++++++++++-------- tests/github/test_github_checks.py | 27 +++++++++++++ tests/github/test_github_issue_webhook.py | 25 ++++++++++-- tests/reviewer/test_reviewer_publish.py | 49 ++++++++++++++++++++--- 5 files changed, 113 insertions(+), 31 deletions(-) diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index c34ee753..729e77b4 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -415,10 +415,6 @@ async def _publish_review_async( ), "verdict_authorization": verdict_authorization, } - if verdict_ignored_reason: - skip_result["verdict_submitted"] = False - skip_result["verdict_ignored"] = True - skip_result["verdict_ignored_reason"] = verdict_ignored_reason conclusion, check_title, check_summary = review_check_conclusion(skip_result) await settle_review_check_run( thread_id=thread_id, diff --git a/agent/utils/github_checks.py b/agent/utils/github_checks.py index 7e522bb9..055df892 100644 --- a/agent/utils/github_checks.py +++ b/agent/utils/github_checks.py @@ -180,6 +180,28 @@ def review_check_conclusion( else None ) ignored_reason = publish_outcome.get("verdict_ignored_reason") + verdict_authorization = publish_outcome.get("verdict_authorization") + if ( + blocking_count is not None + and blocking_count > 0 + and verdict_authorization + in { + "requested", + "consistent", + } + and ignored_reason + in { + None, + "approve_with_open_findings", + "self_review", + } + ): + issue_word = "issue" if blocking_count == 1 else "issues" + return ( + "failure", + f"Found {blocking_count} blocking {issue_word}", + f"Open SWE found {blocking_count} blocking {issue_word} on this pull request.", + ) if ignored_reason and ignored_reason != "self_review": return ( "neutral", @@ -193,23 +215,6 @@ def review_check_conclusion( "Open SWE reviewed this pull request and found no blocking issues.", ) - verdict_authorization = publish_outcome.get("verdict_authorization") - if ( - blocking_count is not None - and blocking_count > 0 - and verdict_authorization - in { - "requested", - "consistent", - } - ): - issue_word = "issue" if blocking_count == 1 else "issues" - return ( - "failure", - f"Found {blocking_count} blocking {issue_word}", - f"Open SWE found {blocking_count} blocking {issue_word} on this pull request.", - ) - surfaced_count_raw = publish_outcome.get("surfaced_count") surfaced_count = ( surfaced_count_raw diff --git a/tests/github/test_github_checks.py b/tests/github/test_github_checks.py index 64e10b93..af62e783 100644 --- a/tests/github/test_github_checks.py +++ b/tests/github/test_github_checks.py @@ -181,6 +181,15 @@ async def test_post_autofix_status_check_completes_neutral( "failure", "Found 2 blocking issues", ), + ( + { + "blocking_finding_count": 2, + "verdict_authorization": "consistent", + "verdict_ignored_reason": "approve_with_open_findings", + }, + "failure", + "Found 2 blocking issues", + ), ( { "blocking_finding_count": 1, @@ -208,6 +217,24 @@ async def test_post_autofix_status_check_completes_neutral( "neutral", "Verdict withheld", ), + ( + { + "blocking_finding_count": 1, + "verdict_authorization": "consistent", + "verdict_ignored_reason": "author_unknown", + }, + "neutral", + "Verdict withheld", + ), + ( + { + "blocking_finding_count": 1, + "verdict_authorization": "consistent", + "verdict_ignored_reason": "head_check_failed", + }, + "neutral", + "Verdict withheld", + ), ( { "blocking_finding_count": 3, diff --git a/tests/github/test_github_issue_webhook.py b/tests/github/test_github_issue_webhook.py index 95d8af66..103c2196 100644 --- a/tests/github/test_github_issue_webhook.py +++ b/tests/github/test_github_issue_webhook.py @@ -8,6 +8,7 @@ import json import logging from unittest.mock import AsyncMock +import pytest from fastapi.testclient import TestClient from agent.api import app as api_app @@ -1155,8 +1156,16 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: } -def test_trigger_pr_review_from_ref_reuses_open_check_on_same_head(monkeypatch) -> None: +@pytest.mark.parametrize( + "tracked_head", + ["head-sha", "old-head-sha"], + ids=["same-head", "stale-check-different-head"], +) +def test_trigger_pr_review_from_ref_tracks_check_for_current_head( + monkeypatch, tracked_head: str +) -> None: created_check = AsyncMock(return_value=88) + set_metadata = AsyncMock() async def fake_token() -> tuple[str | None, str | None]: return "app-token", None @@ -1185,13 +1194,13 @@ def test_trigger_pr_review_from_ref_reuses_open_check_on_same_head(monkeypatch) monkeypatch.setattr( webhook_common, "_get_thread_metadata_safe", - AsyncMock(return_value={"review_check_run_id": 77, "head_sha": "head-sha"}), + AsyncMock(return_value={"review_check_run_id": 77, "head_sha": tracked_head}), ) monkeypatch.setattr( webhook_common, "_resolve_verdict_authorization", AsyncMock(return_value=True) ) monkeypatch.setattr(webhook_common, "cache_github_token_for_thread", lambda *a, **k: None) - monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", AsyncMock()) + monkeypatch.setattr(webhook_common, "set_reviewer_thread_metadata", set_metadata) monkeypatch.setattr(webhook_common, "create_review_check_run", created_check) monkeypatch.setattr(webhook_common, "post_review_started_comment", AsyncMock(return_value=1)) monkeypatch.setattr(webhook_common, "_store_current_reviewer_run_id", AsyncMock()) @@ -1211,7 +1220,15 @@ def test_trigger_pr_review_from_ref_reuses_open_check_on_same_head(monkeypatch) ) assert result["success"] is True - created_check.assert_not_awaited() + if tracked_head == "head-sha": + created_check.assert_not_awaited() + else: + created_check.assert_awaited_once() + assert created_check.await_args.kwargs["head_sha"] == "head-sha" + assert any( + call.kwargs.get("extra") == {"review_check_run_id": 88} + for call in set_metadata.await_args_list + ) def test_trigger_pr_review_from_ref_threads_verdict_request(monkeypatch) -> None: diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index 611c1388..cfc6c8a3 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -2628,8 +2628,13 @@ async def test_publish_async_consistent_approve_clean_posts_approve() -> None: from agent.tools.publish_review import _publish_review_async post_review = AsyncMock(return_value={"id": 2001, "state": "APPROVED"}) + settle_check = AsyncMock() with ExitStack() as stack: - for p in _verdict_publish_patches(findings=[], post_review=post_review): + for p in _verdict_publish_patches( + findings=[], + post_review=post_review, + settle_check=settle_check, + ): stack.enter_context(p) result = await _publish_review_async( owner="o", @@ -2652,6 +2657,7 @@ async def test_publish_async_consistent_approve_clean_posts_approve() -> None: body = post_review.await_args.kwargs["body"] assert "Automatic verdict evaluation (`approve`) pending" in body assert "requester" not in body + assert settle_check.await_args.kwargs["conclusion"] == "success" async def test_publish_async_consistent_approve_with_open_findings_downgrades() -> None: @@ -2661,8 +2667,13 @@ async def test_publish_async_consistent_approve_with_open_findings_downgrades() findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)] post_review = AsyncMock(return_value={"id": 2002, "state": "COMMENTED"}) + settle_check = AsyncMock() with ExitStack() as stack: - for p in _verdict_publish_patches(findings=findings, post_review=post_review): + for p in _verdict_publish_patches( + findings=findings, + post_review=post_review, + settle_check=settle_check, + ): stack.enter_context(p) result = await _publish_review_async( owner="o", @@ -2683,6 +2694,7 @@ async def test_publish_async_consistent_approve_with_open_findings_downgrades() assert result["verdict_ignored"] is True assert result["verdict_ignored_reason"] == "approve_with_open_findings" assert post_review.await_args.kwargs["event"] == "COMMENT" + assert settle_check.await_args.kwargs["conclusion"] == "failure" async def test_publish_async_requested_approve_ignores_open_findings_gate() -> None: @@ -2722,8 +2734,13 @@ async def test_publish_async_request_changes_maps_event() -> None: findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)] post_review = AsyncMock(return_value={"id": 1000, "state": "CHANGES_REQUESTED"}) + settle_check = AsyncMock() with ExitStack() as stack: - for p in _verdict_publish_patches(findings=findings, post_review=post_review): + for p in _verdict_publish_patches( + findings=findings, + post_review=post_review, + settle_check=settle_check, + ): stack.enter_context(p) result = await _publish_review_async( owner="o", @@ -2741,6 +2758,7 @@ async def test_publish_async_request_changes_maps_event() -> None: assert result["verdict_submitted"] is True assert result["verdict_event"] == "REQUEST_CHANGES" assert post_review.await_args.kwargs["event"] == "REQUEST_CHANGES" + assert settle_check.await_args.kwargs["conclusion"] == "failure" async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None: @@ -2918,9 +2936,15 @@ async def test_publish_async_verdict_fails_closed_on_unknown_author() -> None: from agent.tools.publish_review import _publish_review_async post_review = AsyncMock(return_value={"id": 2002, "state": "COMMENTED"}) + settle_check = AsyncMock() 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={}): + for p in _verdict_publish_patches( + findings=[], + post_review=post_review, + thread_metadata={}, + settle_check=settle_check, + ): stack.enter_context(p) result = await _publish_review_async( owner="o", @@ -2938,6 +2962,7 @@ async def test_publish_async_verdict_fails_closed_on_unknown_author() -> None: assert result["verdict_ignored"] is True assert result["verdict_ignored_reason"] == "author_unknown" assert post_review.await_args.kwargs["event"] == "COMMENT" + assert settle_check.await_args.kwargs["conclusion"] == "neutral" async def test_publish_async_self_review_guard_is_case_insensitive() -> None: @@ -3192,8 +3217,13 @@ async def test_publish_async_unauthorized_verdict_posts_withheld_empty_re_review from agent.tools.publish_review import _publish_review_async post_review = AsyncMock(return_value={"id": 4003, "state": "COMMENTED"}) + settle_check = AsyncMock() with ExitStack() as stack: - for patcher in _verdict_publish_patches(findings=[], post_review=post_review): + for patcher in _verdict_publish_patches( + findings=[], + post_review=post_review, + settle_check=settle_check, + ): stack.enter_context(patcher) result = await _publish_review_async( owner="o", @@ -3212,6 +3242,7 @@ async def test_publish_async_unauthorized_verdict_posts_withheld_empty_re_review 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"] + assert settle_check.await_args.kwargs["conclusion"] == "neutral" async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None: @@ -3221,8 +3252,13 @@ async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None post_review = AsyncMock(return_value={"id": 4004, "state": "COMMENTED"}) update_body = AsyncMock(return_value=False) + settle_check = AsyncMock() with ExitStack() as stack: - for patcher in _verdict_publish_patches(findings=[], post_review=post_review): + for patcher in _verdict_publish_patches( + findings=[], + post_review=post_review, + settle_check=settle_check, + ): stack.enter_context(patcher) stack.enter_context( patch( @@ -3252,6 +3288,7 @@ async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None assert "Verdict withheld (head check failed)" in post_review.await_args.kwargs["body"] update_body.assert_not_awaited() assert "body_update_failed" not in result + assert settle_check.await_args.kwargs["conclusion"] == "neutral" async def test_publish_async_rechecks_live_head_immediately_before_verdict_post() -> None: