-
-
Notifications
You must be signed in to change notification settings - Fork 45
[MCP SDK] ToolCallHandler
now returns structuredContent
if applicable
#225
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
base: main
Are you sure you want to change the base?
Conversation
2f2dc9f
to
ad7601d
Compare
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.
I have a similar patch in local testing :)
ToolCallHandler
now returns structuredContent if applicable
ToolCallHandler
now returns structuredContent if applicableToolCallHandler
now returns structuredContent
if applicable
Please rebase, thanks |
ad7601d
to
f81c226
Compare
@OskarStark done 👍 |
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.
Two things, I think this is good timing to add a test for checking the two options for type text.
Isn't it that with the current approach we end up with something like "text": "\"foobar\""
for simple, unstructured text values?
Open to finish this PR @xavierleune ? |
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.
Pull Request Overview
This PR implements structured content support in the MCP SDK's ToolCallHandler
to comply with the MCP specification. The change allows tools to return structured data (arrays/objects) which will be included in both the structuredContent
field and serialized as JSON in the text content for backwards compatibility.
- Modified
ToolCallResult
to accept mixed data types instead of strings only - Updated
ToolCallHandler
to detect structured content and include it in thestructuredContent
field - Fixed a typo in the default MIME type from "text/plan" to "text/plain"
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/mcp-sdk/src/Capability/Tool/ToolCallResult.php | Changed result property from string to mixed type and fixed MIME type typo |
src/mcp-sdk/src/Server/RequestHandler/ToolCallHandler.php | Added structured content detection and JSON encoding with proper error handling |
]); | ||
]; | ||
|
||
if ('text' === $result->type && \is_array($result->result) || \is_object($result->result)) { |
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.
The operator precedence is incorrect. The condition should be grouped as ('text' === $result->type) && (\is_array($result->result) || \is_object($result->result))
to ensure structured content is only added for text type results.
if ('text' === $result->type && \is_array($result->result) || \is_object($result->result)) { | |
if ('text' === $result->type && (\is_array($result->result) || \is_object($result->result))) { |
Copilot uses AI. Check for mistakes.
As per documentation we should include any structured content in a dedicated field