diff --git a/.changeset/steady-quails-roar.md b/.changeset/steady-quails-roar.md new file mode 100644 index 000000000..2e67f39d8 --- /dev/null +++ b/.changeset/steady-quails-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2846 +--- +**Cursor, Windsurf, and Codex hooks no longer fail with `require is not defined` under an ESM config root** — GSD now writes the `{"type":"commonjs"}` marker into the hooks directory alongside the staged `.js` scripts for these three runtimes (it already did for every other runtime), so Node loads them as CommonJS regardless of the runtime config's `"type"`. (#2717) diff --git a/bin/install.js b/bin/install.js index 369c9e43c..d942836cb 100755 --- a/bin/install.js +++ b/bin/install.js @@ -8501,6 +8501,17 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { console.log(` ${green}✓${reset} Removed ${removedLibFiles} hooks/lib/ helper(s)`); } } + + // #2717: remove the CommonJS marker GSD wrote into hooks/ for runtimes that + // stage .js hooks via dedicated paths (cursor/windsurf/codex) — but ONLY if + // it still carries GSD's exact content (a user-authored package.json is + // never deleted). Safe no-op for runtimes whose marker lives at the config + // root (the shared-bundle path) or that never received one. + try { + if (hooksSurface.removeCommonJsMarkerIfGsdOwned(hooksDir)) { + console.log(` ${green}✓${reset} Removed GSD hooks/package.json (CommonJS marker)`); + } + } catch { /* best-effort */ } } // 4z. Remove the native plugin adapter (#1914, extended to Kilo by #2093). @@ -11646,6 +11657,17 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { } } console.log(` ${green}✓${reset} Installed hooks (Codex)`); + // #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 + // writes for the other runtimes. Without it, a ~/.codex/package.json + // declaring {"type":"module"} makes Node load gsd-check-update.js / + // gsd-context-monitor.js as ESM and their require() calls fail silently. + // Reuses the same helper the Cursor/Windsurf writers call so the marker + // content + user-file-preservation contract is identical everywhere. + if (hooksSurface.ensureCommonJsMarker(codexHooksDest)) { + console.log(` ${green}✓${reset} Wrote hooks/package.json (CommonJS mode)`); + } } // Add Codex hooks (SessionStart for update checking) — requires codex_hooks feature flag diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index 608a37047..b2a9b6cd1 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -199,6 +199,81 @@ function atomicWriteFileSync(target: string, data: string, options: fs.WriteFile } } +// --------------------------------------------------------------------------- +// CommonJS package.json marker for staged .js hook scripts (#2717) +// +// Node resolves the nearest package.json walking up from a .js file. When a +// runtime's config root (e.g. ~/.cursor, ~/.codeium/windsurf, ~/.codex) — or any +// parent — declares {"type":"module"}, Node loads GSD's staged CommonJS hook +// scripts as ESM and every require() fails with "require is not defined", +// silently disabling that runtime's lifecycle hooks. +// +// installSharedHooksBundle writes this marker for the 12 runtimes that go +// through the shared hooks bundle, but cursor/windsurf (skipSharedHooksInstall) +// and codex (the !isCodex gate) stage their .js hooks via the dedicated paths +// below and never reached it. These helpers decouple the marker write from the +// shared bundle so any code path that stages .js hooks can ensure the marker +// lands in the SAME directory as the scripts (#2717). +// +// The marker content is byte-identical to installSharedHooksBundle's +// (bin/install.js installSharedHooksBundle): {"type":"commonjs"}\n. +// --------------------------------------------------------------------------- + +/** The exact marker content GSD writes, matching installSharedHooksBundle. */ +const COMMONJS_MARKER_CONTENT = '{"type":"commonjs"}\n'; + +/** + * Ensure a `package.json` forcing CommonJS mode exists in `dir` (the directory + * holding GSD-staged `.js` hook scripts). Idempotent: a no-op if the marker is + * already present with GSD's content. Overwrites only when the file is absent + * or already carries GSD's exact marker — it never clobbers a distinct + * user-authored package.json (it leaves such a file in place; the user owns it). + * + * @param dir - absolute path to the directory holding the staged .js hooks + * @returns `true` if the marker is present after the call (written or already there) + */ +function ensureCommonJsMarker(dir: string): boolean { + const markerPath = path.join(dir, 'package.json'); + try { + if (fs.existsSync(markerPath)) { + const existing = fs.readFileSync(markerPath, 'utf8'); + // Already GSD's marker (tolerant of trailing-whitespace variants) — done. + if (existing.trim() === '{"type":"commonjs"}') return true; + // A distinct package.json the user owns — do NOT clobber. The hook will + // load as whatever type the user declared; that is the user's choice. + return false; + } + fs.writeFileSync(markerPath, COMMONJS_MARKER_CONTENT); + return true; + } catch { + // Best-effort: a marker write failure must not fail the whole install. + return false; + } +} + +/** + * Remove the CommonJS marker from `dir` on uninstall — but ONLY if it carries + * GSD's exact marker content. A user-authored package.json is never deleted. + * Mirrors the kimi uninstall guard in bin/install.js. + * + * @param dir - absolute path to the directory that held the staged .js hooks + * @returns `true` if a GSD-owned marker was removed + */ +function removeCommonJsMarkerIfGsdOwned(dir: string): boolean { + const markerPath = path.join(dir, 'package.json'); + try { + if (!fs.existsSync(markerPath)) return false; + const content = fs.readFileSync(markerPath, 'utf8').trim(); + if (content === '{"type":"commonjs"}') { + fs.unlinkSync(markerPath); + return true; + } + return false; + } catch { + return false; + } +} + // --------------------------------------------------------------------------- // parseTomlValue + findMultilineBasicStringClose // (needed by rewriteLegacyCodexHookBlock — pure TOML helpers, no state) @@ -1191,6 +1266,13 @@ function writeCursorHooksJson(targetDir: string, src: string, opts?: WriteCursor } } + // #2717: write the CommonJS marker into hooks/ alongside the staged .js + // scripts. Cursor sets skipSharedHooksInstall, so it never reaches + // installSharedHooksBundle (the only other writer of this marker); without + // it, a ~/.cursor/package.json declaring {"type":"module"} makes Node load + // these require()-using scripts as ESM and every Cursor hook fails silently. + ensureCommonJsMarker(hooksDir); + const hookOpts: BuildHookCommandOpts = { runtime: 'cursor', platform: opts.platform || process.platform }; const commands: Record = {}; for (const ev of events) { @@ -1224,6 +1306,11 @@ function removeCursorHooksJson(targetDir: string): { changed: boolean } { ); if (!hasAnyEvents) { fs.unlinkSync(hooksJsonPath); + // #2717: also remove the CommonJS marker GSD wrote into hooks/ — but + // only if it still carries GSD's exact content (a user-authored + // package.json is never deleted). Best-effort: a failure here must not + // mask the hooks.json removal above. + try { removeCommonJsMarkerIfGsdOwned(path.join(targetDir, 'hooks')); } catch { /* leave it */ } return { changed: true }; } } catch { /* best-effort: leave the file */ } @@ -1392,6 +1479,13 @@ function writeWindsurfHooksJson(targetDir: string, src: string, opts?: WriteWind } } + // #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 + // it, a config-root package.json declaring {"type":"module"} makes Node load + // these require()-using scripts as ESM and the Windsurf hooks fail silently. + ensureCommonJsMarker(hooksDir); + const hookOpts: BuildHookCommandOpts = { runtime: 'windsurf', platform: opts.platform || process.platform }; const commands: Record = {}; for (const ev of WINDSURF_HOOK_EVENTS) { @@ -1433,6 +1527,10 @@ function removeWindsurfHooksJson(targetDir: string): { changed: boolean } { ); if (!hasAnyEvents) { fs.unlinkSync(hooksJsonPath); + // #2717: also remove the CommonJS marker GSD wrote into hooks/ — but + // only if it still carries GSD's exact content (a user-authored + // package.json is never deleted). Best-effort. + try { removeCommonJsMarkerIfGsdOwned(path.join(targetDir, 'hooks')); } catch { /* leave it */ } return { changed: true }; } } catch { /* best-effort: leave the file */ } @@ -2352,6 +2450,8 @@ export = { GSD_WINDSURF_PRE_WRITE_HOOK_SCRIPT, GSD_WINDSURF_PRE_COMMAND_HOOK_SCRIPT, GSD_WINDSURF_HOOK_SCRIPTS, + ensureCommonJsMarker, + removeCommonJsMarkerIfGsdOwned, GSD_WINDSURF_HOOK_MARKER, // Copilot diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json deleted file mode 100644 index c3d67deff..000000000 --- a/tests/emitted-drift-ack.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "version": 1, - "paths": { - "review.md": { - "reason": "#2794 / ADR-2782 Phase 1. Three deliberate additions to gsd-core/workflows/review.md: (1) an explicit `` marker above each of the 11 invoke_reviewers legs, which is what makes a leg machine-identifiable for the DEFECT.GENERATIVE-FIX parity assertion — the legs are prose-labelled and five NON-lane bold labels in the same step share the bold-then-fence shape a heuristic matcher would key on, so inference was not an option; (2) the ADR-2782 D4 explicit-selection contract in the detect_clis prose, without which the selector correction is unobservable (review-reviewer-selection.cts has no production caller — the workflow narrates the policy and the reviewing agent executes it, so code and prose must move together); (3) the qwen leg's stderr sidecar, the last lane still discarding stderr to /dev/null (#2494/#2605 class). Growth is prose and comments only; no lane's observable command shape changed apart from the qwen redirect." - }, - "hooks/gsd-check-update-worker.js": { - "reason": "#2695: the Codex installer now delivers the complete four-file update-check hook set for every profile. gsd-check-update.js spawn()s gsd-check-update-worker.js, which require()s managed-hooks-registry.cjs — so all three are now emitted into Codex installs alongside the parent. The diff is in bin/install.js (allowlist + copy-loop + profile gate), not in the hook sources themselves, so the hooks-built attribution rule flags the emitted worker/registry paths. This ripple is the intended fix." - }, - "hooks/managed-hooks-registry.cjs": { - "reason": "#2695: the Codex installer now delivers the complete four-file update-check hook set for every profile. gsd-check-update.js spawn()s gsd-check-update-worker.js, which require()s managed-hooks-registry.cjs for MANAGED_HOOKS — so the registry is now emitted into Codex installs byte-for-byte. The diff is in bin/install.js (allowlist + raw-copy fallback + profile gate), not in the registry source, so the hooks-built attribution rule flags the emitted registry path. This ripple is the intended fix." - }, - "code-review.md": { - "reason": "#2694: three inline node -e frontmatter-boundary one-liners now normalize \\r\\n -> \\n before matching (content.replace(/\\r\\n/g,'\\n').match(...)), so CRLF-saved SUMMARY.md/REVIEW.md artifacts are no longer silently dropped. The growth is the inserted .replace(/\\r\\n/g,'\\n') at each of the 3 boundary sites; no observable command shape changed otherwise. This is the intended fix." - }, - "code-review-fix.md": { - "reason": "#2694: six inline node -e frontmatter-boundary one-liners now normalize \\r\\n -> \\n before matching (content.replace(/\\r\\n/g,'\\n').match(...)), so CRLF-saved REVIEW.md/REVIEW-FIX.md artifacts are no longer silently dropped (status/depth/files_reviewed_list extraction). The growth is the inserted .replace(/\\r\\n/g,'\\n') at each of the 6 boundary sites; no observable command shape changed otherwise. This is the intended fix." - } - } -} diff --git a/tests/fix-2717-cursor-windsurf-codex-commonjs-marker.test.cjs b/tests/fix-2717-cursor-windsurf-codex-commonjs-marker.test.cjs new file mode 100644 index 000000000..2721b127f --- /dev/null +++ b/tests/fix-2717-cursor-windsurf-codex-commonjs-marker.test.cjs @@ -0,0 +1,147 @@ +'use strict'; + +// Regression tests for #2717: cursor, windsurf, and codex stage `.js` hook +// scripts via dedicated paths that bypass installSharedHooksBundle (the only +// writer of the {"type":"commonjs"} marker). Under a config root declaring +// {"type":"module"}, Node loaded those scripts as ESM and every require() failed +// with "require is not defined", silently disabling the runtime's lifecycle +// hooks. +// +// These tests assert the invariant structurally: whenever a runtime stages one +// or more `.js` hooks into its GSD-owned hooks directory, a package.json forcing +// CommonJS mode exists in that SAME directory, and a require()-using hook +// actually loads under an ESM-typed parent. + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); +const { createTempDir, cleanup } = require('./helpers.cjs'); +const { runMinimalInstall } = require('./helpers/install-shared.cjs'); + +const COMMONJS_MARKER = '{"type":"commonjs"}\n'; + +// Runtimes that stage `.js` hooks via the dedicated cursor/windsurf/codex paths +// (skipSharedHooksInstall or the !isCodex gate) — the three #2717 covers. +const AFFECTED_RUNTIMES = [ + { runtime: 'cursor', sampleHook: 'gsd-cursor-session-start.js' }, + { runtime: 'windsurf', sampleHook: 'gsd-windsurf-pre-write.js' }, + { runtime: 'codex', sampleHook: 'gsd-check-update.js' }, +]; + +function readMarker(configDir) { + const p = path.join(configDir, 'hooks', 'package.json'); + if (!fs.existsSync(p)) return null; + return fs.readFileSync(p, 'utf8'); +} + +describe('#2717 CommonJS marker for staged .js hooks', () => { + for (const { runtime, sampleHook } of AFFECTED_RUNTIMES) { + test(`${runtime}: install writes {"type":"commonjs"} into hooks/ alongside the staged .js scripts`, (t) => { + const { configDir, root } = runMinimalInstall({ runtime, scope: 'global' }); + t.after(() => cleanup(root)); + + // The sample hook must actually be staged (sanity — confirms the install + // reached the dedicated .js-staging path for this runtime). + const hookPath = path.join(configDir, 'hooks', sampleHook); + assert.ok( + fs.existsSync(hookPath), + `${runtime} install must stage ${sampleHook} (got: ${fs.readdirSync(path.join(configDir, 'hooks')).join(',')})`, + ); + + // The marker must exist in the SAME directory, with GSD's exact content. + const marker = readMarker(configDir); + assert.strictEqual( + marker, + COMMONJS_MARKER, + `${runtime}: hooks/package.json must be exactly {"type":"commonjs"}\\n so Node loads the staged .js hooks as CommonJS even when the config root declares {"type":"module"}`, + ); + }); + } + + // Reproduces the exact failure mode in the issue: a config-root package.json + // declaring {"type":"module"}. Pre-fix, Node walked up from the .js hook, + // found this file, and loaded the hook as ESM → require() threw. Post-fix, + // the GSD-written hooks/package.json is nearer and wins. The hook may exit + // non-zero for benign reasons (no STATE.md, no config, etc.) — the ONLY + // failure we gate on is the ESM/require error on stderr. + function assertHookLoadsUnderEsmRoot(t, runtime, hookFile, stdinPayload) { + const { configDir, root } = runMinimalInstall({ runtime, scope: 'global' }); + t.after(() => cleanup(root)); + + // Plant the hostile ESM-typed package.json at the config root. + fs.writeFileSync(path.join(configDir, 'package.json'), '{"type":"module"}\n'); + + const hookPath = path.join(configDir, 'hooks', hookFile); + assert.ok(fs.existsSync(hookPath), `${runtime} hook ${hookFile} must be staged`); + + let stderr = ''; + try { + execFileSync(process.execPath, [hookPath], { + cwd: root, + input: stdinPayload, + encoding: 'utf8', + timeout: 20000, + stdio: ['pipe', 'pipe', 'pipe'], + }); + } catch (e) { + // Non-zero exit is allowed (benign); capture stderr for the ESM check. + stderr = String(e.stderr || ''); + } + assert.ok( + !/require is not defined/i.test(stderr), + `${runtime} hook ${hookFile} must load as CommonJS under an ESM-typed config root; got ESM error:\n${stderr}`, + ); + } + + test('cursor: a require()-using hook loads under an ESM-typed config root after install', (t) => { + assertHookLoadsUnderEsmRoot(t, 'cursor', 'gsd-cursor-session-start.js', JSON.stringify({ workspace_roots: [] })); + }); + + test('codex: a require()-using hook loads under an ESM-typed config root after install', (t) => { + // codex is the !isCodex-gated path most likely to regress (its marker write + // lives in bin/install.js, not the surface). gsd-check-update.js uses + // require() at module load, so it surfaces the ESM failure immediately. + assertHookLoadsUnderEsmRoot(t, 'codex', 'gsd-check-update.js', ''); + }); + + test('uninstall path: removeCommonJsMarkerIfGsdOwned removes only GSD-owned markers', (t) => { + // The uninstall cleanup uses removeCommonJsMarkerIfGsdOwned (exported from + // the runtime-hooks-surface). Assert its contract directly: it deletes a + // GSD-written marker but never a user-authored package.json. + const { + removeCommonJsMarkerIfGsdOwned, + ensureCommonJsMarker, + } = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + + // Case 1: GSD-owned marker is removed. + const dirA = createTempDir('gsd-2717-rmA-'); + t.after(() => cleanup(dirA)); + assert.ok(ensureCommonJsMarker(dirA), 'ensureCommonJsMarker writes the marker'); + const markerA = path.join(dirA, 'package.json'); + assert.strictEqual(fs.readFileSync(markerA, 'utf8'), COMMONJS_MARKER); + assert.ok(removeCommonJsMarkerIfGsdOwned(dirA), 'removes a GSD-owned marker'); + assert.ok(!fs.existsSync(markerA), 'GSD-owned marker is gone'); + + // Case 2: user-authored package.json is preserved. + const dirB = createTempDir('gsd-2717-keepB-'); + t.after(() => cleanup(dirB)); + const userContent = '{"name":"user-owned","type":"module"}\n'; + fs.writeFileSync(path.join(dirB, 'package.json'), userContent); + assert.ok(!removeCommonJsMarkerIfGsdOwned(dirB), 'does not remove a non-GSD package.json'); + assert.strictEqual(fs.readFileSync(path.join(dirB, 'package.json'), 'utf8'), userContent); + + // Case 3: no marker → no-op, no throw. + const dirC = createTempDir('gsd-2717-noopC-'); + t.after(() => cleanup(dirC)); + assert.ok(!removeCommonJsMarkerIfGsdOwned(dirC), 'no-op when no marker exists'); + + // Case 4: ensureCommonJsMarker is idempotent and does not clobber a user file. + const dirD = createTempDir('gsd-2717-idemD-'); + t.after(() => cleanup(dirD)); + fs.writeFileSync(path.join(dirD, 'package.json'), userContent); + assert.ok(!ensureCommonJsMarker(dirD), 'does not overwrite a user-authored package.json'); + assert.strictEqual(fs.readFileSync(path.join(dirD, 'package.json'), 'utf8'), userContent); + }); +}); diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 8772e77ed..ae09259dd 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -437,6 +437,7 @@ "hooks/gsd-check-update.js", "hooks/gsd-context-monitor.js", "hooks/managed-hooks-registry.cjs", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index ada754e49..a8ffc685f 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -405,6 +405,7 @@ "hooks/gsd-cursor-subagent-start.js", "hooks/gsd-cursor-subagent-stop.js", "hooks/lib/cursor-workspace.js", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index 2dd72c19b..5b0f9e5cd 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -329,6 +329,7 @@ "gsd-core/workflows/verify-work.md", "hooks/gsd-windsurf-pre-command.js", "hooks/gsd-windsurf-pre-write.js", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/helpers/emitted-provenance.cjs b/tests/helpers/emitted-provenance.cjs index 077740808..0855de0fd 100644 --- a/tests/helpers/emitted-provenance.cjs +++ b/tests/helpers/emitted-provenance.cjs @@ -347,6 +347,16 @@ const PROVENANCE_RULES = [ // bytes; its CONTENT never does — see the `sources` comment below for why // that rules out attributing to `hooks/.js`. pattern: /^(?!gsd-session\.json$).+$/, + // `hooks/package.json` (the CommonJS marker) is ALSO code-derived, not built + // from a tracked hooks/package.json source: its bytes are a fixed literal + // emitted at install time by ensureCommonJsMarker (HOOKS_WINDOWS_SHIM_SRC, + // a.k.a. src/runtime-hooks-surface.cts) for cursor/windsurf, and by the codex + // copy block (bin/install.js) which calls that same exported helper (#2717). + // So — like the `.cmd` shim below — it routes `sources`/`transforms` to that + // source file rather than to a nonexistent `hooks/package.json`. The codex + // emission path is covered transitively: it requires + calls the helper whose + // content literal defines the marker bytes. + // // `.cmd` shim bytes are code-derived (a literal template + the install-time // interpreter/path tokens in HOOKS_WINDOWS_SHIM_SRC) — the wrapped `.js` // file's NAME flows in (as a hardcoded literal filename inside that same @@ -356,8 +366,8 @@ const PROVENANCE_RULES = [ // used elsewhere in this table (copilot-hook-registration, cline-rules- // code-derived, hermes-category-description). The redundancy between // `sources` and `transforms` here is harmless — the mis-attribution was not. - sources: (m) => [m[0].endsWith('.cmd') ? HOOKS_WINDOWS_SHIM_SRC : `hooks/${m[0]}`], - transforms: (m) => (m[0].endsWith('.cmd') ? [HOOKS_WINDOWS_SHIM_SRC] : []), + sources: (m) => [m[0].endsWith('.cmd') || m[0] === 'package.json' ? HOOKS_WINDOWS_SHIM_SRC : `hooks/${m[0]}`], + transforms: (m) => (m[0].endsWith('.cmd') || m[0] === 'package.json' ? [HOOKS_WINDOWS_SHIM_SRC] : []), }, { id: 'copilot-hook-registration',