-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: prevent MCP connection errors when toggling auto-approve on running tools #7190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ing tools - Modified updateServerToolList to update local tool state without making MCP requests - Added fallback error handling for cases where MCP requests fail - Fixes issue where clicking auto-approve checkbox on running tools caused connection errors Fixes #7189
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
} catch (error) { | ||
// If fetching fails, just notify with current state | ||
console.warn(`Failed to refresh tools list for ${serverName}, using cached state:`, error) | ||
await this.notifyWebviewOfServerChanges() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional that we don't handle the case where tools might be undefined after a failed fetch? The catch block logs a warning but continues, and if connection.server.tools
becomes undefined after the fetch fails, the subsequent code might not behave as expected.
@@ -1690,8 +1690,31 @@ export class McpHub { | |||
|
|||
await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) | |||
|
|||
if (connection) { | |||
connection.server.tools = await this.fetchToolsList(serverName, source) | |||
// Update the local tools list without making an MCP request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment could be more specific about which connection issues this prevents. Consider:
// Update the local tool's alwaysAllow or enabledForPrompt property | ||
const tool = connection.server.tools.find((t) => t.name === toolName) | ||
if (tool) { | ||
if (listName === "alwaysAllow") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consider extracting this duplicated logic into a helper method? This same pattern appears in the fetchToolsList
method. Something like:
This would make the code more maintainable and reduce duplication.
Closing this PR as it doesn't actually fix the root cause. The problem is that when we write to the config file, the file watcher triggers and calls The real fix needs to prevent the server restart when only |
Summary
This PR fixes an issue where clicking the auto-approve checkbox on a running MCP tool would cause a connection error.
Problem
When a user clicks the auto-approve checkbox for an MCP tool that is already running, the system attempts to refresh the tools list by making an MCP request. This can fail if:
This results in the error: "Error executing MCP tool: MCP error -32000: Connection closed"
Solution
Modified the
updateServerToolList
method inMcpHub.ts
to:Testing
Related Issue
Fixes #7189
Important
Fixes MCP connection errors when toggling auto-approve on running tools by updating local state in
updateServerToolList
inMcpHub.ts
.updateServerToolList
inMcpHub.ts
.This description was created by
for 25ecf5d. You can customize this summary. It will automatically update as commits are pushed.