733bec3ad11bea8226ef389a0df519fb14aa673c
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
4e1c449281 |
enh(#3811): add hooks.commit_types config surface to gsd-validate-commit (#4340)
* enh(#3811): add hooks.commit_types config surface to gsd-validate-commit Extends the opt-in Conventional Commits hook with a hooks.commit_types config array that adds project-specific types to the 10 built-ins without replacing them. Configured values pass a safe-token filter before reaching the compiled regex, so a config entry can never alter the pattern's structure. The regex alternation, the human-readable error text, and a new typed valid_types JSON field all derive from one list instead of the two hand-synced copies this replaces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3811): 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> |
||
|
Behruz Nassre Esfahani
|
4499933807 |
fix(#3802): resolve the heredoc body before validating the commit subject (#3816)
* fix(#3802): resolve the heredoc body before validating the commit subject With hooks.community: true, gsd-validate-commit.sh blocked EVERY heredoc-form commit with CONVENTIONAL_COMMITS_VIOLATION regardless of the message, including Claude Code's own documented idiom: git commit -m "$(cat <<'EOF' feat(auth): add login flow EOF )" Reproduced before changing anything: conforming heredoc -> exit 2; plain -m "feat(auth): add login flow" -> exit 0. Root cause is the extraction regex `-m[[:space:]]+"([^"]+)"`. Bash `[^"]` matches newlines, so the capture ran from the quote after -m to the FINAL quote at `)"`, swallowing the whole span. `head -1` then returned the literal `$(cat <<'EOF'` as the subject, which can never satisfy Conventional Commits. Fixed by not answering a regex bug with another regex. hooks/lib/git-cmd.js already exists because "a naive regex misses all three" invocation forms, and extractBranchArgument is the established precedent for pulling an argument off a git command line. extractCommitSubject joins it on the same tokenizeShellLike seam — which, checked first, already returns the entire heredoc span as ONE token, leaving only "resolve the body to its first line" as new logic. Because the walk starts at the subcommand, `git -C <path> commit` and env-prefixed invocations now extract correctly too — forms the raw string scan never handled. Deliberately unchanged, and pinned as such: a glued `-mfeat: x` and `--message=...` still yield no message, exactly as the regex left them. The fix stays scoped to the reported defect rather than widening on a true observation. Two things I got wrong and corrected by measuring rather than reasoning: - I expected `git commit -m ""` to be blocked. Checked against the ORIGINAL hook: allowed before, allowed now, identical. The scanner drops the empty token so it takes the null path. My expectation was wrong, not the code. - That exposed a false comment I had just written, claiming the exit-status split prevents silently allowing `-m ""`. It does not. The split IS load-bearing, but for a heredoc whose body's first line is blank, which resolves to an empty subject and is correctly blocked. The comment now names the real case and records that `-m ""` is not it. Tests at both layers: 9 unit rows on extractCommitSubject beside its sibling in tests/worktree-safety.test.cjs, and 5 behavioral rows piping real PreToolUse payloads through the hook in tests/hooks-opt-in.test.cjs. Replacing firstLineOfMessageArg with a plain first-line return reds 8 of them across both files. (A first mutation attempt silently no-opped and reported green — the mutated body is echoed in the transcript for the run that counted.) Out of scope, per the issue: the hooks.commit_types config surface, split off by the maintainer as #3811 and explicitly sequenced after this. Verified: `npm run lint:ci` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3802): confine the fix to heredoc resolution, closing four regressions Codex review of the first attempt. It was right, and the finding is one my own rules already name: a true observation is not a licence to widen the diff. The first attempt replaced the shell's `-m` extraction with a token walk. That looked like the better abstraction — this module exists precisely because a naive regex misses invocation forms — but selecting WHICH argument is the message was never the defect, and changing it regressed four forms that upstream allowed, plus opened a bypass: - `git commit -- -m WIP` -- introduces pathspecs; `-m` is a path - `git commit --amend && echo -m WIP` a later command's flag became the message - `git commit -m "" --allow-empty-message` the shared scanner drops empty tokens, so the next flag became the message - `git commit -m WIP` unquoted argument - `-m "WIP notes <<EOF\nfix: smuggled subject"` was ALLOWED — the opener was recognised unanchored, so validation skipped past the real, non-conforming subject. An enforcement bypass, not a misclassification. Now confined to the actual defect. The shell's `-m` capture is restored byte for byte, and only the subject-from-message step is delegated, to a PURE STRING helper `resolveCommitSubject()` that never tokenizes. Verified as a differential against the upstream hook run inside the real tree: the only behaviours that change are the two intended heredoc rows (2 -> 0); all four forms above read identical, and the bypass case blocks. That differential also corrected my own control. An earlier comparison ran the upstream hook from a scratch directory, where its `lib/` could not resolve `../../gsd-core/bin/lib/token-scanner.cjs`, so the classifier failed open and reported exit 0 for everything. That made a real regression look pre-existing. Re-run inside the tree, `<<-"TAG"` (a double-quoted tag nested in the double-quoted argument) is genuinely pre-existing — the capture truncates — and is now recorded as a known limitation rather than silently "fixed". Also fixed from the review: - `<<-` strips leading TABS from body lines; returning the raw line blocked a conforming message. - a non-identifier tag such as `END-MSG` is a valid bash word and was rejected. - an immediately-following terminator is an EMPTY message, not a subject. - a node/library failure now falls back to the previous `head -1` instead of skipping validation, so a broken extractor degrades to old behaviour rather than becoming a new silent-allow path. Tests strengthened per the review: the opener-spelling rows now assert BOTH directions per spelling, since "conforming passes" alone would also pass if the resolver returned an empty subject for a spelling it failed to parse. Added differential rows pinning the five previously-allowed forms, and a row for the bypass. Dropped two rows whose comments claimed the raw scan could not handle `-C`/env-prefix invocations — it could; the claim was wrong. Replacing resolveCommitSubject with a plain first-line return reds 9 rows across both files. (Mutant body echoed in the transcript; an earlier mutation attempt on this branch silently no-opped and reported green.) Verified: `npm run lint:ci` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3802): keep the installed hook runtime-neutral `hooks/lib/git-cmd.js` ships into every runtime, including hermes and qwen, where tests/install.test.cjs enforces that no Claude reference leaks into the installed tree. My JSDoc named the idiom after the runtime that documents it. Reworded to describe the SHAPE rather than the vendor; the runtime is still named in the changeset, which feeds CHANGELOG.md where such references are allowed, and in the tests, which are not installed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#3802): backfill changeset pr number The fragment shipped with the documented `pr: 0` placeholder, which the changeset lint treats as always-silent, because the number does not exist until the PR is opened. Backfilled to 3816 now that it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3802): close the truncated-capture hole, add the required test artifacts Review round 1. Major 3 was the one that mattered, and it disproved a claim I had stated in falsifiable form — the PR body said only two behaviours change; the differential found five. Major 3 — an embedded `"` truncates the `-m` capture, so the resolver received a PREFIX of the real subject and the length gate measured the wrong string. Before this fix the whole form was blocked outright, so the gate was unreachable; the fix opened the path and then mismeasured it. A new enforcement hole, so it is CLOSED here rather than declared. Closed precisely rather than bluntly. A first attempt refused to resolve any body with no terminator, which also blocked commits whose SUBJECT was intact and whose quote sat further down the body — a false positive of its own. Truncation is only fatal to the line it lands IN, and a captured line is complete exactly when another line follows it, because the capture kept its newline. So an unterminated body whose subject line is followed by more text stays measurable; only a subject line running to the end of a truncated capture falls back to the opener, which fails the format gate exactly as this form did before the fix. Major 1 — fast-check property rows for the new parser, via the shared seeded setup helper rather than requiring fast-check directly, per repo convention: totality (a security property here, since an exception on this path fails OPEN), idempotency, and that the result is always a single line drawn from the input — the third catches a resolver that concatenated or trimmed while satisfying the first two. Major 2 — the 72-char gate is now exercised at {71, 72, 73} on the RESOLVED heredoc subject, with the fixture length asserted so a mis-built fixture cannot silently pass. 92 chars did not show which side of `> 72` the code sits on. Minor 1 — leading blank body lines are skipped, as git's cleanup=whitespace does. A conforming commit written that way was still blocked, which is the same defect class #3802 reports. Nit 1 — a backslash-escaped delimiter (`<<\EOF`) is now the same delimiter rather than failing closed on a delimiter that includes the backslash. Nit 5 — changeset trimmed from 2,208 chars of design note to the user-visible change. Mutation discipline, including a correction to my own: dropping the truncation guard reds the unit rows, and the pre-review naive shape reds the hook-level row too. My first mutant did NOT distinguish the hook row — removing the guard made an empty slice and blocked for an unrelated reason, so the row passed and looked proven. Only mutating to the actual pre-review shape showed it discriminates. Verified: `npm run lint:ci` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3802): measure the subject as git does — strip trailing whitespace, split CRLF git's cleanup=whitespace strips whitespace at BOTH ends of a line; the resolver handled only the leading direction, so a 72-char subject with trailing spaces measured 75 and stayed blocked — the defect class #3802 reports, surviving one round further (review of #3816, Major 2). The resolved subject now drops trailing spaces and tabs; the plain non- heredoc path is untouched, keeping the fix confined to heredoc resolution. The length-gate boundary rows gain dirty fixtures: 72+3 trailing spaces passes, 73+1 stays blocked on LENGTH. split('\n') left \r on every body line, so on CRLF input the delimiter never matched: the truncation guard was inert, an empty message resolved to 'EOF\r', and a real 72-char subject measured 73. Split on /\r?\n/ (Minor 3). The three property tests never reached the parser — the pinned-seed fc.string corpus contained no newline and no opener, so every property reduced to f(s) === s (Major 1). The generator now constructs heredoc- shaped input (all opener spellings, <<- tabs, optional terminator, CRLF) and each property asserts a floor on inputs its corpus actually resolved. All new rows proved failing-first against the pre-fix resolver. Also records the unquoted-delimiter expansion limit as one JSDoc sentence (Informational 5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#3802): close two recognition bypasses, pin the dquoted-delimiter limit Codex whole-PR review found two enforcement bypasses in the resolver: - The opener's path prefix was \S*, which accepted `id;/bin/cat` — the resolver then validated the heredoc BODY while bash runs `id` first and git's real subject is id's OUTPUT. The prefix is now a path-character class; any shell metacharacter fails recognition and the form falls back to the opener line and the format gate. - The blank-line skip used JavaScript trim(), whose Unicode whitespace class skips lines git KEEPS: a NBSP first body line resolved to the SECOND line while git's real subject is the NBSP line (verified against git stripspace — the c2a0 bytes survive). Blank is now git's ASCII space/tab only; a Unicode-blank line is returned and fails the format gate, the same fail-closed direction git takes. Both proven failing-first at resolver AND hook level. Also: the <<"TAG" spelling is recorded as a documented limit — the -m capture stops at the delimiter's own quote so the caller can never deliver it (fail closed; widening the capture would change every embedded-quote case) — with a hook-level row pinning the limit; and the derivation property no longer accepts '' unconditionally, only for heredoc-shaped input, so a conditional constant-'' regression can't satisfy the corpus floor unnoticed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#3802): recognition whitespace is ASCII, and '' answers to the generator Codex round 2: the opener's \s accepted Unicode whitespace bash does not split on — $(<NBSP>/bin/cat was recognized here while bash reads <NBSP>/bin/cat as the executable NAME, so recognition claimed a substitution that does not run cat. Every whitespace position in the recognition is now [ \t], the same ASCII rule as the blank-line skip, proven failing-first. The derivation property's ''-acceptance now consults GENERATION-TIME metadata: the heredoc generator records whether it built an empty message (terminator reachable, all scanned lines ASCII-blank, <<- tab stripping accounted for), and '' is accepted exactly then — a resolver conditionally degrading to '' on non-empty heredocs now fails, closing the residual round-1 permissiveness without re-deriving resolver logic. The changeset no longer overstates the opener spellings: it names the capture-deliverable set and the documented <<"EOF" limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#3802): nothing after the terminator escapes measurement Round-3 BLOCKER: everything after the heredoc terminator was silently discarded, so `-m "$(cat <<'EOF'\nfeat: ok\nEOF\n) <200 a's>"` — one 200+ char real subject once bash substitutes — measured 8 chars and dodged COMMIT_SUBJECT_TOO_LONG, a hole the base did not have. The canonical idiom's tail is exactly one closing-paren line; any other tail now falls back to the opener line and the format gate, the pre-fix behaviour for the whole form. Proven failing-first at resolver and hook level, including the glued-text and second-substitution variants. Also from round 3: `cat<<'EOF'` (no space) is legal bash and now resolves — the token before << is still literally cat; the env-prefixed and option-terminated spellings join the JSDoc KNOWN LIMIT list instead (fail closed, modelling bash prefix words is cost with no reported user); the changeset states the embedded-quote truncation limit for the message body, not just the <<"EOF" spelling; the dquoted unit and hook rows now cross-reference each other; and the fast-check setup helper's docstring no longer claims property-file exclusivity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#3802): glued text outside the closing quote must not shrink the measurement Codex on the round-3 guard: bash concatenates -m "$(…)"suffix into ONE argument, but the capture holds only the quoted part — so the resolver measured the heredoc body (8 chars) for a 200+ char real subject, a net-new length-gate bypass the base did not have (base measured the opener and blocked). When the closing quote is followed by anything but whitespace or end-of-command, the hook now skips the resolver and keeps the pre-fix first-line subject: the heredoc form fails the format gate exactly as on base, and the plain single-line form keeps base behavior unchanged — both pinned as differential rows, the glued-suffix row proven failing-first against the unguarded script. The property generator's ''-oracle now models the post-terminator guard it previously predated: expectEmpty requires the FIRST reachable terminator to be followed by the one canonical closing-paren line, so a resolver regressing to '' on a non-canonical tail (e.g. a body line that doubles as an early terminator) fails the derivation property instead of being blessed by stale metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: retrigger CI — the previous wave never started (Actions queue stall) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#3802): only resolve a heredoc whose body bash does not rewrite Round-4 review found two net-new enforcement bypasses: commands the base hook blocked (exit 2) that this branch allowed (exit 0). Both reproduced as a base-vs-head differential against the real hook, not inferred. The predicate "may I resolve this?" was computed from the resolver's input string alone, while two of its determinants live outside that string: 1. WHICH -m quote arm produced the input. Inside -m '...' bash performs no command substitution, so $(cat <<'EOF' is literal text and git's real subject is the opener line. The resolver ran on both arms, so all four delimiter spellings went 2 -> 0 on the sq arm — reachable by the ordinary slip of typing ' for ". The hook now records MSG_QUOTE and gates the resolver on dq; sq keeps head -1, exact base parity. 2. WHETHER the delimiter suppresses expansion. Only <<'D', <<"D" and <<\D do; a bare <<D is expanded by bash before git sees it. Resolving the literal dodged the format gate (feat: $UNSET_VAR reaches git as feat:) and the length gate (feat: ${LONG} reaches it at any length). The opener regex now separates the backslash-quoted and bare alternatives and refuses the bare one — the same fail-closed rule the metacharacter, truncation and post-terminator guards already follow. A test row asserted exit 0 for a bare-delimiter body, so the suite defended the second bypass and the fix could not land without editing a test that read as intentional. That row and its two unit counterparts now assert the block, per RULESET.TESTS.delete-bad-tests. Two unrelated rows used <<-EOF to exercise tab stripping; they move to <<-'EOF' so each tests what it names. Scoping the adjacency guard to the matched arm — required by the fix above — also removes a spurious block (round-4 Minor 1): a double-quoted heredoc whose body mentioned a glued single-quoted token tripped the sq arm. The JSDoc claimed <<"EOF" was unreachable through the caller and that the bare-delimiter gap was pre-existing. Round 4 disproved both; both corrected here, along with the matching changeset sentence. Verified: 7 bypass commands now block at head (was allow), the #3802 fix and plain-form parity are unchanged across 8 control commands, hooks-opt-in 44/44, worktree-safety 401/401, property-test non-vacuity 73/200 against a floor of 20, lint:ci exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EAbQy7n4mLMB7h3TnZ8GdG * fix(#3802): resolve only where the captured text is provably git's subject Codex review of the full PR found two more inputs where the validated text is not the subject git receives, both net-new bypasses (base 2 -> head 0), plus one escalation of round-4 Minor 2. All reproduced here against the real hook and confirmed against real commits before fixing. BLOCKER — the matched -m need not be git's message. The capture is a search over the whole command and the double-quoted arm runs first, so it could select a -m that is not the subject at all. git concatenates multiple -m values and takes the FIRST as the subject, so git commit -m 'WIP first' -m "$(cat <<'EOF' … )" commits the subject `WIP first` while the hook validated the heredoc. Same for an unquoted earlier -m, for a heredoc after `--` (a pathspec, not a message), and for one belonging to a later `&& echo`. The mis-selection is pre-existing; resolving it is what made it a bypass. The hook now resolves only when nothing before the matched -m could have been an earlier message, an end-of-options marker, or another command. BLOCKER — cleanup mode is part of the predicate. The resolver skips leading blank lines and strips trailing whitespace because git's DEFAULT cleanup=whitespace does. Under --cleanup=verbatim git does neither, so a 72-char subject plus three trailing spaces is committed at 75 bytes while the hook measured 72 — COMMIT_SUBJECT_TOO_LONG dodged. This one hides from `git log --pretty=%s`, which strips trailing whitespace in its own output; the raw commit object shows 75 vs 72. Any named mode other than whitespace, in either the --cleanup= or -c commit.cleanup= form, now refuses to resolve. MAJOR — recognition trusted any path ending in /cat, so a planted `../evil/cat` printing `WIP injected` had its heredoc body validated while git's real subject was `WIP injected`. Only a bare `cat` or an absolute path is recognised now. A bare `cat` shadowed on PATH is a documented residual and is not fixable from a string — nor a meaningful boundary, since planting an executable already allows running git directly. The changeset and the JSDoc both asserted that a `"` anywhere in the message blocks. Measured false: a `"` on a later body line resolves fine, because the subject completes before the truncation point; only a `"` in the subject line blocks. The changeset also listed <<"EOF" as covered when it measures 2/2. Both rewritten to claim only what is measured, and the residual false positives are now named. Verified: 4 + 2 + 3 new bypass commands now block, with non-vacuity controls proving the default path still resolves; all round-4 maintainer blockers stay closed; the #3802 fix and plain-form parity unchanged across 7 controls; hooks-opt-in 47/47, worktree-safety 402/402, property non-vacuity 73/200 against a floor of 20, lint:ci exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EAbQy7n4mLMB7h3TnZ8GdG * fix(#3802): scope the cleanup-mode guard to the command outside the message The guard scanned the whole $CMD for `--cleanup=` / `commit.cleanup=`, and the heredoc BODY sits verbatim inside $CMD, so any conforming message that merely MENTIONED the token was refused, fell back to the opener line, and was blocked with CONVENTIONAL_COMMITS_VIOLATION. These are ordinary English in this repository, whose own hooks and docs discuss cleanup modes constantly. Reproduced against the real hook: `fix: document commit.cleanup=strip behavior` blocked, the same message without the token allowed (review of #3816, round 5 — BLOCKER). Scoping to $MSG_PREFIX alone, as prescribed, would have reopened the round-4 length-gate bypass the guard exists for: git accepts the flag on EITHER side of -m, and `git commit -m "<heredoc>" --cleanup=verbatim` is caught today only because the scan is command-wide. Measured, not assumed. The scan now covers MSG_PREFIX + MSG_SUFFIX — the whole command minus the one span that is message text — joined with a space so a token cannot be forged across the seam. Swept the guard class rather than the reported instance. The adjacency guard does not share the defect: an in-body `-m "foo"bar` is refused by the already-documented embedded-quote capture limit (any `"` in the subject line truncates the capture), and an in-body `-m ` without quotes resolves and is allowed. Deliberately untouched. Both directions pinned failing-first: the three false-positive rows red against the unscoped guard, and the trailing-flag row reds against prefix-only scoping. Each mutation was echoed back to prove it landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XogDtuuuGQEfsWaLSaZCLB * fix(#3802): read commit options the way bash hands them to git Round 6 reported the adjacency guard scanning all of $CMD for a glued `-m "..."`, so a glued -m belonging to a chained-after command refused a heredoc that was never truncated. Glue is a property of the ONE character following the matched span, so that character is now the whole window. Separators and redirections are excluded because bash does not concatenate across them: in `-m "msg"&& echo hi` the argument ends at the quote, so there is no truncated capture to defend against. An independent full-PR pass then found three accept-direction defects this PR had introduced in earlier rounds, each measured against a real commit by reading the raw commit object — `git log --pretty=%s` strips the trailing whitespace that makes the length wrong and hides it: --cle=verbatim git accepts any unambiguous prefix of a long option, so the mode was set by a token that is not the literal --cleanup. 75-byte subject recorded, 72 measured. -am 'WIP first' git reads this as -a -m, so the real subject is `WIP first` and the heredoc is only the second message. The scan looked for a standalone -m. --clean""up=verbatim bash removes quotes before git sees the -""m argument, so a spliced spelling is the same option and matched no literal. The two option-name scans now read their window with quote characters removed, which is what bash does to it, and the cleanup class covers git's abbreviations. The adjacency test deliberately keeps the raw text: it asks about a literal character position, not an option name. Narrowing the cleanup window to git's own command segment was tried and reverted. `;`, `&` and `|` end a command only outside quotes, and this is a substring scan, not a parse: an unconditional trim cut the window short on `--author "a&b"`, and a quote-aware trim still cut it on `--author a\&b`. Each hid a real trailing --cleanup=verbatim and accepted a 75-byte subject. The resulting false positive — a --cleanup carried by a chained command refuses the commit — is documented and pinned instead. Refusing a commit git would take is recoverable; accepting an over-long subject is not. Sixteen rows in tests/hooks-opt-in.test.cjs. Seven mutations, including both reverted narrowings, so no dead end can be reintroduced silently. * fix(#3802): close six accept-direction bypasses in the resolve guards Round 7's FIRST-MESSAGE GUARD Major does not reproduce. Measured against the real hook in a complete tree at the reviewed head: the classifier gate runs before any guard, so `git add -A && git commit …` (git->add stops on a non-commit subcommand) and `cd dir && git commit …` (the first executable is not git) exit 0 without a guard being evaluated. The control is the proof — a subject the bare form blocks with CONVENTIONAL_COMMITS_VIOLATION exits 0 in both chained forms, so the hook never validated them and cannot be over-blocking them. The guard is unchanged; scoping this scan to $MSG_PREFIX alone is what reopened the round-4 trailing-flag bypass. The class was real, though, one shape further out: `FOO=bar; git commit …` IS classified and then refused, because assignment detection is prefix-anchored and the tokenizer does not split operators. Pinned as a counterexample and disclosed rather than generalised away; narrowing it means changing isGitSubcommand, the shared git-commit detector every gating hook uses, and it fails closed. Six accept-direction bypasses are fixed. Each let the hook resolve and ALLOW a commit whose real subject the rules refuse; the three that turn on git's recorded subject were confirmed against the RAW COMMIT OBJECT, since `git log --pretty=%s` strips trailing whitespace and hid two of them: --cleanup=whitespace -m <72+spaces> --cleanup=verbatim git kept 75 bytes -mWIP -m <heredoc> git recorded `WIP` --mes=WIP -m <heredoc> git recorded `WIP` -\m WIP -m <heredoc> git recorded `WIP` git commit --amend --no-edit \n echo -m <heredoc> echo's argument read --squash=HEAD -m <heredoc> `squash! …` Causes: one BASH_REMATCH inspected only the FIRST cleanup directive while git applies the last, so multiplicity now refuses rather than guesses at an argument order a substring scan cannot recover; the option scan required a trailing space or `=`, missing attached values and long-option abbreviations; dequoting removed quotes but not the syntactic backslashes bash also removes; the separator scan omitted newline; and --squash/--fixup have git compose the subject, so the supplied message is not the subject at all. Every fix widens refusal, the direction this file documents as recoverable. The multiplicity count first broke the hook outright: the script runs under `set -euo pipefail` and grep exits 1 when it matches nothing, which is the common case, so every ordinary commit died at exit 1 with no verdict. Guarded, and only caught because the probe runs the real hook rather than the scan. Five new rows, all five proven red against the pre-fix hook, each carrying a non-vacuity assertion that the canonical single-`-m` heredoc still resolves. Changeset corrected on three counts: "all fail-closed" was wrong (persistent commit.cleanup fails OPEN, as do the -C/-c/-F/-t message sources), "global options are all walked through" was too broad, and the chained-before claim now states what is measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FUAVz49BghqxoJgwt7EW9 * fix(#3802): stop the separator and glue classes matching a literal backslash Round 8's Major, with two corrections to its account. `;`, `&` and `|` are metacharacters inside `[[ ]]`, so an inline bracket class must escape each one. POSIX bracket expressions have no escape mechanism of their own, so on bash 3.2 -- the system /bin/bash on macOS, already a supported target here per the `declare -A` ban in tests/install.test.cjs -- those backslashes reach the regex engine and add a literal `\` to the class. bash 4+ consumes them, which is why this is invisible on a modern bash. The hazard is specific to bracket expressions: `\(` outside one is made literal correctly on every version, and the subject validator and the `-m` capture classes were checked and are unaffected. The prescribed fix is not taken, because it does not parse. Inline `[;&|]` is a bash SYNTAX ERROR on 3.2 and on 5.3 alike -- the backslashes exist to get the metacharacters past the `[[ ]]` parser, so removing them leaves an unparseable script. Each class is held in a variable and expanded unquoted on the right of `=~` instead, which is a plain regex on both versions. One root cause, consequences in BOTH directions. The reported half is the separator scan over-blocking. The half not reported is the accept direction, and it is the more serious: the glue class is NEGATED, so on bash 3.2 a backslash-glued suffix fell inside the exclusion and the hook RESOLVED a heredoc it should have declined -- measured exit 0 on 3.2 against the unfixed hook, exit 2 everywhere else, with a letter-glued control refused in all four cells. The reported repro is not actually fixed by this, and the changeset says so. A `\`-newline line continuation carries a literal newline, which the round-7 separator guard refuses on every bash, so that shape stays blocked with or without this change. Narrowing the newline guard is not attempted: telling a continuation from a separator by substring scan is the class that was tried twice in earlier rounds and reverted both times, and an escaped backslash sitting immediately before a real newline is indistinguishable from a continuation. Disclosed as a known fail-closed limit instead. Every new row runs under each bash on the machine. Against the unfixed hook both bash 3.2 rows go red while all four bash 5.3 rows stay green -- written the ordinary way these rows would run under PATH bash, pass against the broken hook, and prove nothing. Two non-vacuity controls per interpreter prove the validator is reached rather than passing everything. All 8 rows of the established differential harness are byte-identical before and after on both versions: no regression, no new refusal. * fix(#3802): remove the $ of a dollar-quote from the option-name scans Independent round-8 review, accept direction. The option-name windows are dequoted so they match "the command as bash hands it to git" -- round 6 removed quote characters, round 7 removed syntactic backslashes. Both passes missed that bash has two further quoting forms whose introducer is a `$`: `$'...'` and `$"..."`. Removing the quote characters alone left that `$` stranded INSIDE the option name, so `-$"m"` dequoted to `-$m` and matched no literal, while bash passed a real `-m` to git. Measured on bash 3.2.57 and 5.3.15 against a real repository: the hook allowed git commit --allow-empty -$"m" WIP -m "$(cat <<'EOF' fix: a perfectly ordinary conforming subject EOF )" with exit 0, and `git cat-file -p HEAD` recorded the subject `WIP`. The comparison that establishes this is HEAD-internal, not a differential: the same command spelled `-m WIP` is refused (exit 2). The merge-base refuses EVERY heredoc form, including a perfectly conforming one, so its exit 2 on this input says nothing about whether any guard fired -- it is the absence of the feature, not a working check. The same miss covered `$'m'`, spliced `--message`, `--cleanup`, `--squash` and `--fixup`. An option NAME finished by a command substitution -- `--clean$(printf up)=verbatim` -- is a different problem and gets its own guard: bash runs a program to complete the name, so the argv git receives is not derivable from this string at all, and resolution is refused rather than guessed. The guard is scoped to the NAME: the class is a `-`-leading token whose characters up to the substitution contain no `=`. A substitution supplying a VALUE -- the ordinary `--author="$(git config user.name)"`, spaced or glued, in either window -- is untouched and still resolves, pinned in both directions. It is a SHAPE, not a segmentation of the command line; segmenting was tried twice in earlier rounds and reverted both times, and that reasoning stands. Both new rows fail against the unfixed tree with their own assertions, proven in a complete worktree at the previous head rather than a hook copied out of its tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy * fix(#3802): recognise a canonical cat, not any absolute path ending in /cat Independent round-8 review, accept direction. Round 4 restricted heredoc-opener recognition to an absolute path, after a relative `./cat` was measured being trusted to echo its stdin. It stopped at "absolute", so any absolute path ENDING in `/cat` was still trusted -- the same claim the round-4 reasoning had rejected one spelling earlier. Measured on bash 3.2.57 and 5.3.15 against a real commit: with an executable at `/.../fake-cat/cat` printing `WIP injected`, the hook validated the conforming heredoc body and allowed the commit (exit 0) while `git cat-file -p HEAD` recorded the subject `WIP injected`. The same command through `./cat` was already refused, which is the control that shows this is the round-4 class one spelling out rather than a new one. Recognition is now the canonical system locations -- bare `cat`, `/bin/cat`, `/usr/bin/cat` -- which is the only identity claim a string can support. `/usr/local/bin` is deliberately excluded: it is user-writable on ordinary machines, which is the plantable case this guard exists for. Anything else falls back to the opener line and the format gate: fail closed, exactly the pre-fix behaviour for the form. The pre-existing residual is unchanged and still documented: a bare `cat` shadowed earlier on PATH is indistinguishable here, and is not a meaningful boundary -- anyone able to plant an executable on PATH can run `git commit` directly. This hook stays an authoring guard, not a security control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy * fix(#3802): an option name carrying a shell expansion is unresolvable Independent review, round 9, accept direction. Four more spellings, and a change of strategy that is the actual point of this commit. Rounds 6, 7 and 8 each tried to EMULATE what bash does to an argument before git sees it -- round 6 removed quote characters, round 7 syntactic backslashes, round 8 the `$` that introduces a dollar-quote -- and each round review found another transform that had been missed. Round 9 found four more. All measured on bash 3.2.57 and 5.3.15 against a real repository, each with the plain spelling of the same command as its control (refused, exit 2) and `git cat-file -p HEAD` for the subject git actually recorded: -$'\155' WIP hook 0, real subject `WIP` ANSI-C octal -> m -$'\x6d' WIP hook 0, real subject `WIP` ANSI-C hex -> m -`printf m` WIP hook 0, real subject `WIP` backtick substitution x= … -${x}m WIP hook 0, real subject `WIP` parameter expansion -? WIP hook 0, real subject `WIP` pathname expansion and the same class through the cleanup guard, where git recorded a 75-character subject the length gate had measured as 72: --cle$'\141'nup=verbatim, --clean`printf up`=verbatim, --cle?nup=verbatim The last two settle it. An option name finished by a PARAMETER expansion depends on a variable's value at run time; one finished by a PATHNAME expansion depends on the contents of the working directory. Neither is derivable from the command string at any level of effort, so emulation cannot be completed -- not "has not been completed yet". A fifth patch in that direction would have the same shape as the previous four. The rule is therefore no longer "normalise it and match the literal". It is: an option NAME carrying a shell expansion or quoting construct is UNRESOLVABLE, and unresolvable refuses. One rule covers every spelling above and every spelling nobody has thought of yet, in the fail-closed direction. The dequoting passes are kept rather than replaced: they still normalise the deterministic removals, so the guards RECOGNISE `--clean""up=` and `-\m` as the options they are instead of merely refusing them, which keeps the existing rows meaningful. Scope is unchanged and still pinned in both directions: the class is a `-`-leading token whose characters up to the construct contain no `=`, so a construct supplying a VALUE -- `--author="$(git config user.name)"`, the backtick spelling, `--date="${NOW}"`, a glob character inside an author string, a pathspec after `--` -- still resolves. Nine such forms are asserted to pass beside the seven that must refuse. The class is bracket-only and holds no backslash, per round 8: a POSIX bracket expression has no escape mechanism, and a backslash written inside one becomes a literal member on bash 3.2. The new rows fail against the previous head with their own assertion message, in a complete worktree with the lib built, not a copied hook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy * docs(#3802): disclose and pin the two spellings the round-9 class over-blocks A scoped review of the round-9 class asked one question -- does it refuse a conforming heredoc commit that the previous head accepted -- and found two spellings that it does. Both measured on bash 3.2.57 and 5.3.15, previous head 518d97b64 exit 0, current head exit 2: git commit -S$SIGNING_KEY -m <conforming heredoc> git commit -m <conforming heredoc> -- -*.txt Disclosed and pinned rather than narrowed, for two reasons. Narrowing is not available cheaply. Dropping the bare `$` member reopens `-$xm`: with `xm=m` bash hands git a real `-m`, which is the parameter expansion bypass the round-9 commit exists to close. Skipping tokens after `--` means deciding where git's options end from a substring scan, which is the class this file has already reverted twice for opening accept-direction holes -- a `--` inside a quoted value (`--author "a -- b"`) would truncate the window and hide a real trailing directive. And the limits are narrower than they look, because in both cases the spelling a developer actually reaches for still resolves: -S "$KEY" and --gpg-sign="$KEY" resolve '-*.txt', "-*.txt", ':(exclude)-*.txt' resolve The pathspec one is worth stating precisely: a glob only reaches git AS a pathspec when it is quoted, because an unquoted one is expanded by the shell before git is executed. So the refused spelling is not passing a glob to git at all, and the spellings that do are unaffected. Refusing a commit git would take is the recoverable direction; accepting a non-conforming subject is not. That is the trade this file already makes everywhere else, and it is made explicitly here. Nine rows pin the working spellings beside the three that refuse, so a later narrowing cannot silently drop the cases that must keep working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy * fix(#3802): join backslash-newline continuations before the resolve guards Round 9's Major, with a correction to its diagnosis. The cited bracket classes at :223 and :260 no longer exist -- round 8 moved both into SEP_CLASS and GLUE_CLASS, and a lone backslash before -m resolves (exit 0) at the reviewed head on both bash 3.2.57 and 5.3.15. What refuses the repro is the NEWLINE a `\`-continuation carries: round 7's separator guard reads any newline in a window as a command boundary, and `git commit \` newline ` -m "$(cat <<'EOF' …` was refused for that reason. Round 8 disclosed it as a fail-closed limit; round 9 calls the idiom common and the limit a Major, and it is fixed here. It was left as a limit because "is this newline a continuation" looked like the segmentation question this file has reverted twice. It is not: bash's rule is local and character-level. A newline preceded by an ODD run of backslashes is a continuation and bash removes both; an EVEN run (`\\` then newline) is a literal backslash followed by a real newline, which IS a separator. Both scan windows are joined that way immediately after they are cut from the command and before any dequote copy is derived, in three bash-3.2-safe parameter expansions: every `\\` pair is parked on \x01, any backslash-newline that remains is a lone one and is removed, then the pairs are restored. Measured on both bashes, both directions: git commit \<nl> -m <heredoc> 2 -> 0 the fix git commit \\<nl> -m <heredoc> 2 -> 2 literal \ + real separator git commit<nl> -m <heredoc> 2 -> 2 bare newline -m <heredoc>\<nl>suffix 2 -> 2 bash glues it; the glue guard sees it glued git commit … \<nl> --allow-empty<nl>echo -m … 2 -> 2 the REAL newline still separates The prescribed `[\;&|]` is not taken: a backslash written inside a bracket expression becomes a literal member on bash 3.2, which is the round-8 defect from the other side. Rows run under each bash on the machine. The fix row fails against the previous head in a complete worktree with the lib built; the four control rows were measured against that same head and were already refused, so they pin existing behaviour rather than the change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
2ea5efc151 |
enhance(#3911): hooks declare their crash policy (#3960)
* enhance(#3911): give hooks an exit seam that needs no build ADR-3889 Phase 7 foundation. The 19 shipped enforcement hooks hold 91 of the epic's 128 terminators and cannot reach `terminateNow` today. The obvious route — requiring `gsd-core/bin/lib/cli-exit.cjs`, as gsd-agent-isolation-guard.js already does for two other modules — is rejected. That precedent carries its own warning (#3582): those files are tsc output, gitignored and absent on a raw plugin-marketplace or git-clone install, so the hook must first call ensureRuntimeBuild() to self-heal. Making the module a hook needs IN ORDER TO TERMINATE depend on a build inverts the dependency, and its failure mode is precisely the fail-open this phase exists to remove: a guard that cannot terminate cannot deny. `lint-hooks-runtime-build-seam` already encodes that concern, and Design B would have had to add an ensureRuntimeBuild() call to all 19 hooks to satisfy it. So `hooks/lib/` becomes a third emit location for cli-exit and a fifth for the registry, preserving the invariant `src/cli-exit.cts`'s own header states: it imports nothing but node:fs and its sibling registry, and the generator dual-emits that sibling alongside each copy so a relative require resolves next to whichever copy loaded it. Shipping needed no change — build-hooks.js already declares HOOKS_SUBDIRS_TO_COPY = ['lib']. Proven, not asserted: the two files are copied into an otherwise-empty tmpdir and a child process requires them and terminates — PASS exits 0, HOOK_DENY exits 2 with the payload on both stdout and stderr. That test fails the moment the hooks copy gains a require reaching outside hooks/lib/. Also fixed inline: the registry's fifth target let any `--write` test overwrite the real committed hooks/lib/exit-code-registry.js, because the test helper derived only three of the other output paths. It now redirects all five, and a regression test asserts every committed artifact is byte-identical after a redirected write. Install-tree goldens pick up the two new shipped paths across 11 runtimes — insertions only, no removals. lint:ci was green while they were stale, so this was found by regenerating rather than by a gate. Verification runs on the remote runner. Refs #3911 * enhance(#3911): declare a crash policy, and migrate the write guard Adds `hooks/lib/hook-exit.js` — the hook-facing vocabulary over `terminateNow`, hand-written because the cli-exit copy beside it is generated: allow(payload) exit 0 deny(payload, stderr?) exit 2 crash(onCrash, payload) whichever the hook DECLARED `crash()` takes the policy as a required argument with no default, which is the whole mechanism: fail-open by accident stops being expressible. A hook must name ALLOW or DENY at the call site, and an unrecognized value terminates INTERNAL rather than guessing. Fail-open stays legal; fail-open by omission does not. `gsd-write-guard.js` is the first hook migrated, all 12 sites, and it exposed a gap in the seam. `terminateNow`'s doc comment justified its fd-2 write by citing this hook's `emitBlock` — but modeled it as sending the same bytes to both streams, when `emitBlock` actually sends full JSON to stdout and only the bare `reason` string to stderr, because Kimi's hook bus feeds stderr verbatim back to the model. Migrating as written would have turned a readable sentence into a JSON blob for Kimi-backed agents. #3911 requires both "all 19 hooks terminate through terminateNow" and "no hook's effective default changes". Those are jointly satisfiable only by teaching the seam to carry a distinct stderr payload, so `terminateNow` gains an optional third argument: omitted, behavior is byte-for-byte what it was; a string is written raw, which is exactly the Kimi case. The doc comment's inaccurate claim about emitBlock is corrected in place. Proven rather than asserted: the pre-migration file is reconstructed from HEAD and driven with the same catastrophic-shrink payload as the migrated one — exit code, stdout and stderr all byte-identical. Verification runs on the remote runner. Refs #3911 * enhance(#3911): all 19 hooks terminate through the seam Migrates the remaining 18 enforcement hooks onto allow/deny/crash. An AST walk now reports zero `process.exit(` call sites across every `hooks/*.js` — down from the 91 the census measured. Each hook with an outer catch declares its policy once, at module top, with the reason that policy is right for that specific guard: a read guard that cannot scan must not block the read; a statusline that renders every prompt must degrade rather than crash; an injection scanner must not retroactively block a result already returned. Those sentences are the deliverable — they are what turns fail-open-by-accident into fail-open-on-purpose. No hook's effective default changed. Wiring exposed two defects, both fixed here rather than noted. A SECOND stdout/stderr-splitting site turned up in `gsd-workflow-guard.js`'s `emitForceAddBlock`, matching the pattern already known from the write guard — full JSON to stdout, bare reason to stderr for the Kimi bus. It uses the `stderrPayload` argument added in the previous commit, which is now carrying its second real caller rather than one special case. More seriously, `terminateNow` emitted both streams inside ONE try, so a payload that failed to serialize aborted before the stderr write ever ran. The two windsurf guards write nothing to stdout on a block and only a reason string to stderr, so `deny(undefined, reason)` exited 2 with EMPTY stderr — a deny that silently loses its reason, which is the exact "fails with success" class this epic exists to close. The streams are now emitted independently, each with its own guard, and `undefined` means "nothing to write for this stream" rather than an error. Regression tests inject a throwing write on one fd and assert the other still receives its payload; they fail against the single-try version. Byte-identity was proven per hook, not assumed: each pre-change file is reconstructed from HEAD and driven side by side with the migrated one across its normal path, its deny path, malformed stdin and empty stdin — exit code, stdout and stderr compared. Verification runs on the remote runner. Refs #3911 * enhance(#3911): harden the three shell hooks, and pin every hook's policy `gsd-phase-boundary.sh`, `gsd-session-state.sh` and `gsd-validate-commit.sh` gain `set -euo pipefail`. The expected hazard did not materialize, and that is worth recording: every intentionally-non-zero command in all three is already the condition of an `if`/`elif`, which `set -e` never fires on, and none of them reads a possibly-unset variable or pipes through a grep that may legitimately match nothing. No `|| true` guards were needed. Each hook was still checked command-by-command before the flags went in rather than after. Twenty-one before/after cases across the three hooks — disabled and enabled, planning and non-planning, missing STATE.md, malformed JSON, the Kimi payload shape, quoted and unquoted `-m`, valid and over-long Conventional Commits — all match on exit code, stdout and stderr. The hardening is shown to actually fire, not merely added: with a stubbed `node` that fails at the JSON-emit step, phase-boundary and session-state go from silently exiting 0 with empty stdout to failing visibly with the error surfaced. No such case could be constructed for `gsd-validate-commit.sh`, whose every statement already sits inside an if-condition — recorded as unproven rather than claimed. `tests/hooks-crash-policy.test.cjs` adds the per-hook coverage the issue asks for, table-driven over all 19 hooks rather than 76 hand-written cases: normal allow, deny where a deny path exists, crash-honors-the-declared-policy, and an unclosed-stdin case — the one `process.exitCode` structurally cannot serve. The deny assertions encode each hook's ACTUAL stream split rather than a uniform shape, since four of the six deliberately differ. A drift guard enumerates `hooks/*.js` and fails if a terminating hook is ever added without a row. Writing those tests surfaced two hooks that emit a block decision in their JSON body and exit 0. Both were checked rather than assumed, and neither is a fails-with-success: `gsd-read-injection-scanner.js` is PostToolUse, where the tool has already run and exit 2 has no meaning, and `gsd-cursor-subagent-start.js` follows Cursor's JSON-body protocol. They are deliberately left alone — a mechanical sweep to `deny()` would have broken exactly these two. Verification runs on the remote runner. Refs #3911 * fix(#3838): the commit validator says when it could not validate #3911 claims to subsume #3838. Measurement said otherwise, so this closes it for real rather than by assertion. `set -euo pipefail`, added earlier on this branch, does NOT fix #3838: bash exempts a command used as an `if` condition from `set -e`, and all three of the hook's swallow-and-pass sites are exactly that shape. Verified against the hardened hook with a node shim that fails only the classifier call — a non-conforming commit still exited 0 with empty stdout AND empty stderr, indistinguishable from "your commit conforms". That is the defect verbatim. All three sites named in #3838 now capture the real exit status instead of consuming it as a condition, and each distinguishes its genuine negative from "could not run": - the classifier: 0 = is a git commit, 1 = genuinely not one, anything else = could not classify. Its `node -e` now wraps the require and the call in try/catch and exits 3 on a throw, so a broken require chain can never be mistaken for `isGitSubcommand` legitimately returning false — which is the arm that matters, since `token-scanner.cjs` is a gitignored build artifact and a fresh checkout lands there. - the opt-in config read and the JSON command extraction get the same treatment. On "could not run" the hook emits a diagnostic to stderr naming which check failed and why, then exits 0. The issue confirms this is safe — it is a PreToolUse hook, so stderr does not disturb the JSON protocol — and ranks it the smallest sufficient fix. The gate still fails open, but it can no longer do so silently, which is the whole complaint: a validator that disables itself quietly costs more than one that is absent, because it is trusted. Both controls are unchanged and pinned by tests: a conforming commit still passes silently, a non-conforming one still exits 2 with its existing block payload. The defect test asserts stderr is non-empty and names the failure; it fails against the pre-fix hook. Verification runs on the remote runner. Refs #3911, #3838 * docs(#3911): document the hook crash-policy contract Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it), INVENTORY rows for the three new hooks/lib files, and an ARCHITECTURE note on the hooks section. How-To: docs/how-to/declare-a-hook-crash-policy.md, indexed from docs/README.md — a hook author now has to choose and declare a crash policy, which is more than one step and crosses into which harness protocol their hook speaks. It covers allow/deny/crash, writing an ON_CRASH reason that is actually useful, when a deny needs a distinct stderr payload, the two hooks whose harness reads a JSON-body decision and must NOT use deny(), and what to do when a check cannot run at all — with #3838 as the worked example. Refs #3911 * test(#3911): prove the seam actually ships, and stop hand-rolling temp cleanup Two review findings. The acceptance criterion 'hooks/dist/** stays in parity via the build seam (lint:hooks-runtime-build-seam)' was misstated and unmet: that lint checks something else — that a hook requiring a compiled gsd-core/bin/lib module also calls ensureRuntimeBuild(). Nothing exercised that the three new hooks/lib files reach hooks/dist/lib at all. That gap is not theoretical: #770 is a recorded ship-blocking bug where a new hook never shipped because a copy list missed it. The suite now builds dist through the repo's own ensureBuiltHooks(), byte-compares each shipped copy against its source, and spawns a child that requires the SHIPPED dist copy and denies — which is what catches a copy that exists but cannot resolve its sibling registry. gsd-validate-commit.sh hand-duplicated mktemp/run/rm three times; one idempotent trap on EXIT replaces them, guarded so cleanup cannot alter the exit status. Behavior-neutral across five cases, with temp-file counts taken before and after each run. Refs #3911 * fix(#3911): stage transitive hook lib requires, not just one level The remote run returned 7 failures across 3 real causes. The important one is a PRODUCTION bug this phase exposed rather than caused. `writeCursorHooksJson` scanned each hook script for `./lib/X` requires exactly one level deep and never re-scanned the lib files it staged for their own sibling requires. Nothing had a transitive lib dependency before, so the gap was invisible. Adding hook-exit.js -> cli-exit.js -> exit-code-registry.js made real Cursor installs ship a bundle that dies at require time with MODULE_NOT_FOUND. It now walks to a fixed point, and a real installed Cursor hook runs to completion. The staging harness in shared-hooks-dir-resolution hand-copied its fixture, so the injection scanner crashed at require time and its exit-1 was being read as a policy decision. Migrated to copyScriptWithDeps, which walks the require graph — the repo's recorded rule for this class, since adding another copyFileSync keeps it alive for the next person. The missing-lib-source test in cursor-hook-workspace-roots hardcoded which lib file it expected to be named in the abort message; the same throw now fires for a different file first. Its assertion is unchanged in substance — staging still must abort rather than ship a broken hook — only the name is no longer pinned. The last one was my own test asserting an uppercase reason code. Measured against origin/next: the pre-change hook emits the same lowercase 'config_unreadable', so the test was wrong, not the migration. Corrected to the real value rather than making the code match the test. Verification runs on the remote runner. Refs #3911 * chore(#3911): regenerate the cursor install-tree golden The staging fix means a Cursor install now correctly carries the two transitive lib files it was silently missing. Additive only — no path was removed. The golden diff is the evidence the packaging defect was real. Refs #3911 * chore(#3911): backfill the changeset PR number Refs #3911 * fix(#3911): a git probe that timed out is not a negative A macOS CI lane failed three deny cases at 2084ms, 2112ms and 2177ms — just past the 2000ms budget these hooks give their git probes. The three that passed took 72ms, 595ms and 651ms. Under shard contention `git rev-parse` overruns, the hook reads the non-zero result as "not a git repo", and allows with exit 0 and empty stdout AND empty stderr. Under load, the guards silently stop guarding. That is ADR-3889's thesis exactly, sitting inside the security hooks this phase is about. The repo had already recognized the class in one place — gsd-cursor-subagent-start.js fail-closed-denies on `git_timed_out` (#3045) — but nowhere else. `hooks/lib/git-probe.js` classifies a probe's outcome, distinguishing a real non-zero exit from ETIMEDOUT, a signal kill, and a spawn failure, rather than folding all four into `status !== 0`. Three guards route their eight git probes through it. The resolution is the same shape #3838 took, and the same one that issue endorsed as smallest-sufficient: fail open, but loudly. **No exit code changes on any path** — a developer on a loaded machine is still not blocked, which keeps #3911's declaration-pass contract intact for exit codes. What changes is that the hook now says on stderr which probe could not answer, instead of presenting silence as a clean verdict. Scope was checked across every hooks/*.js, not just the three that failed: gsd-agent-isolation-guard spawns no git; gsd-statusline's two probes gate only a cosmetic display segment, not an allow/deny decision, and are left alone. The C2 deny assertion was a real-race test — it demanded exit 2 while a slow git legitimately yields 0. It now requires the hook to either deny, or allow with a diagnostic naming the probe that could not run; a silent allow still fails, so the assertion is not vacuous. A deterministic regression stubs git on PATH to sleep past the budget rather than waiting for load to reproduce it. Verification runs on the remote runner. Refs #3911 * test(#3911): a PATH shim cannot intercept the hooks' git spawn on Windows The deterministic timeout regression stubbed git on PATH and asserted the guard reports rather than silently allows. It passes on Linux and macOS and failed on Windows in 83ms and 176ms — the stub was never invoked at all. Mechanism: the hooks call spawnSync('git', args) with no shell:true, so on Windows CreateProcess resolves git.exe only and never a PATH .cmd shim. The git.cmd branch could not have worked and is removed rather than left implying a Windows path that does. Adding shell:true to the hooks to serve a test would change product behavior and widen an injection surface, so the case is skipped on win32 only, with the mechanism written into the skip reason so a future reader does not 'fix' it that way. Linux and macOS keep the coverage, and macOS is where the underlying fail-open was actually caught. Refs #3911 --------- Co-authored-by: sim <sim@local> |
||
|
Otavio Salvador
|
8ca86b5e24 |
fix: use #!/usr/bin/env bash in community .sh hooks for distro portability
The three opt-in bash hooks (gsd-phase-boundary.sh, gsd-session-state.sh,
gsd-validate-commit.sh) shipped with #!/bin/bash, which fails on distros
that don't ship bash at /bin/bash (NixOS, minimal Alpine images, some
container runtimes). POSIX guarantees /bin/sh but not /bin/bash.
This is latent in the default install path because Claude Code wires the
hooks as `bash <path>` from settings.json (PATH-resolved — the script's
own shebang is read as a comment by bash). The fix matters when scripts
are run directly: tests, future installer changes, or manual debugging.
Changes:
- hooks/gsd-{phase-boundary,session-state,validate-commit}.sh: shebang
switched to #!/usr/bin/env bash, matching the convention already used
in scripts/*.sh.
- tests/bug-2136-sh-hook-version.test.cjs: assertion updated to expect
the new shebang; comment updated to spell out the rationale.
- tests/bug-2979-hook-absolute-node.test.cjs: doc-comment updated — the
prior wording cited "POSIX std PATH always has /bin" as the reason
bare `bash` is OK. The actual reason is that bare `bash` is
PATH-resolved, which is portable across distros that don't ship
/bin/bash. POSIX std PATH guarantees /bin/sh, not /bin/bash.
- bin/install.js::buildHookCommand: comment block clarifying the same.
No behavior change in this file — bare `bash` was already correct.
- .changeset/portable-bash-shebang-hooks.md: changeset entry.
Verified locally on NixOS:
- npm run build:hooks: hooks/dist/*.sh shebangs propagate correctly.
- node --test tests/bug-2136-*.cjs tests/bug-2979-*.cjs
tests/bug-1817-*.cjs tests/bug-1834-*.cjs tests/bug-1906-*.cjs
tests/bug-2557-*.cjs tests/bug-3017-*.cjs tests/security-scan.test.cjs
tests/hooks-doc-parity.test.cjs: 126/126 pass.
- node scripts/run-tests.cjs (full suite): 6944 pass / 0 fail / 5 skip.
|
||
|
Tom Boucher
|
7827e1ddee |
fix(#3129): replace bypassed bash regex with token-walk git-cmd.js classifier (#3141)
* fix(#3129): replace bypassed bash regex with token-walk git-cmd.js classifier Root cause: gsd-validate-commit.sh used: if [[ "$CMD" =~ ^git[[:space:]]+commit ]] This regex silently bypasses Conventional Commits enforcement for: git -C /path commit -m ... (working-directory prefix) GIT_AUTHOR_NAME=x git commit (env-var prefix) /usr/bin/git commit -m ... (full-path executable) Fix: introduces hooks/lib/git-cmd.js with isGitSubcommand(cmd, sub) — a token-walk classifier that handles all four forms by: 1. Skipping leading VAR=VALUE env assignments 2. Validating the git executable (basename check for full-path support) 3. Consuming git global options (-C <path>, --git-dir=, -p, etc.) 4. Checking the subcommand token The hook delegates to this classifier via node shell-out. node is already called twice in this hook (config check + JSON parse), so no new runtime dependency. This becomes the single source of truth for all hooks that gate on git subcommands (pre-commit-review-gate, post-push-verify, etc.). Regression test: 27 assertions — tokenize correctness, 12 must-match cases (including all 3 bypass forms), 8 must-not-match cases, 3 source checks. All are real behavioral tests, not string comparisons. Suite: 7035/7035. Closes #3129. * fix(lint+hook+changeset): allow-test-rule, fix HOOK_DIR quote injection, fix changeset pr+typo |
||
|
Tom Boucher
|
f55069ecbf |
test(#2974): migrate 8 test files to typed-IR assertions (#3016)
* test(#2974): migrate 8 test files to typed-IR assertions Replaces raw stdout/stderr substring matching with structured-field assertions per CONTRIBUTING.md "Prohibited: Raw Text Matching on Test Outputs". Adds shared infrastructure for typed error emission so this pattern is the easy path going forward. Shared infrastructure: - core.cjs: ERROR_REASON frozen enum + setJsonErrorMode/getJsonErrorMode - gsd-tools.cjs: --json-errors CLI flag, parsed before subcommand dispatch - config.cjs: typed reasons at all 7 error sites - graphify.cjs: GRAPHIFY_REASON enum + reason/timeout_ms in execGraphify result - bin/install.js: pure buildSdkFailFastReport() IR builder + renderer - hooks/gsd-session-state.sh, gsd-phase-boundary.sh: emit Claude Code hookSpecificOutput JSON envelope with typed state_present/config_mode/ planning_modified/file_path fields (no-op when hooks.community is off) Test migrations (all pass, 171 tests across the 8 files): - bug-2649-sdk-fail-fast: assert on ir.reason / ir.context / ir.fix_command - bug-2687-config-read-warning-parity: assert.equal stderr === '' - bug-2796-arg-parsing-regression: assert on result.json.updated/.phase - bug-2838-summary-rescue: parse rescue footer, assert mtime invariant - bug-2943-config-get-context-window: parse JSON, assert ERROR_REASON.CONFIG_KEY_NOT_FOUND - graphify: assert reason === GRAPHIFY_REASON.ENOENT/TIMEOUT - hooks-opt-in: parse hookSpecificOutput, assert typed fields - security-scan: reclassified as source-text-is-the-product (scan label output and CI workflow YAML ARE the deployed contract) Verification: lint-no-source-grep clean (0 violations), full suite 6741/6741 pass. Closes #2974 * test(#2974): address CR feedback — typed code field, robust idempotency Two CodeRabbit findings on #3016 addressed: 1. tests/hooks-opt-in.test.cjs:355 (Minor, inline) — parsed.reason.includes('Conventional Commits') was still substring matching after the typed-IR migration. Fixed at the source: the gsd-validate-commit hook now emits a typed `code` field ('CONVENTIONAL_COMMITS_VIOLATION', 'COMMIT_SUBJECT_TOO_LONG') alongside the human-readable `reason`. Test asserts strictEqual on the code; the prose copy is no longer part of the test contract. 2. tests/bug-2838-summary-rescue-gitignored-planning.test.cjs:224-250 (Outside-diff) — mtimeMs alone can stay unchanged on coarse-grained filesystems (HFS+, FAT) when two rewrites land within the same timestamp tick, falsely passing the idempotency assertion. Replaced with a full snapshot (mtimeMs, ctimeMs, size, ino, sha256 of contents) compared via assert.deepStrictEqual — the hash catches any rewrite the timestamp would miss. Verification: 30/30 pass on the two affected files; lint-no-source-grep clean (0 violations across 368 test files). |
||
|
Tom Boucher
|
50f61bfd9a |
fix(hooks): complete stale-hooks false-positive fix — stamp .sh version headers + fix detector regex (#2224)
* fix(hooks): stamp gsd-hook-version in .sh hooks and fix stale detection regex (#2136, #2206) Three-part fix for the persistent "⚠ stale hooks — run /gsd-update" false positive that appeared on every session after a fresh install. Root cause: the stale-hook detector (gsd-check-update.js) could only match the JS comment syntax // in its version regex — never the bash # syntax used in .sh hooks. And the bash hooks had no version header at all, so they always landed in the "unknown / stale" branch regardless. Neither partial fix (PR #2207 regex only, PR #2215 install stamping only) was sufficient alone: - Regex fix without install stamping: hooks install with literal "{{GSD_VERSION}}", the {{-guard silently skips them, bash hook staleness permanently undetectable after future updates. - Install stamping without regex fix: hooks are stamped correctly with "# gsd-hook-version: 1.36.0" but the detector's // regex can't read it; still falls to the unknown/stale branch on every session. Fix: 1. Add "# gsd-hook-version: {{GSD_VERSION}}" header to gsd-phase-boundary.sh, gsd-session-state.sh, gsd-validate-commit.sh 2. Extend install.js (both bundled and Codex paths) to substitute {{GSD_VERSION}} in .sh files at install time (same as .js hooks) 3. Extend gsd-check-update.js versionMatch regex to handle bash "#" comment syntax: /(?:\/\/|#) gsd-hook-version:\s*(.+)/ Tests: 11 new assertions across 5 describe blocks covering all three fix parts independently plus an E2E install+detect round-trip. 3885/3885 pass. Approach credit: PR #2207 (j2h4u / Maxim Brashenko) for the regex fix; PR #2215 (nitsan2dots) for the install.js substitution approach. Closes #2136, #2206, #2209, #2210, #2212 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(hooks): extract check-update worker to dedicated file, eliminating template-literal regex escaping Move stale-hook detection logic from inline `node -e '<template literal>'` subprocess to a standalone gsd-check-update-worker.js. Benefits: - Regex is plain JS with no double-escaping (root cause of the (?:\\/\\/|#) confusion) - Worker is independently testable and can be read directly by tests - Uses execFileSync (array args) to satisfy security hook that blocks execSync - MANAGED_HOOKS now includes gsd-check-update-worker.js itself Update tests to read worker file instead of main hook for regex/configDir assertions. All 3886 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tibsfox
|
4157c7f20a |
feat(hooks): add opt-in community hooks for GSD projects
Port 3 community hooks from gsd-skill-creator, gated behind hooks.community config flag. All hooks are registered on install but are no-ops unless the project config has hooks: { community: true }.
gsd-session-state.sh (SessionStart): outputs STATE.md head for orientation. gsd-validate-commit.sh (PreToolUse/Bash): blocks non-Conventional-Commits messages. gsd-phase-boundary.sh (PostToolUse/Write|Edit): warns when .planning/ files are modified.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|