feat(mcp-ts)!: swap to @modelcontextprotocol/client 2.0 - #4093
poshinchen wants to merge 1 commit into
Conversation
|
@strandly-the-agent review this PR carefully. |
poshinchen
changed the title
github-actions
Bot
added
chore
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -221,11 +223,17 @@ export class McpClient { | |||
| } | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Issue: The notification/request handlers now key on string literals ('notifications/message' here, 'elicitation/create' at L322) instead of the old schema constants. If the 2.0 client types these method names as a plain string, a typo would silently register a handler that never fires — the compiler wouldn't catch it, and the log/elicitation path would go quietly dead.
Suggestion: Confirm @modelcontextprotocol/client provides typed overloads keyed on these literal method names (so a typo is a compile error). If it doesn't, consider referencing an exported constant/schema to keep this checked. Non-blocking if the overloads are typed.
There was a problem hiding this comment.
Confirmed — a typo is a compile error. The 2.0 client types both handler registrations as overloads keyed on literal method maps, so a misspelling fails to compile:
src/mcp/client.ts(226,41): error TS2345: Argument of type '"notifications/mesage"'
is not assignable to parameter of type 'NotificationMethod'.
A valid but wrong method is caught too, because the handler's parameter type is keyed to the method — swapping in 'notifications/cancelled' errors on the missing level/data fields. Since type-check is a pre-commit and CI gate, the string-literal form is no weaker than the schema form here.
|
API review label: This PR is labeled
Suggestion: These change an existing public API contract, so per |
|
Assessment: Comment Clean, well-scoped swap to Review Categories
Nice work keeping the public |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
Verdict: requesting changes — the swap itself is solid and the legacy-transport compat claim held up under a live repro, but tasksConfig breaks in two independently reproduced ways for users who upgrade without opting into anything, and four doc comments promise the opposite behavior.
Endorsing the earlier API-label comment: this needs api/needs-review and a Breaking Changes section — and note the changelog tooling marks a release breaking only via ! in the title or a breaking change label, so as titled/labelled today this publishes as non-breaking.
Inline: 2 🔴 (task-required tools become uncallable · ttl silently becomes a hard cap) and 4 🟡. Everything non-blocking is collapsed below.
Evidence — what was verified
✅ reviewed branch feat/mcp-v2-ts-swap @ 4d3c211 (base main @ 1adf5f1)
✅ build · type-check · lint · format:check — all clean
✅ npm run test → 4505/4507; the 2 failures are src/telemetry/__tests__/config.test.ts picking up this
sandbox's OTEL_RESOURCE_ATTRIBUTES env (6/6 pass with it unset) — unrelated to this diff
✅ focused MCP + tools suites → 277/277
⚪ integ suites not run here (need AWS credentials)
✅ legacy-transport compat survived a live repro: legacy 1.30 server + legacy in-memory transport →
probe server/discover → -32601 → legacy initialize fallback → connect + listTools + callTool OK
(one extra round trip per connect)
✅ elicitation round-trip on a 2.0 stack (callback receives request.params; context.mcpReq.signal is a
real AbortSignal; 'signal' in context === false) and abort-mid-call (~200 ms, with and without
tasksConfig) both behave as documented
🔴 repro: task-required tool — main returns the result; this branch throws ProtocolError -32602
🔴 repro: ttl 1200 ms + progress every 300 ms → timed out at 1203 ms; identical call with a progress
handler registered → resolved at 3011 ms
🔴 measured: stdio server spawns per connect — legacy mode 1, auto mode 2
✅ proposed fixes verified: onprogress → tsc clean (3 assertions to update); elicitation shim → tsc
clean (5 fixtures to update); warn-test addition passes and catches the mutation
Process note: seven independent review passes ran (correctness, scope/alignment, API design, adversarial repro, test quality, model-facing text, docs accuracy); the API-design and adversarial passes were re-run on a smaller model tier after infrastructure timeouts on the first attempt.
Questions (3 blocking · 4 non-blocking)
Blocking
- Is the wholesale peer swap the ratified direction? #3708 (maintainer-approved) took the opposite posture ("the dependency pin stays
<2and nothing changes for current installs"), Python still pinsmcp<2.0.0behind a compat shim, and #4038 asked exactly this question — dual support vs a breaking swap, offering to write a design proposal — with no maintainer answer yet. I found no decision record inteam/DECISIONS.md,team/designs/, #1659 or its sub-issues. - Is dropping TS task execution outright the agreed shape of "unwinding both tasks implementations"? Python keeps real task execution behind an identically-named
TasksConfig, so after this PR the same name means different things in the two SDKs. ShouldtasksConfigbe deprecated/no-op for the gap rather than repurposed? - May vendor types appear directly in the Strands public surface (see the
ElicitationContextcomment)? Worth a decision record either way — this is the second break of that exact field.
Non-blocking
- Would client-level
requestTimeoutsand/or the existing per-call options be a cleaner home for these knobs than repurposingtasksConfig? - Is
McpTransportstill needed now that 2.0'sTransportalready declaressessionId?: string | undefined— and are theas McpTransport/as Transportcasts in this diff intentional? - Is tools-changed auto-refresh still functional against a 2026-07-28 stateless server (the client is 2.0 but the
listChangedoptions are legacy-shaped), and is any gap tracked? - What drives the new
@modelcontextprotocol/serverdevDependency versus reusing the legacy fixture server the integ fixtures deliberately keep?
Reading order
Start with strands-ts/package.json — the peer moving from sdk to client is the whole user-visible contract, and most findings trace back to it. Then src/mcp/client.ts, the bulk of the change: the import collapse at the top, the constructor (negotiation mode and the new warning), the elicitation handler registration, and last the callTool tasks branch with its doc comments — that branch is where both blockers live. src/types/elicitation.ts is three lines but is the headline breaking change; read it next, then src/tools/mcp-tool.ts for the error-class rename and its model-facing text, then src/mcp/config.node.ts (import paths only). Tests after that: client.test.ts (the new mock strategy, then the tasksConfig and elicitation blocks), config.test.node.ts, and client-annotations.test.ts — the one place a real 2.0 server is driven. Finish with the two describe.skips in test/integ/mcp/mcp-tasks.test.node.ts, which read best alongside the first blocker.
Appendix — non-blocking (7)
- ⚪
TasksConfigfield docs still describe polling ("time-to-live … for task polling", "wait for task completion during polling") when nothing polls — folds into the doc pass of the second blocker; the earlier bot comment noted this too. - ⚪ The
McpTransportdoc rationale is stale (it describes the legacy base type; 2.0 already declaressessionId?: string | undefined), and "without requiring explicit casts" is contradicted by this diff's ownas McpTransport/as Transportcasts. - ⚪ The new
tasksConfigwarning isn't actionable: no tracking reference, no "what to do instead", and no@experimental/@deprecatedJSDoc tag despite in-repo precedent for both — IDEs and coding agents see nothing. - ⚪ The auto-negotiation probe sets no probe timeout (inherits the 60 s default): a non-conforming server that silently swallows unknown methods stalls
connect()for 60 s and then hard-fails. Contrived — spec-conforming servers reply-32601. - ⚪ A
-32042with an empty/missingelicitationspayload now loses the "URL elicitation required" semantic in the model-facing text (bareError: Auth required). Degenerate servers only. (Otherwise the error-text change is a net improvement for the model — the numeric code never reached it as a field anyway.) - ⚪
client.test.ts:1091,1119build the expected text from the sameString(error)expression the implementation uses, so they can't detect rendering drift — the exact thing this PR changes. The sibling tests pin literals; these two should too. - Pre-existing, filed #4095:
connect(true)cannot reconnect a StreamableHTTP client on main or this branch (already started!), with a latent skip-handshake hazard noted for whoever fixes it.
Suggested follow-ups once direction is settled: a user-like install CI job (peer-only — would have caught the README/examples imports), a "restore task-augmented invocation + re-enable the mcp-tasks integ suites" tracker cited from the skips, and an ./mcp subpath export as the path to an optional peer.
| ...(this._elicitationCallback ? { capabilities: { elicitation: { form: {}, url: {} } } } : undefined), | ||
| // Probe for protocol revision 2026-07-28 and fall back to the legacy initialize | ||
| // handshake, mirroring the Python SDK's negotiate_auto posture. | ||
| versionNegotiation: { mode: 'auto' }, |
There was a problem hiding this comment.
🟡 Auto negotiation is unconditional, and it starts every stdio MCP server process twice per connect.
Measured with the repo's own fixture server, only this option differing:
legacy mode: 1 spawn
auto mode: 2 spawns
The probe runs on a throwaway sibling process, so any stdio server with startup side effects — locks, state writes, registration, billed cold starts — does them twice. Every stdio user gets this with no opt-out (the dependency's own default is 'legacy'), and on HTTP the probe adds hard connect() failure modes where the previous code connected.
Suggestion: expose the mode as an McpClientOptions field defaulting to 'auto' so probe-hostile setups have an escape hatch, and mention the double start in the PR body. Cost: one new public option, which is itself API surface.
4d3c211 to
79d37c1
Compare
|
Re-review (updated commit) — Assessment: Comment (was Request Changes) The two blockers from the prior review are resolved, and cleanly:
Two smaller items remain open (details on the respective threads): Remaining
Nice turnaround on the two hard problems — the deprecate-and-throw plus |
|
@strandly-the-agent review the updated changes |
79d37c1 to
61312dd
Compare
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
Re-review of 79d37c1 — both blockers are fixed and I re-verified them with live repros; what's left is small and known.
| Prior finding | Status |
|---|---|
| 🔴 task-required tools uncallable | ✅ Fixed — deprecate-and-throw; repro re-run (listTools works, callTool throws with the #1659 link) |
🔴 resetTimeoutOnProgress inert |
✅ Fixed — requestTimeouts + onprogress shim; full timeout matrix repro'd against a live server |
| 🟡 warn test one-sided / stale elicitation fixtures | ½ — warn test now bidirectional ✅; fixtures still pass the pre-2.0 { signal } shape (mcpReq appears nowhere in tests) |
🟡 README + examples/mcp install break |
Still open (thread stands) — 3 import lines + 1 package.json line |
🟡 ElicitationContext vendor alias · 🟡 auto-negotiation double stdio spawn |
Open, acknowledged in the body as pending maintainer input |
The rewritten PR body (Breaking Changes section, "Type of Change: Breaking change") resolves my process finding — but the labels haven't caught up: still chore, no api/needs-review, and the changelog tooling only marks a release breaking via ! in the title or a breaking change label, so as labelled this still publishes as non-breaking. With the new requestTimeouts surface added, the api/needs-review label is now doubly warranted.
I'd flip to approve once the README/examples fix and the labels land; the remaining design threads are maintainer calls, not author defects.
Evidence — what was verified on 79d37c1
✅ delta reviewed: 4d3c211 → 79d37c1 (452 diff lines; force-push, same single commit)
✅ build · type-check · lint · format:check — all clean
✅ unit tests → 4508/4508 (run with the sandbox's OTEL_RESOURCE_ATTRIBUTES unset, per yesterday's finding)
✅ repro (tasksConfig): listTools OK with tasksConfig set; callTool threw as documented at ~122 ms with the
#1659 link; construction warn fired, and only when tasksConfig is set
✅ repro matrix (requestTimeouts, live 2.0 server, tool answering at 3 s, progress every 300 ms):
· {timeout:1200, resetTimeoutOnProgress:true} → RESOLVED at 3005 ms (reset works via the shim)
· {timeout:1200} → timed out at 1201 ms (reset off, as documented)
· {timeout:1200, reset:true, maxTotalTimeout:2000} → capped at 2106 ms (total cap reachable now)
· reset:true + per-call abort at 500 ms → rejected at 501 ms (signal survives the merge)
✅ the corrected doc comments now match measured behavior (the four false progress-reset claims are gone)
Repro caveat worth recording: my first run of the reset case timed out and briefly looked like the fix was broken — the fault was my harness (wrong notify signature corrupted the progress notifications on the wire). With well-formed notifications the fix works exactly as documented.
New in this delta — minor (2)
- ⚪ The new
loadServersper-serverrequestTimeoutskey (config.node.ts:123) has no test inconfig.test.node.ts— the siblingtasksConfigplumb line does. - ⚪ "Per-call options take precedence on overlap" (
client.ts:127):McpCallToolOptionsis still{ signal? }, so no overlap is actually expressible today — fine as future-proofing, just noting the doc slightly leads the type.
Process note
Per the follow-up-review procedure this delta was verified directly by me (repros + gates) rather than a full pass fan-out, since it responds to my own findings — with one exception: the new public requestTimeouts/McpRequestTimeouts surface warranted a fresh API-design pass, which is still running as I post. If it surfaces anything material I'll add it to this thread; absence of a follow-up comment means it came back clean.
|
Re-review ( Every defect raised across the prior rounds is now closed and verified against the branch: Resolved
The only things still open are the three maintainer-direction questions the author explicitly lists (swap-vs-dual-support per #4038, whether vendor types may sit on the public surface, exposing the negotiation mode) — those are governance decisions, not code issues, so I'm leaving the verdict at Comment for a maintainer to make the directional/ Solid, responsive iteration — the deprecate-and-throw + |
|
The API-design pass on the new 🟡 The PR body's Python-parity rationale doesn't match the merged Python code. The body says the throw "mirrors the recorded Python decision … raise a clear error rather than maintain a second implementation" — but Design questions (non-blocking) + one nit
|
poshinchen
changed the title
9561319 to
39c394b
Compare
|
Re-review ( The breaking-marking gap from the previous round is closed, and the Verified on this commit
Suggestion (non-blocking): the underlying commit subject is still Handing the three open direction questions (swap-vs-dual-support #4038, vendor types on the public surface, exposing negotiation mode) to the |
39c394b to
c664101
Compare
|
Rebase check ( Verified the rebase-risk areas rather than taking "no functional changes" on faith: What I checked on the rebased tip
Only remaining items are still the three API-direction questions for the |
|
@strandly-the-agent review the changes and descriptions |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approving at c664101 — every code-level finding from my two earlier rounds is fixed and re-verified on the rebased tip, and the description now matches the code. What remains is the api/needs-review sign-off on two design threads I left open (vendor type as ElicitationContext; unconditional auto-negotiation), which are maintainer calls, not defects.
| Prior finding | Status on c664101 |
|---|---|
🔴 task-required tools uncallable · 🔴 resetTimeoutOnProgress inert |
✅ still fixed — live repros re-run on the rebased tip (throw with the new wording; reset-on survives to 3 s, reset-off dies at 1.2 s, maxTotalTimeout caps at 2 s) |
🟡 README + examples/mcp install break |
✅ fixed — both import @modelcontextprotocol/client, example declares the peer |
| 🟡 elicitation fixtures on pre-2.0 shape | ✅ fixed — { mcpReq: { signal } } and an assertion on receivedContext.mcpReq.signal |
| 🟡 Python-parity claim in the body | ✅ fixed — now cites #4125 (verified merged 2026-09-04) instead of a non-existent "raise" decision |
🟡 ElicitationContext vendor alias · 🟡 stdio double-spawn under auto negotiation |
Open by design → api/needs-review owner |
Description nits, none blocking: the body should mention the negotiation probe's side effect (stdio servers start twice per connect) since it now has a place for behavior changes; "per-call options taking precedence" still can't fire because McpCallToolOptions is only { signal? }; the Testing section lost the npm run check line (I ran the TS gates — all green); and chore is still on a feat! PR.
Evidence — verified on c664101 (rebased onto 7462908)
✅ interdiff of the PR's own change, 79d37c1 → c664101: 87 lines, all accounted for — README/example imports,
example package.json peer, elicitation fixtures + mcpReq.signal assertion, new loadServers requestTimeouts
test, @deprecated tags removed (0 left in src/mcp) → "temporarily unavailable" wording in JSDoc / warn /
throw, resetTimeoutOnProgress docstring now documents the internal handler. No behavior change; the rest
of the diff-vs-main delta is the rebase (prefixWithServerName + requestTimeouts coexist in config.node.ts).
✅ build · type-check · lint · format:check — clean
✅ unit tests → 4611/4611 (165 files)
✅ repros (live 2.0 server, InMemoryTransport): tasksConfig → listTools OK, callTool throws with #1659;
requestTimeouts {1200, reset:true} → resolved 3006 ms; {1200} → timed out 1202 ms;
{1200, reset:true, maxTotal:2000} → capped 2106 ms
✅ description claims: #4125 merged (Python tasks on both mcp lines) — true; #3661 exists as an open draft
with an unfilled template ("candidate implementation" is fair, "draft" would be more precise);
tasksConfig "not deprecated" — true; Breaking Changes 1–4 match the code; title carries `!`.
⚪ integ suites not run here (need AWS credentials)
Process note: the delta only repairs earlier findings plus wording, so I verified it directly rather than fanning out passes. Threads I opened that the author resolved were re-checked against the tip before I counted them fixed.
c664101 to
fd2e762
Compare
|
Re-verify ( |
fd2e762 to
e4c101f
Compare
|
Re-review ( 🔴 Critical —
🟡 Important — 🟢 Suggestion (a good change the description undersells) — Net: the swap/transport/tasks core still looks solid, but the description no longer describes this commit. Worth confirming whether the |
|
@strandly-the-agent review the updated changes. |
There was a problem hiding this comment.
Re-review of e4c101f — code is clean and the description now matches it (the bot's "out of sync" flag predates the body edit). The delta since my approval does two things, both verified: requestTimeouts is removed end to end, and ElicitationContext gains a compat signal field so the breaking change becomes a deprecation. One description sentence still misleads; the rest is the maintainer's release-plan call.
| Delta item | Verified |
|---|---|
requestTimeouts removed (type, option, loadServers key, wiring, tests) |
✅ zero references left in src/ or test/; callTool forwards per-call options as on main; body no longer advertises it |
ElicitationContext = ClientContext & { signal } shim (client.ts:329) |
✅ lossless at runtime — the vendor context is a plain object with no prototype members or getters, mcpReq is copied by reference with notify/send intact, signal === mcpReq.signal, elicitation round-trip completes; test pins both paths (client.test.ts:587-603) |
| Breaking Changes list | ✅ ElicitationContext correctly moved out of it; items 1–3 match the code |
| My open thread: unconditional auto-negotiation (stdio double spawn) | still open → api/needs-review owner |
🟡 Description, Breaking Change #2 — "while tasksConfig is disabled … affected tool calls fall back to the MCP client's default 60-second inactivity timeout" reads as if calls degrade gracefully. They don't: while tasksConfig is set, every callTool throws (client.ts:471); the 60-second default only applies after the user removes the option. Suggest: "callTool throws while tasksConfig is set. After removing it, tool calls run under the MCP client's default 60-second inactivity timeout with no ttl/pollTimeout equivalent until #3661 lands."
For pgrayy's open question, the concrete exposure if #3661 does not make the same release: a tasksConfig user upgrades, connects fine, and hits a thrown error on the first tool call; with requestTimeouts gone there is no timeout knob to migrate to — only the 60 s default plus a per-call AbortSignal. If that's acceptable as a same-release dependency, nothing else blocks on the code side.
Evidence — verified on e4c101f (base 381ab48)
✅ interdiff of the PR's own change, c664101 → e4c101f: 196 lines — requestTimeouts removal (McpRequestTimeouts,
McpClientOptions/McpServerConfig fields, config.node.ts plumb, _buildCallOptions, 4 tests, both barrel exports),
ElicitationContext intersection + @deprecated signal, spread shim in the elicitation handler, tests asserting
receivedContext.signal and receivedContext.mcpReq.signal, warn/throw text drops the requestTimeouts pointer
✅ build · type-check · lint · format:check — clean
✅ unit tests → 4658/4658 (167 files)
✅ shim probe (live 2.0 server + InMemoryTransport, elicitation triggered from a tool): raw vendor context own
props = sessionId,mcpReq,http, prototype = Object.prototype, 0 getters/symbols → spread loses nothing;
shimmed context adds signal, identity preserved, callback result round-trips to the server
✅ tasksConfig path unchanged from c664101: warn at construction, listTools works, callTool throws with #1659
⚪ integ suites not run here (need AWS credentials)
Process note: delta is a removal plus a shim I had proposed, so I verified it directly (interdiff, gates, runtime probe) rather than fanning out passes. One forward-looking caveat on the shim, not a finding: it's lossless because the vendor context is a plain object today; if a future @modelcontextprotocol/client makes it a class instance, the spread would drop prototype members silently — an owned interface would be immune, but that's a later refactor, not a blocker.
This branch has not been deployed
3 participants
Description
MCP revision 2026-07-28 replaces the initialize handshake, sessions, and server-initiated requests. The legacy
@modelcontextprotocol/sdkpackage supports protocol versions only up to 2025-11-25, and support for the new revision will not be added to it. This PR swaps the MCP internals to@modelcontextprotocol/client2.0, which negotiates the revision on each connect: probeserver/discover, fall back to legacyinitialize. Servers on both revisions work from one code path. No compat layer is needed on our side, unlike Python's_compat; the official client handles both protocol versions internally.Part of #1659.
The two design decisions in this PR
Transports: no runtime adapter. The 2.0
Transportinterface only adds optional members over the legacy one, so transport instances built from the legacy package stay structurally assignable.McpTransportremains a type-level widening, and user-supplied legacy transports keep working unwrapped. The MCP integration suite drives the new client through legacy stdio and streamable HTTP transport instances to prove it.Tasks: temporarily disabled, pending the SEP-2663 rebuild. The 2.0 client ships the SEP-2663 wire types but no task runtime, and the legacy experimental tasks API is gone, so this PR has nothing to drive
tasksConfigwith.tasksConfigis not deprecated and keeps its name and shape. While it is set, the client warns at construction andcallToolthrows with a pointer to #1659.listToolsstill works, soloadServersconfigs keep connecting. Throwing a clear error was chosen over silently changing what the option means. PR #3661 restores task execution on both server generations by rebuilding it on the finalized SEP-2663 tasks extension, matching strands-py, which supports tasks on both mcp lines since #4125. The intent is to land #3661 in the same release as this PR sotasksConfigusers never see the throw. Until it lands, tool calls run under the MCP client's default 60-second inactivity timeout, the same default that applied on main withouttasksConfig. The task-only integration suites are skipped citing #1659.Public API Changes
Temporarily disabled:
tasksConfig(and theDEFAULT_TTL/DEFAULT_POLL_TIMEOUTstatics). The type and option remain exported, andtasksConfigis not deprecated. Setting it makescallToolthrow until the tasks rebuild lands (#1659, restored by #3661).Changed (compatible):
ElicitationContextis now the 2.0ClientContextplus the existing top-levelsignalfield. The 2.0 client surfaces the abort signal atcontext.mcpReq.signal, and the SDK mirrors it tocontext.signalso existing callbacks keep working.context.signalis deprecated in favor ofcontext.mcpReq.signal:Breaking Changes
@modelcontextprotocol/sdk→@modelcontextprotocol/client. Consumers who install the peer manually must switch packages.tasksConfigtemporarily performs no task-augmented execution.callToolthrows while it is set. Task execution returns with the SEP-2663 tasks rebuild in feat(mcp/ts): add SEP-2663 task support #3661 (tracked in [FEATURE] MCP Specification 2026-07-28 adoption (parent tracker) #1659), intended to land in the same release. WhiletasksConfigis disabled, itsttl/pollTimeoutvalues also stop shaping request timeouts, so affected tool calls fall back to the MCP client's default 60-second inactivity timeout.MCP error <code>:message prefix.ProtocolError.messagecarries the plain message.Related Issues
Part of #1659
Documentation PR
The MCP docs under
site/still show legacy-sdk transport imports. Those imports keep working against this client; a docs refresh follows as a separate PR.Type of Change
Breaking change
Testing
MCP integration suites against real in-process servers, including legacy stdio and streamable HTTP client transports driving the 2.0 client end to end (agent conversations, filtering, prefixing, error handling, elicitation)
Manual end-to-end scripts against
@modelcontextprotocol/server2.0 overInMemoryTransport:listTools/callToolround trips, andtasksConfigthrows with the tracker linkUnit tests pin the elicitation compatibility contract: the callback receives the abort signal at both
context.signalandcontext.mcpReq.signalI ran
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.