Initial support for reloading extensions in the CLI - mcp servers only#12239
Initial support for reloading extensions in the CLI - mcp servers only#12239
Conversation
…disabled or unloaded/loaded
…rnal gemini state for tools
Summary of ChangesHello @jakemac53, 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 implements initial support for dynamically reloading extensions in the interactive CLI, specifically for MCP (Model Context Protocol) servers. This enhancement allows the CLI to respond to extension updates, enabling, or disabling by seamlessly unloading and reloading the affected MCP servers and their associated tools without needing a full application restart. The feature is opt-in via an experimental setting and includes robust handling for discovery state management to ensure stability during these dynamic operations. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces experimental support for reloading extensions in the interactive CLI, which is a great enhancement for developer experience. The implementation correctly wires the new extensionReloading setting through the configuration and UI components. The addition of an integration test is also very valuable for such a cross-cutting feature.
My review focuses on a couple of critical race conditions in the McpClientManager that could lead to incorrect state when loading or unloading extensions with multiple MCP servers. I've also pointed out a potential bug in the UI that could display "undefined" to the user, and a source of potential flakiness in the new integration test. Addressing these issues will make the feature more robust.
| await run.expectText( | ||
| 'Extension "test-extension" successfully updated: 0.0.1 → 0.0.2', | ||
| ); | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
Using a fixed setTimeout can lead to flaky tests, especially in a CI environment where performance can vary. If the reload operation takes longer than 1 second, the test will fail. Conversely, if it's much faster, the test is unnecessarily slow.
A more robust approach would be to poll for a specific condition that indicates the reload is complete. For instance, you could repeatedly run /extensions list and check the output until the version is v0.0.2 or the status is (updated). Your TestRig or InteractiveRun could be enhanced with a polling helper for this purpose.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { expect, test } from 'vitest'; |
There was a problem hiding this comment.
Can you test when env vars are changed?
There was a problem hiding this comment.
Oh interesting, so like if the extension has settings, and on update you have to add new settings, does it pick those up?
| } | ||
|
|
||
| /** | ||
| * For all the MCP servers associated with this extension: |
There was a problem hiding this comment.
Wondering if we can/should store session IDs for http connections to persist
There was a problem hiding this comment.
Hmm yeah that could be interesting but I think probably isn't worth the complexity right now. But, worth filing a bug to track?
There was a problem hiding this comment.
Sg-- I think unexpectedly losing state for a user is something we'd want to be careful to avoid
|
How can we ensure the prompt is accurate and not polluted by the previous state? |
I was trying to avoid using the model in these tests but I could actually make it call into the model - basically ask it to make a tool call to the old tool, and check that it can't do that. Or, ask it what tools it has available. |
|
Another option regarding the tests is I could try setting up a unit test but one that doesn't many or even any mocks, and then I could potentially validate the internals a bit better (like the exact tool list available to the GeminiChat). |
…appears and can be missed
|
Size Change: +5.06 kB (+0.02%) Total Size: 20.3 MB
ℹ️ View Unchanged
|
|
Ok, I took a look at trying to do more of a unit test but I ran into a number of issues. Abandoning that for now, integration test it is. |
…he CLI - mcp servers only (google-gemini#12239)
Summary
Adds support for unloading and reloading extensions MCP servers within the interactive CLI, based on events sent from the ExtensionLoader.
Details
The only time this can happen today is in response to an
/extensions updatecommand, which causes an "unload" and then "load" event.On "unload" or "disable" we will shut down all previous MCP servers from an extension, and clear the tools from the tool manager and gemini chat context.
On "load" or "enable" we start up any MCP servers from an extension, and add its tools to the tool manager and gemini chat context.
This functionality is guarded behind an
experimental.extensionReloadingsetting.I also updated the discovery state management in the McpClientManager to avoid race conditions.
Because this is so intertwined across different systems, I wrote an integration test instead of unit tests. If you would like, I could also add unit tests for some of it, but they won't be very valuable imo.
Related Issues
First part of #11108
How to Validate
Install an extension, add an update to it (add a new mcp server), and update it using
/extensions update <your-extension>. You should see its status show as updated, and if running/mcp listyou should see the new mcp server connected. You should also be able to use gemini to invoke those tools.Pre-Merge Checklist