-
Notifications
You must be signed in to change notification settings - Fork 98
feat: Add output schema support to MCP tools #153
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?
feat: Add output schema support to MCP tools #153
Conversation
|
Hi @chr-hertel, & @CodeWithKyrian the OutputSchema support PR is back up |
ec01d9a to
a2d7b6d
Compare
9234613 to
b3b69cd
Compare
|
Hi @chr-hertel, @Nyholm can I get some eyes on here for the output schema support. Ty |
|
Interesting. I do like the idea. I'll spend some more time to read the code. |
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.
In general this looks pretty good already, just one thing i noticed with one test scenario. structuredContent should be optional in response payload
| "isError": false, | ||
| "structuredContent": { | ||
| "result": "System status: OK (discovered)" | ||
| } |
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 wonder a bit about this change here - it's not adding value to me. spec is unclear about it, but i think we shouldn't just add it always.
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.
@chr-hertel this has now been actioned, so structrued content will only be added if outputschema is present and explicility specified
|
@chr-hertel To make |
e8de647 to
329533b
Compare
|
The case that i was pointing out was mostly just returning a string, but still had that |
c8f19cc to
2772869
Compare
4e96598 to
13d5b77
Compare
Good shout ! I was working with the wrong assumption that the result wrapper was in the spec. |
| } | ||
|
|
||
| return $toolExecutionResult; | ||
| } |
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.
shouldn't we leave that to implementations of the handler? I'm not sure we should alter the content at any point in the sdk.
| } | ||
|
|
||
| return $value; | ||
| } |
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.
Not sure to understand this already happens in formatResult if our goal is to return a structured content maybe that we should add a condition above?
| public static function success(array $content, ?array $meta = null, ?array $structuredContent = null): self | ||
| { | ||
| return new self($content, false, null, $meta); | ||
| return new self($content, false, $meta, $structuredContent); |
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.
not sure to why this is needed content can be a structured content since #93 or am I missing something?
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.
Missed that PR. We can use the existing structure. Thanks for flagging
| * required: string[]|null | ||
| * } | ||
| * @phpstan-type ToolOutputSchema array{ | ||
| * type: 'object', |
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.
| * type: 'object', | |
| * type: 'object', |
Can't the schema be a list? object type is not specified in the spec for the output.
| if (isset($data['outputSchema']['properties']) && \is_array($data['outputSchema']['properties']) && empty($data['outputSchema']['properties'])) { | ||
| $data['outputSchema']['properties'] = new \stdClass(); | ||
| } | ||
| } |
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 advise against validating this here we should leave that to the user discretion, it'll make the sdk more portable and subject to specification changes.
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 was following the validation pattern we've for the inputSchema setup above. Also, if we don't add validation for outputSchema, how can we have a structured output format? Users might use a structure we don't support, don't you think?
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.
Yeah I'm actually not a huge fan of the above inputSchema implementation either :).
I think it's the responsability of another abstraction to do this, and its also part of a bigger discussion around json schema and validation (I suggested already that these should not be coded into the php-sdk as they're quite complicated subjects).
|
|
||
| $result = $rawResult; | ||
| if (!$rawResult instanceof CallToolResult) { | ||
| $result = new CallToolResult($reference->formatResult($rawResult), structuredContent: $structuredContent); |
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'd try not to change this part of the code, I understand we want to comply with the spec that says:
Servers MUST provide structured results that conform to this schema.
If possible I think it should be handled in the formatResult.
My personal opinion would be to throw if the handler doesn't return a structured content.
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.
Yes, we're trying to comply with what the spec says here. I also think it reads better having it here otherwise, @chr-hertel feels otherwise
| "text": "{\n \"result\": 50,\n \"operation\": \"10 multiply 5\",\n \"precision\": 2,\n \"within_bounds\": true\n}" | ||
| } | ||
| ], | ||
| "isError": false |
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.
this should probably not change
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.
Just indentation, no code change
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 know but why as we didn't change anything to the json serialization this is weird to me and pollutes the diff
| } | ||
| ], | ||
| "isError": false | ||
| } |
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.
this should probably not change
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.
Just indentation, no code change
| "text": "{\n \"success\": true,\n \"config\": {\n \"app\": {\n \"name\": \"TestApp\",\n \"env\": \"development\",\n \"debug\": true,\n \"url\": \"https://example.com\",\n \"port\": 8080\n },\n \"generated_at\": \"2025-01-01T00:00:00+00:00\",\n \"version\": \"1.0.0\",\n \"features\": {\n \"logging\": true,\n \"caching\": false,\n \"analytics\": false,\n \"rate_limiting\": false\n }\n },\n \"validation\": {\n \"app_name_valid\": true,\n \"url_valid\": true,\n \"port_in_range\": true\n }\n}" | ||
| } | ||
| ], | ||
| "isError": false |
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.
this should probably not change (same for the other snapshots not sure why they change)
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.
Just indentation, no code change
| 'type' => 'object', | ||
| 'additionalProperties' => true, | ||
| ] | ||
| )] |
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.
on a general matter I think (if possible) that we should avoid to change existing fixtures but instead add new ones, as for that we need to change existing tests.
|
Nice addition we definitely miss this! Thanks! |
|
@bigdevlarry we have a misunderstanding here: my point was that output schema - in my understanding - is only needed for more complex structures - not for simple string responses for example. compared with the typescript-sdk: my take-away here is: there's no need for the |
|
@chr-hertel At the moment, if |



The changes add comprehensive support for outputSchema in the Tools definition, enabling MCP servers to specify the expected structure and types of tool responses.
This includes:
Added outputSchema parameter to Builder::addTool() method
Updated the Tool class constructor and JSON serialization to support outputSchema
Added validation for outputSchema structure and type requirements
Enhanced existing tests to verify outputSchema functionality works correctly
Fixed related type annotation issues and code style problems
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context