Conversation
c9cc22a to
bc76e74
Compare
9b0c1b1 to
3791666
Compare
3791666 to
3902778
Compare
This comment was marked as spam.
This comment was marked as spam.
|
Confirmed and fixed in Went with your option 1: since the PR description's own stated intent is "enabling clients to negotiate older or custom protocol versions," an override has to actually win, not depend on whether the server happens to answer Added:
Both fail on the pre-fix code and pass after (verified via Separate, smaller point from your review I did not change: the arbitrary-custom-version consistency gap ( One more thing surfaced while testing this end-to-end: for an in-process |
negotiate_auto only consulted protocol_version in its initialize() fallback calls, so mode="auto" (the default) silently dropped the override whenever the server/discover probe succeeded first - the override only ever took effect when the probe failed. Since the whole point of protocol_version_override is to let a caller pin an older or custom protocol version, an override must always win: when set, skip the discover probe entirely and go straight to the legacy handshake at that version. Regression tests: a unit test on negotiate_auto proving the probe is skipped even when the stub's discover script would otherwise succeed, and an e2e test over a real streamable-HTTP server (mode="auto" + override) proving only `initialize` is sent and `server/discover` never is. Reported by a static review pass on this PR; verified independently by tracing the actual control flow before applying this fix.
f55e795 to
dce39ec
Compare
RequestResponder and mcp.shared.session were removed upstream since this test was written; message_handler callbacks now receive IncomingMessage (ServerNotification | Exception), matching every other message_handler in this file.
|
Rebased onto main (89 commits) and fixed a genuine lint failure the rebase surfaced: RequestResponder and mcp.shared.session were removed upstream since this test was added. Updated message_handler to the current IncomingMessage type, matching every other handler in this file. Ruff, pyright, and all 92 tests in the file pass. |
pyright's pre-commit hook flagged reportOptionalMemberAccess on client.server_info.name in two new protocol_version_override tests. server_info is Implementation | None; every other call site in this file already asserts not-None first.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/session.py">
<violation number="1" location="src/mcp/client/session.py:658">
P2: Older protocol overrides still advertise form and URL elicitation capabilities when the callback is configured. Gate each capability by the requested protocol version; otherwise the server can use an advertisement for features unavailable in the selected protocol.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # The handshake negotiates only legacy versions, where no claim is active. | ||
| capabilities=self._build_capabilities(LATEST_HANDSHAKE_VERSION), | ||
| protocol_version=protocol_version, | ||
| capabilities=self._build_capabilities(protocol_version), |
There was a problem hiding this comment.
P2: Older protocol overrides still advertise form and URL elicitation capabilities when the callback is configured. Gate each capability by the requested protocol version; otherwise the server can use an advertisement for features unavailable in the selected protocol.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/session.py, line 658:
<comment>Older protocol overrides still advertise form and URL elicitation capabilities when the callback is configured. Gate each capability by the requested protocol version; otherwise the server can use an advertisement for features unavailable in the selected protocol.</comment>
<file context>
@@ -648,15 +648,14 @@ def _build_capabilities(self, version: str) -> types.ClientCapabilities:
- # The handshake negotiates only legacy versions, where no claim is active.
- capabilities=self._build_capabilities(LATEST_HANDSHAKE_VERSION),
+ protocol_version=protocol_version,
+ capabilities=self._build_capabilities(protocol_version),
client_info=self._client_info,
),
</file context>
…mode connect cubic's review found two real gaps in the protocol_version_override feature: - Client(server, mode="auto", protocol_version_override=...) against an in-process Server/MCPServer failed to connect. negotiate_auto's override path calls session.initialize() directly, but _connect_inproc picked the handshake-less DirectDispatcher for every non-"legacy" mode, so there was no JSON-RPC dispatcher for initialize() to run on. _build_session now collapses this combination to the legacy connector. - protocol_version_override accepted any string uncritically: a modern version silently misbehaved instead of being rejected, and setting it alongside a version-pinned mode was silently ignored rather than erroring. Both are now validated in __post_init__, mirroring the existing mode check. Also pinned the untested default (None) branch's exact initialize() call shape in test_session_group.py, per the same review.
|
Fixed cubic's P1 (in-proc auto-mode + override never connected) and P2 (no validation on the override) findings, plus the test-coverage gap. Left the elicitation-capability-gating note unaddressed pending confirmation it applies across handshake-era sub-versions. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/client.py">
<violation number="1" location="src/mcp/client/client.py:395">
P2: The `hint` expression's `else ""` branch executes only when `protocol_version_override` is in neither `HANDSHAKE_PROTOCOL_VERSIONS` nor `MODERN_PROTOCOL_VERSIONS`; every test passes either `"2024-11-05"` or `LATEST_MODERN_VERSION`, so that branch is uncovered and the repo's 100% branch-coverage CI (`fail_under = 100`, `branch = true`) will fail. Add a test passing an invalid non-modern override (e.g. `"1999-01-01"`) that asserts the same `ValueError`, exercising the empty-hint arm.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| if self.protocol_version_override is not None: | ||
| if self.protocol_version_override not in HANDSHAKE_PROTOCOL_VERSIONS: | ||
| hint = ( |
There was a problem hiding this comment.
P2: The hint expression's else "" branch executes only when protocol_version_override is in neither HANDSHAKE_PROTOCOL_VERSIONS nor MODERN_PROTOCOL_VERSIONS; every test passes either "2024-11-05" or LATEST_MODERN_VERSION, so that branch is uncovered and the repo's 100% branch-coverage CI (fail_under = 100, branch = true) will fail. Add a test passing an invalid non-modern override (e.g. "1999-01-01") that asserts the same ValueError, exercising the empty-hint arm.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/client.py, line 395:
<comment>The `hint` expression's `else ""` branch executes only when `protocol_version_override` is in neither `HANDSHAKE_PROTOCOL_VERSIONS` nor `MODERN_PROTOCOL_VERSIONS`; every test passes either `"2024-11-05"` or `LATEST_MODERN_VERSION`, so that branch is uncovered and the repo's 100% branch-coverage CI (`fail_under = 100`, `branch = true`) will fail. Add a test passing an invalid non-modern override (e.g. `"1999-01-01"`) that asserts the same `ValueError`, exercising the empty-hint arm.</comment>
<file context>
@@ -385,6 +390,23 @@ def __post_init__(self) -> None:
+ if self.protocol_version_override is not None:
+ if self.protocol_version_override not in HANDSHAKE_PROTOCOL_VERSIONS:
+ hint = (
+ f" ({self.protocol_version_override!r} is a modern version; mode='auto' already negotiates it)"
+ if self.protocol_version_override in MODERN_PROTOCOL_VERSIONS
</file context>
This branch has not been deployed
2 participants
Description
This PR adds support for client session protocol version overrides during initialization, enabling clients to negotiate older or custom protocol versions (e.g.
2024-11-05) with MCP servers.Changes
ClientSession.initialize(...)to accept a customprotocol_version.protocol_versionfield to the high-levelClientdataclass.protocol_versionfield toClientSessionParametersinClientSessionGroup.