diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 06964c734..be87880f4 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1106,6 +1106,19 @@ def is_opencode_review(review: dict[str, Any]) -> bool: return review_author_login(review) in {"opencode-agent", "opencode-agent[bot]"} +def is_legacy_actions_opencode_review(review: dict[str, Any]) -> bool: + """Return whether a legacy Actions-authored review contains OpenCode evidence.""" + login = review_author_login(review) + return login in {"github-actions", "github-actions[bot]"} and "opencode" in ( + review.get("body") or "" + ).lower() + + +def is_automated_opencode_review(review: dict[str, Any]) -> bool: + """Return whether a review is OpenCode automation evidence, including legacy writes.""" + return is_opencode_review(review) or is_legacy_actions_opencode_review(review) + + def is_deterministic_fallback_approval(review: dict[str, Any]) -> bool: """Return whether an old fail-open approval body is not review evidence.""" if (review.get("state") or "").upper() != "APPROVED": @@ -1157,11 +1170,7 @@ def stale_opencode_change_request_ids(pr: dict[str, Any]) -> list[int]: continue if review_matches_current_head(review, pr): continue - login = review_author_login(review) - legacy_actions_review = login in {"github-actions", "github-actions[bot]"} and "opencode" in ( - review.get("body") or "" - ).lower() - if not (is_opencode_review(review) or legacy_actions_review): + if not is_automated_opencode_review(review): continue review_id = review.get("databaseId") if isinstance(review_id, int) and review_id > 0: @@ -1169,6 +1178,130 @@ def stale_opencode_change_request_ids(pr: dict[str, Any]) -> list[int]: return review_ids +def stale_opencode_approval_ids(pr: dict[str, Any]) -> list[int]: + """Return active automated approvals whose evidence is not for the live head. + + GitHub evaluates the latest review from each author. Older review objects may + remain ``APPROVED`` after a later same-author review supersedes them, and the + dismissal API treats those historical objects as no-ops. Inspect only the + latest OpenCode review per automation identity so cleanup targets effective + policy state rather than immutable review history. + """ + latest_by_author: dict[str, dict[str, Any]] = {} + for review in (pr.get("reviews") or {}).get("nodes") or []: + if not is_automated_opencode_review(review): + continue + latest_by_author[review_author_login(review)] = review + + review_ids: list[int] = [] + for review in latest_by_author.values(): + if (review.get("state") or "").upper() != "APPROVED": + continue + if review_matches_current_head(review, pr): + continue + review_id = review.get("databaseId") + if isinstance(review_id, int) and review_id > 0: + review_ids.append(review_id) + return review_ids + + +def dismiss_pull_request_review( + repo: str, + number: str, + review_id: int, + *, + message: str, +) -> bool: + """Dismiss one review and verify GitHub actually changed its state.""" + try: + run( + [ + "gh", + "api", + "-X", + "PUT", + f"repos/{repo}/pulls/{number}/reviews/{review_id}/dismissals", + "-f", + f"message={message}", + ] + ) + live_state = run_github_read( + [ + "gh", + "api", + f"repos/{repo}/pulls/{number}/reviews/{review_id}", + "--jq", + ".state", + ] + ).strip().upper() + except RuntimeError as exc: + print( + "::warning::Stale OpenCode review dismissal failed for " + f"PR #{number} review {review_id}: {scrub_sensitive_data(str(exc))}" + ) + return False + if live_state == "DISMISSED": + return True + print( + "::warning::GitHub accepted stale OpenCode review dismissal for " + f"PR #{number} review {review_id}, but the verified review state is " + f"{live_state or ''}; the review remains non-authoritative unless its explicit " + "Head SHA matches the live PR head." + ) + return False + + +def dismiss_stale_opencode_approvals( + repo: str, + pr: dict[str, Any], + *, + dry_run: bool, +) -> tuple[int, int]: + """Dismiss latest automated approvals that do not match the exact live head.""" + review_ids = stale_opencode_approval_ids(pr) + if not review_ids: + return 0, 0 + if dry_run: + return len(review_ids), 0 + + require_github_actions_mutation_actor("dismiss-stale-opencode-approval") + repo = validate_github_repository(repo) + number = str(int(pr["number"])) + expected_head = validate_git_sha(pr["headRefOid"]) + live_head = run_github_read( + ["gh", "api", f"repos/{repo}/pulls/{number}", "--jq", ".head.sha"] + ).strip() + if live_head != expected_head: + raise RuntimeError( + "PR head changed before stale approval dismissal; " + f"expected {expected_head}, observed {live_head or ''}" + ) + + dismissed = 0 + for review_id in review_ids: + message = ( + "Superseded automated OpenCode approval whose explicit review evidence does not match " + f"exact current head {expected_head}; a fresh current-head review is required." + ) + if dismiss_pull_request_review(repo, number, review_id, message=message): + dismissed += 1 + return dismissed, len(review_ids) - dismissed + + +def stale_approval_cleanup_note(dismissed: int, retained: int, *, dry_run: bool) -> str | None: + """Render exact stale-approval cleanup evidence for scheduler logs.""" + notes: list[str] = [] + if dismissed: + verb = "would dismiss" if dry_run else "dismissed" + notes.append(f"{verb} {dismissed} latest previous-head automated OpenCode approval(s)") + if retained: + notes.append( + f"GitHub retained {retained} stale automated approval(s) after dismissal attempts; " + "their head evidence remains non-authoritative" + ) + return "; ".join(notes) if notes else None + + def dismiss_stale_opencode_change_requests(repo: str, pr: dict[str, Any], *, dry_run: bool) -> int: """Dismiss previous-head automated gates only after exact-head approval.""" if not has_current_head_approval(pr): @@ -1434,7 +1567,19 @@ def post_update_branch_followup( "wait for GitHub to refresh branch-freshness and required-check evidence" ) + dismissed_approvals, retained_approvals = dismiss_stale_opencode_approvals( + repo, + updated_pr, + dry_run=dry_run, + ) + cleanup_note = stale_approval_cleanup_note( + dismissed_approvals, + retained_approvals, + dry_run=dry_run, + ) head_note = f"updated head {short_sha(updated_head)} observed after update-branch" + if cleanup_note: + head_note = f"{head_note}; {cleanup_note}" if not trigger_reviews: return f"{head_note}; review dispatch is disabled for this scheduler run" if not review_dispatch_allowed: @@ -1873,6 +2018,11 @@ def inspect_pr( outdated_cleanup_count = resolve_outdated_review_threads(pr, dry_run=dry_run) stale_review_cleanup_count = 0 + stale_approval_cleanup_count, retained_stale_approval_count = dismiss_stale_opencode_approvals( + repo, + pr, + dry_run=dry_run, + ) def finish(decision: Decision) -> Decision: """Attach obsolete review cleanup evidence to the final decision.""" @@ -1893,6 +2043,18 @@ def finish(decision: Decision) -> Decision: decision.reason, (*decision.notes, note), ) + approval_note = stale_approval_cleanup_note( + stale_approval_cleanup_count, + retained_stale_approval_count, + dry_run=dry_run, + ) + if approval_note: + decision = Decision( + decision.pr, + decision.action, + decision.reason, + (*decision.notes, approval_note), + ) return decision def decide(action: str, reason: str) -> Decision: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 6ec60ef30..674b2d9f7 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1124,6 +1124,43 @@ def test_review_state_and_failed_checks(): ) assert sched.stale_opencode_change_request_ids(stale_gate_reviews) == [101, 102] + exact_head_approval = { + **opencode_review("APPROVED", exact_head), + "databaseId": 302, + "body": f"## Gate evidence\n\n- Head SHA: `{exact_head}`", + } + stale_approval_history = make_pr( + headRefOid=exact_head, + reviews={ + "nodes": [ + { + **opencode_review("APPROVED", exact_head), + "databaseId": 300, + "body": f"## Gate evidence\n\n- Head SHA: `{stale_body_head}`", + }, + { + **opencode_review("APPROVED", exact_head), + "databaseId": 301, + "body": f"## Gate evidence\n\n- Head SHA: `{stale_body_head}`", + }, + exact_head_approval, + { + **opencode_review("APPROVED", exact_head, login="github-actions[bot]"), + "databaseId": 303, + "body": f"OpenCode gate.\n\n- Head SHA: `{stale_body_head}`", + }, + { + **opencode_review("APPROVED", exact_head, login="human"), + "databaseId": 304, + "body": f"OpenCode mentioned.\n\n- Head SHA: `{stale_body_head}`", + }, + ] + }, + ) + assert sched.stale_opencode_approval_ids(stale_approval_history) == [303] + stale_approval_history["reviews"]["nodes"].remove(exact_head_approval) + assert sched.stale_opencode_approval_ids(stale_approval_history) == [301, 303] + failed = make_pr( statusCheckRollup={ "contexts": { @@ -2074,6 +2111,103 @@ def test_dismiss_stale_opencode_change_requests_is_current_head_guarded(monkeypa assert len(calls) == 1 +def test_dismiss_stale_opencode_approvals_verifies_live_state(monkeypatch, capsys): + exact_head = "a" * 40 + stale_head = "b" * 40 + pr = make_pr( + headRefOid=exact_head, + reviews={ + "nodes": [ + { + **opencode_review("APPROVED", exact_head), + "databaseId": 301, + "body": f"## Gate evidence\n\n- Head SHA: `{stale_head}`", + } + ] + }, + ) + calls = [] + states = iter([exact_head, "DISMISSED"]) + monkeypatch.setattr(sched, "run_github_read", lambda args, stdin=None: calls.append(args) or next(states)) + monkeypatch.setattr(sched, "run", lambda args, stdin=None: calls.append(args) or "") + + assert sched.dismiss_stale_opencode_approvals("owner/repo", pr, dry_run=True) == (1, 0) + assert calls == [] + + monkeypatch.delenv("GITHUB_ACTIONS", raising=False) + monkeypatch.setenv("GH_TOKEN", "local-token") + with pytest.raises(RuntimeError, match="refused outside GitHub Actions"): + sched.dismiss_stale_opencode_approvals("owner/repo", pr, dry_run=False) + assert calls == [] + + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "workflow-token") + assert sched.dismiss_stale_opencode_approvals("owner/repo", pr, dry_run=False) == (1, 0) + assert calls[0] == ["gh", "api", "repos/owner/repo/pulls/1", "--jq", ".head.sha"] + assert calls[1][:5] == ["gh", "api", "-X", "PUT", "repos/owner/repo/pulls/1/reviews/301/dismissals"] + assert calls[2] == [ + "gh", + "api", + "repos/owner/repo/pulls/1/reviews/301", + "--jq", + ".state", + ] + + calls.clear() + states = iter([exact_head, "APPROVED"]) + assert sched.dismiss_stale_opencode_approvals("owner/repo", pr, dry_run=False) == (0, 1) + assert "GitHub accepted stale OpenCode review dismissal" in capsys.readouterr().out + assert sched.stale_approval_cleanup_note(0, 1, dry_run=False) == ( + "GitHub retained 1 stale automated approval(s) after dismissal attempts; " + "their head evidence remains non-authoritative" + ) + + calls.clear() + states = iter(["c" * 40]) + with pytest.raises(RuntimeError, match="head changed before stale approval dismissal"): + sched.dismiss_stale_opencode_approvals("owner/repo", pr, dry_run=False) + assert len(calls) == 1 + + +def test_inspect_pr_reports_stale_approval_cleanup_in_final_decision(): + exact_head = "a" * 40 + stale_head = "b" * 40 + pr = make_pr( + headRefOid=exact_head, + reviews={ + "nodes": [ + { + **opencode_review("APPROVED", exact_head), + "databaseId": 301, + "body": f"## Gate evidence\n\n- Head SHA: `{stale_head}`", + } + ] + }, + ) + + decision = inspect(pr) + + assert decision.action == "security_dispatch" + assert decision.notes == ( + "would dismiss 1 latest previous-head automated OpenCode approval(s)", + ) + + +def test_dismiss_pull_request_review_logs_mutation_failures(monkeypatch, capsys): + def fail(_args, stdin=None): + raise RuntimeError("Resource not accessible by integration") + + monkeypatch.setattr(sched, "run", fail) + + assert not sched.dismiss_pull_request_review( + "owner/repo", + "1", + 301, + message="stale review", + ) + assert "Resource not accessible by integration" in capsys.readouterr().out + + def test_print_summary_writes_github_step_summary(monkeypatch, tmp_path, capsys): summary_path = tmp_path / "summary.md" monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary_path)) @@ -3000,6 +3134,49 @@ def followup(updated_pr, **overrides): ) +def test_post_update_branch_followup_dismisses_stale_approval_before_dispatch(monkeypatch): + original = make_pr(headRefOid="old-head") + updated = make_pr( + headRefOid="new-head", + reviews={ + "nodes": [ + { + **opencode_review("APPROVED", "new-head"), + "databaseId": 301, + "body": f"Head SHA: `{'a' * 40}`", + } + ] + }, + statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, + ) + events = [] + monkeypatch.setattr(sched, "wait_for_updated_branch_head", lambda repo, pr: updated) + monkeypatch.setattr( + sched, + "dismiss_stale_opencode_approvals", + lambda repo, pr, dry_run: events.append(("dismiss", pr["headRefOid"])) or (1, 0), + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: events.append(("dispatch", pr["headRefOid"])), + ) + + note = sched.post_update_branch_followup( + "owner/repo", + original, + dry_run=False, + trigger_reviews=True, + review_dispatch_allowed=True, + workflow="OpenCode Review", + security_workflow="Strix Security Scan", + stale_opencode_minutes=45, + ) + + assert events == [("dismiss", "new-head"), ("dispatch", "new-head")] + assert "dismissed 1 latest previous-head automated OpenCode approval(s)" in note + + def test_post_update_branch_followup_waits_for_central_strix_without_dispatch_credential(monkeypatch): monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REF", "main")