diff --git a/.changeset/sturdy-finches-caper.md b/.changeset/sturdy-finches-caper.md new file mode 100644 index 000000000..68e068865 --- /dev/null +++ b/.changeset/sturdy-finches-caper.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3273 +--- +**The optional pre-commit hook now actually checks command-alias drift** — every guard in `.githooks/pre-commit` was inert: nine matched paths under the retired `sdk/` tree and invoked npm scripts that no longer exist, and the tenth watched gitignored build outputs that git can never stage. Staging `src/command-aliases.cts` now runs `check:alias-drift` instead of passing silently. (#2725) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 58005bc7a..d90a8661b 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -6,42 +6,33 @@ set -euo pipefail GIT_CMD="${GIT_OVERRIDE:-git}" NPM_CMD="${NPM_OVERRIDE:-npm}" -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/command-manifest\.|^sdk/src/query/command-aliases\.generated\.ts$|^gsd-core/bin/lib/command-aliases\.cjs$|^sdk/scripts/gen-command-aliases\.ts$"; then +# Tracked sources of the command-alias surface. These are the `src/*.cts` files, +# never the `gsd-core/bin/lib/*.cjs` artifacts they compile to: those are +# gitignored, so `git diff --cached --name-only` never lists them and a guard +# keyed on them can never fire (#2725). Kept in sync with +# scripts/lib/alias-drift-families.cjs by tests/precommit-alias-drift-hook.test.cjs. +alias_drift_paths=( + 'src/command-aliases.cts' + 'src/state-command-router.cts' + 'src/verify-command-router.cts' + 'src/init-command-router.cts' + 'src/phase-command-router.cts' + 'src/phases-command-router.cts' + 'src/validate-command-router.cts' + 'src/roadmap-command-router.cts' + 'src/eval-command-router.cts' + 'scripts/check-alias-drift.cjs' + 'scripts/lib/alias-drift-families.cjs' +) + +# `tr -d '\r'` first: a whole-line match fails on a CR-terminated path. +# Collected into a variable rather than piped straight into grep, because +# `grep -q` exits on first match and would SIGPIPE its upstream — which under +# `set -o pipefail` turns a successful match into a non-zero pipeline status. +staged_paths="$("$GIT_CMD" diff --cached --name-only | tr -d '\r')" + +# -F -x: fixed strings, whole line. Exact path equality, no regex anchoring to +# get wrong, and no escaping burden as the list grows. +if grep -Fxqf <(printf '%s\n' "${alias_drift_paths[@]}") <<< "$staged_paths"; then "$NPM_CMD" run check:alias-drift fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/state-document\.|^gsd-core/bin/lib/state-document\.generated\.cjs$|^sdk/scripts/gen-state-document\.ts$|^sdk/scripts/check-state-document-fresh\.mjs$"; then - "$NPM_CMD" run check:state-document-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/configuration/|^sdk/shared/config-(defaults|schema)\.manifest\.json$|^gsd-core/bin/lib/configuration\.generated\.cjs$|^sdk/scripts/gen-configuration\.mjs$"; then - "$NPM_CMD" run check:configuration-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/workstream-inventory/|^gsd-core/bin/lib/workstream-inventory-builder\.generated\.cjs$|^sdk/scripts/gen-workstream-inventory-builder\.mjs$|^sdk/scripts/check-workstream-inventory-builder-fresh\.mjs$"; then - "$NPM_CMD" run check:workstream-inventory-builder-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/project-root/|^gsd-core/bin/lib/project-root\.generated\.cjs$|^sdk/scripts/gen-project-root\.mjs$|^sdk/scripts/check-project-root-fresh\.mjs$"; then - "$NPM_CMD" run check:project-root-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/plan-scan\.ts$|^gsd-core/bin/lib/plan-scan\.generated\.cjs$|^sdk/scripts/gen-plan-scan\.mjs$|^sdk/scripts/check-plan-scan-fresh\.mjs$"; then - "$NPM_CMD" run check:plan-scan-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/secrets\.ts$|^gsd-core/bin/lib/secrets\.generated\.cjs$|^sdk/scripts/gen-secrets\.mjs$|^sdk/scripts/check-secrets-fresh\.mjs$"; then - "$NPM_CMD" run check:secrets-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/schema-detect\.ts$|^gsd-core/bin/lib/schema-detect\.generated\.cjs$|^sdk/scripts/gen-schema-detect\.mjs$|^sdk/scripts/check-schema-detect-fresh\.mjs$"; then - "$NPM_CMD" run check:schema-detect-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/query/decisions\.ts$|^gsd-core/bin/lib/decisions\.generated\.cjs$|^sdk/scripts/gen-decisions\.mjs$|^sdk/scripts/check-decisions-fresh\.mjs$"; then - "$NPM_CMD" run check:decisions-fresh -fi - -if "$GIT_CMD" diff --cached --name-only | grep -Eq "^sdk/src/workstream-name-policy\.ts$|^gsd-core/bin/lib/workstream-name-policy\.generated\.cjs$|^sdk/scripts/gen-workstream-name-policy\.mjs$|^sdk/scripts/check-workstream-name-policy-fresh\.mjs$"; then - "$NPM_CMD" run check:workstream-name-policy-fresh -fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f1b100b54..2dd63216d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -927,13 +927,18 @@ This gives maintainers a faster, higher-confidence signal than CI-only validatio ### Pre-PR Seam Checks (Manifest/Alias Routing) -If you touched any of the command-manifest or generated alias files, run: +If you touched `src/command-aliases.cts` or any of the eight `src/*-command-router.cts` +sources it feeds, run: ```bash npm run check:alias-drift ``` -This verifies generated alias artifacts are in sync with manifest source-of-truth. +This verifies the built alias artifacts under `gsd-core/bin/lib/` agree with their +source of truth — each family's `*_SUBCOMMANDS` list must match the `subcommand` +values derived from its `*_COMMAND_ALIASES` table, in order, and each router must +reference its own list. The surface is enumerated once in +`scripts/lib/alias-drift-families.cjs`. ### Editing shipped content (gsd-core/workflows, references, templates, contexts, agents/, commands/gsd/) @@ -1002,61 +1007,43 @@ npm run regen:derived Optional local pre-commit hook entry (Git-native): +`.githooks/pre-commit` is **committed** — you do not write it, you only point git at +it. It runs `check:alias-drift` when you stage one of the tracked sources that check +reads, and stays silent otherwise. + ```bash # one-time setup -mkdir -p .githooks -cat > .githooks/pre-commit <<'EOF' -#!/usr/bin/env bash -set -euo pipefail - -if git diff --cached --name-only | grep -Eq "^sdk/src/query/command-manifest\.|^sdk/src/query/command-aliases\.generated\.ts$|^gsd-core/bin/lib/command-aliases\.generated\.cjs$|^sdk/scripts/gen-command-aliases\.ts$"; then - npm run check:alias-drift -fi -EOF -chmod +x .githooks/pre-commit git config core.hooksPath .githooks ``` +This is opt-in and stays that way: nothing in `npm install` sets `core.hooksPath` for +you, so a fresh clone acquires no hooks. To stop using them, `git config --unset +core.hooksPath`. + +Do not paste a copy of the hook body into your own `.githooks/pre-commit`. Bash cannot +`require()` a CommonJS module, so the hook does carry the watched paths as literals — +but `tests/precommit-alias-drift-hook.test.cjs` runs the real hook against every source +derived from `scripts/lib/alias-drift-families.cjs` and fails in **both** directions: if +the hook stops watching a source the checker reads, and if it keeps watching a router the +checker dropped. A copy in your own tree has no such test behind it, and a hand-maintained +copy is exactly what silently rotted the previous version of this recipe (#2725) — every +path in it named the retired `sdk/` tree or a gitignored build output, so the guard +matched nothing for months. + Optional local pre-push hook to block a private author-email pattern: +`.githooks/pre-push` is committed too, and is covered by the same +`core.hooksPath` opt-in above. It is a no-op until you set the regex, so enabling +hooks does not enable this check: + ```bash # set locally in your shell profile (example) -export GSD_BLOCKED_AUTHOR_REGEX='@example-corp\\.com$' - -cat > .githooks/pre-push <<'EOF' -#!/usr/bin/env bash -set -euo pipefail - -zero_sha='0000000000000000000000000000000000000000' -blocked_regex="${GSD_BLOCKED_AUTHOR_REGEX:-}" -[[ -z "$blocked_regex" ]] && exit 0 -violations=() - -while read -r local_ref local_sha remote_ref remote_sha; do - [[ "$local_sha" == "$zero_sha" ]] && continue - if [[ "$remote_sha" == "$zero_sha" ]]; then - commits=$(git rev-list "$local_sha" --not --remotes) - else - commits=$(git rev-list "$remote_sha..$local_sha") - fi - while read -r commit; do - [[ -z "$commit" ]] && continue - email=$(git show -s --format='%ae' "$commit" | tr '[:upper:]' '[:lower:]') - if printf '%s' "$email" | grep -Eq "$blocked_regex"; then - violations+=("$commit <$email>") - fi - done <<< "$commits" -done - -if [[ ${#violations[@]} -gt 0 ]]; then - echo "Push blocked: commit author email matched local blocked regex ($blocked_regex)." >&2 - printf ' - %s\n' "${violations[@]}" >&2 - exit 1 -fi -EOF -chmod +x .githooks/pre-push +export GSD_BLOCKED_AUTHOR_REGEX='@example-corp\.com$' ``` +With that exported, a push carrying a commit whose author email matches is blocked, +and the hook names the offending commits. Unset the variable to disable it. + ### CI Test Quality Checks The following checks run on every PR in addition to the test suite: diff --git a/bin/install.js b/bin/install.js index 566ae8a79..e0b07e86e 100755 --- a/bin/install.js +++ b/bin/install.js @@ -374,7 +374,7 @@ const GSD_CHANGESET_FILES = [ 'github-release-notes.cjs', 'lint.cjs', 'new.cjs', 'README.md', // documentation only — not user-authored ]; -const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs']; +const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs', 'alias-drift-families.cjs']; /** * Resolve a runtime's shared-hooks directory name from its descriptor. diff --git a/scripts/check-alias-drift.cjs b/scripts/check-alias-drift.cjs index abaca0be8..8dcc75681 100644 --- a/scripts/check-alias-drift.cjs +++ b/scripts/check-alias-drift.cjs @@ -4,9 +4,10 @@ const fs = require('node:fs'); const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { FAMILIES, ALIAS_TABLE } = require('./lib/alias-drift-families.cjs'); const ROOT = path.resolve(__dirname, '..'); -const aliasesPath = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'command-aliases.cjs'); +const aliasesPath = path.join(ROOT, 'gsd-core', 'bin', 'lib', `${ALIAS_TABLE}.cjs`); function fail(message) { process.stderr.write(`${message}\n`); @@ -36,48 +37,11 @@ function main() { const aliases = require(aliasesPath); - const families = [ - { - commandAliases: 'STATE_COMMAND_ALIASES', - subcommands: 'STATE_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'state-command-router.cjs'), - }, - { - commandAliases: 'VERIFY_COMMAND_ALIASES', - subcommands: 'VERIFY_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'verify-command-router.cjs'), - }, - { - commandAliases: 'INIT_COMMAND_ALIASES', - subcommands: 'INIT_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'init-command-router.cjs'), - }, - { - commandAliases: 'PHASE_COMMAND_ALIASES', - subcommands: 'PHASE_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'phase-command-router.cjs'), - }, - { - commandAliases: 'PHASES_COMMAND_ALIASES', - subcommands: 'PHASES_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'phases-command-router.cjs'), - }, - { - commandAliases: 'VALIDATE_COMMAND_ALIASES', - subcommands: 'VALIDATE_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'validate-command-router.cjs'), - }, - { - commandAliases: 'ROADMAP_COMMAND_ALIASES', - subcommands: 'ROADMAP_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'roadmap-command-router.cjs'), - }, - { - commandAliases: 'EVAL_COMMAND_ALIASES', - subcommands: 'EVAL_SUBCOMMANDS', - routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', 'eval-command-router.cjs'), - }, - ]; + const families = FAMILIES.map((family) => ({ + commandAliases: family.commandAliases, + subcommands: family.subcommands, + routerPath: path.join(ROOT, 'gsd-core', 'bin', 'lib', `${family.router}.cjs`), + })); for (const family of families) { const commandAliases = aliases[family.commandAliases]; diff --git a/scripts/lib/alias-drift-families.cjs b/scripts/lib/alias-drift-families.cjs new file mode 100644 index 000000000..ca7f62103 --- /dev/null +++ b/scripts/lib/alias-drift-families.cjs @@ -0,0 +1,46 @@ +'use strict'; + +/** + * Single source of truth for the command-alias drift surface. + * + * Two parallel surfaces consume this: `scripts/check-alias-drift.cjs`, which + * validates the built alias artifacts, and `.githooks/pre-commit`, which decides + * when to run that validation. Until #2725 the hook's watched-path list had + * drifted from the checker's family table until every pattern named either the + * retired `sdk/` tree (ADR-0174) or a gitignored build output, so the guard was + * silently inert. Both surfaces derive from this list now, and + * `tests/precommit-alias-drift-hook.test.cjs` fails if they diverge again. + */ + +/** + * Alias families the drift check validates. `router` is the module basename, + * shared by the `src/.cts` source and the `gsd-core/bin/lib/.cjs` + * artifact it compiles to. + */ +const FAMILIES = Object.freeze([ + { commandAliases: 'STATE_COMMAND_ALIASES', subcommands: 'STATE_SUBCOMMANDS', router: 'state-command-router' }, + { commandAliases: 'VERIFY_COMMAND_ALIASES', subcommands: 'VERIFY_SUBCOMMANDS', router: 'verify-command-router' }, + { commandAliases: 'INIT_COMMAND_ALIASES', subcommands: 'INIT_SUBCOMMANDS', router: 'init-command-router' }, + { commandAliases: 'PHASE_COMMAND_ALIASES', subcommands: 'PHASE_SUBCOMMANDS', router: 'phase-command-router' }, + { commandAliases: 'PHASES_COMMAND_ALIASES', subcommands: 'PHASES_SUBCOMMANDS', router: 'phases-command-router' }, + { commandAliases: 'VALIDATE_COMMAND_ALIASES', subcommands: 'VALIDATE_SUBCOMMANDS', router: 'validate-command-router' }, + { commandAliases: 'ROADMAP_COMMAND_ALIASES', subcommands: 'ROADMAP_SUBCOMMANDS', router: 'roadmap-command-router' }, + { commandAliases: 'EVAL_COMMAND_ALIASES', subcommands: 'EVAL_SUBCOMMANDS', router: 'eval-command-router' }, +].map(Object.freeze)); + +/** Basename of the generated alias table every family is checked against. */ +const ALIAS_TABLE = 'command-aliases'; + +/** + * Repo-relative TRACKED sources whose edit should re-run the drift check. + * + * Deliberately the `src/*.cts` sources, never the `gsd-core/bin/lib/*.cjs` + * build outputs they compile to: those are gitignored, so + * `git diff --cached --name-only` can never list them and a pre-commit guard + * keyed on them can never fire (#2725). + */ +function stagedSourcePaths() { + return [`src/${ALIAS_TABLE}.cts`, ...FAMILIES.map((family) => `src/${family.router}.cts`)]; +} + +module.exports = { FAMILIES, ALIAS_TABLE, stagedSourcePaths }; diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index eec9ca4f0..e029172d3 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -413,6 +413,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index c47c4b525..d646d27c8 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -483,6 +483,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index 769141d23..ea845fc3b 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -482,6 +482,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs" diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index aeec5503b..a754641ff 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -411,6 +411,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/cline.json b/tests/fixtures/install-tree/cline.json index 305ac0683..e469b877b 100644 --- a/tests/fixtures/install-tree/cline.json +++ b/tests/fixtures/install-tree/cline.json @@ -383,6 +383,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 8be621622..4d8a51ddd 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -483,6 +483,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 325bfb8d5..c5bd41e73 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -491,6 +491,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs" diff --git a/tests/fixtures/install-tree/copilot.json b/tests/fixtures/install-tree/copilot.json index 66c8011ab..ebdf2fe99 100644 --- a/tests/fixtures/install-tree/copilot.json +++ b/tests/fixtures/install-tree/copilot.json @@ -382,6 +382,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index ae6ed8c60..88319b726 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -389,6 +389,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index aa429b175..f5988a958 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -412,6 +412,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 6921ef1ab..19d08fc7b 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -486,6 +486,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 8002fc43a..d1eda65f2 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -412,6 +412,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/kimi.json b/tests/fixtures/install-tree/kimi.json index e2bfcb266..3d55c88e9 100644 --- a/tests/fixtures/install-tree/kimi.json +++ b/tests/fixtures/install-tree/kimi.json @@ -448,6 +448,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 7b183d482..b1da53bf5 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -486,6 +486,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index 5e7e17721..15b976fbe 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -380,6 +380,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs" diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 2caa99098..ccba25186 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -412,6 +412,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/trae.json b/tests/fixtures/install-tree/trae.json index 107b9cdce..fe49afaa9 100644 --- a/tests/fixtures/install-tree/trae.json +++ b/tests/fixtures/install-tree/trae.json @@ -380,6 +380,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index b6997cc77..58567748c 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -383,6 +383,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs" diff --git a/tests/fixtures/install-tree/zcode.json b/tests/fixtures/install-tree/zcode.json index df2cefaae..64325b2c4 100644 --- a/tests/fixtures/install-tree/zcode.json +++ b/tests/fixtures/install-tree/zcode.json @@ -451,6 +451,7 @@ "scripts/fix-slash-commands.cjs", "scripts/gen-capability-registry.cjs", "scripts/gen-loop-host-contract.cjs", + "scripts/lib/alias-drift-families.cjs", "scripts/lib/allowlist-ratchet.cjs", "scripts/lib/cli-exit.cjs", "scripts/lib/drift-scan.cjs", diff --git a/tests/precommit-alias-drift-hook.test.cjs b/tests/precommit-alias-drift-hook.test.cjs index 1866fe6da..2794fdffd 100644 --- a/tests/precommit-alias-drift-hook.test.cjs +++ b/tests/precommit-alias-drift-hook.test.cjs @@ -4,13 +4,33 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const fc = require('fast-check'); const { runHook } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { stagedSourcePaths } = require('../scripts/lib/alias-drift-families.cjs'); const ROOT = path.resolve(__dirname, '..'); const HOOK_PATH = path.join(ROOT, '.githooks', 'pre-commit'); +/** + * Derived, never restated: the sources `scripts/check-alias-drift.cjs` actually + * reads. The hook's watched list and the checker's family table are parallel + * surfaces over one constant set, and their silent divergence is exactly what + * made the guard inert (#2725) — so this test asserts the hook against the + * checker's own table rather than against a second copy of it. + */ +const DRIFT_SOURCES = stagedSourcePaths(); + +/** The two script paths the hook additionally watches, beyond the sources above. */ +const WATCHED_SCRIPTS = [ + 'scripts/check-alias-drift.cjs', + 'scripts/lib/alias-drift-families.cjs', +]; + +const WATCHED_PATHS = new Set([...DRIFT_SOURCES, ...WATCHED_SCRIPTS]); + /** * Write a mock bash script to a .sh file in tmpDir and return its absolute path. * The hook invokes it via GIT_OVERRIDE / NPM_OVERRIDE — bash executes the path @@ -22,58 +42,266 @@ function writeMock(tmpDir, name, content) { return filePath; } -describe('.githooks/pre-commit alias drift guard', () => { - test('runs npm check when staged files include command-manifest/alias artifacts', (t) => { - const tmpDir = createTempDir('gsd-precommit-hook-'); - t.after(() => cleanup(tmpDir)); +/** + * Run the hook with a mock `git diff --cached --name-only` that emits + * `stagedLines` verbatim, and a mock `npm` that appends one line to a marker + * file per invocation. + * + * `stagedLines` is a raw string, not an array, so a row can exercise the exact + * byte shape git produces — including a CR-terminated line. Every other row + * builds it from LF-joined paths, which is what git actually emits. + * + * Returns `{ result, npmCalls }`. Counting invocations (rather than testing for + * file existence) is what lets the many-staged-files row assert the check runs + * exactly once. + */ +function runPreCommit(t, stagedLines) { + const tmpDir = createTempDir('gsd-precommit-hook-'); + t.after(() => cleanup(tmpDir)); - const mockGit = writeMock(tmpDir, 'git', `#!/usr/bin/env bash\nprintf "%s\\n" "${'sdk/src/query/command-manifest.phase.ts'}"\n`); - const mockNpm = writeMock(tmpDir, 'npm', `#!/usr/bin/env bash\nprintf "called" > "$GSD_TEST_NPM_MARKER"\n`); + const marker = path.join(tmpDir, 'npm-calls.txt'); - const marker = path.join(tmpDir, 'npm-called.txt'); + // The payload goes through a FILE that the mock `cat`s, never through printf. + // `printf '%s' "…\n"` emits a literal backslash-n — bash does not expand escapes + // in a double-quoted string and printf does not expand them in a %s argument — so + // the hook saw one unterminated line and no whole-line match could ever succeed. + // `cat` reproduces bytes verbatim, which is also what lets the CR-terminated row + // and the empty-staged-list row mean what they say. + const stagedFile = path.join(tmpDir, 'staged-paths.txt'); + fs.writeFileSync(stagedFile, stagedLines); + const mockGit = writeMock( + tmpDir, + 'git', + `#!/usr/bin/env bash\ncat ${JSON.stringify(stagedFile)}\n`, + ); + const mockNpm = writeMock( + tmpDir, + 'npm', + `#!/usr/bin/env bash\nprintf 'call %s\\n' "$*" >> "$GSD_TEST_NPM_MARKER"\n`, + ); - throwIfFailed( - runHook(HOOK_PATH, [], { - interpreter: 'bash', - cwd: ROOT, - env: { - ...process.env, - GIT_OVERRIDE: mockGit, - NPM_OVERRIDE: mockNpm, - GSD_TEST_NPM_MARKER: marker, - }, - timeoutMs: 15000, - }), - `bash ${HOOK_PATH}`, - ); - - assert.ok(fs.existsSync(marker), 'expected npm run check:alias-drift to be invoked'); + const result = runHook(HOOK_PATH, [], { + interpreter: 'bash', + cwd: ROOT, + env: { + ...process.env, + GIT_OVERRIDE: mockGit, + NPM_OVERRIDE: mockNpm, + GSD_TEST_NPM_MARKER: marker, + }, + timeoutMs: PROBE_TIMEOUT_MS, }); - test('does not run npm check when staged files are unrelated', (t) => { - const tmpDir = createTempDir('gsd-precommit-hook-'); - t.after(() => cleanup(tmpDir)); + const npmCalls = fs.existsSync(marker) + ? fs.readFileSync(marker, 'utf8').split(/\r?\n/).filter(Boolean).length + : 0; - const mockGit = writeMock(tmpDir, 'git', `#!/usr/bin/env bash\nprintf "%s\\n" "README.md"\n`); - const mockNpm = writeMock(tmpDir, 'npm', `#!/usr/bin/env bash\nprintf "called" > "$GSD_TEST_NPM_MARKER"\n`); + return { result, npmCalls }; +} - const marker = path.join(tmpDir, 'npm-called.txt'); +/** Join paths the way `git diff --cached --name-only` emits them. */ +function staged(...paths) { + return paths.length === 0 ? '' : `${paths.join('\n')}\n`; +} - throwIfFailed( - runHook(HOOK_PATH, [], { - interpreter: 'bash', - cwd: ROOT, - env: { - ...process.env, - GIT_OVERRIDE: mockGit, - NPM_OVERRIDE: mockNpm, - GSD_TEST_NPM_MARKER: marker, - }, - timeoutMs: 15000, - }), - `bash ${HOOK_PATH}`, +describe('.githooks/pre-commit alias drift guard', () => { + test('runs the drift check when the alias table source is staged', (t) => { + const { result, npmCalls } = runPreCommit(t, staged('src/command-aliases.cts')); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal( + npmCalls, + 1, + 'staging the tracked alias source must invoke check:alias-drift — ' + + 'the guard previously watched gitignored build output, which git never stages (#2725)', + ); + }); + + test('fires when the drift checker itself is staged', (t) => { + const { result, npmCalls } = runPreCommit(t, staged('scripts/check-alias-drift.cjs')); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, 1, 'editing the checker must re-run it'); + }); + + test('fires when the shared family table is staged', (t) => { + const { result, npmCalls } = runPreCommit( + t, + staged('scripts/lib/alias-drift-families.cjs'), ); - assert.ok(!fs.existsSync(marker), 'expected npm check to be skipped for unrelated staged files'); + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, 1, 'editing the family table must re-run the check'); + }); + + test('fires for every source the drift check actually reads', (t) => { + assert.ok(DRIFT_SOURCES.length > 0, 'the drift surface must not be empty'); + + for (const source of DRIFT_SOURCES) { + const { npmCalls } = runPreCommit(t, staged(source)); + + assert.equal( + npmCalls, + 1, + `${source} is in the drift checker's family table but .githooks/pre-commit ` + + 'does not watch it — the two surfaces have diverged again (#2725)', + ); + } + }); + + test('does not watch routers the drift check dropped', (t) => { + // The reverse direction of the parity row above. That row catches the hook + // UNDER-watching; this one catches it OVER-watching, using every + // `src/*-command-router.cts` on disk as the universe. Drop a family from + // scripts/lib/alias-drift-families.cjs without narrowing the hook and this + // fails — which is the half of the divergence #2725 would otherwise leave open. + const allRouters = fs + .readdirSync(path.join(ROOT, 'src')) + .filter((name) => name.endsWith('-command-router.cts')) + .map((name) => `src/${name}`); + + const unwatched = allRouters.filter((routerPath) => !WATCHED_PATHS.has(routerPath)); + + assert.ok( + allRouters.length > DRIFT_SOURCES.length - 1, + 'expected more routers on disk than the drift check reads — otherwise this row proves nothing', + ); + assert.ok(unwatched.length > 0, 'expected at least one router outside the drift surface'); + + for (const routerPath of unwatched) { + const { npmCalls } = runPreCommit(t, staged(routerPath)); + + assert.equal( + npmCalls, + 0, + `${routerPath} is not in the drift checker's family table, but .githooks/pre-commit ` + + 'still watches it — the hook over-watches relative to the module (#2725)', + ); + } + }); + + test('does not run the drift check for unrelated staged files', (t) => { + const { result, npmCalls } = runPreCommit(t, staged('README.md')); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, 0, 'expected npm check to be skipped for unrelated staged files'); + }); + + test('does not fire for a src file outside the drift surface', (t) => { + const outsider = 'src/milestone.cts'; + // Guards the row against going vacuous if milestone ever joins the family. + assert.ok( + !WATCHED_PATHS.has(outsider), + `${outsider} joined the drift surface — pick a different outsider for this row`, + ); + + const { result, npmCalls } = runPreCommit(t, staged(outsider)); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal( + npmCalls, + 0, + 'a guard that fires on every src/ edit trains contributors to bypass it', + ); + }); + + test('does not invoke a missing npm script for retired sdk paths', (t) => { + const { result, npmCalls } = runPreCommit(t, staged('sdk/src/query/state-document.ts')); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal( + npmCalls, + 0, + 'the retired sdk/ guards called npm scripts that do not exist, aborting the commit (#2725)', + ); + }); + + test('does not watch the gitignored build output', (t) => { + const { result, npmCalls } = runPreCommit( + t, + staged('gsd-core/bin/lib/command-aliases.cjs'), + ); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal( + npmCalls, + 0, + 'build outputs are gitignored and can never be staged — watching them is what made the guard inert', + ); + }); + + test('exits clean when nothing is staged', (t) => { + const { result, npmCalls } = runPreCommit(t, staged()); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, 0); + }); + + test('runs the check once when one of many staged files matches', (t) => { + const { result, npmCalls } = runPreCommit( + t, + staged( + 'README.md', + 'docs/adr/0174-retire-gsd-sdk-package-boundary.md', + 'src/command-aliases.cts', + 'tests/helpers.cjs', + ), + ); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, 1, 'expected exactly one check:alias-drift invocation'); + }); + + test('matches a CR-terminated staged path', (t) => { + const { result, npmCalls } = runPreCommit(t, 'src/command-aliases.cts\r\n'); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal( + npmCalls, + 1, + 'a $-anchored grep fails on a CR-terminated line — the hook must strip \\r first', + ); + }); + + describe('anchors the pattern exactly (limit-1 / limit / limit+1)', () => { + const cases = [ + ['src/command-aliases.ct', 0, 'limit-1: one character short of the extension'], + ['src/command-aliases.cts', 1, 'limit: the exact tracked source'], + ['src/command-aliases.ctsx', 0, 'limit+1: one character past the extension'], + ['src/command-aliases.cts.bak', 0, 'suffix past the end anchor'], + ['notsrc/command-aliases.cts', 0, 'prefix before the start anchor'], + ]; + + for (const [stagedPath, expectedCalls, label] of cases) { + test(label, (t) => { + const { result, npmCalls } = runPreCommit(t, staged(stagedPath)); + + throwIfFailed(result, `bash ${HOOK_PATH}`); + assert.equal(npmCalls, expectedCalls, `${stagedPath} -> ${expectedCalls} npm call(s)`); + }); + } + }); + + test('property: never fires for a path outside the watched set', (t) => { + // Biased toward near-misses (src/-prefixed, .cts-suffixed) because that is + // where over-matching actually hides. numRuns is held low: every sample + // spawns bash. + const nearMiss = fc + .tuple( + fc.constantFrom('src/', 'srcx/', 'notsrc/', 'docs/', 'tests/', ''), + fc.stringMatching(/^[a-z][a-z0-9-]{0,20}$/), + fc.constantFrom('.cts', '.cjs', '.ts', '.md', '.cts.bak', ''), + ) + .map(([dir, stem, ext]) => `${dir}${stem}${ext}`) + .filter((p) => !WATCHED_PATHS.has(p)); + + fc.assert( + fc.property(nearMiss, (stagedPath) => { + const { result, npmCalls } = runPreCommit(t, staged(stagedPath)); + throwIfFailed(result, `bash ${HOOK_PATH}`); + return npmCalls === 0; + }), + { numRuns: 20 }, + ); }); });