From 8c1962200d98d238738c3ccc02a1ffca9a7f1cff Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 4 Aug 2026 14:31:07 -0400 Subject: [PATCH] fix(#2911): resolve surface re-stage destinations the way the installer does (#3049) * fix(#2911): resolve surface re-stage destinations the way the installer does Two writers computed the same destination differently. The installer honors a skills-kind home override; the surface re-stage ignored it and always resolved against configDir. For a global Codex install that override points at $HOME/.agents, so every re-stage built a second GSD-managed skill tree under $CODEX_HOME alongside the correct one, with nothing indicating which was live. Honors the override as a fallback, never a replacement -- runtimes without one still resolve against configDir, which is most of them. The real deliverable is the parity test, not the one-expression fix: it walks every runtime in the registry across both scopes, computes the installer and surface destinations, and fails naming the runtime if they ever disagree. Today only Codex global carries an override, so it discriminates on exactly one runtime -- stating that plainly rather than implying broader coverage -- but it is derived from the registry, so a newly-added runtime is covered without anyone remembering to add it. Two further defects fixed rather than deferred: - The legacy dev-preferences migration carried the identical defect, which the issue flagged as a latent instance of the same shape. - Fixing it exposed a symlink-escape guard confined against the wrong root: it checked the span between configDir and the skill dir, but a home override moves the skill dir outside configDir entirely, so the span was meaningless and threw a false-positive escape. Now confined against the install root the destination actually resolves under. The guard is unchanged in strength and still honors its opt-in; only the root it measures from is corrected. Co-Authored-By: Claude Opus 5 * fix(#2911): honor the home override in the fourth destination writer too Adversarial review found a writer the fix had missed: the opencode-family skills installer resolved its destination and its symlink guard against targetDir, never consulting the skills-kind home override, while its three siblings all already honored it. Pre-existing and currently dormant -- it is reachable only for the combined-family runtimes, and none of them declares an override today, so no user is affected right now. Fixed anyway rather than left as a latent instance of the same shape, which is exactly what this issue asked for in the case of the legacy migration. Mirrors the shape used for the other three: a single installRoot local that both the destination and the guard derive from, so the two cannot drift apart. The guard's message now names the root it actually confined against. Coverage extended to this writer and proven non-theatre: reverting the change in a scratch build makes it fail for both combined-family runtimes. Enumerated every remaining site that computes a destination from destSubpath or calls the confinement helper -- install and uninstall paths, the surface module, the read-side skills-root reporter. All honor the override or structurally cannot express one. No fifth defect. The one adjacent shape, the flat command directory, reads a different descriptor field that no kind declares an override for in the current schema; noted rather than papered over with a fallback for a field that cannot exist. Verified no behavior change for the affected runtimes today: normalized file-tree hashes before and after are identical. Co-Authored-By: Claude Opus 5 * chore(#2911): backfill changeset pr number Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .changeset/mellow-eagles-rally.md | 5 + src/install-engine.cts | 39 ++- src/surface.cts | 12 +- tests/install-runtime-artifacts.test.cjs | 74 +++++ .../runtime-artifact-layout-surface.test.cjs | 305 ++++++++++++++++++ 5 files changed, 424 insertions(+), 11 deletions(-) create mode 100644 .changeset/mellow-eagles-rally.md diff --git a/.changeset/mellow-eagles-rally.md b/.changeset/mellow-eagles-rally.md new file mode 100644 index 000000000..70caebcf7 --- /dev/null +++ b/.changeset/mellow-eagles-rally.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3049 +--- +**A Codex surface re-stage no longer creates a duplicate skill tree** — re-staging skills on a global Codex install wrote them to `$CODEX_HOME/skills` while the installer had correctly placed them in `$HOME/.agents/skills`, leaving two active GSD skill trees and no signal which one was live. The re-stage and the legacy dev-preferences migration now resolve the same destination the installer uses. (#2911) diff --git a/src/install-engine.cts b/src/install-engine.cts index 213e0a804..cf37038aa 100644 --- a/src/install-engine.cts +++ b/src/install-engine.cts @@ -343,24 +343,36 @@ function hasExistingSymlinkBetween( function migrateLegacyDevPreferencesToSkill(targetDir: string, saved: Map, runtime?: string, scope: string = 'global'): boolean { if (!saved || !saved.has('dev-preferences.md')) return false; let skillDir: string; + // #2911: the actual install root the skill dir resolves under — defaults to + // targetDir, but a skills-kind `home` override (e.g. Codex -> $HOME/.agents) + // moves it entirely outside targetDir. Every confinement/guard check below + // must confine against installRoot, not targetDir, or it would flag the + // legitimate override destination as an escape. + let installRoot: string = targetDir; if (runtime) { const layout: any = runtimeArtifactLayout.resolveRuntimeArtifactLayout(runtime, targetDir, scope as any); const skillsKindEntry = layout.kinds.find((k: any) => k.kind === 'skills'); if (!skillsKindEntry) return false; // runtime has no skills layout at this scope (e.g. cline local) const stemName = skillsKindEntry.prefix === '' ? 'dev-preferences' : 'gsd-dev-preferences'; - skillDir = path.join(runtimeArtifactInstallPlan.assertDestWithinConfigHome(targetDir, skillsKindEntry.destSubpath), stemName); + // #2911: same destination-root defect as _copyStaged/applySurface — honor + // skillsKindEntry.home as a FALLBACK-preferred override (e.g. Codex skills + // -> $HOME/.agents) instead of always resolving against targetDir, so a + // legacy dev-preferences migration lands in the SAME tree the installer + // and surface-apply use. Runtimes with no `home` override are unaffected. + installRoot = skillsKindEntry.home ?? targetDir; + skillDir = path.join(runtimeArtifactInstallPlan.assertDestWithinConfigHome(installRoot, skillsKindEntry.destSubpath), stemName); } else { // Legacy fallback for callers that have not yet been updated to pass runtime skillDir = path.join(runtimeArtifactInstallPlan.assertDestWithinConfigHome(targetDir, 'skills'), 'gsd-dev-preferences'); } const skillFile = path.join(skillDir, 'SKILL.md'); if (fs.existsSync(skillFile)) return false; - // Symlink-escape guard: reject if any path component between targetDir and - // skillDir is a symlink that would redirect writes outside the config root. + // Symlink-escape guard: reject if any path component between installRoot and + // skillDir is a symlink that would redirect writes outside the install root. // #2393: honor GSD_ALLOW_SYMLINKED_DEST for intentional user-owned symlink layouts. - if (hasExistingSymlinkBetween(path.resolve(targetDir), skillDir, { allowOptInFollow: isSymlinkedDestOptIn() })) { + if (hasExistingSymlinkBetween(path.resolve(installRoot), skillDir, { allowOptInFollow: isSymlinkedDestOptIn() })) { throw new Error( - `migrateLegacyDevPreferencesToSkill: skillDir "${skillDir}" contains a symlink the install root "${targetDir}" does not trust — refusing to write. If this is an intentional user-owned symlink layout, re-run with GSD_ALLOW_SYMLINKED_DEST=1.`, + `migrateLegacyDevPreferencesToSkill: skillDir "${skillDir}" contains a symlink the install root "${installRoot}" does not trust — refusing to write. If this is an intentional user-owned symlink layout, re-run with GSD_ALLOW_SYMLINKED_DEST=1.`, ); } try { @@ -920,13 +932,20 @@ function installOpencodeFamilySkills( ); } - const dest = runtimeArtifactInstallPlan.assertDestWithinConfigHome(targetDir, skillsKindEntry.destSubpath); - // Symlink-escape guard: reject if any path component between targetDir and - // dest is a symlink that would redirect writes outside the config root. + // #2911: same destination-root defect as _copyStaged/migrateLegacyDevPreferencesToSkill + // — honor skillsKindEntry.home as a FALLBACK-preferred override (e.g. Codex skills + // -> $HOME/.agents) instead of always resolving against targetDir, so this bespoke + // OpenCode/Kilo writer lands in the SAME tree the installer and surface-apply use. + // Runtimes with no `home` override (opencode, kilo today) are unaffected. Must stay + // in lockstep with the sibling writers — the destination-parity test enforces it. + const installRoot: string = skillsKindEntry.home ?? targetDir; + const dest = runtimeArtifactInstallPlan.assertDestWithinConfigHome(installRoot, skillsKindEntry.destSubpath); + // Symlink-escape guard: reject if any path component between installRoot and + // dest is a symlink that would redirect writes outside the install root. // #2393: honor GSD_ALLOW_SYMLINKED_DEST for intentional user-owned symlink layouts. - if (hasExistingSymlinkBetween(path.resolve(targetDir), dest, { allowOptInFollow: isSymlinkedDestOptIn() })) { + if (hasExistingSymlinkBetween(path.resolve(installRoot), dest, { allowOptInFollow: isSymlinkedDestOptIn() })) { throw new Error( - `installOpencodeFamilySkills: destDir "${dest}" contains a symlink the install root "${targetDir}" does not trust — refusing to write. If this is an intentional user-owned symlink layout, re-run with GSD_ALLOW_SYMLINKED_DEST=1.`, + `installOpencodeFamilySkills: destDir "${dest}" contains a symlink the install root "${installRoot}" does not trust — refusing to write. If this is an intentional user-owned symlink layout, re-run with GSD_ALLOW_SYMLINKED_DEST=1.`, ); } fs.mkdirSync(dest, { recursive: true }); diff --git a/src/surface.cts b/src/surface.cts index aebe78abf..a12117f9c 100644 --- a/src/surface.cts +++ b/src/surface.cts @@ -72,6 +72,10 @@ interface ArtifactKind { kind: string; destSubpath: string; prefix: string; + // #2911: optional install-root override (e.g. Codex skills -> $HOME/.agents), + // set by resolveRuntimeArtifactLayout's dispatchKindEntry. Must be honored as + // a FALLBACK by every destination-computation call site — see applySurface. + home?: string; stage: (resolvedProfile: { name: string; skills: Set | '*'; agents: Set }, agentCtx?: AgentCtx) => string; } @@ -415,7 +419,13 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map $HOME/.agents), never a blanket replacement — kinds without + // a `home` must keep resolving against layout.configDir. This must stay + // in lockstep with _copyStaged's root selection in src/install-engine.cts; + // the parity test in tests/runtime-artifact-layout-surface.test.cjs + // enforces that the two writers never diverge again. + const dest = assertDestWithinConfigHome(kind.home ?? layout.configDir, kind.destSubpath); _syncGsdDir(staged, dest, kind, skillManifest, layout.runtime); } } finally { diff --git a/tests/install-runtime-artifacts.test.cjs b/tests/install-runtime-artifacts.test.cjs index ff7489589..c95f3cf1c 100644 --- a/tests/install-runtime-artifacts.test.cjs +++ b/tests/install-runtime-artifacts.test.cjs @@ -4762,6 +4762,80 @@ describe('Bug #2973: installer migrates existing legacy dev-preferences.md to sk }); }); +// ─── #2911: migrateLegacyDevPreferencesToSkill is the latent instance of the ── +// ─── same installer-vs-surface destination-root defect ─────────────────────── +// +// migrateLegacyDevPreferencesToSkill (src/install-engine.cts) resolved the +// skill dir as `assertDestWithinConfigHome(targetDir, skillsKindEntry.destSubpath)` +// — always against targetDir (configDir), ignoring skillsKindEntry.home. For +// codex/global (the only current `home`-override runtime/scope), a legacy +// dev-preferences.md migration would have landed under $CODEX_HOME/skills +// instead of the canonical $HOME/.agents/skills tree used by both the +// installer's _copyStaged and (post-#2911-fix) applySurface. Fixed the same +// way: `skillsKindEntry.home ?? targetDir`. +describe('Bug #2911: migrateLegacyDevPreferencesToSkill honors the skills-kind home override (codex)', () => { + const { resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + + function withFakeHome(fakeHome, fn) { + const savedHome = process.env.HOME; + const savedUserProfile = process.env.USERPROFILE; + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + try { + return fn(); + } finally { + if (savedHome === undefined) delete process.env.HOME; else process.env.HOME = savedHome; + if (savedUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = savedUserProfile; + } + } + + test('codex + global: migration writes SKILL.md under $HOME/.agents/skills, NOT under $CODEX_HOME/skills', () => { + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-mig-home-')); + const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-mig-codexhome-')); + try { + withFakeHome(fakeHome, () => { + const inst = installEngine; + const layout = resolveRuntimeArtifactLayout('codex', codexHome, 'global'); + const skillsKindEntry = layout.kinds.find((k) => k.kind === 'skills'); + assert.equal(skillsKindEntry.home, path.join(fakeHome, '.agents'), 'pre-condition: codex global skills kind declares the $HOME/.agents override'); + + const saved = new Map([['dev-preferences.md', '# my legacy preferences\n']]); + const migrated = inst.migrateLegacyDevPreferencesToSkill(codexHome, saved, 'codex', 'global'); + assert.equal(migrated, true, 'expected migration to succeed when no SKILL.md exists'); + + const correctSkillFile = path.join(fakeHome, '.agents', 'skills', 'gsd-dev-preferences', 'SKILL.md'); + assert.equal(fs.existsSync(correctSkillFile), true, `expected SKILL.md at ${correctSkillFile}`); + assert.equal(fs.readFileSync(correctSkillFile, 'utf-8'), '# my legacy preferences\n'); + + const legacySkillFile = path.join(codexHome, 'skills', 'gsd-dev-preferences', 'SKILL.md'); + assert.equal(fs.existsSync(legacySkillFile), false, `migration must NOT also write a second copy at the legacy location ${legacySkillFile}`); + }); + } finally { + cleanup(fakeHome); + cleanup(codexHome); + } + }); + + test('codex + local: no home override — migration destination is unchanged ($CODEX_HOME/skills)', () => { + const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-mig-local-')); + try { + const inst = installEngine; + const layout = resolveRuntimeArtifactLayout('codex', codexHome, 'local'); + const skillsKindEntry = layout.kinds.find((k) => k.kind === 'skills'); + assert.equal(skillsKindEntry.home, undefined, 'pre-condition: codex local scope declares NO home override'); + + const saved = new Map([['dev-preferences.md', '# my legacy preferences\n']]); + const migrated = inst.migrateLegacyDevPreferencesToSkill(codexHome, saved, 'codex', 'local'); + assert.equal(migrated, true); + + const skillFile = path.join(codexHome, 'skills', 'gsd-dev-preferences', 'SKILL.md'); + assert.equal(fs.existsSync(skillFile), true, `expected SKILL.md at ${skillFile} (unchanged, no home override)`); + } finally { + cleanup(codexHome); + } + }); +}); + // ─── #3003 CR follow-up: installRuntimeArtifacts preserves user-owned skills ── // // Production install() calls installRuntimeArtifacts() without a prior diff --git a/tests/runtime-artifact-layout-surface.test.cjs b/tests/runtime-artifact-layout-surface.test.cjs index 80a090361..23f731705 100644 --- a/tests/runtime-artifact-layout-surface.test.cjs +++ b/tests/runtime-artifact-layout-surface.test.cjs @@ -1110,3 +1110,308 @@ describe('applySurface — commands kind path rewrite (#1615 adversarial review) } }); }); + +// ─── #2911: skills-kind destination parity (installer vs surface-apply) ────── +// +// _copyStaged (src/install-engine.cts) resolves the write root as +// `kind.home ?? layout.configDir`. Before the #2911 fix, applySurface's +// _syncGsdDir call always resolved against `layout.configDir`, ignoring +// `kind.home`. For runtimes whose skills kind declares a `home` override +// (currently only codex: home='.agents' at GLOBAL scope — see +// capabilities/codex/capability.json), a fresh install landed skills under +// the override root while a surface re-stage created a SECOND tree under +// the runtime's own configDir. +// +// These tests exercise the REAL applySurface code path (not a hand-copy of +// its destination formula) so a future regression that re-diverges the two +// writers is caught by actual file placement, not by a self-consistent +// re-derivation of the (possibly still-buggy) formula. +describe('skills-kind destination parity: installer vs surface-apply (#2911)', () => { + const runtimeArtifactInstallPlan = require('../gsd-core/bin/lib/runtime-artifact-install-plan.cjs'); + const capabilityRegistry = require('../gsd-core/bin/lib/capability-registry.cjs'); + + const RUNTIME_IDS = Object.keys(capabilityRegistry.runtimes); + const PARITY_MANIFEST = loadSkillsManifest(REAL_COMMANDS_DIR); + + // os.homedir() on POSIX/Windows reads HOME/USERPROFILE from the environment + // (Node docs), so redirecting it here for the duration of a test is safe and + // avoids ever touching the real developer home directory even though the + // only current `home`-override runtime (codex) resolves via os.homedir(). + function withFakeHome(fakeHome, fn) { + const savedHome = process.env.HOME; + const savedUserProfile = process.env.USERPROFILE; + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + try { + return fn(); + } finally { + if (savedHome === undefined) delete process.env.HOME; else process.env.HOME = savedHome; + if (savedUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = savedUserProfile; + } + } + + // Runtimes whose registry entry declares a `home` override on any kind — + // stated explicitly per the #2911 acceptance criteria, not hidden. Recomputed + // from the registry (not hardcoded) so a newly-added override is picked up. + function runtimesWithHomeOverride() { + const found = []; + for (const runtime of RUNTIME_IDS) { + for (const scope of ['global', 'local']) { + let layout; + try { + layout = resolveRuntimeArtifactLayout(runtime, '/tmp/fake-config-dir-2911', scope); + } catch { + continue; + } + if (layout.kinds.some((k) => typeof k.home === 'string' && k.home !== '')) { + found.push(`${runtime}/${scope}`); + } + } + } + return found; + } + + test('registry home-override discrimination report (#2911)', () => { + const overrides = runtimesWithHomeOverride(); + // Stated per the brief: at time of writing only codex/global has a `home` + // override, so this parity test discriminates on exactly one runtime/scope + // pair. This assertion documents that fact and fails loudly if the set + // ever changes shape unexpectedly empty (a discrimination-less parity + // test would be silently vacuous). + assert.ok(overrides.length > 0, 'expected at least one runtime/scope with a home override (codex/global)'); + assert.deepStrictEqual(overrides, ['codex/global'], `home-override set changed — update this test's documentation. Found: ${overrides.join(', ')}`); + }); + + for (const scope of ['global', 'local']) { + test(`applySurface writes skills-kind output to the SAME destination the installer would use, for every runtime in the registry (${scope})`, (t) => { + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-fakehome-')); + t.after(() => cleanup(fakeHome)); + + const failures = []; + let discriminatingRuntimes = 0; + + for (const runtime of RUNTIME_IDS) { + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), `gsd-2911-parity-${runtime}-${scope}-`)); + t.after(() => cleanup(configDir)); + + withFakeHome(fakeHome, () => { + let layout; + try { + layout = resolveRuntimeArtifactLayout(runtime, configDir, scope); + } catch { + return; // runtime/scope combination not supported + } + const skillsKind = layout.kinds.find((k) => k.kind === 'skills'); + if (!skillsKind) return; // e.g. cline/kimi at local scope: no skills kind + + if (typeof skillsKind.home === 'string' && skillsKind.home !== '') { + discriminatingRuntimes++; + } + + writeActiveProfile(configDir, 'core'); + writeSurface(configDir, { + baseProfile: 'core', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + + applySurface(configDir, layout, PARITY_MANIFEST, CLUSTERS); + + // The installer's REAL destination-selection formula (verbatim from + // createRuntimeArtifactInstallPlan / _copyStaged): honor kind.home as + // a FALLBACK-preferred override, else configDir. + const installerDest = runtimeArtifactInstallPlan.assertDestWithinConfigHome( + skillsKind.home ?? layout.configDir, + skillsKind.destSubpath, + ); + + if (!fs.existsSync(installerDest) || fs.readdirSync(installerDest).length === 0) { + failures.push( + `${runtime}/${scope}: applySurface did NOT write skills-kind output to the installer's ` + + `destination "${installerDest}" — surface-apply and installer destination formulas have diverged.`, + ); + return; + } + + // If the runtime declares a home override, the OLD buggy root + // (configDir/destSubpath) must not have ALSO been populated — + // otherwise a re-stage creates a second, stale tree (#2911 symptom). + if (typeof skillsKind.home === 'string' && skillsKind.home !== '') { + const legacyDest = path.join(layout.configDir, skillsKind.destSubpath); + if (legacyDest !== installerDest && fs.existsSync(legacyDest) && fs.readdirSync(legacyDest).length > 0) { + failures.push( + `${runtime}/${scope}: applySurface ALSO wrote a second tree at the legacy location ` + + `"${legacyDest}" (kind.home override "${skillsKind.home}" was not honored consistently).`, + ); + } + } + }); + } + + assert.deepStrictEqual(failures, [], `#2911 parity failures (${scope}):\n${failures.join('\n')}`); + // Not a hard requirement, but surfaces how many runtime/scope pairs this + // run actually discriminated on (i.e. exercised a non-trivial home + // override), matching the acceptance-criteria ask to state this plainly. + t.diagnostic(`${scope}: ${discriminatingRuntimes} runtime(s) discriminated on a home override`); + }); + } +}); + +// ─── #2911: codex-specific regression (skills-kind home override) ─────────── +describe('codex skills-kind destination: home override (#2911)', () => { + const runtimeArtifactInstallPlan = require('../gsd-core/bin/lib/runtime-artifact-install-plan.cjs'); + + function withFakeHome(fakeHome, fn) { + const savedHome = process.env.HOME; + const savedUserProfile = process.env.USERPROFILE; + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + try { + return fn(); + } finally { + if (savedHome === undefined) delete process.env.HOME; else process.env.HOME = savedHome; + if (savedUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = savedUserProfile; + } + } + + test('codex + global: applySurface stages skills under $HOME/.agents, NOT under $CODEX_HOME (#2911 AC)', (t) => { + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-codex-home-')); + t.after(() => cleanup(fakeHome)); + const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-codex-home-dir-')); + t.after(() => cleanup(codexHome)); + + withFakeHome(fakeHome, () => { + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + writeActiveProfile(codexHome, 'core'); + writeSurface(codexHome, { + baseProfile: 'core', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + const layout = resolveRuntimeArtifactLayout('codex', codexHome, 'global'); + const skillsKind = layout.kinds.find((k) => k.kind === 'skills'); + assert.ok(skillsKind, 'pre-condition: codex global layout has a skills kind'); + assert.strictEqual(skillsKind.home, path.join(fakeHome, '.agents'), 'pre-condition: codex skills kind declares the $HOME/.agents override'); + + applySurface(codexHome, layout, manifest, CLUSTERS); + + const expectedDest = runtimeArtifactInstallPlan.assertDestWithinConfigHome( + path.join(fakeHome, '.agents'), + skillsKind.destSubpath, + ); + assert.ok( + fs.existsSync(expectedDest) && fs.readdirSync(expectedDest).length > 0, + `#2911: codex global surface-apply must stage skills under $HOME/.agents (expected non-empty dir at ${expectedDest})`, + ); + + const wrongDest = path.join(codexHome, skillsKind.destSubpath); + assert.ok( + !fs.existsSync(wrongDest) || fs.readdirSync(wrongDest).length === 0, + `#2911: codex global surface-apply must NOT create a second skills tree under $CODEX_HOME (found populated dir at ${wrongDest})`, + ); + }); + }); + + test('codex + local: no home override — surface-apply destination is unchanged (#2911 AC3)', (t) => { + const codexHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2911-codex-local-')); + t.after(() => cleanup(codexHome)); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + writeActiveProfile(codexHome, 'core'); + writeSurface(codexHome, { + baseProfile: 'core', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + const layout = resolveRuntimeArtifactLayout('codex', codexHome, 'local'); + const skillsKind = layout.kinds.find((k) => k.kind === 'skills'); + assert.ok(skillsKind, 'pre-condition: codex local layout has a skills kind'); + assert.strictEqual(skillsKind.home, undefined, 'pre-condition (AC3): codex local scope declares NO home override'); + + applySurface(codexHome, layout, manifest, CLUSTERS); + + const expectedDest = runtimeArtifactInstallPlan.assertDestWithinConfigHome(codexHome, skillsKind.destSubpath); + assert.ok( + fs.existsSync(expectedDest) && fs.readdirSync(expectedDest).length > 0, + `#2911 AC3: codex local surface-apply must stage skills under $CODEX_HOME (expected non-empty dir at ${expectedDest})`, + ); + }); +}); + +// ─── installOpencodeFamilySkills destination parity (#2911 sibling coverage) ─ +// +// installOpencodeFamilySkills (src/install-engine.cts) is a FOURTH destination- +// computation writer, alongside _copyStaged, the inline guard in +// installRuntimeArtifacts, and createRuntimeArtifactUninstallPlan — all three of +// which honor `skillsKindEntry.home ?? `. This writer originally did +// not, and would silently reproduce the #2911 duplicate-tree symptom the moment +// any combined-family runtime (opencode, kilo) gains a `home` override. Neither +// declares one today, so this test exercises the REAL production code path +// (installOpencodeFamilySkills, via the module-ref call convention documented at +// src/install-engine.cts:37-38) under a synthetic `home` override injected by +// monkeypatching resolveRuntimeArtifactLayout's shared module export — the same +// object install-engine.cjs calls through at runtime — rather than re-deriving +// the destination formula by hand. This proves discrimination even though no +// registry runtime exercises it yet, and will catch a future divergence the +// moment a combined-family runtime's descriptor grows a `home` override. +describe('installOpencodeFamilySkills destination parity (#2911 sibling coverage)', () => { + const installEngine = require('../gsd-core/bin/lib/install-engine.cjs'); + const runtimeArtifactLayoutModule = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + + function stageRawCommands(runtime, configDir) { + const layout = resolveRuntimeArtifactLayout(runtime, configDir, 'global'); + const commandsKind = layout.kinds.find((k) => k.kind === 'commands'); + return commandsKind.stage(resolveProfile({ modes: ['core'], manifest: loadSkillsManifest(REAL_COMMANDS_DIR) })); + } + + for (const runtime of ['opencode', 'kilo']) { + test(`${runtime}: installOpencodeFamilySkills honors a skills-kind home override instead of always resolving against configDir (#2911 sibling)`, (t) => { + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), `gsd-2911-ocfs-${runtime}-`)); + const fakeHomeOverride = fs.mkdtempSync(path.join(os.tmpdir(), `gsd-2911-ocfs-home-${runtime}-`)); + t.after(() => { cleanup(configDir); cleanup(fakeHomeOverride); }); + + const originalResolve = runtimeArtifactLayoutModule.resolveRuntimeArtifactLayout; + // Capture the real destSubpath before patching so the assertion below + // never hardcodes a literal path fragment. + const realLayout = originalResolve(runtime, configDir, 'global'); + const skillsKindReal = realLayout.kinds.find((k) => k.kind === 'skills'); + assert.ok(skillsKindReal, `pre-condition: ${runtime} global layout has a skills kind`); + + runtimeArtifactLayoutModule.resolveRuntimeArtifactLayout = function (rt, targetDir, scope) { + const layout = originalResolve(rt, targetDir, scope); + if (rt === runtime) { + const patchedKinds = layout.kinds.map((k) => (k.kind === 'skills' ? { ...k, home: fakeHomeOverride } : k)); + return { ...layout, kinds: patchedKinds }; + } + return layout; + }; + + try { + const raw = stageRawCommands(runtime, configDir); + const count = installEngine.installOpencodeFamilySkills(runtime, configDir, raw, `${configDir}/`); + assert.ok(count >= 1, `${runtime}: installOpencodeFamilySkills should report installed skills`); + + const overrideDest = path.join(fakeHomeOverride, skillsKindReal.destSubpath); + assert.ok( + fs.existsSync(overrideDest) && fs.readdirSync(overrideDest).length > 0, + `${runtime}: installOpencodeFamilySkills must write skills-kind output under the home override ` + + `"${overrideDest}" — it must not always resolve against configDir.`, + ); + + // If the home override is not honored, output lands under the legacy + // configDir/destSubpath location instead (the #2911 duplicate-tree symptom). + const legacyDest = path.join(configDir, skillsKindReal.destSubpath); + assert.ok( + !fs.existsSync(legacyDest) || fs.readdirSync(legacyDest).length === 0, + `${runtime}: installOpencodeFamilySkills must NOT ALSO write a second tree at the legacy location ` + + `"${legacyDest}" once a home override is declared.`, + ); + } finally { + runtimeArtifactLayoutModule.resolveRuntimeArtifactLayout = originalResolve; + } + }); + } +});