diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index c50dc153e..36c8c1ead 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -1010,6 +1010,36 @@ async function chooseInstallPlan(projectRoot, flags, { yes } = {}) { return { targets, scope, installRoot, hookRoot: projectRoot, detections }; } +/** + * Whether `localSkillsDir` is a symlink that points at ANOTHER in-project + * provider's skills dir (e.g. `.claude/skills -> ../.agents/skills`, the shape a + * prior `npx skills` install can leave behind). Only these get dropped so each + * provider can receive its own compiled variant. A symlink to anywhere else - + * notably a user's external shared skills dir (`~/.claude/skills -> + * ~/.config/agents/skills`) - is preserved and written through. See issue #295. + */ +function isInProjectProviderLink(localSkillsDir, root, provider) { + let target; + try { + if (!lstatSync(localSkillsDir).isSymbolicLink()) return false; + target = readlinkSync(localSkillsDir); + } catch { + return false; // not a symlink, or unreadable + } + // Resolve the link's TARGET lexically against the link's own directory. We + // deliberately do NOT realpathSync the target: + // * it lets a not-yet-created in-project target still match, so a dangling + // `.claude/skills -> ../.agents/skills` is still dropped; + // * it compares the ACTUAL target, not a shared realpath, so two providers + // pointing at the SAME external dir are never misread as in-project. + const resolvedTarget = resolve(dirname(localSkillsDir), target); + for (const other of PROVIDER_DIRS) { + if (other === provider) continue; + if (resolvedTarget === join(root, other, 'skills')) return true; + } + return false; +} + /** * Copy each target provider's compiled skill variant from an extracted bundle * into the project. Writes real directories (copy, never symlink) so every @@ -1022,10 +1052,12 @@ function copyProviderSkills(bundleDir, root, targets) { if (existsSync(srcDir)) { const localSkillsDir = join(root, provider, 'skills'); // A previous `npx skills` install may have left this provider's skills dir - // as a symlink to another provider's canonical copy. Drop the link so we - // write a real, provider-specific directory instead of writing through it. + // as a symlink to ANOTHER in-project provider's canonical copy. Drop only + // that link so we write a real, provider-specific directory. A user's + // external shared-skills symlink (e.g. ~/.claude/skills -> + // ~/.config/agents/skills) is preserved and written through. See #295. try { - if (lstatSync(localSkillsDir).isSymbolicLink()) unlinkSync(localSkillsDir); + if (isInProjectProviderLink(localSkillsDir, root, provider)) unlinkSync(localSkillsDir); } catch {} for (const skill of readdirSync(srcDir, { withFileTypes: true })) { if (!skill.isDirectory()) continue; diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index e412e3247..a829a055d 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -134,6 +134,84 @@ if (WANT_CLI_REMOTE_E2E) { } const describeRemote = (WANT_CLI_REMOTE_E2E && bundleReachable) ? describe : describe.skip; +describe('copyProviderSkills: symlink handling', () => { + test('preserves an external shared-skills symlink and writes through it (#295)', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-295-ext-')); + const root = join(tmp, 'home'); + const shared = join(tmp, 'shared'); + mkdirSync(root, { recursive: true }); + mkdirSync(join(shared, 'other-skill'), { recursive: true }); + writeFileSync(join(shared, 'other-skill', 'SKILL.md'), '---\nname: other-skill\n---\n'); + mkdirSync(join(root, '.claude'), { recursive: true }); + symlinkSync(shared, join(root, '.claude', 'skills'), 'dir'); + + const bundle = createFakeUniversalBundle(tmp, ['.claude']); + copyProviderSkills(bundle, root, ['.claude']); + + const skillsPath = join(root, '.claude', 'skills'); + expect(lstatSync(skillsPath).isSymbolicLink()).toBe(true); + expect(realpathSync(skillsPath)).toBe(realpathSync(shared)); + expect(existsSync(join(skillsPath, 'other-skill', 'SKILL.md'))).toBe(true); + expect(existsSync(join(shared, 'impeccable', 'SKILL.md'))).toBe(true); + rmSync(tmp, { recursive: true, force: true }); + }); + + test('still converts an in-project cross-provider link to a real dir', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-295-inproj-')); + mkdirSync(join(tmp, '.agents', 'skills'), { recursive: true }); + mkdirSync(join(tmp, '.claude'), { recursive: true }); + symlinkSync('../.agents/skills', join(tmp, '.claude', 'skills'), 'dir'); + + const bundle = createFakeUniversalBundle(tmp, ['.claude']); + copyProviderSkills(bundle, tmp, ['.claude']); + + const skillsPath = join(tmp, '.claude', 'skills'); + expect(lstatSync(skillsPath).isSymbolicLink()).toBe(false); + expect(existsSync(join(skillsPath, 'impeccable', 'SKILL.md'))).toBe(true); + rmSync(tmp, { recursive: true, force: true }); + }); + + test('preserves external symlinks when two providers share one external dir (#295, multi-tool)', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-295-multi-')); + const root = join(tmp, 'home'); + const shared = join(tmp, 'shared'); + mkdirSync(root, { recursive: true }); + mkdirSync(join(shared, 'other-skill'), { recursive: true }); + writeFileSync(join(shared, 'other-skill', 'SKILL.md'), '---\nname: other-skill\n---\n'); + for (const provider of ['.claude', '.agents']) { + mkdirSync(join(root, provider), { recursive: true }); + symlinkSync(shared, join(root, provider, 'skills'), 'dir'); + } + + const bundle = createFakeUniversalBundle(tmp, ['.claude', '.agents']); + copyProviderSkills(bundle, root, ['.claude', '.agents']); + + for (const provider of ['.claude', '.agents']) { + const skillsPath = join(root, provider, 'skills'); + expect(lstatSync(skillsPath).isSymbolicLink()).toBe(true); + expect(realpathSync(skillsPath)).toBe(realpathSync(shared)); + } + expect(existsSync(join(shared, 'other-skill', 'SKILL.md'))).toBe(true); + expect(existsSync(join(shared, 'impeccable', 'SKILL.md'))).toBe(true); + rmSync(tmp, { recursive: true, force: true }); + }); + + test('replaces a dangling in-project cross-provider link with a real dir', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-295-dangling-')); + // Link to another provider's in-project skills dir that does NOT exist yet. + mkdirSync(join(tmp, '.claude'), { recursive: true }); + symlinkSync('../.agents/skills', join(tmp, '.claude', 'skills'), 'dir'); + + const bundle = createFakeUniversalBundle(tmp, ['.claude']); + copyProviderSkills(bundle, tmp, ['.claude']); + + const skillsPath = join(tmp, '.claude', 'skills'); + expect(lstatSync(skillsPath).isSymbolicLink()).toBe(false); + expect(existsSync(join(skillsPath, 'impeccable', 'SKILL.md'))).toBe(true); + rmSync(tmp, { recursive: true, force: true }); + }); +}); + describe('skills install: already-installed detection', () => { test('detects impeccable sentinel and bails', () => { const tmp = mkdtempSync(join(tmpdir(), 'imp-test-'));