#834 Remove dangling owned symlinks during uninstall

packages/agents/src/commands/__tests__/uninstall.test.ts

@@ -1,6 +1,6 @@
 import assert from 'node:assert';
-import { existsSync } from 'node:fs';
-import { mkdir, readFile, rm, writeFile } from 'node:fs/promises';
+import { existsSync, lstatSync } from 'node:fs';
+import { mkdir, readFile, rm, symlink, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import path from 'node:path';
 
@@ -33,6 +33,33 @@ describe('uninstallCommand', () => {
     };
   }
 
+  /** Creates an owned symlink under the claude harness home pointing to a fresh source file, recorded in a single-entry manifest. */
+  async function writeLinkedEntry(
+    relativePath: string,
+    sourceContent: string,
+  ): Promise<{ linkPath: string; source: string }> {
+    const linkPath = path.join(tempDir, '.claude', relativePath);
+    await mkdir(path.dirname(linkPath), { recursive: true });
+    const source = path.join(tempDir, 'source', path.basename(relativePath));
+    await mkdir(path.dirname(source), { recursive: true });
+    await writeFile(source, sourceContent, 'utf8');
+    await symlink(source, linkPath);
+
+    const manifest: AgentsManifest = {
+      schemaVersion: 1,
+      harnesses: {
+        claude: {
+          harness: 'claude',
+          version: '0.0.0',
+          installedAt: new Date().toISOString(),
+          entries: [{ relativePath, contentHash: 'sha256:linked', linked: true }],
+        },
+      },
+    };
+    await writeManifest(getManifestPath(tempDir), manifest);
+    return { linkPath, source };
+  }
+
   it('should remove only manifest-tracked files', async () => {
     const claudeHome = path.join(tempDir, '.claude');
     await mkdir(path.join(claudeHome, 'skills'), { recursive: true });
@@ -187,4 +214,48 @@ describe('uninstallCommand', () => {
     const manifest = await readManifest(getManifestPath(tempDir));
     expect(manifest.harnesses.claude).toBeUndefined();
   });
+
+  it('removes a dangling owned symlink whose source was deleted', async () => {
+    const { linkPath, source } = await writeLinkedEntry('skills/helper.mjs', 'export const x = 1;');
+    await rm(source); // Leave the symlink dangling, as a deleted source would.
+
+    await uninstallCommand({ harness: 'claude', force: false }, tempDir);
+
+    // existsSync already reads false for a dangling link, so assert the link entry itself is gone.
+    expect(() => lstatSync(linkPath)).toThrow();
+    const manifest = await readManifest(getManifestPath(tempDir));
+    expect(manifest.harnesses.claude).toBeUndefined();
+  });
+
+  it('removes an owned symlink whose target content changed, without force', async () => {
+    const { linkPath, source } = await writeLinkedEntry('skills/helper.mjs', 'original');
+    await writeFile(source, 'changed', 'utf8'); // A symlink has no user content to preserve.
+
+    await uninstallCommand({ harness: 'claude', force: false }, tempDir);
+
+    expect(existsSync(linkPath)).toBe(false);
+    const manifest = await readManifest(getManifestPath(tempDir));
+    expect(manifest.harnesses.claude).toBeUndefined();
+  });
+
+  it('treats a tracked entry already gone from disk as removed', async () => {
+    await mkdir(path.join(tempDir, '.claude', 'skills'), { recursive: true });
+    const manifest: AgentsManifest = {
+      schemaVersion: 1,
+      harnesses: {
+        claude: {
+          harness: 'claude',
+          version: '0.0.0',
+          installedAt: new Date().toISOString(),
+          entries: [{ relativePath: 'skills/missing.md', contentHash: 'sha256:missing', linked: false }],
+        },
+      },
+    };
+    await writeManifest(getManifestPath(tempDir), manifest);
+
+    await expect(uninstallCommand({ harness: 'claude', force: false }, tempDir)).resolves.not.toThrow();
+
+    const updated = await readManifest(getManifestPath(tempDir));
+    expect(updated.harnesses.claude).toBeUndefined();
+  });
 });

packages/agents/src/commands/install.ts

@@ -3,6 +3,7 @@ import path from 'node:path';
 
 import { resolveContentDir } from '../lib/content-resolver.ts';
 import { expandIncludes } from '../lib/directive-expander.ts';
+import { pruneOrphanedEntries } from '../lib/entry-remover.ts';
 import { mergeFrontmatter, parseFrontmatter } from '../lib/frontmatter-merger.ts';
 import { HARNESSES, resolveHarnessIds, resolveHarnessPaths } from '../lib/harness.js';
 import { checkSymlinkSafety, copyItem, linkItem, removeItem, unlinkIfSymlink } from '../lib/installer.ts';
@@ -20,7 +21,6 @@ import {
   injectMarkersInDirectory,
   injectProvenanceMarker,
 } from '../lib/marker-injector.js';
-import { pruneOrphanedEntries } from '../lib/orphan-pruner.ts';
 import { rewritePathsInDirectory, rewritePathsInFile } from '../lib/path-rewriter.js';
 import { loadToolMapping, rewriteToolNames } from '../lib/tool-name-rewriter.js';
 import { isEnoent, isMissingFile } from '../lib/type-guards.ts';

packages/agents/src/commands/uninstall.ts

@@ -1,6 +1,7 @@
+import { classifyOwnedEntry } from '../lib/entry-remover.ts';
 import { resolveHarnessIds, resolveHarnessPaths } from '../lib/harness.js';
 import { removeItem } from '../lib/installer.js';
-import { detectDrift, getManifestPath, readManifest, resolveSharedHome, writeManifest } from '../lib/manifest.js';
+import { getManifestPath, readManifest, resolveSharedHome, writeManifest } from '../lib/manifest.js';
 import type { AgentsManifest, InstallOptions, ManifestEntry, SharedManifest } from '../lib/types.js';
 
 /**
@@ -35,28 +36,7 @@ export async function uninstallCommand(
 
     console.info(`\nUninstalling for harness: ${harnessId}`);
     const paths = resolveHarnessPaths(harnessId, baseDir);
-    let removedCount = 0;
-    const skippedEntries: ManifestEntry[] = [];
-
-    for (const entry of harnessManifest.entries) {
-      const drift = await detectDrift(entry, paths.harnessHome);
-
-      if (drift === 'missing') {
-        // Already gone
-        removedCount++;
-        continue;
-      }
-
-      if (drift === 'modified' && !options.force) {
-        console.warn(`  ⚠️ Skipping modified file: ${entry.relativePath}`);
-        skippedEntries.push(entry);
-        continue;
-      }
-
-      const fullPath = `${paths.harnessHome}/${entry.relativePath}`;
-      await removeItem(fullPath);
-      removedCount++;
-    }
+    const skippedEntries = await removeTrackedEntries(harnessManifest.entries, paths.harnessHome, options.force, '');
 
     // Remove harness from manifest or retain only skipped entries
     if (skippedEntries.length === 0) {
@@ -68,8 +48,6 @@ export async function uninstallCommand(
         [harnessId]: { ...harnessManifest, entries: skippedEntries },
       };
     }
-
-    console.info(`  ✅ Removed ${removedCount} items, skipped ${skippedEntries.length} modified items`);
   }
 
   const updatedManifest: AgentsManifest = {
@@ -98,33 +76,48 @@ async function uninstallSharedGuidance(
 
   console.info('\nUninstalling shared guidance');
   const sharedHome = resolveSharedHome(baseDir);
+  const skippedEntries = await removeTrackedEntries(sharedManifest.entries, sharedHome, options.force, '~/.agents/');
+
+  // Retain shared manifest only with the entries that were skipped
+  if (skippedEntries.length > 0) {
+    return { ...sharedManifest, entries: skippedEntries };
+  }
+  return undefined;
+}
+
+// region | Helpers
+
+/**
+ * Removes each tracked entry the policy marks for removal, collects user-modified entries to keep tracking,
+ * reports the tally, and returns the skipped entries. `displayPrefix` is prepended to the relative path in
+ * skip warnings (e.g., `~/.agents/` for shared guidance).
+ */
+async function removeTrackedEntries(
+  entries: ReadonlyArray<ManifestEntry>,
+  home: string,
+  force: boolean,
+  displayPrefix: string,
+): Promise<ManifestEntry[]> {
   let removedCount = 0;
   const skippedEntries: ManifestEntry[] = [];
 
-  for (const entry of sharedManifest.entries) {
-    const drift = await detectDrift(entry, sharedHome);
-
-    if (drift === 'missing') {
-      removedCount++;
-      continue;
-    }
+  for (const entry of entries) {
+    const verdict = await classifyOwnedEntry(entry, home, force);
 
-    if (drift === 'modified' && !options.force) {
-      console.warn(`  ⚠️ Skipping modified file: ~/.agents/${entry.relativePath}`);
+    if (verdict === 'retain') {
+      console.warn(`  ⚠️ Skipping modified file: ${displayPrefix}${entry.relativePath}`);
       skippedEntries.push(entry);
       continue;
     }
 
-    const fullPath = `${sharedHome}/${entry.relativePath}`;
-    await removeItem(fullPath);
+    if (verdict === 'remove') {
+      await removeItem(`${home}/${entry.relativePath}`);
+    }
     removedCount++;
   }
 
   console.info(`  ✅ Removed ${removedCount} items, skipped ${skippedEntries.length} modified items`);
-
-  // Retain shared manifest only with the entries that were skipped
-  if (skippedEntries.length > 0) {
-    return { ...sharedManifest, entries: skippedEntries };
-  }
-  return undefined;
+  return skippedEntries;
 }
+
+// endregion | Helpers

packages/agents/src/lib/__tests__/orphan-pruner.test.ts → packages/agents/src/lib/__tests__/entry-remover.test.ts

@@ -5,8 +5,8 @@ import path from 'node:path';
 
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 
+import { classifyOwnedEntry, pruneOrphanedEntries } from '../entry-remover.ts';
 import { computeContentHash } from '../manifest.ts';
-import { pruneOrphanedEntries } from '../orphan-pruner.ts';
 import type { ManifestEntry } from '../types.ts';
 
 describe(pruneOrphanedEntries, () => {
@@ -110,3 +110,61 @@ describe(pruneOrphanedEntries, () => {
     expect(result.removedPaths).toEqual([entry.relativePath]);
   });
 });
+
+describe(classifyOwnedEntry, () => {
+  let home: string;
+
+  beforeEach(async () => {
+    home = path.join(tmpdir(), `agents-test-classify-${Date.now()}-${Math.random().toString(36).slice(2)}`);
+    await mkdir(home, { recursive: true });
+  });
+
+  afterEach(async () => {
+    await rm(home, { recursive: true, force: true });
+  });
+
+  async function writeTracked(relativePath: string, content: string): Promise<ManifestEntry> {
+    const fullPath = path.join(home, relativePath);
+    await mkdir(path.dirname(fullPath), { recursive: true });
+    await writeFile(fullPath, content, 'utf8');
+    return { relativePath, contentHash: await computeContentHash(fullPath), linked: false };
+  }
+
+  it('returns remove for a linked entry whose target is gone', async () => {
+    const target = path.join(home, 'target.sh');
+    await writeFile(target, 'content', 'utf8');
+    const linkPath = path.join(home, 'scripts', 'linked.sh');
+    await mkdir(path.dirname(linkPath), { recursive: true });
+    await symlink(target, linkPath);
+    await rm(target); // Leave the symlink dangling, as a deleted source would.
+    const entry: ManifestEntry = { relativePath: 'scripts/linked.sh', contentHash: 'sha256:linked', linked: true };
+
+    expect(await classifyOwnedEntry(entry, home, false)).toBe('remove');
+  });
+
+  it('returns absent for an unlinked entry that is gone from disk', async () => {
+    const entry: ManifestEntry = { relativePath: 'skills/never/SKILL.md', contentHash: 'sha256:absent', linked: false };
+
+    expect(await classifyOwnedEntry(entry, home, false)).toBe('absent');
+  });
+
+  it('returns retain for an unlinked user-modified entry when force is unset', async () => {
+    const entry = await writeTracked('scripts/edited.sh', 'original');
+    await writeFile(path.join(home, entry.relativePath), 'user edit', 'utf8');
+
+    expect(await classifyOwnedEntry(entry, home, false)).toBe('retain');
+  });
+
+  it('returns remove for an unlinked user-modified entry when force is set', async () => {
+    const entry = await writeTracked('scripts/edited.sh', 'original');
+    await writeFile(path.join(home, entry.relativePath), 'user edit', 'utf8');
+
+    expect(await classifyOwnedEntry(entry, home, true)).toBe('remove');
+  });
+
+  it('returns remove for an unlinked unmodified entry', async () => {
+    const entry = await writeTracked('scripts/clean.sh', 'original');
+
+    expect(await classifyOwnedEntry(entry, home, false)).toBe('remove');
+  });
+});

packages/agents/src/lib/orphan-pruner.ts → packages/agents/src/lib/entry-remover.ts

@@ -4,6 +4,9 @@ import { removeItem } from './installer.ts';
 import { detectDrift } from './manifest.ts';
 import type { ManifestEntry } from './types.ts';
 
+/** Fate of an owned manifest entry during removal: delete it, keep it (user-modified, no force), or note it is already gone from disk. */
+export type OwnedEntryVerdict = 'remove' | 'retain' | 'absent';
+
 /** Options controlling orphan pruning, mirroring the install flags that govern it. */
 interface PruneOptions {
   readonly force: boolean;
@@ -16,14 +19,36 @@ interface PruneResult {
   readonly removedPaths: ReadonlyArray<string>;
 }
 
+/**
+ * Decides an owned manifest entry's fate during removal. A symlink (no user-modifiable content) or an
+ * unmodified-or-forced file is removed; a user-modified file without `force` is retained; an entry already
+ * gone from disk is absent. Checking `linked` before drift detection is what lets a dangling symlink — which
+ * `detectDrift` reports as `missing` — be removed rather than treated as already gone.
+ */
+export async function classifyOwnedEntry(
+  entry: ManifestEntry,
+  home: string,
+  force: boolean,
+): Promise<OwnedEntryVerdict> {
+  if (entry.linked) {
+    return 'remove';
+  }
+
+  const drift = await detectDrift(entry, home);
+  if (drift === 'missing') {
+    return 'absent';
+  }
+  if (drift === 'modified' && !force) {
+    return 'retain';
+  }
+  return 'remove';
+}
+
 /**
  * Removes installed files recorded in `previousEntries` whose source no longer exists — those whose
- * `relativePath` is absent from `currentEntries` — resolving paths against the install root `home`.
- *
- * A `linked` orphan is a symlink CodeAssembly owns whose source is gone, so it is removed outright (a symlink
- * has no content to be user-modified). An unlinked orphan is removed unless the user modified it and `force`
- * is unset, in which case it is retained so the manifest keeps tracking it. In `dryRun`, removals are reported
- * and recorded but not performed.
+ * `relativePath` is absent from `currentEntries` — resolving paths against the install root `home`. Each
+ * orphan's fate follows `classifyOwnedEntry`; a retained (user-modified, unforced) orphan stays tracked in
+ * the manifest. In `dryRun`, removals are reported and recorded but not performed.
  */
 export async function pruneOrphanedEntries(
   previousEntries: ReadonlyArray<ManifestEntry>,
@@ -38,27 +63,20 @@ export async function pruneOrphanedEntries(
   const removedPaths: Array<string> = [];
 
   for (const orphan of orphans) {
-    const fullPath = path.join(home, orphan.relativePath);
+    const verdict = await classifyOwnedEntry(orphan, home, options.force);
 
-    if (orphan.linked) {
-      await removeOrphan(fullPath, orphan.relativePath, removedPaths, options.dryRun);
+    if (verdict === 'retain') {
+      console.warn(`  ⚠️ Keeping modified stale item: ${orphan.relativePath}`);
+      retained.push(orphan);
       continue;
     }
 
-    const drift = await detectDrift(orphan, home);
-
-    if (drift === 'missing') {
+    if (verdict === 'absent') {
       removedPaths.push(orphan.relativePath);
       continue;
     }
 
-    if (drift === 'modified' && !options.force) {
-      console.warn(`  ⚠️ Keeping modified stale item: ${orphan.relativePath}`);
-      retained.push(orphan);
-      continue;
-    }
-
-    await removeOrphan(fullPath, orphan.relativePath, removedPaths, options.dryRun);
+    await removeOrphan(path.join(home, orphan.relativePath), orphan.relativePath, removedPaths, options.dryRun);
   }
 
   return { retained, removedPaths };