From ad16279680d326c9ba5e9543dd6d59bcef94d8fd Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 19:31:24 +0700 Subject: [PATCH 01/12] refactor(cli): delegate actionability to isCompatibilityActionable Bundle @skillx/contracts into the CLI (workspace devDependency, tsup noExternal) and drop the hand-written mirror of the actionability rule. The CLI now decides every status in COMPATIBILITY_STATUSES exactly as the contract does, which the parity notes no longer have to excuse. Closes #48 Co-Authored-By: Claude Opus 5.5 --- .../lib/parity/compatibility-parity.test.ts | 12 ++++++----- packages/cli/src/lib/skill-lookup.test.ts | 10 +++++++++ packages/cli/src/lib/skill-lookup.ts | 21 +++++++++++++------ packages/cli/tsup.config.ts | 3 +++ packages/contracts/src/compatibility.test.ts | 9 +++----- 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/apps/web/app/lib/parity/compatibility-parity.test.ts b/apps/web/app/lib/parity/compatibility-parity.test.ts index 98bac6c..175b7b0 100644 --- a/apps/web/app/lib/parity/compatibility-parity.test.ts +++ b/apps/web/app/lib/parity/compatibility-parity.test.ts @@ -11,11 +11,13 @@ * layer stubbed, so it exercises the handler and the protocol envelope rather * than a re-implementation of them. * - * The CLI is not imported here: `apps/web/tsconfig.cloudflare.json` lists its own - * files only, so a cross-package import fails the project boundary. The CLI's - * rule is pinned by value in packages/cli/src/lib/skill-lookup.test.ts, and its - * drift risk is that it is the one surface holding a hand-written mirror of - * `isCompatibilityActionable` instead of calling it. + * The CLI is covered from its own package, because `apps/web/tsconfig.cloudflare.json` + * lists the app's files only. It imports `isCompatibilityActionable` from this + * contract (bundled into the shipped binary), and two tests pin it: one compares + * the CLI decision with the contract for every status in COMPATIBILITY_STATUSES + * (packages/cli/src/lib/skill-lookup.test.ts), and one runs the built + * `bin/skillx.js check --json` against a stub API for all five statuses + * (packages/cli/src/built-cli.test.ts). */ import { beforeEach, describe, expect, it, vi } from "vitest"; diff --git a/packages/cli/src/lib/skill-lookup.test.ts b/packages/cli/src/lib/skill-lookup.test.ts index 1ce60b6..f892e9d 100644 --- a/packages/cli/src/lib/skill-lookup.test.ts +++ b/packages/cli/src/lib/skill-lookup.test.ts @@ -17,6 +17,7 @@ vi.mock("./api-client.js", () => ({ }, })); +import { COMPATIBILITY_STATUSES, isCompatibilityActionable } from "@skillx/contracts"; import { fetchSkillInspection, isActionable } from "./skill-lookup.js"; beforeEach(() => { @@ -73,6 +74,15 @@ describe("isActionable", () => { expect(isActionable("blocked")).toBe(false); }); + it.each([...COMPATIBILITY_STATUSES])("decides %s exactly as the contract does", (status) => { + expect(isActionable(status)).toBe(isCompatibilityActionable(status)); + }); + + it("never permits action for a status outside the contract's vocabulary", () => { + expect(isActionable("supported")).toBe(false); + expect(isActionable("")).toBe(false); + }); + it("treats a missing answer as not actionable", () => { expect(isActionable(null)).toBe(false); expect(isActionable(undefined)).toBe(false); diff --git a/packages/cli/src/lib/skill-lookup.ts b/packages/cli/src/lib/skill-lookup.ts index fba78fe..601e06b 100644 --- a/packages/cli/src/lib/skill-lookup.ts +++ b/packages/cli/src/lib/skill-lookup.ts @@ -6,13 +6,14 @@ * uses `use`'s identifier parsing and endpoint builder, so a slug (or source * identity) that `use` finds is the one `inspect` reports. * - * The compatibility shapes here mirror the API's JSON. They are declared locally - * because this package is a thin HTTP client and does not depend on - * `@skillx/contracts`; a contract test guards the two from drifting. + * The compatibility shapes here describe the API's JSON as received, so status + * fields stay `string`. The decision about a status is never made here: it is + * `isCompatibilityActionable` from `@skillx/contracts`, bundled into the CLI. */ +import { COMPATIBILITY_STATUSES, isCompatibilityActionable, type CompatibilityStatus } from '@skillx/contracts'; import { apiRequest } from './api-client.js'; -import { buildSkillEndpoint, parseIdentifier, parseRepoPath } from '../commands/use.js'; +import { buildSkillEndpoint, parseIdentifier, parseRepoPath } from './skill-resolver.js'; import type { SkillReference, SkillScript } from '../commands/use-display.js'; export interface CompatibilityRequirement { @@ -139,7 +140,15 @@ function resolveLookupTarget(identifier: string): { slug: string; identity?: { r } } -/** True when the answer permits action without further evidence. */ +/** True when the API sent a status from the contract's vocabulary. */ +function isKnownStatus(status: string | null | undefined): status is CompatibilityStatus { + return (COMPATIBILITY_STATUSES as readonly string[]).includes(status ?? ''); +} + +/** + * True when the answer permits action without further evidence. The rule is the + * contract's; a status outside its vocabulary (or none) is never actionable. + */ export function isActionable(status: string | null | undefined): boolean { - return status === 'declared' || status === 'verified'; + return isKnownStatus(status) && isCompatibilityActionable(status); } diff --git a/packages/cli/tsup.config.ts b/packages/cli/tsup.config.ts index 0854ae7..5f7a92b 100644 --- a/packages/cli/tsup.config.ts +++ b/packages/cli/tsup.config.ts @@ -27,6 +27,9 @@ export default defineConfig({ splitting: false, sourcemap: true, shims: true, + // Workspace packages ship TypeScript source and are never published, so they are inlined into + // the bundle. Their own runtime dependencies (e.g. `yaml`) stay external via `dependencies`. + noExternal: ['@skillx/contracts', '@skillx/skill-tree'], define: { __SKILLX_VERSION__: JSON.stringify(readCliVersion()), }, diff --git a/packages/contracts/src/compatibility.test.ts b/packages/contracts/src/compatibility.test.ts index 9296e4a..8b541c7 100644 --- a/packages/contracts/src/compatibility.test.ts +++ b/packages/contracts/src/compatibility.test.ts @@ -168,12 +168,9 @@ describe("compatibility engine — five states", () => { expect(validateCompatibilityDeclaration(COMPATIBILITY_DECLARATION_FIXTURE).valid).toBe(true); }); - it("pins the vocabulary, because the CLI mirrors it by hand", () => { - // `packages/cli/src/lib/skill-lookup.ts` does not import this package: its - // tsconfig sets `rootDir: ./src` and its bundler does not resolve a - // cross-package path, so `isActionable` enumerates these literals instead. - // Adding a status here would otherwise leave that mirror silently wrong, and - // no test in either package would notice. + it("pins the vocabulary", () => { + // Every surface (API, CLI, MCP, Markdown) reads its decisions from this list; + // changing it is a public contract change and must be deliberate. expect([...COMPATIBILITY_STATUSES].sort()).toEqual([ "blocked", "declared", From abe719c82f77094c5cb9a410e0dc98c8e6300b08 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 19:31:24 +0700 Subject: [PATCH 02/12] feat(cli): one JSON envelope and exit-code table for every command Add runCommand/emit/CliError so config, report, use and find join check, inspect and search on the same --json envelope and exit codes. Parse errors exit 2 with a usage_error envelope, 429 maps to rate_limited (7), and a programming TypeError is no longer reported as a network outage. Exit codes are set on process.exitCode so piped stdout drains. find now reads the search API's real shape (avg_rating, results) and opens the pick through use, instead of crashing on undefined fields. Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/built-cli.test.ts | 76 +++++- packages/cli/src/commands/check.ts | 133 +++++----- packages/cli/src/commands/config.ts | 164 ++++++------ packages/cli/src/commands/find.ts | 168 ++++-------- packages/cli/src/commands/inspect.ts | 41 ++- packages/cli/src/commands/report.ts | 118 ++++----- packages/cli/src/commands/search.ts | 106 ++++---- packages/cli/src/commands/use-display.ts | 2 +- packages/cli/src/commands/use.ts | 314 ++++++----------------- packages/cli/src/index.ts | 5 +- packages/cli/src/lib/output.test.ts | 25 +- packages/cli/src/lib/output.ts | 53 +++- packages/cli/src/lib/run-command.ts | 121 +++++++++ packages/cli/src/lib/skill-resolver.ts | 215 ++++++++++++++++ packages/cli/src/utils/config-store.ts | 11 + 15 files changed, 903 insertions(+), 649 deletions(-) create mode 100644 packages/cli/src/lib/run-command.ts create mode 100644 packages/cli/src/lib/skill-resolver.ts diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 3483d7b..10c4d55 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -8,6 +8,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; +import { COMPATIBILITY_STATUSES, isCompatibilityActionable } from "@skillx/contracts"; /** * Smoke tests of the built CLI, run exactly the way the release ships it: `pnpm build` (tsup) and @@ -35,6 +36,17 @@ async function cli(...args: string[]) { return execFileAsync(process.execPath, [BIN, ...args], { cwd: root, env: childEnv, encoding: "utf8" }); } +/** Runs the built CLI and keeps a non-zero exit as data, since exit codes are part of the contract. */ +async function cliExit(...args: string[]): Promise<{ code: number; stdout: string }> { + try { + const { stdout } = await cli(...args); + return { code: 0, stdout }; + } catch (error) { + const failed = error as { code?: number; stdout?: string }; + return { code: failed.code ?? -1, stdout: failed.stdout ?? "" }; + } +} + beforeAll(() => { // Through a shell: on Windows pnpm is a .cmd shim that execFile cannot start directly. execSync("pnpm build", { cwd: CLI_DIR, stdio: "pipe" }); @@ -164,4 +176,66 @@ describe("built cli", () => { for (const command of ["import", "validate", "pack", "publish", "promote", "release"]) expect(help).toContain(command); expect((await cli("promote", "--help")).stdout).toContain("--to "); }); -}); \ No newline at end of file + + it("inlines @skillx/contracts instead of leaving a workspace import", () => { + const bundle = readFileSync(BUNDLE, "utf8"); + expect(bundle).not.toMatch(/["']@skillx\//); + expect(bundle).toContain("isCompatibilityActionable"); + }); + + it("answers check --json for all five statuses with the contract's decision and exit code", async () => { + const server = createServer((req, res) => { + const url = new URL(String(req.url), "http://stub"); + const status = url.pathname.replace("/api/skills/s-", ""); + if (!(COMPATIBILITY_STATUSES as readonly string[]).includes(status)) { + res.writeHead(404, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "not found" })); + return; + } + const target = { runtime: "agentkit", status, versions: null, scopes: [], requirements: [], evidence: null, reasons: [] }; + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ skill: { slug: `s-${status}` }, compatibility: { declared: [], target } })); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + try { + const { port } = server.address() as AddressInfo; + const setUrl = await cliExit("config", "set-url", `http://127.0.0.1:${port}`, "--json"); + expect(setUrl.code).toBe(0); + expect(JSON.parse(setUrl.stdout)).toEqual({ ok: true, data: { baseUrl: `http://127.0.0.1:${port}` } }); + + for (const status of COMPATIBILITY_STATUSES) { + const { code, stdout } = await cliExit("check", `s-${status}`, "--target", "agentkit@1.2.0", "--json"); + const envelope = JSON.parse(stdout) as { ok: boolean; data: { status: string; actionable: boolean } }; + const actionable = isCompatibilityActionable(status); + expect(envelope).toMatchObject({ ok: true, data: { status, actionable } }); + expect(code).toBe(actionable ? 0 : 4); + } + + const missing = await cliExit("check", "s-nope", "--target", "agentkit", "--json"); + expect(missing.code).toBe(3); + expect(JSON.parse(missing.stdout)).toMatchObject({ ok: false, error: { code: "not_found" } }); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }); + + it("reports a parse error as usage_error with exit 2", async () => { + const { code, stdout } = await cliExit("check", "some-skill", "--json"); + expect(code).toBe(2); + expect(JSON.parse(stdout)).toMatchObject({ ok: false, error: { code: "usage_error" } }); + }); + + it("validates report arguments before any request", async () => { + const { code, stdout } = await cliExit("report", "some-skill", "maybe", "--json"); + expect(code).toBe(2); + expect(JSON.parse(stdout)).toMatchObject({ ok: false, error: { code: "usage_error", details: { outcome: "maybe" } } }); + }); + + it("shows config as an envelope without the full key", async () => { + const { code, stdout } = await cliExit("config", "show", "--json"); + const envelope = JSON.parse(stdout) as { ok: boolean; data: { apiKey: { configured: boolean; source: string; masked: string } } }; + expect(code).toBe(0); + expect(envelope.data.apiKey).toMatchObject({ configured: true, source: "env" }); + expect(stdout).not.toContain("sx_test_smoke_key"); + }); +}); diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 75c4e28..7606172 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -1,83 +1,86 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import { fetchSkillInspection, isActionable } from '../lib/skill-lookup.js'; -import { EXIT_CODES, exitCodeFor, failure, fromError, serialize, success } from '../lib/output.js'; +import { fetchSkillInspection, isActionable, type CompatibilityAnswer } from '../lib/skill-lookup.js'; +import { CliError, EXIT_CODES } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; interface CheckOptions { target: string; json?: boolean; } +export interface CheckResult { + slug: string; + target: { runtime: string; version: string | null }; + status: string; + versions: string | null; + requirements: CompatibilityAnswer['requirements']; + evidence: CompatibilityAnswer['evidence']; + reasons: CompatibilityAnswer['reasons']; + actionable: boolean; +} + +/** Builds the `check` result from the API's target answer; the decision is the contract's. */ +export function toCheckResult(slug: string, requested: string, answer: CompatibilityAnswer): CheckResult { + return { + slug, + target: { runtime: answer.runtime, version: requested.includes('@') ? requested.split('@')[1] : null }, + status: answer.status, + versions: answer.versions, + requirements: answer.requirements, + evidence: answer.evidence, + reasons: answer.reasons, + actionable: isActionable(answer.status), + }; +} + +function render(result: CheckResult): void { + const { actionable } = result; + const paint = actionable ? chalk.green : chalk.red; + console.log(`\n${paint(actionable ? '✓' : '✗')} ${result.slug} on ${result.target.runtime}: ${paint(result.status)}`); + if (result.versions) console.log(chalk.dim(` declared range ${result.versions}`)); + for (const reason of result.reasons) { + console.log(` ${chalk.bold(reason.code)} ${reason.message}`); + } + if (result.evidence) { + console.log( + chalk.dim(` evidence ${result.evidence.probeId} by ${result.evidence.verifier} at ${result.evidence.verifiedAt}`), + ); + } + if (!actionable) { + console.log(chalk.dim('\nOnly `declared` and `verified` mean the skill will run. A missing declaration is not support.')); + } + console.log(); +} + /** * `check` is a gate, so its exit code is the answer: 0 when the target may be * acted on, 4 when it may not. A caller can branch on that without reading text. + * The envelope is `ok: true` either way, because the check itself succeeded. */ export const checkCommand = new Command('check') .description('Decide whether a skill can run on a target, and print the reason') .argument('', 'slug, author/skill, or org/repo/skill') .requiredOption('--target ', 'runtime to evaluate, optionally runtime@version') .option('--json', 'emit a machine-readable envelope instead of text') - .action(async (identifier: string, options: CheckOptions) => { - const useJson = options.json === true; - - try { - const { slug, inspection } = await fetchSkillInspection(identifier, options.target); - const answer = inspection.compatibility?.target; - - if (!answer) { + .action((identifier: string, options: CheckOptions) => + runCommand( + { + json: options.json === true, + exitCode: (result: CheckResult) => (result.actionable ? EXIT_CODES.ok : EXIT_CODES.incompatible), + errors: { notFoundMessage: `Skill not found: ${identifier}` }, + }, + async () => { + const { slug, inspection } = await fetchSkillInspection(identifier, options.target); + const answer = inspection.compatibility?.target; // The API returns no target answer only when the request never carried one. - const envelope = failure('unexpected_error', 'The API did not return a target answer.', { - details: { slug, target: options.target }, - }); - if (useJson) console.log(serialize(envelope)); - else console.error(chalk.red(`\n✗ ${envelope.error.message}`)); - process.exit(exitCodeFor(envelope)); - } - - const actionable = isActionable(answer.status); - const payload = { - slug, - target: { runtime: answer.runtime, version: options.target.includes('@') ? options.target.split('@')[1] : null }, - status: answer.status, - versions: answer.versions, - requirements: answer.requirements, - evidence: answer.evidence, - reasons: answer.reasons, - actionable, - }; - - if (useJson) { - const envelope = success(payload); - console.log(serialize(envelope)); - process.exit(actionable ? EXIT_CODES.ok : EXIT_CODES.incompatible); - } - - const paint = actionable ? chalk.green : chalk.red; - console.log(`\n${paint(actionable ? '✓' : '✗')} ${slug} on ${answer.runtime}: ${paint(answer.status)}`); - if (answer.versions) console.log(chalk.dim(` declared range ${answer.versions}`)); - for (const reason of answer.reasons) { - console.log(` ${chalk.bold(reason.code)} ${reason.message}`); - } - if (answer.evidence) { - console.log( - chalk.dim( - ` evidence ${answer.evidence.probeId} by ${answer.evidence.verifier} at ${answer.evidence.verifiedAt}`, - ), - ); - } - if (!actionable) { - console.log( - chalk.dim( - '\nOnly `declared` and `verified` mean the skill will run. A missing declaration is not support.', - ), - ); - } - console.log(); - process.exit(actionable ? EXIT_CODES.ok : EXIT_CODES.incompatible); - } catch (error) { - const envelope = fromError(error); - if (useJson) console.log(serialize(envelope)); - else console.error(chalk.red(`\n✗ ${envelope.error.message}`)); - process.exit(exitCodeFor(envelope)); - } - }); + if (!answer) { + throw new CliError('unexpected_error', 'The API did not return a target answer.', { + details: { slug, target: options.target }, + }); + } + return toCheckResult(slug, options.target, answer); + }, + render, + ), + ); diff --git a/packages/cli/src/commands/config.ts b/packages/cli/src/commands/config.ts index a216aec..b6a09ef 100644 --- a/packages/cli/src/commands/config.ts +++ b/packages/cli/src/commands/config.ts @@ -1,91 +1,111 @@ import { Command } from 'commander'; import chalk from 'chalk'; import * as readline from 'node:readline/promises'; -import { stdin as input, stdout as output } from 'node:process'; -import { getApiKey, setApiKey, getBaseUrl, setBaseUrl } from '../utils/config-store.js'; +import { stdin as input, stdout, stderr } from 'node:process'; +import { CliError } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; +import { + getApiKey, + getApiKeySource, + getBaseUrl, + getConfigPath, + setApiKey, + setBaseUrl, +} from '../utils/config-store.js'; -export const configCommand = new Command('config') - .description('Manage SkillX CLI configuration'); +interface JsonOption { + json?: boolean; +} -configCommand - .command('set-key') - .description('Set your SkillX API key') - .action(async () => { - const rl = readline.createInterface({ input, output }); +/** Never prints a whole key: a prefix, a mask, and the last four characters. */ +export function maskApiKey(apiKey: string): string { + return `${apiKey.substring(0, 8)}${'*'.repeat(Math.max(0, apiKey.length - 12))}${apiKey.substring(Math.max(8, apiKey.length - 4))}`; +} - try { - console.log(chalk.bold('\nSkillX API Key Configuration')); - console.log(chalk.dim('Get your API key from: https://skillx.sh/settings/api\n')); +function apiKeyState() { + const apiKey = getApiKey(); + return { configured: Boolean(apiKey), source: getApiKeySource(), masked: apiKey ? maskApiKey(apiKey) : null }; +} - const apiKey = await rl.question(chalk.cyan('Enter your API key: ')); +export const configCommand = new Command('config').description('Manage SkillX CLI configuration'); - if (!apiKey || apiKey.trim().length === 0) { - console.error(chalk.red('\n✗ API key cannot be empty')); - process.exit(1); - } +configCommand + .command('set-key') + .description('Set your SkillX API key (read from the prompt, or from stdin when piped)') + .option('--json', 'emit a machine-readable envelope; the prompt goes to stderr') + .action((options: JsonOption) => { + const json = options.json === true; + return runCommand( + { json }, + async () => { + // With --json, stdout carries only the envelope, so the prompt is written to stderr. + const rl = readline.createInterface({ input, output: json ? stderr : stdout }); + let apiKey: string; + try { + if (!json) { + console.log(chalk.bold('\nSkillX API Key Configuration')); + console.log(chalk.dim('Get your API key from: https://skillx.sh/settings/api\n')); + } + apiKey = (await rl.question(json ? 'Enter your API key: ' : chalk.cyan('Enter your API key: '))).trim(); + } finally { + rl.close(); + } + if (apiKey.length === 0) throw new CliError('usage_error', 'API key cannot be empty.'); - setApiKey(apiKey.trim()); - console.log(chalk.green('\n✓ API key saved successfully')); - console.log( - chalk.dim( - `Stored in: ${process.platform === 'win32' ? '%APPDATA%' : '~'}/.config/skillx/config.json` - ) - ); - } catch (error) { - if (error instanceof Error) { - console.error(chalk.red(`\n✗ Error: ${error.message}`)); - } - process.exit(1); - } finally { - rl.close(); - } + setApiKey(apiKey); + return { saved: true, path: getConfigPath(), apiKey: apiKeyState() }; + }, + (result) => { + console.log(chalk.green('\n✓ API key saved successfully')); + console.log(chalk.dim(`Stored in: ${result.path}`)); + if (result.apiKey.source === 'env') { + console.log(chalk.yellow('SKILLX_API_KEY is set and takes precedence over the saved key.')); + } + }, + ); }); configCommand .command('set-url') .description('Set custom API base URL (default: https://skillx.sh)') .argument('', 'Base URL for SkillX API') - .action((url: string) => { - try { - new URL(url); - setBaseUrl(url); - console.log(chalk.green(`\n✓ Base URL set to: ${url}`)); - } catch { - console.error(chalk.red('\n✗ Invalid URL format')); - console.error(chalk.dim('Example: https://api.skillx.sh')); - process.exit(1); - } - }); + .option('--json', 'emit a machine-readable envelope instead of text') + .action((url: string, options: JsonOption) => + runCommand( + { json: options.json === true }, + async () => { + try { + new URL(url); + } catch { + throw new CliError('usage_error', `Invalid URL format: ${url}`, { hint: 'Example: https://api.skillx.sh' }); + } + setBaseUrl(url); + return { baseUrl: url }; + }, + (result) => console.log(chalk.green(`\n✓ Base URL set to: ${result.baseUrl}`)), + ), + ); configCommand .command('show') .description('Display current configuration') - .action(() => { - const apiKey = getApiKey(); - const baseUrl = getBaseUrl(); - - console.log(chalk.bold('\nSkillX Configuration:\n')); - console.log(chalk.cyan('Base URL:'), baseUrl); - - if (apiKey) { - const masked = `${apiKey.substring(0, 8)}${'*'.repeat(Math.max(0, apiKey.length - 12))}${apiKey.substring(Math.max(8, apiKey.length - 4))}`; - console.log(chalk.cyan('API Key: '), masked); - - if (process.env.SKILLX_API_KEY) { - console.log(chalk.dim(' (loaded from SKILLX_API_KEY environment variable)')); - } else { - console.log( - chalk.dim( - ` (loaded from ${process.platform === 'win32' ? '%APPDATA%' : '~'}/.config/skillx/config.json)` - ) - ); - } - } else { - console.log(chalk.cyan('API Key: '), chalk.dim('not configured')); - console.log( - chalk.dim(` Run ${chalk.cyan('skillx config set-key')} to configure`) - ); - } - - console.log(); - }); + .option('--json', 'emit a machine-readable envelope instead of text') + .action((options: JsonOption) => + runCommand( + { json: options.json === true }, + async () => ({ baseUrl: getBaseUrl(), path: getConfigPath(), apiKey: apiKeyState() }), + (state) => { + console.log(chalk.bold('\nSkillX Configuration:\n')); + console.log(chalk.cyan('Base URL:'), state.baseUrl); + if (state.apiKey.masked) { + console.log(chalk.cyan('API Key: '), state.apiKey.masked); + const from = state.apiKey.source === 'env' ? 'SKILLX_API_KEY environment variable' : state.path; + console.log(chalk.dim(` (loaded from ${from})`)); + } else { + console.log(chalk.cyan('API Key: '), chalk.dim('not configured')); + console.log(chalk.dim(` Run ${chalk.cyan('skillx config set-key')} to configure`)); + } + console.log(); + }, + ), + ); diff --git a/packages/cli/src/commands/find.ts b/packages/cli/src/commands/find.ts index 564cfe0..8aa81cc 100644 --- a/packages/cli/src/commands/find.ts +++ b/packages/cli/src/commands/find.ts @@ -1,131 +1,61 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import ora from 'ora'; import * as readline from 'node:readline/promises'; import { stdin as input, stdout as output } from 'node:process'; -import { apiRequest, ApiError } from '../lib/api-client.js'; - -interface SearchResult { - slug: string; - name: string; - category: string; - rating: number; - description: string; -} - -interface SearchResponse { - results: SearchResult[]; - total: number; -} - -interface SkillDetails { - slug: string; - name: string; - description: string; - category: string; - rating: number; - install_command?: string; - content: string; +import { searchSkills, type SearchResult } from '../lib/search-api.js'; +import { runCommand } from '../lib/run-command.js'; +import { resolveAndUseSkill } from './use.js'; + +/** Lists the hits, asks which one to open, and opens it the way `use ` does. */ +async function pickAndUse(results: SearchResult[]): Promise { + if (results.length === 0) { + console.log(chalk.yellow('\nNo skills found matching your query.')); + console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); + return; + } + + console.log(chalk.bold.green(`\n✓ Found ${results.length} skill(s)\n`)); + results.forEach((skill, i) => { + const rating = chalk.yellow(`⭐ ${(skill.avg_rating ?? 0).toFixed(1)}`); + console.log(`${chalk.bold.white(`[${i + 1}]`)} ${chalk.cyan(skill.name)} ${chalk.magenta(skill.category)} ${rating}`); + console.log(` ${chalk.dim(truncate(skill.description, 80))}`); + }); + console.log(); + + const rl = readline.createInterface({ input, output }); + let answer: string; + try { + answer = await rl.question(chalk.cyan(`Select a skill [1-${results.length}] or press Enter to cancel: `)); + } finally { + rl.close(); + } + + const choice = parseInt(answer.trim(), 10); + if (isNaN(choice) || choice < 1 || choice > results.length) { + console.log(chalk.dim('\nCancelled.')); + return; + } + await resolveAndUseSkill(results[choice - 1].slug, { raw: false }); } +/** + * `find` is interactive for people. With `--json` it never prompts: it returns + * the hits, and the caller follows up with `skillx use `. + */ export const findCommand = new Command('find') .description('Search and use a skill in one command') .argument('', 'Search query') - .action(async (query: string) => { - const spinner = ora('Searching for skills...').start(); - - try { - const response = await apiRequest('/api/search', { - method: 'POST', - body: JSON.stringify({ query }), - }); - - spinner.stop(); - - if (!response.results || response.results.length === 0) { - console.log(chalk.yellow('\nNo skills found matching your query.')); - console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); - return; - } - - // Display numbered results - console.log(chalk.bold.green(`\n✓ Found ${response.total} skill(s)\n`)); - - response.results.forEach((skill, i) => { - const num = chalk.bold.white(`[${i + 1}]`); - const name = chalk.cyan(skill.name); - const category = chalk.magenta(skill.category); - const rating = chalk.yellow(`⭐ ${skill.rating.toFixed(1)}`); - console.log(`${num} ${name} ${category} ${rating}`); - console.log(` ${chalk.dim(truncate(skill.description, 80))}`); - }); - - console.log(); - - // Prompt user to select - const rl = readline.createInterface({ input, output }); - try { - const answer = await rl.question( - chalk.cyan(`Select a skill [1-${response.results.length}] or press Enter to cancel: `) - ); - - const choice = parseInt(answer.trim(), 10); - if (isNaN(choice) || choice < 1 || choice > response.results.length) { - console.log(chalk.dim('\nCancelled.')); - return; - } - - const selected = response.results[choice - 1]; - - // Fetch and display full skill details - const detailSpinner = ora(`Fetching ${selected.name}...`).start(); - const skill = await apiRequest(`/api/skills/${selected.slug}`); - detailSpinner.stop(); - - console.log(chalk.bold.green(`\n✓ Skill: ${skill.name}\n`)); - console.log(chalk.dim('─'.repeat(80))); - console.log(chalk.bold('Description:')); - console.log(skill.description); - console.log(); - console.log(chalk.bold('Category:'), chalk.magenta(skill.category)); - console.log(chalk.bold('Rating:'), chalk.yellow(`⭐ ${skill.rating.toFixed(1)}`)); - console.log(); - - if (skill.install_command) { - console.log(chalk.bold.cyan('Install Command:')); - console.log(chalk.bgBlack.white(` ${skill.install_command} `)); - console.log(); - } - - console.log(chalk.bold('Content Preview:')); - console.log(chalk.dim('─'.repeat(80))); - const preview = skill.content.split('\n').slice(0, 30).join('\n'); - console.log(preview); - - if (skill.content.split('\n').length > 30) { - console.log(chalk.dim('\n... (content truncated)')); - } - - console.log(chalk.dim('\n─'.repeat(80))); - console.log(chalk.dim(`\nUse ${chalk.cyan(`skillx use ${skill.slug} --raw`)} to output full content`)); - console.log(chalk.dim(`View online at: ${chalk.underline(`https://skillx.sh/skills/${skill.slug}`)}`)); - } finally { - rl.close(); - } - } catch (error) { - spinner.stop(); - - if (error instanceof ApiError) { - console.error(chalk.red(`\n✗ API Error: ${error.message}`)); - } else if (error instanceof Error) { - console.error(chalk.red(`\n✗ Network Error: ${error.message}`)); - console.error(chalk.dim('Check your internet connection or try again later.')); - } else { - console.error(chalk.red('\n✗ An unexpected error occurred')); - } - process.exit(1); - } - }); + .option('--json', 'emit the hits as a machine-readable envelope, without prompting') + .action((query: string, options: { json?: boolean }) => + runCommand( + { json: options.json === true, spinner: 'Searching for skills...' }, + async () => { + const { results } = await searchSkills(query); + return { query, results, count: results.length }; + }, + (page) => pickAndUse(page.results), + ), + ); function truncate(str: string, maxLen: number): string { return str.length > maxLen ? str.substring(0, maxLen - 3) + '...' : str; diff --git a/packages/cli/src/commands/inspect.ts b/packages/cli/src/commands/inspect.ts index 4eb6b2f..ad0959c 100644 --- a/packages/cli/src/commands/inspect.ts +++ b/packages/cli/src/commands/inspect.ts @@ -1,9 +1,8 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import ora from 'ora'; import { fetchSkillInspection } from '../lib/skill-lookup.js'; import type { SkillInspection } from '../lib/skill-lookup.js'; -import { exitCodeFor, fromError, serialize, success } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; interface InspectOptions { json?: boolean; @@ -89,26 +88,20 @@ export const inspectCommand = new Command('inspect') .argument('', 'slug, author/skill, or org/repo/skill') .option('--target ', 'also answer for one specific runtime') .option('--json', 'emit a machine-readable envelope instead of text') - .action(async (identifier: string, options: InspectOptions) => { - const useJson = options.json === true; - const spinner = useJson ? null : ora(`Inspecting ${identifier}...`).start(); - - try { - const { slug, inspection } = await fetchSkillInspection(identifier, options.target); - spinner?.stop(); - - const payload = { slug, skill: inspection.skill, compatibility: inspection.compatibility ?? { declared: [] } }; - if (useJson) { - const envelope = success(payload); - console.log(serialize(envelope)); - process.exit(exitCodeFor(envelope)); - } - render(inspection, slug); - } catch (error) { - spinner?.stop(); - const envelope = fromError(error); - if (useJson) console.log(serialize(envelope)); - else console.error(chalk.red(`\n✗ ${envelope.error.message}`)); - process.exit(exitCodeFor(envelope)); - } + .action((identifier: string, options: InspectOptions) => { + // The envelope carries the summary payload; human text also lists references and scripts. + let fetched: SkillInspection | undefined; + return runCommand( + { + json: options.json === true, + spinner: `Inspecting ${identifier}...`, + errors: { notFoundMessage: `Skill not found: ${identifier}` }, + }, + async () => { + const { slug, inspection } = await fetchSkillInspection(identifier, options.target); + fetched = inspection; + return { slug, skill: inspection.skill, compatibility: inspection.compatibility ?? { declared: [] } }; + }, + (data) => render(fetched as SkillInspection, data.slug), + ); }); diff --git a/packages/cli/src/commands/report.ts b/packages/cli/src/commands/report.ts index 5d52e4a..428b5c1 100644 --- a/packages/cli/src/commands/report.ts +++ b/packages/cli/src/commands/report.ts @@ -1,12 +1,16 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import ora from 'ora'; -import { apiRequest, ApiError } from '../lib/api-client.js'; +import { apiRequest } from '../lib/api-client.js'; +import { CliError } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; import { getApiKey } from '../utils/config-store.js'; +const OUTCOMES = ['success', 'failure', 'partial'] as const; +type Outcome = (typeof OUTCOMES)[number]; + interface ReportPayload { skill_slug: string; - outcome: 'success' | 'failure' | 'partial'; + outcome: Outcome; model?: string; duration_ms?: number; } @@ -16,75 +20,61 @@ interface ReportResponse { message: string; } +interface ReportOptions { + model?: string; + duration?: string; + json?: boolean; +} + +/** Validates the arguments before any request, so a bad call is a usage error (exit 2). */ +export function buildReportPayload(slug: string, outcome: string, options: ReportOptions): ReportPayload { + if (!(OUTCOMES as readonly string[]).includes(outcome)) { + throw new CliError('usage_error', `Invalid outcome "${outcome}". Must be: ${OUTCOMES.join(', ')}.`, { + details: { outcome, allowed: OUTCOMES }, + }); + } + const payload: ReportPayload = { skill_slug: slug, outcome: outcome as Outcome }; + if (options.model) payload.model = options.model; + if (options.duration !== undefined) { + const duration = Number(options.duration); + if (!Number.isFinite(duration) || duration < 0) { + throw new CliError('usage_error', `Invalid duration "${options.duration}". Pass milliseconds as a number.`); + } + payload.duration_ms = duration; + } + return payload; +} + export const reportCommand = new Command('report') .description('Report skill usage to SkillX (requires API key)') .argument('', 'Skill slug identifier') .argument('', 'Outcome: success, failure, or partial') .option('-m, --model ', 'AI model used (e.g., claude-sonnet-4)') - .option('-d, --duration ', 'Execution duration in milliseconds', parseFloat) - .action( - async (slug: string, outcome: string, options: { model?: string; duration?: number }) => { - const apiKey = getApiKey(); - if (!apiKey) { - console.error(chalk.red('\n✗ API key required for reporting')); - console.error( - chalk.dim(`Configure your API key with: ${chalk.cyan('skillx config set-key')}`) - ); - process.exit(1); - } - - if (!['success', 'failure', 'partial'].includes(outcome)) { - console.error(chalk.red('\n✗ Invalid outcome. Must be: success, failure, or partial')); - process.exit(1); - } - - const spinner = ora(`Reporting usage for ${slug}...`).start(); - - try { - const payload: ReportPayload = { - skill_slug: slug, - outcome: outcome as 'success' | 'failure' | 'partial', - }; - - if (options.model) { - payload.model = options.model; + .option('-d, --duration ', 'Execution duration in milliseconds') + .option('--json', 'emit a machine-readable envelope instead of text') + .action((slug: string, outcome: string, options: ReportOptions) => + runCommand( + { + json: options.json === true, + spinner: `Reporting usage for ${slug}...`, + errors: { notFoundMessage: `Skill not found: ${slug}` }, + }, + async () => { + const payload = buildReportPayload(slug, outcome, options); + if (!getApiKey()) { + throw new CliError('auth_error', 'API key required for reporting.', { + hint: 'Run `skillx config set-key` or set SKILLX_API_KEY.', + }); } - - if (options.duration !== undefined) { - payload.duration_ms = options.duration; - } - const response = await apiRequest('/api/report', { method: 'POST', body: JSON.stringify(payload), }); - - spinner.stop(); - console.log(chalk.green(`\n✓ ${response.message}`)); - console.log( - chalk.dim(`Reported ${chalk.bold(outcome)} for skill: ${chalk.cyan(slug)}`) - ); - } catch (error) { - spinner.stop(); - - if (error instanceof ApiError) { - if (error.status === 401) { - console.error(chalk.red('\n✗ Invalid API key')); - console.error( - chalk.dim(`Update your API key with: ${chalk.cyan('skillx config set-key')}`) - ); - } else if (error.status === 404) { - console.error(chalk.red(`\n✗ Skill not found: ${slug}`)); - } else { - console.error(chalk.red(`\n✗ API Error: ${error.message}`)); - } - } else if (error instanceof Error) { - console.error(chalk.red(`\n✗ Network Error: ${error.message}`)); - console.error(chalk.dim('Check your internet connection or try again later.')); - } else { - console.error(chalk.red('\n✗ An unexpected error occurred')); - } - process.exit(1); - } - } + return { ...payload, message: response.message }; + }, + (result) => { + console.log(chalk.green(`\n✓ ${result.message}`)); + console.log(chalk.dim(`Reported ${chalk.bold(result.outcome)} for skill: ${chalk.cyan(result.skill_slug)}`)); + }, + ), ); diff --git a/packages/cli/src/commands/search.ts b/packages/cli/src/commands/search.ts index d8e5c90..c53cdf7 100644 --- a/packages/cli/src/commands/search.ts +++ b/packages/cli/src/commands/search.ts @@ -1,9 +1,8 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import ora from 'ora'; import { searchSkills } from '../lib/search-api.js'; import type { CompatibilityFilterReport, SearchResult } from '../lib/search-api.js'; -import { exitCodeFor, fromError, serialize, success } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; import { resolveAndUseSkill } from './use.js'; interface SearchOptions { @@ -47,73 +46,68 @@ export const searchCommand = new Command('search') .option('-u, --use', 'Auto-pick the top result and show its details') .option('-c, --compatible ', 'only return skills that declare support for this runtime') .option('--json', 'emit a machine-readable envelope instead of text') - .action(async (query: string, options: SearchOptions) => { - const useJson = options.json === true; - const spinner = useJson ? null : ora('Searching for skills...').start(); - - try { - const outcome = await searchSkills(query, { - ...(options.compatible ? { compatible: options.compatible } : {}), - }); - spinner?.stop(); - - if (useJson) { - const envelope = success({ + .action((query: string, options: SearchOptions) => + runCommand( + { json: options.json === true, spinner: 'Searching for skills...' }, + async () => { + const outcome = await searchSkills(query, { + ...(options.compatible ? { compatible: options.compatible } : {}), + }); + return { query, results: outcome.results, count: outcome.results.length, ...(outcome.compatibilityFilter ? { compatibilityFilter: outcome.compatibilityFilter } : {}), ...(outcome.note ? { note: outcome.note } : {}), - }); - console.log(serialize(envelope)); - process.exit(exitCodeFor(envelope)); - } - - if (outcome.results.length === 0) { - console.log(chalk.yellow('\nNo skills found matching your query.')); - if (outcome.note) console.log(chalk.dim(outcome.note)); - else console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); - if (outcome.compatibilityFilter) printFilterReport(outcome.compatibilityFilter); - return; - } - - if (options.use) { - const top = outcome.results[0]; + }; + }, + async (page) => { + const top = page.results[0]; + if (!options.use || !top) return renderResults(page, options.compatible); + // `--use` continues into `use` with the top hit; it is a human convenience. console.log(chalk.dim(`Top result for "${query}": ${chalk.cyan(`${top.author}/${top.name}`)}\n`)); await resolveAndUseSkill(`${top.author}/${top.name}`, { raw: false }); - return; - } + }, + ), + ); + +interface SearchPage { + results: SearchResult[]; + compatibilityFilter?: CompatibilityFilterReport; + note?: string; +} - console.log(chalk.bold.green(`\n✓ Found ${outcome.results.length} skill(s)\n`)); +function renderResults(page: SearchPage, runtime: string | undefined): void { + if (page.results.length === 0) { + console.log(chalk.yellow('\nNo skills found matching your query.')); + if (page.note) console.log(chalk.dim(page.note)); + else console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); + if (page.compatibilityFilter) printFilterReport(page.compatibilityFilter); + return; + } - const runtime = options.compatible; - const header = `${padRight('SKILL', 40)} ${padRight('CATEGORY', 15)} ${padRight('RATING', 8)}${runtime ? ` ${padRight(runtime.toUpperCase(), 12)}` : ''} DESCRIPTION`; - console.log(chalk.bold(header)); - console.log(chalk.dim('─'.repeat(header.length))); + console.log(chalk.bold.green(`\n✓ Found ${page.results.length} skill(s)\n`)); - outcome.results.forEach((skill) => { - const displayId = `${skill.author}/${skill.name}`; - const rating = skill.avg_rating ?? 0; - const cols = [ - chalk.cyan(padRight(displayId, 40)), - chalk.magenta(padRight(skill.category, 15)), - chalk.yellow(padRight(`⭐ ${rating.toFixed(1)}`, 8)), - ]; - if (runtime) cols.push(padRight(compatibilityCell(skill, runtime), 12)); - console.log(`${cols.join(' ')} ${truncate(skill.description, 40)}`); - }); + const header = `${padRight('SKILL', 40)} ${padRight('CATEGORY', 15)} ${padRight('RATING', 8)}${runtime ? ` ${padRight(runtime.toUpperCase(), 12)}` : ''} DESCRIPTION`; + console.log(chalk.bold(header)); + console.log(chalk.dim('─'.repeat(header.length))); - if (outcome.compatibilityFilter) printFilterReport(outcome.compatibilityFilter); - console.log(chalk.dim(`\nUse ${chalk.cyan('skillx use ')} to view and install a skill`)); - } catch (error) { - spinner?.stop(); - const envelope = fromError(error); - if (useJson) console.log(serialize(envelope)); - else console.error(chalk.red(`\n✗ ${envelope.error.message}`)); - process.exit(exitCodeFor(envelope)); - } + page.results.forEach((skill) => { + const displayId = `${skill.author}/${skill.name}`; + const rating = skill.avg_rating ?? 0; + const cols = [ + chalk.cyan(padRight(displayId, 40)), + chalk.magenta(padRight(skill.category, 15)), + chalk.yellow(padRight(`⭐ ${rating.toFixed(1)}`, 8)), + ]; + if (runtime) cols.push(padRight(compatibilityCell(skill, runtime), 12)); + console.log(`${cols.join(' ')} ${truncate(skill.description, 40)}`); }); + if (page.compatibilityFilter) printFilterReport(page.compatibilityFilter); + console.log(chalk.dim(`\nUse ${chalk.cyan('skillx use ')} to view and install a skill`)); +} + function padRight(str: string, width: number): string { return str.length >= width ? str.substring(0, width - 3) + '...' : str.padEnd(width); } diff --git a/packages/cli/src/commands/use-display.ts b/packages/cli/src/commands/use-display.ts index d0c9c25..67dd6aa 100644 --- a/packages/cli/src/commands/use-display.ts +++ b/packages/cli/src/commands/use-display.ts @@ -156,7 +156,7 @@ export function displaySkill( } /** Fire-and-forget install tracking */ -function trackInstall(slug: string): void { +export function trackInstall(slug: string): void { const installHeaders: Record = { 'X-Device-Id': getDeviceId(), }; diff --git a/packages/cli/src/commands/use.ts b/packages/cli/src/commands/use.ts index a6fd6e6..e9c451d 100644 --- a/packages/cli/src/commands/use.ts +++ b/packages/cli/src/commands/use.ts @@ -1,230 +1,67 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import ora from 'ora'; -import { apiRequest, ApiError } from '../lib/api-client.js'; -import { searchSkills } from '../lib/search-api.js'; -import { hasPublishedRelease } from '../lib/release-lookup.js'; -import { - displaySkill, - type SkillDetails, - type SkillDetailResponse, - type DisplayOptions, -} from './use-display.js'; - -interface RegisterResponse { - skill?: SkillDetails; - created?: boolean; - skills?: Array<{ slug: string; name: string; author: string }>; - registered?: number; - skipped?: number; -} - -type IdentifierType = 'search' | 'repo-path' | 'two-part' | 'slug'; - -interface ParsedIdentifier { - type: IdentifierType; - parts: string[]; -} - -/** Source identity of an `org/repo/` identifier. */ -export interface SourceTarget { - /** Slug guess, used only when the server has no skill for the identity yet. */ - slug: string; - identity: { repo: string; path: string }; -} - -/** Parse identifier into type + parts */ -export function parseIdentifier(input: string): ParsedIdentifier { - if (input.includes(' ')) return { type: 'search', parts: [input] }; - - const slashParts = input.split('/'); - // org/repo/: the skill path can be several segments deep, so none of it is dropped - if (slashParts.length >= 3) return { type: 'repo-path', parts: slashParts }; - if (slashParts.length === 2) return { type: 'two-part', parts: slashParts }; - return { type: 'slug', parts: [input] }; -} - -/** - * Map `org/repo/` onto a full source identity. The last path segment is the - * readable name; the whole path is kept so deep paths round-trip to the server. - */ -export function parseRepoPath(parts: string[]): SourceTarget { - const [owner, repo, ...rest] = parts; - const path = rest.join('/'); - const leaf = rest.length > 0 ? rest[rest.length - 1] : repo; - - return { - slug: `${owner}-${leaf}`.toLowerCase(), - identity: { repo: `${owner}/${repo}`, path }, - }; -} - -/** Skill detail endpoint, optionally pinned to a source identity. */ -export function buildSkillEndpoint(slug: string, identity?: { repo: string; path: string }): string { - const endpoint = `/api/skills/${encodeURIComponent(slug)}`; - if (!identity) return endpoint; - - const query = new URLSearchParams({ repo: identity.repo, path: identity.path }); - return `${endpoint}?${query.toString()}`; -} - -/** - * Resolve identifier and use the skill. Exported for use by search --use. - * - * Resolution chain: - * - spaces → search mode - * - x/y/z... (repo path) → identity lookup on repo + full path, fallback register from that path - * - x/y (two-part) → DB lookup slug "x-y", fallback scan repo y for skills - * - single word → DB lookup, fallback search - */ -export async function resolveAndUseSkill( - identifier: string, - options: DisplayOptions, -): Promise { - const parsed = parseIdentifier(identifier); - - switch (parsed.type) { - case 'search': - return searchAndUse(parsed.parts[0], options); - - case 'repo-path': { - const [org, repo] = parsed.parts; - const { slug, identity } = parseRepoPath(parsed.parts); - return resolveBySlug(slug, identifier, options, { - identity, - registerFallback: { owner: org, repo, skill_path: identity.path }, - }); - } - - case 'two-part': { - const [author, skillName] = parsed.parts; - const slug = `${author}-${skillName}`.toLowerCase(); - return resolveBySlug(slug, identifier, options, { - registerFallback: { owner: author, repo: skillName, scan: true }, - }); - } - - case 'slug': - return resolveBySlug(parsed.parts[0], identifier, options, { - searchFallback: true, - }); - } -} - -/** Core resolution: DB lookup with fallback to register or search */ -async function resolveBySlug( - slug: string, - displayId: string, - options: DisplayOptions, - fallback: { - registerFallback?: { owner: string; repo: string; skill_path?: string; scan?: boolean }; - searchFallback?: boolean; - identity?: { repo: string; path: string }; - }, -): Promise { - const spinner = ora(`Fetching skill: ${displayId}...`).start(); - - try { - const res = await apiRequest(buildSkillEndpoint(slug, fallback.identity)); - spinner.stop(); - await showSkill(res, displayId, options); - } catch (err) { - if (!(err instanceof ApiError && err.status === 404)) { - spinner.stop(); - throw err; - } - - if (fallback.registerFallback) { - spinner.text = 'Skill not found. Scanning GitHub...'; - try { - const registerRes = await apiRequest('/api/skills/register', { - method: 'POST', - body: JSON.stringify(fallback.registerFallback), - }); - spinner.stop(); - await handleRegisterResult(registerRes, displayId, options); - } catch (regErr) { - spinner.stop(); - throw regErr; - } - } else if (fallback.searchFallback) { - spinner.stop(); - console.log(chalk.dim(`Skill "${displayId}" not found, searching...`)); - await searchAndUse(displayId, options); - } else { - spinner.stop(); - throw err; - } - } +import { runCommand } from '../lib/run-command.js'; +import { resolveSkill, type ResolvedSkill } from '../lib/skill-resolver.js'; +import { discoveryOnlyNotice, displaySkill, trackInstall, type DisplayOptions } from './use-display.js'; + +export { buildSkillEndpoint, parseIdentifier, parseRepoPath, type SourceTarget } from '../lib/skill-resolver.js'; + +interface UseOptions { + raw?: boolean; + search?: boolean; + includeRefs?: boolean; + includeScripts?: boolean; + json?: boolean; } -/** - * Display a skill detail. Only a multi-file listing can be discovery-only, so - * only then is the release listing asked whether a published release exists. - */ -async function showSkill(detail: SkillDetailResponse, displayId: string, options: DisplayOptions): Promise { - const multiFile = (detail.references?.length ?? 0) + (detail.scripts?.length ?? 0) > 0; - const releaseExists = multiFile ? await hasPublishedRelease(displayId) : false; - displaySkill(detail.skill, displayId, options, detail.references, detail.scripts, releaseExists); -} +/** Human rendering of a resolution. */ +function renderResolved(resolved: ResolvedSkill, options: DisplayOptions): void { + // In raw mode stdout is the skill content only; the detours go to stderr. + const note = options.raw ? console.error : (text: string) => console.log(chalk.dim(text)); + for (const text of resolved.notes) note(text); -/** - * The register response carries the skill row only, so its references and - * scripts come from the detail endpoint. If that read fails, the row is - * shown without them (and so without the discovery-only label). - */ -async function registeredDetail(skill: SkillDetails): Promise { - try { - return await apiRequest(buildSkillEndpoint(skill.slug)); - } catch { - return { skill }; - } -} - -/** Handle register API response (single skill or multi-skill scan) */ -async function handleRegisterResult( - res: RegisterResponse, - displayId: string, - options: DisplayOptions, -): Promise { - if (res.skill) { - if (res.created) { - console.log(chalk.green(`\nRegistered new skill from GitHub: ${displayId}`)); - } - await showSkill(await registeredDetail(res.skill), displayId, options); + if (resolved.kind === 'skill') { + if (resolved.registered) console.log(chalk.green(`\nRegistered new skill from GitHub: ${resolved.displayId}`)); + const { skill, references, scripts } = resolved.detail; + displaySkill(skill, resolved.displayId, options, references, scripts, resolved.releaseExists); return; } - if (res.skills && res.skills.length > 0) { - console.log(chalk.bold.green(`\nFound ${res.skills.length} skill(s) in repo:\n`)); - res.skills.forEach((s) => { - console.log(` ${chalk.cyan(`${s.author}/${s.name}`)}`); - }); - console.log(chalk.dim(`\nUse ${chalk.cyan(`skillx use ${displayId}/`)} to use a specific skill`)); - if (res.registered) { - console.log(chalk.dim(`(${res.registered} newly registered, ${res.skipped || 0} already existed)`)); + if (resolved.kind === 'repo') { + console.log(chalk.bold.green(`\nFound ${resolved.skills.length} skill(s) in repo:\n`)); + resolved.skills.forEach((s) => console.log(` ${chalk.cyan(`${s.author}/${s.name}`)}`)); + console.log(chalk.dim(`\nUse ${chalk.cyan(`skillx use ${resolved.displayId}/`)} to use a specific skill`)); + if (resolved.registered) { + console.log(chalk.dim(`(${resolved.registered} newly registered, ${resolved.skipped} already existed)`)); } return; } - console.log(chalk.yellow(`\nNo skills found in ${displayId}`)); + console.log(chalk.yellow(`\nNo skills found for ${resolved.displayId}`)); + console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); } -/** Search for a keyword and use the top result */ -async function searchAndUse(query: string, options: DisplayOptions): Promise { - const spinner = ora(`Searching for "${query}"...`).start(); - const { results } = await searchSkills(query); - spinner.stop(); - - if (results.length === 0) { - console.log(chalk.yellow('\nNo skills found matching your query.')); - console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); - return; - } +/** The `use --json` payload: the resolution, with the discovery-only decision made explicit. */ +export function toUsePayload(resolved: ResolvedSkill) { + if (resolved.kind !== 'skill') return resolved; + const { skill, references = [], scripts = [] } = resolved.detail; + const notice = discoveryOnlyNotice(references, scripts, resolved.releaseExists); + return { + kind: resolved.kind, + displayId: resolved.displayId, + registered: resolved.registered, + skill, + references, + scripts, + discoveryOnly: notice !== null, + ...(notice ? { discoveryOnlyNotice: notice } : {}), + notes: resolved.notes, + }; +} - const top = results[0]; - console.log(chalk.dim(`Top result for "${query}": ${chalk.cyan(`${top.author}/${top.name}`)}\n`)); - await resolveAndUseSkill(`${top.author}/${top.name}`, options); +/** Resolve identifier and display the skill. Exported for use by search --use. */ +export async function resolveAndUseSkill(identifier: string, options: DisplayOptions): Promise { + renderResolved(await resolveSkill(identifier), options); } export const useCommand = new Command('use') @@ -234,32 +71,29 @@ export const useCommand = new Command('use') .option('-s, --search', 'Force search mode') .option('--include-refs', 'Include references in output') .option('--include-scripts', 'Include scripts in output') - .action(async (identifier: string, options: { raw?: boolean; search?: boolean; includeRefs?: boolean; includeScripts?: boolean }) => { - const raw = options.raw ?? false; - const includeRefs = options.includeRefs ?? false; - const includeScripts = options.includeScripts ?? false; - - try { - if (options.search) { - await searchAndUse(identifier, { raw, includeRefs, includeScripts }); - return; - } - await resolveAndUseSkill(identifier, { raw, includeRefs, includeScripts }); - } catch (error) { - if (error instanceof ApiError) { - if (error.status === 404) { - console.error(chalk.red(`\n✗ Skill not found: ${identifier}`)); - } else if (error.status === 429) { - console.error(chalk.red('\n✗ Rate limited. Try again later.')); - } else { - console.error(chalk.red(`\n✗ API Error: ${error.message}`)); - } - } else if (error instanceof Error) { - console.error(chalk.red(`\n✗ Network Error: ${error.message}`)); - console.error(chalk.dim('Check your internet connection or try again later.')); - } else { - console.error(chalk.red('\n✗ An unexpected error occurred')); - } - process.exit(1); - } + .option('--json', 'emit a machine-readable envelope instead of text') + .action((identifier: string, options: UseOptions) => { + const json = options.json === true; + const display: DisplayOptions = { + raw: options.raw ?? false, + includeRefs: options.includeRefs ?? false, + includeScripts: options.includeScripts ?? false, + }; + // The envelope flattens the skill; the human renderer needs the resolution itself. + let resolved: ResolvedSkill | undefined; + return runCommand( + { + json, + // A spinner would interleave with piped raw content. + ...(display.raw ? {} : { spinner: `Fetching skill: ${identifier}...` }), + errors: { notFoundMessage: `Skill not found: ${identifier}` }, + }, + async (progress) => { + resolved = await resolveSkill(identifier, { forceSearch: options.search, progress }); + // Human display tracks the install itself; the envelope path counts the same use. + if (json && resolved.kind === 'skill') trackInstall(resolved.detail.skill.slug); + return toUsePayload(resolved); + }, + () => renderResolved(resolved as ResolvedSkill, display), + ); }); diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 5681c2e..06e5955 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 { routeUsageErrors } from './lib/run-command.js'; /** Injected at build time from packages/cli/package.json by tsup (see tsup.config.ts). */ declare const __SKILLX_VERSION__: string; @@ -41,4 +42,6 @@ program.addCommand(promoteCommand); program.addCommand(releaseCommand); for (const command of [installCommand, updateCommand, uninstallCommand, listCommand, syncCommand, mcpCommand]) program.addCommand(command); -program.parse(process.argv); +// Parse errors exit 2 (and print a usage_error envelope under --json); see docs/cli-output.md. +routeUsageErrors(program); +await program.parseAsync(process.argv); diff --git a/packages/cli/src/lib/output.test.ts b/packages/cli/src/lib/output.test.ts index b04ff8c..a8b6475 100644 --- a/packages/cli/src/lib/output.test.ts +++ b/packages/cli/src/lib/output.test.ts @@ -10,6 +10,7 @@ import { describe, expect, it } from "vitest"; import { ApiError } from "./api-client.js"; import { + CliError, EXIT_CODES, exitCodeFor, failure, @@ -48,7 +49,7 @@ describe("fromError", () => { it.each([401, 403])("maps %s to auth_error with a hint", (status) => { const envelope = fromError(new ApiError(status, "denied")); expect(envelope.error.code).toBe("auth_error"); - expect(envelope.error.hint).toContain("apiKey"); + expect(envelope.error.hint).toContain("set-key"); expect(exitCodeFor(envelope)).toBe(EXIT_CODES.auth); }); @@ -58,6 +59,28 @@ describe("fromError", () => { expect(exitCodeFor(envelope)).toBe(EXIT_CODES.network); }); + it("does not mistake a CLI bug for a network outage", () => { + expect(fromError(new TypeError("x.toFixed is not a function")).error.code).toBe("unexpected_error"); + }); + + it("maps a 429 to rate_limited, which a caller retries", () => { + const envelope = fromError(new ApiError(429, "slow down")); + expect(envelope.error.code).toBe("rate_limited"); + expect(exitCodeFor(envelope)).toBe(EXIT_CODES.rateLimited); + }); + + it("uses the command's wording for a 404", () => { + expect(fromError(new ApiError(404, "x"), { notFoundMessage: "Skill not found: foo" }).error.message).toBe( + "Skill not found: foo", + ); + }); + + it("keeps a CliError's own classification", () => { + const envelope = fromError(new CliError("usage_error", "bad outcome", { hint: "use success" })); + expect(envelope.error).toEqual({ code: "usage_error", message: "bad outcome", hint: "use success" }); + expect(exitCodeFor(envelope)).toBe(EXIT_CODES.usage); + }); + it("maps any other API status to api_error", () => { expect(fromError(new ApiError(500, "boom")).error.code).toBe("api_error"); }); diff --git a/packages/cli/src/lib/output.ts b/packages/cli/src/lib/output.ts index f5181ff..c6416ae 100644 --- a/packages/cli/src/lib/output.ts +++ b/packages/cli/src/lib/output.ts @@ -25,6 +25,8 @@ export const EXIT_CODES = { auth: 5, /** The API could not be reached. */ network: 6, + /** The API asked the caller to slow down; retry later. */ + rateLimited: 7, /** Local files block an install change (modified, unowned or shared). */ conflict: 8, } as const; @@ -37,6 +39,7 @@ export type CliErrorCode = | 'incompatible' | 'auth_error' | 'network_error' + | 'rate_limited' | 'api_error' | 'integrity_error' | 'conflict' @@ -48,6 +51,7 @@ const EXIT_BY_CODE: Record = { incompatible: EXIT_CODES.incompatible, auth_error: EXIT_CODES.auth, network_error: EXIT_CODES.network, + rate_limited: EXIT_CODES.rateLimited, api_error: EXIT_CODES.error, integrity_error: EXIT_CODES.error, conflict: EXIT_CODES.conflict, @@ -73,6 +77,21 @@ export interface CliFailure { export type CliEnvelope = CliSuccess | CliFailure; +/** + * A failure a command raises on purpose, already classified. Throwing one from + * inside `runCommand` produces exactly this code, message and hint. + */ +export class CliError extends Error { + constructor( + readonly code: CliErrorCode, + message: string, + readonly extra: { details?: unknown; hint?: string } = {}, + ) { + super(message); + this.name = 'CliError'; + } +} + export function success(data: T): CliSuccess { return { ok: true, data }; } @@ -97,28 +116,44 @@ export function exitCodeFor(envelope: CliEnvelope): ExitCode { return envelope.ok ? EXIT_CODES.ok : EXIT_BY_CODE[envelope.error.code]; } +/** Per-command wording for errors whose class is fixed but whose subject varies. */ +export interface ErrorContext { + /** Replaces the generic 404 message, e.g. "Skill not found: foo". */ + notFoundMessage?: string; +} + /** * Maps a thrown error onto the envelope vocabulary. * - * A 404 is "the skill does not exist" and a 401/403 is "your credentials are + * A 404 is "the thing does not exist" and a 401/403 is "your credentials are * wrong"; both are decisions a caller makes differently, so they do not share a * code with a generic API failure. */ -export function fromError(error: unknown): CliFailure { +export function fromError(error: unknown, context: ErrorContext = {}): CliFailure { + if (error instanceof CliError) return failure(error.code, error.message, error.extra); + if (error instanceof ApiError) { if (error.status === 404) { - return failure('not_found', 'The requested skill was not found.', { details: { status: 404 } }); + return failure('not_found', context.notFoundMessage ?? 'The requested resource was not found.', { + details: { status: 404 }, + }); } if (error.status === 401 || error.status === 403) { return failure('auth_error', 'The API rejected this request as unauthenticated.', { details: { status: error.status }, - hint: 'Run `skillx config set apiKey ` or check the key has not been revoked.', + hint: 'Run `skillx config set-key` or set SKILLX_API_KEY, and check the key has not been revoked.', + }); + } + if (error.status === 429) { + return failure('rate_limited', 'Rate limited by the API.', { + details: { status: 429 }, + hint: 'Wait and retry.', }); } return failure('api_error', error.message, { details: { status: error.status } }); } - if (error instanceof TypeError) { + if (isFetchFailure(error)) { // `fetch` rejects with TypeError when the host cannot be reached. return failure('network_error', error.message, { hint: 'Check the network connection and the configured base URL.', @@ -129,6 +164,14 @@ export function fromError(error: unknown): CliFailure { return failure('unexpected_error', 'An unknown error occurred.'); } +/** + * Node's fetch rejects with `TypeError('fetch failed', { cause })`. Any other + * TypeError is a bug in the CLI and must not be reported as a network outage. + */ +function isFetchFailure(error: unknown): error is TypeError { + return error instanceof TypeError && (error.message === 'fetch failed' || error.cause !== undefined); +} + /** Serializes an envelope for stdout. */ export function serialize(envelope: CliEnvelope): string { return JSON.stringify(envelope, null, 2); diff --git a/packages/cli/src/lib/run-command.ts b/packages/cli/src/lib/run-command.ts new file mode 100644 index 0000000..60d49d6 --- /dev/null +++ b/packages/cli/src/lib/run-command.ts @@ -0,0 +1,121 @@ +/** + * One way for a command to finish. + * + * Every command reports through `runCommand`, so `--json` always yields one + * envelope on stdout and the exit code always follows `output.ts`. Human text + * stays the default. The exit code is set on `process.exitCode` instead of + * calling `process.exit`, so piped stdout drains and fire-and-forget requests + * (install tracking) are not cut off. + */ + +import chalk from 'chalk'; +import ora from 'ora'; +import { CommanderError, type Command } from 'commander'; +import { + EXIT_CODES, + exitCodeFor, + failure, + fromError, + serialize, + success, + type CliEnvelope, + type CliFailure, + type ErrorContext, + type ExitCode, +} from './output.js'; + +export interface RunOptions { + /** `--json` was passed: print the envelope instead of human text. */ + json: boolean; + /** Spinner text shown in human mode while the task runs. */ + spinner?: string; + /** Exit code for a successful result, when success is not always 0 (e.g. `check`). */ + exitCode?: (data: T) => ExitCode; + /** Wording for errors whose subject varies by command. */ + errors?: ErrorContext; +} + +/** Writes an envelope (or human text) and sets the exit code. */ +export function emit( + envelope: CliEnvelope, + json: boolean, + human: (data: T) => void, + exitCode?: ExitCode, +): void { + if (json) console.log(serialize(envelope)); + else if (envelope.ok) human(envelope.data); + else printFailure(envelope); + process.exitCode = exitCode ?? exitCodeFor(envelope); +} + +/** Human rendering of a failure: the message, then the hint when there is one. */ +export function printFailure(envelope: CliFailure): void { + console.error(chalk.red(`\n✗ ${envelope.error.message}`)); + if (envelope.error.hint) console.error(chalk.dim(envelope.error.hint)); +} + +/** + * Runs a command body and reports its result. + * + * `task` returns the data that becomes `envelope.data`; throwing (a `CliError`, + * an `ApiError`, anything) becomes a failure envelope. `human` renders the same + * data as text, so both modes describe one result. A human renderer may itself + * continue into further requests (`search --use`); its failure is reported the + * same way. + */ +export async function runCommand( + options: RunOptions, + task: (progress: (text: string) => void) => Promise, + human: (data: T) => void | Promise, +): Promise { + const spinner = !options.json && options.spinner ? ora(options.spinner).start() : null; + const progress = (text: string) => { + if (spinner) spinner.text = text; + }; + + let envelope: CliEnvelope; + try { + envelope = success(await task(progress)); + } catch (error) { + envelope = fromError(error, options.errors); + } finally { + spinner?.stop(); + } + + const code = envelope.ok && options.exitCode ? options.exitCode(envelope.data) : undefined; + if (options.json || !envelope.ok) { + emit(envelope, options.json, () => {}, code); + return; + } + try { + await human(envelope.data); + process.exitCode = code ?? EXIT_CODES.ok; + } catch (error) { + emit(fromError(error, options.errors), false, () => {}); + } +} + +/** + * Commander exits 1 on every parse problem. A caller has to tell "you called me + * wrong" (2) from a failed run (1), and with `--json` it expects an envelope on + * stdout, so parse errors are routed through the same vocabulary. Help and + * `--version` still exit 0. + */ +export function routeUsageErrors(program: Command, argv: readonly string[] = process.argv): void { + const json = argv.includes('--json'); + const apply = (command: Command) => { + command.exitOverride((error: CommanderError) => { + if (error.code === 'commander.helpDisplayed' || error.code === 'commander.version') { + process.exit(EXIT_CODES.ok); + } + const envelope = failure('usage_error', error.message.replace(/^error:\s*/, ''), { + details: { reason: error.code }, + hint: 'Run `skillx --help` or `skillx --help`.', + }); + if (json) console.log(serialize(envelope)); + process.exit(EXIT_CODES.usage); + }); + command.commands.forEach(apply); + }; + apply(program); +} diff --git a/packages/cli/src/lib/skill-resolver.ts b/packages/cli/src/lib/skill-resolver.ts new file mode 100644 index 0000000..fdb63e5 --- /dev/null +++ b/packages/cli/src/lib/skill-resolver.ts @@ -0,0 +1,215 @@ +/** + * Resolves what `skillx use ` refers to, without printing anything. + * + * Resolution chain: + * - spaces → search mode, top hit + * - x/y/z... (repo path) → identity lookup on repo + full path, fallback register from that path + * - x/y (two-part) → DB lookup slug "x-y", fallback scan repo y for skills + * - single word → DB lookup, fallback search + * + * The result is plain data, so `use --json` and the human renderer report the + * same resolution. `notes` records the detours taken (a fallback search, the + * top hit picked), which the human renderer prints and the envelope carries. + */ + +import { apiRequest, ApiError } from './api-client.js'; +import { searchSkills } from './search-api.js'; +import { hasPublishedRelease } from './release-lookup.js'; +import type { SkillDetailResponse, SkillDetails } from '../commands/use-display.js'; + +type IdentifierType = 'search' | 'repo-path' | 'two-part' | 'slug'; + +interface ParsedIdentifier { + type: IdentifierType; + parts: string[]; +} + +/** Source identity of an `org/repo/` identifier. */ +export interface SourceTarget { + /** Slug guess, used only when the server has no skill for the identity yet. */ + slug: string; + identity: { repo: string; path: string }; +} + +/** Parse identifier into type + parts */ +export function parseIdentifier(input: string): ParsedIdentifier { + if (input.includes(' ')) return { type: 'search', parts: [input] }; + + const slashParts = input.split('/'); + // org/repo/: the skill path can be several segments deep, so none of it is dropped + if (slashParts.length >= 3) return { type: 'repo-path', parts: slashParts }; + if (slashParts.length === 2) return { type: 'two-part', parts: slashParts }; + return { type: 'slug', parts: [input] }; +} + +/** + * Map `org/repo/` onto a full source identity. The last path segment is the + * readable name; the whole path is kept so deep paths round-trip to the server. + */ +export function parseRepoPath(parts: string[]): SourceTarget { + const [owner, repo, ...rest] = parts; + const path = rest.join('/'); + const leaf = rest.length > 0 ? rest[rest.length - 1] : repo; + + return { + slug: `${owner}-${leaf}`.toLowerCase(), + identity: { repo: `${owner}/${repo}`, path }, + }; +} + +/** Skill detail endpoint, optionally pinned to a source identity. */ +export function buildSkillEndpoint(slug: string, identity?: { repo: string; path: string }): string { + const endpoint = `/api/skills/${encodeURIComponent(slug)}`; + if (!identity) return endpoint; + + const query = new URLSearchParams({ repo: identity.repo, path: identity.path }); + return `${endpoint}?${query.toString()}`; +} + +export type ResolvedSkill = + | { + kind: 'skill'; + displayId: string; + /** True when the register fallback created the listing just now. */ + registered: boolean; + detail: SkillDetailResponse; + /** A published release exists, so the listing is not discovery-only. */ + releaseExists: boolean; + notes: string[]; + } + | { + kind: 'repo'; + displayId: string; + skills: Array<{ slug: string; name: string; author: string }>; + registered: number; + skipped: number; + notes: string[]; + } + | { kind: 'none'; displayId: string; notes: string[] }; + +interface RegisterResponse { + skill?: SkillDetails; + created?: boolean; + skills?: Array<{ slug: string; name: string; author: string }>; + registered?: number; + skipped?: number; +} + +interface Fallback { + registerFallback?: { owner: string; repo: string; skill_path?: string; scan?: boolean }; + searchFallback?: boolean; + identity?: { repo: string; path: string }; +} + +type Progress = (text: string) => void; + +export async function resolveSkill( + identifier: string, + options: { forceSearch?: boolean; progress?: Progress } = {}, +): Promise { + const progress = options.progress ?? (() => {}); + const notes: string[] = []; + if (options.forceSearch) return searchTop(identifier, notes, progress); + + const parsed = parseIdentifier(identifier); + switch (parsed.type) { + case 'search': + return searchTop(parsed.parts[0], notes, progress); + case 'repo-path': { + const [org, repo] = parsed.parts; + const { slug, identity } = parseRepoPath(parsed.parts); + return bySlug(slug, identifier, notes, progress, { + identity, + registerFallback: { owner: org, repo, skill_path: identity.path }, + }); + } + case 'two-part': { + const [author, skillName] = parsed.parts; + return bySlug(`${author}-${skillName}`.toLowerCase(), identifier, notes, progress, { + registerFallback: { owner: author, repo: skillName, scan: true }, + }); + } + case 'slug': + return bySlug(parsed.parts[0], identifier, notes, progress, { searchFallback: true }); + } +} + +/** Core resolution: DB lookup with fallback to register or search */ +async function bySlug( + slug: string, + displayId: string, + notes: string[], + progress: Progress, + fallback: Fallback, +): Promise { + progress(`Fetching skill: ${displayId}...`); + try { + const detail = await apiRequest(buildSkillEndpoint(slug, fallback.identity)); + return skillResult(detail, displayId, false, notes); + } catch (err) { + if (!(err instanceof ApiError && err.status === 404)) throw err; + + if (fallback.registerFallback) { + progress('Skill not found. Scanning GitHub...'); + const res = await apiRequest('/api/skills/register', { + method: 'POST', + body: JSON.stringify(fallback.registerFallback), + }); + return registerResult(res, displayId, notes); + } + if (fallback.searchFallback) { + notes.push(`Skill "${displayId}" not found, searching...`); + return searchTop(displayId, notes, progress); + } + throw err; + } +} + +/** + * Only a multi-file listing can be discovery-only, so only then is the release + * listing asked whether a published release exists. + */ +async function skillResult( + detail: SkillDetailResponse, + displayId: string, + registered: boolean, + notes: string[], +): Promise { + const multiFile = (detail.references?.length ?? 0) + (detail.scripts?.length ?? 0) > 0; + const releaseExists = multiFile ? await hasPublishedRelease(displayId) : false; + return { kind: 'skill', displayId, registered, detail, releaseExists, notes }; +} + +/** + * The register response carries the skill row only, so its references and + * scripts come from the detail endpoint. If that read fails, the row is + * shown without them (and so without the discovery-only label). + */ +async function registerResult(res: RegisterResponse, displayId: string, notes: string[]): Promise { + if (res.skill) { + let detail: SkillDetailResponse; + try { + detail = await apiRequest(buildSkillEndpoint(res.skill.slug)); + } catch { + detail = { skill: res.skill }; + } + return skillResult(detail, displayId, res.created === true, notes); + } + if (res.skills && res.skills.length > 0) { + return { kind: 'repo', displayId, skills: res.skills, registered: res.registered ?? 0, skipped: res.skipped ?? 0, notes }; + } + return { kind: 'none', displayId, notes }; +} + +/** Search for a keyword and resolve the top result */ +async function searchTop(query: string, notes: string[], progress: Progress): Promise { + progress(`Searching for "${query}"...`); + const { results } = await searchSkills(query); + const top = results[0]; + if (!top) return { kind: 'none', displayId: query, notes }; + + const topId = `${top.author}/${top.name}`; + notes.push(`Top result for "${query}": ${topId}`); + const resolved = await resolveSkill(topId, { progress }); + return { ...resolved, notes: [...notes, ...resolved.notes] }; +} diff --git a/packages/cli/src/utils/config-store.ts b/packages/cli/src/utils/config-store.ts index a4c6cdf..f93ac7f 100644 --- a/packages/cli/src/utils/config-store.ts +++ b/packages/cli/src/utils/config-store.ts @@ -7,6 +7,17 @@ export function getApiKey(): string | undefined { return process.env.SKILLX_API_KEY || (config.get('apiKey') as string | undefined); } +/** Where the active API key comes from; the environment wins over the config file. */ +export function getApiKeySource(): 'env' | 'config' | null { + if (process.env.SKILLX_API_KEY) return 'env'; + return config.get('apiKey') ? 'config' : null; +} + +/** Absolute path of the config file on this machine. */ +export function getConfigPath(): string { + return config.path; +} + export function setApiKey(key: string): void { config.set('apiKey', key); } From 9caf090ca23c4cd8d76f3139b5feff9418af7281 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 19:35:31 +0700 Subject: [PATCH 03/12] feat(mcp): add get_kit read tool over personal kits get_kit reads one kit's effective revision with the personal kit API's visibility: a public kit whose revision was authored public, reviewed and activated is readable by anyone, an owner (session or unscoped API key) reads their own kit, and everything else is not_found. With the feature flag off the tool fails with feature_disabled. The other four tools still never read the request's credentials. Co-Authored-By: Claude Opus 5.5 --- apps/web/app/lib/mcp/handlers-kits.test.ts | 117 +++++++++++++++++++++ apps/web/app/lib/mcp/handlers-kits.ts | 72 +++++++++++++ apps/web/app/lib/mcp/handlers.ts | 8 +- apps/web/app/lib/mcp/mcp.test.ts | 3 +- apps/web/app/lib/mcp/protocol.ts | 13 +++ apps/web/app/lib/mcp/tools.ts | 20 +++- apps/web/app/routes/api.mcp.test.ts | 50 +++++++-- apps/web/app/routes/api.mcp.ts | 24 ++++- 8 files changed, 292 insertions(+), 15 deletions(-) create mode 100644 apps/web/app/lib/mcp/handlers-kits.test.ts create mode 100644 apps/web/app/lib/mcp/handlers-kits.ts diff --git a/apps/web/app/lib/mcp/handlers-kits.test.ts b/apps/web/app/lib/mcp/handlers-kits.test.ts new file mode 100644 index 0000000..e9fed92 --- /dev/null +++ b/apps/web/app/lib/mcp/handlers-kits.test.ts @@ -0,0 +1,117 @@ +/** + * `get_kit` must hide exactly what the personal kit API hides: a stranger sees + * a kit only when it is public and its effective revision was authored public, + * reviewed and activated; everything else looks like a kit that does not exist. + * The owner reads their own kit whatever its visibility. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const h = vi.hoisted(() => ({ kit: null as Record | null, revision: null as Record | null })); + +vi.mock("~/lib/personal-kits/kit-store", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + findKit: async () => h.kit, + getRevision: async () => h.revision, + }; +}); + +import { callTool } from "./tools"; + +const ENV = { SKILLX_PERSONAL_KITS_API: "1", DB: {} } as never; +const OWNER = "user_owner"; + +function kitRow(visibility: string, effective: string | null = "rev_1") { + return { + id: "kit_1", + name: "@alpha/kit", + publisher_id: "pub_1", + owner_user_id: OWNER, + local_id: "local-1", + visibility, + generation: 3, + effective_revision_id: effective, + latest_revision_number: 1, + created_at: 0, + updated_at: 0, + }; +} + +function revisionRow(state: string, docVisibility: string, activated = true) { + return { + id: "rev_1", + kit_id: "kit_1", + revision_number: 1, + parent_revision_id: null, + revision_digest: "sha256:abc", + document_json: JSON.stringify({ schema: "skillx.personal-kit/v1", kit: "@alpha/kit", visibility: docVisibility, skills: [] }), + state, + origin: "manual", + author_user_id: OWNER, + idempotency_key: "k", + approval_json: null, + created_at: 0, + reviewed_at: 0, + activated_at: activated ? 0 : null, + rejected_at: null, + quarantined_at: null, + }; +} + +async function getKit(userId: string | null, env = ENV, kit: unknown = "@alpha/kit") { + const result = await callTool("get_kit", { kit }, { env, userId }); + return { isError: result.isError === true, body: JSON.parse(result.content[0].text) as Record }; +} + +beforeEach(() => { + h.kit = null; + h.revision = null; +}); + +describe("get_kit", () => { + it("fails with feature_disabled while personal kits are off", async () => { + const { isError, body } = await getKit(null, { DB: {} } as never); + expect(isError).toBe(true); + expect(body.details.code).toBe("feature_disabled"); + }); + + it("rejects a malformed kit name as invalid arguments", async () => { + const { isError, body } = await getKit(null, ENV, "not a kit"); + expect(isError).toBe(true); + expect(body.details.code).toBe("invalid_arguments"); + }); + + it("returns a public, activated kit to anyone, with its document but without owner bookkeeping", async () => { + h.kit = kitRow("public"); + h.revision = revisionRow("reviewed", "public"); + const { body } = await getKit(null); + expect(body).toMatchObject({ found: true, viewer: "public", kit: { kit: "@alpha/kit", visibility: "public" } }); + expect(body.kit).not.toHaveProperty("localId"); + expect(body.effectiveRevision.document.visibility).toBe("public"); + }); + + it.each([ + ["an unknown kit", null, null], + ["a private kit", kitRow("private"), revisionRow("reviewed", "private")], + ["a public kit with no effective revision", kitRow("public", null), null], + ["a public kit whose revision was authored private", kitRow("public"), revisionRow("reviewed", "private")], + ["a public kit whose revision is not reviewed", kitRow("public"), revisionRow("draft", "public")], + ["a public kit whose revision was never activated", kitRow("public"), revisionRow("reviewed", "public", false)], + ])("answers not_found to a stranger for %s", async (_label, kit, revision) => { + h.kit = kit; + h.revision = revision; + const { isError, body } = await getKit("someone_else"); + expect(isError).toBe(false); + expect(body).toEqual({ found: false, code: "not_found", kit: "@alpha/kit" }); + }); + + it("lets the owner read their own private kit", async () => { + h.kit = kitRow("private"); + h.revision = revisionRow("reviewed", "private"); + const { body } = await getKit(OWNER); + expect(body).toMatchObject({ found: true, viewer: "owner", kit: { visibility: "private", localId: "local-1", generation: 3 } }); + expect(body.effectiveRevision.digest).toBe("sha256:abc"); + }); +}); diff --git a/apps/web/app/lib/mcp/handlers-kits.ts b/apps/web/app/lib/mcp/handlers-kits.ts new file mode 100644 index 0000000..a64a57b --- /dev/null +++ b/apps/web/app/lib/mcp/handlers-kits.ts @@ -0,0 +1,72 @@ +/** + * The `get_kit` MCP handler: a read of one personal kit's effective revision. + * + * Visibility follows the personal kit API and its parent rule + * (`kit-verify.ts`), so MCP never shows a kit that API would hide: + * - the owner (an unscoped API key or a web session) reads their own kit, + * private or public, as `GET /api/v1/kits/@h/n` would; + * - anyone else reads a kit only when it is public and its effective + * revision was authored public, reviewed and activated by its owner. + * Everything else (unknown, private, never activated) is `not_found`, so a + * kit's existence is not revealed. + * + * While `SKILLX_PERSONAL_KITS_API` is off the tool fails with + * `feature_disabled` instead of pretending the kit does not exist, so an agent + * can tell "turned off here" from "no such kit". + */ + +import { ReleaseHttpError } from "~/lib/releases/release-http"; +import { parseKitName, personalKitsApiEnabled } from "~/lib/personal-kits/kit-http"; +import { documentOf, findKit, getRevision, revisionView } from "~/lib/personal-kits/kit-store"; +import type { RevisionRow } from "~/lib/personal-kits/kit-store"; +import { ToolCodedError, ToolInputError, asRecord } from "./protocol"; +import { requiredString } from "./args"; +import type { ToolContext } from "./handlers"; + +/** Read by the public: reviewed, activated, and authored public (the foreign-parent rule). */ +const publiclyReadable = (row: RevisionRow) => + row.state === "reviewed" && row.activated_at !== null && documentOf(row).visibility === "public"; + +/** `@handle/name`, validated by the kit API's own parser. */ +function readKitName(args: Record): string { + const name = requiredString(args, "kit"); + const slash = name.indexOf("/"); + try { + return parseKitName(slash < 0 ? name : name.slice(0, slash), slash < 0 ? undefined : name.slice(slash + 1)); + } catch (error) { + if (error instanceof ReleaseHttpError) throw new ToolInputError(error.message); + throw error; + } +} + +export async function getKit(rawArgs: unknown, ctx: ToolContext) { + const name = readKitName(asRecord(rawArgs)); + if (!personalKitsApiEnabled(ctx.env as { SKILLX_PERSONAL_KITS_API?: string })) { + throw new ToolCodedError( + "feature_disabled", + "Personal kits are not enabled on this server (SKILLX_PERSONAL_KITS_API is off).", + ); + } + + const notFound = { found: false, code: "not_found", kit: name }; + const kit = await findKit(ctx.env.DB, name); + if (kit === null) return notFound; + + const owner = typeof ctx.userId === "string" && kit.owner_user_id === ctx.userId; + if (!owner && kit.visibility !== "public") return notFound; + + const effective = kit.effective_revision_id === null ? null : await getRevision(ctx.env.DB, kit.id, kit.effective_revision_id); + if (!owner && (effective === null || !publiclyReadable(effective))) return notFound; + + return { + found: true, + viewer: owner ? "owner" : "public", + kit: { + kit: kit.name, + visibility: kit.visibility, + // Owner-only bookkeeping stays with the owner. + ...(owner ? { localId: kit.local_id, generation: Number(kit.generation) } : {}), + }, + effectiveRevision: effective === null ? null : revisionView(effective, true), + }; +} diff --git a/apps/web/app/lib/mcp/handlers.ts b/apps/web/app/lib/mcp/handlers.ts index 8aa3271..10ad8ae 100644 --- a/apps/web/app/lib/mcp/handlers.ts +++ b/apps/web/app/lib/mcp/handlers.ts @@ -1,7 +1,7 @@ /** * MCP tool registry. * - * The handler bodies live in `handlers-packages.ts` and `handlers-releases.ts`; this + * The handler bodies live in `handlers-packages.ts`, `handlers-releases.ts` and `handlers-kits.ts`; this * module only names them, so `ToolName` is the single list of what the server can * do. `tools.ts` asserts that every advertised tool has an entry here, which is * what stops a tool from being listed and then refusing to run. @@ -9,15 +9,18 @@ import { searchPackages, getPackage, checkCompatibility } from "./handlers-packages"; import { getRelease } from "./handlers-releases"; +import { getKit } from "./handlers-kits"; export type ToolName = | "search_packages" | "get_package" | "get_release" - | "check_compatibility"; + | "check_compatibility" + | "get_kit"; export interface ToolContext { env: Env; + /** The authenticated caller; the route sets it for `get_kit` only, every other tool runs anonymous. */ userId?: string | null; } @@ -28,4 +31,5 @@ export const HANDLERS: Record = { get_package: (args, ctx) => getPackage(args, ctx), get_release: (args, ctx) => getRelease(args, ctx), check_compatibility: (args, ctx) => checkCompatibility(args, ctx), + get_kit: (args, ctx) => getKit(args, ctx), }; diff --git a/apps/web/app/lib/mcp/mcp.test.ts b/apps/web/app/lib/mcp/mcp.test.ts index 92c5864..b07c746 100644 --- a/apps/web/app/lib/mcp/mcp.test.ts +++ b/apps/web/app/lib/mcp/mcp.test.ts @@ -101,9 +101,10 @@ describe("envelope builders", () => { }); describe("tool catalog", () => { - it("exposes exactly the four read-only tools", () => { + it("exposes exactly the five read-only tools", () => { expect([...TOOL_NAMES].sort()).toEqual([ "check_compatibility", + "get_kit", "get_package", "get_release", "search_packages", diff --git a/apps/web/app/lib/mcp/protocol.ts b/apps/web/app/lib/mcp/protocol.ts index 4cafe2b..7c259f6 100644 --- a/apps/web/app/lib/mcp/protocol.ts +++ b/apps/web/app/lib/mcp/protocol.ts @@ -51,6 +51,19 @@ export interface ToolResult { /** Thrown by a handler when the caller's arguments are unusable. */ export class ToolInputError extends Error {} +/** + * Thrown by a handler that declines for a stable, machine-readable reason + * (e.g. `feature_disabled`). The code and message reach the caller. + */ +export class ToolCodedError extends Error { + constructor( + readonly code: string, + message: string, + ) { + super(message); + } +} + /** * Narrows `params.arguments` to a plain object. Anything else (a list, a string, * `null`) becomes `{}`, so handlers validate fields rather than shapes. diff --git a/apps/web/app/lib/mcp/tools.ts b/apps/web/app/lib/mcp/tools.ts index 2e21af6..72c25a7 100644 --- a/apps/web/app/lib/mcp/tools.ts +++ b/apps/web/app/lib/mcp/tools.ts @@ -9,7 +9,7 @@ */ import { HANDLERS, type ToolContext, type ToolName } from "./handlers"; -import { ToolInputError, asRecord, errorResult, textResult, type ToolResult } from "./protocol"; +import { ToolCodedError, ToolInputError, asRecord, errorResult, textResult, type ToolResult } from "./protocol"; export interface McpToolDefinition { name: ToolName; @@ -89,6 +89,23 @@ export const TOOL_DEFINITIONS: McpToolDefinition[] = [ additionalProperties: false, }, }, + { + name: "get_kit", + title: "Get kit", + description: + "Read one personal kit (`kit` is `@handle/name`) and its effective revision document. " + + "Anyone can read a public kit whose effective revision was authored public and activated by " + + "its owner; the owner, authenticated with a web session or an unscoped API key, can also read " + + "their own private kit. Every other kit, drafts and proposals included, answers `found: false` " + + "with code `not_found`. When personal kits are turned off on this server the call fails with " + + "code `feature_disabled`.", + inputSchema: { + type: "object", + properties: { kit: STRING }, + required: ["kit"], + additionalProperties: false, + }, + }, ]; /** The only failure detail a caller sees when a handler throws unexpectedly. */ @@ -119,6 +136,7 @@ export async function callTool( return textResult(await HANDLERS[name](asRecord(rawArgs), ctx)); } catch (error) { if (error instanceof ToolInputError) return errorResult(error.message, { code: "invalid_arguments" }); + if (error instanceof ToolCodedError) return errorResult(error.message, { code: error.code }); // The cause stays in the server log: driver and query text never reach the caller. console.error(`MCP tool ${name} failed:`, error); return errorResult(`${name} failed`, { code: TOOL_INTERNAL_ERROR }); diff --git a/apps/web/app/routes/api.mcp.test.ts b/apps/web/app/routes/api.mcp.test.ts index babebe5..d8031f3 100644 --- a/apps/web/app/routes/api.mcp.test.ts +++ b/apps/web/app/routes/api.mcp.test.ts @@ -11,6 +11,9 @@ import { describe, expect, it, vi } from "vitest"; const authSpy = vi.hoisted(() => vi.fn(async () => ({ userId: "user-with-session" }))); vi.mock("~/lib/auth/authenticate-request", () => ({ authenticateRequest: authSpy })); +const keySpy = vi.hoisted(() => vi.fn(async (_db: unknown, key: string) => (key === "sx_good" ? { userId: "user-with-key" } : null))); +vi.mock("~/lib/auth/authenticate-api-key", () => ({ authenticateApiKey: keySpy })); +vi.mock("~/lib/db", () => ({ getDb: () => ({}) })); const callToolSpy = vi.hoisted(() => ({ ctx: [] as Array<{ userId?: string | null }> })); vi.mock("~/lib/mcp/tools", async (importOriginal) => { @@ -60,12 +63,45 @@ describe("POST /api/mcp", () => { expect(json.result.instructions).toContain("never read `unknown` as support"); }); - it("lists the four read-only tools", async () => { + it("lists the five read-only tools", async () => { const { response, json } = await post('{"jsonrpc":"2.0","id":2,"method":"tools/list"}'); expect(response.status).toBe(200); - expect(json.result.tools).toHaveLength(4); - expect(json.result.tools.map((t: { name: string }) => t.name)).not.toContain("get_kit"); - expect(json.result.tools.map((t: { name: string }) => t.name)).toContain("check_compatibility"); + expect(json.result.tools.map((t: { name: string }) => t.name).sort()).toEqual([ + "check_compatibility", + "get_kit", + "get_package", + "get_release", + "search_packages", + ]); + }); + + it("resolves the caller for get_kit only, from the session or an unscoped key", async () => { + callToolSpy.ctx.length = 0; + const call = JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }); + await post(call, {}, { Cookie: "session=abc" }); + await post(call, {}, { Authorization: "Bearer sx_good" }); + expect(callToolSpy.ctx.map((ctx) => ctx.userId)).toEqual(["user-with-session", "user-with-key"]); + }); + + it("reports a rejected API key on get_kit as a tool error, never as anonymous", async () => { + callToolSpy.ctx.length = 0; + const { response, json } = await post( + JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }), + {}, + { Authorization: "Bearer sx_bad" }, + ); + expect(response.status).toBe(200); + expect(json.result.isError).toBe(true); + expect(JSON.parse(json.result.content[0].text).details.code).toBe("invalid_api_key"); + expect(callToolSpy.ctx).toHaveLength(0); + }); + + it("answers get_kit with feature_disabled while personal kits are off", async () => { + const { json } = await post( + JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }), + ); + expect(json.result.isError).toBe(true); + expect(JSON.parse(json.result.content[0].text).details.code).toBe("feature_disabled"); }); it("returns an unknown tool as a JSON-RPC success with isError, not a transport error", async () => { @@ -78,8 +114,9 @@ describe("POST /api/mcp", () => { expect(json.result.isError).toBe(true); }); - it("runs every tool as the anonymous viewer and never reads the request's credentials", async () => { + it("runs every tool but get_kit as the anonymous viewer and never reads the request's credentials", async () => { authSpy.mockClear(); + keySpy.mockClear(); callToolSpy.ctx.length = 0; for (const name of ["search_packages", "get_package", "check_compatibility", "get_release"]) { await post( @@ -89,6 +126,7 @@ describe("POST /api/mcp", () => { ); } expect(authSpy).not.toHaveBeenCalled(); + expect(keySpy).not.toHaveBeenCalled(); expect(callToolSpy.ctx).toHaveLength(4); expect(callToolSpy.ctx.every((ctx) => ctx.userId === null)).toBe(true); }); @@ -109,7 +147,7 @@ describe("POST /api/mcp", () => { const { response, json } = await post('{"jsonrpc":"2.0","id":0,"method":"tools/list"}'); expect(response.status).toBe(200); expect(json.id).toBe(0); - expect(json.result.tools).toHaveLength(4); + expect(json.result.tools).toHaveLength(5); }); it("reports malformed JSON as a parse error with a 400", async () => { diff --git a/apps/web/app/routes/api.mcp.ts b/apps/web/app/routes/api.mcp.ts index c6d99b9..503407f 100644 --- a/apps/web/app/routes/api.mcp.ts +++ b/apps/web/app/routes/api.mcp.ts @@ -9,8 +9,10 @@ * Two protocol rules are enforced here rather than in the tool layer: * - a request with no `id` is a notification and is answered with 202 and no * body; replying to one is a protocol violation. - * - every tool runs as the anonymous viewer; the request's session or API key is - * never read. + * - every tool except `get_kit` runs as the anonymous viewer, and for them the + * request's session or API key is never read. `get_kit` resolves the caller + * the way the personal kit API does (unscoped API key or session), so an + * owner can read their own private kit; a bad key is a tool error. * - the tool handlers are thin adapters over the same executors as the HTTP * API and the CLI, so MCP cannot drift from them. * @@ -26,10 +28,13 @@ import type { ActionFunctionArgs, LoaderFunctionArgs } from "react-router"; import { checkReleaseRateLimit, rateLimitCaller } from "~/lib/releases/release-rate-limit"; import { readBoundedBody, MAX_MCP_BODY_BYTES } from "~/lib/mcp/request-body"; import { callTool, TOOL_DEFINITIONS } from "~/lib/mcp/tools"; +import { resolveViewer } from "~/lib/releases/release-read"; +import { ReleaseHttpError } from "~/lib/releases/release-http"; import { MCP_PROTOCOL_VERSION, RPC, SERVER_NAME, + errorResult, SERVER_VERSION, isNotification, normalizeId, @@ -113,9 +118,18 @@ export async function action({ request, context }: ActionFunctionArgs) { { headers: JSON_HEADERS }, ); } - // Every MCP caller is the anonymous viewer: no session or API key is read, so a - // credential on the request never widens what any tool returns. - const result = await callTool(params.name, params.arguments, { env, userId: null }); + // Only `get_kit` reads the caller, and only to show an owner their own kit. Every + // other tool stays anonymous, so a credential never widens what they return. + let userId: string | null = null; + if (params.name === "get_kit") { + try { + userId = await resolveViewer(request, env); + } catch (error) { + if (!(error instanceof ReleaseHttpError)) throw error; + return Response.json(rpcResult(id, errorResult(error.message, { code: error.code })), { headers: JSON_HEADERS }); + } + } + const result = await callTool(params.name, params.arguments, { env, userId }); return Response.json(rpcResult(id, result), { headers: JSON_HEADERS }); } From ad879b697895292543494c160e780e6bfa0cdbd7 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 19:37:34 +0700 Subject: [PATCH 04/12] feat(webmcp): mirror get_package and get_release from remote MCP The in-page tools post a tools/call to the same-origin /api/mcp route and return that tool's answer, so access codes and protected-payload rules stay on the server. Names, schemas and descriptions are held equal to the remote definitions by a test. Co-Authored-By: Claude Opus 5.5 --- .../app/lib/webmcp/mcp-mirror-tools.test.ts | 65 ++++++++++++++ apps/web/app/lib/webmcp/mcp-mirror-tools.ts | 86 +++++++++++++++++++ apps/web/app/lib/webmcp/read-tools.ts | 5 ++ apps/web/app/lib/webmcp/webmcp.test.ts | 10 ++- 4 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 apps/web/app/lib/webmcp/mcp-mirror-tools.test.ts create mode 100644 apps/web/app/lib/webmcp/mcp-mirror-tools.ts diff --git a/apps/web/app/lib/webmcp/mcp-mirror-tools.test.ts b/apps/web/app/lib/webmcp/mcp-mirror-tools.test.ts new file mode 100644 index 0000000..ab1475c --- /dev/null +++ b/apps/web/app/lib/webmcp/mcp-mirror-tools.test.ts @@ -0,0 +1,65 @@ +/** + * The in-page `get_package` and `get_release` must be the remote MCP tools, + * not look-alikes: same names, same schemas and descriptions, and the answer + * of the real `/api/mcp` route for the same arguments. + */ + +import { describe, expect, it } from "vitest"; +import { createMcpMirrorTools, MIRRORED_TOOL_NAMES } from "./mcp-mirror-tools"; +import { TOOL_DEFINITIONS, callTool } from "~/lib/mcp/tools"; +import { action } from "~/routes/api.mcp"; + +/** A fetch that answers through the real MCP route, as the same origin would. */ +function routeFetch(env: Record = {}) { + const calls: Array<{ url: string; body: unknown }> = []; + const impl = (async (input: RequestInfo | URL, init?: RequestInit) => { + const request = new Request(new URL(String(input), "https://skillx.sh"), init); + calls.push({ url: request.url, body: JSON.parse(String(init?.body)) }); + return (await action({ request, context: { cloudflare: { env } } } as never)) as Response; + }) as typeof fetch; + return { impl, calls }; +} + +const fixedFetch = (body: string, status = 200) => + (async () => new Response(body, { status, headers: { "Content-Type": "application/json" } })) as unknown as typeof fetch; + +describe("WebMCP mirror of remote MCP tools", () => { + it("advertises the remote tools' names, schemas and descriptions unchanged", () => { + const local = createMcpMirrorTools({ fetchImpl: fixedFetch("{}") }); + for (const name of MIRRORED_TOOL_NAMES) { + const remote = TOOL_DEFINITIONS.find((t) => t.name === name)!; + const mirror = local.find((t) => t.name === name)!; + expect(mirror.inputSchema).toEqual(remote.inputSchema); + expect(mirror.description).toBe(remote.description); + } + }); + + it("returns exactly what the remote get_release answers", async () => { + const { impl, calls } = routeFetch(); + const [, getRelease] = createMcpMirrorTools({ fetchImpl: impl, baseUrl: "https://skillx.sh/" }); + const args = { package: "@alpha/skill", version: "1.0.0" }; + + const viaPage = await getRelease.execute(args); + const remote = await callTool("get_release", args, { env: {} as never, userId: null }); + + expect(calls[0].url).toBe("https://skillx.sh/api/mcp"); + expect(calls[0].body).toMatchObject({ method: "tools/call", params: { name: "get_release", arguments: args } }); + expect(viaPage).toEqual(JSON.parse(remote.content[0].text)); + expect(viaPage).toMatchObject({ found: false, code: "not_found" }); + }); + + it("keeps a tool error distinguishable from an answer", async () => { + const { impl } = routeFetch(); + const [getPackage] = createMcpMirrorTools({ fetchImpl: impl }); + expect(await getPackage.execute({})).toMatchObject({ isError: true, details: { code: "invalid_arguments" } }); + }); + + it("reports a JSON-RPC error and an unreadable body instead of a wrong shape", async () => { + const rpcError = JSON.stringify({ jsonrpc: "2.0", id: 1, error: { code: -32029, message: "Too many requests" } }); + const [, fromRpc] = createMcpMirrorTools({ fetchImpl: fixedFetch(rpcError, 429) }); + expect(await fromRpc.execute({ package: "@a/b" })).toEqual({ isError: true, error: "Too many requests", rpcCode: -32029, status: 429 }); + + const [, fromHtml] = createMcpMirrorTools({ fetchImpl: fixedFetch("", 502) }); + expect(await fromHtml.execute({ package: "@a/b" })).toMatchObject({ isError: true, status: 502 }); + }); +}); diff --git a/apps/web/app/lib/webmcp/mcp-mirror-tools.ts b/apps/web/app/lib/webmcp/mcp-mirror-tools.ts new file mode 100644 index 0000000..3bd5ad0 --- /dev/null +++ b/apps/web/app/lib/webmcp/mcp-mirror-tools.ts @@ -0,0 +1,86 @@ +/** + * WebMCP tools that mirror the remote MCP server's `get_package` and + * `get_release`. + * + * They do not re-implement either tool: each one posts a `tools/call` to the + * same-origin `/api/mcp` endpoint and returns that tool's own answer. So an + * in-page agent gets byte-for-byte what a remote MCP client gets, including the + * release access codes (`release_quarantined`, `publisher_revoked`, ...), and + * the danger and protected-payload rules stay on the server. The names, input + * schemas and descriptions match `lib/mcp/tools.ts`; a test holds them equal. + * + * The server module is not imported here because it pulls database code into + * the client bundle. + */ + +import type { WebMcpTool } from "./model-context"; + +const STRING = { type: "string" } as const; + +export const MIRRORED_TOOL_NAMES = ["get_package", "get_release"] as const; + +/** A tool-level failure stays distinguishable from a successful answer. */ +async function callRemoteTool(fetchImpl: typeof fetch, base: string, name: string, args: unknown): Promise { + const response = await fetchImpl(`${base}/api/mcp`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/call", params: { name, arguments: args ?? {} } }), + }); + + let rpc: { result?: { content?: Array<{ text?: string }>; isError?: boolean }; error?: { code: number; message: string } }; + try { + rpc = (await response.json()) as typeof rpc; + } catch { + return { isError: true, error: "Response was not valid JSON", status: response.status }; + } + if (rpc.error) return { isError: true, error: rpc.error.message, rpcCode: rpc.error.code, status: response.status }; + + const text = rpc.result?.content?.[0]?.text; + if (typeof text !== "string") return { isError: true, error: "MCP result carried no content", status: response.status }; + let value: unknown; + try { + value = JSON.parse(text) as unknown; + } catch { + return { isError: true, error: "MCP result was not valid JSON", status: response.status }; + } + return rpc.result?.isError ? { isError: true, ...(value as Record) } : value; +} + +export function createMcpMirrorTools(options: { fetchImpl: typeof fetch; baseUrl?: string }): WebMcpTool[] { + const { fetchImpl } = options; + const base = (options.baseUrl ?? "").replace(/\/+$/, ""); + + return [ + { + name: "get_package", + description: + "Fetch one listing by slug: metadata, references, scripts, and its declared compatibility. " + + "Pass `target` as `runtime` or `runtime@version` to also get the resolved answer for that " + + "runtime. Sets `contentIncluded` to say whether the SKILL.md body is part of this response; " + + "a protected listing answers without its body rather than with an empty one.", + inputSchema: { + type: "object", + properties: { slug: STRING, target: STRING }, + required: ["slug"], + additionalProperties: false, + }, + execute: (args) => callRemoteTool(fetchImpl, base, "get_package", args), + }, + { + name: "get_release", + description: + "Read the published releases of a public registry package (`package` is `@publisher/slug`), " + + "newest first, or resolve one `version`. Uses the same access rules as the public release " + + "API: private, unknown and draft releases answer `found: false` with code `not_found`; a " + + "quarantined tree answers `release_quarantined`, a revoked publisher `publisher_revoked`, " + + "and a yanked version `release_yanked`. Fields match `GET /api/v1/packages/:scope/:slug/releases`.", + inputSchema: { + type: "object", + properties: { package: STRING, version: STRING }, + required: ["package"], + additionalProperties: false, + }, + execute: (args) => callRemoteTool(fetchImpl, base, "get_release", args), + }, + ]; +} diff --git a/apps/web/app/lib/webmcp/read-tools.ts b/apps/web/app/lib/webmcp/read-tools.ts index e08b0b5..2d157b9 100644 --- a/apps/web/app/lib/webmcp/read-tools.ts +++ b/apps/web/app/lib/webmcp/read-tools.ts @@ -7,11 +7,15 @@ * different answer from the API, and the protected payload boundary stays in * exactly one place: the server. * + * `get_package` and `get_release` mirror the remote MCP tools of the same name + * (see `mcp-mirror-tools.ts`). + * * `fetchImpl` is injected so the tools are testable without a live network and * so the caller controls credentials and origin. */ import type { WebMcpTool } from "./model-context"; +import { createMcpMirrorTools } from "./mcp-mirror-tools"; const STRING = { type: "string" } as const; @@ -110,5 +114,6 @@ export function createReadTools(options: ReadToolOptions): WebMcpTool[] { return { slug: args.slug, target: args.target, compatibility: record.compatibility ?? null }; }, }, + ...createMcpMirrorTools(options), ]; } diff --git a/apps/web/app/lib/webmcp/webmcp.test.ts b/apps/web/app/lib/webmcp/webmcp.test.ts index ab0af33..08ea974 100644 --- a/apps/web/app/lib/webmcp/webmcp.test.ts +++ b/apps/web/app/lib/webmcp/webmcp.test.ts @@ -101,9 +101,15 @@ function fakeFetch(payload: unknown, capture: { url?: string; init?: RequestInit } describe("read tools", () => { - it("exposes the three read tools", () => { + it("exposes the read tools, including the two that mirror remote MCP", () => { const tools = createReadTools({ fetchImpl: fakeFetch({}), baseUrl: "https://skillx.sh/" }); - expect(tools.map((t) => t.name)).toEqual(["search_skills", "get_skill", "check_skill_compatibility"]); + expect(tools.map((t) => t.name)).toEqual([ + "search_skills", + "get_skill", + "check_skill_compatibility", + "get_package", + "get_release", + ]); }); it("posts a search to the same endpoint the page uses", async () => { From 1fa2dec4c3824bc2943394771ea293b5bc99ccb5 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 19:39:02 +0700 Subject: [PATCH 05/12] docs: document the CLI envelope, exit codes and new MCP read tools Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- docs/api-reference.md | 13 +++++++-- docs/cli-reference.md | 58 +++++++++++++++++++++++++++------------ packages/cli/src/index.ts | 2 +- 4 files changed, 54 insertions(+), 21 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index fdb1198..4aeeb94 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,7 +84,7 @@ pnpm db:migrate:remote # Apply migrations to remote D1 | `/skills/.md` | lib/markdown/skill-markdown-handler.ts, dispatched in `workers/app.ts` (React Router has no partial dynamic segment) | None; always anonymous, public cache | | `/llms.txt`, `/llms-full.txt` | llms-txt.ts, llms-full-txt.ts (`?limit=` snaps to fixed sizes; KV-cached per origin and size) | None; protected bodies withheld | | `/docs/harness`, `/docs/publishers` | docs-harness.tsx, docs-publishers.tsx | None | -| `/api/mcp` | api.mcp.ts (read-only MCP over JSON-RPC: `search_packages`, `get_package`, `get_release`, `check_compatibility`; 64 KiB body cap, `RELEASE_READ_LIMITER` under `mcp:`) | Optional Key/Session; `get_release` is always anonymous | +| `/api/mcp` | api.mcp.ts (read-only MCP over JSON-RPC: `search_packages`, `get_package`, `get_release`, `check_compatibility`, `get_kit`; 64 KiB body cap, `RELEASE_READ_LIMITER` under `mcp:`) | None; only `get_kit` reads an unscoped Key/Session (owner view) | | `/profile` | profile.tsx | Session | | `/settings` | settings.tsx | Session | | `/api/auth/*` | auth-catchall.tsx | Better Auth | diff --git a/docs/api-reference.md b/docs/api-reference.md index 2e45aca..1e11039 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1765,8 +1765,10 @@ shown beside them. ### `POST /api/mcp` Remote MCP server (JSON-RPC 2.0, streamable HTTP; `GET` answers 405). Methods: -`initialize`, `tools/list`, `tools/call`; notifications get `202` with no body. Every tool runs -as the anonymous viewer: the request's session or API key is never read. +`initialize`, `tools/list`, `tools/call`; notifications get `202` with no body. Every tool except +`get_kit` runs as the anonymous viewer: the request's session or API key is never read. +`get_kit` alone reads an unscoped API key or a session to recognise the owner; a scoped, +unknown or revoked key is a tool error with `details.code = "invalid_api_key"`. | Tool | Arguments | Answer | |------|-----------|--------| @@ -1774,6 +1776,7 @@ as the anonymous viewer: the request's session or API key is never read. | `get_package` | `slug`, `target?` | Listing detail (alias-aware) and declared compatibility | | `get_release` | `package` (`@handle/slug`), `version?` | Public published releases only, through the release access module; private, draft, and unsigned rows are `not_found`, quarantined is `release_quarantined` | | `check_compatibility` | `slug`, `target` (`runtime[@version]`) | Version-aware status plus reason codes; returns the canonical slug | +| `get_kit` | `kit` (`@handle/name`) | One personal kit and its effective revision. The owner reads their own kit at any visibility; anyone else only a `public` kit whose effective revision is reviewed, activated and authored public, otherwise `not_found`. While `SKILLX_PERSONAL_KITS_API` is off it is a tool error with `details.code = "feature_disabled"` | A miss is `{ "found": false, "code": "not_found" }`. Invalid arguments are a tool error with `details.code = "invalid_arguments"`; an unexpected failure is a tool @@ -1782,6 +1785,12 @@ Guards: request bodies above 64 KiB get `413`; each client IP is counted on `RELEASE_READ_LIMITER` under an `mcp:` key and a refusal is `429` with `Retry-After` (a missing binding lets the request through). +**WebMCP.** Where the browser exposes `navigator.modelContext`, pages register read tools +for in-page agents (`app/lib/webmcp/`): `search_skills`, `get_skill`, +`check_skill_compatibility`, plus `get_package` and `get_release`, which post a `tools/call` +to this endpoint and return its answer unchanged (same names, schemas and access codes). +Without the API nothing is registered. + ## Error Format All error responses follow a consistent shape: diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 9c37388..09e59b3 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -64,6 +64,7 @@ Smart skill lookup and display with multiple identifier formats. | `-s, --search` | Force search mode regardless of identifier format | `false` | | `--include-refs` | Include references in output | `false` | | `--include-scripts` | Include scripts in output | `false` | +| `--json` | Envelope with the resolution (`kind`: `skill`, `repo` scan, or `none` when a search found nothing) and its `notes` | `false` | **Resolution Logic:** 1. Spaces in query → search mode @@ -129,18 +130,21 @@ Interactive search — browse results and select a skill. **Arguments:** - `` (required) — Search query string -**Options:** None +**Options:** +| Flag | Description | +|------|-------------| +| `--json` | Emit the hits as an envelope (`{ query, results, count }`) without prompting | **Flow:** 1. Displays numbered results: `[1]`, `[2]`, `[3]`... 2. Shows name, category, rating, description per result 3. Prompts: `Select a skill [1-N] or press Enter to cancel:` -4. Fetches and displays full skill details with content preview (30 lines max) +4. Shows the selected skill exactly as `skillx use ` would **Examples:** ```bash skillx find "testing" -skillx find "api integration" +skillx find "api integration" --json ``` --- @@ -308,8 +312,10 @@ Report skill usage outcome (requires API key). |------|-------------|---------| | `-m, --model ` | AI model used (e.g., `claude-sonnet-4`) | — | | `-d, --duration ` | Execution duration in milliseconds | — | +| `--json` | Machine-readable envelope | `false` | -**Requires:** API key configured. +**Requires:** API key configured. An invalid outcome or duration is a usage error (exit `2`) +checked before any request; a missing key is `auth_error` (exit `5`). **Examples:** ```bash @@ -324,6 +330,10 @@ skillx report awesome-skill partial -m gpt-4 -d 2500 Manage CLI configuration. +All three subcommands accept `--json`. `config show --json` reports the key only as +`apiKey: { configured, source, masked }`; the plaintext key is never printed. Under +`set-key --json` the prompt goes to stderr, so stdout stays one envelope. + #### `skillx config set-key` Set API key interactively. @@ -585,22 +595,36 @@ Code 7 is reserved for `rate_limited` in the shared CLI envelope. --- -## Error Handling +## JSON Output and Exit Codes -| Scenario | Message | -|----------|---------| -| 401 Unauthorized | "Authentication failed. Check your API key." | -| 403 Forbidden | "Permission denied. Must be collaborator/owner of repo." | -| 404 Not Found | "Skill not found: ``" / "No SKILL.md found in repository." | -| 429 Rate Limited | "Rate limited. Please try again later." | -| Invalid outcome | "Invalid outcome. Must be: success, failure, or partial" | -| Missing API key | "API key required. Run `skillx config set-key`" | -| Invalid URL | "Invalid URL format. Example: https://api.skillx.sh" | +Every command except `publish` and `release *` (which print their own JSON) accepts +`--json` and then writes exactly one envelope to stdout; spinners and prompts never reach +stdout in that mode. -Legacy commands exit with code 1 on error; installer commands use the typed -[exit codes](#exit-codes) above. +```json +{ "ok": true, "data": { "...": "command-specific payload" } } +{ "ok": false, "error": { "code": "not_found", "message": "...", "hint": "...", "details": {} } } +``` ---- +`hint` and `details` appear only when there is something concrete to say. The exit code +carries the same answer, so a script can branch without parsing either form: + +| Exit | `error.code` | Meaning | +|------|--------------|---------| +| `0` | — | Success (`check`: the target may be acted on) | +| `1` | `api_error`, `unexpected_error` | The API failed without a better class, or a bug | +| `2` | `usage_error` | Invalid arguments or options, unknown command, missing argument | +| `3` | `not_found` | The skill or resource does not exist | +| `4` | `incompatible` | `check`: the skill exists but the target may not act on it | +| `5` | `auth_error` | API key missing or rejected (`401`/`403`); the hint names `skillx config set-key` | +| `6` | `network_error` | The API could not be reached | +| `7` | `rate_limited` | The API answered `429`; retry later | + +Parse failures follow the same table: with `--json` on the command line, an unknown option +prints a `usage_error` envelope and exits `2`. `--help` and `--version` exit `0`. The +implementation lives in [`output.ts`](../packages/cli/src/lib/output.ts) and +[`run-command.ts`](../packages/cli/src/lib/run-command.ts); a command wraps its work in +`runCommand` and throws `CliError` for failures it classifies itself. ## GoClaw / AI Agent Integration diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 06e5955..dcb0f08 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -42,6 +42,6 @@ program.addCommand(promoteCommand); program.addCommand(releaseCommand); for (const command of [installCommand, updateCommand, uninstallCommand, listCommand, syncCommand, mcpCommand]) program.addCommand(command); -// Parse errors exit 2 (and print a usage_error envelope under --json); see docs/cli-output.md. +// Parse errors exit 2 (and print a usage_error envelope under --json); see docs/cli-reference.md. routeUsageErrors(program); await program.parseAsync(process.argv); From 6402eab10e3c5f9f1db2d08daebc3712cc599b64 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 20:15:20 +0700 Subject: [PATCH 06/12] fix(mcp): limit get_kit to metadata for non-owners and honour revoked publishers A stranger now sees a public kit as metadata and revision digest only, the reference a foreign ancestor gets in the lock; the document stays with the owner. A suspended or revoked publisher answers publisher_revoked. The route checks the personal kits flag before reading credentials and reports an unexpected viewer failure as internal_error. Co-Authored-By: Claude Opus 5.5 --- apps/web/app/lib/mcp/handlers-kits.test.ts | 56 +++++++++++++++++++--- apps/web/app/lib/mcp/handlers-kits.ts | 30 +++++++++--- apps/web/app/lib/mcp/tools.ts | 13 ++--- apps/web/app/routes/api.mcp.test.ts | 33 ++++++++----- apps/web/app/routes/api.mcp.ts | 14 ++++-- 5 files changed, 110 insertions(+), 36 deletions(-) diff --git a/apps/web/app/lib/mcp/handlers-kits.test.ts b/apps/web/app/lib/mcp/handlers-kits.test.ts index e9fed92..54c0243 100644 --- a/apps/web/app/lib/mcp/handlers-kits.test.ts +++ b/apps/web/app/lib/mcp/handlers-kits.test.ts @@ -7,7 +7,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -const h = vi.hoisted(() => ({ kit: null as Record | null, revision: null as Record | null })); +const h = vi.hoisted(() => ({ + kit: null as Record | null, + revision: null as Record | null, + publisherState: "active", +})); vi.mock("~/lib/personal-kits/kit-store", async (importOriginal) => { const actual = await importOriginal(); @@ -20,7 +24,11 @@ vi.mock("~/lib/personal-kits/kit-store", async (importOriginal) => { import { callTool } from "./tools"; -const ENV = { SKILLX_PERSONAL_KITS_API: "1", DB: {} } as never; +/** Only the publisher-state read reaches the database; kit rows come from the mocked store. */ +const DB = { + prepare: () => ({ bind: () => ({ first: async () => ({ state: h.publisherState }) }) }), +}; +const ENV = { SKILLX_PERSONAL_KITS_API: "1", DB } as never; const OWNER = "user_owner"; function kitRow(visibility: string, effective: string | null = "rev_1") { @@ -68,6 +76,7 @@ async function getKit(userId: string | null, env = ENV, kit: unknown = "@alpha/k beforeEach(() => { h.kit = null; h.revision = null; + h.publisherState = "active"; }); describe("get_kit", () => { @@ -83,13 +92,46 @@ describe("get_kit", () => { expect(body.details.code).toBe("invalid_arguments"); }); - it("returns a public, activated kit to anyone, with its document but without owner bookkeeping", async () => { + it.each([ + ["an anonymous caller", null], + ["a signed-in stranger", "someone_else"], + ])("shows %s a public, activated kit as metadata and digest only", async (_label, userId) => { + h.kit = kitRow("public"); + h.revision = revisionRow("reviewed", "public"); + const { body } = await getKit(userId); + expect(body).toEqual({ + found: true, + viewer: "public", + kit: { kit: "@alpha/kit", visibility: "public" }, + effectiveRevision: { digest: "sha256:abc", number: 1, activatedAt: new Date(0).toISOString() }, + }); + }); + + it("gives the owner of a public kit the full document", async () => { + h.kit = kitRow("public"); + h.revision = revisionRow("reviewed", "public"); + const { body } = await getKit(OWNER); + expect(body.viewer).toBe("owner"); + expect(body.effectiveRevision.document).toMatchObject({ kit: "@alpha/kit", visibility: "public" }); + }); + + it.each([ + ["a stranger", "someone_else"], + ["the owner", OWNER], + ])("answers publisher_revoked to %s when the publisher is not active", async (_label, userId) => { h.kit = kitRow("public"); h.revision = revisionRow("reviewed", "public"); - const { body } = await getKit(null); - expect(body).toMatchObject({ found: true, viewer: "public", kit: { kit: "@alpha/kit", visibility: "public" } }); - expect(body.kit).not.toHaveProperty("localId"); - expect(body.effectiveRevision.document.visibility).toBe("public"); + h.publisherState = "revoked"; + const { isError, body } = await getKit(userId); + expect(isError).toBe(false); + expect(body).toEqual({ found: false, code: "publisher_revoked", kit: "@alpha/kit" }); + }); + + it("still hides a private kit of a revoked publisher from a stranger as not_found", async () => { + h.kit = kitRow("private"); + h.revision = revisionRow("reviewed", "private"); + h.publisherState = "suspended"; + expect((await getKit("someone_else")).body.code).toBe("not_found"); }); it.each([ diff --git a/apps/web/app/lib/mcp/handlers-kits.ts b/apps/web/app/lib/mcp/handlers-kits.ts index a64a57b..34e5541 100644 --- a/apps/web/app/lib/mcp/handlers-kits.ts +++ b/apps/web/app/lib/mcp/handlers-kits.ts @@ -2,13 +2,18 @@ * The `get_kit` MCP handler: a read of one personal kit's effective revision. * * Visibility follows the personal kit API and its parent rule - * (`kit-verify.ts`), so MCP never shows a kit that API would hide: + * (`kit-verify.ts`), so MCP never shows more than that API would: * - the owner (an unscoped API key or a web session) reads their own kit, - * private or public, as `GET /api/v1/kits/@h/n` would; - * - anyone else reads a kit only when it is public and its effective - * revision was authored public, reviewed and activated by its owner. - * Everything else (unknown, private, never activated) is `not_found`, so a - * kit's existence is not revealed. + * private or public, with the effective revision's full document, as + * `GET /api/v1/kits/@h/n` would; + * - anyone else sees a kit only when it is public and its effective + * revision was authored public, reviewed and activated by its owner, and + * then only its metadata and revision digest, the same reference a foreign + * ancestor gets in `chainView` and the lock. The document stays with the + * owner. Everything else (unknown, private, never activated) is + * `not_found`, so a kit's existence is not revealed. + * - a kit whose publisher is suspended or revoked answers + * `publisher_revoked`, as `get_release` does for that publisher's packages. * * While `SKILLX_PERSONAL_KITS_API` is off the tool fails with * `feature_disabled` instead of pretending the kit does not exist, so an agent @@ -27,6 +32,11 @@ import type { ToolContext } from "./handlers"; const publiclyReadable = (row: RevisionRow) => row.state === "reviewed" && row.activated_at !== null && documentOf(row).visibility === "public"; +async function publisherActive(db: D1Database, publisherId: string): Promise { + const row = await db.prepare("SELECT state FROM publishers WHERE id = ?").bind(publisherId).first<{ state: string }>(); + return row?.state === "active"; +} + /** `@handle/name`, validated by the kit API's own parser. */ function readKitName(args: Record): string { const name = requiredString(args, "kit"); @@ -57,6 +67,7 @@ export async function getKit(rawArgs: unknown, ctx: ToolContext) { const effective = kit.effective_revision_id === null ? null : await getRevision(ctx.env.DB, kit.id, kit.effective_revision_id); if (!owner && (effective === null || !publiclyReadable(effective))) return notFound; + if (!(await publisherActive(ctx.env.DB, kit.publisher_id))) return { found: false, code: "publisher_revoked", kit: name }; return { found: true, @@ -67,6 +78,11 @@ export async function getKit(rawArgs: unknown, ctx: ToolContext) { // Owner-only bookkeeping stays with the owner. ...(owner ? { localId: kit.local_id, generation: Number(kit.generation) } : {}), }, - effectiveRevision: effective === null ? null : revisionView(effective, true), + effectiveRevision: + effective === null + ? null + : owner + ? revisionView(effective, true) + : { digest: effective.revision_digest, number: Number(effective.revision_number), activatedAt: revisionView(effective).activatedAt }, }; } diff --git a/apps/web/app/lib/mcp/tools.ts b/apps/web/app/lib/mcp/tools.ts index 72c25a7..f7c691a 100644 --- a/apps/web/app/lib/mcp/tools.ts +++ b/apps/web/app/lib/mcp/tools.ts @@ -93,12 +93,13 @@ export const TOOL_DEFINITIONS: McpToolDefinition[] = [ name: "get_kit", title: "Get kit", description: - "Read one personal kit (`kit` is `@handle/name`) and its effective revision document. " + - "Anyone can read a public kit whose effective revision was authored public and activated by " + - "its owner; the owner, authenticated with a web session or an unscoped API key, can also read " + - "their own private kit. Every other kit, drafts and proposals included, answers `found: false` " + - "with code `not_found`. When personal kits are turned off on this server the call fails with " + - "code `feature_disabled`.", + "Read one personal kit (`kit` is `@handle/name`) and its effective revision. The owner, " + + "authenticated with a web session or an unscoped API key, reads their own kit (private too) " + + "with the full revision document. Anyone else sees a public kit whose effective revision was " + + "authored public and activated by its owner as metadata and revision digest only. Every other " + + "kit, drafts and proposals included, answers `found: false` with code `not_found`; a suspended " + + "or revoked publisher answers `publisher_revoked`. When personal kits are turned off on this " + + "server the call fails with code `feature_disabled`.", inputSchema: { type: "object", properties: { kit: STRING }, diff --git a/apps/web/app/routes/api.mcp.test.ts b/apps/web/app/routes/api.mcp.test.ts index d8031f3..9851637 100644 --- a/apps/web/app/routes/api.mcp.test.ts +++ b/apps/web/app/routes/api.mcp.test.ts @@ -31,6 +31,8 @@ import { MCP_PROTOCOL_VERSION, RPC } from "~/lib/mcp/protocol"; import { MAX_MCP_BODY_BYTES } from "~/lib/mcp/request-body"; const CTX = { cloudflare: { env: {} } }; +const KITS_ON = { SKILLX_PERSONAL_KITS_API: "1" }; +const GET_KIT = JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }); async function post(body: BodyInit, env: Record = {}, headers: Record = {}) { const response = (await action({ @@ -77,31 +79,38 @@ describe("POST /api/mcp", () => { it("resolves the caller for get_kit only, from the session or an unscoped key", async () => { callToolSpy.ctx.length = 0; - const call = JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }); - await post(call, {}, { Cookie: "session=abc" }); - await post(call, {}, { Authorization: "Bearer sx_good" }); + await post(GET_KIT, KITS_ON, { Cookie: "session=abc" }); + await post(GET_KIT, KITS_ON, { Authorization: "Bearer sx_good" }); expect(callToolSpy.ctx.map((ctx) => ctx.userId)).toEqual(["user-with-session", "user-with-key"]); }); it("reports a rejected API key on get_kit as a tool error, never as anonymous", async () => { callToolSpy.ctx.length = 0; - const { response, json } = await post( - JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }), - {}, - { Authorization: "Bearer sx_bad" }, - ); + const { response, json } = await post(GET_KIT, KITS_ON, { Authorization: "Bearer sx_bad" }); expect(response.status).toBe(200); expect(json.result.isError).toBe(true); expect(JSON.parse(json.result.content[0].text).details.code).toBe("invalid_api_key"); expect(callToolSpy.ctx).toHaveLength(0); }); - it("answers get_kit with feature_disabled while personal kits are off", async () => { - const { json } = await post( - JSON.stringify({ jsonrpc: "2.0", id: 9, method: "tools/call", params: { name: "get_kit", arguments: { kit: "@a/k" } } }), - ); + it("answers get_kit with feature_disabled while personal kits are off, without reading credentials", async () => { + keySpy.mockClear(); + const { json } = await post(GET_KIT, {}, { Authorization: "Bearer sx_bad" }); expect(json.result.isError).toBe(true); expect(JSON.parse(json.result.content[0].text).details.code).toBe("feature_disabled"); + expect(keySpy).not.toHaveBeenCalled(); + }); + + it("reports an unexpected viewer failure on get_kit as internal_error, without the cause", async () => { + keySpy.mockImplementationOnce(async () => { + throw new Error("D1 exploded: secret detail"); + }); + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const { response, json } = await post(GET_KIT, KITS_ON, { Authorization: "Bearer sx_good" }); + error.mockRestore(); + expect(response.status).toBe(200); + expect(json.result.isError).toBe(true); + expect(JSON.parse(json.result.content[0].text)).toEqual({ error: "get_kit failed", details: { code: "internal_error" } }); }); it("returns an unknown tool as a JSON-RPC success with isError, not a transport error", async () => { diff --git a/apps/web/app/routes/api.mcp.ts b/apps/web/app/routes/api.mcp.ts index 503407f..d0c90e2 100644 --- a/apps/web/app/routes/api.mcp.ts +++ b/apps/web/app/routes/api.mcp.ts @@ -27,9 +27,10 @@ import type { ActionFunctionArgs, LoaderFunctionArgs } from "react-router"; import { checkReleaseRateLimit, rateLimitCaller } from "~/lib/releases/release-rate-limit"; import { readBoundedBody, MAX_MCP_BODY_BYTES } from "~/lib/mcp/request-body"; -import { callTool, TOOL_DEFINITIONS } from "~/lib/mcp/tools"; +import { callTool, TOOL_DEFINITIONS, TOOL_INTERNAL_ERROR } from "~/lib/mcp/tools"; import { resolveViewer } from "~/lib/releases/release-read"; import { ReleaseHttpError } from "~/lib/releases/release-http"; +import { personalKitsApiEnabled } from "~/lib/personal-kits/kit-http"; import { MCP_PROTOCOL_VERSION, RPC, @@ -121,12 +122,17 @@ export async function action({ request, context }: ActionFunctionArgs) { // Only `get_kit` reads the caller, and only to show an owner their own kit. Every // other tool stays anonymous, so a credential never widens what they return. let userId: string | null = null; - if (params.name === "get_kit") { + // With personal kits off the tool answers `feature_disabled` without touching credentials. + if (params.name === "get_kit" && personalKitsApiEnabled(env as { SKILLX_PERSONAL_KITS_API?: string })) { try { userId = await resolveViewer(request, env); } catch (error) { - if (!(error instanceof ReleaseHttpError)) throw error; - return Response.json(rpcResult(id, errorResult(error.message, { code: error.code })), { headers: JSON_HEADERS }); + if (error instanceof ReleaseHttpError) { + return Response.json(rpcResult(id, errorResult(error.message, { code: error.code })), { headers: JSON_HEADERS }); + } + // Same shape as any other unexpected tool failure; the cause is only logged. + console.error("[mcp] get_kit viewer resolution failed", error); + return Response.json(rpcResult(id, errorResult("get_kit failed", { code: TOOL_INTERNAL_ERROR })), { headers: JSON_HEADERS }); } } const result = await callTool(params.name, params.arguments, { env, userId }); From fb029653f66f96d5f0f0790f6afd2482479b18dd Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 20:15:20 +0700 Subject: [PATCH 07/12] fix(cli): exit 0 for help subcommand, not_found for an empty use, and read --json only before -- Co-Authored-By: Claude Opus 5.5 --- docs/api-reference.md | 2 +- docs/cli-reference.md | 11 ++++++++--- packages/cli/src/built-cli.test.ts | 30 +++++++++++++++++++++++++++++ packages/cli/src/commands/use.ts | 7 +++++++ packages/cli/src/lib/run-command.ts | 12 +++++++----- 5 files changed, 53 insertions(+), 9 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index 1e11039..c417692 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1776,7 +1776,7 @@ unknown or revoked key is a tool error with `details.code = "invalid_api_key"`. | `get_package` | `slug`, `target?` | Listing detail (alias-aware) and declared compatibility | | `get_release` | `package` (`@handle/slug`), `version?` | Public published releases only, through the release access module; private, draft, and unsigned rows are `not_found`, quarantined is `release_quarantined` | | `check_compatibility` | `slug`, `target` (`runtime[@version]`) | Version-aware status plus reason codes; returns the canonical slug | -| `get_kit` | `kit` (`@handle/name`) | One personal kit and its effective revision. The owner reads their own kit at any visibility; anyone else only a `public` kit whose effective revision is reviewed, activated and authored public, otherwise `not_found`. While `SKILLX_PERSONAL_KITS_API` is off it is a tool error with `details.code = "feature_disabled"` | +| `get_kit` | `kit` (`@handle/name`) | One personal kit and its effective revision. The owner reads their own kit at any visibility, with the full revision document. Anyone else sees only a `public` kit whose effective revision is reviewed, activated and authored public, and then only its metadata and `{ digest, number, activatedAt }` (the reference a foreign ancestor gets in the lock); otherwise `not_found`. A suspended or revoked publisher answers `publisher_revoked`. While `SKILLX_PERSONAL_KITS_API` is off it is a tool error with `details.code = "feature_disabled"` and credentials are not read | A miss is `{ "found": false, "code": "not_found" }`. Invalid arguments are a tool error with `details.code = "invalid_arguments"`; an unexpected failure is a tool diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 09e59b3..96c1b3b 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -64,7 +64,7 @@ Smart skill lookup and display with multiple identifier formats. | `-s, --search` | Force search mode regardless of identifier format | `false` | | `--include-refs` | Include references in output | `false` | | `--include-scripts` | Include scripts in output | `false` | -| `--json` | Envelope with the resolution (`kind`: `skill`, `repo` scan, or `none` when a search found nothing) and its `notes` | `false` | +| `--json` | Envelope with the resolution (`kind`: `skill` or `repo` scan) and its `notes`; nothing found is `not_found` (exit `3`) | `false` | **Resolution Logic:** 1. Spaces in query → search mode @@ -615,13 +615,18 @@ carries the same answer, so a script can branch without parsing either form: | `1` | `api_error`, `unexpected_error` | The API failed without a better class, or a bug | | `2` | `usage_error` | Invalid arguments or options, unknown command, missing argument | | `3` | `not_found` | The skill or resource does not exist | -| `4` | `incompatible` | `check`: the skill exists but the target may not act on it | +| `4` | — | `check` only: `ok: true` with `data.actionable: false`; the skill exists but the target may not act on it | | `5` | `auth_error` | API key missing or rejected (`401`/`403`); the hint names `skillx config set-key` | | `6` | `network_error` | The API could not be reached | | `7` | `rate_limited` | The API answered `429`; retry later | +Exit `4` is an answer, not a failure, so its envelope is a success. Only flags before `--` +count: `skillx check -- --json` treats `--json` as an operand. + Parse failures follow the same table: with `--json` on the command line, an unknown option -prints a `usage_error` envelope and exits `2`. `--help` and `--version` exit `0`. The +prints a `usage_error` envelope and exits `2`. `--help`, `help ` and `--version` exit +`0`; a bare `skillx` or a group without its subcommand (`skillx config`) prints help and exits +`2`. The implementation lives in [`output.ts`](../packages/cli/src/lib/output.ts) and [`run-command.ts`](../packages/cli/src/lib/run-command.ts); a command wraps its work in `runCommand` and throws `CliError` for failures it classifies itself. diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 10c4d55..657fefc 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -186,6 +186,11 @@ describe("built cli", () => { it("answers check --json for all five statuses with the contract's decision and exit code", async () => { const server = createServer((req, res) => { const url = new URL(String(req.url), "http://stub"); + if (url.pathname === "/api/search") { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ results: [] })); + return; + } const status = url.pathname.replace("/api/skills/s-", ""); if (!(COMPATIBILITY_STATUSES as readonly string[]).includes(status)) { res.writeHead(404, { "Content-Type": "application/json" }); @@ -214,6 +219,10 @@ describe("built cli", () => { const missing = await cliExit("check", "s-nope", "--target", "agentkit", "--json"); expect(missing.code).toBe(3); expect(JSON.parse(missing.stdout)).toMatchObject({ ok: false, error: { code: "not_found" } }); + + const nothing = await cliExit("use", "no such skill anywhere", "--json"); + expect(nothing.code).toBe(3); + expect(JSON.parse(nothing.stdout)).toMatchObject({ ok: false, error: { code: "not_found" } }); } finally { await new Promise((resolve) => server.close(() => resolve())); } @@ -225,6 +234,27 @@ describe("built cli", () => { expect(JSON.parse(stdout)).toMatchObject({ ok: false, error: { code: "usage_error" } }); }); + it.each([["help", "check"], ["help", "check", "--json"], ["check", "--help", "--json"]])( + "treats `%s %s` as help, exit 0, with no usage_error envelope", + async (...args) => { + const { code, stdout } = await cliExit(...args); + expect(code).toBe(0); + expect(stdout).toContain("Usage: skillx check"); + expect(stdout).not.toContain("usage_error"); + }, + ); + + it("reports a missing command as usage_error with exit 2", async () => { + expect((await cliExit()).code).toBe(2); + expect((await cliExit("config")).code).toBe(2); + }); + + it("reads --json only before --", async () => { + const { code, stdout } = await cliExit("check", "--", "--json"); + expect(code).toBe(2); + expect(stdout).not.toContain("usage_error"); + }); + it("validates report arguments before any request", async () => { const { code, stdout } = await cliExit("report", "some-skill", "maybe", "--json"); expect(code).toBe(2); diff --git a/packages/cli/src/commands/use.ts b/packages/cli/src/commands/use.ts index e9c451d..20cd8b3 100644 --- a/packages/cli/src/commands/use.ts +++ b/packages/cli/src/commands/use.ts @@ -1,6 +1,7 @@ import { Command } from 'commander'; import chalk from 'chalk'; import { runCommand } from '../lib/run-command.js'; +import { CliError } from '../lib/output.js'; import { resolveSkill, type ResolvedSkill } from '../lib/skill-resolver.js'; import { discoveryOnlyNotice, displaySkill, trackInstall, type DisplayOptions } from './use-display.js'; @@ -90,6 +91,12 @@ export const useCommand = new Command('use') }, async (progress) => { resolved = await resolveSkill(identifier, { forceSearch: options.search, progress }); + if (resolved.kind === 'none') { + throw new CliError('not_found', `No skills found for ${resolved.displayId}`, { + details: { notes: resolved.notes }, + hint: 'Try different search terms or browse all skills at https://skillx.sh', + }); + } // Human display tracks the install itself; the envelope path counts the same use. if (json && resolved.kind === 'skill') trackInstall(resolved.detail.skill.slug); return toUsePayload(resolved); diff --git a/packages/cli/src/lib/run-command.ts b/packages/cli/src/lib/run-command.ts index 60d49d6..2e32b09 100644 --- a/packages/cli/src/lib/run-command.ts +++ b/packages/cli/src/lib/run-command.ts @@ -99,15 +99,17 @@ export async function runCommand( * Commander exits 1 on every parse problem. A caller has to tell "you called me * wrong" (2) from a failed run (1), and with `--json` it expects an envelope on * stdout, so parse errors are routed through the same vocabulary. Help and - * `--version` still exit 0. + * `--version` still exit 0; help shown because a command was called without + * its subcommand (`skillx`, `skillx config`) is a usage error. */ export function routeUsageErrors(program: Command, argv: readonly string[] = process.argv): void { - const json = argv.includes('--json'); + // After `--` everything is an operand, so a literal "--json" there is not the flag. + const end = argv.indexOf('--'); + const json = (end < 0 ? argv : argv.slice(0, end)).includes('--json'); const apply = (command: Command) => { command.exitOverride((error: CommanderError) => { - if (error.code === 'commander.helpDisplayed' || error.code === 'commander.version') { - process.exit(EXIT_CODES.ok); - } + // Help and version (`--help`, `help `, `--version`) are answers, not errors. + if (error.exitCode === 0) process.exit(EXIT_CODES.ok); const envelope = failure('usage_error', error.message.replace(/^error:\s*/, ''), { details: { reason: error.code }, hint: 'Run `skillx --help` or `skillx --help`.', From 045845f58cd675f55271b00c5c3195d172370dcb Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 20:48:05 +0700 Subject: [PATCH 08/12] fix(cli): report a closed stdin as usage_error and an empty match as not_found everywhere config set-key and find raced nothing against stdin closing, so an empty pipe left an unsettled await and Node exited 13 without an envelope. Both now read through promptLine and fail with usage_error. use, search --use and find share one not_found path for an empty resolution. The incompatible error code is gone: check reports exit 4 on a success envelope. Co-Authored-By: Claude Opus 5.5 --- docs/cli-reference.md | 11 +++++-- packages/cli/src/built-cli.test.ts | 34 ++++++++++++++++++-- packages/cli/src/commands/config.ts | 22 ++++++------- packages/cli/src/commands/find.ts | 16 ++++----- packages/cli/src/commands/use.ts | 50 ++++++++++++++--------------- packages/cli/src/lib/output.test.ts | 6 +--- packages/cli/src/lib/output.ts | 4 +-- packages/cli/src/lib/prompt-line.ts | 21 ++++++++++++ 8 files changed, 105 insertions(+), 59 deletions(-) create mode 100644 packages/cli/src/lib/prompt-line.ts diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 96c1b3b..069d95c 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -64,7 +64,10 @@ Smart skill lookup and display with multiple identifier formats. | `-s, --search` | Force search mode regardless of identifier format | `false` | | `--include-refs` | Include references in output | `false` | | `--include-scripts` | Include scripts in output | `false` | -| `--json` | Envelope with the resolution (`kind`: `skill` or `repo` scan) and its `notes`; nothing found is `not_found` (exit `3`) | `false` | +| `--json` | Envelope with the resolution (`kind`: `skill` or `repo` scan) and its `notes` | `false` | + +When nothing matches, `use` (and `search --use`, `find` after a pick) fails with `not_found` +and exits `3` in both modes; the human message goes to stderr. **Resolution Logic:** 1. Spaces in query → search mode @@ -141,6 +144,9 @@ Interactive search — browse results and select a skill. 3. Prompts: `Select a skill [1-N] or press Enter to cancel:` 4. Shows the selected skill exactly as `skillx use ` would +If stdin closes before a selection (no terminal, empty pipe), `find` fails with `usage_error` +(exit `2`); use `--json` in scripts. + **Examples:** ```bash skillx find "testing" @@ -332,7 +338,8 @@ Manage CLI configuration. All three subcommands accept `--json`. `config show --json` reports the key only as `apiKey: { configured, source, masked }`; the plaintext key is never printed. Under -`set-key --json` the prompt goes to stderr, so stdout stays one envelope. +`set-key --json` the prompt goes to stderr, so stdout stays one envelope. The key may be +piped on stdin; an empty or closed stdin is a `usage_error` (exit `2`). #### `skillx config set-key` diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 657fefc..4b3aec8 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; -import { execFile, execFileSync, execSync } from "node:child_process"; +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"; @@ -47,6 +47,18 @@ async function cliExit(...args: string[]): Promise<{ code: number; stdout: strin } } +/** Runs the built CLI with `input` on stdin, then closes it; the exit code is data. */ +function cliWithInput(input: string, ...args: string[]): Promise<{ code: number; stdout: string }> { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [BIN, ...args], { cwd: root, env: childEnv }); + let stdout = ""; + child.stdout.setEncoding("utf8").on("data", (chunk: string) => (stdout += chunk)); + child.on("error", reject); + child.on("close", (code) => resolve({ code: code ?? -1, stdout })); + child.stdin.end(input); + }); +} + beforeAll(() => { // Through a shell: on Windows pnpm is a .cmd shim that execFile cannot start directly. execSync("pnpm build", { cwd: CLI_DIR, stdio: "pipe" }); @@ -187,8 +199,14 @@ describe("built cli", () => { const server = createServer((req, res) => { const url = new URL(String(req.url), "http://stub"); if (url.pathname === "/api/search") { - res.writeHead(200, { "Content-Type": "application/json" }); - res.end(JSON.stringify({ results: [] })); + // One hit for "one hit", none for anything else. + let body = ""; + req.on("data", (chunk) => (body += chunk)); + req.on("end", () => { + const hit = { slug: "s-declared", name: "s-declared", author: "a", category: "c", description: "d" }; + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ results: body.includes("one hit") ? [hit] : [] })); + }); return; } const status = url.pathname.replace("/api/skills/s-", ""); @@ -223,6 +241,10 @@ describe("built cli", () => { const nothing = await cliExit("use", "no such skill anywhere", "--json"); expect(nothing.code).toBe(3); expect(JSON.parse(nothing.stdout)).toMatchObject({ ok: false, error: { code: "not_found" } }); + + // A prompt on a closed stdin is a usage error, not a hang or Node's exit 13. + const noPick = await cliWithInput("", "find", "one hit"); + expect(noPick.code).toBe(2); } finally { await new Promise((resolve) => server.close(() => resolve())); } @@ -255,6 +277,12 @@ describe("built cli", () => { expect(stdout).not.toContain("usage_error"); }); + it.each([[""], [" \n"]])("refuses config set-key --json with no key on stdin (%j) as usage_error", async (input) => { + const { code, stdout } = await cliWithInput(input, "config", "set-key", "--json"); + expect(code).toBe(2); + expect(JSON.parse(stdout)).toMatchObject({ ok: false, error: { code: "usage_error" } }); + }); + it("validates report arguments before any request", async () => { const { code, stdout } = await cliExit("report", "some-skill", "maybe", "--json"); expect(code).toBe(2); diff --git a/packages/cli/src/commands/config.ts b/packages/cli/src/commands/config.ts index b6a09ef..709d8d9 100644 --- a/packages/cli/src/commands/config.ts +++ b/packages/cli/src/commands/config.ts @@ -1,8 +1,8 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import * as readline from 'node:readline/promises'; -import { stdin as input, stdout, stderr } from 'node:process'; +import { stdout, stderr } from 'node:process'; import { CliError } from '../lib/output.js'; +import { promptLine } from '../lib/prompt-line.js'; import { runCommand } from '../lib/run-command.js'; import { getApiKey, @@ -38,18 +38,14 @@ configCommand return runCommand( { json }, async () => { - // With --json, stdout carries only the envelope, so the prompt is written to stderr. - const rl = readline.createInterface({ input, output: json ? stderr : stdout }); - let apiKey: string; - try { - if (!json) { - console.log(chalk.bold('\nSkillX API Key Configuration')); - console.log(chalk.dim('Get your API key from: https://skillx.sh/settings/api\n')); - } - apiKey = (await rl.question(json ? 'Enter your API key: ' : chalk.cyan('Enter your API key: '))).trim(); - } finally { - rl.close(); + if (!json) { + console.log(chalk.bold('\nSkillX API Key Configuration')); + console.log(chalk.dim('Get your API key from: https://skillx.sh/settings/api\n')); } + // With --json, stdout carries only the envelope, so the prompt is written to stderr. + const line = await promptLine(json ? 'Enter your API key: ' : chalk.cyan('Enter your API key: '), json ? stderr : stdout); + if (line === null) throw new CliError('usage_error', 'No API key on stdin.'); + const apiKey = line.trim(); if (apiKey.length === 0) throw new CliError('usage_error', 'API key cannot be empty.'); setApiKey(apiKey); diff --git a/packages/cli/src/commands/find.ts b/packages/cli/src/commands/find.ts index 8aa81cc..f398474 100644 --- a/packages/cli/src/commands/find.ts +++ b/packages/cli/src/commands/find.ts @@ -1,7 +1,8 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import * as readline from 'node:readline/promises'; -import { stdin as input, stdout as output } from 'node:process'; +import { stdout } from 'node:process'; +import { CliError } from '../lib/output.js'; +import { promptLine } from '../lib/prompt-line.js'; import { searchSkills, type SearchResult } from '../lib/search-api.js'; import { runCommand } from '../lib/run-command.js'; import { resolveAndUseSkill } from './use.js'; @@ -22,12 +23,11 @@ async function pickAndUse(results: SearchResult[]): Promise { }); console.log(); - const rl = readline.createInterface({ input, output }); - let answer: string; - try { - answer = await rl.question(chalk.cyan(`Select a skill [1-${results.length}] or press Enter to cancel: `)); - } finally { - rl.close(); + const answer = await promptLine(chalk.cyan(`Select a skill [1-${results.length}] or press Enter to cancel: `), stdout); + if (answer === null) { + throw new CliError('usage_error', 'No selection on stdin.', { + hint: 'Use `skillx find --json` to list hits without prompting.', + }); } const choice = parseInt(answer.trim(), 10); diff --git a/packages/cli/src/commands/use.ts b/packages/cli/src/commands/use.ts index 20cd8b3..ce0ade2 100644 --- a/packages/cli/src/commands/use.ts +++ b/packages/cli/src/commands/use.ts @@ -15,8 +15,19 @@ interface UseOptions { json?: boolean; } -/** Human rendering of a resolution. */ -function renderResolved(resolved: ResolvedSkill, options: DisplayOptions): void { +type Match = Exclude; + +/** Nothing matched is `not_found` (exit 3) for `use`, `search --use` and `find` alike. */ +function requireMatch(resolved: ResolvedSkill): Match { + if (resolved.kind !== 'none') return resolved; + throw new CliError('not_found', `No skills found for ${resolved.displayId}`, { + details: { notes: resolved.notes }, + hint: 'Try different search terms or browse all skills at https://skillx.sh', + }); +} + +/** Human rendering of a match. */ +function renderResolved(resolved: Match, options: DisplayOptions): void { // In raw mode stdout is the skill content only; the detours go to stderr. const note = options.raw ? console.error : (text: string) => console.log(chalk.dim(text)); for (const text of resolved.notes) note(text); @@ -28,18 +39,12 @@ function renderResolved(resolved: ResolvedSkill, options: DisplayOptions): void return; } - if (resolved.kind === 'repo') { - console.log(chalk.bold.green(`\nFound ${resolved.skills.length} skill(s) in repo:\n`)); - resolved.skills.forEach((s) => console.log(` ${chalk.cyan(`${s.author}/${s.name}`)}`)); - console.log(chalk.dim(`\nUse ${chalk.cyan(`skillx use ${resolved.displayId}/`)} to use a specific skill`)); - if (resolved.registered) { - console.log(chalk.dim(`(${resolved.registered} newly registered, ${resolved.skipped} already existed)`)); - } - return; + console.log(chalk.bold.green(`\nFound ${resolved.skills.length} skill(s) in repo:\n`)); + resolved.skills.forEach((s) => console.log(` ${chalk.cyan(`${s.author}/${s.name}`)}`)); + console.log(chalk.dim(`\nUse ${chalk.cyan(`skillx use ${resolved.displayId}/`)} to use a specific skill`)); + if (resolved.registered) { + console.log(chalk.dim(`(${resolved.registered} newly registered, ${resolved.skipped} already existed)`)); } - - console.log(chalk.yellow(`\nNo skills found for ${resolved.displayId}`)); - console.log(chalk.dim('Try different search terms or browse all skills at https://skillx.sh')); } /** The `use --json` payload: the resolution, with the discovery-only decision made explicit. */ @@ -62,7 +67,7 @@ export function toUsePayload(resolved: ResolvedSkill) { /** Resolve identifier and display the skill. Exported for use by search --use. */ export async function resolveAndUseSkill(identifier: string, options: DisplayOptions): Promise { - renderResolved(await resolveSkill(identifier), options); + renderResolved(requireMatch(await resolveSkill(identifier)), options); } export const useCommand = new Command('use') @@ -81,7 +86,7 @@ export const useCommand = new Command('use') includeScripts: options.includeScripts ?? false, }; // The envelope flattens the skill; the human renderer needs the resolution itself. - let resolved: ResolvedSkill | undefined; + let resolved: Match | undefined; return runCommand( { json, @@ -90,17 +95,12 @@ export const useCommand = new Command('use') errors: { notFoundMessage: `Skill not found: ${identifier}` }, }, async (progress) => { - resolved = await resolveSkill(identifier, { forceSearch: options.search, progress }); - if (resolved.kind === 'none') { - throw new CliError('not_found', `No skills found for ${resolved.displayId}`, { - details: { notes: resolved.notes }, - hint: 'Try different search terms or browse all skills at https://skillx.sh', - }); - } + const match = requireMatch(await resolveSkill(identifier, { forceSearch: options.search, progress })); + resolved = match; // Human display tracks the install itself; the envelope path counts the same use. - if (json && resolved.kind === 'skill') trackInstall(resolved.detail.skill.slug); - return toUsePayload(resolved); + if (json && match.kind === 'skill') trackInstall(match.detail.skill.slug); + return toUsePayload(match); }, - () => renderResolved(resolved as ResolvedSkill, display), + () => renderResolved(resolved as Match, display), ); }); diff --git a/packages/cli/src/lib/output.test.ts b/packages/cli/src/lib/output.test.ts index a8b6475..6238d48 100644 --- a/packages/cli/src/lib/output.test.ts +++ b/packages/cli/src/lib/output.test.ts @@ -29,10 +29,6 @@ describe("exit codes", () => { expect(exitCodeFor(success({ anything: true }))).toBe(EXIT_CODES.ok); }); - it("is 4 for an incompatible target, which is not a generic failure", () => { - expect(exitCodeFor(failure("incompatible", "no"))).toBe(EXIT_CODES.incompatible); - }); - it("is 2 for a usage error and 3 for a missing skill", () => { expect(exitCodeFor(failure("usage_error", "no"))).toBe(EXIT_CODES.usage); expect(exitCodeFor(failure("not_found", "no"))).toBe(EXIT_CODES.notFound); @@ -109,7 +105,7 @@ describe("serialize", () => { it("carries machine-readable details when present", () => { const parsed = JSON.parse( - serialize(failure("incompatible", "blocked", { details: { reasonCodes: ["COMPAT_RANGE_MISMATCH"] } })), + serialize(failure("not_found", "blocked", { details: { reasonCodes: ["COMPAT_RANGE_MISMATCH"] } })), ); expect(parsed.error.details.reasonCodes).toEqual(["COMPAT_RANGE_MISMATCH"]); }); diff --git a/packages/cli/src/lib/output.ts b/packages/cli/src/lib/output.ts index c6416ae..7bdca9a 100644 --- a/packages/cli/src/lib/output.ts +++ b/packages/cli/src/lib/output.ts @@ -19,7 +19,7 @@ export const EXIT_CODES = { usage: 2, /** The skill does not exist. */ notFound: 3, - /** The skill exists but cannot run on the requested target. */ + /** `check` only: the skill exists but the target may not act on it. A success envelope, not an error code. */ incompatible: 4, /** Credentials are missing or rejected. */ auth: 5, @@ -36,7 +36,6 @@ export type ExitCode = (typeof EXIT_CODES)[keyof typeof EXIT_CODES]; export type CliErrorCode = | 'usage_error' | 'not_found' - | 'incompatible' | 'auth_error' | 'network_error' | 'rate_limited' @@ -48,7 +47,6 @@ export type CliErrorCode = const EXIT_BY_CODE: Record = { usage_error: EXIT_CODES.usage, not_found: EXIT_CODES.notFound, - incompatible: EXIT_CODES.incompatible, auth_error: EXIT_CODES.auth, network_error: EXIT_CODES.network, rate_limited: EXIT_CODES.rateLimited, diff --git a/packages/cli/src/lib/prompt-line.ts b/packages/cli/src/lib/prompt-line.ts new file mode 100644 index 0000000..b5ebf75 --- /dev/null +++ b/packages/cli/src/lib/prompt-line.ts @@ -0,0 +1,21 @@ +/** + * Reads one line from stdin, or `null` when stdin closes first. + * + * `rl.question` alone never settles on a closed or empty stdin, so Node exits 13 + * ("unsettled top-level await") with no envelope. Racing it against `close` + * lets the command report the missing input itself. + */ + +import * as readline from 'node:readline/promises'; +import { once } from 'node:events'; +import { stdin } from 'node:process'; + +export async function promptLine(question: string, output: NodeJS.WritableStream): Promise { + const rl = readline.createInterface({ input: stdin, output }); + const closed = once(rl, 'close').then(() => null); + try { + return await Promise.race([rl.question(question), closed]); + } finally { + rl.close(); + } +} From 9445324437cd93062cd0cbe6d63dad4ee6dd322d Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 20:50:50 +0700 Subject: [PATCH 09/12] test(cli): give built-cli smoke cases room under a full parallel run Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/built-cli.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 4b3aec8..07bb131 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -82,7 +82,8 @@ afterAll(() => { if (root) rmSync(root, { recursive: true, force: true }); }); -describe("built cli", () => { +// Each case spawns Node one or more times; under a full parallel run that can pass the 5s default. +describe("built cli", { timeout: 30_000 }, () => { it("keeps the runtime dependencies of bundled workspace packages external", () => { const bundledDeps = Object.keys(readJson(join(CLI_DIR, "..", "skill-tree", "package.json")).dependencies ?? {}); expect(Object.keys(MANIFEST.dependencies)).toEqual(expect.arrayContaining(bundledDeps)); From 5a6ee5368a07b78797deaec6b4b1f5b7a22afaf0 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 19 Oct 2026 21:11:05 +0700 Subject: [PATCH 10/12] fix(cli): accept a piped line without a trailing newline at a prompt Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/built-cli.test.ts | 18 ++++++++++++++++++ packages/cli/src/lib/prompt-line.ts | 10 +++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index 07bb131..bc798d4 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -246,6 +246,12 @@ describe("built cli", { timeout: 30_000 }, () => { // A prompt on a closed stdin is a usage error, not a hang or Node's exit 13. const noPick = await cliWithInput("", "find", "one hit"); expect(noPick.code).toBe(2); + + // A piped selection without a trailing newline still selects. + const picked = await cliWithInput("1", "find", "one hit"); + expect(picked.code).toBe(0); + expect(picked.stdout).toContain("s-declared"); + expect(picked.stdout).not.toContain("Cancelled"); } finally { await new Promise((resolve) => server.close(() => resolve())); } @@ -278,6 +284,18 @@ describe("built cli", { timeout: 30_000 }, () => { expect(stdout).not.toContain("usage_error"); }); + it.each([["unterminated", "pk_unterminated_9z"], ["newline-terminated", "pk_terminated_9z\n"]])( + "saves a %s key piped into config set-key --json", + async (_label, input) => { + const { code, stdout } = await cliWithInput(input, "config", "set-key", "--json"); + const envelope = JSON.parse(stdout) as { ok: boolean; data: { saved: boolean; path: string } }; + expect(code).toBe(0); + expect(envelope).toMatchObject({ ok: true, data: { saved: true } }); + expect(stdout).not.toContain(input.trim()); + expect(readJson(envelope.data.path).apiKey).toBe(input.trim()); + }, + ); + it.each([[""], [" \n"]])("refuses config set-key --json with no key on stdin (%j) as usage_error", async (input) => { const { code, stdout } = await cliWithInput(input, "config", "set-key", "--json"); expect(code).toBe(2); diff --git a/packages/cli/src/lib/prompt-line.ts b/packages/cli/src/lib/prompt-line.ts index b5ebf75..c2c8a19 100644 --- a/packages/cli/src/lib/prompt-line.ts +++ b/packages/cli/src/lib/prompt-line.ts @@ -2,8 +2,11 @@ * Reads one line from stdin, or `null` when stdin closes first. * * `rl.question` alone never settles on a closed or empty stdin, so Node exits 13 - * ("unsettled top-level await") with no envelope. Racing it against `close` - * lets the command report the missing input itself. + * ("unsettled top-level await") with no envelope. It also resolves only on a + * newline, while a piped value often has none (`printf 'key' |`, `echo -n`, + * secret managers). At end of input readline emits that last partial line as a + * `line` event before `close`, so the race listens for `line` first, then + * `close`. */ import * as readline from 'node:readline/promises'; @@ -12,9 +15,10 @@ import { stdin } from 'node:process'; export async function promptLine(question: string, output: NodeJS.WritableStream): Promise { const rl = readline.createInterface({ input: stdin, output }); + const line = once(rl, 'line').then(([text]) => String(text)); const closed = once(rl, 'close').then(() => null); try { - return await Promise.race([rl.question(question), closed]); + return await Promise.race([rl.question(question), line, closed]); } finally { rl.close(); } From 6d5835051b7671847469e5ef714ca71faa23a09b Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Tue, 20 Oct 2026 09:05:07 +0700 Subject: [PATCH 11/12] refactor(cli): route registry and installer commands through the shared envelope The registry commands had their own CliError and runner, and the installer its own error type, so the envelope and exit codes could drift apart. Both now use the shared CliError, runCommand and exit-code table; import moves off process.exit and gains --json, and a 429 from the registry is rate_limited. The docs keep one exit-code table (0-8), and a built-CLI test checks that every command offers --json and fails with an envelope. Co-Authored-By: Claude Opus 5.5 --- docs/cli-reference.md | 47 +++-- packages/cli/src/built-cli.test.ts | 33 ++++ packages/cli/src/commands/import.ts | 160 ++++++++---------- packages/cli/src/commands/install-runner.ts | 13 +- packages/cli/src/commands/publish.ts | 2 +- .../cli/src/commands/release-commands.test.ts | 8 +- .../cli/src/lib/install/install-errors.ts | 8 +- packages/cli/src/lib/output.test.ts | 3 + packages/cli/src/lib/output.ts | 8 +- packages/cli/src/lib/release-cli.ts | 32 +--- packages/cli/src/lib/run-command.ts | 13 +- 11 files changed, 160 insertions(+), 167 deletions(-) diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 069d95c..b8ac4fa 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -171,6 +171,7 @@ warning on stderr, and will be removed. | `-p, --path ` | Specific skill subfolder path | — | | `-s, --scan` | Scan entire repo for all SKILL.md files | `false` | | `--dry-run` | Preview what would be imported without calling API | `false` | +| `--json` | Envelope with the request (dry run) or the register response | `false` | **Resolution Logic:** - No flags → auto-detect (root SKILL.md or scan all) @@ -193,12 +194,10 @@ skillx import --dry-run ### Release commands: `validate`, `pack`, `publish`, `promote` -These commands work on immutable, digest-pinned releases. With `--json` they -print exactly one envelope on stdout, `{"ok": true, "data": {...}}` or -`{"ok": false, "error": {"code", "message", "details"?, "hint"?}}`, and the -exit code follows it: `0` ok, `1` registry or unexpected error, `2` usage -(invalid input or a package the registry would reject), `3` not found, `5` -credentials missing or rejected, `6` network. `skillx release pack` and +These commands work on immutable, digest-pinned releases. Like every other +command they take `--json` and follow +[JSON Output and Exit Codes](#json-output-and-exit-codes); `2` covers invalid +input and a package the registry would reject. `skillx release pack` and `skillx release publish` remain as aliases of `skillx pack` and `skillx publish `. @@ -562,20 +561,12 @@ the CLI's `--force`. ### Exit codes -Installer commands print a `{ok, data}` / `{ok, error}` envelope with `--json`. - -| Code | Error | Meaning | -|------|-------|---------| -| 0 | — | Success | -| 1 | `integrity_error`, `api_error` | Verification failed (nothing written), or the API failed | -| 2 | `usage_error` | Bad ref, flag or lock | -| 3 | `not_found` | Package, release, kit or collection not found | -| 4 | `incompatible` | The release refuses the runtime | -| 5 | `auth_error` | Credentials missing or rejected | -| 6 | `network_error` | Registry unreachable | -| 8 | `conflict` | Local edits, unowned directory, a link inside a skill, shared version, drift, stale plan, or a path in use | - -Code 7 is reserved for `rate_limited` in the shared CLI envelope. +Installer commands use the shared envelope and the one exit-code table in +[JSON Output and Exit Codes](#json-output-and-exit-codes). The codes they add +are `1` `integrity_error` (verification failed, nothing written), `4` +`incompatible` (the release refuses the runtime) and `8` `conflict` (local +edits, an unowned directory, a link inside a skill, a shared version, drift, a +stale plan, or a path in use). --- @@ -604,9 +595,10 @@ Code 7 is reserved for `rate_limited` in the shared CLI envelope. ## JSON Output and Exit Codes -Every command except `publish` and `release *` (which print their own JSON) accepts -`--json` and then writes exactly one envelope to stdout; spinners and prompts never reach -stdout in that mode. +Every command, the release and installer commands included, accepts `--json` and then +writes exactly one envelope to stdout; spinners and prompts never reach stdout in that mode. +The one exception is `skillx mcp`, which speaks MCP on stdio and has no single result to +wrap. ```json { "ok": true, "data": { "...": "command-specific payload" } } @@ -619,15 +611,16 @@ carries the same answer, so a script can branch without parsing either form: | Exit | `error.code` | Meaning | |------|--------------|---------| | `0` | — | Success (`check`: the target may be acted on) | -| `1` | `api_error`, `unexpected_error` | The API failed without a better class, or a bug | +| `1` | `api_error`, `unexpected_error`, `integrity_error` | The API failed without a better class, a bug, or a release failed verification (nothing written) | | `2` | `usage_error` | Invalid arguments or options, unknown command, missing argument | -| `3` | `not_found` | The skill or resource does not exist | -| `4` | — | `check` only: `ok: true` with `data.actionable: false`; the skill exists but the target may not act on it | +| `3` | `not_found` | The skill, package, release, kit or collection does not exist | +| `4` | `incompatible` | The target may not act on the skill. `check` answers it on a success envelope (`data.actionable: false`); the installer fails with `incompatible` | | `5` | `auth_error` | API key missing or rejected (`401`/`403`); the hint names `skillx config set-key` | | `6` | `network_error` | The API could not be reached | | `7` | `rate_limited` | The API answered `429`; retry later | +| `8` | `conflict` | Installer only: the change would overwrite something it does not own (see [Exit codes](#exit-codes)) | -Exit `4` is an answer, not a failure, so its envelope is a success. Only flags before `--` +For `check`, exit `4` is an answer, not a failure, so its envelope is a success. Only flags before `--` count: `skillx check -- --json` treats `--json` as an operand. Parse failures follow the same table: with `--json` on the command line, an unknown option diff --git a/packages/cli/src/built-cli.test.ts b/packages/cli/src/built-cli.test.ts index bc798d4..5b2dedb 100644 --- a/packages/cli/src/built-cli.test.ts +++ b/packages/cli/src/built-cli.test.ts @@ -257,6 +257,39 @@ describe("built cli", { timeout: 30_000 }, () => { } }); + /** Leaf commands from `--help`, walked recursively (`config set-key`, `release pack`, ...). */ + async function leafCommands(path: string[] = []): Promise { + const help = (await cli(...path, "--help")).stdout; + const section = help.split(/^Commands:$/m)[1]; + if (section === undefined) return [path]; + const names = [...section.matchAll(/^ (\S+)/gm)].map((m) => m[1]!).filter((name) => name !== "help"); + return (await Promise.all(names.map((name) => leafCommands([...path, name])))).flat(); + } + + it("offers --json on every command but the stdio MCP server", async () => { + const leaves = await leafCommands(); + expect(leaves.length).toBeGreaterThan(20); + const missing = []; + for (const leaf of leaves) { + if (leaf.join(" ") === "mcp") continue; // speaks MCP on stdio; there is no single result to wrap + if (!(await cli(...leaf, "--help")).stdout.includes("--json")) missing.push(leaf.join(" ")); + } + expect(missing).toEqual([]); + }); + + it.each([ + [["install", "notaref"], "usage_error", 2], + [["list", "--runtime", "zzz"], "usage_error", 2], + [["validate", "./nope"], "usage_error", 2], + [["pack", "./nope", "--out", "x.tar"], "usage_error", 2], + [["promote", "bad", "--to", "stable"], "usage_error", 2], + [["import", "not-a-repo"], "usage_error", 2], + ])("answers a failing `%s` with one %s envelope and exit %i", async (args, code, exit) => { + const result = await cliExit(...args, "--json"); + expect(result.code).toBe(exit); + expect(JSON.parse(result.stdout)).toMatchObject({ ok: false, error: { code } }); + }); + it("reports a parse error as usage_error with exit 2", async () => { const { code, stdout } = await cliExit("check", "some-skill", "--json"); expect(code).toBe(2); diff --git a/packages/cli/src/commands/import.ts b/packages/cli/src/commands/import.ts index e7337f2..c76dedb 100644 --- a/packages/cli/src/commands/import.ts +++ b/packages/cli/src/commands/import.ts @@ -1,8 +1,9 @@ import { Command } from 'commander'; import { execSync } from 'node:child_process'; import chalk from 'chalk'; -import ora from 'ora'; import { apiRequest, ApiError } from '../lib/api-client.js'; +import { CliError } from '../lib/output.js'; +import { runCommand } from '../lib/run-command.js'; import { getApiKey } from '../utils/config-store.js'; interface RegisterResponse { @@ -54,101 +55,73 @@ export interface ImportOptions { path?: string; scan?: boolean; dryRun?: boolean; + json?: boolean; } -/** Registers the skills of a GitHub repository with the marketplace (POST /api/skills/register). */ -export async function runImport(repoArg: string | undefined, options: ImportOptions): Promise { - try { - // Require API key - const apiKey = getApiKey(); - if (!apiKey) { - console.error(chalk.red('\nAPI key required to publish skills.')); - console.error(chalk.dim('Set your API key with: skillx config set api-key ')); - console.error(chalk.dim('Get an API key at: https://skillx.sh/settings')); - process.exit(1); - } - - // Resolve owner/repo - let remote: { owner: string; repo: string } | null = null; - - if (repoArg) { - remote = parseRepoArg(repoArg); - if (!remote) { - console.error(chalk.red(`\nInvalid repo format: "${repoArg}". Expected: owner/repo`)); - process.exit(1); - } - } else { - remote = detectGitRemote(); - if (!remote) { - console.error(chalk.red('\nCould not detect GitHub repo from git remote.')); - console.error(chalk.dim('Specify explicitly: skillx import owner/repo')); - process.exit(1); - } - console.log(chalk.dim(`Detected repo: ${remote.owner}/${remote.repo}`)); - } - - // Build request body - const body: Record = { - owner: remote.owner, - repo: remote.repo, - }; - - if (options.path) { - body.skill_path = options.path; - } else if (options.scan) { - body.scan = true; - } - - // Dry run — just show what would happen - if (options.dryRun) { - console.log(chalk.bold('\nDry run — would publish:')); - console.log(` Repository: ${chalk.cyan(`${remote.owner}/${remote.repo}`)}`); - if (options.path) { - console.log(` Skill path: ${chalk.cyan(options.path)}`); - } else if (options.scan) { - console.log(` Mode: ${chalk.cyan('Scan all SKILL.md files')}`); - } else { - console.log(` Mode: ${chalk.cyan('Auto-detect (root skill or scan)')}`); - } - console.log(chalk.dim('\nRemove --dry-run to publish.')); - return; - } - - // Call register API - const spinner = ora('Publishing to SkillX...').start(); - - try { - const res = await apiRequest('/api/skills/register', { - method: 'POST', - body: JSON.stringify(body), - }); - spinner.stop(); - displayResult(res, remote.owner, remote.repo); - } catch (err) { - spinner.stop(); - throw err; - } - } catch (error) { - if (error instanceof ApiError) { - if (error.status === 401) { - console.error(chalk.red('\nAuthentication failed. Check your API key.')); - console.error(chalk.dim('Update with: skillx config set api-key ')); - } else if (error.status === 403) { - console.error(chalk.red('\nPermission denied. You must have access to this GitHub repo.')); - console.error(chalk.dim('Make sure your GitHub account is a collaborator or owner of the repo.')); - } else if (error.status === 404) { - console.error(chalk.red('\nNo SKILL.md found in the repository.')); - console.error(chalk.dim('Create a SKILL.md file in your repo and try again.')); - } else { - console.error(chalk.red(`\nAPI Error: ${error.message}`)); - } - } else if (error instanceof Error) { - console.error(chalk.red(`\nError: ${error.message}`)); - } else { - console.error(chalk.red('\nAn unexpected error occurred')); - } - process.exit(1); +type ImportResult = + | { dryRun: true; owner: string; repo: string; request: Record } + | { dryRun: false; owner: string; repo: string; detected: boolean; response: RegisterResponse }; + +/** Resolves the repository and registers it, or describes the request under --dry-run. */ +async function importRepo(repoArg: string | undefined, options: ImportOptions): Promise { + const remote = repoArg ? parseRepoArg(repoArg) : detectGitRemote(); + if (!remote) { + throw repoArg + ? new CliError('usage_error', `Invalid repo format: "${repoArg}". Expected: owner/repo`) + : new CliError('usage_error', 'Could not detect GitHub repo from git remote.', { hint: 'Specify explicitly: skillx import owner/repo' }); + } + + // Arguments are checked before credentials, so a bad call is a usage error either way. + if (!getApiKey()) { + throw new CliError('auth_error', 'API key required to import skills.', { + hint: 'Run `skillx config set-key` or set SKILLX_API_KEY. Get a key at https://skillx.sh/settings', + }); + } + + const request: Record = { owner: remote.owner, repo: remote.repo }; + if (options.path) request.skill_path = options.path; + else if (options.scan) request.scan = true; + if (options.dryRun) return { dryRun: true, ...remote, request }; + + try { + const response = await apiRequest('/api/skills/register', { method: 'POST', body: JSON.stringify(request) }); + return { dryRun: false, ...remote, detected: !repoArg, response }; + } catch (error) { + if (error instanceof ApiError && error.status === 403) { + throw new CliError('auth_error', 'Permission denied. You must have access to this GitHub repo.', { + details: { status: 403 }, + hint: 'Make sure your GitHub account is a collaborator or owner of the repo.', + }); } + throw error; + } +} + +function renderImport(result: ImportResult, options: ImportOptions): void { + if (!result.dryRun) { + if (result.detected) console.log(chalk.dim(`Detected repo: ${result.owner}/${result.repo}`)); + displayResult(result.response, result.owner, result.repo); + return; + } + console.log(chalk.bold('\nDry run — would publish:')); + console.log(` Repository: ${chalk.cyan(`${result.owner}/${result.repo}`)}`); + if (options.path) console.log(` Skill path: ${chalk.cyan(options.path)}`); + else if (options.scan) console.log(` Mode: ${chalk.cyan('Scan all SKILL.md files')}`); + else console.log(` Mode: ${chalk.cyan('Auto-detect (root skill or scan)')}`); + console.log(chalk.dim('\nRemove --dry-run to publish.')); +} + +/** Registers the skills of a GitHub repository with the marketplace (POST /api/skills/register). */ +export function runImport(repoArg: string | undefined, options: ImportOptions): Promise { + return runCommand( + { + json: options.json === true, + ...(options.dryRun ? {} : { spinner: 'Publishing to SkillX...' }), + errors: { notFoundMessage: 'No SKILL.md found in the repository.' }, + }, + () => importRepo(repoArg, options), + (result) => renderImport(result, options), + ); } export const importCommand = new Command('import') @@ -157,6 +130,7 @@ export const importCommand = new Command('import') .option('-p, --path ', 'Specific skill subfolder path') .option('-s, --scan', 'Scan entire repo for all SKILL.md files') .option('--dry-run', 'Show what would be imported without calling API') + .option('--json', 'emit a machine-readable envelope instead of text') .action(runImport); function displayResult(res: RegisterResponse, owner: string, repo: string): void { diff --git a/packages/cli/src/commands/install-runner.ts b/packages/cli/src/commands/install-runner.ts index 7460108..ee589cd 100644 --- a/packages/cli/src/commands/install-runner.ts +++ b/packages/cli/src/commands/install-runner.ts @@ -1,11 +1,10 @@ import chalk from 'chalk'; import { createInstallContext } from '../lib/install/install-context.js'; import type { InstallContext } from '../lib/install/install-context.js'; -import { InstallError, usageError } from '../lib/install/install-errors.js'; +import { usageError } from '../lib/install/install-errors.js'; import type { SourcePlan } from '../lib/install/install-operations.js'; import { createRegistry } from '../lib/install/registry-client.js'; -import { exitCodeFor, failure, fromError, serialize, success } from '../lib/output.js'; -import type { CliFailure } from '../lib/output.js'; +import { exitCodeFor, fromError, serialize, success } from '../lib/output.js'; import { getApiKey, getBaseUrl } from '../utils/config-store.js'; /** Options every installer command accepts. */ @@ -30,11 +29,6 @@ export function contextFrom(options: InstallerOptions): InstallContext { }); } -function toFailure(error: unknown): CliFailure { - if (error instanceof InstallError) return failure(error.code, error.message, error.details === undefined ? {} : { details: error.details }); - return fromError(error); -} - /** Runs one command body: JSON envelope or human text, and the exit code of the outcome. */ export async function runInstaller(options: InstallerOptions, body: () => Promise, human: (data: T) => void): Promise { try { @@ -43,7 +37,8 @@ export async function runInstaller(options: InstallerOptions, body: () => Pro else human(data); process.exit(0); } catch (error) { - const envelope = toFailure(error); + // InstallError is a CliError, so its code and details pass through unchanged. + const envelope = fromError(error); if (options.json) console.log(serialize(envelope)); else { console.error(chalk.red(`\n✗ ${envelope.error.message}`)); diff --git a/packages/cli/src/commands/publish.ts b/packages/cli/src/commands/publish.ts index 2355b4b..f873f38 100644 --- a/packages/cli/src/commands/publish.ts +++ b/packages/cli/src/commands/publish.ts @@ -39,7 +39,7 @@ export const publishCommand = withReleasePublishOptions( ).action(async (source: string | undefined, options: PublishOptions) => { if (publishMode(source, options) === 'import') { console.error(chalk.yellow(IMPORT_DEPRECATION)); - await runImport(source, { path: options.path, scan: options.scan, dryRun: options.dryRun }); + await runImport(source, { path: options.path, scan: options.scan, dryRun: options.dryRun, json: options.json }); return; } await runCommand('publish', options.json, () => runReleasePublish(source ?? '.', options), renderPublished); diff --git a/packages/cli/src/commands/release-commands.test.ts b/packages/cli/src/commands/release-commands.test.ts index 5ef3a31..58edbf4 100644 --- a/packages/cli/src/commands/release-commands.test.ts +++ b/packages/cli/src/commands/release-commands.test.ts @@ -40,8 +40,10 @@ describe('promote', () => { expect((init.headers as Record).Authorization).toBe('Bearer sx_key'); fetchMock.mockResolvedValueOnce(json({ error: { code: 'promote.demotion_forbidden', message: 'no' } }, 403)); - await expect(runPromote('@alpha/demo@1.0.0', { to: 'beta', registry: 'https://reg.test' })).rejects.toMatchObject({ failure: { error: { code: 'auth_error' } } }); - await expect(runPromote('@alpha/demo@1.0.0', { to: 'nightly', registry: 'https://reg.test' })).rejects.toMatchObject({ failure: { error: { code: 'usage_error' } } }); + await expect(runPromote('@alpha/demo@1.0.0', { to: 'beta', registry: 'https://reg.test' })).rejects.toMatchObject({ code: 'auth_error' }); + await expect(runPromote('@alpha/demo@1.0.0', { to: 'nightly', registry: 'https://reg.test' })).rejects.toMatchObject({ code: 'usage_error' }); + fetchMock.mockResolvedValueOnce(json({ error: { code: 'rate_limited', message: 'slow down' } }, 429)); + await expect(runPromote('@alpha/demo@1.0.0', { to: 'beta', registry: 'https://reg.test' })).rejects.toMatchObject({ code: 'rate_limited' }); }); }); @@ -59,6 +61,6 @@ describe('OIDC credential', () => { it('fails as auth_error outside GitHub Actions', async () => { vi.stubEnv('ACTIONS_ID_TOKEN_REQUEST_URL', ''); - await expect(publishCredential('https://reg.test', 'a', { oidc: true })).rejects.toMatchObject({ failure: { error: { code: 'auth_error' } } }); + await expect(publishCredential('https://reg.test', 'a', { oidc: true })).rejects.toMatchObject({ code: 'auth_error' }); }); }); diff --git a/packages/cli/src/lib/install/install-errors.ts b/packages/cli/src/lib/install/install-errors.ts index f4dad61..5f6cce9 100644 --- a/packages/cli/src/lib/install/install-errors.ts +++ b/packages/cli/src/lib/install/install-errors.ts @@ -1,17 +1,17 @@ -import type { CliErrorCode } from '../output.js'; +import { CliError, type CliErrorCode } from '../output.js'; /** * A classified installer failure. `code` is an envelope error code, so a * command maps it to its exit code without parsing the message; `details` * is machine-readable context (never file contents or credentials). */ -export class InstallError extends Error { +export class InstallError extends CliError { constructor( - readonly code: CliErrorCode, + code: CliErrorCode, message: string, readonly details?: unknown, ) { - super(message); + super(code, message, details === undefined ? {} : { details }); this.name = 'InstallError'; } } diff --git a/packages/cli/src/lib/output.test.ts b/packages/cli/src/lib/output.test.ts index 6238d48..6d0cf7f 100644 --- a/packages/cli/src/lib/output.test.ts +++ b/packages/cli/src/lib/output.test.ts @@ -32,6 +32,9 @@ describe("exit codes", () => { it("is 2 for a usage error and 3 for a missing skill", () => { expect(exitCodeFor(failure("usage_error", "no"))).toBe(EXIT_CODES.usage); expect(exitCodeFor(failure("not_found", "no"))).toBe(EXIT_CODES.notFound); + expect(exitCodeFor(failure("incompatible", "no"))).toBe(EXIT_CODES.incompatible); + expect(exitCodeFor(failure("integrity_error", "no"))).toBe(EXIT_CODES.error); + expect(exitCodeFor(failure("conflict", "no"))).toBe(EXIT_CODES.conflict); }); }); diff --git a/packages/cli/src/lib/output.ts b/packages/cli/src/lib/output.ts index 7bdca9a..a89807c 100644 --- a/packages/cli/src/lib/output.ts +++ b/packages/cli/src/lib/output.ts @@ -19,7 +19,11 @@ export const EXIT_CODES = { usage: 2, /** The skill does not exist. */ notFound: 3, - /** `check` only: the skill exists but the target may not act on it. A success envelope, not an error code. */ + /** + * The skill exists but the target may not act on it. `check` answers it on a + * success envelope (`data.actionable: false`); the installer fails with the + * `incompatible` error code. + */ incompatible: 4, /** Credentials are missing or rejected. */ auth: 5, @@ -36,6 +40,7 @@ export type ExitCode = (typeof EXIT_CODES)[keyof typeof EXIT_CODES]; export type CliErrorCode = | 'usage_error' | 'not_found' + | 'incompatible' | 'auth_error' | 'network_error' | 'rate_limited' @@ -47,6 +52,7 @@ export type CliErrorCode = const EXIT_BY_CODE: Record = { usage_error: EXIT_CODES.usage, not_found: EXIT_CODES.notFound, + incompatible: EXIT_CODES.incompatible, auth_error: EXIT_CODES.auth, network_error: EXIT_CODES.network, rate_limited: EXIT_CODES.rateLimited, diff --git a/packages/cli/src/lib/release-cli.ts b/packages/cli/src/lib/release-cli.ts index 8c6a460..2623b17 100644 --- a/packages/cli/src/lib/release-cli.ts +++ b/packages/cli/src/lib/release-cli.ts @@ -4,39 +4,22 @@ * whose failures map onto that vocabulary, and the publish credential (an API * key, or a GitHub Actions OIDC token exchanged for a short-lived one). */ -import chalk from 'chalk'; -import { exitCodeFor, failure, fromError, serialize, success } from './output.js'; -import type { CliErrorCode, CliFailure } from './output.js'; +import { CliError } from './output.js'; +import { runCommand as runShared } from './run-command.js'; import { getApiKey } from '../utils/config-store.js'; -/** A failure that already knows its envelope code. */ -export class CliError extends Error { - readonly failure: CliFailure; - - constructor(code: CliErrorCode, message: string, extra: { details?: unknown; hint?: string } = {}) { - super(message); - this.failure = failure(code, message, extra); - } -} +export { CliError }; export const usageError = (message: string) => new CliError('usage_error', message); /** - * Runs one command. With `--json` stdout carries exactly one envelope, success - * or failure; without it `render` prints for humans and errors go to stderr. + * Runs one release command through the shared `runCommand`: with `--json` + * stdout carries exactly one envelope, success or failure; without it `render` + * prints for humans and errors go to stderr, prefixed with the command label. * The exit code follows the envelope either way. */ export async function runCommand(label: string, json: boolean | undefined, work: () => Promise, render: (data: T) => void): Promise { - try { - const data = await work(); - if (json) console.log(serialize(success(data))); - else render(data); - } catch (error) { - const envelope = error instanceof CliError ? error.failure : fromError(error); - if (json) console.log(serialize(envelope)); - else console.error(chalk.red(`${label} failed: ${envelope.error.message}`)); - process.exitCode = exitCodeFor(envelope); - } + await runShared({ json: json === true, failureLabel: label }, work, render); } const NAME_PATTERN = /^@([a-z0-9][a-z0-9._-]*)\/([a-z0-9][a-z0-9._-]*)$/; @@ -70,6 +53,7 @@ export async function registryFetch(url: string, init: RequestInit): Promise { exitCode?: (data: T) => ExitCode; /** Wording for errors whose subject varies by command. */ errors?: ErrorContext; + /** Human failures read `