refactor(client,server): move stdio transports to ./stdio subpath export#1871
refactor(client,server): move stdio transports to ./stdio subpath export#1871felixweinberger wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 46fe6d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
There was a problem hiding this comment.
LGTM — clean implementation following the existing _shims conditional-export pattern.
Extended reasoning...
Overview
The PR adds browser and workerd export conditions to the client and server package roots that swap the real stdio transports for throwing stubs. This prevents browser/CF Workers bundlers from pulling in node:child_process, node:stream, and cross-spawn. Changes span 14 files: two new index.browser.ts barrel files, two new stdioStub.ts files, updated package.json exports, build config updates, tests, a changeset, and a new SdkErrorCode.TransportNotSupported enum value in core.
Security Risks
None. The stubs throw a helpful error on use rather than silently failing. No auth, crypto, or permissions code is touched.
Level of Scrutiny
Moderate — package.json exports conditions are critical for correct module resolution across runtimes, but the change is additive (new conditions prepended before the existing default) and non-breaking. A diff of the browser vs. node barrel files shows the only difference is the stdio import source, confirming the mirrors are accurate.
Other Factors
The only reported bug is a cosmetic duplicate comment label in sdkErrors.ts, which has no functional impact. @modelcontextprotocol/core is a private (unpublished) workspace package, so the absence of a core entry in the changeset is correct. Tests verify the browser bundle excludes Node-only imports while still exporting the stub class. The approach follows the existing _shims conditional-export pattern already in the repo.
| CapabilityNotSupported = 'CAPABILITY_NOT_SUPPORTED', | ||
|
|
||
| // Transport errors | ||
| /** Transport is not available in the current runtime (e.g. stdio in a browser) */ | ||
| TransportNotSupported = 'TRANSPORT_NOT_SUPPORTED', | ||
| /** Request timed out waiting for response */ | ||
| RequestTimeout = 'REQUEST_TIMEOUT', | ||
| /** Connection was closed */ |
There was a problem hiding this comment.
🟡 The file now has two sections both labeled // Transport errors (lines 22 and 32 in sdkErrors.ts); the second section covering ClientHttp* codes should be labeled // HTTP client transport errors to distinguish the categories. This is a pre-existing naming issue, but the PR touches this file and had a natural opportunity to fix it.
Extended reasoning...
What the bug is
packages/core/src/errors/sdkErrors.ts now contains two distinct enum sections both labeled with the comment // Transport errors. The first section (line 22) groups general transport-level errors: TransportNotSupported, RequestTimeout, ConnectionClosed, and SendFailed. The second section (line 32) groups HTTP-client-specific error codes: ClientHttpNotImplemented, ClientHttpAuthentication, ClientHttpForbidden, ClientHttpUnexpectedContent, ClientHttpFailedToOpenStream, and ClientHttpFailedToTerminateSession.
The specific code path
The PR inserts TransportNotSupported correctly into the first // Transport errors section. However, the second // Transport errors comment (before the ClientHttp* codes) was already present in the file before this PR and was not renamed. The result is that a developer reading the enum sees two identically-named sections without a clear indication that one covers general transport failures and the other covers HTTP-specific client transport errors.
Why existing code does not prevent it
This is purely a naming/organizational issue in source code. There is no linter or test that enforces unique comment labels within an enum. The duplicate label slipped in as a pre-existing issue and the PR, while it touched this file, did not address it.
Impact
No functional impact. The enum values themselves are unique strings (TRANSPORT_NOT_SUPPORTED, CLIENT_HTTP_NOT_IMPLEMENTED, etc.) so there is no runtime confusion. The issue is purely a readability concern: developers scanning the enum may be confused about which section ClientHttp* codes belong to, or may mistakenly assume the two sections are redundant groupings of the same category.
How to fix
Rename the second comment from // Transport errors to // HTTP client transport errors:
// Transport errors
TransportNotSupported = "TRANSPORT_NOT_SUPPORTED",
RequestTimeout = "REQUEST_TIMEOUT",
ConnectionClosed = "CONNECTION_CLOSED",
SendFailed = "SEND_FAILED",
// HTTP client transport errors
ClientHttpNotImplemented = "CLIENT_HTTP_NOT_IMPLEMENTED",
// ...Addressing the refutation
One verifier noted this is pre-existing and style issues should only be flagged in egregious cases. While true that the duplicate pre-dates this PR, the PR directly modifies sdkErrors.ts and adds a new entry to the first // Transport errors section, making the distinction between the two sections more relevant than before (one now explicitly contains a runtime-availability error, the other HTTP-protocol errors). The fix is a one-line comment change and the naming confusion is more pronounced after this PR.
There was a problem hiding this comment.
Stale. The PR was reworked to a ./stdio subpath export and no longer touches sdkErrors.ts (not in the current diff).
5350730 to
95b5eca
Compare
Stdio transports require a process-spawning runtime (Node.js, Bun, Deno). Exporting them from the package root meant browser and Cloudflare Workers bundlers pulled in node:child_process, node:stream, and cross-spawn. Move StdioClientTransport, getDefaultEnvironment, DEFAULT_INHERITED_ENV_VARS, and StdioServerParameters to @modelcontextprotocol/client/stdio, and StdioServerTransport to @modelcontextprotocol/server/stdio. The root entry is now browser-safe; stdio consumers update one import path.
95b5eca to
46fe6d2
Compare
Move stdio transports to a
./stdiosubpath export so the package root no longer pulls innode:child_process,node:stream, orcross-spawn. Fixes bundling for browser and Cloudflare Workers consumers. Node.js, Bun, and Deno consumers update one import path.Motivation and Context
The v2 root barrel exports
StdioClientTransport, which top-level importscross-spawnandnode:child_process. Any bundler targeting browsers fails. v1 avoided this with deep subpath imports; this restores that boundary as a single./stdiosubpath.How Has This Been Tested?
barrelClean.test.tsin both packages asserts the builtdist/index.mjs(and its transitive chunks) contain zero process-spawning runtime imports, and thatdist/stdio.mjsexports the moved symbols.Breaking Changes
Yes — stdio symbols are no longer exported from the package root. Import from
@modelcontextprotocol/{client,server}/stdio.Types of changes
Checklist
Additional context
Also moves
getDefaultEnvironment,DEFAULT_INHERITED_ENV_VARS, and theStdioServerParameterstype to the client./stdiosubpath.