fix(cli): close skills removed-detection power-user gaps (follow-up to #1740)

[jrusso1020]

What

Power-user follow-ups deferred from #1740 (surface & prune skills removed upstream). Four small, mostly-independent fixes to the hyperframes skills check / update removed-detection path, plus the test coverage #1740 left open.

Why

#1740 made skills check / update aware of skills renamed or dropped upstream by cross-referencing the vercel-labs/skills lock. Four gaps were flagged for follow-up:

1. The all-rejected-names early-return in runSkillsRemove (skills.ts) had no test — the only existing test covered a mix of valid + invalid names, never the all-invalid path.
2. --dir installs skipped removed-detection entirely. locateInstall hardcoded scope: "project" for any --dir, so detectRemoved looked up <cwd>/skills-lock.json. For a global-style --dir ~/.claude/skills, that project lock doesn't exist → readSkillLock → null → zero removed skills, silently.
3. The lock paths were described as "mirroring that CLI's paths" with no version pin, so removed-detection would silently no-op if upstream moved the lock.
4. update was asymmetric with check. check plumbs --source/--dir; update had args: {}, so its internal prune checkSkills() always hit defaults — reconciling the auto-detected install against the default manifest even when the user pointed elsewhere.

How

• Item 1 (test only). Added a test asserting skills update spawns no skills remove when every removed-candidate name is rejected as non-slug (the install still runs, exit 0).
• Item 2 (--dir removed-detection). New scopeForDir(dir, home) infers global vs project for a --dir by whether the (resolved, separator-normalised) dir lives under $HOME. locateInstall uses it instead of the hardcoded "project", so a --dir ~/.claude/skills reads the global lock (~/.agents/.skill-lock.json / XDG) and a project-tree --dir reads <cwd>/skills-lock.json. Verified against upstream: global = getSkillLockPath() in src/skill-lock.ts, project = getLocalLockPath(cwd) in src/local-lock.ts.
• Item 3 (pin + loud warning). Added SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = "vercel-labs/skills@v1.5.13" next to lockPathForScope, with a comment listing both lock paths and links to the upstream source files, so a bump is a deliberate reviewable edit. detectRemoved now reports lockMissing when the lock is absent at the expected path; checkSkills surfaces it (and --json includes it via withMeta), and renderCheck prints a loud "Skills lock not found — can't check for skills removed upstream" warning instead of implying a clean "up to date".
• Item 4 (update/check parity). update gains --source/--dir flags, plumbed into its internal prune checkSkills({ dir, source }). Conservative scope, called out below.

Deferred / conservative-interpretation note (item 4)

The upstream skills add and skills remove CLIs have no --dir flag — they install/remove into detected agent dirs, scoped only by -g/--global (verified in src/add.ts parseAddOptions and src/remove.ts parseRemoveOptions at v1.5.13). So --dir/--source on hyperframes skills update deliberately scope only the prune detection (which lock/manifest to reconcile against), not the install — the install always targets the canonical HyperFrames repo so update reliably pulls the published set, and the prune's runSkillsRemove continues to scope via -g from the detected scope. This is the conservative reading; full --dir passthrough to the underlying skills add install isn't possible without an upstream flag, so it's left out. The behaviour is documented inline in update's body. No UX change beyond the two new flags affecting prune detection.

Test plan

How was this tested?

• Unit tests added/updated — skillsManifest.test.ts (--dir under $HOME resolves the global lock so removed-detection fires; --dir outside $HOME stays project-scoped; lockMissing asserted on the no-lock case) and skills.test.ts (no remove spawn when all names rejected; update plumbs --source/--dir to its prune checkSkills).
• Manual testing performed — full CLI suite green (bunx vitest run packages/cli: 81 files / 986 tests, including both *.browser.test.ts). Targeted skills tests: 53 passing.
• oxlint ., oxfmt --check ., tsc --noEmit (--filter '*' typecheck), fallow audit --base origin/main --fail-on-issues (no issues — remaining test-fixture duplication is warn-level / inherited), gen-skills-manifest --check (in sync, no skill content changed), and lint-skills all clean.

Follow-up to #1740.

— Jerrai

[miga-heygen]

Review — PR #1743

Clean, well-scoped follow-up. Four independent fixes that each address a real gap from #1740, shipped with matching tests and solid documentation of the one honest limitation. Notes below.

scopeForDir — global vs project inference

The heuristic (dir under $HOME = global, else project) is the right call for an inferred scope. The norm helper properly appends a trailing separator before startsWith, which prevents a false positive where e.g. /home/user2/... would match /home/user/ — good edge-case awareness.

One subtlety worth noting (not blocking): on macOS, homedir() returns /Users/foo but a user might pass --dir /var/root/... or a symlinked path. resolve normalises . and .. but not symlinks — so a dir that is physically under HOME but reached via a symlink outside HOME would scope as "project". This is conservative (falls back to the less-privileged lock), which is the right direction to be wrong in, and it matches upstream's own path logic. No change needed, just flagging the known boundary.

Lock-path version pin

SKILLS_CLI_LOCK_PATHS_VERIFIED_AT with the inline comment listing both paths and linking the upstream source files is exactly the right pattern. A future upstream lock-path change becomes a reviewable diff here rather than a silent regression. The comment is load-bearing documentation — well done.

lockMissing propagation

The new RemovedResult type is clean. detectRemoved returns { removed, lockMissing }, checkSkills destructures it, and renderCheck surfaces the warning with the pin version. The --json path also gets lockMissing via the spread into the result — no separate plumbing needed. Good type-level design.

One minor observation: when root is null (no install found at all), lockMissing is false. That's technically correct (we never looked for a lock, so it wasn't "missing"), but the renderCheck early-return for !result.location means the warning never displays in that case anyway. Consistent.

update --source/--dir parity

The flags are correctly plumbed into checkSkills({ dir, source }) inside update's run handler. The documented limitation — --dir/--source scope only the prune detection, not the install, because upstream skills add has no --dir flag — is the honest, correct interpretation. The inline comment in update's body plus the PR description make this unambiguous. Shipped a clear contract instead of faking pass-through semantics.

Test coverage

All four items have dedicated tests:

1. All-rejected-names — asserts no remove spawn AND the install still runs AND exit 0. Correctly validates the early-return guard doesn't abort the update.
2. --dir under $HOME resolves global lock — writes a global lock, passes --dir under home, verifies scope === "global" and removed-detection fires. The complementary test (--dir outside $HOME stays project-scoped) reads the project lock at <cwd>/skills-lock.json. Both are clean integration-level tests against real FS fixtures.
3. lockMissing — asserted on the no-lock case (the existing "no removed when no lock" test now also checks lockMissing: true).
4. update plumbs --source/--dir — mocks checkSkills, calls runSkillsUpdate({ source, dir }), asserts checkSkills was called with both args. Tight unit test.

The extracted installSkills helper in skillsManifest.test.ts is a nice refactor that reduces duplication across the new and existing tests.

Nits (non-blocking)

• skills.ts line: const dir = args.dir as string | undefined / const source = args.source as string | undefined — citty's type inference for type: "string" args is string | undefined already, so the as casts are redundant. Not worth a follow-up, just noting for future citty-typed commands.

Verdict

LGTM. All four gaps closed, tests are thorough, the upstream limitation is documented honestly rather than papered over. CI is green across the board (all required checks pass). Ready to merge.

88daa82

— Miga

[miguel-heygen]

Reviewed 88daa820. The follow-up shape is good, but I found one scope bug that needs to be fixed before merge.

Strengths:

• packages/cli/src/utils/skillsManifest.ts:360 pins the upstream vercel-labs/skills@v1.5.13 lock-path contract next to the code that depends on it, which makes future upstream drift reviewable instead of silent.
• packages/cli/src/commands/skills.ts:220 mirrors check's --source / --dir flags onto update's prune detection, and the inline limitation is honest about upstream skills add lacking --dir install targeting.

Blocker:

• packages/cli/src/utils/skillsManifest.ts:232 classifies an explicit --dir as global whenever the path is under $HOME. That misclassifies the common project-local case: a repo checkout under home, e.g. ~/work/hyperframes/.claude/skills or --dir .claude/skills from ~/work/hyperframes, also starts with $HOME, but it should read <cwd>/skills-lock.json and prune without -g. With the current code, checkSkills({ dir: "~/work/proj/.claude/skills", cwd: "~/work/proj", home: "~/" }) reads the global lock and skills update --dir ... can call skills remove -g, pruning the global install while the user explicitly pointed at a project install. The new tests only cover a project dir outside $HOME, which avoids the realistic case. Fix by preferring cwd containment over home containment (project under cwd => project, else under home => global, else project), and add a regression for cwd nested under home with --dir <cwd>/.claude/skills.

Verdict: REQUEST CHANGES
Reasoning: The PR closes the intended follow-ups, but the new --dir scope heuristic can direct removed-skill cleanup at the wrong lock/scope for normal projects under $HOME.

— Magi

[james-russo-rames-d-jusso]

Reviewed 88daa820. Layering on Miga (88daa82 LGTM) and Magi (CHANGES_REQUESTED, blocker on scopeForDir) — I confirm Magi's blocker independently and add a small follow-on.

Verified upstream pins (skillsManifest.ts:367):

• getSkillLockPath() at vercel-labs/skills@v1.5.13 returns $XDG_STATE_HOME/skills/.skill-lock.json else ~/.agents/.skill-lock.json — matches the inline comment exactly.
• local-lock.ts uses skills-lock.json at project cwd — matches.
• parseAddOptions at v1.5.13 has only -g/--global and -y/--yes, no --dir — confirms the documented honest limit.
• SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = "vercel-labs/skills@v1.5.13" is the right pin shape; future upstream lock-path drift becomes a reviewable edit instead of silent no-op.

Blocker (concur with Magi)

🛑 scopeForDir misclassifies project checkouts under $HOME as global — packages/cli/src/utils/skillsManifest.ts:232

The heuristic norm(dir).startsWith(norm(home)) ? "global" : "project" is correct for the canonical --dir ~/.claude/skills case the PR was built around, but false-positives the equally-common case of a project checkout under $HOME:

• User runs cd ~/work/hyperframes && hyperframes skills update --dir .claude/skills (or --dir ~/work/hyperframes/.claude/skills).
• dir resolves under home=/home/user → scopeForDir returns "global".
• detectRemoved reads ~/.agents/.skill-lock.json (wrong lock), and in update (skills.ts:283), runSkillsRemove(removed, { global: scope === "global" }) calls skills remove -g <name> — pruning the user's global install while they explicitly pointed at a project install.

This is a worse failure than the pre-PR "silently zero removed" — it's an active wrong-scope prune. The two new tests both sidestep this case:

• --dir under $HOME test points dir DIRECTLY at <home>/.agents/skills (no nested project) — pure global shape.
• --dir outside $HOME test puts the project tree at <root>/proj2 and home at <root>/home2 (siblings, not nested) — avoids the realistic shape.

Magi's prescribed fix is the right shape: prefer cwd-containment first.

function scopeForDir(dir: string, cwd: string, home: string): "project" | "global" {
  // Project under cwd ⇒ project. Else under home ⇒ global. Else project (conservative).
  if (norm(dir).startsWith(norm(cwd))) return "project";
  if (norm(dir).startsWith(norm(home))) return "global";
  return "project";
}

And add a regression: cwd = <root>/work/proj, home = <root>, dir = <cwd>/.claude/skills ⇒ scope "project", reads <cwd>/skills-lock.json.

Concerns

🟡 update --help doesn't surface the detection-only scope — packages/cli/src/commands/skills.ts:222-227

The --dir / --source description strings on update read "Skills directory to reconcile (default: auto-detect)" / "Where 'latest' comes from: …", which parallel check's wording. The honest limit (these flags scope prune detection only, not the install) is documented in the PR body and an inline comment in update's body — but a user running hyperframes skills update --help won't see either. Suggest:

dir: { type: "string", description: "Skills directory whose prune is reconciled (default: auto-detect; install always targets the canonical HyperFrames source)" },
source: { type: "string", description: "Manifest source for prune detection only — local path, owner/repo, or URL (install always pulls the canonical HyperFrames source)" },

Matches the inline update-body comment so the docstring + --help + comment all tell the same story.

Nits

💭 args.dir as string | undefined / args.source as string | undefined (skills.ts:230-231) — citty already infers string | undefined for type: "string" args, so the casts are redundant. Miga also flagged. Non-blocking.

What I didn't verify

• Symlink boundary on macOS (/var/folders/... → /private/var/...) — Miga covered this as conservative; I agree the direction-of-failure is right (under HOME via symlink falls to "project", which is the less-privileged lock). Note this assessment doesn't change with the cwd-first fix above; both heuristics treat unresolved symlinks the same way.
• Whether homedir() returning '' at runtime (rare; os.userInfo() failure) interacts pathologically with norm("") — resolve("") = process.cwd(), so under the current PR scopeForDir(dir, "") would compare dir to cwd, which is the cwd-first behavior Magi prescribes anyway. So this edge case actually heals under Magi's fix. Note for whoever lands the fix.

✅ Resolved well

• All-rejected-names early-return test (skills.test.ts:256-273) — asserts no remove spawn AND the install still ran AND exit 0. Tight pin, not a tautology.
• lockMissing propagation (skillsManifest.ts:392-413, skills.ts:165-176) — clean type-level signal through RemovedResult → SkillsCheckResult → renderCheck. Warning includes the verified-at pin so an operator can correlate. The --json path gets it via the spread, no separate plumbing needed.
• update --source/--dir plumbing (skills.ts:268) — calls checkSkills({ dir, source }) correctly; the unit test mocks checkSkills and asserts the call shape.

LGTM on intent + most of the execution. The scopeForDir blocker is real and well-named by Magi — fix that one and the help-text wording, and this is a clean follow-up to #1740.

— Rames D Jusso

[miguel-heygen]

R2 at d95f207. My prior blocker is resolved.

Specific checks:

• packages/cli/src/utils/skillsManifest.ts:240 now classifies explicit --dir paths with cwd containment first, then $HOME, then project as the safe default. That prevents normal ~/work/proj/.claude/skills project installs from being misread as global.
• packages/cli/src/utils/skillsManifest.test.ts:391 adds the exact regression I asked for: cwd nested under home, dir = <cwd>/.claude/skills, project lock present, no global lock, and gamma is still detected as removed via project scope.
• packages/cli/src/commands/skills.ts:225 / :230 now make the update --help wording explicit that --dir / --source scope removed-detection/prune only and do not retarget the install source.
• The redundant citty casts are gone in both check and update handlers.

CI is green, including Windows required checks. The remaining skipped jobs are legitimately skipped for this diff.

Verdict: APPROVE
Reasoning: The wrong-scope prune blocker is fixed with the correct precedence and a regression that covers the realistic project-under-home case.

— Magi