From 6a549039461695360844668b002872e2b9f12c3a Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Sat, 11 Jul 2026 10:51:16 -0700 Subject: [PATCH 1/4] feat(copilot): verify managed MCP connections --- .../management/manage-mcp-tool.test.ts | 202 ++++++++++++++++++ .../handlers/management/manage-mcp-tool.ts | 30 +++ apps/sim/lib/mcp/service.test.ts | 27 +++ apps/sim/lib/mcp/service.ts | 27 +++ apps/sim/lib/mcp/types.ts | 24 +++ 5 files changed, 310 insertions(+) create mode 100644 apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.test.ts 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..1e7f23fbb15 --- /dev/null +++ b/apps/sim/lib/copilot/tools/handlers/management/manage-mcp-tool.test.ts @@ -0,0 +1,202 @@ +/** + * @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 }, + }) + 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 }, + }) + + 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', + }), + }) + ) + }) +}) 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..0b98cf122fe 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,15 @@ export async function executeManageMcpTool( return { success: false, error: `MCP server not found: ${params.serverId}` } } + const changesConnection = ['url', 'transport', 'headers', 'timeout', 'enabled'].some( + (key) => config[key as keyof ManageMcpToolConfig] !== undefined + ) + const verification = !result.server.enabled + ? skippedVerification('server_disabled') + : changesConnection + ? await mcpService.verifyServerConnection(context.userId, params.serverId, workspaceId) + : skippedVerification('connection_unchanged') + return { success: true, output: { @@ -155,6 +184,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/service.test.ts b/apps/sim/lib/mcp/service.test.ts index f4e5817f69e..619dc0fa356 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -366,4 +366,31 @@ describe('McpService.discoverTools per-server caching', () => { expect(after.map((t) => t.name)).toEqual(['a1']) expect(mockListTools).toHaveBeenCalledTimes(1) }) + + 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', + }) + }) }) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 7e97d3fc2de..6f571a5a2d2 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' @@ -33,6 +34,7 @@ import { type McpServerConfig, type McpServerStatusConfig, type McpServerSummary, + type McpServerVerificationResult, type McpTool, type McpToolCall, type McpToolResult, @@ -41,6 +43,7 @@ import { import { MCP_CLIENT_CONSTANTS, MCP_CONSTANTS } from '@/lib/mcp/utils' const logger = createLogger('McpService') +const MAX_VERIFICATION_ERROR_LENGTH = 200 function serverCacheKey(workspaceId: string, serverId: string): string { return `workspace:${workspaceId}:server:${serverId}` @@ -423,6 +426,30 @@ class McpService { } } + 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 = getErrorMessage(error, 'Connection failed').split('\n')[0] + return { + verified: false, + toolCount: 0, + requiresAuthorization: + error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError, + error: truncate(message, MAX_VERIFICATION_ERROR_LENGTH, ''), + } + } + } + async discoverTools( userId: string, workspaceId: string, 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 From 9a8931df51918e134e627d2b5d41c4d1dcd47507 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Mon, 13 Jul 2026 10:47:26 -0700 Subject: [PATCH 2/4] Fix managed MCP verification state (#5596) - persist failed verification status before returning - classify OAuth redirect failures as authorization-required - cover status persistence and immediate OAuth retry --- apps/sim/lib/mcp/service.test.ts | 73 +++++++++++++++++++++++++++++--- apps/sim/lib/mcp/service.ts | 45 +++++++++++++++++--- 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 619dc0fa356..3ab5153b5b6 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. @@ -392,5 +417,43 @@ describe('McpService.discoverTools per-server caching', () => { 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 6f571a5a2d2..4f0c35570f9 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -19,6 +19,7 @@ import { import { getOrCreateOauthRow, loadPreregisteredClient, + McpOauthRedirectRequired, SimMcpOauthProvider, withMcpOauthRefreshLock, } from '@/lib/mcp/oauth' @@ -45,6 +46,14 @@ 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}` } @@ -395,7 +404,7 @@ class McpService { serverId: string, error: unknown ): Promise { - if (error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError) { + if (isMcpAuthorizationRequired(error)) { return } try { @@ -426,6 +435,21 @@ 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, @@ -439,13 +463,24 @@ class McpService { requiresAuthorization: false, } } catch (error) { - const message = getErrorMessage(error, 'Connection failed').split('\n')[0] + 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 instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError, - error: truncate(message, MAX_VERIFICATION_ERROR_LENGTH, ''), + requiresAuthorization, + error: message, } } } From 98e1c24865e476b936454220fce1e2d1e842796f Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Mon, 13 Jul 2026 10:58:30 -0700 Subject: [PATCH 3/4] Unify MCP authorization classification (#5596) - use the shared OAuth-required classifier in aggregate discovery - keep server summaries consistent for redirect-required auth - cover both sibling paths with focused regressions --- apps/sim/lib/mcp/service.test.ts | 41 ++++++++++++++++++++++++++++++++ apps/sim/lib/mcp/service.ts | 10 ++------ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 3ab5153b5b6..2368e2b85ef 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -392,6 +392,47 @@ describe('McpService.discoverTools per-server caching', () => { 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')]) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 4f0c35570f9..cb71817ed82 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -541,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 { @@ -809,10 +806,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, From 256e22c873a07bb3a8bdee7b872a844b9de86780 Mon Sep 17 00:00:00 2001 From: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Date: Mon, 13 Jul 2026 11:09:21 -0700 Subject: [PATCH 4/4] Fix MCP edit verification feedback (#5596) --- .../management/manage-mcp-tool.test.ts | 87 +++++++++++++++++++ .../handlers/management/manage-mcp-tool.ts | 5 +- .../orchestration/server-lifecycle.test.ts | 53 +++++++++++ .../lib/mcp/orchestration/server-lifecycle.ts | 47 +++++++--- apps/sim/lib/mcp/service.ts | 15 +--- 5 files changed, 179 insertions(+), 28 deletions(-) create mode 100644 apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts 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 index 1e7f23fbb15..a1dc035faf2 100644 --- 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 @@ -149,6 +149,7 @@ describe('executeManageMcpTool verification', () => { mockUpdateMcpServer.mockResolvedValue({ success: true, server: { id: 'mcp-1', name: 'Memory', enabled: true }, + connectionChanged: true, }) mockVerifyServerConnection.mockResolvedValue({ verified: true, @@ -177,6 +178,7 @@ describe('executeManageMcpTool verification', () => { mockUpdateMcpServer.mockResolvedValue({ success: true, server: { id: 'mcp-1', name: 'Renamed Memory', enabled: true }, + connectionChanged: false, }) const result = await executeManageMcpTool( @@ -199,4 +201,89 @@ describe('executeManageMcpTool verification', () => { }) ) }) + + 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 0b98cf122fe..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 @@ -167,12 +167,9 @@ export async function executeManageMcpTool( return { success: false, error: `MCP server not found: ${params.serverId}` } } - const changesConnection = ['url', 'transport', 'headers', 'timeout', 'enabled'].some( - (key) => config[key as keyof ManageMcpToolConfig] !== undefined - ) const verification = !result.server.enabled ? skippedVerification('server_disabled') - : changesConnection + : result.connectionChanged ? await mcpService.verifyServerConnection(context.userId, params.serverId, workspaceId) : skippedVerification('connection_unchanged') 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.ts b/apps/sim/lib/mcp/service.ts index cb71817ed82..faaa25cc03f 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -594,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') {