Fix: Always return CallToolResult when structured_content exists (#3596)#3604
Fix: Always return CallToolResult when structured_content exists (#3596)#3604hnshah wants to merge 2 commits intoPrefectHQ:mainfrom
Conversation
…fectHQ#3596) Problem: In FastMCP 3.1, CallToolResult.data field returns None even when structured data is present. This breaks existing code that relies on result.data to access structured tool outputs. Root Cause: The to_mcp_result() method in ToolResult was returning a tuple (content, structured_content) instead of a CallToolResult object when meta=None but structured_content exists. The client's _parse_call_tool_result() function expects a CallToolResult with a structuredContent field to populate .data, but when receiving a tuple, it couldn't extract the data properly. Solution: Always return CallToolResult when structured_content is not None, regardless of whether meta is present. This ensures the client consistently receives an object with the structuredContent field, which gets properly parsed into the .data field. The optimization of returning bare tuples was breaking the client contract. Consistency is more important than micro-optimization here. Test Case: result = await client.call_tool("tool_name", {}) assert result.data is not None # Now passes Fixes PrefectHQ#3596
marvin-context-protocol
Bot
added
the
too-long
|
Thanks for the report. This issue goes beyond what our contributor guidelines ask for — we just need a short problem description and an MRE. Please see our contributing guidelines and condense this issue. We'll triage it once it's trimmed down. |
Test Failure AnalysisSummary: The PR breaks an existing test by dropping Root Cause: The reordering of conditions in Original code (before PR): if self.meta is not None: # ← handles meta-only case
return CallToolResult(...)
if self.structured_content is None:
return self.content
return self.content, self.structured_content # ← the bug being fixedPR's new code: if self.structured_content is not None: # ← fixes the tuple bug...
return CallToolResult(...)
return self.content # ← ...but drops meta when structured_content is None!The failing test ( Suggested Solution: Use a combined condition to return def to_mcp_result(self):
if self.meta is not None or self.structured_content is not None:
return CallToolResult(
structuredContent=self.structured_content,
content=self.content,
_meta=self.meta, # type: ignore[call-arg]
)
return self.contentThis preserves the fix for issue #3596 (structured_content without meta returns Detailed AnalysisFailing test ( @fastmcp_server.tool
def tool_with_meta(value: str) -> ToolResult:
return ToolResult(
content=f"Result: {value}",
meta={"custom_key": "custom_value", "processed": True},
)
result = await client.call_tool("tool_with_meta", {"value": "test"})
assert result.meta == {"custom_key": "custom_value", "processed": True} # FAILSError from logs: The test fails on all matrix combinations (Python 3.10/3.13, ubuntu/windows, lowest-direct deps). Related Files
|
marvin-context-protocol bot correctly identified that the previous fix broke test_call_tool_forwards_meta by dropping the meta field when structured_content is None. Updated to use OR condition: return CallToolResult when EITHER meta OR structured_content is present, not just when structured_content exists. This fixes both: - Original bug PrefectHQ#3596 (structured_content without meta) - Regression (meta without structured_content) Co-authored-by: marvin-context-protocol[bot]
|
Thanks @marvin-context-protocol! You're absolutely right - I broke the meta-only case. Fixed in latest commit (c8cdef7):
The test should pass now. Apologies for the oversight! |
|
See the note on #3596 please; this issue needs an MRE before it has a PR |
|
Closing, see #3596 |
2 participants
Fixes #3596
Problem
CallToolResult.datareturnsNonewhen structured data is present.Root Cause
to_mcp_result()returned tuple instead ofCallToolResultwhenmeta=Nonebutstructured_contentexists.Solution
Return
CallToolResultwhen eithermetaorstructured_contentis present (OR condition).MRE
Fixes both:
Co-authored-by: marvin-context-protocol[bot]