diff --git a/.changeset/lucky-pandas-commit.md b/.changeset/lucky-pandas-commit.md new file mode 100644 index 000000000..e360390c7 --- /dev/null +++ b/.changeset/lucky-pandas-commit.md @@ -0,0 +1,82 @@ +--- +type: Fixed +pr: 3816 +--- +**The commit-message hook no longer blocks every heredoc-form commit** — with `hooks.community: true`, `gsd-validate-commit.sh` rejected `git commit -m "$(cat <<'EOF' … EOF)"` with `CONVENTIONAL_COMMITS_VIOLATION` whatever the message said, because its `-m` capture matches across newlines and the message's first line was the literal `$(cat <<'EOF'` rather than the subject. That opener is a standard agent-authored commit idiom, so enabling the toggle — which also carries the session-state and phase-boundary hooks — made that pattern fail every time. + +The subject is now resolved from the captured message before validation, for the canonical form: a single `-m "…"` holding one `$(cat …)` substitution, under git's default `cleanup=whitespace`. Resolution handles the delimiter spellings bash does not expand (`<<'EOF'` and `<<\EOF`, with or without `<<-`, spaced or space-free), the leading tabs `<<-` strips, CRLF line endings, and both directions of `cleanup=whitespace` — leading blank lines are skipped, and trailing whitespace on the subject is not counted against the 72-character limit. + +Everywhere the validated text could differ from the subject git actually receives, resolution is refused and the commit stays blocked exactly as it was before this change. That covers: a `-m '…'` single-quoted argument, in which bash performs no command substitution at all; a bare `< head=0: a + # net-new bypass reachable by the ordinary authoring slip of typing `'` + # for `"`. The sq arm therefore keeps the pre-fix `head -1`, which is exact + # base parity. + # + # ADJACENCY GUARD (review of #3816): text glued to the CLOSING quote — + # `-m "$(cat <<'EOF' ... )"suffix` — is concatenated by bash into the SAME + # argument, so the capture above holds only a PREFIX of the real message. + # Resolving a heredoc from a prefix hands the length gate a fraction of the + # real subject: a net-new bypass relative to base, which measured the + # opener line and blocked. When the quote is not followed by whitespace or + # the end of the command, skip the resolver and keep the pre-fix subject + # (first captured line): the heredoc form then fails the format gate + # exactly as it did on base, and the plain single-line form keeps base + # behavior unchanged. The guard is tested against the arm that MATCHED, + # not against both: testing both let a double-quoted heredoc whose BODY + # mentions a glued single-quoted token (`-m "... -m 'foo'bar ..."`) trip + # the sq arm and lose the fix for a message that never had a prefix + # problem (review of #3816, round 4, Minor 1). + # RESOLVER PRECONDITIONS. The resolver may run only where the captured text + # is provably the subject git receives. Each guard names an input where it + # is not; every refusal falls back to `head -1`, the pre-fix subject, which + # fails the format gate exactly as this whole form did before the fix. + RESOLVE=0 + if [ "$MSG_QUOTE" = dq ]; then + RESOLVE=1 + # Text before the message we matched. The heredoc BODY always sits after + # the match, so this window cannot be contaminated by message content — + # which is what lets the two guards below scan for tokens that would also + # be legal inside a commit message. + MSG_PREFIX="${CMD%%"$MSG_MATCH"*}" + # Text after it. Together, PREFIX and SUFFIX are the whole command MINUS + # the message — the window a guard must use when the token it scans for + # is also legal English inside a commit message, but may legally appear + # on EITHER side of the message on the command line. + MSG_SUFFIX="${CMD#*"$MSG_MATCH"}" + # LINE CONTINUATIONS ARE NOT SEPARATORS (review of #3816, rounds 8 and 9). + # `git commit \` newline ` -m "$(cat <<'EOF' …` is an ordinary way to + # spread an invocation over lines, and every guard below reads a newline in + # a window as a command separator, so the whole form was refused. That was + # disclosed as a fail-closed limit in round 8 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. So the windows are + # joined the way bash joins them, in three bash-3.2-safe steps: every `\\` + # pair is parked on \x01, a byte no real command line carries, any + # backslash-newline that remains is a lone (odd) one and is removed, then + # the pairs are restored. Applied to BOTH windows, BEFORE the dequote + # copies are derived, so every scan sees the joined text. + # + # KNOWN OVER-BLOCK, fail-closed: a literal \x01 that IS present in the + # command is restored as `\\`, so an option-shaped token carrying one + # (`-\x01m`) reads as `-\\m`, dequotes to `-m`, and refuses where it did + # not before (independent review, round 9). Refusing is the recoverable + # direction; a control byte in an option name is not a spelling anyone + # types, and it is not a hole in the accept direction. + # + # Direction check: a continuation glued to the closing quote + # (`"$(…)"\` newline `suffix`) joins to `"$(…)"suffix`, which the glue + # guard refuses exactly as bash would have glued it; `\\` + newline keeps + # its newline and is still refused by the separator guard. Measured on + # bash 3.2.57 and 5.3.15 in tests/hooks-opt-in.test.cjs. + CONT_PARK=$'\x01' + MSG_PREFIX="${MSG_PREFIX//\\\\/$CONT_PARK}" + MSG_PREFIX="${MSG_PREFIX//\\$'\n'/}" + MSG_PREFIX="${MSG_PREFIX//$CONT_PARK/\\\\}" + MSG_SUFFIX="${MSG_SUFFIX//\\\\/$CONT_PARK}" + MSG_SUFFIX="${MSG_SUFFIX//\\$'\n'/}" + MSG_SUFFIX="${MSG_SUFFIX//$CONT_PARK/\\\\}" + + # QUOTE-SPLICED SPELLINGS (independent review of #3816, round 6). Bash + # removes quotes before git ever sees an argument, so the same option has + # unboundedly many spellings on the command line: `--clean""up=verbatim` + # IS `--cleanup=verbatim` to git, and `-""m` IS `-m`. Both matched no + # literal and were measured ACCEPTING a 75-byte subject the length gate + # had recorded as 72. The guards below therefore scan a copy of their + # window with quote characters removed, which is what bash does to it. + # Only the two OPTION-NAME scans use it; the adjacency test deliberately + # does not, because it asks about a literal character position, and the + # message span itself is excluded from both windows either way. + MSG_PREFIX_DEQ="${MSG_PREFIX//[\"\']/}" + MSG_SUFFIX_DEQ="${MSG_SUFFIX//[\"\']/}" + # BACKSLASH-SPLICED SPELLINGS (independent review of #3816, round 7). + # Quote removal alone was not "the command as bash hands it to git": bash + # also removes syntactic backslashes, so `-\m WIP` IS `-m WIP` and + # `--clean\up=verbatim` IS `--cleanup=verbatim` to git, and both matched + # no literal. Measured: `-\m WIP -m ` accepted the + # heredoc while git recorded `WIP`, and a trailing `--clean\up=verbatim` + # accepted a 75-byte subject the length gate measured as 72. Stripped in a + # second pass so the class is unambiguous. + MSG_PREFIX_DEQ="${MSG_PREFIX_DEQ//\\/}" + MSG_SUFFIX_DEQ="${MSG_SUFFIX_DEQ//\\/}" + # DOLLAR-QUOTED SPELLINGS (independent review of #3816, round 8). The two + # passes above still were not "the command as bash hands it to git": bash + # has TWO more quoting forms whose introducer is a `$`, and removing the + # quote characters alone leaves that `$` stranded in the middle of the + # option name. `-$"m"` became `-$m` here while bash passes a real `-m` to + # git, and `--mes$'sage'=WIP` became `--mes$sage=WIP`; neither matched any + # literal, so the first-message guard below never fired. Measured on bash + # 3.2.57 and 5.3.15 against a real repository: the hook allowed + # `-$"m" WIP -m ` (exit 0) while `git cat-file -p` + # recorded the subject `WIP` — the same command spelled `-m WIP` is + # refused (exit 2). Stripping `$` closes both dollar-quote forms. + # + # RESIDUAL, and not fixable from a string: an option name assembled by an + # EXPANSION — `-${x}m`, `-$(printf m)` — is not knowable without running + # the command, the same limit this file already documents for expanded + # heredoc bodies. Stripping `$` makes those spellings collapse toward the + # literal too, which over-matches, and over-matching only refuses more. + MSG_PREFIX_DEQ="${MSG_PREFIX_DEQ//\$/}" + MSG_SUFFIX_DEQ="${MSG_SUFFIX_DEQ//\$/}" + + # ADJACENCY GUARD (review of #3816): text glued to the CLOSING quote — + # `-m "$(cat <<'EOF' ... )"suffix` — is concatenated by bash into the SAME + # argument, so the capture holds only a PREFIX of the real message, and + # the length gate would measure a fraction of the real subject. + # SCOPE (review of #3816, round 6 — MAJOR). Glue is a property of the ONE + # character following the MATCHED span, so that character is the whole + # window. Scanning $CMD for the shape anywhere refused any conforming + # commit whose command merely CONTAINED a glued `-m` elsewhere — + # `git commit -m "" && echo -m "test"z` stayed blocked with + # CONVENTIONAL_COMMITS_VIOLATION. Base blocks it too, because base blocks + # EVERY heredoc form (that is #3802): this was the fix not reaching the + # shape, measured base=2 -> pre=2 -> post=0, not a regression. + # The 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. + # The class is held in a VARIABLE, not written inline. Inline, every + # member needs a backslash to get past the `[[ ]]` parser (`;`, `&` and + # `|` are metacharacters there) — and on bash 3.2, the system /bin/bash on + # macOS, those backslashes are passed THROUGH to the regex engine instead + # of being consumed by the shell, silently adding a literal `\` to the + # class. Unquoted expansion of a variable on the right of `=~` is the one + # spelling that is a plain regex on 3.2 and 5.x alike (review of #3816, + # round 8). Writing `[^[:space:];&|()<>]` inline is NOT the fix: it is a + # bash syntax error on both versions. + GLUE_CLASS='^[^[:space:];&|()<>]' + if [[ "$MSG_SUFFIX" =~ $GLUE_CLASS ]]; then RESOLVE=0; fi + + # FIRST-MESSAGE GUARD (Codex review of #3816, round 4 — BLOCKER). The + # capture is a SEARCH over the whole command and the double-quoted arm is + # tried first, so it can select a `-m` that is not git's subject at all: + # + # git commit -m 'WIP first' -m "$(cat <<'EOF' -> git concatenates; the + # git commit -m WIP -m "$(cat <<'EOF' subject is `WIP first` + # git commit -m WIP -- -m "$(cat <<'EOF' -> after --, not a message + # git commit -m WIP && echo -m "$(cat <<'EOF' -> belongs to `echo` + # + # All four measured base=2 -> head=0, with git recording the FIRST message + # as the subject (verified against real commits, not the man page). The + # mis-selection is pre-existing; resolving it is what turned it into an + # enforcement bypass. Resolve only when nothing before the match could + # have been an earlier message, an end-of-options marker, or another + # command. + # BUNDLED SHORT OPTIONS (independent review of #3816, round 6). git splits + # `-am 'WIP first'` into `-a -m`, so the real subject is `WIP first` and + # the heredoc is git's SECOND message — measured accepting the heredoc's + # subject while git recorded `WIP first`. A standalone `-m` is therefore + # not the only spelling that claims the message; any short-option cluster + # ending in `m` does. + # ATTACHED VALUES AND --message ABBREVIATIONS (independent review of + # #3816, round 7). The scan required a space or `=` after the option name, + # so two spellings git accepts matched nothing: an ATTACHED short-option + # value (`-mWIP`, which git reads as `-m WIP`) and a long-option + # abbreviation (`--mes=WIP`), the same abbreviation behaviour this file + # already models for `--cleanup`. Both were measured accepting a later + # conforming heredoc while git recorded `WIP` as the subject — confirmed + # against the raw commit object, not `git log --pretty=%s`. The short arm + # therefore drops its trailing requirement entirely: a `-` followed by + # letters ending in `m` claims the message however it is spelled. Wider + # than git's own abbreviation set on purpose — over-matching only refuses + # more, which is the recoverable direction. + # Variable-held for the same bash-3.2 reason as GLUE_CLASS above. + # AN OPTION NAME BUILT BY A COMMAND SUBSTITUTION IS UNRESOLVABLE + # (independent review of #3816, round 8). Stripping `$` above collapses the + # two dollar-QUOTE forms onto their literals, but `--clean$(printf up)=` + # is a different thing: bash RUNS a program to finish the option name, so + # the argv git receives is not derivable from this string at all. Measured + # accepting a 75-byte subject the length gate had recorded as 72. + # + # SCOPED TO THE NAME, NOT THE VALUE. The class is a `-`-leading token whose + # characters up to the substitution contain no `=` — an option NAME being + # assembled. `--author="$(git config user.name)"` and `--author "$(…)"` + # both put the substitution in the VALUE, which this file never models and + # which stays allowed; only `-…$(` before any `=` refuses. Scanned on the + # RAW windows on purpose: the dequoted copies have had their `$` removed, + # so the shape is no longer visible there. + # + # This is a SHAPE, not a segmentation: it never tries to decide where + # git's own command ends. Two attempts at that were reverted for opening + # accept-direction holes, and the reasoning above still stands. + # WIDENED, and the strategy changed with it (independent review, round 9). + # The `$(`-only spelling above was the fourth patch in a row that tried to + # EMULATE what bash does to an argument before git sees it -- round 6 + # removed quotes, round 7 backslashes, round 8 the `$` of a dollar-quote, + # and each time review found another transform that had been missed. Round + # 9 found four more, all measured accepting `WIP` as the real subject on + # bash 3.2.57 and 5.3.15 while the plain spelling of the same command is + # refused: + # + # -$'\155' WIP ANSI-C octal escape decodes to `m` + # -$'\x6d' WIP ANSI-C hex escape decodes to `m` + # -`printf m` WIP command substitution, backtick spelling + # x= … -${x}m WIP parameter expansion + # -? WIP pathname expansion, with a file named `-m` + # + # The last two settle the strategy: an option name finished by a PARAMETER + # expansion depends on a variable's runtime value, and 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". + # + # So the rule is no longer "normalise it and match the literal". It is: an + # option NAME containing 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 above are kept: they still normalise the + # deterministic removals so the guards RECOGNISE `--clean""up=` and `-\m` + # rather than merely refusing them, which keeps the existing rows honest. + # + # SCOPED TO THE NAME, NOT THE VALUE, exactly as before: the class is a + # `-`-leading token whose characters up to the substitution contain no `=`. + # `--author="$(git config user.name)"` and `--author "$(…)"` put the + # construct in the VALUE and still resolve, pinned in both directions. + # Scanned on the RAW windows, because the dequoted copies have had `$` and + # the quote characters removed and the shape is no longer visible there. + # + # 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. + SUBST_NAME_CLASS='(^|[[:space:]])-[^[:space:]=]*[$`?*[]' + if [[ "$MSG_PREFIX" =~ $SUBST_NAME_CLASS ]] \ + || [[ "$MSG_SUFFIX" =~ $SUBST_NAME_CLASS ]]; then RESOLVE=0; fi + SEP_CLASS='[;&|]' + if [[ "$MSG_PREFIX_DEQ" =~ (^|[[:space:]])(-[a-zA-Z]*m|--m[a-z]*([=[:space:]]|$)) ]] \ + || [[ "$MSG_PREFIX" =~ (^|[[:space:]])--([[:space:]]|$) ]] \ + || [[ "$MSG_PREFIX" =~ $SEP_CLASS ]] \ + || [[ "$MSG_PREFIX" == *$'\n'* ]]; then RESOLVE=0; fi + # NEWLINE IS A COMMAND SEPARATOR TOO (independent review of #3816, round + # 7) — the test above. The separator scan covered `;`, `&` and `|` but not + # a literal newline, so a LATER command's heredoc-shaped `-m` was taken + # for this commit's message: + # + # git commit --amend --no-edit + # echo -m "$(cat <<'EOF' + # fix: conforming text unrelated to the commit + # EOF + # )" + # + # The classifier recognises the leading commit, the capture reaches across + # the newline into `echo`'s argument, and a conforming string with no + # relationship to the commit was validated and allowed. Tested as a glob + # rather than folded into the bracket class, because a literal newline + # inside a bash regex bracket expression is not portably expressible. + + # CLEANUP-MODE GUARD (Codex review of #3816, round 4 — BLOCKER). 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 as a 75-byte subject while the hook + # measured 72 — COMMIT_SUBJECT_TOO_LONG dodged (measured base=2 -> head=0; + # confirmed by reading the raw commit object, since `git log --pretty=%s` + # strips trailing whitespace in its own output and hides it). + # Any named mode other than `whitespace` refuses. A mode set persistently + # in git config is invisible here and stays a documented residual limit. + # SCOPE (review of #3816, round 5 — BLOCKER). This scan must exclude the + # message. `--cleanup=` and `commit.cleanup=` are ordinary English inside + # a commit message — this repository's own hooks and docs discuss them + # constantly — and the heredoc BODY sits verbatim inside $CMD, so + # scanning $CMD refused to resolve any conforming message that merely + # MENTIONED the token, blocking it with CONVENTIONAL_COMMITS_VIOLATION. + # Scanning $MSG_PREFIX alone (the fix as first prescribed) would reopen + # the bypass this guard exists for: git accepts the flag on either side + # of -m, and `git commit -m "" --cleanup=verbatim` is caught + # today only because the scan is command-wide. PREFIX + SUFFIX keeps both + # positions covered while excluding the one span that is message text. + # The two are joined with a space so a token cannot be forged across the + # seam out of a prefix tail and a suffix head. + # KNOWN LIMIT, deliberately fail-closed (#3816, round 6). This window is + # the whole command minus the message, so a `--cleanup=` that belongs to a + # DIFFERENT command — `git commit -m "" && echo --cleanup=verbatim` + # — also refuses, and a conforming commit git would accept stays blocked. + # Narrowing it to git's own segment was tried and reverted: deciding where + # git's command ends needs a shell parse, and a substring scan is not one. + # Trimming at the first `;&|` cut the window short whenever a separator sat + # inside an ordinary argument — `--author "a&b"`, and equally `--author + # a\&b` — which hid a REAL trailing `--cleanup=verbatim` and ACCEPTED a + # 75-byte subject the length gate had measured as 72. Two successive + # narrowings each reopened that hole on a shape the previous one missed, so + # the scan stays wide: refusing a commit git would take is recoverable, + # accepting an over-long subject is not. + # ABBREVIATIONS (independent review of #3816, round 6). git accepts any + # unambiguous prefix of a long option, so `--cle=verbatim` sets the mode + # while matching no literal `--cleanup` — measured accepting a 75-byte + # subject recorded as 72. The class is deliberately wider than git's own + # abbreviation set: over-matching only refuses more, which is the safe + # direction, and no other `--cl` option exists for git commit. + # LAST DIRECTIVE WINS, AND ONE MATCH CANNOT SEE IT (independent review of + # #3816, round 7). A bash regex yields ONE BASH_REMATCH, so only the + # FIRST cleanup directive was inspected — and git applies the LAST one. + # `--cleanup=whitespace -m --cleanup=verbatim` therefore read as + # mode=whitespace, resolution stayed enabled, and a 72-character subject + # plus trailing spaces was accepted while git recorded 75 bytes with the + # whitespace preserved (confirmed against the raw commit object). Deciding + # WHICH directive is last needs an argv order this substring scan does not + # have, so multiplicity itself refuses: more than one directive is + # unresolvable, not "probably fine". Single-directive behaviour is + # unchanged. + CLEANUP_WINDOW="$MSG_PREFIX_DEQ $MSG_SUFFIX_DEQ" + # `|| true` is load-bearing: this script runs under `set -euo pipefail`, + # and grep exits 1 when it matches NOTHING — which is the common case, a + # command with no cleanup directive at all. Without it the pipeline's + # non-zero status killed the hook outright (exit 1, no verdict) for every + # ordinary commit. Caught by running the real hook rather than the scan. + CLEANUP_HITS=$( { printf '%s' "$CLEANUP_WINDOW" | grep -oE '(--cl[a-z]*|commit\.cleanup)[=[:space:]]+[^[:space:]]+' || true; } | wc -l | tr -d ' ') + if [ "${CLEANUP_HITS:-0}" -gt 1 ]; then + RESOLVE=0 + elif [[ "$CLEANUP_WINDOW" =~ (--cl[a-z]*|commit\.cleanup)[=[:space:]]+([^[:space:]]+) ]]; then + if [ "${BASH_REMATCH[2]}" != "whitespace" ]; then RESOLVE=0; fi + fi + + # GIT-GENERATED SUBJECTS (independent review of #3816, round 7). With + # `--squash=` or `--fixup=` git composes the subject + # itself — measured recording `squash! base: something` while a conforming + # heredoc supplied via -m sailed through. The supplied message is not the + # subject in these modes at all, so there is nothing here worth measuring + # and resolution is refused outright. Abbreviations included for the same + # reason as --cleanup's. Deliberately NOT extended to the other + # message-SOURCE options (-C/--reuse-message, -c/--reedit-message, + # -F/--file, -t/--template): `-c` is also a git GLOBAL option that legally + # precedes the subcommand, so a scan for it would refuse ordinary + # `git -c k=v commit` invocations. Those remain a disclosed gap rather + # than a guessed guard. + if [[ "$MSG_PREFIX_DEQ $MSG_SUFFIX_DEQ" =~ (^|[[:space:]])--(squash|fixup|sq[a-z]*|fix[a-z]*)[=[:space:]] ]]; then RESOLVE=0; fi + fi + + if [ "$RESOLVE" = 1 ]; 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) + else + SUBJECT=$(echo "$MSG" | head -1) + fi # Validate Conventional Commits format if ! [[ "$SUBJECT" =~ ^(feat|fix|docs|style|refactor|perf|test|build|ci|chore)(\(.+\))?:[[:space:]].+ ]]; then # Emit a typed `code` field alongside `reason` (#2974). Tests assert diff --git a/hooks/lib/git-cmd.js b/hooks/lib/git-cmd.js index 2498bf8cf..b7154d9c4 100644 --- a/hooks/lib/git-cmd.js +++ b/hooks/lib/git-cmd.js @@ -180,4 +180,213 @@ function isGitSubcommand(cmd, sub) { return tokens[subIdx] === sub; } -module.exports = { isGitSubcommand, tokenize, extractBranchArgument, skipToSubcommand }; +/** + * Resolve a `-m` message argument to its SUBJECT — the first line of the commit + * message — resolving the command-substituted heredoc form to the heredoc + * BODY's first line. + * + * PURE STRING FUNCTION. It deliberately does NOT tokenize or walk the command + * line: selecting *which* argument is the message stays with the caller, exactly + * as before, so this cannot change which commands are validated. An earlier + * revision of this fix did walk tokens and regressed four separate cases — + * `git commit -- -m WIP` (a pathspec), `git commit --amend && echo -m WIP` (a + * later command's flag), `-m "" --allow-empty-message` (the scanner drops empty + * tokens, so the following flag became the subject), and unquoted + * `git commit -m WIP`. All were allowed upstream and would have started being + * blocked. Reported in review of #3802. + * + * The defect this DOES fix: `gsd-validate-commit.sh` captured the message with + * `-m[[:space:]]+"([^"]+)"`, and bash `[^"]` matches newlines, so the widely + * used agent-authored commit idiom + * + * git commit -m "$(cat <<'EOF' + * feat(auth): add login flow + * EOF + * )" + * + * captured the whole span up to the final quote at `)"`. Taking its first line + * yielded the literal `$(cat <<'EOF'`, which can never satisfy Conventional + * Commits, so every heredoc-form commit was blocked regardless of its message. + * + * Recognition is anchored at BOTH ends and requires a command substitution, so + * an ordinary message merely CONTAINING — or ending in — `</bin/cat <<'EOF'` was recognized here while bash reads + // `/bin/cat` as the executable NAME, so recognition claimed a + // substitution that does not run cat (Codex review of #3816, round 2). The + // same ASCII rule as the blank-line skip below, for the same reason. + // `cat[ \t]*<<`, not `+`: bash accepts `cat<<'EOF'` with no space (review of + // #3816, round 3), and recognizing it costs nothing — the token before `<<` + // is still literally `cat`. + // + // The path prefix must be ABSOLUTE (Codex review of #3816, round 4). The old + // `[\w./-]*\/` also accepted `./cat` and `../evil/cat`, so a relative + // executable that merely ENDS in `cat` was trusted to echo its stdin: with a + // planted `../evil/cat` printing `WIP injected`, the resolver validated the + // heredoc body while git's real subject was `WIP injected` (measured + // base=2 -> head=0 against a real commit). Requiring `/` up front costs + // nothing real — `/bin/cat` and a bare `cat` both still resolve. + // + // AN ABSOLUTE PATH IS NOT AN IDENTITY EITHER (independent review of #3816, + // round 8). Round 4 stopped at "must be absolute", so any absolute path + // ENDING in `/cat` was still trusted to echo its stdin — the very thing the + // round-4 reasoning rejected one spelling earlier. With an executable at + // `/some/scratch/dir/cat` printing `WIP injected`, the resolver validated + // the conforming heredoc body while git's real subject was `WIP injected` + // (measured on bash 3.2.57 and 5.3.15 against a real commit: hook exit 0, + // `git cat-file -p` subject `WIP injected`, while the same command through + // `./cat` was already refused). The prefix is now the canonical system + // locations, which is the only 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. + // + // RESIDUAL, not fixable from a string: a bare `cat` shadowed earlier on PATH + // has the same effect and is indistinguishable here. It is also not a + // meaningful boundary — anyone who can plant an executable on PATH can run + // `git commit` directly — so this hook stays an authoring guard, not a + // security control. + // + // The delimiter alternatives are split so the BARE spelling is its own group: + // `\\(...)` (backslash-quoted) and `(...)` (bare) were one `\\?(...)` branch, + // which conflated the only two spellings that differ in bash. See the + // expansion guard below. + const opener = /^\$\([ \t]*(?:\/(?:usr\/)?bin\/)?cat[ \t]*<<(-?)[ \t]*(?:'([^']+)'|"([^"]+)"|\\([^\s'"();|&<>\\]+)|([^\s'"();|&<>\\]+))[ \t]*$/ + .exec(lines[0]); + if (!opener) return lines[0]; + + // EXPANSION GUARD (review of #3816, round 4 — BLOCKER). Only `<<'D'`, `<<"D"` + // and `<<\D` suppress expansion. A BARE `< format gate dodged + // -m "$(cat < length gate dodged + // + // Both measured base=2 -> head=0 against the real hook. This is NOT the + // pre-existing plain-`-m` expansion limit an earlier revision claimed it was: + // on base the whole heredoc form was blocked, so no expansion inside a body + // ever reached an allow — this fix created the path, and closes it here. + // Same rule every other guard in this function follows: when the real subject + // cannot be known, fall back to the opener line, which fails the format gate. + if (opener[5]) return lines[0]; + + // `<<-` strips leading TABS from every body line, including the terminator. + const stripTabs = opener[1] === '-'; + const delimiter = opener[2] || opener[3] || opener[4]; + const body = lines.slice(1).map((l) => (stripTabs ? l.replace(/^\t+/, '') : l)); + + // TRUNCATION GUARD. The capture that produced this argument stops at the first + // `"`, so a message containing one arrives here missing its tail — and its + // terminator. Resolving anyway would hand the length gate a PREFIX of the real + // subject and let an over-long message through, an enforcement hole that did + // not exist before this fix (review of #3802). A body with no terminator is + // therefore not resolved at all: returning the opener line fails the format + // gate, which is exactly what this whole form did before the fix. The fix + // applies where the capture is complete and changes nothing where it is not. + const end = body.indexOf(delimiter); + + // POST-TERMINATOR GUARD (review of #3816, round 3 — BLOCKER). Everything + // after the terminator is still part of the real message once bash + // substitutes: `-m "$(cat <<'EOF'\nfeat: ok\nEOF\n) <200 a's>"` expands to a + // single 200+ char subject, but discarding the tail measured 8 and DODGED + // COMMIT_SUBJECT_TOO_LONG — the same prefix-measurement class the truncation + // guard below exists for, missed on the other side of the terminator. The + // canonical idiom's tail is exactly one closing-paren line; anything else + // means the substitution is composed with more text, so fall back to the + // opener line — blocked, the pre-fix behaviour for the whole form. + if (end !== -1) { + const tail = body.slice(end + 1); + if (tail.length !== 1 || !/^[ \t]*\)[ \t]*$/.test(tail[0])) return lines[0]; + } + + // git's default `cleanup=whitespace` strips leading blank lines, so the subject + // is the first NON-EMPTY body line, not blindly the first one. Taking lines[1] + // returned '' for a body that starts blank and falsely blocked a conforming + // commit — the very defect class #3802 reports (review of #3802). + const scan = end === -1 ? body : body.slice(0, end); + // "Blank" is git's ASCII definition — space and tab — never JavaScript's + // trim(), whose Unicode whitespace class skips lines git KEEPS: a body whose + // first line is a NBSP resolved to the SECOND line while git's real subject + // is the NBSP line — an enforcement bypass (Codex review of #3816, verified + // against `git stripspace`, which preserves the c2a0 bytes). A Unicode-blank + // first line is now returned as the subject and fails the format gate: the + // same fail-closed direction git itself takes. + const idx = scan.findIndex((l) => !/^[ \t]*$/.test(l)); + if (idx === -1) return end === -1 ? lines[0] : ''; + + // Truncation is only fatal to the line it lands IN. 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 is still + // measurable; only a subject line that runs to the end of a truncated capture + // is not, and that one falls back to the opener — which fails the format gate, + // exactly as this whole form did before the fix. Without this, the length gate + // measured a PREFIX of the real subject and let an over-long message through + // (review of #3802). + if (end === -1 && idx >= body.length - 1) return lines[0]; + + // git's `cleanup=whitespace` strips whitespace at BOTH ends of the line, not + // just leading blanks. Measuring the raw line rejected `feat: <66 x's>` plus + // three trailing spaces as 75 chars when git's actual subject is a conforming + // 72 — a still-blocked conforming commit, the very defect #3802 reports + // (review of #3816). Trailing only, here: leading blank-LINE handling is the + // findIndex above, and `<<-` leading-tab stripping already happened. + return scan[idx].replace(/[ \t]+$/, ''); +} + +module.exports = { isGitSubcommand, tokenize, extractBranchArgument, skipToSubcommand, resolveCommitSubject }; diff --git a/tests/helpers/fast-check-setup.cjs b/tests/helpers/fast-check-setup.cjs index ae5661c82..68fad5de7 100644 --- a/tests/helpers/fast-check-setup.cjs +++ b/tests/helpers/fast-check-setup.cjs @@ -4,7 +4,8 @@ * fast-check-setup.cjs * * Shared configuration for all property-based tests. Require this at the - * top of every *.property.test.cjs file before any fc.assert() call. + * top of every test file that calls fc.assert() — property-dedicated + * (*.property.test.cjs) or not — before the first fc.assert(). * * Settings: * numRuns: 200 — enough to catch boundary bugs without slow CI diff --git a/tests/hooks-opt-in.test.cjs b/tests/hooks-opt-in.test.cjs index e19b01abf..298500f50 100644 --- a/tests/hooks-opt-in.test.cjs +++ b/tests/hooks-opt-in.test.cjs @@ -336,6 +336,971 @@ describe('hook execution when enabled', { skip: isWindows ? 'bash hooks require `expected typed code: 'CONVENTIONAL_COMMITS_VIOLATION', got: ${JSON.stringify(parsed)}`); }); + // #3802 — the heredoc `-m` form. Claude Code's own documented commit idiom is + // + // git commit -m "$(cat <<'EOF' + // feat(auth): add login flow + // EOF + // )" + // + // The `-m` capture regex spans it whole, because bash `[^"]` matches newlines, + // so the first line was the literal `$(cat <<'EOF'` and EVERY heredoc-form + // commit was blocked regardless of its message. + const heredoc = (body, open = "<<'EOF'", close = 'EOF') => + `git commit -m "$(cat ${open}\n${body}\n${close}\n)"`; + const runHookCmd = (command) => spawnHook(path.join(HOOKS_DIR, 'gsd-validate-commit.sh'), { + input: JSON.stringify({ tool_input: { command } }), + encoding: 'utf-8', + cwd: tmpDir, + }); + + test('validate-commit allows a CONFORMING heredoc-form message', () => { + const result = runHookCmd(heredoc('feat(auth): add login flow')); + assert.strictEqual(result.status, 0, + `a conforming heredoc message must pass; got ${result.status}. stdout: ${result.stdout}`); + }); + + test('validate-commit still BLOCKS a non-conforming heredoc-form message', () => { + const result = runHookCmd(heredoc('wibble wobble no type here')); + assert.strictEqual(result.status, 2, + 'resolving the heredoc body must not become a blanket exemption for the whole form'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); + + + // ─── #3816 round 8 (Major): bracket classes must not smuggle a literal `\` ─── + // + // `;`, `&` and `|` are shell metacharacters inside `[[ ]]`, so an inline + // bracket class has to escape each one: `[\;\&\|]`. POSIX bracket expressions + // have no escape mechanism of their own, and on bash 3.2 — the system + // /bin/bash on macOS, already a supported target here (see the `declare -A` + // ban in tests/install.test.cjs) — those backslashes reach the regex engine + // instead of being consumed by the shell, so the class silently gains a + // literal `\` as a member. bash 4+ consumes them, which is why this is + // invisible on a modern bash. The escape is only a hazard INSIDE a bracket + // expression: `\(` outside one is made literal correctly on every version. + // + // One root cause, consequences in BOTH directions: + // the SEPARATOR scan (positive class) OVER-BLOCKED — a conforming commit + // whose pre-`-m` text held a `\` was refused outright; + // the GLUE scan (NEGATED class) UNDER-REFUSED — a `\` glued to the message + // span fell inside the exclusion, so the hook resolved a heredoc it + // should have declined. That is the accept direction, and it is the row + // that matters most below. + // + // The fix holds each class in a variable expanded unquoted on the right of + // `=~`, which is a plain regex on 3.2 and 5.x alike. Writing the class inline + // without the backslashes is NOT the fix: `[[ x =~ [;&|] ]]` is a bash syntax + // error on both versions. + // + // Every row runs under each bash on the machine, because a row run only under + // bash 4+ passes with or without the fix — vacuous, and silently so. Where + // 3.2 is absent the row simply does not appear; that is disclosed here rather + // than papered over. + const BASHES = (() => { + const seen = new Set(); + const found = []; + for (const candidate of ['/bin/bash', '/usr/local/bin/bash', '/opt/homebrew/bin/bash']) { + if (!fs.existsSync(candidate)) continue; + const probe = runHook('-c', ['printf %s "$BASH_VERSION"'], { + interpreter: candidate, env: hookEnv, timeoutMs: HOOK_TIMEOUT_MS, + }); + const version = (probe.stdout || '').trim(); + if (probe.exitCode !== 0 || !version || seen.has(version)) continue; + seen.add(version); + found.push({ path: candidate, version }); + } + return found; + })(); + + const underBash = (bashPath, command) => runHook( + path.join(HOOKS_DIR, 'gsd-validate-commit.sh'), [], + { + interpreter: bashPath, + env: hookEnv, + timeoutMs: HOOK_TIMEOUT_MS, + input: JSON.stringify({ tool_input: { command } }), + cwd: tmpDir, + }, + ); + + for (const bash of BASHES) { + // ACCEPT DIRECTION — the one that cannot be recovered from. Before the fix, + // bash 3.2 measured exit 0 here: the backslash sat inside the negated glue + // class, so the guard never fired and the heredoc was resolved anyway. + test(`a backslash-glued suffix is still refused (bash ${bash.version})`, () => { + const glued = `${heredoc('fix: a perfectly ordinary conforming subject')}\\zzz`; + const result = underBash(bash.path, glued); + assert.strictEqual(result.exitCode, 2, + `bash ${bash.version}: a suffix glued to the message span with a backslash must be ` + + 'refused exactly like any other glued suffix — resolving it is the accept direction. ' + + `stdout: ${result.stdout}`); + }); + + test(`a letter-glued suffix is still refused (bash ${bash.version})`, () => { + // Non-vacuity control for the row above: proves the glue guard is + // reachable at all under this interpreter, so a refusal there is the + // guard firing rather than the hook refusing everything. + const glued = `${heredoc('fix: a perfectly ordinary conforming subject')}zzz`; + assert.strictEqual(underBash(bash.path, glued).exitCode, 2, + `bash ${bash.version}: the established glued-suffix refusal must be unchanged`); + }); + + // OVER-BLOCK DIRECTION — the reported half. The backslash sits in the text + // BEFORE `-m`, and deliberately with no newline anywhere before it: a + // newline is refused by the separator guard on every bash, which would make + // this row pass for the wrong reason and prove nothing about the class. + test(`a backslash before -m does not block a conforming commit (bash ${bash.version})`, () => { + const command = `git commit --allow-empty --author=a\\,b -m "$(cat <<'EOF'\n` + + `fix: a perfectly ordinary conforming subject\nEOF\n)"`; + const result = underBash(bash.path, command); + assert.strictEqual(result.exitCode, 0, + `bash ${bash.version}: a conforming commit must not be refused merely because its ` + + `pre-message text contains a backslash. stdout: ${result.stdout}`); + }); + + test(`a non-conforming subject is still blocked (bash ${bash.version})`, () => { + // Second non-vacuity control: proves this interpreter reaches the + // validator rather than passing everything — the failure mode that makes + // an allow-row look green for the wrong reason. + const result = underBash(bash.path, 'git commit -m "nope not conventional"'); + assert.strictEqual(result.exitCode, 2, + `bash ${bash.version}: the validator must still be reached and still refuse`); + }); + } + test('validate-commit measures subject length against the RESOLVED heredoc subject', () => { + // RULESET.TESTS.boundary-coverage: N at {limit-1, limit, limit+1}, not merely + // "very long". The limit is 72, and the gate is `> 72`, so 72 must PASS and + // 73 must block. A trivially-oversized subject alone would not show which + // side of the comparison the code sits on. + const at = (n) => { + const prefix = 'feat(auth): '; + return `${prefix}${'x'.repeat(n - prefix.length)}`; + }; + for (const [n, want] of [[71, 0], [72, 0], [73, 2]]) { + const subject = at(n); + assert.strictEqual(subject.length, n, `fixture built wrong: ${subject.length} != ${n}`); + const result = runHookCmd(heredoc(subject)); + assert.strictEqual(result.status, want, + `resolved heredoc subject of ${n} chars: expected exit ${want}, got ${result.status}`); + if (want === 2) { + assert.strictEqual(JSON.parse(result.stdout).code, 'COMMIT_SUBJECT_TOO_LONG', + 'must fail on LENGTH, not format — a format failure would mean the opener was still ' + + 'being read as the subject'); + } + } + + // Review of #3816, Major 2: the clean fixtures above cannot see a + // one-directional cleanup=whitespace implementation. git strips TRAILING + // whitespace too, so a 72-char subject plus trailing spaces is a conforming + // commit — measuring the raw 75 chars re-blocks it, the very defect #3802 + // reports. And the guard must strip, not blanket-allow: 73 chars plus a + // trailing space is still over-long once stripped. + const dirty72 = runHookCmd(heredoc(`${at(72)} `)); + assert.strictEqual(dirty72.status, 0, + `git's actual subject is 72 chars — measuring the raw line as 75 must not block it; ` + + `got ${dirty72.status}: ${dirty72.stdout}`); + const dirty73 = runHookCmd(heredoc(`${at(73)} `)); + assert.strictEqual(dirty73.status, 2, + 'a 73-char subject stays blocked with trailing whitespace attached — stripping must not ' + + 'become an allowance'); + assert.strictEqual(JSON.parse(dirty73.stdout).code, 'COMMIT_SUBJECT_TOO_LONG'); + }); + + test('validate-commit does not resolve a TRUNCATED capture past its own limit', () => { + // Review of #3802, Major 3. An embedded `"` truncates the `-m` capture, so the + // resolver would otherwise measure a PREFIX of the real subject and let an + // over-long message through — an enforcement hole that did not exist before + // this fix. git's real subject here is 100+ chars; the captured prefix is 10. + const result = runHookCmd(`git commit -m "$(cat <<'EOF'\nfeat: aaaa" ${'z'.repeat(90)}\nEOF\n)"`); + assert.strictEqual(result.status, 2, + 'a capture with no terminator cannot be measured, so it must fall back to the pre-fix ' + + 'behaviour (blocked) rather than resolving to a prefix that slips under the length gate'); + }); + + test('validate-commit skips leading blank lines in the heredoc body, as git does', () => { + // Review of #3802, Minor 1. git's default cleanup=whitespace strips leading + // blank lines, so this commit's real subject is conforming — blocking it is + // the same false-positive class #3802 reports. + assert.strictEqual(runHookCmd(heredoc('\nfeat(auth): real subject after a blank line')).status, 0, + 'the subject is the first NON-empty body line'); + }); + + test('validate-commit resolves the QUOTED heredoc opener spellings, both directions', () => { + // Both directions per spelling, deliberately. Asserting only "conforming + // passes" would also pass if the resolver returned an empty subject for a + // spelling it failed to recognise — an allow, but for the wrong reason + // (review of #3802). Pairing it with a non-conforming body that must BLOCK + // proves the body is genuinely being read. + // + // Only the spellings that SUPPRESS expansion belong here: `<<'D'`, `<<"D"` + // and `<<\D`. The bare spellings moved to the row below, which pins the + // opposite contract (review of #3816, round 4). + for (const [label, open, close, indent] of [ + ["<<-'TAG' (tab-stripped)", "<<-'MSG'", '\tMSG', '\t'], + ["<<'END-MSG' (non-identifier tag)", "<<'END-MSG'", 'END-MSG', ''], + ['<<\\TAG (backslash-quoted)', '<<\\EOF', 'EOF', ''], + ]) { + assert.strictEqual(runHookCmd(heredoc(`${indent}fix(api): correct status code`, open, close)).status, 0, + `${label}: a conforming message in this spelling must pass`); + assert.strictEqual(runHookCmd(heredoc(`${indent}wibble wobble`, open, close)).status, 2, + `${label}: a NON-conforming message in this spelling must still block — if this passes, the ` + + 'resolver is returning an empty subject rather than reading the body'); + } + }); + + test('validate-commit BLOCKS a bare heredoc delimiter — bash expands that body (round-4 BLOCKER)', () => { + // Review of #3816, round 4. This row previously asserted the OPPOSITE + // (`['bare < { + // The measured bypass itself, in both its gate-dodging forms. Non-vacuous: + // each literal body IS conforming and IS within 72 chars, so a resolver + // that measured the literal returns exit 0 — which is what head did before + // this fix (base=2 -> head=0, measured against the real hook). + const expanded = runHookCmd('git commit -m "$(cat < { + // Review of #3816, round 4. Inside `-m '...'` bash performs NO command + // substitution, so `$(cat <<'EOF'` is literal text and git's real subject + // is that opener line. Resolving the body there validates a message git + // never receives. All four spellings measured base=2 -> head=0 before this + // fix; reachable by the ordinary slip of typing `'` for `"`. + // + // Non-vacuous by construction: every body below is conforming, so a hook + // that resolves the sq arm returns exit 0 on all four. + // + // The `heredoc()` helper hard-codes the double quote, which is exactly why + // this arm went untested for three rounds — these rows build the command + // directly. + for (const [label, open] of [ + ['<<"EOF"', '<<"EOF"'], + ["<<'EOF'", "<<'EOF'"], + ['<<\\EOF', '<<\\EOF'], + ['bare < { + // Review of #3816, round 4, Minor 1. The guard tested BOTH quote styles + // against the whole command irrespective of which arm produced the + // message, so a double-quoted heredoc whose BODY mentions a glued + // single-quoted token tripped the sq arm and lost the fix for a message + // that never had a prefix problem. Measured 2/2 before, 0 after. + assert.strictEqual( + runHookCmd(heredoc("feat: stop passing -m 'foo'bar to git")).status, 0, + 'a glued single-quoted token inside a DOUBLE-quoted heredoc body must not trip the ' + + 'single-quote adjacency arm'); + }); + + test('validate-commit blocks a substitution composed with more text (round-3 BLOCKER)', () => { + // Review of #3816, round 3. bash expands this -m argument to a SINGLE + // 200+ char subject, but the resolver discarded everything after the + // terminator and measured `feat: ok` (8 chars) — a live length-gate + // bypass the base did not have. The post-terminator guard now falls back + // to the opener line, so the form is blocked by the FORMAT gate, the + // pre-fix behaviour for the whole form. + const result = runHookCmd(`git commit -m "$(cat <<'EOF'\nfeat: ok\nEOF\n) ${'a'.repeat(200)}"`); + assert.strictEqual(result.status, 2, + 'a heredoc substitution composed with trailing text is one long real subject — resolving ' + + 'the body alone dodges COMMIT_SUBJECT_TOO_LONG'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION', + 'fail-closed via the format gate on the opener fallback, matching every other unresolvable shape'); + }); + + test('validate-commit blocks a suffix glued OUTSIDE the closing quote (adjacency guard)', () => { + // Codex review of #3816, round 3. bash concatenates `"$(…)"aaaa…` into ONE + // argument, but the capture holds only the quoted part — so the resolver + // measured `feat: ok` (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). Glued text after the closing quote now skips the resolver and + // keeps the pre-fix first-line subject, which for the heredoc form is the + // opener — blocked, base parity restored. + const result = runHookCmd(`git commit -m "$(cat <<'EOF'\nfeat: ok\nEOF\n)"${'a'.repeat(200)}`); + assert.strictEqual(result.status, 2, + 'a quoted substitution with an adjacent unquoted suffix is one long real subject — the ' + + 'captured prefix must not be resolved and measured on its own'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); + + test('adjacency on a PLAIN single-line message keeps base behavior (pre-existing, unchanged)', () => { + // Differential pin: on base, `-m "feat: ok"zzz` captured `feat: ok`, + // validated it, and ALLOWED the commit even though bash's real argument is + // `feat: okzzz`. That is a pre-existing capture limit (same family as + // rows 16-20 of the round-3 review's table), and the adjacency guard + // deliberately preserves it rather than widening scope: the guard's job is + // to stop the RESOLVER from measuring a prefix, not to fix the capture. + const result = runHookCmd('git commit -m "feat: ok"zzz'); + assert.strictEqual(result.status, 0, + 'base allowed this shape; the adjacency guard must not silently change plain-form behavior'); + }); + + test('validate-commit blocks a command smuggled before the cat', () => { + // Codex review of #3816. `$(id;/bin/cat <<'EOF' ...` runs `id` FIRST, so + // git's real subject is id's output — but the resolver read the heredoc + // body and the conforming `fix: smuggled` sailed through: an enforcement + // bypass end to end. Recognition now rejects a path prefix carrying shell + // metacharacters and the whole form falls back to blocked. + const result = runHookCmd(`git commit -m "$(id;/bin/cat <<'EOF'\nfix: smuggled\nEOF\n)"`); + assert.strictEqual(result.status, 2, + 'a command substitution that runs anything besides cat cannot have its heredoc body ' + + 'trusted as the subject'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); + + test('KNOWN LIMIT: the <<"TAG" spelling stays blocked — the capture cannot deliver it', () => { + // This row pins a LIMIT, not desired behaviour (Codex review of #3816). + // The `-m` capture stops at the first `"`, which in this spelling is the + // delimiter's own quote, so the resolver only ever sees a truncated opener + // and even a conforming message is blocked — the pre-fix behaviour for the + // whole form, fail closed. If this row ever starts passing, the capture + // changed: re-review every embedded-quote case before celebrating. + // Counterpart: the UNIT row in tests/worktree-safety.test.cjs proves the + // pure resolver CAN resolve this spelling — the limit is the capture, + // not the parser; the two rows are correct together (review of #3816, + // round 3, N2). + const result = runHookCmd(heredoc('feat(api): conforming subject', '<<"EOF"', 'EOF')); + assert.strictEqual(result.status, 2, + 'documented residual false-positive on the double-quoted delimiter spelling'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); + + test('validate-commit does not treat a message ENDING in < { + // Enforcement bypass found in review of #3802: an earlier revision recognised + // the opener without anchoring it to a command substitution, so this resolved + // to line 2 and ALLOWED a commit whose real subject is non-conforming. + const result = runHookCmd('git commit -m "WIP notes < { + // Differential pins. Each of these was ALLOWED before this change, and an + // earlier revision that walked tokens to find `-m` started BLOCKING all of + // them (review of #3802). They are not incidental: `--` introduces pathspecs, + // `&&` starts a different command, and the shared scanner drops empty tokens + // so a following flag can be mistaken for the message. + for (const [label, cmd] of [ + ['-- introduces pathspecs', 'git commit -- -m WIP'], + ['a later command\'s flag', 'git commit --amend && echo -m WIP'], + ['empty -m before a flag', 'git commit -m "" --allow-empty-message'], + ['empty -m before a real -m', 'git commit -m "" -m "fix: real subject"'], + ['unquoted -m argument', 'git commit -m WIP'], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 0, + `${label}: this form was allowed before #3802 and must stay allowed — widening WHICH ` + + 'argument counts as the message is out of scope for this fix'); + } + }); + + test('validate-commit resolves only git\'s FIRST message argument (round-4 Codex BLOCKER)', () => { + // The `-m` capture is a SEARCH over the whole command and the double-quoted + // arm is tried first, so it could select a `-m` that is not git's subject. + // git CONCATENATES multiple -m arguments and the SUBJECT is the first one — + // verified against real commits, not the man page: for + // `-m 'WIP first' -m "$(cat …)"` git records `WIP first`. + // + // Every row below measured base=2 -> head=0 before this guard. Non-vacuous + // by construction: each heredoc body is conforming, so a hook that resolves + // the wrong -m returns 0 on all four. The counterpart row above + // ("leaves every non-heredoc form exactly as it was") covers these same + // positions with a plain `WIP`, which never activates the resolver — which + // is exactly why this interaction went unnoticed. + const body = "$(cat <<'EOF'\nfeat: accepted body\nEOF\n)"; + for (const [label, cmd] of [ + ['an earlier single-quoted -m', `git commit --allow-empty -m 'WIP first' -m "${body}"`], + ['an earlier unquoted -m', `git commit -m WIP -m "${body}"`], + ['after -- it is a pathspec, not a message', `git commit -m WIP -- -m "${body}"`], + ['it belongs to a later command', `git commit -m WIP && echo -m "${body}"`], + ]) { + const result = runHookCmd(cmd); + assert.strictEqual(result.status, 2, + `${label}: git's real subject is the FIRST message, which is non-conforming — resolving ` + + 'the later heredoc validates text git never uses as the subject'); + assert.strictEqual(JSON.parse(result.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + } + }); + + test('validate-commit refuses to resolve under a non-default cleanup mode (round-4 Codex BLOCKER)', () => { + // The resolver strips trailing whitespace and skips leading blank lines + // because git's DEFAULT cleanup=whitespace does. Under `--cleanup=verbatim` + // git does neither, so this subject is committed at 75 bytes while the hook + // measured the stripped 72 — COMMIT_SUBJECT_TOO_LONG dodged (base=2 -> + // head=0). Confirmed by reading the RAW commit object: `git log --pretty=%s` + // strips trailing whitespace in its own output and hides the difference. + const subject72 = `feat: ${'x'.repeat(66)}`; + assert.strictEqual(subject72.length, 72, 'fixture built wrong'); + const heredocBody = `"$(cat <<'EOF'\n${subject72} \nEOF\n)"`; + + for (const [label, cmd] of [ + ['--cleanup=verbatim', `git commit --allow-empty --cleanup=verbatim -m ${heredocBody}`], + ['-c commit.cleanup=verbatim', `git -c commit.cleanup=verbatim commit --allow-empty -m ${heredocBody}`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 2, + `${label}: git preserves the trailing whitespace, so the real subject is 75 chars — the ` + + 'hook must not measure the stripped form'); + } + + // Non-vacuity: the DEFAULT mode is the case the fix exists for, and it must + // still resolve and allow. Without these the rows above would pass for a + // hook that simply stopped resolving everything. + for (const [label, cmd] of [ + ['--cleanup=whitespace', `git commit --allow-empty --cleanup=whitespace -m ${heredocBody}`], + ['no cleanup flag', `git commit --allow-empty -m ${heredocBody}`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 0, + `${label}: git strips the trailing whitespace here, so the real subject is a conforming 72`); + } + + // SCOPE (review of #3816, round 5 — BLOCKER). The guard scanned the whole + // command, and the heredoc BODY sits verbatim inside it, so a conforming + // message that merely MENTIONED the token was refused and fell back to the + // opener line — blocked with CONVENTIONAL_COMMITS_VIOLATION. These are + // ordinary English in this repository, whose own hooks and docs discuss + // cleanup modes constantly. Every row is a valid Conventional Commit that + // git would accept without complaint. + for (const [label, subject] of [ + ['commit.cleanup= in the subject', 'fix: document commit.cleanup=strip behavior'], + ['--cleanup= in the subject', 'docs: explain --cleanup=verbatim in the hook guide'], + ['the token on a later body line', 'fix: correct the guard scope\n\nIt scanned --cleanup=verbatim in the body.'], + ]) { + const result = runHookCmd(`git commit -m "$(cat <<'EOF'\n${subject}\nEOF\n)"`); + assert.strictEqual(result.status, 0, + `${label}: the token is message TEXT, not a flag git will act on — refusing to resolve ` + + 'here blocks a commit git would accept'); + } + + // The scope fix must not shrink to $MSG_PREFIX alone. git accepts the flag + // on EITHER side of -m, so a trailing occurrence is a real mode change and + // must still refuse — this row reds against a prefix-only scoping and is + // what keeps the round-4 length-gate bypass closed from both directions. + assert.strictEqual( + runHookCmd(`git commit --allow-empty -m ${heredocBody} --cleanup=verbatim`).status, 2, + 'a --cleanup after the message changes the mode just as one before it does'); + }); + + test('the adjacency guard is scoped to the span it matched (round-6 MAJOR)', () => { + // Glue is a property of the ONE character following the MATCHED span, so + // that character is the whole window. Scanning $CMD for the shape anywhere + // refused any conforming commit whose command merely CONTAINED a glued -m + // elsewhere. Base blocks these too, because base blocks EVERY heredoc form + // (that is #3802), so this is the fix not reaching the shape rather than a + // regression: measured base=2 -> pre=2 -> post=0. + const conforming = "\"$(cat <<'EOF'\nfix: a perfectly ordinary conforming subject\nEOF\n)\""; + + assert.strictEqual( + runHookCmd(`git commit -m ${conforming} && echo -m "test"z`).status, 0, + 'a glued -m in a chained-after command is in another argv and cannot truncate this capture'); + + // Bash does not concatenate across a command separator or a redirection: in + // `-m "msg"&& echo hi` the argument ends at the quote, so there is no + // truncated capture and nothing to defend against. This row reds against a + // bare `^[^[:space:]]` test, which is why the class excludes `;&|()<>`. + assert.strictEqual( + runHookCmd(`git commit -m ${conforming}&& echo hi`).status, 0, + 'a separator abutting the closing quote ends the argument; it does not glue onto it'); + + // A suffix that really is glued still refuses. + assert.strictEqual( + runHookCmd(`git commit -m ${conforming}zzzz`).status, 2, + 'text glued to the closing quote means the capture holds only a prefix of the real message'); + }); + + test('the cleanup guard scans wide on purpose — narrowing it reopened a length hole', () => { + // The window is the whole command minus the message. That is deliberately + // wider than git's own command, and the cost is a known false positive: + // a --cleanup= belonging to a DIFFERENT command refuses a commit git would + // accept. Narrowing it to git's own segment was tried and reverted, because + // deciding where git's command ends needs a shell parse and a substring + // scan is not one. + const long72 = `feat: ${'x'.repeat(66)}`; + const longHd = `"$(cat <<'EOF'\n${long72} \nEOF\n)"`; + + // These rows are the ACCEPT direction and are the reason the guard stays + // wide. Trimming the window at the first `;&|` cut it short whenever a + // separator sat inside an ordinary argument, hiding the real trailing + // --cleanup=verbatim: git then commits the trailing whitespace verbatim and + // the real subject is 75 bytes while the gate measured 72. Both the quoted + // and the backslash-escaped spelling must stay blocked; each reds against + // one of the two narrowings that were attempted. + for (const [label, author] of [ + ['quoted separator', '"a&b"'], + ['quoted pipe', '"a|b"'], + ['quoted semicolon', '"a;b"'], + ['escaped separator', 'a\\&b'], + ['escaped pipe', 'a\\|b'], + ['escaped semicolon', 'a\\;b'], + ]) { + assert.strictEqual( + runHookCmd(`git commit --allow-empty -m ${longHd} --author ${author} --cleanup=verbatim`).status, + 2, + `${label}: a separator inside an argument must not hide the --cleanup that follows it`); + } + + // The documented false positive, pinned so the trade-off is visible rather + // than accidental. If this ever needs to pass, it needs a real shell parse. + assert.strictEqual( + runHookCmd(`git commit -m "$(cat <<'EOF'\nfix: a perfectly ordinary conforming subject\nEOF\n)" && echo --cleanup=verbatim`).status, + 2, + 'known limit: a --cleanup in a later command also refuses — fail-closed, and preferred ' + + 'over the accept-direction hole that narrowing the window reopened'); + }); + + test('option scans read the command the way bash hands it to git (round-6 accept direction)', () => { + // Three ways the same option can be spelled without matching a literal. + // Every row below was measured ACCEPTING a commit whose real subject git + // records as 75 bytes, or whose real subject is a different -m argument + // entirely — verified against real commits by reading the raw commit + // object, since `git log --pretty=%s` strips the trailing whitespace that + // makes the length wrong and hides it. All are base=2 -> pre=0, so each is + // an accept-direction regression this PR introduced before this round. + const long72 = `feat: ${'x'.repeat(66)}`; + const longHd = `"$(cat <<'EOF'\n${long72} \nEOF\n)"`; + const conforming = "\"$(cat <<'EOF'\nfix: a perfectly ordinary conforming subject\nEOF\n)\""; + + // git accepts any unambiguous prefix of a long option, so the mode is set + // by a token that is not the literal `--cleanup`. Reds against `--cleanup`. + for (const spelling of ['--cle', '--clea', '--clean', '--cleanu', '--cleanup']) { + assert.strictEqual( + runHookCmd(`git commit --allow-empty -m ${longHd} ${spelling}=verbatim`).status, 2, + `${spelling}=verbatim sets the mode as surely as the unabbreviated spelling does`); + } + + // git splits `-am` into `-a -m`, making the FIRST message the subject and + // the heredoc merely the second. Reds against a standalone `-m` scan. + for (const cluster of ['-am', '-sm', '-anm']) { + assert.strictEqual( + runHookCmd(`git commit --allow-empty ${cluster} 'WIP first' -m ${conforming}`).status, 2, + `${cluster} carries git's first message, so the matched heredoc is not the subject`); + } + + // Bash removes quotes before git sees the argument, so a spliced spelling + // is the same option. Reds unless the option-name scans are dequoted. + assert.strictEqual( + runHookCmd(`git commit --allow-empty -m ${longHd} --clean""up=verbatim`).status, 2, + 'a quote spliced into the option name does not change the option git receives'); + assert.strictEqual( + runHookCmd(`git commit --allow-empty -""m 'WIP first' -m ${conforming}`).status, 2, + 'a quote spliced into -m does not stop it claiming the first message'); + + // Dequoting must not spill into the adjacency test, which asks about a + // literal character position rather than an option name. This is the + // round-6 MAJOR and must stay fixed. + assert.strictEqual( + runHookCmd(`git commit -m ${conforming} && echo -m "test"z`).status, 0, + 'quotes in a chained-after command must not refuse the matched heredoc'); + }); + + test('the two reported chained-before shapes are never reached by any guard (round-7 pin)', () => { + // Round 7 reported that the FIRST-MESSAGE GUARD's `[;&|]` prefix scan + // blocks `git add -A && git commit -m ` and + // `cd dir && git commit -m `. It does not, and cannot: the + // CLASSIFIER GATE runs first and neither shape reaches the guards at all. + // `isGitSubcommand` token-walks from the START of the command — `git`→`add` + // stops on a non-commit subcommand, and a leading `cd` is not git — so the + // hook exits 0 before a single guard is evaluated. + // + // Both rounds 6 and 7 produced this finding by extracting the guard logic + // into a standalone script and feeding it command strings directly, which + // bypasses the gate. These rows exist so the same measurement cannot + // produce a third phantom: they run the REAL hook, end to end. + // + // The NON-CONFORMING rows are what make the pin load-bearing. A row + // asserting only that a conforming chained message exits 0 is satisfied + // both by "resolved correctly" and by "never validated" — the two + // hypotheses under dispute. A message that the bare form blocks, passing + // in the chained form, can only mean the hook never validated it. + const conforming = `"$(cat <<'EOF' +fix: a perfectly ordinary conforming subject +EOF +)"`; + const NONCONFORMING = 'nope not conventional'; + + // Control FIRST: the hook demonstrably blocks this message when it does + // classify the command. Without this row the two below prove nothing, + // because a hook that blocks nothing at all also "allows" them. + const bare = runHookCmd(`git commit -m "${NONCONFORMING}"`); + assert.strictEqual(bare.status, 2, + 'control: the bare form must BLOCK a non-conforming subject — otherwise the chained rows ' + + 'below cannot distinguish "not validated" from "validated and allowed"'); + assert.strictEqual(JSON.parse(bare.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + + for (const [label, prefix] of [ + ['stage-then-commit', 'git add -A && '], + ['cd-then-commit', 'cd /repo && '], + ]) { + // The heredoc shape round 7 says is blocked. It is not. + assert.strictEqual(runHookCmd(`${prefix}git commit -m ${conforming}`).status, 0, + `${label}: a conforming chained heredoc commit is not blocked`); + // ...and the same shape carrying a message the control just proved is + // blockable ALSO exits 0, which is only possible if no validation ran. + assert.strictEqual(runHookCmd(`${prefix}git commit -m "${NONCONFORMING}"`).status, 0, + `${label}: a subject the bare form BLOCKS exits 0 here — proof the classifier gate ` + + 'returns before the guards, so the guards cannot be over-blocking this shape'); + } + + // COUNTEREXAMPLE, so this row is not misread as a blanket claim about every + // chained-before command (independent review of #3816, round 7). Assignment + // detection is prefix-anchored and the tokenizer does not split operators, + // so `FOO=bar;` is read as an assignment prefix and the classifier DOES + // reach `git commit` — which the separator scan then refuses. That makes it + // a genuine false positive of exactly the class round 7 describes, reachable + // where the two reported shapes are not. Left unfixed deliberately: + // narrowing it means changing `isGitSubcommand`, the shared git-commit + // detector every gating hook uses, and it fails CLOSED (a conforming commit + // is blocked, which is recoverable). Disclosed in the changeset instead. + assert.strictEqual(runHookCmd(`FOO=bar; git commit -m ${conforming}`).status, 2, + 'a leading assignment carrying a separator IS classified and then refused — pinned as a ' + + 'known fail-closed false positive, not as desired behaviour. If this ever starts passing, ' + + 'the classifier or tokenizer changed: re-read the chained-before disclosure before ' + + 'celebrating.'); + }); + + // ─── round 7: six accept-direction bypasses found by independent review ─── + // + // Every row below was measured base-vs-head against the REAL hook, and the + // three that turn on which subject git actually records were confirmed + // against the RAW COMMIT OBJECT rather than `git log --pretty=%s`, which + // strips trailing whitespace and would have hidden two of them: + // + // git commit -mWIP -m -> subject `WIP` + // --cleanup=whitespace -m <72+spaces> --cleanup=verbatim -> 75 bytes, WS kept + // git commit --squash= -m -> `squash! …` + // + // In every case the hook resolved and validated the heredoc and ALLOWED the + // commit, while git recorded something the rules would have refused. All six + // fixes widen refusal, never narrow it — the direction this file already + // documents as the recoverable one. + + const HD_OK = `"$(cat <<'EOF'\nfix: a perfectly ordinary conforming subject\nEOF\n)"`; + const S72 = `feat: ${'x'.repeat(66)}`; + const HD_LONG_DIRTY = `"$(cat <<'EOF'\n${S72} \nEOF\n)"`; + + test('the first-message guard recognises attached values and --message abbreviations (round 7)', () => { + // The scan required a space or `=` after the option name, so `-mWIP` (git + // reads it as `-m WIP`) and `--mes=WIP` (git accepts any unambiguous long + // prefix — behaviour this file already models for --cleanup) matched + // nothing. git took `WIP` as the subject; the hook validated the later + // heredoc and allowed it. + assert.strictEqual(S72.length, 72, 'fixture built wrong'); + for (const [label, cmd] of [ + ['attached short-option value', `git commit -mWIP -m ${HD_OK}`], + ['--message abbreviation', `git commit --mes=WIP -m ${HD_OK}`], + ]) { + const r = runHookCmd(cmd); + assert.strictEqual(r.status, 2, + `${label}: git takes the FIRST message as the subject, so the heredoc is not the subject ` + + 'and must not be resolved and measured as though it were'); + assert.strictEqual(JSON.parse(r.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + } + // Non-vacuity: the canonical form still resolves. Without this, the rows + // above would also pass if resolution had simply been disabled outright. + assert.strictEqual(runHookCmd(`git commit -m ${HD_OK}`).status, 0, + 'the canonical single -m heredoc must still pass — that is the fix this PR exists for'); + }); + + test('option-name scans remove backslashes, as bash does (round 7)', () => { + // Round 6 dequoted the option-name windows and the changeset claimed they + // are matched "as bash hands it to git". That was not true: bash also + // removes syntactic backslashes, so `-\\m` IS `-m` and `--clean\\up=` IS + // `--cleanup=`, and both matched no literal. + const r1 = runHookCmd(`git commit -\\m WIP -m ${HD_OK}`); + assert.strictEqual(r1.status, 2, + 'a backslash spliced into -m does not stop it claiming the first message'); + const r2 = runHookCmd(`git commit --allow-empty -m ${HD_LONG_DIRTY} --clean\\up=verbatim`); + assert.strictEqual(r2.status, 2, + 'a backslash spliced into --cleanup does not change the mode git applies, and under verbatim ' + + 'the trailing spaces count toward the 72-character limit'); + assert.strictEqual(runHookCmd(`git commit -m ${HD_OK}`).status, 0, 'non-vacuity: canonical form still resolves'); + }); + + test('option-name scans remove the $ of a dollar-quote, as bash does (round 8)', () => { + // Round 7 removed quotes and backslashes and the changeset again claimed the + // windows are matched "as bash hands it to git". Still not true: bash has two + // more quoting forms, $'…' and $"…", whose introducer is a `$`. 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 the command (exit 0) and `git cat-file -p` recorded the + // subject `WIP`, while the same command spelled `-m WIP` was refused. + for (const [label, cmd] of [ + ['locale-quoted short option', `git commit -$"m" WIP -m ${HD_OK}`], + ['ANSI-quoted short option', `git commit -$'m' WIP -m ${HD_OK}`], + ['locale-spliced --message', `git commit --mes$"sage"=WIP -m ${HD_OK}`], + ['ANSI-spliced --cleanup', `git commit --allow-empty -m ${HD_LONG_DIRTY} --clean$'up'=verbatim`], + ]) { + const r = runHookCmd(cmd); + assert.strictEqual(r.status, 2, + `${label}: a dollar-quote is removed by bash before git sees the argument, so it does not ` + + 'stop the option claiming the message'); + } + assert.strictEqual(runHookCmd(`git commit -m ${HD_OK}`).status, 0, 'non-vacuity: canonical form still resolves'); + }); + + test('an option NAME carrying a shell expansion is unresolvable (round 9)', () => { + // Rounds 6-8 each tried to EMULATE what bash does to an argument before git + // sees it -- remove quotes, then backslashes, then the `$` of a dollar-quote + // -- and review found another missed transform every time. Round 9 found + // four more, all measured accepting `WIP` as the real subject on bash 3.2.57 + // and 5.3.15 while the plain spelling of the same command is refused. + // + // The last two settle the strategy: an option name finished by a PARAMETER + // expansion depends on a variable's runtime value, and one finished by a + // PATHNAME expansion depends on the contents of the working directory. + // Neither is derivable from the command string at all, so the rule is no + // longer "normalise and match the literal" but "an option NAME carrying a + // shell expansion or quoting construct is unresolvable, and unresolvable + // refuses" -- which covers the spellings nobody has thought of yet. + for (const [label, cmd] of [ + ['$() substitution', `git commit --allow-empty -m ${HD_LONG_DIRTY} --clean$(printf up)=verbatim`], + ['backtick substitution', 'git commit --allow-empty -m ' + HD_LONG_DIRTY + ' --clean`printf up`=verbatim'], + ['ANSI-C octal escape', `git commit --allow-empty -$'\\155' WIP -m ${HD_OK}`], + ['ANSI-C hex escape', `git commit --allow-empty -$'\\x6d' WIP -m ${HD_OK}`], + ['parameter expansion', `x= git commit --allow-empty -\${x}m WIP -m ${HD_OK}`], + ['pathname expansion', `git commit --allow-empty -? WIP -m ${HD_OK}`], + ['short-option substitution', `git commit -$(printf m) WIP -m ${HD_OK}`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 2, + `${label}: an option name the shell finishes cannot be read off the command line, so the ` + + 'later heredoc must not be resolved and measured as though it were the subject'); + } + + // SCOPE, in both directions. The class is a `-`-leading token whose + // characters up to the construct contain no `=` -- an option NAME being + // assembled. A construct in the VALUE is something this file never models + // and must stay allowed, or the guard refuses the ordinary + // `--author="$(git config user.name)"` idiom. Without these rows the + // assertions above would also pass for a guard that refuses every `$`. + for (const [label, cmd] of [ + ['spaced value', `git commit --author "$(git config user.name)" -m ${HD_OK}`], + ['glued value', `git commit --author="$(git config user.name)" -m ${HD_OK}`], + ['backtick value', 'git commit --author="`git config user.name`" -m ' + HD_OK], + ['parameter expansion value', `git commit --date="\${NOW}" -m ${HD_OK}`], + ['glob character in a value', `git commit --author="a*b " -m ${HD_OK}`], + ['value in the suffix window', `git commit -m ${HD_OK} --author="$(id -un)"`], + ['pathspec after --', `git commit -m ${HD_OK} -- src/*.js`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 0, + `${label}: a construct in an option VALUE, or outside an option name entirely, is not an ` + + 'option name being assembled and must still resolve'); + } + }); + + test('the round-9 class over-blocks two spellings, and both have working forms', () => { + // Disclosed rather than narrowed. The class refuses a `-`-leading token that + // carries an expansion before any `=`, and two legitimate-looking spellings + // fall inside it. Narrowing to exclude them was considered and rejected: + // dropping the bare-`$` member reopens `-$xm` (with `xm=m` bash hands git a + // real `-m`), and 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. Refusing a + // commit git would take is the recoverable direction; accepting a + // non-conforming subject is not. + for (const [label, cmd] of [ + ['attached short-option value from an expansion', `git commit -S$SIGNING_KEY -m ${HD_OK}`], + ['attached short-option value, quoted', `git commit -S"$SIGNING_KEY" -m ${HD_OK}`], + ['unquoted dash-leading glob after --', `git commit -m ${HD_OK} -- -*.txt`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 2, `${label}: known fail-closed limit of the round-9 class`); + } + + // The working spellings, which is what makes the limit acceptable. Note the + // pathspec ones in particular: a glob only reaches git as a PATHSPEC when it + // is quoted, because an unquoted one is expanded by the shell before git + // sees it. So the spelling that actually passes a glob to git is the one + // that resolves here. + for (const [label, cmd] of [ + ['detached signing key', `git commit -S "$SIGNING_KEY" -m ${HD_OK}`], + ['long signing option', `git commit --gpg-sign="$SIGNING_KEY" -m ${HD_OK}`], + ['single-quoted glob pathspec', `git commit -m ${HD_OK} -- '-*.txt'`], + ['double-quoted glob pathspec', `git commit -m ${HD_OK} -- "-*.txt"`], + ['magic pathspec', `git commit -m ${HD_OK} -- ':(exclude)-*.txt'`], + ]) { + assert.strictEqual(runHookCmd(cmd).status, 0, + `${label}: the spelling a developer reaches for must still resolve`); + } + }); + + test('a backslash-newline continuation before -m is joined, not treated as a separator (round 9)', () => { + // `git commit \` newline ` -m ` was refused because every guard + // read the newline as a separator — disclosed in round 8 as a fail-closed + // limit, re-raised in round 9 as a Major. bash's rule is local: a newline + // preceded by an ODD run of backslashes is a continuation (both removed); + // an EVEN run is a literal backslash plus a REAL newline. Both windows are + // joined the same way before any guard runs. Not bash-version dependent — + // the join is plain parameter expansion — but run under each bash on the + // machine anyway, since the round-8 classes were. + const bashes = ['/bin/bash', '/opt/homebrew/bin/bash'].filter((b) => fs.existsSync(b)); + assert.ok(bashes.length >= 1, 'at least one bash must exist'); + const run = (bash, command) => spawnHook(path.join(HOOKS_DIR, 'gsd-validate-commit.sh'), { + input: JSON.stringify({ tool_input: { command } }), encoding: 'utf-8', cwd: tmpDir, interpreter: bash, + }); + for (const bash of bashes) { + // The idiom itself, on both sides of the message. + assert.strictEqual(run(bash, `git commit \\\n -m ${HD_OK}`).status, 0, + `${bash}: a continuation before -m is joined by bash, so it must be joined here and resolve`); + assert.strictEqual(run(bash, `git commit --allow-empty \\\n --no-verify \\\n -m ${HD_OK}`).status, 0, + `${bash}: several continuations resolve`); + assert.strictEqual(run(bash, `git commit -m ${HD_OK} \\\n --allow-empty`).status, 0, + `${bash}: a continuation in the suffix window resolves`); + + // ACCEPT-DIRECTION CONTROLS. An even run is a literal backslash followed + // by a REAL newline, which is a separator and must still refuse. + const evenRun = run(bash, `git commit \\\\\n -m ${HD_OK}`); + assert.strictEqual(evenRun.status, 2, + `${bash}: backslash-backslash-newline is a literal \\ then a real separator; joining it would let a later command's -m be taken for this commit's`); + // A plain newline is unchanged. + assert.strictEqual(run(bash, `git commit\n -m ${HD_OK}`).status, 2, `${bash}: a bare newline is still a separator`); + // Continuation GLUE: bash joins `"$(…)"\` newline `suffix` into one argument, + // so the glue guard must see it glued and refuse, same as without the newline. + assert.strictEqual(run(bash, `git commit -m ${HD_OK}\\\nsuffix`).status, 2, + `${bash}: a continuation glued to the closing quote is glue, and the joined text must show it`); + // A later command's heredoc-shaped -m after a continuation is STILL a later + // command once the real separator is reached. + assert.strictEqual(run(bash, `git commit --amend --no-edit \\\n --allow-empty\necho -m ${HD_OK}`).status, 2, + `${bash}: joining continuations must not hide the real newline separator that follows`); + } + }); + + test('a newline is a command separator for the first-message guard (round 7)', () => { + // The separator scan covered `;`, `&` and `|` but not a literal newline, so + // a LATER command's heredoc-shaped -m was taken for this commit's message. + // The classifier recognises the leading `git commit`, and the capture reads + // across the newline into echo's argument — a conforming string with no + // relationship to the commit was validated and the commit allowed. + const r = runHookCmd(`git commit --amend --no-edit\necho -m ${HD_OK}`); + assert.strictEqual(r.status, 2, + "a later command's -m is not this commit's message; a newline separates commands exactly as " + + '`;` does'); + assert.strictEqual(JSON.parse(r.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); + + test('more than one cleanup directive is unresolvable — git applies the LAST (round 7)', () => { + // A bash regex yields ONE BASH_REMATCH, so only the FIRST directive was + // inspected while git applies the last. Measured: mode read as whitespace, + // resolution stayed on, and a 72-character subject plus trailing spaces was + // accepted while git recorded 75 bytes with the whitespace preserved. + // Which directive is last needs an argv order a substring scan does not + // have, so multiplicity itself refuses. + const r = runHookCmd(`git commit --allow-empty --cleanup=whitespace -m ${HD_LONG_DIRTY} --cleanup=verbatim`); + assert.strictEqual(r.status, 2, + 'a leading whitespace directive must not vouch for a trailing verbatim one'); + + // Non-vacuity in BOTH directions: a single whitespace directive still + // resolves, and a single non-whitespace one still refuses. Without these, + // the row above passes for a guard that simply refuses every cleanup. + assert.strictEqual(runHookCmd(`git commit --cleanup=whitespace -m ${HD_OK}`).status, 0, + 'one explicit whitespace directive is the documented default and must still resolve'); + assert.strictEqual(runHookCmd(`git commit --allow-empty --cleanup=verbatim -m ${HD_LONG_DIRTY}`).status, 2, + 'one verbatim directive must still refuse — unchanged behaviour'); + }); + + test('a git-GENERATED subject is never measured against the supplied message (round 7)', () => { + // With --squash/--fixup git composes the subject itself, so the supplied + // message is not the subject at all. Measured recording + // `squash! base: something` while a conforming heredoc sailed through. + for (const [label, opt] of [['--squash', '--squash=HEAD'], ['--fixup', '--fixup=HEAD']]) { + const r = runHookCmd(`git commit ${opt} -m ${HD_OK}`); + assert.strictEqual(r.status, 2, + `${label}: git composes the subject, so there is nothing in the -m text worth measuring`); + assert.strictEqual(JSON.parse(r.stdout).code, 'CONVENTIONAL_COMMITS_VIOLATION'); + } + assert.strictEqual(runHookCmd(`git commit -m ${HD_OK}`).status, 0, 'non-vacuity: canonical form still resolves'); + }); + + 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 + // printing `WIP injected`, the resolver validated the heredoc body while + // git's real subject was `WIP injected` (measured base=2 -> head=0 against + // a real commit). Only an absolute path or a bare `cat` is recognised now. + // + // RESIDUAL, and not fixable from a string: a bare `cat` shadowed earlier on + // PATH behaves identically and is indistinguishable here. It is also not a + // meaningful boundary — anyone able to plant an executable on PATH can run + // `git commit` directly. + const body = "<<'EOF'\nfeat: accepted body\nEOF\n)"; + for (const [label, prog] of [['./cat', './cat'], ['../evil/cat', '../evil/cat'], ['x/cat', 'x/cat']]) { + assert.strictEqual(runHookCmd(`git commit -m "$(${prog} ${body}"`).status, 2, + `${label}: a relative executable merely ENDING in cat is not known to echo its stdin`); + } + // Non-vacuity: the legitimate absolute and bare forms still resolve. + assert.strictEqual(runHookCmd(`git commit -m "$(/bin/cat ${body}"`).status, 0, + 'an absolute /bin/cat is the same canonical form and must still resolve'); + assert.strictEqual(runHookCmd(`git commit -m "$(cat ${body}"`).status, 0, + 'a bare cat is the canonical idiom #3802 is about'); + }); + + test('an ABSOLUTE path is not an identity either (round-8 independent review)', () => { + // Round 4 stopped at "must be absolute", so any absolute path ENDING in + // `/cat` was still trusted to echo its stdin — the very thing the round-4 + // reasoning rejected one spelling earlier. With an executable at + // `/some/scratch/dir/cat` printing `WIP injected`, the resolver validated the + // conforming heredoc body while git's real subject was `WIP injected` + // (measured on bash 3.2.57 and 5.3.15 against a real commit: hook exit 0, + // `git cat-file -p` subject `WIP injected`). Recognition is now the canonical + // system locations, the only claim a string can support. + const body = "<<'EOF'\nfeat: accepted body\nEOF\n)"; + for (const [label, prog] of [ + ['a scratch directory', '/tmp/evil/cat'], + ['a home directory', '/Users/someone/bin/cat'], + ['user-writable /usr/local/bin', '/usr/local/bin/cat'], + ]) { + assert.strictEqual(runHookCmd(`git commit -m "$(${prog} ${body}"`).status, 2, + `${label}: an absolute path merely ENDING in cat is not known to echo its stdin`); + } + // Non-vacuity: the canonical spellings must all still resolve, or this guard + // has simply disabled the feature #3802 exists for. + for (const prog of ['cat', '/bin/cat', '/usr/bin/cat']) { + assert.strictEqual(runHookCmd(`git commit -m "$(${prog} ${body}"`).status, 0, + `${prog} is a canonical cat and must still resolve`); + } + }); + test('validate-commit allows non-commit commands', () => { const hookPath = path.join(HOOKS_DIR, 'gsd-validate-commit.sh'); const input = JSON.stringify({ diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index d1d63e5c4..02cd6f9c6 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -4359,7 +4359,11 @@ const path = require('node:path'); const fs = require('node:fs'); const ROOT = path.join(__dirname, '..'); -const { isGitSubcommand, tokenize, extractBranchArgument } = require(path.join(ROOT, 'hooks', 'lib', 'git-cmd.js')); +// Seeded fast-check convention: the shared setup helper, NOT 'fast-check' +// directly, so numRuns/seed are configured globally before any fc.assert(). +// Required by RULESET.TESTS.property-based-testing for the parser added below. +const fc = require('./helpers/fast-check-setup.cjs'); +const { isGitSubcommand, tokenize, extractBranchArgument, resolveCommitSubject } = require(path.join(ROOT, 'hooks', 'lib', 'git-cmd.js')); // ── tokenize ───────────────────────────────────────────────────────────────── @@ -4457,6 +4461,372 @@ describe('gsd-validate-commit.sh delegates to git-cmd.js', () => { }); }); +// ── resolveCommitSubject (#3802) ───────────────────────────────────────────── +// A PURE STRING helper: it maps an already-selected `-m` argument to the subject +// to validate. It deliberately does not tokenize — an earlier revision walked +// tokens and regressed four cases that upstream allowed (`git commit -- -m WIP`, +// `git commit --amend && echo -m WIP`, `-m "" --allow-empty-message`, and +// unquoted `git commit -m WIP`). Reported in review of #3802. +describe('git-cmd.js resolveCommitSubject', () => { + const sub = (open, body, close) => `$(cat ${open}\n${body}\n${close}\n)`; + + test('resolves the heredoc body rather than the opener', () => { + assert.strictEqual(resolveCommitSubject(sub("<<'EOF'", 'feat(auth): add login flow', 'EOF')), + 'feat(auth): add login flow'); + }); + + test('accepts the QUOTED opener spellings, which bash does not expand', () => { + // Only the spellings that SUPPRESS expansion may be resolved. The two bare + // rows that used to live here — `< { + // Codex review of #3816, round 4. The path class accepted `./cat` and + // `../evil/cat`, so any relative executable merely ENDING in `cat` was + // trusted to echo its stdin. With a planted one printing `WIP injected`, + // the resolver validated the heredoc body while git's real subject was + // `WIP injected` (measured base=2 -> head=0 against a real commit). + // Non-vacuous: each body below is conforming, so a resolver that still + // recognised these returns the body. + for (const prog of ['./cat', '../evil/cat', 'x/cat']) { + assert.strictEqual(resolveCommitSubject(`$(${prog} <<'EOF'\nfix: body\nEOF\n)`), + `$(${prog} <<'EOF'`, `${prog}: a relative path is not a known cat`); + } + }); + + test('a path-qualified cat is still the same form', () => { + assert.strictEqual(resolveCommitSubject("$(/bin/cat <<'EOF'\nfix: pathed cat\nEOF\n)"), + 'fix: pathed cat'); + }); + + test('<<- strips the leading tabs bash strips', () => { + // With `<<-`, bash removes leading TABS from body lines, so the subject the + // user sees has none. Returning the raw line blocked a conforming message. + // The delimiter is QUOTED here because `<<-` and quoting are independent: + // `<<-` controls tab stripping, the quote controls expansion. This row is + // about tab stripping, so it uses a spelling that is resolvable at all — + // a bare `<<-EOF` is refused by the round-4 expansion guard, which is that + // guard's row to assert, not this one's. + assert.strictEqual(resolveCommitSubject("$(cat <<-'EOF'\n\tfix(parser): strip heredoc tabs\n\tEOF\n)"), + 'fix(parser): strip heredoc tabs'); + }); + + test('an immediately-following terminator is an EMPTY message, not a subject', () => { + // `$(cat <<'EOF'` then straight to `EOF` — the message is empty, and the + // delimiter must not be mistaken for the subject. + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nEOF\n)"), ''); + assert.strictEqual(resolveCommitSubject("$(cat <<-'EOF'\n\tEOF\n)"), '', + 'the <<- form strips the tab first, so the terminator still matches'); + }); + + // The security half. Recognition is anchored at BOTH ends and requires a + // command substitution, so a message that merely contains — or ENDS IN — + // `< { + // Without the `^$(` anchor this resolved to line 2 and ALLOWED a + // non-conforming commit (review of #3802). + assert.strictEqual(resolveCommitSubject('WIP notes < { + assert.strictEqual(resolveCommitSubject('fix(parser): preserve literal < { + // Codex review of #3816: `\S*` as the path prefix accepted `id;/bin/cat`, + // so the resolver validated the heredoc BODY while bash runs `id` first and + // git's real subject is id's OUTPUT — an enforcement bypass. A prefix + // carrying any shell metacharacter now fails recognition and falls back to + // the opener line, which the format gate rejects. + assert.strictEqual(resolveCommitSubject("$(id;/bin/cat <<'EOF'\nfix: smuggled\nEOF\n)"), + "$(id;/bin/cat <<'EOF'"); + assert.strictEqual(resolveCommitSubject("$(x&&/bin/cat <<'EOF'\nfix: smuggled\nEOF\n)"), + "$(x&&/bin/cat <<'EOF'"); + assert.strictEqual(resolveCommitSubject("$(a|b/cat <<'EOF'\nfix: smuggled\nEOF\n)"), + "$(a|b/cat <<'EOF'"); + // Round 2: Unicode whitespace after `$(` is NOT bash whitespace — bash + // reads `/bin/cat` as the executable NAME, so recognizing it here + // claimed a substitution that does not run cat. Recognition whitespace is + // ASCII space/tab only. + assert.strictEqual(resolveCommitSubject("$(\u00a0/bin/cat <<'EOF'\nfix: smuggled\nEOF\n)"), + "$(\u00a0/bin/cat <<'EOF'"); + // the legitimate path-qualified form is unchanged + assert.strictEqual(resolveCommitSubject("$(/usr/bin/cat <<'EOF'\nfix: pathed\nEOF\n)"), + 'fix: pathed'); + }); + + test('a Unicode-blank first line is the SUBJECT — git keeps what trim() skips', () => { + // Codex review of #3816, verified against `git stripspace`: git's blank is + // ASCII space/tab, so a NBSP line is PRESERVED and is the real subject. + // JavaScript's trim() treated it as blank and resolved to the second line — + // validating a line git never uses, an enforcement bypass. + const nbsp = '\u00a0'; + assert.strictEqual(resolveCommitSubject(`$(cat <<'EOF'\n${nbsp}\nfix: smuggled\nEOF\n)`), nbsp, + 'the NBSP line must be returned (and fail the format gate), never skipped past'); + }); + + test('MAJOR 3: a TRUNCATED capture is not resolved at all', () => { + // The `-m` capture stops at the first `"`, so a message containing one + // arrives here without its tail — and without its terminator. Resolving + // anyway hands the length gate a PREFIX of the real subject and lets an + // over-long message through: an enforcement hole that did not exist before + // this fix. Falling back to the opener fails the format gate, which is what + // this whole form did before the fix (review of #3802). + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: aaaa"), "$(cat <<'EOF'", + 'the subject line runs to the end of a truncated capture, so it cannot be measured'); + // Truncation is only fatal to the line it lands IN: a captured line is + // complete exactly when another line follows it. A quote further down the + // BODY leaves the subject intact and measurable, so blocking it would be a + // false positive the blunt version of this guard would have introduced. + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: short\nbody with a "), 'feat: short', + 'a complete subject line stays measurable even when the capture truncates later'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'"), "$(cat <<'EOF'", + 'an opener with no body at all is likewise unresolvable'); + }); + + test('BLOCKER (round 3): text after the terminator is part of the real message', () => { + // `-m "$(cat <<'EOF'\nfeat: ok\nEOF\n) <200 a's>"` expands to ONE long + // subject; discarding the tail measured a PREFIX (8 chars vs 200+) and + // dodged COMMIT_SUBJECT_TOO_LONG — the truncation-guard class from the + // other side of the terminator (review of #3816, round 3). Only the + // canonical single closing-paren line may follow the terminator; anything + // else falls back to the opener and the format gate. + assert.strictEqual(resolveCommitSubject(`$(cat <<'EOF'\nfeat: ok\nEOF\n) ${'a'.repeat(200)}`), + "$(cat <<'EOF'", 'a substitution composed with more text cannot have its body trusted'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: ok\nEOF\n)$(printf x)"), + "$(cat <<'EOF'", 'a second substitution after the close is the same composition'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nEOF\n)feat: sneaky"), + "$(cat <<'EOF'", 'text glued straight onto the closing paren too'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: ok\nEOF"), + "$(cat <<'EOF'", 'a terminator with NO closing line at all is not the canonical shape either'); + // the canonical tail still resolves — including an indented or space-padded close + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: ok\nEOF\n)"), 'feat: ok'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\nfeat: ok\nEOF\n\t) "), 'feat: ok'); + }); + + test('MINOR 1: leading blank body lines are skipped, as git does', () => { + // git's default cleanup=whitespace strips leading blank lines, so the real + // subject is the first NON-empty line. Taking lines[1] blindly returned '' + // and falsely blocked a conforming commit — the defect class #3802 reports. + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\n\nfeat: after blank\nEOF\n)"), + 'feat: after blank'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\n\n\n \nfeat: after several\nEOF\n)"), + 'feat: after several'); + }); + + test('a backslash-escaped delimiter is the same delimiter', () => { + // `<<\\D` suppresses expansion exactly as `<<'D'` does, so it stays + // resolvable. This is the row that makes the bare-delimiter guard below a + // real distinction rather than a blanket refusal: the two spellings differ + // by one character and by whether bash expands the body. + assert.strictEqual(resolveCommitSubject('$(cat <<\\EOF\nfix: backslash tag\nEOF\n)'), + 'fix: backslash tag'); + }); + + test('BLOCKER (round 4): a BARE delimiter is not resolved — bash expands that body', () => { + // Review of #3816, round 4. Only `<<'D'`, `<<"D"` and `<<\\D` suppress + // expansion. With a bare `< git gets `feat:`) and the length gate + // (`feat: ${LONG}` -> git gets any length). Falling back to the opener line + // is the same fail-closed rule the metacharacter, truncation and + // post-terminator guards follow. + // + // Non-vacuous: every body below is conforming, so a resolver that still + // read the body returns the body and these fail. + assert.strictEqual(resolveCommitSubject(sub('< { + const word = quote === '\\' ? `\\${delim}` : quote ? `${quote}${delim}${quote}` : delim; + const opener = `$(${catPath} <<${dash ? '-' : ''}${spaced ? ' ' : ''}${word}`; + const emitted = [...body.map((l) => (dash ? `\t${l}` : l))]; + if (terminated) emitted.push(dash ? `\t${delim}` : delim, ')'); + // GENERATION-TIME oracle for the one result the derivation check cannot + // classify by membership: ''. Computed from what the generator KNOWS it + // built — never by re-running resolver logic — so a resolver degrading to + // '' anywhere it should not fails the property (Codex review of #3816, + // rounds 1+2). '' is legitimate exactly when the FIRST reachable + // terminator is followed by the one canonical closing-paren line (the + // round-3 post-terminator guard: any other tail must fall back to the + // opener, never to '') and every scanned line before that terminator is + // ASCII-blank. A body line that reads as the delimiter after <<- tab + // stripping terminates early, and whatever follows it is its tail. + const seen = emitted.map((l) => (dash ? l.replace(/^\t+/, '') : l)); + const stop = seen.indexOf(delim); + const tail = stop === -1 ? null : seen.slice(stop + 1); + const canonicalTail = tail !== null && tail.length === 1 && /^[ \t]*\)[ \t]*$/.test(tail[0]); + const expectEmpty = canonicalTail + && seen.slice(0, stop).every((l) => /^[ \t]*$/.test(l)); + return { text: [opener, ...emitted].join(eol), expectEmpty }; + }); + const messageArb = fc.oneof( + { weight: 3, arbitrary: heredocArb }, + // plain single- and multi-line messages: the subject is the first line + // verbatim, so '' is legitimate only when the first line IS ''. + fc.string({ maxLength: 400 }).map((s) => ({ text: s, expectEmpty: s.split(/\r?\n/)[0] === '' })), + // multi-line plain messages — the old generator never produced a newline + fc.array(bodyLineArb, { minLength: 1, maxLength: 4 }) + .map((ls) => ({ text: ls.join('\n'), expectEmpty: ls[0] === '' })), + ); + const firstLineOf = (input) => String(input).split(/\r?\n/)[0]; + // Floor for the resolved-input count across the seeded corpus. Deliberately + // far below the ~60% heredoc weighting so generator drift cannot flake it, + // while still failing loudly if the corpus stops reaching the parser — the + // exact vacuity Major 1 caught. + const MIN_RESOLVED = 20; + + test('property: total — never throws, always returns a string', () => { + // Totality is a SECURITY property here, not tidiness: this runs inside a + // PreToolUse hook whose caller treats a failed extraction as "nothing to + // validate", so an exception fails OPEN. Backed by a corpus that reaches + // the parser, which is what makes the claim about the PARSER and not about + // fc.string pass-through. + let resolved = 0; + fc.assert(fc.property(messageArb, (m) => { + const out = resolveCommitSubject(m.text); + assert.strictEqual(typeof out, 'string'); + if (out !== firstLineOf(m.text)) resolved += 1; + })); + assert.ok(resolved >= MIN_RESOLVED, + `only ${resolved} corpus inputs were actually resolved past the first line — the property is ` + + 'running on inputs that never reach the parser again (review of #3816, Major 1)'); + for (const odd of [null, undefined, '', '\n', '\n\n\n', '\r\n', '$(cat <<', '$(cat <<-']) { + assert.strictEqual(typeof resolveCommitSubject(odd), 'string', JSON.stringify(odd)); + } + }); + + test('property: idempotent — resolving a resolved subject changes nothing', () => { + let resolved = 0; + fc.assert(fc.property(messageArb, (m) => { + const once = resolveCommitSubject(m.text); + assert.strictEqual(resolveCommitSubject(once), once); + if (once !== firstLineOf(m.text)) resolved += 1; + })); + assert.ok(resolved >= MIN_RESOLVED, + `only ${resolved} corpus inputs were actually resolved — vacuous corpus (review of #3816)`); + }); + + test('property: the result is a single line derived from an input line by git\'s own strips', () => { + // The subject is a LINE, never a synthesised string: whatever comes back + // must be one of the input's own lines, modulo exactly the transformations + // git itself performs — `<<-` leading-tab stripping and cleanup=whitespace + // trailing-whitespace stripping. A resolver that concatenated lines or + // trimmed anything MORE than that would fail this. The one result + // membership cannot classify — '' — is judged by the GENERATOR's own + // metadata (`expectEmpty`, computed from what it built, not from resolver + // logic), so a resolver conditionally degrading to '' fails loudly (Codex + // review of #3816, rounds 1+2). + let resolved = 0; + fc.assert(fc.property(messageArb, (m) => { + const out = resolveCommitSubject(m.text); + assert.ok(!/[\n\r]/.test(out), 'a subject is one line'); + const lines = String(m.text).split(/\r?\n/); + const derivations = (l) => { + const untabbed = l.replace(/^\t+/, ''); + return [l, untabbed, untabbed.replace(/[ \t]+$/, '')]; + }; + if (out === '') { + assert.ok(m.expectEmpty, + `resolved to '' for an input the generator did NOT build as an empty message: ` + + JSON.stringify(m.text)); + } else { + assert.ok(lines.some((l) => derivations(l).includes(out)), + `result ${JSON.stringify(out)} is not derived from any line of the input`); + } + if (out !== firstLineOf(m.text)) resolved += 1; + })); + assert.ok(resolved >= MIN_RESOLVED, + `only ${resolved} corpus inputs were actually resolved — vacuous corpus (review of #3816)`); + }); + + test('MAJOR 2: trailing whitespace is stripped, as git cleanup=whitespace does', () => { + // git strips whitespace at BOTH ends of the line, not just leading blank + // lines. Measuring the raw line rejected a body of `feat: ` + 66 x's + three + // spaces as 75 chars when git's actual subject is a conforming 72 — a + // still-blocked conforming commit, the defect #3802 reports (review of #3816). + const subject72 = `feat: ${'x'.repeat(66)}`; + assert.strictEqual(subject72.length, 72, 'fixture built wrong'); + assert.strictEqual(resolveCommitSubject(sub("<<'EOF'", `${subject72} `, 'EOF')), subject72); + assert.strictEqual(resolveCommitSubject(sub("<<'EOF'", 'feat: tab tail\t \t', 'EOF')), + 'feat: tab tail', 'tabs are trailing whitespace too'); + }); + + test('MINOR 3: CRLF bodies resolve identically to LF bodies', () => { + // split('\n') left \r on every body line, so the delimiter never matched on + // CRLF input: the truncation guard was inert, an empty CRLF message resolved + // to "EOF\r" instead of '', and a real 72-char subject measured 73 + // (review of #3816). + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\r\nfeat: crlf subject\r\nEOF\r\n)"), + 'feat: crlf subject'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\r\nEOF\r\n)"), '', + 'an empty CRLF message is EMPTY — it used to resolve to the terminator plus \\r'); + assert.strictEqual(resolveCommitSubject("$(cat <<'EOF'\r\nfeat: aaaa"), "$(cat <<'EOF'", + 'the truncation guard must be live on CRLF input, not defeated by an unmatchable delimiter'); + }); + + test('ordinary messages pass through as their first line', () => { + assert.strictEqual(resolveCommitSubject('feat(auth): add login flow'), 'feat(auth): add login flow'); + assert.strictEqual(resolveCommitSubject('feat: subject\n\nBody paragraph.'), 'feat: subject'); + assert.strictEqual(resolveCommitSubject(''), ''); + assert.strictEqual(resolveCommitSubject(null), ''); + assert.strictEqual(resolveCommitSubject(undefined), ''); + }); +}); + // ── extractBranchArgument (#3212 Phase 3, #3414) ───────────────────────────── // New capability on the shared scanner (design doc §1.2) — not a migration of // existing duplicated logic; no existing consumer wired to it this phase.