diff --git a/.changeset/2544-shared-hooks-package-json-clobber.md b/.changeset/2544-shared-hooks-package-json-clobber.md new file mode 100644 index 000000000..33e23f000 --- /dev/null +++ b/.changeset/2544-shared-hooks-package-json-clobber.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2593 +--- +**Installing or updating GSD no longer destroys a user-authored `package.json` at the runtime config root** — the CommonJS marker (`{"type":"commonjs"}`) that pins GSD's staged `.js` scripts is now written into the directories GSD itself fills (`hooks/`, and `plugins/`/`extensions/` for the runtimes with a native plugin adapter) instead of over `/package.json`. Previously every install and every `/gsd-update` re-install overwrote that file unconditionally — no existence check, no merge, no backup — permanently destroying any `name`, `type`, `dependencies`, or `scripts` the user or host tool had put there. This hit 11 runtimes and was worst on OpenCode and Kilo, where the config-root `package.json` is the documented place to declare local-plugin npm dependencies. Install and uninstall now share one ownership predicate, so a `package.json` GSD did not write is never overwritten and never removed; uninstall still retires the marker left behind by earlier versions. (#2544) diff --git a/.gitignore b/.gitignore index aa5437924..9f9586510 100644 --- a/.gitignore +++ b/.gitignore @@ -67,6 +67,7 @@ build/ # by `npm run build:lib`). Source of truth is src/; these are emitted, never edited. # Published via prepublishOnly; built before test via pretest. Grows as modules migrate. /tsconfig.build.tsbuildinfo +/gsd-core/bin/lib/commonjs-marker.cjs /gsd-core/bin/lib/broken-windows.cjs /gsd-core/bin/lib/host-integration.cjs /gsd-core/bin/lib/host-integration-sdk.cjs @@ -159,6 +160,7 @@ build/ /gsd-core/bin/lib/installer-migrations/004-prune-stale-pristine-snapshots.cjs /gsd-core/bin/lib/installer-migrations/005-opencode-baseline-commands-dir.cjs /gsd-core/bin/lib/installer-migrations/006-pi-extension-cjs-to-js.cjs +/gsd-core/bin/lib/installer-migrations/007-retire-config-root-commonjs-marker.cjs /gsd-core/bin/lib/observability/logger.cjs /gsd-core/bin/lib/active-workstream-store.cjs /gsd-core/bin/lib/adr-parser.cjs diff --git a/CONTEXT.md b/CONTEXT.md index 43997714a..88b1f269e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -151,6 +151,9 @@ Host-integration hook (`hooks/gsd-statusline.js`) that renders the session statu ### Install Engine Module Module owning the layout-driven runtime-artifact install pipeline — `installRuntimeArtifacts`, `uninstallRuntimeArtifacts`, `installOpencodeFamilySkills`, and their cluster helpers (`_copyStaged`, `_snapshotDir`/`_restoreDir`, legacy-migration + GSD-entry pruning, user-artifact preserve/restore). Extracted from the 12k-line `bin/install.js` (ADR-1239 Phase B, #1679) so adapters import the engine instead of reaching into the installer. Commit-attribution resolution stays in `bin/install.js` and is injected via a `resolveAttribution` parameter (the engine takes no config I/O). Source: `src/install-engine.cts` -> `gsd-core/bin/lib/install-engine.cjs`. +### CommonJS Marker Module +Module owning the `{"type":"commonjs"}` module-type marker GSD writes beside its own staged `.js` files (#2544) — `markerPathFor`, `classifyMarker`, `ensureCommonJsMarker`, `removeCommonJsMarker`, and the `COMMONJS_MARKER` / `COMMONJS_MARKER_CONTENT` constants. Exists so two rules are enforced in exactly one place: **write only where GSD owns the contents** (`hooks/`, and the `nativePlugin.dir` for runtimes declaring one — never the shared runtime config root, which on OpenCode and Kilo is documented, user-writable territory for local-plugin npm dependencies), and **never overwrite a file GSD did not write**. Ownership is decided by exact content match, the same predicate the uninstall path always used; `classifyMarker` is the shared seam behind both the write and the remove path so install and uninstall cannot drift apart again. Fails **closed** throughout: `lstat` (not `existsSync`) so a dangling symlink is never classified `absent`; anything that is not a regular file is `foreign`; a present-but-unreadable file is `foreign`, never downgraded to the permissive answer; the write uses `flag:'wx'` so anything appearing between classify and write yields `preserved-foreign` rather than a follow-or-overwrite. `ensureCommonJsMarker` **never throws** — an unwritable directory returns `failed` and both call sites warn and continue, matching the best-effort posture of every other marker interaction. The stale pre-#2544 config-root marker is retired by `src/installer-migrations/007-retire-config-root-commonjs-marker.cts` (and, for kimi's out-of-configDir root, by `bin/install.js` directly). Source: `src/commonjs-marker.cts` -> `gsd-core/bin/lib/commonjs-marker.cjs`. Test anchor: `tests/commonjs-marker.test.cjs`. + ### Installer Migration Authoring Guard Module Module owning validation for Installer Migration Module records and planned actions. It enforces migration metadata, explicit install scopes, ownership evidence for destructive/config actions, and runtime contract citations for runtime config rewrites before a migration can enter planning or apply. diff --git a/bin/install.js b/bin/install.js index 99f729dc4..26b3a8940 100755 --- a/bin/install.js +++ b/bin/install.js @@ -49,6 +49,11 @@ const { createImperativeAdapter } = require('../gsd-core/bin/lib/adapter-imperat // workflow .md content at emit time, before any per-runtime rewrite runs. const { composeWorkflow } = require('../gsd-core/bin/lib/workflow-fragments.cjs'); const runtimeArtifactConversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); +// #2544: the CommonJS marker's single source of truth. classifyMarker() backs +// BOTH ensureCommonJsMarker() (install) and removeCommonJsMarker() (uninstall), +// so the write side can no longer clobber a package.json the remove side would +// correctly refuse to delete. +const { ensureCommonJsMarker, removeCommonJsMarker } = require('../gsd-core/bin/lib/commonjs-marker.cjs'); // Canonical set of hook files shipped to users. Imported here so writeManifest() // records exactly the same set that build-hooks.js copies to hooks/dist/, making // the manifest and the installed hooks/ dir structurally identical. Avoids the @@ -8097,23 +8102,24 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { } } + // #2544: the marker now lives inside kimi's hooks/ dir — remove it + // before the emptiness check below, or the dir would never prune. + if (removeCommonJsMarker(kimiHooksDir)) { + removedCount++; + console.log(` ${green}✓${reset} Removed GSD package.json from ${kimiHooksDir}`); + } + try { if (fs.readdirSync(kimiHooksDir).length === 0) fs.rmdirSync(kimiHooksDir); } catch (_) { /* not empty — leave it */ } } - const kimiPkgJsonPath = path.join(kimiHooksRoot, 'package.json'); - if (fs.existsSync(kimiPkgJsonPath)) { - try { - const content = fs.readFileSync(kimiPkgJsonPath, 'utf8').trim(); - if (content === '{"type":"commonjs"}') { - fs.unlinkSync(kimiPkgJsonPath); - removedCount++; - console.log(` ${green}✓${reset} Removed GSD package.json from ${kimiHooksRoot}`); - } - } catch (e) { - // Ignore read errors - } + // Retire the pre-#2544 marker at kimi's root (~/.kimi), where the bundle + // used to write it. Exact content match — a user's own package.json in + // kimi's native config home is never touched. + if (removeCommonJsMarker(kimiHooksRoot)) { + removedCount++; + console.log(` ${green}✓${reset} Removed GSD package.json from ${kimiHooksRoot} (pre-#2544 marker)`); } } @@ -8421,13 +8427,18 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { } } - // #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. + // Retire the CommonJS marker staged into hooks/. hooks/ is shared space and + // is deliberately never rmdir'd here, so the marker must be removed + // explicitly or it would be left behind. Removed ONLY when it still carries + // GSD's exact content — a user-authored package.json is never deleted. + // + // #2717 reaches the runtimes that stage .js hooks via dedicated paths + // (cursor/windsurf/codex); #2544 reaches the shared-bundle runtimes, whose + // marker this PR moves out of the config root and into hooks/. Both land in + // the same directory, so one guarded call covers both. try { if (hooksSurface.removeCommonJsMarkerIfGsdOwned(hooksDir)) { + removedCount++; console.log(` ${green}✓${reset} Removed GSD hooks/package.json (CommonJS marker)`); } } catch { /* best-effort */ } @@ -8442,12 +8453,38 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { if (_np) { const pluginsDir = path.join(targetDir, _np.dir); const pluginPath = path.join(pluginsDir, _np.file); + // Tracks whether GSD actually removed anything from pluginsDir. The rmdir + // below is gated on it: pruning a directory GSD never wrote to is the same + // "don't touch territory GSD didn't fill" violation this issue is about, + // just inverted — a user-created but empty plugin/ or extensions/ dir is + // theirs, and an uninstall that never removed anything has no business + // deleting it. + let removedFromPluginsDir = false; if (fs.existsSync(pluginPath)) { try { fs.unlinkSync(pluginPath); removedCount++; + removedFromPluginsDir = true; console.log(` ${green}✓${reset} Removed native plugin adapter (${runtime})`); } catch (_) { /* best-effort */ } + } + // #2544: the adapter's CommonJS marker sits beside it. Cleaned up OUTSIDE + // the adapter-exists guard above — a partial install (or a hand-deleted + // adapter) would otherwise strand GSD's marker forever and keep the dir + // from ever pruning. Conditioned on the adapter being GONE, though: if the + // unlink above failed, pulling the marker out from under a still-present + // CommonJS adapter would leave it unloadable. The exact content match + // still leaves any user-authored package.json in place. + if (!fs.existsSync(pluginPath) && removeCommonJsMarker(pluginsDir)) { + removedCount++; + removedFromPluginsDir = true; + console.log(` ${green}✓${reset} Removed GSD package.json from ${_np.dir}/`); + } + // Only prune a dir GSD emptied. Pre-fix this rmdir sat inside the + // adapter-exists guard, so it could never fire on a dir GSD had not + // written to; hoisting it out to catch the marker-only case must not + // silently widen it to "any empty plugin dir". + if (removedFromPluginsDir) { try { fs.rmdirSync(pluginsDir); } catch (_) { /* not empty — user plugins present */ } } } @@ -8508,19 +8545,14 @@ function uninstall(isGlobal, runtime = DEFAULT_RUNTIME) { } // 5. Remove GSD package.json (CommonJS mode marker) - const pkgJsonPath = path.join(targetDir, 'package.json'); - if (fs.existsSync(pkgJsonPath)) { - try { - const content = fs.readFileSync(pkgJsonPath, 'utf8').trim(); - // Only remove if it's our minimal CommonJS marker - if (content === '{"type":"commonjs"}') { - fs.unlinkSync(pkgJsonPath); - removedCount++; - console.log(` ${green}✓${reset} Removed GSD package.json`); - } - } catch (e) { - // Ignore read errors - } + // Since #2544 the marker is staged into hooks/ (and the nativePlugin dir, + // handled at 4z above) rather than at targetDir. The targetDir removal is + // retained to retire the marker written by pre-#2544 installs — same exact + // content match as before, so a user-authored package.json is still never + // touched. + if (removeCommonJsMarker(targetDir)) { + removedCount++; + console.log(` ${green}✓${reset} Removed GSD package.json (pre-#2544 config-root marker)`); } // 6. Clean up settings.json (remove GSD hooks and statusline) @@ -10984,14 +11016,16 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { // a safe no-op when the dir is already present. fs.mkdirSync(destRootDir, { recursive: true }); - // Write package.json to force CommonJS mode for GSD scripts - // Prevents "require is not defined" errors when project has "type": "module" - // Node.js walks up looking for package.json - this stops inheritance from project - const pkgJsonDest = path.join(destRootDir, 'package.json'); - fs.writeFileSync(pkgJsonDest, '{"type":"commonjs"}\n'); - console.log(` ${green}✓${reset} Wrote package.json (CommonJS mode)`); + // #2544: the CommonJS marker is NOT written here (destRootDir is the + // runtime's shared config root — user-writable territory on OpenCode and + // Kilo, where it is the documented place to declare local-plugin npm + // dependencies). It is written into hooks/ below, the directory GSD + // creates and fills with its own .js scripts, once that directory exists. let hooksOk = true; + // #2544: true once GSD has actually written into destRootDir/hooks/, which + // is what licenses the CommonJS marker below. + let stagedHooks = false; // Copy hooks from dist/ (bundled with dependencies) // Template paths for the target runtime (replaces '.claude' with correct config dir) @@ -11000,6 +11034,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { const hooksDest = path.join(destRootDir, 'hooks'); fs.mkdirSync(hooksDest, { recursive: true }); const hookEntries = fs.readdirSync(hooksSrc); + if (hookEntries.some((e) => fs.statSync(path.join(hooksSrc, e)).isFile())) stagedHooks = true; const configDirReplacement = getConfigDirFromHome(runtime, isGlobal); for (const entry of hookEntries) { const srcFile = path.join(hooksSrc, entry); @@ -11094,9 +11129,47 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { const hooksLibDest = path.join(destRootDir, 'hooks', 'lib'); fs.mkdirSync(hooksLibDest, { recursive: true }); copyLibDir(hooksLibSrc, hooksLibDest, GSD_HOOK_LIB_FILES); + if (GSD_HOOK_LIB_FILES.some((f) => fs.existsSync(path.join(hooksLibDest, f)))) stagedHooks = true; console.log(` ${green}✓${reset} Installed hooks/lib/ helpers (git-cmd, graphify-rebuild, ...)`); } + // #2544: pin the staged hook scripts to CommonJS from inside hooks/ — the + // directory GSD just created and filled — instead of from destRootDir. + // Scoping the marker to GSD's own directory keeps `require` working in + // hooks/*.js and hooks/lib/*.js under any ambient "type": "module", while + // leaving the shared config root untouched. + // + // Gated on `stagedHooks`, NOT on the directory merely existing: hooks/ is + // shared space, so an existence check would drop a GSD marker into a + // hooks/ directory the user created and GSD never wrote to — the same + // write-into-someone-else's-territory this issue is about. And never + // written over a package.json GSD does not own. + // + // ALSO gated on `hooksOk`: `stagedHooks` is computed from the SOURCE + // listing before the copy loop, so it stays true when the copies land but + // `verifyInstalled` then fails. Marking a hooks/ GSD did not successfully + // populate as CommonJS claims an ownership the install did not earn — the + // two flags answer different questions ("did we intend to fill it" vs "is + // it actually filled"), and the marker needs both. + const hooksMarkerDir = path.join(destRootDir, 'hooks'); + if (stagedHooks && hooksOk) { + switch (ensureCommonJsMarker(hooksMarkerDir)) { + case 'written': + console.log(` ${green}✓${reset} Wrote hooks/package.json (CommonJS mode)`); + break; + case 'preserved-foreign': + console.warn(` ${yellow}⚠${reset} Left existing hooks/package.json untouched (not GSD's marker) — GSD hooks may not resolve as CommonJS`); + break; + case 'failed': + // Best-effort: a read-only or full config dir must not abort the + // install with a raw stack trace. The hooks themselves are staged. + console.warn(` ${yellow}⚠${reset} Could not write hooks/package.json (CommonJS mode) — install continued; GSD hooks may not resolve as CommonJS`); + break; + default: + break; + } + } + return hooksOk; } @@ -11545,6 +11618,10 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { const codexHooksDest = path.join(targetDir, 'hooks'); fs.mkdirSync(codexHooksDest, { recursive: true }); const configDirReplacement = getConfigDirFromHome(runtime, isGlobal); + // #2544: track whether anything was actually staged. hooks/dist existing + // is not the same as an allowlisted file landing in it — see the marker + // gate below. + let codexStagedHooks = false; for (const entry of fs.readdirSync(codexHooksSrc)) { if (!CODEX_HOOKS_TO_COPY.includes(entry)) continue; const srcFile = path.join(codexHooksSrc, entry); @@ -11578,6 +11655,7 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { fs.copyFileSync(srcFile, destFile); try { fs.chmodSync(destFile, 0o755); } catch (e) { /* Windows */ } } + codexStagedHooks = true; } console.log(` ${green}✓${reset} Installed hooks (Codex)`); // #2717: write the CommonJS marker into hooks/ alongside the staged .js @@ -11588,7 +11666,12 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { // 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)) { + // + // #2544: gated on codexStagedHooks, mirroring installSharedHooksBundle's + // `stagedHooks`. The enclosing guard only proves hooks/dist EXISTS; if it + // holds none of CODEX_HOOKS_TO_COPY, this block mkdirs hooks/ and stages + // nothing, and an ungated marker would claim a directory GSD did not fill. + if (codexStagedHooks && hooksSurface.ensureCommonJsMarker(codexHooksDest)) { console.log(` ${green}✓${reset} Wrote hooks/package.json (CommonJS mode)`); } } @@ -11839,6 +11922,20 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { if (!installSharedHooksBundle(kimiHooksRoot)) { console.warn(` ${yellow}⚠${reset} Kimi hook bundle did not verify at ${path.join(kimiHooksRoot, 'hooks')} — GSD lifecycle hooks may be incomplete`); } + // #2544: retire the pre-fix marker at kimi's root. installSharedHooksBundle + // used to write {"type":"commonjs"} at destRootDir itself; it now writes it + // under destRootDir/hooks/, so on an upgrade the old root file is stale and + // would keep ~/.kimi pinned to CommonJS. + // + // Done HERE rather than in installer-migration 007 (which retires the same + // stale marker for every other runtime) because kimi's hook root is + // ~/.kimi — resolved by resolveKimiHooksTomlDir, OUTSIDE kimi's configDir. + // Migration relPaths are structurally confined to configDir, so the + // framework cannot address this path at all. Same exact-content predicate + // either way, so a user-authored ~/.kimi/package.json is never touched. + if (removeCommonJsMarker(kimiHooksRoot)) { + console.log(` ${green}✓${reset} Removed stale package.json from ${kimiHooksRoot} (pre-#2544 marker)`); + } const kimiHookOpts = { portableHooks: hasPortableHooks, runtime }; const kimiHooksTomlPath = path.join(kimiHooksRoot, 'config.toml'); const kimiHooksResult = writeKimiHooksToml(kimiHooksTomlPath, kimiHooksRoot, { hookOpts: kimiHookOpts }); diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 586f9cf22..31e47af22 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -341,6 +341,7 @@ "command-roster.cjs", "command-routing-hub.cjs", "commands.cjs", + "commonjs-marker.cjs", "config-loader.cjs", "config-schema.cjs", "config-types.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 25e81d891..1b4d0811d 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -437,6 +437,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `clusters.cjs` | Skill cluster definitions for the runtime surface module (ADR-0011 Phase 2) | | `code-review-flags.cjs` | Typed flag parser for `/gsd:code-review`; exports `parseCodeReviewFlags(argv)` (→ `{ fix, all, auto, depth, files }`) and `resolveCodeReviewWorkflow(flags)` (→ `'code-review.md' \| 'code-review-fix.md'`); canonical dispatch seam for `--fix`/`--all`/`--auto` routing | | `command-aliases.cjs` | Alias/subcommand metadata for manifest-backed family routers | +| `commonjs-marker.cjs` | Ownership-guarded `{"type":"commonjs"}` marker used to pin GSD's staged `.js` scripts to CommonJS; exports `classifyMarker` (absent/gsd-owned/foreign, fail-closed), `ensureCommonJsMarker`, and `removeCommonJsMarker` so install and uninstall share one predicate and never touch a user-authored `package.json` (#2544) | | `command-arg-projection.cjs` | Typed flag and positional argument projection helpers shared across command-family routers | | `command-roster.cjs` | Read-only discovery of canonical `commands/gsd/*.md` command stems for runtime artifact conversion and namespace rewrites | | `command-routing-hub.cjs` | Pure-result dispatch hub that centralizes mode decision (SDK vs CJS), error taxonomy, and no-throw contract for all command-family routers (#3788) | diff --git a/docs/adr/457-generated-cjs-single-source.md b/docs/adr/457-generated-cjs-single-source.md index 04ea91a0c..fb28f9013 100644 --- a/docs/adr/457-generated-cjs-single-source.md +++ b/docs/adr/457-generated-cjs-single-source.md @@ -49,9 +49,14 @@ lint config that already advertises — in a comment and in a 12-file ignore lis This distinction is the crux of the decision, and the earlier draft erased it: - **Value baking (exists, forced).** `package-identity.cjs` must be generated - because the *installed* tree ships a synthetic `{"type":"commonjs"}` - `package.json` with no `.name`, so a runtime `require('package.json').name` - is `undefined` (bug #378). The values literally cannot be read at runtime; + because the *installed* tree carries no `package.json` with a `.name`. The + only ones GSD stages are synthetic `{"type":"commonjs"}` markers — and since + #2544 those sit inside the directories GSD owns (`hooks/`, and the native + plugin dir), not at the runtime config root — so a runtime + `require('package.json').name` is `undefined` where it resolves at all, and + a `MODULE_NOT_FOUND` where it does not (bug #378; see also the Codex case in + `src/runtime-artifact-conversion.cts`, whose root never carried one). The + values literally cannot be read at runtime; baking them at build time is the only option. **Deletion test:** remove the generator and the complexity reappears across every consumer. It is a deep seam and earns its keep. diff --git a/docs/how-to/install-on-your-runtime.md b/docs/how-to/install-on-your-runtime.md index 9cea2cc22..41c347703 100644 --- a/docs/how-to/install-on-your-runtime.md +++ b/docs/how-to/install-on-your-runtime.md @@ -125,6 +125,12 @@ The installer writes four surfaces under `~/.config/opencode/` (XDG) or `~/.open **GSD safety hooks on OpenCode.** OpenCode does not register lifecycle hooks the way Claude Code does (its `hooksSurface` is `none`), so GSD's prompt-injection guard, read-before-edit guard, injection scanner, and context monitor would otherwise be inert. The bundled plugin (`plugins/gsd-core.js`) closes that gap: OpenCode auto-discovers `plugins/*.{ts,js}` files under its config directory at startup and the adapter bridges OpenCode's event bus (`tool.execute.before`/`after`, `session.created`, `file.edited`) onto GSD's existing hook scripts, spawning them as subprocesses. No `opencode.json` entry is needed — the plugin is loaded by directory auto-discovery (the config `plugin` array is for npm packages only). A blocking hook aborts the tool call; an advisory hook surfaces its message without blocking. +**Your plugin directory is pinned to CommonJS (accepted trade-off, #2544).** GSD's adapter is a CommonJS `.js` file, and Node decides a `.js` file's module type by walking up for the nearest `package.json`. So the installer writes a minimal `{"type":"commonjs"}` marker into the plugin directory itself — `plugins/package.json` on OpenCode and Kilo, `extensions/package.json` on pi. It is written only when GSD actually stages its adapter there, it never overwrites a `package.json` GSD did not write, and uninstall removes only its own. + +The trade-off: that marker shadows your config root for **every** `.js` file in that directory, not just GSD's. If you author your own plugins as ESM `.js` and rely on a `"type": "module"` at the config root, they will stop resolving as ESM. This is deliberate — it is strictly narrower than the pre-#2544 behavior, which wrote the marker over `/package.json` itself and destroyed whatever was there — but it is a real constraint rather than a pure improvement, which is why it is stated here. + +**Mitigation:** author your own plugins as `.ts`. OpenCode and Kilo compile plugin TypeScript with Bun, and a `package.json` `type` field does not affect `.ts` resolution — so a `.ts` plugin is unaffected by the marker. Failing that, keep ESM plugins outside the auto-discovered directory and load them as npm packages via the config `plugin` array. + **Override the install directory:** ```bash diff --git a/docs/installer-migrations.md b/docs/installer-migrations.md index d15cb82e8..82a52da04 100644 --- a/docs/installer-migrations.md +++ b/docs/installer-migrations.md @@ -504,6 +504,7 @@ Each row corresponds to one migration record in `src/installer-migrations/`. | `2026-06-09-prune-stale-pristine-get-shit-done` | `004-prune-stale-pristine-snapshots.cts` | 1.4.3 | global, local | Yes | Removes stale `gsd-pristine/get-shit-done/` snapshot files left behind by migration 003, which caused false `verify-reapply-patches` failures (#934). | `2026-07-17-opencode-baseline-commands-dir` | `005-opencode-baseline-commands-dir.cts` | 1.7.0 | global, local | No | Baselines pre-existing files under OpenCode's `commands/` (plural) directory during the first-time scan. #2329 moved OpenCode command materialization to `commands/`, but 000's `RUNTIME_SURFACES.opencode` is a shipped, immutable body that still only names the legacy `command/` alias, so this fix-forward migration widens the scanned surface. OpenCode only; Kilo is unaffected. | | `2026-07-20-pi-extension-cjs-to-js` | `006-pi-extension-cjs-to-js.cts` | 1.7.1 | global, local | Yes | Removes the stale `extensions/gsd.cjs` left by pre-#2470 pi installs. pi's extension auto-discovery (`isExtensionFile()`) accepts only `.ts`/`.js`, so the `.cjs` file was never loaded and `/gsd` never registered; #2470 renamed the installed artifact to `extensions/gsd.js`, orphaning the old path. Locally modified copies are backed up rather than deleted; an unmanifested `gsd.cjs` is preserved as a user file. pi only. | +| `2026-07-28-retire-config-root-commonjs-marker` | `007-retire-config-root-commonjs-marker.cts` | 1.8.0 | global, local | Yes | Removes `/package.json` when it is exactly the `{"type":"commonjs"}` marker pre-#2544 installs wrote there. #2544 moved that marker into the directories GSD fills (`hooks/`, and the native plugin dir), so an upgraded install would otherwise keep both and stay pinned to CommonJS at a config root GSD no longer writes. Ownership is proven by exact content match, not the manifest (the marker was never manifest-recorded) — a `package.json` with any other content is left untouched, with no backup-and-remove branch. All runtimes; kimi's root marker lives outside `configDir` and is retired by the installer instead. | ## Prior Art diff --git a/eslint.config.mjs b/eslint.config.mjs index 0da9e9a21..1206fbb56 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -64,6 +64,7 @@ export default tseslint.config( 'gsd-core/bin/lib/host-integration-sdk.cjs', 'gsd-core/bin/lib/install-effort-resolver.cjs', 'gsd-core/bin/lib/install-engine.cjs', + 'gsd-core/bin/lib/commonjs-marker.cjs', 'gsd-core/bin/lib/capability-loader.cjs', 'gsd-core/bin/lib/capability-source.cjs', 'gsd-core/bin/lib/capability-ledger.cjs', @@ -135,6 +136,10 @@ export default tseslint.config( 'gsd-core/bin/lib/installer-migrations/003-rename-get-shit-done-to-gsd-core.cjs', 'gsd-core/bin/lib/installer-migrations/004-prune-stale-pristine-snapshots.cjs', 'gsd-core/bin/lib/installer-migrations/005-opencode-baseline-commands-dir.cjs', + // 007 is tsc output like its siblings, but unlike 006 it imports node + // builtins — so tsc emits its `__importDefault` helper, which uses `var` + // and trips no-var. ADR-457: the linted source is the .cts. + 'gsd-core/bin/lib/installer-migrations/007-retire-config-root-commonjs-marker.cjs', 'gsd-core/bin/lib/observability/logger.cjs', 'gsd-core/bin/lib/active-workstream-store.cjs', 'gsd-core/bin/lib/adr-parser.cjs', diff --git a/hooks/gsd-check-update-worker.js b/hooks/gsd-check-update-worker.js index 850b6872c..37653423e 100644 --- a/hooks/gsd-check-update-worker.js +++ b/hooks/gsd-check-update-worker.js @@ -15,9 +15,12 @@ const { isSemverNewer } = require('../gsd-core/bin/lib/semver-compare.cjs'); // Latest-version lookup is delegated to the single deterministic adapter // (#498). checkLatestVersion() owns the npm-view call, the timeout/semver // policy, and the package name — sourced from the baked Package Identity seam. -// The previous `require('../package.json').name` (#378) resolved to undefined -// in the installed tree (only a {"type":"commonjs"} marker ships), so the -// background check never reported updates. +// The previous `require('../package.json').name` (#378) never yielded a name in +// the installed tree — at the time it resolved to the synthetic +// {"type":"commonjs"} marker GSD wrote at the config root, which has no `.name`, +// so the background check never reported updates. Since #2544 GSD writes no +// marker there at all, so that require would now fail to resolve outright. +// Either way the name must come from the baked seam, never a walk-up. const { checkLatestVersion } = require('../gsd-core/bin/check-latest-version.cjs'); const { PACKAGE_NAME } = require('../gsd-core/bin/lib/package-identity.cjs'); // Authoritative list of managed hooks — shared with tests to retire source-grep diff --git a/scripts/generate-package-identity.cjs b/scripts/generate-package-identity.cjs index 1adbddd65..d24a98a66 100644 --- a/scripts/generate-package-identity.cjs +++ b/scripts/generate-package-identity.cjs @@ -7,8 +7,10 @@ * `deriveIdentity(pkg)` is the pure core: it turns a parsed package.json into * the coordinate record every consumer needs. The generated runtime module * `gsd-core/bin/lib/package-identity.cjs` bakes those values at build - * time, because the installed tree carries only a synthetic - * `{"type":"commonjs"}` package.json (no `.name`) — so a runtime + * time, because the installed tree carries no package.json with a `.name` — + * the only ones GSD stages are synthetic `{"type":"commonjs"}` markers, which + * since #2544 live inside GSD's own directories (`hooks/`, and the native + * plugin dir) rather than at the config root — so a runtime * `require('package.json').name` resolves to `undefined` (the #378 bug this * seam retires). Baking from package.json reconciles #378 (renames survive) * with #2992 (the value is never an LLM runtime choice). diff --git a/src/commonjs-marker.cts b/src/commonjs-marker.cts new file mode 100644 index 000000000..7551e2d75 --- /dev/null +++ b/src/commonjs-marker.cts @@ -0,0 +1,142 @@ +'use strict'; + +/** + * CommonJS module-type marker — single source of truth (#2544). + * + * GSD stages its own hook scripts and native plugin adapters as `.js` files. + * Node resolves a `.js` file's module type by walking up for the nearest + * `package.json`, so an ambient `"type": "module"` above the install location + * makes every one of those scripts fail with `require is not defined`. GSD + * pins them to CommonJS by writing a minimal `{"type":"commonjs"}` marker. + * + * Two rules govern that marker, and this module exists so both are enforced in + * exactly one place: + * + * 1. **Write only where GSD owns the contents.** The marker belongs in the + * directories GSD fills with its own `.js` files (`hooks/`, and the + * `nativePlugin.dir` for the runtimes that declare one) — never at the + * runtime's shared config root, which on OpenCode and Kilo is documented, + * user-writable territory for declaring local-plugin npm dependencies. + * + * 2. **Never overwrite a file GSD did not write.** Before #2544 the install + * path wrote the marker unconditionally while the uninstall path already + * compared content before unlinking. That asymmetry is the defect: the + * discipline existed in the codebase, it was simply not applied on the + * write side. `classifyMarker` is now the shared predicate behind both + * `ensureCommonJsMarker` and `removeCommonJsMarker`, so install and + * uninstall cannot drift apart again. + * + * Ownership is decided by exact content match against the marker GSD itself + * writes — the same test the uninstall path has always used. + */ + +import fs from 'node:fs'; +import path from 'node:path'; + +/** The exact marker content GSD writes (and the only content it will remove). */ +export const COMMONJS_MARKER = '{"type":"commonjs"}'; + +/** File bytes written to disk — the marker plus a trailing newline. */ +export const COMMONJS_MARKER_CONTENT = `${COMMONJS_MARKER}\n`; + +/** + * `absent` — no package.json here; GSD may create one. + * `gsd-owned` — content is exactly GSD's marker; GSD may rewrite or remove it. + * `foreign` — anything else, including a present-but-unreadable file. GSD + * must leave it strictly alone. + */ +export type MarkerOwnership = 'absent' | 'gsd-owned' | 'foreign'; + +/** + * Outcome of an `ensureCommonJsMarker` call, for caller-side reporting. + * + * `failed` is the best-effort outcome: the marker could not be written for an + * environmental reason (`EACCES` on a read-only `hooks/`, `EROFS`, `ENOSPC`). + * It is reported, never thrown — see `ensureCommonJsMarker`. + */ +export type MarkerWriteOutcome = 'written' | 'unchanged' | 'preserved-foreign' | 'failed'; + +/** The marker path for a directory. */ +export function markerPathFor(dir: string): string { + return path.join(dir, 'package.json'); +} + +/** + * Classify the package.json in `dir` by ownership. + * + * Fails CLOSED: a file that exists but cannot be read is reported `foreign`, + * never `absent`. Reporting it absent would license the overwrite this module + * exists to prevent. (Same posture as the unreadable-config branch in + * capability-command-router.cjs: present-but-unreadable never downgrades to + * the permissive answer.) + */ +export function classifyMarker(dir: string): MarkerOwnership { + const markerPath = markerPathFor(dir); + let stat: import('node:fs').Stats; + try { + // lstat, not existsSync: existsSync follows symlinks and reports `false` + // for a DANGLING one, which would classify the path `absent` and let the + // write below follow the link and land outside the directory GSD owns. + stat = fs.lstatSync(markerPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return 'absent'; + return 'foreign'; + } + // Anything that is not a regular file (symlink, directory, socket) is not + // something GSD wrote, so it is never ours to overwrite or remove. + if (!stat.isFile()) return 'foreign'; + try { + const content = fs.readFileSync(markerPath, 'utf8'); + return content.trim() === COMMONJS_MARKER ? 'gsd-owned' : 'foreign'; + } catch { + return 'foreign'; + } +} + +/** + * Write the CommonJS marker into `dir`, unless a file GSD does not own is + * already there. + * + * Creates `dir` when needed. Returns what happened so the caller can report + * it; a `preserved-foreign` result is not an error — it is the guard working. + * + * NEVER THROWS. Every other marker interaction in this module is best-effort — + * `removeCommonJsMarker` swallows unlink failures, `classifyMarker` swallows + * read failures — and the write path is the one most likely to fail on a + * locked-down config dir (`EACCES` on a read-only `hooks/`, `EROFS`, `ENOSPC`). + * Letting it throw made an unwritable marker abort the entire install with a + * raw stack trace, which is a strictly worse outcome than hooks that resolve as + * ESM: the caller can warn and continue, and does. Both the `mkdir` and the + * write are inside the guard — creating the directory is the same environmental + * hazard as writing into it. + */ +export function ensureCommonJsMarker(dir: string): MarkerWriteOutcome { + const ownership = classifyMarker(dir); + if (ownership === 'foreign') return 'preserved-foreign'; + if (ownership === 'gsd-owned') return 'unchanged'; + try { + fs.mkdirSync(dir, { recursive: true }); + // Exclusive create closes the gap between classifying and writing: if + // anything at all appeared at the path in between — including a symlink — + // this fails with EEXIST instead of following or overwriting it. + fs.writeFileSync(markerPathFor(dir), COMMONJS_MARKER_CONTENT, { flag: 'wx' }); + return 'written'; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'EEXIST') return 'preserved-foreign'; + return 'failed'; + } +} + +/** + * Remove the CommonJS marker from `dir` — only when the content is exactly + * the marker GSD writes. Returns true when a file was removed. + */ +export function removeCommonJsMarker(dir: string): boolean { + if (classifyMarker(dir) !== 'gsd-owned') return false; + try { + fs.unlinkSync(markerPathFor(dir)); + return true; + } catch { + return false; + } +} diff --git a/src/install-engine.cts b/src/install-engine.cts index 519646be8..c53b5b995 100644 --- a/src/install-engine.cts +++ b/src/install-engine.cts @@ -30,6 +30,7 @@ import installProfiles = require('./install-profiles.cjs'); import installerMigrations = require('./installer-migrations.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; import { isPathConfined } from './external-descriptor-trust.cjs'; +import { ensureCommonJsMarker } from './commonjs-marker.cjs'; const { processAttribution } = runtimeArtifactConversion; // resolveRuntimeArtifactLayout: accessed via module ref (not destructured) so @@ -1121,6 +1122,33 @@ function _installNativePluginIfDeclared( ); fs.mkdirSync(path.dirname(destPath), { recursive: true }); fs.copyFileSync(pluginSrc, destPath); + // #2544: the staged adapter is a `.js` file, so Node decides its module + // type by walking up for the nearest package.json. It used to find the + // marker the installer wrote at the config root — the write that + // clobbered user-authored files. Pin it from the plugin's own directory + // instead, leaving the config root alone. The marker cannot disturb + // plugin discovery: OpenCode auto-discovers `plugins/*.{ts,js}` and pi's + // isExtensionFile() accepts only `.ts`/`.js` (see installer-migration + // 006), so a package.json here is never treated as a plugin. Never + // written over a package.json GSD does not own — but when one is already + // there, say so: the adapter is CommonJS and will not load under a + // foreign `"type": "module"`, and a silent no-op would leave every guard + // the adapter spawns dead with no diagnostic (the #2305 failure shape). + const markerOutcome = ensureCommonJsMarker(path.dirname(destPath)); + if (markerOutcome === 'preserved-foreign') { + console.warn( + ` ⚠ ${np.dir}/package.json is not GSD's CommonJS marker — left untouched. ` + + `If it declares "type": "module", ${np.file} will not load.`, + ); + } else if (markerOutcome === 'failed') { + // Best-effort, never fatal: an unwritable plugin dir must not abort the + // install. Same warn-and-continue posture as the foreign-marker branch — + // the adapter is staged either way, it just may not resolve as CommonJS. + console.warn( + ` ⚠ Could not write ${np.dir}/package.json (CommonJS marker) — install continued. ` + + `If the config root declares "type": "module", ${np.file} will not load.`, + ); + } } } } diff --git a/src/installer-migrations/007-retire-config-root-commonjs-marker.cts b/src/installer-migrations/007-retire-config-root-commonjs-marker.cts new file mode 100644 index 000000000..802bb9fb2 --- /dev/null +++ b/src/installer-migrations/007-retire-config-root-commonjs-marker.cts @@ -0,0 +1,196 @@ +/** + * Installer migration: retire the config-root `{"type":"commonjs"}` marker that + * pre-#2544 installs wrote over `/package.json`. + * + * What old artifact is being retired? + * `package.json` at the runtime config root. Before #2544, + * `installSharedHooksBundle(destRootDir)` wrote `{"type":"commonjs"}` there + * unconditionally on every install and every re-install, to pin GSD's staged + * `.js` hook scripts to CommonJS via Node's ancestor walk. #2544 moved that + * marker into the directories GSD actually fills (`hooks/`, and the + * `nativePlugin.dir` for runtimes declaring one) and stopped writing the + * config root at all. Without this migration an upgrader keeps BOTH markers: + * the new one under `hooks/` and the stale one at the root, so the config + * root stays pinned to CommonJS and the PR's own claim — that GSD no longer + * writes the shared config root — is false for every install made since the + * marker was introduced, until the user uninstalls. + * + * How do we prove it is GSD-owned? + * By exact content match, NOT by the manifest. The config-root marker was + * never recorded in `gsd-file-manifest.json` — `writeManifest()` records + * `hooks/`, `agents/`, `commands/`, `scripts/`, and the native plugin, and + * has never had a `manifest.files['package.json']` entry — so + * `classifyArtifact('package.json')` answers `unknown` and the planner's + * own guard would downgrade a `remove-managed` to `preserve-user`. + * This migration therefore supplies the "purpose-built detector for an old + * GSD-owned shape" that `docs/installer-migrations.md#remove-managed` + * sanctions, and declares the resulting classification on the action: the + * file is removed only when its bytes are exactly the marker GSD writes + * (`{"type":"commonjs"}`, trailing whitespace tolerated). That is the same + * predicate `removeCommonJsMarker` has always used on the uninstall side, so + * install, uninstall, and migration cannot drift apart. + * + * What happens if the user modified it? + * Then it is not the marker, and this migration does not touch it. Any + * `package.json` carrying a `name`, `dependencies`, `scripts`, or any key + * beyond the single `type` — i.e. every file the #2544 defect destroyed — + * fails the exact-content test and is left exactly as found. There is + * deliberately no `backup-and-remove` branch: a modified file here is not a + * patched GSD artifact, it is somebody else's file. + * + * What happens if it is missing? + * No actions. Fresh post-#2544 installs never wrote it, already-migrated + * installs no longer have it, and the executor additionally journals a + * `missing` outcome if it disappears between plan and apply. Idempotent. + * + * What runtime and scope does it affect? + * Every runtime whose config root received the marker — i.e. every runtime + * not excluded from `installSharedHooksBundle(targetDir)` by + * `hostBehaviors.skipSharedHooksInstall` and not Codex: antigravity, + * augment, claude, claude-local, codebuddy, hermes, qwen, kilo, opencode, + * and pi. The `runtimes` field is OMITTED — the framework's "all runtimes" — + * rather than carrying that hand-list: a runtime that never received the + * marker simply has no file to match, so enumerating them would add a second + * place for the set to drift out of date without changing behavior. Note it + * must be omitted and not `[]`; see the field's own comment below. + * + * ONE DELIBERATE CARVE-OUT — kimi. Kimi's marker was written to its native + * hook root (`~/.kimi`, `resolveKimiHooksTomlDir`), which is NOT under + * kimi's `configDir` (its generic Agent-Skills root). Migration relPaths are + * structurally confined to `configDir` (`validateSafeRelPath` / + * `ensureInsideConfig`), so this framework cannot address that path at all. + * Kimi's stale root marker is retired by the installer instead, at the same + * call site that writes its replacement — see the `kimi-hooks-toml` branch + * in `bin/install.js`. Named here so the gap is not mistaken for an + * oversight. + * + * Is the action safe in non-interactive install? + * Yes. `remove-managed` is non-interactive and journaled, the executor takes + * a rollback snapshot before unlinking, and no branch of this migration can + * emit `prompt-user`. A file that is not byte-identical to GSD's marker + * produces no action at all. + * + * See docs/installer-migrations.md#shipped-migrations and #action-types. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import crypto from 'node:crypto'; + +type ArtifactClassification = string; + +interface ClassifiedArtifact { + classification: ArtifactClassification; + [key: string]: unknown; +} + +type ActionType = 'remove-managed'; + +interface MigrationAction { + type: ActionType; + relPath: string; + reason: string; + ownershipEvidence: string; + classification: string; + originalHash: string; + currentHash: string; +} + +interface MigrationPlanContext { + configDir: string; + classifyArtifact(relPath: string): ClassifiedArtifact; +} + +interface InstallerMigration { + id: string; + title: string; + description: string; + introducedIn: string; + /** + * OMITTED, not `[]`, to mean "every runtime". The authoring validator treats + * the field as optional but requires it to be NON-EMPTY when present + * (`validateStringArray`), while the runtime filter treats an empty array the + * same as an absent one. Only the validator actually runs on the record, so + * `runtimes: []` throws at plan time and the migration never executes. + */ + runtimes?: string[]; + scopes: string[]; + destructive: boolean; + plan: (ctx: MigrationPlanContext) => MigrationAction[]; +} + +/** The config-root path the pre-#2544 installer wrote, relative to configDir. */ +const STALE_ROOT_MARKER = 'package.json'; + +/** + * The exact marker content GSD wrote. Duplicated as a literal rather than + * imported from `src/commonjs-marker.cts` on purpose: a migration record is a + * frozen historical statement about what a PAST version installed, and it must + * keep matching those bytes even if the live module's constant is ever changed. + * Importing would silently re-point this detector at a future value. + */ +const LEGACY_MARKER_CONTENT = '{"type":"commonjs"}'; + +const OWNERSHIP_EVIDENCE = + 'file content is byte-identical to the {"type":"commonjs"} marker pre-#2544 installs ' + + 'wrote at the config root (installSharedHooksBundle); same exact-content predicate ' + + 'removeCommonJsMarker uses on uninstall. Never manifest-recorded, so this is the ' + + 'purpose-built detector permitted by docs/installer-migrations.md#remove-managed'; + +const REASON = + 'superseded by the hooks/ and plugin-dir markers (#2544); leaving it pins the shared ' + + 'config root to CommonJS and keeps GSD occupying a file it no longer writes'; + +const migration: InstallerMigration = { + id: '2026-07-28-retire-config-root-commonjs-marker', + title: 'Retire the pre-#2544 config-root CommonJS marker', + description: + 'Remove /package.json when it is exactly the {"type":"commonjs"} marker ' + + 'pre-#2544 installs wrote there, now superseded by markers scoped to the directories ' + + 'GSD owns. A package.json with any other content is left untouched.', + introducedIn: '1.8.0', + scopes: ['global', 'local'], + destructive: true, + plan: (ctx: MigrationPlanContext): MigrationAction[] => { + const markerPath = path.join(ctx.configDir, STALE_ROOT_MARKER); + + let stat: import('node:fs').Stats; + try { + // lstat, not existsSync: existsSync follows symlinks and reports false for + // a dangling one. A symlink here is not something GSD wrote, and removing + // it is never ours to do — mirrors classifyMarker's fail-closed posture. + stat = fs.lstatSync(markerPath); + } catch { + return []; + } + if (!stat.isFile()) return []; + + let content: string; + try { + content = fs.readFileSync(markerPath, 'utf8'); + } catch { + // Present but unreadable never downgrades to the permissive answer. + return []; + } + if (content.trim() !== LEGACY_MARKER_CONTENT) return []; + + const hash = crypto.createHash('sha256').update(content).digest('hex'); + return [ + { + type: 'remove-managed', + relPath: STALE_ROOT_MARKER, + reason: REASON, + ownershipEvidence: OWNERSHIP_EVIDENCE, + // Declared, not derived: classifyArtifact answers 'unknown' for this + // never-manifested path, and the planner downgrades a remove-managed on + // an 'unknown' classification to preserve-user. The exact-content match + // above IS the ownership proof, so the classification is stated here. + classification: 'managed-pristine', + originalHash: hash, + currentHash: hash, + }, + ]; + }, +}; + +export = migration; diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index cfb228eec..44de4d78d 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -30,11 +30,14 @@ import { posixNormalize } from './shell-command-projection.cjs'; // `require('../../../package.json')`. That require ran at module load on every // gsd-tools invocation (this module sits in the gsd-tools loader chain) and // threw `Cannot find module '../../../package.json'` on runtimes whose root has -// no package.json — notably Codex, where the installer omits the synthetic root -// package.json — taking the entire CLI down before it did anything. And even -// where it resolved (Claude's synthetic `{"type":"commonjs"}`), there is no -// `version` field, so the single consumer below already emitted -// `version: undefined`. Resolve lazily and defensively instead: +// no package.json — originally just Codex, where the installer never wrote the +// synthetic root package.json; since #2544 that is true of EVERY runtime, as +// GSD's markers moved into `hooks/` and the native plugin dir and the config +// root is no longer written at all — taking the entire CLI down before it did +// anything. And even where it used to resolve (the synthetic +// `{"type":"commonjs"}`), there is no `version` field, so the single consumer +// below already emitted `version: undefined`. Resolve lazily and defensively +// instead: // 1. Installed trees carry /gsd-core/VERSION (written by the installer); // this module lives at /gsd-core/bin/lib, so VERSION is two dirs up. // 2. The source / npm-package tree has no gsd-core/VERSION but carries a real diff --git a/src/runtime-hooks-surface.cts b/src/runtime-hooks-surface.cts index b2a9b6cd1..b7b5c9ad8 100644 --- a/src/runtime-hooks-surface.cts +++ b/src/runtime-hooks-surface.cts @@ -27,6 +27,13 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; +// #2544: the single source of truth for the CommonJS module-type marker. The +// two helpers below are thin boolean-returning shims over these — see the +// marker section for why this file no longer carries its own copy. +import { + ensureCommonJsMarker as ensureCommonJsMarkerOwned, + removeCommonJsMarker as removeCommonJsMarkerOwned, +} from './commonjs-marker.cjs'; import { CURSOR_HOOK_EVENTS, CURSOR_EVENT_SCRIPT_MAP, @@ -217,61 +224,53 @@ function atomicWriteFileSync(target: string, data: string, options: fs.WriteFile // // The marker content is byte-identical to installSharedHooksBundle's // (bin/install.js installSharedHooksBundle): {"type":"commonjs"}\n. +// +// #2544: the two helpers below no longer carry their own copy of the write and +// remove rules — they DELEGATE to src/commonjs-marker.cts, which #2544 makes the +// single place both rules are enforced. Keeping a second copy here was not +// merely redundant; the copies had drifted apart on exactly the two properties +// that matter: +// +// - ownership probe: `fs.existsSync` FOLLOWS symlinks and reports `false` for +// a DANGLING one, so a dangling `package.json` symlink classified as absent +// and the write below followed the link outside the directory GSD owns. +// `classifyMarker` uses `lstat` + `isFile()`, so a symlink or a directory at +// the marker path is classified `foreign` and left strictly alone. +// - create: a plain `writeFileSync` leaves the classify->write window open. +// `ensureCommonJsMarker` creates with `flag: 'wx'` (O_EXCL), so anything +// that appears at the path in between fails with EEXIST instead of being +// followed or overwritten. +// +// The exported signatures are unchanged (both still return a boolean), so every +// caller and the #2717 tests are unaffected. // --------------------------------------------------------------------------- -/** 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). + * already present with GSD's content. 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) + * @returns `true` if GSD's 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; - } + // 'written' | 'unchanged' -> the marker is ours and present. + // 'preserved-foreign' -> a file GSD does not own is there; left untouched. + // 'failed' -> environmental (EACCES/EROFS/ENOSPC); best-effort. + const outcome = ensureCommonJsMarkerOwned(dir); + return outcome === 'written' || outcome === 'unchanged'; } /** * 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; - } + return removeCommonJsMarkerOwned(dir); } // --------------------------------------------------------------------------- @@ -1271,7 +1270,15 @@ function writeCursorHooksJson(targetDir: string, src: string, opts?: WriteCursor // 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); + // + // #2544: gated on having actually staged a script, mirroring + // installSharedHooksBundle's `stagedHooks` gate. hooks/ is shared space, and + // this function mkdirs it unconditionally — so an ungated write drops a GSD + // marker into a directory GSD created but did not fill, which is the same + // write-into-someone-else's-territory this issue is about. + if (installedScripts.size > 0) { + ensureCommonJsMarker(hooksDir); + } const hookOpts: BuildHookCommandOpts = { runtime: 'cursor', platform: opts.platform || process.platform }; const commands: Record = {}; @@ -1484,7 +1491,12 @@ function writeWindsurfHooksJson(targetDir: string, src: string, opts?: WriteWind // 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); + // + // #2544: gated on having actually staged a script — see the identical gate in + // the Cursor writer above and `stagedHooks` in installSharedHooksBundle. + if (installedScripts.size > 0) { + ensureCommonJsMarker(hooksDir); + } const hookOpts: BuildHookCommandOpts = { runtime: 'windsurf', platform: opts.platform || process.platform }; const commands: Record = {}; diff --git a/tests/commonjs-marker.test.cjs b/tests/commonjs-marker.test.cjs new file mode 100644 index 000000000..80c514492 --- /dev/null +++ b/tests/commonjs-marker.test.cjs @@ -0,0 +1,659 @@ +'use strict'; + +/** + * CommonJS marker ownership — regression coverage for #2544. + * + * `installSharedHooksBundle` used to write `{"type":"commonjs"}` over + * `/package.json` unconditionally — no existence check, no merge, + * no backup. On OpenCode and Kilo that file is documented, user-writable + * territory (it is where local-plugin npm dependencies are declared), so every + * install and every `/gsd-update` destroyed the user's `name`, `type`, + * `dependencies`, and `scripts`. + * + * The uninstall path had always read the file first and unlinked it only on an + * exact content match. The defect was that asymmetry: the discipline existed, + * it just was not applied on the write side. + * + * The fix moves the marker into the directories GSD actually fills with its own + * `.js` files — `hooks/` and the `nativePlugin.dir` — and routes install and + * uninstall through one shared ownership predicate. These tests pin both + * halves: the config root is never written, and a user-authored package.json is + * never overwritten even where GSD does write. + * + * Coverage maps to the issue's acceptance criteria: + * AC1 — a user-authored config-root package.json survives a fresh install + * AC2 — it survives a second install (the `/gsd-update` re-install path) + * AC3 — GSD's staged .js files still resolve as CommonJS + * AC4 — uninstall removes only markers GSD wrote + */ + +const { test, describe, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const crypto = require('node:crypto'); +const { spawnSync } = require('node:child_process'); + +const { cleanup } = require('./helpers.cjs'); + +const { + COMMONJS_MARKER, + classifyMarker, + ensureCommonJsMarker, + removeCommonJsMarker, +} = require('../gsd-core/bin/lib/commonjs-marker.cjs'); + +const INSTALL_SCRIPT = path.join(__dirname, '..', 'bin', 'install.js'); +const BUILD_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); + +/** A realistic OpenCode-shape config-root package.json (the issue's repro). */ +const USER_PACKAGE_JSON = JSON.stringify( + { + name: 'my-opencode-config', + type: 'module', + dependencies: { shescape: '^2.1.0', zod: '^3.23.8' }, + scripts: { postinstall: 'echo user-owned' }, + }, + null, + 2, +) + '\n'; + +const sha256 = (buf) => crypto.createHash('sha256').update(buf).digest('hex'); + +function mkTmp(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +/** + * Run the real installer against a throwaway config root. + * + * HOME/USERPROFILE/CLAUDE_CONFIG_DIR are all redirected into the temp tree so + * the installer can never reach the developer's live profile — gsd-core's + * installer resolves through exactly those variables. + */ +function runInstall(root, runtime, extraArgs = []) { + const env = { ...process.env, HOME: root, USERPROFILE: root, CLAUDE_CONFIG_DIR: root }; + delete env.GSD_TEST_MODE; + const result = spawnSync( + process.execPath, + [INSTALL_SCRIPT, `--${runtime}`, '--global', '--config-dir', root, ...extraArgs], + { cwd: root, encoding: 'utf8', env }, + ); + assert.equal( + result.status, + 0, + `installer exited ${result.status}\nstdout: ${result.stdout}\nstderr: ${result.stderr}`, + ); + return result; +} + +describe('commonjs-marker: ownership predicate', () => { + test('classifies absent, GSD-owned, and foreign package.json files', (t) => { + const dir = mkTmp('cjs-marker-classify-'); + t.after(() => cleanup(dir)); + + assert.equal(classifyMarker(dir), 'absent'); + + fs.writeFileSync(path.join(dir, 'package.json'), `${COMMONJS_MARKER}\n`); + assert.equal(classifyMarker(dir), 'gsd-owned'); + + fs.writeFileSync(path.join(dir, 'package.json'), USER_PACKAGE_JSON); + assert.equal(classifyMarker(dir), 'foreign'); + }); + + test('ensureCommonJsMarker never overwrites a foreign package.json', (t) => { + const dir = mkTmp('cjs-marker-ensure-'); + t.after(() => cleanup(dir)); + const target = path.join(dir, 'package.json'); + + assert.equal(ensureCommonJsMarker(dir), 'written'); + assert.equal(fs.readFileSync(target, 'utf8').trim(), COMMONJS_MARKER); + + // Idempotent: a re-install must not churn the file. + assert.equal(ensureCommonJsMarker(dir), 'unchanged'); + + // Foreign content is preserved byte-for-byte. + fs.writeFileSync(target, USER_PACKAGE_JSON); + const before = sha256(fs.readFileSync(target)); + assert.equal(ensureCommonJsMarker(dir), 'preserved-foreign'); + assert.equal(sha256(fs.readFileSync(target)), before); + }); + + test('a symlinked package.json is foreign — never followed, never removed', (t) => { + const dir = mkTmp('cjs-marker-symlink-'); + t.after(() => cleanup(dir)); + const outside = path.join(dir, 'outside.json'); + const owned = path.join(dir, 'owned'); + fs.mkdirSync(owned); + const link = path.join(owned, 'package.json'); + + // A DANGLING symlink is the dangerous case: existsSync() reports false for + // it, so an existsSync-based guard would classify `absent` and then write + // straight through the link, landing outside the directory GSD owns. + fs.symlinkSync(outside, link); + assert.equal(classifyMarker(owned), 'foreign'); + assert.equal(ensureCommonJsMarker(owned), 'preserved-foreign'); + assert.ok(!fs.existsSync(outside), 'the write must not follow the symlink out of the directory'); + assert.equal(removeCommonJsMarker(owned), false, 'a symlink is never GSD-owned'); + assert.ok(fs.lstatSync(link).isSymbolicLink(), 'the symlink itself must survive'); + }); + + test('the #2717 hooks-surface helpers inherit the same symlink guard', (t) => { + const dir = mkTmp('cjs-marker-hookssurface-'); + t.after(() => cleanup(dir)); + + // #2717 added a SECOND copy of these helpers in runtime-hooks-surface.cts + // for the runtimes that stage .js hooks via dedicated paths + // (cursor/windsurf/codex). That copy probed with `fs.existsSync`, which + // follows symlinks and reports false for a DANGLING one — so it classified + // the link `absent` and wrote straight through it, landing outside the + // directory GSD owns. #2544 makes it delegate to commonjs-marker instead. + // + // This asserts the hardening reaches that path, not just the module: it is + // the only coverage that fails if the duplicate is ever reintroduced, since + // the two implementations agree on every non-adversarial input. + const hooksSurface = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + + const outside = path.join(dir, 'escaped.json'); + const owned = path.join(dir, 'hooks'); + fs.mkdirSync(owned); + fs.symlinkSync(outside, path.join(owned, 'package.json')); + + assert.equal( + hooksSurface.ensureCommonJsMarker(owned), + false, + 'a dangling symlink is not GSD-owned, so the marker must not be reported present', + ); + assert.ok( + !fs.existsSync(outside), + 'the write must not follow the symlink out of the hooks directory', + ); + assert.equal( + hooksSurface.removeCommonJsMarkerIfGsdOwned(owned), + false, + 'a symlink is never GSD-owned, so uninstall must not remove it', + ); + }); + + // The #2717 writers mkdir hooks/ unconditionally, then staged their scripts + // conditionally on the source existing — so with an empty hooks source they + // created a directory, filled it with nothing, and marked it as GSD's anyway. + // That is the same write-into-territory-GSD-did-not-fill this issue is about, + // and it is what `stagedHooks` guards on the shared-bundle path. These pin the + // matching gate on the two dedicated writers. + for (const rt of ['cursor', 'windsurf']) { + test(`${rt}: staging zero hook scripts leaves hooks/ marker-free`, (t) => { + const hooksSurface = require('../gsd-core/bin/lib/runtime-hooks-surface.cjs'); + const root = mkTmp(`gsd-2544-${rt}-nostage-`); + t.after(() => cleanup(root)); + + // A src tree whose hooks/ dir exists but holds none of the runtime's + // scripts — the writer stages nothing and must not claim the directory. + const emptySrc = path.join(root, 'src'); + fs.mkdirSync(path.join(emptySrc, 'hooks'), { recursive: true }); + const targetDir = path.join(root, 'target'); + fs.mkdirSync(targetDir, { recursive: true }); + + const write = rt === 'cursor' + ? hooksSurface.writeCursorHooksJson + : hooksSurface.writeWindsurfHooksJson; + write(targetDir, emptySrc); + + const hooksDir = path.join(targetDir, 'hooks'); + const staged = fs.existsSync(hooksDir) + ? fs.readdirSync(hooksDir).filter((f) => f.endsWith('.js')) + : []; + assert.equal(staged.length, 0, 'precondition: the writer staged no scripts'); + assert.ok( + !fs.existsSync(path.join(hooksDir, 'package.json')), + `${rt} must not mark a hooks/ directory it staged nothing into`, + ); + }); + } + + test('removeCommonJsMarker removes only GSD-owned markers', (t) => { + const dir = mkTmp('cjs-marker-remove-'); + t.after(() => cleanup(dir)); + const target = path.join(dir, 'package.json'); + + fs.writeFileSync(target, USER_PACKAGE_JSON); + assert.equal(removeCommonJsMarker(dir), false); + assert.ok(fs.existsSync(target), 'a foreign package.json must survive uninstall'); + + fs.writeFileSync(target, `${COMMONJS_MARKER}\n`); + assert.equal(removeCommonJsMarker(dir), true); + assert.ok(!fs.existsSync(target)); + }); +}); + +/** + * Fault injection (CONTRIBUTING.md:514-531, mandatory for install/uninstall + * flows). Every branch below is one whose doc comment claims it as the module's + * safety posture, and none of them is reachable from a happy-path test. + * + * These override fs methods and restore in `finally` rather than using + * `chmod 0o000`: chmod does not fault under root, so a permissions-based test + * passes vacuously with zero coverage in root Docker and CI containers. + */ +describe('commonjs-marker: fault injection', () => { + /** Swap one fs method for the duration of `fn`, restoring even on throw. */ + function withPatched(key, impl, fn) { + const original = fs[key]; + fs[key] = impl; + try { + return fn(); + } finally { + fs[key] = original; + } + } + + const errWith = (code) => Object.assign(new Error(`synthetic ${code}`), { code }); + + test('classifyMarker: a non-ENOENT lstat error fails CLOSED to foreign', (t) => { + const dir = mkTmp('cjs-marker-lstat-fault-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, 'package.json'), `${COMMONJS_MARKER}\n`); + + // EACCES on the stat itself. Reporting `absent` here would license the + // overwrite this module exists to prevent, so the answer must be `foreign`. + const result = withPatched('lstatSync', () => { throw errWith('EACCES'); }, + () => classifyMarker(dir)); + assert.equal(result, 'foreign'); + }); + + test('classifyMarker: ENOENT from lstat still reports absent', (t) => { + const dir = mkTmp('cjs-marker-enoent-'); + t.after(() => cleanup(dir)); + // The discriminating control for the test above: only ENOENT means absent. + const result = withPatched('lstatSync', () => { throw errWith('ENOENT'); }, + () => classifyMarker(dir)); + assert.equal(result, 'absent'); + }); + + test('classifyMarker: an unreadable file fails CLOSED to foreign', (t) => { + const dir = mkTmp('cjs-marker-read-fault-'); + t.after(() => cleanup(dir)); + fs.writeFileSync(path.join(dir, 'package.json'), `${COMMONJS_MARKER}\n`); + + // Present-but-unreadable never downgrades to the permissive answer — the + // file's bytes are exactly GSD's marker, and it must STILL classify foreign + // because we could not prove it. + const result = withPatched('readFileSync', () => { throw errWith('EACCES'); }, + () => classifyMarker(dir)); + assert.equal(result, 'foreign'); + }); + + test('classifyMarker: a DIRECTORY at the marker path is foreign', (t) => { + const dir = mkTmp('cjs-marker-dir-'); + t.after(() => cleanup(dir)); + // CONTRIBUTING.md:521 names this case explicitly. The symlink case is + // covered above with a real symlink; the directory case needs no fault + // injection at all, just a real directory. + fs.mkdirSync(path.join(dir, 'package.json')); + + assert.equal(classifyMarker(dir), 'foreign'); + assert.equal(ensureCommonJsMarker(dir), 'preserved-foreign'); + assert.equal(removeCommonJsMarker(dir), false); + assert.ok(fs.statSync(path.join(dir, 'package.json')).isDirectory(), + 'the directory must survive untouched'); + }); + + test('ensureCommonJsMarker: the TOCTOU EEXIST branch returns preserved-foreign', (t) => { + const dir = mkTmp('cjs-marker-toctou-'); + t.after(() => cleanup(dir)); + + // classifyMarker says `absent`, then something appears at the path before + // the write lands. `flag:'wx'` turns that race into EEXIST instead of a + // follow-or-overwrite — this branch is the entire reason for `wx`. + const result = withPatched('writeFileSync', () => { throw errWith('EEXIST'); }, + () => ensureCommonJsMarker(dir)); + assert.equal(result, 'preserved-foreign'); + }); + + test('ensureCommonJsMarker: a write error is reported, never thrown', (t) => { + const dir = mkTmp('cjs-marker-write-fault-'); + t.after(() => cleanup(dir)); + + // EACCES on a read-only hooks/, EROFS, ENOSPC. Every other marker + // interaction is best-effort; this one used to be fatal and abort the whole + // install with a raw stack trace. + for (const code of ['EACCES', 'EROFS', 'ENOSPC']) { + const result = withPatched('writeFileSync', () => { throw errWith(code); }, + () => ensureCommonJsMarker(dir)); + assert.equal(result, 'failed', `${code} must report failed, not throw`); + } + }); + + test('ensureCommonJsMarker: a mkdir error is reported, never thrown', (t) => { + const dir = mkTmp('cjs-marker-mkdir-fault-'); + t.after(() => cleanup(dir)); + + // Creating the directory is the same environmental hazard as writing into + // it, so it lives inside the same guard. This sat OUTSIDE the try until + // #2544 review round 2. + const result = withPatched('mkdirSync', () => { throw errWith('EROFS'); }, + () => ensureCommonJsMarker(path.join(dir, 'nested'))); + assert.equal(result, 'failed'); + }); + + test('removeCommonJsMarker: an unlink failure returns false, never throws', (t) => { + const dir = mkTmp('cjs-marker-unlink-fault-'); + t.after(() => cleanup(dir)); + const target = path.join(dir, 'package.json'); + fs.writeFileSync(target, `${COMMONJS_MARKER}\n`); + + const result = withPatched('unlinkSync', () => { throw errWith('EACCES'); }, + () => removeCommonJsMarker(dir)); + assert.equal(result, false); + assert.ok(fs.existsSync(target), 'the file is still there — the report must say so'); + }); +}); + +describe('#2544 regression: install must not clobber the config-root package.json', () => { + // hooks/dist is gitignored and built; scoped CI lanes do not run build:hooks, + // so build it idempotently before driving a real install. + before(() => { + const build = spawnSync(process.execPath, [BUILD_SCRIPT], { encoding: 'utf8' }); + assert.equal(build.status, 0, `build:hooks failed: ${build.stderr}`); + }); + + for (const runtime of ['opencode', 'claude']) { + test(`${runtime}: a user-authored package.json survives install and re-install`, (t) => { + const root = mkTmp(`gsd-2544-${runtime}-`); + t.after(() => cleanup(root)); + const userPkg = path.join(root, 'package.json'); + fs.writeFileSync(userPkg, USER_PACKAGE_JSON); + const before = sha256(fs.readFileSync(userPkg)); + + // AC1 — fresh install leaves it untouched. + runInstall(root, runtime); + assert.equal( + sha256(fs.readFileSync(userPkg)), + before, + 'fresh install must not modify the user-authored config-root package.json', + ); + + // AC2 — the /gsd-update re-install path leaves it untouched too. + runInstall(root, runtime); + assert.equal( + sha256(fs.readFileSync(userPkg)), + before, + 're-install must not modify the user-authored config-root package.json', + ); + + // The user's own keys are still readable and intact. + const parsed = JSON.parse(fs.readFileSync(userPkg, 'utf8')); + assert.equal(parsed.name, 'my-opencode-config'); + assert.equal(parsed.type, 'module'); + assert.deepEqual(parsed.dependencies, { shescape: '^2.1.0', zod: '^3.23.8' }); + assert.deepEqual(parsed.scripts, { postinstall: 'echo user-owned' }); + + // AC3 — GSD's own staged scripts still get a CommonJS marker, from the + // directory GSD owns, so `require` keeps working under "type": "module". + const hooksMarker = path.join(root, 'hooks', 'package.json'); + assert.ok(fs.existsSync(hooksMarker), 'hooks/package.json marker must be staged'); + assert.equal(JSON.parse(fs.readFileSync(hooksMarker, 'utf8')).type, 'commonjs'); + }); + } + + test('staged hook helpers still load as CommonJS under a "type": "module" config root', (t) => { + const root = mkTmp('gsd-2544-esm-'); + t.after(() => cleanup(root)); + // The config root declares ESM — the exact shape that breaks Node's + // walk-up resolution for GSD's staged .js files. + fs.writeFileSync(path.join(root, 'package.json'), USER_PACKAGE_JSON); + runInstall(root, 'opencode'); + + // Actually require a staged CommonJS helper. Without a marker inside + // hooks/, the walk-up lands on the user's "type": "module" and this throws + // ERR_REQUIRE_ESM / "require is not defined" — the regression AC3 forbids. + const target = path.join(root, 'hooks', 'lib', 'git-cmd.js'); + assert.ok(fs.existsSync(target), 'hooks/lib/git-cmd.js must be staged'); + const probe = spawnSync( + process.execPath, + ['-e', `const m = require(${JSON.stringify(target)}); if (typeof m.isGitSubcommand !== 'function') { throw new Error('unexpected exports'); } console.log('loaded');`], + { cwd: root, encoding: 'utf8' }, + ); + assert.equal( + probe.status, + 0, + `staged hook helper must load as CommonJS under an ESM config root\nstderr: ${probe.stderr}`, + ); + assert.match(probe.stdout, /loaded/); + }); + + test('opencode: the native plugin dir gets its own marker', (t) => { + const root = mkTmp('gsd-2544-plugin-'); + t.after(() => cleanup(root)); + runInstall(root, 'opencode'); + + // The adapter is staged as .js, so it needs a marker in its own directory + // now that the config root no longer carries one. A package.json here is + // inert to plugin discovery: OpenCode globs plugins/*.{ts,js}. + assert.ok(fs.existsSync(path.join(root, 'plugins', 'gsd-core.js'))); + const pluginMarker = path.join(root, 'plugins', 'package.json'); + assert.ok(fs.existsSync(pluginMarker), 'plugins/package.json marker must be staged'); + assert.equal(JSON.parse(fs.readFileSync(pluginMarker, 'utf8')).type, 'commonjs'); + }); + + test('install writes no package.json at the config root when none existed', (t) => { + const root = mkTmp('gsd-2544-noroot-'); + t.after(() => cleanup(root)); + runInstall(root, 'opencode'); + + assert.ok( + !fs.existsSync(path.join(root, 'package.json')), + 'GSD must not create a package.json in the runtime config root', + ); + }); + + test('uninstall removes GSD markers but preserves a user-authored one', (t) => { + const root = mkTmp('gsd-2544-uninstall-'); + t.after(() => cleanup(root)); + const userPkg = path.join(root, 'package.json'); + fs.writeFileSync(userPkg, USER_PACKAGE_JSON); + const before = sha256(fs.readFileSync(userPkg)); + + runInstall(root, 'opencode'); + runInstall(root, 'opencode', ['--uninstall']); + + // AC4 — GSD's own markers are gone; the user's file is untouched. + assert.ok(fs.existsSync(userPkg), 'uninstall must not remove a user-authored package.json'); + assert.equal(sha256(fs.readFileSync(userPkg)), before); + assert.ok( + !fs.existsSync(path.join(root, 'hooks', 'package.json')), + 'uninstall must remove the hooks/ marker it wrote', + ); + assert.ok( + !fs.existsSync(path.join(root, 'plugins', 'package.json')), + 'uninstall must remove the plugin-dir marker it wrote', + ); + }); + + test('uninstall does not prune a plugin dir GSD removed nothing from', (t) => { + const root = mkTmp('gsd-2544-rmdir-'); + t.after(() => cleanup(root)); + runInstall(root, 'opencode'); + + // Strip GSD's own artifacts by hand, leaving an EMPTY plugins/ directory + // that — from uninstall's point of view — GSD never filled. Hoisting the + // rmdir out of the adapter-exists guard (so the marker-only case could + // prune) must not widen it into deleting a user-created empty plugin dir: + // that is the same "don't touch territory GSD didn't fill" principle this + // issue is about, inverted. + const pluginsDir = path.join(root, 'plugins'); + fs.unlinkSync(path.join(pluginsDir, 'gsd-core.js')); + fs.unlinkSync(path.join(pluginsDir, 'package.json')); + assert.deepEqual(fs.readdirSync(pluginsDir), [], 'precondition: the dir is empty'); + + runInstall(root, 'opencode', ['--uninstall']); + + assert.ok( + fs.existsSync(pluginsDir), + 'an empty plugin dir GSD removed nothing from must survive uninstall', + ); + }); + + test('uninstall reclaims the plugin-dir marker even if the adapter is already gone', (t) => { + const root = mkTmp('gsd-2544-partial-'); + t.after(() => cleanup(root)); + runInstall(root, 'opencode'); + + // Model a partial install / hand-deleted adapter. The marker cleanup must + // not be gated on the adapter still being present, or it is stranded and + // the directory can never prune. + fs.unlinkSync(path.join(root, 'plugins', 'gsd-core.js')); + runInstall(root, 'opencode', ['--uninstall']); + + assert.ok( + !fs.existsSync(path.join(root, 'plugins', 'package.json')), + 'the plugin-dir marker must be reclaimed even without the adapter', + ); + }); + + test('a pre-existing hooks/ dir GSD never fills stays marker-free', (t) => { + const root = mkTmp('gsd-2544-stagedhooks-'); + t.after(() => cleanup(root)); + + // Scope, stated precisely: this pins the OUTCOME — a pre-existing, + // GSD-untouched hooks/ stays marker-free — for a runtime GSD stages no .js + // into. It does NOT exercise installSharedHooksBundle's `stagedHooks` gate: + // zcode declares skipSharedHooksInstall, so the outer guard in bin/install.js + // skips that helper entirely and the gate is never evaluated. For a runtime + // that DOES reach the bundle, `stagedHooks` is true whenever any hook source + // exists, so the gate is only distinguishable under fault injection. The two + // `staging zero hook scripts` tests above are the ones that pin a real + // staged-nothing gate, on the #2717 writers. + // + // ZCode, not Windsurf. Windsurf was the original choice because + // hostBehaviors.skipSharedHooksInstall kept it out of the shared bundle — + // but #2717 then began staging cursor/windsurf/codex .js hooks via dedicated + // paths and writing the marker beside them, so for those three GSD now DOES + // fill hooks/ and the marker is correct. ZCode is the durable choice: per + // #1821 it has hooksSurface:'none' AND no plugin surface to spawn hooks, so + // GSD stages no .js there by either route. (Measured on this tree: zcode + // stages 0 .js hooks and gets no marker; windsurf stages 2 and gets one.) + const userHooks = path.join(root, 'hooks'); + fs.mkdirSync(userHooks, { recursive: true }); + fs.writeFileSync(path.join(userHooks, 'my-hook.js'), '// user-authored\n'); + + runInstall(root, 'zcode'); + + assert.ok( + !fs.existsSync(path.join(userHooks, 'package.json')), + 'GSD must not mark a hooks/ directory it never staged into', + ); + assert.ok( + fs.existsSync(path.join(userHooks, 'my-hook.js')), + "the user's own hooks/ contents must be untouched", + ); + }); + + test('pi: the extensions/ marker is installed and reclaimed on uninstall', (t) => { + const root = mkTmp('gsd-2544-pi-'); + t.after(() => cleanup(root)); + + runInstall(root, 'pi'); + const marker = path.join(root, 'extensions', 'package.json'); + assert.ok(fs.existsSync(marker), 'pi extensions/package.json marker must be staged'); + assert.equal(JSON.parse(fs.readFileSync(marker, 'utf8')).type, 'commonjs'); + + runInstall(root, 'pi', ['--uninstall']); + assert.ok(!fs.existsSync(marker), 'uninstall must remove the extensions/ marker it wrote'); + }); + + test('pi: a user-authored extensions/package.json survives install and uninstall', (t) => { + const root = mkTmp('gsd-2544-pi-user-'); + t.after(() => cleanup(root)); + + const extDir = path.join(root, 'extensions'); + fs.mkdirSync(extDir, { recursive: true }); + const userPkg = path.join(extDir, 'package.json'); + fs.writeFileSync(userPkg, USER_PACKAGE_JSON); + const before = sha256(fs.readFileSync(userPkg)); + + runInstall(root, 'pi'); + assert.equal(sha256(fs.readFileSync(userPkg)), before, + 'install must not overwrite a user-authored extensions/package.json'); + + runInstall(root, 'pi', ['--uninstall']); + assert.ok(fs.existsSync(userPkg), 'uninstall must not remove it either'); + assert.equal(sha256(fs.readFileSync(userPkg)), before); + }); + + test('kimi: the marker lives under hooks/, and the legacy root marker is retired', (t) => { + const root = mkTmp('gsd-2544-kimi-'); + t.after(() => cleanup(root)); + + // HOME is redirected to `root`, so kimi's native hook root + // (resolveKimiHooksTomlDir → the `.kimi` dot-home) resolves inside the + // temp tree. That root is OUTSIDE kimi's configDir, which is why migration + // 007 cannot reach it and bin/install.js retires it directly. + const kimiRoot = path.join(root, '.kimi'); + + runInstall(root, 'kimi'); + assert.ok( + fs.existsSync(path.join(kimiRoot, 'hooks', 'package.json')), + 'kimi marker must be staged inside .kimi/hooks/', + ); + assert.ok( + !fs.existsSync(path.join(kimiRoot, 'package.json')), + 'kimi must not carry a marker at its native hook root', + ); + + // Model a pre-#2544 install, which left the marker at the .kimi root, then + // upgrade. The stale marker must be retired by the install itself. + fs.writeFileSync(path.join(kimiRoot, 'package.json'), `${COMMONJS_MARKER}\n`); + runInstall(root, 'kimi'); + assert.ok( + !fs.existsSync(path.join(kimiRoot, 'package.json')), + 'upgrading must retire the pre-#2544 marker at kimi\'s root', + ); + }); + + test('kimi: a user-authored package.json at the .kimi root is never retired', (t) => { + const root = mkTmp('gsd-2544-kimi-user-'); + t.after(() => cleanup(root)); + const kimiRoot = path.join(root, '.kimi'); + fs.mkdirSync(kimiRoot, { recursive: true }); + const userPkg = path.join(kimiRoot, 'package.json'); + fs.writeFileSync(userPkg, USER_PACKAGE_JSON); + const before = sha256(fs.readFileSync(userPkg)); + + runInstall(root, 'kimi'); + + assert.ok(fs.existsSync(userPkg), 'a user file at the .kimi root must survive'); + assert.equal(sha256(fs.readFileSync(userPkg)), before); + }); + + test('kimi: uninstall reclaims the hooks/ marker', (t) => { + const root = mkTmp('gsd-2544-kimi-uninstall-'); + t.after(() => cleanup(root)); + const kimiRoot = path.join(root, '.kimi'); + + runInstall(root, 'kimi'); + assert.ok(fs.existsSync(path.join(kimiRoot, 'hooks', 'package.json'))); + + runInstall(root, 'kimi', ['--uninstall']); + assert.ok( + !fs.existsSync(path.join(kimiRoot, 'hooks', 'package.json')), + 'uninstall must remove the kimi hooks/ marker it wrote', + ); + }); + + test('uninstall retires a pre-#2544 config-root marker', (t) => { + const root = mkTmp('gsd-2544-legacy-'); + t.after(() => cleanup(root)); + + runInstall(root, 'opencode'); + // Model an install made before the fix, which left the marker at the root. + fs.writeFileSync(path.join(root, 'package.json'), `${COMMONJS_MARKER}\n`); + + runInstall(root, 'opencode', ['--uninstall']); + assert.ok( + !fs.existsSync(path.join(root, 'package.json')), + 'uninstall must still retire the legacy config-root marker', + ); + }); +}); diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index 7c04b58d2..a1f6b9d11 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -355,8 +355,8 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", + "hooks/package.json", "mcp_config.json", - "package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index 087edac29..113bc8abd 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -426,7 +426,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index 198119e20..43932ee4d 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -425,7 +425,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index 462646883..22304022f 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -354,7 +354,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 961fea930..166aa8571 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -426,7 +426,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index 6ae9455a1..6d9e5cc24 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -355,7 +355,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 5a17d20e4..4495e38c0 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -426,9 +426,10 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", + "hooks/package.json", "kilo.json", - "package.json", "plugins/gsd-core.js", + "plugins/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index c0f0e5032..440421ec8 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -29,7 +29,7 @@ ".kimi/hooks/lib/git-cmd.js", ".kimi/hooks/lib/gsd-graphify-rebuild.sh", ".kimi/hooks/managed-hooks-registry.cjs", - ".kimi/package.json", + ".kimi/hooks/package.json", "agents/gsd-advisor-researcher.md", "agents/gsd-ai-researcher.md", "agents/gsd-assumptions-analyzer.md", diff --git a/tests/fixtures/install-tree/kimi.json b/tests/fixtures/install-tree/kimi.json index 9547457d0..25e4e6801 100644 --- a/tests/fixtures/install-tree/kimi.json +++ b/tests/fixtures/install-tree/kimi.json @@ -29,7 +29,7 @@ ".kimi/hooks/lib/git-cmd.js", ".kimi/hooks/lib/gsd-graphify-rebuild.sh", ".kimi/hooks/managed-hooks-registry.cjs", - ".kimi/package.json", + ".kimi/hooks/package.json", "agents/gsd.md", "agents/gsd.yaml", "agents/subagents/gsd-advisor-researcher.md", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 237c1f12a..e03be7292 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -426,9 +426,10 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", + "hooks/package.json", "opencode.json", - "package.json", "plugins/gsd-core.js", + "plugins/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index dd9b91d95..6998a3854 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -2,6 +2,7 @@ ".gsd-profile", ".gsd/defaults.json", "extensions/gsd.js", + "extensions/package.json", "gsd-core/.gsd-runtime", "gsd-core/VERSION", "gsd-core/bin/check-latest-version.cjs", @@ -322,7 +323,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 72cff8197..2493e5632 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -355,7 +355,7 @@ "hooks/lib/git-cmd.js", "hooks/lib/gsd-graphify-rebuild.sh", "hooks/managed-hooks-registry.cjs", - "package.json", + "hooks/package.json", "scripts/changeset/README.md", "scripts/changeset/cli.cjs", "scripts/changeset/github-release-notes.cjs", diff --git a/tests/gsd-check-update-worker-platform-gate.test.cjs b/tests/gsd-check-update-worker-platform-gate.test.cjs index 24651a247..1e8f90e4a 100644 --- a/tests/gsd-check-update-worker-platform-gate.test.cjs +++ b/tests/gsd-check-update-worker-platform-gate.test.cjs @@ -265,11 +265,13 @@ describe('Issue #815: --next dist-tag support', () => { * which 404s from the registry, leaving update_available permanently false. * * Original #378 fix derived the name from `require('../package.json').name`. - * That is broken at runtime (#498): the installed tree carries only a synthetic - * `{"type":"commonjs"}` package.json (no `.name`), so post-install the worker - * queried `npm view undefined version` → latest stayed null → update_available - * permanently false. The old structural test passed only because it grepped the - * DEV tree, where package.json still has a name. + * That is broken at runtime (#498): no package.json in the installed tree + * carries a `.name`. It used to resolve to the synthetic `{"type":"commonjs"}` + * marker GSD wrote at the config root, so post-install the worker queried + * `npm view undefined version` → latest stayed null → update_available + * permanently false; since #2544 GSD writes no marker there at all, so the + * require would now fail to resolve outright. The old structural test passed + * only because it grepped the DEV tree, where package.json still has a name. * * New contract (#498): the worker no longer resolves the package name itself. * It delegates the latest-version lookup to check-latest-version.cjs's @@ -339,9 +341,10 @@ describe('bug #378 / #498: update worker queries the scoped name via the seam', workerCodeOnly(), /require\s*\(\s*['"][^'"]*package\.json['"]\s*\)\s*\.name/, [ - 'require(package.json).name resolves to undefined in the installed tree', - '(only a {"type":"commonjs"} marker ships). The worker must delegate to', - 'checkLatestVersion(), which sources the name from the baked seam.', + 'require(package.json).name never yields a name in the installed tree —', + 'GSD stages only {"type":"commonjs"} markers, and since #2544 none at the', + 'config root. The worker must delegate to checkLatestVersion(), which', + 'sources the name from the baked seam.', ].join(' '), ); }); diff --git a/tests/helpers/emitted-provenance.cjs b/tests/helpers/emitted-provenance.cjs index 0855de0fd..2116377da 100644 --- a/tests/helpers/emitted-provenance.cjs +++ b/tests/helpers/emitted-provenance.cjs @@ -91,6 +91,14 @@ const HOOKS_WINDOWS_SHIM_SRC = 'src/runtime-hooks-surface.cts'; * (writeHermesCategoryDescription) as a code literal. */ const INSTALLER_SRC = 'bin/install.js'; +/** Module owning the #2544 `{"type":"commonjs"}` marker literal and the + * write/remove ownership predicate behind it. */ +const COMMONJS_MARKER_SRC = 'src/commonjs-marker.cts'; + +/** Engine module that stages the native plugin adapter and writes the marker + * beside it (_installNativePluginIfDeclared). */ +const INSTALL_ENGINE_SRC = 'src/install-engine.cts'; + /** Source file holding the Kimi root-agent literal (runtime-artifact-layout.cts:303). */ const KIMI_ROOT_AGENT_SRC = 'src/runtime-artifact-layout.cts'; @@ -331,7 +339,9 @@ const PROVENANCE_RULES = [ // Attribute to the REPO source a PR actually edits, not the build artifact. // Excludes Copilot's hook-registration JSON (next rule) — that is a code // literal, not a built script, and attributing it here resolved to a - // nonexistent `hooks/gsd-session.json`. + // nonexistent `hooks/gsd-session.json`. `package.json` is excluded for the + // same reason (the #2544 `commonjs-marker` rule below): there is no + // `hooks/package.json` in the repo to attribute to. // // `.cmd` shims are a SEPARATE, Windows-only emission path folded into this // SAME rule rather than a dedicated one (see the `transforms` doc above for @@ -346,16 +356,17 @@ const PROVENANCE_RULES = [ // same source file. The wrapped `.js` file's NAME flows into the `.cmd` // 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. + pattern: /^(?!gsd-session\.json$|package\.json$).+$/, + // `package.json` (the CommonJS marker) is excluded here and owned by the + // dedicated `commonjs-marker` rule below. #2717 attributed it inside THIS + // rule, routing it to HOOKS_WINDOWS_SHIM_SRC because at that point + // src/runtime-hooks-surface.cts was its only emitter (cursor/windsurf, plus + // the codex copy block calling the same exported helper). #2544 adds a + // second emitter — src/commonjs-marker.cts, via installSharedHooksBundle and + // _installNativePluginIfDeclared — and two roots this rule does not cover + // (`plugins`, `extensions`). A rule keyed to HOOKS_ROOTS with a single + // source can express neither, so the marker moves to its own rule and that + // rule names BOTH emitters. See the `commonjs-marker` entry below. // // `.cmd` shim bytes are code-derived (a literal template + the install-time // interpreter/path tokens in HOOKS_WINDOWS_SHIM_SRC) — the wrapped `.js` @@ -366,8 +377,57 @@ 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') || 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] : []), + // No `package.json` arm here: the pattern above excludes it, so the branch + // #2717 added for it is unreachable from this rule. + sources: (m) => [m[0].endsWith('.cmd') ? HOOKS_WINDOWS_SHIM_SRC : `hooks/${m[0]}`], + transforms: (m) => (m[0].endsWith('.cmd') ? [HOOKS_WINDOWS_SHIM_SRC] : []), + }, + { + id: 'commonjs-marker', + kind: 'code-derived', + // Every root GSD stages its own `.js` files into and therefore pins to + // CommonJS: the hooks roots, plus the native plugin/extension dirs. + roots: [...HOOKS_ROOTS, 'plugins', 'extensions'], + // #2544: a `{"type":"commonjs"}` module-type marker, written as a code + // literal so Node's ancestor walk resolves GSD's staged `.js` files as + // CommonJS under an ambient `"type": "module"`. Like the Copilot + // registration JSON above it is emitted, never built — there is no + // `hooks/package.json` or `plugins/package.json` in the repo, so the + // built-script rule would attribute it to a path that does not exist. + // Sources are scoped PER ROOT, not declared as one flat union. Each root has + // exactly one writer besides the shared marker module, and a flat list would + // attribute every root to all of them — `extensions/package.json` to the + // hooks-surface writer that never touches it, `.kimi/hooks/package.json` to + // the native-plugin writer, and so on. That matters because + // `emitted-diff.cjs` accepts the FIRST satisfied source: a flat list + // containing `bin/install.js` lets any change anywhere in that 13k-line file + // authorise marker drift for every root — the blanket escape hatch this + // file's own agents-verbatim comment (above) refuses for the same reason. + // + // hooks/ (shared bundle) -> bin/install.js (installSharedHooksBundle) + // hooks/ (#2717 runtimes) -> src/runtime-hooks-surface.cts + bin/install.js + // (the codex copy block calls the exported helper) + // .kimi/hooks/ -> bin/install.js (the kimi hooks-root bundle) + // plugins/, extensions/ -> src/install-engine.cts + // (_installNativePluginIfDeclared) + // + // COMMONJS_MARKER_SRC is in every root: it owns the marker BYTES, so a change + // to it can move any of them. + // The rule ctx is `{ rel, runtime }` — it carries no `root`, so the root is + // derived from `rel` here. Keying on a ctx field that does not exist would + // send every path down one branch silently, which is the failure this + // per-root split exists to prevent. + pattern: /^package\.json$/, + sources: (_m, ctx) => { + const root = String(ctx.rel).replace(/\/package\.json$/, ''); + if (root === 'plugins' || root === 'extensions') { + return [COMMONJS_MARKER_SRC, INSTALL_ENGINE_SRC]; + } + if (root === '.kimi/hooks') return [COMMONJS_MARKER_SRC, INSTALLER_SRC]; + // 'hooks' — written by the shared bundle for most runtimes and by the + // #2717 dedicated paths for cursor/windsurf/codex. + return [COMMONJS_MARKER_SRC, INSTALLER_SRC, HOOKS_WINDOWS_SHIM_SRC]; + }, }, { id: 'copilot-hook-registration', diff --git a/tests/installer-migration-config-root-marker.test.cjs b/tests/installer-migration-config-root-marker.test.cjs new file mode 100644 index 000000000..846faf607 --- /dev/null +++ b/tests/installer-migration-config-root-marker.test.cjs @@ -0,0 +1,343 @@ +'use strict'; + +/** + * Installer migration coverage for + * 2026-07-28-retire-config-root-commonjs-marker (#2544). + * + * #2544 moved GSD's `{"type":"commonjs"}` marker out of the runtime config root + * and into the directories GSD actually fills. Without this migration an + * UPGRADED install keeps both markers, so the config root stays pinned to + * CommonJS and the fix's own claim — that GSD no longer writes the shared + * config root — is false for every install made before it. + * + * The migration is unusual in one respect, and that is what most of this file + * pins: the config-root marker was never recorded in `gsd-file-manifest.json`, + * so `classifyArtifact` answers `unknown` for it and the planner's own guard + * downgrades a `remove-managed` on an `unknown` classification to + * `preserve-user`. The migration therefore supplies the "purpose-built detector + * for an old GSD-owned shape" that docs/installer-migrations.md#remove-managed + * sanctions, and DECLARES the resulting classification on the action. If that + * declaration ever stops being honoured the migration silently does nothing, so + * the end-to-end planner test below is a negative control, not a formality. + * + * Authoring-workflow coverage (docs/installer-migrations.md#authoring-workflow): + * dry-run plan output ....... "plan() emits remove-managed" + * apply behaviour ........... "applies through the real planner + executor" + * locally modified file ..... "a user-authored package.json is never touched" + * user-owned files nearby ... "leaves sibling files alone" + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const crypto = require('node:crypto'); + +const migration = require('../gsd-core/bin/lib/installer-migrations/007-retire-config-root-commonjs-marker.cjs'); + +const { + classifyArtifact: realClassifyArtifact, + readInstallManifest, + planInstallerMigrations, + applyInstallerMigrationPlan, +} = require('../gsd-core/bin/lib/installer-migrations.cjs'); + +// The shared teardown helper, not a local rmSync: it chdir's out of the target +// first (Windows cannot remove a directory that is the CWD) and retries +// 20 x 250ms to absorb the deferred-scan handle Windows Defender holds on +// newly-written files. This repo runs a windows-latest lane, so both matter. +const { cleanup } = require('./helpers.cjs'); + +const MARKER = '{"type":"commonjs"}'; +const ROOT_REL = 'package.json'; + +/** The shape #2544 was filed about — an OpenCode config-root manifest. */ +const USER_PACKAGE_JSON = JSON.stringify( + { + name: 'my-opencode-config', + type: 'module', + dependencies: { shescape: '^2.1.0' }, + }, + null, + 2, +) + '\n'; + +function createTempDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-migration-007-test-')); +} + +function writeFile(root, relPath, content) { + const fullPath = path.join(root, relPath); + fs.mkdirSync(path.dirname(fullPath), { recursive: true }); + fs.writeFileSync(fullPath, content, 'utf8'); +} + +function writeManifest(root, files) { + fs.writeFileSync( + path.join(root, 'gsd-file-manifest.json'), + JSON.stringify( + { version: '1.8.0', timestamp: '2026-07-28T00:00:00.000Z', mode: 'full', files }, + null, + 2, + ), + 'utf8', + ); +} + +function makePlanCtx(configDir) { + const manifest = readInstallManifest(configDir); + return { + configDir, + classifyArtifact: (relPath) => realClassifyArtifact(configDir, relPath, manifest), + }; +} + +const sha256 = (buf) => crypto.createHash('sha256').update(buf).digest('hex'); + +// --------------------------------------------------------------------------- +// 1. Metadata +// --------------------------------------------------------------------------- + +describe('migration 007 metadata', () => { + test('exports a single migration object with the required authoring fields', () => { + assert.equal(typeof migration, 'object'); + assert.equal(typeof migration.id, 'string'); + assert.ok(migration.id.length > 0, 'id must be non-empty'); + assert.equal(typeof migration.title, 'string'); + assert.equal(typeof migration.description, 'string'); + assert.equal(typeof migration.introducedIn, 'string'); + assert.ok(Array.isArray(migration.scopes), 'scopes must be an array'); + assert.ok(migration.scopes.includes('global')); + assert.ok(migration.scopes.includes('local')); + assert.strictEqual(migration.destructive, true); + assert.equal(typeof migration.plan, 'function'); + }); + + test('applies to every runtime — the marker was written for all of them', () => { + // "All runtimes" is expressed by OMITTING `runtimes`, never by `[]`. The two + // halves of the framework disagree about the empty array and only one of + // them runs on the record: `validateStringArray` requires the field to be a + // NON-EMPTY string array when present and throws otherwise, while the + // runtime filter (`Array.isArray(runtimes) && runtimes.length > 0`) would + // have treated `[]` as "all". A `runtimes: []` record therefore throws at + // plan time and the migration never runs at all — which is precisely how + // this migration was first written. + assert.equal(migration.runtimes, undefined, 'runtimes must be omitted, not []'); + }); + + test('id carries the expected date prefix and names the retired artifact', () => { + assert.ok(migration.id.startsWith('2026-07-28-'), `unexpected id: ${migration.id}`); + assert.match(migration.id, /config-root-commonjs-marker/); + }); +}); + +// --------------------------------------------------------------------------- +// 2. plan() behaviour +// --------------------------------------------------------------------------- + +describe('migration 007 plan()', () => { + test('emits no actions when the config root has no package.json (fresh install)', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeManifest(dir, {}); + + assert.deepEqual(migration.plan(makePlanCtx(dir)), [], 'no file -> no actions'); + }); + + test('emits remove-managed for the exact legacy marker, with declared ownership', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, `${MARKER}\n`); + writeManifest(dir, {}); + + const actions = migration.plan(makePlanCtx(dir)); + assert.equal(actions.length, 1); + const [action] = actions; + assert.equal(action.type, 'remove-managed'); + assert.equal(action.relPath, ROOT_REL); + assert.ok(action.ownershipEvidence && action.ownershipEvidence.length > 0); + // The declared classification IS the ownership proof — see the planner test. + assert.equal(action.classification, 'managed-pristine'); + assert.equal(action.originalHash, action.currentHash); + assert.equal(action.currentHash, sha256(`${MARKER}\n`)); + }); + + test('tolerates trailing-whitespace variants of the marker', (t) => { + for (const content of [MARKER, `${MARKER}\n`, `${MARKER}\n\n`, ` ${MARKER} \n`]) { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, content); + writeManifest(dir, {}); + assert.equal( + migration.plan(makePlanCtx(dir)).length, + 1, + `expected a match for ${JSON.stringify(content)}`, + ); + } + }); + + test('a user-authored package.json is never touched', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, USER_PACKAGE_JSON); + writeManifest(dir, {}); + + assert.deepEqual( + migration.plan(makePlanCtx(dir)), + [], + 'a package.json that is not exactly the marker must produce no action', + ); + }); + + test('a marker with any extra key is foreign — no backup-and-remove branch', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + // Semantically "the marker plus something the user added". The exact-content + // predicate rejects it, and there is deliberately no modified-file branch: + // this is somebody else's file, not a patched GSD artifact. + writeFile(dir, ROOT_REL, `${JSON.stringify({ type: 'commonjs', name: 'mine' })}\n`); + writeManifest(dir, {}); + + assert.deepEqual(migration.plan(makePlanCtx(dir)), []); + }); + + test('a symlinked package.json is never followed or planned', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + const outside = path.join(dir, 'outside.json'); + fs.writeFileSync(outside, `${MARKER}\n`); + fs.symlinkSync(outside, path.join(dir, ROOT_REL)); + writeManifest(dir, {}); + + assert.deepEqual( + migration.plan(makePlanCtx(dir)), + [], + 'a symlink is not something GSD wrote — never ours to remove', + ); + assert.ok(fs.existsSync(outside), 'the link target must survive'); + }); + + test('a directory at the marker path is never planned', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + fs.mkdirSync(path.join(dir, ROOT_REL)); + writeManifest(dir, {}); + + assert.deepEqual(migration.plan(makePlanCtx(dir)), []); + }); + + test('leaves sibling files in the config root alone', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, `${MARKER}\n`); + writeFile(dir, 'opencode.json', '{"theme":"mine"}\n'); + writeManifest(dir, {}); + + const actions = migration.plan(makePlanCtx(dir)); + assert.equal(actions.length, 1); + assert.equal(actions[0].relPath, ROOT_REL, 'only the marker is ever planned'); + }); +}); + +// --------------------------------------------------------------------------- +// 3. End-to-end through the REAL planner + executor +// --------------------------------------------------------------------------- + +describe('migration 007 through the real planner', () => { + test('the declared classification survives the planner (negative control)', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, `${MARKER}\n`); + writeManifest(dir, {}); + + // The config-root marker is NOT in the manifest, so the planner's own + // classify() answers 'unknown' for it... + const manifest = readInstallManifest(dir); + assert.equal( + realClassifyArtifact(dir, ROOT_REL, manifest).classification, + 'unknown', + 'precondition: the marker was never manifest-recorded', + ); + + // ...and `remove-managed` on an 'unknown' classification is downgraded to + // `preserve-user`. This assertion is the whole reason the migration declares + // its own classification: without that declaration the plan below would + // contain a preserve-user action and the migration would be inert. + const plan = planInstallerMigrations({ + configDir: dir, + runtime: 'opencode', + scope: 'global', + migrations: [migration], + }); + + const actions = plan.actions.filter((a) => a.relPath === ROOT_REL); + assert.equal(actions.length, 1, 'the marker must survive planning as one action'); + assert.equal( + actions[0].type, + 'remove-managed', + 'declared ownership must NOT be downgraded to preserve-user', + ); + }); + + test('applying the plan removes the marker and journals a rollback', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, `${MARKER}\n`); + writeFile(dir, 'opencode.json', '{"theme":"mine"}\n'); + writeManifest(dir, {}); + + const plan = planInstallerMigrations({ + configDir: dir, + runtime: 'opencode', + scope: 'global', + migrations: [migration], + }); + applyInstallerMigrationPlan({ configDir: dir, plan, runtime: 'opencode', scope: 'global' }); + + assert.ok( + !fs.existsSync(path.join(dir, ROOT_REL)), + 'the stale config-root marker must be gone after apply', + ); + assert.ok( + fs.existsSync(path.join(dir, 'opencode.json')), + 'sibling config files must survive', + ); + }); + + test('is idempotent — a second run plans nothing', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, `${MARKER}\n`); + writeManifest(dir, {}); + + const first = planInstallerMigrations({ + configDir: dir, runtime: 'opencode', scope: 'global', migrations: [migration], + }); + applyInstallerMigrationPlan({ configDir: dir, plan: first, runtime: 'opencode', scope: 'global' }); + + // Re-plan against the post-apply tree. The file is gone, so plan() short- + // circuits at the lstat and there is nothing left to do. + assert.deepEqual(migration.plan(makePlanCtx(dir)), []); + }); + + test('a user-authored package.json survives the full plan+apply cycle', (t) => { + const dir = createTempDir(); + t.after(() => cleanup(dir)); + writeFile(dir, ROOT_REL, USER_PACKAGE_JSON); + writeManifest(dir, {}); + const before = sha256(fs.readFileSync(path.join(dir, ROOT_REL))); + + const plan = planInstallerMigrations({ + configDir: dir, runtime: 'opencode', scope: 'global', migrations: [migration], + }); + applyInstallerMigrationPlan({ configDir: dir, plan, runtime: 'opencode', scope: 'global' }); + + assert.ok(fs.existsSync(path.join(dir, ROOT_REL)), 'the user file must survive'); + assert.equal( + sha256(fs.readFileSync(path.join(dir, ROOT_REL))), + before, + 'the user file must be byte-identical after the migration runs', + ); + }); +}); diff --git a/tests/installer-migration-install.integration.test.cjs b/tests/installer-migration-install.integration.test.cjs index 4f8f25f82..b8454d1a2 100644 --- a/tests/installer-migration-install.integration.test.cjs +++ b/tests/installer-migration-install.integration.test.cjs @@ -24,29 +24,34 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); const installScript = path.join(__dirname, '..', 'bin', 'install.js'); const SUPPORTED_RUNTIMES = installModule.allRuntimes; const RUNTIME_INSTALL_CONTRACTS = { - claude: { surface: 'flat-skills', settings: true, packageJson: true }, - antigravity: { surface: 'flat-skills', settings: true, packageJson: true }, - augment: { surface: 'flat-skills', settings: true, packageJson: true }, - cline: { surface: 'clinerules', settings: false, packageJson: false }, - codebuddy: { surface: 'flat-skills', settings: true, packageJson: true }, - codex: { surface: 'flat-skills', settings: false, packageJson: false, codexConfig: true }, - copilot: { surface: 'flat-skills', settings: false, packageJson: false, copilotInstructions: true }, - cursor: { surface: 'flat-skills', settings: false, packageJson: false }, - gemini: { surface: 'commands-gsd', settings: true, packageJson: true }, - hermes: { surface: 'hermes-skills', settings: true, packageJson: true }, - kimi: { surface: 'kimi-skills-agents', settings: false, packageJson: false }, + claude: { surface: 'flat-skills', settings: true, hooksPackageJson: true }, + antigravity: { surface: 'flat-skills', settings: true, hooksPackageJson: true }, + augment: { surface: 'flat-skills', settings: true, hooksPackageJson: true }, + cline: { surface: 'clinerules', settings: false, hooksPackageJson: false }, + codebuddy: { surface: 'flat-skills', settings: true, hooksPackageJson: true }, + // codex/cursor/windsurf: #2717 stages their .js hooks via dedicated paths + // (skipSharedHooksInstall / the !isCodex gate keep them out of the shared + // bundle) and writes the marker beside those scripts, so hooks/package.json + // is expected for all three. Measured on this tree: codex stages 3 .js hooks, + // cursor 6, windsurf 2 — each with the marker. + codex: { surface: 'flat-skills', settings: false, hooksPackageJson: true, codexConfig: true }, + copilot: { surface: 'flat-skills', settings: false, hooksPackageJson: false, copilotInstructions: true }, + cursor: { surface: 'flat-skills', settings: false, hooksPackageJson: true }, + gemini: { surface: 'commands-gsd', settings: true, hooksPackageJson: true }, + hermes: { surface: 'hermes-skills', settings: true, hooksPackageJson: true }, + kimi: { surface: 'kimi-skills-agents', settings: false, hooksPackageJson: false }, // #2454: Kimi Code (Node CLI) has NO custom named subagents (per official // docs), so its install surface is skills-only (flat-skills), NOT // kimi-skills-agents. The kimi-agents YAML layout is Python kimi-cli only. - 'kimi-code': { surface: 'flat-skills', settings: false, packageJson: false }, + 'kimi-code': { surface: 'flat-skills', settings: false, hooksPackageJson: false }, // #2305: Kilo's native plugin spawns the staged guard hooks, so it receives // the shared hooks bundle + the CommonJS package.json marker, like OpenCode. // (#1821 excluded Kilo on the false premise that it had no plugin surface.) - kilo: { surface: 'flat-command', settings: false, packageJson: true }, + kilo: { surface: 'flat-command', settings: false, hooksPackageJson: true }, // #2329: OpenCode discovers commands from the PLURAL `commands/` dir — the // singular `command/` (still correct for Kilo) made all /gsd-* commands // invisible to OpenCode. commandDirName overrides the flat-command default. - opencode: { surface: 'flat-command', settings: true, packageJson: true, commandDirName: 'commands' }, + opencode: { surface: 'flat-command', settings: true, hooksPackageJson: true, commandDirName: 'commands' }, // #2102 Stage 1/2: pi is a PLUGIN-ONLY install (hostBehaviors.pluginOnlyInstall) // for commands/agents/skills — NO commands/, agents/, or skills/ dir. pi's // /gsd command is registered programmatically by the native extension @@ -60,13 +65,13 @@ const RUNTIME_INSTALL_CONTRACTS = { // like OpenCode and (since #2305) Kilo — architecturally identical: // hooksSurface:'none' + a native plugin that spawns the staged hooks — NOT // like ZCode (no plugin surface, where the same hooks are dead weight). - pi: { surface: 'plugin-only', settings: false, packageJson: true }, - qwen: { surface: 'flat-skills', settings: true, packageJson: true }, - trae: { surface: 'flat-skills', settings: false, packageJson: false }, - windsurf: { surface: 'global-artifacts-noop', settings: false, packageJson: false }, + pi: { surface: 'plugin-only', settings: false, hooksPackageJson: true }, + qwen: { surface: 'flat-skills', settings: true, hooksPackageJson: true }, + trae: { surface: 'flat-skills', settings: false, hooksPackageJson: false }, + windsurf: { surface: 'global-artifacts-noop', settings: false, hooksPackageJson: true }, // #1821: ZCode (hooksSurface:none, no plugin surface) no longer receives the // dead hook scripts or the CommonJS package.json marker. - zcode: { surface: 'flat-skills', settings: false, packageJson: false }, + zcode: { surface: 'flat-skills', settings: false, hooksPackageJson: false }, }; function sha256(content) { @@ -384,10 +389,18 @@ function assertFreshInstallContract(runtime, targetDir) { contract.settings, `${runtime} settings.json presence should match the runtime contract` ); + // #2544: the CommonJS marker lives in hooks/ (the dir GSD fills with its own + // .js scripts), never at the config root — that file is user-owned territory + // on OpenCode/Kilo and was being clobbered on every install. + assert.equal( + fs.existsSync(path.join(targetDir, 'hooks', 'package.json')), + contract.hooksPackageJson, + `${runtime} hooks/package.json presence should match the runtime contract` + ); assert.equal( fs.existsSync(path.join(targetDir, 'package.json')), - contract.packageJson, - `${runtime} package.json presence should match the runtime contract` + false, + `${runtime} must not receive a GSD package.json at the config root (#2544)` ); if (contract.codexConfig) { diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index fa5d5c751..d2ee4669b 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -1673,6 +1673,15 @@ test('shipped installer-migration checksums are locked to a committed baseline ( // old path drops out of the manifest and uninstall can never remove it. '2026-07-20-pi-extension-cjs-to-js': 'sha256:185fa926ae24d83cbdd95c31a9ad2cc8d123e176ad543669b3b0ed75e6ca6f4a', + // Migration 007 (NEW, added here per this test's own sanctioned "adding a new + // migration" case — not a shipped-body edit): retire the pre-#2544 + // {"type":"commonjs"} marker at the runtime config root. #2544 moved that + // marker into the directories GSD fills, so without this an upgraded install + // keeps both and the config root stays pinned to CommonJS. Ownership is proven + // by exact content match rather than the manifest — the config-root marker was + // never manifest-recorded — so the action declares its own classification. + '2026-07-28-retire-config-root-commonjs-marker': + 'sha256:8f2140cbe8f2dd8f7dfd52a0f6957c5edfe966c52d7e6e4d74ec7366930e0e1d', }; const { DEFAULT_MIGRATIONS_DIR, migrationChecksum: computeChecksum } = require('../gsd-core/bin/lib/installer-migrations.cjs'); diff --git a/tests/issue-498-package-identity.test.cjs b/tests/issue-498-package-identity.test.cjs index 1e8fd0513..7331e8894 100644 --- a/tests/issue-498-package-identity.test.cjs +++ b/tests/issue-498-package-identity.test.cjs @@ -5,8 +5,10 @@ process.env.GSD_TEST_MODE = '1'; // The package coordinates (npm name, bin name, repo slug, changelog URL) are // DERIVED from package.json, not re-typed. deriveIdentity is the pure core; // the generated runtime module gsd-core/bin/lib/package-identity.cjs -// bakes those values at build time so it survives the install layout where -// the only package.json present is the synthetic {"type":"commonjs"} marker. +// bakes those values at build time so it survives the install layout, where no +// package.json carries a .name — the only ones GSD stages are synthetic +// {"type":"commonjs"} markers, and since #2544 those live in GSD's own +// directories (hooks/, the native plugin dir) rather than the config root. const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); diff --git a/tests/kilo-upgrades.test.cjs b/tests/kilo-upgrades.test.cjs index c96a79bcf..68d83dce3 100644 --- a/tests/kilo-upgrades.test.cjs +++ b/tests/kilo-upgrades.test.cjs @@ -333,10 +333,17 @@ for (const scope of ['global', 'local']) { const hookPath = path.join(configDir, 'hooks', hook); assert.ok(fs.existsSync(hookPath), `${hookPath} must be staged by the install`); } - // The CommonJS marker installSharedHooksBundle writes alongside hooks/. - const marker = path.join(configDir, 'package.json'); - assert.ok(fs.existsSync(marker), 'CommonJS package.json marker must be staged'); - assert.equal(JSON.parse(fs.readFileSync(marker, 'utf8')).type, 'commonjs'); + // #2544: the CommonJS marker is staged INSIDE the directories GSD owns and + // fills — hooks/ (the staged guard scripts) and plugins/ (the native + // adapter) — never at the config root, which is user-writable territory on + // Kilo (where a package.json declares local-plugin npm dependencies). + for (const ownedDir of ['hooks', 'plugins']) { + const marker = path.join(configDir, ownedDir, 'package.json'); + assert.ok(fs.existsSync(marker), `CommonJS package.json marker must be staged in ${ownedDir}/`); + assert.equal(JSON.parse(fs.readFileSync(marker, 'utf8')).type, 'commonjs'); + } + assert.ok(!fs.existsSync(path.join(configDir, 'package.json')), + 'the config root must not receive a GSD package.json (#2544)'); // Staged hooks are tracked in the manifest (drift/uninstall accounting). assert.ok(manifest && manifest.files['hooks/gsd-prompt-guard.js'], diff --git a/tests/kimi-upgrades.test.cjs b/tests/kimi-upgrades.test.cjs index f6f67e00e..d5ec96367 100644 --- a/tests/kimi-upgrades.test.cjs +++ b/tests/kimi-upgrades.test.cjs @@ -118,8 +118,13 @@ test('kimi --global: native config.toml [[hooks]] bus wired at /.kimi/conf 'hooks/ must NOT be installed under the generic Agent-Skills configDir for kimi'); assert.ok(!fs.existsSync(path.join(root, 'package.json')), 'package.json (CommonJS marker) must NOT be installed under the generic Agent-Skills configDir for kimi'); - assert.ok(fs.existsSync(path.join(root, '.kimi', 'package.json')), - 'package.json (CommonJS marker) must be installed alongside config.toml under ~/.kimi'); + // #2544: the marker moved INSIDE hooks/ — the directory GSD itself creates + // and fills — so kimi's own config home (~/.kimi) is never written to. The + // pre-#2544 root marker is retired by uninstall; a fresh install writes none. + assert.ok(fs.existsSync(path.join(hooksDir, 'package.json')), + 'package.json (CommonJS marker) must be installed inside ~/.kimi/hooks — the GSD-owned dir (#2544)'); + assert.ok(!fs.existsSync(path.join(root, '.kimi', 'package.json')), + 'package.json (CommonJS marker) must NOT be written at ~/.kimi root — kimi\'s config home is not GSD territory (#2544)'); }); test('kimi --global: reinstalling is idempotent — the GSD [[hooks]] block is not duplicated', (t) => {