fix(skills): stop mirroring skills into Codex and remove old mirror links (#4105)

* fix(skills): avoid duplicate Codex skill mirrors * fix(skills): remove Codex mirror links left by earlier skill updates * fix(skills): guard the Codex mirror cleanup and keep links without a universal copy --------- Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com>
2026-10-30 09:16:55 +00:00 · 2026-10-02 08:05:44 -07:00
parent 67dbcf7bdb
commit d865b5a7cb
2 changed files with 154 additions and 7 deletions
@@ -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
@@ -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),