fix: elicitation scalar return, resource auto-serialization, Client.new() state, prompt errors#3859
fix: elicitation scalar return, resource auto-serialization, Client.new() state, prompt errors#3859
Conversation
@chrisguidry was this intentional? |
678f3d2 to
732a6a6
Compare
Test Failure Analysis
Summary: The Root Cause:
Suggested Solution:
Detailed AnalysisFailure 1 — ruff-check Ruff's auto-fix diff: --- a/src/fastmcp/client/client.py
+++ b/src/fastmcp/client/client.py
@@ -70,7 +70,6 @@ from .transports import (
PythonStdioTransport,
SessionKwargs,
SSETransport,
- StdioTransport,
StreamableHttpTransport,
infer_transport,
)The import was needed before this PR to satisfy the if not isinstance(self.transport, StdioTransport):
new_client._session_state = ClientSessionState()That condition was removed by the PR (always reset now), but the import was not cleaned up. Failure 2 — ty check
Related Files
|
No, that does not sound intentional to me, I'd think we'd want each client to start fresh. If the server supports task listing then a client can ask for them, so we don't need to copy these between instances. |
…ew() state, prompt errors
- Auto-wrap scalar elicitation responses for ScalarElicitationType schemas
so handlers can return T directly for ctx.elicit("msg", str/int/float)
- Auto-serialize dict/int/float/bool/None resource returns to JSON text
instead of crashing with TypeError
- Reset _task_registry and _submitted_task_ids in Client.new() so cloned
clients have independent task tracking state
- Include original error message in prompt render errors (matching tool
error behavior)
Fixes #3856
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…urces The comment said "list/tuple of primitives" but the isinstance check didn't include list or tuple. Now it does, and the comment matches. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ng for JSON resources Two review-identified bugs: 1. Client.new() shallow-copies _session_kwargs, so the cloned client's TaskNotificationHandler still dispatches to the original client. Fix: create a fresh _session_kwargs dict with a new handler bound to the new client. 2. convert_result() for dict/int/float/bool/None fell through to ResourceResult(raw_value) which lost component meta (CSP, permissions). The str/bytes path correctly wrapped in ResourceContent with meta. Fix: explicitly serialize JSON-native types and wrap with meta, matching the str/bytes path. Other types still fall through for error handling. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only replace the message handler with a new TaskNotificationHandler if the current handler IS a TaskNotificationHandler. If the user provided a custom message_handler, preserve it in the clone. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bbd1ebd to
1759276
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1759276a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
One more thing before merge — the Codex P2 about Minimal fix is to short-circuit list-of-ResourceContent ahead of the JSON path: # list[ResourceContent] passes through — otherwise JSON-serialize below
if isinstance(raw_value, list) and all(
isinstance(item, ResourceContent) for item in raw_value
):
return ResourceResult(raw_value)Happy to push it myself if easier — but flagging so it doesn't slip through. |
A bare list[ResourceContent] would match the isinstance(list) check and get JSON-serialized instead of passing through to ResourceResult normalization. Check for ResourceContent items first. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Four client bugs found during a systematic audit of the FastMCP client API surface.
Elicitation scalar return: Handlers returning
Tdirectly forctx.elicit("msg", str)crashed with "must be serializable as a JSON object." The type signature promises this works — now it does by auto-wrapping scalars into{"value": scalar}forScalarElicitationTypeschemas.Resource auto-serialization: Resources returning
dict,int,float,bool, orNonecrashed withTypeErrordespite the docstring promising auto-conversion. Now auto-serialized to JSON text.Client.new() shared state:
copy.copy()shared_task_registryand_submitted_task_idsbetween original and cloned clients, violating the "fresh session state" contract.Prompt error message:
function_prompt.pydropped the original exception message, unlike tool errors which preserve it.Fixes #3856
🤖 Generated with Claude Code