fix(mcp): stdio/unix transports skip initialize handshake (#890)#935
fix(mcp): stdio/unix transports skip initialize handshake (#890)#935zmanian merged 1 commit intonearai:stagingfrom
Conversation
fixes nearai#890 - Always call initialize() before list_tools()/call_tool(), removing the session_manager.is_some() guard that caused stdio/unix clients to skip the MCP protocol handshake entirely - Add local AtomicBool flag for idempotent initialization when no session manager is present - Fire-and-forget JSON-RPC notifications (id=None) in stdio/unix transports instead of registering a pending response that would block for 30s waiting on a reply that never comes - Fix mcp test panic on stdio/unix servers by using create_client_from_config() instead of new_with_config() which asserts HTTP-only transport
github-actions
bot
added
scope: channel/cli
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 addresses a critical bug in the MCP client where stdio and Unix socket transports were failing to properly initialize due to a conditional guard. The changes ensure that the Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the MCP client initialization to ensure idempotency across all transport types (HTTP, stdio, unix sockets) by introducing an initialized flag in McpClient and adjusting the send_request method to handle JSON-RPC notifications as fire-and-forget. This change also involves using a new client factory function in test_server. However, the review identified two issues: the test_server function leaks child processes for stdio-based servers due to local McpProcessManager creation without proper cleanup, potentially leading to resource exhaustion, and it hardcodes the user_id to "default" when creating clients, which could lead to user isolation issues and potential security vulnerabilities.
| // No OAuth and no tokens - try unauthenticated | ||
| McpClient::new_with_config(server.clone()) | ||
| // Use the factory to dispatch on transport type (HTTP, stdio, unix) | ||
| let process_manager = Arc::new(McpProcessManager::new()); |
There was a problem hiding this comment.
A new McpProcessManager is created locally within the test_server function and dropped when the function returns. Since McpProcessManager and StdioMcpTransport do not automatically kill child processes on drop (and kill_on_drop(true) is not used in the transport implementation), every call to test_server for a stdio-based server will leak a running child process, potentially leading to resource exhaustion.
| &session_manager, | ||
| &process_manager, | ||
| None, | ||
| "default", |
There was a problem hiding this comment.
The user_id argument passed to create_client_from_config is hardcoded to "default", ignoring the user_id parameter provided to the test_server function. While secrets is currently passed as None in this specific call, this pattern breaks user isolation and could lead to incorrect context usage or security bypasses if the factory logic is extended.
References
- Tools that interact with user-owned resources, such as sandbox jobs, must verify that the authenticated user ID in the tool context matches the resource owner's ID (e.g., by using a
ContextManager) before performing any read or write operations to prevent unauthorized cross-user access.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: fix(mcp): stdio/unix transports skip initialize handshake (#890)
Verdict: APPROVE
Core fix is correct and well-structured across 4 files.
Root cause and fix
The session_manager.is_some() guard meant stdio/unix clients (which don't use session managers) never called initialize(). The fix removes this guard and adds a local AtomicBool for idempotency when no session manager is present. Relaxed ordering is fine -- worst case is a redundant no-op initialize call.
Fire-and-forget notifications
Correctly handles JSON-RPC notifications (no id) as fire-and-forget in both stdio_transport.rs and unix_transport.rs. Per the JSON-RPC spec, notifications expect no response -- the old code registered a pending response that would block for 30s waiting on a reply that never comes.
Other changes
cli/mcp.rstest_servernow usescreate_client_from_config()factory -- correct dispatch for all transport types- The
idvariable extraction removes redundant.unwrap_or(0)calls -- nice cleanup - Two good regression tests covering auto-initialization and idempotency
Re: automated review concerns
- Child process leak in
test_server:McpProcessManageris created locally and dropped when the function returns. This is pre-existing behavior, not introduced by this PR. - Hardcoded
user_id = "default":test_serveris a local diagnostic CLI command, not multi-user. Fine here.
Clone behavior
The initialized flag in the Clone impl copies the current value, so cloned clients that were already initialized won't re-initialize. This is correct -- they share the same transport and the server has already been initialized.
No blocking issues found.
|
I see it's already approved — is this good to merge? |
nearai#935) fixes nearai#890 - Always call initialize() before list_tools()/call_tool(), removing the session_manager.is_some() guard that caused stdio/unix clients to skip the MCP protocol handshake entirely - Add local AtomicBool flag for idempotent initialization when no session manager is present - Fire-and-forget JSON-RPC notifications (id=None) in stdio/unix transports instead of registering a pending response that would block for 30s waiting on a reply that never comes - Fix mcp test panic on stdio/unix servers by using create_client_from_config() instead of new_with_config() which asserts HTTP-only transport
2 participants
Summary
fixes #890
Change Type
Linked Issue
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featuresSecurity Impact
Database Impact
Blast Radius
Rollback Plan
Review track: