mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
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
This commit is contained in:
parent
8c349767fa
commit
ec4b453417
2 changed files with 112 additions and 11 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue