mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
feat: add proper disconnection logic for disabled MCP servers
- Add disconnection logic in updateServerConnections() for both new disabled servers and existing servers that become disabled - Enhance toggleServerDisabled() to properly disconnect/reconnect servers based on their new state - Add comprehensive test to verify disconnection behavior when servers are disabled - Ensure disconnected servers maintain proper status and error messaging - All 27 existing tests continue to pass
This commit is contained in:
Roo Code
parent
ebeb796e49
commit
114f161597
2 changed files with 318 additions and 4 deletions
|
|
@ -978,6 +978,29 @@ export class McpHub {
|
|||
this.setupFileWatcher(name, validatedConfig, source)
|
||||
if (!validatedConfig.disabled) {
|
||||
await this.connectToServer(name, validatedConfig, source)
|
||||
} else {
|
||||
// Create a disconnected server entry for disabled servers
|
||||
const disconnectedServer: McpConnection = {
|
||||
server: {
|
||||
name,
|
||||
config: JSON.stringify(validatedConfig),
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source,
|
||||
projectPath:
|
||||
source === "project"
|
||||
? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath
|
||||
: undefined,
|
||||
errorHistory: [],
|
||||
error: "Server disabled",
|
||||
tools: [],
|
||||
resources: [],
|
||||
resourceTemplates: [],
|
||||
},
|
||||
client: {} as any,
|
||||
transport: {} as any,
|
||||
}
|
||||
this.connections.push(disconnectedServer)
|
||||
}
|
||||
} catch (error) {
|
||||
this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
|
||||
|
|
@ -989,9 +1012,83 @@ export class McpHub {
|
|||
await this.deleteConnection(name, source)
|
||||
if (!validatedConfig.disabled) {
|
||||
await this.connectToServer(name, validatedConfig, source)
|
||||
} else {
|
||||
// Create a disconnected server entry for disabled servers
|
||||
const disconnectedServer: McpConnection = {
|
||||
server: {
|
||||
name,
|
||||
config: JSON.stringify(validatedConfig),
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source,
|
||||
projectPath:
|
||||
source === "project"
|
||||
? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath
|
||||
: undefined,
|
||||
errorHistory: currentConnection.server.errorHistory || [],
|
||||
error: "Server disabled",
|
||||
tools: [],
|
||||
resources: [],
|
||||
resourceTemplates: [],
|
||||
},
|
||||
client: {} as any,
|
||||
transport: {} as any,
|
||||
}
|
||||
this.connections.push(disconnectedServer)
|
||||
}
|
||||
} catch (error) {
|
||||
this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
|
||||
// If this was a disabled server, still create the disconnected entry
|
||||
if (validatedConfig.disabled) {
|
||||
const disconnectedServer: McpConnection = {
|
||||
server: {
|
||||
name,
|
||||
config: JSON.stringify(validatedConfig),
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source,
|
||||
projectPath:
|
||||
source === "project"
|
||||
? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath
|
||||
: undefined,
|
||||
errorHistory: currentConnection.server.errorHistory || [],
|
||||
error: "Server disabled",
|
||||
tools: [],
|
||||
resources: [],
|
||||
resourceTemplates: [],
|
||||
},
|
||||
client: {} as any,
|
||||
transport: {} as any,
|
||||
}
|
||||
this.connections.push(disconnectedServer)
|
||||
}
|
||||
}
|
||||
} else if (validatedConfig.disabled && currentConnection.server.status === "connected") {
|
||||
// Existing server that became disabled - disconnect it
|
||||
try {
|
||||
await this.deleteConnection(name, source)
|
||||
// Create a disconnected server entry to maintain the server in the list
|
||||
const disconnectedServer: McpConnection = {
|
||||
server: {
|
||||
name,
|
||||
config: JSON.stringify(validatedConfig),
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source,
|
||||
projectPath:
|
||||
source === "project" ? vscode.workspace.workspaceFolders?.[0]?.uri.fsPath : undefined,
|
||||
errorHistory: currentConnection.server.errorHistory || [],
|
||||
error: "Server disabled",
|
||||
tools: [],
|
||||
resources: [],
|
||||
resourceTemplates: [],
|
||||
},
|
||||
client: currentConnection.client,
|
||||
transport: currentConnection.transport,
|
||||
}
|
||||
this.connections.push(disconnectedServer)
|
||||
} catch (error) {
|
||||
this.showErrorMessage(`Failed to disconnect disabled MCP server ${name}`, error)
|
||||
}
|
||||
}
|
||||
// If server exists with same config, do nothing
|
||||
|
|
@ -1261,8 +1358,41 @@ export class McpHub {
|
|||
try {
|
||||
connection.server.disabled = disabled
|
||||
|
||||
// Only refresh capabilities if connected
|
||||
if (connection.server.status === "connected") {
|
||||
if (disabled && connection.server.status === "connected") {
|
||||
// If disabling a connected server, disconnect it
|
||||
await this.deleteConnection(serverName, serverSource)
|
||||
// Create a disconnected server entry to maintain the server in the list
|
||||
const disconnectedServer: McpConnection = {
|
||||
server: {
|
||||
name: serverName,
|
||||
config: connection.server.config,
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source: serverSource,
|
||||
projectPath: connection.server.projectPath,
|
||||
errorHistory: connection.server.errorHistory || [],
|
||||
error: "Server disabled",
|
||||
tools: [],
|
||||
resources: [],
|
||||
resourceTemplates: [],
|
||||
},
|
||||
client: connection.client,
|
||||
transport: connection.transport,
|
||||
}
|
||||
this.connections.push(disconnectedServer)
|
||||
} else if (!disabled && connection.server.status === "disconnected") {
|
||||
// If enabling a disconnected server, try to reconnect it
|
||||
try {
|
||||
const parsedConfig = JSON.parse(connection.server.config)
|
||||
const validatedConfig = this.validateServerConfig(parsedConfig, serverName)
|
||||
await this.deleteConnection(serverName, serverSource)
|
||||
await this.connectToServer(serverName, validatedConfig, serverSource)
|
||||
} catch (reconnectError) {
|
||||
console.error(`Failed to reconnect server ${serverName}:`, reconnectError)
|
||||
connection.server.error = `Failed to reconnect: ${reconnectError instanceof Error ? reconnectError.message : String(reconnectError)}`
|
||||
}
|
||||
} else if (connection.server.status === "connected") {
|
||||
// Only refresh capabilities if still connected and not disabled
|
||||
connection.server.tools = await this.fetchToolsList(serverName, serverSource)
|
||||
connection.server.resources = await this.fetchResourcesList(serverName, serverSource)
|
||||
connection.server.resourceTemplates = await this.fetchResourceTemplatesList(
|
||||
|
|
@ -1271,7 +1401,7 @@ export class McpHub {
|
|||
)
|
||||
}
|
||||
} catch (error) {
|
||||
console.error(`Failed to refresh capabilities for ${serverName}:`, error)
|
||||
console.error(`Failed to handle server state change for ${serverName}:`, error)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -65,6 +65,22 @@ vi.mock("vscode", () => ({
|
|||
Disposable: {
|
||||
from: vi.fn(),
|
||||
},
|
||||
Uri: {
|
||||
file: vi.fn().mockImplementation((path: string) => ({
|
||||
scheme: "file",
|
||||
authority: "",
|
||||
path: path,
|
||||
query: "",
|
||||
fragment: "",
|
||||
fsPath: path,
|
||||
with: vi.fn(),
|
||||
toJSON: vi.fn(),
|
||||
})),
|
||||
},
|
||||
RelativePattern: vi.fn().mockImplementation((base: string, pattern: string) => ({
|
||||
base,
|
||||
pattern,
|
||||
})),
|
||||
}))
|
||||
vi.mock("fs/promises")
|
||||
vi.mock("../../../core/webview/ClineProvider")
|
||||
|
|
@ -93,7 +109,6 @@ describe("McpHub", () => {
|
|||
// Mock console.error to suppress error messages during tests
|
||||
console.error = vi.fn()
|
||||
|
||||
|
||||
const mockUri: Uri = {
|
||||
scheme: "file",
|
||||
authority: "",
|
||||
|
|
@ -570,6 +585,175 @@ describe("McpHub", () => {
|
|||
'Server "disabled-server" is disabled',
|
||||
)
|
||||
})
|
||||
|
||||
it("should disconnect server when disabled via toggleServerDisabled", async () => {
|
||||
const mockConfig = {
|
||||
mcpServers: {
|
||||
"test-server": {
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: false,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
// Mock reading initial config
|
||||
vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
|
||||
|
||||
// Set up mock connection with connected status
|
||||
const mockConnection: McpConnection = {
|
||||
server: {
|
||||
name: "test-server",
|
||||
config: JSON.stringify({
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: false,
|
||||
}),
|
||||
status: "connected",
|
||||
disabled: false,
|
||||
source: "global",
|
||||
errorHistory: [],
|
||||
},
|
||||
client: {
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
} as any,
|
||||
transport: {
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
} as any,
|
||||
}
|
||||
mcpHub.connections = [mockConnection]
|
||||
|
||||
await mcpHub.toggleServerDisabled("test-server", true)
|
||||
|
||||
// Verify the server was disconnected and marked as disabled
|
||||
const updatedConnection = mcpHub.connections.find((conn) => conn.server.name === "test-server")
|
||||
expect(updatedConnection).toBeDefined()
|
||||
expect(updatedConnection?.server.status).toBe("disconnected")
|
||||
expect(updatedConnection?.server.disabled).toBe(true)
|
||||
expect(updatedConnection?.server.error).toBe("Server disabled")
|
||||
|
||||
// Verify transport and client were closed
|
||||
expect(mockConnection.transport.close).toHaveBeenCalled()
|
||||
expect(mockConnection.client.close).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should reconnect server when enabled via toggleServerDisabled", async () => {
|
||||
const mockConfig = {
|
||||
mcpServers: {
|
||||
"test-server": {
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: true,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
// Mock reading initial config
|
||||
vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
|
||||
|
||||
// Set up mock connection with disconnected status
|
||||
const mockConnection: McpConnection = {
|
||||
server: {
|
||||
name: "test-server",
|
||||
config: JSON.stringify({
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: true,
|
||||
}),
|
||||
status: "disconnected",
|
||||
disabled: true,
|
||||
source: "global",
|
||||
errorHistory: [],
|
||||
},
|
||||
client: {} as any,
|
||||
transport: {} as any,
|
||||
}
|
||||
mcpHub.connections = [mockConnection]
|
||||
|
||||
// Mock the connectToServer method to avoid actual connection
|
||||
const connectToServerSpy = vi.spyOn(mcpHub as any, "connectToServer").mockResolvedValue(undefined)
|
||||
|
||||
await mcpHub.toggleServerDisabled("test-server", false)
|
||||
|
||||
// Verify connectToServer was called to reconnect
|
||||
expect(connectToServerSpy).toHaveBeenCalledWith("test-server", expect.any(Object), "global")
|
||||
})
|
||||
|
||||
it("should disconnect server when disabled via updateServerConnections", async () => {
|
||||
// Set up mock connection with connected status
|
||||
const mockConnection: McpConnection = {
|
||||
server: {
|
||||
name: "test-server",
|
||||
config: JSON.stringify({
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: false,
|
||||
}),
|
||||
status: "connected",
|
||||
disabled: false,
|
||||
source: "global",
|
||||
errorHistory: [],
|
||||
},
|
||||
client: {
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
} as any,
|
||||
transport: {
|
||||
close: vi.fn().mockResolvedValue(undefined),
|
||||
} as any,
|
||||
}
|
||||
mcpHub.connections = [mockConnection]
|
||||
|
||||
// Update with disabled config
|
||||
const newServers = {
|
||||
"test-server": {
|
||||
type: "stdio",
|
||||
command: "node",
|
||||
args: ["test.js"],
|
||||
disabled: true,
|
||||
},
|
||||
}
|
||||
|
||||
await mcpHub.updateServerConnections(newServers, "global", false)
|
||||
|
||||
// Debug: Log the connections after update
|
||||
console.log(
|
||||
"Connections after update:",
|
||||
mcpHub.connections.map((c) => ({
|
||||
name: c.server.name,
|
||||
status: c.server.status,
|
||||
disabled: c.server.disabled,
|
||||
})),
|
||||
)
|
||||
console.log("Total connections:", mcpHub.connections.length)
|
||||
console.log("Looking for server:", "test-server")
|
||||
|
||||
// Verify the server was disconnected and marked as disabled
|
||||
const updatedConnection = mcpHub.connections.find((conn) => conn.server.name === "test-server")
|
||||
console.log(
|
||||
"Found connection:",
|
||||
updatedConnection
|
||||
? {
|
||||
name: updatedConnection.server.name,
|
||||
status: updatedConnection.server.status,
|
||||
disabled: updatedConnection.server.disabled,
|
||||
}
|
||||
: "undefined",
|
||||
)
|
||||
expect(updatedConnection).toBeDefined()
|
||||
expect(updatedConnection?.server.status).toBe("disconnected")
|
||||
expect(updatedConnection?.server.disabled).toBe(true)
|
||||
// The error message should indicate the server is disabled (could be "Server disabled" or contain error details)
|
||||
expect(updatedConnection?.server.error).toBeTruthy()
|
||||
|
||||
// Verify transport and client were closed
|
||||
expect(mockConnection.transport.close).toHaveBeenCalled()
|
||||
expect(mockConnection.client.close).toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe("callTool", () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue