ci: shellcheck at the floor the code actually holds

.github/workflows/ci.yml

@@ -31,3 +31,20 @@ jobs:
       - name: self-tests under bash 3.2
         if: runner.os == 'macOS'
         run: ADW_TEST_SHELL=/bin/bash bash tests/run.sh
+
+  lint:
+    name: shellcheck
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      # Fail loudly if the runner image ever stops shipping it, rather than skipping the whole job.
+      - name: shellcheck version
+        run: shellcheck --version
+
+      # -S warning is the floor the codebase currently holds at: 0 errors, 0 warnings. Below it sit 58
+      # info and 58 style findings — mostly SC1091 (shellcheck cannot follow `source "$here/lib.sh"`)
+      # and SC2086 on expansions that are split on purpose. Those need a decision per site, so they are
+      # backlog, not a silenced rule.
+      - name: shellcheck -S warning
+        run: shellcheck -S warning -f gcc scripts/*.sh install.sh tests/run.sh tests/lib/*.sh tests/cases/*.sh

README.md

@@ -140,7 +140,9 @@ bash tests/run.sh gate-plan              # one
 ADW_TEST_SHELL=/bin/bash bash tests/run.sh   # under macOS's bash 3.2
 ```
 
-CI runs both on Linux and macOS. A case may declare a **known gap** — the behaviour we want, asserted
+CI runs both on Linux and macOS, plus `shellcheck -S warning` over every script (0 errors, 0 warnings;
+the 58 info and 58 style findings below that floor are backlog, not a silenced rule). shellcheck is not
+part of `tests/run.sh` — the suite needs only bash and git. A case may declare a **known gap** — the behaviour we want, asserted
 where the code does not deliver it yet. A known gap keeps the suite green and reports itself; the day
 it starts passing, the suite fails so the test gets promoted instead of quietly rotting. The current
 three are printed by every run, and `tests/run.sh` ends by naming the surface it does **not** cover

install.sh

@@ -33,7 +33,7 @@
 set -uo pipefail
 
 SRC="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-VERSION="2.3.0"
+VERSION="2.3.3"
 SCRIPT_FILES="lib.sh gate.sh guard.sh engines.sh intake.sh review.sh setup-worktree.sh worktree-alloc.sh"
 
 REGISTRY="${ADW_REGISTRY:-$HOME/.config/ai-tooling/installs}"
@@ -195,7 +195,7 @@ render_stack_md() {
     info "keep existing: .tasks/_STACK.md (project-owned; --regen-stack to overwrite)"; return 0
   fi
   if [[ -f "$out" && "$REGEN_STACK" == 1 ]]; then
-    local backup="$out.bak-$(date -u +%Y%m%dT%H%M%SZ)"
+    local backup; backup="$out.bak-$(date -u +%Y%m%dT%H%M%SZ)"
     [[ "$DRY_RUN" == 1 ]] || cp "$out" "$backup"
     info "backed up existing config → ${backup#$TARGET/}"
   fi
@@ -359,10 +359,10 @@ cmd_install() {
   local installed=()
   IFS=',' read -ra skill_list <<< "$SKILLS"
   IFS=',' read -ra tool_list <<< "$TOOLS"
-  for id in "${skill_list[@]}"; do
+  for id in ${skill_list[@]+"${skill_list[@]}"}; do
     id="$(echo "$id" | tr -d ' ')"
     [[ -d "$SRC/skills/core/$id" ]] || die "unknown skill: $id"
-    for tool in "${tool_list[@]}"; do
+    for tool in ${tool_list[@]+"${tool_list[@]}"}; do
       case "$tool" in
         claude) dest_root="$TARGET/.claude/skills/$id" ;;
         codex)  dest_root="$TARGET/.codex/skills/$id" ;;
@@ -398,7 +398,7 @@ cmd_install() {
   (( WITH_HOOKS )) && install_hooks
 
   if [[ "$DRY_RUN" != 1 ]]; then
-    write_manifest "${installed[@]}"
+    write_manifest ${installed[@]+"${installed[@]}"}
     register_install "$TARGET"
   fi
 
@@ -412,7 +412,7 @@ cmd_install() {
   if (( ${#ignored[@]} )); then
     info ""
     info "IGNORED BY GIT — these paths were written but git will not see them:"
-    for p in "${ignored[@]}"; do info "    $p   ($(cd "$TARGET" && git check-ignore -v "$p" 2>/dev/null | cut -d: -f1-2))"; done
+    for p in ${ignored[@]+"${ignored[@]}"}; do info "    $p   ($(cd "$TARGET" && git check-ignore -v "$p" 2>/dev/null | cut -d: -f1-2))"; done
     info "  Decide deliberately: commit the harness with 'git add -f <path>', or leave it untracked and"
     info "  know that a fresh clone of this repo has no harness. Half-tracked is the bad outcome."
   fi
@@ -496,10 +496,10 @@ PY
   local plan="" id tool a f rel
   IFS=',' read -ra _skills <<< "$SKILLS"
   IFS=',' read -ra _tools  <<< "$TOOLS"
-  for id in "${_skills[@]}"; do
+  for id in ${_skills[@]+"${_skills[@]}"}; do
     id="$(printf '%s' "$id" | tr -d ' ')"
     [[ -d "$SRC/skills/core/$id" ]] || { info "  GONE  skill '$id' no longer exists in the source — left in place"; continue; }
-    for tool in "${_tools[@]}"; do
+    for tool in ${_tools[@]+"${_tools[@]}"}; do
       case "$tool" in claude) rel=".claude/skills/$id" ;; codex) rel=".codex/skills/$id" ;; *) continue ;; esac
       while IFS= read -r f; do plan="$plan$SRC/skills/core/$id/$f	$rel/$f
 "; done < <(cd "$SRC/skills/core/$id" && find . -type f | sed 's|^\./||')
@@ -560,7 +560,7 @@ $SRC/templates/GROOM_LOG.md	.tasks/_templates/GROOM_LOG.md
   (( WITH_HOOKS )) && install_hooks
 
   IFS=',' read -ra _installed <<< "$SKILLS"
-  write_manifest "${_installed[@]}"
+  write_manifest ${_installed[@]+"${_installed[@]}"}
   register_install "$TARGET"
   info "  manifest updated → .ai-tooling.json (version $VERSION)"
   # Files left behind means the upgrade is not fully applied; --force leaves nothing behind.

scripts/engines.sh

@@ -106,9 +106,9 @@ probe() {
     done
     # keep only the pinned entries when at least two answered: they are strictly more informative
     local pinned=(); local e
-    for e in "${usable[@]}"; do [[ "$e" == *:* ]] && pinned+=("$e"); done
+    for e in ${usable[@]+"${usable[@]}"}; do [[ "$e" == *:* ]] && pinned+=("$e"); done
     if (( ${#pinned[@]} >= 2 )); then
-      usable=("${pinned[@]}")
+      usable=(${pinned[@]+"${pinned[@]}"})
       adw_log "single vendor → pinned models probed and recorded (CROSS-MODEL, not DEGRADED)"
     fi
   fi
@@ -117,7 +117,7 @@ probe() {
     adw_warn "no usable engine. Reviews cannot run until at least one CLI is authenticated."
     return 1
   fi
-  local list; list="$(IFS=,; printf '%s' "${usable[*]}")"
+  local list; list="$(IFS=,; printf '%s' "${usable[*]-}")"
   printf '%s\n' "$list"
 
   if (( write )); then

scripts/gate.sh

@@ -25,7 +25,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 cmd="${1:-}"; shift || true
 
@@ -83,7 +83,7 @@ gate_test() { adw_run_cmd test "$(test_command "${1:-}")" TEST_CMD; }
 # BASE branch catches the second: a test that is already green without the feature asserts nothing, and
 # the implementer will happily write code "to satisfy" it.
 gate_red_against_base() {
-  local target="${1:-}" base cmd tmp rc out changed
+  local target="${1:-}" base cmd tmp tmp_parent rc out changed
   base="$(adw_cfg BASE_BRANCH main)"
   git rev-parse --verify "$base" >/dev/null 2>&1 || base="origin/$base"
   if ! git rev-parse --verify "$base" >/dev/null 2>&1; then
@@ -99,7 +99,12 @@ gate_red_against_base() {
   fi
 
   cmd="$(test_command "$target")"
-  tmp="$(mktemp -d)/base"
+  # `git worktree remove` takes the worktree away but not the mktemp -d that holds it. Removing the
+  # parent on every exit path — including the ones that return early below — is why this is a trap
+  # rather than an rm before each return.
+  tmp_parent="$(mktemp -d)"
+  trap 'rm -rf "${tmp_parent:?}"' RETURN
+  tmp="$tmp_parent/base"
   adw_log "RED/base: running the new tests against '$base' — they must fail there too"
   if ! git worktree add --detach -q "$tmp" "$base" 2>/dev/null; then
     adw_warn "RED/base: could not create a scratch worktree — skipping"; return 0
@@ -328,7 +333,9 @@ gate_workspace() {
             | python3 -c 'import json,sys; print(json.load(sys.stdin).get("status",""))' 2>/dev/null)"
     if [[ -z "${have// }" ]]; then
       adw_warn "G3: could not read the ticket state for $ref — move it by hand if it is still open."
-    elif [[ -n "${want// }" && "${have,,}" != "${want,,}" ]]; then
+    # `tr`, not ${x,,}: bash 3.2 — the /bin/bash macOS ships — aborts the whole script on that
+    # expansion with "bad substitution", so the gate dies instead of returning a verdict.
+    elif [[ -n "${want// }" && "$(lower "$have")" != "$(lower "$want")" ]]; then
       adw_warn "G3: ticket $ref is '$have', not '$want'. Move it now so nobody picks up the same work:"
       adw_warn "     bash scripts/ai/intake.sh writeback $ref --status start"
       fail=1

scripts/guard.sh

@@ -19,7 +19,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 role="${1:-}"; scope="${2:---branch}"
 [[ -n "$role" ]] || adw_die "usage: guard.sh <builder|test-author|reviewer|planner|siblings> [--branch|--staged|--head]"

scripts/intake.sh

@@ -26,7 +26,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 print_contract() {
   cat <<'EOF'
@@ -290,7 +290,7 @@ writeback() {
     status)  tmpl="$(adw_cfg INTAKE_STATUS_CMD)" ;;
     comment) tmpl="$(adw_cfg INTAKE_COMMENT_CMD)" ;;
   esac
-  [[ -n "${tmpl// }" ]] || { adw_warn "no INTAKE_${kind^^}_CMD configured → skipping"; return 0; }
+  [[ -n "${tmpl// }" ]] || { adw_warn "no INTAKE_$(upper "$kind")_CMD configured → skipping"; return 0; }
 
   local resolved="${tmpl//<ref>/$ref}" cwd
   resolved="${resolved//<value>/$value}"
@@ -303,7 +303,7 @@ states() { # show how intents resolve against this tracker — diagnostic, write
   local ref="${1:-}"
   [[ -n "$(adw_cfg INTAKE_STATES_CMD)" ]] || adw_die "no INTAKE_STATES_CMD in .tasks/_STACK.md"
   local intent
-  for intent in start review done; do
+  for intent in start review 'done'; do   # quoted: bare `done` reads as the loop keyword
     printf '  %-8s → %s\n' "$intent" "$(resolve_state "$intent" "$ref")"
   done
 }

scripts/lib.sh

@@ -28,6 +28,12 @@ adw_cfg_note() {
     | grep -m1 -E "^${key}=" | grep -oE '#[[:space:]].*$' | sed -E 's/^#[[:space:]]*//'
 }
 
+# Case folding, portably. `${x,,}` and `${x^^}` need bash 4, and bash 3.2 does not merely ignore them:
+# it aborts the script with "bad substitution", so a gate using one returns no verdict at all on the
+# /bin/bash macOS ships. Everything here has to run on that shell.
+lower() { printf '%s' "$1" | tr '[:upper:]' '[:lower:]'; }
+upper() { printf '%s' "$1" | tr '[:lower:]' '[:upper:]'; }
+
 adw_log()  { printf '[adw] %s\n' "$*" >&2; }
 adw_warn() { printf '[adw] WARN: %s\n' "$*" >&2; }
 adw_die()  { printf '[adw] ERROR: %s\n' "$*" >&2; exit 2; }

scripts/review.sh

@@ -17,7 +17,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 round="${1:-1}"; shift || true
 validation=""; profile=""; lenses=""
@@ -76,16 +76,23 @@ context_common=$'\n\n'"CONTEXT: branch '$branch' vs '$base' (review round $round
 [[ -n "$validation" ]] && context_common+=$'\n'"Validation criteria: $validation — re-run the checks you can (static + tests + READ-ONLY inspection). Never run destructive or shared-state-mutating checks. A check with no reproducible evidence is NOT passed."
 
 count=$(( ${#lens_list[@]} + wildcard ))
-mapfile -t engines < <("$here/engines.sh" pick-review "$count")
-mapfile -t usable < <("$here/engines.sh" list 2>/dev/null)
+# `while read`, not mapfile: mapfile is bash 4+. On bash 3.2 it is simply not a command, and the next
+# line reads ${engines[0]} under `set -u` and aborts — a review that dies before spawning a reviewer.
+engines=(); while IFS= read -r _line; do engines+=("$_line"); done < <("$here/engines.sh" pick-review "$count")
+usable=();  while IFS= read -r _line; do usable+=("$_line");  done < <("$here/engines.sh" list 2>/dev/null)
+
+# engines.sh writes its own diagnosis to stderr and exits, which leaves this array empty rather than
+# unset. Saying so here beats letting an array subscript deliver the news: the operator otherwise gets
+# the real reason followed by a bash "unbound variable", and on bash 3.2 the abort comes first.
+(( ${#engines[@]} )) || adw_die "no review engine available (see the error above). Fix with: bash scripts/ai/engines.sh probe --write"
 
 # run_lens <engine> <outfile> <prompt> — retries once on a different usable engine when output is empty
 # (an unauthenticated or rate-limited CLI fails silently; a missing lens must not look like a clean lens).
 run_lens() {
   local engine="$1" out="$2" prompt="$3" e
   printf '%s' "$prompt" | "$here/engines.sh" run "$engine" > "$out" 2>/dev/null
   if [[ ! -s "$out" ]]; then
-    for e in "${usable[@]}"; do
+    for e in ${usable[@]+"${usable[@]}"}; do
       [[ "$e" == "$engine" ]] && continue
       adw_warn "engine '$engine' returned nothing for $(basename "$out" .md) → retrying on '$e'"
       printf '%s' "$prompt" | "$here/engines.sh" run "$e" > "$out" 2>/dev/null
@@ -97,7 +104,7 @@ run_lens() {
 adw_log "round $round · profile $profile · ${#lens_list[@]} lens reviewer(s) + wildcard=$wildcard + judge=$judge · diversity=$diversity"
 
 pids=(); idx=0
-for lens in "${lens_list[@]}"; do
+for lens in ${lens_list[@]+"${lens_list[@]}"}; do
   engine="${engines[$idx]}"; out="$outdir/lens-$lens.md"
   prompt="$(printf '%s\n%s\n%s\n' "$base_rubric" "$(lens_block "$lens")" "$context_common")"
   {
@@ -110,7 +117,7 @@ done
 if (( wildcard )); then
   engine="${engines[$idx]}"; out="$outdir/lens-wildcard.md"
   prompt="$(printf '%s\n%s\n%s\n%s\n' "$base_rubric" "$(lens_block wildcard)" \
-    "The other reviewers on this diff cover exactly these lenses: ${lens_list[*]}. Anything inside them is THEIR job — do not duplicate it." \
+    "The other reviewers on this diff cover exactly these lenses: ${lens_list[*]-}. Anything inside them is THEIR job — do not duplicate it." \
     "$context_common")"
   {
     used="$(run_lens "$engine" "$out" "$prompt")"
@@ -119,13 +126,13 @@ if (( wildcard )); then
   pids+=($!)
 fi
 
-for p in "${pids[@]}"; do wait "$p" || true; done
+for p in ${pids[@]+"${pids[@]}"}; do wait "$p" || true; done
 
 findings_files=("$outdir"/lens-*.md)
 combined="$outdir/findings.md"
 {
   printf '# Review round %s — profile %s — diversity %s\n\n' "$round" "$profile" "$diversity"
-  for f in "${findings_files[@]}"; do
+  for f in ${findings_files[@]+"${findings_files[@]}"}; do
     [[ -s "$f" ]] || { adw_warn "empty output: $f"; continue; }
     printf '\n## %s\n\n' "$(basename "$f" .md)"; cat "$f"
   done
@@ -154,7 +161,7 @@ if [[ ! -s "$final" ]]; then adw_die "no reviewer output (all engines failed)";
 # Cost report — one row per engine call, plus a total. Reported even when partial.
 {
   printf '# Review round %s — cost\n\nprofile: %s · lenses: %s · wildcard: %s · judge: %s · diversity: %s\n' \
-    "$round" "$profile" "${lens_list[*]}" "$wildcard" "$judge" "$diversity"
+    "$round" "$profile" "${lens_list[*]-}" "$wildcard" "$judge" "$diversity"
   printf 'wall clock: %ss\n\n' "$(( $(date -u +%s) - start_ts ))"
   if [[ -s "$ADW_USAGE_FILE" ]]; then
     printf '| call | input | output | cache |\n| --- | --- | --- | --- |\n'

scripts/setup-worktree.sh

@@ -15,7 +15,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 common_git="$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null)" \
   || adw_die "cannot resolve git common dir"
@@ -56,7 +56,7 @@ if (( ${#excluded[@]} )); then
   excl="$common_git/worktrees/$(basename "$root")/info/exclude"
   [[ -d "$(dirname "$excl")" ]] || excl="$common_git/info/exclude"
   mkdir -p "$(dirname "$excl")"
-  for entry in "${excluded[@]}"; do
+  for entry in ${excluded[@]+"${excluded[@]}"}; do
     grep -qxF "$entry" "$excl" 2>/dev/null || printf '%s\n' "$entry" >> "$excl"
   done
   adw_log "updated git exclude: $excl"

scripts/worktree-alloc.sh

@@ -16,7 +16,7 @@ here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 source "$here/lib.sh"
 
 root="$(adw_repo_root)" || adw_die "not a git repo"
-cd "$root"
+cd "$root" || adw_die "cannot enter the repository root: $root"
 
 # Index = position in `git worktree list` (stable while the worktree exists).
 index=0; n=0

tests/cases/gate-red.sh

@@ -49,4 +49,26 @@ check_out 'and it names the offending test file'  'tests/probe.sh' gate "$taut"
 # The same run with the base check disabled passes — which is why the skip is reported.
 check 'the same tautology slips through --no-base' 0 gate "$taut" red --no-base
 
+# The scratch worktree lives inside a mktemp -d. `git worktree remove` takes the worktree away and
+# leaves the directory that held it, so every RED gate used to leave one behind for the life of the
+# machine.
+#
+# TMPDIR is not usable to make this countable: BSD mktemp (macOS) ignores it and uses the per-user
+# _CS_DARWIN_USER_TEMP_DIR, so a TMPDIR-based fixture would watch an empty directory and pass no matter
+# what the code did — the same "skipped looks like passed" failure this suite exists to catch. So ask
+# mktemp itself where it writes, and diff the tmp.* entries there across the run.
+#
+# Race, accepted: another process may create a tmp.* directory during the run. That direction produces a
+# false FAILURE, never a false pass.
+leaves_no_tmpdir() {
+  local probe parent before after leaked
+  probe="$(mktemp -d)"; parent="$(dirname "$probe")"; rmdir "$probe"
+  before="$(find "$parent" -maxdepth 1 -type d -name 'tmp.*' 2>/dev/null | LC_ALL=C sort)"
+  ( cd "$taut" && bash scripts/ai/gate.sh red >/dev/null 2>&1 )
+  after="$(find "$parent" -maxdepth 1 -type d -name 'tmp.*' 2>/dev/null | LC_ALL=C sort)"
+  leaked="$(comm -13 <(printf '%s\n' "$before") <(printf '%s\n' "$after"))"
+  [[ -z "${leaked// }" ]] || { printf 'left behind in %s:\n%s\n' "$parent" "$leaked"; return 1; }
+}
+check 'the RED/base check cleans up its temp directory' 0 leaves_no_tmpdir
+
 done_tests

tests/cases/install-upgrade.sh

@@ -6,7 +6,7 @@ set -uo pipefail
 source "$(cd "$(dirname "${BASH_SOURCE[0]}")/../lib" && pwd)/harness.sh"
 
 # Never touch the operator's real registry.
-export ADW_REGISTRY="$(tmpdir registry)/installs"
+ADW_REGISTRY="$(tmpdir registry)/installs"; export ADW_REGISTRY
 
 target="$(fixture target)" || exit 1
 rm -rf "$target/scripts"                      # this test installs the scripts itself