golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

enhance(#3588): add an opt-in commit_docs pre-commit hook (#3609)

Browse Source 
* 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 <sim@local>
This commit is contained in:
Tom Boucher
2026-08-18 00:25:32 -04:00
committed by GitHub
parent fba3b9c24f
commit fe64704ace
9 changed files with 864 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
src/command-aliases.cts
View File

@@ -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": [],

243
src/commands.cts
View File

@@ -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.<phase-id>` 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,
};

4
src/io.cts
View File

@@ -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