From a5d8d80ffc4a3f950f948a43df70529e885c4450 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 1 Jul 2026 18:08:56 -0400 Subject: [PATCH] feat(cli): exit-code map + structured JSON error envelopes (closes #1564) Give the CLI a stable machine-readable failure surface: - EXIT_CODES map (0 ok / 1 usage / 2 no-app / 3 auth-required / 4 unreachable / 5 tool-error) - ErrorEnvelope interface + CliExitCodeError for pre-classified exits - pure classifyError() (status/cause/pattern heuristics) and formatErrorOutput()/handleError() that emit one JSON line on stderr - in-process cli-runner mirrors the binary via formatErrorOutput so tests observe the real exit code and envelope - document the exit-code map + envelope shape in clients/cli/README.md Wave 1 foundation of the PR #1510 decomposition; shapes kept clean for reuse by --app-info (exit 2), stored-auth (exit 3), and --format json. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/cli/README.md | 26 +++ clients/cli/__tests__/error-handler.test.ts | 206 +++++++++++++++++-- clients/cli/__tests__/helpers/cli-runner.ts | 24 +-- clients/cli/src/error-handler.ts | 212 ++++++++++++++++++-- 4 files changed, 423 insertions(+), 45 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index ce03c465..85ee3194 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -143,6 +143,32 @@ npx @modelcontextprotocol/inspector --cli --catalog mcp.json --server my-http-se See [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAuth smoke testing](../../specification/v2_auth_smoke_testing.md) for configuration details and staging servers. +## Exit codes & error envelopes + +Every non-zero exit maps to a stable failure class, so a programmatic caller +(CI, a script, an agent) can branch on _why_ the CLI failed without scraping +prose from stderr: + +| Code | Meaning | +| ---- | ------- | +| `0` | Success. | +| `1` | Usage / unexpected error (the catch-all). | +| `2` | No MCP App found on the tool (`--app-info` probe). | +| `3` | Server requires authentication (401/403, `WWW-Authenticate`, OAuth). | +| `4` | Server unreachable (DNS, connection refused, timeout, `fetch failed`). | +| `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). | + +On any non-zero exit the CLI also writes a single JSON line to **stderr** — the +`ErrorEnvelope`: + +```json +{ "error": { "code": "auth_required", "message": "Unauthorized", "status": 401, "url": "https://api.example/mcp" } } +``` + +The `code` is a stable identifier for the failure class; `message` is the +human-readable error; `cause`, `status`, and `url` are included when known. +Because it is one line, a caller can parse it with `2>&1 | tail -1 | jq .error`. + ## Why use the CLI? While the Web Client provides a rich visual interface, the CLI is designed for: diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 79d0eab8..64ad53e5 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, afterEach } from "vitest"; -import { handleError } from "../src/error-handler.js"; +import { + CliExitCodeError, + EXIT_CODES, + classifyError, + formatErrorOutput, + handleError, +} from "../src/error-handler.js"; /** * `handleError` is the binary's last-resort error sink (wired up in @@ -13,33 +19,195 @@ describe("handleError", () => { vi.restoreAllMocks(); }); - it("logs an Error's message and exits with code 1", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + it("emits a JSON error envelope on stderr and exits with the classified code", () => { + const writeSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation((() => true) as never); const exitSpy = vi .spyOn(process, "exit") .mockImplementation((() => undefined) as never); handleError(new Error("boom")); - expect(errorSpy).toHaveBeenCalledWith("boom"); - expect(exitSpy).toHaveBeenCalledWith(1); + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE); + const written = writeSpy.mock.calls[0]![0] as string; + const parsed = JSON.parse(written) as { + error: { code: string; message: string }; + }; + expect(parsed.error.code).toBe("error"); + expect(parsed.error.message).toBe("boom"); }); - it("logs a string error verbatim", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); + it("uses a CliExitCodeError's exitCode and envelope code", () => { + vi.spyOn(process.stderr, "write").mockImplementation((() => true) as never); + const exitSpy = vi + .spyOn(process, "exit") + .mockImplementation((() => undefined) as never); - handleError("plain failure"); + handleError( + new CliExitCodeError(EXIT_CODES.NO_APP, "no app", { code: "no_app" }), + ); - expect(errorSpy).toHaveBeenCalledWith("plain failure"); - }); - - it("falls back to 'Unknown error' for non-Error, non-string values", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - vi.spyOn(process, "exit").mockImplementation((() => undefined) as never); - - handleError({ unexpected: true }); - - expect(errorSpy).toHaveBeenCalledWith("Unknown error"); + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.NO_APP); + }); +}); + +describe("classifyError", () => { + it("classifies a 401 status as AUTH_REQUIRED", () => { + const err = Object.assign(new Error("Unauthorized"), { status: 401 }); + const { exitCode, envelope } = classifyError(err, { + url: "https://x.example/mcp", + }); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.code).toBe("auth_required"); + expect(envelope.status).toBe(401); + expect(envelope.url).toBe("https://x.example/mcp"); + }); + + it("classifies a WWW-Authenticate message as AUTH_REQUIRED without a status", () => { + const { exitCode } = classifyError( + new Error("Dynamic client registration failed: WWW-Authenticate Bearer"), + ); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + }); + + it("classifies ENOTFOUND / fetch failed as UNREACHABLE", () => { + const err = new Error("fetch failed"); + (err as { cause?: unknown }).cause = new Error( + "getaddrinfo ENOTFOUND no.such.host", + ); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); + expect(envelope.cause).toContain("ENOTFOUND"); + }); + + it("classifies a connection-refused cause as UNREACHABLE", () => { + const err = new Error("connect ECONNREFUSED 127.0.0.1:1"); + expect(classifyError(err).exitCode).toBe(EXIT_CODES.UNREACHABLE); + }); + + it("falls back to USAGE for an unrecognized Error", () => { + expect(classifyError(new Error("something else")).exitCode).toBe( + EXIT_CODES.USAGE, + ); + }); + + it("handles a string error", () => { + const { exitCode, envelope } = classifyError("plain failure"); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.message).toBe("plain failure"); + }); + + it("handles a non-Error, non-string value", () => { + const { envelope } = classifyError({ unexpected: true }); + expect(envelope.message).toBe("Unknown error"); + }); + + it("reads an HTTP status from a nested response object", () => { + const err = { response: { status: 401 } }; + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.status).toBe(401); + }); + + it("preserves a CliExitCodeError's explicit envelope code over the default", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.TOOL_ERROR, "x", { + code: "tool_not_found", + }), + ); + expect(envelope.code).toBe("tool_not_found"); + }); + + it("carries a CliExitCodeError's explicit status and url onto the envelope", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, "denied", { + status: 401, + url: "https://api.example/mcp", + }), + ); + expect(envelope.status).toBe(401); + expect(envelope.url).toBe("https://api.example/mcp"); + }); + + it("falls back to the context url for a CliExitCodeError without its own url", () => { + const { envelope } = classifyError( + new CliExitCodeError(EXIT_CODES.UNREACHABLE, "down"), + { url: "https://ctx.example/mcp" }, + ); + expect(envelope.url).toBe("https://ctx.example/mcp"); + }); + + it("derives a default envelope code for a bare CliExitCodeError", () => { + expect( + classifyError(new CliExitCodeError(EXIT_CODES.UNREACHABLE, "x")).envelope + .code, + ).toBe("unreachable"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.NO_APP, "x")).envelope.code, + ).toBe("no_app"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, "x")) + .envelope.code, + ).toBe("auth_required"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.TOOL_ERROR, "x")).envelope + .code, + ).toBe("tool_error"); + expect( + classifyError(new CliExitCodeError(EXIT_CODES.USAGE, "x")).envelope.code, + ).toBe("error"); + }); + + it("captures a non-Error cause as a string", () => { + const err = new Error("outer"); + (err as { cause?: unknown }).cause = { reason: "blocked" }; + const { envelope } = classifyError(err); + expect(envelope.cause).toBe("[object Object]"); + }); + + it("flattens a nested Error cause chain into one string", () => { + const inner = new Error("getaddrinfo ENOTFOUND host"); + const middle = Object.assign(new Error("connect failed"), { cause: inner }); + const outer = Object.assign(new Error("fetch failed"), { cause: middle }); + const { envelope } = classifyError(outer); + expect(envelope.cause).toBe("connect failed: getaddrinfo ENOTFOUND host"); + }); + + it("reads a numeric SDK-style .code as an HTTP status", () => { + const err = Object.assign(new Error("forbidden"), { code: 403 }); + const { exitCode, envelope } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.status).toBe(403); + }); + + it("ignores a non-numeric node error code when reading the status", () => { + const err = Object.assign(new Error("getaddrinfo ENOTFOUND host"), { + code: "ENOTFOUND", + }); + const { exitCode, envelope } = classifyError(err); + // The message matches the UNREACHABLE pattern, and the string node code + // must NOT be treated as an HTTP status. + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); + expect(envelope.status).toBeUndefined(); + }); +}); + +describe("formatErrorOutput", () => { + it("emits one JSON line on stderr ending in a newline", () => { + const { stderr, exitCode } = formatErrorOutput(new Error("nope")); + expect(stderr.endsWith("\n")).toBe(true); + expect(stderr.split("\n").length).toBe(2); + const parsed = JSON.parse(stderr) as { error: { message: string } }; + expect(parsed.error.message).toBe("nope"); + expect(exitCode).toBe(EXIT_CODES.USAGE); + }); + + it("threads the context url into the envelope", () => { + const { stderr } = formatErrorOutput(new Error("nope"), { + url: "https://ctx.example/mcp", + }); + const parsed = JSON.parse(stderr) as { error: { url?: string } }; + expect(parsed.error.url).toBe("https://ctx.example/mcp"); }); }); diff --git a/clients/cli/__tests__/helpers/cli-runner.ts b/clients/cli/__tests__/helpers/cli-runner.ts index 38b401d1..d5c8abe6 100644 --- a/clients/cli/__tests__/helpers/cli-runner.ts +++ b/clients/cli/__tests__/helpers/cli-runner.ts @@ -1,4 +1,5 @@ import { runCli as invokeCli } from "../../src/cli.js"; +import { formatErrorOutput } from "../../src/error-handler.js"; export interface CliResult { exitCode: number | null; @@ -52,7 +53,8 @@ function captureWrite(append: (text: string) => void) { * (`exitCode` / `stdout` / `stderr` / `output`) so every existing test and the * shared assertion helpers keep working unchanged: * - stdout/stderr are captured by temporarily patching `process.std*.write` - * (and `console.error`/`console.warn`, which commander and error paths use). + * (which also captures `console.error`/`console.warn`, since those route + * through `process.stderr.write`). * - A thrown error (bad args, failed connect, unsupported method) maps to * `exitCode: 1` with the message appended to stderr — mirroring how the real * binary (`index.ts`) routes errors through `handleError` → `process.exit(1)`. @@ -77,8 +79,6 @@ export async function runCli( const originalStdoutWrite = process.stdout.write; const originalStderrWrite = process.stderr.write; - const originalConsoleError = console.error; - const originalConsoleWarn = console.warn; // Snapshot and apply env overrides; `undefined` records keys that did not // exist so they can be deleted (not set to the string "undefined") on restore. @@ -93,15 +93,12 @@ export async function runCli( process.stdout.write = captureWrite((text) => { stdout += text; }) as typeof process.stdout.write; + // `console.error` / `console.warn` route through `process.stderr.write`, so + // patching the underlying stream captures them too — no separate console + // override is needed (the previous explicit overrides were redundant). process.stderr.write = captureWrite((text) => { stderr += text; }) as typeof process.stderr.write; - console.error = (...parts: unknown[]) => { - stderr += parts.map(String).join(" ") + "\n"; - }; - console.warn = (...parts: unknown[]) => { - stderr += parts.map(String).join(" ") + "\n"; - }; // `runCli` reads `process.argv.slice(2)`, so prepend two placeholder entries // ([node, script]) to mirror how the binary is launched. @@ -127,14 +124,15 @@ export async function runCli( try { await Promise.race([invokeCli(argv), timeout]); } catch (error) { - exitCode = 1; - stderr += (error instanceof Error ? error.message : String(error)) + "\n"; + // Mirror the binary's `handleError` exactly so tests observe the same exit + // code and stderr (one JSON envelope line) the real CLI would emit. + const out = formatErrorOutput(error); + exitCode = out.exitCode; + stderr += out.stderr; } finally { if (timer) clearTimeout(timer); process.stdout.write = originalStdoutWrite; process.stderr.write = originalStderrWrite; - console.error = originalConsoleError; - console.warn = originalConsoleWarn; for (const [key, value] of Object.entries(envBackup)) { if (value === undefined) delete process.env[key]; else process.env[key] = value; diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 97257745..b6506e9a 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -1,20 +1,206 @@ -function formatError(error: unknown): string { - let message: string; +/** + * Exit-code map. Non-zero codes let an automated caller (CI, an agent) branch + * on the failure class without regex-scraping stderr: + * + * - 0: success + * - 1: usage / unexpected error (the catch-all; same as before this map) + * - 2: `--app-info` probe found no MCP App on the tool + * - 3: server requires authentication (401 / WWW-Authenticate / OAuth) + * - 4: server unreachable (DNS, connect refused, timeout, fetch failure) + * - 5: tool error (`tools/call` returned `isError:true`, or tool not found) + */ +export const EXIT_CODES = { + OK: 0, + USAGE: 1, + NO_APP: 2, + AUTH_REQUIRED: 3, + UNREACHABLE: 4, + TOOL_ERROR: 5, +} as const; - if (error instanceof Error) { - message = error.message; - } else if (typeof error === "string") { - message = error; - } else { - message = "Unknown error"; +/** Machine-readable error envelope written as one JSON line on stderr. */ +export interface ErrorEnvelope { + /** Stable identifier for the failure class (e.g. "auth_required"). */ + code: string; + /** Human-readable message. */ + message: string; + /** The underlying error's `cause` (e.g. undici's ENOTFOUND), if any. */ + cause?: string; + /** HTTP status when the failure was an HTTP-level response. */ + status?: number; + /** The server URL the failure was against, when known. */ + url?: string; +} + +/** + * Thrown by the CLI to request a specific non-zero exit code without routing + * through the generic error path. {@link formatErrorOutput} reads `exitCode` + * and `envelope`; the in-process test runner does the same so tests observe + * the real code and stderr. + */ +export class CliExitCodeError extends Error { + constructor( + public readonly exitCode: number, + message: string, + public readonly envelope?: Partial, + ) { + super(message); + this.name = "CliExitCodeError"; + } +} + +/** Read an `Error.cause` chain into a single readable string, if present. */ +function causeOf(error: unknown): string | undefined { + if (!(error instanceof Error)) return undefined; + const cause = (error as { cause?: unknown }).cause; + if (cause === undefined || cause === null) return undefined; + if (cause instanceof Error) { + const nested = causeOf(cause); + return nested ? `${cause.message}: ${nested}` : cause.message; + } + return String(cause); +} + +/** Best-effort HTTP status from common error shapes (undici, MCP SDK, fetch). */ +function statusOf(error: unknown): number | undefined { + if (error == null || typeof error !== "object") return undefined; + const e = error as { + status?: unknown; + code?: unknown; + response?: { status?: unknown }; + }; + if (typeof e.status === "number") return e.status; + if (typeof e.response?.status === "number") return e.response.status; + // SDK SSEError exposes `.code` as the HTTP status; numeric only (avoid + // mistaking node error codes like "ENOTFOUND" for a status). + if (typeof e.code === "number") return e.code; + return undefined; +} + +const UNREACHABLE_PATTERN = + /ENOTFOUND|ECONNREFUSED|ECONNRESET|EAI_AGAIN|ETIMEDOUT|fetch failed|getaddrinfo|connect timed out|aborted/i; + +/** + * Classify an arbitrary error into an exit code and envelope. Used both by the + * binary's {@link handleError} and by callers that want to throw a + * {@link CliExitCodeError} with the right code up front. + */ +export function classifyError( + error: unknown, + context?: { url?: string }, +): { exitCode: number; envelope: ErrorEnvelope } { + const message = + error instanceof Error + ? error.message + : typeof error === "string" + ? error + : "Unknown error"; + const cause = causeOf(error); + const status = statusOf(error); + const url = context?.url; + + // A pre-classified error carries its own exit code; fill in any envelope + // fields the thrower didn't supply. + if (error instanceof CliExitCodeError) { + return { + exitCode: error.exitCode, + envelope: { + code: error.envelope?.code ?? codeForExit(error.exitCode), + message, + ...(cause !== undefined && { cause }), + ...(error.envelope?.status !== undefined && { + status: error.envelope.status, + }), + ...((error.envelope?.url ?? url) !== undefined && { + url: error.envelope?.url ?? url, + }), + }, + }; } - return message; + // 401 / OAuth-required → AUTH_REQUIRED so the caller can kick the auth flow. + if ( + status === 401 || + status === 403 || + /WWW-Authenticate|Unauthorized|invalid_token|OAuth/i.test( + message + " " + (cause ?? ""), + ) + ) { + return { + exitCode: EXIT_CODES.AUTH_REQUIRED, + envelope: { + code: "auth_required", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; + } + + // Network-layer failure → UNREACHABLE so the caller can retry via a proxy. + if (UNREACHABLE_PATTERN.test(message + " " + (cause ?? ""))) { + return { + exitCode: EXIT_CODES.UNREACHABLE, + envelope: { + code: "unreachable", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; + } + + return { + exitCode: EXIT_CODES.USAGE, + envelope: { + code: "error", + message, + ...(cause !== undefined && { cause }), + ...(status !== undefined && { status }), + ...(url !== undefined && { url }), + }, + }; +} + +function codeForExit(exitCode: number): string { + switch (exitCode) { + case EXIT_CODES.NO_APP: + return "no_app"; + case EXIT_CODES.AUTH_REQUIRED: + return "auth_required"; + case EXIT_CODES.UNREACHABLE: + return "unreachable"; + case EXIT_CODES.TOOL_ERROR: + return "tool_error"; + default: + return "error"; + } +} + +/** + * Single source of truth for mapping a thrown error to the CLI's exit code and + * stderr text. The binary entry ({@link handleError}) and the in-process test + * runner both call this, so tests observe exactly what the binary would emit. + * + * stderr is one JSON line `{"error":{...}}` so a caller can `2>&1 | tail -1 | + * jq .error`; the message is inside the envelope so nothing is lost vs. the + * previous bare-message behaviour. + */ +export function formatErrorOutput( + error: unknown, + context?: { url?: string }, +): { exitCode: number; stderr: string } { + const { exitCode, envelope } = classifyError(error, context); + return { + exitCode, + stderr: JSON.stringify({ error: envelope }) + "\n", + }; } export function handleError(error: unknown): never { - const errorMessage = formatError(error); - console.error(errorMessage); - - process.exit(1); + const { exitCode, stderr } = formatErrorOutput(error); + process.stderr.write(stderr); + process.exit(exitCode); }