Uh oh!
There was an error while loading. Please reload this page.
docs(migration): dual-role instanceof caveat in the Errors section - #2385
Conversation
The HTTP-status classification guidance recommends instanceof checks that silently fail across the separately bundled client and server copies of the error classes (gateways, hosts, in-process tests). Document the limitation where the patterns are taught and point at the cross-bundle-safe forms: field/code matching and ProtocolError.fromError.
|
@modelcontextprotocol/client@modelcontextprotocol/codemod@modelcontextprotocol/core@modelcontextprotocol/server@modelcontextprotocol/server-legacy@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
There was a problem hiding this comment.
LGTM — docs-only, one paragraph in the migration guide; the cross-package instanceof claim and the ProtocolError.fromError signature both check out against the code.
Extended reasoning...
Overview
This PR adds a single paragraph to the Errors section of docs/migration/upgrade-to-v2.md, documenting that instanceof checks on SDK error classes do not work across the @modelcontextprotocol/client / @modelcontextprotocol/server package boundary in dual-role processes, and recommending field-based matching (error.code, presence checks) or ProtocolError.fromError instead. No code, schema, or example changes.
Verification of the claims
- The bundling claim is accurate:
packages/client/tsdown.config.ts(and the server package) lists@modelcontextprotocol/core-internalundernoExternal, so each published package ships its own copy of the error classes and cross-packageinstanceofindeed fails. ProtocolError.fromError(code, message, data)exists with exactly that signature inpackages/core-internal/src/types/errors.ts:26, and the guide already describes it as working across bundle boundaries in the Typed ProtocolError subclasses section — the new paragraph is consistent with that.- The recommended cross-bundle-safe forms (string
SdkErrorCode, numeric JSON-RPC codes,OAuthErrorCodestrings,'status' in e) match the rest of the Errors/Auth sections.
Security risks
None — documentation-only change with no executable content.
Level of scrutiny
Low. This is a one-paragraph docs addition in a migration guide, follow-up to #2382, with no API surface or behavioral impact. The main risk for docs PRs is prose contradicting the implementation, which I checked directly.
Other factors
No changeset is needed for a docs-only change (changeset-bot warning is expected). The bug-hunting system found no issues, and the new text fits the existing structure and tone of the Errors section.
Uh oh!
There was an error while loading. Please reload this page.
Follow-up to #2382. The Errors section teaches
e instanceof SdkHttpError && e.status === 401-style classification; in a process that uses both@modelcontextprotocol/clientand@modelcontextprotocol/server(gateway, host, in-process test), each package bundles its own copy of the error classes, so an error constructed by one package failsinstanceofagainst the class imported from the other — silently. With the runtime-identity fix (#2384) not proceeding, the guide should say so where the patterns are taught.Motivation and Context
Four independent v1→v2 migrations hit silent cross-package
instanceofmisses. This adds one paragraph documenting the limitation and the cross-bundle-safe forms:error.code/ field-presence matching, andProtocolError.fromErrorfor typed protocol errors.How Has This Been Tested?
Docs-only; prettier clean.
Breaking Changes
None.
Types of changes
Checklist