From 0f43215c0e14bad2f228e149c0d762fd3658347b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 02:08:13 -0700 Subject: [PATCH 1/2] fix(mcp): keep a legacy server row from blanking the whole server list The server-list response validated transport/authType/connectionStatus with strict enums, but those columns are free text and can hold legacy values (e.g. transport 'http'/'sse' from before the streamable-http consolidation, including rows copied verbatim by workspace fork). Since the client strict- parses the whole array with retry:false, a single off-enum row blanked the entire workspace's MCP list. - Tolerate off-enum values in the response via Zod .catch() (request bodies stay strict): transport -> streamable-http, authType/connectionStatus -> undefined - Stop the workspace-fork copy from propagating legacy transport values - Reset connection status on a headers->OAuth flip in the create/upsert path, mirroring the update path (kills a false "connected" state) - Drop the positive tool cache on OAuth-pending in bulk discovery, matching the single-server path (no stale tools after re-auth is required) --- .../lib/copy/copy-resources.ts | 3 +++ apps/sim/lib/api/contracts/mcp.ts | 23 ++++++++++++++++--- .../lib/mcp/orchestration/server-lifecycle.ts | 11 +++++++-- apps/sim/lib/mcp/service.ts | 13 +++++++++-- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts index 47e8315be70..2c7eda693b5 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts @@ -395,6 +395,9 @@ export async function copyForkResourceContainers( createdBy: userId, url: typeof row.url === 'string' ? rewriteEnv(row.url) : row.url, headers, + // Normalize legacy `http`/`sse` transports to the only supported value so a + // forked row never carries a transport the API contract would reject. + transport: 'streamable-http', connectionStatus: 'disconnected', lastConnected: null, lastError: null, diff --git a/apps/sim/lib/api/contracts/mcp.ts b/apps/sim/lib/api/contracts/mcp.ts index b41cff74d95..a8fed5369ee 100644 --- a/apps/sim/lib/api/contracts/mcp.ts +++ b/apps/sim/lib/api/contracts/mcp.ts @@ -26,7 +26,12 @@ const optionalNumberFromNullableSchema = z.preprocess( const optionalConnectionStatusFromNullableSchema = z.preprocess( (value) => (value === null ? undefined : value), - z.enum(['connected', 'disconnected', 'error']).optional() + // `connection_status` is a free-text column; tolerate an off-enum value as undefined + // rather than failing the whole list's validation. + z + .enum(['connected', 'disconnected', 'error']) + .optional() + .catch(undefined) ) const optionalHeadersFromNullableSchema = z.preprocess( @@ -36,6 +41,16 @@ const optionalHeadersFromNullableSchema = z.preprocess( export const mcpTransportSchema = z.enum(['streamable-http']) +/** + * Transport as read back from storage. The `transport` column is free text, and + * rows predating the Streamable HTTP consolidation (or copied verbatim by an + * older fork) may still hold legacy `http`/`sse` values. Every server is operated + * over Streamable HTTP regardless, so any non-canonical value normalizes to the + * supported transport — this stops a single legacy row from failing the entire + * server list's response validation. + */ +const mcpTransportResponseSchema = mcpTransportSchema.catch('streamable-http') + export const mcpAuthTypeSchema = z.enum(['none', 'headers', 'oauth']) const consecutiveFailuresSchema = z.preprocess( @@ -98,8 +113,10 @@ export const mcpServerSchema = z workspaceId: z.string(), name: z.string(), description: optionalStringFromNullableSchema, - transport: mcpTransportSchema, - authType: mcpAuthTypeSchema.optional(), + transport: mcpTransportResponseSchema, + // Response-side tolerance: `auth_type` is a free-text column, so a value outside + // the enum normalizes to undefined rather than failing the whole list's validation. + authType: mcpAuthTypeSchema.optional().catch(undefined), url: optionalStringFromNullableSchema, timeout: optionalNumberFromNullableSchema, retries: optionalNumberFromNullableSchema, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index 75c3071ee9e..520af57301c 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -169,7 +169,10 @@ export async function performCreateMcpServer( currentEncryptedClientSecret: existingServer.oauthClientSecret, }) const isRevival = existingServer.deletedAt !== null - const shouldClearOauth = urlChanged || credsChanged || isRevival + const authTypeChanged = existingServer.authType !== resolvedAuthType + // Turning OAuth off orphans its tokens; revoke and delete them, mirroring the update path. + const oauthDisabled = existingServer.authType === 'oauth' && resolvedAuthType !== 'oauth' + const shouldClearOauth = urlChanged || credsChanged || isRevival || oauthDisabled if (shouldClearOauth) await revokeMcpOauthTokens(serverId) @@ -191,9 +194,13 @@ export async function performCreateMcpServer( deletedAt: null, } if (resolvedAuthType === 'oauth') { - if (shouldClearOauth) { + // A flip to OAuth (headers→OAuth carries no tokens to clear) or an OAuth URL/creds + // change leaves no valid session: reset to disconnected so the UI shows + // "authorization required" instead of a stale connected state until the flow completes. + if (authTypeChanged || shouldClearOauth) { updateValues.connectionStatus = 'disconnected' updateValues.lastConnected = null + updateValues.lastError = null } } else { updateValues.connectionStatus = 'connected' diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index dfa4d1b7e5a..e95d3269437 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -760,11 +760,20 @@ class McpService { return } if (outcome.kind === 'oauth-pending') { - // Mark disconnected so the UI surfaces the re-auth button. + // Mark disconnected so the UI surfaces the re-auth button, and drop the positive + // tool cache so a follow-up force-refresh can't serve tools for a server that now + // needs re-auth (mirrors the single-server discovery path). logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) deferredSideEffects.push( this.markServerOauthPending(server.id, workspaceId, discoveryStartedAt).then( - () => undefined + async (statusApplied) => { + if (!statusApplied) return + await this.cacheAdapter + .delete(serverCacheKey(workspaceId, server.id)) + .catch((err) => + logger.warn(`[${requestId}] Cache delete failed for ${server.name}:`, err) + ) + } ) ) return From 40a5552411f56fb3eae2025f744db0646a96b2b0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 02:20:40 -0700 Subject: [PATCH 2/2] fix(mcp): reset connection status on any auth-type flip in the create/upsert path The non-OAuth branch optimistically marked the server connected on an oauth->headers flip, leaving a stale lastError and diverging from performUpdateMcpServer. Mirror the update path exactly: reset to disconnected and clear lastError on an auth-type flip or OAuth URL/creds change; only mark connected for a non-OAuth (re-)registration with unchanged auth. Adds a create-path regression test. --- .../orchestration/server-lifecycle.test.ts | 59 ++++++++++++++++--- .../lib/mcp/orchestration/server-lifecycle.ts | 20 +++---- 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts index a1861c1ad63..8b11a5ec4f7 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts @@ -14,13 +14,19 @@ import { } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockClearCache, mockOauthCredsChanged, mockRevokeOauthTokens, mockEvictServerConnections } = - vi.hoisted(() => ({ - mockClearCache: vi.fn(), - mockOauthCredsChanged: vi.fn(), - mockRevokeOauthTokens: vi.fn(), - mockEvictServerConnections: vi.fn(), - })) +const { + mockClearCache, + mockOauthCredsChanged, + mockRevokeOauthTokens, + mockEvictServerConnections, + mockGenerateMcpServerId, +} = vi.hoisted(() => ({ + mockClearCache: vi.fn(), + mockOauthCredsChanged: vi.fn(), + mockRevokeOauthTokens: vi.fn(), + mockEvictServerConnections: vi.fn(), + mockGenerateMcpServerId: vi.fn(), +})) vi.mock('@sim/audit', () => auditMock) vi.mock('@sim/db', () => ({ @@ -52,10 +58,11 @@ vi.mock('@/lib/mcp/service', () => ({ evictServerConnections: mockEvictServerConnections, }, })) -vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: vi.fn() })) +vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: mockGenerateMcpServerId })) vi.mock('@/lib/posthog/server', () => posthogServerMock) import { + performCreateMcpServer, performDeleteMcpServer, performUpdateMcpServer, } from '@/lib/mcp/orchestration/server-lifecycle' @@ -149,6 +156,42 @@ describe('MCP server lifecycle orchestration', () => { expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1') }) + it('resets to disconnected when a create/upsert flips an existing OAuth server to headers', async () => { + mockGenerateMcpServerId.mockReturnValue('server-1') + dbChainMockFns.limit.mockResolvedValueOnce([ + { + id: 'server-1', + deletedAt: null, + url: 'https://example.com/mcp', + authType: 'oauth', + oauthClientId: 'client-1', + oauthClientSecret: 'secret-1', + }, + ]) + + const result = await performCreateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + name: 'Example', + url: 'https://example.com/mcp', + authType: 'headers', + }) + + expect(result.success).toBe(true) + // Upsert must mirror the update path: an auth-type flip resets to disconnected and clears the + // stale error instead of optimistically marking the server connected. + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ + authType: 'headers', + connectionStatus: 'disconnected', + lastConnected: null, + lastError: null, + }) + ) + // ...and revoke the now-orphaned OAuth tokens. + expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1') + }) + it('evicts the deleted server from the connection pool (row is already gone from clearCache)', async () => { dbChainMockFns.returning.mockResolvedValueOnce([ { id: 'server-1', workspaceId: 'workspace-1', name: 'Example', transport: 'streamable-http' }, diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index 520af57301c..2468900e386 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -193,16 +193,16 @@ export async function performCreateMcpServer( updatedAt: new Date(), deletedAt: null, } - if (resolvedAuthType === 'oauth') { - // A flip to OAuth (headers→OAuth carries no tokens to clear) or an OAuth URL/creds - // change leaves no valid session: reset to disconnected so the UI shows - // "authorization required" instead of a stale connected state until the flow completes. - if (authTypeChanged || shouldClearOauth) { - updateValues.connectionStatus = 'disconnected' - updateValues.lastConnected = null - updateValues.lastError = null - } - } else { + if (authTypeChanged || (shouldClearOauth && resolvedAuthType === 'oauth')) { + // An auth-type flip, or an OAuth URL/creds change, invalidates any prior connection: + // reset to disconnected and clear the stale error so the UI never shows + // connected-with-error until re-discovery. Mirrors performUpdateMcpServer. + updateValues.connectionStatus = 'disconnected' + updateValues.lastConnected = null + updateValues.lastError = null + } else if (resolvedAuthType !== 'oauth') { + // A non-OAuth (re-)registration with unchanged auth optimistically marks the server + // reachable; discovery corrects it if the endpoint is unhealthy. updateValues.connectionStatus = 'connected' updateValues.lastConnected = new Date() }