-
Notifications
You must be signed in to change notification settings - Fork 442
Match the shape of CallToolResult schema
#377
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
Match the shape of CallToolResult schema
#377
Conversation
e7b34e8 to
5102dfd
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.
Pull Request Overview
This PR modifies the CallToolResult schema to make the content field mandatory and always present, aligning with the MCP (Model Context Protocol) schema specification. The change ensures compatibility with MCP implementations that expect the content field to always exist.
Key changes:
- Changed
contentfield fromOption<Vec<Content>>toVec<Content>in theCallToolResultstruct - Updated JSON schema to make
contenta required field and remove null type - Updated all test code to work with the non-optional
contentfield
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/rmcp/src/model.rs | Modified CallToolResult struct to make content field mandatory and updated related methods |
| crates/rmcp/tests/test_message_schema/server_json_rpc_message_schema.json | Updated JSON schema to make content required and remove null type |
| crates/rmcp/tests/test_tool_macros.rs | Updated test code to handle non-optional content field |
| crates/rmcp/tests/test_structured_output.rs | Updated test assertions and content access for mandatory content field |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| (Some(value), _) => return serde_json::from_value(value), | ||
| (None, Some(contents)) => { | ||
| if let Some(text) = contents.first().and_then(|c| c.as_text()) { | ||
| if let Some(text) = contents.as_text() { |
Copilot
AI
Aug 18, 2025
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 pattern match (None, Some(contents)) is unreachable because self.content is now a Vec<Content> (not Option<Vec<Content>>), so it can never be Some(contents). This should be (None, contents) and the condition should check if contents is not empty.
| (Some(value), _) => return serde_json::from_value(value), | ||
| (None, Some(contents)) => { | ||
| if let Some(text) = contents.first().and_then(|c| c.as_text()) { | ||
| if let Some(text) = contents.as_text() { |
Copilot
AI
Aug 18, 2025
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 variable contents is now of type &Content (from first()) rather than &Vec<Content>, so calling contents.as_text() directly is incorrect. This should be accessing the first content item's text method.
|
fix ci please |
BREAKING CHANGE: makes the `content` field non-optional
e1f79f2 to
38e42a7
Compare
|
thanks |
Adjusts the
contentfield to always be presentMotivation and Context
The MCP schema has the
contentfield as mandatory: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/3b3874b4fc010f34ef5b106f044a4867534a9499/schema/2025-06-18/schema.ts#L782Some of the MCP implementations (I.e, zed editor) assume (& rightly so) that this field is always present.
How Has This Been Tested?
Running tests
Breaking Changes
It is essentially a breaking change on the type
CallToolResultTypes of changes
Checklist
Additional context