From ec4b4534171fd734e1b7b292a9c7ad89adee08b9 Mon Sep 17 00:00:00 2001
From: Roo Code <roomote@roocode.com>
Date: Fri, 18 Jul 2025 20:09:13 +0000
Subject: [PATCH] fix: prevent disabled MCP servers from starting (#2797)

- Add disabled checks before calling connectToServer() in updateServerConnections()
- Prevent new disabled servers from connecting (line 977)
- Disconnect servers when they become disabled (line 985-993)
- Add test cases for disabled server connection prevention
- Follows existing pattern used in readResource() and callTool() methods
---
 src/services/mcp/McpHub.ts                | 33 ++++++---
 src/services/mcp/__tests__/McpHub.spec.ts | 90 +++++++++++++++++++++++
 2 files changed, 112 insertions(+), 11 deletions(-)

diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts
index 10a74712ef..0d1177d5bb 100644
--- a/src/services/mcp/McpHub.ts
+++ b/src/services/mcp/McpHub.ts
@@ -974,20 +974,31 @@ export class McpHub {
 
 			if (!currentConnection) {
 				// New server
-				try {
-					this.setupFileWatcher(name, validatedConfig, source)
-					await this.connectToServer(name, validatedConfig, source)
-				} catch (error) {
-					this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
+				if (!validatedConfig.disabled) {
+					try {
+						this.setupFileWatcher(name, validatedConfig, source)
+						await this.connectToServer(name, validatedConfig, source)
+					} catch (error) {
+						this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
+					}
 				}
 			} else if (!deepEqual(JSON.parse(currentConnection.server.config), config)) {
 				// Existing server with changed config
-				try {
-					this.setupFileWatcher(name, validatedConfig, source)
-					await this.deleteConnection(name, source)
-					await this.connectToServer(name, validatedConfig, source)
-				} catch (error) {
-					this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
+				if (!validatedConfig.disabled) {
+					try {
+						this.setupFileWatcher(name, validatedConfig, source)
+						await this.deleteConnection(name, source)
+						await this.connectToServer(name, validatedConfig, source)
+					} catch (error) {
+						this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
+					}
+				} else {
+					// Server is now disabled, disconnect it
+					try {
+						await this.deleteConnection(name, source)
+					} catch (error) {
+						this.showErrorMessage(`Failed to disconnect disabled MCP server ${name}`, error)
+					}
 				}
 			}
 			// If server exists with same config, do nothing
diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts
index 98ef4514c2..cae0c30aeb 100644
--- a/src/services/mcp/__tests__/McpHub.spec.ts
+++ b/src/services/mcp/__tests__/McpHub.spec.ts
@@ -570,6 +570,96 @@ describe("McpHub", () => {
 				'Server "disabled-server" is disabled',
 			)
 		})
+
+		it("should not connect to disabled servers during updateServerConnections", async () => {
+			const mockConfig = {
+				mcpServers: {
+					"disabled-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: true,
+					},
+				},
+			}
+
+			// Mock reading initial config
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
+
+			// Ensure no connections exist initially
+			mcpHub.connections = []
+
+			// Mock the connectToServer method to track if it's called
+			const connectToServerSpy = vi.spyOn(mcpHub as any, "connectToServer")
+
+			// Update server connections with disabled server
+			await mcpHub.updateServerConnections(mockConfig.mcpServers, "global", false)
+
+			// Verify that connectToServer was never called for the disabled server
+			expect(connectToServerSpy).not.toHaveBeenCalled()
+
+			// Verify no connections were created
+			expect(mcpHub.connections.length).toBe(0)
+		})
+
+		it("should disconnect server when it becomes disabled", async () => {
+			// First, set up an enabled server
+			const enabledConfig = {
+				mcpServers: {
+					"test-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: false,
+					},
+				},
+			}
+
+			// Mock reading initial config
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(enabledConfig))
+
+			// Set up existing connection
+			const mockConnection: McpConnection = {
+				server: {
+					name: "test-server",
+					config: JSON.stringify(enabledConfig.mcpServers["test-server"]),
+					status: "connected",
+					disabled: false,
+					source: "global",
+				},
+				client: {
+					close: vi.fn().mockResolvedValue(undefined),
+				} as any,
+				transport: {
+					close: vi.fn().mockResolvedValue(undefined),
+				} as any,
+			}
+			mcpHub.connections = [mockConnection]
+
+			// Now update with disabled config
+			const disabledConfig = {
+				mcpServers: {
+					"test-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: true,
+					},
+				},
+			}
+
+			// Mock reading disabled config
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(disabledConfig))
+
+			// Mock the deleteConnection method to track if it's called
+			const deleteConnectionSpy = vi.spyOn(mcpHub as any, "deleteConnection")
+
+			// Update server connections with disabled server
+			await mcpHub.updateServerConnections(disabledConfig.mcpServers, "global", false)
+
+			// Verify that deleteConnection was called to disconnect the server
+			expect(deleteConnectionSpy).toHaveBeenCalledWith("test-server", "global")
+		})
 	})
 
 	describe("callTool", () => {