Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
23 changes: 20 additions & 3 deletions apps/sim/lib/api/contracts/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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,
Expand Down
59 changes: 51 additions & 8 deletions apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => ({
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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' },
Expand Down
21 changes: 14 additions & 7 deletions apps/sim/lib/mcp/orchestration/server-lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -190,12 +193,16 @@ export async function performCreateMcpServer(
updatedAt: new Date(),
deletedAt: null,
}
if (resolvedAuthType === 'oauth') {
if (shouldClearOauth) {
updateValues.connectionStatus = 'disconnected'
updateValues.lastConnected = 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()
}
Expand Down
13 changes: 11 additions & 2 deletions apps/sim/lib/mcp/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading