fix(setup): remove emptied skill directories on uninstall (#2182)

* fix(setup): remove emptied skill directories on uninstall

Uninstall removes only the managed skill files so user files survive,
but it left every emptied reverse-engineer-anything directory and its
references/ subdirectory behind. After the ownership manifest is removed,
remove the managed directories deepest first with a non-recursive rmdir,
which keeps any directory that still holds user files.

Fixes #2159

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(setup): report emptied skill directories that cannot be removed

Only a directory that still holds user files, is already absent, or is no
longer a directory is kept silently. Any other rmdir failure, such as a
parent that denies deletion, now fails the skill item with the directory
path instead of reporting the skill as removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

src/application/Uninstall.ts

@@ -7,7 +7,7 @@ import {
   withClientServers,
   type ClientConfigurationDocument,
 } from "./ClientConfigurationDocument.js";
-import { copyFile, lstat, realpath, rm } from "node:fs/promises";
+import { copyFile, lstat, realpath, rm, rmdir } from "node:fs/promises";
 import { constants as fsConstants } from "node:fs";
 import { homeDirectoryFromEnvironment } from "../config/homeDirectory.js";
 import { join } from "node:path";
@@ -51,6 +51,8 @@ export interface UninstallFileSystem {
   stat(path: string): Promise<ManagedPathStats>;
   realpath?(path: string): Promise<string>;
   remove(path: string, recursive?: boolean): Promise<void>;
+  /** Remove a directory only if it is empty; a non-empty one is kept. */
+  removeEmptyDirectory?(path: string): Promise<void>;
 }
 
 const systemFileSystem = (
@@ -65,6 +67,7 @@ const systemFileSystem = (
   stat: (path) => lstat(path),
   realpath: (path) => realpath(path),
   remove: (path, recursive = true) => rm(path, { recursive }),
+  removeEmptyDirectory: (path) => rmdir(path),
 });
 
 /** One explicitly classified uninstall action. */
@@ -602,10 +605,20 @@ const removeManagedSkill = async (
         "REA ownership manifest was retained because a managed file could not be safely removed.",
       ),
     );
-  else
-    results.push(
-      await removeManagedSkillFile(path, manifestName, name, fileSystem),
+  else {
+    const manifest = await removeManagedSkillFile(
+      path,
+      manifestName,
+      name,
+      fileSystem,
     );
+    results.push(manifest);
+    const directories =
+      manifest.status === "removed"
+        ? await removeEmptyManagedDirectories(path, name, fileSystem)
+        : undefined;
+    if (directories !== undefined) results.push(directories);
+  }
   const failed = results.find(({ status }) => status === "failed");
   if (failed !== undefined)
     return item(name, "failed", results.map(({ detail }) => detail).join(" "));
@@ -617,6 +630,46 @@ const removeManagedSkill = async (
   );
 };
 
+/** Directories setup creates for managed files, deepest first, then the root. */
+const MANAGED_SKILL_DIRECTORIES = [
+  ...new Set(
+    MANAGED_SKILL_FILES.map((relativePath) =>
+      relativePath.split("/").slice(0, -1).join("/"),
+    ).filter((directory) => directory.length > 0),
+  ),
+].sort((left, right) => right.split("/").length - left.split("/").length);
+
+/** Remove managed directories left empty; any remaining user file keeps them. */
+const removeEmptyManagedDirectories = async (
+  root: string,
+  name: string,
+  fileSystem: UninstallFileSystem,
+): Promise<UninstallItem | undefined> => {
+  if (fileSystem.removeEmptyDirectory === undefined) return undefined;
+  for (const directory of [
+    ...MANAGED_SKILL_DIRECTORIES.map((relative) => join(root, relative)),
+    root,
+  ]) {
+    try {
+      await fileSystem.removeEmptyDirectory(directory);
+    } catch (cause: unknown) {
+      // User files, an absent directory, or a non-directory keep it as is.
+      if (!isKeptDirectory(cause))
+        return item(
+          name,
+          "failed",
+          `Managed skill directory could not be removed: ${directory}.`,
+        );
+    }
+  }
+  return undefined;
+};
+
+const isKeptDirectory = (cause: unknown): boolean =>
+  cause instanceof Error &&
+  "code" in cause &&
+  ["ENOTEMPTY", "EEXIST", "ENOENT", "ENOTDIR"].includes(String(cause.code));
+
 const removeManagedSkillFile = async (
   root: string,
   relativePath: string,

tests/boundary/filesystem/clientLifecycle.test.ts

@@ -1,5 +1,6 @@
 import { constants as fsConstants } from "node:fs";
 import {
+  chmod,
   copyFile,
   lstat,
   mkdir,
@@ -1527,6 +1528,68 @@ describe("client configuration filesystem removal", () => {
     );
   });
 
+  it("removes emptied skill directories and keeps ones with user files", async () => {
+    const home = await createTestTempDirectory("rea-uninstall-skill-dirs-");
+    roots.push(home);
+    const shared = join(home, ".agents/skills/reverse-engineer-anything");
+    const claude = join(home, ".claude/skills/reverse-engineer-anything");
+    for (const root of [shared, claude]) {
+      await mkdir(join(root, "references"), { recursive: true });
+      await writeFile(join(root, "SKILL.md"), ownedSkillManifest);
+      await writeFile(
+        join(root, "references/evidence-workflows.md"),
+        "managed",
+      );
+    }
+    await writeFile(join(claude, "notes.md"), "user");
+
+    const result = await runUninstall(
+      false,
+      systemUninstallHost(home, undefined, {
+        HOME: home,
+        CLAUDE_CONFIG_DIR: join(home, ".claude"),
+      }),
+    );
+
+    expect(result.status).toBe("complete");
+    await expect(lstat(shared)).rejects.toMatchObject({ code: "ENOENT" });
+    await expect(lstat(join(claude, "references"))).rejects.toMatchObject({
+      code: "ENOENT",
+    });
+    expect(await readFile(join(claude, "notes.md"), "utf8")).toBe("user");
+  });
+
+  it.skipIf(process.platform === "win32" || process.getuid?.() === 0)(
+    "reports an emptied skill directory that cannot be removed",
+    async () => {
+      const home = await createTestTempDirectory("rea-uninstall-skill-perm-");
+      roots.push(home);
+      const parent = join(home, ".agents/skills");
+      const root = join(parent, "reverse-engineer-anything");
+      await mkdir(join(root, "references"), { recursive: true });
+      await writeFile(join(root, "SKILL.md"), ownedSkillManifest);
+      await chmod(parent, 0o555);
+      try {
+        const result = await runUninstall(
+          false,
+          systemUninstallHost(home, undefined, { HOME: home }),
+        );
+        expect(result.status).toBe("failed");
+        expect(result.items).toContainEqual(
+          expect.objectContaining({
+            name: "skill",
+            status: "failed",
+            detail: expect.stringContaining(
+              `Managed skill directory could not be removed: ${root}.`,
+            ),
+          }),
+        );
+      } finally {
+        await chmod(parent, 0o755);
+      }
+    },
+  );
+
   it("removes an owned entry through a symlink without replacing it", async () => {
     const home = await createTestTempDirectory("rea-uninstall-symlink-");
     roots.push(home);