fix(core): implement atomic update in MCP tool discovery

[luisfelipe-alt]

Implement atomic update in refreshTools to retain MCP tools during transient failures

Summary

Fixed the "tool not found" error that occurs during transient network drops by ensuring an atomic update pattern for MCP tools. This ensures that the tool registry remains populated with the last known good state
if a background refresh attempt fails (e.g., due to a network drop).

Details

• Exception Propagation in discoverTools: Modified discoverTools to throw an error when discovery fails (e.g., due to a network timeout) instead of silently returning an empty list.
• Atomic Update in refreshTools: By propagating the error, the refreshTools loop now correctly aborts before clearing the registry. This ensures that existing tools are only replaced if a new set of tools
  is successfully discovered.
• Architectural Integrity: This approach correctly distinguishes between a server intentionally removing all tools (by returning an empty list) and a failure to communicate with the server (which now preserve
  s the current state).
• Test Coverage: Updated mcp-client.test.ts to verify that the registry is not cleared when a refresh fails and tightened existing assertions for the disconnect lifecycle.

Related Issues

How to Validate

Run the following commands to verify the fix and ensure no regressions:

• npx vitest run packages/core/src/tools/mcp-client.test.ts
• npm run build

Pre-Merge Checklist

• Updated relevant documentation and README (if needed)
• Added/updated tests (if needed)
• Noted breaking changes (if any)
• Validated on required platforms/methods:
  • MacOS
    • npm run
    • npx
    • Docker
    • Podman
    • Seatbelt
  • Windows
    • npm run
    • npx
    • Docker
  • Linux
    • npm run
    • npx
    • Docker

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 improves the robustness of MCP tool discovery by implementing an atomic update mechanism. By ensuring that the tool registry is not cleared when a background refresh fails due to transient issues, the system maintains its last known good state. This change also includes updated test cases to validate the new error handling and registry behavior.

Highlights

• Atomic Tool Updates: Implemented an atomic update pattern in refreshTools to ensure the tool registry is only updated upon successful discovery, preventing data loss during transient network failures.
• Error Propagation: Modified discoverTools to propagate errors instead of returning an empty list, allowing the refresh loop to abort gracefully when discovery fails.
• Test Coverage: Updated mcp-client.test.ts to verify that the registry remains intact during failed refreshes and tightened assertions for the disconnect lifecycle.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[luisfelipe-alt]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the discoverTools function in packages/core/src/tools/mcp-client.ts to rethrow the caught error during tool discovery. Corresponding unit tests in packages/core/src/tools/mcp-client.test.ts have been updated to assert that server-specific resources are removed upon disconnection, and that tools are not removed when discovery fails. There are no review comments, so I have no feedback to provide.

[gemini-code-assist]

Code Review

This pull request updates the discoverTools function in packages/core/src/tools/mcp-client.ts to throw an error when tool discovery fails, rather than swallowing it. Correspondingly, unit tests in packages/core/src/tools/mcp-client.test.ts have been updated to assert that tools are not removed during a failed discovery (atomic refresh) and to ensure that disconnect registry removals are called with the correct server name. There are no review comments, and I have no feedback to provide.

[luisfelipe-alt]

Let me provide my answers of your comments in previous PR

🚨 Critical Concerns (P0/P1)
Action required before merging.

packages/core/src/tools/mcp-client.ts:287: The PR description explicitly claims to have "Removed the eager registry wipe in disconnect()", but this change is completely missing from the diff. The disconnect() method still loops over registries and calls removeMcpToolsByServer(). If a transient network drop triggers a full disconnect/reconnect lifecycle, tools will still be lost. You must either remove the wipe from disconnect() (and update the tests) or clarify the connection lifecycle.

Why the Current Code is Correct (Clarifying the Connection Lifecycle):

1. What happens during a network drop: When a network connection drops, the underlying MCP client triggers an onerror event. In mcp-client.ts (inside the connect() method), this onerror handler simply updates the internal status to MCPServerStatus.DISCONNECTED. It does not call the disconnect() method.
2. When is disconnect() actually called? The disconnect() method is an explicit shutdown action. It is only called by the McpClientManager when the user explicitly disables a server, when an extension is stopped, or when the entire CLI application is shutting down. In these cases, we want the registry to be wiped to prevent stale state.
3. How the throw solves the bug (b-488476022.md): The bug reported that tools were lost during transient drops. This happened because a background refresh (refreshTools()) would trigger, attempt to call discoverTools(), fail due to the network drop, and previously return an empty list, which caused refreshTools() to eagerly wipe the registry.
   * By adding the throw error in discoverTools(), we implemented an Atomic Update.
   * Now, if a network drop occurs during a refresh, discoverTools() throws an error.
   * The catch block in refreshTools() catches this error and aborts the refresh process before it reaches the removeMcpToolsByServer() line.
   * As a result, the existing tools remain safely in the registry, solving the issue reported by the user.

Conclusion: The current code is architecturally sound. It preserves tools during transient network failures (via exception propagation in the refresh loop) while correctly cleaning up state during explicit shutdowns (via the disconnect() method). No further code changes are needed.

🧹 Refactoring & Nits (P2/P3)
Recommended improvements.

packages/core/src/tools/mcp-client.test.ts: The update from .toHaveBeenCalledOnce() to .toHaveBeenCalledWith('test-server') is a good tightening of the test assertions. No changes needed here unless disconnect() is updated.

Explanation of the Test Tightening:

1. Precision: Changing .toHaveBeenCalledOnce() to .toHaveBeenCalledWith('test-server') ensures that not only is the cleanup method called, but it is called with the correct server name. This prevents a false positive where the method might be called for the wrong server.
2. Robustness: It verifies that the McpClient is correctly identifying itself to the registries during the cleanup process.

How the Current Code Solves the Bug (b-488476022.md):
The core of the fix is the throw error added to the discoverTools() method. This change alone is sufficient to solve the "tool not found" issue without modifying the disconnect() method.

• The Problem: Previously, if a network drop occurred during a background refresh, discoverTools() would fail and return an empty list. The refresh loop would then proceed to wipe the registry, thinking the
  server intentionally removed all tools. This led to the "tool not found" error for the user.
• The Solution (Atomic Update): By throwing an error instead of returning an empty list, we prevent the refresh loop from reaching the cleanup code. The catch block in refreshTools() aborts the update, leaving
  the last known good state (the existing tools) in the registry.
• Consistency with disconnect(): As explained previously, disconnect() is for explicit shutdowns where we want to clear the registry. Since transient network drops do not call disconnect(), keeping the cleanup
  logic there is correct and does not cause the reported bug.

Conclusion: The current implementation is complete and correct. The test updates provide better verification of the existing (and desired) cleanup behavior during explicit disconnections.

📝 Metadata Review
Feedback on PR description or commit message clarity.

Out of Sync Description: The PR description claims the atomic update uses a length check (if (newTools.length > 0)). The actual implementation correctly uses exception propagation (throw error in discoverTools). While the implemented approach is architecturally superior (it correctly allows a server to intentionally remove all tools), the PR description is misleading and needs to be updated to reflect the final code.

Correct. The PR description has been synchronized with the final code to correctly describe the exception propagation approach.

[github-actions]

✅ 72 tests passed successfully on gemini-3-flash-preview.

🧠 Model Steering Guidance

This PR modifies files that affect the model's behavior (prompts, tools, or instructions).

• ⚠️ Consider adding Evals: No behavioral evaluations (evals/*.eval.ts) were added or updated in this PR. Consider adding a test case to verify the new behavior and prevent regressions.
• 🚀 Maintainer Reminder: Please ensure that these changes do not regress results on benchmark evals before merging.

---

This is an automated guidance message triggered by steering logic signatures.

[github-actions]

📊 PR Size: size/S

• Lines changed: 17
• Additions: +12
• Deletions: -5
• Files changed: 2

[gemini-cli]

Hi there! Thank you for your interest in contributing to Gemini CLI.

To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'.

This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding.