From 9727e2ae9415dc890d62b0648f6eb1339c36f0e5 Mon Sep 17 00:00:00 2001 From: Joe Slitzker Date: Sat, 20 Jun 2026 13:46:30 -0500 Subject: [PATCH] fix(#1477): key getInstallExports cache per configDir; document marker contract Address second maintainer review on #1487: - Blocker 1: getInstallExports now caches per runtimeConfigDir (Map) so a no-arg warm-up via the legacy walk-up path cannot poison a later getInstallExports(configDir) call. Add a regression test (verified to fail on the singleton cache) proving the no-arg warm-up does not poison the configDir-keyed resolution. - Blocker 3: document the .gsd-source two-party provisioning contract in the CONTEXT.md Runtime Artifact Layout Module entry (writer, reader, content, guard, fall-through, package-root invariant, per-configDir cache). Claude-Session: https://claude.ai/code/session_01XNT3SWgzjmEycNuweURDme --- CONTEXT.md | 2 +- src/runtime-artifact-layout.cts | 20 ++++++++-- tests/runtime-artifact-layout.test.cjs | 52 ++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 71788c750..8d00e8d0e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -152,7 +152,7 @@ Module owning install detection for `/gsd:update`. `resolveUpdateContext({ home, Module owning which skills and agents are written to runtime config directories at install time (Phase 1) and at runtime via cluster-level toggles (Phase 2). Phase 1: `gsd-core/bin/lib/install-profiles.cjs` defines named profiles (`core`, `standard`, `full`), computes transitive closure over `requires:` frontmatter, stages skills/agents to runtime config dirs, and persists the chosen profile in a `.gsd-profile` marker. Profile resolution precedence: explicit `--profile=` flag > `.gsd-profile` marker > `full`. `--minimal`/`--core-only` are back-compat aliases for `--profile=core`. Phase 2: `gsd-core/bin/lib/surface.cjs` implements the `/gsd:surface` slash command for cluster-level enable/disable without reinstall; cluster definitions live in `gsd-core/bin/lib/clusters.cjs`; per-runtime state persists in `/.gsd-surface.json` independent from the `.gsd-profile` marker. See ADR-0011. ### Runtime Artifact Layout Module -Module owning the per-runtime mapping from artifact kind to filesystem placement. ADR-3660 defines the typed `kinds` per runtime (`commands`, `agents`, `skills`) with destination subpath, prefix, and stage adapter (with per-runtime converters in `bin/install.js`: `convertClaudeCommandToClaudeSkill`, `…CodexSkill`, `…CopilotSkill`, `…AntigravitySkill`). Owns the per-runtime `nested` skill-bundle decision (#69): a `skillsKind` flag in `src/runtime-artifact-layout.cts` drives whether a runtime receives the nested router layout (6 `gsd-ns-*` routers + concrete skills under `/skills//`) or the flat `skills/gsd-/` layout; the evidence/doc-link matrix is recorded in a comment above `resolveRuntimeArtifactLayout`. Phase 1 applies this seam to the Runtime Surface Module (`surface.cjs:applySurface`); as of #813, `applySurface` applies the same per-runtime skill-body path rewrites as `installRuntimeArtifacts` for `skills` kinds — re-surfacing no longer overwrites installed SKILL.md bodies with converter-default `~/.claude` paths. The shared accessor `getInstallExports` (exported from `runtime-artifact-layout.cjs`) is the single-source seam through which `surface.cjs` reaches `computePathPrefix` and `applyRuntimeContentRewritesInPlace`; the resolved `scope` (`'local'`|`'global'`) is now carried on the `Layout` object returned by `resolveRuntimeArtifactLayout` so `applySurface` derives the same `pathPrefix` (global `$HOME` form vs. absolute) as a fresh install. Phase 2 is planned to migrate install/uninstall in `bin/install.js` so all lifecycle sites iterate one shared layout table instead of re-encoding runtime layout logic. This design is intended to remove the #3659 class of omissions. Migrations remain under the Installer Migration Module (ADR-0008). See ADR-3660. +Module owning the per-runtime mapping from artifact kind to filesystem placement. ADR-3660 defines the typed `kinds` per runtime (`commands`, `agents`, `skills`) with destination subpath, prefix, and stage adapter (with per-runtime converters in `bin/install.js`: `convertClaudeCommandToClaudeSkill`, `…CodexSkill`, `…CopilotSkill`, `…AntigravitySkill`). Owns the per-runtime `nested` skill-bundle decision (#69): a `skillsKind` flag in `src/runtime-artifact-layout.cts` drives whether a runtime receives the nested router layout (6 `gsd-ns-*` routers + concrete skills under `/skills//`) or the flat `skills/gsd-/` layout; the evidence/doc-link matrix is recorded in a comment above `resolveRuntimeArtifactLayout`. Phase 1 applies this seam to the Runtime Surface Module (`surface.cjs:applySurface`); as of #813, `applySurface` applies the same per-runtime skill-body path rewrites as `installRuntimeArtifacts` for `skills` kinds — re-surfacing no longer overwrites installed SKILL.md bodies with converter-default `~/.claude` paths. The shared accessor `getInstallExports` (exported from `runtime-artifact-layout.cjs`) is the single-source seam through which `surface.cjs` reaches `computePathPrefix` and `applyRuntimeContentRewritesInPlace`; the resolved `scope` (`'local'`|`'global'`) is now carried on the `Layout` object returned by `resolveRuntimeArtifactLayout` so `applySurface` derives the same `pathPrefix` (global `$HOME` form vs. absolute) as a fresh install. Phase 2 is planned to migrate install/uninstall in `bin/install.js` so all lifecycle sites iterate one shared layout table instead of re-encoding runtime layout logic. This design is intended to remove the #3659 class of omissions. Migrations remain under the Installer Migration Module (ADR-0008). The `.gsd-source` marker (#1477) is a two-party provisioning contract that lets source resolution succeed on the Claude global skills layout, which ships `gsd-core/{bin,contexts,references,templates,workflows}` but no `commands/gsd` source tree for `findInstallSourceRoot` to walk up to: the writer is `bin/install.js`, which writes `/.gsd-source` (content: the absolute path to its own `commands/gsd`, terminated by a newline) when `runtime === 'claude' && isGlobal`, guarded by `fs.existsSync` so a half-published package never writes a dangling marker; the reader is `findInstallSourceRoot(configDir)`, which prefers the marker over its walk-up but falls through to the walk-up if the marker is absent, dangling, or empty/whitespace-only. The marker's package root must also hold `bin/install.js` — `getInstallExports(configDir)` derives the installer-exports path from the resolved source root (`/commands/gsd` → `/bin/install.js`), and caches per `configDir` so a no-arg warm-up call cannot poison later marker-aware resolution. See ADR-3660. ### Runtime Artifact Conversion Module Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index a49738afd..7eb726dc5 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -86,11 +86,23 @@ function loadInstallExports(runtimeConfigDir?: string): InstallExports { } } -/** Cache after first successful load. */ -let _installExports: InstallExports | null = null; +/** + * Cache after first successful load, keyed on runtimeConfigDir. The derived + * install.js path depends on the configDir (marker-aware in a deployed layout + * vs. walk-up in the repo), so a single module-level singleton would let a + * no-arg warm-up call (legacy relative path) poison every later + * getInstallExports(configDir) call. Keying on the arg keeps each layout's + * resolution independent. The empty string stands in for the no-arg case. + */ +const _installExportsByConfigDir = new Map(); function getInstallExports(runtimeConfigDir?: string): InstallExports { - if (!_installExports) _installExports = loadInstallExports(runtimeConfigDir); - return _installExports; + const key = runtimeConfigDir ?? ''; + let exports = _installExportsByConfigDir.get(key); + if (!exports) { + exports = loadInstallExports(runtimeConfigDir); + _installExportsByConfigDir.set(key, exports); + } + return exports; } // --------------------------------------------------------------------------- diff --git a/tests/runtime-artifact-layout.test.cjs b/tests/runtime-artifact-layout.test.cjs index c3ba616e8..4c3dc4c93 100644 --- a/tests/runtime-artifact-layout.test.cjs +++ b/tests/runtime-artifact-layout.test.cjs @@ -775,6 +775,58 @@ describe('#1477 .gsd-source marker provisioning + deployed install-exports resol 'getInstallExports must expose applyRuntimeContentRewritesInPlace (used by applySurface)'); }); + // ── Failure 2 cache correctness: getInstallExports keys on runtimeConfigDir ── + // A module-level singleton cache would let a no-arg warm-up call (legacy + // walk-up path) poison every later getInstallExports(configDir) call — + // applySurface would then load the wrong install.js. Proves the cache is + // keyed per configDir so the marker-derived path always wins for its key. + test('getInstallExports caches per configDir — a no-arg warm-up does not poison a later configDir call', () => { + // A standalone package whose commands/gsd marker derives a sibling + // bin/install.js exporting a sentinel that the real repo install.js lacks. + const pkgRoot = path.join(tmpRoot, 'sentinel-pkg'); + fs.mkdirSync(path.join(pkgRoot, 'commands', 'gsd'), { recursive: true }); + fs.mkdirSync(path.join(pkgRoot, 'bin'), { recursive: true }); + fs.writeFileSync( + path.join(pkgRoot, 'bin', 'install.js'), + "module.exports = { sentinel: 'PKG', computePathPrefix: () => '', applyRuntimeContentRewritesInPlace: () => {} };\n", + 'utf8', + ); + const cfgDir = path.join(tmpRoot, 'sentinel-cfg'); + fs.mkdirSync(cfgDir, { recursive: true }); + fs.writeFileSync( + path.join(cfgDir, '.gsd-source'), + path.join(pkgRoot, 'commands', 'gsd') + '\n', + 'utf8', + ); + + // Fresh module instance so the per-key cache starts empty for this test. + const layoutPath = require.resolve('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + const savedModule = require.cache[layoutPath]; + delete require.cache[layoutPath]; + try { + const fresh = require(layoutPath); + + // Warm the cache via the no-arg legacy walk-up path (resolves the real + // repo bin/install.js, which has no `sentinel`). + const warm = fresh.getInstallExports(); + assert.equal(warm.sentinel, undefined, 'no-arg path resolves the repo install.js'); + + // The configDir call must re-derive from the marker, NOT return the + // cached no-arg result. A singleton cache would return `warm` here. + const derived = fresh.getInstallExports(cfgDir); + assert.equal(derived.sentinel, 'PKG', + 'no-arg warm-up must not poison the configDir-keyed resolution'); + + // Re-querying the same key returns its cached instance, not a re-derive. + assert.strictEqual(fresh.getInstallExports(cfgDir), derived, + 'same configDir key must return the cached instance'); + } finally { + // Restore the original shared module instance for later tests. + if (savedModule) require.cache[layoutPath] = savedModule; + else delete require.cache[layoutPath]; + } + }); + // ── Adversarial marker-reader cases (no full install needed) ───────────────── describe('findInstallSourceRoot marker handling', () => { let cfgDir;