fix(core): make instanceof on SDK error classes work across bundled copies#2384
fix(core): make instanceof on SDK error classes work across bundled copies#2384felixweinberger wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 92b1400 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| These classes (and `OAuthError`) brand-match under `instanceof`, so checks work across | ||
| separately bundled copies of the SDK — e.g. a process using both | ||
| `@modelcontextprotocol/client` and `@modelcontextprotocol/server`. |
There was a problem hiding this comment.
🟡 The 'Typed ProtocolError subclasses' section later in this same file still says ProtocolError.fromError "works across bundle boundaries where instanceof doesn't" — but this PR makes instanceof brand-matched and work across bundled copies, as the new paragraph added in this Errors section states. The trailing "where instanceof doesn't" clause should be removed or rewritten (e.g. "…so it also works without relying on class identity").
Extended reasoning...
What the issue is
This PR adds a paragraph in the Errors section of docs/migration/upgrade-to-v2.md (lines 436–438): "These classes (and OAuthError) brand-match under instanceof, so checks work across separately bundled copies of the SDK…". However, about 80 lines later in the same Errors section, under Typed ProtocolError subclasses (line ~517), the unchanged text still reads:
ProtocolError.fromError(code, message, data)reconstructs the typed subclass from code + data alone, so it works across bundle boundaries whereinstanceofdoesn't.
Why this is now contradictory
The whole point of this PR is that instanceof on ProtocolError and its typed subclasses (ResourceNotFoundError, MissingRequiredClientCapabilityError, etc.) now does work across separately bundled copies: instances are stamped with brand strings under a registry symbol (Symbol.for('mcp.sdk.errorBrands')), and Symbol.hasInstance resolves instanceof against the brand set. The new crossBundleBrand.test.ts suite even covers exactly this case — ProtocolError.fromError from a foreign module copy producing an instance that satisfies instanceof ResourceNotFoundError in the local copy.
Step-by-step proof of the inconsistency
- A reader migrating to v2 reads the new paragraph at lines 436–438: brand-matched
instanceofworks across bundles. - They continue down the same Errors section to "Typed
ProtocolErrorsubclasses" and read line 517:fromErroris recommended becauseinstanceofdoesn't work across bundles. - The two statements directly contradict each other within ~80 lines of the same section, in the same revision of the file. Post-PR behavior matches statement (1), not (2): in the new test,
protocolErrors.ProtocolError.fromError(InvalidParams, 'gone', { uri: 'file:///missing' })from a re-imported (foreign) copy returns an instance for whichreconstructed instanceof ResourceNotFoundErroristrueagainst the original copy's class.
Why the existing changes don't cover it
The PR did sweep the analogous claims in the JSDoc of ResourceNotFoundError and MissingRequiredClientCapabilityError in packages/core-internal/src/types/errors.ts (which previously said not to rely on instanceof across bundles) and added the new Errors-section paragraph — but missed this one sentence. A search of the docs confirms it is the only surviving instance of the old "instanceof doesn't work across bundles" claim.
Impact
Documentation-only. A migrating user could be steered toward fromError-based reconstruction or code/data switching when a plain instanceof check is now fully supported, and the contradiction undermines confidence in the new guarantee the PR is advertising.
Fix
Drop or rewrite the trailing clause on line 517, e.g.:
ProtocolError.fromError(code, message, data)reconstructs the typed subclass from code + data alone, so it also works without relying on class identity at all.
| it('does not leak the brand through enumeration or serialization', () => { | ||
| const err = new SdkHttpError(SdkErrorCode.ClientHttpForbidden, 'nope', { status: 403 }); | ||
| expect(Object.keys(err)).not.toContain(expect.stringContaining('mcp')); | ||
| const json = JSON.stringify({ ...err }); | ||
| expect(json).not.toContain('mcp.Sdk'); | ||
| }); |
There was a problem hiding this comment.
🟡 The assertion expect(Object.keys(err)).not.toContain(expect.stringContaining('mcp')) is vacuous: toContain compares array elements with strict equality and never evaluates asymmetric matchers, so this negated check passes unconditionally and would not catch an enumerable brand key leaking. Consider expect(Object.keys(err).some(k => k.includes('mcp'))).toBe(false) or .not.toEqual(expect.arrayContaining([expect.stringContaining('mcp')])) so the test actually exercises the non-enumerability claim.
Extended reasoning...
What the bug is. In the 'does not leak the brand through enumeration or serialization' test, the line expect(Object.keys(err)).not.toContain(expect.stringContaining('mcp')) does not assert what it appears to. Vitest's toContain (matching Jest's semantics) checks array membership using strict / SameValueZero equality on elements (and substring matching only when both actual and expected are strings); it does not evaluate asymmetric matchers like expect.stringContaining. Asymmetric matchers only participate in the deep-equality matchers (toEqual, toContainEqual, expect.arrayContaining).
Why it always passes. Object.keys(err) is an array of plain strings ('code', 'data', 'name', …). None of those strings can ever be strictly equal to the AsymmetricMatcher object produced by expect.stringContaining('mcp'), so toContain is always false, and the negated form .not.toContain(...) is therefore always true — regardless of whether a brand key leaks through enumeration.
Step-by-step proof. Suppose the implementation regressed and stamped the brand under an enumerable string key, e.g. this.mcpBrands = brands instead of the symbol-keyed Object.defineProperty. Then Object.keys(err) would be ['code', 'data', 'name', 'mcpBrands']. The assertion evaluates ['code','data','name','mcpBrands'].includes(AsymmetricMatcher{sample:'mcp'}), which is false because no element is strictly equal to the matcher object. .not.toContain then passes — the test stays green even though the exact regression it is meant to catch has occurred. Conversely, in the current correct implementation it also passes, for the same reason: the assertion's outcome is independent of the code under test.
Why this is only a nit. The production behavior is correct: stampErrorBrands defines the brand under Symbol.for('mcp.sdk.errorBrands') with enumerable: false, and symbol-keyed properties never appear in Object.keys anyway. The very next assertion in the same test — JSON.stringify({ ...err }) not containing 'mcp.Sdk' — is a real, non-vacuous check that provides partial coverage of the serialization-non-leak claim. So this is purely a test-quality issue: an assertion that doesn't exercise the property its test name promises, giving false confidence rather than an incorrect runtime.
How to fix. Replace line 115 with one of:
expect(Object.keys(err).some(k => k.includes('mcp'))).toBe(false);
// or
expect(Object.keys(err)).not.toEqual(expect.arrayContaining([expect.stringContaining('mcp')]));Either form actually evaluates the substring condition against each key, so it would fail if an enumerable mcp* key ever leaked onto instances. It may also be worth adding expect(Object.getOwnPropertySymbols(err).length) -based or propertyIsEnumerable checks if you want the test to pin down the symbol-keyed, non-enumerable storage explicitly.
| * have to declare for the request to be served. On HTTP, the response status | ||
| * is `400 Bad Request`. | ||
| * | ||
| * Recognize this error by its code and `data.requiredCapabilities` rather than | ||
| * by class identity (`instanceof` does not work across separately bundled | ||
| * copies of the SDK). | ||
| * Recognize this error by its code and `data.requiredCapabilities`, or by | ||
| * `instanceof` — checks are brand-matched and work across separately bundled | ||
| * copies of the SDK. |
There was a problem hiding this comment.
🟡 Several test-file header comments still document the pre-PR assumption that instanceof does not work across bundled copies of the SDK, contradicting the brand-matched guarantee this PR adds (and the JSDoc it rewrites in this file): packages/core-internal/test/types/missingClientCapabilityError.test.ts:6, packages/core-internal/test/types/errorSurfacePins.test.ts:5-6 and :95-96, packages/core-internal/test/types/crossBundleErrorRecognition.test.ts:5-9, and test/e2e/scenarios/resources.test.ts:202. Comment-only — sweep or soften them in this PR (e.g. "works across bundles only when both copies carry the brand"); note the existing review comment about the migration guide claimed line ~517 was the only surviving instance, which these sites disprove.
Extended reasoning...
What the issue is. This PR's whole point is that instanceof on the SDK error classes is now brand-matched (Symbol.hasInstance + a Symbol.for registry symbol) and therefore works across separately bundled copies of the SDK. The PR sweeps the old "do not rely on instanceof across bundles" wording from the JSDoc on ResourceNotFoundError and MissingRequiredClientCapabilityError in packages/core-internal/src/types/errors.ts and adds a paragraph in the migration guide advertising the new guarantee. However, several sibling comments documenting the old assumption were not swept and now contradict the guarantee the same diff advertises.
Surviving sites (verified at the PR head):
packages/core-internal/test/types/missingClientCapabilityError.test.ts:4-6— header: recognition is by code +data.requiredCapabilitiesshape, "never byinstanceofacross bundles". This directly contradicts the new JSDoc onMissingRequiredClientCapabilityErrorthat this PR rewrites to sayinstanceofchecks "are brand-matched and work across separately bundled copies of the SDK".packages/core-internal/test/types/errorSurfacePins.test.ts:5-6— "not only by enum member orinstanceof(which breaks across bundled package boundaries)" — and:95-96— "instanceof breaks when core is bundled into both the client and server dists". No longer categorically true for the branded classes.packages/core-internal/test/types/crossBundleErrorRecognition.test.ts:4-9— "a typed error class constructed inside one bundle is NOTinstanceofthe 'same' class imported from another bundle … must never rely oninstanceofacross the package boundary". (The simulated foreign class in that test is intentionally unbranded, so the test still passes — only the prose is stale.)test/e2e/scenarios/resources.test.ts:201-203— "(noinstanceofacross bundles)" justifying the data-parse recognizer.
Step-by-step proof of the contradiction. A maintainer reads the new JSDoc on MissingRequiredClientCapabilityError (packages/core-internal/src/types/errors.ts:182-187): "Recognize this error by its code and data.requiredCapabilities, or by instanceof — checks are brand-matched and work across separately bundled copies of the SDK." They then open the test that pins this class's behavior, missingClientCapabilityError.test.ts, and the very first paragraph says recognition is "never by instanceof across bundles". Both statements are in the tree at the same commit and cannot both be right; post-PR behavior matches the JSDoc, not the test header — the new crossBundleBrand.test.ts suite proves a foreign-copy ResourceNotFoundError satisfies instanceof against the local copy's class.
Why the existing review comment doesn't cover it. The existing claude[bot] inline comment targets only docs/migration/upgrade-to-v2.md line ~517 ("works across bundle boundaries where instanceof doesn't") and explicitly claims that line was "the only surviving instance" of the old claim. These four test-file sites disprove that claim and would not be touched by fixing the doc line, so this is an additional, distinct set of locations.
Impact. Comment-only — no runtime or test-outcome change. But the comments describe pre-PR behavior and will mislead maintainers (and anyone reading the tests as documentation of the recognition contract) about whether instanceof is reliable across bundles, and they undercut the guarantee this PR's own JSDoc and migration-guide paragraph advertise. The statements remain true only in narrower edge cases (against pre-brand SDK copies, or for plain wire-shape values that are never branded), which is exactly the nuance the comments should be softened to.
How to fix. Sweep these comments in the same PR. Either delete the stale parentheticals, or soften them to the precise post-PR contract, e.g. "instanceof works across bundles only when both copies carry the brand (this release or later); the data-parse recognizer remains the version-agnostic path and works on plain wire shapes that are never branded." That keeps the rationale for the data-parse contract the tests pin while no longer contradicting the new guarantee.
…opies @modelcontextprotocol/client and @modelcontextprotocol/server each bundle their own copy of the runtime internals, so an error constructed by one package failed prototype-identity instanceof against the same class re-exported by the other - exactly the check a dual-role process (gateway, host, in-process test) writes. ProtocolError and its typed subclasses, SdkError/SdkHttpError, and OAuthError now stamp instances with the brand strings of their class chain under a registry symbol (Symbol.for, shared across bundles) and resolve instanceof via Symbol.hasInstance against the brand set, with ordinary prototype instanceof as the fallback. User-defined subclasses that do not declare a brand keep plain prototype semantics, so a foreign base-class instance never satisfies instanceof UserSubclass.
A brand reachable through the prototype chain no longer satisfies the check, so polluting a shared prototype with the registry symbol cannot make arbitrary objects pass instanceof against the branded error classes. Real instances are stamped with an own property and are unaffected.
- Move brands from declared protected static fields into static blocks: the field declaration reached the .d.mts and TypeScript's protected-member rule made the constructor types nominally incompatible across the two packages (typeof ProtocolError from client was unassignable to server's). Static blocks keep the own runtime property with no type-surface footprint. - Harden brandedHasInstance against hostile Proxy traps and throwing accessors: instanceof must not throw where it previously returned false. - Brand SseError: the migration guide's HTTP-status classification row pairs instanceof SdkHttpError with instanceof SseError; both are now cross-bundle-robust (two client copies in one process). - Lock the cross-version contract in tests (identity, not shape) and document it plus the both-copies requirement in the changeset and module JSDoc.
felixweinberger
force-pushed
the
fweinberger/cross-bundle-error-instanceof
branch
from
0d8362a to
92b1400
Compare
|
Reopened as a draft to serve as a starting point for a successor PR. Status notes for whoever picks it up:
|
1 participant
@modelcontextprotocol/clientand@modelcontextprotocol/servereach bundle their own copy of the runtime internals, so an error constructed by one package fails prototype-identityinstanceofagainst the same class re-exported by the other — exactly the check a dual-role process (gateway, host, in-process test) writes.Motivation and Context
Hit independently by four different migrated projects: cross-package
instanceof OAuthError/ProtocolError/SdkErrorchecks silently stop matching when both packages are installed, and the failure is runtime-only. This is the single most-reported blocking issue from real v1→v2 migrations.ProtocolError(and its typed subclasses),SdkError/SdkHttpError, andOAuthErrornow stamp instances with the brand strings of their class chain under a registry symbol (Symbol.for, shared across bundles and realms) and resolveinstanceofviaSymbol.hasInstanceagainst the brand set, with ordinary prototypeinstanceofpreserved as the fallback:clientConstructedError instanceof ServerReexportedClass→ matches by brand.SdkHttpErrorinstance satisfiesinstanceof SdkError).instanceof UserSubclass.JSON.stringify/ spread /Object.keysleakage.This does not change the packaging itself — whether to converge on a single shared runtime core remains a separate decision; this removes the user-visible breakage in the meantime.
How Has This Been Tested?
New
crossBundleBrand.test.tssuite simulates the dual-bundle situation faithfully (vi.resetModules()+ dynamic import → genuinely distinct class objects sharing only the symbol registry): cross-copy positives in both directions, subclass→base,fromErrorreconstruction from a foreign copy, hierarchy disjointness, user-subclass safety, serialization non-leakage, and primitive/null tolerance. Full suites green: core-internal 1301, client 698, server 350. Typecheck and lint clean.Breaking Changes
None. Same-bundle behavior is unchanged (prototype fallback); the change strictly widens which checks succeed.
Types of changes
Checklist
Additional context
JSDoc on
ResourceNotFoundError/MissingRequiredClientCapabilityErrorpreviously told consumers not to rely oninstanceofacross bundles — updated accordingly, plus a one-line note in the migration guide's Errors section.SseErrorandUnauthorizedErrorare client-package-only (single bundle) and deliberately not branded.