From 7f949075983c653d4d7877c33dda54b712dd9b0e Mon Sep 17 00:00:00 2001 From: minhn4 Date: Thu, 30 Jul 2026 23:23:11 +0700 Subject: [PATCH 1/2] feat: install pulled skills for every agent, not just Claude Code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skill pull` wrote only to `.claude/skills`, so a hub skill was usable by Claude Code and nothing else — a gap in a product whose flagship agent is CoDev Code. Which directory each agent reads was established from the agents' own source, not their docs: the table baked into the CoDev Code bundle lists only the `~/…` paths and omits the project-scope walk, which is the opposite of what `packages/opencode/src/skill/index.ts#discoverSkills` does. It scans `.claude` AND `.agents` under `$HOME` and again walking cwd up to the worktree root. Codex reads `.agents/skills` at both scopes; Claude Code reads `.claude/skills` at both. So two directories cover all four agents with one rule at both scopes, and nothing needs a per-agent directory. resolveTargets extracts once into whichever directory covers the most selected agents and links the other only when some selected agent can't reach it — when Codex isn't selected, `.claude/skills` alone serves everyone and no link is created at all. A hub skill can run to thousands of files, so a copy per agent is not free. Links are relative symlinks, matching this repo's own committed skill (`.claude/skills/vercel-… -> ../../.agents/skills/vercel-…`, git mode 120000) — relative is what survives a clone. linkOrCopy degrades to a Windows junction (no Developer Mode or admin needed) and then to a copy, reporting the mode verbatim so a copy is never called a link. Claude Code follows symlinked skill dirs only from v2.1.203, so that one link probes `claude --version` and copies below the floor. `--here`/`--global` now select scope only; `--agent `/`--all-agents` select agents, reusing the launch names `restore` accepts. CoDev Code is always in the set and the picker renders it locked. `--dir` is unchanged and rejects `--agent`. Verified end-to-end against the real agents: CoDev Code lists the skill via `/skill` (v1 — `/api/skill` is v2 and reports nothing from these directories, which looks exactly like a broken install), Codex has it in its session context, and Claude Code follows the relative symlink. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 26 ++++ src/SkillPullApp.tsx | 95 ++++++++++-- src/index.tsx | 1 + src/lib/help.ts | 7 +- src/lib/skill-dirs.ts | 220 +++++++++++++++++++++++++++ src/lib/skill-install.ts | 262 ++++++++++++++++++++++++++------ tests/SkillPullApp.test.tsx | 131 +++++++++++++++- tests/lib/skill-dirs.test.ts | 215 ++++++++++++++++++++++++++ tests/lib/skill-install.test.ts | 182 ++++++++++++++++++++-- 9 files changed, 1057 insertions(+), 82 deletions(-) create mode 100644 src/lib/skill-dirs.ts create mode 100644 tests/lib/skill-dirs.test.ts diff --git a/AGENTS.md b/AGENTS.md index 99d2d56..8ca2b25 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -249,6 +249,32 @@ The reader side is `src/lib/logs.ts` (`codevhub logs`): bare mode prints the mos Testing: logging is a silent no-op until `initLogging` runs, so ordinary tests need no setup and never write files. Tests that assert documents stub `CODEV_LOG_DIR`, call `initLogging(cmd, [], { installProcessHooks: false })` (so vitest's process stays free of our exit/crash listeners), and `resetLogging()` in `afterEach`. Related: `login()`'s force-login probe is keyed off `~/.codev-hub/auth.json` — not the `~/.codev-hub` dir — precisely because the logger creates `~/.codev-hub/logs` at the entry of every command. +## Where skills go (`codevhub skill pull`) + +`src/lib/skill-dirs.ts` owns the one fact this feature turns on: **which directory each agent reads.** Establish it from the agents' own source, never from their docs — the table baked into the CoDev Code bundle lists only the `~/…` paths and omits the project-scope walk, which is the opposite of what the code does. + +The authority is `packages/opencode/src/skill/index.ts#discoverSkills` (CoDev Code / OpenCode — same module): `externalDirs = [".claude", ".agents"]`, scanned under `global.home` **and** walked from cwd up to the worktree root. Codex reads `.agents/skills` at both scopes (and `$HOME`); Claude Code reads `.claude/skills` at both. So: + +| | `.agents/skills` | `.claude/skills` | +|---|---|---| +| Codex | yes | no | +| CoDev Code / OpenCode | yes | yes | +| Claude Code | no | yes | + +Neither directory alone covers all four; together they do, with the **same rule at both scopes** — scope only chooses the root (`process.cwd()` vs `homedir()`). No agent needs a directory of its own, and in particular there is no `.codev/skills` or `.opencode/skills` link to write. + +`resolveTargets` therefore extracts **once** into whichever directory covers the most selected agents and links the other only if some selected agent can't reach it. When Codex isn't selected, `.claude/skills` alone serves everyone and only one directory is created. Adding Codex is what forces the second into existence. A hub skill can run to thousands of files (one is ~11k), so fanning out a copy per agent is not free. + +**Links are relative symlinks.** This repo's own skill is wired exactly that way — `.claude/skills/vercel-react-best-practices -> ../../.agents/skills/vercel-react-best-practices`, committed as git mode 120000 — and relative is what survives `git clone`; an absolute link breaks in every other checkout. `linkOrCopy` degrades in order: relative symlink → Windows **junction** (absolute, but needs neither Developer Mode nor admin, unlike a Windows symlink) → recursive copy. The mode is reported verbatim in `InstallResult.placements`, so a fallback copy is never described as a link. + +**Claude Code follows a symlinked skill directory only from v2.1.203.** Below that the link reads as a file with no SKILL.md and the skill is simply invisible, so `claudeFollowsSymlinks()` probes `claude --version` and forces a copy for that one link. Tests must stub `npm.execAsync` or they depend on whatever Claude Code the machine happens to have. + +**The duplicate-name warning is expected, not a bug.** When both directories exist, CoDev Code and OpenCode scan both, reach the same skill twice, and log `duplicate skill name` (`index.ts:125`); the last scan wins and the skill resolves to a single entry. It is log-only — warnings are not published as session events, unlike the parse errors just above them — and it already happens for any user whose skill sits in both directories, independent of CoDev. Users have `CODEV_DISABLE_CLAUDE_CODE_SKILLS` / `CODEV_DISABLE_EXTERNAL_SKILLS` (`packages/opencode/src/effect/runtime-flags.ts`); **CoDev must not set those on their behalf.** + +**Verifying against CoDev Code: query `/skill`, not `/api/skill`.** They are two different services. `/api/skill` is v2 (`packages/core/src/skill.ts` + `config/plugin/skill.ts`), which registers only `/skill{,s}` plus `skills.paths` and so reports **nothing** from `.claude`/`.agents` — querying it will look exactly like a broken install. `/skill` is v1, the service `session/system.ts` uses to build the actual prompt. If CoDev Code ever migrates the prompt to v2, `.agents/skills` will need a `skills.paths` entry and this design changes. + +CoDev Code is the flagship: `ALWAYS_AGENT` is in every target set, the picker renders it locked, and `--agent` folds it in whether or not it was named. + ## CodeGraph integration `src/lib/codegraph.ts` integrates the external [CodeGraph](https://www.npmjs.com/package/@colbymchenry/codegraph) tool (a CLI + MCP server). Two surfaces: diff --git a/src/SkillPullApp.tsx b/src/SkillPullApp.tsx index c2c00f5..b58caab 100644 --- a/src/SkillPullApp.tsx +++ b/src/SkillPullApp.tsx @@ -7,11 +7,17 @@ import { Step } from "@/components/Step.js"; import { useCanType } from "@/components/useCanType.js"; import { stripControlChars } from "@/lib/sanitize.js"; import { + AGENT_LABELS, + ALWAYS_AGENT, + SKILL_AGENTS, + type SkillAgent, +} from "@/lib/skill-dirs.js"; +import { + defaultAgents, formatInstallResult, type InstallLocation, type InstallResult, installResolvedSkill, - skillsDirFor, } from "@/lib/skill-install.js"; import { getSkillMeta, type SkillMeta } from "@/lib/skillhub.js"; @@ -19,12 +25,21 @@ interface SkillPullAppProps { target: string; force: boolean; json: boolean; + // Agent set from --agent/--all-agents. Absent ⇒ prompt for it, pre-checked + // with defaultAgents(). + agents?: SkillAgent[]; // Reports success/failure so the caller can set the process exit code, then // the app exits on its own. Optional so tests can omit it. onDone?: (ok: boolean) => void; } -type Phase = "resolving" | "select" | "installing" | "done" | "error"; +type Phase = + | "resolving" + | "select" + | "agents" + | "installing" + | "done" + | "error"; const LOCATIONS: { key: InstallLocation; label: string }[] = [ { key: "current", label: "Current directory" }, @@ -35,6 +50,7 @@ export function SkillPullApp({ target, force, json, + agents, onDone, }: SkillPullAppProps) { const { exit } = useApp(); @@ -44,6 +60,13 @@ export function SkillPullApp({ const [index, setIndex] = useState(0); const [result, setResult] = useState(null); const [error, setError] = useState(null); + const [scope, setScope] = useState("current"); + const [agentIndex, setAgentIndex] = useState(0); + // Pre-check: whatever CoDev has configured, plus the always-on flagship. + // Computed once — detectCodevTools() hits the filesystem. + const [picked, setPicked] = useState>( + () => new Set(agents ?? defaultAgents()), + ); // Signal the outcome, then unmount. exit() takes no error — the exit code is // carried by onDone — so waitUntilExit resolves cleanly either way. @@ -78,12 +101,12 @@ export function SkillPullApp({ }, [target, finish]); const start = useCallback( - async (location: InstallLocation) => { + async (location: InstallLocation, chosen: readonly SkillAgent[]) => { if (!meta) return; setPhase("installing"); try { const r = await installResolvedSkill(meta, { - rootDir: skillsDirFor(location), + target: { kind: "agents", agents: [...chosen], scope: location }, force, }); setResult(r); @@ -99,14 +122,14 @@ export function SkillPullApp({ ); // The dispatcher already routes a keyboard-less terminal to the plain runner, - // so reaching "select" without one means Ink's stdin isn't the process's own. + // so reaching a prompt without one means Ink's stdin isn't the process's own. // Say so and exit rather than mounting a picker that can never be answered: // unlike the ungated case this is a silent hang, not a throw. useEffect(() => { - if (phase !== "select" || canType) return; + if ((phase !== "select" && phase !== "agents") || canType) return; setError( - "This terminal cannot supply keystrokes, so the location prompt cannot be shown.\n" + - "Pass --here, --global, or --dir to choose a location without it.", + "This terminal cannot supply keystrokes, so the install prompts cannot be shown.\n" + + "Pass --here, --global, or --dir to choose a location without them.", ); setPhase("error"); finish(false); @@ -120,12 +143,47 @@ export function SkillPullApp({ } else if (key.downArrow) { setIndex((i) => (i + 1) % LOCATIONS.length); } else if (key.return) { - void start(LOCATIONS[index]?.key ?? "current"); + const location = LOCATIONS[index]?.key ?? "current"; + setScope(location); + // An explicit --agent/--all-agents already answered the second + // question; don't ask it again. + if (agents) { + void start(location, agents); + return; + } + setPhase("agents"); } }, { isActive: canType && phase === "select" }, ); + useInput( + (input, key) => { + if (phase !== "agents") return; + if (key.upArrow) { + setAgentIndex((i) => (i === 0 ? SKILL_AGENTS.length - 1 : i - 1)); + } else if (key.downArrow) { + setAgentIndex((i) => (i + 1) % SKILL_AGENTS.length); + } else if (input === " ") { + const agent = SKILL_AGENTS[agentIndex]; + // CoDev Code is the flagship and is never opted out of. + if (!agent || agent === ALWAYS_AGENT) return; + setPicked((prev) => { + const next = new Set(prev); + if (next.has(agent)) next.delete(agent); + else next.add(agent); + return next; + }); + } else if (key.return) { + void start( + scope, + SKILL_AGENTS.filter((a) => picked.has(a)), + ); + } + }, + { isActive: canType && phase === "agents" }, + ); + // Sanitize the hub-sourced name before rendering it to the terminal. const skillName = meta ? stripControlChars(meta.name) : null; const title = skillName ? `Install ${skillName} skill` : "Install skill"; @@ -153,6 +211,25 @@ export function SkillPullApp({ ))} )} + {phase === "agents" && ( + + For which agents? + {SKILL_AGENTS.map((agent, i) => { + const locked = agent === ALWAYS_AGENT; + const checked = locked || picked.has(agent); + return ( + + {`${i === agentIndex ? "❯ " : " "}[${checked ? "x" : " "}] ${AGENT_LABELS[agent]}`} + + ); + })} + space toggles · enter confirms + + )} {phase === "installing" && ( diff --git a/src/index.tsx b/src/index.tsx index 700d744..a253b5a 100644 --- a/src/index.tsx +++ b/src/index.tsx @@ -448,6 +448,7 @@ switch (command) { target={parsed.target} force={parsed.force} json={parsed.json} + agents={parsed.agents} onDone={(v) => { ok = v; }} diff --git a/src/lib/help.ts b/src/lib/help.ts index 0f45235..848b9c3 100644 --- a/src/lib/help.ts +++ b/src/lib/help.ts @@ -41,9 +41,10 @@ Skill hub: skill search Search the public skill hub (--json for machine-readable output, --limit to cap results, default 20) - skill pull Download and install a skill by name or id - (prompts for location; --here or --global to skip the - prompt, --dir for an exact path, --force to + skill pull Download and install a skill for your agents + (prompts for location and agents; --here or --global to + set the scope, --agent or --all-agents to set the + agents, --dir for an exact path, --force to overwrite; --json for machine-readable output) skill push Publish a skill (a directory with SKILL.md, or a .zip) (previews and confirms before upload; --draft-only to stop diff --git a/src/lib/skill-dirs.ts b/src/lib/skill-dirs.ts new file mode 100644 index 0000000..58a22b2 --- /dev/null +++ b/src/lib/skill-dirs.ts @@ -0,0 +1,220 @@ +import { cp, mkdir, symlink } from "node:fs/promises"; +import { homedir } from "node:os"; +import { isAbsolute, join, relative, resolve, sep } from "node:path"; +import { execAsync } from "@/lib/npm.js"; + +// Where each agent looks for skills, and how one extracted copy is made to serve +// several of them. +// +// Established by reading the agents themselves rather than their docs — the +// tables inside the CoDev Code bundle list only the `~/…` paths and omit the +// project-scope walk, which is the opposite of what the code does +// (`packages/opencode/src/skill/index.ts#discoverSkills`): +// +// externalDirs = [".claude", ".agents"] +// for (const dir of externalDirs) // GLOBAL: / +// scan(join(global.home, dir), "skills/**|SKILL.md", { dot: true }) +// for (const root of up({ targets: externalDirs, // PROJECT: cwd → worktree +// start: directory, stop: worktree })) +// scan(root, "skills/**|SKILL.md", { dot: true }) +// +// So CoDev Code and OpenCode read BOTH `.claude/skills` and `.agents/skills`, at +// BOTH scopes. Codex reads only `.agents/skills` (also cwd → repo root, plus +// `$HOME`); Claude Code reads only `.claude/skills`. Two directories therefore +// cover all four agents, and no agent needs a directory of its own. + +export const SKILL_AGENTS = ["claude", "codex", "opencode", "codev"] as const; +export type SkillAgent = (typeof SKILL_AGENTS)[number]; + +export function isSkillAgent(value: string): value is SkillAgent { + return (SKILL_AGENTS as readonly string[]).includes(value); +} + +// CoDev Code is the flagship agent and is always installed for — it is never +// absent from a target set, and the picker renders it as locked. +export const ALWAYS_AGENT: SkillAgent = "codev"; + +export const AGENT_LABELS: Record = { + claude: "Claude Code", + codex: "Codex", + opencode: "OpenCode", + codev: "CoDev Code", +}; + +export type Scope = "current" | "global"; + +// The two directories, relative to a scope root. +const CLAUDE_DIR = join(".claude", "skills"); +const AGENTS_DIR = join(".agents", "skills"); + +// Which agents read which directory. Both scopes use the same rule. +const READS_AGENTS_DIR: Record = { + codex: true, + codev: true, + opencode: true, + claude: false, +}; +const READS_CLAUDE_DIR: Record = { + codex: false, + codev: true, + opencode: true, + claude: true, +}; + +export function scopeRoot(scope: Scope): string { + return scope === "current" ? process.cwd() : homedir(); +} + +// How a link target was actually produced. `store` is the real extraction; the +// rest describe a second path pointing at it. Reported verbatim so a copy is +// never described as a link. +export type LinkMode = "store" | "symlink" | "junction" | "copy"; + +export interface SkillTargets { + // Absolute path the archive is extracted into — the single real copy. + store: string; + // Absolute paths that must end up pointing at `store`. + links: string[]; +} + +// Pick the directory covering the most selected agents as the store, then link +// the other one only if some selected agent can't reach the store. +// +// The asymmetry is deliberate: when Codex isn't selected, `.claude/skills` alone +// serves every remaining agent, so only one directory is created and CoDev +// Code/OpenCode never see the skill twice. Adding Codex is what forces the +// second directory into existence. +export function resolveTargets( + agents: readonly SkillAgent[], + scope: Scope, + name: string, +): SkillTargets { + const root = scopeRoot(scope); + const claudePath = join(root, CLAUDE_DIR, name); + const agentsPath = join(root, AGENTS_DIR, name); + + const needsAgentsDir = agents.some((a) => !READS_CLAUDE_DIR[a]); + if (!needsAgentsDir) return { store: claudePath, links: [] }; + + const needsClaudeDir = agents.some((a) => !READS_AGENTS_DIR[a]); + return { + store: agentsPath, + links: needsClaudeDir ? [claudePath] : [], + }; +} + +// Which selected agents a given path serves — so the result can say what each +// agent got rather than printing bare directories. +export function agentsServedBy( + path: string, + agents: readonly SkillAgent[], + scope: Scope, + name: string, +): SkillAgent[] { + const root = scopeRoot(scope); + const isClaudeDir = path === join(root, CLAUDE_DIR, name); + const table = isClaudeDir ? READS_CLAUDE_DIR : READS_AGENTS_DIR; + return agents.filter((a) => table[a]); +} + +// Stubbable indirection, same idiom as tlsApi / httpApi / spawner: the fallback +// chain below is chosen by whether these throw, which no test could otherwise +// provoke on a machine where symlinks work. +export const linkApi = { + symlink, + cp, +}; + +// Point `linkPath` at `store`, degrading rather than failing: +// +// 1. A RELATIVE symlink. This repo's own skill is wired exactly this way +// (`.claude/skills/vercel-react-best-practices -> +// ../../.agents/skills/vercel-react-best-practices`, committed as git mode +// 120000), and relative is what survives `git clone` — an absolute link +// would break in every other checkout. +// 2. A Windows junction. Unlike a Windows symlink it needs neither Developer +// Mode nor administrator rights, which is what makes this viable for the +// Git Bash audience. Junctions resolve only absolute targets, so a +// project-scope junction isn't meaningfully committable — a Windows-only +// degradation, not a regression. +// 3. A recursive copy, reported as such. +export async function linkOrCopy( + store: string, + linkPath: string, +): Promise { + await mkdir(join(linkPath, ".."), { recursive: true }); + const target = relativeTarget(store, linkPath); + + try { + await linkApi.symlink(target, linkPath, "dir"); + return "symlink"; + } catch { + // fall through + } + if (process.platform === "win32") { + try { + await linkApi.symlink(resolve(store), linkPath, "junction"); + return "junction"; + } catch { + // fall through + } + } + await linkApi.cp(store, linkPath, { recursive: true }); + return "copy"; +} + +// The link body: `store` expressed relative to the directory holding the link. +// Falls back to an absolute target only when the two share no common root +// (different Windows volumes), where a relative path cannot be formed at all. +function relativeTarget(store: string, linkPath: string): string { + const from = join(linkPath, ".."); + const rel = relative(resolve(from), resolve(store)); + if (!rel || isAbsolute(rel)) return resolve(store); + return rel; +} + +// Claude Code follows a symlinked skill directory only from v2.1.203 — earlier +// versions read the link as a plain file and find no SKILL.md. Below the floor +// the Claude link has to be a real copy, so probe before linking. +const CLAUDE_SYMLINK_FLOOR = [2, 1, 203] as const; + +export async function claudeFollowsSymlinks(): Promise { + const r = await execAsync("claude", ["--version"]); + if (r.error) return false; + const parsed = /(\d+)\.(\d+)\.(\d+)/.exec(r.stdout); + if (!parsed) return false; + const version = [ + Number(parsed[1]), + Number(parsed[2]), + Number(parsed[3]), + ] as const; + for (let i = 0; i < CLAUDE_SYMLINK_FLOOR.length; i++) { + const have = version[i] ?? 0; + const need = CLAUDE_SYMLINK_FLOOR[i] ?? 0; + if (have !== need) return have > need; + } + return true; +} + +// True when `linkPath` is the Claude directory — the only link whose mechanism +// depends on the agent's version. +export function isClaudeLink( + linkPath: string, + scope: Scope, + name: string, +): boolean { + return linkPath === join(scopeRoot(scope), CLAUDE_DIR, name); +} + +// Guard the skill name the same way skill-install does before it becomes a path +// segment under either directory. Exported so callers validate once and both +// target paths inherit it. +export function safeSegment(name: string): boolean { + return ( + name !== "" && + !name.startsWith(".") && + !name.includes(sep) && + !name.includes("/") && + !isAbsolute(name) + ); +} diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index 1041f82..dc6eeeb 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -1,6 +1,5 @@ -import { mkdir, rm } from "node:fs/promises"; -import { homedir } from "node:os"; -import { isAbsolute, join, relative, resolve, sep } from "node:path"; +import { cp, mkdir, rm } from "node:fs/promises"; +import { isAbsolute, relative, resolve, sep } from "node:path"; import { detectAndStripRoot, extractZip, @@ -8,6 +7,22 @@ import { pathExists, } from "@/lib/archive.js"; import { stripControlChars } from "@/lib/sanitize.js"; +import { detectCodevTools } from "@/lib/shims.js"; +import { + AGENT_LABELS, + ALWAYS_AGENT, + agentsServedBy, + claudeFollowsSymlinks, + isClaudeLink, + isSkillAgent, + type LinkMode, + linkOrCopy, + resolveTargets, + type Scope, + SKILL_AGENTS, + type SkillAgent, + safeSegment, +} from "@/lib/skill-dirs.js"; import { downloadSkill, getSkillMeta, type SkillMeta } from "@/lib/skillhub.js"; // The server's skill name becomes a directory under rootDir. Never trust it as @@ -30,35 +45,60 @@ function safeSkillDir(rootDir: string, name: string): string { return dir; } -export type InstallLocation = "current" | "global"; +// Scope is now the only thing --here/--global choose; which agents get the +// skill is an orthogonal axis (see lib/skill-dirs.ts). +export type InstallLocation = Scope; // One usage string for both entry points (dispatcher and non-interactive // runner), so the flag list can't drift between them. export const PULL_USAGE = - "Usage: codevhub skill pull [--here|--global|--dir ] [--force] [--json]"; - -// Root dir for each prompt choice. "current" is cwd-relative (created if -// missing on install); "global" is the home skills dir. Claude Agent Skills -// live under `.claude/skills` in both cases. -export function skillsDirFor(location: InstallLocation): string { - return location === "current" - ? join(process.cwd(), ".claude", "skills") - : join(homedir(), ".claude", "skills"); -} + "Usage: codevhub skill pull [--here|--global|--dir ] [--agent |--all-agents] [--force] [--json]"; export interface ParsedPull { target?: string; // Exact install root, verbatim from --dir: the skill lands in /, - // with no `.claude/skills` segment added. Mutually exclusive with `location`. + // with no agent directories and no links. Mutually exclusive with `location`. dir?: string; - // The picker's choice expressed as a flag (--here / --global), resolved - // through skillsDirFor. Mutually exclusive with `dir`. + // The picker's scope choice expressed as a flag (--here / --global). + // Mutually exclusive with `dir`. location?: InstallLocation; + // Explicit agent set from --agent/--all-agents. Absent ⇒ the caller picks the + // default (defaultAgents()); the prompt uses it as the pre-check. + agents?: SkillAgent[]; force: boolean; json: boolean; error?: string; } +// The set installed for when the user names none: CoDev Code always (the +// flagship agent is never opted out of), plus every other agent CoDev has +// actually configured on this machine. Used as both the prompt's pre-check and +// the non-interactive default, so CI never has to spell out --agent. +export function defaultAgents(): SkillAgent[] { + const detected = new Set(detectCodevTools()); + return SKILL_AGENTS.filter( + (agent) => agent === ALWAYS_AGENT || detected.has(agent), + ); +} + +// Parse a --agent value: comma-separated launch names, the same vocabulary +// `codevhub restore ` accepts. CoDev Code is folded in whether or not it +// was named — it is not opt-out. +export function parseAgentList(value: string): SkillAgent[] | string { + const names = value + .split(",") + .map((n) => n.trim()) + .filter((n) => n !== ""); + if (names.length === 0) return "Missing value for --agent."; + const unknown = names.filter((n) => !isSkillAgent(n)); + if (unknown.length > 0) { + return `Unknown agent: ${unknown.join(", ")}. Valid: ${SKILL_AGENTS.join(", ")}.`; + } + const chosen = new Set(names.filter(isSkillAgent)); + chosen.add(ALWAYS_AGENT); + return SKILL_AGENTS.filter((agent) => chosen.has(agent)); +} + // Every flag `pull` accepts. Anything else starting with "-" is a typo, and is // rejected rather than ignored — a silently dropped `--forse` looks like a // successful run that just didn't do what was asked. @@ -73,12 +113,14 @@ const PULL_FLAGS = new Set([ "--json", "--here", "--global", + "--agent", + "--all-agents", ]); // Parse `pull` args: first non-flag token is the target (name|id); flags are -// --here/--global, --dir , --force, --json. Shared by the interactive -// (index → app) and non-interactive (runSkillInstall) paths so parsing lives in -// one place. +// --here/--global, --dir , --agent/--all-agents, --force, --json. Shared +// by the interactive (index → app) and non-interactive (runSkillInstall) paths +// so parsing lives in one place. export function parsePullArgs(args: string[]): ParsedPull { const force = args.includes("--force"); const json = args.includes("--json"); @@ -95,17 +137,28 @@ export function parsePullArgs(args: string[]): ParsedPull { : undefined; let dir: string | undefined; + let agents: SkillAgent[] | undefined; const positionals: string[] = []; for (let i = 0; i < args.length; i++) { const arg = args[i] as string; - if (arg === "--dir") { + if (arg === "--dir" || arg === "--agent") { // Consume the value here so it is never mistaken for a flag or a target, // whatever it contains. const value = args[++i]; if (!value || value.startsWith("-")) { - return { force, json, error: "Missing value for --dir." }; + return { force, json, error: `Missing value for ${arg}.` }; } - dir = value; + if (arg === "--dir") { + dir = value; + continue; + } + const parsed = parseAgentList(value); + if (typeof parsed === "string") return { force, json, error: parsed }; + agents = parsed; + continue; + } + if (arg === "--all-agents") { + agents = [...SKILL_AGENTS]; continue; } if (arg.startsWith("-")) { @@ -124,62 +177,154 @@ export function parsePullArgs(args: string[]): ParsedPull { error: "Pass either --dir or --here/--global, not both.", }; } - return { target: positionals[0], dir, location, force, json }; + // --dir is the raw escape hatch: one exact directory, no agent directories + // and no links. An agent set would have nowhere to go. + if (dir !== undefined && agents !== undefined) { + return { + force, + json, + error: + "--dir installs to an exact path; it can't be combined with --agent.", + }; + } + return { target: positionals[0], dir, location, agents, force, json }; +} + +// Where one extraction landed, and which of the selected agents it serves. +// `mode` is reported verbatim so a copy is never described as a link. +export interface InstallPlacement { + path: string; + mode: LinkMode; + agents: SkillAgent[]; } export interface InstallResult { name: string; version?: string; id: string; + // The single real extraction. Kept as `dir` so the --json shape stays + // compatible with callers written against the single-directory install. dir: string; strippedRoot: string | null; + placements: InstallPlacement[]; +} + +// Where an install should go: either one exact directory (--dir) or the agent +// directories resolved from a scope. +export type InstallTarget = + | { kind: "dir"; rootDir: string } + | { kind: "agents"; agents: SkillAgent[]; scope: Scope }; + +// Resolve where this install writes, and check every destination is safe BEFORE +// any filesystem work — especially before the rm() below, so a hostile hub name +// can never delete a directory outside the intended root. +function planPlacements( + target: InstallTarget, + name: string, +): { store: string; links: string[]; agents: SkillAgent[] } { + if (target.kind === "dir") { + return { store: safeSkillDir(target.rootDir, name), links: [], agents: [] }; + } + // resolveTargets joins the name into two fixed roots, so validating the name + // as a single path segment covers both destinations at once. + if (!safeSegment(name)) { + throw new Error(`Refusing to install skill with unsafe name "${name}".`); + } + const { store, links } = resolveTargets(target.agents, target.scope, name); + return { store, links, agents: target.agents }; } // Download + extract a skill whose metadata is already resolved. The install // dir is named after the canonical skill name (meta.name), not the id or the // ZIP's root folder — the root is only stripped so files aren't double-nested. +// +// The archive is extracted exactly once, into `store`; every other agent +// directory becomes a link to it (lib/skill-dirs.ts#linkOrCopy). A hub skill can +// run to thousands of files, so fanning out copies per agent is not free. export async function installResolvedSkill( meta: SkillMeta, - opts: { rootDir: string; force: boolean }, + opts: { target: InstallTarget; force: boolean }, ): Promise { - // Validate the target path before any filesystem work (especially before the - // rm() below), so an unsafe server name can never write to — or delete — a - // location outside rootDir. - const skillDir = safeSkillDir(opts.rootDir, meta.name); + const plan = planPlacements(opts.target, meta.name); const zip = await downloadSkill(meta.id); // Vet the archive's size/entry counts before inflating or extracting it. inspectZip(zip); const { buffer, stripped } = detectAndStripRoot(zip); - if (await pathExists(skillDir)) { + // Every destination is checked before anything is removed, so a refusal on + // the second one can't leave the first already deleted. + for (const dir of [plan.store, ...plan.links]) { + if (!(await pathExists(dir))) continue; if (!opts.force) { throw new Error( - `Already installed at ${skillDir}. Pass --force to overwrite.`, + `Already installed at ${dir}. Pass --force to overwrite.`, ); } - await rm(skillDir, { recursive: true, force: true }); + } + for (const dir of [plan.store, ...plan.links]) { + await rm(dir, { recursive: true, force: true }); } - await mkdir(skillDir, { recursive: true }); - await extractZip(buffer, skillDir); + await mkdir(plan.store, { recursive: true }); + await extractZip(buffer, plan.store); + + const placements: InstallPlacement[] = [ + { + path: plan.store, + mode: "store", + agents: agentsFor(plan.store, opts.target, meta.name), + }, + ]; + for (const link of plan.links) { + // Claude Code only follows a symlinked skill directory from v2.1.203; on an + // older build the link reads as a file with no SKILL.md inside, so it has + // to be a real copy. + const mustCopy = + opts.target.kind === "agents" && + isClaudeLink(link, opts.target.scope, meta.name) && + !(await claudeFollowsSymlinks()); + const mode = mustCopy + ? await copyInto(plan.store, link) + : await linkOrCopy(plan.store, link); + placements.push({ + path: link, + mode, + agents: agentsFor(link, opts.target, meta.name), + }); + } return { name: meta.name, version: meta.version, id: meta.id, - dir: skillDir, + dir: plan.store, strippedRoot: stripped, + placements, }; } +function agentsFor( + path: string, + target: InstallTarget, + name: string, +): SkillAgent[] { + if (target.kind === "dir") return []; + return agentsServedBy(path, target.agents, target.scope, name); +} + +async function copyInto(store: string, dest: string): Promise { + await cp(store, dest, { recursive: true }); + return "copy"; +} + // Resolve (id or name) to its canonical metadata, then download + // extract. No console I/O — returns the result or throws. Used by the // non-interactive runner; the Ink app resolves metadata itself (to show the // name in the prompt) and calls installResolvedSkill directly. export async function installSkill( target: string, - opts: { rootDir: string; force: boolean }, + opts: { target: InstallTarget; force: boolean }, ): Promise { const meta = await getSkillMeta(target); return installResolvedSkill(meta, opts); @@ -194,19 +339,36 @@ export function formatInstallResult(r: InstallResult, json: boolean): string { id: r.id, dir: r.dir, strippedRoot: r.strippedRoot, + placements: r.placements, }); } - // Sanitize hub-sourced name/version for terminal display (dir is a local, - // already-validated path). JSON output above needs no scrubbing — stringify - // escapes control characters. + // Sanitize hub-sourced name/version for terminal display (paths are local and + // already validated). JSON output above needs no scrubbing — stringify escapes + // control characters. const name = stripControlChars(r.name); const versionSuffix = r.version ? `@${stripControlChars(r.version)}` : ""; - return `Installed ${name}${versionSuffix} -> ${r.dir}`; + const served = r.placements.flatMap((p) => p.agents); + // --dir installs for no particular agent, so it keeps the original one-liner. + if (served.length === 0) { + return `Installed ${name}${versionSuffix} -> ${r.dir}`; + } + const labels = SKILL_AGENTS.filter((a) => served.includes(a)).map( + (a) => AGENT_LABELS[a], + ); + const lines = [`Installed ${name}${versionSuffix} for ${labels.join(", ")}`]; + for (const placement of r.placements) { + // Name the mechanism only when it isn't the real copy — and never call a + // fallback copy a link. + const suffix = placement.mode === "store" ? "" : ` (${placement.mode})`; + lines.push(` ${placement.path}${suffix}`); + } + return lines.join("\n"); } // Non-interactive path (a location flag given, or piped/CI). The interactive // prompt lives in SkillPullApp; here a location MUST be explicit, so no flag at -// all is an error rather than a silent default. Returns the exit code. +// all is an error rather than a silent default. The agent set does default +// though (defaultAgents()) — CI should not have to spell out --agent. export async function runSkillInstall(args: string[]): Promise { const parsed = parsePullArgs(args); if (parsed.error) { @@ -217,15 +379,19 @@ export async function runSkillInstall(args: string[]): Promise { console.error(PULL_USAGE); return 1; } - // --here/--global reproduce the picker's two choices (and so append - // `.claude/skills`); --dir is the escape hatch and is used verbatim. - const rootDir = + // --dir is the escape hatch and is used verbatim; --here/--global resolve to + // the selected agents' directories for that scope. + const target: InstallTarget | null = parsed.dir !== undefined - ? resolve(parsed.dir) + ? { kind: "dir", rootDir: resolve(parsed.dir) } : parsed.location !== undefined - ? skillsDirFor(parsed.location) + ? { + kind: "agents", + agents: parsed.agents ?? defaultAgents(), + scope: parsed.location, + } : null; - if (rootDir === null) { + if (target === null) { console.error( "Not a terminal — pass --here, --global, or --dir to choose an install location.", ); @@ -234,7 +400,7 @@ export async function runSkillInstall(args: string[]): Promise { try { const result = await installSkill(parsed.target, { - rootDir, + target, force: parsed.force, }); console.log(formatInstallResult(result, parsed.json)); diff --git a/tests/SkillPullApp.test.tsx b/tests/SkillPullApp.test.tsx index 495b7c9..3018188 100644 --- a/tests/SkillPullApp.test.tsx +++ b/tests/SkillPullApp.test.tsx @@ -2,6 +2,7 @@ import { homedir } from "node:os"; import { join } from "node:path"; import { cleanup, render } from "ink-testing-library"; import { afterEach, describe, expect, test, vi } from "vitest"; +import type { SkillAgent } from "@/lib/skill-dirs.js"; import * as install from "@/lib/skill-install.js"; import * as skillhub from "@/lib/skillhub.js"; import { SkillPullApp } from "@/SkillPullApp.js"; @@ -54,15 +55,22 @@ async function confirm( } function okResult(dir: string): install.InstallResult { + const path = join(dir, "pg-tuner"); return { name: "pg-tuner", version: "1.2.0", id: ID, - dir: join(dir, "pg-tuner"), + dir: path, strippedRoot: "pg-tuner", + placements: [{ path, mode: "store", agents: ["codev"] }], }; } +// Most tests here are about the location step, so they pass `agents` explicitly: +// that skips the agent picker AND keeps them off detectCodevTools(), which reads +// the real machine. The picker has its own tests below. +const CODEV_ONLY: SkillAgent[] = ["codev"]; + function mockResolve(meta: skillhub.SkillMeta = META) { return vi.spyOn(skillhub, "getSkillMeta").mockResolvedValue(meta); } @@ -97,12 +105,17 @@ describe("SkillPullApp", () => { .mockResolvedValue(okResult(currentRoot)); const { stdin, lastFrame } = render( - , + , ); await confirm(stdin, lastFrame, () => spy.mock.calls.length > 0); expect(spy).toHaveBeenCalledWith(META, { - rootDir: currentRoot, + target: { kind: "agents", agents: CODEV_ONLY, scope: "current" }, force: false, }); await waitFor(() => @@ -118,13 +131,18 @@ describe("SkillPullApp", () => { .mockResolvedValue(okResult(globalRoot)); const { stdin, lastFrame } = render( - , + , ); await moveToGlobal(stdin, lastFrame); await confirm(stdin, lastFrame, () => spy.mock.calls.length > 0); expect(spy).toHaveBeenCalledWith(META, { - rootDir: globalRoot, + target: { kind: "agents", agents: CODEV_ONLY, scope: "global" }, force: false, }); }); @@ -136,12 +154,113 @@ describe("SkillPullApp", () => { .mockResolvedValue(okResult(join(process.cwd(), ".claude", "skills"))); const { stdin, lastFrame } = render( - , + , ); await confirm(stdin, lastFrame, () => spy.mock.calls.length > 0); expect(spy.mock.calls[0]?.[1]).toMatchObject({ force: true }); }); + // The agent picker. `agents` is left off so the second prompt appears; + // detectCodevTools is stubbed so the pre-check doesn't depend on the machine. + describe("agent picker", () => { + function renderPicker(detected: SkillAgent[] = []) { + vi.spyOn(install, "defaultAgents").mockReturnValue([ + ...detected, + "codev", + ]); + return render(); + } + + test("appears after the location choice, pre-checked with detected agents", async () => { + mockResolve(); + vi.spyOn(install, "installResolvedSkill").mockResolvedValue( + okResult(join(process.cwd(), ".claude", "skills")), + ); + const { stdin, lastFrame } = renderPicker(["claude"]); + + await waitFor(() => { + if (frameText(lastFrame).includes("For which agents?")) return true; + if (inSelect(lastFrame)) stdin.write("\r"); + return false; + }); + + const frame = frameText(lastFrame); + expect(frame).toContain("[x] Claude Code"); + expect(frame).toContain("[x] CoDev Code"); + // Not configured on this machine, so not pre-checked. + expect(frame).toContain("[ ] Codex"); + expect(frame).toContain("[ ] OpenCode"); + }); + + test("space toggles an agent, and enter installs the checked set", async () => { + mockResolve(); + const spy = vi + .spyOn(install, "installResolvedSkill") + .mockResolvedValue(okResult(join(process.cwd(), ".agents", "skills"))); + const { stdin, lastFrame } = renderPicker(); + + await waitFor(() => { + if (frameText(lastFrame).includes("For which agents?")) return true; + if (inSelect(lastFrame)) stdin.write("\r"); + return false; + }); + // Cursor starts on Claude Code; move to Codex and check it. + await waitFor(() => { + if (frameText(lastFrame).includes("[x] Codex")) return true; + if (frameText(lastFrame).includes("❯ [ ] Codex")) stdin.write(" "); + else stdin.write(DOWN); + return false; + }); + await waitFor(() => { + if (spy.mock.calls.length > 0) return true; + stdin.write("\r"); + return false; + }); + + expect(spy.mock.calls[0]?.[1]).toMatchObject({ + target: { kind: "agents", agents: ["codex", "codev"] }, + }); + }); + + // CoDev Code is the flagship: the picker must not let it be turned off. + test("CoDev Code cannot be unchecked", async () => { + mockResolve(); + const spy = vi + .spyOn(install, "installResolvedSkill") + .mockResolvedValue(okResult(join(process.cwd(), ".claude", "skills"))); + const { stdin, lastFrame } = renderPicker(); + + await waitFor(() => { + if (frameText(lastFrame).includes("For which agents?")) return true; + if (inSelect(lastFrame)) stdin.write("\r"); + return false; + }); + // Walk to CoDev Code and press space repeatedly — it stays checked. + await waitFor(() => { + if (frameText(lastFrame).includes("❯ [x] CoDev Code")) return true; + stdin.write(DOWN); + return false; + }); + stdin.write(" "); + stdin.write(" "); + expect(frameText(lastFrame)).toContain("[x] CoDev Code"); + + await waitFor(() => { + if (spy.mock.calls.length > 0) return true; + stdin.write("\r"); + return false; + }); + expect(spy.mock.calls[0]?.[1]).toMatchObject({ + target: { agents: ["codev"] }, + }); + }); + }); + // A terminal with no raw mode (Git Bash on Windows — see lib/tty.ts). The // dispatcher normally routes those to the plain runner, so this covers the // case where Ink's stdin isn't the process's own. Unlike an ungated useInput diff --git a/tests/lib/skill-dirs.test.ts b/tests/lib/skill-dirs.test.ts new file mode 100644 index 0000000..094a636 --- /dev/null +++ b/tests/lib/skill-dirs.test.ts @@ -0,0 +1,215 @@ +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + readlinkSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { isAbsolute, join } from "node:path"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import * as npm from "@/lib/npm.js"; +import { + agentsServedBy, + claudeFollowsSymlinks, + isClaudeLink, + linkApi, + linkOrCopy, + resolveTargets, + type SkillAgent, + safeSegment, +} from "@/lib/skill-dirs.js"; + +let tempHome: string; +let tempCwd: string; + +const CLAUDE = join(".claude", "skills"); +const AGENTS = join(".agents", "skills"); + +beforeEach(() => { + tempHome = mkdtempSync(join(tmpdir(), "codev-skill-dirs-home-")); + tempCwd = mkdtempSync(join(tmpdir(), "codev-skill-dirs-cwd-")); + // homedir() reads USERPROFILE on Windows, HOME on POSIX. Stub both so tests + // never touch the real home directory. + vi.stubEnv("HOME", tempHome); + vi.stubEnv("USERPROFILE", tempHome); + vi.spyOn(process, "cwd").mockReturnValue(tempCwd); +}); +afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + rmSync(tempHome, { recursive: true, force: true }); + rmSync(tempCwd, { recursive: true, force: true }); +}); + +// Which directory each agent reads was established from the agents' own source +// (see the header of lib/skill-dirs.ts). These pin the consequence: whichever +// directory covers the most selected agents becomes the single real copy. +describe("resolveTargets", () => { + test("without Codex, .claude/skills alone serves every agent", () => { + for (const agents of [ + ["codev"], + ["codev", "claude"], + ["codev", "opencode"], + ["claude", "codev", "opencode"], + ] as SkillAgent[][]) { + const t = resolveTargets(agents, "global", "pg-tuner"); + expect(t.store).toBe(join(tempHome, CLAUDE, "pg-tuner")); + // One directory, so CoDev Code and OpenCode never see the skill twice. + expect(t.links).toEqual([]); + } + }); + + test("Codex without Claude Code needs only .agents/skills", () => { + const t = resolveTargets(["codev", "codex"], "global", "pg-tuner"); + expect(t.store).toBe(join(tempHome, AGENTS, "pg-tuner")); + expect(t.links).toEqual([]); + }); + + test("Codex plus Claude Code is the only case needing two directories", () => { + const t = resolveTargets( + ["claude", "codex", "codev"], + "global", + "pg-tuner", + ); + expect(t.store).toBe(join(tempHome, AGENTS, "pg-tuner")); + expect(t.links).toEqual([join(tempHome, CLAUDE, "pg-tuner")]); + }); + + test("scope only changes the root — the rule is identical", () => { + const global = resolveTargets(["claude", "codex"], "global", "x"); + const current = resolveTargets(["claude", "codex"], "current", "x"); + expect(global.store).toBe(join(tempHome, AGENTS, "x")); + expect(current.store).toBe(join(tempCwd, AGENTS, "x")); + expect(current.links).toEqual([join(tempCwd, CLAUDE, "x")]); + }); +}); + +describe("agentsServedBy", () => { + test("reports every selected agent that reads a given directory", () => { + const agents: SkillAgent[] = ["claude", "codex", "codev"]; + const store = join(tempHome, AGENTS, "x"); + const link = join(tempHome, CLAUDE, "x"); + // .agents/skills is read by Codex, CoDev Code and OpenCode — not Claude. + expect(agentsServedBy(store, agents, "global", "x")).toEqual([ + "codex", + "codev", + ]); + // .claude/skills is read by Claude Code, CoDev Code and OpenCode. + expect(agentsServedBy(link, agents, "global", "x")).toEqual([ + "claude", + "codev", + ]); + }); +}); + +describe("linkOrCopy", () => { + function makeStore(): string { + const store = join(tempHome, AGENTS, "pg-tuner"); + mkdirSync(store, { recursive: true }); + writeFileSync(join(store, "SKILL.md"), "# pg-tuner"); + return store; + } + + test("creates a relative symlink that resolves to the store", async () => { + const store = makeStore(); + const link = join(tempHome, CLAUDE, "pg-tuner"); + + const mode = await linkOrCopy(store, link); + + expect(mode).toBe("symlink"); + expect(lstatSync(link).isSymbolicLink()).toBe(true); + // Relative, not absolute — this repo's own skill is wired the same way and + // committed as git mode 120000, which only survives a clone if relative. + const target = readlinkSync(link); + expect(isAbsolute(target)).toBe(false); + expect(target).toContain(".."); + // And it actually resolves. + expect(readFileSync(join(link, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + }); + + test("falls back to a copy when symlinking fails, and says so", async () => { + const store = makeStore(); + const link = join(tempHome, CLAUDE, "pg-tuner"); + vi.spyOn(linkApi, "symlink").mockRejectedValue( + Object.assign(new Error("operation not permitted"), { code: "EPERM" }), + ); + + const mode = await linkOrCopy(store, link); + + // Reported as a copy — never described as a link it isn't. + expect(mode).toBe("copy"); + expect(lstatSync(link).isSymbolicLink()).toBe(false); + expect(readFileSync(join(link, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + }); + + test("creates the parent directory when the agent has never been used", async () => { + const store = makeStore(); + const link = join(tempHome, CLAUDE, "pg-tuner"); + expect(existsSync(join(tempHome, CLAUDE))).toBe(false); + + await linkOrCopy(store, link); + + expect(existsSync(link)).toBe(true); + }); +}); + +describe("claudeFollowsSymlinks", () => { + function version(stdout: string) { + return vi + .spyOn(npm, "execAsync") + .mockResolvedValue({ error: null, stdout, stderr: "" }); + } + + test("true at and above the 2.1.203 floor", async () => { + version("2.1.203 (Claude Code)"); + expect(await claudeFollowsSymlinks()).toBe(true); + version("2.1.220 (Claude Code)"); + expect(await claudeFollowsSymlinks()).toBe(true); + version("3.0.0 (Claude Code)"); + expect(await claudeFollowsSymlinks()).toBe(true); + }); + + test("false below it, so the Claude link becomes a real copy", async () => { + version("2.1.202 (Claude Code)"); + expect(await claudeFollowsSymlinks()).toBe(false); + version("2.0.999 (Claude Code)"); + expect(await claudeFollowsSymlinks()).toBe(false); + }); + + test("false when claude is absent or unparseable", async () => { + vi.spyOn(npm, "execAsync").mockResolvedValue({ + error: Object.assign(new Error("ENOENT"), { code: "ENOENT" }), + stdout: "", + stderr: "", + }); + expect(await claudeFollowsSymlinks()).toBe(false); + + version("some unexpected banner"); + expect(await claudeFollowsSymlinks()).toBe(false); + }); +}); + +describe("isClaudeLink", () => { + test("identifies the one link whose mechanism depends on a version", () => { + expect(isClaudeLink(join(tempHome, CLAUDE, "x"), "global", "x")).toBe(true); + expect(isClaudeLink(join(tempHome, AGENTS, "x"), "global", "x")).toBe( + false, + ); + }); +}); + +describe("safeSegment", () => { + test("rejects anything that would escape a skills directory", () => { + expect(safeSegment("pg-tuner")).toBe(true); + expect(safeSegment("..")).toBe(false); + expect(safeSegment("../evil")).toBe(false); + expect(safeSegment("nested/name")).toBe(false); + expect(safeSegment("")).toBe(false); + // A leading dot would collide with the dot-directories the agents scan. + expect(safeSegment(".hidden")).toBe(false); + }); +}); diff --git a/tests/lib/skill-install.test.ts b/tests/lib/skill-install.test.ts index dc89e49..fb43277 100644 --- a/tests/lib/skill-install.test.ts +++ b/tests/lib/skill-install.test.ts @@ -1,19 +1,17 @@ import { existsSync, + lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, } from "node:fs"; -import { homedir, tmpdir } from "node:os"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import AdmZip from "adm-zip"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { - parsePullArgs, - runSkillInstall, - skillsDirFor, -} from "@/lib/skill-install.js"; +import * as npm from "@/lib/npm.js"; +import { parsePullArgs, runSkillInstall } from "@/lib/skill-install.js"; import * as skillhub from "@/lib/skillhub.js"; let tempDir: string; @@ -56,6 +54,14 @@ function captureErr() { beforeEach(() => { tempDir = mkdtempSync(join(tmpdir(), "codev-skill-install-")); + // The Claude link's mechanism depends on `claude --version`. Pin it above the + // symlink floor so these tests don't spawn a real process and don't change + // behavior on a machine where Claude Code is absent or old. + vi.spyOn(npm, "execAsync").mockResolvedValue({ + error: null, + stdout: "2.1.220 (Claude Code)", + stderr: "", + }); }); afterEach(() => { vi.restoreAllMocks(); @@ -182,9 +188,9 @@ describe("runSkillInstall", () => { expect(errs.join("\n")).toMatch(/--here, --global, or --dir/i); }); - // --dir is an exact path (`/`), while --here reproduces the - // picker's "Current directory" choice — which is the layout agents read. - test("--dir installs verbatim; --here adds the .claude/skills segment", async () => { + // --dir is an exact path (`/`), while --here resolves the selected + // agents' directories under the cwd — the layout the agents actually read. + test("--dir installs verbatim; --here resolves agent directories", async () => { mockMeta(); mockDownload(); vi.spyOn(process, "cwd").mockReturnValue(tempDir); @@ -192,21 +198,165 @@ describe("runSkillInstall", () => { expect(await runSkillInstall(["pg-tuner", "--dir", tempDir])).toBe(0); expect(existsSync(join(tempDir, "pg-tuner", "SKILL.md"))).toBe(true); - expect(await runSkillInstall(["pg-tuner", "--here"])).toBe(0); + // CoDev Code alone reads .claude/skills, so that is the whole install. + expect( + await runSkillInstall(["pg-tuner", "--here", "--agent", "codev"]), + ).toBe(0); expect( existsSync(join(tempDir, ".claude", "skills", "pg-tuner", "SKILL.md")), ).toBe(true); }); - test("--global resolves to the home skills dir", () => { - // Asserted through the pure helpers so the test never writes to a real home. - expect(parsePullArgs(["pg-tuner", "--global"]).location).toBe("global"); - expect(skillsDirFor("global")).toBe(join(homedir(), ".claude", "skills")); - expect(skillsDirFor("current")).toBe( - join(process.cwd(), ".claude", "skills"), + // The one arrangement that needs two directories: Codex reads only + // .agents/skills, Claude Code only .claude/skills. + test("--agent claude,codex extracts once and links the second directory", async () => { + mockMeta(); + const dl = mockDownload(); + vi.spyOn(process, "cwd").mockReturnValue(tempDir); + + expect( + await runSkillInstall(["pg-tuner", "--here", "--agent", "claude,codex"]), + ).toBe(0); + + const store = join(tempDir, ".agents", "skills", "pg-tuner"); + const link = join(tempDir, ".claude", "skills", "pg-tuner"); + expect(readFileSync(join(store, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + // Downloaded once, extracted once — the second path is a link to the first. + expect(dl).toHaveBeenCalledTimes(1); + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(readFileSync(join(link, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + }); + + // Claude Code only follows a symlinked skill dir from v2.1.203; below that the + // link has to be a real copy or the skill is simply invisible to it. + test("an older Claude Code gets a copy instead of a link", async () => { + mockMeta(); + mockDownload(); + vi.spyOn(process, "cwd").mockReturnValue(tempDir); + vi.spyOn(npm, "execAsync").mockResolvedValue({ + error: null, + stdout: "2.1.202 (Claude Code)", + stderr: "", + }); + const out = captureLog(); + + expect( + await runSkillInstall(["pg-tuner", "--here", "--agent", "claude,codex"]), + ).toBe(0); + + const link = join(tempDir, ".claude", "skills", "pg-tuner"); + expect(lstatSync(link).isSymbolicLink()).toBe(false); + expect(readFileSync(join(link, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + // And it says so rather than claiming a link. + expect(out.join("\n")).toContain("(copy)"); + }); + + test("--all-agents installs for every agent", async () => { + mockMeta(); + mockDownload(); + vi.spyOn(process, "cwd").mockReturnValue(tempDir); + const out = captureLog(); + + expect(await runSkillInstall(["pg-tuner", "--here", "--all-agents"])).toBe( + 0, + ); + + const text = out.join("\n"); + for (const label of ["Claude Code", "Codex", "OpenCode", "CoDev Code"]) { + expect(text).toContain(label); + } + }); + + // The upgrade path: a skill installed before agent support is a real directory + // in .claude/skills. Re-pulling with Codex selected has to convert it to a + // link rather than leave a stale second copy behind. + test("--force converts a pre-existing real directory into a link", async () => { + mockMeta(); + mockDownload(); + vi.spyOn(process, "cwd").mockReturnValue(tempDir); + const stale = join(tempDir, ".claude", "skills", "pg-tuner"); + mkdirSync(stale, { recursive: true }); + captureLog(); + + expect( + await runSkillInstall([ + "pg-tuner", + "--here", + "--agent", + "claude,codex", + "--force", + ]), + ).toBe(0); + + expect(lstatSync(stale).isSymbolicLink()).toBe(true); + expect(readFileSync(join(stale, "SKILL.md"), "utf-8")).toBe("# pg-tuner"); + }); + + test("without --force an existing agent directory stops the install", async () => { + mockMeta(); + mockDownload(); + vi.spyOn(process, "cwd").mockReturnValue(tempDir); + mkdirSync(join(tempDir, ".claude", "skills", "pg-tuner"), { + recursive: true, + }); + const errs = captureErr(); + + const code = await runSkillInstall([ + "pg-tuner", + "--here", + "--agent", + "claude,codex", + ]); + + expect(code).toBe(1); + expect(errs.join("\n")).toMatch(/Already installed/); + // Refused before anything was removed — the store was never created. + expect(existsSync(join(tempDir, ".agents", "skills", "pg-tuner"))).toBe( + false, ); }); + test("rejects an unknown agent name", async () => { + const getMeta = vi.spyOn(skillhub, "getSkillMeta"); + const errs = captureErr(); + + const code = await runSkillInstall([ + "pg-tuner", + "--here", + "--agent", + "cursor", + ]); + + expect(code).toBe(1); + expect(getMeta).not.toHaveBeenCalled(); + expect(errs.join("\n")).toContain("Unknown agent: cursor"); + }); + + // CoDev Code is the flagship: naming other agents never drops it. + test("CoDev Code is always in the agent set", () => { + expect(parsePullArgs(["x", "--agent", "claude"]).agents).toEqual([ + "claude", + "codev", + ]); + expect(parsePullArgs(["x", "--agent", "codex"]).agents).toEqual([ + "codex", + "codev", + ]); + }); + + test("--dir cannot be combined with --agent", async () => { + const errs = captureErr(); + const code = await runSkillInstall([ + "pg-tuner", + "--dir", + tempDir, + "--agent", + "claude", + ]); + expect(code).toBe(1); + expect(errs.join("\n")).toMatch(/exact path/i); + }); + // A mistyped flag must fail loudly. Silently ignoring `--forse` would look // like a successful run that quietly refused to overwrite. test("rejects an unknown flag instead of ignoring it", async () => { From 58f62cc7a777cd9b400d85363e438e57cd57c7da Mon Sep 17 00:00:00 2001 From: minhn4 Date: Thu, 30 Jul 2026 23:37:45 +0700 Subject: [PATCH 2/2] feat(skill pull): use a check mark in the agent picker checkboxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[x]` reads as "removed" or "wrong" more than "selected". `[✓]` keeps the checkbox affordance the brackets give while matching the ✓ the CLI already uses for a completed Step. Same single-column width, so the rows stay aligned. Co-Authored-By: Claude Opus 5 (1M context) --- src/SkillPullApp.tsx | 2 +- tests/SkillPullApp.test.tsx | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/SkillPullApp.tsx b/src/SkillPullApp.tsx index b58caab..fde8b2f 100644 --- a/src/SkillPullApp.tsx +++ b/src/SkillPullApp.tsx @@ -223,7 +223,7 @@ export function SkillPullApp({ color={i === agentIndex ? "cyan" : undefined} dimColor={locked} > - {`${i === agentIndex ? "❯ " : " "}[${checked ? "x" : " "}] ${AGENT_LABELS[agent]}`} + {`${i === agentIndex ? "❯ " : " "}[${checked ? "✓" : " "}] ${AGENT_LABELS[agent]}`} ); })} diff --git a/tests/SkillPullApp.test.tsx b/tests/SkillPullApp.test.tsx index 3018188..6acf1c5 100644 --- a/tests/SkillPullApp.test.tsx +++ b/tests/SkillPullApp.test.tsx @@ -190,8 +190,8 @@ describe("SkillPullApp", () => { }); const frame = frameText(lastFrame); - expect(frame).toContain("[x] Claude Code"); - expect(frame).toContain("[x] CoDev Code"); + expect(frame).toContain("[✓] Claude Code"); + expect(frame).toContain("[✓] CoDev Code"); // Not configured on this machine, so not pre-checked. expect(frame).toContain("[ ] Codex"); expect(frame).toContain("[ ] OpenCode"); @@ -211,7 +211,7 @@ describe("SkillPullApp", () => { }); // Cursor starts on Claude Code; move to Codex and check it. await waitFor(() => { - if (frameText(lastFrame).includes("[x] Codex")) return true; + if (frameText(lastFrame).includes("[✓] Codex")) return true; if (frameText(lastFrame).includes("❯ [ ] Codex")) stdin.write(" "); else stdin.write(DOWN); return false; @@ -242,13 +242,13 @@ describe("SkillPullApp", () => { }); // Walk to CoDev Code and press space repeatedly — it stays checked. await waitFor(() => { - if (frameText(lastFrame).includes("❯ [x] CoDev Code")) return true; + if (frameText(lastFrame).includes("❯ [✓] CoDev Code")) return true; stdin.write(DOWN); return false; }); stdin.write(" "); stdin.write(" "); - expect(frameText(lastFrame)).toContain("[x] CoDev Code"); + expect(frameText(lastFrame)).toContain("[✓] CoDev Code"); await waitFor(() => { if (spy.mock.calls.length > 0) return true;