ci: close the if that cost last night's release, and parse-check the rest - #122
Merged
Conversation
…rest The Collect guard added to the publish job in #121 shipped without its closing `fi`. Run 32096242353 -- the first nightly to reach the new single publish job -- built all 107 devices, staged 219 assets, and then died in `bash -e` with line 25: syntax error: unexpected end of file from `if' command on line 14 before writing anything. No nightly-20260818-* release exists, `nightly` and `latest` still carry the 17th's images, and ci-gate went red. The gate did its job; there was simply nothing left to publish by then. Nothing caught it earlier because the publish job carries `if: github.event_name != 'pull_request'` -- there are no artifacts to publish on a PR, so the entire path is unreachable from PR CI by construction. Both of #121's runs were green without ever executing the step. #121 said as much ("none of this path can run on a pull request") and checked the four states offline, but offline-checking the logic does not check that the script parses, and that is what broke. So the one-line fix, plus the check that would have caught it in seconds without a runner: lint-workflow-shell.py parses every `run:` block in .github/workflows/ with `bash -n`, and lint.yml runs it on PRs and on master pushes that touch either. Notes on the checker: - ${{ }} is not shell, so each expression is replaced with a plain word first. That is a real limitation -- an expression interpolating shell syntax is checked as the word, not as what it expands to -- and the substitution preserves line counts so reported lines still point at the right line. --self-test asserts an unterminated `if` containing an expression is still rejected while the closed form passes, because the way this breaks is by making everything pass. - Steps are found by walking for any mapping with a `run:` key rather than by the jobs.*.steps[*] path, so nothing depends on guessing where they live, and a discovery floor fails the run if the walk stops finding them. A checker that silently checks nothing looks exactly like a clean tree. - Syntax only. It says nothing about quoting or `-e` semantics; a shellcheck pass wants per-block disables for the substitution above and is a separate change. Verified: the checker flags the merged master.yml with the same message the runner gave, passes the fixed tree (22 blocks), and all five guard states behave as #121 intended -- normal night publishes, a broken download and a green-but-empty dist both fail, and a dead matrix with nothing built still exits clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoFix broken publish guard and add CI lint for workflow
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
The new lint.yml and lint-workflow-shell.py were names ci-matrix.py had never heard of, and unknown widens by design -- so the first push of the previous commit queued all 107 devices to prove a one-line YAML fix. #121, which touched only master.yml, ran 19. Neither file can change a byte of what a build produces, which is exactly what NO_BUILD_WORKFLOWS and NO_BUILD_SCRIPTS are for. Self-test cases added for both, alongside the manifest pair they mirror. This PR still takes the full matrix, because it now edits ci-matrix.py itself and that always widens -- the selector is not trusted to pick a smaller matrix for its own changes. That rule is working as intended; the classification only takes effect for later changes to these files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Both from Qodo's review of #122. PyYAML was an undeclared dependency. The job passed twice because ubuntu-latest happens to ship it, which is the problem rather than the defence: the check stops being about this repo the day the image drops it. Installed explicitly via apt -- 24.04 is PEP 668, and it matches the busybox install in OpenIPC/firmware's shell-tests. The import is guarded too, so running this locally without PyYAML says what to install instead of printing a traceback. The workflow declared no permissions, so its token was whatever the repo or org default is. It reads the tree and reports; contents: read. Neither was going to fail the job today. The first fails loudly rather than quietly if it ever fires, which is the right direction, but an undeclared dependency and an inherited token are both the kind of thing that is free to fix now and annoying to diagnose later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
Both findings addressed in 71eaed7. 1. PyYAML not installed (High) — real, and the fact that the job passed twice is the point: 2. Implicit token permissions (Medium) — added Neither would have failed the job today. Both fail loudly rather than quietly if they ever fire, which is the right direction, but an undeclared dependency and an inherited token are cheap now and annoying to diagnose later. Re-verified after the change: self-test passes, full scan clean at 23 run blocks (the new apt step adds one), the linter still flags the merged |
widgetii
added a commit
that referenced
this pull request
Aug 18, 2026
The unconditional `apt-get update` added to satisfy review on #122 was fine on the PR runs (~5s) and then sat for six minutes on the first master push, still in progress when it was cancelled. Nothing was wrong with the tree; apt was just slow. That is a bad trade for this job in particular. Its argument for existing -- and for carrying a push trigger at all -- is that it answers in seconds, and it was made to depend on a network fetch it does not normally need. ubuntu-latest ships PyYAML. So import first and install only if that fails. The dependency is still handled rather than assumed, which was the point of the review finding, but a working runner pays nothing for it. `if` rather than `python3 -c 'import yaml' && exit 0`, because under `bash -e` the latter fails the step on the branch where the import fails -- exactly when the install needs to run. Both paths checked under -e. Also timeout-minutes: 10. The default is six hours, which is how a step that hangs rather than fails occupies a runner and tells nobody. The whole job now completes in three seconds, and selects 0 devices rather than 107 because #122 classified lint.yml as no-build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 19, 2026
widgetii
added a commit
that referenced
this pull request
Aug 19, 2026
Two bugs in the linter #122 added, found by Qodo reviewing the port of it to OpenIPC/firmware#2290 and confirmed against this copy rather than taken on trust. walk() collected any mapping with a scalar `run` key, anywhere in the document. That is one key name away from linting things that are not shell: an action input, an env var or a matrix field called `run` holds arbitrary text, and feeding it to bash -n fails a workflow that is fine. On a fixture with one real step plus an `env: run:` and a `with: run:`, this copy collects three blocks. Steps now have to come out of a `steps:` sequence, which is the actual schema invariant and still not a hardcoded jobs.*.steps[*] path, so composite actions (`runs: steps:`) keep working. The expression substitution used `\$\{\{.*?\}\}`, which stops at the first `}}` even when it is inside a string literal. On x=${{ fromJSON('{"a": {"b": 1}}') }} this copy produces `x=__GHA_EXPR__') }}` and bash -n then reports an unbalanced quote -- a false failure on a valid block, in the direction that trains people to ignore the job. Replaced with a scanner that tracks GitHub's single-quoted strings, including the doubled '' escape, and returns the text untouched if an expression is never closed. Both are latent here: nothing in this repo has a `run` key outside a step or a `}}` inside an expression string, and the block count is unchanged at 23. They are fixed because the failure mode is a red job with nothing wrong with the tree, which is how a check stops being believed. Self-tests for both. The docstring described the behaviour this replaces, so it is corrected too, and the now-unused `re` import drops. Keeps this copy identical to firmware's apart from the incident paragraph and the block floor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
widgetii
added a commit
to OpenIPC/firmware
that referenced
this pull request
Aug 19, 2026
The publish job in build.yml carries `if: github.event_name != 'pull_request'` -- there are no artifacts to publish on a PR, so the whole path is unreachable from PR CI by construction. Every edit to it ships unexecuted, and the first thing to run it is the 22:30 cron. That is not hypothetical. OpenIPC/builder runs the same publish job, ported from this one, and OpenIPC/builder#121 edited its Collect step and shipped an `if` with no closing `fi`. Both PR runs were green without ever reaching the step. The nightly then built all 107 devices, staged 219 assets, and died in `bash -e` with "syntax error: unexpected end of file" before writing a single release. Nothing published that night. The guard that broke there is the one this repo backported in #2279, so the same typo is one edit away here, in a job whose failures surface at 22:30 to nobody. lint-workflow-shell.py parses every `run:` block in .github/workflows/ with `bash -n`; lint.yml runs it on PRs and on master pushes that touch either. Ported from OpenIPC/builder#122, #123 and #124. The 43 run blocks already in this repo all parse clean, so this lands green -- it is a guard against the next edit, not a fix for a current break. Confirmed it would earn its keep by removing the `fi` from this repo's own Collect guard in a scratch copy: flagged at build.yml:376 (Collect assets), with the same message the runner gives. Notes on the checker: - A step is a mapping with a scalar `run:` that came out of a `steps:` sequence, which is the schema invariant and no more than that. Collecting any `run` key anywhere would lint an action input, an env var or a matrix field called `run` as shell and fail a workflow that is fine; requiring the `steps:` parent still leaves composite actions (`runs: steps:`) covered by the same rule. - ${{ }} is not shell, so each expression is replaced with a plain word first. Where one ends is scanned rather than regexed: `.*?\}\}` stops at the first `}}` even inside a string literal, so `${{ fromJSON('{"a": {"b": 1}}') }}` would be cut mid-literal and the leftover `') }}` fails as an unbalanced quote. Line counts are preserved so reported lines still point at the right line. - --self-test runs before the linter in CI and asserts an unterminated `if` containing an expression is still rejected while the closed form passes, because the way this breaks is by making everything pass. A discovery floor fails the run if the walk stops finding blocks: a checker that silently checks nothing looks exactly like a clean tree. - Syntax only. It says nothing about quoting or `-e` semantics. actionlint would cover more and is worth considering separately. Its own file rather than a fourth job in shell-tests.yml, which deliberately has no push trigger: sharing one would start running the busybox and sysupgrade jobs on every master push too. PyYAML is imported first and installed only if that fails, because an unconditional `apt-get update` in this step sat for six minutes on builder's first master push. ci-matrix.py classifies both new files as no-build; unknown widens, and they cannot change a byte of what reaches a camera. This PR still took the full matrix because it edits ci-matrix.py itself, which always widens -- the selector is not trusted to pick a smaller matrix for its own changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What broke
The Collect guard added to the publish job in #121 shipped without its closing
fi.Run 32096242353 — the first nightly to reach the new single publish job — built all 107 devices, staged 219 assets, and then died in
bash -e:Nothing was written. There is no
nightly-20260818-*release;nightlyandlateststill carry the 17th's images (assets last touched2026-08-17T04:19).manifest.ymlran afterwards and re-indexed yesterday's data. ci-gate went red — the gate did its job, there was simply nothing left to publish by the time it ran.Without this, tonight's 03:00 cron fails the same way.
Why CI was green
The publish job carries
if: github.event_name != 'pull_request'— there are no artifacts to publish on a PR, so the whole path is unreachable from PR CI by construction. Both of #121's runs passed without ever executing the step.#121 was explicit about this ("none of this path can run on a pull request") and checked the four guard states offline. But offline-checking the logic doesn't check that the script parses, and that's what broke. Any edit to that job ships unexecuted.
What's here
fiinmaster.yml..github/scripts/lint-workflow-shell.py— parses everyrun:block in.github/workflows/withbash -n..github/workflows/lint.yml— runs it on PRs and on master pushes touching either file. Seconds, no runners, no matrix.Notes on the checker
${{ }}is not shell, so each expression is replaced with a plain word first. Real limitation: an expression interpolating shell syntax is checked as the word, not as what it expands to — the same limitationbash -nhas with any variable. The substitution preserves line counts so reported lines still point at the right line of the workflow.--self-testruns first in CI. The way this class of checker breaks is by making everything pass, which is indistinguishable from a clean tree. So it asserts an unterminatedifcontaining an expression is still rejected while the closed form passes.run:key — not by thejobs.*.steps[*]path, so nothing depends on guessing where they live. A discovery floor fails the run if the walk stops finding them.-esemantics. A shellcheck pass wants per-block disables for the substitution above and is a bigger, separate change.Verification
master.ymlwith the same message the runner gave, pointing atmaster.yml:354 (Collect assets)Related
OpenIPC/firmware's backport (#2279) has thefiand is unaffected — its nightly ran clean at 22:46 UTC: 397 assets collected, paced uploads, dated release with 109 images + 96 sizes sidecars, both alias tags refreshed and moved to the built commit. Firmware has no equivalent workflow-shell lint either, so the same gap exists there; worth porting separately.🤖 Generated with Claude Code