From 7d28f29114fa4c46bc42bf52d53abbffc54b3715 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Sat, 27 Jun 2026 03:31:26 +0800 Subject: [PATCH] fix(cli): scope skill installs to relevant agents, not all ~70 via --all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hyperframes skills`, `skills update`, and `init` all shelled out to `skills add ... --all`, which the upstream CLI expands to `--skill '*' --agent '*' -y` — every skill into every one of the ~70 agent conventions it knows about. A user who only runs Claude Code got skill folders for Cursor, Codex, and 50+ tools they never touch. Replace `--all` with a resolved target set (new resolveAgentTargets): 1. If the project already has agent skill folders (`.claude/skills`, `.hermes/skills`, …), install ONLY to those — an existing folder is the strongest signal of intent, so honour it exactly. 2. Otherwise (blank project): a. Running under Claude Code (CLAUDECODE) → just claude-code. b. Else probe PATH for installed agent CLIs (claude, hermes, droid, cursor, codex, opencode, gemini) — the gstack approach. c. Else fall back to claude-code + the shared `.agents` universal dir, which Cursor, Codex, OpenCode, Gemini, Copilot and ~14 others read from in project scope. Never `--agent '*'`. Installs stay `--skill '*'` (all skills) + `--copy` (faithful, detectable by `skills check`); only the agent fan-out is scoped. The dir<->key map is explicit because dir names differ from upstream keys (`.factory`/droid, `.hermes`/hermes-agent) and many agents share `.agents` (-> the single `universal` key). Keys verified against vercel-labs/skills@v1.5.13. Verified end-to-end in an isolated project + HOME: the new args land exactly `.claude/skills` (19) + `.agents/skills` (19) and nothing else — 2 folders, not 50+. Pure resolver covered by skillsTargets.test.ts; command-arg shape and the "never --all" regression pinned in skills.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/cli/src/commands/init.ts | 13 +- packages/cli/src/commands/skills.test.ts | 55 ++++++- packages/cli/src/commands/skills.ts | 35 +++- packages/cli/src/utils/skillsTargets.test.ts | 130 +++++++++++++++ packages/cli/src/utils/skillsTargets.ts | 158 +++++++++++++++++++ 5 files changed, 373 insertions(+), 18 deletions(-) create mode 100644 packages/cli/src/utils/skillsTargets.test.ts create mode 100644 packages/cli/src/utils/skillsTargets.ts diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 90814d4612..78f14afb2c 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -584,12 +584,6 @@ async function scaffoldProject( async function ensureSkillsCurrent(destDir: string): Promise { const { installAllSkills } = await import("./skills.js"); const { checkSkills } = await import("../utils/skillsManifest.js"); - // --all pulls every skill (incl. ones not yet installed); --yes keeps it - // non-interactive. When Claude Code is driving, target its native dir so - // skills land in .claude/skills/. - const extraArgs = process.env["CLAUDECODE"] - ? ["--all", "--agent", "claude-code", "--yes"] - : ["--all", "--yes"]; console.log(); console.log(c.bold("Checking AI coding skills against GitHub...")); @@ -602,7 +596,12 @@ async function ensureSkillsCurrent(destDir: string): Promise { } if (needsInstall) { - await installAllSkills({ cwd: destDir, extraArgs }); + // installAllSkills resolves the agent target set from destDir + the + // environment (Claude Code → claude-code; otherwise installed CLIs, else a + // Claude-Code + `.agents` floor). A freshly-scaffolded project has no agent + // folders yet, so this lands skills where the running agent will read them + // rather than spraying to every agent convention. + await installAllSkills({ cwd: destDir }); } else { console.log(c.success("AI coding skills are already up to date.")); } diff --git a/packages/cli/src/commands/skills.test.ts b/packages/cli/src/commands/skills.test.ts index e8dcce9029..b080ae191a 100644 --- a/packages/cli/src/commands/skills.test.ts +++ b/packages/cli/src/commands/skills.test.ts @@ -56,6 +56,17 @@ vi.mock("../utils/skillsManifest.js", () => ({ checkSkills: vi.fn(async () => ({ skills: [] })), })); +// Agent-target resolution probes the real cwd / PATH / env, which would make +// the spawned-args assertions environment-dependent. Pin it to a fixed result +// so these tests verify how the command BUILDS the spawn, not what's installed +// on the test host. The resolver's own decision tree is covered in +// skillsTargets.test.ts. buildSkillsAddArgs is reproduced (it's trivial) so the +// arg shape under test stays real. +vi.mock("../utils/skillsTargets.js", () => ({ + resolveAgentTargets: vi.fn(() => ({ agents: ["claude-code", "universal"], reason: "test" })), + buildSkillsAddArgs: (agents: string[]) => ["--skill", "*", "--agent", ...agents, "--yes"], +})); + function setPlatform(platform: NodeJS.Platform): void { Object.defineProperty(process, "platform", { value: platform, @@ -109,13 +120,35 @@ describe("hyperframes skills", () => { "linux", "npx", ["--version"], - ["skills", "add", "https://github.com/heygen-com/hyperframes", "--all", "--copy"], + [ + "skills", + "add", + "https://github.com/heygen-com/hyperframes", + "--skill", + "*", + "--agent", + "claude-code", + "universal", + "--yes", + "--copy", + ], ], [ "darwin", "npx", ["--version"], - ["skills", "add", "https://github.com/heygen-com/hyperframes", "--all", "--copy"], + [ + "skills", + "add", + "https://github.com/heygen-com/hyperframes", + "--skill", + "*", + "--agent", + "claude-code", + "universal", + "--yes", + "--copy", + ], ], [ "win32", @@ -129,7 +162,12 @@ describe("hyperframes skills", () => { "skills", "add", "https://github.com/heygen-com/hyperframes", - "--all", + "--skill", + "*", + "--agent", + "claude-code", + "universal", + "--yes", "--copy", ], ], @@ -162,9 +200,16 @@ describe("hyperframes skills", () => { setPlatform("linux"); await runSkillsUpdate(); expect(process.exitCode).toBe(0); + const args = state.spawnCalls[0]?.args ?? []; // pulls the full set straight from GitHub - expect(state.spawnCalls[0]?.args).toContain("https://github.com/heygen-com/hyperframes"); - expect(state.spawnCalls[0]?.args).toContain("--all"); + expect(args).toContain("https://github.com/heygen-com/hyperframes"); + // every skill, but to a scoped agent set — never the `--all` (= `--agent '*'`) spray + expect(args).toContain("--skill"); + expect(args).toContain("--agent"); + expect(args).not.toContain("--all"); + // `--agent` must be followed by a concrete key, never the `'*'` wildcard + const agentValue = args[args.indexOf("--agent") + 1]; + expect(agentValue).not.toBe("*"); }); // `skills add --all` never deletes, so update must separately prune skills the diff --git a/packages/cli/src/commands/skills.ts b/packages/cli/src/commands/skills.ts index abd51efdaa..f3b2b093ce 100644 --- a/packages/cli/src/commands/skills.ts +++ b/packages/cli/src/commands/skills.ts @@ -10,6 +10,7 @@ import { type SkillDiff, type SkillsCheckResult, } from "../utils/skillsManifest.js"; +import { buildSkillsAddArgs, resolveAgentTargets } from "../utils/skillsTargets.js"; import type { Example } from "./_examples.js"; export const examples: Example[] = [ @@ -57,6 +58,25 @@ function runSkillsAdd( source: string, opts: { cwd?: string; extraArgs?: string[] } = {}, ): Promise { + // Targeting: an explicit `extraArgs` wins (callers/tests that know exactly + // what they want); otherwise resolve which agents to install to. We must NOT + // use the upstream `--all` (= `--skill '*' --agent '*' -y`), which sprays the + // skills into every one of ~70 agent conventions on the machine. Instead we + // install every skill (`--skill '*'`) to a scoped agent set: the project's + // existing skill folders, else the agent running us / installed agent CLIs, + // else a Claude-Code + `.agents` floor. See resolveAgentTargets. + let extraArgs = opts.extraArgs; + if (!extraArgs) { + const targets = resolveAgentTargets({ + cwd: opts.cwd ?? process.cwd(), + env: process.env, + pathStr: process.env["PATH"] ?? "", + platform: process.platform, + }); + console.log(c.dim(`Installing to: ${targets.agents.join(", ")} — ${targets.reason}`)); + extraArgs = buildSkillsAddArgs(targets.agents); + } + // `--copy` writes real files into each target agent's skills dir, instead of // the upstream default (a canonical `.agents/skills` store + per-agent // symlinks). That default re-serialises each SKILL.md's frontmatter, so an @@ -64,7 +84,7 @@ function runSkillsAdd( // check` then reports a freshly-installed set as outdated, and the symlinked // layout doesn't reliably land where the agent actually reads. Real copies // keep the install faithful to the manifest and detectable by `skills check`. - return spawnNpx(["skills", "add", source, ...(opts.extraArgs ?? ["--all"]), "--copy"], opts); + return spawnNpx(["skills", "add", source, ...extraArgs, "--copy"], opts); } // Skill names are kebab-case directory names. Refuse anything that isn't one @@ -244,12 +264,15 @@ const updateCommand = defineCommand({ 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 + // The install 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. + // runSkillsAdd resolves the agent target set itself (existing project + // folders → installed CLIs → a Claude-Code + `.agents` floor); we no longer + // spray to every agent via `--all`. // // Note: the upstream `skills add` CLI has no `--dir` flag (it installs into - // detected agent dirs), so `--dir` here scopes only the *prune* detection + // the resolved 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. @@ -259,14 +282,14 @@ const updateCommand = defineCommand({ // fails (no npx, `skills add` exits non-zero) it must exit non-zero too — // otherwise the `||` chain passes while nothing actually changed. try { - await installAllSkills({ extraArgs: ["--all", "--yes"], strict: true }); + await installAllSkills({ strict: true }); } catch (err) { clack.log.error(c.error(`Update failed: ${(err as Error).message}`)); process.exitCode = 1; return; } - // `skills add --all` never deletes, so a skill renamed or dropped upstream + // `skills add` never deletes, so a skill renamed or dropped upstream // (e.g. graphic-overlays → talking-head-recut) would linger forever. Prune // skills the lock attributes to our source that the manifest no longer // ships, so `check || update` fully reconciles the install to the manifest. diff --git a/packages/cli/src/utils/skillsTargets.test.ts b/packages/cli/src/utils/skillsTargets.test.ts new file mode 100644 index 0000000000..5d4f6fcbb2 --- /dev/null +++ b/packages/cli/src/utils/skillsTargets.test.ts @@ -0,0 +1,130 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { buildSkillsAddArgs, resolveAgentTargets } from "./skillsTargets.js"; + +const tmpDirs: string[] = []; + +function tempDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tmpDirs.push(dir); + return dir; +} + +/** A project root containing the given `/skills` folders. */ +function projectWith(...hostDirs: string[]): string { + const root = tempDir("hf-targets-proj-"); + for (const host of hostDirs) mkdirSync(join(root, host, "skills"), { recursive: true }); + return root; +} + +/** A PATH-style string pointing at a dir that contains the given fake executables. */ +function pathWith(...bins: string[]): string { + const dir = tempDir("hf-targets-bin-"); + for (const bin of bins) writeFileSync(join(dir, bin), ""); + return dir; +} + +afterEach(() => { + for (const dir of tmpDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe("resolveAgentTargets", () => { + const blank = { env: {}, pathStr: "", platform: "linux" as const }; + + // ── 1. Existing project folders win, mapped dir → upstream key ────────────── + + it("honours an existing `.hermes/skills` folder, nothing else", () => { + const result = resolveAgentTargets({ ...blank, cwd: projectWith(".hermes") }); + expect(result.agents).toEqual(["hermes-agent"]); + }); + + it("maps `.factory` → droid and `.kiro` → kiro-cli (dir names differ from keys)", () => { + const result = resolveAgentTargets({ ...blank, cwd: projectWith(".factory", ".kiro") }); + expect(result.agents).toEqual(["droid", "kiro-cli"]); + }); + + it("maps the shared `.agents` dir to the single `universal` key", () => { + const result = resolveAgentTargets({ ...blank, cwd: projectWith(".agents") }); + expect(result.agents).toEqual(["universal"]); + }); + + it("returns claude-code first across multiple existing folders", () => { + const result = resolveAgentTargets({ ...blank, cwd: projectWith(".agents", ".claude") }); + expect(result.agents).toEqual(["claude-code", "universal"]); + }); + + it("existing folders take precedence over CLAUDECODE and PATH", () => { + const result = resolveAgentTargets({ + cwd: projectWith(".hermes"), + env: { CLAUDECODE: "1" }, + pathStr: pathWith("claude", "cursor"), + platform: "linux", + }); + expect(result.agents).toEqual(["hermes-agent"]); + }); + + // ── 2a. Claude Code env on a blank project ────────────────────────────────── + + it("targets just claude-code when running under Claude Code", () => { + const result = resolveAgentTargets({ + ...blank, + cwd: projectWith(), + env: { CLAUDECODE: "1" }, + }); + expect(result.agents).toEqual(["claude-code"]); + }); + + // ── 2b. gstack route: installed agent CLIs on PATH ────────────────────────── + + it("detects installed agent CLIs on PATH (blank project, no CLAUDECODE)", () => { + const result = resolveAgentTargets({ + cwd: projectWith(), + env: {}, + pathStr: pathWith("claude", "hermes"), + platform: "linux", + }); + expect(result.agents).toEqual(["claude-code", "hermes-agent"]); + }); + + it("collapses universal-bucket CLIs (cursor/codex/…) to a single `universal`", () => { + const result = resolveAgentTargets({ + cwd: projectWith(), + env: {}, + pathStr: pathWith("cursor", "codex", "gemini"), + platform: "linux", + }); + expect(result.agents).toEqual(["universal"]); + }); + + // ── 2c. Floor ─────────────────────────────────────────────────────────────── + + it("falls back to claude-code + universal (.claude + .agents) when nothing is found", () => { + const result = resolveAgentTargets({ ...blank, cwd: projectWith() }); + expect(result.agents).toEqual(["claude-code", "universal"]); + }); + + // ── Invariant: never the `--all` spray ────────────────────────────────────── + + it("never returns the `'*'` wildcard agent", () => { + for (const cwd of [projectWith(), projectWith(".hermes"), projectWith(".claude")]) { + const result = resolveAgentTargets({ ...blank, cwd }); + expect(result.agents).not.toContain("*"); + expect(result.agents.length).toBeGreaterThan(0); + } + }); +}); + +describe("buildSkillsAddArgs", () => { + it("installs every skill to the given agents, non-interactive — not `--all`", () => { + expect(buildSkillsAddArgs(["claude-code", "universal"])).toEqual([ + "--skill", + "*", + "--agent", + "claude-code", + "universal", + "--yes", + ]); + }); +}); diff --git a/packages/cli/src/utils/skillsTargets.ts b/packages/cli/src/utils/skillsTargets.ts new file mode 100644 index 0000000000..1e12b38e35 --- /dev/null +++ b/packages/cli/src/utils/skillsTargets.ts @@ -0,0 +1,158 @@ +// Decide WHICH agents a `skills add` should install to, so HyperFrames never +// sprays its skills into every one of the ~70 agent conventions the upstream +// `skills` CLI knows about (its `--all` is shorthand for `--agent '*'`). +// +// The policy, in priority order: +// 1. If the project already has agent skill folders (`.claude/skills`, +// `.hermes/skills`, …), install ONLY to those. An existing folder is the +// strongest signal of intent — honour it exactly, add nothing else. +// 2. Otherwise (blank project), pick targets from the machine: +// 2a. Running under Claude Code (`CLAUDECODE`) → just claude-code. +// 2b. Else probe the PATH for installed agent CLIs (the gstack approach: +// an executable on PATH means that agent is actually installed here). +// 2c. Else fall back to the floor: claude-code + the shared `.agents` +// universal dir (which Cursor, Codex, OpenCode, Gemini, Copilot and a +// dozen others read from in project scope). +// +// All paths are PROJECT-scoped (the default for `skills add` without `--global`), +// which is why the dir map below is the project-scope layout. + +import { existsSync, statSync } from "node:fs"; +import { delimiter, join } from "node:path"; + +/** + * Project-scope host directory → the upstream `skills` `--agent` key that + * installs into it. The dir name deliberately differs from the key for several + * agents (`.factory` ↔ `droid`, `.hermes` ↔ `hermes-agent`), and many agents + * share the `.agents` universal dir, so the mapping is explicit rather than + * derived. Keys verified against vercel-labs/skills@v1.5.13. + */ +const DIR_TO_KEY: Readonly> = { + ".claude": "claude-code", + ".agents": "universal", + ".hermes": "hermes-agent", + ".factory": "droid", + ".kiro": "kiro-cli", +}; + +/** + * Agent CLIs we probe for on PATH, paired with the project-scope host dir each + * installs into. Several (Cursor, Codex, OpenCode, Gemini) share `.agents`, so + * detecting any of them resolves — via DIR_TO_KEY — to the single `universal` + * key and one write to `.agents/skills`. OpenClaw is intentionally absent: its + * project skills dir is a bare `skills/`, which collides with common project + * layouts, so we never auto-target it (an existing folder or explicit `--agent` + * still works upstream). + */ +const DETECTABLE: ReadonlyArray<{ bin: string; dir: string }> = [ + { bin: "claude", dir: ".claude" }, + { bin: "hermes", dir: ".hermes" }, + { bin: "droid", dir: ".factory" }, + { bin: "cursor", dir: ".agents" }, + { bin: "codex", dir: ".agents" }, + { bin: "opencode", dir: ".agents" }, + { bin: "gemini", dir: ".agents" }, +]; + +export interface ResolveTargetsInput { + /** Project root the install targets (cwd, or the init destination). */ + cwd: string; + /** Process env — read for the `CLAUDECODE` signal. */ + env: NodeJS.ProcessEnv; + /** PATH string for the on-PATH binary probe. */ + pathStr: string; + /** Platform — selects the executable extensions probed on Windows. */ + platform: NodeJS.Platform; +} + +export interface ResolvedTargets { + /** Upstream `--agent` keys to install to (never `'*'`). */ + agents: string[]; + /** Short human-readable explanation of why these were chosen. */ + reason: string; +} + +function isDir(path: string): boolean { + try { + return existsSync(path) && statSync(path).isDirectory(); + } catch { + return false; + } +} + +/** True if `bin` resolves on any PATH entry (Windows also tries .exe/.cmd/.bat). */ +function isOnPath(bin: string, pathStr: string, platform: NodeJS.Platform): boolean { + const exts = platform === "win32" ? ["", ".exe", ".cmd", ".bat"] : [""]; + for (const dir of pathStr.split(delimiter)) { + if (!dir) continue; + for (const ext of exts) { + try { + if (existsSync(join(dir, bin + ext))) return true; + } catch { + // Unreadable PATH entry — skip it. + } + } + } + return false; +} + +/** Map a set of host dirs to deduped `--agent` keys, claude-code first (DIR_TO_KEY order). */ +function keysForDirs(dirs: ReadonlySet): string[] { + return Object.entries(DIR_TO_KEY) + .filter(([dir]) => dirs.has(dir)) + .map(([, key]) => key); +} + +/** Agent skill folders that already exist under the project root. */ +function existingProjectAgents(cwd: string): string[] { + const dirs = new Set(); + for (const dir of Object.keys(DIR_TO_KEY)) { + if (isDir(join(cwd, dir, "skills"))) dirs.add(dir); + } + return keysForDirs(dirs); +} + +/** Agent CLIs installed on this machine (by PATH probe), as `--agent` keys. */ +function detectInstalledAgents(pathStr: string, platform: NodeJS.Platform): string[] { + const dirs = new Set(); + for (const { bin, dir } of DETECTABLE) { + if (isOnPath(bin, pathStr, platform)) dirs.add(dir); + } + return keysForDirs(dirs); +} + +/** + * Resolve the `--agent` targets for an install. See the file header for the + * full policy. Pure: all inputs are passed in, so it is fully unit-testable. + */ +export function resolveAgentTargets(input: ResolveTargetsInput): ResolvedTargets { + // 1. Honour what the project already has — nothing more. + const existing = existingProjectAgents(input.cwd); + if (existing.length > 0) { + return { agents: existing, reason: `existing project skill folders (${existing.join(", ")})` }; + } + + // 2a. Strongest live signal: the agent running this command. + if (input.env["CLAUDECODE"]) { + return { agents: ["claude-code"], reason: "running under Claude Code" }; + } + + // 2b. gstack approach: agent CLIs actually installed on this machine. + const detected = detectInstalledAgents(input.pathStr, input.platform); + if (detected.length > 0) { + return { agents: detected, reason: `installed agent CLIs (${detected.join(", ")})` }; + } + + // 2c. Floor: Claude Code + the shared `.agents` universal dir. Never `--agent '*'`. + return { agents: ["claude-code", "universal"], reason: "default (.claude + .agents)" }; +} + +/** + * Build the `skills add` arguments for a resolved target set: every skill + * (`--skill '*'`) to the chosen agents only, non-interactive. This replaces the + * upstream `--all` (= `--skill '*' --agent '*' -y`) so the agent fan-out is + * scoped instead of universal. + */ +export function buildSkillsAddArgs(agents: string[]): string[] { + return ["--skill", "*", "--agent", ...agents, "--yes"]; +}