From dc5c1c113c468d384421a9c598bd3a6a081816a5 Mon Sep 17 00:00:00 2001 From: zak <36251971+zrm625@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:33:25 -0400 Subject: [PATCH] Allow a read-only MCP tool as a connection health check Allow an explicitly selected read-only MCP tool to validate connection health with JSON arguments and an optional identity field. Keep discovery-only health when no probe is configured. Fixes #2354 --- .changeset/mcp-health-tool-probes.md | 5 + .../health-checks-mcp-probe-ui.test.ts | 124 ++++++ packages/core/sdk/src/executor.ts | 41 +- packages/core/sdk/src/health-check.ts | 8 + packages/core/sdk/src/index.ts | 1 + packages/core/sdk/src/plugin.ts | 17 + .../mcp/src/react/McpAccountsPanel.tsx | 2 + packages/plugins/mcp/src/sdk/connection.ts | 35 ++ packages/plugins/mcp/src/sdk/discover.ts | 18 +- packages/plugins/mcp/src/sdk/errors.ts | 4 + .../mcp/src/sdk/health-candidates.test.ts | 110 +++++ .../plugins/mcp/src/sdk/health-candidates.ts | 86 ++++ .../plugins/mcp/src/sdk/health-probe.test.ts | 173 ++++++++ packages/plugins/mcp/src/sdk/health-probe.ts | 174 ++++++++ packages/plugins/mcp/src/sdk/invoke.ts | 9 +- packages/plugins/mcp/src/sdk/plugin.test.ts | 415 +++++++++++++++++- packages/plugins/mcp/src/sdk/plugin.ts | 224 +++++++--- .../src/components/add-account-modal.test.ts | 37 ++ .../src/components/add-account-modal.tsx | 79 +++- .../components/health-check-editor.test.tsx | 83 ++++ .../src/components/health-check-editor.tsx | 105 ++++- .../react/src/lib/health-check-args.test.ts | 44 +- packages/react/src/lib/health-check-args.ts | 21 + 23 files changed, 1692 insertions(+), 123 deletions(-) create mode 100644 .changeset/mcp-health-tool-probes.md create mode 100644 e2e/scenarios/health-checks-mcp-probe-ui.test.ts create mode 100644 packages/plugins/mcp/src/sdk/health-candidates.test.ts create mode 100644 packages/plugins/mcp/src/sdk/health-candidates.ts create mode 100644 packages/plugins/mcp/src/sdk/health-probe.test.ts create mode 100644 packages/plugins/mcp/src/sdk/health-probe.ts create mode 100644 packages/react/src/components/health-check-editor.test.tsx diff --git a/.changeset/mcp-health-tool-probes.md b/.changeset/mcp-health-tool-probes.md new file mode 100644 index 0000000000..8b8b34796f --- /dev/null +++ b/.changeset/mcp-health-tool-probes.md @@ -0,0 +1,5 @@ +--- +"executor": minor +--- + +Allow an explicitly selected read-only MCP tool to check connection health with JSON arguments and an optional account identity field. Keep discovery-only checks when no tool is configured, and report tool errors, rejected credentials, and insufficient scope without retaining upstream error text. diff --git a/e2e/scenarios/health-checks-mcp-probe-ui.test.ts b/e2e/scenarios/health-checks-mcp-probe-ui.test.ts new file mode 100644 index 0000000000..5dad7e49af --- /dev/null +++ b/e2e/scenarios/health-checks-mcp-probe-ui.test.ts @@ -0,0 +1,124 @@ +// Opt-in MCP health probes. Without a configured probe an MCP connection's +// health is discovery only (dial + tools/list), and the editor offers only +// tools the server declares read-only on every connection. +// +// The upstream is a per-run instance of the published MCP emulator. Its tools +// carry no annotations, so none of them may serve as a probe: the editor stays +// hidden, nothing is saved, and no check ever calls a tool. The positive path +// (picking and probing a read-only tool) needs an upstream that advertises +// `readOnlyHint`, which the published emulator does not; plugin tests cover it. +import { randomBytes } from "node:crypto"; + +import { Effect } from "effect"; +import { expect } from "@effect/vitest"; +import { composePluginApi } from "@executor-js/api/server"; +import { connectEmulator } from "@executor-js/emulate"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { variable } from "@executor-js/sdk/http-auth"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; + +import { createEmulatorInstance } from "../src/emulator-instance"; +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([mcpHttpPlugin()] as const); +const template = AuthTemplateSlug.make("bearer"); +const name = ConnectionName.make("main"); + +scenario( + "Health checks (UI) · MCP offers no unannotated tool and checks discovery only", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const slug = IntegrationSlug.make(`hc-mcp-probe-${randomBytes(4).toString("hex")}`); + const baseUrl = yield* createEmulatorInstance("mcp", "mcp-health-probe"); + const emulator = yield* Effect.promise(() => connectEmulator({ baseUrl })); + const credential = yield* Effect.promise(() => + emulator.credentials.mint({ type: "bearer-token" }), + ); + const token = credential.token; + if (!token) return yield* Effect.die("Emulator did not mint a bearer token"); + + yield* Effect.ensuring( + Effect.gen(function* () { + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: "Emulated MCP", + endpoint: `${baseUrl}/mcp`, + slug: String(slug), + authenticationTemplate: [ + { + slug: "bearer", + type: "apiKey", + headers: { Authorization: ["Bearer ", variable("token")] }, + }, + ], + }, + }); + yield* client.connections.create({ + payload: { owner: "org", name, integration: slug, template, value: token }, + }); + + const candidates = yield* client.integrations.healthCheckCandidates({ + params: { slug }, + }); + expect(candidates, "an unannotated tool is never a probe candidate").toEqual([]); + + yield* browser.session(identity, async ({ page, step }) => { + await step("The integration offers no MCP health check to set up", async () => { + await visit(page, `/integrations/${slug}`); + await page.getByText("main", { exact: true }).first().waitFor(); + expect( + await page.getByRole("heading", { name: "Health check", exact: true }).count(), + ).toBe(0); + expect(await page.getByRole("button", { name: "Set up", exact: true }).count()).toBe( + 0, + ); + }); + }); + + expect(yield* client.integrations.healthCheckGet({ params: { slug } })).toBeNull(); + const discoveryOnly = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name }, + query: {}, + }); + expect(discoveryOnly.status, "no probe configured: discovery health").toBe("healthy"); + expect(discoveryOnly.detail).toContain("discovery only"); + + const rejected = yield* client.connections.validate({ + payload: { owner: "org", integration: slug, template, value: "wrong-token" }, + }); + expect(rejected.status, "a rejected key is expired, not healthy").toBe("expired"); + expect(JSON.stringify(rejected)).not.toContain("Requires authentication"); + + const requests = yield* Effect.promise(() => emulator.ledger.list()); + expect( + requests.filter((request) => + JSON.stringify(request.request.body ?? null).includes('"tools/call"'), + ), + "no health check called a tool", + ).toEqual([]); + expect( + requests.some((request) => + JSON.stringify(request.request.body ?? null).includes('"tools/list"'), + ), + "discovery reached the emulator", + ).toBe(true); + }), + Effect.gen(function* () { + yield* client.connections + .remove({ params: { owner: "org", integration: slug, name } }) + .pipe(Effect.ignore); + yield* client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore); + }), + ); + }), + ), +); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 2a6b823a5f..b859bf11e1 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -177,6 +177,7 @@ import type { OwnerBinding, PluginCtx, PluginExtensions, + RefreshedToolCatalog, ResolveToolsResult, StaticIntegrationDecl, StaticToolDecl, @@ -3518,7 +3519,7 @@ export const createExecutor = "explicit" | "background", - ): Effect.Effect => + ): Effect.Effect => Effect.gen(function* () { const runtime = runtimes.get(integrationRow.plugin_id); const keys = yield* Effect.try({ @@ -3638,7 +3639,7 @@ export const createExecutor = rowToTool(row as ConnectionToolRow)); + return { + tools: keptRows.map((row) => rowToTool(row as ConnectionToolRow)), + complete: false, + }; } if ( @@ -3704,7 +3708,10 @@ export const createExecutor = rowToTool(row as ConnectionToolRow)); + return { + tools: keptRows.map((row) => rowToTool(row as ConnectionToolRow)), + complete: false, + }; } } @@ -3747,7 +3754,7 @@ export const createExecutor = + const tools = result.tools.map((tool: ToolDef) => rowToTool( { tenant: keys.tenant, @@ -3767,11 +3774,12 @@ export const createExecutor = ; + readonly deferred: Deferred.Deferred; mode: "explicit" | "background"; } const toolProductionInFlight = new Map(); @@ -3779,7 +3787,7 @@ export const createExecutor = => + ): Effect.Effect => Effect.suspend(() => { const key = `${ref.owner}:${String(ref.integration)}:${String(ref.name)}`; const existing = toolProductionInFlight.get(key); @@ -3789,7 +3797,7 @@ export const createExecutor = (), + deferred: Deferred.makeUnsafe(), mode: requestedMode, }; toolProductionInFlight.set(key, entry); @@ -4863,10 +4871,10 @@ export const createExecutor = => Effect.gen(function* () { @@ -4886,6 +4894,9 @@ export const createExecutor = + connectionsRefreshCatalog(ref).pipe(Effect.map((catalog) => catalog.tools)); + // No health-check capability ⇒ "unknown" rather than an error: the caller // can still render the connection, just without a liveness verdict. const unknownHealth = (): HealthCheckResult => ({ @@ -5603,8 +5614,8 @@ export const createExecutor = [] = []; - const deferred: Effect.Effect[] = []; + const urgent: Effect.Effect[] = []; + const deferred: Effect.Effect[] = []; for (const connection of connections) { const integrationRow = integrationBySlug.get(connection.integration); if (!integrationRow) continue; @@ -5641,6 +5652,7 @@ export const createExecutor = connectionsUpdate(ref, input), remove: (ref) => connectionsRemove(ref), refresh: (ref) => connectionsRefresh(ref), + refreshCatalog: (ref) => connectionsRefreshCatalog(ref), checkHealth: (ref, options) => connectionCheckHealth(ref, options), markToolsStale: (ref) => connectionsMarkToolsStale(ref), resolveValue: (ref) => resolveConnectionValueByRef(ref), diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index 9e39047d55..68e9a2eef9 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -187,6 +187,14 @@ export const HealthCheckCandidate = Schema.Struct({ /** True for mutating methods (post/put/patch/delete), ranked last and shown * with a warning, since a health check should be safe to run repeatedly. */ destructive: Schema.Boolean, + /** The user must pick this candidate themselves: surfaces never pre-select + * it, run it, or save it as the check on their own. Set when "read-only" + * is the upstream's own claim (MCP tool annotations are hints) rather than + * a protocol guarantee such as HTTP GET. */ + explicitSelectionOnly: Schema.optional(Schema.Boolean), + /** The arguments are one typed JSON object (MCP tool arguments), edited + * whole rather than as one text field per parameter. */ + jsonArguments: Schema.optional(Schema.Boolean), /** Operation summary / description for display, when known. */ summary: Schema.optional(Schema.String), /** The operation's parameters, so the editor can offer pinned-arg inputs. */ diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index 627d3de1de..9c3cccfb96 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -411,6 +411,7 @@ export { type IntegrationPresetAuthentication, type IntegrationPresetCatalogEntry, type ResolveToolsInput, + type RefreshedToolCatalog, type ResolveToolsResult, type ToolInvocationCredential, type HealthCheckInput, diff --git a/packages/core/sdk/src/plugin.ts b/packages/core/sdk/src/plugin.ts index e079f5ab8a..d54a06d607 100644 --- a/packages/core/sdk/src/plugin.ts +++ b/packages/core/sdk/src/plugin.ts @@ -265,6 +265,16 @@ export interface PluginCtx { readonly Tool[], ConnectionNotFoundError | IntegrationNotFoundError | OrgWriteDeniedError | StorageFailure >; + /** `refresh`, also reporting whether the listing was authoritative. When + * discovery came back incomplete, `tools` is the retained prior catalog + * and `complete` is false — for callers that must not treat a stale + * catalog as the live one. */ + readonly refreshCatalog: ( + ref: ConnectionRef, + ) => Effect.Effect< + RefreshedToolCatalog, + ConnectionNotFoundError | IntegrationNotFoundError | OrgWriteDeniedError | StorageFailure + >; /** Run the integration's declared health check against a saved connection * and persist the verdict. `ifStaleMs` serves the persisted verdict when * younger than that window, so concurrent readers collapse to one probe; @@ -405,6 +415,13 @@ export interface ResolveToolsResult { readonly health?: HealthCheckResult; } +/** A connection's catalog after a refresh. `complete` is false when the + * listing was non-authoritative and core kept the prior persisted catalog. */ +export interface RefreshedToolCatalog { + readonly tools: readonly Tool[]; + readonly complete: boolean; +} + export interface ProjectToolSchemaInput { readonly ctx: PluginCtx; readonly toolRow: ToolInvocationRow; diff --git a/packages/plugins/mcp/src/react/McpAccountsPanel.tsx b/packages/plugins/mcp/src/react/McpAccountsPanel.tsx index bb33a8a999..fd0dac9a97 100644 --- a/packages/plugins/mcp/src/react/McpAccountsPanel.tsx +++ b/packages/plugins/mcp/src/react/McpAccountsPanel.tsx @@ -5,6 +5,7 @@ import * as Exit from "effect/Exit"; import { IntegrationSlug } from "@executor-js/sdk/shared"; import type { IntegrationAccountHandoff } from "@executor-js/sdk/client"; +import { HealthCheckEditor } from "@executor-js/react/components/health-check-editor"; import { AccountsSection } from "@executor-js/react/components/accounts-section"; import type { AuthMethod, Placement } from "@executor-js/react/lib/auth-placements"; import { @@ -108,6 +109,7 @@ export default function McpAccountsPanel(props: { createCustomMethod={canConfigureAuth ? createCustomMethod : undefined} removeCustomMethod={canConfigureAuth ? removeCustomMethod : undefined} /> + ); } diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index f8ddefca7e..06f0bfcac5 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -430,6 +430,40 @@ const connectionFromClient = (client: Client): McpConnection => ({ close: () => client.close(), }); +type Transport = Parameters[0]; + +/** + * Every MCP revision requires `content` on a `tools/call` result, and the + * 2026 codec refuses a result without it. The 2025 codec instead defaults it + * to `[]` for SDK v1 parity, so `{}` or a bare `structuredContent` would reach + * callers as an empty success. Marking the absence as `null` on the raw + * response lets the SDK's own result validation refuse it there too. Call + * after a legacy-era `connect`, which installs the handler this wraps. + */ +const requireToolCallContent = (sdk: McpClientSdk, transport: Transport): void => { + const toolCalls = new Set(); + const send = transport.send.bind(transport); + transport.send = (message, options) => { + if (sdk.client.isJSONRPCRequest(message) && message.method === "tools/call") { + toolCalls.add(message.id); + } + return send(message, options); + }; + const deliver = transport.onmessage; + transport.onmessage = (message, extra) => { + // Any response, result or error, settles its request; only a result is refined. + if ( + sdk.client.isJSONRPCResponse(message) && + message.id !== undefined && + toolCalls.delete(message.id) && + sdk.client.isJSONRPCResultResponse(message) + ) { + message.result.content ??= null; + } + deliver?.(message, extra); + }; +}; + const connectionFailure = ( transport: string, message: string, @@ -540,6 +574,7 @@ const connectClient = (input: { attributes: { "plugin.mcp.transport": input.transport }, }), ); + if (client.getProtocolEra() === "legacy") requireToolCallContent(sdk, transportInstance); return connectionFromClient(client); }); diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index d3c672361d..0b128ececc 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -7,7 +7,7 @@ import { Duration, Effect, Option, Predicate, Schema } from "effect"; import { hasNestedOAuthReauthorization, type McpConnection, type McpConnector } from "./connection"; import { McpToolDiscoveryError } from "./errors"; import { createMcpConnector, type ConnectorInput } from "./connection"; -import { httpStatusFromCause } from "./http-status"; +import { httpStatusFromCause, insufficientScopeFromCause } from "./http-status"; import { decodeListToolsPage, extractManifestFromListToolsResult, @@ -89,7 +89,18 @@ const listAllTools = ( // implementing it; callers can retry with legacy negotiation. const modernContractViolation = connection.client.getProtocolEra?.() === "modern" && failure?.code === "INVALID_RESULT"; + // Raised below the SDK by the fetch adapter (connection.ts) on an + // RFC 6750 challenge, or recognised in a 403 body. + if (Predicate.isTagged(cause, "McpInsufficientScopeError")) { + return new McpToolDiscoveryError({ + stage: "list_tools", + message: "Failed listing MCP tools (HTTP 403: insufficient scope)", + httpStatus: 403, + insufficientScope: true, + }); + } const httpStatus = httpStatusFromCause(cause); + const insufficientScope = httpStatus === 403 && insufficientScopeFromCause(cause); return new McpToolDiscoveryError({ stage: "list_tools", message: @@ -100,6 +111,7 @@ const listAllTools = ( : `Failed listing MCP tools (HTTP ${httpStatus})`, ...(httpStatus === undefined ? {} : { httpStatus }), ...(modernContractViolation ? { modernContractViolation: true } : {}), + ...(insufficientScope ? { insufficientScope: true } : {}), }); }, }); @@ -204,12 +216,16 @@ export const discoverTools = ( const timedOut = Predicate.isTagged(failure, "McpConnectionError") && failure.failureKind === "timeout"; + const insufficientScope = + Predicate.isTagged(failure, "McpConnectionError") && + failure.insufficientScope === true; return new McpToolDiscoveryError({ stage: "connect", message: `Failed connecting to MCP server: ${failure.message}`, ...(httpStatus !== undefined ? { httpStatus } : {}), ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), ...(timedOut ? { timedOut } : {}), + ...(insufficientScope ? { insufficientScope } : {}), }); }), ), diff --git a/packages/plugins/mcp/src/sdk/errors.ts b/packages/plugins/mcp/src/sdk/errors.ts index 73e59db64b..78660fd5a5 100644 --- a/packages/plugins/mcp/src/sdk/errors.ts +++ b/packages/plugins/mcp/src/sdk/errors.ts @@ -56,6 +56,10 @@ export class McpToolDiscoveryError extends Schema.TaggedErrorClass ({ + toolId: name, + toolName: name, + description: null, + inputSchema, + annotations: { readOnlyHint: true }, +}); + +const names = (catalogs: readonly (readonly McpToolManifestEntry[])[]) => + compatibleHealthCandidates(catalogs).map((candidate) => candidate.operation); + +describe("integration-wide MCP health candidates", () => { + it("offers only tools that every connection lists read-only with identical schemas", () => { + const common = operation("health"); + expect( + names([ + [common, operation("only_a")], + [common, operation("only_b")], + ]), + ).toEqual(["health"]); + for (const peer of [ + { ...common, annotations: { readOnlyHint: false } }, + { ...common, inputSchema: { type: "object", properties: { limit: { type: "number" } } } }, + { ...common, outputSchema: { type: "string" } }, + ]) { + expect(names([[common], [peer]])).toEqual([]); + } + }); + + it("offers nothing when any catalog is empty, unreadable, or ambiguous", () => { + const common = operation("health"); + expect(names([[common], []])).toEqual([]); + expect(names([])).toEqual([]); + expect(names([[common], [common, common]])).toEqual([]); + expect(names([[common, common]])).toEqual([]); + }); + + it("treats only read-only, non-destructive annotations as probe-safe", () => { + const tool = operation("health"); + for (const annotations of [ + { readOnlyHint: true, destructiveHint: true }, + { destructiveHint: false }, + undefined, + ]) { + expect(names([[{ ...tool, annotations }]])).toEqual([]); + } + }); + + it("refuses a schema that requires an undeclared or malformed argument list", () => { + expect(names([[operation("health", { type: "object", required: ["missing"] })]])).toEqual([]); + expect(names([[operation("health", { type: "object", required: "limit" })]])).toEqual([]); + }); + + it("ignores key order and offers JSON-object arguments that must be picked explicitly", () => { + const first = operation("health", { + type: "object", + properties: { + limit: { type: "number", description: "Page size" }, + body: { type: "string" }, + options: { type: "object" }, + }, + required: ["limit", "body"], + }); + const peer = { + ...first, + inputSchema: { + required: ["limit", "body"], + properties: { + options: { type: "object" }, + body: { type: "string" }, + limit: { description: "Page size", type: "number" }, + }, + type: "object", + }, + }; + const [candidate, ...rest] = compatibleHealthCandidates([[first], [peer]]); + expect(rest).toEqual([]); + expect(candidate).toMatchObject({ + operation: "health", + method: "mcp", + destructive: false, + explicitSelectionOnly: true, + jsonArguments: true, + requiredArgCount: 2, + }); + expect(candidate?.parameters).toEqual([ + { name: "limit", location: "argument", required: true }, + { name: "body", location: "argument", required: true }, + { name: "options", location: "argument", required: false }, + ]); + }); + + it("projects identity fields from the output schema, following its $defs", () => { + const outputSchema = { + type: "object", + properties: { user: { $ref: "#/$defs/User" } }, + $defs: { User: { type: "object", properties: { email: { type: "string" } } } }, + }; + const [candidate] = compatibleHealthCandidates([[{ ...operation("whoami"), outputSchema }]]); + expect(candidate?.responseFields).toEqual([{ path: "user.email", type: "string" }]); + }); +}); diff --git a/packages/plugins/mcp/src/sdk/health-candidates.ts b/packages/plugins/mcp/src/sdk/health-candidates.ts new file mode 100644 index 0000000000..f09e3dbc25 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/health-candidates.ts @@ -0,0 +1,86 @@ +import { Equal, Option, Schema } from "effect"; +import { projectResponseFields, type HealthCheckCandidate } from "@executor-js/sdk/core"; + +import type { McpToolManifestEntry } from "./manifest"; + +/** A health probe runs unattended and repeatedly, so only a tool the server + * lists exactly once, declared read-only and not also destructive, may serve + * as one. */ +export const findProbeTool = ( + tools: readonly McpToolManifestEntry[], + name: string, +): McpToolManifestEntry | undefined => { + const [tool, ...duplicates] = tools.filter((entry) => entry.toolName === name); + return tool !== undefined && + duplicates.length === 0 && + tool.annotations?.readOnlyHint === true && + tool.annotations.destructiveHint !== true + ? tool + : undefined; +}; + +const InputSchema = Schema.Struct({ + properties: Schema.optional(Schema.Record(Schema.String, Schema.Unknown)), + required: Schema.optional(Schema.Array(Schema.String)), +}); +const decodeInputSchema = Schema.decodeUnknownOption(InputSchema); + +const SchemaDefs = Schema.Struct({ + $defs: Schema.optional(Schema.Record(Schema.String, Schema.Unknown)), +}); +const decodeSchemaDefs = Schema.decodeUnknownOption(SchemaDefs); + +/** Project a tool into a candidate whose arguments are edited as one JSON + * object. A schema that requires an argument it never declares cannot be + * filled in, so that tool is not offered. */ +const toCandidate = (tool: McpToolManifestEntry): HealthCheckCandidate[] => { + const input = Option.getOrUndefined(decodeInputSchema(tool.inputSchema)); + if (input === undefined) return []; + const properties = input.properties ?? {}; + const required = new Set(input.required ?? []); + if ([...required].some((name) => !Object.hasOwn(properties, name))) return []; + const defs = Option.getOrUndefined(decodeSchemaDefs(tool.outputSchema))?.$defs; + return [ + { + operation: tool.toolName, + method: "mcp", + destructive: false, + explicitSelectionOnly: true, + jsonArguments: true, + requiredArgCount: required.size, + ...(tool.description === null ? {} : { summary: tool.description }), + parameters: Object.keys(properties).map((name) => ({ + name, + location: "argument", + required: required.has(name), + })), + responseFields: projectResponseFields(tool.outputSchema, defs), + }, + ]; +}; + +/** + * The tools that can serve as the integration's health probe. One spec runs + * against every connection, so a tool qualifies only when every catalog has + * it as a probe tool with identical input and output schemas. An empty catalog + * (including one that could not be read) therefore empties the result instead + * of being skipped. + */ +export const compatibleHealthCandidates = ( + catalogs: readonly (readonly McpToolManifestEntry[])[], +): readonly HealthCheckCandidate[] => { + const [first = [], ...rest] = catalogs; + return first.flatMap((tool) => + findProbeTool(first, tool.toolName) === tool && + rest.every((catalog) => { + const peer = findProbeTool(catalog, tool.toolName); + return ( + peer !== undefined && + Equal.equals(peer.inputSchema, tool.inputSchema) && + Equal.equals(peer.outputSchema, tool.outputSchema) + ); + }) + ? toCandidate(tool) + : [], + ); +}; diff --git a/packages/plugins/mcp/src/sdk/health-probe.test.ts b/packages/plugins/mcp/src/sdk/health-probe.test.ts new file mode 100644 index 0000000000..c7e9377c59 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/health-probe.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, it } from "@effect/vitest"; +import type { ContentBlock } from "@modelcontextprotocol/client"; + +import { classifyMcpHealthFailure, classifyMcpHealthProbe } from "./health-probe"; + +describe("configured MCP health probe", () => { + it("treats an isError result as a failure despite a successful transport", () => { + const result = classifyMcpHealthProbe({ + isError: true, + content: [{ type: "text", text: "SESSION_REQUIRED_OR_INVALID" }], + structuredContent: { email: "synthetic@example.test" }, + }); + expect(result).toEqual({ + status: "degraded", + checkedAt: expect.any(Number), + reason: "probe_failed", + detail: "MCP health probe returned an error.", + }); + }); + + it("keeps no tool error text, not even a code-shaped one", () => { + const errorContents: ContentBlock[][] = [ + [{ type: "text", text: "synthetic private account and message" }], + [{ type: "text", text: "CUSTOMER_PRIVATE_NAME" }], + [ + { type: "text", text: "FIRST_CODE" }, + { type: "text", text: "SECOND_CODE" }, + ], + [{ type: "text", text: "KEY_SYNTHETICSECRET" }], + ]; + for (const content of errorContents) { + const result = classifyMcpHealthProbe( + { isError: true, content }, + { secrets: ["SYNTHETICSECRET"] }, + ); + expect(result.detail).toBe("MCP health probe returned an error."); + expect(result.responseSample).toBeUndefined(); + expect(JSON.stringify(result)).not.toMatch(/PRIVATE|private|CODE|SYNTHETICSECRET/); + } + }); + + // The SDK client has already validated the rest of the result; structured + // output is the one part it leaves untyped. + it("refuses structured output that is not a JSON object", () => { + for (const structuredContent of [42, "text", null, ["array"]]) { + expect(classifyMcpHealthProbe({ content: [], structuredContent })).toMatchObject({ + status: "degraded", + reason: "probe_failed", + detail: "Malformed MCP probe response.", + }); + } + }); + + it("takes identity from structured output and redacts credential values", () => { + const result = classifyMcpHealthProbe( + { + content: [], + structuredContent: { name: "synthetic", token: "x", innocent: "pre-secret-post" }, + }, + { identityField: "name", secrets: ["secret", ""] }, + ); + expect(result.status).toBe("healthy"); + expect(result.identity).toBe("synthetic"); + expect(result.responseSample).toEqual([ + { path: "name", value: "synthetic" }, + { path: "token", value: "[redacted]" }, + { path: "innocent", value: "pre-[redacted]-post" }, + ]); + }); + + it("redacts an identity field that names a credential or echoes one", () => { + const structuredContent = { api_key: "sk-synthetic", login: "user-secret" }; + expect( + classifyMcpHealthProbe({ content: [], structuredContent }, { identityField: "api_key" }) + .identity, + ).toBe("[redacted]"); + expect( + classifyMcpHealthProbe( + { content: [], structuredContent }, + { identityField: "login", secrets: ["secret"] }, + ).identity, + ).toBe("user-[redacted]"); + }); + + it("reads identity from a later JSON text block unless structured output exists", () => { + const content: ContentBlock[] = [ + { type: "image", data: "", mimeType: "image/png" }, + { type: "text", text: "synthetic explanation" }, + { type: "text", text: '{"email":"synthetic@example.test"}' }, + ]; + expect(classifyMcpHealthProbe({ content }, { identityField: "email" })).toMatchObject({ + identity: "synthetic@example.test", + responseSample: [{ path: "email", value: "synthetic@example.test" }], + }); + expect( + classifyMcpHealthProbe( + { content, structuredContent: { email: "structured@example.test" } }, + { identityField: "email" }, + ).identity, + ).toBe("structured@example.test"); + }); + + it("stays healthy without identity when the response has no usable body", () => { + const result = classifyMcpHealthProbe( + { content: [{ type: "text", text: "plain prose" }] }, + { identityField: "email" }, + ); + expect(result.status).toBe("healthy"); + expect(result.identity).toBeUndefined(); + expect(result.responseSample).toEqual([]); + }); + + it("drops a sample whose path holds a credential", () => { + const result = classifyMcpHealthProbe( + { + content: [], + structuredContent: { SYNTHETIC_CREDENTIAL: "ok", nested: { SYNTHETIC_CREDENTIAL: 1 } }, + }, + { secrets: ["SYNTHETIC_CREDENTIAL"] }, + ); + expect(result.status).toBe("healthy"); + expect(result.responseSample).toEqual([]); + expect(JSON.stringify(result)).not.toContain("SYNTHETIC_CREDENTIAL"); + }); + + it("removes overlapping and nested credentials whole", () => { + const result = classifyMcpHealthProbe( + { + content: [], + structuredContent: { + contained: "a-abc-b-abcdef-c", + overlapping: "x-abcXdef-y", + }, + }, + { secrets: ["abc", "abcdef", "abcX", "Xdef"] }, + ); + expect(result.responseSample).toEqual([ + { path: "contained", value: "a-[redacted]-b-[redacted]-c" }, + { path: "overlapping", value: "x-[redacted]-y" }, + ]); + }); +}); + +describe("MCP health failure classification", () => { + it("separates scope shortfall, rejected credential, and unavailable server", () => { + expect( + classifyMcpHealthFailure({ stage: "list_tools", httpStatus: 403, insufficientScope: true }), + ).toMatchObject({ status: "misconfigured", httpStatus: 403, reason: "upstream_status" }); + expect(classifyMcpHealthFailure({ stage: "connect", httpStatus: 401 })).toMatchObject({ + status: "expired", + reason: "upstream_status", + }); + expect( + classifyMcpHealthFailure({ stage: "list_tools", reauthorizationRequired: true }), + ).toMatchObject({ status: "expired", reason: "probe_failed" }); + expect(classifyMcpHealthFailure({ stage: "connect", timedOut: true })).toMatchObject({ + status: "degraded", + reason: "probe_timeout", + }); + expect(classifyMcpHealthFailure({ stage: "call_tool", httpStatus: 503 })).toEqual({ + status: "degraded", + checkedAt: expect.any(Number), + httpStatus: 503, + reason: "upstream_status", + detail: "MCP health probe call failed (HTTP 503).", + }); + expect(classifyMcpHealthFailure({ stage: "list_tools" })).toMatchObject({ + status: "degraded", + reason: "probe_failed", + detail: "Failed listing MCP tools.", + }); + }); +}); diff --git a/packages/plugins/mcp/src/sdk/health-probe.ts b/packages/plugins/mcp/src/sdk/health-probe.ts new file mode 100644 index 0000000000..cdbd99d928 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/health-probe.ts @@ -0,0 +1,174 @@ +import type { CallToolResult, ContentBlock } from "@modelcontextprotocol/client"; +import { Option, Schema } from "effect"; +import { + classifyHttpStatus, + extractIdentity, + extractResponseFields, + pathNamesASecret, + REDACTED_SAMPLE_VALUE, + type HealthCheckReason, + type HealthCheckResult, + type HealthStatus, +} from "@executor-js/sdk/core"; + +// The SDK client validates every `tools/call` result (each connector, the +// app-server bridge included, is a transport under it) except +// `structuredContent`, which it types as unknown. The protocol requires a JSON +// object there, and identity is read from it. +const decodeStructuredContent = Schema.decodeUnknownOption( + Schema.UndefinedOr(Schema.Record(Schema.String, Schema.Unknown)), +); +const decodeJsonText = Schema.decodeUnknownOption(Schema.UnknownFromJsonString); + +export interface McpHealthProbeOptions { + /** Dot-path into the response body whose value names the account. */ + readonly identityField?: string; + /** Credential values that must not appear in anything persisted. */ + readonly secrets?: readonly string[]; +} + +/** Structured output wins; otherwise the first text block that parses as JSON. + * A valid result with neither (prose, an image) is still a success, just one + * without a body to sample. */ +const responseBody = (structuredContent: unknown, content: readonly ContentBlock[]): unknown => + structuredContent ?? + content + .flatMap((block) => (block.type === "text" ? [decodeJsonText(block.text)] : [])) + .find(Option.isSome)?.value; + +/** Replace every occurrence of every secret. Matches are located on the + * original text and merged before replacing, so a secret that contains or + * overlaps another is removed whole rather than leaving a suffix behind. */ +export const redactSecrets = (text: string, secrets: readonly string[]): string => { + const spans: [number, number][] = []; + for (const secret of secrets) { + if (secret.length === 0) continue; + for (let at = text.indexOf(secret); at !== -1; at = text.indexOf(secret, at + 1)) { + spans.push([at, at + secret.length]); + } + } + if (spans.length === 0) return text; + spans.sort((a, b) => a[0] - b[0]); + let out = ""; + let cursor = 0; + let end = -1; + for (const [start, stop] of spans) { + if (start > end) { + out += text.slice(cursor, start) + REDACTED_SAMPLE_VALUE; + } + end = Math.max(end, stop); + cursor = end; + } + return out + text.slice(cursor); +}; + +const failed = (detail: string): HealthCheckResult => ({ + status: "degraded", + checkedAt: Date.now(), + reason: "probe_failed", + detail, +}); + +/** + * Classify a `tools/call` result from a configured health probe. A result that + * reports `isError` failed even though its HTTP exchange succeeded, and its + * content is never kept: tool error text is free-form upstream prose. Only a + * successful result is sampled. Sample values and the identity are redacted, + * and a sample whose path contains a credential is dropped, since a redacted + * path would no longer select the field it names. + */ +export const classifyMcpHealthProbe = ( + result: CallToolResult, + options: McpHealthProbeOptions = {}, +): HealthCheckResult => { + if (result.isError === true) return failed("MCP health probe returned an error."); + const structuredContent = decodeStructuredContent(result.structuredContent); + if (Option.isNone(structuredContent)) return failed("Malformed MCP probe response."); + + const secrets = options.secrets ?? []; + const scrub = (value: string): string => redactSecrets(value, secrets); + const body = responseBody(structuredContent.value, result.content); + const identityField = options.identityField ?? ""; + const identity = extractIdentity(body, identityField); + return { + status: "healthy", + checkedAt: Date.now(), + detail: "Configured MCP probe succeeded (point-in-time check).", + ...(identity === undefined + ? {} + : { identity: pathNamesASecret(identityField) ? REDACTED_SAMPLE_VALUE : scrub(identity) }), + responseSample: extractResponseFields(body, { scrub }).filter( + (sample) => scrub(sample.path) === sample.path, + ), + }; +}; + +/** The structural signals an MCP connect, `tools/list`, or `tools/call` + * failure carries. Messages are deliberately absent: they can embed upstream + * response text, and health detail is persisted. */ +export interface McpHealthFailure { + readonly stage: "connect" | "list_tools" | "call_tool"; + readonly httpStatus?: number; + readonly timedOut?: boolean; + readonly reauthorizationRequired?: boolean; + readonly insufficientScope?: boolean; + /** Fallback only: a status-less failure whose own text says the server + * refused authorization. Read here, never persisted. */ + readonly authRefusedWithoutStatus?: boolean; +} + +const STAGE_FAILURE_DETAIL: Record = { + connect: "Failed connecting to MCP server", + list_tools: "Failed listing MCP tools", + call_tool: "MCP health probe call failed", +}; + +/** + * Classify a failed health check from structural fields alone. A scope + * shortfall is `misconfigured`: re-authenticating the same grant cannot fix + * it. A rejected credential (401/403, reauthorization) is `expired`. Anything + * else means the server or probe is unavailable, which is `degraded`. + */ +export const classifyMcpHealthFailure = (failure: McpHealthFailure): HealthCheckResult => { + const { httpStatus } = failure; + const result = ( + status: HealthStatus, + reason: HealthCheckReason, + detail: string, + ): HealthCheckResult => ({ + status, + checkedAt: Date.now(), + ...(httpStatus === undefined ? {} : { httpStatus }), + reason, + detail, + }); + const statusReason = httpStatus === undefined ? "probe_failed" : "upstream_status"; + if (failure.insufficientScope === true) { + return result( + "misconfigured", + statusReason, + "MCP server requires additional scope. Update the credential grants.", + ); + } + if (failure.reauthorizationRequired === true) { + return result("expired", statusReason, "MCP OAuth reauthorization required."); + } + if (failure.timedOut === true) { + return result("degraded", "probe_timeout", "MCP server did not respond in time."); + } + const stage = STAGE_FAILURE_DETAIL[failure.stage]; + if (httpStatus === undefined) { + return result( + failure.authRefusedWithoutStatus === true ? "expired" : "degraded", + "probe_failed", + `${stage}.`, + ); + } + return classifyHttpStatus(httpStatus) === "expired" + ? result( + "expired", + "upstream_status", + `MCP server rejected the credential (HTTP ${httpStatus}).`, + ) + : result("degraded", "upstream_status", `${stage} (HTTP ${httpStatus}).`); +}; diff --git a/packages/plugins/mcp/src/sdk/invoke.ts b/packages/plugins/mcp/src/sdk/invoke.ts index ed67dd2312..dd5fa29854 100644 --- a/packages/plugins/mcp/src/sdk/invoke.ts +++ b/packages/plugins/mcp/src/sdk/invoke.ts @@ -16,7 +16,7 @@ import { Cause, Effect, Exit, Option, Predicate, Schema } from "effect"; -import type { ClientContext, ProtocolError } from "@modelcontextprotocol/client"; +import type { CallToolResult, ClientContext, ProtocolError } from "@modelcontextprotocol/client"; // SDK error classes come through the lazy loader; by the time a tool call can // fail, the connect path has always loaded the module (see client-module.ts). @@ -352,7 +352,8 @@ const installToolListChangedHandler = ( }; // --------------------------------------------------------------------------- -// Single tool call — install handlers, callTool, return raw result +// Single tool call — install handlers, callTool, return the result the SDK +// client validated against the protocol's CallToolResult schema // --------------------------------------------------------------------------- const useConnection = ( @@ -361,7 +362,7 @@ const useConnection = ( args: Record, elicit: Elicit, onToolListChanged: (() => void) | undefined, -): Effect.Effect => +): Effect.Effect => Effect.gen(function* () { const deadline = yield* Effect.acquireRelease( Effect.sync(() => makeActiveWorkDeadline()), @@ -457,7 +458,7 @@ export interface InvokeMcpToolInput { export const invokeMcpTool = ( input: InvokeMcpToolInput, ): Effect.Effect< - unknown, + CallToolResult, McpConnectionError | McpInvocationError | McpOAuthReauthorizationRequired > => Effect.gen(function* () { diff --git a/packages/plugins/mcp/src/sdk/plugin.test.ts b/packages/plugins/mcp/src/sdk/plugin.test.ts index 487874677b..a6b6beedbd 100644 --- a/packages/plugins/mcp/src/sdk/plugin.test.ts +++ b/packages/plugins/mcp/src/sdk/plugin.test.ts @@ -47,6 +47,7 @@ const JsonRpcId = Schema.Union([Schema.String, Schema.Number, Schema.Null]); const JsonRpcRequest = Schema.Struct({ id: Schema.optional(JsonRpcId), method: Schema.String, + params: Schema.optional(Schema.Unknown), }); type JsonRpcRequest = typeof JsonRpcRequest.Type; @@ -64,7 +65,20 @@ const jsonRpcResult = (request: JsonRpcRequest, result: unknown) => // scenario supplies that branch via a `CallToolResponder`. type CallToolResponder = (rpc: JsonRpcRequest) => ReturnType; -const callToolFixtureResponse = (rpc: JsonRpcRequest, callTool: CallToolResponder) => { +/** Answers a handshake or listing request in place of the scaffold, or + * returns undefined to let the scaffold answer. */ +type FixtureOverride = ( + rpc: JsonRpcRequest, +) => ReturnType | undefined; + +const callToolFixtureResponse = ( + rpc: JsonRpcRequest, + callTool: CallToolResponder, + readOnly = false, + override?: FixtureOverride, +) => { + const overridden = override?.(rpc); + if (overridden !== undefined) return overridden; if (rpc.method === "initialize") { return jsonRpcResult(rpc, { protocolVersion: "2025-06-18", @@ -80,6 +94,7 @@ const callToolFixtureResponse = (rpc: JsonRpcRequest, callTool: CallToolResponde tools: [ { name: "explode", + annotations: { readOnlyHint: readOnly }, description: "Returns a failure from tools/call", inputSchema: { type: "object", properties: {} }, }, @@ -92,7 +107,11 @@ const callToolFixtureResponse = (rpc: JsonRpcRequest, callTool: CallToolResponde return HttpServerResponse.text("Unexpected JSON-RPC method", { status: 400 }); }; -const serveCallToolServer = (callTool: CallToolResponder) => +const serveCallToolServer = ( + callTool: CallToolResponder, + readOnly = false, + override?: FixtureOverride, +) => serveTestHttpApp((request) => Effect.gen(function* () { if (request.method === "GET") { @@ -102,7 +121,7 @@ const serveCallToolServer = (callTool: CallToolResponder) => const body = yield* request.text.pipe(Effect.orDie); return Option.match(decodeJsonRpcRequest(body), { onNone: () => HttpServerResponse.text("Invalid JSON-RPC fixture request", { status: 400 }), - onSome: (rpc) => callToolFixtureResponse(rpc, callTool), + onSome: (rpc) => callToolFixtureResponse(rpc, callTool, readOnly, override), }); }), ); @@ -256,10 +275,12 @@ const seedCallToolExecutor = (input: { * transport gets a real authProvider — the production OAuth path where the * SDK's own challenge handling would otherwise swallow scope signals. */ oauth?: boolean; + readOnly?: boolean; + override?: FixtureOverride; }) => Effect.acquireRelease( Effect.gen(function* () { - const server = yield* serveCallToolServer(input.callTool); + const server = yield* serveCallToolServer(input.callTool, input.readOnly, input.override); const config = makeTestConfig({ plugins: [memoryCredentialsPlugin(), mcpPlugin()] as const, }); @@ -999,6 +1020,392 @@ describe("mcpPlugin", () => { ); } + describe("configured health probe", () => { + // Every case counts tools/call requests: the probe must call the selected + // tool at most once and never replay it to hide an auth failure. + const succeedWith = + (result: unknown): CallToolResponder => + (rpc) => + jsonRpcResult(rpc, result); + + const seedProbe = (input: { + slug: string; + readOnly?: boolean; + oauth?: boolean; + callTool?: CallToolResponder; + /** Applied only after seeding, so setup still discovers normally. */ + override?: FixtureOverride; + }) => + Effect.gen(function* () { + const calls: JsonRpcRequest[] = []; + let seeded = false; + const { executor } = yield* seedCallToolExecutor({ + slug: input.slug, + readOnly: input.readOnly ?? true, + ...(input.oauth ? { oauth: true } : {}), + override: (rpc) => (seeded ? input.override?.(rpc) : undefined), + callTool: (rpc) => { + calls.push(rpc); + return (input.callTool ?? succeedWith({ content: [] }))(rpc); + }, + }); + const validate = (spec?: { + operation: string; + args?: Record; + identityField?: string; + }) => + executor.connections.validate({ + owner: "org", + integration: IntegrationSlug.make(input.slug), + template: input.oauth ? AuthTemplateSlug.make("oauth2") : TEMPLATE, + value: input.oauth ? "static-access-token" : "", + ...(spec ? { spec } : {}), + }); + seeded = true; + return { executor, calls, validate } as const; + }); + + it.effect("lists read-only tools as candidates without calling any", () => + Effect.scoped( + Effect.gen(function* () { + const slug = "health_candidates"; + const { executor, calls } = yield* seedProbe({ slug, callTool: succeedWith({}) }); + const candidates = yield* executor.integrations.healthCheck.candidates( + IntegrationSlug.make(slug), + ); + expect(candidates.map((candidate) => candidate.operation)).toEqual(["explode"]); + expect(candidates[0]?.explicitSelectionOnly).toBe(true); + expect(calls).toEqual([]); + }), + ), + ); + + it.effect("offers no candidates while a connection's catalog cannot be listed", () => + Effect.scoped( + Effect.gen(function* () { + const slug = "health_candidates_stale"; + let listFails = false; + const { executor, calls } = yield* seedProbe({ + slug, + override: (rpc) => + listFails && rpc.method === "tools/list" + ? HttpServerResponse.jsonUnsafe({ + jsonrpc: "2.0", + id: rpc.id ?? null, + error: { code: -32603, message: "catalog unavailable" }, + }) + : undefined, + }); + const integration = IntegrationSlug.make(slug); + const candidates = executor.integrations.healthCheck + .candidates(integration) + .pipe(Effect.map((found) => found.map((candidate) => candidate.operation))); + expect(yield* candidates).toEqual(["explode"]); + + // Core keeps the prior catalog for ordinary callers over the outage, + // but that retained catalog is not evidence the tool is still offered. + listFails = true; + expect(yield* candidates).toEqual([]); + const ref = { owner: "org" as const, integration, name: ConnectionName.make("main") }; + const retained = yield* executor.connections.refresh(ref); + expect(retained.map((tool) => String(tool.name))).toEqual(["explode"]); + + listFails = false; + expect(yield* candidates).toEqual(["explode"]); + expect(calls).toEqual([]); + }), + ), + ); + + it.effect("offers identity paths that resolve against the probe's structured result", () => + Effect.scoped( + Effect.gen(function* () { + const slug = "health_candidate_identity"; + const { executor, calls, validate } = yield* seedProbe({ + slug, + callTool: succeedWith({ + content: [{ type: "text", text: "synthetic summary" }], + structuredContent: { user: { email: "synthetic@example.test" } }, + }), + override: (rpc) => + rpc.method === "tools/list" + ? jsonRpcResult(rpc, { + tools: [ + { + name: "explode", + annotations: { readOnlyHint: true }, + inputSchema: { type: "object", properties: {} }, + outputSchema: { + type: "object", + properties: { user: { $ref: "#/$defs/User" } }, + $defs: { + User: { type: "object", properties: { email: { type: "string" } } }, + }, + }, + }, + ], + }) + : undefined, + }); + const [candidate] = yield* executor.integrations.healthCheck.candidates( + IntegrationSlug.make(slug), + ); + // Paths address the tool's own body, not the CallToolResult + // envelope (`structuredContent.*`, `content`, `isError`). + expect(candidate?.responseFields).toEqual([{ path: "user.email", type: "string" }]); + expect(calls).toEqual([]); + + const identityField = candidate?.responseFields?.[0]?.path; + const result = yield* validate({ operation: "explode", identityField }); + expect(result).toMatchObject({ status: "healthy", identity: "synthetic@example.test" }); + expect(calls).toHaveLength(1); + }), + ), + ); + + it.effect("calls the selected tool once with typed JSON arguments", () => + Effect.scoped( + Effect.gen(function* () { + const { calls, validate } = yield* seedProbe({ + slug: "health_typed_args", + callTool: succeedWith({ + content: [{ type: "text", text: "{}" }], + structuredContent: { user: { email: "synthetic@example.test" } }, + }), + }); + const args = { limit: 1, enabled: false, tags: ["a"], options: { nested: null } }; + const result = yield* validate({ operation: "explode", args }); + expect(result).toMatchObject({ status: "healthy" }); + expect(calls.map((rpc) => rpc.params)).toEqual([{ name: "explode", arguments: args }]); + }), + ), + ); + + const scopeChallenge = () => + HttpServerResponse.text("do-not-leak synthetic detail", { + status: 403, + headers: { "www-authenticate": 'Bearer error="insufficient_scope", scope="health.read"' }, + }); + const onMethod = + (method: string, respond: CallToolResponder): FixtureOverride => + (rpc) => + rpc.method === method ? respond(rpc) : undefined; + const readOnlyTool = { + name: "explode", + annotations: { readOnlyHint: true }, + inputSchema: { type: "object", properties: {} }, + }; + const unavailableProbe = { + status: "degraded", + reason: "probe_failed", + detail: expect.stringContaining("unavailable or not read-only"), + }; + const scopeShortfall = { + status: "misconfigured", + httpStatus: 403, + detail: expect.stringContaining("additional scope"), + }; + + // Each case validates every listed operation (undefined: no probe + // configured), expects the same verdict carrying no upstream text, and + // counts the tools/call requests the probe sent. + const verdictCases: readonly { + readonly name: string; + readonly readOnly?: boolean; + readonly oauth?: boolean; + readonly callTool?: CallToolResponder; + readonly override?: FixtureOverride; + readonly operations?: readonly (string | undefined)[]; + readonly expected: Record; + readonly calls: number; + }[] = [ + { + name: "checks discovery only when no probe is configured", + operations: [undefined], + expected: { status: "healthy" }, + calls: 0, + }, + { + name: "refuses to call a configured tool that is missing or not read-only", + readOnly: false, + operations: ["explode", "missing"], + expected: unavailableProbe, + calls: 0, + }, + { + name: "refuses to call a tool name the server lists twice", + override: onMethod("tools/list", (rpc) => + jsonRpcResult(rpc, { tools: [readOnlyTool, readOnlyTool] }), + ), + expected: unavailableProbe, + calls: 0, + }, + { + name: "accepts an empty content list as a successful probe", + callTool: succeedWith({ content: [] }), + expected: { status: "healthy" }, + calls: 1, + }, + { + name: "treats an isError result over HTTP 200 as degraded", + callTool: succeedWith({ + isError: true, + content: [{ type: "text", text: "do-not-leak synthetic account" }], + }), + expected: { status: "degraded", reason: "probe_failed" }, + calls: 1, + }, + // Malformed results are refused by the SDK client every connector shares + // (a missing `content` included; see `requireToolCallContent`), or, for + // structured output it leaves untyped, by the probe itself. + ...[ + {}, + { structuredContent: { user: { email: "do-not-leak@example.test" } } }, + { content: [null] }, + { content: [{ type: "text" }] }, + { content: [{ type: "unknown-kind" }] }, + { content: [], isError: "yes" }, + { content: [], structuredContent: 42 }, + { content: [], structuredContent: ["array"] }, + ].map((result) => ({ + name: `refuses the malformed result ${JSON.stringify(result)}`, + callTool: succeedWith(result), + expected: { status: "degraded", reason: "probe_failed" }, + calls: 1, + })), + ...( + [ + [401, "expired"], + [403, "expired"], + [500, "degraded"], + ] as const + ).map(([status, expected]) => ({ + name: `classifies HTTP ${status} from the probe call without replaying it`, + callTool: httpStatusCallTool(status), + expected: { status: expected, httpStatus: status, reason: "upstream_status" }, + calls: 1, + })), + { + name: "reports a 401 from an OAuth probe call without replaying it", + oauth: true, + callTool: httpStatusCallTool(401), + expected: { status: "expired" }, + calls: 1, + }, + { + name: "reports a scope shortfall as misconfigured without replaying it", + oauth: true, + callTool: scopeChallenge, + expected: { ...scopeShortfall, reason: "upstream_status" }, + calls: 1, + }, + { + name: "keeps no upstream text from a refused tools/list", + override: onMethod("tools/list", (rpc) => + HttpServerResponse.jsonUnsafe({ + jsonrpc: "2.0", + id: rpc.id ?? null, + error: { code: -32603, message: "do-not-leak SYNTHETIC_CREDENTIAL" }, + }), + ), + operations: [undefined, "explode"], + expected: { + status: "degraded", + reason: "probe_failed", + detail: "Failed listing MCP tools.", + }, + calls: 0, + }, + ...(["initialize", "tools/list"] as const).map((method) => ({ + name: `reports a scope shortfall at ${method} as misconfigured`, + oauth: true, + override: onMethod(method, scopeChallenge), + operations: [undefined, "explode"], + expected: scopeShortfall, + calls: 0, + })), + ]; + + verdictCases.forEach( + ({ name, operations = ["explode"], expected, calls: sent, ...fixture }, i) => + it.effect(name, () => + Effect.scoped( + Effect.gen(function* () { + const { calls, validate } = yield* seedProbe({ + slug: `health_verdict_${i}`, + ...fixture, + }); + for (const operation of operations) { + const result = yield* validate(operation === undefined ? undefined : { operation }); + expect(result).toMatchObject(expected); + expect(JSON.stringify(result)).not.toMatch(/do-not-leak|SYNTHETIC_CREDENTIAL/); + } + expect(calls).toHaveLength(sent); + }), + ), + ), + ); + + it.effect("reports a scope shortfall at the probe's own handshake as misconfigured", () => + Effect.scoped( + Effect.gen(function* () { + // Discovery's handshake succeeds; the probe dials a second session, + // whose handshake is refused before any tools/call is sent. + let handshakes = 0; + const { calls, validate } = yield* seedProbe({ + slug: "health_scope_probe_connect", + oauth: true, + override: (rpc) => + rpc.method === "initialize" && ++handshakes > 1 ? scopeChallenge() : undefined, + }); + const result = yield* validate({ operation: "explode" }); + expect(result).toMatchObject({ + status: "misconfigured", + httpStatus: 403, + detail: expect.stringContaining("additional scope"), + }); + expect(handshakes).toBe(2); + expect(calls).toEqual([]); + }), + ), + ); + }); + // The 2025 codec would default a missing `content` to `[]`; every revision + // requires it, so a contentless result fails rather than reading as an + // empty success, while JSON-RPC errors and complete envelopes reach the + // caller intact on the same retained session. + it.effect("refuses a tools/call result without content and keeps complete envelopes", () => + Effect.scoped( + Effect.gen(function* () { + const envelope = { + content: [{ type: "text", text: "ok" }], + structuredContent: { ok: true }, + _meta: { trace: "synthetic" }, + }; + let result: unknown; + const { executor, toolAddress } = yield* seedCallToolExecutor({ + slug: "call_contentless", + callTool: (rpc) => + result === undefined ? jsonRpcErrorCallTool(-32000)(rpc) : jsonRpcResult(rpc, result), + }); + + expect( + yield* executor.execute(toolAddress, {}, { onElicitation: "accept-all" }), + ).toMatchObject({ ok: false, error: { code: "mcp_tool_error" } }); + // The same failure as any other result the SDK refuses as invalid. + result = {}; + const refused = yield* Effect.flip( + executor.execute(toolAddress, {}, { onElicitation: "accept-all" }), + ); + expect(refused.cause).toMatchObject({ sdkFailure: { code: "INVALID_RESULT" } }); + result = envelope; + expect( + yield* executor.execute(toolAddress, {}, { onElicitation: "accept-all" }), + ).toMatchObject({ ok: true, data: envelope }); + }), + ), + ); + // The lone-401 replay above is restricted to read-only methods. A // `tools/call` may have executed its side effect before the server answered // 401 (HTTP gives no such guarantee), so the adapter must never re-send it diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 079b89dce5..7044f83de5 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -19,7 +19,6 @@ import { tool, ToolResult, type AuthMethodDescriptor, - classifyHttpStatus, type HealthCheckResult, type Integration, type IntegrationConfig, @@ -30,6 +29,7 @@ import { type PluginCtx, type StaticToolSchema, type StorageFailure, + type Tool, type ToolAnnotations, type ToolDef, ToolName, @@ -49,10 +49,11 @@ import { createMcpConnectionPool } from "./connection-pool"; import { discoverToolsFromInput } from "./discover"; import { McpConnectionError, - type McpConnectionFailureKind, McpOAuthReauthorizationRequired, McpToolDiscoveryError, } from "./errors"; +import { compatibleHealthCandidates, findProbeTool } from "./health-candidates"; +import { classifyMcpHealthFailure, classifyMcpHealthProbe } from "./health-probe"; import { invokeMcpTool, isUnknownToolMessage } from "./invoke"; import { deriveMcpNamespace, type McpToolManifestEntry } from "./manifest"; import { mcpPresets } from "./presets"; @@ -77,43 +78,16 @@ import { const MCP_PLUGIN_ID = "mcp" as const; -/** Classify a failed liveness probe. The structural `httpStatus` carried on the - * connect error is the primary signal (401/403 = auth wall = expired); message - * substrings are only a fallback for causes with no status (OAuth - * re-authorization, servers whose auth rejection isn't an HTTP status). - * Anything else (server down, wrong transport) is degraded, not a credential - * problem. */ -const mcpLivenessFailureStatus = (failure: { - readonly message: string; - readonly httpStatus?: number; -}): "expired" | "degraded" => { - if (failure.httpStatus !== undefined) { - // A failed connect can't be healthy; the shared classifier decides - // expired vs degraded so "which statuses mean expired" lives in one place. - const classified = classifyHttpStatus(failure.httpStatus); - return classified === "expired" ? "expired" : "degraded"; - } - const lower = failure.message.toLowerCase(); - const authWalled = +/** Last-resort signal for a failure that carries no HTTP status: its own text + * says the server refused authorization. Used only to choose the status; the + * message is never copied into health detail. */ +const authRefusedWithoutStatus = (message: string): boolean => { + const lower = message.toLowerCase(); + return ( lower.includes("oauth re-authorization") || lower.includes("unauthorized") || - lower.includes("forbidden"); - return authWalled ? "expired" : "degraded"; -}; - -/** Enumerable failure MECHANISM for a failed liveness probe, beside the status - * classifier above: a hit deadline is `probe_timeout` (a slow-but-alive - * server, not a dead one), an HTTP verdict is `upstream_status`, and anything - * else (transport, protocol, config) is `probe_failed`. Structural fields - * only — never the message. */ -const mcpLivenessFailureReason = (failure: { - readonly httpStatus?: number; - readonly timedOut?: boolean; - readonly failureKind?: McpConnectionFailureKind; -}): "probe_timeout" | "upstream_status" | "probe_failed" => { - if (failure.timedOut === true || failure.failureKind === "timeout") return "probe_timeout"; - if (failure.httpStatus !== undefined) return "upstream_status"; - return "probe_failed"; + lower.includes("forbidden") + ); }; const legacyOAuthClientSlugCandidate = (value: string): string | null => { @@ -531,6 +505,14 @@ const mcpCallToolResultOutputSchema = (structuredContentSchema?: unknown): JsonS }; }; +const McpCallToolResultOutputSchema = Schema.Struct({ + properties: Schema.Struct({ structuredContent: Schema.Unknown }), + required: Schema.Array(Schema.String), +}); +const decodeMcpCallToolResultOutputSchema = Schema.decodeUnknownOption( + McpCallToolResultOutputSchema, +); + /** Build the executor-facing ToolDef for one discovered MCP tool, stamping the * real MCP tool name, the upstream annotations, and the tool's `_meta` map * into the persisted annotations so they survive to invokeTool with no @@ -557,6 +539,27 @@ const toToolDef = (entry: McpToolManifestEntry): ToolDef => { }; }; +/** Invert `toToolDef` for a persisted tool: the tool as discovered, its own + * output schema unwrapped from the CallToolResult envelope (undefined when + * it declared none, since the envelope then requires only `content`). */ +const fromToolDef = (tool: Tool): McpToolManifestEntry[] => { + const stamp = readStamp(tool.annotations); + if (!stamp) return []; + const envelope = Option.getOrUndefined(decodeMcpCallToolResultOutputSchema(tool.outputSchema)); + return [ + { + toolId: String(tool.name), + toolName: stamp.toolName, + description: tool.description, + inputSchema: tool.inputSchema, + outputSchema: envelope?.required.includes("structuredContent") + ? envelope.properties.structuredContent + : undefined, + ...(stamp.upstream ? { annotations: stamp.upstream } : {}), + }, + ]; +}; + const McpTextContent = Schema.Struct({ type: Schema.Literal("text"), text: Schema.String }); const McpToolCallEnvelope = Schema.Struct({ isError: Schema.optional(Schema.Boolean), @@ -1931,14 +1934,34 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { return out; }), - // Liveness-only health check. MCP has no usable identity source (no - // id_token/userinfo, no standard whoami), so this answers "is this - // credential still alive?" by dialing the server and listing tools (the same - // path resolveTools uses); identity stays the user-supplied connection label. - // Only checkHealth is implemented (no candidates/describe/set), so the - // operation/identity editor stays hidden while the status dot + "Check now" - // light up. - checkHealth: ({ ctx, credential }) => + // One spec probes every connection, so candidates are the read-only tools + // all of them agree on. Listing reads each connection's catalog + // (tools/list) and never calls a tool. A catalog that cannot be read — + // including a failed listing where core kept the prior catalog — counts as + // empty, which empties the intersection rather than guessing. + listHealthCheckCandidates: ({ ctx, integration }) => + Effect.gen(function* () { + const connections = yield* ctx.connections.list({ integration: integration.slug }); + const catalogs = yield* Effect.forEach(connections, (connection) => + ctx.connections + .refreshCatalog({ + owner: connection.owner, + integration: integration.slug, + name: connection.name, + }) + .pipe( + Effect.map(({ tools, complete }) => (complete ? tools.flatMap(fromToolDef) : [])), + Effect.orElseSucceed((): McpToolManifestEntry[] => []), + ), + ); + return compatibleHealthCandidates(catalogs); + }), + + // Without a spec this is a liveness check: dial the server and list tools + // (the resolveTools path); identity stays the user-supplied label. With a + // spec, it also calls exactly that tool, provided the live catalog still + // declares it read-only. + checkHealth: ({ ctx, credential, spec }) => Effect.gen(function* () { const parsed = parseMcpIntegrationConfig(credential.config); if (!parsed) { @@ -1969,40 +1992,99 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } } } - const connector = yield* buildConnectorInput( + const built = yield* buildConnectorInput( parsed, credential.values, credential.template === null ? null : String(credential.template), allowStdio, options?.httpClientLayer ?? ctx.httpClientLayer, - ); + ).pipe(Effect.option); + if (Option.isNone(built)) { + // The only rejection is a stdio server on a host that disallows it. + return { + status: "degraded" as const, + checkedAt: Date.now(), + detail: "MCP stdio transport is disabled on this host.", + reason: "probe_failed" as const, + } satisfies HealthCheckResult; + } + const connector = built.value; + const { manifest } = yield* discoverToolsFromInput(connector); - return yield* discoverToolsFromInput(connector).pipe( - Effect.map( - () => - ({ status: "healthy" as const, checkedAt: Date.now() }) satisfies HealthCheckResult, - ), - Effect.catchTag("McpToolDiscoveryError", (error) => - Effect.succeed({ - status: mcpLivenessFailureStatus(error), - checkedAt: Date.now(), - ...(error.httpStatus !== undefined ? { httpStatus: error.httpStatus } : {}), - detail: error.message, - reason: mcpLivenessFailureReason(error), - } satisfies HealthCheckResult), + if (!spec) { + return { + status: "healthy" as const, + checkedAt: Date.now(), + detail: "MCP discovery only; no upstream session probe configured.", + } satisfies HealthCheckResult; + } + // The same rule as candidate listing, against the live catalog. + const tool = findProbeTool(manifest.tools, spec.operation); + if (!tool) { + return { + status: "degraded" as const, + checkedAt: Date.now(), + detail: + "Configured MCP health probe is unavailable or not read-only. Update the server or choose a different probe.", + reason: "probe_failed" as const, + } satisfies HealthCheckResult; + } + // A tools/call is never replayed (see invoke.ts), so a rejected + // credential surfaces as a failure here rather than being retried. + const raw = yield* invokeMcpTool({ + toolId: tool.toolId, + toolName: tool.toolName, + args: spec.args ?? {}, + transport: parsed.transport, + connector: createMcpConnector(connector), + // Nobody is present to answer an unattended probe. + elicit: () => Effect.succeed({ action: "decline" as const }), + }); + return classifyMcpHealthProbe(raw, { + identityField: spec.identityField, + secrets: Object.values(credential.values).filter( + (value): value is string => typeof value === "string", ), - ); + }); }).pipe( - // buildConnectorInput rejects (e.g. stdio disabled / missing config). - Effect.catchTag("McpConnectionError", (error) => - Effect.succeed({ - status: mcpLivenessFailureStatus(error), - checkedAt: Date.now(), - ...(error.httpStatus !== undefined ? { httpStatus: error.httpStatus } : {}), - detail: error.message, - reason: mcpLivenessFailureReason(error), - } satisfies HealthCheckResult), - ), + // Discovery fails only with McpToolDiscoveryError; the probe call fails + // with the invoke errors. buildConnectorInput's McpConnectionError is + // already absorbed by Effect.option, so each tag has exactly one origin. + Effect.catchTags({ + McpToolDiscoveryError: (error) => + Effect.succeed( + classifyMcpHealthFailure({ + stage: error.stage, + httpStatus: error.httpStatus, + timedOut: error.timedOut, + reauthorizationRequired: error.reauthorizationRequired, + insufficientScope: error.insufficientScope, + authRefusedWithoutStatus: authRefusedWithoutStatus(error.message), + }), + ), + McpInvocationError: (error) => + Effect.succeed( + classifyMcpHealthFailure({ + stage: "call_tool", + httpStatus: error.status, + insufficientScope: error.insufficientScope, + }), + ), + // The probe dials its own session, whose handshake can fail too. + McpConnectionError: (error) => + Effect.succeed( + classifyMcpHealthFailure({ + stage: "connect", + httpStatus: error.httpStatus, + timedOut: error.failureKind === "timeout", + insufficientScope: error.insufficientScope, + }), + ), + McpOAuthReauthorizationRequired: () => + Effect.succeed( + classifyMcpHealthFailure({ stage: "connect", reauthorizationRequired: true }), + ), + }), // Every failure above folds onto the SUCCESS channel, so without this // the span ends green on an expired credential — a 401 auth wall and a // healthy dial were indistinguishable in traces. diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 90558590e6..c1c7906539 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -5,6 +5,7 @@ import { OAuthClientSlug, ProviderItemId, ProviderKey, + type HealthCheckCandidate, type OAuthProbeResult, type Owner, } from "@executor-js/sdk/shared"; @@ -24,6 +25,7 @@ import { oauthIdentityLabelFromHealth, runAutomaticOAuthConnect, runCimdConnect, + seedHealthCheckCandidate, typedIdentityLabel, uniqueConnectionName, } from "./add-account-modal"; @@ -260,6 +262,41 @@ describe("oauthIdentityLabelFromHealth", () => { }); }); +describe("seedHealthCheckCandidate", () => { + const candidate = ( + operation: string, + overrides: Partial = {}, + ): HealthCheckCandidate => ({ + operation, + method: "get", + requiredArgCount: 0, + destructive: false, + ...overrides, + }); + + it("prefers a zero-argument read, then any non-destructive candidate", () => { + expect( + seedHealthCheckCandidate([ + candidate("delete", { destructive: true }), + candidate("search", { requiredArgCount: 1 }), + candidate("me"), + ])?.operation, + ).toBe("me"); + expect( + seedHealthCheckCandidate([ + candidate("delete", { destructive: true }), + candidate("search", { requiredArgCount: 1 }), + ])?.operation, + ).toBe("search"); + }); + + it("never seeds a candidate that must be picked explicitly", () => { + const tool = candidate("whoami", { method: "mcp", explicitSelectionOnly: true }); + expect(seedHealthCheckCandidate([tool])).toBeNull(); + expect(seedHealthCheckCandidate([tool, candidate("me")])?.operation).toBe("me"); + }); +}); + describe("DEFAULT_CONNECTION_OWNER", () => { // The 'saved to' owner defaults to Personal: a connection is most often a // personal credential. diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 222dd0f682..ea36b7c7ab 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -89,6 +89,8 @@ import { import { RemoveOAuthAppDialog } from "./remove-oauth-app-dialog"; import { AddCustomMethodForm, type CreateCustomMethod } from "./add-custom-method-modal"; import { CredentialGuidancePanel } from "./credential-guidance"; +import { JsonArgumentsField } from "./health-check-editor"; +import { parseJsonArguments } from "../lib/health-check-args"; import { PlacementLine, type AuthMethod } from "../lib/auth-placements"; import { connectionIdentifier } from "../lib/connection-name"; import { Badge } from "./badge"; @@ -629,6 +631,20 @@ export const connectionExistsMessage = (label: string): string => * explicit choice. Personal: a connection is most often a personal credential. */ export const DEFAULT_CONNECTION_OWNER: Owner = "user"; +/** The candidate the key check starts from: the best one that runs without + * arguments, else the best non-destructive one. Continue may run the seed and + * save it as the integration's check, so a candidate that must be picked + * explicitly (an MCP tool, whose read-only claim is only a hint) is never + * seeded. */ +export const seedHealthCheckCandidate = ( + candidates: readonly HealthCheckCandidate[], +): HealthCheckCandidate | null => { + const seedable = candidates.filter( + (candidate) => !candidate.destructive && candidate.explicitSelectionOnly !== true, + ); + return seedable.find((candidate) => candidate.requiredArgCount === 0) ?? seedable[0] ?? null; +}; + /** Prefer OAuth only when its picker has a matching, usable client. Otherwise * prefer a credential method; OAuth-only integrations still expose setup. */ export const preferredMethodId = ( @@ -1291,6 +1307,8 @@ function RequestCheckPanel(props: { readonly configuredOperation: string | null; readonly args: Record; readonly onArgChange: (name: string, value: string) => void; + readonly jsonArgs: string; + readonly onJsonArgsChange: (text: string) => void; readonly ready: boolean; readonly validating: boolean; readonly result: HealthCheckResult | null; @@ -1308,7 +1326,9 @@ function RequestCheckPanel(props: { })), [candidates], ); - const requiredParams = (selected?.parameters ?? []).filter((p) => p.required); + const requiredParams = selected?.jsonArguments + ? [] + : (selected?.parameters ?? []).filter((p) => p.required); const healthy = result?.status === "healthy"; // Identity-looking fields first, then response order; capped so a chatty // response can't blow the modal up (the hidden tail adds nothing, since the @@ -1358,6 +1378,18 @@ function RequestCheckPanel(props: { + {!configuredOperation && selected?.jsonArguments ? ( +
+ +
+ ) : null} + {/* Required args, only when the chosen call needs them. */} {!configuredOperation && requiredParams.length > 0 ? (
@@ -1481,6 +1513,7 @@ function AddAccountModalView(props: AddAccountModalProps) { const [validationResult, setValidationResult] = useState(null); const [hcOperation, setHcOperation] = useState(""); const [hcArgs, setHcArgs] = useState>({}); + const [hcJsonArgs, setHcJsonArgs] = useState(""); // Credential methods run as a TWO-STEP wizard in the same modal: step 1 is // the key + proving it works (the request/response panel sits right below // the key, which stays visible and editable the whole time); step 2 is @@ -1699,12 +1732,7 @@ function AddAccountModalView(props: AddAccountModalProps) { // the seed is simply the best one that can run without arguments, falling // back to the best non-destructive one. const hcBestCandidate = useMemo( - () => - healthCheckCandidates.find( - (candidate) => !candidate.destructive && candidate.requiredArgCount === 0, - ) ?? - healthCheckCandidates.find((candidate) => !candidate.destructive) ?? - null, + () => seedHealthCheckCandidate(healthCheckCandidates), [healthCheckCandidates], ); useEffect(() => { @@ -1731,13 +1759,25 @@ function AddAccountModalView(props: AddAccountModalProps) { })); }, [validationResult]); const hcRequiredParams = (hcSelected?.parameters ?? []).filter((p) => p.required); - const hcMissingRequired = hcRequiredParams.some( - (p) => (hcArgs[p.name] ?? "").trim().length === 0, - ); + // Trimmed text per HTTP parameter, blanks dropped; a tool's arguments as one + // JSON object. + const hcParsedArgs = hcSelected?.jsonArguments + ? parseJsonArguments(hcJsonArgs) + : { + ok: true as const, + args: Object.fromEntries( + Object.entries(hcArgs) + .map(([key, value]) => [key, value.trim()] as const) + .filter(([, value]) => value.length > 0), + ), + }; const canCheckKey = !isOAuth && !isNoAuth && (hasHealthCheck || healthCheckCandidates.length > 0); - // Pick mode's probe is runnable once an operation is chosen and its required - // args are filled. - const hcCandidateReady = hcOperation.length > 0 && !hcMissingRequired; + // Pick mode's probe is runnable once an operation is chosen and its + // arguments are valid, required ones included. + const hcCandidateReady = + hcOperation.length > 0 && + hcParsedArgs.ok && + hcRequiredParams.every((p) => Object.hasOwn(hcParsedArgs.args, p.name)); // The distinct credential inputs the selected method needs — one per variable // across its placements. A single-input method yields one field (`token`); a @@ -2171,13 +2211,10 @@ function AddAccountModalView(props: AddAccountModalProps) { }; const candidateHealthSpec = (): HealthCheckSpec | null => { - if (!hcCandidateReady) return null; - const argEntries = Object.entries(hcArgs) - .map(([key, value]) => [key, value.trim()] as const) - .filter(([, value]) => value.length > 0); + if (!hcCandidateReady || !hcParsedArgs.ok) return null; return { operation: hcOperation, - ...(argEntries.length > 0 ? { args: Object.fromEntries(argEntries) } : {}), + ...(Object.keys(hcParsedArgs.args).length > 0 ? { args: hcParsedArgs.args } : {}), }; }; @@ -3221,6 +3258,7 @@ function AddAccountModalView(props: AddAccountModalProps) { onOperationChange={(next) => { setHcOperation(next); setHcArgs({}); + setHcJsonArgs(""); clearKeyCheck(); }} configuredOperation={configuredHealthCheck?.operation ?? null} @@ -3229,6 +3267,11 @@ function AddAccountModalView(props: AddAccountModalProps) { setHcArgs((prev) => ({ ...prev, [name]: value })); clearKeyCheck(); }} + jsonArgs={hcJsonArgs} + onJsonArgsChange={(text) => { + setHcJsonArgs(text); + clearKeyCheck(); + }} ready={ credentialPayloadOrigin !== null && !validating && diff --git a/packages/react/src/components/health-check-editor.test.tsx b/packages/react/src/components/health-check-editor.test.tsx new file mode 100644 index 0000000000..5f45af3a3e --- /dev/null +++ b/packages/react/src/components/health-check-editor.test.tsx @@ -0,0 +1,83 @@ +import { describe, expect, it } from "@effect/vitest"; +import type { HealthCheckCandidate } from "@executor-js/sdk/shared"; +import { renderToStaticMarkup } from "react-dom/server"; + +import { HealthCheckConfigFields, initialHealthCheckOperation } from "./health-check-editor"; + +const candidate = ( + operation: string, + overrides: Partial = {}, +): HealthCheckCandidate => ({ + operation, + method: "get", + requiredArgCount: 0, + destructive: false, + ...overrides, +}); + +describe("initialHealthCheckOperation", () => { + it("never preselects a candidate that must be picked explicitly", () => { + const tool = candidate("whoami", { method: "mcp", explicitSelectionOnly: true }); + expect(initialHealthCheckOperation(null, [tool])).toBe(""); + expect(initialHealthCheckOperation(null, [tool, candidate("me")])).toBe("me"); + }); + + it("keeps the saved operation and the first HTTP candidate default", () => { + const tool = candidate("whoami", { method: "mcp", explicitSelectionOnly: true }); + expect(initialHealthCheckOperation({ operation: "whoami" }, [tool])).toBe("whoami"); + expect(initialHealthCheckOperation({ operation: "gone" }, [])).toBe("gone"); + expect(initialHealthCheckOperation(null, [candidate("first"), candidate("second")])).toBe( + "first", + ); + expect(initialHealthCheckOperation(null, [])).toBe(""); + }); +}); + +describe("HealthCheckConfigFields arguments", () => { + const render = (selected: HealthCheckCandidate, jsonArgs = "") => + renderToStaticMarkup( + undefined} + identityField="" + onIdentityFieldChange={() => undefined} + args={{}} + onArgChange={() => undefined} + jsonArgs={jsonArgs} + onJsonArgsChange={() => undefined} + idPrefix="hc" + />, + ); + + it("edits a tool's arguments as one JSON object, flagging a non-object", () => { + const tool = candidate("search", { + method: "mcp", + jsonArguments: true, + requiredArgCount: 1, + parameters: [ + { name: "query", location: "argument", required: true }, + { name: "limit", location: "argument", required: false }, + ], + }); + const markup = render(tool, '{"query": "a"}'); + expect(markup.match(/