feat(client): UnauthorizedError under auto, ClientOptions.logLevel, list page-1 opt-out, typed era-validation error#2377
Conversation
🦋 Changeset detectedLatest commit: 2dcacb5 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}
| if (reply.kind === 'send-error' && reply.error instanceof UnauthorizedError) { | ||
| // Authorization required: not era evidence and not a negotiation | ||
| // failure — surface the same UnauthorizedError the legacy connect | ||
| // path throws so callers can finishAuth() and reconnect. (The | ||
| // alternative — treating 401 as a legacy-fallback signal — would | ||
| // re-run the entire OAuth flow on the initialize send before | ||
| // throwing the same error.) Downstream cleanup (window detach, | ||
| // transport close) is the existing throw path. | ||
| throw reply.error; | ||
| } |
There was a problem hiding this comment.
🔴 docs/client.md (lines 300–303) and its sourced runnable example examples/guides/clientGuide.examples.ts (lines 296–299, the auth_finishAuth snippet) still document and demonstrate the connect-time 401 surfacing wrapped as SdkError(EraNegotiationFailed) with the .data.cause unwrapping — the exact shape this PR removes (the negotiateEra short-circuit now rethrows UnauthorizedError directly in every negotiation mode). Both should be simplified to the plain if (e instanceof UnauthorizedError) pattern this PR enables, matching the migration-table wording added in this diff.
Extended reasoning...
What is stale
This PR's headline ergonomic change is that negotiateEra() in packages/client/src/client/versionNegotiation.ts (the new short-circuit at ~lines 354–363) rethrows the UnauthorizedError from the connect-time probe directly, so a connect-time 401 is no longer wrapped in SdkError(EraNegotiationFailed).data.cause under versionNegotiation: 'auto' / { pin }. The PR description shows the before/after, the new tests pin the direct rethrow, and the migration table edited in this same diff (docs/migration/upgrade-to-v2.md) now states: "Auth-required connects throw UnauthorizedError directly in every negotiation mode — EraNegotiationFailed is reserved for genuine negotiation failures."
However, the primary OAuth client guide was not updated. docs/client.md:300-303 still says: "With version negotiation, the connect-time 401 may surface wrapped as SdkError(EraNegotiationFailed) whose .data.cause is the UnauthorizedError" and demonstrates digging error.data.cause out of the wrapped error. The guide snippet is sourced from the runnable example examples/guides/clientGuide.examples.ts:296-299 (region auth_finishAuth), which carries the identical comment and unwrapping code:
// With version negotiation, the connect-time 401 may surface wrapped as
// SdkError(EraNegotiationFailed) whose .data.cause is the UnauthorizedError.
const root = error instanceof UnauthorizedError ? error : (error as { data?: { cause?: unknown } }).data?.cause;
if (!(root instanceof UnauthorizedError)) throw error;Step-by-step proof that the documented shape no longer exists
- A user follows docs/client.md and connects with
versionNegotiation: 'auto'against an auth-gated server. The transport's auth layer rejects the probe send withUnauthorizedError. - With this PR,
negotiateEra()hits the newreply.kind === 'send-error' && reply.error instanceof UnauthorizedErrorcheck and rethrows theUnauthorizedErroritself; the olderror.name === 'UnauthorizedError'row innormalizeReply(which fed the legacy-fallback / wrapping path) was deleted in this same diff. connect()therefore rejects with a bareUnauthorizedError— exactly what the new test "'auto': UnauthorizedError from the probe send rejects connect() directly — no EraNegotiationFailed wrapping" asserts.- In the documented snippet, the
error instanceof UnauthorizedErrorbranch matches first, so the code still works at runtime — but the comment describing a wrappedEraNegotiationFailedshape and the.data.causefallback are now describing behavior the SDK never produces. The guide is teaching readers the precise workaround wart this PR exists to eliminate.
Why nothing else prevents it
Neither docs/client.md nor examples/guides/clientGuide.examples.ts is touched by this PR, and the example still type-checks and behaves correctly (the instanceof branch handles the new direct throw), so no test or build step flags the stale prose. All four verifiers confirmed the staleness independently; none refuted it. This is a documentation-consistency issue, not a runtime bug.
Impact and fix
Readers of the main client OAuth guide will keep writing the .data.cause unwrapping dance, undermining the PR's stated ergonomic win and contradicting the migration table updated in this very diff. The fix is small: update both sites (the example file is the source of truth for the doc snippet) to the plain pattern the PR description itself shows —
} catch (error) {
if (!(error instanceof UnauthorizedError)) throw error;
// The transport called redirectToAuthorization(); fall through to the browser callback.
}— and drop the now-obsolete comment about the wrapped EraNegotiationFailed shape.
| // Legacy half of ClientOptions.logLevel: a single best-effort | ||
| // logging/setLevel after the handshake when the server advertises | ||
| // logging. A failure here must not fail connect(). The connect-time | ||
| // signal/onprogress are deliberately NOT propagated — this request | ||
| // outlives connect() and a post-resolve abort would surface as a | ||
| // spurious onerror. | ||
| if (this._logLevel !== undefined && this._serverCapabilities?.logging) { | ||
| void this.setLoggingLevel(this._logLevel).catch(error_ => | ||
| this.onerror?.(error_ instanceof Error ? error_ : new Error(String(error_))) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 The fire-and-forget logging/setLevel sent here is not awaited, so if the caller closes the client (or the transport drops) before the response arrives, the pending request is rejected with SdkError(ConnectionClosed) and the .catch routes it to onerror — the consumer sees a spurious error event for a connection they closed cleanly. Consider filtering SdkErrorCode.ConnectionClosed (and arguably RequestTimeout) in the catch, consistent with the best-effort intent stated in the comment above.
Extended reasoning...
What the bug is. The legacy half of ClientOptions.logLevel fires void this.setLoggingLevel(this._logLevel).catch(error_ => this.onerror?.(...)) at the end of _legacyHandshake, deliberately not awaited so a failure cannot fail connect(). Because the request outlives connect(), it is still in flight when the consumer's first close() (or a transport drop) lands — and at that point the SDK reports an error to onerror for a teardown the user initiated cleanly.
The code path. setLoggingLevel() goes through Protocol.request(), which registers a response handler keyed by request id. Protocol.close() / transport close runs _onclose() (packages/core-internal/src/shared/protocol.ts:798-807), which settles every outstanding response handler with SdkError(SdkErrorCode.ConnectionClosed, 'Connection closed'). That rejection propagates into the new .catch at packages/client/src/client/client.ts:1076-1078, which forwards it unconditionally to this.onerror.
Step-by-step proof. (1) new Client(info, { logLevel: 'info' }) connects to a 2025-era server that advertises logging in its capabilities. (2) _legacyHandshake completes the initialize exchange, sees this._logLevel !== undefined && this._serverCapabilities?.logging, and fires logging/setLevel without awaiting; connect() resolves immediately. (3) The caller does one quick operation (or none) and calls client.close() — a perfectly normal one-shot CLI / health-probe pattern — before the logging/setLevel response round-trips. (4) close() triggers Protocol._onclose(), which rejects the pending setLoggingLevel promise with SdkError(ConnectionClosed). (5) The .catch calls this.onerror?.(error), so the consumer's error handler fires (logging noise, alerting, etc.) for a connection they shut down deliberately and successfully.
Why existing code doesn't prevent it. The inline comment explicitly addresses one half of this class of problem: the connect-time signal/onprogress are not propagated because "a post-resolve abort would surface as a spurious onerror." But the post-resolve close race produces exactly the same spurious onerror, and nothing filters it — the catch forwards every rejection. Note that the existing modern-era auto-listen path (around client.ts:1212-1228) is awaited inside connect(), so it does not have this outlives-connect race; this fire-and-forget request is the only one the client issues.
Impact. Limited to noise: connect() still succeeds, close() still completes, and no state is corrupted. But onerror is the consumer's error-reporting hook, so SDK-initiated traffic intermittently emitting errors on clean shutdown will look like a real failure (and is timing-dependent, so it shows up flakily). The same applies to a RequestTimeout if a server that advertised logging simply never answers setLevel — also not actionable for the consumer given the request is best-effort by design.
How to fix. Swallow the expected best-effort failure modes in the catch, e.g.:
void this.setLoggingLevel(this._logLevel).catch(error_ => {
if (error_ instanceof SdkError && (error_.code === SdkErrorCode.ConnectionClosed || error_.code === SdkErrorCode.RequestTimeout)) {
return; // best-effort: the connection was closed (or the server never answered) before the response arrived
}
this.onerror?.(error_ instanceof Error ? error_ : new Error(String(error_)));
});Alternatively, track the pending promise and skip reporting once the client has been closed. Either keeps genuine failures (e.g. a protocol error from the server) visible while aligning the implementation with the stated best-effort intent.
| /** | ||
| * Per-request log opt-in. On 2026-07-28 connections the level is attached | ||
| * to every outgoing request as the `io.modelcontextprotocol/logLevel` | ||
| * `_meta` envelope key (without it, servers MUST NOT emit | ||
| * `notifications/message` for the request). On 2025-era connections the | ||
| * client sends a single best-effort `logging/setLevel` after the handshake | ||
| * when the server advertises the `logging` capability. A per-call | ||
| * `params._meta[LOG_LEVEL_META_KEY]` still overrides this default. | ||
| * | ||
| * The envelope key rides every outgoing message (notifications included); | ||
| * it is inert on messages that are not request-scoped logs. | ||
| * | ||
| * Note: MCP-level logging is in the SEP-2577 deprecation window; this | ||
| * option fronts that vocabulary deliberately because it is currently the | ||
| * only way to receive request-tied server logs on 2026-07-28 connections. | ||
| */ | ||
| logLevel?: LoggingLevel; | ||
|
|
There was a problem hiding this comment.
🟡 docs/migration/support-2026-07-28.md (section "ctx.mcpReq.log() and the per-request logLevel", ~lines 166-171) still says "The SDK Client does not auto-attach logLevel", but with this PR the Client does auto-attach the io.modelcontextprotocol/logLevel envelope key when ClientOptions.logLevel is set. Please amend that sentence (and the comparison table around line 441) to mention ClientOptions.logLevel as the SDK-level opt-in (default still off) — the PR updated upgrade-to-v2.md and the changeset but not this doc.
Extended reasoning...
What the bug is. This PR introduces ClientOptions.logLevel, which makes the Client auto-attach the io.modelcontextprotocol/logLevel _meta envelope key on every outgoing request on a 2026-07-28 connection (and send a single best-effort logging/setLevel after a 2025-era handshake). However, docs/migration/support-2026-07-28.md — the canonical doc for the 2026-07-28 feature area — still states in its logging section (~lines 166-171): "The SDK Client does not auto-attach logLevel, so handler logs on a default 2026-era exchange are silently suppressed until the client opts in." That prose now describes only the pre-PR state and omits the very SDK-level opt-in this PR ships.\n\nThe code path that contradicts it. packages/client/src/client/client.ts now stores options?.logLevel (_logLevel, ~line 539/622) and passes it into outboundEnvelope() (~line 692). The 2026 codec (packages/core-internal/src/wire/rev2026-07-28/codec.ts, outboundEnvelope) stamps LOG_LEVEL_META_KEY whenever material.logLevel !== undefined. The new tests in envelopeAutoEmission.test.ts ("attaches the LOG_LEVEL_META_KEY envelope key to every outgoing request on a modern connection") confirm the auto-attach behavior.\n\nWhy existing docs don't cover it. The PR updated docs/migration/upgrade-to-v2.md (only the SdkErrorCode table and EraNegotiationFailed wording) and the changeset, but no prose documentation was added for the new option, and the 2026 migration doc — the natural place a reader would look for "how do I receive request-tied server logs on a 2026 connection" — was not touched. Its comparison table (~line 441) likewise only mentions the per-request _meta key path.\n\nStep-by-step reader impact. 1) A user upgrades to a 2026-07-28 server and notices their handler's ctx.mcpReq.log() output never arrives. 2) They open support-2026-07-28.md and find the section titled "ctx.mcpReq.log() and the per-request logLevel". 3) The doc tells them the SDK Client does not auto-attach logLevel and that the only opt-in is per-call params._meta['io.modelcontextprotocol/logLevel']. 4) They hand-roll the per-call _meta attachment on every request — exactly the boilerplate this PR's ClientOptions.logLevel was added to eliminate — never discovering the option exists.\n\nWhy this is a nit rather than a hard contradiction. The default behavior is unchanged (no option set ⇒ no key attached), so the sentence is stale/incomplete rather than flat wrong, and "until the client opts in" could charitably be read as covering the new option. Nothing breaks at runtime. Still, the repo's review conventions ask to flag docs/**/*.md prose that now describes old behavior and to verify new features get prose documentation, not just JSDoc.\n\nHow to fix. Amend the sentence to something like: "The SDK Client does not attach logLevel by default; set ClientOptions.logLevel to have the Client auto-attach the io.modelcontextprotocol/logLevel envelope key on every request (a per-call _meta value still overrides it)." Optionally add a row/mention in the comparison table at ~line 441 pointing at ClientOptions.logLevel.
felixweinberger
force-pushed
the
fweinberger/client-ergo
branch
from
c572850 to
be8da40
Compare
| } catch (error) { | ||
| // Under `--legacy` the transport surfaces `UnauthorizedError` directly; | ||
| // under `mode: 'auto'` the version-negotiation probe is what got 401'd | ||
| // and wraps it in an EraNegotiationFailed `SdkError` whose `data.cause` | ||
| // and wraps it in an VersionNegotiationFailed `SdkError` whose `data.cause` | ||
| // is the original `UnauthorizedError`. Either way the auth driver has | ||
| // already run by the time we land here — DCR done, auth URL captured. | ||
| const root = error instanceof UnauthorizedError ? error : (error as { data?: { cause?: unknown } }).data?.cause; |
There was a problem hiding this comment.
🟡 The comment edited by this PR (the EraNegotiationFailed → VersionNegotiationFailed rename on this line) still says that under mode: 'auto' the connect-time 401 is wrapped in a VersionNegotiationFailed SdkError whose data.cause carries the original UnauthorizedError — but this same PR removes that wrapping (negotiateEra() now rethrows the UnauthorizedError directly in every negotiation mode). Suggest simplifying the comment + catch to the plain if (!(error instanceof UnauthorizedError)) throw error; pattern the PR description showcases (which also fixes the "an VersionNegotiationFailed" grammar).
Extended reasoning...
What is stale
This PR edits the comment in examples/oauth/client.ts (lines ~115–121), but only mechanically: it renames EraNegotiationFailed to VersionNegotiationFailed while leaving the surrounding claim intact — that under mode: 'auto' "the version-negotiation probe is what got 401'd and wraps it in an VersionNegotiationFailed SdkError whose data.cause is the original UnauthorizedError". That is exactly the behavior this PR removes. Note this is a different file from the earlier review comment, which covered docs/client.md and examples/guides/clientGuide.examples.ts (files the PR does not touch); this site is actively edited by the diff.
Why the claim is no longer true
negotiateEra() in packages/client/src/client/versionNegotiation.ts (~lines 354–363) now short-circuits on reply.kind === 'send-error' && reply.error instanceof UnauthorizedError and rethrows the UnauthorizedError itself, and the old error.name === 'UnauthorizedError' row in normalizeReply (the path that previously fed the wrapping/fallback) is deleted in this same diff. The new tests pin it: "'auto': UnauthorizedError from the probe send rejects connect() directly — no VersionNegotiationFailed wrapping". The migration table edited in this diff says the same: "Auth-required connects throw UnauthorizedError directly in every negotiation mode".
Step-by-step proof
- The example connects with
versionNegotiation: { mode: 'auto' }and aStreamableHTTPClientTransportwhoseauthProviderrejects the probe send withUnauthorizedError(the 401 path the example exists to exercise). - With this PR,
negotiateEra()hits the new short-circuit and rethrows the bareUnauthorizedError;connect()rejects with it directly. - In the example's catch,
error instanceof UnauthorizedErrormatches first, sorootis the error itself — the(error as { data?: { cause?: unknown } }).data?.causefallback on the next line is now dead code under'auto', and the comment describes a wrapped shape the SDK never produces after this PR.
Why nothing flags it
The example still runs and passes its checks (the instanceof branch handles the new direct throw), so no test or type-check catches the stale prose. Impact is purely misleading documentation: the file is one of the OAuth reference examples, and it teaches the .data?.cause unwrapping dance this PR's headline change exists to eliminate, contradicting the migration-table wording added in the same diff.
How to fix
Simplify the catch to the pattern shown in the PR description:
} catch (error) {
// Either way (--legacy or mode: 'auto') the transport's auth driver has
// already run by the time we land here — DCR done, auth URL captured —
// and connect() rejects with the UnauthorizedError directly.
if (!(error instanceof UnauthorizedError)) throw error;
challenged = true;
}This drops the now-false wrapping claim, removes the dead data?.cause unwrapping, and incidentally fixes the "an VersionNegotiationFailed" grammar introduced by the rename.
felixweinberger
force-pushed
the
fweinberger/client-ergo
branch
from
be8da40 to
2dcacb5
Compare
1 participant
Five small client-side ergonomics fixes.
UnauthorizedErrorunderversionNegotiation: 'auto'negotiateEra()short-circuits and rethrows theUnauthorizedErrorat the probe; the deaderror.namerow is removed;UnauthorizedErrornow sets.name.ClientOptions.logLevelThe client populates
OutboundEnvelopeMaterial.logLevel; on legacy connections it best-effort callslogging/setLevelonce after the handshake.{ allPages: false }on list verbs — returns the first page only with a rawnextCursor, instead of always auto-aggregating.SdkErrorCode.ResultProtocolMismatch(new) — when a peer returns a result whose shape doesn't match the negotiated protocol version's schema (e.g. a 2026 result missingresultType), the client now throws this typed code instead of a genericInvalidResult.SdkErrorCode.EraNegotiationFailed→VersionNegotiationFailed— renamed to match the publicversionNegotiationoption (the "era" grouping is internal). The codemod handles the rename.getDiscoverResult()— JSDoc now documents that missingttlMs/cacheScopeare defaulted (the spec says receivers SHOULD), unlikeresultTypewhich is rejected.How Has This Been Tested?
client 707/707, core-internal 1293/1293, e2e + integration cells.
Breaking Changes
None (additive; one new error code).
Types of changes
Checklist