fix: prevent disabled MCP servers from starting processes and show correct status (RooCodeInc#6084)

Co-authored-by: Roo Code <[email protected]>
Co-authored-by: hannesrudolph <[email protected]>
Co-authored-by: Daniel Riccio <[email protected]>

Author: 4 people

src/core/webview/webviewMessageHandler.ts

@@ -901,6 +901,13 @@ export const webviewMessageHandler = async (
 		case "mcpEnabled":
 			const mcpEnabled = message.bool ?? true
 			await updateGlobalState("mcpEnabled", mcpEnabled)
+
+			// Delegate MCP enable/disable logic to McpHub
+			const mcpHubInstance = provider.getMcpHub()
+			if (mcpHubInstance) {
+				await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)
+			}
+
 			await provider.postStateToWebview()
 			break
 		case "enableMcpServerCreation":

src/services/mcp/McpHub.ts

@@ -33,12 +33,29 @@ import { fileExistsAtPath } from "../../utils/fs"
 import { arePathsEqual } from "../../utils/path"
 import { injectVariables } from "../../utils/config"
 
-export type McpConnection = {
+// Discriminated union for connection states
+export type ConnectedMcpConnection = {
+	type: "connected"
 	server: McpServer
 	client: Client
 	transport: StdioClientTransport | SSEClientTransport | StreamableHTTPClientTransport
 }
 
+export type DisconnectedMcpConnection = {
+	type: "disconnected"
+	server: McpServer
+	client: null
+	transport: null
+}
+
+export type McpConnection = ConnectedMcpConnection | DisconnectedMcpConnection
+
+// Enum for disable reasons
+export enum DisableReason {
+	MCP_DISABLED = "mcpDisabled",
+	SERVER_DISABLED = "serverDisabled",
+}
+
 // Base configuration schema for common settings
 const BaseConfigSchema = z.object({
 	disabled: z.boolean().optional(),
@@ -497,6 +514,7 @@ export class McpHub {
 			const result = McpSettingsSchema.safeParse(config)
 
 			if (result.success) {
+				// Pass all servers including disabled ones - they'll be handled in updateServerConnections
 				await this.updateServerConnections(result.data.mcpServers || {}, source, false)
 			} else {
 				const errorMessages = result.error.errors
@@ -552,6 +570,49 @@ export class McpHub {
 		await this.initializeMcpServers("project")
 	}
 
+	/**
+	 * Creates a placeholder connection for disabled servers or when MCP is globally disabled
+	 * @param name The server name
+	 * @param config The server configuration
+	 * @param source The source of the server (global or project)
+	 * @param reason The reason for creating a placeholder (mcpDisabled or serverDisabled)
+	 * @returns A placeholder DisconnectedMcpConnection object
+	 */
+	private createPlaceholderConnection(
+		name: string,
+		config: z.infer<typeof ServerConfigSchema>,
+		source: "global" | "project",
+		reason: DisableReason,
+	): DisconnectedMcpConnection {
+		return {
+			type: "disconnected",
+			server: {
+				name,
+				config: JSON.stringify(config),
+				status: "disconnected",
+				disabled: reason === DisableReason.SERVER_DISABLED ? true : config.disabled,
+				source,
+				projectPath: source === "project" ? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath : undefined,
+				errorHistory: [],
+			},
+			client: null,
+			transport: null,
+		}
+	}
+
+	/**
+	 * Checks if MCP is globally enabled
+	 * @returns Promise<boolean> indicating if MCP is enabled
+	 */
+	private async isMcpEnabled(): Promise<boolean> {
+		const provider = this.providerRef.deref()
+		if (!provider) {
+			return true // Default to enabled if provider is not available
+		}
+		const state = await provider.getState()
+		return state.mcpEnabled ?? true
+	}
+
 	private async connectToServer(
 		name: string,
 		config: z.infer<typeof ServerConfigSchema>,
@@ -560,6 +621,26 @@ export class McpHub {
 		// Remove existing connection if it exists with the same source
 		await this.deleteConnection(name, source)
 
+		// Check if MCP is globally enabled
+		const mcpEnabled = await this.isMcpEnabled()
+		if (!mcpEnabled) {
+			// Still create a connection object to track the server, but don't actually connect
+			const connection = this.createPlaceholderConnection(name, config, source, DisableReason.MCP_DISABLED)
+			this.connections.push(connection)
+			return
+		}
+
+		// Skip connecting to disabled servers
+		if (config.disabled) {
+			// Still create a connection object to track the server, but don't actually connect
+			const connection = this.createPlaceholderConnection(name, config, source, DisableReason.SERVER_DISABLED)
+			this.connections.push(connection)
+			return
+		}
+
+		// Set up file watchers for enabled servers
+		this.setupFileWatcher(name, config, source)
+
 		try {
 			const client = new Client(
 				{
@@ -733,7 +814,9 @@ export class McpHub {
 				transport.start = async () => {}
 			}
 
-			const connection: McpConnection = {
+			// Create a connected connection
+			const connection: ConnectedMcpConnection = {
+				type: "connected",
 				server: {
 					name,
 					config: JSON.stringify(configInjected),
@@ -826,8 +909,8 @@ export class McpHub {
 			// Use the helper method to find the connection
 			const connection = this.findConnection(serverName, source)
 
-			if (!connection) {
-				throw new Error(`Server ${serverName} not found`)
+			if (!connection || connection.type !== "connected") {
+				return []
 			}
 
 			const response = await connection.client.request({ method: "tools/list" }, ListToolsResultSchema)
@@ -881,7 +964,7 @@ export class McpHub {
 	private async fetchResourcesList(serverName: string, source?: "global" | "project"): Promise<McpResource[]> {
 		try {
 			const connection = this.findConnection(serverName, source)
-			if (!connection) {
+			if (!connection || connection.type !== "connected") {
 				return []
 			}
 			const response = await connection.client.request({ method: "resources/list" }, ListResourcesResultSchema)
@@ -898,7 +981,7 @@ export class McpHub {
 	): Promise<McpResourceTemplate[]> {
 		try {
 			const connection = this.findConnection(serverName, source)
-			if (!connection) {
+			if (!connection || connection.type !== "connected") {
 				return []
 			}
 			const response = await connection.client.request(
@@ -913,15 +996,20 @@ export class McpHub {
 	}
 
 	async deleteConnection(name: string, source?: "global" | "project"): Promise<void> {
+		// Clean up file watchers for this server
+		this.removeFileWatchersForServer(name)
+
 		// If source is provided, only delete connections from that source
 		const connections = source
 			? this.connections.filter((conn) => conn.server.name === name && conn.server.source === source)
 			: this.connections.filter((conn) => conn.server.name === name)
 
 		for (const connection of connections) {
 			try {
-				await connection.transport.close()
-				await connection.client.close()
+				if (connection.type === "connected") {
+					await connection.transport.close()
+					await connection.client.close()
+				}
 			} catch (error) {
 				console.error(`Failed to close transport for ${name}:`, error)
 			}
@@ -975,15 +1063,21 @@ export class McpHub {
 			if (!currentConnection) {
 				// New server
 				try {
-					this.setupFileWatcher(name, validatedConfig, source)
+					// Only setup file watcher for enabled servers
+					if (!validatedConfig.disabled) {
+						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)
+					// Only setup file watcher for enabled servers
+					if (!validatedConfig.disabled) {
+						this.setupFileWatcher(name, validatedConfig, source)
+					}
 					await this.deleteConnection(name, source)
 					await this.connectToServer(name, validatedConfig, source)
 				} catch (error) {
@@ -1066,10 +1160,21 @@ export class McpHub {
 		this.fileWatchers.clear()
 	}
 
+	private removeFileWatchersForServer(serverName: string) {
+		const watchers = this.fileWatchers.get(serverName)
+		if (watchers) {
+			watchers.forEach((watcher) => watcher.close())
+			this.fileWatchers.delete(serverName)
+		}
+	}
+
 	async restartConnection(serverName: string, source?: "global" | "project"): Promise<void> {
 		this.isConnecting = true
-		const provider = this.providerRef.deref()
-		if (!provider) {
+
+		// Check if MCP is globally enabled
+		const mcpEnabled = await this.isMcpEnabled()
+		if (!mcpEnabled) {
+			this.isConnecting = false
 			return
 		}
 
@@ -1111,6 +1216,23 @@ export class McpHub {
 			return
 		}
 
+		// Check if MCP is globally enabled
+		const mcpEnabled = await this.isMcpEnabled()
+		if (!mcpEnabled) {
+			// Clear all existing connections
+			const existingConnections = [...this.connections]
+			for (const conn of existingConnections) {
+				await this.deleteConnection(conn.server.name, conn.server.source)
+			}
+
+			// Still initialize servers to track them, but they won't connect
+			await this.initializeMcpServers("global")
+			await this.initializeMcpServers("project")
+
+			await this.notifyWebviewOfServerChanges()
+			return
+		}
+
 		this.isConnecting = true
 		vscode.window.showInformationMessage(t("mcp:info.refreshing_all"))
 
@@ -1257,8 +1379,21 @@ export class McpHub {
 				try {
 					connection.server.disabled = disabled
 
-					// Only refresh capabilities if connected
-					if (connection.server.status === "connected") {
+					// If disabling a connected server, disconnect it
+					if (disabled && connection.server.status === "connected") {
+						// Clean up file watchers when disabling
+						this.removeFileWatchersForServer(serverName)
+						await this.deleteConnection(serverName, serverSource)
+						// Re-add as a disabled connection
+						await this.connectToServer(serverName, JSON.parse(connection.server.config), serverSource)
+					} else if (!disabled && connection.server.status === "disconnected") {
+						// If enabling a disabled server, connect it
+						const config = JSON.parse(connection.server.config)
+						await this.deleteConnection(serverName, serverSource)
+						// When re-enabling, file watchers will be set up in connectToServer
+						await this.connectToServer(serverName, config, serverSource)
+					} else if (connection.server.status === "connected") {
+						// Only refresh capabilities if connected
 						connection.server.tools = await this.fetchToolsList(serverName, serverSource)
 						connection.server.resources = await this.fetchResourcesList(serverName, serverSource)
 						connection.server.resourceTemplates = await this.fetchResourceTemplatesList(
@@ -1439,7 +1574,7 @@ export class McpHub {
 
 	async readResource(serverName: string, uri: string, source?: "global" | "project"): Promise<McpResourceResponse> {
 		const connection = this.findConnection(serverName, source)
-		if (!connection) {
+		if (!connection || connection.type !== "connected") {
 			throw new Error(`No connection found for server: ${serverName}${source ? ` with source ${source}` : ""}`)
 		}
 		if (connection.server.disabled) {
@@ -1463,7 +1598,7 @@ export class McpHub {
 		source?: "global" | "project",
 	): Promise<McpToolCallResponse> {
 		const connection = this.findConnection(serverName, source)
-		if (!connection) {
+		if (!connection || connection.type !== "connected") {
 			throw new Error(
 				`No connection found for server: ${serverName}${source ? ` with source ${source}` : ""}. Please make sure to use MCP servers available under 'Connected MCP Servers'.`,
 			)
@@ -1609,6 +1744,64 @@ export class McpHub {
 		}
 	}
 
+	/**
+	 * Handles enabling/disabling MCP globally
+	 * @param enabled Whether MCP should be enabled or disabled
+	 * @returns Promise<void>
+	 */
+	async handleMcpEnabledChange(enabled: boolean): Promise<void> {
+		if (!enabled) {
+			// If MCP is being disabled, disconnect all servers with error handling
+			const existingConnections = [...this.connections]
+			const disconnectionErrors: Array<{ serverName: string; error: string }> = []
+
+			for (const conn of existingConnections) {
+				try {
+					await this.deleteConnection(conn.server.name, conn.server.source)
+				} catch (error) {
+					const errorMessage = error instanceof Error ? error.message : String(error)
+					disconnectionErrors.push({
+						serverName: conn.server.name,
+						error: errorMessage,
+					})
+					console.error(`Failed to disconnect MCP server ${conn.server.name}: ${errorMessage}`)
+				}
+			}
+
+			// If there were errors, notify the user
+			if (disconnectionErrors.length > 0) {
+				const errorSummary = disconnectionErrors.map((e) => `${e.serverName}: ${e.error}`).join("\n")
+				vscode.window.showWarningMessage(
+					t("mcp:errors.disconnect_servers_partial", {
+						count: disconnectionErrors.length,
+						errors: errorSummary,
+					}) ||
+						`Failed to disconnect ${disconnectionErrors.length} MCP server(s). Check the output for details.`,
+				)
+			}
+
+			// Re-initialize servers to track them in disconnected state
+			try {
+				await this.refreshAllConnections()
+			} catch (error) {
+				console.error(`Failed to refresh MCP connections after disabling: ${error}`)
+				vscode.window.showErrorMessage(
+					t("mcp:errors.refresh_after_disable") || "Failed to refresh MCP connections after disabling",
+				)
+			}
+		} else {
+			// If MCP is being enabled, reconnect all servers
+			try {
+				await this.refreshAllConnections()
+			} catch (error) {
+				console.error(`Failed to refresh MCP connections after enabling: ${error}`)
+				vscode.window.showErrorMessage(
+					t("mcp:errors.refresh_after_enable") || "Failed to refresh MCP connections after enabling",
+				)
+			}
+		}
+	}
+
 	async dispose(): Promise<void> {
 		// Prevent multiple disposals
 		if (this.isDisposed) {