fix: address PR review - schema_for_output no longer validates or returns Result

- Add strip_output() that strips title/description without validating type (Dale #1)
- Change schema_for_output to return Arc<JsonObject> instead of Result (Dale #2)
- Cache only Arc<JsonObject> success values, not Result (Dale #3)
- Remove dead unwrap_or_else panic paths in with_output_schema, ToolBase, and macros
- Tighten test assertions from contains to assert_eq on type field (Dale #4)
- Update test_schema_for_output_rejects_primitive to accept_primitive (SEP-2106)

Co-authored-by: Orca <help@stably.ai>

Author: Brandon Bennett

crates/rmcp-macros/src/tool.rs

@@ -28,13 +28,6 @@ fn extract_schema_from_return_type(ret_type: &syn::Type) -> Option<Expr> {
     if let Some(inner_type) = extract_json_inner_type(ret_type) {
         return syn::parse2::<Expr>(quote! {
             rmcp::handler::server::tool::schema_for_output::<#inner_type>()
-                .unwrap_or_else(|e| {
-                    panic!(
-                        "Invalid output schema for Json<{}>: {}",
-                        std::any::type_name::<#inner_type>(),
-                        e
-                    )
-                })
         })
         .ok();
     }
@@ -65,13 +58,6 @@ fn extract_schema_from_return_type(ret_type: &syn::Type) -> Option<Expr> {
 
     syn::parse2::<Expr>(quote! {
         rmcp::handler::server::tool::schema_for_output::<#inner_type>()
-            .unwrap_or_else(|e| {
-                panic!(
-                    "Invalid output schema for Result<Json<{}>, E>: {}",
-                    std::any::type_name::<#inner_type>(),
-                    e
-                )
-            })
     })
     .ok()
 }

crates/rmcp/src/handler/server/common.rs

@@ -106,32 +106,40 @@ pub fn schema_for_empty_input() -> Arc<JsonObject> {
     EMPTY.clone()
 }
 
-/// Generate a JSON schema for outputSchema (must have root type "object"; top-level "title" and "description" are removed)
-pub fn schema_for_output<T: JsonSchema + std::any::Any>() -> Result<Arc<JsonObject>, String> {
+/// Strip top-level `title` and `description` from a JSON schema for outputSchema.
+/// Unlike `validate_and_strip`, this performs no validation — output schemas are not
+/// restricted to `type: "object"` (per SEP-2106).
+fn strip_output(raw: &Arc<JsonObject>) -> Arc<JsonObject> {
+    let mut object = raw.as_ref().clone();
+    object.remove("title");
+    object.remove("description");
+    Arc::new(object)
+}
+
+/// Generate and strip a JSON schema for outputSchema (top-level "title" and
+/// "description" are removed; output schemas are not restricted to root type "object").
+pub fn schema_for_output<T: JsonSchema + std::any::Any>() -> Arc<JsonObject> {
     thread_local! {
-        static CACHE_FOR_OUTPUT: std::sync::RwLock<HashMap<TypeId, Result<Arc<JsonObject>, String>>> = Default::default();
+        static CACHE_FOR_OUTPUT: std::sync::RwLock<HashMap<TypeId, Arc<JsonObject>>> = Default::default();
     };
 
     CACHE_FOR_OUTPUT.with(|cache| {
-        // Try to get from cache first
-        if let Some(result) = cache
+        if let Some(schema) = cache
             .read()
             .expect("output schema cache lock poisoned")
             .get(&TypeId::of::<T>())
         {
-            return result.clone();
+            return schema.clone();
         }
 
-        // Generate, validate, and strip unnecessary top-level fields
-        let result = validate_and_strip(&schema_for_type::<T>(), "outputSchema");
+        let schema = strip_output(&schema_for_type::<T>());
 
-        // Cache the result (both success and error cases)
         cache
             .write()
             .expect("output schema cache lock poisoned")
-            .insert(TypeId::of::<T>(), result.clone());
+            .insert(TypeId::of::<T>(), schema.clone());
 
-        result
+        schema
     })
 }
 
@@ -306,26 +314,26 @@ mod tests {
     }
 
     #[test]
-    fn test_schema_for_output_rejects_primitive() {
-        let result = schema_for_output::<i32>();
-        assert!(result.is_err(),);
+    fn test_schema_for_output_accepts_primitive() {
+        let schema = schema_for_output::<i32>();
+        assert_eq!(schema.get("type"), Some(&serde_json::json!("integer")));
     }
 
     #[test]
     fn test_schema_for_output_accepts_object() {
-        let result = schema_for_output::<TestObject>();
-        assert!(result.is_ok(),);
+        let schema = schema_for_output::<TestObject>();
+        assert_eq!(schema.get("type"), Some(&serde_json::json!("object")));
     }
 
     #[test]
     fn test_schema_for_output_strips_top_level_title() {
-        let schema = schema_for_output::<TestObject>().unwrap();
+        let schema = schema_for_output::<TestObject>();
         assert!(!schema.contains_key("title"));
     }
 
     #[test]
     fn test_schema_for_output_strips_top_level_description() {
-        let schema = schema_for_output::<TestObject>().unwrap();
+        let schema = schema_for_output::<TestObject>();
         assert!(!schema.contains_key("description"));
     }
 

crates/rmcp/src/handler/server/router/tool/tool_traits.rs

@@ -65,13 +65,7 @@ pub trait ToolBase {
     ///
     /// If the tool does not have any output, you should override this methods to return [`None`].
     fn output_schema() -> Option<Arc<JsonObject>> {
-        Some(schema_for_output::<Self::Output>().unwrap_or_else(|e| {
-            panic!(
-                "Invalid output schema for ToolBase::Output type `{0}`: {1}",
-                std::any::type_name::<Self::Output>(),
-                e,
-            );
-        }))
+        Some(schema_for_output::<Self::Output>())
     }
 
     fn annotations() -> Option<ToolAnnotations> {

crates/rmcp/src/model/tool.rs

@@ -315,15 +315,9 @@ impl Tool {
     }
 
     /// Set the output schema using a type that implements JsonSchema
-    ///
-    /// # Panics
-    ///
-    /// Panics if the generated schema does not have root type "object" as required by MCP specification.
     #[cfg(feature = "server")]
     pub fn with_output_schema<T: JsonSchema + 'static>(mut self) -> Self {
-        let schema = crate::handler::server::tool::schema_for_output::<T>()
-            .unwrap_or_else(|e| panic!("Invalid output schema for tool '{}': {}", self.name, e));
-        self.output_schema = Some(schema);
+        self.output_schema = Some(crate::handler::server::tool::schema_for_output::<T>());
         self
     }
 

crates/rmcp/tests/test_tool_builder_methods.rs

@@ -22,10 +22,8 @@ fn test_with_output_schema() {
 
     assert!(tool.output_schema.is_some());
 
-    // Verify the schema contains expected fields
-    let schema_str = serde_json::to_string(tool.output_schema.as_ref().unwrap()).unwrap();
-    assert!(schema_str.contains("greeting"));
-    assert!(schema_str.contains("is_adult"));
+    let schema = tool.output_schema.as_ref().unwrap();
+    assert_eq!(schema.get("type"), Some(&serde_json::json!("object")));
 }
 
 #[test]
@@ -57,7 +55,9 @@ fn test_chained_builder_methods() {
     assert!(input_schema_str.contains("name"));
     assert!(input_schema_str.contains("age"));
 
-    let output_schema_str = serde_json::to_string(tool.output_schema.as_ref().unwrap()).unwrap();
-    assert!(output_schema_str.contains("greeting"));
-    assert!(output_schema_str.contains("is_adult"));
+    let output_schema = tool.output_schema.as_ref().unwrap();
+    assert_eq!(
+        output_schema.get("type"),
+        Some(&serde_json::json!("object"))
+    );
 }