mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-05 06:02:15 +00:00
fix(reviewer): handle verdict publication edge cases
This commit is contained in:
parent
b2b5957233
commit
404b854544
2 changed files with 140 additions and 10 deletions
|
|
@ -127,7 +127,8 @@ async def publish_review(
|
||||||
``"approve_with_open_findings"``,
|
``"approve_with_open_findings"``,
|
||||||
``"request_changes_without_open_findings"``, ``"self_review"``,
|
``"request_changes_without_open_findings"``, ``"self_review"``,
|
||||||
``"head_moved"`` — the reviewed commit is no longer the PR head —
|
``"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"}:
|
if severity_threshold not in {"low", "medium", "high", "critical"}:
|
||||||
return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"}
|
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:
|
if verdict_attempted and not verdict_submitted and verdict_ignored_reason is None:
|
||||||
verdict_ignored_reason = "github_state_mismatch"
|
verdict_ignored_reason = "github_state_mismatch"
|
||||||
|
body_update_failed = False
|
||||||
if verdict_attempted and isinstance(review_id, int) and recorded_state:
|
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,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
|
|
@ -606,11 +608,13 @@ async def _publish_review_async(
|
||||||
recorded_state=recorded_state,
|
recorded_state=recorded_state,
|
||||||
verdict=verdict,
|
verdict=verdict,
|
||||||
verdict_requester=verdict_requester,
|
verdict_requester=verdict_requester,
|
||||||
|
verdict_authorization=verdict_authorization,
|
||||||
verdict_submitted=verdict_submitted,
|
verdict_submitted=verdict_submitted,
|
||||||
verdict_ignored_reason=verdict_ignored_reason,
|
verdict_ignored_reason=verdict_ignored_reason,
|
||||||
),
|
),
|
||||||
token=token,
|
token=token,
|
||||||
)
|
)
|
||||||
|
body_update_failed = body_updated is not True
|
||||||
await _reconcile_last_verdict(
|
await _reconcile_last_verdict(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -718,6 +722,12 @@ async def _publish_review_async(
|
||||||
result["verdict_submitted"] = False
|
result["verdict_submitted"] = False
|
||||||
result["verdict_ignored"] = True
|
result["verdict_ignored"] = True
|
||||||
result["verdict_ignored_reason"] = verdict_ignored_reason
|
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:
|
if unresolvable_findings:
|
||||||
result["unresolvable_findings"] = unresolvable_findings
|
result["unresolvable_findings"] = unresolvable_findings
|
||||||
result["hint"] = (
|
result["hint"] = (
|
||||||
|
|
@ -806,7 +816,10 @@ async def _post_review_guarded(
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
token=token,
|
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
|
submitted_verdict = None
|
||||||
ignored_reason = "head_moved"
|
ignored_reason = "head_moved"
|
||||||
post_head_sha = live_head_sha
|
post_head_sha = live_head_sha
|
||||||
|
|
@ -815,6 +828,7 @@ async def _post_review_guarded(
|
||||||
decorated_body = _decorate_review_body(
|
decorated_body = _decorate_review_body(
|
||||||
body,
|
body,
|
||||||
verdict=submitted_verdict,
|
verdict=submitted_verdict,
|
||||||
|
verdict_authorization=verdict_authorization,
|
||||||
verdict_requester=verdict_requester,
|
verdict_requester=verdict_requester,
|
||||||
verdict_ignored_reason=ignored_reason,
|
verdict_ignored_reason=ignored_reason,
|
||||||
)
|
)
|
||||||
|
|
@ -876,13 +890,19 @@ def _decorate_review_body(
|
||||||
body: str,
|
body: str,
|
||||||
*,
|
*,
|
||||||
verdict: str | None,
|
verdict: str | None,
|
||||||
|
verdict_authorization: str,
|
||||||
verdict_requester: str,
|
verdict_requester: str,
|
||||||
verdict_ignored_reason: str | None,
|
verdict_ignored_reason: str | None,
|
||||||
) -> str:
|
) -> str:
|
||||||
"""Append verdict attribution / downgrade context to the review body."""
|
"""Append verdict attribution / downgrade context to the review body."""
|
||||||
if verdict is not None:
|
if verdict is not None:
|
||||||
requester = f"@{verdict_requester}" if verdict_requester else "the requester"
|
if verdict_authorization == "consistent":
|
||||||
return f"{body}\n\nVerdict (`{verdict}`) requested by {requester}."
|
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:
|
if verdict_ignored_reason:
|
||||||
reason = verdict_ignored_reason.replace("_", " ")
|
reason = verdict_ignored_reason.replace("_", " ")
|
||||||
return f"{body}\n\n> Verdict withheld ({reason}). Published as a comment review."
|
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,
|
recorded_state: str,
|
||||||
verdict: str | None,
|
verdict: str | None,
|
||||||
verdict_requester: str,
|
verdict_requester: str,
|
||||||
|
verdict_authorization: str,
|
||||||
verdict_submitted: bool,
|
verdict_submitted: bool,
|
||||||
verdict_ignored_reason: str | None,
|
verdict_ignored_reason: str | None,
|
||||||
) -> str:
|
) -> str:
|
||||||
if verdict_submitted and verdict is not None:
|
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 (
|
return (
|
||||||
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
||||||
f"Verdict (`{verdict}`) recorded for {requester}."
|
f"Verdict (`{verdict}`) recorded for {requester}."
|
||||||
|
|
|
||||||
|
|
@ -2569,7 +2569,10 @@ def _verdict_publish_patches(
|
||||||
"agent.tools.publish_review.fetch_pull_request_head_sha",
|
"agent.tools.publish_review.fetch_pull_request_head_sha",
|
||||||
AsyncMock(return_value="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(
|
patch(
|
||||||
"agent.tools.publish_review.dismiss_pull_request_review",
|
"agent.tools.publish_review.dismiss_pull_request_review",
|
||||||
dismiss or AsyncMock(return_value=True),
|
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 "skipped_empty_re_review" not in result
|
||||||
assert post_review.await_args.kwargs["event"] == "APPROVE"
|
assert post_review.await_args.kwargs["event"] == "APPROVE"
|
||||||
body = post_review.await_args.kwargs["body"]
|
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:
|
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 result["verdict_event"] == "APPROVE"
|
||||||
assert post_review.await_args.kwargs["event"] == "APPROVE"
|
assert post_review.await_args.kwargs["event"] == "APPROVE"
|
||||||
body = post_review.await_args.kwargs["body"]
|
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:
|
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 post_review.await_args.kwargs["event"] == expected_event
|
||||||
assert result.get("verdict_submitted") is should_submit
|
assert result.get("verdict_submitted") is should_submit
|
||||||
assert f"Review outcome: **{recorded_state}**" in update_body.await_args.kwargs["body"]
|
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:
|
if not should_submit:
|
||||||
expected_reason = "verdict_not_requested"
|
expected_reason = "verdict_not_requested"
|
||||||
if authorization == "consistent":
|
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"]
|
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:
|
async def test_publish_async_rechecks_live_head_immediately_before_verdict_post() -> None:
|
||||||
from contextlib import ExitStack
|
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
|
assert lock_state["held"] is True
|
||||||
order.append("post")
|
order.append("post")
|
||||||
assert kwargs["event"] == "COMMENT"
|
assert kwargs["event"] == "COMMENT"
|
||||||
return {"id": 4002, "state": "COMMENTED"}
|
return {"id": 4005, "state": "COMMENTED"}
|
||||||
|
|
||||||
post = AsyncMock(side_effect=post_review)
|
post = AsyncMock(side_effect=post_review)
|
||||||
with ExitStack() as stack:
|
with ExitStack() as stack:
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue