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)', () => {