diff --git a/docs/cli-reference.md b/docs/cli-reference.md index ca82708..ae0b840 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -209,6 +209,114 @@ skillx import --dry-run --- +### `skillx add ` + +Install skills straight from GitHub, an https git URL or a local directory. +`add` is a thin front end to the [installer](#installer): skills land in the +same runtime skills directory (`--runtime`, default `claude-code`) with the same +receipts, install lock, conflicts and backups as `skillx install`. Unlike a +registry release, a skill added this way is **not scanned, signed or pinned in +`skillx.lock.json`**; review it before use. + +**Sources:** +| Source | Meaning | +|--------|---------| +| `owner/repo` | A GitHub repository (default branch) | +| `owner/repo/skill` | One skill of it, by name or directory name (same as `--skill skill`) | +| `github:owner/repo[/path][@ref]` | A directory of the repository, at a branch or tag | +| `https://github.com/owner/repo[/tree/[/]]` | Read as `github:` | +| `https://host/repo.git` | Any https git repository (default branch) | +| `./dir`, `../dir`, an absolute path | A local directory | + +Only `https` is accepted. `http`, `ssh`, `git@host:` (scp style), `file://`, +`ext::` and other `::` transport helpers, a URL with a query or fragment, and +anything starting with `-` are refused as `usage_error`; git is also run with +every other protocol disabled. A path or ref with `..`, `.`, empty, `\` or `:` +segments is refused. `@publisher/package`, `kit:` and `collection:` belong to +`skillx install`; `@agentkit/*` is reserved. + +**Options:** +| Flag | Description | Default | +|------|-------------|---------| +| `-s, --skill ` | Skills to install, by front matter name or directory name (`*` for all) | ask; the only skill; all with `-y` | +| `-l, --list` | List the skills in the source; install nothing | `false` | +| `-y, --yes` | Do not ask (no selection prompt, no confirmation); required to install in human mode when stdin is not a terminal | `false` | +| `--force` | Replace a skill added before, or install over a directory nothing owns (edited files are backed up, unknown files kept) | `false` | +| `--runtime ` | `claude-code` or `codex` | `claude-code` | +| `--json` | Envelope with `status` (`listed`, `installed` or `cancelled`); never asks, so several skills need `--skill` | `false` | + +**How a skill is found and read.** A skill is a directory with a regular +`SKILL.md` whose front matter (between a first `---` line and the next `---` +line) is plain YAML with a non-empty `name` and `description`. The front matter +is parsed as data only (YAML 1.2 core schema): a `---js`/`---javascript` fence +is not front matter, and tags or duplicate keys reject the file, so nothing in +a source ever runs. When the source (or `github:` path) is itself a skill, it +is the only one; otherwise every skill up to 6 directories deep counts, +outside `.git`, other VCS and cache directories and `node_modules`, and a +skill's own subdirectories are its content. Skills that cannot be installed are +listed under `skipped` with the reason. + +- **Names.** `name` is the installed directory name and is never rewritten: + it must be a valid alias (up to 64 lowercase letters, digits, `.`, `_`, `-`, + not ending in `.`, not a Windows device name). Names starting with `ak-` or `ak:` are reserved for + AgentKit and refused. Naming a skipped skill with `--skill` is a + `usage_error`; two chosen skills with one name are a `usage_error`. +- **Content.** Each chosen skill is checked with the release tree rules + (`agentkit.skill-tree/v1`): a symlink, junction, hardlink, git submodule or + other special entry, an unsafe or colliding path, or a tree over the release + limits (4,096 files, 32 MiB per file, 256 MiB in all) refuses the skill as + `incompatible`. Links are never followed or copied, neither while searching + nor while reading. +- **Git.** The clone is shallow, single-branch and without a checkout, into a + temporary directory removed afterwards: files are read from git objects, so + no repository file is written, and no hook, filter or LFS download runs. git + runs without a shell, with `--` before the URL and all credential prompts + off (`core.askPass=`, `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=never`), so + a private repository works only with credentials git already has (a + credential helper, or a token in the URL). +- **Credentials.** A token in the URL is used to clone and nothing else: it is + removed from the recorded source and masked (`https://***@host`) in every + message and envelope. + +**Where it is recorded.** Each added skill is its own installer source, +`add:`, recorded with the source it came from (no credentials): its +receipt's `package` is the skill's origin (for example +`github:owner/repo/skills/pdf`) and `version` the commit read, or `local`. +`skillx list` shows it, and `skillx uninstall add:` removes it with +ownership proof. `skillx update` skips added skills (naming one is a +`usage_error`); refresh one with `skillx add --force`. `sync --locked` +does not restore them. + +**Overwrites.** Nothing installed is replaced without `--force`: adding a name +that `add` installed before from a different tree is a `conflict`, and so is a +directory no source owns (with `--force` its unknown files are kept). A skill +that another source (a registry package, kit or collection) installed at +other content is a `conflict` even with `--force`; the same content just adds +`add:` as a second owner. + +**Exit codes.** `0` installed, listed, or cancelled by an empty answer at a +prompt; `2` `usage_error` (bad source, refused transport or path, reserved or +invalid name, several skills under `--json` without `--skill`, or stdin not a +terminal without `--yes` or `--json`: pass `--yes`, with `--skill` to choose, +since human mode cannot ask); `3` `not_found` (no skill, +no such `--skill`, path, repository or branch); `4` `incompatible` (a link, +special file or limit in a chosen skill); `5` `auth_error` (git asked for +credentials); `6` `network_error` (clone failed or timed out after 120 s); +`8` `conflict` (see Overwrites). Nothing is installed when any chosen skill +fails. + +**Examples:** +```bash +skillx add anthropics/skills --list +skillx add anthropics/skills/pdf +skillx add github:acme/agent-skills/skills@v1.2 --skill lint review --yes +skillx add https://git.example.com/team/skills.git --skill '*' --json +skillx add ./my-skills --force +skillx uninstall add:pdf +``` + +--- + ### Release commands: `validate`, `pack`, `publish`, `promote` These commands work on immutable, digest-pinned releases. Like every other @@ -505,7 +613,10 @@ is sent only over `https`; plain `http` is refused except for `localhost`, - **update** re-resolves installed sources (all, or those named) and applies the difference, then refreshes the lock file when its scope was updated. With nothing installed it prints `Nothing to update.` (`--json`: empty `outcomes`). -- **uninstall** drops a kit, a collection, or one or more packages; each + Skills installed by [`skillx add`](#skillx-add-source) (`add:`) are + skipped; naming one is a `usage_error`. +- **uninstall** drops a kit, a collection, one or more packages, or skills + installed by `skillx add` (`add:`, named on their own); each explicit package is its own source (`packages:@scope/name`), whether it was installed alone, with others, or by `sync --locked` from a packages lock. Each package is dropped from a packages lock file as soon as it is diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 4dedc59..c671f1d 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -3,7 +3,7 @@ import { execFile, execFileSync, execSync, spawn } from "node:child_process"; import { createHash } from "node:crypto"; import { createServer } from "node:http"; import type { AddressInfo } from "node:net"; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -357,6 +357,56 @@ describe("built cli", { timeout: 30_000 }, () => { return (await Promise.all(names.map((name) => leafCommands([...path, name])))).flat(); } + it("add installs a local skill through the installer as one envelope, with installer exit codes", async () => { + const home = join(root, "add-home"); + const env = { ...childEnv, HOME: home, USERPROFILE: home, SKILLX_HOME: join(home, ".skillx"), CLAUDE_CONFIG_DIR: join(home, ".claude") }; + const run = async (...args: string[]) => { + try { + return { code: 0, stdout: (await execFileAsync(process.execPath, [BIN, ...args], { cwd: root, env, encoding: "utf8" })).stdout }; + } catch (error) { + const failed = error as { code?: number; stdout?: string }; + return { code: failed.code ?? -1, stdout: failed.stdout ?? "" }; + } + }; + const src = join(root, "add-src"); + mkdirSync(join(src, "references"), { recursive: true }); + writeFileSync(join(src, "SKILL.md"), "---\nname: smoke-skill\ndescription: Built CLI smoke\n---\n# Smoke\n"); + writeFileSync(join(src, "references", "notes.md"), "notes\n"); + const listed = JSON.parse((await run("add", src, "--list", "--json")).stdout) as { ok: true; data: { status: string; skills: Array<{ name: string }> } }; + expect(listed.data).toMatchObject({ status: "listed", skills: [{ name: "smoke-skill" }] }); + + const installed = await run("add", src, "--json"); + expect(installed.code).toBe(0); + expect(JSON.parse(installed.stdout)).toMatchObject({ ok: true, data: { status: "installed", skills: [{ alias: "smoke-skill" }] } }); + expect(readFileSync(join(home, ".claude", "skills", "smoke-skill", "references", "notes.md"), "utf8")).toBe("notes\n"); + const owners = JSON.parse((await run("list", "--json")).stdout) as { data: { sources: Array<{ owner: string }> } }; + expect(owners.data.sources.map((s) => s.owner)).toEqual(["add:smoke-skill"]); + + // The same name from a changed tree is a conflict (exit 8) until --force. + writeFileSync(join(src, "SKILL.md"), "---\nname: smoke-skill\ndescription: Built CLI smoke v2\n---\n"); + const conflict = await run("add", src, "--json"); + expect(conflict.code).toBe(8); + expect(JSON.parse(conflict.stdout)).toMatchObject({ ok: false, error: { code: "conflict" } }); + expect((await run("add", src, "--force", "--json")).code).toBe(0); + expect((await run("uninstall", "add:smoke-skill", "--json")).code).toBe(0); + + // Human mode on a piped stdin cannot ask: a usage error, never a silent exit-0 cancel. + const piped = await run("add", src); + expect(piped.code).toBe(2); + expect(piped.stdout).not.toContain("Cancelled"); + expect(existsSync(join(home, ".claude", "skills", "smoke-skill"))).toBe(false); + + const missing = await run("add", join(root, "no-such-dir"), "--json"); + expect(missing.code).toBe(2); + expect(JSON.parse(missing.stdout)).toMatchObject({ ok: false, error: { code: "usage_error" } }); + // Human mode fails and exits too; nothing keeps the process alive. + expect((await run("add", "ext::sh -c id")).code).toBe(2); + const token = "ghp_SECRETTOKEN123"; + const leaked = await run("add", `http://user:${token}@127.0.0.1:9/x.git`, "--json"); + expect(leaked.code).toBe(2); + expect(leaked.stdout).not.toContain(token); + }); + it("offers --json on every command but the stdio MCP server", async () => { const leaves = await leafCommands(); expect(leaves.length).toBeGreaterThan(20); diff --git a/packages/cli/src/commands/add-display.ts b/packages/cli/src/commands/add-display.ts new file mode 100644 index 0000000..e3e6427 --- /dev/null +++ b/packages/cli/src/commands/add-display.ts @@ -0,0 +1,53 @@ +import chalk from 'chalk'; +import type { ApplyResult } from '../lib/install/install-engine.js'; +import type { FoundSkill, SkippedSkill } from '../lib/add/skill-discovery.js'; + +/** What `skillx add` reports; the same data backs the `--json` envelope. */ +export type AddResult = + | { status: 'cancelled' } + | { status: 'listed'; source: string; commit?: string; skills: FoundSkill[]; skipped: SkippedSkill[] } + | { + status: 'installed'; + source: string; + commit?: string; + runtime: string; + skillsDir: string; + skills: Array<{ alias: string; origin: string; digest: string }>; + skipped: SkippedSkill[]; + planDigest: string; + applied: ApplyResult[]; + recovered: { rolledBack: string[]; rolledForward: string[] }; + }; + +/** Refused skills go to stderr, so piped stdout stays clean. */ +export function printSkipped(skipped: readonly SkippedSkill[]): void { + for (const s of skipped) console.error(chalk.yellow(` ! skipped ${s.dir || '(root)'}: ${s.reason}`)); +} + +export function printAdd(result: AddResult): void { + if (result.status === 'cancelled') { + console.log(chalk.dim('Cancelled; nothing was installed.')); + return; + } + const at = result.commit ? chalk.dim(` @ ${result.commit.slice(0, 12)}`) : ''; + if (result.status === 'listed') { + console.log(chalk.bold(`\n${result.source}`) + at); + for (const s of result.skills) console.log(` ${chalk.cyan(s.name.padEnd(28))} ${chalk.dim(s.dir || '.')} ${s.description.slice(0, 80)}`); + if (result.skills.length === 0) console.log(chalk.dim(' no installable skill')); + printSkipped(result.skipped); + return; + } + console.log(chalk.bold(`\n${result.source}`) + at + chalk.dim(` → ${result.skillsDir}`)); + for (const a of result.applied) { + for (const c of a.changed) { + console.log(` ${chalk.green(c.action.padEnd(9))} ${c.alias}`); + if (c.backupDir) console.log(chalk.yellow(` local changes saved to ${c.backupDir}`)); + if (c.preserved.length > 0) console.log(chalk.yellow(` kept files not in the skill: ${c.preserved.slice(0, 5).join(', ')}`)); + // Stderr, like the installer: an executable file is reported, never run. + if (c.executable) console.error(chalk.yellow(` ! ${c.alias}: ${c.executable.length} executable file(s) written (${c.executable.slice(0, 5).join(', ')}); skillx never runs them, review before use`)); + } + if (a.changed.length === 0) console.log(` ${chalk.dim('unchanged'.padEnd(9))} ${a.owner.slice(a.owner.indexOf(':') + 1)}`); + } + printSkipped(result.skipped); + console.log(chalk.green('\n✓ done') + chalk.dim(' Skills from git or a local path are not scanned or signed; review them before use.')); +} diff --git a/packages/cli/src/commands/add-prompts.ts b/packages/cli/src/commands/add-prompts.ts new file mode 100644 index 0000000..f96c76e --- /dev/null +++ b/packages/cli/src/commands/add-prompts.ts @@ -0,0 +1,85 @@ +import chalk from 'chalk'; +import { InstallError, usageError } from '../lib/install/install-errors.js'; +import type { FoundSkill, SkippedSkill } from '../lib/add/skill-discovery.js'; +import { promptLine } from '../lib/prompt-line.js'; + +/** + * Which skills `skillx add` installs. Names match a skill's front matter name + * or its directory name, case-insensitively; `*` takes every installable + * skill. A name that matches a refused skill (reserved, invalid front matter) + * fails with the reason instead of being skipped quietly. Without names, one + * skill is taken as is; several are all taken with `--yes`, chosen at a + * prompt in human mode, and must be named under `--json`. + */ + +const baseName = (dir: string) => dir.slice(dir.lastIndexOf('/') + 1); +const matches = (name: string, skill: { name?: string; dir: string }) => { + const n = name.toLowerCase(); + return skill.name?.toLowerCase() === n || (skill.dir !== '' && baseName(skill.dir).toLowerCase() === n); +}; + +export function pickByName(found: readonly FoundSkill[], skipped: readonly SkippedSkill[], names: readonly string[]): FoundSkill[] { + const picked = new Set(); + for (const name of names) { + const hits = found.filter((s) => matches(name, s)); + if (hits.length > 0) { + hits.forEach((s) => picked.add(s)); + continue; + } + const refused = skipped.find((s) => matches(name, s)); + if (refused) throw usageError(`cannot install ${name}: ${refused.reason}`, { skill: name, dir: refused.dir }); + throw new InstallError('not_found', `no skill named ${name} in this source`, { skill: name, available: found.map((s) => s.name) }); + } + return [...picked]; +} + +/** Parses `1,3`, `2-4` or `all` against a list of `count` items; null when the answer is not one of those. */ +export function parseChoice(answer: string, count: number): number[] | null { + const text = answer.trim().toLowerCase(); + if (text === 'all' || text === '*') return Array.from({ length: count }, (_, i) => i); + const picked = new Set(); + for (const part of text.split(/[\s,]+/).filter(Boolean)) { + const m = /^(\d+)(?:-(\d+))?$/.exec(part); + if (!m) return null; + const [from, to] = [Number(m[1]), Number(m[2] ?? m[1])]; + if (from < 1 || to > count || from > to) return null; + for (let i = from; i <= to; i++) picked.add(i - 1); + } + return picked.size > 0 ? [...picked].sort((a, b) => a - b) : null; +} + +/** + * Human mode asks before it installs, so it needs a terminal. A pipe, a closed + * stdin or CI is a usage error, never a silent "cancelled" with exit 0. + */ +export function assertCanPrompt(mode: { yes?: boolean; json?: boolean }, isTTY = process.stdin.isTTY === true): void { + if (mode.yes || mode.json || isTTY) return; + throw usageError('stdin is not a terminal, so skillx add cannot ask; pass --yes to install without a prompt (with --skill to choose skills)'); +} + +export async function selectSkills( + found: readonly FoundSkill[], + skipped: readonly SkippedSkill[], + names: readonly string[], + mode: { yes?: boolean; json?: boolean }, +): Promise { + if (names.length > 0 && !names.includes('*')) return pickByName(found, skipped, names); + if (found.length === 0) { + throw new InstallError('not_found', 'no installable skill found (a skill is a directory with a SKILL.md that has a name and a description)', { skipped }); + } + if (names.length > 0 || found.length === 1 || mode.yes) return [...found]; + if (mode.json) throw usageError(`${found.length} skills found; name them with --skill or --skill '*'`, { available: found.map((s) => s.name) }); + found.forEach((s, i) => console.log(` ${chalk.dim(String(i + 1).padStart(2))} ${chalk.cyan(s.name)} ${chalk.dim(s.description.slice(0, 70))}`)); + for (;;) { + const answer = await promptLine('Install which skills? (e.g. 1,3 or 2-4 or all; empty to cancel) ', process.stdout); + if (answer === null || answer.trim() === '') return null; + const picked = parseChoice(answer, found.length); + if (picked) return picked.map((i) => found[i]!); + console.log(chalk.yellow(` answer with numbers from 1 to ${found.length}, ranges, or all`)); + } +} + +export async function confirmInstall(): Promise { + const answer = await promptLine('Install? [y/N] ', process.stdout); + return answer !== null && /^y(es)?$/i.test(answer.trim()); +} diff --git a/packages/cli/src/commands/add.ts b/packages/cli/src/commands/add.ts new file mode 100644 index 0000000..e5a339e --- /dev/null +++ b/packages/cli/src/commands/add.ts @@ -0,0 +1,117 @@ +import { join } from 'node:path'; +import { Command } from 'commander'; +import chalk from 'chalk'; +import { applyAdd, planAdd } from '../lib/add/add-install.js'; +import { parseAddSource, redactCredentials, searchPathOf } from '../lib/add/add-source.js'; +import type { AddSource } from '../lib/add/add-source.js'; +import { openGitSource } from '../lib/add/git-source.js'; +import { openLocalSource } from '../lib/add/local-source.js'; +import { discoverSkills, prepareSkill } from '../lib/add/skill-discovery.js'; +import type { SourceSnapshot } from '../lib/add/skill-discovery.js'; +import { CliError } from '../lib/output.js'; +import { assertCanPrompt, confirmInstall, selectSkills } from './add-prompts.js'; +import { printAdd, printSkipped } from './add-display.js'; +import type { AddResult } from './add-display.js'; +import { contextFrom, printPlan, runInstaller } from './install-runner.js'; +import type { InstallerOptions } from './install-runner.js'; + +/** + * `skillx add `: install skills straight from GitHub, an https git + * URL or a local directory, through the generic installer (same skills + * directory, receipts, owners, install lock, conflicts and backups as + * `skillx install`). Sources are parsed and refused in `lib/add/add-source.ts`, + * read without following links in `git-source.ts`/`local-source.ts`, and each + * chosen skill is checked by the release tree rules before the installer sees + * it. Every failure message is redacted, so a token in a URL never prints. + */ + +export interface AddOptions extends InstallerOptions { + skill?: string[]; + list?: boolean; + yes?: boolean; +} + +export function openSource(source: AddSource): SourceSnapshot { + if (source.kind === 'local') return openLocalSource(source.dir); + return openGitSource(source.cloneUrl, source.label, source.kind === 'github' ? source.ref : undefined); +} + +/** Where a skill came from: the source plus the skill's directory in it, never credentials. */ +export function originOf(source: AddSource, dir: string): string { + if (source.kind === 'local') return dir === '' ? source.dir : join(source.dir, ...dir.split('/')); + if (source.kind === 'github') return `github:${source.owner}/${source.repo}${dir ? `/${dir}` : ''}${source.ref ? `@${source.ref}` : ''}`; + return dir === '' ? source.label : `${source.label}//${dir}`; +} + +export async function runAdd(input: string, options: AddOptions): Promise { + const source = parseAddSource(input); + // Resolve the runtime before any clone, so a bad --runtime fails fast. + const ctx = options.list ? null : contextFrom(options); + if (ctx !== null) assertCanPrompt(options); + if (!options.json && source.kind !== 'local') console.error(chalk.dim(`cloning ${source.label} ...`)); + const snapshot = openSource(source); + try { + const { skills, skipped } = discoverSkills(snapshot, searchPathOf(source)); + const base = { source: source.label, ...(snapshot.commit ? { commit: snapshot.commit } : {}), skipped }; + if (ctx === null) return { status: 'listed', ...base, skills }; + const names = [...(options.skill ?? []), ...(source.kind === 'github' && source.skill ? [source.skill] : [])]; + const chosen = await selectSkills(skills, skipped, names, options); + if (chosen === null) return { status: 'cancelled' }; + const added = chosen.map((s) => ({ alias: s.name, origin: originOf(source, s.dir), tree: prepareSkill(snapshot, s) })); + const info = { ref: source.label, version: snapshot.commit ?? 'local' }; + let expectDigest: string | undefined; + if (!options.json && !options.yes) { + const plan = await planAdd(ctx, info, added); + printPlan(plan); + printSkipped(skipped); + if (!(await confirmInstall())) return { status: 'cancelled' }; + expectDigest = plan.planDigest; + } + const outcome = await applyAdd(ctx, info, added, expectDigest); + return { + status: 'installed', + ...base, + runtime: ctx.runtime, + skillsDir: ctx.skillsDir, + skills: added.map((a) => ({ alias: a.alias, origin: a.origin, digest: a.tree.digest })), + planDigest: outcome.planDigest, + applied: outcome.applied, + recovered: outcome.recovered, + }; + } finally { + snapshot.dispose(); + } +} + +/** The same failure with every URL credential masked in its message, hint and details. */ +export function redactError(error: unknown): unknown { + if (error instanceof CliError) { + const { details, hint } = error.extra; + return new CliError(error.code, redactCredentials(error.message), { + ...(details === undefined ? {} : { details: JSON.parse(redactCredentials(JSON.stringify(details))) as unknown }), + ...(hint ? { hint: redactCredentials(hint) } : {}), + }); + } + if (error instanceof Error) return new Error(redactCredentials(error.message)); + return error; +} + +export const addCommand = new Command('add') + .description('Install skills from GitHub, an https git URL or a local directory through the installer') + .argument('', 'owner/repo[/skill] | github:owner/repo[/path][@ref] | https://host/repo.git | ./local/dir') + .option('-s, --skill ', "skills to install, by name or directory name ('*' for all)") + .option('-l, --list', 'list the skills in the source; install nothing') + .option('-y, --yes', 'do not ask; without --skill, install every skill found') + .option('--force', 'replace a skill added before or an unowned directory (edits backed up, unknown files kept)') + .option('--runtime ', 'target runtime: claude-code or codex', 'claude-code') + .option('--json', 'emit a machine-readable envelope instead of text; never asks') + .action((source: string, options: AddOptions) => + runInstaller( + options, + () => + runAdd(source, options).catch((error: unknown) => { + throw redactError(error); + }), + printAdd, + ), + ); diff --git a/packages/cli/src/commands/install.ts b/packages/cli/src/commands/install.ts index f56dca1..0ea89ea 100644 --- a/packages/cli/src/commands/install.ts +++ b/packages/cli/src/commands/install.ts @@ -94,7 +94,7 @@ export const updateCommand = withCommon(new Command('update')) export const uninstallCommand = withCommon(new Command('uninstall')) .description('Remove a source; skills another source still needs stay, edited and unknown files are kept') - .argument('', 'the source as installed: kit:@handle/kit, collection: or @publisher/package') + .argument('', 'the source as installed: kit:@handle/kit, collection:, @publisher/package or add:') .option('--lock ', 'packages lock file to drop the package from', INSTALL_LOCK_FILENAME) .option('--no-lock', 'leave the lock file alone') .action((refs: string[], options: ApplyOptions) => diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 997ba36..9ac0e24 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -15,6 +15,7 @@ import { promoteCommand } from './commands/promote.js'; import { releaseCommand } from './commands/release-publish.js'; import { installCommand, listCommand, syncCommand, uninstallCommand, updateCommand } from './commands/install.js'; import { mcpCommand } from './commands/mcp.js'; +import { addCommand } from './commands/add.js'; import { libraryCommand } from './commands/library.js'; import { kitCommand } from './commands/kit.js'; import { routeUsageErrors } from './lib/run-command.js'; @@ -43,6 +44,7 @@ program.addCommand(publishCommand); program.addCommand(promoteCommand); program.addCommand(releaseCommand); for (const command of [installCommand, updateCommand, uninstallCommand, listCommand, syncCommand, mcpCommand]) program.addCommand(command); +program.addCommand(addCommand); program.addCommand(libraryCommand); program.addCommand(kitCommand); diff --git a/packages/cli/src/lib/add/add-install.test.ts b/packages/cli/src/lib/add/add-install.test.ts new file mode 100644 index 0000000..fc5730e --- /dev/null +++ b/packages/cli/src/lib/add/add-install.test.ts @@ -0,0 +1,172 @@ +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { runAdd } from '../../commands/add.js'; +import { assertCanPrompt } from '../../commands/add-prompts.js'; +import type { AddResult } from '../../commands/add-display.js'; +import { createInstallContext } from '../install/install-context.js'; +import type { InstallContext } from '../install/install-context.js'; +import { listInstalled, uninstallRef, updateInstalled } from '../install/install-operations.js'; +import { readOwners, readReceipt } from '../install/install-state.js'; +import { createRegistry } from '../install/registry-client.js'; +import { CliError } from '../output.js'; +import { applyAdd } from './add-install.js'; +import { openLocalSource } from './local-source.js'; +import { discoverSkills, prepareSkill } from './skill-discovery.js'; + +/** + * `skillx add` from a local path, end to end into a temporary home: the + * skills directory, receipts and owners all live under `root`, and the + * registry is never contacted. + */ + +let root: string; +let src: string; +let ctx: (force?: boolean) => InstallContext; +const saved = { ...process.env }; + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'skillx-add-e2e-')); + src = join(root, 'src'); + const env = { HOME: join(root, 'home'), USERPROFILE: join(root, 'home'), SKILLX_HOME: join(root, 'skillx'), CLAUDE_CONFIG_DIR: join(root, 'claude') }; + // runAdd builds its context from process.env, like the CLI. + Object.assign(process.env, env); + ctx = (force = false) => createInstallContext({ registry: createRegistry({ baseUrl: 'http://127.0.0.1:9' }), env, force }); +}); +afterEach(() => { + process.env = { ...saved }; + rmSync(root, { recursive: true, force: true }); +}); + +function write(rel: string, text: string): void { + const path = join(src, ...rel.split('/')); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, text); +} +const skillMd = (name: string, body = '') => `---\nname: ${name}\ndescription: ${name} skill\n---\n# ${name}\n${body}`; +const skillsDir = () => join(root, 'claude', 'skills'); + +async function add(names: string[], force = false): Promise { + return runAdd(src, { json: true, skill: names, ...(force ? { force } : {}) }); +} + +async function refusal(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + expect(error).toBeInstanceOf(CliError); + return error as CliError; + } + throw new Error('expected a refusal'); +} + +describe('skillx add from a local path', () => { + it('installs through the installer: files, receipt and an add: owner', async () => { + write('skills/demo/SKILL.md', skillMd('demo')); + write('skills/demo/references/guide.md', 'guide\n'); + write('skills/other/SKILL.md', skillMd('other')); + const result = await add(['demo']); + expect(result).toMatchObject({ status: 'installed', source: src, runtime: 'claude-code', skillsDir: skillsDir(), skills: [{ alias: 'demo', origin: join(src, 'skills', 'demo') }] }); + expect(readFileSync(join(skillsDir(), 'demo', 'references', 'guide.md'), 'utf8')).toBe('guide\n'); + expect(existsSync(join(skillsDir(), 'other'))).toBe(false); + + const receipt = readReceipt(join(root, 'skillx'), 'claude-code', 'demo')!; + expect(receipt).toMatchObject({ package: join(src, 'skills', 'demo'), version: 'local', requiredBy: ['add:demo'] }); + expect(receipt.files.map((f) => f.path)).toEqual(['SKILL.md', 'references/guide.md']); + expect(readOwners(join(root, 'skillx'), 'claude-code').owners['add:demo']?.ref).toBe(src); + expect(listInstalled(ctx()).skills.map((s) => s.alias)).toEqual(['demo']); + }); + + it('is a no-op when the same tree is added again, and refuses a changed one without --force', async () => { + write('demo/SKILL.md', skillMd('demo')); + await add([]); + const again = await add([]); + expect(again.status === 'installed' && again.applied[0]!.changed).toEqual([]); + + write('demo/SKILL.md', skillMd('demo', 'v2\n')); + const refused = await refusal(() => add([])); + expect(refused.code).toBe('conflict'); + expect(refused.message).toContain('--force'); + expect(readFileSync(join(skillsDir(), 'demo', 'SKILL.md'), 'utf8')).not.toContain('v2'); + + await add([], true); + expect(readFileSync(join(skillsDir(), 'demo', 'SKILL.md'), 'utf8')).toContain('v2'); + }); + + it('never installs over an existing directory it does not own without --force, and keeps unknown files with it', async () => { + write('demo/SKILL.md', skillMd('demo')); + mkdirSync(join(skillsDir(), 'demo'), { recursive: true }); + writeFileSync(join(skillsDir(), 'demo', 'MY-EDITS.md'), 'mine\n'); + const refused = await refusal(() => add([])); + expect(refused.code).toBe('conflict'); + expect(refused.extra.details).toMatchObject({ conflicts: [{ alias: 'demo', reason: 'unowned_directory' }] }); + expect(existsSync(join(skillsDir(), 'demo', 'SKILL.md'))).toBe(false); + + await add([], true); + expect(readFileSync(join(skillsDir(), 'demo', 'MY-EDITS.md'), 'utf8')).toBe('mine\n'); + expect(existsSync(join(skillsDir(), 'demo', 'SKILL.md'))).toBe(true); + }); + + it('refuses reserved names when asked for one, and never installs them with *', async () => { + write('skills/ak-plan/SKILL.md', skillMd('ak-plan')); + write('skills/fine/SKILL.md', skillMd('fine')); + expect((await refusal(() => add(['ak-plan']))).code).toBe('usage_error'); + const all = await add(['*']); + expect(all.status === 'installed' && all.skills.map((s) => s.alias)).toEqual(['fine']); + expect(all.status === 'installed' && all.skipped).toEqual([{ dir: 'skills/ak-plan', reason: expect.stringContaining('reserved') }]); + expect(existsSync(join(skillsDir(), 'ak-plan'))).toBe(false); + }); + + it('asks for names under --json when the source holds several skills', async () => { + write('a/SKILL.md', skillMd('a')); + write('b/SKILL.md', skillMd('b')); + expect((await refusal(() => runAdd(src, { json: true }))).code).toBe('usage_error'); + expect((await refusal(() => add(['nope']))).code).toBe('not_found'); + }); + + it('refuses to prompt without a terminal unless --yes or --json says not to ask', () => { + const refused = (() => { + try { + assertCanPrompt({}, false); + } catch (error) { + return error as CliError; + } + throw new Error('expected a refusal'); + })(); + expect(refused).toBeInstanceOf(CliError); + expect(refused.code).toBe('usage_error'); + expect(refused.message).toContain('--yes'); + expect(() => assertCanPrompt({ yes: true }, false)).not.toThrow(); + expect(() => assertCanPrompt({ json: true }, false)).not.toThrow(); + expect(() => assertCanPrompt({}, true)).not.toThrow(); + }); + + it('lists without installing anything', async () => { + write('a/SKILL.md', skillMd('a')); + const listed = await runAdd(src, { json: true, list: true }); + expect(listed).toMatchObject({ status: 'listed', skills: [{ name: 'a', dir: 'a', description: 'a skill' }], skipped: [] }); + expect(existsSync(skillsDir())).toBe(false); + }); + + it('uninstalls add: with ownership proof, and update leaves added skills alone', async () => { + write('demo/SKILL.md', skillMd('demo')); + await add([]); + expect(await updateInstalled(ctx(), [])).toEqual([]); + expect((await refusal(() => updateInstalled(ctx(), ['add:demo']))).code).toBe('usage_error'); + + writeFileSync(join(skillsDir(), 'demo', 'notes.md'), 'my notes\n'); + const [removed] = await uninstallRef(ctx(), ['add:demo']); + expect(removed!.removed[0]).toMatchObject({ alias: 'demo', removed: ['SKILL.md'], keptUnknown: ['notes.md'] }); + expect(readReceipt(join(root, 'skillx'), 'claude-code', 'demo')).toBeNull(); + expect(readOwners(join(root, 'skillx'), 'claude-code').owners['add:demo']).toBeUndefined(); + }); + + it('refuses two selected skills that install under one name', async () => { + write('one/SKILL.md', skillMd('same')); + write('two/SKILL.md', skillMd('same')); + const snapshot = openLocalSource(src); + const added = discoverSkills(snapshot, '').skills.map((s) => ({ alias: s.name, origin: s.dir, tree: prepareSkill(snapshot, s) })); + expect((await refusal(() => applyAdd(ctx(), { ref: src, version: 'local' }, added))).code).toBe('usage_error'); + }); +}); diff --git a/packages/cli/src/lib/add/add-install.ts b/packages/cli/src/lib/add/add-install.ts new file mode 100644 index 0000000..2dfd143 --- /dev/null +++ b/packages/cli/src/lib/add/add-install.ts @@ -0,0 +1,83 @@ +import { INSTALL_LOCK_SCHEMA, INSTALL_RESOLVER_VERSION, canonicalJsonBytes, lockDigest, sortLockPackages } from '@skillx/contracts'; +import type { InstallLock, InstallLockPackage } from '@skillx/contracts'; +import type { VerifiedTree } from '../install/fetch-verify.js'; +import { sha256Hex } from '../install/fetch-verify.js'; +import type { InstallContext } from '../install/install-context.js'; +import { planOwned } from '../install/install-engine.js'; +import type { OwnedLock } from '../install/install-engine.js'; +import { conflictError, usageError } from '../install/install-errors.js'; +import { ADD_OWNER_PREFIX, applyPlanned } from '../install/install-operations.js'; +import type { ApplyOutcome, PlannedItem, SourcePlan } from '../install/install-operations.js'; +import { readReceipt } from '../install/install-state.js'; + +/** + * `skillx add` through the generic installer. Each added skill is its own + * owner, `add:`, recorded in the owners file with the source it came + * from, so `list` shows it and `uninstall add:` removes it with + * ownership proof. Trees were read and checked by the caller; the installer + * plans, stages, journals and swaps them like any release, under the same + * install lock, conflicts and backups. + * + * On top of the installer's rules (an unowned directory needs `--force`, a + * skill another source needs at another digest is never replaced), `add` + * never replaces a skill it added before without `--force`. Nothing goes to + * `skillx.lock.json`: a git or local tree has no signed release to pin. + */ + +export interface AddedSkill { + alias: string; + /** Where the skill came from, e.g. `github:owner/repo/skills/demo`; no credentials. */ + origin: string; + tree: VerifiedTree; +} + +export interface AddSourceInfo { + /** The source as the user can give it again; no credentials. */ + ref: string; + /** The commit read, or `local`. */ + version: string; +} + +export const addOwnerOf = (alias: string) => `${ADD_OWNER_PREFIX}${alias}`; + +function owned(source: AddSourceInfo, skill: AddedSkill): OwnedLock { + const pkg: InstallLockPackage = { alias: skill.alias, package: skill.origin, version: source.version, releaseDigest: skill.tree.digest, compositionDigest: skill.tree.digest, overlayDigests: [] }; + const lock: InstallLock = { schema: INSTALL_LOCK_SCHEMA, resolver: INSTALL_RESOLVER_VERSION, source: { kind: 'packages' }, runtimes: {}, packages: [pkg] }; + return { owner: addOwnerOf(skill.alias), ref: source.ref, lock, overlays: new Map(), trees: new Map([[skill.alias, skill.tree]]) }; +} + +/** Plans the skills; refuses duplicates and, without `--force`, replacing a skill added before. */ +export async function planAdd(ctx: InstallContext, source: AddSourceInfo, skills: readonly AddedSkill[]): Promise { + if (skills.length === 0) throw usageError('no skill selected'); + const seen = new Set(); + for (const s of skills) { + if (seen.has(s.alias)) throw usageError(`two selected skills install as '${s.alias}'; pick one with --skill`, { alias: s.alias }); + seen.add(s.alias); + } + const items: PlannedItem[] = skills.map((s) => { + const o = owned(source, s); + return { ...o, plan: planOwned(ctx, o) }; + }); + if (!ctx.force) { + const replaced = items.flatMap((i) => i.plan.actions).filter((a) => a.action === 'update'); + if (replaced.length > 0) { + const from = replaced.map((a) => `${a.alias} (from ${readReceipt(ctx.home, ctx.runtime, a.alias)?.package ?? 'an earlier add'})`); + throw conflictError(`${from.join(', ')} already installed; pass --force to replace`, { + conflicts: replaced.map((a) => ({ alias: a.alias, reason: 'installed', files: [] })), + }); + } + } + const lock: InstallLock = { ...items[0]!.lock, packages: sortLockPackages(items.map((i) => i.lock.packages[0]!)) }; + return { + lock, + lockDigest: await lockDigest(lock), + items, + planDigest: `sha256:${sha256Hex(canonicalJsonBytes(items.map((i) => i.plan.planDigest)))}`, + warnings: [], + }; +} + +/** Plans afresh under the install lock and applies; `expectDigest` pins the plan the user confirmed. */ +export function applyAdd(ctx: InstallContext, source: AddSourceInfo, skills: readonly AddedSkill[], expectDigest?: string): Promise { + return applyPlanned(ctx, () => planAdd(ctx, source, skills), expectDigest); +} diff --git a/packages/cli/src/lib/add/add-source.test.ts b/packages/cli/src/lib/add/add-source.test.ts new file mode 100644 index 0000000..1c82989 --- /dev/null +++ b/packages/cli/src/lib/add/add-source.test.ts @@ -0,0 +1,108 @@ +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { CliError } from '../output.js'; +import { parseAddSource, redactCredentials } from './add-source.js'; +import { redactError } from '../../commands/add.js'; + +const refusal = (input: string) => { + try { + parseAddSource(input); + } catch (error) { + expect(error).toBeInstanceOf(CliError); + return error as CliError; + } + throw new Error(`expected '${input}' to be refused`); +}; + +describe('parseAddSource', () => { + it('reads github: sources with a path and a ref', () => { + expect(parseAddSource('github:acme/skills')).toMatchObject({ kind: 'github', owner: 'acme', repo: 'skills', path: '', cloneUrl: 'https://github.com/acme/skills.git', label: 'github:acme/skills' }); + expect(parseAddSource('github:acme/skills/tools/lint@v1.2')).toMatchObject({ path: 'tools/lint', ref: 'v1.2', label: 'github:acme/skills/tools/lint@v1.2' }); + expect(parseAddSource('github:acme/skills@release/2026')).toMatchObject({ path: '', ref: 'release/2026' }); + }); + + it('reads the owner/repo[/skill] shorthand as a GitHub source with a skill name', () => { + expect(parseAddSource('acme/skills')).toMatchObject({ kind: 'github', owner: 'acme', repo: 'skills', path: '' }); + expect(parseAddSource('acme/skills/pdf')).toMatchObject({ kind: 'github', skill: 'pdf', path: '' }); + }); + + it('maps github.com web URLs to github: and keeps other https URLs as git sources', () => { + expect(parseAddSource('https://github.com/acme/skills.git')).toMatchObject({ kind: 'github', owner: 'acme', repo: 'skills', path: '' }); + expect(parseAddSource('https://github.com/acme/skills/tree/main/skills/pdf')).toMatchObject({ kind: 'github', ref: 'main', path: 'skills/pdf' }); + expect(parseAddSource('https://git.example.com/team/skills.git')).toEqual({ kind: 'git', cloneUrl: 'https://git.example.com/team/skills.git', label: 'https://git.example.com/team/skills.git' }); + }); + + it('reads local directories relative to the working directory', () => { + const cwd = resolve('/work/project'); + expect(parseAddSource('./skills', cwd)).toEqual({ kind: 'local', dir: resolve(cwd, 'skills'), label: resolve(cwd, 'skills') }); + expect(parseAddSource('..', cwd)).toMatchObject({ kind: 'local', dir: resolve(cwd, '..') }); + expect(parseAddSource(resolve('/abs/skills'), cwd)).toMatchObject({ kind: 'local', dir: resolve('/abs/skills') }); + }); + + it.each([ + ['ext::sh -c touch% /tmp/pwned', 'transport helpers'], + ['fd::17', 'transport helpers'], + ['file:///etc/passwd', 'only https'], + ['http://git.example.com/x.git', 'only https'], + ['ssh://git@github.com/acme/skills.git', 'only https'], + ['git@github.com:acme/skills.git', 'scp-style'], + ['--upload-pack=touch /tmp/pwned', 'option'], + ['-u touch /tmp/pwned', 'option'], + ['https://git.example.com/x.git?private_token=abc', 'no query'], + ])('refuses %s as a usage error', (input, why) => { + const error = refusal(input); + expect(error.code).toBe('usage_error'); + expect(error.message).toContain(why); + }); + + it.each(['github:acme/skills/../../etc', 'github:acme/skills/a/./b', 'github:acme/skills/a\\b', 'github:acme/..', 'https://github.com/acme/skills/tree/main/..%2F..'])( + 'refuses the traversal or unsafe path in %s', + (input) => { + expect(refusal(input).code).toBe('usage_error'); + }, + ); + + it.each(['github:acme/skills@-oops', 'github:acme/skills@a..b', 'github:acme/skills@x@{1}', 'github:acme/skills@main.lock'])('refuses the ref in %s', (input) => { + expect(refusal(input).code).toBe('usage_error'); + }); + + it('sends registry sources to skillx install and refuses @agentkit', () => { + expect(refusal('@agentkit/plan').message).toContain('reserved'); + expect(refusal('@acme/pdf').message).toContain('skillx install'); + expect(refusal('kit:@acme/core').message).toContain('skillx install'); + }); +}); + +describe('credentials in URLs', () => { + const token = 'ghp_SECRETTOKEN123'; + + it('clones with the credentials but never puts them in the label', () => { + const source = parseAddSource(`https://${token}@git.example.com/team/skills.git`); + expect(source).toMatchObject({ kind: 'git', cloneUrl: `https://${token}@git.example.com/team/skills.git`, label: 'https://git.example.com/team/skills.git' }); + // github.com with credentials stays a plain git source, so the token is never dropped silently either. + const gh = parseAddSource(`https://user:${token}@github.com/acme/private.git`); + expect(gh.kind).toBe('git'); + expect(gh.label).not.toContain(token); + }); + + it('redacts userinfo anywhere in a text, also inside JSON', () => { + expect(redactCredentials(`fatal: could not read from https://user:${token}@host/x.git and http://${token}@h`)).toBe('fatal: could not read from https://***@host/x.git and http://***@h'); + const json = JSON.stringify({ hint: `https://a:${token}@h/x`, other: 'mail me@example.com' }); + expect(JSON.parse(redactCredentials(json))).toEqual({ hint: 'https://***@h/x', other: 'mail me@example.com' }); + expect(redactCredentials('https://github.com/acme/skills')).toBe('https://github.com/acme/skills'); + }); + + it('never echoes a token in a refusal', () => { + for (const input of [`http://${token}@host/x.git`, `https://${token}@host/x.git#main`, `ssh://${token}@host/x`]) { + expect(refusal(input).message).not.toContain(token); + } + }); + + it('redactError masks message, hint and details of any failure', () => { + const error = new CliError('network_error', `cannot clone https://u:${token}@h/x`, { hint: `retry https://${token}@h`, details: { url: `https://${token}@h/x` } }); + const out = redactError(error) as CliError; + expect(JSON.stringify({ m: out.message, e: out.extra })).not.toContain(token); + expect(out.code).toBe('network_error'); + expect((redactError(new Error(`boom https://${token}@h`)) as Error).message).toBe('boom https://***@h'); + }); +}); diff --git a/packages/cli/src/lib/add/add-source.ts b/packages/cli/src/lib/add/add-source.ts new file mode 100644 index 0000000..8fc28f9 --- /dev/null +++ b/packages/cli/src/lib/add/add-source.ts @@ -0,0 +1,129 @@ +import { isAbsolute, resolve } from 'node:path'; +import { usageError } from '../install/install-errors.js'; + +/** + * Sources `skillx add` accepts, parsed before anything touches the network or + * the disk: + * + * - `github:owner/repo[/path][@ref]`: a GitHub repository, optionally a + * directory in it and a branch or tag; + * - `owner/repo[/skill]`: GitHub shorthand, optionally one skill by name; + * - `https://github.com/owner/repo[/tree/[/]]` maps to `github:`; + * - any other `https://` git URL; every other transport (`http`, `ssh`, + * `git@host:`, `file://`, `ext::` and other `::` helpers) is refused, and so + * is anything that starts with `-`, so a source never becomes a git option; + * - a local directory: an absolute path, or one starting with `.`/`..`. + * + * Credentials in a URL may be used to clone but never leave this module in a + * label: `label` strips them, and `redactCredentials` masks them in any text. + */ + +export type AddSource = + | { kind: 'github'; owner: string; repo: string; path: string; ref?: string; skill?: string; cloneUrl: string; label: string } + | { kind: 'git'; cloneUrl: string; label: string } + | { kind: 'local'; dir: string; label: string }; + +const OWNER = '[A-Za-z0-9][A-Za-z0-9-]{0,38}'; +const REPO = '[A-Za-z0-9._-]{1,100}'; +const SHORTHAND = new RegExp(`^(${OWNER})/(${REPO})(?:/([A-Za-z0-9._-]{1,128}))?$`); +const GITHUB_REF = new RegExp(`^(${OWNER})/(${REPO})(/[^@]*)?(?:@(.+))?$`); +const GITHUB_URL = new RegExp(`^/(${OWNER})/(${REPO}?)(?:\\.git)?(?:/tree/([^/]+)((?:/[^/]+)*))?/?$`); +/** A branch or tag name git accepts and that cannot read as an option or a revision expression. */ +const REF = /^(?!-)(?!.*\.\.)(?!.*\/\/)(?!.*@\{)(?!.*\.lock$)(?!.*\/$)[A-Za-z0-9._/-]{1,200}$/; +const USAGE = 'owner/repo[/skill], github:owner/repo[/path][@ref], an https git URL or a local directory'; + +/** + * Masks the userinfo of every `scheme://user:secret@host` in `text`. Quotes + * end a match, so the result of redacting serialized JSON is still JSON. + */ +export function redactCredentials(text: string): string { + return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s/?#@"'<>]*(?:@[^\s/?#@"'<>]*)*@/gi, '$1***@'); +} + +const refuse = (input: string, why: string) => usageError(`refusing source '${redactCredentials(input)}': ${why}`); + +function checkRepo(repo: string, input: string): void { + if (repo === '.' || repo === '..') throw refuse(input, 'the repository name is not a name'); +} + +/** A repository path: plain `/`-separated segments, never `.`, `..`, empty, `\` or `:`. */ +export function checkRepoPath(path: string, input: string): string { + const trimmed = path.replace(/^\/+|\/+$/g, ''); + if (trimmed === '') return ''; + for (const segment of trimmed.split('/')) { + if (segment === '' || segment === '.' || segment === '..' || /[\\:\0-\x1f]/.test(segment)) throw refuse(input, `unsafe path '${trimmed}'`); + } + return trimmed; +} + +function checkRef(ref: string, input: string): string { + if (!REF.test(ref)) throw refuse(input, `'${ref}' is not a branch or tag name`); + return ref; +} + +function github(input: string, owner: string, repo: string, path: string, ref: string | undefined, skill?: string): AddSource { + checkRepo(repo, input); + const label = `github:${owner}/${repo}${path ? `/${path}` : ''}${ref ? `@${ref}` : ''}`; + return { kind: 'github', owner, repo, path, ...(ref ? { ref } : {}), ...(skill ? { skill } : {}), cloneUrl: `https://github.com/${owner}/${repo}.git`, label }; +} + +function fromUrl(input: string): AddSource { + if (input.includes('::')) throw refuse(input, 'git transport helpers (ext::, fd:: and the like) are not accepted'); + if (/^[^/:]+@[^/:]+:/.test(input)) throw refuse(input, 'scp-style ssh sources are not accepted; use an https URL'); + let url: URL; + try { + url = new URL(input); + } catch { + throw refuse(input, 'not a valid URL'); + } + if (url.protocol !== 'https:') throw refuse(input, `only https git URLs are accepted, not ${url.protocol.replace(/:$/, '')}`); + if (url.hostname === '') throw refuse(input, 'the URL has no host'); + if (url.search !== '' || url.hash !== '') throw refuse(input, 'a git URL takes no query or fragment'); + const credentials = url.username !== '' || url.password !== ''; + const gh = url.hostname === 'github.com' && url.port === '' && !credentials ? GITHUB_URL.exec(url.pathname) : null; + if (gh) { + // Decoded first, so an encoded `..%2F..` is checked as the `../..` GitHub would see. + const decode = (part: string) => { + try { + return decodeURIComponent(part); + } catch { + throw refuse(input, 'the URL path is not valid percent-encoding'); + } + }; + return github(input, gh[1]!, gh[2]!, checkRepoPath(decode(gh[4] ?? ''), input), gh[3] ? checkRef(decode(gh[3]), input) : undefined); + } + const bare = new URL(url.href); + bare.username = ''; + bare.password = ''; + return { kind: 'git', cloneUrl: url.href, label: bare.href }; +} + +function isLocalPath(input: string): boolean { + return isAbsolute(input) || /^\.\.?(?:[/\\]|$)/.test(input) || /^[A-Za-z]:[/\\]/.test(input); +} + +/** Parses a source string; every refusal is a `usage_error` whose message never carries credentials. */ +export function parseAddSource(input: string, cwd: string = process.cwd()): AddSource { + const raw = input.trim(); + if (raw === '') throw usageError(`name a source: ${USAGE}`); + if (/[\0-\x1f\x7f]/.test(raw)) throw refuse(raw, 'it contains control characters'); + if (raw.startsWith('-')) throw refuse(raw, 'it reads as an option'); + if (raw.startsWith('@agentkit/')) throw refuse(raw, '@agentkit is reserved for AgentKit kit releases'); + if (raw.startsWith('@') || /^(?:kit|collection):/.test(raw)) throw refuse(raw, 'registry sources are installed with skillx install'); + if (isLocalPath(raw)) { + const dir = resolve(cwd, raw); + return { kind: 'local', dir, label: dir }; + } + if (raw.startsWith('github:')) { + const m = GITHUB_REF.exec(raw.slice('github:'.length)); + if (!m) throw refuse(raw, 'expected github:owner/repo[/path][@ref]'); + return github(raw, m[1]!, m[2]!, checkRepoPath(m[3] ?? '', raw), m[4] === undefined ? undefined : checkRef(m[4], raw)); + } + if (raw.includes('://') || raw.includes('::') || /^[^/:]+@[^/:]+:/.test(raw)) return fromUrl(raw); + const m = SHORTHAND.exec(raw); + if (m) return github(raw, m[1]!, m[2]!, '', undefined, m[3]); + throw usageError(`'${redactCredentials(raw)}' is not a source: ${USAGE}`); +} + +/** Where discovery starts inside the source: the `github:` path, else the root. */ +export const searchPathOf = (source: AddSource) => (source.kind === 'github' ? source.path : ''); diff --git a/packages/cli/src/lib/add/git-source.test.ts b/packages/cli/src/lib/add/git-source.test.ts new file mode 100644 index 0000000..37f5ebd --- /dev/null +++ b/packages/cli/src/lib/add/git-source.test.ts @@ -0,0 +1,131 @@ +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { CliError } from '../output.js'; +import { cloneArgs, gitEnv, openGitSource } from './git-source.js'; +import { discoverSkills, prepareSkill } from './skill-discovery.js'; + +/** + * Clones a local repository through the `file` transport, which only tests + * may allow; the CLI always passes `https`. The repository holds a skill and + * a skill with a symlink entry that points out of the repository. + */ + +let root: string; +let repoUrl: string; + +function git(cwd: string, args: string[], input?: string): string { + return execFileSync('git', ['-c', 'core.autocrlf=false', '-c', 'commit.gpgsign=false', '-c', 'user.name=Test', '-c', 'user.email=test@example.com', ...args], { cwd, input, encoding: 'utf8' }).trim(); +} + +beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'skillx-add-git-')); + const repo = join(root, 'repo'); + const write = (rel: string, text: string) => { + mkdirSync(dirname(join(repo, rel)), { recursive: true }); + writeFileSync(join(repo, rel), text); + }; + mkdirSync(repo); + git(repo, ['init', '-q', '-b', 'main']); + write('skills/demo/SKILL.md', '---\nname: demo\ndescription: Demo skill\n---\n# Demo\n'); + write('skills/demo/references/guide.md', 'guide\n'); + write('skills/leaky/SKILL.md', '---\nname: leaky\ndescription: Leaks\n---\n'); + git(repo, ['add', '-A']); + // A symlink entry made without the filesystem, so the test runs where symlinks need privileges. + const oid = git(repo, ['hash-object', '-w', '--stdin'], '../../../../home/user/.ssh/id_rsa'); + git(repo, ['update-index', '--add', '--cacheinfo', `120000,${oid},skills/leaky/leak`]); + git(repo, ['commit', '-q', '-m', 'skills']); + repoUrl = pathToFileURL(repo).href; +}, 60_000); + +afterAll(() => rmSync(root, { recursive: true, force: true })); + +function failure(run: () => unknown): CliError { + try { + run(); + } catch (error) { + expect(error).toBeInstanceOf(CliError); + return error as CliError; + } + throw new Error('expected a failure'); +} + +describe('git sources', () => { + it('clones with no prompt, no checkout and the URL after --', () => { + const args = cloneArgs('https://host/x.git', '/tmp/dest', 'main'); + expect(args.slice(0, 6)).toEqual(['-c', 'core.askPass=', '-c', 'protocol.allow=never', '-c', 'protocol.https.allow=always']); + expect(args).toEqual(expect.arrayContaining(['--depth', '1', '--no-checkout', '--branch', 'main'])); + expect(args.slice(-3)).toEqual(['--', 'https://host/x.git', '/tmp/dest']); + expect(gitEnv({})).toMatchObject({ GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never', GIT_ASKPASS: '' }); + }); + + it('reads skills from git objects and refuses a skill holding a symlink entry', () => { + const source = openGitSource(repoUrl, 'test-repo', 'main', 'file'); + try { + expect(source.commit).toMatch(/^[0-9a-f]{40}$/); + const { skills } = discoverSkills(source, ''); + expect(skills.map((s) => s.name)).toEqual(['demo', 'leaky']); + expect(prepareSkill(source, skills[0]!).files.map((f) => f.path)).toEqual(['SKILL.md', 'references/guide.md']); + const refused = failure(() => prepareSkill(source, skills[1]!)); + expect(refused.code).toBe('incompatible'); + expect(refused.extra.details).toMatchObject({ code: 'tree.unsupported_entry', path: 'leak' }); + expect(discoverSkills(source, 'skills/demo').skills.map((s) => s.name)).toEqual(['demo']); + expect(failure(() => discoverSkills(source, 'skills/leaky/leak')).code).toBe('not_found'); + } finally { + source.dispose(); + } + }); + + it('drops inherited variables that point git at another repository', () => { + const env = gitEnv({ GIT_DIR: '/x/.git', git_work_tree: '/x', GIT_INDEX_FILE: 'i', GIT_COMMON_DIR: 'c', PATH: '/bin' }); + expect(env).not.toHaveProperty('GIT_DIR'); + expect(env).not.toHaveProperty('git_work_tree'); + expect(env).not.toHaveProperty('GIT_INDEX_FILE'); + expect(env).not.toHaveProperty('GIT_COMMON_DIR'); + expect(env.PATH).toBe('/bin'); + }); + + it('reads the clone even when GIT_DIR names another repository', () => { + const other = join(root, 'other'); + mkdirSync(join(other, 'skills', 'intruder'), { recursive: true }); + writeFileSync(join(other, 'skills', 'intruder', 'SKILL.md'), '---\nname: intruder\ndescription: Wrong repo\n---\n'); + git(other, ['init', '-q', '-b', 'main']); + git(other, ['add', '-A']); + git(other, ['commit', '-q', '-m', 'other']); + const expected = git(join(root, 'repo'), ['rev-parse', 'HEAD']); + const saved = process.env.GIT_DIR; + process.env.GIT_DIR = join(other, '.git'); + try { + const source = openGitSource(repoUrl, 'test-repo', 'main', 'file'); + try { + expect(source.commit).toBe(expected); + expect(discoverSkills(source, '').skills.map((s) => s.name)).toEqual(['demo', 'leaky']); + } finally { + source.dispose(); + } + } finally { + if (saved === undefined) delete process.env.GIT_DIR; + else process.env.GIT_DIR = saved; + } + }); + + it('lets git itself refuse every transport but https', () => { + const refused = failure(() => openGitSource(repoUrl, 'test-repo')); + expect(refused.code).toBe('usage_error'); + expect(refused.message).toContain('not allowed'); + }); + + it('reports a missing branch as not found', () => { + expect(failure(() => openGitSource(repoUrl, 'test-repo', 'no-such-branch', 'file')).code).toBe('not_found'); + }); + + it('never prints the credentials of a failing clone', () => { + const token = 'ghp_SECRETTOKEN123'; + const refused = failure(() => openGitSource(`https://user:${token}@127.0.0.1:9/x.git`, 'https://127.0.0.1:9/x.git')); + expect(refused.code).toBe('network_error'); + expect(JSON.stringify({ m: refused.message, e: refused.extra })).not.toContain(token); + }, 60_000); +}); diff --git a/packages/cli/src/lib/add/git-source.ts b/packages/cli/src/lib/add/git-source.ts new file mode 100644 index 0000000..4f10ee2 --- /dev/null +++ b/packages/cli/src/lib/add/git-source.ts @@ -0,0 +1,147 @@ +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { InstallError } from '../install/install-errors.js'; +import { redactCredentials } from './add-source.js'; +import type { ListedEntry, SourceSnapshot } from './skill-discovery.js'; +import { isDiscoverable } from './skill-discovery.js'; + +/** + * A git source, cloned into a private temporary directory and read from git + * objects only. The clone is shallow, single-branch and has no checkout, so + * no file of the repository is ever written to disk, no checkout filter, LFS + * smudge or hook runs, and a symlink stays a `120000` tree entry that the + * tree rules refuse. git runs with argument arrays (no shell), `--` before the + * URL, every credential prompt off (`core.askPass=` plus an empty + * `GIT_ASKPASS`, `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=never`), and only + * the https transport allowed, so `ext::`, `file://` and `ssh` are refused by + * git itself too, also on a redirect. Variables that point git at another + * repository (`GIT_DIR`, `GIT_WORK_TREE`, ...) are never inherited, and every + * read names the clone's `--git-dir`. Every message is redacted. + */ + +const CLONE_TIMEOUT_MS = 120_000; +const MAX_OUTPUT = 1024 * 1024 * 1024; + +/** Variables that redirect git to another repository, index or object store. */ +const REPO_ENV = new Set(['GIT_DIR', 'GIT_WORK_TREE', 'GIT_INDEX_FILE', 'GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES', 'GIT_COMMON_DIR', 'GIT_NAMESPACE', 'GIT_CEILING_DIRECTORIES']); + +export function gitEnv(base: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + // Compared in upper case: Windows environment names are case-insensitive. + const env = Object.fromEntries(Object.entries(base).filter(([key]) => !REPO_ENV.has(key.toUpperCase()))); + return { ...env, GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never', GIT_ASKPASS: '', SSH_ASKPASS: '', GIT_LFS_SKIP_SMUDGE: '1' }; +} + +/** Pins a read to the clone itself, never a repository found from the directory or environment. */ +const inClone = (repo: string) => ['-C', repo, `--git-dir=${join(repo, '.git')}`]; + +/** The clone command line. `transport` is `https` outside tests. */ +export function cloneArgs(url: string, dest: string, ref?: string, transport = 'https'): string[] { + return [ + '-c', 'core.askPass=', + '-c', 'protocol.allow=never', + '-c', `protocol.${transport}.allow=always`, + 'clone', '--depth', '1', '--no-checkout', '--single-branch', '--no-tags', + ...(ref === undefined ? [] : ['--branch', ref]), + '--', url, dest, + ]; +} + +function git(args: string[], what: string, input?: string): Buffer { + const r = spawnSync('git', args, { env: gitEnv(), input, maxBuffer: MAX_OUTPUT, timeout: CLONE_TIMEOUT_MS, windowsHide: true }); + if (r.error) { + const code = (r.error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') throw new InstallError('unexpected_error', 'git is not installed or not on PATH'); + if (code === 'ETIMEDOUT') throw new InstallError('network_error', `${what} timed out after ${CLONE_TIMEOUT_MS / 1000}s`); + throw new InstallError('unexpected_error', `${what} failed: ${redactCredentials(r.error.message)}`); + } + if (r.status !== 0) { + const stderr = redactCredentials(r.stderr.toString('utf8')).trim(); + const line = stderr.split('\n').filter((l) => !/^\s*$/.test(l)).pop() ?? `exit ${r.status}`; + throw new InstallError(classify(stderr), `${what} failed: ${line}`); + } + return r.stdout; +} + +function classify(stderr: string): 'auth_error' | 'not_found' | 'usage_error' | 'network_error' { + if (/authentication failed|could not read (username|password)|terminal prompts disabled|returned error: 40[13]/i.test(stderr)) return 'auth_error'; + if (/repository .*not found|not found|remote branch .* not found|returned error: 404/i.test(stderr)) return 'not_found'; + if (/transport .* not allowed|unsupported protocol/i.test(stderr)) return 'usage_error'; + return 'network_error'; +} + +/** `ls-tree -r -l -z` records: every blob and submodule of the commit, with its mode and size. */ +function listTree(dir: string): Array { + const out = git([...inClone(dir), 'ls-tree', '-r', '-l', '-z', '--full-tree', 'HEAD'], 'git ls-tree'); + const decoder = new TextDecoder('utf-8', { fatal: true }); + const entries: Array = []; + for (const record of out.toString('latin1').split('\0')) { + if (record === '') continue; + const tab = record.indexOf('\t'); + const m = /^(\d{6}) \w+ ([0-9a-f]{40,64}) +(\S+)$/.exec(record.slice(0, tab)); + if (tab < 0 || !m) throw new InstallError('unexpected_error', 'git ls-tree printed an unexpected record'); + let path: string; + try { + path = decoder.decode(Buffer.from(record.slice(tab + 1), 'latin1')); + } catch { + throw new InstallError('incompatible', 'a path in the repository is not valid UTF-8'); + } + const [mode, oid, size] = [m[1]!, m[2]!, m[3]!]; + const kind = mode === '120000' ? 'symlink' : mode === '160000' ? 'special' : 'regular'; + entries.push({ path, kind, mode: mode === '100664' ? '100644' : mode, size: size === '-' ? 0 : Number(size), oid }); + } + return entries; +} + +/** Splits `git cat-file --batch` output into blob contents, in request order. */ +function readBlobs(dir: string, oids: readonly string[]): Uint8Array[] { + if (oids.length === 0) return []; + const out = git([...inClone(dir), 'cat-file', '--batch'], 'git cat-file', `${oids.join('\n')}\n`); + const blobs: Uint8Array[] = []; + let at = 0; + for (const oid of oids) { + const newline = out.indexOf(0x0a, at); + const [id, type, size] = out.subarray(at, newline).toString('utf8').split(' '); + if (id !== oid || type !== 'blob' || size === undefined) throw new InstallError('unexpected_error', `git returned an unexpected object for ${oid}`); + const start = newline + 1; + blobs.push(new Uint8Array(out.subarray(start, start + Number(size)))); + at = start + Number(size) + 1; + } + return blobs; +} + +/** Clones `url` and returns a snapshot of its HEAD; the clone is removed by `dispose`. */ +export function openGitSource(url: string, label: string, ref?: string, transport = 'https'): SourceSnapshot { + const dir = mkdtempSync(join(tmpdir(), 'skillx-add-')); + const dispose = () => rmSync(dir, { recursive: true, force: true }); + try { + git(cloneArgs(url, join(dir, 'repo'), ref, transport), `cloning ${label}`); + const repo = join(dir, 'repo'); + let commit: string; + try { + commit = git([...inClone(repo), 'rev-parse', '--verify', 'HEAD^{commit}'], 'git rev-parse').toString('utf8').trim(); + } catch { + throw new InstallError('not_found', `${label} has no commit to read`); + } + const entries = listTree(repo); + const byPath = new Map(entries.map((e) => [e.path, e])); + const under = (d: string) => (p: string) => d === '' || p.startsWith(`${d}/`); + return { + label, + commit, + skillFiles: (searchPath) => + entries.filter((e) => e.kind === 'regular' && under(searchPath)(e.path) && (e.path === 'SKILL.md' || e.path.endsWith('/SKILL.md')) && isDiscoverable(e.path, searchPath)).map((e) => e.path), + hasDir: (path) => entries.some((e) => e.path.startsWith(`${path}/`)), + entriesUnder: (d) => entries.filter((e) => under(d)(e.path)).map(({ path, kind, mode, size }) => ({ path: d === '' ? path : path.slice(d.length + 1), kind, mode, size })), + read: (paths) => { + const blobs = readBlobs(repo, paths.map((p) => byPath.get(p)!.oid)); + return new Map(paths.map((p, i) => [p, blobs[i]!])); + }, + dispose, + }; + } catch (error) { + dispose(); + throw error; + } +} diff --git a/packages/cli/src/lib/add/local-source.ts b/packages/cli/src/lib/add/local-source.ts new file mode 100644 index 0000000..e29ae72 --- /dev/null +++ b/packages/cli/src/lib/add/local-source.ts @@ -0,0 +1,90 @@ +import { lstatSync, readFileSync, readdirSync, realpathSync } from 'node:fs'; +import type { Stats } from 'node:fs'; +import { join } from 'node:path'; +import { InstallError, usageError } from '../install/install-errors.js'; +import { containedPath, linkFreeParents } from '../install/safe-fs.js'; +import { LIMITS } from '@skillx/skill-tree'; +import type { EntryKind } from '@skillx/skill-tree'; +import type { ListedEntry, SourceSnapshot } from './skill-discovery.js'; +import { isDiscoverable } from './skill-discovery.js'; + +/** + * A local directory source. Everything is listed with `lstat` and links are + * never followed: a symlink or junction is listed as `symlink` (and refused + * by the tree rules if it sits inside a chosen skill), a regular file with + * more than one hard link as `hardlink` (refused too), anything else that is + * not a file or directory as `special`. A file is read only when it is still a + * regular, single-link file reached through real directories, so a swap after + * the listing is refused, not followed. + */ + +/** Discovery stops listing past this many entries: point at a smaller directory. */ +const MAX_DISCOVERY_ENTRIES = 100_000; + +function kindOf(stat: Stats): EntryKind { + if (stat.isSymbolicLink()) return 'symlink'; + if (stat.isDirectory()) return 'directory'; + if (!stat.isFile()) return 'special'; + return stat.nlink > 1 ? 'hardlink' : 'regular'; +} + +const modeOf = (stat: Stats) => ((stat.mode & 0o111) !== 0 ? '100755' : '100644'); + +/** Lists `root/rel` recursively; `enter` decides which directories to walk into. */ +function walk(root: string, rel: string, enter: (rel: string) => boolean, limit: number, tooMany: () => Error): ListedEntry[] { + const out: ListedEntry[] = []; + const visit = (dirRel: string) => { + const abs = dirRel === '' ? root : containedPath(root, dirRel); + for (const name of readdirSync(abs).sort()) { + const path = dirRel === '' ? name : `${dirRel}/${name}`; + const stat = lstatSync(join(abs, name)); + const kind = kindOf(stat); + if (kind === 'directory') { + if (enter(path)) visit(path); + continue; + } + out.push({ path, kind, mode: modeOf(stat), size: stat.size }); + if (out.length > limit) throw tooMany(); + } + }; + visit(rel); + return out; +} + +export function openLocalSource(dir: string): SourceSnapshot { + let root: string; + try { + root = realpathSync(dir); + if (!lstatSync(root).isDirectory()) throw new Error('not a directory'); + } catch { + throw usageError(`${dir} is not a directory`); + } + const strip = (d: string) => (e: ListedEntry) => ({ ...e, path: d === '' ? e.path : e.path.slice(d.length + 1) }); + return { + label: dir, + skillFiles: (searchPath) => + walk(root, searchPath, (rel) => isDiscoverable(`${rel}/SKILL.md`, searchPath), MAX_DISCOVERY_ENTRIES, () => + usageError(`more than ${MAX_DISCOVERY_ENTRIES} files under ${dir}; point at the skill directory or a smaller tree`), + ) + .filter((e) => e.kind === 'regular' && (e.path === 'SKILL.md' || e.path.endsWith('/SKILL.md'))) + .map((e) => e.path), + hasDir: (path) => linkFreeParents(root, `${path}/x`), + entriesUnder: (d) => + walk(root, d, () => true, LIMITS.maxEntries, () => + new InstallError('incompatible', `${d || dir} has more than ${LIMITS.maxEntries} entries`, { code: 'tree.limit_exceeded', dir: d }), + ).map(strip(d)), + read: (paths) => { + const out = new Map(); + for (const path of paths) { + const abs = containedPath(root, path); + const stat = lstatSync(abs); + if (!linkFreeParents(root, path) || kindOf(stat) !== 'regular') { + throw new InstallError('incompatible', `${path} changed into a link or special file while it was read`, { path }); + } + out.set(path, new Uint8Array(readFileSync(abs))); + } + return out; + }, + dispose: () => {}, + }; +} diff --git a/packages/cli/src/lib/add/skill-discovery.ts b/packages/cli/src/lib/add/skill-discovery.ts new file mode 100644 index 0000000..4bd25fa --- /dev/null +++ b/packages/cli/src/lib/add/skill-discovery.ts @@ -0,0 +1,127 @@ +import { LIMITS, buildTree, isNoisePath, isSkillTreeError } from '@skillx/skill-tree'; +import type { EntryKind, TreeEntry } from '@skillx/skill-tree'; +import type { VerifiedTree } from '../install/fetch-verify.js'; +import { sha256Hex } from '../install/fetch-verify.js'; +import { InstallError } from '../install/install-errors.js'; +import { readFrontmatter } from './skill-frontmatter.js'; + +/** + * Finds skills in a source and turns a chosen one into a checked tree. + * + * A source is a snapshot: a listing of entries (from git objects or an + * `lstat` walk, links never followed) and a reader for regular files. A skill + * is a directory holding a regular `SKILL.md`; when the search path itself is + * one, it is the only skill. Otherwise every such directory up to + * `MAX_DEPTH` below it counts, outside noise directories and `node_modules`, + * and a skill's own subdirectories are its content, not more skills. + * + * `prepareSkill` builds the tree with the registry's `agentkit.skill-tree/v1` + * rules: a symlink, junction, hardlink, submodule or other special entry, + * an unsafe or colliding path, or a tree over the limits refuses the skill + * (`incompatible`); nothing is followed or copied from outside it. + */ + +export interface ListedEntry { + /** POSIX path relative to the directory it was listed under. */ + path: string; + kind: EntryKind; + mode: string; + size: number; +} + +export interface SourceSnapshot { + /** Display name, never carrying credentials. */ + label: string; + /** Commit read, for a git source. */ + commit?: string; + /** Regular `SKILL.md` files under `searchPath` that discovery may consider, relative to the source root. */ + skillFiles(searchPath: string): string[]; + /** True when `path` names a directory of the source. */ + hasDir(path: string): boolean; + /** Every entry under directory `dir` ('' for the root), relative to it. */ + entriesUnder(dir: string): ListedEntry[]; + /** Bytes of regular files, by path relative to the source root. */ + read(paths: readonly string[]): Map; + dispose(): void; +} + +export interface FoundSkill { + name: string; + description: string; + /** Directory relative to the source root ('' for the root). */ + dir: string; +} + +export interface SkippedSkill { + dir: string; + reason: string; +} + +export const MAX_DEPTH = 6; +const SKIPPED_DIRS = new Set(['node_modules']); + +/** True when discovery may look at this `SKILL.md` path (depth, noise and skipped directories). */ +export function isDiscoverable(path: string, searchPath: string): boolean { + const rel = searchPath === '' ? path : path.slice(searchPath.length + 1); + const segments = rel.split('/'); + return segments.length - 1 <= MAX_DEPTH && !isNoisePath(rel) && !segments.slice(0, -1).some((s) => SKIPPED_DIRS.has(s)); +} + +const parentOf = (path: string) => (path.includes('/') ? path.slice(0, path.lastIndexOf('/')) : ''); +const join = (dir: string, name: string) => (dir === '' ? name : `${dir}/${name}`); + +export function discoverSkills(source: SourceSnapshot, searchPath: string): { skills: FoundSkill[]; skipped: SkippedSkill[] } { + if (searchPath !== '' && !source.hasDir(searchPath)) { + throw new InstallError('not_found', `${searchPath} is not a directory in ${source.label}`, { path: searchPath }); + } + const files = source.skillFiles(searchPath); + const root = join(searchPath, 'SKILL.md'); + const dirs = files.includes(root) ? [searchPath] : files.map(parentOf).sort(); + // A skill's subdirectories are its content: drop any skill nested inside another one. + const outer = dirs.filter((d) => !dirs.some((o) => o !== d && (o === '' || d.startsWith(`${o}/`)))); + const bytes = source.read(outer.map((d) => join(d, 'SKILL.md'))); + const skills: FoundSkill[] = []; + const skipped: SkippedSkill[] = []; + for (const dir of outer) { + const fm = readFrontmatter(bytes.get(join(dir, 'SKILL.md'))!); + if (fm.ok) skills.push({ name: fm.name, description: fm.description, dir }); + else skipped.push({ dir, reason: fm.reason }); + } + return { skills, skipped }; +} + +const refused = (skill: FoundSkill, detail: string, extra: Record = {}) => + new InstallError('incompatible', `cannot install ${skill.name} from ${skill.dir || 'the source root'}: ${detail}`, { skill: skill.name, dir: skill.dir, ...extra }); + +/** Checks the limits from the listing before any byte is read. */ +function checkListedLimits(skill: FoundSkill, entries: readonly ListedEntry[]): void { + if (entries.length > LIMITS.maxEntries) throw refused(skill, `more than ${LIMITS.maxEntries} entries`, { code: 'tree.limit_exceeded' }); + const files = entries.filter((e) => e.kind === 'regular'); + if (files.length > LIMITS.maxFiles) throw refused(skill, `more than ${LIMITS.maxFiles} files`, { code: 'tree.limit_exceeded' }); + const big = files.find((e) => e.size > LIMITS.maxFileSize); + if (big) throw refused(skill, `${big.path} is larger than ${LIMITS.maxFileSize} bytes`, { code: 'tree.limit_exceeded', path: big.path }); + if (files.reduce((sum, e) => sum + e.size, 0) > LIMITS.maxTreeSize) throw refused(skill, `more than ${LIMITS.maxTreeSize} bytes`, { code: 'tree.limit_exceeded' }); +} + +/** The skill's files as a checked tree, ready for the installer. */ +export function prepareSkill(source: SourceSnapshot, skill: FoundSkill): VerifiedTree { + const listed = source.entriesUnder(skill.dir); + checkListedLimits(skill, listed); + const regular = listed.filter((e) => e.kind === 'regular' && !isNoisePath(e.path)); + const bytes = source.read(regular.map((e) => join(skill.dir, e.path))); + const byPath = new Map(regular.map((e) => [e.path, bytes.get(join(skill.dir, e.path))!])); + const entries: TreeEntry[] = listed.map((e) => { + const data = byPath.get(e.path); + return { path: e.path, kind: e.kind, mode: e.mode, size: data?.length ?? e.size, sha256: data ? sha256Hex(data) : '0'.repeat(64) }; + }); + try { + const manifest = buildTree(entries); + return { + digest: manifest.digest, + files: manifest.files.map((f) => ({ path: f.path, mode: f.mode as '100644' | '100755', sha256: f.sha256, size: f.size, bytes: byPath.get(f.path)! })), + }; + } catch (error) { + if (isSkillTreeError(error)) throw refused(skill, error.message, { code: error.code, path: error.path }); + throw error; + } +} diff --git a/packages/cli/src/lib/add/skill-frontmatter.ts b/packages/cli/src/lib/add/skill-frontmatter.ts new file mode 100644 index 0000000..38f5dc3 --- /dev/null +++ b/packages/cli/src/lib/add/skill-frontmatter.ts @@ -0,0 +1,62 @@ +import { isInstallAlias } from '@skillx/contracts'; +import { parseDocument } from 'yaml'; + +/** + * SKILL.md front matter, read as data only. The block is the text between a + * first line `---` and the next `---` line (a UTF-8 BOM and CRLF are + * tolerated), parsed by `yaml` with the YAML 1.2 core schema. No engine is + * ever picked from the fence: `---js`, `---javascript` or `---coffee` is not + * a fence, so such a file has no front matter and is skipped, never run. A + * tag, a duplicate key or any parse problem rejects the block. + * + * The skill's `name` becomes its installed directory, so it must already be a + * valid install alias; it is never rewritten, so two names never merge. Names + * starting with `ak-` or `ak:` belong to AgentKit and are refused. + */ + +export type Frontmatter = { ok: true; name: string; description: string } | { ok: false; reason: string }; + +const isFence = (line: string | undefined) => line !== undefined && line.replace(/[ \t]+$/, '') === '---'; +const fail = (reason: string): Frontmatter => ({ ok: false, reason }); + +/** True for names AgentKit reserves for its kit skills, in any case. */ +export const isReservedSkillName = (name: string) => /^ak[-:]/i.test(name); + +export function readFrontmatter(bytes: Uint8Array): Frontmatter { + let text: string; + try { + text = new TextDecoder('utf-8', { fatal: true }).decode(bytes); + } catch { + return fail('SKILL.md is not valid UTF-8'); + } + const lines = text.replace(/^/, '').replace(/\r\n/g, '\n').split('\n'); + if (!isFence(lines[0])) return fail('SKILL.md has no front matter (the first line must be ---)'); + const end = lines.findIndex((line, i) => i > 0 && isFence(line)); + if (end < 0) return fail('SKILL.md front matter is not closed by a --- line'); + const doc = parseDocument(lines.slice(1, end).join('\n'), { version: '1.2', schema: 'core', uniqueKeys: true }); + if (doc.errors.length > 0 || doc.warnings.length > 0) return fail('SKILL.md front matter is not plain YAML'); + if (hasTags(doc.contents)) return fail('SKILL.md front matter uses YAML tags'); + let data: unknown; + try { + // Throws when aliases expand past the cap (a "billion laughs" block). + data = doc.toJS({ maxAliasCount: 10 }); + } catch { + return fail('SKILL.md front matter uses too many aliases'); + } + if (data === null || typeof data !== 'object' || Array.isArray(data)) return fail('SKILL.md front matter is not a mapping'); + const { name, description } = data as Record; + if (typeof name !== 'string' || name.trim() === '') return fail('SKILL.md front matter needs a non-empty name'); + if (typeof description !== 'string' || description.trim() === '') return fail('SKILL.md front matter needs a non-empty description'); + if (isReservedSkillName(name)) return fail(`'${name}' is reserved: ak- and ak: names belong to AgentKit`); + if (!isInstallAlias(name)) return fail(`'${name}' is not a valid skill directory name (lowercase letters, digits, '.', '_' and '-')`); + return { ok: true, name, description: description.trim() }; +} + +/** True when any node of the document carries an explicit tag. */ +function hasTags(node: unknown): boolean { + if (node === null || typeof node !== 'object') return false; + const n = node as { tag?: string; items?: unknown[]; key?: unknown; value?: unknown }; + if (typeof n.tag === 'string' && n.tag !== '') return true; + if (Array.isArray(n.items)) return n.items.some(hasTags); + return hasTags(n.key) || hasTags(n.value); +} diff --git a/packages/cli/src/lib/add/skill-safety.test.ts b/packages/cli/src/lib/add/skill-safety.test.ts new file mode 100644 index 0000000..815d489 --- /dev/null +++ b/packages/cli/src/lib/add/skill-safety.test.ts @@ -0,0 +1,167 @@ +import { linkSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { CliError } from '../output.js'; +import { openLocalSource } from './local-source.js'; +import { discoverSkills, prepareSkill } from './skill-discovery.js'; +import { readFrontmatter } from './skill-frontmatter.js'; + +const enc = (text: string) => new TextEncoder().encode(text); +const skillMd = (name: string, extra = '') => `---\nname: ${name}\ndescription: ${name} does things\n${extra}---\n# ${name}\n`; + +let root: string; +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'skillx-add-safety-')); +}); +afterEach(() => rmSync(root, { recursive: true, force: true })); + +function write(rel: string, text: string): string { + const path = join(root, ...rel.split('/')); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, text); + return path; +} + +const fileLinksWork = (() => { + const dir = mkdtempSync(join(tmpdir(), 'skillx-probe-')); + try { + writeFileSync(join(dir, 'a'), ''); + symlinkSync(join(dir, 'a'), join(dir, 'b'), 'file'); + return true; + } catch { + return false; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +})(); + +function refusedWith(run: () => unknown): CliError { + try { + run(); + } catch (error) { + expect(error).toBeInstanceOf(CliError); + return error as CliError; + } + throw new Error('expected a refusal'); +} + +describe('front matter is data, never code', () => { + it.each(['---js', '---javascript', '---coffee'])('treats a %s fence as no front matter and runs nothing', (fence) => { + const marker = '__skillx_frontmatter_ran__'; + const text = `${fence}\n(globalThis['${marker}'] = true, { name: 'evil', description: 'x' })\n---\n# body\n`; + const result = readFrontmatter(enc(text)); + expect(result).toEqual({ ok: false, reason: expect.stringContaining('no front matter') }); + expect((globalThis as Record)[marker]).toBeUndefined(); + }); + + it('refuses YAML tags, duplicate keys and non-mappings', () => { + expect(readFrontmatter(enc(skillMd('demo', 'x: !!js/function "function () {}"\n'))).ok).toBe(false); + expect(readFrontmatter(enc('---\nname: !!str demo\ndescription: d\n---\n')).ok).toBe(false); + expect(readFrontmatter(enc('---\nname: a\nname: b\ndescription: d\n---\n')).ok).toBe(false); + expect(readFrontmatter(enc('---\n- a\n---\n')).ok).toBe(false); + }); + + it('refuses alias abuse as a skipped skill instead of aborting discovery', () => { + const laughs = 'a: &a [x, x, x, x, x, x, x, x, x]\nb: &b [*a, *a, *a, *a, *a, *a, *a, *a, *a]\nc: &c [*b, *b, *b, *b, *b, *b, *b, *b, *b]\nd: [*c, *c, *c, *c, *c, *c, *c, *c, *c]\n'; + expect(readFrontmatter(enc(skillMd('bomb', laughs)))).toEqual({ ok: false, reason: 'SKILL.md front matter uses too many aliases' }); + write('skills/bomb/SKILL.md', skillMd('bomb', laughs)); + write('skills/fine/SKILL.md', skillMd('fine')); + const { skills, skipped } = discoverSkills(openLocalSource(root), ''); + expect(skills.map((s) => s.name)).toEqual(['fine']); + expect(skipped).toEqual([{ dir: 'skills/bomb', reason: 'SKILL.md front matter uses too many aliases' }]); + }); + + it('accepts plain front matter with a BOM and CRLF', () => { + expect(readFrontmatter(enc('---\r\nname: demo\r\ndescription: Does a thing\r\n---\r\n'))).toEqual({ ok: true, name: 'demo', description: 'Does a thing' }); + }); + + it.each(['ak-plan', 'AK-plan', 'ak:plan'])('refuses the AgentKit-reserved name %s', (name) => { + expect(readFrontmatter(enc(skillMd(name)))).toEqual({ ok: false, reason: expect.stringContaining('reserved') }); + }); + + it.each(['Foo Bar', 'con', 'trailing.', '../up', 'a/b'])('refuses %s as a directory name instead of rewriting it', (name) => { + expect(readFrontmatter(enc(skillMd(JSON.stringify(name)))).ok).toBe(false); + }); +}); + +describe('discovery', () => { + it('takes the search root alone when it is a skill', () => { + write('SKILL.md', skillMd('root-skill')); + write('skills/inner/SKILL.md', skillMd('inner')); + const { skills } = discoverSkills(openLocalSource(root), ''); + expect(skills.map((s) => s.name)).toEqual(['root-skill']); + }); + + it('finds nested skills, skips node_modules and noise, and reports refused ones', () => { + write('skills/a/SKILL.md', skillMd('a')); + write('skills/a/examples/sub/SKILL.md', skillMd('a-example')); + write('skills/b/SKILL.md', skillMd('ak-b')); + write('node_modules/pkg/SKILL.md', skillMd('vendored')); + write('.git/x/SKILL.md', skillMd('git-internal')); + const { skills, skipped } = discoverSkills(openLocalSource(root), ''); + expect(skills.map((s) => [s.name, s.dir])).toEqual([['a', 'skills/a']]); + expect(skipped).toEqual([{ dir: 'skills/b', reason: expect.stringContaining('reserved') }]); + }); + + it('never enters a linked directory', () => { + const outside = mkdtempSync(join(tmpdir(), 'skillx-outside-')); + try { + write('skills/real/SKILL.md', skillMd('real')); + mkdirSync(join(outside, 'secret'), { recursive: true }); + writeFileSync(join(outside, 'secret', 'SKILL.md'), skillMd('secret')); + symlinkSync(join(outside, 'secret'), join(root, 'skills', 'linked'), 'junction'); + const { skills } = discoverSkills(openLocalSource(root), ''); + expect(skills.map((s) => s.name)).toEqual(['real']); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); +}); + +describe('a chosen skill is refused, never copied, when it holds links', () => { + const found = { name: 'leaky', description: 'd', dir: 'leaky' }; + + it('refuses a junction inside the skill', () => { + write('leaky/SKILL.md', skillMd('leaky')); + mkdirSync(join(root, 'secret')); + writeFileSync(join(root, 'secret', 'id_rsa'), 'PRIVATE'); + symlinkSync(join(root, 'secret'), join(root, 'leaky', 'dirleak'), 'junction'); + const error = refusedWith(() => prepareSkill(openLocalSource(root), found)); + expect(error.code).toBe('incompatible'); + expect(error.extra.details).toMatchObject({ code: 'tree.unsupported_entry', path: 'dirleak' }); + }); + + it.skipIf(!fileLinksWork)('refuses a file symlink inside the skill', () => { + write('leaky/SKILL.md', skillMd('leaky')); + const secret = write('secret.txt', 'PRIVATE'); + symlinkSync(secret, join(root, 'leaky', 'leak'), 'file'); + expect(refusedWith(() => prepareSkill(openLocalSource(root), found)).extra.details).toMatchObject({ code: 'tree.unsupported_entry', path: 'leak' }); + }); + + it('refuses a hardlink inside the skill', () => { + write('leaky/SKILL.md', skillMd('leaky')); + const secret = write('secret.txt', 'PRIVATE'); + linkSync(secret, join(root, 'leaky', 'hard')); + expect(refusedWith(() => prepareSkill(openLocalSource(root), found)).extra.details).toMatchObject({ code: 'tree.unsupported_entry', path: 'hard' }); + }); + + it('builds a tree of regular files only, leaving out noise', () => { + write('leaky/SKILL.md', skillMd('leaky')); + write('leaky/references/guide.md', 'guide\n'); + write('leaky/.git/config', '[remote "origin"]\n\turl = https://token@host/x\n'); + const tree = prepareSkill(openLocalSource(root), found); + expect(tree.files.map((f) => f.path)).toEqual(['SKILL.md', 'references/guide.md']); + expect(tree.digest).toMatch(/^sha256:[0-9a-f]{64}$/); + }); + + it('refuses a missing search path as not found', () => { + write('a/SKILL.md', skillMd('a')); + expect(refusedWith(() => discoverSkills(openLocalSource(root), 'nope')).code).toBe('not_found'); + }); + + it('refuses a local source that is not a directory', () => { + const file = write('file.txt', 'x'); + expect(refusedWith(() => openLocalSource(file)).code).toBe('usage_error'); + }); +}); diff --git a/packages/cli/src/lib/install/install-engine.ts b/packages/cli/src/lib/install/install-engine.ts index 0c7dad7..c5a5f42 100644 --- a/packages/cli/src/lib/install/install-engine.ts +++ b/packages/cli/src/lib/install/install-engine.ts @@ -5,7 +5,7 @@ import type { Recovery } from './apply-transaction.js'; import { fetchVerified } from './fetch-verify.js'; import type { VerifiedTree } from './fetch-verify.js'; import type { InstallContext } from './install-context.js'; -import { conflictError } from './install-errors.js'; +import { conflictError, integrityError } from './install-errors.js'; import { RECEIPT_SCHEMA, listReceipts, readOwners, readReceipt, writeOwners, writeReceipt } from './install-state.js'; import type { InstallReceipt } from './install-state.js'; import { packagesToFetch, planInstall } from './planner.js'; @@ -28,6 +28,12 @@ export interface OwnedLock { ref: string; lock: InstallLock; overlays: Map; + /** + * Trees already read and checked by the caller, by alias (`skillx add`, whose + * bytes come from git or a local path, not the registry). When set, nothing + * is fetched: a planned alias without a tree here is an integrity error. + */ + trees?: ReadonlyMap; } /** Splits a lock into owners: kits and collections own all their packages; explicit packages own themselves. */ @@ -90,7 +96,11 @@ export async function applyOwned(ctx: InstallContext, owned: OwnedLock, plan: In refuseConflicts(plan); const trees = new Map(); const deps = { registry: ctx.registry, keys: ctx.keys, cacheDir: join(ctx.home, 'cache', 'blobs'), runtime: ctx.runtime }; - for (const pkg of packagesToFetch(plan, owned.lock)) trees.set(pkg.alias, await fetchVerified(deps, pkg, owned.overlays.get(pkg.alias) ?? [])); + for (const pkg of packagesToFetch(plan, owned.lock)) { + const given = owned.trees?.get(pkg.alias); + if (owned.trees !== undefined && given === undefined) throw integrityError(`no checked tree for ${pkg.alias}`, { alias: pkg.alias }); + trees.set(pkg.alias, given ?? (await fetchVerified(deps, pkg, owned.overlays.get(pkg.alias) ?? []))); + } const result: ApplyResult = { owner: owned.owner, planDigest: plan.planDigest, changed: [], removed: [] }; const byAlias = new Map(owned.lock.packages.map((p) => [p.alias, p])); diff --git a/packages/cli/src/lib/install/install-operations.ts b/packages/cli/src/lib/install/install-operations.ts index cd8d3ab..75c8a13 100644 --- a/packages/cli/src/lib/install/install-operations.ts +++ b/packages/cli/src/lib/install/install-operations.ts @@ -1,11 +1,11 @@ -import { canonicalJsonBytes, lockDigest } from '@skillx/contracts'; +import { canonicalJsonBytes, isInstallAlias, lockDigest } from '@skillx/contracts'; import type { InstallLock } from '@skillx/contracts'; import { sha256Hex } from './fetch-verify.js'; import { applyOwned, ownedLocks, planOwned, recover } from './install-engine.js'; import type { ApplyResult, OwnedLock } from './install-engine.js'; import type { InstallContext } from './install-context.js'; import { withInstallLock } from './install-context.js'; -import { InstallError, conflictError } from './install-errors.js'; +import { InstallError, conflictError, usageError } from './install-errors.js'; import { listReceipts, readOwners } from './install-state.js'; import type { InstallPlan } from './planner.js'; import { checkRuntimes, parseRefs, resolveSource } from './resolver.js'; @@ -17,6 +17,10 @@ import type { Resolution, SourceRef } from './resolver.js'; * recovers interrupted transactions first and then plans afresh. */ +/** Owner key prefix of a skill installed by `skillx add`: one owner per skill, `add:`. */ +export const ADD_OWNER_PREFIX = 'add:'; +export const isAddOwner = (owner: string) => owner.startsWith(ADD_OWNER_PREFIX); + export interface PlannedItem extends OwnedLock { plan: InstallPlan; } @@ -80,6 +84,13 @@ export const installRefs = (ctx: InstallContext, refs: readonly string[], expect * splits them, so each package is updated and uninstalled on its own. */ export function ownersOfRefs(refs: readonly string[]): string[] { + const added = refs.filter(isAddOwner); + if (added.length > 0) { + if (added.length !== refs.length) throw usageError(`name ${ADD_OWNER_PREFIX} owners apart from registry sources`); + const bad = added.find((r) => !isInstallAlias(r.slice(ADD_OWNER_PREFIX.length))); + if (bad !== undefined) throw usageError(`'${bad}' is not ${ADD_OWNER_PREFIX}`); + return [...new Set(added)]; + } const ref = parseRefs(refs); if (ref.kind === 'kit') return [`kit:${ref.kit}`]; if (ref.kind === 'collection') return [`collection:${ref.id}`]; @@ -109,10 +120,19 @@ export async function uninstallRef(ctx: InstallContext, refs: readonly string[], }); } -/** Re-resolves the named installed sources (all when none is named) and applies the changes. */ +/** + * Re-resolves the named installed sources (all when none is named) and applies + * the changes. Skills from `skillx add` have no registry source to re-resolve: + * updating all leaves them alone, and naming one is a usage error that points + * at `skillx add --force`. + */ export async function updateInstalled(ctx: InstallContext, refs: readonly string[]): Promise { const records = readOwners(ctx.home, ctx.runtime).owners; - const wanted = refs.length > 0 ? ownersOfRefs(refs) : Object.keys(records).sort(); + const named = refs.length > 0 ? ownersOfRefs(refs) : []; + if (named.some(isAddOwner)) { + throw new InstallError('usage_error', `${named.join(', ')} came from skillx add; refresh with skillx add --force`, { sources: named.map((o) => records[o]?.ref ?? null) }); + } + const wanted = refs.length > 0 ? named : Object.keys(records).filter((o) => !isAddOwner(o)).sort(); const outcomes: ApplyOutcome[] = []; for (const owner of wanted) { const record = records[owner];