mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-03 17:53:20 +00:00
fix(reviewer): harden verdict enforcement
This commit is contained in:
parent
eb91104db8
commit
b2b5957233
6 changed files with 531 additions and 215 deletions
|
|
@ -82,7 +82,7 @@ def normalize_finding_title(title: str | None, description: str = "") -> str:
|
||||||
|
|
||||||
Severity = Literal["low", "medium", "high", "critical"]
|
Severity = Literal["low", "medium", "high", "critical"]
|
||||||
Confidence = Literal["low", "medium", "high"]
|
Confidence = Literal["low", "medium", "high"]
|
||||||
FindingStatus = Literal["open", "resolved", "dismissed"]
|
FindingStatus = Literal["open", "needs_reassessment", "resolved", "dismissed"]
|
||||||
DiffSide = Literal["LEFT", "RIGHT"]
|
DiffSide = Literal["LEFT", "RIGHT"]
|
||||||
SurfaceState = Literal["not_surfaced", "surfaced", "resolve_pending", "resolved", "error"]
|
SurfaceState = Literal["not_surfaced", "surfaced", "resolve_pending", "resolved", "error"]
|
||||||
InteractionKind = Literal["human_reply", "bot_reply"]
|
InteractionKind = Literal["human_reply", "bot_reply"]
|
||||||
|
|
|
||||||
|
|
@ -540,6 +540,28 @@ async def open_swe_review_exists(
|
||||||
params["page"] += 1
|
params["page"] += 1
|
||||||
|
|
||||||
|
|
||||||
|
async def fetch_pull_request_head_sha(
|
||||||
|
*,
|
||||||
|
owner: str,
|
||||||
|
repo: str,
|
||||||
|
pr_number: int,
|
||||||
|
token: str,
|
||||||
|
) -> str | None:
|
||||||
|
"""Fetch the PR head directly from GitHub immediately before a verdict."""
|
||||||
|
url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/pulls/{pr_number}"
|
||||||
|
async with github_client(token=token) as client:
|
||||||
|
try:
|
||||||
|
response = await github_request(client, "GET", url)
|
||||||
|
response.raise_for_status()
|
||||||
|
except httpx.HTTPError:
|
||||||
|
logger.exception("Failed to fetch live PR head for %s/%s#%s", owner, repo, pr_number)
|
||||||
|
return None
|
||||||
|
payload = response.json()
|
||||||
|
head = payload.get("head") if isinstance(payload, dict) else None
|
||||||
|
sha = head.get("sha") if isinstance(head, dict) else None
|
||||||
|
return sha if isinstance(sha, str) and sha else None
|
||||||
|
|
||||||
|
|
||||||
_REVIEW_EVENTS = {"COMMENT", "APPROVE", "REQUEST_CHANGES"}
|
_REVIEW_EVENTS = {"COMMENT", "APPROVE", "REQUEST_CHANGES"}
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -636,6 +658,33 @@ async def post_pull_request_review(
|
||||||
return {"_error": (f"HTTP {response.status_code}: non-dict response body: {body_excerpt}")}
|
return {"_error": (f"HTTP {response.status_code}: non-dict response body: {body_excerpt}")}
|
||||||
|
|
||||||
|
|
||||||
|
async def update_pull_request_review_body(
|
||||||
|
*,
|
||||||
|
owner: str,
|
||||||
|
repo: str,
|
||||||
|
pr_number: int,
|
||||||
|
review_id: int,
|
||||||
|
body: str,
|
||||||
|
token: str,
|
||||||
|
) -> bool:
|
||||||
|
"""Replace a submitted review body with its recorded GitHub outcome."""
|
||||||
|
url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}"
|
||||||
|
async with github_client(token=token) as client:
|
||||||
|
try:
|
||||||
|
response = await github_request(client, "PUT", url, json={"body": body})
|
||||||
|
response.raise_for_status()
|
||||||
|
except httpx.HTTPError:
|
||||||
|
logger.exception(
|
||||||
|
"Failed to update PR review body for %s/%s#%s review %s",
|
||||||
|
owner,
|
||||||
|
repo,
|
||||||
|
pr_number,
|
||||||
|
review_id,
|
||||||
|
)
|
||||||
|
return False
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
async def dismiss_pull_request_review(
|
async def dismiss_pull_request_review(
|
||||||
*,
|
*,
|
||||||
owner: str,
|
owner: str,
|
||||||
|
|
|
||||||
|
|
@ -176,12 +176,28 @@ def _sync_thread_status(finding: Finding, matches: list[ReviewThreadMatch]) -> b
|
||||||
|
|
||||||
if resolved_thread_ids != _str_list(finding.get("github_resolved_thread_ids")):
|
if resolved_thread_ids != _str_list(finding.get("github_resolved_thread_ids")):
|
||||||
finding["github_resolved_thread_ids"] = resolved_thread_ids
|
finding["github_resolved_thread_ids"] = resolved_thread_ids
|
||||||
if not all_resolved:
|
status = finding.get("status", "open")
|
||||||
|
if status in {"open", "needs_reassessment"}:
|
||||||
|
if status != "needs_reassessment":
|
||||||
|
finding["status"] = "needs_reassessment"
|
||||||
|
updated = True
|
||||||
|
note = (
|
||||||
|
"GitHub review thread was resolved or outdated by the pull request author; "
|
||||||
|
"reassess the finding before changing its status."
|
||||||
|
)
|
||||||
|
if finding.get("last_reconciliation_note") != note:
|
||||||
|
finding["last_reconciliation_note"] = note
|
||||||
|
updated = True
|
||||||
|
if isinstance(finding.get("id"), str):
|
||||||
|
surface = _coerce_surface(finding, str(finding["id"]))
|
||||||
|
if surface.get("state") != "surfaced":
|
||||||
|
surface["state"] = "surfaced"
|
||||||
|
updated = True
|
||||||
|
finding["surface"] = surface
|
||||||
return updated
|
return updated
|
||||||
|
|
||||||
if finding.get("status") == "open":
|
if not all_resolved:
|
||||||
finding["status"] = "resolved"
|
return updated
|
||||||
updated = True
|
|
||||||
if not finding.get("github_thread_resolved"):
|
if not finding.get("github_thread_resolved"):
|
||||||
finding["github_thread_resolved"] = True
|
finding["github_thread_resolved"] = True
|
||||||
updated = True
|
updated = True
|
||||||
|
|
|
||||||
|
|
@ -16,7 +16,10 @@ from ..review.findings import (
|
||||||
Finding,
|
Finding,
|
||||||
ReviewerThreadMissingError,
|
ReviewerThreadMissingError,
|
||||||
Severity,
|
Severity,
|
||||||
|
_coerce_findings_list,
|
||||||
_coerce_surface,
|
_coerce_surface,
|
||||||
|
_finding_mutation_lock,
|
||||||
|
_get_thread_metadata_strict,
|
||||||
filter_findings_for_publish,
|
filter_findings_for_publish,
|
||||||
get_thread_id_from_runtime,
|
get_thread_id_from_runtime,
|
||||||
get_thread_last_reviewed_sha,
|
get_thread_last_reviewed_sha,
|
||||||
|
|
@ -35,6 +38,7 @@ from ..review.publish import (
|
||||||
clear_review_started_comment,
|
clear_review_started_comment,
|
||||||
dismiss_pull_request_review,
|
dismiss_pull_request_review,
|
||||||
fetch_pr_review_threads,
|
fetch_pr_review_threads,
|
||||||
|
fetch_pull_request_head_sha,
|
||||||
fetch_review_comments,
|
fetch_review_comments,
|
||||||
fetch_review_thread_id_for_comment,
|
fetch_review_thread_id_for_comment,
|
||||||
open_swe_review_exists,
|
open_swe_review_exists,
|
||||||
|
|
@ -46,6 +50,7 @@ from ..review.publish import (
|
||||||
reply_to_review_comment,
|
reply_to_review_comment,
|
||||||
resolve_review_thread,
|
resolve_review_thread,
|
||||||
settle_review_check_run,
|
settle_review_check_run,
|
||||||
|
update_pull_request_review_body,
|
||||||
)
|
)
|
||||||
from ..review.reconcile import reconcile_findings_with_review_threads
|
from ..review.reconcile import reconcile_findings_with_review_threads
|
||||||
from ..utils.dashboard_links import dashboard_review_url
|
from ..utils.dashboard_links import dashboard_review_url
|
||||||
|
|
@ -90,16 +95,12 @@ async def publish_review(
|
||||||
mentioned in the review summary with a link to the web app, but are
|
mentioned in the review summary with a link to the web app, but are
|
||||||
not posted as inline PR comments.
|
not posted as inline PR comments.
|
||||||
verdict: Optional review verdict — ``"approve"`` or
|
verdict: Optional review verdict — ``"approve"`` or
|
||||||
``"request_changes"``. ``"request_changes"`` is honored ONLY when
|
``"request_changes"``. Explicitly requested verdicts are honored
|
||||||
this run was explicitly authorized to submit a verdict (the
|
subject to the safety checks below. Automatically authorized
|
||||||
triggering user asked for one). ``"approve"`` is also honored on a
|
verdicts must agree with authoritative finding state: approve
|
||||||
run without that authorization when the review is clean — zero
|
requires no open findings and request_changes requires at least
|
||||||
open findings — so a clean review lands as a real APPROVE; with
|
one. A downgraded verdict is posted as a comment review and carries
|
||||||
open findings an unsolicited approve is downgraded to a plain
|
``verdict_ignored: true``.
|
||||||
comment and the result carries ``verdict_ignored: true``. Never
|
|
||||||
describe an ignored verdict as an approval. Verdicts are also
|
|
||||||
downgraded to a comment when the PR was authored by Open SWE
|
|
||||||
itself (self-review).
|
|
||||||
Returns:
|
Returns:
|
||||||
Dictionary with ``success``, ``review_id``, ``surfaced_count``,
|
Dictionary with ``success``, ``review_id``, ``surfaced_count``,
|
||||||
``hidden_count``, ``resolved_thread_count``, and sometimes
|
``hidden_count``, ``resolved_thread_count``, and sometimes
|
||||||
|
|
@ -123,9 +124,10 @@ async def publish_review(
|
||||||
``verdict_submitted`` (GitHub confirmed the requested APPROVE/
|
``verdict_submitted`` (GitHub confirmed the requested APPROVE/
|
||||||
REQUEST_CHANGES state) or ``verdict_ignored`` +
|
REQUEST_CHANGES state) or ``verdict_ignored`` +
|
||||||
``verdict_ignored_reason`` (``"verdict_not_requested"``,
|
``verdict_ignored_reason`` (``"verdict_not_requested"``,
|
||||||
``"approve_with_open_findings"`` — an unsolicited approve on a run
|
``"approve_with_open_findings"``,
|
||||||
with open findings — ``"self_review"``, ``"head_moved"`` — the
|
``"request_changes_without_open_findings"``, ``"self_review"``,
|
||||||
reviewed commit is no longer the PR head — or ``"author_unknown"``).
|
``"head_moved"`` — the reviewed commit is no longer the PR head —
|
||||||
|
``"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}"}
|
||||||
|
|
@ -182,24 +184,12 @@ async def publish_review(
|
||||||
if not token:
|
if not token:
|
||||||
return {"success": False, "error": "No GitHub token available"}
|
return {"success": False, "error": "No GitHub token available"}
|
||||||
|
|
||||||
# Verdict authorization is enforced here in code, not in the prompt.
|
if configurable.get("verdict_requested") is True:
|
||||||
# Explicit requests may submit either verdict. Automatic approvals require
|
verdict_authorization = "requested"
|
||||||
# the dispatch-set verdict_authorized flag and still pass the clean-review
|
elif configurable.get("verdict_authorized") is True:
|
||||||
# gate in _publish_review_async.
|
verdict_authorization = "consistent"
|
||||||
verdict_not_requested = False
|
else:
|
||||||
unsolicited_approve = False
|
verdict_authorization = "none"
|
||||||
if verdict is not None and configurable.get("verdict_requested") is not True:
|
|
||||||
if verdict == "approve" and configurable.get("verdict_authorized") is True:
|
|
||||||
unsolicited_approve = True
|
|
||||||
else:
|
|
||||||
logger.info(
|
|
||||||
"publish_review verdict %r dropped: run not authorized to submit verdicts "
|
|
||||||
"(pr_number=%s)",
|
|
||||||
verdict,
|
|
||||||
pr_number,
|
|
||||||
)
|
|
||||||
verdict = None
|
|
||||||
verdict_not_requested = True
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
result = await _publish_review_async(
|
result = await _publish_review_async(
|
||||||
|
|
@ -215,12 +205,8 @@ async def publish_review(
|
||||||
trace_link_config_override=configurable.get("review_trace_link_enabled"),
|
trace_link_config_override=configurable.get("review_trace_link_enabled"),
|
||||||
verdict=verdict,
|
verdict=verdict,
|
||||||
verdict_requester=str(configurable.get("github_login") or ""),
|
verdict_requester=str(configurable.get("github_login") or ""),
|
||||||
unsolicited_approve=unsolicited_approve,
|
verdict_authorization=verdict_authorization,
|
||||||
)
|
)
|
||||||
if verdict_not_requested:
|
|
||||||
result["verdict_ignored"] = True
|
|
||||||
result["verdict_ignored_reason"] = "verdict_not_requested"
|
|
||||||
result["verdict_submitted"] = False
|
|
||||||
return result
|
return result
|
||||||
except ReviewerThreadMissingError as exc:
|
except ReviewerThreadMissingError as exc:
|
||||||
return thread_missing_tool_result(exc)
|
return thread_missing_tool_result(exc)
|
||||||
|
|
@ -320,10 +306,11 @@ async def _publish_review_async(
|
||||||
trace_link_config_override: object = None,
|
trace_link_config_override: object = None,
|
||||||
verdict: str | None = None,
|
verdict: str | None = None,
|
||||||
verdict_requester: str = "",
|
verdict_requester: str = "",
|
||||||
unsolicited_approve: bool = False,
|
verdict_authorization: str = "requested",
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
thread_id = get_thread_id_from_runtime()
|
thread_id = get_thread_id_from_runtime()
|
||||||
verdict_ignored_reason: str | None = None
|
verdict_ignored_reason: str | None = None
|
||||||
|
verdict_attempted = verdict is not None
|
||||||
reviewed_head_sha = head_sha
|
reviewed_head_sha = head_sha
|
||||||
# The run config's head_sha is frozen at run creation; a push that arrived
|
# The run config's head_sha is frozen at run creation; a push that arrived
|
||||||
# mid-run updated the live head in thread metadata. Prefer that so the
|
# mid-run updated the live head in thread metadata. Prefer that so the
|
||||||
|
|
@ -331,56 +318,6 @@ async def _publish_review_async(
|
||||||
# reviewed, not the stale one this run was created for.
|
# reviewed, not the stale one this run was created for.
|
||||||
head_sha = await resolve_review_head_sha(thread_id, {"head_sha": head_sha})
|
head_sha = await resolve_review_head_sha(thread_id, {"head_sha": head_sha})
|
||||||
|
|
||||||
if verdict is not None:
|
|
||||||
metadata = await get_thread_metadata(thread_id)
|
|
||||||
# A verdict is merge-affecting (a real APPROVE/REQUEST_CHANGES), so it
|
|
||||||
# must reflect exactly the commit the agent reviewed. If a push landed
|
|
||||||
# mid-run and moved the head, the resolved head no longer matches what
|
|
||||||
# this run examined — downgrade to a comment rather than stamp an
|
|
||||||
# approval onto unreviewed code (and let the push's own re-review submit
|
|
||||||
# a fresh verdict). A plain comment publish still safely retargets.
|
|
||||||
if reviewed_head_sha and head_sha and head_sha != reviewed_head_sha:
|
|
||||||
logger.info(
|
|
||||||
"publish_review verdict %r downgraded to comment: head moved %s -> %s "
|
|
||||||
"mid-run for %s/%s#%s",
|
|
||||||
verdict,
|
|
||||||
reviewed_head_sha,
|
|
||||||
head_sha,
|
|
||||||
owner,
|
|
||||||
repo,
|
|
||||||
pr_number,
|
|
||||||
)
|
|
||||||
verdict = None
|
|
||||||
verdict_ignored_reason = "head_moved"
|
|
||||||
else:
|
|
||||||
pr_author = _pr_author_from_thread(metadata)
|
|
||||||
bot_logins = {login.casefold() for login in INTERNAL_BOT_LOGINS}
|
|
||||||
if not pr_author:
|
|
||||||
# Fail closed: a verdict on a PR whose author we cannot confirm
|
|
||||||
# might be a self-review on an Open SWE PR. Downgrade rather than
|
|
||||||
# risk approving our own work.
|
|
||||||
logger.info(
|
|
||||||
"publish_review verdict %r downgraded to comment: PR author "
|
|
||||||
"unknown for %s/%s#%s",
|
|
||||||
verdict,
|
|
||||||
owner,
|
|
||||||
repo,
|
|
||||||
pr_number,
|
|
||||||
)
|
|
||||||
verdict = None
|
|
||||||
verdict_ignored_reason = "author_unknown"
|
|
||||||
elif pr_author.casefold() in bot_logins:
|
|
||||||
logger.info(
|
|
||||||
"publish_review verdict %r downgraded to comment: PR %s/%s#%s was "
|
|
||||||
"authored by internal bot %r (self-review)",
|
|
||||||
verdict,
|
|
||||||
owner,
|
|
||||||
repo,
|
|
||||||
pr_number,
|
|
||||||
pr_author,
|
|
||||||
)
|
|
||||||
verdict = None
|
|
||||||
verdict_ignored_reason = "self_review"
|
|
||||||
findings = await _backfill_findings_from_pr_threads(
|
findings = await _backfill_findings_from_pr_threads(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -389,25 +326,6 @@ async def _publish_review_async(
|
||||||
token=token,
|
token=token,
|
||||||
)
|
)
|
||||||
|
|
||||||
# An unsolicited approve (no verdict_requested on the run) is honored only
|
|
||||||
# for a clean review: any open finding — new, previously published, or
|
|
||||||
# below the surfacing threshold — means changes are effectively being
|
|
||||||
# requested, so the approve downgrades to a comment.
|
|
||||||
if verdict == "approve" and unsolicited_approve:
|
|
||||||
open_findings_count = sum(1 for f in findings if f.get("status", "open") == "open")
|
|
||||||
if open_findings_count:
|
|
||||||
logger.info(
|
|
||||||
"publish_review unsolicited approve downgraded to comment: %d open "
|
|
||||||
"finding(s) for %s/%s#%s",
|
|
||||||
open_findings_count,
|
|
||||||
owner,
|
|
||||||
repo,
|
|
||||||
pr_number,
|
|
||||||
)
|
|
||||||
verdict = None
|
|
||||||
verdict_ignored_reason = "approve_with_open_findings"
|
|
||||||
|
|
||||||
event = _VERDICT_EVENTS.get(verdict or "", "COMMENT")
|
|
||||||
review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override)
|
review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override)
|
||||||
review_ui_url = dashboard_review_url(owner, repo, pr_number)
|
review_ui_url = dashboard_review_url(owner, repo, pr_number)
|
||||||
|
|
||||||
|
|
@ -465,7 +383,7 @@ async def _publish_review_async(
|
||||||
# plain comment publishes.
|
# plain comment publishes.
|
||||||
if (
|
if (
|
||||||
not inline_comments
|
not inline_comments
|
||||||
and verdict is None
|
and not verdict_attempted
|
||||||
and await _open_swe_already_reviewed(
|
and await _open_swe_already_reviewed(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -508,21 +426,15 @@ async def _publish_review_async(
|
||||||
skip_result["verdict_ignored_reason"] = verdict_ignored_reason
|
skip_result["verdict_ignored_reason"] = verdict_ignored_reason
|
||||||
return skip_result
|
return skip_result
|
||||||
|
|
||||||
review_body = _decorate_review_body(
|
review_body = render_review_body(
|
||||||
render_review_body(
|
pr_number=pr_number,
|
||||||
pr_number=pr_number,
|
surfaced_count=len(inline_comments),
|
||||||
surfaced_count=len(inline_comments),
|
trace_url=review_trace_url,
|
||||||
trace_url=review_trace_url,
|
ui_url=review_ui_url,
|
||||||
ui_url=review_ui_url,
|
additional_findings_count=additional_findings_count,
|
||||||
additional_findings_count=additional_findings_count,
|
|
||||||
),
|
|
||||||
verdict=verdict,
|
|
||||||
verdict_requester=verdict_requester,
|
|
||||||
verdict_ignored_reason=verdict_ignored_reason,
|
|
||||||
unsolicited_approve=unsolicited_approve,
|
|
||||||
)
|
)
|
||||||
|
review_response, event, verdict_ignored_reason, findings = await _post_review_guarded(
|
||||||
review_response = await post_pull_request_review(
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
|
|
@ -530,7 +442,10 @@ async def _publish_review_async(
|
||||||
body=review_body,
|
body=review_body,
|
||||||
inline_comments=inline_comments,
|
inline_comments=inline_comments,
|
||||||
token=token,
|
token=token,
|
||||||
event=event,
|
verdict=verdict,
|
||||||
|
verdict_authorization=verdict_authorization,
|
||||||
|
verdict_requester=verdict_requester,
|
||||||
|
reviewed_head_sha=reviewed_head_sha,
|
||||||
)
|
)
|
||||||
# If GitHub rejected the batch because one or more inline comments anchor
|
# If GitHub rejected the batch because one or more inline comments anchor
|
||||||
# to a file/line that's not in the PR diff, drop just those findings and
|
# to a file/line that's not in the PR diff, drop just those findings and
|
||||||
|
|
@ -550,20 +465,15 @@ async def _publish_review_async(
|
||||||
)
|
)
|
||||||
if dropped_ids and valid_with_payload:
|
if dropped_ids and valid_with_payload:
|
||||||
retry_inline = [p for _, p in valid_with_payload]
|
retry_inline = [p for _, p in valid_with_payload]
|
||||||
retry_body = _decorate_review_body(
|
retry_body = render_review_body(
|
||||||
render_review_body(
|
pr_number=pr_number,
|
||||||
pr_number=pr_number,
|
surfaced_count=len(retry_inline),
|
||||||
surfaced_count=len(retry_inline),
|
trace_url=review_trace_url,
|
||||||
trace_url=review_trace_url,
|
ui_url=review_ui_url,
|
||||||
ui_url=review_ui_url,
|
additional_findings_count=additional_findings_count,
|
||||||
additional_findings_count=additional_findings_count,
|
|
||||||
),
|
|
||||||
verdict=verdict,
|
|
||||||
verdict_requester=verdict_requester,
|
|
||||||
verdict_ignored_reason=verdict_ignored_reason,
|
|
||||||
unsolicited_approve=unsolicited_approve,
|
|
||||||
)
|
)
|
||||||
retry_response = await post_pull_request_review(
|
retry_response, event, verdict_ignored_reason, findings = await _post_review_guarded(
|
||||||
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
|
|
@ -571,10 +481,14 @@ async def _publish_review_async(
|
||||||
body=retry_body,
|
body=retry_body,
|
||||||
inline_comments=retry_inline,
|
inline_comments=retry_inline,
|
||||||
token=token,
|
token=token,
|
||||||
event=event,
|
verdict=verdict,
|
||||||
|
verdict_authorization=verdict_authorization,
|
||||||
|
verdict_requester=verdict_requester,
|
||||||
|
reviewed_head_sha=reviewed_head_sha,
|
||||||
)
|
)
|
||||||
if isinstance(retry_response, dict) and "_error" not in retry_response:
|
if isinstance(retry_response, dict) and "_error" not in retry_response:
|
||||||
review_response = retry_response
|
review_response = retry_response
|
||||||
|
review_body = retry_body
|
||||||
inline_comments = retry_inline
|
inline_comments = retry_inline
|
||||||
eligible_with_payload = valid_with_payload
|
eligible_with_payload = valid_with_payload
|
||||||
unresolvable_findings = dropped_ids
|
unresolvable_findings = dropped_ids
|
||||||
|
|
@ -598,20 +512,20 @@ async def _publish_review_async(
|
||||||
# diff. GitHub accepts a bodied review with zero inline comments, so
|
# diff. GitHub accepts a bodied review with zero inline comments, so
|
||||||
# post the authorized verdict rather than dropping it — the verdict
|
# post the authorized verdict rather than dropping it — the verdict
|
||||||
# must land even when the findings can't be anchored.
|
# must land even when the findings can't be anchored.
|
||||||
verdict_only_body = _decorate_review_body(
|
verdict_only_body = render_review_body(
|
||||||
render_review_body(
|
pr_number=pr_number,
|
||||||
pr_number=pr_number,
|
surfaced_count=0,
|
||||||
surfaced_count=0,
|
trace_url=review_trace_url,
|
||||||
trace_url=review_trace_url,
|
ui_url=review_ui_url,
|
||||||
ui_url=review_ui_url,
|
additional_findings_count=additional_findings_count,
|
||||||
additional_findings_count=additional_findings_count,
|
|
||||||
),
|
|
||||||
verdict=verdict,
|
|
||||||
verdict_requester=verdict_requester,
|
|
||||||
verdict_ignored_reason=verdict_ignored_reason,
|
|
||||||
unsolicited_approve=unsolicited_approve,
|
|
||||||
)
|
)
|
||||||
verdict_only_response = await post_pull_request_review(
|
(
|
||||||
|
verdict_only_response,
|
||||||
|
event,
|
||||||
|
verdict_ignored_reason,
|
||||||
|
findings,
|
||||||
|
) = await _post_review_guarded(
|
||||||
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
|
|
@ -619,10 +533,14 @@ async def _publish_review_async(
|
||||||
body=verdict_only_body,
|
body=verdict_only_body,
|
||||||
inline_comments=[],
|
inline_comments=[],
|
||||||
token=token,
|
token=token,
|
||||||
event=event,
|
verdict=verdict,
|
||||||
|
verdict_authorization=verdict_authorization,
|
||||||
|
verdict_requester=verdict_requester,
|
||||||
|
reviewed_head_sha=reviewed_head_sha,
|
||||||
)
|
)
|
||||||
if isinstance(verdict_only_response, dict) and "_error" not in verdict_only_response:
|
if isinstance(verdict_only_response, dict) and "_error" not in verdict_only_response:
|
||||||
review_response = verdict_only_response
|
review_response = verdict_only_response
|
||||||
|
review_body = verdict_only_body
|
||||||
inline_comments = []
|
inline_comments = []
|
||||||
eligible_with_payload = []
|
eligible_with_payload = []
|
||||||
unresolvable_findings = dropped_ids
|
unresolvable_findings = dropped_ids
|
||||||
|
|
@ -665,19 +583,34 @@ async def _publish_review_async(
|
||||||
}
|
}
|
||||||
review_id = review_response.get("id") if isinstance(review_response, dict) else None
|
review_id = review_response.get("id") if isinstance(review_response, dict) else None
|
||||||
|
|
||||||
# Trust GitHub's recorded review state, not the event we asked for: GitHub
|
# Trust GitHub's recorded review state, not the event we asked for.
|
||||||
# can accept the POST (returning an id) yet land the review as COMMENTED
|
|
||||||
# (e.g. a same-identity re-approval). Only claim a verdict when the returned
|
|
||||||
# state actually matches. When the response omits state, fall back to the
|
|
||||||
# requested event so a valid submission isn't under-reported.
|
|
||||||
returned_state = review_response.get("state") if isinstance(review_response, dict) else None
|
returned_state = review_response.get("state") if isinstance(review_response, dict) else None
|
||||||
|
recorded_state = returned_state.upper() if isinstance(returned_state, str) else ""
|
||||||
expected_state = _EVENT_TO_STATE.get(event)
|
expected_state = _EVENT_TO_STATE.get(event)
|
||||||
if expected_state is None:
|
verdict_submitted = bool(
|
||||||
verdict_submitted = False
|
verdict_attempted
|
||||||
elif isinstance(returned_state, str) and returned_state:
|
and expected_state
|
||||||
verdict_submitted = returned_state.upper() == expected_state and review_id is not None
|
and recorded_state == expected_state
|
||||||
else:
|
and review_id is not None
|
||||||
verdict_submitted = review_id is not None
|
)
|
||||||
|
if verdict_attempted and not verdict_submitted and verdict_ignored_reason is None:
|
||||||
|
verdict_ignored_reason = "github_state_mismatch"
|
||||||
|
if verdict_attempted and isinstance(review_id, int) and recorded_state:
|
||||||
|
await update_pull_request_review_body(
|
||||||
|
owner=owner,
|
||||||
|
repo=repo,
|
||||||
|
pr_number=pr_number,
|
||||||
|
review_id=review_id,
|
||||||
|
body=_decorate_recorded_review_body(
|
||||||
|
review_body,
|
||||||
|
recorded_state=recorded_state,
|
||||||
|
verdict=verdict,
|
||||||
|
verdict_requester=verdict_requester,
|
||||||
|
verdict_submitted=verdict_submitted,
|
||||||
|
verdict_ignored_reason=verdict_ignored_reason,
|
||||||
|
),
|
||||||
|
token=token,
|
||||||
|
)
|
||||||
await _reconcile_last_verdict(
|
await _reconcile_last_verdict(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -776,6 +709,8 @@ async def _publish_review_async(
|
||||||
"hidden_count": max(len(open_unpublished) - len(inline_comments), 0),
|
"hidden_count": max(len(open_unpublished) - len(inline_comments), 0),
|
||||||
"resolved_thread_count": resolved_thread_count,
|
"resolved_thread_count": resolved_thread_count,
|
||||||
}
|
}
|
||||||
|
if recorded_state:
|
||||||
|
result["review_state"] = recorded_state
|
||||||
if verdict_submitted:
|
if verdict_submitted:
|
||||||
result["verdict_submitted"] = True
|
result["verdict_submitted"] = True
|
||||||
result["verdict_event"] = event
|
result["verdict_event"] = event
|
||||||
|
|
@ -792,6 +727,110 @@ async def _publish_review_async(
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
|
||||||
|
def _finding_blocks_verdict(finding: Finding) -> bool:
|
||||||
|
status = finding.get("status", "open")
|
||||||
|
if status in {"open", "needs_reassessment"}:
|
||||||
|
return True
|
||||||
|
interactions = finding.get("interactions")
|
||||||
|
if not isinstance(interactions, list) or not interactions:
|
||||||
|
return False
|
||||||
|
latest = interactions[-1]
|
||||||
|
return isinstance(latest, dict) and latest.get("needs_reassessment") is True
|
||||||
|
|
||||||
|
|
||||||
|
async def _post_review_guarded(
|
||||||
|
*,
|
||||||
|
thread_id: str,
|
||||||
|
owner: str,
|
||||||
|
repo: str,
|
||||||
|
pr_number: int,
|
||||||
|
head_sha: str,
|
||||||
|
body: str,
|
||||||
|
inline_comments: list[dict[str, Any]],
|
||||||
|
token: str,
|
||||||
|
verdict: str | None,
|
||||||
|
verdict_authorization: str,
|
||||||
|
verdict_requester: str,
|
||||||
|
reviewed_head_sha: str,
|
||||||
|
) -> tuple[dict[str, Any] | None, str, str | None, list[Finding]]:
|
||||||
|
if verdict is None:
|
||||||
|
response = await post_pull_request_review(
|
||||||
|
owner=owner,
|
||||||
|
repo=repo,
|
||||||
|
pr_number=pr_number,
|
||||||
|
head_sha=head_sha,
|
||||||
|
body=body,
|
||||||
|
inline_comments=inline_comments,
|
||||||
|
token=token,
|
||||||
|
event="COMMENT",
|
||||||
|
)
|
||||||
|
return response, "COMMENT", None, await list_findings_async(thread_id)
|
||||||
|
|
||||||
|
async with _finding_mutation_lock(thread_id):
|
||||||
|
metadata = await _get_thread_metadata_strict(thread_id)
|
||||||
|
findings = _coerce_findings_list(metadata.get("findings"))
|
||||||
|
submitted_verdict = verdict
|
||||||
|
ignored_reason: str | None = None
|
||||||
|
post_head_sha = head_sha
|
||||||
|
|
||||||
|
if verdict_authorization == "none":
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "verdict_not_requested"
|
||||||
|
elif verdict_authorization == "consistent":
|
||||||
|
open_count = sum(1 for finding in findings if _finding_blocks_verdict(finding))
|
||||||
|
if verdict == "approve" and open_count:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "approve_with_open_findings"
|
||||||
|
elif verdict == "request_changes" and not open_count:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "request_changes_without_open_findings"
|
||||||
|
|
||||||
|
if submitted_verdict is not None:
|
||||||
|
pr_author = _pr_author_from_thread(metadata)
|
||||||
|
bot_logins = {login.casefold() for login in INTERNAL_BOT_LOGINS}
|
||||||
|
if not pr_author:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "author_unknown"
|
||||||
|
elif pr_author.casefold() in bot_logins:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "self_review"
|
||||||
|
|
||||||
|
if submitted_verdict is not None and head_sha != reviewed_head_sha:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "head_moved"
|
||||||
|
|
||||||
|
if submitted_verdict is not None:
|
||||||
|
live_head_sha = await fetch_pull_request_head_sha(
|
||||||
|
owner=owner,
|
||||||
|
repo=repo,
|
||||||
|
pr_number=pr_number,
|
||||||
|
token=token,
|
||||||
|
)
|
||||||
|
if live_head_sha is not None and live_head_sha != reviewed_head_sha:
|
||||||
|
submitted_verdict = None
|
||||||
|
ignored_reason = "head_moved"
|
||||||
|
post_head_sha = live_head_sha
|
||||||
|
|
||||||
|
event = _VERDICT_EVENTS.get(submitted_verdict or "", "COMMENT")
|
||||||
|
decorated_body = _decorate_review_body(
|
||||||
|
body,
|
||||||
|
verdict=submitted_verdict,
|
||||||
|
verdict_requester=verdict_requester,
|
||||||
|
verdict_ignored_reason=ignored_reason,
|
||||||
|
)
|
||||||
|
response = await post_pull_request_review(
|
||||||
|
owner=owner,
|
||||||
|
repo=repo,
|
||||||
|
pr_number=pr_number,
|
||||||
|
head_sha=post_head_sha,
|
||||||
|
body=decorated_body,
|
||||||
|
inline_comments=inline_comments,
|
||||||
|
token=token,
|
||||||
|
event=event,
|
||||||
|
)
|
||||||
|
return response, event, ignored_reason, findings
|
||||||
|
|
||||||
|
|
||||||
async def _open_swe_already_reviewed(
|
async def _open_swe_already_reviewed(
|
||||||
*,
|
*,
|
||||||
thread_id: str,
|
thread_id: str,
|
||||||
|
|
@ -839,23 +878,39 @@ def _decorate_review_body(
|
||||||
verdict: str | None,
|
verdict: str | None,
|
||||||
verdict_requester: str,
|
verdict_requester: str,
|
||||||
verdict_ignored_reason: str | None,
|
verdict_ignored_reason: str | None,
|
||||||
unsolicited_approve: bool = False,
|
|
||||||
) -> 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:
|
||||||
if unsolicited_approve:
|
|
||||||
return f"{body}\n\nVerdict (`approve`) submitted — the review found no open issues."
|
|
||||||
requester = f"@{verdict_requester}" if verdict_requester else "the requester"
|
requester = f"@{verdict_requester}" if verdict_requester else "the requester"
|
||||||
return f"{body}\n\nVerdict (`{verdict}`) submitted at the request of {requester}."
|
return f"{body}\n\nVerdict (`{verdict}`) requested by {requester}."
|
||||||
if verdict_ignored_reason == "self_review":
|
if verdict_ignored_reason:
|
||||||
return (
|
reason = verdict_ignored_reason.replace("_", " ")
|
||||||
f"{body}\n\n> Note: a review verdict was requested, but Open SWE does not "
|
return f"{body}\n\n> Verdict withheld ({reason}). Published as a comment review."
|
||||||
"approve or request changes on its own pull requests. Published as a "
|
|
||||||
"comment review instead."
|
|
||||||
)
|
|
||||||
return body
|
return body
|
||||||
|
|
||||||
|
|
||||||
|
def _decorate_recorded_review_body(
|
||||||
|
body: str,
|
||||||
|
*,
|
||||||
|
recorded_state: str,
|
||||||
|
verdict: str | None,
|
||||||
|
verdict_requester: 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"
|
||||||
|
return (
|
||||||
|
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
||||||
|
f"Verdict (`{verdict}`) recorded for {requester}."
|
||||||
|
)
|
||||||
|
reason = (verdict_ignored_reason or "github_state_mismatch").replace("_", " ")
|
||||||
|
return (
|
||||||
|
f"{body}\n\nReview outcome: **{recorded_state}**. "
|
||||||
|
f"The requested verdict was withheld ({reason})."
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
async def _reconcile_last_verdict(
|
async def _reconcile_last_verdict(
|
||||||
*,
|
*,
|
||||||
thread_id: str,
|
thread_id: str,
|
||||||
|
|
|
||||||
|
|
@ -2431,14 +2431,12 @@ async def test_publish_review_drops_verdict_when_not_requested() -> None:
|
||||||
):
|
):
|
||||||
result = await publish_review(verdict="request_changes")
|
result = await publish_review(verdict="request_changes")
|
||||||
|
|
||||||
assert publish_async.call_args.kwargs["verdict"] is None
|
assert publish_async.call_args.kwargs["verdict"] == "request_changes"
|
||||||
assert result["verdict_ignored"] is True
|
assert publish_async.call_args.kwargs["verdict_authorization"] == "none"
|
||||||
assert result["verdict_ignored_reason"] == "verdict_not_requested"
|
assert result["success"] is True
|
||||||
assert result["verdict_submitted"] is False
|
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_review_forwards_unsolicited_approve() -> None:
|
async def test_publish_review_forwards_consistency_authorized_approve() -> None:
|
||||||
"""A dispatch-authorized approve reaches the clean-review gate."""
|
|
||||||
from agent.tools.publish_review import publish_review
|
from agent.tools.publish_review import publish_review
|
||||||
|
|
||||||
publish_async = AsyncMock(
|
publish_async = AsyncMock(
|
||||||
|
|
@ -2464,12 +2462,12 @@ async def test_publish_review_forwards_unsolicited_approve() -> None:
|
||||||
result = await publish_review(verdict="approve")
|
result = await publish_review(verdict="approve")
|
||||||
|
|
||||||
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
||||||
assert publish_async.call_args.kwargs["unsolicited_approve"] is True
|
assert publish_async.call_args.kwargs["verdict_authorization"] == "consistent"
|
||||||
assert result["verdict_submitted"] is True
|
assert result["verdict_submitted"] is True
|
||||||
assert "verdict_ignored" not in result
|
assert "verdict_ignored" not in result
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_review_drops_unauthorized_unsolicited_approve() -> None:
|
async def test_publish_review_marks_unauthorized_approve_for_downgrade() -> None:
|
||||||
from agent.tools.publish_review import publish_review
|
from agent.tools.publish_review import publish_review
|
||||||
|
|
||||||
publish_async = AsyncMock(return_value={"success": True, "review_id": 9})
|
publish_async = AsyncMock(return_value={"success": True, "review_id": 9})
|
||||||
|
|
@ -2491,11 +2489,9 @@ async def test_publish_review_drops_unauthorized_unsolicited_approve() -> None:
|
||||||
):
|
):
|
||||||
result = await publish_review(verdict="approve")
|
result = await publish_review(verdict="approve")
|
||||||
|
|
||||||
assert publish_async.call_args.kwargs["verdict"] is None
|
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
||||||
assert publish_async.call_args.kwargs["unsolicited_approve"] is False
|
assert publish_async.call_args.kwargs["verdict_authorization"] == "none"
|
||||||
assert result["verdict_ignored"] is True
|
assert result["success"] is True
|
||||||
assert result["verdict_ignored_reason"] == "verdict_not_requested"
|
|
||||||
assert result["verdict_submitted"] is False
|
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_review_forwards_authorized_verdict() -> None:
|
async def test_publish_review_forwards_authorized_verdict() -> None:
|
||||||
|
|
@ -2526,6 +2522,7 @@ async def test_publish_review_forwards_authorized_verdict() -> None:
|
||||||
|
|
||||||
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
assert publish_async.call_args.kwargs["verdict"] == "approve"
|
||||||
assert publish_async.call_args.kwargs["verdict_requester"] == "amoussa1229"
|
assert publish_async.call_args.kwargs["verdict_requester"] == "amoussa1229"
|
||||||
|
assert publish_async.call_args.kwargs["verdict_authorization"] == "requested"
|
||||||
assert result["verdict_submitted"] is True
|
assert result["verdict_submitted"] is True
|
||||||
assert "verdict_ignored" not in result
|
assert "verdict_ignored" not in result
|
||||||
|
|
||||||
|
|
@ -2545,6 +2542,8 @@ def _verdict_publish_patches(
|
||||||
) -> list[Any]:
|
) -> list[Any]:
|
||||||
if thread_metadata is _UNSET_METADATA:
|
if thread_metadata is _UNSET_METADATA:
|
||||||
thread_metadata = {"pr": {"author": "external-contributor"}}
|
thread_metadata = {"pr": {"author": "external-contributor"}}
|
||||||
|
strict_metadata = dict(thread_metadata or {})
|
||||||
|
strict_metadata["findings"] = findings
|
||||||
return [
|
return [
|
||||||
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
|
||||||
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
|
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
|
||||||
|
|
@ -2562,6 +2561,15 @@ def _verdict_publish_patches(
|
||||||
"agent.tools.publish_review.get_thread_metadata",
|
"agent.tools.publish_review.get_thread_metadata",
|
||||||
AsyncMock(return_value=thread_metadata or {}),
|
AsyncMock(return_value=thread_metadata or {}),
|
||||||
),
|
),
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review._get_thread_metadata_strict",
|
||||||
|
AsyncMock(return_value=strict_metadata),
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"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(
|
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),
|
||||||
|
|
@ -2576,7 +2584,7 @@ async def test_publish_async_approve_with_zero_findings_bypasses_empty_skip() ->
|
||||||
|
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
post_review = AsyncMock(return_value={"id": 999})
|
post_review = AsyncMock(return_value={"id": 999, "state": "APPROVED"})
|
||||||
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)
|
||||||
|
|
@ -2600,17 +2608,15 @@ 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`) submitted at the request of @amoussa1229." in body
|
assert "Verdict (`approve`) requested by @amoussa1229." in body
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_unsolicited_approve_clean_posts_approve() -> None:
|
async def test_publish_async_consistent_approve_clean_posts_approve() -> None:
|
||||||
"""A clean review (zero open findings) lands an unsolicited approve as a
|
|
||||||
real APPROVE, with automatic (not requester) attribution."""
|
|
||||||
from contextlib import ExitStack
|
from contextlib import ExitStack
|
||||||
|
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
post_review = AsyncMock(return_value={"id": 2001})
|
post_review = AsyncMock(return_value={"id": 2001, "state": "APPROVED"})
|
||||||
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)
|
||||||
|
|
@ -2625,7 +2631,7 @@ async def test_publish_async_unsolicited_approve_clean_posts_approve() -> None:
|
||||||
is_re_review=False,
|
is_re_review=False,
|
||||||
verdict="approve",
|
verdict="approve",
|
||||||
verdict_requester="",
|
verdict_requester="",
|
||||||
unsolicited_approve=True,
|
verdict_authorization="consistent",
|
||||||
)
|
)
|
||||||
|
|
||||||
assert result["success"] is True
|
assert result["success"] is True
|
||||||
|
|
@ -2633,19 +2639,16 @@ async def test_publish_async_unsolicited_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`) submitted — the review found no open issues." in body
|
assert "Verdict (`approve`) requested by the requester." in body
|
||||||
assert "at the request of" not in body
|
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_unsolicited_approve_with_open_findings_downgrades() -> None:
|
async def test_publish_async_consistent_approve_with_open_findings_downgrades() -> None:
|
||||||
"""An unsolicited approve alongside open findings is downgraded to a
|
|
||||||
comment review — approving while requesting changes is contradictory."""
|
|
||||||
from contextlib import ExitStack
|
from contextlib import ExitStack
|
||||||
|
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
||||||
post_review = AsyncMock(return_value={"id": 2002})
|
post_review = AsyncMock(return_value={"id": 2002, "state": "COMMENTED"})
|
||||||
with ExitStack() as stack:
|
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):
|
||||||
stack.enter_context(p)
|
stack.enter_context(p)
|
||||||
|
|
@ -2660,7 +2663,7 @@ async def test_publish_async_unsolicited_approve_with_open_findings_downgrades()
|
||||||
is_re_review=False,
|
is_re_review=False,
|
||||||
verdict="approve",
|
verdict="approve",
|
||||||
verdict_requester="",
|
verdict_requester="",
|
||||||
unsolicited_approve=True,
|
verdict_authorization="consistent",
|
||||||
)
|
)
|
||||||
|
|
||||||
assert result["success"] is True
|
assert result["success"] is True
|
||||||
|
|
@ -2670,7 +2673,7 @@ async def test_publish_async_unsolicited_approve_with_open_findings_downgrades()
|
||||||
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_authorized_approve_ignores_open_findings_gate() -> None:
|
async def test_publish_async_requested_approve_ignores_open_findings_gate() -> None:
|
||||||
"""The explicit-request path is unchanged: an authorized approve is honored
|
"""The explicit-request path is unchanged: an authorized approve is honored
|
||||||
even when open findings exist (the requester asked for the verdict)."""
|
even when open findings exist (the requester asked for the verdict)."""
|
||||||
from contextlib import ExitStack
|
from contextlib import ExitStack
|
||||||
|
|
@ -2678,7 +2681,7 @@ async def test_publish_async_authorized_approve_ignores_open_findings_gate() ->
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
||||||
post_review = AsyncMock(return_value={"id": 2003})
|
post_review = AsyncMock(return_value={"id": 2003, "state": "APPROVED"})
|
||||||
with ExitStack() as stack:
|
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):
|
||||||
stack.enter_context(p)
|
stack.enter_context(p)
|
||||||
|
|
@ -2706,7 +2709,7 @@ async def test_publish_async_request_changes_maps_event() -> None:
|
||||||
from agent.tools.publish_review import _publish_review_async
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
|
||||||
post_review = AsyncMock(return_value={"id": 1000})
|
post_review = AsyncMock(return_value={"id": 1000, "state": "CHANGES_REQUESTED"})
|
||||||
with ExitStack() as stack:
|
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):
|
||||||
stack.enter_context(p)
|
stack.enter_context(p)
|
||||||
|
|
@ -2733,7 +2736,7 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> 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": 1001})
|
post_review = AsyncMock(return_value={"id": 1001, "state": "COMMENTED"})
|
||||||
with ExitStack() as stack:
|
with ExitStack() as stack:
|
||||||
for p in _verdict_publish_patches(
|
for p in _verdict_publish_patches(
|
||||||
findings=[],
|
findings=[],
|
||||||
|
|
@ -2760,7 +2763,7 @@ async def test_publish_async_self_review_downgrades_verdict_to_comment() -> None
|
||||||
assert result["verdict_ignored_reason"] == "self_review"
|
assert result["verdict_ignored_reason"] == "self_review"
|
||||||
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
assert post_review.await_args.kwargs["event"] == "COMMENT"
|
||||||
body = post_review.await_args.kwargs["body"]
|
body = post_review.await_args.kwargs["body"]
|
||||||
assert "does not approve or request changes on its own pull requests" in body
|
assert "Verdict withheld (self review)" in body
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_dismisses_stale_approval_on_new_findings() -> None:
|
async def test_publish_async_dismisses_stale_approval_on_new_findings() -> None:
|
||||||
|
|
@ -2964,14 +2967,17 @@ async def test_publish_async_verdict_lands_when_all_findings_out_of_diff() -> No
|
||||||
assert post_review.await_args.kwargs["inline_comments"] == []
|
assert post_review.await_args.kwargs["inline_comments"] == []
|
||||||
|
|
||||||
|
|
||||||
async def test_publish_async_verdict_not_submitted_when_github_coerces_state() -> None:
|
@pytest.mark.parametrize("recorded_state", ["COMMENTED", "CHANGES_REQUESTED", None])
|
||||||
|
async def test_publish_async_verdict_not_submitted_when_github_state_mismatches(
|
||||||
|
recorded_state: str | None,
|
||||||
|
) -> None:
|
||||||
"""GitHub can accept the POST but land the review as COMMENTED; the result
|
"""GitHub can accept the POST but land the review as COMMENTED; the result
|
||||||
must not claim a verdict in that case."""
|
must not claim a verdict in that case."""
|
||||||
from contextlib import ExitStack
|
from contextlib import ExitStack
|
||||||
|
|
||||||
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": "COMMENTED"})
|
post_review = AsyncMock(return_value={"id": 2005, "state": recorded_state})
|
||||||
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)
|
||||||
|
|
@ -2990,3 +2996,145 @@ async def test_publish_async_verdict_not_submitted_when_github_coerces_state() -
|
||||||
|
|
||||||
assert result["success"] is True
|
assert result["success"] is True
|
||||||
assert result.get("verdict_submitted") is not True
|
assert result.get("verdict_submitted") is not True
|
||||||
|
assert result["verdict_ignored_reason"] == "github_state_mismatch"
|
||||||
|
if recorded_state is None:
|
||||||
|
assert "review_state" not in result
|
||||||
|
else:
|
||||||
|
assert result["review_state"] == recorded_state
|
||||||
|
assert "submitted" not in post_review.await_args.kwargs["body"].lower()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("finding_state", ["clean", "open", "needs_reassessment"])
|
||||||
|
@pytest.mark.parametrize("verdict", ["approve", "request_changes"])
|
||||||
|
@pytest.mark.parametrize("authorization", ["requested", "consistent", "none"])
|
||||||
|
async def test_publish_async_verdict_authorization_matrix(
|
||||||
|
authorization: str,
|
||||||
|
verdict: str,
|
||||||
|
finding_state: str,
|
||||||
|
) -> None:
|
||||||
|
from contextlib import ExitStack
|
||||||
|
|
||||||
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
|
findings = []
|
||||||
|
if finding_state != "clean":
|
||||||
|
finding = _f(id="f_matrix", severity="high", file="a.py", start_line=1, end_line=1)
|
||||||
|
finding["status"] = finding_state
|
||||||
|
findings = [finding]
|
||||||
|
|
||||||
|
has_open = finding_state != "clean"
|
||||||
|
should_submit = authorization == "requested" or (
|
||||||
|
authorization == "consistent"
|
||||||
|
and ((verdict == "approve" and not has_open) or (verdict == "request_changes" and has_open))
|
||||||
|
)
|
||||||
|
expected_event = (
|
||||||
|
("APPROVE" if verdict == "approve" else "REQUEST_CHANGES") if should_submit else "COMMENT"
|
||||||
|
)
|
||||||
|
recorded_state = {
|
||||||
|
"APPROVE": "APPROVED",
|
||||||
|
"REQUEST_CHANGES": "CHANGES_REQUESTED",
|
||||||
|
"COMMENT": "COMMENTED",
|
||||||
|
}[expected_event]
|
||||||
|
post_review = AsyncMock(return_value={"id": 4001, "state": recorded_state})
|
||||||
|
update_body = AsyncMock(return_value=True)
|
||||||
|
|
||||||
|
with ExitStack() as stack:
|
||||||
|
for patcher in _verdict_publish_patches(findings=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=verdict,
|
||||||
|
verdict_authorization=authorization,
|
||||||
|
)
|
||||||
|
|
||||||
|
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 not should_submit:
|
||||||
|
expected_reason = "verdict_not_requested"
|
||||||
|
if authorization == "consistent":
|
||||||
|
expected_reason = (
|
||||||
|
"approve_with_open_findings"
|
||||||
|
if verdict == "approve"
|
||||||
|
else "request_changes_without_open_findings"
|
||||||
|
)
|
||||||
|
assert result["verdict_ignored_reason"] == expected_reason
|
||||||
|
assert "Published as a comment review" in post_review.await_args.kwargs["body"]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_publish_async_rechecks_live_head_immediately_before_verdict_post() -> None:
|
||||||
|
from contextlib import ExitStack
|
||||||
|
|
||||||
|
from agent.tools.publish_review import _publish_review_async
|
||||||
|
|
||||||
|
order: list[str] = []
|
||||||
|
lock_state = {"held": False}
|
||||||
|
|
||||||
|
class TrackingLock:
|
||||||
|
async def __aenter__(self) -> None:
|
||||||
|
lock_state["held"] = True
|
||||||
|
|
||||||
|
async def __aexit__(self, *_args: Any) -> None:
|
||||||
|
lock_state["held"] = False
|
||||||
|
|
||||||
|
async def snapshot(_thread_id: str) -> dict[str, Any]:
|
||||||
|
assert lock_state["held"] is True
|
||||||
|
order.append("snapshot")
|
||||||
|
return {"pr": {"author": "external-contributor"}, "findings": []}
|
||||||
|
|
||||||
|
async def fetch_live_head(**_kwargs: Any) -> str:
|
||||||
|
assert lock_state["held"] is True
|
||||||
|
order.append("head")
|
||||||
|
return "moved"
|
||||||
|
|
||||||
|
async def post_review(**kwargs: Any) -> dict[str, Any]:
|
||||||
|
assert lock_state["held"] is True
|
||||||
|
order.append("post")
|
||||||
|
assert kwargs["event"] == "COMMENT"
|
||||||
|
return {"id": 4002, "state": "COMMENTED"}
|
||||||
|
|
||||||
|
post = AsyncMock(side_effect=post_review)
|
||||||
|
with ExitStack() as stack:
|
||||||
|
for patcher in _verdict_publish_patches(findings=[], post_review=post):
|
||||||
|
stack.enter_context(patcher)
|
||||||
|
stack.enter_context(
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review.fetch_pull_request_head_sha",
|
||||||
|
AsyncMock(side_effect=fetch_live_head),
|
||||||
|
)
|
||||||
|
)
|
||||||
|
stack.enter_context(
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review._get_thread_metadata_strict",
|
||||||
|
AsyncMock(side_effect=snapshot),
|
||||||
|
)
|
||||||
|
)
|
||||||
|
stack.enter_context(
|
||||||
|
patch("agent.tools.publish_review._finding_mutation_lock", return_value=TrackingLock())
|
||||||
|
)
|
||||||
|
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 order == ["snapshot", "head", "post"]
|
||||||
|
assert lock_state["held"] is False
|
||||||
|
assert result["verdict_ignored_reason"] == "head_moved"
|
||||||
|
|
|
||||||
|
|
@ -8,7 +8,7 @@ from agent.review.reconcile import reconcile_findings_with_review_threads
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_reconcile_marks_resolved_github_thread_resolved() -> None:
|
async def test_reconcile_marks_author_resolved_thread_needs_reassessment() -> None:
|
||||||
findings = [
|
findings = [
|
||||||
{
|
{
|
||||||
"id": "f1",
|
"id": "f1",
|
||||||
|
|
@ -35,8 +35,9 @@ async def test_reconcile_marks_resolved_github_thread_resolved() -> None:
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
|
|
||||||
assert result[0]["status"] == "resolved"
|
assert result[0]["status"] == "needs_reassessment"
|
||||||
assert result[0]["github_thread_resolved"] is True
|
assert result[0].get("github_thread_resolved") is not True
|
||||||
|
assert "reassess" in result[0]["last_reconciliation_note"]
|
||||||
replace.assert_awaited_once()
|
replace.assert_awaited_once()
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -212,13 +213,60 @@ async def test_reconcile_duplicate_markers_stay_open_when_some_threads_only_outd
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
|
|
||||||
assert result[0]["status"] == "open"
|
assert result[0]["status"] == "needs_reassessment"
|
||||||
assert "last_reconciliation_note" not in result[0]
|
assert "reassess" in result[0]["last_reconciliation_note"]
|
||||||
assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"]
|
assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"]
|
||||||
assert result[0].get("github_thread_resolved") is not True
|
assert result[0].get("github_thread_resolved") is not True
|
||||||
replace.assert_awaited_once()
|
replace.assert_awaited_once()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_reconcile_marks_author_outdated_thread_needs_reassessment() -> None:
|
||||||
|
findings = [{"id": "f1", "status": "open", "github_review_comment_id": 11}]
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("agent.review.reconcile.list_findings", AsyncMock(return_value=findings)),
|
||||||
|
patch("agent.review.reconcile.replace_findings", AsyncMock()),
|
||||||
|
):
|
||||||
|
result = await reconcile_findings_with_review_threads(
|
||||||
|
"tid",
|
||||||
|
[
|
||||||
|
{
|
||||||
|
"id": "THREAD_1",
|
||||||
|
"is_resolved": False,
|
||||||
|
"is_outdated": True,
|
||||||
|
"comments": [{"id": 11, "author": "open-swe[bot]", "body": "bug"}],
|
||||||
|
}
|
||||||
|
],
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result[0]["status"] == "needs_reassessment"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_reconcile_preserves_reviewer_resolved_status() -> None:
|
||||||
|
findings = [{"id": "f1", "status": "resolved", "github_review_comment_id": 11}]
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("agent.review.reconcile.list_findings", AsyncMock(return_value=findings)),
|
||||||
|
patch("agent.review.reconcile.replace_findings", AsyncMock()),
|
||||||
|
):
|
||||||
|
result = await reconcile_findings_with_review_threads(
|
||||||
|
"tid",
|
||||||
|
[
|
||||||
|
{
|
||||||
|
"id": "THREAD_1",
|
||||||
|
"is_resolved": True,
|
||||||
|
"is_outdated": False,
|
||||||
|
"comments": [{"id": 11, "author": "open-swe[bot]", "body": "bug"}],
|
||||||
|
}
|
||||||
|
],
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result[0]["status"] == "resolved"
|
||||||
|
assert result[0]["github_thread_resolved"] is True
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_reconcile_ignores_spoofed_non_bot_marker() -> None:
|
async def test_reconcile_ignores_spoofed_non_bot_marker() -> None:
|
||||||
findings = [
|
findings = [
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue