From 55a8b5eaea88c20fa5c5f1852262db613b8ddb9f Mon Sep 17 00:00:00 2001 From: smrht <62819456+smrht@users.noreply.github.com> Date: Tue, 15 Sep 2026 02:52:38 +0200 Subject: [PATCH 01/25] fix(openapi): fetch analyticsdata Discovery from the service host (#1955) * fix(openapi): fetch analyticsdata Discovery from the service host The central Discovery directory does not list the GA4 Data API, so https://www.googleapis.com/discovery/v1/apis/analyticsdata/v1beta/rest answers 404 and importing the source fails. Route analyticsdata to its own host the way forms, keep and photospicker already are. * Test queue timeout with a controlled clock * Verify Analytics Data discovery import end to end --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/great-hoops-repeat.md | 9 +++ .../src/mcp/session-build-semaphore.test.ts | 11 +++- .../google-analyticsdata-discovery.test.ts | 57 +++++++++++++++++++ .../src/providers/google/discovery.test.ts | 10 ++++ .../openapi/src/providers/google/discovery.ts | 1 + 5 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 .changeset/great-hoops-repeat.md create mode 100644 e2e/scenarios/google-analyticsdata-discovery.test.ts diff --git a/.changeset/great-hoops-repeat.md b/.changeset/great-hoops-repeat.md new file mode 100644 index 0000000000..9943a92068 --- /dev/null +++ b/.changeset/great-hoops-repeat.md @@ -0,0 +1,9 @@ +--- +"@executor-js/plugin-openapi": patch +--- + +Fetch Google Analytics Data (`analyticsdata`) Discovery from the service's own +host. The central directory does not list the GA4 Data API, so the canonical +`https://www.googleapis.com/discovery/v1/apis/analyticsdata/v1beta/rest` answers +404 and the source fails to import. Same treatment `forms`, `keep` and +`photospicker` already get. diff --git a/apps/cloud/src/mcp/session-build-semaphore.test.ts b/apps/cloud/src/mcp/session-build-semaphore.test.ts index 3d4ad76343..584b65ee0e 100644 --- a/apps/cloud/src/mcp/session-build-semaphore.test.ts +++ b/apps/cloud/src/mcp/session-build-semaphore.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach } from "@effect/vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "@effect/vitest"; import { acquireBuildSlot, @@ -13,6 +13,10 @@ describe("session-build-semaphore", () => { resetBuildSlotsForTest(); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("grants up to the cap immediately, with no wait", async () => { const results = await Promise.all([ acquireBuildSlot().promise, @@ -214,6 +218,7 @@ describe("session-build-semaphore", () => { }); it("proceeds without a slot when the queue wait exceeds the timeout, and does not count it as active", async () => { + vi.useFakeTimers(); await Promise.all([ acquireBuildSlot().promise, acquireBuildSlot().promise, @@ -223,6 +228,10 @@ describe("session-build-semaphore", () => { expect(currentActiveBuildsForTest()).toBe(4); const timedOutHandle = acquireBuildSlot(10); + await vi.advanceTimersByTimeAsync(9); + expect(currentQueueLengthForTest()).toBe(1); + expect(currentActiveBuildsForTest()).toBe(4); + await vi.advanceTimersByTimeAsync(1); const result = await timedOutHandle.promise; expect(result).toEqual({ acquired: false, waitMs: expect.any(Number), timedOut: true }); diff --git a/e2e/scenarios/google-analyticsdata-discovery.test.ts b/e2e/scenarios/google-analyticsdata-discovery.test.ts new file mode 100644 index 0000000000..5a18a94822 --- /dev/null +++ b/e2e/scenarios/google-analyticsdata-discovery.test.ts @@ -0,0 +1,57 @@ +import { randomBytes, randomUUID } from "node:crypto"; +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + ProviderItemId, +} from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +scenario( + "Google Analytics Data · public Discovery import exposes report tools", + { timeout: 120_000 }, + Effect.gen(function* () { + const target = yield* Target; + const { client } = yield* Api; + const identity = yield* target.newIdentity(); + const apiClient = yield* client(api, identity); + const slug = IntegrationSlug.make(`analytics_${randomBytes(4).toString("hex")}`); + // Fetch Google's public document through the real server import path. + // No Analytics account or authenticated report request is needed. + const added = yield* apiClient.openapi.addSpec({ + payload: { + slug, + spec: { + kind: "url", + url: "https://www.googleapis.com/discovery/v1/apis/analyticsdata/v1beta/rest", + }, + }, + }); + yield* Effect.gen(function* () { + expect(added.toolCount).toBeGreaterThan(0); + const providers = yield* apiClient.providers.list(); + const provider = providers[0]; + if (provider === undefined) return yield* Effect.die("No credential provider available"); + yield* apiClient.connections.create({ + payload: { + owner: "org", + name: ConnectionName.make("main"), + integration: slug, + template: AuthTemplateSlug.make("googleOAuth2"), + from: { provider, id: ProviderItemId.make(randomUUID()) }, + }, + }); + const tools = yield* apiClient.tools.list({ query: { integration: slug } }); + expect(tools.some((tool) => tool.name.endsWith("runReport"))).toBe(true); + expect(tools.some((tool) => tool.name.endsWith("runRealtimeReport"))).toBe(true); + }).pipe(Effect.ensuring(apiClient.openapi.removeSpec({ params: { slug } }).pipe(Effect.orDie))); + }), +); diff --git a/packages/plugins/openapi/src/providers/google/discovery.test.ts b/packages/plugins/openapi/src/providers/google/discovery.test.ts index 748f8bc917..a6a6f18702 100644 --- a/packages/plugins/openapi/src/providers/google/discovery.test.ts +++ b/packages/plugins/openapi/src/providers/google/discovery.test.ts @@ -159,6 +159,11 @@ it("accepts only supported HTTPS Google Discovery endpoints", () => { "https://www.googleapis.com/discovery/v1/apis/photospicker/v1/rest", ), ).toBe("https://photospicker.googleapis.com/$discovery/rest?version=v1"); + expect( + normalizeGoogleDiscoveryUrl( + "https://www.googleapis.com/discovery/v1/apis/analyticsdata/v1beta/rest", + ), + ).toBe("https://analyticsdata.googleapis.com/$discovery/rest?version=v1beta"); expect( normalizeGoogleDiscoveryUrl("https://www.googleapis.com/discovery/v1/apis/forms/v1/rest"), ).toBe("https://forms.googleapis.com/$discovery/rest?version=v1"); @@ -171,6 +176,11 @@ it("accepts only supported HTTPS Google Discovery endpoints", () => { expect( normalizeGoogleDiscoveryUrl("https://photospicker.googleapis.com/$discovery/rest?version=v1"), ).toBe("https://photospicker.googleapis.com/$discovery/rest?version=v1"); + expect( + normalizeGoogleDiscoveryUrl( + "https://analyticsdata.googleapis.com/$discovery/rest?version=v1beta", + ), + ).toBe("https://analyticsdata.googleapis.com/$discovery/rest?version=v1beta"); expect( normalizeGoogleDiscoveryUrl("https://forms.googleapis.com/$discovery/rest?version=v1"), ).toBe("https://forms.googleapis.com/$discovery/rest?version=v1"); diff --git a/packages/plugins/openapi/src/providers/google/discovery.ts b/packages/plugins/openapi/src/providers/google/discovery.ts index 9d79076986..8d346561c3 100644 --- a/packages/plugins/openapi/src/providers/google/discovery.ts +++ b/packages/plugins/openapi/src/providers/google/discovery.ts @@ -41,6 +41,7 @@ type GoogleDiscoveryServiceOverride = { }; const GOOGLE_DISCOVERY_SERVICE_OVERRIDES: Record = { + analyticsdata: { preserveServiceHostedUrl: true }, forms: { preserveServiceHostedUrl: true }, keep: { preserveServiceHostedUrl: true }, [GOOGLE_PHOTOS_PICKER_SERVICE]: { From d873caf6fb3aa7408270b42aaad77f53cf9ec090 Mon Sep 17 00:00:00 2001 From: yingchao Date: Tue, 15 Sep 2026 08:52:42 +0800 Subject: [PATCH 02/25] Advertise refresh-token grant in OAuth client metadata (#1974) * Advertise refresh-token grant in OAuth client metadata * Test OAuth metadata through connection and tool use * Test queue timeout with a controlled clock --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/cimd-refresh-token.md | 10 +++ e2e/selfhost/mcp-oauth-cimd-connect.test.ts | 70 +++++++++++++++++-- .../src/server/oauth-client-metadata.test.ts | 2 + .../api/src/server/oauth-client-metadata.ts | 10 ++- 4 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 .changeset/cimd-refresh-token.md diff --git a/.changeset/cimd-refresh-token.md b/.changeset/cimd-refresh-token.md new file mode 100644 index 0000000000..22408ee8d1 --- /dev/null +++ b/.changeset/cimd-refresh-token.md @@ -0,0 +1,10 @@ +--- +"@executor-js/api": patch +--- + +Advertise refresh-token support in OAuth client ID metadata documents. + +OAuth providers may reject the `offline_access` scope when the client's +metadata declares only the authorization-code grant. Hosted and local client +metadata now declare both `authorization_code` and `refresh_token`, matching +Executor's dynamic client registration behavior. diff --git a/e2e/selfhost/mcp-oauth-cimd-connect.test.ts b/e2e/selfhost/mcp-oauth-cimd-connect.test.ts index 94f9d5fa1c..dfb6827121 100644 --- a/e2e/selfhost/mcp-oauth-cimd-connect.test.ts +++ b/e2e/selfhost/mcp-oauth-cimd-connect.test.ts @@ -20,7 +20,7 @@ import { visit } from "../src/surfaces/browser"; const api = composePluginApi([mcpHttpPlugin()] as const); scenario( - "MCP OAuth · advertised CIMD starts authorization without dynamic registration", + "MCP OAuth · CIMD advertises refresh support and completes connection without dynamic registration", { timeout: 180_000 }, Effect.scoped( Effect.gen(function* () { @@ -30,7 +30,7 @@ scenario( const oauth = yield* OAuthTestServer; const server = yield* serveMcpServerWithOAuth( () => makeGreetingMcpServer({ name: "cimd-connect-mcp" }), - { path: "/mcp" }, + { path: "/mcp", scopes: ["read", "offline_access"] }, ); const identity = yield* target.newIdentity(); const client = yield* makeApiClient(api, identity); @@ -69,17 +69,68 @@ scenario( authorize, "the popup reached the discovered authorization endpoint", ).toBeDefined(); - const clientId = authorize?.query["client_id"]; - createdClientId = clientId; + expect( + (authorize?.query["scope"] ?? "").split(" "), + "authorization requests the resource's offline access scope", + ).toContain("offline_access"); + const clientId = authorize?.query["client_id"] ?? ""; + createdClientId = clientId || undefined; expect( clientId, "authorization uses Executor's metadata document as client_id", ).toMatch(/^https?:\/\/[^/]+\/api\/oauth\/client-id-metadata\/.+\.json$/); - await popup.close(); + const metadataResponse = await page.request.get(clientId); + expect(metadataResponse.status(), "the client metadata document is reachable").toBe( + 200, + ); + expect( + await metadataResponse.json(), + "the client declares the grant required by offline_access", + ).toMatchObject({ + grant_types: ["authorization_code", "refresh_token"], + }); + expect(authorize).toBeDefined(); + // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- test boundary: authorization must exist before completing the flow + if (authorize === undefined) throw new Error("Missing authorization request"); + const completed = await Effect.runPromise( + oauth.completeAuthorizationCodeFlow({ authorizationUrl: authorize.url }), + ); + await popup.goto(completed.callbackUrl); + await page + .getByRole("heading", { name: /Add connection/ }) + .waitFor({ state: "hidden" }); + await popup.close().catch(() => undefined); }); }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "the OAuth callback saved the connection").toHaveLength(1); + const tools = yield* client.tools.list({ query: { integration: slug } }); + expect( + tools.some((tool) => tool.name === "simple_echo"), + "authenticated discovery finds the upstream tool", + ).toBe(true); + + const invoked = yield* client.executions.execute({ + payload: { + code: `return await ${tools[0]?.address}({});`, + autoApprove: true, + }, + }); + expect(invoked.status).toBe("completed"); + expect(invoked.text, "the connected tool runs through authenticated MCP").toContain( + "mcp-ok", + ); + const requests = yield* oauth.requests; + expect( + requests.some( + (request) => + request.path === "/token" && + new URLSearchParams(request.body).get("grant_type") === "authorization_code", + ), + "the callback exchanged the code using the advertised client", + ).toBe(true); expect( requests.filter((request) => request.method === "POST" && request.path === "/register"), "CIMD wins when the server also advertises DCR", @@ -105,5 +156,12 @@ scenario( ), ); }), - ).pipe(Effect.provide(OAuthTestServer.layer({ clientIdMetadataDocumentSupported: true }))), + ).pipe( + Effect.provide( + OAuthTestServer.layer({ + clientIdMetadataDocumentSupported: true, + scopes: ["read", "offline_access"], + }), + ), + ), ); diff --git a/packages/core/api/src/server/oauth-client-metadata.test.ts b/packages/core/api/src/server/oauth-client-metadata.test.ts index fd730dabe7..384c0e3187 100644 --- a/packages/core/api/src/server/oauth-client-metadata.test.ts +++ b/packages/core/api/src/server/oauth-client-metadata.test.ts @@ -19,6 +19,7 @@ describe("OAuth client ID metadata document", () => { "http://100.81.219.45:42384/api/oauth/client-id-metadata/acme.json", ); expect(metadata.redirect_uris).toEqual(["http://100.81.219.45:42384/api/oauth/callback"]); + expect(metadata.grant_types).toEqual(["authorization_code", "refresh_token"]); expect(metadata.token_endpoint_auth_method).toBe("none"); expect(metadata.application_type).toBe("web"); }); @@ -63,6 +64,7 @@ describe("OAuth client ID metadata document", () => { "http://localhost/api/oauth/callback", "http://[::1]/api/oauth/callback", ]); + expect(metadata.grant_types).toEqual(["authorization_code", "refresh_token"]); expect(metadata.application_type).toBe("native"); }); diff --git a/packages/core/api/src/server/oauth-client-metadata.ts b/packages/core/api/src/server/oauth-client-metadata.ts index c597745d59..d2e30d729c 100644 --- a/packages/core/api/src/server/oauth-client-metadata.ts +++ b/packages/core/api/src/server/oauth-client-metadata.ts @@ -9,6 +9,10 @@ export const OAUTH_CLIENT_ID_METADATA_DOCUMENT_TARGET_PATH_PREFIX = export const OAUTH_CLIENT_ID_METADATA_DOCUMENT_DEFAULT_TARGET = "default" as const; export const OAUTH_CLIENT_ID_METADATA_DOCUMENT_LOCAL_TARGET = "local" as const; +// Keep CIMD aligned with DCR: providers may reject `offline_access` unless the +// client declares that it can use the refresh-token grant. +const OAUTH_CLIENT_GRANT_TYPES = ["authorization_code", "refresh_token"] as const; + type MetadataTarget = | typeof OAUTH_CLIENT_ID_METADATA_DOCUMENT_DEFAULT_TARGET | typeof OAUTH_CLIENT_ID_METADATA_DOCUMENT_LOCAL_TARGET @@ -19,7 +23,7 @@ interface OAuthClientIdMetadataDocument { readonly client_name: string; readonly client_uri: string; readonly redirect_uris: readonly string[]; - readonly grant_types: readonly ["authorization_code"]; + readonly grant_types: typeof OAUTH_CLIENT_GRANT_TYPES; readonly response_types: readonly ["code"]; readonly token_endpoint_auth_method: "none"; readonly application_type: "web" | "native"; @@ -129,7 +133,7 @@ export const oauthClientIdMetadataDocumentFromRequest = ({ client_name: "Executor Local", client_uri: url.origin, redirect_uris: localLoopbackRedirectUris(mountPrefix), - grant_types: ["authorization_code"], + grant_types: OAUTH_CLIENT_GRANT_TYPES, response_types: ["code"], token_endpoint_auth_method: "none", application_type: "native", @@ -150,7 +154,7 @@ export const oauthClientIdMetadataDocumentFromRequest = ({ client_name: "Executor", client_uri: url.origin, redirect_uris: [redirectUri.toString()], - grant_types: ["authorization_code"], + grant_types: OAUTH_CLIENT_GRANT_TYPES, response_types: ["code"], token_endpoint_auth_method: "none", application_type: "web", From 61f71c56fe799b6e0faa2b2f82a91f631bc6a979 Mon Sep 17 00:00:00 2001 From: Aditya kumar singh <143548997+Adityakk9031@users.noreply.github.com> Date: Tue, 15 Sep 2026 06:22:46 +0530 Subject: [PATCH 03/25] fix(mcp): shut down scoped executor on session eviction (#1917) (#1971) * fix(mcp): shut down scoped executor on session eviction (#1917) Retain the scoped Executor created for each MCP session and shut it down when an in-memory MCP session is evicted, closed, or fails eager initialization. This ensures child tool subprocesses and connection pools owned by the session are properly cleaned up. * test(cloud): fix flaky timer precision assertion in session build semaphore test * Verify MCP session disposal closes upstream resources * Test queue timeout with a controlled clock --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/mcp-session-executor-leak.md | 7 ++ .../mcp-session-resource-cleanup.test.ts | 85 +++++++++++++++++++ packages/core/api/src/server/mcp-build.ts | 2 +- .../mcp/src/in-memory-session-store.test.ts | 85 +++++++++++++++++++ .../hosts/mcp/src/in-memory-session-store.ts | 37 ++++++-- 5 files changed, 210 insertions(+), 6 deletions(-) create mode 100644 .changeset/mcp-session-executor-leak.md create mode 100644 e2e/selfhost/mcp-session-resource-cleanup.test.ts diff --git a/.changeset/mcp-session-executor-leak.md b/.changeset/mcp-session-executor-leak.md new file mode 100644 index 0000000000..e8419ec356 --- /dev/null +++ b/.changeset/mcp-session-executor-leak.md @@ -0,0 +1,7 @@ +--- +"@executor-js/host-mcp": patch +"@executor-js/api": patch +"executor": patch +--- + +Shut down scoped executors and tool subprocess resources upon MCP session eviction and disposal in the in-process session store. diff --git a/e2e/selfhost/mcp-session-resource-cleanup.test.ts b/e2e/selfhost/mcp-session-resource-cleanup.test.ts new file mode 100644 index 0000000000..9056388480 --- /dev/null +++ b/e2e/selfhost/mcp-session-resource-cleanup.test.ts @@ -0,0 +1,85 @@ +import { randomBytes } from "node:crypto"; +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServer } from "@executor-js/plugin-mcp/testing"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +scenario( + "MCP · deleting a host session closes its upstream connection", + { timeout: 120_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const apiClient = yield* makeClient(api, identity); + const slug = IntegrationSlug.make(`session_cleanup_${randomBytes(4).toString("hex")}`); + const upstream = yield* serveMcpServer(makeGreetingMcpServer); + yield* apiClient.mcp.addServer({ + payload: { + transport: "remote", + name: "Session cleanup", + endpoint: upstream.url, + slug, + remoteTransport: "streamable-http", + }, + }); + yield* Effect.gen(function* () { + yield* apiClient.connections.create({ + payload: { + owner: "org", + name: ConnectionName.make("main"), + integration: slug, + template: AuthTemplateSlug.make("none"), + value: "", + }, + }); + yield* Effect.promise(() => expect.poll(upstream.inFlightRequests).toBe(0)); + const transport = new StreamableHTTPClientTransport(new URL(target.mcpUrl), { + requestInit: { headers: identity.headers }, + }); + const client = yield* Effect.acquireRelease( + Effect.promise(async () => { + const connected = new Client({ name: "session-cleanup-e2e", version: "1" }); + await connected.connect(transport); + return connected; + }), + (connected) => Effect.promise(() => connected.close()), + ); + const result = yield* Effect.promise(() => + client.callTool({ + name: "execute", + arguments: { code: `return await tools.${slug}.org.main.simple_echo({});` }, + }), + ); + expect(result.isError).toBeFalsy(); + expect(JSON.stringify(result.content)).toContain("mcp-ok"); + yield* Effect.promise(() => + expect + .poll(upstream.inFlightRequests, { + message: "the tool leaves its upstream event stream open for reuse", + }) + .toBeGreaterThan(0), + ); + yield* Effect.promise(() => transport.terminateSession()); + yield* Effect.promise(() => + expect + .poll(upstream.inFlightRequests, { + timeout: 10_000, + message: "closing the host session releases its upstream event stream", + }) + .toBe(0), + ); + }).pipe(Effect.ensuring(apiClient.mcp.removeServer({ params: { slug } }).pipe(Effect.orDie))); + }), + ), +); diff --git a/packages/core/api/src/server/mcp-build.ts b/packages/core/api/src/server/mcp-build.ts index 2512436557..299aa853ec 100644 --- a/packages/core/api/src/server/mcp-build.ts +++ b/packages/core/api/src/server/mcp-build.ts @@ -88,7 +88,7 @@ export const makeMcpBuildServer = ...(options ?? {}), }).pipe( Effect.withSpan("mcp.server.create"), - Effect.map((mcpServer) => ({ mcpServer, engine })), + Effect.map((mcpServer) => ({ mcpServer, engine, executor })), ), ), ); diff --git a/packages/hosts/mcp/src/in-memory-session-store.test.ts b/packages/hosts/mcp/src/in-memory-session-store.test.ts index 9bfa914488..bdc51db85d 100644 --- a/packages/hosts/mcp/src/in-memory-session-store.test.ts +++ b/packages/hosts/mcp/src/in-memory-session-store.test.ts @@ -691,4 +691,89 @@ describe("pre-initialize dispatch through the in-memory session store", () => { expect(response.status).toBe(406); await sessions.close(); }); + + it("shuts down the scoped executor and custom closer when an idle session is evicted", async () => { + let executorClosed = 0; + let customClosed = 0; + const realExecutor = await Effect.runPromise(createExecutor(makeTestConfig())); + const testExecutor = { + ...realExecutor, + close: () => + realExecutor.close().pipe( + Effect.tap(() => + Effect.sync(() => { + executorClosed += 1; + }), + ), + ), + }; + const engine = makeIdleTestEngine(); + const sessions = makeInMemoryMcpSessionStore( + () => + createExecutorMcpServer({ engine }).pipe( + Effect.map((mcpServer) => ({ + mcpServer, + engine, + executor: testExecutor, + close: () => { + customClosed += 1; + return Promise.resolve(); + }, + })), + ), + { sessionIdleTtlMs: IDLE_TTL_MS }, + ); + + // oxlint-disable-next-line executor/no-try-catch-or-throw -- test boundary: always close the store + try { + await openSession(sessions); + expect(sessions.sessionCount()).toBe(1); + expect(executorClosed).toBe(0); + expect(customClosed).toBe(0); + + // Advance clock past idle window and sweep. + expect(await sessions.sweepIdleSessions(Date.now() + IDLE_TTL_MS + 1)).toBe(1); + expect(sessions.sessionCount()).toBe(0); + expect(executorClosed).toBe(1); + expect(customClosed).toBe(1); + } finally { + await sessions.close(); + } + }); + + it("shuts down the scoped executor when sessions.close() is called", async () => { + let executorClosed = 0; + const realExecutor = await Effect.runPromise(createExecutor(makeTestConfig())); + const testExecutor = { + ...realExecutor, + close: () => + realExecutor.close().pipe( + Effect.tap(() => + Effect.sync(() => { + executorClosed += 1; + }), + ), + ), + }; + const engine = makeIdleTestEngine(); + const sessions = makeInMemoryMcpSessionStore( + () => + createExecutorMcpServer({ engine }).pipe( + Effect.map((mcpServer) => ({ + mcpServer, + engine, + executor: testExecutor, + })), + ), + { sessionIdleTtlMs: IDLE_TTL_MS }, + ); + + await openSession(sessions); + expect(sessions.sessionCount()).toBe(1); + expect(executorClosed).toBe(0); + + await sessions.close(); + expect(executorClosed).toBe(1); + expect(sessions.sessionCount()).toBe(0); + }); }); diff --git a/packages/hosts/mcp/src/in-memory-session-store.ts b/packages/hosts/mcp/src/in-memory-session-store.ts index af9e149741..47747e1a24 100644 --- a/packages/hosts/mcp/src/in-memory-session-store.ts +++ b/packages/hosts/mcp/src/in-memory-session-store.ts @@ -3,7 +3,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { WebStandardStreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js"; import { formatPausedExecution, type ExecutionEngine } from "@executor-js/execution"; -import type { OrgWriteAccess } from "@executor-js/sdk"; +import type { Executor, OrgWriteAccess } from "@executor-js/sdk"; import { buildResumeApprovalUrl, @@ -98,6 +98,8 @@ export class McpEngineBuildError extends Data.TaggedError("McpEngineBuildError") export interface BuiltMcpServer { readonly mcpServer: McpServer; readonly engine: ExecutionEngine; + readonly executor?: Executor; + readonly close?: () => Promise; } /** The browser-mode wiring the store hands a build call when a session opts in. */ @@ -243,6 +245,8 @@ export const makeInMemoryMcpSessionStore = ( const servers = new Map(); const owners = new Map(); const engines = new Map>(); + const executors = new Map(); + const closers = new Map Promise>(); const approvals: InProcessBrowserApprovalStore = makeInProcessBrowserApprovalStore(); // Monotonic-ish last-touch stamp per live session, the first input the idle // sweep reads. Written on create and on every forwarded request. @@ -294,20 +298,33 @@ export const makeInMemoryMcpSessionStore = ( ): Promise => ignoreClose(id, "engine", engine ? () => Effect.runPromise(engine.shutdown) : undefined); + /** + * Shut down a session's scoped executor and its plugin resources (such as + * tool subprocesses and connection pools). Every disposal path goes through here. + */ + const shutdownExecutor = (id: string | null, executor: Executor | undefined): Promise => + ignoreClose(id, "executor", executor ? () => Effect.runPromise(executor.close()) : undefined); + const dispose = async (id: string, opts: { transport?: boolean; server?: boolean } = {}) => { const transport = transports.get(id); const server = servers.get(id); const engine = engines.get(id); + const executor = executors.get(id); + const closer = closers.get(id); transports.delete(id); servers.delete(id); owners.delete(id); engines.delete(id); + executors.delete(id); + closers.delete(id); lastSeen.delete(id); activeRequests.delete(id); if (opts.transport) await ignoreClose(id, "transport", transport ? () => transport.close() : undefined); if (opts.server) await ignoreClose(id, "server", server ? () => server.close() : undefined); await shutdownEngine(id, engine); + await shutdownExecutor(id, executor); + await ignoreClose(id, "session", closer); }; /** @@ -430,7 +447,7 @@ export const makeInMemoryMcpSessionStore = ( ...buildOptionsFor(request, () => createdSessionId), resource, }).pipe( - Effect.flatMap(({ mcpServer, engine }) => + Effect.flatMap(({ mcpServer, engine, executor, close }) => Effect.gen(function* () { const transport = new WebStandardStreamableHTTPServerTransport({ sessionIdGenerator: () => crypto.randomUUID(), @@ -441,6 +458,8 @@ export const makeInMemoryMcpSessionStore = ( servers.set(sid, mcpServer); owners.set(sid, { principal, resource }); engines.set(sid, engine); + if (executor) executors.set(sid, executor); + if (close) closers.set(sid, close); lastSeen.set(sid, Date.now()); }, onsessionclosed: (sid) => void dispose(sid, { server: true }), @@ -458,11 +477,13 @@ export const makeInMemoryMcpSessionStore = ( orgWriteAccessForPrincipal(principal), () => { // Nothing was ever registered under a session id, so `dispose` has - // no entry to work from — release the three handles by hand, engine - // included. + // no entry to work from — release the handles by hand, engine + // and executor included. void ignoreClose(null, "transport", () => transport.close()); void ignoreClose(null, "server", () => mcpServer.close()); void shutdownEngine(null, engine); + void shutdownExecutor(null, executor); + if (close) void ignoreClose(null, "session", close); }, ); }), @@ -623,7 +644,13 @@ export const makeInMemoryMcpSessionStore = ( sweepIdleSessions, close: async () => { if (sweepTimer !== undefined) clearInterval(sweepTimer); - const ids = new Set([...transports.keys(), ...servers.keys(), ...engines.keys()]); + const ids = new Set([ + ...transports.keys(), + ...servers.keys(), + ...engines.keys(), + ...executors.keys(), + ...closers.keys(), + ]); await Promise.all([...ids].map((id) => dispose(id, { transport: true, server: true }))); }, }; From 513426cd48deee53b336badfea051f57e630d66e Mon Sep 17 00:00:00 2001 From: Saatvik Arya Date: Tue, 15 Sep 2026 06:22:50 +0530 Subject: [PATCH 04/25] fix(mcp): bound discovery connection teardown (#1878) * fix(mcp): bound discovery connection teardown Discovery cleanup runs in an interruption-masked finalizer. Bound an unresponsive transport close so a completed tool listing cannot strand health checks or other callers indefinitely. * test(mcp): match current discovery client contract * test(mcp): avoid wall-clock teardown assertion * Exercise discovery teardown with real MCP connections --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .../mcp/src/sdk/discover-close.test.ts | 68 +++++++++++++++++++ packages/plugins/mcp/src/sdk/discover.ts | 24 ++++--- 2 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 packages/plugins/mcp/src/sdk/discover-close.test.ts diff --git a/packages/plugins/mcp/src/sdk/discover-close.test.ts b/packages/plugins/mcp/src/sdk/discover-close.test.ts new file mode 100644 index 0000000000..89f8e78e37 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/discover-close.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { createMcpConnector, type McpConnector } from "./connection"; +import { discoverTools } from "./discover"; +import { makeEchoMcpServer, serveMcpServer } from "../testing"; + +// Exercise the real MCP handshake and catalog. The connector owns teardown, +// so a wrapper can reproduce a transport that closes its sockets but never +// settles its close promise without replacing the protocol client. +const hangingCloseConnector = (connector: McpConnector, state: { closes: number }): McpConnector => + Effect.map(connector, (connection) => ({ + client: connection.client, + close: async () => { + state.closes += 1; + await connection.close(); + return new Promise(() => {}); + }, + })); + +describe("MCP discovery teardown", () => { + it.live("preserves a real catalog when close never settles", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveMcpServer(() => makeEchoMcpServer({ name: "hanging-close" })); + const state = { closes: 0 }; + const manifest = yield* discoverTools( + hangingCloseConnector( + createMcpConnector({ + transport: "remote", + endpoint: server.url, + remoteTransport: "streamable-http", + }), + state, + ), + ); + expect(state.closes).toBe(1); + expect(manifest.server?.name).toBe("hanging-close"); + expect(manifest.tools.length).toBeGreaterThan(0); + }), + ), + ); + + it.live("preserves listing failure when close never settles", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveMcpServer(() => makeEchoMcpServer()); + yield* server.rejectSessionMethod("tools/list", 403); + const state = { closes: 0 }; + const result = yield* discoverTools( + hangingCloseConnector( + createMcpConnector({ + transport: "remote", + endpoint: server.url, + remoteTransport: "streamable-http", + }), + state, + ), + ).pipe(Effect.result); + expect(state.closes).toBe(1); + expect(result).toMatchObject({ + _tag: "Failure", + failure: { stage: "list_tools", httpStatus: 403 }, + }); + }), + ), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index 754333eb35..d3c672361d 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -31,6 +31,12 @@ const MAX_LIST_TOOLS_PAGES = 100; // shape probe's single unauth POST. const DEFAULT_DISCOVER_TIMEOUT = Duration.seconds(15); +// Teardown is best-effort and paid for by the request that performed discovery. +// A remote transport may accept close and then never settle, so use the same +// bound as the invocation connection pool instead of stranding the caller in an +// uninterruptible finalizer after discovery itself has already completed. +const CLOSE_TIMEOUT = Duration.seconds(2); + // --------------------------------------------------------------------------- // Public API // --------------------------------------------------------------------------- @@ -243,13 +249,11 @@ export const discoverTools = ( const closeConnection = (connection: { readonly close: () => Promise; }): Effect.Effect => - Effect.ignore( - Effect.tryPromise({ - try: () => connection.close(), - catch: () => - new McpToolDiscoveryError({ - stage: "list_tools", - message: "Failed closing MCP connection", - }), - }), - ); + Effect.tryPromise({ + try: () => connection.close(), + catch: () => + new McpToolDiscoveryError({ + stage: "list_tools", + message: "Failed closing MCP connection", + }), + }).pipe(Effect.timeout(CLOSE_TIMEOUT), Effect.ignore); From 905e0972614aed5a3bb279b51dc060f87f892d75 Mon Sep 17 00:00:00 2001 From: ra-co88 Date: Tue, 15 Sep 2026 01:52:53 +0100 Subject: [PATCH 05/25] fix(react): honor prefers-reduced-motion in the shared stylesheet (#1892) * fix(react): honor prefers-reduced-motion in the shared stylesheet * Test queue timeout with a controlled clock * Cover reduced motion changes in the browser --------- Co-authored-by: pt-act <211776491+pt-act@users.noreply.github.com> Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/reduced-motion.md | 10 +++++ e2e/scenarios/reduced-motion.test.ts | 59 +++++++++++++++++++++++++++ packages/react/src/styles/globals.css | 21 ++++++++++ 3 files changed, 90 insertions(+) create mode 100644 .changeset/reduced-motion.md create mode 100644 e2e/scenarios/reduced-motion.test.ts diff --git a/.changeset/reduced-motion.md b/.changeset/reduced-motion.md new file mode 100644 index 0000000000..0ce1a65ad0 --- /dev/null +++ b/.changeset/reduced-motion.md @@ -0,0 +1,10 @@ +--- +"@executor-js/react": patch +--- + +fix: honor prefers-reduced-motion in the shared stylesheet + +Adds a `prefers-reduced-motion: reduce` block to the global stylesheet that +caps transition/animation durations to 0.01ms and disables smooth scrolling, +so motion-sensitive users get a stable UI. The loading spinner renders +statically under reduced motion (its meaning is preserved via `role="status"`). diff --git a/e2e/scenarios/reduced-motion.test.ts b/e2e/scenarios/reduced-motion.test.ts new file mode 100644 index 0000000000..f2e751421e --- /dev/null +++ b/e2e/scenarios/reduced-motion.test.ts @@ -0,0 +1,59 @@ +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; + +import { scenario } from "../src/scenario"; +import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +scenario( + "Accessibility · reduced motion removes dialog animation and control transitions", + {}, + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const identity = yield* target.newIdentity(); + + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the API key dialog with normal motion", async () => { + await page.emulateMedia({ reducedMotion: "no-preference" }); + await visit(page, "/api-keys"); + await page.getByRole("button", { name: "New key" }).click(); + await page.getByRole("dialog").waitFor(); + const duration = await page + .getByRole("dialog") + .evaluate((element) => Number.parseFloat(getComputedStyle(element).transitionDuration)); + expect(duration, "normal motion retains the dialog transition").toBeGreaterThan(0.00001); + }); + + await step("Enable reduced motion while the dialog is open", async () => { + await page.emulateMedia({ reducedMotion: "reduce" }); + const styles = await page.getByRole("dialog").evaluate((element) => { + const style = getComputedStyle(element); + return { + animation: Number.parseFloat(style.animationDuration), + iterations: style.animationIterationCount, + transition: Number.parseFloat(style.transitionDuration), + scroll: style.scrollBehavior, + }; + }); + expect(styles).toEqual({ + animation: 0.00001, + iterations: "1", + transition: 0.00001, + scroll: "auto", + }); + await page.locator("#create-key-name").fill("Reduced motion check"); + expect(await page.locator("#create-key-name").inputValue()).toBe("Reduced motion check"); + }); + + await step("Restore normal motion without losing the form", async () => { + await page.emulateMedia({ reducedMotion: "no-preference" }); + expect(await page.locator("#create-key-name").inputValue()).toBe("Reduced motion check"); + const duration = await page + .getByRole("dialog") + .evaluate((element) => Number.parseFloat(getComputedStyle(element).transitionDuration)); + expect(duration).toBeGreaterThan(0.00001); + }); + }); + }), +); diff --git a/packages/react/src/styles/globals.css b/packages/react/src/styles/globals.css index e82bae6ca7..97977327b8 100644 --- a/packages/react/src/styles/globals.css +++ b/packages/react/src/styles/globals.css @@ -319,3 +319,24 @@ opacity: 0.25; } } + +/* --------------------------------------------------------------------------- + * Reduced motion — WCAG 2.2 (motion-sensitive users). + * + * Neutralizes transitions/animations/scroll-behavior when the OS requests + * reduced motion, per the standard modern-CSS-reset pattern. The spinner + * (ios-spinner-fade) is capped to a single 0.01ms iteration — effectively + * static — which is the standard behavior: its meaning is preserved by + * `role="status"` (announced by assistive tech), and an opacity pulse would + * be a non-motion affordance but is not required for reduced-motion users. + * ------------------------------------------------------------------------- */ +@media (prefers-reduced-motion: reduce) { + *, + *::before, + *::after { + animation-duration: 0.01ms !important; + animation-iteration-count: 1 !important; + transition-duration: 0.01ms !important; + scroll-behavior: auto !important; + } +} From de13821e4efd3f571ae7f41d46f74d761fae8988 Mon Sep 17 00:00:00 2001 From: yingchao Date: Tue, 15 Sep 2026 08:59:51 +0800 Subject: [PATCH 06/25] Keep artifact deletion optimistic across navigation (#1901) * Keep artifact deletion optimistic across navigation * Restore gallery-card delete coverage and bound the Saved artifacts wait in the artifacts e2e * Test queue timeout with a controlled clock --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/artifact-delete-navigation.md | 5 ++ e2e/scenarios/artifacts.test.ts | 86 ++++++++++++++++++-- packages/react/src/pages/artifact-detail.tsx | 15 +++- 3 files changed, 98 insertions(+), 8 deletions(-) create mode 100644 .changeset/artifact-delete-navigation.md diff --git a/.changeset/artifact-delete-navigation.md b/.changeset/artifact-delete-navigation.md new file mode 100644 index 0000000000..aa9ba333ad --- /dev/null +++ b/.changeset/artifact-delete-navigation.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Prevent deleted artifacts from briefly reappearing after returning to the artifact gallery. diff --git a/e2e/scenarios/artifacts.test.ts b/e2e/scenarios/artifacts.test.ts index fa9a82d03b..f020b1d21c 100644 --- a/e2e/scenarios/artifacts.test.ts +++ b/e2e/scenarios/artifacts.test.ts @@ -582,8 +582,10 @@ scenario( const suffix = uniqueSuffix(); const originalTitle = `Draft Dashboard ${suffix}`; const renamedTitle = `Quarterly Dashboard ${suffix}`; + const listDeleteTitle = `List Card ${suffix}`; let artifactId: ArtifactId | undefined; + let listArtifactId: ArtifactId | undefined; yield* Effect.gen(function* () { const rendered = yield* session.call("create-artifact", { @@ -633,26 +635,88 @@ scenario( originalTitle, ); + // A second artifact for the gallery card's own delete path: the detail + // page steps below consume the renamed one, and the card's hover → + // Delete affordance is a separate surface the console must keep working. + const listed = yield* session.call("create-artifact", { + code: artifactSource(`list-delete-${suffix}`), + title: listDeleteTitle, + description: "A dashboard the user will delete from the gallery", + }); + expect(listed.ok, `create-artifact succeeded: ${listed.text}`).toBe(true); + listArtifactId = structuredOf(listed).artifactId as ArtifactId; + expect(listArtifactId, "the second artifact was persisted").toBeTruthy(); + yield* browser.session(identity, async ({ page, step }) => { - await step("Delete the artifact from the list", async () => { + await step("Delete the artifact from its gallery card", async () => { await visit(page, "/artifacts"); + // Card actions reveal on hover; the card is the link's enclosing tile. const card = page.locator('[data-slot="artifact-card"]').filter({ - hasText: renamedTitle, + hasText: listDeleteTitle, }); await card.waitFor({ timeout: 20_000 }); await card.hover(); await card.getByRole("button", { name: "Delete" }).click(); const confirm = page.getByRole("alertdialog"); - await confirm.getByRole("heading", { name: `Delete ${renamedTitle}?` }).waitFor(); + await confirm.getByRole("heading", { name: `Delete ${listDeleteTitle}?` }).waitFor(); await confirm.getByRole("button", { name: "Delete Artifact" }).click(); await confirm.waitFor({ state: "hidden", timeout: 20_000 }); + await page + .getByRole("link", { name: `Open artifact ${listDeleteTitle}` }) + .waitFor({ state: "detached", timeout: 20_000 }); }); - await step("The artifact is gone from the list", async () => { - await page + let releaseListRefresh = () => {}; + let markListRefreshStarted = () => {}; + const listRefreshGate = new Promise((resolve) => { + releaseListRefresh = resolve; + }); + const listRefreshStarted = new Promise((resolve) => { + markListRefreshStarted = resolve; + }); + + await step("Open the artifact and delete it from its detail page", async () => { + await visit(page, "/artifacts"); + await page.getByRole("link", { name: `Open artifact ${renamedTitle}` }).click(); + await page.getByRole("heading", { name: renamedTitle }).waitFor({ timeout: 20_000 }); + + // Hold the post-delete list refresh open. The redirected gallery must + // carry the optimistic removal across the route handoff rather than + // relying on a fast canonical response to hide a stale-cache flash. + await page.route("**/artifacts", async (route) => { + if (route.request().method() !== "GET") { + await route.continue(); + return; + } + markListRefreshStarted(); + await listRefreshGate; + await route.continue(); + }); + + await page.getByRole("button", { name: "Delete" }).click(); + const confirm = page.getByRole("alertdialog"); + await confirm.getByRole("heading", { name: `Delete ${renamedTitle}?` }).waitFor(); + await confirm.getByRole("button", { name: "Delete Artifact" }).click(); + }); + + await step("The redirected gallery already omits the deleted artifact", async () => { + await page.waitForURL((url) => /\/artifacts\/?$/.test(url.pathname), { + timeout: 20_000, + }); + await page.getByRole("heading", { name: "Saved artifacts" }).waitFor({ timeout: 20_000 }); + await listRefreshStarted; + + const deletedCardCount = await page .getByRole("link", { name: `Open artifact ${renamedTitle}` }) - .waitFor({ state: "detached", timeout: 20_000 }); + .count(); + releaseListRefresh(); + await page.unrouteAll({ behavior: "wait" }); + + expect( + deletedCardCount, + "the optimistic delete survives navigation while the list refresh is pending", + ).toBe(0); }); }); @@ -660,12 +724,22 @@ scenario( expect(afterDelete.text, "the agent no longer offers the deleted artifact").not.toContain( renamedTitle, ); + expect(afterDelete.text, "nor the artifact deleted from its gallery card").not.toContain( + listDeleteTitle, + ); const missing = yield* session.call("show-artifact", { id: artifactId }); expect(missing.ok, "fetching a deleted artifact is an error, not an empty render").toBe( false, ); }).pipe( + Effect.ensuring( + Effect.suspend(() => + listArtifactId === undefined + ? Effect.void + : client.artifacts.remove({ params: { artifactId: listArtifactId } }), + ).pipe(Effect.ignore), + ), Effect.ensuring( Effect.suspend(() => artifactId === undefined diff --git a/packages/react/src/pages/artifact-detail.tsx b/packages/react/src/pages/artifact-detail.tsx index c782474e04..7ce115a8a1 100644 --- a/packages/react/src/pages/artifact-detail.tsx +++ b/packages/react/src/pages/artifact-detail.tsx @@ -1,5 +1,5 @@ import { Suspense, useCallback, useEffect, useMemo, useState } from "react"; -import { useAtomRefresh, useAtomSet, useAtomValue } from "@effect/atom-react"; +import { useAtomMount, useAtomRefresh, useAtomSet, useAtomValue } from "@effect/atom-react"; import { ClientOnly, useNavigate } from "@tanstack/react-router"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; import * as Exit from "effect/Exit"; @@ -8,7 +8,12 @@ import type { ArtifactId } from "@executor-js/sdk/shared"; import { trackEvent } from "../api/analytics"; import { useArtifactRenderer, usePreloadArtifactRenderer } from "../api/artifact-renderer"; -import { artifactAtom, removeArtifactOptimistic, renameArtifactOptimistic } from "../api/atoms"; +import { + artifactAtom, + artifactsOptimisticAtom, + removeArtifactOptimistic, + renameArtifactOptimistic, +} from "../api/atoms"; import { artifactWriteKeys } from "../api/reactivity-keys"; import { createHttpShellHost } from "../api/shell-host"; import { @@ -38,6 +43,12 @@ import { RenameArtifactDialog } from "./artifact-rename-dialog"; export function ArtifactDetailPage(props: { readonly artifactId: ArtifactId }) { const artifact = useAtomValue(artifactAtom(props.artifactId)); const refresh = useAtomRefresh(artifactAtom(props.artifactId)); + + // Detail-page writes reduce over the gallery's optimistic surface. Keep that + // surface mounted here so its rename/delete transition survives the route + // handoff back to the gallery instead of exposing the list query's cached + // pre-mutation value while its authoritative refresh is still in flight. + useAtomMount(artifactsOptimisticAtom); const doRename = useAtomSet(renameArtifactOptimistic, { mode: "promiseExit" }); const doRemove = useAtomSet(removeArtifactOptimistic, { mode: "promiseExit" }); const navigate = useNavigate(); From d64639b1a50d2d292235aff8f727ca11fe9e43a6 Mon Sep 17 00:00:00 2001 From: yingchao Date: Tue, 15 Sep 2026 08:59:55 +0800 Subject: [PATCH 07/25] Surface unreachable OpenAPI upstreams as network errors (#1960) * Surface unreachable upstreams as network error * Test queue timeout with a controlled clock * Classify transport failures without hiding invocation defects * Name the integration and origin in the unreachable-upstream message Executor sends the request, not the user's browser, so "check your network" pointed at the wrong place. Name the integration and the origin that could not be reached and tell the user to verify the base URL and that the service is online. Only the host is lifted off the transport failure; the path, query, and headers stay out of the message. * Record the cause of unreachable upstreams without the request Classifying transport failures as a typed tool result took them off the hosts' defect path, which was the only place the cause was logged. Lift the errno-style code (ECONNREFUSED, ENOTFOUND, UND_ERR_SOCKET, ...) off the fetch cause chain, log a warning and annotate the span with the integration, host, and code, and return the same sanitized pair in the tool result details. The raw TransportError stays out of details on purpose: it carries the whole request, including resolved auth headers. --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/openapi-transport-unreachable.md | 6 + .../openapi-unreachable-artifact.test.ts | 226 ++++++++++++++++++ packages/plugins/openapi/src/sdk/backing.ts | 39 ++- packages/plugins/openapi/src/sdk/errors.ts | 14 +- packages/plugins/openapi/src/sdk/invoke.ts | 52 +++- .../openapi/src/sdk/upstream-failures.test.ts | 175 +++++++++++++- 6 files changed, 498 insertions(+), 14 deletions(-) create mode 100644 .changeset/openapi-transport-unreachable.md create mode 100644 e2e/scenarios/openapi-unreachable-artifact.test.ts diff --git a/.changeset/openapi-transport-unreachable.md b/.changeset/openapi-transport-unreachable.md new file mode 100644 index 0000000000..05623ff515 --- /dev/null +++ b/.changeset/openapi-transport-unreachable.md @@ -0,0 +1,6 @@ +--- +"executor": patch +"@executor-js/plugin-openapi": patch +--- + +OpenAPI tools that cannot reach the upstream server now return an `upstream_unreachable` error instead of `Internal tool error [id]`. The message names the integration and origin that could not be reached, `details` carries the sanitized `host` and errno-style `code` (`ECONNREFUSED`, `ENOTFOUND`, …), and the failure is logged with the same classification. diff --git a/e2e/scenarios/openapi-unreachable-artifact.test.ts b/e2e/scenarios/openapi-unreachable-artifact.test.ts new file mode 100644 index 0000000000..1b5adae46e --- /dev/null +++ b/e2e/scenarios/openapi-unreachable-artifact.test.ts @@ -0,0 +1,226 @@ +// Cross-target: an artifact whose OpenAPI query cannot reach its upstream gets +// an actionable network error, not the opaque defect mask. This walks the real +// path from a saved artifact through the nested shell, execute-action, sandbox, +// OpenAPI transport, and back into ArtifactError. +import { randomBytes } from "node:crypto"; +import { createServer } from "node:http"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import type { Page } from "playwright"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { ConnectionName, IntegrationSlug, type ArtifactId } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Mcp, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; +import type { McpSession } from "../src/surfaces/mcp"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`; + +type DroppingUpstream = { + readonly url: string; + readonly requests: () => number; + readonly close: () => void; +}; + +// Accept the request, then drop the socket before sending response headers. +// This produces a real transport failure without relying on a hardcoded or +// temporarily-unused port. +const serveDroppingUpstream = () => + Effect.acquireRelease( + Effect.callback((resume) => { + let hits = 0; + const server = createServer((_request, response) => { + hits += 1; + response.destroy(); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}`, + requests: () => hits, + close: () => { + server.close(); + server.closeAllConnections(); + }, + }), + ); + }); + }), + (server) => Effect.sync(server.close), + ); + +const unreachableSpec = (baseUrl: string): string => + JSON.stringify({ + openapi: "3.0.3", + info: { title: "Unreachable API", version: "1.0.0" }, + servers: [{ url: baseUrl }], + paths: { + "/things": { + get: { + tags: ["things"], + operationId: "listThings", + summary: "List things", + responses: { + "200": { + description: "Things", + content: { + "application/json": { + schema: { type: "array", items: { type: "object" } }, + }, + }, + }, + }, + }, + }, + }, + }); + +const createConnectionCode = (slug: string) => ` +const created = await tools.executor.coreTools.connections.create({ + owner: "org", + name: "public", + integration: ${JSON.stringify(slug)}, + template: "none", +}); +return JSON.stringify(created.ok ? { ok: true } : { ok: false, error: created.error }); +`; + +const executeApproved = (session: McpSession, code: string) => + Effect.gen(function* () { + let result = yield* session.call("execute", { code }); + let guard = 0; + while (result.text.includes("executionId:") && guard < 10) { + result = yield* session.approvePaused(result.text); + guard += 1; + } + expect(result.ok, `execute completed (got: ${result.text.slice(0, 400)})`).toBe(true); + return result.text; + }); + +const artifactSource = (slug: string) => ` +function App() { + const query = useQuery(tools.${slug}.things.listThings.queryOptions({})); + const result = query.data; + return ( +
+

Upstream status

+
+ {query.isLoading ? ( + + ) : query.error ? ( + + ) : result?.ok === false ? ( + + ) : ( +

Unexpected upstream success

+ )} +
+
+ ); +} +`; + +const structuredOf = (result: { readonly raw: unknown }): Record => + ((result.raw as { structuredContent?: Record }).structuredContent ?? + {}) as Record; + +const artifactContent = (page: Page) => + page.frameLocator('[data-testid="artifact-shell-frame"]').frameLocator("iframe"); + +scenario( + "Artifacts · an unreachable OpenAPI host shows actionable retry guidance instead of an internal error", + { timeout: 180_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const mcp = yield* Mcp; + const { client: makeClient } = yield* Api; + + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const session = mcp.session(identity); + const upstream = yield* serveDroppingUpstream(); + const slug = unique("unreachable"); + const title = `Unreachable upstream ${randomBytes(4).toString("hex")}`; + let artifactId: ArtifactId | undefined; + + yield* Effect.ensuring( + Effect.gen(function* () { + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: unreachableSpec(upstream.url) }, + slug, + baseUrl: upstream.url, + }, + }); + + const created = yield* executeApproved(session, createConnectionCode(slug)); + expect(created, `the no-auth connection was created: ${created}`).toContain('"ok":true'); + + const rendered = yield* session.call("create-artifact", { + code: artifactSource(slug), + title, + description: "Shows whether the upstream API is reachable", + connections: { [slug]: `${slug}.org.public` }, + }); + expect(rendered.ok, `create-artifact succeeded: ${rendered.text}`).toBe(true); + + const structured = structuredOf(rendered); + artifactId = structured.artifactId as ArtifactId; + expect(artifactId, "the artifact was persisted").toBeTruthy(); + + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the artifact that reads from the unreachable API", async () => { + await visit(page, String(structured.url)); + await page.getByRole("heading", { name: title }).waitFor({ timeout: 20_000 }); + }); + + await step( + "The artifact explains that the upstream host could not be reached", + async () => { + const state = artifactContent(page).getByTestId("upstream-state"); + await state.locator('[data-slot="artifact-error"]').waitFor({ timeout: 30_000 }); + const message = await state.innerText(); + + expect(message, "the user gets actionable network guidance").toContain( + `Could not reach the upstream server for "${slug}"`, + ); + expect(message, "the opaque defect mask never reaches the artifact").not.toContain( + "Internal tool error", + ); + expect(message, "the request path is not leaked").not.toContain("/things"); + }, + ); + }); + + expect(upstream.requests(), "the artifact made a real upstream request").toBeGreaterThan( + 0, + ); + }), + Effect.gen(function* () { + if (artifactId !== undefined) { + yield* client.artifacts.remove({ params: { artifactId } }).pipe(Effect.ignore); + } + yield* client.connections + .remove({ + params: { + owner: "org", + integration: IntegrationSlug.make(slug), + name: ConnectionName.make("public"), + }, + }) + .pipe(Effect.ignore); + yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); + }), + ); + }), + ), +); diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index b7a54d4f43..58427c5f54 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -629,6 +629,22 @@ export const resolveOpenApiBackedTools = ({ }; }); +// Transport failures used to escape as defects, which the hosts log with a +// correlation id. As a typed tool failure nothing else records them, so log +// and annotate the span with the sanitized classification operators need to +// tell DNS from refused from TLS. +const recordUpstreamUnreachable = (integration: string, error: OpenApiInvocationError) => { + const annotations = { + "plugin.openapi.integration": integration, + "plugin.openapi.upstream.host": error.upstreamHost ?? "unknown", + "plugin.openapi.upstream.transport_code": error.transportCode ?? "unknown", + }; + return Effect.logWarning("OpenAPI upstream unreachable").pipe( + Effect.annotateLogs(annotations), + Effect.andThen(Effect.annotateCurrentSpan(annotations)), + ); +}; + export const invokeOpenApiBackedTool = (input: { readonly ctx: PluginCtx; readonly toolRow: { readonly integration: string; readonly name: string }; @@ -727,7 +743,28 @@ export const invokeOpenApiBackedTool = (input: { details: error.cause ?? error, }), }) - : Effect.fail(error), + : error.reason === "transport_error" + ? recordUpstreamUnreachable(integration, error).pipe( + Effect.as({ + ok: false as const, + failure: ToolResult.fail({ + code: "upstream_unreachable", + // Executor sends the request, not the user's browser, so + // point at what the user can act on: the configured + // origin and the service behind it. + message: `Could not reach the upstream server for "${integration}"${error.upstreamHost ? ` at ${error.upstreamHost}` : ""}. Verify the integration's base URL and that the service is online, then try again.`, + // Unlike the timeout branches, `error.cause` is withheld: + // the TransportError carries the whole request, including + // resolved auth headers. Absent fields are dropped, not + // `undefined`: the result must stay a JSON value. + details: { + ...(error.upstreamHost !== undefined ? { host: error.upstreamHost } : {}), + ...(error.transportCode !== undefined ? { code: error.transportCode } : {}), + }, + }), + }), + ) + : Effect.fail(error), ), ); diff --git a/packages/plugins/openapi/src/sdk/errors.ts b/packages/plugins/openapi/src/sdk/errors.ts index 6a5fc4a8cc..a13a2aa29e 100644 --- a/packages/plugins/openapi/src/sdk/errors.ts +++ b/packages/plugins/openapi/src/sdk/errors.ts @@ -39,7 +39,19 @@ export class OpenApiSpecOverrideError extends Schema.TaggedErrorClass; - readonly reason?: "response_headers_timeout" | "response_body_timeout" | "unknown_arguments"; + readonly reason?: + | "response_headers_timeout" + | "response_body_timeout" + | "unknown_arguments" + | "transport_error"; + // `host[:port]` of a request that failed at the transport layer. It is the + // integration's configured origin, so it is safe to show; the path, query, + // and headers stay on `cause`. + readonly upstreamHost?: string | undefined; + // Errno-style code behind a transport failure (`ECONNREFUSED`, `ENOTFOUND`, + // `UND_ERR_SOCKET`, …) when the runtime exposes one. Tells DNS from refused + // from TLS without exposing the request. + readonly transportCode?: string | undefined; readonly cause?: unknown; }> {} diff --git a/packages/plugins/openapi/src/sdk/invoke.ts b/packages/plugins/openapi/src/sdk/invoke.ts index 2f9bd0f6f5..8d180e6952 100644 --- a/packages/plugins/openapi/src/sdk/invoke.ts +++ b/packages/plugins/openapi/src/sdk/invoke.ts @@ -1,5 +1,10 @@ -import { Effect, Exit, Fiber, Layer, Option, Schema, Stream } from "effect"; -import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http"; +import { Effect, Exit, Fiber, Layer, Option, Predicate, Schema, Stream } from "effect"; +import { + HttpClient, + type HttpClientError, + HttpClientRequest, + HttpClientResponse, +} from "effect/unstable/http"; import { isToolFile, type ToolFileValue } from "@executor-js/sdk/core"; import { OpenApiInvocationError } from "./errors"; @@ -1148,6 +1153,44 @@ export const buildRequest = Effect.fn("OpenApi.buildRequest")(function* ( return request; }); +// --------------------------------------------------------------------------- +// Transport failure classification +// --------------------------------------------------------------------------- + +const urlHost = Option.liftThrowable((url: string) => new URL(url).host); + +// `fetch` rejects with a generic `TypeError("fetch failed")`; the errno-style +// code (`ECONNREFUSED`, `ENOTFOUND`, `UND_ERR_SOCKET`, …) sits on the innermost +// link of its `cause` chain. The walk is bounded so a cyclic cause cannot spin. +const TransportCauseLink = Schema.Struct({ + code: Schema.optional(Schema.String), + cause: Schema.optional(Schema.Unknown), +}); +const decodeTransportCauseLink = Schema.decodeUnknownOption(TransportCauseLink); +const TRANSPORT_CAUSE_MAX_DEPTH = 5; + +const transportFailureCode = (cause: unknown, depth = 0): string | undefined => + Option.match(decodeTransportCauseLink(cause), { + onNone: () => undefined, + onSome: (link) => + (link.cause !== undefined && depth < TRANSPORT_CAUSE_MAX_DEPTH + ? transportFailureCode(link.cause, depth + 1) + : undefined) ?? link.code, + }); + +// A transport failure produced no response: DNS, connection refused, TLS, or a +// socket dropped before headers. The TransportError carries the whole request +// (URL, headers, credentials), so only the origin and the errno-style code are +// lifted onto the invocation error. +const transportFailureFields = (reason: HttpClientError.HttpClientError["reason"]) => + Predicate.isTagged(reason, "TransportError") + ? { + reason: "transport_error" as const, + upstreamHost: Option.getOrUndefined(urlHost(reason.request.url)), + transportCode: transportFailureCode(reason.cause), + } + : {}; + // --------------------------------------------------------------------------- // Public API — invoke a single operation // --------------------------------------------------------------------------- @@ -1183,6 +1226,7 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( (err) => new OpenApiInvocationError({ message: "HTTP request failed", + ...transportFailureFields(err.reason), statusCode: Option.none(), cause: err, }), @@ -1197,7 +1241,6 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( }), ), ); - const fiber = runFork(responseEffect); const interrupt = () => { runFork(Fiber.interrupt(fiber)); }; @@ -1207,6 +1250,7 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( interrupt(); resume(Effect.succeed(Option.none())); }, responseHeadersTimeoutMs); + const fiber = runFork(responseEffect); signal.addEventListener("abort", interrupt, { once: true }); return Effect.sync(() => { clearTimeout(timer); @@ -1257,7 +1301,6 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( }), ), ); - const fiber = runFork(bodyEffect); const interrupt = () => { runFork(Fiber.interrupt(fiber)); }; @@ -1267,6 +1310,7 @@ export const invoke = Effect.fn("OpenApi.invoke")(function* ( interrupt(); resume(Effect.succeed(Option.none())); }, responseBodyTimeoutMs); + const fiber = runFork(bodyEffect); signal.addEventListener("abort", interrupt, { once: true }); return Effect.sync(() => { clearTimeout(timer); diff --git a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts index 2089e2fdf0..1f52b30c50 100644 --- a/packages/plugins/openapi/src/sdk/upstream-failures.test.ts +++ b/packages/plugins/openapi/src/sdk/upstream-failures.test.ts @@ -11,8 +11,14 @@ // --------------------------------------------------------------------------- import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Schema } from "effect"; -import { FetchHttpClient, HttpServerRequest, HttpServerResponse } from "effect/unstable/http"; +import { Cause, Data, Effect, Exit, Layer, Logger, References, Schema } from "effect"; +import { + FetchHttpClient, + HttpClient, + HttpClientError, + HttpServerRequest, + HttpServerResponse, +} from "effect/unstable/http"; import { HttpApi, HttpApiBuilder, @@ -43,8 +49,10 @@ import { import { openApiPlugin } from "./plugin"; -const testPlugins = () => - [openApiPlugin({ httpClientLayer: FetchHttpClient.layer }), memoryCredentialsPlugin()] as const; +class AdapterDefect extends Data.TaggedError("AdapterDefect") {} + +const testPlugins = (httpClientLayer = FetchHttpClient.layer) => + [openApiPlugin({ httpClientLayer }), memoryCredentialsPlugin()] as const; // `/things` GET op `listThings` under group "things" → tool path // `things.listThings`, used verbatim (dots and all) as the address tool segment. @@ -102,6 +110,30 @@ const startDroppingServer = () => (s) => Effect.sync(() => s.close()), ); +// Bind an ephemeral port, then release it so nothing listens there and the +// kernel refuses the connection (`ECONNREFUSED`). Port 1 is not equivalent: +// `fetch` rejects it as a bad port before dialing, with no errno. +const refusedBaseUrl = () => + Effect.callback((resume) => { + const server = createServer(); + server.listen(0, "127.0.0.1", () => { + const port = (server.address() as AddressInfo).port; + server.close(() => resume(Effect.succeed(`http://127.0.0.1:${port}`))); + }); + }); + +type CapturedLog = { readonly message: string; readonly annotations: Record }; + +const capturingLogger = (sink: Array) => + Logger.layer([ + Logger.make((options) => { + sink.push({ + message: String(options.message), + annotations: options.fiber.getRef(References.CurrentLogAnnotations), + }); + }), + ]); + const ThingsGroup = HttpApiGroup.make("things").add( HttpApiEndpoint.get("listThings", "/things", { success: Schema.Array(Schema.Record(Schema.String, Schema.Unknown)), @@ -114,9 +146,11 @@ const FailureApi = HttpApi.make("failuresTest") // Build an executor + connection from the FailureApi HttpApi against an // arbitrary baseUrl (used for the Node-transport socket-drop / slow cases). -const buildExecutor = (baseUrl: string) => +const buildExecutor = (baseUrl: string, httpClientLayer = FetchHttpClient.layer) => Effect.gen(function* () { - const executor = yield* createExecutor(makeTestConfig({ plugins: testPlugins() })); + const executor = yield* createExecutor( + makeTestConfig({ plugins: testPlugins(httpClientLayer) }), + ); yield* executor.openapi.addSpec( makeOpenApiHttpApiTestIntegrationConfig(FailureApi, { slug: "f", baseUrl }), ); @@ -394,9 +428,12 @@ describe("OpenAPI upstream failure modes", () => { const { baseUrl } = yield* startDroppingServer(); const { executor, address } = yield* buildExecutor(baseUrl); - const exit = yield* executor.execute(address, {}).pipe(Effect.exit); + const result = yield* executor.execute(address, {}); - expect(Exit.isFailure(exit)).toBe(true); + expect(result).toMatchObject({ + ok: false, + error: { code: "upstream_unreachable", details: { code: expect.any(String) } }, + }); }), ); @@ -446,4 +483,126 @@ describe("OpenAPI upstream failure modes", () => { expect(result.data).toEqual([]); }), ); + + it.effect("request encoding failures remain invocation failures", () => + Effect.gen(function* () { + const httpClientLayer = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.fail( + new HttpClientError.HttpClientError({ + reason: new HttpClientError.EncodeError({ request, cause: new AdapterDefect() }), + }), + ), + ), + ); + const { executor, address } = yield* buildExecutor( + "https://upstream.example", + httpClientLayer, + ); + const exit = yield* executor.execute(address, {}).pipe(Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + }), + ); + + it.effect("transport defects remain defects", () => + Effect.gen(function* () { + const defect = new AdapterDefect(); + const httpClientLayer = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make(() => Effect.die(defect)), + ); + const { executor, address } = yield* buildExecutor( + "https://upstream.example", + httpClientLayer, + ); + const exit = yield* executor.execute(address, {}).pipe(Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + expect(Exit.match(exit, { onFailure: Cause.hasDies, onSuccess: () => false })).toBe(true); + }), + ); + + it.effect("interrupted transport remains interrupted", () => + Effect.gen(function* () { + const httpClientLayer = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make(() => Effect.interrupt), + ); + const { executor, address } = yield* buildExecutor( + "https://upstream.example", + httpClientLayer, + ); + const exit = yield* executor.execute(address, {}).pipe(Effect.exit); + expect(Exit.isFailure(exit)).toBe(true); + expect(Exit.match(exit, { onFailure: Cause.hasInterrupts, onSuccess: () => false })).toBe( + true, + ); + }), + ); + + // Port 1 refuses immediately. The same path used to throw `Internal tool + // error [hex]` because the raw HttpClientError carries the request URL. + // Executor makes the request, so the message names the integration and + // origin the user can fix instead of blaming their own network. + it.effect("connection refused names the integration and origin without the path", () => + Effect.gen(function* () { + const { executor, address } = yield* buildExecutor("http://127.0.0.1:1"); + + const result = yield* executor.execute(address, {}); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "upstream_unreachable", + message: expect.stringContaining( + 'Could not reach the upstream server for "f" at 127.0.0.1:1.', + ), + }, + }); + const failure = result as { + readonly ok: false; + readonly error: { readonly message: string; readonly details?: unknown }; + }; + // No errno here (`fetch` rejects port 1 before dialing). The result + // crosses a JSON boundary, so the missing code must be absent rather + // than an `undefined` property. + expect(failure.error.details).toStrictEqual({ host: "127.0.0.1:1" }); + expect(failure.error.message).toContain("base URL"); + expect(failure.error.message).not.toContain("your network"); + expect(failure.error.message).not.toContain("Internal tool error"); + expect(failure.error.message).not.toContain("/things"); + }), + ); + + // Classifying the failure took it off the hosts' correlation-id defect log, + // so the sanitized cause must reach both the caller (details) and operators + // (log) — and never the request, which carries the resolved auth header. + it.effect("connection refused reports the errno code to the caller and the log", () => + Effect.gen(function* () { + const baseUrl = yield* refusedBaseUrl(); + const { executor, address } = yield* buildExecutor(baseUrl); + const logged: Array = []; + + const result = yield* executor + .execute(address, {}) + .pipe(Effect.provide(capturingLogger(logged))); + + const host = new URL(baseUrl).host; + expect(result).toMatchObject({ + ok: false, + error: { code: "upstream_unreachable", details: { host, code: "ECONNREFUSED" } }, + }); + const rendered = JSON.stringify(result); + expect(rendered).not.toContain("/things"); + // The apiKey value `buildExecutor` puts on the connection. + expect(rendered).not.toContain("token"); + + const warning = logged.find((entry) => entry.message.includes("upstream unreachable")); + expect(warning?.annotations).toMatchObject({ + "plugin.openapi.integration": "f", + "plugin.openapi.upstream.host": host, + "plugin.openapi.upstream.transport_code": "ECONNREFUSED", + }); + }), + ); }); From 89b0f8d74cfb7d6a839bf08a267d892fb0cc676e Mon Sep 17 00:00:00 2001 From: Dhairyashil Shinde <93669429+dhairyashiil@users.noreply.github.com> Date: Tue, 15 Sep 2026 06:29:58 +0530 Subject: [PATCH 08/25] fix(mcp): report completed connected tool (#1964) * fix(mcp): report completed connected tool * Test queue timeout with a controlled clock * Verify connected tool provenance over MCP * Exclude upstream tool failures from completion metadata --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/completed-mcp-tool-name.md | 5 + e2e/scenarios/tool-call-contract.test.ts | 102 ++++++++++++++++-- packages/core/execution/src/engine.test.ts | 29 +++++ packages/core/execution/src/engine.ts | 26 ++++- .../core/execution/src/tool-invoker.test.ts | 19 ++++ packages/core/execution/src/tool-invoker.ts | 11 +- packages/hosts/mcp/src/tool-server.test.ts | 11 +- packages/kernel/core/src/types.ts | 2 + 8 files changed, 191 insertions(+), 14 deletions(-) create mode 100644 .changeset/completed-mcp-tool-name.md diff --git a/.changeset/completed-mcp-tool-name.md b/.changeset/completed-mcp-tool-name.md new file mode 100644 index 0000000000..1f97e79c55 --- /dev/null +++ b/.changeset/completed-mcp-tool-name.md @@ -0,0 +1,5 @@ +--- +"@executor-js/execution": patch +--- + +Completed MCP execute results now include `toolName` when a script successfully uses exactly one connected tool. Executions that use distinct tools remain unlabeled, and internal call provenance is not exposed in the MCP response. diff --git a/e2e/scenarios/tool-call-contract.test.ts b/e2e/scenarios/tool-call-contract.test.ts index 7cb5394e3b..b693e50602 100644 --- a/e2e/scenarios/tool-call-contract.test.ts +++ b/e2e/scenarios/tool-call-contract.test.ts @@ -21,7 +21,7 @@ import { randomBytes } from "node:crypto"; import { createServer } from "node:http"; import { expect } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Schema } from "effect"; import { composePluginApi } from "@executor-js/api/server"; import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; import { @@ -38,6 +38,13 @@ import type { McpSession } from "../src/surfaces/mcp"; const api = composePluginApi([openApiHttpPlugin()] as const); +const completion = Schema.Struct({ + structuredContent: Schema.Struct({ + status: Schema.String, + toolName: Schema.optional(Schema.String), + }), +}); + const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`; type UpstreamHandle = { @@ -114,14 +121,14 @@ const executeApproved = (session: McpSession, code: string) => guard += 1; } expect(result.ok, `execute completed (got: ${result.text.slice(0, 400)})`).toBe(true); - return result.text; + return result; }); /** Invoke a dynamic tool by full address and parse the envelope it returns. */ const invokeEnvelope = (session: McpSession, address: string, args: unknown = {}) => Effect.map( executeApproved(session, invokeByAddressCode(address, args)), - (text) => JSON.parse(text) as ToolEnvelope, + (result) => JSON.parse(result.text) as ToolEnvelope, ); // --------------------------------------------------------------------------- @@ -136,6 +143,20 @@ const widgetsSpec = (baseUrl: string): string => info: { title: "Widgets API", version: "1.0.0" }, servers: [{ url: baseUrl }], paths: { + "/unavailable": { + get: { + operationId: "unavailableWidgets", + summary: "Unavailable widgets", + responses: { "200": { description: "widgets" } }, + }, + }, + "/widgets/count": { + get: { + operationId: "countWidgets", + summary: "Count widgets", + responses: { "200": { description: "widget count" } }, + }, + }, "/widgets": { get: { operationId: "listWidgets", @@ -184,7 +205,7 @@ scenario( }, }); const created = JSON.parse( - yield* executeApproved(session, createConnectionCode(slug)), + (yield* executeApproved(session, createConnectionCode(slug))).text, ) as ToolEnvelope; expect(created.ok, `the no-auth connection was created: ${JSON.stringify(created)}`).toBe( true, @@ -199,7 +220,8 @@ scenario( const path = address!.replace(/^tools\./, ""); // 1. A well-addressed call executes and carries the upstream's payload. - const success = yield* invokeEnvelope(session, address!); + const successfulCall = yield* executeApproved(session, invokeByAddressCode(address!, {})); + const success = JSON.parse(successfulCall.text) as ToolEnvelope; expect( success.ok, `the call succeeded (got: ${JSON.stringify(success.error ?? {}).slice(0, 400)})`, @@ -207,7 +229,73 @@ scenario( expect(JSON.stringify(success.data), "the upstream's payload comes back").toContain( "anvil", ); + const structured = (successfulCall.raw as { readonly structuredContent?: unknown }) + .structuredContent; + expect(structured, "the completed MCP result includes structured content").toMatchObject({ + status: "completed", + toolName: path, + }); + expect(structured, "the internal tool-call trace is not exposed").not.toHaveProperty( + "toolPaths", + ); expect(upstream.requests(), "the upstream served exactly one call").toBe(1); + const discovery = yield* executeApproved( + session, + 'return await tools.search({ query: "widgets" });', + ); + const discoveryResult = yield* Schema.decodeUnknownEffect(completion)(discovery.raw); + expect(discoveryResult.structuredContent.toolName).toBeUndefined(); + expect(discovery.raw).not.toHaveProperty("structuredContent.toolPaths"); + + const failed = yield* executeApproved( + session, + invokeByAddressCode(address!.replace(/listWidgets$/, "missingWidget"), {}), + ); + const failedResult = yield* Schema.decodeUnknownEffect(completion)(failed.raw); + expect(failedResult.structuredContent.toolName).toBeUndefined(); + expect(failed.raw).not.toHaveProperty("structuredContent.toolPaths"); + const unavailableAddress = yield* Schema.decodeUnknownEffect(Schema.String)( + tools + .filter((tool) => String(tool.integration) === slug) + .map((tool) => String(tool.address)) + .find((candidate) => candidate.endsWith("unavailableWidgets")), + ); + const upstreamFailure = yield* executeApproved( + session, + invokeByAddressCode(unavailableAddress, {}), + ); + const upstreamFailureResult = yield* Schema.decodeUnknownEffect(completion)( + upstreamFailure.raw, + ); + expect(upstreamFailure.text).toContain('"ok":false'); + expect(upstreamFailureResult.structuredContent.toolName).toBeUndefined(); + expect(upstreamFailure.raw).not.toHaveProperty("structuredContent.toolPaths"); + + const repeated = yield* executeApproved( + session, + ` + await tools[${JSON.stringify(path)}]({}); + return await tools[${JSON.stringify(path)}]({}); + `, + ); + const repeatedResult = yield* Schema.decodeUnknownEffect(completion)(repeated.raw); + expect(repeatedResult.structuredContent.toolName).toBe(path); + expect(repeated.raw).not.toHaveProperty("structuredContent.toolPaths"); + const anotherAddress = tools + .filter((tool) => String(tool.integration) === slug) + .map((tool) => String(tool.address)) + .find((candidate) => candidate.endsWith("countWidgets")); + const anotherPath = yield* Schema.decodeUnknownEffect(Schema.String)(anotherAddress); + const multiple = yield* executeApproved( + session, + ` + await tools[${JSON.stringify(path)}]({}); + return await tools[${JSON.stringify(anotherPath.replace(/^tools\./, ""))}]({}); + `, + ); + const multipleResult = yield* Schema.decodeUnknownEffect(completion)(multiple.raw); + expect(multipleResult.structuredContent.toolName).toBeUndefined(); + expect(multiple.raw).not.toHaveProperty("structuredContent.toolPaths"); // 2a. A wrong TOOL name on a live connection: tool_not_found, and the // suggestions name the connection's real tools so the agent can @@ -257,7 +345,9 @@ scenario( "the defect mask never surfaces for a missing connection", ).not.toContain("Internal tool error"); - expect(upstream.requests(), "no misaddressed call ever reached the upstream").toBe(1); + expect(upstream.requests(), "only the five successful calls reached the upstream").toBe( + 5, + ); }), // Selfhost shares one workspace identity — leaked resources fail other // scenarios' zero-state assertions. diff --git a/packages/core/execution/src/engine.test.ts b/packages/core/execution/src/engine.test.ts index 06b9e20888..e1ca25eef6 100644 --- a/packages/core/execution/src/engine.test.ts +++ b/packages/core/execution/src/engine.test.ts @@ -309,6 +309,35 @@ describe("formatExecuteResult output identity", () => { expect(formatted.isError).toBe(false); }); + it("returns the sole distinct connected tool name without exposing the call trace", () => { + const result = { + result: { issues: [] }, + logs: [], + toolPaths: ["linear.org.work.issues.list", "linear.org.work.issues.list"], + } as ExecuteResult & { readonly toolPaths: readonly string[] }; + + const formatted = formatExecuteResult(result); + + expect(formatted.structured).toEqual({ + status: "completed", + result: { issues: [] }, + toolName: "linear.org.work.issues.list", + logs: [], + }); + }); + + it("omits a tool name when distinct connected tools were used", () => { + const result = { + result: { issues: [], projects: [] }, + logs: [], + toolPaths: ["linear.org.work.issues.list", "linear.org.work.projects.list"], + } as ExecuteResult & { readonly toolPaths: readonly string[] }; + + const formatted = formatExecuteResult(result); + + expect(formatted.structured).not.toHaveProperty("toolName"); + }); + it("truncates a long preview with the exact suffix and untouched structured value", () => { const value = { data: "é🎉".repeat(12_000) }; const pretty = JSON.stringify(value, null, 2); diff --git a/packages/core/execution/src/engine.ts b/packages/core/execution/src/engine.ts index 8bb9bda071..7e4e06340b 100644 --- a/packages/core/execution/src/engine.ts +++ b/packages/core/execution/src/engine.ts @@ -138,6 +138,11 @@ const truncate = (value: string, max: number): string => ? `${value.slice(0, max)}\n... [truncated ${value.length - max} chars]` : value; +const soleConnectedToolName = (toolPaths: readonly string[] | undefined): string | undefined => { + const names = [...new Set(toolPaths ?? [])]; + return names.length === 1 ? names[0] : undefined; +}; + export const formatExecuteResult = ( result: ExecuteResult, ): { @@ -183,11 +188,13 @@ export const formatExecuteResult = ( ? `(no return value; ${emittedNote})` : "(no result)"; const parts = [resultPart, ...(logText ? [`\nLogs:\n${logText}`] : [])]; + const toolName = soleConnectedToolName(result.toolPaths); return { text: parts.join("\n"), structured: { status: "completed", result: result.result ?? null, + ...(toolName ? { toolName } : {}), ...emittedField, logs: result.logs ?? [], }, @@ -318,8 +325,9 @@ const makeFullInvoker = ( executor: Executor, invokeOptions: InvokeOptions, toolDiscoveryProvider: ToolDiscoveryProvider, + onConnectedToolCall?: (path: string) => void, ): SandboxToolInvoker => { - const base = makeExecutorToolInvoker(executor, { invokeOptions }); + const base = makeExecutorToolInvoker(executor, { invokeOptions, onConnectedToolCall }); return { invoke: ({ path, args }) => { if (path === "search") { @@ -694,13 +702,18 @@ export const createExecutionEngine = toolPaths.push(path), ); fiber = yield* Effect.forkDetach( - codeExecutor.execute(code, invoker).pipe(Effect.withSpan("executor.code.exec")), + codeExecutor.execute(code, invoker).pipe( + Effect.map((result) => (toolPaths.length === 0 ? result : { ...result, toolPaths })), + Effect.withSpan("executor.code.exec"), + ), ); liveSandboxFibers.add(fiber); @@ -825,16 +838,19 @@ export const createExecutionEngine = toolPaths.push(path), + ); + const result = yield* codeExecutor.execute(code, invoker).pipe( + Effect.map((result) => (toolPaths.length === 0 ? result : { ...result, toolPaths })), + Effect.withSpan("executor.code.exec"), ); - const result = yield* codeExecutor - .execute(code, invoker) - .pipe(Effect.withSpan("executor.code.exec")); yield* annotateExecuteOutcome(result); return result; }); diff --git a/packages/core/execution/src/tool-invoker.test.ts b/packages/core/execution/src/tool-invoker.test.ts index 747bd23105..97beb69ed4 100644 --- a/packages/core/execution/src/tool-invoker.test.ts +++ b/packages/core/execution/src/tool-invoker.test.ts @@ -674,6 +674,25 @@ describe("tool discovery", () => { }), ); + it.effect("records only the connected tool resolved after discovery", () => + Effect.gen(function* () { + const executor = yield* makeSearchExecutor(); + const engine = createExecutionEngine({ executor, codeExecutor }); + + const execution = yield* engine.execute( + [ + 'const search = await tools.search({ query: "repository details", namespace: "github", limit: 1 });', + 'const result = await tools[search.items[0].path]({ owner: "executor", repo: "executor" });', + "return result;", + ].join("\n"), + { onElicitation: acceptAll }, + ); + + expect(execution.error).toBeUndefined(); + expect(execution.toolPaths).toEqual(["github.org.main.getRepositoryDetails"]); + }), + ); + it.effect("lets execution hosts provide custom tool discovery", () => Effect.gen(function* () { const executor = yield* makeSearchExecutor(); diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index 2df47644ed..6a09de027b 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -306,7 +306,10 @@ const extractNamespace = (path: string): string => { */ export const makeExecutorToolInvoker = ( executor: Executor, - options: { readonly invokeOptions: InvokeOptions }, + options: { + readonly invokeOptions: InvokeOptions; + readonly onConnectedToolCall?: (path: string) => void; + }, ): SandboxToolInvoker => ({ invoke: Effect.fn("mcp.tool.dispatch")(function* ({ path, args }) { yield* Effect.annotateCurrentSpan({ @@ -372,6 +375,12 @@ export const makeExecutorToolInvoker = ( // outcome annotation the dispatch span reads as healthy even when the // caller hit an upstream error or auth wall. yield* annotateToolResultOutcome(result); + const connectedToolPath = parseToolAddress(String(address)) + ? addressToPath(String(address)) + : undefined; + if (connectedToolPath && (!isToolResult(result) || result.ok)) { + options.onConnectedToolCall?.(connectedToolPath); + } if (isToolResult(result)) { return result; } diff --git a/packages/hosts/mcp/src/tool-server.test.ts b/packages/hosts/mcp/src/tool-server.test.ts index 1b7bb8aa45..c83fdabb0f 100644 --- a/packages/hosts/mcp/src/tool-server.test.ts +++ b/packages/hosts/mcp/src/tool-server.test.ts @@ -1039,12 +1039,15 @@ describe("MCP host server — native form-only elicitation", () => { // --------------------------------------------------------------------------- describe("MCP host server — client without elicitation (pause/resume)", () => { - it("completed execution returns result directly", async () => { + it("completed execution returns result and connected-tool metadata directly", async () => { const engine = makeStubEngine({ executeWithPause: () => Effect.succeed({ status: "completed", - result: { result: "done" }, + result: { + result: "done", + toolPaths: ["linear.org.work.issues.list"], + }, }), }); @@ -1054,6 +1057,10 @@ describe("MCP host server — client without elicitation (pause/resume)", () => arguments: { code: "ok" }, }); expect(result.content).toEqual([{ type: "text", text: "done" }]); + expect(result.structuredContent).toMatchObject({ + status: "completed", + toolName: "linear.org.work.issues.list", + }); expect(result.isError).toBeFalsy(); }); }); diff --git a/packages/kernel/core/src/types.ts b/packages/kernel/core/src/types.ts index 1480b27b9a..d6dee10ad4 100644 --- a/packages/kernel/core/src/types.ts +++ b/packages/kernel/core/src/types.ts @@ -46,6 +46,8 @@ export type ExecuteResult = { /** Enumerable failure class for telemetry; never carries message content. */ errorKind?: ExecuteErrorKind; logs?: string[]; + /** Successful connected-tool paths observed during this execution. */ + toolPaths?: readonly string[]; }; /** From b961092443435e9cc0796626436f2cc2a8001b3b Mon Sep 17 00:00:00 2001 From: Davies Ayo Date: Tue, 15 Sep 2026 11:00:02 +1000 Subject: [PATCH 09/25] Return tool annotations from describe.tool (#1980) * Return tool annotations from describe.tool `ToolSchemaView` carried everything about a tool except the annotations a plugin declares on it, so code written inside `execute` could not tell an approval-gated tool from a plain one except by reading its prose description. The data was already persisted on the tool row and already used by the executor's own approval copy. `tools.schema` now projects the three fields declared in `ToolAnnotations` (`requiresApproval`, `approvalDescription`, `mayElicit`), and `describe.tool` passes them through. The fields are picked explicitly rather than spread, because plugins keep private bookkeeping alongside the contract: the mcp plugin stores its upstream tool name and `_meta` map in the same column, and none of that should reach a caller. The key is omitted when a tool declares no annotations, so the describe payload does not grow for the tools that have nothing to say. Co-Authored-By: Claude Opus 5 (1M context) * Test queue timeout with a controlled clock * Verify annotation projections and expose resume failure context * Model private annotation metadata through the test adapter * Use the static tool address for schema discovery --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/describe-tool-annotations.md | 5 +++ .../src/worker.e2e.node.test.ts | 4 +- e2e/scenarios/tool-annotations.test.ts | 44 +++++++++++++++++++ .../core/execution/src/tool-invoker.test.ts | 34 ++++++++++++++ packages/core/execution/src/tool-invoker.ts | 10 ++++- packages/core/sdk/src/executor.ts | 28 +++++++++++- packages/core/sdk/src/index.ts | 2 +- packages/core/sdk/src/types.ts | 12 +++++ 8 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 .changeset/describe-tool-annotations.md create mode 100644 e2e/scenarios/tool-annotations.test.ts diff --git a/.changeset/describe-tool-annotations.md b/.changeset/describe-tool-annotations.md new file mode 100644 index 0000000000..127b8f38af --- /dev/null +++ b/.changeset/describe-tool-annotations.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Return a tool's declared annotations from `tools.schema` and `describe.tool`. Code inside `execute` can now read `requiresApproval`, `approvalDescription` and `mayElicit` without parsing the tool's prose description. diff --git a/apps/host-cloudflare/src/worker.e2e.node.test.ts b/apps/host-cloudflare/src/worker.e2e.node.test.ts index 738a358827..0a9bc35d1f 100644 --- a/apps/host-cloudflare/src/worker.e2e.node.test.ts +++ b/apps/host-cloudflare/src/worker.e2e.node.test.ts @@ -513,7 +513,9 @@ describe("cloudflare host e2e (workerd/miniflare)", () => { }; }>(resume); expect(resumed.result?.structuredContent?.status).not.toBe("execution_not_found"); - expect(resumed.result?.structuredContent?.recovery).not.toBe("re_execute"); + expect(resumed.result?.structuredContent?.recovery, JSON.stringify(resumed.result)).not.toBe( + "re_execute", + ); expect(resumed.result?.isError).toBeFalsy(); expect(resumed.result?.structuredContent?.status).toBe("completed"); }, 60_000); diff --git a/e2e/scenarios/tool-annotations.test.ts b/e2e/scenarios/tool-annotations.test.ts new file mode 100644 index 0000000000..398583db7f --- /dev/null +++ b/e2e/scenarios/tool-annotations.test.ts @@ -0,0 +1,44 @@ +import { expect } from "@effect/vitest"; +import { Effect, Schema } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { ToolAddress } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Mcp, Target } from "../src/services"; + +const api = composePluginApi([] as const); +const annotations = Schema.Struct({ requiresApproval: Schema.Boolean }); +const descriptions = Schema.Struct({ + gated: Schema.Struct({ annotations }), + plain: Schema.Record(Schema.String, Schema.Unknown), +}); + +scenario( + "Tool discovery · declared approval annotations survive API and sandbox descriptions", + { timeout: 120_000 }, + Effect.gen(function* () { + const target = yield* Target; + const mcp = yield* Mcp; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const session = mcp.session(identity); + const address = ToolAddress.make("executor.coreTools.connections.create"); + const view = yield* client.tools.schema({ query: { address } }); + expect(view.annotations).toEqual({ requiresApproval: true }); + const described = yield* session.call("execute", { + code: ` + return JSON.stringify({ + gated: await tools.describe.tool({ path: "executor.coreTools.connections.create" }), + plain: await tools.describe.tool({ path: "executor.coreTools.connections.list" }), + }); + `, + }); + expect(described.ok, described.text).toBe(true); + const result = yield* Schema.decodeUnknownEffect(Schema.fromJsonString(descriptions))( + described.text, + ); + expect(result.gated.annotations).toEqual(view.annotations); + expect(result.plain).not.toHaveProperty("annotations"); + }), +); diff --git a/packages/core/execution/src/tool-invoker.test.ts b/packages/core/execution/src/tool-invoker.test.ts index 97beb69ed4..a2aa222d17 100644 --- a/packages/core/execution/src/tool-invoker.test.ts +++ b/packages/core/execution/src/tool-invoker.test.ts @@ -18,6 +18,7 @@ import { type AnyPlugin, type CredentialProvider, type Elicit, + type ToolAnnotations, type ToolDef, } from "@executor-js/sdk"; import { @@ -127,6 +128,7 @@ type TestToolSpec = { readonly outputJsonSchema?: unknown; /** Standard-schema validator applied to args in `invokeTool`. */ readonly validator?: Validator; + readonly annotations?: ToolAnnotations; readonly handler: (input: ToolHandlerInput) => Effect.Effect; }; @@ -160,6 +162,12 @@ const validateArgs = ( ); }; +const withPrivateAnnotations = (annotations: ToolAnnotations) => ({ + ...annotations, + upstreamToolName: "private-provider-tool", + _meta: { privateMarker: "not-public" }, +}); + const makeTestPlugin = (config: { readonly pluginId: string; readonly integration: string; @@ -179,6 +187,11 @@ const makeTestPlugin = (config: { description: spec.description, inputSchema: spec.inputJsonSchema, outputSchema: spec.outputJsonSchema, + ...(spec.annotations + ? { + annotations: withPrivateAnnotations(spec.annotations), + } + : {}), }), ), }), @@ -238,6 +251,11 @@ const crmPlugin = makeTestPlugin({ description: "Create a CRM contact record", inputJsonSchema: ContactInputJson, validator: ContactValidator, + annotations: { + requiresApproval: true, + approvalDescription: "Creates a contact record in the CRM", + mayElicit: false, + }, handler: () => Effect.succeed({ id: "contact_1" }), }, { @@ -897,6 +915,22 @@ describe("tool discovery", () => { }), ); + it.effect("describes a tool's declared annotations, and omits the key when it has none", () => + Effect.gen(function* () { + const executor = yield* makeSearchExecutor(); + + const annotated = yield* describeTool(executor, "crm.org.main.createContact"); + expect(annotated.annotations).toEqual({ + requiresApproval: true, + approvalDescription: "Creates a contact record in the CRM", + mayElicit: false, + }); + + const plain = yield* describeTool(executor, "crm.org.main.listContacts"); + expect(plain.annotations).toBeUndefined(); + }), + ); + it.effect("serves an observed shape with a provenance note once a schemaless tool runs", () => Effect.gen(function* () { const executor = yield* makeSearchExecutor(); diff --git a/packages/core/execution/src/tool-invoker.ts b/packages/core/execution/src/tool-invoker.ts index 6a09de027b..b8458929a4 100644 --- a/packages/core/execution/src/tool-invoker.ts +++ b/packages/core/execution/src/tool-invoker.ts @@ -82,6 +82,13 @@ type DescribedTool = { readonly outputTypeScript?: string; readonly outputTypeScriptNote?: string; readonly typeScriptDefinitions?: Record; + /** The tool's declared annotations, when it carries any. Lets code inside + * `execute` branch on approval posture without parsing the description. */ + readonly annotations?: { + readonly requiresApproval?: boolean; + readonly approvalDescription?: string; + readonly mayElicit?: boolean; + }; /** Set when the path resolves to no tool — mirrors invoke's tool_not_found. */ readonly error?: { readonly code: "tool_not_found"; @@ -135,7 +142,7 @@ const BUILTIN_TOOL_DESCRIPTIONS: ReadonlyMap = new Map< outputTypeScript: "DescribedTool", typeScriptDefinitions: { DescribedTool: - '{ path: string; name: string; description?: string; inputTypeScript?: string; outputTypeScript?: string; typeScriptDefinitions?: { [k: string]: string; }; error?: { code: "tool_not_found"; message: string; suggestions?: string[]; }; }', + '{ path: string; name: string; description?: string; inputTypeScript?: string; outputTypeScript?: string; typeScriptDefinitions?: { [k: string]: string; }; annotations?: { requiresApproval?: boolean; approvalDescription?: string; mayElicit?: boolean; }; error?: { code: "tool_not_found"; message: string; suggestions?: string[]; }; }', }, }, ], @@ -892,6 +899,7 @@ export const describeTool = Effect.fn("executor.tools.describe")(function* ( } : {}), typeScriptDefinitions: withToolResultDefinitions(schema.typeScriptDefinitions), + ...(schema.annotations ? { annotations: schema.annotations } : {}), }; return described; }); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 6bfe859466..451fc0f54b 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -199,7 +199,7 @@ import { ORG_SUBJECT, type ExecutorOwnerPolicyContext, } from "./owner-policy"; -import { ToolSchemaView, type IntegrationDetectionResult } from "./types"; +import { ToolAnnotationsView, ToolSchemaView, type IntegrationDetectionResult } from "./types"; import { type Tool, type ToolAnnotations, type ToolDef, type ToolListFilter } from "./tool"; import { buildToolTypeScriptPreview } from "./schema-types"; import { collectReferencedDefinitions } from "./schema-refs"; @@ -1219,6 +1219,30 @@ const rowToTool = ( }; }; +// Projects a tool's annotations onto the schema view. Plugins persist extra +// keys alongside the declared contract (the mcp plugin stores its upstream tool +// name and `_meta` there so they survive to invokeTool), so the three declared +// fields are picked explicitly rather than spread: a caller reading the view +// gets the contract in `tool.ts` and nothing a plugin keeps for itself. +const toolAnnotationsView = ( + annotations: ToolAnnotations | undefined, +): ToolAnnotationsView | undefined => { + if (!annotations) return undefined; + const view: { + requiresApproval?: boolean; + approvalDescription?: string; + mayElicit?: boolean; + } = {}; + if (typeof annotations.requiresApproval === "boolean") { + view.requiresApproval = annotations.requiresApproval; + } + if (typeof annotations.approvalDescription === "string") { + view.approvalDescription = annotations.approvalDescription; + } + if (typeof annotations.mayElicit === "boolean") view.mayElicit = annotations.mayElicit; + return Object.keys(view).length > 0 ? ToolAnnotationsView.make(view) : undefined; +}; + // --------------------------------------------------------------------------- // Condition builders // --------------------------------------------------------------------------- @@ -5692,6 +5716,7 @@ export const createExecutor = Date: Tue, 15 Sep 2026 06:30:06 +0530 Subject: [PATCH 10/25] fix(oauth): request offline_access for Vercel MCP (#1951) * fix(oauth): request offline_access for Vercel MCP * Test queue timeout with a controlled clock * Verify Vercel lifecycle scopes through an OAuth connection --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/vercel-mcp-refresh-token.md | 5 + e2e/selfhost/vercel-oauth-lifecycle.test.ts | 146 ++++++++++++++++++ .../sdk/src/oauth-register-dynamic.test.ts | 33 ++++ .../core/sdk/src/oauth-scope-union.test.ts | 66 +++++++- packages/core/sdk/src/oauth-service.ts | 24 ++- 5 files changed, 267 insertions(+), 7 deletions(-) create mode 100644 .changeset/vercel-mcp-refresh-token.md create mode 100644 e2e/selfhost/vercel-oauth-lifecycle.test.ts diff --git a/.changeset/vercel-mcp-refresh-token.md b/.changeset/vercel-mcp-refresh-token.md new file mode 100644 index 0000000000..dbcf4e5d2a --- /dev/null +++ b/.changeset/vercel-mcp-refresh-token.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Keep Vercel MCP connections renewable by requesting the provider's `offline_access` lifecycle scope during registration and authorization. diff --git a/e2e/selfhost/vercel-oauth-lifecycle.test.ts b/e2e/selfhost/vercel-oauth-lifecycle.test.ts new file mode 100644 index 0000000000..cc02af906f --- /dev/null +++ b/e2e/selfhost/vercel-oauth-lifecycle.test.ts @@ -0,0 +1,146 @@ +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { connectEmulator } from "@executor-js/emulate"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; + +import { createEmulatorInstance } from "../src/emulator-instance"; +import { scenario } from "../src/scenario"; +import { Api, Browser, Mcp, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +scenario( + "Vercel OAuth · lifecycle scopes survive registration and a complete MCP connection", + { timeout: 180_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const mcp = yield* Mcp; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const session = mcp.session(identity); + // Authenticate the host MCP transport before minting upstream credentials. + // The hosted emulator currently loses its OAuth token map on eviction. + const ready = yield* session.call("execute", { code: "return true;" }); + expect(ready.ok).toBe(true); + const base = yield* createEmulatorInstance("mcp", "vercel-lifecycle"); + const emulator = yield* Effect.promise(() => + connectEmulator({ baseUrl: base, service: "mcp" }), + ); + yield* Effect.promise(() => + emulator.seed({ users: [{ login: "lifecycle-user" }], scopes: ["openid"] }), + ); + const slug = IntegrationSlug.make(`lifecycle-${randomBytes(4).toString("hex")}`); + const app = OAuthClientSlug.make(`${slug}-app`); + yield* Effect.addFinalizer(() => + client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore), + ); + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: "Lifecycle MCP", + endpoint: `${base}/mcp`, + slug, + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + const registered = yield* client.oauth.registerDynamic({ + payload: { + owner: "org", + slug: app, + issuer: base, + registrationEndpoint: `${base}/register`, + authorizationUrl: "https://vercel.com/oauth/authorize", + tokenUrl: `${base}/token`, + resource: `${base}/mcp`, + scopes: ["openid"], + tokenEndpointAuthMethodsSupported: ["none"], + originIntegration: slug, + }, + }); + yield* Effect.addFinalizer(() => + client.oauth + .removeClient({ params: { slug: registered.client }, payload: { owner: "org" } }) + .pipe(Effect.ignore), + ); + const started = yield* client.oauth.start({ + payload: { + owner: "org", + client: registered.client, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: slug, + template: AuthTemplateSlug.make("oauth2"), + }, + }); + if (started.status !== "redirect") + return yield* Effect.die("Expected authorization redirect"); + yield* Effect.addFinalizer(() => + client.oauth.cancel({ payload: { state: started.state } }).pipe(Effect.ignore), + ); + const authorize = new URL(started.authorizationUrl); + expect(authorize.origin + authorize.pathname).toBe("https://vercel.com/oauth/authorize"); + expect(authorize.searchParams.get("scope")?.split(" ")).toEqual(["openid", "offline_access"]); + // Route only the provider transport to the published emulator. The product + // produced all OAuth parameters; DCR, PKCE, consent, callback and MCP are real. + // This fixture does not claim to prove Vercel's refresh-token issuance policy. + const consentUrl = `${base}/authorize${authorize.search}`; + yield* browser.session(identity, async ({ page, step }) => { + await step("Review the provider consent request", async () => { + await page.goto(consentUrl); + await page.getByText("Authorize MCP client", { exact: true }).waitFor(); + }); + await step("Approve the requested access and complete the callback", async () => { + const form = Object.fromEntries(authorize.searchParams); + const approved = await page.request.post(`${base}/authorize/approve`, { + form: { ...form, login: "lifecycle-user" }, + maxRedirects: 0, + }); + expect(approved.status()).toBe(302); + const callback = approved.headers().location; + expect(callback).toBeDefined(); + await page.goto(callback!); + await page + .getByText(/connected|complete|success/i) + .first() + .waitFor({ timeout: 30_000 }); + }); + }); + const tools = yield* client.tools.list({ query: { integration: slug } }); + const tool = tools.find((entry) => entry.name === "get_me"); + expect(tool).toBeDefined(); + if (!tool) return yield* Effect.die("Connected MCP has no get_me tool"); + let result = yield* session.call("execute", { + code: `const path = ${JSON.stringify(String(tool.address))}.split(".").slice(1); let call = tools; for (const part of path) call = call[part]; return await call({});`, + }); + for (let attempts = 0; result.text.includes("executionId:") && attempts < 10; attempts += 1) + result = yield* session.approvePaused(result.text); + expect(result.ok).toBe(true); + expect(result.text).toContain("lifecycle-user"); + const ledger = yield* Effect.promise(() => emulator.ledger.list()); + const registration = ledger.find( + (entry) => entry.method === "POST" && entry.path.endsWith("/register"), + ); + expect(registration?.request.body).toMatchObject({ scope: "openid offline_access" }); + expect( + ledger.some( + (entry) => + entry.method === "POST" && + entry.path.endsWith("/token") && + entry.response.status === 200, + ), + ).toBe(true); + }), + ), +); diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index 6a5a281407..bc13050ded 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -373,6 +373,39 @@ describe("oauth.registerDynamicClient", () => { ), ); + it.effect("registers Vercel clients with offline_access for refresh tokens", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ + scopes: ["openid", "offline_access"], + }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + + yield* executor.oauth.registerDynamicClient({ + owner: "org", + slug: CLIENT, + issuer: "https://vercel.com", + registrationEndpoint: server.registrationEndpoint, + authorizationUrl: "https://vercel.com/oauth/authorize", + tokenUrl: server.tokenEndpoint, + resource: "https://mcp.vercel.com/", + scopes: ["openid"], + tokenEndpointAuthMethodsSupported: ["none"], + clientName: "Executor", + redirectUri: FLOW_REDIRECT_URI, + originIntegration: INTEG, + }); + + const requests = yield* server.requests; + const registration = requests.find( + (request) => request.path === "/register" && request.method === "POST", + ); + expect(registration?.body).toContain('"scope":"openid offline_access"'); + }), + ), + ); + it.effect("reuses a legacy DCR row once its origin_issuer is backfilled", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-scope-union.test.ts b/packages/core/sdk/src/oauth-scope-union.test.ts index 6e7aa41669..78f6d6df2b 100644 --- a/packages/core/sdk/src/oauth-scope-union.test.ts +++ b/packages/core/sdk/src/oauth-scope-union.test.ts @@ -159,11 +159,14 @@ const serveMetadataServer = (config: { * given server as its resource, returning the executor ready to `oauth.start`. * The shared setup for the discovery cases below; case (h) inlines its own (no * `resource`) because the absent resource IS the case under test. */ -const setupMcpScopeClient = (server: { - readonly authorizationEndpoint: string; - readonly tokenEndpoint: string; - readonly mcpResourceUrl: string; -}) => +const setupMcpScopeClient = ( + server: { + readonly authorizationEndpoint: string; + readonly tokenEndpoint: string; + readonly mcpResourceUrl: string; + }, + options: { readonly authorizationEndpoint?: string } = {}, +) => Effect.gen(function* () { const plugins = [memoryCredentialsPlugin(), makeMcpScopePlugin({ scopes: null })] as const; const { executor } = yield* makeTestWorkspaceHarness({ plugins }); @@ -171,7 +174,7 @@ const setupMcpScopeClient = (server: { yield* executor.oauth.createClient({ owner: "org", slug: CLIENT, - authorizationUrl: server.authorizationEndpoint, + authorizationUrl: options.authorizationEndpoint ?? server.authorizationEndpoint, tokenUrl: server.tokenEndpoint, grant: "authorization_code", clientId: "test-client", @@ -405,6 +408,57 @@ describe("oauth.start integration-driven scopes", () => { ), ); + it.effect("requests Vercel offline_access so authorization-code connections can refresh", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveMetadataServer({ + prm: { scopesSupported: ["openid"] }, + }); + const executor = yield* setupMcpScopeClient(server, { + authorizationEndpoint: "https://vercel.com/oauth/authorize", + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + + expect(scopesFromAuthorizeUrl(started.authorizationUrl)).toEqual([ + "openid", + "offline_access", + ]); + }), + ), + ); + + it.effect("does not add Vercel lifecycle scopes on a different port", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveMetadataServer({ prm: { scopesSupported: ["openid"] } }); + const executor = yield* setupMcpScopeClient(server, { + authorizationEndpoint: "https://vercel.com:8443/oauth/authorize", + }); + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + expect(scopesFromAuthorizeUrl(started.authorizationUrl)).toEqual(["openid"]); + }), + ), + ); + it.effect( "(e) for MCP, discovers scopes from a cross-origin authorization server named in resource metadata", () => diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 5fcf64c26f..fdf727c38d 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -556,6 +556,23 @@ interface LoadedOAuthClient { readonly tokenRequestFormat?: "form" | "json"; } +/** Provider lifecycle scopes that are required to keep an authorization-code + * connection renewable but are omitted from the protected resource's API + * scope list. Vercel's MCP resource advertises only `openid`, while its + * authorization server issues a refresh token only when `offline_access` is + * requested. Keep the exception bound to Vercel's exact official authorize + * endpoint so an unrelated OAuth server never receives a broader request. */ +const additionalAuthorizationLifecycleScopes = (client: { + readonly authorizationUrl: string; +}): readonly string[] => { + if (!URL.canParse(client.authorizationUrl)) return []; + const authorization = new URL(client.authorizationUrl); + return authorization.origin === "https://vercel.com" && + authorization.pathname === "/oauth/authorize" + ? ["offline_access"] + : []; +}; + /** Where an OAuth app's client secret is stored in the default writable * provider — derived solely from the app's (owner, slug) identity. */ const clientSecretItemId = (owner: Owner, slug: OAuthClientSlug): string => @@ -1416,6 +1433,10 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }); } const authMethod = pickDcrAuthMethod(input.tokenEndpointAuthMethodsSupported); + const registrationScopes = dedupeScopes([ + ...input.scopes, + ...additionalAuthorizationLifecycleScopes(input), + ]); const information = yield* registerDynamicClientDcr( { registrationEndpoint: input.registrationEndpoint, @@ -1426,7 +1447,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { response_types: ["code"], token_endpoint_auth_method: authMethod, application_type: isLoopbackHttpUrl(flowRedirectUri) ? "native" : "web", - scope: input.scopes.length > 0 ? input.scopes.join(" ") : undefined, + scope: registrationScopes.length > 0 ? registrationScopes.join(" ") : undefined, }, }, { httpClientLayer, endpointUrlPolicy: deps.endpointUrlPolicy }, @@ -1988,6 +2009,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { const completeAuthorizationScopes = dedupeScopes([ ...authorizationRequestedScopes, ...(firstParty?.additionalAuthorizationScopes ?? []), + ...additionalAuthorizationLifecycleScopes(client), ]); // authorization_code: persist a session + build the authorize URL. From 5ecb8812287c96b8ea4c3bf33f3c1ee34badb1df Mon Sep 17 00:00:00 2001 From: ra-co88 Date: Tue, 15 Sep 2026 02:15:32 +0100 Subject: [PATCH 11/25] fix(cloud): require CSRF state in the WorkOS login callback (#1886) * fix(cloud): require CSRF state in the WorkOS login callback * Test queue timeout with a controlled clock * Exercise browser binding and replay protection for login state * Capture provider redirect before testing callback state * Wait for key revocation before checking authentication --------- Co-authored-by: pt-act <211776491+pt-act@users.noreply.github.com> Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/tidy-login-state.md | 17 ++ apps/cloud/src/auth/handlers.ts | 24 +-- .../auth/workos-callback-state.node.test.ts | 166 ++++++++++++++++++ e2e/cloud/login-csrf.test.ts | 75 ++++++++ e2e/cloud/org-api-keys-console.test.ts | 4 +- 5 files changed, 273 insertions(+), 13 deletions(-) create mode 100644 .changeset/tidy-login-state.md create mode 100644 apps/cloud/src/auth/workos-callback-state.node.test.ts create mode 100644 e2e/cloud/login-csrf.test.ts diff --git a/.changeset/tidy-login-state.md b/.changeset/tidy-login-state.md new file mode 100644 index 0000000000..7cbf222e3f --- /dev/null +++ b/.changeset/tidy-login-state.md @@ -0,0 +1,17 @@ +--- +"@executor-js/cloud": patch +--- + +fix: make login CSRF state mandatory in the WorkOS callback + +The callback previously skipped its CSRF check whenever the redirect carried +no `state` value ("some WorkOS-initiated redirects don't include one"). That +bypass let an attacker complete their own OAuth round-trip and redirect a +victim's browser through the callback with the attacker's `code` and no +`state`, silently signing the victim into the attacker's account (login CSRF). + +The check is now unconditional: a callback without a state matching the +`wos-login-state` cookie set on `/login` is rejected with 400. This is a +breaking change for any client relying on the undocumented no-state entry +path; server-initiated flows that cannot carry state must be redesigned with +a signed nonce instead of re-adding the bypass. diff --git a/apps/cloud/src/auth/handlers.ts b/apps/cloud/src/auth/handlers.ts index ae91bd35a4..a568dadecf 100644 --- a/apps/cloud/src/auth/handlers.ts +++ b/apps/cloud/src/auth/handlers.ts @@ -189,17 +189,17 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group( const workos = yield* WorkOSClient; const users = yield* UserStoreService; const cookieState = request.cookies[STATE_COOKIE] ?? null; - // CSRF check is only enforced when the redirect carries a state - // value — some WorkOS-initiated redirects don't include one. - // When state is present, it MUST match the cookie we set on - // /login. - if (query.state !== undefined) { - if (!cookieState || !timingSafeEqual(cookieState, query.state)) { - return deleteResponseCookie( - HttpServerResponse.text("Invalid login state", { status: 400 }), - STATE_COOKIE, - ); - } + // CSRF is unconditional: every callback must carry a state that + // matches the cookie set on /login. There is no legitimate + // no-state entry path — omitting state previously allowed an + // attacker to complete their own OAuth round-trip and redirect a + // victim's browser through this callback, signing the victim into + // the attacker's account (login CSRF). + if (!cookieState || !timingSafeEqual(cookieState, query.state ?? "")) { + return deleteResponseCookie( + HttpServerResponse.text("Invalid login state", { status: 400 }), + STATE_COOKIE, + ); } const result = yield* workos.authenticateWithCode(query.code); @@ -210,7 +210,7 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group( let sealedSession = result.sealedSession; // Resume where the SSR gate interrupted them. The state passed the - // CSRF check above whenever it's present, but it's still a + // CSRF check above, but it's still a // round-tripped value, so the returnTo inside it is re-validated like // any other untrusted path. const returnTo = safeReturnTo(decodeLoginState(query.state)?.returnTo) ?? "/"; diff --git a/apps/cloud/src/auth/workos-callback-state.node.test.ts b/apps/cloud/src/auth/workos-callback-state.node.test.ts new file mode 100644 index 0000000000..3787f57b01 --- /dev/null +++ b/apps/cloud/src/auth/workos-callback-state.node.test.ts @@ -0,0 +1,166 @@ +// --------------------------------------------------------------------------- +// Focused tests — the WorkOS login callback's CSRF gate. +// +// The callback's CSRF check must be unconditional: no state ⇒ 400 before any +// WorkOS call; a replayed (already consumed) state ⇒ 400; a fresh state +// matching the cookie ⇒ 302 + session. +// +// Test seams follow repo conventions: @effect/vitest, Layer.succeed stubs +// (see org-selector-auth.node.test.ts), and HttpRouter.toWebHandler for the +// HTTP surface (see api.request-scope.node.test.ts). +// --------------------------------------------------------------------------- + +import { afterAll, describe, expect, it } from "@effect/vitest"; +import { Effect, Layer } from "effect"; +import { HttpRouter, HttpServer } from "effect/unstable/http"; +import { HttpApiBuilder } from "effect/unstable/httpapi"; +import { HttpApi } from "effect/unstable/httpapi"; + +import { CloudAuthPublicHandlers } from "./handlers"; +import { CloudAuthPublicApi } from "./api"; +import { UserStoreService } from "./context"; +import { WorkOSClient, type WorkOSClientService } from "./workos"; +import { encodeLoginState } from "./login-state"; + +// The route under test serves under the `/api` prefix in the composed app; +// toWebHandler mounts the raw group, so paths here are relative to the group. +const SESSION_COOKIE = "wos-session"; +const STATE_COOKIE = "wos-login-state"; + +const STUB_USER_ID = "user_test"; +const STUB_SESSION = "sealed-session-stub"; +const STUB_ORG_ID = "org_test"; + +const stubWorkOS = Layer.succeed( + WorkOSClient, + new Proxy({} as WorkOSClientService, { + get: (_t, prop) => { + if (prop === "authenticateWithCode") { + return () => + Effect.succeed({ + user: { id: STUB_USER_ID, email: "u@test" }, + organizationId: STUB_ORG_ID, + sealedSession: STUB_SESSION, + }); + } + if (prop === "listUserMemberships") { + return () => Effect.succeed({ data: [] }); + } + return () => Effect.die(`unexpected WorkOSClient.${String(prop)} call`); + }, + }), +); + +const stubUsers = Layer.succeed(UserStoreService)({ + use: (_op, fn) => + Effect.promise(() => + fn({ + ensureAccount: async (id: string) => ({ id, createdAt: new Date() }), + getAccount: async (id: string) => ({ id, createdAt: new Date() }), + upsertOrganization: async (org: { id: string; name: string }) => ({ + ...org, + slug: org.id, + createdAt: new Date(), + }), + getOrganization: async (id: string) => ({ + id, + name: "Org " + id, + slug: id, + createdAt: new Date(), + }), + getOrganizationBySlug: async (slug: string) => ({ + id: slug, + name: slug, + slug, + createdAt: new Date(), + }), + deleteOrganizationCascade: async () => {}, + }), + ), +}); + +// Only the public group is under test; the session group (and its SessionAuth +// middleware, which needs a live DB) is out of scope — the callback route lives +// in CloudAuthPublicApi and requires no middleware. +const PublicApi = HttpApi.make("cloudWeb").add(CloudAuthPublicApi); + +const App = HttpApiBuilder.layer(PublicApi).pipe( + Layer.provide(CloudAuthPublicHandlers), + Layer.provide(stubWorkOS), + Layer.provide(stubUsers), + Layer.provide(HttpServer.layerServices), +); + +const app = HttpRouter.toWebHandler(App, { disableLogger: true }); +afterAll(() => app.dispose()); + +const run = (request: Request) => { + // beta.59: the handler type expects a context argument; this layer stack + // needs none at runtime — pass undefined like the api.request-scope tests. + return app.handler(request, undefined as never); +}; + +const callbackUrl = (state?: string, code = "code_1") => + `https://executor.test/auth/callback${state ? `?state=${encodeURIComponent(state)}` : ""}${state ? "&" : "?"}code=${code}`; + +describe("workos callback · CSRF state hardening", () => { + it("rejects a callback with NO state (the former bypass) before any WorkOS call", async () => { + const res = await run(new Request(callbackUrl(undefined), { redirect: "manual" })); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Invalid login state"); + expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE); + }); + + it("rejects missing state even when the browser has a login cookie", async () => { + const res = await run( + new Request(callbackUrl(undefined), { + headers: { cookie: `${STATE_COOKIE}=victim-login-state` }, + redirect: "manual", + }), + ); + expect(res.status).toBe(400); + expect(await res.text()).toBe("Invalid login state"); + expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE); + }); + + it("rejects a state that does not match the login cookie", async () => { + const res = await run( + new Request(callbackUrl("attacker-controlled-state"), { + headers: { cookie: `${STATE_COOKIE}=victim-login-state` }, + redirect: "manual", + }), + ); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Invalid login state"); + }); + + it("accepts a fresh state matching the cookie and issues a session (302 + cookie)", async () => { + // /login sets the cookie; simulate its value for this callback. + const state = encodeLoginState({ nonce: "nonce-123", returnTo: "/" }); + const res = await run( + new Request(callbackUrl(state), { + headers: { cookie: `${STATE_COOKIE}=${state}` }, + redirect: "manual", + }), + ); + expect(res.status).toBe(302); + expect(res.headers.get("set-cookie") ?? "").toContain(SESSION_COOKIE); + }); + + it("rejects a replayed state (single-use contract preserved downstream)", async () => { + // Replay of a state whose cookie is gone (already consumed by the login + // round-trip) must fail closed. + const state = encodeLoginState({ nonce: "nonce-replay", returnTo: "/" }); + const first = await run( + new Request(callbackUrl(state), { + headers: { cookie: `${STATE_COOKIE}=${state}` }, + redirect: "manual", + }), + ); + expect(first.status).toBe(302); + + // Second callback: same state, no cookie (session-store consumed it). + const replay = await run(new Request(callbackUrl(state), { redirect: "manual" })); + expect(replay.status).toBe(400); + }); +}); diff --git a/e2e/cloud/login-csrf.test.ts b/e2e/cloud/login-csrf.test.ts new file mode 100644 index 0000000000..fbc237e8b3 --- /dev/null +++ b/e2e/cloud/login-csrf.test.ts @@ -0,0 +1,75 @@ +import { randomUUID } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; + +import { scenario } from "../src/scenario"; +import { Browser, Target } from "../src/services"; + +scenario( + "Login CSRF · state is required, bound to the browser, and consumed after login", + { timeout: 180_000 }, + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const email = `csrf-${randomUUID()}@e2e.test`; + yield* browser.session({ label: "anonymous" }, async ({ page, step }) => { + const interceptCallback = async (): Promise => { + let callback: string | undefined; + // Pause the real provider response before its redirect reaches the app. + // Playwright does not route subsequent hops of a redirect chain. + await page.route("**/user_management/authorize/submit", async (route) => { + const response = await route.fetch({ maxRedirects: 0 }); + expect(response.status()).toBe(302); + callback = response.headers().location; + await route.fulfill({ + status: 200, + contentType: "text/plain", + body: "Authorization ready for callback validation", + }); + }); + await page.goto(new URL("/api/auth/login", target.baseUrl).toString()); + await page.getByPlaceholder("new-user@example.com").fill(email); + await page.getByRole("button", { name: /Continue/ }).click(); + await expect.poll(() => callback).toBeDefined(); + await page.unroute("**/user_management/authorize/submit"); + if (!callback) throw new Error("AuthKit did not return a callback"); + return callback; + }; + await step("Refuse a valid authorization code with no state", async () => { + const callback = new URL(await interceptCallback()); + callback.searchParams.delete("state"); + const response = await page.request.get(callback.toString(), { maxRedirects: 0 }); + expect(response.status()).toBe(400); + expect(await response.text()).toBe("Invalid login state"); + expect( + (await page.context().cookies()).some((cookie) => cookie.name === "wos-session"), + ).toBe(false); + }); + await step("Refuse a state from another login", async () => { + const callback = new URL(await interceptCallback()); + callback.searchParams.set("state", "another-browser-state"); + const response = await page.request.get(callback.toString(), { maxRedirects: 0 }); + expect(response.status()).toBe(400); + expect(await response.text()).toBe("Invalid login state"); + expect( + (await page.context().cookies()).some((cookie) => cookie.name === "wos-session"), + ).toBe(false); + }); + await step("Complete a fresh login, then reject the same callback again", async () => { + const callback = await interceptCallback(); + await page.goto(callback); + await page.waitForURL((url) => url.pathname === "/create-org", { timeout: 30_000 }); + const cookies = await page.context().cookies(); + expect(cookies.some((cookie) => cookie.name === "wos-session")).toBe(true); + expect(cookies.some((cookie) => cookie.name === "wos-login-state")).toBe(false); + const me = await page.request.get(new URL("/api/auth/me", target.baseUrl).toString()); + expect(me.status()).toBe(200); + expect(await me.json()).toMatchObject({ user: { email } }); + const replay = await page.request.get(callback, { maxRedirects: 0 }); + expect(replay.status()).toBe(400); + expect(await replay.text()).toBe("Invalid login state"); + }); + }); + }), +); diff --git a/e2e/cloud/org-api-keys-console.test.ts b/e2e/cloud/org-api-keys-console.test.ts index 828dcb9afb..f4b34dafb8 100644 --- a/e2e/cloud/org-api-keys-console.test.ts +++ b/e2e/cloud/org-api-keys-console.test.ts @@ -110,7 +110,9 @@ scenario( .getByRole("heading", { name: "Revoke organization key" }) .waitFor({ state: "hidden", timeout: 30_000 }); - // The revoked value no longer authenticates. + // The dialog closes when revocation starts. Wait for the confirmed + // provider mutation before asserting the key no longer authenticates. + await page.getByText("Revoked e2e backend reader", { exact: true }).waitFor(); const after = await fetch(new URL("/api/admin/users", target.baseUrl), { headers: { authorization: `Bearer ${mintedValue}` }, }); From 65d939ebab6f77a00a3435fe3575399cd1cd3b7f Mon Sep 17 00:00:00 2001 From: Lloyd Vickery <115056532+LloydVickeryASI@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:15:36 +1200 Subject: [PATCH 12/25] Fix HubSpot optional scopes for workspace OAuth (#1898) * Fix HubSpot optional scopes for workspace OAuth * chore: rerun flaky cloud E2E * Honor integration-declared HubSpot optional scopes * Test queue timeout with a controlled clock * Verify optional OAuth scopes through consent and tool execution --------- Co-authored-by: Lloyd Vickery Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .../hubspot-workspace-optional-scopes.md | 5 + .../src/engine/first-party-oauth-clients.ts | 7 +- e2e/selfhost/oauth-optional-scopes.test.ts | 172 ++++++++++++++++++ packages/core/sdk/src/executor.ts | 10 +- packages/core/sdk/src/host-internal.ts | 6 +- packages/core/sdk/src/oauth-helpers.test.ts | 22 ++- packages/core/sdk/src/oauth-helpers.ts | 35 +++- .../core/sdk/src/oauth-scope-union.test.ts | 81 ++++++++- packages/core/sdk/src/oauth-service.ts | 30 ++- 9 files changed, 347 insertions(+), 21 deletions(-) create mode 100644 .changeset/hubspot-workspace-optional-scopes.md create mode 100644 e2e/selfhost/oauth-optional-scopes.test.ts diff --git a/.changeset/hubspot-workspace-optional-scopes.md b/.changeset/hubspot-workspace-optional-scopes.md new file mode 100644 index 0000000000..71af545f24 --- /dev/null +++ b/.changeset/hubspot-workspace-optional-scopes.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Send HubSpot optional permissions in `optional_scope` for workspace OAuth clients so accounts can connect without optional product features. diff --git a/apps/cloud/src/engine/first-party-oauth-clients.ts b/apps/cloud/src/engine/first-party-oauth-clients.ts index 9c646e5d58..aa1359c161 100644 --- a/apps/cloud/src/engine/first-party-oauth-clients.ts +++ b/apps/cloud/src/engine/first-party-oauth-clients.ts @@ -6,6 +6,7 @@ import { } from "@executor-js/plugin-openapi/providers/microsoft"; import { slackMcpUserScopes } from "@executor-js/react/lib/slack-mcp-oauth"; import { IntegrationSlug, type FirstPartyOAuthClientConfig } from "@executor-js/sdk"; +import { HUBSPOT_OPTIONAL_SCOPES } from "@executor-js/sdk/host-internal"; import { makeGoogleOAuthListing } from "../analytics/google-oauth-listing"; import { POSTHOG_INGEST_HOST } from "../edge/passthrough"; @@ -152,12 +153,6 @@ const HUBSPOT_REQUIRED_SCOPES = [ "timeline", ] as const; -const HUBSPOT_OPTIONAL_SCOPES = [ - "content", - "crm.objects.custom.read", - "crm.schemas.custom.read", -] as const; - const MICROSOFT_SCOPES = [ "User.Read", "Calendars.ReadWrite", diff --git a/e2e/selfhost/oauth-optional-scopes.test.ts b/e2e/selfhost/oauth-optional-scopes.test.ts new file mode 100644 index 0000000000..b14dac1274 --- /dev/null +++ b/e2e/selfhost/oauth-optional-scopes.test.ts @@ -0,0 +1,172 @@ +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { connectEmulator } from "@executor-js/emulate"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; + +import { createEmulatorInstance } from "../src/emulator-instance"; +import { scenario } from "../src/scenario"; +import { Api, Browser, Mcp, Target } from "../src/services"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +scenario( + "OAuth optional scopes · the integration partitions scopes and completes an authenticated connection", + { timeout: 180_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const mcp = yield* Mcp; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const session = mcp.session(identity); + expect((yield* session.call("execute", { code: "return true;" })).ok).toBe(true); + const base = yield* createEmulatorInstance("github", "optional-scopes"); + const emulator = yield* Effect.promise(() => + connectEmulator({ baseUrl: base, service: "github" }), + ); + yield* Effect.promise(() => emulator.seed({ users: [{ login: "optional-scope-user" }] })); + const slug = IntegrationSlug.make(`optional-${randomBytes(4).toString("hex")}`); + const app = OAuthClientSlug.make(`${slug}-app`); + yield* Effect.addFinalizer(() => + client.openapi.removeSpec({ params: { slug } }).pipe(Effect.orDie), + ); + yield* Effect.addFinalizer(() => + client.oauth + .removeClient({ params: { slug: app }, payload: { owner: "org" } }) + .pipe(Effect.orDie), + ); + // GitHub supplies the real OAuth transport and protected resource. The test + // verifies Executor's partitioning contract; it does not claim that GitHub + // implements HubSpot's optional-grant policy. + const authorizationUrl = `${base}/login/oauth/authorize`; + const tokenUrl = `${base}/login/oauth/access_token`; + yield* client.openapi.addSpec({ + payload: { + slug, + baseUrl: base, + spec: { + kind: "blob", + value: JSON.stringify({ + openapi: "3.0.3", + info: { title: "Optional scope API", version: "1" }, + paths: { + "/user": { + get: { + operationId: "getUser", + security: [{ oauth: ["read:user"] }], + responses: { "200": { description: "Authenticated user" } }, + }, + }, + }, + components: { + securitySchemes: { + oauth: { + type: "oauth2", + flows: { + authorizationCode: { + authorizationUrl, + tokenUrl, + scopes: { + "read:user": "Read user", + "user:email": "Read email when granted", + }, + }, + }, + }, + }, + }, + }), + }, + authenticationTemplate: [ + { + slug: "oauth", + kind: "oauth2", + authorizationUrl: `${authorizationUrl}?optional_scope=user%3Aemail`, + tokenUrl, + scopes: ["read:user", "user:email"], + }, + ], + }, + }); + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: app, + grant: "authorization_code", + authorizationUrl, + tokenUrl, + clientId: "optional-test-client", + clientSecret: "optional-test-secret", + originIntegration: slug, + }, + }); + const started = yield* client.oauth.start({ + payload: { + owner: "org", + client: app, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: slug, + template: AuthTemplateSlug.make("oauth"), + }, + }); + if (started.status !== "redirect") + return yield* Effect.die("Expected authorization redirect"); + + yield* Effect.addFinalizer(() => + client.oauth.cancel({ payload: { state: started.state } }).pipe(Effect.orDie), + ); + const url = new URL(started.authorizationUrl); + expect(url.searchParams.get("scope")).toBe("read:user"); + expect(url.searchParams.get("optional_scope")).toBe("user:email"); + yield* browser.session(identity, async ({ page, step }) => { + await step("Review and approve the OAuth consent request", async () => { + await page.goto(started.authorizationUrl); + await page.getByRole("button", { name: /optional-scope-user/ }).click(); + await page.getByText("Connected", { exact: true }).waitFor({ timeout: 30_000 }); + }); + }); + yield* Effect.addFinalizer(() => + client.connections + .remove({ + params: { owner: "org", integration: slug, name: ConnectionName.make("main") }, + }) + .pipe(Effect.orDie), + ); + const catalog = yield* client.tools.list({ query: { integration: slug } }); + const tool = catalog.find((entry) => entry.name.endsWith("getUser")); + if (!tool) + return yield* Effect.die( + `Authenticated getUser tool missing: ${catalog.map((entry) => entry.name).join(", ")}`, + ); + let result = yield* session.call("execute", { + code: `const path = ${JSON.stringify(String(tool.address))}.split(".").slice(1); let call = tools; for (const part of path) call = call[part]; return await call({});`, + }); + for (let attempts = 0; result.text.includes("executionId:") && attempts < 10; attempts += 1) + result = yield* session.approvePaused(result.text); + expect(result.ok).toBe(true); + expect(result.text).toContain("optional-scope-user"); + const ledger = yield* Effect.promise(() => emulator.ledger.list()); + const authorize = ledger.find( + (entry) => entry.method === "GET" && entry.path.endsWith("/login/oauth/authorize"), + ); + expect(new URLSearchParams(authorize?.query).get("optional_scope")).toBe("user:email"); + expect( + ledger + .filter((entry) => entry.path === "/user" && entry.response.status === 200) + .map((entry) => entry.identity.user?.login), + ).toContain("optional-scope-user"); + }), + ), +); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 451fc0f54b..22fb07ef65 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -208,6 +208,7 @@ import { exchangeClientCredentials, isPermanentTokenRejection, isUnusableSuccessTokenResponse, + optionalScopesFromAuthorizationUrl, shouldRefreshToken, type OAuth2TokenResponse, type OAuthEndpointUrlPolicy, @@ -6711,7 +6712,14 @@ export const createExecutor = { // buildAuthorizationUrl // --------------------------------------------------------------------------- -describe("providerAuthorizeExtras (Google offline/consent quirk)", () => { +describe("providerAuthorizeExtras (provider authorization quirks)", () => { it("adds access_type=offline + prompt=consent for the Google authorize host", () => { expect(providerAuthorizeExtras("https://accounts.google.com/o/oauth2/v2/auth")).toEqual({ access_type: "offline", prompt: "consent", }); }); - it("adds nothing for non-Google hosts or an unparseable URL (token host ≠ authorize host)", () => { + + it("adds optional_scope for workspace-owned HubSpot OAuth clients", () => { + expect(providerAuthorizeExtras("https://app.hubspot.com/oauth/authorize")).toEqual({ + optional_scope: "content crm.objects.custom.read crm.schemas.custom.read", + }); + }); + + it("reads integration-declared optional_scope values from an authorization URL", () => { + expect( + optionalScopesFromAuthorizationUrl( + "https://app.hubspot.com/oauth/authorize?optional_scope=crm.objects.contacts.read+crm.objects.contacts.write+crm.objects.contacts.read", + ), + ).toEqual(["crm.objects.contacts.read", "crm.objects.contacts.write"]); + expect(optionalScopesFromAuthorizationUrl("not a url")).toEqual([]); + }); + + it("adds nothing for unrelated hosts, token hosts, or an unparseable URL", () => { expect(providerAuthorizeExtras("https://accounts.spotify.com/authorize")).toEqual({}); expect(providerAuthorizeExtras("https://oauth2.googleapis.com/token")).toEqual({}); + expect(providerAuthorizeExtras("https://api.hubapi.com/oauth/v3/token")).toEqual({}); expect(providerAuthorizeExtras("not a url")).toEqual({}); }); }); diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index e40bdda068..c6e3209fe7 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -113,6 +113,16 @@ export const OAUTH2_REFRESH_SKEW_MS = 60_000; /** Default token-endpoint timeout. */ export const OAUTH2_DEFAULT_TIMEOUT_MS = 20_000; +/** HubSpot scopes that the registered app may grant but must receive through + * HubSpot's non-standard `optional_scope` authorize parameter. Keeping these + * out of the RFC `scope` parameter lets accounts without the corresponding + * product features complete consent while still granting them when present. */ +export const HUBSPOT_OPTIONAL_SCOPES = [ + "content", + "crm.objects.custom.read", + "crm.schemas.custom.read", +] as const; + /** RFC 8693 §2.1 token-exchange grant. */ export const TOKEN_EXCHANGE_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:token-exchange"; @@ -247,7 +257,12 @@ export const buildAuthorizationUrl = (input: BuildAuthorizationUrlInput): string * re-consent can silently keep the old scope set. Do not add * `include_granted_scopes=true` here: with historical grants on the same Google * consent app, Google folds those unrelated scopes into the new consent flow and - * can fail inside accounts.google.com before returning to our callback. */ + * can fail inside accounts.google.com before returning to our callback. + * + * HubSpot: app scopes marked optional are ignored when they are omitted from + * the provider-specific `optional_scope` parameter. The OpenAPI auth template + * can only declare RFC scopes, so this host-level quirk must apply to both + * first-party and workspace-owned HubSpot OAuth clients. */ export const providerAuthorizeExtras = ( authorizationUrl: string, ): Readonly> => { @@ -257,12 +272,30 @@ export const providerAuthorizeExtras = ( if (host === "accounts.google.com") { return { access_type: "offline", prompt: "consent" }; } + if (host === "app.hubspot.com") { + return { optional_scope: HUBSPOT_OPTIONAL_SCOPES.join(" ") }; + } } catch { // Unparseable authorization URL — let buildAuthorizationUrl surface the error. } return {}; }; +/** Provider-specific scopes embedded in an integration's authorization + * endpoint. HubSpot models app-optional permissions with the non-standard + * `optional_scope` query parameter, so they are part of the integration's + * request contract rather than the registered OAuth app identity. */ +export const optionalScopesFromAuthorizationUrl = (authorizationUrl: string): readonly string[] => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: URL() throws on invalid input -> no optional scopes + try { + const value = new URL(authorizationUrl).searchParams.get("optional_scope"); + if (value == null) return []; + return [...new Set(value.split(/\s+/).filter(Boolean))]; + } catch { + return []; + } +}; + // --------------------------------------------------------------------------- // Regional token-endpoint rebind // diff --git a/packages/core/sdk/src/oauth-scope-union.test.ts b/packages/core/sdk/src/oauth-scope-union.test.ts index 78f6d6df2b..47b92d16e0 100644 --- a/packages/core/sdk/src/oauth-scope-union.test.ts +++ b/packages/core/sdk/src/oauth-scope-union.test.ts @@ -37,7 +37,10 @@ const DECLARED_SCOPES = ["calendar", "gmail", "drive", "sheets"] as const; * scopes (the MCP/no-template-scopes case). */ const makeScopePluginWithId = ( id: TId, - config: { readonly scopes: readonly string[] | null }, + config: { + readonly scopes: readonly string[] | null; + readonly authorizationUrl?: string; + }, options: { readonly discoversScopes?: boolean; readonly discoveryUrl?: string } = {}, ) => definePlugin(() => ({ @@ -49,7 +52,10 @@ const makeScopePluginWithId = ( }), invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), describeAuthMethods: (record: IntegrationRecord): readonly AuthMethodDescriptor[] => { - const cfg = record.config as { readonly scopes?: readonly string[] | null } | null; + const cfg = record.config as { + readonly scopes?: readonly string[] | null; + readonly authorizationUrl?: string; + } | null; const scopes = cfg?.scopes; if (scopes == null) { // No declared oauth scopes. Server-targeting methods (MCP) expose a @@ -73,7 +79,12 @@ const makeScopePluginWithId = ( label: "OAuth2", kind: "oauth", template: String(TEMPLATE), - oauth: { scopes }, + oauth: { + scopes, + ...(cfg?.authorizationUrl === undefined + ? {} + : { authorizationUrl: cfg.authorizationUrl }), + }, }, ]; }, @@ -82,13 +93,20 @@ const makeScopePluginWithId = ( ctx.core.integrations.register({ slug: INTEG, description: "Acme", - config: { scopes: config.scopes }, + config: { + scopes: config.scopes, + ...(config.authorizationUrl === undefined + ? {} + : { authorizationUrl: config.authorizationUrl }), + }, }), }), }))(); -const makeScopePlugin = (config: { readonly scopes: readonly string[] | null }) => - makeScopePluginWithId("acme", config); +const makeScopePlugin = (config: { + readonly scopes: readonly string[] | null; + readonly authorizationUrl?: string; +}) => makeScopePluginWithId("acme", config); const makeMcpScopePlugin = (config: { readonly scopes: readonly string[] | null }) => makeScopePluginWithId("mcp", config, { discoversScopes: true }); @@ -226,6 +244,57 @@ describe("oauth.start integration-driven scopes", () => { ), ); + it.effect("moves integration-declared optional scopes out of scope into optional_scope", () => + Effect.scoped( + Effect.gen(function* () { + const declared = [ + "oauth", + "crm.objects.contacts.read", + "crm.objects.companies.read", + ] as const; + const optional = [ + "crm.objects.contacts.read", + "crm.objects.companies.read", + "content", + ] as const; + const server = yield* serveOAuthTestServer({ scopes: [...declared] }); + const plugins = [ + memoryCredentialsPlugin(), + makeScopePlugin({ + scopes: declared, + authorizationUrl: `${server.authorizationEndpoint}?optional_scope=${optional.join("+")}`, + }), + ] as const; + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + + const url = new URL(started.authorizationUrl); + expect(scopesFromAuthorizeUrl(started.authorizationUrl)).toEqual(["oauth"]); + expect(url.searchParams.get("optional_scope")?.split(/\s+/)).toEqual(optional); + }), + ), + ); + it.effect("filters stale declared scopes against authorization-server metadata", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index fdf727c38d..bbb85bc360 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -199,7 +199,14 @@ const startErrorFromEnterpriseManaged = (cause: EnterpriseManagedMintError): OAu * 8707 resource: a user may clear the client's resource (Entra v2 rejects * the parameter, #1789) without losing scope discovery. */ export type OAuthScopePolicy = - | { readonly kind: "scopes"; readonly scopes: readonly string[] } + | { + readonly kind: "scopes"; + readonly scopes: readonly string[]; + /** Provider-specific scopes declared on the integration's authorization + * endpoint (HubSpot `optional_scope`). These must not also be sent in + * the RFC `scope` parameter. */ + readonly optionalScopes?: readonly string[]; + } | { readonly kind: "discover"; readonly discoveryUrl: string }; /** Everything the OAuth service needs from the executor: fuma access for the @@ -2006,11 +2013,23 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { : scopePolicy.kind === "discover" ? requestedScopes : yield* filterAuthorizationCodeScopes(client, requestedScopes); + const providerExtras = providerAuthorizeExtras(client.authorizationUrl); + const workspaceOptionalScopes = firstPartyFlow + ? [] + : dedupeScopes([ + ...(providerExtras.optional_scope ?? "").split(/\s+/).filter(Boolean), + ...(scopePolicy.kind === "scopes" ? (scopePolicy.optionalScopes ?? []) : []), + ]); + const workspaceOptionalScopeSet = new Set(workspaceOptionalScopes); const completeAuthorizationScopes = dedupeScopes([ - ...authorizationRequestedScopes, + ...authorizationRequestedScopes.filter((scope) => !workspaceOptionalScopeSet.has(scope)), ...(firstParty?.additionalAuthorizationScopes ?? []), ...additionalAuthorizationLifecycleScopes(client), ]); + const completeRequestedScopes = dedupeScopes([ + ...completeAuthorizationScopes, + ...workspaceOptionalScopes, + ]); // authorization_code: persist a session + build the authorize URL. const verifier = createPkceCodeVerifier(); @@ -2072,7 +2091,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { payload: { owner: input.owner, clientOwner: input.clientOwner, - requestedScopes: completeAuthorizationScopes, + requestedScopes: completeRequestedScopes, }, expires_at: expiresAt, created_at: now, @@ -2094,7 +2113,10 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // without these Google returns no refresh token and won't re-consent // to widen scopes on reconnect. extraParams: { - ...providerAuthorizeExtras(client.authorizationUrl), + ...providerExtras, + ...(workspaceOptionalScopes.length > 0 + ? { optional_scope: workspaceOptionalScopes.join(" ") } + : {}), ...(firstParty?.authorizationExtraParams ?? {}), }, endpointUrlPolicy: deps.endpointUrlPolicy, From 1f67d83609b13a73d3dc8d630f48c8f54a02e6ca Mon Sep 17 00:00:00 2001 From: Alp Date: Mon, 14 Sep 2026 21:15:41 -0400 Subject: [PATCH 13/25] Support POST health checks for HTTP RPC APIs (#1952) * Support POST health checks for HTTP RPC APIs * Preserve existing health-check method behavior --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> --- .changeset/post-health-checks.md | 6 + e2e/scenarios/health-check-rpc-ui.test.ts | 238 ++++++++++++++++++ e2e/scenarios/health-checks.test.ts | 20 +- .../src/react/AddOpenApiIntegration.tsx | 19 +- packages/plugins/openapi/src/sdk/backing.ts | 25 +- packages/plugins/openapi/src/sdk/extract.ts | 2 + .../src/sdk/health-check-operation.test.ts | 63 +++++ .../openapi/src/sdk/health-check-operation.ts | 18 ++ packages/plugins/openapi/src/sdk/preview.ts | 27 +- .../react/src/components/accounts-section.tsx | 4 +- .../src/components/health-check-editor.tsx | 110 ++++---- .../react/src/lib/health-check-args.test.ts | 31 +++ packages/react/src/lib/health-check-args.ts | 30 +++ 13 files changed, 505 insertions(+), 88 deletions(-) create mode 100644 .changeset/post-health-checks.md create mode 100644 e2e/scenarios/health-check-rpc-ui.test.ts create mode 100644 packages/plugins/openapi/src/sdk/health-check-operation.test.ts create mode 100644 packages/plugins/openapi/src/sdk/health-check-operation.ts create mode 100644 packages/react/src/lib/health-check-args.test.ts create mode 100644 packages/react/src/lib/health-check-args.ts diff --git a/.changeset/post-health-checks.md b/.changeset/post-health-checks.md new file mode 100644 index 0000000000..21fd2d0281 --- /dev/null +++ b/.changeset/post-health-checks.md @@ -0,0 +1,6 @@ +--- +"@executor-js/plugin-openapi": patch +"@executor-js/react": patch +--- + +Support POST health checks for APIs that expose reads through HTTP RPC. Warn that POST can change data, allow validated JSON request bodies, and display the reason when a configured probe cannot run. diff --git a/e2e/scenarios/health-check-rpc-ui.test.ts b/e2e/scenarios/health-check-rpc-ui.test.ts new file mode 100644 index 0000000000..b6bf13df25 --- /dev/null +++ b/e2e/scenarios/health-check-rpc-ui.test.ts @@ -0,0 +1,238 @@ +import { randomBytes } from "node:crypto"; +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { connectEmulator } from "@executor-js/emulate"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/sdk/shared"; +import { variable } from "@executor-js/sdk/http-auth"; + +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([openApiHttpPlugin()] as const); +const template = AuthTemplateSlug.make("apiKey"); +const name = ConnectionName.make("test"); +const authenticationTemplate = [ + { + slug: template, + type: "apiKey" as const, + headers: { authorization: ["Bearer ", variable("token")] }, + }, +]; +const spec = (baseUrl: string) => + JSON.stringify({ + openapi: "3.0.3", + info: { title: "RPC account health", version: "1" }, + servers: [{ url: baseUrl }], + paths: { + "/api/auth.test": { + post: { + operationId: "getAccount", + requestBody: { + required: true, + content: { "application/json": { schema: { type: "object" } } }, + }, + responses: { "200": { description: "OK" } }, + }, + }, + "/account": { + delete: { operationId: "deleteAccount", responses: { "204": { description: "Deleted" } } }, + }, + }, + }); + +scenario( + "Health checks (UI) · configure and run a POST probe with a warning", + {}, + 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-rpc-${randomBytes(4).toString("hex")}`); + const baseUrl = yield* createEmulatorInstance("slack", "rpc-health"); + 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"); + const body = { include: { identity: true }, fields: ["user", "team"] }; + + yield* Effect.ensuring( + Effect.gen(function* () { + yield* client.openapi.addSpec({ + payload: { + slug, + baseUrl, + spec: { kind: "blob", value: spec(baseUrl) }, + authenticationTemplate, + }, + }); + yield* browser.session(identity, async ({ page, step }) => { + await step("Choose a POST health check and read its warning", async () => { + await visit(page, `/integrations/${slug}`); + await page.getByRole("button", { name: "Set up", exact: true }).click(); + await page + .getByRole("combobox", { name: "Operation", exact: true }) + .fill("getAccount"); + await page.getByRole("option", { name: /POST.*getAccount/ }).click(); + await page + .getByRole("alert") + .filter({ hasText: "POST requests can change data." }) + .waitFor(); + }); + await step("Reject malformed JSON before running or saving", async () => { + await page.getByRole("textbox", { name: "Request body (JSON)" }).fill('{"include":'); + expect( + await page.getByRole("button", { name: "Save", exact: true }).isEnabled(), + ).toBe(false); + await page.getByText("Enter a valid JSON request body.").waitFor(); + }); + await step("Preview the POST read with a JSON request body", async () => { + await page + .getByRole("textbox", { name: "Request body (JSON)" }) + .fill(JSON.stringify(body)); + await page.getByLabel("Test credential", { exact: true }).fill(token); + await page.getByRole("button", { name: "Preview", exact: true }).click(); + await page.getByText("Healthy", { exact: true }).waitFor(); + await page.getByText("Healthy", { exact: true }).scrollIntoViewIfNeeded(); + }); + await step("Save the health check and reopen its JSON body", async () => { + await page.getByRole("button", { name: "Save", exact: true }).click(); + await page.locator("#health-check-operation").waitFor({ state: "hidden" }); + await page.reload(); + const section = page.locator("section").filter({ + has: page.getByRole("heading", { name: "Health check", exact: true }), + }); + await section.getByRole("button", { name: "Edit", exact: true }).click(); + expect( + JSON.parse( + await page.getByRole("textbox", { name: "Request body (JSON)" }).inputValue(), + ), + ).toEqual(body); + await page + .getByRole("alert") + .filter({ hasText: "POST requests can change data." }) + .waitFor(); + }); + }); + const saved = yield* client.integrations.healthCheckGet({ params: { slug } }); + expect(saved?.args).toEqual({ body }); + yield* client.connections.create({ + payload: { owner: "org", integration: slug, name, template, value: token }, + }); + const result = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name }, + query: {}, + }); + expect(result.status).toBe("healthy"); + expect(result.httpStatus).toBe(200); + const requests = yield* Effect.promise(() => emulator.ledger.list()); + const probes = requests.filter((request) => request.path === "/api/auth.test"); + expect( + probes.length, + "preview and saved-connection check both reach the upstream", + ).toBeGreaterThanOrEqual(2); + for (const probe of probes) { + expect(probe.method).toBe("POST"); + expect(probe.request.body).toEqual(body); + expect(probe.response.status).toBe(200); + expect(probe.response.body).toMatchObject({ ok: true }); + expect(probe.sideEffects).toEqual([]); + } + }), + Effect.gen(function* () { + yield* client.connections + .remove({ params: { owner: "org", integration: slug, name } }) + .pipe(Effect.ignore); + yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); + yield* Effect.promise(() => emulator.reset()); + }), + ); + }), + ), +); + +scenario( + "Health checks (UI) · explain why an unsupported method cannot run", + {}, + 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-refused-${randomBytes(4).toString("hex")}`); + const baseUrl = "https://example.invalid"; + yield* Effect.ensuring( + Effect.gen(function* () { + yield* client.openapi.addSpec({ + payload: { + slug, + baseUrl, + spec: { kind: "blob", value: spec(baseUrl) }, + authenticationTemplate, + }, + }); + const candidates = yield* client.integrations.healthCheckCandidates({ params: { slug } }); + const mutation = candidates.find((candidate) => candidate.method === "delete"); + if (!mutation) return yield* Effect.die("Expected an unsupported DELETE operation"); + yield* client.integrations.healthCheckSet({ + params: { slug }, + payload: { spec: { operation: mutation.operation } }, + }); + yield* client.connections.create({ + payload: { + owner: "org", + integration: slug, + name, + template, + value: "test-only-credential", + }, + }); + const result = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name }, + query: {}, + }); + expect(result.status).toBe("unknown"); + expect(result.httpStatus).toBeUndefined(); + expect(result.detail).toContain("not supported for health checks"); + yield* browser.session(identity, async ({ page, step }) => { + await step("See why the configured health check could not run", async () => { + await visit(page, `/integrations/${slug}`); + await page.getByText(result.detail!, { exact: true }).waitFor(); + expect( + await page.getByText("No health check configured.", { exact: true }).count(), + ).toBe(0); + }); + await step("Edit the unsupported operation to see how to fix it", async () => { + const section = page.locator("section").filter({ + has: page.getByRole("heading", { name: "Health check", exact: true }), + }); + await section.getByRole("button", { name: "Edit", exact: true }).click(); + await page + .getByText("This method is not supported for health checks.", { exact: false }) + .waitFor(); + expect( + await page.getByRole("button", { name: "Save", exact: true }).isEnabled(), + ).toBe(false); + }); + }); + }), + Effect.gen(function* () { + yield* client.connections + .remove({ params: { owner: "org", integration: slug, name } }) + .pipe(Effect.ignore); + yield* client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore); + }), + ); + }), + ), +); diff --git a/e2e/scenarios/health-checks.test.ts b/e2e/scenarios/health-checks.test.ts index cb8adf53c0..c24a763ad2 100644 --- a/e2e/scenarios/health-checks.test.ts +++ b/e2e/scenarios/health-checks.test.ts @@ -70,6 +70,11 @@ const identitySpec = (baseUrl: string): string => summary: "Send a message", responses: { "201": { description: "created" } }, }, + delete: { + operationId: "deleteMessages", + summary: "Delete messages", + responses: { "204": { description: "deleted" } }, + }, }, }, }); @@ -486,14 +491,13 @@ scenario( Effect.gen(function* () { yield* registerIdentityIntegration(client, slug, server.url); - // Deliberately declare the DESTRUCTIVE POST as the health check (the - // editor warns but allows saving; the runtime is the enforcement). + // API configuration must not bypass the runtime's method restriction. const candidates = yield* client.integrations.healthCheckCandidates({ params: { slug } }); - const post = candidates.find((candidate) => candidate.method === "post"); - if (!post) return yield* Effect.die("identity spec exposed no POST candidate"); + const mutation = candidates.find((candidate) => candidate.method === "delete"); + if (!mutation) return yield* Effect.die("identity spec exposed no DELETE candidate"); yield* client.integrations.healthCheckSet({ params: { slug }, - payload: { spec: { operation: post.operation } }, + payload: { spec: { operation: mutation.operation } }, }); yield* client.connections.create({ @@ -508,13 +512,15 @@ scenario( // A health check runs unattended and repeatedly with no approval // gate, so the probe REFUSES to execute a mutating operation: the - // result is unknown-with-reason and the upstream never sees a POST. + // result is unknown-with-reason and the upstream never sees a DELETE. const result = yield* client.connections.checkHealth({ params: { owner: "org", integration: slug, name }, query: {}, }); expect(result.status, "a mutating probe refuses to run").toBe("unknown"); - expect(result.detail ?? "", "the refusal names the problem").toContain("mutating"); + expect(result.detail ?? "", "the refusal names the problem").toContain( + "not supported for health checks", + ); expect(result.httpStatus, "no request reached the upstream").toBeUndefined(); }), Effect.gen(function* () { diff --git a/packages/plugins/openapi/src/react/AddOpenApiIntegration.tsx b/packages/plugins/openapi/src/react/AddOpenApiIntegration.tsx index bb8e303a32..9fcb2de7fb 100644 --- a/packages/plugins/openapi/src/react/AddOpenApiIntegration.tsx +++ b/packages/plugins/openapi/src/react/AddOpenApiIntegration.tsx @@ -18,7 +18,11 @@ import { useIntegrationIdentity, } from "@executor-js/react/plugins/integration-identity"; import { Button } from "@executor-js/react/components/button"; -import { HealthCheckConfigFields } from "@executor-js/react/components/health-check-editor"; +import { + HealthCheckConfigFields, + isUnsupportedHealthCheck, + parseHealthCheckArgs, +} from "@executor-js/react/components/health-check-editor"; import { AuthMethodListEditor, useAuthMethodList, @@ -374,6 +378,7 @@ export default function AddOpenApiIntegration(props: { const hcMissingRequired = hcRequiredParams.some( (p) => (hcArgs[p.name] ?? "").trim().length === 0, ); + const hcParsedArgs = parseHealthCheckArgs(hcArgs); const onHcOperationChange = (next: string) => { setHcOperation(next); @@ -398,7 +403,10 @@ export default function AddOpenApiIntegration(props: { parsedSpecOverrides.ok && !slugAlreadyExists && (!previewHasNoServers || resolvedBaseUrl.length > 0) && - !(hcOperation.length > 0 && hcMissingRequired); + !( + hcOperation.length > 0 && + (hcMissingRequired || !hcParsedArgs.ok || isUnsupportedHealthCheck(hcSelected)) + ); // ---- Handlers ---- @@ -499,14 +507,11 @@ export default function AddOpenApiIntegration(props: { // the user (re-submitting the form hits the slug-already-exists guard). The // check stays editable from the integration's detail page, so on failure we // proceed to onComplete regardless and let the user fix it there. - if (hcOperation.length > 0) { + if (hcOperation.length > 0 && hcParsedArgs.ok) { const identity = hcIdentityField.trim(); - const argEntries = Object.entries(hcArgs) - .map(([key, value]) => [key, value.trim()] as const) - .filter(([, value]) => value.length > 0); const spec: HealthCheckSpec = { operation: hcOperation, - ...(argEntries.length > 0 ? { args: Object.fromEntries(argEntries) } : {}), + ...(Object.keys(hcParsedArgs.args).length > 0 ? { args: hcParsedArgs.args } : {}), ...(identity.length > 0 ? { identityField: identity } : {}), }; // Best-effort: the exit is intentionally ignored so a save failure cannot diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 58427c5f54..82cd6dcdba 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -53,6 +53,7 @@ import { parse, type ParsedDocument } from "./parse"; import { parseEntry, structuralSplit, type KeepPathItem, type SpecStructure } from "./split"; import { type OpenapiStore, type StoredOperation } from "./store"; import { OperationBinding } from "./types"; +import { getHealthCheckParameters } from "./health-check-operation"; const STRINGIFIED_BODY_CAP = 1024; const UpstreamMessageBody = Schema.Struct({ message: Schema.String }); @@ -942,16 +943,15 @@ export const checkHealthOpenApi = (input: { } satisfies HealthCheckResult; } - // HARD block, not just a ranking hint: a health check runs unattended and - // repeatedly, so a mutating operation must never execute through it. The - // normal tool path gates these behind approval, and this path has no - // approval step. The candidate list labels these "(writes)"; refusing here - // is the enforcement. - if (REQUIRE_APPROVAL.has(binding.method.toLowerCase())) { + // HTTP RPC reads can use POST; the editor warns users before enabling them. + if ( + REQUIRE_APPROVAL.has(binding.method.toLowerCase()) && + binding.method.toLowerCase() !== "post" + ) { return { status: "unknown", checkedAt, - detail: `Health check operation "${spec.operation}" is a ${binding.method.toUpperCase()} (mutating): pick a read-only operation.`, + detail: `Health check operation "${spec.operation}" uses ${binding.method.toUpperCase()} and is not supported for health checks. Pick a read-only operation.`, } satisfies HealthCheckResult; } @@ -1105,19 +1105,12 @@ export const listHealthCheckCandidatesOpenApi = (input: { const candidates = operations.map((op): HealthCheckCandidate => { const method = op.binding.method.toLowerCase(); - const parameters = op.binding.parameters.map((parameter) => ({ - name: parameter.name, - location: parameter.location, - required: parameter.required, - ...(Option.isSome(parameter.description) - ? { description: parameter.description.value } - : {}), - })); + const parameters = getHealthCheckParameters(op.binding); const responseFields = responseFieldsByTool.get(op.toolName); return { operation: op.toolName, method, - requiredArgCount: op.binding.parameters.filter((parameter) => parameter.required).length, + requiredArgCount: parameters.filter((parameter) => parameter.required).length, destructive: REQUIRE_APPROVAL.has(method), summary: summaries.get(op.toolName) ?? `${method.toUpperCase()} ${op.binding.pathTemplate}`, ...(parameters.length > 0 ? { parameters } : {}), diff --git a/packages/plugins/openapi/src/sdk/extract.ts b/packages/plugins/openapi/src/sdk/extract.ts index c242160a18..c11c5e0f99 100644 --- a/packages/plugins/openapi/src/sdk/extract.ts +++ b/packages/plugins/openapi/src/sdk/extract.ts @@ -976,6 +976,7 @@ export interface StreamedPreviewParameter { * add screen's operation list and health-check candidate ranking need, and * nothing that scales with schema size. */ export interface StreamedPreviewOperation { + readonly requestBodyRequired?: boolean; readonly operationId: string; /** Tool path planned over the full kept operation set, so preview candidates * match the names registration will assign. */ @@ -1050,6 +1051,7 @@ export const streamPreviewOperations = ( }), ); metas.push({ + requestBodyRequired: extractRequestBody(operation, r)?.required, operationId, method, pathTemplate: resolvedPathTemplate, diff --git a/packages/plugins/openapi/src/sdk/health-check-operation.test.ts b/packages/plugins/openapi/src/sdk/health-check-operation.test.ts new file mode 100644 index 0000000000..b478da2554 --- /dev/null +++ b/packages/plugins/openapi/src/sdk/health-check-operation.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; +import { dump } from "js-yaml"; + +import { previewSpecText, previewSpecTextStreaming } from "./preview"; + +const readOperation = { + operationId: "getAccount", + parameters: [ + { + name: "RPC-Version", + in: "header", + required: true, + schema: { type: "string" }, + }, + ], + responses: { "200": { description: "OK" } }, +}; + +describe("health-check operations", () => { + it.effect("keeps body requirements and POST risk visible in both preview paths", () => + Effect.gen(function* () { + for (const required of [true, false]) { + const specText = dump({ + openapi: "3.0.3", + info: { title: "Account RPC API", version: "1" }, + servers: [{ url: "https://api.example.test" }], + paths: { + "/rpc/getAccount": { + post: { + ...readOperation, + requestBody: { + required, + content: { "application/json": { schema: { type: "object" } } }, + }, + }, + }, + "/me": { + get: { + operationId: "getMe", + responses: { "200": { description: "OK" } }, + }, + }, + }, + }); + const whole = yield* previewSpecText(specText); + const streamed = yield* previewSpecTextStreaming(specText); + expect(streamed.healthCheckCandidates).toEqual(whole.healthCheckCandidates); + expect(whole.healthCheckCandidates[0]?.method).toBe("get"); + expect( + whole.healthCheckCandidates.find((candidate) => candidate.method === "post"), + ).toMatchObject({ + destructive: true, + requiredArgCount: required ? 2 : 1, + parameters: [ + { name: "RPC-Version", location: "header", required: true }, + { name: "body", location: "body", required }, + ], + }); + } + }), + ); +}); diff --git a/packages/plugins/openapi/src/sdk/health-check-operation.ts b/packages/plugins/openapi/src/sdk/health-check-operation.ts new file mode 100644 index 0000000000..f238665858 --- /dev/null +++ b/packages/plugins/openapi/src/sdk/health-check-operation.ts @@ -0,0 +1,18 @@ +import { Option } from "effect"; +import type { HealthCheckCandidateParameter } from "@executor-js/sdk/core"; + +import type { OperationBinding } from "./types"; + +export const getHealthCheckParameters = ( + operation: Pick, +): HealthCheckCandidateParameter[] => [ + ...operation.parameters.map((parameter) => ({ + name: parameter.name, + location: parameter.location, + required: parameter.required, + ...(Option.isSome(parameter.description) ? { description: parameter.description.value } : {}), + })), + ...(Option.isSome(operation.requestBody) + ? [{ name: "body", location: "body", required: operation.requestBody.value.required }] + : []), +]; diff --git a/packages/plugins/openapi/src/sdk/preview.ts b/packages/plugins/openapi/src/sdk/preview.ts index 51279af8f8..84506ad475 100644 --- a/packages/plugins/openapi/src/sdk/preview.ts +++ b/packages/plugins/openapi/src/sdk/preview.ts @@ -25,10 +25,10 @@ import { type KeepPathItem, } from "./split"; import { HttpMethod, ServerInfo, type ExtractedOperation, type ExtractionResult } from "./types"; +import { getHealthCheckParameters } from "./health-check-operation"; -// Mutating HTTP methods: mirrors `REQUIRE_APPROVAL` in `./invoke` but kept -// inline so this browser-safe preview module never pulls in the HTTP execution -// path. A health check should be safe to re-run, so these rank last. +// Keep potentially mutating methods ranked below ordinary reads. This mirrors +// REQUIRE_APPROVAL without importing the HTTP execution path into the browser. const DESTRUCTIVE_METHODS = new Set(["post", "put", "patch", "delete"]); // Cap on health-check candidate METADATA carried in the preview, so the add @@ -487,18 +487,11 @@ const buildPreviewHealthCheckCandidates = ( .map((def): HealthCheckCandidate => { const op = def.operation; const method = op.method.toLowerCase(); - const parameters = op.parameters.map((parameter) => ({ - name: parameter.name, - location: parameter.location, - required: parameter.required, - ...(Option.isSome(parameter.description) - ? { description: parameter.description.value } - : {}), - })); + const parameters = getHealthCheckParameters(op); return { operation: def.toolPath, method, - requiredArgCount: op.parameters.filter((parameter) => parameter.required).length, + requiredArgCount: parameters.filter((parameter) => parameter.required).length, destructive: DESTRUCTIVE_METHODS.has(method), summary: Option.getOrUndefined(op.summary) ?? @@ -590,13 +583,19 @@ export const previewSpecText = Effect.fn("OpenApi.previewSpecText")(function* (s const streamedCandidate = (op: StreamedPreviewOperation): HealthCheckCandidate => { const method = op.method.toLowerCase(); + const parameters = [ + ...op.parameters, + ...(op.requestBodyRequired === undefined + ? [] + : [{ name: "body", location: "body", required: op.requestBodyRequired }]), + ]; return { operation: op.toolPath, method, - requiredArgCount: op.parameters.filter((parameter) => parameter.required).length, + requiredArgCount: parameters.filter((parameter) => parameter.required).length, destructive: DESTRUCTIVE_METHODS.has(method), summary: op.summary ?? op.description ?? `${method.toUpperCase()} ${op.pathTemplate}`, - ...(op.parameters.length > 0 ? { parameters: op.parameters } : {}), + ...(parameters.length > 0 ? { parameters } : {}), }; }; diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index a87332e654..b87b5d8229 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -175,7 +175,7 @@ function AccountRow(props: { // below, because the remediation is a console visit, not a reconnect. const misconfigured = status === "misconfigured"; const needsHealthAttention = status === "expired" || status === "degraded"; - const healthDetail = needsHealthAttention ? probe?.detail : undefined; + const healthDetail = needsHealthAttention || status === "unknown" ? probe?.detail : undefined; const missingOAuthScopes = connection.missingOAuthScopes ?? []; const handleCheck = async () => { @@ -204,7 +204,7 @@ function AccountRow(props: { } else if (exit.value.status === "degraded") { toast.warning(exit.value.detail ?? "Connection check returned an error"); } else { - toast.message("No health check is configured for this integration"); + toast.message(exit.value.detail ?? "Health check did not run"); } }; diff --git a/packages/react/src/components/health-check-editor.tsx b/packages/react/src/components/health-check-editor.tsx index ea6076fb6e..f3c41257d5 100644 --- a/packages/react/src/components/health-check-editor.tsx +++ b/packages/react/src/components/health-check-editor.tsx @@ -22,9 +22,11 @@ import { import { healthCheckWriteKeys } from "../api/reactivity-keys"; import { messageFromExit } from "../api/error-reporting"; import { HEALTH_STATUS_LABEL, HEALTH_TEXT_CLASS } from "../lib/health-display"; +import { formatHealthCheckArgs, parseHealthCheckArgs } from "../lib/health-check-args"; import { Button } from "./button"; import { FreeformCombobox, type FreeformComboboxOption } from "./combobox"; import { Input } from "./input"; +import { Textarea } from "./textarea"; import { Label } from "./label"; import { NativeSelect, NativeSelectOption } from "./native-select"; import { @@ -70,11 +72,14 @@ export interface HealthCheckLivePreview { }>; } -/** "GET /users/me" style label for a candidate, with a writes marker so a - * mutating operation picked as a health check reads as the hazard it is. */ +export const isUnsupportedHealthCheck = (candidate: HealthCheckCandidate | null): boolean => + candidate?.destructive === true && candidate.method.toLowerCase() !== "post"; + +/** POST may be a read operation in an HTTP RPC API, but needs a warning. */ const candidateLabel = (candidate: HealthCheckCandidate): string => { const head = `${candidate.method.toUpperCase()} ${candidate.operation}`; - return candidate.destructive ? `${head} (writes)` : head; + if (candidate.method.toLowerCase() === "post") return `${head} (may change data)`; + return isUnsupportedHealthCheck(candidate) ? `${head} (unsupported for health checks)` : head; }; /** The summary line for the configured spec: the operation, prefixed with its @@ -137,8 +142,8 @@ function HealthCheckConfigFields(props: { [selected], ); - const requiredParams = useMemo( - () => (selected?.parameters ?? []).filter((p) => p.required), + const pinnedParams = useMemo( + () => (selected?.parameters ?? []).filter((p) => p.required || p.location === "body"), [selected], ); @@ -158,40 +163,64 @@ function HealthCheckConfigFields(props: { {selected?.summary ? (

{selected.summary}

) : null} - {selected?.destructive ? ( + {selected?.method.toLowerCase() === "post" ? ( +

+ POST requests can change data. Health checks run automatically and repeatedly; choose an + operation that only reads data. +

+ ) : isUnsupportedHealthCheck(selected) ? (

- This operation writes data. Prefer a read-only (GET) operation for a health check. + This method is not supported for health checks. Pick a read-only operation.

) : null} - {requiredParams.length > 0 ? ( + {pinnedParams.length > 0 ? (
-

Required arguments

+

Pinned arguments

Pinned into every probe. An identity endpoint often needs a fixed value here (for example resourceName ={" "} people/me).

- {requiredParams.map((param) => ( + {pinnedParams.map((param) => (
- ) => - props.onArgChange(param.name, e.target.value) - } - placeholder={param.description ?? `Value for ${param.name}`} - disabled={disabled} - /> + {param.location === "body" ? ( + <> +