From cb6866b2c6c5665fb48e981620fb84294dd4accc Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 15:50:14 -0700 Subject: [PATCH] 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}" \