Skip to content

Client call_tool: input_responses/request_state retry params; InputRequiredResult via allow_input_required#2968

Merged
maxisbey merged 6 commits into
mainfrom
client-mrtr-call-tool
Jun 25, 2026
Merged

Client call_tool: input_responses/request_state retry params; InputRequiredResult via allow_input_required#2968
maxisbey merged 6 commits into
mainfrom
client-mrtr-call-tool

Conversation

@maxisbey

Copy link
Copy Markdown
Contributor

Stacks on s3-client-modern-path (#2950). Widens the client-side call_tool path so an InputRequiredResult can pass through and the caller can retry with input_responses / request_state.

Changes

ClientSession.send_requestresult_type now accepts TypeAdapter[T] alongside type[T], so callers can parse a union result. All existing single-class callers are unchanged.

ClientSession.call_tool (mechanics layer) — adds input_responses= / request_state= retry kwargs and returns CallToolResult | InputRequiredResult (matching MONOLITH_RESULTS["tools/call"]). Output-schema validation is gated on isinstance(result, CallToolResult).

Client.call_tool / ClientSessionGroup.call_tool (policy layer) — @overload on allow_input_required: the default (False) still returns CallToolResult so existing callers are unaffected; allow_input_required=True returns the union. With the default, an InputRequiredResult raises RuntimeError with a retry steer — # TODO(L80) marks where the auto-loop driver replaces this.

Direct ClientSession.call_tool callers — examples and tests that bypass Client now narrow with isinstance(result, CallToolResult). README.v2.md is regenerated from the updated snippets.

Not in this PR

Conformance

sep-2322-client-request-state already exists upstream at the pinned conformance build; it's currently waived in expected-failures*.yml. Un-waiving + adding the @register handler in .github/actions/conformance/client.py is the burndown step once this is on main.

Part of #2891.

AI Disclaimer

@maxisbey maxisbey added the spec-2026-07-28 2026-07-28 MCP spec release work label Jun 25, 2026
Base automatically changed from s3-client-modern-path to main June 25, 2026 14:09
@maxisbey maxisbey marked this pull request as ready for review June 25, 2026 14:22
maxisbey added 3 commits June 25, 2026 14:22
…equiredResult via allow_input_required opt-in

- ClientSession.send_request: accept TypeAdapter[T] alongside type[T] for
  result_type so callers can parse union results.
- ClientSession.call_tool (mechanics): add input_responses= / request_state=
  retry kwargs; return CallToolResult | InputRequiredResult; gate
  output-schema validation on isinstance(result, CallToolResult).
- Client.call_tool / ClientSessionGroup.call_tool (policy): @overload on
  allow_input_required — Literal[False] (default) returns CallToolResult;
  Literal[True] returns the union. Default raises RuntimeError on
  InputRequiredResult with a retry steer (TODO(L80) marks where the auto-loop
  driver replaces this).
- Examples and tests that call ClientSession.call_tool directly narrow with
  isinstance(result, CallToolResult); README.v2.md regenerated from snippets.
@maxisbey maxisbey force-pushed the client-mrtr-call-tool branch from adbfd73 to 0b4d8a9 Compare June 25, 2026 14:23
The gate now lives once on ClientSession (mechanics layer); Client and
ClientSessionGroup are pure passthroughs that forward allow_input_required.
Third 'bool' overload on ClientSession.call_tool lets the passthrough impls
type-check. Reverts the isinstance narrowing in examples/ and tests/shared/;
default return is CallToolResult everywhere, so the change is additive.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/mcp/client/session.py:402-418 — The Raises section of send_discover documents a ProbeNotRecognized exception, but no class with that name exists anywhere in the repository — callers cannot catch it. The bare-4xx case it describes is actually surfaced as MCPError (the streamable-HTTP transport maps it into a JSON-RPC error frame), so this entry should be removed or folded into the existing MCPError line.

    Extended reasoning...

    The bug: The docstring of ClientSession.send_discover (src/mcp/client/session.py:402-418) lists two exception types under Raises: MCPError for a JSON-RPC error from the server, and ProbeNotRecognized for "the transport bounced the request at its own layer (HTTP 4xx without a JSON-RPC error body)". The second entry references an exception class that does not exist: a repo-wide grep finds ProbeNotRecognized exactly once — in this docstring. It is not defined or exported in mcp.shared.exceptions (which has MCPError, MCPDeprecationWarning, NoBackChannelError, …), mcp.client.streamable_http, or anywhere else, so a caller writing except ProbeNotRecognized: would hit an ImportError/NameError before even reaching the call.

    Why the documented path can't happen as described: The streamable-HTTP transport never lets a bare HTTP 4xx escape as a transport-layer exception. In _handle_post_request (src/mcp/client/streamable_http.py), a non-2xx response with no parseable JSON-RPC error body is converted into a synthetic JSONRPCError frame and written onto the read stream — this PR even adds the new pre-session mapping where a bare 404 becomes METHOD_NOT_FOUND ("Not Found"), and other statuses become INVALID_REQUEST / INTERNAL_ERROR. The dispatcher then surfaces that frame to send_discover's send_raw_request call as an MCPError — exactly the exception type the first Raises line already documents.

    Step-by-step proof:

    1. negotiate_auto (or discover()) calls session.send_discover("2026-07-28"), which POSTs server/discover.
    2. A legacy server / gateway answers with a bare 404 and no JSON-RPC body.
    3. _handle_post_request sees response.is_success is false, fails to parse a JSON-RPC error from the body, and (since self.session_id is None) builds ErrorData(code=METHOD_NOT_FOUND, message="Not Found"), wrapping it in a JSONRPCError sent to the read stream (this is pinned by tests/client/test_streamable_http.py::test_pre_session_bare_404_maps_to_method_not_found).
    4. JSONRPCDispatcher correlates the error frame to the pending request and raises MCPError(code=-32601) from send_raw_request.
    5. The caller of send_discover therefore catches MCPError — never any ProbeNotRecognized. Consistently, both negotiate_auto in _probe.py and ClientSession.discover() only catch MCPError, and _probe.py's own module docstring says HTTP-layer 4xx rejections "are mapped into MCPError codes".

    Why nothing else prevents it: This is documentation only — no code references the name, so type checkers and tests are silent. It looks like a leftover from an earlier design where the transport surfaced bare 4xx as a distinct exception before the JSONRPCError-frame mapping was adopted.

    Impact: A reader of the docstring may try to import and catch ProbeNotRecognized and get a confusing ImportError, or wrongly conclude their except MCPError block won't cover the bare-4xx case. No functional impact on the SDK itself.

    Fix: Delete the ProbeNotRecognized entry, or fold it into the MCPError line, e.g. "MCPError: The server returned a JSON-RPC error, or the transport rejected the request at the HTTP layer (a bare 4xx is mapped into a JSON-RPC error frame)."

Comment thread src/mcp/client/session.py
Aligns with the class branch and the existing validate_python(..., by_name=False)
call sites in types/methods.py — wire data must use camelCase aliases only.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found and verified against the latest diff

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/mcp/client/client.py Outdated
Comment thread src/mcp/client/session_group.py Outdated
@maxisbey

Copy link
Copy Markdown
Contributor Author

Re the "additional findings" note about ProbeNotRecognized in send_discover's docstring: stale — grep -rn ProbeNotRecognized src/ tests/ docs/ returns nothing on this branch, and the docstring already reads MCPError: … a bare HTTP 4xx is synthesized into a JSON-RPC error by the transport, which is the suggested wording.

AI Disclaimer

…ayers

A runtime bool variable now matches the second overload (union return);
literal False and the default still match the first (CallToolResult).
Drops the redundant Literal[True] arm.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both follow-up fixes look right — the TypeAdapter branch now passes by_name=False (78b1f9d) and the overloads are collapsed to Literal[False]/bool consistently across all three layers (68fbef9) — but this widens the public call_tool API surface for a new protocol feature, so it deserves a human maintainer's look rather than auto-approval.

Extended reasoning...

Overview

This PR widens the client-side call_tool path across three layers (ClientSession, Client, ClientSessionGroup) so a 2026-07-28 tools/call can return an InputRequiredResult, adding input_responses/request_state retry kwargs and an allow_input_required opt-in gate (default False raises RuntimeError). ClientSession.send_request is generalized to accept a TypeAdapter result type, and migration docs plus tests for all three layers are added.

Security risks

None identified. The change is client-side result parsing and parameter plumbing; no auth, crypto, or permission logic is touched. The earlier wire-strictness inconsistency (lenient snake_case acceptance on the new TypeAdapter branch) was fixed in 78b1f9d by passing by_name=False, restoring uniform alias-only validation of inbound server data.

Level of scrutiny

Moderate-to-high. Although no bugs remain after the follow-up commits, this is a deliberate public-API design change (union return type, new opt-in flag, overload shape, a TODO placeholder for a future auto-loop driver) on a core client code path used by every SDK consumer. API-surface and naming decisions like allow_input_required and the interim RuntimeError behavior are judgment calls a maintainer should sign off on, especially since the description notes follow-up work (get_prompt/read_resource widening, conformance un-waiving) builds on this shape.

Other factors

Both prior reviewer findings (mine on by_name=False, cubic's on the missing bool overload) were addressed in commits 78b1f9d and 68fbef9, and I verified the fixes are present in the current branch. New tests cover the InputRequiredResult passthrough, the params threading, the no-opt-in RuntimeError, and the session-group forwarding. Existing default-path callers keep the narrow CallToolResult return via the Literal[False] overload, so backward compatibility looks preserved.

@maxisbey maxisbey merged commit 03681ed into main Jun 25, 2026
37 checks passed
@maxisbey maxisbey deleted the client-mrtr-call-tool branch June 25, 2026 15:37
@github-actions

Copy link
Copy Markdown
Contributor

This pull request is included in pre-release v2.0.0a3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spec-2026-07-28 2026-07-28 MCP spec release work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants