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
This commit is contained in:
@@ -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 `<runtimeConfigDir>/.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 `<router>/skills/<name>/`) or the flat `skills/gsd-<stem>/` 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 `<router>/skills/<name>/`) or the flat `skills/gsd-<stem>/` 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 `<configDir>/.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 (`<packageRoot>/commands/gsd` → `<packageRoot>/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.
|
||||
|
||||
@@ -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<string, InstallExports>();
|
||||
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;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user