Repository navigation
Conversation
A POST carrying only notifications/responses/errors awaits no reply, so nothing in the response is load-bearing. The client accepted only 202/204, a literal Content-Length: 0, or JSON/SSE content types; a 200 with text/plain, or a chunked empty body with no Content-Type, returned UnexpectedContentType. Against servers that answer notifications/initialized with 200, that fails the handshake. Accept any 2xx in the content-type fallback for these messages, in both the reqwest and unix-socket transports. JSON and SSE replies are still parsed and streamed as before; requests waiting on a reply keep the strict content-type check. Tests: extend test_streamable_http_empty_2xx_notification.rs with the text/plain and chunked-empty-body shapes plus a request-path guard.
The unix-socket transport got the same "accept any 2xx for a POST that awaits no response" fallback as the reqwest transport, but its 202-only test handler never exercised it. Add two regression tests driving UnixSocketHttpClient::post_message with notifications/initialized against a socket server that answers 200 with text/plain, and 200 with no Content-Type/Content-Length (chunked empty body). Both expect Ok(Accepted). The file is cfg(unix), so these run on Linux CI and compile out on Windows.
github-actions
Bot
added
T-test
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
@jamadeo is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Streamable HTTP client rejects a
200 OKreply to a notification-only POST (e.g.notifications/initialized) when the reply'sContent-Typeis absent or non-JSON: a success reply without a body is accepted only for 202/204,Content-Length: 0,application/jsonortext/event-stream, and anything else falls intoUnexpectedContentType. For a notification the reply carries nothing the client needs, so the handshake dies against a server whose only deviation is answering200instead of202— even though peers like the go-sdk tolerate200for non-call POSTs, and this repo already accepts the empty-200case (#849).The same fatal fallback exists in the unix-socket transport.
Change
For messages that await no reply (
Notification,Response,Error), any2xxreply is accepted before content-type dispatch — in both the reqwest and unix-socket transports. Requests (ClientRequest) keep the existing strict content-type handling: a reply to a call must still be JSON or SSE. The 202/204/Content-Length: 0/JSON/SSE branches are untouched.This restarts the work from #1063, which was closed as stale (not rejected). Per the review discussion there, the unix-socket guard now ships with equivalent regression tests.
Tests
crates/rmcp/tests/test_streamable_http_empty_2xx_notification.rs:200+text/plain, and200with noContent-Type(chunked empty body) →Ok(Accepted); both fail before the fix withUnexpectedContentType(…).crates/rmcp/tests/test_unix_socket_transport.rs: the same two cases forUnixSocketHttpClient(compiled on unix; runs on Linux CI).cargo test -p rmcp --libwith the client/transport features: 347 passed; targeted integration tests (4xx error body, client concurrency, initialize error) pass;cargo +nightly fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warningsare clean. The two new unix-socket tests are compile-verified forx86_64-unknown-linux-gnuon this host and run on Linux CI.