feat(cli): surface and prune skills removed upstream

packages/cli/src/commands/skills.test.ts

@@ -45,27 +45,49 @@ 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,
     configurable: true,
   });
 }
 
+/** Invoke the `skills update` subcommand from a freshly-imported module. */
+async function runSkillsUpdate(): Promise<void> {
+  const { default: skillsCmd } = await import("./skills.js");
+  const subs = skillsCmd.subCommands as unknown as Record<string, typeof skillsCmd>;
+  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<string, typeof skillsCmd>;
-      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<string, typeof skillsCmd>;
-      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");
   });
 });

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<void> {
+  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}`));
     }
   },
 });