feat(ci): fail CI when a workflow file does not parse (BLO-23511) - #1429
Conversation
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
An unparseable workflow produces zero jobs, so it cannot fail any gate. GitHub manufactures a run that completes instantly with `conclusion: failure` and `jobs: []`, and every required check stays green because the broken workflow never ran. `docker-agent.yml` was unparseable on master for ~19h (f94d521 -> b6ac6af): no toolchain build, no image promote, no cluster image bump. The job that broke the file was the `alert-on-failure` job added to detect that very workflow failing, so the alert could never fire. `check-github-runner-labels.mjs` reads these same files but scans them line by line, and a line scanner reads a structurally broken file happily — so the repo had a workflow-scanning gate constitutionally unable to detect a workflow that does not parse. Adds actionlint to the `policy` job, pinned by version and SHA256. The `policy` job never installs node_modules (setup-pnpm runs with the default `run_install: false`) and `js-yaml` is only a version constraint in package.json, not a dependency; declaring one would change pnpm-lock.yaml, which `Block manual lockfile edits` rejects in the same job. A static binary sidesteps both, parses with a real YAML parser, and checks more besides. The gate fails closed. A missing binary or an empty workflow set is an error, not a skip — quietly no-opping would reproduce the exact bug being fixed. `-shellcheck=`/`-pyflakes=` are pinned off so the verdict depends on the pinned binary alone rather than on what a runner happens to preinstall. Verified: rejects f94d521's docker-agent.yml at line 409, the exact line the YAML parser cites; passes all 25 workflows on master. Refs BLO-23511 Refs BLO-23128
4cacd0e to
eecccd9
Compare
allyblockcast
Bot
changed the title
|
@ally please review at head eecccd9 — new CI gate that fails the Specific review focus:
Verification already done, for context:
|
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: eecccd9
Looks good. This is a well-scoped CI hardening change with no blocking findings. I independently verified the two things that actually carry risk here:
- The checksum pin is genuine.
023070a287cd8cccd71515fedc843f1985bf96c436b7effaecce67290e7e0757matchesactionlint_1.7.7_linux_amd64.tar.gzin upstream's own publishedactionlint_1.7.7_checksums.txtbyte for byte. The trust anchor is real, and it is enforced beforechmod +x. - The gate does not break the existing tree.
policypasses at this head (1m14s), which means actionlint 1.7.7 lints all current workflows clean under.github/actionlint.yaml— so this does not land a repo-wide red.
The two red checks are not caused by this diff: General tests (workspaces-a/b) died with exit code 130 / The runner has received a shutdown signal during pnpm install, and verify is red only because it aggregates that lane (GENERAL_TESTS_RESULT: failure). Infra interruption, matching the BLO-20867 pattern the verify job's own error text calls out. Re-run to get a real result.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (4)
-
[gstack/review]
.github/actionlint.yaml:3— The ARC label allowlist is now duplicated: these eight labels are byte-identical toALLOWED_RUNNERSinscripts/check-github-runner-labels.mjs:8. Adding a ninth pool means editing two files, and forgetting this one fails the new gate while the dedicated label checker passes — a confusing split verdict on an unrelated PR.- Consider generating
.github/actionlint.yamlfromALLOWED_RUNNERS(or vice versa), or at minimum a cross-reference comment in each file naming the other. Worth noting the two checkers are genuinely complementary, not redundant:check-github-runner-labels.mjsvalidates both branches of theTERNARY_RUNNER_EXPRESSIONmerge-queue shape, which actionlint treats as an opaque expression.
- Consider generating
-
[pr-review-toolkit / types]
scripts/check-workflows-parse.mjs:59— The{ readdir = readdirSync } = {}injection seam is dead. The only caller,scripts/check-workflows-parse.mjs:76, invokescollectWorkflowFiles(repoRoot)with no options, and no test supplies it either — the directory-failure test instead relies on a genuinely absent path.- Either drop the parameter, or thread it through
runCheckso the readdir failure path can be exercised without depending on filesystem state.
- Either drop the parameter, or thread it through
-
[native-codex]
.github/actions/setup-actionlint/action.yml:37— The declaredoutputs.binis never consumed.pr.ymluses the action without anid:, and the gate resolves the binary off$GITHUB_PATH(action.yml:93) via the script'sACTIONLINT_BIN || "actionlint"default.- Fine to keep as a deliberate public interface for the composite action, but the
$GITHUB_PATHwrite is the load-bearing mechanism — worth a one-line note saying so, since a future refactor could plausibly drop the$GITHUB_PATHline believing the output is what wires it up.
- Fine to keep as a deliberate public interface for the composite action, but the
-
[pr-review-toolkit / tests]
scripts/check-workflows-parse.test.mjs:86— Test is named "an unreadable workflows directory" but the fixture is a missing directory (temp root with no.github/workflows), so it exercises ENOENT rather than EACCES. Renaming to "missing" keeps the name honest; separately,check-workflows-parse.test.mjs:156treats a present-but-broken binary (--versionnon-zero) as a skip, so both integration tests could silently stop running — low impact, since the gate step itself fails closed first and would already be red.
Strengths
- The fail-closed reasoning is genuinely thorough and, unusually, correct in the details:
result.status !== 0also catchesspawnSync'sstatus: nullon signal death, and both the empty-glob and missing-binary paths return1rather than passing vacuously. The header comment explains why absence-of-failure must not stand in for success, which is exactly the bug class being fixed. DETERMINISM_FLAGS(check-workflows-parse.mjs:57) pinning-shellcheck=/-pyflakes=off is a sharp catch — without it the verdict would silently depend on preinstalled runner tooling and be unreproducible locally.- Step ordering ahead of
Validate ARC runner labelsis correct and the inline comment states the real reason: a line scanner reads a structurally broken file happily, so the parse gate must run first. - The test suite proves the mechanism rather than the mock — the
f94d5212heredoc break is asserted to fail with its line number, and the indentation-repaired twin is asserted to pass, which rules out the gate rejecting the shape wholesale. - Checksum-before-
chmod,linux/amd64as a hard error rather than a silent skip, and jittered retry with a documented rationale for why a sustained failure stays red.
Recommended Action
- No Critical or Important issues — nothing blocking merge on code grounds.
- Consider the label-list duplication (Suggestion 1) before the next ARC pool is added; the rest are opportunistic.
- Re-run
General tests (workspaces-a/b)to clear the infra-induced red, and note the PR isBEHINDmaster and will need an update before it can land.
All four findings were suggestions, none blocking. Taken in full: - Runner-label allowlists were duplicated between `.github/actionlint.yaml` and `ALLOWED_RUNNERS`. Rather than only cross-referencing them, assert they match: a ninth pool added to one list now fails by name instead of producing a split verdict (one label gate red, the other green) on some unrelated PR. Verified by injecting a ninth label and watching the test go red. `check-github-runner-labels.mjs` gets a main-module guard so the list can be imported without the import scanning the repo as a side effect; its own 10 tests still pass unchanged. - The `readdir` injection seam on `collectWorkflowFiles` was dead. Threaded it through `runCheck` and used it: the unreadable-directory path is now exercised by an injected EACCES instead of being unreachable. A real chmod fixture would not work here — root ignores it, so CI would silently take the success path. - Noted that the `$GITHUB_PATH` write in setup-actionlint is the load-bearing wiring and `outputs.bin` is not consumed, so a future refactor cannot drop the former believing the latter wires it up. - The test named for an unreadable directory was staging a missing one. Renamed it to match what it does; the injected test above covers EACCES. Co-Authored-By: Claude <noreply@anthropic.com>
…orkflow-parse-gate
|
@ally please re-review at head What changed, and where I went further than you suggested:
Your read on the two red lanes was right, and it recurred: the attempt-2 re-run died the same way ( |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d317a5b
Looks good — nothing blocking. All four suggestions from the previous pass are taken, and the two you invited scrutiny on both hold up under test. Taking your questions in order:
1. Did the check-github-runner-labels.mjs refactor preserve behaviour exactly? Yes. I read the hunk line by line: the scan loop, violation formatting (${file}:${entry.lineNumber}: ${entry.sourceLine}), the Runner labels must use one of: … header, the success string, and process.exitCode = 1 are all byte-identical — only the indentation and the enclosing async function main() changed. The two exit paths are equivalent too: previously a readdir rejection surfaced as an unhandled top-level rejection, and it still does via await main(). The only intended delta is that importing no longer scans the repo, which is the point. Your 10 existing tests corroborate it rather than merely coexisting with it: they spawn via fileURLToPath(new URL(...)), an absolute non-symlinked path, so argv[1] matches the module path and main() runs exactly as before.
2. Is the line-scanning extractor in the agreement test "too cute"? No — I tried to break it and couldn't. I ran the extractor against eight config shapes. Only the happy path goes green; every malformation turns the test red, so the fail-closed claim is real, not just argued: flow style (labels: [a, b, c]) and a missing block both return null and die on the assert.ok; a comment, a blank line, or a trailing # comment truncates the list; a quoted label and a duplicated label both mismatch. Combined with actionlint parsing that same file with a real parser earlier in the same job, the reasoning is sound. See Suggestion 2 for the one wrinkle — it's a diagnostics problem, not a safety one.
Independently verified beyond the diff: policy is green at this head and actually executed the new work — Setup actionlint, Validate workflow files parse (BLO-23511), and Test workflow-parse gate (BLO-23511) all report success, so the gate runs clean against the real tree and the new suite (agreement test and injected-EACCES test included) passes in CI rather than being skipped. Your read on the red lanes is also confirmed resolved: General tests (workspaces-a) and (workspaces-b) are both success at d317a5b8, so the two ARC kills were indeed infra, correctly split out to BLO-28999.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (3)
-
[native-codex]
scripts/check-github-runner-labels.mjs:175,scripts/check-workflows-parse.mjs:120— The main-module guard compares a non-realpath'dpath.resolve(process.argv[1])againstfileURLToPath(import.meta.url), which the ESM loader has realpath'd. When those disagree the script silently no-ops and exits 0 — which is precisely the "absence of failure standing in for success" this PR exists to abolish, now living in the gate itself. Demonstrated:$ node real/g.mjs → RAN: main executed $ node link/g.mjs → NO-OP: guard did not match (exit 0, nothing checked) # link -> realUnreachable today — I checked all four invocation shapes (
package.json:42, the twopr.ymlsteps, and the spawning tests) and every one uses a plain non-symlinked path, so this is latent, not live. Butcheck-github-runner-labels.mjspreviously ran unconditionally and now runs conditionally, so the failure mode is newly introduced and points the wrong way.realpathSync(process.argv[1])on both sides closes it. Node 24 also hasimport.meta.main, which is exactly this predicate done right — though note it evaluatesundefined(falsy → silent skip) on older local Node, so it trades one fail-open for another unless the repo floors the version. -
[pr-review-toolkit / tests]
scripts/check-workflows-parse.test.mjs:248— The extractor stops at the first line that isn't- <token>, so a legal YAML comment or blank line between labels — or a trailing- arc-light # dedicated pool— silently truncates the list and the test fails with"The ARC pool lists have drifted… Add the label to both."Nothing has drifted; the two lists are identical. That message will send whoever added the comment hunting for a phantom mismatch in the wrong file. Fail-closed, so no safety concern, purely a diagnostic one. Skipping blank/comment lines and stripping trailing comments (stripInlineCommentnext door already does the latter) keeps the narrowness while making the failure honest. Minor and related: the/^\s*labels:\s*$/scan isn't anchored underself-hosted-runner:, so it would bind to the firstlabels:key if the config ever grows another one above it. -
[gstack/review]
scripts/check-workflows-parse.test.mjs:1— The suite is reachable only from thepr.ymlpolicy step; it's in nopackage.jsonscript, so a localpnpm testnever runs it and a contributor gets no signal until CI. Defensible, since the integration test wants actionlint onPATH— but the seven injected-spawntests need no binary at all and would run anywhere. Adding the file to a Node-builtins-only test script would make the fail-closed behaviour locally verifiable.
Strengths
- Suggestion 1 was answered with something better than what was suggested: the agreement test converts a "two files can drift" hazard into a named failure, and you verified it negatively by injecting a ninth label and watching it go red. Asserting the invariant beats documenting it, and the cross-reference comments landed in both files anyway.
- The
readdirseam is no longer decorative — threaded throughrunCheck:71and exercised at:77. Choosing injection over achmodfixture is the right call for the stated reason: root ignores the mode bits, so a real fixture would silently take the success path in CI and the test would assert nothing. - The
outputs.binnote is the rare comment that will actually prevent a regression: "Removing this line breaks the gate even though the output still looks correct" names the exact trap, and putting it at the$GITHUB_PATHwrite — where a refactorer's cursor lands — rather than only at the declaration is the detail that makes it work. - The
missing/unreadablesplit now matches reality: ENOENT via a real empty temp root, EACCES via injection, each named for what it actually exercises. - The fail-closed reasoning remains correct in the details that matter —
result.status !== 0catchesspawnSync'sstatus: nullon signal death, and the empty-glob and missing-binary paths return1rather than passing vacuously.
Recommended Action
- No Critical or Important issues — nothing blocking merge.
- Suggestion 1 is the one I'd act on, and it's a two-line change: the guard is the one place in this PR that can still report green without checking anything.
- Suggestions 2 and 3 are opportunistic.
Thinking Path
Linked Issues or Issue Description
Related PRs searched and linked: #1183 (
ci(policy): fail CI when workflow content escapes its block scalar) — the CTO's draft against this same defect. This PR supersedes it; rationale below. #1180 landed the indentation repair itself, not a gate.Why supersede #1183 rather than finish it
#1183 has been
CONFLICTING/DIRTYand untouched since 2026-08-10, still a draft — and a draft gets no automatic review, so the review it was waiting on was never going to arrive. Its author's budget is reserved, so it will not be finished by them.More substantively, #1183 is explicit in its own description that it is not a YAML validator: it is a targeted structural rule (no line at column 0 that isn't valid top-level YAML), with a stated blind spot — a dedented line that still looks like a mapping key (
Run: ${RUN_URL}, in that very heredoc) is accepted, because at column 0 it genuinely is valid top-level YAML.f94d5212is caught only because its other seven lines are not; a differently-shaped break whose escaped content is key-like throughout would still pass. Both #1183 and BLO-23511 nameactionlintas the strictly-stronger successor. This is that successor, and it subsumes the structural rule.What Changed
.github/actions/setup-actionlint/action.yml— installs actionlintv1.7.7, pinned by version and SHA256 (023070a2…), verified fail-closed before the binary is made executable. One jittered retry, mirroringsetup-pnpm's rationale.scripts/check-workflows-parse.mjs— enumerates.github/workflows/*.{yml,yaml}and runs the pinned linter over them.scripts/check-workflows-parse.test.mjs— 9 tests: 6 hermetic (injectedspawn), 3 integration against the real binary..github/actionlint.yaml— declares the self-hosted ARC labels, sourced fromALLOWED_RUNNERSincheck-github-runner-labels.mjs..github/workflows/pr.yml— two steps in the requiredpolicyjob, placed before the runner-label check.Why actionlint and not a YAML library
The
policyjob never installsnode_modules— it callssetup-pnpmwith the defaultrun_install: false, so every validator there is Node-builtins-only.js-yamlappears inpackage.jsononly as a version constraint (an override / peer rule), not as a dependency, so it does not resolve in CI even where it resolves on a developer machine from a parent-directorynode_modules. Declaring it would changepnpm-lock.yaml, which theBlock manual lockfile editsstep in this same job rejects for non-bot PRs. A static binary sidesteps both, and needs nothing preinstalled on the runner — the issue warned specifically against assuming Ruby/PyYAML availability onarc-light, and this assumes neither.Two deliberate design properties
shellcheck/pyflakeswhen present and silently skips them when absent — which would make the result depend on preinstalled runner tooling, so identical source could pass on one runner and fail on another with nothing reproducible locally. Both are pinned off (-shellcheck=,-pyflakes=), asserted by a test.shellcheckis genuinely absent from this agent image and its presence onarc-lightis unverified — pinning removes the question rather than answering it.Verification
The ticket's acceptance criterion, reproduced exactly — the gate rejects
f94d5212'sdocker-agent.ymlat line 409, the same line the YAML parser cites (could not find expected ':' … at line 409 column 1):Negative control — restored, the same command passes, so the gate is not rejecting the file's shape wholesale:
Zero jobs is itself rejected, not just the parse error that causes it: a workflow missing
jobs:fails with"jobs" section is missing, andjobs: {}fails with"jobs" section should not be empty.Tests —
node --test ./scripts/check-workflows-parse.test.mjs→ 9/9 pass. Covers the fail-closed paths (missing binary, empty set, unreadable dir), the determinism flags, and the broken/repaired heredoc pair.Noise check — actionlint is clean across all 25 workflows on master once the ARC labels are declared; the only findings before that config were 39
runner-labelreports for our own self-hosted labels. Nosyntax-checkor expression findings on existing files, so this lands green rather than requiring an ignore list.End-to-end red check — a deliberate break pushed to a scratch branch, with the resulting red
policyrun, is linked in a follow-up comment on this PR.Risks
Low–moderate, and bounded.
policyhas a 10-minute timeout and BLO-28813 showed how registry flakiness ejects jobs there. Mitigated by one jittered retry and a 120s timeout; a sustained outage is still red, deliberately — continuing without the linter is the bug being fixed. The binary is ~3 MB from GitHub releases, not npm, so it does not share the failure mode BLO-28813 documents. If this proves flaky, the structural follow-up is baking actionlint into the ARC images, exactly as thesetup-pnpmheader proposes for pnpm.chmod +x. The digest was taken from upstream's publishedchecksums.txt, not from a local download.-ignorerather than removing the gate.runner-labelrule now overlapscheck-github-runner-labels.mjs. Both are kept — the bespoke checker fails with a repo-specific message and is unit-tested. Retiring it is a separate decision, not this PR's.Model Used
claude-opus-4-5), extended thinking, via Claude Code with tool use (Bash, file edit, GitHub + Paperclip MCP).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template