diff --git a/packages/adapter-utils/src/server-utils.test.ts b/packages/adapter-utils/src/server-utils.test.ts index 36a748957c8..fd817196f27 100644 --- a/packages/adapter-utils/src/server-utils.test.ts +++ b/packages/adapter-utils/src/server-utils.test.ts @@ -362,16 +362,18 @@ describe("removeMaintainerOnlySkillSymlinks", () => { const skillsHome = path.join(root, "skills"); await fs.mkdir(skillsHome, { recursive: true }); - // A skill this lane materialized as a plain directory (the shape an - // earlier build without the manifest could leave behind) and no - // longer selects. + // A skill this lane materialized as an owned copy (the shape the + // Agent Client Protocol lane's permission-error fallback writes) and + // no longer selects. const staleManagedDir = path.join(skillsHome, "old-skill"); - await fs.mkdir(staleManagedDir, { recursive: true }); + const staleSource = path.join(root, "old-skill-source"); + await fs.mkdir(staleSource, { recursive: true }); await fs.writeFile( - path.join(staleManagedDir, "SKILL.md"), + path.join(staleSource, "SKILL.md"), "# old\n", "utf8", ); + await materializePaperclipSkillCopy(staleSource, staleManagedDir); await writeManagedGeminiSkillsManifest(skillsHome, ["old-skill"]); // An entry the manifest never named: a skill the user put in their @@ -485,6 +487,70 @@ describe("removeMaintainerOnlySkillSymlinks", () => { await fs.rm(root, { recursive: true, force: true }); } }); + + it("keeps a user's own directory that replaced a formerly managed entry between runs", 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"), "# notes\n", "utf8"); + + // 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"]); + + // Between runs, the user removes the link by hand and writes their + // own directory at the same name. + await fs.unlink(target); + await fs.mkdir(target, { recursive: true }); + await fs.writeFile( + path.join(target, "SKILL.md"), + "# my own notes\n", + "utf8", + ); + + // Run 2: the skill is no longer selected. The manifest still names + // "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, []); + + expect(removed).toEqual([]); + await expect( + fs.readFile(path.join(target, "SKILL.md"), "utf8"), + ).resolves.toBe("# my own notes\n"); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it("keeps a manifest-named entry that is a regular file, not a lane-owned shape", async () => { + const root = await fs.mkdtemp( + path.join(os.tmpdir(), "paperclip-gemini-skills-"), + ); + try { + const skillsHome = path.join(root, "skills"); + await fs.mkdir(skillsHome, { recursive: true }); + + const target = path.join(skillsHome, "stray-file"); + await fs.writeFile(target, "not a skill\n", "utf8"); + await writeManagedGeminiSkillsManifest(skillsHome, ["stray-file"]); + + const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []); + + expect(removed).toEqual([]); + await expect(fs.readFile(target, "utf8")).resolves.toBe( + "not a skill\n", + ); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); }); describe("Gemini managed-skills manifest records only owned entries", () => { diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index dde4fa71ec8..c2bbb0604ab 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/packages/adapter-utils/src/server-utils.ts @@ -4505,6 +4505,29 @@ export async function writeManagedGeminiSkillsManifest( ); } +/** + * Test if `target` carries a valid materialized-skill sentinel, the marker + * `materializePaperclipSkillCopy` writes into a directory it owns. + */ +async function hasValidMaterializedSkillSentinel( + target: string, +): Promise { + try { + const raw = JSON.parse( + await fs.readFile( + path.join(target, MATERIALIZED_SKILL_SENTINEL), + "utf8", + ), + ) as unknown; + const parsed = parseObject(raw); + return ( + parsed.version === 1 && typeof parsed.sourceFingerprint === "string" + ); + } catch { + return false; + } +} + /** * Test if the Gemini lane owns the entry at `target` after it materializes * `source` there. Only an owned entry may enter the managed-skills @@ -4537,25 +4560,37 @@ export async function isManagedGeminiSkillEntry( } if (existing.isDirectory()) { - try { - const raw = JSON.parse( - await fs.readFile( - path.join(target, MATERIALIZED_SKILL_SENTINEL), - "utf8", - ), - ) as unknown; - const parsed = parseObject(raw); - return ( - parsed.version === 1 && typeof parsed.sourceFingerprint === "string" - ); - } catch { - return false; - } + return hasValidMaterializedSkillSentinel(target); } return false; } +/** + * 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: + * + * - a symbolic link, of any target; 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. + */ +export async function isLaneOwnedSkillEntryShape( + target: string, +): Promise { + const existing = await fs.lstat(target).catch(() => null); + if (!existing) return false; + if (existing.isSymbolicLink()) return true; + if (existing.isDirectory()) { + return hasValidMaterializedSkillSentinel(target); + } + return false; +} + export async function removeMaintainerOnlySkillSymlinks( skillsHome: string, allowedSkillNames: Iterable, @@ -4570,12 +4605,23 @@ export async function removeMaintainerOnlySkillSymlinks( const target = path.join(skillsHome, entry.name); - // A name in the manifest is a skill this lane owns. Prune it in full — - // a directory, a file, or a symlink to any source — because the lane - // itself wrote it and no longer selects it. + // 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)) { - await fs.rm(target, { recursive: true, force: true }).catch(() => {}); - removed.push(entry.name); + if (await isLaneOwnedSkillEntryShape(target)) { + await fs.rm(target, { recursive: true, force: true }).catch(() => {}); + removed.push(entry.name); + } 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.", + ); + } continue; }