Files

0xdhx cc3ee301a7 fix(#2544 ): stage the CommonJS marker in GSD-owned dirs, not the config root (#2593 )

* fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root

installSharedHooksBundle wrote `{"type":"commonjs"}` over
<configRoot>/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/<X>` to a repo source `hooks/<X>`. 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 <trekkie@nomorestars.com>

2026-08-01 21:00:23 -04:00

9.5 KiB

Raw Blame History

ADR 457: Generation model for `bin/lib/*.cjs` type safety [Accepted]

Status: Accepted
Date: 2026-05-28 (rewritten and accepted 2026-05-31 after correcting fabricated context)

Accepted direction: build-at-publish (model 2 below). Implementation is tracked in a separate migration issue and proceeds module-by-module. The sole prerequisite — the ESLint harness (ADR 452) — is already accepted/merged.

Provenance note. An earlier draft of this ADR (and issue #457) was authored by an agent and asserted a codebase state that did not exist — "~13 files generated from .ts via tsc", gsd-core/src/ / sdk/src/ source trees, and a tests/cjs-ts-parity.test.cjs. None of those existed. This rewrite grounds the decision in verified ground truth. Do not restore the earlier "natural completion of the 13 generated files" framing; it was fiction.

Context

What actually exists today (verified 2026-05-31)

gsd-core/bin/lib/ holds 84 .cjs files. Exactly one carries a // @generated header: package-identity.cjs.
That one generated file is not tsc output. It is produced by scripts/generate-package-identity.cjs — a plain Node script that reads package.json and bakes literal coordinate values into a CJS module.
There is no gsd-core/src/ or sdk/src/ TypeScript tree. There is no TS→CJS transpilation pipeline. There is no tests/cjs-ts-parity.test.cjs. The only parity test is tests/issue-498-package-identity.test.cjs, scoped to the one baked file: it regenerates from package.json and asserts the committed output is not stale.
tsconfig.lint.json exists with allowJs + checkJs, but it is not wired into eslint.config.mjs. The .cjs config block (eslint.config.mjs lines 61–92) sets only sourceType/globals — no parser, no parserOptions.project, no projectService, and no @typescript-eslint rule is enabled. A comment on line 60 nonetheless claims "Type-aware via parserOptions.project=tsconfig.lint.json" — so the file is linted without type information while the config advertises the opposite.
eslint.config.mjs lists 12 files in GENERATED_CJS_IGNORES and treats them as generated (never linted) — but those 12 are all hand-written (verified: none carry an @generated header). This is a latent inconsistency: the lint config already pretends a generation pipeline exists for them.

So the real situation is: 83 hand-written .cjs, 1 value-baked .cjs, and a lint config that already advertises — in a comment and in a 12-file ignore list — a type-aware generation pipeline that was never built.

Two different things are both called "generation"

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 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.
Transpilation (proposed, optional). Authoring bin/lib logic as TS and emitting .cjs via tsc. Deletion test: remove it and nothing reappears — a hand-written .cjs and a tsc-emitted .cjs are behaviorally identical at runtime. The seam buys no runtime leverage. Its entire value is author-time and CI type checking.

package-identity is therefore not precedent for the proposed transpilation work. They are different techniques with different forcing functions.

The problem actually worth solving

Type safety on the hand-written runtime surface is second-class: type errors surface (if at all) as lint findings via the un-wired tsconfig.lint.json, not as compile errors. Any contributor — human or agent — adding a bin/lib file must decide hand-write vs generate, with no enforced answer. That inconsistency is real and grows.

The decision this ADR must make

Type-checking TS sources is the goal. The load-bearing question the earlier draft skipped is: do we check the generated .cjs into git, or treat it as a build artifact? Three models:

Check in both .ts source and .cjs output. Creates a permanent "two copies must match" invariant, requiring parity tests, dual commits, and a pre-commit/CI drift gate. This is the model the earlier draft assumed — inherited from value-baking, where checking in output is forced. For transpilation, nothing forces it, so this imports maximum friction for no runtime gain.
Build at publish (recommended). bin/lib/*.cjs becomes a gitignored build artifact emitted from a TS src/ tree by tsc; npm publishes the built output. Feasible today: package.json already ships gsd-core and scripts via its files array, and already runs a pre-publish build step ("prepublishOnly": "npm run build:hooks") — the .cjs emit hooks into the same step, and npm pack includes on-disk artifacts regardless of .gitignore. No drift invariant, no parity test, no dual commits — the "Negative" consequences below mostly evaporate. Cost: contributors run a build to exercise local changes, and CI must build before test.
Build at install. Rejected: fragile across Node versions and platforms (CONTEXT.md notes Windows / Node 24 hazards) and slows every install.

Decision [Accepted]

Pursue type safety via a TS src/ tree compiled with tsc, adopting model 2 (build at publish): TS source is canonical, .cjs is a gitignored artifact. This dissolves the drift-policing machinery rather than building it.
Keep value baking separate. package-identity.cjs stays a checked-in baked artifact under its existing generator and parity test; the install-tree #378 constraint is unaffected by this ADR.
Migrate incrementally, lowest-coupling module first, behind one pilot PR that stands up the src/ tree + build wiring for a single module before any bulk move.
Reconcile the lint config to reality first. The 12-entry GENERATED_CJS_IGNORES list currently lies about hand-written files; it must be corrected (those files linted as hand-written) before, not after, a pipeline exists — otherwise the inconsistency masks the migration's progress.
Wire type-aware linting to the real tsconfig.json as modules become TS; retire tsconfig.lint.json only when the last hand-written .cjs is gone.

Consequences

Positive

Type-aware typescript-eslint rules apply to migrated runtime code.
No checked-in generated .cjs, so no drift invariant and no parity test to maintain for the transpiled surface (contrast: the rejected model 1).
One enforced answer to "hand-write or generate?" for new bin/lib code.

Negative

A build step now sits between editing src/*.ts and running bin/lib/*.cjs. Local dev and CI must build before exercising runtime behavior.
Migration touches ~83 files; each may surface latent type errors to fix.
Tooling that today reads bin/lib/*.cjs from a checkout (not an install) must build first or read from src/.

For testing

Tests importing bin/lib/*.cjs keep working only if the build has run; the test command must depend on the build. This is the main behavioral change versus today, where the .cjs is always present in the tree.

Rejected Alternatives

(a) Keep the split indefinitely — leaves the type-aware gap and the un-wired tsconfig.lint.json permanently. Rejected as a final state.
(b) Check in .ts + generated .cjs (model 1) — imports a drift invariant, parity tests, and dual commits for zero runtime benefit. Rejected in favor of build-at-publish.
(c) Full ESM rewrite — breaks require() consumers (no "type":"module" today); semver-major. Out of scope.
(d) Wire tsconfig.lint.json into ESLint and stop there — keeps the stopgap permanent and never delivers compile-level (vs lint-level) type errors. Rejected as the final state, but acceptable as an interim while the pilot proves out.

Open questions

Does any consumer rely on bin/lib/*.cjs being present in a raw (un-built) checkout? If so, build-at-publish needs a prepare-script bridge.
tsc CJS interop details (esModuleInterop, __importDefault shims) for the modules that re-require each other.
Whether the pilot should be a leaf utility or one of the 12 mislabeled GENERATED_CJS_IGNORES files (which already advertise themselves as generated).

References

ESLint harness prerequisite (accepted): 452-eslint-lint-harness.md
Test-rigor policies (accepted): 456-test-rigor-architecture.md
Single-runtime collapse (accepted): 0174-retire-gsd-sdk-package-boundary.md
Superseded shared-module seam: 3524-cjs-sdk-hard-seam.md
Tracking issue: #457
Value-baking precedent (distinct technique): scripts/generate-package-identity.cjs, tests/issue-498-package-identity.test.cjs

9.5 KiB Raw Blame History Unescape Escape

ADR 457: Generation model for bin/lib/*.cjs type safety [Accepted]