From c557c25e144640c7e913a10bd8fe1afcae49aa5c Mon Sep 17 00:00:00 2001 From: fforres Date: Thu, 29 Oct 2026 19:19:46 -0700 Subject: [PATCH] fix(mcp): classify opaque execute failures and log their cause --- .../hosts/mcp/src/failure-category.test.ts | 103 ++++++++++++++++++ packages/hosts/mcp/src/failure-category.ts | 81 ++++++++++++++ packages/hosts/mcp/src/tool-server.test.ts | 26 +++++ packages/hosts/mcp/src/tool-server.ts | 16 ++- 4 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 packages/hosts/mcp/src/failure-category.test.ts create mode 100644 packages/hosts/mcp/src/failure-category.ts diff --git a/packages/hosts/mcp/src/failure-category.test.ts b/packages/hosts/mcp/src/failure-category.test.ts new file mode 100644 index 0000000000..578107cabe --- /dev/null +++ b/packages/hosts/mcp/src/failure-category.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Cause, Data } from "effect"; + +import { classifyFailure, describeFailure } from "./failure-category"; + +class WrappedError extends Data.TaggedError("WrappedError")<{ + readonly message: string; + readonly cause: unknown; +}> {} +class OtherError extends Data.TaggedError("OtherError")<{ readonly message: string }> {} +class RequestError extends Data.TaggedError("RequestError")<{ readonly message: string }> {} +class ResponseError extends Data.TaggedError("ResponseError")<{ + readonly message: string; + readonly status: number; +}> {} +class TimeoutException extends Data.TaggedError("TimeoutException")<{ + readonly message: string; +}> {} +class CredentialResolutionError extends Data.TaggedError("CredentialResolutionError")<{ + readonly message: string; +}> {} + +describe("classifyFailure", () => { + it("classifies a timeout defect as retryable", () => { + expect(classifyFailure(Cause.die(new TimeoutException({ message: "x" })))).toEqual({ + category: "upstream_timeout", + retryable: true, + }); + }); + + it("classifies an AbortError defect as a timeout", () => { + const abort = new DOMException("The operation was aborted", "AbortError"); + expect(classifyFailure(Cause.die(abort))).toEqual({ + category: "upstream_timeout", + retryable: true, + }); + }); + + it("classifies a transport failure as retryable network failure", () => { + expect(classifyFailure(Cause.fail(new RequestError({ message: "x" })))).toEqual({ + category: "upstream_network", + retryable: true, + }); + }); + + it("finds the category through a wrapped cause chain", () => { + const wrapped = new WrappedError({ + message: "wrapper", + cause: new TimeoutException({ message: "inner" }), + }); + expect(classifyFailure(Cause.die(wrapped)).category).toBe("upstream_timeout"); + }); + + it("marks 5xx and 429 as retryable and 4xx as not", () => { + expect(classifyFailure(Cause.fail(new ResponseError({ message: "x", status: 503 })))).toEqual({ + category: "upstream_http", + retryable: true, + status: 503, + }); + expect(classifyFailure(Cause.fail(new ResponseError({ message: "x", status: 429 })))).toEqual({ + category: "upstream_http", + retryable: true, + status: 429, + }); + expect(classifyFailure(Cause.fail(new ResponseError({ message: "x", status: 404 })))).toEqual({ + category: "upstream_http", + retryable: false, + status: 404, + }); + }); + + it("classifies credential failures as auth and not retryable", () => { + expect(classifyFailure(Cause.fail(new CredentialResolutionError({ message: "x" })))).toEqual({ + category: "auth", + retryable: false, + }); + }); + + it("falls back to internal for anything else", () => { + expect(classifyFailure(Cause.die(new OtherError({ message: "boom" })))).toEqual({ + category: "internal", + retryable: false, + }); + }); +}); + +describe("describeFailure", () => { + it("returns an empty suffix for internal failures", () => { + expect(describeFailure({ category: "internal", retryable: false })).toBe(""); + }); + + it("names the tool and signals retry for a timeout", () => { + expect( + describeFailure({ category: "upstream_timeout", retryable: true }, "slack.org.main.history"), + ).toBe(": upstream timeout calling slack.org.main.history. Retry may succeed."); + }); + + it("includes the status and omits the retry hint when not retryable", () => { + expect(describeFailure({ category: "upstream_http", retryable: false, status: 404 })).toBe( + ": upstream returned HTTP 404.", + ); + }); +}); diff --git a/packages/hosts/mcp/src/failure-category.ts b/packages/hosts/mcp/src/failure-category.ts new file mode 100644 index 0000000000..94219655ac --- /dev/null +++ b/packages/hosts/mcp/src/failure-category.ts @@ -0,0 +1,81 @@ +import { Predicate } from "effect"; +import * as Cause from "effect/Cause"; + +export type FailureCategory = + | "upstream_timeout" + | "upstream_network" + | "upstream_http" + | "auth" + | "internal"; + +export interface FailureClassification { + readonly category: FailureCategory; + readonly retryable: boolean; + readonly status?: number; +} + +const MAX_CAUSE_DEPTH = 6; +const TIMEOUT_NAMES = new Set(["TimeoutError", "TimeoutException", "AbortError"]); +const NETWORK_TAGS = new Set(["RequestError", "TransportError", "FetchError"]); +const RESPONSE_TAGS = new Set(["ResponseError", "StatusCodeError"]); +const AUTH_TAGS = new Set(["CredentialResolutionError", "OAuthRefreshError"]); + +const isRetryableStatus = (status: number): boolean => status === 429 || status >= 500; + +const hasName = (value: object, names: ReadonlySet): boolean => { + const name = (value as { readonly name?: unknown }).name; + return typeof name === "string" && names.has(name); +}; + +const isOneOf = (value: object, tags: ReadonlySet): boolean => + hasName(value, tags) || [...tags].some((tag) => Predicate.isTagged(value, tag)); + +const statusOf = (value: unknown): number | undefined => { + if (typeof value !== "object" || value === null) return undefined; + const status = (value as { readonly status?: unknown }).status; + return typeof status === "number" && status >= 400 && status < 600 ? status : undefined; +}; + +const classifyValue = (value: unknown, depth: number): FailureClassification | undefined => { + if (depth > MAX_CAUSE_DEPTH || typeof value !== "object" || value === null) return undefined; + if (isOneOf(value, TIMEOUT_NAMES)) return { category: "upstream_timeout", retryable: true }; + if (isOneOf(value, NETWORK_TAGS)) return { category: "upstream_network", retryable: true }; + if (isOneOf(value, AUTH_TAGS)) return { category: "auth", retryable: false }; + const status = statusOf(value); + if (isOneOf(value, RESPONSE_TAGS) && status !== undefined) { + return { category: "upstream_http", retryable: isRetryableStatus(status), status }; + } + return classifyValue((value as { readonly cause?: unknown }).cause, depth + 1); +}; + +/** Reads only error tags, names and numeric status codes, never messages, so + * the result is safe to show to the sandbox. */ +export const classifyFailure = (cause: Cause.Cause): FailureClassification => { + for (const reason of cause.reasons) { + const value = Cause.isFailReason(reason) + ? reason.error + : Cause.isDieReason(reason) + ? reason.defect + : undefined; + const found = classifyValue(value, 0); + if (found) return found; + } + return { category: "internal", retryable: false }; +}; + +const SUMMARY: Record, string> = { + upstream_timeout: "upstream timeout", + upstream_network: "upstream network failure", + auth: "authentication failure, reconnect the integration", +}; + +/** Text appended after `Internal tool error [id]`; empty when nothing safe is known. */ +export const describeFailure = (failure: FailureClassification, toolPath?: string): string => { + if (failure.category === "internal") return ""; + const target = toolPath === undefined ? "" : ` calling ${toolPath}`; + const summary = + failure.category === "upstream_http" + ? `upstream returned HTTP ${failure.status}${target}` + : `${SUMMARY[failure.category]}${target}`; + return `: ${summary}.${failure.retryable ? " Retry may succeed." : ""}`; +}; diff --git a/packages/hosts/mcp/src/tool-server.test.ts b/packages/hosts/mcp/src/tool-server.test.ts index 9efba2049b..778e4aa66e 100644 --- a/packages/hosts/mcp/src/tool-server.test.ts +++ b/packages/hosts/mcp/src/tool-server.test.ts @@ -31,6 +31,10 @@ class TestExecutionError extends Data.TaggedError("TestExecutionError")<{ readonly message: string; }> {} +class TestTimeoutException extends Data.TaggedError("TimeoutException")<{ + readonly message: string; +}> {} + const expectString: (value: unknown) => asserts value is string = (value) => { expect(typeof value).toBe("string"); }; @@ -775,6 +779,28 @@ describe("MCP host server — native elicitation mode", () => { }); }); + it("execute tool names a retryable upstream timeout without leaking the message", async () => { + const engine = makeStubEngine({ + // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: test injects a timeout defect to verify classification + execute: () => + Effect.die(new TestTimeoutException({ message: "https://api.example.com/?token=secret" })), + }); + + await withNativeClient(engine, ELICITATION_CAPS, async (client) => { + const result = await client.callTool({ name: "execute", arguments: { code: "run" } }); + const text = textOf(result); + expect(text).toMatch( + /^Error: Internal tool error \[[0-9a-f]{8}\]: upstream timeout\. Retry may succeed\.$/, + ); + expect(text).not.toContain("secret"); + expect(result.structuredContent).toMatchObject({ + status: "error", + errorCategory: "upstream_timeout", + retryable: true, + }); + }); + }); + it("form elicitation is bridged from engine to MCP client and back", async () => { const engine = makeElicitingEngine( FormElicitation.make({ diff --git a/packages/hosts/mcp/src/tool-server.ts b/packages/hosts/mcp/src/tool-server.ts index 335559465b..c84074d39c 100644 --- a/packages/hosts/mcp/src/tool-server.ts +++ b/packages/hosts/mcp/src/tool-server.ts @@ -90,6 +90,7 @@ import { import { MCP_ORG_WRITE_ACCESS_HEADER } from "./seams"; import { passthroughInstructions, SEARCH_INVOKE_SKILL } from "./passthrough-tools"; import type { McpToolMode } from "./browser-approval"; +import { classifyFailure, describeFailure } from "./failure-category"; import { logToolCall, trackEngineRun, type McpCodeRuntime } from "./tool-call-log"; // --------------------------------------------------------------------------- @@ -862,8 +863,9 @@ const formatResumeApprovalRequired = (input: { }, }); -const toMcpFailureResult = (cause: Cause.Cause): McpToolResult => { +const toMcpFailureResult = (cause: Cause.Cause, toolPath?: string): McpToolResult => { const correlationId = newCorrelationId(); + const failure = classifyFailure(cause); const defect = Cause.findDefect(cause); const nativeElicitationFailed = Result.isSuccess(defect) && @@ -872,19 +874,25 @@ const toMcpFailureResult = (cause: Cause.Cause): McpToolResult => { try { console.error(`[executor:mcp] execute defect correlation_id=${correlationId}`, { nativeElicitationFailed, + category: failure.category, + retryable: failure.retryable, + tool: toolPath, + cause: Cause.pretty(cause), }); } catch { /* ignore logger failures */ } const text = nativeElicitationFailed ? `Native elicitation transport failed [${correlationId}]. Reconnect the MCP client and try again.` - : `Internal tool error [${correlationId}]`; + : `Internal tool error [${correlationId}]${describeFailure(failure, toolPath)}`; return { content: [{ type: "text", text: `Error: ${text}` }], structuredContent: { status: "error", error: text, ...(nativeElicitationFailed ? { errorCode: "native_elicitation_transport_failed" } : {}), + ...(failure.category === "internal" ? {} : { errorCategory: failure.category }), + ...(failure.category === "internal" ? {} : { retryable: failure.retryable }), }, isError: true, }; @@ -1329,6 +1337,7 @@ const registerPassthroughTools = ( const boundary = ( effect: Effect.Effect, extra: McpRequestJoinKeys, + toolPath?: string, ) => Effect.runPromiseWith(context)( effect.pipe( @@ -1336,7 +1345,7 @@ const registerPassthroughTools = ( CurrentOrgWriteAccess, makeOrgWriteAccessState(requestOrgWriteAccess(extra)), ), - Effect.catchCause((cause) => Effect.succeed(toMcpFailureResult(cause))), + Effect.catchCause((cause) => Effect.succeed(toMcpFailureResult(cause, toolPath))), ), ); yield* Effect.sync(() => { @@ -1574,6 +1583,7 @@ const registerPassthroughTools = ( return yield* run(address, checked.data, extra); }), extra, + id, ), ); });