diff --git a/.changeset/clever-rams-hop.md b/.changeset/clever-rams-hop.md new file mode 100644 index 000000000..f5fa9d336 --- /dev/null +++ b/.changeset/clever-rams-hop.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4537 +--- +**`/gsd-pr-branch` no longer silently drops a planning-only commit that mixes a structural `.planning/` path (STATE.md, ROADMAP.md, etc.) with a transient or other planning path** — such a commit matched none of the classification's four arms and was excluded, which could break `STATE.md`'s per-commit revision chain in default mode. A fifth arm now covers this shape and includes it, same as a mixed code+planning commit. (#4447) diff --git a/.changeset/plucky-jays-travel.md b/.changeset/plucky-jays-travel.md new file mode 100644 index 000000000..63b33bd63 --- /dev/null +++ b/.changeset/plucky-jays-travel.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4537 +--- +**Fixed an intermittent commit-hook failure (SIGPIPE race)** — `gsd-validate-commit.sh`'s subject/config extraction used `echo|head -1`-style pipes under `set -euo pipefail`; a real (multi-line) commit message or configured commit-type list could occasionally trip a SIGPIPE that aborted the whole hook instead of the intended pass/reject, appearing as a spurious `git commit` failure. Replaced with pure bash parameter expansion, eliminating the race entirely. diff --git a/gsd-core/workflows/pr-branch.md b/gsd-core/workflows/pr-branch.md index 3ffa66ff0..aad0ed79b 100644 --- a/gsd-core/workflows/pr-branch.md +++ b/gsd-core/workflows/pr-branch.md @@ -278,16 +278,27 @@ For each commit, check what it touches: FILES=$(git diff-tree --no-commit-id --name-only -r $HASH) NON_PLANNING=$(echo "$FILES" | grep -c -v "^\.planning/" || true) STRUCTURAL=$(echo "$FILES" | grep -Ec "$STRUCTURAL_RE" || true) +PLANNING_COUNT=$(echo "$FILES" | grep -c "^\.planning/" || true) ``` -Classify: -- **Code commits**: touch at least one non-`.planning/` file → INCLUDE (both modes) -- **Mixed commits**: touch code + any planning files → INCLUDE (both modes; the planning - paths are filtered out by `create_pr_branch`, not the commit) -- **Structural planning commits**: touch only structural `.planning/` files → INCLUDE in +Classify, using `NON_PLANNING`, `STRUCTURAL`, and `PLANNING_COUNT` computed above — every arm's +condition is explicit and computable so no reading of it is ambiguous: +- **Code commits**: `NON_PLANNING > 0` and `PLANNING_COUNT == 0` → INCLUDE (both modes) +- **Mixed code+planning commits**: `NON_PLANNING > 0` and `PLANNING_COUNT > 0` → INCLUDE (both + modes; the planning paths are filtered out by `create_pr_branch`, not the commit) +- **Structural-only planning commits**: `NON_PLANNING == 0` and `STRUCTURAL == PLANNING_COUNT` + and `PLANNING_COUNT > 0` (every `.planning/` file touched is structural) → INCLUDE in **default** mode; **EXCLUDE** in strict mode, which has no structural carve-out -- **Transient planning commits**: touch only `.planning/` paths that are not structural → - EXCLUDE (both modes) +- **Mixed planning commits (#4447)**: `NON_PLANNING == 0` and `STRUCTURAL > 0` and + `STRUCTURAL < PLANNING_COUNT` (some but not all `.planning/` files touched are structural — + the rest are transient and/or the "other" bucket, e.g. `config.json`/`intel/`) → INCLUDE in + **default** mode (the transient-dir subset of the non-structural paths is filtered out by + `create_pr_branch`'s universal per-commit filter exactly as for a mixed code+planning commit; + any "other" non-structural, non-transient path — `config.json`, `intel/`, etc. — is simply + preserved, same as default mode already does for such paths on any commit); **EXCLUDE** in + strict mode +- **Transient-only planning commits**: `NON_PLANNING == 0` and `STRUCTURAL == 0` and + `PLANNING_COUNT > 0` → EXCLUDE (both modes) In strict mode this collapses to a single rule: `NON_PLANNING > 0` → INCLUDE, else EXCLUDE. @@ -297,6 +308,7 @@ Commits to include: {N} (code changes{, + structural planning — default mode o Commits to exclude: {N} (planning-only) Mixed commits: {N} (code + planning — included, planning paths filtered) Structural planning commits: {N} ({included|excluded — strict mode}) +Mixed planning commits: {N} ({included — structural + transient/other, planning paths filtered|excluded — strict mode}) ``` diff --git a/hooks/gsd-validate-commit.sh b/hooks/gsd-validate-commit.sh index a6e5adf88..b171f3daa 100755 --- a/hooks/gsd-validate-commit.sh +++ b/hooks/gsd-validate-commit.sh @@ -69,7 +69,11 @@ if [ -f .planning/config.json ]; then echo "gsd-validate-commit.sh: could not read .planning/config.json (opt-in check) — validator disabled for this call. $(cat "$ENABLED_ERR")" >&2 exit 0 fi - ENABLED=$(printf '%s\n' "$CONFIG_OUT" | head -1) + # Pure parameter expansion, not `printf ... | head -1`: same SIGPIPE race + # class as the SUBJECT extraction below (`echo "$MSG" | head -1`) — CONFIG_OUT + # is multi-line whenever extra commit types are configured, and `head -1` + # closing early can SIGPIPE `printf` under `set -euo pipefail`. + ENABLED="${CONFIG_OUT%%$'\n'*}" if [ "$ENABLED" != "1" ]; then exit 0; fi # Remaining lines (if any) are the sanitized, deduped configured commit # types beyond the 10 built-ins (#3811). Read into a bash-3.2-safe array — @@ -521,9 +525,18 @@ if [ "$CLASSIFY_STATUS" = "0" ]; then SUBJECT=$(GIT_CMD_LIB="$HOOK_DIR/lib/git-cmd.js" MSG="$MSG" node -e " const {resolveCommitSubject}=require(process.env.GIT_CMD_LIB); process.stdout.write(resolveCommitSubject(process.env.MSG)); - " 2>/dev/null) || SUBJECT=$(echo "$MSG" | head -1) + " 2>/dev/null) || SUBJECT="${MSG%%$'\n'*}" else - SUBJECT=$(echo "$MSG" | head -1) + # Pure parameter expansion, not `echo "$MSG" | head -1`: that pipeline + # raced a SIGPIPE under `set -euo pipefail` whenever $MSG had a body + # (the common case) — `head -1` can close its read end as soon as it + # has the first line, and if `echo`'s write lands after that close, + # `echo` dies with signal 13 (exit 141), which is NOT suppressed by + # `set -e` and aborted the whole hook intermittently (observed in + # tests/hooks-opt-in.test.cjs's --fixup=HEAD "round 7" case). Zero + # subprocesses here means zero pipe/race surface. Equivalent to + # `head -1` for single-line, multi-line, and trailing-newline input. + SUBJECT="${MSG%%$'\n'*}" 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 diff --git a/tests/hooks-opt-in.test.cjs b/tests/hooks-opt-in.test.cjs index 3c586b2aa..0f146155e 100644 --- a/tests/hooks-opt-in.test.cjs +++ b/tests/hooks-opt-in.test.cjs @@ -1356,6 +1356,48 @@ EOF assert.strictEqual(runHookCmd(`git commit -m ${HD_OK}`).status, 0, 'non-vacuity: canonical form still resolves'); }); + test('subject extraction uses no pipe-to-head (SIGPIPE race, #4447 follow-on)', () => { + // `echo "$MSG" | head -1` (and the equivalent `printf ... | head -1` for + // the opt-in ENABLED flag) is a SIGPIPE race under `set -euo pipefail`: + // `head -1` can close its read end as soon as it has one line, and a real + // commit message/config output is multi-line, so `echo`/`printf` can die + // with signal 13 (exit 141) if its write lands after that close — which + // aborts the whole hook instead of the expected exit 2. This is a static, + // by-construction pin (not a timing repro, per this repo's policy against + // forcing scheduling races to reproduce deterministically): the fix + // (`${VAR%%$'\n'*}`, a pure parameter expansion with zero subprocesses and + // therefore zero pipe/race surface) must be present, and the vulnerable + // pipe pattern must be gone. + const hookSrc = fs.readFileSync(path.join(HOOKS_DIR, 'gsd-validate-commit.sh'), 'utf8'); + // Regexes require the `$(...)` command-substitution wrapper so this only + // matches the actual dangerous CODE pattern, never the explanatory prose + // comments left at the fix site (which quote the bare pipeline, without + // the `$(...)` wrapper, for documentation purposes). `\s+`/`\s*` tolerate + // incidental reformatting (extra spaces, an appended `2>/dev/null`, etc.) + // so a cosmetically-reworded reintroduction of the SAME dangerous shape + // does not silently escape this check (review finding: an exact-string + // match would). + // Requires the `$(...)` command-substitution wrapper (real CODE, never + // the explanatory prose comments left at the fix site, which quote the + // bare "$MSG" | head -1 / "$CONFIG_OUT" | head -1 shape WITHOUT a `$(` + // in front — an earlier, unwrapped version of this same regex matched + // those comments and false-failed). Content between `$(` and `"$MSG"`/ + // `"$CONFIG_OUT"` and between the quote and `head -1` is a tolerant + // `[^)]*`, so a cosmetically-reworded reintroduction of the SAME + // dangerous shape (extra spaces, an appended `2>/dev/null`, a different + // command before the pipe) does not silently escape this check — only + // the presence of a real `$( ... "$MSG" ... | head -1 ... )` / + // `$( ... "$CONFIG_OUT" ... | head -1 ... )` substitution matters. + assert.ok(!/\$\([^)]{0,200}"\$MSG"[^)]{0,200}\|\s*head\s+-1[^)]{0,200}\)/.test(hookSrc), + 'no $(...) command substitution may pipe "$MSG" into head -1 (SIGPIPE race under set -o pipefail)'); + assert.ok(!/\$\([^)]{0,200}"\$CONFIG_OUT"[^)]{0,200}\|\s*head\s+-1[^)]{0,200}\)/.test(hookSrc), + 'no $(...) command substitution may pipe "$CONFIG_OUT" into head -1 (SIGPIPE race under set -o pipefail)'); + assert.ok(hookSrc.includes('SUBJECT="${MSG%%$\'\\n\'*}"'), + 'subject extraction must use the pure parameter-expansion form'); + assert.ok(hookSrc.includes('ENABLED="${CONFIG_OUT%%$\'\\n\'*}"'), + 'ENABLED extraction must use the pure parameter-expansion form'); + }); + test('validate-commit does not trust a relative path ending in cat (round-4 Codex MAJOR)', () => { // Recognition accepted any path ending in `/cat`, so a planted `./cat` or // `../evil/cat` was trusted to echo its stdin. With such an executable diff --git a/tests/pr-branch-planning-filter.test.cjs b/tests/pr-branch-planning-filter.test.cjs index 62fb0b7ad..5b0271785 100644 --- a/tests/pr-branch-planning-filter.test.cjs +++ b/tests/pr-branch-planning-filter.test.cjs @@ -164,6 +164,26 @@ describe('#2971 — pr-branch.md planning.pr_strict filter (failing-first)', () assert.strictEqual(classifyCommit([], strictOpts), 'exclude'); }); + // #4447: a `.planning/`-only commit that mixes a structural path with a + // non-structural planning path (transient-dir or the "other" bucket) is + // the exact shape the workflow's prose could not classify — the four + // arms as written never compute a total planning-file count, so they + // cannot tell "only structural" from "structural plus something else". + // The JS model here already resolves it correctly (`classifyCommit`'s + // structural check has no upper bound against the total), so these pin + // that behavior; the actual defect is fixed in the workflow prose itself + // (see test 51). + test('49: #4447 [.planning/STATE.md, .planning/phases/PLAN.md] (structural + transient, no code) includes default, excludes strict', () => { + const files = ['.planning/STATE.md', '.planning/phases/PLAN.md']; + assert.strictEqual(classifyCommit(files, defaultOpts), 'include'); + assert.strictEqual(classifyCommit(files, strictOpts), 'exclude'); + }); + + test('50: #4447 [.planning/STATE.md, .planning/config.json] (structural + third-bucket, no code) includes default', () => { + const files = ['.planning/STATE.md', '.planning/config.json']; + assert.strictEqual(classifyCommit(files, defaultOpts), 'include'); + }); + test('8: [.planning/STATE.md, src/a.ts] — forbiddenPaths [] default, [.planning/STATE.md] strict', () => { const files = ['.planning/STATE.md', 'src/a.ts']; assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); @@ -842,5 +862,28 @@ describe('#2971 — pr-branch.md planning.pr_strict filter (failing-first)', () 'the unconditional "No .planning/ files in PR branch diff" success line must be removed/replaced', ); }); + + // #4447: this pins the actual documented defect — ambiguous prose read by + // an LLM executing the workflow, not the already-correct JS model in + // pr-branch-filter.cjs (see tests 49/50). Before the fix, `analyze_commits` + // computed FILES/NON_PLANING/STRUCTURAL but never a total planning-file + // count, so its four classification arms had no way to distinguish + // "only structural" `.planning/` commits from "structural plus a + // transient/other `.planning/` path" — that second shape matched none of + // the four arms and was silently dropped, breaking STATE.md's per-commit + // revision chain in default mode. This must FAIL against the original + // (unedited) step text, which had no `PLANNING_COUNT=` assignment at all. + test('51: #4447 analyze_commits computes an explicit total .planning/ file count (PLANNING_COUNT), closing the gap that let a structural+transient/other planning commit match none of the four classification arms', () => { + const text = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const stepMatch = text.match(/([\s\S]{0,20000}?)<\/step>/); + assert.ok(stepMatch, 'analyze_commits step not found in pr-branch.md'); + assert.match( + stepMatch[1], + /^PLANNING_COUNT=\$\(/m, + 'analyze_commits must compute an explicit total planning-file count via a real shell ' + + 'assignment (PLANNING_COUNT=$(...)) so the classification arms can distinguish ' + + '"only structural" planning commits from "structural plus transient/other" ones', + ); + }); }); });