From cc3ee301a7827a06bca912539594a3e88134fd7f Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Sat, 1 Aug 2026 20:00:23 -0500 Subject: [PATCH] fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root (#2593) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root installSharedHooksBundle wrote `{"type":"commonjs"}` over /package.json unconditionally — no existence check, no merge, no backup — on every install and every /gsd-update re-install. On the 11 affected runtimes that file is often user-owned; on OpenCode and Kilo it is the documented place to declare local-plugin npm dependencies, so a user's name/type/dependencies/scripts were destroyed on each run. The uninstall path already read the file and unlinked it only on an exact content match. That asymmetry was the defect: the discipline existed in the codebase, it just was not applied on the write side. Move the marker into the directories GSD creates and fills with its own .js files — hooks/ (all shared-hooks runtimes, incl. Kimi's own root) and the nativePlugin dir (plugins/ for OpenCode+Kilo, extensions/ for pi) — and stop writing the config root entirely. New src/commonjs-marker.cts owns the marker string plus one ownership predicate (absent / gsd-owned / foreign, fail-closed on an unreadable file) shared by ensureCommonJsMarker and removeCommonJsMarker, so install and uninstall cannot drift apart again. Nothing else depended on the config-root marker: package identity is baked at build time (#378/#498) and version resolution prefers gsd-core/VERSION and already tolerates a missing root package.json (#1383) — Codex has installed without one all along. A package.json in plugins/ or extensions/ is inert to plugin discovery, which globs *.{ts,js} only (see installer-migration 006). Uninstall retires the pre-fix config-root marker, so upgrading users are cleaned up on removal, and still never touches a file it did not write. * fix(#2544): point the changeset fragment at the filed PR The fragment's `pr:` field is only knowable after `gh pr create` returns. * fix(#2544): register commonjs-marker.cjs in the tsc-generated ESLint ignore set bin/lib/commonjs-marker.cjs is tsc output (src/commonjs-marker.cts is the linted source), so it belongs in the ADR-457 ignore list like its siblings. Clears the lint-tests no-var failure and the repo-invariants "linted xor ignored" migration-state test. * fix(#2544): pin the kimi CommonJS marker to hooks/, not the ~/.kimi root The UPGRADE 1 test still asserted the pre-#2544 marker location (~/.kimi/package.json). The marker now lives inside ~/.kimi/hooks — the directory GSD itself creates — matching the updated golden-install-parity and install-tree fixtures. Also asserts the root marker is NOT written. * fix(#2544): make the CommonJS marker write path non-fatal Review round 2, Major 3 + Minor 1 + the stagedHooks nit. ensureCommonJsMarker rethrew any non-EEXIST write error and neither call site caught it, so EACCES on a read-only hooks/, EROFS, or ENOSPC aborted the whole install with a raw stack trace. Every other marker interaction in the module is best-effort — removeCommonJsMarker swallows unlink failures, classifyMarker swallows read failures — and this was the write path, i.e. the one most likely to fail on a locked-down config dir. It now returns a new 'failed' outcome and both call sites warn and continue. Sibling found while sweeping for the same defect class: fs.mkdirSync sat OUTSIDE the try block, so an unwritable parent threw past the guard entirely. Creating the directory is the same environmental hazard as writing into it, so it moved inside. Also in this file: - The hooks marker is now gated on `stagedHooks && hooksOk`, not stagedHooks alone. 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 claims an ownership the install did not earn. - The uninstall rmdir of the native plugin dir is gated on GSD having actually removed something from it. Hoisting it out of the adapter-exists guard (so the marker-only case could prune) had silently widened it into deleting a user-created but empty plugins/ or extensions/ dir — the same "don't touch territory GSD didn't fill" principle this issue is about, inverted. - Kimi's pre-#2544 marker at its native hook root (~/.kimi) is retired at the same call site that writes its replacement. That path is outside kimi's configDir, so installer-migration 007 structurally cannot reach it. * fix(#2544): retire the stale config-root marker via installer-migration 007 Review round 2, Major 1 — the PR's headline claim was false for existing installs. Upgraders kept BOTH markers: the new one under hooks/ and the stale {"type":"commonjs"} at the config root, so their config root stayed pinned to CommonJS and their dependency manifest stayed gone until they uninstalled. The migration is unusual in one way, and it is the part worth reviewing: the config-root marker was never recorded in gsd-file-manifest.json (writeManifest records hooks/, agents/, commands/, scripts/ and the native plugin, never a root package.json), so classifyArtifact answers 'unknown' for it and the planner's own guard downgrades a remove-managed on an 'unknown' classification to preserve-user. 007 therefore supplies the "purpose-built detector for an old GSD-owned shape" that docs/installer-migrations.md#remove-managed sanctions — exact content match, the same predicate removeCommonJsMarker has always used — and declares the resulting classification on the action. A package.json with any other content is left untouched, and there is deliberately no backup-and-remove branch: a non-matching file here is not a patched GSD artifact, it is somebody else's file. Scope is all runtimes. The `runtimes` field is OMITTED rather than `[]`: validateStringArray requires the field to be non-empty WHEN PRESENT, while the runtime filter treats an empty array as "all" — so `runtimes: []` throws at plan time and the migration never runs. The metadata test pins this. Kimi is a deliberate carve-out, named in the migration's own header: its marker lived at ~/.kimi, outside kimi's configDir, and migration relPaths are structurally confined to configDir. It is retired by the installer instead. Registration: shipped-migrations table, .gitignore for the emitted .cjs, the EXPECTED_CHECKSUMS baseline, and the ESLint ignore set. That last one is not copied from migration 006 by rote — 006 needs no entry because it imports nothing, while 007 imports node builtins, so tsc emits its __importDefault helper and the `var` in it trips no-var. This is the same lint gate that made round 1 red. * test(#2544): fault-injection and multi-runtime marker coverage Review round 2, Major 2 + Minors 4 and 5. Major 2 — CONTRIBUTING.md:514-531 is mandatory for install/uninstall flows and the suite had no fs monkeypatching at all. Every branch now covered is one whose doc comment claims it as the module's safety posture: - classifyMarker non-ENOENT lstat error -> 'foreign' (the fail-closed rule), with an ENOENT control alongside it so the test discriminates rather than just asserting one side - classifyMarker readFileSync throw -> 'foreign' (present-but-unreadable never downgrades to the permissive answer) — the fixture's bytes are exactly GSD's marker, so the test fails if the code ever answers on content it could not read - a DIRECTORY at the marker path (CONTRIBUTING:521; the symlink case was already covered with a real symlink, the directory case needs no injection at all) - the ensureCommonJsMarker TOCTOU EEXIST branch — the entire reason for flag:'wx' - the new 'failed' outcome, for both writeFileSync (EACCES/EROFS/ENOSPC) and the mkdirSync that used to sit outside the guard - removeCommonJsMarker unlink throw -> false These save and restore fs methods in `finally` rather than using chmod 0o000, which does not fault under root and would pass vacuously in root Docker and CI. Minor 4 — uninstall was driven for opencode only. pi's extensions/ and both kimi locations now have behavioral coverage, install and uninstall, each paired with a user-authored-file case proving GSD leaves it alone. Minor 5 — the stagedHooks gate had no assertion behind its stated reason. A pre-existing, GSD-untouched hooks/ directory is now driven through a runtime that declares skipSharedHooksInstall and asserted to stay marker-free, with its user content intact. Also regression-tests the uninstall rmdir gate from the previous commit: an empty plugin dir GSD removed nothing from must survive. * docs(#2544): correct stale marker prose, register the module, document the trade-off Review round 2, Minors 2, 3 and 6. Minor 2 — six files asserted the installed ROOT ships the synthetic marker. None was load-bearing (all three walk-up consumers are VERSION-first with try/catch and the marker never carried a `version`), but ADR-457:52 is the rationale for keeping a generated module, so a future reader would mis-derive the constraint from it. Each site is corrected to what is now true: the installed tree carries no package.json with a .name at all, because the only ones GSD stages are {"type":"commonjs"} markers and they now live in GSD's own directories. Two of the six needed more than a location swap. hooks/gsd-check-update-worker.js and the platform-gate test both described `require('../package.json').name` resolving to undefined; post-#2544 that require does not resolve at all, so the history is kept accurate and the present-tense claim corrected rather than just moved. And src/runtime-artifact-conversion.cts described the no-root-package.json case as Codex-only — it is now every runtime, which strengthens that comment's own argument for lazy resolution. The generated .cjs sibling needs no edit: it is gitignored build output, not a tracked file. Minor 3 — src/commonjs-marker.cts had no CONTEXT.md entry, unlike every peer module, and CONTEXT.md is the #2 co-change partner of bin/install.js. Added, including the fail-closed posture and the never-throws contract. Minor 6 — the plugins//extensions/ marker shadows the config root for all .js siblings, so an OpenCode/Kilo user's ESM plugin/*.js stays broken. That is exactly what #2544's Fix section prescribed and it is disclosed in the PR body, but the PR body is not documentation. It now lives in the OpenCode section of docs/how-to/install-on-your-runtime.md, stated as a real constraint rather than a pure improvement, with the .ts mitigation and a fallback for ESM plugins. * test(#2544): attribute the CommonJS marker in the emitted-provenance rules The differential emitted-attribution gate (#2723, landed on `next` after this branch was cut) went red on the macOS shards once this PR rebased onto it. Two distinct causes, both real gaps rather than noise: 1. `plugins/package.json` and `extensions/package.json` matched NO rule — the `native-plugin` rule covers `*.{js,cjs,mjs}` only, so the marker read as an unattributed emitted family. 2. `hooks/package.json` fell through to `hooks-built`, which attributes an emitted `hooks/` to a repo source `hooks/`. There is no `hooks/package.json` in the repo, so it resolved to a nonexistent path. Cause 2 is exactly the failure already documented three lines above it for Copilot's `gsd-session.json` — "a code literal, not a built script" — so the fix follows that precedent rather than inventing one: `package.json` is excluded from `hooks-built` the same way, and a dedicated `commonjs-marker` rule attributes the family across all four roots it can appear in (both hooks roots plus `plugins`/`extensions`) to the sources that actually emit it. Deliberately a RULE, not an entry in tests/emitted-drift-ack.json. An ack is for a one-off ripple and goes stale by design — the gate fails a stale ack precisely so it cannot pre-clear the next change on that path. These markers are a permanent part of the emitted tree from #2544 onward, so they need standing attribution. Verified by reproducing the CI failure locally with GSD_EMITTED_BASE: 3 provenance errors + 12 unattributed paths before, 35/35 green after. * fix(#2544): route the #2717 hooks-surface marker helpers through commonjs-marker #2717 landed a second copy of ensureCommonJsMarker/removeCommonJsMarkerIfGsdOwned in src/runtime-hooks-surface.cts for the runtimes that stage .js hooks via dedicated paths (cursor/windsurf/codex). That copy had drifted from this PR's module on 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 went straight through it. Demonstrated: against the pre-fix copy, ensureCommonJsMarker() on a hooks/ dir holding a dangling package.json symlink returns true and creates {"type":"commonjs"} OUTSIDE that directory. - create: a plain writeFileSync leaves the classify->write window open, where commonjs-marker creates with flag:'wx' (O_EXCL). Both helpers now delegate to src/commonjs-marker.cts, which is what this PR's own docstring already claimed was the single place these rules are enforced. Exported signatures are unchanged (still boolean), so bin/install.js and the #2717 tests are unaffected. The new subtest is the only coverage that fails if the duplicate is ever reintroduced — the two implementations agree on every non-adversarial input, so the existing suites pass against both. * test(#2544): pin the stagedHooks gate on zcode, not windsurf The Minor-5 coverage picked windsurf because hostBehaviors.skipSharedHooksInstall kept it out of the shared hooks bundle, so GSD staged nothing into hooks/ and the marker was correctly absent. #2717 changed that premise: cursor/windsurf/codex now stage their .js hooks via dedicated paths and get the marker beside those scripts. Measured on this tree, windsurf stages 2 .js hooks and receives a marker — so the assertion was pinning behaviour that is now wrong, not the gate it was written for. 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: 0 staged, no marker). The property under test is unchanged — a user-created hooks/ directory GSD never fills stays marker-free. * test(#2544): use the shared cleanup helper in the migration test Addresses the review's Major 1. The suppression's stated reason — "no helpers import available" — was not correct: tests/helpers.cjs exports cleanup, and the other test file added in this same PR imports it (tests/commonjs-marker.test.cjs). The local reimplementation dropped two protections that are live on this repo's windows-latest lane: the CWD guard (Windows cannot remove a directory that is the current working directory) and the 20 x 250ms retry budget that absorbs the deferred-scan handle Windows Defender holds on newly-written files. Local function and suppression both removed; local/no-raw-rmsync-in-tests now passes without one. * test(#2544): expect hooks/package.json for the #2717 runtimes The fresh-install contract table predates #2717, which stages cursor/windsurf/ codex .js hooks via dedicated paths and writes the CommonJS marker beside them. All three therefore now receive hooks/package.json legitimately. Measured on this tree: codex stages 3 .js hooks, cursor 6, windsurf 2 — each with the marker; cline/copilot/trae/zcode stage none and get none, so their contracts are unchanged. * fix(#2544): gate the #2717 marker writes on having staged something The three dedicated marker writers #2717 added ran unconditionally. Each one mkdirs hooks/ up front and stages its scripts conditionally on the source existing, so with an absent or empty hook source they created a directory, filled it with nothing, and marked it as GSD's anyway. That is the same write-into-someone-else's-territory this issue is about, and installSharedHooksBundle already guards the identical case with `stagedHooks`. The dedicated paths now carry the matching gate: - cursor / windsurf: `installedScripts.size > 0` - codex: a new `codexStagedHooks` flag. The enclosing guard only proves that hooks/dist EXISTS; it says nothing about whether any CODEX_HOOKS_TO_COPY entry landed. Covered for cursor and windsurf by driving each writer against a src tree whose hooks/ dir is empty. The codex leg is defensive and deliberately uncovered: its trigger state needs a package tree where hooks/dist exists but holds none of the allowlist, which is not constructible from a real checkout. * test(#2544): scope the commonjs-marker sources per root The rule declared one flat source list for every marker root, so `extensions/package.json` was attributed to runtime-hooks-surface.cts (which never writes there) and `.kimi/hooks/package.json` to install-engine.cts. That is not merely untidy. emitted-diff.cjs accepts the FIRST satisfied source, so a flat list containing bin/install.js let 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 refuses for exactly the same reason. Sources are now derived per root from ctx.rel. Note the rule ctx is `{ rel, runtime }` and carries no `root`, so keying on ctx.root would have sent every path down one branch silently. * test(#2544): state precisely what the zcode assertion pins The comment claimed the test pinned installSharedHooksBundle's `stagedHooks` gate. It does not, and neither did the windsurf version it replaced: zcode declares skipSharedHooksInstall, so the outer guard skips that helper entirely and the gate is never evaluated. The test passes on the runtime exclusion. What it does pin — the outcome a pre-existing, GSD-untouched hooks/ stays marker-free — is still worth having, and is what the review asked for. The two `staging zero hook scripts` tests are the ones that pin a real staged-nothing gate. Comment corrected rather than left implying coverage that is not there. --------- Co-authored-by: Tom Boucher --- .../2544-shared-hooks-package-json-clobber.md | 5 + .gitignore | 2 + CONTEXT.md | 3 + bin/install.js | 171 ++++- docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 1 + docs/adr/457-generated-cjs-single-source.md | 11 +- docs/how-to/install-on-your-runtime.md | 6 + docs/installer-migrations.md | 1 + eslint.config.mjs | 5 + hooks/gsd-check-update-worker.js | 9 +- scripts/generate-package-identity.cjs | 6 +- src/commonjs-marker.cts | 142 ++++ src/install-engine.cts | 28 + ...007-retire-config-root-commonjs-marker.cts | 196 ++++++ src/runtime-artifact-conversion.cts | 13 +- src/runtime-hooks-surface.cts | 88 ++- tests/commonjs-marker.test.cjs | 659 ++++++++++++++++++ tests/fixtures/install-tree/antigravity.json | 2 +- tests/fixtures/install-tree/augment.json | 2 +- tests/fixtures/install-tree/claude-local.json | 2 +- tests/fixtures/install-tree/claude.json | 2 +- tests/fixtures/install-tree/codebuddy.json | 2 +- tests/fixtures/install-tree/hermes.json | 2 +- tests/fixtures/install-tree/kilo.json | 3 +- tests/fixtures/install-tree/kimi-code.json | 2 +- tests/fixtures/install-tree/kimi.json | 2 +- tests/fixtures/install-tree/opencode.json | 3 +- tests/fixtures/install-tree/pi.json | 3 +- tests/fixtures/install-tree/qwen.json | 2 +- ...check-update-worker-platform-gate.test.cjs | 19 +- tests/helpers/emitted-provenance.cjs | 86 ++- ...ller-migration-config-root-marker.test.cjs | 343 +++++++++ ...ler-migration-install.integration.test.cjs | 55 +- tests/installer-migrations.test.cjs | 9 + tests/issue-498-package-identity.test.cjs | 6 +- tests/kilo-upgrades.test.cjs | 15 +- tests/kimi-upgrades.test.cjs | 9 +- 38 files changed, 1766 insertions(+), 150 deletions(-) create mode 100644 .changeset/2544-shared-hooks-package-json-clobber.md create mode 100644 src/commonjs-marker.cts create mode 100644 src/installer-migrations/007-retire-config-root-commonjs-marker.cts create mode 100644 tests/commonjs-marker.test.cjs create mode 100644 tests/installer-migration-config-root-marker.test.cjs 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) => {