diff --git a/.githooks/validate-spdx.sh b/.githooks/validate-spdx.sh index 54c33adc..03d2e750 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 6498c105..f6c2e4f1 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