From d3fc4d615ba9043f29bd0e93f93bd108dd6edc28 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:33:57 +0100 Subject: [PATCH] fix(hooks): validate-spdx silently skipped paths containing a space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.githooks/validate-spdx.sh` iterated its file list with an unquoted `for file in $FILES_TO_CHECK`, which splits on $IFS. A path containing a space became two paths, each of which then failed `[ -f "$file" ]` and was `continue`d: no error, no increment of $CHECKED, exit 0. The validator reported a pass on a file it had never opened. That is not hypothetical. The estate contains a literal-space directory `_RSR _SET`, so every source file beneath it passed this gate unread. Replaced with a `while IFS= read -r` loop fed by `<<< "$FILES_TO_CHECK"`. `<<<` and not a `| while` pipeline, because a pipeline runs the loop in a subshell and would discard $ERRORS and $CHECKED. The tests had to change in the same commit, and that is the finding worth recording: this suite could never have caught the bug, because it encoded the bug as its contract. Its multi-file cases passed STAGED_FILES SPACE-separated (`"good.sh bad.sh"`), which only ever worked because of the word-splitting being fixed here. The real and only caller, `.githooks/pre-commit`, builds the list with `git diff --cached --name-only --diff-filter=ACM` — one path per LINE. The suite asked a different question than its consumer. Multi-file cases are now newline-delimited, matching the caller. Four new `_RSR _SET` controls are the regression proof, and they kill the mutant: against the validator as it stood on main, all four fail and the other thirteen still pass — so the conversion is a contract correction, not a weakening. One of the four asserts the DENOMINATOR via a new `ck_count`, because a skipped file and a checked file both exit 0 and only the count tells them apart. Refs #912 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ji1bq3TypfycfUPAR7hSxR --- .githooks/validate-spdx.sh | 18 ++++++++- scripts/tests/validate-spdx-test.sh | 62 ++++++++++++++++++++++++++--- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/.githooks/validate-spdx.sh b/.githooks/validate-spdx.sh index 54c33adcf..03d2e7501 100755 --- a/.githooks/validate-spdx.sh +++ b/.githooks/validate-spdx.sh @@ -51,7 +51,21 @@ fi [ -z "$FILES_TO_CHECK" ] && exit 0 -for file in $FILES_TO_CHECK; do +# ⚠ NEWLINE-DELIMITED, NOT WORD-SPLIT. This was `for file in $FILES_TO_CHECK`, +# which splits on $IFS — so a path containing a space became two paths, each +# of which then failed `[ -f "$file" ]` and was skipped by the `continue` +# below. Silently: no error, and the file never counted toward $CHECKED, so +# the denominator under-reported too. +# +# That is not hypothetical here. The estate contains a literal-space directory +# `_RSR _SET`, so EVERY source file beneath it passed this validator without +# ever being read. A validator that skips what it cannot name is worse than no +# validator, because it reports a pass. +# +# `<<<` keeps the loop in the CURRENT shell; a `... | while read` pipeline +# would run it in a subshell and discard $ERRORS and $CHECKED. +while IFS= read -r file; do + [ -n "$file" ] || continue [ -f "$file" ] || continue is_source_file "$file" || continue @@ -82,7 +96,7 @@ for file in $FILES_TO_CHECK; do echo "[validate-spdx] ERROR: $file missing SPDX header" >&2 ERRORS=$((ERRORS + 1)) fi -done +done <<< "$FILES_TO_CHECK" # Always print the denominator: "0 errors" out of 0 files examined is a # vacuous pass, and it must not read the same as a real one. diff --git a/scripts/tests/validate-spdx-test.sh b/scripts/tests/validate-spdx-test.sh index 6498c1055..f6c2e4f1e 100755 --- a/scripts/tests/validate-spdx-test.sh +++ b/scripts/tests/validate-spdx-test.sh @@ -4,15 +4,15 @@ # # Tests for .githooks/validate-spdx.sh, the pre-commit gate. # -# ⚠ TEST 1 IS THE REASON THIS EXISTS, and it FAILS against the previous version. +# ⚠ TEST 1 IS THE REASON THIS EXISTS, and it FAILS against the previous version. # # The validator had two modes asking two different questions. Full-scan mode # filtered by source extension inside `find`; staged mode did not filter at all: # # FILES_TO_CHECK=$STAGED_FILES # every staged path, whatever it is # -# Only the staged branch is reachable in practice — this validator is invoked by -# .githooks/pre-commit and by no workflow — so the mode that HAD the filter never +# Only the staged branch is reachable in practice — this validator is invoked by +# .githooks/pre-commit and by no workflow — so the mode that HAD the filter never # ran, and the mode that ran had none. # # The casualty was .github/workflows/actions.lock: machine-generated by @@ -30,7 +30,38 @@ T="$(mktemp -d)"; trap 'rm -rf "$T"' EXIT mkdir -p "$T/.github/workflows" pass=0; fail=0 -ck() { # name expected_exit staged_files + +# ⚠ STAGED_FILES IS NEWLINE-DELIMITED, and these tests must say so. +# +# The real and only caller is .githooks/pre-commit, which builds the list with +# `git diff --cached --name-only --diff-filter=ACM` — one path per LINE. This +# suite used to pass multi-file cases SPACE-separated ("good.sh bad.sh"), which +# the validator only ever handled because its loop was an unquoted +# `for file in $FILES_TO_CHECK` that split on $IFS. +# +# So the suite encoded the BUG as its contract. It asked a different question +# than the consumer, and that is precisely why it could never catch #912: a +# path containing a space became two paths, each failed `[ -f ]`, and each was +# silently skipped — not counted, not reported. Every source file under the +# estate's literal-space `_RSR _SET` directory passed without being read. +# +# Multi-file cases therefore use $'a\nb', matching the caller exactly, and the +# SPACE IN A PATH controls below are the regression proof. +# Asserts the DENOMINATOR, not just the exit code. A skipped file and a checked +# file both exit 0; only the count tells them apart, which is the whole of #912. +ck_count() { # name expected_exit expected_checked staged_files + local out rc n + out="$(cd "$T" && INPUT_STAGED_FILES="$4" bash "$HOOK" 2>&1)"; rc=$? + n="$(printf '%s' "$out" | grep -oE '[0-9]+ (of [0-9]+ )?source files' | grep -oE '^[0-9]+|of [0-9]+' | tail -1 | grep -oE '[0-9]+')" + if [ "$rc" = "$2" ] && [ "${n:-}" = "$3" ]; then + printf ' ok %s (exit %s, checked %s)\n' "$1" "$rc" "$n"; pass=$((pass+1)) + else + printf ' FAIL %s (expected exit %s checked %s, got exit %s checked %s) output=%s\n' \ + "$1" "$2" "$3" "$rc" "${n:-}" "${out:-}"; fail=$((fail+1)) + fi +} + +ck() { # name expected_exit staged_files (NEWLINE-delimited) local out rc out="$(cd "$T" && INPUT_STAGED_FILES="$3" bash "$HOOK" 2>&1)"; rc=$? if [ "$rc" = "$2" ]; then printf ' ok %s (exit %s)\n' "$1" "$rc"; pass=$((pass+1)) @@ -54,11 +85,11 @@ printf 'binary-ish payload, not source\n' > "$T/README.md" echo "validate-spdx.sh" ck "PLANTED POSITIVE: headerless actions.lock must PASS" 0 ".github/workflows/actions.lock" -ck "negative control: headerless .sh must FAIL" 1 "good.sh bad.sh" +ck "negative control: headerless .sh must FAIL" 1 $'good.sh\nbad.sh' ck "negative control: headerless .yml must FAIL" 1 ".github/workflows/bad.yml" ck "valid header must PASS" 0 "good.sh" ck "non-source README.md must PASS" 0 "README.md" -ck "lockfile alongside a valid source file must PASS" 0 ".github/workflows/actions.lock good.sh" +ck "lockfile alongside a valid source file must PASS" 0 $'.github/workflows/actions.lock\ngood.sh' # PARITY: both modes must agree about the same tree. A tree whose only headerless # files are non-source must pass a full scan exactly as it passes staged mode. @@ -85,5 +116,24 @@ ck "empty SPDX identifier must FAIL" 1 "empty.scm" ck "trailing junk after expression must FAIL" 1 "junk.sh" ck "OCaml (* *) terminator must PASS" 0 "good.ml" +# SPACE IN A PATH — the #912 regression. The estate contains a literal-space +# directory `_RSR _SET`, so this is a real path shape, not a contrived one. +# +# Under the old word-splitting loop `_RSR _SET/bad.sh` became TWO words, each +# failed `[ -f "$file" ]`, and each was silently `continue`d: no error, no +# increment of $CHECKED, exit 0. The validator reported a pass on a file it had +# never opened. Both directions are asserted, because only the pair distinguishes +# "read it and it was fine" from "never read it": +# - headerless under a space dir must FAIL (kills the mutant on rc) +# - headered under a space dir must PASS *and count 1* (kills it on the denominator) +mkdir -p "$T/_RSR _SET" +printf 'echo bad\n' > "$T/_RSR _SET/bad.sh" +printf '# SPDX-License-Identifier: MPL-2.0\necho ok\n' > "$T/_RSR _SET/good.sh" +ck "SPACE IN PATH: headerless under a space dir must FAIL" 1 "_RSR _SET/bad.sh" +ck_count "SPACE IN PATH: headered under a space dir must PASS" 0 1 "_RSR _SET/good.sh" +ck "SPACE IN PATH: mixed list, one headerless, must FAIL" 1 "$(printf '%s\n%s' '_RSR _SET/good.sh' '_RSR _SET/bad.sh')" +ck_scan "SPACE IN PATH: full-scan mode must see into a space dir" 1 "_RSR _SET" + + printf '\n%s passed, %s failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] || exit 1