mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 12:59:13 +01:00
fix(adapter-utils): prune managed Gemini skills by manifest, not symlink guess
The Gemini skills home is the user's own directory. A prior fix only removed a stale entry when it was a symlink into a maintainer-only source, so a deselected skill materialized as a plain directory, or as a symlink into any other source, stayed on disk and still reached the agent. Record the names this lane materializes in a manifest next to the skills, and prune a stale entry only when the manifest names it. An entry the manifest never named — the user's own skill — always survives. The old symlink-into-a-maintainer-only-source check stays as a fallback for a skills home written before this manifest existed. Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
f70accd3a4
commit
ee333e1219
4 files changed
+212
-2
No files matched your search
@@ -68,6 +68,7 @@ import {
|
||||
selectPaperclipTaskMarkdown,
|
||||
resolveLegacyPaperclipDesiredSkillNames,
|
||||
removeMaintainerOnlySkillSymlinks,
|
||||
writeManagedGeminiSkillsManifest,
|
||||
rewriteWorkspaceCwdEnvVarsForExecution,
|
||||
shapePaperclipWorkspaceEnvForExecution,
|
||||
stringifyPaperclipWakePayload,
|
||||
@@ -1313,7 +1314,7 @@ async function prepareGeminiSkillRuntime(input: {
|
||||
const allowedSkillNames = selectedSkills.map((entry) => entry.runtimeName);
|
||||
const removedSkills = await removeMaintainerOnlySkillSymlinks(skillsHome, allowedSkillNames);
|
||||
for (const skillName of removedSkills) {
|
||||
await input.onLog("stdout", `[paperclip] Removed maintainer-only ACPX Gemini skill "${skillName}" from ${skillsHome}\n`);
|
||||
await input.onLog("stdout", `[paperclip] Removed stale ACPX Gemini skill "${skillName}" from ${skillsHome}\n`);
|
||||
}
|
||||
|
||||
for (const entry of selectedSkills) {
|
||||
@@ -1341,6 +1342,7 @@ async function prepareGeminiSkillRuntime(input: {
|
||||
);
|
||||
}
|
||||
}
|
||||
await writeManagedGeminiSkillsManifest(skillsHome, allowedSkillNames);
|
||||
|
||||
return {
|
||||
identity: {
|
||||
|
||||
@@ -20,12 +20,14 @@ import {
|
||||
materializePaperclipSkillCopy,
|
||||
PAPERCLIP_OPERATIONAL_SKILL_KEY,
|
||||
refreshPaperclipWorkspaceEnvForExecution,
|
||||
removeMaintainerOnlySkillSymlinks,
|
||||
renderPaperclipWakePrompt,
|
||||
resolveLegacyPaperclipDesiredSkillNames,
|
||||
resolvePaperclipDesiredSkillNames,
|
||||
selectPaperclipTaskMarkdown,
|
||||
runningProcesses,
|
||||
runChildProcess,
|
||||
writeManagedGeminiSkillsManifest,
|
||||
sanitizeSshRemoteEnv,
|
||||
signalRunningProcess,
|
||||
shapePaperclipWorkspaceEnvForExecution,
|
||||
@@ -349,6 +351,140 @@ describe("materializePaperclipSkillCopy", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("removeMaintainerOnlySkillSymlinks", () => {
|
||||
it("prunes a stale managed directory the manifest names, and keeps an unmanaged directory", 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 });
|
||||
|
||||
// A skill this lane materialized as a plain directory (the shape an
|
||||
// earlier build without the manifest could leave behind) and no
|
||||
// longer selects.
|
||||
const staleManagedDir = path.join(skillsHome, "old-skill");
|
||||
await fs.mkdir(staleManagedDir, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(staleManagedDir, "SKILL.md"),
|
||||
"# old\n",
|
||||
"utf8",
|
||||
);
|
||||
await writeManagedGeminiSkillsManifest(skillsHome, ["old-skill"]);
|
||||
|
||||
// An entry the manifest never named: a skill the user put in their
|
||||
// own Gemini skills home, not one Paperclip materialized.
|
||||
const unmanagedDir = path.join(skillsHome, "users-own-skill");
|
||||
await fs.mkdir(unmanagedDir, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(unmanagedDir, "SKILL.md"),
|
||||
"# mine\n",
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []);
|
||||
|
||||
expect(removed).toEqual(["old-skill"]);
|
||||
await expect(fs.stat(staleManagedDir)).rejects.toThrow();
|
||||
await expect(
|
||||
fs.readFile(path.join(unmanagedDir, "SKILL.md"), "utf8"),
|
||||
).resolves.toBe("# mine\n");
|
||||
} finally {
|
||||
await fs.rm(root, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("prunes a stale managed symlink the manifest names", 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, ["linked-skill"]);
|
||||
|
||||
const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []);
|
||||
|
||||
expect(removed).toEqual(["linked-skill"]);
|
||||
await expect(fs.lstat(target)).rejects.toThrow();
|
||||
// The link is gone, but the skill source it pointed to must survive —
|
||||
// this lane only ever prunes its own target directory.
|
||||
await expect(
|
||||
fs.readFile(path.join(source, "SKILL.md"), "utf8"),
|
||||
).resolves.toBe("# skill\n");
|
||||
} 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-"),
|
||||
);
|
||||
try {
|
||||
const skillsHome = path.join(root, "skills");
|
||||
await fs.mkdir(skillsHome, { recursive: true });
|
||||
const unmanagedDir = path.join(skillsHome, "users-own-skill");
|
||||
await fs.mkdir(unmanagedDir, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(unmanagedDir, "SKILL.md"),
|
||||
"# mine\n",
|
||||
"utf8",
|
||||
);
|
||||
// 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, []);
|
||||
|
||||
expect(removed).toEqual([]);
|
||||
await expect(
|
||||
fs.readFile(path.join(unmanagedDir, "SKILL.md"), "utf8"),
|
||||
).resolves.toBe("# mine\n");
|
||||
} finally {
|
||||
await fs.rm(root, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("still removes a legacy maintainer-only symlink absent from any manifest", async () => {
|
||||
const root = await fs.mkdtemp(
|
||||
path.join(os.tmpdir(), "paperclip-gemini-skills-"),
|
||||
);
|
||||
try {
|
||||
const skillsHome = path.join(root, "skills");
|
||||
const maintainerSource = path.join(
|
||||
root,
|
||||
".agents",
|
||||
"skills",
|
||||
"maintainer-skill",
|
||||
);
|
||||
await fs.mkdir(skillsHome, { recursive: true });
|
||||
await fs.mkdir(maintainerSource, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(maintainerSource, "SKILL.md"),
|
||||
"# maintainer\n",
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const target = path.join(skillsHome, "maintainer-skill");
|
||||
await fs.symlink(maintainerSource, target);
|
||||
// No manifest is written: this covers the pre-manifest legacy case.
|
||||
|
||||
const removed = await removeMaintainerOnlySkillSymlinks(skillsHome, []);
|
||||
|
||||
expect(removed).toEqual(["maintainer-skill"]);
|
||||
await expect(fs.lstat(target)).rejects.toThrow();
|
||||
} finally {
|
||||
await fs.rm(root, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("adapter skill snapshots", () => {
|
||||
const requiredEntry = {
|
||||
key: "paperclipai/paperclip/paperclip",
|
||||
|
||||
@@ -174,6 +174,7 @@ const PAPERCLIP_SKILL_ROOT_RELATIVE_CANDIDATES = [
|
||||
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";
|
||||
|
||||
function expandHomePrefix(value: string): string {
|
||||
if (value === "~") return os.homedir();
|
||||
@@ -4453,11 +4454,63 @@ 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.
|
||||
*/
|
||||
export async function readManagedGeminiSkillsManifest(
|
||||
skillsHome: string,
|
||||
): Promise<Set<string>> {
|
||||
try {
|
||||
const raw = JSON.parse(
|
||||
await fs.readFile(
|
||||
path.join(skillsHome, MANAGED_GEMINI_SKILLS_MANIFEST),
|
||||
"utf8",
|
||||
),
|
||||
) 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,
|
||||
)
|
||||
: [];
|
||||
return new Set(names);
|
||||
} catch {
|
||||
return new Set();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* 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.
|
||||
*/
|
||||
export async function writeManagedGeminiSkillsManifest(
|
||||
skillsHome: string,
|
||||
skillNames: Iterable<string>,
|
||||
): Promise<void> {
|
||||
const managedSkillNames = Array.from(new Set(skillNames)).sort();
|
||||
await fs.writeFile(
|
||||
path.join(skillsHome, MANAGED_GEMINI_SKILLS_MANIFEST),
|
||||
`${JSON.stringify({ version: 1, managedSkillNames }, null, 2)}\n`,
|
||||
"utf8",
|
||||
);
|
||||
}
|
||||
|
||||
export async function removeMaintainerOnlySkillSymlinks(
|
||||
skillsHome: string,
|
||||
allowedSkillNames: Iterable<string>,
|
||||
): Promise<string[]> {
|
||||
const allowed = new Set(Array.from(allowedSkillNames));
|
||||
const managed = await readManagedGeminiSkillsManifest(skillsHome);
|
||||
try {
|
||||
const entries = await fs.readdir(skillsHome, { withFileTypes: true });
|
||||
const removed: string[] = [];
|
||||
@@ -4465,6 +4518,20 @@ export async function removeMaintainerOnlySkillSymlinks(
|
||||
if (allowed.has(entry.name)) continue;
|
||||
|
||||
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.
|
||||
if (managed.has(entry.name)) {
|
||||
await fs.rm(target, { recursive: true, force: true }).catch(() => {});
|
||||
removed.push(entry.name);
|
||||
continue;
|
||||
}
|
||||
|
||||
// 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;
|
||||
|
||||
|
||||
@@ -44,6 +44,7 @@ import {
|
||||
readPaperclipIssueWorkModeFromContext,
|
||||
resolveLegacyPaperclipDesiredSkillNames,
|
||||
removeMaintainerOnlySkillSymlinks,
|
||||
writeManagedGeminiSkillsManifest,
|
||||
parseObject,
|
||||
renderTemplate,
|
||||
renderPaperclipWakePrompt,
|
||||
@@ -164,7 +165,7 @@ async function ensureGeminiSkillsInjected(
|
||||
for (const skillName of removedSkills) {
|
||||
await onLog(
|
||||
"stderr",
|
||||
`[paperclip] Removed maintainer-only Gemini skill "${skillName}" from ${skillsHome}\n`,
|
||||
`[paperclip] Removed stale Gemini skill "${skillName}" from ${skillsHome}\n`,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -185,6 +186,10 @@ async function ensureGeminiSkillsInjected(
|
||||
);
|
||||
}
|
||||
}
|
||||
await writeManagedGeminiSkillsManifest(
|
||||
skillsHome,
|
||||
selectedEntries.map((entry) => entry.runtimeName),
|
||||
);
|
||||
}
|
||||
|
||||
async function buildGeminiSkillsDir(
|
||||
|
||||
Reference in new issue
Block a user