Repository navigation
ci: shellcheck at the floor the code actually holds - #8
Merged
Merged
Conversation
Three core scripts used bash 4+ syntax. macOS ships bash 3.2.57 as /bin/bash, and
`#!/usr/bin/env bash` finds a newer one only when Homebrew's is on PATH — not on a stock machine, and
not on a macOS CI runner. Every script parses fine under 3.2 (`bash -n` is clean), so this only
appeared at runtime, mid-run, on the operator's laptop:
- gate.sh `${have,,}` / `${want,,}` — bad substitution is FATAL in a non-interactive shell, so
`gate.sh workspace` with INTAKE_WRITEBACK=true aborted instead of returning a verdict. Not a failing
gate: no gate at all.
- intake.sh `${kind^^}` — same, in the writeback path.
- review.sh `mapfile` — not a command on 3.2, so the array stayed unset and the next line read
`${engines[$idx]}` under `set -u` and aborted. A review that dies before spawning a reviewer.
lib.sh gains `lower`/`upper` (tr-based) since case folding is now needed in two scripts, and review.sh
reads the engine lists with `while IFS= read -r`.
Closes the last known gap in the suite: `check_xfail 'core scripts avoid bash-4-only constructs'` is
promoted to `check`, and a new assertion exercises `lower`, `upper` and the array read under
/bin/bash itself rather than trusting the replacements.
The detector needed fixing too. It shared a comment-and-quote stripper with the GNU-tool rule, but
`"${have,,}"` sits inside double quotes nearly every time it appears, so stripping quoted strings
blinded it to exactly what it looks for. Split into two: comments only for bash-4 expansions, comments
and quoted strings for tool names (a binary named in a warning message is not a use of it).
Verified by mutation, one construct at a time: restoring `${have,,}` in gate.sh, `${kind^^}` in
intake.sh, `mapfile` in review.sh, or breaking `lower()` each fails exactly one assertion. The first
attempt at this reported zero failures for two of them — the perl one-liners had silently not applied.
The mutation runner now fails loudly when its own edit does not match, which is the only reason the
quote-stripping blind spot above was found rather than shipped.
Suite: 145 passed, 0 failed, 0 known gaps, under bash 5.2 and 3.2.
VERSION 2.3.0 → 2.3.1.
Refs RUS-188
Two defects on the same platform, found while verifying the claim made in PR #6. RUS-194 — bash 3.2 aborts on "${arr[@]}" and "${arr[*]}" when the array is EMPTY and set -u is on: `unbound variable`, exit 127, script over. Bash 4.4 made the empty expansion legal, so this is invisible on Linux CI and on any machine with Homebrew bash, and the lines themselves look perfectly portable — the grep added in the previous commit does not catch them. Where it could fire: review.sh's retry loop over `usable` (fed by `engines.sh list` with stderr suppressed, so an unconfigured probe arrives as silence — and it runs exactly when a lens already returned nothing), the `engines`/`lens_list`/`pids`/`findings_files` loops, engines.sh's `usable` and `pinned`, and setup-worktree.sh's `excluded`. Every [@] expansion now carries ${arr[@]+"${arr[@]}"} and every [*] carries `-`. The rule is blanket on purpose: whether a given array can really be empty is a judgement no static check can make, and the guard costs nothing. review.sh also checks the engine list explicitly. engines.sh writes its diagnosis to stderr and exits, which leaves the array empty rather than unset — so the operator used to get the real reason followed by a bash "unbound variable", and on 3.2 the abort came first. Correction to PR #6, which described this as "pick-review returns nothing": it dies. The array ends up empty either way. RUS-189 — gate_red_against_base created its scratch worktree inside a `mktemp -d` and removed only the worktree, leaving the parent behind on every RED gate for the life of the machine. Now a RETURN trap, because the function returns from three places and the early ones are exactly where an rm-before-each- return would be forgotten. The leak test was wrong twice before it was right, and both mistakes are the disease this repo exists to cure: 1. It pointed TMPDIR at a countable directory. BSD mktemp (macOS) IGNORES TMPDIR — it uses _CS_DARWIN_USER_TEMP_DIR — so the fixture watched an empty directory and passed no matter what the code did. Verified by restoring the leak and watching the test stay green. 2. Fixed by asking mktemp itself where it writes and diffing the tmp.* entries there. Race accepted and written down: a concurrent tmp.* directory produces a false FAILURE, never a false pass. Tests: a grep rule for unguarded expansions (it has to strip the guarded form first, since that form CONTAINS the bare one), a runtime assertion that the guarded shapes survive an empty array under /bin/bash, and the leak check above. Each verified by mutation: restoring an unguarded [@], an unguarded [*], or the temp leak fails exactly one assertion. Suite: 148 passed, 0 failed, 0 known gaps, under bash 5.2 and 3.2. VERSION 2.3.1 → 2.3.2. Refs RUS-194, RUS-189
Deferred twice for the same reason: shellcheck was not installed on the authoring machine, so its
output on ~1700 lines of bash was unknown, and the two ways to add it blind are both bad — fail CI on
day one, or wire it with `|| true`, which is a check that cannot fail.
Measured first, then wired. The census over scripts/, install.sh and tests/:
error 0
warning 9
info 58
style 58
All nine warnings were real and cheap, so the floor is `-S warning` with nothing silenced:
- SC2164 ×6 — `cd "$root"` unguarded in every script that enters the repo root. The line above already
dies when `git rev-parse` fails, but a root that exists and cannot be entered ran every gate against
whatever directory the caller happened to be in. Now `|| adw_die`.
- SC1010 — `for intent in start review done` reads the third word as the loop keyword. Bash parses it
correctly; quoting it says so on purpose.
- SC2155 ×2 — `local x="$(cmd)"` masks the command's exit status.
info and style are backlog, not exclusions, and the workflow says which they are: mostly SC1091
(shellcheck cannot follow `source "$here/lib.sh"`) and SC2086 on expansions that are word-split
deliberately. Each needs a decision per site; a blanket `-e` would hide the ones that matter.
The lint is a separate CI job on ubuntu, and prints `shellcheck --version` first so a runner image that
stops shipping it fails loudly instead of skipping. It is deliberately NOT part of tests/run.sh: that
suite runs anywhere with bash and git, and adding a tool it has to degrade around would reintroduce
exactly the "skipped looks like passed" shape the suite exists to catch. run.sh's coverage footer says
so.
Verified: `shellcheck -S warning` exits 0 on the tree, restoring one unguarded `cd` makes it exit 1,
and the new guard reports the path and exits 2 rather than continuing in the wrong directory. Suite
still 148 passed, 0 failed, under bash 5.2 and 3.2.
VERSION 2.3.2 → 2.3.3.
Refs RUS-193
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #7. Closes RUS-193.
Deferred twice for the same reason: shellcheck was not installed here, so its output on ~1700 lines of
bash was unknown, and the two ways to add it blind are both bad — fail CI on day one, or wire it with
|| true, which is a check that cannot fail.Measured first
All nine warnings were real and cheap, so the floor is
-S warningwith nothing silenced:cd "$root"unguarded in every script that enters the repo root. The line above already dies whengit rev-parsefails — but a root that exists and cannot be entered ran every gate against whatever directory the caller happened to be in. Now|| adw_die.for intent in start review donereads the third word as the loop keyword. Bash parses it correctly; quoting says so on purpose.local x="$(cmd)"masks the command's exit status.What is below the floor
infoandstyleare backlog, not exclusions, and the workflow comment says which they are: mostlySC1091 (shellcheck cannot follow
source "$here/lib.sh") and SC2086 on expansions that are word-splitdeliberately. Each needs a decision per site — a blanket
-e SC2086would hide the ones that matter.Where it runs
A separate
lintjob on ubuntu, printingshellcheck --versionfirst so a runner image that stopsshipping it fails loudly instead of skipping.
Deliberately not part of
tests/run.sh. That suite runs anywhere with bash and git; adding a tool itwould have to degrade around reintroduces exactly the "skipped looks like passed" shape the suite exists
to catch.
run.sh's coverage footer now says where linting lives.How to verify
Restoring one unguarded
cdingate.shmakes shellcheck exit 1. And the guard is not decorative:Not verified
tests/run.sh. It passes locally against shellcheck 0.11.0; a runner on an older version could reportdifferently at the same severity.
judged which of them are real.