fix(cli): stop skills update deleting skills the manifest never covered

[jrusso1020]

What

Scopes skills update's removed-skill prune to skills the published manifest is actually authoritative for, so it stops deleting skills the same command just installed.

Fixes #3111.

Why

hyperframes skills update deleted skills from every agent directory on the machine and reported them as "no longer published" — while they were, in fact, published.

The chain:

1. skills add --skill '*' installs every skill in the repo. That includes the repo-native ones under .claude/skills/ and .agents/skills/, not just the 19 under skills/.
2. The upstream lock attributes all of them to heygen-com/hyperframes — source is identical for every entry.
3. The published manifest is generated from <repoRoot>/skills only (packages/cli/scripts/gen-skills-manifest.ts:18), so it never lists the repo-native ones.
4. detectRemoved read "absent from the manifest" as "removed upstream" and pruned them.

So check || update could never converge: add reinstalls those skills, the next update deletes them again, forever.

The category error is in step 4. The manifest answers "is this skill's content current?" for the set it covers. The prune reuses it to answer "does this skill still exist upstream?" — a different question, and for anything outside skills/ the manifest is simply silent. Silence was being read as a delete instruction.

How

1. Scope the prune to the manifest's coverage root (skillsManifest.ts). The lock's skillPath records where in the repo each skill came from — skills/general-video/SKILL.md vs .agents/skills/seam-craft/SKILL.md — and it is the only field that separates them, since source is the same for both. detectRemoved now only considers entries under skills/.

An entry with no skillPath (older upstream lock format) is treated as not covered. This is a delete path, so unknown provenance fails safe.

2. Resolve the prune's manifest canonically (skills.ts). Without it, resolveLatestManifest takes the findRepoManifest shortcut: any skills-manifest.json within 16 parent directories of cwd becomes "latest". Since HyperFrames' own manifest declares source: heygen-com/hyperframes, such a file matches lock attribution and drives deletion. The install-side check already resolved canonically (#2176); the deleting path did not — and the comment asserting that was deliberate and "tested separately" had no such test. An explicit --source still wins, because canonical only decides what "latest" means when no source was given.

Scope note for reviewers: only change 1 is backed by a reproduction. Change 2 fixes a contract the code documents but does not enforce; I found it while investigating and it sits directly in this deletion path, but I could not produce a user-visible failure from it at main (there, local and canonical manifests are identical). Easy to split out if you'd rather.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

End to end against the real CLI, sandboxed HOME, run from a checkout at b08cefea6:

	before	after
skills add --skill '*'	25 skills installed	25 installed
skills update	Removing 6 skill(s) no longer published: captions-overlay, changelog-video, cut-the-curve, motion-doctrine, oversized-cursor, seam-craft	no removal line
~/.claude/skills	27 → 21	27 → 27

Two hand-authored, unrelated skills were present in the store throughout; they survived both runs (see the issue thread — they are not what this bug deletes).

Regression tests, all three fail on the pre-fix source and pass after:

• never prunes a skill installed outside the manifest's coverage root — the bug itself.
• leaves an entry with no skillPath alone rather than guessing — the fail-safe.
• resolves the prune's manifest canonically, so a local manifest can never drive deletion — asserts on every checkSkills call so a future caller can't reintroduce a non-canonical delete path.

Plus still prunes a manifest-covered skill that was genuinely dropped upstream, which passes both before and after — the guard against over-correcting and blunting the retired-skill convergence from #2176.

Existing lock fixtures were widened to carry skillPath. They previously omitted a field upstream writes on every entry (verified against a real ~/.agents/.skill-lock.json written by skills@1.5.22), so they under-modelled the real lock; the helper defaults to the covered root, which keeps each existing assertion meaning exactly what it meant before.

oxfmt, oxlint, and tsc --noEmit clean; 98 tests pass across both affected files.

— Rames Jusso

[miguel-heygen]

No blocking findings.

1. skillPath is the right discriminator. The manifest authority is explicitly the repo skills/ root (packages/cli/src/utils/skillsManifest.ts:518-539), and removed detection now requires both source attribution and a path under that root (packages/cli/src/utils/skillsManifest.ts:554-565). I checked this against vercel-labs/skills@1.5.22: GitHub installs write repo-relative, forward-slash-normalized skillPath values to both lock formats, so skills/<name>/SKILL.md and .agents/.claude roots are distinguishable exactly this way. I also checked the generated manifest: it walks only <repo>/skills.

2. Failing safe when skillPath is absent is the correct trade. A destructive reconciliation cannot infer coverage from source alone. The regression test at packages/cli/src/utils/skillsManifest.test.ts:582-598 pins that behavior, while :566-580 proves a covered retired skill still converges. This also matches upstream’s own update delete path, which refuses removal when a lock entry has no skillPath. An old-format stale entry may persist, but that is preferable to another false-positive delete.

3. The canonical-manifest change belongs in this fix. The prune is a delete decision, so a nearby/stale checkout manifest must not define “published.” The call at packages/cli/src/commands/skills.ts:729-740 now uses the same canonical source as installation; resolveLatestManifest still honors any explicit source override (packages/cli/src/utils/skillsManifest.ts:706-735). The tests at packages/cli/src/utils/skillsManifest.test.ts:740-804 pin both the bypass and override behavior. This closes a separate route to the same destructive misclassification even though it did not reproduce visibly at current main.

4. The fixture widening is legitimate. Current upstream GitHub lock entries do carry skillPath; defaulting existing fixtures to skills/<name>/SKILL.md preserves their original semantic (“manifest-covered published skill”). The new tests explicitly opt out for the repo-native root (packages/cli/src/utils/skillsManifest.test.ts:536-564) and bypass the helper entirely for the legacy no-path case, so the suite is not hiding either boundary.

Locally verified the two touched suites: 98/98 tests passed. Exact-head CI at 611dbd237 is terminal-green; I also confirmed the new tests ran in the general Test job despite the narrower skills jobs being path-skipped. The PR correctly does not claim to explain the reporter’s separate unrelated-skill loss, so keeping #3111 open is appropriate.

Verdict: APPROVE. The delete authority is now scoped to what the manifest can actually speak for, unknown provenance fails safe, and retired manifest-covered skills still converge.

— Magi