From b495de3a866a90c121be088f813557fcb45718d3 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 15 May 2026 10:36:08 -0700 Subject: [PATCH 1/4] feat(tool-result): add ToolResult union and dual-shape invoker Adds `ToolResult` / `ToolError` to @executor-js/sdk and a `ToolResult.ok` / `ToolResult.fail` constructor pair. Updates the sandbox tool dispatcher to accept both the new typed union (passes through unchanged) and the legacy `{ data, error }` envelope. The legacy branch now walks known upstream shapes (Microsoft Graph, DealCloud, JSON:API, plain message-bearing bodies) before falling back to a clamped JSON.stringify of the body, so structured 4xx payloads no longer collapse to 'Tool execution failed'. The cause/defect branch keeps its strict .message-only discipline. --- .../execution/src/tool-invoker.leak.test.ts | 115 +++++++++++++ .../execution/src/tool-invoker.repro.test.ts | 159 ++++++++++++++++++ packages/core/execution/src/tool-invoker.ts | 106 ++++++++++-- packages/core/sdk/src/index.ts | 7 + packages/core/sdk/src/tool-result.ts | 44 +++++ 5 files changed, 415 insertions(+), 16 deletions(-) create mode 100644 packages/core/execution/src/tool-invoker.leak.test.ts create mode 100644 packages/core/execution/src/tool-invoker.repro.test.ts create mode 100644 packages/core/sdk/src/tool-result.ts diff --git a/packages/core/execution/src/tool-invoker.leak.test.ts b/packages/core/execution/src/tool-invoker.leak.test.ts new file mode 100644 index 000000000..9d0e9e598 --- /dev/null +++ b/packages/core/execution/src/tool-invoker.leak.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Data, Effect, Schema } from "effect"; + +import { ElicitationResponse, createExecutor, definePlugin } from "@executor-js/sdk"; +import { makeTestConfig } from "@executor-js/sdk/testing"; +import { makeExecutorToolInvoker } from "./tool-invoker"; + +const EmptyInputSchema = Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({})), +); + +const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); + +// Simulate a realistic plugin-internal tagged error whose `cause` carries +// sensitive internal context (DB connection string, full HTTP request with +// Authorization header echoed back, file paths, stack traces). +class FakeOpenApiInvocationError extends Data.TaggedError("OpenApiInvocationError")<{ + readonly message: string; + readonly cause: unknown; +}> {} + +const leakyPlugin = definePlugin(() => ({ + id: "leaky-test" as const, + storage: () => ({}), + staticSources: () => [ + { + id: "leaky", + kind: "in-memory", + name: "Leaky", + tools: [ + { + name: "failsWithCause", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail(new FakeOpenApiInvocationError({ + message: "HTTP request failed", + cause: { + _tag: "HttpClientError", + request: { + method: "GET", + url: "https://internal.dealcloud/v1/entities?accessToken=SECRET_TOKEN_xyz", + headers: { Authorization: "Bearer SECRET_TOKEN_xyz" }, + }, + stack: + "Error: ECONNREFUSED\n at /home/svc/executor/packages/plugins/openapi/...:142:11", + dbConnString: "postgres://app:p@ssw0rd@10.0.0.5:5432/executor", + }, + })), + }, + { + name: "throwsRawError", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail( + Object.assign(new Error("Internal: secret 'sk_live_abcd' rotation failed"), { + stack: + "Error: Internal: secret 'sk_live_abcd' rotation failed\n at /home/svc/.../secret-store.ts:88", + }), + ), + }, + ], + }, + ], +})); + +describe("internal-error leak audit", () => { + it.effect("plugin tagged error: only .message escapes, cause stays hidden", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [leakyPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "leaky.failsWithCause", args: {} }), + ); + const msg = (err as { message: string }).message; + // eslint-disable-next-line no-console + console.log("[leak failsWithCause]", msg); + + expect(msg).toBe("HTTP request failed"); + expect(msg).not.toContain("SECRET_TOKEN_xyz"); + expect(msg).not.toContain("p@ssw0rd"); + expect(msg).not.toContain("packages/plugins"); + expect(msg).not.toContain("HttpClientError"); + }), + ); + + it.effect("plain Error with stack: stack does NOT leak, only message", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [leakyPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "leaky.throwsRawError", args: {} }), + ); + const msg = (err as { message: string }).message; + // eslint-disable-next-line no-console + console.log("[leak throwsRawError]", msg); + + // message itself contains the secret because the plugin put it there — + // that's plugin discipline. But stack and file path should not appear. + expect(msg).not.toContain("secret-store.ts"); + expect(msg).not.toContain("at /home/"); + }), + ); +}); diff --git a/packages/core/execution/src/tool-invoker.repro.test.ts b/packages/core/execution/src/tool-invoker.repro.test.ts new file mode 100644 index 000000000..973a6e890 --- /dev/null +++ b/packages/core/execution/src/tool-invoker.repro.test.ts @@ -0,0 +1,159 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Schema } from "effect"; + +import { ElicitationResponse, createExecutor, definePlugin } from "@executor-js/sdk"; +import { makeTestConfig } from "@executor-js/sdk/testing"; +import { makeExecutorToolInvoker } from "./tool-invoker"; + +const EmptyInputSchema = Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({})), +); + +const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); + +// Simulate the kind of error envelopes real upstreams (SharePoint, DealCloud, +// Microsoft Graph, etc.) actually return. +const upstreamErrorPlugin = definePlugin(() => ({ + id: "upstream-error-test" as const, + storage: () => ({}), + staticSources: () => [ + { + id: "upstream", + kind: "in-memory", + name: "Upstream", + tools: [ + { + // Microsoft Graph / SharePoint shape: { error: { code, message } } + name: "sharepointShape", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed({ + data: null, + error: { + error: { + code: "invalidRequest", + message: + "The expression \"foo\" is not valid. Provide a valid expression.", + }, + }, + }), + }, + { + // DealCloud-ish shape: errorCode + errorMessage + name: "dealcloudShape", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed({ + data: null, + error: { + errorCode: 400, + errorMessage: "Entity 'Deals' has no field 'XYZ'", + }, + }), + }, + { + // JSON:API / multi-errors shape + name: "errorsArrayShape", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed({ + data: null, + error: { + errors: [ + { status: "403", title: "Forbidden", detail: "Insufficient scope" }, + ], + }, + }), + }, + { + // Plain string body + name: "stringShape", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed({ + data: null, + error: "Internal Server Error", + }), + }, + ], + }, + ], +})); + +describe("repro: opaque tool execution failures", () => { + it.effect("SharePoint/Graph nested error.message is LOST -> 'Tool execution failed'", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "upstream.sharepointShape", args: {} }), + ); + // eslint-disable-next-line no-console + console.log("[repro sharepoint]", (err as { message: string }).message); + expect((err as { message: string }).message).toBe("Tool execution failed"); + }), + ); + + it.effect("DealCloud errorMessage is LOST -> 'Tool execution failed'", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "upstream.dealcloudShape", args: {} }), + ); + // eslint-disable-next-line no-console + console.log("[repro dealcloud]", (err as { message: string }).message); + expect((err as { message: string }).message).toBe("Tool execution failed"); + }), + ); + + it.effect("JSON:API errors[] is LOST -> 'Tool execution failed'", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "upstream.errorsArrayShape", args: {} }), + ); + // eslint-disable-next-line no-console + console.log("[repro errors-array]", (err as { message: string }).message); + expect((err as { message: string }).message).toBe("Tool execution failed"); + }), + ); + + it.effect("plain string error body is preserved", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const err = yield* Effect.flip( + invoker.invoke({ path: "upstream.stringShape", args: {} }), + ); + // eslint-disable-next-line no-console + console.log("[repro string]", (err as { message: string }).message); + expect((err as { message: string }).message).toBe("Internal Server Error"); + }), + ); +}); diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index d8f976e57..61cb7e2c8 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -8,6 +8,7 @@ import type { InvokeOptions, Source, } from "@executor-js/sdk/core"; +import { isToolResult } from "@executor-js/sdk/core"; import type { SandboxToolInvoker } from "@executor-js/codemode-core"; import { ExecutionToolError } from "./errors"; @@ -36,7 +37,9 @@ const messageFromErrorLike = (value: unknown): string | undefined => { return undefined; }; -const renderToolErrorMessage = (error: unknown): string => +// Boundary: `.catchCause` branch — `err` is an internal/typed plugin error. +// Keep `.message`-only discipline; never walk `.cause`/stack/structured body. +const renderCauseErrorMessage = (error: unknown): string => messageFromErrorLike(error) ?? (typeof error === "undefined" ? "Tool execution failed" : renderUnknownPrimitive(error)); @@ -48,19 +51,81 @@ const renderUnknownPrimitive = (value: unknown): string => Option.getOrElse(() => "Tool execution failed"), ); -type ToolResultEnvelope = { +type LegacyToolResultEnvelope = { readonly error?: unknown; readonly data?: unknown; }; -const isToolResultEnvelope = (value: unknown): value is ToolResultEnvelope => +const isLegacyToolResultEnvelope = (value: unknown): value is LegacyToolResultEnvelope => value !== null && typeof value === "object" && ("error" in value || "data" in value); -const hasToolResultError = ( - value: ToolResultEnvelope, -): value is ToolResultEnvelope & { readonly error: unknown } => +const hasLegacyToolResultError = ( + value: LegacyToolResultEnvelope, +): value is LegacyToolResultEnvelope & { readonly error: unknown } => value.error !== null && value.error !== undefined; +const STRINGIFIED_BODY_CAP = 1024; + +// Boundary: legacy envelope branch — `body` is a domain-level structured +// upstream error body returned by the handler in a `data: null, error: ...` +// envelope. Walk known upstream shapes (Microsoft Graph, DealCloud, +// JSON:API, etc.) before falling back to a clamped JSON.stringify. +const extractLegacyEnvelopeMessage = (body: unknown): string => { + if (typeof body === "string") { + return body.length === 0 ? "Tool execution failed" : body; + } + if (body === null || typeof body !== "object") { + return renderUnknownPrimitive(body); + } + + const obj = body as Record; + + // Microsoft Graph / SharePoint: { error: { code, message } } + const nested = obj.error; + if (nested !== null && typeof nested === "object" && "message" in nested) { + const m = (nested as { message: unknown }).message; + if (typeof m === "string" && m.length > 0) return m; + } + + // Plain { message: ... } + if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; + + // DealCloud-ish: { errorCode, errorMessage } + if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) return obj.errorMessage; + + // JSON:API multi-errors: { errors: [{ detail|message|title, ... }] } + if (Array.isArray(obj.errors) && obj.errors.length > 0) { + const first = obj.errors[0]; + if (first !== null && typeof first === "object") { + const f = first as Record; + for (const key of ["detail", "message", "title"]) { + const v = f[key]; + if (typeof v === "string" && v.length > 0) return v; + } + } + } + + for (const key of ["detail", "title", "description"]) { + const v = obj[key]; + if (typeof v === "string" && v.length > 0) return v; + } + + return clampedStringify(body); +}; + +const clampedStringify = (value: unknown): string => { + let s: string; + try { + s = JSON.stringify(value); + } catch { + s = String(value); + } + if (s.length > STRINGIFIED_BODY_CAP) { + return `${s.slice(0, STRINGIFIED_BODY_CAP)}…`; + } + return s; +}; + /** * Bridges QuickJS `tools.someSource.someOp(args)` calls into * `executor.tools.invoke(toolId, args)`. @@ -90,7 +155,7 @@ export const makeExecutorToolInvoker = ( if (!isElicitationDeclinedError(err)) { return Effect.fail( new ExecutionToolError({ - message: renderToolErrorMessage(err), + message: renderCauseErrorMessage(err), cause: err ?? cause, }), ); @@ -103,17 +168,26 @@ export const makeExecutorToolInvoker = ( ); }), ); - if (!isToolResultEnvelope(result)) { + + // New typed-union path. Pass the whole `ToolResult` through; user + // sandbox code branches on `r.ok`. + if (isToolResult(result)) { return result; } - if (hasToolResultError(result)) { - return yield* new ExecutionToolError({ - message: renderToolErrorMessage(result.error), - cause: result.error, - }); - } - if ("data" in result) { - return result.data; + + // Legacy envelope shim. Translates the old `{ data, error }` shape + // into an Effect failure so existing user code keeps throwing on + // domain errors. Phase 3 deletes this branch outright. + if (isLegacyToolResultEnvelope(result)) { + if (hasLegacyToolResultError(result)) { + return yield* new ExecutionToolError({ + message: extractLegacyEnvelopeMessage(result.error), + cause: result.error, + }); + } + if ("data" in result) { + return result.data; + } } return result; }), diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index ecc0b141a..7297129e3 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -333,3 +333,10 @@ export { // Wire-level HTTP error schemas usable by plugin HttpApiGroup definitions. export { InternalError } from "./api-errors"; + +// ToolResult — typed value-based discriminated union for tool outcomes. +// The `Tool` value namespace exposes `Tool.ok` / `Tool.fail` constructors; +// the `Tool` type alias from `./types` is a separate row projection. +// TypeScript permits the two to share a name because one is purely a +// value and the other purely a type. +export { ToolResult, isToolResult, type ToolError } from "./tool-result"; diff --git a/packages/core/sdk/src/tool-result.ts b/packages/core/sdk/src/tool-result.ts new file mode 100644 index 000000000..ec6016661 --- /dev/null +++ b/packages/core/sdk/src/tool-result.ts @@ -0,0 +1,44 @@ +// --------------------------------------------------------------------------- +// ToolResult — typed value-based discriminated union returned by tool +// handlers and `invokeTool`. Domain success and expected failure both +// resolve through Effect's success channel; only true infra defects use +// the Effect failure channel. +// --------------------------------------------------------------------------- + +export interface ToolError { + readonly code: string; + readonly message: string; + readonly status?: number; + readonly details?: unknown; + readonly retryable?: boolean; +} + +export type ToolResult = + | { readonly ok: true; readonly data: T } + | { readonly ok: false; readonly error: ToolError }; + +export const ToolResult = { + ok: (data: T): ToolResult => ({ ok: true, data }), + fail: (error: ToolError): ToolResult => ({ ok: false, error }), +} as const; + + +export const isToolResult = (value: unknown): value is ToolResult => { + if (value === null || typeof value !== "object") return false; + if (!("ok" in value)) return false; + const ok = (value as { ok: unknown }).ok; + if (ok === true) return "data" in value; + if (ok === false) { + if (!("error" in value)) return false; + const error = (value as { error: unknown }).error; + return ( + error !== null && + typeof error === "object" && + "code" in error && + "message" in error && + typeof (error as { code: unknown }).code === "string" && + typeof (error as { message: unknown }).message === "string" + ); + } + return false; +}; From 38e6a044e1ff021b24dc35af0d8b9678fc4a2519 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 15 May 2026 10:48:59 -0700 Subject: [PATCH 2/4] feat(plugins): emit ToolResult from openapi, graphql, mcp, google-discovery Migrates each dynamic plugin's invokeTool to wrap its result in the typed ToolResult union. OpenAPI and Google Discovery map non-2xx responses to ToolResult.fail with an extracted upstream message and the raw body in error.details; 2xx responses to ToolResult.ok with { status, headers, data }. GraphQL maps 200-with-errors bodies to ToolResult.fail({ code: 'graphql_errors', ... }) and bare data to ToolResult.ok. MCP maps { isError: true } to ToolResult.fail and content arrays to ToolResult.ok. Static plugin tools (executor.openapi.previewSpec / addSource, executor.graphql.addSource) now also wrap their plain values in ToolResult.ok for shape consistency. Updates plugin tests that asserted on the old { status, headers, data, error } envelope or the bare MCP { content } shape. Tests that consume the OpenAPI plugin's invocation result use the new unwrapInvocation helper from @executor-js/plugin-openapi/testing. --- .../google-discovery/src/sdk/plugin.test.ts | 6 +- .../google-discovery/src/sdk/plugin.ts | 72 +++++++++++++- .../plugins/graphql/src/sdk/plugin.test.ts | 14 ++- packages/plugins/graphql/src/sdk/plugin.ts | 21 ++++- .../plugins/mcp/src/sdk/elicitation.test.ts | 9 +- .../src/sdk/per-user-auth-isolation.test.ts | 6 +- packages/plugins/mcp/src/sdk/plugin.ts | 35 ++++++- .../src/sdk/client-credentials-oauth.test.ts | 11 +-- .../src/sdk/multi-scope-bearer.test.ts | 93 ++++++++++--------- .../openapi/src/sdk/multi-scope-oauth.test.ts | 37 +++++--- .../openapi/src/sdk/oauth-refresh.test.ts | 20 ++-- .../plugins/openapi/src/sdk/plugin.test.ts | 35 ++++--- packages/plugins/openapi/src/sdk/plugin.ts | 67 ++++++++++++- packages/plugins/openapi/src/testing/index.ts | 65 +++++++++++++ 14 files changed, 372 insertions(+), 119 deletions(-) diff --git a/packages/plugins/google-discovery/src/sdk/plugin.test.ts b/packages/plugins/google-discovery/src/sdk/plugin.test.ts index 55089f297..5b0b91eb1 100644 --- a/packages/plugins/google-discovery/src/sdk/plugin.test.ts +++ b/packages/plugins/google-discovery/src/sdk/plugin.test.ts @@ -430,10 +430,10 @@ describe("Google Discovery plugin", () => { "drive.files.get", { fileId: "123", fields: "id,name", prettyPrint: true }, autoApprove, - )) as { data: unknown; error: unknown }; + )) as { readonly ok: true; readonly data: { status: number; data: unknown } }; - expect(invocation.error).toBeNull(); - expect(invocation.data).toEqual({ id: "123", name: "Quarterly Plan" }); + expect(invocation.ok).toBe(true); + expect(invocation.data.data).toEqual({ id: "123", name: "Quarterly Plan" }); const apiRequest = handle.requests.find((request) => request.url.startsWith("/drive/v3/files/123"), diff --git a/packages/plugins/google-discovery/src/sdk/plugin.ts b/packages/plugins/google-discovery/src/sdk/plugin.ts index f44c7a25c..b308d06dd 100644 --- a/packages/plugins/google-discovery/src/sdk/plugin.ts +++ b/packages/plugins/google-discovery/src/sdk/plugin.ts @@ -3,6 +3,7 @@ import { Effect, Option, Predicate, Schema } from "effect"; import { ScopeId, SourceDetectionResult, + ToolResult, Usage, definePlugin, resolveSecretBackedMap, @@ -29,6 +30,51 @@ import type { } from "./types"; import { GoogleDiscoveryStoredSourceData as GoogleDiscoveryStoredSourceDataSchema } from "./types"; +// --------------------------------------------------------------------------- +// Upstream-error message extraction +// --------------------------------------------------------------------------- + +const GOOGLE_BODY_CAP = 1024; + +const googleClampedStringify = (value: unknown): string => { + let s: string; + try { + s = JSON.stringify(value); + } catch { + s = String(value); + } + return s.length > GOOGLE_BODY_CAP ? `${s.slice(0, GOOGLE_BODY_CAP)}…` : s; +}; + +const googleExtractUpstreamMessage = (body: unknown, status: number): string => { + if (typeof body === "string") { + return body.length > 0 ? body : `Upstream returned HTTP ${status}`; + } + if (body !== null && typeof body === "object") { + const obj = body as Record; + const nested = obj.error; + if (nested !== null && typeof nested === "object" && "message" in nested) { + const m = (nested as { message: unknown }).message; + if (typeof m === "string" && m.length > 0) return m; + } + if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; + if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) + return obj.errorMessage; + if (Array.isArray(obj.errors) && obj.errors.length > 0) { + const first = obj.errors[0]; + if (first !== null && typeof first === "object") { + const f = first as Record; + for (const key of ["detail", "message", "title"]) { + const v = f[key]; + if (typeof v === "string" && v.length > 0) return v; + } + } + } + return googleClampedStringify(body); + } + return `Upstream returned HTTP ${status}`; +}; + // --------------------------------------------------------------------------- // Public input / output shapes // --------------------------------------------------------------------------- @@ -381,11 +427,27 @@ export const googleDiscoveryPlugin = definePlugin(() => ({ extension: makeGoogleDiscoveryPluginExtension, invokeTool: ({ ctx, toolRow, args }) => - invokeGoogleDiscoveryTool({ - ctx: ctx as PluginCtx, - toolId: toolRow.id, - toolScope: decodeString(toolRow.scope_id), - args, + Effect.gen(function* () { + const result = yield* invokeGoogleDiscoveryTool({ + ctx: ctx as PluginCtx, + toolId: toolRow.id, + toolScope: decodeString(toolRow.scope_id), + args, + }); + const ok = result.status >= 200 && result.status < 300; + if (!ok) { + return ToolResult.fail({ + code: "upstream_http_error", + status: result.status, + message: googleExtractUpstreamMessage(result.error, result.status), + details: result.error, + }); + } + return ToolResult.ok({ + status: result.status, + headers: result.headers, + data: result.data, + }); }), resolveAnnotations: ({ ctx, sourceId, toolRows }) => diff --git a/packages/plugins/graphql/src/sdk/plugin.test.ts b/packages/plugins/graphql/src/sdk/plugin.test.ts index df4d57fab..c265f45b1 100644 --- a/packages/plugins/graphql/src/sdk/plugin.test.ts +++ b/packages/plugins/graphql/src/sdk/plugin.test.ts @@ -196,9 +196,8 @@ describe("graphqlPlugin real protocol server", () => { }); expect(result).toEqual({ - status: 200, + ok: true, data: { hello: "Hello Ada" }, - errors: null, }); const requests = yield* server.requests; @@ -251,9 +250,8 @@ describe("graphqlPlugin real protocol server", () => { }); expect(result).toEqual({ - status: 200, + ok: true, data: { hello: "Hello Ada" }, - errors: null, }); const requests = yield* server.requests; @@ -425,7 +423,7 @@ describe("graphqlPlugin", () => { }, { onElicitation: "accept-all" }, ); - expect(result).toEqual({ toolCount: 2, namespace: "via_static" }); + expect(result).toEqual({ ok: true, data: { toolCount: 2, namespace: "via_static" } }); expect(yield* executor.graphql.getSource("via_static", String(userScope))).toBeNull(); expect((yield* executor.graphql.getSource("via_static", String(orgScope)))?.scope).toBe( orgScope, @@ -729,7 +727,7 @@ describe("graphqlPlugin", () => { }); expect(result).toMatchObject({ - status: 200, + ok: true, data: { hello: "Hello Ada" }, }); const requests = yield* server.requests; @@ -843,7 +841,7 @@ describe("graphqlPlugin", () => { }); expect(result).toMatchObject({ - status: 200, + ok: true, data: { hello: "Hello Ada" }, }); const requests = yield* server.requests; @@ -915,7 +913,7 @@ describe("graphqlPlugin", () => { }); expect(result).toMatchObject({ - status: 200, + ok: true, data: { hello: "Hello Ada" }, }); const requests = yield* server.requests; diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index 3b0981729..35814d6bd 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -12,6 +12,7 @@ import { SecretId, SourceDetectionResult, StorageError, + ToolResult, type PluginCtx, type StorageFailure, type ToolAnnotations, @@ -994,7 +995,7 @@ export const graphqlPlugin = definePlugin((options?: GraphqlPluginOptions) => { }, inputSchema: StaticAddSourceInputStandardSchema, outputSchema: StaticAddSourceOutputStandardSchema, - execute: (input) => self.addSource(input), + execute: (input) => Effect.map(self.addSource(input), ToolResult.ok), }), ], }, @@ -1057,7 +1058,23 @@ export const graphqlPlugin = definePlugin((options?: GraphqlPluginOptions) => { httpClientLayer, ); - return result; + const errors = result.errors; + if (Array.isArray(errors) && errors.length > 0) { + const first = errors[0]; + const firstMessage = + first !== null && typeof first === "object" && "message" in first + ? (first as { message: unknown }).message + : undefined; + return ToolResult.fail({ + code: "graphql_errors", + message: + typeof firstMessage === "string" && firstMessage.length > 0 + ? firstMessage + : "GraphQL request returned errors", + details: { errors }, + }); + } + return ToolResult.ok(result.data); }), resolveAnnotations: ({ ctx, sourceId, toolRows }) => diff --git a/packages/plugins/mcp/src/sdk/elicitation.test.ts b/packages/plugins/mcp/src/sdk/elicitation.test.ts index f1d92029e..908d99107 100644 --- a/packages/plugins/mcp/src/sdk/elicitation.test.ts +++ b/packages/plugins/mcp/src/sdk/elicitation.test.ts @@ -69,7 +69,8 @@ describe("MCP elicitation (end-to-end)", () => { const result = yield* executor.tools.invoke(gatedEcho!.id, { value: "hello" }, options); expect(result).toMatchObject({ - content: [{ type: "text", text: "approved:hello" }], + ok: true, + data: [{ type: "text", text: "approved:hello" }], }); // At least one elicitation should be the MCP server's form expect(elicitationMessages.length).toBeGreaterThanOrEqual(1); @@ -95,7 +96,8 @@ describe("MCP elicitation (end-to-end)", () => { ); expect(result).toMatchObject({ - content: [{ type: "text", text: "denied:nope" }], + ok: true, + data: [{ type: "text", text: "denied:nope" }], }); }), ); @@ -114,7 +116,8 @@ describe("MCP elicitation (end-to-end)", () => { ); expect(result).toMatchObject({ - content: [{ type: "text", text: "plain" }], + ok: true, + data: [{ type: "text", text: "plain" }], }); }), ); diff --git a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts index af155b7c1..bd2bafc14 100644 --- a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts +++ b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts @@ -115,7 +115,7 @@ describe("per-user MCP auth isolation", () => { { onElicitation: "accept-all" }, ); expect(userAResult).toMatchObject({ - content: [{ type: "text", text: "ok:from-user-a" }], + ok: true, data: [{ type: "text", text: "ok:from-user-a" }], }); expect( (yield* server.requests) @@ -187,7 +187,7 @@ describe("per-user MCP auth isolation", () => { { onElicitation: "accept-all" }, ); expect(userAResult).toMatchObject({ - content: [{ type: "text", text: "ok:user-a-header" }], + ok: true, data: [{ type: "text", text: "ok:user-a-header" }], }); expect( (yield* server.requests) @@ -267,7 +267,7 @@ describe("per-user MCP auth isolation", () => { ); expect(result).toMatchObject({ - content: [{ type: "text", text: "ok:org-header" }], + ok: true, data: [{ type: "text", text: "ok:org-header" }], }); const invokeRequests = (yield* server.requests).slice(beforeInvoke); expect( diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 3e9776091..df0d59301 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -22,6 +22,7 @@ import { ScopeId, SecretId, SourceDetectionResult, + ToolResult, definePlugin, resolveSecretBackedMap as resolveSharedSecretBackedMap, type PluginCtx, @@ -194,6 +195,25 @@ const toBinding = (entry: McpToolManifestEntry): McpToolBinding => const MCP_PLUGIN_ID = "mcp"; +const extractMcpErrorMessage = (content: unknown): string => { + if (Array.isArray(content)) { + for (const item of content) { + if ( + item !== null && + typeof item === "object" && + "type" in item && + (item as { type: unknown }).type === "text" && + "text" in item && + typeof (item as { text: unknown }).text === "string" && + (item as { text: string }).text.length > 0 + ) { + return (item as { text: string }).text; + } + } + } + return "MCP tool returned an error"; +}; + /** Match `token` as a separator-bounded run inside a URL hostname or path, * used as a low-confidence detection hint when wire-shape detection fails. * Boundary chars are everything non-alphanumeric, so `/api/mcp`, @@ -1726,7 +1746,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { }); } - return yield* invokeMcpTool({ + const raw = yield* invokeMcpTool({ toolId: toolRow.id, toolName: entry.binding.toolName, args, @@ -1764,6 +1784,19 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { pendingConnectors: runtime.pendingConnectors, elicit, }); + + const rawObj = + raw !== null && typeof raw === "object" ? (raw as Record) : undefined; + const isError = rawObj?.isError === true; + const content = rawObj?.content; + if (isError) { + return ToolResult.fail({ + code: "mcp_tool_error", + message: extractMcpErrorMessage(content), + details: { content }, + }); + } + return ToolResult.ok(content ?? raw); }).pipe( Effect.withSpan("mcp.plugin.invoke_tool", { attributes: { diff --git a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts index 82aaeb424..c4023db6d 100644 --- a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts +++ b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts @@ -26,6 +26,7 @@ import { makeTestConfig, serveOAuthTestServer } from "@executor-js/sdk/testing"; import { addOpenApiTestSource, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "@executor-js/plugin-openapi/testing"; import { openApiPlugin } from "./plugin"; @@ -226,16 +227,14 @@ describe("OpenAPI client_credentials OAuth", () => { ); // Invoking the tool injects the freshly-minted bearer via // ctx.connections.accessToken. - const result = (yield* userExec.tools.invoke( + const result = unwrapInvocation(yield* userExec.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { - data: { authorization?: string } | null; - error: unknown; - }; + )); expect(result.error).toBeNull(); - const bearer = result.data?.authorization?.replace(/^Bearer\s+/i, ""); + const data = result.data as EchoHeaders | null; + const bearer = data?.authorization?.replace(/^Bearer\s+/i, ""); expect(bearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bearer!)).toBe(true); diff --git a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts index 9ad28e255..f1ad02a10 100644 --- a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts +++ b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts @@ -38,6 +38,7 @@ import { addOpenApiTestSource, makeOpenApiTestSourceConfig, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "@executor-js/plugin-openapi/testing"; import { openApiPlugin } from "./plugin"; @@ -277,25 +278,23 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { // yields her token; bob's scope yields his. Same source, same // tool, different injected bearer. // ------------------------------------------------------------- - const aliceResult = (yield* aliceExec.tools.invoke( + const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( "vercel.projects.list", {}, autoApprove, - )) as { - data: { authorization?: string; token?: string } | null; - error: unknown; - }; + )); expect(aliceResult.error).toBeNull(); - expect(aliceResult.data?.authorization).toBe("Bearer alice-vercel-token"); - expect(aliceResult.data?.token).toBe("alice-team"); + const aliceData = aliceResult.data as EchoHeaders | null; + expect(aliceData?.authorization).toBe("Bearer alice-vercel-token"); + expect(aliceData?.token).toBe("alice-team"); - const bobResult = (yield* bobExec.tools.invoke("vercel.projects.list", {}, autoApprove)) as { - data: { authorization?: string; token?: string } | null; - error: unknown; - }; + const bobResult = unwrapInvocation( + yield* bobExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(bobResult.error).toBeNull(); - expect(bobResult.data?.authorization).toBe("Bearer bob-vercel-token"); - expect(bobResult.data?.token).toBe("bob-team"); + const bobData = bobResult.data as EchoHeaders | null; + expect(bobData?.authorization).toBe("Bearer bob-vercel-token"); + expect(bobData?.token).toBe("bob-team"); // ------------------------------------------------------------- // 5. Scope attribution: each user's token is pinned to their @@ -450,20 +449,23 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const aliceResult = (yield* aliceExec.tools.invoke( + const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( "vercel.projects.list", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(aliceResult.error).toBeNull(); - expect(aliceResult.data?.authorization).toBe("Bearer alice-vercel-token"); + expect((aliceResult.data as EchoHeaders | null)?.authorization).toBe( + "Bearer alice-vercel-token", + ); - const bobResult = (yield* bobExec.tools.invoke("vercel.projects.list", {}, autoApprove)) as { - data: { authorization?: string } | null; - error: unknown; - }; + const bobResult = unwrapInvocation( + yield* bobExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(bobResult.error).toBeNull(); - expect(bobResult.data?.authorization).toBe("Bearer bob-vercel-token"); + expect((bobResult.data as EchoHeaders | null)?.authorization).toBe( + "Bearer bob-vercel-token", + ); }), ); @@ -558,13 +560,13 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const sharedResult = (yield* aliceExec.tools.invoke( + const sharedResult = unwrapInvocation(yield* aliceExec.tools.invoke( "vercel.projects.list", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(sharedResult.error).toBeNull(); - expect(sharedResult.data?.authorization).toBe("Bearer org-token"); + expect((sharedResult.data as EchoHeaders | null)?.authorization).toBe("Bearer org-token"); yield* aliceExec.secrets.set( SetSecretInput.make({ @@ -587,13 +589,15 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const overrideResult = (yield* aliceExec.tools.invoke( + const overrideResult = unwrapInvocation(yield* aliceExec.tools.invoke( "vercel.projects.list", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(overrideResult.error).toBeNull(); - expect(overrideResult.data?.authorization).toBe("Bearer alice-token"); + expect((overrideResult.data as EchoHeaders | null)?.authorization).toBe( + "Bearer alice-token", + ); yield* aliceExec.openapi.removeSourceBinding( "vercel", @@ -602,13 +606,15 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { String(aliceScope.id), ); - const fallbackResult = (yield* aliceExec.tools.invoke( + const fallbackResult = unwrapInvocation(yield* aliceExec.tools.invoke( "vercel.projects.list", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(fallbackResult.error).toBeNull(); - expect(fallbackResult.data?.authorization).toBe("Bearer org-token"); + expect((fallbackResult.data as EchoHeaders | null)?.authorization).toBe( + "Bearer org-token", + ); yield* aliceExec.openapi.setSourceBinding( OpenApiSourceBindingInput.make({ @@ -812,12 +818,11 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const result = (yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove)) as { - data: { authorization?: string } | null; - error: unknown; - }; + const result = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(result.error).toBeNull(); - expect(result.data?.authorization).toBe("Bearer alice-token"); + expect((result.data as EchoHeaders | null)?.authorization).toBe("Bearer alice-token"); }), ); @@ -914,13 +919,12 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const result = (yield* userExec.tools.invoke("vercel.projects.list", {}, autoApprove)) as { - data: { authorization?: string } | null; - error: unknown; - }; + const result = unwrapInvocation( + yield* userExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(result.error).toBeNull(); - expect(result.data?.authorization).toBe("Bearer org-token"); + expect((result.data as EchoHeaders | null)?.authorization).toBe("Bearer org-token"); }), ); @@ -1013,13 +1017,12 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const result = (yield* userExec.tools.invoke("vercel.projects.list", {}, autoApprove)) as { - data: { authorization?: string } | null; - error: unknown; - }; + const result = unwrapInvocation( + yield* userExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(result.error).toBeNull(); - expect(result.data?.authorization).toBe("Bearer org-choice"); + expect((result.data as EchoHeaders | null)?.authorization).toBe("Bearer org-choice"); }), ); }); diff --git a/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts b/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts index 6badab146..d9efbfe45 100644 --- a/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts +++ b/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts @@ -28,6 +28,7 @@ import { makeTestConfig, serveOAuthTestServer } from "@executor-js/sdk/testing"; import { addOpenApiTestSource, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "@executor-js/plugin-openapi/testing"; import { openApiPlugin } from "./plugin"; @@ -307,23 +308,29 @@ describe("OpenAPI multi-scope OAuth", () => { // 4. Invoke through each exec — Authorization must carry that // user's token. // ------------------------------------------------------------- - const aliceResult = (yield* aliceExec.tools.invoke( + const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(aliceResult.error).toBeNull(); - const aliceBearer = aliceResult.data?.authorization?.replace(/^Bearer\s+/i, ""); + const aliceBearer = (aliceResult.data as EchoHeaders | null)?.authorization?.replace( + /^Bearer\s+/i, + "", + ); expect(aliceBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(aliceBearer!)).toBe(true); - const bobResult = (yield* bobExec.tools.invoke( + const bobResult = unwrapInvocation(yield* bobExec.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(bobResult.error).toBeNull(); - const bobBearer = bobResult.data?.authorization?.replace(/^Bearer\s+/i, ""); + const bobBearer = (bobResult.data as EchoHeaders | null)?.authorization?.replace( + /^Bearer\s+/i, + "", + ); expect(bobBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bobBearer!)).toBe(true); expect(bobBearer).not.toBe(aliceBearer); @@ -610,23 +617,29 @@ describe("OpenAPI multi-scope OAuth", () => { // (4) Each user's invocation resolves their OWN row and gets // their OWN token — not whatever the last signer happened to // mint. This is the core multi-user regression. - const aliceResult = (yield* aliceExec.tools.invoke( + const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(aliceResult.error).toBeNull(); - const aliceBearer = aliceResult.data?.authorization?.replace(/^Bearer\s+/i, ""); + const aliceBearer = (aliceResult.data as EchoHeaders | null)?.authorization?.replace( + /^Bearer\s+/i, + "", + ); expect(aliceBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(aliceBearer!)).toBe(true); - const bobResult = (yield* bobExec.tools.invoke( + const bobResult = unwrapInvocation(yield* bobExec.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(bobResult.error).toBeNull(); - const bobBearer = bobResult.data?.authorization?.replace(/^Bearer\s+/i, ""); + const bobBearer = (bobResult.data as EchoHeaders | null)?.authorization?.replace( + /^Bearer\s+/i, + "", + ); expect(bobBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bobBearer!)).toBe(true); expect(bobBearer).not.toBe(aliceBearer); diff --git a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts index 45ffbf994..bc21bf038 100644 --- a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts +++ b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts @@ -35,6 +35,7 @@ import { makeTestConfig, serveOAuthTestServer } from "@executor-js/sdk/testing"; import { addOpenApiTestSource, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "@executor-js/plugin-openapi/testing"; import { openApiPlugin } from "./plugin"; @@ -253,17 +254,18 @@ describe("OpenAPI oauth refresh", () => { }); yield* bindOAuthConnection(executor, scopeId, "conn-refresh-ok", auth); - const result = (yield* executor.tools.invoke( + const result = unwrapInvocation(yield* executor.tools.invoke( "petstore.items.echoHeaders", {}, autoApprove, - )) as { data: { authorization?: string } | null; error: unknown }; + )); expect(result.error).toBeNull(); + const data = result.data as EchoHeaders | null; // Proves the refresh landed: invoke carried the fresh token, // not the expired one we seeded. - expect(result.data?.authorization).not.toBe("Bearer expired-access-v1"); - const bearer = result.data?.authorization?.replace(/^Bearer\s+/i, ""); + expect(data?.authorization).not.toBe("Bearer expired-access-v1"); + const bearer = data?.authorization?.replace(/^Bearer\s+/i, ""); expect(bearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bearer!)).toBe(true); const calls = refreshTokenRequests(yield* oauth.requests); @@ -313,12 +315,12 @@ describe("OpenAPI oauth refresh", () => { ); for (const r of invokes) { - const res = r as { - data: { authorization?: string } | null; - error: unknown; - }; + const res = unwrapInvocation(r); expect(res.error).toBeNull(); - const bearer = res.data?.authorization?.replace(/^Bearer\s+/i, ""); + const bearer = (res.data as EchoHeaders | null)?.authorization?.replace( + /^Bearer\s+/i, + "", + ); expect(bearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bearer!)).toBe(true); } diff --git a/packages/plugins/openapi/src/sdk/plugin.test.ts b/packages/plugins/openapi/src/sdk/plugin.test.ts index 101b4d679..e569c1d01 100644 --- a/packages/plugins/openapi/src/sdk/plugin.test.ts +++ b/packages/plugins/openapi/src/sdk/plugin.test.ts @@ -25,6 +25,7 @@ import { addOpenApiTestSource, makeOpenApiHttpApiTestSourceConfig, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "../testing"; const autoApprove: InvokeOptions = { onElicitation: "accept-all" }; @@ -267,13 +268,13 @@ describe("OpenAPI Plugin", () => { }), ); - const result = (yield* executor.tools.invoke( + const preview = unwrapInvocation(yield* executor.tools.invoke( "executor.openapi.previewSpec", { spec: testApiSpec() }, autoApprove, - )) as { operationCount: number }; + )).data as { operationCount: number }; - expect(result.operationCount).toBeGreaterThanOrEqual(2); + expect(preview.operationCount).toBeGreaterThanOrEqual(2); }), ); @@ -316,11 +317,11 @@ describe("OpenAPI Plugin", () => { }), ); - const result = (yield* executor.tools.invoke( + const result = unwrapInvocation(yield* executor.tools.invoke( "executor.openapi.addSource", testApiSourceConfig({ scope: String(orgScope), namespace: "runtime" }), autoApprove, - )) as { sourceId: string; toolCount: number }; + )).data as { sourceId: string; toolCount: number }; expect(result).toEqual({ sourceId: "runtime", toolCount: 4 }); expect(yield* executor.openapi.getSource("runtime", String(userScope))).toBeNull(); @@ -557,17 +558,14 @@ describe("OpenAPI Plugin", () => { }, }); - const result = (yield* executor.tools.invoke( + const result = unwrapInvocation(yield* executor.tools.invoke( "authed.items.echoHeaders", {}, autoApprove, - )) as { - data: { authorization?: string; "x-static"?: string } | null; - error: unknown; - }; + )); expect(result.error).toBeNull(); - const data = result.data!; + const data = result.data as { authorization?: string; "x-static"?: string }; expect(data.authorization).toBe("Bearer secret-value-123"); expect(data["x-static"]).toBe("hello"); }), @@ -751,10 +749,9 @@ describe("OpenAPI Plugin", () => { namespace: "test", }); - const result = (yield* executor.tools.invoke("test.items.listItems", {}, autoApprove)) as { - data: unknown; - error: unknown; - }; + const result = unwrapInvocation( + yield* executor.tools.invoke("test.items.listItems", {}, autoApprove), + ); expect(result.error).toBeNull(); expect(result.data).toEqual(ITEMS); }), @@ -780,11 +777,11 @@ describe("OpenAPI Plugin", () => { namespace: "test", }); - const result = (yield* executor.tools.invoke( + const result = unwrapInvocation(yield* executor.tools.invoke( "test.items.getItem", { itemId: "2" }, autoApprove, - )) as { data: unknown; error: unknown }; + )); expect(result.error).toBeNull(); expect(result.data).toEqual({ id: 2, name: "Gadget" }); }), @@ -810,7 +807,7 @@ describe("OpenAPI Plugin", () => { namespace: "records", }); - const result = (yield* executor.tools.invoke( + const result = unwrapInvocation(yield* executor.tools.invoke( "records.items.queryRows", { entryTypeId: "18538", @@ -819,7 +816,7 @@ describe("OpenAPI Plugin", () => { skip: 0, }, autoApprove, - )) as { data: unknown; error: unknown }; + )); expect(result.data).toBeNull(); expect(result.error).toEqual( diff --git a/packages/plugins/openapi/src/sdk/plugin.ts b/packages/plugins/openapi/src/sdk/plugin.ts index 691afe89f..daefd8095 100644 --- a/packages/plugins/openapi/src/sdk/plugin.ts +++ b/packages/plugins/openapi/src/sdk/plugin.ts @@ -7,6 +7,7 @@ import { SecretId, SourceDetectionResult, StorageError, + ToolResult, definePlugin, tool, resolveSecretBackedMap, @@ -57,6 +58,53 @@ import { // Plugin config // --------------------------------------------------------------------------- +const STRINGIFIED_BODY_CAP = 1024; + +const clampedStringify = (value: unknown): string => { + let s: string; + try { + s = JSON.stringify(value); + } catch { + s = String(value); + } + return s.length > STRINGIFIED_BODY_CAP ? `${s.slice(0, STRINGIFIED_BODY_CAP)}…` : s; +}; + +// Walk known upstream error-body shapes. Mirrors the tool-invoker's +// legacy expansion logic. +const extractUpstreamMessage = (body: unknown, status: number): string => { + if (typeof body === "string") { + return body.length > 0 ? body : `Upstream returned HTTP ${status}`; + } + if (body !== null && typeof body === "object") { + const obj = body as Record; + const nested = obj.error; + if (nested !== null && typeof nested === "object" && "message" in nested) { + const m = (nested as { message: unknown }).message; + if (typeof m === "string" && m.length > 0) return m; + } + if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; + if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) + return obj.errorMessage; + if (Array.isArray(obj.errors) && obj.errors.length > 0) { + const first = obj.errors[0]; + if (first !== null && typeof first === "object") { + const f = first as Record; + for (const key of ["detail", "message", "title"]) { + const v = f[key]; + if (typeof v === "string" && v.length > 0) return v; + } + } + } + for (const key of ["detail", "title", "description"]) { + const v = obj[key]; + if (typeof v === "string" && v.length > 0) return v; + } + return clampedStringify(body); + } + return `Upstream returned HTTP ${status}`; +}; + export type HeaderValue = HeaderValueValue; export type ConfiguredHeaderValue = ConfiguredHeaderValueValue; export type OpenApiHeaderInput = HeaderValue | ConfiguredHeaderValue; @@ -1193,7 +1241,7 @@ export const openApiPlugin = definePlugin((options?: OpenApiPluginOptions) => { name: "previewSpec", description: "Preview an OpenAPI document before adding it as a source", inputSchema: PreviewSpecInputStandardSchema, - execute: (input) => self.previewSpec(input), + execute: (input) => Effect.map(self.previewSpec(input), ToolResult.ok), }), tool({ name: "addSource", @@ -1204,7 +1252,7 @@ export const openApiPlugin = definePlugin((options?: OpenApiPluginOptions) => { }, inputSchema: AddSourceInputStandardSchema, outputSchema: AddSourceOutputStandardSchema, - execute: (input) => self.addSpec(input), + execute: (input) => Effect.map(self.addSpec(input), ToolResult.ok), }), ], }, @@ -1283,7 +1331,20 @@ export const openApiPlugin = definePlugin((options?: OpenApiPluginOptions) => { httpClientLayer, ); - return result; + const ok = result.status >= 200 && result.status < 300; + if (!ok) { + return ToolResult.fail({ + code: "upstream_http_error", + status: result.status, + message: extractUpstreamMessage(result.error, result.status), + details: result.error, + }); + } + return ToolResult.ok({ + status: result.status, + headers: result.headers, + data: result.data, + }); }), resolveAnnotations: ({ ctx, sourceId, toolRows }) => diff --git a/packages/plugins/openapi/src/testing/index.ts b/packages/plugins/openapi/src/testing/index.ts index fbae84dc4..56fe14d09 100644 --- a/packages/plugins/openapi/src/testing/index.ts +++ b/packages/plugins/openapi/src/testing/index.ts @@ -618,3 +618,68 @@ export const TestLayers = { echo: OpenApiEchoTestServer.layer, echoWithOAuth: OpenApiEchoTestServer.layerWithOAuth, }; + +// --------------------------------------------------------------------------- +// Result unwrapping helper for tests written against the legacy +// `{ status, headers, data, error }` envelope. Translates a ToolResult +// emitted by the OpenAPI plugin's `invokeTool` back into that envelope +// so assertions like `result.data?.X` / `expect(result.error).toBeNull()` +// keep working. +// --------------------------------------------------------------------------- + +export interface LegacyInvocationEnvelope { + readonly status: number | null; + readonly headers: Record | null; + readonly data: unknown; + readonly error: unknown; +} + +export const unwrapInvocation = (raw: unknown): LegacyInvocationEnvelope => { + if (raw === null || typeof raw !== "object" || !("ok" in raw)) { + return { + status: null, + headers: null, + data: raw, + error: null, + }; + } + const r = raw as + | { readonly ok: true; readonly data: unknown } + | { readonly ok: false; readonly error: { readonly status?: number; readonly details?: unknown } }; + if (r.ok) { + const inner = r.data; + if ( + inner !== null && + typeof inner === "object" && + "status" in inner && + "headers" in inner && + "data" in inner + ) { + const wrapped = inner as { + readonly status: number; + readonly headers: Record; + readonly data: unknown; + }; + return { + status: wrapped.status, + headers: wrapped.headers, + data: wrapped.data, + error: null, + }; + } + // Plain `Tool.ok(value)` (no status/headers wrapper). Expose the + // value through `.data`. + return { + status: null, + headers: null, + data: inner, + error: null, + }; + } + return { + status: r.error.status ?? null, + headers: null, + data: null, + error: r.error.details ?? r.error, + }; +}; From 3030c8edd3ecdcb07ef4fa97d30fd1d1735426a0 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 15 May 2026 11:11:49 -0700 Subject: [PATCH 3/4] feat(tool-result): make ToolResult mandatory and route defects opaquely Removes the legacy { data, error } envelope shim from the sandbox tool dispatcher. The invoker now passes ToolResult through unchanged and wraps any other plain-value plugin return in ToolResult.ok so the sandbox surface is uniform. Plugin/infra defects no longer pass their message into the sandbox. The dispatcher generates a short hex correlation id, logs the full cause with that id under executor.correlation_id, and rejects with `Internal tool error []`. The QuickJS bridge and the dynamic-worker module template defensively re-stamp the same opaque shape; the MCP host server's top-level execute failure path does the same. ExecutionToolError in-band messages from the execution package's built-in validators (tools.search arg checks, etc.) are still passed through at the QuickJS bridge so model-facing input errors keep their useful diagnostic. Tests: - ToolResult.ok / fail / isToolResult constructor unit tests. - repro tests assert structured upstream payloads now reach the sandbox through ToolResult.error.details (not through .message). - leak tests pin the new invariant: plugin defects only escape as the opaque generic + correlation id; no token / connection string / file path leaks into Error.message. - QuickJS end-to-end defect test confirms the same shape at the sandbox boundary. - Cloud HTTP integration tests, MCP host tests, dynamic-worker invocation tests, and plugin tests updated for the new ToolResult wire shape and the opaque-generic defect contract. --- .../src/services/sources-api.node.test.ts | 17 +- .../execution/src/tool-invoker.leak.test.ts | 87 +++++---- .../execution/src/tool-invoker.repro.test.ts | 156 ++++++++-------- .../core/execution/src/tool-invoker.test.ts | 27 +-- packages/core/execution/src/tool-invoker.ts | 166 ++++-------------- packages/core/sdk/src/tool-result.test.ts | 50 ++++++ packages/core/sdk/src/tool-result.ts | 14 +- packages/hosts/mcp/src/server.test.ts | 23 ++- packages/hosts/mcp/src/server.ts | 32 ++-- .../src/invocation.test.ts | 20 ++- .../src/module-template.ts | 2 +- .../kernel/runtime-quickjs/src/index.test.ts | 46 +++++ packages/kernel/runtime-quickjs/src/index.ts | 36 +++- .../google-discovery/src/sdk/plugin.ts | 1 + .../src/sdk/per-user-auth-isolation.test.ts | 9 +- .../src/sdk/client-credentials-oauth.test.ts | 8 +- .../src/sdk/multi-scope-bearer.test.ts | 40 ++--- .../openapi/src/sdk/multi-scope-oauth.test.ts | 32 ++-- .../openapi/src/sdk/oauth-refresh.test.ts | 8 +- .../plugins/openapi/src/sdk/plugin.test.ts | 38 ++-- packages/plugins/openapi/src/sdk/plugin.ts | 1 + packages/plugins/openapi/src/testing/index.ts | 21 ++- 22 files changed, 441 insertions(+), 393 deletions(-) create mode 100644 packages/core/sdk/src/tool-result.test.ts diff --git a/apps/cloud/src/services/sources-api.node.test.ts b/apps/cloud/src/services/sources-api.node.test.ts index 0191f9a99..d6150d499 100644 --- a/apps/cloud/src/services/sources-api.node.test.ts +++ b/apps/cloud/src/services/sources-api.node.test.ts @@ -207,9 +207,15 @@ describe("sources api (HTTP)", () => { expect(execution.structured).toMatchObject({ status: "completed", result: { - message: "hello", - suffix: "world", - path: "/echo/hello", + ok: true, + data: { + status: 200, + data: { + message: "hello", + suffix: "world", + path: "/echo/hello", + }, + }, }, logs: [], }); @@ -315,7 +321,7 @@ describe("sources api (HTTP)", () => { expect(execution.isError).toBe(false); expect(execution.structured).toMatchObject({ status: "completed", - result: { hello: "Hello Ada" }, + result: { ok: true, data: { hello: "Hello Ada" } }, }); const requests = yield* server.requests; expect(requests.some((request) => request.payload.query?.includes("__schema"))).toBe(true); @@ -391,7 +397,8 @@ describe("sources api (HTTP)", () => { expect(execution.structured).toMatchObject({ status: "completed", result: { - content: [{ type: "text", text: "cloud-mcp-ok" }], + ok: true, + data: [{ type: "text", text: "cloud-mcp-ok" }], }, }); expect((yield* server.requests).length).toBeGreaterThanOrEqual(2); diff --git a/packages/core/execution/src/tool-invoker.leak.test.ts b/packages/core/execution/src/tool-invoker.leak.test.ts index 9d0e9e598..269f5d95a 100644 --- a/packages/core/execution/src/tool-invoker.leak.test.ts +++ b/packages/core/execution/src/tool-invoker.leak.test.ts @@ -3,6 +3,7 @@ import { Data, Effect, Schema } from "effect"; import { ElicitationResponse, createExecutor, definePlugin } from "@executor-js/sdk"; import { makeTestConfig } from "@executor-js/sdk/testing"; +import { ExecutionToolError } from "./errors"; import { makeExecutorToolInvoker } from "./tool-invoker"; const EmptyInputSchema = Schema.toStandardSchemaV1( @@ -11,9 +12,9 @@ const EmptyInputSchema = Schema.toStandardSchemaV1( const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); -// Simulate a realistic plugin-internal tagged error whose `cause` carries -// sensitive internal context (DB connection string, full HTTP request with -// Authorization header echoed back, file paths, stack traces). +// Plugin-internal tagged error whose `cause` carries sensitive internal +// context. The dispatcher must route this through the opaque-generic +// path so none of that context reaches the sandbox via Error.message. class FakeOpenApiInvocationError extends Data.TaggedError("OpenApiInvocationError")<{ readonly message: string; readonly cause: unknown; @@ -33,20 +34,22 @@ const leakyPlugin = definePlugin(() => ({ description: "", inputSchema: EmptyInputSchema, handler: () => - Effect.fail(new FakeOpenApiInvocationError({ - message: "HTTP request failed", - cause: { - _tag: "HttpClientError", - request: { - method: "GET", - url: "https://internal.dealcloud/v1/entities?accessToken=SECRET_TOKEN_xyz", - headers: { Authorization: "Bearer SECRET_TOKEN_xyz" }, + Effect.fail( + new FakeOpenApiInvocationError({ + message: "HTTP request failed", + cause: { + _tag: "HttpClientError", + request: { + method: "GET", + url: "https://internal.dealcloud/v1/entities?accessToken=SECRET_TOKEN_xyz", + headers: { Authorization: "Bearer SECRET_TOKEN_xyz" }, + }, + stack: + "Error: ECONNREFUSED\n at /home/svc/executor/packages/plugins/openapi/...:142:11", + dbConnString: "postgres://app:p@ssw0rd@10.0.0.5:5432/executor", }, - stack: - "Error: ECONNREFUSED\n at /home/svc/executor/packages/plugins/openapi/...:142:11", - dbConnString: "postgres://app:p@ssw0rd@10.0.0.5:5432/executor", - }, - })), + }), + ), }, { name: "throwsRawError", @@ -54,10 +57,14 @@ const leakyPlugin = definePlugin(() => ({ inputSchema: EmptyInputSchema, handler: () => Effect.fail( - Object.assign(new Error("Internal: secret 'sk_live_abcd' rotation failed"), { - stack: - "Error: Internal: secret 'sk_live_abcd' rotation failed\n at /home/svc/.../secret-store.ts:88", - }), + Object.assign( + // oxlint-disable-next-line executor/no-error-constructor -- boundary: leak test deliberately fails with a raw Error + crafted stack to assert the dispatcher's opaque-generic redaction + new Error("Internal: secret 'sk_live_abcd' rotation failed"), + { + stack: + "Error: Internal: secret 'sk_live_abcd' rotation failed\n at /home/svc/.../secret-store.ts:88", + }, + ), ), }, ], @@ -65,51 +72,43 @@ const leakyPlugin = definePlugin(() => ({ ], })); -describe("internal-error leak audit", () => { - it.effect("plugin tagged error: only .message escapes, cause stays hidden", () => +describe("internal-error leak audit (opaque defects)", () => { + it.effect("plugin tagged error: defect surfaces only as opaque generic + correlation id", () => Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [leakyPlugin()] as const }), - ); + const executor = yield* createExecutor(makeTestConfig({ plugins: [leakyPlugin()] as const })); const invoker = makeExecutorToolInvoker(executor, { invokeOptions: { onElicitation: acceptAll }, }); - const err = yield* Effect.flip( - invoker.invoke({ path: "leaky.failsWithCause", args: {} }), - ); + const err = yield* Effect.flip(invoker.invoke({ path: "leaky.failsWithCause", args: {} })); + expect(err).toBeInstanceOf(ExecutionToolError); + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: leak test inspects the rendered message to assert it is the opaque generic const msg = (err as { message: string }).message; - // eslint-disable-next-line no-console - console.log("[leak failsWithCause]", msg); - - expect(msg).toBe("HTTP request failed"); + // Must be the canonical opaque shape: "Internal tool error []" + expect(msg).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); + // Crucially, no internal context leaks expect(msg).not.toContain("SECRET_TOKEN_xyz"); expect(msg).not.toContain("p@ssw0rd"); expect(msg).not.toContain("packages/plugins"); expect(msg).not.toContain("HttpClientError"); + expect(msg).not.toContain("HTTP request failed"); }), ); - it.effect("plain Error with stack: stack does NOT leak, only message", () => + it.effect("plain Error with stack: stack and message do NOT escape", () => Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [leakyPlugin()] as const }), - ); + const executor = yield* createExecutor(makeTestConfig({ plugins: [leakyPlugin()] as const })); const invoker = makeExecutorToolInvoker(executor, { invokeOptions: { onElicitation: acceptAll }, }); - const err = yield* Effect.flip( - invoker.invoke({ path: "leaky.throwsRawError", args: {} }), - ); + const err = yield* Effect.flip(invoker.invoke({ path: "leaky.throwsRawError", args: {} })); + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: leak test inspects the rendered message to assert it is the opaque generic const msg = (err as { message: string }).message; - // eslint-disable-next-line no-console - console.log("[leak throwsRawError]", msg); - - // message itself contains the secret because the plugin put it there — - // that's plugin discipline. But stack and file path should not appear. + expect(msg).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); expect(msg).not.toContain("secret-store.ts"); expect(msg).not.toContain("at /home/"); + expect(msg).not.toContain("sk_live_abcd"); }), ); }); diff --git a/packages/core/execution/src/tool-invoker.repro.test.ts b/packages/core/execution/src/tool-invoker.repro.test.ts index 973a6e890..9f7895640 100644 --- a/packages/core/execution/src/tool-invoker.repro.test.ts +++ b/packages/core/execution/src/tool-invoker.repro.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; import { Effect, Schema } from "effect"; -import { ElicitationResponse, createExecutor, definePlugin } from "@executor-js/sdk"; +import { ElicitationResponse, ToolResult, createExecutor, definePlugin } from "@executor-js/sdk"; import { makeTestConfig } from "@executor-js/sdk/testing"; import { makeExecutorToolInvoker } from "./tool-invoker"; @@ -11,8 +11,11 @@ const EmptyInputSchema = Schema.toStandardSchemaV1( const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); -// Simulate the kind of error envelopes real upstreams (SharePoint, DealCloud, -// Microsoft Graph, etc.) actually return. +// Plugins now emit ToolResult directly. Mirrors the structured upstream +// payloads each real plugin extracts a top-line message from — the +// invoker passes the whole ToolResult through unchanged so the model +// in the sandbox sees `r.ok === false` and `r.error.details` carrying +// the full body. const upstreamErrorPlugin = definePlugin(() => ({ id: "upstream-error-test" as const, storage: () => ({}), @@ -28,16 +31,19 @@ const upstreamErrorPlugin = definePlugin(() => ({ description: "", inputSchema: EmptyInputSchema, handler: () => - Effect.succeed({ - data: null, - error: { - error: { - code: "invalidRequest", - message: - "The expression \"foo\" is not valid. Provide a valid expression.", + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 400, + message: 'The expression "foo" is not valid. Provide a valid expression.', + details: { + error: { + code: "invalidRequest", + message: 'The expression "foo" is not valid. Provide a valid expression.', + }, }, - }, - }), + }), + ), }, { // DealCloud-ish shape: errorCode + errorMessage @@ -45,13 +51,17 @@ const upstreamErrorPlugin = definePlugin(() => ({ description: "", inputSchema: EmptyInputSchema, handler: () => - Effect.succeed({ - data: null, - error: { - errorCode: 400, - errorMessage: "Entity 'Deals' has no field 'XYZ'", - }, - }), + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 400, + message: "Entity 'Deals' has no field 'XYZ'", + details: { + errorCode: 400, + errorMessage: "Entity 'Deals' has no field 'XYZ'", + }, + }), + ), }, { // JSON:API / multi-errors shape @@ -59,51 +69,35 @@ const upstreamErrorPlugin = definePlugin(() => ({ description: "", inputSchema: EmptyInputSchema, handler: () => - Effect.succeed({ - data: null, - error: { - errors: [ - { status: "403", title: "Forbidden", detail: "Insufficient scope" }, - ], - }, - }), - }, - { - // Plain string body - name: "stringShape", - description: "", - inputSchema: EmptyInputSchema, - handler: () => - Effect.succeed({ - data: null, - error: "Internal Server Error", - }), + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 403, + message: "Insufficient scope", + details: { + errors: [{ status: "403", title: "Forbidden", detail: "Insufficient scope" }], + }, + }), + ), }, ], }, ], })); -describe("repro: opaque tool execution failures", () => { - it.effect("SharePoint/Graph nested error.message is LOST -> 'Tool execution failed'", () => - Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), - ); - const invoker = makeExecutorToolInvoker(executor, { - invokeOptions: { onElicitation: acceptAll }, - }); - - const err = yield* Effect.flip( - invoker.invoke({ path: "upstream.sharepointShape", args: {} }), - ); - // eslint-disable-next-line no-console - console.log("[repro sharepoint]", (err as { message: string }).message); - expect((err as { message: string }).message).toBe("Tool execution failed"); - }), - ); +const isFailedToolResult = ( + value: unknown, +): value is { + readonly ok: false; + readonly error: { readonly code: string; readonly message: string; readonly details?: unknown }; +} => + value !== null && + typeof value === "object" && + "ok" in value && + (value as { ok: unknown }).ok === false; - it.effect("DealCloud errorMessage is LOST -> 'Tool execution failed'", () => +describe("regression: structured upstream failures surface through ToolResult", () => { + it.effect("SharePoint/Graph nested error.message reaches the sandbox via ToolResult.fail", () => Effect.gen(function* () { const executor = yield* createExecutor( makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), @@ -112,16 +106,23 @@ describe("repro: opaque tool execution failures", () => { invokeOptions: { onElicitation: acceptAll }, }); - const err = yield* Effect.flip( - invoker.invoke({ path: "upstream.dealcloudShape", args: {} }), + const result = yield* invoker.invoke({ path: "upstream.sharepointShape", args: {} }); + expect(isFailedToolResult(result)).toBe(true); + if (!isFailedToolResult(result)) return; + expect(result.error.code).toBe("upstream_http_error"); + expect(result.error.message).toBe( + 'The expression "foo" is not valid. Provide a valid expression.', ); - // eslint-disable-next-line no-console - console.log("[repro dealcloud]", (err as { message: string }).message); - expect((err as { message: string }).message).toBe("Tool execution failed"); + expect(result.error.details).toEqual({ + error: { + code: "invalidRequest", + message: 'The expression "foo" is not valid. Provide a valid expression.', + }, + }); }), ); - it.effect("JSON:API errors[] is LOST -> 'Tool execution failed'", () => + it.effect("DealCloud errorMessage reaches the sandbox via ToolResult.fail", () => Effect.gen(function* () { const executor = yield* createExecutor( makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), @@ -130,16 +131,18 @@ describe("repro: opaque tool execution failures", () => { invokeOptions: { onElicitation: acceptAll }, }); - const err = yield* Effect.flip( - invoker.invoke({ path: "upstream.errorsArrayShape", args: {} }), - ); - // eslint-disable-next-line no-console - console.log("[repro errors-array]", (err as { message: string }).message); - expect((err as { message: string }).message).toBe("Tool execution failed"); + const result = yield* invoker.invoke({ path: "upstream.dealcloudShape", args: {} }); + expect(isFailedToolResult(result)).toBe(true); + if (!isFailedToolResult(result)) return; + expect(result.error.message).toBe("Entity 'Deals' has no field 'XYZ'"); + expect(result.error.details).toMatchObject({ + errorCode: 400, + errorMessage: "Entity 'Deals' has no field 'XYZ'", + }); }), ); - it.effect("plain string error body is preserved", () => + it.effect("JSON:API errors[] reaches the sandbox via ToolResult.fail", () => Effect.gen(function* () { const executor = yield* createExecutor( makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), @@ -148,12 +151,13 @@ describe("repro: opaque tool execution failures", () => { invokeOptions: { onElicitation: acceptAll }, }); - const err = yield* Effect.flip( - invoker.invoke({ path: "upstream.stringShape", args: {} }), - ); - // eslint-disable-next-line no-console - console.log("[repro string]", (err as { message: string }).message); - expect((err as { message: string }).message).toBe("Internal Server Error"); + const result = yield* invoker.invoke({ path: "upstream.errorsArrayShape", args: {} }); + expect(isFailedToolResult(result)).toBe(true); + if (!isFailedToolResult(result)) return; + expect(result.error.message).toBe("Insufficient scope"); + expect(result.error.details).toMatchObject({ + errors: [{ detail: "Insufficient scope" }], + }); }), ); }); diff --git a/packages/core/execution/src/tool-invoker.test.ts b/packages/core/execution/src/tool-invoker.test.ts index 686573407..ffb05224e 100644 --- a/packages/core/execution/src/tool-invoker.test.ts +++ b/packages/core/execution/src/tool-invoker.test.ts @@ -4,6 +4,7 @@ import { Effect, Fiber, Schema } from "effect"; import { ElicitationResponse, FormElicitation, + ToolResult, createExecutor, definePlugin, } from "@executor-js/sdk"; @@ -106,13 +107,12 @@ const errorPlugin = definePlugin(() => ({ description: "Query rows", inputSchema: EmptyInputSchema, handler: () => - Effect.succeed({ - data: null, - error: { - message: 'Field with name "DisplayName" does not exist', + Effect.succeed( + ToolResult.fail({ code: "invalid_query", - }, - }), + message: 'Field with name "DisplayName" does not exist', + }), + ), }, ], }, @@ -405,20 +405,21 @@ describe("tool discovery", () => { }), ); - it.effect("converts message-bearing tool error results into execution errors", () => + it.effect("passes ToolResult.fail through to the sandbox as a value (no throw)", () => Effect.gen(function* () { const executor = yield* createExecutor(makeTestConfig({ plugins: [errorPlugin()] as const })); const invoker = makeExecutorToolInvoker(executor, { invokeOptions: { onElicitation: acceptAll }, }); - const error = yield* Effect.flip(invoker.invoke({ path: "records.queryRows", args: {} })); - - expect(error).toEqual( - expect.objectContaining({ + const result = yield* invoker.invoke({ path: "records.queryRows", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "invalid_query", message: 'Field with name "DisplayName" does not exist', - }), - ); + }, + }); }), ); }); diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index 61cb7e2c8..144064c19 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -1,4 +1,4 @@ -import { Effect, Match, Option, Predicate } from "effect"; +import { Effect, Predicate } from "effect"; import * as Cause from "effect/Cause"; import type { Executor, @@ -12,6 +12,16 @@ import { isToolResult } from "@executor-js/sdk/core"; import type { SandboxToolInvoker } from "@executor-js/codemode-core"; import { ExecutionToolError } from "./errors"; +const OPAQUE_DEFECT_MESSAGE = "Internal tool error"; + +const newCorrelationId = (): string => { + // 8-hex-char correlation id; enough entropy to disambiguate within a + // single deployment without leaking host process info. + return Math.floor(Math.random() * 0x1_0000_0000) + .toString(16) + .padStart(8, "0"); +}; + /** * Extract the source namespace from a tool path. Tool paths look like * "." or ".." — we take the first @@ -24,108 +34,6 @@ const extractSourceNamespace = (path: string): string => { return idx === -1 ? path : path.slice(0, idx); }; -const hasStringMessage = (value: unknown): value is { readonly message: string } => - value !== null && - typeof value === "object" && - "message" in value && - typeof value.message === "string"; - -const messageFromErrorLike = (value: unknown): string | undefined => { - if (hasStringMessage(value)) { - return value.message; - } - return undefined; -}; - -// Boundary: `.catchCause` branch — `err` is an internal/typed plugin error. -// Keep `.message`-only discipline; never walk `.cause`/stack/structured body. -const renderCauseErrorMessage = (error: unknown): string => - messageFromErrorLike(error) ?? - (typeof error === "undefined" ? "Tool execution failed" : renderUnknownPrimitive(error)); - -const renderUnknownPrimitive = (value: unknown): string => - Match.value(value).pipe( - Match.when(Match.string, (s) => s), - Match.whenOr(Match.number, Match.boolean, Match.bigint, Match.symbol, (x) => x.toString()), - Match.option, - Option.getOrElse(() => "Tool execution failed"), - ); - -type LegacyToolResultEnvelope = { - readonly error?: unknown; - readonly data?: unknown; -}; - -const isLegacyToolResultEnvelope = (value: unknown): value is LegacyToolResultEnvelope => - value !== null && typeof value === "object" && ("error" in value || "data" in value); - -const hasLegacyToolResultError = ( - value: LegacyToolResultEnvelope, -): value is LegacyToolResultEnvelope & { readonly error: unknown } => - value.error !== null && value.error !== undefined; - -const STRINGIFIED_BODY_CAP = 1024; - -// Boundary: legacy envelope branch — `body` is a domain-level structured -// upstream error body returned by the handler in a `data: null, error: ...` -// envelope. Walk known upstream shapes (Microsoft Graph, DealCloud, -// JSON:API, etc.) before falling back to a clamped JSON.stringify. -const extractLegacyEnvelopeMessage = (body: unknown): string => { - if (typeof body === "string") { - return body.length === 0 ? "Tool execution failed" : body; - } - if (body === null || typeof body !== "object") { - return renderUnknownPrimitive(body); - } - - const obj = body as Record; - - // Microsoft Graph / SharePoint: { error: { code, message } } - const nested = obj.error; - if (nested !== null && typeof nested === "object" && "message" in nested) { - const m = (nested as { message: unknown }).message; - if (typeof m === "string" && m.length > 0) return m; - } - - // Plain { message: ... } - if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; - - // DealCloud-ish: { errorCode, errorMessage } - if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) return obj.errorMessage; - - // JSON:API multi-errors: { errors: [{ detail|message|title, ... }] } - if (Array.isArray(obj.errors) && obj.errors.length > 0) { - const first = obj.errors[0]; - if (first !== null && typeof first === "object") { - const f = first as Record; - for (const key of ["detail", "message", "title"]) { - const v = f[key]; - if (typeof v === "string" && v.length > 0) return v; - } - } - } - - for (const key of ["detail", "title", "description"]) { - const v = obj[key]; - if (typeof v === "string" && v.length > 0) return v; - } - - return clampedStringify(body); -}; - -const clampedStringify = (value: unknown): string => { - let s: string; - try { - s = JSON.stringify(value); - } catch { - s = String(value); - } - if (s.length > STRINGIFIED_BODY_CAP) { - return `${s.slice(0, STRINGIFIED_BODY_CAP)}…`; - } - return s; -}; - /** * Bridges QuickJS `tools.someSource.someOp(args)` calls into * `executor.tools.invoke(toolId, args)`. @@ -152,44 +60,46 @@ export const makeExecutorToolInvoker = ( const result = yield* executor.tools.invoke(path as ToolId, args, options.invokeOptions).pipe( Effect.catchCause((cause): Effect.Effect => { const err = cause.reasons.find(Cause.isFailReason)?.error; - if (!isElicitationDeclinedError(err)) { + if (isElicitationDeclinedError(err)) { return Effect.fail( new ExecutionToolError({ - message: renderCauseErrorMessage(err), - cause: err ?? cause, + message: `Tool "${err.toolId}" requires approval but the request was ${err.action === "cancel" ? "cancelled" : "declined"} by the user.`, + cause: err, }), ); } - return Effect.fail( - new ExecutionToolError({ - message: `Tool "${err.toolId}" requires approval but the request was ${err.action === "cancel" ? "cancelled" : "declined"} by the user.`, - cause: err, + // Any other failure here is an infra/plugin defect. Emit an + // opaque generic with a correlation id so internal context (URLs + // with tokens, DB connection strings, file paths in stacks) + // can't leak through Error.message into the sandbox. The full + // cause is logged with the same correlation id so operators can + // still trace the failure. + const correlationId = newCorrelationId(); + return Effect.logError("tool dispatch failed", cause).pipe( + Effect.annotateLogs({ + "executor.correlation_id": correlationId, + "mcp.tool.name": path, }), + Effect.flatMap(() => + Effect.fail( + new ExecutionToolError({ + message: `${OPAQUE_DEFECT_MESSAGE} [${correlationId}]`, + cause: err ?? cause, + }), + ), + ), ); }), ); - // New typed-union path. Pass the whole `ToolResult` through; user - // sandbox code branches on `r.ok`. + // Strict: plugins emit ToolResult. Anything else is treated as a + // raw success value and wrapped — keeps the sandbox-facing contract + // uniform without forcing every tiny test plugin to import + // `ToolResult.ok`. if (isToolResult(result)) { return result; } - - // Legacy envelope shim. Translates the old `{ data, error }` shape - // into an Effect failure so existing user code keeps throwing on - // domain errors. Phase 3 deletes this branch outright. - if (isLegacyToolResultEnvelope(result)) { - if (hasLegacyToolResultError(result)) { - return yield* new ExecutionToolError({ - message: extractLegacyEnvelopeMessage(result.error), - cause: result.error, - }); - } - if ("data" in result) { - return result.data; - } - } - return result; + return { ok: true, data: result }; }), }); diff --git a/packages/core/sdk/src/tool-result.test.ts b/packages/core/sdk/src/tool-result.test.ts new file mode 100644 index 000000000..c253203b5 --- /dev/null +++ b/packages/core/sdk/src/tool-result.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "@effect/vitest"; +import { ToolResult, isToolResult } from "./tool-result"; + +describe("ToolResult", () => { + it("ok wraps a value", () => { + const r = ToolResult.ok({ count: 3 }); + expect(r).toEqual({ ok: true, data: { count: 3 } }); + expect(isToolResult(r)).toBe(true); + }); + + it("fail wraps a ToolError", () => { + const r = ToolResult.fail({ + code: "upstream_http_error", + status: 404, + message: "Not found", + details: { id: "x" }, + }); + expect(r).toEqual({ + ok: false, + error: { + code: "upstream_http_error", + status: 404, + message: "Not found", + details: { id: "x" }, + }, + }); + expect(isToolResult(r)).toBe(true); + }); + + it("isToolResult rejects unrelated shapes", () => { + expect(isToolResult(null)).toBe(false); + expect(isToolResult({})).toBe(false); + expect(isToolResult({ ok: true })).toBe(false); + expect(isToolResult({ ok: false })).toBe(false); + expect(isToolResult({ ok: false, error: {} })).toBe(false); + expect(isToolResult({ ok: false, error: { code: 1, message: "x" } })).toBe(false); + expect(isToolResult({ ok: "yes", data: 1 })).toBe(false); + }); + + it("isToolResult accepts both branches of the union", () => { + expect(isToolResult({ ok: true, data: 1 })).toBe(true); + expect(isToolResult({ ok: true, data: null })).toBe(true); + expect( + isToolResult({ + ok: false, + error: { code: "x", message: "y" }, + }), + ).toBe(true); + }); +}); diff --git a/packages/core/sdk/src/tool-result.ts b/packages/core/sdk/src/tool-result.ts index ec6016661..297479e88 100644 --- a/packages/core/sdk/src/tool-result.ts +++ b/packages/core/sdk/src/tool-result.ts @@ -22,7 +22,6 @@ export const ToolResult = { fail: (error: ToolError): ToolResult => ({ ok: false, error }), } as const; - export const isToolResult = (value: unknown): value is ToolResult => { if (value === null || typeof value !== "object") return false; if (!("ok" in value)) return false; @@ -31,14 +30,11 @@ export const isToolResult = (value: unknown): value is ToolResult => { if (ok === false) { if (!("error" in value)) return false; const error = (value as { error: unknown }).error; - return ( - error !== null && - typeof error === "object" && - "code" in error && - "message" in error && - typeof (error as { code: unknown }).code === "string" && - typeof (error as { message: unknown }).message === "string" - ); + if (error === null || typeof error !== "object") return false; + if (!("code" in error) || !("message" in error)) return false; + const errorObj = error as { readonly code: unknown; readonly message: unknown }; + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: structural type guard; `message` is a required ToolError field, not a thrown JS error + return typeof errorObj.code === "string" && typeof errorObj.message === "string"; } return false; }; diff --git a/packages/hosts/mcp/src/server.test.ts b/packages/hosts/mcp/src/server.test.ts index f6663bc0d..606487659 100644 --- a/packages/hosts/mcp/src/server.test.ts +++ b/packages/hosts/mcp/src/server.test.ts @@ -116,7 +116,7 @@ describe("MCP host server — client with elicitation", () => { }); }); - it("execute tool resolves failed engine effects as MCP error results", async () => { + it("execute tool surfaces failed engine effects as an opaque generic with correlation id", async () => { const engine = makeStubEngine({ execute: () => Effect.fail(new TestExecutionError({ message: "Unexpected token ':'" })), }); @@ -126,11 +126,11 @@ describe("MCP host server — client with elicitation", () => { name: "execute", arguments: { code: "const x: any = 1;" }, }); - expect(textOf(result)).toBe("Error: Unexpected token ':'"); - expect(result.structuredContent).toEqual({ - status: "error", - error: "Unexpected token ':'", - }); + const text = textOf(result); + expect(text).toMatch(/^Error: Internal tool error \[[0-9a-f]{8}\]$/); + expect(text).not.toContain("Unexpected token"); + const structured = (result.structuredContent as { readonly error?: string }).error ?? ""; + expect(structured).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); expect(result.isError).toBe(true); }); }); @@ -146,11 +146,16 @@ describe("MCP host server — client with elicitation", () => { name: "execute", arguments: { code: "run" }, }); - expect(textOf(result)).toBe("Error: Tool execution failed"); - expect(result.structuredContent).toEqual({ + const text = textOf(result); + expect(text).toMatch(/^Error: Internal tool error \[[0-9a-f]{8}\]$/); + // Sensitive internal context must NOT leak through the MCP error path. + expect(text).not.toContain("secret internal detail"); + expect(result.structuredContent).toMatchObject({ status: "error", - error: "Tool execution failed", }); + const structuredError = (result.structuredContent as { readonly error?: string }).error ?? ""; + expect(structuredError).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); + expect(structuredError).not.toContain("secret internal detail"); expect(result.isError).toBe(true); }); }); diff --git a/packages/hosts/mcp/src/server.ts b/packages/hosts/mcp/src/server.ts index abda5f550..5b6342e8d 100644 --- a/packages/hosts/mcp/src/server.ts +++ b/packages/hosts/mcp/src/server.ts @@ -260,20 +260,28 @@ const toMcpPausedResult = (formatted: ReturnType): structuredContent: formatted.structured, }); -const formatFailureMessage = (value: unknown): string | null => { - if (typeof value === "object" && value !== null && "message" in value) { - const message = (value as { readonly message?: unknown }).message; - if (typeof message === "string" && message.length > 0) return message; - } - if (typeof value === "string" && value.length > 0) return value; - return null; -}; +// `execute` failures reaching the MCP host are infra defects — domain +// failures from tools are now expressed as `ToolResult` values (success +// channel) and flow through `formatExecuteResult`. Emit an opaque +// generic plus a fresh correlation id and log the cause out-of-band so +// the model can't read internal context off `.message`. +const newCorrelationId = (): string => + Math.floor(Math.random() * 0x1_0000_0000) + .toString(16) + .padStart(8, "0"); const toMcpFailureResult = (cause: Cause.Cause): McpToolResult => { - const failure = cause.reasons.find(Cause.isFailReason); - const text = failure - ? (formatFailureMessage(failure.error) ?? "Tool execution failed") - : "Tool execution failed"; + const correlationId = newCorrelationId(); + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: best-effort defect logging must tolerate non-serializable causes + try { + console.error( + `[executor:mcp] execute defect correlation_id=${correlationId}`, + Cause.pretty(cause), + ); + } catch { + /* ignore logger failures */ + } + const text = `Internal tool error [${correlationId}]`; return { content: [{ type: "text", text: `Error: ${text}` }], structuredContent: { status: "error", error: text }, diff --git a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts index 3b48e0fd6..8106277a1 100644 --- a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts +++ b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts @@ -246,7 +246,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.result).toBe(7); }); - it("surfaces tool errors in execution result", async () => { + it("surfaces infra defects through the worker bridge as an opaque generic", async () => { const executor = makeDynamicWorkerExecutor({ loader }); const invoker = failingInvoker("not authorized"); @@ -254,7 +254,7 @@ describe("makeDynamicWorkerExecutor", () => { executor.execute("async () => { return await tools.secret.read({}); }", invoker), ); - expect(result.error).toBe("not authorized"); + expect(result.error).toBe("Internal tool error"); }); it("does not expose host error stack details to sandbox error handlers", async () => { @@ -284,11 +284,12 @@ describe("makeDynamicWorkerExecutor", () => { ); expect(result.error).toBeUndefined(); - expect(result.result).toMatchObject({ message: "not authorized" }); + expect(result.result).toMatchObject({ message: "Internal tool error" }); expect((result.result as { stack?: string }).stack).not.toContain("secret host stack"); + expect((result.result as { message?: string }).message).not.toContain("not authorized"); }); - it("surfaces object-shaped tool errors in execution result", async () => { + it("collapses object-shaped tool defects to an opaque generic", async () => { const executor = makeDynamicWorkerExecutor({ loader }); const invoker = { invoke: () => @@ -302,11 +303,11 @@ describe("makeDynamicWorkerExecutor", () => { executor.execute("async () => { return await tools.secret.read({}); }", invoker), ); - expect(result.error).toBe('{"code":"forbidden","detail":"missing team access"}'); + expect(result.error).toBe("Internal tool error"); expect(result.result).toBeNull(); }); - it("surfaces message-bearing object tool errors in execution result", async () => { + it("collapses message-bearing object tool defects to an opaque generic", async () => { const executor = makeDynamicWorkerExecutor({ loader }); const invoker = { invoke: () => @@ -320,7 +321,8 @@ describe("makeDynamicWorkerExecutor", () => { executor.execute("async () => { return await tools.records.query({}); }", invoker), ); - expect(result.error).toBe('Field with name "DisplayName" does not exist'); + expect(result.error).toBe("Internal tool error"); + expect(result.error).not.toContain("DisplayName"); expect(result.result).toBeNull(); }); @@ -366,7 +368,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.error).toBe("Tool RPC payload contains a circular reference"); }); - it("returns an execution error for circular tool results", async () => { + it("returns an opaque generic when a tool result can't be serialized", async () => { const executor = makeDynamicWorkerExecutor({ loader }); const cyclic: Record = {}; cyclic.self = cyclic; @@ -377,7 +379,7 @@ describe("makeDynamicWorkerExecutor", () => { ); expect(result.result).toBeNull(); - expect(result.error).toBe("Tool RPC payload contains a circular reference"); + expect(result.error).toBe("Internal tool error"); }); it("respects timeout", async () => { diff --git a/packages/kernel/runtime-dynamic-worker/src/module-template.ts b/packages/kernel/runtime-dynamic-worker/src/module-template.ts index 23627e722..3cc8fabaa 100644 --- a/packages/kernel/runtime-dynamic-worker/src/module-template.ts +++ b/packages/kernel/runtime-dynamic-worker/src/module-template.ts @@ -138,7 +138,7 @@ export const buildExecutorModule = (body: string, timeoutMs: number): string => " return (async () => {", " const encoded = await __encodeBinary(args[0]);", " const data = await __dispatcher.call(toolPath, encoded);", - " if (!data.ok) throw new Error(data.error && data.error.message ? data.error.message : 'Tool execution failed');", + " if (!data.ok) throw new Error(data.error && typeof data.error.message === 'string' && data.error.message.startsWith('Internal tool error') ? data.error.message : 'Internal tool error');", " return __decodeBinary(data.result);", " })();", " },", diff --git a/packages/kernel/runtime-quickjs/src/index.test.ts b/packages/kernel/runtime-quickjs/src/index.test.ts index 4435d246b..30954f7eb 100644 --- a/packages/kernel/runtime-quickjs/src/index.test.ts +++ b/packages/kernel/runtime-quickjs/src/index.test.ts @@ -121,6 +121,52 @@ describe("quickjs executor", () => { }), ); + it.effect("internal defects reach the sandbox as an opaque generic only", () => + Effect.gen(function* () { + // Plugin defect carrying sensitive context. The bridge's reject + // path must strip everything except the canonical + // "Internal tool error []" shape — or fall back to the + // bare generic if the upstream invoker hasn't already stamped + // the correlation id (this test exercises the latter path + // because it bypasses makeExecutorToolInvoker). + const invoker: SandboxToolInvoker = { + invoke: () => + Effect.fail( + Object.assign( + new Error("Authorization: Bearer SECRET_TOKEN_xyz failed against host 10.0.0.5"), + { + stack: "Error\n at /home/svc/executor/packages/plugins/foo:142:11", + }, + ) as never, + ), + }; + + const result = yield* executor.execute( + ` + try { + await tools.leaky.call({}); + return "should not reach"; + } catch (e) { + return e.message; + } + `, + invoker, + ); + + expect(result.error).toBeUndefined(); + const message = String(result.result); + // Either the canonical opaque generic with a correlation id, or + // the bare fallback. Neither must contain any sensitive context. + expect( + message === "Internal tool error" || /^Internal tool error \[[0-9a-f]{8}\]$/.test(message), + ).toBe(true); + expect(message).not.toContain("SECRET_TOKEN_xyz"); + expect(message).not.toContain("Authorization"); + expect(message).not.toContain("10.0.0.5"); + expect(message).not.toContain("packages/plugins"); + }), + ); + it.effect("handles unknown tool path", () => Effect.gen(function* () { const invoker = makeTestInvoker({}); diff --git a/packages/kernel/runtime-quickjs/src/index.ts b/packages/kernel/runtime-quickjs/src/index.ts index af8683b4f..d90790123 100644 --- a/packages/kernel/runtime-quickjs/src/index.ts +++ b/packages/kernel/runtime-quickjs/src/index.ts @@ -47,6 +47,30 @@ const EXECUTION_FILENAME = "executor-quickjs-runtime.js"; const toError = (cause: unknown): Error => cause instanceof Error ? cause : new Error(String(cause)); +// Defect surfaced to the sandbox when a tool dispatch reaches the +// reject path. Two sources reach here: +// - `ExecutionToolError` emitted by the execution package — message +// is already either the canonical +// `Internal tool error []` opaque generic (plugin defects +// passed through `makeExecutorToolInvoker`'s catchCause) or an +// intentional public string (validators in engine.ts: +// "tools.search expects an object: ..."). +// - Anything else — assume infra defect and emit a bare opaque +// generic. +// `_tag === "ExecutionToolError"` is the in-band signal that the +// message was generated by code we control and is safe to pass +// through; we never read `.cause` / `.stack`. +const sandboxDefectMessage = (cause: unknown): string => { + if (cause !== null && typeof cause === "object") { + const tagged = cause as { readonly _tag?: unknown; readonly message?: unknown }; + // oxlint-disable-next-line executor/no-manual-tag-check, executor/no-unknown-error-message -- boundary: QuickJS reject path receives an already-rendered ExecutionToolError; we pass its in-band public message through and otherwise emit the opaque generic + if (tagged._tag === "ExecutionToolError" && typeof tagged.message === "string") { + return tagged.message; + } + } + return "Internal tool error"; +}; + const toErrorMessage = (cause: unknown): string => { if (typeof cause === "object" && cause !== null) { const message = @@ -216,7 +240,17 @@ const createToolBridge = ( return; } - const errorHandle = context.newError(toErrorMessage(cause)); + // The reject path is reserved for true infra defects. The + // upstream tool-invoker has already replaced the message with + // an opaque generic plus a correlation id, but defensively log + // the cause here and emit a stable generic if upstream changes. + const message = sandboxDefectMessage(cause); + try { + console.error("[executor:quickjs] tool dispatch defect", { path, cause }); + } catch { + /* ignore logger failures */ + } + const errorHandle = context.newError(message); deferred.reject(errorHandle); errorHandle.dispose(); }, diff --git a/packages/plugins/google-discovery/src/sdk/plugin.ts b/packages/plugins/google-discovery/src/sdk/plugin.ts index b308d06dd..38f000cf9 100644 --- a/packages/plugins/google-discovery/src/sdk/plugin.ts +++ b/packages/plugins/google-discovery/src/sdk/plugin.ts @@ -38,6 +38,7 @@ const GOOGLE_BODY_CAP = 1024; const googleClampedStringify = (value: unknown): string => { let s: string; + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: JSON.stringify may throw on cycles; fall back to String() so the upstream body can still be surfaced as ToolError.details fallback text try { s = JSON.stringify(value); } catch { diff --git a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts index bd2bafc14..cd4fd09f0 100644 --- a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts +++ b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts @@ -115,7 +115,8 @@ describe("per-user MCP auth isolation", () => { { onElicitation: "accept-all" }, ); expect(userAResult).toMatchObject({ - ok: true, data: [{ type: "text", text: "ok:from-user-a" }], + ok: true, + data: [{ type: "text", text: "ok:from-user-a" }], }); expect( (yield* server.requests) @@ -187,7 +188,8 @@ describe("per-user MCP auth isolation", () => { { onElicitation: "accept-all" }, ); expect(userAResult).toMatchObject({ - ok: true, data: [{ type: "text", text: "ok:user-a-header" }], + ok: true, + data: [{ type: "text", text: "ok:user-a-header" }], }); expect( (yield* server.requests) @@ -267,7 +269,8 @@ describe("per-user MCP auth isolation", () => { ); expect(result).toMatchObject({ - ok: true, data: [{ type: "text", text: "ok:org-header" }], + ok: true, + data: [{ type: "text", text: "ok:org-header" }], }); const invokeRequests = (yield* server.requests).slice(beforeInvoke); expect( diff --git a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts index c4023db6d..7a4bc7982 100644 --- a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts +++ b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts @@ -227,11 +227,9 @@ describe("OpenAPI client_credentials OAuth", () => { ); // Invoking the tool injects the freshly-minted bearer via // ctx.connections.accessToken. - const result = unwrapInvocation(yield* userExec.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const result = unwrapInvocation( + yield* userExec.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(result.error).toBeNull(); const data = result.data as EchoHeaders | null; const bearer = data?.authorization?.replace(/^Bearer\s+/i, ""); diff --git a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts index f1ad02a10..55e8c4909 100644 --- a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts +++ b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts @@ -278,11 +278,9 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { // yields her token; bob's scope yields his. Same source, same // tool, different injected bearer. // ------------------------------------------------------------- - const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "vercel.projects.list", - {}, - autoApprove, - )); + const aliceResult = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(aliceResult.error).toBeNull(); const aliceData = aliceResult.data as EchoHeaders | null; expect(aliceData?.authorization).toBe("Bearer alice-vercel-token"); @@ -449,11 +447,9 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "vercel.projects.list", - {}, - autoApprove, - )); + const aliceResult = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(aliceResult.error).toBeNull(); expect((aliceResult.data as EchoHeaders | null)?.authorization).toBe( "Bearer alice-vercel-token", @@ -560,11 +556,9 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const sharedResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "vercel.projects.list", - {}, - autoApprove, - )); + const sharedResult = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(sharedResult.error).toBeNull(); expect((sharedResult.data as EchoHeaders | null)?.authorization).toBe("Bearer org-token"); @@ -589,11 +583,9 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { }), ); - const overrideResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "vercel.projects.list", - {}, - autoApprove, - )); + const overrideResult = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(overrideResult.error).toBeNull(); expect((overrideResult.data as EchoHeaders | null)?.authorization).toBe( "Bearer alice-token", @@ -606,11 +598,9 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { String(aliceScope.id), ); - const fallbackResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "vercel.projects.list", - {}, - autoApprove, - )); + const fallbackResult = unwrapInvocation( + yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), + ); expect(fallbackResult.error).toBeNull(); expect((fallbackResult.data as EchoHeaders | null)?.authorization).toBe( "Bearer org-token", diff --git a/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts b/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts index d9efbfe45..85cde6e4d 100644 --- a/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts +++ b/packages/plugins/openapi/src/sdk/multi-scope-oauth.test.ts @@ -308,11 +308,9 @@ describe("OpenAPI multi-scope OAuth", () => { // 4. Invoke through each exec — Authorization must carry that // user's token. // ------------------------------------------------------------- - const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const aliceResult = unwrapInvocation( + yield* aliceExec.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(aliceResult.error).toBeNull(); const aliceBearer = (aliceResult.data as EchoHeaders | null)?.authorization?.replace( /^Bearer\s+/i, @@ -321,11 +319,9 @@ describe("OpenAPI multi-scope OAuth", () => { expect(aliceBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(aliceBearer!)).toBe(true); - const bobResult = unwrapInvocation(yield* bobExec.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const bobResult = unwrapInvocation( + yield* bobExec.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(bobResult.error).toBeNull(); const bobBearer = (bobResult.data as EchoHeaders | null)?.authorization?.replace( /^Bearer\s+/i, @@ -617,11 +613,9 @@ describe("OpenAPI multi-scope OAuth", () => { // (4) Each user's invocation resolves their OWN row and gets // their OWN token — not whatever the last signer happened to // mint. This is the core multi-user regression. - const aliceResult = unwrapInvocation(yield* aliceExec.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const aliceResult = unwrapInvocation( + yield* aliceExec.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(aliceResult.error).toBeNull(); const aliceBearer = (aliceResult.data as EchoHeaders | null)?.authorization?.replace( /^Bearer\s+/i, @@ -630,11 +624,9 @@ describe("OpenAPI multi-scope OAuth", () => { expect(aliceBearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(aliceBearer!)).toBe(true); - const bobResult = unwrapInvocation(yield* bobExec.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const bobResult = unwrapInvocation( + yield* bobExec.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(bobResult.error).toBeNull(); const bobBearer = (bobResult.data as EchoHeaders | null)?.authorization?.replace( /^Bearer\s+/i, diff --git a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts index bc21bf038..930e61b20 100644 --- a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts +++ b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts @@ -254,11 +254,9 @@ describe("OpenAPI oauth refresh", () => { }); yield* bindOAuthConnection(executor, scopeId, "conn-refresh-ok", auth); - const result = unwrapInvocation(yield* executor.tools.invoke( - "petstore.items.echoHeaders", - {}, - autoApprove, - )); + const result = unwrapInvocation( + yield* executor.tools.invoke("petstore.items.echoHeaders", {}, autoApprove), + ); expect(result.error).toBeNull(); const data = result.data as EchoHeaders | null; diff --git a/packages/plugins/openapi/src/sdk/plugin.test.ts b/packages/plugins/openapi/src/sdk/plugin.test.ts index e569c1d01..a8619aa02 100644 --- a/packages/plugins/openapi/src/sdk/plugin.test.ts +++ b/packages/plugins/openapi/src/sdk/plugin.test.ts @@ -558,11 +558,9 @@ describe("OpenAPI Plugin", () => { }, }); - const result = unwrapInvocation(yield* executor.tools.invoke( - "authed.items.echoHeaders", - {}, - autoApprove, - )); + const result = unwrapInvocation( + yield* executor.tools.invoke("authed.items.echoHeaders", {}, autoApprove), + ); expect(result.error).toBeNull(); const data = result.data as { authorization?: string; "x-static"?: string }; @@ -777,11 +775,9 @@ describe("OpenAPI Plugin", () => { namespace: "test", }); - const result = unwrapInvocation(yield* executor.tools.invoke( - "test.items.getItem", - { itemId: "2" }, - autoApprove, - )); + const result = unwrapInvocation( + yield* executor.tools.invoke("test.items.getItem", { itemId: "2" }, autoApprove), + ); expect(result.error).toBeNull(); expect(result.data).toEqual({ id: 2, name: "Gadget" }); }), @@ -807,16 +803,18 @@ describe("OpenAPI Plugin", () => { namespace: "records", }); - const result = unwrapInvocation(yield* executor.tools.invoke( - "records.items.queryRows", - { - entryTypeId: "18538", - query: JSON.stringify([{ DisplayName: "Example" }]), - limit: 10, - skip: 0, - }, - autoApprove, - )); + const result = unwrapInvocation( + yield* executor.tools.invoke( + "records.items.queryRows", + { + entryTypeId: "18538", + query: JSON.stringify([{ DisplayName: "Example" }]), + limit: 10, + skip: 0, + }, + autoApprove, + ), + ); expect(result.data).toBeNull(); expect(result.error).toEqual( diff --git a/packages/plugins/openapi/src/sdk/plugin.ts b/packages/plugins/openapi/src/sdk/plugin.ts index daefd8095..3e9428354 100644 --- a/packages/plugins/openapi/src/sdk/plugin.ts +++ b/packages/plugins/openapi/src/sdk/plugin.ts @@ -62,6 +62,7 @@ const STRINGIFIED_BODY_CAP = 1024; const clampedStringify = (value: unknown): string => { let s: string; + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: JSON.stringify may throw on cycles; fall back to String() so the upstream body can still be surfaced as ToolError.details fallback text try { s = JSON.stringify(value); } catch { diff --git a/packages/plugins/openapi/src/testing/index.ts b/packages/plugins/openapi/src/testing/index.ts index 56fe14d09..21eb77d49 100644 --- a/packages/plugins/openapi/src/testing/index.ts +++ b/packages/plugins/openapi/src/testing/index.ts @@ -627,25 +627,30 @@ export const TestLayers = { // keep working. // --------------------------------------------------------------------------- -export interface LegacyInvocationEnvelope { +export interface LegacyInvocationEnvelope | unknown[] | null> { readonly status: number | null; readonly headers: Record | null; - readonly data: unknown; + readonly data: TData; readonly error: unknown; } -export const unwrapInvocation = (raw: unknown): LegacyInvocationEnvelope => { +export const unwrapInvocation = | null>( + raw: unknown, +): LegacyInvocationEnvelope => { if (raw === null || typeof raw !== "object" || !("ok" in raw)) { return { status: null, headers: null, - data: raw, + data: raw as TData, error: null, }; } const r = raw as | { readonly ok: true; readonly data: unknown } - | { readonly ok: false; readonly error: { readonly status?: number; readonly details?: unknown } }; + | { + readonly ok: false; + readonly error: { readonly status?: number; readonly details?: unknown }; + }; if (r.ok) { const inner = r.data; if ( @@ -663,7 +668,7 @@ export const unwrapInvocation = (raw: unknown): LegacyInvocationEnvelope => { return { status: wrapped.status, headers: wrapped.headers, - data: wrapped.data, + data: wrapped.data as TData, error: null, }; } @@ -672,14 +677,14 @@ export const unwrapInvocation = (raw: unknown): LegacyInvocationEnvelope => { return { status: null, headers: null, - data: inner, + data: inner as TData, error: null, }; } return { status: r.error.status ?? null, headers: null, - data: null, + data: null as TData, error: r.error.details ?? r.error, }; }; From e38616a3b038db9800946667fd6bd0a2a8f536e9 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 16 May 2026 21:09:07 -0700 Subject: [PATCH 4/4] Fix tool result contracts --- .../src/services/sources-api.node.test.ts | 2 +- .../server/migrate-oauth-connections.test.ts | 48 +-- packages/core/execution/src/description.ts | 1 + .../execution/src/tool-invoker.leak.test.ts | 33 +- .../execution/src/tool-invoker.repro.test.ts | 163 ---------- .../core/execution/src/tool-invoker.test.ts | 298 +++++++++++++++++- packages/core/execution/src/tool-invoker.ts | 16 +- packages/core/sdk/src/tool-result.ts | 43 ++- packages/kernel/core/src/strip-types.test.ts | 4 +- .../src/invocation.test.ts | 22 ++ .../src/module-template.ts | 10 +- ...json-repro.test.ts => option-json.test.ts} | 8 +- .../google-discovery/src/sdk/plugin.ts | 55 ++-- .../plugins/graphql/src/sdk/plugin.test.ts | 43 ++- packages/plugins/graphql/src/sdk/plugin.ts | 40 ++- .../plugins/mcp/src/react/AddMcpSource.tsx | 27 +- .../plugins/mcp/src/sdk/elicitation.test.ts | 30 +- .../src/sdk/per-user-auth-isolation.test.ts | 6 +- packages/plugins/mcp/src/sdk/plugin.ts | 35 +- packages/plugins/mcp/src/testing/server.ts | 17 + .../src/sdk/client-credentials-oauth.test.ts | 8 +- .../src/sdk/multi-scope-bearer.test.ts | 8 +- .../openapi/src/sdk/oauth-refresh.test.ts | 5 +- .../plugins/openapi/src/sdk/plugin.test.ts | 24 +- packages/plugins/openapi/src/sdk/plugin.ts | 73 +++-- .../openapi/src/sdk/upstream-failures.test.ts | 9 +- packages/plugins/openapi/src/testing/index.ts | 54 ++-- .../workos-vault/src/sdk/secret-store.test.ts | 2 +- 28 files changed, 683 insertions(+), 401 deletions(-) delete mode 100644 packages/core/execution/src/tool-invoker.repro.test.ts rename packages/plugins/google-discovery/src/sdk/{option-json-repro.test.ts => option-json.test.ts} (96%) diff --git a/apps/cloud/src/services/sources-api.node.test.ts b/apps/cloud/src/services/sources-api.node.test.ts index d6150d499..5458fed09 100644 --- a/apps/cloud/src/services/sources-api.node.test.ts +++ b/apps/cloud/src/services/sources-api.node.test.ts @@ -398,7 +398,7 @@ describe("sources api (HTTP)", () => { status: "completed", result: { ok: true, - data: [{ type: "text", text: "cloud-mcp-ok" }], + data: { content: [{ type: "text", text: "cloud-mcp-ok" }] }, }, }); expect((yield* server.requests).length).toBeGreaterThanOrEqual(2); diff --git a/apps/local/src/server/migrate-oauth-connections.test.ts b/apps/local/src/server/migrate-oauth-connections.test.ts index 7c35a9d7c..795467d5e 100644 --- a/apps/local/src/server/migrate-oauth-connections.test.ts +++ b/apps/local/src/server/migrate-oauth-connections.test.ts @@ -159,7 +159,7 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { `); db.prepare("INSERT INTO `openapi_source` (id, scope_id, oauth2) VALUES (?, ?, ?)").run( - "dealcloud_api", + "example_api", "org-1", JSON.stringify({ kind: "oauth2", @@ -174,27 +174,27 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { "INSERT INTO `connection` (id, scope_id, provider, provider_state, scope, updated_at) VALUES (?, ?, ?, ?, ?, ?)", ); insertConnection.run( - "openapi-oauth2-app-dealcloud_api", + "openapi-oauth2-app-example_api", "org-1", "oauth2", JSON.stringify({ kind: "client-credentials", - tokenEndpoint: "https://resolve.dealcloud.com/oauth/token", - clientIdSecretId: "dealcloud-client-id", - clientSecretSecretId: "dealcloud-client-secret", + tokenEndpoint: "https://auth.example.test/oauth/token", + clientIdSecretId: "example-client-id", + clientSecretSecretId: "example-client-secret", }), null, now, ); insertConnection.run( - "openapi-oauth2-app-dealcloud_api", + "openapi-oauth2-app-example_api", "user-org:user-jd:org-1", "openapi:oauth2", JSON.stringify({ kind: "client-credentials", - tokenEndpoint: "https://resolve.dealcloud.com/oauth/token", - clientIdSecretId: "dealcloud-client-id-jd", - clientSecretSecretId: "dealcloud-client-secret-jd", + tokenEndpoint: "https://auth.example.test/oauth/token", + clientIdSecretId: "example-client-id-jd", + clientSecretSecretId: "example-client-secret-jd", }), null, now, @@ -207,12 +207,12 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { "org-client-id", "org-1", "openapi", - "dealcloud_api", + "example_api", "org-1", "oauth2:oauth2:client-id", "secret", null, - "dealcloud-client-id-jd", + "example-client-id-jd", null, now, now, @@ -221,12 +221,12 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { "org-client-secret", "org-1", "openapi", - "dealcloud_api", + "example_api", "org-1", "oauth2:oauth2:client-secret", "secret", null, - "dealcloud-client-secret-jd", + "example-client-secret-jd", null, now, now, @@ -235,13 +235,13 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { "org-connection", "org-1", "openapi", - "dealcloud_api", + "example_api", "org-1", "oauth2:oauth2:connection", "connection", null, null, - "openapi-oauth2-app-dealcloud_api", + "openapi-oauth2-app-example_api", now, now, ); @@ -249,13 +249,13 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { "jd-connection", "user-org:user-jd:org-1", "openapi", - "dealcloud_api", + "example_api", "org-1", "oauth2:oauth2:connection", "connection", null, null, - "openapi-oauth2-app-dealcloud_api", + "openapi-oauth2-app-example_api", now, now, ); @@ -271,20 +271,20 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { .prepare( "SELECT scope_id, slot_key, kind, secret_id, connection_id FROM `credential_binding` WHERE source_id = ? ORDER BY scope_id, slot_key", ) - .all("dealcloud_api"); + .all("example_api"); expect(bindings).toEqual([ { scope_id: "org-1", slot_key: "oauth2:oauth2:client-id", kind: "secret", - secret_id: "dealcloud-client-id", + secret_id: "example-client-id", connection_id: null, }, { scope_id: "org-1", slot_key: "oauth2:oauth2:client-secret", kind: "secret", - secret_id: "dealcloud-client-secret", + secret_id: "example-client-secret", connection_id: null, }, { @@ -292,20 +292,20 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { slot_key: "oauth2:oauth2:connection", kind: "connection", secret_id: null, - connection_id: "openapi-oauth2-app-dealcloud_api", + connection_id: "openapi-oauth2-app-example_api", }, { scope_id: "user-org:user-jd:org-1", slot_key: "oauth2:oauth2:client-id", kind: "secret", - secret_id: "dealcloud-client-id-jd", + secret_id: "example-client-id-jd", connection_id: null, }, { scope_id: "user-org:user-jd:org-1", slot_key: "oauth2:oauth2:client-secret", kind: "secret", - secret_id: "dealcloud-client-secret-jd", + secret_id: "example-client-secret-jd", connection_id: null, }, { @@ -313,7 +313,7 @@ describe("0009_repair_openapi_oauth_cutover_residue", () => { slot_key: "oauth2:oauth2:connection", kind: "connection", secret_id: null, - connection_id: "openapi-oauth2-app-dealcloud_api", + connection_id: "openapi-oauth2-app-example_api", }, ]); }); diff --git a/packages/core/execution/src/description.ts b/packages/core/execution/src/description.ts index fdf05a4fd..97ad689ff 100644 --- a/packages/core/execution/src/description.ts +++ b/packages/core/execution/src/description.ts @@ -48,6 +48,7 @@ const formatDescription = (sources: readonly Source[]): string => { "- `tools.search()` returns paginated, ranked matches: `{ items, total, hasMore, nextOffset }`. Best-first. Use short intent phrases like `github issues`, `repo details`, or `create calendar event`.", '- When you already know the namespace, narrow with `tools.search({ namespace: "github", query: "issues" })`.', "- `tools.executor.sources.list()` returns the same paged shape: `{ items: [{ id, toolCount, ... }], total, hasMore, nextOffset }`.", + "- Tool calls return a value union: `{ ok: true, data }` for success or `{ ok: false, error: { code, message, status?, details?, retryable? } }` for expected tool/domain failures. Branch on `result.ok`.", "- If `hasMore` is true and you didn't find what you need, fetch the next page: `tools.search({ query, offset: nextOffset, limit })`. Same `offset` parameter on `tools.executor.sources.list({ offset, limit })`.", "- Always use the namespace prefix when calling tools: `tools..(args)`. Example: `tools.home_assistant_rest_api.states.getState(...)` — not `tools.states.getState(...)`.", "- The `tools` object is a lazy proxy — `Object.keys(tools)` won't work. Use `tools.search()` or `tools.executor.sources.list()` instead.", diff --git a/packages/core/execution/src/tool-invoker.leak.test.ts b/packages/core/execution/src/tool-invoker.leak.test.ts index 269f5d95a..650728f22 100644 --- a/packages/core/execution/src/tool-invoker.leak.test.ts +++ b/packages/core/execution/src/tool-invoker.leak.test.ts @@ -12,10 +12,10 @@ const EmptyInputSchema = Schema.toStandardSchemaV1( const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); -// Plugin-internal tagged error whose `cause` carries sensitive internal -// context. The dispatcher must route this through the opaque-generic -// path so none of that context reaches the sandbox via Error.message. -class FakeOpenApiInvocationError extends Data.TaggedError("OpenApiInvocationError")<{ +// Plugin-internal tagged error whose `cause` carries internal diagnostics. +// The dispatcher must route this through the opaque-generic path so none of +// that context reaches the sandbox via Error.message. +class FakePluginInvocationError extends Data.TaggedError("PluginInvocationError")<{ readonly message: string; readonly cause: unknown; }> {} @@ -35,18 +35,17 @@ const leakyPlugin = definePlugin(() => ({ inputSchema: EmptyInputSchema, handler: () => Effect.fail( - new FakeOpenApiInvocationError({ - message: "HTTP request failed", + new FakePluginInvocationError({ + message: "Upstream request failed", cause: { - _tag: "HttpClientError", + _tag: "InternalTransportError", request: { method: "GET", - url: "https://internal.dealcloud/v1/entities?accessToken=SECRET_TOKEN_xyz", - headers: { Authorization: "Bearer SECRET_TOKEN_xyz" }, + url: "https://internal.service.local/v1/resources?trace=trace-123", + headers: { "x-internal-routing": "private-cluster" }, }, - stack: - "Error: ECONNREFUSED\n at /home/svc/executor/packages/plugins/openapi/...:142:11", - dbConnString: "postgres://app:p@ssw0rd@10.0.0.5:5432/executor", + stack: "Error: connect failed\n at plugin-transport.ts:42:11", + note: "internal diagnostic detail", }, }), ), @@ -87,11 +86,11 @@ describe("internal-error leak audit (opaque defects)", () => { // Must be the canonical opaque shape: "Internal tool error []" expect(msg).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); // Crucially, no internal context leaks - expect(msg).not.toContain("SECRET_TOKEN_xyz"); - expect(msg).not.toContain("p@ssw0rd"); - expect(msg).not.toContain("packages/plugins"); - expect(msg).not.toContain("HttpClientError"); - expect(msg).not.toContain("HTTP request failed"); + expect(msg).not.toContain("trace-123"); + expect(msg).not.toContain("private-cluster"); + expect(msg).not.toContain("internal.service.local"); + expect(msg).not.toContain("InternalTransportError"); + expect(msg).not.toContain("Upstream request failed"); }), ); diff --git a/packages/core/execution/src/tool-invoker.repro.test.ts b/packages/core/execution/src/tool-invoker.repro.test.ts deleted file mode 100644 index 9f7895640..000000000 --- a/packages/core/execution/src/tool-invoker.repro.test.ts +++ /dev/null @@ -1,163 +0,0 @@ -import { describe, expect, it } from "@effect/vitest"; -import { Effect, Schema } from "effect"; - -import { ElicitationResponse, ToolResult, createExecutor, definePlugin } from "@executor-js/sdk"; -import { makeTestConfig } from "@executor-js/sdk/testing"; -import { makeExecutorToolInvoker } from "./tool-invoker"; - -const EmptyInputSchema = Schema.toStandardSchemaV1( - Schema.toStandardJSONSchemaV1(Schema.Struct({})), -); - -const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); - -// Plugins now emit ToolResult directly. Mirrors the structured upstream -// payloads each real plugin extracts a top-line message from — the -// invoker passes the whole ToolResult through unchanged so the model -// in the sandbox sees `r.ok === false` and `r.error.details` carrying -// the full body. -const upstreamErrorPlugin = definePlugin(() => ({ - id: "upstream-error-test" as const, - storage: () => ({}), - staticSources: () => [ - { - id: "upstream", - kind: "in-memory", - name: "Upstream", - tools: [ - { - // Microsoft Graph / SharePoint shape: { error: { code, message } } - name: "sharepointShape", - description: "", - inputSchema: EmptyInputSchema, - handler: () => - Effect.succeed( - ToolResult.fail({ - code: "upstream_http_error", - status: 400, - message: 'The expression "foo" is not valid. Provide a valid expression.', - details: { - error: { - code: "invalidRequest", - message: 'The expression "foo" is not valid. Provide a valid expression.', - }, - }, - }), - ), - }, - { - // DealCloud-ish shape: errorCode + errorMessage - name: "dealcloudShape", - description: "", - inputSchema: EmptyInputSchema, - handler: () => - Effect.succeed( - ToolResult.fail({ - code: "upstream_http_error", - status: 400, - message: "Entity 'Deals' has no field 'XYZ'", - details: { - errorCode: 400, - errorMessage: "Entity 'Deals' has no field 'XYZ'", - }, - }), - ), - }, - { - // JSON:API / multi-errors shape - name: "errorsArrayShape", - description: "", - inputSchema: EmptyInputSchema, - handler: () => - Effect.succeed( - ToolResult.fail({ - code: "upstream_http_error", - status: 403, - message: "Insufficient scope", - details: { - errors: [{ status: "403", title: "Forbidden", detail: "Insufficient scope" }], - }, - }), - ), - }, - ], - }, - ], -})); - -const isFailedToolResult = ( - value: unknown, -): value is { - readonly ok: false; - readonly error: { readonly code: string; readonly message: string; readonly details?: unknown }; -} => - value !== null && - typeof value === "object" && - "ok" in value && - (value as { ok: unknown }).ok === false; - -describe("regression: structured upstream failures surface through ToolResult", () => { - it.effect("SharePoint/Graph nested error.message reaches the sandbox via ToolResult.fail", () => - Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), - ); - const invoker = makeExecutorToolInvoker(executor, { - invokeOptions: { onElicitation: acceptAll }, - }); - - const result = yield* invoker.invoke({ path: "upstream.sharepointShape", args: {} }); - expect(isFailedToolResult(result)).toBe(true); - if (!isFailedToolResult(result)) return; - expect(result.error.code).toBe("upstream_http_error"); - expect(result.error.message).toBe( - 'The expression "foo" is not valid. Provide a valid expression.', - ); - expect(result.error.details).toEqual({ - error: { - code: "invalidRequest", - message: 'The expression "foo" is not valid. Provide a valid expression.', - }, - }); - }), - ); - - it.effect("DealCloud errorMessage reaches the sandbox via ToolResult.fail", () => - Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), - ); - const invoker = makeExecutorToolInvoker(executor, { - invokeOptions: { onElicitation: acceptAll }, - }); - - const result = yield* invoker.invoke({ path: "upstream.dealcloudShape", args: {} }); - expect(isFailedToolResult(result)).toBe(true); - if (!isFailedToolResult(result)) return; - expect(result.error.message).toBe("Entity 'Deals' has no field 'XYZ'"); - expect(result.error.details).toMatchObject({ - errorCode: 400, - errorMessage: "Entity 'Deals' has no field 'XYZ'", - }); - }), - ); - - it.effect("JSON:API errors[] reaches the sandbox via ToolResult.fail", () => - Effect.gen(function* () { - const executor = yield* createExecutor( - makeTestConfig({ plugins: [upstreamErrorPlugin()] as const }), - ); - const invoker = makeExecutorToolInvoker(executor, { - invokeOptions: { onElicitation: acceptAll }, - }); - - const result = yield* invoker.invoke({ path: "upstream.errorsArrayShape", args: {} }); - expect(isFailedToolResult(result)).toBe(true); - if (!isFailedToolResult(result)) return; - expect(result.error.message).toBe("Insufficient scope"); - expect(result.error.details).toMatchObject({ - errors: [{ detail: "Insufficient scope" }], - }); - }), - ); -}); diff --git a/packages/core/execution/src/tool-invoker.test.ts b/packages/core/execution/src/tool-invoker.test.ts index ffb05224e..dca052997 100644 --- a/packages/core/execution/src/tool-invoker.test.ts +++ b/packages/core/execution/src/tool-invoker.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "@effect/vitest"; import { Effect, Fiber, Schema } from "effect"; +import * as ts from "typescript"; import { ElicitationResponse, @@ -19,6 +20,10 @@ const RepoInputSchema = Schema.toStandardSchemaV1( Schema.toStandardJSONSchemaV1(Schema.Struct({ owner: Schema.String, repo: Schema.String })), ); +const RepoDetailsOutputSchema = Schema.toStandardSchemaV1( + Schema.toStandardJSONSchemaV1(Schema.Struct({ defaultBranch: Schema.String })), +); + const ContactInputSchema = Schema.toStandardSchemaV1( Schema.toStandardJSONSchemaV1(Schema.Struct({ email: Schema.String })), ); @@ -29,6 +34,58 @@ const EmptyInputSchema = Schema.toStandardSchemaV1( const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); +type DescribedToolContract = { + readonly outputTypeScript: string; + readonly typeScriptDefinitions: Record; +}; + +const typeCheckDescribedInvocation = ( + described: DescribedToolContract, + runtimeResult: unknown, + consumerSource: string, +): readonly string[] => { + const fileName = "described-tool-contract.ts"; + const source = [ + ...Object.entries(described.typeScriptDefinitions).map(([name, definition]) => { + return `type ${name} = ${definition};`; + }), + `type ToolOutput = ${described.outputTypeScript};`, + `const invokedResult: ToolOutput = ${JSON.stringify(runtimeResult)};`, + consumerSource, + ].join("\n"); + + const options: ts.CompilerOptions = { + module: ts.ModuleKind.ESNext, + noEmit: true, + skipLibCheck: true, + strict: true, + target: ts.ScriptTarget.ES2022, + }; + const host = ts.createCompilerHost(options); + const originalGetSourceFile = host.getSourceFile.bind(host); + const originalReadFile = host.readFile.bind(host); + const originalFileExists = host.fileExists.bind(host); + + host.getSourceFile = (candidate, languageVersion, onError, shouldCreateNewSourceFile) => { + if (candidate === fileName) { + return ts.createSourceFile(candidate, source, languageVersion, true); + } + return originalGetSourceFile(candidate, languageVersion, onError, shouldCreateNewSourceFile); + }; + host.readFile = (candidate) => (candidate === fileName ? source : originalReadFile(candidate)); + host.fileExists = (candidate) => candidate === fileName || originalFileExists(candidate); + + const program = ts.createProgram([fileName], options, host); + return ts.getPreEmitDiagnostics(program).map((diagnostic) => { + const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, "\n"); + if (!diagnostic.file || diagnostic.start === undefined) { + return message; + } + const position = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start); + return `${diagnostic.file.fileName}:${position.line + 1}:${position.character + 1} ${message}`; + }); +}; + // --------------------------------------------------------------------------- // Test plugins — each one declares a namespace as a static source with N // tools. Handlers return static data; the suite only cares about discovery @@ -54,6 +111,7 @@ const githubPlugin = definePlugin(() => ({ name: "getRepositoryDetails", description: "Get repository details including the default branch", inputSchema: RepoInputSchema, + outputSchema: RepoDetailsOutputSchema, handler: () => Effect.succeed({ defaultBranch: "main" }), }, { @@ -119,6 +177,72 @@ const errorPlugin = definePlugin(() => ({ ], })); +const structuredFailurePlugin = definePlugin(() => ({ + id: "structured-failure-test" as const, + storage: () => ({}), + staticSources: () => [ + { + id: "upstream", + kind: "in-memory", + name: "Upstream", + tools: [ + { + name: "nestedErrorBody", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 400, + message: 'The expression "foo" is not valid. Provide a valid expression.', + details: { + error: { + code: "invalidRequest", + message: 'The expression "foo" is not valid. Provide a valid expression.', + }, + }, + }), + ), + }, + { + name: "flatErrorBody", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 400, + message: "Field 'XYZ' does not exist", + details: { + errorCode: 400, + errorMessage: "Field 'XYZ' does not exist", + }, + }), + ), + }, + { + name: "errorsArrayBody", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.succeed( + ToolResult.fail({ + code: "upstream_http_error", + status: 403, + message: "Insufficient scope", + details: { + errors: [{ status: "403", title: "Forbidden", detail: "Insufficient scope" }], + }, + }), + ), + }, + ], + }, + ], +})); + const makeSearchExecutor = () => createExecutor(makeTestConfig({ plugins: [githubPlugin(), crmPlugin()] as const })); @@ -342,8 +466,102 @@ describe("tool discovery", () => { expect(described.name).toBe("listRepositoryIssues"); expect(described.description).toBe("List issues for a repository"); expect(described.inputTypeScript).toBe("{ owner: string; repo: string; }"); - expect(described.outputTypeScript).toBeUndefined(); - expect(described.typeScriptDefinitions).toBeUndefined(); + expect(described.outputTypeScript).toBe( + "{ ok: true; data: unknown } | { ok: false; error: ToolError }", + ); + expect(described.typeScriptDefinitions).toEqual({ + ToolError: + "{ code: string; message: string; status?: number; details?: unknown; retryable?: boolean }", + }); + }), + ); + + it.effect("describes a return type that accepts the sandbox invocation result", () => + Effect.gen(function* () { + const executor = yield* makeSearchExecutor(); + const engine = createExecutionEngine({ executor, codeExecutor }); + + const execution = yield* engine.execute( + [ + 'const details = await tools.describe.tool({ path: "github.getRepositoryDetails" });', + "const result = await tools.github.getRepositoryDetails({ owner: 'executor', repo: 'executor' });", + "return {", + " outputTypeScript: details.outputTypeScript,", + " typeScriptDefinitions: details.typeScriptDefinitions,", + " result,", + "};", + ].join("\n"), + { onElicitation: acceptAll }, + ); + + expect(execution.error).toBeUndefined(); + const observed = execution.result as DescribedToolContract & { readonly result: unknown }; + const diagnostics = typeCheckDescribedInvocation( + observed, + observed.result, + [ + "function readDefaultBranch(result: ToolOutput): string {", + " if (!result.ok) return result.error.message;", + " return result.data.defaultBranch;", + "}", + "readDefaultBranch(invokedResult);", + ].join("\n"), + ); + expect(diagnostics).toEqual([]); + }), + ); + + it.effect( + "describes an error-as-value return type that accepts sandbox invocation failures", + () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [errorPlugin()] as const }), + ); + const engine = createExecutionEngine({ executor, codeExecutor }); + + const execution = yield* engine.execute( + [ + 'const details = await tools.describe.tool({ path: "records.queryRows" });', + "const result = await tools.records.queryRows({});", + "return {", + " outputTypeScript: details.outputTypeScript,", + " typeScriptDefinitions: details.typeScriptDefinitions,", + " result,", + "};", + ].join("\n"), + { onElicitation: acceptAll }, + ); + + expect(execution.error).toBeUndefined(); + const observed = execution.result as DescribedToolContract & { readonly result: unknown }; + const diagnostics = typeCheckDescribedInvocation( + observed, + observed.result, + [ + "function readToolResult(result: ToolOutput): unknown {", + " if (!result.ok) return result.error.message;", + " return result.data;", + "}", + "readToolResult(invokedResult);", + ].join("\n"), + ); + expect(diagnostics).toEqual([]); + }), + ); + + it.effect("describes the ToolResult wrapper through the direct describe helper", () => + Effect.gen(function* () { + const executor = yield* makeSearchExecutor(); + const described = yield* describeTool(executor, "github.getRepositoryDetails"); + + expect(described.outputTypeScript).toBe( + "{ ok: true; data: { defaultBranch: string; } } | { ok: false; error: ToolError }", + ); + expect(described.typeScriptDefinitions).toEqual({ + ToolError: + "{ code: string; message: string; status?: number; details?: unknown; retryable?: boolean }", + }); }), ); @@ -422,6 +640,82 @@ describe("tool discovery", () => { }); }), ); + + it.effect("preserves nested upstream error bodies through ToolResult.fail", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [structuredFailurePlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const result = yield* invoker.invoke({ path: "upstream.nestedErrorBody", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "upstream_http_error", + status: 400, + message: 'The expression "foo" is not valid. Provide a valid expression.', + details: { + error: { + code: "invalidRequest", + message: 'The expression "foo" is not valid. Provide a valid expression.', + }, + }, + }, + }); + }), + ); + + it.effect("preserves flat upstream error bodies through ToolResult.fail", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [structuredFailurePlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const result = yield* invoker.invoke({ path: "upstream.flatErrorBody", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "upstream_http_error", + status: 400, + message: "Field 'XYZ' does not exist", + details: { + errorCode: 400, + errorMessage: "Field 'XYZ' does not exist", + }, + }, + }); + }), + ); + + it.effect("preserves upstream errors arrays through ToolResult.fail", () => + Effect.gen(function* () { + const executor = yield* createExecutor( + makeTestConfig({ plugins: [structuredFailurePlugin()] as const }), + ); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const result = yield* invoker.invoke({ path: "upstream.errorsArrayBody", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "upstream_http_error", + status: 403, + message: "Insufficient scope", + details: { + errors: [{ status: "403", title: "Forbidden", detail: "Insufficient scope" }], + }, + }, + }); + }), + ); }); // --------------------------------------------------------------------------- diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index 144064c19..5c4b4f53d 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -13,6 +13,18 @@ import type { SandboxToolInvoker } from "@executor-js/codemode-core"; import { ExecutionToolError } from "./errors"; const OPAQUE_DEFECT_MESSAGE = "Internal tool error"; +const TOOL_ERROR_TYPESCRIPT = + "{ code: string; message: string; status?: number; details?: unknown; retryable?: boolean }"; + +const wrapOutputTypeScript = (outputTypeScript?: string): string => + `{ ok: true; data: ${outputTypeScript ?? "unknown"} } | { ok: false; error: ToolError }`; + +const withToolResultDefinitions = ( + definitions?: Record, +): Record => ({ + ...(definitions ?? {}), + ToolError: TOOL_ERROR_TYPESCRIPT, +}); const newCorrelationId = (): string => { // 8-hex-char correlation id; enough entropy to disambiguate within a @@ -497,7 +509,7 @@ export const describeTool = Effect.fn("executor.tools.describe")(function* ( name: schema.name ?? path, description: schema.description, inputTypeScript: schema.inputTypeScript, - outputTypeScript: schema.outputTypeScript, - typeScriptDefinitions: schema.typeScriptDefinitions, + outputTypeScript: wrapOutputTypeScript(schema.outputTypeScript), + typeScriptDefinitions: withToolResultDefinitions(schema.typeScriptDefinitions), }; }); diff --git a/packages/core/sdk/src/tool-result.ts b/packages/core/sdk/src/tool-result.ts index 297479e88..c4ac138b3 100644 --- a/packages/core/sdk/src/tool-result.ts +++ b/packages/core/sdk/src/tool-result.ts @@ -5,13 +5,17 @@ // the Effect failure channel. // --------------------------------------------------------------------------- -export interface ToolError { - readonly code: string; - readonly message: string; - readonly status?: number; - readonly details?: unknown; - readonly retryable?: boolean; -} +import { Schema } from "effect"; + +export const ToolErrorSchema = Schema.Struct({ + code: Schema.String, + message: Schema.String, + status: Schema.optional(Schema.Number), + details: Schema.optional(Schema.Unknown), + retryable: Schema.optional(Schema.Boolean), +}); + +export type ToolError = typeof ToolErrorSchema.Type; export type ToolResult = | { readonly ok: true; readonly data: T } @@ -22,19 +26,12 @@ export const ToolResult = { fail: (error: ToolError): ToolResult => ({ ok: false, error }), } as const; -export const isToolResult = (value: unknown): value is ToolResult => { - if (value === null || typeof value !== "object") return false; - if (!("ok" in value)) return false; - const ok = (value as { ok: unknown }).ok; - if (ok === true) return "data" in value; - if (ok === false) { - if (!("error" in value)) return false; - const error = (value as { error: unknown }).error; - if (error === null || typeof error !== "object") return false; - if (!("code" in error) || !("message" in error)) return false; - const errorObj = error as { readonly code: unknown; readonly message: unknown }; - // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: structural type guard; `message` is a required ToolError field, not a thrown JS error - return typeof errorObj.code === "string" && typeof errorObj.message === "string"; - } - return false; -}; +const ToolResultSchema = Schema.Union([ + Schema.Struct({ ok: Schema.Literal(true), data: Schema.Unknown }), + Schema.Struct({ ok: Schema.Literal(false), error: ToolErrorSchema }), +]); + +const isUnknownToolResult = Schema.is(ToolResultSchema); + +export const isToolResult = (value: unknown): value is ToolResult => + isUnknownToolResult(value); diff --git a/packages/kernel/core/src/strip-types.test.ts b/packages/kernel/core/src/strip-types.test.ts index 086d71391..83291dde2 100644 --- a/packages/kernel/core/src/strip-types.test.ts +++ b/packages/kernel/core/src/strip-types.test.ts @@ -59,11 +59,11 @@ describe("stripTypeScript", () => { // axiom://7bf76f79c5d807272781e9554040aab3 — typed annotation in // a function expression. const code = ` - const fetchDeals = async (sourceId: string): Promise> => { + const fetchResources = async (sourceId: string): Promise> => { const result = await tools.executor.sources.list(); return result.items; }; - return fetchDeals('dealcloud'); + return fetchResources('example-source'); `; const out = stripTypeScript(code); expect(out).not.toContain(": string"); diff --git a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts index 8106277a1..d2ee3aef4 100644 --- a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts +++ b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts @@ -4,6 +4,7 @@ import * as Cause from "effect/Cause"; import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; import type { SandboxToolInvoker } from "@executor-js/codemode-core"; +import { ExecutionToolError } from "@executor-js/execution"; import { ToolDispatcher, makeDynamicWorkerExecutor, @@ -257,6 +258,27 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.error).toBe("Internal tool error"); }); + it("preserves public ExecutionToolError messages across the worker bridge", async () => { + const executor = makeDynamicWorkerExecutor({ loader }); + const invoker = { + invoke: () => + Effect.fail( + new ExecutionToolError({ + message: + "tools.search expects an object: { query?: string; namespace?: string; limit?: number; offset?: number }", + }), + ), + } satisfies SandboxToolInvoker; + + const result = await Effect.runPromise( + executor.execute("async () => await tools.search('github')", invoker), + ); + + expect(result.error).toBe( + "tools.search expects an object: { query?: string; namespace?: string; limit?: number; offset?: number }", + ); + }); + it("does not expose host error stack details to sandbox error handlers", async () => { const executor = makeDynamicWorkerExecutor({ loader }); const invoker = { diff --git a/packages/kernel/runtime-dynamic-worker/src/module-template.ts b/packages/kernel/runtime-dynamic-worker/src/module-template.ts index 3cc8fabaa..754eca1b4 100644 --- a/packages/kernel/runtime-dynamic-worker/src/module-template.ts +++ b/packages/kernel/runtime-dynamic-worker/src/module-template.ts @@ -127,6 +127,14 @@ export const buildExecutorModule = (body: string, timeoutMs: number): string => " for (const [k, v] of Object.entries(value)) out[k] = __decodeBinary(v, seen);", " return out;", " };", + " const __publicToolErrorMessage = (error) => {", + " const values = [error && error.primary, ...(Array.isArray(error && error.failures) ? error.failures : [])];", + " for (const value of values) {", + " if (value && value.__type === 'Error' && value.name === 'ExecutionToolError' && typeof value.message === 'string') return value.message;", + " }", + " if (error && typeof error.message === 'string' && error.message.startsWith('Internal tool error')) return error.message;", + " return null;", + " };", " const __makeToolsProxy = (path = []) => new Proxy(() => undefined, {", " get(_target, prop) {", " if (prop === 'then' || typeof prop === 'symbol') return undefined;", @@ -138,7 +146,7 @@ export const buildExecutorModule = (body: string, timeoutMs: number): string => " return (async () => {", " const encoded = await __encodeBinary(args[0]);", " const data = await __dispatcher.call(toolPath, encoded);", - " if (!data.ok) throw new Error(data.error && typeof data.error.message === 'string' && data.error.message.startsWith('Internal tool error') ? data.error.message : 'Internal tool error');", + " if (!data.ok) throw new Error(__publicToolErrorMessage(data.error) || 'Internal tool error');", " return __decodeBinary(data.result);", " })();", " },", diff --git a/packages/plugins/google-discovery/src/sdk/option-json-repro.test.ts b/packages/plugins/google-discovery/src/sdk/option-json.test.ts similarity index 96% rename from packages/plugins/google-discovery/src/sdk/option-json-repro.test.ts rename to packages/plugins/google-discovery/src/sdk/option-json.test.ts index 668322549..4a87e10df 100644 --- a/packages/plugins/google-discovery/src/sdk/option-json-repro.test.ts +++ b/packages/plugins/google-discovery/src/sdk/option-json.test.ts @@ -1,6 +1,6 @@ -// Reproduces the PR 706 bug class using Effect-native primitives only — +// Covers the Option JSON boundary using Effect-native primitives only: // no JSON.parse, no JSON.stringify, no node:fs on our side. We split the -// JSON boundary into two Effect schema steps: +// boundary into two Effect schema steps: // // 1. Schema.encodeEffect(Inner)(value) → encoded JS shape // 2. Schema.encodeEffect(UnknownFromJsonString) → JSON string @@ -13,7 +13,7 @@ // step 2's JSON-stringify (driven by Effect, not us) flattens the Option // to {_id,_tag,value}, and step 5 rejects the shape. // -// Run: vitest run packages/plugins/google-discovery/src/sdk/option-json-repro.test.ts +// Run: vitest run packages/plugins/google-discovery/src/sdk/option-json.test.ts import { describe, expect, it } from "@effect/vitest"; import * as NodeFileSystem from "@effect/platform-node/NodeFileSystem"; @@ -28,7 +28,7 @@ const fixed = { description: Option.some("hello") }; const withTmpFile = (fn: (path: string) => Effect.Effect) => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; - const dir = yield* fs.makeTempDirectoryScoped({ prefix: "option-repro-" }); + const dir = yield* fs.makeTempDirectoryScoped({ prefix: "option-json-" }); return yield* fn(`${dir}/binding.json`); }).pipe(Effect.scoped, Effect.provide(NodeFileSystem.layer)); diff --git a/packages/plugins/google-discovery/src/sdk/plugin.ts b/packages/plugins/google-discovery/src/sdk/plugin.ts index 38f000cf9..2f6c0f6e5 100644 --- a/packages/plugins/google-discovery/src/sdk/plugin.ts +++ b/packages/plugins/google-discovery/src/sdk/plugin.ts @@ -35,6 +35,23 @@ import { GoogleDiscoveryStoredSourceData as GoogleDiscoveryStoredSourceDataSchem // --------------------------------------------------------------------------- const GOOGLE_BODY_CAP = 1024; +const UpstreamMessageBody = Schema.Struct({ message: Schema.String }); +const UpstreamErrorMessageBody = Schema.Struct({ errorMessage: Schema.String }); +const UpstreamNestedErrorBody = Schema.Struct({ error: UpstreamMessageBody }); +const UpstreamErrorsArrayBody = Schema.Struct({ + errors: Schema.Array( + Schema.Struct({ + detail: Schema.optional(Schema.String), + message: Schema.optional(Schema.String), + title: Schema.optional(Schema.String), + }), + ), +}); + +const decodeUpstreamMessageBody = Schema.decodeUnknownOption(UpstreamMessageBody); +const decodeUpstreamErrorMessageBody = Schema.decodeUnknownOption(UpstreamErrorMessageBody); +const decodeUpstreamNestedErrorBody = Schema.decodeUnknownOption(UpstreamNestedErrorBody); +const decodeUpstreamErrorsArrayBody = Schema.decodeUnknownOption(UpstreamErrorsArrayBody); const googleClampedStringify = (value: unknown): string => { let s: string; @@ -47,30 +64,30 @@ const googleClampedStringify = (value: unknown): string => { return s.length > GOOGLE_BODY_CAP ? `${s.slice(0, GOOGLE_BODY_CAP)}…` : s; }; +const firstNonEmpty = (...values: readonly (string | undefined)[]): string | undefined => + values.find((value) => value !== undefined && value.length > 0); + const googleExtractUpstreamMessage = (body: unknown, status: number): string => { if (typeof body === "string") { return body.length > 0 ? body : `Upstream returned HTTP ${status}`; } + const nested = Option.getOrUndefined(decodeUpstreamNestedErrorBody(body)); + const messageBody = Option.getOrUndefined(decodeUpstreamMessageBody(body)); + const errorMessageBody = Option.getOrUndefined(decodeUpstreamErrorMessageBody(body)); + const errorsBody = Option.getOrUndefined(decodeUpstreamErrorsArrayBody(body)); + const arrayMessage = errorsBody?.errors + .map(({ detail, message: upstreamMessage, title }) => + firstNonEmpty(detail, upstreamMessage, title), + ) + .find((message) => message !== undefined); + const message = firstNonEmpty( + nested?.error.message, + messageBody?.message, + errorMessageBody?.errorMessage, + arrayMessage, + ); + if (message !== undefined) return message; if (body !== null && typeof body === "object") { - const obj = body as Record; - const nested = obj.error; - if (nested !== null && typeof nested === "object" && "message" in nested) { - const m = (nested as { message: unknown }).message; - if (typeof m === "string" && m.length > 0) return m; - } - if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; - if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) - return obj.errorMessage; - if (Array.isArray(obj.errors) && obj.errors.length > 0) { - const first = obj.errors[0]; - if (first !== null && typeof first === "object") { - const f = first as Record; - for (const key of ["detail", "message", "title"]) { - const v = f[key]; - if (typeof v === "string" && v.length > 0) return v; - } - } - } return googleClampedStringify(body); } return `Upstream returned HTTP ${status}`; diff --git a/packages/plugins/graphql/src/sdk/plugin.test.ts b/packages/plugins/graphql/src/sdk/plugin.test.ts index c265f45b1..c97cd9c8e 100644 --- a/packages/plugins/graphql/src/sdk/plugin.test.ts +++ b/packages/plugins/graphql/src/sdk/plugin.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "@effect/vitest"; import { Effect, Predicate } from "effect"; +import { HttpServerRequest, HttpServerResponse } from "effect/unstable/http"; import { ConnectionId, @@ -13,7 +14,7 @@ import { SecretId, TokenMaterial, } from "@executor-js/sdk"; -import { makeTestConfig } from "@executor-js/sdk/testing"; +import { makeTestConfig, serveTestHttpApp } from "@executor-js/sdk/testing"; import { memorySecretsPlugin } from "@executor-js/sdk/testing"; import { graphqlPlugin } from "./plugin"; @@ -208,6 +209,46 @@ describe("graphqlPlugin real protocol server", () => { }), ); + it.effect("surfaces non-2xx invocation responses as ToolResult.fail", () => + Effect.gen(function* () { + const server = yield* serveTestHttpApp((request) => + Effect.gen(function* () { + const webRequest = yield* HttpServerRequest.toWeb(request); + const body = yield* Effect.promise(() => webRequest.text()); + if (body.includes("__schema")) { + return HttpServerResponse.jsonUnsafe({ data: introspectionResult }); + } + return HttpServerResponse.text("temporary upstream outage", { + status: 503, + contentType: "text/plain", + }); + }), + ); + const executor = yield* createExecutor( + makeTestConfig({ plugins: [graphqlPlugin()] as const }), + ); + + yield* executor.graphql.addSource({ + endpoint: server.url("/graphql"), + scope: TEST_SCOPE, + namespace: "http_error_graph", + }); + + const result = yield* executor.tools.invoke("http_error_graph.query.hello", { + name: "Ada", + }); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "graphql_http_error", + status: 503, + message: "GraphQL request failed with HTTP 503", + }, + }); + }), + ); + it.effect("invokes OAuth-backed sources with a bearer token", () => Effect.gen(function* () { const server = yield* serveGreetingServer; diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index 35814d6bd..76fb6cf35 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -66,6 +66,19 @@ import { // Plugin config // --------------------------------------------------------------------------- +const GraphqlErrorBody = Schema.Struct({ message: Schema.String }); +const GraphqlErrorsBody = Schema.Array(Schema.Unknown); +const decodeGraphqlErrorBody = Schema.decodeUnknownOption(GraphqlErrorBody); +const decodeGraphqlErrorsBody = Schema.decodeUnknownOption(GraphqlErrorsBody); + +const decodeGraphqlErrors = (errors: unknown): readonly unknown[] | undefined => + Option.getOrUndefined(decodeGraphqlErrorsBody(errors)); + +const extractGraphqlErrorMessage = (errors: readonly unknown[]): string | undefined => + errors + .map((error) => Option.getOrUndefined(decodeGraphqlErrorBody(error))?.message) + .find((message) => message !== undefined && message.length > 0); + export type HeaderValue = HeaderValueValue; export type GraphqlCredentialValue = ConfiguredGraphqlCredentialValue; @@ -1058,22 +1071,27 @@ export const graphqlPlugin = definePlugin((options?: GraphqlPluginOptions) => { httpClientLayer, ); - const errors = result.errors; - if (Array.isArray(errors) && errors.length > 0) { - const first = errors[0]; - const firstMessage = - first !== null && typeof first === "object" && "message" in first - ? (first as { message: unknown }).message - : undefined; + const errors = decodeGraphqlErrors(result.errors); + if (errors !== undefined && errors.length > 0) { + const firstMessage = extractGraphqlErrorMessage(errors); return ToolResult.fail({ code: "graphql_errors", - message: - typeof firstMessage === "string" && firstMessage.length > 0 - ? firstMessage - : "GraphQL request returned errors", + message: firstMessage !== undefined ? firstMessage : "GraphQL request returned errors", details: { errors }, }); } + if (result.status < 200 || result.status >= 300) { + return ToolResult.fail({ + code: "graphql_http_error", + status: result.status, + message: `GraphQL request failed with HTTP ${result.status}`, + details: { + status: result.status, + data: result.data, + errors: result.errors, + }, + }); + } return ToolResult.ok(result.data); }), diff --git a/packages/plugins/mcp/src/react/AddMcpSource.tsx b/packages/plugins/mcp/src/react/AddMcpSource.tsx index 2bb7f8ea3..6efbb94b0 100644 --- a/packages/plugins/mcp/src/react/AddMcpSource.tsx +++ b/packages/plugins/mcp/src/react/AddMcpSource.tsx @@ -55,6 +55,13 @@ import { MCP_OAUTH_CONNECTION_SLOT, type McpCredentialInput } from "../sdk/types const ErrorMessage = Schema.Struct({ message: Schema.String }); const decodeErrorMessage = Schema.decodeUnknownOption(ErrorMessage); +const STDIO_ENV_ESCAPE_REPLACEMENTS: Readonly> = { + "\\": "\\", + n: "\n", + r: "\r", + t: "\t", + '"': '"', +}; const errorMessageFromExit = (exit: Exit.Exit, fallback: string): string => Option.match(Option.flatMap(Exit.findErrorOption(exit), decodeErrorMessage), { @@ -525,22 +532,10 @@ export default function AddMcpSource(props: { const inner = value.slice(1, -1); if (quote === "'") return inner; - return inner.replace(/\\([\\nrt"])/g, (_, escaped: string) => { - switch (escaped) { - case "\\": - return "\\"; - case "n": - return "\n"; - case "r": - return "\r"; - case "t": - return "\t"; - case '"': - return '"'; - default: - return escaped; - } - }); + return inner.replace( + /\\([\\nrt"])/g, + (_, escaped: string) => STDIO_ENV_ESCAPE_REPLACEMENTS[escaped] ?? escaped, + ); }; const parseStdioEnv = (raw: string): Record | undefined => { diff --git a/packages/plugins/mcp/src/sdk/elicitation.test.ts b/packages/plugins/mcp/src/sdk/elicitation.test.ts index 908d99107..34b008f4b 100644 --- a/packages/plugins/mcp/src/sdk/elicitation.test.ts +++ b/packages/plugins/mcp/src/sdk/elicitation.test.ts @@ -70,7 +70,7 @@ describe("MCP elicitation (end-to-end)", () => { expect(result).toMatchObject({ ok: true, - data: [{ type: "text", text: "approved:hello" }], + data: { content: [{ type: "text", text: "approved:hello" }] }, }); // At least one elicitation should be the MCP server's form expect(elicitationMessages.length).toBeGreaterThanOrEqual(1); @@ -97,7 +97,7 @@ describe("MCP elicitation (end-to-end)", () => { expect(result).toMatchObject({ ok: true, - data: [{ type: "text", text: "denied:nope" }], + data: { content: [{ type: "text", text: "denied:nope" }] }, }); }), ); @@ -117,7 +117,31 @@ describe("MCP elicitation (end-to-end)", () => { expect(result).toMatchObject({ ok: true, - data: [{ type: "text", text: "plain" }], + data: { content: [{ type: "text", text: "plain" }] }, + }); + }), + ); + + it.effect("successful tool invocation preserves structured MCP result fields", () => + Effect.gen(function* () { + const server = yield* serveElicitationTestServer; + const executor = yield* makeTestExecutor(server.url); + const tools = yield* executor.tools.list(); + const structuredEcho = tools.find((t) => t.name === "structured_echo")!; + + const result = yield* executor.tools.invoke( + structuredEcho.id, + { value: "plain" }, + { onElicitation: "accept-all" }, + ); + + expect(result).toMatchObject({ + ok: true, + data: { + content: [{ type: "text", text: "plain" }], + structuredContent: { value: "plain", upper: "PLAIN" }, + _meta: { trace: "kept" }, + }, }); }), ); diff --git a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts index cd4fd09f0..fc75f6154 100644 --- a/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts +++ b/packages/plugins/mcp/src/sdk/per-user-auth-isolation.test.ts @@ -116,7 +116,7 @@ describe("per-user MCP auth isolation", () => { ); expect(userAResult).toMatchObject({ ok: true, - data: [{ type: "text", text: "ok:from-user-a" }], + data: { content: [{ type: "text", text: "ok:from-user-a" }] }, }); expect( (yield* server.requests) @@ -189,7 +189,7 @@ describe("per-user MCP auth isolation", () => { ); expect(userAResult).toMatchObject({ ok: true, - data: [{ type: "text", text: "ok:user-a-header" }], + data: { content: [{ type: "text", text: "ok:user-a-header" }] }, }); expect( (yield* server.requests) @@ -270,7 +270,7 @@ describe("per-user MCP auth isolation", () => { expect(result).toMatchObject({ ok: true, - data: [{ type: "text", text: "ok:org-header" }], + data: { content: [{ type: "text", text: "ok:org-header" }] }, }); const invokeRequests = (yield* server.requests).slice(beforeInvoke); expect( diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index df0d59301..ee13f962e 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -8,6 +8,7 @@ import { Predicate, Result, Scope, + Schema, ScopedCache, } from "effect"; import type { HttpClient } from "effect/unstable/http"; @@ -194,21 +195,20 @@ const toBinding = (entry: McpToolManifestEntry): McpToolBinding => }); const MCP_PLUGIN_ID = "mcp"; +const McpTextContent = Schema.Struct({ type: Schema.Literal("text"), text: Schema.String }); +const McpToolCallEnvelope = Schema.Struct({ + isError: Schema.optional(Schema.Boolean), + content: Schema.optional(Schema.Array(Schema.Unknown)), +}); + +const decodeMcpTextContent = Schema.decodeUnknownOption(McpTextContent); +const decodeMcpToolCallEnvelope = Schema.decodeUnknownOption(McpToolCallEnvelope); const extractMcpErrorMessage = (content: unknown): string => { if (Array.isArray(content)) { for (const item of content) { - if ( - item !== null && - typeof item === "object" && - "type" in item && - (item as { type: unknown }).type === "text" && - "text" in item && - typeof (item as { text: unknown }).text === "string" && - (item as { text: string }).text.length > 0 - ) { - return (item as { text: string }).text; - } + const decoded = Option.getOrUndefined(decodeMcpTextContent(item)); + if (decoded !== undefined && decoded.text.length > 0) return decoded.text; } } return "MCP tool returned an error"; @@ -1785,18 +1785,15 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { elicit, }); - const rawObj = - raw !== null && typeof raw === "object" ? (raw as Record) : undefined; - const isError = rawObj?.isError === true; - const content = rawObj?.content; - if (isError) { + const envelope = Option.getOrUndefined(decodeMcpToolCallEnvelope(raw)); + if (envelope?.isError === true) { return ToolResult.fail({ code: "mcp_tool_error", - message: extractMcpErrorMessage(content), - details: { content }, + message: extractMcpErrorMessage(envelope.content), + details: { content: envelope.content }, }); } - return ToolResult.ok(content ?? raw); + return ToolResult.ok(raw); }).pipe( Effect.withSpan("mcp.plugin.invoke_tool", { attributes: { diff --git a/packages/plugins/mcp/src/testing/server.ts b/packages/plugins/mcp/src/testing/server.ts index f4f46b35e..6222a9235 100644 --- a/packages/plugins/mcp/src/testing/server.ts +++ b/packages/plugins/mcp/src/testing/server.ts @@ -375,6 +375,23 @@ export const makeElicitationMcpServer = () => { }), ); + server.registerTool( + "structured_echo", + { + description: "Returns text plus structured data", + inputSchema: { value: z.string() }, + outputSchema: { + value: z.string(), + upper: z.string(), + }, + }, + async ({ value }: { value: string }) => ({ + content: [{ type: "text" as const, text: value }], + structuredContent: { value, upper: value.toUpperCase() }, + _meta: { trace: "kept" }, + }), + ); + return server; }; diff --git a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts index 7a4bc7982..e58d90fe0 100644 --- a/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts +++ b/packages/plugins/openapi/src/sdk/client-credentials-oauth.test.ts @@ -238,10 +238,10 @@ describe("OpenAPI client_credentials OAuth", () => { // The connection lives at the innermost (user) scope, which // preserves per-user credential resolution: if each user has - // their own `dealcloud_client_id`/`dealcloud_client_secret` - // shadowed at their user scope, each user mints their own - // token. A single shared connection slot still lets every caller - // reach the right physical row through scoped credential bindings. + // their own OAuth client credentials shadowed at their user + // scope, each user mints their own token. A single shared + // connection slot still lets every caller reach the right + // physical row through scoped credential bindings. const userConnections = yield* userExec.connections.list(); const connection = userConnections.find((c) => c.id === completedConnection.connectionId); expect(connection).toBeDefined(); diff --git a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts index 55e8c4909..35d648c7b 100644 --- a/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts +++ b/packages/plugins/openapi/src/sdk/multi-scope-bearer.test.ts @@ -459,9 +459,7 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { yield* bobExec.tools.invoke("vercel.projects.list", {}, autoApprove), ); expect(bobResult.error).toBeNull(); - expect((bobResult.data as EchoHeaders | null)?.authorization).toBe( - "Bearer bob-vercel-token", - ); + expect((bobResult.data as EchoHeaders | null)?.authorization).toBe("Bearer bob-vercel-token"); }), ); @@ -602,9 +600,7 @@ describe("OpenAPI multi-scope bearer (Vercel-style)", () => { yield* aliceExec.tools.invoke("vercel.projects.list", {}, autoApprove), ); expect(fallbackResult.error).toBeNull(); - expect((fallbackResult.data as EchoHeaders | null)?.authorization).toBe( - "Bearer org-token", - ); + expect((fallbackResult.data as EchoHeaders | null)?.authorization).toBe("Bearer org-token"); yield* aliceExec.openapi.setSourceBinding( OpenApiSourceBindingInput.make({ diff --git a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts index 930e61b20..0ba99b4f7 100644 --- a/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts +++ b/packages/plugins/openapi/src/sdk/oauth-refresh.test.ts @@ -315,10 +315,7 @@ describe("OpenAPI oauth refresh", () => { for (const r of invokes) { const res = unwrapInvocation(r); expect(res.error).toBeNull(); - const bearer = (res.data as EchoHeaders | null)?.authorization?.replace( - /^Bearer\s+/i, - "", - ); + const bearer = (res.data as EchoHeaders | null)?.authorization?.replace(/^Bearer\s+/i, ""); expect(bearer).toBeDefined(); expect(yield* oauth.acceptsAccessToken(bearer!)).toBe(true); } diff --git a/packages/plugins/openapi/src/sdk/plugin.test.ts b/packages/plugins/openapi/src/sdk/plugin.test.ts index a8619aa02..4aee8f035 100644 --- a/packages/plugins/openapi/src/sdk/plugin.test.ts +++ b/packages/plugins/openapi/src/sdk/plugin.test.ts @@ -268,11 +268,13 @@ describe("OpenAPI Plugin", () => { }), ); - const preview = unwrapInvocation(yield* executor.tools.invoke( - "executor.openapi.previewSpec", - { spec: testApiSpec() }, - autoApprove, - )).data as { operationCount: number }; + const preview = unwrapInvocation( + yield* executor.tools.invoke( + "executor.openapi.previewSpec", + { spec: testApiSpec() }, + autoApprove, + ), + ).data as { operationCount: number }; expect(preview.operationCount).toBeGreaterThanOrEqual(2); }), @@ -317,11 +319,13 @@ describe("OpenAPI Plugin", () => { }), ); - const result = unwrapInvocation(yield* executor.tools.invoke( - "executor.openapi.addSource", - testApiSourceConfig({ scope: String(orgScope), namespace: "runtime" }), - autoApprove, - )).data as { sourceId: string; toolCount: number }; + const result = unwrapInvocation( + yield* executor.tools.invoke( + "executor.openapi.addSource", + testApiSourceConfig({ scope: String(orgScope), namespace: "runtime" }), + autoApprove, + ), + ).data as { sourceId: string; toolCount: number }; expect(result).toEqual({ sourceId: "runtime", toolCount: 4 }); expect(yield* executor.openapi.getSource("runtime", String(userScope))).toBeNull(); diff --git a/packages/plugins/openapi/src/sdk/plugin.ts b/packages/plugins/openapi/src/sdk/plugin.ts index 3e9428354..ee4260753 100644 --- a/packages/plugins/openapi/src/sdk/plugin.ts +++ b/packages/plugins/openapi/src/sdk/plugin.ts @@ -59,6 +59,29 @@ import { // --------------------------------------------------------------------------- const STRINGIFIED_BODY_CAP = 1024; +const UpstreamMessageBody = Schema.Struct({ message: Schema.String }); +const UpstreamErrorMessageBody = Schema.Struct({ errorMessage: Schema.String }); +const UpstreamNestedErrorBody = Schema.Struct({ error: UpstreamMessageBody }); +const UpstreamErrorsArrayBody = Schema.Struct({ + errors: Schema.Array( + Schema.Struct({ + detail: Schema.optional(Schema.String), + message: Schema.optional(Schema.String), + title: Schema.optional(Schema.String), + }), + ), +}); +const UpstreamDescriptionBody = Schema.Struct({ + detail: Schema.optional(Schema.String), + title: Schema.optional(Schema.String), + description: Schema.optional(Schema.String), +}); + +const decodeUpstreamMessageBody = Schema.decodeUnknownOption(UpstreamMessageBody); +const decodeUpstreamErrorMessageBody = Schema.decodeUnknownOption(UpstreamErrorMessageBody); +const decodeUpstreamNestedErrorBody = Schema.decodeUnknownOption(UpstreamNestedErrorBody); +const decodeUpstreamErrorsArrayBody = Schema.decodeUnknownOption(UpstreamErrorsArrayBody); +const decodeUpstreamDescriptionBody = Schema.decodeUnknownOption(UpstreamDescriptionBody); const clampedStringify = (value: unknown): string => { let s: string; @@ -71,36 +94,36 @@ const clampedStringify = (value: unknown): string => { return s.length > STRINGIFIED_BODY_CAP ? `${s.slice(0, STRINGIFIED_BODY_CAP)}…` : s; }; -// Walk known upstream error-body shapes. Mirrors the tool-invoker's -// legacy expansion logic. +const firstNonEmpty = (...values: readonly (string | undefined)[]): string | undefined => + values.find((value) => value !== undefined && value.length > 0); + +// Walk known upstream error-body shapes so ToolError.message stays concise +// while ToolError.details preserves the original body. const extractUpstreamMessage = (body: unknown, status: number): string => { if (typeof body === "string") { return body.length > 0 ? body : `Upstream returned HTTP ${status}`; } + const nested = Option.getOrUndefined(decodeUpstreamNestedErrorBody(body)); + const messageBody = Option.getOrUndefined(decodeUpstreamMessageBody(body)); + const errorMessageBody = Option.getOrUndefined(decodeUpstreamErrorMessageBody(body)); + const errorsBody = Option.getOrUndefined(decodeUpstreamErrorsArrayBody(body)); + const descriptionBody = Option.getOrUndefined(decodeUpstreamDescriptionBody(body)); + const arrayMessage = errorsBody?.errors + .map(({ detail, message: upstreamMessage, title }) => + firstNonEmpty(detail, upstreamMessage, title), + ) + .find((message) => message !== undefined); + const message = firstNonEmpty( + nested?.error.message, + messageBody?.message, + errorMessageBody?.errorMessage, + arrayMessage, + descriptionBody?.detail, + descriptionBody?.title, + descriptionBody?.description, + ); + if (message !== undefined) return message; if (body !== null && typeof body === "object") { - const obj = body as Record; - const nested = obj.error; - if (nested !== null && typeof nested === "object" && "message" in nested) { - const m = (nested as { message: unknown }).message; - if (typeof m === "string" && m.length > 0) return m; - } - if (typeof obj.message === "string" && obj.message.length > 0) return obj.message; - if (typeof obj.errorMessage === "string" && obj.errorMessage.length > 0) - return obj.errorMessage; - if (Array.isArray(obj.errors) && obj.errors.length > 0) { - const first = obj.errors[0]; - if (first !== null && typeof first === "object") { - const f = first as Record; - for (const key of ["detail", "message", "title"]) { - const v = f[key]; - if (typeof v === "string" && v.length > 0) return v; - } - } - } - for (const key of ["detail", "title", "description"]) { - const v = obj[key]; - if (typeof v === "string" && v.length > 0) return v; - } return clampedStringify(body); } return `Upstream returned HTTP ${status}`; diff --git a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts index 4d9740c2b..3a8e16cce 100644 --- a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts +++ b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts @@ -37,6 +37,7 @@ import { makeOpenApiTestSourceConfig, type OpenApiTestServerShape, serveOpenApiHttpApiTestServer, + unwrapInvocation, } from "../testing"; import { openApiPlugin } from "./plugin"; @@ -303,10 +304,10 @@ describe("OpenAPI upstream failure modes", () => { const { baseUrl } = yield* slowServer; const executor = yield* buildExecutor(baseUrl); - const result = yield* executor.tools.invoke("f.things.listThings", {}, autoApprove); - // Empty array via .data envelope or directly — accept either shape. - const data = (result as { data?: unknown }).data ?? result; - expect(data).toEqual([]); + const result = unwrapInvocation( + yield* executor.tools.invoke("f.things.listThings", {}, autoApprove), + ); + expect(result.data).toEqual([]); }), ); }); diff --git a/packages/plugins/openapi/src/testing/index.ts b/packages/plugins/openapi/src/testing/index.ts index 21eb77d49..fb3a45037 100644 --- a/packages/plugins/openapi/src/testing/index.ts +++ b/packages/plugins/openapi/src/testing/index.ts @@ -1,4 +1,4 @@ -import { Context, Data, Effect, Layer, Predicate, Ref, Schema, Scope } from "effect"; +import { Context, Data, Effect, Layer, Option, Predicate, Ref, Schema, Scope } from "effect"; import { HttpClient, HttpRouter, @@ -13,7 +13,7 @@ import { OpenApi, } from "effect/unstable/httpapi"; import { OAuthTestServer, serveTestHttpServerLayer } from "@executor-js/sdk/testing"; -import type { ScopeId } from "@executor-js/sdk/core"; +import { isToolResult, type ScopeId } from "@executor-js/sdk/core"; import type { OpenApiPluginExtension, OpenApiSpecConfig } from "../sdk/plugin"; export class OpenApiTestServerAddressError extends Data.TaggedError( @@ -619,15 +619,15 @@ export const TestLayers = { echoWithOAuth: OpenApiEchoTestServer.layerWithOAuth, }; -// --------------------------------------------------------------------------- -// Result unwrapping helper for tests written against the legacy -// `{ status, headers, data, error }` envelope. Translates a ToolResult -// emitted by the OpenAPI plugin's `invokeTool` back into that envelope -// so assertions like `result.data?.X` / `expect(result.error).toBeNull()` -// keep working. -// --------------------------------------------------------------------------- +const OpenApiTransportEnvelope = Schema.Struct({ + status: Schema.Number, + headers: Schema.Record(Schema.String, Schema.String), + data: Schema.Unknown, +}); + +const decodeOpenApiTransportEnvelope = Schema.decodeUnknownOption(OpenApiTransportEnvelope); -export interface LegacyInvocationEnvelope | unknown[] | null> { +export interface OpenApiInvocationResult | unknown[] | null> { readonly status: number | null; readonly headers: Record | null; readonly data: TData; @@ -636,8 +636,8 @@ export interface LegacyInvocationEnvelope | unkn export const unwrapInvocation = | null>( raw: unknown, -): LegacyInvocationEnvelope => { - if (raw === null || typeof raw !== "object" || !("ok" in raw)) { +): OpenApiInvocationResult => { + if (!isToolResult(raw)) { return { status: null, headers: null, @@ -645,26 +645,10 @@ export const unwrapInvocation = | null>( error: null, }; } - const r = raw as - | { readonly ok: true; readonly data: unknown } - | { - readonly ok: false; - readonly error: { readonly status?: number; readonly details?: unknown }; - }; - if (r.ok) { - const inner = r.data; - if ( - inner !== null && - typeof inner === "object" && - "status" in inner && - "headers" in inner && - "data" in inner - ) { - const wrapped = inner as { - readonly status: number; - readonly headers: Record; - readonly data: unknown; - }; + if (raw.ok) { + const inner = raw.data; + const wrapped = Option.getOrUndefined(decodeOpenApiTransportEnvelope(inner)); + if (wrapped !== undefined) { return { status: wrapped.status, headers: wrapped.headers, @@ -672,8 +656,6 @@ export const unwrapInvocation = | null>( error: null, }; } - // Plain `Tool.ok(value)` (no status/headers wrapper). Expose the - // value through `.data`. return { status: null, headers: null, @@ -682,9 +664,9 @@ export const unwrapInvocation = | null>( }; } return { - status: r.error.status ?? null, + status: raw.error.status ?? null, headers: null, data: null as TData, - error: r.error.details ?? r.error, + error: raw.error.details ?? raw.error, }; }; diff --git a/packages/plugins/workos-vault/src/sdk/secret-store.test.ts b/packages/plugins/workos-vault/src/sdk/secret-store.test.ts index c612398f1..2d6cb3637 100644 --- a/packages/plugins/workos-vault/src/sdk/secret-store.test.ts +++ b/packages/plugins/workos-vault/src/sdk/secret-store.test.ts @@ -256,7 +256,7 @@ describe("WorkOS Vault secret provider", () => { const client = makeFakeClient({ rejectReadNamesLongerThan: 80 }); const executor = yield* makeExecutor(client); const longSecretId = SecretId.make( - "openapi-oauth-dealcloud-api-oauth2-user-org-user-01kp6xm1zpvqvtpj77f0yv4eax.access_token", + "openapi-oauth-example-api-oauth2-user-org-user-01kp6xm1zpvqvtpj77f0yv4eax.access_token", ); yield* executor.secrets.set(