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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX
This commit is contained in:
parent
066676a241
commit
a5d8d80ffc
2 changed files with 420 additions and 42 deletions
|
|
@ -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");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Reference in a new issue