fix(adapter-utils): remove a lane-owned Gemini skill link without a recursive call

removeMaintainerOnlySkillSymlinks checked a manifest-named entry's shape,
then removed it with fs.rm(recursive, force) in every case. A concurrent
process could replace an approved symlink with a populated directory in
the gap between the check and the removal; the recursive call would then
delete that directory's contents.

Branch the removal on the shape the check confirmed: fs.unlink for a
symlink, so a directory swapped in during the race fails the unlink
instead of being deleted, and fs.rm(recursive) only for a directory the
check already approved.

Co-authored-by: Paperclip <noreply@paperclip.ing>

packages/adapter-utils/src/server-utils.test.ts

@@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto";
 import fs from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 import { CONNECTION_INTENT_AGENT_GUIDANCE } from "@paperclipai/shared";
 import {
   applyPaperclipWorkspaceEnv,
@@ -512,6 +512,67 @@ describe("removeMaintainerOnlySkillSymlinks", () => {
     }
   });
 
+  it("keeps a directory that replaces a managed symlink in the instant between the ownership check and removal", async () => {
+    const root = await fs.mkdtemp(
+      path.join(os.tmpdir(), "paperclip-gemini-skills-"),
+    );
+    try {
+      const skillsHome = path.join(root, "skills");
+      const source = path.join(root, "source-skill");
+      await fs.mkdir(skillsHome, { recursive: true });
+      await fs.mkdir(source, { recursive: true });
+      await fs.writeFile(path.join(source, "SKILL.md"), "# skill\n", "utf8");
+
+      const target = path.join(skillsHome, "raced-skill");
+      await fs.symlink(source, target);
+      await writeManagedGeminiSkillsManifest(skillsHome, [
+        { name: "raced-skill", source },
+      ]);
+
+      // The prune checks the link target, confirms lane ownership, then
+      // removes the entry. Between those two steps a concurrent process can
+      // replace the link with a populated directory of its own. Simulate
+      // that race at the read that follows the ownership check, so the
+      // removal call runs against the replaced directory, not the symlink
+      // it approved.
+      const originalReadlink = fs.readlink.bind(fs);
+      const readlinkSpy = vi
+        .spyOn(fs, "readlink")
+        .mockImplementationOnce(async (
+          ...readlinkArgs: Parameters<typeof fs.readlink>
+        ) => {
+          const real = await originalReadlink(...readlinkArgs);
+          await fs.unlink(target);
+          await fs.mkdir(target, { recursive: true });
+          await fs.writeFile(
+            path.join(target, "SKILL.md"),
+            "# raced in\n",
+            "utf8",
+          );
+          return real;
+        });
+
+      try {
+        const { removed, failedToRemove } =
+          await removeMaintainerOnlySkillSymlinks(skillsHome, []);
+
+        // The removal call must not delete the directory that replaced the
+        // approved symlink. It must report the entry as failed, not
+        // removed, so a caller retries it on the next pass instead of
+        // silently dropping it from the manifest.
+        expect(removed).toEqual([]);
+        expect(failedToRemove).toEqual([{ name: "raced-skill", source }]);
+        await expect(
+          fs.readFile(path.join(target, "SKILL.md"), "utf8"),
+        ).resolves.toBe("# raced in\n");
+      } finally {
+        readlinkSpy.mockRestore();
+      }
+    } finally {
+      await fs.rm(root, { recursive: true, force: true });
+    }
+  });
+
   it("keeps a non-symlink entry the manifest never named, even when it is not selected", async () => {
     const root = await fs.mkdtemp(
       path.join(os.tmpdir(), "paperclip-gemini-skills-"),

packages/adapter-utils/src/server-utils.ts

@@ -4620,6 +4620,15 @@ export async function isManagedGeminiSkillEntry(
   return false;
 }
 
+/**
+ * The kind of lane-owned shape `laneOwnedSkillEntryShape` found at an entry,
+ * or `null` when the entry is not lane-owned. A caller uses the kind to
+ * choose a removal call that cannot cross into a different entry kind if
+ * the entry changes between the check and the removal — see
+ * `removeMaintainerOnlySkillSymlinks`.
+ */
+type LaneOwnedSkillEntryKind = "symlink" | "directory" | null;
+
 /**
  * Test if `target` carries a shape the Gemini lane could have created for
  * `expectedSource`. `removeMaintainerOnlySkillSymlinks` calls this at the
@@ -4634,24 +4643,40 @@ export async function isManagedGeminiSkillEntry(
  * with no sentinel, a regular file, and every other entry type are not a
  * lane-owned shape, even when the manifest names the entry.
  */
+async function laneOwnedSkillEntryShape(
+  target: string,
+  expectedSource: string,
+): Promise<LaneOwnedSkillEntryKind> {
+  const existing = await fs.lstat(target).catch(() => null);
+  if (!existing) return null;
+  if (existing.isSymbolicLink()) {
+    const linkedPath = await fs.readlink(target).catch(() => null);
+    if (!linkedPath) return null;
+    const resolvedLinkedPath = path.isAbsolute(linkedPath)
+      ? linkedPath
+      : path.resolve(path.dirname(target), linkedPath);
+    return resolvedLinkedPath === path.resolve(expectedSource)
+      ? "symlink"
+      : null;
+  }
+  if (existing.isDirectory()) {
+    return (await hasValidMaterializedSkillSentinel(target))
+      ? "directory"
+      : null;
+  }
+  return null;
+}
+
+/**
+ * Test if `target` carries a shape the Gemini lane could have created for
+ * `expectedSource`. This wraps `laneOwnedSkillEntryShape` for a caller that
+ * only needs the yes/no answer, not the matched kind.
+ */
 export async function isLaneOwnedSkillEntryShape(
   target: string,
   expectedSource: string,
 ): Promise<boolean> {
-  const existing = await fs.lstat(target).catch(() => null);
-  if (!existing) return false;
-  if (existing.isSymbolicLink()) {
-    const linkedPath = await fs.readlink(target).catch(() => null);
-    if (!linkedPath) return false;
-    const resolvedLinkedPath = path.isAbsolute(linkedPath)
-      ? linkedPath
-      : path.resolve(path.dirname(target), linkedPath);
-    return resolvedLinkedPath === path.resolve(expectedSource);
-  }
-  if (existing.isDirectory()) {
-    return hasValidMaterializedSkillSentinel(target);
-  }
-  return false;
+  return (await laneOwnedSkillEntryShape(target, expectedSource)) !== null;
 }
 
 export async function removeMaintainerOnlySkillSymlinks(
@@ -4679,9 +4704,20 @@ export async function removeMaintainerOnlySkillSymlinks(
     // link or directory at the same name. Remove it only when its shape on
     // disk is still lane-owned for the recorded source.
     if (managedEntry) {
-      if (await isLaneOwnedSkillEntryShape(target, managedEntry.source)) {
+      const shape = await laneOwnedSkillEntryShape(target, managedEntry.source);
+      if (shape) {
         try {
-          await fs.rm(target, { recursive: true, force: true });
+          // Remove by the exact kind the check just confirmed, not a
+          // recursive call that would also remove a directory. A directory
+          // can replace a symlink in the instant between the check above
+          // and this removal; `fs.unlink` only ever removes a symlink, so
+          // that race can fail this call but can never delete a directory
+          // the check never approved.
+          if (shape === "symlink") {
+            await fs.unlink(target);
+          } else {
+            await fs.rm(target, { recursive: true, force: true });
+          }
           removed.push(entry.name);
         } catch (err) {
           // The removal failed, so this lane still owns the entry and a