Skip to content

Commit d865b5a

Browse files
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>
1 parent 67dbcf7 commit d865b5a

2 files changed

Lines changed: 154 additions & 7 deletions

File tree

‎packages/cli/src/utils/skillsMirror.test.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,15 @@ function seedStore(home: string, skills: string[]): void {
3737
}
3838
}
3939

40+
/** Seed skill bundles in the universal ~/.agents/skills store. */
41+
function seedUniversal(home: string, skills: string[]): void {
42+
for (const name of skills) {
43+
const dir = join(home, ".agents", "skills", name);
44+
mkdirSync(dir, { recursive: true });
45+
writeFileSync(join(dir, "SKILL.md"), `# ${name}\n`, "utf8");
46+
}
47+
}
48+
4049
/** Pretend an agent is installed by creating its marker dir. */
4150
function installMarker(home: string, marker: string): void {
4251
mkdirSync(join(home, ...marker.split("/")), { recursive: true });
@@ -243,6 +252,108 @@ describe("mirrorGlobalSkills", () => {
243252
expect(realpathSync(link)).toBe(realpathSync(join(home, ".claude", "skills", "hyperframes")));
244253
});
245254

255+
it("does not mirror Codex when the universal store is available", () => {
256+
const home = makeHome();
257+
seedStore(home, ["hyperframes"]);
258+
installMarker(home, ".codex");
259+
mkdirSync(join(home, ".agents", "skills", "hyperframes"), { recursive: true });
260+
writeFileSync(join(home, ".agents", "skills", "hyperframes", "SKILL.md"), "# universal\n");
261+
mkdirSync(join(home, ".codex", "skills", ".system"), { recursive: true });
262+
263+
const { mirrored } = mirrorGlobalSkills({
264+
skills: ["hyperframes"],
265+
home,
266+
platform: "linux",
267+
env: ENV,
268+
});
269+
270+
expect(mirrored.map((entry) => entry.agent)).not.toContain("codex");
271+
expect(existsSync(join(home, ".codex", "skills", ".system"))).toBe(true);
272+
expect(existsSync(join(home, ".codex", "skills", "hyperframes"))).toBe(false);
273+
});
274+
275+
it("does not mirror Codex under a custom CODEX_HOME", () => {
276+
const home = makeHome();
277+
const codexHome = makeHome();
278+
seedStore(home, ["hyperframes"]);
279+
installMarker(home, ".codex");
280+
mkdirSync(join(home, ".agents", "skills", "hyperframes"), { recursive: true });
281+
writeFileSync(join(home, ".agents", "skills", "hyperframes", "SKILL.md"), "# universal\n");
282+
const existing = join(codexHome, "skills", "hyperframes");
283+
mkdirSync(existing, { recursive: true });
284+
writeFileSync(join(existing, "SKILL.md"), "# locally managed\n", "utf8");
285+
286+
const { mirrored } = mirrorGlobalSkills({
287+
skills: ["hyperframes"],
288+
home,
289+
platform: "linux",
290+
env: { CODEX_HOME: codexHome },
291+
});
292+
293+
expect(mirrored.map((entry) => entry.agent)).not.toContain("codex");
294+
expect(readFileSync(join(existing, "SKILL.md"), "utf8")).toBe("# locally managed\n");
295+
});
296+
297+
it("removes Codex links an earlier mirror created and keeps the user's own entries", () => {
298+
const home = makeHome();
299+
seedStore(home, ["hyperframes", "media-use"]);
300+
const codexSkills = join(home, ".codex", "skills");
301+
mkdirSync(join(codexSkills, ".system"), { recursive: true });
302+
mkdirSync(join(codexSkills, "my-skill"), { recursive: true });
303+
// What main's mirror wrote: a relative link back into the Claude store.
304+
symlinkSync(
305+
join("..", "..", ".claude", "skills", "hyperframes"),
306+
join(codexSkills, "hyperframes"),
307+
);
308+
// Same skill name, but the user's own link to somewhere else.
309+
const userCopy = join(home, "my-media-use");
310+
mkdirSync(userCopy);
311+
symlinkSync(userCopy, join(codexSkills, "media-use"));
312+
seedUniversal(home, ["hyperframes", "media-use"]);
313+
314+
mirrorGlobalSkills({ skills: ["hyperframes", "media-use"], home, platform: "linux", env: ENV });
315+
316+
expect(existsSync(join(codexSkills, "hyperframes"))).toBe(false);
317+
expect(realpathSync(join(codexSkills, "media-use"))).toBe(realpathSync(userCopy));
318+
expect(existsSync(join(codexSkills, ".system"))).toBe(true);
319+
expect(existsSync(join(codexSkills, "my-skill"))).toBe(true);
320+
expect(existsSync(join(home, ".claude", "skills", "hyperframes", "SKILL.md"))).toBe(true);
321+
});
322+
323+
it("keeps a Codex link when the universal store lacks that skill", () => {
324+
const home = makeHome();
325+
seedStore(home, ["hyperframes"]);
326+
const codexLink = join(home, ".codex", "skills", "hyperframes");
327+
mkdirSync(join(home, ".codex", "skills"), { recursive: true });
328+
symlinkSync(join("..", "..", ".claude", "skills", "hyperframes"), codexLink);
329+
330+
mirrorGlobalSkills({ skills: ["hyperframes"], home, platform: "linux", env: ENV });
331+
332+
expect(lstatSync(codexLink).isSymbolicLink()).toBe(true);
333+
});
334+
335+
it("never unlinks through a Codex dir that aliases the Claude store", () => {
336+
const home = makeHome();
337+
const checkout = join(home, "checkout", "hyperframes");
338+
mkdirSync(checkout, { recursive: true });
339+
writeFileSync(join(checkout, "SKILL.md"), "# checkout\n", "utf8");
340+
mkdirSync(join(home, ".claude", "skills"), { recursive: true });
341+
symlinkSync(checkout, join(home, ".claude", "skills", "hyperframes"));
342+
mkdirSync(join(home, ".codex"), { recursive: true });
343+
symlinkSync(join("..", ".claude", "skills"), join(home, ".codex", "skills"));
344+
seedUniversal(home, ["hyperframes"]);
345+
346+
const { skipped } = mirrorGlobalSkills({
347+
skills: ["hyperframes"],
348+
home,
349+
platform: "linux",
350+
env: ENV,
351+
});
352+
353+
expect(lstatSync(join(home, ".claude", "skills", "hyperframes")).isSymbolicLink()).toBe(true);
354+
expect(skipped).toContainEqual(expect.objectContaining({ agent: "codex" }));
355+
});
356+
246357
// Pi natively discovers BOTH ~/.pi/agent/skills and the universal
247358
// ~/.agents/skills (pi's packages/coding-agent/docs/skills.md#locations).
248359
// A mirrored per-agent copy collides with the universal one and Pi skips

‎packages/cli/src/utils/skillsMirror.ts‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@
33
// `skills add --global --agent claude-code universal --copy` writes REAL files
44
// to two global stores: the Claude store (~/.claude/skills — what Claude Code
55
// reads, at global priority) and the shared universal store (~/.agents/skills,
6-
// which Cursor/Codex/… read in PROJECT scope and the .agents-family agents read
7-
// globally). But every other agent reads its OWN global dir (~/.cursor/skills,
6+
// which Cursor/… read in PROJECT scope and Codex, Pi and the .agents-family
7+
// agents read globally). But every other agent reads its OWN global dir (~/.cursor/skills,
88
// goose → ~/.config/goose/skills, …), which upstream's --global does NOT
99
// populate.
1010
//
1111
// So we mirror the canonical Claude store into each of those per-agent dirs, but
1212
// only for agents the machine actually has (their marker dir exists). Agents
13-
// that already consume the universal ~/.agents/skills store globally (Pi) are
13+
// that already consume the universal ~/.agents/skills store globally (Pi, Codex) are
1414
// skipped: their universal copy is authoritative and a per-agent copy would
1515
// collide with it (#3294). On Unix
1616
// each skill is a relative symlink back into the store (one source of truth,
@@ -32,6 +32,7 @@ import {
3232
rmSync,
3333
statSync,
3434
symlinkSync,
35+
unlinkSync,
3536
} from "node:fs";
3637
import { homedir } from "node:os";
3738
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";
4243
* in ADDITION to their own agent-specific directory. Mirroring into their own
4344
* dir makes every skill discoverable twice.
4445
*
45-
* Pi is the known case (earendil-works/pi): it reads both `~/.pi/agent/skills/`
46+
* Pi and Codex are known cases. Pi (earendil-works/pi) reads both `~/.pi/agent/skills/`
4647
* and `~/.agents/skills/` as global locations (pi's packages/coding-agent/docs/
4748
* skills.md#locations), so a mirrored entry collides with the universal copy
48-
* and Pi skips the universal one on name conflict (#3294).
49+
* and Pi skips the universal one on name conflict (#3294). Codex likewise
50+
* discovers the user-level `~/.agents/skills/` store.
4951
*
5052
* The generated table cannot carry this capability — it is a plain
5153
* (agent, base, sub) list synced from vercel-labs/skills — so the set lives
5254
* here next to the mirror logic that needs it.
5355
*/
54-
const UNIVERSAL_STORE_READERS = new Set(["pi"]);
56+
const UNIVERSAL_STORE_READERS = new Set(["pi", "codex"]);
5557

5658
export interface MirrorResult {
5759
/** The store mirrored from, or null when no global Claude store was found. */
@@ -236,6 +238,33 @@ function mirrorInto(
236238
return { mirrored: true };
237239
}
238240

241+
/**
242+
* Unlink symlinks an earlier mirror left in `targetDir` (they resolve to the same source skill),
243+
* only where the universal store holds that skill. Real dirs and Windows copies stay.
244+
*/
245+
function removeMirrorLinks(
246+
targetDir: string,
247+
source: string,
248+
universalStore: string,
249+
skills: string[],
250+
safety: () => MirrorSkipReason | null,
251+
): MirrorSkipReason | null {
252+
const unsafe = safety();
253+
if (unsafe) return unsafe;
254+
for (const skill of skills) {
255+
const targetSkill = join(targetDir, skill);
256+
try {
257+
if (!existsSync(join(universalStore, skill, "SKILL.md"))) continue;
258+
if (!lstatSync(targetSkill).isSymbolicLink()) continue;
259+
if (realpathSync(targetSkill) !== realpathSync(join(source, skill))) continue;
260+
unlinkSync(targetSkill);
261+
} catch {
262+
// absent or dangling: nothing of ours to remove
263+
}
264+
}
265+
return null;
266+
}
267+
239268
/**
240269
* Mirror the global Claude store into every installed agent's global skills
241270
* dir. Best-effort and idempotent: a no-op when the store is absent, and per
@@ -282,7 +311,14 @@ export function mirrorGlobalSkills(opts: {
282311
for (const { agent, base, sub } of AGENT_GLOBAL_DIRS) {
283312
const targetDir = join(bases[base], ...sub.split("/").filter(Boolean));
284313
if (targetDir === source || targetDir === universalStore) continue; // install-owned
285-
if (UNIVERSAL_STORE_READERS.has(agent)) continue; // already reads the universal store (#3294)
314+
if (UNIVERSAL_STORE_READERS.has(agent)) {
315+
// Already reads the universal store (#3294); drop links an older version mirrored here.
316+
const skipReason = removeMirrorLinks(targetDir, source, universalStore, skills, () =>
317+
targetSafety(targetDir, resolvedProtectedPaths),
318+
);
319+
if (skipReason) skipped.push({ agent, dir: targetDir, reason: skipReason });
320+
continue;
321+
}
286322
if (!existsSync(dirname(targetDir))) continue; // agent not installed (no marker)
287323
const attempt = mirrorInto(targetDir, source, skills, platform, () =>
288324
targetSafety(targetDir, resolvedProtectedPaths),

0 commit comments

Comments
 (0)