diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 6f9b5bfda9..20591e09bd 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -119,7 +119,7 @@ vi.mock("../utils/skillsManifest.js", async (importOriginal) => { // the real $HOME. Stub it so these arg-shape tests never create symlinks in the // dev machine's agent dirs — the mirror has its own isolated-HOME unit tests. vi.mock("../utils/skillsMirror.js", () => ({ - mirrorGlobalSkills: vi.fn(() => ({ source: null, mirrored: [] })), + mirrorGlobalSkills: vi.fn(() => ({ source: null, mirrored: [], skipped: [] })), })); // The reconcile commands drop the background nudge's cached verdict on diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index 6a7c188380..5920bc27c4 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -176,7 +176,7 @@ function mirrorToInstalledAgents(): void { try { const names = hyperframesSkillNames({ scope: "global" }); if (names.length === 0) return; - const { mirrored } = mirrorGlobalSkills({ skills: names }); + const { mirrored, skipped } = mirrorGlobalSkills({ skills: names }); const n = mirrored.length; if (n > 0) { // stderr (via diag): reachable from `skills update --json` (via installSkills) @@ -185,6 +185,14 @@ function mirrorToInstalledAgents(): void { c.dim(`Linked skills into ${n} other agent ${n === 1 ? "directory" : "directories"}.`), ); } + if (skipped.length > 0) { + const agents = [...new Set(skipped.map((entry) => entry.agent))].join(", "); + diag.warn( + c.warn( + `Skipped unsafe skill mirror target${skipped.length === 1 ? "" : "s"} for ${agents}; canonical skill stores were left unchanged.`, + ), + ); + } } catch { // best-effort } diff --git a/packages/cli/src/utils/skillsMirror.test.ts b/packages/cli/src/utils/skillsMirror.test.ts index daf4b1b99e..ae5d60f967 100644 --- a/packages/cli/src/utils/skillsMirror.test.ts +++ b/packages/cli/src/utils/skillsMirror.test.ts @@ -8,6 +8,7 @@ import { readFileSync, realpathSync, rmSync, + symlinkSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; @@ -46,6 +47,62 @@ afterEach(() => { }); describe("mirrorGlobalSkills", () => { + it.each([ + [ + "absolute", + (home: string, source: string) => symlinkSync(source, join(home, ".cursor", "skills")), + ], + [ + "relative", + (home: string) => symlinkSync("../.claude/skills", join(home, ".cursor", "skills")), + ], + ["intermediate", (home: string) => symlinkSync(".claude", join(home, ".cursor"))], + ])( + "fails closed when the target reaches the canonical store through a %s alias", + (_kind, alias) => { + const home = makeHome(); + seedStore(home, ["hyperframes"]); + const source = join(home, ".claude", "skills"); + if (_kind !== "intermediate") installMarker(home, ".cursor"); + alias(home, source); + + const result = mirrorGlobalSkills({ + skills: ["hyperframes"], + home, + platform: "linux", + env: ENV, + }); + + expect(lstatSync(join(source, "hyperframes")).isDirectory()).toBe(true); + expect(readFileSync(join(source, "hyperframes", "SKILL.md"), "utf8")).toBe("# hyperframes\n"); + expect(result.mirrored.map((entry) => entry.agent)).not.toContain("cursor"); + expect(result.skipped).toContainEqual( + expect.objectContaining({ agent: "cursor", reason: "aliases_install_owned_store" }), + ); + }, + ); + + it("fails closed and reports an unresolvable self-loop before destructive mirroring", () => { + const home = makeHome(); + seedStore(home, ["hyperframes"]); + installMarker(home, ".cursor"); + symlinkSync("skills", join(home, ".cursor", "skills")); + + const result = mirrorGlobalSkills({ + skills: ["hyperframes"], + home, + platform: "linux", + env: ENV, + }); + + expect(readFileSync(join(home, ".claude", "skills", "hyperframes", "SKILL.md"), "utf8")).toBe( + "# hyperframes\n", + ); + expect(result.skipped).toContainEqual( + expect.objectContaining({ agent: "cursor", reason: "unresolvable_target" }), + ); + }); + it("no-ops when there is no global Claude store", () => { const home = makeHome(); const result = mirrorGlobalSkills({ diff --git a/packages/cli/src/utils/skillsMirror.ts b/packages/cli/src/utils/skillsMirror.ts index 1f3c5927ff..50985b6b4a 100644 --- a/packages/cli/src/utils/skillsMirror.ts +++ b/packages/cli/src/utils/skillsMirror.ts @@ -22,9 +22,19 @@ // uses (XDG_CONFIG_HOME, CODEX_HOME, CLAUDE_CONFIG_DIR, …), so a machine with // those set mirrors into the exact dir the agent reads. -import { cpSync, existsSync, mkdirSync, readdirSync, rmSync, symlinkSync } from "node:fs"; +import { + cpSync, + existsSync, + lstatSync, + mkdirSync, + readdirSync, + realpathSync, + rmSync, + statSync, + symlinkSync, +} from "node:fs"; import { homedir } from "node:os"; -import { dirname, isAbsolute, join, relative } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; import { AGENT_GLOBAL_DIRS, type AgentDirBase } from "./agentDirs.generated.js"; /** @@ -48,6 +58,101 @@ export interface MirrorResult { source: string | null; /** Agents whose global dir was (re)populated. */ mirrored: { agent: string; dir: string }[]; + /** Agent targets skipped because their filesystem identity was unsafe. */ + skipped: { + agent: string; + dir: string; + reason: "aliases_install_owned_store" | "unresolvable_target"; + }[]; +} + +type MirrorSkipReason = MirrorResult["skipped"][number]["reason"]; + +/** Resolve a path through existing ancestors without creating its missing tail. */ +function canonicalCandidate(input: string): string | null { + let current = resolve(input); + const missing: string[] = []; + while (true) { + try { + lstatSync(current); + break; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT" && code !== "ENOTDIR") return null; + const parent = dirname(current); + if (parent === current) return null; + missing.unshift(basename(current)); + current = parent; + } + } + try { + return resolve(realpathSync(current), ...missing); + } catch { + return null; + } +} + +function pathsOverlap(left: string, right: string): boolean { + const fromLeft = relative(left, right); + const leftContainsRight = + fromLeft === "" || + (fromLeft !== ".." && !fromLeft.startsWith(`..${sep}`) && !isAbsolute(fromLeft)); + if (leftContainsRight) return true; + const fromRight = relative(right, left); + return fromRight !== ".." && !fromRight.startsWith(`..${sep}`) && !isAbsolute(fromRight); +} + +function sameExistingNode(left: string, right: string): boolean { + try { + const leftStat = statSync(left); + const rightStat = statSync(right); + return leftStat.dev === rightStat.dev && leftStat.ino === rightStat.ino; + } catch { + return false; + } +} + +function targetSafety( + target: string, + protectedPaths: ReadonlyArray<{ lexical: string; canonical: string }>, +): MirrorSkipReason | null { + const canonical = canonicalCandidate(target); + if (!canonical) return "unresolvable_target"; + for (const protectedPath of protectedPaths) { + if ( + pathsOverlap(canonical, protectedPath.canonical) || + sameExistingNode(target, protectedPath.lexical) + ) { + return "aliases_install_owned_store"; + } + } + return null; +} + +function skillTargetSafety( + sourceSkill: string, + targetSkill: string, + targetDirSafety: () => MirrorSkipReason | null, +): MirrorSkipReason | null { + const directoryReason = targetDirSafety(); + if (directoryReason) return directoryReason; + try { + // A final symlink is the normal Unix mirror shape. rmSync unlinks it + // without traversing its target, so canonical equality is safe here. + if (lstatSync(targetSkill).isSymbolicLink()) return null; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") return "unresolvable_target"; + } + const sourceCanonical = canonicalCandidate(sourceSkill); + const targetCanonical = canonicalCandidate(targetSkill); + if (!sourceCanonical || !targetCanonical) return "unresolvable_target"; + if ( + pathsOverlap(sourceCanonical, targetCanonical) || + sameExistingNode(sourceSkill, targetSkill) + ) { + return "aliases_install_owned_store"; + } + return null; } /** Resolve each env-overridable base dir exactly as upstream agents.ts does. */ @@ -78,39 +183,57 @@ function listSkillDirs(store: string): string[] { * copy, or a previous install) is removed first so the mirror always reflects * the canonical store — that's the whole point of "update". */ -function linkOrCopy(sourceSkill: string, targetSkill: string, platform: NodeJS.Platform): void { +function linkOrCopy( + sourceSkill: string, + targetSkill: string, + platform: NodeJS.Platform, + safety: () => MirrorSkipReason | null, +): MirrorSkipReason | null { + const unsafe = safety(); + if (unsafe) return unsafe; rmSync(targetSkill, { recursive: true, force: true }); if (platform === "win32") { cpSync(sourceSkill, targetSkill, { recursive: true }); } else { symlinkSync(relative(dirname(targetSkill), sourceSkill), targetSkill); } + return null; } /** * Populate one agent's global dir from the store. Best-effort and idempotent; - * per-skill failures don't abort the others. Returns false if the dir couldn't - * be created at all. + * per-skill failures don't abort the others. Unsafe target identity stops the + * agent before the next destructive operation and returns a reportable reason. */ function mirrorInto( targetDir: string, source: string, skills: string[], platform: NodeJS.Platform, -): boolean { + safety: () => MirrorSkipReason | null, +): { mirrored: boolean; skipReason?: MirrorSkipReason } { + const beforeCreate = safety(); + if (beforeCreate) return { mirrored: false, skipReason: beforeCreate }; try { mkdirSync(targetDir, { recursive: true }); } catch { - return false; + return { mirrored: false }; } + const afterCreate = safety(); + if (afterCreate) return { mirrored: false, skipReason: afterCreate }; for (const skill of skills) { try { - linkOrCopy(join(source, skill), join(targetDir, skill), platform); + const sourceSkill = join(source, skill); + const targetSkill = join(targetDir, skill); + const skipReason = linkOrCopy(sourceSkill, targetSkill, platform, () => + skillTargetSafety(sourceSkill, targetSkill, safety), + ); + if (skipReason) return { mirrored: false, skipReason }; } catch { // best-effort per skill } } - return true; + return { mirrored: true }; } /** @@ -132,7 +255,7 @@ export function mirrorGlobalSkills(opts: { // reads from the Claude store and must never link/copy onto either of them. const source = join(bases.claudeHome, "skills"); const universalStore = join(home, ".agents", "skills"); - if (!existsSync(source)) return { source: null, mirrored: [] }; + if (!existsSync(source)) return { source: null, mirrored: [], skipped: [] }; // Mirror ONLY HyperFrames' own skills (by name), NEVER everything in the // store: ~/.claude/skills is shared, so a user's gstack / personal / company @@ -141,15 +264,32 @@ export function mirrorGlobalSkills(opts: { // hyperframesSkillNames). const allowed = new Set(opts.skills); const skills = listSkillDirs(source).filter((name) => allowed.has(name)); - if (skills.length === 0) return { source, mirrored: [] }; + if (skills.length === 0) return { source, mirrored: [], skipped: [] }; + + const protectedPaths = [source, universalStore].map((lexical) => ({ + lexical, + canonical: canonicalCandidate(lexical), + })); + const resolvedProtectedPaths = protectedPaths.filter( + (entry): entry is { lexical: string; canonical: string } => entry.canonical !== null, + ); + if (resolvedProtectedPaths.length !== protectedPaths.length) { + return { source, mirrored: [], skipped: [] }; + } const mirrored: { agent: string; dir: string }[] = []; + const skipped: MirrorResult["skipped"] = []; 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 (!existsSync(dirname(targetDir))) continue; // agent not installed (no marker) - if (mirrorInto(targetDir, source, skills, platform)) mirrored.push({ agent, dir: targetDir }); + const attempt = mirrorInto(targetDir, source, skills, platform, () => + targetSafety(targetDir, resolvedProtectedPaths), + ); + if (attempt.mirrored) mirrored.push({ agent, dir: targetDir }); + else if (attempt.skipReason) + skipped.push({ agent, dir: targetDir, reason: attempt.skipReason }); } - return { source, mirrored }; + return { source, mirrored, skipped }; }