From b1f1c0571e0dbbc6f12ccfde68345349704ee86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 10 Jul 2026 19:57:55 -0400 Subject: [PATCH] fix(cli): make skills update converge on a skill retired upstream (#2176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- packages/cli/src/cli.commands.test.ts | 13 ++ packages/cli/src/cli.ts | 14 +- packages/cli/src/commands/skills.test.ts | 114 ++++++++++ packages/cli/src/commands/skills.ts | 44 +++- packages/cli/src/telemetry/config.ts | 3 + packages/cli/src/utils/skillsManifest.test.ts | 205 +++++++++++++++++- packages/cli/src/utils/skillsManifest.ts | 77 ++++++- .../cli/src/utils/skillsUpdateCheck.test.ts | 111 ++++++++++ packages/cli/src/utils/skillsUpdateCheck.ts | 18 +- 9 files changed, 587 insertions(+), 12 deletions(-) create mode 100644 packages/cli/src/utils/skillsUpdateCheck.test.ts diff --git a/packages/cli/src/cli.commands.test.ts b/packages/cli/src/cli.commands.test.ts index 4a533eeb5..cc62420cf 100644 --- a/packages/cli/src/cli.commands.test.ts +++ b/packages/cli/src/cli.commands.test.ts @@ -26,4 +26,17 @@ describe("CLI command registration", () => { '["keyframes", "Inspect keyframes and render onion-shot diagnostics"]', ); }); + + // A command actively reconciling skills (`skills check`/`skills update`) + // must not also nudge the user to go reconcile skills — that nudge is + // either redundant (it just ran) or misleading (a stale cached count from + // the 24h background check, contradicting whatever it just reported). + it("excludes 'skills' from the background skills-nudge gate, alongside 'upgrade' and 'events'", () => { + const match = cliSource.match(/if \(([\s\S]*?)\) \{\s*\/\/ Report any completed auto-install/); + expect(match, "expected to find the background nudge gate's if-condition").toBeTruthy(); + const condition = match![1]!; + expect(condition).toContain('command !== "upgrade"'); + expect(condition).toContain('command !== "events"'); + expect(condition).toContain('command !== "skills"'); + }); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 69c7281d7..a640bf218 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -225,7 +225,19 @@ if (!isHelp && command !== "telemetry" && command !== "events" && command !== "u // `events` skips the update check too — a skill-usage beacon must not add // network latency or trigger a background self-upgrade on the calling skill. -if (!isHelp && !hasJsonFlag && command !== "upgrade" && command !== "events") { +// `skills` is excluded from the SKILLS nudge for the same reason `upgrade` is +// excluded from the self-update notice: a command that is itself actively +// checking/reconciling skills (`skills check`, `skills update`) must not also +// tell the user to go run `skills update` — that's either redundant (it just +// did) or, worse, misleading (it printed a stale nudge count from the last +// cached check while reporting fresh results of its own). +if ( + !isHelp && + !hasJsonFlag && + command !== "upgrade" && + command !== "events" && + command !== "skills" +) { // Report any completed auto-install from the previous run first, before // kicking off the next check — so the user sees "updated to vX" once and // we don't over-print. diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 6995f28bd..0c4b2e85d 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -97,6 +97,11 @@ vi.mock("../utils/skillsManifest.js", async (importOriginal) => { checkSkills: vi.fn(async () => DEFAULT_CHECK), hyperframesSkillNames: vi.fn(() => ["hyperframes"]), presentSkills: vi.fn((names: readonly string[]) => [...names]), + // Default: nothing left to prune after `runSkillsRemove`. The real + // (unmocked) fs-level behavior is covered in skillsManifest.test.ts; + // here we only assert the wiring — what update passes in, and that it + // isn't reached when there's nothing removed. + pruneOrphanedLockEntries: vi.fn(() => []), }; }); @@ -383,6 +388,81 @@ describe("hyperframes skills", () => { expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); }); + // Retired-skill regression (variant 1): the update engine's OWN targeted- + // install check must resolve the canonical (published) manifest, never a + // stale local `skills-manifest.json` a checkout might still have lying + // around — see resolveLatestManifest's in-repo shortcut. Without this, a + // skill retired upstream but still listed locally gets forced into + // `targets` (isCoreSkill pattern-matches `hyperframes-*`), `skills add` + // silently declines to install something that doesn't exist canonically, + // and the old code strict-threw on a "failure" that was never real. + it("checks freshness against the canonical manifest, never a possibly-stale local one", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + + await runSkillsUpdate(); + + // The update engine's own check (first call) must ask for canonical; + // the prune's check (last call, tested separately) intentionally doesn't. + expect(checkSkills).toHaveBeenNthCalledWith(1, expect.objectContaining({ canonical: true })); + }); + + // Retired-skill regression (variant 2): `skills remove` is a silent no-op + // for a lock entry with no on-disk bundle (upstream scans disk, not the + // lock, to decide what's "installed" — see pruneOrphanedLockEntries's + // doc comment). `skills update` must self-heal that lock entry itself so + // `check || update` actually converges instead of re-flagging it forever. + it("self-heals an orphaned lock entry after `skills remove` no-ops on it", async () => { + setPlatform("linux"); + const { checkSkills, pruneOrphanedLockEntries } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills) + .mockResolvedValueOnce(DEFAULT_CHECK as never) + .mockResolvedValueOnce({ + scope: "global", + skills: [{ name: "hyperframes-captions", status: "removed" }], + } as never); + vi.mocked(pruneOrphanedLockEntries).mockReturnValueOnce(["hyperframes-captions"]); + + await runSkillsUpdate(); + + expect(pruneOrphanedLockEntries).toHaveBeenCalledWith(["hyperframes-captions"], "global"); + expect(process.exitCode).toBe(0); + }); + + // The idempotent-second-run contract at the command level: once nothing is + // left attributed as removed (the fs-level idempotency of the prune itself + // is covered directly in skillsManifest.test.ts), a second `skills update` + // must be a clean no-op — no `skills remove` spawn, no prune call finding + // anything, still exit 0. + it("running update twice in a row converges — the second run prunes nothing", async () => { + setPlatform("linux"); + const { checkSkills, pruneOrphanedLockEntries } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills) + .mockResolvedValueOnce(DEFAULT_CHECK as never) + .mockResolvedValueOnce({ + scope: "global", + skills: [{ name: "hyperframes-captions", status: "removed" }], + } as never); + vi.mocked(pruneOrphanedLockEntries).mockReturnValueOnce(["hyperframes-captions"]); + + await runSkillsUpdate(); + expect(process.exitCode).toBe(0); + expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(true); + + // Second run: nothing attributed as removed anymore (the lock entry was + // pruned above), so there's nothing left to reconcile. + state.spawnCalls = []; + vi.mocked(checkSkills) + .mockResolvedValueOnce(DEFAULT_CHECK as never) + .mockResolvedValueOnce({ scope: "global", skills: [] } as never); + + await runSkillsUpdate(); + expect(process.exitCode).toBe(0); + expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); + // Nothing to prune this time — pruneOrphanedLockEntries isn't even reached. + expect(pruneOrphanedLockEntries).toHaveBeenCalledTimes(1); + }); + // `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. @@ -615,6 +695,40 @@ describe("hyperframes skills update ", () => { expect(process.exitCode).toBe(1); }); + it("a malformed canonical manifest warns distinctly, then still degrades to presence mode", async () => { + setPlatform("linux"); + const clack = await import("@clack/prompts"); + vi.mocked(clack.log.warn).mockClear(); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockRejectedValue( + new Error("Malformed skills manifest from https://raw.githubusercontent.com/…"), + ); + + await runSkillsUpdateWith(["pr-to-video"]); + + const warnedMalformed = vi + .mocked(clack.log.warn) + .mock.calls.some((args) => String(args[0]).includes("malformed")); + expect(warnedMalformed).toBe(true); + // Still degrades rather than failing the whole command. + expect(process.exitCode).toBe(0); + }); + + it("a genuine offline error degrades silently — no malformed-manifest warning", async () => { + setPlatform("linux"); + const clack = await import("@clack/prompts"); + vi.mocked(clack.log.warn).mockClear(); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockRejectedValue(new Error("fetch failed")); + + await runSkillsUpdateWith(["pr-to-video"]); + + const warnedMalformed = vi + .mocked(clack.log.warn) + .mock.calls.some((args) => String(args[0]).includes("malformed")); + expect(warnedMalformed).toBe(false); + }); + it("--json emits a parseable result on success", async () => { setPlatform("linux"); const logSpy = vi.spyOn(console, "log"); diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index d7e7e87ff..a7a4c0d72 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -10,6 +10,7 @@ import { hyperframesSkillNames, isCoreSkill, presentSkills, + pruneOrphanedLockEntries, SKILLS_CLI_LOCK_PATHS_VERIFIED_AT, type SkillDiff, type SkillsCheckResult, @@ -307,8 +308,29 @@ export async function updateSkills( let check: SkillsCheckResult | null = null; try { - check = await checkSkills({ cwd: opts.cwd }); - } catch { + // `canonical: true` — target selection must match what `skills add` + // actually installs from (the canonical published repo), never a local + // checkout's `skills-manifest.json`. Without this, running from inside a + // stale hyperframes checkout could resolve "latest" from that stale local + // file, which may still list a skill that's since been retired/renamed + // upstream. `isCoreSkill` would then force it into `targets`/`toInstall`, + // `skills add` would correctly (and silently) decline to install a skill + // that no longer exists, and verifyInstalled would strict-throw on a + // "failure" that was never real. Resolving canonically means a retired + // skill simply never appears as a target in the first place. + check = await checkSkills({ cwd: opts.cwd, canonical: true }); + } catch (err) { + // A *malformed* canonical manifest (the server was reached, but served a + // bad shape) is otherwise indistinguishable from being offline — both fall + // through to presence-only mode below. Surface it distinctly so ops can + // tell an upstream/CDN problem apart from a genuine network failure. + if (err instanceof Error && err.message.startsWith("Malformed skills manifest")) { + clack.log.warn( + c.warn( + "Canonical skills manifest was malformed — falling back to presence-only mode (an upstream/CDN issue, not your network).", + ), + ); + } check = null; // manifest unreachable (offline / rate-limited) — presence mode below } if (!check) return updateSkillsOffline(requested, { strict, cwd: opts.cwd }); @@ -687,6 +709,24 @@ const updateCommand = defineCommand({ c.dim(`Removing ${removed.length} skill(s) no longer published: ${removed.join(", ")}`), ); await runSkillsRemove(removed, { global: scope === "global" }); + // Self-heal: `skills remove` only clears a lock entry for a name it + // found an on-disk bundle for (see pruneOrphanedLockEntries). A skill + // retired before it ever shipped a bundle to this machine has none, so + // the call above is a silent no-op for it — the lock entry lingers and + // would be re-flagged "removed" on every future run. Prune whatever is + // still attributed after the call so `check || update` converges + // instead of looping forever. Best-effort and scoped to exactly the + // lock the remove above targeted (same `scope`); a write failure here + // must not fail the update — the install already succeeded. + const scopeForPrune = scope ?? "global"; + const stillOrphaned = pruneOrphanedLockEntries(removed, scopeForPrune); + if (stillOrphaned.length) { + console.log( + c.dim( + `Reconciled ${stillOrphaned.length} orphaned lock entr${stillOrphaned.length === 1 ? "y" : "ies"} with no on-disk bundle: ${stillOrphaned.join(", ")}`, + ), + ); + } } } catch (err) { clack.log.warn(c.warn(`Skipped removed-skill cleanup: ${(err as Error).message}`)); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 0a6d3461c..5ae15f8d7 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -63,6 +63,8 @@ export interface HyperframesConfig { skillsOutdatedCount?: number; /** How many skills were missing (not installed) at the last check. */ skillsMissingCount?: number; + /** How many installed skills were flagged removed-upstream at the last check. */ + skillsRemovedCount?: number; } const DEFAULT_CONFIG: HyperframesConfig = { @@ -108,6 +110,7 @@ export function readConfig(): HyperframesConfig { skillsUpdateAvailable: parsed.skillsUpdateAvailable, skillsOutdatedCount: parsed.skillsOutdatedCount, skillsMissingCount: parsed.skillsMissingCount, + skillsRemovedCount: parsed.skillsRemovedCount, }; cachedConfig = config; diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index c3c7682f0..f5a56728c 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -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): 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 /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"]); + }); +}); diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index e58883dca..5145cc034 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -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 -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 { * - 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 { // 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 { - 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) : {}; diff --git a/packages/cli/src/utils/skillsUpdateCheck.test.ts b/packages/cli/src/utils/skillsUpdateCheck.test.ts new file mode 100644 index 000000000..9028f9051 --- /dev/null +++ b/packages/cli/src/utils/skillsUpdateCheck.test.ts @@ -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; + +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 { + 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"); + }); +}); diff --git a/packages/cli/src/utils/skillsUpdateCheck.ts b/packages/cli/src/utils/skillsUpdateCheck.ts index d29c2f3a0..c2cf384d2 100644 --- a/packages/cli/src/utils/skillsUpdateCheck.ts +++ b/packages/cli/src/utils/skillsUpdateCheck.ts @@ -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 { - 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 { // 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