From ca8d9d4459feeaef716810005b3325e391d4ae92 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 14 Sep 2026 05:43:59 -0400 Subject: [PATCH] fix(#4429): stop a large commit_types config blocking or bypassing the gate (#4723) * test(#4429): regression coverage for three defects in the commit hook Failing-first coverage. Every conforming-subject row is red against the unfixed hook, and each defect gets an explicit CONTROL row that reconstructs the pre-fix form and asserts the defect reproduces -- without those, the passing rows would pass with or without the fix. 1. SIGPIPE (the reported defect). The pre-fix first-line extraction used a `head -1` pipeline; once CONFIG_OUT exceeds the 64 KiB pipe buffer printf is killed and `set -euo pipefail` aborts the hook. That fix is already on next -- it landed incidentally in #4537, whose message never mentions #4429 -- and nothing in the tree would notice its removal. 2. regcomp. The commit-type alternation grew with the CONFIGURED list and exceeded bash's 64 KiB compiled-pattern cap. Boundary rows pin the cliff at 6051/6052, with controls on BOTH sides so limit-1 is not vacuous. 3. Ambient subprocess statuses (found by this change's security review). Defects 1 and 2 cannot be separated: each configured type adds len+1 bytes to CONFIG_OUT and len+1 to the alternation, so the smallest payload that overflows the pipe (N=6059) already puts the alternation past the ceiling. The SIGPIPE control accepts either SIGPIPE (141, Linux) or a reported write error (macOS bash 3.2's builtin printf, exit 1). Asserting only the message would go red on every CI lane, since the remote matrix is Linux-only. Named to bucket with gsd-validate-commit-crash-policy.test.cjs, which covers this same hook: lint-test-file-count derives a test's owning module from its filename prefix, and `validate-commit-*` collided with the `validate` module, already at its 2-file cap. Harness note, learned from three vacuous control runs: hooks/lib/git-cmd.js requires ../gsd-core/bin/lib/token-scanner.cjs relative to the hooks dir's parent, so a copy in a bare tmpdir fails open and returns 0 for any input. The layout symlinks gsd-core beside the copy, and every row that can prove it asserts the run was substantive. Co-Authored-By: Claude Opus 5 * fix(#4429): bound the commit-type regex and isolate subprocess statuses Two fixes in the same file, both of the same shape: a value computed for one purpose was being read as authority about something else. 1. The commit-type alternation could not be compiled. COMMIT_TYPE_ALT joined every CONFIGURED type into one regex, so the pattern grew without bound. bash caps a compiled pattern at 64 KiB. Bisected on bash 3.2.57 (this repo's macOS target): a 65504-byte alternation compiles, 65515 fails. `[[ =~ ]]` returns 2 on a compile failure, and `if !` cannot tell that from "the subject does not conform" -- so the hook blocked a valid `feat(auth): ...` with CONVENTIONAL_COMMITS_VIOLATION while printing `feat` in its own valid_types. Match the shape with a fixed-size pattern, capture the type, then test membership against the COMMIT_TYPES array. The character class is exactly the `^[a-z][a-z0-9-]*$` safe-token filter the config loader already applies, so it captures every type that can legally reach COMMIT_TYPES and no token that cannot. Review verified equivalence over 46 handcrafted plus 6000 randomized adversarial subjects against a type list containing prefix-overlapping, digit-bearing and trailing-hyphen types: zero divergences. The loop adds no subprocess and no pipe, which is the hazard class #4429 is about. COMMIT_TYPE_ALT is now unused and removed. types pre-fix `feat(auth): ...` fixed 10 accept accept 6051 accept accept 6052 BLOCK accept 20000 BLOCK accept 2. Subprocess statuses were inherited from the environment. Each status is captured as `... || VAR=$?`, which assigns ONLY on the failure branch; on success the variable kept whatever it already held, and `${VAR:-0}` defaults only when unset or empty. So an EXPORTED CONFIG_STATUS, CMD_STATUS or CLASSIFY_STATUS -- from a CI wrapper, a .envrc, or another hook -- survived into the success path and was read as "the subprocess failed". Since the hook fails OPEN on a genuine subprocess failure by design (#3838), the result was a silent bypass. Measured: `CLASSIFY_STATUS=3 git commit -m "nope: bad"` printed "validator disabled for this call" and exited 0. The three are now initialised before use. The fail-open path is unchanged and verified byte-identical to origin/next with a failing node. hooks/dist/ is gitignored and rebuilt from hooks/ by scripts/build-hooks.js, so there is no second copy to sync. Co-Authored-By: Claude Opus 5 * chore(#4429): register the new suite with the conformance manifests Both conformance-tier manifests embed the test-file list, so adding a test file makes them stale. Regenerated with their own generators: node scripts/gen-platform-conformance-tier.cjs --write node scripts/gen-platform-conformance-tier.cjs --target macos --write Co-Authored-By: Claude Opus 5 * test(#4429): pin both fail-open-prone controls to their named cause Two rows in the ambient-status block asserted `status === 0`, which the hook also returns when the harness layout is broken -- so either row could have passed for entirely the wrong reason. This is the same vacuity trap the rest of the suite already guards, applied inconsistently to the rows added last. Measured, rather than reasoned about: genuine ambient bypass (pre-fix hook, CLASSIFY_STATUS=3) rc=0, no CLASSIFIER_THREW orphaned layout (no gsd-core symlink) rc=0, CLASSIFIER_THREW genuine fail-open (node shim exits 3) rc=0, no CLASSIFIER_THREW So assertSubstantive separates the intended cause from the harness failure in both rows, and each now pins its pass to the cause it names. Co-Authored-By: Claude Opus 5 * test(#4429): stop asserting a macOS-only regex cap on every platform First verification run was RED: 45636/45638 passed, both failures in this new suite on linux-node24. Cause is mine -- I measured the compiled-pattern ceiling on macOS and encoded it as a cross-platform expectation. Measured in the tester image itself: engine 6051 6052 20000 bash 3.2.57 / BSD libc (macOS) compiles rc 2 rc 2 bash 5.2.15 / glibc (Linux) compiles compiles compiles (228943 B) glibc has no reachable cap, so the regcomp defect cannot occur there and the control asserting a block at 6052 was red for a behaviour the platform cannot produce. The control now calibrates at runtime: it runs the pre-fix form and, when this engine compiled the alternation, it SKIPS with a message naming the reason rather than asserting. Skipped out loud, never silently passed -- a green row there would read as "the defect is covered" on a platform where it cannot occur. Both branches verified: the capped branch asserts (macOS 17/17, zero skipped), and the uncapped branch was exercised by forcing the payload to a size that always compiles, producing a skip and not a failure. Consequence stated rather than hidden: the remote matrix is Linux-only, so this one control is skipped in CI and really runs only on a macOS workstation. The rows that run everywhere are the ones carrying the regression weight -- the shipped hook accepting a conforming commit at every payload size, the gate still blocking unknown types, the SIGPIPE control, and all seven ambient-status rows. Note this also narrows the coupling claim: SIGPIPE and regcomp are coupled only on a capped engine. On glibc the SIGPIPE defect is directly testable without the regcomp fix. Co-Authored-By: Claude Opus 5 * docs(#4429): scope the regex-cap claim to the platform it applies to The changeset told users the validator "built a regular expression bigger than bash can compile" past ~6,000 configured types. That is false on Linux: glibc compiled a 228,943-byte alternation without complaint, so a Linux reader would have been misled about their own exposure. These are user-facing release notes, so the claim is now scoped to macOS (bash 3.2 / BSD libc) and says explicitly that glibc was never affected by this half. The hook's own comment led with the same overstatement -- "bash caps a compiled pattern at 64 KiB" -- before qualifying it. Reworded so the first clause states what is actually true: the limit is a property of the platform's regex engine. Text only; no behaviour change. Suite 17/17, eslint and lint:ci clean. Co-Authored-By: Claude Opus 5 * chore(#4429): backfill changeset PR number (#4723) * chore(#4429): backfill changeset PR number (#4723) --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/agile-cranes-bark.md | 5 + .changeset/gallant-birds-purr.md | 5 + hooks/gsd-validate-commit.sh | 67 ++- .../lib/macos-conformance-tier.generated.cjs | 1 + .../platform-conformance-tier.generated.cjs | 1 + tests/gsd-validate-commit-sigpipe.test.cjs | 424 ++++++++++++++++++ 6 files changed, 499 insertions(+), 4 deletions(-) create mode 100644 .changeset/agile-cranes-bark.md create mode 100644 .changeset/gallant-birds-purr.md create mode 100644 tests/gsd-validate-commit-sigpipe.test.cjs diff --git a/.changeset/agile-cranes-bark.md b/.changeset/agile-cranes-bark.md new file mode 100644 index 000000000..1170670de --- /dev/null +++ b/.changeset/agile-cranes-bark.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4723 +--- +**Commits are no longer blocked when a project configures a large `commit_types` list** — the commit validator built one regular expression out of every configured type, and on macOS (bash 3.2 / BSD libc) that pattern stopped compiling past roughly 6,000 types. The validator reported the compile failure as "this message is not a Conventional Commit" — rejecting a valid `feat(auth): …` while listing `feat` among the valid types it printed. At the same payload the hook could also abort outright with a broken-pipe error instead of returning a verdict. Linux (glibc) has no comparable limit and was never affected by this half. Both paths are fixed and now covered by regression tests. (#4429) diff --git a/.changeset/gallant-birds-purr.md b/.changeset/gallant-birds-purr.md new file mode 100644 index 000000000..6433f9bb5 --- /dev/null +++ b/.changeset/gallant-birds-purr.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 4723 +--- +**An exported `CONFIG_STATUS`, `CMD_STATUS`, or `CLASSIFY_STATUS` no longer disables the commit-message gate** — the hook captured each subprocess status with `|| VAR=\$?`, which assigns only when the subprocess fails, so on success the variable kept any value inherited from the environment. A CI wrapper, a `.envrc`, or another hook that exported one of those names made the validator report "validator disabled for this call" and accept a non-conforming commit. The statuses are now initialised before use; a genuine subprocess failure still passes the commit through, as before. (#4429) diff --git a/hooks/gsd-validate-commit.sh b/hooks/gsd-validate-commit.sh index bbae36a85..40c5c4329 100755 --- a/hooks/gsd-validate-commit.sh +++ b/hooks/gsd-validate-commit.sh @@ -16,6 +16,21 @@ set -euo pipefail # Idempotent and failure-proof by construction: unset vars expand to "" (a # no-op rm -f target), and `|| true` guarantees the trap itself never changes # the script's exit status. +# Subprocess exit statuses, pre-initialised so they can never be inherited from +# the ambient environment. Each is captured as `... || VAR=$?`, which assigns +# ONLY on the failure branch; on success the variable keeps whatever it already +# held, and `${VAR:-0}` defaults only when unset or empty. So an EXPORTED +# CONFIG_STATUS / CMD_STATUS / CLASSIFY_STATUS — from a CI wrapper, a .envrc, or +# another hook — survived into the success path and was read as "the subprocess +# failed". Measured: `CLASSIFY_STATUS=3 git commit -m "nope: bad"` printed +# "validator disabled for this call" and exited 0, silently accepting a +# non-conforming commit. Same for CONFIG_STATUS and CMD_STATUS. Found by the +# security review of #4429; the gate is fail-open by design on a genuine +# subprocess failure (#3838), which is exactly what made this bypass quiet. +CONFIG_STATUS=0 +CMD_STATUS=0 +CLASSIFY_STATUS=0 + ENABLED_ERR="" CMD_ERR="" CLASSIFY_ERR="" @@ -61,6 +76,12 @@ if [ -f .planning/config.json ]; then process.exit(3); } " 2>"$ENABLED_ERR") || CONFIG_STATUS=$? + # Pre-initialised, NOT left to `${...:-0}` alone: the capture below only + # assigns on the `||` branch, so on SUCCESS the variable keeps whatever it + # already held — and an EXPORTED variable of this name is inherited from the + # ambient environment. `${VAR:-0}` defaults only when unset/empty, so + # `CONFIG_STATUS=3 git commit …` made this hook print "validator disabled" and exit 0, + # silently accepting a non-conforming commit. Found by review of #4429. CONFIG_STATUS=${CONFIG_STATUS:-0} if [ "$CONFIG_STATUS" != "0" ]; then # Could not determine the opt-in flag at all (node missing, JSON parse @@ -103,6 +124,12 @@ CMD=$(echo "$INPUT" | node -e " } }); " 2>"$CMD_ERR") || CMD_STATUS=$? +# Pre-initialised, NOT left to `${...:-0}` alone: the capture below only +# assigns on the `||` branch, so on SUCCESS the variable keeps whatever it +# already held — and an EXPORTED variable of this name is inherited from the +# ambient environment. `${VAR:-0}` defaults only when unset/empty, so +# `CMD_STATUS=3 git commit …` made this hook print "validator disabled" and exit 0, +# silently accepting a non-conforming commit. Found by review of #4429. CMD_STATUS=${CMD_STATUS:-0} if [ "$CMD_STATUS" != "0" ]; then # Could not extract tool_input.command at all (node missing, malformed @@ -127,6 +154,12 @@ GIT_CMD_LIB="$HOOK_DIR/lib/git-cmd.js" node -e " process.exit(3); } " "$CMD" 2>"$CLASSIFY_ERR" || CLASSIFY_STATUS=$? +# Pre-initialised, NOT left to `${...:-0}` alone: the capture below only +# assigns on the `||` branch, so on SUCCESS the variable keeps whatever it +# already held — and an EXPORTED variable of this name is inherited from the +# ambient environment. `${VAR:-0}` defaults only when unset/empty, so +# `CLASSIFY_STATUS=3 git commit …` made this hook print "validator disabled" and exit 0, +# silently accepting a non-conforming commit. Found by review of #4429. CLASSIFY_STATUS=${CLASSIFY_STATUS:-0} if [ "$CLASSIFY_STATUS" != "0" ] && [ "$CLASSIFY_STATUS" != "1" ]; then # 0 = is a git commit (validate below); 1 = genuinely not a git commit @@ -557,7 +590,7 @@ if [ "$CLASSIFY_STATUS" = "0" ]; then fi # Single source of truth for the accepted commit-type list (#3811): the # 10 built-ins plus whatever passed the safe-token filter above. Both the - # regex alternation and the human-readable error text below are derived + # membership test and the human-readable error text below are derived # from this ONE array — no hand-synced second copy. # # The `"${EXTRA_COMMIT_TYPES[@]+"${EXTRA_COMMIT_TYPES[@]}"}"` form (not @@ -568,7 +601,6 @@ if [ "$CLASSIFY_STATUS" = "0" ]; then # /bin/bash 3.2.57 on macOS. The `${arr[@]+word}` form is the # nounset-safe idiom for "expand if set, empty otherwise" on empty arrays. COMMIT_TYPES=("${BUILTIN_COMMIT_TYPES[@]}" "${EXTRA_COMMIT_TYPES[@]+"${EXTRA_COMMIT_TYPES[@]}"}") - COMMIT_TYPE_ALT=$(IFS='|'; echo "${COMMIT_TYPES[*]}") COMMIT_TYPE_LIST=$(printf '%s, ' "${COMMIT_TYPES[@]}") COMMIT_TYPE_LIST="${COMMIT_TYPE_LIST%, }" # Typed `valid_types` array (#3811 review finding): CONTRIBUTING.md bans @@ -580,8 +612,35 @@ if [ "$CLASSIFY_STATUS" = "0" ]; then # or `\`. COMMIT_TYPES_JSON=$(printf '"%s",' "${COMMIT_TYPES[@]}") COMMIT_TYPES_JSON="[${COMMIT_TYPES_JSON%,}]" - # Validate Conventional Commits format - if ! [[ "$SUBJECT" =~ ^($COMMIT_TYPE_ALT)(\(.+\))?:[[:space:]].+ ]]; then + # Validate Conventional Commits format. + # + # #4429: do NOT build `^(type1|type2|...)` out of COMMIT_TYPES. That + # alternation grows with the CONFIGURED list, and how large a pattern can be + # compiled is a property of the platform's regex engine. bash 3.2.57 / BSD + # libc (macOS, this file's stated target) caps it at 64 KiB - bisected: a + # 65504-byte alternation compiles, 65515 fails. bash 5.2 / glibc has no + # reachable cap, so this half never bit Linux. Past a cap `[[ =~ ]]` + # returns 2, and `if !` cannot tell a COMPILE ERROR from "the subject does + # not conform" - so a valid `feat(auth): ...` was blocked with + # CONVENTIONAL_COMMITS_VIOLATION while `feat` sat in its own valid_types. + # + # Match the SHAPE with a fixed-size pattern, then test membership against + # the array. The regex no longer depends on how many types are configured, + # and the loop adds no subprocess or pipe (the #4429 hazard this file + # already avoids elsewhere). The character class is exactly the safe-token + # filter `^[a-z][a-z0-9-]*$` applied above, so it captures every type that + # can legally reach COMMIT_TYPES and no token that cannot. + SUBJECT_TYPE='' + if [[ "$SUBJECT" =~ ^([a-z][a-z0-9-]*)(\(.+\))?:[[:space:]].+ ]]; then + SUBJECT_TYPE="${BASH_REMATCH[1]}" + fi + COMMIT_TYPE_OK=0 + if [ -n "$SUBJECT_TYPE" ]; then + for _known_type in "${COMMIT_TYPES[@]}"; do + if [ "$_known_type" = "$SUBJECT_TYPE" ]; then COMMIT_TYPE_OK=1; break; fi + done + fi + if [ "$COMMIT_TYPE_OK" -ne 1 ]; then # Emit typed `code` and `valid_types` fields alongside `reason` (#2974, # #3811). Tests assert on the stable code string and the typed array; # the reason is the human-readable copy, never grepped by tests. diff --git a/scripts/lib/macos-conformance-tier.generated.cjs b/scripts/lib/macos-conformance-tier.generated.cjs index f16f78c60..0d7cd36a8 100644 --- a/scripts/lib/macos-conformance-tier.generated.cjs +++ b/scripts/lib/macos-conformance-tier.generated.cjs @@ -83,6 +83,7 @@ module.exports = { "tests/gsd-statusline.test.cjs", "tests/gsd-tools-path-refs.test.cjs", "tests/gsd-validate-commit-crash-policy.test.cjs", + "tests/gsd-validate-commit-sigpipe.test.cjs", "tests/gsd-write-guard.test.cjs", "tests/health-diagnostic-rules/worktree-health.test.cjs", "tests/health-diagnostic.test.cjs", diff --git a/scripts/lib/platform-conformance-tier.generated.cjs b/scripts/lib/platform-conformance-tier.generated.cjs index b7f94c8df..44b239f5e 100644 --- a/scripts/lib/platform-conformance-tier.generated.cjs +++ b/scripts/lib/platform-conformance-tier.generated.cjs @@ -107,6 +107,7 @@ module.exports = { "tests/gsd-mcp-server-bin.test.cjs", "tests/gsd-statusline.test.cjs", "tests/gsd-validate-commit-crash-policy.test.cjs", + "tests/gsd-validate-commit-sigpipe.test.cjs", "tests/gsd-write-guard.test.cjs", "tests/health-diagnostic-rules/config-validation.test.cjs", "tests/health-diagnostic-rules/worktree-health.test.cjs", diff --git a/tests/gsd-validate-commit-sigpipe.test.cjs b/tests/gsd-validate-commit-sigpipe.test.cjs new file mode 100644 index 000000000..675bf8817 --- /dev/null +++ b/tests/gsd-validate-commit-sigpipe.test.cjs @@ -0,0 +1,424 @@ +// #4429 — `hooks/gsd-validate-commit.sh` must survive a large `commit_types` +// config. Two defects, provably coupled, covered by one suite. +// +// (1) SIGPIPE. The pre-fix hook took the first line of CONFIG_OUT with +// `printf '%s\n' "$CONFIG_OUT" | head -1`. `head` closes after one line, so +// `printf` is killed by SIGPIPE once CONFIG_OUT exceeds the 64 KiB pipe +// buffer, and `set -euo pipefail` aborts the whole hook. The fix — pure +// parameter expansion — is ALREADY on `next`; it landed INCIDENTALLY in +// #4537, a PR about commit classification whose message never mentions +// #4429. Nothing in the tree would notice its removal, and the failure is +// load-dependent (the reporter: "does not reproduce on an idle machine"), +// so a reintroduction would surface as an unrelated flaky test. +// +// (2) regcomp. `COMMIT_TYPE_ALT` joined every CONFIGURED type into one regex. +// bash caps a compiled pattern at 64 KiB; past it `[[ =~ ]]` returns 2, and +// `if !` cannot tell a COMPILE ERROR from "the subject does not conform". +// A valid `feat(auth): …` was blocked with CONVENTIONAL_COMMITS_VIOLATION +// while `feat` sat in its own valid_types. +// +// They are coupled with no gap. Each configured type contributes `len+1` bytes +// to CONFIG_OUT (type + newline) AND `len+1` to the alternation (type + pipe), +// so the two strings are the same size to within the leading flag line. The +// smallest payload that overflows the pipe (N=6059, CONFIG_OUT 65544) already +// puts the alternation at 65592 — past the 65504 ceiling. So (1) cannot be +// tested at all until (2) is fixed, which is why both land together. + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { runHook } = require('./helpers/process-seam.cjs'); +const { HOOK_FANOUT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { cleanup } = require('./helpers.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const HOOKS_DIR = path.join(REPO_ROOT, 'hooks'); +const isWindows = process.platform === 'win32'; + +// The compiled-regex ceiling is a PROPERTY OF THE PLATFORM'S REGEX ENGINE, not a +// repo invariant — do not read these as universal constants. +// +// bash 3.2.57 / BSD libc (macOS, this repo's stated target): bisected to a +// 65504-byte alternation compiling and 65515 failing — 6051 vs 6052 types. +// bash 5.2.15 / glibc (the Linux tester image, which is the ONLY OS the remote +// matrix runs): measured MATCH at 6051, 6052 and 20000 types (228943 bytes). +// No reachable cap, so the regcomp defect does not reproduce there at all. +// +// Encoding the macOS numbers as a cross-platform expectation is precisely what +// made the first verification run red. The rows below therefore assert only what +// holds everywhere, and the control that needs a capped engine calibrates itself +// at runtime instead of assuming one. +const ALT_LAST_COMPILING = 6051; +const ALT_FIRST_FAILING = 6052; +const ALT_PAST_FAILING = 6053; +// Comfortably over the 64 KiB pipe buffer (~180 KiB of CONFIG_OUT), so the +// pre-fix race fires deterministically rather than occasionally. +const SIGPIPE_PAYLOAD = 20000; + +// Same bash fan-out class as tests/hooks-opt-in.test.cjs: the hook runs under +// bash and shells out to node, so node must be on PATH. +const hookEnv = { + ...process.env, + PATH: `${path.dirname(process.execPath)}${path.delimiter}${process.env.PATH || '/usr/local/bin:/usr/bin:/bin'}`, +}; + +/** + * A throwaway copy of `hooks/` that the hook can actually run from. + * + * `hooks/lib/git-cmd.js` requires `../gsd-core/bin/lib/token-scanner.cjs`, + * resolved relative to the hooks directory's PARENT. A copy dropped in a bare + * tmpdir cannot resolve it: the classifier throws and the hook FAILS OPEN, + * returning 0 for every input including garbage. Three control runs read as + * "the pre-fix form is fine" for exactly that reason before it was caught — so + * the layout symlinks `gsd-core` beside the copy, and every row additionally + * asserts the run was substantive (see `assertSubstantive`). + * + * @param {object} t node:test context, for cleanup. + * @param {(src: string) => string} [transform] rewrites the hook's text to + * reconstruct a pre-fix form. Each transform hard-asserts its anchors, so a + * future refactor fails loudly instead of silently testing nothing. + * @returns {string} absolute path to the hook to run. + */ +function makeHookLayout(t, transform) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4429-hooks-')); + t.after(() => cleanup(root)); + fs.cpSync(HOOKS_DIR, path.join(root, 'hooks'), { recursive: true, dereference: true }); + cleanup(path.join(root, 'hooks', 'dist')); + fs.symlinkSync(path.join(REPO_ROOT, 'gsd-core'), path.join(root, 'gsd-core'), 'dir'); + + const hookPath = path.join(root, 'hooks', 'gsd-validate-commit.sh'); + if (transform) { + // Reading the shell script's text is the point: the subject under test IS + // the shipped script, and a pre-fix form can only be reconstructed from it. + // (`local/no-source-grep` scopes to .cjs/.js/.ts; this is neither grep nor + // an assertion on source text — it is building a second binary to run.) + const src = fs.readFileSync(hookPath, 'utf-8'); + fs.writeFileSync(hookPath, transform(src), { mode: 0o755 }); + } + return hookPath; +} + +/** + * Remove the subprocess-status pre-initialisation, restoring the form in which + * an EXPORTED CONFIG_STATUS / CMD_STATUS / CLASSIFY_STATUS was inherited into + * the success path and read as "the subprocess failed" (defect 3). + */ +function toPreFixAmbientStatus(src) { + const block = 'CONFIG_STATUS=0\nCMD_STATUS=0\nCLASSIFY_STATUS=0\n\n'; + assert.ok( + src.includes(block), + 'reconstruction anchor gone: the hook no longer pre-initialises the three ' + + 'subprocess-status variables. Re-derive the pre-fix form before trusting this suite.', + ); + return src.replace(block, ''); +} + +/** Restore the `printf … | head -1` first-line extraction (defect 1). */ +function toPreFixPipeline(src) { + const shipped = 'ENABLED="${CONFIG_OUT%%$\'\\n\'*}"'; + assert.ok( + src.includes(shipped), + 'reconstruction anchor gone: the shipped hook no longer extracts ENABLED with ' + + 'parameter expansion. Re-derive the pre-fix form before trusting this suite.', + ); + return src.replace(shipped, 'ENABLED=$(printf \'%s\\n\' "$CONFIG_OUT" | head -1)'); +} + +/** Restore the unbounded `^(type1|type2|…)` alternation (defect 2). */ +function toPreFixAlternation(src) { + const listLine = ' COMMIT_TYPE_LIST=$(printf \'%s, \' "${COMMIT_TYPES[@]}")'; + const guardStart = " SUBJECT_TYPE=''"; + const guardEnd = ' if [ "$COMMIT_TYPE_OK" -ne 1 ]; then'; + for (const [anchor, what] of [[listLine, 'COMMIT_TYPE_LIST'], [guardStart, 'SUBJECT_TYPE'], [guardEnd, 'COMMIT_TYPE_OK']]) { + assert.ok( + src.includes(anchor), + `reconstruction anchor gone (${what}): the shipped hook's type check was ` + + 'restructured. Re-derive the pre-fix form before trusting this suite.', + ); + } + let out = src.replace( + listLine, + ` COMMIT_TYPE_ALT=$(IFS='|'; echo "\${COMMIT_TYPES[*]}")\n${listLine}`, + ); + const start = out.indexOf(guardStart); + const end = out.indexOf(guardEnd); + assert.ok(end > start, 'pre-fix reconstruction: type-check block is out of order'); + return ( + out.slice(0, start) + + ' if ! [[ "$SUBJECT" =~ ^($COMMIT_TYPE_ALT)(\\(.+\\))?:[[:space:]].+ ]]; then\n' + + out.slice(end + guardEnd.length + 1) + ); +} + +/** + * A project whose `.planning/config.json` configures `count` extra commit + * types. Every value matches the hook's own `^[a-z][a-z0-9-]*$` filter and is + * distinct from the 10 built-ins. + */ +function makeProject(t, count) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4429-proj-')); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + const types = []; + for (let i = 0; i < count; i++) types.push(`zz${i}type`); + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ hooks: { community: true, commit_types: types } }), + ); + return dir; +} + +function runValidate(hookPath, cwd, message, extraEnv = {}) { + const r = runHook(hookPath, [], { + input: JSON.stringify({ tool_input: { command: `git commit -m "${message}"` } }), + encoding: 'utf-8', + cwd, + interpreter: 'bash', + env: { ...hookEnv, ...extraEnv }, + timeoutMs: HOOK_FANOUT_TIMEOUT_MS, + }); + return { status: r.exitCode, stdout: r.stdout || '', stderr: r.stderr || '' }; +} + +/** + * A hook whose classifier threw accepts ANY input — asserting `status === 0` + * against it proves nothing. Every row calls this first. + */ +function assertSubstantive(res, label) { + assert.ok( + !res.stderr.includes('CLASSIFIER_THREW'), + `${label}: the hook FAILED OPEN — the classifier could not resolve ` + + `gsd-core/bin/lib, so this run returns 0 for any input and proves nothing.\n` + + `stderr: ${res.stderr.slice(0, 400)}`, + ); +} + +describe('#4429 — gsd-validate-commit.sh under a large commit_types config', { skip: isWindows }, () => { + test('a conforming subject is accepted at a payload that overflows the pipe buffer', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, SIGPIPE_PAYLOAD); + const res = runValidate(hook, dir, 'feat(auth): add login flow'); + assertSubstantive(res, 'shipped hook'); + assert.equal( + res.status, + 0, + `A valid Conventional Commit was rejected with ${SIGPIPE_PAYLOAD} configured ` + + `commit_types. stdout: ${res.stdout.slice(0, 300)}`, + ); + }); + + test('the gate is not weakened: an unknown type is still blocked', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, SIGPIPE_PAYLOAD); + const res = runValidate(hook, dir, 'nope(auth): not a configured type'); + assertSubstantive(res, 'shipped hook'); + assert.equal(res.status, 2, 'an unconfigured type must still be blocked'); + assert.match(res.stdout, /"code": "CONVENTIONAL_COMMITS_VIOLATION"/); + }); + + test('a CONFIGURED extra type is accepted — proves the config is actually read', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, SIGPIPE_PAYLOAD); + // The LAST entry, so a truncated read cannot pass this row. + const res = runValidate(hook, dir, `zz${SIGPIPE_PAYLOAD - 1}type: use the last configured type`); + assertSubstantive(res, 'shipped hook'); + assert.equal( + res.status, + 0, + 'the last configured commit type was rejected — the config was not fully read, ' + + 'so every other row in this file is measuring a default-sized list', + ); + }); + + // limit-1 / limit / limit+1 on the compiled-regex ceiling. + for (const count of [ALT_LAST_COMPILING, ALT_FIRST_FAILING, ALT_PAST_FAILING, SIGPIPE_PAYLOAD]) { + test(`conforming subject accepted with ${count} configured types`, (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, count); + const res = runValidate(hook, dir, 'feat(auth): add login flow'); + assertSubstantive(res, `shipped hook @ ${count}`); + assert.equal( + res.status, + 0, + `Rejected a valid commit at ${count} configured types. Above ~6051 the old ` + + 'alternation exceeded bash\'s 64 KiB regcomp cap; [[ =~ ]] returned 2 and ' + + '`if !` read that compile error as "does not conform".', + ); + }); + } + + test('CONTROL: the pre-fix alternation blocks a valid commit wherever the engine caps pattern size', (t) => { + const hook = makeHookLayout(t, toPreFixAlternation); + const dir = makeProject(t, ALT_FIRST_FAILING); + const res = runValidate(hook, dir, 'feat(auth): add login flow'); + assertSubstantive(res, 'pre-fix alternation control'); + + if (res.status === 0) { + // Self-calibrating rather than assuming a cap: this engine compiled the + // whole list-derived alternation, so the regcomp defect is NOT reachable + // here and there is nothing for this control to reproduce. Skipped out + // loud — never silently passed — because a green row here would otherwise + // read as "the defect is covered" on a platform where it cannot occur. + // glibc/bash 5.2 (the Linux tester image) measured exactly this. + t.skip( + `this platform's regex engine compiled a ${ALT_FIRST_FAILING}-type alternation, ` + + 'so it has no reachable compile cap and the regcomp defect cannot be ' + + 'reproduced. The defect and this control are specific to a capped engine ' + + '(bash 3.2 / BSD libc on macOS). The SIGPIPE control below is unaffected.', + ); + return; + } + + assert.equal( + res.status, + 2, + 'this engine rejected the alternation somewhere, but not as a block — the ' + + 'pre-fix reconstruction did not reproduce the regcomp defect, so the rows ' + + 'above would pass with or without the fix. Re-derive it before trusting them.', + ); + }); + + test('CONTROL: the pre-fix alternation still ACCEPTS one type below the cliff', (t) => { + // On a capped engine this pins the boundary's LOW side, so the limit-1 row is + // not vacuous there. On an uncapped engine it passes trivially — that is + // acknowledged, not hidden: the row it guards is the macOS-specific one, and + // the paired high-side control above skips out loud on such platforms. + const hook = makeHookLayout(t, toPreFixAlternation); + const dir = makeProject(t, ALT_LAST_COMPILING); + const res = runValidate(hook, dir, 'feat(auth): add login flow'); + assertSubstantive(res, 'pre-fix alternation control @ limit-1'); + assert.equal( + res.status, + 0, + `the pre-fix form already failed at ${ALT_LAST_COMPILING} types, so the ` + + 'measured regcomp boundary in this suite is wrong — re-bisect it.', + ); + }); + + test('CONTROL: the pre-fix `head -1` pipeline aborts instead of returning a verdict', (t) => { + const hook = makeHookLayout(t, toPreFixPipeline); + const dir = makeProject(t, SIGPIPE_PAYLOAD); + const res = runValidate(hook, dir, 'feat(auth): add login flow'); + // No assertSubstantive here, deliberately: the pre-fix abort happens while + // READING the config, before the classifier ever runs, so CLASSIFIER_THREW + // could never appear on this row and asserting its absence would prove + // nothing. Substantiveness of this layout is established by the paired + // shipped-hook row above, which returns a real verdict from the same + // payload and the same tmpdir construction. + // Deliberately NOT `status === 141`. The issue saw SIGPIPE (141) on Linux CI; + // on macOS bash 3.2 the BUILTIN printf reports "write error: Broken pipe" and + // the script exits 1 instead of dying from the signal. Pinning 141 would be a + // mac-red/Linux-green split. The platform-independent invariant is that the + // hook never reaches its own verdict. + assert.ok( + res.status !== 0 && res.status !== 2, + 'the pre-fix pipeline form returned a real verdict, so it did not reproduce ' + + `#4429 — this control proves nothing. status=${res.status}`, + ); + // The EVIDENCE differs by platform and the remote matrix is Linux-only, so + // this must accept both. On Linux (bash >= 4.3) the process is killed by + // SIGPIPE -> status 141 with NO message. On macOS bash 3.2 the BUILTIN + // printf traps EPIPE and reports `write error: Broken pipe`, exiting 1. + // Asserting only the message would go red on every CI lane. + assert.ok( + res.status === 141 || /broken pipe/i.test(`${res.stdout}${res.stderr}`), + 'expected a broken-pipe abort of the `head -1` pipeline — either SIGPIPE ' + + `(141) or a reported write error. status=${res.status} ` + + `stderr=${res.stderr.slice(0, 200)}`, + ); + }); +}); + +// A second, quieter defect in the same file, found by the security review of +// this change: each subprocess status is captured as `... || VAR=$?`, which +// assigns ONLY on the failure branch, and is then read via `${VAR:-0}` — which +// defaults only when unset or empty. So on the SUCCESS path the variable kept +// whatever it already held, and an EXPORTED variable of that name (a CI wrapper, +// a .envrc, another hook) was read as a subprocess failure. Because the hook +// fails OPEN on a genuine subprocess failure by design (#3838), the result was a +// silent bypass: the gate printed "validator disabled for this call" and exited +// 0 on a commit it should have blocked. +describe('#4429 — subprocess statuses must not be inherited from the environment', { skip: isWindows }, () => { + const NON_CONFORMING = 'nope: definitely not conventional'; + + for (const varName of ['CONFIG_STATUS', 'CMD_STATUS', 'CLASSIFY_STATUS']) { + test(`an ambient ${varName} cannot disable the gate`, (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, 0); + const res = runValidate(hook, dir, NON_CONFORMING, { [varName]: '3' }); + assertSubstantive(res, `shipped hook with ambient ${varName}`); + assert.equal( + res.status, + 2, + `exporting ${varName}=3 disabled the commit gate — a non-conforming ` + + `commit was accepted. stderr: ${res.stderr.slice(0, 200)}`, + ); + }); + } + + test('all three set at once still cannot disable the gate', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, 0); + const res = runValidate(hook, dir, NON_CONFORMING, { + CONFIG_STATUS: '9', CMD_STATUS: '5', CLASSIFY_STATUS: '127', + }); + assertSubstantive(res, 'shipped hook with all three ambient'); + assert.equal(res.status, 2); + }); + + test('a conforming commit is still accepted with the variables set', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, 0); + const res = runValidate(hook, dir, 'feat(auth): add login flow', { CLASSIFY_STATUS: '3' }); + assertSubstantive(res, 'shipped hook, conforming, ambient CLASSIFY_STATUS'); + assert.equal(res.status, 0); + }); + + test('CONTROL: without the pre-init, an ambient status really does bypass the gate', (t) => { + const hook = makeHookLayout(t, toPreFixAmbientStatus); + const dir = makeProject(t, 0); + const res = runValidate(hook, dir, NON_CONFORMING, { CLASSIFY_STATUS: '3' }); + // Load-bearing, not ceremony: an orphaned layout ALSO exits 0 here, so + // without this the row would pass for entirely the wrong reason. Measured: + // the genuine bypass emits no CLASSIFIER_THREW, an orphaned layout does. + assertSubstantive(res, 'pre-fix ambient-status control'); + assert.equal( + res.status, + 0, + 'the pre-fix reconstruction did NOT reproduce the bypass, so the rows above ' + + 'would pass with or without the fix. Re-derive it before trusting them.', + ); + }); + + test('a GENUINE subprocess failure still fails open, as #3838 requires', (t) => { + const hook = makeHookLayout(t); + const dir = makeProject(t, 0); + // A `node` that always fails, rather than removing node from PATH: node + // lives in /usr/bin on many Linux images, so a PATH edit is not portable + // and would silently stop testing anything. + const binDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4429-bin-')); + t.after(() => cleanup(binDir)); + const shim = path.join(binDir, 'node'); + fs.writeFileSync(shim, '#!/bin/sh\nexit 3\n', { mode: 0o755 }); + + const res = runValidate(hook, dir, NON_CONFORMING, { + PATH: `${binDir}${path.delimiter}${hookEnv.PATH}`, + }); + // This row EXPECTS a fail-open, so exit 0 alone cannot tell "the node shim + // made the config read fail" from "the layout was broken and the classifier + // could not load". Only the latter emits CLASSIFIER_THREW, so this pins the + // pass to the cause the row actually names. + assertSubstantive(res, 'genuine fail-open row'); + assert.equal( + res.status, + 0, + 'a real subprocess failure must still disable the validator and pass (#3838); ' + + 'the pre-init must close the ambient bypass WITHOUT closing this path', + ); + assert.match(res.stderr, /validator disabled for this call/); + }); +});