feat(client): MRTR retry loop, ClientTransport, version-gate notifications/initialized (SEP-2322/2575)#2065
Conversation
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, this adds a new public transport interface and wraps every outbound request in an MRTR override — the ClientTransport shape, the _requestWithSchema override strategy, and the notifications/initialized version gate on the existing pipe path are all design-level calls that warrant a maintainer's eyes (and a changeset).
Extended reasoning...
Overview
This PR touches the core of packages/client: it introduces a new public ClientTransport interface (plus ClientFetchOptions, isClientTransport), overrides Client._requestWithSchema so every outbound request now flows through a SEP-2322 MRTR retry loop, and overrides connect/notification/close to support the request-shaped path. It also changes existing pipe-transport behavior by version-gating notifications/initialized to < '2026-06-30'.
Security risks
Low. No auth, crypto, or untrusted-input parsing changes. The MRTR loop is bounded by mrtrMaxRounds (default 16) and respects maxTotalTimeout/signal, so a misbehaving server can't pin the client indefinitely. inputRequests are dispatched through the existing handler registry, so capability gating still applies.
Level of scrutiny
High. Per the repo's review conventions, new public exports need to be intentional and justified, and "one way to do things" applies — adding a parallel ClientTransport shape alongside Transport is exactly the kind of API-surface decision a maintainer should sign off on. The _requestWithSchema override sits on the hot path for every callTool/listTools/readResource/etc. call, and the notifications/initialized gate is a behavior change for all existing clients on newer protocol versions, not just the new transport path.
Other factors
- Three inline nits were found (dropped handler notifications creating a transport-shape divergence, a misleadingly-titled test that leaves the pipe-transport MRTR branch uncovered, and missing
try/finallyaround user-suppliedct.close()— the last one is a documented recurring catch in this repo). - No changeset despite new public API and behavior changes.
_initializeOrDiscoverlargely duplicates the init block inconnect(); whether to consolidate is a structural call for the reviewer.- Test coverage exercises the
ClientTransportpath well but, as noted inline, the pipe-transport MRTR fallthrough has no real coverage.
| return out; | ||
| } | ||
|
|
||
| /** Dispatch one server-initiated request through this client's handler registry. */ | ||
| private async _serviceInboundRequest(r: JSONRPCRequest, signal?: AbortSignal): Promise<JSONRPCResultResponse | JSONRPCErrorResponse> { | ||
| let final: JSONRPCResultResponse | JSONRPCErrorResponse | undefined; | ||
| for await (const out of this.dispatch(r, { signal })) { | ||
| if (out.kind === 'response') final = out.message as JSONRPCResultResponse | JSONRPCErrorResponse; | ||
| } | ||
| if (!final) { |
There was a problem hiding this comment.
🟡 nit: _serviceInboundRequest iterates this.dispatch(r) but only captures out.kind === 'response' — {kind: 'notification'} items (emitted when a handler calls ctx.mcpReq.notify()) are silently dropped. On the pipe path StreamDriver._onrequest forwards these via pipe.send({relatedRequestId}), so a sampling handler that reports progress will behave differently depending on transport shape. Consider passing notify: n => void this.notification(n) in the dispatch env (at least for the onrequest/subscribe call sites where a live channel exists), or document that handler-emitted notifications are discarded on this path.
Extended reasoning...
What the bug is
_serviceInboundRequest drives this.dispatch(r, {signal}) and only inspects out.kind === 'response'. But Dispatcher.dispatch can also yield {kind: 'notification', message}: when no env.notify is supplied, the dispatcher installs a default ctx.mcpReq.notify that queues the notification and yields it from the generator (dispatcher.ts:221-227, 263, 273). Those yielded notifications fall through the if here and are discarded.
Why this is a transport-shape divergence
On the pipe-Transport path the equivalent code is StreamDriver._onrequest (streamDriver.ts:272-278), which does forward them:
for await (const out of this.dispatcher.dispatch(request, env)) {
if (out.kind === 'notification') {
await this.pipe.send(out.message, { relatedRequestId: request.id });
} else ...
}So the same client handler — e.g. a sampling/createMessage handler that calls ctx.mcpReq.notify({method: 'notifications/progress', ...}) during a long generation — will deliver progress to the server over stdio/pipe but go silent over a ClientTransport. The repo's stated guideline is "no transport-specific assumptions"; this introduces one.
Step-by-step proof
- User registers
client.setRequestHandler('sampling/createMessage', async (req, ctx) => { ctx.mcpReq.notify({method:'notifications/progress', params:{progressToken: req.params._meta.progressToken, progress: 1}}); ...; return result; }). - Client connects via a request-shaped
ClientTransportwhose response stream carries a server-initiatedsampling/createMessage(delivered viaClientFetchOptions.onrequest). _requestRoundRawinvokesonrequest: r => this._serviceInboundRequest(r, signal)._serviceInboundRequestcallsthis.dispatch(r, {signal}). Noenv.notifyis passed, so dispatcher.ts:221-227 installs the queuing default.- Handler calls
ctx.mcpReq.notify(...)→ queued → yielded as{kind:'notification', message}at dispatcher.ts:263. - The
for awaitloop here sees it, theif (out.kind === 'response')test is false, the notification is dropped. Server never receives the progress update. - Same handler over a pipe
Transportwould hit streamDriver.ts:273-274 and the notification would reach the server.
On the dispatchToResponse precedent and the MRTR path
It's true that Dispatcher.dispatchToResponse (dispatcher.ts:418-425) does the identical discard and is documented as such. The difference is that dispatchToResponse is a generic convenience helper, whereas _serviceInboundRequest is specifically the ClientTransport replacement for StreamDriver._onrequest — which does forward — so the asymmetry is observable to handler authors.
It's also true that for the MRTR _serviceInputRequests caller there is no meaningful destination: the request id is synthetic (mrtr:${key}), the server already returned IncompleteResult, and it is awaiting a re-POST body, not progress on a token it never issued. Dropping is correct there. But _serviceInboundRequest has two other callers introduced in this PR — onrequest on the fetch response stream and onrequest on subscribe() — and on both of those a live outbound channel exists (this.notification() → this._clientTransport.notify()), the server issued a real request id, and forwarding is exactly what the pipe path does.
Impact
Low today: client-side handlers (sampling/elicitation/roots/ping) rarely call ctx.mcpReq.notify() in practice, and the report itself acknowledges that. This is why it's a nit rather than a blocker. But sampling-with-progress is the canonical case where a client handler legitimately emits notifications, and silently diverging from the pipe path is the kind of thing that's painful to debug later.
Suggested fix
Either pass notify in the dispatch env so the generator never yields notification items:
for await (const out of this.dispatch(r, { signal, notify: n => void this.notification(n).catch(e => this.onerror?.(e)) })) {or add an explicit else branch that forwards via this.notification(out.message). If the MRTR-synthetic caller should keep discarding, gate on whether this._clientTransport is set / the id is non-synthetic, or just document the limitation alongside the dispatchToResponse precedent.
|
|
||
| test('round-trip on the pipe-transport path: super._requestWithSchema sees IncompleteResult and loop retries', async () => { | ||
| // Drive both rounds through a fake ClientTransport but assert the loop machinery is on | ||
| // the override (not the fetch path) by checking that subscribe-stream notifications | ||
| // also flow when delivered via fetch onnotification. | ||
| let progressDelivered = false; | ||
| const ct = fakeClientTransport(LATEST_PROTOCOL_VERSION, { | ||
| 'tools/call': async (req, opts) => { | ||
| opts?.onnotification?.({ | ||
| jsonrpc: '2.0', | ||
| method: 'notifications/message', | ||
| params: { level: 'info', data: 'x' } | ||
| } as JSONRPCNotification); | ||
| return { jsonrpc: '2.0', id: req.id, result: { content: [] } }; | ||
| } | ||
| }); | ||
| const client = new Client({ name: 't', version: '0' }); | ||
| client.setNotificationHandler('notifications/message', async () => { | ||
| progressDelivered = true; | ||
| }); | ||
| await client.connect(ct); | ||
| await client.callTool({ name: 'x', arguments: {} }); | ||
| expect(progressDelivered).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 The test title and comment claim this exercises the pipe-transport path (super._requestWithSchema seeing an IncompleteResult and the loop retrying), but the body uses fakeClientTransport (kind: 'request'), so it actually goes through _clientTransport.fetch, never returns resultType: 'incomplete', and only asserts onnotification → dispatchNotification wiring. Either rename it to reflect what it actually covers, or replace it with a real InMemoryTransport-pair test that returns resultType: 'incomplete' — as written, the pipe-transport MRTR fallthrough (super._requestWithSchema(req, RAW_RESULT_SCHEMA, ...)) has zero coverage despite the title implying otherwise.
Extended reasoning...
What the test claims vs. what it does
The test at mrtr.test.ts:176 is titled "round-trip on the pipe-transport path: super._requestWithSchema sees IncompleteResult and loop retries", and its inline comment says it will "assert the loop machinery is on the override (not the fetch path)". Both statements describe behavior the test body never exercises.
Step-by-step trace of what actually executes
- The test constructs
fakeClientTransport(...), which returns an object withkind: 'request'(line 32 of the test file). client.connect(ct)callsisClientTransport(ct), which checks(t as ClientTransport).kind === 'request'→ true, so the client setsthis._clientTransport = ctand runs_initializeOrDiscover.super.connect(transport)is never called and no pipe-shapedTransportis wired.client.callTool(...)enters the overridden_requestWithSchema, which calls_requestRoundRaw. Becausethis._clientTransportis set, it takes the first branch and callsthis._clientTransport.fetch(...). The fallthroughreturn super._requestWithSchema(req, RAW_RESULT_SCHEMA, options)— the line the test title names — is never reached.- The fake handler immediately returns
{ content: [] }. There is noresultType: 'incomplete'anywhere, soisIncompleteResult(raw)is false on round 0 and the loop never retries. - The only assertion is
expect(progressDelivered).toBe(true), which verifies that theonnotificationcallback passed intofetchroutes throughdispatchNotificationto a registerednotifications/messagehandler.
So the test actually covers: "onnotification flows to a registered notification handler on the ClientTransport.fetch path". That is useful, but it is the opposite of what the title says (fetch path, not pipe path; no IncompleteResult; no retry).
Why this matters
The pipe-transport branch of _requestRoundRaw — return super._requestWithSchema(req, RAW_RESULT_SCHEMA, options) — is the codepath every existing stdio / SSE / Streamable-HTTP user will hit when a server returns an MRTR IncompleteResult. After this PR, that branch has zero test coverage, and the misleading title makes it look covered. A future regression in MRTR handling on pipe transports (e.g. RAW_RESULT_SCHEMA stripping resultType, or the override accidentally calling this._requestWithSchema and recursing) would not be caught.
Why nothing else catches it
All other tests in this file also use fakeClientTransport, so every MRTR assertion in the suite goes through the _clientTransport.fetch branch. There is no InMemoryTransport-pair (or other pipe-shaped Transport) test that returns resultType: 'incomplete'.
Suggested fix
Either:
- Rename this test to something like "onnotification routes to dispatchNotification on the ClientTransport.fetch path" and drop the misleading comment, and/or
- Add a real pipe-transport MRTR test: connect via an
InMemoryTransportpair (or a minimal pipe-shapedTransportstub), have the server side respond totools/callwith{ resultType: 'incomplete', requestState, inputRequests: {...} }on round 0 and a final result on round 1, and assert the retry carriesinputResponses/requestState. That would actually exercisesuper._requestWithSchema(req, RAW_RESULT_SCHEMA, ...)inside the loop.
This is a test-quality / coverage-gap issue rather than a runtime bug, hence flagged as a nit.
| if (this._clientTransport) { | ||
| const ct = this._clientTransport; | ||
| this._clientTransport = undefined; | ||
| await ct.close(); | ||
| this.onclose?.(); | ||
| return; | ||
| } | ||
| return super.close(); | ||
| } |
There was a problem hiding this comment.
🟡 nit: ct.close() is user-implemented (ClientTransport is a public interface), so if it throws, this.onclose?.() is skipped — and since _initializeOrDiscover does void this.close(), a throwing close() also becomes an unhandled rejection. Wrap it like InMemoryTransport.close() does: try { await ct.close(); } finally { this.onclose?.(); }.
Extended reasoning...
What
In the new close() override for the ClientTransport path:
override async close(): Promise<void> {
if (this._clientTransport) {
const ct = this._clientTransport;
this._clientTransport = undefined;
await ct.close();
this.onclose?.();
return;
}
return super.close();
}await ct.close() is not guarded by try/finally. ClientTransport is a newly-exported public interface that SDK consumers implement, so ct.close() is user-supplied code. If it throws, this.onclose?.() is never invoked.
Why existing code does not prevent it
_clientTransport is correctly cleared before the await, so the client's internal state is consistent (a second close() will fall through to super.close() and no-op). The only piece of teardown that gets skipped is the onclose callback — but that is exactly the callback consumers attach to learn the client has shut down (e.g. to release pooled resources, tear down UI, stop reconnection logic). Nothing else in the close path will fire it.
There is also a secondary effect: _initializeOrDiscover (added in this PR) does void this.close() in its catch block. If the user's ct.close() rejects there, the rejection has nowhere to go and surfaces as an unhandled promise rejection.
Step-by-step
- Consumer implements
ClientTransportwhoseclose()doesawait this.socket.end()and the socket is already torn — it throwsError: socket closed. - Consumer wires
client.onclose = () => pool.release(conn). - Consumer calls
await client.close(). _clientTransportis set → branch taken;_clientTransport = undefined.await ct.close()rejects withsocket closed.- Control leaves
close()via the throw;this.onclose?.()on the next line never runs. pool.release(conn)never runs; the connection leaks. (And on the_initializeOrDiscover→void this.close()path, the same throw becomes an unhandled rejection.)
Repo precedent
This is the repo's recurring lifecycle catch (#1735 / #1763): "In close()/shutdown paths, wrap user-supplied or chained callbacks in try/finally so a throw can't skip the remaining teardown." InMemoryTransport.close() (packages/core/src/util/inMemory.ts:51-55) already follows the pattern:
try {
await other?.close();
} finally {
this.onclose?.();
}Fix
const ct = this._clientTransport;
this._clientTransport = undefined;
try {
await ct.close();
} finally {
this.onclose?.();
}
return;Practical impact is limited (requires a throwing user close() implementation), so flagging as a nit rather than a blocker — but it is exactly the pattern the repo guidance asks reviewers to catch, and the fix is one line.
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
d7d52d9 to
ef0c0ce
Compare
felixweinberger
force-pushed
the
fweinberger/f2-client-mrtr
branch
from
397f185 to
8d81019
Compare
|
@claude review |
| if (transport.subscribe) { | ||
| void (async () => { | ||
| try { | ||
| for await (const n of transport.subscribe!({ onrequest: r => this._serviceInboundRequest(r) })) { | ||
| await this.dispatchNotification(n).catch(error => this.onerror?.(error as Error)); | ||
| } | ||
| } catch (error) { | ||
| this.onerror?.(error as Error); | ||
| } | ||
| })(); | ||
| } |
There was a problem hiding this comment.
🟡 nit: the fire-and-forget subscribe() loop has no closed-state guard — subscribe's opts type is Pick<ClientFetchOptions, 'onrequest'> (no signal), and the loop body never checks this._clientTransport !== transport before calling dispatchNotification/_serviceInboundRequest. close() clears _clientTransport and awaits ct.close(), but the ClientTransport.close() JSDoc only says 'release resources'; a transport that doesn't terminate its iterator (or a notification already yielded at close time) keeps dispatching after close, and on reconnect routes stale items into the new session. A one-line if (this._clientTransport !== transport) return; at the top of the loop body (or an AbortController aborted in close() and threaded into subscribe) would close the gap.
Extended reasoning...
What
In connect() for the ClientTransport path, the unsolicited stream is started fire-and-forget:
if (transport.subscribe) {
void (async () => {
try {
for await (const n of transport.subscribe!({ onrequest: r => this._serviceInboundRequest(r) })) {
await this.dispatchNotification(n).catch(error => this.onerror?.(error as Error));
}
} catch (error) {
this.onerror?.(error as Error);
}
})();
}There is no AbortSignal threaded into subscribe() — its opts type is Pick<ClientFetchOptions, 'onrequest'>, so the SDK has no way to signal cancellation — and the loop body never checks whether this._clientTransport === transport is still true before invoking dispatchNotification or _serviceInboundRequest. Termination therefore depends entirely on the user's ct.close() implementation ending the async iterable.
Why nothing prevents it
close() does clear _clientTransport and await ct.close(), but the ClientTransport.close() JSDoc only says "close the transport and release resources" — it does not contractually require terminating an active subscribe() iterator. ClientTransport is a newly-exported public interface that SDK consumers implement, so this is user-supplied code. Even with a well-behaved transport there is a race: a notification the iterator has already yielded (loop is suspended at the await this.dispatchNotification(n)) when close() runs will still be processed after _clientTransport is cleared.
Step-by-step
- User implements a
ClientTransportwhosesubscribe()returns an SSE-backed async iterable, and whoseclose()tears down the fetch session but does not calliterator.return()on the SSE stream (a plausible omission — the contract doesn't say it must). client.connect(ct)sets_clientTransport = ct, completes init, and starts the fire-and-forget loop. The loop capturestransport(===ct) andthisin its closure.- Later the user calls
await client.close()._clientTransportis set toundefined,await ct.close()resolves,onclosefires. - The SSE stream is still open; the server pushes
notifications/tools/list_changed. The loop body runsawait this.dispatchNotification(n)→ the registered list-changed handler fires → it callsthis.listTools()on a closed client. (Or, on theonrequestpath, a stale server request is routed into_serviceInboundRequestafter close.) - Worse, if the user then calls
client.connect(newCt),_clientTransportis nownewCt. The old loop is still iterating the old transport's stream; itsonrequest: r => this._serviceInboundRequest(r)closure dispatches stale server requests through the client's handler registry, and any handler that issues a follow-up request (this.notification(...),this.listTools()) goes out on the new transport. Two subscribe loops are now feeding the same client.
Repo precedent
This is the repo's documented recurring lifecycle catch (#1735 / #1763): "deferred callbacks must check closed/aborted state before mutating this._* or starting I/O — a callback scheduled pre-close can fire after close/reconnect and corrupt the new connection's state." The pipe-Transport path (StreamDriver.close() → pipe.close()) has the same implicit trust model, which is why this is a nit and not a blocker — but the pipe path is SDK-implemented on both sides, whereas ClientTransport is user-implemented, so the contract is weaker here.
Fix
Either or both of:
for await (const n of transport.subscribe!({ onrequest: r => this._serviceInboundRequest(r) })) {
if (this._clientTransport !== transport) return;
await this.dispatchNotification(n).catch(error => this.onerror?.(error as Error));
}or hold an AbortController on the client, abort it in close(), extend subscribe's opts to Pick<ClientFetchOptions, 'onrequest' | 'signal'>, and pass signal so well-behaved transports can break the iterator. The first option is one line and matches how the rest of the SDK guards deferred callbacks.
| /** SEP-2322 client-side detection. The server signals it cannot complete without client input. */ | ||
| /** | ||
| * Methods the SEP-2322 retry loop will service from `inputRequests`. The server cannot | ||
| * use this channel to invoke arbitrary client handlers (e.g. a custom-method handler the | ||
| * application registered for unrelated purposes); only the spec-defined client-side | ||
| * request methods are dispatched. | ||
| */ | ||
| const ALLOWED_INPUT_REQUEST_METHODS: ReadonlySet<string> = new Set(['sampling/createMessage', 'elicitation/create', 'roots/list', 'ping']); |
There was a problem hiding this comment.
🟡 nit: this one-line JSDoc ("SEP-2322 client-side detection…") describes isIncompleteResult(), but commit 8d81019 inserted ALLOWED_INPUT_REQUEST_METHODS and its own JSDoc between the comment and its target — so the line is now orphaned (shadowed by the block immediately below it) and isIncompleteResult has no doc. Move it down above isIncompleteResult or just delete it.
Extended reasoning...
What
Lines 217–224 currently read:
/** SEP-2322 client-side detection. The server signals it cannot complete without client input. */
/**
* Methods the SEP-2322 retry loop will service from `inputRequests`. ...
*/
const ALLOWED_INPUT_REQUEST_METHODS: ReadonlySet<string> = new Set([...]);
function isIncompleteResult(r: unknown): r is IncompleteResult { ... }Two JSDoc blocks are stacked back-to-back above ALLOWED_INPUT_REQUEST_METHODS. The first one ("SEP-2322 client-side detection. The server signals it cannot complete without client input.") semantically describes isIncompleteResult() — the function that detects the incomplete-result signal — not the method allowlist. Per JSDoc/TSDoc attachment rules only the immediately preceding block binds to the declaration, so line 217 is dead and isIncompleteResult ends up with no doc comment at all.
How it got there (step-by-step)
- Commit 324c93b introduced the MRTR loop with
/** SEP-2322 client-side detection... */sitting directly abovefunction isIncompleteResult(...), where it belonged. - Commit 8d81019 ("allowlist inputRequest methods…") inserted
ALLOWED_INPUT_REQUEST_METHODSplus its own multi-line JSDoc between that comment and the function. - Result: the original one-liner now sits above the wrong symbol, immediately shadowed by the new block. Classic refactor artifact.
Why nothing else catches it
TypeScript and TSDoc tooling silently attach the second (closer) block to ALLOWED_INPUT_REQUEST_METHODS and ignore the first; there is no lint rule in the repo for stacked JSDoc blocks. Both symbols are module-private (neither is exported via index.ts), so generated API docs are unaffected.
Impact
Cosmetic only — zero runtime effect, zero public-API-doc effect. This is why it's flagged as a nit rather than anything blocking. The only practical downside is that a reader hovering isIncompleteResult in an IDE gets no tooltip, and a reader scanning the source sees a comment that doesn't describe the constant beneath it. Acknowledging the counter-argument that this is borderline too minor to mention given three substantive comments already on the PR: it's raised only because it's a one-keystroke cleanup of an artifact introduced by this PR's own follow-up commit, so it's cheapest to fix now while the author is in the file.
Fix
Either move line 217 down to sit directly above function isIncompleteResult:
const ALLOWED_INPUT_REQUEST_METHODS: ReadonlySet<string> = new Set([...]);
/** SEP-2322 client-side detection. The server signals it cannot complete without client input. */
function isIncompleteResult(r: unknown): r is IncompleteResult {or delete it outright (the function name plus the inline comment in its body already convey the intent).
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
ef0c0ce to
c1b00ac
Compare
felixweinberger
force-pushed
the
fweinberger/f2-client-mrtr
branch
from
8d81019 to
68d1762
Compare
|
@claude review |
| /** | ||
| * Request-shaped client transport. One JSON-RPC request in, one terminal response out. | ||
| * The transport may be stateful internally (session id, protocol version) but the | ||
| * contract is per-call. | ||
| * | ||
| * Legacy pipe-shaped {@linkcode Transport} instances are accepted by | ||
| * {@linkcode client/client.Client.connect | Client.connect} unchanged; this interface | ||
| * is for transports that natively implement a fetch-per-request shape. | ||
| */ | ||
| export interface ClientTransport { |
There was a problem hiding this comment.
🟡 nit: this exports new public API (ClientTransport, ClientFetchOptions, isClientTransport, ClientOptions.mrtrMaxRounds) and widens Client.connect()'s signature, but no prose docs (docs/client.md, CLAUDE.md's Transport System section) or examples/client/ are updated — REVIEW.md's checklist asks for prose docs alongside JSDoc for new features, and the PR's own '[x] documentation' box is checked. Acknowledged this is mid-stack (v2-stateless, no concrete ClientTransport shipped yet) so deferring to a later PR may be intentional — if so, worth a one-line note in the PR description and unchecking the docs box.
Extended reasoning...
What's missing
This PR exports three new public symbols from packages/client/src/index.ts — ClientTransport, ClientFetchOptions, isClientTransport — adds the public ClientOptions.mrtrMaxRounds field, widens Client.connect() to accept Transport | ClientTransport, and changes user-visible behavior (SEP-2575 gates notifications/initialized on negotiated version). None of docs/client.md, docs/client-quickstart.md, CLAUDE.md's 'Transport System' / 'Outbound Flow' sections, or examples/client/ are touched.
Why the repo's own rules flag this
REVIEW.md → Tests & docs says: "New feature: verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example." The JSDoc on ClientTransport is good (it explains the request-shaped vs. pipe-shaped distinction and that legacy Transport keeps working), but there's no prose telling a consumer when to implement one over the other, and CLAUDE.md's transport overview still describes only the pipe model.
Earlier commits in the same v2-stateless stack (e.g. 299ec12, 0a5b206) did update docs/migration.md / docs/migration-SKILL.md for their user-facing changes, so 'docs deferred to end of stack' is not consistently the pattern here. The PR description also has '[x] I have added or updated documentation as needed' checked, which the diff contradicts.
Step-by-step verification
grep -r '\bClientTransport\b' docs/ CLAUDE.md→ only matches insideStreamableHTTPClientTransport/SSEClientTransport/StdioClientTransport; zero hits for the new bare interface.grep -r 'mrtrMaxRounds\|ClientFetchOptions\|request-shaped\|SEP-2322\|SEP-2575' docs/ CLAUDE.md→ no matches.git diff --name-onlyfor this PR → four files, none underdocs/orexamples/.packages/client/src/index.tsexports the three new symbols → public API.- PR checklist line:
[x] I have added or updated documentation as needed→ not reflected in diff.
Addressing the 'mid-stack plumbing, defer docs' objection
The strongest counter-argument is that this is one commit in a stacked series (label v2-stateless, 'Depends on S3', external review-guide gist) where no SDK-shipped concrete ClientTransport exists yet, so an examples/client/ snippet would have to show users hand-rolling the interface — not the intended use. REVIEW.md says to assess whether examples are needed, and 'defer until the concrete request-shaped transport ships' is a reasonable assessment.
That objection is largely correct, which is why this is a nit and not a blocker. But two things still warrant a one-line callout: (a) the PR's own checklist claims docs were updated when they weren't — that should be unchecked or annotated; and (b) mrtrMaxRounds and the SEP-2575 init-gate are user-observable on the existing pipe-transport path today (independent of any future concrete ClientTransport), so a sentence in docs/client.md for those would not be premature even mid-stack.
Impact
Documentation only — no runtime effect. Existing docs are incomplete, not contradictory (pipe transports still work exactly as described). The risk is that consumers reading the released API surface see ClientTransport / mrtrMaxRounds with no prose guidance on when or why to use them.
Suggested fix
Either:
- Add a short 'Request-shaped transports' subsection to
docs/client.md(when to implementClientTransportvs. wrap a pipeTransport) and a line onmrtrMaxRounds, plus update CLAUDE.md's 'Transport System' / 'Outbound Flow' to mention both shapes; or - If docs are intentionally deferred to a later stack PR, uncheck '[x] I have added or updated documentation as needed' and add a 'Docs: follow-up in S' note under Additional context so the gap is tracked.
…e notifications/initialized (SEP-2322/2575)
…mpling/elicitation/roots/ping only)
felixweinberger
force-pushed
the
fweinberger/s3-requires-input
branch
from
c1b00ac to
e579bff
Compare
felixweinberger
force-pushed
the
fweinberger/f2-client-mrtr
branch
from
68d1762 to
51d0c2e
Compare
|
@claude review |
1 participant
Client.request()detectsIncompleteResult, servicesinputRequestsvia local handlers, and retries withparams.{inputResponses, requestState}. AddsClientTransportinterface ({fetch, notify, subscribe?}) andClient.connect(Transport | ClientTransport).notifications/initializedis gated to negotiatedprotocolVersion < '2026-06-30'.Motivation and Context
Client side of SEP-2322 (MRTR retry) and SEP-2575 (init optional on discover path).
ClientTransportlets request-shaped transports (one POST per call) skip the pipe-shapedsend/onmessagecorrelation.How Has This Been Tested?
pnpm test:all+ client conformance.Breaking Changes
None.
connect(Transport)keeps working unchanged.Types of changes
Checklist
Additional context
Depends on S3. Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.