From 49d21bcb4dd280b8cf1e785fa37648d6ff616feb Mon Sep 17 00:00:00 2001 From: Defame1297 Date: Fri, 14 Aug 2026 08:03:17 +0000 Subject: [PATCH] fix(providers): guard the statusline's unguarded array expansion `parts` is seeded empty and all seven appends are conditional, so "${parts[@]}" at the join loop can expand an empty array. install.sh deploys this file to every user machine. Two things had to both hold for the bare form to be safe: this file enabling no `set -u`, and the shell being bash 4.4+, which stopped treating an empty-array expansion as unbound. On bash 3.2 -- macOS's system bash, an explicit repo target -- adding `set -u` aborts here. That is also why the hazard is unreproducible on a modern dev box and why the enforcement is a static scan rather than a runtime test. Adds the `providers` glob to test-vale-wrap.sh's bash-3.2 scan, which excluded it precisely because of this defect. Floor is 1 rather than "count minus slack": the glob holds one file, so any slack at all means a floor of 0, which passes vacuously on a renamed directory. Also adds case 27, the regression test for the stale `shellcheck source=` directives fixed in the next commit (#97 item 2). It lives in this file because that is where the exemption it guards lives. Closes #96 Refs #97 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X7GvKuJfy2WrdBmUttV4DT --- providers/claude-code/statusline-command.sh | 11 +- tests/test-vale-wrap.sh | 126 ++++++++++++++++++-- 2 files changed, 124 insertions(+), 13 deletions(-) diff --git a/providers/claude-code/statusline-command.sh b/providers/claude-code/statusline-command.sh index b3d6bb8..6a24296 100755 --- a/providers/claude-code/statusline-command.sh +++ b/providers/claude-code/statusline-command.sh @@ -92,8 +92,17 @@ parts=() [ -n "$vim_mode" ] && parts+=("${COLOR_MAGENTA}${vim_mode}${COLOR_RESET}") # --- Join with ' · ' separator and print --- +# `parts` is seeded empty above and every append is conditional, so all seven can +# be false and the array can reach here with no elements. Two things had to both +# stay true for bare "${parts[@]}" to be safe: this file enabling no `set -u` +# (house style in every other script here is `set -euo pipefail`), and the shell +# being bash 4.4+, which stopped treating an empty-array expansion as unbound. +# On bash 3.2 -- macOS's system bash, and an explicit repo target -- adding +# `set -u` aborts here. The guarded form removes the tripwire instead of relying +# on both conditions holding. install.sh deploys this file to every user +# machine. See issue #96. output="" -for part in "${parts[@]}"; do +for part in ${parts[@]+"${parts[@]}"}; do [ -z "$output" ] && output="$part" || output="${output} · ${part}" done diff --git a/tests/test-vale-wrap.sh b/tests/test-vale-wrap.sh index a42dae3..191700b 100755 --- a/tests/test-vale-wrap.sh +++ b/tests/test-vale-wrap.sh @@ -452,21 +452,28 @@ fi # this branch introduced, whose own header (batch-run.sh:9-11) documents it as # bash-3.2-safe — along with four other scripts/*.sh. Deriving the list means a # new script is covered the moment it lands. AGENTS.md names bash 3.2 as an -# explicit repo target, so the scope is three globs, each floor-asserted below: +# explicit repo target, so the scope is four globs, each floor-asserted below: # - scripts/**/*.sh — repo tooling and pre-commit hook scripts # - tests/*.sh — the runners and every regression test # - plugins/*/.apm/**/*.sh — the scripts plugins ship to users +# - providers/**/*.sh — the scripts install.sh deploys to user machines # plugins/*/skills/** is deliberately NOT scanned: it is the generated mirror of # .apm/, so scanning both double-reports every finding, and mirror-vs-source # drift is already sync-plugin-content.sh --check's job. Scanning .apm/ is what # closed the gap where skill-audit's vale-wrap.sh was covered but agent-audit's # byte-identical copy of it was not. -# providers/**/*.sh is the one shipped script deliberately left out: -# providers/claude-code/statusline-command.sh seeds `parts=()` empty at :83 and -# expands it unguarded at :96. That is a real latent hazard rather than a false -# positive — it just cannot abort today because the file enables no `set -u`. -# Fixing it is a change to a file this case does not own; once :96 uses the -# guarded form, add a `providers` glob to the table below. +# providers/**/*.sh was excluded until issue #96: statusline-command.sh seeded +# `parts=()` empty and expanded it unguarded, a real latent hazard rather than a +# false positive, in a file this case did not own. That expansion is now guarded, +# so the glob is in the table below and the deployed provider scripts get the +# same coverage as scripts/, tests/ and plugins/*/.apm/. This is the glob most +# worth having: install.sh copies providers/ content onto every user machine, and +# it is also the glob most likely to look redundant to a future reader, because +# the hazard it catches is invisible on any modern bash — bash 4.4 stopped +# treating an empty-array expansion as unbound, so a bare "${a[@]}" under `set -u` +# runs clean on the maintainer's bash 5 and aborts only on macOS's bash 3.2. +# A runtime test cannot demonstrate that without a 3.2 binary; this static scan +# is the enforcement, which is why its floor must never be dropped to zero. # # Four constructs are checked, because the expansion scan cannot see any of the # other three: @@ -588,19 +595,28 @@ bash32_glob() { scripts) find "$REPO_ROOT/scripts" -name '*.sh' -type f ;; tests) find "$REPO_ROOT/tests" -maxdepth 1 -name '*.sh' -type f ;; plugins) find "$REPO_ROOT/plugins" -path '*/.apm/*' -name '*.sh' -type f ;; + providers) find "$REPO_ROOT/providers" -name '*.sh' -type f ;; *) echo "bash32_glob: unknown glob '$1'" >&2; return 1 ;; esac } # Floors are PER GLOB, not on the merged total. A single total floor cannot -# detect the failure this assertion exists to name: with 12 + 17 + 13 files, -# losing the whole `scripts` glob still leaves 30 and losing the whole `tests` -# glob still leaves 25, so any total floor low enough to survive normal churn +# detect the failure this assertion exists to name: with 12 + 18 + 13 + 1 files, +# losing the whole `scripts` glob still leaves 32 and losing the whole `tests` +# glob still leaves 26, so any total floor low enough to survive normal churn # is too low to notice an entire glob silently resolving to nothing. Each floor # sits a little under its current count so ordinary file removal does not trip # it, but a broken or renamed path does. Parallel arrays rather than an # associative one — `declare -A` is bash 4.0+, which this very case forbids. -BASH32_GLOB_NAMES=(scripts tests plugins) -BASH32_GLOB_FLOORS=(10 14 10) +# +# `providers` is floored at 1 rather than "current count minus slack" because it +# holds exactly one file: any slack at all would be a floor of 0, which passes on +# a renamed or deleted directory and is precisely the silent-zero failure the +# per-glob floors exist to catch. A floor of 1 means removing the last provider +# script trips this assertion — correct, because at that point the glob is dead +# weight and should be deleted from the table deliberately, not left to pass +# vacuously. +BASH32_GLOB_NAMES=(scripts tests plugins providers) +BASH32_GLOB_FLOORS=(10 14 10 1) BASH32_SCRIPTS=() BASH32_IDX=0 while [[ $BASH32_IDX -lt ${#BASH32_GLOB_NAMES[@]} ]]; do @@ -1091,6 +1107,92 @@ else pass "all 6 never-empty shell arrays are exempt and all 5 sometimes-empty ones are still flagged" fi +# Case 27 pins the `sourced_files()` exemption against the failure mode that +# actually happened (issue #97 item 2): a stale source-directive path. +# +# The exemption exists because array seeding often lives in the sourced file +# rather than the sourcing one -- install.sh's DEPLOY_* come from +# deploy-manifest.sh -- so without it those expansions read as hazards. It works +# by parsing each file's own source-directive comments. That makes it silently +# dependent on those paths resolving: a directive naming a file that is not +# there does not error, it just contributes no seed file, and the exemption +# quietly stops covering what it was written to cover. +# +# tests/run-tests.sh shipped exactly that for the length of PR #95 -- a directive +# resolving to neither the repo root nor the script's own directory. Nothing +# caught it: shellcheck's own SC1091 is `info`, and .pre-commit-config.yaml pins +# `--severity=warning`. The live consequence was nil only because both of +# run-tests.sh's expansions were independently guarded. +# +# Part C is the assertion that would have caught it, and it is the reason this +# case is not just fixture theatre: it holds every real directive in the scanned +# corpus to the resolution rule, so the next stale one fails here rather than +# lying dormant. Parts A and B pin the mechanism Part C depends on -- that +# resolution is what switches the exemption on, and non-resolution silently +# switches it off. +echo "" +echo "--- every shellcheck source directive resolves, so no seeding exemption is silently off ---" +FIXTURE27="$(mktemp -d)" +new_fixture "$FIXTURE27" +# The seeded array lives ONLY in the sourced file, never in the sourcing one -- +# that separation is the whole point of the exemption. +{ + echo "#!/usr/bin/env bash" + echo "SEEDED27=(a b c)" +} > "$FIXTURE27/seed-lib.sh" + +# Both fixture bodies are emitted through printf with the closing `]` passed as +# an argument, never written literally. This file is itself inside the `tests` +# glob, so a literal bare expansion here would be scanned as a hazard in +# test-vale-wrap.sh's own source -- the trailing-comment/inside-a-string caveat +# documented above strip_comments(). Splitting the token means the emitted +# fixture holds the real text while this file holds no bare spelling, the same +# trick the `npro[c]` rule uses. +EXPANSION27="$(printf 'echo "${SEEDED27[@%s}"' ']')" + +# Part A: a directive that resolves against the script's own directory exempts +# the expansion, so no hazard is reported. +{ + echo "#!/usr/bin/env bash" + echo "# shellcheck source=seed-lib.sh" + echo 'source "$(dirname "$0")/seed-lib.sh"' + echo "$EXPANSION27" +} > "$FIXTURE27/resolves.sh" + +# Part B: byte-identical except the directive names a file that is not there. +{ + echo "#!/usr/bin/env bash" + echo "# shellcheck source=nonexistent/seed-lib.sh" + echo 'source "$(dirname "$0")/seed-lib.sh"' + echo "$EXPANSION27" +} > "$FIXTURE27/stale.sh" + +RESOLVES27="$(unguarded_expansions "$FIXTURE27/resolves.sh")" +STALE27="$(unguarded_expansions "$FIXTURE27/stale.sh")" + +# Part C: no real directive in the derived corpus may fail to resolve. Counted +# rather than diffed, because sourced_files() reports resolution only by +# omission -- an unresolvable directive produces no output line at all. +UNRESOLVED27="" +for BASH32_SCRIPT in ${BASH32_SCRIPTS[@]+"${BASH32_SCRIPTS[@]}"}; do + DECLARED27="$(grep -cE '^[[:space:]]*#[[:space:]]*shellcheck[[:space:]]+source=[^[:space:]]+' "$BASH32_SCRIPT" || true)" + [[ "$DECLARED27" -gt 0 ]] || continue + RESOLVED27="$(sourced_files "$BASH32_SCRIPT" | grep -c . || true)" + if [[ "$RESOLVED27" -lt "$DECLARED27" ]]; then + UNRESOLVED27+="${BASH32_SCRIPT#"$REPO_ROOT"/} ($RESOLVED27/$DECLARED27) " + fi +done + +if [[ -n "$RESOLVES27" ]]; then + fail "an array seeded only in a resolvable sourced file was reported as a hazard, so the seeding exemption is not reading source directives at all" +elif [[ -z "$STALE27" ]]; then + fail "an array seeded only in an UNRESOLVABLE sourced file was still exempted, so this case cannot detect a stale directive and Part C proves nothing" +elif [[ -n "$UNRESOLVED27" ]]; then + fail "shellcheck source directive(s) resolve to nothing, silently disarming the seeding exemption for those files: $UNRESOLVED27" +else + pass "the seeding exemption switches on only for resolvable directives, and every directive in the scanned corpus resolves" +fi + echo "" echo "Results: $PASS passed, $FAIL failed" [[ $FAIL -eq 0 ]]