Skip to content

fix(scheduler): require independent exact-head approval - #1270

Merged
seonghobae merged 18 commits into
mainfrom
codex/scheduler-independent-exact-head-approval-20260824
Aug 31, 2026
Merged

fix(scheduler): require independent exact-head approval#1270
seonghobae merged 18 commits into
mainfrom
codex/scheduler-independent-exact-head-approval-20260824

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Customer outcome

The organization merge scheduler refuses direct merge and auto-merge until GitHub reports reviewDecision: APPROVED and 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 startedAt is 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

  • Head: 63cd827f8a66ba36c0bc0af61e0830fbab0622a2
  • Live PR base: main@1d8e872487838e16a003e96e76df9300c388e258
  • Protected central main at revalidation: 1ff8268255b061461d9d49b4cab4febf9a8e7bfa
  • Current changed files: CHANGELOG.md, docs/doctoring/scheduler-independent-current-head-approval.md, scripts/ci/pr_review_merge_scheduler.py, and tests/test_pr_review_merge_scheduler.py.
  • Exact-head Security Scan, SAST, CodeQL, OSV, SBOM, Python Security, Scorecard, Secret Scan, organization quality, coverage quality, artifact attestation, and changed-path Strix quality are terminal success.
  • Required Strix run 33328950520 completed authoritative analysis on this exact head and reported zero exploitable vulnerabilities. Artifact 9737188756 is bound to this head with digest sha256:aa8bc70e4bdc3da73216911da419968e778963ca6c4619a295299da317816e4d.
  • Required OpenCode run 33328950514 correctly failed because the Reviews API has no exact-current-head APPROVED or CHANGES_REQUESTED OpenCode verdict.
  • Required Noema run 33328950526 emitted Current head does not have a primary OpenCode approval; Noema review skipped but concluded GitHub-success. This is the known fail-closed control defect owned by fix(review): fail closed before every Noema skip path #1198 and is non-passing evidence.
  • Current unresolved review thread count: 7. All seven are current, non-outdated Devin Info threads; they remain unresolved and are therefore non-passing under the live thread-resolution contract.
  • Exact-current-head qualifying independent formal approval count: 0.

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.


Open in Devin Review

Summary by CodeRabbit

  • 변경 사항

    • 병합 및 자동 병합 전에 전체 승인 상태와 현재 커밋에 대한 유효한 독립 승인을 모두 확인합니다.
    • 오래된 승인, 작성자 승인, 댓글 기반 승인은 병합 승인으로 인정하지 않습니다.
    • 충돌, 실패한 검사, 미해결 스레드 및 커버리지 문제 발생 시 안전하게 병합을 차단합니다.
    • 커버리지 문제가 해결되면 최신 커밋 기준으로 검증과 리뷰를 다시 실행합니다.
    • 초안 변경은 정확한 변경 요청과 커밋이 확인된 경우에만 자동 리뷰를 요청합니다.
  • 문서

    • 새로운 승인 정책과 검증·운영 절차를 문서화했습니다.

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4edb0ad1-8262-4cc5-ac61-7e06696c1315

📥 Commits

Reviewing files that changed from the base of the PR and between 80c7f38 and 98aa3a1.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • scripts/ci/pr_review_merge_scheduler.py
  • tests/test_pr_review_merge_scheduler.py

📝 Walkthrough

Walkthrough

PR Review Merge Scheduler가 현재 HEAD의 aggregate 승인과 독립 승인을 병합 조건으로 적용합니다. 최신 CheckRun과 Coverage 실행을 판별합니다. 초안 PR에는 검증된 artifact가 있을 때만 review-only 리뷰를 디스패치합니다.

Changes

병합 스케줄러 정책

Layer / File(s) Summary
리뷰 이력 및 최신 검사 선택
scripts/ci/pr_review_merge_scheduler.py, tests/test_pr_review_merge_scheduler.py
GraphQL과 REST 리뷰 이력을 전체 페이지에서 수집합니다. CheckSuite 시각과 CheckRun 시도를 사용해 최신 결과를 선택합니다.
현재 HEAD 승인 및 병합 게이트
scripts/ci/pr_review_merge_scheduler.py, tests/test_pr_review_merge_scheduler.py, docs/doctoring/scheduler-independent-current-head-approval.md
reviewDecisionAPPROVED이고 비작성자·비-OpenCode 주체의 exact-head formal approval이 있을 때만 병합합니다. 병합과 auto-merge 전에 상태를 재검증합니다.
Coverage 재시도 및 초안 리뷰 흐름
scripts/ci/pr_review_merge_scheduler.py, tests/test_pr_review_merge_scheduler.py, CHANGELOG.md
권위 있는 Coverage evidence를 선택하고, Coverage 전용 blocker가 해소되면 현재 HEAD에 리뷰를 재디스패치합니다. 검증된 artifact 또는 명시적 단일 PR 옵션이 있을 때만 초안 PR에 review-only 흐름을 사용합니다.
정책 검증 및 변경 기록
tests/test_pr_review_merge_scheduler.py, docs/doctoring/scheduler-independent-current-head-approval.md, CHANGELOG.md
pagination, 승인 철회, stale head, 충돌, 실패 검사, branch update 및 auto-merge 해제 동작을 검증하고 변경 사항을 기록합니다.

Estimated code review effort: 5 (Critical) | ~120 minutes

Merge Risk: 🟠 High · up to 80c7f

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 ReviewWorkflow
  Scheduler->>GitHubAPI: PR, head, reviews, checks 조회
  GitHubAPI-->>Scheduler: 승인 상태와 검사 결과
  Scheduler->>CoverageEvidence: 최신 Coverage evidence 확인
  CoverageEvidence-->>Scheduler: Coverage 상태와 재시도 시각
  Scheduler->>ReviewWorkflow: 현재 head 리뷰 디스패치
  ReviewWorkflow-->>GitHubAPI: Strix 및 OpenCode CheckRun 생성
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.12% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 138 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 스케줄러가 독립적인 exact-head 승인을 요구하도록 변경한 PR의 핵심 내용을 정확하고 간결하게 설명합니다.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/scheduler-independent-exact-head-approval-20260824

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please review the exact current head 5da3a5b0c1f0b48011d7b81ccc721f0a55c6f455. This is the protected-main replacement for stale PR #828: it reuses the existing scheduler, requires aggregate approval plus an independent non-author, non-bot exact-head approval, guards both direct merge and auto-merge, and disarms already-armed auto-merge when authorization is absent. Local exact evidence: 126 focused tests; 1,405 passed/1 skipped; 8,370 statements + 3,230 branches at 100%; docstrings 100%.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please review exact head aa0c93e5d461daf64c160b91066f90bad57a532f. Verify the independent-approval helper uses each eligible reviewer’s latest exact-head approval-affecting state, including that a later same-head CHANGES_REQUESTED or DISMISSED state revokes an earlier approval. Also re-evaluate direct merge, auto-merge disarm, repository reviewDecision, tests, coverage, doctoring, and changelog without transferring evidence from #828 or predecessor head 5da3a5b0.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae enabled auto-merge (squash) August 24, 2026 04:02
devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 24, 2026 04:36
@seonghobae
seonghobae enabled auto-merge (squash) August 24, 2026 04:45
@seonghobae

Copy link
Copy Markdown
Contributor Author

@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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review Exact head: 6cb7144. Review the complete changed diff and report substantive findings with file/line evidence.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head verification receipt

  • Exact head: 6cb714470c5a0c31b36cb64df8959cd020f22d52
  • Exact base: 0c6b9a6459c9dbdf5e23fb01df7a32a8a14964b3
  • Verified merge-result tree: ed152dd8264301552e763a733e0ede848222746a (equal to exact head tree)
  • Focused scheduler/queue regression: 191 passed
  • Full suite: 1410 passed, 1 skipped, 16 subtests
  • Production statement and branch coverage: 100%
  • Public docstrings: 100%
  • Ruff, Python compile, diff check, and interrogate: passed
  • Hosted current evidence: CodeQL, coverage, dependency, OSV, Noema, scheduler, and Trivy checks passed; Strix failure is the known provider-only NIM/OpenAI fallback failure, and current OpenCode review remains pending.
  • Decision: WAIT_AND_REMEDIATE until current-head review/Strix gates are terminal; normal auto-merge remains enabled.

No source, workflow, ruleset, protected-branch, or bypass mutation was used.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head changed by the remote stack maintainer; predecessor evidence is discarded.

Current exact head: ccc1220cd7710d023d4f10d79ccd170a636b4516
Current exact base: 0c6b9a6459c9dbdf5e23fb01df7a32a8a14964b3

#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.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@seonghobae

seonghobae commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

@seonghobae

Copy link
Copy Markdown
Contributor Author

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.

devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 24, 2026 08:34
…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

Copy link
Copy Markdown
Contributor Author

Fixed the remaining gap Devin flagged after the previous fix on this PR ("although a separate clean-PR path remains affected").

The unguarded code path

In inspect_pr() (scripts/ci/pr_review_merge_scheduler.py), once current_head_approved is False and the branch is not behind base (behind_by falsy, so neither behind_by-based disarm block applies), execution could fall through several wait/dispatch branches that never checked pr.get("autoMergeRequest") before returning:

  • the OpenCode-running wait (opencode_state == "running")
  • the workflow_run deterministic-fallback wait
  • the stale-OpenCode retry dispatch (opencode_state == "stale")
  • the ordinary Strix/OpenCode dispatch cascade under trigger_reviews (missing/running Strix evidence, OpenCode dispatch) — the everyday state for a PR between or during reviews

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, CLEAN-mergeability PR with auto-merge armed (from a stale/invalidated prior approval, or armed early by a human before any review ran) that was actively going through its normal review cycle would return a plain security_dispatch/wait decision with auto-merge left queued. If GitHub's own required checks don't 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 — the exact bypass this PR exists to prevent.

The fix

Hoisted a single unconditional check, if not current_head_approved and auto_merge_enabled:, immediately after the behind_by gate and the last-push-approval restamp check (both of which only fire when current_head_approved is True, so the hoist cannot affect the already-approved path) and before any of the wait/dispatch branches above. It calls the same disable_auto_merge_decision(...) the rest of the file already uses, with the same reason text the old catch-all used ("current head has no OpenCode approval; wait for fresh same-head approval before re-enabling auto-merge"). Consistent with the existing behind_by disarm path and the doctoring doc ("an already armed auto-merge request is disabled when authorization is absent"), no distinction is drawn between "armed before any review ran" and "armed on a now-stale approval" — the scheduler disarms unconditionally in both cases, matching precedent already in the file.

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 merge_state == "UNKNOWN" branch's own auto-merge check is untouched and remains live for its one still-reachable case: an approved PR whose mergeability is still being calculated (verified with a new test, see below).

Regression test evidence

Before the fix (reproduced first):

FAILED tests/test_pr_review_merge_scheduler.py::test_clean_unapproved_armed_pr_disarms_auto_merge_before_review_dispatch
AssertionError: assert 'security_dispatch' == 'disable_auto_merge'

A CLEAN, unapproved PR with autoMergeRequest armed and no Strix evidence returned security_dispatch, leaving auto-merge queued.

After the fix: same test now passes — the scheduler returns disable_auto_merge instead.

Also added test_approved_unknown_mergeability_disarms_auto_merge_pending_evaluation to keep the merge_state == "UNKNOWN" branch's approved-only auto-merge check covered now that the unapproved case no longer reaches it, and updated the existing unapproved+UNKNOWN-mergeability assertion in test_inspect_pr_blocks_and_waits_for_policy_states to the new, more accurate "current head has no OpenCode approval" reason text (the action, disable_auto_merge, is unchanged).

Full verification (from the fix commit)

python3 -m pytest tests/ -q
→ 1949 passed, 1 skipped, 21 subtests passed

coverage run -m pytest tests && coverage report --show-missing
→ scripts/ci/pr_review_merge_scheduler.py: 1952 stmts, 0 missed, 100%
→ TOTAL: 10167 stmts, 0 missed, 100%

interrogate
→ RESULT: PASSED (minimum: 100.0%, actual: 100.0%)

python3 -m ruff check scripts/ci/pr_review_merge_scheduler.py tests/test_pr_review_merge_scheduler.py
→ 6 pre-existing findings, all unrelated to this diff and present on the unmodified branch head (verified via git stash)

git diff --check
→ clean

Pushed as 6370b320 on top of 731e427b.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

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

Copy link
Copy Markdown
Contributor Author

Fixed: cross-workflow coverage-evidence rerun ordering (commit 39f4ae65)

Fixed the sibling of the latest_check_runs() queued-check-ordering bug (731e427b), this time in latest_coverage_evidence_index().

The bug

latest_coverage_evidence_index() picked the newest coverage-evidence check run across workflow names using max(coverage_indices, key=lambda item: (parse_github_datetime(...) or datetime.min, item)). GitHub leaves CheckRun.startedAt null while a check is QUEUED — that null timestamp sorted as datetime.min (oldest possible), so a freshly dispatched coverage-evidence rerun in one workflow could lose the max() comparison to an older, already-completed coverage-evidence run in a different workflow. Downstream, coverage_evidence_state(pr) would then report stale "complete" from the wrong run instead of "running", potentially authorizing an OpenCode redispatch decision (coverage_retry_wait_reason) before the real, current coverage result was known. Exact same class of bug as the just-landed latest_check_runs() fix — just triggered across workflow names instead of within one (workflow, name) key.

The fix

Mirrors the earlier fix's design exactly, via a shared helper rather than a duplicated inline check:

  • Extracted the recency rule latest_check_runs() introduced (defer to the check run's own pending status via running_check_state() only when a timestamp comparison is impossible, otherwise compare timestamps, with index as the final tiebreaker) into a new check_run_supersedes(started_at, node, index, previous_started_at, previous_index) -> bool helper.
  • latest_check_runs() now calls this helper instead of the inline three-branch comparison it previously had — no behavioral change, so its existing tests pass unmodified.
  • latest_coverage_evidence_index() now folds its candidate indices through the same helper (replacing the max()-with-timestamp-key implementation), so a genuinely newer but still-QUEUED coverage-evidence run in one workflow correctly outranks an older completed run in a different workflow, while a null-timestamp entry with no pending status still falls back to timestamp ordering.

Regression test (reproduced first, then fixed)

Added test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run_across_workflows, sibling to the existing same-workflow test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run: an older, completed SUCCESS coverage-evidence run from "Required OpenCode Review" vs. a newer QUEUED (startedAt: null) coverage-evidence run from "OpenCode Review Dispatch" (both workflow-name conventions already used elsewhere in this test file).

  • Before the fix: test failed — assert check_runs[latest_index]["status"] == "QUEUED"AssertionError: assert 'COMPLETED' == 'QUEUED'.
  • After the fix: test passes — the queued run is correctly selected as authoritative and coverage_evidence_state(pr) == "running".

Full verification (from the isolated worktree, on top of 6370b320)

python3 -m pytest tests/ -q
  1950 passed, 1 skipped, 21 subtests passed

coverage run -m pytest tests -q && coverage report
  scripts/ci/pr_review_merge_scheduler.py   1959 stmts, 0 missed, 822 branches, 0 partial  100%
  TOTAL                                     10174 stmts, 0 missed, 4034 branches, 0 partial  100%

interrogate
  RESULT: PASSED (minimum: 100.0%, actual: 100.0%)

python3 -m ruff check scripts/ci/pr_review_merge_scheduler.py tests/test_pr_review_merge_scheduler.py
  6 pre-existing findings, unchanged in count/content before and after this diff (verified via git stash);
  none introduced by this change

git diff --check
  clean

Pushed to codex/scheduler-independent-exact-head-approval-20260824 at 39f4ae65 (on top of 6370b320).


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

claude added 2 commits August 30, 2026 17:43
…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.

Copy link
Copy Markdown
Contributor Author

Triaged and fixed 2 more CodeRabbit findings on top of 39f4ae65. Both confirmed real; commits 92d3afad and 7ac1ed3b (pushed to this branch, current head 7ac1ed3b).

Finding 1 — GraphQL bot actors can omit the [bot] login suffix — FIXED

Empirical verification (this environment's gh/GraphQL access turned out to be sandbox-restricted to a pinned operation set, so I verified against this org's own real data instead via the GitHub MCP tools):

Called pull_request_read on this PR (#1270) through two different API surfaces for the same known bot accounts:

  • get_reviews (REST, user.login): "login":"coderabbitai[bot]", "login":"devin-ai-integration[bot]" — suffix present, as expected.
  • get_review_comments (GraphQL-backed — thread IDs are PRRT_..., GraphQL-only node IDs): the same accounts came back as "author":"coderabbitai" and "author":"devin-ai-integration"no [bot] suffix.

That's a direct, live confirmation of CodeRabbit's claim using real production data from this repo, not a hypothetical. has_independent_current_head_approval's reviewer.endswith("[bot]") check (added in an earlier fix on this same PR, commit b748ccbf, for a different CodeRabbit finding) is therefore incomplete: a GraphQL-fetched bot review whose login happens to omit the suffix would count as an "independent" human approval — an authorization-bypass-adjacent bug in the exact separation-of-duties gate this PR exists to add.

Fix:

  • Added __typename to the author field selection in both review-fetching GraphQL queries (PULL_REQUEST_FIELDS_FRAGMENT's reviews block and PR_REVIEWS_PAGE_QUERY).
  • Added is_bot_review_author(): excludes a review when its login ends with [bot] or its author.__typename == "Bot". Kept the suffix check (REST's rest_review_node never sets __typename, so REST reviews still rely on it).
  • Wired is_bot_review_author() into has_independent_current_head_approval in place of the bare suffix check.

Before/after regression evidence: New tests (test_scheduler_review_queries_request_bot_typename, test_independent_approval_excludes_graphql_bot_actor_missing_suffix, test_is_bot_review_author_covers_suffix_and_typename_and_neither) were run against the pre-fix code first and failed as expected (missing __typename in the fragment/query; AttributeError: module has no attribute 'is_bot_review_author'). After the fix, all three pass.

Finding 2 — unbounded pagination in the same-head OpenCode dispatch-history lookup — FIXED

Verification: Read active_workflow_runs — it calls gh api ... --paginate --slurp with no depth limit and no event/created query params; the one place a caller narrows results is entirely client-side, after the full fetch. latest_opencode_dispatch_started_at calls it as active_workflow_runs(dispatch_repo, ("completed",)) (line ~2503 pre-fix) with zero filtering, then matches on event, a display-title prefix, and exact head SHA — all client-side. Confirmed this is live, reachable code: coverage_retry_wait_reasonlatest_opencode_dispatch_started_at is called directly from inspect_pr's main scheduler path (not dead/test-only), and a RuntimeError there does produce exactly the "same-head OpenCode dispatch history is unavailable" wait reason CodeRabbit described. GitHub's "List workflow runs for a repository" REST endpoint does document both event and created query parameters.

Fix:

  • active_workflow_runs gained optional keyword-only event/created params that add matching -f query args to the gh api call. Omitted by default, so every other caller (there are 4 call sites total) keeps its exact prior unfiltered request — only this one call site needed bounding.
  • latest_opencode_dispatch_started_at gained an optional since: datetime | None param; when given, it requests event=repository_dispatch and created=>=<since> server-side. Its only caller (coverage_retry_wait_reason) passes the coverage-request review's own submittedAt as since — this is exact, not a heuristic window: a dispatch run created at or before that timestamp can never become the function's returned maximum (the caller only acts on dispatch_started_at > retry_anchor, and retry_anchor starts at that same submittedAt), so narrowing the query this way cannot change which run is selected.

Before/after regression evidence: New tests assert the constructed gh api args list contains event=repository_dispatch and created=>=2026-08-24T00:00:00Z for a bounded call, that omitting the new kwargs produces no event=/created= args at all (unfiltered callers unaffected), and that since forwards correctly from latest_opencode_dispatch_started_at into active_workflow_runs. Three pre-existing tests that monkeypatched the old 2-arg/3-arg signatures were updated to accept the new keyword-only params (no behavioral change to those tests, just signature compatibility).

Full verification (from the fixed worktree, both commits applied)

python3 -m pytest tests/ -q            → 1957 passed, 1 skipped, 21 subtests passed
coverage run -m pytest tests -q        → 1957 passed, 1 skipped, 21 subtests passed
coverage report --show-missing         → scripts/ci/pr_review_merge_scheduler.py: 1969 stmts / 828 branches, 100%; TOTAL 100%
interrogate                            → RESULT: PASSED (minimum: 100.0%, actual: 100.0%)
ruff check scripts/ci/pr_review_merge_scheduler.py tests/test_pr_review_merge_scheduler.py
                                        → 6 findings, all pre-existing on unmodified 39f4ae65 (confirmed by diffing
                                          finding locations against the original file — only line numbers shifted);
                                          zero new findings introduced
git diff --check                       → clean

Both findings were real; no false positives to report this time.


Generated by Claude Code

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 3 new potential issues.

Devin Review

Comment thread scripts/ci/pr_review_merge_scheduler.py Outdated
Comment on lines +1595 to +1605
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"

@devin-ai-integration devin-ai-integration Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread scripts/ci/pr_review_merge_scheduler.py
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.

Copy link
Copy Markdown
Contributor Author

Fixed the non-transitive coverage-evidence fold Devin flagged

Confirmed the bug by reading the current code (as of 7ac1ed3b) and reproducing it before touching anything.

Root cause

check_run_supersedes(started_at, node, index, previous_started_at, previous_index) was a pairwise "does the new candidate beat the current best" predicate. latest_check_runs folded it per (workflow, name) key (fine — same-key reruns only ever have 2 live candidates in practice), but latest_coverage_evidence_index folded the same predicate left-to-right across every coverage-evidence check run regardless of workflow name, treating a non-transitive relation as if it were a valid total order.

The specific asymmetry: when the running "best so far" had no startedAt (a genuinely QUEUED rerun), the branch if previous_started_at is None and started_at is not None: return True let any subsequently-folded timestamped candidate override it unconditionally — without checking whether that untimed "best" was itself a presumed-newest pending run. So:

  • (1) completed@02:00 → best = 02:00
  • (2) queued(startedAt=null) supersedes 02:00 (correct in isolation — a pending rerun is presumed newer than anything already resolved) → best = queued
  • (3) completed@01:00 unconditionally supersedes the untimed "best" (queued) purely because it has a real timestamp → best = 01:00

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).

Fix

Replaced the pairwise fold with a single derived recency key, check_run_recency_key(node, started_at, index) -> (bucket, timestamp_or_epoch, index):

  • bucket 0 — no startedAt and not currently pending (e.g. cancelled before it started): no recency signal, sorts lowest.
  • bucket 1 — a real startedAt: ranked by that timestamp.
  • bucket 2 — no startedAt yet but actively pending (running_check_state(node) == "running", i.e. QUEUED/IN_PROGRESS/etc): GitHub only ever creates such a row after any run it might supersede, so it's presumed newer than every already-resolved run, regardless of that run's timestamp.
  • Ties within a bucket fall back to the later index (same tie-break the old code documented).

latest_check_runs now keeps (recency_key, node) per key and replaces the stored value only when the new key is >= the stored one. latest_coverage_evidence_index now just does max(coverage_indices, key=lambda i: check_run_recency_key(...)).

Why this is provably transitive: Python tuple comparison is a valid total order (lexicographic on </>=), and each candidate's key depends only on that candidate's own fields — never on "whatever happens to be the current best." That eliminates the exact failure mode above: there is no intermediate "best so far" state to lose information about (e.g. forgetting that the current best was itself a presumed-newest pending run once a later candidate is compared against it). max()/sorted() over these keys is order-independent and count-independent by construction — it is not an ad hoc pairwise rule being trusted to compose correctly across N candidates, it is a real order relation.

Regression tests added (both of Devin's named scenarios)

  1. test_latest_coverage_evidence_index_stays_transitive_across_three_candidates — exact 3-candidate order Devin described: completed@02:00, queued(null), completed@01:00 across three different workflow names. Asserts the stale 01:00 run never wins, and specifically that the queued run wins (consistent with the existing, already-locked-in precedent that a pending rerun is presumed newer than any already-resolved run — see test_coverage_evidence_state_prefers_queued_rerun_over_stale_completed_run_across_workflows).
  2. test_latest_coverage_evidence_index_prefers_queued_when_it_appears_first — the queued node placed before the completed node, verifying the queued-first ordering doesn't independently misbehave.

Before the fix (confirmed by running the new tests against the unfixed code): both failed —

  • Scenario 1: latest_index == 2 (the stale 01:00 run) instead of the queued run → AssertionError: the stale 01:00 completed run must never win the fold.
  • Scenario 2: check_runs[latest_index]["status"] == "COMPLETED" instead of "QUEUED".

After the fix: both pass, along with every pre-existing test.

Verification

python3 -m pytest tests/ -q
→ 1959 passed, 1 skipped, 21 subtests passed
coverage run -m pytest tests && coverage report --show-missing
→ scripts/ci/pr_review_merge_scheduler.py  1958 stmts  0 missing  822 branches  0 missing  100% cover
→ TOTAL 100%
interrogate
→ RESULT: PASSED (minimum: 100.0%, actual: 100.0%)
git diff --check
→ clean (no whitespace errors)

python3 -m ruff check scripts/ci/pr_review_merge_scheduler.py tests/test_pr_review_merge_scheduler.py reports 6 findings (unrelated shebang/import-sort/style nits), but I verified by stashing my change and re-running against the unmodified branch tip (7ac1ed3b) that these are the identical 6 pre-existing findings, none touching the lines I changed — there's no ruff.toml/pyproject.toml [tool.ruff] config in this repo and no CI workflow invokes ruff check, so this isn't one of the repo's enforced gates (those are pytest/coverage/interrogate per .github/CLAUDE.md). Left untouched to avoid unrelated diff churn against a file with exact-string contract tests.

Also confirmed check_run_supersedes had no other callers or doc references anywhere in the repo before removing it.

Pushed as 7ac1ed3b..e655e5ce on this branch (no force-push; branch was unchanged from 7ac1ed3b at push time).


Generated by Claude Code

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 2 new potential issues.

Devin Review

Comment thread scripts/ci/pr_review_merge_scheduler.py
Comment on lines 3140 to +3152
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"
),
)
)

@devin-ai-integration devin-ai-integration Bot Aug 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Devin Review

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

Copy link
Copy Markdown
Contributor Author

Fixed: revoked approval could still authorize merge (TOCTOU) — commit 80c7f382

Devin Review flagged a security-severity race on this head: a same-head approval revoked after approval_reason is computed can still authorize a merge.

The bug (confirmed by reading inspect_pr() in full)

current_head_approved = has_current_head_approval(pr) and approval_reason = merge_approval_block_reason(pr) if current_head_approved else None are computed once, early in inspect_pr(), from the GraphQL/REST pr snapshot this scheduler invocation fetched at the start of its run (fetch_open_prs/fetch_pr, called well before inspect_pr in main()). I traced every downstream path and found four mutating call sites that still consume that stale approval_reason/current_head_approved pair much later in the same function call:

  1. merge_state == "CLEAN" fast path → merge_pr(...) (direct/direct_or_auto)
  2. same fast path → enable_auto_merge(...) (plain auto mode)
  3. the other-mergeable-state path → merge_pr(...) (direct_or_auto)
  4. same path → enable_auto_merge(...) (plain auto mode)

Each site already has if approval_reason: return decide("wait", approval_reason) guarding it — but that check reads the same upfront value. If the independent reviewer dismisses/revokes their exact-head approval (or GitHub otherwise recomputes reviewDecision) in the window between the initial snapshot and the mutating call — realistically many seconds to low-minutes, since this scheduler processes many PRs and makes several API calls per PR — approval_reason is still None from the stale read, so the merge proceeds on authorization that no longer holds. The --match-head-commit guard those mutations already carry only protects against the commit changing in that window; it does nothing for the review state changing on the identical commit.

The fix

Added revalidate_current_head_approval(repo, pr): re-fetches the PR via the existing fetch_pr() helper and recomputes the exact same decision using the exact same helpers as the original snapshot (has_current_head_approval + merge_approval_block_reason — no duplicated logic). It fails closed on a re-fetch error, on the PR no longer being returned (closed/inaccessible), and — as defense in depth ahead of --match-head-commit — if the head itself moved between snapshot and re-check.

A small revalidate_before_merge() closure inside inspect_pr() calls it (skipped entirely when dry_run — nothing mutates in dry-run, so the extra fetch buys nothing and every existing dry-run-based test in this file keeps working unmodified) and is invoked immediately before each of the four call sites above, 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 just waits with the fresh reason. This composes with, and does not replace, the SHA guard — both must hold at mutation time now, not just at snapshot time.

TDD (reproduced first, confirmed failing pre-fix)

Stashed only the production-code change and ran the new tests against unmodified e655e5ce:

FAILED test_inspect_pr_direct_merge_blocked_when_approval_revoked_before_merge
FAILED test_inspect_pr_direct_or_auto_merge_blocked_when_approval_revoked_before_merge
FAILED test_inspect_pr_auto_merge_blocked_when_approval_revoked_before_enable
FAILED test_inspect_pr_disables_queued_auto_merge_when_approval_revoked_before_merge
FAILED test_inspect_pr_blocked_direct_or_auto_merge_blocked_when_approval_revoked_before_merge
FAILED test_inspect_pr_blocked_auto_merge_blocked_when_approval_revoked_before_enable
FAILED test_inspect_pr_direct_merge_proceeds_when_revalidation_confirms_approval
FAILED test_inspect_pr_fails_closed_when_revalidation_refetch_errors
FAILED test_revalidate_current_head_approval_* (7 unit tests — attribute doesn't exist yet)

i.e. pre-fix, mocking fetch_pr to return a freshly-revoked snapshot between the approved snapshot and the mutating call confirmed the scheduler would still call merge_pr/enable_auto_merge despite the revocation, across all four call sites (both CLEAN and BLOCKED mergeability × direct/direct_or_auto/auto merge modes). Restoring the fix: all pass.

New coverage also confirms: a still-valid re-check lets the merge proceed normally (fetch called, merge_pr called); a re-check API failure fails closed without merging; dry_run=True never pays for the extra fetch (asserted fetch_pr uncalled across every existing dry-run merge/auto_merge assertion in this file).

Full verification (isolated worktree, base e655e5ce, fetched immediately before push to confirm nothing else had moved the branch)

PYTHONPATH=. python3 -m pytest tests -q
→ 1975 passed, 1 skipped, 21 subtests passed

coverage run -m pytest tests && coverage report --show-missing
→ scripts/ci/pr_review_merge_scheduler.py   1998 stmts / 844 branches   100%
→ TOTAL                                     10213 stmts / 4056 branches 100%

interrogate
→ RESULT: PASSED (minimum: 100.0%, actual: 100.0%)

ruff check .
→ 1 finding (unused `typing.Any` import in an unrelated test file), byte-for-byte
  identical output on the unmodified base commit e655e5ce (confirmed via git stash) —
  zero new findings introduced by this diff

git diff --check
→ clean

No CHANGELOG.md/docs/doctoring entry was added, matching this session's established pattern for the last 6 rounds on this PR (2139aec1 through e655e5ce) — none of those touched CHANGELOG.md or docs/.

Pushed as 80c7f382 on codex/scheduler-independent-exact-head-approval-20260824 (fast-forward from e655e5ce; branch confirmed unmoved immediately before push).


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

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

Copy link
Copy Markdown
Contributor Author

Fixed: canceled reruns inheriting stale success — commit 63cd827f

Devin Review flagged a BUG on this head: check_run_recency_key (added earlier this session, e655e5ce, to fix the non-transitive coverage-evidence fold) gave a completed-with-no-startedAt row the lowest tier unconditionally — but GitHub reports a check run as completed/cancelled with startedAt: null when a queued rerun is canceled before it ever starts, not only for rows that never mattered. So a newer rerun canceled before starting could always lose to an older, already-completed SUCCESS row of the same (workflow, name) — the scheduler could merge on a stale success.

Confirmed and reproduced first

Reproduced both of Devin's named scenarios against pre-fix code before changing anything:

  • older SUCCESS@startedAt vs. newer canceled-before-start (startedAt: null) → pre-fix picks the stale SUCCESS (bug confirmed).
  • older QUEUED vs. newer completed rerun → pre-fix picks the older QUEUED (the "queued presumed newest" heuristic also has no real chronology to fall back on).

What I ruled out first

Devin's fix framing suggested "fetch a reliable creation or sequence signal." I checked what's actually queryable rather than assuming:

  • GraphQL CheckRun has no createdAt (verified against GitHub's public schema) — only startedAt/completedAt.
  • CheckRun.databaseId / CheckSuite.databaseId (Int) looked promising as a sequence signal, but I verified against a live query on this repo that real check-run/check-suite ids are already ~10^11 — well past the 32-bit Int range those fields are typed as. That's the exact reason GitHub has been migrating other databaseId: Int fields (PullRequest, Issue, ProjectV2*, etc.) to fullDatabaseId: BigInt in this same schema. CheckRun/CheckSuite haven't gotten that migration yet, so databaseId can't be trusted as an "always present" signal here — rejected.

The fix

CheckSuite.createdAt is a real, non-nullable DateTime! field, and CheckRun.checkSuite is itself non-nullable — so it's always populated, for every check run, regardless of how it resolved. GitHub sets it unconditionally the instant the triggering push/rerun/dispatch creates the check suite, strictly before any check run inside it can be queued, start, or be canceled before starting. check_run_recency_key now prefers checkSuite.createdAt over startedAt as the recency timestamp, falling back to the old startedAt/pending-tier heuristic only when a payload omits it (kept for backward compatibility with existing fixtures). This is a general fix, not a special case for conclusion == "cancelled": every check run is ranked by the earliest-available real creation timestamp.

Added createdAt to the GraphQL check-run fragment (checkSuite { createdAt ... }), and kept the REST fallback in parity: rest_pr_node now also fetches commits/{sha}/check-suites and joins each check run's checkSuite.createdAt by check_suite.id, threaded through rest_check_node.

Test evidence

  • test_failed_status_checks_treats_cancelled_before_start_rerun_as_authoritative (same-(workflow, name) case, via latest_check_runs) and test_latest_coverage_evidence_index_ranks_cancelled_before_start_rerun_above_stale_success (cross-workflow case, via latest_coverage_evidence_index) — both confirmed failing against pre-fix code, passing post-fix.
  • test_latest_coverage_evidence_index_prefers_newer_completed_run_over_older_queued_run locks in Devin's second scenario: with checkSuite.createdAt available, a genuinely newer completed rerun still outranks an older queued run (no regression of the "queued presumed newest" fallback).
  • All existing check_run_recency_key/latest_check_runs/latest_coverage_evidence_index tests — including the round-6 transitivity regressions (test_latest_coverage_evidence_index_stays_transitive_across_three_candidates, test_latest_coverage_evidence_index_prefers_queued_when_it_appears_first) — pass unmodified, since they don't set checkSuite.createdAt and hit the preserved fallback path unchanged.
  • test_rest_pr_fallback_shapes_reviews_and_checks updated for the new REST call and asserted checkSuite.createdAt in its output.

Full verification (worktree, post-fix)

  • PYTHONPATH=. python3 -m pytest tests -q → 1978 passed, 1 skipped, 21 subtests passed.
  • coverage run -m pytest tests && coverage report → 100% on scripts/ci (statements + branches).
  • interrogate → 100% docstrings.
  • git diff --check → clean.
  • ruff check . → no new findings; the one pre-existing F401 in tests/test_repository_branch_coverage_javascript_and_noema.py (confirmed unrelated in round 7) is identical on base commit 80c7f382.

Pushed as a fast-forward commit on top of 80c7f382 (fetched immediately before pushing; branch hadn't moved).


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Merge of origin/main into this branch — partially resolved, one conflict needs a decision

Attempted the requested merge in an isolated worktree (not the shared checkout): git merge origin/main (38e7dddf, which now includes #1433 and #1456) into this branch's tip 63cd827f, via git merge --no-edit (no rebase, no force-push, no amend).

CHANGELOG.md was not actually the conflict. Both sides' ## [Unreleased] additions landed in different sections of the file and Git auto-merged it cleanly — this PR's ### Changed/### Fixed entries (scheduler exact-head approval, coverage-evidence supersession) and main's new top ## [Unreleased] entries (PR #1456 Devin-review follow-ups) are both intact, newest-first, no manual edits needed.

The real conflicts were in scripts/ci/pr_review_merge_scheduler.py and tests/test_pr_review_merge_scheduler.py, in 3 regions. Two are independent/adjacent additions at the same line and are safe to combine — I resolved those. The third is a genuine same-logic conflict, so per the task's instructions I'm not guessing on it — flagging it here instead.

Resolved: rest_pr_node() (adjacent additions, both needed)

HEAD added a check-suite creation-time lookup (check_suites/suite_created_at_by_id, feeding checkSuite.createdAt into the REST-fallback rollup shape); origin/main (#1456) added fetching classic commit statuses (statuses, feeding rest_status_node). Both outputs are already consumed together, unconflicted, a few lines below in the same function's returned statusCheckRollup.contexts.nodes list. Resolved by keeping both fetches; matching test fixture/assertions in tests/test_pr_review_merge_scheduler.py updated to expect both API calls and both pieces of evidence.

Resolved: new-function block after current_head_can_attempt_merge() (adjacent additions, both needed)

HEAD added revalidate_current_head_approval() (this branch's own prior commit, re-confirming exact-head approval immediately before a merge-authorizing mutation). origin/main (#1456) added the draft-review-only dispatch helpers (draft_review_request_artifact_name, _draft_review_request_records, active_draft_review_request, dispatch_draft_review_only). Both are called from unrelated, non-conflicting call sites later in inspect_pr(). Resolved by keeping both function sets.

Needs a decision: failed_status_checks() — both sides rewrote the same dedup loop differently

Both branches independently replaced the same "latest check-run per (workflow, name) identity" loop with different algorithms:

<<<<<<< HEAD (this PR, 63cd827f)
    check_runs = latest_check_runs(pr)
    superseded_coverage_indices = superseded_coverage_evidence_indices(check_runs)
    status_contexts = [
        node
        for node in context_nodes(pr)
        if node.get("__typename") != "CheckRun"
    ]

=======  (origin/main, from #1456)
    nodes = latest_check_run_attempts(context_nodes(pr))
    status_contexts = [node for node in nodes if node.get("__typename") != "CheckRun"]
>>>>>>> origin/main
    successful_status_contexts = { ... }
<<<<<<< HEAD
    for index, node in enumerate(check_runs):
        if is_non_authoritative_coverage_check_run(node):
=======
    for node in nodes:
        if node.get("__typename") != "CheckRun":
>>>>>>> origin/main
            continue
        ...
            if index in superseded_coverage_indices:   # HEAD only
                continue
  • This branch's latest_check_runs() + check_run_recency_key(): ranks recency primarily by checkSuite.createdAt (falling back to startedAt, then "currently pending"), fixing exactly the chronology bug this PR's description calls out ("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"). It also adds a superseded_coverage_evidence_indices() / is_non_authoritative_coverage_check_run() concept letting a newer coverage-evidence check supersede an older failing one.
  • origin/main's latest_check_run_attempts(): extracted from the pre-existing (pre-fix(scheduler): require independent exact-head approval #1270) inline dedup logic, which only ever used startedAt — it's a refactor for reuse (also called from the now-rewritten strix_evidence_state(), which gained a "failed" state), not a deliberate redesign of the ranking rule.

Note: strix_evidence_state() itself is not in conflict — main's rewrite of it (using latest_check_run_attempts, adding "failed") merged in cleanly since this branch never touched that function, so it's already sitting in the merged tree as main wrote it.

I did not pick a side. For context: this branch's own already-merged (non-conflicting) tests at tests/test_pr_review_merge_scheduler.py (test_failed_status_checks_uses_latest_check_run_for_same_workflow_name, test_failed_status_checks_prefers_timestamped_duplicate_check_runs, test_failed_status_checks_treats_cancelled_before_start_rerun_as_authoritative, and the coverage-supersession tests) call sched.latest_check_runs(...) directly — taking main's side as-is would delete that function and break those tests. But that's evidence about which resolution keeps the test suite green, not a decision about the intended design (e.g., whether strix_evidence_state()/latest_check_run_attempts() should also be upgraded to the checkSuite.createdAt-based ranking for consistency, which would be a larger change than a merge conflict resolution should make unilaterally).

State left behind

  • Worked in an isolated worktree (/tmp/github-1270-conflict), the shared checkout at /home/user/.github was never touched.
  • git merge origin/main is in progress (MERGE_HEAD = 38e7dddf), not committed, not pushed. The two adjacent-addition conflicts above are resolved in the working tree; the failed_status_checks() conflict above is still marked with <<<<<<</=======/>>>>>>> exactly as shown.
  • No verification suite was run to completion (the file has live conflict markers, so it isn't even valid Python yet); pytest, coverage, interrogate, and a full-tree git diff --check grep for stray markers will all be run once failed_status_checks() is resolved per whichever direction is chosen.

Let me know which way to take failed_status_checks() (keep this branch's checkSuite.createdAt-based ranking + coverage-supersession wholesale, as its own tests require; port that ranking into latest_check_run_attempts/strix_evidence_state too for consistency; or something else) and I'll finish the merge, resolve, verify, and push the merge commit to this same branch.


Generated by Claude Code

…endent-exact-head-approval-20260824

# Conflicts:
#	scripts/ci/pr_review_merge_scheduler.py
#	tests/test_pr_review_merge_scheduler.py

Copy link
Copy Markdown
Contributor Author

Merge with origin/main completed and pushed

Continuing from the prior analysis comment, which resolved two of three conflicts and laid out the decision for the third. This comment implements that decision and reports full verification.

Third conflict: failed_status_checks() / latest_check_run_attempts() dedup divergence

Both HEAD (this PR) and origin/main (via #1456's strix_evidence_state()) independently rewrote "dedupe CheckRun reruns to the latest attempt per (workflow, name) identity" — HEAD's latest_check_runs/check_run_recency_key correctly using checkSuite.createdAt (falling back to startedAt, then a pending tier), main's latest_check_run_attempts still using the older buggy startedAt-only comparison.

Per the agreed resolution:

  1. failed_status_checks() is resolved to HEAD's version verbatim: latest_check_runs(pr), superseded_coverage_evidence_indices, is_non_authoritative_coverage_check_run, and un-deduped status_contexts from context_nodes(pr).
  2. latest_check_run_attempts(nodes) is fixed rather than left divergent. Extracted the shared recency-keyed dedup core into a new _newest_check_run_per_identity(indexed_check_runs) helper (used by both latest_check_runs and latest_check_run_attempts), so both functions now rank CheckRun reruns with the identical check_run_recency_key signal and can't silently diverge again. latest_check_run_attempts keeps its original contract unchanged: it takes an already-extracted nodes list, passes non-CheckRun (classic commit-status) nodes through unchanged, and preserves overall relative ordering. Its docstring now explains the fix the same way check_run_recency_key's does.

TDD evidence for the latest_check_run_attempts fix

Added test_strix_evidence_state_treats_cancelled_before_start_rerun_as_authoritative (sibling to the existing test_failed_status_checks_treats_cancelled_before_start_rerun_as_authoritative): a Strix CheckRun canceled before starting (startedAt: null, later checkSuite.createdAt) that is genuinely newer than an earlier completed/successful Strix run.

  • Confirmed this new test fails against the old startedAt-only latest_check_run_attempts logic (temporarily reverted locally to prove it): assert attempts[0]["conclusion"] == "CANCELLED"AssertionError: assert 'SUCCESS' == 'CANCELLED', i.e. strix_evidence_state would have reported stale "complete" instead of the correct "failed".
  • Confirmed it passes with the fix in place.

Full verification (all green)

  • grep -rn '<<<<<<<\|=======\|>>>>>>>' across the entire repo: no real conflict markers remain (only pre-existing prose in pr-review-autofix.yml describing what markers look like, and literal === lines inside log-excerpt fixtures in scripts/ci/test_strix_quick_gate.sh — neither is an actual conflict).
  • PYTHONPATH=. python3 -m pytest tests -q2047 passed, 1 skipped, 21 subtests passed, no regressions.
  • coverage run -m pytest tests && coverage report --show-missing100% overall, including scripts/ci/pr_review_merge_scheduler.py at 2103/2103 statements, 896/896 branches.
  • interrogate100.0% docstring coverage.
  • git diff --check → clean, no whitespace errors.
  • git diff origin/main -- .github/workflows/strix.yml scripts/ci/test_strix_quick_gate.shempty, confirming both files (touched by already-merged feat(ci): report free-route provider-family diversity for Strix pool evidence #1433 on main, not part of this PR's own diff) came through the merge unchanged from main.
  • The two already-resolved conflicts (rest_pr_node()'s combined check_suites+statuses fetch, and the four draft-review-dispatch functions coexisting with revalidate_current_head_approval()) were verified still intact post-merge.

Result

  • Merge commit: 98aa3a145f9fa0731917fb3a1a364ce44daa3c3f (parents: 63cd827f HEAD, 38e7dddf origin/main tip), pushed to codex/scheduler-independent-exact-head-approval-20260824 with a plain git push (no force, no rebase).
  • git merge-base --is-ancestor origin/main HEAD succeeds locally.
  • GitHub now reports mergeable_state: "behind" for the PR (base sha 38e7dddf... = exact origin/main tip) — no longer "dirty"/conflicted.

Worktree at /tmp/github-1270-conflict will be removed now that the push is confirmed landed on origin.


Generated by Claude Code

@seonghobae
seonghobae merged commit d02f31d into main Aug 31, 2026
38 of 47 checks passed
@seonghobae
seonghobae deleted the codex/scheduler-independent-exact-head-approval-20260824 branch August 31, 2026 01:09
@github-project-automation github-project-automation Bot moved this from In Progress to Done in naruon Platform Roadmap Aug 31, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Devin Review

Comment on lines +3689 to +3691
revalidation = revalidate_before_merge()
if revalidation:
return revalidation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Fallback auto-merge uses stale approval

When a direct merge is refused, revalidate_before_merge has already run. A concurrent approval revocation can leave fallback auto-merge authorized.

Prompt for agents
In scripts/ci/pr_review_merge_scheduler.py, both direct_or_auto branches revalidate approval before calling merge_pr, but a policy refusal can occur later and lead to enable_auto_merge or preservation of an existing auto-merge request without another fresh approval read. Revalidate again after the direct merge failure and immediately before either fallback action. If approval or the exact head changed, disable any existing auto-merge request and return a waiting decision. Add regression tests that revoke independent approval during the simulated direct merge refusal for both CLEAN and BLOCKED paths, including an already-queued auto-merge request.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants