From 0ca4146473122dc9e2316023580b444fbd2b949e Mon Sep 17 00:00:00 2001 From: nickyleach <331803+nickyleach@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:04:01 +0000 Subject: [PATCH] fix(adapter-utils): verify symlink identity and removal success before pruning a managed Gemini skill The prune treated any symlink at a manifest-named path as lane-owned, so it could delete a user's own replacement link. It also reported a removal as done even when fs.rm failed, so a failed entry dropped out of the manifest and a later prune could not retry it. The manifest now records each managed skill's resolved source path. The prune compares a symlink's current target against that source before removal, and only reports a removal after fs.rm succeeds. An entry the removal fails on stays in the manifest for a later retry. Co-authored-by: Paperclip --- cli/src/commands/client/agent.ts | 10 +- .../adapter-utils/src/acpx-engine/execute.ts | 20 +- .../adapter-utils/src/server-utils.test.ts | 117 ++++++++- packages/adapter-utils/src/server-utils.ts | 240 ++++++++++++------ .../cursor-local/src/server/execute.ts | 2 +- .../gemini-local/src/server/execute.ts | 17 +- .../opencode-local/src/server/execute.ts | 2 +- .../adapters/pi-local/src/server/execute.ts | 2 +- .../__tests__/paperclip-skill-utils.test.ts | 2 +- 9 files changed, 304 insertions(+), 108 deletions(-) diff --git a/cli/src/commands/client/agent.ts b/cli/src/commands/client/agent.ts index 3f81da6c6b..22d50cec72 100644 --- a/cli/src/commands/client/agent.ts +++ b/cli/src/commands/client/agent.ts @@ -137,10 +137,12 @@ async function installSkillsForTarget( await fs.mkdir(targetSkillsDir, { recursive: true }); const entries = await fs.readdir(sourceSkillsDir, { withFileTypes: true }); - summary.removed = await removeMaintainerOnlySkillSymlinks( - targetSkillsDir, - entries.filter((entry) => entry.isDirectory()).map((entry) => entry.name), - ); + summary.removed = ( + await removeMaintainerOnlySkillSymlinks( + targetSkillsDir, + entries.filter((entry) => entry.isDirectory()).map((entry) => entry.name), + ) + ).removed; for (const entry of entries) { if (!entry.isDirectory()) continue; const source = path.join(sourceSkillsDir, entry.name); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index c7b4b03692..1fffd56898 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -1354,12 +1354,24 @@ async function prepareGeminiSkillRuntime(input: { await fs.mkdir(skillsHome, { recursive: true }); const allowedSkillNames = selectedSkills.map((entry) => entry.runtimeName); - const removedSkills = await removeMaintainerOnlySkillSymlinks(skillsHome, allowedSkillNames); + const { removed: removedSkills, failedToRemove } = await removeMaintainerOnlySkillSymlinks( + skillsHome, + allowedSkillNames, + ); for (const skillName of removedSkills) { await input.onLog("stdout", `[paperclip] Removed stale ACPX Gemini skill "${skillName}" from ${skillsHome}\n`); } + for (const failedEntry of failedToRemove) { + await input.onLog( + "stderr", + `[paperclip] Failed to remove stale ACPX Gemini skill "${failedEntry.name}" from ${skillsHome}; it stays in the managed-skill manifest for a later retry.\n`, + ); + } - const ownedSkillNames: string[] = []; + // Keep every entry this lane still owns but could not remove, so the next + // prune can retry it. None of these names overlap `selectedSkills`: the + // loop above only reports a failure for a name outside `allowedSkillNames`. + const ownedSkills: Array<{ name: string; source: string }> = [...failedToRemove]; for (const entry of selectedSkills) { const target = path.join(skillsHome, entry.runtimeName); try { @@ -1389,10 +1401,10 @@ async function prepareGeminiSkillRuntime(input: { // pointed at the right source, both return "skipped" too, so the // result string alone cannot tell an owned entry from a user one. if (await isManagedGeminiSkillEntry(target, entry.source)) { - ownedSkillNames.push(entry.runtimeName); + ownedSkills.push({ name: entry.runtimeName, source: path.resolve(entry.source) }); } } - await writeManagedGeminiSkillsManifest(skillsHome, ownedSkillNames); + await writeManagedGeminiSkillsManifest(skillsHome, ownedSkills); return { identity: { diff --git a/packages/adapter-utils/src/server-utils.test.ts b/packages/adapter-utils/src/server-utils.test.ts index fd817196f2..103f2b6540 100644 --- a/packages/adapter-utils/src/server-utils.test.ts +++ b/packages/adapter-utils/src/server-utils.test.ts @@ -374,7 +374,9 @@ describe("removeMaintainerOnlySkillSymlinks", () => { "utf8", ); await materializePaperclipSkillCopy(staleSource, staleManagedDir); - await writeManagedGeminiSkillsManifest(skillsHome, ["old-skill"]); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "old-skill", source: staleSource }, + ]); // An entry the manifest never named: a skill the user put in their // own Gemini skills home, not one Paperclip materialized. @@ -386,7 +388,7 @@ describe("removeMaintainerOnlySkillSymlinks", () => { "utf8", ); - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual(["old-skill"]); await expect(fs.stat(staleManagedDir)).rejects.toThrow(); @@ -411,9 +413,11 @@ describe("removeMaintainerOnlySkillSymlinks", () => { const target = path.join(skillsHome, "linked-skill"); await fs.symlink(source, target); - await writeManagedGeminiSkillsManifest(skillsHome, ["linked-skill"]); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "linked-skill", source }, + ]); - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual(["linked-skill"]); await expect(fs.lstat(target)).rejects.toThrow(); @@ -427,6 +431,87 @@ describe("removeMaintainerOnlySkillSymlinks", () => { } }); + it("keeps a symbolic link the manifest names when the user repointed it at their own target", async () => { + const root = await fs.mkdtemp( + path.join(os.tmpdir(), "paperclip-gemini-skills-"), + ); + try { + const skillsHome = path.join(root, "skills"); + const paperclipSource = path.join(root, "paperclip-source-skill"); + const usersOwnSource = path.join(root, "users-own-source"); + await fs.mkdir(skillsHome, { recursive: true }); + await fs.mkdir(paperclipSource, { recursive: true }); + await fs.mkdir(usersOwnSource, { recursive: true }); + await fs.writeFile( + path.join(usersOwnSource, "SKILL.md"), + "# mine\n", + "utf8", + ); + + // Run 1: the lane links its own skill and records the link as managed. + const target = path.join(skillsHome, "shared-name"); + await fs.symlink(paperclipSource, target); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "shared-name", source: paperclipSource }, + ]); + + // Between runs, the user removes the link and points a new symbolic + // link of the same name at their own skill. The manifest still names + // "shared-name", but the link no longer resolves to the source this + // lane materialized. + await fs.unlink(target); + await fs.symlink(usersOwnSource, target); + + // Run 2: the skill is no longer selected. The prune must keep the + // user's replacement link and must not report it as removed. + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + + expect(removed).toEqual([]); + const linkedPath = await fs.readlink(target); + expect(path.resolve(path.dirname(target), linkedPath)).toBe( + usersOwnSource, + ); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it("reports a manifest-named entry it could not remove as failed, not removed, and keeps it for retry", 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, "linked-skill"); + await fs.symlink(source, target); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "linked-skill", source }, + ]); + + // Deny write access to skillsHome itself, so removing an entry inside + // it fails, the way a permission error or a busy mount would. + await fs.chmod(skillsHome, 0o555); + try { + const { removed, failedToRemove } = + await removeMaintainerOnlySkillSymlinks(skillsHome, []); + + expect(removed).toEqual([]); + expect(failedToRemove).toEqual([{ name: "linked-skill", source }]); + // The failed removal must not have deleted the link. + await expect(fs.lstat(target)).resolves.toBeDefined(); + } finally { + await fs.chmod(skillsHome, 0o755); + } + } 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-"), @@ -444,7 +529,7 @@ describe("removeMaintainerOnlySkillSymlinks", () => { // No manifest is written at all: this skills home predates the // manifest, or the entry was never Paperclip's to manage. - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual([]); await expect( @@ -479,7 +564,7 @@ describe("removeMaintainerOnlySkillSymlinks", () => { await fs.symlink(maintainerSource, target); // No manifest is written: this covers the pre-manifest legacy case. - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual(["maintainer-skill"]); await expect(fs.lstat(target)).rejects.toThrow(); @@ -502,7 +587,9 @@ describe("removeMaintainerOnlySkillSymlinks", () => { // Run 1: the lane links the selected skill and records it as managed. const target = path.join(skillsHome, "notes"); await ensurePaperclipSkillSymlink(source, target); - await writeManagedGeminiSkillsManifest(skillsHome, ["notes"]); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "notes", source }, + ]); // Between runs, the user removes the link by hand and writes their // own directory at the same name. @@ -518,7 +605,7 @@ describe("removeMaintainerOnlySkillSymlinks", () => { // "notes", but the entry on disk is now the user's own directory, not // a lane-owned shape. The prune must keep it and must not report it // as removed. - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual([]); await expect( @@ -539,9 +626,11 @@ describe("removeMaintainerOnlySkillSymlinks", () => { const target = path.join(skillsHome, "stray-file"); await fs.writeFile(target, "not a skill\n", "utf8"); - await writeManagedGeminiSkillsManifest(skillsHome, ["stray-file"]); + await writeManagedGeminiSkillsManifest(skillsHome, [ + { name: "stray-file", source: path.join(root, "source-skill") }, + ]); - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, []); expect(removed).toEqual([]); await expect(fs.readFile(target, "utf8")).resolves.toBe( @@ -563,8 +652,8 @@ describe("Gemini managed-skills manifest records only owned entries", () => { entries: Array<{ runtimeName: string; source: string }>, ): Promise { const selectedNames = entries.map((entry) => entry.runtimeName); - await removeMaintainerOnlySkillSymlinks(skillsHome, selectedNames); - const ownedNames: string[] = []; + const { failedToRemove } = await removeMaintainerOnlySkillSymlinks(skillsHome, selectedNames); + const owned: Array<{ name: string; source: string }> = [...failedToRemove]; for (const entry of entries) { const target = path.join(skillsHome, entry.runtimeName); try { @@ -573,10 +662,10 @@ describe("Gemini managed-skills manifest records only owned entries", () => { await materializePaperclipSkillCopy(entry.source, target); } if (await isManagedGeminiSkillEntry(target, entry.source)) { - ownedNames.push(entry.runtimeName); + owned.push({ name: entry.runtimeName, source: entry.source }); } } - await writeManagedGeminiSkillsManifest(skillsHome, ownedNames); + await writeManagedGeminiSkillsManifest(skillsHome, owned); } it("keeps a real user directory alive across selection and deselection", async () => { diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index c2bbb0604a..252deabd2d 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/packages/adapter-utils/src/server-utils.ts @@ -175,6 +175,11 @@ const MATERIALIZED_SKILL_SENTINEL = ".paperclip-materialized-skill.json"; const MATERIALIZED_SKILL_LOCK_OWNER = "owner.json"; const MATERIALIZED_SKILL_LOCK_STALE_MS = 30_000; const MANAGED_GEMINI_SKILLS_MANIFEST = ".paperclip-managed-skills.json"; +// Version 2 adds each entry's expected source path, so a later prune can +// tell a lane-owned symlink from a user's own replacement at the same name. +// A version-1 manifest carries no source, so this lane treats it as empty +// and rebuilds it on the next materialize pass. +const MANAGED_GEMINI_SKILLS_MANIFEST_VERSION = 2; function expandHomePrefix(value: string): string { if (value === "~") return os.homedir(); @@ -313,6 +318,28 @@ export interface MaterializedPaperclipSkillCopyResult { skippedSymlinks: string[]; } +/** + * One skill name this lane manages in a Gemini skills home, and the + * resolved skill source path it linked or copied there. The prune in + * `removeMaintainerOnlySkillSymlinks` uses `source` to confirm a symbolic + * link at this name still points at the skill this lane put there, before + * removing it. + */ +export interface ManagedGeminiSkillEntry { + name: string; + source: string; +} + +export interface RemoveMaintainerOnlySkillSymlinksResult { + removed: string[]; + /** + * A manifest-named entry this lane still owns but could not remove, with + * the source `fs.rm` failed on. Keep every one of these in the manifest a + * caller writes next, so a later prune retries the removal. + */ + failedToRemove: ManagedGeminiSkillEntry[]; +} + interface PersistentSkillSnapshotOptions { adapterType: string; availableEntries: PaperclipSkillEntry[]; @@ -4455,16 +4482,19 @@ export async function materializePaperclipSkillCopy( } /** - * Read the set of skill names the Gemini lane manages in `skillsHome`. A - * managed name is a name `writeManagedGeminiSkillsManifest` wrote after this - * lane materialized it. A missing or unreadable manifest yields an empty - * set, so a skills home from before this manifest existed manages nothing - * yet — the legacy symlink check in `removeMaintainerOnlySkillSymlinks` - * still covers that case. + * Read the skills the Gemini lane manages in `skillsHome`, keyed by name. A + * managed entry is one `writeManagedGeminiSkillsManifest` wrote after this + * lane materialized it, and it carries the resolved source path this lane + * linked or copied at that name. A missing, unreadable, or old-version + * manifest yields an empty map, so a skills home from before this manifest + * existed, or before it recorded a source, manages nothing yet — the legacy + * symlink check in `removeMaintainerOnlySkillSymlinks` still covers that + * case, and the next materialize pass rewrites the manifest at the current + * version. */ export async function readManagedGeminiSkillsManifest( skillsHome: string, -): Promise> { +): Promise> { try { const raw = JSON.parse( await fs.readFile( @@ -4473,34 +4503,58 @@ export async function readManagedGeminiSkillsManifest( ), ) as unknown; const parsed = parseObject(raw); - const names = Array.isArray(parsed.managedSkillNames) - ? parsed.managedSkillNames.filter( - (value): value is string => - typeof value === "string" && value.trim().length > 0, - ) + if (parsed.version !== MANAGED_GEMINI_SKILLS_MANIFEST_VERSION) { + return new Map(); + } + const rawEntries = Array.isArray(parsed.managedSkills) + ? parsed.managedSkills : []; - return new Set(names); + const managed = new Map(); + for (const rawEntry of rawEntries) { + const entry = parseObject(rawEntry); + const name = + typeof entry.name === "string" ? entry.name.trim() : ""; + const source = + typeof entry.source === "string" ? entry.source.trim() : ""; + if (!name || !source) continue; + managed.set(name, { name, source }); + } + return managed; } catch { - return new Set(); + return new Map(); } } /** - * Record the skill names the Gemini lane just materialized into - * `skillsHome`. Call this after every materialize pass, so the manifest - * always names exactly the entries this lane owns. Only a name in this - * manifest is a valid prune target for `removeMaintainerOnlySkillSymlinks` — - * a name never written here is a skill the user or another tool put in - * their own Gemini skills home, and it must survive. + * Record the skills the Gemini lane just materialized into `skillsHome`, + * each with the resolved source path this lane linked or copied at that + * name. Call this after every materialize pass, so the manifest always + * names exactly the entries this lane owns, together with enough identity + * to confirm ownership again at prune time. Only a name in this manifest is + * a candidate prune target for `removeMaintainerOnlySkillSymlinks` — a name + * never written here is a skill the user or another tool put in their own + * Gemini skills home, and it must survive. */ export async function writeManagedGeminiSkillsManifest( skillsHome: string, - skillNames: Iterable, + managedSkills: Iterable, ): Promise { - const managedSkillNames = Array.from(new Set(skillNames)).sort(); + const bySortedName = new Map(); + for (const { name, source } of managedSkills) { + if (!name || !source) continue; + bySortedName.set(name, source); + } + const managedSkillNames = Array.from(bySortedName.keys()).sort(); + const payload = { + version: MANAGED_GEMINI_SKILLS_MANIFEST_VERSION, + managedSkills: managedSkillNames.map((name) => ({ + name, + source: bySortedName.get(name), + })), + }; await fs.writeFile( path.join(skillsHome, MANAGED_GEMINI_SKILLS_MANIFEST), - `${JSON.stringify({ version: 1, managedSkillNames }, null, 2)}\n`, + `${JSON.stringify(payload, null, 2)}\n`, "utf8", ); } @@ -4567,24 +4621,33 @@ export async function isManagedGeminiSkillEntry( } /** - * Test if `target` carries a shape the Gemini lane could have created, with - * no check against a specific skill source. `removeMaintainerOnlySkillSymlinks` - * calls this at the point of removal, because the manifest only records what - * the lane owned at the end of the last run — it does not prove the entry is - * still the lane's now. A lane-owned shape is one of: + * Test if `target` carries a shape the Gemini lane could have created for + * `expectedSource`. `removeMaintainerOnlySkillSymlinks` calls this at the + * point of removal, because the manifest only records what the lane owned + * at the end of the last run — it does not prove the entry is still the + * lane's now. A lane-owned shape is one of: * - * - a symbolic link, of any target; or + * - a symbolic link that still resolves to `expectedSource`; or * - a directory that carries a valid materialized-skill sentinel. * - * A plain directory with no sentinel, a regular file, and every other entry - * type are not a lane-owned shape, even when the manifest names the entry. + * A symbolic link the user repointed at their own target, a plain directory + * with no sentinel, a regular file, and every other entry type are not a + * lane-owned shape, even when the manifest names the entry. */ export async function isLaneOwnedSkillEntryShape( target: string, + expectedSource: string, ): Promise { const existing = await fs.lstat(target).catch(() => null); if (!existing) return false; - if (existing.isSymbolicLink()) return true; + 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); } @@ -4594,65 +4657,86 @@ export async function isLaneOwnedSkillEntryShape( export async function removeMaintainerOnlySkillSymlinks( skillsHome: string, allowedSkillNames: Iterable, -): Promise { +): Promise { const allowed = new Set(Array.from(allowedSkillNames)); const managed = await readManagedGeminiSkillsManifest(skillsHome); - try { - const entries = await fs.readdir(skillsHome, { withFileTypes: true }); - const removed: string[] = []; - for (const entry of entries) { - if (allowed.has(entry.name)) continue; + const removed: string[] = []; + const failedToRemove: ManagedGeminiSkillEntry[] = []; - const target = path.join(skillsHome, entry.name); + const entries = await fs + .readdir(skillsHome, { withFileTypes: true }) + .catch(() => []); - // A name in the manifest is a skill this lane owned at the end of the - // last run. That does not prove the lane still owns it now: a user - // action between runs can replace the entry with their own directory - // at the same name. Remove it only when its shape on disk is still - // lane-owned — a symbolic link, or a directory with a valid - // materialized-skill sentinel. - if (managed.has(entry.name)) { - if (await isLaneOwnedSkillEntryShape(target)) { - await fs.rm(target, { recursive: true, force: true }).catch(() => {}); + for (const entry of entries) { + if (allowed.has(entry.name)) continue; + + const target = path.join(skillsHome, entry.name); + const managedEntry = managed.get(entry.name); + + // A name in the manifest is a skill this lane owned at the end of the + // last run. That does not prove the lane still owns it now: a user + // action between runs can replace the entry with their own symbolic + // 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)) { + try { + await fs.rm(target, { recursive: true, force: true }); removed.push(entry.name); - } else { + } catch (err) { + // The removal failed, so this lane still owns the entry and a + // later pass must retry it. Report it as failed, not removed, and + // let the caller keep it in the manifest it writes next. + failedToRemove.push(managedEntry); // eslint-disable-next-line no-console console.warn( - `[paperclip] kept Gemini managed-skill entry "${entry.name}" — ` + - "its shape on disk is not lane-owned.", + `[paperclip] failed to remove Gemini managed-skill entry "${entry.name}": ` + + `${err instanceof Error ? err.message : String(err)}`, ); } - continue; + } else { + // eslint-disable-next-line no-console + console.warn( + `[paperclip] kept Gemini managed-skill entry "${entry.name}" — ` + + "its shape on disk is not lane-owned.", + ); } - - // A name outside the manifest predates it, or belongs to the user. - // Only remove it when it is a symlink into a maintainer-only source — - // the narrow legacy case a skills home can carry from before this - // manifest existed. Every other unmanaged entry survives untouched. - const existing = await fs.lstat(target).catch(() => null); - if (!existing?.isSymbolicLink()) continue; - - const linkedPath = await fs.readlink(target).catch(() => null); - if (!linkedPath) continue; - - const resolvedLinkedPath = path.isAbsolute(linkedPath) - ? linkedPath - : path.resolve(path.dirname(target), linkedPath); - if ( - !isMaintainerOnlySkillTarget(linkedPath) && - !isMaintainerOnlySkillTarget(resolvedLinkedPath) - ) { - continue; - } - - await fs.unlink(target); - removed.push(entry.name); + continue; } - return removed; - } catch { - return []; + // A name outside the manifest predates it, or belongs to the user. + // Only remove it when it is a symlink into a maintainer-only source — + // the narrow legacy case a skills home can carry from before this + // manifest existed. Every other unmanaged entry survives untouched. + const existing = await fs.lstat(target).catch(() => null); + if (!existing?.isSymbolicLink()) continue; + + const linkedPath = await fs.readlink(target).catch(() => null); + if (!linkedPath) continue; + + const resolvedLinkedPath = path.isAbsolute(linkedPath) + ? linkedPath + : path.resolve(path.dirname(target), linkedPath); + if ( + !isMaintainerOnlySkillTarget(linkedPath) && + !isMaintainerOnlySkillTarget(resolvedLinkedPath) + ) { + continue; + } + + try { + await fs.unlink(target); + removed.push(entry.name); + } catch (err) { + // eslint-disable-next-line no-console + console.warn( + `[paperclip] failed to remove legacy Gemini skill symlink "${entry.name}": ` + + `${err instanceof Error ? err.message : String(err)}`, + ); + } } + + return { removed, failedToRemove }; } export async function ensureCommandResolvable( diff --git a/packages/adapters/cursor-local/src/server/execute.ts b/packages/adapters/cursor-local/src/server/execute.ts index eed1a926a5..5c26d3fa37 100644 --- a/packages/adapters/cursor-local/src/server/execute.ts +++ b/packages/adapters/cursor-local/src/server/execute.ts @@ -166,7 +166,7 @@ export async function ensureCursorSkillsInjected( ); return; } - const removedSkills = await removeMaintainerOnlySkillSymlinks( + const { removed: removedSkills } = await removeMaintainerOnlySkillSymlinks( skillsHome, skillsEntries.map((entry) => entry.runtimeName), ); diff --git a/packages/adapters/gemini-local/src/server/execute.ts b/packages/adapters/gemini-local/src/server/execute.ts index 36c7be45af..c6e4dd5ca2 100644 --- a/packages/adapters/gemini-local/src/server/execute.ts +++ b/packages/adapters/gemini-local/src/server/execute.ts @@ -159,7 +159,7 @@ async function ensureGeminiSkillsInjected( ); return; } - const removedSkills = await removeMaintainerOnlySkillSymlinks( + const { removed: removedSkills, failedToRemove } = await removeMaintainerOnlySkillSymlinks( skillsHome, selectedEntries.map((entry) => entry.runtimeName), ); @@ -169,8 +169,17 @@ async function ensureGeminiSkillsInjected( `[paperclip] Removed stale Gemini skill "${skillName}" from ${skillsHome}\n`, ); } + for (const failedEntry of failedToRemove) { + await onLog( + "stderr", + `[paperclip] Failed to remove stale Gemini skill "${failedEntry.name}" from ${skillsHome}; it stays in the managed-skill manifest for a later retry.\n`, + ); + } - const ownedSkillNames: string[] = []; + // Keep every entry this lane still owns but could not remove, so the next + // prune can retry it. None of these names overlap `selectedEntries`: the + // loop above only reports a failure for a name outside that selection. + const ownedSkills: Array<{ name: string; source: string }> = [...failedToRemove]; for (const entry of selectedEntries) { const target = path.join(skillsHome, entry.runtimeName); @@ -192,10 +201,10 @@ async function ensureGeminiSkillsInjected( // the attempt above. A real user directory returns "skipped" too, so // the result string alone cannot tell them apart. if (await isManagedGeminiSkillEntry(target, entry.source)) { - ownedSkillNames.push(entry.runtimeName); + ownedSkills.push({ name: entry.runtimeName, source: path.resolve(entry.source) }); } } - await writeManagedGeminiSkillsManifest(skillsHome, ownedSkillNames); + await writeManagedGeminiSkillsManifest(skillsHome, ownedSkills); } async function buildGeminiSkillsDir( diff --git a/packages/adapters/opencode-local/src/server/execute.ts b/packages/adapters/opencode-local/src/server/execute.ts index dba49d84c8..d006cc9397 100644 --- a/packages/adapters/opencode-local/src/server/execute.ts +++ b/packages/adapters/opencode-local/src/server/execute.ts @@ -176,7 +176,7 @@ async function ensureOpenCodeSkillsInjected( await fs.mkdir(skillsHome, { recursive: true }); const desiredSet = new Set(desiredSkillNames ?? skillsEntries.map((entry) => entry.key)); const selectedEntries = skillsEntries.filter((entry) => desiredSet.has(entry.key)); - const removedSkills = await removeMaintainerOnlySkillSymlinks( + const { removed: removedSkills } = await removeMaintainerOnlySkillSymlinks( skillsHome, selectedEntries.map((entry) => entry.runtimeName), ); diff --git a/packages/adapters/pi-local/src/server/execute.ts b/packages/adapters/pi-local/src/server/execute.ts index 08f80fb991..b5426709f4 100644 --- a/packages/adapters/pi-local/src/server/execute.ts +++ b/packages/adapters/pi-local/src/server/execute.ts @@ -93,7 +93,7 @@ async function ensurePiSkillsInjected( const selectedEntries = skillsEntries.filter((entry) => desiredSet.has(entry.key)); if (selectedEntries.length === 0) return; await fs.mkdir(PI_AGENT_SKILLS_DIR, { recursive: true }); - const removedSkills = await removeMaintainerOnlySkillSymlinks( + const { removed: removedSkills } = await removeMaintainerOnlySkillSymlinks( PI_AGENT_SKILLS_DIR, selectedEntries.map((entry) => entry.runtimeName), ); diff --git a/server/src/__tests__/paperclip-skill-utils.test.ts b/server/src/__tests__/paperclip-skill-utils.test.ts index 10da704274..f95b0d35bf 100644 --- a/server/src/__tests__/paperclip-skill-utils.test.ts +++ b/server/src/__tests__/paperclip-skill-utils.test.ts @@ -574,7 +574,7 @@ describe("paperclip skill utils", () => { await fs.symlink(customSkill, path.join(skillsHome, "release-notes")); await fs.symlink(staleMaintainerSkill, path.join(skillsHome, "release")); - const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, ["paperclip"]); + const { removed } = await removeMaintainerOnlySkillSymlinks(skillsHome, ["paperclip"]); expect(removed).toEqual(["release"]); await expect(fs.lstat(path.join(skillsHome, "release"))).rejects.toThrow();