Add helpers for converting FunctionTool and TransformedTool to SamplingTool#3062
Add helpers for converting FunctionTool and TransformedTool to SamplingTool#3062
Conversation
…ngTool Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
strawgate
changed the title
marvin-context-protocol
Bot
added
enhancement
|
/marvin do.we actually need to do inline imports? I'm not convinced there's a circular dependency issue. Also adding any to the type hint is crazy, please address |
WalkthroughAdds a classmethod Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @strawgate's task in 2m 45s —— View job Fixed inline imports and type hints ✅
AnalysisCircular dependency claim: FALSE I verified that there's no circular dependency:
Changes Made
Files modified:
✅ All 26 sampling tests pass --- | Branch |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9785b1e5bc
ℹ️ 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".
| # If there's structured_content, use that | ||
| if result.structured_content is not None: | ||
| # Handle wrapped results | ||
| if ( | ||
| isinstance(result.structured_content, dict) | ||
| and "result" in result.structured_content | ||
| ): | ||
| return result.structured_content["result"] | ||
| return result.structured_content |
There was a problem hiding this comment.
The new TransformedTool wrapper treats any structured_content dict containing a "result" key as a wrapped payload and returns only that value. This loses data for legitimate schemas that include a result field alongside other properties (e.g., {result: ..., source: ...}), so sampling will drop fields and stringify only the inner value. This only occurs for TransformedTool outputs with structured content that happen to include a result key; consider checking the tool’s output schema (e.g., x-fastmcp-wrap-result) before unwrapping, otherwise return the dict unchanged.
Useful? React with 👍 / 👎.
There is no circular dependency between tools and sampling modules. Moved FunctionTool/TransformedTool imports to top-level and replaced Any type hints with proper FunctionTool | TransformedTool unions. Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
|
One suggestion on the ToolResult unwrapping logic (similar to codex): async def wrapper(**kwargs: Any) -> Any:
result = await tool.run(kwargs)
if isinstance(result, ToolResult):
# Check tool's schema - this is the source of truth
if tool.output_schema and tool.output_schema.get("x-fastmcp-wrap-result"):
# Tool wraps results: {"result": value} -> value
return result.structured_content["result"]
else:
# No wrapping: use structured_content directly
return result.structured_content
return resultThis mirrors what the client does in ^ may need a fallback for no structured content which I think you already ahve |
|
/marvin can you address jeremiah's feedback please |
|
Claude finished @strawgate's task in 1m 51s —— View job Addressed ToolResult unwrapping feedback
Changes MadeUpdated the Before: if (
isinstance(result.structured_content, dict)
and "result" in result.structured_content
):
return result.structured_content["result"]After: if tool.output_schema and tool.output_schema.get("x-fastmcp-wrap-result"):
# Tool wraps results: {"result": value} -> value
return result.structured_content.get("result")
else:
# No wrapping: use structured_content directly
return result.structured_contentAll tests pass and linting checks are clean. --- | Branch | |
Test Failure AnalysisSummary: Static type checking () is failing because test cases that intentionally pass invalid types to test error handling are being flagged as type errors. Root Cause: Three tests are deliberately passing wrong types to verify runtime error handling:
While these tests correctly verify runtime error handling, the static type checker ( Suggested Solution: Add # In tests/server/sampling/test_prepare_tools.py:106
with pytest.raises(TypeError, match="Expected SamplingTool, FunctionTool"):
prepare_tools(["not a tool"]) # type: ignore[arg-type]
# In tests/server/sampling/test_sampling_tool.py:226
SamplingTool.from_callable_tool(NotATool()) # type: ignore[arg-type]
# In tests/server/sampling/test_sampling_tool.py:235
SamplingTool.from_callable_tool(my_function) # type: ignore[arg-type]Detailed AnalysisThe type checker reports: This is expected behavior - the tests are verifying that the functions properly reject invalid types at runtime. The Related Files
|
Only unwrap the 'result' key from structured_content when the tool's output schema has x-fastmcp-wrap-result set. Otherwise, return structured_content directly. This mirrors the client's logic and prevents data loss for legitimate schemas with a 'result' field. Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
strawgate
changed the title
|
/marvin add a brief note about this to the v3-features document rc1 section (you'll see it if you pull main) |
|
Claude finished @jlowin's task in 13m 37s —— View job Added note to v3-features document ✅
Added a new "3.0.0rc1" section to All tests passed (4284 passed) and linting checks are clean. --- | Branch |
Co-authored-by: Jeremiah Lowin <jlowin@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fastmcp/server/sampling/sampling_tool.py (1)
171-171: Nit: redundant length check.
result.content and len(result.content) > 0— the truthiness check on the list already implies non-empty;len(...) > 0is redundant.Simplify the condition
- if result.content and len(result.content) > 0: + if result.content:
|
/marvin resolve merge conflicts please |
|
Claude finished @strawgate's task in 1m 18s —— View job Resolving merge conflicts
|
|
/marvin there's one conflict, what are you talking about. please resolve conflicts |
|
Claude finished @strawgate's task in 6m 20s —— View job Resolved merge conflict ✅
Successfully resolved the single merge conflict in All checks pass:
The merge commit has been pushed to the branch. | Branch |
…60203-0115 # Conflicts: # docs/development/v3-notes/v3-features.mdx
|
/marvin can you investigate the feedback? should we be using run directly? |
|
Claude finished @strawgate's task in 3m 40s —— View job Addressed feedback on tool.run() usage ✅
SummaryThe reviewer was correct - using Issue identified:
Changes made:
All 28 sampling tests pass ✅ |
- Changed from_callable_tool() to use tool.run() for both FunctionTool and TransformedTool - This ensures output_schema, serializers, and x-fastmcp-wrap-result flags are respected - Added unified ToolResult unwrapping logic for both tool types - Added tests for output_schema handling with and without wrap-result - Updated existing test to check callable instead of function identity Co-authored-by: Bill Easton <strawgate@users.noreply.github.com>
2 participants
Overview
This PR adds helpers to convert existing server tools (FunctionTool and TransformedTool) into SamplingTool objects, enabling servers to reuse their tools in sampling contexts without duplication.
Previously, if you had a tool defined with
@mcp.tooland wanted to use it inctx.sample(), you had to recreate the function separately. Now you can pass FunctionTool and TransformedTool instances directly to sampling methods.Changes
Added
SamplingTool.from_callable_tool()- Converts FunctionTool or TransformedTool to SamplingToolEnhanced
prepare_tools()- Now accepts FunctionTool and TransformedTool directlyctx.sample()andctx.sample_step()work with these tool typesExample
Fixes #3021
🤖 Generated with Claude Code