diff --git a/packages/cli/src/utils/skillsMirror.test.ts b/packages/cli/src/utils/skillsMirror.test.ts index 06dccbb033..73fc8f21a8 100644 --- a/packages/cli/src/utils/skillsMirror.test.ts +++ b/packages/cli/src/utils/skillsMirror.test.ts @@ -37,6 +37,15 @@ function seedStore(home: string, skills: string[]): void { } } +/** Seed skill bundles in the universal ~/.agents/skills store. */ +function seedUniversal(home: string, skills: string[]): void { + for (const name of skills) { + const dir = join(home, ".agents", "skills", name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "SKILL.md"), `# ${name}\n`, "utf8"); + } +} + /** Pretend an agent is installed by creating its marker dir. */ function installMarker(home: string, marker: string): void { mkdirSync(join(home, ...marker.split("/")), { recursive: true }); @@ -243,6 +252,108 @@ describe("mirrorGlobalSkills", () => { expect(realpathSync(link)).toBe(realpathSync(join(home, ".claude", "skills", "hyperframes"))); }); + it("does not mirror Codex when the universal store is available", () => { + const home = makeHome(); + seedStore(home, ["hyperframes"]); + installMarker(home, ".codex"); + mkdirSync(join(home, ".agents", "skills", "hyperframes"), { recursive: true }); + writeFileSync(join(home, ".agents", "skills", "hyperframes", "SKILL.md"), "# universal\n"); + mkdirSync(join(home, ".codex", "skills", ".system"), { recursive: true }); + + const { mirrored } = mirrorGlobalSkills({ + skills: ["hyperframes"], + home, + platform: "linux", + env: ENV, + }); + + expect(mirrored.map((entry) => entry.agent)).not.toContain("codex"); + expect(existsSync(join(home, ".codex", "skills", ".system"))).toBe(true); + expect(existsSync(join(home, ".codex", "skills", "hyperframes"))).toBe(false); + }); + + it("does not mirror Codex under a custom CODEX_HOME", () => { + const home = makeHome(); + const codexHome = makeHome(); + seedStore(home, ["hyperframes"]); + installMarker(home, ".codex"); + mkdirSync(join(home, ".agents", "skills", "hyperframes"), { recursive: true }); + writeFileSync(join(home, ".agents", "skills", "hyperframes", "SKILL.md"), "# universal\n"); + const existing = join(codexHome, "skills", "hyperframes"); + mkdirSync(existing, { recursive: true }); + writeFileSync(join(existing, "SKILL.md"), "# locally managed\n", "utf8"); + + const { mirrored } = mirrorGlobalSkills({ + skills: ["hyperframes"], + home, + platform: "linux", + env: { CODEX_HOME: codexHome }, + }); + + expect(mirrored.map((entry) => entry.agent)).not.toContain("codex"); + expect(readFileSync(join(existing, "SKILL.md"), "utf8")).toBe("# locally managed\n"); + }); + + it("removes Codex links an earlier mirror created and keeps the user's own entries", () => { + const home = makeHome(); + seedStore(home, ["hyperframes", "media-use"]); + const codexSkills = join(home, ".codex", "skills"); + mkdirSync(join(codexSkills, ".system"), { recursive: true }); + mkdirSync(join(codexSkills, "my-skill"), { recursive: true }); + // What main's mirror wrote: a relative link back into the Claude store. + symlinkSync( + join("..", "..", ".claude", "skills", "hyperframes"), + join(codexSkills, "hyperframes"), + ); + // Same skill name, but the user's own link to somewhere else. + const userCopy = join(home, "my-media-use"); + mkdirSync(userCopy); + symlinkSync(userCopy, join(codexSkills, "media-use")); + seedUniversal(home, ["hyperframes", "media-use"]); + + mirrorGlobalSkills({ skills: ["hyperframes", "media-use"], home, platform: "linux", env: ENV }); + + expect(existsSync(join(codexSkills, "hyperframes"))).toBe(false); + expect(realpathSync(join(codexSkills, "media-use"))).toBe(realpathSync(userCopy)); + expect(existsSync(join(codexSkills, ".system"))).toBe(true); + expect(existsSync(join(codexSkills, "my-skill"))).toBe(true); + expect(existsSync(join(home, ".claude", "skills", "hyperframes", "SKILL.md"))).toBe(true); + }); + + it("keeps a Codex link when the universal store lacks that skill", () => { + const home = makeHome(); + seedStore(home, ["hyperframes"]); + const codexLink = join(home, ".codex", "skills", "hyperframes"); + mkdirSync(join(home, ".codex", "skills"), { recursive: true }); + symlinkSync(join("..", "..", ".claude", "skills", "hyperframes"), codexLink); + + mirrorGlobalSkills({ skills: ["hyperframes"], home, platform: "linux", env: ENV }); + + expect(lstatSync(codexLink).isSymbolicLink()).toBe(true); + }); + + it("never unlinks through a Codex dir that aliases the Claude store", () => { + const home = makeHome(); + const checkout = join(home, "checkout", "hyperframes"); + mkdirSync(checkout, { recursive: true }); + writeFileSync(join(checkout, "SKILL.md"), "# checkout\n", "utf8"); + mkdirSync(join(home, ".claude", "skills"), { recursive: true }); + symlinkSync(checkout, join(home, ".claude", "skills", "hyperframes")); + mkdirSync(join(home, ".codex"), { recursive: true }); + symlinkSync(join("..", ".claude", "skills"), join(home, ".codex", "skills")); + seedUniversal(home, ["hyperframes"]); + + const { skipped } = mirrorGlobalSkills({ + skills: ["hyperframes"], + home, + platform: "linux", + env: ENV, + }); + + expect(lstatSync(join(home, ".claude", "skills", "hyperframes")).isSymbolicLink()).toBe(true); + expect(skipped).toContainEqual(expect.objectContaining({ agent: "codex" })); + }); + // Pi natively discovers BOTH ~/.pi/agent/skills and the universal // ~/.agents/skills (pi's packages/coding-agent/docs/skills.md#locations). // A mirrored per-agent copy collides with the universal one and Pi skips diff --git a/packages/cli/src/utils/skillsMirror.ts b/packages/cli/src/utils/skillsMirror.ts index 50985b6b4a..e37ff06e3c 100644 --- a/packages/cli/src/utils/skillsMirror.ts +++ b/packages/cli/src/utils/skillsMirror.ts @@ -3,14 +3,14 @@ // `skills add --global --agent claude-code universal --copy` writes REAL files // to two global stores: the Claude store (~/.claude/skills — what Claude Code // reads, at global priority) and the shared universal store (~/.agents/skills, -// which Cursor/Codex/… read in PROJECT scope and the .agents-family agents read -// globally). But every other agent reads its OWN global dir (~/.cursor/skills, +// which Cursor/… read in PROJECT scope and Codex, Pi and the .agents-family +// agents read globally). But every other agent reads its OWN global dir (~/.cursor/skills, // goose → ~/.config/goose/skills, …), which upstream's --global does NOT // populate. // // So we mirror the canonical Claude store into each of those per-agent dirs, but // only for agents the machine actually has (their marker dir exists). Agents -// that already consume the universal ~/.agents/skills store globally (Pi) are +// that already consume the universal ~/.agents/skills store globally (Pi, Codex) are // skipped: their universal copy is authoritative and a per-agent copy would // collide with it (#3294). On Unix // each skill is a relative symlink back into the store (one source of truth, @@ -32,6 +32,7 @@ import { rmSync, statSync, symlinkSync, + unlinkSync, } from "node:fs"; import { homedir } from "node:os"; import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; @@ -42,16 +43,17 @@ import { AGENT_GLOBAL_DIRS, type AgentDirBase } from "./agentDirs.generated.js"; * in ADDITION to their own agent-specific directory. Mirroring into their own * dir makes every skill discoverable twice. * - * Pi is the known case (earendil-works/pi): it reads both `~/.pi/agent/skills/` + * Pi and Codex are known cases. Pi (earendil-works/pi) reads both `~/.pi/agent/skills/` * and `~/.agents/skills/` as global locations (pi's packages/coding-agent/docs/ * skills.md#locations), so a mirrored entry collides with the universal copy - * and Pi skips the universal one on name conflict (#3294). + * and Pi skips the universal one on name conflict (#3294). Codex likewise + * discovers the user-level `~/.agents/skills/` store. * * The generated table cannot carry this capability — it is a plain * (agent, base, sub) list synced from vercel-labs/skills — so the set lives * here next to the mirror logic that needs it. */ -const UNIVERSAL_STORE_READERS = new Set(["pi"]); +const UNIVERSAL_STORE_READERS = new Set(["pi", "codex"]); export interface MirrorResult { /** The store mirrored from, or null when no global Claude store was found. */ @@ -236,6 +238,33 @@ function mirrorInto( return { mirrored: true }; } +/** + * Unlink symlinks an earlier mirror left in `targetDir` (they resolve to the same source skill), + * only where the universal store holds that skill. Real dirs and Windows copies stay. + */ +function removeMirrorLinks( + targetDir: string, + source: string, + universalStore: string, + skills: string[], + safety: () => MirrorSkipReason | null, +): MirrorSkipReason | null { + const unsafe = safety(); + if (unsafe) return unsafe; + for (const skill of skills) { + const targetSkill = join(targetDir, skill); + try { + if (!existsSync(join(universalStore, skill, "SKILL.md"))) continue; + if (!lstatSync(targetSkill).isSymbolicLink()) continue; + if (realpathSync(targetSkill) !== realpathSync(join(source, skill))) continue; + unlinkSync(targetSkill); + } catch { + // absent or dangling: nothing of ours to remove + } + } + return null; +} + /** * Mirror the global Claude store into every installed agent's global skills * dir. Best-effort and idempotent: a no-op when the store is absent, and per @@ -282,7 +311,14 @@ export function mirrorGlobalSkills(opts: { for (const { agent, base, sub } of AGENT_GLOBAL_DIRS) { const targetDir = join(bases[base], ...sub.split("/").filter(Boolean)); if (targetDir === source || targetDir === universalStore) continue; // install-owned - if (UNIVERSAL_STORE_READERS.has(agent)) continue; // already reads the universal store (#3294) + if (UNIVERSAL_STORE_READERS.has(agent)) { + // Already reads the universal store (#3294); drop links an older version mirrored here. + const skipReason = removeMirrorLinks(targetDir, source, universalStore, skills, () => + targetSafety(targetDir, resolvedProtectedPaths), + ); + if (skipReason) skipped.push({ agent, dir: targetDir, reason: skipReason }); + continue; + } if (!existsSync(dirname(targetDir))) continue; // agent not installed (no marker) const attempt = mirrorInto(targetDir, source, skills, platform, () => targetSafety(targetDir, resolvedProtectedPaths),