From 4288eee96176ee7d7f4e552f0a717bd04da640fc Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Sun, 5 Apr 2026 14:44:39 -0700 Subject: [PATCH] fix(cleanup): scope stale rename cleanup to legacy compound files Make stale skill and agent removal fingerprint-aware so shared install roots keep user-owned files with overlapping names. Also move Qwen cleanup ahead of agent writes and clean Pi legacy agent directories from skills/ instead of prompts, with regression tests for both cases. --- src/targets/pi.ts | 2 +- src/targets/qwen.ts | 10 +-- src/utils/legacy-cleanup.ts | 153 ++++++++++++++++++++++++++++++++- tests/legacy-cleanup.test.ts | 160 ++++++++++++++++++++++++++++++----- tests/pi-writer.test.ts | 25 ++++++ tests/qwen-writer.test.ts | 22 +++++ 6 files changed, 346 insertions(+), 26 deletions(-) diff --git a/src/targets/pi.ts b/src/targets/pi.ts index b556268f..14e2ab6f 100644 --- a/src/targets/pi.ts +++ b/src/targets/pi.ts @@ -37,7 +37,7 @@ export async function writePiBundle(outputRoot: string, bundle: PiBundle): Promi // TODO(cleanup): Remove after v3 transition (circa Q3 2026) await cleanupStaleSkillDirs(paths.skillsDir) - await cleanupStaleAgents(paths.promptsDir, ".md") + await cleanupStaleAgents(paths.skillsDir, null) for (const prompt of bundle.prompts) { await writeText(path.join(paths.promptsDir, `${sanitizePathName(prompt.name)}.md`), prompt.content + "\n") diff --git a/src/targets/qwen.ts b/src/targets/qwen.ts index 19c567bb..f59149f0 100644 --- a/src/targets/qwen.ts +++ b/src/targets/qwen.ts @@ -21,9 +21,14 @@ export async function writeQwenBundle(outputRoot: string, bundle: QwenBundle): P await writeText(qwenPaths.contextPath, bundle.contextFile + "\n") } + // TODO(cleanup): Remove after v3 transition (circa Q3 2026) + await cleanupStaleSkillDirs(qwenPaths.skillsDir) + // Write agents const agentsDir = qwenPaths.agentsDir await ensureDir(agentsDir) + await cleanupStaleAgents(agentsDir, ".yaml") + await cleanupStaleAgents(agentsDir, ".md") for (const agent of bundle.agents) { const ext = agent.format === "yaml" ? "yaml" : "md" await writeText(path.join(agentsDir, `${sanitizePathName(agent.name)}.${ext}`), agent.content + "\n") @@ -37,11 +42,6 @@ export async function writeQwenBundle(outputRoot: string, bundle: QwenBundle): P await writeText(dest, commandFile.content + "\n") } - // TODO(cleanup): Remove after v3 transition (circa Q3 2026) - await cleanupStaleSkillDirs(qwenPaths.skillsDir) - await cleanupStaleAgents(agentsDir, ".yaml") - await cleanupStaleAgents(agentsDir, ".md") - // Copy skills if (bundle.skillDirs.length > 0) { const skillsRoot = qwenPaths.skillsDir diff --git a/src/utils/legacy-cleanup.ts b/src/utils/legacy-cleanup.ts index b929a3a1..c977581e 100644 --- a/src/utils/legacy-cleanup.ts +++ b/src/utils/legacy-cleanup.ts @@ -15,6 +15,8 @@ import fs from "fs/promises" import path from "path" +import { fileURLToPath } from "url" +import { parseFrontmatter } from "./frontmatter" /** Old skill directory names that no longer exist after the v3 rename. */ const STALE_SKILL_DIRS = [ @@ -128,6 +130,150 @@ const STALE_PROMPT_FILES = [ "ce-work-beta.md", ] +type LegacyFingerprints = { + skills: Map + agents: Map +} + +let legacyFingerprintsPromise: Promise | null = null + +function currentSkillNameForLegacy(legacyName: string): string { + switch (legacyName) { + case "git-commit": + return "ce-commit" + case "git-commit-push-pr": + return "ce-commit-push-pr" + case "git-worktree": + return "ce-worktree" + case "git-clean-gone-branches": + return "ce-clean-gone-branches" + case "report-bug-ce": + return "ce-report-bug" + case "document-review": + case "ce-document-review": + return "ce-doc-review" + case "ce-review": + return "ce-code-review" + default: + return legacyName.startsWith("ce-") ? legacyName : `ce-${legacyName}` + } +} + +async function pathExists(targetPath: string): Promise { + try { + await fs.access(targetPath) + return true + } catch { + return false + } +} + +async function findRepoRoot(startDir: string): Promise { + let current = startDir + while (true) { + const pluginRoot = path.join(current, "plugins", "compound-engineering") + if (await pathExists(pluginRoot)) return current + const parent = path.dirname(current) + if (parent === current) return null + current = parent + } +} + +async function buildSkillIndex(skillsRoot: string): Promise> { + const entries = await fs.readdir(skillsRoot, { withFileTypes: true }) + const index = new Map() + for (const entry of entries) { + if (!entry.isDirectory()) continue + const skillPath = path.join(skillsRoot, entry.name, "SKILL.md") + if (await pathExists(skillPath)) { + index.set(entry.name, skillPath) + } + } + return index +} + +async function buildAgentIndex(dir: string): Promise> { + const index = new Map() + const stack = [dir] + + while (stack.length > 0) { + const current = stack.pop() + if (!current) continue + const entries = await fs.readdir(current, { withFileTypes: true }) + for (const entry of entries) { + const fullPath = path.join(current, entry.name) + if (entry.isDirectory()) { + stack.push(fullPath) + continue + } + if (entry.isFile() && entry.name.endsWith(".md")) { + index.set(path.basename(entry.name, ".md"), fullPath) + } + } + } + + return index +} + +async function readDescription(filePath: string): Promise { + try { + const raw = await fs.readFile(filePath, "utf8") + const { data } = parseFrontmatter(raw, filePath) + return typeof data.description === "string" ? data.description : null + } catch { + return null + } +} + +async function loadLegacyFingerprints(): Promise { + if (!legacyFingerprintsPromise) { + legacyFingerprintsPromise = (async () => { + const repoRoot = await findRepoRoot(path.dirname(fileURLToPath(import.meta.url))) + if (!repoRoot) { + return { skills: new Map(), agents: new Map() } + } + + const pluginRoot = path.join(repoRoot, "plugins", "compound-engineering") + const [skillIndex, agentIndex] = await Promise.all([ + buildSkillIndex(path.join(pluginRoot, "skills")), + buildAgentIndex(path.join(pluginRoot, "agents")), + ]) + + const skills = new Map() + const agents = new Map() + + for (const legacyName of STALE_SKILL_DIRS) { + const currentPath = skillIndex.get(currentSkillNameForLegacy(legacyName)) + if (!currentPath) continue + const description = await readDescription(currentPath) + if (description) skills.set(legacyName, description) + } + + for (const legacyName of STALE_AGENT_NAMES) { + const currentPath = agentIndex.get(`ce-${legacyName}`) + if (!currentPath) continue + const description = await readDescription(currentPath) + if (description) agents.set(legacyName, description) + } + + return { skills, agents } + })() + } + + return legacyFingerprintsPromise +} + +async function isLegacyPluginOwned( + targetPath: string, + expectedDescription: string | undefined, + extension: string | null, +): Promise { + if (!expectedDescription) return false + const filePath = extension === null ? path.join(targetPath, "SKILL.md") : targetPath + const actualDescription = await readDescription(filePath) + return actualDescription === expectedDescription +} + async function removeIfExists(targetPath: string): Promise { try { const stat = await fs.stat(targetPath) @@ -148,9 +294,12 @@ async function removeIfExists(targetPath: string): Promise { * Call before writing new skills. */ export async function cleanupStaleSkillDirs(skillsRoot: string): Promise { + const { skills } = await loadLegacyFingerprints() let removed = 0 for (const name of STALE_SKILL_DIRS) { - if (await removeIfExists(path.join(skillsRoot, name))) removed++ + const targetPath = path.join(skillsRoot, name) + if (!(await isLegacyPluginOwned(targetPath, skills.get(name), null))) continue + if (await removeIfExists(targetPath)) removed++ } return removed } @@ -164,11 +313,13 @@ export async function cleanupStaleAgents( dir: string, extension: string | null, ): Promise { + const { agents } = await loadLegacyFingerprints() let removed = 0 for (const name of STALE_AGENT_NAMES) { const target = extension ? path.join(dir, `${name}${extension}`) : path.join(dir, name) + if (!(await isLegacyPluginOwned(target, agents.get(name), extension))) continue if (await removeIfExists(target)) removed++ } return removed diff --git a/tests/legacy-cleanup.test.ts b/tests/legacy-cleanup.test.ts index 8c65b8ba..3613cbba 100644 --- a/tests/legacy-cleanup.test.ts +++ b/tests/legacy-cleanup.test.ts @@ -2,16 +2,17 @@ import { describe, expect, test } from "bun:test" import fs from "fs/promises" import path from "path" import os from "os" +import { parseFrontmatter } from "../src/utils/frontmatter" import { cleanupStaleSkillDirs, cleanupStaleAgents, cleanupStalePrompts } from "../src/utils/legacy-cleanup" -async function createDir(dir: string) { +async function createDir(dir: string, content = "placeholder") { await fs.mkdir(dir, { recursive: true }) - await fs.writeFile(path.join(dir, "SKILL.md"), "placeholder") + await fs.writeFile(path.join(dir, "SKILL.md"), content) } -async function createFile(filePath: string) { +async function createFile(filePath: string, content = "placeholder") { await fs.mkdir(path.dirname(filePath), { recursive: true }) - await fs.writeFile(filePath, "placeholder") + await fs.writeFile(filePath, content) } async function exists(p: string): Promise { @@ -23,12 +24,47 @@ async function exists(p: string): Promise { } } +async function pluginDescription(relativePath: string): Promise { + const raw = await fs.readFile(path.join(import.meta.dir, "..", relativePath), "utf8") + const { data } = parseFrontmatter(raw, relativePath) + if (typeof data.description !== "string") { + throw new Error(`Missing description in ${relativePath}`) + } + return data.description +} + +function skillContent(name: string, description: string): string { + return `---\nname: ${name}\ndescription: ${JSON.stringify(description)}\n---\n\n# ${name}\n` +} + +function agentContent(name: string, description: string): string { + return `---\nname: ${name}\ndescription: ${JSON.stringify(description)}\n---\n\nBody\n` +} + describe("cleanupStaleSkillDirs", () => { test("removes known stale skill directories", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-skills-")) - await createDir(path.join(root, "git-commit")) - await createDir(path.join(root, "setup")) - await createDir(path.join(root, "document-review")) + await createDir( + path.join(root, "git-commit"), + skillContent( + "git-commit", + await pluginDescription("plugins/compound-engineering/skills/ce-commit/SKILL.md"), + ), + ) + await createDir( + path.join(root, "setup"), + skillContent( + "setup", + await pluginDescription("plugins/compound-engineering/skills/ce-setup/SKILL.md"), + ), + ) + await createDir( + path.join(root, "document-review"), + skillContent( + "document-review", + await pluginDescription("plugins/compound-engineering/skills/ce-doc-review/SKILL.md"), + ), + ) const removed = await cleanupStaleSkillDirs(root) @@ -54,8 +90,20 @@ describe("cleanupStaleSkillDirs", () => { test("removes ce-review and ce-document-review (renamed skills)", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-renamed-")) - await createDir(path.join(root, "ce-review")) - await createDir(path.join(root, "ce-document-review")) + await createDir( + path.join(root, "ce-review"), + skillContent( + "ce-review", + await pluginDescription("plugins/compound-engineering/skills/ce-code-review/SKILL.md"), + ), + ) + await createDir( + path.join(root, "ce-document-review"), + skillContent( + "ce-document-review", + await pluginDescription("plugins/compound-engineering/skills/ce-doc-review/SKILL.md"), + ), + ) const removed = await cleanupStaleSkillDirs(root) @@ -68,13 +116,38 @@ describe("cleanupStaleSkillDirs", () => { const removed = await cleanupStaleSkillDirs("/tmp/nonexistent-cleanup-dir-12345") expect(removed).toBe(0) }) + + test("preserves same-named user skill directories when content does not match plugin fingerprints", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-user-skill-")) + await createDir( + path.join(root, "setup"), + skillContent("setup", "User-owned setup skill unrelated to compound-engineering."), + ) + + const removed = await cleanupStaleSkillDirs(root) + + expect(removed).toBe(0) + expect(await exists(path.join(root, "setup"))).toBe(true) + }) }) describe("cleanupStaleAgents", () => { test("removes flat .md agent files", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-agents-md-")) - await createFile(path.join(root, "adversarial-reviewer.md")) - await createFile(path.join(root, "learnings-researcher.md")) + await createFile( + path.join(root, "adversarial-reviewer.md"), + agentContent( + "adversarial-reviewer", + await pluginDescription("plugins/compound-engineering/agents/review/ce-adversarial-reviewer.md"), + ), + ) + await createFile( + path.join(root, "learnings-researcher.md"), + agentContent( + "learnings-researcher", + await pluginDescription("plugins/compound-engineering/agents/research/ce-learnings-researcher.md"), + ), + ) const removed = await cleanupStaleAgents(root, ".md") @@ -85,8 +158,20 @@ describe("cleanupStaleAgents", () => { test("removes .agent.md files (Copilot format)", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-agents-copilot-")) - await createFile(path.join(root, "security-sentinel.agent.md")) - await createFile(path.join(root, "performance-oracle.agent.md")) + await createFile( + path.join(root, "security-sentinel.agent.md"), + agentContent( + "security-sentinel", + await pluginDescription("plugins/compound-engineering/agents/review/ce-security-sentinel.md"), + ), + ) + await createFile( + path.join(root, "performance-oracle.agent.md"), + agentContent( + "performance-oracle", + await pluginDescription("plugins/compound-engineering/agents/review/ce-performance-oracle.md"), + ), + ) const removed = await cleanupStaleAgents(root, ".agent.md") @@ -96,8 +181,20 @@ describe("cleanupStaleAgents", () => { test("removes agent directories when extension is null", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-agents-dir-")) - await createDir(path.join(root, "code-simplicity-reviewer")) - await createDir(path.join(root, "repo-research-analyst")) + await createDir( + path.join(root, "code-simplicity-reviewer"), + skillContent( + "code-simplicity-reviewer", + await pluginDescription("plugins/compound-engineering/agents/review/ce-code-simplicity-reviewer.md"), + ), + ) + await createDir( + path.join(root, "repo-research-analyst"), + skillContent( + "repo-research-analyst", + await pluginDescription("plugins/compound-engineering/agents/research/ce-repo-research-analyst.md"), + ), + ) const removed = await cleanupStaleAgents(root, null) @@ -108,14 +205,27 @@ describe("cleanupStaleAgents", () => { test("preserves ce-prefixed agent files", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-agents-keep-")) - await createFile(path.join(root, "ce-adversarial-reviewer.md")) - await createFile(path.join(root, "ce-learnings-researcher.md")) + await createFile(path.join(root, "ce-adversarial-reviewer.md"), agentContent("ce-adversarial-reviewer", "custom")) + await createFile(path.join(root, "ce-learnings-researcher.md"), agentContent("ce-learnings-researcher", "custom")) const removed = await cleanupStaleAgents(root, ".md") expect(removed).toBe(0) expect(await exists(path.join(root, "ce-adversarial-reviewer.md"))).toBe(true) }) + + test("preserves same-named user agent files when content does not match plugin fingerprints", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-agents-user-")) + await createFile( + path.join(root, "lint.md"), + agentContent("lint", "A project-local lint helper unrelated to compound-engineering."), + ) + + const removed = await cleanupStaleAgents(root, ".md") + + expect(removed).toBe(0) + expect(await exists(path.join(root, "lint.md"))).toBe(true) + }) }) describe("cleanupStalePrompts", () => { @@ -147,8 +257,20 @@ describe("cleanupStalePrompts", () => { describe("idempotency", () => { test("running cleanup twice returns 0 on second run", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "cleanup-idempotent-")) - await createDir(path.join(root, "git-commit")) - await createFile(path.join(root, "adversarial-reviewer.md")) + await createDir( + path.join(root, "git-commit"), + skillContent( + "git-commit", + await pluginDescription("plugins/compound-engineering/skills/ce-commit/SKILL.md"), + ), + ) + await createFile( + path.join(root, "adversarial-reviewer.md"), + agentContent( + "adversarial-reviewer", + await pluginDescription("plugins/compound-engineering/agents/review/ce-adversarial-reviewer.md"), + ), + ) const first = await cleanupStaleSkillDirs(root) + await cleanupStaleAgents(root, ".md") expect(first).toBe(2) diff --git a/tests/pi-writer.test.ts b/tests/pi-writer.test.ts index 55fecbd5..a4a33fb2 100644 --- a/tests/pi-writer.test.ts +++ b/tests/pi-writer.test.ts @@ -15,6 +15,31 @@ async function exists(filePath: string): Promise { } describe("writePiBundle", () => { + test("removes stale generated agent skills without touching prompt files", async () => { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "pi-cleanup-targets-")) + const outputRoot = path.join(tempRoot, ".pi") + + await fs.mkdir(path.join(outputRoot, "skills", "lint"), { recursive: true }) + await fs.writeFile( + path.join(outputRoot, "skills", "lint", "SKILL.md"), + `---\nname: lint\ndescription: ${JSON.stringify("Use this agent when you need to run linting and code quality checks on Ruby and ERB files. Run before pushing to origin.")}\n---\n\nLegacy agent\n`, + ) + await fs.mkdir(path.join(outputRoot, "prompts"), { recursive: true }) + await fs.writeFile(path.join(outputRoot, "prompts", "lint.md"), "user-owned prompt") + + const bundle: PiBundle = { + prompts: [], + skillDirs: [], + generatedSkills: [], + extensions: [], + } + + await writePiBundle(outputRoot, bundle) + + expect(await exists(path.join(outputRoot, "skills", "lint"))).toBe(false) + expect(await exists(path.join(outputRoot, "prompts", "lint.md"))).toBe(true) + }) + test("writes prompts, skills, extensions, mcporter config, and AGENTS.md block", async () => { const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "pi-writer-")) const outputRoot = path.join(tempRoot, ".pi") diff --git a/tests/qwen-writer.test.ts b/tests/qwen-writer.test.ts index 53f861a1..e14732b1 100644 --- a/tests/qwen-writer.test.ts +++ b/tests/qwen-writer.test.ts @@ -21,7 +21,29 @@ function makeBundle(mcpServers?: Record): QwenBundl } } +const LEGACY_LINT_DESCRIPTION = "Use this agent when you need to run linting and code quality checks on Ruby and ERB files. Run before pushing to origin." + describe("writeQwenBundle", () => { + test("cleans legacy agents before writing new agent files", async () => { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "qwen-agent-cleanup-order-")) + + const bundle: QwenBundle = { + ...makeBundle(), + agents: [ + { + name: "lint", + format: "markdown", + content: `---\nname: lint\ndescription: ${JSON.stringify(LEGACY_LINT_DESCRIPTION)}\n---\n\nReplacement agent\n`, + }, + ], + } + + await writeQwenBundle(tempRoot, bundle) + + const lintPath = path.join(tempRoot, "agents", "lint.md") + expect(await fs.readFile(lintPath, "utf8")).toContain("Replacement agent") + }) + test("removes stale plugin MCP servers on re-install", async () => { const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "qwen-converge-"))