diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 48ee0b3e0d..8c25cd1ba1 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -45,9 +45,17 @@ vi.mock("node:child_process", () => ({ vi.mock("@clack/prompts", () => ({ log: { error: vi.fn(), + warn: vi.fn(), }, })); +// `skills update` calls checkSkills() to find skills removed upstream, then +// prunes them. Mock it so these tests don't touch the real FS / network; the +// default returns nothing removed, and the prune test overrides per-call. +vi.mock("../utils/skillsManifest.js", () => ({ + checkSkills: vi.fn(async () => ({ skills: [] })), +})); + function setPlatform(platform: NodeJS.Platform): void { Object.defineProperty(process, "platform", { value: platform, @@ -55,17 +63,31 @@ function setPlatform(platform: NodeJS.Platform): void { }); } +/** Invoke the `skills update` subcommand from a freshly-imported module. */ +async function runSkillsUpdate(): Promise { + const { default: skillsCmd } = await import("./skills.js"); + const subs = skillsCmd.subCommands as unknown as Record; + expect(subs.update).toBeDefined(); + await subs.update!.run?.({ args: {}, rawArgs: [], cmd: subs.update } as never); +} + describe("hyperframes skills", () => { + let prevExitCode: typeof process.exitCode; + beforeEach(() => { state.execCalls = []; state.spawnCalls = []; state.spawnExitCode = 0; vi.resetModules(); + // Each test asserts on process.exitCode; isolate it from the runner's own. + prevExitCode = process.exitCode; + process.exitCode = 0; }); afterEach(() => { setPlatform(originalPlatform); vi.restoreAllMocks(); + process.exitCode = prevExitCode; }); it("sets GIT_CLONE_PROTECTION_ACTIVE=0 on the spawned skills CLI child (GH #316)", async () => { @@ -131,39 +153,84 @@ describe("hyperframes skills", () => { it("skills update exits non-zero when the install fails", async () => { setPlatform("linux"); state.spawnExitCode = 1; // simulate `skills add` exiting non-zero - - const prevExit = process.exitCode; - process.exitCode = 0; - try { - const { default: skillsCmd } = await import("./skills.js"); - const subs = skillsCmd.subCommands as unknown as Record; - const updateCmd = subs.update; - expect(updateCmd).toBeDefined(); - await updateCmd!.run?.({ args: {}, rawArgs: [], cmd: updateCmd } as never); - expect(process.exitCode).toBe(1); - } finally { - process.exitCode = prevExit; - } + await runSkillsUpdate(); + expect(process.exitCode).toBe(1); }); it("skills update exits zero on a successful install", async () => { setPlatform("linux"); - state.spawnExitCode = 0; + await runSkillsUpdate(); + expect(process.exitCode).toBe(0); + // pulls the full set straight from GitHub + expect(state.spawnCalls[0]?.args).toContain("https://github.com/heygen-com/hyperframes"); + expect(state.spawnCalls[0]?.args).toContain("--all"); + }); - const prevExit = process.exitCode; - process.exitCode = 0; - try { - const { default: skillsCmd } = await import("./skills.js"); - const subs = skillsCmd.subCommands as unknown as Record; - const updateCmd = subs.update; - expect(updateCmd).toBeDefined(); - await updateCmd!.run?.({ args: {}, rawArgs: [], cmd: updateCmd } as never); - expect(process.exitCode).toBe(0); - // pulls the full set straight from GitHub - expect(state.spawnCalls[0]?.args).toContain("https://github.com/heygen-com/hyperframes"); - expect(state.spawnCalls[0]?.args).toContain("--all"); - } finally { - process.exitCode = prevExit; - } + // `skills add --all` never deletes, so update must separately prune skills the + // manifest dropped (renames/removals) for `check || update` to fully reconcile. + it("skills update prunes skills removed upstream, in the attributed scope", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockResolvedValueOnce({ + scope: "global", + skills: [{ name: "graphic-overlays", status: "removed" }], + } as never); + + await runSkillsUpdate(); + + // install first, then a `skills remove` for the dropped skill + expect(state.spawnCalls[0]?.args).toContain("add"); + const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); + expect(removeCall, "expected a `skills remove` spawn").toBeDefined(); + expect(removeCall!.args).toContain("graphic-overlays"); + expect(removeCall!.args).toContain("--yes"); + expect(removeCall!.args).toContain("-g"); // attributed from the global lock → remove globally + expect(process.exitCode).toBe(0); + }); + + // The scope the skill was attributed from drives the remove scope: a + // project-scoped removal must NOT pass -g (which would target a different, + // possibly user-owned, global skill of the same name). + it("skills update prunes in project scope without -g", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockResolvedValueOnce({ + scope: "project", + skills: [{ name: "graphic-overlays", status: "removed" }], + } as never); + + await runSkillsUpdate(); + + const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); + expect(removeCall, "expected a `skills remove` spawn").toBeDefined(); + expect(removeCall!.args).toContain("graphic-overlays"); + expect(removeCall!.args).not.toContain("-g"); + }); + + it("skills update does not prune when nothing was removed upstream", async () => { + setPlatform("linux"); + await runSkillsUpdate(); + expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); + }); + + // Skill names come from lock-file JSON keys; a flag-like / shell-special name + // must never reach the spawn (esp. the Windows cmd.exe path). + it("skills update never passes a non-slug skill name to remove", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockResolvedValueOnce({ + scope: "global", + skills: [ + { name: "graphic-overlays", status: "removed" }, + { name: "--config=evil.js", status: "removed" }, + ], + } as never); + + await runSkillsUpdate(); + + const removeCall = state.spawnCalls.find((s) => s.args.includes("remove")); + expect(removeCall, "expected a `skills remove` spawn for the valid name").toBeDefined(); + expect(removeCall!.args).toContain("graphic-overlays"); + expect(removeCall!.args).not.toContain("--config=evil.js"); }); }); diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index 6a352ed516..5949cd3eb2 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -4,7 +4,7 @@ import * as clack from "@clack/prompts"; import { c } from "../ui/colors.js"; import { buildNpxCommand } from "../utils/npxCommand.js"; import { withMeta } from "../utils/updateCheck.js"; -import { checkSkills, type SkillsCheckResult } from "../utils/skillsManifest.js"; +import { checkSkills, type SkillDiff, type SkillsCheckResult } from "../utils/skillsManifest.js"; import type { Example } from "./_examples.js"; export const examples: Example[] = [ @@ -55,6 +55,27 @@ function runSkillsAdd( return spawnNpx(["skills", "add", source, ...(opts.extraArgs ?? ["--all"])], opts); } +// Skill names are kebab-case directory names. Refuse anything that isn't one +// before spreading it into a spawn: a corrupt or crafted lock entry (these +// names originate as lock-file JSON keys) could otherwise smuggle a flag-like +// (`--config=…`) or shell-special token into the command — which matters most +// on the Windows `cmd.exe` spawn path, where arg escaping is fragile. +const PLAIN_SKILL_NAME = /^[a-z0-9][a-z0-9._-]*$/i; + +function runSkillsRemove(names: string[], opts: { global: boolean }): Promise { + const safe = names.filter((n) => PLAIN_SKILL_NAME.test(n)); + const rejected = names.filter((n) => !PLAIN_SKILL_NAME.test(n)); + if (rejected.length) { + clack.log.warn(c.warn(`Skipping unexpected skill name(s): ${rejected.join(", ")}`)); + } + if (!safe.length) return Promise.resolve(); + // `skills remove --yes` deletes the bundle dir, every agent symlink, and the + // lock entry non-interactively. `-g` targets the global install; without it, + // the project (cwd) install — we pass whichever scope detection attributed + // these names from, so we never reach into a scope we didn't inspect. + return spawnNpx(["skills", "remove", ...safe, ...(opts.global ? ["-g"] : []), "--yes"]); +} + // Use the full GitHub URL (not the `owner/repo` slug) so `skills add` git-clones // the repo directly at latest `main`, bypassing the skills.sh registry — which // can lag behind the repo. Our freshness check already resolves "latest" @@ -89,6 +110,21 @@ export async function installAllSkills( // ── check ──────────────────────────────────────────────────────────────────── +/** Print a labelled list of skills (nothing if empty), each line uniformly coloured. */ +function printSkillSection( + result: SkillsCheckResult, + status: SkillDiff["status"], + title: string, + mark: string, + color: (s: string) => string, +): void { + const items = result.skills.filter((s) => s.status === status); + if (!items.length) return; + console.log(); + console.log(` ${color(title)}`); + for (const s of items) console.log(` ${color(`${mark} ${s.name}`)}`); +} + function renderCheck(result: SkillsCheckResult): void { const { summary } = result; console.log(); @@ -108,21 +144,18 @@ function renderCheck(result: SkillsCheckResult): void { const parts = [c.success(`✓ ${summary.current} current`)]; if (summary.outdated) parts.push(c.warn(`↑ ${summary.outdated} outdated`)); if (summary.missing) parts.push(c.dim(`◦ ${summary.missing} not installed`)); + if (summary.removed) parts.push(c.warn(`✗ ${summary.removed} removed upstream`)); console.log(` ${parts.join(" ")}`); - const outdated = result.skills.filter((s) => s.status === "outdated"); - const missing = result.skills.filter((s) => s.status === "missing"); - - if (outdated.length) { - console.log(); - console.log(` ${c.warn("Outdated:")}`); - for (const s of outdated) console.log(` ${c.warn("↑")} ${s.name}`); - } - if (missing.length) { - console.log(); - console.log(` ${c.dim("Not installed:")}`); - for (const s of missing) console.log(` ${c.dim("◦ " + s.name)}`); - } + printSkillSection(result, "outdated", "Outdated:", "↑", c.warn); + printSkillSection(result, "missing", "Not installed:", "◦", c.dim); + printSkillSection( + result, + "removed", + "Removed upstream (renamed or dropped — no longer published):", + "✗", + c.warn, + ); console.log(); if (result.updateAvailable) { @@ -163,7 +196,8 @@ const checkCommand = defineCommand({ const updateCommand = defineCommand({ meta: { name: "update", - description: "Update all HyperFrames skills to the latest — installs any not yet present", + description: + "Update all HyperFrames skills to the latest — installs any not yet present, removes any no longer published", }, args: {}, async run() { @@ -180,6 +214,32 @@ const updateCommand = defineCommand({ } catch (err) { clack.log.error(c.error(`Update failed: ${(err as Error).message}`)); process.exitCode = 1; + return; + } + + // `skills add --all` never deletes, so a skill renamed or dropped upstream + // (e.g. graphic-overlays → talking-head-recut) would linger forever. Prune + // skills the lock attributes to our source that the manifest no longer + // ships, so `check || update` fully reconciles the install to the manifest. + // + // Safety: `removed` only ever contains skills the lock records as installed + // from our source (see detectRemoved) — never a user's own or another + // source's skills. We remove in the exact scope detection attributed from, + // so we never reach into a scope we didn't inspect. Best-effort: cleanup + // failure doesn't fail the update — the install the CI contract gates on + // already succeeded. + try { + const { skills, scope } = await checkSkills(); + const removed = skills.filter((s) => s.status === "removed").map((s) => s.name); + if (removed.length) { + console.log(); + console.log( + c.dim(`Removing ${removed.length} skill(s) no longer published: ${removed.join(", ")}`), + ); + await runSkillsRemove(removed, { global: scope === "global" }); + } + } catch (err) { + clack.log.warn(c.warn(`Skipped removed-skill cleanup: ${(err as Error).message}`)); } }, }); diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index fc1386c272..977f269c70 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -7,6 +7,7 @@ import { buildManifest, checkSkills, diffSkills, + skillsAttributedToSource, type SkillsManifest, type SkillEntry, } from "./skillsManifest.js"; @@ -255,3 +256,101 @@ describe("checkSkills install detection", () => { expect(res.agent).toBe("claude-code"); }); }); + +describe("skillsAttributedToSource", () => { + it("matches by slug or git clone URL and ignores other sources", () => { + const lock = { + skills: { + a: { source: "heygen-com/hyperframes" }, + b: { sourceUrl: "https://github.com/heygen-com/hyperframes.git" }, + c: { source: "https://github.com/heygen-com/hyperframes" }, + d: { source: "greensock/gsap-skills" }, + }, + }; + expect(skillsAttributedToSource(lock, "heygen-com/hyperframes").sort()).toEqual([ + "a", + "b", + "c", + ]); + }); + + it("returns [] for a null/empty lock or empty source", () => { + expect(skillsAttributedToSource(null, "x")).toEqual([]); + expect(skillsAttributedToSource({}, "x")).toEqual([]); + expect(skillsAttributedToSource({ skills: { a: { source: "x" } } }, "")).toEqual([]); + }); +}); + +describe("checkSkills removed-upstream detection", () => { + // The global lock path honours XDG_STATE_HOME; clear it so the fixture under + // /.agents/.skill-lock.json is the one that's read. + let xdg: string | undefined; + beforeEach(() => { + xdg = process.env.XDG_STATE_HOME; + delete process.env.XDG_STATE_HOME; + }); + afterEach(() => { + if (xdg === undefined) delete process.env.XDG_STATE_HOME; + else process.env.XDG_STATE_HOME = xdg; + }); + + // Shared fixture: a project + home with two skills installed globally + // (alpha + gamma), plus a manifest path. Tests then write the lock they need. + function setup(manifest: SkillsManifest): { + home: string; + opts: { source: string; cwd: string; home: string }; + } { + const project = join(root, "project"); + const home = join(root, "home"); + mkdirSync(project, { recursive: true }); + const skillsDir = join(home, ".agents/skills"); + for (const name of ["alpha", "gamma"]) { + mkdirSync(join(skillsDir, name), { recursive: true }); + writeFileSync(join(skillsDir, name, "SKILL.md"), `# ${name}`); + } + const manifestPath = join(root, "manifest.json"); + writeFileSync(manifestPath, JSON.stringify(manifest)); + return { home, opts: { source: manifestPath, cwd: project, home } }; + } + function writeGlobalLock(home: string, skills: Record): void { + writeFileSync(join(home, ".agents/.skill-lock.json"), JSON.stringify({ version: 3, skills })); + } + + it("flags a lock-attributed skill the manifest dropped, ignoring other sources", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + writeGlobalLock(home, { + alpha: { source: "test" }, // still ours and in the manifest + gamma: { source: "test" }, // ours, dropped from manifest → removed + delta: { source: "someone/else" }, // a different source → never ours + }); + + const res = await checkSkills(opts); + const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); + expect(byName.gamma).toBe("removed"); + expect(byName.delta).toBeUndefined(); + expect(res.summary.removed).toBe(1); + }); + + it("a removed skill alone makes an update available (no outdated/missing)", async () => { + // Manifest lists only alpha, with its REAL hash → "current" (not outdated). + const { home, opts } = setup({ source: "test", skills: {} }); + const manifest: SkillsManifest = { + source: "test", + skills: { alpha: hashSkillBundle(join(home, ".agents/skills/alpha")) }, + }; + writeFileSync(opts.source, JSON.stringify(manifest)); + writeGlobalLock(home, { alpha: { source: "test" }, gamma: { source: "test" } }); + + const res = await checkSkills(opts); + expect(res.summary).toEqual({ current: 1, outdated: 0, missing: 0, removed: 1 }); + expect(res.updateAvailable).toBe(true); + }); + + it("reports no removed skills when there is no lock to attribute from", async () => { + const { opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + // gamma is an orphan on disk, but with no lock we can't attribute it to us. + const res = await checkSkills(opts); + expect(res.summary.removed).toBe(0); + expect(res.skills.some((s) => s.status === "removed")).toBe(false); + }); +}); diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index 2e94c36a13..bf46c6e80b 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -58,7 +58,9 @@ export interface SkillsManifest { skills: Record; } -export type SkillStatus = "current" | "outdated" | "missing"; +// "removed" = installed from our source but no longer in the manifest (renamed +// or dropped upstream). Attributed via the skills lock — see detectRemoved. +export type SkillStatus = "current" | "outdated" | "missing" | "removed"; export interface SkillDiff { name: string; @@ -67,13 +69,22 @@ export interface SkillDiff { latestHash?: string; } +/** The pure manifest diff (current / outdated / missing — what `diffSkills` returns). */ +export interface SkillsDiff { + updateAvailable: boolean; + summary: { current: number; outdated: number; missing: number }; + skills: SkillDiff[]; +} + export interface SkillsCheckResult { /** Install location that was checked (absolute path), or null if none found. */ location: string | null; /** Agent convention inferred from the location (claude-code, codex, …). */ agent: string | null; + /** Scope of the located install — so a caller prunes in the same scope it attributed from. */ + scope: "project" | "global" | null; updateAvailable: boolean; - summary: { current: number; outdated: number; missing: number }; + summary: { current: number; outdated: number; missing: number; removed: number }; skills: SkillDiff[]; } @@ -240,10 +251,11 @@ function hashInstalled(root: SkillRoot, skillNames: string[]): Record, latest: SkillsManifest, -): Omit { +): SkillsDiff { // Report only on skills the manifest knows about. A skill on disk that isn't - // in the manifest isn't necessarily ours — `.../skills` is shared across - // sources — so it's not something we can meaningfully diff, and is ignored. + // in the manifest is handled separately (see detectRemoved): we can only call + // one "ours but removed" via the lock's source attribution, never the bare + // directory name — `.../skills` is shared across sources. const skills: SkillDiff[] = []; const summary = { current: 0, outdated: 0, missing: 0 }; @@ -276,6 +288,78 @@ export function diffSkills( }; } +// ── Removed-upstream (orphaned) skills ──────────────────────────────────────── +// +// `skills add` / `init` / `hyperframes skills update` only ever add or refresh — +// none of them delete a skill that was renamed or dropped upstream (e.g. +// graphic-overlays → talking-head-recut), so a stale bundle lingers forever and +// the manifest-only diff above can't see it. We surface these by cross-checking +// the vercel-labs/skills lock: a skill the lock attributes to OUR manifest +// `source` that the manifest no longer lists is "removed". Attribution is the +// whole point — `.../skills` is shared across sources, so we never infer "ours" +// from a directory name alone (that would flag every other source's skills too). + +interface LockEntry { + source?: string; + sourceUrl?: string; +} + +/** The slice of the vercel-labs/skills lock file we read. */ +export interface SkillLock { + skills?: Record; +} + +/** Normalise a slug or git clone URL to a bare lowercase `owner/repo`. */ +function repoSlug(s: string | undefined): string { + return (s ?? "") + .replace(/^git\+/, "") + .replace(/^https?:\/\/github\.com\//, "") + .replace(/\.git$/, "") + .toLowerCase(); +} + +/** Skill names the lock attributes to `source` (matched by slug or clone URL). */ +export function skillsAttributedToSource(lock: SkillLock | null, source: string): string[] { + const want = repoSlug(source); + if (!want || !lock?.skills) return []; + return Object.entries(lock.skills) + .filter(([, e]) => repoSlug(e.source) === want || repoSlug(e.sourceUrl) === want) + .map(([name]) => name); +} + +/** Locate the vercel-labs/skills lock for a scope, mirroring that CLI's paths. */ +function lockPathForScope( + scope: "project" | "global", + opts: { cwd?: string; home?: string }, +): string { + if (scope === "project") return join(opts.cwd ?? process.cwd(), "skills-lock.json"); + const xdgStateHome = process.env.XDG_STATE_HOME; + if (xdgStateHome) return join(xdgStateHome, "skills", ".skill-lock.json"); + return join(opts.home ?? homedir(), ".agents", ".skill-lock.json"); +} + +function readSkillLock(path: string): SkillLock | null { + try { + return JSON.parse(readFileSync(path, "utf8")) as SkillLock; + } catch { + // No lock (or unreadable / not JSON) → we can't attribute, so report none. + return null; + } +} + +/** Skills the lock attributes to our source that the manifest no longer ships. */ +function detectRemoved( + root: SkillRoot, + latest: SkillsManifest, + opts: { cwd?: string; home?: string }, +): SkillDiff[] { + const lock = readSkillLock(lockPathForScope(root.scope, opts)); + return skillsAttributedToSource(lock, latest.source) + .filter((name) => !(name in latest.skills)) + .sort() + .map((name) => ({ name, status: "removed" as const })); +} + // ── Resolving the "latest" manifest ────────────────────────────────────────── /** Walk up from `cwd` to find a repo checkout that ships the manifest. */ @@ -403,5 +487,15 @@ export async function checkSkills( const root = locateInstall(skillNames, { dir: opts.dir, cwd: opts.cwd, home: opts.home }); const installed = root ? hashInstalled(root, skillNames) : {}; const diff = diffSkills(installed, latest); - return { location: root?.dir ?? null, agent: root?.agent ?? null, ...diff }; + const removed = root ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home }) : []; + return { + location: root?.dir ?? null, + agent: root?.agent ?? null, + scope: root?.scope ?? null, + // Removed skills also mean the install isn't reconciled with the manifest — + // `skills update` now prunes them, so they count toward "update available". + updateAvailable: diff.updateAvailable || removed.length > 0, + summary: { ...diff.summary, removed: removed.length }, + skills: [...diff.skills, ...removed], + }; }