* fix(#4447): classify mixed structural+transient planning commits as a 5th arm pr-branch.md's analyze_commits step computed only NON_PLANNING and STRUCTURAL per commit -- never a total planning-file count -- so its four classification arms assumed every planning-only commit was either wholly structural or wholly non-structural. A commit touching both a structural .planning/ path and a transient/other one matched no arm, and the ambiguous prose let an LLM executing the workflow silently drop it, breaking STATE.md's per-commit revision chain in default mode. Adds an explicit PLANNING_COUNT variable and rewrites the four arms into five, each with an exact computable condition. The new "mixed planning commit" arm (structural + transient/other, no code) gets the same treatment mixed code+planning commits already get: INCLUDE, relying on create_pr_branch's existing universal per-commit filter to strip the transient/other paths -- no new filtering logic needed. tests/helpers/pr-branch-filter.cjs's classifyCommit already returned 'include' for this shape (no upper bound on its structural check); the defect was entirely in the workflow's own prose spec, which is what an executing agent actually reads. New tests pin both: classifyCommit's already-correct behavior (tests 49-50), and a failing-first assertion that analyze_commits computes an explicit planning-total signal (test 51, fails against the pre-fix text). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4447): address code-review findings on the mixed-planning arm - Correct the mixed-planning arm's prose: create_pr_branch's universal filter only strips the TRANSIENT_DIRS subset, not the "other" bucket (config.json, intel/, etc.) -- that subset is preserved, not filtered, same as default mode already does for it on any commit. - Fix the "Mixed planning commits" display line to use the same mode-conditional bracket form as "Structural planning commits" -- it was hardcoding "included" even though the arm is EXCLUDE in strict mode, which would have misled a strict-mode user. - Tighten test 51's regex from unanchored /PLANNING_COUNT=/ to /^PLANNING_COUNT=\$\(/m so it requires the real shell-assignment shape, not just the substring appearing anywhere in prose. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Emitted-Drift-Ack-Growth: pr-branch.md — growth is this PR's own #4447 fix (5th classification arm with explicit computable conditions), not incidental drift * fix(#4447): bound test 51's regex quantifier (local/no-unbounded-quantifier) lint:ci flagged the unbounded [\s\S]*? over readFileSync content as a catastrophic-backtracking risk (CWE-1333 class). Bounded to {0,20000}, comfortably larger than the analyze_commits step's actual size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4447): add changeset for the pr-branch mixed-planning classification fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: eliminate SIGPIPE race in gsd-validate-commit.sh subject/config extraction Discovered while verifying an unrelated PR (#4447): tests/hooks-opt-in.test.cjs's "a git-GENERATED subject is never measured against the supplied message (round 7)" test intermittently got r.status===141 instead of the expected 2 for the --fixup=HEAD case, on a run where the identical code had passed cleanly moments earlier -- confirming a timing race, not a deterministic bug in the test's own assertions. Root cause: gsd-validate-commit.sh runs under `set -euo pipefail` and extracted the commit subject via `SUBJECT=$(echo "$MSG" | head -1)` (two call sites) and the opt-in ENABLED flag via `$(printf '%s\n' "$CONFIG_OUT" | head -1)`. `head -1` closes its read end as soon as it has one line; a real commit message or multi-command-type CONFIG_OUT is multi-line, so the writer can receive SIGPIPE (exit 128+13=141) if its write lands after that close. Under pipefail this is NOT suppressed -- it aborts the whole hook instead of the intended exit-2 rejection. Fix: replace both patterns with pure bash parameter expansion (`${VAR%%$'\n'*}`) -- zero subprocesses, zero pipe/race surface, and behaviorally identical to `head -1` for single-line, multi-line, and trailing-newline input (verified directly). The third similar pipe (`tail -n +2` feeding a `while read` loop that drains to EOF) is a different, race-free shape and was left alone. Regression test is a static, by-construction assertion (per this repo's policy against forcing scheduling races to reproduce deterministically): the vulnerable pipe patterns must be absent from the shipped script, and the parameter-expansion forms must be present. This overrides one-concern-per-PR per CLAUDE.md's Defects & Warnings policy -- a genuine defect discovered mid-work is fixed inline, not deferred to a separate issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add changeset for the gsd-validate-commit.sh SIGPIPE race fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: harden SIGPIPE-race regression test against reformatted reintroduction Code review found the test's original exact-string regexes would miss a cosmetically-reworded reintroduction of the same dangerous head-1 pipe (extra whitespace, an appended 2>/dev/null). Broadened to content-tolerant but still $(...)-wrapped regexes (bounded quantifiers per local/no-unbounded-quantifier) -- verified against both the current file (no false positive, including the fix's own explanatory comments that quote the bare unwrapped pattern in prose) and a synthetic reformatted reintroduction (correctly caught). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4447): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/clever-rams-hop.md
Normal file
5
.changeset/clever-rams-hop.md
Normal file
@@ -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)
|
||||
5
.changeset/plucky-jays-travel.md
Normal file
5
.changeset/plucky-jays-travel.md
Normal file
@@ -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.
|
||||
@@ -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})
|
||||
```
|
||||
</step>
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(/<step name="analyze_commits">([\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',
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user