diff --git a/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.test.ts b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.test.ts new file mode 100644 index 00000000000..a1dc035faf2 --- /dev/null +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.test.ts @@ -0,0 +1,289 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCreateMcpServer, + mockDeleteMcpServer, + mockUpdateMcpServer, + mockVerifyServerConnection, +} = vi.hoisted(() => ({ + mockCreateMcpServer: vi.fn(), + mockDeleteMcpServer: vi.fn(), + mockUpdateMcpServer: vi.fn(), + mockVerifyServerConnection: vi.fn(), +})) + +vi.mock('@sim/db', () => ({ db: {} })) + +vi.mock('@/lib/mcp/orchestration', () => ({ + performCreateMcpServer: mockCreateMcpServer, + performDeleteMcpServer: mockDeleteMcpServer, + performUpdateMcpServer: mockUpdateMcpServer, +})) + +vi.mock('@/lib/mcp/service', () => ({ + mcpService: { verifyServerConnection: mockVerifyServerConnection }, +})) + +import type { ExecutionContext } from '@/lib/copilot/request/types' +import { executeManageMcpTool } from './manage-mcp-tool' + +const context = { + workspaceId: 'workspace-1', + userId: 'user-1', + userPermission: 'write', +} as ExecutionContext + +describe('executeManageMcpTool verification', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('verifies a newly added server before returning it to the agent', async () => { + mockCreateMcpServer.mockResolvedValue({ + success: true, + serverId: 'mcp-1', + updated: false, + authType: 'headers', + }) + mockVerifyServerConnection.mockResolvedValue({ + verified: true, + toolCount: 3, + requiresAuthorization: false, + }) + + const result = await executeManageMcpTool( + { + operation: 'add', + config: { name: 'Memory', url: 'https://memory.example.com/mcp' }, + }, + context + ) + + expect(mockVerifyServerConnection).toHaveBeenCalledWith('user-1', 'mcp-1', 'workspace-1') + expect(result).toEqual( + expect.objectContaining({ + success: true, + output: expect.objectContaining({ + serverId: 'mcp-1', + verification: { + verified: true, + toolCount: 3, + requiresAuthorization: false, + }, + }), + }) + ) + }) + + it('retains a created server and reports a failed verification', async () => { + mockCreateMcpServer.mockResolvedValue({ + success: true, + serverId: 'mcp-1', + updated: false, + authType: 'headers', + }) + mockVerifyServerConnection.mockResolvedValue({ + verified: false, + toolCount: 0, + requiresAuthorization: false, + error: 'Connection failed', + }) + + const result = await executeManageMcpTool( + { + operation: 'add', + config: { name: 'Memory', url: 'https://memory.example.com/mcp' }, + }, + context + ) + + expect(result.success).toBe(true) + expect(result.output).toEqual( + expect.objectContaining({ + verification: expect.objectContaining({ + verified: false, + error: 'Connection failed', + }), + }) + ) + }) + + it('skips verification when a newly added server is disabled', async () => { + mockCreateMcpServer.mockResolvedValue({ + success: true, + serverId: 'mcp-1', + updated: false, + authType: 'headers', + }) + + const result = await executeManageMcpTool( + { + operation: 'add', + config: { + name: 'Memory', + url: 'https://memory.example.com/mcp', + enabled: false, + }, + }, + context + ) + + expect(mockVerifyServerConnection).not.toHaveBeenCalled() + expect(result.output).toEqual( + expect.objectContaining({ + verification: { + verified: false, + toolCount: 0, + requiresAuthorization: false, + skipped: true, + reason: 'server_disabled', + }, + }) + ) + }) + + it('re-verifies a server after editing its connection config', async () => { + mockUpdateMcpServer.mockResolvedValue({ + success: true, + server: { id: 'mcp-1', name: 'Memory', enabled: true }, + connectionChanged: true, + }) + mockVerifyServerConnection.mockResolvedValue({ + verified: true, + toolCount: 2, + requiresAuthorization: false, + }) + + const result = await executeManageMcpTool( + { + operation: 'edit', + serverId: 'mcp-1', + config: { headers: { 'X-API-Key': '{{MEMORY_KEY}}' } }, + }, + context + ) + + expect(mockVerifyServerConnection).toHaveBeenCalledWith('user-1', 'mcp-1', 'workspace-1') + expect(result.output).toEqual( + expect.objectContaining({ + verification: expect.objectContaining({ verified: true, toolCount: 2 }), + }) + ) + }) + + it('skips verification after a cosmetic-only edit', async () => { + mockUpdateMcpServer.mockResolvedValue({ + success: true, + server: { id: 'mcp-1', name: 'Renamed Memory', enabled: true }, + connectionChanged: false, + }) + + const result = await executeManageMcpTool( + { + operation: 'edit', + serverId: 'mcp-1', + config: { name: 'Renamed Memory' }, + }, + context + ) + + expect(mockVerifyServerConnection).not.toHaveBeenCalled() + expect(result.output).toEqual( + expect.objectContaining({ + verification: expect.objectContaining({ + verified: false, + skipped: true, + reason: 'connection_unchanged', + }), + }) + ) + }) + + it('skips verification when an edit echoes unchanged connection config', async () => { + mockUpdateMcpServer.mockResolvedValue({ + success: true, + server: { id: 'mcp-1', name: 'Renamed Memory', enabled: true }, + connectionChanged: false, + }) + + const result = await executeManageMcpTool( + { + operation: 'edit', + serverId: 'mcp-1', + config: { + name: 'Renamed Memory', + url: 'https://memory.example.com/mcp', + transport: 'streamable-http', + headers: { Authorization: 'Bearer {{MEMORY_KEY}}' }, + timeout: 30000, + enabled: true, + }, + }, + context + ) + + expect(mockVerifyServerConnection).not.toHaveBeenCalled() + expect(result.output).toEqual( + expect.objectContaining({ + verification: expect.objectContaining({ + skipped: true, + reason: 'connection_unchanged', + }), + }) + ) + }) + + it('re-verifies a server when an edit re-enables it', async () => { + mockUpdateMcpServer.mockResolvedValue({ + success: true, + server: { id: 'mcp-1', name: 'Memory', enabled: true }, + connectionChanged: true, + }) + mockVerifyServerConnection.mockResolvedValue({ + verified: true, + toolCount: 2, + requiresAuthorization: false, + }) + + await executeManageMcpTool( + { + operation: 'edit', + serverId: 'mcp-1', + config: { enabled: true }, + }, + context + ) + + expect(mockVerifyServerConnection).toHaveBeenCalledWith('user-1', 'mcp-1', 'workspace-1') + }) + + it('skips verification when the edited server is disabled', async () => { + mockUpdateMcpServer.mockResolvedValue({ + success: true, + server: { id: 'mcp-1', name: 'Memory', enabled: false }, + connectionChanged: true, + }) + + const result = await executeManageMcpTool( + { + operation: 'edit', + serverId: 'mcp-1', + config: { url: 'https://new-memory.example.com/mcp', enabled: false }, + }, + context + ) + + expect(mockVerifyServerConnection).not.toHaveBeenCalled() + expect(result.output).toEqual( + expect.objectContaining({ + verification: expect.objectContaining({ + skipped: true, + reason: 'server_disabled', + }), + }) + ) + }) +}) diff --git a/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts index a6b571c0441..cb798843432 100644 --- a/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.ts @@ -9,6 +9,8 @@ import { performDeleteMcpServer, performUpdateMcpServer, } from '@/lib/mcp/orchestration' +import { mcpService } from '@/lib/mcp/service' +import type { McpServerVerificationResult } from '@/lib/mcp/types' const logger = createLogger('CopilotToolExecutor') @@ -29,6 +31,18 @@ interface ManageMcpToolParams { config?: ManageMcpToolConfig } +function skippedVerification( + reason: 'server_disabled' | 'connection_unchanged' +): McpServerVerificationResult { + return { + verified: false, + toolCount: 0, + requiresAuthorization: false, + skipped: true, + reason, + } +} + export async function executeManageMcpTool( rawParams: Record, context: ExecutionContext @@ -109,6 +123,11 @@ export async function executeManageMcpTool( } } + const verification = + config.enabled === false + ? skippedVerification('server_disabled') + : await mcpService.verifyServerConnection(context.userId, result.serverId, workspaceId) + return { success: true, output: { @@ -119,6 +138,7 @@ export async function executeManageMcpTool( message: result.updated ? `Updated existing MCP server "${config.name}"` : `Added MCP server "${config.name}"`, + verification, }, } } @@ -147,6 +167,12 @@ export async function executeManageMcpTool( return { success: false, error: `MCP server not found: ${params.serverId}` } } + const verification = !result.server.enabled + ? skippedVerification('server_disabled') + : result.connectionChanged + ? await mcpService.verifyServerConnection(context.userId, params.serverId, workspaceId) + : skippedVerification('connection_unchanged') + return { success: true, output: { @@ -155,6 +181,7 @@ export async function executeManageMcpTool( serverId: params.serverId, name: result.server.name, message: `Updated MCP server "${result.server.name}"`, + verification, }, } } diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts new file mode 100644 index 00000000000..d20c2435dc2 --- /dev/null +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts @@ -0,0 +1,53 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { hasMcpServerConnectionChanged } from '@/lib/mcp/orchestration/server-lifecycle' + +const currentConnection = { + url: 'https://memory.example.com/mcp', + transport: 'streamable-http', + headers: { + Authorization: 'Bearer {{MEMORY_KEY}}', + 'X-Workspace': 'workspace-1', + }, + timeout: 30000, + retries: 3, + enabled: true, + authType: 'headers', +} + +describe('hasMcpServerConnectionChanged', () => { + it('treats echoed connection settings as unchanged regardless of header key order', () => { + expect( + hasMcpServerConnectionChanged(currentConnection, { + url: currentConnection.url, + transport: currentConnection.transport, + headers: { + 'X-Workspace': 'workspace-1', + Authorization: 'Bearer {{MEMORY_KEY}}', + }, + timeout: currentConnection.timeout ?? undefined, + retries: currentConnection.retries ?? undefined, + enabled: currentConnection.enabled, + authType: currentConnection.authType as 'headers', + }) + ).toBe(false) + }) + + it.each([ + [{ url: 'https://other.example.com/mcp' }], + [{ transport: 'sse' }], + [{ headers: { Authorization: 'Bearer changed' } }], + [{ timeout: 15000 }], + [{ retries: 1 }], + [{ enabled: false }], + [{ authType: 'oauth' as const }], + ])('detects an actual connection setting change', (updates) => { + expect(hasMcpServerConnectionChanged(currentConnection, updates)).toBe(true) + }) + + it('detects an OAuth credential change', () => { + expect(hasMcpServerConnectionChanged(currentConnection, {}, true)).toBe(true) + }) +}) diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index d8d2d73115a..2556bac2ee5 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -4,6 +4,7 @@ import { mcpServerOauth } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' import { and, eq, isNull } from 'drizzle-orm' +import { isEqual } from 'es-toolkit' import type { NextRequest } from 'next/server' import { encryptSecret } from '@/lib/core/security/encryption' import { @@ -84,6 +85,30 @@ export interface PerformMcpServerResult { server?: typeof mcpServers.$inferSelect updated?: boolean authType?: McpAuthType + connectionChanged?: boolean +} + +export function hasMcpServerConnectionChanged( + currentServer: Pick< + typeof mcpServers.$inferSelect, + 'url' | 'transport' | 'headers' | 'timeout' | 'retries' | 'enabled' | 'authType' + >, + updates: Pick< + PerformUpdateMcpServerParams, + 'url' | 'transport' | 'headers' | 'timeout' | 'retries' | 'enabled' | 'authType' + >, + oauthCredentialsChanged = false +): boolean { + return ( + oauthCredentialsChanged || + (updates.url !== undefined && updates.url !== currentServer.url) || + (updates.transport !== undefined && updates.transport !== currentServer.transport) || + (updates.headers !== undefined && !isEqual(updates.headers, currentServer.headers ?? {})) || + (updates.timeout !== undefined && updates.timeout !== currentServer.timeout) || + (updates.retries !== undefined && updates.retries !== currentServer.retries) || + (updates.enabled !== undefined && updates.enabled !== currentServer.enabled) || + (updates.authType !== undefined && updates.authType !== currentServer.authType) + ) } type ValidateMcpServerUrlResult = @@ -316,6 +341,11 @@ export async function performUpdateMcpServer( const [currentServer] = await db .select({ url: mcpServers.url, + transport: mcpServers.transport, + headers: mcpServers.headers, + timeout: mcpServers.timeout, + retries: mcpServers.retries, + enabled: mcpServers.enabled, authType: mcpServers.authType, oauthClientId: mcpServers.oauthClientId, oauthClientSecret: mcpServers.oauthClientSecret, @@ -349,6 +379,11 @@ export async function performUpdateMcpServer( currentClientId: currentServer.oauthClientId, currentEncryptedClientSecret: currentServer.oauthClientSecret, }) + const connectionChanged = hasMcpServerConnectionChanged( + currentServer, + { ...params, authType: updateData.authType as McpAuthType | undefined }, + credsChanged + ) const shouldClearOauth = urlChanged || credsChanged const resolvedAuthType = (updateData.authType ?? currentServer.authType) as McpAuthType if (shouldClearOauth && resolvedAuthType === 'oauth') { @@ -381,15 +416,7 @@ export async function performUpdateMcpServer( if (!server) return { success: false, error: 'Server not found', errorCode: 'not_found' } - const shouldClearCache = - urlChanged || - credsChanged || - params.enabled !== undefined || - params.headers !== undefined || - params.timeout !== undefined || - params.retries !== undefined - - if (shouldClearCache) await mcpService.clearCache(params.workspaceId) + if (connectionChanged) await mcpService.clearCache(params.workspaceId) recordAudit({ workspaceId: params.workspaceId, @@ -410,7 +437,7 @@ export async function performUpdateMcpServer( request: params.request, }) - return { success: true, server } + return { success: true, server, connectionChanged } } catch (error) { logger.error('Failed to update MCP server', { error }) return { success: false, error: 'Failed to update MCP server', errorCode: 'internal' } diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index f4e5817f69e..2368e2b85ef 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -14,10 +14,22 @@ const { mockValidateSsrf, mockIsDomainAllowed, mockCacheAdapter, + mockDbUpdateSet, + MockMcpOauthRedirectRequired, + mockGetOrCreateOauthRow, + mockLoadPreregisteredClient, + mockWithMcpOauthRefreshLock, } = vi.hoisted(() => { const mockListTools = vi.fn() const mockConnect = vi.fn() const mockDisconnect = vi.fn() + class MockMcpOauthRedirectRequired extends Error { + constructor(public readonly authorizationUrl: string) { + super('MCP OAuth redirect required') + this.name = 'McpOauthRedirectRequired' + } + } + const mockDbUpdateSet = vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }) // In-memory cache adapter so the service never touches the real Redis the // local .env points at (unreachable in CI/sandbox → hangs). Honors TTL via // an expiry timestamp so negative-cache assertions behave like production. @@ -67,6 +79,11 @@ const { mockValidateDomain: vi.fn(), mockValidateSsrf: vi.fn(), mockIsDomainAllowed: vi.fn(() => true), + mockDbUpdateSet, + MockMcpOauthRedirectRequired, + mockGetOrCreateOauthRow: vi.fn(), + mockLoadPreregisteredClient: vi.fn(), + mockWithMcpOauthRefreshLock: vi.fn(), } }) @@ -80,13 +97,12 @@ vi.mock('@sim/db', () => { }) return thenable } - const setter = vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }) return { db: { select: vi.fn().mockReturnValue({ from: vi.fn().mockReturnValue({ where }), }), - update: vi.fn().mockReturnValue({ set: setter }), + update: vi.fn().mockReturnValue({ set: mockDbUpdateSet }), insert: vi.fn(), delete: vi.fn(), }, @@ -108,10 +124,11 @@ vi.mock('@/lib/mcp/domain-check', () => ({ })) vi.mock('@/lib/mcp/oauth', () => ({ - getOrCreateOauthRow: vi.fn(), - loadPreregisteredClient: vi.fn(), + getOrCreateOauthRow: mockGetOrCreateOauthRow, + loadPreregisteredClient: mockLoadPreregisteredClient, + McpOauthRedirectRequired: MockMcpOauthRedirectRequired, SimMcpOauthProvider: vi.fn(), - withMcpOauthRefreshLock: vi.fn(), + withMcpOauthRefreshLock: mockWithMcpOauthRefreshLock, })) vi.mock('@/lib/mcp/resolve-config', () => ({ @@ -123,6 +140,7 @@ vi.mock('@/lib/mcp/storage', () => ({ getMcpCacheType: () => 'memory', })) +import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' import { mcpService } from '@/lib/mcp/service' import { McpOauthAuthorizationRequiredError } from '@/lib/mcp/types' @@ -171,6 +189,13 @@ describe('McpService.discoverTools per-server caching', () => { mockResolveEnvVars.mockImplementation((config: { url: string }) => Promise.resolve({ config: { ...config, url: config.url }, missingVars: [] }) ) + mockGetOrCreateOauthRow.mockResolvedValue({ + tokens: { access_token: 'access-token', token_type: 'bearer' }, + }) + mockLoadPreregisteredClient.mockResolvedValue(undefined) + mockWithMcpOauthRefreshLock.mockImplementation((_serverId: string, callback: () => unknown) => + callback() + ) mockConnect.mockResolvedValue(undefined) mockDisconnect.mockResolvedValue(undefined) // The McpService singleton holds cache state across imports. @@ -366,4 +391,110 @@ describe('McpService.discoverTools per-server caching', () => { expect(after.map((t) => t.name)).toEqual(['a1']) expect(mockListTools).toHaveBeenCalledTimes(1) }) + + it('treats an OAuth redirect during aggregate discovery as authorization pending', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })]) + mockConnect.mockRejectedValueOnce( + new McpOauthRedirectRequired('https://mcp-a.example.com/authorize') + ) + + await expect(mcpService.discoverTools(USER_ID, WORKSPACE_ID)).resolves.toEqual([]) + expect(mockDbUpdateSet).toHaveBeenCalledTimes(1) + expect(mockDbUpdateSet).toHaveBeenCalledWith({ + connectionStatus: 'disconnected', + lastError: null, + updatedAt: expect.any(Date), + }) + + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + const tools = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + + expect(tools.map((item) => item.name)).toEqual(['a1']) + expect(mockListTools).toHaveBeenCalledTimes(1) + }) + + it('reports an OAuth redirect as a disconnected server summary without a hard error', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })]) + mockConnect.mockRejectedValueOnce( + new McpOauthRedirectRequired('https://mcp-a.example.com/authorize') + ) + + await expect(mcpService.getServerSummaries(USER_ID, WORKSPACE_ID)).resolves.toEqual([ + { + id: 'mcp-a', + name: 'A', + url: 'https://mcp-a.example.com/mcp', + transport: 'streamable-http', + status: 'disconnected', + toolCount: 0, + lastSeen: undefined, + error: undefined, + }, + ]) + }) + + it('verifies a server with a force-refreshed live tools handshake', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a'), tool('a2', 'mcp-a')]) + + await expect( + mcpService.verifyServerConnection(USER_ID, 'mcp-a', WORKSPACE_ID) + ).resolves.toEqual({ + verified: true, + toolCount: 2, + requiresAuthorization: false, + }) + }) + + it('returns a structured verification failure without throwing', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockRejectedValueOnce(new Error('Request timed out\ninternal detail')) + + await expect( + mcpService.verifyServerConnection(USER_ID, 'mcp-a', WORKSPACE_ID) + ).resolves.toEqual({ + verified: false, + toolCount: 0, + requiresAuthorization: false, + error: 'Request timed out', + }) + expect(mockDbUpdateSet).toHaveBeenCalledWith({ + connectionStatus: 'disconnected', + lastError: 'Request timed out', + statusConfig: { + consecutiveFailures: 1, + lastSuccessfulDiscovery: null, + }, + updatedAt: expect.any(Date), + }) + }) + + it('treats an OAuth redirect as authorization-required without poisoning status or cache', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })]) + mockConnect.mockRejectedValueOnce( + new McpOauthRedirectRequired('https://mcp-a.example.com/authorize') + ) + + await expect( + mcpService.verifyServerConnection(USER_ID, 'mcp-a', WORKSPACE_ID) + ).resolves.toEqual({ + verified: false, + toolCount: 0, + requiresAuthorization: true, + error: 'MCP OAuth redirect required', + }) + expect(mockDbUpdateSet).toHaveBeenCalledTimes(1) + expect(mockDbUpdateSet).toHaveBeenCalledWith({ + connectionStatus: 'disconnected', + lastError: null, + updatedAt: expect.any(Date), + }) + + mockListTools.mockClear() + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + const tools = await mcpService.discoverServerTools(USER_ID, 'mcp-a', WORKSPACE_ID) + + expect(tools.map((item) => item.name)).toEqual(['a1']) + expect(mockListTools).toHaveBeenCalledTimes(1) + }) }) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 7e97d3fc2de..faaa25cc03f 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -5,6 +5,7 @@ import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' +import { truncate } from '@sim/utils/string' import { and, eq, isNull } from 'drizzle-orm' import { isTest } from '@/lib/core/config/env-flags' import { generateRequestId } from '@/lib/core/utils/request' @@ -18,6 +19,7 @@ import { import { getOrCreateOauthRow, loadPreregisteredClient, + McpOauthRedirectRequired, SimMcpOauthProvider, withMcpOauthRefreshLock, } from '@/lib/mcp/oauth' @@ -33,6 +35,7 @@ import { type McpServerConfig, type McpServerStatusConfig, type McpServerSummary, + type McpServerVerificationResult, type McpTool, type McpToolCall, type McpToolResult, @@ -41,6 +44,15 @@ import { import { MCP_CLIENT_CONSTANTS, MCP_CONSTANTS } from '@/lib/mcp/utils' const logger = createLogger('McpService') +const MAX_VERIFICATION_ERROR_LENGTH = 200 + +function isMcpAuthorizationRequired(error: unknown): boolean { + return ( + error instanceof McpOauthAuthorizationRequiredError || + error instanceof McpOauthRedirectRequired || + error instanceof UnauthorizedError + ) +} function serverCacheKey(workspaceId: string, serverId: string): string { return `workspace:${workspaceId}:server:${serverId}` @@ -392,7 +404,7 @@ class McpService { serverId: string, error: unknown ): Promise { - if (error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError) { + if (isMcpAuthorizationRequired(error)) { return } try { @@ -423,6 +435,56 @@ class McpService { } } + private async updateServerAuthorizationRequiredStatus(serverId: string): Promise { + try { + await db + .update(mcpServers) + .set({ + connectionStatus: 'disconnected', + lastError: null, + updatedAt: new Date(), + }) + .where(eq(mcpServers.id, serverId)) + } catch (error) { + logger.error(`Failed to update authorization-required status for ${serverId}:`, error) + } + } + + async verifyServerConnection( + userId: string, + serverId: string, + workspaceId: string + ): Promise { + try { + const tools = await this.discoverServerTools(userId, serverId, workspaceId, true) + return { + verified: true, + toolCount: tools.length, + requiresAuthorization: false, + } + } catch (error) { + const message = truncate( + getErrorMessage(error, 'Connection failed').split('\n')[0], + MAX_VERIFICATION_ERROR_LENGTH, + '' + ) + const requiresAuthorization = isMcpAuthorizationRequired(error) + + if (requiresAuthorization) { + await this.updateServerAuthorizationRequiredStatus(serverId) + } else { + await this.updateServerStatus(serverId, workspaceId, false, message) + } + + return { + verified: false, + toolCount: 0, + requiresAuthorization, + error: message, + } + } + } + async discoverTools( userId: string, workspaceId: string, @@ -479,10 +541,7 @@ class McpService { await client.disconnect() } } catch (error) { - if ( - error instanceof McpOauthAuthorizationRequiredError || - error instanceof UnauthorizedError - ) { + if (isMcpAuthorizationRequired(error)) { return { kind: 'oauth-pending' } } return { @@ -535,20 +594,7 @@ class McpService { if (outcome.kind === 'oauth-pending') { // Mark disconnected so the UI surfaces the re-auth button. logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) - deferredSideEffects.push( - db - .update(mcpServers) - .set({ - connectionStatus: 'disconnected', - lastError: null, - updatedAt: new Date(), - }) - .where(eq(mcpServers.id, server.id)) - .then(() => undefined) - .catch((err) => { - logger.warn(`[${requestId}] Failed to mark server ${server.id} disconnected:`, err) - }) - ) + deferredSideEffects.push(this.updateServerAuthorizationRequiredStatus(server.id)) return } if (outcome.kind === 'unhealthy') { @@ -747,10 +793,7 @@ class McpService { error: undefined, }) } catch (error) { - if ( - error instanceof McpOauthAuthorizationRequiredError || - error instanceof UnauthorizedError - ) { + if (isMcpAuthorizationRequired(error)) { summaries.push({ id: config.id, name: config.name, diff --git a/apps/sim/lib/mcp/types.ts b/apps/sim/lib/mcp/types.ts index 575e86ea2f8..7133a87e333 100644 --- a/apps/sim/lib/mcp/types.ts +++ b/apps/sim/lib/mcp/types.ts @@ -10,6 +10,30 @@ export interface McpServerStatusConfig { lastSuccessfulDiscovery: string | null } +export type McpServerVerificationResult = + | { + verified: true + toolCount: number + requiresAuthorization: false + error?: never + skipped?: false + } + | { + verified: false + toolCount: 0 + requiresAuthorization: boolean + error: string + skipped?: false + } + | { + verified: false + toolCount: 0 + requiresAuthorization: false + error?: never + skipped: true + reason: 'server_disabled' | 'connection_unchanged' + } + export interface McpServerConfig { id: string name: string