-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(client): probe stdio servers on a disposable sibling process #2514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| Probe stdio servers on a disposable sibling process. Some stdio servers exit on any pre-`initialize` request (servers built on the official Rust SDK, rmcp, behave this way), so under `versionNegotiation: { mode: 'auto' }` the `server/discover` probe previously killed the server and `connect()` hard-failed. The probe now runs on a short-lived sibling spawned from the same parameters — its stderr is discarded and it is reaped once the era is known — and the caller's transport spawns exactly once, afterwards: a legacy verdict connects with the plain `initialize` handshake (byte-identical to `mode: 'legacy'`), a modern verdict is adopted directly, and the session wire never carries `server/discover`. Closing the caller's transport during the probe aborts `connect()` with the typed `SdkError(EraNegotiationFailed)` and the session child is never spawned. On HTTP — and on custom stdio-shaped transports, which probe in place — a mid-probe connection close keeps rejecting with the typed connect error, now naming the close in pin-mode and modern-only diagnostics. |
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Two paragraphs in the "Who should not default to 'auto'" section still describe the pre-sibling probe behavior: lines ~114-119 claim "the probe round trip changes recorded transcripts/raw logs" on stdio, but this PR's sibling design makes the session wire byte-identical to a plain
mode: 'legacy'connect (pinned by this PR's own unit test) — this same diff already reworded the exact parallel::: warningin docs/protocol-versions.md to drop the transcript claim and name the real remaining cost (one extra short-lived spawn per connect). Similarly, lines ~121-126 ("the legacy-fallback path also begins with one enveloped probe") are now true only for HTTP and in-place stdio-shaped transports; on the baseStdioClientTransportthe enveloped probe rides the disposable sibling and never touches the session wire. Reword both to match the protocol-versions.md rewording.Extended reasoning...
What's stale. This PR moves the SDK stdio probe onto a disposable sibling process, and it carefully reworded the affected prose almost everywhere — including the exact parallel
::: warningblock indocs/protocol-versions.md, which the diff changed from "the extra round trip changes recorded transcripts" to "the probe spawns an extra short-lived server process per connect." But the sibling paragraph indocs/migration/support-2026-07-28.md(in the "Who should not default to'auto'" section, immediately below the Probe policy section this PR rewrites) survived untouched:Why it's now wrong for exactly its stated audience. The paragraph is explicitly scoped to spawn-per-invocation CLI tools on stdio. Post-PR, on the base
StdioClientTransport(the transport such a tool uses), theserver/discoverprobe rides the disposable sibling and the caller's session wire is byte-identical to a plainmode: 'legacy'connect. This is pinned by the PR's own tests: the unit test "the session's traffic is byte-identical to a plain mode:'legacy' connect" (packages/client/test/client/versionNegotiation.test.ts) assertsJSON.stringify(autoSession.sent) === JSON.stringify(plainSession.sent), and the integration test asserts the session pid's wire log containsinitializebut neverserver/discover.Step-by-step proof. (1) A spawn-per-invocation CLI tool defaults to
'auto'and records its own transport transcript (the population the paragraph warns). (2)Client._connectNegotiatedcallsreadStdioServerParams(transport); the base class's prototype owns_disposeand retains_serverParams, so the sibling path is taken. (3)negotiateStdioViaSiblingspawns the sibling, runs the probe there, reaps it. (4) Only then does the session transport start, and_legacyHandshakesends a plaininitialize— the recorded transcript is identical to amode: 'legacy'run. The "changes recorded transcripts" rationale is therefore false for this transport, while the paragraph omits the cost that IS new and real: one extra short-lived server spawn per connect (which is exactly what the rewordedprotocol-versions.mdwarning and the PR description's Breaking Changes section both name).Secondary staleness two paragraphs down (~lines 121-126): "Tooling that classifies traffic must not treat 'saw an envelope' as 'modern era negotiated': the legacy-fallback path also begins with one enveloped probe." On the base stdio transport the enveloped probe went to the sibling; the session wire's legacy fallback begins with a plain un-enveloped
initialize. The claim remains true only for HTTP and for in-place stdio-shaped transports/subclasses, so it needs the same scoping the rest of the diff applies (e.g. "on HTTP and in-place stdio probes, the legacy-fallback path also begins with one enveloped probe; on the SDK's base stdio transport the enveloped probe rides the sibling and never touches the session wire").Why nothing catches it. Docs prose isn't pinned by any test, and the file's other stale sites (lines 59 and 79-81, flagged in an earlier round) were fixed by this PR — these two paragraphs sit just below the rewritten section and were simply missed. This matches the repo's Completeness recurring catch (surviving instances of a replaced phrasing pattern in a file the same diff edits) and the Documentation checklist item ("flag prose that now contradicts the implementation").
How to fix. Mirror the
protocol-versions.mdrewording: in the "Who should not default to'auto'" paragraph, replace the transcript sentence with the extra-spawn cost ("and the probe spawns an extra short-lived server process per connect"), and scope the "saw an envelope" caveat to HTTP and in-place stdio probes. Docs-only; nothing breaks at runtime.