fix(cli): close skills removed-detection power-user gaps (follow-up to #1740)

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

@@ -64,11 +64,11 @@ function setPlatform(platform: NodeJS.Platform): void {
 }
 
 /** Invoke the `skills update` subcommand from a freshly-imported module. */
-async function runSkillsUpdate(): Promise<void> {
+async function runSkillsUpdate(args: Record<string, unknown> = {}): 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);
+  await subs.update!.run?.({ args, rawArgs: [], cmd: subs.update } as never);
 }
 
 describe("hyperframes skills", () => {
@@ -213,6 +213,19 @@ describe("hyperframes skills", () => {
     expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false);
   });
 
+  // `update`'s prune runs the same removed-detection as `check`, so its
+  // --source/--dir must reach the internal checkSkills() — otherwise the prune
+  // reconciles against defaults even when the user pointed elsewhere.
+  it("skills update plumbs --source/--dir to its prune detection (parity with check)", async () => {
+    setPlatform("linux");
+    const { checkSkills } = await import("../utils/skillsManifest.js");
+    vi.mocked(checkSkills).mockResolvedValueOnce({ scope: "project", skills: [] } as never);
+
+    await runSkillsUpdate({ source: "owner/repo", dir: "/custom/skills" });
+
+    expect(checkSkills).toHaveBeenCalledWith({ source: "owner/repo", dir: "/custom/skills" });
+  });
+
   // 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 () => {
@@ -233,4 +246,29 @@ describe("hyperframes skills", () => {
     expect(removeCall!.args).toContain("graphic-overlays");
     expect(removeCall!.args).not.toContain("--config=evil.js");
   });
+
+  // The early-return guard in runSkillsRemove: when EVERY candidate name is
+  // rejected as non-slug, no `skills remove` is spawned at all (the prior test
+  // only covers a mix of valid + invalid). A spawn here would run `skills remove
+  // --yes` with no names — which the upstream CLI treats as "remove nothing" at
+  // best, or prompts interactively at worst — so we must not reach it.
+  it("skills update spawns no remove when every removed name is rejected", async () => {
+    setPlatform("linux");
+    const { checkSkills } = await import("../utils/skillsManifest.js");
+    vi.mocked(checkSkills).mockResolvedValueOnce({
+      scope: "global",
+      skills: [
+        { name: "--config=evil.js", status: "removed" },
+        { name: "../escape", status: "removed" },
+      ],
+    } as never);
+
+    await runSkillsUpdate();
+
+    expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false);
+    // The install still ran and the update still succeeded — a cleanup no-op
+    // doesn't fail the update.
+    expect(state.spawnCalls[0]?.args).toContain("add");
+    expect(process.exitCode).toBe(0);
+  });
 });

packages/cli/src/commands/skills.ts

@@ -4,7 +4,12 @@ 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 SkillDiff, type SkillsCheckResult } from "../utils/skillsManifest.js";
+import {
+  checkSkills,
+  SKILLS_CLI_LOCK_PATHS_VERIFIED_AT,
+  type SkillDiff,
+  type SkillsCheckResult,
+} from "../utils/skillsManifest.js";
 import type { Example } from "./_examples.js";
 
 export const examples: Example[] = [
@@ -157,6 +162,19 @@ function renderCheck(result: SkillsCheckResult): void {
     c.warn,
   );
 
+  // Removed-detection cross-references the upstream skills lock. If that lock is
+  // absent where we expect it (e.g. upstream moved its path), removed-detection
+  // silently reports zero — so warn rather than imply a clean "up to date".
+  if (result.lockMissing) {
+    console.log();
+    console.log(
+      `  ${c.warn(`! Skills lock not found — can't check for skills removed upstream.`)}`,
+    );
+    console.log(
+      `  ${c.dim(`  (lock paths verified against ${SKILLS_CLI_LOCK_PATHS_VERIFIED_AT})`)}`,
+    );
+  }
+
   console.log();
   if (result.updateAvailable) {
     console.log(`  ${c.accent("Update: npx hyperframes skills update")}`);
@@ -199,12 +217,31 @@ const updateCommand = defineCommand({
     description:
       "Update all HyperFrames skills to the latest — installs any not yet present, removes any no longer published",
   },
-  args: {},
-  async run() {
+  // Mirror `check`'s flags: the prune step runs the same removed-detection, so it
+  // must respect the same overrides. Without these, `update`'s internal
+  // checkSkills() fell back to defaults — pruning the auto-detected install
+  // against the default manifest even when the user pointed `check` elsewhere.
+  args: {
+    dir: { type: "string", description: "Skills directory to reconcile (default: auto-detect)" },
+    source: {
+      type: "string",
+      description: "Where 'latest' comes from: local path, owner/repo, or URL",
+    },
+  },
+  async run({ args }) {
+    const dir = args.dir as string | undefined;
+    const source = args.source as string | undefined;
+
     // `skills add --all` re-fetches every skill to the latest AND installs ones
     // not yet present — so "update" pulls the full set, not just what is already
     // installed. This is where `init` and the stale-skills nudge both lead.
     //
+    // Note: the upstream `skills add` CLI has no `--dir` flag (it installs into
+    // detected agent dirs), so `--dir` here scopes only the *prune* detection
+    // below, not the install. `--source` likewise drives where the prune's
+    // "latest" manifest comes from; the install always targets the canonical
+    // HyperFrames repo so `update` reliably pulls the published set.
+    //
     // strict: this is the documented recovery path for the agent/CI contract
     // `hyperframes skills check || hyperframes skills update`. If the install
     // fails (no npx, `skills add` exits non-zero) it must exit non-zero too —
@@ -229,7 +266,7 @@ const updateCommand = defineCommand({
     // failure doesn't fail the update — the install the CI contract gates on
     // already succeeded.
     try {
-      const { skills, scope } = await checkSkills();
+      const { skills, scope } = await checkSkills({ dir, source });
       const removed = skills.filter((s) => s.status === "removed").map((s) => s.name);
       if (removed.length) {
         console.log();

packages/cli/src/utils/skillsManifest.test.ts

@@ -294,6 +294,14 @@ describe("checkSkills removed-upstream detection", () => {
     else process.env.XDG_STATE_HOME = xdg;
   });
 
+  // Install one or more `<skillsDir>/<name>/SKILL.md` bundles.
+  function installSkills(skillsDir: string, names: string[]): void {
+    for (const name of names) {
+      mkdirSync(join(skillsDir, name), { recursive: true });
+      writeFileSync(join(skillsDir, name, "SKILL.md"), `# ${name}`);
+    }
+  }
+
   // 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): {
@@ -303,11 +311,7 @@ describe("checkSkills removed-upstream detection", () => {
     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}`);
-    }
+    installSkills(join(home, ".agents/skills"), ["alpha", "gamma"]);
     const manifestPath = join(root, "manifest.json");
     writeFileSync(manifestPath, JSON.stringify(manifest));
     return { home, opts: { source: manifestPath, cwd: project, home } };
@@ -352,5 +356,58 @@ describe("checkSkills removed-upstream detection", () => {
     const res = await checkSkills(opts);
     expect(res.summary.removed).toBe(0);
     expect(res.skills.some((s) => s.status === "removed")).toBe(false);
+    // No install was located via auto-detect (skills live under <home>/.agents/skills
+    // which IS discovered) — so lockMissing reflects the genuinely-absent lock.
+    expect(res.lockMissing).toBe(true);
+  });
+
+  // A `--dir` pointing at a global-scoped install (under $HOME) must resolve the
+  // GLOBAL lock (<home>/.agents/.skill-lock.json), not the project lock
+  // (<cwd>/skills-lock.json). Before this fix, locateInstall hardcoded scope
+  // "project" for every --dir, so the global lock was never read and
+  // removed-detection silently found nothing for --dir installs.
+  it("--dir under $HOME resolves the global lock so removed-detection works", async () => {
+    const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } });
+    writeGlobalLock(home, {
+      alpha: { source: "test" }, // ours and still in the manifest
+      gamma: { source: "test" }, // ours, dropped from manifest → removed
+    });
+
+    const dir = join(home, ".agents/skills");
+    const res = await checkSkills({ source: opts.source, dir, cwd: opts.cwd, home });
+    expect(res.scope).toBe("global");
+    const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status]));
+    expect(byName.gamma).toBe("removed");
+    expect(res.summary.removed).toBe(1);
+    expect(res.lockMissing).toBe(false);
+  });
+
+  // The complementary case: a `--dir` under the project tree (not $HOME) stays
+  // project-scoped and reads <cwd>/skills-lock.json.
+  it("--dir outside $HOME stays project-scoped and reads the project lock", async () => {
+    const project = join(root, "proj2");
+    const skillsDir = join(project, ".claude/skills");
+    installSkills(skillsDir, ["alpha", "gamma"]);
+    const manifestPath = join(root, "m2.json");
+    writeFileSync(
+      manifestPath,
+      JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }),
+    );
+    // Project lock lives at <cwd>/skills-lock.json — point cwd at the project.
+    writeFileSync(
+      join(project, "skills-lock.json"),
+      JSON.stringify({
+        version: 1,
+        skills: { alpha: { source: "test" }, gamma: { source: "test" } },
+      }),
+    );
+    const home = join(root, "home2");
+    mkdirSync(home, { recursive: true });
+
+    const res = await checkSkills({ source: manifestPath, dir: skillsDir, cwd: project, home });
+    expect(res.scope).toBe("project");
+    const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status]));
+    expect(byName.gamma).toBe("removed");
+    expect(res.summary.removed).toBe(1);
   });
 });

packages/cli/src/utils/skillsManifest.ts

@@ -19,7 +19,7 @@ import { execFile } from "node:child_process";
 import { createHash } from "node:crypto";
 import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
 import { homedir } from "node:os";
-import { isAbsolute, join, relative, sep } from "node:path";
+import { isAbsolute, join, relative, resolve, sep } from "node:path";
 import { promisify } from "node:util";
 
 const execFileAsync = promisify(execFile);
@@ -86,6 +86,13 @@ export interface SkillsCheckResult {
   updateAvailable: boolean;
   summary: { current: number; outdated: number; missing: number; removed: number };
   skills: SkillDiff[];
+  /**
+   * True when an install was located but the upstream skills lock was absent at
+   * the expected path, so removed-detection couldn't run (it silently reports
+   * zero removed). Lets the CLI warn instead of misreporting "up to date" — a
+   * guard against the lock path silently no-op'ing if upstream moves it.
+   */
+  lockMissing: boolean;
 }
 
 const DEFAULT_REPO_SLUG = "heygen-com/hyperframes";
@@ -212,18 +219,41 @@ function discoverSkillRoots(base: string, scope: "project" | "global"): SkillRoo
   });
 }
 
+/**
+ * Decide whether an explicit `--dir` is a project- or global-scoped install, so
+ * removed-detection reads the *right* lock. The upstream `skills` CLI keeps two
+ * locks: a project lock at `<cwd>/skills-lock.json` and a global lock under
+ * `$HOME` (see lockPathForScope). A `--dir` that lives under `$HOME` (e.g.
+ * `~/.claude/skills`) is a global install; one under the project tree (or
+ * anywhere else) is treated as project. Without this, `--dir ~/.claude/skills`
+ * was always scoped "project", so detectRemoved looked for a non-existent
+ * `<cwd>/skills-lock.json` and silently found zero removed skills.
+ */
+function scopeForDir(dir: string, home: string): "project" | "global" {
+  const norm = (p: string): string => {
+    const r = resolve(p);
+    return r.endsWith(sep) ? r : r + sep;
+  };
+  return norm(dir).startsWith(norm(home)) ? "global" : "project";
+}
+
 /**
  * Find the first skill root that actually contains HyperFrames skills. A
- * `--dir` override (if given) is treated as a `.../skills` directory directly.
- * Otherwise scan project (cwd) then global ($HOME), auto-discovering hosts.
+ * `--dir` override (if given) is treated as a `.../skills` directory directly;
+ * its scope is inferred (see scopeForDir) so removed-detection reads the right
+ * lock. Otherwise scan project (cwd) then global ($HOME), auto-discovering hosts.
  */
 function locateInstall(
   skillNames: string[],
   opts: { dir?: string; cwd?: string; home?: string } = {},
 ): SkillRoot | null {
   if (opts.dir) {
     return existsSync(opts.dir)
-      ? { dir: opts.dir, agent: agentFromDir(opts.dir), scope: "project" }
+      ? {
+          dir: opts.dir,
+          agent: agentFromDir(opts.dir),
+          scope: scopeForDir(opts.dir, opts.home ?? homedir()),
+        }
       : null;
   }
   const roots = [
@@ -327,7 +357,19 @@ export function skillsAttributedToSource(lock: SkillLock | null, source: string)
     .map(([name]) => name);
 }
 
-/** Locate the vercel-labs/skills lock for a scope, mirroring that CLI's paths. */
+// Removed-detection reads the vercel-labs/skills lock, whose on-disk paths live
+// in *their* repo, not ours — so if upstream moves the lock, our cross-reference
+// silently finds nothing and `detectRemoved` no-ops without a peep. Pin the
+// upstream version these paths were verified against so a future bump is a
+// deliberate, reviewable edit (re-check src/skill-lock.ts `getSkillLockPath`
+// and src/local-lock.ts `getLocalLockPath` when bumping):
+//   - global:  $XDG_STATE_HOME/skills/.skill-lock.json  else  ~/.agents/.skill-lock.json
+//   - project: <cwd>/skills-lock.json
+// https://github.com/vercel-labs/skills/blob/v1.5.13/src/skill-lock.ts (global)
+// https://github.com/vercel-labs/skills/blob/v1.5.13/src/local-lock.ts (project)
+export const SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = "vercel-labs/skills@v1.5.13";
+
+/** Locate the vercel-labs/skills lock for a scope (paths pinned to the version above). */
 function lockPathForScope(
   scope: "project" | "global",
   opts: { cwd?: string; home?: string },
@@ -347,17 +389,24 @@ function readSkillLock(path: string): SkillLock | null {
   }
 }
 
+interface RemovedResult {
+  removed: SkillDiff[];
+  /** The lock was absent at the expected path — removed-detection silently no-ops. */
+  lockMissing: boolean;
+}
+
 /** 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[] {
+): RemovedResult {
   const lock = readSkillLock(lockPathForScope(root.scope, opts));
-  return skillsAttributedToSource(lock, latest.source)
+  const removed = skillsAttributedToSource(lock, latest.source)
     .filter((name) => !(name in latest.skills))
     .sort()
     .map((name) => ({ name, status: "removed" as const }));
+  return { removed, lockMissing: lock === null };
 }
 
 // ── Resolving the "latest" manifest ──────────────────────────────────────────
@@ -487,7 +536,10 @@ 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);
-  const removed = root ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home }) : [];
+  const removedResult = root
+    ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home })
+    : { removed: [], lockMissing: false };
+  const { removed, lockMissing } = removedResult;
   return {
     location: root?.dir ?? null,
     agent: root?.agent ?? null,
@@ -497,5 +549,6 @@ export async function checkSkills(
     updateAvailable: diff.updateAvailable || removed.length > 0,
     summary: { ...diff.summary, removed: removed.length },
     skills: [...diff.skills, ...removed],
+    lockMissing,
   };
 }