fix(scheduler): require independent exact-head approval - #1270
fix(scheduler): require independent exact-head approval#1270seonghobae wants to merge 17 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughPR Review Merge Scheduler가 현재 HEAD의 독립 승인과 GitHub aggregate 승인을 병합 조건으로 적용합니다. 최신 CheckRun과 Coverage 실행을 판별하고, 조건 충족 후 OpenCode 리뷰를 재디스패치합니다. Changes병합 스케줄러 정책
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to The scheduler strengthens exact-head independent approval, but two merge paths can still retain or enable auto-merge after approval becomes stale or revoked. That could allow a protected merge without the required current independent authorization, so the PR is not ready to merge until those transitions are addressed. Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant GitHubAPI
participant CoverageEvidence
participant OpenCodeWorkflow
Scheduler->>GitHubAPI: 현재 PR, 리뷰, head 및 CheckRun 조회
GitHubAPI-->>Scheduler: 승인 상태와 검사 결과
Scheduler->>CoverageEvidence: 최신 coverage evidence 확인
CoverageEvidence-->>Scheduler: Coverage 상태
Scheduler->>OpenCodeWorkflow: 조건 충족 시 현재 HEAD 리뷰 재디스패치
OpenCodeWorkflow-->>GitHubAPI: OpenCode CheckRun 생성
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@opencode-agent Please review the exact current head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent Please review exact head |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent review Review exact current head 6cb7144 against protected main@0c6b9a6459c9dbdf5e23fb01df7a32a8a14964b3. Walk through the scheduler approval, exact-head binding, stale-review handling, and failure evidence; publish a substantive Reviews API verdict on this SHA. |
|
Exact-head local verification for 6cb7144: full pytest passed (1410 passed, 1 skipped, 16 subtests); coverage.py statement and branch coverage both 100% across scripts/ci; interrogate public docstring gate passed for scripts/ci/pr_review_merge_scheduler.py; compileall and git diff --check passed. No source change was needed after review. Hosted Checks and an independent exact-head approval remain required. |
|
@opencode-agent review Exact head: 6cb7144. Review the complete changed diff and report substantive findings with file/line evidence. |
|
Current exact-head Strix RCA: the required Strix run reached the scan gate, emitted zero vulnerabilities, but produced no authoritative report artifact. The log records NVIDIA NIM provider exhaustion/rate limiting followed by the configured openai-direct fallback returning HTTP 404. This is typed provider/backend infrastructure failure and fail-closed behavior, not a source finding. Next action: rerun the same-head Strix evidence after provider availability is restored; do not merge until the exact-head security evidence is complete. |
Exact-head review follow-up\n\nCurrent exact head: 6cb7144\n\nThe Strix provider rerun is now executing for this head after the earlier provider/backend failure. Please review the full scheduler diff and publish a substantive exact-head verdict only after the current required Checks complete. No predecessor success or review evidence is being reused. |
Exact-head verification receipt
No source, workflow, ruleset, protected-branch, or bypass mutation was used. |
|
Exact-head changed by the remote stack maintainer; predecessor evidence is discarded. Current exact head: #1266 was merged into this branch through the normal PR path, so the combined scheduler tree requires a fresh full review and hosted verification. @opencode-agent please inspect the complete current diff, exact-head dispatch-history retry floor, central active-run boundary, and the merged coverage-retry behavior, then publish a substantive Reviews API verdict tied only to this SHA. Do not reuse predecessor reviews or checks. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact current-head remediation on c55eb40: fixed failed_status_checks so a newer successful coverage-evidence run supersedes an older failed run in both normal merge and coverage-only retry paths. Reused one UTC-aware datetime.min fallback in latest_check_runs. Focused scheduler suite: 146 passed; git diff --check passed. The branch remains protected with no current formal approval. Please issue a fresh independent substantive review and required checks for this exact head; predecessor evidence is not reused. No bypass or gate weakening was used. |
|
Correction: exact current-head remediation on c55eb40. Fixed failed_status_checks so a newer successful coverage-evidence run supersedes an older failed run in both normal merge and coverage-only retry paths. Reused one UTC-aware datetime.min fallback in latest_check_runs. Focused scheduler suite: 146 passed; git diff --check passed. The branch remains protected with no current formal approval. Please issue a fresh independent substantive review and required checks for this exact head; predecessor evidence is not reused. No bypass or gate weakening was used. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head local verification completed for c55eb40: full pytest 1429 passed, 1 skipped, 16 subtests; coverage branch and statement 100 percent across scripts/ci (8499 statements, 3300 branches); interrogate docstring coverage 100 percent; Ruff lint passed; compileall and git diff --check passed. Ruff format check still reports pre-existing formatting drift in the two full files, so no unrelated reformat was introduced. Hosted required checks and independent approval remain pending; normal merge only. |
|
Exact-head follow-up on 8423ff1: fixed the valid central coverage authority gap. When SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY is set, metadata-only Required OpenCode Review coverage-evidence no longer supersedes a failed OpenCode Review Dispatch run; retry and failed-check decisions use authoritative dispatch evidence while ordinary same-workflow reruns still supersede older failures. Added regression coverage and changelog entry. Verification: full pytest 1430 passed, 1 skipped, 16 subtests; statement and branch coverage 100 percent across scripts/ci (8504 statements, 3302 branches); interrogate docstring coverage 100 percent; Ruff lint, compileall, and git diff --check passed. The branch remains protected with no independent current-head approval. Please issue a fresh substantive review and required hosted checks for this exact head; no bypass or gate weakening was used. |
|
Disposition for the two exact-head review threads on 8423ff1: the cross-workflow coverage finding was valid and is fixed by excluding the central Required OpenCode Review metadata placeholder from authoritative coverage superseding; the new regression proves a newer placeholder success cannot mask failed OpenCode Review Dispatch evidence. The independent-approval observation is intentionally fail-closed: GitHub App and bot identities are not independent human approval under this scheduler policy. Both findings are now dispositioned; no source uncertainty remains from these threads. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolved the stale merge conflict against
|
| 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") |
There was a problem hiding this comment.
📝 Info: Coverage retry keeps sibling blockers
ignore_opencode suppresses only OpenCode’s prior result. Failed sibling jobs and unrelated status contexts still block the same-head retry.
Was this helpful? React with 👍 or 👎 to provide feedback.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Fixed: PR review data was silently truncated past 100 reviews in both fetch pathsThe bug (as confirmed)
Once a PR passed 100 total review events, an early genuine independent Call-graph finding (step 1)I traced Checking What changed
Regression tests added (
|
There was a problem hiding this comment.
📝 Info: REST fallback remains fail-closed
The REST path assigns REVIEW_REQUIRED because it lacks authoritative aggregate approval. GraphQL outages therefore pause merges rather than weakening authorization.
Was this helpful? React with 👍 or 👎 to provide feedback.
| nodes = list(page.get("nodes") or []) + nodes | ||
| page_info = page.get("pageInfo") or {} |
There was a problem hiding this comment.
📝 Info: Pagination preserves review ordering
Older GraphQL pages are prepended to the newest window. Reverse scans still evaluate each reviewer’s latest exact-head policy state first.
Was this helpful? React with 👍 or 👎 to provide feedback.
| seen_reviewers.add(reviewer) | ||
| if state == "APPROVED": | ||
| return True |
There was a problem hiding this comment.
📝 Info: Revocations suppress earlier approvals
A reviewer’s latest exact-head dismissal or change request enters seen_reviewers without authorizing. Older approvals from that reviewer cannot resurface.
Was this helpful? React with 👍 or 👎 to provide feedback.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Pushed a follow-up commit ( Bug A — queued coverage rerun losing to a stale completed run
New regression test
The existing tests that specifically pin "time, not rollup order" behavior when both runs have real timestamps ( Bug B — unapproved PRs retaining a stale auto-merge arm through the branch-update path
Fix: inside that New regression test
I also updated the existing tests in VerificationFrom
Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…scade
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Fixed the remaining gap Devin flagged after the previous fix on this PR ("although a separate clean-PR path remains affected"). The unguarded code pathIn
Only a final catch-all, reached solely once every one of those branches had nothing left to do, checked and disarmed auto-merge. So an unapproved, The fixHoisted a single unconditional check, The now-unreachable check in the final catch-all was removed (it is dead code once the hoist runs first for every unapproved+armed case at that point in the function) to avoid an uncovered branch under the 100%-coverage gate. The Regression test evidenceBefore the fix (reproduced first): A After the fix: same test now passes — the scheduler returns Also added Full verification (from the fix commit)Pushed as Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Fixed: cross-workflow coverage-evidence rerun ordering (commit
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ffix
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.
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.
|
Triaged and fixed 2 more CodeRabbit findings on top of Finding 1 — GraphQL bot actors can omit the
|
| 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 {} | ||
| ) | ||
| return workflow.get("name") == "Required OpenCode Review" |
There was a problem hiding this comment.
📝 Info: Placeholder exclusion is narrowly scoped
Central mode ignores only Required OpenCode Review’s coverage-evidence job. Dispatch evidence and failed sibling jobs remain authoritative.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
Fixed the non-transitive coverage-evidence fold Devin flaggedConfirmed the bug by reading the current code (as of Root cause
The specific asymmetry: when the running "best so far" had no
Final answer: the stale 01:00 run — neither the actual newest timestamped run (02:00) nor the correctly-pending run. Reproduced both of Devin's named scenarios exactly (see below). FixReplaced the pairwise fold with a single derived recency key,
Why this is provably transitive: Python tuple comparison is a valid total order (lexicographic on Regression tests added (both of Devin's named scenarios)
Before the fix (confirmed by running the new tests against the unfixed code): both failed —
After the fix: both pass, along with every pre-existing test. Verification
Also confirmed Pushed as Generated by Claude Code |
| 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" | ||
| ), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
📝 Info: Approval loss disarms queued merges
Missing aggregate or independent approval disables existing auto-merge before later wait paths return. Unapproved heads receive the same protection before review dispatch.
Was this helpful? React with 👍 or 👎 to provide feedback.
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Fixed: revoked approval could still authorize merge (TOCTOU) — commit
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 80c7f38). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Fixed: canceled reruns inheriting stale success — commit
|
| 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) |
There was a problem hiding this comment.
📝 Info: Fresh approval guards merge mutations
Each merge branch re-fetches approval before mutation. The fresh check covers head movement, OpenCode revocation, aggregate policy, and independent approval.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Customer outcome
The organization merge scheduler refuses direct merge and auto-merge until GitHub reports
reviewDecision: APPROVEDand a non-author, non-OpenCode formal approval is bound to the exact live head. If stale authorization left auto-merge armed, the scheduler disables it.Root cause and minimal repair
PR #828 identified a real separation-of-duties gap but its old-base implementation replayed stale files and split the mature scheduler into a second core. This clean replacement reuses the existing scheduler and adds the shared authorization boundary, exact PR-author evidence, regressions, changelog, and APA 7 doctoring.
The current head also repairs check-run chronology: canceled-before-start and queued reruns use their non-null check-suite creation time, so a predecessor success cannot outrank a newer terminal or pending rerun merely because
startedAtis absent.The independent-review helper considers only each eligible reviewer's latest exact-head approval-affecting state. A later same-head change request or dismissal therefore revokes that reviewer's earlier approval. Generic
github-actions[bot], missing identities, self-review, comment-only review, and predecessor-head approval remain fail closed. GitHub remains authoritative for last-pusher, CODEOWNERS, required-review, and ruleset policy.Exact current-head evidence
63cd827f8a66ba36c0bc0af61e0830fbab0622a2main@1d8e872487838e16a003e96e76df9300c388e2581ff8268255b061461d9d49b4cab4febf9a8e7bfaCHANGELOG.md,docs/doctoring/scheduler-independent-current-head-approval.md,scripts/ci/pr_review_merge_scheduler.py, andtests/test_pr_review_merge_scheduler.py.9737188756is bound to this head with digestsha256:aa8bc70e4bdc3da73216911da419968e778963ca6c4619a295299da317816e4d.APPROVEDorCHANGES_REQUESTEDOpenCode verdict.Current head does not have a primary OpenCode approval; Noema review skippedbut concluded GitHub-success. This is the known fail-closed control defect owned by fix(security): repair pip audit and schedule orchestrator review #1198 and is non-passing evidence.Infothreads; they remain unresolved and are therefore non-passing under the live thread-resolution contract.Merge boundary
Do not transfer review or check evidence from #828 or any predecessor head. This exact head remains unmergeable under its own separation-of-duties contract until required Noema is fail-closed, a qualifying current-head OpenCode verdict exists, and an independent exact-head formal approval is present. Project #1 remains
In Progress.Decision: WAIT_FOR_EXACT_HEAD_FORMAL_VERDICTS_AND_INDEPENDENT_APPROVAL.