diff --git a/agent/review/findings.py b/agent/review/findings.py index 0e26985c..05a83d0e 100644 --- a/agent/review/findings.py +++ b/agent/review/findings.py @@ -82,7 +82,7 @@ def normalize_finding_title(title: str | None, description: str = "") -> str: Severity = Literal["low", "medium", "high", "critical"] Confidence = Literal["low", "medium", "high"] -FindingStatus = Literal["open", "resolved", "dismissed"] +FindingStatus = Literal["open", "needs_reassessment", "resolved", "dismissed"] DiffSide = Literal["LEFT", "RIGHT"] SurfaceState = Literal["not_surfaced", "surfaced", "resolve_pending", "resolved", "error"] InteractionKind = Literal["human_reply", "bot_reply"] diff --git a/agent/review/publish.py b/agent/review/publish.py index 4e34efeb..fe483527 100644 --- a/agent/review/publish.py +++ b/agent/review/publish.py @@ -540,6 +540,28 @@ async def open_swe_review_exists( 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"} @@ -636,6 +658,33 @@ async def post_pull_request_review( 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( *, owner: str, diff --git a/agent/review/reconcile.py b/agent/review/reconcile.py index a4cabf26..3d3648c6 100644 --- a/agent/review/reconcile.py +++ b/agent/review/reconcile.py @@ -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")): 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 - if finding.get("status") == "open": - finding["status"] = "resolved" - updated = True + if not all_resolved: + return updated if not finding.get("github_thread_resolved"): finding["github_thread_resolved"] = True updated = True diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 2f90256a..f1254873 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -16,7 +16,10 @@ from ..review.findings import ( Finding, ReviewerThreadMissingError, Severity, + _coerce_findings_list, _coerce_surface, + _finding_mutation_lock, + _get_thread_metadata_strict, filter_findings_for_publish, get_thread_id_from_runtime, get_thread_last_reviewed_sha, @@ -35,6 +38,7 @@ from ..review.publish import ( clear_review_started_comment, dismiss_pull_request_review, fetch_pr_review_threads, + fetch_pull_request_head_sha, fetch_review_comments, fetch_review_thread_id_for_comment, open_swe_review_exists, @@ -46,6 +50,7 @@ from ..review.publish import ( reply_to_review_comment, resolve_review_thread, settle_review_check_run, + update_pull_request_review_body, ) from ..review.reconcile import reconcile_findings_with_review_threads 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 not posted as inline PR comments. verdict: Optional review verdict — ``"approve"`` or - ``"request_changes"``. ``"request_changes"`` is honored ONLY when - this run was explicitly authorized to submit a verdict (the - triggering user asked for one). ``"approve"`` is also honored on a - run without that authorization when the review is clean — zero - open findings — so a clean review lands as a real APPROVE; with - open findings an unsolicited approve is downgraded to a plain - 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). + ``"request_changes"``. Explicitly requested verdicts are honored + subject to the safety checks below. Automatically authorized + verdicts must agree with authoritative finding state: approve + requires no open findings and request_changes requires at least + one. A downgraded verdict is posted as a comment review and carries + ``verdict_ignored: true``. Returns: Dictionary with ``success``, ``review_id``, ``surfaced_count``, ``hidden_count``, ``resolved_thread_count``, and sometimes @@ -123,9 +124,10 @@ async def publish_review( ``verdict_submitted`` (GitHub confirmed the requested APPROVE/ REQUEST_CHANGES state) or ``verdict_ignored`` + ``verdict_ignored_reason`` (``"verdict_not_requested"``, - ``"approve_with_open_findings"`` — an unsolicited approve on a run - with open findings — ``"self_review"``, ``"head_moved"`` — the - reviewed commit is no longer the PR head — or ``"author_unknown"``). + ``"approve_with_open_findings"``, + ``"request_changes_without_open_findings"``, ``"self_review"``, + ``"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"}: return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"} @@ -182,24 +184,12 @@ async def publish_review( if not token: return {"success": False, "error": "No GitHub token available"} - # Verdict authorization is enforced here in code, not in the prompt. - # Explicit requests may submit either verdict. Automatic approvals require - # the dispatch-set verdict_authorized flag and still pass the clean-review - # gate in _publish_review_async. - verdict_not_requested = False - unsolicited_approve = False - 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 + if configurable.get("verdict_requested") is True: + verdict_authorization = "requested" + elif configurable.get("verdict_authorized") is True: + verdict_authorization = "consistent" + else: + verdict_authorization = "none" try: result = await _publish_review_async( @@ -215,12 +205,8 @@ async def publish_review( trace_link_config_override=configurable.get("review_trace_link_enabled"), verdict=verdict, 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 except ReviewerThreadMissingError as exc: return thread_missing_tool_result(exc) @@ -320,10 +306,11 @@ async def _publish_review_async( trace_link_config_override: object = None, verdict: str | None = None, verdict_requester: str = "", - unsolicited_approve: bool = False, + verdict_authorization: str = "requested", ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() verdict_ignored_reason: str | None = None + verdict_attempted = verdict is not None reviewed_head_sha = head_sha # 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 @@ -331,56 +318,6 @@ async def _publish_review_async( # reviewed, not the stale one this run was created for. 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( thread_id=thread_id, owner=owner, @@ -389,25 +326,6 @@ async def _publish_review_async( 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_ui_url = dashboard_review_url(owner, repo, pr_number) @@ -465,7 +383,7 @@ async def _publish_review_async( # plain comment publishes. if ( not inline_comments - and verdict is None + and not verdict_attempted and await _open_swe_already_reviewed( thread_id=thread_id, owner=owner, @@ -508,21 +426,15 @@ async def _publish_review_async( skip_result["verdict_ignored_reason"] = verdict_ignored_reason return skip_result - review_body = _decorate_review_body( - render_review_body( - pr_number=pr_number, - surfaced_count=len(inline_comments), - trace_url=review_trace_url, - ui_url=review_ui_url, - additional_findings_count=additional_findings_count, - ), - verdict=verdict, - verdict_requester=verdict_requester, - verdict_ignored_reason=verdict_ignored_reason, - unsolicited_approve=unsolicited_approve, + review_body = render_review_body( + pr_number=pr_number, + surfaced_count=len(inline_comments), + trace_url=review_trace_url, + ui_url=review_ui_url, + additional_findings_count=additional_findings_count, ) - - review_response = await post_pull_request_review( + review_response, event, verdict_ignored_reason, findings = await _post_review_guarded( + thread_id=thread_id, owner=owner, repo=repo, pr_number=pr_number, @@ -530,7 +442,10 @@ async def _publish_review_async( body=review_body, inline_comments=inline_comments, 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 # 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: retry_inline = [p for _, p in valid_with_payload] - retry_body = _decorate_review_body( - render_review_body( - pr_number=pr_number, - surfaced_count=len(retry_inline), - trace_url=review_trace_url, - ui_url=review_ui_url, - additional_findings_count=additional_findings_count, - ), - verdict=verdict, - verdict_requester=verdict_requester, - verdict_ignored_reason=verdict_ignored_reason, - unsolicited_approve=unsolicited_approve, + retry_body = render_review_body( + pr_number=pr_number, + surfaced_count=len(retry_inline), + trace_url=review_trace_url, + ui_url=review_ui_url, + additional_findings_count=additional_findings_count, ) - retry_response = await post_pull_request_review( + retry_response, event, verdict_ignored_reason, findings = await _post_review_guarded( + thread_id=thread_id, owner=owner, repo=repo, pr_number=pr_number, @@ -571,10 +481,14 @@ async def _publish_review_async( body=retry_body, inline_comments=retry_inline, 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: review_response = retry_response + review_body = retry_body inline_comments = retry_inline eligible_with_payload = valid_with_payload unresolvable_findings = dropped_ids @@ -598,20 +512,20 @@ async def _publish_review_async( # diff. GitHub accepts a bodied review with zero inline comments, so # post the authorized verdict rather than dropping it — the verdict # must land even when the findings can't be anchored. - verdict_only_body = _decorate_review_body( - render_review_body( - pr_number=pr_number, - surfaced_count=0, - trace_url=review_trace_url, - ui_url=review_ui_url, - additional_findings_count=additional_findings_count, - ), - verdict=verdict, - verdict_requester=verdict_requester, - verdict_ignored_reason=verdict_ignored_reason, - unsolicited_approve=unsolicited_approve, + verdict_only_body = render_review_body( + pr_number=pr_number, + surfaced_count=0, + trace_url=review_trace_url, + ui_url=review_ui_url, + additional_findings_count=additional_findings_count, ) - 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, repo=repo, pr_number=pr_number, @@ -619,10 +533,14 @@ async def _publish_review_async( body=verdict_only_body, inline_comments=[], 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: review_response = verdict_only_response + review_body = verdict_only_body inline_comments = [] eligible_with_payload = [] 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 - # Trust GitHub's recorded review state, not the event we asked for: GitHub - # 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. + # Trust GitHub's recorded review state, not the event we asked for. 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) - if expected_state is None: - verdict_submitted = False - elif isinstance(returned_state, str) and returned_state: - verdict_submitted = returned_state.upper() == expected_state and review_id is not None - else: - verdict_submitted = review_id is not None + verdict_submitted = bool( + verdict_attempted + and expected_state + and recorded_state == expected_state + and 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( thread_id=thread_id, owner=owner, @@ -776,6 +709,8 @@ async def _publish_review_async( "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), "resolved_thread_count": resolved_thread_count, } + if recorded_state: + result["review_state"] = recorded_state if verdict_submitted: result["verdict_submitted"] = True result["verdict_event"] = event @@ -792,6 +727,110 @@ async def _publish_review_async( 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( *, thread_id: str, @@ -839,23 +878,39 @@ def _decorate_review_body( verdict: str | None, verdict_requester: str, verdict_ignored_reason: str | None, - unsolicited_approve: bool = False, ) -> str: """Append verdict attribution / downgrade context to the review body.""" 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" - return f"{body}\n\nVerdict (`{verdict}`) submitted at the request of {requester}." - if verdict_ignored_reason == "self_review": - return ( - f"{body}\n\n> Note: a review verdict was requested, but Open SWE does not " - "approve or request changes on its own pull requests. Published as a " - "comment review instead." - ) + return f"{body}\n\nVerdict (`{verdict}`) requested by {requester}." + if verdict_ignored_reason: + reason = verdict_ignored_reason.replace("_", " ") + return f"{body}\n\n> Verdict withheld ({reason}). Published as a comment review." 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( *, thread_id: str, diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index 3202a307..bb308543 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -2431,14 +2431,12 @@ async def test_publish_review_drops_verdict_when_not_requested() -> None: ): result = await publish_review(verdict="request_changes") - assert publish_async.call_args.kwargs["verdict"] is None - assert result["verdict_ignored"] is True - assert result["verdict_ignored_reason"] == "verdict_not_requested" - assert result["verdict_submitted"] is False + assert publish_async.call_args.kwargs["verdict"] == "request_changes" + assert publish_async.call_args.kwargs["verdict_authorization"] == "none" + assert result["success"] is True -async def test_publish_review_forwards_unsolicited_approve() -> None: - """A dispatch-authorized approve reaches the clean-review gate.""" +async def test_publish_review_forwards_consistency_authorized_approve() -> None: from agent.tools.publish_review import publish_review publish_async = AsyncMock( @@ -2464,12 +2462,12 @@ async def test_publish_review_forwards_unsolicited_approve() -> None: result = await publish_review(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 "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 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") - assert publish_async.call_args.kwargs["verdict"] is None - assert publish_async.call_args.kwargs["unsolicited_approve"] is False - assert result["verdict_ignored"] is True - assert result["verdict_ignored_reason"] == "verdict_not_requested" - assert result["verdict_submitted"] is False + assert publish_async.call_args.kwargs["verdict"] == "approve" + assert publish_async.call_args.kwargs["verdict_authorization"] == "none" + assert result["success"] is True 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_requester"] == "amoussa1229" + assert publish_async.call_args.kwargs["verdict_authorization"] == "requested" assert result["verdict_submitted"] is True assert "verdict_ignored" not in result @@ -2545,6 +2542,8 @@ def _verdict_publish_patches( ) -> list[Any]: if thread_metadata is _UNSET_METADATA: thread_metadata = {"pr": {"author": "external-contributor"}} + strict_metadata = dict(thread_metadata or {}) + strict_metadata["findings"] = findings return [ 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)), @@ -2562,6 +2561,15 @@ def _verdict_publish_patches( "agent.tools.publish_review.get_thread_metadata", 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( "agent.tools.publish_review.dismiss_pull_request_review", 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 - post_review = AsyncMock(return_value={"id": 999}) + post_review = AsyncMock(return_value={"id": 999, "state": "APPROVED"}) with ExitStack() as stack: for p in _verdict_publish_patches(findings=[], post_review=post_review): 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 post_review.await_args.kwargs["event"] == "APPROVE" 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: - """A clean review (zero open findings) lands an unsolicited approve as a - real APPROVE, with automatic (not requester) attribution.""" +async def test_publish_async_consistent_approve_clean_posts_approve() -> None: from contextlib import ExitStack 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: for p in _verdict_publish_patches(findings=[], post_review=post_review): stack.enter_context(p) @@ -2625,7 +2631,7 @@ async def test_publish_async_unsolicited_approve_clean_posts_approve() -> None: is_re_review=False, verdict="approve", verdict_requester="", - unsolicited_approve=True, + verdict_authorization="consistent", ) 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 post_review.await_args.kwargs["event"] == "APPROVE" body = post_review.await_args.kwargs["body"] - assert "Verdict (`approve`) submitted — the review found no open issues." in body - assert "at the request of" not in body + assert "Verdict (`approve`) requested by the requester." in body -async def test_publish_async_unsolicited_approve_with_open_findings_downgrades() -> None: - """An unsolicited approve alongside open findings is downgraded to a - comment review — approving while requesting changes is contradictory.""" +async def test_publish_async_consistent_approve_with_open_findings_downgrades() -> None: from contextlib import ExitStack 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)] - post_review = AsyncMock(return_value={"id": 2002}) + post_review = AsyncMock(return_value={"id": 2002, "state": "COMMENTED"}) with ExitStack() as stack: for p in _verdict_publish_patches(findings=findings, post_review=post_review): stack.enter_context(p) @@ -2660,7 +2663,7 @@ async def test_publish_async_unsolicited_approve_with_open_findings_downgrades() is_re_review=False, verdict="approve", verdict_requester="", - unsolicited_approve=True, + verdict_authorization="consistent", ) 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" -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 even when open findings exist (the requester asked for the verdict).""" 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 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: for p in _verdict_publish_patches(findings=findings, post_review=post_review): 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 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: for p in _verdict_publish_patches(findings=findings, post_review=post_review): 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 - post_review = AsyncMock(return_value={"id": 1001}) + post_review = AsyncMock(return_value={"id": 1001, "state": "COMMENTED"}) with ExitStack() as stack: for p in _verdict_publish_patches( 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 post_review.await_args.kwargs["event"] == "COMMENT" 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: @@ -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"] == [] -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 must not claim a verdict in that case.""" from contextlib import ExitStack 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: for p in _verdict_publish_patches(findings=[], post_review=post_review): 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.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" diff --git a/tests/reviewer/test_reviewer_reconcile.py b/tests/reviewer/test_reviewer_reconcile.py index 887c92d0..9773534f 100644 --- a/tests/reviewer/test_reviewer_reconcile.py +++ b/tests/reviewer/test_reviewer_reconcile.py @@ -8,7 +8,7 @@ from agent.review.reconcile import reconcile_findings_with_review_threads @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 = [ { "id": "f1", @@ -35,8 +35,9 @@ async def test_reconcile_marks_resolved_github_thread_resolved() -> None: ], ) - assert result[0]["status"] == "resolved" - assert result[0]["github_thread_resolved"] is True + assert result[0]["status"] == "needs_reassessment" + assert result[0].get("github_thread_resolved") is not True + assert "reassess" in result[0]["last_reconciliation_note"] 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 "last_reconciliation_note" not in result[0] + assert result[0]["status"] == "needs_reassessment" + assert "reassess" in result[0]["last_reconciliation_note"] assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"] assert result[0].get("github_thread_resolved") is not True 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 async def test_reconcile_ignores_spoofed_non_bot_marker() -> None: findings = [