Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/add-sdk-http-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@modelcontextprotocol/core": minor
"@modelcontextprotocol/client": minor
---

Add `SdkHttpError` subclass with typed `.status` / `.statusText` accessors for HTTP transport failures. `StreamableHTTPClientTransport` now throws `SdkHttpError` (which extends `SdkError`) for non-OK HTTP responses; `SSEClientTransport` throws `SdkHttpError` for 401-after-reauth (circuit breaker).
22 changes: 12 additions & 10 deletions docs/migration-SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,19 @@
| `ErrorCode` | `ProtocolErrorCode` |
| `ErrorCode.RequestTimeout` | `SdkErrorCode.RequestTimeout` |
| `ErrorCode.ConnectionClosed` | `SdkErrorCode.ConnectionClosed` |
| `StreamableHTTPError` | REMOVED (use `SdkError` with `SdkErrorCode.ClientHttp*`) |
| `StreamableHTTPError` | REMOVED (use `SdkHttpError` with `SdkErrorCode.ClientHttp*`) |
| `WebSocketClientTransport` | REMOVED (use `StreamableHTTPClientTransport` or `StdioClientTransport`) |

All other **type** symbols from `@modelcontextprotocol/sdk/types.js` retain their original names. **Zod schemas** (e.g., `CallToolResultSchema`, `ListToolsResultSchema`) are no longer part of the public API — they are internal to the SDK. For runtime validation, use
`isSpecType.TypeName(value)` (e.g., `isSpecType.CallToolResult(v)`) or `specTypeSchemas.TypeName` for the `StandardSchemaV1` validator object. The keys are typed as `SpecTypeName`, a literal union of all spec type names.

### Error class changes

Two error classes now exist:
Three error classes now exist:

- **`ProtocolError`** (renamed from `McpError`): Protocol errors that cross the wire as JSON-RPC responses
- **`SdkError`** (new): Local SDK errors that never cross the wire
- **`SdkHttpError`** (extends `SdkError`): HTTP transport errors with typed `.status` and `.statusText` accessors

| Error scenario | v1 type | v2 type |
| --------------------------------- | -------------------------------------------- | ----------------------------------------------------------------- |
Expand All @@ -115,12 +116,12 @@
| Capability not supported | `new Error(...)` | `SdkError` with `SdkErrorCode.CapabilityNotSupported` |
| Not connected | `new Error('Not connected')` | `SdkError` with `SdkErrorCode.NotConnected` |
| Invalid params (server response) | `McpError` with `ErrorCode.InvalidParams` | `ProtocolError` with `ProtocolErrorCode.InvalidParams` |
| HTTP transport error | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttp*` |
| Failed to open SSE stream | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToOpenStream` |
| 401 after re-auth (circuit break) | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpAuthentication` |
| 403 after upscoping | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpForbidden` |
| HTTP transport error | `StreamableHTTPError` | `SdkHttpError` with `SdkErrorCode.ClientHttp*` |
| Failed to open SSE stream | `StreamableHTTPError` | `SdkHttpError` with `SdkErrorCode.ClientHttpFailedToOpenStream` |
| 401 after re-auth (circuit break) | `StreamableHTTPError` | `SdkHttpError` with `SdkErrorCode.ClientHttpAuthentication` |
| 403 after upscoping | `StreamableHTTPError` | `SdkHttpError` with `SdkErrorCode.ClientHttpForbidden` |
| Unexpected content type | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpUnexpectedContent` |
| Session termination failed | `StreamableHTTPError` | `SdkError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
| Session termination failed | `StreamableHTTPError` | `SdkHttpError` with `SdkErrorCode.ClientHttpFailedToTerminateSession` |
| Response result fails schema | `ZodError` (raw) | `SdkError` with `SdkErrorCode.InvalidResult` |

New `SdkErrorCode` enum values:
Expand Down Expand Up @@ -161,10 +162,11 @@
}

// v2
import { SdkError, SdkErrorCode } from '@modelcontextprotocol/client';
if (error instanceof SdkError && error.code === SdkErrorCode.ClientHttpFailedToOpenStream) {
const status = (error.data as { status?: number })?.status;
import { SdkHttpError, SdkErrorCode } from '@modelcontextprotocol/client';
if (error instanceof SdkHttpError) {
console.log('HTTP status:', error.status); // number — typed accessor
console.log('Status text:', error.statusText); // string | undefined
}

Check warning on line 169 in docs/migration-SKILL.md

View check run for this annotation

Claude / Claude Code Review

Unused SdkErrorCode import in migration-SKILL.md v2 example

nit: `SdkErrorCode` is imported here but never used in the example body — the review-fix rewrote the body to use `error.status`/`error.statusText` and dropped the `.code` check, but left `SdkErrorCode` in the import. Either drop it from the import, or add a `switch (error.code) { case SdkErrorCode.ClientHttp*: ... }` block to match the parallel example in `docs/migration.md`.
Comment thread
claude[bot] marked this conversation as resolved.
```

### OAuth error consolidation
Expand Down
13 changes: 7 additions & 6 deletions docs/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -652,10 +652,11 @@ These replace the pattern of calling `server.sendLoggingMessage()`, `server.crea

### Error hierarchy refactoring

The SDK now distinguishes between two types of errors:
The SDK now distinguishes between three types of errors:

1. **`ProtocolError`** (renamed from `McpError`): Protocol errors that cross the wire as JSON-RPC error responses
2. **`SdkError`**: Local SDK errors that never cross the wire (timeouts, connection issues, capability checks)
3. **`SdkHttpError`** (extends `SdkError`): HTTP transport errors with typed `.status` and `.statusText` accessors

#### Renamed exports

Expand Down Expand Up @@ -725,7 +726,7 @@ The new `SdkErrorCode` enum contains string-valued codes for local SDK errors:

#### `StreamableHTTPError` removed

The `StreamableHTTPError` class has been removed. HTTP transport errors are now thrown as `SdkError` with specific `SdkErrorCode` values that provide more granular error information:
The `StreamableHTTPError` class has been removed. HTTP transport errors are now thrown as `SdkHttpError` (a subclass of `SdkError` with typed `.status` and `.statusText` accessors) with specific `SdkErrorCode` values that provide more granular error information:

**Before (v1):**

Expand All @@ -744,12 +745,14 @@ try {
**After (v2):**

```typescript
import { SdkError, SdkErrorCode } from '@modelcontextprotocol/client';
import { SdkHttpError, SdkErrorCode } from '@modelcontextprotocol/client';

try {
await transport.send(message);
} catch (error) {
if (error instanceof SdkError) {
if (error instanceof SdkHttpError) {
console.log('HTTP status:', error.status); // number — no cast needed
console.log('Status text:', error.statusText); // string | undefined
switch (error.code) {
case SdkErrorCode.ClientHttpAuthentication:
console.log('Auth failed — server rejected token after re-auth');
Expand All @@ -764,8 +767,6 @@ try {
console.log('HTTP request failed');
break;
}
// Access HTTP status code from error.data if needed
const httpStatus = (error.data as { status?: number })?.status;
}
}
```
Expand Down
11 changes: 9 additions & 2 deletions packages/client/src/client/sse.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import type { FetchLike, JSONRPCMessage, Transport } from '@modelcontextprotocol/core';
import { createFetchWithInit, JSONRPCMessageSchema, normalizeHeaders, SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
import {
createFetchWithInit,
JSONRPCMessageSchema,
normalizeHeaders,
SdkError,
SdkErrorCode,
SdkHttpError
} from '@modelcontextprotocol/core';
import type { ErrorEvent, EventSourceInit } from 'eventsource';
import { EventSource } from 'eventsource';

Expand Down Expand Up @@ -286,7 +293,7 @@ export class SSEClientTransport implements Transport {
}
await response.text?.().catch(() => {});
if (isAuthRetry) {
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
throw new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
status: 401
});
}
Expand Down
25 changes: 15 additions & 10 deletions packages/client/src/client/streamableHttp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import {
JSONRPCMessageSchema,
normalizeHeaders,
SdkError,
SdkErrorCode
SdkErrorCode,
SdkHttpError
} from '@modelcontextprotocol/core';
import { EventSourceParserStream } from 'eventsource-parser/stream';

Expand Down Expand Up @@ -273,7 +274,7 @@ export class StreamableHTTPClientTransport implements Transport {
}
await response.text?.().catch(() => {});
if (isAuthRetry) {
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
throw new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
status: 401
});
}
Expand All @@ -288,7 +289,7 @@ export class StreamableHTTPClientTransport implements Transport {
return;
}

throw new SdkError(SdkErrorCode.ClientHttpFailedToOpenStream, `Failed to open SSE stream: ${response.statusText}`, {
throw new SdkHttpError(SdkErrorCode.ClientHttpFailedToOpenStream, `Failed to open SSE stream: ${response.statusText}`, {
status: response.status,
statusText: response.statusText
});
Expand Down Expand Up @@ -581,7 +582,7 @@ export class StreamableHTTPClientTransport implements Transport {
}
await response.text?.().catch(() => {});
if (isAuthRetry) {
throw new SdkError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
throw new SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication', {
status: 401
});
}
Expand All @@ -598,7 +599,7 @@ export class StreamableHTTPClientTransport implements Transport {

// Check if we've already tried upscoping with this header to prevent infinite loops.
if (this._lastUpscopingHeader === wwwAuthHeader) {
throw new SdkError(SdkErrorCode.ClientHttpForbidden, 'Server returned 403 after trying upscoping', {
throw new SdkHttpError(SdkErrorCode.ClientHttpForbidden, 'Server returned 403 after trying upscoping', {
status: 403,
text
});
Expand Down Expand Up @@ -629,7 +630,7 @@ export class StreamableHTTPClientTransport implements Transport {
}
}

throw new SdkError(SdkErrorCode.ClientHttpNotImplemented, `Error POSTing to endpoint: ${text}`, {
throw new SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, `Error POSTing to endpoint: ${text}`, {
status: response.status,
text
});
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -725,10 +726,14 @@ export class StreamableHTTPClientTransport implements Transport {
// We specifically handle 405 as a valid response according to the spec,
// meaning the server does not support explicit session termination
if (!response.ok && response.status !== 405) {
throw new SdkError(SdkErrorCode.ClientHttpFailedToTerminateSession, `Failed to terminate session: ${response.statusText}`, {
status: response.status,
statusText: response.statusText
});
throw new SdkHttpError(
SdkErrorCode.ClientHttpFailedToTerminateSession,
`Failed to terminate session: ${response.statusText}`,
{
status: response.status,
statusText: response.statusText
}
);
}

this._sessionId = undefined;
Expand Down
9 changes: 5 additions & 4 deletions packages/client/test/client/sse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { createServer } from 'node:http';
import type { AddressInfo } from 'node:net';

import type { JSONRPCMessage, OAuthTokens } from '@modelcontextprotocol/core';
import { OAuthError, OAuthErrorCode, SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
import { OAuthError, OAuthErrorCode, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
import type { Mock, Mocked, MockedFunction, MockInstance } from 'vitest';

Expand Down Expand Up @@ -1575,7 +1575,7 @@ describe('SSEClientTransport', () => {
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
});

it('enforces circuit breaker on double-401: onUnauthorized called once, then throws SdkError', async () => {
it('enforces circuit breaker on double-401: onUnauthorized called once, then throws SdkHttpError', async () => {
postResponses = [401, 401];
await setupServer();

Expand All @@ -1587,8 +1587,9 @@ describe('SSEClientTransport', () => {
await transport.start();

const error = await transport.send(message).catch(e => e);
expect(error).toBeInstanceOf(SdkError);
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect(error).toBeInstanceOf(SdkHttpError);
expect((error as SdkHttpError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect((error as SdkHttpError).status).toBe(401);
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
expect(postCount).toBe(2);
});
Expand Down
9 changes: 5 additions & 4 deletions packages/client/test/client/streamableHttp.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { JSONRPCMessage, JSONRPCRequest } from '@modelcontextprotocol/core';
import { OAuthError, OAuthErrorCode, SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
import { OAuthError, OAuthErrorCode, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';
import type { Mock, Mocked } from 'vitest';

import type { OAuthClientProvider } from '../../src/client/auth.js';
Expand Down Expand Up @@ -240,7 +240,7 @@ describe('StreamableHTTPClientTransport', () => {
transport.onerror = errorSpy;

await expect(transport.send(message)).rejects.toThrow(
new SdkError(SdkErrorCode.ClientHttpNotImplemented, 'Error POSTing to endpoint: Session not found', {
new SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, 'Error POSTing to endpoint: Session not found', {
status: 404,
text: 'Session not found'
})
Expand Down Expand Up @@ -1871,8 +1871,9 @@ describe('StreamableHTTPClientTransport', () => {
.mockResolvedValueOnce(unauthedResponse);

const error = await transport.send(message).catch(e => e);
expect(error).toBeInstanceOf(SdkError);
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect(error).toBeInstanceOf(SdkHttpError);
expect((error as SdkHttpError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect((error as SdkHttpError).status).toBe(401);
expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({
access_token: 'new-access-token',
token_type: 'Bearer',
Expand Down
9 changes: 5 additions & 4 deletions packages/client/test/client/tokenProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { IncomingMessage, Server } from 'node:http';
import { createServer } from 'node:http';

import type { JSONRPCMessage, OAuthClientInformation, OAuthClientMetadata, OAuthTokens } from '@modelcontextprotocol/core';
import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core';
import { SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
import type { Mock } from 'vitest';

Expand Down Expand Up @@ -86,7 +86,7 @@ describe('StreamableHTTPClientTransport with AuthProvider', () => {
expect(retryInit.headers.get('Authorization')).toBe('Bearer new-token');
});

it('should throw SdkError(ClientHttpAuthentication) if retry after onUnauthorized also gets 401', async () => {
it('should throw SdkHttpError(ClientHttpAuthentication) if retry after onUnauthorized also gets 401', async () => {
const authProvider: AuthProvider = {
token: vi.fn(async () => 'still-bad'),
onUnauthorized: vi.fn(async () => {})
Expand All @@ -99,8 +99,9 @@ describe('StreamableHTTPClientTransport with AuthProvider', () => {
.mockResolvedValueOnce({ ok: false, status: 401, headers: new Headers(), text: async () => 'unauthorized' });

const error = await transport.send(message).catch(e => e);
expect(error).toBeInstanceOf(SdkError);
expect((error as SdkError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect(error).toBeInstanceOf(SdkHttpError);
expect((error as SdkHttpError).code).toBe(SdkErrorCode.ClientHttpAuthentication);
expect((error as SdkHttpError).status).toBe(401);
expect(authProvider.onUnauthorized).toHaveBeenCalledTimes(1);
});

Expand Down
14 changes: 13 additions & 1 deletion packages/core/src/errors/sdkErrors.examples.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @module
*/

import { SdkError, SdkErrorCode } from './sdkErrors.js';
import { SdkError, SdkErrorCode, SdkHttpError } from './sdkErrors.js';

/**
* Example: Throwing and catching SDK errors.
Expand All @@ -25,3 +25,15 @@ function SdkError_basicUsage() {
}
//#endregion SdkError_basicUsage
}

/**
* Example: Checking for HTTP transport errors.
*/
function SdkHttpError_basicUsage(error: unknown) {
//#region SdkHttpError_basicUsage
if (error instanceof SdkHttpError) {
console.log(error.status); // number
console.log(error.statusText); // string | undefined
}
//#endregion SdkHttpError_basicUsage
}
42 changes: 42 additions & 0 deletions packages/core/src/errors/sdkErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,45 @@ export class SdkError extends Error {
this.name = 'SdkError';
}
}

/**
* Typed shape for HTTP error data carried by {@linkcode SdkHttpError}.
*/
export interface SdkHttpErrorData {
status: number;
statusText?: string;
[key: string]: unknown;
}

/**
* An {@linkcode SdkError} subclass for HTTP transport failures.
*
* Thrown by the streamable HTTP transport when the server responds with a
* non-OK status code. Narrows {@linkcode SdkError.data | data} to
* {@linkcode SdkHttpErrorData} so consumers can inspect the HTTP status
* without unsafe casting.
*
* @example
* ```ts source="./sdkErrors.examples.ts#SdkHttpError_basicUsage"
* if (error instanceof SdkHttpError) {
* console.log(error.status); // number
* console.log(error.statusText); // string | undefined
* }
* ```
Comment thread
claude[bot] marked this conversation as resolved.
*/
export class SdkHttpError extends SdkError {
declare readonly data: SdkHttpErrorData;

constructor(code: SdkErrorCode, message: string, data: SdkHttpErrorData) {
super(code, message, data);
this.name = 'SdkHttpError';
}

get status(): number {
return this.data.status;
}

get statusText(): string | undefined {
return this.data.statusText;
}
Comment on lines +107 to +109

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: The new .statusText getter has no test assertions — the three tests this PR touched assert (error as SdkHttpError).status but none assert .statusText, and there's no SdkHttpError unit test in packages/core/test/. Since commit bfdca86 now populates statusText at every throw site, consider adding expect((error as SdkHttpError).statusText).toBe('Unauthorized') alongside the existing .status checks (e.g. streamableHttp.test.ts:1877) so both new accessors are covered.

Extended reasoning...

What

This PR adds two new public getters to SdkHttpError (packages/core/src/errors/sdkErrors.ts:101-109):

get status(): number { return this.data.status; }
get statusText(): string | undefined { return this.data.statusText; }

The .status getter is asserted in three tests this PR touched — streamableHttp.test.ts:1877, sse.test.ts:1592, and tokenProvider.test.ts:104 — each adds expect((error as SdkHttpError).status).toBe(401) directly below the instanceof / .code assertions. But the parallel .statusText getter has zero assertions anywhere in the test suite.

Step-by-step proof

  1. Grep packages/**/test/**/*.ts for .statusText → the only hits are mock Response objects being constructed (e.g. streamableHttp.test.ts:234 statusText: 'Not Found', :769 statusText: 'Forbidden', :1835 statusText: 'Unauthorized') and an unrelated response.statusText read in middleware.test.ts. None are expect(...).statusText assertions on an SdkHttpError instance.
  2. The 404 test at streamableHttp.test.ts:243-247 does rejects.toThrow(new SdkHttpError(..., { statusText: 'Not Found', ... })), but vitest's toThrow(errorInstance) matches only on .message — it does not compare data fields or exercise the getter, so this is not coverage of .statusText.
  3. Grep packages/core/test/ for SdkHttpError → no matches; there is no unit test for the class itself.
  4. ⇒ The new public .statusText accessor — advertised in the changeset (.changeset/add-sdk-http-error.md), the JSDoc @example, and both migration docs (docs/migration.md:755, docs/migration-SKILL.md:168) — ships with no test verifying it returns what was passed in.

Why this is worth a nit now

The previous review round (#3247761715) specifically asked the author to populate statusText at the five throw sites that were omitting it, and commit bfdca86 did so — every SdkHttpError throw site now passes statusText: response.statusText. So the data plumbing exists end-to-end, but the only half of the new accessor pair that's actually verified is .status. Per REVIEW.md § Tests & docs ("New behavior has vitest coverage"), a new public getter that's documented in the changelog and migration guide should have at least one assertion.

Why nothing catches it

The getter is a one-line passthrough (return this.data.statusText) structurally identical to the already-tested .status getter, so type-checking and the existing suite are happy. There's no coverage threshold gate on accessor lines. Risk of an actual bug is near-zero — hence nit, not a blocker.

Fix

Add a one-liner alongside any of the existing .status assertions, e.g. in streamableHttp.test.ts (the mock response at line 1835 already sets statusText: 'Unauthorized'):

expect((error as SdkHttpError).status).toBe(401);
expect((error as SdkHttpError).statusText).toBe('Unauthorized');

Or add a tiny unit test in packages/core/test/errors/sdkErrors.test.ts covering both getters directly:

const e = new SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, 'msg', { status: 500, statusText: 'Internal Server Error' });
expect(e.status).toBe(500);
expect(e.statusText).toBe('Internal Server Error');

}
Comment thread
claude[bot] marked this conversation as resolved.
3 changes: 2 additions & 1 deletion packages/core/src/exports/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
export { OAuthError, OAuthErrorCode } from '../../auth/errors.js';

// SDK error types (local errors that never cross the wire)
export { SdkError, SdkErrorCode } from '../../errors/sdkErrors.js';
export type { SdkHttpErrorData } from '../../errors/sdkErrors.js';
export { SdkError, SdkErrorCode, SdkHttpError } from '../../errors/sdkErrors.js';
Comment thread
claude[bot] marked this conversation as resolved.

// Auth TypeScript types (NOT Zod schemas like OAuthMetadataSchema)
export type {
Expand Down
Loading