feat(cli): surface and prune skills removed upstream

[jrusso1020]

What

Make hyperframes skills check / update aware of skills that were renamed or removed upstream (e.g. graphic-overlays → talking-head-recut in #1720).

• skills check now reports a removed category and counts it toward the non-zero exit.
• skills update prunes those skills after installing, so check || update fully reconciles the install with the manifest.

Why

skills add / init / hyperframes skills update (which wraps skills add --all) only ever add or refresh — none of them delete a skill that's been renamed or dropped upstream. And diffSkills deliberately ignored any installed skill not in the manifest, so a stale bundle (like graphic-overlays after the talking-head-recut rename) lingered forever: skills check said "up to date" while a dead skill sat in the agent's skills dir, and there was no first-class cleanup path.

This came up cleaning up a real install that had accumulated renamed/removed skills across several releases.

How

• Attribution, not guessing. .../skills is shared across sources, so we can't call a directory "ours but removed" from its name alone. We cross-reference the vercel-labs/skills lock ($HOME/.agents/.skill-lock.json or the XDG path; skills-lock.json for project scope): a skill the lock attributes to our manifest source (heygen-com/hyperframes, matched by slug or clone URL) that the manifest no longer lists is removed.
• SkillStatus gains "removed"; SkillsCheckResult gains removed (summary) and scope. diffSkills is unchanged (pure manifest diff) — removed-detection lives in checkSkills, which reads the lock for the located install's scope.
• updateAvailable now also trips on removed skills, since update can fix them — the check || update CI/agent contract stays coherent.
• skills update runs skills add --all, then prunes via skills remove <names> --yes. Best-effort: a cleanup failure doesn't fail the update (the install — what the contract gates on — already succeeded).
• renderCheck prints a "Removed upstream" section; --json includes the new fields.

No change to skills-manifest.json, the generator, or the published skill set.

Safety — we never touch a user's own (or another source's) skills

Two guarantees, both tested:

1. Source attribution. A skill is only ever a prune candidate if the lock records its source as heygen-com/hyperframes. A hand-placed/custom skill (no lock entry) or one from another source (e.g. greensock/gsap-skills, anthropics/skills) is never matched, so never flagged and never removed.
2. Scope match. The prune removes in the exact scope detection attributed from (checkSkills returns scope; -g is passed only when global). This prevents a project-scoped attribution from deleting a same-named global skill (or vice-versa) that could belong to the user or another source.

Test plan

• Unit tests added/updated — skillsManifest.test.ts (pure skillsAttributedToSource: slug/URL matching, other-source isolation, empty lock; end-to-end checkSkills: removed detection, other-source not flagged, removed-alone-trips-updateAvailable, no-lock → no removed) and skills.test.ts (update prunes in global scope with -g; prunes in project scope without -g; doesn't prune when nothing was removed).
• Full CLI suite green (bunx vitest run packages/cli — the 2 *.browser.test.ts errors are a local happy-dom resolution quirk under ad-hoc vitest, unrelated).
• oxfmt --check, oxlint, tsc, and fallow audit --base origin/main all clean.

🤖 Generated with Claude Code

[miga-heygen]

Review — feat(cli): surface and prune skills removed upstream

SHA reviewed: 51edaac

Good feature — closing the "skills linger forever after a rename/removal" gap is the right thing to do, and the attribution-via-lock approach is exactly right. The separation between the manifest diff (what diffSkills does) and the removed-detection (what detectRemoved does) is clean, the repoSlug normalization covers the realistic URL variants, and the best-effort semantics on the cleanup step respect the CI contract without being fragile. Tests cover the important paths well.

A few things I want to flag:

1. runSkillsRemove missing -g test assertion

The implementation passes -g (global scope) to skills remove:

return spawnNpx(["skills", "remove", ...names, "-g", "--yes"]);

But the "prunes skills removed upstream" test only asserts on --yes, never on -g:

expect(removeCall!.args).toContain("graphic-overlays");
expect(removeCall!.args).toContain("--yes");

If someone accidentally dropped -g, the prune would target the wrong scope and the test would still pass. Add expect(removeCall!.args).toContain("-g");.

2. Command injection surface via skill names in runSkillsRemove

runSkillsRemove spreads names directly into the spawn args:

return spawnNpx(["skills", "remove", ...names, "-g", "--yes"]);

The names come from checkSkills() → detectRemoved() → skillsAttributedToSource(), which reads them as JSON keys from the lock file on disk. The lock is local-only (not user-supplied over the network), so the practical risk is low. But if a lock file were ever corrupted or crafted (e.g. a name like --config=evil.js), the spawn would interpret it as a flag. Spawning via an array (not shell) already avoids classic shell injection, but -- before the names would be a cheap hardening:

return spawnNpx(["skills", "remove", "--", ...names, "-g", "--yes"]);

Low priority — just a defense-in-depth note.

3. SkillsDiff type exported but only used internally

The new SkillsDiff interface is exported but never consumed outside skillsManifest.ts — diffSkills returns it and checkSkills immediately destructures it. Consider keeping it unexported (interface SkillsDiff) unless there is an upcoming consumer. Minor — no blocking concern.

4. Edge case: what if a removed skill is also in the manifest?

detectRemoved filters !(name in latest.skills), which is correct. But checkSkills then concatenates [...diff.skills, ...removed] without deduplication. If a skill appeared in both the lock (attributed to our source) AND the manifest (somehow), detectRemoved would correctly filter it out, so this is fine in practice. Just confirming I traced the logic end to end — the filter is in the right place.

5. printSkillSection — nice DRY refactor

The extraction of the three repeated console-log blocks into printSkillSection is a clean simplification. The uniform coloring per line (mark + name) matches the existing style. Good.

---

Verdict: Looks good overall. The lock-based attribution is the right design for a shared skills/ directory. The one actionable item is adding the -g assertion in the test (#1). The rest are minor observations — not blockers. Approve after #1.

— Miga

[james-russo-rames-d-jusso]

Review — feat(cli): surface and prune skills removed upstream

SHA reviewed: 51edaac8c92ce17bae5da31b7b434e9ab5cc159f
Layering on: Miga's R1 (same SHA, 51edaac) — agreeing with her findings and adding a few more from a different angle.

This is a clean follow-up to #1738 — the lock-attribution approach is the right way to handle "ours vs another source's" in a shared .../skills directory, and the best-effort cleanup respects the check || update CI contract (install is the gated surface, prune is icing). SkillStatus / summary.removed is an additive extension to the existing JSON shape, so downstream CI consumers won't break. --json rendering, updateAvailable rollup, and renderCheck formatting all thread through correctly.

Per the iterative-merge rubric: nothing here is a hard-to-walk-back contract change. Additive --json field, additive enum value, and the prune step is gated on positive lock attribution (no false positives by construction). Comfortable with the merge-and-iterate stance.

Commenting (not approving) — leaving the stamp call to you per usual.

🟡 Concerns

1. --dir users silently skip removed-detection — skillsManifest.ts:222-225.
locateInstall hard-codes scope: "project" whenever --dir is given, regardless of whether the override path is actually under cwd or $HOME:

if (opts.dir) {
  return existsSync(opts.dir)
    ? { dir: opts.dir, agent: agentFromDir(opts.dir), scope: "project" }
    : null;
}

That scope feeds lockPathForScope (skillsManifest.ts:329-337), which then reads ./skills-lock.json — almost certainly absent for the typical --dir ~/.claude/skills invocation. Result: detectRemoved reads a missing file, falls through to readSkillLock's catch → null, and silently reports zero removed skills. A user pointing --dir at their actual home-global install would never see a removed row.

Two reasonable fixes:

• Infer scope from the path (compare against homedir() / known XDG paths), OR
• Try both project and global lock paths in detectRemoved and union the attributions.

Not a blocker because --dir is power-user-only and the existing check/update happy path (no --dir) is unaffected. Worth a follow-up.

2. update doesn't accept --source or --dir, but check does — skills.ts:181-225 vs :154-177.
checkCommand plumbs through --json, --dir, --source; updateCommand has args: {}. So:

hyperframes skills check --source fork/repo  # checks against the fork manifest
hyperframes skills update                     # installs from heygen-com/hyperframes

For the check || update CI contract this is fine — both default to the same source. But update's prune step calls checkSkills() (no args, skills.ts:212) to discover what to remove, so a user testing against a fork would see different removed-sets between check and update. Probably worth either:

• Adding --source / --dir to update for symmetry, OR
• Documenting that update is heygen-com-only in the meta.description.

3. <HOME>/.agents/.skill-lock.json is plausible but version-pinning is implicit — skillsManifest.ts:336.
The comment says "mirroring that CLI's paths", but if vercel-labs/skills ever moves the lock (e.g. to <XDG_CONFIG_HOME>/.agents/.skill-lock.json, or under .config/skills/), detectRemoved silently returns nothing again. The same failure mode as #1 above: best-effort cleanup that just stops working with no signal to the user. Worth pinning the assumed lock-path schema to a vercel-labs/skills version in the comment, so future drift is at least obvious during code search.

💭 Layering on Miga's findings

• Her #1 (test missing -g assertion) — strong agree. Easy fix, prevents silent scope regression.
• Her #2 (-- for flag-injection hardening) — agree this is defense-in-depth, not active risk. spawnNpx uses spawn with an args array (no shell interpolation), so classic injection is already out. The realistic risk is a corrupted/crafted lock with a name like --foo getting interpreted as a flag by upstream skills remove. Cheap to harden:

  return spawnNpx(["skills", "remove", "--", ...names, "-g", "--yes"]);

  Only worth it if upstream skills remove actually respects -- as the end-of-flags sentinel — worth a one-line check before adding.
• Her #3 (SkillsDiff exported but unused externally) — agree, minor.
• Her #4 (dedup trace) — also traced this end-to-end, also confirmed it's fine — detectRemoved filters !(name in latest.skills) so the concatenation in checkSkills (skillsManifest.ts:496) can't double-report.

✅ Strong points

• printSkillSection refactor — clean DRY, and the uniform coloring per line matches the existing style.
• repoSlug normalization handles git+, https://github.com/, .git, lowercase — covers the realistic URL/slug variants the lock would record.
• Best-effort prune semantics: install failure exits non-zero (gated CI contract surface), prune failure is a clack.log.warn (icing, not contract). Right rubric.
• Test fixtures use real filesystem writes via mkdtempSync — no f(x)===f(x) reflexivity, no shared-fallback duration-source. The "removed-alone trips updateAvailable" test in particular is a good one — confirms the rollup logic.
• --json output stably extended (additive summary.removed + additive "removed" enum value), so existing CI consumers won't break.

— Rames D Jusso

[jrusso1020]

Thanks @miga-heygen — both points addressed.

1. -g test assertion — resolved (the review was on 51edaac; the follow-up 0e13a759d landed just after). runSkillsRemove no longer hardcodes -g: checkSkills now returns the located install's scope, and update removes in that exact scope so attribution scope and removal scope always match. Both paths are now asserted:

• global → expect(removeCall!.args).toContain("-g")
• project → expect(removeCall!.args).not.toContain("-g")

This also closes a latent bug your point implied: hardcoded -g while skills add defaults to project scope could have removed a same-named global skill from a different source.

2. Crafted skill-name in the spawn — hardened in 314c6f6d, but via name validation rather than --. I checked the upstream skills arg parser: it silently ignores any unknown --prefixed token and has no -- end-of-options handling, so -- would be a no-op there. Instead, runSkillsRemove now filters names through a strict kebab-case pattern (/^[a-z0-9][a-z0-9._-]*$/i) and warns on anything rejected. That's parser-independent and, more importantly, also covers the Windows cmd.exe spawn path where arg escaping is fragile. New test asserts a --config=evil.js-style lock key never reaches the spawn while the valid name still does.

[james-russo-rames-d-jusso]

R2 verification — feat(cli): surface and prune skills removed upstream

SHA reviewed: 314c6f6db4b30b32c276ec209830ab35c6dc2b2e (R1 was 51edaac, +2 commits: 0e13a759 scope-aware prune, 314c6f6d skill-name validation)
Layering on: Miga's R1 + my R1, both at 51edaac.

Verified each R1 finding against HEAD. Both Miga's load-bearing items addressed — one straightforwardly, one resolved-differently in a better shape than the suggestion. My three 🟡s are acknowledged as documented power-user follow-ups (per James's "addressed feedback" framing in his issue comment); no blocker movement.

Commenting (not approving) — stamp call is yours.

Miga's R1 findings — verified at HEAD

Miga #1 — -g test assertion gap → ✅ resolved-and-extended
skills.test.ts:187 now asserts toContain("-g") on the global path; the rename "prunes skills removed upstream, in the attributed scope" reflects the new shape. Sibling test added at :194-208 asserts not.toContain("-g") on the project path. Per James's reply: the original hardcoded -g masked a latent cross-scope bug (project attribution + global remove could have hit a same-named global skill from another source). Both directions now scope-symmetric.

Miga #2 — -- flag-injection hardening → ✅ resolved-differently (stronger than suggested)
skills.ts:58-77: runSkillsRemove now filters names through PLAIN_SKILL_NAME = /^[a-z0-9][a-z0-9._-]*$/i, warns on rejects via clack.log.warn, and early-returns Promise.resolve() if zero valid names remain. James verified upstream skills arg parser ignores unknown --prefixed tokens and has no -- handling, so the -- suggestion would have been a no-op. The validation approach is better:

• Parser-independent — works regardless of upstream behavior.
• Covers the Windows cmd.exe spawn path where arg escaping is fragile (you flagged this rationale).
• Per-name granularity: a valid name in the same batch as a rejected one still gets removed (test at skills.test.ts:218-235 confirms --config=evil.js filtered while graphic-overlays passes through).

Regex check: starts with [a-z0-9] (locks out - / . / _ prefixes including all flag forms), then [a-z0-9._-]*. Realistic skill names (graphic-overlays, talking-head-recut, 2d-animation, mixed-case via /i) all match. Anything with =, space, ;, &, |, or shell metachars is rejected.

Miga #3 — SkillsDiff exported but only used internally → 📝 acknowledged-kept
skillsManifest.ts:73 still export interface SkillsDiff; still only used at line 254. Author judgment call — minor, not a blocker.

Miga #4 — Dedup trace → ✅ non-issue confirmed (no code change needed; logic is correct).

Miga #5 — printSkillSection praise → 📝 still present at skills.ts:114-126.

My R1 findings — verified at HEAD

My #1 — --dir users silently skip removed-detection (skillsManifest.ts:222-225) → ❌ open, acknowledged-power-user
locateInstall still hardcodes scope: "project" for any --dir (now at :220-228). Not addressed in this PR. Acceptable as a documented power-user follow-up — fix shape (path-aware scope inference, or both-scope union in detectRemoved) is still viable for a follow-up.

My #2 — update doesn't accept --source / --dir (skills.ts:181-225) → ❌ open, acknowledged-symmetry-gap
updateCommand.args still {} at :202. update's prune step still hits defaults via checkSkills() at :232. Documented divergence with check — fine for the heygen-com primary path.

My #3 — Lock-path version pinning is implicit (skillsManifest.ts:336) → ❌ open, acknowledged
lockPathForScope comment at :330 still says "mirroring that CLI's paths" with no vercel-labs version pin. Same failure-mode class as #1. Not addressed; same follow-up rubric.

NEW at HEAD

🟡 NEW #1 — All-rejected path uncovered by tests (skills.ts:71).
if (!safe.length) return Promise.resolve(); — the early return on all-names-rejected is the right behavior (don't spawn skills remove with zero positional args), but no test covers it. A regression that flipped the early-return condition or dropped it would silently start spawning skills remove -g --yes with nothing to remove, which skills remove would either error on or interpret unexpectedly. Cheap to add: a third test case where removed: ["--config=evil.js"] (sole entry, all-rejected) → assert state.spawnCalls.find(s => s.args.includes("remove")) === undefined. Test-only, not a blocker.

✅ NEW #2 — Latent cross-scope bug surfaced by Miga's #1.
Worth calling out explicitly: as your issue comment notes, the original -g hardcode + project-default skills add could have produced cross-scope mismatches that silently deleted same-named global skills from other sources. The R2 fix — checkSkills returning scope, update threading it through { global: scope === "global" } — closes this end-to-end. The new safety comment at skills.ts:225-230 ("Safety: removed only ever contains skills the lock records as installed from our source… We remove in the exact scope detection attributed from") locks the invariant in for the next maintainer.

Strong points (still)

• Scope plumbed end-to-end: SkillRoot.scope → SkillsCheckResult.scope (additive at skillsManifest.ts:84) → runSkillsRemove({global}) → spawn args. Single source of truth, easy to trace.
• Kebab-case validator is parser-independent and Windows-spawn-aware — stronger than the -- shape Miga and I both initially proposed.
• Test diff is high-density: one renamed/extended test, one new scope-symmetric test, one new validator test. All assertions are exact-args on spawnCalls, no f(x)===f(x) reflexivity.
• --json contract stays additive-only (scope field appended to SkillsCheckResult); existing CI consumers unaffected.

— Rames D Jusso

[miguel-heygen]

Reviewed current HEAD 314c6f6d.

Strengths:

• packages/cli/src/utils/skillsManifest.ts uses lock-source attribution for removed-upstream detection, so shared skills/ directories do not produce false removals.
• packages/cli/src/commands/skills.ts now prunes in the same scope returned by checkSkills(), avoiding cross-scope deletion.
• packages/cli/src/commands/skills.ts validates lock-derived names before spawning skills remove; packages/cli/src/commands/skills.test.ts pins global/project scope and invalid-name behavior.

Notes:

• Rames’s --dir scope, update --source/--dir symmetry, lock-path version comment, and all-invalid-name test are valid follow-ups, but not merge blockers for the default check || update path.
• Focused local CLI tests (49/49) and oxfmt passed. Required CI is green at review time; optional regression shards are still running.

Verdict: APPROVE
Reasoning: The current head resolves the load-bearing feedback and keeps removal constrained to lock-attributed skills in the attributed scope; remaining issues are power-user or test-polish follow-ups.

— Magi