feat(cli): add /mcp remove slash command for interactive server removal#26305
feat(cli): add /mcp remove slash command for interactive server removal#26305martin-hsu-test wants to merge 1 commit into
Conversation
Adds a new `/mcp remove <server-name> [--scope user|workspace|all]` slash command (alias `rm`) that removes an MCP server entry from user or workspace settings without leaving the CLI session. - If the server is only defined in one scope it is removed automatically. - If defined in BOTH workspace and user scopes the command refuses and asks the user to disambiguate with `--scope user`, `--scope workspace`, or `--scope all`. This avoids silently leaving a zombie copy in the other scope. - Servers contributed by extensions are rejected with a hint to use /extensions instead. - After removal the MCP client manager is restarted so the server is disconnected and its tools become unavailable immediately. No core API changes — uses the existing `McpClientManager.restart()` path. Closes google-gemini#18388
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the MCP lifecycle management within the CLI by adding the ability to remove configured servers interactively. By providing a dedicated command for removal, it eliminates the need for manual JSON configuration editing, while incorporating safety checks to prevent configuration drift and ensure immediate synchronization of the MCP client state. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the /mcp remove (alias /mcp rm) command, enabling users to delete MCP server configurations directly from the CLI. The implementation handles scope-specific removals (user, workspace, or both) and includes corresponding documentation and unit tests. Feedback focuses on a critical security and functional issue: the code currently uses resolved settings (with expanded environment variables) when modifying the configuration. This could result in leaking secrets into the settings.json file and breaking future variable expansion. It is recommended to use originalSettings to ensure raw configuration is preserved and to remove redundant nullish coalescing operators per project guidelines.
| const foundIn: RemovableScope[] = []; | ||
| for (const scope of scopeOrder) { | ||
| const file = settings.forScope(scope); | ||
| const scoped = (file.settings.mcpServers ?? {}) as Record<string, unknown>; |
There was a problem hiding this comment.
This check should use file.originalSettings.mcpServers instead of file.settings.mcpServers. The settings property contains values after environment variable expansion, whereas originalSettings contains the raw configuration from the file. Using originalSettings is more consistent with the intent of identifying which physical file contains the definition. Additionally, redundant nullish coalescing operators should be removed as the schema serves as the source of truth for defaults.
| const scoped = (file.settings.mcpServers ?? {}) as Record<string, unknown>; | |
| const scoped = file.originalSettings.mcpServers as Record<string, unknown>; |
References
- Rely on the schema as the single source of truth for configuration defaults, avoiding redundant nullish coalescing operators.
|
|
||
| for (const scope of targetScopes) { | ||
| const file = settings.forScope(scope); | ||
| const scoped = (file.settings.mcpServers ?? {}) as Record<string, unknown>; |
There was a problem hiding this comment.
Using file.settings.mcpServers here is a critical issue. The settings object contains resolved environment variables (e.g., actual API keys instead of $MY_KEY). When settings.setValue is called, it updates originalSettings with these resolved values, which are then persisted to disk. This leaks secrets into the settings.json file and breaks environment variable expansion for future use. You must use file.originalSettings.mcpServers to ensure the raw configuration is preserved. Redundant nullish coalescing has been removed per repository rules.
| const scoped = (file.settings.mcpServers ?? {}) as Record<string, unknown>; | |
| const scoped = file.originalSettings.mcpServers as Record<string, unknown>; |
References
- Use a sanitized environment for variable expansion in stdio-based MCP server configurations to prevent extensions from bypassing environment variable redaction.
- Rely on the schema as the single source of truth for configuration defaults, avoiding redundant nullish coalescing operators.
| forScope: (scope: SettingScope) => ({ | ||
| settings: { mcpServers: scopeContents[scope] ?? {} }, | ||
| }), |
There was a problem hiding this comment.
The mock should include originalSettings to align with the implementation changes needed to avoid environment variable leakage. The handleRemove function should operate on the raw settings to preserve placeholders. Redundant nullish coalescing operators have been removed.
| forScope: (scope: SettingScope) => ({ | |
| settings: { mcpServers: scopeContents[scope] ?? {} }, | |
| }), | |
| forScope: (scope: SettingScope) => ({ | |
| settings: { mcpServers: scopeContents[scope] }, | |
| originalSettings: { mcpServers: scopeContents[scope] }, | |
| }), |
References
- Rely on the schema as the single source of truth for configuration defaults, avoiding redundant nullish coalescing operators.
- BerriAI/litellm#27059 (Grok 4.20 azure_ai metadata) merge-after-nits - QwenLM/qwen-code#3743 (path-vs-slash-command classifier) merge-after-nits - QwenLM/qwen-code#3767 (capture actual wire request in OpenAI logger) merge-after-nits - google-gemini/gemini-cli#26306 (bound retry fallback to prevent infinite loop) merge-after-nits - google-gemini/gemini-cli#26305 (/mcp remove slash command) merge-after-nits
scidomino
left a comment
There was a problem hiding this comment.
Address the gemni comments which seem valid and mark them as resolved when you are done.
|
Thank you for your interest in contributing to the project! We are closing this PR due to inactivity. |
Adds the missing third corner of the
/mcplifecycle:/mcp add(existing — via settings file)/mcp enable | disable(existing)/mcp remove(this PR) — interactive removal without leaving the sessionCloses the asymmetry where adding an MCP is one step but removing requires manual JSON editing.
What
Aliases:
/mcp rm <name>.Behavior
--scope workspace,--scope user, or--scope all). This avoids the silent "zombie config" pitfall where deleting one copy leaves a forgotten copy in the other scope still active after the next restart./extensionsinstead (they live in the extension manifest, not in user/workspace settings).McpClientManager.restart()is called so the server is disconnected and its tools become unavailable immediately.Why
--scope(not--session)Mirrors the existing
gemini mcp remove --scopeCLI subcommand naming.--session(used byenable/disable) refers to in-memory persistence — orthogonal to the file-location concept thatremovecares about.Notes
McpClientManager.restart()path; no need to add a newremoveServer()method.cli/srcplus its test plus a docs section. Smallest possible surface.--scope user|workspace|allpaths, invalid--scopevalue, completion filtering.npm run preflightis green.Context
Previous attempt at this issue: #19025 (closed after CI failed and the contributor went inactive for 26 days). This PR is a clean-room rewrite against current
mainrather than a rebase — different design (no--scope all/zombie-detection, no core API change, no UI/snapshot churn) and a fresh test suite.