78013b3b74
* 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>
6 lines
463 B
Markdown
6 lines
463 B
Markdown
---
|
|
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)
|