fix(scheduler): do not cancel same-SHA scan-pr-queue on review - #2289
seonghobae wants to merge 18 commits into
Conversation
A pull_request_review shares the PR concurrency group with pull_request_target, so cancel-in-progress on that event retired scan-pr-queue even when the head SHA had not changed.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
P1 concurrency finding at exact head 846cee70c79ad8645d185f548e7ea3017733ea0f: removing pull_request_review from cancel-in-progress preserves an already-running scan, but GitHub's concurrency contract still cancels and replaces an existing pending run when another run enters the same group. Thus a review event can still retire a queued scan-pr-queue for the same exact head. The authoritative contract is https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-workflow-concurrency (queue: single is the default; queue: max preserves multiple pending runs; it cannot be combined with cancel-in-progress: true). Repair should first add a RED contract for pending preservation, then use the native queue contract and rely on the scheduler's live PR/head validation to make stale events fail closed. No arbitrary retry/count fallback is appropriate.
|
Repair receipt — exact head
Full pytest is not claimed because pytest is absent from the isolated execution environment. Fresh hosted exact-head Checks and qualifying independent approval remain mandatory; no self-approval, gate relaxation, force push, or stale evidence was used. |
queue: max already keeps a pending run when cancellation is off, but the close-event contracts still required the cancellation key GitHub rejects beside that queue. State both cases in the contract instead. Co-authored-by: Cursor <cursoragent@cursor.com>
6784c19 evidence boundaryPlatform contract, not a live run of this SHA:
GitHub has not validated this head's workflow file. The 76 passed tests in Close-event cleanup, static only: Independent review: GraphQL |
Corrected RED: preserve same-head pending scans with an exact-head workflow queue, while requiring an actions:write metadata cleanup to retire only predecessor-head runs after repository/PR/head revalidation. Focused result before production repair: 4 failed, 72 passed.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
P1 exact-head inconsistency at e2647c5b6d72259e99a36a4955faeb0a2967931c: the newly committed contracts require an exact-head concurrency key and a bounded cancel-superseded-pr-runs metadata job, but the production workflow contains neither that job nor client_payload.pr_head_sha, force-cancel, live_target_matches, or TARGET_PR_HEAD_SHA. The exact test tree is therefore RED and the PR body still names predecessor head be512207…. Keep Draft/Proposed, implement the tested production boundary, and regenerate exact-head evidence before review admission.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
P0 scheduler contract finding on exact head e2647c5b6d72259e99a36a4955faeb0a2967931c.
queue: max itself is now an official GitHub Actions concurrency contract (up to 100 pending runs; it cannot coexist with cancel-in-progress: true), so that part is valid. The current production group, however, is still PR-stable: pull_request_target / pull_request_review resolve to pr-{number} and do not include github.event.pull_request.head.sha or dispatch pr_head_sha. That directly contradicts the new tests/docstrings, which assert both head-SHA expressions are present and describe an “exact-head queue” where a successor head enters another workflow group. The workflow comment also still says “the group is the PR number, not the head SHA.”
RED: execute the current exact-head focused tests against the actual workflow blob and prove two concurrent admissions for head A are serialized/preserved while head B is admitted into a distinct group; the literal contract assertions should currently fail because the group contains no head SHA. Also cover targeted repository_dispatch, where client_payload.pr_head_sha must be bound to the same identity contract.
GREEN: choose one causal contract and make production/tests/docs agree. If the intended design is exact-head workflow groups, include the validated live head SHA in every PR-targeted group and keep the bounded PR-level predecessor cleanup as the retirement boundary. If the intended design is PR-stable FIFO, remove the exact-head assertions/claims and prove that a queued stale predecessor cannot delay or mutate the successor beyond the stated SLA. Preserve queue: max, live PR revalidation, and fail-closed stale-event behavior. Then re-run exact-head hosted scheduler tests/security checks. The PR body’s Current authority SHA (be512207…) is also stale versus this live head and should be currentized before merge review.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
P0 single-writer follow-up: live .github#2283 already owns this exact scheduler-admission boundary at 91c19fd49f00bb44f8143dea8c0f0ecb2de6a564 and has the stronger layered contract: workflow-level {repository, PR, exact head} + queue: max, with downstream {repository, PR} cancellation to retire predecessor-head review work. Its focused/full regressions also cover malformed workflow-run pages and explicit present/stale/missing admission states.
That makes #2289 a parallel writer on the same pr-review-merge-scheduler.yml concurrency contract, and its current PR-stable group is semantically behind #2283. Do not independently repair the same boundary here or close this PR merely as “duplicate.”
Acceptance: inventory every valid #2289 delta/evidence not already present in #2283 (including the observed #2284 cancellation incident, queue-limit/FIFO documentation, close-event cleanup assertions, and Gap-row evidence). Ordinary-forward those missing deltas into the canonical scheduler owner or a verified successor, then prove source/test/fixture/docs equivalence at exact heads. Only after complete succession is verified can #2289 become PR=0. If #2289 contains no unique valid semantic delta after that comparison, record the zero-delta/equivalence proof rather than landing a second scheduler implementation.
actionlint
|
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Follow-up on exact 406c7178974c3b887694a229a59c66a02e0810dd: the earlier production/tests/docs contradiction is source-level repaired. The workflow group now actually binds PR admissions to exact head SHA (including dispatch pr_head_sha), keeps queue: max, and introduces a revalidated predecessor-retirement job; the focused contracts now describe the implementation they test. That closes my prior RED about a PR-stable production group masquerading as an exact-head queue.
The remaining gate is single-writer succession, not the original concurrency bug. .github#2283 still owns the same scheduler-admission boundary and already carries the broader duplicate-dispatch/formal-receipt/malformed-page state machine. Before either PR can disappear, compare the two exact heads and move every valid #2289-only delta into one canonical successor—especially the observed #2284 cancellation incident, close-event cleanup behavior, queue: max evidence, and any predecessor-run retirement tests/docs that #2283 does not yet own. Then prove source/test/fixture/Gap evidence equivalence. Until that succession is verified, #2289 is not PR=0 and should remain Draft.
Traceability is also still stale: the body’s Current authority says be512207… while the live head is 406c7178…. Currentize that evidence and reacquire exact-head hosted checks/independent review after consolidation.
Preserve both divergent exact-head admission lineages with an ordinary two-parent merge. Resolve the shared queue-contract test by retaining the scheduler predecessor-retirement contracts and the OpenCode exact-head dispatch contracts.
|
Exact-head repair receipt —
|
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head direct repair receipt for c39f57d4890be5bab12e8845014eeefe1e99f7ae (tree 722fc4d1…). RED: predecessor source accepted force-cancel without proving terminal state and did not re-read the live PR after candidate selection; three executable regressions failed. GREEN 8067fb65 revalidates immediately before every mutation and requires completed/cancelled within six bounded reads. Exact combined scheduler-contract file is 74/74 GREEN in both default and GITHUB_ACTIONS=true; compile and diff checks are GREEN. c39f57d4 updates the canonical Gap evidence. The branch remains Draft/Proposed pending #2283, hosted exact-head Checks, and independent approval.
Review scope against the live base, then readyThe 15-file / 40-commit figure is real, and it is not this pull request's unique diff. The live base is
The OpenCode workflow and inflight-dispatch files in the 40-commit range are already on this base, so they are outside the pull request's own delta. Review of record for the increment remains #2289 (comment). No listed Orca worktree on this host or on s1 is checked out at |
|
Ready 이후 현재 head 필수 컨텍스트의 최신 완료 결론은 21:58Z 독립 비작성자 APPROVE는 없습니다. |
|
스택: #2289 |
|
같은 SHA draft 재전환의 원인. 이번 턴에 ready/draft는 바꾸지 않았습니다. 이벤트 actor는 전부 GitHub 사용자
실행되는 파일은 이 head가 아닙니다. 리뷰 skip도 그 base 파일에 있습니다. 조치 소유: Keep Draft 리뷰를 쓴 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
21:03:00Z와 21:44:30Z Keep Draft 리뷰에 대한 답입니다. 이번 턴에 ready/draft는 바꾸지 않았습니다.
merge 조건과 ready 조건은 다릅니다. merge는 비작성자 APPROVE, 필수 체크 성공, 그리고 이 PR보다 #2283이 main에 들어가는 순서입니다. ready는 그 승인이 아닙니다. opencode-review.yml은 live draft이면 리뷰 dispatch를 하지 않고, changed-scope job은 converted_to_draft 이벤트를 제외합니다. CodeRabbit/Devin도 draft를 건너뜁니다. 그래서 merge 게이트를 이유로 ConvertToDraft를 내면, 같은 SHA의 대기 중인 scan이 base workflow의 cancel-in-progress: true로 취소되고 리뷰 job은 시작되지 않습니다.
GitHub actor seonghobae는 사람 직접 조작의 증거가 아니고, 그 리뷰의 세션 소유는 명령 근거 없이 확정하지 않습니다. 같은 SHA 021ffb2bb에 ConvertToDraft를 반복하지 말아 주십시오.
현재 순서는 이렇습니다. 22:04:01Z 런 35472240809, 35472240824, 35472240839, 35472240849는 아직 QUEUED입니다. 35472240839의 Detect changed scope는 이미 SKIPPED라 이 런은 OpenCode를 요청하지 않습니다. 이 넷이 끝나기 전에 ready를 누르면 네 그룹이 함께 취소되므로 지금은 누르지 않습니다. #2283 91c19fd49는 이미 ready이고 22:18:29Z 런 35472949574 외가 QUEUED입니다. 그 대기열이 리뷰 입구이고 두 번째 이벤트는 만들지 않습니다. 이 넷이 terminal이 된 뒤에 ready 한 번이 #2289 리뷰 개시입니다. 그 전에도 후에도 같은 SHA draft 반복은 하지 않습니다.
|
Ready once: original same-SHA
|
|
Governance handoff: this successor's exact-head This is not a request to undo #2289 or copy its mutable source elsewhere. Canonical AGENTS lane #1885 now owns the explicit reconciliation finding (comment 5747791655), and #712 tracks the operational classification consequence. Preserve #2289 and its tests/evidence as candidate architecture; do not claim ordinary merge authority until protected governance and prerequisite #2283 agree on the same event-order-independent contract. No close, self-approval, bypass, force update, or synthetic GREEN is requested. |
Merge-prep (lead, local three-dot — no REST Actions re-poll)Stack: base Delta (local Blockers (unchanged policy):
REST Actions listing hit rate limit this tick — no further run-list polling; resume cancel triage from proven CLOSED/superseded evidence when core quota recovers. |
Independent review request (lead, pre-CI-complete)Exact head Merge still requires fail=0 ∧ pending=0 ∧ independent non-author APPROVE — bot COMMENT is not merge APPROVE evidence. |
|
Ordinary/non-force descendant repair completed after canonical #2283 advanced. #2283 moved from I preserved prior #2289 Fresh compare against current #2283 is now ahead 18 / behind 0, merge base exactly New exact-head hosted generation is natural, not a wake commit: CodeQL |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-stack review after concurrent ordinary-forward movement. e2b699a45a3f29d48fd0891ae9cb42997789e9ec is an ordinary two-parent adoption commit with parents prior #2289 021ffb2… and current canonical #2283 368b87cd…. Fresh compare from current base 368b87cd… leaves exactly the eight scheduler-owned effective paths; the two opencode_inflight_dispatch_gate paths owned by #2283 disappear from the remaining delta. This resolves the stale-base/parallel-owner finding without force/rebase or source copying. Fresh current-head CodeQL/SAST/Security are queued, so no hosted or review evidence from 021ffb2… transfers to this head. COMMENT only; qualifying independent approval and terminal exact-head acceptance remain required.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/pr-review-merge-scheduler.yml— GitHub Actions review jobCHANGELOG.d/20260920-scheduler-cancellation-verification.md— repository behaviordocs/product-technical-gap-baseline.md— operator or user guidancetests/test_close_empty_pr_queue_pressure.py— regression suitetests/test_current_head_coalescer_self_cancellation.py— regression suitetests/test_opencode_agent_contract.py— regression suitetests/test_opencode_workflow_shell_syntax.py— regression suitetests/test_required_workflow_queue_contract.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: pr-review-merge-scheduler.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: pr-review-merge-scheduler.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: 20260920-scheduler-cancellation-verification.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: 20260920-scheduler-cancellation-verification.md"]
R2 --> V2["required checks"]
Evidence --> S3["Docs: product-technical-gap-baseline.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: product-technical-gap-baseline.md"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_close_empty_pr_queue_pressure.py (5 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_close_empty_pr_queue_pressure.py (5 files)"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
e2b699a45a3f29d48fd0891ae9cb42997789e9ec - Workflow run: 35529329097
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: pr-review-merge-scheduler.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: pr-review-merge-scheduler.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: 20260920-scheduler-cancellation-verification.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: 20260920-scheduler-cancellation-verification.md"]
R2 --> V2["required checks"]
Evidence --> S3["Docs: product-technical-gap-baseline.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: product-technical-gap-baseline.md"]
R3 --> V3["docs review"]
Evidence --> S4["Test: test_close_empty_pr_queue_pressure.py (5 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_close_empty_pr_queue_pressure.py (5 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR correctly implements the transition from cancel-in-progress to queue: max for the scheduler, ensuring that runs for the same head are preserved while runs for superseded heads are retired via the new cancel-superseded-pr-runs job. This resolves the 'pending-preservation' P1 issues identified in prior threads. Security is improved by enforcing a case-insensitive match between the targeted dispatch's TARGET_HEAD_SHA_INPUT and the live PR head SHA (lines 418-421). The implementation is verified by three distinct executable probes in tests/test_required_workflow_queue_contract.py: test_scheduler_cleanup_revalidates_target_after_run_selection (falsifies concurrent head advance regression), test_scheduler_cleanup_fails_when_accepted_cancel_never_finishes (falsifies non-terminal cancellation regression), and test_scheduler_cleanup_verifies_accepted_cancelled_state (falsifies asynchronous terminal proof regression). All changes align with the updated product-technical gap baseline.
Reviewed changed lines
.github/workflows/pr-review-merge-scheduler.yml:90 (RIGHT): The updated concurrency group now includes the PR head SHA as a suffix. This ensures that runs for the same head are grouped together, while runs for different heads enter distinct groups. Combined withqueue: max, this preserves pending runs for the same head while allowing new heads to proceed, resolving the previous pending-preservation issue..github/workflows/pr-review-merge-scheduler.yml:110 (RIGHT): Thecancel-superseded-pr-runsjob introduces a robust metadata-only cleanup. It implementslive_target_matchesto revalidate the PR state before and during the cancellation loop, preventing accidental cancellation of runs if the head advances concurrently. Theforce-cancelloop correctly polls for a terminalcompleted/cancelledstatus rather than assuming a successful POST request implies completion..github/workflows/pr-review-merge-scheduler.yml:420 (RIGHT): The targeted dispatch validation now requiresTARGET_HEAD_SHA_INPUTto match the live PR head SHA (case-insensitive). This closes the security/correctness gap where a caller could have triggered a scheduler run for a stale head.tests/test_required_workflow_queue_contract.py:1552 (RIGHT): The new test casestest_scheduler_cleanup_revalidates_target_after_run_selection,test_scheduler_cleanup_fails_when_accepted_cancel_never_finishes, andtest_scheduler_cleanup_verifies_accepted_cancelled_stateprovide direct executable proof that the three critical failure modes (concurrent advance, non-terminal state, and asynchronous terminal proof) are handled correctly.
Adversarial validation
tests/test_required_workflow_queue_contract.py:1552 (RIGHT)falsified: Concurrent head advance after run selection might still lead to cancellation of the new head. — The test simulates a head change between the selection of a run to cancel and the actual cancellation call, proving thatlive_target_matchesprevents the mutation.tests/test_required_workflow_queue_contract.py:1552 (RIGHT)falsified: A successful API response toforce-cancelis treated as terminal, potentially leaving a zombie run. — The test mocks a scenario whereforce-cancelreturns 200 but the run remainsin_progress, proving the cleanup job correctly errors out.tests/test_required_workflow_queue_contract.py:1552 (RIGHT)falsified: Asynchronous cancellation state updates are missed, leading to false failure reporting. — The test mocks a delayed transition tocompleted/cancelledand proves the polling loop correctly identifies the terminal state.- Residual risk: low
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
e2b699a45a3f29d48fd0891ae9cb42997789e9ec - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
This branch has not been deployed
1 participant
Current authority — 2026-09-22 KST
Exact head is
e2b699a45a3f29d48fd0891ae9cb42997789e9ec, OPEN / Draft, still based on canonical OpenCode admission owner #2283 predecessor368b87cd929438241032bc6110e33856965b1f5a. The parent has since moved ordinary-forward to928fc276891b185943a2a93ac003e227e4e47f6a; this successor is therefore intentionally stale-base until that parent obtains complete fresh exact-head acceptance. Do not duplicate the parent fixture repair or shared dependency repair into this effective delta.The earlier stale-base finding was repaired ordinary-forward at
e2b699a…as a two-parent adoption commit over prior #2289021ffb2bb6914f810f192cc69980ecc1245c79d0and then-current #2283368b87cd…. Compare from that accepted-at-the-time base leaves the eight scheduler-owned effective paths; #2283's admission implementation/test are base-owned. Current-head COMMENT review5260136260records that single-writer split.This lane owns exact-head scheduler serialization, metadata-only predecessor cleanup, live PR-head revalidation immediately before cancellation, and bounded proof that accepted cancellations reach terminal
completed/cancelled. The canonical #2283 admission state machine remains parent-owned.#2283's former Runtime Quality RED was causally source-repaired:
0d472b4c546388e115fad9c1919696d7aeb3a098adds only the missing"status":"queued"to the retained already-in-flight fixture, and928fc276891b185943a2a93ac003e227e4e47f6arestores trailing-newline collateral. Current parent evidence is now split: Runtime Quality35552210925, Security Scan35552210897, and SAST35552210972are exact-head success; Python Security35552210893is terminal failure afterpip-auditreports AnyIO4.14.0CVE-2026-63374 / CVE-2026-64847 / CVE-2026-63349 inrequirements-strix-ci-hashes.txt; CodeQL35552210898remains queued. Canonical dependency owner #2278 already owns AnyIO4.14.0 -> 4.14.2, so this successor must not copy that repair. Parent evidence: #2283 comment5763321717; the delayed Security recovery / remaining CodeQL queue is tracked under #712 comment5763458164.Current #2289 hosted generation remains predecessor-relative and cannot override the moving parent: SAST
35501047691completed success; CodeQL PR35501047686and Security Scan35501047693were queued on this head. None transfer after a future restack.Completion requires #2278 accepted integration/adoption and #2283 complete fresh exact-head terminal acceptance first, then ordinary/non-force reconciliation onto that accepted parent, followed by fresh exact-head hosted acceptance for this successor, no actionable review, qualifying independent approval, and ordinary protected-main merge. No self-approval, source-neutral wake/no-op commit, blind rerun, dependency-fix duplication, paid fallback, force push, destructive rebase, synthetic passing status, or gate weakening.