Skip to content

Commit cd0bc93

Browse files
roomoteroomote
committed
fix: prevent disabled MCP servers from starting processes and show correct status
- Add disabled check in updateServerConnections() before connecting to servers - Disconnect servers immediately when toggling to disabled state - Update UI to show grey/disabled color for disabled servers - Add comprehensive tests for disabled server behavior Fixes #6036
1 parent 2eb586b commit cd0bc93

File tree

3 files changed

+225
-9
lines changed

3 files changed

+225
-9
lines changed

‎src/services/mcp/McpHub.ts‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -973,19 +973,24 @@ export class McpHub {
973973
}
974974

975975
if (!currentConnection) {
976-
// New server
977-
try {
978-
this.setupFileWatcher(name, validatedConfig, source)
979-
await this.connectToServer(name, validatedConfig, source)
980-
} catch (error) {
981-
this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
976+
// New server - only connect if not disabled
977+
if (!validatedConfig.disabled) {
978+
try {
979+
this.setupFileWatcher(name, validatedConfig, source)
980+
await this.connectToServer(name, validatedConfig, source)
981+
} catch (error) {
982+
this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
983+
}
982984
}
983985
} else if (!deepEqual(JSON.parse(currentConnection.server.config), config)) {
984986
// Existing server with changed config
985987
try {
986988
this.setupFileWatcher(name, validatedConfig, source)
987989
await this.deleteConnection(name, source)
988-
await this.connectToServer(name, validatedConfig, source)
990+
// Only reconnect if not disabled
991+
if (!validatedConfig.disabled) {
992+
await this.connectToServer(name, validatedConfig, source)
993+
}
989994
} catch (error) {
990995
this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
991996
}
@@ -1257,8 +1262,17 @@ export class McpHub {
12571262
try {
12581263
connection.server.disabled = disabled
12591264

1260-
// Only refresh capabilities if connected
1261-
if (connection.server.status === "connected") {
1265+
// If disabling the server, disconnect it
1266+
if (disabled && connection.server.status === "connected") {
1267+
await this.deleteConnection(serverName, serverSource)
1268+
} else if (!disabled && connection.server.status !== "connected") {
1269+
// If enabling the server and it's not connected, connect it
1270+
const config = JSON.parse(connection.server.config)
1271+
await this.connectToServer(serverName, config, serverSource)
1272+
}
1273+
1274+
// Only refresh capabilities if connected and not disabled
1275+
if (!disabled && connection.server.status === "connected") {
12621276
connection.server.tools = await this.fetchToolsList(serverName, serverSource)
12631277
connection.server.resources = await this.fetchResourcesList(serverName, serverSource)
12641278
connection.server.resourceTemplates = await this.fetchResourceTemplatesList(

‎src/services/mcp/__tests__/McpHub.spec.ts‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,19 @@ vi.mock("vscode", () => ({
6565
Disposable: {
6666
from: vi.fn(),
6767
},
68+
Uri: {
69+
file: vi.fn((path) => ({
70+
scheme: "file",
71+
authority: "",
72+
path,
73+
query: "",
74+
fragment: "",
75+
fsPath: path,
76+
with: vi.fn(),
77+
toJSON: vi.fn(),
78+
})),
79+
},
80+
RelativePattern: vi.fn((base, pattern) => ({ base, pattern })),
6881
}))
6982
vi.mock("fs/promises")
7083
vi.mock("../../../core/webview/ClineProvider")
@@ -569,6 +582,190 @@ describe("McpHub", () => {
569582
'Server "disabled-server" is disabled',
570583
)
571584
})
585+
586+
it("should not connect to disabled servers during initialization", async () => {
587+
// Clear all mocks before this test
588+
vi.clearAllMocks()
589+
590+
const mockConfig = {
591+
mcpServers: {
592+
"disabled-server": {
593+
type: "stdio",
594+
command: "node",
595+
args: ["test.js"],
596+
disabled: true,
597+
},
598+
"enabled-server": {
599+
type: "stdio",
600+
command: "node",
601+
args: ["test2.js"],
602+
disabled: false,
603+
},
604+
},
605+
}
606+
607+
// Mock reading initial config
608+
vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(mockConfig))
609+
610+
// Mock StdioClientTransport
611+
const mockTransport = {
612+
start: vi.fn().mockResolvedValue(undefined),
613+
close: vi.fn().mockResolvedValue(undefined),
614+
stderr: {
615+
on: vi.fn(),
616+
},
617+
onerror: null,
618+
onclose: null,
619+
}
620+
621+
const StdioClientTransport = (await import("@modelcontextprotocol/sdk/client/stdio.js"))
622+
.StdioClientTransport as ReturnType<typeof vi.fn>
623+
StdioClientTransport.mockClear()
624+
StdioClientTransport.mockImplementation(() => mockTransport)
625+
626+
// Mock Client
627+
const Client = (await import("@modelcontextprotocol/sdk/client/index.js")).Client as ReturnType<
628+
typeof vi.fn
629+
>
630+
Client.mockClear()
631+
Client.mockImplementation(() => ({
632+
connect: vi.fn().mockResolvedValue(undefined),
633+
close: vi.fn().mockResolvedValue(undefined),
634+
getInstructions: vi.fn().mockReturnValue("test instructions"),
635+
request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }),
636+
}))
637+
638+
// Create a new McpHub instance
639+
const newMcpHub = new McpHub(mockProvider as ClineProvider)
640+
641+
// Wait for initialization
642+
await new Promise((resolve) => setTimeout(resolve, 100))
643+
644+
// Verify that only the enabled server was connected
645+
expect(StdioClientTransport).toHaveBeenCalledTimes(1)
646+
expect(StdioClientTransport).toHaveBeenCalledWith(
647+
expect.objectContaining({
648+
args: ["test2.js"], // Only the enabled server
649+
}),
650+
)
651+
})
652+
653+
it("should disconnect server when toggling to disabled", async () => {
654+
const mockConfig = {
655+
mcpServers: {
656+
"test-server": {
657+
type: "stdio",
658+
command: "node",
659+
args: ["test.js"],
660+
disabled: false,
661+
},
662+
},
663+
}
664+
665+
// Mock reading initial config
666+
vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
667+
668+
// Set up mock connection
669+
const mockTransport = {
670+
close: vi.fn().mockResolvedValue(undefined),
671+
}
672+
const mockClient = {
673+
close: vi.fn().mockResolvedValue(undefined),
674+
}
675+
const mockConnection: McpConnection = {
676+
server: {
677+
name: "test-server",
678+
config: JSON.stringify({
679+
type: "stdio",
680+
command: "node",
681+
args: ["test.js"],
682+
disabled: false,
683+
}),
684+
status: "connected",
685+
disabled: false,
686+
source: "global",
687+
},
688+
client: mockClient as any,
689+
transport: mockTransport as any,
690+
}
691+
mcpHub.connections = [mockConnection]
692+
693+
// Toggle to disabled
694+
await mcpHub.toggleServerDisabled("test-server", true)
695+
696+
// Verify the server was disconnected
697+
expect(mockTransport.close).toHaveBeenCalled()
698+
expect(mockClient.close).toHaveBeenCalled()
699+
expect(mcpHub.connections.length).toBe(0)
700+
})
701+
702+
it("should reconnect server when toggling to enabled", async () => {
703+
const mockConfig = {
704+
mcpServers: {
705+
"test-server": {
706+
type: "stdio",
707+
command: "node",
708+
args: ["test.js"],
709+
disabled: true,
710+
},
711+
},
712+
}
713+
714+
// Mock reading initial config
715+
vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
716+
717+
// Mock StdioClientTransport
718+
const mockTransport = {
719+
start: vi.fn().mockResolvedValue(undefined),
720+
close: vi.fn().mockResolvedValue(undefined),
721+
stderr: {
722+
on: vi.fn(),
723+
},
724+
onerror: null,
725+
onclose: null,
726+
}
727+
728+
const StdioClientTransport = (await import("@modelcontextprotocol/sdk/client/stdio.js"))
729+
.StdioClientTransport as ReturnType<typeof vi.fn>
730+
StdioClientTransport.mockImplementation(() => mockTransport)
731+
732+
// Mock Client
733+
const Client = (await import("@modelcontextprotocol/sdk/client/index.js")).Client as ReturnType<
734+
typeof vi.fn
735+
>
736+
const mockClient = {
737+
connect: vi.fn().mockResolvedValue(undefined),
738+
close: vi.fn().mockResolvedValue(undefined),
739+
getInstructions: vi.fn().mockReturnValue("test instructions"),
740+
request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }),
741+
}
742+
Client.mockImplementation(() => mockClient)
743+
744+
// Set up mock connection for disabled server
745+
const mockConnection: McpConnection = {
746+
server: {
747+
name: "test-server",
748+
config: JSON.stringify({
749+
type: "stdio",
750+
command: "node",
751+
args: ["test.js"],
752+
disabled: true,
753+
}),
754+
status: "disconnected",
755+
disabled: true,
756+
source: "global",
757+
},
758+
client: {} as any,
759+
transport: {} as any,
760+
}
761+
mcpHub.connections = [mockConnection]
762+
763+
// Toggle to enabled
764+
await mcpHub.toggleServerDisabled("test-server", false)
765+
766+
// Verify the server was connected
767+
expect(mockClient.connect).toHaveBeenCalled()
768+
})
572769
})
573770

574771
describe("callTool", () => {

‎webview-ui/src/components/mcp/McpView.tsx‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,11 @@ const ServerRow = ({ server, alwaysAllowMcp }: { server: McpServer; alwaysAllowM
218218
]
219219

220220
const getStatusColor = () => {
221+
// Show grey/disabled color for disabled servers regardless of connection status
222+
if (server.disabled) {
223+
return "var(--vscode-disabledForeground)"
224+
}
225+
221226
switch (server.status) {
222227
case "connected":
223228
return "var(--vscode-testing-iconPassed)"

0 commit comments

Comments
 (0)