diff --git a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts index 92de4b3bd..22fbfe50f 100644 --- a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts +++ b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts @@ -90,7 +90,7 @@ import type { IAppMeta } from "@posthog/platform/app-meta"; import type { IBundledResources } from "@posthog/platform/bundled-resources"; import type { IStoragePaths } from "@posthog/platform/storage-paths"; import { PosthogPluginService } from "./posthog-plugin"; -import { syncCodexSkills } from "./update-skills-saga"; +import { overlayDownloadedSkills, syncCodexSkills } from "./update-skills-saga"; /** Expose private members for testing without `as any`. */ interface TestablePluginService { @@ -481,6 +481,80 @@ describe("PosthogPluginService", () => { expect(vol.existsSync(CODEX_SKILLS_DIR)).toBe(false); }); + + it("prunes skills removed from the plugin but keeps foreign skills", async () => { + // First sync: plugin ships two skills. + vol.mkdirSync(`${BUNDLED_PLUGIN_DIR}/skills/skill-a`, { + recursive: true, + }); + vol.writeFileSync(`${BUNDLED_PLUGIN_DIR}/skills/skill-a/SKILL.md`, "# A"); + vol.mkdirSync(`${BUNDLED_PLUGIN_DIR}/skills/skill-b`, { + recursive: true, + }); + vol.writeFileSync(`${BUNDLED_PLUGIN_DIR}/skills/skill-b/SKILL.md`, "# B"); + + // A skill placed in the Codex dir by another tool — must be preserved. + vol.mkdirSync(`${CODEX_SKILLS_DIR}/foreign-skill`, { recursive: true }); + vol.writeFileSync( + `${CODEX_SKILLS_DIR}/foreign-skill/SKILL.md`, + "# Foreign", + ); + + await syncCodexSkills(BUNDLED_PLUGIN_DIR, CODEX_SKILLS_DIR); + + expect(vol.existsSync(`${CODEX_SKILLS_DIR}/skill-a/SKILL.md`)).toBe(true); + expect(vol.existsSync(`${CODEX_SKILLS_DIR}/skill-b/SKILL.md`)).toBe(true); + + // Second sync: skill-b has been removed from the plugin. + vol.rmSync(`${BUNDLED_PLUGIN_DIR}/skills/skill-b`, { recursive: true }); + + await syncCodexSkills(BUNDLED_PLUGIN_DIR, CODEX_SKILLS_DIR); + + expect(vol.existsSync(`${CODEX_SKILLS_DIR}/skill-a/SKILL.md`)).toBe(true); + // Removed from the source → pruned from the Codex dir. + expect(vol.existsSync(`${CODEX_SKILLS_DIR}/skill-b`)).toBe(false); + // Never managed by us → left untouched. + expect(vol.existsSync(`${CODEX_SKILLS_DIR}/foreign-skill/SKILL.md`)).toBe( + true, + ); + }); + }); + + describe("overlayDownloadedSkills", () => { + it("does nothing when the skills cache does not exist", async () => { + await overlayDownloadedSkills("/nonexistent", RUNTIME_PLUGIN_DIR); + + expect(vol.existsSync(`${RUNTIME_PLUGIN_DIR}/skills`)).toBe(false); + }); + + it("prunes skills removed from the cache since the last overlay", async () => { + // First overlay: cache holds two skills. + vol.mkdirSync(`${RUNTIME_SKILLS_DIR}/skill-a`, { recursive: true }); + vol.writeFileSync(`${RUNTIME_SKILLS_DIR}/skill-a/SKILL.md`, "# A"); + vol.mkdirSync(`${RUNTIME_SKILLS_DIR}/skill-b`, { recursive: true }); + vol.writeFileSync(`${RUNTIME_SKILLS_DIR}/skill-b/SKILL.md`, "# B"); + + await overlayDownloadedSkills(RUNTIME_SKILLS_DIR, RUNTIME_PLUGIN_DIR); + + expect( + vol.existsSync(`${RUNTIME_PLUGIN_DIR}/skills/skill-a/SKILL.md`), + ).toBe(true); + expect( + vol.existsSync(`${RUNTIME_PLUGIN_DIR}/skills/skill-b/SKILL.md`), + ).toBe(true); + + // Second overlay: skill-b was removed from the cache (atomic swap). + vol.rmSync(`${RUNTIME_SKILLS_DIR}/skill-b`, { recursive: true }); + + await overlayDownloadedSkills(RUNTIME_SKILLS_DIR, RUNTIME_PLUGIN_DIR); + + expect( + vol.existsSync(`${RUNTIME_PLUGIN_DIR}/skills/skill-a/SKILL.md`), + ).toBe(true); + expect(vol.existsSync(`${RUNTIME_PLUGIN_DIR}/skills/skill-b`)).toBe( + false, + ); + }); }); describe("copyBundledPlugin", () => { diff --git a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts b/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts index c96c547ed..eb1036fee 100644 --- a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts +++ b/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts @@ -12,9 +12,81 @@ import { basename, dirname, join } from "node:path"; import { Saga } from "@posthog/shared"; import { extractZip, unzipAsync } from "./extract-zip"; +/** + * Tracks which skill directories a sync wrote into a destination, so a later + * sync can remove the ones that have since disappeared from the source without + * touching skills it never managed (e.g. skills another tool placed in the + * shared Codex dir). Mirrors the `.sync-manifest` approach used by the + * ai-plugin skill-sync workflow. + */ +const SYNC_MANIFEST_FILE = ".sync-manifest"; + +async function readSyncManifest(destDir: string): Promise { + try { + const content = await readFile(join(destDir, SYNC_MANIFEST_FILE), "utf-8"); + return content + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); + } catch { + return []; + } +} + +async function writeSyncManifest( + destDir: string, + names: string[], +): Promise { + const sorted = [...names].sort(); + await writeFile( + join(destDir, SYNC_MANIFEST_FILE), + sorted.length > 0 ? `${sorted.join("\n")}\n` : "", + ); +} + +/** + * Mirrors the skill directories from `sourceDir` into `destDir`: + * - copies/overwrites each source skill into the destination, and + * - removes any skill this sync previously wrote (tracked in `.sync-manifest`) + * that is no longer present in the source. + * + * Skills in `destDir` that were never written by a previous sync are left + * untouched, so this is safe to run against a directory shared with other tools. + */ +async function syncSkillDirs( + sourceDir: string, + destDir: string, +): Promise { + await mkdir(destDir, { recursive: true }); + + const sourceEntries = await readdir(sourceDir, { withFileTypes: true }); + const sourceNames = sourceEntries + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + const sourceSet = new Set(sourceNames); + + // Remove skills we previously synced that have since vanished from the source. + const previouslySynced = await readSyncManifest(destDir); + for (const name of previouslySynced) { + if (!sourceSet.has(name)) { + await rm(join(destDir, name), { recursive: true, force: true }); + } + } + + // Overlay the current source skills. + for (const name of sourceNames) { + const dest = join(destDir, name); + await rm(dest, { recursive: true, force: true }); + await cp(join(sourceDir, name), dest, { recursive: true }); + } + + await writeSyncManifest(destDir, sourceNames); +} + /** * Overlays previously-downloaded skills on top of the runtime plugin dir. - * Each skill directory in the cache replaces the same-named one in the plugin. + * Each skill directory in the cache replaces the same-named one in the plugin, + * and skills removed from the cache since the last overlay are pruned. */ export async function overlayDownloadedSkills( runtimeSkillsDir: string, @@ -24,22 +96,12 @@ export async function overlayDownloadedSkills( return; } - const destSkillsDir = join(runtimePluginDir, "skills"); - await mkdir(destSkillsDir, { recursive: true }); - - const entries = await readdir(runtimeSkillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (entry.isDirectory()) { - const src = join(runtimeSkillsDir, entry.name); - const dest = join(destSkillsDir, entry.name); - await rm(dest, { recursive: true, force: true }); - await cp(src, dest, { recursive: true }); - } - } + await syncSkillDirs(runtimeSkillsDir, join(runtimePluginDir, "skills")); } /** - * Syncs skills from the effective plugin dir to `codexSkillsDir` for Codex. + * Syncs skills from the effective plugin dir to `codexSkillsDir` for Codex, + * pruning skills removed from the plugin since the last sync. */ export async function syncCodexSkills( pluginPath: string, @@ -51,17 +113,7 @@ export async function syncCodexSkills( } try { - await mkdir(codexSkillsDir, { recursive: true }); - - const entries = await readdir(effectiveSkillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (entry.isDirectory()) { - const src = join(effectiveSkillsDir, entry.name); - const dest = join(codexSkillsDir, entry.name); - await rm(dest, { recursive: true, force: true }); - await cp(src, dest, { recursive: true }); - } - } + await syncSkillDirs(effectiveSkillsDir, codexSkillsDir); } catch { // Fire-and-forget — don't block startup or updates on Codex sync }