Skip to content

Conversation

vorporeal
Copy link
Contributor

@vorporeal vorporeal commented Sep 11, 2025

Motivation and Context

As reported in #412, a non-conforming stdio transport-based server can break this client library by writing non-JSONRPC text to stdout before the initialization handshake.

This changes the JsonRpcMessageCodec::decode to ignore lines that don't parse as JSONRPC instead of propagating up an error.

How Has This Been Tested?

Tested this in Warp, using the @circleci/mcp-server-circleci server. We've had this change live in production (via a fork) for a week now and have not received any reports from users of it causing issues.

Breaking Changes

No, this does not affect any public APIs, and should strictly improve compatibility with stdio-based MCP servers.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Important

I don't love this change. While it is effective (and was a good quick fix in our fork), it ultimately makes behavioral decisions in parsing code that belong in calling code.

I think the better fix here is to change the signature of Transport::receive() to produce Result<RxJsonRpcMessage<R>, _> instead of Option<RxJsonRpcMessage<R>>, so that we can surface errors to the service layer and let it make decisions. For example, the service layer might want to ignore decoding errors (and read another message), but handle I/O errors.

Figured I'd start by sending this change as-is in case y'all are ok with it as a quick fix for the problem, and want to make the API change later (given that it would be a semver breaking change).

@alexhancock
Copy link
Contributor

I am not sure about this - I am of two minds:

  • Having the client ignore things that aren't valid mcp messages would be convenient for servers we don't control and are outputting invalid content to their stdout.
  • But then I think it helps us hold the line that servers should only output valid mcp messages to have the client error when a server does so. The python sdk and others also do this.

It's actually been helpful for us at Block/goose to find servers that are logging extraneous info and clean them up.

What do you think? Also cc @jokemanfire @4t145 @jamadeo

@jamadeo
Copy link
Collaborator

jamadeo commented Oct 1, 2025

Yeah, this is tricky. It's so easy to write to stdout and break the contract within a server that it becomes a common bug. The fact that the inspector does not have a problem with it probably makes the issue worse.

I tried running an example client from the python sdk which hung when I sent it invalid messages, which seems even worse than killing the connection. I haven't tried a typescript client yet but from looking at the code it looks like it lets you specify an onerror handler.

That might be the way to go: make this configurable. The default could be what it is now (a connection-closing error) and users could optionally supply a handler to log/ignore/do whatever.

@vorporeal
Copy link
Contributor Author

Yeah, making it configurable would be a reasonable middle-ground.

I'd love to hold the line on "this violates the spec, therefore it is an error" but in practice, server authors are going to fail to comply with the spec, and users will (rightfully) blame us for failing to talk to the server when other products do it successfully (due to more permissive client implementations).

@jokemanfire
Copy link
Collaborator

From the perspective of SDK developers, I tend to follow strict standards. If the errors of application developers like this can be ignored and users can run normally under certain conditions, it is incorrect for SDK developers because they hide certain errors. I think we should prevent the transmission of errors, even though sometimes it may seem normal from a business perspective.

@alexhancock
Copy link
Contributor

alexhancock commented Oct 9, 2025

I agree with @jokemanfire on this and share this view. Making it so the SDK allows things that are not valid MCP messages to pass encourages implementation of servers which violate the spec, and may not work with other language's client SDKs.

When the MCP spec says:

Writing to stdout will corrupt the JSON-RPC messages and break your server.

For HTTP-based servers: Standard output logging is fine since it doesn’t interfere with HTTP responses.

https://modelcontextprotocol.io/docs/develop/build-server#logging-in-mcp-servers

I don't think it makes sense for us to ignore things that violate this in one SDK.

If there was a change to the spec (https://modelcontextprotocol.io/community/governance#specification-enhancement-proposal-sep) that changed this, there was a standard approach to dealing with invalid messages the community could agree on, and we implement it in all the SDKs then it could make sense. For now at the SDK level, I think it's best to simply adhere to the spec and also what other languages are doing.

@alexhancock alexhancock closed this Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-core Core library changes T-transport Transport layer changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants