diff --git a/packages/core/execution/src/tool-invoker.leak.test.ts b/packages/core/execution/src/tool-invoker.leak.test.ts index 650728f22..a31c8c123 100644 --- a/packages/core/execution/src/tool-invoker.leak.test.ts +++ b/packages/core/execution/src/tool-invoker.leak.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Data, Effect, Schema } from "effect"; +import { Data, Effect, Option, Schema } from "effect"; import { ElicitationResponse, createExecutor, definePlugin } from "@executor-js/sdk"; import { makeTestConfig } from "@executor-js/sdk/testing"; @@ -20,6 +20,17 @@ class FakePluginInvocationError extends Data.TaggedError("PluginInvocationError" readonly cause: unknown; }> {} +// Mirrors the openapi plugin's OpenApiInvocationError shape (the dispatcher +// matches it structurally by tag, not by class identity, since this package +// does not depend on the plugin). Pre-flight raises carry statusCode: None +// and no cause and must surface verbatim; transport/post-response failures +// must stay opaque. +class FakeOpenApiInvocationError extends Data.TaggedError("OpenApiInvocationError")<{ + readonly message: string; + readonly statusCode: Option.Option; + readonly cause?: unknown; +}> {} + const leakyPlugin = definePlugin(() => ({ id: "leaky-test" as const, storage: () => ({}), @@ -66,6 +77,59 @@ const leakyPlugin = definePlugin(() => ({ ), ), }, + { + name: "failsPreflightPathParam", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail( + new FakeOpenApiInvocationError({ + message: "Missing required path parameter: datasourceId", + statusCode: Option.none(), + }), + ), + }, + { + name: "failsPreflightBody", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail( + new FakeOpenApiInvocationError({ + message: "Missing required request body", + statusCode: Option.none(), + }), + ), + }, + { + name: "failsOpenApiTransport", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail( + new FakeOpenApiInvocationError({ + message: "HTTP request failed", + statusCode: Option.none(), + cause: { + _tag: "RequestError", + request: { url: "https://internal.service.local/v1?token=tok-456" }, + }, + }), + ), + }, + { + name: "failsOpenApiPostResponse", + description: "", + inputSchema: EmptyInputSchema, + handler: () => + Effect.fail( + new FakeOpenApiInvocationError({ + message: "Failed to read response body", + statusCode: Option.some(502), + cause: { _tag: "ResponseError", note: "internal parser detail" }, + }), + ), + }, ], }, ], @@ -111,3 +175,80 @@ describe("internal-error leak audit (opaque defects)", () => { }), ); }); + +describe("openapi pre-flight invocation errors (expected failures)", () => { + it.effect("missing path parameter surfaces verbatim as invalid_tool_arguments", () => + Effect.gen(function* () { + const executor = yield* createExecutor(makeTestConfig({ plugins: [leakyPlugin()] as const })); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const result = yield* invoker.invoke({ path: "leaky.failsPreflightPathParam", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "invalid_tool_arguments", + message: "Missing required path parameter: datasourceId", + }, + }); + }), + ); + + it.effect("missing request body surfaces verbatim as invalid_tool_arguments", () => + Effect.gen(function* () { + const executor = yield* createExecutor(makeTestConfig({ plugins: [leakyPlugin()] as const })); + const invoker = makeExecutorToolInvoker(executor, { + invokeOptions: { onElicitation: acceptAll }, + }); + + const result = yield* invoker.invoke({ path: "leaky.failsPreflightBody", args: {} }); + expect(result).toEqual({ + ok: false, + error: { + code: "invalid_tool_arguments", + message: "Missing required request body", + }, + }); + }), + ); + + it.effect("transport failure (statusCode None but cause present) stays opaque", () => + 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.failsOpenApiTransport", 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; + expect(msg).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); + expect(msg).not.toContain("internal.service.local"); + expect(msg).not.toContain("tok-456"); + expect(msg).not.toContain("HTTP request failed"); + }), + ); + + it.effect("post-response failure (statusCode Some) stays opaque", () => + 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.failsOpenApiPostResponse", 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; + expect(msg).toMatch(/^Internal tool error \[[0-9a-f]{8}\]$/); + expect(msg).not.toContain("Failed to read response body"); + expect(msg).not.toContain("internal parser detail"); + }), + ); +}); diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index 8ce6d31bc..be6105d6d 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -148,6 +148,29 @@ const validationIssues = (value: unknown): readonly unknown[] | null => { return Array.isArray(issues) ? issues : null; }; +// Pre-flight OpenAPI invocation failures (missing/unresolved path params, +// missing or malformed request body) are caller-argument errors raised before +// any HTTP request is sent. Their messages are generated locally from the +// spec (no upstream data, no transport internals), so they are safe to +// surface, and actionable: the fix is renaming or adding an argument. +// Discriminator: pre-flight raises carry `statusCode: None` and no `cause`. +// Transport failures share `statusCode: None` but wrap the raw client error +// (which can hold internal URLs) as `cause`, and post-response failures carry +// a status code; both stay on the opaque-defect path. Matched structurally +// because this package does not depend on the openapi plugin. +const openApiPreflightMessage = (value: unknown): string | null => { + if (!Predicate.isTagged(value, "OpenApiInvocationError")) return null; + const err = value as { + readonly message?: unknown; + readonly statusCode?: unknown; + readonly cause?: unknown; + }; + if (err.cause !== undefined) return null; + if (!Predicate.isTagged(err.statusCode, "None")) return null; + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: structural match on the openapi plugin's tagged error; the pre-flight message is locally generated (no upstream data) and is the payload being surfaced + return typeof err.message === "string" && err.message.length > 0 ? err.message : null; +}; + const credentialResolutionToolFailure = (input: { readonly label: string; readonly message: string; @@ -186,7 +209,8 @@ const expectedToolFailure = (value: unknown): ToolError | null => { }; } if (Predicate.isTagged(value, "ToolInvocationError")) { - const issues = validationIssues((value as { readonly cause?: unknown }).cause); + const cause = (value as { readonly cause?: unknown }).cause; + const issues = validationIssues(cause); if (issues) { return { code: "invalid_tool_arguments", @@ -194,6 +218,13 @@ const expectedToolFailure = (value: unknown): ToolError | null => { details: { issues }, }; } + const preflight = openApiPreflightMessage(cause); + if (preflight) { + return { + code: "invalid_tool_arguments", + message: preflight, + }; + } } return null; }; diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 5dfd80b19..530462a8a 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -2935,6 +2935,17 @@ export const createExecutor = { + if (policy.action === "approve") return false; + return policy.action === "require_approval" || annotations?.requiresApproval === true; + }; + const enforceApproval = ( annotations: ToolAnnotations | undefined, address: ToolAddress, @@ -2943,9 +2954,8 @@ export const createExecutor = Effect.gen(function* () { - if (policy.action === "approve") return; + if (!approvalRequired(annotations, policy)) return; const policyForcesApproval = policy.action === "require_approval"; - if (!policyForcesApproval && !annotations?.requiresApproval) return; const message = annotations?.approvalDescription ? annotations.approvalDescription : policyForcesApproval && policy.pattern @@ -3154,6 +3164,16 @@ export const createExecutor = { readonly elicit: Elicit; } +/** Input for `validateToolArgs` — no credential/elicit: validation runs + * before approval enforcement and credential resolution, so it must depend + * on nothing but the tool row and the caller's args. */ +export interface ValidateToolArgsInput { + readonly ctx: PluginCtx; + readonly toolRow: ToolInvocationRow; + readonly args: unknown; +} + /** Called when the executor removes / refreshes a connection owned by this * plugin's integration — plugin-side cleanup or re-resolution only; the * executor handles the core tool rows. */ @@ -522,6 +531,16 @@ export interface PluginSpec< * address. The plugin applies `input.credential` to the outbound request. */ readonly invokeTool?: (input: InvokeToolInput) => Effect.Effect; + /** Validate a dynamic tool's args before the executor enforces approval, + * so a call guaranteed to fail is rejected instead of pausing for an + * approval the user can only waste. Fail with the same error `invokeTool` + * would raise for those args; succeed (void) when the args would reach the + * wire. Must be side-effect free: no credential use, no elicitation, no + * outbound request. Omit to skip pre-approval validation. */ + readonly validateToolArgs?: ( + input: ValidateToolArgsInput, + ) => Effect.Effect; + /** Bulk resolve annotations for a set of tool rows under one connection. */ readonly resolveAnnotations?: (input: { readonly ctx: PluginCtx; diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 0018671e9..63ed632b6 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -28,7 +28,7 @@ import { streamOperationBindingsFromStructure, } from "./extract"; import { compileToolDefinitions, type ToolDefinition } from "./definitions"; -import { annotationsForOperation, invokeWithLayer } from "./invoke"; +import { annotationsForOperation, buildRequest, invokeWithLayer } from "./invoke"; import { parse, type ParsedDocument } from "./parse"; import { parseEntry, structuralSplit, type KeepPathItem, type SpecStructure } from "./split"; import { type OpenapiStore, type StoredOperation } from "./store"; @@ -645,6 +645,27 @@ export const invokeOpenApiBackedTool = (input: { }); }); +/** Pre-approval argument validation: run the exact request-building pass + * `invoke` runs (path params, request body, base64 payloads) and discard the + * built request. Fails with the same pre-flight OpenApiInvocationError the + * real invocation would raise, so a call guaranteed to fail is rejected + * before the executor pauses it for approval. Legacy rows without a + * persisted binding skip validation (re-parsing the spec here would repeat + * invokeTool's heavy fallback for no user-visible gain). */ +export const validateOpenApiBackedToolArgs = (input: { + readonly ctx: PluginCtx; + readonly toolRow: { readonly integration: string; readonly name: string }; + readonly args: unknown; +}) => + Effect.gen(function* () { + const operation = yield* input.ctx.storage.getOperation( + input.toolRow.integration, + input.toolRow.name, + ); + if (!operation) return; + yield* buildRequest(operation.binding, (input.args ?? {}) as Record, {}); + }); + export const resolveOpenApiBackedAnnotations = (input: { readonly ctx: PluginCtx; readonly integration: string; diff --git a/packages/plugins/openapi/src/sdk/index.ts b/packages/plugins/openapi/src/sdk/index.ts index ea9ef5041..f2657a10d 100644 --- a/packages/plugins/openapi/src/sdk/index.ts +++ b/packages/plugins/openapi/src/sdk/index.ts @@ -12,7 +12,7 @@ export { type ByteRange, type KeepPathItem, } from "./split"; -export { invoke, invokeWithLayer, annotationsForOperation } from "./invoke"; +export { invoke, invokeWithLayer, buildRequest, annotationsForOperation } from "./invoke"; export { buildDefsJsonStreaming, compileAndPersistOpenApiOperations, @@ -28,6 +28,7 @@ export { openApiToolDefsFromCompiled, resolveOpenApiBackedAnnotations, resolveOpenApiBackedTools, + validateOpenApiBackedToolArgs, type CompiledOpenApiSpec, type OpenApiPersistResult, } from "./backing"; diff --git a/packages/plugins/openapi/src/sdk/invoke.ts b/packages/plugins/openapi/src/sdk/invoke.ts index bf2d0c5dd..0ba054d53 100644 --- a/packages/plugins/openapi/src/sdk/invoke.ts +++ b/packages/plugins/openapi/src/sdk/invoke.ts @@ -643,25 +643,22 @@ const applyRequestBody = ( }; // --------------------------------------------------------------------------- -// Public API — invoke a single operation +// Public API — build the outbound request for an operation +// +// The full pre-flight phase of an invocation: read/validate caller args +// (path params, request body, base64 payloads) and construct the +// HttpClientRequest. Pure — nothing is sent. Exposed separately so the +// executor's approval flow can validate args BEFORE raising an approval +// elicitation, guaranteeing the validation semantics are exactly what +// `invoke` applies (one code path, not a parallel re-implementation). // --------------------------------------------------------------------------- -export const invoke = Effect.fn("OpenApi.invoke")(function* ( +export const buildRequest = Effect.fn("OpenApi.buildRequest")(function* ( operation: OperationBinding, args: Record, resolvedHeaders: Record, sourceQueryParams: Record = {}, ) { - const client = yield* HttpClient.HttpClient; - - yield* Effect.annotateCurrentSpan({ - "http.method": operation.method.toUpperCase(), - "http.route": operation.pathTemplate, - "plugin.openapi.method": operation.method.toUpperCase(), - "plugin.openapi.path_template": operation.pathTemplate, - "plugin.openapi.headers.resolved_count": Object.keys(resolvedHeaders).length, - }); - const resolvedPath = yield* resolvePath(operation.pathTemplate, args, operation.parameters); const path = resolvedPath.startsWith("/") ? resolvedPath : `/${resolvedPath}`; @@ -808,6 +805,31 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( request = applyHeaders(request, resolvedHeaders); + return request; +}); + +// --------------------------------------------------------------------------- +// Public API — invoke a single operation +// --------------------------------------------------------------------------- + +export const invoke = Effect.fn("OpenApi.invoke")(function* ( + operation: OperationBinding, + args: Record, + resolvedHeaders: Record, + sourceQueryParams: Record = {}, +) { + const client = yield* HttpClient.HttpClient; + + yield* Effect.annotateCurrentSpan({ + "http.method": operation.method.toUpperCase(), + "http.route": operation.pathTemplate, + "plugin.openapi.method": operation.method.toUpperCase(), + "plugin.openapi.path_template": operation.pathTemplate, + "plugin.openapi.headers.resolved_count": Object.keys(resolvedHeaders).length, + }); + + const request = yield* buildRequest(operation, args, resolvedHeaders, sourceQueryParams); + const response = yield* client.execute(request).pipe( Effect.mapError( (err) => diff --git a/packages/plugins/openapi/src/sdk/plugin.test.ts b/packages/plugins/openapi/src/sdk/plugin.test.ts index 05519ce2e..5829c648a 100644 --- a/packages/plugins/openapi/src/sdk/plugin.test.ts +++ b/packages/plugins/openapi/src/sdk/plugin.test.ts @@ -565,6 +565,40 @@ describe("OpenAPI Plugin", () => { ), ); + it.effect("rejects invalid args before raising the approval elicitation", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* servePluginTestApi(); + const executor = yield* createExecutor(makeTestConfig({ plugins: testPlugins() })); + + const conn = yield* addOpenApiTestConnection(executor, server, { slug: "test" }); + const calls = { count: 0 }; + // createItem requires approval (POST) and a request body. Omitting + // the body must fail pre-flight validation WITHOUT consuming an + // approval: the user would otherwise approve a call that can only + // fail. + const failure = yield* executor + .execute( + conn.address("items.createItem"), + {}, + { + onElicitation: () => + Effect.sync(() => { + calls.count++; + return { action: "accept" as const, content: {} }; + }), + }, + ) + .pipe(Effect.flip); + + expect(calls.count).toBe(0); + expect(Predicate.isTagged(failure, "ToolInvocationError")).toBe(true); + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: asserts the exact caller-facing message the pre-flight failure carries + expect((failure as { message: string }).message).toBe("Missing required request body"); + }), + ), + ); + it.effect("describes OpenAPI invocation results payload-first with http meta beside data", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/plugins/openapi/src/sdk/plugin.ts b/packages/plugins/openapi/src/sdk/plugin.ts index a6de92ddd..d5bd42e09 100644 --- a/packages/plugins/openapi/src/sdk/plugin.ts +++ b/packages/plugins/openapi/src/sdk/plugin.ts @@ -44,6 +44,7 @@ import { openApiStoredOperationsFromCompiled, resolveOpenApiBackedAnnotations, resolveOpenApiBackedTools, + validateOpenApiBackedToolArgs, } from "./backing"; import { resolveServerUrl } from "./openapi-utils"; @@ -1031,6 +1032,9 @@ export const openApiPlugin = definePlugin((options?: OpenApiPluginOptions) => { }); }, + validateToolArgs: ({ ctx: validateCtx, toolRow, args }) => + validateOpenApiBackedToolArgs({ ctx: validateCtx, toolRow, args }), + resolveAnnotations: ({ ctx: annotationsCtx, integration, toolRows }) => resolveOpenApiBackedAnnotations({ ctx: annotationsCtx,