Skip to content

feat: multi round-trip requests — neutral contract and client auto-fulfilment engine#2313

Open
felixweinberger wants to merge 4 commits into
v2-2026-07-28from
fweinberger/mrtr-neutral-contract-engine
Open

feat: multi round-trip requests — neutral contract and client auto-fulfilment engine#2313
felixweinberger wants to merge 4 commits into
v2-2026-07-28from
fweinberger/mrtr-neutral-contract-engine

fix(client): era-consistent embedded sampling validation, whole-flow …

e461f95
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jun 16, 2026 in 14m 40s

Code review found 1 important issue

Found 5 candidates, confirmed 5. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 4
🟣 Pre-existing 0
Severity File:Line Issue
🔴 Important packages/client/src/client/client.ts:297-299 docs/migration.md still says input_required rejects — contradicts default-on auto-fulfilment
🟡 Nit packages/core/src/shared/inputRequired.ts:126-138 acceptedContent() is a userland reader added to the SDK surface (minimalism)
🟡 Nit packages/core/src/shared/inputRequiredDriver.ts:188-198 Concurrent embedded-request fulfilment never cancels sibling handlers on failure
🟡 Nit packages/core/src/shared/protocol.ts:239-250 Synthesized ctx.mcpReq.send/notify throw synchronously instead of rejecting
🟡 Nit packages/core/src/wire/rev2026-07-28/codec.ts:140-166 input_required decode is not gated to the three multi-round-trip methods

Annotations

Check failure on line 299 in packages/client/src/client/client.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

docs/migration.md still says input_required rejects — contradicts default-on auto-fulfilment

The migration guide (docs/migration.md, not touched by this PR) still documents the old input_required behavior and now contradicts the shipped default. Line 921 says input_required 'rejects with a typed local error — SdkErrorCode.UnsupportedResultType ... Full multi-round-trip support will replace that error arm', and line 950 says 'input_required / unknown kinds reject with UnsupportedResultType / InvalidResult' — but this PR sets DEFAULT_INPUT_REQUIRED_AUTO_FULFILL = true, so on 2026-07-28 co

Check warning on line 138 in packages/core/src/shared/inputRequired.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

acceptedContent() is a userland reader added to the SDK surface (minimalism)

`acceptedContent<T>(responses, key)` is a ~6-line content reader over the SDK-unvalidated `inputResponses` map (it checks `entry.action === 'accept'` and returns `entry.content`) — it does no protocol or validation work the SDK must own, and the bare responses are already surfaced at `ctx.mcpReq.inputResponses`. Per REVIEW.md Guiding Principle #1 ("content helpers belong in userland") and the API-surface checklist ("helpers users can write themselves belong in a cookbook, not the SDK"), consider

Check warning on line 198 in packages/core/src/shared/inputRequiredDriver.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Concurrent embedded-request fulfilment never cancels sibling handlers on failure

When a round's embedded requests are fulfilled concurrently with `Promise.all(entries.map(... dispatchInputRequest ...))` (inputRequiredDriver.ts ~188-198), a fast-failing entry (missing handler, unknown kind, or handler throw) rejects the whole call but never aborts the still-running sibling handlers — e.g. `input_required` with `{ ask: elicitation/create, sample: sampling/createMessage }` and no sampling handler: the `sample` dispatch throws `CapabilityNotSupported` immediately while the elici

Check warning on line 250 in packages/core/src/shared/protocol.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

Synthesized ctx.mcpReq.send/notify throw synchronously instead of rejecting

The synthesized embedded-handler context in _dispatchInputRequest binds ctx.mcpReq.send/notify to relatedMessagingUnavailable(), which throws synchronously, but BaseContext.mcpReq.send/notify are declared Promise-returning (and the wire-path implementations are async). A handler reused across wire and MRTR dispatch that uses the documented fire-and-forget idiom ctx.mcpReq.notify(n).catch(...) will have the throw escape before .catch can attach on the MRTR path, hard-failing the whole input-requi

Check warning on line 166 in packages/core/src/wire/rev2026-07-28/codec.ts

See this annotation in the file changed.

@claude claude / Claude Code Review

input_required decode is not gated to the three multi-round-trip methods

decodeResult() in packages/core/src/wire/rev2026-07-28/codec.ts discriminates input_required on the raw resultType alone, with no check on the method, so a 2026 server that answers a NON-MRTR method (e.g. completion/complete, tools/list) with input_required is driven into the auto-fulfilment driver (dispatching the client's elicitation/sampling/roots handlers and retrying the original call) instead of being rejected. This deviates from the SDK's own wire-response contract in schemas.ts, where on