mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-06 17:02:14 +00:00
fix(reviewer): clarify recorded verdict outcomes
This commit is contained in:
parent
404b854544
commit
9a2535a9ec
2 changed files with 43 additions and 5 deletions
|
|
@ -597,7 +597,7 @@ 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
|
body_update_failed = False
|
||||||
if verdict_attempted and isinstance(review_id, int) and recorded_state:
|
if verdict_submitted and isinstance(review_id, int) and recorded_state:
|
||||||
body_updated = await update_pull_request_review_body(
|
body_updated = await update_pull_request_review_body(
|
||||||
owner=owner,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
|
|
@ -931,6 +931,11 @@ def _decorate_recorded_review_body(
|
||||||
f"Verdict (`{verdict}`) recorded for {requester}."
|
f"Verdict (`{verdict}`) recorded for {requester}."
|
||||||
)
|
)
|
||||||
reason = (verdict_ignored_reason or "github_state_mismatch").replace("_", " ")
|
reason = (verdict_ignored_reason or "github_state_mismatch").replace("_", " ")
|
||||||
|
if verdict_authorization == "consistent":
|
||||||
|
return (
|
||||||
|
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
||||||
|
f"The automatic verdict evaluation was withheld ({reason})."
|
||||||
|
)
|
||||||
return (
|
return (
|
||||||
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
||||||
f"The requested verdict was withheld ({reason})."
|
f"The requested verdict was withheld ({reason})."
|
||||||
|
|
|
||||||
|
|
@ -2982,9 +2982,13 @@ async def test_publish_async_verdict_not_submitted_when_github_state_mismatches(
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
post_review = AsyncMock(return_value={"id": 2005, "state": recorded_state})
|
post_review = AsyncMock(return_value={"id": 2005, "state": recorded_state})
|
||||||
|
update_body = AsyncMock(return_value=True)
|
||||||
with ExitStack() as stack:
|
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):
|
||||||
stack.enter_context(p)
|
stack.enter_context(p)
|
||||||
|
stack.enter_context(
|
||||||
|
patch("agent.tools.publish_review.update_pull_request_review_body", update_body)
|
||||||
|
)
|
||||||
result = await _publish_review_async(
|
result = await _publish_review_async(
|
||||||
owner="o",
|
owner="o",
|
||||||
repo="r",
|
repo="r",
|
||||||
|
|
@ -3006,6 +3010,8 @@ async def test_publish_async_verdict_not_submitted_when_github_state_mismatches(
|
||||||
else:
|
else:
|
||||||
assert result["review_state"] == recorded_state
|
assert result["review_state"] == recorded_state
|
||||||
assert "submitted" not in post_review.await_args.kwargs["body"].lower()
|
assert "submitted" not in post_review.await_args.kwargs["body"].lower()
|
||||||
|
update_body.assert_not_awaited()
|
||||||
|
assert "body_update_failed" not in result
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("finding_state", ["clean", "open", "needs_reassessment"])
|
@pytest.mark.parametrize("finding_state", ["clean", "open", "needs_reassessment"])
|
||||||
|
|
@ -3063,10 +3069,14 @@ 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"]
|
if should_submit:
|
||||||
if authorization == "consistent" and should_submit:
|
assert f"Review outcome: **{recorded_state}**" in update_body.await_args.kwargs["body"]
|
||||||
assert "Automatic verdict" in update_body.await_args.kwargs["body"]
|
if authorization == "consistent":
|
||||||
assert "requester" not in update_body.await_args.kwargs["body"]
|
assert "Automatic verdict" in update_body.await_args.kwargs["body"]
|
||||||
|
assert "requester" not in update_body.await_args.kwargs["body"]
|
||||||
|
else:
|
||||||
|
update_body.assert_not_awaited()
|
||||||
|
assert "body_update_failed" not in result
|
||||||
if not should_submit:
|
if not should_submit:
|
||||||
expected_reason = "verdict_not_requested"
|
expected_reason = "verdict_not_requested"
|
||||||
if authorization == "consistent":
|
if authorization == "consistent":
|
||||||
|
|
@ -3079,6 +3089,23 @@ 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"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_recorded_body_uses_automatic_wording_for_consistent_withheld_verdict() -> None:
|
||||||
|
from agent.tools.publish_review import _decorate_recorded_review_body
|
||||||
|
|
||||||
|
body = _decorate_recorded_review_body(
|
||||||
|
"summary",
|
||||||
|
recorded_state="COMMENTED",
|
||||||
|
verdict="approve",
|
||||||
|
verdict_requester="",
|
||||||
|
verdict_authorization="consistent",
|
||||||
|
verdict_submitted=False,
|
||||||
|
verdict_ignored_reason="approve_with_open_findings",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert "automatic verdict evaluation was withheld" in body.lower()
|
||||||
|
assert "requested" not in body.lower()
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_body_update_failure_keeps_initial_body_neutral() -> None:
|
async def test_publish_async_body_update_failure_keeps_initial_body_neutral() -> None:
|
||||||
from contextlib import ExitStack
|
from contextlib import ExitStack
|
||||||
|
|
||||||
|
|
@ -3148,6 +3175,7 @@ async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
post_review = AsyncMock(return_value={"id": 4004, "state": "COMMENTED"})
|
post_review = AsyncMock(return_value={"id": 4004, "state": "COMMENTED"})
|
||||||
|
update_body = AsyncMock(return_value=False)
|
||||||
with ExitStack() as stack:
|
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):
|
||||||
stack.enter_context(patcher)
|
stack.enter_context(patcher)
|
||||||
|
|
@ -3157,6 +3185,9 @@ async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None
|
||||||
AsyncMock(return_value=None),
|
AsyncMock(return_value=None),
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
stack.enter_context(
|
||||||
|
patch("agent.tools.publish_review.update_pull_request_review_body", update_body)
|
||||||
|
)
|
||||||
result = await _publish_review_async(
|
result = await _publish_review_async(
|
||||||
owner="o",
|
owner="o",
|
||||||
repo="r",
|
repo="r",
|
||||||
|
|
@ -3174,6 +3205,8 @@ async def test_publish_async_failed_live_head_check_downgrades_verdict() -> None
|
||||||
assert result["verdict_ignored_reason"] == "head_check_failed"
|
assert result["verdict_ignored_reason"] == "head_check_failed"
|
||||||
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
||||||
assert "Verdict withheld (head check failed)" in post_review.await_args.kwargs["body"]
|
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
|
||||||
|
|
||||||
|
|
||||||
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:
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue