Skip to content

fix: prevent MCP server restart when toggling tool auto-approve #7262

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 server restart when toggling tool auto-approve #7262

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
57 changes: 47 additions & 10 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,28 @@ export class McpHub {
})
}

/**
* Compares two server configurations to determine if a restart is needed.
* Excludes alwaysAllow and disabledTools from comparison as these don't require restart.
* @param config1 First configuration to compare
* @param config2 Second configuration to compare
* @returns true if configurations are equal (no restart needed), false otherwise
*/
private configsEqualExcludingToolSettings(config1: any, config2: any): boolean {
Copy link
Contributor Author

@roomote roomote bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks good, but it's missing test coverage. Since this is critical functionality that prevents unnecessary server restarts, could we add tests to verify:

  • Configs are considered equal when only alwaysAllow/disabledTools differ
  • Configs trigger restart when other properties change
  • Edge cases like undefined or null configs are handled

// Create copies of the configs without alwaysAllow and disabledTools
const config1Copy = { ...config1 }
Copy link
Contributor Author

@roomote roomote bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding defensive programming here. What if config1 or config2 are null/undefined? The spread operator would throw. Maybe add:

const config2Copy = { ...config2 }

// Remove fields that don't require server restart
delete config1Copy.alwaysAllow
Copy link
Contributor Author

@roomote roomote bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion: These magic strings ('alwaysAllow', 'disabledTools') appear in multiple places. Consider extracting them as constants at the top of the file for maintainability:

delete config1Copy.disabledTools
delete config2Copy.alwaysAllow
delete config2Copy.disabledTools

// Compare the remaining fields
return deepEqual(config1Copy, config2Copy)
}

async updateServerConnections(
newServers: Record<string, any>,
source: "global" | "project" = "global",
Expand Down Expand Up @@ -1071,17 +1093,32 @@ export class McpHub {
} 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 {
// Only setup file watcher for enabled servers
if (!validatedConfig.disabled) {
this.setupFileWatcher(name, validatedConfig, source)
} else {
// Use smart comparison that excludes alwaysAllow and disabledTools
const currentConfig = JSON.parse(currentConnection.server.config)
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider wrapping the JSON.parse of the stored config in a try/catch block to defensively handle any unexpected parsing issues, even though the config is expected to be valid.

This comment was generated because it violated a code review rule: irule_PTI8rjtnhwrWq6jS.

const needsRestart = !this.configsEqualExcludingToolSettings(currentConfig, config)

if (needsRestart) {
// Existing server with changed config that requires restart
try {
// Only setup file watcher for enabled servers
if (!validatedConfig.disabled) {
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 {
// Only tool settings changed, update without restart
// Update the stored config to reflect the new alwaysAllow/disabledTools
currentConnection.server.config = JSON.stringify(config)

// Refresh the tools list to reflect the new settings
if (currentConnection.server.status === "connected") {
Copy link
Contributor Author

@roomote roomote bot Aug 20, 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 only refresh tools when status is 'connected'? What happens if the server is in 'connecting' state when tool settings change? Should we queue the refresh or handle other states?

currentConnection.server.tools = await this.fetchToolsList(name, source)
Copy link
Contributor Author

@roomote roomote bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If fetchToolsList fails here, the error will be silently swallowed. Should we add error handling to notify the user that the tools list couldn't be refreshed? Something like:
Failed to refresh tools list for

}
await this.deleteConnection(name, source)
await this.connectToServer(name, validatedConfig, source)
} catch (error) {
this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
}
}
// If server exists with same config, do nothing
Expand Down
Loading