From 8f1ace59bdb89f8ab5eb25578c02144d25297c63 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 03:05:42 +0900 Subject: [PATCH 01/16] fix(scheduler): require independent exact-head approval --- CHANGELOG.md | 4 + ...duler-independent-current-head-approval.md | 58 +++++ scripts/ci/pr_review_merge_scheduler.py | 91 ++++++- tests/test_pr_review_merge_scheduler.py | 232 ++++++++++++++---- 4 files changed, 333 insertions(+), 52 deletions(-) create mode 100644 docs/doctoring/scheduler-independent-current-head-approval.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 1630c32d4f..17b584ebe4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,10 @@ Semantic Versioning where the repository publishes a release. ### Changed +- Require the PR Review Merge Scheduler to observe both GitHub's aggregate + `APPROVED` decision and a non-author, non-OpenCode formal approval bound to + the exact live head before direct merge or auto-merge. Existing auto-merge + is disarmed when either authorization is absent. - Emit completed repository pull-list requests as they finish in the five-minute agent-mention sweep, while retaining the four-worker ceiling, rotation, and exact-name dispatch ledger, so one slow repository cannot hide ready sibling diff --git a/docs/doctoring/scheduler-independent-current-head-approval.md b/docs/doctoring/scheduler-independent-current-head-approval.md new file mode 100644 index 0000000000..0e6c0c2c26 --- /dev/null +++ b/docs/doctoring/scheduler-independent-current-head-approval.md @@ -0,0 +1,58 @@ +# Scheduler independent exact-head approval + +## Decision + +The organization merge scheduler fails closed unless both of these statements +are true: + +1. GitHub reports the pull request's aggregate `reviewDecision` as `APPROVED`. +2. A formal `APPROVED` review from a non-author, non-OpenCode identity is bound + to the exact live head SHA. + +Exact-head OpenCode approval remains a review gate, not independent merge +authority. Missing identities, author self-review, generic GitHub Actions +reviews, comment-only reviews, predecessor-head approvals, and absent aggregate +state never satisfy this control. + +## Root cause and repair + +The prior scheduler could call direct merge or enable native auto-merge after an +exact-head OpenCode approval without first proving repository approval state or +an independent exact-head review. A credential with bypass capability could +therefore turn advisory automation evidence into merge authority. + +The repair reuses the existing scheduler and review/head matcher: + +- the existing GitHub query now includes the pull-request author; +- the REST fallback records the author but remains fail closed because it does + not provide an authoritative aggregate review decision; +- one helper filters exact-head formal approvals by author and automation + identity; +- both direct/automatic merge paths share the same authorization reason; and +- an already armed auto-merge request is disabled when authorization is absent. + +Checks, security evidence, unresolved conversations, mergeability, branch +freshness, and expected-head merge protection remain independent blockers. +GitHub's server-side last-pusher, CODEOWNERS, required-review, and ruleset checks +remain authoritative; the scheduler does not infer those identities. + +## Verification and operations + +Regression cases cover missing author/reviewer identity, self-review, generic +Actions review, OpenCode review, non-approval, stale head, missing aggregate +approval, disarming existing auto-merge, and the complete authorized direct +merge path. Before lifecycle action, refetch the live base, head, reviews, +threads, checks, and rules. A head change invalidates all predecessor evidence. + +Rollback means reverting the reviewed scheduler change while keeping scheduler +merge modes disabled until an equivalent fail-closed authorization control is +available. Never roll back by lowering required-review counts or granting a +bypass. + +## References + +GitHub. (n.d.). *About protected branches*. GitHub Docs. Retrieved August 24, +2026, from https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches + +GitHub. (n.d.). *Available rules for rulesets*. GitHub Docs. Retrieved August +24, 2026, from https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/available-rules-for-rulesets diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 44620fcab5..d3d98f10eb 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -24,6 +24,7 @@ fragment SchedulerPullRequestFields on PullRequest { number title + author { login } isDraft mergeable mergeStateStatus @@ -807,6 +808,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: return { "number": number, "title": pr.get("title"), + "author": {"login": ((pr.get("user") or {}).get("login"))}, "isDraft": bool(pr.get("draft")), "mergeable": pr.get("mergeable"), "mergeStateStatus": rest_merge_state, @@ -1272,6 +1274,41 @@ def has_current_head_approval(pr: dict[str, Any]) -> bool: return current_head_review_state(pr, "APPROVED") +def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: + """Return whether a non-author, non-OpenCode reviewer approved the exact head.""" + author = ((pr.get("author") or {}).get("login") or "").lower() + if not author: + return False + for review in reversed((pr.get("reviews") or {}).get("nodes") or []): + reviewer = review_author_login(review) + if ( + (review.get("state") or "").upper() == "APPROVED" + and review_matches_current_head(review, pr) + and reviewer + and reviewer != author + and not is_automated_opencode_review(review) + and reviewer not in {"github-actions", "github-actions[bot]"} + ): + return True + return False + + +def merge_approval_block_reason(pr: dict[str, Any]) -> str | None: + """Return the fail-closed repository and independent approval blocker.""" + review_decision = str(pr.get("reviewDecision") or "").upper() + if review_decision != "APPROVED": + return ( + "current-head OpenCode review approved, but GitHub reviewDecision is " + f"{review_decision or ''}; repository approval policy is unsatisfied" + ) + if not has_independent_current_head_approval(pr): + return ( + "current-head OpenCode review approved, but no independent non-author " + "exact-current-head formal APPROVED review exists" + ) + return None + + def has_current_head_changes_requested(pr: dict[str, Any]) -> bool: """Return whether OpenCode requested changes on the exact current head.""" return current_head_review_state(pr, "CHANGES_REQUESTED") @@ -2504,6 +2541,7 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return decide("block", "current-head OpenCode review requested changes") current_head_approved = has_current_head_approval(pr) + approval_reason = merge_approval_block_reason(pr) if current_head_approved else None if current_head_approved: stale_review_cleanup_count = dismiss_stale_opencode_change_requests( repo, @@ -2511,6 +2549,18 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio dry_run=dry_run, ) auto_merge_enabled = bool(pr.get("autoMergeRequest")) + if approval_reason and auto_merge_enabled: + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + f"{approval_reason}; obtain fresh independent approval before " + "re-enabling auto-merge" + ), + ) + ) if merge_state in {"DIRTY", "CONFLICTING"}: conflict_reason = merge_conflict_guidance(pr, merge_state) if current_head_approved: @@ -2579,6 +2629,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio merge_state == "CLEAN" or merge_mode in {"direct", "direct_or_auto"} ) if current_head_approved and merge_before_update: + if approval_reason: + return decide("wait", approval_reason) if not same_repository_head(repo, pr): return decide("wait", external_head_merge_reason(repo, pr)) if not enable_auto_merge_flag: @@ -2739,6 +2791,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return decide("wait", "mergeability is still being calculated and no branch freshness evidence is available") if current_head_approved: + if approval_reason: + return decide("wait", approval_reason) if pr.get("autoMergeRequest"): return decide("wait", auto_merge_wait_reason(merge_state, pr)) if not same_repository_head(repo, pr): @@ -3336,6 +3390,7 @@ def self_test_scheduler_invariants() -> None: pass sample = { "number": 1, + "author": {"login": "pull-request-author"}, "headRefOid": "abc", "baseRefName": "main", "baseRefOid": "base", @@ -3346,7 +3401,7 @@ def self_test_scheduler_invariants() -> None: "isCrossRepository": False, "maintainerCanModify": False, "headRepository": {"nameWithOwner": "owner/repo"}, - "reviewDecision": "REVIEW_REQUIRED", + "reviewDecision": "APPROVED", "commits": { "nodes": [ { @@ -3367,7 +3422,13 @@ def self_test_scheduler_invariants() -> None: "body": "OpenCode Agent approved this head.", "submittedAt": "2026-06-25T15:42:19Z", "commit": {"oid": "abc"}, - } + }, + { + "state": "APPROVED", + "author": {"login": "independent-reviewer"}, + "submittedAt": "2026-06-25T15:43:19Z", + "commit": {"oid": "abc"}, + }, ] }, "statusCheckRollup": {"contexts": {"nodes": []}}, @@ -3601,6 +3662,13 @@ def self_test_scheduler_invariants() -> None: sample["isCrossRepository"] = False sample["maintainerCanModify"] = False sample["autoMergeRequest"] = {"enabledAt": "2026-01-01T00:02:00Z"} + sample["reviews"]["nodes"].append( + { + "state": "APPROVED", + "author": {"login": "independent-reviewer"}, + "commit": {"oid": "abc"}, + } + ) decision = inspect_pr( "owner/repo", sample, @@ -3705,6 +3773,7 @@ def self_test_scheduler_invariants() -> None: assert "git status --short" in conflict_guidance["commands"] blocked_sample = { "number": 2, + "author": {"login": "pull-request-author"}, "headRefOid": "abc", "baseRefName": "main", "baseRefOid": "base", @@ -3733,14 +3802,20 @@ def self_test_scheduler_invariants() -> None: "reviewThreads": {"nodes": []}, "reviews": { "nodes": [ - { - "state": "APPROVED", - "author": {"login": "opencode-agent"}, + { + "state": "APPROVED", + "author": {"login": "opencode-agent"}, "body": "OpenCode Agent approved this head.", "submittedAt": "2026-06-25T15:42:19Z", - "commit": {"oid": "abc"}, - } - ] + "commit": {"oid": "abc"}, + }, + { + "state": "APPROVED", + "author": {"login": "independent-reviewer"}, + "submittedAt": "2026-06-25T15:43:19Z", + "commit": {"oid": "abc"}, + }, + ] }, "statusCheckRollup": { "contexts": { diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 0e71bdbe24..99e5a54c23 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -47,6 +47,7 @@ def make_pr(**overrides): value = { "number": 1, "title": "Central review", + "author": {"login": "pull-request-author"}, "isDraft": False, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", @@ -94,6 +95,16 @@ def opencode_review( } +def merge_approved_reviews(commit="head"): + """Return exact-head OpenCode and independent formal approvals.""" + return { + "nodes": [ + opencode_review("APPROVED", commit), + opencode_review("APPROVED", commit, login="independent-reviewer"), + ] + } + + def strix_check(status="COMPLETED", conclusion="SUCCESS", workflow="Strix Security Scan", details_url=None): value = { "__typename": "CheckRun", @@ -141,7 +152,7 @@ def last_push_restamp_candidate(**overrides): restMergeableState="BLOCKED", reviewDecision="APPROVED", autoMergeRequest={"enabledAt": "now"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviews=merge_approved_reviews(), statusCheckRollup={"contexts": {"nodes": [strix_check()]}}, commits={ "nodes": [ @@ -1384,6 +1395,107 @@ def test_review_state_and_failed_checks(): assert sched.failed_status_checks(manual_opencode_supersedes_pr_target_failure) == ["lint"] +def test_scheduler_query_requests_pull_request_author(): + """Fetch the authoritative author identity used by the independent-review gate.""" + assert "\n author { login }\n" in sched.PULL_REQUEST_FIELDS_FRAGMENT + + +@pytest.mark.parametrize( + ("author", "reviewer", "state", "commit"), + ( + ("", "independent-reviewer", "APPROVED", "head"), + ("pull-request-author", "", "APPROVED", "head"), + ("pull-request-author", "pull-request-author", "APPROVED", "head"), + ("pull-request-author", "opencode-agent", "APPROVED", "head"), + ("pull-request-author", "github-actions[bot]", "APPROVED", "head"), + ("pull-request-author", "independent-reviewer", "COMMENTED", "head"), + ("pull-request-author", "independent-reviewer", "APPROVED", "old"), + ), +) +def test_independent_approval_fails_closed_for_invalid_evidence( + author, + reviewer, + state, + commit, +): + """Reject missing, self, automation, non-approval, and stale-head evidence.""" + pr = make_pr( + author={"login": author}, + reviewDecision="APPROVED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review(state, commit, login=reviewer), + ] + }, + ) + + assert not sched.has_independent_current_head_approval(pr) + assert "independent" in sched.merge_approval_block_reason(pr).lower() + + +def test_independent_exact_head_approval_allows_direct_merge(): + """Preserve direct merge only when GitHub and independent evidence both pass.""" + pr = make_pr( + reviewDecision="APPROVED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review("APPROVED", "head", login="independent-reviewer"), + ] + }, + ) + + assert sched.has_independent_current_head_approval(pr) + assert sched.merge_approval_block_reason(pr) is None + assert inspect(pr, merge_mode="direct").action == "merge" + + +@pytest.mark.parametrize("review_decision", ("", "REVIEW_REQUIRED")) +def test_repository_review_policy_blocks_merge_and_disarms_auto_merge(review_decision): + """Never leave merge authority armed while GitHub review policy is unsatisfied.""" + reviews = { + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review("APPROVED", "head", login="independent-reviewer"), + ] + } + blocked = make_pr(reviewDecision=review_decision, reviews=reviews) + blocked_mergeability = make_pr( + mergeStateStatus="BLOCKED", + reviewDecision=review_decision, + reviews=reviews, + ) + armed = make_pr( + reviewDecision=review_decision, + reviews=reviews, + autoMergeRequest={"enabledAt": "now"}, + ) + + decision = inspect(blocked, merge_mode="direct") + disarm = inspect(armed, merge_mode="direct") + + assert decision.action == "wait" + assert "reviewDecision" in decision.reason + assert inspect(blocked_mergeability, merge_mode="direct").action == "wait" + assert disarm.action == "disable_auto_merge" + assert "reviewDecision" in disarm.reason + + +def test_missing_independent_approval_blocks_and_disarms_auto_merge(): + """Do not let aggregate GitHub state substitute for exact independent evidence.""" + reviews = {"nodes": [opencode_review("APPROVED", "head")]} + blocked = make_pr(reviewDecision="APPROVED", reviews=reviews) + armed = make_pr( + reviewDecision="APPROVED", + reviews=reviews, + autoMergeRequest={"enabledAt": "now"}, + ) + + assert inspect(blocked, merge_mode="direct").action == "wait" + assert inspect(armed, merge_mode="direct").action == "disable_auto_merge" + + def test_workflow_run_followup_defers_deterministic_fallback_retry(monkeypatch): head = "a" * 40 fallback_review = { @@ -1437,9 +1549,11 @@ def test_body_head_sha_approval_prevents_same_run_opencode_rerun(monkeypatch): { **opencode_review("APPROVED", ""), "body": f"## Gate evidence\n\n- Head SHA: `{head}`", - } + }, + opencode_review("APPROVED", head, login="independent-reviewer"), ] }, + reviewDecision="APPROVED", statusCheckRollup={ "contexts": { "nodes": [ @@ -1495,11 +1609,13 @@ def test_current_head_approval_cleans_previous_head_change_gate_before_merge(): { **opencode_review("CHANGES_REQUESTED", "old"), "databaseId": 301, - }, - opencode_review("APPROVED", "head"), - ] - } - ) + }, + opencode_review("APPROVED", "head"), + opencode_review("APPROVED", "head", login="independent-reviewer"), + ] + }, + reviewDecision="APPROVED", + ) decision = inspect(pr) @@ -3053,7 +3169,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): make_pr( mergeStateStatus="BEHIND", restMergeableState="CLEAN", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) ) assert rest_clean.action == "auto_merge" @@ -3061,7 +3178,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): outdated_only = inspect( make_pr( reviewThreads={"nodes": [{"id": "outdated-thread", "isResolved": False, "isOutdated": True}]}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) ) assert outdated_only.action == "auto_merge" @@ -3162,7 +3280,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): assert "workflow action required: opencode-review" in action_required_auto.reason same_head_auto = make_pr( autoMergeRequest={"enabledAt": "now"}, - reviews={"nodes": [opencode_review("APPROVED", "head", submitted_at="2026-06-25T06:59:59Z")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) disabled = [] monkeypatch.setattr(sched, "disable_auto_merge", lambda repo, pr, dry_run: disabled.append((repo, pr["number"], dry_run))) @@ -3179,11 +3298,9 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): reviews={"nodes": [opencode_review("APPROVED", "head")]}, ) blocked_auto_decision = inspect(blocked_auto) - assert blocked_auto_decision.action == "wait" - assert "GitHub mergeability is BLOCKED" in blocked_auto_decision.reason + assert blocked_auto_decision.action == "disable_auto_merge" assert "GitHub reviewDecision is REVIEW_REQUIRED" in blocked_auto_decision.reason - assert "required approving review" in blocked_auto_decision.reason - assert "rerun the scheduler" in blocked_auto_decision.reason + assert disabled == [("owner/repo", 1, True)] assert sched.latest_commit_headline(make_pr(commits={"nodes": []})) == "" restamp_candidate = last_push_restamp_candidate() @@ -3214,6 +3331,13 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): current_head_approved=True, auto_merge_enabled=True, ) + assert not sched.should_restamp_for_last_push_approval( + "owner/repo", + last_push_restamp_candidate(reviewDecision="REVIEW_REQUIRED"), + "BLOCKED", + current_head_approved=True, + auto_merge_enabled=True, + ) disabled_restamp = inspect(restamp_candidate, update_branches=False) assert disabled_restamp.action == "wait" @@ -3360,7 +3484,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): called.clear() behind_auto_merge_enabled = make_pr( mergeStateStatus="BEHIND", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ) disabled.clear() @@ -3373,7 +3498,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): rest_behind = make_pr( mergeStateStatus="CLEAN", restMergeableState="BEHIND", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ) rest_behind_decision = inspect(rest_behind) @@ -3391,7 +3517,8 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): mergeStateStatus="BLOCKED", restMergeableState="BLOCKED", compareBehindBy=2, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, statusCheckRollup={ "contexts": { @@ -3565,7 +3692,8 @@ def test_inspect_pr_blocks_auto_merge_for_approved_conflicts(monkeypatch): approved_conflict = make_pr( mergeStateStatus="DIRTY", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) decision = inspect(approved_conflict) assert decision.action == "block" @@ -3576,10 +3704,11 @@ def test_inspect_pr_blocks_auto_merge_for_approved_conflicts(monkeypatch): assert disables == [] already_queued = inspect( - make_pr( - mergeStateStatus="CONFLICTING", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, - autoMergeRequest={"enabledAt": "now"}, + make_pr( + mergeStateStatus="CONFLICTING", + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), + autoMergeRequest={"enabledAt": "now"}, ) ) assert already_queued.action == "disable_auto_merge" @@ -3684,7 +3813,8 @@ def test_inspect_pr_dispatches_strix_after_update_branch_observes_new_head(monke dispatched = [] old_head_pr = make_pr( mergeStateStatus="BEHIND", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ) new_head_pr = make_pr(headRefOid="new-head", reviews={"nodes": []}) @@ -3990,17 +4120,19 @@ def test_update_branch_summary_includes_followup_notes(): def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): - approved = make_pr(reviews={"nodes": [opencode_review("APPROVED", "head")]}) + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) failed = make_pr( - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), statusCheckRollup={"contexts": {"nodes": [{"__typename": "CheckRun", "name": "strix", "conclusion": "FAILURE"}]}}, ) assert inspect(failed).reason == "failed check(s): strix" - assert inspect(make_pr(reviews={"nodes": [opencode_review("APPROVED", "head")]}, autoMergeRequest={"enabledAt": "now"})).reason == ( + assert inspect(make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"})).reason == ( "current head is approved; auto-merge already enabled" ) approved_with_auto_merge = make_pr( - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ) assert inspect(approved_with_auto_merge, enable_auto_merge_flag=False).reason == ( @@ -4020,7 +4152,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): ) blocked_approved = make_pr( mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) assert inspect(blocked_approved, enable_auto_merge_flag=False).reason == ( "current head is approved; auto-merge disabled by scheduler inputs" @@ -4034,7 +4167,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): blocked_unmergeable = make_pr( mergeable="UNKNOWN", mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) assert inspect(blocked_unmergeable, enable_auto_merge_flag=False).reason == ( "current head is approved; auto-merge disabled by scheduler inputs" @@ -4057,7 +4191,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): mergeStateStatus="BLOCKED", isCrossRepository=True, headRepository={"nameWithOwner": "fork/repo"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4073,7 +4208,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): blocked_direct = inspect( make_pr( mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct", ) @@ -4094,7 +4230,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): already_auto_direct_or_auto = inspect( make_pr( autoMergeRequest={"enabledAt": "now"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4110,7 +4247,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): make_pr( mergeStateStatus="CLEAN", compareBehindBy=20, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4127,7 +4265,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): make_pr( mergeStateStatus="BLOCKED", compareBehindBy=20, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4148,7 +4287,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): blocked_direct_or_auto = inspect( make_pr( mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4167,15 +4307,14 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): make_pr( mergeStateStatus="BLOCKED", autoMergeRequest={"enabledAt": "now"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) assert blocked_already_auto.action == "wait" assert "auto-merge is already enabled" in blocked_already_auto.reason assert "GitHub mergeability is BLOCKED" in blocked_already_auto.reason - assert "GitHub reviewDecision is REVIEW_REQUIRED" in blocked_already_auto.reason - assert "required approving review" in blocked_already_auto.reason assert direct_merges == [ ("owner/repo", 1, True), ("owner/repo", 1, True), @@ -4195,7 +4334,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): make_pr( isCrossRepository=True, headRepository={"nameWithOwner": "fork/repo"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4207,7 +4347,8 @@ def test_inspect_pr_handles_approved_reviews_and_dispatch(monkeypatch): mergeStateStatus="BLOCKED", isCrossRepository=True, headRepository={"nameWithOwner": "fork/repo"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4319,7 +4460,7 @@ def test_inspect_pr_waits_when_same_head_dispatch_is_already_running(monkeypatch def test_direct_or_auto_falls_back_to_auto_merge_when_branch_policy_blocks_direct_merge(monkeypatch): - approved = make_pr(reviews={"nodes": [opencode_review("APPROVED", "head")]}) + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) auto_merges = [] def policy_blocked_merge(repo, pr, dry_run): @@ -4347,7 +4488,8 @@ def policy_blocked_merge(repo, pr, dry_run): already_queued = inspect( make_pr( autoMergeRequest={"enabledAt": "now"}, - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ), merge_mode="direct_or_auto", ) @@ -4358,7 +4500,8 @@ def policy_blocked_merge(repo, pr, dry_run): blocked = make_pr( mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) blocked_decision = inspect(blocked, merge_mode="direct_or_auto") @@ -4383,7 +4526,8 @@ def non_policy_merge_failure(repo, pr, dry_run): def test_direct_or_auto_attempts_direct_merge_when_mergeability_is_blocked(monkeypatch): approved = make_pr( mergeStateStatus="BLOCKED", - reviews={"nodes": [opencode_review("APPROVED", "head")]}, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), ) direct_merges = [] From bdeae495f5154ab0bfce1e9dd1ba607878e43b3e Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 03:20:31 +0900 Subject: [PATCH 02/16] fix(scheduler): honor latest independent review state --- CHANGELOG.md | 7 ++--- ...duler-independent-current-head-approval.md | 12 +++++---- scripts/ci/pr_review_merge_scheduler.py | 20 +++++++++----- tests/test_pr_review_merge_scheduler.py | 26 +++++++++++++++++++ 4 files changed, 50 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17b584ebe4..9839febd41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,9 +53,10 @@ Semantic Versioning where the repository publishes a release. ### Changed - Require the PR Review Merge Scheduler to observe both GitHub's aggregate - `APPROVED` decision and a non-author, non-OpenCode formal approval bound to - the exact live head before direct merge or auto-merge. Existing auto-merge - is disarmed when either authorization is absent. + `APPROVED` decision and the latest effective non-author, non-OpenCode formal + approval bound to the exact live head before direct merge or auto-merge. + A later same-head change request revokes that reviewer's earlier approval, + and existing auto-merge is disarmed when either authorization is absent. - Emit completed repository pull-list requests as they finish in the five-minute agent-mention sweep, while retaining the four-worker ceiling, rotation, and exact-name dispatch ledger, so one slow repository cannot hide ready sibling diff --git a/docs/doctoring/scheduler-independent-current-head-approval.md b/docs/doctoring/scheduler-independent-current-head-approval.md index 0e6c0c2c26..ab7c5f2051 100644 --- a/docs/doctoring/scheduler-independent-current-head-approval.md +++ b/docs/doctoring/scheduler-independent-current-head-approval.md @@ -27,7 +27,8 @@ The repair reuses the existing scheduler and review/head matcher: - the REST fallback records the author but remains fail closed because it does not provide an authoritative aggregate review decision; - one helper filters exact-head formal approvals by author and automation - identity; + identity, considering only each reviewer's latest approval-affecting state so + a later change request or dismissal revokes that reviewer's earlier approval; - both direct/automatic merge paths share the same authorization reason; and - an already armed auto-merge request is disabled when authorization is absent. @@ -39,10 +40,11 @@ remain authoritative; the scheduler does not infer those identities. ## Verification and operations Regression cases cover missing author/reviewer identity, self-review, generic -Actions review, OpenCode review, non-approval, stale head, missing aggregate -approval, disarming existing auto-merge, and the complete authorized direct -merge path. Before lifecycle action, refetch the live base, head, reviews, -threads, checks, and rules. A head change invalidates all predecessor evidence. +Actions review, OpenCode review, non-approval, stale head, a later same-head +change request, missing aggregate approval, disarming existing auto-merge, and +the complete authorized direct merge path. Before lifecycle action, refetch the +live base, head, reviews, threads, checks, and rules. A head change invalidates +all predecessor evidence. Rollback means reverting the reviewed scheduler change while keeping scheduler merge modes disabled until an equivalent fail-closed authorization control is diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index d3d98f10eb..34dc844b99 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1275,20 +1275,26 @@ def has_current_head_approval(pr: dict[str, Any]) -> bool: def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: - """Return whether a non-author, non-OpenCode reviewer approved the exact head.""" + """Return whether an eligible reviewer's latest exact-head policy state approves.""" author = ((pr.get("author") or {}).get("login") or "").lower() if not author: return False + seen_reviewers: set[str] = set() for review in reversed((pr.get("reviews") or {}).get("nodes") or []): reviewer = review_author_login(review) + state = (review.get("state") or "").upper() if ( - (review.get("state") or "").upper() == "APPROVED" - and review_matches_current_head(review, pr) - and reviewer - and reviewer != author - and not is_automated_opencode_review(review) - and reviewer not in {"github-actions", "github-actions[bot]"} + not reviewer + or reviewer == author + or is_automated_opencode_review(review) + or reviewer in {"github-actions", "github-actions[bot]"} + or not review_matches_current_head(review, pr) + or state not in {"APPROVED", "CHANGES_REQUESTED", "DISMISSED"} + or reviewer in seen_reviewers ): + continue + seen_reviewers.add(reviewer) + if state == "APPROVED": return True return False diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 99e5a54c23..bdced4e564 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1451,6 +1451,32 @@ def test_independent_exact_head_approval_allows_direct_merge(): assert inspect(pr, merge_mode="direct").action == "merge" +def test_independent_approval_uses_reviewers_latest_policy_state(): + """A later same-head change request revokes that reviewer's approval.""" + pr = make_pr( + reviewDecision="CHANGES_REQUESTED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review( + "APPROVED", + "head", + login="independent-reviewer", + submitted_at="2026-06-25T07:01:00Z", + ), + opencode_review( + "CHANGES_REQUESTED", + "head", + login="independent-reviewer", + submitted_at="2026-06-25T07:02:00Z", + ), + ] + }, + ) + + assert not sched.has_independent_current_head_approval(pr) + + @pytest.mark.parametrize("review_decision", ("", "REVIEW_REQUIRED")) def test_repository_review_policy_blocks_merge_and_disarms_auto_merge(review_decision): """Never leave merge authority armed while GitHub review policy is unsatisfied.""" From 9069eed9632dcb627cfbcf5997ea2026ef75d549 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 00:20:09 -0700 Subject: [PATCH 03/16] fix(scheduler): retry OpenCode after coverage blockers clear (#1266) * fix: retry reviews after coverage blockers clear * fix(scheduler): match coverage blocker review body * test(scheduler): cover coverage evidence gate branches * test(scheduler): reproduce coverage retry self-block * fix(scheduler): unblock coverage-only review retry * fix(scheduler): preserve central coverage retry * test(scheduler): cover status-only coverage retry * fix(scheduler): prefer newest coverage rerun * fix(scheduler): keep coverage retries fail closed * fix(scheduler): ignore superseded coverage failures * fix(scheduler): bind coverage retries to one check snapshot * fix(scheduler): avoid duplicate coverage review dispatch * fix(scheduler): rate-limit same-head coverage retries * fix(scheduler): wait for active coverage re-review * fix(scheduler): disable auto-merge during coverage retry floor * fix(scheduler): bound coverage retry dispatches --- CHANGELOG.md | 7 + scripts/ci/pr_review_merge_scheduler.py | 363 ++++++++- tests/test_pr_review_merge_scheduler.py | 952 +++++++++++++++++++++++- 3 files changed, 1279 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9839febd41..681de5d352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,13 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Re-dispatch an exact-head OpenCode review after its coverage-only blocker is + cleared, selecting the newest coverage rerun by timestamp across workflow + names and ignoring only the superseded `opencode-review` failure and central + required-workflow placeholder. Conflicting heads and failed sibling jobs in an + OpenCode workflow remain fail-closed alongside unresolved threads, Strix, + coverage, and unrelated failed checks. + - Publish only the sanitized cumulative Strix report tree, avoiding a later copy of relative scanner output that could reintroduce known internal warning text into uploaded security evidence. diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 34dc844b99..ccf73c00d5 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -15,7 +15,7 @@ import time from collections.abc import Iterator, Sequence from dataclasses import dataclass -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from typing import Any from urllib.parse import quote @@ -118,6 +118,7 @@ # remains deliberately larger than the job cap while recovering genuine zombie # checks in the same operating window instead of leaving them for seven hours. DEFAULT_STALE_OPENCODE_MINUTES = 90 +DEFAULT_COVERAGE_RETRY_FLOOR_MINUTES = 60 DEFAULT_UPDATE_BRANCH_HEAD_POLL_ATTEMPTS = 6 DEFAULT_UPDATE_BRANCH_HEAD_POLL_SECONDS = 5.0 OPENCODE_WORKFLOW_NAMES = { @@ -160,6 +161,11 @@ "deterministic fallback approval", "did not emit a usable current-head control block", ) +COVERAGE_REVIEW_MARKERS = ( + "coverage evidence did not pass", + "coverage-evidence", + "required test/docstring evidence", +) LAST_PUSH_APPROVAL_RESTAMP_MESSAGE = "chore: refresh head for last-push approval" @@ -1023,6 +1029,20 @@ def context_nodes(pr: dict[str, Any]) -> list[dict[str, Any]]: return contexts.get("nodes") or [] +def is_opencode_check_run(node: dict[str, Any]) -> bool: + """Return whether a CheckRun carries the OpenCode workflow identity.""" + if node.get("__typename") != "CheckRun": + return False + workflow = ( + ((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") + or {} + ) + return ( + node.get("name") == "opencode-review" + or workflow.get("name") in OPENCODE_WORKFLOW_NAMES + ) + + def is_opencode_context(node: dict[str, Any]) -> bool: """Return whether a check or status context belongs to OpenCode Review.""" if node.get("__typename") == "CheckRun": @@ -1031,11 +1051,7 @@ def is_opencode_context(node: dict[str, Any]) -> bool: # status. Organization required-workflow CheckRuns are deliberately # non-authoritative placeholders and must not suppress that dispatch. return False - workflow = ( - ((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") - or {} - ) - return node.get("name") == "opencode-review" or workflow.get("name") in OPENCODE_WORKFLOW_NAMES + return is_opencode_check_run(node) return node.get("context") == "opencode-review" @@ -1085,6 +1101,39 @@ def parse_github_datetime(value: str | None) -> datetime | None: return parsed.astimezone(timezone.utc) +def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: + """Return the newest check run for each workflow and check-name pair.""" + latest: dict[ + tuple[str, str], + tuple[datetime | None, int, dict[str, Any]], + ] = {} + for index, node in enumerate(context_nodes(pr)): + if node.get("__typename") != "CheckRun": + continue + workflow = ( + (((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name") + or "" + ) + key = (workflow, node.get("name") or "check-run") + started_at = parse_github_datetime(node.get("startedAt")) + previous = latest.get(key) + if previous is None: + latest[key] = (started_at, index, node) + continue + previous_started_at, previous_index, _ = previous + if started_at is None and previous_started_at is not None: + continue + if previous_started_at is None and started_at is not None: + latest[key] = (started_at, index, node) + continue + if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= ( + previous_started_at or datetime.min.replace(tzinfo=timezone.utc), + previous_index, + ): + latest[key] = (started_at, index, node) + return [node for _, _, node in sorted(latest.values(), key=lambda item: item[1])] + + def review_matches_current_head(review: dict[str, Any], pr: dict[str, Any]) -> bool: """Return whether a review is valid evidence for the current head commit.""" head = pr.get("headRefOid") @@ -1320,6 +1369,117 @@ def has_current_head_changes_requested(pr: dict[str, Any]) -> bool: return current_head_review_state(pr, "CHANGES_REQUESTED") +def latest_current_head_coverage_change_request( + pr: dict[str, Any], +) -> dict[str, Any] | None: + """Return the latest exact-head OpenCode request that only cites coverage.""" + for review in reversed((pr.get("reviews") or {}).get("nodes") or []): + if not is_opencode_review(review) or not review_matches_current_head(review, pr): + continue + if (review.get("state") or "").upper() != "CHANGES_REQUESTED": + return None + body = (review.get("body") or "").lower() + return review if all(marker in body for marker in COVERAGE_REVIEW_MARKERS) else None + return None + + +def current_head_coverage_change_request(pr: dict[str, Any]) -> bool: + """Return whether the latest current-head request is only a coverage gate.""" + return latest_current_head_coverage_change_request(pr) is not None + + +def coverage_retry_wait_reason( + pr: dict[str, Any], + *, + repo: str | None = None, + workflow: str | None = None, + now: datetime | None = None, + floor_minutes: int = DEFAULT_COVERAGE_RETRY_FLOOR_MINUTES, +) -> str | None: + """Return a wait reason until one same-head coverage retry interval elapses. + + The latest exact-head review submission or completed dispatch timestamp is the + durable same-head retry marker. Missing or malformed timestamps fail closed so + a repeated coverage-only review cannot create an unbounded dispatch loop. + """ + review = latest_current_head_coverage_change_request(pr) + if review is None: + return None + submitted_at = parse_github_datetime(review.get("submittedAt")) + if submitted_at is None: + return "current-head OpenCode coverage review has no valid submission timestamp; defer same-head re-review" + retry_anchor = submitted_at + if repo and workflow: + try: + dispatch_started_at = latest_opencode_dispatch_started_at(repo, workflow, pr) + except RuntimeError: + return "same-head OpenCode dispatch history is unavailable; defer same-head re-review" + if dispatch_started_at and dispatch_started_at > retry_anchor: + retry_anchor = dispatch_started_at + current_time = now or datetime.now(timezone.utc) + if current_time < retry_anchor + timedelta(minutes=max(0, floor_minutes)): + return "same-head OpenCode coverage retry floor has not elapsed" + return None + + +def coverage_evidence_indices(check_runs: Sequence[dict[str, Any]]) -> list[int]: + """Return indexes of coverage-evidence checks in one check-run snapshot.""" + return [ + index + for index, node in enumerate(check_runs) + if (node.get("name") or "").lower() == "coverage-evidence" + ] + + +def latest_coverage_evidence_index(check_runs: Sequence[dict[str, Any]]) -> int | None: + """Return the newest coverage-evidence index across workflow names.""" + coverage_indices = coverage_evidence_indices(check_runs) + if not coverage_indices: + return None + return max( + coverage_indices, + key=lambda item: ( + parse_github_datetime(check_runs[item].get("startedAt")) + or datetime.min.replace(tzinfo=timezone.utc), + item, + ), + ) + + +def coverage_evidence_state(pr: dict[str, Any]) -> str: + """Return missing, running, complete, or failed for the latest coverage gate.""" + check_runs = latest_check_runs(pr) + latest_index = latest_coverage_evidence_index(check_runs) + if latest_index is not None: + node = check_runs[latest_index] + status = (node.get("status") or "").upper() + if status in RUNNING_CHECK_STATES: + return "running" + return "complete" if (node.get("conclusion") or "").upper() == "SUCCESS" else "failed" + for node in reversed(context_nodes(pr)): + if node.get("__typename") == "CheckRun": + continue + name = (node.get("name") or node.get("context") or "").lower() + if name != "coverage-evidence": + continue + status = (node.get("status") or node.get("state") or "").upper() + if status in RUNNING_CHECK_STATES: + return "running" + return "complete" if status == "SUCCESS" else "failed" + return "missing" + + +def superseded_coverage_evidence_indices(check_runs: Sequence[dict[str, Any]]) -> set[int]: + """Return older coverage checks superseded by a newer successful run.""" + authoritative_index = latest_coverage_evidence_index(check_runs) + if authoritative_index is None: + return set() + authoritative = check_runs[authoritative_index] + if (authoritative.get("conclusion") or "").upper() != "SUCCESS": + return set() + return set(coverage_evidence_indices(check_runs)) - {authoritative_index} + + def stale_opencode_change_request_ids(pr: dict[str, Any]) -> list[int]: """Return dismissible automated change requests tied to previous heads.""" review_ids: list[int] = [] @@ -1502,48 +1662,41 @@ def dismiss_stale_opencode_change_requests(repo: str, pr: dict[str, Any], *, dry return len(review_ids) -def failed_status_checks(pr: dict[str, Any]) -> list[str]: - """Return failing check or status context names from the PR rollup.""" +def failed_status_checks( + pr: dict[str, Any], + *, + ignore_opencode: bool = False, +) -> list[str]: + """Return failing check or status context names from the PR rollup. + + ``ignore_opencode`` is reserved for the authenticated coverage-only retry + path: the previous ``opencode-review`` job or status is expected to be + failing there because it published the current-head coverage change request + being retried. Sibling jobs in the same workflow remain authoritative. + """ failed: list[str] = [] - latest_check_runs: dict[ - tuple[str, str], - tuple[datetime | None, int, dict[str, Any]], - ] = {} - status_contexts: list[dict[str, Any]] = [] - for index, node in enumerate(context_nodes(pr)): - if node.get("__typename") != "CheckRun": - status_contexts.append(node) - continue - workflow = ( - (((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name") - or "" - ) - key = (workflow, node.get("name") or "check-run") - started_at = parse_github_datetime(node.get("startedAt")) - previous = latest_check_runs.get(key) - if previous is None: - latest_check_runs[key] = (started_at, index, node) - continue - previous_started_at, previous_index, _ = previous - if started_at is None and previous_started_at is not None: - continue - if previous_started_at is None and started_at is not None: - latest_check_runs[key] = (started_at, index, node) - continue - if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= ( - previous_started_at or datetime.min.replace(tzinfo=timezone.utc), - previous_index, - ): - latest_check_runs[key] = (started_at, index, node) + check_runs = latest_check_runs(pr) + superseded_coverage_indices = ( + superseded_coverage_evidence_indices(check_runs) if ignore_opencode else set() + ) + status_contexts = [ + node + for node in context_nodes(pr) + if node.get("__typename") != "CheckRun" + ] successful_status_contexts = { node.get("context") for node in status_contexts if (node.get("state") or "").upper() == "SUCCESS" } - for _, _, node in sorted(latest_check_runs.values(), key=lambda item: item[1]): + for index, node in enumerate(check_runs): conclusion = (node.get("conclusion") or "").upper() if conclusion in FAILED_CHECK_CONCLUSIONS: + if index in superseded_coverage_indices: + continue + if ignore_opencode and node.get("name") == "opencode-review": + continue if is_strix_context(node) and "strix" in successful_status_contexts: continue if is_opencode_context(node) and "opencode-review" in successful_status_contexts: @@ -1552,6 +1705,8 @@ def failed_status_checks(pr: dict[str, Any]) -> list[str]: for node in status_contexts: state = (node.get("state") or "").upper() if state in {"FAILURE", "ERROR"}: + if ignore_opencode and is_opencode_context(node): + continue failed.append(node.get("context") or "status-context") return failed @@ -2112,6 +2267,46 @@ def active_opencode_run_refs( ) +def latest_opencode_dispatch_started_at( + repo: str, + workflow: str, + pr: dict[str, Any], +) -> datetime | None: + """Return the latest completed same-head OpenCode dispatch start time.""" + target_repo = validate_github_repository(repo) + dispatch_repo = repository_dispatch_target(target_repo) + head = str(pr.get("headRefOid") or "").lower() + number = int(pr["number"]) + title_prefixes = tuple( + f"{title} {target_repo}#{number}@" + for title in sorted( + {"Required OpenCode Review", *OPENCODE_WORKFLOW_NAMES}, + key=len, + reverse=True, + ) + ) + latest: datetime | None = None + for run_data in active_workflow_runs(dispatch_repo, ("completed",)): + if run_data.get("event") != "repository_dispatch": + continue + display_title = str(run_data.get("display_title") or "") + prefix = next( + (candidate for candidate in title_prefixes if display_title.startswith(candidate)), + None, + ) + if prefix is None: + continue + dispatched_head = display_title.removeprefix(prefix).lower() + if not GIT_SHA_RE.fullmatch(dispatched_head) or dispatched_head != head: + continue + started_at = parse_github_datetime( + run_data.get("run_started_at") or run_data.get("created_at") + ) + if started_at and (latest is None or started_at > latest): + latest = started_at + return latest + + def active_opencode_run_ids( repo: str, workflow: str, @@ -2535,16 +2730,102 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return request_branch_update( "current-head OpenCode review requested changes; branch is outdated before re-review" ) + coverage_retry_progress = opencode_progress_state( + pr, stale_after_minutes=stale_opencode_minutes + ) + coverage_ready = ( + merge_state not in {"DIRTY", "CONFLICTING"} + and trigger_reviews + and review_dispatch_allowed + and current_head_coverage_change_request(pr) + and coverage_evidence_state(pr) == "complete" + and strix_evidence_state(pr) == "complete" + and not failed_status_checks(pr, ignore_opencode=True) + ) + if coverage_ready: + if coverage_retry_progress == "running": + if pr.get("autoMergeRequest"): + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + "current-head OpenCode coverage evidence is complete; disable " + "auto-merge while same-head re-review is already running" + ), + ) + ) + return decide( + "wait", + "current-head OpenCode coverage evidence is complete; " + "same-head OpenCode re-review is already running", + ) + retry_wait_reason = coverage_retry_wait_reason( + pr, + repo=repo if not dry_run else None, + workflow=workflow if not dry_run else None, + ) + if retry_wait_reason: + if pr.get("autoMergeRequest"): + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + f"{retry_wait_reason}; disable auto-merge until the same-head " + "coverage retry floor elapses" + ), + ) + ) + return decide("wait", retry_wait_reason) + if pr.get("autoMergeRequest"): + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + "current-head OpenCode coverage blocker is cleared; disable auto-merge " + "before same-head re-review" + ), + ) + ) + wait_reason = repository_dispatch_wait_reason(repo, workflow) + if wait_reason: + return decide("wait", wait_reason) + dispatch_result = dispatch_opencode_review(repo, workflow, pr, dry_run=dry_run) + if dispatch_result == "already_running": + return decide( + "wait", + "current-head coverage evidence is complete, but a same-head OpenCode workflow run is already active", + ) + return decide( + "review_dispatch", + "current-head OpenCode coverage blocker is cleared; same-head OpenCode re-dispatched", + ) + conflict_suffix = ( + f"; {merge_conflict_guidance(pr, merge_state)}" + if merge_state in {"DIRTY", "CONFLICTING"} + else "" + ) if pr.get("autoMergeRequest"): return finish( disable_auto_merge_decision( repo, pr, dry_run=dry_run, - reason="current-head OpenCode review requested changes; address the review before re-enabling auto-merge", + reason=( + "current-head OpenCode review requested changes; address the review " + f"before re-enabling auto-merge{conflict_suffix}" + ), ) ) - return decide("block", "current-head OpenCode review requested changes") + return decide( + "block", + f"current-head OpenCode review requested changes{conflict_suffix}", + ) current_head_approved = has_current_head_approval(pr) approval_reason = merge_approval_block_reason(pr) if current_head_approved else None diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index bdced4e564..5ee152c9fc 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -933,6 +933,7 @@ def test_context_review_and_check_helpers(monkeypatch): assert sched.context_nodes(make_pr()) == [] assert sched.compare_behind_by({"compareBehindBy": "2"}) == 2 assert sched.compare_behind_by({"compareBehindBy": "unknown"}) == 0 + assert not sched.is_opencode_check_run({"context": "opencode-review"}) assert sched.is_opencode_context({"__typename": "CheckRun", "name": "opencode-review"}) assert sched.is_opencode_context( { @@ -1107,6 +1108,549 @@ def test_central_progress_ignores_required_workflow_checkrun_placeholder( ) +def test_central_coverage_retry_ignores_failed_required_workflow_placeholder( + monkeypatch, +): + """A non-authoritative OpenCode CheckRun cannot self-block its retry.""" + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: "dispatched", + ) + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + { + **opencode_check(status="COMPLETED"), + "conclusion": "FAILURE", + }, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "review_dispatch" + assert decision.reason == ( + "current-head OpenCode coverage blocker is cleared; " + "same-head OpenCode re-dispatched" + ) + + +def test_coverage_retry_disables_auto_merge_before_dispatch(monkeypatch): + """A coverage retry must not leave an unsafe auto-merge request enabled.""" + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + disabled = [] + dispatched = [] + monkeypatch.setattr( + sched, + "disable_auto_merge", + lambda repo, pr, dry_run: disabled.append((repo, pr["number"], dry_run)), + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatched.append((args, kwargs)) or "dispatched", + ) + coverage_request = make_pr( + autoMergeRequest={"enabledAt": "now"}, + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + { + **opencode_check(status="COMPLETED"), + "conclusion": "FAILURE", + }, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "disable_auto_merge" + assert "before same-head re-review" in decision.reason + assert disabled == [("owner/repo", 1, True)] + assert dispatched == [] + + +def test_coverage_retry_waits_for_visible_opencode_run(monkeypatch): + """A visible same-head OpenCode run prevents duplicate coverage dispatch.""" + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatched.append((args, kwargs)) or "dispatched", + ) + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + opencode_check( + status="IN_PROGRESS", + started_at=datetime.now(timezone.utc).isoformat(), + ), + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "wait" + assert decision.reason == ( + "current-head OpenCode coverage evidence is complete; " + "same-head OpenCode re-review is already running" + ) + assert dispatched == [] + + +def test_coverage_retry_disables_auto_merge_for_visible_opencode_run(monkeypatch): + """An active same-head re-review disables any pre-existing auto-merge request.""" + disabled = [] + monkeypatch.setattr( + sched, + "disable_auto_merge", + lambda repo, pr, dry_run: disabled.append((repo, pr["number"], dry_run)), + ) + coverage_request = make_pr( + autoMergeRequest={"enabledAt": "now"}, + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "coverage evidence did not pass; coverage-evidence reported that " + "required test/docstring evidence was not proven" + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + opencode_check( + status="IN_PROGRESS", + started_at=datetime.now(timezone.utc).isoformat(), + ), + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "disable_auto_merge" + assert disabled == [("owner/repo", 1, True)] + + +def test_coverage_retry_waits_for_same_head_retry_floor(monkeypatch): + """A fresh coverage-only review disables auto-merge during its retry floor.""" + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + disabled = [] + monkeypatch.setattr( + sched, + "disable_auto_merge", + lambda repo, pr, dry_run: disabled.append((repo, pr["number"], dry_run)), + ) + dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatched.append((args, kwargs)) or "dispatched", + ) + coverage_request = make_pr( + autoMergeRequest={"enabledAt": "now"}, + reviews={ + "nodes": [ + { + **opencode_review( + "CHANGES_REQUESTED", + "head", + submitted_at="2999-01-01T00:00:00Z", + ), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + {**opencode_check(status="COMPLETED"), "conclusion": "FAILURE"}, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "disable_auto_merge" + assert "same-head OpenCode coverage retry floor has not elapsed" in decision.reason + assert "disable auto-merge until the same-head coverage retry floor elapses" in decision.reason + assert disabled == [("owner/repo", 1, True)] + assert dispatched == [] + + +def test_coverage_retry_floor_uses_latest_dispatch_timestamp(monkeypatch): + """A completed dispatch without a review still receives a bounded retry floor.""" + coverage_review = { + **opencode_review( + "CHANGES_REQUESTED", + "head", + submitted_at="2026-08-24T00:00:00Z", + ), + "body": ( + "OpenCode cannot approve yet because required coverage evidence did not pass. " + "The coverage-evidence gate reported that required test/docstring evidence was not proven." + ), + } + coverage_request = make_pr(reviews={"nodes": [coverage_review]}) + monkeypatch.setattr( + sched, + "latest_opencode_dispatch_started_at", + lambda repo, workflow, pr: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), + ) + + assert sched.coverage_retry_wait_reason( + coverage_request, + repo="owner/repo", + workflow="OpenCode Review", + now=datetime(2026, 8, 24, 1, 59, tzinfo=timezone.utc), + ) == "same-head OpenCode coverage retry floor has not elapsed" + assert sched.coverage_retry_wait_reason( + coverage_request, + repo="owner/repo", + workflow="OpenCode Review", + now=datetime(2026, 8, 24, 2, 0, tzinfo=timezone.utc), + ) is None + + +def test_coverage_retry_wait_reason_fails_closed_without_coverage_review(): + """A non-coverage change request cannot authorize a retry.""" + assert sched.coverage_retry_wait_reason(make_pr()) is None + + +def test_coverage_retry_wait_reason_fails_closed_when_dispatch_history_is_unavailable( + monkeypatch, +): + """Unavailable dispatch history prevents an unbounded same-head retry.""" + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review( + "CHANGES_REQUESTED", + "head", + submitted_at="2026-08-24T00:00:00Z", + ), + "body": ( + "coverage evidence did not pass; coverage-evidence reported that " + "required test/docstring evidence was not proven" + ), + } + ] + } + ) + monkeypatch.setattr( + sched, + "latest_opencode_dispatch_started_at", + lambda repo, workflow, pr: (_ for _ in ()).throw(RuntimeError("temporary API failure")), + ) + + assert sched.coverage_retry_wait_reason( + coverage_request, + repo="owner/repo", + workflow="OpenCode Review", + ) == "same-head OpenCode dispatch history is unavailable; defer same-head re-review" + + +def test_coverage_retry_floor_keeps_newer_review_timestamp(monkeypatch): + """An older dispatch cannot extend or replace the newer review timestamp.""" + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review( + "CHANGES_REQUESTED", + "head", + submitted_at="2026-08-24T02:00:00Z", + ), + "body": ( + "coverage evidence did not pass; coverage-evidence reported that " + "required test/docstring evidence was not proven" + ), + } + ] + } + ) + monkeypatch.setattr( + sched, + "latest_opencode_dispatch_started_at", + lambda repo, workflow, pr: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), + ) + + assert sched.coverage_retry_wait_reason( + coverage_request, + repo="owner/repo", + workflow="OpenCode Review", + now=datetime(2026, 8, 24, 2, 30, tzinfo=timezone.utc), + ) == "same-head OpenCode coverage retry floor has not elapsed" + + +def test_coverage_retry_without_timestamp_fails_closed(monkeypatch): + """A coverage-only review without a timestamp cannot authorize redispatch.""" + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + coverage_review = opencode_review("CHANGES_REQUESTED", "head") + coverage_review.pop("submittedAt") + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **coverage_review, + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + {**opencode_check(status="COMPLETED"), "conclusion": "FAILURE"}, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "wait" + assert "no valid submission timestamp" in decision.reason + + +def test_coverage_retry_keeps_failed_opencode_workflow_siblings_fail_closed( + monkeypatch, +): + """Only the superseded review job is ignored during coverage retry.""" + monkeypatch.setenv( + "SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", + "ContextualWisdomLab/.github", + ) + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatched.append((args, kwargs)) or "dispatched", + ) + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + { + **opencode_check(status="COMPLETED"), + "conclusion": "FAILURE", + }, + { + "__typename": "CheckRun", + "name": "coverage-source-tree", + "status": "COMPLETED", + "conclusion": "FAILURE", + "checkSuite": { + "workflowRun": { + "workflow": {"name": "Required OpenCode Review"} + } + }, + }, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert sched.failed_status_checks(coverage_request, ignore_opencode=True) == [ + "coverage-source-tree" + ] + assert decision.action == "block" + assert decision.reason == "current-head OpenCode review requested changes" + assert dispatched == [] + + +def test_conflicting_coverage_retry_blocks_with_conflict_guidance(monkeypatch): + """Coverage-only retries never dispatch while the exact head conflicts.""" + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda *_: None) + dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda *args, **kwargs: dispatched.append((args, kwargs)) or "dispatched", + ) + coverage_request = make_pr( + mergeStateStatus="CONFLICTING", + restMergeableState="CONFLICTING", + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence " + "did not pass. The coverage-evidence gate reported that required " + "test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + { + "__typename": "CheckRun", + "name": "opencode-review", + "status": "COMPLETED", + "conclusion": "FAILURE", + }, + ] + } + }, + ) + + decision = inspect(coverage_request) + + assert decision.action == "block" + assert "merge conflict: CONFLICTING" in decision.reason + assert dispatched == [] + + def test_review_state_and_failed_checks(): pr = make_pr(reviews={"nodes": [opencode_review("APPROVED", "old"), opencode_review("APPROVED", "head")]}) assert sched.current_head_review_state(pr, "APPROVED") @@ -1263,6 +1807,102 @@ def test_review_state_and_failed_checks(): assert sched.has_current_head_approval(superseded) assert not sched.has_current_head_changes_requested(superseded) + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence did not pass. " + "The coverage-evidence gate reported that required test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + ] + } + }, + ) + assert sched.current_head_coverage_change_request(coverage_request) + assert sched.coverage_evidence_state(coverage_request) == "complete" + assert sched.coverage_evidence_state( + make_pr( + statusCheckRollup={ + "contexts": {"nodes": [{"name": "coverage-evidence", "state": "PENDING"}]} + } + ) + ) == "running" + assert sched.coverage_evidence_state( + make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "IN_PROGRESS", + } + ] + } + } + ) + ) == "running" + assert sched.coverage_evidence_state( + make_pr(statusCheckRollup={"contexts": {"nodes": [strix_check()]}}) + ) == "missing" + assert sched.coverage_evidence_state( + make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + {"name": "coverage-evidence", "state": "SUCCESS"}, + {"name": "lint", "state": "SUCCESS"}, + ] + } + } + ) + ) == "complete" + assert sched.coverage_evidence_state( + make_pr( + statusCheckRollup={ + "contexts": {"nodes": [{"name": "coverage-evidence", "state": "FAILURE"}]} + } + ) + ) == "failed" + assert sched.coverage_evidence_state(make_pr()) == "missing" + human_coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head", login="human"), + "body": "coverage evidence did not pass; coverage-evidence; required test/docstring evidence", + } + ] + } + ) + assert not sched.current_head_coverage_change_request(human_coverage_request) + assert not sched.current_head_coverage_change_request( + make_pr(reviews={"nodes": [opencode_review("APPROVED", "head")]}) + ) + ordinary_request = make_pr( + reviews={ + "nodes": [ + {**opencode_review("CHANGES_REQUESTED", "head"), "body": "Fix the estimator."} + ] + } + ) + assert not sched.current_head_coverage_change_request(ordinary_request) + stale_gate_reviews = make_pr( reviews={ "nodes": [ @@ -1344,6 +1984,22 @@ def test_review_state_and_failed_checks(): } ) assert sched.failed_status_checks(failed) == ["strix", "lint"] + assert sched.failed_status_checks( + make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "StatusContext", + "context": "opencode-review", + "state": "FAILURE", + } + ] + } + } + ), + ignore_opencode=True, + ) == [] action_required = make_pr( statusCheckRollup={ "contexts": { @@ -1758,6 +2414,139 @@ def test_failed_status_checks_prefers_timestamped_duplicate_check_runs(): assert sched.failed_status_checks(missing_then_timestamped) == [] +def test_coverage_evidence_state_prefers_newest_rerun(): + """An older failed rerun cannot hide newer successful coverage evidence.""" + + def coverage_check(started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": "OpenCode Review"}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check("2026-08-24T02:00:00Z", "SUCCESS"), + coverage_check("2026-08-24T01:00:00Z", "FAILURE"), + ] + } + } + ) + + assert sched.coverage_evidence_state(pr) == "complete" + + +def test_coverage_evidence_state_prefers_newest_run_across_workflows(): + """Coverage evidence uses time, not rollup order, across workflow names.""" + + def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check( + "OpenCode Review Dispatch", + "2026-08-24T02:00:00Z", + "SUCCESS", + ), + coverage_check( + "Required OpenCode Review", + "2026-08-24T01:00:00Z", + "FAILURE", + ), + ] + } + } + ) + + assert sched.coverage_evidence_state(pr) == "complete" + + +def test_coverage_retry_ignores_superseded_failure_across_workflows(): + """A newer successful workflow supersedes an older coverage failure.""" + + def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check( + "Required OpenCode Review", + "2026-08-24T01:00:00Z", + "FAILURE", + ), + coverage_check( + "OpenCode Review Dispatch", + "2026-08-24T02:00:00Z", + "SUCCESS", + ), + ] + } + } + ) + + assert sched.failed_status_checks(pr) == ["coverage-evidence"] + assert sched.failed_status_checks(pr, ignore_opencode=True) == [] + + +def test_coverage_retry_keeps_newest_failed_run_authoritative_across_workflows(): + """An unsuccessful newest run remains a blocking coverage failure.""" + + def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check( + "Required OpenCode Review", + "2026-08-24T01:00:00Z", + "SUCCESS", + ), + coverage_check( + "OpenCode Review Dispatch", + "2026-08-24T02:00:00Z", + "FAILURE", + ), + ] + } + } + ) + + assert sched.failed_status_checks(pr, ignore_opencode=True) == ["coverage-evidence"] + + def test_run_command_failure_scrubs_secrets(monkeypatch): import subprocess @@ -2436,6 +3225,74 @@ def fake_run(args, stdin=None): assert not any(call[:3] == ["gh", "workflow", "run"] for call in calls) +def test_latest_opencode_dispatch_started_at_matches_exact_completed_run(monkeypatch): + """Completed same-head repository dispatch runs provide a retry timestamp.""" + head_sha = "a" * 40 + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda repo, statuses: [ + {"event": "push", "display_title": "irrelevant"}, + { + "event": "repository_dispatch", + "display_title": "Different workflow owner/repo#1@" + head_sha, + "created_at": "2026-08-24T00:30:00Z", + }, + { + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "run_started_at": "2026-08-24T01:00:00Z", + }, + { + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "created_at": "2026-08-24T02:00:00Z", + }, + { + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + "created_at": "2026-08-24T01:30:00Z", + }, + { + "event": "repository_dispatch", + "display_title": f"Required OpenCode Review owner/repo#1@{head_sha}", + }, + { + "event": "repository_dispatch", + "display_title": "Required OpenCode Review owner/repo#1@not-a-sha", + "created_at": "2026-08-24T03:00:00Z", + }, + ], + ) + + assert sched.latest_opencode_dispatch_started_at( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid=head_sha), + ) == datetime(2026, 8, 24, 2, 0, tzinfo=timezone.utc) + + +def test_latest_opencode_dispatch_started_at_returns_none_without_exact_run(monkeypatch): + """Unrelated completed runs cannot become a same-head retry marker.""" + monkeypatch.setattr( + sched, + "active_workflow_runs", + lambda repo, statuses: [ + { + "event": "repository_dispatch", + "display_title": "Required OpenCode Review owner/repo#1@" + "b" * 40, + "created_at": "2026-08-24T02:00:00Z", + } + ], + ) + + assert sched.latest_opencode_dispatch_started_at( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid="a" * 40), + ) is None + + def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, capsys): calls = [] head_sha = "a" * 40 @@ -3274,10 +4131,101 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): ) ) assert conflict_with_stale_review.action == "block" - assert conflict_with_stale_review.reason == ( - "current-head OpenCode review requested changes" + assert conflict_with_stale_review.reason.startswith( + "current-head OpenCode review requested changes; merge conflict:" ) + assert merge_state in conflict_with_stale_review.reason assert update_calls == [] + coverage_request = make_pr( + reviews={ + "nodes": [ + { + **opencode_review("CHANGES_REQUESTED", "head"), + "body": ( + "OpenCode cannot approve yet because required coverage evidence did not pass. " + "The coverage-evidence gate reported that required test/docstring evidence was not proven." + ), + } + ] + }, + statusCheckRollup={ + "contexts": { + "nodes": [ + strix_check(), + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + }, + { + "__typename": "CheckRun", + "name": "opencode-review", + "status": "COMPLETED", + "conclusion": "FAILURE", + "checkSuite": { + "workflowRun": {"workflow": {"name": "OpenCode Review"}} + }, + }, + ] + } + }, + ) + dispatched = [] + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: dispatched.append( + (repo, workflow, pr["headRefOid"], dry_run) + ) + or "dispatched", + ) + monkeypatch.setattr( + sched, + "repository_dispatch_wait_reason", + lambda repo, workflow: "review dispatch waits", + ) + coverage_wait = inspect(coverage_request) + assert coverage_wait.action == "wait" + assert coverage_wait.reason == "review dispatch waits" + monkeypatch.setattr(sched, "repository_dispatch_wait_reason", lambda repo, workflow: None) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: "already_running", + ) + coverage_active = inspect(coverage_request) + assert coverage_active.action == "wait" + assert coverage_active.reason == ( + "current-head coverage evidence is complete, but a same-head OpenCode workflow run is already active" + ) + monkeypatch.setattr( + sched, + "dispatch_opencode_review", + lambda repo, workflow, pr, dry_run: dispatched.append( + (repo, workflow, pr["headRefOid"], dry_run) + ) + or "dispatched", + ) + coverage_decision = inspect(coverage_request) + assert coverage_decision.action == "review_dispatch" + assert coverage_decision.reason == ( + "current-head OpenCode coverage blocker is cleared; same-head OpenCode re-dispatched" + ) + assert dispatched == [("owner/repo", "OpenCode Review", "head", True)] + + coverage_request["statusCheckRollup"]["contexts"]["nodes"].append( + { + "__typename": "CheckRun", + "name": "Security Scan", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ) + unrelated_failure = inspect(coverage_request) + assert unrelated_failure.action == "block" + assert unrelated_failure.reason == "current-head OpenCode review requested changes" + action_required_pr = make_pr( statusCheckRollup={ "contexts": { From 91bd43c427cf8fd6a06b75e264254907cf263305 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 16:52:31 +0900 Subject: [PATCH 04/16] fix(scheduler): exclude automated bot approvals --- scripts/ci/pr_review_merge_scheduler.py | 3 ++- tests/test_pr_review_merge_scheduler.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index ccf73c00d5..ba288816d8 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1336,7 +1336,8 @@ def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: not reviewer or reviewer == author or is_automated_opencode_review(review) - or reviewer in {"github-actions", "github-actions[bot]"} + or reviewer == "github-actions" + or reviewer.endswith("[bot]") or not review_matches_current_head(review, pr) or state not in {"APPROVED", "CHANGES_REQUESTED", "DISMISSED"} or reviewer in seen_reviewers diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 5ee152c9fc..95f4350496 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2064,6 +2064,7 @@ def test_scheduler_query_requests_pull_request_author(): ("pull-request-author", "pull-request-author", "APPROVED", "head"), ("pull-request-author", "opencode-agent", "APPROVED", "head"), ("pull-request-author", "github-actions[bot]", "APPROVED", "head"), + ("pull-request-author", "noema-review[bot]", "APPROVED", "head"), ("pull-request-author", "independent-reviewer", "COMMENTED", "head"), ("pull-request-author", "independent-reviewer", "APPROVED", "old"), ), From 833975e4a51555b71d876c4e4ac0ee185e4c78c6 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 16:57:49 +0900 Subject: [PATCH 05/16] fix(scheduler): ignore superseded coverage failures --- scripts/ci/pr_review_merge_scheduler.py | 9 ++++----- tests/test_pr_review_merge_scheduler.py | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index ba288816d8..9df8f0fceb 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1103,6 +1103,7 @@ def parse_github_datetime(value: str | None) -> datetime | None: def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: """Return the newest check run for each workflow and check-name pair.""" + epoch = datetime.min.replace(tzinfo=timezone.utc) latest: dict[ tuple[str, str], tuple[datetime | None, int, dict[str, Any]], @@ -1126,8 +1127,8 @@ def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: if previous_started_at is None and started_at is not None: latest[key] = (started_at, index, node) continue - if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= ( - previous_started_at or datetime.min.replace(tzinfo=timezone.utc), + if (started_at or epoch, index) >= ( + previous_started_at or epoch, previous_index, ): latest[key] = (started_at, index, node) @@ -1677,9 +1678,7 @@ def failed_status_checks( """ failed: list[str] = [] check_runs = latest_check_runs(pr) - superseded_coverage_indices = ( - superseded_coverage_evidence_indices(check_runs) if ignore_opencode else set() - ) + superseded_coverage_indices = superseded_coverage_evidence_indices(check_runs) status_contexts = [ node for node in context_nodes(pr) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 95f4350496..b1fbc8157c 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2509,7 +2509,7 @@ def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: } ) - assert sched.failed_status_checks(pr) == ["coverage-evidence"] + assert sched.failed_status_checks(pr) == [] assert sched.failed_status_checks(pr, ignore_opencode=True) == [] From 111e5b6b056ad95bec8559e056c1a1bbd195233c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 17:22:13 +0900 Subject: [PATCH 06/16] fix(scheduler): protect central coverage authority --- CHANGELOG.md | 3 ++ scripts/ci/pr_review_merge_scheduler.py | 12 ++++++++ tests/test_pr_review_merge_scheduler.py | 37 +++++++++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 681de5d352..bcb6befeb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,9 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Keep the central required-workflow coverage placeholder from superseding a + failed repository-dispatch coverage run; coverage retry and merge decisions + now use authoritative execution evidence for the central scheduler. - Re-dispatch an exact-head OpenCode review after its coverage-only blocker is cleared, selecting the newest coverage rerun by timestamp across workflow names and ignoring only the superseded `opencode-review` failure and central diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 9df8f0fceb..6ab15e6f19 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1424,12 +1424,24 @@ def coverage_retry_wait_reason( return None +def is_non_authoritative_coverage_check_run(node: dict[str, Any]) -> bool: + """Return whether central metadata-only coverage evidence is non-authoritative.""" + if not (os.environ.get("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY") or "").strip(): + return False + workflow = ( + ((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") + or {} + ) + return workflow.get("name") == "Required OpenCode Review" + + def coverage_evidence_indices(check_runs: Sequence[dict[str, Any]]) -> list[int]: """Return indexes of coverage-evidence checks in one check-run snapshot.""" return [ index for index, node in enumerate(check_runs) if (node.get("name") or "").lower() == "coverage-evidence" + and not is_non_authoritative_coverage_check_run(node) ] diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index b1fbc8157c..6fc78c93a2 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2477,6 +2477,43 @@ def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: assert sched.coverage_evidence_state(pr) == "complete" +def test_central_coverage_placeholder_cannot_mask_dispatch_failure(monkeypatch): + """Central metadata-only coverage success cannot hide failed dispatch evidence.""" + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") + + def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check( + "Required OpenCode Review", + "2026-08-24T03:00:00Z", + "SUCCESS", + ), + coverage_check( + "OpenCode Review Dispatch", + "2026-08-24T02:00:00Z", + "FAILURE", + ), + ] + } + } + ) + + assert sched.coverage_evidence_state(pr) == "failed" + assert sched.failed_status_checks(pr) == ["coverage-evidence"] + + def test_coverage_retry_ignores_superseded_failure_across_workflows(): """A newer successful workflow supersedes an older coverage failure.""" From ad01b4e69eae8a149560bc39e60bb693ab9028eb Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 17:36:51 +0900 Subject: [PATCH 07/16] fix(scheduler): ignore central coverage placeholder failures --- scripts/ci/pr_review_merge_scheduler.py | 4 +++ tests/test_pr_review_merge_scheduler.py | 37 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 6ab15e6f19..3688135646 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1428,6 +1428,8 @@ def is_non_authoritative_coverage_check_run(node: dict[str, Any]) -> bool: """Return whether central metadata-only coverage evidence is non-authoritative.""" if not (os.environ.get("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY") or "").strip(): return False + if (node.get("name") or "").lower() != "coverage-evidence": + return False workflow = ( ((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {} @@ -1703,6 +1705,8 @@ def failed_status_checks( if (node.get("state") or "").upper() == "SUCCESS" } for index, node in enumerate(check_runs): + if is_non_authoritative_coverage_check_run(node): + continue conclusion = (node.get("conclusion") or "").upper() if conclusion in FAILED_CHECK_CONCLUSIONS: if index in superseded_coverage_indices: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 6fc78c93a2..29506e392f 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2514,6 +2514,43 @@ def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: assert sched.failed_status_checks(pr) == ["coverage-evidence"] +def test_central_coverage_placeholder_failure_cannot_block_dispatch_success(monkeypatch): + """Central metadata-only coverage failure cannot block successful dispatch evidence.""" + monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") + + def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + coverage_check( + "Required OpenCode Review", + "2026-08-24T03:00:00Z", + "FAILURE", + ), + coverage_check( + "OpenCode Review Dispatch", + "2026-08-24T02:00:00Z", + "SUCCESS", + ), + ] + } + } + ) + + assert sched.coverage_evidence_state(pr) == "complete" + assert sched.failed_status_checks(pr) == [] + + def test_coverage_retry_ignores_superseded_failure_across_workflows(): """A newer successful workflow supersedes an older coverage failure.""" From 2139aec14503d154ebfec896a6bebc73f704e6c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 16:30:05 +0000 Subject: [PATCH 08/16] fix(scheduler): paginate PR reviews past the 100-review window reviews(last: 100) in the shared GraphQL fragment and the REST fallback's single per_page=100 page both silently truncate a PR's review history once it accumulates more than 100 review events. An early independent APPROVED review can fall out of that window, permanently blinding has_independent_current_head_approval (and its sibling exact-head-review consumers) even though GitHub still has the review. Call-graph investigation showed the bulk queue-scan path (fetch_open_prs / fetch_open_prs_rest) feeds merge decisions directly -- main() calls inspect_pr on those PR nodes without ever re-fetching a single PR when the scheduler runs its push-triggered or org-queue-sweep sweeps (no --pr-number) -- so both the bulk and single-PR fetch paths needed the fix, not just fetch_pr/fetch_pr_rest. - GraphQL: add pageInfo to the shared reviews connection and a new PR_REVIEWS_PAGE_QUERY that walks backward (last/before) past the initial window; complete_all_pr_reviews backfills any PR whose first page reports hasPreviousPage, called from both fetch_open_prs and fetch_pr. - REST: rest_pr_node (shared by fetch_open_prs_rest and fetch_pr_rest) now calls fetch_all_pr_reviews_rest, which loops page=1,2,... until a short page ends the history. - Both paginators propagate any page-fetch failure (fail closed) and bound their loop (MAX_REVIEW_PAGINATION_PAGES for GraphQL; REST terminates on a short/empty page) against a pathological pageInfo loop. Adds regression tests for both fetch paths' pagination, ordering, and fail-closed behavior on a page-fetch failure, plus an end-to-end test reproducing the reported bug (a genuine independent APPROVED review at position 1 of 106 reviews, invisible before the fix and visible after). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 125 ++++++- tests/test_pr_review_merge_scheduler.py | 430 +++++++++++++++++++++++- 2 files changed, 552 insertions(+), 3 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 5ad3aacbca..c6832e75ff 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -54,6 +54,7 @@ nodes { path } } reviews(last: 100) { + pageInfo { hasPreviousPage startCursor } nodes { databaseId state @@ -112,7 +113,37 @@ } """ + PULL_REQUEST_FIELDS_FRAGMENT +# Follow-up query for one pull request's reviews, walking backward past the +# ``reviews(last: 100)`` window in SchedulerPullRequestFields. GraphQL +# connections keep chronological (oldest-first) node order regardless of +# pagination direction, so ``last: 100, before: $cursor`` returns the up-to-100 +# reviews immediately preceding the cursor, still oldest-first. +PR_REVIEWS_PAGE_QUERY = """\ +query($owner: String!, $name: String!, $number: Int!, $cursor: String!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviews(last: 100, before: $cursor) { + pageInfo { hasPreviousPage startCursor } + nodes { + databaseId + state + body + submittedAt + author { login } + commit { oid } + } + } + } + } +} +""" + OPEN_PRS_PAGE_SIZE = 25 +# Defends against a pathological GraphQL pageInfo loop when backfilling a PR's +# full review history; 500 pages * 100 reviews/page is far beyond any +# realistic PR review count, so hitting it indicates a bug upstream rather +# than a PR that legitimately needs more pagination. +MAX_REVIEW_PAGINATION_PAGES = 500 # Must exceed the 45-minute OpenCode job cap plus typical runner-queue wait. # QUEUED counts as running and the age clock starts at check creation, so this # remains deliberately larger than the job cap while recovering genuine zombie @@ -760,6 +791,69 @@ def gh_graphql(query: str, **fields: str | int) -> dict[str, Any]: time.sleep(delay) +def complete_paginated_pr_reviews( + owner: str, name: str, number: int, reviews: dict[str, Any] +) -> dict[str, Any]: + """Backfill one pull request's review history past the GraphQL 100-node window. + + ``reviews(last: 100)`` in SchedulerPullRequestFields only returns the + newest 100 reviews on a pull request. Once a PR accumulates more than 100 + review events (bot reviewers post multiple reviews per push in this org), + ``pageInfo.hasPreviousPage`` comes back true and earlier reviews -- + including a genuine independent APPROVED review made early in the PR's + life -- are silently missing from ``nodes``. This walks backward with + ``before`` cursors via PR_REVIEWS_PAGE_QUERY, merging each page in front of + the ones already collected so the result stays oldest-first (the order + every ``reversed(...)`` consumer in this module expects), until GitHub + reports no earlier page. A page-fetch failure propagates (fail closed) + rather than returning a partial history. + """ + page_info = reviews.get("pageInfo") or {} + nodes = list(reviews.get("nodes") or []) + pages_fetched = 0 + while page_info.get("hasPreviousPage"): + pages_fetched += 1 + if pages_fetched > MAX_REVIEW_PAGINATION_PAGES: + raise RuntimeError( + f"Pull request {owner}/{name}#{number} review pagination exceeded " + f"{MAX_REVIEW_PAGINATION_PAGES} pages without exhausting hasPreviousPage; " + "refusing to loop indefinitely." + ) + cursor = page_info.get("startCursor") + if not cursor: + raise RuntimeError( + f"Pull request {owner}/{name}#{number} reported hasPreviousPage=true " + "without a startCursor; cannot continue review pagination." + ) + payload = gh_graphql( + PR_REVIEWS_PAGE_QUERY, owner=owner, name=name, number=number, cursor=cursor + ) + pull_request = ((payload.get("data") or {}).get("repository") or {}).get( + "pullRequest" + ) or {} + page = pull_request.get("reviews") or {} + nodes = list(page.get("nodes") or []) + nodes + page_info = page.get("pageInfo") or {} + return {"nodes": nodes} + + +def complete_all_pr_reviews(owner: str, name: str, prs: list[dict[str, Any]]) -> None: + """Backfill full review history in place for every fetched PR node that needs it. + + Only PRs whose initial ``reviews(last: 100)`` window reported + ``hasPreviousPage`` pay the extra round trip; PRs with 100 or fewer + reviews (the overwhelming majority) are untouched. + """ + for pr in prs: + reviews = pr.get("reviews") + if not reviews: + continue + if (reviews.get("pageInfo") or {}).get("hasPreviousPage"): + pr["reviews"] = complete_paginated_pr_reviews( + owner, name, pr.get("number"), reviews + ) + + def github_resource_inaccessible(exc: RuntimeError) -> bool: """Return whether GitHub denied an API read for the current integration token.""" @@ -786,6 +880,29 @@ def rest_review_node(review: dict[str, Any]) -> dict[str, Any]: } +def fetch_all_pr_reviews_rest(repo: str, number: int) -> list[dict[str, Any]]: + """Fetch every REST review for a pull request, paginating past 100. + + A single ``per_page=100`` page silently drops earlier reviews once a PR + accumulates more than 100 review events, the same truncation the GraphQL + ``reviews(last: 100)`` window hits. This walks ``page=1,2,3,...`` -- + mirroring ``fetch_open_prs_rest``'s pagination style -- until a page + shorter than 100 rows confirms the end of the history. A page-fetch + failure propagates (fail closed) rather than returning a partial history. + """ + reviews: list[dict[str, Any]] = [] + page = 1 + while True: + batch = gh_api_json(f"repos/{repo}/pulls/{number}/reviews?per_page=100&page={page}") + if not batch: + break + reviews.extend(batch) + if len(batch) < 100: + break + page += 1 + return reviews + + def rest_check_node(check: dict[str, Any]) -> dict[str, Any]: """Convert a REST check-run payload into the GraphQL status rollup shape.""" @@ -807,7 +924,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: head = pr.get("head") or {} base = pr.get("base") or {} head_repo = head.get("repo") or {} - reviews = gh_api_json(f"repos/{repo}/pulls/{number}/reviews?per_page=100") + reviews = fetch_all_pr_reviews_rest(repo, number) checks = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-runs?per_page=100") files = gh_api_json(f"repos/{repo}/pulls/{number}/files?per_page=20") rest_merge_state = REST_MERGEABLE_STATE_MAP.get( @@ -908,6 +1025,11 @@ def fetch_open_prs(repo: str, max_prs: int) -> list[dict[str, Any]]: break cursor = pr_page["pageInfo"]["endCursor"] + # Bulk-scan results feed merge decisions directly (the scheduler's push- + # triggered and org-queue-sweep runs never re-fetch a single PR before + # calling inspect_pr), so this path needs the same full review history as + # fetch_pr, not just the first/last 100-review window. + complete_all_pr_reviews(owner, name, prs) enrich_rest_mergeable_states(repo, prs) return prs @@ -923,6 +1045,7 @@ def fetch_pr(repo: str, number: int) -> list[dict[str, Any]]: raise pr = payload["data"]["repository"].get("pullRequest") prs = [pr] if pr else [] + complete_all_pr_reviews(owner, name, prs) enrich_rest_mergeable_states(repo, prs) return prs diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 216123b9df..855a10e53a 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -105,6 +105,18 @@ def merge_approved_reviews(commit="head"): } +def review_node(state="APPROVED", login="reviewer", commit="head", submitted_at="2026-06-25T07:00:00Z"): + """Return a minimal GraphQL-shaped review node for pagination tests.""" + return { + "databaseId": None, + "state": state, + "body": None, + "submittedAt": submitted_at, + "author": {"login": login}, + "commit": {"oid": commit}, + } + + def strix_check(status="COMPLETED", conclusion="SUCCESS", workflow="Strix Security Scan", details_url=None): value = { "__typename": "CheckRun", @@ -351,6 +363,420 @@ def fake_graphql(query, **fields): assert seen == [{"owner": "owner", "name": "repo", "number": 42}] +def test_complete_paginated_pr_reviews_merges_pages_oldest_first(monkeypatch): + """Backward pagination must prepend older pages so nodes stay oldest-first.""" + calls = [] + + def fake_graphql(query, **fields): + calls.append(fields) + assert fields == {"owner": "owner", "name": "repo", "number": 7, "cursor": "cursor-1"} + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": [review_node(login="independent-reviewer", submitted_at="t0")], + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + } + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + + newer_page = { + "nodes": [review_node(state="COMMENTED", login="bot[bot]", submitted_at="t1")], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + } + + merged = sched.complete_paginated_pr_reviews("owner", "repo", 7, newer_page) + + assert [node["author"]["login"] for node in merged["nodes"]] == [ + "independent-reviewer", + "bot[bot]", + ] + assert len(calls) == 1 + + +def test_complete_paginated_pr_reviews_walks_multiple_pages(monkeypatch): + """More than one prior page must all be folded in, oldest page first.""" + pages_by_cursor = { + "cursor-2": { + "nodes": [review_node(login="independent-reviewer", submitted_at="t0")], + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + }, + "cursor-1": { + "nodes": [review_node(state="COMMENTED", login="bot-a[bot]", submitted_at="t1")], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-2"}, + }, + } + + def fake_graphql(query, **fields): + return { + "data": { + "repository": {"pullRequest": {"reviews": pages_by_cursor[fields["cursor"]]}} + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + + newest_page = { + "nodes": [review_node(state="COMMENTED", login="bot-b[bot]", submitted_at="t2")], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + } + + merged = sched.complete_paginated_pr_reviews("owner", "repo", 7, newest_page) + + assert [node["author"]["login"] for node in merged["nodes"]] == [ + "independent-reviewer", + "bot-a[bot]", + "bot-b[bot]", + ] + + +def test_complete_paginated_pr_reviews_raises_without_start_cursor(): + """A truthful hasPreviousPage without a cursor cannot be paginated further.""" + reviews = {"nodes": [], "pageInfo": {"hasPreviousPage": True, "startCursor": None}} + + with pytest.raises(RuntimeError, match="startCursor"): + sched.complete_paginated_pr_reviews("owner", "repo", 7, reviews) + + +def test_complete_paginated_pr_reviews_propagates_page_fetch_failure(monkeypatch): + """A page-fetch failure must fail closed, never fall back to a partial history.""" + + def fail_graphql(*args, **kwargs): + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(sched, "gh_graphql", fail_graphql) + reviews = {"nodes": [], "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}} + + with pytest.raises(RuntimeError, match="HTTP 502"): + sched.complete_paginated_pr_reviews("owner", "repo", 7, reviews) + + +def test_complete_paginated_pr_reviews_bounds_pathological_loop(monkeypatch): + """A pageInfo that never resolves hasPreviousPage=false must not loop forever.""" + + def always_more(query, **fields): + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": [], + "pageInfo": { + "hasPreviousPage": True, + "startCursor": fields["cursor"] + "x", + }, + } + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", always_more) + monkeypatch.setattr(sched, "MAX_REVIEW_PAGINATION_PAGES", 3) + reviews = {"nodes": [], "pageInfo": {"hasPreviousPage": True, "startCursor": "c0"}} + + with pytest.raises(RuntimeError, match="exceeded 3 pages"): + sched.complete_paginated_pr_reviews("owner", "repo", 7, reviews) + + +def test_complete_all_pr_reviews_skips_prs_that_do_not_need_pagination(monkeypatch): + """PRs with no reviews key or a complete first page must not trigger a fetch.""" + calls = [] + monkeypatch.setattr(sched, "gh_graphql", lambda *args, **kwargs: calls.append((args, kwargs))) + + no_reviews_key = {"number": 1} + already_complete = { + "number": 2, + "reviews": {"nodes": [], "pageInfo": {"hasPreviousPage": False, "startCursor": None}}, + } + prs = [no_reviews_key, already_complete] + + sched.complete_all_pr_reviews("owner", "repo", prs) + + assert calls == [] + assert prs == [no_reviews_key, already_complete] + + +def test_complete_all_pr_reviews_backfills_only_truncated_prs(monkeypatch): + """Only the PR flagged hasPreviousPage gets its reviews replaced.""" + + def fake_graphql(query, **fields): + assert fields["number"] == 5 + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": [review_node(login="independent-reviewer")], + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + } + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + + complete_pr = { + "number": 4, + "reviews": {"nodes": [review_node(login="already-here")], "pageInfo": {"hasPreviousPage": False}}, + } + truncated_pr = { + "number": 5, + "reviews": { + "nodes": [review_node(state="COMMENTED", login="bot[bot]", submitted_at="t1")], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + }, + } + prs = [complete_pr, truncated_pr] + + sched.complete_all_pr_reviews("owner", "repo", prs) + + assert [n["author"]["login"] for n in complete_pr["reviews"]["nodes"]] == ["already-here"] + assert [n["author"]["login"] for n in truncated_pr["reviews"]["nodes"]] == [ + "independent-reviewer", + "bot[bot]", + ] + + +def test_fetch_pr_backfills_reviews_past_graphql_window(monkeypatch): + """fetch_pr (the single-PR path used right before a merge decision) must + paginate past the reviews(last: 100) window before returning.""" + + def fake_graphql(query, **fields): + if "before: $cursor" in query: + assert fields["cursor"] == "cursor-1" + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": [review_node(login="independent-reviewer", submitted_at="t0")], + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + } + } + } + } + } + return { + "data": { + "repository": { + "pullRequest": { + "number": fields["number"], + "reviews": { + "nodes": [review_node(state="COMMENTED", login="bot[bot]", submitted_at="t1")], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + }, + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + monkeypatch.setattr(sched, "enrich_rest_mergeable_states", lambda repo, prs: None) + + prs = sched.fetch_pr("owner/repo", 7) + + assert [n["author"]["login"] for n in prs[0]["reviews"]["nodes"]] == [ + "independent-reviewer", + "bot[bot]", + ] + + +def test_fetch_open_prs_backfills_reviews_past_graphql_window(monkeypatch): + """fetch_open_prs (the bulk queue-scan path) also feeds merge decisions + directly -- see inspect_pr's callers in main() -- so it must paginate past + the reviews(last: 100) window too, not just the single-PR fetch path.""" + + def fake_graphql(query, **fields): + if "before: $cursor" in query: + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": [review_node(login="independent-reviewer", submitted_at="t0")], + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + } + } + } + } + } + return { + "data": { + "repository": { + "pullRequests": { + "nodes": [ + { + "number": 7, + "reviews": { + "nodes": [ + review_node(state="COMMENTED", login="bot[bot]", submitted_at="t1") + ], + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + }, + } + ], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + monkeypatch.setattr(sched, "enrich_rest_mergeable_states", lambda repo, prs: None) + + prs = sched.fetch_open_prs("owner/repo", 5) + + assert [n["author"]["login"] for n in prs[0]["reviews"]["nodes"]] == [ + "independent-reviewer", + "bot[bot]", + ] + + +def test_fetch_all_pr_reviews_rest_paginates_past_100(monkeypatch): + """The REST fallback must walk page=1,2,... until a short page ends it.""" + page1 = [{"id": i} for i in range(100)] + page2 = [{"id": 100}, {"id": 101}] + calls = [] + + def fake_api(path): + calls.append(path) + if path.endswith("page=1"): + return page1 + if path.endswith("page=2"): + return page2 + raise AssertionError(f"unexpected path {path}") + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + reviews = sched.fetch_all_pr_reviews_rest("owner/repo", 7) + + assert [r["id"] for r in reviews] == list(range(102)) + assert calls == [ + "repos/owner/repo/pulls/7/reviews?per_page=100&page=1", + "repos/owner/repo/pulls/7/reviews?per_page=100&page=2", + ] + + +def test_fetch_all_pr_reviews_rest_stops_at_first_short_page(monkeypatch): + """A first page shorter than per_page must not trigger a second request.""" + calls = [] + + def fake_api(path): + calls.append(path) + return [{"id": 1}] + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + assert sched.fetch_all_pr_reviews_rest("owner/repo", 7) == [{"id": 1}] + assert calls == ["repos/owner/repo/pulls/7/reviews?per_page=100&page=1"] + + +def test_fetch_all_pr_reviews_rest_stops_on_empty_page_after_full_page(monkeypatch): + """A full 100-row page followed by an empty page must terminate cleanly.""" + calls = [] + + def fake_api(path): + calls.append(path) + if path.endswith("page=1"): + return [{"id": i} for i in range(100)] + return [] + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + reviews = sched.fetch_all_pr_reviews_rest("owner/repo", 7) + + assert len(reviews) == 100 + assert calls == [ + "repos/owner/repo/pulls/7/reviews?per_page=100&page=1", + "repos/owner/repo/pulls/7/reviews?per_page=100&page=2", + ] + + +def test_fetch_all_pr_reviews_rest_propagates_page_fetch_failure(monkeypatch): + """A later-page REST failure must fail closed rather than return a partial history.""" + + def fake_api(path): + if path.endswith("page=1"): + return [{"id": i} for i in range(100)] + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(sched, "gh_api_json", fake_api) + + with pytest.raises(RuntimeError, match="HTTP 502"): + sched.fetch_all_pr_reviews_rest("owner/repo", 7) + + +def test_fetch_pr_pagination_recovers_independent_approval_past_100_reviews(monkeypatch): + """End-to-end regression for the reported bug: a genuine independent + APPROVED review made early in a PR's life must still satisfy + has_independent_current_head_approval once the PR has accumulated more + than 100 total review events and fetch_pr completes GraphQL's pagination. + """ + genuine_approval = review_node( + state="APPROVED", login="independent-reviewer", submitted_at="t000", commit="head" + ) + noise = [ + review_node( + state="COMMENTED", + login=f"noise-bot-{i}[bot]", + submitted_at=f"t{i + 1:03d}", + commit="head", + ) + for i in range(105) + ] + full_history = [genuine_approval] + noise # oldest-first, 106 reviews total + first_page = full_history[-100:] # reviews(last: 100) -- drops the genuine approval + second_page = full_history[:6] # the remaining 6, including the genuine approval + assert genuine_approval not in first_page + assert genuine_approval in second_page + + def fake_graphql(query, **fields): + if "before: $cursor" in query: + assert fields["cursor"] == "cursor-1" + return { + "data": { + "repository": { + "pullRequest": { + "reviews": { + "nodes": second_page, + "pageInfo": {"hasPreviousPage": False, "startCursor": None}, + } + } + } + } + } + return { + "data": { + "repository": { + "pullRequest": { + "number": 7, + "author": {"login": "pull-request-author"}, + "headRefOid": "head", + "reviews": { + "nodes": first_page, + "pageInfo": {"hasPreviousPage": True, "startCursor": "cursor-1"}, + }, + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", fake_graphql) + monkeypatch.setattr(sched, "enrich_rest_mergeable_states", lambda repo, prs: None) + + pr = sched.fetch_pr("owner/repo", 7)[0] + + assert len(pr["reviews"]["nodes"]) == 106 + assert sched.has_independent_current_head_approval(pr) + + def test_gh_graphql_retries_transient_gateway_errors(monkeypatch): calls = [] sleeps = [] @@ -552,7 +978,7 @@ def raise_compare_error(repo, pr): def test_rest_pr_fallback_shapes_reviews_and_checks(monkeypatch): calls = [] payloads = { - "repos/owner/repo/pulls/42/reviews?per_page=100": [ + "repos/owner/repo/pulls/42/reviews?per_page=100&page=1": [ { "state": "APPROVED", "body": "Head SHA: `abc123`", @@ -602,7 +1028,7 @@ def fake_api(path): ) assert calls == [ - "repos/owner/repo/pulls/42/reviews?per_page=100", + "repos/owner/repo/pulls/42/reviews?per_page=100&page=1", "repos/owner/repo/commits/abc123/check-runs?per_page=100", "repos/owner/repo/pulls/42/files?per_page=20", ] From 731e427b51569fcfb9fc9875c731366d13f88411 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 17:00:02 +0000 Subject: [PATCH 09/16] fix(scheduler): disarm stale auto-merge and fix queued-check ordering Two bugs found by review on this PR's independent-approval gate: - latest_check_runs() treated a check run with a null startedAt as unconditionally older than a predecessor that already has a timestamp. GitHub leaves startedAt null while a check is QUEUED, so a freshly dispatched coverage-evidence rerun could lose to a stale, already-completed run of the same (workflow, name) key, letting coverage_evidence_state() report "complete" from the wrong run. Fall back to the check run's own pending status (the same predicate running_check_state() already uses) to break the tie only when a timestamp comparison is impossible, while a node with no timestamp and no pending status still defers to the timestamped predecessor. - inspect_pr() only evaluated merge_approval_block_reason() when current_head_approved was already True, so a PR with autoMergeRequest armed from before an unreviewed push, and an outdated branch, fell into the branch-update wait path with the auto-merge request left queued. Once the updated head's required checks passed, GitHub's own native auto-merge could complete the merge without this scheduler ever requiring a fresh independent approval on the new head. The behind-by branch now disarms auto-merge immediately whenever it is armed without a live current-head approval, instead of preserving it through the branch update. Regression tests added for both; existing tests that asserted the buggy "auto-merge already enabled ... remains queued" outcome for unapproved PRs were updated to the corrected fail-closed contract. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 53 +++++++-- tests/test_pr_review_merge_scheduler.py | 148 +++++++++++++++++++----- 2 files changed, 160 insertions(+), 41 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index c6832e75ff..bc5340471e 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1249,6 +1249,20 @@ def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: continue previous_started_at, previous_index, _ = previous if started_at is None and previous_started_at is not None: + # A rerun that has not started yet has no startedAt, so it can + # never win a chronological comparison against an + # already-started predecessor -- even though GitHub only ever + # creates it after that predecessor. Fall back to the check + # run's own actively-pending status (QUEUED/IN_PROGRESS/etc, the + # same predicate ``running_check_state`` uses) as the recency + # signal in that case: within one (workflow, name) key, a + # currently-pending run always supersedes a predecessor that + # already has a result, regardless of timestamps. A node with no + # startedAt and no pending status (e.g. cancelled before it + # started) carries no such signal and keeps deferring to the + # timestamped predecessor. + if running_check_state(node) == "running": + latest[key] = (started_at, index, node) continue if previous_started_at is None and started_at is not None: latest[key] = (started_at, index, node) @@ -3170,25 +3184,40 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio behind_by = branch_outdated_by_base(pr, merge_state) if behind_by and (current_head_approved or auto_merge_enabled): + if not current_head_approved: + # auto_merge_enabled must be True to have reached this branch (the + # outer condition requires current_head_approved or + # auto_merge_enabled). An outdated branch is routine and does not + # by itself justify disarming auto-merge -- but an auto-merge + # request armed with no live current-head approval is exactly the + # stale authorization this scheduler exists to catch, and simply + # requesting a branch update here would leave it queued: once the + # updated head's required checks pass, GitHub's own native + # auto-merge could merge it without this scheduler ever getting a + # chance to require a fresh independent approval on that new + # head. Disarm before requesting the update rather than after. + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + f"branch is {behind_by} commit(s) behind base (GitHub mergeability is " + f"{merge_state}) with no live current-head approval to authorize " + "auto-merge; obtain fresh independent approval before re-enabling auto-merge" + ), + ) + ) if not update_branches: - if current_head_approved: - return decide("wait", "current-head OpenCode review approved; branch update disabled") - return decide("wait", "auto-merge already enabled; branch update disabled") + return decide("wait", "current-head OpenCode review approved; branch update disabled") if not can_update_pr_head(repo, pr): return decide("wait", non_mutable_head_reason(repo, pr)) suffix = "; existing auto-merge request remains queued" if auto_merge_enabled else "" - if current_head_approved and merge_state == "BEHIND": + if merge_state == "BEHIND": freshness_reason = "current-head OpenCode review approved" - elif current_head_approved: - freshness_reason = ( - "current-head OpenCode review approved; " - f"base branch is {behind_by} commit(s) ahead even though GitHub mergeability is {merge_state}" - ) - elif merge_state == "BEHIND": - freshness_reason = "auto-merge already enabled" else: freshness_reason = ( - "auto-merge already enabled; " + "current-head OpenCode review approved; " f"base branch is {behind_by} commit(s) ahead even though GitHub mergeability is {merge_state}" ) return request_branch_update(freshness_reason, suffix=suffix) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 855a10e53a..b073067625 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2605,6 +2605,34 @@ def test_missing_independent_approval_blocks_and_disarms_auto_merge(): assert inspect(armed, merge_mode="direct").action == "disable_auto_merge" +def test_outdated_unapproved_branch_disarms_stale_auto_merge_instead_of_updating(): + """A stale auto-merge request must not survive an unapproved branch update. + + ``autoMergeRequest`` can still be armed from before a new, as-yet-unreviewed + push landed (GitHub does not always dismiss stale approvals on push). If the + branch is also behind base, silently requesting a branch update while + leaving that auto-merge request queued would let GitHub's own native + auto-merge complete the merge once the updated head's required checks pass + -- without this scheduler ever getting a chance to require a fresh + independent approval on the new head. The scheduler must disarm auto-merge + instead of preserving it through the branch-update wait path. + """ + outdated_unapproved_armed = make_pr( + mergeStateStatus="BEHIND", + autoMergeRequest={"enabledAt": "now"}, + reviews={"nodes": []}, + ) + + decision = inspect(outdated_unapproved_armed) + + assert decision.action == "disable_auto_merge" + assert "no live current-head approval" in decision.reason + assert "branch is 1 commit(s) behind base" in decision.reason + # The prior behavior's literal wording documented the bug: it kept the + # stale auto-merge request queued instead of disarming it. + assert "remains queued" not in decision.reason + + def test_workflow_run_followup_defers_deterministic_fallback_retry(monkeypatch): head = "a" * 40 fallback_review = { @@ -3010,6 +3038,48 @@ def coverage_check(started_at: str, conclusion: str) -> dict: assert sched.coverage_evidence_state(pr) == "complete" +def test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run(): + """A freshly queued rerun with no startedAt outranks an older completed run. + + GitHub's ``CheckRun.startedAt`` is legitimately null while a check is + still ``QUEUED``. A newly dispatched coverage-evidence rerun (appearing + later in the rollup than the run it replaces) must not lose to that + older, already-completed run just because it has not started yet -- + otherwise the scheduler could authorize a redispatch decision based on a + stale conclusion while the real, currently-relevant rerun is still + pending. + """ + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + "startedAt": "2026-08-24T01:00:00Z", + "checkSuite": {"workflowRun": {"workflow": {"name": "OpenCode Review"}}}, + }, + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "QUEUED", + "conclusion": None, + "startedAt": None, + "checkSuite": {"workflowRun": {"workflow": {"name": "OpenCode Review"}}}, + }, + ] + } + } + ) + + check_runs = sched.latest_check_runs(pr) + assert len(check_runs) == 1 + assert check_runs[0]["status"] == "QUEUED" + assert sched.coverage_evidence_state(pr) == "running" + + def test_coverage_evidence_state_prefers_newest_run_across_workflows(): """Coverage evidence uses time, not rollup order, across workflow names.""" @@ -5243,13 +5313,16 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): } }, ) + disabled.clear() blocked_without_opencode_decision = inspect(blocked_failed_behind_auto_without_opencode_approval) - assert blocked_without_opencode_decision.action == "update_branch" - assert "auto-merge already enabled" in blocked_without_opencode_decision.reason - assert "base branch is 2 commit(s) ahead" in blocked_without_opencode_decision.reason - assert "existing auto-merge request remains queued" in blocked_without_opencode_decision.reason - assert called == [("owner/repo", 1, True)] + assert blocked_without_opencode_decision.action == "disable_auto_merge" + assert "branch is 2 commit(s) behind base" in blocked_without_opencode_decision.reason + assert "GitHub mergeability is BLOCKED" in blocked_without_opencode_decision.reason + assert "no live current-head approval" in blocked_without_opencode_decision.reason + assert called == [] + assert disabled == [("owner/repo", 1, True)] called.clear() + disabled.clear() blocked_compare_behind_auto = make_pr( mergeStateStatus="BLOCKED", restMergeableState="BLOCKED", @@ -5265,12 +5338,14 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): }, ) blocked_compare_behind_decision = inspect(blocked_compare_behind_auto) - assert blocked_compare_behind_decision.action == "update_branch" - assert "auto-merge already enabled" in blocked_compare_behind_decision.reason - assert "base branch is 1 commit(s) ahead" in blocked_compare_behind_decision.reason - assert "existing auto-merge request remains queued" in blocked_compare_behind_decision.reason - assert called == [("owner/repo", 1, True)] + assert blocked_compare_behind_decision.action == "disable_auto_merge" + assert "branch is 1 commit(s) behind base" in blocked_compare_behind_decision.reason + assert "GitHub mergeability is BLOCKED" in blocked_compare_behind_decision.reason + assert "no live current-head approval" in blocked_compare_behind_decision.reason + assert called == [] + assert disabled == [("owner/repo", 1, True)] called.clear() + disabled.clear() diverged_failed_auto = make_pr( mergeStateStatus="BLOCKED", restMergeableState="BLOCKED", @@ -5288,12 +5363,14 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): }, ) diverged_failed_decision = inspect(diverged_failed_auto) - assert diverged_failed_decision.action == "update_branch" - assert "auto-merge already enabled" in diverged_failed_decision.reason - assert "base branch is 184 commit(s) ahead" in diverged_failed_decision.reason - assert "existing auto-merge request remains queued" in diverged_failed_decision.reason - assert called == [("owner/repo", 1, True)] + assert diverged_failed_decision.action == "disable_auto_merge" + assert "branch is 184 commit(s) behind base" in diverged_failed_decision.reason + assert "GitHub mergeability is BLOCKED" in diverged_failed_decision.reason + assert "no live current-head approval" in diverged_failed_decision.reason + assert called == [] + assert disabled == [("owner/repo", 1, True)] called.clear() + disabled.clear() unknown_compare_behind_auto = make_pr( mergeStateStatus="UNKNOWN", restMergeableState="UNKNOWN", @@ -5309,29 +5386,38 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): }, ) unknown_compare_behind_decision = inspect(unknown_compare_behind_auto) - assert unknown_compare_behind_decision.action == "update_branch" - assert "auto-merge already enabled" in unknown_compare_behind_decision.reason - assert "base branch is 1 commit(s) ahead" in unknown_compare_behind_decision.reason + assert unknown_compare_behind_decision.action == "disable_auto_merge" + assert "branch is 1 commit(s) behind base" in unknown_compare_behind_decision.reason assert "GitHub mergeability is UNKNOWN" in unknown_compare_behind_decision.reason - assert "existing auto-merge request remains queued" in unknown_compare_behind_decision.reason - assert called == [("owner/repo", 1, True)] + assert "no live current-head approval" in unknown_compare_behind_decision.reason + assert called == [] + assert disabled == [("owner/repo", 1, True)] called.clear() disabled.clear() - assert ( - inspect(blocked_failed_behind_auto_without_opencode_approval, update_branches=False).reason - == "auto-merge already enabled; branch update disabled" - ) + # An unapproved, outdated PR with auto-merge already armed is disarmed + # regardless of the update_branches flag -- disarming a stale + # authorization is a safety action, not a branch mutation, so it is not + # gated behind the same feature flag that controls branch updates. + update_branches_disabled_decision = inspect( + blocked_failed_behind_auto_without_opencode_approval, update_branches=False + ) + assert update_branches_disabled_decision.action == "disable_auto_merge" + assert "branch is 2 commit(s) behind base" in update_branches_disabled_decision.reason + assert "no live current-head approval" in update_branches_disabled_decision.reason assert called == [] - assert disabled == [] + assert disabled == [("owner/repo", 1, True)] + disabled.clear() behind_auto_without_opencode_approval = make_pr( mergeStateStatus="BEHIND", autoMergeRequest={"enabledAt": "now"}, ) behind_without_opencode_decision = inspect(behind_auto_without_opencode_approval) - assert behind_without_opencode_decision.action == "update_branch" - assert behind_without_opencode_decision.reason.startswith("auto-merge already enabled; branch update requested") - assert "existing auto-merge request remains queued" in behind_without_opencode_decision.reason - assert called == [("owner/repo", 1, True)] + assert behind_without_opencode_decision.action == "disable_auto_merge" + assert "branch is 1 commit(s) behind base" in behind_without_opencode_decision.reason + assert "GitHub mergeability is BEHIND" in behind_without_opencode_decision.reason + assert "no live current-head approval" in behind_without_opencode_decision.reason + assert called == [] + assert disabled == [("owner/repo", 1, True)] def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch): @@ -6296,6 +6382,8 @@ def test_main_limits_review_dispatches_and_branch_updates(monkeypatch, capsys): mergeStateStatus="BLOCKED", restMergeableState="BLOCKED", compareBehindBy=2, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ), make_pr( @@ -6303,6 +6391,8 @@ def test_main_limits_review_dispatches_and_branch_updates(monkeypatch, capsys): mergeStateStatus="BLOCKED", restMergeableState="BLOCKED", compareBehindBy=3, + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), autoMergeRequest={"enabledAt": "now"}, ), ] From 6370b320a70f99e415e326f33448b30b2bdb4a08 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 17:14:42 +0000 Subject: [PATCH 10/16] fix(scheduler): disarm stale auto-merge during the review-dispatch cascade inspect_pr() only checked pr.get("autoMergeRequest") in the merge_state == "UNKNOWN" branch and in a final catch-all reached once every wait/dispatch branch above it had nothing left to do. Once current_head_approved was False and the branch was not behind base (so neither behind_by disarm path applied), execution could fall through the OpenCode-running wait, the workflow_run deterministic-fallback wait, the stale-OpenCode retry dispatch, and the ordinary Strix/OpenCode dispatch cascade -- the everyday state for a PR between or during reviews -- and return a plain wait/dispatch decision without ever disarming a stale auto-merge request. If GitHub's own required checks do not themselves gate on this scheduler's OpenCode approval, GitHub's native auto-merge could complete the merge before the scheduler's next run ever reached the catch-all. Hoist a single unconditional check -- `not current_head_approved and auto_merge_enabled` -- right after the behind_by and last-push-approval- restamp gates (both of which only apply when current_head_approved is True, so this hoist cannot affect the already-approved path) and before any of those wait/dispatch branches. It disarms with the same reason text the old catch-all used, which is now dead code for the unapproved case and has been removed (kept for the merge_state == "UNKNOWN" branch's own, still-reachable approved+armed case). Added a regression test reproducing the gap (a CLEAN, unapproved, auto-merge- armed PR with no Strix evidence previously returned "security_dispatch" with auto-merge left queued; now returns "disable_auto_merge"). Updated the existing unapproved+UNKNOWN-mergeability test to the new, more accurate "no OpenCode approval" reason, and added a new approved+UNKNOWN+armed test to keep that branch's own check covered now that the unapproved case no longer reaches it. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 44 +++++++++++++---- tests/test_pr_review_merge_scheduler.py | 65 ++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index bc5340471e..120ecc7a25 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -3267,6 +3267,35 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio ) ) + if not current_head_approved and auto_merge_enabled: + # Neither behind-by disarm path applies (the branch is not behind + # base) and the last-push-approval restamp does not apply either (it + # requires current_head_approved). Yet auto-merge is still armed with + # no live current-head approval -- whether from a previously valid + # approval a new push has since invalidated, or from auto-merge armed + # before any review ever ran, this scheduler draws no distinction + # between the two (see the behind-by disarm path and the prior + # unconditional catch-all below, neither of which drew one either). + # Disarm immediately here, before any of the wait/dispatch branches + # below (OpenCode running, deterministic-fallback wait, stale-review + # retry, or the ordinary Strix/OpenCode dispatch cascade -- the + # everyday state for a PR between or during reviews) can return + # without having done so. Relying on a catch-all reached only once + # dispatch has nothing left to do would let GitHub's own native + # auto-merge complete the merge first if this scheduler is the only + # thing enforcing the OpenCode-approval requirement. + return finish( + disable_auto_merge_decision( + repo, + pr, + dry_run=dry_run, + reason=( + "current head has no OpenCode approval; wait for fresh same-head " + "approval before re-enabling auto-merge" + ), + ) + ) + opencode_state = opencode_progress_state(pr, stale_after_minutes=stale_opencode_minutes) if opencode_state == "running": return decide("wait", "OpenCode review is already in progress") @@ -3420,16 +3449,11 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio "current head has completed Strix evidence; same-head OpenCode dispatched", ) - if pr.get("autoMergeRequest"): - return finish( - disable_auto_merge_decision( - repo, - pr, - dry_run=dry_run, - reason="current head has no OpenCode approval; wait for fresh same-head approval before re-enabling auto-merge", - ) - ) - + # No autoMergeRequest re-check is needed here: the hoisted + # `not current_head_approved and auto_merge_enabled` guard above already + # disarmed and returned before any of the wait/dispatch branches between + # it and here could be reached, so auto-merge cannot still be armed by + # this point. return decide("block", "current head has no OpenCode approval") diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index b073067625..c76ff43460 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -2633,6 +2633,36 @@ def test_outdated_unapproved_branch_disarms_stale_auto_merge_instead_of_updating assert "remains queued" not in decision.reason +def test_clean_unapproved_armed_pr_disarms_auto_merge_before_review_dispatch(): + """An unapproved CLEAN PR with stale auto-merge armed must disarm, not dispatch-and-leave-armed. + + ``behind_by`` is falsy here (the branch is not behind base), so neither + behind-by disarm path applies. With no current-head approval and no + completed Strix evidence, the ordinary review-dispatch cascade would + normally return a plain ``security_dispatch``/``wait`` decision -- and + every branch in that cascade previously returned without ever checking + ``autoMergeRequest``. That left a stale auto-merge request (e.g. armed by + an approval a later push has since invalidated, or armed by a human + before any review ran) queued through the PR's ordinary, everyday + review-in-progress state. If GitHub's own required checks are not + themselves gated on this scheduler's OpenCode approval, GitHub's native + auto-merge could complete the merge without this scheduler ever getting a + chance to require a fresh independent approval -- the exact bypass this + scheduler exists to prevent. + """ + clean_unapproved_armed = make_pr( + mergeStateStatus="CLEAN", + autoMergeRequest={"enabledAt": "now"}, + reviews={"nodes": []}, + ) + + decision = inspect(clean_unapproved_armed) + + assert decision.action == "disable_auto_merge" + assert "current head has no OpenCode approval" in decision.reason + assert "wait for fresh same-head approval" in decision.reason + + def test_workflow_run_followup_defers_deterministic_fallback_retry(monkeypatch): head = "a" * 40 fallback_review = { @@ -4841,8 +4871,14 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): autoMergeRequest={"enabledAt": "now"}, ) ) + # An unapproved PR disarms as soon as "no current-head approval" is + # established, regardless of mergeability -- the hoisted guard in + # inspect_pr() fires before the merge_state == "UNKNOWN" branch's own + # (approved-only, see test_approved_unknown_mergeability_disarms_auto_merge) + # check is ever reached, so the reason names the real blocker (missing + # approval) rather than the unresolved mergeability calculation. assert unknown_auto_merge.action == "disable_auto_merge" - assert "mergeability is still being calculated" in unknown_auto_merge.reason + assert "current head has no OpenCode approval" in unknown_auto_merge.reason rest_clean = inspect( make_pr( mergeStateStatus="BEHIND", @@ -5420,6 +5456,33 @@ def test_inspect_pr_blocks_and_waits_for_policy_states(monkeypatch): assert disabled == [("owner/repo", 1, True)] +def test_approved_unknown_mergeability_disarms_auto_merge_pending_evaluation(): + """An approved PR whose mergeability is still UNKNOWN keeps its own disarm reason. + + Unlike the unapproved case (see the ``unknown_auto_merge`` assertion in + ``test_inspect_pr_blocks_and_waits_for_policy_states``), the hoisted + "no current-head approval" guard in ``inspect_pr`` does not apply here -- + ``current_head_approved`` is True, so this PR still reaches the + ``merge_state == "UNKNOWN"`` branch's own, more specific check: with + ``merge_mode="auto"`` and mergeability not yet CLEAN, the scheduler cannot + yet attempt the merge, so an auto-merge request armed on this still-being- + evaluated head is disarmed pending a resolved mergeability calculation, + not for any approval defect. + """ + approved_unknown_armed = make_pr( + mergeStateStatus="CLEAN", + restMergeableState="UNKNOWN", + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), + autoMergeRequest={"enabledAt": "now"}, + ) + + decision = inspect(approved_unknown_armed, merge_mode="auto") + + assert decision.action == "disable_auto_merge" + assert "mergeability is still being calculated" in decision.reason + + def test_stale_opencode_run_ids_filters_current_head_and_missing_ids(monkeypatch): runs = [ {"name": "Other", "id": 10, "head_sha": "old", "pull_requests": [{"number": 1}]}, From 39f4ae6573c8a6445e61220df572615ce94e8135 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 17:25:46 +0000 Subject: [PATCH 11/16] fix(scheduler): fix cross-workflow coverage-evidence rerun ordering latest_coverage_evidence_index() picked the newest coverage-evidence check run across workflow names using a naive timestamp max(), which had the same flaw latest_check_runs() was just fixed for: GitHub leaves CheckRun.startedAt null while a check is QUEUED, so a freshly dispatched coverage-evidence rerun in one workflow could lose to an older, already-completed coverage-evidence run in a *different* workflow, letting coverage_evidence_state() report stale "complete" status while the real, currently-relevant rerun was still pending. Extract the recency rule latest_check_runs() uses (defer to the check run's own pending status via running_check_state() only when a timestamp comparison is impossible) into a shared check_run_supersedes() helper, and fold latest_coverage_evidence_index()'s candidates through it instead of using max() with a pure-timestamp key. latest_check_runs() itself is unchanged in behavior -- it now just calls the extracted helper -- so its existing tests continue to pass unmodified. Added a regression test mirroring the existing same-workflow queued- rerun test, but across two different workflow names ("Required OpenCode Review" completed older vs. "OpenCode Review Dispatch" queued newer with startedAt: null); it fails against the prior implementation and passes after the fix. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 84 +++++++++++++++---------- tests/test_pr_review_merge_scheduler.py | 48 ++++++++++++++ 2 files changed, 99 insertions(+), 33 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 120ecc7a25..4db8bb2fc8 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1227,9 +1227,40 @@ def parse_github_datetime(value: str | None) -> datetime | None: return parsed.astimezone(timezone.utc) +def check_run_supersedes( + started_at: datetime | None, + node: dict[str, Any], + index: int, + previous_started_at: datetime | None, + previous_index: int, +) -> bool: + """Return whether a check run is newer than the current best-known run. + + Shared recency rule for folding a sequence of same-purpose check runs + (either the reruns sharing one (workflow, name) key in + ``latest_check_runs``, or the coverage-evidence runs + ``latest_coverage_evidence_index`` compares across workflow names) down + to the single newest one. A rerun that has not started yet has no + ``startedAt``, so it can never win a chronological comparison against an + already-started predecessor -- even though GitHub only ever creates it + after that predecessor. Fall back to the check run's own actively-pending + status (QUEUED/IN_PROGRESS/etc, the same predicate ``running_check_state`` + uses) as the recency signal in that case: a currently-pending run always + supersedes a predecessor that already has a result, regardless of + timestamps. A node with no startedAt and no pending status (e.g. + cancelled before it started) carries no such signal and keeps deferring + to the timestamped predecessor. Ties fall back to the later index. + """ + epoch = datetime.min.replace(tzinfo=timezone.utc) + if started_at is None and previous_started_at is not None: + return running_check_state(node) == "running" + if previous_started_at is None and started_at is not None: + return True + return (started_at or epoch, index) >= (previous_started_at or epoch, previous_index) + + def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: """Return the newest check run for each workflow and check-name pair.""" - epoch = datetime.min.replace(tzinfo=timezone.utc) latest: dict[ tuple[str, str], tuple[datetime | None, int, dict[str, Any]], @@ -1248,29 +1279,7 @@ def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: latest[key] = (started_at, index, node) continue previous_started_at, previous_index, _ = previous - if started_at is None and previous_started_at is not None: - # A rerun that has not started yet has no startedAt, so it can - # never win a chronological comparison against an - # already-started predecessor -- even though GitHub only ever - # creates it after that predecessor. Fall back to the check - # run's own actively-pending status (QUEUED/IN_PROGRESS/etc, the - # same predicate ``running_check_state`` uses) as the recency - # signal in that case: within one (workflow, name) key, a - # currently-pending run always supersedes a predecessor that - # already has a result, regardless of timestamps. A node with no - # startedAt and no pending status (e.g. cancelled before it - # started) carries no such signal and keeps deferring to the - # timestamped predecessor. - if running_check_state(node) == "running": - latest[key] = (started_at, index, node) - continue - if previous_started_at is None and started_at is not None: - latest[key] = (started_at, index, node) - continue - if (started_at or epoch, index) >= ( - previous_started_at or epoch, - previous_index, - ): + if check_run_supersedes(started_at, node, index, previous_started_at, previous_index): latest[key] = (started_at, index, node) return [node for _, _, node in sorted(latest.values(), key=lambda item: item[1])] @@ -1588,18 +1597,27 @@ def coverage_evidence_indices(check_runs: Sequence[dict[str, Any]]) -> list[int] def latest_coverage_evidence_index(check_runs: Sequence[dict[str, Any]]) -> int | None: - """Return the newest coverage-evidence index across workflow names.""" + """Return the newest coverage-evidence index across workflow names. + + Folds the candidates through the same ``check_run_supersedes`` recency + rule ``latest_check_runs`` uses within one (workflow, name) key, so a + freshly QUEUED coverage-evidence rerun (``startedAt: null``) in one + workflow correctly outranks an older, already-completed coverage-evidence + run in a *different* workflow instead of losing a naive timestamp + comparison because it has not started yet. + """ coverage_indices = coverage_evidence_indices(check_runs) if not coverage_indices: return None - return max( - coverage_indices, - key=lambda item: ( - parse_github_datetime(check_runs[item].get("startedAt")) - or datetime.min.replace(tzinfo=timezone.utc), - item, - ), - ) + best_index = coverage_indices[0] + best_started_at = parse_github_datetime(check_runs[best_index].get("startedAt")) + for item in coverage_indices[1:]: + node = check_runs[item] + started_at = parse_github_datetime(node.get("startedAt")) + if check_run_supersedes(started_at, node, item, best_started_at, best_index): + best_index = item + best_started_at = started_at + return best_index def coverage_evidence_state(pr: dict[str, Any]) -> str: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index c76ff43460..56853cf6f7 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -3145,6 +3145,54 @@ def coverage_check(workflow: str, started_at: str, conclusion: str) -> dict: assert sched.coverage_evidence_state(pr) == "complete" +def test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run_across_workflows(): + """A freshly queued rerun in a different workflow outranks an older completed run. + + Sibling of test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run: + that test covers the same-(workflow, name) case inside latest_check_runs; this one + covers latest_coverage_evidence_index's own cross-workflow-name selection, which has + the same null-``startedAt``-while-``QUEUED`` pitfall. An older, already-completed + "Required OpenCode Review" coverage-evidence run must not beat a newer, still-QUEUED + "OpenCode Review Dispatch" coverage-evidence run just because GitHub has not yet + populated the queued run's ``startedAt`` -- otherwise the scheduler could report + stale "complete" coverage state while the real, currently-relevant rerun is pending. + """ + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "COMPLETED", + "conclusion": "SUCCESS", + "startedAt": "2026-08-24T01:00:00Z", + "checkSuite": { + "workflowRun": {"workflow": {"name": "Required OpenCode Review"}} + }, + }, + { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": "QUEUED", + "conclusion": None, + "startedAt": None, + "checkSuite": { + "workflowRun": {"workflow": {"name": "OpenCode Review Dispatch"}} + }, + }, + ] + } + } + ) + + check_runs = sched.latest_check_runs(pr) + assert len(check_runs) == 2 + latest_index = sched.latest_coverage_evidence_index(check_runs) + assert check_runs[latest_index]["status"] == "QUEUED" + assert sched.coverage_evidence_state(pr) == "running" + + def test_central_coverage_placeholder_cannot_mask_dispatch_failure(monkeypatch): """Central metadata-only coverage success cannot hide failed dispatch evidence.""" monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") From 92d3afadceb392e7d1e1cbef304bacc4143072ed Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 17:43:47 +0000 Subject: [PATCH 12/16] fix(scheduler): exclude GraphQL bot actors missing the [bot] login suffix CodeRabbit flagged has_independent_current_head_approval's reviewer.endswith("[bot]") check as unreliable because GitHub's GraphQL API can omit the "[bot]" suffix from a bot actor's login (unlike REST, which reliably appends it). Verified empirically against this org's own PR #1270: the GraphQL-backed pull_request_read get_review_comments path returned "coderabbitai" and "devin-ai-integration" (no suffix) for the same accounts REST's get_reviews returned as "coderabbitai[bot]" and "devin-ai-integration[bot]". A GraphQL bot review whose login happens to omit the suffix could therefore count as an "independent" human approval in this exact separation-of-duties gate. Add __typename to the author field selection in both review-fetching GraphQL queries, and add is_bot_review_author() to exclude a review whenever its login ends with "[bot]" OR its author __typename is "Bot" -- keeping the suffix check for REST reviews, which never carry __typename. Reproduced first: the new tests failed against the prior code (missing __typename in the fragment/query, and an AttributeError for the not-yet-added helper) before the fix, and pass after it. --- scripts/ci/pr_review_merge_scheduler.py | 23 ++++++++-- tests/test_pr_review_merge_scheduler.py | 59 ++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 4db8bb2fc8..56e2bc83e5 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -60,7 +60,7 @@ state body submittedAt - author { login } + author { login __typename } commit { oid } } } @@ -129,7 +129,7 @@ state body submittedAt - author { login } + author { login __typename } commit { oid } } } @@ -1417,6 +1417,23 @@ def review_author_login(review: dict[str, Any]) -> str: return ((review.get("author") or {}).get("login") or "").lower() +def is_bot_review_author(review: dict[str, Any]) -> bool: + """Return whether a review's author is a GitHub bot actor. + + GitHub's REST API appends the ``[bot]`` suffix to a bot actor's + ``login`` (e.g. ``dependabot[bot]``), but GitHub's GraphQL API can + return the bare account name for that same actor (e.g. ``dependabot``) + while exposing ``__typename: "Bot"`` on the ``author`` field instead of + the suffix. Checking both keeps bot exclusion correct regardless of + which API surface -- and which suffix convention -- produced the + review node; ``rest_review_node`` never sets ``__typename``, so REST + reviews continue to rely solely on the login suffix. + """ + if review_author_login(review).endswith("[bot]"): + return True + return ((review.get("author") or {}).get("__typename")) == "Bot" + + def is_opencode_review(review: dict[str, Any]) -> bool: """Return whether a review was authored by the OpenCode agent.""" return review_author_login(review) in {"opencode-agent", "opencode-agent[bot]"} @@ -1487,7 +1504,7 @@ def has_independent_current_head_approval(pr: dict[str, Any]) -> bool: or reviewer == author or is_automated_opencode_review(review) or reviewer == "github-actions" - or reviewer.endswith("[bot]") + or is_bot_review_author(review) or not review_matches_current_head(review, pr) or state not in {"APPROVED", "CHANGES_REQUESTED", "DISMISSED"} or reviewer in seen_reviewers diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 56853cf6f7..0baee359eb 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -86,10 +86,14 @@ def opencode_review( commit="head", login="opencode-agent", submitted_at="2026-06-25T07:01:00Z", + author_typename=None, ): + author = {"login": login} + if author_typename is not None: + author["__typename"] = author_typename return { "state": state, - "author": {"login": login}, + "author": author, "submittedAt": submitted_at, "commit": {"oid": commit}, } @@ -2482,6 +2486,18 @@ def test_scheduler_query_requests_pull_request_author(): assert "\n author { login }\n" in sched.PULL_REQUEST_FIELDS_FRAGMENT +def test_scheduler_review_queries_request_bot_typename(): + """Request `__typename` on review authors so GraphQL bot actors are detectable. + + GitHub's GraphQL API can omit the `[bot]` suffix from a bot actor's + `login` (unlike REST, which reliably appends it), but exposes + `__typename: "Bot"` instead. Both review-fetching queries must select + it so `is_bot_review_author` can fall back to it. + """ + assert "author { login __typename }" in sched.PULL_REQUEST_FIELDS_FRAGMENT + assert "author { login __typename }" in sched.PR_REVIEWS_PAGE_QUERY + + @pytest.mark.parametrize( ("author", "reviewer", "state", "commit"), ( @@ -2517,6 +2533,47 @@ def test_independent_approval_fails_closed_for_invalid_evidence( assert "independent" in sched.merge_approval_block_reason(pr).lower() +def test_independent_approval_excludes_graphql_bot_actor_missing_suffix(): + """Exclude a GraphQL bot actor even when its `login` lacks the `[bot]` suffix. + + GitHub's GraphQL API can return a bot actor's raw account name (e.g. + `noema-review` instead of REST's `noema-review[bot]`) while still + identifying it as a bot via `__typename: "Bot"`. A login-suffix-only + check would let this slip through as an "independent" human approval -- + an authorization bypass in the exact separation-of-duties gate this + scheduler enforces. + """ + pr = make_pr( + reviewDecision="APPROVED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review( + "APPROVED", + "head", + login="noema-review", + author_typename="Bot", + ), + ] + }, + ) + + assert sched.is_bot_review_author(pr["reviews"]["nodes"][1]) + assert not sched.has_independent_current_head_approval(pr) + assert "independent" in sched.merge_approval_block_reason(pr).lower() + + +def test_is_bot_review_author_covers_suffix_and_typename_and_neither(): + """Detect a bot via either the REST `[bot]` login suffix or GraphQL `__typename`.""" + assert sched.is_bot_review_author(opencode_review(login="dependabot[bot]")) + assert sched.is_bot_review_author( + opencode_review(login="dependabot", author_typename="Bot") + ) + assert not sched.is_bot_review_author( + opencode_review(login="independent-reviewer", author_typename="User") + ) + + def test_independent_exact_head_approval_allows_direct_merge(): """Preserve direct merge only when GitHub and independent evidence both pass.""" pr = make_pr( From 7ac1ed3ba617343183298097b81d74fa2dea1ac3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 17:46:52 +0000 Subject: [PATCH 13/16] fix(scheduler): bound the same-head OpenCode dispatch-history lookup latest_opencode_dispatch_started_at called active_workflow_runs(dispatch_repo, ("completed",)) with no filter, which paginates every completed run ever recorded in the central dispatch repository via `gh api --paginate --slurp` -- verified by reading active_workflow_runs itself, which has no depth limit and applies all matching (event, title prefix, exact head SHA) client-side after the fetch. Since that repository's completed-run count only grows, this call site (reachable from the live inspect_pr path via coverage_retry_wait_reason, not dead code) could page through hundreds of runs per inspected PR and exhaust the REST rate limit; a resulting RuntimeError there specifically makes coverage_retry_wait_reason report "same-head OpenCode dispatch history is unavailable" and stall retries. GitHub's "List workflow runs for a repository" REST endpoint supports both `event` and `created` query parameters. Give active_workflow_runs optional event/created kwargs that add matching `-f` query parameters (existing callers that omit them keep the prior unfiltered request), and have latest_opencode_dispatch_started_at accept an optional `since` bound -- passed by its only caller as the coverage-request review's own submittedAt, since a dispatch created at or before that timestamp can never become the returned maximum. This narrows the REST query itself instead of changing which run is returned. Regression tests assert the constructed gh api args carry `event=repository_dispatch` and the `created=>=...` lower bound, that `since` forwards into `active_workflow_runs`, and that callers omitting the new kwargs still request the unfiltered query. --- scripts/ci/pr_review_merge_scheduler.py | 80 ++++++++++++----- tests/test_pr_review_merge_scheduler.py | 113 ++++++++++++++++++++++-- 2 files changed, 166 insertions(+), 27 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 56e2bc83e5..f172bcf3eb 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1579,7 +1579,9 @@ def coverage_retry_wait_reason( retry_anchor = submitted_at if repo and workflow: try: - dispatch_started_at = latest_opencode_dispatch_started_at(repo, workflow, pr) + dispatch_started_at = latest_opencode_dispatch_started_at( + repo, workflow, pr, since=retry_anchor + ) except RuntimeError: return "same-head OpenCode dispatch history is unavailable; defer same-head re-review" if dispatch_started_at and dispatch_started_at > retry_anchor: @@ -2329,27 +2331,45 @@ def rerun_actions_job(repo: str, job_id: str, *, dry_run: bool, action: str) -> run_github_actions(["gh", "api", "-X", "POST", f"repos/{repo}/actions/jobs/{job_id}/rerun"]) -def active_workflow_runs(repo: str, statuses: Sequence[str] = ("queued", "in_progress")) -> list[dict[str, Any]]: - """Return active workflow runs for a repository.""" +def active_workflow_runs( + repo: str, + statuses: Sequence[str] = ("queued", "in_progress"), + *, + event: str | None = None, + created: str | None = None, +) -> list[dict[str, Any]]: + """Return workflow runs for a repository, optionally narrowed server-side. + + ``event`` and ``created`` map directly onto GitHub's ``List workflow + runs for a repository`` REST query parameters (``event`` selects the + triggering webhook event, ``created`` accepts a date/range qualifier + such as ``>=2026-08-24T00:00:00Z``). Both are omitted by default so + existing callers keep fetching every run for the given statuses + unfiltered; a caller with a naturally bounded lookup -- one whose + target repository's run history only grows, such as a same-head + dispatch search -- should pass them to avoid paginating history it can + never use. + """ runs: list[dict[str, Any]] = [] for status in statuses: - payload = json.loads( - run_github_actions( - [ - "gh", - "api", - "--method", - "GET", - f"repos/{repo}/actions/runs", - "--paginate", - "--slurp", - "-f", - f"status={status}", - "-F", - "per_page=100", - ] - ) - ) + args = [ + "gh", + "api", + "--method", + "GET", + f"repos/{repo}/actions/runs", + "--paginate", + "--slurp", + "-f", + f"status={status}", + "-F", + "per_page=100", + ] + if event: + args += ["-f", f"event={event}"] + if created: + args += ["-f", f"created={created}"] + payload = json.loads(run_github_actions(args)) pages = payload if isinstance(payload, list) else [payload] for page in pages: runs.extend(page.get("workflow_runs") or []) @@ -2485,8 +2505,21 @@ def latest_opencode_dispatch_started_at( repo: str, workflow: str, pr: dict[str, Any], + *, + since: datetime | None = None, ) -> datetime | None: - """Return the latest completed same-head OpenCode dispatch start time.""" + """Return the latest completed same-head OpenCode dispatch start time. + + The dispatch repository hosting ``repository_dispatch`` runs only + accumulates completed-run history over time, so this narrows GitHub's + REST query server-side to ``event=repository_dispatch`` plus a + ``created`` lower bound of ``since``, instead of paginating every + completed run ever recorded there and filtering client-side. ``since`` + is safe to pass whenever the caller only cares about a dispatch strictly + newer than a known anchor timestamp -- any run created at or before that + anchor cannot become the returned maximum -- and is left unset (no lower + bound) for callers with no such anchor. + """ target_repo = validate_github_repository(repo) dispatch_repo = repository_dispatch_target(target_repo) head = str(pr.get("headRefOid") or "").lower() @@ -2499,8 +2532,11 @@ def latest_opencode_dispatch_started_at( reverse=True, ) ) + created = f">={since.strftime('%Y-%m-%dT%H:%M:%SZ')}" if since else None latest: datetime | None = None - for run_data in active_workflow_runs(dispatch_repo, ("completed",)): + for run_data in active_workflow_runs( + dispatch_repo, ("completed",), event="repository_dispatch", created=created + ): if run_data.get("event") != "repository_dispatch": continue display_title = str(run_data.get("display_title") or "") diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 0baee359eb..6e11109dfe 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1828,7 +1828,7 @@ def test_coverage_retry_floor_uses_latest_dispatch_timestamp(monkeypatch): monkeypatch.setattr( sched, "latest_opencode_dispatch_started_at", - lambda repo, workflow, pr: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), + lambda repo, workflow, pr, since=None: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), ) assert sched.coverage_retry_wait_reason( @@ -1874,7 +1874,9 @@ def test_coverage_retry_wait_reason_fails_closed_when_dispatch_history_is_unavai monkeypatch.setattr( sched, "latest_opencode_dispatch_started_at", - lambda repo, workflow, pr: (_ for _ in ()).throw(RuntimeError("temporary API failure")), + lambda repo, workflow, pr, since=None: (_ for _ in ()).throw( + RuntimeError("temporary API failure") + ), ) assert sched.coverage_retry_wait_reason( @@ -1906,7 +1908,7 @@ def test_coverage_retry_floor_keeps_newer_review_timestamp(monkeypatch): monkeypatch.setattr( sched, "latest_opencode_dispatch_started_at", - lambda repo, workflow, pr: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), + lambda repo, workflow, pr, since=None: datetime(2026, 8, 24, 1, 0, tzinfo=timezone.utc), ) assert sched.coverage_retry_wait_reason( @@ -4129,7 +4131,7 @@ def test_latest_opencode_dispatch_started_at_matches_exact_completed_run(monkeyp monkeypatch.setattr( sched, "active_workflow_runs", - lambda repo, statuses: [ + lambda repo, statuses, event=None, created=None: [ {"event": "push", "display_title": "irrelevant"}, { "event": "repository_dispatch", @@ -4175,7 +4177,7 @@ def test_latest_opencode_dispatch_started_at_returns_none_without_exact_run(monk monkeypatch.setattr( sched, "active_workflow_runs", - lambda repo, statuses: [ + lambda repo, statuses, event=None, created=None: [ { "event": "repository_dispatch", "display_title": "Required OpenCode Review owner/repo#1@" + "b" * 40, @@ -4191,6 +4193,107 @@ def test_latest_opencode_dispatch_started_at_returns_none_without_exact_run(monk ) is None +def test_latest_opencode_dispatch_started_at_passes_since_as_created_lower_bound( + monkeypatch, +): + """Forward ``since`` into the underlying ``active_workflow_runs`` REST filters.""" + captured = {} + + def fake_active_workflow_runs(repo, statuses, event=None, created=None): + captured["repo"] = repo + captured["statuses"] = statuses + captured["event"] = event + captured["created"] = created + return [] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_workflow_runs) + + sched.latest_opencode_dispatch_started_at( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid="a" * 40), + since=datetime(2026, 8, 24, 0, 0, tzinfo=timezone.utc), + ) + + assert captured["statuses"] == ("completed",) + assert captured["event"] == "repository_dispatch" + assert captured["created"] == ">=2026-08-24T00:00:00Z" + + +def test_latest_opencode_dispatch_started_at_without_since_leaves_created_unbounded( + monkeypatch, +): + """No ``since`` anchor means no ``created`` lower bound is requested.""" + captured = {} + + def fake_active_workflow_runs(repo, statuses, event=None, created=None): + captured["event"] = event + captured["created"] = created + return [] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_workflow_runs) + + sched.latest_opencode_dispatch_started_at( + "owner/repo", + "OpenCode Review", + make_pr(headRefOid="a" * 40), + ) + + assert captured["event"] == "repository_dispatch" + assert captured["created"] is None + + +def test_active_workflow_runs_narrows_query_with_event_and_created(monkeypatch): + """Request server-side ``event``/``created`` filters instead of full history. + + ``active_workflow_runs(dispatch_repo, ("completed",))`` with no filter + fetches the dispatch repository's *entire* completed-run history, since + matching against a target PR happens client-side after the fetch. The + central dispatch repository's completed-run count only grows, so a + same-head dispatch-history lookup must narrow the REST query itself. + """ + calls = [] + + def fake_run(args, stdin=None): + del stdin + calls.append(args) + return json.dumps([{"workflow_runs": []}]) + + monkeypatch.setattr(sched, "run_github_actions", fake_run) + + sched.active_workflow_runs( + "owner/repo", + ("completed",), + event="repository_dispatch", + created=">=2026-08-24T00:00:00Z", + ) + + assert len(calls) == 1 + args = calls[0] + assert "event=repository_dispatch" in args + assert "created=>=2026-08-24T00:00:00Z" in args + assert args.count("-f") == 3 # status, event, created + + +def test_active_workflow_runs_omits_filters_by_default(monkeypatch): + """Existing unfiltered callers keep requesting no ``event``/``created`` params.""" + calls = [] + + def fake_run(args, stdin=None): + del stdin + calls.append(args) + return json.dumps([{"workflow_runs": []}]) + + monkeypatch.setattr(sched, "run_github_actions", fake_run) + + sched.active_workflow_runs("owner/repo", ("queued",)) + + assert len(calls) == 1 + args = calls[0] + assert not any(str(arg).startswith("event=") for arg in args) + assert not any(str(arg).startswith("created=") for arg in args) + + def test_dispatch_strix_cancels_stale_central_run_and_keeps_current(monkeypatch, capsys): calls = [] head_sha = "a" * 40 From e655e5ce0909e94de0bd540c39d94a833c57fb72 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 18:04:34 +0000 Subject: [PATCH 14/16] fix(scheduler): make coverage-evidence recency ranking transitive check_run_supersedes was a pairwise "does B supersede A" predicate folded left-to-right across candidates. That is only valid when the relation is a transitive total order, and it was not: a queued/null-startedAt candidate could legitimately supersede an older completed predecessor, but a later, differently-timestamped completed candidate could then override that queued winner too -- purely because a timestamped candidate unconditionally beat a null-timestamp current-best -- even when that later candidate was itself older than whichever run the queued candidate had already displaced. Replace the pairwise fold with a single derived recency key (check_run_recency_key) per candidate: no-signal < timestamped (ranked by timestamp) < pending-with-no-timestamp-yet, ties broken by index. Comparing keys directly via Python tuple ordering is a valid total order by construction, so latest_check_runs and latest_coverage_evidence_index now pick the correct newest run regardless of candidate count or order. Adds regression tests for both scenarios a bot reviewer identified on this PR: a 3-candidate fold (completed@02:00, queued(null), completed@01:00) that previously settled on the stale 01:00 run, and the queued candidate appearing before the completed one it should outrank. --- scripts/ci/pr_review_merge_scheduler.py | 113 +++++++++++++----------- tests/test_pr_review_merge_scheduler.py | 66 ++++++++++++++ 2 files changed, 128 insertions(+), 51 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index f172bcf3eb..47d321cd4f 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -1227,43 +1227,55 @@ def parse_github_datetime(value: str | None) -> datetime | None: return parsed.astimezone(timezone.utc) -def check_run_supersedes( - started_at: datetime | None, - node: dict[str, Any], - index: int, - previous_started_at: datetime | None, - previous_index: int, -) -> bool: - """Return whether a check run is newer than the current best-known run. - - Shared recency rule for folding a sequence of same-purpose check runs - (either the reruns sharing one (workflow, name) key in - ``latest_check_runs``, or the coverage-evidence runs - ``latest_coverage_evidence_index`` compares across workflow names) down - to the single newest one. A rerun that has not started yet has no - ``startedAt``, so it can never win a chronological comparison against an - already-started predecessor -- even though GitHub only ever creates it - after that predecessor. Fall back to the check run's own actively-pending - status (QUEUED/IN_PROGRESS/etc, the same predicate ``running_check_state`` - uses) as the recency signal in that case: a currently-pending run always - supersedes a predecessor that already has a result, regardless of - timestamps. A node with no startedAt and no pending status (e.g. - cancelled before it started) carries no such signal and keeps deferring - to the timestamped predecessor. Ties fall back to the later index. +def check_run_recency_key( + node: dict[str, Any], started_at: datetime | None, index: int +) -> tuple[int, datetime, int]: + """Return a single comparable recency key for one same-purpose check run. + + Ranking a sequence of same-purpose check runs (either the reruns sharing + one (workflow, name) key in ``latest_check_runs``, or the + coverage-evidence runs ``latest_coverage_evidence_index`` compares across + workflow names) down to the single newest one used to be done by folding + a pairwise "does B supersede A" predicate left-to-right across the + candidates. That is only valid when the predicate is a transitive total + order, and it was not: a queued/null-``startedAt`` candidate could + legitimately supersede an older completed predecessor, but a later, + differently-timestamped completed candidate could then override that + queued winner too -- purely because "a timestamped candidate beats a + null-timestamp current-best" -- even when the later candidate was itself + older than whichever run the queued candidate had already displaced. + + Building one derived key per candidate instead, and comparing those keys + directly, cannot go non-transitive: Python tuple ordering is already a + valid total order, so ``max()``/``sorted()`` over these keys give a + result that does not depend on candidate order. + + Three tiers, low to high: + + * ``0`` -- no recency signal at all: no ``startedAt`` and not currently + pending (e.g. cancelled before it ever started). + * ``1`` -- a real ``startedAt``: ranked by that timestamp. + * ``2`` -- no ``startedAt`` yet, but actively pending (queued/in + progress/etc, via ``running_check_state``): GitHub only ever creates + such a row after any run it might supersede, so it is presumed newer + than every already-resolved run, regardless of that run's timestamp. + + Ties within a tier fall back to the later index, matching the order + ``context_nodes`` returns them in. """ epoch = datetime.min.replace(tzinfo=timezone.utc) - if started_at is None and previous_started_at is not None: - return running_check_state(node) == "running" - if previous_started_at is None and started_at is not None: - return True - return (started_at or epoch, index) >= (previous_started_at or epoch, previous_index) + if started_at is not None: + return (1, started_at, index) + if running_check_state(node) == "running": + return (2, epoch, index) + return (0, epoch, index) def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: """Return the newest check run for each workflow and check-name pair.""" latest: dict[ tuple[str, str], - tuple[datetime | None, int, dict[str, Any]], + tuple[tuple[int, datetime, int], dict[str, Any]], ] = {} for index, node in enumerate(context_nodes(pr)): if node.get("__typename") != "CheckRun": @@ -1274,14 +1286,11 @@ def latest_check_runs(pr: dict[str, Any]) -> list[dict[str, Any]]: ) key = (workflow, node.get("name") or "check-run") started_at = parse_github_datetime(node.get("startedAt")) + recency_key = check_run_recency_key(node, started_at, index) previous = latest.get(key) - if previous is None: - latest[key] = (started_at, index, node) - continue - previous_started_at, previous_index, _ = previous - if check_run_supersedes(started_at, node, index, previous_started_at, previous_index): - latest[key] = (started_at, index, node) - return [node for _, _, node in sorted(latest.values(), key=lambda item: item[1])] + if previous is None or recency_key >= previous[0]: + latest[key] = (recency_key, node) + return [node for _, node in sorted(latest.values(), key=lambda item: item[0][2])] def review_matches_current_head(review: dict[str, Any], pr: dict[str, Any]) -> bool: @@ -1618,25 +1627,27 @@ def coverage_evidence_indices(check_runs: Sequence[dict[str, Any]]) -> list[int] def latest_coverage_evidence_index(check_runs: Sequence[dict[str, Any]]) -> int | None: """Return the newest coverage-evidence index across workflow names. - Folds the candidates through the same ``check_run_supersedes`` recency - rule ``latest_check_runs`` uses within one (workflow, name) key, so a - freshly QUEUED coverage-evidence rerun (``startedAt: null``) in one - workflow correctly outranks an older, already-completed coverage-evidence - run in a *different* workflow instead of losing a naive timestamp - comparison because it has not started yet. + Ranks every coverage-evidence candidate with ``check_run_recency_key`` + and picks the single largest key via ``max()``, so a freshly QUEUED + coverage-evidence rerun (``startedAt: null``) in one workflow correctly + outranks an older, already-completed coverage-evidence run in a + *different* workflow instead of losing a naive timestamp comparison + because it has not started yet -- and, unlike folding a pairwise + supersession predicate two at a time, the answer does not depend on how + many other candidates are present or what order they arrive in, because + each candidate's key depends only on its own timestamp/pending status. """ coverage_indices = coverage_evidence_indices(check_runs) if not coverage_indices: return None - best_index = coverage_indices[0] - best_started_at = parse_github_datetime(check_runs[best_index].get("startedAt")) - for item in coverage_indices[1:]: - node = check_runs[item] - started_at = parse_github_datetime(node.get("startedAt")) - if check_run_supersedes(started_at, node, item, best_started_at, best_index): - best_index = item - best_started_at = started_at - return best_index + return max( + coverage_indices, + key=lambda item: check_run_recency_key( + check_runs[item], + parse_github_datetime(check_runs[item].get("startedAt")), + item, + ), + ) def coverage_evidence_state(pr: dict[str, Any]) -> str: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 6e11109dfe..9b08cafa1b 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -3252,6 +3252,72 @@ def test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run_a assert sched.coverage_evidence_state(pr) == "running" +def _coverage_check(workflow: str, status: str, conclusion: str | None, started_at: str | None) -> dict: + """Build a minimal coverage-evidence CheckRun node for fold-order tests.""" + return { + "__typename": "CheckRun", + "name": "coverage-evidence", + "status": status, + "conclusion": conclusion, + "startedAt": started_at, + "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + } + + +def test_latest_coverage_evidence_index_stays_transitive_across_three_candidates(): + """A left-to-right pairwise fold over 3+ candidates must not go non-transitive. + + Regression test for the exact scenario a GitHub bot reviewer ("Devin") + identified on this PR: folding ``check_run_supersedes`` two at a time + across MORE than two coverage-evidence candidates is only valid if the + relation it applies is a transitive total order. It was not: a + queued/null-``startedAt`` candidate legitimately supersedes an older + completed predecessor in isolation, but a THIRD, differently-timestamped + completed candidate arriving later in fold order could silently override + that queued winner just because "a timestamped candidate beats a + null-timestamp current-best" -- even though that third candidate is + itself OLDER than the timestamped run the queued candidate had already + displaced. + + Candidate order: completed@02:00, queued(startedAt=None), completed@01:00, + each in a different workflow so all three survive into + ``latest_coverage_evidence_index`` unchanged. A currently-pending rerun + is presumed newer than any already-resolved run (the same rule the + queued-vs-single-completed-run tests above already lock in), so the + queued candidate must win outright -- and, either way, the stale 01:00 + completed run (older than the 02:00 run it never legitimately beat) must + never be the answer. + """ + check_runs = [ + _coverage_check("Workflow A", "COMPLETED", "SUCCESS", "2026-08-24T02:00:00Z"), + _coverage_check("Workflow B", "QUEUED", None, None), + _coverage_check("Workflow C", "COMPLETED", "FAILURE", "2026-08-24T01:00:00Z"), + ] + + latest_index = sched.latest_coverage_evidence_index(check_runs) + + assert latest_index != 2, "the stale 01:00 completed run must never win the fold" + assert check_runs[latest_index]["status"] == "QUEUED" + + +def test_latest_coverage_evidence_index_prefers_queued_when_it_appears_first(): + """A queued rerun ranks correctly regardless of its position in the fold. + + Sibling of the reordering check above: places the queued/null-``startedAt`` + candidate FIRST instead of last, so a left-to-right fold cannot lean on + "the queued run happened to already be the running best" to get the + right answer by accident. + """ + check_runs = [ + _coverage_check("OpenCode Review Dispatch", "QUEUED", None, None), + _coverage_check("Required OpenCode Review", "COMPLETED", "SUCCESS", "2026-08-24T02:00:00Z"), + ] + + latest_index = sched.latest_coverage_evidence_index(check_runs) + + assert check_runs[latest_index]["status"] == "QUEUED" + + def test_central_coverage_placeholder_cannot_mask_dispatch_failure(monkeypatch): """Central metadata-only coverage success cannot hide failed dispatch evidence.""" monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") From 80c7f382cae5470ee87f37151e580e059fe3a8d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 18:23:17 +0000 Subject: [PATCH 15/16] fix(scheduler): re-validate current-head approval immediately before merge Devin Review flagged a TOCTOU race on PR #1270: inspect_pr() computes current_head_approved/approval_reason once, early in the function, from the GraphQL/REST snapshot this scheduler invocation fetched at the start of its run. Much later in the same invocation it reaches a branch that calls merge_pr()/enable_auto_merge() using that stale snapshot. If the reviewer who approved the exact head SHA dismisses or revokes that review -- or GitHub otherwise recomputes reviewDecision -- in the window between the snapshot and the mutating call (which can be many seconds to low-minutes, since this scheduler processes many PRs and dispatches several API calls per PR), the merge proceeds on authorization that no longer holds. The existing --match-head-commit guard only protects against the *commit* changing in that window; it does nothing to protect against the *review state* changing on the identical commit. Add revalidate_current_head_approval(repo, pr), which re-fetches the PR via the existing fetch_pr() helper and recomputes the exact same decision (has_current_head_approval + merge_approval_block_reason, the same helpers used for the original snapshot) from the fresh data. It fails closed on any re-fetch error or on the PR no longer being returned (closed/inaccessible), and also catches the head moving between snapshot and re-check as a defense in depth ahead of GitHub's own --match-head-commit guard. Call it, via a small revalidate_before_merge() closure inside inspect_pr(), immediately before each of the four merge_pr()/enable_auto_merge() call sites (both the merge_state == "CLEAN" fast path and the other-mergeable-state path each have a direct/direct_or_auto branch and a plain "auto" branch) -- not once upfront. If the fresh re-check no longer authorizes the merge, an already-queued auto-merge request is disarmed via the existing disable_auto_merge_decision() (never left queued for GitHub's own native auto-merge to complete unsupervised); otherwise the scheduler waits. dry_run inspection never mutates anything, so it skips the extra re-fetch entirely -- preserving every existing dry-run-based test in this file unmodified. TDD: added tests/test_pr_review_merge_scheduler.py coverage that first reproduces the race (mocking fetch_pr to return a freshly-revoked snapshot between the initial approved snapshot and the mutating call) and confirms it fails against the pre-fix code -- the scheduler would still call merge_pr()/enable_auto_merge() despite the revocation -- then confirms the fix blocks it across all four call sites (CLEAN and BLOCKED mergeability x direct/direct_or_auto/auto merge modes), confirms a still-valid re-check lets the merge proceed normally, confirms a re-check API failure fails closed without merging, and confirms dry-run skips the extra fetch. Full suite: 1975 passed, 1 skipped; coverage 100% on scripts/ci; interrogate 100%; no new ruff findings (diffed against the pre-fix commit). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 89 ++++++ tests/test_pr_review_merge_scheduler.py | 367 ++++++++++++++++++++++++ 2 files changed, 456 insertions(+) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 47d321cd4f..39845154e5 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -2850,6 +2850,63 @@ def current_head_can_attempt_merge(pr: dict[str, Any], merge_state: str) -> bool return False +def revalidate_current_head_approval(repo: str, pr: dict[str, Any]) -> str | None: + """Re-check exact-head approval immediately before a merge-authorizing mutation. + + ``inspect_pr`` computes ``current_head_approved``/``approval_reason`` once, early + in the function, from the GraphQL/REST snapshot this scheduler invocation fetched + at the start of its run. Much later in the same invocation it reaches a branch + that calls ``merge_pr``/``enable_auto_merge`` using that stale snapshot. If the + reviewer who approved the exact head SHA dismisses or revokes that review -- or + GitHub otherwise recomputes ``reviewDecision`` -- in the window between the + snapshot and the mutating call, the merge would proceed on authorization that no + longer holds. The ``--match-head-commit`` guard those mutations carry only + protects against the *commit* changing in that window; it does nothing to protect + against the *review state* changing on the identical commit. + + Re-fetch the pull request right before the mutating call and recompute the exact + same independent exact-head approval decision (``has_current_head_approval`` and + ``merge_approval_block_reason``, the same helpers used for the original snapshot) + from the fresh data. Returns ``None`` when the fresh snapshot still authorizes the + merge, or a human-readable reason to block it otherwise. Any failure to re-fetch -- + a transient API error, or the pull request no longer being open or accessible -- + fails closed: it is treated exactly like a freshly observed missing approval so a + merge can never proceed on evidence this scheduler could not actually reconfirm. + """ + number = pr["number"] + try: + refreshed = fetch_pr(repo, number) + except RuntimeError as exc: + return ( + "re-checking current-head approval immediately before merge failed " + f"({exc}); treating the exact-head approval as unconfirmed" + ) + if not refreshed: + return ( + "re-checking current-head approval immediately before merge found PR " + f"#{number} no longer open or accessible; treating the exact-head " + "approval as unconfirmed" + ) + fresh_pr = refreshed[0] + expected_head = pr.get("headRefOid") + fresh_head = fresh_pr.get("headRefOid") + if expected_head and fresh_head and fresh_head != expected_head: + return ( + f"current head changed from {short_sha(expected_head)} to " + f"{short_sha(fresh_head)} immediately before merge; the exact-head " + "approval no longer applies to the current commit" + ) + if not has_current_head_approval(fresh_pr): + return ( + "current-head OpenCode approval was revoked immediately before merge; " + "the merge-authorizing snapshot is no longer current" + ) + reason = merge_approval_block_reason(fresh_pr) + if reason: + return f"{reason} (re-confirmed immediately before merge)" + return None + + def inspect_pr( repo: str, pr: dict[str, Any], @@ -2960,6 +3017,26 @@ def decide(action: str, reason: str) -> Decision: """Create a decision after applying shared cleanup notes.""" return finish(Decision(number, action, reason)) + def revalidate_before_merge() -> Decision | None: + """Return a blocking decision if a fresh re-check just revoked approval. + + Call this immediately before every ``merge_pr``/``enable_auto_merge`` + invocation below, after every other authorization check has already passed + against the (possibly stale) snapshot fetched at the top of this scheduler + invocation -- closing the TOCTOU window between that snapshot and the + mutating call. Returns ``None`` when the fresh re-check still authorizes the + merge, so the caller proceeds unchanged. dry-run inspection never mutates + anything, so it skips the extra re-fetch entirely. + """ + if dry_run: + return None + reason = revalidate_current_head_approval(repo, pr) + if not reason: + return None + if pr.get("autoMergeRequest"): + return finish(disable_auto_merge_decision(repo, pr, dry_run=dry_run, reason=reason)) + return decide("wait", reason) + def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decision: """Request update-branch and attach any same-head evidence follow-up.""" if not branch_update_allowed: @@ -3231,6 +3308,9 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return decide("wait", auto_merge_wait_reason(merge_state, pr)) return decide("wait", "current head is approved; merge mode disabled by scheduler inputs") if merge_mode in {"direct", "direct_or_auto"}: + revalidation = revalidate_before_merge() + if revalidation: + return revalidation try: merge_pr(repo, pr, dry_run=dry_run) except RuntimeError as exc: @@ -3261,6 +3341,9 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return decide("wait", f"current head is approved; unsupported merge mode: {merge_mode}") if pr.get("autoMergeRequest"): return decide("wait", auto_merge_wait_reason(merge_state, pr)) + revalidation = revalidate_before_merge() + if revalidation: + return revalidation enable_auto_merge(repo, pr, dry_run=dry_run) return decide("auto_merge", "current head is approved; auto-merge enabled") @@ -3436,6 +3519,9 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio return decide("wait", "current head is approved; merge mode disabled by scheduler inputs") if merge_mode in {"direct", "direct_or_auto"}: if merge_mode == "direct_or_auto": + revalidation = revalidate_before_merge() + if revalidation: + return revalidation try: merge_pr(repo, pr, dry_run=dry_run) except RuntimeError as exc: @@ -3460,6 +3546,9 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio ) if merge_mode != "auto": return decide("wait", f"current head is approved; unsupported merge mode: {merge_mode}") + revalidation = revalidate_before_merge() + if revalidation: + return revalidation enable_auto_merge(repo, pr, dry_run=dry_run) return decide("auto_merge", "current head is approved; auto-merge enabled") diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 9b08cafa1b..fa50ec90d8 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -7250,3 +7250,370 @@ def test_run_masks_secrets_in_args(): err_msg = str(exc_info.value) assert token not in err_msg assert "***" in err_msg + + +# --- TOCTOU close: re-validate current-head approval immediately before merge --- +# +# `current_head_approved`/`approval_reason` are computed once, early in +# `inspect_pr`, from the snapshot this scheduler invocation fetched at the +# start of its run. `revalidate_current_head_approval` re-fetches the PR and +# recomputes the same decision immediately before each merge_pr/enable_auto_merge +# call site, so a same-head approval revoked in that window cannot still +# authorize a merge. The `--match-head-commit` guard on those mutations only +# protects against the commit changing, not the review state changing on the +# identical commit. + + +def test_revalidate_current_head_approval_confirms_still_valid_approval(monkeypatch): + """A fresh re-fetch that still shows exact-head approval returns no block.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [approved]) + + assert sched.revalidate_current_head_approval("owner/repo", approved) is None + + +def test_revalidate_current_head_approval_catches_revoked_review_decision(monkeypatch): + """GitHub reviewDecision falling out of APPROVED between snapshot and merge blocks.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(reviewDecision="REVIEW_REQUIRED", reviews=merge_approved_reviews()) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [revoked_fresh]) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason is not None + assert "reviewDecision is REVIEW_REQUIRED" in reason + assert "re-confirmed immediately before merge" in reason + + +def test_revalidate_current_head_approval_catches_dismissed_independent_review(monkeypatch): + """An independent approver's exact-head review moving to DISMISSED blocks, even + when GitHub's cached reviewDecision has not yet caught up.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr( + reviewDecision="APPROVED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head"), + opencode_review("APPROVED", "head", login="independent-reviewer", submitted_at="2026-06-25T07:01:00Z"), + opencode_review("DISMISSED", "head", login="independent-reviewer", submitted_at="2026-06-25T07:02:00Z"), + ] + }, + ) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [revoked_fresh]) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason is not None + assert "no independent non-author exact-current-head formal APPROVED review exists" in reason + assert "re-confirmed immediately before merge" in reason + + +def test_revalidate_current_head_approval_catches_revoked_opencode_approval(monkeypatch): + """OpenCode's own exact-head approval being superseded blocks before merge.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr( + reviewDecision="APPROVED", + reviews={ + "nodes": [ + opencode_review("APPROVED", "head", submitted_at="2026-06-25T07:01:00Z"), + opencode_review("CHANGES_REQUESTED", "head", submitted_at="2026-06-25T07:02:00Z"), + opencode_review("APPROVED", "head", login="independent-reviewer"), + ] + }, + ) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [revoked_fresh]) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason == ( + "current-head OpenCode approval was revoked immediately before merge; " + "the merge-authorizing snapshot is no longer current" + ) + + +def test_revalidate_current_head_approval_catches_head_moving_before_merge(monkeypatch): + """A head that moved between the snapshot and the re-check is never treated as + still-approved evidence, even before GitHub's own --match-head-commit guard runs.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + moved_fresh = make_pr( + headRefOid="new-head", + reviewDecision="APPROVED", + reviews=merge_approved_reviews(commit="new-head"), + ) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: [moved_fresh]) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason is not None + assert "current head changed" in reason + + +def test_revalidate_current_head_approval_fails_closed_on_refetch_error(monkeypatch): + """A re-fetch failure must never be treated as a still-valid approval.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + + def raise_refetch(repo, number): + raise RuntimeError("gh api graphql: 502 Bad Gateway") + + monkeypatch.setattr(sched, "fetch_pr", raise_refetch) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason is not None + assert "re-checking current-head approval immediately before merge failed" in reason + assert "502 Bad Gateway" in reason + + +def test_revalidate_current_head_approval_fails_closed_when_pr_disappears(monkeypatch): + """A PR no longer returned by a re-fetch (closed/inaccessible) fails closed.""" + snapshot = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + monkeypatch.setattr(sched, "fetch_pr", lambda repo, number: []) + + reason = sched.revalidate_current_head_approval("owner/repo", snapshot) + + assert reason is not None + assert "no longer open or accessible" in reason + + +def test_inspect_pr_direct_merge_blocked_when_approval_revoked_before_merge(monkeypatch): + """Reproduces the confirmed TOCTOU finding: a same-head approval revoked after + approval_reason is computed must not still authorize a direct merge.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(reviewDecision="REVIEW_REQUIRED", reviews=merge_approved_reviews()) + + fetch_calls = [] + merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + + decision = inspect(approved, merge_mode="direct", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [] + assert decision.action == "wait" + assert "reviewDecision is REVIEW_REQUIRED" in decision.reason + assert "re-confirmed immediately before merge" in decision.reason + + +def test_inspect_pr_direct_or_auto_merge_blocked_when_approval_revoked_before_merge(monkeypatch): + """The direct_or_auto merge path re-validates before attempting merge_pr too.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(reviewDecision="CHANGES_REQUESTED", reviews=merge_approved_reviews()) + + fetch_calls = [] + merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + + decision = inspect(approved, merge_mode="direct_or_auto", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [] + assert decision.action == "wait" + assert "reviewDecision is CHANGES_REQUESTED" in decision.reason + + +def test_inspect_pr_auto_merge_blocked_when_approval_revoked_before_enable(monkeypatch): + """The plain auto merge-mode path re-validates before enable_auto_merge too.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(reviewDecision="REVIEW_REQUIRED", reviews=merge_approved_reviews()) + + fetch_calls = [] + auto_merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, + "enable_auto_merge", + lambda repo, pr, dry_run: auto_merge_calls.append((repo, pr["number"], dry_run)), + ) + + decision = inspect(approved, merge_mode="auto", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert auto_merge_calls == [] + assert decision.action == "wait" + assert "reviewDecision is REVIEW_REQUIRED" in decision.reason + + +def test_inspect_pr_disables_queued_auto_merge_when_approval_revoked_before_merge(monkeypatch): + """A queued auto-merge request must be disarmed, not left queued, when the fresh + re-check reveals the authorizing approval no longer holds.""" + approved_with_auto_merge = make_pr( + reviewDecision="APPROVED", + reviews=merge_approved_reviews(), + autoMergeRequest={"enabledAt": "now"}, + ) + revoked_fresh = make_pr( + reviewDecision="REVIEW_REQUIRED", + reviews=merge_approved_reviews(), + autoMergeRequest={"enabledAt": "now"}, + ) + + fetch_calls = [] + merge_calls = [] + disabled = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + monkeypatch.setattr( + sched, + "disable_auto_merge", + lambda repo, pr, dry_run: disabled.append((repo, pr["number"], dry_run)), + ) + + decision = inspect(approved_with_auto_merge, merge_mode="direct_or_auto", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [] + assert disabled == [("owner/repo", 1, False)] + assert decision.action == "disable_auto_merge" + assert "reviewDecision is REVIEW_REQUIRED" in decision.reason + assert "re-confirmed immediately before merge" in decision.reason + + +def test_inspect_pr_blocked_direct_or_auto_merge_blocked_when_approval_revoked_before_merge(monkeypatch): + """The BLOCKED-mergeability direct_or_auto branch (reached when merge_state is not + CLEAN, so outside the merge_before_update gate) also re-validates before merge_pr.""" + approved = make_pr(mergeStateStatus="BLOCKED", reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(mergeStateStatus="BLOCKED", reviewDecision="REVIEW_REQUIRED", reviews=merge_approved_reviews()) + + fetch_calls = [] + merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + + decision = inspect(approved, merge_mode="direct_or_auto", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [] + assert decision.action == "wait" + assert "reviewDecision is REVIEW_REQUIRED" in decision.reason + + +def test_inspect_pr_blocked_auto_merge_blocked_when_approval_revoked_before_enable(monkeypatch): + """The BLOCKED-mergeability plain auto branch (outside the merge_before_update + gate) also re-validates before enable_auto_merge.""" + approved = make_pr(mergeStateStatus="BLOCKED", reviewDecision="APPROVED", reviews=merge_approved_reviews()) + revoked_fresh = make_pr(mergeStateStatus="BLOCKED", reviewDecision="REVIEW_REQUIRED", reviews=merge_approved_reviews()) + + fetch_calls = [] + auto_merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [revoked_fresh], + ) + monkeypatch.setattr( + sched, + "enable_auto_merge", + lambda repo, pr, dry_run: auto_merge_calls.append((repo, pr["number"], dry_run)), + ) + + decision = inspect(approved, merge_mode="auto", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert auto_merge_calls == [] + assert decision.action == "wait" + assert "reviewDecision is REVIEW_REQUIRED" in decision.reason + + +def test_inspect_pr_direct_merge_proceeds_when_revalidation_confirms_approval(monkeypatch): + """Approval still valid at the immediate re-check: the merge proceeds normally.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + still_approved_fresh = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + + fetch_calls = [] + merge_calls = [] + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [still_approved_fresh], + ) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + + decision = inspect(approved, merge_mode="direct", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [("owner/repo", 1, False)] + assert decision.action == "merge" + + +def test_inspect_pr_fails_closed_when_revalidation_refetch_errors(monkeypatch): + """A re-check API failure right before merge must never let the merge proceed.""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + + fetch_calls = [] + merge_calls = [] + + def raise_refetch(repo, number): + fetch_calls.append((repo, number)) + raise RuntimeError("gh api graphql: 502 Bad Gateway") + + monkeypatch.setattr(sched, "cancel_stale_pr_runs", lambda repo, pr, dry_run: []) + monkeypatch.setattr(sched, "fetch_pr", raise_refetch) + monkeypatch.setattr( + sched, "merge_pr", lambda repo, pr, dry_run: merge_calls.append((repo, pr["number"], dry_run)) + ) + + decision = inspect(approved, merge_mode="direct", dry_run=False) + + assert fetch_calls == [("owner/repo", 1)] + assert merge_calls == [] + assert decision.action == "wait" + assert "re-checking current-head approval immediately before merge failed" in decision.reason + + +def test_inspect_pr_dry_run_skips_merge_revalidation_refetch(monkeypatch): + """Dry-run inspection never mutates anything, so it must not pay for the extra + re-fetch either -- and every existing dry-run merge/auto_merge assertion in this + file relies on that (no fetch_pr mock is installed for those).""" + approved = make_pr(reviewDecision="APPROVED", reviews=merge_approved_reviews()) + fetch_calls = [] + monkeypatch.setattr( + sched, + "fetch_pr", + lambda repo, number: fetch_calls.append((repo, number)) or [approved], + ) + + direct_decision = inspect(approved, merge_mode="direct") + auto_decision = inspect(approved, merge_mode="auto") + + assert direct_decision.action == "merge" + assert auto_decision.action == "auto_merge" + assert fetch_calls == [] From 63cd827f8a66ba36c0bc0af61e0830fbab0622a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 18:44:43 +0000 Subject: [PATCH 16/16] fix(scheduler): rank check-run recency by check-suite creation time check_run_recency_key gave a completed check run with no startedAt (GitHub's shape for "cancelled before it ever started") the lowest tier unconditionally, below any row with a real startedAt -- so an older, already-successful check run could outrank a newer rerun that was cancelled before starting, and the scheduler could merge on a stale success (Devin BUG finding on PR #1270). Fix: prefer checkSuite.createdAt over startedAt as the recency signal. GitHub creates the check suite unconditionally the instant the triggering push/rerun/dispatch fires, strictly before any of its check runs can be queued, start, or be cancelled before starting, and CheckSuite.createdAt is non-nullable in GitHub's schema -- so it is always available, unlike startedAt. This is a general signal, not a special case for conclusion == "cancelled": every check run is now ranked by the earliest-available real creation timestamp, falling back to the old startedAt/pending-tier heuristic only when checkSuite.createdAt is absent (kept for backward compatibility with existing fixtures/tests that predate this field). Note: GraphQL CheckRun/CheckSuite.databaseId (Int) was considered and rejected -- verified against GitHub's public schema and a live query that today's check-run/check-suite database ids are ~10^11, well past the 32-bit Int range those fields are typed as (the exact reason GitHub has been migrating other databaseId fields to fullDatabaseId: BigInt), so it cannot be trusted as an always-present signal. checkSuite.createdAt is a real DateTime field with no such limit. Also fetches createdAt in the GraphQL check-run fragment, and adds a parity REST fallback path (one extra check-suites-for-ref call, joined by check_suite.id) so rest_check_node carries the same signal as the GraphQL path. Tests: two new regressions reproduce Devin's scenario (a) through latest_check_runs and latest_coverage_evidence_index respectively (confirmed failing pre-fix), and a third locks in scenario (b) -- a newer completed rerun still outranks an older queued run once checkSuite.createdAt is available, so the fix doesn't regress the existing "queued presumed newest" fallback behavior. All existing check_run_recency_key/latest_check_runs/latest_coverage_evidence_index tests (including the round-6 transitivity regressions) pass unmodified in intent. Full suite: 1978 passed, 1 skipped. coverage: 100% on scripts/ci. interrogate: 100% docstrings. ruff: no new findings (the one pre-existing unrelated F401 in tests/test_repository_branch_coverage_javascript_and_noema.py, already confirmed unrelated in round 7, is present identically on base commit 80c7f382). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX --- scripts/ci/pr_review_merge_scheduler.py | 73 +++++++++--- tests/test_pr_review_merge_scheduler.py | 146 +++++++++++++++++++++++- 2 files changed, 203 insertions(+), 16 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 39845154e5..f9c08e7db1 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -75,6 +75,7 @@ startedAt detailsUrl checkSuite { + createdAt workflowRun { workflow { name } } @@ -903,9 +904,20 @@ def fetch_all_pr_reviews_rest(repo: str, number: int) -> list[dict[str, Any]]: return reviews -def rest_check_node(check: dict[str, Any]) -> dict[str, Any]: - """Convert a REST check-run payload into the GraphQL status rollup shape.""" - +def rest_check_node( + check: dict[str, Any], suite_created_at_by_id: dict[int, str] | None = None +) -> dict[str, Any]: + """Convert a REST check-run payload into the GraphQL status rollup shape. + + ``suite_created_at_by_id`` maps each check suite's REST ``id`` to its + ``created_at`` timestamp -- the REST check-run payload itself only + carries the check suite's bare ``id`` (see ``rest_pr_node``), so that + lookup is how this function attaches the same ``checkSuite.createdAt`` + signal the GraphQL fragment fetches directly, keeping + ``check_run_recency_key`` behaviorally consistent across both paths. + """ + suite_id = (check.get("check_suite") or {}).get("id") + suite_created_at = (suite_created_at_by_id or {}).get(suite_id) return { "__typename": "CheckRun", "name": check.get("name"), @@ -913,7 +925,7 @@ def rest_check_node(check: dict[str, Any]) -> dict[str, Any]: "conclusion": (check.get("conclusion") or "").upper() if check.get("conclusion") else None, "startedAt": check.get("started_at"), "detailsUrl": check.get("details_url"), - "checkSuite": {"workflowRun": {"workflow": {}}}, + "checkSuite": {"createdAt": suite_created_at, "workflowRun": {"workflow": {}}}, } @@ -926,6 +938,12 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: head_repo = head.get("repo") or {} reviews = fetch_all_pr_reviews_rest(repo, number) checks = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-runs?per_page=100") + check_suites = gh_api_json(f"repos/{repo}/commits/{head.get('sha')}/check-suites?per_page=100") + suite_created_at_by_id = { + suite["id"]: suite.get("created_at") + for suite in (check_suites.get("check_suites") or []) + if suite.get("id") is not None + } files = gh_api_json(f"repos/{repo}/pulls/{number}/files?per_page=20") rest_merge_state = REST_MERGEABLE_STATE_MAP.get( str(pr.get("mergeable_state") or "").lower(), @@ -953,7 +971,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]: "statusCheckRollup": { "contexts": { "nodes": [ - rest_check_node(check) + rest_check_node(check, suite_created_at_by_id) for check in (checks.get("check_runs") or []) ] } @@ -1250,22 +1268,49 @@ def check_run_recency_key( valid total order, so ``max()``/``sorted()`` over these keys give a result that does not depend on candidate order. + A ``startedAt``-only signal has a gap: GitHub reports a check run as + ``completed``/``cancelled`` with ``startedAt: null`` when a queued rerun + is cancelled before it ever starts, so that row carries no timestamp and + is not pending either -- nothing (short of hardcoding the ``cancelled`` + conclusion, which would only patch this one case) distinguishes it from + a run that legitimately never mattered. ``checkSuite.createdAt`` closes + that gap generally instead of special-casing it: GitHub creates the + check suite unconditionally the moment the triggering push, rerun, or + dispatch happens, strictly before any check run inside it can be queued, + start, or be cancelled before starting, and ``CheckSuite.createdAt`` is + non-nullable in GitHub's schema. So it is a recency signal that is + always available, for every check run regardless of how it resolved -- + unlike ``startedAt``, which is genuinely absent for a run that never + started. + Three tiers, low to high: - * ``0`` -- no recency signal at all: no ``startedAt`` and not currently - pending (e.g. cancelled before it ever started). - * ``1`` -- a real ``startedAt``: ranked by that timestamp. - * ``2`` -- no ``startedAt`` yet, but actively pending (queued/in - progress/etc, via ``running_check_state``): GitHub only ever creates - such a row after any run it might supersede, so it is presumed newer - than every already-resolved run, regardless of that run's timestamp. + * ``0`` -- no recency signal at all: neither the check run's own check + suite ``createdAt`` nor its ``startedAt`` is available, and it is not + currently pending either. Real GitHub responses always carry + ``checkSuite.createdAt``, so this tier is only reachable for + payloads that omit it (e.g. hand-built fixtures). + * ``1`` -- a real timestamp: the check run's own check suite + ``createdAt`` when present, else its ``startedAt``. Preferring the + check-suite timestamp means two runs are ranked by when each was + actually triggered, not by whether either one got far enough to + start -- a rerun cancelled before starting still ranks correctly + relative to an older, already-completed run. + * ``2`` -- no timestamp of any kind, but actively pending (queued/in + progress/etc, via ``running_check_state``): kept only as the + fallback for payloads without ``checkSuite.createdAt``, where GitHub + only ever creates such a row after any run it might supersede, so it + is presumed newer than every already-resolved run in that same + payload shape, regardless of that run's timestamp. Ties within a tier fall back to the later index, matching the order ``context_nodes`` returns them in. """ epoch = datetime.min.replace(tzinfo=timezone.utc) - if started_at is not None: - return (1, started_at, index) + suite_created_at = parse_github_datetime((node.get("checkSuite") or {}).get("createdAt")) + recency_timestamp = suite_created_at or started_at + if recency_timestamp is not None: + return (1, recency_timestamp, index) if running_check_state(node) == "running": return (2, epoch, index) return (0, epoch, index) diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index fa50ec90d8..40693299cd 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -999,9 +999,15 @@ def test_rest_pr_fallback_shapes_reviews_and_checks(monkeypatch): "conclusion": "success", "started_at": "2026-06-30T00:00:00Z", "details_url": "https://github.com/owner/repo/actions/runs/1/job/2", + "check_suite": {"id": 555}, } ] }, + "repos/owner/repo/commits/abc123/check-suites?per_page=100": { + "check_suites": [ + {"id": 555, "created_at": "2026-06-30T00:00:00Z"}, + ] + }, "repos/owner/repo/pulls/42/files?per_page=20": [ {"filename": "scripts/ci/pr_review_merge_scheduler.py"}, ], @@ -1034,6 +1040,7 @@ def fake_api(path): assert calls == [ "repos/owner/repo/pulls/42/reviews?per_page=100&page=1", "repos/owner/repo/commits/abc123/check-runs?per_page=100", + "repos/owner/repo/commits/abc123/check-suites?per_page=100", "repos/owner/repo/pulls/42/files?per_page=20", ] assert node["number"] == 42 @@ -1046,6 +1053,10 @@ def fake_api(path): assert node["reviews"]["nodes"][0]["commit"]["oid"] == "abc123" assert node["statusCheckRollup"]["contexts"]["nodes"][0]["status"] == "COMPLETED" assert node["statusCheckRollup"]["contexts"]["nodes"][0]["conclusion"] == "SUCCESS" + assert ( + node["statusCheckRollup"]["contexts"]["nodes"][0]["checkSuite"]["createdAt"] + == "2026-06-30T00:00:00Z" + ) def test_fetch_pr_falls_back_to_rest_when_graphql_denied(monkeypatch): @@ -3100,6 +3111,65 @@ def test_failed_status_checks_prefers_timestamped_duplicate_check_runs(): assert sched.failed_status_checks(missing_then_timestamped) == [] +def test_failed_status_checks_treats_cancelled_before_start_rerun_as_authoritative(): + """A newer rerun cancelled before starting outranks an older stale success. + + Regression test for the BUG a GitHub bot reviewer ("Devin") found on this + PR: ``check_run_recency_key`` gave a completed-with-no-``startedAt`` row + (GitHub's shape for "cancelled before it ever started") the lowest tier, + unconditionally below any row with a real ``startedAt`` -- so an older, + already-successful run could outrank a newer rerun that got cancelled + before starting, purely because the older run happened to have a + timestamp and the newer one did not, regardless of which one actually + happened more recently. This must fail against the pre-fix code and pass + once ``check_run_recency_key`` prefers ``checkSuite.createdAt`` -- set + unconditionally the moment the triggering push/rerun/dispatch creates the + check suite, strictly before any of its check runs can start -- over a + bare ``startedAt``. Both runs here carry a ``checkSuite.createdAt``, as + real GitHub responses always do; the newer one must win even though it + never started. + """ + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "scan-pr-queue", + "status": "COMPLETED", + "conclusion": "SUCCESS", + "startedAt": "2026-08-24T01:00:00Z", + "checkSuite": { + "createdAt": "2026-08-24T00:59:00Z", + "workflowRun": { + "workflow": {"name": "Required PR Review Merge Scheduler"} + }, + }, + }, + { + "__typename": "CheckRun", + "name": "scan-pr-queue", + "status": "COMPLETED", + "conclusion": "CANCELLED", + "startedAt": None, + "checkSuite": { + "createdAt": "2026-08-24T02:00:00Z", + "workflowRun": { + "workflow": {"name": "Required PR Review Merge Scheduler"} + }, + }, + }, + ] + } + } + ) + + check_runs = sched.latest_check_runs(pr) + assert len(check_runs) == 1 + assert check_runs[0]["conclusion"] == "CANCELLED" + assert sched.failed_status_checks(pr) == ["scan-pr-queue"] + + def test_coverage_evidence_state_prefers_newest_rerun(): """An older failed rerun cannot hide newer successful coverage evidence.""" @@ -3252,7 +3322,13 @@ def test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run_a assert sched.coverage_evidence_state(pr) == "running" -def _coverage_check(workflow: str, status: str, conclusion: str | None, started_at: str | None) -> dict: +def _coverage_check( + workflow: str, + status: str, + conclusion: str | None, + started_at: str | None, + suite_created_at: str | None = None, +) -> dict: """Build a minimal coverage-evidence CheckRun node for fold-order tests.""" return { "__typename": "CheckRun", @@ -3260,7 +3336,10 @@ def _coverage_check(workflow: str, status: str, conclusion: str | None, started_ "status": status, "conclusion": conclusion, "startedAt": started_at, - "checkSuite": {"workflowRun": {"workflow": {"name": workflow}}}, + "checkSuite": { + "createdAt": suite_created_at, + "workflowRun": {"workflow": {"name": workflow}}, + }, } @@ -3318,6 +3397,69 @@ def test_latest_coverage_evidence_index_prefers_queued_when_it_appears_first(): assert check_runs[latest_index]["status"] == "QUEUED" +def test_latest_coverage_evidence_index_ranks_cancelled_before_start_rerun_above_stale_success(): + """Cross-workflow sibling of the same-name cancelled-before-start regression. + + Same BUG as ``test_failed_status_checks_treats_cancelled_before_start_rerun_as_authoritative``, + exercised through ``latest_coverage_evidence_index``'s cross-workflow-name + comparison instead of ``latest_check_runs``' same-(workflow, name) one. An + older "Required OpenCode Review" success must not beat a newer "OpenCode + Review Dispatch" rerun that was cancelled before it ever started, once + both carry a ``checkSuite.createdAt``. + """ + check_runs = [ + _coverage_check( + "Required OpenCode Review", + "COMPLETED", + "SUCCESS", + "2026-08-24T01:00:00Z", + suite_created_at="2026-08-24T00:59:00Z", + ), + _coverage_check( + "OpenCode Review Dispatch", + "COMPLETED", + "CANCELLED", + None, + suite_created_at="2026-08-24T02:00:00Z", + ), + ] + + latest_index = sched.latest_coverage_evidence_index(check_runs) + + assert check_runs[latest_index]["conclusion"] == "CANCELLED" + + +def test_latest_coverage_evidence_index_prefers_newer_completed_run_over_older_queued_run(): + """A genuinely newer completed rerun still outranks an older queued run. + + Devin's fix framing's second scenario: with ``checkSuite.createdAt`` + available for both candidates (as real GitHub responses always provide), + the fold must rank by that real creation time rather than blindly + presuming "queued always means newest" -- otherwise an older queued run + could wrongly out-rank a newer, already-completed result. + """ + check_runs = [ + _coverage_check( + "OpenCode Review Dispatch", + "QUEUED", + None, + None, + suite_created_at="2026-08-24T01:00:00Z", + ), + _coverage_check( + "Required OpenCode Review", + "COMPLETED", + "SUCCESS", + "2026-08-24T02:05:00Z", + suite_created_at="2026-08-24T02:00:00Z", + ), + ] + + latest_index = sched.latest_coverage_evidence_index(check_runs) + + assert check_runs[latest_index]["conclusion"] == "SUCCESS" + + def test_central_coverage_placeholder_cannot_mask_dispatch_failure(monkeypatch): """Central metadata-only coverage success cannot hide failed dispatch evidence.""" monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github")