Fix MCP edit verification feedback (#5596)

Author: j15z

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',
+        }),
+      })
+    )
+  })
 })

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')
 

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)
+  })
+})

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' }

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') {