From cda1c572d9e04bebfc35186be64524dc104bd2e9 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 25 Jul 2026 19:03:32 -0700 Subject: [PATCH] Guard the OpenCode legacy migration against symlinks and home-rooted repos Both review bots caught real hazards in the migration: a symlinked ~/.opencode/skills (shared skill storage) would have its target emptied through the link, and in a home-rooted repo that path is a live project-scope install, not a stranded pre-#406 global copy. The migration now requires a real directory (lstat), compares the just-written dir by realpath instead of string, and skips entirely when the home dir is itself a repo. Two regression tests cover the symlink and dotfiles-repo shapes. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code --- cli/bin/commands/skills.mjs | 14 ++++++++++- tests/skills-cli.test.js | 47 +++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index 952a0a560..668130d92 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -1162,9 +1162,21 @@ function copyProviderSkills(bundleDir, root, targets, { scope } = {}) { // location OpenCode never reads. Now that the real copy sits in the // config dir, drop exactly the skills just written from the stranded // location; sibling skills and everything else in ~/.opencode stay. + // Guards (both flagged in review): a symlinked skills dir is shared + // storage whose target must not be emptied through the link, the + // just-written dir must be compared by realpath rather than string, + // and a home-rooted repo makes `.opencode/skills` a live + // project-scope install rather than a stranded global one. if (scope === 'user' && provider === '.opencode') { const legacyDir = join(root, '.opencode', 'skills'); - if (legacyDir !== localSkillsDir && existsSync(legacyDir)) { + let migratable = false; + try { + migratable = existsSync(legacyDir) + && !lstatSync(legacyDir).isSymbolicLink() + && realpathSync(legacyDir) !== realpathSync(localSkillsDir) + && !existsSync(join(root, '.git')); + } catch { migratable = false; } + if (migratable) { for (const skill of readdirSync(srcDir, { withFileTypes: true })) { if (!skill.isDirectory()) continue; rmSync(join(legacyDir, skill.name), { recursive: true, force: true }); diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index 8b8826cbc..05c33eb36 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -956,6 +956,53 @@ describe('skills install/update: local universal bundle e2e', () => { rmSync(home, { recursive: true, force: true }); }, 15000); + test('OpenCode migration never follows a symlinked legacy skills dir (#406)', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-oc-symlink-')); + const home = mkdtempSync(join(tmpdir(), 'imp-home-oc-symlink-')); + execSync('git init', { cwd: tmp }); + // Shared skill storage with ~/.opencode/skills symlinked at it. Deleting + // "the legacy copy" through the link would destroy the shared original. + writeSkill(join(home, '.config'), 'agents', 'impeccable'); + mkdirSync(join(home, '.opencode'), { recursive: true }); + symlinkSync(join(home, '.config', 'agents', 'skills'), join(home, '.opencode', 'skills'), 'dir'); + const bundleRoot = createFakeUniversalBundle(tmp, ['.opencode']); + const env = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot }; + delete env.OPENCODE_CONFIG_DIR; + delete env.XDG_CONFIG_HOME; + + run('skills install -y --providers=opencode --scope=global --no-hooks', { cwd: tmp, env }); + + expect(existsSync(join(home, '.config', 'opencode', 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + // The shared store behind the symlink is intact, link included. + expect(existsSync(join(home, '.config', 'agents', 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + expect(lstatSync(join(home, '.opencode', 'skills')).isSymbolicLink()).toBe(true); + + rmSync(tmp, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); + }, 15000); + + test('OpenCode migration leaves a home-rooted repo project install alone (#406)', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-oc-homerepo-')); + const home = mkdtempSync(join(tmpdir(), 'imp-home-oc-homerepo-')); + execSync('git init', { cwd: tmp }); + // The home dir IS a repo (dotfiles setup): .opencode/skills there is a + // live project-scope install, not a stranded pre-#406 global one. + execSync('git init', { cwd: home }); + writeSkill(home, '.opencode', 'impeccable'); + const bundleRoot = createFakeUniversalBundle(tmp, ['.opencode']); + const env = { ...process.env, HOME: home, IMPECCABLE_BUNDLE_PATH: bundleRoot }; + delete env.OPENCODE_CONFIG_DIR; + delete env.XDG_CONFIG_HOME; + + run('skills install -y --providers=opencode --scope=global --no-hooks', { cwd: tmp, env }); + + expect(existsSync(join(home, '.config', 'opencode', 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + expect(existsSync(join(home, '.opencode', 'skills', 'impeccable', 'SKILL.md'))).toBe(true); + + rmSync(tmp, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); + }, 15000); + test('global install detects OpenCode from ~/.config/opencode alone (#406)', () => { const tmp = mkdtempSync(join(tmpdir(), 'imp-test-oc-detect-')); const home = mkdtempSync(join(tmpdir(), 'imp-home-oc-detect-'));