Edward Langley
cb6866b2c6
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
81 lines
3.5 KiB
Bash
Executable File
81 lines
3.5 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# check_the_checker for scripts/assemble-stamp.sh. The gate it holds was
|
|
# deleted on PR #69 by injection and the assembly still exited 0 with both
|
|
# failures printed -- a guard nobody had watched fail (improvise-w8ih).
|
|
set -uo pipefail
|
|
|
|
here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
|
RUN="$here/assemble-stamp.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 (exit %s, wanted %s)\n' "$1" "$2" "$3"; fail=$((fail+1)); }
|
|
|
|
# $1 name, $2 expected exit, $3 expected stderr substring ("" for none), rest: args
|
|
run_case() {
|
|
local name="$1" want="$2" says="$3"; shift 3
|
|
local err
|
|
err="$("$RUN" "$@" 2>&1 >/dev/null)"
|
|
local got=$?
|
|
if [ "$got" != "$want" ]; then
|
|
bad "$name" "$got" "$want"
|
|
elif [ -n "$says" ] && [[ "$err" != *"$says"* ]]; then
|
|
bad "$name: stderr did not name it" "$err" "$says"
|
|
else
|
|
ok "$name"
|
|
fi
|
|
}
|
|
|
|
printf 'import "./pkg/improvise_wasm_client.js?v=abc";\n' > "$work/stamped.js"
|
|
printf 'import "./worker-pkg/improvise_worker_server.js?v=abc";\n' > "$work/stamped2.js"
|
|
printf 'import "./pkg/improvise_wasm_client.js";\n' > "$work/bare.js"
|
|
printf 'import "./worker-pkg/improvise_worker_server.js";\n' > "$work/bare2.js"
|
|
|
|
run_case "a stamped file passes" 0 "" \
|
|
"$work/stamped.js" "pkg/improvise_wasm_client.js?v=abc"
|
|
|
|
run_case "an unstamped file is refused and named" 1 "bare.js" \
|
|
"$work/bare.js" "pkg/improvise_wasm_client.js?v=abc"
|
|
|
|
# The bug the gate exists for: without the final `test -z`, the misses print
|
|
# and the script exits 0. Both must be NAMED, which is what exiting on the
|
|
# first one cost.
|
|
run_case "two misses are both named, not just the first" 1 "bare2.js" \
|
|
"$work/bare.js" "pkg/improvise_wasm_client.js?v=abc" \
|
|
"$work/bare2.js" "worker-pkg/improvise_worker_server.js?v=abc"
|
|
|
|
run_case "a miss among hits is still refused" 1 "bare2.js" \
|
|
"$work/stamped.js" "pkg/improvise_wasm_client.js?v=abc" \
|
|
"$work/bare2.js" "worker-pkg/improvise_worker_server.js?v=abc"
|
|
|
|
run_case "all four stamped is the shipping case" 0 "" \
|
|
"$work/stamped.js" "pkg/improvise_wasm_client.js?v=abc" \
|
|
"$work/stamped2.js" "worker-pkg/improvise_worker_server.js?v=abc"
|
|
|
|
# A file the assembly never copied is indistinguishable from an unstamped one
|
|
# to `grep`, which errors rather than failing quietly -- but only because the
|
|
# -f test runs first. Without it `set -e` would abort mid-list and the later
|
|
# files would go unreported, which is the failure this gate was built to end.
|
|
run_case "a file that does not exist is a miss, not an abort" 1 "vanished.js" \
|
|
"$work/vanished.js" "anything" \
|
|
"$work/bare2.js" "worker-pkg/improvise_worker_server.js?v=abc"
|
|
|
|
# A caller that drops one argument would otherwise have the last FILE read as
|
|
# a PATTERN over the file before it -- a check that passes by accident.
|
|
# The pattern is a literal. As a regex, `.` matches any character, so a stamp
|
|
# corrupted in exactly that position would read as present -- the one direction
|
|
# a guard must not fail in.
|
|
printf 'import "./pkg/improvise_wasm_clientXjs?v=abc";\n' > "$work/corrupt.js"
|
|
run_case "a pattern is matched literally, not as a regex" 1 "corrupt.js" \
|
|
"$work/corrupt.js" "pkg/improvise_wasm_client.js?v=abc"
|
|
|
|
run_case "an odd number of arguments is a usage error" 2 "pairs" \
|
|
"$work/stamped.js" "pkg/improvise_wasm_client.js?v=abc" "$work/bare.js"
|
|
|
|
run_case "no arguments at all is a usage error, not a pass" 2 "usage"
|
|
|
|
printf '\nassemble-stamp-test: %d passed, %d failed\n' "$pass" "$fail"
|
|
[ "$fail" -eq 0 ]
|