diff --git a/.changeset/json-error-envelope-for-transport-failures.md b/.changeset/json-error-envelope-for-transport-failures.md new file mode 100644 index 0000000..b0420b1 --- /dev/null +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -0,0 +1,45 @@ +--- +"@call-e/cli": patch +--- + +Always emit the documented JSON error envelope, and surface upstream error detail safely. + +`runCli` previously rethrew every error that was not an `InvalidArgumentsError`, so any +transport or upstream HTTP failure escaped to `main()` and printed a bare message to stderr +with nothing on stdout. Agent hosts are instructed to treat all command output as JSON, so a +failed `auth login` left them with an empty stdout and no `error.code` to branch on. + +Every failure now leaves through `writeCommandError`. `error.code` is always CLI-owned: +`broker_unavailable` when the brokered-login service returns a 5xx, `http_error` for other +non-success statuses, and `transport_error` when `fetch` rejects or times out before a +response arrives. Upstream detail is exposed only under `error.remote_error` after passing +through the same sanitizer used for MCP call errors: only `code` and `message` are read +(top-level or nested under `error`), all other fields are dropped unread, codes are +constrained to `[A-Za-z0-9_.:-]` and 64 characters, messages are capped at 500 characters, +and ANSI/C0/C1 terminal control sequences are stripped before anything reaches stdout or +stderr. An upstream body cannot set the top-level code, so it cannot impersonate stable +local codes such as `auth_required`. + +Before, against a broker returning 502: + +```text +Client error '502 Bad Gateway' for url '.../api/v1/openagent-auth/sessions' +``` + +After: + +```json +{ + "ok": false, + "error": { + "code": "broker_unavailable", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client. err_type=HTTPStatusError" + } + } +} +``` + +The CLI reference and README now document the error envelope and its stable fields. diff --git a/packages/cli/README.md b/packages/cli/README.md index f159391..8ee183a 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -105,9 +105,12 @@ original confirmation context without printing it. If only the initial status query fails, the command still returns the accepted `run_id` and a `call status` `next_command`. -Successful command stdout is JSON except help and version output. Some -top-level or local failures may print plain stderr. Access tokens are read from -the local cache and are never printed. +Command stdout is JSON except help and version output, for failures as well as +successes: every error writes a JSON envelope with a CLI-owned `error.code` to +stdout, a one-line summary to stderr, and exits non-zero. Upstream error details +appear only under `error.remote_error`, sanitized and bounded. See +[Error Envelopes](./docs/cli-reference.md#error-envelopes). Access tokens are +read from the local cache and are never printed. ## Options diff --git a/packages/cli/docs/cli-reference.md b/packages/cli/docs/cli-reference.md index 466e75b..3bdf0e0 100644 --- a/packages/cli/docs/cli-reference.md +++ b/packages/cli/docs/cli-reference.md @@ -4,8 +4,10 @@ This is the canonical reference for `calle` commands, options, defaults, and parameter examples. When changing CLI commands or options, update this document and any synchronized command guidance in the same change. -Successful command stdout is JSON except `--help`, `-h`, `--version`, and `-V`. -Some top-level or local failures may print plain stderr. +Command stdout is JSON except `--help`, `-h`, `--version`, and `-V`. This holds +for failures too: every error leaves through the same JSON envelope on stdout, +with a one-line summary on stderr and a non-zero exit code. See +[Error Envelopes](#error-envelopes). ## JSON Result Envelopes @@ -45,6 +47,57 @@ than the latest call state. See the [MCP tool result envelope](../../../docs/mcp/openagent-oauth.md#tool-result-envelope) for the direct protocol shape and SDK field-name differences. +## Error Envelopes + +Every failure, including argument errors, transport failures, and upstream HTTP +errors, writes one JSON object to stdout and exits non-zero: + +```json +{ + "ok": false, + "server_url": "https://example.test/mcp/openagent_oauth", + "error": { + "code": "broker_unavailable", + "message": "Client error '502 Bad Gateway' for url '...' Failed to register an OAuth client. The CALL-E login service is unavailable. ...", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client." + } + } +} +``` + +Stable fields: + +| Field | Always present | Meaning | +| --- | --- | --- | +| `ok` | yes | `false` for every error envelope. | +| `server_url` | yes | Configured MCP server URL, or `null` when configuration could not be resolved. | +| `error.code` | yes | A code owned by the CLI. Branch on this. | +| `error.message` | yes | Human-readable summary; the same text is written to stderr. | +| `error.status_code` | HTTP errors | Upstream HTTP status. | +| `error.remote_error` | when an upstream body was readable | `{ code?, message? }` extracted from the upstream response, sanitized and bounded. Informational only. | +| `error.cause_code` | transport errors | Node.js error code such as `ENOTFOUND` or `ECONNREFUSED`, when known. | +| `help_command` | argument errors only | A directly runnable `--help` command. | + +`error.code` values: + +| Code | Exit | When | +| --- | --- | --- | +| `invalid_arguments` | 2 | Unknown command, missing or invalid option. `help_command` is set. | +| `auth_required` | 1 | No usable token, or the server rejected the token. Run `auth login`. | +| `broker_unavailable` | 1 | The brokered-login service returned a 5xx. Not a local problem. | +| `http_error` | 1 | Any other non-success HTTP status from a CLI-side request. | +| `transport_error` | 1 | The request never received a response: DNS, connection, TLS, or timeout. | +| `mcp_error` | 1 | An MCP-level failure, including stage failures from `call` commands, which add `stage`, `call_started`, `retry_safe`, and recovery fields. | + +`error.code` is never taken from an upstream response. Upstream error codes and +messages appear only under `error.remote_error`, after sanitization: unknown +fields are dropped unread, codes are limited to `[A-Za-z0-9_.:-]` and 64 +characters, messages are limited to 500 characters, and terminal control +sequences are removed before anything reaches stdout or stderr. + ## Finding Command Help Help is available at the root, command-group, and subcommand levels: diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 4bf9c4c..c58d20f 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -23,6 +23,7 @@ import { resolveRuntimeConfig, } from "./config.js"; import { ensurePendingLogin, loginWithBroker } from "./broker-client.js"; +import { HttpStatusError } from "./http.js"; import { AuthRequiredError, McpHttpError, @@ -882,6 +883,51 @@ function errorPayload(error, config, helpCommand = null) { }; } + if (error instanceof HttpStatusError) { + const remoteError = sanitizedRemoteError(error.responseText); + const brokerUnavailable = isBrokerRegistrationFailure(error); + // error.message embeds the upstream status text, so it is remote-influenced too. + const messageParts = [safeRemoteString(error.message, LOCAL_MESSAGE_LIMIT) ?? "HTTP request failed."]; + if (remoteError?.message) { + messageParts.push(remoteError.message); + } + if (brokerUnavailable) { + messageParts.push(BROKER_UNAVAILABLE_HINT); + } + return { + exitCode: 1, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + // The top-level code is always locally owned. An upstream body must never be + // able to impersonate a stable local code such as `auth_required`. + code: brokerUnavailable ? "broker_unavailable" : "http_error", + message: messageParts.join(" "), + status_code: error.statusCode, + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + + if (isTransportFailure(error)) { + const causeCode = safeRemoteCode(error?.cause?.code); + const detail = safeRemoteString(error?.message, LOCAL_MESSAGE_LIMIT) ?? "Request failed before a response was received."; + return { + exitCode: 1, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: "transport_error", + message: causeCode ? `${detail} (${causeCode})` : detail, + ...(causeCode ? { cause_code: causeCode } : {}), + }, + }, + }; + } + return { exitCode: 1, body: { @@ -889,17 +935,86 @@ function errorPayload(error, config, helpCommand = null) { server_url: config?.serverUrl ?? null, error: { code: "mcp_error", - message: error?.message || String(error), + message: safeRemoteString(error?.message ?? String(error), LOCAL_MESSAGE_LIMIT) ?? "Unknown error.", }, }, }; } +const LOCAL_MESSAGE_LIMIT = 300; +const REMOTE_MESSAGE_LIMIT = 500; + +const BROKER_UNAVAILABLE_HINT = + "The CALL-E login service is unavailable. This is not a local configuration problem, so reinstalling the CLI will not help. Retry later, or use the Developer API with a dashboard API key, which does not depend on brokered login."; + +/** + * Reduce an upstream HTTP error body to at most two sanitized strings. + * + * Allowlist only: `code` (or a string-valued `error`) and `message`, read from the top level + * or from a nested `error` object. Every other field is dropped unread, so token-like or + * internal fields in a response can never reach stdout or stderr. Both strings pass through + * the same sanitizer as MCP call errors; codes are additionally constrained to a machine-safe + * character set and length. + */ +function sanitizedRemoteError(responseText) { + const raw = typeof responseText === "string" ? responseText : ""; + if (!raw.trim()) { + return null; + } + + let parsed; + try { + parsed = JSON.parse(raw); + } catch { + parsed = undefined; + } + + const record = recordObject(parsed); + if (!record) { + // Non-JSON (typically a gateway's HTML page) or a JSON array/scalar: keep a bounded, + // control-stripped excerpt and nothing else. + const message = safeRemoteString(raw, REMOTE_MESSAGE_LIMIT); + return message ? { message } : null; + } + + const nested = recordObject(record.error) || {}; + const codeValue = nested.code ?? record.code ?? (typeof record.error === "string" ? record.error : undefined); + const code = safeRemoteCode(codeValue); + const message = safeRemoteString(nested.message ?? record.message, REMOTE_MESSAGE_LIMIT); + + if (code === undefined && message === undefined) { + return null; + } + return { + ...(code !== undefined ? { code } : {}), + ...(message !== undefined ? { message } : {}), + }; +} + +function isBrokerRegistrationFailure(error) { + return Number(error?.statusCode) >= 500 + && /\/api\/v1\/openagent-auth\/sessions/u.test(String(error?.message ?? "")); +} + +// A rejected fetch (DNS failure, connection refused, TLS error) surfaces as a TypeError with +// a `cause`; the core HTTP layer turns an aborted request into a plain timeout Error. +function isTransportFailure(error) { + if (error instanceof HttpStatusError || error instanceof McpHttpError) { + return false; + } + if (error instanceof TypeError) { + return true; + } + return /^Request timed out for /u.test(String(error?.message ?? "")); +} + function writeCommandError(stdout, stderr, error, config, helpCommand = null) { const formatted = errorPayload(error, config, helpCommand); writeJson(stdout, formatted.body); + // Remote-derived strings are sanitized at the source; this is the last line of defence + // for the one channel that goes straight to a terminal. stderr([ - formatted.body.error.message, + stripTerminalControls(formatted.body.error.message), ...(formatted.body.help_command ? [`Run '${formatted.body.help_command}' for usage.`] : []), ].join("\n")); return formatted.exitCode; @@ -1018,11 +1133,38 @@ function structuredPayload(result) { return result?.structuredContent || result?.structured_content || result || {}; } +// ANSI/VT escape sequences (CSI, OSC, and single-character ESC forms) plus C0/C1 control +// characters. Anything remote-supplied that reaches a log line or a terminal goes through +// this, so a hostile or misconfigured upstream cannot inject cursor movement, colour, line +// breaks, or hidden text into agent-visible output. +const TERMINAL_CONTROL_RE = + /\u001b\[[0-?]*[ -\/]*[@-~]|\u001b\][^\u0007\u001b]*(?:\u0007|\u001b\\)|\u001b[@-_]|[\u0000-\u001f\u007f-\u009f]/gu; + +function stripTerminalControls(value) { + return String(value).replace(TERMINAL_CONTROL_RE, " "); +} + function safeRemoteString(value, maxLength = 1000) { - if (typeof value !== "string" || !value.trim()) { + if (typeof value !== "string") { + return undefined; + } + const cleaned = stripTerminalControls(value).trim(); + if (!cleaned) { + return undefined; + } + return cleaned.slice(0, maxLength); +} + +// Machine codes from upstream are kept only as an opaque, normalized token under +// `remote_error`; they never become the CLI's own `error.code`, which agent hosts branch on. +const REMOTE_CODE_RE = /^[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u; + +function safeRemoteCode(value) { + if (typeof value !== "string") { return undefined; } - return value.trim().slice(0, maxLength); + const cleaned = stripTerminalControls(value).trim(); + return REMOTE_CODE_RE.test(cleaned) ? cleaned : undefined; } function safeRemoteCallError(result) { @@ -1716,10 +1858,6 @@ export async function runCli(argv, deps = {}) { try { return await runCliCommand(argv, deps); } catch (error) { - if (!(error instanceof InvalidArgumentsError)) { - throw error; - } - const stdout = deps.stdout || ((text) => process.stdout.write(text)); const stderr = deps.stderr || ((text) => process.stderr.write(`${text}\n`)); const [group, command, ...rest] = argv; @@ -1730,7 +1868,13 @@ export async function runCli(argv, deps = {}) { } catch { // Invalid option syntax may prevent runtime configuration from being resolved. } - return writeCommandError(stdout, stderr, error, config, helpCommandFor(group, command)); + // Every failure leaves through the documented JSON envelope, not just argument errors. + // Agent hosts are instructed to treat all command output as JSON, so a transport or + // upstream failure that printed a bare string left them with nothing to parse. + const helpCommand = error instanceof InvalidArgumentsError + ? helpCommandFor(group, command) + : null; + return writeCommandError(stdout, stderr, error, config, helpCommand); } } diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index 6fcb880..b0acc71 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -370,6 +370,206 @@ test("auth login start-only returns authorization hint without polling", async ( assert.doesNotMatch(result.stdout, /secret-1/); }); +test("auth login surfaces the upstream error body when brokered login registration fails", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-broker-5xx"); + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response( + JSON.stringify({ + error: "oauth_register_failed", + message: "Failed to register an OAuth client. err_type=HTTPStatusError", + }), + { status: 502, headers: { "content-type": "application/json" } } + ); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", + "--cache-root", + cacheRoot, + ], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.ok, false); + assert.equal(payload.error.status_code, 502); + assert.equal(payload.error.code, "broker_unavailable", "top-level code stays CLI-owned"); + assert.deepEqual(payload.error.remote_error, { + code: "oauth_register_failed", + message: "Failed to register an OAuth client. err_type=HTTPStatusError", + }); + assert.match(payload.error.message, /Failed to register an OAuth client/); + assert.match(payload.error.message, /login service is unavailable/); + assert.match(payload.error.message, /dashboard API key/); + assert.match(result.stderr, /Failed to register an OAuth client/); +}); + +test("auth login keeps a non-JSON upstream error body readable and bounded", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-broker-html"); + const body = `${"gateway ".repeat(200)}`; + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response(body, { status: 503, headers: { "content-type": "text/html" } }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + + const result = await run( + [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", + "--cache-root", + cacheRoot, + ], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.status_code, 503); + assert.equal(payload.error.code, "broker_unavailable"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.equal(payload.error.remote_error.code, undefined); +}); + +function brokerFailure(status, body, contentType = "application/json") { + return async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return new Response(typeof body === "string" ? body : JSON.stringify(body), { + status, + headers: { "content-type": contentType }, + }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; +} + +const LOGIN_ARGS = [ + "auth", + "login", + "--start-only", + "--no-browser-open", + "--base-url", + "https://mcp.example", +]; + +const CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f]/u; + +test("auth login never lets an upstream body impersonate a local error code", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-forged-code"); + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { fetchImpl: brokerFailure(502, { error: "auth_required", message: "please log in again" }) } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_unavailable"); + assert.equal(payload.status, undefined, "must not look like a login_required response"); + assert.equal(payload.assistant_hint, undefined); + assert.equal(payload.login_url, undefined); + assert.equal(payload.error.remote_error.code, "auth_required"); + assert.equal(payload.error.remote_error.message, "please log in again"); +}); + +test("auth login bounds and sanitizes hostile upstream JSON", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-hostile"); + const longMessage = "x".repeat(20_000); + const hostile = { + error: { + code: "bad code\u001b[31m", + message: `line one\r\ninjected line\u001b[2J\u001b[H${longMessage}`, + access_token: "sk_live_SUPERSECRET_DO_NOT_PRINT", + }, + token: "tok_ALSO_SECRET", + refresh_token: "rt_SECRET_TOO", + }; + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { fetchImpl: brokerFailure(502, hostile) } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "broker_unavailable"); + assert.equal(payload.error.remote_error.code, undefined, "unsafe code is dropped, not sanitized into something plausible"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.ok(payload.error.message.length < 1200); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + assert.doesNotMatch(payload.error.message, CONTROL_CHARS); + assert.doesNotMatch(result.stderr, /\u001b|\r/u); + for (const secret of ["SUPERSECRET", "tok_ALSO_SECRET", "rt_SECRET_TOO", "access_token", "refresh_token"]) { + assert.doesNotMatch(result.stdout, new RegExp(secret)); + assert.doesNotMatch(result.stderr, new RegExp(secret)); + } +}); + +test("auth login reads a nested upstream error object and drops everything else", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-nested"); + const result = await run( + [...LOGIN_ARGS, "--cache-root", cacheRoot], + { + fetchImpl: brokerFailure(500, { + error: { code: "nested.code-1", message: "nested message", details: { internal: "trace-abc" } }, + request_id: "req_123", + }), + } + ); + const payload = JSON.parse(result.stdout); + + assert.deepEqual(payload.error.remote_error, { code: "nested.code-1", message: "nested message" }); + assert.doesNotMatch(result.stdout, /trace-abc|req_123|details|request_id/u); +}); + +test("auth login returns a transport_error envelope when fetch rejects", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-fetch-rejected"); + const fetchImpl = async () => { + const error = new TypeError("fetch failed"); + error.cause = { code: "ENOTFOUND", syscall: "getaddrinfo", hostname: "mcp.example" }; + throw error; + }; + const result = await run([...LOGIN_ARGS, "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.ok, false); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.cause_code, "ENOTFOUND"); + assert.equal(payload.help_command, undefined); + assert.match(payload.error.message, /fetch failed/u); + assert.ok(result.stderr.length < 500); + // The test harness terminates each stderr write with a newline; everything else must be clean. + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); +}); + +test("auth login classifies a request timeout as a transport_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-timeout"); + const fetchImpl = async () => { + throw new Error("Request timed out for POST https://mcp.example/api/v1/openagent-auth/sessions"); + }; + const result = await run([...LOGIN_ARGS, "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.cause_code, undefined); + assert.match(payload.error.message, /timed out/u); +}); + test("auth login start-only replaces locally active pending cache when broker reports it expired", async () => { const cacheRoot = makeTempRoot("calle-cli-login-start-only-expired-broker"); const serverUrl = "https://mcp.example/mcp/openagent_oauth";