Client auto-resolves InputRequiredResult via existing callbacks (SEP-2322) - #2998
Client auto-resolves InputRequiredResult via existing callbacks (SEP-2322)#2998maxisbey merged 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
3 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="examples/stories/mrtr/server.py">
<violation number="1" location="examples/stories/mrtr/server.py:27">
P1: Static request_state token is forgeable, allowing confirmation-flow bypass.</violation>
<violation number="2" location="examples/stories/mrtr/server.py:31">
P1: Truthy check accepts non-boolean confirm values and can deploy unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Sorry, something went wrong.
| # Retry round: the client echoed request_state byte-exact and supplied the answer. | ||
| assert ctx.request_state == "awaiting-confirm", ctx.request_state | ||
| answer = responses["confirm"] | ||
| if isinstance(answer, ElicitResult) and answer.action == "accept" and (answer.content or {}).get("confirm"): |
There was a problem hiding this comment.
P1: Truthy check accepts non-boolean confirm values and can deploy unexpectedly.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At examples/stories/mrtr/server.py, line 31:
<comment>Truthy check accepts non-boolean confirm values and can deploy unexpectedly.</comment>
<file context>
@@ -0,0 +1,39 @@
+ # Retry round: the client echoed request_state byte-exact and supplied the answer.
+ assert ctx.request_state == "awaiting-confirm", ctx.request_state
+ answer = responses["confirm"]
+ if isinstance(answer, ElicitResult) and answer.action == "accept" and (answer.content or {}).get("confirm"):
+ return f"deployed to {env}"
+ return f"deployment to {env} cancelled"
</file context>
Sorry, something went wrong.
| ask = ElicitRequest( | ||
| params=ElicitRequestFormParams(message=f"Deploy to {env}?", requested_schema=CONFIRM_SCHEMA) | ||
| ) | ||
| return InputRequiredResult(input_requests={"confirm": ask}, request_state="awaiting-confirm") |
There was a problem hiding this comment.
P1: Static request_state token is forgeable, allowing confirmation-flow bypass.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At examples/stories/mrtr/server.py, line 27:
<comment>Static request_state token is forgeable, allowing confirmation-flow bypass.</comment>
<file context>
@@ -0,0 +1,39 @@
+ ask = ElicitRequest(
+ params=ElicitRequestFormParams(message=f"Deploy to {env}?", requested_schema=CONFIRM_SCHEMA)
+ )
+ return InputRequiredResult(input_requests={"confirm": ask}, request_state="awaiting-confirm")
+ # Retry round: the client echoed request_state byte-exact and supplied the answer.
+ assert ctx.request_state == "awaiting-confirm", ctx.request_state
</file context>
Sorry, something went wrong.
| async def test_session_call_tool_returns_input_required_result_when_opted_in() -> None: | ||
| """`ClientSession.call_tool(..., allow_input_required=True)` surfaces the | ||
| raw `InputRequiredResult` so the caller can drive the loop manually.""" | ||
|
|
||
| # `on_call_tool` is still typed `-> CallToolResult` on this branch (#2967 widens it later); | ||
| # `add_request_handler` is `HandlerResult`-typed and accepts `InputRequiredResult` cleanly. | ||
| async def handler(ctx: ServerRequestContext, params: types.CallToolRequestParams) -> types.InputRequiredResult: |
There was a problem hiding this comment.
🟡 Stale comment: this says on_call_tool is still typed -> CallToolResult and that #2967 "widens it later", but #2967 has already landed on this base — src/mcp/server/lowlevel/server.py already types on_call_tool as CallToolResult | InputRequiredResult, and this PR's own examples/stories/mrtr/server_lowlevel.py relies on the widened type. Since this test was renamed and re-docstringed in this PR, the obsolete comment should be updated or removed (nit).
Extended reasoning...What the issue is
The test test_session_call_tool_returns_input_required_result_when_opted_in (renamed and given a new docstring in this PR) carries forward this comment:
# `on_call_tool` is still typed `-> CallToolResult` on this branch (#2967 widens it later);
# `add_request_handler` is `HandlerResult`-typed and accepts `InputRequiredResult` cleanly.The first half of that claim is no longer true on this PR's base.
Why it's stale
src/mcp/server/lowlevel/server.py (around lines 149–152 on this base) already types on_call_tool as Awaitable[types.CallToolResult | types.InputRequiredResult] — the widening from #2967 (ae13ede) has landed. The PR itself depends on this: examples/stories/mrtr/server_lowlevel.py annotates its call_tool handler as -> types.CallToolResult | types.InputRequiredResult and passes it directly as on_call_tool=call_tool, and the new MCPServer-tier tests in tests/client/test_client.py return str | types.InputRequiredResult from decorated tools. The old examples/stories/mrtr/README.md text that this PR deletes also stated explicitly that #2967 already widened the handler return types in this base.
Why it's in scope for this PR
The PR edits the lines immediately around the comment — the test was renamed from test_call_tool_returns_input_required_result_when_server_requests_input and a docstring was added directly above the comment, plus the call inside was switched to client.session.call_tool(...). The obsolete comment was simply carried along in that edit.
Impact
No behavioral impact — the test still works as written, and using add_request_handler is fine. But the comment now misleads readers in two ways: it implies on_call_tool cannot return InputRequiredResult (it can), and it implies add_request_handler is used here out of typing necessity rather than choice. A reader following the comment might avoid on_call_tool for MRTR handlers unnecessarily.
Step-by-step proof
- Open src/mcp/server/lowlevel/server.py on this branch: the on_call_tool parameter type is Callable[..., Awaitable[types.CallToolResult | types.InputRequiredResult]] — already widened.
- Open this PR's examples/stories/mrtr/server_lowlevel.py: async def call_tool(...) -> types.CallToolResult | types.InputRequiredResult is passed as on_call_tool=call_tool and type-checks — which would be impossible if the comment's claim were still accurate.
- Therefore the parenthetical "(lowlevel Server: widen on_* return types for InputRequiredResult; add subscriptions/listen slot #2967 widens it later)" describes a state that no longer exists on this base.
How to fix
Delete the first comment line, or rewrite it to state the actual (current) reason for using add_request_handler here (e.g. "register a raw handler so the test controls the exact wire-shape result"), keeping the second line about add_request_handler if still useful.
Sorry, something went wrong.
…2322) Client.call_tool/get_prompt/read_resource now resolve InputRequiredResult automatically by dispatching the embedded sampling/elicitation/roots input_requests to the same callbacks that already serve legacy server-to-client RPCs, then retrying with the collected input_responses + echoed request_state until a terminal result arrives. allow_input_required is removed from Client; manual control is via client.session.<method>(..., allow_input_required=True). - New client/_input_required.py: pure method-agnostic driver. max_rounds=10 default; state-only legs back off 50ms doubling to 250ms cap (counter resets on any leg with input_requests). request_state passed through byte-exact. InputRequiredRoundsExceededError on cap; MCPError when a callback returns ErrorData. - ClientSession: extracted _dispatch_input_request from the _on_request match so the legacy RPC path and the driver share one dispatch table. get_prompt/read_resource gain the allow_input_required overload set. - Client: input_required_max_rounds field; shared _drive_input_required helper; call_tool/get_prompt/read_resource collapse to a single signature returning the bare result type. - examples/stories/mrtr promoted from deferred stub to runnable. - Conformance fixture sep-2322-client-request-state rewritten to drive the same five wire-shape checks via the auto-loop. - docs/advanced/multi-round-trip.md + docs_src/mrtr/tutorial003.py updated.
…o-drive-yourself - multi-round-trip.md: spell out that all three callbacks serve both eras (standalone RPCs gone in 2026, same payloads ride in input_requests); expand the manual-loop section with the distributed-client / inspection / wall-clock-bound use cases. - callbacks.md: note that the 2026 auto-loop dispatches to the same elicitation/sampling/roots callbacks; clarify it's the RPCs that go, not the callbacks. - deprecated.md: roots replacement now points at MRTR (ListRootsRequest embedded in InputRequiredResult); clarify payload-survives-RPC-doesn't. - tutorial/elicitation.md: recap bullet linking to MRTR.
| ### `InputRequiredResult` handling differs between `Client` and `ClientSession` | ||
|
|
||
| For protocol 2026-07-28, a `tools/call` request may return an `InputRequiredResult` asking the client to supply additional input and retry. By default `call_tool` (on `ClientSession`, `Client`, and `ClientSessionGroup`) still returns `CallToolResult` and raises `RuntimeError` if the server requests input. Pass `allow_input_required=True` to receive the `InputRequiredResult` instead, then retry with `input_responses=` / `request_state=`. | ||
| For protocol 2026-07-28, `tools/call`, `prompts/get`, and `resources/read` may return an `InputRequiredResult` asking the client to supply additional input (sampling, elicitation, roots) and retry. | ||
|
|
||
| On the high-level `Client`, `call_tool`, `get_prompt`, and `read_resource` resolve this automatically: they dispatch each requested input to the matching callback (`sampling_callback`, `elicitation_callback`, `list_roots_callback`) and retry until a final result is returned, so the call still returns the bare `CallToolResult` / `GetPromptResult` / `ReadResourceResult`. The round limit is `Client(input_required_max_rounds=...)` (default 10). Earlier v2 prereleases exposed an `allow_input_required` parameter on these `Client` methods; that parameter has been removed. For manual control use `client.session.call_tool(..., allow_input_required=True)`. Note that `read_timeout_seconds` now bounds each underlying round, not the whole loop; wrap the call in `anyio.fail_after(...)` for a whole-loop bound. | ||
|
|
||
| On `ClientSession`, `call_tool` / `get_prompt` / `read_resource` still return the bare result and raise `RuntimeError` if the server requests input. Pass `allow_input_required=True` to receive the `InputRequiredResult` instead, then drive the loop yourself with `input_responses=` / `request_state=`. `ClientSessionGroup.call_tool` accepts the same flag. |
There was a problem hiding this comment.
🟡 The new sentence "Earlier v2 prereleases exposed an allow_input_required parameter on these Client methods" overstates history: only Client.call_tool ever had allow_input_required — Client.get_prompt and Client.read_resource never accepted it (they gain MRTR parameters for the first time in this PR). Consider rewording to "on Client.call_tool" so the migration note doesn't send users hunting for a parameter that never existed on those two methods.
Extended reasoning...What the issue is
The reworked migration note in docs/migration.md (lines 398–404) says:
Earlier v2 prereleases exposed an allow_input_required parameter on these Client methods; that parameter has been removed.
The immediately preceding sentence enumerates call_tool, get_prompt, and read_resource on the high-level Client, so "these Client methods" reads as all three. But the history it describes is only true for one of them.
Why the claim is inaccurate for two of the three methods
The pre-PR state is visible in this PR's own diff of src/mcp/client/client.py:
- Client.call_tool had the @overload pair and the allow_input_required: bool = False keyword — that is exactly what this PR removes.
- Client.get_prompt's old signature was (name, arguments=None, *, meta=None) — no allow_input_required, no input_responses, no request_state.
- Client.read_resource's old signature was (uri, *, meta=None) — likewise.
get_prompt and read_resource gain input_responses/request_state (and, on ClientSession, the allow_input_required overload set) for the first time in this PR. So no prerelease ever exposed allow_input_required on Client.get_prompt or Client.read_resource, and the migration note misstates what users are migrating from.
Impact
Low. The actionable guidance in the paragraph is still correct regardless: the auto-loop now resolves InputRequiredResult on all three Client methods, and manual control lives at client.session.<method>(..., allow_input_required=True). A user migrating get_prompt/read_resource calls simply finds nothing to remove. One refutation argued this can't lead any reader to a wrong action — that's fair on the action level, which is why this is a nit rather than a blocking finding; but a migration guide's job is to describe what changed, and stating that a parameter "has been removed" from methods it never existed on is the kind of inaccuracy that costs readers a fruitless changelog/git-history search when they try to reconcile the claim with their own code.
Step-by-step proof
- Take the pre-PR Client.get_prompt from the left side of this diff: async def get_prompt(self, name, arguments=None, *, meta=None) -> GetPromptResult. There is no allow_input_required to remove.
- The new migration text says that parameter "has been removed" from "these Client methods", where the antecedent (previous sentence) is call_tool, get_prompt, and read_resource.
- A reader who used get_prompt on an earlier prerelease and consults this note to learn what changed is told a parameter was removed from a method that never had it — false for 2 of the 3 methods named.
How to fix
One-phrase reword, e.g.: "Earlier v2 prereleases exposed an allow_input_required parameter on Client.call_tool; that parameter has been removed, and Client.get_prompt / Client.read_resource never had it." (Or simply "on Client.call_tool".)
Sorry, something went wrong.
Client.call_tool / get_prompt / read_resource now resolve InputRequiredResult automatically: each entry in input_requests is dispatched to the same sampling_callback / elicitation_callback / list_roots_callback that already serve legacy server-to-client RPCs, the responses are collected, and the call is retried with input_responses + the echoed request_state until a terminal result arrives. allow_input_required is removed from Client; for manual control (inspecting rounds, persisting request_state across processes) use client.session.call_tool(..., allow_input_required=True) which still returns the raw union.
Motivation and Context
Client half of SEP-2322 / #2898. The server side and the manual ClientSession surface landed in #2974; this adds the policy-layer driver so a high-level Client user never sees InputRequiredResult directly.
Shape
Also: examples/stories/mrtr/ promoted from deferred stub; docs/advanced/multi-round-trip.md + docs_src/mrtr/tutorial003.py rewritten; conformance fixture sep-2322-client-request-state rewritten to drive the same five wire-shape checks via the auto-loop with a registered elicitation_callback.
How Has This Been Tested?
Breaking Changes
Alpha-only API removal: Client.call_tool / get_prompt / read_resource no longer accept allow_input_required. Use client.session.<method>(..., allow_input_required=True) for the manual surface. Noted in docs/migration.md.
Types of changes
Checklist
Additional context
Open for review:
Part of #2898.
AI Disclaimer