-
Notifications
You must be signed in to change notification settings - Fork 79
[Server] Structured content return and tool error #93
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
[Server] Structured content return and tool error #93
Conversation
f340e0d to
cfbfff8
Compare
cfbfff8 to
1f4ccdf
Compare
src/Capability/Tool/ToolCaller.php
Outdated
| ]); | ||
|
|
||
| return new CallToolResult($formattedResult); | ||
| return CallToolResult::fromArray($formattedResult); |
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'm not fond of this change, I think it'd be better to have some kind of Result that has both contents and structured content:
- we could still return a collection of content's (and add structured content if the result has some)
- at some point we will probably only return structured content.
For example:
StructuredContentResult {
public 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.
You mean something like
StructuredContentResult implements CallToolResult {
public StructuredContent
public array $contents
}
ContentResult implements CallToolResult {
public array $contents
}
interface CallToolResult extends \JsonSerializable{
}
Then we can expect instance of CallToolResult interface as return and maybe skip
/** @var array<int, TextContent|ImageContent|EmbeddedResource|AudioContent|StructuredContent> $formattedResult */
$formattedResult = $toolReference->formatResult($result);and expect user to return proper response from tool?
at some point we will probably only return structured content.
Structured content is not required while outputSchema is not set, and I can argue that for some tasks text response works better.
At least for now content response required even if structuredContent is set.
This implementation is far from best, so if you could steer me in right direction would be great.
I used it in my browser-mcp tools, most crucial part was ability to return proper tool usage errors with hints to help model correct tool calls. Structured content provided no improvements for tool calling and quality so I just removed it from final version.
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'm using this:
<?php
namespace ApiPlatform\Mcp\Schema\Result;
use Mcp\Schema\JsonRpc\ResultInterface;
class StructuredContentResult implements ResultInterface
{
/**
* Create a new StructuredContentResult.
*
* @param array $structuredContent The JSON content
* @param ResultInterface $result A traditional result
*/
public function __construct(
public array $structuredContent,
public readonly ?ResultInterface $result = null
) {
}
public function jsonSerialize(): mixed
{
return ['structuredContent' => $this->structuredContent] + ($this->result?->jsonSerialize() ?? []);
}
}This is merely a suggestion :).
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 changed implementation, removed some changes.
Now we have CallToolResultInterface and 2 classes implementing it:
CallToolResultas our general result classCallToolStructuredContentResultas SturcturedContent result which pretty much decoratesCallToolResult
Tested on my Browser MCP it's working as expected.
Also added Unit tests to cover behavior.
As for example of usage, maybe we can add one after #88 merged as they go hand by hand together.
@chr-hertel @CodeWithKyrian could you take a look when will get time, it's ready for proper CR now.
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'm a bit confused - the PR description doesn't match the code or do I miss 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.
I like the idea of enabling tools to return the CallToolResult optionally if they want to have more control, but let's stick with one implementation here.
If we patch it in the lib we don't need an additional decoration, but can extend the CallToolResult with an optional structure argument, that is omitted in case it's empty. That would be also more consistent with what we have in other data classes and makes it easier in change scenarios. currently spec changes are only additive for example and users relying on using those classes would need to 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'm a bit confused - the PR description doesn't match the code or do I miss something?
Yeah, I changed implementation a lot after previous comment and did rework, I can update it.
Comments here describe main idea more.
If we patch it in the lib we don't need an additional decoration, but can extend the
CallToolResultwith an optional structure argument, that is omitted in case it's empty. That would be also more consistent with what we have in other data classes and makes it easier in change scenarios. currently spec changes are only additive for example and users relying on using those classes would need to change.
Okay, gonna rework to keep single CallToolResult with optional structuredContent.
I like the idea of enabling tools to return the
CallToolResultoptionally if they want to have more control, but let's stick with one implementation here.
That was actually even more beneficial for me to return proper isError to LLM in response with hints and error explanation so it could do correction, if isError=false always returned as it is now LLM don't always understand what happened and that it needs to correct tool call.
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 Moved structuredContent to CallToolResult. Fixed description. Updated unit tests accordingly. Tested responses in inspector, works as expected.
619034f to
538549f
Compare
cdff778 to
e20862f
Compare
8e3f9c7 to
6da70b8
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.
Step by step - thanks @ineersa! :)
Added optional
structuredContentforCallToolResultwhich enables return of structured content when we provide output schema for a tool.Added ability to return
CallToolResultfrom tool directly, which enables proper return of tool errors to allow LLM to self-correct tool call.Motivation and Context
While testing my MCP server I've noticed inconsistencies between output with Python MCP server.
structuredContentisError=trueAlso while
outputSchemabeing added in #88 , withoutputSchemaMCP server MUST provide structured content alongside text representation.How Has This Been Tested?
Tested on my MCP server to ensure same response between PHP and Python versions.
Added unit tests.
Breaking Changes
No breaking changes
Types of changes
Checklist
Additional context