-
Notifications
You must be signed in to change notification settings - Fork 79
[Server] issues-68 No Ability to set outputSchema #88
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?
[Server] issues-68 No Ability to set outputSchema #88
Conversation
|
@chr-hertel PR to address #68 is now up for review |
|
Hi @bigdevlarry, thanks for working on this - look great already. was thinking tho if it makes sense to extend maybe one of our examples - or bring in a new one to demo this? WDYT & cc @CodeWithKyrian |
|
I didn’t know how to extend but I’m happy with either extending or bringing a new example tho … |
ebca579 to
b9a1058
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 guess the #[McpTool] attribute also needs to be extended as well
and you could extend the tool in example no5
b9a1058 to
4d56220
Compare
|
Okay, that make sense. You can have a look again |
|
Pipeline issue related to modelcontextprotocol/inspector#834 |
|
This is solid work, @bigdevlarry ! Appreciate the effort you put into this! On a side note, I was wondering if there’s a way we could also infer the Just a thought though...curious what you all think. |
|
https://modelcontextprotocol.io/specification/2025-06-18/server/tools#structured-content While working on my MCP this one is not seems to be working too. It looks like if outputSchema provided server must also provide structuredContent alongside content. I will try this PR in my fork and will try to implement structured content in CallToolResult. Because of this I wouldn't add auto schema definition, developer should be aware that by adding |
3d52d35 to
16e3b32
Compare
|
@CodeWithKyrian, thanks for the shout and feedback. I've included support to help infer the outputSchema when it’s not explicitly defined, similar to how we handle inputSchema. Also on this one -
I'm not sure how you mean here. If you create a separate issue with more description, then I can pick it up. @chr-hertel Could you give the PR another look? I think should be good |
16e3b32 to
4f9df3d
Compare
|
hi @chr-hertel Still need a review/feedback here |
| */ | ||
| public function getReturnTypeString(?DocBlock $docBlock): ?string | ||
| { | ||
| if (!$docBlock) { |
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.
always like to be explicit
| if (!$docBlock) { | |
| if (null === $docBlock) { |
| } | ||
|
|
||
| $returnTags = $docBlock->getTagsByName('return'); | ||
| if (empty($returnTags)) { |
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.
and try to avoid empty:
| if (empty($returnTags)) { | |
| if ([] === $returnTags) { |
| } | ||
|
|
||
| $returnTag = $returnTags[0]; | ||
| if (method_exists($returnTag, 'getType') && $returnTag->getType()) { |
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.
can we check for TagWithType instead of using method_exists?
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.
and should be inverted to:
if (!$returnTag instanceof TagWithType) {
return null;
}it's better to continue with the style of early exits - like you did in the beginning of the method
| /** | ||
| * Generates a JSON Schema object (as a PHP array) for a method's or function's return type. | ||
| * | ||
| * @return array<string, mixed>|null | ||
| */ | ||
| public function generateOutputSchema(\ReflectionMethod|\ReflectionFunction $reflection): ?array | ||
| { | ||
| $docComment = $reflection->getDocComment() ?: null; | ||
| $docBlock = $this->docBlockParser->parseDocBlock($docComment); | ||
|
|
||
| $docBlockReturnType = $this->docBlockParser->getReturnTypeString($docBlock); | ||
| $returnDescription = $this->docBlockParser->getReturnDescription($docBlock); | ||
|
|
||
| $reflectionReturnType = $reflection->getReturnType(); | ||
| $reflectionReturnTypeString = $reflectionReturnType | ||
| ? $this->getTypeStringFromReflection($reflectionReturnType, $reflectionReturnType->allowsNull()) | ||
| : null; | ||
|
|
||
| // Use DocBlock with generics, otherwise reflection, otherwise DocBlock | ||
| $returnTypeString = ($docBlockReturnType && str_contains($docBlockReturnType, '<')) | ||
| ? $docBlockReturnType | ||
| : ($reflectionReturnTypeString ?: $docBlockReturnType); | ||
|
|
||
| if (!$returnTypeString || 'void' === strtolower($returnTypeString)) { | ||
| return null; | ||
| } | ||
|
|
||
| return $this->buildOutputSchemaFromType($returnTypeString, $returnDescription); | ||
| } | ||
|
|
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.
please add tests for this method
| public function getReturnDescription(?DocBlock $docBlock): ?string | ||
| { | ||
| if (!$docBlock) { | ||
| return null; | ||
| } | ||
|
|
||
| $returnTags = $docBlock->getTagsByName('return'); | ||
| if (empty($returnTags)) { | ||
| return null; | ||
| } | ||
|
|
||
| $returnTag = $returnTags[0]; | ||
| $description = method_exists($returnTag, 'getDescription') | ||
| ? trim((string) $returnTag->getDescription()) | ||
| : ''; | ||
|
|
||
| return $description ?: null; | ||
| } |
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.
similar comments would apply to this method like with getReturnTypeString
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:
This allows MCP clients to validate tool responses against the defined schema and provides better type safety and documentation for tool interactions.
Motivation and Context
This was raised as an issue for the output schema to be added
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context