Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions crates/rmcp/src/handler/server/router/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use schemars::JsonSchema;
use crate::{
handler::server::tool::{
CallToolHandler, DynCallToolHandler, ToolCallContext, schema_for_type,
validate_against_schema,
},
model::{CallToolResult, Tool, ToolAnnotations},
};
Expand Down Expand Up @@ -246,19 +245,6 @@ where

let result = (item.call)(context).await?;

// Validate structured content against output schema if present
if let Some(ref output_schema) = item.attr.output_schema {
// When output_schema is defined, structured_content is required
if result.structured_content.is_none() {
return Err(crate::ErrorData::invalid_params(
"Tool with output_schema must return structured_content",
None,
));
}
// Validate the structured content against the schema
validate_against_schema(result.structured_content.as_ref().unwrap(), output_schema)?;
}

Ok(result)
}

Expand Down
37 changes: 0 additions & 37 deletions crates/rmcp/src/handler/server/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,43 +67,6 @@ pub fn schema_for_type<T: JsonSchema>() -> JsonObject {
}
}

/// Validate that a JSON value conforms to basic type constraints from a schema.
///
/// Note: This is a basic validation that only checks type compatibility.
/// For full JSON Schema validation, a dedicated validation library would be needed.
pub fn validate_against_schema(
value: &serde_json::Value,
schema: &JsonObject,
) -> Result<(), crate::ErrorData> {
// Basic type validation
if let Some(schema_type) = schema.get("type").and_then(|t| t.as_str()) {
let value_type = get_json_value_type(value);

if schema_type != value_type {
return Err(crate::ErrorData::invalid_params(
format!(
"Value type does not match schema. Expected '{}', got '{}'",
schema_type, value_type
),
None,
));
}
}

Ok(())
}

fn get_json_value_type(value: &serde_json::Value) -> &'static str {
match value {
serde_json::Value::Null => "null",
serde_json::Value::Bool(_) => "boolean",
serde_json::Value::Number(_) => "number",
serde_json::Value::String(_) => "string",
serde_json::Value::Array(_) => "array",
serde_json::Value::Object(_) => "object",
}
}

/// Call [`schema_for_type`] with a cache
pub fn cached_schema_for_type<T: JsonSchema + std::any::Any>() -> Arc<JsonObject> {
thread_local! {
Expand Down
38 changes: 31 additions & 7 deletions crates/rmcp/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub use extension::*;
pub use meta::*;
pub use prompt::*;
pub use resource::*;
use serde::{Deserialize, Serialize};
use serde::{Deserialize, Serialize, de::DeserializeOwned};
use serde_json::Value;
pub use tool::*;

Expand Down Expand Up @@ -1260,12 +1260,32 @@ impl CallToolResult {
}
}

/// Validate that content or structured content is provided
pub fn validate(&self) -> Result<(), &'static str> {
match (&self.content, &self.structured_content) {
(None, None) => Err("either content or structured_content must be provided"),
_ => Ok(()),
/// Convert the `structured_content` part of response into a certain type.
///
/// # About json schema validation
/// Since rust is a strong type language, we don't need to do json schema validation here.
///
/// But if you do have to validate the response data, you can use [`jsonschema`](https://crates.io/crates/jsonschema) crate.
pub fn into_typed<T>(self) -> Result<T, serde_json::Error>
where
T: DeserializeOwned,
{
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()) {
Copy link

Copilot AI Aug 14, 2025

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.

Copilot uses AI. Check for mistakes.
let text = &text.text;
Some(text)
Copy link

Copilot AI Aug 14, 2025

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.

Suggested change
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(" "))

Copilot uses AI. Check for mistakes.
} else {
None
}
}
(None, None) => None,
};
if let Some(text) = raw_text {
return serde_json::from_str(text);
}
serde_json::from_value(serde_json::Value::Null)
Copy link

Copilot AI Aug 14, 2025

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.

Suggested change
serde_json::from_value(serde_json::Value::Null)
Err(serde_json::Error::custom("No structured content or valid text content found"))

Copilot uses AI. Check for mistakes.
}
}

Expand Down Expand Up @@ -1294,7 +1314,11 @@ impl<'de> Deserialize<'de> for CallToolResult {
};

// Validate mutual exclusivity
result.validate().map_err(serde::de::Error::custom)?;
if result.content.is_none() && result.structured_content.is_none() {
return Err(serde::de::Error::custom(
"CallToolResult must have either content or structured_content",
));
}

Ok(result)
}
Expand Down
17 changes: 14 additions & 3 deletions crates/rmcp/tests/test_structured_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,24 @@ async fn test_structured_error_in_call_result() {

#[tokio::test]
async fn test_mutual_exclusivity_validation() {
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
pub struct Response {
message: String,
}
let response = Response {
message: "Hello".into(),
};
// Test that content and structured_content can both be passed separately
let content_result = CallToolResult::success(vec![Content::text("Hello")]);
let content_result = CallToolResult::success(vec![Content::json(response.clone()).unwrap()]);
let structured_result = CallToolResult::structured(json!({"message": "Hello"}));

// Verify the validation
assert!(content_result.validate().is_ok());
assert!(structured_result.validate().is_ok());
content_result
.into_typed::<Response>()
.expect("Failed to extract content");
structured_result
.into_typed::<Response>()
.expect("Failed to extract content");

// Try to create a result with both fields
let json_with_both = json!({
Expand Down