fix(core): start OAuth on a modern/auto-era connect (#1805) - #1809
Conversation
The SDK's `server/discover` negotiation probe runs before anything else on `protocolEra: "auto" | "modern"`, and its classifier reports whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved to `data.cause`. That buried both connect-time auth signals: the remote path's `AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the authorization URL) and a direct transport's `AuthChallengeError`. An OAuth-protected server that authorized fine on the legacy era instead produced a dead-end "Version negotiation probe failed" — no redirect, and the connection status flipped to `error` despite `isConnectAuthRecoveryError` existing to prevent exactly that. - `findNestedAuthError()` walks `cause` / `data.cause` for either typed error. Not gated on the SDK's error code or message, so it survives rewording. - `connect()` unwraps the rejection at the earliest point, ahead of `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the outer status guard, and every client's connect error handling — so web, CLI, and TUI are all fixed without a client-side change. - On the direct path, enable `interceptAuthChallenges` for the probing eras even with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely, no status and not even a cause for the walk to find. Verified against a live dual-era server that answers 401 on both `initialize` and `server/discover`: `auto` and `modern` now reject with `AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the fix is load-bearing — reverting either fails the new integration tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om
…workaround Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as #1807). Make its lifespan explicit so whoever picks up #1807 knows this block is the thing to delete, rather than having to re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
@claude review |
|
Claude finished @cliffhall's task in 4m 38s —— View job Review — PR #1809 (
|
Review findings on #1809: - connect() built its handshake promise once, outside the runConnect closure withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg re-awaited the already-rejected promise and rejected a connect() whose client was in fact connected. Short-circuit the retry when already connected, and skip the post-connect block the nested connect already ran (a duplicate `connect` event re-triggers every list-state manager's refresh). - Document the accepted 403 side effect of enabling interceptAuthChallenges with no stored tokens, and why every `{ pin }` counts as probing. - Replace the private-method double cast in the era-probe test with an observable assertion on the transport options, and split the non-auth failure case per era instead of a message alternation that passed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
Thanks — all four addressed in fc0395e. Point by point: 1. Intercept also catches a non-auth 403 — accepted, now documentedAgreed on the analysis and on the trade-off. Kept the behavior (narrowing to 401 would mean threading a status filter through a transport option that is boolean today, for a clause we intend to delete), and wrote the side effect into the workaround comment so it's not rediscovered later:
2. Satisfied recovery rejecting a connected client — real, fixed hereConfirmed by reading the path:
New 3.
|
|
@claude review — second pass. Since the last review: the satisfied-recovery |
|
Claude finished @cliffhall's task in 3m 57s —— View job Review — PR #1809, second pass
Verdict: the short-circuit is correct, and all four first-pass items are settled. I traced the state machine rather than taking the test's word for it; the reasoning below is why I think the The short-circuit — why it holds
The failure path also stays coherent: if the nested connect throws (refreshed token still rejected, or Findings1. Narrow race: if the recovered session dies before the retry leg runs, The discriminator is live status, so an Rejecting there is arguably right (the connection is genuinely gone); it's the error identity that's off. The structural form would be to key off "am I the retry leg" rather than live status — e.g. set the flag in 2. Two one-line strengthenings for the regression test The test pins the load-bearing facts (resolves,
A negative case (satisfied recovery → nested connect fails → 3. Pre-existing, unchanged: recovery recursion isn't bounded across the nesting boundary
First-pass items — all settled
One small optional gap in the new intercept test: it only exercises the Not verified locally: code reading only — no |
…ries Second-pass review findings on #1809: - The satisfied-recovery short-circuit keyed off live connection status, so an onclose landing between the nested connect() and the retry leg fell through to the rejected handshake promise and reported the stale 401 as the reason the session died. withDirectAuthRecovery now passes `attempt` to its operation, so connect() identifies the retry leg structurally and reports the teardown when the recovered session is already gone. - Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a satisfied outcome recovers via a nested connect() with a fresh counter, so a server that keeps rejecting freshly refreshed tokens recursed without bound. Counted across the nesting boundary, capped at 3. - Strengthen the regression test (server info populated by the nested connect, no duplicate statusChange) and add the two missing cases: a failed reconnect still rejects, and the recursion bound terminates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
All three addressed in 54f904d — including the one you scoped out, since this PR is handling its own follow-ups rather than filing them. 1. The stale-401 race — fixed structurallyYou're right that the error identity was the problem, and the structural form is better than the status proxy. Took your first suggestion: const runConnect = async (attempt: number): Promise<void> => {
if (attempt > 0) {
recoveredByNestedConnect = true;
if (this.status !== "connected") {
throw new Error(
"Connection closed during authorization recovery, after re-authorizing successfully",
);
}
return;
}
…So the close-between-legs case now rejects with the teardown reason instead of the buried handshake 401, and the rejected 2. Test strengthenings — both added, plus the negative case
3. Unbounded recovery recursion — bounded hereHandling it in this PR rather than filing it. New test drives the pathological shape (every transport challenges, every challenge reports satisfied) and pins termination at exactly 4 transports — the initial connect plus three bounded recoveries — instead of recursing to a stack overflow. On the optional left-arm caseAgreed it isn't owed by this diff — the Gate
One note in case it bites someone else: with |
|
@claude review — third pass. Since the second review: the retry leg is now identified by |
|
Claude finished @cliffhall's task in 5m 3s —— View job Review — PR #1809, third pass
Verdict: both changes are correct, and the
|
Third-pass review findings on #1809, both about tests added in the previous commit: - The retry leg's teardown error had no coverage, so a regression to the old status fall-through would have stayed green. Covered deterministically: the nested connect dispatches `connect` after setting "connected", so closing the live transport from that listener flips the status before the retry leg runs. Asserts the error identity, which is the point of the change. - "still rejects when the reconnect underneath the recovery fails" used a challenging transport for every connect, so it walked the same bounded chain as the recursion-bound test rather than covering a non-auth reconnect failure, and its comment claimed an "error" status the auth path never reaches. Now fails the reconnect with a plain error and asserts both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt
|
Findings 1 and 2 are fair — both are defects in tests I added last round, so they're fixed in f1af65a. The rest I'm declining, and I'm closing the review here rather than running a fourth pass. Reasoning at the end. 1. Teardown branch untested — fixedYou're right that this was the one line the commit was named for and the only one not asserted. Covered with your suggested mechanism: the nested connect dispatches 2. Duplicated case with a wrong comment — fixedCorrect on both counts: it was byte-identical in behavior to the bound test, and the comment's 3.
|
|
@claude review — fourth pass, scoped confirmation only. The only change since the third pass is f1af65a, which is test-only: it covers the retry leg's teardown branch (asserting the error identity) and rewrites Please verify just those two things are now correct. As stated above, the remaining items from pass 3 (the |
|
Claude finished @cliffhall's task in 1m 44s —— View job Review — PR #1809, fourth pass (scoped confirmation)
Both are correct. Confirmed — nothing further from me, and I'm not re-raising the four declined items. I traced each test against the source rather than trusting the names. 1. Teardown branch — covered, and it pins the error identity
The mechanism works and is deterministic, not timing-dependent:
It discriminates both regressions, which is what makes it worth having: a revert to a bare 2.
|
|
Fourth pass came back clean — no findings, both test fixes confirmed, and the reviewer explicitly closed out ("nothing further from me, and I'm not re-raising the four declined items"). So the review reached its natural end rather than being cut short. No further changes are planned on this PR. Why:
Note for merge: this targets |
2 participants
Closes #1805
Closes #1806
Maintainer-run version of @rinormaloku's #1806 — the same fix, rebased onto current
v2/main, with the one review change applied and the fullcichain run locally (external PRs can't trigger workflows without approval, and #1806 had never been through the gate).Supersedes #1806; please close that PR in favour of this one. Original commit authorship is preserved.
The bug
Connecting to an OAuth-protected HTTP server with Protocol Era = Auto or Modern never started the authorization flow. The same server authorized fine on Legacy, and the user saw a red toast instead of the OAuth redirect:
Root cause
Era
auto/modernsends the SDK'sserver/discovernegotiation probe before anything else, so the probe — notinitialize— is where authorization first surfaces. Its classifier reports whatever the transport threw asSdkError(ERA_NEGOTIATION_FAILED), keeping the real error atdata.cause. That buried both connect-time auth signals:interceptAuthChallenges: true, so a 401 becomesAuthChallengeError→auth_challenge→RemoteClientTransportrejects withAuthRecoveryRequiredError(carrying the authorization URL). Wrapped, it no longer matchederr instanceof AuthRecoveryRequiredErrorinApp.tsx, andisUnauthorizedError()couldn't see it either (nostatus/code). It also flipped connection status toerror, whichisConnectAuthRecoveryErrorexists to prevent.authProvider, so no interception, so the 401 reached the SDK as a rawSdkHttpError. The classifier ignores the HTTP status — it only looks for a JSON-RPC error body — and verdicts "not a modern server"; pin mode then rethrew that with the 401 discarded entirely: no status, not even acause.client.connect()AuthRecoveryRequiredErrorSdkError/ERA_NEGOTIATION_FAILEDSdkError/ERA_NEGOTIATION_FAILEDChanges
core/auth/challenge.ts—findNestedAuthError()walkscause/data.causefor a nestedAuthRecoveryRequiredError/AuthChallengeError. Deliberately not gated on the SDK's error code or message, so it survives SDK rewording. This is the permanent fix.core/mcp/inspectorClient.ts—connect()unwraps theclient.connect()rejection at the earliest point: ahead ofwithDirectAuthRecovery(whoseisAuthChallengeErrorcheck is shallow), the outer status guard, and every client's connect-error handling. Web, CLI, and TUI are all fixed with no client-side change.core/mcp/inspectorClient.ts— on the direct path, enableinterceptAuthChallengesfor the probing eras even with no stored tokens, so the 401 becomes a typedAuthChallengeErrorthat survives the probe asdata.cause. This half is a workaround for the upstream gap and is now commented as removable — see below.Review change applied on top of #1806
The era-scoped guard is kept exactly as written; only its comment changed, so its lifespan is explicit for whoever picks up #1807:
Tests
clients/web/src/test/core/auth/challenge.test.ts— 8 cases over the walk (both link names, multi-level, already-typed, absent, non-object/nulldata, cyclic chain).clients/web/src/test/core/mcp/inspectorClient-era-probe-auth.test.ts—connect()surfaces each typed error onauto/modern, leaves a non-auth probe failure untouched, and keeps the legacy baseline; plusprobesProtocolEra().clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts— live modern-era server behindrequireAuth:connect()rejects with a recoverable auth error (not a negotiation failure) on both probing eras, and connects reporting the modern era once authorization completes.Each half of the fix is load-bearing — reverting either one fails the new integration tests.
Gate:
npm run cirun locally on this branch — green end to end (validate→coverageper-file ≥90 gate →verify:build-gate→smokeincl. headless-Chromium web boot → Storybook 108 files / 460 tests).Follow-ups (tracked, not in this PR)
AuthorizationServerMismatchError).🤖 Generated with Claude Code
https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt