* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * chore(#2911): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user