fix: prevent MCP server restart when toggling tool auto-approve #7262
Conversation
- Added configsEqualExcludingToolSettings helper to compare configs without alwaysAllow and disabledTools - Modified updateServerConnections to use smart comparison that only restarts servers when essential config changes - When only tool settings change, update config and refresh tools list without restarting the server - Fixes #7189 where MCP connections were closing when clicking auto-approve checkbox
dosubot
bot
added
size:M
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| this.setupFileWatcher(name, validatedConfig, source) | ||
| } else { | ||
| // Use smart comparison that excludes alwaysAllow and disabledTools | ||
| const currentConfig = JSON.parse(currentConnection.server.config) |
There was a problem hiding this comment.
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.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| * @param config2 Second configuration to compare | ||
| * @returns true if configurations are equal (no restart needed), false otherwise | ||
| */ | ||
| private configsEqualExcludingToolSettings(config1: any, config2: any): boolean { |
There was a problem hiding this comment.
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
| currentConnection.server.config = JSON.stringify(config) | ||
|
|
||
| // Refresh the tools list to reflect the new settings | ||
| if (currentConnection.server.status === "connected") { |
There was a problem hiding this comment.
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?
|
|
||
| // Refresh the tools list to reflect the new settings | ||
| if (currentConnection.server.status === "connected") { | ||
| currentConnection.server.tools = await this.fetchToolsList(name, source) |
There was a problem hiding this comment.
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
| */ | ||
| private configsEqualExcludingToolSettings(config1: any, config2: any): boolean { | ||
| // Create copies of the configs without alwaysAllow and disabledTools | ||
| const config1Copy = { ...config1 } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Minor suggestion: These magic strings ('alwaysAllow', 'disabledTools') appear in multiple places. Consider extracting them as constants at the top of the file for maintainability:
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
After reviewing this PR, I've identified a simpler and more robust solution: Instead of comparing configs and excluding specific fields, we should prevent server restarts entirely when any MCP tools are currently running. Since we already track tool execution status in the UI (showing "Running"), we can use this information to block restarts during active tool calls. This approach:
Closing this PR to implement the cleaner solution. Will update the issue with proper scoping. |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Problem
When the auto-approve checkbox is clicked for an MCP tool:
updateServerToolListwrites the config file with updatedalwaysAllowordisabledToolsarrayshandleConfigFileChangeupdateServerConnectionswhich usesdeepEqualto compare configsSolution
Modified
updateServerConnectionsinMcpHub.tsto be smarter about when to restart servers:configsEqualExcludingToolSettingsthat compares configs while excludingalwaysAllowanddisabledToolsarrayscommand,args,url, etc.) have changedalwaysAllowordisabledToolschanged, just update the local state and refresh the tools list without restartingThis way, toggling auto-approve won't restart the server and interrupt running tools.
Testing
Fixes #7189
Important
Prevents unnecessary MCP server restarts when only tool settings change by updating
updateServerConnectionsinMcpHub.ts.alwaysAllowordisabledToolschange inupdateServerConnectionsinMcpHub.ts.configsEqualExcludingToolSettingsto compare configs excludingalwaysAllowanddisabledTools.This description was created by
for 04d29fa. You can customize this summary. It will automatically update as commits are pushed.