Fix: preserve external ~/.claude/skills symlink on first install (#295) (#308)

* Fix: preserve external skills symlink on first install (#295)

* Fix review comments: target-based in-project link detection (#295, #308)

- isInProjectProviderLink now inspects the symlink TARGET lexically instead of comparing shared realpaths, so two providers pointing at the same external dir are no longer misflagged as in-project (cursor High / greptile P1).
- A dangling in-project cross-provider link is now correctly replaced with a real per-provider dir (cursor Medium).
- Adds regression tests for both scenarios.

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;

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-'));