adam/open-swe
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/open-swe.git synced 2026-09-30 06:53:14 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix(reviewer): enforce blocking review checks

Browse source
This commit is contained in:
Adam Moussa 2026-07-31 13:34:29 -04:00 • committed by Adam Moussa
parent 39c768aa3f
commit 4fea5f21c5
5 changed files with 113 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
agent/tools/publish_review.py
View file

@ -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,

39
agent/utils/github_checks.py
View file

@ -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

27
tests/github/test_github_checks.py
View file

@ -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,

25
tests/github/test_github_issue_webhook.py
View file

@ -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:

49
tests/reviewer/test_reviewer_publish.py
View file

@ -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: