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

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

Address power-user follow-ups deferred from #1740 (skills removed-detection):

- `--dir` installs now run removed-detection. `locateInstall` hardcoded
  scope "project" for every `--dir`, so a `--dir ~/.claude/skills` (a global
  install) read a non-existent `<cwd>/skills-lock.json` and found zero
  removed skills. New `scopeForDir` infers global vs project from whether the
  dir is under $HOME, so the right lock is read.
- Pin the upstream lock paths to vercel-labs/skills@v1.5.13 (verified against
  src/skill-lock.ts + src/local-lock.ts) and warn loudly when the lock is
  absent where expected, so removed-detection no longer silently no-ops if
  upstream moves the lock. checkSkills returns lockMissing; --json surfaces it.
- skills update gains --source/--dir (parity with check), plumbed into its
  internal prune checkSkills() so the prune respects the same overrides.
- Add the missing test for the all-rejected-names early-return in
  runSkillsRemove (no skills remove spawned when every candidate name is
  rejected), plus tests for the --dir scope inference and update flag parity.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(cli): scope --dir by CWD-containment before HOME (project installs under $HOME)

Address Magi's REQUEST_CHANGES on #1743 (88daa820). The new scopeForDir
heuristic treated every explicit --dir under $HOME as GLOBAL, but the common
project-local case is also under $HOME (e.g. ~/work/proj/.claude/skills, or
--dir .claude/skills run from ~/work/proj). So checkSkills could read the
GLOBAL lock and `skills update --dir ...` could prune with `skills remove -g`
even when the user pointed at a PROJECT install — a wrong-scope prune.

Change precedence to CWD-containment FIRST, then HOME:
- dir resolves under cwd  -> project (even when cwd is itself under $HOME)
- else dir under home     -> global
- else                    -> project (safe default, never prune globally)

Keeps the existing resolve/normalize + trailing-separator guard (so /home/user2
does not false-match /home/user). scopeForDir now takes cwd; locateInstall
threads opts.cwd through.

Add a regression test for Magi's exact failing case: cwd nested under home
(cwd = <home>/work/proj) with --dir <cwd>/.claude/skills resolves to PROJECT.
Existing tests stay green (global --dir ~/.claude/skills from an unrelated cwd
still resolves to GLOBAL).

Also (Miga's nit): drop the redundant `as string | undefined` casts on the
citty args in skills.ts (citty already infers that type), and clarify the
`skills update` --dir/--source help text to note they scope removed-detection
only, not the install location.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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")}`);
@@ -178,8 +196,8 @@ const checkCommand = defineCommand({
   },
   async run({ args }) {
     const result = await checkSkills({
-      dir: args.dir as string | undefined,
-      source: args.source as string | undefined,
+      dir: args.dir,
+      source: args.source,
     });
 
     if (args.json) console.log(JSON.stringify(withMeta(result), null, 2));
@@ -199,12 +217,36 @@ 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 dir for removed-detection only — scopes the prune, not the install (default: auto-detect)",
+    },
+    source: {
+      type: "string",
+      description:
+        "Where 'latest' comes from for removed-detection (local path, owner/repo, or URL) — does not change the install source",
+    },
+  },
+  async run({ args }) {
+    const dir = args.dir;
+    const source = args.source;
+
     // `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 +271,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,93 @@ 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);
+  });
+
+  // Regression (Magi REQUEST_CHANGES at 88daa820): the common project-local case
+  // is *also* under $HOME — a project checkout at `<home>/work/proj` with skills
+  // at `<cwd>/.claude/skills`. A HOME-first heuristic misclassified this as
+  // global, so checkSkills read the global lock and `skills update` would prune
+  // with `-g` against a project install. CWD-containment must win: `--dir` under
+  // `cwd` resolves to PROJECT even when `cwd` itself is nested under $HOME.
+  it("--dir under a cwd that is itself nested under $HOME stays project-scoped", async () => {
+    const home = join(root, "home");
+    const project = join(home, "work", "proj"); // cwd nested INSIDE home
+    const skillsDir = join(project, ".claude/skills");
+    installSkills(skillsDir, ["alpha", "gamma"]);
+    const manifestPath = join(root, "m3.json");
+    writeFileSync(
+      manifestPath,
+      JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }),
+    );
+    // Project lock at <cwd>/skills-lock.json — only read if scope is "project".
+    // No global lock exists under $HOME, so a wrong-scope ("global") read would
+    // find no lock → zero removed (gamma would NOT be flagged). The project lock
+    // being read (gamma → removed) is the proof CWD-containment won.
+    writeFileSync(
+      join(project, "skills-lock.json"),
+      JSON.stringify({
+        version: 1,
+        skills: { alpha: { source: "test" }, gamma: { source: "test" } },
+      }),
+    );
+
+    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);
+  });
+
+  // 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,10 +219,40 @@ 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).
+ *
+ * Precedence is CWD-containment FIRST, then HOME — because the common
+ * project-local case is *also* under `$HOME` (e.g. `~/work/proj/.claude/skills`,
+ * or `--dir .claude/skills` run from `~/work/proj`). Checking HOME first would
+ * misclassify every such project install as global, reading the wrong lock and
+ * (worse) letting `skills update` prune with `-g`. So:
+ *   - `dir` under `cwd`  → project (even when that's also under $HOME)
+ *   - else `dir` under $HOME → global (a real `~/.claude/skills`-style install)
+ *   - else → project (safe default — never prune globally for an unknown path)
+ *
+ * Each base is normalised with a trailing separator before the prefix test so a
+ * sibling like `/home/user2` doesn't false-match `/home/user`.
+ */
+function scopeForDir(dir: string, home: string, cwd: string): "project" | "global" {
+  const norm = (p: string): string => {
+    const r = resolve(p);
+    return r.endsWith(sep) ? r : r + sep;
+  };
+  const d = norm(dir);
+  if (d.startsWith(norm(cwd))) return "project";
+  if (d.startsWith(norm(home))) return "global";
+  return "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[],
@@ -223,7 +260,11 @@ function locateInstall(
 ): 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(), opts.cwd ?? process.cwd()),
+        }
       : null;
   }
   const roots = [
@@ -327,7 +368,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 +400,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 +547,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 +560,6 @@ export async function checkSkills(
     updateAvailable: diff.updateAvailable || removed.length > 0,
     summary: { ...diff.summary, removed: removed.length },
     skills: [...diff.skills, ...removed],
+    lockMissing,
   };
 }