SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)

[cliffhall]

Closes #1624

Migrates all three clients + core + test-servers from @modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior identical against legacy (2025-11-25) servers with versionNegotiation: { mode: 'legacy' }. This is the behavior-neutral foundation ("1 of 10") for the SDK V2 + New Spec stack.

Packages

• @modelcontextprotocol/sdk → @modelcontextprotocol/client (Client, transports, OAuth, error taxonomy, all protocol types) + @modelcontextprotocol/core (Zod *Schema values). stdio → client/stdio; Ajv → client/validators/ajv. Test-servers → @modelcontextprotocol/server (+ server-legacy/sse).
• Import routing was done with a deterministic rewrite (schemas→core, everything else→client), because the official codemod misroutes client code to @modelcontextprotocol/server.
• new Client(..., { versionNegotiation: { mode: 'legacy' } }).

Runtime (core/mcp/inspectorClient.ts)

• Error taxonomy: McpError→ProtocolError, ErrorCode→ProtocolErrorCode; timeouts → SdkError/SdkErrorCode.
• Handlers take method strings; tasks/* + notifications/tasks/* use the 3-arg custom form (v2 excludes them from the spec-method set).
• Legacy tasks kept working (project requirement — the Inspector debugs servers of any spec era):
  • Requestor tasks re-implemented over raw client.request + explicit deprecated schemas; experimental.tasks.callToolStream → a local pollTaskToolCall async generator, with _progressHandlers revival so task-execution progress still reaches the caller.
  • Receiver (server-initiated) tasks: v2's Client validates a sampling/elicitation handler result and rejects the legacy { task } response with -32602. installReceiverTaskResponseBypass swaps the wrapped entry in the private _requestHandlers map for the task-augmented branch so { task } reaches the wire. Fixes server-initiated sampling-via-task (verified against @modelcontextprotocol/server-everything + e2e).
• Pagination: list verbs drop to raw client.request({method}, List*ResultSchema) for single-page behavior (v2's client.listX() auto-aggregate all pages).

Web / auth / apps

• Local JsonSchemaType → InspectorFormSchema + a toFormSchema() structural narrow at the SDK-schema→form boundary.
• ext-apps still peers on SDK v1 → one compat boundary with documented casts (delete when ext-apps#702 ships a v2 peer).
• OAuth provider saveClientInformation reconciled with v2's OAuthClientInformationContext; InvalidClientMetadataError re-homed as a local class (server-legacy /auth isn't browser-safe).
• vitest.shared.mts dedupes client+core to one copy so vi.mock intercepts the SUT's import.

Accepted v2 behavior changes (per #1624 / spec §9.2)

Unknown-tool call rejects with -32602; cancellation is stream-abort (no guaranteed notifications/cancelled frame); output-schema strictness restored via the Inspector's own Ajv check on the default path.

Verification — npm run ci green

• validate: web 3087, cli 212, tui 274, launcher 5; all builds green
• coverage (≥90 per-file, all four dimensions): web / cli / tui pass
• smoke: launcher + cli (tools/list over stdio) + web (GET / → 200)
• storybook: 410

Note: tsc -p test-servers --noEmit reports 5 pre-existing (non-SDK) errors that also exist on origin/v2/main (OAuth discriminated-union narrowing ×4 + a vitest types resolution); the build uses --noCheck and tsc -b is clean, so they don't block CI. Can file a follow-up.

---

RFC 9207 iss — a real v2 regression found by live smoke testing

Smoke-testing EMA against live xaa.dev staging (Okta's public XAA sandbox) surfaced a genuine regression this PR introduced. It is now fixed; recording it here because it is the one place the migration was not behavior-neutral, and because the CI gap that hid it is worth knowing about.

Symptom

Real IdP login succeeded, then the leg 1 code exchange threw:

EMA leg 1 (IdP authorization code exchange): Issuer mismatch in authorization
response (RFC 9207): expected "https://idp.xaa.dev", received undefined

Cause

SDK v2 added RFC 9207 §2.4 authorization-response issuer validation (validateAuthorizationResponseIssuer / IssuerMismatchError) as a mix-up-attack defense, gained an iss parameter, and made it strict:

if (iss === undefined) {
  if (issParameterSupported) throw new IssuerMismatchError("authorization_response", expectedIssuer, undefined);
  return; // lenient only when the AS does not advertise it
}

SDK v1.29.0 had no iss parameter and no RFC 9207 validation at all (verified against the installed v1 tree — exchangeAuthorization(url, { metadata, clientInformation, authorizationCode, codeVerifier, redirectUri, resource, addClientAuthentication, fetchFn })). So dropping the callback iss was harmless under v1 and instantly fatal under v2 against any AS advertising authorization_response_iss_parameter_supported: true — which real IdPs do.

The migration swapped in a stricter SDK without feeding it the new input it requires. Behavior-neutral against the SDK surface; not against real authorization servers.

Scope — this was not EMA-only

iss was dropped in three places, with the plumbing half-built (oauthManager.completeOAuthFlow and inspectorClient.completeOAuthFlow already accepted iss, but nothing supplied or forwarded it):

Layer	Gap
core/auth/utils.ts	parseOAuthCallbackParams never extracted iss from the callback query
core/auth/mcpAuth.ts	never forwarded options.iss to SDK auth() — standard OAuth broke identically
core/auth/ema/idpOidc.ts	never passed iss to exchangeAuthorization (EMA leg 1)

mcpAuth.ts carried this comment on origin/v2/main, which predicted the exact gap:

/** RFC 9207 callback iss — forwarded on v2 upgrade; ignored by v1 SDK auth(). */

This is the v2 upgrade; the forwarding was never wired. Any RFC 9207 server would have broken standard OAuth too.

Fix

Thread iss end to end: callback query → CallbackParams → resumeAfterOAuth → completeOAuthFlow → {mcpAuth | EMA leg 1} → SDK. The Node callback server + runner carry it so CLI/TUI match web.

Why CI did not catch it

ema-mock-servers.ts never advertised authorization_response_iss_parameter_supported — the string appeared nowhere in the repo. The mock IdP was therefore more permissive than any real IdP, so the SDK took its lenient branch and all EMA tests passed over a broken flow.

The mock now advertises it, making it at least as strict as production. Flipping that one flag made three existing tests immediately reproduce the live failure. Added regression coverage for iss forwarding and for rejecting a mismatched iss before any network call.

Also note the e2e (inspectorClient-ema-e2e.test.ts) only covers silent EMA (cached IdP session → legs 2–3); it never drives leg 1's code exchange. Leg 1 interactive coverage remains unit-level — worth a follow-up.

Verification

Manually verified end to end against live xaa.dev staging: EMA legs 1–3 (IdP SSO → ID-JAG mint → resource token) complete and the MCP session initializes against a local protected-resource test server (test-servers/configs/xaa-ema-http.json).

Suites after the fix: web 3099 (was 3087), cli 212, tui 274 — green.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 0s —— View job

---

Review: SDK v2 migration (#1688)

I read the diff against origin/v2/main, focusing on the substantive files (core/mcp/inspectorClient.ts, the apps compat boundary, toFormSchema, vitest.shared.mts) rather than the ~90 mechanical one-line import rewrites. Overall this is a careful, well-documented migration — the tricky parts (receiver-task bypass, pollTaskToolCall, legacy pagination) each carry a clear rationale comment, there are no leftover @modelcontextprotocol/sdk imports in source, the ext-apps v1→v2 casts are confined to single boundaries with ext-apps#702 TODOs, and CI is green. Nice work. A few things worth a look before merge:

1. core/ was left unformatted (slips through CI) — minor

The deterministic import rewrite collapsed the previously multi-line imports into single long lines and left statements like throw new ProtocolError(ProtocolErrorCode.InvalidParams, ...) unwrapped (inspectorClient.ts:727). These are not Prettier-clean:

• core/mcp/inspectorClient.ts was Prettier-clean on origin/v2/main and is not after this PR.
• This passes CI only because core/ is in no format gate — web's format:check is prettier --check src, and the repo root has no format script. prettier --check "core/**/*.ts" reports ~50 warnings (mostly pre-existing, but this PR added inspectorClient.ts, types.ts regressions to the pile).

Not a functional issue, but easy to fix and keeps the diff idiomatic. Suggest running prettier --write core/ and, separately, considering whether core/ should be added to a format gate so this doesn't drift.
Fix this →

2. Default-path output validation now throws a plain Error — worth verifying

In callToolWithRetries the output-schema check changed from SDK-does-it to we-do-it:

const outputValidationError = this.validateToolOutput(tool, result as CallToolResult);
if (outputValidationError && !options?.skipOutputValidation) {
  throw new Error(outputValidationError);   // <- plain Error
}

Restoring the strict default check is the right call (v2's callTool relaxed it). But on v1 the strict violation surfaced as the SDK's typed protocol error (McpError/ProtocolError with a code), whereas now it's a bare Error with only a message. Any downstream handling that branches on instanceof ProtocolError or an error code (e.g. the retry/cancel/MethodNotFound logic elsewhere in this file) will see a different shape for output-schema failures. Worth a quick confirm that nothing downstream keys off the error type here; if it does, consider throwing a ProtocolError(ProtocolErrorCode.InvalidParams, …) to match the prior contract.

3. pollTaskToolCall progress-subscription revival is a fragile heuristic — low risk, document the assumption

The "find the only new key in _progressHandlers" diff (inspectorClient.ts) relies on no other request registering a progress handler between the keysBeforeRequest snapshot and this request's synchronous registration:

const keysBeforeRequest = new Set(progressHandlers.keys());
const requestPromise = client.request({ method: "tools/call", params }, …);
const progressSubscriptionId = requestOptions.onprogress
  ? [...progressHandlers.keys()].find((k) => !keysBeforeRequest.has(k))
  : undefined;

find returns the first new key, so two task-augmented tools/calls constructed in the same microtask window could cross-wire their subscription ids. In practice tool calls are user-driven and serial, so this is very unlikely — but since it (like installReceiverTaskResponseBypass) reaches into SDK privates, it's worth an explicit "assumes serial construction" note so a future concurrent-calls change doesn't silently regress task progress. Both private-field accesses are already flagged as delete-when-SDK-models-this-natively, which is the right framing.

Nits

• AppDetailPanel.tsx:49 — the /* v8 ignore next -- unreachable */ on toFormSchema(inputSchema) ?? {} is justified (a Tool.inputSchema is always an object), and the ignore is correctly scoped to the fallback line. Good.
• toFormSchema / InspectorFormSchema structural narrow and the vitest.shared.mts dedupe of client+core are clean and well-commented.

None of these block the behavior-neutral goal; #1 and #2 are the ones I'd resolve before merge. Everything else LGTM.

[cliffhall]

Thanks for the review — all three addressed in f9f79ff:

1. core/ unformatted. Ran Prettier on the 60 core/ files this PR touches — the deterministic import rewrite had collapsed the multi-line imports and left throw statements unwrapped. Now Prettier-clean. Adding core/ to a format gate is a good idea but out of scope here (it'd flag the ~50 pre-existing warnings in untouched core/ files and balloon this behavior-neutral diff) — worth a dedicated follow-up.

2. Plain Error on default-path output validation. Good catch — switched to ProtocolError(ProtocolErrorCode.InvalidParams, …) to match the prior v1 contract (the SDK surfaced a typed error with a .code), so any downstream instanceof ProtocolError / .code branching keeps working. The strict-violation test still passes.

3. pollTaskToolCall progress heuristic. Added an explicit "ASSUMES SERIAL CONSTRUCTION" note documenting the invariant (no other request registers a progress handler between the snapshot and this request's synchronous registration — true for user-driven serial tool calls) and pointing at the delete-when-SDK-models-this-natively framing.

Nits acknowledged (no change needed). npm run ci still green after the changes.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 15s —— View job

---

Re-review: SDK v2 migration — after f9f79ff

• Read the substantive files (inspectorClient.ts, receiver-task bypass, pollTaskToolCall, toFormSchema, apps compat)
• Verify the three prior-review fixes landed in f9f79ff
• Scan the changed core / web / test-server files for new issues
• Post review

I re-read the diff against origin/v2/main with a focus on f9f79ff (the fix commit) and the tricky runtime seams. All three points from the last review are correctly resolved, and I found nothing new that blocks merge. Details:

✅ 1. core/ formatting

The 60 touched core/ files are now clean under the repo's pinned Prettier (^3.8.1). Confirmed the collapsed imports were re-wrapped and the throw at inspectorClient.ts:2046 is now multi-line. (Heads-up so it doesn't cause future confusion: running an ad-hoc npx prettier fetches 3.9.x, which changed union-type formatting and will falsely flag files like types.ts/connection-state.ts — those are clean under 3.8.x, so ignore that noise. Adding core/ to a real format gate pinned to the repo version remains a good follow-up, as you noted.)

✅ 2. Typed output-validation error

callToolWithRetries now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) on the default strict path (inspectorClient.ts:2046-2049), restoring the v1 contract so downstream instanceof ProtocolError / .code branching keeps working. The skipOutputValidation (MCP Apps passthrough) path correctly stays non-fatal and surfaces the same message as an advisory (outputValidationError on the invocation). Good.

✅ 3. pollTaskToolCall progress heuristic

The ASSUMES SERIAL CONSTRUCTION note (inspectorClient.ts:2209-2216) documents the invariant precisely — no other request may register a progress handler between the keysBeforeRequest snapshot and this request's synchronous registration — and ties it to the same delete-when-SDK-models-this-natively framing as installReceiverTaskResponseBypass. Exactly the explicit assumption I was after.

Also spot-checked (all clean)

• No leftover @modelcontextprotocol/sdk source imports anywhere in core/, clients/*/src, clients/web/server, or test-servers/src — the only v1 residue is the ext-apps peer dep, which is expected and confined to the documented cast boundary.
• createAppBridgeFactory.ts — the v1→v2 AppBridge casts use ConstructorParameters<typeof AppBridge> / Parameters<…> rather than blanket any, are localized to the single construction/getToolUiResourceUri boundary, and carry the ext-apps#702 TODO. Nicely contained.
• installReceiverTaskResponseBypass — wraps the handler once per connect against a fresh Client, dispatches the task-augmented branch to the raw handler only when receiverTasks && task != null, and falls through to the validating handler otherwise. No double-wrap risk on reconnect. Sound.
• AppDetailPanel.tsx — toFormSchema(inputSchema) ?? {} with the correctly-scoped /* v8 ignore next */ on the unreachable fallback is fine.

Behavior-neutral goal looks met. LGTM to merge. 👍

[cliffhall]

Human review of all changes. Thee are some requests, primarily "as unknown as" - justify or refactor.

[cliffhall]

Addressed your review (commit dbb8c40) — replied inline on each thread. Summary:

as unknown as casts — evaluated every one in the PR; they're two SDK gaps, now each commented with the specific missing API so it can inform SDK-side type work:

• Private-field access (_requestHandlers client + server, _progressHandlers): the SDK has no public way to opt a handler out of _wrapHandler's result validation, nor to keep a progress subscription alive across a resolved request. These are the receiver-task and task-progress bypasses.
• Task-augmented returns (sampling/elicit { task }): the 2-arg setRequestHandler overload types the return as the spec result only, not the wire-valid CreateTaskResult. Refactored to construct a typed CreateTaskResult first (shape now checked) so only the honest gap-bridging cast remains.
• serverList.ts (pre-existing): plain object→Record structural widening (no index signature → TS2352 without the unknown step), not an SDK workaround — commented; can fold into a typed helper in a follow-up if you'd like.

"Did X schema disappear?" — no, none did. ListTools/ListResources/ListResourceTemplates/Subscribe/UnsubscribeRequestSchema all still live in @modelcontextprotocol/core. The change is the handler API: v2's setRequestHandler takes a method string ('tools/list', 'resources/subscribe', …) instead of the schema object and resolves/validates it internally — same on client and server.

typecheck + receiver-task/e2e tests still green.