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

[miguel-heygen]

Summary

• hyperframes skills update now converges when a skill has been retired/renamed upstream while still installed locally — instead of failing hard or looping forever.
• Target selection resolves the canonical published manifest, so a retired skill is never (re)installed from a repo that no longer ships it.
• Orphaned lock entries with no on-disk bundle are pruned directly, so check || update reconciles instead of re-flagging them on every run.
• The stale-skills nudge no longer fires from skills / skills update itself, and its count now includes removed-upstream skills.

Reproduction

Both surfaced after hyperframes-media was folded into media-use (and earlier hyperframes-captions/-compose/-tts were consolidated), for users who still had the old skills installed:

• Inside a repo checkout: npx hyperframes skills update → Update failed: Skill(s) still missing after install: hyperframes-media.
• Outside any repo: npx hyperframes skills update → No matching skills found for: hyperframes-captions, hyperframes-compose, hyperframes-tts, and the same nudge reappears on every rerun.

Root cause

skills update had no notion of "gone upstream". A retired skill reached the engine two ways, and both dead-ended:

• Install path — resolveLatestManifest could resolve latest from a stale local skills-manifest.json (findRepoManifest), while skills add always installs from the canonical repo. isCoreSkill matches by the hyperframes- prefix, so a retired skill was forced into the target set; skills add silently declined it (exit 0), and strict verifyInstalled threw.
• Prune path — upstream skills remove scans on-disk directories, so a lock entry retired before it ever shipped a bundle has nothing to match. The call is a silent exit-0 no-op that never touches the lock, so detectRemoved re-flags the same entry forever.

The nudge compounded it: it fired even from skills update, pointing users back at the command that just failed, and its count ignored the removed bucket (the "2 vs 3" mismatch).

Fix

• updateSkills resolves targets against the canonical manifest (checkSkills({ canonical: true })), so a retired skill never becomes a target.
• New pruneOrphanedLockEntries clears the orphaned lock entries the upstream remover can't, reusing the pinned lock path (SKILLS_CLI_LOCK_PATHS_VERIFIED_AT). Idempotent — a second run is a clean no-op.
• Exclude skills from the update-nudge gate, and thread the removed count through the nudge total.

Test plan

• bun run --filter @hyperframes/cli test — green. Covers the canonical-manifest check, lock self-heal, two-runs-converge wiring, and the pruneOrphanedLockEntries unit cases (remove-only-named, idempotent, missing-lock no-op, scope resolution).
• bunx oxlint / bunx oxfmt --check on changed files.
• bun run build.
• CI green on all platforms (incl. Windows).

[miguel-heygen]

Thanks for the thorough pass. Addressed the non-blockers in eded948c8:

• Atomic lock write + mode preservation — pruneOrphanedLockEntries now writes to a temp file and renameSyncs into place, preserving the original mode (statSync(path).mode & 0o777). Worth noting: config.ts's writeConfig on this branch isn't actually atomic, so I mirrored the real temp+rename pattern already used in utils/download.ts / utils/autoUpdate.ts. This also covers the file-permissions note.
• Lock on-disk shape — dropped the trailing newline; the write is now JSON.stringify(lock, null, 2) (2-space indent, no trailing \n), matching the upstream lock's shape, so a prune is a minimal diff instead of reformat noise.
• Source-of-truth consistency — refreshSkillsCache now calls checkSkills({ canonical: true }), so the background nudge count agrees with updateSkills. Safe offline: checkSkillsForUpdate already try/catches and falls back to cached meta on a fetch failure.

New test asserts the atomic write, no trailing newline, no leftover temp file, and preserved mode; plus a canonical: true assertion on the nudge refresh.

Acknowledged, intentionally left as-is:

• Offline skills update inside a checkout now degrades to presence mode rather than resolving via the in-repo manifest — intentional, since that shortcut was exactly the variant-1 bug vector. Flagging it for anyone who relied on it.
• nudge-gate source-regex test — agreed it's shape-fragile, but the intent assertion is deliberate; leaving it.

Lock-JSON indentation confirmed against the on-disk upstream lock (2-space). SKILLS_CLI_LOCK_PATHS_VERIFIED_AT remains the pin to re-check on any upstream bump.

[vanceingalls]

R1 — Verdict: 🟢 LGTM

The mystery of the perpetually reappearing hyperframes-media, and its cousin the nightly nudge counting to two when the truth was three, has been closed with — I am pleased to report — the correct instruments. Two failure modes, two mechanisms, and a convergence proof that both a. the immediate bug and b. the second-run-still-fails class are neutralised.

I audited head eded948c89972bef776f4138ab1908cf08a4b3a2. (An earlier SHA I began with, 06c74119, had already been superseded by force-push during the read; the two atomic-write refinements below are new to eded948c and I re-read the whole surface at the new SHA.)

The five load-bearing invariants — verified

1. pruneOrphanedLockEntries — the new self-heal primitive — packages/cli/src/utils/skillsManifest.ts:539-566. Reads the lock at exactly the same pinned path the upstream CLI reads/writes (SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = vercel-labs/skills@v1.5.13, unchanged constant), filters names against lock.skills (idempotent by construction — a second call finds nothing left, no rewrite), and — this is the interesting refinement at the new SHA — writes the lock atomically via <path>.tmp + renameSync, preserving mode. So a crash mid-write cannot leave a truncated JSON behind that would poison every subsequent load. The false-prune-on-live-skill hazard I looked for is absent: the function only ever touches entries whose names it was passed, which come solely from check.skills.filter(status === "removed") — the source-attributed, manifest-diff'd removed set. A concurrent live install writing new keys would sit in a different position in the map and remain untouched.

2. checkSkills({ canonical: true }) bypasses the in-repo shortcut — packages/cli/src/utils/skillsManifest.ts:662-677. resolveLatestManifest now branches: canonical: true skips the findRepoManifest(cwd) climb entirely, always fetching from raw.githubusercontent.com with git ls-remote SHA-pinning. An explicit --source still wins (deliberate caller intent). Failure mode when the fetch dies: the try/catch in updateSkills at packages/cli/src/commands/skills.ts:309-325 swallows to null and falls into updateSkillsOffline, which uses FALLBACK_CORE_SKILLS — a pinned enumeration that does not include hyperframes-media. So there is no silent path where a retired skill re-enters the target set on a degraded network. Belt AND braces.

3. Nudge exclusion + removed count threaded through — packages/cli/src/cli.ts:224-230 excludes skills from firing the nudge (the loop vance hit outside a repo cannot recur — skills update no longer re-prints "run skills update"). packages/cli/src/utils/skillsUpdateCheck.ts:83-88 computes total = outdated + missing + removed, and — again at the new SHA — the background refresh at line 41 now calls checkSkills({ canonical: true }), so the cached nudge count agrees with what updateSkills will actually reconcile rather than a stale in-repo manifest's view. Test at skillsUpdateCheck.test.ts:38-50 pins skillsRemovedCount: 3 and total: 4, exactly the pre-fix "misleading 2 vs 3" shape.

4. Two-runs-converge — packages/cli/src/commands/skills.test.ts:437-464. First run: removed: ["hyperframes-captions"] → runSkillsRemove spawns, pruneOrphanedLockEntries returns ["hyperframes-captions"], exit 0. Second run: checkSkills mocked to return skills: [], so no remove spawn, no prune call. pruneOrphanedLockEntries remains called only once across both runs. That is the exact contract for vance's Shape A (the in-repo hyperframes-media install-verify-throw): after run one, the lock entry is gone; run two finds nothing to reconcile.

5. isCoreSkill prefix match is defused by ordering — packages/cli/src/utils/skillsManifest.ts:130-132. isCoreSkill still matches hyperframes-* as a prefix, which is fine because it is now applied AFTER the canonical filter: manifestSkills = check.skills.filter(s => s.status !== "removed") at commands/skills.ts:329, and only THEN .filter(s => ... isCoreSkill(s.name) ...) at line 343. A retired skill never appears in manifestSkills at all (canonical no longer ships it), so the pattern match has nothing to force in. The prefix check itself is deliberately unchanged — it still needs to catch future hyperframes-* additions before they land in the fallback list.

Both of vance's shapes are covered

• Shape A (in-repo) — Skill(s) still missing after install: hyperframes-media — the install-path/verify-throw. Covered by the canonical-manifest test at skills.test.ts:399-408 (mechanism) plus the two-runs-converge test at 437-464 (end-to-end). The retired skill never enters toInstall, verifyInstalled never asserts on it, no strict-throw.
• Shape B (outside repo) — No matching skills found for: hyperframes-captions, hyperframes-compose, hyperframes-tts + 2 HyperFrames skills out of date or missing. Run: npx hyperframes skills update — the nudge-loop. Covered by the exclusion at cli.ts:224-230 (the nudge never fires from inside skills update itself), the count fix at skillsUpdateCheck.ts:84-87 (removed skills counted), and the self-heal at skills.ts:709-718 + skillsManifest.ts:539-566 (the underlying lock entry actually gets cleared, so the cached count on the next run's refresh drops to zero).

Small notes, not requesting changes

• The atomic-write on the lock (renameSync) is the correct pattern for a file the upstream CLI also reads/writes; a crash-mid-write leaving the lock corrupt would have been a nastier failure mode than the one this PR was fixing, so I appreciate seeing it addressed proactively rather than as a follow-up.
• The failed-fetch path in updateSkills is a silent swallow to null (line 322 catch). That is the correct choice for offline-degrade — but if some future refactor turns a malformed canonical response (asSkillsManifest throw) into a silent skip, it would look identical to genuine offline and land users in updateSkillsOffline presence-only mode. Not a fix-now concern (the guard is already there and the mock covers offline correctly), just worth flagging: a malformed-canonical case masquerading as offline would degrade the guarantee. If future you cares, a distinct log line for "unreachable" vs "malformed" would help ops.

CI green when I looked. Envelope hygiene: no Co-Authored-By: Claude trailers, no 🤖 Generated with [Claude Code] in the body — clean.

R1 by Via

[miguel-heygen]

Thanks for the deep audit — and good catch on the malformed-vs-offline ambiguity. I folded it in (9b5638dba) rather than leaving it for future-me:

The updateSkills catch now distinguishes a malformed canonical manifest (server reached, bad shape → asSkillsManifest throw) from a genuine network failure. A malformed response emits a distinct clack.log.warn ("Canonical skills manifest was malformed — falling back to presence-only mode (an upstream/CDN issue, not your network)") before degrading, so ops can tell "the CDN served us garbage" apart from "you're offline". Rate-limit/HTTP errors stay in the offline bucket on purpose — a 403/429 really is a degraded-network condition, whereas a bad shape is the upstream problem you flagged.

Two tests cover it: a malformed manifest warns and still degrades (exit 0); a plain network error degrades silently with no false malformed warning.

Everything else stands as you verified. Appreciated the note on the atomic write too.