fix(cli): make skills update converge on a skill retired upstream (#2176)

`hyperframes skills update` failed hard or looped forever once a skill was retired/renamed upstream while still installed locally (hyperframes-media folded into media-use; hyperframes-captions/compose/tts consolidated earlier). Two paths dead-ended: - Install: target selection could trust a stale local skills-manifest.json (findRepoManifest) while `skills add` always installs from the canonical repo. isCoreSkill matches the `hyperframes-` prefix, so a retired skill was forced into the target set, `skills add` silently declined it (exit 0), and strict verifyInstalled threw "Skill(s) still missing after install". - Prune: upstream `skills remove` scans on-disk directories, so a lock entry retired before it ever shipped a bundle has nothing to match — a silent exit-0 no-op that never clears the lock, so detectRemoved re-flags it on every run. The stale-skills nudge compounded it: it fired even from `skills update` itself (pointing users back at the failing command) and its count ignored the removed bucket. Resolve update targets against the canonical manifest (checkSkills({ canonical: true })) so a retired skill is never targeted. Add pruneOrphanedLockEntries to clear the orphaned lock entries the upstream remover can't (idempotent, so a second run is a clean no-op). Exclude `skills` from the update-nudge gate and thread the removed count through the nudge total.
2026-09-03 12:54:29 +00:00 · 2026-07-10 19:57:55 -04:00
parent 1d97ddaf8c
commit b1f1c0571e
9 changed files with 587 additions and 12 deletions
@@ -1,5 +1,15 @@
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
-import { existsSync, mkdtempSync, mkdirSync, readdirSync, rmSync, writeFileSync } from "node:fs";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+import {
+  chmodSync,
+  existsSync,
+  mkdtempSync,
+  mkdirSync,
+  readdirSync,
+  readFileSync,
+  rmSync,
+  statSync,
+  writeFileSync,
+} from "node:fs";
 import { tmpdir } from "node:os";
 import { dirname, join } from "node:path";
 import { fileURLToPath } from "node:url";
@@ -10,12 +20,33 @@ import {
  diffSkills,
  FALLBACK_CORE_SKILLS,
  isCoreSkill,
+  MANIFEST_FILE,
  presentSkills,
+  pruneOrphanedLockEntries,
  skillsAttributedToSource,
  type SkillsManifest,
  type SkillEntry,
 } from "./skillsManifest.js";

+// The retired-skill regression tests below drive `checkSkills`'s real
+// `canonical: true` network path (see resolveLatestManifest) instead of an
+// explicit local `source` — that's the whole point (it must NOT read a stale
+// local repo manifest). Stub the two network boundaries it can reach so those
+// tests stay fast and offline: `git ls-remote` (remoteHeadSha) always "fails"
+// so it falls back to the branch URL, and `fetch` is stubbed per-test. `vi.mock`
+// is hoisted above these imports regardless of source position. No existing
+// test in this file omits `source`, so nothing else touches this mock.
+vi.mock("node:child_process", () => ({
+  execFile: vi.fn(
+    (
+      _cmd: string,
+      _args: readonly string[],
+      _opts: unknown,
+      callback: (err: Error | null) => void,
+    ) => callback(new Error("no git in tests")),
+  ),
+}));
+
 let root: string;

 beforeEach(() => {
@@ -551,3 +582,173 @@ describe("checkSkills removed-upstream detection", () => {
    expect(res.summary.removed).toBe(1);
  });
 });
+
+// Regression coverage for "variant 1" of the retired-skill bug: `updateSkills`
+// (see commands/skills.ts) resolves its own targeted-install check with
+// `canonical: true` specifically so it never trusts a stale local
+// `skills-manifest.json` — this is the mechanism that makes that safe.
+describe("checkSkills canonical bypass of the in-repo manifest shortcut", () => {
+  afterEach(() => {
+    vi.unstubAllGlobals();
+  });
+
+  function stubFetchedManifest(manifest: SkillsManifest): void {
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(async () => ({ ok: true, json: async () => manifest }) as unknown as Response),
+    );
+  }
+
+  it("without canonical, a stale in-repo manifest wins (documented dev/CI shortcut)", async () => {
+    const project = join(root, "project");
+    const home = join(root, "home");
+    mkdirSync(project, { recursive: true });
+    mkdirSync(home, { recursive: true });
+    // A checked-out repo's own manifest, stale: it still lists a skill that
+    // has since been retired from the canonical published repo.
+    writeFileSync(
+      join(project, MANIFEST_FILE),
+      JSON.stringify({
+        source: "heygen-com/hyperframes",
+        skills: { "retired-skill": { hash: "x", files: 1 } },
+      }),
+    );
+
+    const res = await checkSkills({ cwd: project, home });
+    expect(res.skills.map((s) => s.name)).toContain("retired-skill");
+  });
+
+  it("with canonical:true, the same stale in-repo manifest is ignored — the fetched manifest wins", async () => {
+    const project = join(root, "project");
+    const home = join(root, "home");
+    mkdirSync(project, { recursive: true });
+    mkdirSync(home, { recursive: true });
+    writeFileSync(
+      join(project, MANIFEST_FILE),
+      JSON.stringify({
+        source: "heygen-com/hyperframes",
+        skills: { "retired-skill": { hash: "x", files: 1 }, kept: { hash: "y", files: 1 } },
+      }),
+    );
+    // The canonical (fetched) manifest no longer ships `retired-skill`.
+    stubFetchedManifest({
+      source: "heygen-com/hyperframes",
+      skills: { kept: { hash: "y", files: 1 } },
+    });
+
+    const res = await checkSkills({ cwd: project, home, canonical: true });
+    expect(res.skills.map((s) => s.name)).not.toContain("retired-skill");
+    expect(res.skills.map((s) => s.name)).toContain("kept");
+  });
+
+  it("canonical:true still honors an explicit local `source` override", async () => {
+    const project = join(root, "project");
+    const home = join(root, "home");
+    mkdirSync(project, { recursive: true });
+    mkdirSync(home, { recursive: true });
+    writeFileSync(
+      join(project, MANIFEST_FILE),
+      JSON.stringify({ source: "test", skills: { "from-repo-shortcut": { hash: "x", files: 1 } } }),
+    );
+    const explicitSource = join(root, "explicit-manifest.json");
+    writeFileSync(
+      explicitSource,
+      JSON.stringify({
+        source: "test",
+        skills: { "from-explicit-source": { hash: "y", files: 1 } },
+      }),
+    );
+
+    // An explicit `source` is a deliberate caller choice — canonical must not
+    // override it, only the silent in-repo shortcut.
+    const res = await checkSkills({ source: explicitSource, cwd: project, home, canonical: true });
+    expect(res.skills.map((s) => s.name)).toEqual(["from-explicit-source"]);
+  });
+});
+
+describe("pruneOrphanedLockEntries", () => {
+  function writeLock(path: string, skills: Record<string, { source: string }>): void {
+    writeFileSync(path, JSON.stringify({ version: 1, skills, dismissed: [] }));
+  }
+
+  it("removes only the given names, leaving other entries and lock fields intact", () => {
+    const home = join(root, "home");
+    mkdirSync(join(home, ".agents"), { recursive: true });
+    const lockPath = join(home, ".agents", ".skill-lock.json");
+    writeLock(lockPath, {
+      a: { source: "heygen-com/hyperframes" },
+      b: { source: "heygen-com/hyperframes" },
+      c: { source: "heygen-com/hyperframes" },
+    });
+
+    const pruned = pruneOrphanedLockEntries(["a", "b"], "global", { home });
+
+    expect(pruned.sort()).toEqual(["a", "b"]);
+    const rewritten = JSON.parse(readFileSync(lockPath, "utf8"));
+    expect(Object.keys(rewritten.skills)).toEqual(["c"]);
+    expect(rewritten.version).toBe(1); // other lock fields survive the rewrite
+  });
+
+  it("is idempotent — a second call with the same names finds nothing left and no-ops", () => {
+    const home = join(root, "home");
+    mkdirSync(join(home, ".agents"), { recursive: true });
+    const lockPath = join(home, ".agents", ".skill-lock.json");
+    writeLock(lockPath, { a: { source: "heygen-com/hyperframes" } });
+
+    const first = pruneOrphanedLockEntries(["a"], "global", { home });
+    expect(first).toEqual(["a"]);
+
+    const before = readFileSync(lockPath, "utf8");
+    const second = pruneOrphanedLockEntries(["a"], "global", { home });
+    expect(second).toEqual([]);
+    // No entries left to touch → the file is never rewritten a second time.
+    expect(readFileSync(lockPath, "utf8")).toBe(before);
+  });
+
+  it("writes atomically with no trailing newline, no leftover temp file, and preserves the file mode", () => {
+    const home = join(root, "home-atomic");
+    mkdirSync(join(home, ".agents"), { recursive: true });
+    const lockPath = join(home, ".agents", ".skill-lock.json");
+    writeLock(lockPath, {
+      a: { source: "heygen-com/hyperframes" },
+      b: { source: "heygen-com/hyperframes" },
+    });
+    chmodSync(lockPath, 0o640);
+
+    const pruned = pruneOrphanedLockEntries(["a"], "global", { home });
+
+    expect(pruned).toEqual(["a"]);
+    const raw = readFileSync(lockPath, "utf8");
+    expect(raw.endsWith("\n")).toBe(false);
+    expect(JSON.parse(raw).skills).toEqual({ b: { source: "heygen-com/hyperframes" } });
+    // No `.tmp` sibling left behind by the temp-file + rename.
+    expect(readdirSync(join(home, ".agents"))).toEqual([".skill-lock.json"]);
+    // Original permissions survive the rewrite (POSIX only — Windows's fs
+    // layer reports 0o666 regardless of the mode we set, so the bits aren't
+    // meaningful there).
+    if (process.platform !== "win32") {
+      expect(statSync(lockPath).mode & 0o777).toBe(0o640);
+    }
+  });
+
+  it("no-ops without throwing when the lock file doesn't exist", () => {
+    const home = join(root, "home-without-lock");
+    mkdirSync(home, { recursive: true });
+    expect(pruneOrphanedLockEntries(["a"], "global", { home })).toEqual([]);
+  });
+
+  it("resolves the project lock at <cwd>/skills-lock.json for scope: project", () => {
+    const project = join(root, "project");
+    mkdirSync(project, { recursive: true });
+    writeLock(join(project, "skills-lock.json"), {
+      a: { source: "heygen-com/hyperframes" },
+      b: { source: "heygen-com/hyperframes" },
+    });
+
+    const pruned = pruneOrphanedLockEntries(["a"], "project", { cwd: project });
+
+    expect(pruned).toEqual(["a"]);
+    const rewritten = JSON.parse(readFileSync(join(project, "skills-lock.json"), "utf8"));
+    expect(Object.keys(rewritten.skills)).toEqual(["b"]);
+  });
+});
@@ -17,7 +17,14 @@

 import { execFile } from "node:child_process";
 import { createHash } from "node:crypto";
-import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
+import {
+  existsSync,
+  readdirSync,
+  readFileSync,
+  renameSync,
+  statSync,
+  writeFileSync,
+} from "node:fs";
 import { homedir } from "node:os";
 import { isAbsolute, join, relative, resolve, sep } from "node:path";
 import { promisify } from "node:util";
@@ -515,6 +522,51 @@ function detectRemoved(
  return { removed, lockMissing: lock === null };
 }

+/**
+ * Remove `names` from the vercel-labs/skills lock at `scope`, writing the file
+ * back if anything changed. Self-heals the half of removed-upstream detection
+ * that `skills remove` can't: upstream's `remove` command scans ON-DISK skill
+ * directories to decide what's installed (see vercel-labs/skills'
+ * `removeCommand`), so a lock entry for a skill retired before it ever shipped
+ * a bundle to this machine has no on-disk dir to match. That makes `skills
+ * remove <name> -g --yes` a silent no-op — it prints "No matching skills found
+ * for: …" and exits 0 WITHOUT touching the lock. Left alone, `detectRemoved`
+ * re-flags the same lock entry as "removed" on every future run, forever.
+ *
+ * Reuses the pinned lock path (see SKILLS_CLI_LOCK_PATHS_VERIFIED_AT above —
+ * re-check that comment before bumping the upstream version this is pinned
+ * against) so this writes to exactly where the upstream CLI itself reads and
+ * writes the lock.
+ *
+ * Idempotent by construction: only entries still present in the lock are ever
+ * touched, so calling this again with the same names — after the upstream
+ * `skills remove` no-op reported above has already run once — finds nothing
+ * left and returns `[]`.
+ */
+export function pruneOrphanedLockEntries(
+  names: readonly string[],
+  scope: "project" | "global",
+  opts: { cwd?: string; home?: string } = {},
+): string[] {
+  const path = lockPathForScope(scope, opts);
+  const lock = readSkillLock(path);
+  if (!lock?.skills) return [];
+  const pruned = names.filter((name) => name in lock.skills!);
+  if (pruned.length === 0) return [];
+  for (const name of pruned) delete lock.skills[name];
+  // Atomic write (temp file + rename, same pattern as telemetry/autoUpdate.ts
+  // and utils/download.ts) so a crash mid-write can never leave a truncated
+  // lock behind. `path` is guaranteed to exist here (readSkillLock already
+  // returned a non-null lock), so preserving its mode on the temp file before
+  // the rename is safe. No trailing newline: matches the upstream
+  // vercel-labs/skills lock's on-disk shape, so a prune stays a minimal diff.
+  const mode = statSync(path).mode & 0o777;
+  const tmp = `${path}.tmp`;
+  writeFileSync(tmp, JSON.stringify(lock, null, 2), { mode });
+  renameSync(tmp, path);
+  return pruned;
+}
+
 // ── Resolving the "latest" manifest ──────────────────────────────────────────

 /** Walk up from `cwd` to find a repo checkout that ships the manifest. */
@@ -613,17 +665,27 @@ async function fetchRemoteManifest(source?: string): Promise<SkillsManifest> {
 *   - undefined → in-repo manifest if present (dev / CI), else fetch from GitHub
 *   - a local path to a manifest file or a repo root containing `skills/`
 *   - an `owner/repo` slug or full URL → fetched from GitHub
+ *
+ * `canonical: true` skips the in-repo shortcut (the `!source` branch below)
+ * even when one is found, and always resolves over the network instead. Use
+ * it for any decision that must match what `skills add` actually installs
+ * from — the canonical published repo — never a local checkout's manifest,
+ * which can be stale (e.g. still listing a skill that was retired/renamed
+ * upstream since that checkout's last pull). An explicit local `source`
+ * override is a deliberate caller choice and still wins regardless of
+ * `canonical`.
 */
 async function resolveLatestManifest(
  source?: string,
  cwd = process.cwd(),
+  opts: { canonical?: boolean } = {},
 ): Promise<SkillsManifest> {
  // A local path is a relative one (./ ../) or an absolute one — isAbsolute
  // covers POSIX `/…` and Windows `C:\…` / `\…` on their respective platforms.
  if (source && (source.startsWith(".") || isAbsolute(source))) {
    return resolveLocalManifest(source);
  }
-  if (!source) {
+  if (!source && !opts.canonical) {
    const repoManifest = findRepoManifest(cwd);
    if (repoManifest) return JSON.parse(readFileSync(repoManifest, "utf8")) as SkillsManifest;
  }
@@ -635,9 +697,16 @@ async function resolveLatestManifest(
 * manifest. Pure-ish (network only via `resolveLatestManifest`).
 */
 export async function checkSkills(
-  opts: { dir?: string; source?: string; cwd?: string; home?: string } = {},
+  opts: {
+    dir?: string;
+    source?: string;
+    cwd?: string;
+    home?: string;
+    /** See resolveLatestManifest — bypass the in-repo manifest shortcut. */
+    canonical?: boolean;
+  } = {},
 ): Promise<SkillsCheckResult> {
-  const latest = await resolveLatestManifest(opts.source, opts.cwd);
+  const latest = await resolveLatestManifest(opts.source, opts.cwd, { canonical: opts.canonical });
  const skillNames = Object.keys(latest.skills);
  const root = locateInstall(skillNames, { dir: opts.dir, cwd: opts.cwd, home: opts.home });
  const installed = root ? hashInstalled(root, skillNames) : {};
@@ -0,0 +1,111 @@
+// Nudge-count regression coverage: `refreshSkillsCache` must persist
+// `summary.removed` (renamed/dropped skills), and the printed nudge total must
+// include it — otherwise the background nudge undercounts what a plain
+// `skills update` would actually reconcile (the "misleading 2 vs 3" bug).
+
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+type FakeConfig = Record<string, unknown>;
+
+let config: FakeConfig;
+
+vi.mock("../telemetry/config.js", () => ({
+  readConfig: () => ({ ...config }),
+  writeConfig: (next: FakeConfig) => {
+    config = { ...next };
+  },
+}));
+
+vi.mock("./updateCheck.js", () => ({
+  updateNoticesSuppressed: () => false,
+}));
+
+const mockCheckSkills = vi.fn();
+vi.mock("./skillsManifest.js", () => ({
+  checkSkills: (...args: unknown[]) => mockCheckSkills(...args),
+}));
+
+describe("skillsUpdateCheck", () => {
+  beforeEach(() => {
+    vi.resetModules();
+    config = {};
+    mockCheckSkills.mockReset();
+  });
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  it("refreshSkillsCache persists the removed count alongside outdated/missing", async () => {
+    mockCheckSkills.mockResolvedValue({
+      location: "/home/user/.claude/skills",
+      updateAvailable: true,
+      summary: { current: 1, outdated: 2, missing: 3, coreMissing: 1, removed: 3 },
+    });
+
+    const { checkSkillsForUpdate } = await import("./skillsUpdateCheck.js");
+    const meta = await checkSkillsForUpdate(true);
+
+    expect(meta).toEqual({ updateAvailable: true, outdated: 2, missing: 1, removed: 3 });
+    expect(config["skillsRemovedCount"]).toBe(3);
+    // Must resolve against the canonical upstream manifest, not a possibly
+    // stale in-repo skills-manifest.json, so this nudge agrees with what
+    // `updateSkills` would actually reconcile.
+    expect(mockCheckSkills).toHaveBeenCalledWith({ canonical: true });
+  });
+
+  it("does not persist anything when no install was located (nothing meaningful to cache)", async () => {
+    mockCheckSkills.mockResolvedValue({
+      location: null,
+      updateAvailable: false,
+      summary: { current: 0, outdated: 0, missing: 0, coreMissing: 0, removed: 0 },
+    });
+
+    const { checkSkillsForUpdate } = await import("./skillsUpdateCheck.js");
+    await checkSkillsForUpdate(true);
+
+    expect(config["skillsRemovedCount"]).toBeUndefined();
+  });
+
+  /** Drive printSkillsUpdateNotice from the given cache shape; returns what it wrote (if anything). */
+  async function noticeTextFor(cache: FakeConfig): Promise<string | null> {
+    config = cache;
+    const { printSkillsUpdateNotice } = await import("./skillsUpdateCheck.js");
+    const writeSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
+
+    printSkillsUpdateNotice();
+
+    if (writeSpy.mock.calls.length === 0) return null;
+    expect(writeSpy).toHaveBeenCalledTimes(1);
+    return String(writeSpy.mock.calls[0]?.[0]);
+  }
+
+  it("the cached nudge total counts removed skills, not just outdated/missing", async () => {
+    // Cache pre-populated as if a prior refreshSkillsCache had run — only
+    // outdated + missing, no removed (the pre-fix shape).
+    const text = await noticeTextFor({
+      skillsOutdatedCount: 1,
+      skillsMissingCount: 1,
+      skillsRemovedCount: 2,
+    });
+    // 1 outdated + 1 missing + 2 removed = 4, not the pre-fix "2".
+    expect(text).toContain("4 HyperFrames skills out of date or missing");
+  });
+
+  it("prints nothing when outdated, missing, and removed are all zero", async () => {
+    const text = await noticeTextFor({
+      skillsOutdatedCount: 0,
+      skillsMissingCount: 0,
+      skillsRemovedCount: 0,
+    });
+    expect(text).toBeNull();
+  });
+
+  it("a removed-only count (no outdated/missing) still triggers the nudge", async () => {
+    const text = await noticeTextFor({
+      skillsOutdatedCount: 0,
+      skillsMissingCount: 0,
+      skillsRemovedCount: 1,
+    });
+    expect(text).toContain("1 HyperFrames skill out of date or missing");
+  });
+});
@@ -16,6 +16,8 @@ export interface SkillsUpdateMeta {
  updateAvailable: boolean;
  outdated: number;
  missing: number;
+  /** Installed skills flagged removed-upstream (renamed/dropped) at the last check. */
+  removed: number;
 }

 /** Synchronous read from cache — never fetches. */
@@ -25,6 +27,7 @@ function getSkillsUpdateMeta(): SkillsUpdateMeta {
    updateAvailable: config.skillsUpdateAvailable ?? false,
    outdated: config.skillsOutdatedCount ?? 0,
    missing: config.skillsMissingCount ?? 0,
+    removed: config.skillsRemovedCount ?? 0,
  };
 }

@@ -35,7 +38,10 @@ function cacheFresh(lastSkillsCheck: string | undefined, now: number): boolean {

 /** Run the real check and persist the result to the cache. */
 async function refreshSkillsCache(): Promise<SkillsUpdateMeta> {
-  const result = await checkSkills();
+  // `canonical: true` so this nudge's counts agree with `updateSkills`'s
+  // source of truth — otherwise a stale in-repo skills-manifest.json (e.g.
+  // inside a hyperframes checkout) can produce a false-positive count here.
+  const result = await checkSkills({ canonical: true });
  // Only record a meaningful check when skills were actually found.
  if (result.location) {
    const config = readConfig();
@@ -45,12 +51,18 @@ async function refreshSkillsCache(): Promise<SkillsUpdateMeta> {
    // Core-missing only: skills that install on demand (workflows not yet
    // triggered on this machine) are not "missing" worth nagging about.
    config.skillsMissingCount = result.summary.coreMissing;
+    // Removed-upstream skills are just as reconcilable as outdated/missing
+    // ones (a plain `skills update` prunes them) — omitting them here is what
+    // made the nudge undercount (e.g. reporting "2 skills out of date or
+    // missing" while a 3rd, renamed/dropped skill sat unmentioned).
+    config.skillsRemovedCount = result.summary.removed;
    writeConfig(config);
  }
  return {
    updateAvailable: result.updateAvailable,
    outdated: result.summary.outdated,
    missing: result.summary.coreMissing,
+    removed: result.summary.removed,
  };
 }

@@ -70,9 +82,9 @@ export async function checkSkillsForUpdate(force?: boolean): Promise<SkillsUpdat
  }
 }

-/** The stale-skills nudge text, or null when nothing is outdated or missing. */
+/** The stale-skills nudge text, or null when nothing is outdated, missing, or removed. */
 function skillsNoticeText(meta: SkillsUpdateMeta): string | null {
-  const total = meta.outdated + meta.missing;
+  const total = meta.outdated + meta.missing + meta.removed;
  if (total < 1) return null;
  const noun = total === 1 ? "skill" : "skills";
  return `\n  ${total} HyperFrames ${noun} out of date or missing.\n  Run: npx hyperframes skills update\n\n`;