From cb6866b2c6c5665fb48e981620fb84294dd4accc Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 15:50:14 -0700 Subject: [PATCH 1/3] fix(scripts): one shell and one environment for every checker (improvise-f9qf) `scripts/pr-reviews-test.sh` passed 19/19 locally and 2/19 in CI on the same commit, because a multi-assignment `local` is not left-to-right on bash 5 while macOS's bash 3.2 tolerates it. The obvious half of the fix is pinning the shell. The half that decides the design is not. .envrc DOES already put a nix bash 5.3.15 on PATH, which is CI's semantics -- and it is not enough. `pkgs.mkShell` exports `name`, `out`, `outputs`, `system`, `shell`, `builder`, `stdenv`, `phases` and about twenty more generically-named variables that CI does not have, so a script reading one by accident under `set -u` is SILENT in the dev shell and fatal in CI. That is exactly what happened: the broken clause read `$name`, and in the dev shell `$name` was `nix-shell-env`. Measured at 051f541^ on one machine: macOS /bin/bash 3.2 19 passed, 0 failed direnv exec . ./scripts/pr-reviews-test.sh 19 passed, 0 failed the same, with `name` unset 2 passed, 17 failed Two reasons for green, one of them the correct interpreter. A dev shell is a third environment, not a reproduction of CI. A nix shebang was measured and declined, on availability rather than speed: warm, `nix shell nixpkgs#bash --command true` is 61.8ms against an ambient 5.1ms, and `nix-shell -p bash --run true` -- the manpage form -- is 675.2ms, 132x, inside git's merge machinery for beads-merge-driver.sh. But only the `kb` job installs nix; build, test, wasm, js, harness-lint and browser have none, so a nix shebang needs an installer step on three more jobs to reach a bash those runners already ship. zsh is not a shebang change and the runner does not have it either. Both declined in context/code/tooling.md section 5. What lands instead pins interpreter and environment together, in one place: scripts/checkers.sh is the one entry point for every check_the_checker, used verbatim by CI and by the local gate. It pins a bash >= 5 (ambient first, nix only when the ambient one is older, exit 3 when neither), scrubs the environment to an allowlist, and refuses any shell shebang in scripts/ that is not `#!/usr/bin/env bash`. The pinned bash goes on the front of PATH so inner invocations get it too -- which is where the fault actually lived. The second undetected split is closed with it: the four `#!/bin/sh` and one `#!/usr/bin/env sh` scripts are now bash, and the six places that ran a script through an explicit `sh` -- overriding the callee's own shebang, which is the same disease -- now invoke it directly. Verification, injected against the real script: restoring pr-reviews' multi-assignment `local` leaves `sh scripts/pr-reviews-test.sh` at 22/22 and `direnv exec . ./scripts/pr-reviews-test.sh` at 22/22, while `./scripts/checkers.sh pr-reviews` reports 2 passed, 20 failed, exit 1 -- matching the 2/19 CI actually reported. scripts/checkers-test.sh is twelve cases, each injecting rather than asserting; the pair that carries it runs one fixture both ways, so the `set -u` case is a test of the runner and not of `set -u`. The old local gate was `for t in scripts/*-test.sh; do sh "$t"; done`, which overrides every shebang with /bin/sh -- so it ran every suite under bash 3.2 in sh-mode regardless of what the file said. It is replaced by `./scripts/checkers.sh`. Workspace suite unchanged at 1689 passing over 38 binaries. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- .github/workflows/ci.yml | 26 ++--- context/code/tooling.md | 128 +++++++++++++++++++++ context/test-evidence/tooling.md | 50 +++++++++ scripts/assemble-stamp-test.sh | 2 +- scripts/assemble-stamp.sh | 2 +- scripts/beads-merge-driver.sh | 2 +- scripts/build-web-release.sh | 6 +- scripts/checkers-test.sh | 180 ++++++++++++++++++++++++++++++ scripts/checkers.sh | 154 +++++++++++++++++++++++++ scripts/release-preflight-test.sh | 12 +- scripts/release-preflight.sh | 2 +- web/assemble.sh | 2 +- 12 files changed, 539 insertions(+), 27 deletions(-) create mode 100755 scripts/checkers-test.sh create mode 100755 scripts/checkers.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 321ada37..2428f603 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,7 +43,7 @@ jobs: # assembly still exited 0 (improvise-w8ih). It guards the assemble step # below, so it fails under this job's name rather than another's. - name: The stamp gate refuses what it claims to - run: ./scripts/assemble-stamp-test.sh + run: ./scripts/checkers.sh assemble-stamp - uses: dtolnay/rust-toolchain@stable with: targets: wasm32-unknown-unknown @@ -245,7 +245,7 @@ jobs: # The guard above is only worth its line if its refusals are themselves # checked -- the step it replaces was green while asserting nothing. - name: The node test runner refuses what it claims to - run: ./scripts/node-test-test.sh + run: ./scripts/checkers.sh node-test kb: name: Knowledge base consistency @@ -265,18 +265,18 @@ jobs: # writer could regress green -- the review that found the directive # bypass also found that nothing ran this suite. - name: The kb tool refuses what it claims to - run: nix shell nixpkgs#trealla --command ./scripts/kb-test.sh - # No trealla needed: pr-watch is driven entirely from fixtures. - - name: The PR watcher refuses what it claims to - run: ./scripts/pr-watch-test.sh - # The release preflight is the one guard whose absence is invisible until - # a tag is already public and immutable, so its refusals are checked here. - - name: The release preflight refuses what it claims to - run: ./scripts/release-preflight-test.sh - # pr-reviews is what stands between a merge and an unread review, and it + run: nix shell nixpkgs#trealla --command ./scripts/checkers.sh kb + # Every suite goes through scripts/checkers.sh, which pins a bash >= 5 + # and scrubs the environment, so the shell and the variables a suite + # passes under here are the ones it passes under locally (improvise-f9qf). + - name: The checker runner refuses what it claims to + run: ./scripts/checkers.sh --self-test + # pr-watch is driven entirely from fixtures. The release preflight is the + # one guard whose absence is invisible until a tag is public and + # immutable. pr-reviews stands between a merge and an unread review, and # was blind to inline comments for as long as it existed (improvise-xgra). - - name: The PR review reader refuses what it claims to - run: ./scripts/pr-reviews-test.sh + - name: The remaining checkers refuse what they claim to + run: ./scripts/checkers.sh pr-watch release-preflight pr-reviews # --------------------------------------------------------------------------- # Lints the one crate the `test` job cannot reach. diff --git a/context/code/tooling.md b/context/code/tooling.md index 84803020..0256fb79 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -494,3 +494,131 @@ guarded, so the next reader does not mistake the silence for coverage. **Where it runs.** The `kb` job, beside `scripts/pr-watch-test.sh` — this repo keeps its check_the_checkers there, not in `test`. The step needs no trealla and no nix of its own; it is `sh`, `git archive` and `awk`. + +--- + +## 5. One shell, one environment: `scripts/checkers.sh` (improvise-f9qf) + +`scripts/pr-reviews-test.sh` passed **19/19 locally and 2/19 in CI on the same +commit**, because a multi-assignment `local` does not evaluate left to right on +bash 5 while macOS's bash 3.2 tolerates it. Two real bugs were invisible to the +machine that wrote them. This section is what that cost to close, and the half +of it that is not obvious is worth more than the half that is. + +### The obvious half: two platform splits, neither detected + +At `22ab58f` there were sixteen shell scripts in `scripts/`: eleven +`#!/usr/bin/env bash`, four `#!/bin/sh`, one `#!/usr/bin/env sh`. That is *two* +splits, not one. `bash` is 3.2 on macOS and 5.2 on the CI runner; `sh` is +bash-in-sh-mode on macOS and **dash** on the runner. Neither split had anything +watching it, and the second had never been noticed at all. + +Both are now `#!/usr/bin/env bash`, and `checkers.sh` refuses any other shell +shebang in `scripts/`. Converting the `sh` scripts cost nothing — they were +already a subset — and the point is not that dash is bad, it is that **a script +whose interpreter depends on the machine is a script nothing has tested**. + +### The half that decides the design: direnv gives the right bash and the wrong environment + +The cheap fix looks like "run scripts through direnv", and `.envrc` does put a +nix bash **5.3.15** on `PATH`, which really is CI's semantics. It is not enough, +and the reason generalises far past this ticket. + +`pkgs.mkShell` exports a pile of generically-named lowercase variables that CI +does not have. Measured inside this repo's dev shell: `name`, `out`, `outputs`, +`system`, `shell`, `builder`, `stdenv`, `phases`, `patches`, `buildInputs`, +`nativeBuildInputs`, `doCheck`, `strictDeps` and about twenty more. A script +under `set -u` that reads one of them by accident is **silent inside the dev +shell and fatal in CI**. + +That is exactly what happened. `fixture_file`'s broken third clause read `$name` +— and inside the dev shell `$name` was `nix-shell-env`, so `set -u` never fired, +the `[ -f "$f" ] ||` fallback re-evaluated `$name` correctly on its own line, +and the suite passed. Reproduced at `051f541^`, on one machine, in one sitting: + +| how the suite was run | result | +|---|---| +| macOS `/bin/bash` 3.2 | 19 passed, 0 failed | +| `direnv exec . ./scripts/pr-reviews-test.sh` (nix bash 5.3) | 19 passed, 0 failed | +| the same, with `name` unset | **2 passed, 17 failed** | + +The middle row is the finding. Two different reasons for green, one of them the +*correct interpreter*, and the whole point of reaching for direnv was that it +would not be green. **A dev shell is not a reproduction of CI; it is a third +environment**, and it is the one that hides `set -u` faults. + +### The decision, and the two options that were weighed with numbers + +A **nix shebang** was the ticket's first option and the user's own suggestion. +Measured warm on the orchestrator's machine, 20 invocations each: + +| | ms per invocation | +|---|---| +| ambient `bash -c true` | 5.1 | +| `nix shell nixpkgs#bash --command true` | 61.8 | +| `nix-shell -p bash --run true` (the manpage shebang form) | 675.2 | + +61.8 ms is affordable; 675 ms is 132× the ambient cost, and +`scripts/beads-merge-driver.sh` runs inside git's merge machinery. But the cost +that decides it is not startup: **only the `kb` job installs nix.** `build`, +`test`, `wasm`, `js`, `harness-lint`, `browser` and `pages-deploy` have none, so +a nix shebang on `assemble-stamp-test.sh` or `node-test.sh` would need an +installer step added to three more jobs — paid on every run, to reach a bash +those runners already have. Declined. + +**zsh** was the other option. It is not a shebang change: word splitting differs, +arrays index from 1, globbing differs. Porting sixteen scripts means reading +sixteen scripts, and the review cost is the whole diff rather than the shebang +line. The runner also does not ship zsh, so every job would need an `apt-get`. +Declined. + +What was adopted instead pins the *interpreter and the environment together*, in +one place, with no per-invocation nix cost: + +**`scripts/checkers.sh` is the one entry point for every check_the_checker.** CI +calls it and the local gate calls it, so there is no second invocation to drift. +It does three things a bare `./scripts/X-test.sh` does not: + +1. **Pins a bash ≥ 5.** The ambient one is tried first, so a runner that already + has bash 5.2 pays nothing; nix supplies one only when the ambient bash is + older, which on macOS outside the dev shell it is. No bash ≥ 5 and no nix is + **exit 3** — a refusal, never a quiet run under 3.2. +2. **Scrubs the environment to an allowlist** (`env -i` plus `PATH`, `HOME`, + `TMPDIR`, the `NIX_*` and `GITHUB_*` names, extensible through + `CHECKERS_KEEP`). This is the part that closes the direnv hole above, and it + is a *positive* list on purpose: the failure mode of a missing name is a loud + "command not found", where the failure mode of an extra name is the silence + this section is about. +3. **Enforces the shebang rule**, so the `sh`/`bash` split cannot come back. + +The pinned bash's directory goes on the front of `PATH`, so a script the suite +invokes through its *own* `#!/usr/bin/env bash` — or through `sh`, which +nixpkgs' bash also provides — gets the pinned one too. Pinning only the outer +shell would have left every inner one on the system bash, which is where the +fault actually lived. + +**A suite is `X-test.sh` where `X` or `X.sh` is present beside it.** That rule +exists because `scripts/node-test.sh` matches `*-test.sh` and is a test +*runner*, not a suite — the first version of the glob ran it with no arguments, +which printed its usage inside an otherwise green gate. The naming convention +("a check_the_checker is named after the thing it checks") was already true; it +is now load-bearing. + +### What the old local gate actually was + +The gate in circulation was `for t in scripts/*-test.sh; do sh "$t"; done`. That +loop **overrides every shebang with `/bin/sh`**, so on macOS it ran every suite +under bash 3.2 in sh-mode no matter what the file said — the worst of the +available shells, chosen by the gate rather than by any script. It is replaced +by `./scripts/checkers.sh`. + +### What is still not covered + +`web/*.sh` is outside `scripts/` and outside the shebang check; `web/build.sh` +and `web/assemble.sh` are still whatever their own first lines say. The +allowlist is a list, so a suite that comes to need a new variable fails loudly +and someone edits it — deliberate, but it is a maintenance edge. And the +scrubbing protects the *suites*; a production script run by hand inside the dev +shell still inherits `name` and friends. Closing that would mean a guard in +every script, which `scripts/beads-merge-driver.sh` — running inside git's merge +machinery, possibly outside direnv — is a good reason not to write. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 3e0c9ab2..2356506e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -288,3 +288,53 @@ helper; a fixture holding one array could not have caught this, because it would exercise a shape the live fetch never produces. No PR in this repo has yet had enough inline comments to reach a second page, so the test is the only thing that will ever see it before a user does. + +### The checker runner's own suite — `scripts/checkers-test.sh` + +The bug behind improvise-f9qf was **invisible to a local run**, so the evidence +that matters here is not "the suite is green" but "the suite goes red when the +fault is put back, in the place the fault was not seen." + +**The fault, injected against the real script.** Restoring `pr-reviews`' +multi-assignment `local` — the exact text `051f541` replaced — and running the +suite three ways on one machine in one sitting: + +| gate | result | +|---|---| +| `sh scripts/pr-reviews-test.sh` (the old local gate) | 22 passed, 0 failed | +| `direnv exec . ./scripts/pr-reviews-test.sh` | 22 passed, 0 failed | +| `./scripts/checkers.sh pr-reviews` | **2 passed, 20 failed**, exit 1 | + +The middle row is the one that earns the design. The dev shell supplies the +*correct* bash and would still have shipped this, because `pkgs.mkShell` exports +`name` and the broken clause read `$name`. Only scrubbing the environment as +well as pinning the shell makes the fault visible, and the 2/20 shape matches +the 2/19 CI actually reported. + +**`checkers-test.sh` itself is twelve cases, and each injects rather than +asserts.** The two that carry the argument: + +- *a suite reading an inherited dev-shell variable fails under the runner* — a + fixture suite whose body is `echo "$name"` under `set -euo pipefail`, run with + `name=nix-shell-env` exported, exactly as direnv leaves it. It is paired with + *and passes when run directly with that variable exported*, which asserts the + **same fixture passes the old way**. Without that pair the first case is a + test of `set -u` and proves nothing about the runner. +- *a bash-3.2-only construct fails under the runner's pinned bash* — a fixture + suite containing `local a="$1" b="$a/x"`, which is the bug's shape reduced to + two lines. Green on bash 3.2, fatal on bash 5. + +The rest separate the runner's outcomes from each other: a `#!/bin/sh` script in +the directory is refused **and the same sandbox passes once it is removed** (so +the refusal is not standing in for some other complaint); a failing suite fails +and a passing one passes; an old bash with no nix is exit 3 rather than a quiet +run; an unknown suite name is exit 4 rather than a silent no-op; and `--list` +names exactly the suites present, including the case where an `X-test.sh` has no +`X` beside it — `scripts/node-test.sh` is a test *runner*, and the first version +of the glob ran it as a suite, where it printed its usage inside a green gate. + +**What is asserted about the runner's own environment, and what is not.** The +allowlist is checked only in the sense that seven real suites pass through it; +there is no case asserting a *particular* name is absent beyond `name`. A suite +that grows a dependency on some other inherited variable will fail loudly on +first run rather than be caught here. Stated rather than guarded. diff --git a/scripts/assemble-stamp-test.sh b/scripts/assemble-stamp-test.sh index 0543443f..0ae2228b 100755 --- a/scripts/assemble-stamp-test.sh +++ b/scripts/assemble-stamp-test.sh @@ -17,7 +17,7 @@ bad() { printf ' FAIL %s (exit %s, wanted %s)\n' "$1" "$2" "$3"; fail=$((fail+1 run_case() { local name="$1" want="$2" says="$3"; shift 3 local err - err="$(sh "$RUN" "$@" 2>&1 >/dev/null)" + err="$("$RUN" "$@" 2>&1 >/dev/null)" local got=$? if [ "$got" != "$want" ]; then bad "$name" "$got" "$want" diff --git a/scripts/assemble-stamp.sh b/scripts/assemble-stamp.sh index 12dcec65..06e24841 100755 --- a/scripts/assemble-stamp.sh +++ b/scripts/assemble-stamp.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash # Every cache-bust stamp web/assemble.sh writes must be readable back. # # Usage: scripts/assemble-stamp.sh FILE PATTERN [FILE PATTERN ...] diff --git a/scripts/beads-merge-driver.sh b/scripts/beads-merge-driver.sh index d3e19d1e..e3945220 100755 --- a/scripts/beads-merge-driver.sh +++ b/scripts/beads-merge-driver.sh @@ -1,4 +1,4 @@ -#!/usr/bin/env sh +#!/usr/bin/env bash # Git merge driver for .beads/issues.jsonl. Reasoning: context/code/tooling.md. set -eu diff --git a/scripts/build-web-release.sh b/scripts/build-web-release.sh index eb4c9388..411d5d41 100755 --- a/scripts/build-web-release.sh +++ b/scripts/build-web-release.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash # Capture the assembled site as a release artifact (improvise-oujb). # # `dist` runs this from the repo root as an `[[dist.extra-artifacts]]` build @@ -81,14 +81,14 @@ if [ "${node_major:-0}" != "$NODE_VERSION" ]; then fi node --version -sh "$root/web/build.sh" +"$root/web/build.sh" # The site is assembled into a directory named for what it is, so extracting # the tarball leaves one self-describing folder rather than loose files. stage="$root/target/web-release" rm -rf "$stage" mkdir -p "$stage" -sh "$root/web/assemble.sh" "$stage/improvise-web" "$token" +"$root/web/assemble.sh" "$stage/improvise-web" "$token" rm -f "$out" tar -czf "$out" -C "$stage" improvise-web diff --git a/scripts/checkers-test.sh b/scripts/checkers-test.sh new file mode 100755 index 00000000..01995e07 --- /dev/null +++ b/scripts/checkers-test.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# check_the_checker for scripts/checkers.sh. Every case injects a fault that +# a bare `./scripts/X-test.sh` cannot see, and asserts the runner does. +# See context/test-evidence/tooling.md. +set -uo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +RUNNER="$here/checkers.sh" +work="$(mktemp -d)" +trap 'rm -rf "$work"' EXIT + +pass=0; fail=0 +ok() { printf ' ok %s\n' "$1"; pass=$((pass+1)); } +bad() { printf ' FAIL %s (%s)\n' "$1" "$2"; fail=$((fail+1)); } + +# A sandbox is a copy of the runner beside fixture suites, so `$HERE` inside it +# is the sandbox: the shebang scan and the suite list are both the fixtures'. +sandbox() { + local d="$work/$1" + mkdir -p "$d" + cp "$RUNNER" "$d/checkers.sh" + chmod +x "$d/checkers.sh" + printf '%s' "$d" +} + +# A suite is only discovered when the thing it checks is present, so every +# fixture gets a stub subject beside it. +suite() { + local d="$1" name="$2" + cat > "$d/$name-test.sh" + chmod +x "$d/$name-test.sh" + printf '#!/usr/bin/env bash\nexit 0\n' > "$d/$name.sh" + chmod +x "$d/$name.sh" +} + +# $1 name, $2 expected exit, rest: the command. Nothing follows the runner in +# its own pipeline, which is the rule its own header states. +expect() { + local what="$1" want="$2"; shift 2 + "$@" > "$work/out" 2>&1 + local got=$? + [ "$got" = "$want" ] && ok "$what" || bad "$what" "exit $got, wanted $want" +} + +# --------------------------------------------------------------------------- +# 1. The fault this runner exists for. `nix develop` exports `name`; a suite +# that reads an unset variable under `set -u` is therefore SILENT in the dev +# shell and fatal in CI. That is improvise-f9qf, reproduced exactly. +d="$(sandbox setu)" +suite "$d" masked <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +echo "read: $name" +EOF +export name=nix-shell-env +expect "a suite reading an inherited dev-shell variable fails under the runner" \ + 1 "$d/checkers.sh" masked + +# The same suite run the way the old gate ran it PASSES, which is what makes +# the case above discriminating rather than a test of `set -u`. +"$d/masked-test.sh" > "$work/out" 2>&1 +direct=$? +[ "$direct" = 0 ] \ + && ok "and passes when run directly with that variable exported" \ + || bad "and passes when run directly with that variable exported" \ + "exit $direct, wanted 0" +unset name + +# --------------------------------------------------------------------------- +# 2. The bash-5 `local` semantics. A multi-assignment `local` does not read +# left to right on bash 5, so this suite passes on macOS bash 3.2 and dies +# under `set -u` on bash 5 -- PR #133's 19/19 local against 2/19 in CI. +d="$(sandbox bash5)" +suite "$d" vers <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +f() { local a="$1" b="$a/x"; echo "$b"; } +f one +EOF +expect "a bash-3.2-only construct fails under the runner's pinned bash" \ + 1 "$d/checkers.sh" vers + +# --------------------------------------------------------------------------- +# 3. The shebang rule. `#!/bin/sh` is dash in CI and bash-in-sh-mode on macOS. +d="$(sandbox shebang)" +suite "$d" fine <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +printf '#!/bin/sh\nexit 0\n' > "$d/legacy.sh" +chmod +x "$d/legacy.sh" +expect "a #!/bin/sh script in the scripts directory is refused" 1 \ + "$d/checkers.sh" fine +grep -q 'wanted #!/usr/bin/env bash' "$work/out" \ + && ok "and the refusal names the shebang it wanted" \ + || bad "and the refusal names the shebang it wanted" "no such message" + +# Without the injected file the same sandbox passes, so the case above is not +# passing on some other complaint. +rm -f "$d/legacy.sh" +expect "and the same sandbox passes once it is removed" 0 "$d/checkers.sh" fine + +# --------------------------------------------------------------------------- +# 4. A failing suite is reported, and a passing one is not. +d="$(sandbox rc)" +suite "$d" red <<'EOF' +#!/usr/bin/env bash +exit 1 +EOF +expect "a failing suite fails the runner" 1 "$d/checkers.sh" red + +d="$(sandbox green)" +suite "$d" blue <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +expect "a passing suite passes the runner" 0 "$d/checkers.sh" blue + +# --------------------------------------------------------------------------- +# 5. No bash >= 5 and no nix is exit 3, not a silent run under whatever is +# there. This is the arm that would have refused the macOS system bash. +d="$(sandbox oldbash)" +suite "$d" any <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +mkdir -p "$work/fakebin" +cat > "$work/fakebin/bash" <<'EOF' +#!/bin/sh +case "$*" in *BASH_VERSINFO*) echo 3; exit 0 ;; esac +exit 0 +EOF +chmod +x "$work/fakebin/bash" +# The runner is invoked through a REAL bash, so the fake one below is only +# what its own `command -v bash` probe finds -- not its interpreter. +# /usr/bin:/bin carries the utilities the runner itself needs and carries no +# nix, so the fake bash is the only candidate and the nix fallback is absent. +env PATH="$work/fakebin:/usr/bin:/bin" HOME="$HOME" \ + "${BASH:-bash}" "$d/checkers.sh" any > "$work/out" 2>&1 +got=$? +[ "$got" = 3 ] && ok "an old bash with no nix is exit 3" \ + || bad "an old bash with no nix is exit 3" "exit $got, wanted 3" + +# --------------------------------------------------------------------------- +# 6. A named suite that does not exist is a usage error, not a silent no-op. +d="$(sandbox missing)" +suite "$d" present <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +expect "an unknown suite name is exit 4" 4 "$d/checkers.sh" absent + +# --------------------------------------------------------------------------- +# 7. --list names every suite in the directory and nothing else. +d="$(sandbox listing)" +suite "$d" alpha <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +suite "$d" beta <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +"$d/checkers.sh" --list > "$work/out" 2>&1 +listed="$(tr '\n' ' ' < "$work/out")" +[ "$listed" = "alpha beta " ] && ok "--list names exactly the suites present" \ + || bad "--list names exactly the suites present" "got [$listed]" + +# A test RUNNER also ends in -test.sh. It is not a suite, and running it as +# one is how `scripts/node-test.sh` printed its usage inside a green gate. +printf '#!/usr/bin/env bash\nexit 4\n' > "$d/gamma-test.sh" +chmod +x "$d/gamma-test.sh" +"$d/checkers.sh" --list > "$work/out" 2>&1 +listed="$(tr '\n' ' ' < "$work/out")" +[ "$listed" = "alpha beta " ] \ + && ok "an X-test.sh with no X beside it is not a suite" \ + || bad "an X-test.sh with no X beside it is not a suite" "got [$listed]" + +printf '\ncheckers-test: %d passed, %d failed\n' "$pass" "$fail" +[ "$fail" -eq 0 ] diff --git a/scripts/checkers.sh b/scripts/checkers.sh new file mode 100755 index 00000000..f598cfdc --- /dev/null +++ b/scripts/checkers.sh @@ -0,0 +1,154 @@ +#!/usr/bin/env bash +# The one entry point for every check_the_checker in scripts/. CI calls THIS, +# and so does the local gate, so a suite cannot be green under one shell and +# red under the other. Reasoning: context/code/tooling.md section 3. +# +# It does three things a bare `./scripts/X-test.sh` does not: +# +# 1. pins a bash >= 5, taken from nix when the ambient one is older, so the +# macOS system bash 3.2 can never be the shell a suite passes under; +# 2. scrubs the environment down to an allowlist, because `nix develop` +# exports `name`, `out`, `system`, `shell`, `builder` and two dozen more +# generically-named variables that CI does not have -- a `set -u` fault +# reading one of them is SILENT inside the dev shell and fatal in CI; +# 3. refuses a shell script in scripts/ whose shebang is not +# `#!/usr/bin/env bash`, so the sh/bash split cannot come back. +set -euo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +usage() { + cat >&2 <<'EOF' +usage: checkers.sh [--list] [--self-test] [name ...] + + name a suite to run, spelled without the -test.sh suffix + (e.g. `pr-reviews` for scripts/pr-reviews-test.sh). + With no name, every suite in scripts/ runs. + --list print the suite names and exit + --self-test run scripts/checkers-test.sh, which injects the faults this + runner exists to catch + +exit 0 every suite passed +exit 1 a suite failed, or a shebang is not `#!/usr/bin/env bash` +exit 3 no bash >= 5 could be found and nix could not supply one +exit 4 usage error + +THIS SCRIPT'S `$?` MUST BE THE ONE YOU READ. Nothing may follow it in the same +pipeline or `;` list -- a trailing `echo "exit=$?"` takes the status. +EOF +} + +# The allowlist. A variable not named here does not reach a suite, which is the +# whole point: the dev shell's `name` masked improvise-f9qf's fault for a day. +# CHECKERS_KEEP adds names, space-separated, for a caller that needs one. +ALLOW="HOME TMPDIR TMP TEMP TERM LANG LC_ALL USER LOGNAME + CI GITHUB_ACTIONS GITHUB_TOKEN GH_TOKEN + NIX_PATH NIX_REMOTE NIX_SSL_CERT_FILE SSL_CERT_FILE + XDG_CACHE_HOME XDG_CONFIG_HOME XDG_DATA_HOME" + +# Prints the absolute path of a bash >= 5, or nothing. The ambient one is tried +# first so a machine that already has a modern bash pays no nix startup at all. +find_modern_bash() { + local candidate major + candidate="$(command -v bash || true)" + if [ -n "$candidate" ]; then + major="$("$candidate" -c 'echo "${BASH_VERSINFO[0]}"' 2>/dev/null || echo 0)" + if [ "$major" -ge 5 ] 2>/dev/null; then echo "$candidate"; return 0; fi + fi + command -v nix >/dev/null 2>&1 || return 0 + nix shell nixpkgs#bash --command bash -c 'command -v bash' 2>/dev/null || true +} + +# Every shell script in scripts/ says `#!/usr/bin/env bash` and nothing else. +# `#!/bin/sh` is dash in CI and bash 3.2 in sh-mode on macOS: a second platform +# split, undetected for the same reason the first one was. +check_shebangs() { + local bad=0 f first + for f in "$HERE"/*; do + [ -f "$f" ] || continue + first="$(head -1 "$f")" + case "$first" in + '#!'*sh|'#!'*sh\ *) + case "$first" in + '#!/usr/bin/env bash') ;; + *) printf 'checkers: %s has shebang %s, wanted #!/usr/bin/env bash\n' \ + "${f##*/}" "$first" >&2; bad=1 ;; + esac ;; + esac + done + return $bad +} + +# A check_the_checker is named after the thing it checks, so `X-test.sh` is a +# suite only when `X` or `X.sh` is here too. That excludes `node-test.sh`, +# which is a test RUNNER whose own checker is `node-test-test.sh`. +suites() { + local f n + for f in "$HERE"/*-test.sh; do + [ -f "$f" ] || continue + n="${f##*/}"; n="${n%-test.sh}" + if [ -f "$HERE/$n" ] || [ -f "$HERE/$n.sh" ]; then printf '%s\n' "$n"; fi + done +} + +# $1 the pinned bash, $2 the suite file. The suite runs with the pinned bash +# FIRST on PATH, so a script it invokes through its own `#!/usr/bin/env bash` +# -- or through `sh` -- gets the pinned one too, not the system 3.2. +run_suite() { + local pinned="$1" file="$2" + local bindir; bindir="$(dirname "$pinned")" + local -a env_args=(-i) + local v + for v in $ALLOW ${CHECKERS_KEEP:-}; do + if [ -n "${!v+set}" ]; then env_args+=("$v=${!v}"); fi + done + env_args+=("PATH=$bindir:${PATH:-/usr/bin:/bin}") + env "${env_args[@]}" "$pinned" "$file" +} + +main() { + local self_test=0 + local -a names=() + while [ $# -gt 0 ]; do + case "$1" in + --list) suites; return 0 ;; + --self-test) self_test=1 ;; + -h|--help) usage; return 4 ;; + -*) printf 'checkers: unknown option %s\n' "$1" >&2; usage; return 4 ;; + *) names+=("$1") ;; + esac + shift + done + + local pinned; pinned="$(find_modern_bash)" + if [ -z "$pinned" ]; then + echo 'checkers: no bash >= 5 on PATH and nix cannot supply one.' >&2 + echo ' Enter the dev shell (direnv allow / nix develop) and retry.' >&2 + return 3 + fi + printf 'checkers: bash %s at %s\n' \ + "$("$pinned" -c 'echo "${BASH_VERSINFO[0]}.${BASH_VERSINFO[1]}"')" "$pinned" + + local rc=0 + check_shebangs || rc=1 + + if [ "$self_test" = 1 ]; then + names=(checkers) + elif [ ${#names[@]} -eq 0 ]; then + while IFS= read -r line; do names+=("$line"); done < <(suites) + fi + + local n file + for n in ${names[@]+"${names[@]}"}; do + file="$HERE/$n-test.sh" + if [ ! -f "$file" ]; then + printf 'checkers: no such suite: %s\n' "$n" >&2 + rc=4; continue + fi + printf '\n=== %s ===\n' "$n" + run_suite "$pinned" "$file" || rc=1 + done + return $rc +} + +main "$@" diff --git a/scripts/release-preflight-test.sh b/scripts/release-preflight-test.sh index 4e842891..baa46045 100755 --- a/scripts/release-preflight-test.sh +++ b/scripts/release-preflight-test.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash # check_the_checker for scripts/release-preflight.sh. # # It exists for the reason scripts/assemble-stamp-test.sh does: the guard is the @@ -22,7 +22,7 @@ fails=0 # Refuses, AND the refusal names the tag/version mismatch rather than the tree. refuses_for_the_tag() { what=$1; tag=$2 - out=$(sh "$script" "$tag" 2>&1) && { echo "FAIL: $what -- accepted" >&2; fails=$((fails+1)); return; } + out=$("$script" "$tag" 2>&1) && { echo "FAIL: $what -- accepted" >&2; fails=$((fails+1)); return; } case "$out" in *"Cargo.toml has"*) echo " ok: $what (refused, naming the version)" ;; *"uncommitted changes"*) echo "FAIL: $what -- refused for the DIRTY TREE, so this check proved nothing" >&2; fails=$((fails+1)) ;; @@ -46,13 +46,13 @@ cp -f "$root/Cargo.toml" "$tmp/Cargo.toml" (cd "$tmp" && git init -q . && git add -A && git -c user.email=t@t -c user.name=t commit -qm t) script="$tmp/scripts/release-preflight.sh" -if sh "$script" --self-test >/dev/null 2>&1; then +if "$script" --self-test >/dev/null 2>&1; then echo " ok: --self-test (accepted)" else echo "FAIL: --self-test refused" >&2; fails=$((fails+1)) fi -if sh "$script" >/dev/null 2>&1; then +if "$script" >/dev/null 2>&1; then echo "FAIL: no argument should be a usage error" >&2; fails=$((fails+1)) else echo " ok: no argument is a usage error (refused)" @@ -63,7 +63,7 @@ refuses_for_the_tag "the shape that actually broke CI" "v0.1.0-rc99" refuses_for_the_tag "a bare version with no v" "$version" # The positive direction, which only a clean tree can exercise. -if sh "$script" "v$version" >/dev/null 2>&1; then +if "$script" "v$version" >/dev/null 2>&1; then echo " ok: the matching tag v$version (accepted)" else echo "FAIL: the matching tag v$version was refused" >&2; fails=$((fails+1)) @@ -75,7 +75,7 @@ fi # only be the tree -- which is what separates this check from the ones above # instead of duplicating them. Disabling the guard leaves all six of those green. echo "dirtied by release-preflight-test" >> "$tmp/README.md" -if out=$(sh "$script" "v$version" 2>&1); then +if out=$("$script" "v$version" 2>&1); then echo "FAIL: a dirty tree -- accepted" >&2; fails=$((fails+1)) else case "$out" in diff --git a/scripts/release-preflight.sh b/scripts/release-preflight.sh index ac5dbb60..0a0894b4 100755 --- a/scripts/release-preflight.sh +++ b/scripts/release-preflight.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash # Answer, before a tag exists, the question the Release workflow answers after. # # Usage: scripts/release-preflight.sh vX.Y.Z[-suffix] diff --git a/web/assemble.sh b/web/assemble.sh index 6935bd80..48854d7c 100755 --- a/web/assemble.sh +++ b/web/assemble.sh @@ -84,7 +84,7 @@ sedi "s|'improvise_worker_server_bg\.wasm'|'improvise_worker_server_bg.wasm?v=${ # script so its refusals can be exercised without fabricating a web/pkg # (improvise-w8ih). Its argument is FILE PATTERN, five times -- the page's own # #build-version among them (improvise-oujb). -sh "$root/scripts/assemble-stamp.sh" \ +"$root/scripts/assemble-stamp.sh" \ "$app/index.html" "id=\"build-version\">${token}<" \ "$app/app.js" "pkg/improvise_wasm_client.js?v=${token}" \ "$app/worker.js" "worker-pkg/improvise_worker_server.js?v=${token}" \ From f52c2df0c7f18021660df3b13c9f132f027ca7c6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 16:08:33 -0700 Subject: [PATCH 2/3] fix(dist): the extra-artifacts build honours its script's shebang (improvise-f9qf) `build = ["sh", "scripts/build-web-release.sh"]` overrides the shebang the same way the old local gate did, so the script ran under dash on the release runner while declaring bash. It is the seventh such site and the only one outside a shell file; scripts/checkers.sh cannot see a TOML array, so this one is fixed by reading rather than by a gate. The script is mode 100755, so dropping the `sh` runs it through its own `#!/usr/bin/env bash`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- dist-workspace.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dist-workspace.toml b/dist-workspace.toml index 8f15aa1d..48a15dcd 100644 --- a/dist-workspace.toml +++ b/dist-workspace.toml @@ -36,4 +36,4 @@ install-updater = false # web/assemble.sh, the same two scripts ci.yml and a developer run. [[dist.extra-artifacts]] artifacts = ["improvise-web.tar.gz"] -build = ["sh", "scripts/build-web-release.sh"] +build = ["scripts/build-web-release.sh"] From a14955cfb42e2cb818146768c13fc38b341c1471 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 16:09:12 -0700 Subject: [PATCH 3/3] docs(tooling): the seventh sh-override is a TOML array, which no shebang gate can see Reasoning: context/code/tooling.md section 5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/tooling.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/context/code/tooling.md b/context/code/tooling.md index 0256fb79..bd1d9c6d 100644 --- a/context/code/tooling.md +++ b/context/code/tooling.md @@ -622,3 +622,21 @@ scrubbing protects the *suites*; a production script run by hand inside the dev shell still inherits `name` and friends. Closing that would mean a guard in every script, which `scripts/beads-merge-driver.sh` — running inside git's merge machinery, possibly outside direnv — is a good reason not to write. + +### The seventh site is not a shell file + +Seven places ran a repo script through an explicit `sh`, overriding the callee's +own shebang — five in `scripts/`, one in `web/assemble.sh`, and one in +`dist-workspace.toml`: + +```toml +build = ["sh", "scripts/build-web-release.sh"] +``` + +All seven now invoke the script directly. The TOML one is the interesting one, +and it is why the shebang rule alone is not the whole answer: **`checkers.sh` +scans first lines, so it cannot see an interpreter named in a build +configuration.** A script may declare `#!/usr/bin/env bash`, pass the gate, and +still be handed to dash by whatever invokes it. The only defence there is +reading, which is how this one was found — by grepping for the *call* after +fixing the *declaration*, rather than stopping at the gate going green.