From fe64704acea5246ad816a89fefa8a0fbb18ad7de Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 18 Aug 2026 00:25:32 -0400 Subject: [PATCH] enhance(#3588): add an opt-in commit_docs pre-commit hook (#3609) * feat(#3588): add an opt-in commit_docs pre-commit hook Final phase of epic #2292, scope narrowed to opt-in by maintainer decision: default-on installation and the bin/install.js wiring it would have required are explicitly out of scope. Enabling is an explicit verb call. The hook is written to the repo's real hooks dir resolved via git rev-parse --git-path hooks, so a linked worktree or submodule whose .git is a FILE works rather than getting a literal .git/hooks path. It refuses rather than overwrite a foreign pre-commit, refuses to delete one it did not write, and refuses outright when core.hooksPath is already set -- a written-but-ignored hook is worse than a refusal. Ownership is detected by marker presence, not byte-equality, so a user who appends a line does not make it unrecognizable. Deliberately NOT included: teaching cmdCheckCommit the per-phase commit_docs tier. #3587 was still unmerged when this landed, and implementing precedence against helpers that did not yet exist would have meant a second copy of the resolution chain -- the divergence class this epic has spent three phases fighting. That follows as its own change now that #3587 is on next. The ordering constraint is recorded in the design doc: this must not merge before #3587, or the hook would block a commit cmdCommit itself allows. * fix(#3588): teach the commit_docs guard the per-phase tier and -z paths Part 1, deferred until #3587 merged. cmdCheckCommit read only project-level commit_docs, so once #3587 landed, a phase with phase_commit_docs true under project false was ALLOWED by query commit and BLOCKED by this guard -- and the hook shipped in this same branch shells out to it. It now derives the staged phase via the single-owner detectPhaseNumberFromFiles and resolves through #3587's own resolveCommitDocsPolicy rather than a second precedence copy. Also fixes a proven false negative in the harm direction. git diff --cached --name-only C-style-quotes any path with non-ASCII or special characters, so a staged .planning/cafe.md was emitted as a quoted string, failed startsWith('.planning/'), and slipped past the guard entirely under commit_docs:false. Reading with -z and splitting on NUL removes the quoting at the source. The f.startsWith('.planning\\') branch was dead code under that read -- git emits /-separated paths on every platform -- and is removed rather than left implying coverage it never provided. The earlier C7 test pinned the buggy behavior as intended; it now asserts the file is detected and the commit refused. Self-caught: the commit-docs-guard verb was wired into the routers by this branch's earlier pass but missing from the top-level help listing. * test(#3588): replace try/finally with t.after, add negative-routing cases Standards review findings. CONTRIBUTING bans try/finally inside a test body outright -- it masks failures -- and B8 used one for worktree cleanup. Now t.after(), assertions unchanged. The new commit-docs-guard command family had zero negative-routing coverage, which CONTRIBUTING requires for any change to command dispatch. B11-B15 cover no subcommand, unknown, empty string, whitespace-only and a flag-shaped value, each asserting non-zero exit, a structured error, no stack trace, and -- the one that matters for a command that writes into a user's repo -- that NO hook is written in any of them. Those tests were verified to fail when routeCommitDocsGuard's else-branch is neutered, so they exercise the routing guard rather than any convenient error path. Also made two error() calls' control flow explicit with a return; they were safe only because error() is typed never two files away. * chore(#3588): backfill changeset pr number to 3609 * test(#3588): skip Windows-unrepresentable fixtures on win32 CI's Windows shards caught two of my own tests: fixtures whose filenames contain a quote and a backslash. Both are illegal on Windows -- backslash is the path separator, quote is invalid on NTFS -- so fixture creation failed before any assertion ran. Test-portability defect, not a production one. Those inputs cannot exist on that platform, so the guard has nothing to detect there. Both now check process.platform FIRST, before any fs or git call, and use t.skip() rather than a bare return -- a bare return registers as a PASS and would hide the gap it is meant to record. Each carries a comment saying the input is unrepresentable rather than unverified, so nobody later re-enables it. No padding added: the cafe.md case already exercises git's C-quoting path on every platform, since non-ASCII names are legal on NTFS. This is exactly the coverage the Linux-only remote matrix cannot provide, which the PR body already stated -- CI's Windows shards are what caught it. --------- Co-authored-by: sim --- .changeset/clever-bears-wander.md | 5 + CONTEXT.md | 2 +- docs/CONFIGURATION.md | 27 ++ docs/how-to/keep-planning-docs-private.md | 47 ++ gsd-core/bin/gsd-tools.cjs | 17 +- src/command-aliases.cts | 14 + src/commands.cts | 243 +++++++++- src/io.cts | 4 + tests/commands.test.cjs | 524 ++++++++++++++++++++++ 9 files changed, 864 insertions(+), 19 deletions(-) create mode 100644 .changeset/clever-bears-wander.md diff --git a/.changeset/clever-bears-wander.md b/.changeset/clever-bears-wander.md new file mode 100644 index 000000000..c86499a73 --- /dev/null +++ b/.changeset/clever-bears-wander.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3609 +--- +**Opt-in `.git/hooks/pre-commit` guard for `commit_docs`** — `gsd-tools commit-docs-guard enable`/`disable` writes (or removes) a pre-commit hook that shells out to the existing `check-commit` verb, refusing a commit that stages `.planning/` files while `commit_docs` resolves to `false`. Closes the one bypass earlier phases of epic #2292 could not reach: a plain `git add -A && git commit` run by hand or by a script outside GSD's own tooling. Fully opt-in by maintainer narrowing — no install path wires it in by default (regression-locked by `tests/commands.test.cjs`'s E2 row); `enable` refuses rather than overwrites an existing foreign `pre-commit` hook, refuses when `core.hooksPath` would make the written hook inert, and resolves the real hooks directory via `git rev-parse --git-path hooks` so a linked worktree or submodule (where `.git` is a file) is handled correctly rather than assuming a literal `.git/hooks` path. The hook is identified by a stable `# gsd-core:commit-docs-guard` marker line, checked by presence rather than byte-equality. (#3588) diff --git a/CONTEXT.md b/CONTEXT.md index 932f1cfff..b847c8491 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -224,7 +224,7 @@ Module owning project configuration loading: reads `.planning/config.json`, merg ### Planning Commit Gate (`commit_docs`) The seam deciding whether `.planning/` artifacts reach git. `commit_docs` resolves in the Config Loader Module (`loadConfigResolved`) through an ordered chain — an explicit value (top-level `commit_docs` or its `planning.commit_docs` alias) wins; absent that, `isGitIgnored(cwd, '.planning/')` auto-resolves it to `false`; otherwise the manifest default (`true`) applies. `cmdCommit` (Command Module) is the ONLY sanctioned writer: it returns the typed skip envelope `{ committed: false, skipped: true, hash: null, reason }` with `reason ∈ { 'skipped_commit_docs_false', 'skipped_gitignored', 'skipped_commit_docs_phase_false' }` rather than erroring, and `skipped: true` is explicit so agent prompts match a first-class success signal instead of inferring a skip from a missing `committed` and improvising a raw-git fallback (#3678). Those `reason` strings are a de facto public contract — `agents/gsd-executor.md` pattern-matches on them — so they are additive-only. -**Per-phase override (#3587, epic #2292 Phase 3).** A NEW tier resolves ABOVE the chain above, entirely inside `cmdCommit` (`src/commands.cts`'s `resolveCommitDocsPolicy`/`resolvePhaseCommitDocsOverride`) — deliberately NOT inside `loadConfigResolved`, which has no phase context and is called by nearly every command. The dynamic config key `phase_commit_docs.` (a `{ "": boolean }` map, registered in `config-schema.manifest.json`'s `dynamicKeyPatterns` and threaded through `config-loader.cts`'s `_baseConfig` projection the same way `agent_skills` is — a dynamic key absent from that hand-maintained allowlist is silently dropped on read, the exact failure mode `features.` demonstrates today) lets a tech lead commit one phase's artifacts while the project-wide `commit_docs` stays `false` (or the reverse). The phase being committed is resolved via the PRE-EXISTING `detectPhaseNumberFromFiles(files)` (the same #2539-hardened, project-code-aware derivation `branching_strategy` already used one branch below) and normalized through `normalizePhaseName` on both sides of the comparison, so `3`/`03`/`PROJ-03` hit one entry and a value scoped to a DIFFERENT phase never leaks. A non-boolean stored value (`"true"`, `1`, `null`) is never coerced — it falls through to the pre-existing chain untouched. When this tier suppresses a commit, the envelope's reason is `skipped_commit_docs_phase_false` — deliberately distinct from `skipped_commit_docs_false`, so a per-phase suppression is never reported as "your project setting is false" when it is actually `true`. **AC4 (byte-identical when unset):** with no `phase_commit_docs` key, this tier is a no-op and the three-tier chain above resolves exactly as before — pinned by the `folded:phase-commit-docs` block's C1-C5 in `tests/commit-docs-bypass.test.cjs`. The `phase_commit_docs.` grammar is a hand-copy of the canonical `PHASE_NUMBER_TOKEN_SOURCE` (`src/phase-id.cts`, #2128) into the hand-maintained schema manifest — pinned against drift by that same block's describe 'E' in `tests/commit-docs-bypass.test.cjs` (behavioral, over a shared shape list, per CLAUDE.md's Generative Fix Divergence class). **The two reasons are ordered, not peers, and `skipped_gitignored` is near-unreachable in a real project** (measured #3585): `cmdCommit` tests resolved `commit_docs` FIRST, and whenever `.planning/config.json` exists — which it does in every initialized project — the loader's gitignore auto-detect has already resolved that value to `false`, so the first branch returns `skipped_commit_docs_false` and the `isGitIgnored` branch below it is never reached. `skipped_gitignored` fires only when `config.json` is absent entirely, so the loader falls back to the `true` default and `cmdCommit`'s own check is what fires. A gitignore-driven skip therefore reports the config-driven reason; both members are behaviorally pinned by `tests/commit-docs-bypass.test.cjs` (B1-B3 and G1) so a rename fails loudly, but the reason a user sees does not distinguish *which* input suppressed the commit. **The gate is bypassable only from OUTSIDE the code**: a workflow step that types `git add` into its own shell reaches the index without passing through `cmdCommit`, and no code change can intercept that. Two guards therefore enforce it as text rather than at runtime: `tests/commit-files-pathspec.test.cjs` (#2269) requires every shipped `commit` invocation to declare `--files`, and `tests/commit-docs-bypass.test.cjs` (#1783, made repo-wide by #3585) requires every shipped `git add` able to reach `.planning/` to sit inside an **executable** `commit_docs` check — a markdown prose conditional ("**If `commit_docs` is true:**") is not a guard, because the bash block below it runs regardless. Both consume one shell tokenizer (`tests/helpers/shipped-command-scan.cjs`) and one exemption marker (`# gsd-scan-ignore: #NNN`, reason must cite a tracking ref per ADR-456). Guard state does not cross a fenced-block boundary: each fenced block is its own shell, so a guard opened in one block does not protect a `git add` in the next. Known limit: `.gitignore` has no effect on files git already TRACKS, so a project that committed `.planning/` before ignoring it keeps staging those paths — see `gsd-core/references/planning-config.md` and #3586. +**Per-phase override (#3587, epic #2292 Phase 3).** A NEW tier resolves ABOVE the chain above, entirely inside `cmdCommit` (`src/commands.cts`'s `resolveCommitDocsPolicy`/`resolvePhaseCommitDocsOverride`) — deliberately NOT inside `loadConfigResolved`, which has no phase context and is called by nearly every command. The dynamic config key `phase_commit_docs.` (a `{ "": boolean }` map, registered in `config-schema.manifest.json`'s `dynamicKeyPatterns` and threaded through `config-loader.cts`'s `_baseConfig` projection the same way `agent_skills` is — a dynamic key absent from that hand-maintained allowlist is silently dropped on read, the exact failure mode `features.` demonstrates today) lets a tech lead commit one phase's artifacts while the project-wide `commit_docs` stays `false` (or the reverse). The phase being committed is resolved via the PRE-EXISTING `detectPhaseNumberFromFiles(files)` (the same #2539-hardened, project-code-aware derivation `branching_strategy` already used one branch below) and normalized through `normalizePhaseName` on both sides of the comparison, so `3`/`03`/`PROJ-03` hit one entry and a value scoped to a DIFFERENT phase never leaks. A non-boolean stored value (`"true"`, `1`, `null`) is never coerced — it falls through to the pre-existing chain untouched. When this tier suppresses a commit, the envelope's reason is `skipped_commit_docs_phase_false` — deliberately distinct from `skipped_commit_docs_false`, so a per-phase suppression is never reported as "your project setting is false" when it is actually `true`. **AC4 (byte-identical when unset):** with no `phase_commit_docs` key, this tier is a no-op and the three-tier chain above resolves exactly as before — pinned by the `folded:phase-commit-docs` block's C1-C5 in `tests/commit-docs-bypass.test.cjs`. The `phase_commit_docs.` grammar is a hand-copy of the canonical `PHASE_NUMBER_TOKEN_SOURCE` (`src/phase-id.cts`, #2128) into the hand-maintained schema manifest — pinned against drift by that same block's describe 'E' in `tests/commit-docs-bypass.test.cjs` (behavioral, over a shared shape list, per CLAUDE.md's Generative Fix Divergence class). **The two reasons are ordered, not peers, and `skipped_gitignored` is near-unreachable in a real project** (measured #3585): `cmdCommit` tests resolved `commit_docs` FIRST, and whenever `.planning/config.json` exists — which it does in every initialized project — the loader's gitignore auto-detect has already resolved that value to `false`, so the first branch returns `skipped_commit_docs_false` and the `isGitIgnored` branch below it is never reached. `skipped_gitignored` fires only when `config.json` is absent entirely, so the loader falls back to the `true` default and `cmdCommit`'s own check is what fires. A gitignore-driven skip therefore reports the config-driven reason; both members are behaviorally pinned by `tests/commit-docs-bypass.test.cjs` (B1-B3 and G1) so a rename fails loudly, but the reason a user sees does not distinguish *which* input suppressed the commit. **The gate is bypassable only from OUTSIDE the code**: a workflow step that types `git add` into its own shell reaches the index without passing through `cmdCommit`, and no code change can intercept that. Two guards therefore enforce it as text rather than at runtime: `tests/commit-files-pathspec.test.cjs` (#2269) requires every shipped `commit` invocation to declare `--files`, and `tests/commit-docs-bypass.test.cjs` (#1783, made repo-wide by #3585) requires every shipped `git add` able to reach `.planning/` to sit inside an **executable** `commit_docs` check — a markdown prose conditional ("**If `commit_docs` is true:**") is not a guard, because the bash block below it runs regardless. Both consume one shell tokenizer (`tests/helpers/shipped-command-scan.cjs`) and one exemption marker (`# gsd-scan-ignore: #NNN`, reason must cite a tracking ref per ADR-456). Guard state does not cross a fenced-block boundary: each fenced block is its own shell, so a guard opened in one block does not protect a `git add` in the next. Known limit: `.gitignore` has no effect on files git already TRACKS, so a project that committed `.planning/` before ignoring it keeps staging those paths — see `gsd-core/references/planning-config.md` and #3586. **`cmdCheckCommit`** (Command Module, verb `check-commit`) is a SEPARATE reader of the same `commit_docs` value, for callers outside GSD's own commit path: it inspects the staged set directly and refuses (non-zero exit) when `commit_docs` is `false` and any staged path is under `.planning/`; otherwise it allows. `gsd-tools commit-docs-guard enable`/`disable` (#3588) is the opt-in installer for a `.git/hooks/pre-commit` hook that shells out to exactly this verb, closing the one bypass the text-scan guards above cannot reach — a human or script running a bare `git add -A && git commit` in their OWN shell, outside any GSD-shipped workflow. The hook is identified by a `# gsd-core:commit-docs-guard` marker line (presence-checked, not byte-equality), is written only on explicit request (no install path wires it by default — locked by `tests/commands.test.cjs`'s E2 row), refuses rather than overwrite or delete a foreign `pre-commit`, resolves the real hooks dir via `git rev-parse --git-path hooks` so a linked worktree or submodule whose `.git` is a FILE works, and refuses outright when `core.hooksPath` is already set, because a written-but-ignored hook is worse than a refusal. ### Model Catalog Module Leaf module owning the **static** model tables and the closed vocabularies derived from them — the tier/runtime/provider enums (`VALID_TIERS`, `VALID_AGENT_TIERS`, `KNOWN_RUNTIMES`, `KNOWN_PROVIDERS`, `RUNTIMES_WITH_REASONING_EFFORT`, `RUNTIMES_WITH_FAST_MODE`, `ADAPTIVE_TIER_VALUES`), the alias and profile maps (`MODEL_ALIAS_MAP`, `RUNTIME_PROFILE_MAP`, `PROVIDER_PRESETS`), effort rendering (`renderEffortForRuntime`), and the agent→model projections (`getAgentToModelMapForProfile`, `formatAgentToModelMapAsTable`). A **genuine leaf**: it imports `node:path` and its own `model-catalog.json` and nothing else, which is what makes it the correct home for anything several unrelated surfaces must agree on. Model *ids* live in `model-catalog.json`, never inline — changing one means regenerating goldens (`UPDATE_GOLDEN`). Also owns the **Anthropic-flavored-model rule** (#3241, ADR-2313): `CLAUDE_AGENT_ALIASES` (the frozen four-alias set `opus`/`sonnet`/`haiku`/`fable`) and `isAnthropicFlavoredModel(model)`, which is true for a bare tier alias or for any `claude-*` id in any provider namespacing (`anthropic/claude-*`, `us.anthropic.claude-*`); no OpenAI/Codex model id contains "claude", so the case-insensitive substring test is safe and exhaustive. It was **moved down here from the Model Resolver Module** rather than shared from there, because the Codex posture check (`agent-install-check`, epic #2313 Phase 2) and the Codex `.toml` sync (`commands`, Phase 3) both need the rule and neither may take the `config-loader` dependency `model-resolver` would have brought; `model-resolver` re-exports it for back-compat and a parity test fails if the two ever fork. _Avoid_: "the model list" (ambiguous between the catalog JSON and the derived enums). See Model Resolver Module, ADR-2313, and ADR-0003. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index c79f64d49..3e7fc09ba 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -599,6 +599,33 @@ The prompt injection guard hook (`gsd-prompt-guard.js`) is always active and can When `planning.commit_docs` is `false` and `.planning/` is listed in `.gitignore`, GSD treats planning artifacts as local-only. `planning.search_gitignored: true` ensures broad searches still include the `.planning/` directory in this configuration. See [Keep planning docs out of a shared repo](how-to/keep-planning-docs-private.md) for the full setup, including untracking files git is already tracking. +### `commit_docs` Pre-Commit Guard (opt-in) + +`planning.commit_docs: false` only stops GSD's own `gsd-tools commit`/`gsd-tools state` +write path from committing `.planning/`. It does **not** stop a plain `git add -A` + +`git commit` run by hand, or by a script outside GSD's own tooling, from staging and +committing `.planning/` anyway. + +`gsd-tools commit-docs-guard enable` closes that gap by writing a `.git/hooks/pre-commit` +hook into the **current repository** that refuses any commit staging `.planning/` files +while `commit_docs` resolves to `false`. Resolution goes through the same +[per-phase precedence chain](#per-phase-override-phase_commit_docs) `gsd-tools commit`/`query commit` +use — a `phase_commit_docs.` override for the staged phase is honored here too, so the +hook cannot contradict them. It is entirely opt-in — no install path wires it +automatically: + +```bash +gsd-tools commit-docs-guard enable # write the hook (refuses to clobber an existing pre-commit hook) +gsd-tools commit-docs-guard disable # remove it (refuses to remove a hook GSD didn't write) +``` + +The hook is identified by a stable `# gsd-core:commit-docs-guard` marker line, so `enable`/ +`disable` detect it by presence of that marker rather than by byte-for-byte content — editing +the file afterward does not make it unrecognizable. `enable` refuses (rather than silently +writing an inert file) when `core.hooksPath` is already configured, since a hook written to +`.git/hooks/pre-commit` would never run in that case; wire the guard into the configured hooks +path by hand instead. See [Keep planning docs out of a shared repo](how-to/keep-planning-docs-private.md#pre-commit-guard-hook-optional) for the full walkthrough, including the linked-worktree case. + --- ## Agent Skills Injection diff --git a/docs/how-to/keep-planning-docs-private.md b/docs/how-to/keep-planning-docs-private.md index c51f9474d..ab9756d7d 100644 --- a/docs/how-to/keep-planning-docs-private.md +++ b/docs/how-to/keep-planning-docs-private.md @@ -134,8 +134,55 @@ Full precedence order (highest wins): `phase_commit_docs.` → explici [Configuration reference — per-phase override](../CONFIGURATION.md#per-phase-override-phase_commit_docs) for the complete rules, including how a non-boolean value is handled. +## Pre-commit guard hook (optional) + +Steps 1-4 stop **GSD's own** commit path from writing `.planning/`. They do not stop a plain +`git add -A` + `git commit` — run by hand, by a teammate, or by a script outside GSD's own +tooling — from staging and committing `.planning/` anyway. `gsd-tools commit-docs-guard enable` +closes that specific gap by installing a `.git/hooks/pre-commit` hook that refuses any commit +staging `.planning/` files while `commit_docs` resolves to `false`. Resolution honors the full +precedence chain above — including a `phase_commit_docs.` override for the phase the +staged `.planning/` files belong to — the same resolution `gsd-tools commit`/`query commit` uses, +so the hook never contradicts them. + +This is opt-in only — no GSD install path wires it in for you: + +```bash +gsd-tools commit-docs-guard enable +``` + +If a commit would violate `commit_docs`, the hook blocks it and names the staged files and the +`git reset` command to unstage them, matching `gsd-tools check-commit`'s own message. Remove it +with: + +```bash +gsd-tools commit-docs-guard disable +``` + +**Enable refuses rather than guesses** in three situations, each reported with a reason: + +- **An existing `pre-commit` hook you didn't get from GSD.** The file is left byte-for-byte + unchanged; wire the guard into it by hand (`gsd-tools check-commit --raw` is the check to add). +- **`core.hooksPath` is already configured.** A hook written to `.git/hooks/pre-commit` would + never run in that case, so nothing is written; add the same check to whatever hook lives at the + configured path instead. +- **The current directory is not a git repository.** + +`enable`/`disable` are idempotent and safe to script: a second `enable` is a no-op that reports +success rather than duplicating content, and `disable` on a repo with no hook installed succeeds +rather than erroring. The hook is identified by a stable `# gsd-core:commit-docs-guard` marker +line inside the file, checked by presence rather than exact content — appending your own line to +the installed hook afterward does not make GSD stop recognizing it as its own. In a linked +worktree or submodule (where `.git` is a file, not a directory), `enable` resolves the real, +shared hooks directory via git itself rather than assuming a literal `.git/hooks` path. + +Windows note: the hook runs under Git Bash, same as any other git hook. GSD's own remote test +matrix is Linux-only, so this specific behavior is verified on Linux/macOS plus code review, not +by an automated Windows run. + ## Related - [Configuration reference — `planning.commit_docs`](../CONFIGURATION.md#planning-settings) - [Configuration reference — auto-detection and the tracked-files caveat](../CONFIGURATION.md#auto-detection) - [Configuration reference — per-phase override](../CONFIGURATION.md#per-phase-override-phase_commit_docs) +- [Configuration reference — the pre-commit guard hook](../CONFIGURATION.md#commit_docs-pre-commit-guard-opt-in) diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index a8d82984f..a939c072d 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -20,6 +20,9 @@ * resolve-model Get model for agent based on profile * find-phase Find phase directory by number * commit [--files f1 f2] [--no-verify] Commit planning docs + * commit-docs-guard enable|disable Opt-in .git/hooks/pre-commit guard + * that refuses a commit staging + * .planning/ when commit_docs is false * commit-to-subrepo --files f1 f2 Route commits to sub-repos * verify-summary Verify a SUMMARY.md file * generate-slug Convert text to URL-safe slug @@ -930,6 +933,17 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load commands.cmdCheckCommit(cwd, raw); } + function routeCommitDocsGuard({ args, cwd, raw, error }) { + const subcommand = args[1]; + if (subcommand === 'enable') { + commands.cmdCommitDocsGuardEnable(cwd, raw); + } else if (subcommand === 'disable') { + commands.cmdCommitDocsGuardDisable(cwd, raw); + } else { + error('Unknown commit-docs-guard subcommand. Available: enable, disable', ERROR_REASON.SDK_UNKNOWN_COMMAND); + } + } + function routeCommitToSubrepo({ args, cwd, raw, error }) { const message = args[1]; const filesIndex = args.indexOf('--files'); @@ -3678,6 +3692,7 @@ const HOST_COMMAND_ROUTERS = { 'find-phase': routeFindPhase, 'commit': routeCommit, 'check-commit': routeCheckCommit, + 'commit-docs-guard': routeCommitDocsGuard, 'commit-to-subrepo': routeCommitToSubrepo, 'pr-subrepo': routePrSubrepo, 'verify-summary': routeVerifySummary, @@ -3945,7 +3960,7 @@ function runWithTimeout(argv) { // independently hand-maintained sites and nothing previously caught them // drifting apart when a query command was added to only one or two. const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--ws ] [--json-errors]\n' + - 'Commands: agent, agent-skills, assumption-delta, audit-open, audit-uat, check, check-commit, commit, commit-to-subrepo, pr-subrepo, ' + + 'Commands: agent, agent-skills, assumption-delta, audit-open, audit-uat, check, check-commit, commit, commit-docs-guard, commit-to-subrepo, pr-subrepo, ' + 'config-ensure-section, config-get, config-new-project, config-path, config-set, migrate-config, normalize-test-command, ' + 'context-predicates, current-timestamp, detect-custom-files, docs-init, drift-guard, effort, extract-messages, find-phase, ' + 'from-gsd2, frontmatter, gap-analysis, generate-claude-md, generate-claude-profile, ' + diff --git a/src/command-aliases.cts b/src/command-aliases.cts index b236d0759..d429b970a 100644 --- a/src/command-aliases.cts +++ b/src/command-aliases.cts @@ -710,6 +710,20 @@ export const NON_FAMILY_COMMAND_ALIASES: NonFamilyCommandAlias[] = [ "aliases": [], "mutation": true }, + { + "canonical": "commit-docs-guard.disable", + "aliases": [ + "commit-docs-guard disable" + ], + "mutation": true + }, + { + "canonical": "commit-docs-guard.enable", + "aliases": [ + "commit-docs-guard enable" + ], + "mutation": true + }, { "canonical": "commit-to-subrepo", "aliases": [], diff --git a/src/commands.cts b/src/commands.cts index 5a8c180d1..477f2f3a6 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -8,11 +8,12 @@ import fs from 'node:fs'; import path from 'node:path'; +import { normalizeEol } from './text-lines.cjs'; import { execGit, platformWriteSync, platformReadSync, platformEnsureDir, isSpawnTimeout, retryRenameSync } from './shell-command-projection.cjs'; import { requireSafePath, sanitizeForDisplay } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); -const { output, error } = ioMod; +const { output, error, ERROR_REASON } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import configLoaderMod = require('./config-loader.cjs'); const { loadConfig, isGitIgnored } = configLoaderMod; @@ -2426,35 +2427,239 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { } /** - * Check whether a commit should be allowed based on commit_docs config. - * When commit_docs is false, rejects commits that stage .planning/ files. - * Intended for use as a pre-commit hook guard. + * Check whether a commit should be allowed based on the `commit_docs` + * precedence chain, INCLUDING any `phase_commit_docs.` override + * (#3587/#3601). Rejects commits that stage `.planning/` files when the + * resolved policy is false. Intended for use as a pre-commit hook guard — + * see `commit-docs-guard enable` above. + * + * The phase is derived from the STAGED `.planning/` paths via the single- + * owner `detectPhaseNumberFromFiles` (the same helper `cmdCommit` uses), and + * the policy itself is resolved via the single-owner `resolveCommitDocsPolicy` + * (also shared with `cmdCommit`) — this function never re-derives phase + * detection or precedence, so it cannot diverge from `cmdCommit`'s decision + * for the same staged tree (#3588 Part 1: this guard was previously + * phase-blind, reading only project-level `commit_docs` and directly + * contradicting `gsd-tools query commit`'s phase-aware resolution). + * + * Staged paths are read via `git diff --cached --name-only -z`, NUL- + * delimited, rather than the LF-delimited default. Without `-z`, git + * C-style-quotes (wraps in double quotes, octal-escapes) any path containing + * a non-ASCII byte, a space-adjacent special character, or a literal quote — + * `.planning/café.md` is reported as `".planning/caf\303\251.md"`, which + * does not start with `.planning/`, so the old LF-based filter silently + * missed it and allowed the commit (#3588 F2: a false negative in the harm + * direction this guard exists to prevent). `-z` disables that quoting + * entirely and NUL-terminates each path instead, so every staged path is + * read as literal, unquoted bytes and no unquoting logic is needed. */ function cmdCheckCommit(cwd: string, raw: boolean): void { const config = loadConfig(cwd); - // If commit_docs is true (or not set), allow all commits - if (config['commit_docs'] !== false) { - output({ allowed: true, reason: 'commit_docs_enabled' }, raw, 'allowed'); + const stagedResult = execGit(['diff', '--cached', '--name-only', '-z'], { cwd }); + if (stagedResult.exitCode === 0) { + const files = stagedResult.stdout.split('\0').filter(Boolean); + const planningFiles = files.filter(f => f.startsWith('.planning/')); + + if (planningFiles.length > 0) { + const policy = resolveCommitDocsPolicy( + config, + detectPhaseNumberFromFiles(planningFiles), + () => isGitIgnored(cwd, '.planning'), + ); + if (!policy.resolved) { + error( + `commit_docs is false but ${planningFiles.length} .planning/ file(s) are staged:\n` + + planningFiles.map(f => ` ${f}`).join('\n') + + `\n\nTo unstage: git reset HEAD ${planningFiles.join(' ')}` + ); + return; + } + output( + { allowed: true, reason: policy.source === 'phase' ? 'phase_commit_docs_true' : 'commit_docs_enabled' }, + raw, + 'allowed', + ); + return; + } + } + // exitCode !== 0 (no staged files / not a git repo) or no .planning/ files staged — allow + + output({ allowed: true, reason: 'no_planning_files_staged' }, raw, 'allowed'); +} + +// ─── commit-docs-guard: opt-in pre-commit hook (#3588) ───────────────────── + +/** + * Stable sentinel line identifying a `.git/hooks/pre-commit` file as ours. + * Detection is by PRESENCE of this line, not byte-equality (design "Identifying + * 'our' hook") — a user who appends a line to a GSD-written hook must not make + * it unrecognizable, and a hook lacking this line must never be overwritten or + * deleted by `commit-docs-guard enable`/`disable`. + */ +const COMMIT_DOCS_GUARD_MARKER = '# gsd-core:commit-docs-guard'; + +/** + * Locate `gsd-core/workflows/_runtime-launcher.snippet.sh` — the SAME + * gsd-tools-resolution chain every shipped workflow/agent bash block uses + * (scripts/sync-runtime-launcher.cjs) — by walking up from this module's own + * compiled location rather than a fixed literal `../..` join, so the walk + * tolerates the module living at a different depth under an alternate build + * or bundling layout (same defensive shape as + * runtime-artifact-layout.cts#findInstallSourceRoot). + */ +function findRuntimeLauncherSnippet(): string { + let dir = __dirname; + for (let i = 0; i < 8; i++) { + const candidate = path.join(dir, 'workflows', '_runtime-launcher.snippet.sh'); + if (fs.existsSync(candidate)) return candidate; + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + } + throw new Error(`commit-docs-guard: could not locate workflows/_runtime-launcher.snippet.sh from ${__dirname}`); +} + +/** + * Build the literal `.git/hooks/pre-commit` content `commit-docs-guard enable` + * writes. Reuses the canonical gsd_run resolution preamble byte-for-byte + * (read from disk, never hand-copied — see findRuntimeLauncherSnippet) so this + * hook resolves `gsd-tools` exactly the way every other shipped workflow bash + * block does, and cannot drift from it. + * + * LF-only (#3588 A2): the snippet file and every literal line here are joined + * with `\n`; platformWriteSync additionally normalizes CRLF→LF on write, so a + * CRLF shebang — which is not executable under Git Bash — cannot reach disk. + */ +function buildCommitDocsGuardHookScript(): string { + const snippetPath = findRuntimeLauncherSnippet(); + const preamble = normalizeEol(fs.readFileSync(snippetPath, 'utf8')).replace(/\n+$/, ''); + const lines = [ + '#!/usr/bin/env bash', + COMMIT_DOCS_GUARD_MARKER, + '# Refuses a commit that stages .planning/ files when `commit_docs` resolves', + '# false (honoring any per-phase override). Installed by', + '# `gsd-tools commit-docs-guard enable`; remove with', + '# `gsd-tools commit-docs-guard disable`. See', + '# docs/how-to/keep-planning-docs-private.md.', + 'set -euo pipefail', + '', + preamble, + '', + 'gsd_run check-commit --raw', + ]; + return lines.join('\n') + '\n'; +} + +interface HooksDirResolution { + ok: boolean; + dir?: string; + reason?: string; +} + +/** + * Resolve the real git hooks directory for `cwd` via `git rev-parse + * --git-path hooks` — never a literal `.git/hooks` join (#3588 row 8: a + * linked worktree or submodule's `.git` is a FILE pointing elsewhere, and + * this is the one git-native call that already resolves that correctly). + */ +function resolveCommitDocsGuardHooksDir(cwd: string): HooksDirResolution { + const gitDirResult = execGit(['rev-parse', '--git-dir'], { cwd }); + if (gitDirResult.exitCode !== 0) { + return { ok: false, reason: 'not_a_git_repo' }; + } + const hooksPathResult = execGit(['rev-parse', '--git-path', 'hooks'], { cwd }); + if (hooksPathResult.exitCode !== 0) { + return { ok: false, reason: 'not_a_git_repo' }; + } + const hooksDirRaw = hooksPathResult.stdout.trim(); + const hooksDir = path.isAbsolute(hooksDirRaw) ? hooksDirRaw : path.join(cwd, hooksDirRaw); + return { ok: true, dir: hooksDir }; +} + +/** Marker presence, not byte-equality (#3588 row B10). */ +function isCommitDocsGuardHook(content: string): boolean { + return content.includes(COMMIT_DOCS_GUARD_MARKER); +} + +/** + * `gsd-tools commit-docs-guard enable` — write `.git/hooks/pre-commit`. + * Behavior table (40-design.md rows 1-3, 8-9): refuses to clobber a foreign + * hook, refuses when `core.hooksPath` would make our own write inert, and is + * idempotent when already enabled. + */ +function cmdCommitDocsGuardEnable(cwd: string, raw: boolean): void { + const hooksDir = resolveCommitDocsGuardHooksDir(cwd); + if (!hooksDir.ok || !hooksDir.dir) { + error('not a git repository (or any of the parent directories)', ERROR_REASON.COMMIT_DOCS_GUARD_NOT_A_REPO); return; } - // commit_docs is false — check if any .planning/ files are staged - const stagedResult = execGit(['diff', '--cached', '--name-only'], { cwd }); - if (stagedResult.exitCode === 0) { - const planningFiles = stagedResult.stdout.split('\n').filter(f => f.startsWith('.planning/') || f.startsWith('.planning\\')); + // core.hooksPath already set: our .git/hooks/pre-commit would be inert — + // git would never invoke it. Silently writing an ignored file is worse + // than refusing (design row 9). + const hooksPathConfig = execGit(['config', '--get', 'core.hooksPath'], { cwd }); + if (hooksPathConfig.exitCode === 0 && hooksPathConfig.stdout.trim() !== '') { + const configuredPath = hooksPathConfig.stdout.trim(); + error( + `core.hooksPath is set to "${configuredPath}"; a hook written to ${path.join(hooksDir.dir, 'pre-commit')} ` + + `would never run. Wire commit-docs-guard into "${configuredPath}" manually, or unset core.hooksPath first.`, + ERROR_REASON.COMMIT_DOCS_GUARD_HOOKS_PATH_SET, + ); + return; + } - if (planningFiles.length > 0) { + const hookPath = path.join(hooksDir.dir, 'pre-commit'); + const existing = platformReadSync(hookPath); + if (existing !== null) { + if (!isCommitDocsGuardHook(existing)) { error( - `commit_docs is false but ${planningFiles.length} .planning/ file(s) are staged:\n` + - planningFiles.map(f => ` ${f}`).join('\n') + - `\n\nTo unstage: git reset HEAD ${planningFiles.join(' ')}` + `refusing to overwrite an existing pre-commit hook at ${hookPath} that GSD did not write. ` + + `Remove or rename it, or wire commit-docs-guard into it by hand.`, + ERROR_REASON.COMMIT_DOCS_GUARD_FOREIGN_HOOK, ); } + // Already ours — idempotent no-op (row 3). Leave any user edits intact; + // just make sure the executable bit survived. + try { fs.chmodSync(hookPath, 0o755); } catch { /* best-effort */ } + output({ enabled: true, action: 'already_enabled', path: hookPath }, raw, 'already_enabled'); + return; } - // exitCode !== 0 → no staged files or not a git repo — allow - output({ allowed: true, reason: 'no_planning_files_staged' }, raw, 'allowed'); + platformWriteSync(hookPath, buildCommitDocsGuardHookScript()); + fs.chmodSync(hookPath, 0o755); + output({ enabled: true, action: 'written', path: hookPath }, raw, 'enabled'); +} + +/** + * `gsd-tools commit-docs-guard disable` — remove `.git/hooks/pre-commit` + * ONLY when it is the hook we wrote (marker presence). Never deletes a + * foreign hook (design row 5); a missing hook is a no-op success, not an + * error (row 6). + */ +function cmdCommitDocsGuardDisable(cwd: string, raw: boolean): void { + const hooksDir = resolveCommitDocsGuardHooksDir(cwd); + if (!hooksDir.ok || !hooksDir.dir) { + error('not a git repository (or any of the parent directories)', ERROR_REASON.COMMIT_DOCS_GUARD_NOT_A_REPO); + return; + } + + const hookPath = path.join(hooksDir.dir, 'pre-commit'); + const existing = platformReadSync(hookPath); + if (existing === null) { + output({ disabled: true, action: 'noop', path: hookPath }, raw, 'noop'); + return; + } + if (!isCommitDocsGuardHook(existing)) { + error( + `refusing to remove the pre-commit hook at ${hookPath}: it does not carry the ` + + `${COMMIT_DOCS_GUARD_MARKER} marker, so GSD did not write it.`, + ERROR_REASON.COMMIT_DOCS_GUARD_FOREIGN_HOOK, + ); + } + + fs.unlinkSync(hookPath); + output({ disabled: true, action: 'removed', path: hookPath }, raw, 'disabled'); } export = { @@ -2488,5 +2693,9 @@ export = { cmdScaffold, cmdStats, cmdCheckCommit, + COMMIT_DOCS_GUARD_MARKER, + buildCommitDocsGuardHookScript, + cmdCommitDocsGuardEnable, + cmdCommitDocsGuardDisable, _wsParseRetryAfter, }; diff --git a/src/io.cts b/src/io.cts index f52aaed7c..7bbe0223b 100644 --- a/src/io.cts +++ b/src/io.cts @@ -197,6 +197,10 @@ const ERROR_REASON = Object.freeze({ GRAPHIFY_INVALID_QUERY: 'graphify_invalid_query', // hooks HOOKS_OPT_OUT: 'hooks_opt_out', + // commit-docs-guard (#3588) + COMMIT_DOCS_GUARD_NOT_A_REPO: 'commit_docs_guard_not_a_repo', + COMMIT_DOCS_GUARD_FOREIGN_HOOK: 'commit_docs_guard_foreign_hook', + COMMIT_DOCS_GUARD_HOOKS_PATH_SET: 'commit_docs_guard_hooks_path_set', // security-scan SECURITY_SCAN_FAILED: 'security_scan_failed', // generic diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 1ff64d26d..174224416 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -2647,6 +2647,530 @@ describe('check-commit command', () => { assert.ok(result.error.includes('.planning/'), 'error should mention .planning/ files'); assert.ok(result.error.includes('unstage'), 'error should suggest unstage command'); }); + + // #3588 F1: cmdCheckCommit must resolve the SAME phase_commit_docs. + // tier `gsd-tools query commit` (cmdCommit) already honors (#3587/#3601). + // Before this fix, cmdCheckCommit read only project-level `commit_docs`, so + // a phase with `phase_commit_docs.: true` under project `commit_docs: + // false` was ALLOWED by `query commit` and BLOCKED by this guard — the + // hook shipped in this same branch shells out to check-commit, so that + // contradiction was live. C4/C5 exercise both directions of the override; + // both fail against the pre-fix tree (project-level-only check). + test('C4 (#3588/#3587): project commit_docs:false + per-phase true ALLOWS the commit', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false, phase_commit_docs: { '03': true } }) + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-widgets'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '03-widgets', 'SUMMARY.md'), '# Three'); + gitOrThrow(['add', '.planning/phases/03-widgets/SUMMARY.md'], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok( + result.success, + `phase_commit_docs.03:true must allow the commit even though project commit_docs is false: ${result.error || ''}`, + ); + const output = JSON.parse(result.output); + assert.strictEqual(output.allowed, true); + }); + + test('C5 (#3588/#3587): project commit_docs:true + per-phase false BLOCKS the commit', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: true, phase_commit_docs: { '03': false } }) + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-widgets'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '03-widgets', 'SUMMARY.md'), '# Three'); + gitOrThrow(['add', '.planning/phases/03-widgets/SUMMARY.md'], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok(!result.success, 'phase_commit_docs.03:false must block the commit even though project commit_docs is true'); + assert.ok(result.error.includes('03-widgets/SUMMARY.md'), result.error); + }); + + // #3588 C6: staged paths spanning two phase directories with DIFFERENT + // phase_commit_docs values must resolve against the FIRST phase (in + // detectPhaseNumberFromFiles's staged-path order) — the same first-match + // rule cmdCommit is pinned to (see the folded #3587 `multiPhaseFilesResolves + // AgainstFirstPhase` test above). This replaces the pre-fix baseline test, + // which could only assert the phase-blind "blocks everything" behavior + // because the per-phase tier did not exist here yet. + test('C6 (#3588): staged paths spanning two phase directories resolve against the FIRST phase, matching cmdCommit', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false, phase_commit_docs: { '01': true, '02': false } }) + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-first'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-second'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '01-first', 'SUMMARY.md'), '# One'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'phases', '02-second', 'SUMMARY.md'), '# Two'); + gitOrThrow( + ['add', '.planning/phases/01-first/SUMMARY.md', '.planning/phases/02-second/SUMMARY.md'], + { cwd: tmpDir } + ); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok( + result.success, + `phase 01 (first match) resolves phase_commit_docs.01:true, so the commit must be allowed despite phase 02:false: ${result.error || ''}`, + ); + }); + + // #3588 F2: `git diff --cached --name-only` (no `-z`) C-style-quotes any + // path containing a non-ASCII byte or another special character — a staged + // `.planning/café.md` is reported as `".planning/caf\303\251.md"`, which + // does not start with `.planning/`, so the pre-fix guard MISSED it and + // allowed the commit — a false negative in the harm direction this guard + // exists to prevent. These MUST fail against the pre-fix (LF, no `-z`) + // tree and pass once `-z` + NUL-split lands. + test('F2 (#3588): a staged .planning/ file with a non-ASCII name is detected and blocked', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false }) + ); + const unicodeName = '.planning/café.md'; + fs.writeFileSync(path.join(tmpDir, unicodeName), '# State'); + gitOrThrow(['add', unicodeName], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok(!result.success, 'a staged .planning/café.md must be detected and block the commit'); + assert.ok(result.error.includes('café.md'), result.error); + }); + + test('F2 (#3588): a staged .planning/ file with a space in its name is detected and blocked', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false }) + ); + const spacedName = '.planning/with space.md'; + fs.writeFileSync(path.join(tmpDir, spacedName), '# State'); + gitOrThrow(['add', spacedName], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok(!result.success, 'a staged .planning/ file with a space in its name must be detected and block the commit'); + assert.ok(result.error.includes('with space.md'), result.error); + }); + + test('F2 (#3588): a staged .planning/ file with a quote character in its name is detected and blocked', (t) => { + // `"` is a reserved NTFS character — a file named `with"quote.md` cannot + // exist on Windows at all, so the fixture itself is unrepresentable + // there. This is not a gap in the guard's Windows behavior; it is an + // input that Windows filesystems reject outright. Do not re-enable this + // on win32 — see #3588. + if (process.platform === 'win32') { + t.skip('a `"` filename is illegal on Windows filesystems (#3588); fixture cannot be created'); + return; + } + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false }) + ); + const quotedName = '.planning/with"quote.md'; + fs.writeFileSync(path.join(tmpDir, quotedName), '# State'); + gitOrThrow(['add', quotedName], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok(!result.success, 'a staged .planning/ file with a quote character in its name must be detected and block the commit'); + assert.ok(result.error.includes('quote.md'), result.error); + }); + + // #3588 C7 (flipped): the earlier pass's C7 test pinned a synthetic + // top-level filename (`.planning\STATE.md`, backslash as a literal + // character in a single path component, not a real nested directory — git + // never uses backslash as a tree separator, on any platform) as evidence + // that `f.startsWith('.planning\\')` was unreachable, and left the assertion + // at "currently allowed" pending a fix. That branch is now removed as dead + // code (git's plumbing output is always `/`-normalized, so a real Windows + // `.planning\` path never reaches this filter as a `.planning\` + // prefix). This replaces it with the REAL analog of the same class of bug: + // a genuine `.planning/` file whose name merely CONTAINS a literal + // backslash character. Without `-z` that name is also C-style-quoted + // (`".planning/back\\slash.md"`) and missed; with `-z` it is read as raw, + // unquoted bytes and correctly detected via the plain `.planning/` prefix + // check alone — no backslash-specific branch needed. + test('C7 (#3588, flipped): a staged .planning/ file whose name contains a backslash character is detected and blocked', (t) => { + // `\` is the Windows path separator, not a legal character inside a + // single filename component — a file literally named `back\slash.md` + // cannot be created on Windows filesystems, so the fixture itself is + // unrepresentable there. This is not a gap in the guard's Windows + // behavior; it is an input Windows rejects outright. Do not re-enable + // this on win32 — see #3588. + if (process.platform === 'win32') { + t.skip('a `\\` filename is illegal on Windows filesystems (#3588); fixture cannot be created'); + return; + } + fs.writeFileSync( + path.join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ commit_docs: false }) + ); + const backslashInName = '.planning/back\\slash.md'; + fs.writeFileSync(path.join(tmpDir, backslashInName), '# State'); + gitOrThrow(['add', backslashInName], { cwd: tmpDir }); + + const result = runGsdTools('check-commit', tmpDir); + assert.ok( + !result.success, + 'a staged .planning/ file whose name contains a backslash character must be detected and block the commit', + ); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// commit-docs-guard: opt-in pre-commit hook (#3588) +// ───────────────────────────────────────────────────────────────────────────── + +describe('commit-docs-guard hook script (#3588 A1-A5)', () => { + const { createTempGitProject, TEST_ENV_BASE } = require('./helpers.cjs'); + const { runHook } = require('./helpers/process-seam.cjs'); + const REPO_ROOT = path.join(__dirname, '..'); + const HOOK_MARKER = '# gsd-core:commit-docs-guard'; + let tmpDir; + let hookPath; + + beforeEach(() => { + tmpDir = createTempGitProject(); + const enableResult = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(enableResult.success, `enable failed: ${enableResult.error}`); + hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('A1: hook script carries the gsd-core:commit-docs-guard marker', () => { + const content = fs.readFileSync(hookPath, 'utf8'); + assert.ok(content.includes(HOOK_MARKER), 'written hook must carry the marker line'); + }); + + test('A2: hook script uses LF-only line endings (boundary — Windows)', () => { + const content = fs.readFileSync(hookPath, 'utf8'); + assert.ok(!content.includes('\r'), 'a CRLF shebang is not executable under Git Bash'); + }); + + test('A3: hook file is executable after enable', () => { + if (process.platform === 'win32') return; // exec bit is not the Windows-relevant assertion + const mode = fs.statSync(hookPath).mode; + assert.ok((mode & 0o111) !== 0, 'pre-commit hook must carry the executable bit'); + }); + + test('A4: hook exits zero when the guard allows', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({ commit_docs: true })); + const result = runHook(hookPath, [], { + interpreter: 'bash', + cwd: tmpDir, + env: { ...process.env, ...TEST_ENV_BASE, RUNTIME_DIR: REPO_ROOT }, + }); + assert.strictEqual(result.exitCode, 0, `stdout=${result.stdout} stderr=${result.stderr}`); + }); + + test('A5: hook exits non-zero and names the staged files when the guard blocks', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({ commit_docs: false })); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# State'); + gitOrThrow(['add', '.planning/STATE.md'], { cwd: tmpDir }); + const result = runHook(hookPath, [], { + interpreter: 'bash', + cwd: tmpDir, + env: { ...process.env, ...TEST_ENV_BASE, RUNTIME_DIR: REPO_ROOT }, + }); + assert.notStrictEqual(result.exitCode, 0, 'hook must exit non-zero when the guard blocks'); + assert.ok(result.stderr.includes('.planning/STATE.md'), result.stderr); + }); +}); + +describe('commit-docs-guard enable/disable (#3588 B1-B15)', () => { + const { createTempGitProject } = require('./helpers.cjs'); + let tmpDir; + + afterEach(() => { + if (tmpDir) cleanup(tmpDir); + tmpDir = undefined; + }); + + test('B1: enable writes an executable hook and reports success', () => { + tmpDir = createTempGitProject(); + const result = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(result.success, result.error); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + assert.ok(fs.existsSync(hookPath)); + if (process.platform !== 'win32') { + assert.ok((fs.statSync(hookPath).mode & 0o111) !== 0); + } + }); + + test('B2: enable refuses to clobber an existing foreign pre-commit hook', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const foreignContent = '#!/bin/sh\necho foreign\n'; + fs.writeFileSync(hookPath, foreignContent); + fs.chmodSync(hookPath, 0o755); + const result = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(!result.success, 'enable must refuse to overwrite a foreign hook'); + assert.ok(result.error.includes(hookPath), result.error); + assert.strictEqual(fs.readFileSync(hookPath, 'utf8'), foreignContent, 'foreign hook must be byte-unchanged'); + }); + + test('B3: enable twice is idempotent — no duplicated content', () => { + tmpDir = createTempGitProject(); + const r1 = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(r1.success, r1.error); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const first = fs.readFileSync(hookPath, 'utf8'); + const r2 = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(r2.success, r2.error); + const second = fs.readFileSync(hookPath, 'utf8'); + assert.strictEqual(second, first, 'a second enable must not change or duplicate content'); + const markerCount = (second.match(/# gsd-core:commit-docs-guard/g) || []).length; + assert.strictEqual(markerCount, 1, 'exactly one marker line, never duplicated'); + }); + + test('B4: disable removes our hook', () => { + tmpDir = createTempGitProject(); + const enableResult = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(enableResult.success, enableResult.error); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + assert.ok(fs.existsSync(hookPath)); + const result = runGsdTools('commit-docs-guard disable --raw', tmpDir); + assert.ok(result.success, result.error); + assert.ok(!fs.existsSync(hookPath)); + }); + + test('B5: disable refuses to remove a foreign hook', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const foreignContent = '#!/bin/sh\necho foreign\n'; + fs.writeFileSync(hookPath, foreignContent); + fs.chmodSync(hookPath, 0o755); + const result = runGsdTools('commit-docs-guard disable --raw', tmpDir); + assert.ok(!result.success, 'disable must refuse to remove a foreign hook'); + assert.strictEqual(fs.readFileSync(hookPath, 'utf8'), foreignContent, 'foreign hook must be byte-unchanged'); + }); + + test('B6: disable with no hook present is a success no-op, not an error', () => { + tmpDir = createTempGitProject(); + const result = runGsdTools('commit-docs-guard disable --raw', tmpDir); + assert.ok(result.success, result.error); + }); + + test('B7: enable outside a git repository fails cleanly, nothing written', () => { + tmpDir = createTempDir(); + const before = fs.readdirSync(tmpDir); + const result = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(!result.success, 'enable must fail outside a git repository'); + assert.deepStrictEqual(fs.readdirSync(tmpDir), before, 'nothing may be written'); + }); + + test('B8: enable resolves the real hooks dir when .git is a worktree file', (t) => { + tmpDir = createTempGitProject(); + const worktreeParent = createTempDir(); + t.after(() => cleanup(worktreeParent)); + + const wtDir = path.join(worktreeParent, 'wt'); + gitOrThrow(['worktree', 'add', wtDir, '-b', 'gsd-test-commit-docs-guard-wt'], { cwd: tmpDir }); + assert.ok(fs.statSync(path.join(wtDir, '.git')).isFile(), 'precondition: .git must be a file in a linked worktree'); + + const result = runGsdTools('commit-docs-guard enable --raw', wtDir); + assert.ok(result.success, result.error); + // Hooks are shared across worktrees in the COMMON git dir — never a + // literal `/.git/hooks`. + assert.ok(!fs.existsSync(path.join(wtDir, '.git', 'hooks')), 'must never write a literal /.git/hooks path'); + const commonHookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + assert.ok(fs.existsSync(commonHookPath), 'hook must land in the real (common) hooks directory'); + }); + + test('B9: enable refuses when core.hooksPath is already set', () => { + tmpDir = createTempGitProject(); + gitOrThrow(['config', 'core.hooksPath', 'custom-hooks'], { cwd: tmpDir }); + const result = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(!result.success, 'enable must refuse when core.hooksPath is set'); + assert.ok(result.error.includes('core.hooksPath'), result.error); + assert.ok(!fs.existsSync(path.join(tmpDir, 'custom-hooks', 'pre-commit')), 'must not write into the hooksPath-configured dir either'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.git', 'hooks', 'pre-commit')), 'must not write the ordinary hooks dir either'); + }); + + test('B10: marker detection tolerates a user-appended line', () => { + tmpDir = createTempGitProject(); + const r1 = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(r1.success, r1.error); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + fs.appendFileSync(hookPath, '\n# a user comment appended after install\n'); + + const r2 = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(r2.success, `enable must still recognize the edited hook as ours: ${r2.error}`); + assert.ok( + fs.readFileSync(hookPath, 'utf8').includes('a user comment appended after install'), + 'enable must not silently discard the user edit on a recognized hook' + ); + + const r3 = runGsdTools('commit-docs-guard disable --raw', tmpDir); + assert.ok(r3.success, `disable must still recognize the edited hook as ours: ${r3.error}`); + assert.ok(!fs.existsSync(hookPath)); + }); + + test('B11: no subcommand at all hits the unknown-subcommand routing guard', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const result = runGsdTools(['--json-errors', 'commit-docs-guard'], tmpDir); + assert.ok(!result.success, 'missing subcommand must not succeed'); + assert.notStrictEqual(result.exitCode, 0, 'missing subcommand must exit non-zero'); + const parsed = JSON.parse(result.error); + assert.deepStrictEqual(Object.keys(parsed).sort(), ['message', 'ok', 'reason']); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_unknown_command'); + assert.strictEqual(/\n\s*at\s/.test(result.error), false, 'non-debug failure must not print a stack trace'); + assert.ok(!fs.existsSync(hookPath), 'no hook may be written for a missing subcommand'); + }); + + test('B12: an unknown subcommand hits the unknown-subcommand routing guard', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const result = runGsdTools(['--json-errors', 'commit-docs-guard', 'bogus'], tmpDir); + assert.ok(!result.success, 'an unrecognized subcommand must not succeed'); + assert.notStrictEqual(result.exitCode, 0, 'an unrecognized subcommand must exit non-zero'); + const parsed = JSON.parse(result.error); + assert.deepStrictEqual(Object.keys(parsed).sort(), ['message', 'ok', 'reason']); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_unknown_command'); + assert.strictEqual(/\n\s*at\s/.test(result.error), false, 'non-debug failure must not print a stack trace'); + assert.ok(!fs.existsSync(hookPath), 'no hook may be written for an unrecognized subcommand'); + }); + + test('B13: an empty-string subcommand hits the unknown-subcommand routing guard', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const result = runGsdTools(['--json-errors', 'commit-docs-guard', ''], tmpDir); + assert.ok(!result.success, 'an empty-string subcommand must not succeed'); + assert.notStrictEqual(result.exitCode, 0, 'an empty-string subcommand must exit non-zero'); + const parsed = JSON.parse(result.error); + assert.deepStrictEqual(Object.keys(parsed).sort(), ['message', 'ok', 'reason']); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_unknown_command'); + assert.strictEqual(/\n\s*at\s/.test(result.error), false, 'non-debug failure must not print a stack trace'); + assert.ok(!fs.existsSync(hookPath), 'no hook may be written for an empty-string subcommand'); + }); + + test('B14: a whitespace-only subcommand hits the unknown-subcommand routing guard', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const result = runGsdTools(['--json-errors', 'commit-docs-guard', ' '], tmpDir); + assert.ok(!result.success, 'a whitespace-only subcommand must not succeed'); + assert.notStrictEqual(result.exitCode, 0, 'a whitespace-only subcommand must exit non-zero'); + const parsed = JSON.parse(result.error); + assert.deepStrictEqual(Object.keys(parsed).sort(), ['message', 'ok', 'reason']); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_unknown_command'); + assert.strictEqual(/\n\s*at\s/.test(result.error), false, 'non-debug failure must not print a stack trace'); + assert.ok(!fs.existsSync(hookPath), 'no hook may be written for a whitespace-only subcommand'); + }); + + test('B15: a flag-shaped value in the subcommand position hits the unknown-subcommand routing guard', () => { + tmpDir = createTempGitProject(); + const hookPath = path.join(tmpDir, '.git', 'hooks', 'pre-commit'); + const result = runGsdTools(['--json-errors', 'commit-docs-guard', '--enable'], tmpDir); + assert.ok(!result.success, 'a flag-shaped subcommand must not succeed'); + assert.notStrictEqual(result.exitCode, 0, 'a flag-shaped subcommand must exit non-zero'); + const parsed = JSON.parse(result.error); + assert.deepStrictEqual(Object.keys(parsed).sort(), ['message', 'ok', 'reason']); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_unknown_command'); + assert.strictEqual(/\n\s*at\s/.test(result.error), false, 'non-debug failure must not print a stack trace'); + assert.ok(!fs.existsSync(hookPath), 'no hook may be written for a flag-shaped subcommand'); + }); +}); + +describe('commit-docs-guard real git commit wiring (#3588 D1-D3)', () => { + const { createTempGitProject, TEST_ENV_BASE } = require('./helpers.cjs'); + const { runGit } = require('./helpers/process-seam.cjs'); + const REPO_ROOT = path.join(__dirname, '..'); + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + const enableResult = runGsdTools('commit-docs-guard enable --raw', tmpDir); + assert.ok(enableResult.success, `enable failed: ${enableResult.error}`); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function stagePlanningFile(name) { + fs.writeFileSync(path.join(tmpDir, '.planning', name), '# State\n'); + gitOrThrow(['add', `.planning/${name}`], { cwd: tmpDir }); + } + + function commitEnv() { + // RUNTIME_DIR pins the hook's gsd_run resolution (the + // _runtime-launcher.snippet.sh preamble) to THIS checkout's own + // gsd-core/bin/gsd-tools.cjs rather than relying on an ambient PATH + // install or a config-dir fallback that would not exist in CI. + return { ...process.env, ...TEST_ENV_BASE, RUNTIME_DIR: REPO_ROOT }; + } + + function commitCount() { + return Number(gitOrThrow(['rev-list', '--count', 'HEAD'], { cwd: tmpDir }).trim()); + } + + test('D1: a real `git commit` is refused when commit_docs is false and .planning/ is staged', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({ commit_docs: false })); + stagePlanningFile('STATE.md'); + const before = commitCount(); + const result = runGit(['commit', '-m', 'chore: should be refused'], { cwd: tmpDir, env: commitEnv() }); + assert.notStrictEqual(result.exitCode, 0, `commit should have been refused; stdout=${result.stdout} stderr=${result.stderr}`); + assert.strictEqual(commitCount(), before, 'nothing should have been committed'); + }); + + test('D2: a real `git commit` succeeds when commit_docs is true', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({ commit_docs: true })); + stagePlanningFile('STATE.md'); + const before = commitCount(); + const result = runGit(['commit', '-m', 'chore: should succeed'], { cwd: tmpDir, env: commitEnv() }); + assert.strictEqual(result.exitCode, 0, `commit should have succeeded; stdout=${result.stdout} stderr=${result.stderr}`); + assert.strictEqual(commitCount(), before + 1); + }); + + test('D3: disable actually unwires the hook — the same D1 scenario now succeeds', () => { + const disableResult = runGsdTools('commit-docs-guard disable --raw', tmpDir); + assert.ok(disableResult.success, disableResult.error); + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify({ commit_docs: false })); + stagePlanningFile('STATE.md'); + const before = commitCount(); + const result = runGit(['commit', '-m', 'chore: allowed after disable'], { cwd: tmpDir, env: commitEnv() }); + assert.strictEqual(result.exitCode, 0, `commit should have succeeded after disable; stdout=${result.stdout} stderr=${result.stderr}`); + assert.strictEqual(commitCount(), before + 1); + }); +}); + +describe('commit-docs-guard default install scope guarantee (#3588 E2)', () => { + test('E2: the default install path wires nothing new — commit-docs-guard is opt-in only', () => { + // #3588 scope guarantee (40-design.md): bin/install.js wiring is + // explicitly OUT of scope. This is the regression lock for that + // narrowing — checked structurally (require()'d typed exports, never + // source-text grep) against the THREE surfaces that would make the hook + // install by default. + const { MANAGED_HOOKS } = require('../hooks/managed-hooks-registry.cjs'); + assert.ok( + !MANAGED_HOOKS.some((h) => h.includes('commit-docs-guard')), + 'commit-docs-guard must not be a MANAGED_HOOKS install-time hook' + ); + + // scripts/build-hooks.js HOOKS_TO_COPY is the single source of truth for + // both the shared hooks/dist/ bundle AND bin/install.js's + // INSTALLED_HOOK_FILES/GSD_UNINSTALL_HOOKS (see the "new hook-script + // registration invariants" ripple) — asserting against the exported + // array itself, not grepping either file's source text. + const { HOOKS_TO_COPY } = require('../scripts/build-hooks.js'); + assert.ok( + !HOOKS_TO_COPY.includes('commit-docs-guard'), + 'commit-docs-guard must not be copied into the shared hooks bundle' + ); + + const { GSD_UNINSTALL_HOOKS } = require('../bin/install.js'); + assert.ok( + !GSD_UNINSTALL_HOOKS.includes('commit-docs-guard'), + 'bin/install.js must not list commit-docs-guard among installed/uninstalled hook files' + ); + }); }); describe('_wsParseRetryAfter (#308)', () => {