feat(mcp)!: support both MCP protocol revisions (2025-11-25 and 2026-07-28) - #86
feat(mcp)!: support both MCP protocol revisions (2025-11-25 and 2026-07-28)#86LukasParke wants to merge 41 commits into
Conversation
…-07-28 deprecations MCP protocol revision 2026-07-28 shipped today. This is the non-breaking groundwork; the migration itself is deliberately deferred. Why defer: no released SDK negotiates 2026-07-28 by default. Verified empirically against a local HTTP capture — @modelcontextprotocol/client@2.0.0 still sends the `initialize` handshake with protocolVersion "2025-11-25" and omits the Mcp-Method / Mcp-Name headers the new revision requires, and @modelcontextprotocol/core@2.0.0 does not export LATEST_PROTOCOL_VERSION at all (internal value is "2025-11-25"). Migrating today would restructure the dependency tree without changing a byte on the wire, while breaking published API. Changes: - Fix the self-reported client version: DEFAULT_CLIENT_INFO said '0.1.0' while the package is 0.0.1. The published 0.0.1 tarball ships this, so every server it connects to is told the wrong version. - Add tests/unit/mcp-connection.test.ts (9 cases) covering transport selection and the Streamable HTTP -> SSE fallback. Every existing unit test vi.mocks mcp-connection.js, so this path had no coverage; these fake the SDK transports instead so the real connect() runs. Verified the suite fails when the pinned-transport guard is broken. - Mark @deprecated, type-level only: SerializedMCPServer.sessionId (sessions removed, SEP-2567), CreateMCPToolsOptions.onElicitation (server-initiated elicitation removed for MRTR, SEP-2322), and MCPTransportKind 'sse' (HTTP+SSE deprecated, SEP-2596). - Document the negotiated revision and the full migration gap in the README. No runtime behavior changes and no breaking API changes. Co-Authored-By: Claude <noreply@anthropic.com>
![devin-ai-integration[bot]](https://cdn.github.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cortex-github-agent[bot]](https://cdn.github.com/u/139423088?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Summary
Non-breaking MCP groundwork: corrects the self-reported client version, adds the first real coverage for connect()'s transport selection and Streamable HTTP → SSE fallback, and adds @deprecated/README notes for surfaces that revision 2026-07-28 removes. Deferring the migration is well argued and the new tests are structured correctly (SDK transports faked, real connect() executed); the findings below are advisory, not blocking.
Findings (4)
🟡 minor · packages/mcp/src/mcp-connection.ts:21
The version fix is immediately re-broken by its own changeset: .changeset/mcp-2026-07-28-prep.md is a patch bump, so this ships as 0.0.2 while DEFAULT_CLIENT_INFO.version reads '0.0.1'. The "keep in sync" comment is unenforced — no test, typecheck, or lint rule catches divergence. Read the version from package.json or add a guard test asserting DEFAULT_CLIENT_INFO.version === pkg.version.
🟡 minor · packages/mcp/tests/unit/mcp-connection.test.ts:1-241
The 9 new cases never assert anything about clientInfo — neither the default value (the field this PR fixes) nor that options.clientInfo overrides it. The behavior being corrected is the one part of connect() left untested.
🟡 minor · packages/mcp/src/types.ts:98
@deprecated is applied to onElicitation (and SerializedMCPServer.sessionId, cache-types.ts:36) while they remain the only functional path under the pinned SDK and have no in-package replacement. This surfaces strikethrough/deprecation lint in consumers for correct usage; the explanatory prose conveys the same warning without that side effect.
🟡 minor · packages/mcp/src/mcp-connection.ts:150-160
Pre-existing, now newly covered: when the Streamable HTTP attempt fails, the half-initialized client/transport is discarded without close(), so any opened socket or abort controller leaks before the SSE fallback. The new fallback test asserts clientsCreated === 2 but nothing asserts the failed client was cleaned up — worth a follow-up assertion plus a close().catch(() => {}) in the catch block.
|
Migration tracked in DEV-738 (DevEx › Agent SDKs, Backlog). It carries the full gap analysis — removals with SEP numbers, the new required fields, error renumbering, affected surface in this package, and the published-package constraints. The explicit trigger to start that work is a released SDK that negotiates |
Replaces the hardcoded '0.0.1' in DEFAULT_CLIENT_INFO with a constant
generated from package.json, so the version we self-report to every MCP
server cannot drift from the package we actually publish.
package.json is the source of truth. `build` runs gen-version.mjs before
tsc, so a changesets version bump is picked up automatically before
publish (the release workflow runs `pnpm run build` ahead of
`changeset publish`).
src/version.ts is committed rather than gitignored: CI's lint, typecheck,
and unit-test jobs compile src without a build step, and turbo's
`dependsOn: ["^build"]` only builds upstream packages, so nothing would
regenerate it in those jobs. tests/unit/version.test.ts closes the gap by
failing when the committed constant drifts from package.json.
Verified:
- bump package.json to 0.1.0 without regenerating -> drift test fails
- run build -> file regenerates, test passes
- dist layout unchanged (esm/index.js, not esm/src/), all 6 export-map
paths resolve
- esm/version.js ships in the tarball; scripts/ does not
- 571 tests pass, lint and typecheck clean
Note: importing package.json directly was tried and rejected — it pulls
the file into the compilation, shifting the implicit rootDir so output
becomes esm/src/**, which invalidates every path in the export map.
module: "Node16" also rejects JSON import attributes.
Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Summary
Update replaces the hardcoded client version with generated src/version.ts + a drift-guard test, which resolves my main prior finding and does it properly (committed output, documented rationale, turbo inputs updated for scripts/**). One new wrinkle: the changesets "Version Packages" PR will fail version.test.ts because CI's test task only builds upstream deps, so nothing regenerates the file after the bump. Remaining items are the same advisory ones as before; nothing blocking.
Findings (4)
🟡 minor · packages/mcp/scripts/gen-version.mjs:14
NEW: changeset version bumps package.json without regenerating src/version.ts, and CI's pnpm run test does not run mcp's own build (turbo test uses dependsOn: ["^build"] = upstream only, per turbo.json:26). The auto-generated Version Packages PR will therefore fail version.test.ts every release until someone regenerates by hand. Fix at the source: version: pnpm exec changeset version && pnpm --filter @openrouter/mcp gen:version in .github/workflows/publish.yaml, so the regenerated file lands in the changesets commit. The published tarball is unaffected (publish.yaml runs pnpm run build first).
🟡 minor · packages/mcp/tests/unit/mcp-connection.test.ts:96-125
PARTIALLY RESOLVED / still open: version.test.ts now guards constant-vs-package.json drift, but no test asserts connect() actually passes DEFAULT_CLIENT_INFO to new Client(...), nor that options.clientInfo overrides it. The Client fake still discards its constructor argument, so a regression in the wiring (rather than in the constant) would go unnoticed.
🟡 minor · packages/mcp/src/types.ts:98
OPEN (unchanged): @deprecated on onElicitation, and on SerializedMCPServer.sessionId (cache/cache-types.ts:36-45), still marks the only functional path under the pinned SDK with no in-package migration target — strikethrough + downstream deprecation lint for correct usage. See my earlier thread for the suggested prose-only alternative.
🟡 minor · packages/mcp/src/mcp-connection.ts:150-165
OPEN (unchanged, pre-existing): the failed Streamable HTTP client/transport is discarded without close() before the SSE fallback, leaking any opened socket/abort controller; the new fallback test asserts clientsCreated === 2 but not cleanup.
Replaces @modelcontextprotocol/sdk@^1.29.0 with @modelcontextprotocol/client@^2.0.0. Every item here is compile-forced — the package does not build without all of them — so they land together. - All 8 source import sites plus 4 test specifiers collapse to the single '@modelcontextprotocol/client' package. Notably we do NOT add @modelcontextprotocol/core as a direct dependency: its '.' export is a zod-schema barrel (173 exports, all /Schema/), every type and value we use lives in `client`, and core arrives as a pinned transitive dep. - callTool loses its middle argument: v2 is `callTool(params, options)`. This is the one change with runtime rather than compile-time consequences — leaving the v1 three-arg form would have put `signal` and `onprogress` in a dropped slot, silently killing cancellation and progress streaming. Verified against v2's types: `TS2554: Expected 1-2 arguments, but got 3`. - setRequestHandler / setNotificationHandler are method-name-first in v2. Spec methods supply their own schema; passing a bare zod schema as the second argument crashes at runtime reading '~standard'. The elicitation handler is unchanged otherwise and now serves both protocol eras — on 2026-07-28 the multi-round-trip driver dispatches input_required through this same handler. - Deletes the isTransport runtime guard. It existed solely because SDK v1 typed `sessionId` as `string | undefined` rather than optional, which exactOptionalPropertyTypes rejected at the connect() call site. v2 declares it optional, so client.connect() typechecks directly and connectWith() inlines into its three call sites. - Collapses the three v1 module mocks in mcp-connection.test.ts into one factory on the unified package, and records versionNegotiation.mode per constructed Client so a later commit can assert the negotiation default with no network. customConditions: [] stays — eventsource and eventsource-parser still ship exports.source pointing at raw .ts, which is the original reason. Verified: typecheck clean, 50/50 unit tests pass, real (unmocked) build succeeds. No behavior change intended in this commit. Co-Authored-By: Claude <noreply@anthropic.com>
Two new suites, no source changes — so they characterize the SDK's
current behavior (versionNegotiation defaults to 'legacy') before the
next commit opts us into 'auto'.
tests/unit/protocol-era.test.ts (9 tests) runs a hand-rolled MCP server
over InMemoryTransport, so there is no network, no fixture process, and
no MCP_TEST_URL gate. It pins the facts the rest of the package depends
on:
- legacy server: server/discover probe, then initialize fallback,
getProtocolEra() === 'legacy'
- modern server: NO initialize at all, getProtocolEra() === 'modern'
- modern server still populates getServerVersion() and
getServerCapabilities() — handle.ts reads both synchronously, so if
the modern era left them empty, resource tools would silently vanish
and snapshots would lose serverInfo
- sessionId is undefined in the modern era (SEP-2567)
- input_required is fulfilled through the SAME registered
elicitation/create handler, then the call is retried (SEP-2322) —
this is what justifies keeping onElicitation rather than deprecating
it
- { pin } fails loudly when the revision is not offered
Two field names worth recording, both of which I got wrong first: the
server/discover result field is `supportedVersions` (not
`protocolVersions`), and `inputRequests` is an object keyed by request id
(not an array).
tests/unit/call-tool-shape.test.ts (3 tests) guards the v2 callTool
signature. That regression is silent rather than loud: with the v1
three-arg form, `signal` and `onprogress` land in a slot the SDK does not
read, so cancellation and progress stop working while every other test
still passes. Verified by mutation — restoring the three-arg call fails
all 3.
Both suites mutation-tested: dropping modern advertisement fails 4 era
tests; the MRTR handler-invocation assertion is load-bearing.
62/62 unit tests pass, typecheck and lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
Any MCP server now works out of the box, whether it speaks 2025-11-25 or
2026-07-28. Previously we only spoke the 2025 handshake.
Adds `protocolNegotiation?: 'legacy' | 'auto' | { pin: string }` to
CreateMCPToolsOptions and RehydrateMCPToolsOptions, mapped onto the SDK's
versionNegotiation. Defaults to 'auto' — the SDK itself defaults to
'legacy', but a library whose job is "point it at a server" should reach
both eras with no configuration.
Under 'auto' the client probes with server/discover, then either goes
modern (per-request _meta envelope, no handshake) or falls back to the
2025 initialize handshake. `'legacy'` skips the probe, which matters on
flaky servers: on HTTP a probe timeout is treated as an outage and
rejects, where 'legacy' may still connect. `{ pin }` fails loudly rather
than falling back.
The type is declared in transport-types.ts rather than re-exported from
the SDK, so this does not put an SDK type in our public API — the problem
MCPAuth already has with OAuthClientProvider.
Threaded through every path that reaches connect(), including
FORWARDED_REHYDRATE_KEYS. Omitting it there would have made a cache HIT
silently fall back to the default while a cache MISS honoured the caller
— the same bug class as the pre-existing staleness gap.
`inputRequired` is deliberately left unset: the SDK's defaults
(auto-fulfil on, 10 rounds) are what we want, and pinning them would
freeze values the SDK may tune.
4 new tests assert the 'auto' default, explicit 'legacy', pin
pass-through, and that the policy also applies to the SSE fallback
client. Mutation-verified: reverting the default to 'legacy' fails the
default test.
66/66 unit tests pass, typecheck and lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
Pre-existing gap, unrelated to the protocol migration — separated out so it doesn't read as migration fallout. `staleness.maxAgeMs` was only checked in `createMCPTools`'s cache-hit path (create-mcp-tools.ts). A caller holding their own snapshot and calling `rehydrateMCPTools()` directly got no staleness check at all — `rehydrate.ts` never read `cachedAt` — so tools of unbounded age were replayed silently. Adds `snapshotIsStale()` beside the existing `tokensExpired()` and folds it into the same guard, so a stale snapshot routes through `freshConnect` exactly like expired tokens or missing credentials already do. Also adds `staleness` to `RehydrateMCPToolsOptions` and to FORWARDED_REHYDRATE_KEYS. `toCreateOptions` deliberately does NOT forward it: that builds options for a fresh connect, which has no snapshot age to compare against. 3 tests cover within-maxAge replay, over-maxAge re-list, and no-maxAge replay-regardless-of-age. Mutation-verified: dropping the check fails the over-maxAge test. The fake client in rehydrate.test.ts gained a `listTools` stub, which is now reachable because a stale snapshot falls through to freshConnect. 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com>
PR #86 originally annotated three surfaces as doomed under 2026-07-28, on the premise that we could not speak that revision. Now that we negotiate both, one of those annotations was simply wrong and the other two needed re-tensing. - REMOVE the @deprecated on `onElicitation`. It works on both revisions: 2025-era servers send `elicitation/create`, and on 2026-07-28 the SDK's multi-round-trip driver routes `input_required` through the same handler. Replaced with an explanation of the dual mechanism. - KEEP `transport: 'sse'` deprecated — SEP-2596 stands regardless. - RE-TENSE the `sessionId` notes from "will be removed" to "is undefined on modern connections", and add matching notes on ConnectOptions and MCPConnection, which were newly no-ops against 2026-07-28 servers and carried no annotation at all. - ADD `MCPOAuthClientProvider`, a re-export of the SDK's OAuthClientProvider under our own name. That type is reachable through the public `MCPAuth` oauth variant, so consumers were importing it from the SDK directly — an import path that just changed under them. Now they can name it without depending on our dependency. - REWRITE the README protocol section. Its central claim — "no released SDK negotiates it by default ... migrating today would change our dependency tree without changing a single byte on the wire" — was the stated reason for not migrating, and is now false. Replaced with what actually happens on the wire per revision, how to override, and the three real behavioral differences. 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com>
Bumps minor rather than patch: this release breaks the public API (OAuth provider type) and pre-1.0 minors are the conventional vehicle. A patch would land 0.0.2, indistinguishable from the queued bugfix. Deletes .changeset/mcp-2026-07-28-prep.md and folds its still-true content into the new changeset. It asserted "no released SDK speaks 2026-07-28 by default yet" and claimed the onElicitation deprecation — both false in this same release, and shipping three release notes that contradict each other would be worse than one coherent one. Co-Authored-By: Claude <noreply@anthropic.com>
LukasParke
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The structural gate failed with "Complex functions increased: 9 -> 10". The culprit was `toCreateOptions` at cc=16, one over the max_cc=15 in .sentrux/rules.toml: it had grown to 14 conditional spreads, two of them added by this PR (protocolNegotiation, and staleness reaching the options surface). Replaces the per-key spreads with a typed key list and one copy loop, mirroring `forwardedRehydrateOptions` in create-mcp-tools.ts which already forwards the same set in the opposite direction. `as const satisfies readonly (keyof RehydrateMCPToolsOptions & keyof CreateMCPToolsOptions)[]` means a key that isn't valid on both types fails to compile rather than silently dropping — the same guard the sibling helper uses. `staleness` is deliberately excluded: it compares a snapshot's age, and this builds options for the fresh-connect fallback which has no snapshot. Verified locally with sentrux 0.5.7 (same version CI pins): complex functions back to 9, and the 9 remaining are all pre-existing in packages/agent, untouched here. Behavior unchanged — 69/69 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Summary
The PR has been rewritten from deprecation prep into a breaking SDK-v2 migration that supports both protocol revisions, with the title, body and changeset now honestly reflecting that scope. The engineering is largely solid — the callTool arity catch and the dual-era InMemoryTransport suite are real value — but two things block: the new 'auto' default can break connections that work today and isn't listed as breaking, and our own client wiring (notably the string-keyed tools/list_changed handler) is not exercised against the real SDK anywhere.
Findings (7)
🟠 major · packages/mcp/src/mcp-connection.ts:110
NEW: defaulting versionNegotiation.mode to 'auto' flips behavior for every existing consumer, and by this PR's own docs (transport-types.ts:16-18) a probe timeout over HTTP "is treated as an outage and rejects, where 'legacy' may still connect" — the SSE fallback re-probes and fails identically (mcp-connection.ts:177-190). Servers/proxies that hang or 5xx on an unknown method regress from working to failing. The changeset lists only the OAuth type change as breaking. Either retry once with 'legacy' on probe failure, default to 'legacy' for a release, or document this as a second breaking change with the escape hatch named.
🟠 major · packages/mcp/src/mcp-connection.ts:127
NEW: setNotificationHandler('notifications/tools/list_changed', …) replaced a compile-checked SDK schema value with a bare string, and nothing verifies it. tests/unit/mcp-connection.test.ts:96 mocks the entire @modelcontextprotocol/client module (handlers are no-ops), and protocol-era.test.ts constructs its own Client instead of calling connect(). A wrong key leaves default-on autoRefreshOnListChanged silently dead with every test green — the same silent-failure class as the callTool arity bug, without a guard. Needs one InMemoryTransport test through our own connect()/makeClient, or an assertion tying the literal to the SDK's exported name.
🟡 minor · packages/mcp/scripts/gen-version.mjs:14
STILL OPEN (raised on the previous head): changeset version bumps package.json without regenerating src/version.ts, and CI's test task is dependsOn: ["^build"] (upstream only, turbo.json:26), so the auto-created Version Packages PR fails version.test.ts — now bumping 0.0.1 → 0.1.0, so it will fail on this release. Fix by appending && pnpm --filter @openrouter/mcp gen:version to the version: command in .github/workflows/publish.yaml.
🟡 minor · packages/mcp/tests/unit/mcp-connection.test.ts:265-306
STILL OPEN: the negotiation suite now records versionNegotiation.mode per client, but the fake Client still discards its first constructor argument, so nothing asserts DEFAULT_CLIENT_INFO reaches the SDK or that options.clientInfo overrides it — the behavior the generated version.ts exists to protect.
3 more finding(s)
🟡 minor · packages/mcp/src/mcp-connection.ts:177-190
STILL OPEN (pre-existing, now sharper): the failed Streamable HTTP client/transport is discarded without close() before the SSE fallback, leaking any socket or in-flight probe request; under 'auto' there is now an extra probe round trip in that window.
🟡 minor · packages/mcp/src/transport-types.ts:29-31
MCPProtocolNegotiation's { pin: string } accepts any string, so { pin: '2026-07-08' } typechecks and fails only at connect time. A literal union with a (string & {}) escape hatch would catch typos at compile time for the two known revisions.
🟡 minor · packages/mcp/tests/unit/protocol-era.test.ts:1-379
This suite characterizes the third-party SDK against a hand-rolled server, so the fake defines the contract it verifies: if v2 changes server/discover's shape, the tests keep passing while real servers diverge. Valuable as documentation of the negotiation model, but it exercises no src/ code — it should not be counted as coverage of this package's dual-era behavior.
The Version PR that changesets opens would have failed its own CI. Chain, all three links verified: - packages/mcp/src/version.ts is generated but committed (deliberately — see the docblock in scripts/gen-version.mjs: lint/typecheck/test run without a build step). - tests/unit/version.test.ts asserts PACKAGE_VERSION === package.json's version. - `changeset version` bumps package.json and commits, regenerating nothing. turbo's `test` task is `dependsOn: ["^build"]` — `^` is upstream deps only, so mcp's own build (which runs gen:version) never runs before its tests. Reproduced by bumping packages/mcp/package.json to 0.0.2 and running `turbo run test --filter=@openrouter/mcp`: AssertionError: expected '0.0.1' to be '0.0.2' Fixes it at the source rather than loosening the test, which is the part that actually catches drift. Adds a root `version` script that runs `changeset version`, then `turbo run gen:version`, then refreshes the lockfile — and points changesets/action at it. Same bump, same commit, generated file included. Also adds the `gen:version` turbo task (uncached; package.json in, src/version.ts out) so any future package with committed generated output is picked up automatically. Verified: same bump with the fix applied regenerates version.ts to 0.0.2 and the mcp suite passes 69/69. Reported independently by devin (x2) and cortex on #86.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Summary
This update only touches the release plumbing — root version script, publish.yaml, and a turbo gen:version task — which cleanly resolves the Version-PR breakage I raised. Both blocking findings from the previous review are untouched: the 'auto' negotiation default can still fail where today's behavior succeeds and isn't listed as breaking, and the string-keyed notifications/tools/list_changed registration is still verified nowhere.
Findings (7)
🟠 major · packages/mcp/src/mcp-connection.ts:110
UNCHANGED / STILL OPEN: versionNegotiation.mode still defaults to 'auto', so every existing consumer starts probing with server/discover; per this PR's own docs a probe timeout over HTTP rejects where 'legacy' would have connected, and the SSE fallback re-probes identically (mcp-connection.ts:177-190). Still absent from the changeset's Breaking section. Fix per my inline thread: retry once with 'legacy' on probe failure, or default to 'legacy', or document it as breaking with the escape hatch named.
🟠 major · packages/mcp/src/mcp-connection.ts:127
UNCHANGED / STILL OPEN: setNotificationHandler('notifications/tools/list_changed', …) replaced a compile-checked SDK schema with a bare string that no test exercises — tests/unit/mcp-connection.test.ts:96 stubs the handler as a no-op and protocol-era.test.ts never calls our connect(). Default-on autoRefreshOnListChanged can be silently dead with the whole suite green.
🟡 minor · package.json:15
NEW: the version script appends pnpm install --lockfile-only, which re-resolves all ranges during changeset version. Since the bump changes no dependency ranges (internal deps are workspace:*), its only effect is to smuggle unrelated transitive resolution bumps into the auto-generated release commit. Drop it, or bump the lockfile in a separate reviewable PR.
🟡 minor · packages/mcp/tests/unit/mcp-connection.test.ts:265-306
STILL OPEN: the fake Client records versionNegotiation.mode but discards its first constructor argument, so nothing asserts DEFAULT_CLIENT_INFO reaches the SDK or that options.clientInfo overrides it — the behavior the new generated version.ts exists to protect.
3 more finding(s)
🟡 minor · packages/mcp/src/mcp-connection.ts:177-190
STILL OPEN (pre-existing): the failed Streamable HTTP client/transport is discarded without close() before the SSE fallback; under 'auto' that window now also contains an in-flight probe request.
🟡 minor · packages/mcp/src/transport-types.ts:29-31
STILL OPEN: { pin: string } accepts any string, so a mistyped revision typechecks and fails only at connect time.
⚪ nit · turbo.json:1-88
The file was reformatted to one array element per line, turning a 2-line functional change (gen:version task) into a ~50-line diff in a shared root config that pnpm lint does not cover. Worth reverting the cosmetic churn so the release-plumbing change reads on its own.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Re-review at
|
| tree | baseline coupling | measured | delta | gate |
|---|---|---|---|---|
origin/main |
0.4286 | 0.47 | +0.043 | ✓ passes (just under 0.05) |
| this PR | 0.4286 | 0.4837 | +0.055 | ✗ DEGRADED |
The baseline on both trees is the same stale file (133 import edges, recorded when the repo was less than half its current size). Main has already drifted +0.043 against it; this PR adds ~+0.005 more and tips it over.
That +0.005 is also not new coupling. Holding non-source files constant, base source measures 151 cross-module edges over 309 import edges (0.4790) and head measures 151 over 306 (0.4837) — the numerator is unchanged and the ratio moves because consolidating six @modelcontextprotocol/sdk/* subpath imports into one @modelcontextprotocol/client import removed three resolved edges. A denominator effect. The PR also takes cycles 1 → 0 and quality 5094 → 6088.
So the gate is firing on accumulated main drift with this PR's rounding error on top. The mechanism sentrux offers is sentrux gate --save (no delta allowance, no per-rule suppression, no baseline subcommand); precedent for using it is #73's fcbf9aa. Cleanest sequencing, and the human call I'd ask for:
- refresh the baseline on
mainin its own one-file PR, which retires the +0.043 that main has already accumulated and is reviewable as exactly that; - then this PR's own +0.005 passes on its merits and the gate keeps its regression-detecting power here.
Refreshing on this branch instead also turns the check green, but folds main's drift into an MCP PR where nobody is looking for it.
Verdict
The release fix is right. Blocking on the 'auto' default being documented as breaking (or fallback-retried); the notification-handler finding is refuted and shouldn't hold the PR. structural-gate needs the baseline decision above, not a code change.
…ample Two findings from devin's re-review of e4c0273. 1. `protocolNegotiation` defaulting to 'auto' where the SDK defaults to 'legacy' was documented only in a code comment at mcp-connection.ts. The changeset's Breaking section listed just the OAuth type change, so a consumer reading the CHANGELOG and not the source learned nothing about the riskier change. Now a second Breaking bullet names the failure mode (probe against a gateway that rejects unknown methods goes from working to failing) and the escape hatch. 2. `.agents/skills/public-api-examples/SKILL.md` requires a fenced example in the changeset for any public-API change. This adds `protocolNegotiation` and exports `MCPOAuthClientProvider`, and was prose-only. Shapes verified against source: protocolNegotiation at types.ts:51, `{ pin: string }` at transport-types.ts:31, MCPOAuthClientProvider exported from index.ts:4. Does not resolve whether 'auto' should stay the default — devin proposes a one-shot 'legacy' retry on connect failure, which is a design call for a human. This only stops the change being undocumented.
…o.json Both from review of e4c0273. - `pnpm install --lockfile-only` removed from the `version` script. cortex and devin both flagged it: `changeset version` only rewrites `version` fields and CHANGELOGs, and all internal deps are `workspace:*`, so there is nothing for it to pick up. It is a no-op in the expected case, and its one real effect — an unreviewed lockfile diff inside "chore: version packages" — has no upside. - turbo.json restored to the file's own compact style. My edit had gone through json.dumps(indent=2), expanding every array to one element per line and turning a 5-line addition into a 61-line diff. Nothing formats root turbo.json (`lint` is `turbo run lint`, which only covers packages/*/src and packages/*/tests), so the reformat was gratuitous. The semantic diff against main is now exactly two entries: the new `gen:version` task, and `scripts/**` added to `build.inputs` so a change to gen-version.mjs invalidates the build. Verified by comparing parsed JSON rather than the textual diff — the first pass at un-reformatting silently dropped `scripts/**`. Release fix re-verified after the edits: bump to 0.0.2 → gen:version regenerates → mcp suite 69/69.
There was a problem hiding this comment.
Summary
This update closes the 'auto' default finding the way I said would be acceptable — a second explicit Breaking bullet in the changeset naming the failure mode (gateways that hang or 5xx on unknown methods) and protocolNegotiation: 'legacy' as the exact opt-out — and drops the lockfile side effect plus the turbo.json reformatting churn. One blocking item is left untouched: the notifications/tools/list_changed handler key is verified by no test, so default-on auto-refresh can be silently dead.
Findings (4)
🟠 major · packages/mcp/src/mcp-connection.ts:127
STILL OPEN, now the only blocker: setNotificationHandler('notifications/tools/list_changed', …) replaced a compile-checked SDK schema value with a bare string that nothing exercises — tests/unit/mcp-connection.test.ts:96 stubs it as a no-op and protocol-era.test.ts drives its own Client, never our connect(). If the key or the dispatch path is wrong, default-on autoRefreshOnListChanged fails silently with all 590 tests green, the same class of bug as the callTool arity fix this PR guards. Narrowed ask: expose a seam (export makeClient, or let connect() accept a transport) and add one InMemoryTransport case where the fake server emits the notification and the setToolListChangedHandler callback fires. If v2 types the method parameter as a literal union, say so — mutating the string and showing typecheck fails would satisfy the key half, but the dispatch half still needs the test.
🟡 minor · packages/mcp/tests/unit/mcp-connection.test.ts:265-306
STILL OPEN: the fake Client records versionNegotiation.mode but discards _info, so nothing asserts DEFAULT_CLIENT_INFO reaches the SDK or that options.clientInfo overrides it — the behavior the generated version.ts exists to protect.
🟡 minor · packages/mcp/src/mcp-connection.ts:177-190
STILL OPEN (pre-existing): the failed Streamable HTTP client/transport is discarded without close() before the SSE fallback; under the new 'auto' default that window also holds an in-flight probe.
🟡 minor · packages/mcp/src/transport-types.ts:29-31
STILL OPEN: { pin: string } accepts any string, so a mistyped revision typechecks and only fails at connect time. A literal union with a (string & {}) escape hatch would catch the two known revisions' typos at compile time.
Converged — ready for your review at
|
| Bug | Why it mattered |
|---|---|
| Failed-connect transport leak | SDK doesn't close a transport whose start() threw; 'auto' makes that path common |
| Replay connection leak | Same class, in rehydrate.ts |
Staleness ignored under reconnectOnExpiry: false |
Served unbounded-age tools silently |
refresh() returned cached tools |
v2's response cache made a documented "forced re-read" a lie, up to 24h |
list_resources returned cached listings |
A model's own write looked like it had failed |
Replayed sessionId dropped resource tools |
SDK skips negotiation entirely, leaving capabilities undefined without erroring |
sessionId persisted to cache stores |
Bearer-equivalent credential written for no functionality |
| Sync-throw in teardown masked real errors | And skipped the freshConnect fallback that used to self-heal |
structural-gate failure |
My own refactor pushed rehydrateMCPTools to cc=17 |
Tests 590 → 619, every fix mutation-verified — I reverted each one to confirm its test genuinely fails. Three findings I pushed back on with evidence rather than changing code (the DELETE-on-close concern, read_resource's TTL, keeping SerializedMCPServer.sessionId on the type for old snapshots).
I was wrong twice and reversed myself: I'd argued the persisted sessionId was "inert data" (it's a credential — Devin's framing was right), and I fixed the sync-throw hazard in one file while leaving three other call sites on the old pattern.
Merge decision is yours — I have not merged
The branch is 11 commits behind main but conflict-free, and CI gates the merge ref, so what's green is the merged result.
…ship 1.0.0 Both open decisions resolved by @LukasParke: major release, and option 2 for the `'auto'` default. `'auto'` is now strictly additive. When the caller leaves `protocolNegotiation` unset, a failed connect is retried once with `'legacy'`, so a proxy, WAF, or gateway that hangs or 5xx's on an unknown method connects exactly as it did before this package started probing. Modern servers still get 2026-07-28. The cost is one extra attempt on a path that was already failing, which is where latency matters least. That also closes the pinned-SSE finding without separate work: all three `makeClient` sites — pinned SSE, Streamable HTTP, and the SSE fallback — sit under the retry, so the two-transport fallback stops being a single point of failure against probe-hostile infrastructure. The retry fires on any failure rather than only on probe-shaped errors. Inspecting the cause would save a wasted attempt against a genuinely dead server, but it would couple this to SDK error codes, and a reshaped error would silently disable the degradation — the exact failure mode this review kept surfacing. An explicit `protocolNegotiation` is honoured verbatim, including `'auto'`: naming a mode means accepting its failures, and silently overriding a `{ pin }` would defeat pinning. So `'legacy'` becomes a performance choice rather than a compatibility one, and the docs now say so. `connect()` splits into a public wrapper owning the retry policy and `connectWithNegotiation` performing exactly one mode, which also keeps it under the complexity ceiling. Seven tests, mutation-verified both directions: removing the retry fails 4, and removing the explicit-mode guard fails 6 transport tests that would otherwise silently double their attempt counts. The `probeHostile` fake rejects any probing mode and accepts `'legacy'`, so a test cannot pass unless the retry really switched modes. Changeset is `major` — `1.0.0` (verified: semver.inc('0.0.1','major')). The `'auto'` Breaking note is gone, since it is no longer a break; the OAuth provider change remains the one breaking item.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin caught two real problems with the degradation I added in 7fc4ce5, and my own test was the evidence for the first. **Attempt amplification.** `connectWithNegotiation` already walks Streamable HTTP then SSE when no transport is pinned, so retrying the whole thing meant up to four `connect()` calls against an unreachable server where it used to be two — each with its own request timeout, enough to push a caller past its own deadline. `expect(state.clientsCreated).toBe(4)` was sitting right there asserting the amplification I had just documented as "one extra attempt". The retry now pins the caller's transport preference, so it is a single attempt: a probe-hostile server takes two attempts, a dead one three. **Replayed side effects.** Retrying on any failure meant an `UnauthorizedError` was retried too — re-driving an OAuth provider's authorization flow, so a second `redirectToAuthorization` and a second PKCE verifier overwriting the first. Credentials being rejected is not something a different protocol revision fixes, so the retry is now skipped there. This is the one place the error's identity is inspected; the retry still fires on any other failure rather than trying to recognise probe errors, since that would couple us to SDK error codes. `isAuthFailure` walks the `cause` chain, because `connectWithNegotiation` wraps transport errors — a top-level `instanceof` would miss the nested case and retry anyway. Depth-capped so a cyclic cause cannot hang. Corrected the counts in the changeset and README, which both understated the cost. Four tests, mutation-verified: removing the transport pin fails two, removing the auth skip fails one, and one asserts the nesting that makes the chain walk necessary.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…y attempt Two more from Devin on the bounded retry — the second is a regression I introduced one commit ago while fixing the first review round. **The transport pin broke SSE-only legacy servers.** I pinned Streamable HTTP to cap the attempt count. But a legacy server reachable only over SSE, behind probe-hostile infrastructure, fails both transports under `'auto'` — and with the retry pinned to HTTP it never gets offered SSE again. A server that connected before this PR stopped connecting, which is precisely the regression the degradation exists to prevent. The retry re-walks the ladder again. That puts a genuinely dead server back at four dials, two per mode. I had traded correctness for an attempt count without noticing, so the count is now asserted as a deliberate decision instead: what matters is that it is a fixed multiple, not a retry loop. **The auth guard only saw the last failure.** On the two-transport path `connectWithNegotiation` discarded `httpErr` entirely — the fallback wrapped only `sseErr`. So an `UnauthorizedError` from Streamable HTTP followed by an unrelated SSE failure (the same URL answering 404 to an SSE GET, never reaching the auth path) left no auth error in the chain, and the retry re-drove the OAuth flow anyway. Exactly the side effect the guard was written to prevent, one commit after writing it. `MCPConnectionError` now carries every underlying failure on `errors`, named after `AggregateError` rather than inventing a field, and `isAuthFailure` searches that tree instead of the `cause` spine. Corrected the attempt counts in the changeset and README — the "three attempts" claim I wrote last commit was wrong the moment the pin came off — and added the compilable example the repo's public-API rule requires for the new `errors` field. Mutation-verified: re-pinning the transport fails 3 tests, and narrowing `isAuthFailure` back to the cause spine fails the cross-transport one.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…401/403
Two more from Devin, both on the retry I added two commits ago.
**`errors` was a partial record.** When the legacy retry also failed, its
rejection propagated untouched and the `'auto'` pass's failures vanished — half
the attempts gone, including any auth-shaped rejection `isAuthFailure` had not
matched. That directly contradicted the contract I had just written on
`MCPConnectionError.errors` ("every failure behind this one, in attempt
order"), so the docs promised a complete record while the code delivered half
of one. `connect()` now wraps a failed retry and concatenates both passes,
flattening nested `errors` so callers iterate a list of real attempts rather
than a tree of wrappers.
**The auth guard only recognised `UnauthorizedError`.** Devin asked me to check
the SDK's taxonomy, and the answer is that it is not one type: the
version-negotiation probe does not route 401/403 through the OAuth flow —
`classifyHttpError` turns them into `SdkHttpError` with
`ClientHttpAuthentication` / `ClientHttpForbidden`. So a probe rejected for auth
reasons was retried, re-driving the flow the guard exists to prevent, on the
most common auth path of all.
Now also matches a duck-typed `status` of 401 or 403. Reading the numeric
status rather than `instanceof SdkHttpError` plus an `SdkErrorCode` comparison:
the status is the stable half of that contract, and it additionally catches a
gateway surfacing 401/403 in some other error shape.
Three tests, each mutation-verified: dropping the aggregation fails the
attempt-count one; removing the status check fails the 403 one; widening the
status check to any number fails the 404 one, which exists so the guard cannot
over-match and silently disable the degradation.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ateError Three from Devin's review of fb89f92. **The probe could hang for minutes.** `makeClient` left `probe.timeoutMs` unset, so the SDK gave the probe the full 60s request timeout. Under `'auto'` the probe is the first request of every connection, and with the legacy retry re-walking the ladder that is four attempts — ~4 minutes before `createMCPTools()` rejects, on the path a caller gets with no configuration. Now bounded at 5s, with `probeTimeoutMs` to raise it for a slow server. A probe that misses the window isn't lost: it surfaces as a failure the legacy retry handles, the same path a refusal takes. This is the option I declined two rounds ago as premature pending the `'auto'` decision. That decision is made, and Devin's arithmetic showed the exposure is worse than I'd assumed — so it lands now. **`errors` could contain wrappers.** `flattenAttempts` only unwrapped when `errors` was non-empty, but a single-transport pass wraps its one failure with only `cause` set. On the pinned-HTTP path the aggregate became two opaque `MCPConnectionError`s, so a caller scanning for a rejected token had to know to dig through `cause` on some entries and not others — and the field's own doc promised real attempts. **`isAuthFailure` ignored `AggregateError` members.** Node's happy-eyeballs path reports a 401 as an `AggregateError` member rather than a `cause`, so a spine-only walk missed it and re-drove the OAuth flow. Now reads any array `errors`, which covers ours and `AggregateError` alike. All three mutation-verified. The unwrap test initially passed against its own mutation because I wrote it on the pinned-SSE path, which rethrows raw and never had the bug; retargeted to pinned HTTP, where it fails without the fix.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two from Devin on 57f29e9. The first is a correctness bug I introduced one commit ago, and my own justifying comment was the wrong reasoning. **5s could make modern-only servers unreachable.** I argued the tight ceiling was safe because "a probe that misses the window is not lost: the legacy retry handles it". That holds for 2025-era servers only. On HTTP the SDK classifies a probe timeout as an outage and rejects (`classifyProbeOutcome` returns a legacy verdict for stdio alone), and the legacy retry then sends `initialize` — removed in revision 2026-07-28 (SEP-2575). So a modern-only server slower than the ceiling fails *both* passes and does not connect, where the SDK's own default would have waited and succeeded. Serverless cold starts routinely exceed a few seconds, which makes that a correctness bug rather than the latency tradeoff I described. 30s instead: comfortably past a cold start, and still caps the black-holed-gateway case at roughly half the SDK default rather than four times it. The JSDoc now states that a probe timeout is unrecoverable instead of claiming the opposite. **Pinned SSE rethrew raw while every other path wrapped.** Pre-existing, but the new `connect()` wrapper made it asymmetric: with `protocolNegotiation` unset the outer retry aggregated it into an `MCPConnectionError`; with it set the transport error escaped. Same server, same failure, different `catch`. Now wrapped like everything else. Also fixed a test that had started taking 10s in real time: it asserted the probe bound via `connect`'s `timeout`, which our explicit `probe.timeoutMs` now overrides, so it was waiting out the real default. Uses `probeTimeoutMs`. Both mutation-verified: restoring 5s fails the ceiling test, restoring the raw rethrow fails the wrapping test.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tatus Two from Devin on 961bf65. **Three JSDoc sites still said 5000** after I raised the constant to 30_000 — types.ts, rehydrate.ts, and mcp-connection.ts — while the README, changeset, and tests all said 30s. Anyone reading the IDE hover would have budgeted six times less than the client actually waits, which is the worst kind of doc bug: confidently wrong rather than absent. Fixed, and added a test that pins the JSDoc against the value the code passes, because a number duplicated across four files has now drifted once and will again. It greps the source for `defaults to <n>` near the probe option and compares against what the fake Client observed. Verified it catches the exact drift Devin found. **`isAuthStatus` now requires an `Error`.** Devin asked whether any SDK shape attaches a non-authoritative `status`. Audited: only `SdkHttpError` exposes one, and its value is the real response status — but the SDK does build log records with `status: 0`, and a plain object riding in a `cause` is likelier to be a response or payload than a rejection. Over-matching there would silently suppress the retry and make a probe-hostile-but-authenticated server unreachable, which is the regression the retry exists to prevent, so the guard is worth narrowing even though nothing in v2 trips it today. Both mutation-verified: reintroducing the 5000 doc fails the drift test, loosening the `Error` check fails the payload test.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e ladder Two from Devin on ea7474b. **A store outage failed the whole call.** `refresh()` re-lists and then writes back, so both steps surfaced identically — and the stale path converted either into `MCPStaleSnapshotError`, discarding a connection whose tools had just been read successfully and reporting it as a re-list failure. Worse, the recovery this PR documents rehydrates through the same store, so it failed too: the escape hatch was as broken as the thing it escaped. `writeCache` now tags failures as `MCPCacheWriteError` (subclassing `MCPCacheError`), the stale path treats that as survivable, and the construction-time write is best-effort. A handle is fully usable without its cache entry; the next rehydrate re-reads it. Callers who want a store outage to be fatal can catch the new type. **Auth failures fell through the transport ladder.** A 401 on Streamable HTTP tried SSE with the same `authProvider`, re-entering the SDK's auth path for a second `redirectToAuthorization` and overwriting the saved PKCE verifier. This is the same duplicated side effect `connect()`'s retry guard was written to prevent — I guarded the outer layer and left it intact one level down, then wrote a test asserting both transports are tried on an auth failure without noticing that was the hazard. Three tests repointed: they used a failing cache write as their post-connect failure trigger, which is no longer fatal. They now use a snapshot with duplicate tool names, which `buildTools` genuinely rejects. All three changes mutation-verified.
…t new API Findings from a full self-review of the final state, requested after 14 incremental review rounds. **The freshConnect fallback was a third unguarded reconnect layer.** The same duplicated-OAuth hazard was fixed tonight at two layers — connect()'s legacy retry skips auth failures, and the transport ladder short-circuits on them — but rehydrateMCPTools' catch still routed *any* replay failure into freshConnect with the same auth. A credential rejection on the replay therefore re-drove the OAuth flow one layer up from both guards. The fallback now consults the same isAuthFailure, exported (not via the package entrypoint) so all three layers share one definition and a future widening applies everywhere. **MCPCacheWriteError and probeTimeoutMs shipped without the examples the repo requires.** Same public-api-examples violation as MCPStaleSnapshotError earlier tonight — new export and new public option, present in prose, absent from every ts block in the changeset and from the PR body. Both now have compile-verified examples in both places. Reviewed and found sound, for the record: the connect flow's three layers compose without double-teardown (closeQuietly is idempotent and each layer closes only clients it created); flattenAttempts covers all four wrap shapes; probeTimeoutMs threads through create/rehydrate/replay/freshConnect identically to protocolNegotiation; the untouched modules (tool-wrapper, elicitation, auth-resolver, result-mapper) check out — the progress-pump generator in tool-wrapper cannot drop a trailing event because finalize flips `done` only after the queue drain re-runs.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin found the guard I widened two rounds ago cancelling the recovery in
exactly the scenario the recovery exists for. Proxies and WAFs commonly answer
an unknown method like `server/discover` with 403 — not just 5xx or a hang —
and `isAuthStatus` classified any 401/403 as a credential rejection. That
suppressed both the SSE fallback and the legacy retry, so a server behind such
a gateway failed outright where it connected before this PR.
The duplicated-side-effect rationale only holds for OAuth: a second
`redirectToAuthorization`, an overwritten PKCE verifier. With bearer, headers,
or no auth, a retry merely re-sends a request. So the duck-typed status check
is now consulted only when the caller configured `auth: { kind: 'oauth' }`,
threading the auth kind through `isAuthFailure` at all three guard sites.
`UnauthorizedError` stays unconditional — the SDK only throws it from the
provider-wrapped fetch and the authorization flow, so it inherently means a
provider is in play.
Two new tests pin the un-suppressed cases (no auth, bearer) and the existing
suppression tests now configure OAuth. Mutation-verified: un-scoping the
status check fails both new tests.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin caught the guard passing vacuously. The regex required the digits almost immediately after "efaults to", but mcp-connection.ts writes the default as "Defaults to\n * `DEFAULT_PROBE_TIMEOUT_MS` (30000)" — symbol name, newline, and parens in between — so the file produced zero matches and the for-loop passed on emptiness. The test's stated purpose was to catch the exact drift that had already happened, and it could not have caught it in one of the three files it named. Two changes: the pattern now tolerates up to 80 non-digit characters (including newlines) between the phrase and the number, and each file must produce at least one match — zero means the regex drifted from the prose, not that the file went quiet. Mutation-verified in the file that was previously uncovered: setting its doc back to 5000 now fails the test.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin: the auth short-circuit rethrows the first pass's error untouched, and a single-transport pass builds that error with only `cause` set — so `errors` was `[]` while the docs promised "every underlying failure, flat and in attempt order". A caller iterating `errors` without also inspecting `cause` saw nothing in precisely the auth case, where the rejection is the one thing worth finding. Fixed in the constructor rather than at the rethrow site: `errors` now defaults to `[cause]` when no explicit list is given, so every construction site gets the uniform contract and `flattenAttempts`' unwrap arm becomes a formality rather than load-bearing. The doc's "Empty when only one transport was attempted" claim is gone — it described the bug as if it were the design. Changeset example comment corrected to match. Test pins the exact shape Devin described: an auth failure on the implicit default yields errors of length 1 containing the UnauthorizedError. Mutation-verified — reverting the constructor default fails it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two from Devin on ad8ea0a, both consequences of MCPCacheWriteError now being a distinct, throwing type. **A failed write silenced tool-change subscribers.** `refresh()` adopts the new tools before it writes the snapshot back, so when the write rejected, the auto-refresh handler's `.then()` never ran and listeners were never told — while `handle.tools` already returned the new set. Subscribers stayed permanently out of sync, and the handler's own comment ("listeners keep the last good tool set") had become false: the set was already swapped. The handler now converts MCPCacheWriteError into a notification with the current tools; a failed re-list stays silent, which is correct because nothing was swapped. **A failed read was fatal while writes were best-effort.** `store.get` was awaited unguarded in tryCacheHit, so a store blip on the lookup rejected `createMCPTools()` when a plain miss would have connected fine — meaning the release note's "a store outage leaves you with a working handle" only held if the outage arrived after the read. A failing read is now a miss. Both mutation-verified: re-gating notification on the write fails the subscriber test; making the read fatal again fails the miss test.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin: two behaviors of `reconnectOnExpiry` changed in this PR and neither JSDoc site was updated — the hover text still promised "transparently fall back on any connection failure". 1. An auth failure now bypasses the fallback (the fallback reuses the same auth, so retrying cannot succeed and would re-drive an OAuth flow); those reject with MCPCacheError instead. 2. An over-age snapshot (staleness.maxAgeMs) is now a fallback trigger, which the option's one-line doc never mentioned. Both the field JSDoc and the rehydrateMCPTools block now state the full trigger list and the auth exception, per the repo's changed-option-semantics rule in public-api-examples/SKILL.md.
|
The 🟠 major — 🟡 🟡 failed Streamable HTTP client not 🟡 Verification at head: Three minors left open above — flagging for a human call on whether any should block, since none is the original blocker. |
1 participant
@openrouter/mcpnow works against any MCP server, whether it speaks2025-11-25or the new2026-07-28revision. Previously it only spoke the 2025 handshake.Correcting this PR's original premise
This PR started as deprecation prep, on my conclusion that no released SDK could speak
2026-07-28. That was wrong. I checked only@modelcontextprotocol/client@2.0.0's default wire behavior — which is indeed2025-11-25— and never looked for an opt-in. There is one:ClientOptions.versionNegotiation. So instead of annotating three surfaces as doomed, this PR makes both revisions work.How it works
protocolNegotiation?: 'legacy' | 'auto' | { pin: string }, defaulting to'auto'. The SDK itself defaults to'legacy'; a library whose job is "point it at a server" should reach both eras with nothing configured.2026-07-28server/discover, then requests with the_metaenvelope +Mcp-Methodheader. Noinitialize— the handshake is removed (SEP-2575).2025-11-25and earlierserver/discover, then fallback toinitialize+notifications/initialized, byte-equivalent to a 2025-only client.'legacy'skips the probe, which matters on flaky servers: over HTTP a probe timeout is an outage and rejects, where'legacy'may still connect.{ pin }fails loudly rather than falling back.A silent bug this caught
callToollost its middle argument in v2. The old three-arg call would have putsignalandonprogressin a slot the SDK doesn't read — disabling cancellation and progress streaming with every other test still passing.tests/unit/call-tool-shape.test.tsguards it; mutation-verified that restoring the old shape fails all 3 tests.Commits
chore(mcp)!— dependency swap; all compile-forced changes (specifiers,callToolarity, guard deletion, method-name-first handlers)test(mcp)— dual-era coverage overInMemoryTransport, before behavior changesfeat(mcp)!—protocolNegotiation, default'auto'fix(mcp)— pre-existing staleness gap on the direct rehydrate pathdocs(mcp)— retire the deprecations this made falsechore(mcp)— changeset for1.0.0Breaking changes
OAuthClientProvider: change the import specifier;tokens()now returnsStoredOAuthTokens(same fields, so most providers compile unchanged). Now re-exported asMCPOAuthClientProviderso consumers stop depending on our dependency's path.major→1.0.0, per @LukasParke's call. Two breaking notes and a dependency major justify declaring the API stable rather than shipping a pre-1.0 minor.protocolNegotiationdefaults to'auto'(SDK default:'legacy'), so every connection's first request is aserver/discoverprobe. Not a connectivity break: withprotocolNegotiationunset, a failed connect retries once with'legacy', so a probe-hostile proxy/WAF/gateway still connects as before. Setting it explicitly — including to'auto'— opts out of that retry.Also fixed
clientInfosaid0.1.0while the package was0.0.1— live in the published tarball. Now generated frompackage.jsonso it can't drift.staleness.maxAgeMswas only checked bycreateMCPTools; a directrehydrateMCPTools()replayed snapshots of any age.onElicitationun-deprecated — it works on both revisions, since the multi-round-trip driver (SEP-2322) routesinput_requiredthrough the same handler.API example
Verification
protocol-era.test.tsruns a hand-rolled MCP server overInMemoryTransport— no network, no fixture process, noMCP_TEST_URLgatecloseQuietlyto a bare.catch()fails all four sync-throw tests;cacheMode: 'use'fails the forced-re-read test; re-addingsessionIdforwarding fails the replay test; swapping thelist_changedkey for another valid method fails all four dispatch testsTwo field names I got wrong first and corrected:
server/discoverreturnssupportedVersions(notprotocolVersions), andinputRequestsis an object keyed by request id (not an array).Tracked in DEV-738.
🤖 Generated with Claude Code