From 2ea5efc15172ff6ff39d8d3a50db5ee9cbe260fe Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 27 Aug 2026 22:21:10 -0400 Subject: [PATCH] enhance(#3911): hooks declare their crash policy (#3960) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * enhance(#3911): give hooks an exit seam that needs no build ADR-3889 Phase 7 foundation. The 19 shipped enforcement hooks hold 91 of the epic's 128 terminators and cannot reach `terminateNow` today. The obvious route — requiring `gsd-core/bin/lib/cli-exit.cjs`, as gsd-agent-isolation-guard.js already does for two other modules — is rejected. That precedent carries its own warning (#3582): those files are tsc output, gitignored and absent on a raw plugin-marketplace or git-clone install, so the hook must first call ensureRuntimeBuild() to self-heal. Making the module a hook needs IN ORDER TO TERMINATE depend on a build inverts the dependency, and its failure mode is precisely the fail-open this phase exists to remove: a guard that cannot terminate cannot deny. `lint-hooks-runtime-build-seam` already encodes that concern, and Design B would have had to add an ensureRuntimeBuild() call to all 19 hooks to satisfy it. So `hooks/lib/` becomes a third emit location for cli-exit and a fifth for the registry, preserving the invariant `src/cli-exit.cts`'s own header states: it imports nothing but node:fs and its sibling registry, and the generator dual-emits that sibling alongside each copy so a relative require resolves next to whichever copy loaded it. Shipping needed no change — build-hooks.js already declares HOOKS_SUBDIRS_TO_COPY = ['lib']. Proven, not asserted: the two files are copied into an otherwise-empty tmpdir and a child process requires them and terminates — PASS exits 0, HOOK_DENY exits 2 with the payload on both stdout and stderr. That test fails the moment the hooks copy gains a require reaching outside hooks/lib/. Also fixed inline: the registry's fifth target let any `--write` test overwrite the real committed hooks/lib/exit-code-registry.js, because the test helper derived only three of the other output paths. It now redirects all five, and a regression test asserts every committed artifact is byte-identical after a redirected write. Install-tree goldens pick up the two new shipped paths across 11 runtimes — insertions only, no removals. lint:ci was green while they were stale, so this was found by regenerating rather than by a gate. Verification runs on the remote runner. Refs #3911 * enhance(#3911): declare a crash policy, and migrate the write guard Adds `hooks/lib/hook-exit.js` — the hook-facing vocabulary over `terminateNow`, hand-written because the cli-exit copy beside it is generated: allow(payload) exit 0 deny(payload, stderr?) exit 2 crash(onCrash, payload) whichever the hook DECLARED `crash()` takes the policy as a required argument with no default, which is the whole mechanism: fail-open by accident stops being expressible. A hook must name ALLOW or DENY at the call site, and an unrecognized value terminates INTERNAL rather than guessing. Fail-open stays legal; fail-open by omission does not. `gsd-write-guard.js` is the first hook migrated, all 12 sites, and it exposed a gap in the seam. `terminateNow`'s doc comment justified its fd-2 write by citing this hook's `emitBlock` — but modeled it as sending the same bytes to both streams, when `emitBlock` actually sends full JSON to stdout and only the bare `reason` string to stderr, because Kimi's hook bus feeds stderr verbatim back to the model. Migrating as written would have turned a readable sentence into a JSON blob for Kimi-backed agents. #3911 requires both "all 19 hooks terminate through terminateNow" and "no hook's effective default changes". Those are jointly satisfiable only by teaching the seam to carry a distinct stderr payload, so `terminateNow` gains an optional third argument: omitted, behavior is byte-for-byte what it was; a string is written raw, which is exactly the Kimi case. The doc comment's inaccurate claim about emitBlock is corrected in place. Proven rather than asserted: the pre-migration file is reconstructed from HEAD and driven with the same catastrophic-shrink payload as the migrated one — exit code, stdout and stderr all byte-identical. Verification runs on the remote runner. Refs #3911 * enhance(#3911): all 19 hooks terminate through the seam Migrates the remaining 18 enforcement hooks onto allow/deny/crash. An AST walk now reports zero `process.exit(` call sites across every `hooks/*.js` — down from the 91 the census measured. Each hook with an outer catch declares its policy once, at module top, with the reason that policy is right for that specific guard: a read guard that cannot scan must not block the read; a statusline that renders every prompt must degrade rather than crash; an injection scanner must not retroactively block a result already returned. Those sentences are the deliverable — they are what turns fail-open-by-accident into fail-open-on-purpose. No hook's effective default changed. Wiring exposed two defects, both fixed here rather than noted. A SECOND stdout/stderr-splitting site turned up in `gsd-workflow-guard.js`'s `emitForceAddBlock`, matching the pattern already known from the write guard — full JSON to stdout, bare reason to stderr for the Kimi bus. It uses the `stderrPayload` argument added in the previous commit, which is now carrying its second real caller rather than one special case. More seriously, `terminateNow` emitted both streams inside ONE try, so a payload that failed to serialize aborted before the stderr write ever ran. The two windsurf guards write nothing to stdout on a block and only a reason string to stderr, so `deny(undefined, reason)` exited 2 with EMPTY stderr — a deny that silently loses its reason, which is the exact "fails with success" class this epic exists to close. The streams are now emitted independently, each with its own guard, and `undefined` means "nothing to write for this stream" rather than an error. Regression tests inject a throwing write on one fd and assert the other still receives its payload; they fail against the single-try version. Byte-identity was proven per hook, not assumed: each pre-change file is reconstructed from HEAD and driven side by side with the migrated one across its normal path, its deny path, malformed stdin and empty stdin — exit code, stdout and stderr compared. Verification runs on the remote runner. Refs #3911 * enhance(#3911): harden the three shell hooks, and pin every hook's policy `gsd-phase-boundary.sh`, `gsd-session-state.sh` and `gsd-validate-commit.sh` gain `set -euo pipefail`. The expected hazard did not materialize, and that is worth recording: every intentionally-non-zero command in all three is already the condition of an `if`/`elif`, which `set -e` never fires on, and none of them reads a possibly-unset variable or pipes through a grep that may legitimately match nothing. No `|| true` guards were needed. Each hook was still checked command-by-command before the flags went in rather than after. Twenty-one before/after cases across the three hooks — disabled and enabled, planning and non-planning, missing STATE.md, malformed JSON, the Kimi payload shape, quoted and unquoted `-m`, valid and over-long Conventional Commits — all match on exit code, stdout and stderr. The hardening is shown to actually fire, not merely added: with a stubbed `node` that fails at the JSON-emit step, phase-boundary and session-state go from silently exiting 0 with empty stdout to failing visibly with the error surfaced. No such case could be constructed for `gsd-validate-commit.sh`, whose every statement already sits inside an if-condition — recorded as unproven rather than claimed. `tests/hooks-crash-policy.test.cjs` adds the per-hook coverage the issue asks for, table-driven over all 19 hooks rather than 76 hand-written cases: normal allow, deny where a deny path exists, crash-honors-the-declared-policy, and an unclosed-stdin case — the one `process.exitCode` structurally cannot serve. The deny assertions encode each hook's ACTUAL stream split rather than a uniform shape, since four of the six deliberately differ. A drift guard enumerates `hooks/*.js` and fails if a terminating hook is ever added without a row. Writing those tests surfaced two hooks that emit a block decision in their JSON body and exit 0. Both were checked rather than assumed, and neither is a fails-with-success: `gsd-read-injection-scanner.js` is PostToolUse, where the tool has already run and exit 2 has no meaning, and `gsd-cursor-subagent-start.js` follows Cursor's JSON-body protocol. They are deliberately left alone — a mechanical sweep to `deny()` would have broken exactly these two. Verification runs on the remote runner. Refs #3911 * fix(#3838): the commit validator says when it could not validate #3911 claims to subsume #3838. Measurement said otherwise, so this closes it for real rather than by assertion. `set -euo pipefail`, added earlier on this branch, does NOT fix #3838: bash exempts a command used as an `if` condition from `set -e`, and all three of the hook's swallow-and-pass sites are exactly that shape. Verified against the hardened hook with a node shim that fails only the classifier call — a non-conforming commit still exited 0 with empty stdout AND empty stderr, indistinguishable from "your commit conforms". That is the defect verbatim. All three sites named in #3838 now capture the real exit status instead of consuming it as a condition, and each distinguishes its genuine negative from "could not run": - the classifier: 0 = is a git commit, 1 = genuinely not one, anything else = could not classify. Its `node -e` now wraps the require and the call in try/catch and exits 3 on a throw, so a broken require chain can never be mistaken for `isGitSubcommand` legitimately returning false — which is the arm that matters, since `token-scanner.cjs` is a gitignored build artifact and a fresh checkout lands there. - the opt-in config read and the JSON command extraction get the same treatment. On "could not run" the hook emits a diagnostic to stderr naming which check failed and why, then exits 0. The issue confirms this is safe — it is a PreToolUse hook, so stderr does not disturb the JSON protocol — and ranks it the smallest sufficient fix. The gate still fails open, but it can no longer do so silently, which is the whole complaint: a validator that disables itself quietly costs more than one that is absent, because it is trusted. Both controls are unchanged and pinned by tests: a conforming commit still passes silently, a non-conforming one still exits 2 with its existing block payload. The defect test asserts stderr is non-empty and names the failure; it fails against the pre-fix hook. Verification runs on the remote runner. Refs #3911, #3838 * docs(#3911): document the hook crash-policy contract Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it), INVENTORY rows for the three new hooks/lib files, and an ARCHITECTURE note on the hooks section. How-To: docs/how-to/declare-a-hook-crash-policy.md, indexed from docs/README.md — a hook author now has to choose and declare a crash policy, which is more than one step and crosses into which harness protocol their hook speaks. It covers allow/deny/crash, writing an ON_CRASH reason that is actually useful, when a deny needs a distinct stderr payload, the two hooks whose harness reads a JSON-body decision and must NOT use deny(), and what to do when a check cannot run at all — with #3838 as the worked example. Refs #3911 * test(#3911): prove the seam actually ships, and stop hand-rolling temp cleanup Two review findings. The acceptance criterion 'hooks/dist/** stays in parity via the build seam (lint:hooks-runtime-build-seam)' was misstated and unmet: that lint checks something else — that a hook requiring a compiled gsd-core/bin/lib module also calls ensureRuntimeBuild(). Nothing exercised that the three new hooks/lib files reach hooks/dist/lib at all. That gap is not theoretical: #770 is a recorded ship-blocking bug where a new hook never shipped because a copy list missed it. The suite now builds dist through the repo's own ensureBuiltHooks(), byte-compares each shipped copy against its source, and spawns a child that requires the SHIPPED dist copy and denies — which is what catches a copy that exists but cannot resolve its sibling registry. gsd-validate-commit.sh hand-duplicated mktemp/run/rm three times; one idempotent trap on EXIT replaces them, guarded so cleanup cannot alter the exit status. Behavior-neutral across five cases, with temp-file counts taken before and after each run. Refs #3911 * fix(#3911): stage transitive hook lib requires, not just one level The remote run returned 7 failures across 3 real causes. The important one is a PRODUCTION bug this phase exposed rather than caused. `writeCursorHooksJson` scanned each hook script for `./lib/X` requires exactly one level deep and never re-scanned the lib files it staged for their own sibling requires. Nothing had a transitive lib dependency before, so the gap was invisible. Adding hook-exit.js -> cli-exit.js -> exit-code-registry.js made real Cursor installs ship a bundle that dies at require time with MODULE_NOT_FOUND. It now walks to a fixed point, and a real installed Cursor hook runs to completion. The staging harness in shared-hooks-dir-resolution hand-copied its fixture, so the injection scanner crashed at require time and its exit-1 was being read as a policy decision. Migrated to copyScriptWithDeps, which walks the require graph — the repo's recorded rule for this class, since adding another copyFileSync keeps it alive for the next person. The missing-lib-source test in cursor-hook-workspace-roots hardcoded which lib file it expected to be named in the abort message; the same throw now fires for a different file first. Its assertion is unchanged in substance — staging still must abort rather than ship a broken hook — only the name is no longer pinned. The last one was my own test asserting an uppercase reason code. Measured against origin/next: the pre-change hook emits the same lowercase 'config_unreadable', so the test was wrong, not the migration. Corrected to the real value rather than making the code match the test. Verification runs on the remote runner. Refs #3911 * chore(#3911): regenerate the cursor install-tree golden The staging fix means a Cursor install now correctly carries the two transitive lib files it was silently missing. Additive only — no path was removed. The golden diff is the evidence the packaging defect was real. Refs #3911 * chore(#3911): backfill the changeset PR number Refs #3911 * fix(#3911): a git probe that timed out is not a negative A macOS CI lane failed three deny cases at 2084ms, 2112ms and 2177ms — just past the 2000ms budget these hooks give their git probes. The three that passed took 72ms, 595ms and 651ms. Under shard contention `git rev-parse` overruns, the hook reads the non-zero result as "not a git repo", and allows with exit 0 and empty stdout AND empty stderr. Under load, the guards silently stop guarding. That is ADR-3889's thesis exactly, sitting inside the security hooks this phase is about. The repo had already recognized the class in one place — gsd-cursor-subagent-start.js fail-closed-denies on `git_timed_out` (#3045) — but nowhere else. `hooks/lib/git-probe.js` classifies a probe's outcome, distinguishing a real non-zero exit from ETIMEDOUT, a signal kill, and a spawn failure, rather than folding all four into `status !== 0`. Three guards route their eight git probes through it. The resolution is the same shape #3838 took, and the same one that issue endorsed as smallest-sufficient: fail open, but loudly. **No exit code changes on any path** — a developer on a loaded machine is still not blocked, which keeps #3911's declaration-pass contract intact for exit codes. What changes is that the hook now says on stderr which probe could not answer, instead of presenting silence as a clean verdict. Scope was checked across every hooks/*.js, not just the three that failed: gsd-agent-isolation-guard spawns no git; gsd-statusline's two probes gate only a cosmetic display segment, not an allow/deny decision, and are left alone. The C2 deny assertion was a real-race test — it demanded exit 2 while a slow git legitimately yields 0. It now requires the hook to either deny, or allow with a diagnostic naming the probe that could not run; a silent allow still fails, so the assertion is not vacuous. A deterministic regression stubs git on PATH to sleep past the budget rather than waiting for load to reproduce it. Verification runs on the remote runner. Refs #3911 * test(#3911): a PATH shim cannot intercept the hooks' git spawn on Windows The deterministic timeout regression stubbed git on PATH and asserted the guard reports rather than silently allows. It passes on Linux and macOS and failed on Windows in 83ms and 176ms — the stub was never invoked at all. Mechanism: the hooks call spawnSync('git', args) with no shell:true, so on Windows CreateProcess resolves git.exe only and never a PATH .cmd shim. The git.cmd branch could not have worked and is removed rather than left implying a Windows path that does. Adding shell:true to the hooks to serve a test would change product behavior and widen an injection surface, so the case is skipped on win32 only, with the mechanism written into the skip reason so a future reader does not 'fix' it that way. Linux and macOS keep the coverage, and macOS is where the underlying fail-open was actually caught. Refs #3911 --------- Co-authored-by: sim --- .changeset/silly-finches-squeak.md | 5 + docs/ARCHITECTURE.md | 12 + docs/FEATURES.md | 57 ++ docs/INVENTORY.md | 10 + docs/README.md | 1 + .../hooks-declare-their-crash-policy.md | 57 ++ docs/how-to/declare-a-hook-crash-policy.md | 173 +++++ eslint.config.mjs | 6 + gsd-core/bin/lib/exit-code-registry.cjs | 9 +- hooks/gsd-agent-isolation-guard.js | 29 +- hooks/gsd-config-reload.js | 30 +- hooks/gsd-context-monitor.js | 29 +- hooks/gsd-cursor-post-tool.js | 4 +- hooks/gsd-cursor-pre-tool.js | 4 +- hooks/gsd-cursor-session-start.js | 3 +- hooks/gsd-cursor-stop.js | 3 +- hooks/gsd-cursor-subagent-start.js | 3 +- hooks/gsd-cursor-subagent-stop.js | 4 +- hooks/gsd-ensure-canonical-path.js | 3 +- hooks/gsd-phase-boundary.sh | 1 + hooks/gsd-prompt-guard.js | 23 +- hooks/gsd-read-guard.js | 23 +- hooks/gsd-read-injection-scanner.js | 25 +- hooks/gsd-session-state.sh | 1 + hooks/gsd-statusline.js | 18 +- hooks/gsd-validate-commit.sh | 86 ++- hooks/gsd-windsurf-pre-command.js | 27 +- hooks/gsd-windsurf-pre-write.js | 35 +- hooks/gsd-workflow-guard.js | 50 +- hooks/gsd-worktree-path-guard.js | 57 +- hooks/gsd-write-guard.js | 60 +- hooks/lib/cli-exit.js | 461 ++++++++++++ hooks/lib/exit-code-registry.js | 98 +++ hooks/lib/git-probe.js | 84 +++ hooks/lib/hook-exit.js | 81 ++ package.json | 4 +- scripts/gen-exit-code-registry.cjs | 79 +- scripts/gen-hooks-cli-exit.cjs | 207 ++++++ scripts/lib/cli-exit.cjs | 67 +- scripts/lib/exit-code-registry.cjs | 9 +- src/cli-exit.cts | 70 +- src/runtime-hooks-surface.cts | 44 +- tests/cli-exit.test.cjs | 347 +++++++++ tests/cursor-hook-workspace-roots.test.cjs | 10 +- tests/exit-code-registry.test.cjs | 62 +- tests/fixtures/install-tree/antigravity.json | 4 + tests/fixtures/install-tree/augment.json | 4 + tests/fixtures/install-tree/claude-local.json | 4 + tests/fixtures/install-tree/claude.json | 4 + tests/fixtures/install-tree/codebuddy.json | 4 + tests/fixtures/install-tree/cursor.json | 3 + tests/fixtures/install-tree/hermes.json | 4 + tests/fixtures/install-tree/kilo.json | 4 + tests/fixtures/install-tree/kimi-code.json | 4 + tests/fixtures/install-tree/opencode.json | 4 + tests/fixtures/install-tree/pi.json | 4 + tests/fixtures/install-tree/qwen.json | 4 + .../gsd-validate-commit-crash-policy.test.cjs | 171 +++++ tests/hooks-crash-policy.test.cjs | 697 ++++++++++++++++++ tests/shared-hooks-dir-resolution.test.cjs | 29 +- 60 files changed, 3156 insertions(+), 259 deletions(-) create mode 100644 .changeset/silly-finches-squeak.md create mode 100644 docs/features/hooks-declare-their-crash-policy.md create mode 100644 docs/how-to/declare-a-hook-crash-policy.md create mode 100644 hooks/lib/cli-exit.js create mode 100644 hooks/lib/exit-code-registry.js create mode 100644 hooks/lib/git-probe.js create mode 100644 hooks/lib/hook-exit.js create mode 100644 scripts/gen-hooks-cli-exit.cjs create mode 100644 tests/gsd-validate-commit-crash-policy.test.cjs create mode 100644 tests/hooks-crash-policy.test.cjs 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