Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: prevent MCP connection errors when toggling auto-approve on running tools #7190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@roomote roomote bot Aug 18, 2025

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:

// This avoids connection issues when toggling auto-approve on running tools
if (connection && connection.server.tools) {
// Update the local tool's alwaysAllow or enabledForPrompt property
const tool = connection.server.tools.find((t) => t.name === toolName)
if (tool) {
if (listName === "alwaysAllow") {
Copy link
Contributor Author

@roomote roomote bot Aug 18, 2025

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.

tool.alwaysAllow = addTool
} else if (listName === "disabledTools") {
tool.enabledForPrompt = !addTool
}
}
await this.notifyWebviewOfServerChanges()
} else if (connection && connection.type === "connected") {
// Only fetch tools list if we don't have it cached and the connection is active
try {
connection.server.tools = await this.fetchToolsList(serverName, source)
await this.notifyWebviewOfServerChanges()
} 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()
Copy link
Contributor Author

@roomote roomote bot Aug 18, 2025

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.

}
} else {
// For disconnected servers, just notify with current state
await this.notifyWebviewOfServerChanges()
}
}
Expand Down
Loading