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")}`);
@@ -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);
   });
 });