fix(codeql): serialise the dispatched scan matrix with toJSON - #1926
Conversation
codeql-pr.yml sends client_payload.matrix as an array, and the dispatch handler assigned it straight into env:, where a value must be a scalar. GitHub rejects the step with "A sequence was not expected", so the step dies before running any of its script and the dispatched scan is skipped. The handler has 0 successes against 136 failures since #1776 added it. The validate step already reads the value through jq and checks `type == "array" and length == 1`, so JSON text is what it was written to consume; no consumer changes. Dropping the `|| ''` fallback is safe because an absent matrix yields the string "null", which fails the same array check and reaches the existing error path. Neither yaml.safe_load nor actionlint 1.7.12 reports this file as invalid -- it is an Actions template rule rather than YAML syntax, so only GitHub's validator rejects it and no local gate catches the class. The added string contract test is therefore the only guard that runs before a dispatch does. Refs #1925 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 28 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
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 |
|
Confirmed: this PR will move the failure, not clear it. The body says "may" — I have now measured it, so it is "will". The authorization check at lines 155-157 lives in the same step's script, after the Merge this anyway, but not expecting green CodeQL. It is a genuine and independent defect: even after the identity question is settled, line 149 would still break the step. This is a prerequisite, not the whole repair — and the resulting authorization error is a far more diagnostic failure than a template error that no local linter can see. The identity question is tracked separately in #1927, where peer review traced the change to Sequencing: #1927 decides the identity, this PR unblocks the step that would fail regardless. Neither is sufficient alone. |
|
수정 내용은 독립적으로 도출해서 같은 결론에 도달했습니다. 다만 "어느 스텝도 실행되기 전에 거부된다"는 서술이 사실과 다르고, 그게 영향 평가를 바꿉니다. 수정 자체는 맞습니다 (독립 확인)이 PR을 보기 전에 생산자와 소비자를 직접 따라갔고 같은 답이 나왔습니다.
그래서 그런데 실패는 컴파일 시점이 아니라 스텝 시점입니다PR 본문·CHANGELOG·테스트 docstring이 "GitHub이 파일 전체를 거부하고 어느 스텝도 실행되기 전에 죽는다"고 서술합니다. 실제 레코드는 다릅니다. 최근 실패 12건을 전수로 열었습니다. 가장 최근 실행( 로그의 실제 에러도 그 스텝 안에서 납니다. 즉 워크플로는 정상적으로 컴파일되고, job이 생성되고, 러너를 배정받고, 두 스텝이 성공한 뒤, 세 번째 스텝의 왜 이 구분이 중요한가1. 큐 비용이 발생합니다. 컴파일 거부는 러너를 소비하지 않지만, 스텝 시점 실패는 러너를 온전히 하나 잡습니다. 위 12건의 러너 ID가 전부 다르므로 매 실패가 별도 러너 배정입니다. 현재 누적 실패는 175건입니다(이슈 작성 시점 136건에서 증가). 이 조직은 60-job 상한을 공유하고, 제가 별도로 잰 완주 실행 하나에서 **대기가 벽시계의 99.87%**였습니다. 그러니 이건 "스캔이 안 돈다"만이 아니라 아무것도 만들지 못하면서 슬롯을 175번 점유했다는 뜻이기도 합니다. 2. 다음 사람의 진단을 잘못된 곳으로 보냅니다. docstring이 "before any step runs"라고 하면, 유사 증상을 쫓는 사람은 job이 0개인 실행을 찾게 됩니다. 그런 실행은 없습니다. 3. 실패가 계속 증가하는 이유를 설명합니다. 컴파일 거부라면 조용히 실패하지만, 실제로는 매번 큐를 통과해 러너까지 받고 죽습니다. 제안수정 코드는 그대로 두고 서술만 고치면 좋겠습니다 — CHANGELOG와 테스트 docstring의 "before any step runs" / "the whole file"을 "스텝 3의 범위 한계위 12건은 가장 최근 12건이고 전부 2026-09-05 07:26~08:01Z 구간입니다. |
이 PR이 고치는 두 번째 증상: 디스패치는 성공했는데 verdict가 영원히 안 옵니다
CO#1028/#1031은 설계상 fail-closed입니다: 이 job은 지금 이 증상이 적게 보이는 이유 — 그리고 곧 늘어날 이유최근 갱신된 열린 PR 20건의 체크 상태 분포입니다(attempted=20 ok=20 err=0): 18건은 CodeQL 체크가 아예 실행되지 않아서 이 벽에 도달하지 못한 것뿐입니다. 같은 근본 원인(
|
…sumers vars.OPENCODE_REPOSITORY_DISPATCH_ACTOR is read by three workflows, and the first commit widened only one of them: opencode-review-dispatch.yml:127 covered by the previous commit codeql-scan-dispatch.yml:155 byte-identical gate, was still exact-match pr-review-fix-scheduler.yml:156 same three conditions, different error line Left as-is, codeql-scan-dispatch would keep rejecting the App identity once #1925's toJSON fix lets it reach line 155, and the scheduler would too. Three consumers of one variable with two parsers is the next drift, so all three now run the same comma-separated parse with the same semantics: actor and sender must both equal the SAME listed identity, empty list admits nothing, single value unchanged. The scheduler keeps its own error line. Tests extended in place for both: a listed identity passes (whitespace around commas tolerated), an unlisted one is rejected, and actor/sender that are two different listed identities are rejected. Negative control: both extended tests fail against the unmodified gates on origin/main. The codeql helper creates tmp_path/bin, so each invocation gets its own subdirectory. No blob-SHA pin references either newly edited workflow. No open PR touches either gate: #1926 covers codeql-scan-dispatch 146-152 and #1741 covers pr-review-fix-scheduler 210-217. Verified: 2891 passed, coverage 100%, interrogate 100%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…all three consumers (#1929) (#1932) * fix(dispatch): accept a list of trusted dispatcher identities Two trusted workflows send the opencode-review repository_dispatch: opencode-review.yml through the OpenCode GitHub App (sender opencode-agent[bot], introduced by #1497) and pr-review-merge-scheduler.yml through its own token chain (sender github-actions[bot]). The authorization gate in opencode-review-dispatch.yml compared both actor and sender against a single-valued variable that still names only github-actions[bot], so every app-token dispatch has failed at the first job -- 9611 failures to 466 successes over the workflow's lifetime, and no open PR holds a successful review on its current head (#1929). Parse ALLOWED_DISPATCH_ACTOR as a comma-separated list, exactly as the adjacent ALLOWED_DISPATCH_TARGETS block already does. Semantics are preserved otherwise: actor and sender must both equal the SAME listed identity (a dispatch whose actor and sender are two different listed identities is still rejected), and an empty allowlist admits nothing. This change does not alter the variable. Which identities belong on the list is an authorization decision for the repository owner; this only makes the gate able to express more than one. A single-valued variable keeps working unchanged. Contract test extended in tests/test_opencode_agent_contract.py: both identities pass with a listed allowlist (whitespace around commas tolerated), an unlisted identity is rejected, and mismatched actor/sender is rejected. Negative control: the extended test fails against the unmodified gate on origin/main. REVIEW_DISPATCH_BLOB_SHA recomputed via git hash-object. Verified: 2890 passed, coverage 100%, interrogate 100%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(dispatch): parse the actor allowlist identically in all three consumers vars.OPENCODE_REPOSITORY_DISPATCH_ACTOR is read by three workflows, and the first commit widened only one of them: opencode-review-dispatch.yml:127 covered by the previous commit codeql-scan-dispatch.yml:155 byte-identical gate, was still exact-match pr-review-fix-scheduler.yml:156 same three conditions, different error line Left as-is, codeql-scan-dispatch would keep rejecting the App identity once #1925's toJSON fix lets it reach line 155, and the scheduler would too. Three consumers of one variable with two parsers is the next drift, so all three now run the same comma-separated parse with the same semantics: actor and sender must both equal the SAME listed identity, empty list admits nothing, single value unchanged. The scheduler keeps its own error line. Tests extended in place for both: a listed identity passes (whitespace around commas tolerated), an unlisted one is rejected, and actor/sender that are two different listed identities are rejected. Negative control: both extended tests fail against the unmodified gates on origin/main. The codeql helper creates tmp_path/bin, so each invocation gets its own subdirectory. No blob-SHA pin references either newly edited workflow. No open PR touches either gate: #1926 covers codeql-scan-dispatch 146-152 and #1741 covers pr-review-fix-scheduler 210-217. Verified: 2891 passed, coverage 100%, interrogate 100%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
ConceptWeave consumer evidence independently reproduces this exact failure on the bootstrap prerequisite. Central dispatch run |
…e rejection The validate-dispatch job does get a runner and its first steps run; GitHub rejects only the step whose env: receives the array, when that env: is evaluated. Docstring wording corrected to match the observed job timeline. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit f90c23b3c0386e22528cc1ab8680c4a31fa630b9)
…cks are in flight (#1937) A scheduler run that executes after a 2h+ queue wait finds its scanned PR behind main and merges main into the head before any review is dispatched (inspect_pr, pre-review path). That push cancels every queued check on the old head (22/28 on #1926, 21/30 on #1484) and requeues the PR at the back, so under a saturated queue no head ever finishes its checks: 76 of the 77 PRs merged since 2026-09-04 had 0/12 required contexts satisfied at merge. has_in_flight_check_runs() reuses latest_check_runs()/running_check_state(); the pre-review path now decides "wait" while any newest current-head check run is still queued or running. No age cap on purpose: a cap would restart the loop. The post-approval update path is unchanged (main is strict=true). CLAUDE.md described only the post-approval update; it now names both paths. Refs #1935 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
Prediction verified — 10 of 10 completed runs since this merged (13:57Z) moved the failure exactly as forecast. Before this PR the same step died with Two housekeeping notes for anyone re-checking:
|
codeql-scan-dispatch.yml— the handler that runs the native CodeQL scan — has 0 successes against 136 failures since #1776 added it. Every run dies in the same step.codeql-pr.yml:257dispatchesmatrix:[{language:$language,"build-mode":$build_mode}]— an array — and line 149 assigned it straight intoenv:, where a value must be a scalar.The fix is what the consumer already expects
The validate step reads the value through
jqand checks the parsed shape:So JSON text is the shape it was written for. No consumer changes. Dropping
|| ''is safe: an absent matrix yields the string"null", which fails the sametype == "array"check and reaches the existing error path.Scope — narrower than I first reported
I originally described this as the workflow failing to compile. That was wrong, and #1925 carries the correction. Run
33948513003shows it does run:Two jobs are created and a runner is assigned;
env:is evaluated when its step starts, so only that step dies. The blast radius is oneenv:value.Why a string contract test
Neither local gate flags the broken file:
yaml.safe_loadactionlint1.7.12It is an Actions template rule, not YAML syntax, so no local tool catches the class and the added string contract is the only guard that runs before a dispatch does. Verified in both directions: 18 passed with the fix, the new test fails when the workflow alone is reverted, 18 passed again once restored.
I also swept for the same class elsewhere —
matrixis the only array-valuedclient_payloadfield; every other suchenv:assignment is a scalar (branch names, counts, booleans, SHAs).What this does not establish
This may move the failure rather than clear it. The authorization check (lines 155–157) lives in the same step's script, after the
env:block that currently fails — so it has never executed. Once line 149 evaluates, the step reaches that check for the first time, and if the dispatching identity does not matchvars.OPENCODE_REPOSITORY_DISPATCH_ACTOR(currentlygithub-actions[bot]) it will fail there instead. A single re-dispatch after merge settles it.Also unverified: whether CodeQL scanning reached this repository by another path before #1776. "Broken since it was added" is established; "no CodeQL coverage since then" is not, and #1925 stays open for it.
Full suite 2891 passed, 1 skipped, 21 subtests;
interrogate100%.🤖 Generated with Claude Code