-
Notifications
You must be signed in to change notification settings - Fork 349
Description
Describe the bug
I'm a developer/maintainer of an application that's utilizing this crate for MCP client functionality. We've gotten a couple reports from our users that their stdio-based MCP servers fail to start correctly.
After debugging, I've discovered two initialization failure modes (so far) that are fatal:
- If the server prints out log lines to stdout instead of stderr, the handshake fails.
@circleci/mcp-server-circleci
is an example of this - it prints out a message as soon as it starts, whichrmcp
tries to parse as anInitializeResult
.
- If the server sends notifications before the initialization handshake completes.
effect-mcp
fails in this way - it emitsresources/list_changed
andtools/list_changed
notifications as soon as it starts up.
To Reproduce
Steps to reproduce the behavior:
- Try using this crate's stdio transport client support to connect to either of the above servers (
npx -y @circleci/mcp-server-circleci
ornpx -y effect-mcp
). - Notice that both of them will fail during the initialization handshake.
Expected behavior
In an ideal world, this crate would handle such errors more gracefully.
In our fork, I've made two changes to be more robust to these sorts of non-conforming servers:
- In the
decode()
implementation forJsonRpcMessageCodec
, we read a new message iftry_parse_with_compatibility()
returns an error, instead of propagating that error to the caller.-
This fixes the first case (server logging to stdout instead of stderr).
-
The spec is very clear that this should not be necessary. In the stdio transport spec, it states:
The server MUST NOT write anything to its
stdout
that is not a valid MCP message.Despite this, it may be worth being resilient to servers that do not comply here.
-
- In
serve_client_with_ct_inner
, I added aloop
around the call toexpect_response
and the associated response checking logic, only breaking out of the loop once we receive anInitializeResult
.-
This fixes the second case (notifications emitted before the handshake completes).
-
The spec is less restrictive about server behavior here. It states:
The server SHOULD NOT send requests other than pings and logging before receiving the
initialized
notification.Notably, it says "SHOULD NOT" and not "MUST NOT" - given a server is technically permitted to emit other valid JSONRPC messages before completing the initialization handshake, this library should probably be resilient to them.
-
I'm happy to open PRs for the two changes I've made in our fork, if that would be a helpful starting point for discussing how best to address these issues.