ci: parse-check the shell in workflows, the way builder now does - #2290
Merged
Conversation
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 and #123. 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: - ${{ }} 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, 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. 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 takes 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>
PR Summary by QodoAdd CI syntax checks for workflow shell blocks
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. lint-workflow-shell.py lacks build wiring
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Two Qodo findings on #2290, both real, both confirmed against the shipped implementation 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 the fixture below the old walk collects three blocks where there is one real step. 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}}') }} the old code 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 currently has a `run` key outside a step or a `}}` inside an expression string, and the block count is unchanged at 46. They are fixed because the failure mode is a red job with nothing wrong with the tree. Self-tests for both, and they do fail against the old implementation -- checked by importing builder's copy, which still carries it. That copy needs the same fix. Not addressed: the third finding says a .py under .github/scripts/ needs a package Config.in, .mk rule and a defconfig selecting it. That rule is about firmware sources; this is CI tooling that never reaches an image, and ci-matrix.py and enrich_manifest.py sit in the same directory under the same terms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DISCOVERY and EXPRESSIONS paragraphs still described the behaviour ef9a9c1 replaced -- "any mapping anywhere in the document that has a run: key", and a substitution with no mention of why it is scanned rather than matched. A docstring that describes the previous version is worse than none, since it is the thing the next person reads before deciding what is safe to change. Also drops the now-unused `re` import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
widgetii
added a commit
to OpenIPC/builder
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>
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.
Port of OpenIPC/builder#122 and #123.
Why
The publish job in
build.ymlcarriesif: 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 isn't hypothetical. OpenIPC/builder runs the same publish job, ported from this one, and OpenIPC/builder#121 edited its Collect step and shipped an
ifwith no closingfi. Both PR runs were green without ever reaching the step. The nightly then built all 107 devices, staged 219 assets, and died inbash -ewithsyntax error: unexpected end of filebefore writing a single release. Nothing published that night.The guard that broke there is the one this repo backported in #2279. The same typo is one edit away here, in a job whose failures surface at 22:30 to nobody.
What
.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 workflows or the linterci-matrix.py— classifies both as no-buildThis lands green
All 43 run blocks already in this repo parse clean. This is a guard against the next edit, not a fix for a current break.
To confirm it earns its keep, I removed the
fifrom this repo's own Collect guard in a scratch copy:— the same message the runner gives, exit 1.
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 substitution preserves line counts so reported lines still point at the right line.--self-testruns first in CI. This class of checker breaks 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 — plus a discovery floor, so a broken walk fails instead of reporting green.-esemantics.actionlintwould cover more and is worth considering separately.Two deliberate choices
Its own file, not a fourth job in
shell-tests.yml. That file has no push trigger by design; sharing one would start running the busybox and sysupgrade jobs on every master push too. The push trigger matters here because its absence is how builder#121 reached master broken — PR coverage only holds if master hasn't moved since the last PR run.PyYAML is imported first and installed only on failure. An unconditional
apt-get updatein this step sat for six minutes on builder's first master push (builder#123), on a job whose whole argument for existing is that it answers in seconds.ifrather thancmd && exit 0, because underbash -ethe latter fails the step on exactly the branch where the install needs to run.Cost
This PR takes the full 99-board matrix because it edits
ci-matrix.py, which always widens — the selector isn't trusted to pick a smaller matrix for its own changes. Future changes to the linter orlint.ymlselect 0 boards. Landing the classification separately isn't possible: the selector's self-test asserts classified files exist.🤖 Generated with Claude Code