ci(policy): fail CI when workflow content escapes its block scalar (BLO-23128) - #1183
ci(policy): fail CI when workflow content escapes its block scalar (BLO-23128)#1183allyblockcast[bot] wants to merge 5 commits into
Conversation
…LO-23128) An unparseable workflow file is invisible to every existing gate. GitHub does not report "your workflow is invalid" — it manufactures a run that fails instantly with zero jobs, so there is no failing job for a required check to notice. BLO-23128's own fix hit exactly this: f94d521 wrote an alert-on-failure heredoc body and its closing EOF at column 0 inside a `run: |` block scalar, which terminates the scalar and corrupts the document ("could not find expected ':' ... at line 423 column 1"). Agent image delivery — build, promote, and the cluster ref bump — stayed dead until a human found it by hand. The alert-on-failure job added to catch a red pipeline could never itself run, because the file it lives in does not parse. Adds check-workflow-structure.mjs to the required `policy` job: a non-blank line at column 0 must be a valid top-level YAML construct. The `policy` job never installs node_modules, so this uses Node builtins only rather than a YAML library (js-yaml is in the lockfile only transitively). Verified against every workflow in the repo: rejects the broken docker-agent.yml on master (7 lines, first at 423 — the exact parse location), and passes on all 20 files once #1180's indentation fix is applied. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-23128 |
1 similar comment
|
🔗 Paperclip issue: BLO-23128 |
|
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 |
|
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 |
|
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 |
|
Picked this up because it was red and untouched for ~17h. The gate is sound — nothing here needed a code change. Both reds were external. Red #1:
|
| target | ruby YAML.load_file (independent oracle) |
this gate |
|---|---|---|
docker-agent.yml @ f94d521 |
could not find expected ':' … line 409 column 1 |
exit 1, first offender line 409 (+6 more, incl. the stray EOF) |
| all 20 workflows @ current master | parses | exit 0 |
The gate agrees with a real YAML parser on the exact line, and is clean on today's master. That is the negative control this change needed.
Red #2: ERR_PNPM_LOCKFILE_CONFIG_MISMATCH — a repo-wide window, not this PR
After the first branch update, every job failed on install. Not caused by this PR: its package.json and pnpm-lock.yaml blobs were byte-identical to master's. Master itself was inconsistent:
10b50b1f(08-09 23:02) added thenanoidoverride topackage.jsonwithout the lockfilecfe09406(08-10 03:56) "chore(lockfile): refresh pnpm-lock.yaml" healed it
In that ~5h window four branches and merge-queue entry pr-1236 failed every job the same way. Second branch update picked up the refresh; Build/Typecheck/e2e/shards all green now.
Red #3: one flaky shard — security-audit-overrides
General tests (server 4/4): 1 failed / 1433 passed. The failure is Error: Command failed: pnpm install --lockfile-only --ignore-scripts --no-frozen-lockfile — and that is the whole message, because promisify(execFile) discards pnpm's stderr. Filed BLO-24012 with a fix.
Non-deterministic rather than a regression: pen-2074/release-external-wait-slo was fully green 40 min later, and fix/blo-19123 failed different shards (2/4, 3/4). Master's inputs are correct — override nanoid: ">=5.1.16 <6", lockfile resolves exactly nanoid@5.1.16.
State / next action
- Branch updated twice →
behind_by: 0(required, sinceverifyisstrict: true). - Re-ran the failed shard; result pending as of this comment.
- Still a draft, and I have deliberately not marked it ready or queued it — that is the author's call, not mine.
Owner decision: if the rerun is green, this is ready to un-draft. One substantive review point, not a blocker — the script's own header already says it: this is a targeted structural rule, not a YAML parse, and it accepts a dedented line that happens to look like a mapping key. It rejects the real broken file on its other lines, so it would have caught BLO-23128. actionlint pinned by checksum remains the strictly-stronger follow-up (BLO-23511).
|
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 |
|
Rerun result: still red, still not this PR — and the second failure has a different cause from the first, which is itself the useful finding.
The security-audit test passed on the rerun. What failed instead was a timing-bounded test, on a run where The shard took 70m15s wall-clock against a 90-minute job cap, so it came close to being killed outright — which would have produced no vitest summary at all, and therefore no failing-test name to read. Filed BLO-24012 on the first one (the test discards pnpm's stderr, so its failure message is only StateBranch updated again — the 07:33 batch of 5 merges had put it Next action / ownerThis is now blocked on CI capacity, not on anything in the diff. I've spent two full runs establishing that; I'm not going to keep re-running it, because each attempt costs ~70 min of a constrained pool and the failures are drawn from a different random test each time. Recommendation for whoever owns this: the gate itself is verified correct (it agrees with a real YAML parser on the exact line of the |
|
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 |
|
Third run, third external failure. I'm stopping here rather than spending a fourth — at this point I'd be rolling dice with scarce runner capacity, not testing anything about this diff.
Four runs, four different external causes, and not one failure in a file this PR touches (3 files, all under Two notes on reading that last run: its overall conclusion shows
Where this leaves the PRThe gate itself is verified correct and I'd merge it on any green run:
Branch is at Suggested next action for the owner: un-draft, then merge on the first run where |
Superseded by #1429 (BLO-23511)Heads up — not a review request, and no action needed from you. This PR is I have landed the successor this PR itself names: #1429 — Your diagnosis held up completely, and #1429 is built on it:
Leaving this open for now — I will not close someone else's PR on the strength of an unmerged one. Once #1429 lands on master I will close this as superseded, unless you would rather close it yourself. Refs BLO-23511 |
1 participant
Problem
An unparseable workflow file passes every gate this repo has. GitHub does not surface "your workflow is invalid" — it manufactures a run that fails instantly with zero jobs, so there is no failing job for a required check to catch.
BLO-23128's own fix hit exactly this.
f94d521wrote thealert-on-failureheredoc body and its closingEOFat column 0 inside arun: |block scalar. Column 0 terminates the block scalar, so YAML reads the prose as a new top-level key:Agent image delivery — toolchain build,
Promote verified agent image,Bump agent image refs in cluster— stayed dead. The irony is load-bearing: thealert-on-failurejob added to stop a red pipeline going unnoticed can never run, because the file it lives in does not parse. That is the same failure mode BLO-23128 was opened for.The cheap tell for this class: an instant 0-job run where
created_at == updated_at, and the workflow's name reported as its path rather than itsname:.Change
Adds
.github/scripts/check-workflow-structure.mjsto the requiredpolicyjob. Rule: a non-blank line at column 0 must be a valid top-level YAML construct (mapping key, comment, or document marker). Anything else means content has escaped the block scalar meant to contain it.Why this rule and not a real YAML parse
The
policyjob never installsnode_modules— every validator there is Node-builtins-only — andjs-yamlis present in the lockfile only transitively, not as a resolvable direct dependency. This rule needs no parser, is decidable from the text alone, and has zero false positives across the repo.Stated limit, deliberately: this is a targeted structural gate, not a YAML validator. A dedented line that still looks like a mapping key (
Run: ${RUN_URL}, in this very heredoc) is accepted in isolation, because at column 0 it genuinely is valid top-level YAML. It is the file that must be rejected, and the broken file is rejected on its other seven lines. A fullactionlintgate is the strictly-stronger follow-up once a pinning/vendoring decision is made.Verification
docker-agent.ymlas it stands on master: 7 lines, first at 423 — the exact location the parser cites.node --test), including the exact broken/fixed heredoc pair.check-github-runner-labels.mjsstill green; the rule is dogfooded againstpr.ymlitself.Sequencing — why this is a draft
policyon this branch correctly fails right now, because master genuinely is broken. This must rebase onto a master that already contains #1180 (@kkroo's indentation repair). I have deliberately not included that fix here so it stays #1180's to land. Once #1180 merges I'll rebase and mark this ready.Refs BLO-23128.
🤖 Generated with Claude Code