Skip to content

v2 backwards compat: SdkError status code #2049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KKonstantinov merged 6 commits into from
May 21, 2026
Merged

v2 backwards compat: SdkError status code #2049

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
676bf18
backwards compat: SdkError codes, introduce SdkHttpError
KKonstantinov May 11, 2026
604f95a
lint fix
KKonstantinov May 11, 2026
37b8c37
review fixes
KKonstantinov May 15, 2026
4932aae
review fixes
KKonstantinov May 15, 2026
d9a904f
fix docs
KKonstantinov May 15, 2026
bfdca86
fix nit
KKonstantinov May 15, 2026
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
11 changes: 9 additions & 2 deletions packages/client/src/client/sse.ts
View file
Open in desktop
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
View file
Open in desktop
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
7 changes: 4 additions & 3 deletions packages/client/test/client/sse.test.ts
View file
Open in desktop
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, SdkError, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
import type { Mock, Mocked, MockedFunction, MockInstance } from 'vitest';

Expand Down Expand Up @@ -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
View file
Open in desktop
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, SdkError, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';

Check warning on line 2 in packages/client/test/client/streamableHttp.test.ts

View check run for this annotation

Claude / Claude Code Review

Unused SdkError import left in three test files 

nit: `SdkError` is imported here but no longer referenced after the assertions migrated to `SdkHttpError` — same in `sse.test.ts:6` and `tokenProvider.test.ts:5`. Drop it from all three import lists (and optionally update the two test-name strings that still say "throws SdkError" to say `SdkHttpError`).
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
import type { Mock, Mocked } from 'vitest';

import type { OAuthClientProvider } from '../../src/client/auth.js';
Expand Down Expand Up @@ -240,7 +240,7 @@
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 @@
.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
7 changes: 4 additions & 3 deletions packages/client/test/client/tokenProvider.test.ts
View file
Open in desktop
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 { SdkError, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core';
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
import type { Mock } from 'vitest';

Expand Down Expand Up @@ -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
42 changes: 42 additions & 0 deletions packages/core/src/errors/sdkErrors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,45 @@
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
* if (error instanceof SdkHttpError) {
* console.log(error.status); // number
* console.log(error.statusText); // string | undefined
* }
* ```

Check warning on line 93 in packages/core/src/errors/sdkErrors.ts

View check run for this annotation

Claude / Claude Code Review

SdkHttpError @example is inline, not sourced from .examples.ts 

nit: Per CLAUDE.md § "JSDoc `@example` Code Snippets", `@example` blocks should pull type-checked code from a companion `.examples.ts` file via a `source=` fence — the sibling `SdkError` JSDoc just above (line 47) already does this with `source="./sdkErrors.examples.ts#SdkError_basicUsage"`. Consider adding an `SdkHttpError_basicUsage` region to `sdkErrors.examples.ts` and referencing it here so the snippet is type-checked and kept in sync by `pnpm sync:snippets`.
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

@claude claude Bot May 15, 2026

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
View file
Open in desktop
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';

Check warning on line 17 in packages/core/src/exports/public/index.ts

View check run for this annotation

Claude / Claude Code Review

Missing changeset for new public API export 

This PR adds `SdkHttpError` and `SdkHttpErrorData` to the curated public API surface but doesn't include a changeset (the changeset-bot has already flagged this). Since this is an intentional new-feature export re-exported by `@modelcontextprotocol/client` and `@modelcontextprotocol/server`, please add a `minor` changeset for `@modelcontextprotocol/core` and `@modelcontextprotocol/client` so consumers get a version bump and changelog entry — `pnpm changeset` will generate one.
Comment thread
claude[bot] marked this conversation as resolved.

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