generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 451
fix(mcp): add meta field to MCPToolResult for MCP _meta support #885
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hi, in the current state, we are going to need to do something similar to https://github.com/strands-agents/sdk-python/pull/818/files since if we try to pass _meta to certain models we will get a validation exception.
After #836 this may change. So let's wait until 836 is merged and then I'll re review.
Uh oh!
There was an error while loading. Please reload this page.
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.
So #818 has since been merged as has #836.
I want to understand your needs though.
I see that you stated the following, indicating that you are aware that MCPToolResult has optional fields like
structuredContent.Does adding meta to MCPToolResult not satisfy your needs? I could entertain meta being added more generally to the base
ToolResultbut I'm trying to understand the desire for meta to be a ContentType.This comes down to the following question: Is meta used by the application, in things like Hooks or other AgentTools, or do we want this exposed to the LLM?
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.
Additionally, our ContentType is a discriminated union. We could change that with a meta special case - would need some convincing. But lets chat more about your needs and we can work back from that
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.
Here’s the situation: I found that the official MCP’s
CallToolResultcontent is a list, where each item is aTextContentobject that contains a_metafield. This indicates that the tool results in MCP can be a list, and each item in the list may include its own meta information. So my idea is to implement it according to the MCP protocol.Putting it into MCPToolResult would work, but I think it should be a list:
NotRequired[List[Dict[str, Any]]].I don’t think this should be exposed to the LLM, since it’s an MCP reserved field 🤔 meant to carry business parameters like
token_usage. I believe a hook can carry such business-specific information in a non-intrusive way, so there’s no need for the LLM to be aware of it.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.
@dbschmigelski I’d like to know the latest updates about this _meta. I think my idea aligns with the MCP standards.
Uh oh!
There was an error while loading. Please reload this page.
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 believe my business needs have already been met, because
CallToolResultalso has ametafield.The meta field in CallToolResult corresponds to the one you added: