From c6efe2905c4f90fd06e0d5e5714f68cc56bdf918 Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani <20915308+behruznassre@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:49:37 -0700 Subject: [PATCH] fix(#4087): stage the hook helpers the Codex bundle's hooks require (#4117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4087): stage the hook helpers the Codex bundle's hooks require CODEX_HOOKS_TO_COPY is a flat, hand-maintained filename allowlist that never recursed, and Codex is excluded from installSharedHooksBundle() — the path that stages hooks/lib/ for full-bundle runtimes — by an !isCodex gate. Excluding hooks/lib/ was a correct scoped decision for #3579 until #3911 (2ea5efc15) gave gsd-context-monitor.js a real require('./lib/hook-exit.js'). From then on every fresh --codex install staged the hook without its helper and the hook died with MODULE_NOT_FOUND at module load, before its own try/catch, on every event Codex registers it for. The install still exited 0, so nothing surfaced it. Reproduced before changing anything, in a sandboxed CODEX_HOME: four hooks staged, no lib/, and the installed hook exiting 1 on "Cannot find module './lib/hook-exit.js'". Rather than hand-add today's three helpers — which re-breaks the next time a Codex-bundled hook grows a lib dependency, exactly how this regressed — the transitive-require walk already written for Cursor in 704859e9c is extracted out of writeCursorHooksJson into an exported stageTransitiveHookLibs(), Cursor is rewired onto it, and the Codex copy loop calls it. bin/install.js already required that module, so this adds no new seam. Cursor's staged set is byte-identical to base, compared file by file. Extraction surfaced a latent defect in that walker, fixed here: its regex read `./X` and `./lib/X` identically, but from a hook SCRIPT a bare `./X` is a sibling in hooks/ — gsd-check-update-worker.js requires `./managed-hooks-registry.cjs`, which is not a lib — so it demanded hooks/lib/managed-hooks-registry.cjs and the fail-loud guard threw. Seeds now match only `./lib/X`; lib files still match both, which is the sibling-within-lib case 704859e9c exists for. Cursor never exposed it because none of its scripts carries a bare sibling require. Three further grammar gaps closed after review, each in the fail-closed direction: an extensionless `require('./lib/x')` is valid CommonJS and was resolved literally, failing the install on a legitimate require — now resolved through .js/.cjs and written under its resolved name; a NESTED `./lib/sub/x.js` could not be expressed by the character class and was a SILENT miss, the one failure mode this function exists to remove — now refused loudly; and a capture carrying no alphanumeric character is prose, not a module name — hooks/lib/ injection-patterns.js documents this very mechanism with the literal string require('./lib/...'), which captured `...` and sent the resolver hunting for hooks/lib/... . The scan is still not comment-aware, which is disclosed at the call site rather than papered over. Seeded from the entries THIS invocation staged rather than probing the destination, so a file left by an earlier install whose source is no longer allowlisted cannot contribute helpers for a hook that is no longer shipped. The #3579 boundary holds: three of ten helpers ship, gsd-graphify-rebuild.sh among those correctly absent. Seven rows — three driving a real install into a sandboxed config dir (with HOME sandboxed for the child, since Codex's skills kind resolves from os.homedir() and the #3712 guard rightly refuses otherwise) and four pinning the discovery grammar directly. All proven fail-first; the set-equality row also reds on over-staging, which the count-based version it replaced did not catch. Fixes #4087 Fixes #4098 Emitted-Drift-Ack-Hash: hooks/lib/hook-exit.js — newly emitted for codex because the installer now stages the helpers its hooks require; the helper's own content is unchanged Emitted-Drift-Ack-Hash: hooks/lib/cli-exit.js — newly emitted for codex as hook-exit.js's transitive require; the helper's own content is unchanged Emitted-Drift-Ack-Hash: hooks/lib/exit-code-registry.js — newly emitted for codex as cli-exit.js's transitive require; the helper's own content is unchanged Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018FUAVz49BghqxoJgwt7EW9 * chore(#4087): add changeset Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018FUAVz49BghqxoJgwt7EW9 * fix(#4087): stage the hooks/lib helpers the Windsurf guards require Review of #4117, verified as asked and reproduced against a real install. Windsurf sets hostBehaviors.skipSharedHooksInstall, so like Cursor it never reaches installSharedHooksBundle -- the only other stager of hooks/lib -- and writeWindsurfHooksJson staged its two Cascade guards without the helpers both require at module load: gsd-windsurf-pre-write.js requires ./lib/hook-exit.js and ./lib/git-probe.js, gsd-windsurf-pre-command.js requires ./lib/hook-exit.js. stageTransitiveHookLibs had one call site, Cursor's. Measured on a fresh `--windsurf --global` install into a sandboxed HOME: the installer exited 0, hooks/ held only the two scripts and package.json, and executing either installed guard exited 1 with "Cannot find module './lib/hook-exit.js'" -- so every pre_write_code and pre_run_command event failed at load while the install reported success. The same command with `--cursor` staged four helpers and its hook ran, which is the control. Pre-existing rather than introduced here: at merge-base 05092ff36 the same three require lines exist and writeWindsurfHooksJson already staged no lib/, and this PR's diff carried no reference to Windsurf. Fixed here anyway because the helper this PR extracted is the right tool and a second runtime is a few lines onto it. writeWindsurfHooksJson now calls stageTransitiveHookLibs after staging its scripts, with the same gsd: -> gsd- transform the scripts receive, so a helper is rewritten the same way as its caller. The install-tree fixture regenerates with exactly hook-exit.js, git-probe.js, cli-exit.js and exit-code-registry.js added and no other fixture moved. Two rows execute the INSTALLED guards, beside the Codex rows they mirror; the existing windsurf-hooks-bridge rows run the guards from source and test behaviour, a different question, and are left as they are. Both new rows fail-first against the unfixed compiled artifact -- gsd-core/bin/lib, which is what bin/install.js loads -- on the MODULE_NOT_FOUND assertion. Emitted-Drift-Ack-Hash: hooks/lib/git-probe.js — first staged for Windsurf, whose pre-write guard requires it; the file itself is unchanged Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TadqrpTE2m6gCB7CaNNLcy --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Tom Boucher --- .changeset/noble-goats-gather.md | 5 + bin/install.js | 55 +++- src/runtime-hooks-surface.cts | 199 ++++++++++--- tests/fixtures/install-tree/codex.json | 3 + tests/fixtures/install-tree/windsurf.json | 4 + tests/install-minimal-hooks.test.cjs | 335 ++++++++++++++++++++++ 6 files changed, 558 insertions(+), 43 deletions(-) create mode 100644 .changeset/noble-goats-gather.md diff --git a/.changeset/noble-goats-gather.md b/.changeset/noble-goats-gather.md new file mode 100644 index 000000000..5b2e0423d --- /dev/null +++ b/.changeset/noble-goats-gather.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4117 +--- +**Codex installs no longer ship a hook that cannot load** — with `--codex`, `gsd-context-monitor.js` was staged without the `hooks/lib/` helpers it requires, so it failed with a missing-module error at load, before its own error handling, on every event Codex registers it for. The install still reported success, so the only symptom was a Codex session erroring on each prompt. The helpers a Codex-bundled hook needs are now derived from what the staged scripts actually require, followed through helpers that require other helpers, rather than from a hand-maintained list that could not keep up: the same list had gone stale once already, which is how this broke. Helpers no Codex hook requires are still not shipped, and a hook whose helper is genuinely missing from the source now fails the install loudly instead of installing something that cannot run. Windsurf had the same gap, found in review: both Cascade guards require `hooks/lib/` helpers at load and a fresh `--windsurf` install staged neither, so every `pre_write_code` and `pre_run_command` event failed the same way. Windsurf is now wired onto the same derivation, and its installed guards are executed by the tests rather than only checked for existence. Full-bundle runtimes and Cursor are unaffected — Cursor's staged set is byte-identical. (#4087) (#4098) diff --git a/bin/install.js b/bin/install.js index 203417f9b..372122858 100755 --- a/bin/install.js +++ b/bin/install.js @@ -12064,8 +12064,18 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { // require()s managed-hooks-registry.cjs for MANAGED_HOOKS — so all four must be // installed/refreshed together for every profile, or Codex is wired to a dependency // chain the same installer never delivers. - // We deliberately do *not* copy gsd-graphify-update.sh or hooks/lib/ for Codex - // in this change (graphify auto-update support for Codex is out of scope for #3579). + // We deliberately do *not* copy gsd-graphify-update.sh for Codex in this + // change (graphify auto-update support for Codex is out of scope for #3579). + // hooks/lib/ WAS excluded here for the same reason, and that stopped being + // correct when #3911 (2ea5efc15) gave gsd-context-monitor.js a real + // `require('./lib/hook-exit.js')`: the allowlist below is flat and never + // recursed, so the hook shipped without its helper and died with + // MODULE_NOT_FOUND at load, before its own try/catch, on every registered + // event (#4087, #4098). The libs are now derived from what the staged + // scripts actually require rather than hand-listed — see the + // stageTransitiveHookLibs call after the copy loop. The #3579 boundary is + // preserved: helpers no staged Codex hook requires (graphify tooling among + // them) are still not shipped. const CODEX_HOOKS_TO_COPY = [ 'gsd-check-update.js', 'gsd-check-update-worker.js', @@ -12081,6 +12091,12 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { // is not the same as an allowlisted file landing in it — see the marker // gate below. let codexStagedHooks = false; + // The entries THIS invocation actually staged. Seeding the lib scan from + // `existsSync` over the destination instead would also pick up a file + // left by a PREVIOUS install whose source is no longer staged — e.g. a + // name dropped from the allowlist — and derive helpers for a hook that is + // no longer shipped (review of #4087). + const codexStagedEntries = []; for (const entry of fs.readdirSync(codexHooksSrc)) { if (!CODEX_HOOKS_TO_COPY.includes(entry)) continue; const srcFile = path.join(codexHooksSrc, entry); @@ -12115,8 +12131,43 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } } codexStagedHooks = true; + codexStagedEntries.push(entry); + } + // Stage the hooks/lib/ helpers the staged scripts require, transitively + // (#4087, #4098). Shares writeCursorHooksJson's walker rather than a + // second copy: both reduced bundles hand-pick SCRIPTS, and the identical + // MODULE_NOT_FOUND was already fixed once for Cursor in 704859e9c. A flat + // list of today's three helpers would re-break the next time a + // Codex-bundled hook grows a lib dependency, which is exactly how this + // regressed. Gated on codexStagedHooks for the same reason the CommonJS + // marker below is: hooks/ is shared space, and staging nothing must not + // leave a GSD-owned lib/ behind in a directory GSD created but did not + // fill (#2544). Seeded from the copies staged by THIS invocation, so the + // scan sees the same bytes Node will load and never derives helpers for a + // hook left behind by an earlier install. + let codexStagedLibs = []; + if (codexStagedHooks) { + codexStagedLibs = hooksSurface.stageTransitiveHookLibs({ + seedSources: codexStagedEntries + .map((entry) => fs.readFileSync(path.join(codexHooksDest, entry), 'utf8')), + srcLibDir: path.join(codexHooksSrc, 'lib'), + destLibDir: path.join(codexHooksDest, 'lib'), + runtimeLabel: 'Codex', + // Same substitutions the .js branch above applies to hook scripts, so + // a helper that ever gains a runtime path or version token is + // rewritten identically instead of shipping a Claude-shaped path. + // No-ops on today's helpers, which carry neither. + transform: (content) => content + .replace(/'\.claude'/g, configDirReplacement) + .replace(/\/\.claude\//g, `/${getDirName(runtime)}/`) + .replace(/\.claude\//g, `${getDirName(runtime)}/`) + .replace(/\{\{GSD_VERSION\}\}/g, pkg.version), + }); } console.log(` ${green}✓${reset} Installed hooks (Codex)`); + if (codexStagedLibs.length > 0) { + console.log(` ${green}✓${reset} Installed hooks/lib/ helpers (${codexStagedLibs.join(', ')})`); + } // #2717: write the CommonJS marker into hooks/ alongside the staged .js // scripts. Codex is excluded from installSharedHooksBundle by the // !isCodex gate, so it never received the marker the shared-bundle path diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index 31ef90ce9..cca8859a2 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -1521,6 +1521,135 @@ interface WriteCursorHooksJsonOpts { managedHookEvents?: readonly string[]; } +/** + * Stage the `hooks/lib/` helpers a set of staged hook scripts require, walking + * the require graph TRANSITIVELY to a fixed point. + * + * Extracted from writeCursorHooksJson (#3911 review, commit 704859e9c) so the + * reduced Codex hook bundle can share one implementation instead of growing a + * second, divergent copy (#4087 / #4098). Both reduced bundles hand-pick which + * hook SCRIPTS they ship, and neither can hand-pick their helpers correctly for + * long: `hooks/lib/hook-exit.js` requires `./cli-exit.js`, which requires + * `./exit-code-registry.js` — with NO `./lib/` prefix, because from inside + * `lib/` the sibling is already local. A single-pass scan for the `./lib/…` + * spelling used FROM a hook script stages hook-exit.js and stops, and the + * installed hook then dies on MODULE_NOT_FOUND at load, before its own + * try/catch, on every event it is registered for. + * + * Scans CONTENT rather than paths so a caller can seed from whatever it staged, + * transformed or not, without this helper knowing the caller's layout. + * + * @returns the lib filenames actually staged, in staging order. + */ +function stageTransitiveHookLibs(opts: { + seedSources: string[]; + srcLibDir: string; + destLibDir: string; + runtimeLabel: string; + transform?: (content: string) => string; +}): string[] { + const { seedSources, srcLibDir, destLibDir, runtimeLabel, transform } = opts; + const requiredLibFiles = new Set(); + const scannedLibFiles = new Set(); + const staged: string[] = []; + // `./X` means DIFFERENT things depending on where the scanned file lives, and + // conflating them stages the wrong file. From a hook SCRIPT in hooks/, a bare + // `./X` is a sibling hook-level artifact — Codex's gsd-check-update-worker.js + // requires `./managed-hooks-registry.cjs`, which lives in hooks/, not + // hooks/lib/ — so only the explicit `./lib/X` spelling is a lib requirement. + // From inside a LIB file, the sibling is already local, so `./X` IS a lib + // requirement (hook-exit.js -> ./cli-exit.js -> ./exit-code-registry.js); that + // is the case 704859e9c added and it must keep working. Cursor never exposed + // the difference because none of its staged scripts has a bare sibling + // require; Codex's does, and the fail-loud guard below caught it immediately + // by demanding managed-hooks-registry.cjs out of hooks/dist/lib. + // Fresh per call: a module-level /g regex carries lastIndex across calls and + // would silently skip matches on the second install in one process. + const seedRequireRe = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; + const libRequireRe = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; + // A NESTED helper path is outside the flat layout hooks/lib/ has and the + // build emits, and the character classes above cannot express it — so it + // would be a SILENT miss, staging nothing and shipping a hook that dies at + // load. Detected separately and refused loudly instead: a silent miss is the + // failure mode this whole function exists to remove (review of #4087). + const nestedRequireRe = /require\(\s*['"]\.\/lib\/[A-Za-z0-9._-]+\/[^'"]*['"]\s*\)/; + + const scanForLibRequires = (source: string, fromLib: boolean): void => { + if (nestedRequireRe.test(source)) { + throw new Error( + `A staged ${runtimeLabel} hook requires a NESTED hooks/lib path. hooks/lib/ is flat and ` + + 'this stager only resolves flat helper names, so the nested helper would never be ' + + 'staged and the hook would throw MODULE_NOT_FOUND at load. Flatten the helper or ' + + 'extend this stager deliberately.', + ); + } + const re = fromLib ? libRequireRe : seedRequireRe; + re.lastIndex = 0; + let m: RegExpExecArray | null; + while ((m = re.exec(source)) !== null) { + const candidate = m[1]; + // A capture with no alphanumeric character is not a module name — it is + // prose. This scan reads whole file text, comments included, and + // hooks/lib/injection-patterns.js's own header documents this mechanism + // with the literal string `require('./lib/...')`, which captures `...` + // and would send the resolver hunting for `hooks/lib/...` and fail the + // install (measured; that helper is not staged for either reduced bundle + // today, so it is latent rather than live). + // + // KNOWN LIMIT, disclosed rather than papered over: this does NOT make the + // scan comment-aware. A comment naming a REAL helper — `require( + // './lib/git-cmd.js')` in prose — still registers it and would over-stage + // that helper. Closing that needs a comment-stripping pass; the + // line-based stripper in scripts/lint-hooks-runtime-build-seam.cjs is the + // precedent (its header explains why the naive two-regex strip corrupts + // these very files), but promoting a lint-script helper into installer + // runtime code is a larger change than this fix. + if (!/[A-Za-z0-9]/.test(candidate)) continue; + requiredLibFiles.add(candidate); + } + }; + + for (const source of seedSources) scanForLibRequires(source, false); + if (requiredLibFiles.size === 0) return staged; + + fs.mkdirSync(destLibDir, { recursive: true }); + // 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); + // Node's own extension resolution: `require('./lib/x')` is a valid, working + // CommonJS spelling today, and matching only the extension-bearing form + // resolved `x` literally, found nothing, and failed the install on a + // legitimate require (review of #4087). Try the bare name first so an + // extension-bearing capture still wins, then .js/.cjs. + let resolvedName: string | undefined; + for (const candidate of [libFile, `${libFile}.js`, `${libFile}.cjs`]) { + if (fs.existsSync(path.join(srcLibDir, candidate))) { resolvedName = candidate; break; } + } + const libSrc = path.join(srcLibDir, resolvedName ?? libFile); + if (resolvedName === undefined) { + // FAIL LOUD. Skipping here would ship hook scripts whose top-level + // require() throws before their own try/catch, wedging every session — + // and the install would still exit 0, so nobody would know until a user + // hit it. A missing helper source is a packaging bug; surface it. + throw new Error( + `hooks/lib/${libFile} is required by a staged ${runtimeLabel} hook but is missing from ${srcLibDir}. ` + + 'Installing would ship a hook that throws MODULE_NOT_FOUND at load.', + ); + } + let libContent = fs.readFileSync(libSrc, 'utf8'); + if (transform) libContent = transform(libContent); + // Written under its RESOLVED name so an extensionless require still lands a + // file Node can resolve at the destination. + fs.writeFileSync(path.join(destLibDir, resolvedName), libContent); + staged.push(resolvedName); + scanForLibRequires(libContent, true); + libFile = [...requiredLibFiles].find((f) => !scannedLibFiles.has(f)); + } + return staged; +} + function writeCursorHooksJson(targetDir: string, src: string, opts?: WriteCursorHooksJsonOpts): { hooksJsonPath: string; changed: boolean } { opts = opts || {}; const hooksDir = path.join(targetDir, 'hooks'); @@ -1557,47 +1686,13 @@ function writeCursorHooksJson(targetDir: string, src: string, opts?: WriteCursor // "./lib/X" requires, and every lib file staged is itself scanned for further // "./lib/X" OR bare "./X" (sibling-within-lib) requires, so the requirement // graph is derived to a fixed point instead of one hand-tuned level deep. - const requiredLibFiles = new Set(); - 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 = 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 }); - // 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 - // require() throws before their own try/catch, wedging every session — - // and the install would still exit 0, so nobody would know until a user - // hit it. A missing helper source is a packaging bug; surface it. - throw new Error( - `hooks/lib/${libFile} is required by a staged Cursor hook but is missing from ${srcLibDir}. ` - + 'Installing would ship a hook that throws MODULE_NOT_FOUND at load.', - ); - } - 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)); - } - } + stageTransitiveHookLibs({ + seedSources: [...installedScripts].map((script) => fs.readFileSync(path.join(hooksDir, script), 'utf8')), + srcLibDir: path.join(srcHooksDir, 'lib'), + destLibDir: path.join(hooksDir, 'lib'), + runtimeLabel: 'Cursor', + transform: (content) => content.replace(/gsd:/gi, 'gsd-'), + }); // #2717: write the CommonJS marker into hooks/ alongside the staged .js // scripts. Cursor sets skipSharedHooksInstall, so it never reaches @@ -1820,6 +1915,27 @@ function writeWindsurfHooksJson(targetDir: string, src: string, opts?: WriteWind } } + // Stage the hooks/lib/ helpers these scripts require (#4087 review). Windsurf + // sets hostBehaviors.skipSharedHooksInstall, so like Cursor it never reaches + // installSharedHooksBundle — the only other stager of hooks/lib — and it was + // staging neither. Both Cascade guards require helpers at module load: + // gsd-windsurf-pre-write.js requires ./lib/hook-exit.js and ./lib/git-probe.js, + // gsd-windsurf-pre-command.js requires ./lib/hook-exit.js. Measured against a + // real `--windsurf --global` install before this call existed: the installer + // exited 0, hooks/ held only the two scripts, and running either one exited 1 + // with "Cannot find module './lib/hook-exit.js'" — the same failure #4087 + // reports for Codex, on every pre_write_code / pre_run_command event. + // + // The transform matches the one applied to the scripts above: a helper must be + // rewritten the same way as its caller or the two disagree on the spelling. + stageTransitiveHookLibs({ + seedSources: [...installedScripts].map((script) => fs.readFileSync(path.join(hooksDir, script), 'utf8')), + srcLibDir: path.join(srcHooksDir, 'lib'), + destLibDir: path.join(hooksDir, 'lib'), + runtimeLabel: 'Windsurf', + transform: (content) => content.replace(/gsd:/gi, 'gsd-'), + }); + // #2717: write the CommonJS marker into hooks/ alongside the staged .js // scripts. Windsurf sets skipSharedHooksInstall, so it never reaches // installSharedHooksBundle (the only other writer of this marker); without @@ -2937,6 +3053,7 @@ export = { KIMI_HOOKS_TOML_MARKER_END, // Shared + stageTransitiveHookLibs, buildHookCommand, applySettingsJsonHooks, referencesHook, diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 33aa642b4..e3d9c6bbb 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -433,6 +433,9 @@ "hooks/gsd-check-update-worker.js", "hooks/gsd-check-update.js", "hooks/gsd-context-monitor.js", + "hooks/lib/cli-exit.js", + "hooks/lib/exit-code-registry.js", + "hooks/lib/hook-exit.js", "hooks/managed-hooks-registry.cjs", "hooks/package.json", "scripts/changeset/README.md", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index df74944a9..505cbade9 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -396,6 +396,10 @@ "gsd-core/workflows/verify-work/steps/mvp-uat-framing.md", "hooks/gsd-windsurf-pre-command.js", "hooks/gsd-windsurf-pre-write.js", + "hooks/lib/cli-exit.js", + "hooks/lib/exit-code-registry.js", + "hooks/lib/git-probe.js", + "hooks/lib/hook-exit.js", "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index 086c81966..5e2bbf082 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -2939,6 +2939,277 @@ describe('#1834: installer deploys .sh hooks alongside .js hooks', () => { }); } +// ─── #4087 / #4098: the Codex hook bundle must ship the helpers it requires ─── +// +// CODEX_HOOKS_TO_COPY is a flat, hand-maintained filename allowlist that never +// recursed, and Codex is excluded from installSharedHooksBundle (the path that +// stages hooks/lib/ for full-bundle runtimes). Excluding hooks/lib/ was a +// correct, scoped decision for #3579 — until #3911 (2ea5efc15) gave +// gsd-context-monitor.js a real `require('./lib/hook-exit.js')`. From then on a +// fresh --codex install staged the hook without its helper, and the hook died +// with MODULE_NOT_FOUND at module load — before its own try/catch — on every +// event Codex registers it for. The install still exited 0, so nothing surfaced +// it but the user's own broken session. +// +// These rows drive the REAL installer into a sandboxed config dir and then +// EXECUTE the installed hook. Asserting the files exist is not enough: the +// failure is at load, and a require chain one level deeper than the assertion +// looks identical to success on a file listing. +describe('#4087 regression: Codex install stages the hook helpers its hooks require', () => { + // These rows spawn a REAL install. The host suite sets GSD_TEST_MODE=1 at + // collection time, which the child inherits and which gates bin/install.js's + // whole main() block — the install then writes nothing at all and every + // assertion below fails on an absent hooks/ dir rather than on the defect. + // Same clear-and-restore the folded #1834 block uses for the same reason. + const { before: __gtmBefore, after: __gtmAfter } = require('node:test'); + let __savedGsdTestMode; + __gtmBefore(() => { __savedGsdTestMode = process.env.GSD_TEST_MODE; delete process.env.GSD_TEST_MODE; }); + __gtmAfter(() => { if (__savedGsdTestMode === undefined) delete process.env.GSD_TEST_MODE; else process.env.GSD_TEST_MODE = __savedGsdTestMode; }); + + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir('gsd-install-4087-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function installCodex(configDir) { + // HOME/USERPROFILE must be sandboxed for the CHILD, not just --config-dir. + // Codex's "skills" kind declares a global `home` override, so it resolves + // from os.homedir() rather than the configDir — and install/uninstall PRUNE + // GSD entries there. Without this the #3712 real-home guard refuses the call + // outright (correctly: it would otherwise write into and prune the + // developer's real ~/.agents/skills). sandboxHome() from helpers covers + // IN-PROCESS calls; this install is spawned, so the sandbox goes in the + // child's env. + throwIfFailed( + runNode([INSTALL_SCRIPT, '--codex', '--global', '--yes', '--no-sdk', '--config-dir', configDir], { + timeoutMs: 120000, + env: { ...process.env, HOME: configDir, USERPROFILE: configDir }, + }), + `node ${INSTALL_SCRIPT} --codex --global --config-dir ${configDir}`, + ); + return path.join(configDir, 'hooks'); + } + + test('the installed context-monitor hook LOADS AND RUNS, not merely exists', () => { + const hooksDir = installCodex(tmpDir); + const hook = path.join(hooksDir, 'gsd-context-monitor.js'); + assert.ok(fs.existsSync(hook), 'precondition: the hook itself must be staged'); + + // The actual defect. Before the fix this exited 1 with + // "Cannot find module './lib/hook-exit.js'". + // `exitCode`, not `status`: the process seam returns its own shape + // ({outcome, exitCode, stdout, stderr, ...}) and `status` reads undefined — + // which would compare unequal to 0 and pass this row for the wrong reason + // if the polarity were ever flipped. + const result = runNode([hook], { timeoutMs: 30000, input: '{}', env: { ...process.env } }); + assert.strictEqual( + result.outcome, 'exited', + `the hook must run to completion, not time out or be killed. outcome=${result.outcome}`, + ); + assert.strictEqual( + result.exitCode, 0, + 'the installed Codex hook must load and exit 0 — a MODULE_NOT_FOUND at load fires on every ' + + `registered event and is invisible to the installer's own exit code. stderr: ${result.stderr}`, + ); + assert.doesNotMatch( + String(result.stderr || ''), /MODULE_NOT_FOUND|Cannot find module/, + 'no missing-module error may reach stderr', + ); + }); + + // AC4: this is the row that stops the bug recurring. It derives the + // requirement graph from the SHIPPED files rather than restating today's three + // helpers, so a Codex-bundled hook that grows a new lib dependency fails here + // instead of in a user's session. + test('every helper required by a staged Codex hook — transitively — is staged', () => { + const hooksDir = installCodex(tmpDir); + const libDir = path.join(hooksDir, 'lib'); + + // Seed from hook scripts: only the explicit './lib/X' spelling is a lib + // requirement. A bare './X' from a hook script is a sibling in hooks/ + // (gsd-check-update-worker.js requires './managed-hooks-registry.cjs'), + // which is NOT under lib/ — conflating the two demands the wrong file. + const seedRe = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; + // From inside lib/, a sibling is already local, so './X' IS a lib require. + const libRe = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; + + const required = new Set(); + const scan = (source, re) => { + re.lastIndex = 0; + let m; + while ((m = re.exec(source)) !== null) required.add(m[1]); + }; + + for (const entry of fs.readdirSync(hooksDir)) { + const full = path.join(hooksDir, entry); + if (!fs.statSync(full).isFile()) continue; + if (!/\.(js|cjs)$/.test(entry)) continue; + scan(fs.readFileSync(full, 'utf8'), seedRe); + } + assert.ok( + required.size > 0, + 'precondition: at least one staged Codex hook must require a ./lib/ helper — if this ever ' + + 'goes to zero the bundle changed and this row silently stops testing anything', + ); + + // Walk to a fixed point, exactly as the installer must. + const checked = new Set(); + let next = [...required].find((f) => !checked.has(f)); + while (next !== undefined) { + checked.add(next); + const staged = path.join(libDir, next); + assert.ok( + fs.existsSync(staged), + `hooks/lib/${next} is required (directly or transitively) by a staged Codex hook but was ` + + 'not installed. A Codex-bundled hook gained a helper the installer does not stage — the ' + + 'hook will throw MODULE_NOT_FOUND at load on every event (#4087, #4098).', + ); + scan(fs.readFileSync(staged, 'utf8'), libRe); + next = [...required].find((f) => !checked.has(f)); + } + }); + + // The #3579 boundary this fix must preserve: derive what is needed, do not + // dump the whole helper directory into the reduced bundle. + // ─── the grammar itself, unit-level (review of #4087) ─── + // + // The install rows above prove today's three-helper chain. These pin the + // DISCOVERY GRAMMAR directly, which is what has to hold for the + // "future dependencies cannot silently regress" claim to mean anything. + describe('stageTransitiveHookLibs discovery grammar', () => { + const { stageTransitiveHookLibs } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + let dir; + + beforeEach(() => { dir = createTempDir('gsd-stage-libs-'); }); + afterEach(() => { cleanup(dir); }); + + function fixture(libFiles) { + const srcLibDir = path.join(dir, 'src', 'lib'); + const destLibDir = path.join(dir, 'dest', 'lib'); + fs.mkdirSync(srcLibDir, { recursive: true }); + for (const [name, content] of Object.entries(libFiles)) { + fs.writeFileSync(path.join(srcLibDir, name), content); + } + return { srcLibDir, destLibDir }; + } + + test('an EXTENSIONLESS require resolves — valid CommonJS, was failing the install', () => { + const { srcLibDir, destLibDir } = fixture({ 'helper.js': '// no requires\n' }); + const staged = stageTransitiveHookLibs({ + seedSources: ["require('./lib/helper')"], + srcLibDir, destLibDir, runtimeLabel: 'Test', + }); + assert.deepStrictEqual(staged, ['helper.js'], + "require('./lib/helper') must resolve to helper.js — matching only the extension-bearing " + + 'spelling resolved "helper" literally and failed the install on a legitimate require'); + assert.ok(fs.existsSync(path.join(destLibDir, 'helper.js')), + 'and it must land under its RESOLVED name, or Node cannot resolve it at the destination'); + }); + + test('a NESTED lib require is refused LOUDLY, never silently skipped', () => { + const { srcLibDir, destLibDir } = fixture({ 'helper.js': '' }); + assert.throws( + () => stageTransitiveHookLibs({ + seedSources: ["require('./lib/sub/helper.js')"], + srcLibDir, destLibDir, runtimeLabel: 'Test', + }), + /NESTED hooks\/lib path/, + 'hooks/lib/ is flat and the scan cannot express a nested path, so a nested require would ' + + 'stage nothing and ship a hook that dies at load — it must fail the install instead', + ); + }); + + test('prose that merely LOOKS like a require does not become a dependency', () => { + // hooks/lib/injection-patterns.js's own header documents this mechanism + // with the literal string require('./lib/...'), which captured `...` and + // sent the resolver hunting for hooks/lib/... — failing the install on a + // comment. Measured before the fix. + const { srcLibDir, destLibDir } = fixture({ 'helper.js': '' }); + const staged = stageTransitiveHookLibs({ + seedSources: ["/* the stager auto-discovers require('./lib/...') in staged scripts */"], + srcLibDir, destLibDir, runtimeLabel: 'Test', + }); + assert.deepStrictEqual(staged, [], + 'a capture with no alphanumeric character is prose, not a module name'); + }); + + test('a genuinely missing helper still fails loudly (the guard must not be softened)', () => { + const { srcLibDir, destLibDir } = fixture({ 'other.js': '' }); + assert.throws( + () => stageTransitiveHookLibs({ + seedSources: ["require('./lib/absent.js')"], + srcLibDir, destLibDir, runtimeLabel: 'Test', + }), + /absent\.js is required by a staged Test hook/, + 'the extension-fallback must not turn a real missing helper into a silent skip', + ); + }); + }); + + test('the staged helper set EQUALS the dependency closure — no more, no less', () => { + // Was: "fewer staged than available, and graphify absent". That passes while + // over-staging (an extra git-cmd.js keeps the count below the total and + // leaves graphify absent), so it did not prove its own title — the #3579 + // boundary is that helpers nothing requires must NOT ship (review of #4087). + // Now compared as SETS, with the difference asserted in both directions. + const hooksDir = installCodex(tmpDir); + const libDir = path.join(hooksDir, 'lib'); + const stagedLibs = fs.existsSync(libDir) ? fs.readdirSync(libDir).sort() : []; + assert.ok(stagedLibs.length > 0, 'precondition: some helper must have been staged'); + + // Derive the closure independently of the installer. + const seedRe = /require\(\s*['"]\.\/lib\/([A-Za-z0-9._-]+)['"]\s*\)/g; + const libRe = /require\(\s*['"]\.\/(?:lib\/)?([A-Za-z0-9._-]+)['"]\s*\)/g; + const srcLibDir = path.join(__dirname, '..', 'hooks', 'lib'); + const required = new Set(); + const scan = (source, re) => { + re.lastIndex = 0; + let m; + while ((m = re.exec(source)) !== null) { + if (/[A-Za-z0-9]/.test(m[1])) required.add(m[1]); + } + }; + const resolveName = (name) => [name, `${name}.js`, `${name}.cjs`] + .find((c) => fs.existsSync(path.join(srcLibDir, c))); + + for (const entry of fs.readdirSync(hooksDir)) { + const full = path.join(hooksDir, entry); + if (!fs.statSync(full).isFile() || !/\.(js|cjs)$/.test(entry)) continue; + scan(fs.readFileSync(full, 'utf8'), seedRe); + } + const closure = new Set(); + let next = [...required].find((f) => !closure.has(resolveName(f) || f)); + while (next !== undefined) { + const resolved = resolveName(next); + assert.ok(resolved, `hooks/lib/${next} is required but absent from source — packaging bug`); + closure.add(resolved); + scan(fs.readFileSync(path.join(srcLibDir, resolved), 'utf8'), libRe); + next = [...required].find((f) => !closure.has(resolveName(f) || f)); + } + + const expected = [...closure].sort(); + assert.deepStrictEqual( + stagedLibs, expected, + 'the staged helper set must equal the dependency closure exactly. Extra files violate the ' + + '#3579 boundary (helpers no Codex hook requires must not ship); missing files mean a hook ' + + `throws MODULE_NOT_FOUND at load. staged=${JSON.stringify(stagedLibs)} ` + + `expected=${JSON.stringify(expected)}`, + ); + // Non-vacuity: the source dir must hold MORE than the closure, or an + // over-staging bug would be undetectable by this comparison. + const available = fs.readdirSync(srcLibDir); + assert.ok( + available.length > expected.length, + `precondition: source must offer more helpers than the closure needs (available=${available.length}, closure=${expected.length})`, + ); + }); +}); + // ─── #3023: pi must not stage its shared-hooks bundle in pi's reserved hooks/ ── // // pi (pi.dev) renamed its `hooks/` directory to `extensions/` and now prints a @@ -2951,6 +3222,70 @@ describe('#1834: installer deploys .sh hooks alongside .js hooks', () => { // The expected directory name is asserted as a LITERAL on purpose: importing the // production constant would make the assertion re-derive the very value under // test, and it could then never catch that value changing. +describe('#4087 review: Windsurf install stages the hook helpers its hooks require', () => { + // Same defect class as the Codex rows above, one runtime over. Windsurf sets + // skipSharedHooksInstall, so it never reaches installSharedHooksBundle, and + // writeWindsurfHooksJson staged the two Cascade guards without the hooks/lib + // helpers both require at module load. Measured before the fix against a real + // `--windsurf --global` install: installer exit 0, hooks/ holding only the two + // scripts, and each one exiting 1 with "Cannot find module './lib/hook-exit.js'". + // + // tests/windsurf-hooks-bridge.test.cjs runs these guards from the SOURCE tree, + // where hooks/lib/ is a sibling and require() trivially resolves — the same + // "assert existence, never execute the installed copy" blind spot that let + // #4087 ship. These rows execute the INSTALLED copy. + const { before: __gtmBefore, after: __gtmAfter } = require('node:test'); + let __savedGsdTestMode; + __gtmBefore(() => { __savedGsdTestMode = process.env.GSD_TEST_MODE; delete process.env.GSD_TEST_MODE; }); + __gtmAfter(() => { if (__savedGsdTestMode === undefined) delete process.env.GSD_TEST_MODE; else process.env.GSD_TEST_MODE = __savedGsdTestMode; }); + + let tmpDir; + beforeEach(() => { tmpDir = createTempDir('gsd-install-4087-windsurf-'); }); + afterEach(() => { cleanup(tmpDir); }); + + function installWindsurf(configDir) { + // HOME/USERPROFILE sandboxed for the CHILD, for the same reason as installCodex. + throwIfFailed( + runNode([INSTALL_SCRIPT, '--windsurf', '--global', '--yes', '--config-dir', configDir], { + timeoutMs: 120000, + env: { ...process.env, HOME: configDir, USERPROFILE: configDir }, + }), + `node ${INSTALL_SCRIPT} --windsurf --global --config-dir ${configDir}`, + ); + return path.join(configDir, 'hooks'); + } + + test('both installed Windsurf guards LOAD AND RUN, not merely exist', () => { + const hooksDir = installWindsurf(tmpDir); + for (const script of ['gsd-windsurf-pre-write.js', 'gsd-windsurf-pre-command.js']) { + const hook = path.join(hooksDir, script); + assert.ok(fs.existsSync(hook), `precondition: ${script} must be staged`); + const result = runNode([hook], { timeoutMs: 30000, input: '{}', env: { ...process.env } }); + assert.strictEqual(result.outcome, 'exited', + `${script} must run to completion, not time out or be killed. outcome=${result.outcome}`); + assert.strictEqual(result.exitCode, 0, + `the installed ${script} must load and exit 0 — a MODULE_NOT_FOUND at load fires on every ` + + `pre_write_code / pre_run_command event and is invisible to the installer's exit code. stderr: ${result.stderr}`); + assert.doesNotMatch(String(result.stderr || ''), /MODULE_NOT_FOUND|Cannot find module/, + `no missing-module error may reach stderr for ${script}`); + } + }); + + test('the helpers the Windsurf guards require are staged, transitively', () => { + const hooksDir = installWindsurf(tmpDir); + const libDir = path.join(hooksDir, 'lib'); + assert.ok(fs.existsSync(libDir), 'hooks/lib/ must be staged for Windsurf'); + // Direct requires of the two guards, plus what hook-exit.js itself requires + // (cli-exit.js → exit-code-registry.js). The exact set is also pinned by + // tests/fixtures/install-tree/windsurf.json via the golden-install-tree test; + // this row states the reason each file must be present. + for (const helper of ['hook-exit.js', 'git-probe.js', 'cli-exit.js', 'exit-code-registry.js']) { + assert.ok(fs.existsSync(path.join(libDir, helper)), + `${helper} is on the require path of a staged Windsurf guard and must be staged`); + } + }); +}); + describe('#3023 pi shared-hooks bundle avoids the host-reserved hooks/ directory', () => { const PI_RESERVED_DIR = 'hooks'; const PI_BUNDLE_DIR = 'gsd-hooks';