-
Notifications
You must be signed in to change notification settings - Fork 442
fix(tool): remove unnecessary schema validation #375
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
fix(tool): remove unnecessary schema validation #375
Conversation
|
@ccamel55 please review this. I didn't notice the problem when I merge the pr. |
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 removes unnecessary JSON schema validation from the tool system and introduces a type-safe conversion method for structured outputs. The changes eliminate redundant validation since Rust's deserialization already provides type safety, and replace it with a more idiomatic approach.
Key changes:
- Removes schema validation functions and related validation logic
- Adds
into_typed()method toCallToolResultfor type-safe conversion of structured content - Updates tests to use the new type-safe approach instead of validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/rmcp/src/model.rs |
Replaces validation method with into_typed() for type-safe conversion and simplifies deserialization validation |
crates/rmcp/src/handler/server/tool.rs |
Removes schema validation functions (validate_against_schema and get_json_value_type) |
crates/rmcp/src/handler/server/router/tool.rs |
Removes schema validation logic from tool call execution |
crates/rmcp/tests/test_structured_output.rs |
Updates test to use new into_typed() method instead of validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if let Some(text) = raw_text { | ||
| return serde_json::from_str(text); | ||
| } | ||
| serde_json::from_value(serde_json::Value::Null) |
Copilot
AI
Aug 14, 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.
This line will always attempt to deserialize null into type T, which will likely fail for most types. Consider returning an error indicating that no structured content or valid text content was found instead of trying to deserialize null.
| serde_json::from_value(serde_json::Value::Null) | |
| Err(serde_json::Error::custom("No structured content or valid text content found")) |
| (None, Some(contents)) => { | ||
| if let Some(text) = contents.first().and_then(|c| c.as_text()) { | ||
| let text = &text.text; | ||
| Some(text) |
Copilot
AI
Aug 14, 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.
[nitpick] The logic for extracting content could be simplified. The current implementation checks for text content only in the first element of the content array. Consider making this behavior more explicit or handling multiple content items if that's the intended behavior.
| Some(text) | |
| // Collect all text content from the vector | |
| let texts: Vec<&str> = contents.iter() | |
| .filter_map(|c| c.as_text().map(|t| t.text.as_str())) | |
| .collect(); | |
| if !texts.is_empty() { | |
| // Concatenate all text content with a space separator | |
| Some(texts.join(" ")) |
| let raw_text = match (self.structured_content, &self.content) { | ||
| (Some(value), _) => return serde_json::from_value(value), | ||
| (None, Some(contents)) => { | ||
| if let Some(text) = contents.first().and_then(|c| c.as_text()) { |
Copilot
AI
Aug 14, 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 method only considers the first content item when extracting text. This behavior should be documented in the method's docstring to clarify that only the first text content will be used for deserialization.
ccamel55
left a comment
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.
LGTM but wouldn't this technically be a breaking change since it removes public facing API? I'll leave it to the maintainers to to decide though 😄
|
@ccamel55 It's ok to introduce breaking changes, we haven't release the deleted api. |
jokemanfire
left a comment
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.
It can be Deserialized , so it is structed json data, If I understand it ?
Motivation and Context
We don't really need to do the validation in rust, because deserilization already is a validation. And if user want to do the validation, they can use the jsonschame crate.
And also, the original validation method is a wrong implementation, as I mentioned in #374
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context