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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+13
-13
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user