diff --git a/.changeset/silly-finches-squeak.md b/.changeset/silly-finches-squeak.md new file mode 100644 index 000000000..25bd78049 --- /dev/null +++ b/.changeset/silly-finches-squeak.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3960 +--- +**Every GSD enforcement hook now declares its crash policy.** Hooks used to end their outer catch with a bare `process.exit(0)` or `process.exit(2)`, so whether a hook fails open or closed on its own bug was invisible without reading its source; hooks now terminate through `allow`/`deny`/`crash` and declare a required `ON_CRASH` policy, with no change to any hook's effective exit code. Also fixes #3838: `gsd-validate-commit.sh`'s config/JSON/git-subcommand checks no longer treat "could not run" the same as a genuine negative — a failed check now says so on stderr instead of silently allowing every commit. (#3911) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5439b8d87..28b951269 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -301,6 +301,18 @@ Runtime hooks that integrate with the host AI agent: See [`docs/INVENTORY.md`](INVENTORY.md#hooks) for the authoritative hook roster. +**Crash policy (ADR-3889 Phase 7, #3911).** Every enforcement hook terminates +through `hooks/lib/hook-exit.js`'s `allow(payload)` (exit 0), `deny(payload, +stderrPayload?)` (exit 2), or `crash(onCrash, payload)` — the last dispatching +per a `HOOK_ON_CRASH` policy the hook must declare explicitly (`ALLOW` or +`DENY`, no default), so a hook's fail-open/fail-closed stance is a visible +declaration rather than an inference from a bare `process.exit(N)`. Two hooks +are deliberate exceptions — `gsd-read-injection-scanner.js` (PostToolUse) and +`gsd-cursor-subagent-start.js` (Cursor) — whose harnesses read the block +decision from the JSON response body at exit 0, not from the exit code, so +they never call `deny()`. See +[Declare a hook's crash policy](how-to/declare-a-hook-crash-policy.md). + ### Command Routing Hub (`gsd-core/bin/lib/command-routing-hub.cjs`) CJS command family routers dispatch through `CommandRoutingHub`. The hub owns the no-throw pure-result contract (`hub.dispatch()` catches internal exceptions and returns `{ ok: false, kind, ...typedPayload }`) and the closed runtime error taxonomy (`UnknownCommand`, `InvalidArgs`, `HandlerRefusal`, `HandlerFailure`). Router adapters remain thin CLI translators — they build the hub, call `dispatch`, then map the Result to `output()`/`error()` calls. The runtime is single-path (no dual-runtime mode selection). See `docs/adr/0174-retire-gsd-sdk-package-boundary.md`. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 42258c28e..d531815ed 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -200,6 +200,7 @@ - ["Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract](#3884-failure-is-a-value--strict-argv-rejection-and-the---pick-absence-contract) - [No Silent Swallow, No Verdict From Dropped Data](#3885-no-silent-swallow-no-verdict-from-dropped-data) - [Runtime Marker Resolution, Derived Codex Sandbox, and In-Phase Short-Form Dependencies](#3897-runtime-marker-resolution-derived-codex-sandbox-and-in-phase-short-form-dependencies) + - [Hooks Declare Their Crash Policy](#3911-hooks-declare-their-crash-policy) --- @@ -3919,6 +3920,62 @@ happens, matching the retired behavior exactly. --- +### 3911. Hooks Declare Their Crash Policy + +**Purpose:** Give every shipped enforcement hook (`hooks/*.js`, `hooks/*.sh`) a +named, auditable termination vocabulary instead of a bare `process.exit(N)` +scattered per file — and make a hook's fail-open/fail-closed choice a +declaration a reviewer can see, rather than an inference from which literal +integer follows `process.exit(` in its outer `catch`. + +**What changed (ADR-3889 Phase 7, #3911):** + +- `hooks/lib/hook-exit.js` (hand-written) exposes `allow(payload)` → exit 0, + `deny(payload, stderrPayload?)` → exit 2, and `crash(onCrash, payload)`, + which dispatches to `allow`/`deny` per a `HOOK_ON_CRASH` policy the caller + must supply — `crash()` has no default policy, so a hook cannot fail open + by omission. +- Every one of the 19 enforcement hooks under `hooks/*.js` now declares + `const ON_CRASH = HOOK_ON_CRASH.ALLOW` (or `DENY`) once, with a + hook-specific comment naming why, and calls `crash(ON_CRASH, payload)` from + its outer catch instead of a bare `process.exit(0)` / `process.exit(2)`. No + hook's effective exit code changed — this is a naming-and-declaration + migration, not a behavior change. +- `hooks/lib/cli-exit.js` and `hooks/lib/exit-code-registry.js` are new, + generated, git-tracked copies of the exit-code seam (`src/cli-exit.cts` / + `gsd-core/bin/shared/exit-codes.json`), so a shipped hook can terminate + correctly on a raw, unbuilt clone without depending on `gsd-core/bin/lib/` + tsc output. Generated by `scripts/gen-hooks-cli-exit.cjs` and + `scripts/gen-exit-code-registry.cjs`, both `--check`ed by + `npm run lint:generated-sync`. +- `terminateNow` gained an optional third argument, `stderrPayload`, so a + deny can send a full JSON body to stdout and a distinct plain-text reason + to stderr — needed because `gsd-write-guard.js` (Kimi's native hook bus + reads stderr verbatim back to the model) always sent only the bare reason + string on fd 2. The two streams are now written in independent try/catch + blocks: previously a payload that failed to serialize on fd 1 aborted + before fd 2 ever wrote, producing a deny with an empty stderr reason. +- Two hooks are deliberately **not** migrated to `deny()`: + `gsd-read-injection-scanner.js` (PostToolUse — its harness reads the block + decision from the JSON response body, not the exit code) and + `gsd-cursor-subagent-start.js` (follows Cursor's own `subagentStart` + protocol, which reads `permission: "deny"` from the JSON body at exit 0). + Both still use `allow()`/`crash()` for their no-op and crash paths. +- `gsd-phase-boundary.sh`, `gsd-session-state.sh`, and + `gsd-validate-commit.sh` gained `set -euo pipefail`, and + `gsd-validate-commit.sh`'s three swallow-and-pass sites (the opt-in config + read, JSON command extraction, and the `isGitSubcommand` classifier) now + distinguish a genuine negative from "could not run" — on the latter they + emit a stderr diagnostic and exit 0 instead of silently allowing every + commit (#3838). + +See [Declare a hook's crash policy](../how-to/declare-a-hook-crash-policy.md) +for the full how-to, and +[ADR-3889](../adr/3889-process-exit-contract.md) for the exit-code registry +this vocabulary is layered over. + +--- + _Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features/` and run `--write`._ diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 89f42a921..b6f6e6277 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -717,6 +717,16 @@ Full listing: `hooks/`. | `gsd-graphify-update.sh` | `PostToolUse` | Auto-rebuild knowledge graph after main HEAD advances (opt-in, default off — #3347) | | `gsd-node-runner.sh` | (helper) | Portable node resolver managed JS hook commands route through under `--portable-hooks`: install-time node path first, then `command -v node`, then well-known layouts — resolves at hook-fire time so a shared config root works in every environment (#3662) | +### Hook Library (`hooks/lib/`) + +Shared modules a hook `require()`s relative to its own `__dirname`; not separately invoked and not part of the auto-checked `hooks` manifest family (`scripts/gen-inventory-manifest.cjs` walks `hooks/` non-recursively). New entries below are #3911's; the directory holds other pre-existing helpers (`cursor-workspace.js`, `git-cmd.js`, `injection-patterns.js`, `isolation-deny-reason.js`, `isolation-sentinel.js`, `gsd-graphify-rebuild.sh`) not enumerated here. + +| Module | Purpose | +|--------|---------| +| `hook-exit.js` | Hand-written hook-facing exit vocabulary layered over `cli-exit.js`'s `terminateNow`: `allow(payload)` (exit 0), `deny(payload, stderrPayload?)` (exit 2), `crash(onCrash, payload)` dispatching per a REQUIRED `HOOK_ON_CRASH` policy — no default, so a hook cannot fail open by omission (ADR-3889 Phase 7, #3911) | +| `cli-exit.js` | Generated, git-tracked copy of `src/cli-exit.cts`'s `ExitError`/`runMain`/`terminateNow` seam, so a shipped hook can terminate without depending on `gsd-core/bin/lib/` tsc output. Regenerated by `scripts/gen-hooks-cli-exit.cjs --write`; byte-compared by `npm run lint:generated-sync` (#3911) | +| `exit-code-registry.js` | Generated, git-tracked copy of the exit-code registry (`gsd-core/bin/shared/exit-codes.json`) — `exitCodeFor`/`nameForExitCode`, pure and total over the closed table. Regenerated by `scripts/gen-exit-code-registry.cjs --write`; byte-compared by `npm run lint:generated-sync` (#3905/#3906/#3911, ADR-3889) | + --- ## Maintenance diff --git a/docs/README.md b/docs/README.md index 0e772d0e5..a2506c1ff 100644 --- a/docs/README.md +++ b/docs/README.md @@ -31,6 +31,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [State a failing direction](how-to/state-a-failing-direction.md) — say what output constitutes failure for an `` verify command, and migrate a phase planned before the rule - [Resolve a contract-drift finding](how-to/resolve-contract-drift-findings.md) — bring an agent's completion contract, read-tag gate, or deleted-file test reference back into agreement with the registry - [Resolve unreachable-guard findings](how-to/resolve-unreachable-guard-findings.md) — fix shell guards whose fallback arm cannot run, and tell "nothing to report" apart from "could not look" +- [Declare a hook's crash policy](how-to/declare-a-hook-crash-policy.md) — terminate a GSD hook with `allow`/`deny`/`crash`, declare its `ON_CRASH` policy, and tell a hook's own crash apart from a check that could not run at all - [Resolve a skipped capability probe](how-to/resolve-a-skipped-capability-probe.md) — act on a coverage gate that held your phase for an unestablished scope, or a planning checkpoint that reported `skipped` instead of a verdict - [Diagnose which gsd-tools is running](how-to/diagnose-a-foreign-gsd-tools.md) — tell this package's tool apart from the predecessor's colliding binary and from a gsd-core too old to identify itself - [Resolve an ESLint glob-coverage finding](how-to/resolve-eslint-coverage-findings.md) — bring a source file that matches no lint rule under coverage, or record a reasoned exemption diff --git a/docs/features/hooks-declare-their-crash-policy.md b/docs/features/hooks-declare-their-crash-policy.md new file mode 100644 index 000000000..bb9d6c103 --- /dev/null +++ b/docs/features/hooks-declare-their-crash-policy.md @@ -0,0 +1,57 @@ +--- +id: 3911 +title: Hooks Declare Their Crash Policy +group: v1.7.0 Features +--- + +**Purpose:** Give every shipped enforcement hook (`hooks/*.js`, `hooks/*.sh`) a +named, auditable termination vocabulary instead of a bare `process.exit(N)` +scattered per file — and make a hook's fail-open/fail-closed choice a +declaration a reviewer can see, rather than an inference from which literal +integer follows `process.exit(` in its outer `catch`. + +**What changed (ADR-3889 Phase 7, #3911):** + +- `hooks/lib/hook-exit.js` (hand-written) exposes `allow(payload)` → exit 0, + `deny(payload, stderrPayload?)` → exit 2, and `crash(onCrash, payload)`, + which dispatches to `allow`/`deny` per a `HOOK_ON_CRASH` policy the caller + must supply — `crash()` has no default policy, so a hook cannot fail open + by omission. +- Every one of the 19 enforcement hooks under `hooks/*.js` now declares + `const ON_CRASH = HOOK_ON_CRASH.ALLOW` (or `DENY`) once, with a + hook-specific comment naming why, and calls `crash(ON_CRASH, payload)` from + its outer catch instead of a bare `process.exit(0)` / `process.exit(2)`. No + hook's effective exit code changed — this is a naming-and-declaration + migration, not a behavior change. +- `hooks/lib/cli-exit.js` and `hooks/lib/exit-code-registry.js` are new, + generated, git-tracked copies of the exit-code seam (`src/cli-exit.cts` / + `gsd-core/bin/shared/exit-codes.json`), so a shipped hook can terminate + correctly on a raw, unbuilt clone without depending on `gsd-core/bin/lib/` + tsc output. Generated by `scripts/gen-hooks-cli-exit.cjs` and + `scripts/gen-exit-code-registry.cjs`, both `--check`ed by + `npm run lint:generated-sync`. +- `terminateNow` gained an optional third argument, `stderrPayload`, so a + deny can send a full JSON body to stdout and a distinct plain-text reason + to stderr — needed because `gsd-write-guard.js` (Kimi's native hook bus + reads stderr verbatim back to the model) always sent only the bare reason + string on fd 2. The two streams are now written in independent try/catch + blocks: previously a payload that failed to serialize on fd 1 aborted + before fd 2 ever wrote, producing a deny with an empty stderr reason. +- Two hooks are deliberately **not** migrated to `deny()`: + `gsd-read-injection-scanner.js` (PostToolUse — its harness reads the block + decision from the JSON response body, not the exit code) and + `gsd-cursor-subagent-start.js` (follows Cursor's own `subagentStart` + protocol, which reads `permission: "deny"` from the JSON body at exit 0). + Both still use `allow()`/`crash()` for their no-op and crash paths. +- `gsd-phase-boundary.sh`, `gsd-session-state.sh`, and + `gsd-validate-commit.sh` gained `set -euo pipefail`, and + `gsd-validate-commit.sh`'s three swallow-and-pass sites (the opt-in config + read, JSON command extraction, and the `isGitSubcommand` classifier) now + distinguish a genuine negative from "could not run" — on the latter they + emit a stderr diagnostic and exit 0 instead of silently allowing every + commit (#3838). + +See [Declare a hook's crash policy](../how-to/declare-a-hook-crash-policy.md) +for the full how-to, and +[ADR-3889](../adr/3889-process-exit-contract.md) for the exit-code registry +this vocabulary is layered over. diff --git a/docs/how-to/declare-a-hook-crash-policy.md b/docs/how-to/declare-a-hook-crash-policy.md new file mode 100644 index 000000000..ac6c756ec --- /dev/null +++ b/docs/how-to/declare-a-hook-crash-policy.md @@ -0,0 +1,173 @@ +# Declare a hook's crash policy + +A GSD enforcement hook (`hooks/*.js`, `hooks/*.sh`) ends every run by terminating +the process — but "terminate" is not one decision, it is at least three: pass +the tool call, block it, or explain what happens when the hook's *own* code +breaks. Before ADR-3889 Phase 7 (#3911), each hook answered the third question +with a bare `process.exit(0)` or `process.exit(2)` buried in an outer `catch`, +so a reviewer had to read every hook's catch block to know its fail-open/ +fail-closed stance. `hooks/lib/hook-exit.js` names all three outcomes and makes +the third one a declaration you write once, near the top of the file. + +This page covers terminating a hook correctly, declaring and justifying +`ON_CRASH`, the one case that needs a distinct stderr payload, the two hooks +that must NOT use this vocabulary at all, and what to do when a check inside +your hook cannot run rather than merely returning a negative. + +## The three outcomes + +```js +const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); +``` + +- **`allow(payload)`** — exit 0. The tool call proceeds. `payload` is optional; + most no-op paths call `allow(undefined)`. +- **`deny(payload, stderrPayload?)`** — exit 2, the Claude Code hook-protocol + block code. `payload` is written to stdout; by default the same bytes go to + stderr too. See "A deny needing a distinct stderr payload" below for when + you pass `stderrPayload`. +- **`crash(onCrash, payload)`** — dispatches to `allow`/`deny` per a policy + YOU supply. There is no default: call `crash(ON_CRASH, payload)` from your + outer catch, never a bare `crash(payload)`. + +A real guard shape: + +```js +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // declared once, near the top — see below + +try { + const data = JSON.parse(input); + if (looksFine(data)) allow(undefined); + emitBlock({ decision: 'block', reason: 'why this call is refused' }); +} catch { + // Hook errors must never block a legitimate tool call. + crash(ON_CRASH, undefined); +} + +function emitBlock(output) { + deny(output, output.reason); +} +``` + +## Choosing and declaring `ON_CRASH` + +`HOOK_ON_CRASH` has exactly two values, `ALLOW` and `DENY`. Ask: **if this +hook's own code throws, is it safer for the tool call to proceed, or safer to +block it?** + +- **`ALLOW`** — the hook is advisory, or its enforcement is not the last line + of defense. A crash here should not stop legitimate work. This is every + migrated hook's policy today (19 of 19) — including the two hard-blocking + guards (`gsd-write-guard.js`, `gsd-worktree-path-guard.js`): their threat + model is a confused agent overwriting a file it can already reach by other + means, not a determined adversary, so a hook bug is not treated as worse + than the thing it protects against. +- **`DENY`** — a crash inside a security-critical check is itself suspicious + enough that refusing is the safer failure. Nothing in this repo declares + `DENY` yet; if you are the first, that is a real decision, not a default — + write the reason down (see below) and expect it to be scrutinized in + review. + +Declare it once, near your hook's imports, not inline at the call site: + +```js +// This guard's outer catch has always exited 0 (fail open — a hook error +// must never block a legitimate tool call). Declared ONCE here so the outer +// catch's crash() call states its policy explicitly rather than inheriting +// a default (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; +``` + +**A useful reason names the failure mode, not the mechanism.** "Fails open" +restates what `ALLOW` already means. What the code cannot say is *why this +hook, specifically*: what breaks if it fires wrongly (a legitimate call +blocked) versus what breaks if it stays silent (a narrow threat model, a +check that runs again downstream, an advisory that was never load-bearing). +Write the trade-off, not the translation. + +## A deny needing a distinct stderr payload + +Most callers never pass `stderrPayload` — fd 2 gets the same bytes as fd 1, +unchanged from before this migration. One shape needs it: `gsd-write-guard.js` +guards a curated `.planning/` artifact, but Kimi's native hook bus reads +**stderr verbatim back to the model** on exit 2. A full JSON object on stderr +would show the model a data structure instead of a sentence, so this call +site sends the complete decision to stdout and only the plain-text reason to +stderr: + +```js +function emitBlock(output) { + deny(output, output.reason); +} +``` + +`stderrPayload` is forwarded to `terminateNow` verbatim: a **string** is +written raw (no `JSON.stringify`), anything else is JSON-stringified like the +stdout payload. Reach for this only when a specific downstream reader needs +plain text on stderr — most hooks have no such reader and should leave +`stderrPayload` unset. + +## The exception: two hooks that must NOT call `deny()` + +`allow()`/`deny()`/`crash()` assume the host reads the **exit code** as the +decision. Two shipped hooks have a different contract, where the decision +lives in the **JSON response body** and the process still exits 0: + +- **`gsd-read-injection-scanner.js`** (PostToolUse) — Claude Code's + PostToolUse protocol reads `{ decision: "block", ... }` from stdout, not + from the exit code; the tool call already completed by the time this hook + runs, so there is no exit-code channel to block it through. It calls + `process.stdout.write(JSON.stringify(output))` directly for its verdict, + and uses `allow()`/`crash()` only for its true no-op and crash paths. +- **`gsd-cursor-subagent-start.js`** (Cursor `subagentStart`) — Cursor's own + hook protocol reads `{ permission: "deny", user_message, ... }` from the + JSON body at exit 0; `"ask"` is not even a supported value, and there is no + exit-2 convention here at all. Same split: `process.stdout.write(...)` for + the decision, `allow()` only for its stdin-timeout no-op. + +If you are writing a hook against a harness that reads its verdict from the +response body, follow one of these two, not `deny()` — calling `deny()` there +would exit 2 into a harness that is not listening on the exit code, turning a +block into an unexplained crash. + +## When a check cannot run at all + +`crash()` covers your hook's own bugs. It does not cover the more common +defect: a check inside a *working* hook that silently reads "could not run" +as if it were "ran, found nothing" — passing every case it should have +inspected. #3838 is the worked example: `gsd-validate-commit.sh` had three +swallow-and-pass sites (the opt-in config read, JSON command extraction, and +its `isGitSubcommand` classifier) where a failure and a genuine negative both +fell through to the same `exit 0`, silently disabling commit validation on +any of them. The fix distinguishes the two outcomes and says so on stderr +before falling through: + +```sh +ENABLED=$(node -e "..." 2>"$ENABLED_ERR") || CONFIG_STATUS=$? +CONFIG_STATUS=${CONFIG_STATUS:-0} +if [ "$CONFIG_STATUS" != "0" ]; then + # Could not determine the opt-in flag at all — distinct from + # ".planning/config.json exists and legitimately disables the hook". + echo "gsd-validate-commit.sh: could not read .planning/config.json (opt-in check) — validator disabled for this call. $(cat "$ENABLED_ERR")" >&2 + rm -f "$ENABLED_ERR" + exit 0 +fi +``` + +The exit code is unchanged (still 0, still a pass) — what changed is that the +"could not run" case is no longer indistinguishable from a real negative: it +is diagnosable on stderr instead of vanishing. Apply the same discipline +inside your own hook: a `try { ... } catch { /* silently pass */ }` around a +check is the shape to be suspicious of, whether or not it happens to route +through `hook-exit.js` at all. + +## Related + +- [ADR-3889](../adr/3889-process-exit-contract.md) — the exit-code registry + `hook-exit.js` is layered over, and the rationale for banding codes 0/1/2 + the way it does +- [Hooks declare their crash policy](../features/hooks-declare-their-crash-policy.md) — + the feature summary for this migration +- [Resolve unreachable-guard findings](resolve-unreachable-guard-findings.md) — + a sibling "the loop surfaced something, here is what to do with it" page, + for the shell-guard-drift class rather than the exit-code class diff --git a/eslint.config.mjs b/eslint.config.mjs index e66eed4fc..2603f409a 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -347,6 +347,12 @@ export default tseslint.config( // (stricter than lint: it forbids ANY hand edit, not just bad ones). // Lint the src/cli-exit.cts source, not this emitted copy. 'scripts/lib/cli-exit.cjs', + // #3911 (ADR-3889 Phase 7): tsc-generated runtime artifact — generated + // by scripts/gen-hooks-cli-exit.cjs from the SAME fresh compile of + // src/cli-exit.cts (sibling registry require rewritten to `.js`), and + // byte-guarded by `npm run lint:generated-sync`. Lint the + // src/cli-exit.cts source, not this emitted copy. + 'hooks/lib/cli-exit.js', ], }, diff --git a/gsd-core/bin/lib/exit-code-registry.cjs b/gsd-core/bin/lib/exit-code-registry.cjs index 51af7339a..c6ea8dc24 100644 --- a/gsd-core/bin/lib/exit-code-registry.cjs +++ b/gsd-core/bin/lib/exit-code-registry.cjs @@ -3,10 +3,11 @@ // GENERATED FILE — DO NOT EDIT BY HAND. // Source of truth: gsd-core/bin/shared/exit-codes.json. Regenerate with: // node scripts/gen-exit-code-registry.cjs --write -// This exact content is emitted to TWO locations — gsd-core/bin/lib/exit-code-registry.cjs -// and scripts/lib/exit-code-registry.cjs (the latter committed so scripts/ -// consumers work on an unbuilt clone) — both byte-compared by -// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy). +// This exact content is emitted to THREE locations — gsd-core/bin/lib/exit-code-registry.cjs, +// scripts/lib/exit-code-registry.cjs, and hooks/lib/exit-code-registry.js (the latter two +// committed so scripts/ and hooks/ consumers work on an unbuilt clone) — all byte-compared by +// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy; +// #3911 ADR-3889 Phase 7 added the hooks/lib/ copy). // // exitCodeFor(name) / nameForExitCode(code) are pure and total over this // closed table — each throws for anything not registered here. diff --git a/hooks/gsd-agent-isolation-guard.js b/hooks/gsd-agent-isolation-guard.js index 21c664b3c..05c3d3766 100644 --- a/hooks/gsd-agent-isolation-guard.js +++ b/hooks/gsd-agent-isolation-guard.js @@ -65,6 +65,23 @@ const path = require('path'); const os = require('os'); const { readSentinel, VALID_ISOLATION, extractDispatchIdentifiers, sentinelAppliesToDispatch } = require('./lib/isolation-sentinel.js'); const { REASON_CODE } = require('./lib/isolation-deny-reason.js'); +const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); + +// Required at module top, alongside the other ./lib requires — NOT behind +// ensureRuntimeBuild() below. Terminating on a parse/timeout failure must +// never depend on the gitignored build artifacts this hook self-heals for +// its own registry lookups (#3911). +// +// This guard's outer catch (main(), below) has always exited 0 (fail open): +// that outer catch only covers payload PARSING failing before applicability +// could even be determined (malformed stdin JSON, etc.) — the guard's real +// fail-closed logic (a GSD project whose dispatch-isolation configuration +// cannot be verified) is handled separately, inside evaluateDispatch/ +// resolveIsolationState, and already returns a 'block' decision through the +// normal exit-2 path rather than through this catch. So an unparseable +// payload has nothing to enforce; allowing it preserves today's behavior +// exactly. +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // #3582: gsd-core/bin/lib/*.cjs (runtime-name-policy.cjs, capability-registry.cjs // below) are tsc build artifacts (ADR-457), gitignored and absent on a raw // plugin-marketplace / git-clone install that never ran `npm run build:lib`. @@ -476,7 +493,7 @@ function evaluateDispatch(data, { clock = Date } = {}) { /* istanbul ignore next -- stdin adapter, exercised via spawnSync in tests */ function main() { let input = ''; - const stdinTimeout = setTimeout(() => process.exit(0), 3000); + const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', (chunk) => { input += chunk; }); process.stdin.on('end', () => { @@ -486,18 +503,18 @@ function main() { const decision = evaluateDispatch(data); if (decision.action === 'block') { const out = { decision: 'block', reason: decision.reason, reason_code: decision.reasonCode }; - process.stdout.write(JSON.stringify(out)); // Kimi feeds stderr (not stdout) back to the model on exit 2. - process.stderr.write(decision.reason); - process.exit(2); + deny(out, decision.reason); } - process.exit(0); + allow(undefined); } catch { // Silent fail — never block valid tool calls due to hook errors // (malformed payload, etc.). This is distinct from resolveIsolationState's // internal error handling, which DOES deny — this outer catch only // covers payload parsing before applicability could even be determined. - process.exit(0); + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); } diff --git a/hooks/gsd-config-reload.js b/hooks/gsd-config-reload.js index 184bebf73..ad3e4b381 100644 --- a/hooks/gsd-config-reload.js +++ b/hooks/gsd-config-reload.js @@ -20,11 +20,17 @@ const fs = require('fs'); const path = require('path'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This hook only injects an advisory config-reload summary; a crash mid-parse +// must not block the session or the FileChanged event that triggered it — the +// agent simply keeps using the config context it already had (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; let input = ''; // Timeout guard: if stdin does not close within 8s exit silently rather than // hanging until Claude Code kills the process and reports "hook error". -const stdinTimeout = setTimeout(() => process.exit(0), 8000); +const stdinTimeout = setTimeout(() => allow(undefined), 8000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => (input += chunk)); process.stdin.on('end', () => { @@ -42,24 +48,23 @@ process.stdin.on('end', () => { // inject spurious additionalContext. const basename = path.basename(filePath); if (basename !== 'config.json') { - process.exit(0); + allow(undefined); } const expectedPath = path.resolve(cwd, '.planning', 'config.json'); if (path.resolve(filePath) !== expectedPath) { - process.exit(0); + allow(undefined); } // On unlink (deletion) emit a brief notice and exit if (event === 'unlink') { - process.stdout.write(JSON.stringify({ + allow({ hookSpecificOutput: { hookEventName: 'FileChanged', additionalContext: 'GSD config (.planning/config.json) was deleted. ' + 'Falling back to built-in defaults for this session.', }, - })); - process.exit(0); + }); } // Read the updated config file @@ -68,17 +73,16 @@ process.stdin.on('end', () => { const raw = fs.readFileSync(filePath, 'utf8'); config = JSON.parse(raw); } catch (e) { - if (e && e.code === 'ENOENT') process.exit(0); + if (e && e.code === 'ENOENT') allow(undefined); // Malformed JSON — inform the agent without crashing - process.stdout.write(JSON.stringify({ + allow({ hookSpecificOutput: { hookEventName: 'FileChanged', additionalContext: 'GSD config (.planning/config.json) was modified but could not be parsed. ' + 'Check the file for JSON syntax errors.', }, - })); - process.exit(0); + }); } // Build a concise summary of key config fields the agent cares about @@ -127,7 +131,9 @@ process.stdin.on('end', () => { }, })); } catch (e) { - // Silent fail — never block the session on a config reload error - process.exit(0); + // Silent fail — never block the session on a config reload error. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-context-monitor.js b/hooks/gsd-context-monitor.js index 656540cde..11059710e 100644 --- a/hooks/gsd-context-monitor.js +++ b/hooks/gsd-context-monitor.js @@ -22,6 +22,13 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { spawn } = require('child_process'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This hook only injects an advisory context-usage warning; it never blocks +// the tool call it rides in on. A crash here (e.g. a malformed bridge file) +// must not turn a PostToolUse advisory into a blocked tool call — losing a +// context warning is far cheaper than stalling the agent's work (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; const WARNING_THRESHOLD = 35; // remaining_percentage <= 35% const CRITICAL_THRESHOLD = 25; // remaining_percentage <= 25% @@ -33,7 +40,7 @@ let input = ''; // Windows/Git Bash, or slow Claude Code piping during large outputs), // exit silently instead of hanging until Claude Code kills the process // and reports "hook error". See #775, #1162. -const stdinTimeout = setTimeout(() => process.exit(0), 10000); +const stdinTimeout = setTimeout(() => allow(undefined), 10000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -43,14 +50,14 @@ process.stdin.on('end', () => { const sessionId = data.session_id; if (!sessionId) { - process.exit(0); + allow(undefined); } // Reject session IDs that contain path traversal sequences or path separators. // session_id is used to construct file paths in /tmp — an unsanitized value // could escape the temp directory and read or write arbitrary files. if (/[/\\]|\.\./.test(sessionId)) { - process.exit(0); + allow(undefined); } // Check if context warnings are disabled via config. @@ -61,7 +68,7 @@ process.stdin.on('end', () => { const configPath = path.join(cwd, '.planning', 'config.json'); const config = JSON.parse(fs.readFileSync(configPath, 'utf8')); if (config.hooks?.context_warnings === false) { - process.exit(0); + allow(undefined); } } catch (e) { // Missing or unparseable config → proceed with defaults (context warnings enabled) @@ -77,7 +84,7 @@ process.stdin.on('end', () => { try { metricsRaw = fs.readFileSync(metricsPath, 'utf8'); } catch (e) { - if (e && e.code === 'ENOENT') process.exit(0); + if (e && e.code === 'ENOENT') allow(undefined); throw e; } const metrics = JSON.parse(metricsRaw); @@ -85,7 +92,7 @@ process.stdin.on('end', () => { // Ignore stale metrics if (metrics.timestamp && (now - metrics.timestamp) > STALE_SECONDS) { - process.exit(0); + allow(undefined); } const remaining = metrics.remaining_percentage; @@ -93,7 +100,7 @@ process.stdin.on('end', () => { // No warning needed if (remaining > WARNING_THRESHOLD) { - process.exit(0); + allow(undefined); } // Debounce: check if we warned recently @@ -121,7 +128,7 @@ process.stdin.on('end', () => { if (!firstWarn && warnData.callsSinceWarn < DEBOUNCE_CALLS && !severityEscalated) { // Update counter and exit without warning fs.writeFileSync(warnPath, JSON.stringify(warnData)); - process.exit(0); + allow(undefined); } // Reset debounce counter @@ -208,7 +215,9 @@ process.stdin.on('end', () => { process.stdout.write(JSON.stringify(output)); } } catch (e) { - // Silent fail -- never block tool execution - process.exit(0); + // Silent fail -- never block tool execution. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-cursor-post-tool.js b/hooks/gsd-cursor-post-tool.js index 7f7dec769..add1c2df4 100644 --- a/hooks/gsd-cursor-post-tool.js +++ b/hooks/gsd-cursor-post-tool.js @@ -22,6 +22,8 @@ 'use strict'; +const { allow } = require('./lib/hook-exit.js'); + const WRITE_TOOL_RE = /write|edit|replace|create|delete|remove|append|apply|patch|insert|mkdir/i; const PATH_KEY_RE = /^(path|file|file_?path|filepath|target_?path|target|dir|directory|uri|filename)$/i; const PLANNING_PATH_RE = /(^|[\\/])\.planning([\\/]|$)/; @@ -29,7 +31,7 @@ const PLANNING_PATH_RE = /(^|[\\/])\.planning([\\/]|$)/; let raw = ''; const stdinTimeout = setTimeout(() => { // Timeout guard: exit silently rather than hanging. - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-cursor-pre-tool.js b/hooks/gsd-cursor-pre-tool.js index d0d96e4c2..858236c5c 100644 --- a/hooks/gsd-cursor-pre-tool.js +++ b/hooks/gsd-cursor-pre-tool.js @@ -22,13 +22,15 @@ 'use strict'; +const { allow } = require('./lib/hook-exit.js'); + const WRITE_TOOL_RE = /write|edit|replace|create|delete|remove|append|apply|patch|insert|mkdir/i; const PATH_KEY_RE = /^(path|file|file_?path|filepath|target_?path|target|dir|directory|uri|filename)$/i; const PLANNING_PATH_RE = /(^|[\\/])\.planning([\\/]|$)/; let raw = ''; const stdinTimeout = setTimeout(() => { - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-cursor-session-start.js b/hooks/gsd-cursor-session-start.js index 56f97ffef..75d41eaf9 100644 --- a/hooks/gsd-cursor-session-start.js +++ b/hooks/gsd-cursor-session-start.js @@ -23,6 +23,7 @@ 'use strict'; const fs = require('fs'); +const { allow } = require('./lib/hook-exit.js'); const MSG_PRESENT = 'GSD: .planning/STATE.md is present — review the current phase and any blockers before acting.'; @@ -37,7 +38,7 @@ const { resolveStatePath } = require('./lib/cursor-workspace.js'); let raw = ''; const stdinTimeout = setTimeout(() => { // Timeout guard: exit silently rather than hanging. - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-cursor-stop.js b/hooks/gsd-cursor-stop.js index ac2bcdebe..c39f42310 100644 --- a/hooks/gsd-cursor-stop.js +++ b/hooks/gsd-cursor-stop.js @@ -21,6 +21,7 @@ 'use strict'; const fs = require('fs'); +const { allow } = require('./lib/hook-exit.js'); // Workspace resolution is shared across the Cursor hooks (#2587) — see // hooks/lib/cursor-workspace.js. Staged next to these scripts by @@ -29,7 +30,7 @@ const { resolveStatePath } = require('./lib/cursor-workspace.js'); let raw = ''; const stdinTimeout = setTimeout(() => { - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-cursor-subagent-start.js b/hooks/gsd-cursor-subagent-start.js index f090c51cf..846486276 100644 --- a/hooks/gsd-cursor-subagent-start.js +++ b/hooks/gsd-cursor-subagent-start.js @@ -56,6 +56,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); +const { allow } = require('./lib/hook-exit.js'); // Workspace resolution is shared across the Cursor hooks (#2587) — see // hooks/lib/cursor-workspace.js. Staged next to these scripts by @@ -568,7 +569,7 @@ function evaluateRootIsolation(root, subagentType, { clock = Date, dispatchIds = function main() { let raw = ''; const stdinTimeout = setTimeout(() => { - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-cursor-subagent-stop.js b/hooks/gsd-cursor-subagent-stop.js index 48a3eecb7..2b0d7668f 100644 --- a/hooks/gsd-cursor-subagent-stop.js +++ b/hooks/gsd-cursor-subagent-stop.js @@ -20,8 +20,10 @@ 'use strict'; +const { allow } = require('./lib/hook-exit.js'); + const stdinTimeout = setTimeout(() => { - process.exit(0); + allow(undefined); }, 10000); process.stdin.setEncoding('utf8'); diff --git a/hooks/gsd-ensure-canonical-path.js b/hooks/gsd-ensure-canonical-path.js index 03092af4f..1b6bc4ebe 100644 --- a/hooks/gsd-ensure-canonical-path.js +++ b/hooks/gsd-ensure-canonical-path.js @@ -34,6 +34,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); +const { allow } = require('./lib/hook-exit.js'); // Immutable, bundled subdirectories that the canonical path must expose. These // are the directories `@~/.claude/gsd-core//...` includes point into. @@ -301,5 +302,5 @@ if (require.main === module) { } catch (_) { // Best-effort: a canonical-path failure must never abort a session. } - process.exit(0); + allow(undefined); } diff --git a/hooks/gsd-phase-boundary.sh b/hooks/gsd-phase-boundary.sh index d383424b3..6df86f640 100755 --- a/hooks/gsd-phase-boundary.sh +++ b/hooks/gsd-phase-boundary.sh @@ -6,6 +6,7 @@ # # OPT-IN: This hook is a no-op unless config.json has hooks.community: true. # Enable with: "hooks": { "community": true } in .planning/config.json +set -euo pipefail # Check opt-in config — exit silently if not enabled if [ -f .planning/config.json ]; then diff --git a/hooks/gsd-prompt-guard.js b/hooks/gsd-prompt-guard.js index d9c16f834..a222a58a9 100644 --- a/hooks/gsd-prompt-guard.js +++ b/hooks/gsd-prompt-guard.js @@ -12,6 +12,13 @@ // not to create false-positive deadlocks. const path = require('path'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This guard is advisory-only by design (see header) — it never blocks the +// Write/Edit it scans, only adds context about it. A crash here must not +// start blocking now, which is strictly worse than the advisory it exists +// to add on top of an already-permitted operation (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // Prompt injection patterns — shared with gsd-read-injection-scanner.js via // hooks/lib/injection-patterns.js so the two surfaces cannot drift (#3504). @@ -115,7 +122,7 @@ function normalizeKimiPayload(data) { } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 3000); +const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -126,7 +133,7 @@ process.stdin.on('end', () => { // Only scan Write and Edit operations if (toolName !== 'Write' && toolName !== 'Edit') { - process.exit(0); + allow(undefined); } // #2595 (review Major 3, sibling sweep): typed read. A non-string @@ -138,7 +145,7 @@ process.stdin.on('end', () => { // Only scan files going into .planning/ (agent context files) if (!filePath.includes('.planning/') && !filePath.includes('.planning\\')) { - process.exit(0); + allow(undefined); } // Get the content being written. #3504 (isolated review finding 3): the @@ -157,7 +164,7 @@ process.stdin.on('end', () => { } } if (!content) { - process.exit(0); + allow(undefined); } // Scan for injection patterns @@ -174,7 +181,7 @@ process.stdin.on('end', () => { } if (findings.length === 0) { - process.exit(0); + allow(undefined); } // Advisory warning — does not block the operation @@ -191,7 +198,9 @@ process.stdin.on('end', () => { process.stdout.write(JSON.stringify(output)); } catch { - // Silent fail — never block tool execution - process.exit(0); + // Silent fail — never block tool execution. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-read-guard.js b/hooks/gsd-read-guard.js index b4489ddce..92eb705fb 100644 --- a/hooks/gsd-read-guard.js +++ b/hooks/gsd-read-guard.js @@ -20,6 +20,13 @@ const fs = require('fs'); const path = require('path'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This guard is pure advisory UX — a reminder to Read before Write/Edit on +// non-Claude-Code runtimes. A crash here must not block a legitimate Write/ +// Edit; the worst outcome of failing open is the model hitting the runtime's +// own read-before-edit rejection it was trying to help avoid (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // #2304: Kimi's native hook bus delivers Kimi's tool vocabulary in the payload // (Write → WriteFile, Edit/MultiEdit → StrReplaceFile) while the [[hooks]] @@ -116,7 +123,7 @@ function normalizeKimiPayload(data) { } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 3000); +const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -127,7 +134,7 @@ process.stdin.on('end', () => { // Only intercept Write and Edit tool calls if (toolName !== 'Write' && toolName !== 'Edit') { - process.exit(0); + allow(undefined); } // Claude Code natively enforces read-before-edit — skip the advisory (#1984, #2344, #2520). @@ -151,7 +158,7 @@ process.stdin.on('end', () => { process.env.CLAUDE_SESSION_ID || process.env.CLAUDECODE; if (isClaudeCode) { - process.exit(0); + allow(undefined); } // #2595 (review Major 3, sibling sweep): typed read — same class as the @@ -160,7 +167,7 @@ process.stdin.on('end', () => { ? data.tool_input.file_path : ''; if (!filePath) { - process.exit(0); + allow(undefined); } // Only inject guidance when the file already exists. @@ -174,7 +181,7 @@ process.stdin.on('end', () => { } if (!fileExists) { - process.exit(0); + allow(undefined); } const fileName = path.basename(filePath); @@ -193,7 +200,9 @@ process.stdin.on('end', () => { process.stdout.write(JSON.stringify(output)); } catch { - // Silent fail — never block tool execution - process.exit(0); + // Silent fail — never block tool execution. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-read-injection-scanner.js b/hooks/gsd-read-injection-scanner.js index b89459970..a6bd6b265 100644 --- a/hooks/gsd-read-injection-scanner.js +++ b/hooks/gsd-read-injection-scanner.js @@ -23,6 +23,13 @@ const path = require('path'); const fs = require('fs'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This is a PostToolUse advisory scanner over content the tool call already +// returned; a crash while scanning must not retroactively block that Read/ +// WebFetch/WebSearch result from reaching the agent — losing the injection +// check is safer than failing the tool call it is only observing (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // Summarisation-specific patterns (novel — not in gsd-prompt-guard.js). // These target instructions specifically designed to survive context compression. @@ -213,7 +220,7 @@ function normalizeKimiPayload(data) { } let inputBuf = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 5000); +const stdinTimeout = setTimeout(() => allow(undefined), 5000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => { inputBuf += chunk; }); process.stdin.on('end', () => { @@ -224,7 +231,7 @@ process.stdin.on('end', () => { const toolName = data.tool_name; const SCANNED_TOOLS = new Set(['Read', 'WebFetch', 'WebSearch']); if (!SCANNED_TOOLS.has(toolName)) { - process.exit(0); + allow(undefined); } // Source label + path-exclusion (path-exclusion applies to file reads only) @@ -235,8 +242,8 @@ process.stdin.on('end', () => { source = typeof data.tool_input?.file_path === 'string' ? data.tool_input.file_path : ''; - if (!source) process.exit(0); - if (isExcludedPath(source)) process.exit(0); + if (!source) allow(undefined); + if (isExcludedPath(source)) allow(undefined); } else if (toolName === 'WebFetch') { source = data.tool_input?.url || 'web'; } else { // WebSearch @@ -261,7 +268,7 @@ process.stdin.on('end', () => { } if (!content || content.length < 20) { - process.exit(0); + allow(undefined); } // Typed findings IR — single source of truth for both the machine-readable @@ -307,7 +314,7 @@ process.stdin.on('end', () => { } if (findings.length === 0) { - process.exit(0); + allow(undefined); } // Renders one finding back into the exact prose fragment the advisory has @@ -349,7 +356,9 @@ process.stdin.on('end', () => { process.stdout.write(JSON.stringify(output)); } catch { - // Silent fail — never block tool execution - process.exit(0); + // Silent fail — never block tool execution. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-session-state.sh b/hooks/gsd-session-state.sh index 9b9a05df0..f35b340ba 100755 --- a/hooks/gsd-session-state.sh +++ b/hooks/gsd-session-state.sh @@ -5,6 +5,7 @@ # # OPT-IN: This hook is a no-op unless config.json has hooks.community: true. # Enable with: "hooks": { "community": true } in .planning/config.json +set -euo pipefail # Check opt-in config — exit silently if not enabled if [ -f .planning/config.json ]; then diff --git a/hooks/gsd-statusline.js b/hooks/gsd-statusline.js index c8067f5b8..680834226 100755 --- a/hooks/gsd-statusline.js +++ b/hooks/gsd-statusline.js @@ -9,6 +9,14 @@ const os = require('os'); // Namespace (not destructured) so tests can inject spawn failures by // monkeypatching childProcess.execFileSync. const childProcess = require('child_process'); +const { HOOK_ON_CRASH, allow, crash } = require('./lib/hook-exit.js'); + +// This hook's build-seam outer catch (require.main guard just below) has +// always exited 0 (fail open — the statusline renders on EVERY prompt, so a +// build failure must degrade to a blank line rather than break Claude Code's +// per-render hook). Declared ONCE so that catch's crash() call states its +// policy explicitly rather than inheriting a default (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; // #3582: gsd-core/bin/lib/*.cjs (semver-compare.cjs, state-document.cjs, // active-workstream-store.cjs, planning-workspace.cjs — required below) and // package-identity.cjs are tsc build artifacts (ADR-457), gitignored and @@ -23,8 +31,12 @@ if (require.main === module) { const { ensureRuntimeBuild } = require('../gsd-core/bin/ensure-runtime-build.cjs'); ensureRuntimeBuild(); } catch (e) { - process.stdout.write(''); - process.exit(0); + // #3911: crash(ON_CRASH, ...) with an undefined payload preserves the + // pre-migration `process.stdout.write(''); process.exit(0);` byte-for- + // byte — undefined makes terminateNow's stdout JSON.stringify throw + // internally (swallowed there), so fd 1 stays untouched, same as writing + // an explicit empty string did. + crash(ON_CRASH, undefined); } } const { isSemverNewer } = require('../gsd-core/bin/lib/semver-compare.cjs'); @@ -748,7 +760,7 @@ function runStatusline() { let input = ''; // Timeout guard: if stdin doesn't close within 3s (e.g. pipe issues on // Windows/Git Bash), exit silently instead of hanging. See #775. - const stdinTimeout = setTimeout(() => process.exit(0), 3000); + const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { diff --git a/hooks/gsd-validate-commit.sh b/hooks/gsd-validate-commit.sh index 7bd755241..62f6030d1 100755 --- a/hooks/gsd-validate-commit.sh +++ b/hooks/gsd-validate-commit.sh @@ -7,10 +7,43 @@ # # OPT-IN: This hook is a no-op unless config.json has hooks.community: true. # Enable with: "hooks": { "community": true } in .planning/config.json +set -euo pipefail + +# Temp files created below for subprocess stderr capture (config read, command +# extraction, classifier). A single EXIT trap replaces three hand-rolled +# mktemp/rm-f pairs so an early or unexpected exit path can never leak one — +# and a future fourth check does not need its own copy (#3911 review). +# Idempotent and failure-proof by construction: unset vars expand to "" (a +# no-op rm -f target), and `|| true` guarantees the trap itself never changes +# the script's exit status. +ENABLED_ERR="" +CMD_ERR="" +CLASSIFY_ERR="" +cleanup_temp_files() { + rm -f "${ENABLED_ERR:-}" "${CMD_ERR:-}" "${CLASSIFY_ERR:-}" 2>/dev/null || true +} +trap cleanup_temp_files EXIT # Check opt-in config — exit silently if not enabled if [ -f .planning/config.json ]; then - ENABLED=$(node -e "try{const c=require('./.planning/config.json');process.stdout.write(c.hooks?.community===true?'1':'0')}catch{process.stdout.write('0')}" 2>/dev/null) + ENABLED_ERR=$(mktemp) + ENABLED=$(node -e " + try{ + const c=require('./.planning/config.json'); + process.stdout.write(c.hooks?.community===true?'1':'0'); + }catch(e){ + process.stderr.write('CONFIG_READ_FAILED: '+(e&&e.message?e.message:String(e))); + process.exit(3); + } + " 2>"$ENABLED_ERR") || CONFIG_STATUS=$? + CONFIG_STATUS=${CONFIG_STATUS:-0} + if [ "$CONFIG_STATUS" != "0" ]; then + # Could not determine the opt-in flag at all (node missing, JSON parse + # error other than absence, etc.) — distinct from ".planning/config.json + # exists and legitimately disables the hook". Say so and pass, per #3838. + echo "gsd-validate-commit.sh: could not read .planning/config.json (opt-in check) — validator disabled for this call. $(cat "$ENABLED_ERR")" >&2 + exit 0 + fi if [ "$ENABLED" != "1" ]; then exit 0; fi else exit 0 @@ -19,17 +52,58 @@ fi INPUT=$(cat) # Extract command from JSON using Node (handles escaping correctly, no jq needed) -CMD=$(echo "$INPUT" | node -e "let d='';process.stdin.on('data',c=>d+=c);process.stdin.on('end',()=>{try{process.stdout.write(JSON.parse(d).tool_input?.command||'')}catch{}})" 2>/dev/null) +CMD_ERR=$(mktemp) +CMD=$(echo "$INPUT" | node -e " + let d=''; + process.stdin.on('data',c=>d+=c); + process.stdin.on('end',()=>{ + try{ + process.stdout.write(JSON.parse(d).tool_input?.command||''); + }catch(e){ + process.stderr.write('COMMAND_EXTRACTION_FAILED: '+(e&&e.message?e.message:String(e))); + process.exit(3); + } + }); +" 2>"$CMD_ERR") || CMD_STATUS=$? +CMD_STATUS=${CMD_STATUS:-0} +if [ "$CMD_STATUS" != "0" ]; then + # Could not extract tool_input.command at all (node missing, malformed + # JSON, etc.) — distinct from "there is genuinely no command field". Say + # so and pass, per #3838. + echo "gsd-validate-commit.sh: could not extract tool_input.command from the hook payload — validator disabled for this call. $(cat "$CMD_ERR")" >&2 + exit 0 +fi # Only check git commit commands. # Delegates to hooks/lib/git-cmd.js isGitSubcommand() — the canonical token-walk # classifier that handles env-prefix, -C path, and full-path git invocations. # A naive `^git\s+commit` regex misses all three; this guard fixes that (#3129). HOOK_DIR="$(cd "$(dirname "$0")" && pwd)" -if GIT_CMD_LIB="$HOOK_DIR/lib/git-cmd.js" node -e " - const {isGitSubcommand}=require(process.env.GIT_CMD_LIB); - process.exit(isGitSubcommand(process.argv[1],'commit')?0:1); -" "$CMD" 2>/dev/null; then +CLASSIFY_ERR=$(mktemp) +GIT_CMD_LIB="$HOOK_DIR/lib/git-cmd.js" node -e " + try { + const {isGitSubcommand}=require(process.env.GIT_CMD_LIB); + process.exit(isGitSubcommand(process.argv[1],'commit')?0:1); + } catch(e) { + process.stderr.write('CLASSIFIER_THREW: '+(e&&e.message?e.message:String(e))); + process.exit(3); + } +" "$CMD" 2>"$CLASSIFY_ERR" || CLASSIFY_STATUS=$? +CLASSIFY_STATUS=${CLASSIFY_STATUS:-0} +if [ "$CLASSIFY_STATUS" != "0" ] && [ "$CLASSIFY_STATUS" != "1" ]; then + # 0 = is a git commit (validate below); 1 = genuinely not a git commit + # (real negative, pass silently) — the ONLY intentional non-zero exit the + # script above ever produces on success. Any other status — 127 node + # missing, or 3 from the try/catch above when the git-cmd.js require chain + # throws (e.g. its built dependency, gsd-core/bin/lib/token-scanner.cjs, is + # a gitignored build artifact and absent on a fresh checkout — run + # `npm run build:lib`) — means the classifier could not run at all. Say so + # on stderr and pass (#3838): PreToolUse stderr does not disturb the JSON + # protocol. + echo "gsd-validate-commit.sh: could not classify the command via hooks/lib/git-cmd.js (exit $CLASSIFY_STATUS) — validator disabled for this call. If this persists, run \`npm run build:lib\`. $(cat "$CLASSIFY_ERR")" >&2 + exit 0 +fi +if [ "$CLASSIFY_STATUS" = "0" ]; then # Extract message from -m flag MSG="" if [[ "$CMD" =~ -m[[:space:]]+\"([^\"]+)\" ]]; then diff --git a/hooks/gsd-windsurf-pre-command.js b/hooks/gsd-windsurf-pre-command.js index 4b6483cf1..12615081d 100644 --- a/hooks/gsd-windsurf-pre-command.js +++ b/hooks/gsd-windsurf-pre-command.js @@ -47,6 +47,16 @@ 'use strict'; +const { allow, deny } = require('./lib/hook-exit.js'); + +// #3911 (ADR-3889 Phase 7): the exit(2) call site (block(), below) is +// migrated to hook-exit.js's deny(undefined, reason) now that +// hooks/lib/cli-exit.js's terminateNow emits fd 1 and fd 2 (stderrPayload) +// in INDEPENDENT try/catch blocks — `payload=undefined` cleanly skips the +// fd 1 write (preserving "nothing written to stdout") instead of throwing +// into a shared catch that used to also swallow the fd 2 write, which is +// what silently dropped the deny reason before this fix. + // No realistic destructive command comes anywhere close to this length. const MAX_COMMAND_LENGTH = 4096; @@ -244,16 +254,11 @@ function destructiveReason(cmd) { } function block(reason) { - process.stderr.write(`GSD windsurf pre_run_command guard: ${reason}\n`); - process.exit(2); -} - -function allow() { - process.exit(0); + deny(undefined, `GSD windsurf pre_run_command guard: ${reason}\n`); } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 10000); +const stdinTimeout = setTimeout(() => allow(undefined), 10000); process.stdin.setEncoding('utf8'); process.stdin.on('data', (chunk) => { input += chunk; }); process.stdin.on('end', () => { @@ -262,14 +267,14 @@ process.stdin.on('end', () => { const data = JSON.parse(input || '{}'); const toolInfo = (data && typeof data.tool_info === 'object' && data.tool_info) || {}; const commandLine = typeof toolInfo.command_line === 'string' ? toolInfo.command_line : ''; - if (!commandLine) { allow(); return; } - if (commandLine.length > MAX_COMMAND_LENGTH) { allow(); return; } + if (!commandLine) { allow(undefined); return; } + if (commandLine.length > MAX_COMMAND_LENGTH) { allow(undefined); return; } const reason = destructiveReason(commandLine); if (reason) { block(reason); return; } - allow(); + allow(undefined); } catch { // Silent fail-open — never block a valid tool call due to a hook bug. - allow(); + allow(undefined); } }); diff --git a/hooks/gsd-windsurf-pre-write.js b/hooks/gsd-windsurf-pre-write.js index cf0d020d2..23de75d68 100644 --- a/hooks/gsd-windsurf-pre-write.js +++ b/hooks/gsd-windsurf-pre-write.js @@ -30,6 +30,13 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { allow, deny } = require('./lib/hook-exit.js'); +const { reportIfUndetermined } = require('./lib/git-probe.js'); + +// #3911 (ADR-3889 Phase 7): the exit(2) call site (block(), below) is +// migrated to hook-exit.js's deny(undefined, reason) — see +// gsd-windsurf-pre-command.js's identical note for the fixed defect +// (terminateNow's fd 1/fd 2 writes now run in independent try/catch blocks). const SPAWNOPT = { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 2000, windowsHide: true }; @@ -54,16 +61,11 @@ function nearestExistingDir(start) { } function block(reason) { - process.stderr.write(`GSD windsurf pre_write_code guard: ${reason}\n`); - process.exit(2); -} - -function allow() { - process.exit(0); + deny(undefined, `GSD windsurf pre_write_code guard: ${reason}\n`); } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 10000); +const stdinTimeout = setTimeout(() => allow(undefined), 10000); process.stdin.setEncoding('utf8'); process.stdin.on('data', (chunk) => { input += chunk; }); process.stdin.on('end', () => { @@ -72,14 +74,19 @@ process.stdin.on('end', () => { const data = JSON.parse(input || '{}'); const toolInfo = (data && typeof data.tool_info === 'object' && data.tool_info) || {}; const rawFilePath = typeof toolInfo.file_path === 'string' ? toolInfo.file_path : ''; - if (!rawFilePath) { allow(); return; } + if (!rawFilePath) { allow(undefined); return; } const cwd = process.cwd(); // Determine the active project's git root. No git root at all -> nothing // to enforce a boundary against -> fail open. const cwdTopResult = git(['rev-parse', '--show-toplevel'], cwd); - if (cwdTopResult.status !== 0 || !cwdTopResult.stdout) { allow(); return; } + // #3911: a timeout/spawn-failure result is indistinguishable from a clean + // "no git root here" answer by status/stdout alone — reportIfUndetermined + // is a no-op on a genuine negative and only fires when the probe itself + // could not run. The allow() below is UNCHANGED either way. + reportIfUndetermined('gsd-windsurf-pre-write', 'git rev-parse --show-toplevel (cwd)', cwdTopResult); + if (cwdTopResult.status !== 0 || !cwdTopResult.stdout) { allow(undefined); return; } const cwdTopRaw = cwdTopResult.stdout.trim(); const filePath = path.isAbsolute(rawFilePath) ? path.resolve(rawFilePath) : path.resolve(cwd, rawFilePath); @@ -95,14 +102,16 @@ process.stdin.on('end', () => { } })(), ); - if (!checkDir) { allow(); return; } // synthetic path with no existing ancestor — fail open + if (!checkDir) { allow(undefined); return; } // synthetic path with no existing ancestor — fail open const fileTopResult = git(['rev-parse', '--show-toplevel'], checkDir); + reportIfUndetermined('gsd-windsurf-pre-write', 'git rev-parse --show-toplevel (file location)', fileTopResult); if (fileTopResult.status !== 0 || !fileTopResult.stdout) { // Not inside any git worktree. Distinguish "inside a .git/ internals // directory" (dangerous — BLOCK) from "outside all git repos entirely" // (not the escape vector this guard targets — fail open). const insideGitDir = git(['rev-parse', '--is-inside-git-dir'], checkDir); + reportIfUndetermined('gsd-windsurf-pre-write', 'git rev-parse --is-inside-git-dir', insideGitDir); if (insideGitDir.status === 0 && insideGitDir.stdout && insideGitDir.stdout.trim() === 'true') { block( `'${filePath}' is inside a git internal (.git) directory, not the active project at ` + @@ -111,12 +120,12 @@ process.stdin.on('end', () => { ); return; } - allow(); + allow(undefined); return; } const fileTopRaw = fileTopResult.stdout.trim(); - if (fileTopRaw === cwdTopRaw) { allow(); return; } + if (fileTopRaw === cwdTopRaw) { allow(undefined); return; } // BLOCK: file resolves to a different git root than the active project. block( @@ -127,6 +136,6 @@ process.stdin.on('end', () => { ); } catch { // Silent fail-open — never block a valid tool call due to a hook bug. - allow(); + allow(undefined); } }); diff --git a/hooks/gsd-workflow-guard.js b/hooks/gsd-workflow-guard.js index 390a35d1f..5b67a881c 100644 --- a/hooks/gsd-workflow-guard.js +++ b/hooks/gsd-workflow-guard.js @@ -21,6 +21,18 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); const { tokenize, skipToSubcommand } = require('./lib/git-cmd.js'); +const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); +const { reportIfUndetermined } = require('./lib/git-probe.js'); + +// This guard is almost entirely advisory (fail open — a broken advisory must +// never wedge every tool call), with ONE hard block (#3504 force-add-on- +// agent-branch) that fails CLOSED on internal error instead. That split is +// re-derived dynamically inside the outer catch (failClosedBlockContext), not +// a fixed per-hook policy, so ON_CRASH names only the FINAL, unconditional +// fallback reached when the fail-closed context does not apply — i.e. the +// historical exit(0). Declared ONCE here so that fallback states its policy +// explicitly rather than inheriting a default (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; function forceGitAddCwds(command, defaultCwd) { const tokens = tokenize(command || ''); @@ -82,6 +94,12 @@ function currentBranch(cwd) { timeout: 2000, killSignal: 'SIGTERM', }); + // #3911: a timeout/spawn-failure here previously degraded to '' exactly + // like a clean "not on a branch" answer — indistinguishable from the + // caller's point of view. reportIfUndetermined is a no-op on a genuine + // negative and only emits a diagnostic when the probe itself could not + // run; the '' fallback (and therefore this hook's exit code) is unchanged. + reportIfUndetermined('gsd-workflow-guard', 'git branch --show-current', result); if (result.status !== 0) return ''; return result.stdout.trim(); } @@ -114,9 +132,10 @@ function emitForceAddBlock(origin) { origin: failClosed ? 'fail-closed' : 'force-add-detected', reason, }; - process.stdout.write(JSON.stringify(output)); - // Kimi CLI's exit-2 protocol feeds stderr back to the model (#2304) - process.stderr.write(output.reason); + // Kimi CLI's exit-2 protocol feeds stderr back to the model (#2304) — deny's + // stderrPayload keeps fd 2 to the plain reason string, matching the + // pre-migration two-write byte-for-byte. + deny(output, output.reason); } // #3504 fail-closed context re-derivation. Runs INSIDE the outer catch, after @@ -260,7 +279,7 @@ function normalizeKimiPayload(data) { } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 3000); +const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -282,29 +301,28 @@ process.stdin.on('end', () => { if (toolName === 'Bash') { if (!isWorkflowGuardEnabled) { - process.exit(0); + allow(undefined); } const command = data.tool_input?.command || ''; for (const gitCwd of forceGitAddCwds(command, cwd)) { const branch = currentBranch(gitCwd); if (/^(worktree-)?agent-/.test(branch)) { emitForceAddBlock(); - process.exit(2); } } - process.exit(0); + allow(undefined); } // Only guard Write, Edit, and MultiEdit tool calls if (!['Write', 'Edit', 'MultiEdit'].includes(toolName)) { - process.exit(0); + allow(undefined); } // Check if we're inside a GSD workflow (Task subagent or /gsd- skill) // Subagents have a session_id that differs from the parent // and typically have a description field set by the orchestrator if (data.tool_input?.is_subagent || data.session_type === 'task') { - process.exit(0); + allow(undefined); } // Check the file being edited @@ -319,7 +337,7 @@ process.stdin.on('end', () => { // Allow edits to .planning/ files (GSD state management) if (filePath.includes('.planning/') || filePath.includes('.planning\\')) { - process.exit(0); + allow(undefined); } // Allow edits to common config/docs files that don't need GSD tracking @@ -332,11 +350,11 @@ process.stdin.on('end', () => { /settings\.json$/, ]; if (allowedPatterns.some(p => p.test(filePath))) { - process.exit(0); + allow(undefined); } if (!isWorkflowGuardEnabled) { - process.exit(0); // Guard disabled (default) or no GSD project + allow(undefined); // Guard disabled (default) or no GSD project } // If we get here: GSD project, guard enabled, file edit outside .planning/, @@ -357,13 +375,13 @@ process.stdin.on('end', () => { // #3504: split posture on internal error. The ONE hard block in this hook // (force-add on agent branches) fails CLOSED — if the blocking context can // be re-derived from the payload (Bash tool + guard enabled + determinably - // an agent branch), exit 2 rather than silently allowing. Everything else + // an agent branch), deny rather than silently allowing. Everything else // keeps the historical fail-open posture: a broken advisory guard must - // never wedge the session's tool calls. + // never wedge the session's tool calls. ON_CRASH is declared ALLOW at + // module top for exactly that unconditional fallback (#3911). if (failClosedBlockContext(input)) { emitForceAddBlock('fail-closed'); - process.exit(2); } - process.exit(0); + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-worktree-path-guard.js b/hooks/gsd-worktree-path-guard.js index 6f9425fd9..691577900 100644 --- a/hooks/gsd-worktree-path-guard.js +++ b/hooks/gsd-worktree-path-guard.js @@ -17,6 +17,16 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); +const { reportIfUndetermined } = require('./lib/git-probe.js'); + +// This guard's outer catch has always exited 0 (fail open): a path guard that +// cannot resolve the worktree must not block the user's edit — its whole job +// is a targeted containment check, not a general-purpose file-write blocker, +// and an unresolved worktree root gives it nothing to check against. Declared +// ONCE here so the outer catch's crash() call states its policy explicitly +// rather than inheriting a default (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; const SPAWNOPT = { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 2000, windowsHide: true }; @@ -132,7 +142,7 @@ function normalizeKimiPayload(data) { } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 3000); +const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -143,7 +153,7 @@ process.stdin.on('end', () => { // Only guard Edit, Write, and MultiEdit tool calls if (toolName !== 'Edit' && toolName !== 'Write' && toolName !== 'MultiEdit') { - process.exit(0); + allow(undefined); } const cwd = data.cwd || process.cwd(); @@ -154,15 +164,20 @@ process.stdin.on('end', () => { // In the main repo or a submodule it returns .git (or a path without /worktrees/). // This approach works even when cwd is a subdirectory of the worktree. const gitDirResult = git(['rev-parse', '--git-dir'], cwd); + // #3911: a timeout/spawn-failure result is indistinguishable from a clean + // "not a git repo" answer by status/stdout alone — reportIfUndetermined + // is a no-op on a genuine negative and only fires the diagnostic when the + // probe itself could not run. The allow() below is UNCHANGED either way. + reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --git-dir', gitDirResult); if (gitDirResult.status !== 0 || !gitDirResult.stdout) { - process.exit(0); // not a git repo — pass through + allow(undefined); // not a git repo — pass through } const gitDir = gitDirResult.stdout.trim(); // A linked worktree's --git-dir contains .git/worktrees/ as a path component const isLinkedWorktree = /[/\\]\.git[/\\]worktrees[/\\]/.test(gitDir); if (!isLinkedWorktree) { - process.exit(0); // main repo, submodule, or separate-git-dir — no-op + allow(undefined); // main repo, submodule, or separate-git-dir — no-op } // #1342: Only enforce inside a GSD-managed isolated executor worktree. Those @@ -172,18 +187,20 @@ process.stdin.on('end', () => { // on the user's own branch, so the guard must be a no-op there. Detached HEAD // / error → not GSD-managed → no-op. const branchResult = git(['symbolic-ref', '--short', 'HEAD'], cwd); + reportIfUndetermined('gsd-worktree-path-guard', 'git symbolic-ref --short HEAD', branchResult); const branch = branchResult.status === 0 && branchResult.stdout ? branchResult.stdout.trim() : ''; // #3021: accept worktree-wf_- branches (Workflow backend's naming). if (!/^((worktree-)?agent-|worktree-wf_)[A-Za-z0-9._/-]+$/.test(branch)) { - process.exit(0); // not a GSD-managed executor worktree — no-op + allow(undefined); // not a GSD-managed executor worktree — no-op } // Get the raw --show-toplevel output for the worktree (cwd). // We keep it raw (not path.resolve'd) to compare directly with the // file's toplevel — same git binary, same format, no normalization needed. const wtTopResult = git(['rev-parse', '--show-toplevel'], cwd); + reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --show-toplevel (worktree cwd)', wtTopResult); if (wtTopResult.status !== 0 || !wtTopResult.stdout) { - process.exit(0); // can't determine root — fail open + allow(undefined); // can't determine root — fail open } const wtTopRaw = wtTopResult.stdout.trim(); @@ -201,7 +218,7 @@ process.stdin.on('end', () => { ? data.tool_input.file_path : ''; if (!rawFilePath) { - process.exit(0); + allow(undefined); } // Relative paths resolve against the tool's CWD, which is inside the worktree @@ -219,7 +236,7 @@ process.stdin.on('end', () => { // `../`-laden path exits 0 at this line and escapes the worktree. Stating a // mechanism and an unverified premise — not asserting a live bypass. if (!path.isAbsolute(rawFilePath)) { - process.exit(0); + allow(undefined); } // Normalise .. traversal so /worktree/src/../../../main/file @@ -244,7 +261,7 @@ process.stdin.on('end', () => { // Walked to root without finding any directory — path is synthetic. // A path with no existing ancestor is not the #260 main-repo vector; // #260 is caught by the different-git-root branch below. Fail open. (#1342) - process.exit(0); + allow(undefined); } // Ask git for the toplevel of the file's location. @@ -254,6 +271,7 @@ process.stdin.on('end', () => { // back-slash inconsistencies) — both values come from the same git binary // in the same format by definition. const fileTopResult = git(['rev-parse', '--show-toplevel'], checkDir); + reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --show-toplevel (file location)', fileTopResult); if (fileTopResult.status !== 0 || !fileTopResult.stdout) { // The target's location is not a git work tree. Two sub-cases: @@ -263,6 +281,7 @@ process.stdin.on('end', () => { // - Truly outside all git repositories (e.g. ~/.claude/plans/) → not the // main-repo vector → fail open. (#1342) const insideGitDir = git(['rev-parse', '--is-inside-git-dir'], checkDir); + reportIfUndetermined('gsd-worktree-path-guard', 'git rev-parse --is-inside-git-dir', insideGitDir); if (insideGitDir.status === 0 && insideGitDir.stdout && insideGitDir.stdout.trim() === 'true') { const output = { decision: 'block', @@ -271,20 +290,17 @@ process.stdin.on('end', () => { `not the active worktree at '${wtTopRaw}'. Writing to repository internals via an ` + `absolute path is not permitted from an isolated executor worktree. Use a relative path.`, }; - process.stdout.write(JSON.stringify(output)); - // Kimi feeds stderr (not stdout) back to the model on exit 2. - process.stderr.write(output.reason); - process.exit(2); + deny(output, output.reason); } // Outside all git repositories — fail open (#1342). - process.exit(0); + allow(undefined); } const fileTopRaw = fileTopResult.stdout.trim(); // Same git toplevel → file is inside the worktree → allow if (fileTopRaw === wtTopRaw) { - process.exit(0); + allow(undefined); } // BLOCK: file resolves to a different git root than the active worktree @@ -299,12 +315,11 @@ process.stdin.on('end', () => { `(hook cwd: '${cwd}').`, }; - process.stdout.write(JSON.stringify(output)); - // Kimi feeds stderr (not stdout) back to the model on exit 2. - process.stderr.write(output.reason); - process.exit(2); + deny(output, output.reason); } catch { - // Silent fail — never block valid tool calls due to hook errors - process.exit(0); + // Silent fail — never block valid tool calls due to hook errors. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/gsd-write-guard.js b/hooks/gsd-write-guard.js index e602b6d02..b85bc734e 100644 --- a/hooks/gsd-write-guard.js +++ b/hooks/gsd-write-guard.js @@ -75,6 +75,21 @@ const fs = require('fs'); const path = require('path'); +const { HOOK_ON_CRASH, allow, deny, crash } = require('./lib/hook-exit.js'); + +// This guard's outer catch has always exited 0 (fail open — a hook error +// must never block a legitimate tool call; see the emitBlock/consumeSentinelFor +// header comments). Declared ONCE here so the outer catch's crash() call +// states its policy explicitly rather than inheriting a default (#3911). +const ON_CRASH = HOOK_ON_CRASH.ALLOW; + +// #3911 (ADR-3889 Phase 7) NOTE: the exit(2) call site (emitBlock, below) is +// migrated to hooks/lib/hook-exit.js's deny(), using its `stderrPayload` +// param (added for exactly this site): fd 1 still gets the full JSON +// `output`, fd 2 gets ONLY the plain-text `output.reason` string — because +// Kimi's native hook bus reads stderr verbatim back to the model, and a raw +// JSON-stringified object on stderr is not the same reason text a model +// should read. Byte-identical to the pre-migration emitBlock. // Block when the pending payload has fewer than this fraction of the on-disk // line count (0.4 → a Write shrinking a file below 40% of its current size). @@ -158,20 +173,13 @@ function consumeSentinelFor(filePath, normalized) { // m2 (round 5): the block emission must itself be exception-safe. An EPIPE // from writeSync inside the outer try would land in the fail-OPEN catch — -// the one outcome the fail-closed branches exist to prevent. The decision -// stands regardless of whether the payload could be delivered. +// the one outcome the fail-closed branches exist to prevent. terminateNow +// (via deny()) already guarantees this: a failed write never changes the +// exit code and never throws out of the call. `output.reason` is passed as +// the distinct stderrPayload so fd 2 gets the plain reason string — not the +// full JSON `output` fd 1 gets — matching the pre-migration byte-for-byte. function emitBlock(output) { - try { - // writeSync: pipe writes via process.stdout/stderr are async on Windows - // and process.exit() does not flush them — a truncated block payload is - // a guard that silently half-fired. - fs.writeSync(1, JSON.stringify(output)); - // Kimi feeds stderr (not stdout) back to the model on exit 2. - fs.writeSync(2, output.reason); - } catch { - // Emission failed; the block still stands. - } - process.exit(2); + deny(output, output.reason); } // #2304: Kimi's native hook bus delivers Kimi's tool vocabulary in the payload @@ -223,7 +231,7 @@ function normalizeKimiPayload(data) { } let input = ''; -const stdinTimeout = setTimeout(() => process.exit(0), 3000); +const stdinTimeout = setTimeout(() => allow(undefined), 3000); process.stdin.setEncoding('utf8'); process.stdin.on('data', chunk => input += chunk); process.stdin.on('end', () => { @@ -234,17 +242,17 @@ process.stdin.on('end', () => { // A null/primitive payload has nothing to guard — exit deliberately // rather than throwing into the fail-open catch below (#2595 class). if (data === null || typeof data !== 'object') { - process.exit(0); + allow(undefined); } // Only whole-file Write is catastrophic-by-construction; Edit/MultiEdit // replace bounded spans and are out of scope by design (#2255). if (data.tool_name !== 'Write') { - process.exit(0); + allow(undefined); } if (isOverrideSet()) { - process.exit(0); // documented escape hatch — legitimate reset in progress + allow(undefined); // documented escape hatch — legitimate reset in progress } // Typed read (#2547 class): `[]`/`{}` are truthy, pass a `!value` @@ -254,7 +262,7 @@ process.stdin.on('end', () => { const rawFilePath = typeof rawInput?.file_path === 'string' ? rawInput.file_path : ''; const content = rawInput?.content; if (!rawFilePath || typeof content !== 'string') { - process.exit(0); + allow(undefined); } // Resolve relative paths against the session cwd (the same base the @@ -272,7 +280,7 @@ process.stdin.on('end', () => { const normalized = filePath.replace(/\\/g, '/'); if (!CURATED_PATTERNS.some(re => re.test(normalized))) { - process.exit(0); // not a curated planning artifact + allow(undefined); // not a curated planning artifact } // Only guard overwrites — creating a curated file fresh is fine. @@ -285,7 +293,7 @@ process.stdin.on('end', () => { onDisk = fs.readFileSync(filePath, 'utf8'); } catch (err) { if (err && err.code === 'ENOENT') { - process.exit(0); // does not exist — new-file Write, nothing to clobber + allow(undefined); // does not exist — new-file Write, nothing to clobber } emitBlock({ decision: 'block', @@ -306,17 +314,17 @@ process.stdin.on('end', () => { const newLines = countLines(content); if (oldLines < FLOOR_LINES) { - process.exit(0); // sub-floor stub — ratio checks are meaningless here + allow(undefined); // sub-floor stub — ratio checks are meaningless here } if (newLines >= oldLines * SHRINK_RATIO) { - process.exit(0); // shrink (if any) is within tolerance + allow(undefined); // shrink (if any) is within tolerance } // The mechanical hatch for workflow steps (see header): consulted only // here, at the block point, so a within-tolerance write never burns it. if (consumeSentinelFor(filePath, normalized)) { - process.exit(0); // armed for exactly this file, fresh, now consumed + allow(undefined); // armed for exactly this file, fresh, now consumed } const pct = Math.round((newLines / oldLines) * 100); @@ -353,7 +361,9 @@ process.stdin.on('end', () => { `to bypass this guard once.`, }); } catch { - // Silent fail — never block valid tool calls due to hook errors - process.exit(0); + // Silent fail — never block valid tool calls due to hook errors. + // ON_CRASH is declared ALLOW at module top: this preserves today's + // exit(0) fail-open behavior exactly (#3911). + crash(ON_CRASH, undefined); } }); diff --git a/hooks/lib/cli-exit.js b/hooks/lib/cli-exit.js new file mode 100644 index 000000000..c1f14ad48 --- /dev/null +++ b/hooks/lib/cli-exit.js @@ -0,0 +1,461 @@ +// GENERATED FILE — DO NOT EDIT BY HAND. +// Source of truth: src/cli-exit.cts. Regenerate with: +// node scripts/gen-hooks-cli-exit.cjs --write +// Byte-compared by `npm run lint:generated-sync` (#3911, ADR-3889 Phase 7). +// +// Why this copy exists: hooks/ runs straight from a raw, unbuilt clone — a +// shipped hook must be able to `require('./lib/cli-exit.js')` relative to +// its own __dirname and terminate through `terminateNow` without depending +// on any build artifact. gsd-core/bin/lib/cli-exit.cjs is gitignored tsc +// output and doubles as the build sentinel, so it cannot be required from +// here. `.js`, not `.cjs`, to match the hooks/lib/*.js convention. Hence one +// source, three emitted locations (gsd-core/bin/lib, scripts/lib, hooks/lib). + +"use strict"; +var __importDefault = (this && this.__importDefault) || function (mod) { + return (mod && mod.__esModule) ? mod : { "default": mod }; +}; +/** + * Process-exit primitives (ExitError, runMain, terminateNow) plus the + * json-error-mode and contract-version cells. + * + * Must import nothing but `node:fs` and `./exit-code-registry.cjs` — this + * source is emitted to TWO locations, gsd-core/bin/lib/cli-exit.cjs (tsc + * build output) and scripts/lib/cli-exit.cjs (a generated, committed + * artifact regenerated by scripts/gen-scripts-cli-exit.cjs), and the latter + * must load on an unbuilt clone before anything under ./lib exists. The + * registry require is safe here for the same reason: scripts/gen-exit-code- + * registry.cjs (ADR-3889 Phase 1/2, #3905/#3906) dual-emits its OWN sibling + * artifact, exit-code-registry.cjs, into both of these exact locations, so + * a relative `./exit-code-registry.cjs` resolves next to whichever copy of + * this module loaded it. + */ +const node_fs_1 = __importDefault(require("node:fs")); +// eslint-disable-next-line @typescript-eslint/no-require-imports +const exitCodeRegistryModule = require("./exit-code-registry.js"); +// Called only as exitCodeRegistryModule.exitCodeFor(...), never destructured: +// @typescript-eslint/unbound-method flags a bare function-typed property +// pulled off an object at the point of destructuring, since a detached +// reference COULD be called with the wrong `this` — keeping the member +// access qualified sidesteps that regardless of whether the callee ever +// actually touches `this` (it does not; exitCodeFor is pure). +const exitCodeFor = (name) => exitCodeRegistryModule.exitCodeFor(name); +/** + * The wire value `runMain` stamps into its structured envelope. Declared HERE, + * not in io.cts, because this module must not import anything (see the module + * header): io.cts builds ERROR_REASON.SDK_FAIL_FAST from this constant, so the + * two surfaces share ONE definition rather than two literals kept in step by a + * parity test. + */ +const EXIT_ENVELOPE_REASON = 'sdk_fail_fast'; +/** + * Process-level flag: when true, error paths emit structured JSON to stderr + * instead of plain text. Set by gsd-tools.cjs when the CLI is invoked with + * `--json-errors`; re-exported by io.cts, which is where most callers reach it. + * + * Held in a Symbol-keyed cell on globalThis rather than in module scope, and + * that is load-bearing: this module is emitted to TWO locations + * (gsd-core/bin/lib/cli-exit.cjs and the generated scripts/lib/cli-exit.cjs), + * so a process that loads both would get two independent module instances. A + * module-level `let` would give them two independent flags — one copy could + * think json mode is on while the other thought it was off, which is exactly + * the divergence class ADR-3889 exists to remove. One cell, keyed by a + * registry Symbol, makes that unrepresentable. + */ +const JSON_ERROR_MODE_KEY = Symbol.for('gsd.exit.jsonErrorMode'); +function setJsonErrorMode(v) { + globalThis[JSON_ERROR_MODE_KEY] = !!v; +} +function getJsonErrorMode() { + return globalThis[JSON_ERROR_MODE_KEY] === true; +} +/** The single registered name code 2 may ever be produced for (ADR-3889 §1). */ +const HOOK_DENY_NAME = 'HOOK_DENY'; +const HOOK_DENY_CODE = exitCodeFor(HOOK_DENY_NAME); +/** + * Currently-resolved exit-contract version (ADR-3889 §4). Held in a + * Symbol-keyed globalThis cell rather than a module-level `let`, for the + * exact reason JSON_ERROR_MODE_KEY is (see its comment above): this module + * is emitted to two locations and thus loaded as two independent module + * instances in any process that requires both, so a module-level variable + * would let those two instances disagree about which contract is active. + * `resolveContractVersion` is the only writer; `terminateNow`/`runMain` + * read it internally when projecting a declared outcome. + */ +const CONTRACT_VERSION_KEY = Symbol.for('gsd.exit.contractVersion'); +function setContractVersion(v) { + globalThis[CONTRACT_VERSION_KEY] = v; +} +/** + * Resolve the active exit-contract version, wiring the ambient process to the + * two terminators (ADR-3889 §4/§3). Mirrors how JSON_ERROR_MODE_KEY already + * works: a process-global cell means no entrypoint needs per-call wiring, so + * a `scripts/` tool or a hook gets the same behaviour as `gsd-tools` without + * this module touching either (P8 owns `gsd-tools`; P7 owns hooks). + * + * Precedence: if the cell already holds an explicit version, that wins — + * this is what lets `setContractVersion` override the ambient process (a + * later `GSD_EXIT_CONTRACT=v2` in the same process must NOT unseat an + * explicit `setContractVersion('v1')` call). Otherwise resolve from argv/env + * via `resolveContractVersion`, which itself persists the result into the + * cell — so this is a one-time resolution per process; every later read is + * just the cached cell value. An invalid ambient value (e.g. `v3`) is NOT + * softened to a silent v1 here: `resolveContractVersion` throws, and that + * throw propagates — swallowing it would reintroduce the "nothing fails with + * success" defect ADR-3889 exists to close, on the very selector meant to + * demonstrate the fix. Absent both flag and env, resolution still yields + * 'v1' (the documented default) and that too gets memoized. + */ +function getContractVersion() { + const cached = globalThis[CONTRACT_VERSION_KEY]; + if (cached === 'v1' || cached === 'v2') + return cached; + return resolveContractVersion({ argv: process.argv, env: process.env }); +} +/** + * Project a declared outcome onto an integer exit code for a given contract + * version. Pure and total over its own input space: throws for anything not + * an exact-case registered name (mirrors exitCodeFor's contract) or an + * unrecognized version — it never returns undefined/NaN. + * + * PASS/FAIL and every registered name project IDENTICALLY under v1 and v2 + * (registered names are version-invariant) — the sole exception is DEGRADED: + * + * v1: DEGRADED -> 0. Deliberate, NOT a bug: ADR-2980 ratified 60 + * `output({error})` call sites that already exit 0 on a payload-carried + * error, and ADR-2980's own "Revisit if" clause is what ADR-3889 §4 + * answers — normalizing this to a non-zero code was explicitly + * DECLINED there on measured blast radius. A future reader must not + * "fix" this to look more consistent with v2; the inconsistency IS the + * compatibility boundary. + * v2: DEGRADED -> exitCodeFor('DEGRADED') (80). Looked up through the + * registry, never hardcoded, so a re-allocation of DEGRADED's code + * cannot silently desync this projection from the shipped table. + */ +function projectOutcome(outcome, version) { + if (typeof outcome !== 'string' || outcome.length === 0) { + throw new Error(`projectOutcome: outcome must be a non-empty string, received ${JSON.stringify(outcome)}`); + } + if (version !== 'v1' && version !== 'v2') { + throw new Error(`projectOutcome: version must be 'v1' or 'v2', received ${JSON.stringify(version)}`); + } + if (outcome === 'PASS') + return 0; + if (outcome === 'FAIL') + return 1; + if (outcome === 'DEGRADED') + return version === 'v1' ? 0 : exitCodeFor('DEGRADED'); + // Any other registered name: version-invariant, resolved through the + // registry (throws for anything unregistered/empty/non-string/wrong-case — + // exitCodeFor's own contract, which this function inherits verbatim). + return exitCodeFor(outcome); +} +const EXIT_CONTRACT_FLAG_PREFIX = '--exit-contract='; +/** Scan argv for the FIRST `--exit-contract=` token; undefined if absent. */ +function findExitContractFlag(argv) { + for (const arg of argv) { + if (typeof arg === 'string' && arg.startsWith(EXIT_CONTRACT_FLAG_PREFIX)) { + return arg.slice(EXIT_CONTRACT_FLAG_PREFIX.length); + } + } + return undefined; +} +/** + * Resolve which exit-contract version is active from argv/env, per ADR-3889 + * §4, and persist it to the shared contract-version cell so a later + * `terminateNow`/`runMain` call (through EITHER module copy) projects + * against it without re-parsing argv/env itself. + * + * Precedence: an explicit `--exit-contract=` flag BEATS + * `GSD_EXIT_CONTRACT`, in both directions (flag=v1 + env=v2 -> v1; flag=v2 + + * env=v1 -> v2). Neither present -> 'v1' (the documented default). An empty + * env var reads as UNSET, not as an explicit empty selection — a shell that + * exports `GSD_EXIT_CONTRACT=` with nothing after the `=` must not silently + * select a version. + * + * Casing is decided, not accidental: only the exact lowercase tokens `v1`/ + * `v2` are accepted (matching every example in ADR-3889 and this module's own + * usage docs, both of which write `v2` never `V2`). Anything else recognized + * as PRESENT but not a valid version — `v3`, `garbage`, or an explicitly + * empty flag value (`--exit-contract=`) — THROWS rather than silently + * defaulting to v1. A selector for a contract whose whole thesis is "nothing + * fails with success" must not itself fail open. + */ +function resolveContractVersion(opts = {}) { + const argv = opts.argv ?? process.argv; + const env = opts.env ?? process.env; + const flagValue = findExitContractFlag(argv); + const rawEnvValue = env.GSD_EXIT_CONTRACT; + const envValue = rawEnvValue === undefined || rawEnvValue === '' ? undefined : rawEnvValue; + const selected = flagValue !== undefined ? flagValue : envValue; + let resolved; + if (selected === undefined) { + resolved = 'v1'; + } + else if (selected === 'v1' || selected === 'v2') { + resolved = selected; + } + else { + throw new Error(`resolveContractVersion: unrecognized exit-contract version ${JSON.stringify(selected)} ` + + `(expected 'v1' or 'v2')`); + } + setContractVersion(resolved); + return resolved; +} +/** + * Error carrying a process exit code. CLI logic throws this instead of calling + * process.exit() (banned by n/no-process-exit); runMain() translates it into + * process.exitCode at the entrypoint. + */ +class ExitError extends Error { + code; + hasUserMessage; + constructor(code = 1, message) { + super(message === undefined ? `process exit ${code}` : message); + this.name = 'ExitError'; + this.code = code; + this.hasUserMessage = message !== undefined; + } +} +/** + * Run a CLI main and translate its outcome into process.exitCode (never + * process.exit, so n/no-process-exit stays satisfied; output flushes and + * process.on('exit') cleanup still fires — this is precisely why runMain and + * terminateNow are two different functions: drain-then-exit vs write-then- + * terminate). main may be sync or async. Every arm below except the new + * string one is UNCHANGED from before ADR-3889 Phase 2: + * number return -> process.exitCode = it (unchanged) + * string return -> NEW: process.exitCode = projectOutcome(result, getContractVersion()), + * UNLESS that projection is the HOOK_DENY exit code (see + * the refusal below — 2 may only be produced by terminateNow). + * thrown ExitError -> process.exitCode = err.code (+ stderr err.message if hasUserMessage && code!=0) (unchanged) + * other throw -> when json-error mode is active, emits structured { ok:false, reason, message } + * to stderr; otherwise writes raw stack trace. exit code = 1 in either case. (unchanged) + */ +function runMain(main) { + Promise.resolve() + .then(() => main()) + .then((result) => { + if (typeof result === 'number') { + process.exitCode = result; + return; + } + if (typeof result === 'string') { + const projected = projectOutcome(result, getContractVersion()); + // ADR-3889 §3: exit code 2 (the hook-protocol deny) may + // ONLY be produced by terminateNow, never by runMain. runMain is + // drain-then-exit; a deny drained this way can be truncated on + // Windows, which is exactly why terminateNow (write-then-terminate) + // exists. Gated on the PROJECTED code, not on the literal string + // `'HOOK_DENY'`, so a future registry rename that still resolves to + // this code cannot slip past the guard. + if (projected === HOOK_DENY_CODE) { + process.stderr.write(`runMain: refusing to exit with code ${HOOK_DENY_CODE} — outcome ${JSON.stringify(result)} ` + + `projects to the ${HOOK_DENY_NAME} exit code, which is reserved to terminateNow. ` + + `A hook-protocol deny must be delivered write-then-terminate via terminateNow(${JSON.stringify(result)}, payload), ` + + 'never drain-then-exit via runMain — a drained deny can be truncated on Windows. ' + + 'This is a caller bug: runMain must not be given a main() that returns HOOK_DENY.\n'); + process.exitCode = exitCodeFor('INTERNAL'); + return; + } + process.exitCode = projected; + return; + } + }) + .catch((err) => { + if (err instanceof ExitError) { + if (err.hasUserMessage && err.code !== 0) + process.stderr.write(`${err.message}\n`); + process.exitCode = err.code; + return; + } + if (getJsonErrorMode()) { + const e = err; + const payload = JSON.stringify({ + ok: false, + reason: EXIT_ENVELOPE_REASON, + message: (e && e.message) ? e.message : String(err), + }) + '\n'; + node_fs_1.default.writeSync(2, payload); + } + else { + const e = err; + process.stderr.write(`${e && e.stack ? e.stack : String(err)}\n`); + } + process.exitCode = 1; + }); +} +/** + * Write `payload` fully to fd 1 (and, for a deny, fd 2 too) and terminate the + * process IMMEDIATELY with `outcome` projected through the current contract + * version. This is write-then-terminate, the other half of ADR-3889 §3's + * "two terminators over one registry": hooks fire from contexts (e.g. a + * `setTimeout` stdin-timeout guard) where `process.exitCode = N; return;` + * terminates nothing, so they need an immediate, synchronous exit — the + * exact gap `eslint.config.mjs:563-582` documents for `hooks/**`. + * + * This is THE ONLY sanctioned `process.exit` call site in the repo, and the + * only place exit code 2 can be produced: 2 is reserved to the hook-adapter + * protocol (ADR-3889 §1), and the registry's own one-owner rule already + * guarantees no other registered name resolves to it — the check below is a + * defense-in-depth assertion of that invariant, not the sole thing enforcing + * it. + * + * @param outcome - declared outcome name, projected via projectOutcome. + * @param payload - JSON-serializable value written to fd 1 (and, on a deny + * for which no `stderrPayload` is given, fd 2 too — this is the + * backward-compatible default every existing caller relies on). + * @param stderrPayload - optional, deny-only. When omitted (the default), + * fd 2 gets the SAME serialized `payload` fd 1 got — unchanged behavior. + * When provided, fd 2 gets THIS instead: a string is written raw + * (verbatim, not JSON-stringified), anything else is JSON-stringified + * like `payload`. This exists because `hooks/gsd-write-guard.js`'s + * emitBlock does NOT write the same bytes to both streams today — it + * writes the full JSON `output` to stdout but only the plain-text + * `output.reason` STRING to stderr, because Kimi's native hook bus reads + * stderr verbatim back to the model on exit 2. Migrating that call site + * onto terminateNow requires a way to say "fd 2 gets this different, + * plain-text value" — `stderrPayload` is that seam. Ignored entirely for + * a non-deny outcome: stderr is a deny-only channel. + * + * PAYLOAD-SIZE CONSTRAINT FOR CALLERS (measured for #3906, relevant to P7/ + * #3911 wiring 19 enforcement hooks onto this function): the write-until- + * drained loop above delivers a payload whole regardless of size — verified + * up to 1MB (Node's own `spawnSync` default `maxBuffer`) with no truncation + * and no stall, both with a concurrently-draining async reader (~30ms for a + * 256KB payload) and with the default (internally-drained) pipe stdio a + * spawnSync-based test harness gets for free. Node's `spawnSync` does NOT + * suffer the classic "child blocks writing past the pipe buffer because + * nothing on the parent side is reading yet" deadlock some other languages' + * synchronous-subprocess primitives have; it drains stdout/stderr + * concurrently at the libuv layer while the child runs. The constraint that + * DOES bite on Linux is unrelated to pipe buffering: `execve(2)` enforces + * `MAX_ARG_STRLEN` (128KiB per single argv/envp string; see `man execve` + * NOTES) — so a CALLER that embeds a large literal payload directly into a + * spawned command line (e.g. `node -e "......"`) can fail to + * even start the child on Linux (macOS has no equivalent per-string cap), + * with no relation to this function's own behavior. See + * tests/cli-exit.test.cjs's "a large payload (bigger than a pipe buffer) + * arrives whole" test, which hit exactly this constructing its own fixture + * before being rewritten to build the payload inside the child instead. + */ +function terminateNow(outcome, payload, stderrPayload) { + // terminateNow is total by construction: its callers are enforcement hooks + // (P7/#3911, 19 of them) whose OWN outer catch may fail open (some end in + // `process.exit(0)`). If resolving the contract version, projecting the + // outcome, or the HOOK_DENY-collision guard below threw and that throw + // propagated out of this function, it would unwind straight into that + // caller's catch — turning a deny into a silent allow, exactly the defect + // ADR-3889 exists to close. So every one of those steps is wrapped here: + // on ANY failure this still terminates, deterministically, with INTERNAL + // (never by returning or re-throwing) — a malformed call is a programming + // error to be diagnosed on stderr, not a reason to hand control back. + let versionForDiagnostics = '(unresolved)'; + try { + const version = getContractVersion(); + versionForDiagnostics = version; + const projected = projectOutcome(outcome, version); + if (projected === HOOK_DENY_CODE && outcome !== HOOK_DENY_NAME) { + throw new Error(`terminateNow: exit code ${HOOK_DENY_CODE} is reserved to the ${HOOK_DENY_NAME} outcome; ` + + `got outcome ${JSON.stringify(outcome)}`); + } + // m2 (round 5, hooks/gsd-write-guard.js:159-175): emission must itself be + // exception-safe. A failed write (EPIPE, a full pipe buffer, a throwing + // fs.writeSync in a test) must NOT change the exit code — if it propagated + // out of this function, a caller whose payload could not be delivered + // would fall into ITS OWN outer catch and fail OPEN, which is the exact + // outcome the fail-closed branches this function serves exist to prevent. + // The decision to terminate with `projected` stands regardless of whether + // the payload could be delivered. + // + // The two streams are emitted in TWO SEPARATE try/catch blocks, not one + // shared block (the pre-#3911 defect): fd 1 and fd 2 (deny-only) are + // independent channels with independent failure modes, and a shared try + // meant a serialization failure on fd 1 (e.g. `payload` throwing on + // JSON.stringify) aborted the block before fd 2 ever ran — silently + // dropping a deny's reason. `deny(undefined, 'some reason')` used to exit + // 2 with EMPTY stderr because of exactly this. Each block independently + // treats an `undefined` value for ITS OWN stream as "nothing to write" + // and skips the write cleanly, rather than serializing `undefined` (which + // is not valid JSON text) and throwing into the catch. + try { + // fs.writeSync, never process.stdout.write: pipe writes via + // process.stdout/stderr are async on Windows, and process.exit() below + // does not wait for them to flush — a truncated payload is a silent + // half-emission. Looped over a Buffer (not a bare string call) so a + // payload larger than the destination pipe's buffer — where a single + // write() syscall can legitimately return fewer bytes written than + // requested — still arrives whole rather than truncated. + if (payload !== undefined) { + const buf = Buffer.from(JSON.stringify(payload), 'utf8'); + let offset = 0; + while (offset < buf.length) { + offset += node_fs_1.default.writeSync(1, buf, offset, buf.length - offset); + } + } + } + catch { + // fd 1 emission failed; the exit code decision still stands (see + // above), and fd 2 below is unaffected — it has its own try/catch. + } + if (projected === HOOK_DENY_CODE) { + try { + // Backward-compatible default: when no `stderrPayload` is given, fd 2 + // gets the SAME value fd 1 got (still subject to fd 2's own + // undefined-skips-the-write and string-vs-JSON rules below). + const resolvedStderr = stderrPayload === undefined ? payload : stderrPayload; + if (resolvedStderr !== undefined) { + const stderrBuf = Buffer.from(typeof resolvedStderr === 'string' ? resolvedStderr : JSON.stringify(resolvedStderr), 'utf8'); + let stderrOffset = 0; + while (stderrOffset < stderrBuf.length) { + stderrOffset += node_fs_1.default.writeSync(2, stderrBuf, stderrOffset, stderrBuf.length - stderrOffset); + } + } + } + catch { + // fd 2 emission failed; independent of fd 1 above, and the exit code + // decision still stands regardless. + } + } + // n/no-process-exit is not registered for src/**/*.cts (see the ADR-3889 + // reference note in the module header) and both compiled .cjs copies of + // this module are lint-ignored build/generated artifacts, so no disable + // directive is needed here for the one sanctioned process.exit call site. + process.exit(projected); + } + catch (err) { + // Anything above threw: an unrecognized --exit-contract/GSD_EXIT_CONTRACT + // value, a non-string/empty/unregistered `outcome`, or the HOOK_DENY + // collision guard. Diagnose on stderr — swallowing this silently would + // make a typo'd outcome name or a bad contract-version env var + // undebuggable — then terminate unconditionally. The diagnostic write + // itself gets its own swallow-on-failure guard, because even a failed + // diagnostic must not stop the exit below from happening. + try { + const detail = err instanceof Error ? err.message : String(err); + const message = `terminateNow: programming error — outcome=${JSON.stringify(outcome)} ` + + `version=${JSON.stringify(versionForDiagnostics)}: ${detail}\n` + + `This is a caller bug (unrecognized outcome/exit-contract, or the HOOK_DENY collision ` + + `guard), not a declared outcome. Terminating with INTERNAL rather than propagating: an ` + + `enforcement-hook caller's own outer catch may fail open (process.exit(0)), and unwinding ` + + `into it here would silently convert a deny into an allow.\n`; + node_fs_1.default.writeSync(2, message); + } + catch { + // Diagnostic emission itself failed; the exit below is unconditional + // regardless. + } + process.exit(exitCodeFor('INTERNAL')); + } +} +module.exports = { + ExitError, + runMain, + setJsonErrorMode, + getJsonErrorMode, + EXIT_ENVELOPE_REASON, + projectOutcome, + resolveContractVersion, + getContractVersion, + terminateNow, +}; diff --git a/hooks/lib/exit-code-registry.js b/hooks/lib/exit-code-registry.js new file mode 100644 index 000000000..c6ea8dc24 --- /dev/null +++ b/hooks/lib/exit-code-registry.js @@ -0,0 +1,98 @@ +'use strict'; + +// GENERATED FILE — DO NOT EDIT BY HAND. +// Source of truth: gsd-core/bin/shared/exit-codes.json. Regenerate with: +// node scripts/gen-exit-code-registry.cjs --write +// This exact content is emitted to THREE locations — gsd-core/bin/lib/exit-code-registry.cjs, +// scripts/lib/exit-code-registry.cjs, and hooks/lib/exit-code-registry.js (the latter two +// committed so scripts/ and hooks/ consumers work on an unbuilt clone) — all byte-compared by +// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy; +// #3911 ADR-3889 Phase 7 added the hooks/lib/ copy). +// +// exitCodeFor(name) / nameForExitCode(code) are pure and total over this +// closed table — each throws for anything not registered here. + +const EXIT_CODES = Object.freeze([ + Object.freeze({ + code: 2, + name: "HOOK_DENY", + meaning: "Hook protocol deny — the harness blocks the tool call", + owner: "hook-adapter", + authorizedBy: "ADR-3889", + }), + Object.freeze({ + code: 64, + name: "USAGE", + meaning: "Caller error — bad argv, unknown subcommand, missing argument", + owner: "generic", + authorizedBy: "ADR-3889", + }), + Object.freeze({ + code: 66, + name: "NO_INPUT", + meaning: "Ran; zero units were in scope, and that emptiness is known to be genuine", + owner: "generic", + authorizedBy: "ADR-3889", + }), + Object.freeze({ + code: 69, + name: "UNAVAILABLE", + meaning: "Could not run — prerequisite absent, input unreadable, scope unestablished", + owner: "generic", + authorizedBy: "ADR-3889", + }), + Object.freeze({ + code: 70, + name: "INTERNAL", + meaning: "Self-failure — crash, timeout, killed subprocess", + owner: "generic", + authorizedBy: "ADR-3889", + }), + Object.freeze({ + code: 80, + name: "DEGRADED", + meaning: "Ran to completion and is reporting a condition through its result payload rather than as a process failure", + owner: "gsd-tools", + authorizedBy: "ADR-3889 + ADR-2980", + }) +]); + +const NAME_TO_CODE = new Map(EXIT_CODES.map((entry) => [entry.name, entry.code])); +const CODE_TO_NAME = new Map(EXIT_CODES.map((entry) => [entry.code, entry.name])); + +/** + * Resolve the registered exit code for a symbolic name. Pure, total: throws + * for anything not an exact, registered, exact-case key — including + * non-strings, the empty string, untrimmed strings, wrong case, and + * prototype-chain names like `__proto__`/`constructor`/`toString` (a Map + * lookup never touches the prototype chain, so these are indistinguishable + * from any other unregistered name). + * + * @param {string} name + * @returns {number} + */ +function exitCodeFor(name) { + if (typeof name !== 'string' || name.length === 0) { + throw new Error(`exitCodeFor: name must be a non-empty string, received ${JSON.stringify(name)}`); + } + if (!NAME_TO_CODE.has(name)) { + throw new Error(`exitCodeFor: unregistered exit code name: ${JSON.stringify(name)}`); + } + return NAME_TO_CODE.get(name); +} + +/** + * Reverse of exitCodeFor: resolve the symbolic name for a registered exit + * code. Pure, total: throws for anything not an exact, registered code. + * + * @param {number} code + * @returns {string} + */ +function nameForExitCode(code) { + if (!CODE_TO_NAME.has(code)) { + throw new Error(`nameForExitCode: unregistered exit code: ${JSON.stringify(code)}`); + } + return CODE_TO_NAME.get(code); +} + +module.exports = { EXIT_CODES, exitCodeFor, nameForExitCode }; diff --git a/hooks/lib/git-probe.js b/hooks/lib/git-probe.js new file mode 100644 index 000000000..6b0005212 --- /dev/null +++ b/hooks/lib/git-probe.js @@ -0,0 +1,84 @@ +'use strict'; +// hooks/lib/git-probe.js — classify a bounded spawnSync(git, ...) result, +// distinguishing a GENUINE negative answer (git ran and said "no") from +// "could not determine" (timeout / spawn failure / signal kill) — #3911. +// +// Proven defect: hooks/gsd-worktree-path-guard.js, hooks/gsd-workflow-guard.js, +// and hooks/gsd-windsurf-pre-write.js each treat spawnSync(git, ..., {timeout}) +// returning a non-zero/empty result as "not applicable" and allow silently. +// A macOS CI run showed three deny cases land at 2084ms/2112ms/2177ms — just +// past the 2000ms budget — each returning exit 0 with EMPTY stdout AND EMPTY +// stderr: a timeout is INDISTINGUISHABLE, at the call site, from git cleanly +// answering "not a repo" / "not this worktree" unless the spawnSync() result +// itself is inspected for `error`/`signal`, not just `status`/`stdout`. +// +// This module does not change any hook's decision or exit code — it only +// answers "was this probe's answer real?" so a caller can emit a diagnostic +// on the undetermined branch while keeping the existing fail-open behavior. +// +// Classification mirrors tests/helpers/process-seam.cjs's OUTCOME vocabulary +// (EXITED vs TIMED_OUT/SPAWN_FAILED/KILLED) — same evidence-over-classification +// ordering (a populated `status` outranks an attached `error`, since Node can +// report `error.code === 'ETIMEDOUT'` on a result that also carried a real +// exit that beat the timer) — so a probe here and a subprocess outcome in the +// test suite never disagree on the same raw spawnSync() shape. + +/** + * @param {{error?: (Error & {code?: string})|null, status: number|null, signal: string|null}} result + * the raw return value of `spawnSync('git', ...)`. + * @returns {{determined: true} | {determined: false, reason: string}} + */ +function classifyGitProbe(result) { + const error = result && result.error; + const status = result ? result.status : null; + const signal = result ? result.signal : null; + + if (!error && signal === null) { + // A real exit — status 0 or non-zero is a genuine, trustworthy answer. + return { determined: true }; + } + if (status !== null) { + // `status` is only ever populated by a real exit; it outranks an attached + // `error` even at the exact timeout boundary (process-seam.cjs's own + // "evidence over classification" rule). + return { determined: true }; + } + + const code = error ? (error.code ?? null) : null; + let reason; + if (code === 'ETIMEDOUT' || signal !== null) { + reason = signal ? `timed out (signal ${signal})` : 'timed out'; + } else if (code) { + reason = `spawn failed (${code})`; + } else { + reason = 'could not run (unknown reason)'; + } + return { determined: false, reason }; +} + +/** + * If `result` could not be determined (timeout / spawn failure / signal + * kill), write a stderr diagnostic naming the probe and why — never changes + * any exit code, never throws. A no-op when the probe genuinely ran and + * answered (status 0 or non-zero with no error/signal). + * + * @param {string} hookName - e.g. 'gsd-worktree-path-guard' + * @param {string} probeLabel - e.g. "git rev-parse --show-toplevel" + * @param {*} result - the raw spawnSync() return value + */ +function reportIfUndetermined(hookName, probeLabel, result) { + const classification = classifyGitProbe(result); + if (classification.determined) return; + try { + process.stderr.write( + `${hookName}: git probe '${probeLabel}' ${classification.reason} — ` + + `allowing this call because the probe's answer is unknown, not because ` + + `git answered "no" (#3911).\n` + ); + } catch { + // A stderr write failing (EPIPE, closed fd) must never itself change the + // hook's fail-open outcome. + } +} + +module.exports = { classifyGitProbe, reportIfUndetermined }; diff --git a/hooks/lib/hook-exit.js b/hooks/lib/hook-exit.js new file mode 100644 index 000000000..d37000590 --- /dev/null +++ b/hooks/lib/hook-exit.js @@ -0,0 +1,81 @@ +'use strict'; +// hooks/lib/hook-exit.js — hand-written, NOT generated. A tiny hook-facing +// vocabulary layered over hooks/lib/cli-exit.js's terminateNow (ADR-3889 §3), +// for the 19 enforcement hooks P7/#3911 is migrating off raw process.exit(). +// +// WHY onCrash is a REQUIRED argument, with no default (this is the entire +// point of this module): every hook audited for #3911 ended its outer +// try/catch in ONE of two ways — some `process.exit(0)` (fail open: a hook +// error must never block a legitimate tool call), others `process.exit(2)` +// (fail closed: a hook whose whole job is a security/safety denial must not +// let its own bug wave the denial through). Both are legitimate policies for +// DIFFERENT hooks — but neither is the "obviously correct" one, and a shared +// helper that silently defaulted to either would let a future hook inherit +// the wrong policy by omission, which is exactly the "nothing fails with +// success" defect ADR-3889 exists to close. Requiring the caller to name its +// policy at every call site — not just once at module load — makes "I forgot +// to decide" a load-time/call-time crash instead of a silent behavior. A +// caller that truly wants "allow on crash" must still write ALLOW; there is +// no path that reaches terminateNow without a declared choice. +// +// crash() itself is total: it does not throw on a bad `onCrash`, because +// throwing would unwind into the CALLER's own outer catch — the exact +// fail-open-by-accident hazard this module removes. Instead an unrecognized +// policy terminates via terminateNow('INTERNAL', ...) with a diagnostic +// payload naming the offending value, so a typo'd/misspelled policy is +// debuggable on stderr rather than silently reinterpreted as ALLOW or DENY. +const { terminateNow } = require('./cli-exit.js'); + +/** Frozen enum of the two declarable crash policies. No third option. */ +const HOOK_ON_CRASH = Object.freeze({ + ALLOW: 'allow', + DENY: 'deny', +}); + +/** Terminate with the hook-protocol PASS outcome (exit 0). */ +function allow(payload) { + terminateNow('PASS', payload); +} + +/** + * Terminate with the hook-protocol deny outcome (exit 2, stdout+stderr). + * + * @param {*} payload - JSON-serializable value written to fd 1 (and, when + * `stderrPayload` is omitted, fd 2 too). + * @param {*} [stderrPayload] - optional distinct value for fd 2, forwarded + * verbatim to terminateNow's own `stderrPayload` param — a string is + * written raw, anything else is JSON-stringified. + */ +function deny(payload, stderrPayload) { + terminateNow('HOOK_DENY', payload, stderrPayload); +} + +/** + * Dispatch to allow()/deny() per a DECLARED crash policy. `onCrash` is + * required — see the module header for why there is no default. + * + * @param {'allow'|'deny'} onCrash - one of HOOK_ON_CRASH's two values. + * @param {*} payload - JSON-serializable value forwarded to terminateNow. + */ +function crash(onCrash, payload) { + if (onCrash === HOOK_ON_CRASH.ALLOW) { + allow(payload); + return; + } + if (onCrash === HOOK_ON_CRASH.DENY) { + deny(payload); + return; + } + // Never guess a policy and never throw (a throw would unwind into the + // caller's own outer catch — the fail-open-by-accident defect this module + // exists to remove). Terminate deterministically with INTERNAL, naming the + // offending value so a misspelled/omitted policy is diagnosable. + terminateNow('INTERNAL', { + error: 'hook-exit: crash() called with an unrecognized onCrash policy', + receivedOnCrash: onCrash, + expectedOneOf: [HOOK_ON_CRASH.ALLOW, HOOK_ON_CRASH.DENY], + originalPayload: payload, + }); +} + +module.exports = { HOOK_ON_CRASH, allow, deny, crash }; diff --git a/package.json b/package.json index f303d3f94..e29ba5799 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,7 @@ "gen:registry": "node scripts/gen-registry.cjs --write", "gen:install-tree": "node scripts/gen-install-tree-fixtures.cjs", "gen:section-manifest": "node scripts/gen-section-manifest.cjs --write", - "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree && node scripts/gen-scripts-cli-exit.cjs --write && node scripts/gen-exit-code-registry.cjs --write", + "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree && node scripts/gen-scripts-cli-exit.cjs --write && node scripts/gen-hooks-cli-exit.cjs --write && node scripts/gen-exit-code-registry.cjs --write", "validate:registry": "node scripts/validate-registry.cjs", "prepack": "npm run build:lib", "prepare": "npm run build:lib", @@ -129,7 +129,7 @@ "lint:test-file-count": "node scripts/lint-test-file-count.cjs", "lint:pr-checks": "node scripts/lint-pr-check-project-dir.cjs", "lint:changeset": "node scripts/changeset/lint.cjs", - "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check && node scripts/gen-scripts-cli-exit.cjs --check && node scripts/gen-exit-code-registry.cjs --check", + "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check && node scripts/gen-scripts-cli-exit.cjs --check && node scripts/gen-hooks-cli-exit.cjs --check && node scripts/gen-exit-code-registry.cjs --check", "lint:docs": "node scripts/lint-docs-required.cjs", "lint:qa-smells": "node scripts/qa-smell-ratchet.cjs", "lint:legacy-name": "node scripts/lint-legacy-dir-name.cjs", diff --git a/scripts/gen-exit-code-registry.cjs b/scripts/gen-exit-code-registry.cjs index 2d043d9ef..a5695f723 100644 --- a/scripts/gen-exit-code-registry.cjs +++ b/scripts/gen-exit-code-registry.cjs @@ -1,12 +1,18 @@ #!/usr/bin/env node /** - * gen-exit-code-registry.cjs — generates FOUR byte-identical/derived + * gen-exit-code-registry.cjs — generates FIVE byte-identical/derived * artifacts from the declaration at gsd-core/bin/shared/exit-codes.json: * - gsd-core/bin/lib/exit-code-registry.cjs (tsc-adjacent build tree) * - scripts/lib/exit-code-registry.cjs (committed, for scripts/ * consumers that must work on an unbuilt clone — same reason * scripts/lib/cli-exit.cjs exists alongside gsd-core/bin/lib/cli-exit.cjs; * see scripts/gen-scripts-cli-exit.cjs). + * - hooks/lib/exit-code-registry.js (committed, for hooks/ + * consumers that must work on a raw, unbuilt clone — same reason as the + * scripts/ copy above; see scripts/gen-hooks-cli-exit.cjs, which emits + * hooks/lib/cli-exit.js's sibling `require('./exit-code-registry.js')`. + * `.js`, not `.cjs`, to match the hooks/lib/*.js convention — ADR-3889 + * Phase 7, #3911). * - src/exit-code-registry.d.cts (the ambient type declaration * tsc uses to typecheck src/cli-exit.cts's `require('./exit-code-registry.cjs')` * against the shape the .cjs artifacts above actually export — generated @@ -24,22 +30,24 @@ * the review finding that the .d.cts was hand-maintained with no gate by * generating it here too; Phase 4 (#3908) added the shell fragment so the * three bash scanners can source symbolic names instead of hardcoding - * integers. + * integers; Phase 7 (#3911) added the hooks/lib/ copy so a shipped hook can + * terminate through `terminateNow` without depending on any build artifact. * - * The two .cjs artifacts are byte-identical: serializeRegistry() only encodes - * the DECLARATION path (for the banner comment), never the output path, so - * one generated string is written to both locations unchanged. The .d.cts - * and .sh artifacts are separate, smaller derivations but are generated and - * --check-gated exactly the same way. + * The three .cjs/.js artifacts (primary, scripts, hooks) are byte-identical: + * serializeRegistry() only encodes the DECLARATION path (for the banner + * comment), never the output path, so one generated string is written to all + * three locations unchanged. The .d.cts and .sh artifacts are separate, + * smaller derivations but are generated and --check-gated exactly the same + * way. * * Nothing in this script emits a registered exit code itself; wiring * consumers onto the registry is separate work. * * Usage: * node scripts/gen-exit-code-registry.cjs # same as --write - * node scripts/gen-exit-code-registry.cjs --write # write all four artifacts + * node scripts/gen-exit-code-registry.cjs --write # write all five artifacts * node scripts/gen-exit-code-registry.cjs --check # exit 1 if ANY committed artifact is stale - * node scripts/gen-exit-code-registry.cjs --declaration --out --scripts-out --dts-out --sh-out # override for tests + * node scripts/gen-exit-code-registry.cjs --declaration --out --scripts-out --hooks-out --dts-out --sh-out # override for tests * node scripts/gen-exit-code-registry.cjs --json # emit ONE JSON report on stdout instead of human prose */ @@ -52,6 +60,7 @@ const REPO_ROOT = path.resolve(__dirname, '..'); const DEFAULT_DECLARATION_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'shared', 'exit-codes.json'); const DEFAULT_OUTPUT_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'lib', 'exit-code-registry.cjs'); const DEFAULT_SCRIPTS_OUTPUT_PATH = path.join(REPO_ROOT, 'scripts', 'lib', 'exit-code-registry.cjs'); +const DEFAULT_HOOKS_OUTPUT_PATH = path.join(REPO_ROOT, 'hooks', 'lib', 'exit-code-registry.js'); const DEFAULT_DTS_OUTPUT_PATH = path.join(REPO_ROOT, 'src', 'exit-code-registry.d.cts'); const DEFAULT_SH_OUTPUT_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'shared', 'exit-codes.sh'); @@ -89,13 +98,14 @@ const REASON = Object.freeze({ }); const USAGE_MESSAGE = [ - 'Usage: node scripts/gen-exit-code-registry.cjs [--write|--check] [--declaration ] [--out ] [--scripts-out ] [--dts-out ] [--sh-out ] [--json]', + 'Usage: node scripts/gen-exit-code-registry.cjs [--write|--check] [--declaration ] [--out ] [--scripts-out ] [--hooks-out ] [--dts-out ] [--sh-out ] [--json]', ' (no flag) same as --write', - ' --write write all four generated registry artifacts', + ' --write write all five generated registry artifacts', ' --check exit 1 if ANY committed artifact is stale', ' --declaration override the declaration path (default: gsd-core/bin/shared/exit-codes.json)', ' --out override the primary output artifact path (default: gsd-core/bin/lib/exit-code-registry.cjs)', ' --scripts-out override the secondary output artifact path (default: scripts/lib/exit-code-registry.cjs)', + ' --hooks-out override the hooks output artifact path (default: hooks/lib/exit-code-registry.js)', ' --dts-out override the ambient type declaration path (default: src/exit-code-registry.d.cts)', ' --sh-out override the shell-sourceable fragment path (default: gsd-core/bin/shared/exit-codes.sh)', ' --json emit ONE JSON report ({ok, reason, context, detail?}) on stdout instead of human-readable prose', @@ -318,10 +328,11 @@ function serializeRegistry(entries, declarationPath) { '// GENERATED FILE — DO NOT EDIT BY HAND.', `// Source of truth: ${relDeclaration}. Regenerate with:`, '// node scripts/gen-exit-code-registry.cjs --write', - '// This exact content is emitted to TWO locations — gsd-core/bin/lib/exit-code-registry.cjs', - '// and scripts/lib/exit-code-registry.cjs (the latter committed so scripts/', - '// consumers work on an unbuilt clone) — both byte-compared by', - '// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy).', + '// This exact content is emitted to THREE locations — gsd-core/bin/lib/exit-code-registry.cjs,', + '// scripts/lib/exit-code-registry.cjs, and hooks/lib/exit-code-registry.js (the latter two', + '// committed so scripts/ and hooks/ consumers work on an unbuilt clone) — all byte-compared by', + '// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy;', + '// #3911 ADR-3889 Phase 7 added the hooks/lib/ copy).', '//', '// exitCodeFor(name) / nameForExitCode(code) are pure and total over this', '// closed table — each throws for anything not registered here.', @@ -531,16 +542,16 @@ function emitOk(reason, humanMessage, json) { console.log(humanMessage); } -function doWrite(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json) { +function doWrite(declarationPath, outPath, scriptsOutPath, hooksOutPath, dtsPath, shPath, json) { const result = buildRegistryContent(declarationPath); if (!result.ok) { emitFail(result, json); return 1; } - // The two .cjs artifacts are byte-identical copies of the same generated - // content (serializeRegistry never encodes the output path), so the same - // string is written to both locations unchanged. - for (const target of [outPath, scriptsOutPath]) { + // The three .cjs/.js artifacts are byte-identical copies of the same + // generated content (serializeRegistry never encodes the output path), so + // the same string is written to all three locations unchanged. + for (const target of [outPath, scriptsOutPath, hooksOutPath]) { fs.mkdirSync(path.dirname(target), { recursive: true }); fs.writeFileSync(target, result.content, 'utf8'); } @@ -553,6 +564,7 @@ function doWrite(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json emitOk( REASON.OK, `ok gen-exit-code-registry: wrote ${outPath}\nok gen-exit-code-registry: wrote ${scriptsOutPath}\n` + + `ok gen-exit-code-registry: wrote ${hooksOutPath}\n` + `ok gen-exit-code-registry: wrote ${dtsPath}\nok gen-exit-code-registry: wrote ${shPath}`, json, ); @@ -589,12 +601,12 @@ function checkOneArtifact(artifactLabel, artifactPath, content) { } /** - * --check verifies ALL FOUR committed artifacts against the same freshly + * --check verifies ALL FIVE committed artifacts against the same freshly * generated content and fails naming which one drifted (or is missing) if - * any does. Checked in a fixed order (primary, secondary, dts, sh) so a - * single-artifact failure is always reported deterministically. + * any does. Checked in a fixed order (primary, secondary, hooks, dts, sh) so + * a single-artifact failure is always reported deterministically. */ -function doCheck(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json) { +function doCheck(declarationPath, outPath, scriptsOutPath, hooksOutPath, dtsPath, shPath, json) { const result = buildRegistryContent(declarationPath); if (!result.ok) { emitFail(result, json); @@ -606,6 +618,7 @@ function doCheck(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json const artifacts = [ ['primary', outPath, result.content], ['secondary', scriptsOutPath, result.content], + ['hooks', hooksOutPath, result.content], ['dts', dtsPath, dtsContent], ['sh', shPath, shContent], ]; @@ -621,6 +634,7 @@ function doCheck(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json REASON.OK, `ok gen-exit-code-registry: ${outPath} matches ${declarationPath}\n` + `ok gen-exit-code-registry: ${scriptsOutPath} matches ${declarationPath}\n` + + `ok gen-exit-code-registry: ${hooksOutPath} matches ${declarationPath}\n` + `ok gen-exit-code-registry: ${dtsPath} matches ${declarationPath}\n` + `ok gen-exit-code-registry: ${shPath} matches ${declarationPath}`, json, @@ -629,13 +643,14 @@ function doCheck(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json } /** - * @returns {{mode:'write'|'check', declarationPath:?string, outPath:?string, scriptsOutPath:?string, dtsPath:?string, shPath:?string, json:boolean}} + * @returns {{mode:'write'|'check', declarationPath:?string, outPath:?string, scriptsOutPath:?string, hooksOutPath:?string, dtsPath:?string, shPath:?string, json:boolean}} */ function parseArgs(argv) { let mode = null; let declarationPath = null; let outPath = null; let scriptsOutPath = null; + let hooksOutPath = null; let dtsPath = null; let shPath = null; let json = false; @@ -667,6 +682,12 @@ function parseArgs(argv) { scriptsOutPath = value; } else if (arg.startsWith('--scripts-out=')) { scriptsOutPath = arg.slice('--scripts-out='.length); + } else if (arg === '--hooks-out') { + const value = argv[++i]; + if (value === undefined) throw new Error('--hooks-out requires a value'); + hooksOutPath = value; + } else if (arg.startsWith('--hooks-out=')) { + hooksOutPath = arg.slice('--hooks-out='.length); } else if (arg === '--dts-out') { const value = argv[++i]; if (value === undefined) throw new Error('--dts-out requires a value'); @@ -684,7 +705,7 @@ function parseArgs(argv) { } } - return { mode: mode || 'write', declarationPath, outPath, scriptsOutPath, dtsPath, shPath, json }; + return { mode: mode || 'write', declarationPath, outPath, scriptsOutPath, hooksOutPath, dtsPath, shPath, json }; } function main() { @@ -705,12 +726,13 @@ function main() { const declarationPath = args.declarationPath || DEFAULT_DECLARATION_PATH; const outPath = args.outPath || DEFAULT_OUTPUT_PATH; const scriptsOutPath = args.scriptsOutPath || DEFAULT_SCRIPTS_OUTPUT_PATH; + const hooksOutPath = args.hooksOutPath || DEFAULT_HOOKS_OUTPUT_PATH; const dtsPath = args.dtsPath || DEFAULT_DTS_OUTPUT_PATH; const shPath = args.shPath || DEFAULT_SH_OUTPUT_PATH; return args.mode === 'check' - ? doCheck(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, args.json) - : doWrite(declarationPath, outPath, scriptsOutPath, dtsPath, shPath, args.json); + ? doCheck(declarationPath, outPath, scriptsOutPath, hooksOutPath, dtsPath, shPath, args.json) + : doWrite(declarationPath, outPath, scriptsOutPath, hooksOutPath, dtsPath, shPath, args.json); } if (require.main === module) process.exitCode = main(); @@ -721,6 +743,7 @@ module.exports = { DEFAULT_DECLARATION_PATH, DEFAULT_OUTPUT_PATH, DEFAULT_SCRIPTS_OUTPUT_PATH, + DEFAULT_HOOKS_OUTPUT_PATH, DEFAULT_DTS_OUTPUT_PATH, DEFAULT_SH_OUTPUT_PATH, ENTRY_FIELD_TYPES, diff --git a/scripts/gen-hooks-cli-exit.cjs b/scripts/gen-hooks-cli-exit.cjs new file mode 100644 index 000000000..ada42c8f4 --- /dev/null +++ b/scripts/gen-hooks-cli-exit.cjs @@ -0,0 +1,207 @@ +#!/usr/bin/env node +/** + * gen-hooks-cli-exit.cjs — generates hooks/lib/cli-exit.js from a fresh + * compile of src/cli-exit.cts. + * + * ADR-3889 Phase 7 (#3911): shipped hooks must be able to terminate through + * `terminateNow` WITHOUT depending on any build artifact. hooks/ runs + * straight from a raw, unbuilt clone (a hook is required by name via + * `require('./lib/cli-exit.js')` relative to the hook's own __dirname), so + * the generated file is compiled to a THROWAWAY outDir rather than read from + * gsd-core/bin/lib/ — reading the tracked build output would let a stale + * build produce a false green. This mirrors scripts/gen-scripts-cli-exit.cjs + * exactly (same compile-to-temp strategy, same banner/check/write shape); + * kept as a SIBLING script rather than folded into that one because + * gen-scripts-cli-exit.cjs is hard-coded to a single output path/extension + * (`scripts/lib/cli-exit.cjs`) throughout its BANNER prose and REASON + * messaging — parameterizing it for a second, differently-extensioned + * target (`.js`, not `.cjs`, to match the hooks/lib/*.js convention) would + * tangle a script that is otherwise simple and single-purpose. + * + * Usage: + * node scripts/gen-hooks-cli-exit.cjs # same as --write + * node scripts/gen-hooks-cli-exit.cjs --write # write hooks/lib/cli-exit.js + * node scripts/gen-hooks-cli-exit.cjs --check # exit 1 if committed file is stale + */ + +'use strict'; + +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const REPO_ROOT = path.resolve(__dirname, '..'); +const OUTPUT_PATH = path.join(REPO_ROOT, 'hooks', 'lib', 'cli-exit.js'); +const COMPILE_TIMEOUT_MS = 60_000; + +/** Frozen reason codes so tests assert on structure, not prose. */ +const REASON = Object.freeze({ + OK: 'ok_generated_sync', + DRIFTED: 'fail_generated_drifted', + BUILD_FAILED: 'fail_build_failed', + MISSING_EMIT: 'fail_missing_emit', + USAGE: 'fail_usage', +}); + +const USAGE_MESSAGE = [ + 'Usage: node scripts/gen-hooks-cli-exit.cjs [--write|--check]', + ' (no flag) same as --write', + ' --write write hooks/lib/cli-exit.js', + ' --check exit 1 if the committed file is stale', +].join('\n'); + +const BANNER = [ + '// GENERATED FILE — DO NOT EDIT BY HAND.', + '// Source of truth: src/cli-exit.cts. Regenerate with:', + '// node scripts/gen-hooks-cli-exit.cjs --write', + '// Byte-compared by `npm run lint:generated-sync` (#3911, ADR-3889 Phase 7).', + '//', + '// Why this copy exists: hooks/ runs straight from a raw, unbuilt clone — a', + '// shipped hook must be able to `require(\'./lib/cli-exit.js\')` relative to', + '// its own __dirname and terminate through `terminateNow` without depending', + '// on any build artifact. gsd-core/bin/lib/cli-exit.cjs is gitignored tsc', + '// output and doubles as the build sentinel, so it cannot be required from', + '// here. `.js`, not `.cjs`, to match the hooks/lib/*.js convention. Hence one', + '// source, three emitted locations (gsd-core/bin/lib, scripts/lib, hooks/lib).', + '', + '', +].join('\n'); + +/** Compile the whole project to a throwaway outDir so the work tree is untouched. */ +function compileToTemp() { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-hooks-cli-exit-')); + try { + execFileSync( + process.execPath, + [ + path.join(REPO_ROOT, 'node_modules', 'typescript', 'bin', 'tsc'), + '-p', path.join(REPO_ROOT, 'tsconfig.build.json'), + '--outDir', tmp, + // A throwaway outDir must not reuse the in-tree incremental state, or + // tsc skips emit for files it believes are already current. + '--incremental', 'false', + '--tsBuildInfoFile', 'null', + ], + { cwd: REPO_ROOT, encoding: 'utf8', stdio: 'pipe', timeout: COMPILE_TIMEOUT_MS }, + ); + return { ok: true, dir: tmp }; + } catch (err) { + fs.rmSync(tmp, { recursive: true, force: true }); + const detail = [err.stdout, err.stderr].filter(Boolean).join('\n').trim(); + return { ok: false, detail }; + } +} + +/** + * Compile src/cli-exit.cts to a throwaway outDir and return the expected + * generated content (banner + compiled bytes, with the sibling registry + * require rewritten to the `.js` extension hooks/lib/ uses), or a failure + * descriptor. + * + * @returns {{ ok: true, content: string } | { ok: false, reason: string, detail?: string }} + */ +function buildExpectedContent() { + const build = compileToTemp(); + if (!build.ok) { + return { ok: false, reason: REASON.BUILD_FAILED, detail: build.detail }; + } + try { + const emitted = path.join(build.dir, 'cli-exit.cjs'); + if (!fs.existsSync(emitted)) { + return { ok: false, reason: REASON.MISSING_EMIT, detail: `no emit produced at ${emitted} from src/cli-exit.cts` }; + } + const compiled = fs.readFileSync(emitted, 'utf8'); + // hooks/lib/ ships `.js`, not `.cjs` (matching the hooks/lib/*.js + // convention — see the module header), so the sibling registry require + // tsc emits for src/cli-exit.cts's `require('./exit-code-registry.cjs')` + // must be rewritten to resolve the `.js` copy scripts/gen-exit-code- + // registry.cjs emits alongside this file, not the `.cjs` one. A literal, + // anchored replace (never a regex) so this can only ever touch the exact + // string tsc is known to emit for that one import. + const REGISTRY_REQUIRE_CJS = 'require("./exit-code-registry.cjs")'; + const REGISTRY_REQUIRE_JS = 'require("./exit-code-registry.js")'; + if (!compiled.includes(REGISTRY_REQUIRE_CJS)) { + return { + ok: false, + reason: REASON.MISSING_EMIT, + detail: `expected compiled output to contain ${REGISTRY_REQUIRE_CJS} (the sibling registry require) — tsc's emit shape may have changed`, + }; + } + const rewritten = compiled.split(REGISTRY_REQUIRE_CJS).join(REGISTRY_REQUIRE_JS); + return { ok: true, content: BANNER + rewritten }; + } finally { + fs.rmSync(build.dir, { recursive: true, force: true }); + } +} + +function doWrite() { + const result = buildExpectedContent(); + if (!result.ok) { + console.error(`FAIL gen-hooks-cli-exit: ${result.reason}`); + if (result.detail) console.error(result.detail); + return 1; + } + fs.mkdirSync(path.dirname(OUTPUT_PATH), { recursive: true }); + fs.writeFileSync(OUTPUT_PATH, result.content, 'utf8'); + console.log(`ok gen-hooks-cli-exit: wrote ${path.relative(REPO_ROOT, OUTPUT_PATH)}`); + return 0; +} + +function doCheck() { + const result = buildExpectedContent(); + if (!result.ok) { + console.error(`FAIL gen-hooks-cli-exit: ${result.reason}`); + if (result.detail) console.error(result.detail); + return 1; + } + + if (!fs.existsSync(OUTPUT_PATH)) { + console.error(`FAIL gen-hooks-cli-exit: ${REASON.MISSING_EMIT}`); + console.error(` ${path.relative(REPO_ROOT, OUTPUT_PATH)} does not exist. Run:`); + console.error(' node scripts/gen-hooks-cli-exit.cjs --write'); + return 1; + } + + const committed = fs.readFileSync(OUTPUT_PATH, 'utf8'); + if (committed !== result.content) { + console.error(`FAIL gen-hooks-cli-exit: ${REASON.DRIFTED}`); + console.error( + ` ${path.relative(REPO_ROOT, OUTPUT_PATH)} (${committed.length} bytes) != ` + + `compile of src/cli-exit.cts (${result.content.length} bytes)`, + ); + console.error(''); + console.error('Regenerate with:'); + console.error(' node scripts/gen-hooks-cli-exit.cjs --write'); + return 1; + } + + console.log(`ok gen-hooks-cli-exit: ${path.relative(REPO_ROOT, OUTPUT_PATH)} matches src/cli-exit.cts`); + return 0; +} + +function main() { + const flag = process.argv[2]; + const extra = process.argv[3]; + + if (flag !== undefined && flag !== '--write' && flag !== '--check') { + console.error(`FAIL gen-hooks-cli-exit: ${REASON.USAGE}`); + console.error(` unrecognized argument: ${flag}`); + console.error(USAGE_MESSAGE); + return 1; + } + + if (extra !== undefined) { + console.error(`FAIL gen-hooks-cli-exit: ${REASON.USAGE}`); + console.error(` unexpected extra argument: ${extra}`); + console.error(USAGE_MESSAGE); + return 1; + } + + if (flag === '--check') return doCheck(); + return doWrite(); +} + +if (require.main === module) process.exitCode = main(); + +module.exports = { REASON, buildExpectedContent, OUTPUT_PATH, BANNER }; diff --git a/scripts/lib/cli-exit.cjs b/scripts/lib/cli-exit.cjs index 23adae181..81db88847 100644 --- a/scripts/lib/cli-exit.cjs +++ b/scripts/lib/cli-exit.cjs @@ -300,9 +300,21 @@ function runMain(main) { * it. * * @param outcome - declared outcome name, projected via projectOutcome. - * @param payload - JSON-serializable value written to fd 1 (and, on a deny, - * fd 2 too — Kimi's native hook bus feeds stderr, not stdout, back to the - * model on exit 2, per hooks/gsd-write-guard.js's emitBlock). + * @param payload - JSON-serializable value written to fd 1 (and, on a deny + * for which no `stderrPayload` is given, fd 2 too — this is the + * backward-compatible default every existing caller relies on). + * @param stderrPayload - optional, deny-only. When omitted (the default), + * fd 2 gets the SAME serialized `payload` fd 1 got — unchanged behavior. + * When provided, fd 2 gets THIS instead: a string is written raw + * (verbatim, not JSON-stringified), anything else is JSON-stringified + * like `payload`. This exists because `hooks/gsd-write-guard.js`'s + * emitBlock does NOT write the same bytes to both streams today — it + * writes the full JSON `output` to stdout but only the plain-text + * `output.reason` STRING to stderr, because Kimi's native hook bus reads + * stderr verbatim back to the model on exit 2. Migrating that call site + * onto terminateNow requires a way to say "fd 2 gets this different, + * plain-text value" — `stderrPayload` is that seam. Ignored entirely for + * a non-deny outcome: stderr is a deny-only channel. * * PAYLOAD-SIZE CONSTRAINT FOR CALLERS (measured for #3906, relevant to P7/ * #3911 wiring 19 enforcement hooks onto this function): the write-until- @@ -325,7 +337,7 @@ function runMain(main) { * arrives whole" test, which hit exactly this constructing its own fixture * before being rewritten to build the payload inside the child instead. */ -function terminateNow(outcome, payload) { +function terminateNow(outcome, payload, stderrPayload) { // terminateNow is total by construction: its callers are enforcement hooks // (P7/#3911, 19 of them) whose OWN outer catch may fail open (some end in // `process.exit(0)`). If resolving the contract version, projecting the @@ -353,6 +365,17 @@ function terminateNow(outcome, payload) { // outcome the fail-closed branches this function serves exist to prevent. // The decision to terminate with `projected` stands regardless of whether // the payload could be delivered. + // + // The two streams are emitted in TWO SEPARATE try/catch blocks, not one + // shared block (the pre-#3911 defect): fd 1 and fd 2 (deny-only) are + // independent channels with independent failure modes, and a shared try + // meant a serialization failure on fd 1 (e.g. `payload` throwing on + // JSON.stringify) aborted the block before fd 2 ever ran — silently + // dropping a deny's reason. `deny(undefined, 'some reason')` used to exit + // 2 with EMPTY stderr because of exactly this. Each block independently + // treats an `undefined` value for ITS OWN stream as "nothing to write" + // and skips the write cleanly, rather than serializing `undefined` (which + // is not valid JSON text) and throwing into the catch. try { // fs.writeSync, never process.stdout.write: pipe writes via // process.stdout/stderr are async on Windows, and process.exit() below @@ -361,20 +384,36 @@ function terminateNow(outcome, payload) { // payload larger than the destination pipe's buffer — where a single // write() syscall can legitimately return fewer bytes written than // requested — still arrives whole rather than truncated. - const buf = Buffer.from(JSON.stringify(payload), 'utf8'); - let offset = 0; - while (offset < buf.length) { - offset += node_fs_1.default.writeSync(1, buf, offset, buf.length - offset); - } - if (projected === HOOK_DENY_CODE) { - let stderrOffset = 0; - while (stderrOffset < buf.length) { - stderrOffset += node_fs_1.default.writeSync(2, buf, stderrOffset, buf.length - stderrOffset); + if (payload !== undefined) { + const buf = Buffer.from(JSON.stringify(payload), 'utf8'); + let offset = 0; + while (offset < buf.length) { + offset += node_fs_1.default.writeSync(1, buf, offset, buf.length - offset); } } } catch { - // Emission failed; the exit code decision still stands (see above). + // fd 1 emission failed; the exit code decision still stands (see + // above), and fd 2 below is unaffected — it has its own try/catch. + } + if (projected === HOOK_DENY_CODE) { + try { + // Backward-compatible default: when no `stderrPayload` is given, fd 2 + // gets the SAME value fd 1 got (still subject to fd 2's own + // undefined-skips-the-write and string-vs-JSON rules below). + const resolvedStderr = stderrPayload === undefined ? payload : stderrPayload; + if (resolvedStderr !== undefined) { + const stderrBuf = Buffer.from(typeof resolvedStderr === 'string' ? resolvedStderr : JSON.stringify(resolvedStderr), 'utf8'); + let stderrOffset = 0; + while (stderrOffset < stderrBuf.length) { + stderrOffset += node_fs_1.default.writeSync(2, stderrBuf, stderrOffset, stderrBuf.length - stderrOffset); + } + } + } + catch { + // fd 2 emission failed; independent of fd 1 above, and the exit code + // decision still stands regardless. + } } // n/no-process-exit is not registered for src/**/*.cts (see the ADR-3889 // reference note in the module header) and both compiled .cjs copies of diff --git a/scripts/lib/exit-code-registry.cjs b/scripts/lib/exit-code-registry.cjs index 51af7339a..c6ea8dc24 100644 --- a/scripts/lib/exit-code-registry.cjs +++ b/scripts/lib/exit-code-registry.cjs @@ -3,10 +3,11 @@ // GENERATED FILE — DO NOT EDIT BY HAND. // Source of truth: gsd-core/bin/shared/exit-codes.json. Regenerate with: // node scripts/gen-exit-code-registry.cjs --write -// This exact content is emitted to TWO locations — gsd-core/bin/lib/exit-code-registry.cjs -// and scripts/lib/exit-code-registry.cjs (the latter committed so scripts/ -// consumers work on an unbuilt clone) — both byte-compared by -// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy). +// This exact content is emitted to THREE locations — gsd-core/bin/lib/exit-code-registry.cjs, +// scripts/lib/exit-code-registry.cjs, and hooks/lib/exit-code-registry.js (the latter two +// committed so scripts/ and hooks/ consumers work on an unbuilt clone) — all byte-compared by +// `npm run lint:generated-sync` (#3905 ADR-3889 Phase 1; #3906 Phase 2 added the second copy; +// #3911 ADR-3889 Phase 7 added the hooks/lib/ copy). // // exitCodeFor(name) / nameForExitCode(code) are pure and total over this // closed table — each throws for anything not registered here. diff --git a/src/cli-exit.cts b/src/cli-exit.cts index b2b2919d4..4d925ec55 100644 --- a/src/cli-exit.cts +++ b/src/cli-exit.cts @@ -299,9 +299,21 @@ function runMain(main: () => number | string | void | Promise number | string | void | Promise(); - for (const script of installedScripts) { - const staged = fs.readFileSync(path.join(hooksDir, script), 'utf8'); - // Tolerant of interior whitespace and either quote style: a hook author - // writing `require( "./lib/x.js" )` must still get its helper staged, since - // a miss here surfaces as MODULE_NOT_FOUND at hook load, not at install. - const re = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; + const scannedLibFiles = new Set(); + const libRequireRe = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; + + function scanForLibRequires(source: string): void { + libRequireRe.lastIndex = 0; let m: RegExpExecArray | null; - while ((m = re.exec(staged)) !== null) requiredLibFiles.add(m[1]); + while ((m = libRequireRe.exec(source)) !== null) requiredLibFiles.add(m[1]); } + + for (const script of installedScripts) { + scanForLibRequires(fs.readFileSync(path.join(hooksDir, script), 'utf8')); + } + if (requiredLibFiles.size > 0) { const srcLibDir = path.join(srcHooksDir, 'lib'); const destLibDir = path.join(hooksDir, 'lib'); fs.mkdirSync(destLibDir, { recursive: true }); - for (const libFile of requiredLibFiles) { + // Iterate to a fixed point: staging a lib file can add MORE required lib + // files (its own requires), which must themselves be staged and scanned. + let libFile: string | undefined = [...requiredLibFiles].find((f) => !scannedLibFiles.has(f)); + while (libFile !== undefined) { + scannedLibFiles.add(libFile); const libSrc = path.join(srcLibDir, libFile); if (!fs.existsSync(libSrc)) { // FAIL LOUD. Skipping here would ship hook scripts whose top-level @@ -1542,6 +1556,8 @@ function writeCursorHooksJson(targetDir: string, src: string, opts?: WriteCursor let libContent = fs.readFileSync(libSrc, 'utf8'); libContent = libContent.replace(/gsd:/gi, 'gsd-'); fs.writeFileSync(path.join(destLibDir, libFile), libContent); + scanForLibRequires(libContent); + libFile = [...requiredLibFiles].find((f) => !scannedLibFiles.has(f)); } } diff --git a/tests/cli-exit.test.cjs b/tests/cli-exit.test.cjs index 034d70af2..8c7c8463f 100644 --- a/tests/cli-exit.test.cjs +++ b/tests/cli-exit.test.cjs @@ -21,6 +21,13 @@ const IO_PATH = path.resolve(__dirname, '../gsd-core/bin/lib/io.cjs'); const SCRIPTS_CLI_EXIT_PATH = path.resolve(__dirname, '../scripts/lib/cli-exit.cjs'); const EXIT_CODE_REGISTRY_PATH = path.resolve(__dirname, '../gsd-core/bin/lib/exit-code-registry.cjs'); +// #3911 (ADR-3889 Phase 7): the THIRD emitted copy, for hooks/ consumers that +// must terminate through terminateNow without depending on any build +// artifact (gsd-core/bin/lib is gitignored tsc output, absent on a raw +// plugin-marketplace or git-clone install). +const HOOKS_CLI_EXIT_PATH = path.resolve(__dirname, '../hooks/lib/cli-exit.js'); +const HOOKS_EXIT_CODE_REGISTRY_PATH = path.resolve(__dirname, '../hooks/lib/exit-code-registry.js'); + const { EXIT_CODES } = require(EXIT_CODE_REGISTRY_PATH); const REGISTERED_NAMES = EXIT_CODES.map((e) => e.name); const VERSIONS = ['v1', 'v2']; @@ -1147,6 +1154,126 @@ describe('#3906: terminateNow', () => { assert.equal(r.stderr, ''); }); + test('#3911: HOOK_DENY with NO stderrPayload arg — backward-compatible default: fd1 and fd2 both get the serialized payload', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', { reason: 'blocked-default' });`, + ]); + assert.equal(r.status, 2); + assert.deepEqual(JSON.parse(r.stdout), { reason: 'blocked-default' }); + assert.deepEqual(JSON.parse(r.stderr), { reason: 'blocked-default' }); + }); + + test('#3911: HOOK_DENY with a STRING stderrPayload — fd1 gets JSON, fd2 gets the raw string verbatim', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', { decision: 'block', reason: 'shrink too large' }, 'shrink too large');`, + ]); + assert.equal(r.status, 2); + assert.deepEqual(JSON.parse(r.stdout), { decision: 'block', reason: 'shrink too large' }); + assert.equal(r.stderr, 'shrink too large', `expected raw string on stderr, got: ${r.stderr}`); + assert.throws(() => JSON.parse(r.stderr), SyntaxError, + 'a raw reason string must NOT itself be JSON-parseable as an object'); + }); + + test('#3911: HOOK_DENY with an OBJECT stderrPayload — fd2 gets that object serialized, not the fd1 payload', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', { full: 'stdout-payload' }, { distinct: 'stderr-payload' });`, + ]); + assert.equal(r.status, 2); + assert.deepEqual(JSON.parse(r.stdout), { full: 'stdout-payload' }); + assert.deepEqual(JSON.parse(r.stderr), { distinct: 'stderr-payload' }); + }); + + test('#3911: a PASS outcome with a stderrPayload writes NOTHING to stderr — stderr is a deny-only channel', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('PASS', { ok: true }, 'should never reach stderr');`, + ]); + assert.equal(r.status, 0); + assert.deepEqual(JSON.parse(r.stdout), { ok: true }); + assert.equal(r.stderr, '', `expected empty stderr for a non-deny outcome; got: ${r.stderr}`); + }); + + test('#3911: terminateNow(PASS, undefined) exits 0 with empty stdout', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('PASS', undefined);`, + ]); + assert.equal(r.status, 0, `stderr: ${r.stderr}`); + assert.equal(r.stdout, '', `expected empty stdout, got: ${r.stdout}`); + assert.equal(r.stderr, '', `expected empty stderr, got: ${r.stderr}`); + }); + + test('#3911: terminateNow(HOOK_DENY, undefined) exits 2 with EMPTY stdout and EMPTY stderr', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', undefined);`, + ]); + assert.equal(r.status, 2, `stderr: ${r.stderr}`); + assert.equal(r.stdout, '', `expected empty stdout, got: ${r.stdout}`); + assert.equal(r.stderr, '', `expected empty stderr, got: ${r.stderr}`); + }); + + // The defect this whole file is regressing (#3911): `deny(undefined, + // 'some reason')` used to exit 2 with EMPTY stderr because the fd1 write of + // `undefined` threw (JSON.stringify(undefined) -> undefined, + // Buffer.from(undefined,'utf8') throws) and the SHARED try/catch aborted + // before the fd2 write of `stderrPayload` ever ran. + test('#3911: terminateNow(HOOK_DENY, undefined, "reason text") exits 2 with EMPTY stdout and stderr EXACTLY "reason text"', () => { + const r = spawnTerminateNow([ + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', undefined, 'reason text');`, + ]); + assert.equal(r.status, 2, `stderr: ${r.stderr}`); + assert.equal(r.stdout, '', `expected empty stdout, got: ${r.stdout}`); + assert.equal(r.stderr, 'reason text', `expected exactly "reason text" on stderr, got: ${r.stderr}`); + }); + + // ── #3911 regression: the two stream emissions are INDEPENDENT ─────────── + // These two tests are the direct regression coverage for the defect: a + // shared try/catch around both writes meant a failure serializing/writing + // fd 1 aborted before fd 2 (or vice versa) ever ran. Both must FAIL against + // the pre-fix single-try-block implementation. + test('#3911 regression: fd 1 write throws but fd 2 STILL receives its payload, exit code unchanged', () => { + const r = spawnTerminateNow([ + `const fs = require('node:fs');`, + `const origWriteSync = fs.writeSync;`, + `fs.writeSync = (fd, ...rest) => {`, + ` if (fd === 1) throw new Error('injected fd1 failure');`, + ` return origWriteSync(fd, ...rest);`, + `};`, + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', { reason: 'fd1-throws' });`, + ]); + assert.equal(r.status, 2, `exit code must be unchanged by the fd1 failure; stderr: ${r.stderr}`); + assert.equal(r.stdout, '', 'fd1 write failed, so stdout must be empty (not a partial payload)'); + assert.deepEqual( + JSON.parse(r.stderr), { reason: 'fd1-throws' }, + `fd2 must still receive its payload despite the fd1 failure; got: ${r.stderr}`, + ); + }); + + test('#3911 regression: fd 2 write throws but fd 1 STILL receives its payload, exit code unchanged', () => { + const r = spawnTerminateNow([ + `const fs = require('node:fs');`, + `const origWriteSync = fs.writeSync;`, + `fs.writeSync = (fd, ...rest) => {`, + ` if (fd === 2) throw new Error('injected fd2 failure');`, + ` return origWriteSync(fd, ...rest);`, + `};`, + `const c = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `c.terminateNow('HOOK_DENY', { reason: 'fd2-throws' });`, + ]); + assert.equal(r.status, 2, `exit code must be unchanged by the fd2 failure; stderr: ${r.stderr}`); + assert.deepEqual( + JSON.parse(r.stdout), { reason: 'fd2-throws' }, + `fd1 must still receive its payload despite the fd2 failure; got: ${r.stdout}`, + ); + assert.equal(r.stderr, '', 'fd2 write failed, so stderr must be empty (not a partial payload)'); + }); + // Cross-platform IO-failure injection: a monkeypatched THROWING fs.writeSync, // restored implicitly by process exit — never chmod (CONTRIBUTING.md / // CLAUDE.md: mode-bit tricks are bypassed by root/CI and leak resources). @@ -1366,6 +1493,61 @@ describe('#3906: cross-copy — the built copy and the scripts copy agree', () = } }); + // #3911 (ADR-3889 Phase 7) A6: EXTENDS the two-copy parity above to the + // THIRD emitted copy (hooks/lib/cli-exit.js + hooks/lib/exit-code-registry.js) + // rather than duplicating it. Outcome names are enumerated from the hooks + // registry itself — never hardcoded — so a future registry addition is + // covered automatically and a hooks-registry omission fails loudly here + // instead of silently under-testing the hooks copy. + test('the hooks copy agrees with both existing copies for every registered outcome/version', () => { + const built = require(BUILT_CLI_EXIT_PATH); + const scripts = require(SCRIPTS_CLI_EXIT_PATH); + const hooks = require(HOOKS_CLI_EXIT_PATH); + const { EXIT_CODES: hooksExitCodes } = require(HOOKS_EXIT_CODE_REGISTRY_PATH); + const hooksNames = hooksExitCodes.map((e) => e.name); + + assert.deepStrictEqual( + [...hooksNames].sort(), [...REGISTERED_NAMES].sort(), + 'the hooks registry must declare the exact same outcome set as the primary registry', + ); + + for (const version of VERSIONS) { + for (const outcome of ['PASS', 'FAIL', ...REGISTERED_NAMES]) { + const fromBuilt = built.projectOutcome(outcome, version); + const fromScripts = scripts.projectOutcome(outcome, version); + const fromHooks = hooks.projectOutcome(outcome, version); + assert.equal(fromScripts, fromBuilt, `scripts vs built disagree for ${outcome}/${version}`); + assert.equal(fromHooks, fromBuilt, `hooks vs built disagree for ${outcome}/${version}`); + } + } + }); + + // #3911 A6: the json-error-mode cell is a `globalThis`-backed shared cell + // (see the #3906 "ONE json-error-mode cell" tests above for the built/scripts + // pair) — prove the hooks copy reads and writes the SAME cell, not a third, + // independent module-level flag that would silently diverge. + test('the json-error-mode cell is genuinely shared across all three copies (built, scripts, hooks)', () => { + const built = require(BUILT_CLI_EXIT_PATH); + const scripts = require(SCRIPTS_CLI_EXIT_PATH); + const hooks = require(HOOKS_CLI_EXIT_PATH); + const saved = built.getJsonErrorMode(); + try { + built.setJsonErrorMode(true); + assert.equal(scripts.getJsonErrorMode(), true, 'scripts must observe the mode set through built'); + assert.equal(hooks.getJsonErrorMode(), true, 'hooks must observe the mode set through built'); + + hooks.setJsonErrorMode(false); + assert.equal(built.getJsonErrorMode(), false, 'built must observe the mode set through hooks'); + assert.equal(scripts.getJsonErrorMode(), false, 'scripts must observe the mode set through hooks'); + + scripts.setJsonErrorMode(true); + assert.equal(built.getJsonErrorMode(), true, 'built must observe the mode set through scripts'); + assert.equal(hooks.getJsonErrorMode(), true, 'hooks must observe the mode set through scripts'); + } finally { + built.setJsonErrorMode(saved); + } + }); + test('scripts/lib/cli-exit.cjs + its sibling exit-code-registry.cjs load standalone, no gsd-core sibling', (t) => { const dir = createTempDir('gsd-3906-standalone-'); t.after(() => cleanup(dir)); @@ -1414,3 +1596,168 @@ describe('#3906: cross-copy — the built copy and the scripts copy agree', () = ); }); }); + +// ─── #3911 (ADR-3889 Phase 7, issue #3911): the hooks copy is load-bearing ── +// +// The WHOLE POINT of hooks/lib/cli-exit.js is that a shipped enforcement hook +// can terminate through terminateNow WITHOUT depending on any build +// artifact. gsd-core/bin/lib is gitignored tsc output and is ABSENT on a raw +// plugin-marketplace or git-clone install. These tests prove that property +// hermetically: by copying ONLY hooks/lib/cli-exit.js and its sibling +// hooks/lib/exit-code-registry.js into a fresh, otherwise-empty tmpdir (no +// gsd-core sibling, no node_modules) and requiring the copy from a CHILD +// process rooted there. +describe('#3911: hooks/lib/cli-exit.js loads and terminates with no build present (A4, load-bearing)', () => { + /** Copy both hooks/lib artifacts into a fresh, otherwise-empty tmpdir. */ + function makeStandaloneHooksCopy(t) { + const dir = createTempDir('gsd-3911-hooks-standalone-'); + t.after(() => cleanup(dir)); + const copiedExit = path.join(dir, 'cli-exit.js'); + const copiedRegistry = path.join(dir, 'exit-code-registry.js'); + fs.copyFileSync(HOOKS_CLI_EXIT_PATH, copiedExit); + fs.copyFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH, copiedRegistry); + return { dir, copiedExit, copiedRegistry }; + } + + // This test must FAIL if hooks/lib/cli-exit.js ever gains a require + // reaching outside hooks/lib/ (e.g. back into gsd-core/bin/lib, or a + // node_modules package): the tmpdir contains NOTHING else, so any such + // require resolves to MODULE_NOT_FOUND and the child crashes before + // terminateNow ever runs, failing every assertion below. + test('terminateNow(PASS) exits 0 with the payload on stdout, from a build-free tmpdir', (t) => { + const { dir, copiedExit } = makeStandaloneHooksCopy(t); + const r = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(copiedExit)});`, + `c.terminateNow('PASS', { ok: true, from: 'hooks-standalone' });`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + + assert.ok( + !r.stderr.includes('MODULE_NOT_FOUND'), + `the hooks copy must not require anything outside hooks/lib/; got: ${r.stderr.slice(0, 400)}`, + ); + assert.equal(r.status, 0, `stderr: ${r.stderr}`); + assert.deepEqual(JSON.parse(r.stdout), { ok: true, from: 'hooks-standalone' }); + }); + + test('terminateNow(HOOK_DENY) exits 2 with the payload on BOTH stdout and stderr, from a build-free tmpdir', (t) => { + const { dir, copiedExit } = makeStandaloneHooksCopy(t); + const r = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(copiedExit)});`, + `c.terminateNow('HOOK_DENY', { reason: 'blocked-by-guard', from: 'hooks-standalone' });`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + + assert.ok( + !r.stderr.includes('MODULE_NOT_FOUND'), + `the hooks copy must not require anything outside hooks/lib/; got: ${r.stderr.slice(0, 400)}`, + ); + assert.equal(r.status, 2, `stderr: ${r.stderr}`); + const expected = { reason: 'blocked-by-guard', from: 'hooks-standalone' }; + assert.deepEqual(JSON.parse(r.stdout), expected); + assert.deepEqual(JSON.parse(r.stderr), expected); + }); + + // A5: sibling resolution. hooks/lib/cli-exit.js requires its OWN sibling + // (./exit-code-registry.js), not some other copy sitting elsewhere on the + // machine — proven by DELETING the sibling from the tmpdir and observing + // the require actually fail, then restoring it and observing success again. + test('resolves its OWN sibling exit-code-registry.js, not some other copy (A5)', (t) => { + const { dir, copiedExit, copiedRegistry } = makeStandaloneHooksCopy(t); + const registryBytes = fs.readFileSync(copiedRegistry); + + // Sanity: with the sibling present, the copy loads clean. + const before = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(copiedExit)});`, + `process.stdout.write(JSON.stringify({ hasTerminateNow: typeof c.terminateNow }));`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + assert.equal(before.status, 0, `stderr: ${before.stderr}`); + assert.deepEqual(JSON.parse(before.stdout), { hasTerminateNow: 'function' }); + + // Prove the failure: delete the sibling, require must fail. Single-file + // removal of a fixture we immediately restore below (not directory + // teardown; cleanup(dir) still runs via t.after() for the whole tmpdir). + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- see comment above + fs.rmSync(copiedRegistry); + try { + const missing = toLegacyResult(runNode(['-e', [ + `require(${JSON.stringify(copiedExit)});`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + assert.notEqual(missing.status, 0, 'require must fail once the sibling registry is removed'); + assert.ok( + missing.stderr.includes('MODULE_NOT_FOUND') || missing.stderr.includes('Cannot find module'), + `expected a module-resolution failure naming the missing sibling; got: ${missing.stderr.slice(0, 400)}`, + ); + } finally { + // Restore in a finally so a failing assertion above cannot leave the + // tmpdir fixture (owned by this test, not a committed artifact) broken + // for any later step in this same test. + fs.writeFileSync(copiedRegistry, registryBytes); + } + + // Confirm restoration actually fixes it — the negative-space check is + // meaningless without this positive control. + const after = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(copiedExit)});`, + `process.stdout.write(JSON.stringify({ hasTerminateNow: typeof c.terminateNow }));`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + assert.equal(after.status, 0, `stderr: ${after.stderr}`); + assert.deepEqual(JSON.parse(after.stdout), { hasTerminateNow: 'function' }); + }); +}); + +// ─── #3911 A3: the --check generator guards can actually fail ────────────── +// +// CONTEXT.md's prove-it-can-fail rule: a guard that has never been observed +// to fail is not a guard. For BOTH new committed artifacts, corrupt the +// committed file, run the generator's --check, assert it fails and names the +// file, then restore in a `finally` (so a failing assertion here can never +// leave a committed artifact corrupted) and re-run --check to confirm the +// restore actually cleared the guard. +describe('#3911: the --check guards for the new hooks/lib artifacts can actually fail (A3)', () => { + const REPO_ROOT = path.resolve(__dirname, '..'); + const GEN_HOOKS_CLI_EXIT = path.join(REPO_ROOT, 'scripts', 'gen-hooks-cli-exit.cjs'); + const GEN_EXIT_CODE_REGISTRY = path.join(REPO_ROOT, 'scripts', 'gen-exit-code-registry.cjs'); + // gen-hooks-cli-exit.cjs --check runs a real tsc compile of the whole + // project to a throwaway outDir (see its own COMPILE_TIMEOUT_MS=60000) — + // this needs a longer bound than a plain probe. + const CHECK_TIMEOUT_MS = 90000; + + test('gen-hooks-cli-exit.cjs --check fails on a corrupted hooks/lib/cli-exit.js, names the file, and clears on restore', () => { + const original = fs.readFileSync(HOOKS_CLI_EXIT_PATH); + let corrupted = false; + try { + fs.appendFileSync(HOOKS_CLI_EXIT_PATH, '\n// corrupted-by-A3-test\n'); + corrupted = true; + const r = toLegacyResult(runNode([GEN_HOOKS_CLI_EXIT, '--check'], { timeoutMs: CHECK_TIMEOUT_MS })); + assert.notEqual(r.status, 0, `--check must fail on a corrupted committed artifact; stderr: ${r.stderr}`); + assert.ok( + r.stderr.includes('cli-exit.js'), + `expected the failure to name the drifted file; got: ${r.stderr.slice(0, 400)}`, + ); + } finally { + if (corrupted) fs.writeFileSync(HOOKS_CLI_EXIT_PATH, original); + } + + const restored = toLegacyResult(runNode([GEN_HOOKS_CLI_EXIT, '--check'], { timeoutMs: CHECK_TIMEOUT_MS })); + assert.equal(restored.status, 0, `--check must pass again once the artifact is restored; stderr: ${restored.stderr}`); + }); + + test('gen-exit-code-registry.cjs --check fails on a corrupted hooks/lib/exit-code-registry.js, names the file, and clears on restore', () => { + const original = fs.readFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH); + let corrupted = false; + try { + fs.appendFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH, '\n// corrupted-by-A3-test\n'); + corrupted = true; + const r = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check'], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.notEqual(r.status, 0, `--check must fail on a corrupted committed artifact; stderr: ${r.stderr}`); + assert.ok( + r.stderr.includes('exit-code-registry.js'), + `expected the failure to name the drifted file; got: ${r.stderr.slice(0, 400)}`, + ); + } finally { + if (corrupted) fs.writeFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH, original); + } + + const restored = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check'], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.equal(restored.status, 0, `--check must pass again once the artifact is restored; stderr: ${restored.stderr}`); + }); +}); diff --git a/tests/cursor-hook-workspace-roots.test.cjs b/tests/cursor-hook-workspace-roots.test.cjs index 6d45e5eb3..9c0a61149 100644 --- a/tests/cursor-hook-workspace-roots.test.cjs +++ b/tests/cursor-hook-workspace-roots.test.cjs @@ -301,9 +301,17 @@ describe('#2587: cursor hooks resolve the workspace from workspace_roots, not cw for (const hook of RESOLVING_HOOKS) { fs.copyFileSync(hook, path.join(srcHooks, path.basename(hook))); } + // Any one of the RESOLVING_HOOKS' required lib/ helpers is a valid trip + // wire here — this fixture supplies NONE of them, so whichever helper the + // scan discovers first is reported missing. Coupling this assertion to one + // specific filename (formerly 'cursor-workspace.js') breaks every time the + // discovery order shifts, e.g. #3911 adding an earlier './lib/hook-exit.js' + // require to these same hook scripts. The invariant under test is "some + // required lib helper missing from hooks/lib -> the install aborts", not + // "this exact helper is named first". assert.throws( () => hooksSurface.writeCursorHooksJson(target, fakeSrc, {}), - /cursor-workspace\.js.*missing|missing.*cursor-workspace\.js/s, + /hooks\/lib\/[A-Za-z0-9._-]+\.js is required by a staged Cursor hook but is missing/, 'a missing lib source must abort the install, not ship a broken hook', ); } finally { diff --git a/tests/exit-code-registry.test.cjs b/tests/exit-code-registry.test.cjs index cea243690..d27dc63ab 100644 --- a/tests/exit-code-registry.test.cjs +++ b/tests/exit-code-registry.test.cjs @@ -34,6 +34,10 @@ const REPO_ROOT = path.resolve(__dirname, '..'); const GEN_SCRIPT = path.join(REPO_ROOT, 'scripts', 'gen-exit-code-registry.cjs'); const REAL_DECLARATION_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'shared', 'exit-codes.json'); const REAL_ARTIFACT_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'lib', 'exit-code-registry.cjs'); +const REAL_SCRIPTS_ARTIFACT_PATH = path.join(REPO_ROOT, 'scripts', 'lib', 'exit-code-registry.cjs'); +const REAL_HOOKS_ARTIFACT_PATH = path.join(REPO_ROOT, 'hooks', 'lib', 'exit-code-registry.js'); +const REAL_DTS_ARTIFACT_PATH = path.join(REPO_ROOT, 'src', 'exit-code-registry.d.cts'); +const REAL_SH_ARTIFACT_PATH = path.join(REPO_ROOT, 'gsd-core', 'bin', 'shared', 'exit-codes.sh'); const generator = require(GEN_SCRIPT); const registry = require(REAL_ARTIFACT_PATH); @@ -53,21 +57,23 @@ function makeEntry(overrides) { } /** - * #3906 (ADR-3889 Phase 2): the generator now emits THREE artifacts — a + * #3906 (ADR-3889 Phase 2): the generator now emits FIVE artifacts — a * primary (gsd-core/bin/lib), a secondary (scripts/lib), and the ambient * `.d.cts` type declaration (src/exit-code-registry.d.cts). #3908 (Phase 4) * added a FOURTH: the shell-sourceable fragment (gsd-core/bin/shared/ - * exit-codes.sh). Every existing call site below only overrides the PRIMARY - * path via `--out`; without matching `--scripts-out`/`--dts-out`/`--sh-out` - * overrides, a `--write` here would clobber the real committed - * `scripts/lib/exit-code-registry.cjs`, `src/exit-code-registry.d.cts`, and - * `gsd-core/bin/shared/exit-codes.sh` — dangerous since test files in this - * repo run in parallel. Rather than touch every call site, this single seam - * derives co-located, per-call-unique secondary/dts/sh paths from whatever - * `--out` value the test already supplies, whenever the caller has not - * already supplied its own `--scripts-out`/`--dts-out`/`--sh-out`. Calls - * with no explicit `--out` (the "real committed set" checks) are left - * untouched. + * exit-codes.sh). #3911 (Phase 7) added a FIFTH: the hooks/lib/ copy + * (hooks/lib/exit-code-registry.js). Every existing call site below only + * overrides the PRIMARY path via `--out`; without matching + * `--scripts-out`/`--hooks-out`/`--dts-out`/`--sh-out` overrides, a + * `--write` here would clobber the real committed + * `scripts/lib/exit-code-registry.cjs`, `hooks/lib/exit-code-registry.js`, + * `src/exit-code-registry.d.cts`, and `gsd-core/bin/shared/exit-codes.sh` — + * dangerous since test files in this repo run in parallel. Rather than + * touch every call site, this single seam derives co-located, + * per-call-unique secondary/hooks/dts/sh paths from whatever `--out` value + * the test already supplies, whenever the caller has not already supplied + * its own `--scripts-out`/`--hooks-out`/`--dts-out`/`--sh-out`. Calls with + * no explicit `--out` (the "real committed set" checks) are left untouched. */ function ensureScriptsOut(args) { const outIdx = args.indexOf('--out'); @@ -75,6 +81,7 @@ function ensureScriptsOut(args) { const outValue = args[outIdx + 1]; const extra = []; if (!args.includes('--scripts-out')) extra.push('--scripts-out', `${outValue}.secondary.cjs`); + if (!args.includes('--hooks-out')) extra.push('--hooks-out', `${outValue}.hooks.js`); if (!args.includes('--dts-out')) extra.push('--dts-out', `${outValue}.d.cts`); if (!args.includes('--sh-out')) extra.push('--sh-out', `${outValue}.sh`); return extra.length === 0 ? args : [...args, ...extra]; @@ -562,6 +569,37 @@ describe('gen-exit-code-registry: CLI', () => { assert.equal(report.ok, false); assert.equal(report.reason, generator.REASON.EMPTY_DECLARATION); }); + + // Regression (#3911 follow-up): ensureScriptsOut derived --scripts-out/ + // --dts-out/--sh-out from --out but did not derive --hooks-out, so any + // --write test here silently clobbered the real committed + // hooks/lib/exit-code-registry.js. Assert over ALL FIVE committed + // artifacts so the next added target is covered by construction. + test('a --write run redirected to a tmpdir leaves every committed artifact untouched', () => { + const before = { + out: fs.readFileSync(REAL_ARTIFACT_PATH, 'utf8'), + scripts: fs.readFileSync(REAL_SCRIPTS_ARTIFACT_PATH, 'utf8'), + hooks: fs.readFileSync(REAL_HOOKS_ARTIFACT_PATH, 'utf8'), + dts: fs.readFileSync(REAL_DTS_ARTIFACT_PATH, 'utf8'), + sh: fs.readFileSync(REAL_SH_ARTIFACT_PATH, 'utf8'), + }; + + const decl = validDeclarationPath(tmpDir, 'l-decl.json'); + const out = path.join(tmpDir, 'l-out.cjs'); + const write = runGen(['--write', '--declaration', decl, '--out', out]); + assert.equal(write.exitCode, 0, write.stderr); + + assert.equal(fs.readFileSync(REAL_ARTIFACT_PATH, 'utf8'), before.out, 'primary artifact must be untouched'); + assert.equal(fs.readFileSync(REAL_SCRIPTS_ARTIFACT_PATH, 'utf8'), before.scripts, 'scripts artifact must be untouched'); + assert.equal(fs.readFileSync(REAL_HOOKS_ARTIFACT_PATH, 'utf8'), before.hooks, 'hooks artifact must be untouched'); + assert.equal(fs.readFileSync(REAL_DTS_ARTIFACT_PATH, 'utf8'), before.dts, '.d.cts artifact must be untouched'); + assert.equal(fs.readFileSync(REAL_SH_ARTIFACT_PATH, 'utf8'), before.sh, '.sh artifact must be untouched'); + + assert.ok(fs.existsSync(`${out}.hooks.js`), 'expected the redirected hooks copy to land in the tmpdir'); + assert.ok(fs.existsSync(`${out}.secondary.cjs`), 'expected the redirected scripts copy to land in the tmpdir'); + assert.ok(fs.existsSync(`${out}.d.cts`), 'expected the redirected .d.cts copy to land in the tmpdir'); + assert.ok(fs.existsSync(`${out}.sh`), 'expected the redirected .sh copy to land in the tmpdir'); + }); }); // ── Generator CLI: positive controls (each guard actually FAILS the build) ──── diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index 35333db5f..ff646f413 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -404,9 +404,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index d8ed36e16..92e060da9 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -475,9 +475,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index b76c6db05..23e4ffa1b 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -475,9 +475,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index a84400ccd..72ac7de4d 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -404,9 +404,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 81e6a65a0..451de0a20 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -475,9 +475,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index b8108dbef..04021fb09 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -383,7 +383,10 @@ "hooks/gsd-cursor-stop.js", "hooks/gsd-cursor-subagent-start.js", "hooks/gsd-cursor-subagent-stop.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", + "hooks/lib/hook-exit.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", "hooks/package.json", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index baa18f238..9dbaaf556 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -404,9 +404,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index c9d26aa1e..5dba68c68 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -475,9 +475,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index d75207adf..9cc5918ec 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -405,9 +405,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 604acf31a..4a06d99f2 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -475,9 +475,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index 4a0d0f226..b8334ecc4 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -371,9 +371,13 @@ "gsd-hooks/gsd-workflow-guard.js", "gsd-hooks/gsd-worktree-path-guard.js", "gsd-hooks/gsd-write-guard.js", + "gsd-hooks/lib/cli-exit.js", "gsd-hooks/lib/cursor-workspace.js", + "gsd-hooks/lib/exit-code-registry.js", "gsd-hooks/lib/git-cmd.js", + "gsd-hooks/lib/git-probe.js", "gsd-hooks/lib/gsd-graphify-rebuild.sh", + "gsd-hooks/lib/hook-exit.js", "gsd-hooks/lib/injection-patterns.js", "gsd-hooks/lib/isolation-deny-reason.js", "gsd-hooks/lib/isolation-sentinel.js", diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index d38087302..10c88e76d 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -404,9 +404,13 @@ "hooks/gsd-workflow-guard.js", "hooks/gsd-worktree-path-guard.js", "hooks/gsd-write-guard.js", + "hooks/lib/cli-exit.js", "hooks/lib/cursor-workspace.js", + "hooks/lib/exit-code-registry.js", "hooks/lib/git-cmd.js", + "hooks/lib/git-probe.js", "hooks/lib/gsd-graphify-rebuild.sh", + "hooks/lib/hook-exit.js", "hooks/lib/injection-patterns.js", "hooks/lib/isolation-deny-reason.js", "hooks/lib/isolation-sentinel.js", diff --git a/tests/gsd-validate-commit-crash-policy.test.cjs b/tests/gsd-validate-commit-crash-policy.test.cjs new file mode 100644 index 000000000..210981e95 --- /dev/null +++ b/tests/gsd-validate-commit-crash-policy.test.cjs @@ -0,0 +1,171 @@ +'use strict'; + +/** + * gsd-validate-commit-crash-policy.test.cjs — regression coverage for #3838 + * (subsumed-but-still-present under #3911): hooks/gsd-validate-commit.sh has + * three "swallow-and-pass" sites — the opt-in config read, the JSON command + * extraction, and the isGitSubcommand classifier — each of which used a + * failing subprocess call directly as an `if`/`$(...)` condition. `set -e` + * never fires on a command used as an `if` condition, so a failure there was + * indistinguishable from "genuinely not applicable" and silently disabled + * the whole validator: a non-conforming commit exited 0 with no output, + * identical to "your commit is fine". + * + * This is a sibling file to tests/hooks-crash-policy.test.cjs rather than an + * addition to its table: that file's TABLE and drift guard are scoped + * exclusively to hooks/*.js (the hook-exit.js allow/deny/crash migration, + * #3911 phase 7); gsd-validate-commit.sh is a bash script with its own + * ad hoc exit-code contract that predates and is orthogonal to that + * migration, so it does not belong in that table or its drift guard. + * + * Every case here spawns the real hook via bash (tests/helpers/process-seam.cjs + * `runHook` with `interpreter: 'bash'`) against a real fixture project — + * no source-file grep, no mocked node internals. + */ + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const { createTempDir, cleanup, TEST_ENV_BASE } = require('./helpers.cjs'); +const { runHook, OUTCOME } = require('./helpers/process-seam.cjs'); + +const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-validate-commit.sh'); + +const CONFORMING_COMMIT_PAYLOAD = JSON.stringify({ + tool_input: { command: 'git commit -m "feat: add thing"' }, +}); +const NONCONFORMING_COMMIT_PAYLOAD = JSON.stringify({ + tool_input: { command: 'git commit -m "wibble wobble"' }, +}); + +const cleanupPaths = []; +function tempDir(prefix) { + const dir = createTempDir(prefix); + cleanupPaths.push(dir); + return dir; +} + +let enabledProject; +// A PATH directory whose `node` shim fails ONLY the classifier's `node -e` +// invocation (detected by the presence of `isGitSubcommand` in argv, which +// only that one of the hook's three node calls ever passes), and otherwise +// execs the real node binary the test runner itself is running under. This +// reproduces the exact defect-triggering shape from #3838's repro ("node +// cannot run [for this call]") without touching the other two call sites. +let classifierBrokenPathDir; + +before(() => { + enabledProject = tempDir('gsd-validate-commit-ok-'); + fs.mkdirSync(path.join(enabledProject, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(enabledProject, '.planning', 'config.json'), + JSON.stringify({ hooks: { community: true } }), + ); + + classifierBrokenPathDir = tempDir('gsd-validate-commit-node-shim-'); + const shimPath = path.join(classifierBrokenPathDir, 'node'); + fs.writeFileSync( + shimPath, + [ + '#!/usr/bin/env bash', + `REAL_NODE=${JSON.stringify(process.execPath)}`, + 'for a in "$@"; do', + ' if [[ "$a" == *isGitSubcommand* ]]; then', + ' echo "test-shim: classifier node call intentionally broken (simulated node crash)" >&2', + ' exit 127', + ' fi', + 'done', + 'exec "$REAL_NODE" "$@"', + '', + ].join('\n'), + ); + fs.chmodSync(shimPath, 0o755); +}); + +after(() => { + for (const p of cleanupPaths) cleanup(p); +}); + +function runValidateCommit({ payload, cwd, env } = {}) { + return runHook(HOOK_PATH, [], { + interpreter: 'bash', + cwd, + env: { ...process.env, ...TEST_ENV_BASE, ...env }, + input: payload, + timeoutMs: 15000, + }); +} + +// --------------------------------------------------------------------------- +// Controls — pin that the fix does not weaken or break the working paths. +// --------------------------------------------------------------------------- + +describe('gsd-validate-commit.sh: controls (unchanged behavior)', () => { + test('CONTROL A: node available, conforming commit -> exit 0, no block payload', () => { + const r = runValidateCommit({ payload: CONFORMING_COMMIT_PAYLOAD, cwd: enabledProject }); + assert.equal(r.outcome, OUTCOME.EXITED, `stderr=${r.stderr}`); + assert.equal(r.exitCode, 0, `stdout=${r.stdout} stderr=${r.stderr}`); + assert.equal(r.stdout.trim(), ''); + }); + + test('CONTROL B: node available, non-conforming commit -> exit 2 with block payload', () => { + const r = runValidateCommit({ payload: NONCONFORMING_COMMIT_PAYLOAD, cwd: enabledProject }); + assert.equal(r.outcome, OUTCOME.EXITED, `stderr=${r.stderr}`); + assert.equal(r.exitCode, 2, `stdout=${r.stdout} stderr=${r.stderr}`); + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + assert.equal(out.code, 'CONVENTIONAL_COMMITS_VIOLATION'); + }); +}); + +// --------------------------------------------------------------------------- +// Defect arms — each of the three swallow-and-pass sites, exercised with a +// non-conforming commit so a silent pass is unambiguous: the validator MUST +// NOT report success by omission when it could not actually run. +// --------------------------------------------------------------------------- + +describe('gsd-validate-commit.sh: #3838 could-not-run sites are surfaced, not swallowed', () => { + test('DEFECT (classifier): node cannot run isGitSubcommand -> not a silent pass', () => { + const r = runValidateCommit({ + payload: NONCONFORMING_COMMIT_PAYLOAD, + cwd: enabledProject, + env: { PATH: `${classifierBrokenPathDir}:${process.env.PATH}` }, + }); + assert.equal(r.outcome, OUTCOME.EXITED, `stderr=${r.stderr}`); + // The hook must still exit 0 (PreToolUse "fail open"), but it must not + // be the pre-#3838-fix silent exit 0 with empty stdout AND empty stderr + // — that shape is indistinguishable from "your commit is fine". + assert.equal(r.exitCode, 0, `stdout=${r.stdout} stderr=${r.stderr}`); + assert.notEqual(r.stderr.trim(), '', 'expected a non-empty stderr diagnostic; got silence (the #3838 defect shape)'); + assert.match(r.stderr, /classif/i, 'diagnostic should name the classifier check'); + assert.equal(r.stdout.trim(), '', 'no block payload is expected when validation could not run'); + }); + + test('DEFECT (config read): malformed .planning/config.json -> not a silent pass', () => { + const brokenConfigProject = tempDir('gsd-validate-commit-badconfig-'); + fs.mkdirSync(path.join(brokenConfigProject, '.planning'), { recursive: true }); + // Syntactically invalid JSON -> require() throws a SyntaxError distinct + // from "file legitimately says community:false/absent". + fs.writeFileSync(path.join(brokenConfigProject, '.planning', 'config.json'), '{ this is not json'); + + const r = runValidateCommit({ payload: NONCONFORMING_COMMIT_PAYLOAD, cwd: brokenConfigProject }); + assert.equal(r.outcome, OUTCOME.EXITED, `stderr=${r.stderr}`); + assert.equal(r.exitCode, 0, `stdout=${r.stdout} stderr=${r.stderr}`); + assert.notEqual(r.stderr.trim(), '', 'expected a non-empty stderr diagnostic; got silence'); + assert.match(r.stderr, /config/i, 'diagnostic should name the config-read check'); + }); + + test('DEFECT (command extraction): malformed JSON on stdin -> not a silent pass', () => { + // Malformed top-level JSON on stdin makes JSON.parse(d) throw inside the + // command-extraction node call — distinct from "tool_input.command is + // genuinely absent from a well-formed payload" (which legitimately + // yields CMD='' and is not a git commit). + const r = runValidateCommit({ payload: '{not valid json at all', cwd: enabledProject }); + assert.equal(r.outcome, OUTCOME.EXITED, `stderr=${r.stderr}`); + assert.equal(r.exitCode, 0, `stdout=${r.stdout} stderr=${r.stderr}`); + assert.notEqual(r.stderr.trim(), '', 'expected a non-empty stderr diagnostic; got silence'); + assert.match(r.stderr, /command/i, 'diagnostic should name the command-extraction check'); + }); +}); diff --git a/tests/hooks-crash-policy.test.cjs b/tests/hooks-crash-policy.test.cjs new file mode 100644 index 000000000..6d1fd8fab --- /dev/null +++ b/tests/hooks-crash-policy.test.cjs @@ -0,0 +1,697 @@ +'use strict'; + +/** + * hooks-crash-policy.test.cjs — table-driven coverage of ADR-3889 Phase 7 + * (#3911): every enforcement hook under hooks/*.js now terminates through + * hooks/lib/hook-exit.js (allow/deny/crash) instead of a raw process.exit(). + * + * Rather than ~76 hand-written tests (19 hooks x 4 cases), this file drives + * ONE table — one row per hook, each row derived by reading that hook's own + * source (never guessed) — through four generic cases: + * + * C1 allow — normal input -> exit 0. + * C2 deny — normal input that trips the hook's block path + * (only the 6 hooks that HAVE one) -> exit 2, + * asserting the actual stream(s) that hook uses. + * C3 crash honors policy — an input that makes the hook's own outer catch + * fire, asserting the exit code matches its + * DECLARED HOOK_ON_CRASH policy. Hooks that never + * call crash(ON_CRASH, ...) at all (no declared + * policy) are t.skip()'d with an explicit reason + * — never silently passed via a bare return. + * C4 stdin never closes — spawn with no stdin input and never end it; + * assert the process still terminates (via its + * own bounded stdin-timeout -> allow()) instead + * of hanging on the parent's outer spawn timeout. + * + * The table is the single source of truth: a guard test at the bottom + * enumerates hooks/*.js and fails if a new terminating hook is added without + * a row here. + * + * Crash trigger: malformed JSON on stdin ('{not json'). Every one of the 9 + * hooks that declares an ON_CRASH policy parses its stdin payload as the + * FIRST statement inside its outer try — `JSON.parse(input)` (or the Kimi- + * normalized `normalizeKimiPayload(JSON.parse(input))`) — so a syntax error + * there throws before any applicability logic runs and is a real, hook- + * authored crash, not a synthetic fault injected by this suite. + */ + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +const { createTempDir, cleanup, TEST_ENV_BASE } = require('./helpers.cjs'); +const { runHook: runHookSeam, runNode, OUTCOME } = require('./helpers/process-seam.cjs'); +const { gitOrThrow, GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/git-fixture.cjs'); +const { ensureBuiltHooks } = require('../scripts/run-tests.cjs'); + +const HOOKS_DIR = path.join(__dirname, '..', 'hooks'); + +// Hook scripts that ship under hooks/ but never terminate through +// hooks/lib/hook-exit.js at all — they are long-running/update-check helpers, +// not PreToolUse/PostToolUse/SessionStart enforcement hooks, so #3911's +// migration (and this table) does not apply to them. +const NON_TERMINATING_HOOKS = new Set([ + 'gsd-check-update.js', + 'gsd-check-update-worker.js', + 'gsd-update-banner.js', +]); + +function hookPath(name) { + return path.join(HOOKS_DIR, name); +} + +function baseEnv(extra = {}) { + return { ...TEST_ENV_BASE, ...extra }; +} + +/** + * Run a hook with a payload on stdin (or, for C4, no `input` key at all — + * see below). Thin wrapper over the process-seam so every case in this file + * shares one spawn path and one required timeout. + */ +function runHook(name, { payload, cwd, env, timeoutMs = 15000 } = {}) { + const opts = { env: baseEnv(env), timeoutMs }; + if (cwd !== undefined) opts.cwd = cwd; + if (payload !== undefined) { + opts.input = typeof payload === 'string' ? payload : JSON.stringify(payload); + } + return runHookSeam(hookPath(name), [], opts); +} + +const MALFORMED_JSON = '{not json'; + +// --------------------------------------------------------------------------- +// Shared fixtures +// --------------------------------------------------------------------------- + +let fixtures = {}; +const cleanupPaths = []; + +function tempDir(prefix) { + const dir = createTempDir(prefix); + cleanupPaths.push(dir); + return dir; +} + +before(() => { + // --- worktree fixture: a linked git worktree on an agent-* branch, used by + // gsd-worktree-path-guard.js's deny case (absolute path escaping the active + // worktree's git root) and gsd-windsurf-pre-write.js's deny case (same + // escape shape, different protocol). --------------------------------------- + const mainRepo = tempDir('hooks-crash-main-'); + gitOrThrow(['init', '-q', mainRepo], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', mainRepo, 'config', 'user.email', 'hooks-crash@test.local'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', mainRepo, 'config', 'user.name', 'hooks-crash'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + fs.writeFileSync(path.join(mainRepo, 'README.md'), 'hello\n'); + gitOrThrow(['-C', mainRepo, 'add', '-A'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', mainRepo, 'commit', '-m', 'seed'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + const worktreePath = path.join(os.tmpdir(), `hooks-crash-wt-${process.pid}-${Date.now()}`); + gitOrThrow(['-C', mainRepo, 'worktree', 'add', '-b', 'agent-test', worktreePath], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + cleanupPaths.push(worktreePath); + + // --- gsd-write-guard.js: a curated ROADMAP.md big enough to trip the 40% + // shrink-ratio guard (well above the FLOOR_LINES=40 exemption). ------------- + const wgProject = tempDir('hooks-crash-wg-'); + fs.mkdirSync(path.join(wgProject, '.planning'), { recursive: true }); + const roadmapPath = path.join(wgProject, '.planning', 'ROADMAP.md'); + fs.writeFileSync(roadmapPath, Array.from({ length: 292 }, (_, i) => `line ${i + 1}`).join('\n') + '\n'); + + // --- gsd-workflow-guard.js: a repo on an agent-* branch with + // hooks.workflow_guard enabled, so `git add -f` trips the ONE hard-block. -- + const wfRepo = tempDir('hooks-crash-wf-'); + gitOrThrow(['init', '-q', '-b', 'agent-test', wfRepo], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', wfRepo, 'config', 'user.email', 'hooks-crash@test.local'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', wfRepo, 'config', 'user.name', 'hooks-crash'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + fs.mkdirSync(path.join(wfRepo, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(wfRepo, '.planning', 'config.json'), JSON.stringify({ hooks: { workflow_guard: true } })); + fs.writeFileSync(path.join(wfRepo, 'seed.txt'), 'seed\n'); + gitOrThrow(['-C', wfRepo, 'add', '-A'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['-C', wfRepo, 'commit', '-m', 'seed'], { timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + + // --- gsd-agent-isolation-guard.js: `.planning/config.json` as a DIRECTORY + // (EISDIR) — the guard's documented "cannot verify -> DENY" fail-closed + // path (mirrors tests/gsd-agent-isolation-guard.test.cjs's own fixture). --- + const aigProject = tempDir('hooks-crash-aig-'); + fs.mkdirSync(path.join(aigProject, '.planning', 'config.json'), { recursive: true }); + + fixtures = { mainRepo, worktreePath, wgProject, roadmapPath, wfRepo, aigProject }; +}); + +after(() => { + for (const p of cleanupPaths) cleanup(p); +}); + +// --------------------------------------------------------------------------- +// The table — one row per enforcement hook, derived from reading its source. +// --------------------------------------------------------------------------- + +const TABLE = [ + { + file: 'gsd-worktree-path-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + // #3911: this hook's deny path depends on several bounded (2000ms) + // spawnSync(git, ...) probes. Under load, a probe can time out before it + // answers — the hook still allows (exit 0, unchanged), but now with a + // stderr diagnostic instead of the pre-#3911 silent allow. See the C2 + // loop below and the dedicated stub-git regression suite. + gitProbeMayRace: true, + allow: () => ({ payload: { tool_name: 'Read' } }), + deny: () => ({ + payload: { tool_name: 'Write', tool_input: { file_path: path.join(fixtures.mainRepo, 'README.md') } }, + cwd: fixtures.worktreePath, + }), + assertDeny: (r) => { + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + assert.match(r.stderr, /differs from the active worktree root/); + assert.equal(r.stderr, out.reason, 'stderr must carry the plain reason string (deny stderrPayload)'); + }, + }, + { + file: 'gsd-write-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { tool_name: 'Read' } }), + deny: () => ({ + payload: { tool_name: 'Write', tool_input: { file_path: fixtures.roadmapPath, content: 'short\n' } }, + env: { GSD_ALLOW_PLANNING_SHRINK: undefined }, + }), + assertDeny: (r) => { + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + assert.equal(out.oldLines, 292); + assert.match(r.stderr, /shrink/); + assert.equal(r.stderr, out.reason); + }, + }, + { + file: 'gsd-workflow-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + // #3911: the force-add block depends on a bounded (2000ms) spawnSync(git + // branch --show-current) probe — see gitProbeMayRace note on the + // gsd-worktree-path-guard.js row above. + gitProbeMayRace: true, + allow: () => ({ payload: { tool_name: 'Read' } }), + deny: () => ({ + payload: { tool_name: 'Bash', cwd: fixtures.wfRepo, tool_input: { command: 'git add -f secret.txt' } }, + }), + assertDeny: (r) => { + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + assert.equal(out.code, 'WORKTREE_AGENT_FORCE_ADD_FORBIDDEN'); + assert.match(r.stderr, /force-add|force/i); + assert.equal(r.stderr, out.reason); + }, + }, + { + file: 'gsd-context-monitor.js', + stdinTimeoutMs: 10000, + declaredOnCrash: 'allow', + allow: () => ({ payload: {} }), // no session_id -> allow(undefined) + }, + { + file: 'gsd-config-reload.js', + stdinTimeoutMs: 8000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { file_path: '/tmp/not-a-gsd-config.json' } }), // basename mismatch -> allow + }, + { + file: 'gsd-read-injection-scanner.js', + stdinTimeoutMs: 5000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { tool_name: 'Bash' } }), // not in SCANNED_TOOLS -> allow + }, + { + file: 'gsd-prompt-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { tool_name: 'Read' } }), + }, + { + file: 'gsd-read-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { tool_name: 'Read' } }), + }, + { + file: 'gsd-agent-isolation-guard.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + allow: () => ({ payload: { tool_name: 'Read' } }), + deny: () => ({ + payload: { tool_name: 'Task', tool_input: { subagent_type: 'gsd-executor' } }, + cwd: fixtures.aigProject, + env: { GSD_RUNTIME: undefined }, + }), + assertDeny: (r) => { + const out = JSON.parse(r.stdout); + assert.equal(out.decision, 'block'); + // REASON_CODE.CONFIG_UNREADABLE (hooks/lib/isolation-deny-reason.js) has + // always been the lowercase string 'config_unreadable' — untouched by + // the #3911 crash-policy migration. The uppercase literal here was + // simply wrong. + assert.equal(out.reason_code, 'config_unreadable'); + assert.match(r.stderr, /could not read or resolve/); + assert.equal(r.stderr, out.reason); + }, + }, + { + file: 'gsd-windsurf-pre-command.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, // catch calls allow(undefined) directly — no HOOK_ON_CRASH declared + allow: () => ({ payload: { tool_info: { command_line: 'ls -la' } } }), + deny: () => ({ payload: { tool_info: { command_line: 'rm -rf /' } } }), + assertDeny: (r) => { + assert.equal(r.stdout, '', 'deny(undefined, reason) must skip the fd 1 write entirely'); + assert.match(r.stderr, /rm -rf targeting the filesystem root/); + }, + }, + { + file: 'gsd-windsurf-pre-write.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, // catch calls allow(undefined) directly — no HOOK_ON_CRASH declared + // #3911: this hook's deny path depends on bounded (2000ms) spawnSync(git, + // ...) probes, same shape as gsd-worktree-path-guard.js above. + gitProbeMayRace: true, + allow: () => ({ payload: { tool_info: { file_path: 'nonexistent.txt' } }, cwd: os.tmpdir() }), + deny: () => ({ + payload: { tool_info: { file_path: path.join(fixtures.mainRepo, 'README.md') } }, + cwd: fixtures.worktreePath, + }), + assertDeny: (r) => { + assert.equal(r.stdout, '', 'deny(undefined, reason) must skip the fd 1 write entirely'); + assert.match(r.stderr, /resolves to git root/); + }, + }, + { + file: 'gsd-statusline.js', + stdinTimeoutMs: 3000, + declaredOnCrash: 'allow', + crashSkipReason: + 'the crash() call this hook declares ON_CRASH for guards only the require.main ' + + "self-heal block (ensureRuntimeBuild() failing on an unbuilt gsd-core/bin/lib tree) — " + + "the per-request stdin handler's own catch is a silent fail with no crash() call at all. " + + 'Forcing the self-heal path to fail would require corrupting the built lib tree or actually ' + + 'invoking a build, both out of scope for a behavioral spawn test.', + allow: () => ({ payload: {} }), + }, + { + file: 'gsd-ensure-canonical-path.js', + stdinTimeoutMs: null, // never reads stdin at all — see the C4 note below + declaredOnCrash: null, // no HOOK_ON_CRASH import/usage; allow(undefined) is unconditional + allow: () => ({ payload: undefined }), + }, + { + file: 'gsd-cursor-post-tool.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: { tool_name: 'Read' } }), + }, + { + file: 'gsd-cursor-pre-tool.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: { tool_name: 'Read' } }), + }, + { + file: 'gsd-cursor-session-start.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: {} }), + }, + { + file: 'gsd-cursor-stop.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: {} }), + }, + { + file: 'gsd-cursor-subagent-start.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: {} }), + }, + { + file: 'gsd-cursor-subagent-stop.js', + stdinTimeoutMs: 10000, + declaredOnCrash: null, + allow: () => ({ payload: {} }), + }, +]; + +// --------------------------------------------------------------------------- +// C1 — allow +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: C1 normal allow -> exit 0', () => { + for (const row of TABLE) { + test(`${row.file}: allow input -> exit 0`, () => { + const { payload, cwd, env } = row.allow(); + const r = runHook(row.file, { payload, cwd, env }); + assert.equal(r.outcome, OUTCOME.EXITED, `expected a clean exit; got ${r.outcome} stderr=${r.stderr}`); + assert.equal(r.exitCode, 0, `stdout=${r.stdout} stderr=${r.stderr}`); + }); + } +}); + +// --------------------------------------------------------------------------- +// C2 — deny (only the 6 hooks with a real block path) +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: C2 normal deny -> exit 2, correct stream(s)', () => { + const denyRows = TABLE.filter((row) => typeof row.deny === 'function'); + + test('exactly 6 hooks in this table declare a deny case', () => { + assert.equal(denyRows.length, 6, denyRows.map((r) => r.file).join(', ')); + }); + + for (const row of denyRows) { + test(`${row.file}: deny input -> exit 2` + (row.gitProbeMayRace ? ' (or an undetermined-probe allow with a diagnostic, #3911)' : ''), () => { + const { payload, cwd, env } = row.deny(); + const r = runHook(row.file, { payload, cwd, env }); + assert.equal(r.outcome, OUTCOME.EXITED, `expected a clean exit; got ${r.outcome} stderr=${r.stderr}`); + + if (!row.gitProbeMayRace) { + assert.equal(r.exitCode, 2, `stdout=${r.stdout} stderr=${r.stderr}`); + row.assertDeny(r); + return; + } + + // #3911: this row's deny path depends on a bounded spawnSync(git, ...) + // probe that can, under load, time out before it answers — the ORIGINAL + // real-race defect this test used to have (asserting exit 2 unconditionally + // even though a slow git legitimately yields exit 0). A clean deny is + // still the expected common case and is asserted identically to every + // other row. The ONLY other acceptable outcome is an allow that carries a + // non-empty stderr diagnostic naming the probe that could not run — a + // SILENT allow (exit 0 with EMPTY stdout AND EMPTY stderr) is the actual + // #3911 defect and MUST still fail this test. + if (r.exitCode === 2) { + row.assertDeny(r); + return; + } + assert.equal( + r.exitCode, 0, + `expected either a clean deny (exit 2) or an undetermined-probe allow (exit 0); ` + + `got exitCode=${r.exitCode}. stdout=${r.stdout} stderr=${r.stderr}` + ); + assert.notEqual( + r.stderr, '', + `a git-probe timeout must emit a stderr diagnostic naming the probe (#3911) — got a ` + + `SILENT allow (empty stdout AND empty stderr), which is the exact defect this test exists ` + + `to catch. stdout=${r.stdout}` + ); + assert.match( + r.stderr, /git probe/, + `stderr diagnostic must name the git probe that could not run; got: ${r.stderr}` + ); + }); + } +}); + +// --------------------------------------------------------------------------- +// C3 — crash honors the DECLARED policy (malformed JSON forces the outer +// catch). Hooks with no declared policy are t.skip()'d, never bare-returned. +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: C3 crash -> declared ON_CRASH policy', () => { + const EXPECTED_EXIT = { allow: 0, deny: 2 }; + + for (const row of TABLE) { + test(`${row.file}: malformed-JSON crash honors declared policy`, (t) => { + if (!row.declaredOnCrash) { + t.skip( + `${row.file} never calls crash(ON_CRASH, ...) — its outer catch calls allow()/nothing ` + + 'directly, so it declares no HOOK_ON_CRASH policy for this case to verify.' + ); + return; + } + if (row.crashSkipReason) { + t.skip(row.crashSkipReason); + return; + } + const r = runHook(row.file, { payload: MALFORMED_JSON }); + assert.equal(r.outcome, OUTCOME.EXITED, `expected a clean exit; got ${r.outcome} stderr=${r.stderr}`); + assert.equal( + r.exitCode, + EXPECTED_EXIT[row.declaredOnCrash], + `declared ON_CRASH=${row.declaredOnCrash} -> expected exit ${EXPECTED_EXIT[row.declaredOnCrash]}; ` + + `stdout=${r.stdout} stderr=${r.stderr}` + ); + }); + } +}); + +// --------------------------------------------------------------------------- +// C4 — stdin never closes: no `input` is supplied to the seam at all, so the +// child's stdin pipe is left open. Only the hook's OWN bounded stdin-timeout +// (-> allow() -> terminateNow -> a real process.exit) can end it; a bare +// `process.exitCode = N` inside that timer would never actually terminate a +// process still blocked reading stdin. Never assert on elapsed time — only +// that it terminated, and with what code. +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: C4 stdin never closes -> bounded termination, not a hang', () => { + for (const row of TABLE) { + test(`${row.file}: unclosed stdin still terminates`, () => { + const outerTimeoutMs = (row.stdinTimeoutMs ?? 3000) + 10000; + // No `payload` key at all -> the seam omits `options.input` -> the + // child's stdin is never written to or closed by the parent. + const r = runHook(row.file, { timeoutMs: outerTimeoutMs }); + assert.equal( + r.outcome, OUTCOME.EXITED, + `expected the hook's own stdin-timeout to terminate it before the outer ` + + `${outerTimeoutMs}ms spawn bound; got ${r.outcome} (a TIMED_OUT/KILLED outcome here means ` + + `the hook hung on stdin instead of self-terminating). stderr=${r.stderr}` + ); + assert.equal(r.exitCode, 0, `expected the stdin-timeout's allow() fallback (exit 0); stdout=${r.stdout} stderr=${r.stderr}`); + }); + } +}); + +// --------------------------------------------------------------------------- +// #3911 regression — deterministic git-probe timeout (no load required). +// +// The C2 loop above tolerates a raced timeout but cannot FORCE one: on a +// quiet machine the git probes in gsd-worktree-path-guard.js, +// gsd-workflow-guard.js, and gsd-windsurf-pre-write.js always answer well +// inside their 2000ms budget, so C2 alone would never actually exercise the +// undetermined-probe branch. This suite forces the timeout deterministically +// by putting a stub `git` on PATH that sleeps past every affected hook's own +// spawnSync timeout (2000ms) before exiting — the hook's own bounded budget, +// not real system load, is what triggers ETIMEDOUT, so this is reproducible +// on any machine. Never asserts on elapsed time — only on exit code and the +// stderr diagnostic's presence/content. +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: #3911 git-probe timeout forced via a stub git -> allow WITH a diagnostic', () => { + // Exceeds every affected hook's own spawnSync(git, ...) timeout (2000ms) — + // the hook's timeout fires and kills the stub first, so this value only + // needs to outlast 2000ms; it is never itself asserted on. + const GIT_STUB_SLEEP_MS = 3000; + + let stubDir; + + before(() => { + stubDir = tempDir('hooks-crash-git-stub-'); + // Not built on win32: the affected hooks spawn `git` via spawnSync with + // no `shell: true` (see hooks/gsd-worktree-path-guard.js's SPAWNOPT-based + // `spawnSync('git', args, { ...SPAWNOPT, cwd })` call), so Windows' + // CreateProcess resolves `git.exe` only and never a PATH `.cmd`/`.bat` + // shim — a stub written here could never be exercised. See the + // win32-only t.skip() on each case below. + if (process.platform !== 'win32') { + const shPath = path.join(stubDir, 'git'); + fs.writeFileSync(shPath, `#!/bin/sh\nsleep ${(GIT_STUB_SLEEP_MS / 1000).toFixed(3)}\nexit 0\n`); + fs.chmodSync(shPath, 0o755); + } + }); + + // Every affected hook resolves 'git' with NO explicit `env` override on its + // own spawnSync call (see hooks/*.js's `SPAWNOPT`/`currentBranch`), so it + // inherits the HOOK PROCESS's own `process.env.PATH` — which is exactly the + // `env` this test hands the hook via runHook()/baseEnv(). Setting PATH to + // ONLY the stub dir guarantees the hook's internal git spawn resolves to + // the stub, not a real git binary that might legitimately be fast. + const CASES = [ + { + file: 'gsd-worktree-path-guard.js', + build: () => ({ + payload: { tool_name: 'Write', tool_input: { file_path: path.join(os.tmpdir(), 'gsd-3911-stub-target.txt') } }, + }), + }, + { + file: 'gsd-workflow-guard.js', + build: () => { + const wfProject = tempDir('hooks-crash-wf-stub-'); + fs.mkdirSync(path.join(wfProject, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(wfProject, '.planning', 'config.json'), + JSON.stringify({ hooks: { workflow_guard: true } }) + ); + return { + payload: { tool_name: 'Bash', cwd: wfProject, tool_input: { command: 'git add -f secret.txt' } }, + }; + }, + }, + { + file: 'gsd-windsurf-pre-write.js', + build: () => ({ + payload: { tool_info: { file_path: 'gsd-3911-stub-target.txt' } }, + cwd: os.tmpdir(), + }), + }, + ]; + + for (const c of CASES) { + test(`${c.file}: git timing out still allows, WITH a stderr diagnostic naming the probe (not silent)`, (t) => { + if (process.platform === 'win32') { + // See hooks/gsd-worktree-path-guard.js's spawnSync('git', args, { ...SPAWNOPT, cwd }) + // call (no `shell: true`): CreateProcess resolves git.exe only. + t.skip( + 'win32: the hooks spawn git via spawnSync without shell:true, so CreateProcess ' + + 'resolves git.exe only and never a PATH .cmd shim — the probe cannot be intercepted ' + + 'here. Covered on linux and darwin.' + ); + return; + } + const { payload, cwd } = c.build(); + const r = runHook(c.file, { + payload, + cwd, + // The stub dir must resolve FIRST (git() calls carry no explicit `env` + // override, so they inherit this exact PATH) — but the stub script + // itself still needs a working `sh`/`sleep`, so the real PATH is + // appended after it, never before (a real `git` earlier in PATH would + // defeat the stub entirely). + env: { PATH: [stubDir, process.env.PATH].filter(Boolean).join(path.delimiter) }, + timeoutMs: GIT_STUB_SLEEP_MS + 15000, + }); + assert.equal(r.outcome, OUTCOME.EXITED, `expected a clean exit; got ${r.outcome} stderr=${r.stderr}`); + assert.equal( + r.exitCode, 0, + `a git-probe timeout must still ALLOW (exit code unchanged — #3911 requires no hook's ` + + `effective default changes); stdout=${r.stdout} stderr=${r.stderr}` + ); + assert.notEqual( + r.stderr, '', + `a git-probe timeout must emit a stderr diagnostic instead of the pre-#3911 SILENT allow ` + + `(empty stdout AND empty stderr). stdout=${r.stdout}` + ); + assert.match( + r.stderr, /git probe/, + `stderr diagnostic must name the git probe that could not run; got: ${r.stderr}` + ); + }); + } +}); + +// --------------------------------------------------------------------------- +// hooks/dist parity (#3911 review finding) — lint:hooks-runtime-build-seam +// only checks that a hook requiring a compiled gsd-core/bin/lib/*.cjs module +// also calls ensureRuntimeBuild(); it never compares hooks/dist/** against +// hooks/**. Nothing else in the suite proves the three files P7 adds under +// hooks/lib/ (cli-exit.js, exit-code-registry.js, hook-exit.js) actually +// reach hooks/dist/lib/ — the exact shape of #770 (a new hook file silently +// missing from a copy list). This is behavioral, not a source-grep: it +// builds the real hooks/dist via the same ensureBuiltHooks() chokepoint +// scripts/run-tests.cjs uses, byte-compares the shipped copies, and spawns a +// child that actually requires and calls the SHIPPED copy from its dist +// location (proving it can resolve its sibling registry there, not just +// that the bytes exist). +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: hooks/dist/lib parity (#3911 review finding)', () => { + const HOOKS_LIB_DIR = path.join(HOOKS_DIR, 'lib'); + const DIST_LIB_DIR = path.join(HOOKS_DIR, 'dist', 'lib'); + const SEAM_FILES = ['cli-exit.js', 'exit-code-registry.js', 'hook-exit.js']; + + let buildFailure = null; + + before(() => { + try { + // No overrides: this deliberately builds/verifies the REAL hooks/dist, + // the same gitignored-but-real artifact the installer ships from — a + // temp destination would not prove anything about what users get. + ensureBuiltHooks(); + } catch (e) { + buildFailure = e; + } + }); + + for (const name of SEAM_FILES) { + test(`hooks/dist/lib/${name} exists and is byte-identical to hooks/lib/${name}`, (t) => { + if (buildFailure) { + t.skip(`ensureBuiltHooks() failed to populate hooks/dist: ${buildFailure.message}`); + return; + } + const srcPath = path.join(HOOKS_LIB_DIR, name); + const distPath = path.join(DIST_LIB_DIR, name); + if (!fs.existsSync(distPath)) { + t.skip(`${distPath} does not exist after ensureBuiltHooks() — build seam did not ship it`); + return; + } + const srcBytes = fs.readFileSync(srcPath); + const distBytes = fs.readFileSync(distPath); + assert.ok( + srcBytes.equals(distBytes), + `hooks/dist/lib/${name} is not byte-identical to hooks/lib/${name} — the build seam shipped a stale or divergent copy` + ); + }); + } + + test('the shipped hooks/dist/lib/cli-exit.js is functional from its dist location (resolves its sibling registry)', (t) => { + if (buildFailure) { + t.skip(`ensureBuiltHooks() failed to populate hooks/dist: ${buildFailure.message}`); + return; + } + const distCliExitPath = path.join(DIST_LIB_DIR, 'cli-exit.js'); + if (!fs.existsSync(distCliExitPath)) { + t.skip(`${distCliExitPath} does not exist after ensureBuiltHooks() — build seam did not ship it`); + return; + } + // Spawn a child that requires the SHIPPED copy from ITS OWN dist + // location and drives the one sanctioned process.exit() call site + // (terminateNow) with a HOOK_DENY outcome. A dist copy that exists but + // whose sibling require('./exit-code-registry.js') cannot resolve from + // hooks/dist/lib/ (e.g. only cli-exit.js shipped, not the whole + // directory) would throw here instead of exiting 2 — this is the case a + // byte-comparison alone cannot catch. + const script = [ + `const { terminateNow } = require(${JSON.stringify(distCliExitPath)});`, + `terminateNow('HOOK_DENY', { x: 1 });`, + ].join('\n'); + const r = runNode(['-e', script], { timeoutMs: 15000 }); + assert.equal(r.outcome, OUTCOME.EXITED, `expected a clean exit; got ${r.outcome} stderr=${r.stderr}`); + assert.equal(r.exitCode, 2, `expected HOOK_DENY's registered exit code 2; stdout=${r.stdout} stderr=${r.stderr}`); + }); +}); + +// --------------------------------------------------------------------------- +// Drift guard — the table must not silently fall behind hooks/*.js. +// --------------------------------------------------------------------------- + +describe('hooks-crash-policy: table drift guard', () => { + test('every terminating hook file under hooks/ has a table row, and vice versa', () => { + const onDisk = fs.readdirSync(HOOKS_DIR) + .filter((name) => name.endsWith('.js') && !NON_TERMINATING_HOOKS.has(name)) + .sort(); + const inTable = TABLE.map((row) => row.file).sort(); + assert.deepEqual( + inTable, onDisk, + 'hooks/*.js and this table have drifted — add/remove a row so every ' + + 'enforcement hook (excluding the declared NON_TERMINATING_HOOKS) is covered.' + ); + }); + + test('NON_TERMINATING_HOOKS names actually exist on disk (no stale exclusion)', () => { + for (const name of NON_TERMINATING_HOOKS) { + assert.ok(fs.existsSync(hookPath(name)), `${name} is excluded but no longer exists under hooks/`); + } + }); +}); diff --git a/tests/shared-hooks-dir-resolution.test.cjs b/tests/shared-hooks-dir-resolution.test.cjs index 02e89a0d1..a9e615ca2 100644 --- a/tests/shared-hooks-dir-resolution.test.cjs +++ b/tests/shared-hooks-dir-resolution.test.cjs @@ -40,6 +40,7 @@ const path = require('node:path'); const { runNode, OUTCOME } = require('./helpers/process-seam.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { copyScriptWithDeps } = require('./helpers/copy-script-fixture.cjs'); const REPO_ROOT = path.join(__dirname, '..'); @@ -586,21 +587,21 @@ describe('GROUP C: bundle-directory-name-agnostic hook scripts', () => { t.after(() => cleanup(tmpRoot)); const bundleDir = path.join(tmpRoot, 'gsd-hooks'); - fs.mkdirSync(bundleDir, { recursive: true }); + // Stage via copyScriptWithDeps (walks the real require graph — see its doc + // comment / CLAUDE.md "no new copyFileSync line") into a throwaway staging + // root, then relocate the staged hooks/ subtree onto `gsd-hooks` — a + // NON-"hooks"-named directory is exactly the condition this test exists to + // exercise (#3023), so the staged tree cannot simply be used at its + // repo-relative `hooks/` location. A hand-copied dependency list is what + // caused this exact fixture to miss the scanner's #3911 `./lib/hook-exit.js` + // require (which itself pulls in cli-exit.js + exit-code-registry.js) — + // deriving the list instead of re-declaring it means a future require added + // to the scanner (or any of its deps) cannot be silently omitted here again. + const stageRoot = createTempDir('fix-3023-scanner-stage-'); + t.after(() => cleanup(stageRoot)); + copyScriptWithDeps(REPO_ROOT, stageRoot, 'hooks/gsd-read-injection-scanner.js'); + fs.cpSync(path.join(stageRoot, 'hooks'), bundleDir, { recursive: true }); const scannerPath = path.join(bundleDir, 'gsd-read-injection-scanner.js'); - fs.copyFileSync(path.join(REPO_ROOT, 'hooks', 'gsd-read-injection-scanner.js'), scannerPath); - // #3504: the scanner now requires hooks/lib/injection-patterns.js. Every - // real staging surface ships lib/ alongside the hook (installSharedHooksBundle - // copies dist recursively AND stages hooks/lib from the GSD_HOOK_LIB_FILES - // allowlist into the same shared dir), so this lone-file emulation must - // stage the dependency the same way — a missing lib file is a packaging - // bug that fails loud at hook load (#2587), which is exactly what the - // un-staged version of this fixture now demonstrates. - fs.mkdirSync(path.join(bundleDir, 'lib'), { recursive: true }); - fs.copyFileSync( - path.join(REPO_ROOT, 'hooks', 'lib', 'injection-patterns.js'), - path.join(bundleDir, 'lib', 'injection-patterns.js'), - ); // Node canonicalizes a module's __dirname via the REAL (symlink-resolved) // path, so a payload path must be built from the same realpath — on macOS