fix(cli): handle optional ping in mcp list#25619
fix(cli): handle optional ping in mcp list#25619jasonmatthewsuhari wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
|
/gemini review |
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 improves the reliability of the MCP server connection checks within the CLI. By aligning timeouts with global defaults and making the ping verification step best-effort, the CLI now correctly identifies and reports servers that are functional but do not support the optional ping protocol. 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 updates the MCP connection testing logic in the CLI. It replaces the hardcoded 5-second timeout with a configurable timeout that defaults to MCP_DEFAULT_TIMEOUT_MSEC. Additionally, it improves the robustness of the connection check by treating MethodNotFound errors during the ping phase as a successful connection, acknowledging that some MCP servers may not implement the ping method. New test cases have been added to verify these behaviors. I have no feedback to provide.
There was a problem hiding this comment.
Code Review
This pull request updates the MCP connection testing logic to use a configurable or shared default timeout (MCP_DEFAULT_TIMEOUT_MSEC) instead of a hardcoded value. It also improves robustness by treating MethodNotFound errors during the ping phase as a successful connection, acknowledging that some MCP servers may not implement the ping method. Corresponding unit tests have been added to verify these behaviors. I have no feedback to provide.
Fixes #25599
Summary
gemini mcp listconnection checks with the shared MCP default timeout instead of a hardcoded 5s timeoutping()as best-effort inmcp listso servers that returnMethodNotFoundafter a successful MCP initialize handshake are still reported as connectedping()caseVerification
https://run.googleapis.com/mcp, wheregemini mcp listreportedDisconnectedbefore the fixcloud-run: https://run.googleapis.com/mcp (http) - ConnectedReproduced:

Resolved:
