From 88daa820790930947d631b38d857d5fd7c3b10f3 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 26 Jun 2026 16:41:12 +0000 Subject: [PATCH 1/2] fix(cli): close skills removed-detection power-user gaps (follow-up to #1740) Address power-user follow-ups deferred from #1740 (skills removed-detection): - `--dir` installs now run removed-detection. `locateInstall` hardcoded scope "project" for every `--dir`, so a `--dir ~/.claude/skills` (a global install) read a non-existent `/skills-lock.json` and found zero removed skills. New `scopeForDir` infers global vs project from whether the dir is under $HOME, so the right lock is read. - Pin the upstream lock paths to vercel-labs/skills@v1.5.13 (verified against src/skill-lock.ts + src/local-lock.ts) and warn loudly when the lock is absent where expected, so removed-detection no longer silently no-ops if upstream moves the lock. checkSkills returns lockMissing; --json surfaces it. - skills update gains --source/--dir (parity with check), plumbed into its internal prune checkSkills() so the prune respects the same overrides. - Add the missing test for the all-rejected-names early-return in runSkillsRemove (no skills remove spawned when every candidate name is rejected), plus tests for the --dir scope inference and update flag parity. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/commands/skills.test.ts | 42 ++++++++++- packages/cli/src/commands/skills.ts | 45 ++++++++++-- packages/cli/src/utils/skillsManifest.test.ts | 67 ++++++++++++++++-- packages/cli/src/utils/skillsManifest.ts | 69 ++++++++++++++++--- 4 files changed, 204 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index 8c25cd1ba1..389230e9cf 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -64,11 +64,11 @@ function setPlatform(platform: NodeJS.Platform): void { } /** Invoke the `skills update` subcommand from a freshly-imported module. */ -async function runSkillsUpdate(): Promise { +async function runSkillsUpdate(args: Record = {}): Promise { const { default: skillsCmd } = await import("./skills.js"); const subs = skillsCmd.subCommands as unknown as Record; expect(subs.update).toBeDefined(); - await subs.update!.run?.({ args: {}, rawArgs: [], cmd: subs.update } as never); + await subs.update!.run?.({ args, rawArgs: [], cmd: subs.update } as never); } describe("hyperframes skills", () => { @@ -213,6 +213,19 @@ describe("hyperframes skills", () => { expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); }); + // `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. + it("skills update plumbs --source/--dir to its prune detection (parity with check)", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockResolvedValueOnce({ scope: "project", skills: [] } as never); + + await runSkillsUpdate({ source: "owner/repo", dir: "/custom/skills" }); + + expect(checkSkills).toHaveBeenCalledWith({ source: "owner/repo", dir: "/custom/skills" }); + }); + // Skill names come from lock-file JSON keys; a flag-like / shell-special name // must never reach the spawn (esp. the Windows cmd.exe path). it("skills update never passes a non-slug skill name to remove", async () => { @@ -233,4 +246,29 @@ describe("hyperframes skills", () => { expect(removeCall!.args).toContain("graphic-overlays"); expect(removeCall!.args).not.toContain("--config=evil.js"); }); + + // The early-return guard in runSkillsRemove: when EVERY candidate name is + // rejected as non-slug, no `skills remove` is spawned at all (the prior test + // only covers a mix of valid + invalid). A spawn here would run `skills remove + // --yes` with no names — which the upstream CLI treats as "remove nothing" at + // best, or prompts interactively at worst — so we must not reach it. + it("skills update spawns no remove when every removed name is rejected", async () => { + setPlatform("linux"); + const { checkSkills } = await import("../utils/skillsManifest.js"); + vi.mocked(checkSkills).mockResolvedValueOnce({ + scope: "global", + skills: [ + { name: "--config=evil.js", status: "removed" }, + { name: "../escape", status: "removed" }, + ], + } as never); + + await runSkillsUpdate(); + + expect(state.spawnCalls.some((s) => s.args.includes("remove"))).toBe(false); + // The install still ran and the update still succeeded — a cleanup no-op + // doesn't fail the update. + expect(state.spawnCalls[0]?.args).toContain("add"); + expect(process.exitCode).toBe(0); + }); }); diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index 5949cd3eb2..146dcafe34 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -4,7 +4,12 @@ import * as clack from "@clack/prompts"; import { c } from "../ui/colors.js"; import { buildNpxCommand } from "../utils/npxCommand.js"; import { withMeta } from "../utils/updateCheck.js"; -import { checkSkills, type SkillDiff, type SkillsCheckResult } from "../utils/skillsManifest.js"; +import { + checkSkills, + SKILLS_CLI_LOCK_PATHS_VERIFIED_AT, + type SkillDiff, + type SkillsCheckResult, +} from "../utils/skillsManifest.js"; import type { Example } from "./_examples.js"; export const examples: Example[] = [ @@ -157,6 +162,19 @@ function renderCheck(result: SkillsCheckResult): void { c.warn, ); + // Removed-detection cross-references the upstream skills lock. If that lock is + // absent where we expect it (e.g. upstream moved its path), removed-detection + // silently reports zero — so warn rather than imply a clean "up to date". + if (result.lockMissing) { + console.log(); + console.log( + ` ${c.warn(`! Skills lock not found — can't check for skills removed upstream.`)}`, + ); + console.log( + ` ${c.dim(` (lock paths verified against ${SKILLS_CLI_LOCK_PATHS_VERIFIED_AT})`)}`, + ); + } + console.log(); if (result.updateAvailable) { console.log(` ${c.accent("Update: npx hyperframes skills update")}`); @@ -199,12 +217,31 @@ const updateCommand = defineCommand({ description: "Update all HyperFrames skills to the latest — installs any not yet present, removes any no longer published", }, - args: {}, - async run() { + // Mirror `check`'s flags: the prune step runs the same removed-detection, so it + // must respect the same overrides. Without these, `update`'s internal + // checkSkills() fell back to defaults — pruning the auto-detected install + // against the default manifest even when the user pointed `check` elsewhere. + args: { + dir: { type: "string", description: "Skills directory to reconcile (default: auto-detect)" }, + source: { + type: "string", + description: "Where 'latest' comes from: local path, owner/repo, or URL", + }, + }, + async run({ args }) { + const dir = args.dir as string | undefined; + const source = args.source as string | undefined; + // `skills add --all` re-fetches every skill to the latest AND installs ones // not yet present — so "update" pulls the full set, not just what is already // installed. This is where `init` and the stale-skills nudge both lead. // + // Note: the upstream `skills add` CLI has no `--dir` flag (it installs into + // detected agent dirs), so `--dir` here scopes only the *prune* detection + // below, not the install. `--source` likewise drives where the prune's + // "latest" manifest comes from; the install always targets the canonical + // HyperFrames repo so `update` reliably pulls the published set. + // // strict: this is the documented recovery path for the agent/CI contract // `hyperframes skills check || hyperframes skills update`. If the install // fails (no npx, `skills add` exits non-zero) it must exit non-zero too — @@ -229,7 +266,7 @@ const updateCommand = defineCommand({ // failure doesn't fail the update — the install the CI contract gates on // already succeeded. try { - const { skills, scope } = await checkSkills(); + const { skills, scope } = await checkSkills({ dir, source }); const removed = skills.filter((s) => s.status === "removed").map((s) => s.name); if (removed.length) { console.log(); diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index 977f269c70..9050b62ffa 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -294,6 +294,14 @@ describe("checkSkills removed-upstream detection", () => { else process.env.XDG_STATE_HOME = xdg; }); + // Install one or more `//SKILL.md` bundles. + function installSkills(skillsDir: string, names: string[]): void { + for (const name of names) { + mkdirSync(join(skillsDir, name), { recursive: true }); + writeFileSync(join(skillsDir, name, "SKILL.md"), `# ${name}`); + } + } + // Shared fixture: a project + home with two skills installed globally // (alpha + gamma), plus a manifest path. Tests then write the lock they need. function setup(manifest: SkillsManifest): { @@ -303,11 +311,7 @@ describe("checkSkills removed-upstream detection", () => { const project = join(root, "project"); const home = join(root, "home"); mkdirSync(project, { recursive: true }); - const skillsDir = join(home, ".agents/skills"); - for (const name of ["alpha", "gamma"]) { - mkdirSync(join(skillsDir, name), { recursive: true }); - writeFileSync(join(skillsDir, name, "SKILL.md"), `# ${name}`); - } + installSkills(join(home, ".agents/skills"), ["alpha", "gamma"]); const manifestPath = join(root, "manifest.json"); writeFileSync(manifestPath, JSON.stringify(manifest)); return { home, opts: { source: manifestPath, cwd: project, home } }; @@ -352,5 +356,58 @@ describe("checkSkills removed-upstream detection", () => { const res = await checkSkills(opts); expect(res.summary.removed).toBe(0); expect(res.skills.some((s) => s.status === "removed")).toBe(false); + // No install was located via auto-detect (skills live under /.agents/skills + // which IS discovered) — so lockMissing reflects the genuinely-absent lock. + expect(res.lockMissing).toBe(true); + }); + + // A `--dir` pointing at a global-scoped install (under $HOME) must resolve the + // GLOBAL lock (/.agents/.skill-lock.json), not the project lock + // (/skills-lock.json). Before this fix, locateInstall hardcoded scope + // "project" for every --dir, so the global lock was never read and + // removed-detection silently found nothing for --dir installs. + it("--dir under $HOME resolves the global lock so removed-detection works", async () => { + const { home, opts } = setup({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }); + writeGlobalLock(home, { + alpha: { source: "test" }, // ours and still in the manifest + gamma: { source: "test" }, // ours, dropped from manifest → removed + }); + + const dir = join(home, ".agents/skills"); + const res = await checkSkills({ source: opts.source, dir, cwd: opts.cwd, home }); + expect(res.scope).toBe("global"); + const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); + expect(byName.gamma).toBe("removed"); + expect(res.summary.removed).toBe(1); + expect(res.lockMissing).toBe(false); + }); + + // The complementary case: a `--dir` under the project tree (not $HOME) stays + // project-scoped and reads /skills-lock.json. + it("--dir outside $HOME stays project-scoped and reads the project lock", async () => { + const project = join(root, "proj2"); + const skillsDir = join(project, ".claude/skills"); + installSkills(skillsDir, ["alpha", "gamma"]); + const manifestPath = join(root, "m2.json"); + writeFileSync( + manifestPath, + JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }), + ); + // Project lock lives at /skills-lock.json — point cwd at the project. + writeFileSync( + join(project, "skills-lock.json"), + JSON.stringify({ + version: 1, + skills: { alpha: { source: "test" }, gamma: { source: "test" } }, + }), + ); + const home = join(root, "home2"); + mkdirSync(home, { recursive: true }); + + const res = await checkSkills({ source: manifestPath, dir: skillsDir, cwd: project, home }); + expect(res.scope).toBe("project"); + const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); + expect(byName.gamma).toBe("removed"); + expect(res.summary.removed).toBe(1); }); }); diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index bf46c6e80b..8f2c2b1989 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -19,7 +19,7 @@ import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; import { homedir } from "node:os"; -import { isAbsolute, join, relative, sep } from "node:path"; +import { isAbsolute, join, relative, resolve, sep } from "node:path"; import { promisify } from "node:util"; const execFileAsync = promisify(execFile); @@ -86,6 +86,13 @@ export interface SkillsCheckResult { updateAvailable: boolean; summary: { current: number; outdated: number; missing: number; removed: number }; skills: SkillDiff[]; + /** + * True when an install was located but the upstream skills lock was absent at + * the expected path, so removed-detection couldn't run (it silently reports + * zero removed). Lets the CLI warn instead of misreporting "up to date" — a + * guard against the lock path silently no-op'ing if upstream moves it. + */ + lockMissing: boolean; } const DEFAULT_REPO_SLUG = "heygen-com/hyperframes"; @@ -212,10 +219,29 @@ function discoverSkillRoots(base: string, scope: "project" | "global"): SkillRoo }); } +/** + * Decide whether an explicit `--dir` is a project- or global-scoped install, so + * removed-detection reads the *right* lock. The upstream `skills` CLI keeps two + * locks: a project lock at `/skills-lock.json` and a global lock under + * `$HOME` (see lockPathForScope). A `--dir` that lives under `$HOME` (e.g. + * `~/.claude/skills`) is a global install; one under the project tree (or + * anywhere else) is treated as project. Without this, `--dir ~/.claude/skills` + * was always scoped "project", so detectRemoved looked for a non-existent + * `/skills-lock.json` and silently found zero removed skills. + */ +function scopeForDir(dir: string, home: string): "project" | "global" { + const norm = (p: string): string => { + const r = resolve(p); + return r.endsWith(sep) ? r : r + sep; + }; + return norm(dir).startsWith(norm(home)) ? "global" : "project"; +} + /** * Find the first skill root that actually contains HyperFrames skills. A - * `--dir` override (if given) is treated as a `.../skills` directory directly. - * Otherwise scan project (cwd) then global ($HOME), auto-discovering hosts. + * `--dir` override (if given) is treated as a `.../skills` directory directly; + * its scope is inferred (see scopeForDir) so removed-detection reads the right + * lock. Otherwise scan project (cwd) then global ($HOME), auto-discovering hosts. */ function locateInstall( skillNames: string[], @@ -223,7 +249,11 @@ function locateInstall( ): SkillRoot | null { if (opts.dir) { return existsSync(opts.dir) - ? { dir: opts.dir, agent: agentFromDir(opts.dir), scope: "project" } + ? { + dir: opts.dir, + agent: agentFromDir(opts.dir), + scope: scopeForDir(opts.dir, opts.home ?? homedir()), + } : null; } const roots = [ @@ -327,7 +357,19 @@ export function skillsAttributedToSource(lock: SkillLock | null, source: string) .map(([name]) => name); } -/** Locate the vercel-labs/skills lock for a scope, mirroring that CLI's paths. */ +// Removed-detection reads the vercel-labs/skills lock, whose on-disk paths live +// in *their* repo, not ours — so if upstream moves the lock, our cross-reference +// silently finds nothing and `detectRemoved` no-ops without a peep. Pin the +// upstream version these paths were verified against so a future bump is a +// deliberate, reviewable edit (re-check src/skill-lock.ts `getSkillLockPath` +// and src/local-lock.ts `getLocalLockPath` when bumping): +// - global: $XDG_STATE_HOME/skills/.skill-lock.json else ~/.agents/.skill-lock.json +// - project: /skills-lock.json +// https://github.com/vercel-labs/skills/blob/v1.5.13/src/skill-lock.ts (global) +// https://github.com/vercel-labs/skills/blob/v1.5.13/src/local-lock.ts (project) +export const SKILLS_CLI_LOCK_PATHS_VERIFIED_AT = "vercel-labs/skills@v1.5.13"; + +/** Locate the vercel-labs/skills lock for a scope (paths pinned to the version above). */ function lockPathForScope( scope: "project" | "global", opts: { cwd?: string; home?: string }, @@ -347,17 +389,24 @@ function readSkillLock(path: string): SkillLock | null { } } +interface RemovedResult { + removed: SkillDiff[]; + /** The lock was absent at the expected path — removed-detection silently no-ops. */ + lockMissing: boolean; +} + /** Skills the lock attributes to our source that the manifest no longer ships. */ function detectRemoved( root: SkillRoot, latest: SkillsManifest, opts: { cwd?: string; home?: string }, -): SkillDiff[] { +): RemovedResult { const lock = readSkillLock(lockPathForScope(root.scope, opts)); - return skillsAttributedToSource(lock, latest.source) + const removed = skillsAttributedToSource(lock, latest.source) .filter((name) => !(name in latest.skills)) .sort() .map((name) => ({ name, status: "removed" as const })); + return { removed, lockMissing: lock === null }; } // ── Resolving the "latest" manifest ────────────────────────────────────────── @@ -487,7 +536,10 @@ export async function checkSkills( const root = locateInstall(skillNames, { dir: opts.dir, cwd: opts.cwd, home: opts.home }); const installed = root ? hashInstalled(root, skillNames) : {}; const diff = diffSkills(installed, latest); - const removed = root ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home }) : []; + const removedResult = root + ? detectRemoved(root, latest, { cwd: opts.cwd, home: opts.home }) + : { removed: [], lockMissing: false }; + const { removed, lockMissing } = removedResult; return { location: root?.dir ?? null, agent: root?.agent ?? null, @@ -497,5 +549,6 @@ export async function checkSkills( updateAvailable: diff.updateAvailable || removed.length > 0, summary: { ...diff.summary, removed: removed.length }, skills: [...diff.skills, ...removed], + lockMissing, }; } From d95f20702c5791647ce31c3b2806c89fb160592b Mon Sep 17 00:00:00 2001 From: James Date: Fri, 26 Jun 2026 17:22:56 +0000 Subject: [PATCH 2/2] fix(cli): scope --dir by CWD-containment before HOME (project installs under $HOME) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Magi's REQUEST_CHANGES on #1743 (88daa820). The new scopeForDir heuristic treated every explicit --dir under $HOME as GLOBAL, but the common project-local case is also under $HOME (e.g. ~/work/proj/.claude/skills, or --dir .claude/skills run from ~/work/proj). So checkSkills could read the GLOBAL lock and `skills update --dir ...` could prune with `skills remove -g` even when the user pointed at a PROJECT install — a wrong-scope prune. Change precedence to CWD-containment FIRST, then HOME: - dir resolves under cwd -> project (even when cwd is itself under $HOME) - else dir under home -> global - else -> project (safe default, never prune globally) Keeps the existing resolve/normalize + trailing-separator guard (so /home/user2 does not false-match /home/user). scopeForDir now takes cwd; locateInstall threads opts.cwd through. Add a regression test for Magi's exact failing case: cwd nested under home (cwd = /work/proj) with --dir /.claude/skills resolves to PROJECT. Existing tests stay green (global --dir ~/.claude/skills from an unrelated cwd still resolves to GLOBAL). Also (Miga's nit): drop the redundant `as string | undefined` casts on the citty args in skills.ts (citty already infers that type), and clarify the `skills update` --dir/--source help text to note they scope removed-detection only, not the install location. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/commands/skills.ts | 17 +++++---- packages/cli/src/utils/skillsManifest.test.ts | 35 +++++++++++++++++++ packages/cli/src/utils/skillsManifest.ts | 27 +++++++++----- 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index 146dcafe34..051cd2b623 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -196,8 +196,8 @@ const checkCommand = defineCommand({ }, async run({ args }) { const result = await checkSkills({ - dir: args.dir as string | undefined, - source: args.source as string | undefined, + dir: args.dir, + source: args.source, }); if (args.json) console.log(JSON.stringify(withMeta(result), null, 2)); @@ -222,15 +222,20 @@ const updateCommand = defineCommand({ // checkSkills() fell back to defaults — pruning the auto-detected install // against the default manifest even when the user pointed `check` elsewhere. args: { - dir: { type: "string", description: "Skills directory to reconcile (default: auto-detect)" }, + dir: { + type: "string", + description: + "Skills dir for removed-detection only — scopes the prune, not the install (default: auto-detect)", + }, source: { type: "string", - description: "Where 'latest' comes from: local path, owner/repo, or URL", + description: + "Where 'latest' comes from for removed-detection (local path, owner/repo, or URL) — does not change the install source", }, }, async run({ args }) { - const dir = args.dir as string | undefined; - const source = args.source as string | undefined; + const dir = args.dir; + const source = args.source; // `skills add --all` re-fetches every skill to the latest AND installs ones // not yet present — so "update" pulls the full set, not just what is already diff --git a/packages/cli/src/utils/skillsManifest.test.ts b/packages/cli/src/utils/skillsManifest.test.ts index 9050b62ffa..ecf7cbc6b0 100644 --- a/packages/cli/src/utils/skillsManifest.test.ts +++ b/packages/cli/src/utils/skillsManifest.test.ts @@ -382,6 +382,41 @@ describe("checkSkills removed-upstream detection", () => { expect(res.lockMissing).toBe(false); }); + // Regression (Magi REQUEST_CHANGES at 88daa820): the common project-local case + // is *also* under $HOME — a project checkout at `/work/proj` with skills + // at `/.claude/skills`. A HOME-first heuristic misclassified this as + // global, so checkSkills read the global lock and `skills update` would prune + // with `-g` against a project install. CWD-containment must win: `--dir` under + // `cwd` resolves to PROJECT even when `cwd` itself is nested under $HOME. + it("--dir under a cwd that is itself nested under $HOME stays project-scoped", async () => { + const home = join(root, "home"); + const project = join(home, "work", "proj"); // cwd nested INSIDE home + const skillsDir = join(project, ".claude/skills"); + installSkills(skillsDir, ["alpha", "gamma"]); + const manifestPath = join(root, "m3.json"); + writeFileSync( + manifestPath, + JSON.stringify({ source: "test", skills: { alpha: { hash: "x", files: 1 } } }), + ); + // Project lock at /skills-lock.json — only read if scope is "project". + // No global lock exists under $HOME, so a wrong-scope ("global") read would + // find no lock → zero removed (gamma would NOT be flagged). The project lock + // being read (gamma → removed) is the proof CWD-containment won. + writeFileSync( + join(project, "skills-lock.json"), + JSON.stringify({ + version: 1, + skills: { alpha: { source: "test" }, gamma: { source: "test" } }, + }), + ); + + const res = await checkSkills({ source: manifestPath, dir: skillsDir, cwd: project, home }); + expect(res.scope).toBe("project"); + const byName = Object.fromEntries(res.skills.map((s) => [s.name, s.status])); + expect(byName.gamma).toBe("removed"); + expect(res.summary.removed).toBe(1); + }); + // The complementary case: a `--dir` under the project tree (not $HOME) stays // project-scoped and reads /skills-lock.json. it("--dir outside $HOME stays project-scoped and reads the project lock", async () => { diff --git a/packages/cli/src/utils/skillsManifest.ts b/packages/cli/src/utils/skillsManifest.ts index 8f2c2b1989..b2d2c75903 100644 --- a/packages/cli/src/utils/skillsManifest.ts +++ b/packages/cli/src/utils/skillsManifest.ts @@ -223,18 +223,29 @@ function discoverSkillRoots(base: string, scope: "project" | "global"): SkillRoo * Decide whether an explicit `--dir` is a project- or global-scoped install, so * removed-detection reads the *right* lock. The upstream `skills` CLI keeps two * locks: a project lock at `/skills-lock.json` and a global lock under - * `$HOME` (see lockPathForScope). A `--dir` that lives under `$HOME` (e.g. - * `~/.claude/skills`) is a global install; one under the project tree (or - * anywhere else) is treated as project. Without this, `--dir ~/.claude/skills` - * was always scoped "project", so detectRemoved looked for a non-existent - * `/skills-lock.json` and silently found zero removed skills. + * `$HOME` (see lockPathForScope). + * + * Precedence is CWD-containment FIRST, then HOME — because the common + * project-local case is *also* under `$HOME` (e.g. `~/work/proj/.claude/skills`, + * or `--dir .claude/skills` run from `~/work/proj`). Checking HOME first would + * misclassify every such project install as global, reading the wrong lock and + * (worse) letting `skills update` prune with `-g`. So: + * - `dir` under `cwd` → project (even when that's also under $HOME) + * - else `dir` under $HOME → global (a real `~/.claude/skills`-style install) + * - else → project (safe default — never prune globally for an unknown path) + * + * Each base is normalised with a trailing separator before the prefix test so a + * sibling like `/home/user2` doesn't false-match `/home/user`. */ -function scopeForDir(dir: string, home: string): "project" | "global" { +function scopeForDir(dir: string, home: string, cwd: string): "project" | "global" { const norm = (p: string): string => { const r = resolve(p); return r.endsWith(sep) ? r : r + sep; }; - return norm(dir).startsWith(norm(home)) ? "global" : "project"; + const d = norm(dir); + if (d.startsWith(norm(cwd))) return "project"; + if (d.startsWith(norm(home))) return "global"; + return "project"; } /** @@ -252,7 +263,7 @@ function locateInstall( ? { dir: opts.dir, agent: agentFromDir(opts.dir), - scope: scopeForDir(opts.dir, opts.home ?? homedir()), + scope: scopeForDir(opts.dir, opts.home ?? homedir(), opts.cwd ?? process.cwd()), } : null; }