From 0455db00274f46dff4006b6a8cce6d9d45878fbb Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:44:03 +0300 Subject: [PATCH] ci: close the if that cost last night's release, and parse-check the rest (#122) 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. actionlint would cover more and is worth considering later; keeping this narrow is what makes it noise-free now. ci-matrix.py classifies both new files as no-build. They were names it had never heard of, and unknown widens by design, so the first push queued all 107 devices to prove a one-line YAML fix -- #121, which touched only master.yml, ran 19. 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. PyYAML is installed explicitly rather than inherited from the runner image, and the workflow declares `contents: read` rather than taking the repo default. Both from review; neither would have failed the job today. Verified: the checker flags the merged master.yml with the same message the runner gave, passes the fixed tree, 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 --- .github/scripts/ci-matrix.py | 6 +- .github/scripts/lint-workflow-shell.py | 313 +++++++++++++++++++++++++ .github/workflows/lint.yml | 56 +++++ .github/workflows/master.yml | 1 + 4 files changed, 374 insertions(+), 2 deletions(-) create mode 100755 .github/scripts/lint-workflow-shell.py create mode 100644 .github/workflows/lint.yml diff --git a/.github/scripts/ci-matrix.py b/.github/scripts/ci-matrix.py index 0fbceea..a3f7626 100644 --- a/.github/scripts/ci-matrix.py +++ b/.github/scripts/ci-matrix.py @@ -78,8 +78,8 @@ NOT_BUILT = { # whole filename, never as a prefix: one this list has never heard of is unknown, # and unknown widens. package.sh and repack.sh are developer and end-user tools # that no workflow invokes -- only builder.sh is on the CI path. -NO_BUILD_WORKFLOWS = {"build-one.yml", "cleanup.yml", "manifest.yml"} -NO_BUILD_SCRIPTS = {"enrich_manifest.py"} +NO_BUILD_WORKFLOWS = {"build-one.yml", "cleanup.yml", "lint.yml", "manifest.yml"} +NO_BUILD_SCRIPTS = {"enrich_manifest.py", "lint-workflow-shell.py"} NO_BUILD_FILES = { ".github/CODEOWNERS", ".gitignore", "LICENSE", "package.sh", "repack.sh", } @@ -407,6 +407,8 @@ def self_test(): ([".github/CODEOWNERS"], 0, "codeowners"), ([".github/workflows/manifest.yml"], 0, "manifest never builds"), ([".github/scripts/enrich_manifest.py"], 0, "manifest script"), + ([".github/workflows/lint.yml"], 0, "the workflow linter never builds"), + ([".github/scripts/lint-workflow-shell.py"], 0, "its script"), (["repack.sh"], 0, "end-user tool, no workflow runs it"), (["package.sh"], 0, "developer tool, no workflow runs it"), (["archive/gk7205v200_fpv/202607231714/openipc.tgz"], 0, "build output"), diff --git a/.github/scripts/lint-workflow-shell.py b/.github/scripts/lint-workflow-shell.py new file mode 100755 index 0000000..6d1fe0e --- /dev/null +++ b/.github/scripts/lint-workflow-shell.py @@ -0,0 +1,313 @@ +#!/usr/bin/env python3 +"""Parse-check every `run:` block in .github/workflows/. + +WHY THIS EXISTS. The publish job in master.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. #121 +edited that job's Collect step, both PR runs went green without ever executing +it, and the merge shipped an `if` with no closing `fi`. The first thing to +actually run the step was the 03:00 nightly: 107 devices built, 219 assets +staged, and the job died in `bash -e` with "syntax error: unexpected end of +file" before writing a single release. Nothing published that night. + +A shell typo in a workflow is therefore not caught by the workflow's own CI -- +it is caught by whatever fires that branch first, which for the release path is +a cron nobody is watching at 03:00. This is the cheap check that closes that +gap: it needs no runners, no artifacts, and no matrix, and it would have failed +#121 in seconds. + +SCOPE is syntax only, deliberately. `-n` parses without executing, so a block +that passes here can still be wrong at runtime, and this says nothing about +quoting, unset variables, or the `-e` semantics that bite in these scripts. A +shellcheck pass over the same blocks is a strictly bigger change (it wants +per-block disable lists for the ${{ }} substitution below) and is worth doing +separately rather than smuggling in behind a syntax fix. + +DISCOVERY is by structure, not by path: any mapping anywhere in the document +that has a `run:` key is a step. Hardcoding jobs.*.steps[*] would quietly miss +anything that moves -- composite actions, reusable workflows, a `defaults` +refactor -- and silently checking nothing looks exactly like a clean run, which +is the failure mode this file is here to prevent. Hence the floor check at the +end. + +EXPRESSIONS. `${{ ... }}` is not shell and cannot be parsed as shell, so each +one is replaced with a plain word before the check. That is a real limitation: +an expression that interpolates to something with shell syntax in it -- a +matrix value holding `foo && bar`, say -- is checked as the word, not as what +it expands to. It is the same limitation `bash -n` has with any variable, and +the substitution keeps the line count identical so reported line numbers still +point at the right line of the workflow. --self-test asserts the substitution +did not neuter the check. + +Usage: + lint-workflow-shell.py # check .github/workflows/ + lint-workflow-shell.py PATH... # check specific files + lint-workflow-shell.py --self-test # check this checker still catches things +""" + +import glob +import re +import subprocess +import sys +import tempfile +from pathlib import Path + +try: + import yaml +except ImportError: # pragma: no cover - depends on the host, not the tree + sys.exit( + "lint-workflow-shell: PyYAML is required and missing.\n" + " CI installs it (see .github/workflows/lint.yml); locally:\n" + " apt install python3-yaml # or: pip install pyyaml" + ) + +# Steps with no `shell:` run under bash on a Linux runner; GitHub's default is +# `bash -e {0}`. The -e changes runtime behaviour, never parsing, so -n alone is +# the right check for both. +DEFAULT_SHELL = "bash" + +# How the `shell:` value maps to a parser. Anything not listed is skipped and +# reported, so a step that starts using `python` or `pwsh` shows up as a gap +# rather than being silently dropped. +PARSERS = { + "bash": ["bash", "-n"], + "sh": ["sh", "-n"], +} + +EXPR = re.compile(r"\$\{\{.*?\}\}", re.DOTALL) + +# The floor below is a discovery guard, not a target: it only has to catch "the +# walk broke and found nothing", which is the case that would otherwise report +# green. Deliberately well under the current count (20 across the four +# workflows) so ordinary churn never trips it, and only applied to a full-tree +# scan -- checking one file by hand legitimately finds fewer. +MIN_BLOCKS = 10 + + +def substitute(text): + """Replace every ${{ }} with a word, preserving the line count. + + Line numbers have to survive or the errors point at nothing. A multi-line + expression is replaced by the word plus the newlines it spanned. + """ + + def repl(m): + return "__GHA_EXPR__" + "\n" * m.group(0).count("\n") + + return EXPR.sub(repl, text) + + +def walk(node, shell, out): + """Collect (run_text, shell, line) for every step under `node`. + + `shell` is the default inherited from an enclosing `defaults.run.shell`, + which is how both workflow-level and job-level defaults reach a step + without this needing to know where in the document it is. + """ + if isinstance(node, yaml.MappingNode): + keys = {} + for k, v in node.value: + if isinstance(k, yaml.ScalarNode): + keys[k.value] = v + + # A `defaults: run: shell:` here applies to everything below it. + defaults = keys.get("defaults") + if isinstance(defaults, yaml.MappingNode): + for dk, dv in defaults.value: + if isinstance(dk, yaml.ScalarNode) and dk.value == "run": + if isinstance(dv, yaml.MappingNode): + for rk, rv in dv.value: + if ( + isinstance(rk, yaml.ScalarNode) + and rk.value == "shell" + and isinstance(rv, yaml.ScalarNode) + ): + shell = rv.value + + run = keys.get("run") + if isinstance(run, yaml.ScalarNode): + step_shell = shell + sh = keys.get("shell") + if isinstance(sh, yaml.ScalarNode): + step_shell = sh.value + name = keys.get("name") + label = name.value if isinstance(name, yaml.ScalarNode) else "" + out.append( + { + "text": run.value, + "shell": step_shell, + "line": run.start_mark.line + 1, + "name": label, + } + ) + + for _, v in node.value: + walk(v, shell, out) + + elif isinstance(node, yaml.SequenceNode): + for v in node.value: + walk(v, shell, out) + + +def parse_error(text, shell): + """Return bash's complaint about `text`, or None if it parses.""" + # `shell: bash` and `shell: bash {0}` and `shell: bash --noprofile {0}` all + # mean bash; take the program name and ignore the argument template. + prog = (shell or DEFAULT_SHELL).split()[0] + argv = PARSERS.get(prog) + if argv is None: + return ("skip", prog) + + body = substitute(text) + with tempfile.NamedTemporaryFile("w", suffix=".sh", delete=False) as f: + f.write(body) + tmp = f.name + try: + p = subprocess.run( + argv + [tmp], capture_output=True, text=True, timeout=30 + ) + if p.returncode == 0: + return None + # Rewrite the temp path out of the message; the caller adds the real + # location. Keep the shell's own line number, it is the offset into the + # block and the substitution preserved it. + return ("fail", p.stderr.replace(tmp, "").strip()) + finally: + Path(tmp).unlink(missing_ok=True) + + +def check(paths, enforce_floor): + blocks = 0 + failures = 0 + skipped = [] + + for path in paths: + text = Path(path).read_text() + try: + root = yaml.compose(text) + except yaml.YAMLError as e: + print(f"FAIL {path}: not parseable as YAML: {e}") + failures += 1 + continue + if root is None: + continue + + found = [] + walk(root, DEFAULT_SHELL, found) + + for b in found: + blocks += 1 + where = f"{path}:{b['line']} ({b['name']})" + result = parse_error(b["text"], b["shell"]) + if result is None: + print(f"ok {where}") + elif result[0] == "skip": + skipped.append(f"{where} [shell: {result[1]}]") + else: + print(f"FAIL {where}") + for line in result[1].splitlines(): + print(f" {line}") + failures += 1 + + print() + if skipped: + print(f"skipped {len(skipped)} block(s) in a shell this does not parse:") + for s in skipped: + print(f" -- {s}") + print() + + print(f"checked {blocks} run block(s)") + + # A walk that matched nothing is indistinguishable from a clean run unless + # something asserts it found the work. See the module docstring. + if enforce_floor and blocks < MIN_BLOCKS: + print( + f"FAIL only {blocks} run block(s) discovered, expected at least " + f"{MIN_BLOCKS}; the walk has regressed rather than the workflows " + f"having shrunk that far." + ) + return 1 + + if failures: + print(f"{failures} block(s) failed to parse") + return 1 + + print("all run blocks parse clean") + return 0 + + +def self_test(): + """Prove the checker still rejects what it is supposed to reject. + + The substitution in particular is easy to break in a way that makes + everything pass -- replace ${{ }} with something that swallows the rest of + the line and every block parses forever after. + """ + ok = True + + def expect(name, text, shell, should_fail): + nonlocal ok + got = parse_error(text, shell) + failed = got is not None and got[0] == "fail" + if failed != should_fail: + want = "reject" if should_fail else "accept" + print(f"FAIL self-test: expected to {want} {name}, got {got!r}") + ok = False + else: + print(f"ok self-test: {name}") + + # The exact shape that shipped in #121: an if with no fi. + expect( + "unterminated if", + 'if [ "${{ needs.x.result }}" = "success" ]; then\n echo hi\n', + "bash", + True, + ) + # The same block, closed. If the substitution is broken this fails too, and + # the pair together is what makes the check meaningful. + expect( + "terminated if with an expression in it", + 'if [ "${{ needs.x.result }}" = "success" ]; then\n echo hi\nfi\n', + "bash", + False, + ) + expect("unbalanced quote", 'echo "unterminated\n', "bash", True) + expect("plain block", "echo hello\n", None, False) + + # An expression spanning lines must not shift the reported line number. + shifted = substitute("a\n${{ foo\n .bar }}\nc\n") + if shifted.count("\n") != 4: + print("FAIL self-test: substitution changed the line count") + ok = False + else: + print("ok self-test: substitution preserves line count") + + print() + if not ok: + print("self-test failed") + return 1 + print("self-test passed") + return 0 + + +def main(argv): + if "--self-test" in argv: + return self_test() + + paths = [a for a in argv if not a.startswith("-")] + # The discovery floor only means anything over the whole tree; an explicit + # file list is someone checking one thing on purpose. + full_scan = not paths + if full_scan: + paths = sorted( + glob.glob(".github/workflows/*.yml") + + glob.glob(".github/workflows/*.yaml") + ) + if not paths: + print("FAIL no workflow files found; run this from the repo root.") + return 1 + return check(paths, enforce_floor=full_scan) + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..9f641ec --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,56 @@ +name: lint +on: + pull_request: + branches: + - master + # A push trigger, unlike shell-tests in OpenIPC/firmware, which deliberately + # has none. The reasoning there -- every commit on master was already checked + # on its pull request -- holds only while master has not moved between the + # PR's last run and the merge. This job is the one thing standing between a + # workflow typo and a silently lost nightly, it costs seconds, and #121 cost + # a full night's release because nothing re-checked the merged result. Cheap + # enough to not be clever about. + push: + branches: + - master + paths: + - '.github/workflows/**' + - '.github/scripts/lint-workflow-shell.py' + workflow_dispatch: + +# Reads the tree and reports; writes nothing. Declared rather than inherited so +# the job keeps the narrow token whatever the repo or org default becomes. +permissions: + contents: read + +jobs: + # Same guard the jobs in master.yml carry. A clone pushed to a new repository + # inherits this file, and on a private mirror every master sync push would + # otherwise run on that owner's bill. Upstream the first disjunct is true, so + # the condition is a tautology here. + workflow-shell: + name: workflow run blocks parse + if: >- + github.repository == 'OpenIPC/builder' || + github.event_name == 'workflow_dispatch' || + (github.event_name == 'pull_request' && !github.event.repository.private) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # ubuntu-latest ships PyYAML today, so the linter imports it fine without + # this -- which is the problem. An undeclared dependency on whatever the + # runner image happens to include is a check that stops being about this + # repo the day the image changes. apt rather than pip because 24.04 is + # PEP 668 and this is the same shape as the busybox install in + # OpenIPC/firmware's shell-tests. + - name: Install PyYAML + run: | + sudo apt-get update -qq + sudo apt-get install -y -qq python3-yaml + # Checks the checker before trusting it. The ${{ }} substitution it has to + # do is the kind of thing that breaks by making everything pass, which + # would look identical to a clean tree. + - name: Check the linter still catches what it should + run: python3 .github/scripts/lint-workflow-shell.py --self-test + - name: Parse every workflow run block + run: python3 .github/scripts/lint-workflow-shell.py diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 9566590..51d3f05 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -376,6 +376,7 @@ jobs: fi elif [ "${count}" -eq 0 ]; then echo "::notice::matrix failed and produced nothing; no release to write" + fi # Drive the release writes with gh rather than softprops/action-gh-release, # which uploads assets CONCURRENTLY with no throttle or retry knob. Firing