| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
A stateful (pre-2026) protocol_version pin is a session-layer concern: it controls the protocolVersion field in the InitializeRequest body. #2910 threaded it into the transport unconditionally, which (a) stamped MCP-Protocol-Version on the initialize POST itself and (b) kept stamping the pinned value after the server negotiated a different version. The transport now only stores 2026-07-28+ pins (which never send initialize). For earlier versions it falls back to learning the negotiated version from the InitializeResult, restoring pre-#2910 wire behaviour for stateful sessions. Also adds a docstring caveat to Client.protocol_version noting the in-memory transport does not yet have a modern entry point.
There was a problem hiding this comment.
LGTM — small, well-scoped follow-up that restores the pre-#2910 header behaviour for stateful pins, with direct test coverage of both the omitted-on-initialize and negotiated-version cases.
Extended reasoning...The PR touches three files: a one-conditional change in StreamableHTTPTransport.__init__ (only retain a constructor protocol_version pin when it is in MODERN_PROTOCOL_VERSIONS), accompanying docstring/comment updates in streamable_http.py and client.py, and unit tests. No control flow elsewhere changes; the existing _maybe_extract_protocol_version_from_message snoop simply becomes the source of the header for pre-2026 sessions again, which matches the behaviour before #2910.
None identified. The change only affects which value populates the outgoing MCP-Protocol-Version header on the client side; it does not touch auth, parsing of untrusted input, or server-side validation. If anything it aligns the wire header with the spec (header reflects the negotiated version, and is not sent on the initialize request itself).
This is client transport logic, not security-sensitive code, and the effective diff is a single guarded assignment plus documentation. The semantics (a stateful pin is a session-layer concern carried in the InitializeRequest body, while the transport header follows negotiation) are clearly argued in the PR description and are consistent with how Client threads protocol_version into ClientSession separately. The new test exercises both behaviours directly (MCP-ProtOCOL-Version absent before negotiation, negotiated value stamped after), and the renamed modern-pin test still covers the 2026 path.
The bug-hunting pass found no issues, the PR is a follow-up addressing post-merge review feedback from #2910/#2917, and the author reports the 2025-pinned interaction suite passes unchanged. The deliberately-unaddressed review threads are explicitly called out as out of scope. Overall risk is low and intent is clear.
Sorry, something went wrong.
|
This pull request is included in pre-release v2.0.0a3 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Follow-up to #2910 / #2917 addressing post-merge review feedback.
Motivation and Context
#2910 threaded the new protocol_version pin into StreamableHTTPTransport unconditionally and made the constructor pin win over the InitializeResult snoop. That's correct for a 2026-07-28 pin (no initialize is ever sent), but for a pre-2026 stateful pin it meant:
A stateful pin is a session-layer concern (it picks the protocolVersion field in the InitializeRequest body); the transport doesn't need it. This change drops pre-2026 pins at the transport constructor so the header is populated from the negotiated InitializeResult exactly as before #2910, while modern pins continue to be honoured from the first request.
Also adds a one-line caveat to the Client.protocol_version docstring noting that a 2026-07-28 pin currently requires a URL/HTTP transport (the in-memory Server/MCPServer path doesn't have a modern entry yet).
Review threads not addressed here
How Has This Been Tested?
Breaking Changes
None.
Types of changes
Checklist
Additional context
The early-return guard in _maybe_extract_protocol_version_from_message is now reachable only via direct invocation in the unit test (a modern-pinned transport never sends initialize in production). Left as-is to keep this PR scoped; the single-owner refactor for the client pin will sweep it.
AI Disclaimer