From 361babd4fd61c89e182a067ff7e9d171c08cf957 Mon Sep 17 00:00:00 2001 From: Joakim Persson Date: Tue, 8 Sep 2026 23:41:44 +0200 Subject: [PATCH] ci: gate the release on shell lint, from one shared script v1.8.14's first attempt spent ~46 minutes building a base image for a tree whose own lint had been failing for 24 hours. shellcheck had already flagged the defect (SC2289, severity error) on the push that introduced it; the lint workflow went red at run 186 and nobody read it. lint.yml deliberately skips tag pushes and its reasoning is sound -- the tagged tree was already linted on main, and a tag-ref lint run sorts above the publish run, making a release look finished before anything ships. The missing invariant was never "lint the tag". It was "do not RELEASE a tree whose lint failed", and only a job inside the publish workflow can enforce that. So: extract the shell-lint logic from lint.yml into scripts/lint-shell.sh and call it from both places, then add a lint-gate job that resolve-versions depends on. resolve-versions is the graph root, so gating it gates everything. Cost is ~40 s at the front of a release; the alternative already cost fifty minutes. Extracted rather than copied on purpose. A second copy of a check is the drift this repo keeps paying for -- the same evening produced a skillset mirror that had sat 9579 B behind its upstream through two consecutive edits. The script adds one behaviour the inline version lacked: if shellcheck is not installed it exits 2 rather than silently finding nothing, inheriting the existing "a gate that cannot run must not pass" rule from hooks/pre-commit in the skillset repo. Without that, reordering the install step away would turn the gate into a green tick over zero checks. Verified locally with a stubbed shellcheck (the real binary is not in the devbox), five cases, each with its expectation stated first: absent shellcheck -> rc=2; stub pass -> rc=0 and a non-zero file count; stub fail -> rc=1; a deliberately unterminated `if` planted in scripts/ -> rc=1 via the bash -n half, naming the file; removal -> rc=0 again. Discovery cross-checks against CI's own number: the inline version reported 12 files, the extracted one reports 13, the difference being lint-shell.sh itself. YAML re-parsed (10 jobs, was 9) with an assertion that resolve-versions needs lint-gate, and the repo's check-workflow-shell.sh guard still passes. --- .gitea/workflows/docker-publish.yml | 34 +++++++++++ .gitea/workflows/lint.yml | 30 ++-------- CHANGELOG.md | 14 +++++ scripts/lint-shell.sh | 91 +++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 25 deletions(-) create mode 100644 scripts/lint-shell.sh diff --git a/.gitea/workflows/docker-publish.yml b/.gitea/workflows/docker-publish.yml index 986f729..02101ab 100644 --- a/.gitea/workflows/docker-publish.yml +++ b/.gitea/workflows/docker-publish.yml @@ -157,7 +157,41 @@ jobs: # buildcache silently reuses the layer from whatever pi version was # current when the cache was first populated. Same class of bug as # pi-devbox v0.74.0..v0.75.5 (fixed in v0.75.5b 2026-05-23). + # ── release gate ────────────────────────────────────────────── + # Refuse to spend a base build on a tree whose own shell scripts do not lint. + # + # v1.8.14's first attempt is why this exists. smoke and smoke-studio both failed + # at scripts/smoke-test.sh:770 AFTER build-base had already spent ~46 minutes, + # on a defect shellcheck had flagged as SC2289 (severity error) a day earlier: + # the lint workflow went red on the very push that introduced it (run 186) and + # stayed red for runs 187 and 188, unread. + # + # lint.yml deliberately does not run on tag pushes, and its reasoning is sound + # (the tagged tree was already linted on main; a tag-ref lint run sorts above + # the publish run and makes a release look finished before anything ships). The + # missing invariant was never "lint the tag" -- it was "do not RELEASE a tree + # whose lint failed", and only a job inside THIS workflow can enforce that. + # + # ~40 s, ahead of everything expensive, and it runs scripts/lint-shell.sh -- + # the same file lint.yml calls, not a second copy that drifts. + lint-gate: + runs-on: ubuntu-latest + container: + image: catthehacker/ubuntu:act-latest + steps: + - uses: actions/checkout@v4 + + - name: Install shellcheck + run: | + apt-get update + apt-get install -y --no-install-recommends shellcheck + + - name: "Shellcheck + syntax-check repository scripts (severity: error)" + run: bash scripts/lint-shell.sh + resolve-versions: + # Gated: a defective tree must not reach a 46-minute base build. + needs: [lint-gate] runs-on: ubuntu-latest container: image: catthehacker/ubuntu:act-latest diff --git a/.gitea/workflows/lint.yml b/.gitea/workflows/lint.yml index 6a4eb1a..6ca7f32 100644 --- a/.gitea/workflows/lint.yml +++ b/.gitea/workflows/lint.yml @@ -75,31 +75,11 @@ jobs: # are shell scripts with no extension. -print0/mapfile -d '' so a path # with a space cannot silently split, and the file count is asserted # non-zero — a green tick over an empty file set is not a check. - run: | - # Union of two signals, because either alone misses a real case: - # a shebang scan misses a sourced fragment with no shebang, and a - # *.sh glob misses the extensionless tools in rootfs/usr/local/bin/. - # Silent skipping is precisely the failure mode this gate exists to - # prevent, so err toward over-collecting. - mapfile -d '' -t all_files < <(find . -not -path './.git/*' -type f -print0) - sh_files=() - for f in "${all_files[@]}"; do - case "$f" in *.sh) sh_files+=("$f"); continue;; esac - if head -n1 "$f" 2>/dev/null | grep -qE '^#!.*\b(bash|sh)\b'; then - sh_files+=("$f") - fi - done - echo "Checking ${#sh_files[@]} shell file(s)" - if [ "${#sh_files[@]}" -eq 0 ]; then - echo "::error::no shell files found — the shebang scan or the checkout is wrong" - exit 1 - fi - shellcheck -S error -f gcc "${sh_files[@]}" - rc=0 - for f in "${sh_files[@]}"; do - bash -n "$f" || { echo "::error file=$f::bash -n failed"; rc=1; } - done - exit "$rc" + # + # The implementation moved to scripts/lint-shell.sh on 2026-09-08 so the + # release gate in docker-publish.yml runs the SAME code rather than a + # second copy that drifts. Edit the script, not a copy of it. + run: bash scripts/lint-shell.sh - name: Gitea shell guard (catches the actionlint blind spot) # actionlint models GitHub Actions, where the default run shell is diff --git a/CHANGELOG.md b/CHANGELOG.md index e11fc15..5ad23e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,20 @@ Pre-v1.0.0 tags followed the pi npm version (`v{pi_version}[letter]`). > broken assumption was different, namely that a tree whose lint FAILED would not > then be released. `docker-publish.yml` has no dependency on lint, so it built > for 50 minutes on a tree known to be defective. +> +> **Fixed, then gated.** The prose moved above the `exec_test` call so an +> apostrophe cannot terminate anything, and the shell-lint logic moved out of +> `lint.yml` into **`scripts/lint-shell.sh`** — now called by both `lint.yml` and +> a new `lint-gate` job here that `resolve-versions` depends on. A release with a +> lint error refuses in ~40 s instead of failing after fifty minutes. One copy, +> not two: a duplicated check that drifts is the failure this repo keeps paying +> for. The script also refuses to pass when `shellcheck` is absent, inheriting +> the existing principle that a gate which cannot run must not pass. +> +> **`v1.8.14` was re-pointed** from `601fc98` to the fix commit. Nothing had +> consumed the original tag — no `v1.8.14` image was ever published, only the +> content-addressed `base-a365dd24de21`. `scripts/` does not feed the base hash, +> so the re-run reuses that base and skips the 46-minute rebuild. **A test that was quietly checking nothing, and a version number that was wrong.** Both found by delegating a read-only audit of this repo to a headless worker diff --git a/scripts/lint-shell.sh b/scripts/lint-shell.sh new file mode 100644 index 0000000..d9ab69f --- /dev/null +++ b/scripts/lint-shell.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# Shellcheck + syntax-check every shell script in this repo. Severity: error. +# +# SINGLE SOURCE OF TRUTH for two callers: +# .gitea/workflows/lint.yml — advisory, every branch push and PR +# .gitea/workflows/docker-publish.yml — the release GATE (lint-gate job) +# Extracted from lint.yml on 2026-09-08 rather than copied, because a second +# copy is exactly the drift this repo has been bitten by (see skillset's +# pi-extensions mirror, refreshed the same evening after sitting 9579 B behind). +# +# WHY THIS CHECK EXISTS AT ALL +# actionlint shellchecks workflow `run:` steps only. The repo's own scripts — +# entrypoint.sh, scripts/*.sh, and the extensionless tools under +# rootfs/usr/local/bin/ — were never shellchecked. A sibling repo with the same +# gap shipped a broken `echo "$json" | python3 <<'EOF' ... json.load(sys.stdin)` +# for two months: with no script argument python reads its SCRIPT from stdin, +# so the heredoc IS stdin and json.load hits EOF. shellcheck flags that at +# severity error (SC2259); nothing ever ran it. +# +# WHY THE RELEASE GATES ON IT (added 2026-09-08, the expensive way round) +# v1.8.14's first attempt failed after build-base had already spent ~46 min: +# scripts/smoke-test.sh had an apostrophe inside a single-quoted exec_test body +# ("the fleet\'s"), which CLOSES the string, so the body truncated and its tail +# ran on the CI runner instead of inside the image. shellcheck had already +# caught it as SC2289 at severity error — the lint job went red on the very +# push that introduced it and stayed red for 24 hours, unread. lint.yml +# deliberately does not run on tag pushes (sound: the tagged tree was linted on +# main, and a tag-ref lint run sorts above the publish run and makes a release +# look finished early). The gap was never "lint the tag" — it was that a tree +# whose lint FAILED could still be released. Hence a gate inside the publish +# workflow, ~40 s, ahead of everything expensive. +# +# SEVERITY CHOICE +# -S error is 0 findings across this repo when clean, so it is free to add. +# -S warning is NOT free here (19x SC2088 tilde-in-quotes in +# recreate-sanity-check.sh, plus assorted SC2016 — both intentional), and a +# noisy gate trains people to ignore it. Error-only, matching the +# SHELLCHECK_OPTS philosophy in lint.yml. +# +# Usage: bash scripts/lint-shell.sh [root] (default root: repo top level) +set -uo pipefail + +root="${1:-}" +if [ -z "$root" ]; then + root="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +fi +cd "$root" || { echo "::error::cannot cd to $root"; exit 2; } + +# A gate that cannot run must not pass. Without this, a machine (or a CI job +# whose install step was reordered away) without shellcheck would sail through +# printing nothing, which is the failure mode this whole file exists to prevent. +if ! command -v shellcheck >/dev/null 2>&1; then + echo "::error::shellcheck not found — the gate cannot run, so it must not pass" >&2 + echo " install it (apt-get install -y shellcheck) or run this in CI" >&2 + exit 2 +fi + +# Union of two signals, because either alone misses a real case: a shebang scan +# misses a sourced fragment with no shebang, and a *.sh glob misses the +# extensionless tools in rootfs/usr/local/bin/. Silent skipping is precisely the +# failure mode this gate exists to prevent, so err toward over-collecting. +# -print0/mapfile -d '' so a path containing a space cannot silently split. +mapfile -d '' -t all_files < <(find . -not -path './.git/*' -type f -print0) +sh_files=() +for f in "${all_files[@]}"; do + case "$f" in *.sh) sh_files+=("$f"); continue;; esac + if head -n1 "$f" 2>/dev/null | grep -qE '^#!.*\b(bash|sh)\b'; then + sh_files+=("$f") + fi +done + +echo "Checking ${#sh_files[@]} shell file(s) with $(shellcheck --version | awk '/version:/{print $2}')" +# A green tick over an empty file set is not a check. +if [ "${#sh_files[@]}" -eq 0 ]; then + echo "::error::no shell files found — the shebang scan or the checkout is wrong" + exit 1 +fi + +rc=0 +shellcheck -S error -f gcc "${sh_files[@]}" || rc=1 + +# bash -n catches a different class than shellcheck (unbalanced constructs it +# declines to parse), so both run and both count. +for f in "${sh_files[@]}"; do + bash -n "$f" || { echo "::error file=$f::bash -n failed"; rc=1; } +done + +if [ "$rc" -eq 0 ]; then + echo "OK: ${#sh_files[@]} shell file(s) clean at severity error" +fi +exit "$rc"