v2 backwards compat: SdkError status code#2049
Conversation
🦋 Changeset detectedLatest commit: bfdca86 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/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
| get statusText(): string | undefined { | ||
| return this.data.statusText; | ||
| } |
There was a problem hiding this comment.
🟡 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
- Grep
packages/**/test/**/*.tsfor.statusText→ the only hits are mockResponseobjects being constructed (e.g.streamableHttp.test.ts:234statusText: 'Not Found',:769statusText: 'Forbidden',:1835statusText: 'Unauthorized') and an unrelatedresponse.statusTextread inmiddleware.test.ts. None areexpect(...).statusTextassertions on anSdkHttpErrorinstance. - The 404 test at
streamableHttp.test.ts:243-247doesrejects.toThrow(new SdkHttpError(..., { statusText: 'Not Found', ... })), but vitest'stoThrow(errorInstance)matches only on.message— it does not comparedatafields or exercise the getter, so this is not coverage of.statusText. - Grep
packages/core/test/forSdkHttpError→ no matches; there is no unit test for the class itself. - ⇒ The new public
.statusTextaccessor — 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');
Add
SdkHttpErrorsubclass for typed HTTP transport error handlingMotivation and Context
SdkErrorcarries HTTP failure details (status code, status text) in itsdata?: unknownfield. Consumers who need to inspect the HTTP status — for example, to handle 401 or 403 responses — must resort to unsafe casting:This also affects v1 migration: the old
StreamableHTTPErrorhad a typedcode: numberfor HTTP status, but the v2 replacementSdkErrorusescode: SdkErrorCode(an enum, not an HTTP status). There is no typed way to access the HTTP status in v2.How Has This Been Tested?
pnpm build:all— all packages build successfullypnpm typecheck:all— no type errorspnpm lint:all— no lint or formatting issuespnpm test:all— all existing tests pass (core: 552, client: 364)streamableHttp.test.ts,tokenProvider.test.ts, andsse.test.tsto verifyinstanceof SdkHttpErrorand the.statusaccessorBreaking Changes
None.
SdkHttpError extends SdkError, so existingcatchclauses andinstanceof SdkErrorchecks continue to work. This is a purely additive change — consumers can opt in to more precise error handling viainstanceof SdkHttpErrorwithout changing existing code.Types of changes
Checklist
Alternatives Considered
A. Generic
SdkError<C>with data type map — ASdkErrorDataMapmapping eachSdkErrorCodeto its data shape, withSdkError<C>.dataconditionally typed. Rejected because TypeScript doesn't narrow class generics via property checks — consumers can't writeif (error.code === SdkErrorCode.X)and get narroweddata. Requires a helper function, which is non-standard for error handling.B. Broaden
SdkError.datato{ status?: number; statusText?: string; ... }— Rejected because it's misleading: suggests everySdkErrormight have HTTP status fields, evenNotConnectedorRequestTimeoutwhich have no HTTP context.C. Add
httpStatusaccessor onSdkErrorbase class —get httpStatus(): number | undefinedthat extracts fromdata. Rejected because the base class shouldn't know about HTTP semantics, and consumers can't distinguish HTTP errors structurally at the type level.D. HTTP error subclass (chosen) —
SdkHttpError extends SdkErrorwith typeddata. Standardinstanceofpattern, backward compatible, follows established SDK patterns (AWS SDK v3ServiceException, StripeStripeAPIError, gRPCServiceError). One new class, no generics, no type maps.Additional context
Files changed:
packages/core/src/errors/sdkErrors.ts— AddedSdkHttpErrorDatainterface andSdkHttpErrorclass withstatus/statusTextgetterspackages/core/src/exports/public/index.ts— ExportedSdkHttpErrorandSdkHttpErrorDatapackages/client/src/client/streamableHttp.ts— Changed 5 throw sites fromSdkErrortoSdkHttpErrorfor HTTP failurespackages/client/src/client/sse.ts— Changed 1 throw site (401 after retry) fromSdkErrortoSdkHttpErrorpackages/client/test/client/streamableHttp.test.ts— Updated assertions to useSdkHttpErrorpackages/client/test/client/tokenProvider.test.ts— Updated assertions to useSdkHttpErrorpackages/client/test/client/sse.test.ts— Updated assertions to useSdkHttpErrorDesign decision: The
ClientHttpUnexpectedContenterror instreamableHttp.tswas intentionally left asSdkError— it fires on a 200 OK response with an unexpected content type, so it doesn't carry an HTTP error status and doesn't fit theSdkHttpErrorDatashape.Consumer usage after this change: