From 359ee27df11e70ba5cde1d294e68b3765334a183 Mon Sep 17 00:00:00 2001 From: N DIVIJ Date: Fri, 4 Sep 2026 12:47:38 +0530 Subject: [PATCH 1/3] fix(cli): emit the JSON error envelope for transport failures runCli rethrew every error that was not an InvalidArgumentsError, so an HTTP or transport failure escaped to main() and printed a bare message on stderr with nothing on stdout. The skill instructs agent hosts to treat all command output as JSON, so a failed `auth login` gave them an empty stdout and no error.code to branch on. Route every failure through writeCommandError, and give HttpStatusError its own payload branch carrying status_code, the parsed upstream remote_error, and a bounded fallback for non-JSON bodies. A 5xx from the brokered-login registration endpoint now also says the failure is server-side and that the Developer API path does not depend on it, instead of leaving users to guess that reinstalling the CLI might help. Adds coverage for both the JSON and the non-JSON upstream error body. --- ...n-error-envelope-for-transport-failures.md | 38 ++++++++++ packages/cli/lib/cli.js | 76 +++++++++++++++++-- packages/cli/test/cli.test.js | 75 ++++++++++++++++++ 3 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 .changeset/json-error-envelope-for-transport-failures.md 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..0756bf2 --- /dev/null +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -0,0 +1,38 @@ +--- +"@call-e/cli": patch +--- + +Always emit the documented JSON error envelope, and surface the upstream error body. + +`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. + +Failures now leave through `writeCommandError` like any other error, and `HttpStatusError` +gains its own payload branch carrying `status_code`, the parsed upstream `remote_error`, and +a bounded fallback for non-JSON bodies. When the brokered-login registration endpoint returns +5xx, the message also states that the failure is server-side and that the Developer API path +does not depend on it — the previous output invited users to reinstall the CLI instead. + +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": "oauth_register_failed", + "status_code": 502, + "remote_error": { + "code": "oauth_register_failed", + "message": "Failed to register an OAuth client. err_type=HTTPStatusError" + } + } +} +``` diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 4bf9c4c..6c5e06c 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,34 @@ function errorPayload(error, config, helpCommand = null) { }; } + if (error instanceof HttpStatusError) { + const remoteError = parseRemoteErrorBody(error.responseText); + const brokerUnavailable = isBrokerRegistrationFailure(error); + const messageParts = [error.message]; + if (remoteError?.message) { + messageParts.push(remoteError.message); + } + if (brokerUnavailable) { + messageParts.push( + "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." + ); + } + return { + exitCode: 1, + body: { + ok: false, + server_url: config?.serverUrl ?? null, + error: { + code: remoteError?.code + || (brokerUnavailable ? "broker_unavailable" : "http_error"), + message: messageParts.join(" "), + status_code: error.statusCode, + ...(remoteError ? { remote_error: remoteError } : {}), + }, + }, + }; + } + return { exitCode: 1, body: { @@ -895,6 +924,41 @@ function errorPayload(error, config, helpCommand = null) { }; } +const REMOTE_ERROR_BODY_LIMIT = 500; + +function parseRemoteErrorBody(responseText) { + const text = String(responseText ?? "").trim(); + if (!text) { + return null; + } + let parsed; + try { + parsed = JSON.parse(text); + } catch { + return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + } + if (!parsed || typeof parsed !== "object") { + return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + } + const source = parsed.error && typeof parsed.error === "object" ? parsed.error : parsed; + const code = typeof source.code === "string" + ? source.code + : (typeof parsed.error === "string" ? parsed.error : undefined); + const message = typeof source.message === "string" ? source.message : undefined; + if (!code && !message) { + return null; + } + return { + ...(code ? { code } : {}), + ...(message ? { message } : {}), + }; +} + +function isBrokerRegistrationFailure(error) { + return Number(error?.statusCode) >= 500 + && /\/api\/v1\/openagent-auth\/sessions/u.test(String(error?.message ?? "")); +} + function writeCommandError(stdout, stderr, error, config, helpCommand = null) { const formatted = errorPayload(error, config, helpCommand); writeJson(stdout, formatted.body); @@ -1716,10 +1780,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 +1790,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..4fa90d0 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -370,6 +370,81 @@ 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, "oauth_register_failed"); + 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); +}); + 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"; From f57ed621a1ba55761e882bd110f2596f144622d8 Mon Sep 17 00:00:00 2001 From: N DIVIJ Date: Sat, 5 Sep 2026 20:10:24 +0530 Subject: [PATCH 2/3] fix(cli): keep error.code CLI-owned and sanitize upstream error bodies Address review on the JSON error envelope: - error.code is never taken from an upstream response. HTTP failures use broker_unavailable or http_error; a rejected or timed-out fetch uses transport_error. An upstream body can no longer impersonate a stable local code such as auth_required. - Upstream detail lives only under error.remote_error, reduced to code and message (top-level or nested under `error`); every other field is dropped unread so token-like values cannot reach stdout or stderr. - One sanitizer for all remote-derived strings: safeRemoteString now strips ANSI CSI/OSC/ESC sequences and C0/C1 controls before bounding, and safeRemoteCode constrains machine codes to [A-Za-z0-9_.:-]{1,64}. writeCommandError strips controls again before writing stderr. - Add regressions: forged auth_required, 20 KB message, nested error object with internal fields, CR/LF/ANSI content, secret-like fields absent from stdout and stderr, unsafe code dropped, fetch rejecting with TypeError/ENOTFOUND, and a request timeout. - Document the error envelope and its stable fields in cli-reference.md and README.md, replacing the statement that some failures print plain stderr. Correct the changeset to describe the sanitization. --- ...n-error-envelope-for-transport-failures.md | 21 ++- packages/cli/README.md | 9 +- packages/cli/docs/cli-reference.md | 57 +++++++- packages/cli/lib/cli.js | 134 ++++++++++++++---- packages/cli/test/cli.test.js | 127 ++++++++++++++++- 5 files changed, 307 insertions(+), 41 deletions(-) diff --git a/.changeset/json-error-envelope-for-transport-failures.md b/.changeset/json-error-envelope-for-transport-failures.md index 0756bf2..b0420b1 100644 --- a/.changeset/json-error-envelope-for-transport-failures.md +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -2,18 +2,23 @@ "@call-e/cli": patch --- -Always emit the documented JSON error envelope, and surface the upstream error body. +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. -Failures now leave through `writeCommandError` like any other error, and `HttpStatusError` -gains its own payload branch carrying `status_code`, the parsed upstream `remote_error`, and -a bounded fallback for non-JSON bodies. When the brokered-login registration endpoint returns -5xx, the message also states that the failure is server-side and that the Developer API path -does not depend on it — the previous output invited users to reinstall the CLI instead. +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: @@ -27,7 +32,7 @@ After: { "ok": false, "error": { - "code": "oauth_register_failed", + "code": "broker_unavailable", "status_code": 502, "remote_error": { "code": "oauth_register_failed", @@ -36,3 +41,5 @@ After: } } ``` + +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 6c5e06c..c58d20f 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -884,16 +884,15 @@ function errorPayload(error, config, helpCommand = null) { } if (error instanceof HttpStatusError) { - const remoteError = parseRemoteErrorBody(error.responseText); + const remoteError = sanitizedRemoteError(error.responseText); const brokerUnavailable = isBrokerRegistrationFailure(error); - const messageParts = [error.message]; + // 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( - "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." - ); + messageParts.push(BROKER_UNAVAILABLE_HINT); } return { exitCode: 1, @@ -901,8 +900,9 @@ function errorPayload(error, config, helpCommand = null) { ok: false, server_url: config?.serverUrl ?? null, error: { - code: remoteError?.code - || (brokerUnavailable ? "broker_unavailable" : "http_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 } : {}), @@ -911,6 +911,23 @@ function errorPayload(error, config, helpCommand = null) { }; } + 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: { @@ -918,39 +935,59 @@ 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 REMOTE_ERROR_BODY_LIMIT = 500; - -function parseRemoteErrorBody(responseText) { - const text = String(responseText ?? "").trim(); - if (!text) { +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(text); + parsed = JSON.parse(raw); } catch { - return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + parsed = undefined; } - if (!parsed || typeof parsed !== "object") { - return { message: text.slice(0, REMOTE_ERROR_BODY_LIMIT) }; + + 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 source = parsed.error && typeof parsed.error === "object" ? parsed.error : parsed; - const code = typeof source.code === "string" - ? source.code - : (typeof parsed.error === "string" ? parsed.error : undefined); - const message = typeof source.message === "string" ? source.message : undefined; - if (!code && !message) { + + 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 ? { code } : {}), - ...(message ? { message } : {}), + ...(code !== undefined ? { code } : {}), + ...(message !== undefined ? { message } : {}), }; } @@ -959,11 +996,25 @@ function isBrokerRegistrationFailure(error) { && /\/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; @@ -1082,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) { diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index 4fa90d0..b0acc71 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -403,7 +403,7 @@ test("auth login surfaces the upstream error body when brokered login registrati assert.equal(result.code, 1); assert.equal(payload.ok, false); assert.equal(payload.error.status_code, 502); - assert.equal(payload.error.code, "oauth_register_failed"); + 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", @@ -443,6 +443,131 @@ test("auth login keeps a non-JSON upstream error body readable and bounded", asy 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 () => { From 8f08207e589175aa3115d0545d708dfd310503eb Mon Sep 17 00:00:00 2001 From: N DIVIJ Date: Sun, 6 Sep 2026 11:26:09 +0530 Subject: [PATCH 3/3] fix(core,cli): one sanitization boundary for remote text, typed transport errors Address the second review round on the JSON error envelope: - New @call-e/core/sanitize: stripTerminalControls, redactSecrets, safeRemoteString, safeRemoteCode, sanitizeRemoteError. One implementation shared by the core MCP client and the CLI, so remote text is made safe where the error is created. Credential-shaped substrings inside messages are redacted, not just token-like fields. - McpHttpError.message is always locally authored; the server's JSON-RPC error text lives raw in payload and sanitized in remoteError. The CLI shows it only under error.remote_error. Same for plan_not_ready's clarifying question, which is no longer interpolated into the summary. - error.message and stderr for HTTP failures are built from the status code and our own URL, never from the response's status text or body. - Typed transport boundary: core http.js throws TransportError when fetch rejects or times out; the MCP client marks those McpHttpError with code transport_error, transport, timedOut, causeCode. The CLI classifies transport_error only from those types. An unrelated local TypeError is internal_error and is never described as a network condition. - Single classifyError over an exported ERROR_CODES table drives the envelope, stderr, and telemetry; a test asserts docs/cli-reference.md documents exactly that set, including the call-stage and recovery codes. README carries a sync-with marker for the error-envelope contract. - Tests: hostile MCP tools/list and tools/call errors, hostile clarifying question, rejected fetch with ENOTFOUND, timeout via AbortError, unrelated TypeError, telemetry codes for broker and transport failures, sanitizer unit tests, and the docs contract test. --- ...n-error-envelope-for-transport-failures.md | 44 ++- packages/cli/README.md | 7 +- packages/cli/docs/cli-reference.md | 52 ++-- packages/cli/lib/cli.js | 267 +++++++++--------- packages/cli/lib/sanitize.js | 1 + packages/cli/package.json | 2 +- packages/cli/test/cli.test.js | 221 ++++++++++++++- packages/core/lib/http.d.ts | 22 ++ packages/core/lib/http.js | 55 +++- packages/core/lib/index.d.ts | 1 + packages/core/lib/index.js | 1 + packages/core/lib/mcp-client.d.ts | 10 + packages/core/lib/mcp-client.js | 58 +++- packages/core/lib/sanitize.d.ts | 13 + packages/core/lib/sanitize.js | 139 +++++++++ packages/core/package.json | 5 +- packages/core/test/core.test.js | 85 +++++- 17 files changed, 795 insertions(+), 188 deletions(-) create mode 100644 packages/cli/lib/sanitize.js create mode 100644 packages/core/lib/sanitize.d.ts create mode 100644 packages/core/lib/sanitize.js diff --git a/.changeset/json-error-envelope-for-transport-failures.md b/.changeset/json-error-envelope-for-transport-failures.md index b0420b1..899b13a 100644 --- a/.changeset/json-error-envelope-for-transport-failures.md +++ b/.changeset/json-error-envelope-for-transport-failures.md @@ -1,24 +1,43 @@ --- +"@call-e/core": patch "@call-e/cli": patch --- -Always emit the documented JSON error envelope, and surface upstream error detail safely. +Always emit the documented JSON error envelope, with one sanitization boundary for remote text. `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`. +**core** + +- New `@call-e/core/sanitize`: `stripTerminalControls`, `redactSecrets`, `safeRemoteString`, + `safeRemoteCode`, `sanitizeRemoteError`. One implementation for every remote-supplied + string: terminal control sequences removed, credential-shaped substrings redacted, codes + constrained to `[A-Za-z0-9_.:-]{1,64}`, messages bounded to 500 characters, and only + `code` / `message` read from a body — every other field dropped unread. +- `http.js` throws a typed `TransportError` when `fetch` rejects or times out, carrying + `url`, `method`, `timedOut`, and the cause's Node.js code. `HttpStatusError` now records + `url`. +- `McpHttpError.message` is always locally authored. The server's JSON-RPC error text is kept + raw in `payload` and, sanitized, in the new `remoteError` field. Timeouts and rejected + fetches are `code: "transport_error"` with `transport: true` / `timedOut`. + +**cli** + +- Every failure leaves through `writeCommandError`. `error.code` comes from a single + exported `ERROR_CODES` table via `classifyError`, which the JSON envelope, stderr, and + telemetry all share; a test asserts the table matches `docs/cli-reference.md` exactly. +- `error.message` and stderr are authored by the CLI and never contain remote text. Remote + detail — HTTP bodies, JSON-RPC errors, `plan_not_ready` clarifying questions — appears only + under `error.remote_error` after sanitization. +- `transport_error` (and `error.transport: true`) is set only from the typed transport + boundary. An unrelated local `TypeError` is `internal_error`, never a network condition. +- Hostile-input regressions: forged `auth_required`, 20 KB flat and nested bodies, + CR/LF/ANSI content, secret-like fields and secret-like substrings inside messages absent + from stdout and stderr, hostile MCP `tools/list` and `tools/call` errors, a hostile + clarifying question, rejected fetch, timeout, and an unrelated `TypeError`. Before, against a broker returning 502: @@ -33,6 +52,7 @@ After: "ok": false, "error": { "code": "broker_unavailable", + "message": "HTTP 502 from https://.../api/v1/openagent-auth/sessions. The CALL-E login service is unavailable. ...", "status_code": 502, "remote_error": { "code": "oauth_register_failed", @@ -42,4 +62,4 @@ After: } ``` -The CLI reference and README now document the error envelope and its stable fields. +The CLI reference documents every stable envelope field and the complete `error.code` list. diff --git a/packages/cli/README.md b/packages/cli/README.md index 8ee183a..ad840a8 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -105,10 +105,13 @@ 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`. + 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 +stdout, a CLI-authored one-line summary to stderr, and exits non-zero. +`error.transport: true` marks the only case that is a network condition. Remote +text appears only under `error.remote_error`, sanitized, redacted, and bounded. +The complete code list is in [Error Envelopes](./docs/cli-reference.md#error-envelopes). Access tokens are read from the local cache and are never printed. diff --git a/packages/cli/docs/cli-reference.md b/packages/cli/docs/cli-reference.md index 3bdf0e0..1cc6fee 100644 --- a/packages/cli/docs/cli-reference.md +++ b/packages/cli/docs/cli-reference.md @@ -74,14 +74,18 @@ Stable fields: | --- | --- | --- | | `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: +| `error.code` | yes | A code owned by the CLI, from the table below. Branch on this. | +| `error.message` | yes | A summary **authored by the CLI**. Never contains upstream text. The same text is written to stderr. | +| `error.status_code` | HTTP and MCP errors | Upstream HTTP status, or `null`. | +| `error.transport` | `true` only when no response was received | The request failed at the network layer: DNS, connection, TLS, or timeout. Absent otherwise — an unrelated local error is never described as a network condition. | +| `error.cause_code` | transport errors, when known | `timeout`, or the Node.js error code such as `ENOTFOUND` or `ECONNREFUSED`. | +| `error.remote_error` | when the service said something readable | `{ code?, message? }` from the remote response — an HTTP body, a JSON-RPC error, or a clarifying question — after sanitization. **Untrusted, informational only.** | +| `error.error_code`, `error.status` | `call` stage failures | Sanitized remote call-outcome fields (for example `EXECUTION_ACK_LOST`). | +| `stage`, `call_started`, `retry_safe`, `recovery_id`, `next_command` | `call` stage failures | Which stage failed and whether it is safe to retry. When `retry_safe` is `false`, run the returned `next_command` (`calle call recover …`) instead of starting a new call. | +| `help_command` | `invalid_arguments` only | A directly runnable `--help` command. | + +`error.code` values — this table is the complete set, and the test suite fails +if the CLI can emit a code that is not listed here: | Code | Exit | When | | --- | --- | --- | @@ -89,14 +93,30 @@ Stable fields: | `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. +| `transport_error` | 1 | The request never received a response: DNS, connection, TLS, or timeout. `transport: true`. | +| `mcp_error` | 1 | The MCP server returned a JSON-RPC error. Its message is under `remote_error`. | +| `plan_not_ready` | 1 | `call start`: the plan needs more information. The clarifying question is under `remote_error.message`. | +| `plan_call_invalid_response` | 1 | `call start`: `plan_call` succeeded but returned no usable `plan_id` / `confirm_token`. | +| `run_call_missing_run_id` | 1 | `call start` / `call run`: execution may have been accepted without a stable `run_id`; a `recovery_id` and `next_command` are returned. | +| `recovery_not_found` | 1 | `call recover`: no local recovery record for that id. | +| `recovery_storage_error` | 1 | `call recover`: the local recovery record could not be read or written. | +| `plan_call_error` | 1 | The `plan_call` stage failed with a non-transport error. | +| `plan_call_timeout` | 1 | The `plan_call` stage received no response in time. `transport: true`. | +| `run_call_error` | 1 | The `run_call` stage failed with a non-transport error. | +| `run_call_timeout` | 1 | The `run_call` stage received no response in time. `transport: true`. | +| `get_call_run_error` | 1 | The `get_call_run` stage failed with a non-transport error. | +| `get_call_run_timeout` | 1 | The `get_call_run` stage received no response in time. `transport: true`. | +| `internal_error` | 1 | An unexpected local exception inside the CLI. Not a network condition. | + +`error.code` is never taken from a remote response. Remote text — HTTP bodies, +JSON-RPC error messages, clarifying questions, call-outcome fields — appears only +under `error.remote_error` (and the sanitized `error_code` / `status` stage +fields), after one shared sanitizer: only `code` and `message` are read, every +other field is dropped unread, codes are limited to `[A-Za-z0-9_.:-]` and 64 +characters, messages are limited to 500 characters, credential-shaped +substrings are redacted, and terminal control sequences are removed before +anything reaches stdout or stderr. Telemetry reports the same `error.code` as +the envelope. ## Finding Command Help diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index c58d20f..ca378f3 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -23,7 +23,14 @@ import { resolveRuntimeConfig, } from "./config.js"; import { ensurePendingLogin, loginWithBroker } from "./broker-client.js"; -import { HttpStatusError } from "./http.js"; +import { HttpStatusError, TransportError } from "./http.js"; +import { + REMOTE_MESSAGE_LIMIT, + safeRemoteCode, + safeRemoteString, + sanitizeRemoteError, + stripTerminalControls, +} from "./sanitize.js"; import { AuthRequiredError, McpHttpError, @@ -58,8 +65,11 @@ class CallStageError extends McpHttpError { recoveryId = null, nextCommand = null, remoteError = null, + transport = false, + timedOut = false, + cause, }) { - super(message, { code, statusCode }); + super(message, { code, statusCode, transport, timedOut, ...(cause !== undefined ? { cause } : {}) }); this.name = "CallStageError"; this.stage = stage; this.callStarted = callStarted; @@ -856,12 +866,17 @@ function errorPayload(error, config, helpCommand = null) { }; } + const classified = classifyError(error); + if (error instanceof McpHttpError) { - const remoteError = error instanceof CallStageError && error.remoteError - ? error.remoteError - : null; + const stageRemote = error instanceof CallStageError && error.remoteError ? error.remoteError : null; + // Display detail comes from the sanitized copy the core client attached, or from the + // stage's own sanitized remote fields. Never from `error.payload` directly. + const remoteError = error.remoteError + ?? (stageRemote?.message ? { message: stageRemote.message } : null); + const causeCode = safeRemoteCode(error.causeCode); return { - exitCode: 1, + exitCode: classified.exitCode, body: { ok: false, server_url: config?.serverUrl ?? null, @@ -873,37 +888,33 @@ function errorPayload(error, config, helpCommand = null) { ...(error.nextCommand ? { next_command: error.nextCommand } : {}), } : {}), error: { - code: error.code || "mcp_error", - message: error.message, + code: classified.code, + message: localMessage(error.message), status_code: error.statusCode, - ...(remoteError?.error_code !== undefined ? { error_code: remoteError.error_code } : {}), - ...(remoteError?.status !== undefined ? { status: remoteError.status } : {}), + ...(classified.transport ? { transport: true } : {}), + ...(causeCode ? { cause_code: causeCode } : {}), + ...(stageRemote?.error_code !== undefined ? { error_code: stageRemote.error_code } : {}), + ...(stageRemote?.status !== undefined ? { status: stageRemote.status } : {}), + ...(remoteError ? { remote_error: remoteError } : {}), }, }, }; } 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); - } + const remoteError = sanitizeRemoteError(error.responseText); + const brokerUnavailable = classified.code === "broker_unavailable"; + // The summary is authored here, from the status code and our own URL — never from the + // response, whose status text and body are both remote-controlled. + const summary = `HTTP ${error.statusCode ?? "error"} from ${describeUrl(error.url)}.`; return { - exitCode: 1, + exitCode: classified.exitCode, 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(" "), + code: classified.code, + message: brokerUnavailable ? `${summary} ${BROKER_UNAVAILABLE_HINT}` : summary, status_code: error.statusCode, ...(remoteError ? { remote_error: remoteError } : {}), }, @@ -911,17 +922,20 @@ function errorPayload(error, config, helpCommand = null) { }; } - if (isTransportFailure(error)) { - const causeCode = safeRemoteCode(error?.cause?.code); - const detail = safeRemoteString(error?.message, LOCAL_MESSAGE_LIMIT) ?? "Request failed before a response was received."; + if (error instanceof TransportError) { + const causeCode = safeRemoteCode(error.code); + const summary = error.timedOut + ? `Request timed out for ${error.method ?? "request"} ${describeUrl(error.url)}.` + : `Request failed before a response was received for ${error.method ?? "request"} ${describeUrl(error.url)}.`; return { - exitCode: 1, + exitCode: classified.exitCode, body: { ok: false, server_url: config?.serverUrl ?? null, error: { code: "transport_error", - message: causeCode ? `${detail} (${causeCode})` : detail, + message: causeCode && !error.timedOut ? `${summary} (${causeCode})` : summary, + transport: true, ...(causeCode ? { cause_code: causeCode } : {}), }, }, @@ -929,83 +943,104 @@ function errorPayload(error, config, helpCommand = null) { } return { - exitCode: 1, + exitCode: classified.exitCode, body: { ok: false, server_url: config?.serverUrl ?? null, error: { - code: "mcp_error", - message: safeRemoteString(error?.message ?? String(error), LOCAL_MESSAGE_LIMIT) ?? "Unknown error.", + code: classified.code, + message: localMessage(error?.message ?? String(error)) ?? "Unexpected 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. + * The complete set of `error.code` values the CLI can emit, with their exit codes. * - * 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. + * This object IS the contract. `classifyError` never returns a code outside it, + * `errorTelemetryCode` reports the same value, and the test suite asserts that + * docs/cli-reference.md documents exactly this set — so the three cannot drift apart. */ -function sanitizedRemoteError(responseText) { - const raw = typeof responseText === "string" ? responseText : ""; - if (!raw.trim()) { - return null; - } +export const ERROR_CODES = Object.freeze({ + invalid_arguments: { exitCode: 2, transport: false }, + auth_required: { exitCode: 1, transport: false }, + broker_unavailable: { exitCode: 1, transport: false }, + http_error: { exitCode: 1, transport: false }, + transport_error: { exitCode: 1, transport: true }, + mcp_error: { exitCode: 1, transport: false }, + plan_not_ready: { exitCode: 1, transport: false }, + plan_call_invalid_response: { exitCode: 1, transport: false }, + run_call_missing_run_id: { exitCode: 1, transport: false }, + recovery_not_found: { exitCode: 1, transport: false }, + recovery_storage_error: { exitCode: 1, transport: false }, + plan_call_error: { exitCode: 1, transport: false }, + plan_call_timeout: { exitCode: 1, transport: true }, + run_call_error: { exitCode: 1, transport: false }, + run_call_timeout: { exitCode: 1, transport: true }, + get_call_run_error: { exitCode: 1, transport: false }, + get_call_run_timeout: { exitCode: 1, transport: true }, + internal_error: { exitCode: 1, transport: false }, +}); - let parsed; - try { - parsed = JSON.parse(raw); - } catch { - parsed = undefined; +/** + * Single place that maps a thrown error to a CLI-owned code. Used by the JSON envelope, by + * stderr, and by telemetry, so all three always agree. + */ +export function classifyError(error) { + if (error instanceof InvalidArgumentsError) { + return { code: "invalid_arguments", exitCode: 2, transport: false }; } - - 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; + if (error instanceof AuthRequiredError || isUnauthorizedMcpError(error)) { + return { code: "auth_required", exitCode: 1, transport: false }; + } + if (error instanceof McpHttpError) { + const candidate = typeof error.code === "string" && Object.hasOwn(ERROR_CODES, error.code) + ? error.code + : "mcp_error"; + const transport = Boolean(error.transport || error.timedOut) || ERROR_CODES[candidate].transport; + return { code: candidate, exitCode: ERROR_CODES[candidate].exitCode, transport }; + } + if (error instanceof HttpStatusError) { + const code = isBrokerRegistrationFailure(error) ? "broker_unavailable" : "http_error"; + return { code, exitCode: 1, transport: false }; + } + if (error instanceof TransportError) { + return { code: "transport_error", exitCode: 1, transport: true }; } + // Anything else is a local defect. It is never described as a network condition. + return { code: "internal_error", exitCode: 1, transport: false }; +} - 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); +/** Bound and control-strip a message the CLI authored itself before it reaches an envelope. */ +function localMessage(value) { + return safeRemoteString(value, LOCAL_MESSAGE_LIMIT); +} - if (code === undefined && message === undefined) { - return null; +/** Origin and path of a URL we requested — ours to print, but never the query string. */ +function describeUrl(url) { + if (typeof url !== "string" || !url) { + return "the CALL-E service"; + } + try { + const parsed = new URL(url); + return `${parsed.origin}${parsed.pathname}`; + } catch { + return "the CALL-E service"; } - 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) { + if (Number(error?.statusCode) < 500) { return false; } - if (error instanceof TypeError) { - return true; - } - return /^Request timed out for /u.test(String(error?.message ?? "")); + const url = typeof error?.url === "string" ? error.url : String(error?.message ?? ""); + return /\/api\/v1\/openagent-auth\/sessions/u.test(url); } function writeCommandError(stdout, stderr, error, config, helpCommand = null) { @@ -1029,16 +1064,7 @@ function prePlanInvokedCommand(group, command) { } function errorTelemetryCode(error) { - if (error instanceof InvalidArgumentsError) { - return "invalid_arguments"; - } - if (error instanceof AuthRequiredError || isUnauthorizedMcpError(error)) { - return "auth_required"; - } - if (error instanceof McpHttpError) { - return error.code || "mcp_error"; - } - return "local_error"; + return classifyError(error).code; } function errorTelemetryProperties(error) { @@ -1133,40 +1159,8 @@ 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") { - 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; - } - const cleaned = stripTerminalControls(value).trim(); - return REMOTE_CODE_RE.test(cleaned) ? cleaned : undefined; -} - +// Remote strings are sanitized by @call-e/core/sanitize - one implementation shared with the +// MCP client, so a message is made safe where the error is created, not where it is printed. function safeRemoteCallError(result) { const structured = recordObject(structuredPayload(result)) || {}; const nestedError = recordObject(structured.error) || {}; @@ -1204,14 +1198,20 @@ function callStageErrorFrom(error, { if (error instanceof CallStageError) { return error; } - const timedOut = error instanceof McpHttpError && /timed out/iu.test(error.message); + // Only a genuine transport timeout (typed by the core client) becomes `_timeout`; + // matching on message text would let a remote string choose our error code. + const timedOut = error instanceof McpHttpError && error.timedOut === true; const remoteError = error instanceof McpHttpError && error.payload ? safeRemoteCallError(error.payload) : null; + // The summary is ours. The server's wording, if any, rides along under remote_error. + const message = timedOut + ? `${stage} timed out before the CLI received a response.` + : (error instanceof McpHttpError && error.transport + ? `${stage} failed before a response was received.` + : `${stage} failed.`); return new CallStageError( - timedOut - ? `${stage} timed out before the CLI received a response.` - : remoteError?.message || `${stage} failed: ${error?.message || String(error)}`, + message, { stage, code: timedOut ? `${stage}_timeout` : `${stage}_error`, @@ -1221,6 +1221,9 @@ function callStageErrorFrom(error, { recoveryId, nextCommand, remoteError, + transport: error instanceof McpHttpError ? error.transport : false, + timedOut, + ...(error?.cause !== undefined ? { cause: error.cause } : {}), } ); } @@ -1533,16 +1536,20 @@ async function handleCallCommand({ command, positional, options, config, deps, s }); const structuredPlan = structuredPayload(planResult); if (structuredPlan.ready_to_run === false) { - const question = Array.isArray(structuredPlan.clarifying_questions) - ? structuredPlan.clarifying_questions.find((item) => typeof item === "string" && item.trim())?.trim() + // The clarifying question is server text. It is shown under remote_error, sanitized + // and bounded, never interpolated into the CLI's own summary. + const rawQuestion = Array.isArray(structuredPlan.clarifying_questions) + ? structuredPlan.clarifying_questions.find((item) => typeof item === "string" && item.trim()) : null; + const question = safeRemoteString(rawQuestion, REMOTE_MESSAGE_LIMIT); throw new CallStageError( - `Call plan needs more information before it can run${question ? `: ${question}` : "."}`, + "Call plan needs more information before it can run. See error.remote_error.message for the question the service asked.", { stage: "plan_call", code: "plan_not_ready", callStarted: false, retrySafe: true, + ...(question ? { remoteError: { message: question } } : {}), } ); } diff --git a/packages/cli/lib/sanitize.js b/packages/cli/lib/sanitize.js new file mode 100644 index 0000000..65e0d9d --- /dev/null +++ b/packages/cli/lib/sanitize.js @@ -0,0 +1 @@ +export * from "@call-e/core/sanitize"; diff --git a/packages/cli/package.json b/packages/cli/package.json index 20f195f..81a6527 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -45,7 +45,7 @@ "test:e2e": "node --test ./test/e2e/*.test.js", "verify:live": "node ./scripts/live-e2e.mjs", "verify:live:call": "node ./scripts/live-e2e.mjs --call", - "check": "node ../../scripts/check-runtime-syntax.mjs bin/calle.js lib/broker-client.js lib/cache.js lib/cli.js lib/config.js lib/http.js lib/mcp-client.js lib/telemetry.js scripts/live-e2e.mjs", + "check": "node ../../scripts/check-runtime-syntax.mjs bin/calle.js lib/broker-client.js lib/cache.js lib/cli.js lib/config.js lib/http.js lib/mcp-client.js lib/sanitize.js lib/telemetry.js scripts/live-e2e.mjs", "pack:dry-run": "tmpdir=$(mktemp -d) && trap 'rm -rf \"$tmpdir\"' EXIT && pnpm pack --pack-destination \"$tmpdir\"" } } diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index b0acc71..35b160d 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -408,10 +408,15 @@ test("auth login surfaces the upstream error body when brokered login registrati 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/); + // The upstream wording is available, but only under remote_error. The summary and stderr + // are authored by the CLI. + assert.match(payload.error.remote_error.message, /Failed to register an OAuth client/); + assert.doesNotMatch(payload.error.message, /Failed to register an OAuth client/); + assert.match(payload.error.message, /^HTTP 502 from https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\./); 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/); + assert.doesNotMatch(result.stderr, /Failed to register an OAuth client/); + assert.match(result.stderr, /login service is unavailable/); }); test("auth login keeps a non-JSON upstream error body readable and bounded", async () => { @@ -548,9 +553,14 @@ test("auth login returns a transport_error envelope when fetch rejects", async ( assert.equal(result.code, 1); assert.equal(payload.ok, false); assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.transport, true); assert.equal(payload.error.cause_code, "ENOTFOUND"); assert.equal(payload.help_command, undefined); - assert.match(payload.error.message, /fetch failed/u); + // Locally authored: names our request and the system error code, never the runtime's text. + assert.match( + payload.error.message, + /^Request failed before a response was received for POST https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\. \(ENOTFOUND\)$/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); @@ -559,15 +569,213 @@ test("auth login returns a transport_error envelope when fetch rejects", async ( 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 aborted = new Error("aborted"); + aborted.name = "AbortError"; + throw aborted; }; 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.transport, true); + assert.equal(payload.error.cause_code, "timeout"); + assert.match(payload.error.message, /^Request timed out for POST https:\/\/mcp\.example\/api\/v1\/openagent-auth\/sessions\./u); +}); + +test("an unrelated local TypeError is internal_error, never transport_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-login-local-typeerror"); + const fetchImpl = async (url, init) => { + if (String(url).endsWith("/api/v1/openagent-auth/sessions") && init?.method === "POST") { + return jsonResponse( + { + session_id: "session-1", + session_secret: "secret-1", + login_url: "https://mcp.example/openagent-auth/sessions/session-1/start", + status: "PENDING", + poll_after_ms: 1, + expires_at: "2030-01-01T00:00:00Z", + }, + { status: 201 } + ); + } + if (String(url).endsWith("/api/v1/openagent-auth/sessions/session-1") && init?.method === "GET") { + return jsonResponse({ status: "PENDING", expires_at: "2030-01-01T00:00:00Z" }); + } + throw new Error(`unexpected request: ${init?.method} ${url}`); + }; + // A bug inside the CLI's own polling loop, not a network condition. + const sleepImpl = async () => { + throw new TypeError("Cannot read properties of undefined (reading 'x')"); + }; + + const result = await run( + ["auth", "login", "--no-browser-open", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl, sleepImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "internal_error"); + assert.equal(payload.error.transport, undefined); assert.equal(payload.error.cause_code, undefined); - assert.match(payload.error.message, /timed out/u); + assert.match(payload.error.message, /Cannot read properties/u); +}); + +function mcpFixture({ serverUrl, onToolsList, onToolsCall }) { + return async (url, init) => { + assert.equal(String(url), serverUrl); + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonRpcResponse({ jsonrpc: "2.0", id: payload.id, result: {} }, { headers: { "mcp-session-id": "sess-h" } }); + } + if (payload.method === "notifications/initialized") { + return jsonRpcResponse({}); + } + if (payload.method === "tools/list" && onToolsList) { + return onToolsList(payload); + } + if (payload.method === "tools/call" && onToolsCall) { + return onToolsCall(payload); + } + throw new Error(`unexpected method: ${payload.method}`); + }; +} + +const ESC_CHAR = String.fromCharCode(27); +const HOSTILE_REMOTE_TEXT = + `line one\r\ninjected${ESC_CHAR}[2J${ESC_CHAR}[H bearer abcdefghijklmnopqrstuvwxyz0123 ` + + `access_token=sk_live_ABCDEFGHIJKLMNOPQRST ${"z".repeat(20_000)}`; + +test("mcp tools keeps a hostile JSON-RPC error out of the summary and bounds it under remote_error", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-tools-hostile"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsList: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + error: { code: -32000, message: HOSTILE_REMOTE_TEXT, data: { refresh_token: "rt_SECRET_ABCDEFGH" } }, + }), + }); + + const result = await run(["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], { fetchImpl }); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "mcp_error"); + assert.equal(payload.error.message, "Remote MCP error for tools/list"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.equal(payload.error.remote_error.code, "-32000"); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + for (const secret of ["sk_live_", "abcdefghijklmnopqrstuvwxyz0123", "rt_SECRET", "refresh_token"]) { + assert.doesNotMatch(result.stdout, new RegExp(secret)); + assert.doesNotMatch(result.stderr, new RegExp(secret)); + } + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); + assert.ok(result.stderr.length < 300); +}); + +test("mcp call applies the same boundary to a tool-call error", async () => { + const cacheRoot = makeTempRoot("calle-cli-mcp-call-hostile"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsCall: (payload) => jsonRpcResponse({ jsonrpc: "2.0", id: payload.id, error: { code: -32601, message: HOSTILE_REMOTE_TEXT } }), + }); + + const result = await run( + ["mcp", "call", "get_call_run", "--args-json", '{"run_id":"run_1"}', "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.error.code, "mcp_error"); + assert.equal(payload.error.message, "Remote MCP error for tools/call"); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.doesNotMatch(result.stdout, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr.trimEnd(), CONTROL_CHARS); +}); + +test("call start keeps a hostile clarifying question out of the plan_not_ready summary", async () => { + const cacheRoot = makeTempRoot("calle-cli-call-start-hostile-question"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(cacheRoot, serverUrl, "tool-token"); + const fetchImpl = mcpFixture({ + serverUrl, + onToolsCall: (payload) => jsonRpcResponse({ + jsonrpc: "2.0", + id: payload.id, + result: { structuredContent: { ready_to_run: false, clarifying_questions: [HOSTILE_REMOTE_TEXT] } }, + }), + }); + + const result = await run( + ["call", "start", "--to-phone", "+15551234567", "--goal", "Confirm appointment", "--base-url", "https://mcp.example", "--cache-root", cacheRoot], + { fetchImpl } + ); + const payload = JSON.parse(result.stdout); + + assert.equal(result.code, 1); + assert.equal(payload.stage, "plan_call"); + assert.equal(payload.error.code, "plan_not_ready"); + assert.match(payload.error.message, /^Call plan needs more information before it can run\./u); + assert.doesNotMatch(payload.error.message, /injected|zzzz/u); + assert.ok(payload.error.remote_error.message.length <= 500); + assert.match(payload.error.remote_error.message, /^line one\s+injected/u); + assert.doesNotMatch(payload.error.remote_error.message, CONTROL_CHARS); + assert.doesNotMatch(result.stdout, /sk_live_|abcdefghijklmnopqrstuvwxyz0123/u); + assert.doesNotMatch(result.stderr, /injected|sk_live_/u); +}); + +test("telemetry reports the same error code as the envelope for broker and transport failures", async () => { + const brokerRoot = makeTempRoot("calle-cli-telemetry-broker"); + const brokerEvents = []; + await run( + [...LOGIN_ARGS, "--cache-root", brokerRoot], + { + fetchImpl: brokerFailure(502, { error: "oauth_register_failed", message: "x" }), + env: { CALLE_TELEMETRY: "1" }, + telemetryFetchImpl: captureTelemetry(brokerEvents), + } + ); + const brokerFailed = brokerEvents.find((event) => event.payload.event === "auth_login_local_failed"); + assert.ok(brokerFailed, "auth_login_local_failed telemetry was emitted"); + assert.equal(brokerFailed.payload.properties.error_code, "broker_unavailable"); + + const mcpRoot = makeTempRoot("calle-cli-telemetry-transport"); + const serverUrl = "https://mcp.example/mcp/openagent_oauth"; + writeToken(mcpRoot, serverUrl, "tool-token"); + const mcpEvents = []; + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ECONNREFUSED" }; + const result = await run( + ["mcp", "tools", "--base-url", "https://mcp.example", "--cache-root", mcpRoot], + { fetchImpl: async () => { throw dns; }, env: { CALLE_TELEMETRY: "1" }, telemetryFetchImpl: captureTelemetry(mcpEvents) } + ); + const payload = JSON.parse(result.stdout); + assert.equal(payload.error.code, "transport_error"); + assert.equal(payload.error.cause_code, "ECONNREFUSED"); + const checked = mcpEvents.find((event) => event.payload.event === "mcp_tools_checked" && event.payload.properties.outcome === "failure"); + assert.ok(checked, "mcp_tools_checked failure telemetry was emitted"); + assert.equal(checked.payload.properties.error_code, "transport_error"); +}); + +test("every error code the CLI can emit is documented, and nothing undocumented is emitted", async () => { + const { ERROR_CODES } = await import("../lib/cli.js"); + const reference = fs.readFileSync(new URL("../docs/cli-reference.md", import.meta.url), "utf8"); + const section = reference.split("## Error Envelopes")[1]?.split(/\n## /u)[0] ?? ""; + const documented = new Set([...section.matchAll(/^\| `([a-z_]+)` \| \d /gmu)].map((m) => m[1])); + const emitted = new Set(Object.keys(ERROR_CODES)); + + assert.deepEqual([...documented].sort(), [...emitted].sort()); + for (const [code, meta] of Object.entries(ERROR_CODES)) { + assert.match(section, new RegExp(`^\\| \`${code}\` \\| ${meta.exitCode} `, "mu"), `exit code documented for ${code}`); + } }); test("auth login start-only replaces locally active pending cache when broker reports it expired", async () => { @@ -1684,8 +1892,9 @@ test("call start reports plan clarification and skips run_call when planning is assert.equal(payload.error.code, "plan_not_ready"); assert.equal( payload.error.message, - "Call plan needs more information before it can run: What should the agent ask or say on the call?" + "Call plan needs more information before it can run. See error.remote_error.message for the question the service asked." ); + assert.equal(payload.error.remote_error.message, "What should the agent ask or say on the call?"); }); test("call start rejects a null structured confirm token without calling run_call", async () => { diff --git a/packages/core/lib/http.d.ts b/packages/core/lib/http.d.ts index 4a40fca..93ad520 100644 --- a/packages/core/lib/http.d.ts +++ b/packages/core/lib/http.d.ts @@ -4,6 +4,14 @@ export interface HttpStatusErrorOptions { statusCode?: number | null; responseText?: string; headers?: Record; + url?: string | null; +} + +export interface TransportErrorOptions { + url?: string | null; + method?: string | null; + timedOut?: boolean; + cause?: unknown; } export interface RequestJsonOptions { @@ -18,6 +26,20 @@ export class HttpStatusError extends Error { statusCode: number | null; responseText: string; headers: Record; + url: string | null; +} + +/** The Node.js system error code behind a failed fetch (`ENOTFOUND`, `ECONNREFUSED`, ...), or null. */ +export function causeCodeOf(error: unknown): string | null; + +/** The request never received an HTTP response (DNS, connection, TLS, or timeout). */ +export class TransportError extends Error { + constructor(message: string, options?: TransportErrorOptions); + url: string | null; + method: string | null; + timedOut: boolean; + /** "timeout", or the Node.js error code of the cause (e.g. "ENOTFOUND"), or null. */ + code: string | null; } export function requestJson( diff --git a/packages/core/lib/http.js b/packages/core/lib/http.js index bca9492..bfe14da 100644 --- a/packages/core/lib/http.js +++ b/packages/core/lib/http.js @@ -1,10 +1,41 @@ export class HttpStatusError extends Error { - constructor(message, { statusCode, responseText, headers } = {}) { + constructor(message, { statusCode, responseText, headers, url } = {}) { super(message); this.name = "HttpStatusError"; this.statusCode = statusCode ?? null; this.responseText = responseText ?? ""; this.headers = headers ?? {}; + this.url = url ?? null; + } +} + +/** + * The Node.js system error code behind a failed fetch, if any. `fetch` rejects with + * `TypeError: fetch failed` whose `cause` is the system error (`ENOTFOUND`, `ECONNREFUSED`, + * `CERT_HAS_EXPIRED`, ...), so the code may sit one or two levels down. + */ +export function causeCodeOf(error) { + for (let cursor = error, depth = 0; cursor && depth < 4; cursor = cursor.cause, depth++) { + if (typeof cursor.code === "string" && cursor.code) { + return cursor.code; + } + } + return null; +} + +/** + * The request never received an HTTP response: DNS failure, connection refused, TLS error, + * or the client-side timeout. This is the only condition that may be described to a caller as + * a network problem. An unrelated local exception must not be classified as transport. + */ +export class TransportError extends Error { + constructor(message, { url, method, timedOut = false, cause } = {}) { + super(message, cause !== undefined ? { cause } : undefined); + this.name = "TransportError"; + this.url = url ?? null; + this.method = method ?? null; + this.timedOut = Boolean(timedOut); + this.code = timedOut ? "timeout" : causeCodeOf(cause); } } @@ -20,8 +51,9 @@ export async function requestJson(method, url, { headers = {}, json = undefined, timeout.unref(); } + let response; try { - const response = await fetchImpl(url, { + response = await fetchImpl(url, { method, headers: { Accept: "application/json", @@ -31,12 +63,26 @@ export async function requestJson(method, url, { headers = {}, json = undefined, body: json !== undefined ? JSON.stringify(json) : undefined, signal: controller.signal, }); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new TransportError(`Request timed out for ${method} ${url}`, { url, method, timedOut: true }); + } + throw new TransportError(`Request failed before a response was received for ${method} ${url}`, { + url, + method, + cause: error, + }); + } + + try { const text = await response.text(); if (!response.ok) { throw new HttpStatusError(`Client error '${response.status} ${response.statusText}' for url '${url}'`, { statusCode: response.status, responseText: text, headers: Object.fromEntries(response.headers.entries()), + url, }); } if (!text.trim()) { @@ -47,11 +93,6 @@ export async function requestJson(method, url, { headers = {}, json = undefined, throw new Error(`Expected JSON object response for ${method} ${url}`); } return parsed; - } catch (error) { - if (error?.name === "AbortError") { - throw new Error(`Request timed out for ${method} ${url}`); - } - throw error; } finally { clearTimeout(timeout); } diff --git a/packages/core/lib/index.d.ts b/packages/core/lib/index.d.ts index dd6b575..a2fbe84 100644 --- a/packages/core/lib/index.d.ts +++ b/packages/core/lib/index.d.ts @@ -4,3 +4,4 @@ export * from "./config.js"; export * from "./constants.js"; export * from "./http.js"; export * from "./mcp-client.js"; +export * from "./sanitize.js"; diff --git a/packages/core/lib/index.js b/packages/core/lib/index.js index dd6b575..a2fbe84 100644 --- a/packages/core/lib/index.js +++ b/packages/core/lib/index.js @@ -4,3 +4,4 @@ export * from "./config.js"; export * from "./constants.js"; export * from "./http.js"; export * from "./mcp-client.js"; +export * from "./sanitize.js"; diff --git a/packages/core/lib/mcp-client.d.ts b/packages/core/lib/mcp-client.d.ts index e910935..84aa4c8 100644 --- a/packages/core/lib/mcp-client.d.ts +++ b/packages/core/lib/mcp-client.d.ts @@ -17,6 +17,9 @@ export interface McpHttpErrorOptions { payload?: unknown; headers?: Record; code?: string; + transport?: boolean; + timedOut?: boolean; + cause?: unknown; } export interface McpToolDefinition extends JsonObject { @@ -52,6 +55,13 @@ export class McpHttpError extends Error { payload: unknown; headers: Record; code: string; + /** True only when no HTTP response was received (timeout, DNS, connection, TLS). */ + transport: boolean; + timedOut: boolean; + /** "timeout", the system error code behind a rejected fetch (e.g. "ENOTFOUND"), or null. */ + causeCode: string | null; + /** Sanitized, bounded `{ code?, message? }` from the remote body, or null. Safe to display. */ + remoteError: { code?: string; message?: string } | null; } export function isUnauthorizedMcpError(error: unknown): error is McpHttpError; diff --git a/packages/core/lib/mcp-client.js b/packages/core/lib/mcp-client.js index 2c412a7..2d4d7ac 100644 --- a/packages/core/lib/mcp-client.js +++ b/packages/core/lib/mcp-client.js @@ -5,6 +5,8 @@ import { INTEGRATION_HEADER, MCP_PROTOCOL_VERSION, } from "./constants.js"; +import { causeCodeOf } from "./http.js"; +import { sanitizeRemoteError } from "./sanitize.js"; export class AuthRequiredError extends Error { constructor(message = "A usable CALL-E auth token is required.") { @@ -13,15 +15,34 @@ export class AuthRequiredError extends Error { } } +/** + * `message` is always authored locally and safe to print. Whatever the server said is kept + * raw in `payload` / `responseText` for programmatic use, and in sanitized, bounded form in + * `remoteError` for display. Nothing remote reaches `message`. + */ export class McpHttpError extends Error { - constructor(message, { statusCode = null, responseText = "", payload = null, headers = {}, code = "http_error" } = {}) { - super(message); + constructor(message, { + statusCode = null, + responseText = "", + payload = null, + headers = {}, + code = "http_error", + transport = false, + timedOut = false, + cause, + } = {}) { + super(message, cause !== undefined ? { cause } : undefined); this.name = "McpHttpError"; this.statusCode = statusCode; this.responseText = responseText; this.payload = payload; this.headers = headers; this.code = code; + this.transport = Boolean(transport); + this.timedOut = Boolean(timedOut); + /** "timeout", the system error code behind a rejected fetch, or null. */ + this.causeCode = timedOut ? "timeout" : causeCodeOf(cause); + this.remoteError = sanitizeRemoteError(payload ?? responseText); } } @@ -57,13 +78,32 @@ async function requestJsonRpc(fetchImpl, url, { headers, payload, timeoutMs }) { timeout.unref(); } + let response; try { - const response = await fetchImpl(url, { + response = await fetchImpl(url, { method: "POST", headers, body: JSON.stringify(payload), signal: controller.signal, }); + } catch (error) { + clearTimeout(timeout); + if (error?.name === "AbortError") { + throw new McpHttpError(`MCP request timed out for ${payload.method}`, { + code: "transport_error", + transport: true, + timedOut: true, + }); + } + // fetch rejected before any response: DNS, connection, TLS. Only this path is transport. + throw new McpHttpError(`MCP request failed before a response was received for ${payload.method}`, { + code: "transport_error", + transport: true, + cause: error, + }); + } + + try { const text = await response.text(); let body = null; try { @@ -83,20 +123,16 @@ async function requestJsonRpc(fetchImpl, url, { headers, payload, timeoutMs }) { } if (body?.error) { - const error = body.error; - throw new McpHttpError(error.message || `Remote MCP error for ${payload.method}`, { - payload: error, + // The server's message is untrusted: it is kept in `payload` and, sanitized, in + // `remoteError`. The Error message itself stays locally authored. + throw new McpHttpError(`Remote MCP error for ${payload.method}`, { + payload: body.error, headers: responseHeaders, code: "mcp_error", }); } return { body, headers: responseHeaders }; - } catch (error) { - if (error?.name === "AbortError") { - throw new McpHttpError(`MCP request timed out for ${payload.method}`, { code: "http_error" }); - } - throw error; } finally { clearTimeout(timeout); } diff --git a/packages/core/lib/sanitize.d.ts b/packages/core/lib/sanitize.d.ts new file mode 100644 index 0000000..1d287ea --- /dev/null +++ b/packages/core/lib/sanitize.d.ts @@ -0,0 +1,13 @@ +export const REMOTE_MESSAGE_LIMIT: number; +export const REMOTE_CODE_LIMIT: number; + +export interface SanitizedRemoteError { + code?: string; + message?: string; +} + +export function stripTerminalControls(value: unknown): string; +export function redactSecrets(value: unknown): string; +export function safeRemoteString(value: unknown, maxLength?: number): string | undefined; +export function safeRemoteCode(value: unknown): string | undefined; +export function sanitizeRemoteError(body: unknown): SanitizedRemoteError | null; diff --git a/packages/core/lib/sanitize.js b/packages/core/lib/sanitize.js new file mode 100644 index 0000000..e86aa38 --- /dev/null +++ b/packages/core/lib/sanitize.js @@ -0,0 +1,139 @@ +/** + * One boundary for every remote-supplied string. + * + * Anything that arrives from the network — an MCP JSON-RPC error, an upstream HTTP body, a + * clarifying question inside a tool result — is untrusted. Before it can appear in a JSON + * envelope, a log line, or a terminal, it passes through here: terminal control sequences are + * removed, token-like material is redacted, and the length is bounded. Both the core library + * and the CLI import these helpers so there is exactly one implementation to review. + */ + +const ESC = String.fromCharCode(0x1b); +const BEL = String.fromCharCode(0x07); +const BACKSLASH = String.fromCharCode(0x5c); +const C0_START = String.fromCharCode(0x00); +const C0_END = String.fromCharCode(0x1f); +const DEL = String.fromCharCode(0x7f); +const C1_END = String.fromCharCode(0x9f); + +// CSI (colours, cursor movement, erase), OSC (titles, hyperlinks, terminated by BEL or ESC \), +// two-character ESC sequences, and the C0 / DEL / C1 control ranges (covers CR, LF, TAB). +// The regex source is assembled from character codes so the file itself contains no control +// bytes and no escape sequence that a tool or editor could rewrite. +const LBRACKET = `${BACKSLASH}[`; +const RBRACKET = `${BACKSLASH}]`; +const TERMINAL_CONTROL_RE = new RegExp( + [ + `${ESC}${LBRACKET}[0-?]*[ -/]*[@-~]`, + `${ESC}${RBRACKET}[^${BEL}${ESC}]*(?:${BEL}|${ESC}${BACKSLASH}${BACKSLASH})`, + `${ESC}[@-_]`, + `[${C0_START}-${C0_END}${DEL}-${C1_END}]`, + ].join("|"), + "gu", +); + +export const REMOTE_MESSAGE_LIMIT = 500; +export const REMOTE_CODE_LIMIT = 64; + +const REMOTE_CODE_RE = /^[A-Za-z0-9][A-Za-z0-9_.:-]{0,63}$/u; + +const REDACTION = "[redacted]"; + +// Token-like material that may appear inside an otherwise-allowlisted message string. +// Each pattern is deliberately broad: a false redaction costs a little readability, a missed +// secret ends up in an agent transcript. +const SECRET_PATTERNS = [ + // "Bearer abc..." / "Basic abc..." + /\b(bearer|basic)\s+[A-Za-z0-9._~+/=-]{8,}/giu, + // key=value / key: value / "key": "value" for sensitive key names + /\b(access[_-]?token|refresh[_-]?token|id[_-]?token|token|secret|password|passwd|api[_-]?key|apikey|authorization|cookie|session[_-]?secret|private[_-]?key|client[_-]?secret)\b(\s*["']?\s*[:=]\s*["']?)[^\s"',;)}\]]{4,}/giu, + // Well-known prefixed credentials + /\b(?:sk|pk|rk|tok|rt|xox[abpr]|ghp|gho|ghu|ghs|AKIA|iams|calle)[_-][A-Za-z0-9_-]{12,}/gu, + // Long opaque runs: hex, base64url, uuid-ish + /\b[A-Fa-f0-9]{32,}\b/gu, + /\b[A-Za-z0-9_-]{40,}\b/gu, +]; + +/** Replace terminal control sequences with a single space. Never throws. */ +export function stripTerminalControls(value) { + return String(value ?? "").replace(TERMINAL_CONTROL_RE, " "); +} + +/** Redact credential-shaped substrings. Never throws. */ +export function redactSecrets(value) { + let out = String(value ?? ""); + out = out.replace(SECRET_PATTERNS[0], (_m, scheme) => `${scheme} ${REDACTION}`); + out = out.replace(SECRET_PATTERNS[1], (_m, key, sep) => `${key}${sep}${REDACTION}`); + for (const pattern of SECRET_PATTERNS.slice(2)) { + out = out.replace(pattern, REDACTION); + } + return out; +} + +/** + * A remote string made safe for display: controls stripped, secrets redacted, whitespace + * trimmed, length bounded. Returns undefined for non-strings and empty results so callers can + * omit the field rather than emit an empty one. + */ +export function safeRemoteString(value, maxLength = REMOTE_MESSAGE_LIMIT) { + if (typeof value !== "string") { + return undefined; + } + const cleaned = redactSecrets(stripTerminalControls(value)).trim(); + if (!cleaned) { + return undefined; + } + return cleaned.slice(0, maxLength); +} + +/** + * A remote machine code kept as an opaque token. Anything outside the safe character set is + * dropped rather than "cleaned" into something that merely looks valid. + */ +export function safeRemoteCode(value) { + if (typeof value !== "string") { + return undefined; + } + const cleaned = stripTerminalControls(value).trim(); + return REMOTE_CODE_RE.test(cleaned) ? cleaned : undefined; +} + +/** + * Reduce an arbitrary remote error body — a JSON-RPC error object, an HTTP body, a tool + * result — to at most `{ code, message }`. Reads only `code` (or a string `error`) and + * `message`, at the top level or nested under `error`; everything else is dropped unread. + */ +export function sanitizeRemoteError(body) { + let value = body; + if (typeof value === "string") { + const text = value.trim(); + if (!text) { + return null; + } + try { + value = JSON.parse(text); + } catch { + const message = safeRemoteString(text); + return message ? { message } : null; + } + } + + if (!value || typeof value !== "object" || Array.isArray(value)) { + // JSON scalar or array: keep a bounded excerpt of its serialisation, nothing else. + const message = safeRemoteString(typeof value === "string" ? value : JSON.stringify(value ?? "")); + return message ? { message } : null; + } + + const nested = value.error && typeof value.error === "object" && !Array.isArray(value.error) ? value.error : {}; + const codeValue = nested.code ?? value.code ?? (typeof value.error === "string" ? value.error : undefined); + const code = typeof codeValue === "number" ? String(codeValue) : safeRemoteCode(codeValue); + const message = safeRemoteString(nested.message ?? value.message); + + if (code === undefined && message === undefined) { + return null; + } + return { + ...(code !== undefined ? { code } : {}), + ...(message !== undefined ? { message } : {}), + }; +} diff --git a/packages/core/package.json b/packages/core/package.json index cb79d04..e6ede49 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -24,7 +24,8 @@ "./config": "./lib/config.js", "./constants": "./lib/constants.js", "./http": "./lib/http.js", - "./mcp-client": "./lib/mcp-client.js" + "./mcp-client": "./lib/mcp-client.js", + "./sanitize": "./lib/sanitize.js" }, "files": [ "README.md", @@ -42,7 +43,7 @@ }, "scripts": { "test": "node --test ./test/*.test.js", - "check": "node ../../scripts/check-runtime-syntax.mjs lib/broker-client.js lib/cache.js lib/config.js lib/constants.js lib/http.js lib/index.js lib/mcp-client.js && pnpm run check:types", + "check": "node ../../scripts/check-runtime-syntax.mjs lib/broker-client.js lib/cache.js lib/config.js lib/constants.js lib/http.js lib/index.js lib/mcp-client.js lib/sanitize.js && pnpm run check:types", "check:types": "tsc --noEmit --strict --module NodeNext --moduleResolution NodeNext --target ES2022 --skipLibCheck test/types.ts", "pack:dry-run": "npm pack --dry-run" } diff --git a/packages/core/test/core.test.js b/packages/core/test/core.test.js index 6fccdb9..761309f 100644 --- a/packages/core/test/core.test.js +++ b/packages/core/test/core.test.js @@ -547,9 +547,92 @@ test("MCP client reports request timeouts", async () => { () => listMcpTools({ config, fetchImpl }), (error) => { assert.ok(error instanceof McpHttpError); - assert.equal(error.code, "http_error"); + assert.equal(error.code, "transport_error"); + assert.equal(error.transport, true); + assert.equal(error.timedOut, true); assert.match(error.message, /timed out/i); return true; }, ); }); + +test("MCP client classifies a rejected fetch as transport, and keeps the server message out of Error.message", async () => { + const config = mcpConfig(makeTempRoot("calle-core-mcp-rejected")); + const dns = new TypeError("fetch failed"); + dns.cause = { code: "ENOTFOUND" }; + await assert.rejects( + () => listMcpTools({ config, fetchImpl: async () => { throw dns; } }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "transport_error"); + assert.equal(error.transport, true); + assert.equal(error.timedOut, false); + assert.equal(error.causeCode, "ENOTFOUND"); + return true; + }, + ); + + const hostileConfig = mcpConfig(makeTempRoot("calle-core-mcp-hostile")); + const ESC = String.fromCharCode(27); + const hostileMessage = `${"x".repeat(2000)}${ESC}[31m secret=sk_live_ABCDEFGHIJKLMNOPQRSTUV`; + const fetchImpl = async (_url, init) => { + const payload = JSON.parse(init.body); + if (payload.method === "initialize") { + return jsonResponse({ result: {} }, { headers: { "mcp-session-id": "mcp-session-9" } }); + } + if (payload.method === "notifications/initialized") { + return jsonResponse({}); + } + return jsonResponse({ error: { code: -32000, message: hostileMessage, access_token: "tok_SECRET_VALUE_123456" } }); + }; + await assert.rejects( + () => callMcpTool({ config: hostileConfig, toolName: "plan_call", fetchImpl }), + (error) => { + assert.ok(error instanceof McpHttpError); + assert.equal(error.code, "mcp_error"); + assert.equal(error.message, "Remote MCP error for tools/call", "Error.message is authored locally"); + assert.ok(error.remoteError.message.length <= 500); + assert.doesNotMatch(error.remoteError.message, /sk_live_|tok_SECRET/u); + assert.equal(error.remoteError.message.includes(ESC), false); + assert.equal(error.remoteError.code, "-32000"); + return true; + }, + ); +}); + +test("sanitize helpers strip terminal controls, redact secrets, and bound length", async () => { + const { safeRemoteString, safeRemoteCode, redactSecrets, sanitizeRemoteError } = await import("@call-e/core/sanitize"); + const ESC = String.fromCharCode(27); + const BEL = String.fromCharCode(7); + const cleaned = safeRemoteString(`a${ESC}[2J${ESC}]8;;http://x${BEL}link${ESC}]8;;${BEL}b\r\nc`); + assert.equal(cleaned.includes(ESC), false); + assert.doesNotMatch(cleaned, /[\r\n]/u); + assert.match(cleaned, /^a\s+link\s*b\s+c$/u); + + // A bound on ordinary prose. (An unbroken 10,000-character run is redacted as an opaque + // token instead, which is the intended behaviour and is asserted below.) + assert.equal(safeRemoteString("word ".repeat(3000)).length, 500); + assert.equal(safeRemoteString("x".repeat(10_000)), "[redacted]"); + assert.equal(safeRemoteString(" "), undefined); + assert.equal(safeRemoteString(42), undefined); + + const bearer = redactSecrets("Authorization: Bearer abcdefghijklmnopqrstuvwxyz"); + assert.doesNotMatch(bearer, /abcdefghijklmnopqrstuvwxyz/u); + assert.match(bearer, /^Authorization: .*\[redacted\]/u); + assert.match(redactSecrets("Bearer abcdefghijklmnopqrstuvwxyz"), /^Bearer \[redacted\]$/u); + assert.match(redactSecrets("access_token=abcd1234efgh"), /access_token=\[redacted\]/u); + assert.match(redactSecrets("key sk_live_ABCDEFGHIJKLMNOP1234 here"), /key \[redacted\] here/u); + assert.match(redactSecrets("hash 0123456789abcdef0123456789abcdef0123"), /hash \[redacted\]/u); + assert.equal(redactSecrets("Failed to register an OAuth client. err_type=HTTPStatusError"), "Failed to register an OAuth client. err_type=HTTPStatusError"); + + assert.equal(safeRemoteCode("oauth_register_failed"), "oauth_register_failed"); + assert.equal(safeRemoteCode("nested.code-1"), "nested.code-1"); + assert.equal(safeRemoteCode(`bad code${ESC}[31m`), undefined); + assert.equal(safeRemoteCode("x".repeat(65)), undefined); + + assert.deepEqual(sanitizeRemoteError({ error: "auth_required", message: "please" }), { code: "auth_required", message: "please" }); + assert.deepEqual(sanitizeRemoteError({ error: { code: "n.1", message: "m", details: { internal: "t" } }, request_id: "r" }), { code: "n.1", message: "m" }); + assert.deepEqual(sanitizeRemoteError("gateway"), { message: "gateway" }); + assert.equal(sanitizeRemoteError(""), null); + assert.equal(sanitizeRemoteError({ unrelated: true }), null); +});