fix(core): address review findings on cross-bundle error branding

- 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.

Author: felixweinberger

.changeset/cross-bundle-error-instanceof.md

@@ -3,4 +3,4 @@
 '@modelcontextprotocol/server': patch
 ---
 
-`instanceof` on the SDK error classes (`ProtocolError` and its typed subclasses, `SdkError`/`SdkHttpError`, `OAuthError`) now works across separately bundled copies of the SDK. The classes match by a stable brand (via `Symbol.hasInstance` and a registry symbol) instead of prototype identity, so a process that uses both `@modelcontextprotocol/client` and `@modelcontextprotocol/server` — a gateway, host, or in-process test — can check errors constructed by either package against the class re-exported by the other. Ordinary prototype-based `instanceof` is preserved as a fallback; user-defined subclasses keep plain prototype semantics.
+`instanceof` on the SDK error classes (`ProtocolError` and its typed subclasses, `SdkError`/`SdkHttpError`, `OAuthError`, and the client's `SseError`) now works across separately bundled copies of the SDK. The classes match by a stable brand (via `Symbol.hasInstance` and a registry symbol) instead of prototype identity, so a process that uses both `@modelcontextprotocol/client` and `@modelcontextprotocol/server` - a gateway, host, or in-process test - can check errors constructed by either package against the class re-exported by the other. Ordinary prototype-based `instanceof` is preserved as a fallback; user-defined subclasses keep plain prototype semantics. Notes: cross-bundle matching requires both copies to be at or after this release; brands assert identity, not field shape, across versions - keep reading fields defensively. As a side effect, a foreign-bundle `SdkError` used as an abort reason is now rethrown as-is instead of being wrapped as a `RequestTimeout`.

packages/client/src/client/sse.ts

@@ -1,11 +1,13 @@
 import type { FetchLike, JSONRPCMessage, Transport } from '@modelcontextprotocol/core-internal';
 import {
+    brandedHasInstance,
     createFetchWithInit,
     JSONRPCMessageSchema,
     normalizeHeaders,
     SdkError,
     SdkErrorCode,
-    SdkHttpError
+    SdkHttpError,
+    stampErrorBrands
 } from '@modelcontextprotocol/core-internal';
 import type { ErrorEvent, EventSourceInit } from 'eventsource';
 import { EventSource } from 'eventsource';
@@ -23,12 +25,21 @@ import {
 import type { IssuerMismatchError } from './authErrors';
 
 export class SseError extends Error {
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.SseError' });
+    }
+
+    static override [Symbol.hasInstance](value: unknown): boolean {
+        return brandedHasInstance(this, value);
+    }
+
     constructor(
         public readonly code: number | undefined,
         message: string | undefined,
         public readonly event: ErrorEvent
     ) {
         super(`SSE error: ${message}`);
+        stampErrorBrands(this, new.target);
     }
 }
 

packages/client/test/client/sseErrorBrand.test.ts

@@ -0,0 +1,26 @@
+import { describe, expect, it, vi } from 'vitest';
+
+import { SseError } from '../../src/client/sse';
+
+describe('SseError cross-bundle instanceof', () => {
+    it('same-bundle instanceof and fields keep working', () => {
+        const err = new SseError(401, 'unauthorized', {} as never);
+        expect(err instanceof SseError).toBe(true);
+        expect(err instanceof Error).toBe(true);
+        expect(err.code).toBe(401);
+    });
+
+    it('an instance from a second module copy satisfies instanceof against this copy', async () => {
+        vi.resetModules();
+        const copy2 = await import('../../src/client/sse');
+        expect(copy2.SseError).not.toBe(SseError);
+        const foreign = new copy2.SseError(403, 'forbidden', {} as never);
+        expect(foreign instanceof SseError).toBe(true);
+    });
+
+    it('stays disjoint from the SdkError hierarchy', async () => {
+        const { SdkError } = await import('@modelcontextprotocol/core-internal');
+        const err = new SseError(500, 'boom', {} as never);
+        expect((err as unknown) instanceof SdkError).toBe(false);
+    });
+});

packages/core-internal/src/auth/errors.ts

@@ -104,7 +104,9 @@ export enum OAuthErrorCode {
  * OAuth error class for all OAuth-related errors.
  */
 export class OAuthError extends Error {
-    protected static readonly mcpBrand: string = 'mcp.OAuthError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.OAuthError' });
+    }
 
     static override [Symbol.hasInstance](value: unknown): boolean {
         return brandedHasInstance(this, value);

packages/core-internal/src/errors/crossBundleBrand.ts

@@ -12,10 +12,24 @@
  * brand set. Ordinary prototype-based `instanceof` is kept as a fallback so behavior
  * is unchanged for anything unbranded.
  *
- * A class participates by declaring an **own** `static readonly mcpBrand` and (for
- * hierarchy roots) installing {@linkcode brandedHasInstance} as `Symbol.hasInstance`.
- * User-defined subclasses that do not declare their own brand keep plain prototype
- * semantics — a foreign base-class instance never satisfies `instanceof UserSubclass`.
+ * A class participates by defining an **own** `mcpBrand` static (via a `static {}`
+ * block, so nothing reaches the declaration files — a declared `protected static`
+ * field would make the constructor types nominally incompatible across the bundled
+ * copies) and (for hierarchy roots) installing {@linkcode brandedHasInstance} as
+ * `Symbol.hasInstance`. User-defined subclasses that do not declare their own brand
+ * keep plain prototype semantics — a foreign base-class instance never satisfies
+ * `instanceof UserSubclass`.
+ *
+ * Contract notes:
+ * - Brands assert **identity, not shape**: brand strings are version-less, so an
+ *   instance from one SDK version matches the class of another. Members added to a
+ *   branded class in a later version may be absent on a matched instance — read
+ *   fields defensively, and treat branded classes as additive-only.
+ * - Cross-bundle matching requires **both** copies to be at or after the release
+ *   that introduced branding; against an older copy, behavior degrades to plain
+ *   prototype `instanceof` in both directions.
+ * - A consumer re-bundling the SDK with property mangling (`mangle.props`) would
+ *   break the brand statics; default esbuild/webpack/terser settings do not.
  */
 
 /** Registry symbol — identical across bundled copies and realms. */
@@ -33,6 +47,10 @@ interface BrandedConstructor {
  * Stamp `instance` with the brand of every class in `ctor`'s chain that declares an
  * own `mcpBrand`. Call once from the hierarchy root's constructor with `new.target` —
  * subclasses inherit the stamping without touching their constructors.
+ *
+ * Constructor-time only: never stamp arbitrary objects. A stamped non-instance would
+ * satisfy `instanceof` while lacking the prototype members (getters like `.status`)
+ * that callers reach for after the check.
  */
 export function stampErrorBrands(instance: object, ctor: unknown): void {
     const brands = new Set<string>();
@@ -54,21 +72,27 @@ export function stampErrorBrands(instance: object, ctor: unknown): void {
  * path), falling back to ordinary prototype-based `instanceof` otherwise.
  */
 export function brandedHasInstance(cls: object, value: unknown): boolean {
-    if (
-        typeof value === 'object' &&
-        value !== null &&
-        Object.prototype.hasOwnProperty.call(cls, 'mcpBrand') &&
-        typeof (cls as BrandedConstructor).mcpBrand === 'string' &&
-        // Own-property only: a brand inherited via the prototype chain is never
-        // honored, so polluting Object.prototype with the registry symbol cannot
-        // make arbitrary objects satisfy instanceof (real instances are stamped
-        // with an own property by stampErrorBrands).
-        Object.prototype.hasOwnProperty.call(value, BRANDS)
-    ) {
-        const carried = (value as BrandCarrier)[BRANDS];
-        if (carried && typeof carried.has === 'function' && carried.has((cls as BrandedConstructor).mcpBrand!)) {
-            return true;
+    try {
+        if (
+            typeof value === 'object' &&
+            value !== null &&
+            Object.prototype.hasOwnProperty.call(cls, 'mcpBrand') &&
+            typeof (cls as BrandedConstructor).mcpBrand === 'string' &&
+            // Own-property only: a brand inherited via the prototype chain is never
+            // honored, so polluting Object.prototype with the registry symbol cannot
+            // make arbitrary objects satisfy instanceof (real instances are stamped
+            // with an own property by stampErrorBrands).
+            Object.prototype.hasOwnProperty.call(value, BRANDS)
+        ) {
+            const carried = (value as BrandCarrier)[BRANDS];
+            if (carried && typeof carried.has === 'function' && carried.has((cls as BrandedConstructor).mcpBrand!)) {
+                return true;
+            }
         }
+    } catch {
+        // A hostile Proxy trap or throwing accessor must not make `instanceof`
+        // throw where it previously returned false — fall through to the
+        // ordinary prototype check.
     }
     return Function.prototype[Symbol.hasInstance].call(cls, value);
 }

packages/core-internal/src/errors/sdkErrors.ts

@@ -100,7 +100,9 @@ export enum SdkErrorCode {
  * ```
  */
 export class SdkError extends Error {
-    protected static readonly mcpBrand: string = 'mcp.SdkError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.SdkError' });
+    }
 
     static override [Symbol.hasInstance](value: unknown): boolean {
         return brandedHasInstance(this, value);
@@ -143,7 +145,9 @@ export interface SdkHttpErrorData {
  * ```
  */
 export class SdkHttpError extends SdkError {
-    protected static override readonly mcpBrand: string = 'mcp.SdkHttpError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.SdkHttpError' });
+    }
 
     declare readonly data: SdkHttpErrorData;
 

packages/core-internal/src/index.ts

@@ -1,4 +1,5 @@
 export * from './auth/errors';
+export * from './errors/crossBundleBrand';
 export * from './errors/sdkErrors';
 export * from './shared/auth';
 export * from './shared/authUtils';

packages/core-internal/src/types/errors.ts

@@ -17,7 +17,9 @@ import type {
  * `@modelcontextprotocol/server` in the same process.
  */
 export class ProtocolError extends Error {
-    protected static readonly mcpBrand: string = 'mcp.ProtocolError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.ProtocolError' });
+    }
 
     static override [Symbol.hasInstance](value: unknown): boolean {
         return brandedHasInstance(this, value);
@@ -103,7 +105,9 @@ export class ProtocolError extends Error {
  * are brand-matched and work across separately bundled copies of the SDK.
  */
 export class ResourceNotFoundError extends ProtocolError {
-    protected static override readonly mcpBrand: string = 'mcp.ResourceNotFoundError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.ResourceNotFoundError' });
+    }
 
     constructor(uri: string, message: string = `Resource not found: ${uri}`) {
         super(ProtocolErrorCode.InvalidParams, message, { uri });
@@ -120,7 +124,9 @@ export class ResourceNotFoundError extends ProtocolError {
  * This makes it nicer for the client to handle since there is specific data to work with instead of just a code to check against.
  */
 export class UrlElicitationRequiredError extends ProtocolError {
-    protected static override readonly mcpBrand: string = 'mcp.UrlElicitationRequiredError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.UrlElicitationRequiredError' });
+    }
 
     constructor(elicitations: ElicitRequestURLParams[], message: string = `URL elicitation${elicitations.length > 1 ? 's' : ''} required`) {
         super(ProtocolErrorCode.UrlElicitationRequired, message, {
@@ -143,7 +149,9 @@ export class UrlElicitationRequiredError extends ProtocolError {
  * version that was requested (`requested`).
  */
 export class UnsupportedProtocolVersionError extends ProtocolError {
-    protected static override readonly mcpBrand: string = 'mcp.UnsupportedProtocolVersionError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.UnsupportedProtocolVersionError' });
+    }
 
     constructor(data: UnsupportedProtocolVersionErrorData, message: string = `Unsupported protocol version: ${data.requested}`) {
         super(ProtocolErrorCode.UnsupportedProtocolVersion, message, data);
@@ -179,7 +187,9 @@ export class UnsupportedProtocolVersionError extends ProtocolError {
  * copies of the SDK.
  */
 export class MissingRequiredClientCapabilityError extends ProtocolError {
-    protected static override readonly mcpBrand: string = 'mcp.MissingRequiredClientCapabilityError';
+    static {
+        Object.defineProperty(this, 'mcpBrand', { value: 'mcp.MissingRequiredClientCapabilityError' });
+    }
 
     constructor(
         data: MissingRequiredClientCapabilityErrorData,

packages/core-internal/test/errors/crossBundleBrand.test.ts

@@ -132,6 +132,29 @@ describe('cross-bundle error instanceof (branded)', () => {
         expect(own instanceof SdkError).toBe(true);
     });
 
+    it('matches across versions by identity, not shape (contract lock)', () => {
+        // Brand strings are version-less by design: an instance constructed by a
+        // different SDK version cross-matches. instanceof implies identity, not
+        // field shape - consumers read fields defensively.
+        const BRANDS = Symbol.for('mcp.sdk.errorBrands');
+        const olderAlpha = Object.create(Error.prototype) as { status?: number };
+        Object.defineProperty(olderAlpha, BRANDS, { value: new Set(['mcp.SdkError', 'mcp.SdkHttpError']), enumerable: false });
+        expect((olderAlpha as unknown) instanceof SdkHttpError).toBe(true);
+        expect(olderAlpha.status).toBeUndefined();
+    });
+
+    it('does not throw on hostile proxies (falls back to prototype check)', () => {
+        const hostile = new Proxy(
+            {},
+            {
+                getOwnPropertyDescriptor() {
+                    throw new Error('trap');
+                }
+            }
+        );
+        expect((hostile as unknown) instanceof SdkError).toBe(false);
+    });
+
     it('tolerates primitives, null, and brandless objects', () => {
         for (const value of [null, undefined, 42, 'x', Symbol('s'), {}, new Error('plain')]) {
             expect((value as unknown) instanceof SdkError).toBe(false);