From bb88ff93f6f40fcabf3bb8d990f03263a86284f3 Mon Sep 17 00:00:00 2001 From: nickyleach <331803+nickyleach@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:32:18 +0000 Subject: [PATCH] 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 --- .../adapter-utils/src/server-utils.test.ts | 63 ++++++++++++++++- packages/adapter-utils/src/server-utils.ts | 68 ++++++++++++++----- 2 files changed, 114 insertions(+), 17 deletions(-) diff --git a/packages/adapter-utils/src/server-utils.test.ts b/packages/adapter-utils/src/server-utils.test.ts index 103f2b6540..dcc7d25540 100644 --- a/packages/adapter-utils/src/server-utils.test.ts +++ b/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 + ) => { + 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-"), diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index 252deabd2d..df66cdf2b5 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/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 { + 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 { - 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