-
Notifications
You must be signed in to change notification settings - Fork 55
add server output validation patch #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70dc46abba
ℹ️ 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".
| elif hasattr(results, "__iter__"): | ||
| unstructured_content = cast(UnstructuredContent, results) | ||
| maybe_structured_content = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid treating all iterables as content blocks
The fallback hasattr(results, "__iter__") will treat strings and pydantic models (e.g., EvaluationResult, which BaseTool explicitly allows) as if they were already a list of MCP content blocks. In those cases the server will emit a list of characters or (field, value) tuples instead of TextContent, and downstream consumers (e.g., result_to_string which expects .text/.data) will silently drop the output. This is especially problematic now that output validation is skipped, because invalid content will propagate to clients. Consider restricting this branch to list/tuple of content blocks and explicitly serializing str/BaseModel to TextContent/structuredContent.
Useful? React with 👍 / 👎.
06db7a7 to
7c6a63f
Compare
| unstructured_content = [types.TextContent(type="text", text=text)] | ||
| elif hasattr(results, "__iter__"): | ||
| unstructured_content = list(results) | ||
| maybe_structured_content = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String results incorrectly split into individual characters
Medium Severity
The hasattr(results, "__iter__") check at line 214 incorrectly matches strings and bytes, since these types are iterable in Python. If a tool function returns a string like "hello", calling list(results) will split it into individual characters ['h', 'e', 'l', 'l', 'o'] instead of treating it as a single result. This produces unexpected content that won't match types.TextContent in the subsequent structured content generation logic.
Note
Introduces a server-side patch to improve tool call handling and interoperability with FastMCP.
patch_server_output_validationto overrideServer.call_tool, skipping output validation while still validating inputs withjsonschemacallToolresults (accepts dict/tuple/iterable orCallToolResult) and wraps them intoServerResultstructuredContentfor FastMCP tools whenoutputSchema.x-fastmcp-wrap-resultis set, parsing JSON from text when possibleapply_all_patches()Written by Cursor Bugbot for commit 7c6a63f. This will update automatically on new commits. Configure here.