From f79215c14afa23a6d1a75b544b0f80442efa58c2 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sat, 11 Oct 2025 07:46:45 +0200 Subject: [PATCH] fix(mcp-macros): use JSON serialization instead of Debug format in tool returns Fixes #62 Changes: - Modified utils.rs generate_error_handling() to use serde_json::to_string() instead of format!("{:?}") for tool return values - Now populates structured_content field per MCP 2025-06-18 specification - Graceful fallback to Debug format for types without Serialize trait - Added comprehensive test suite (8 tests) in json_serialization_test.rs Breaking Change: - Tool return types should implement Serialize trait for optimal JSON output - Types without Serialize will fallback to Debug format Before: "SearchResult { items: [...] }" After: {"items": [...]} Version bumped to 0.13.0 due to breaking change requirement. --- .claude/settings.local.json | 3 +- CHANGELOG.md | 70 ++++ Cargo.lock | 28 +- Cargo.toml | 26 +- mcp-macros/src/utils.rs | 59 ++- mcp-macros/tests/json_serialization_test.rs | 436 ++++++++++++++++++++ 6 files changed, 581 insertions(+), 41 deletions(-) create mode 100644 mcp-macros/tests/json_serialization_test.rs diff --git a/.claude/settings.local.json b/.claude/settings.local.json index a7d9248e..a3e65bb1 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -49,7 +49,8 @@ "Bash(gh release list:*)", "Bash(gh release view:*)", "Bash(gh pr list:*)", - "Bash(pre-commit:*)" + "Bash(pre-commit:*)", + "WebFetch(domain:spec.modelcontextprotocol.io)" ], "deny": [] } diff --git a/CHANGELOG.md b/CHANGELOG.md index 72dbce85..1de32fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,76 @@ All notable changes to the PulseEngine MCP Framework will be documented in this The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.13.0] - 2025-01-11 + +### Fixed + +#### mcp-macros + +- **[BREAKING] Fixed `#[mcp_tools]` macro to use JSON serialization instead of Rust Debug format** ([#62](https://github.com/pulseengine/mcp/issues/62)) + - Tool return values are now properly serialized as JSON using `serde_json::to_string()` + - Populates `structured_content` field in `CallToolResult` per MCP 2025-06-18 specification + - Graceful fallback to Debug format for types that don't implement `Serialize` + - **Breaking Change**: Tool return types should implement `Serialize` trait for optimal JSON output + - Previously returned Rust Debug format like `SearchResult { items: [...] }` which broke JSON parsing + - Now returns proper JSON: `{"items": [...]}` + +### Added + +#### Testing + +- **Comprehensive JSON serialization test suite** (`mcp-macros/tests/json_serialization_test.rs`) + - Tests for structured return types (nested structs, vectors, enums) + - Tests for Result return types + - Tests for simple types (string, number, bool, vector) + - Verification that `structured_content` field is populated + - Verification that Debug format markers are not present in output + - 8 comprehensive test cases covering all scenarios + +### Changed + +- **Version bumped to 0.13.0** (breaking change due to Serialize requirement) +- Tool responses now comply with MCP 2025-06-18 specification for structured content + +### Migration Guide + +If you have tools that return structured types: + +```rust +// Add Serialize to your return types +#[derive(Debug, Serialize)] // Add Serialize +struct MyResult { + data: Vec, +} + +#[mcp_tools] +impl MyServer { + pub fn my_tool(&self) -> MyResult { + MyResult { data: vec!["item1".to_string()] } + } +} +``` + +For types that can't implement Serialize, the macro will gracefully fall back to Debug format. + +## [0.12.0] - 2025-01-11 + +### Added + +- **MCP 2025-06-18 protocol support** + - `NumberOrString` type for request IDs + - Optional `_meta` fields across protocol types + +### Fixed + +- **Fixed flaky tests** with `serial_test` crate + - All environment variable tests now run serially to prevent race conditions + - Added `#[serial_test::serial]` to tests in mcp-security-middleware, mcp-cli-derive, and mcp-cli + +### Changed + +- CI now validates all changes with pre-commit hooks + ## [0.4.1] - 2024-07-06 ### Added diff --git a/Cargo.lock b/Cargo.lock index 2cb7ead8..6ac28c22 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1185,7 +1185,7 @@ dependencies = [ [[package]] name = "hello-world-with-auth" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "async-trait", @@ -2297,7 +2297,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-auth" -version = "0.12.0" +version = "0.13.0" dependencies = [ "aes-gcm", "anyhow", @@ -2336,7 +2336,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-cli" -version = "0.12.0" +version = "0.13.0" dependencies = [ "clap", "pulseengine-mcp-cli-derive", @@ -2356,7 +2356,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-cli-derive" -version = "0.12.0" +version = "0.13.0" dependencies = [ "async-trait", "clap", @@ -2375,7 +2375,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-external-validation" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "arbitrary", @@ -2413,7 +2413,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-integration-tests" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "assert_matches", @@ -2441,7 +2441,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-logging" -version = "0.12.0" +version = "0.13.0" dependencies = [ "chrono", "hex", @@ -2460,7 +2460,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-macros" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "async-trait", @@ -2486,7 +2486,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-monitoring" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "chrono", @@ -2506,7 +2506,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-protocol" -version = "0.12.0" +version = "0.13.0" dependencies = [ "async-trait", "chrono", @@ -2523,7 +2523,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-security" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "async-trait", @@ -2545,7 +2545,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-security-middleware" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "assert_matches", @@ -2578,7 +2578,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-server" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "async-trait", @@ -2606,7 +2606,7 @@ dependencies = [ [[package]] name = "pulseengine-mcp-transport" -version = "0.12.0" +version = "0.13.0" dependencies = [ "anyhow", "async-stream", diff --git a/Cargo.toml b/Cargo.toml index bc5d2a29..e50a008a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,7 +30,7 @@ members = [ resolver = "2" [workspace.package] -version = "0.12.0" +version = "0.13.0" rust-version = "1.88" edition = "2024" license = "MIT OR Apache-2.0" @@ -108,18 +108,18 @@ assert_matches = "1.5" serde_yaml = "0.9" # Framework internal dependencies (published versions) -pulseengine-mcp-protocol = { version = "0.12.0", path = "mcp-protocol" } -pulseengine-mcp-logging = { version = "0.12.0", path = "mcp-logging" } -pulseengine-mcp-auth = { version = "0.12.0", path = "mcp-auth" } -pulseengine-mcp-security = { version = "0.12.0", path = "mcp-security" } -pulseengine-mcp-security-middleware = { version = "0.12.0", path = "mcp-security-middleware" } -pulseengine-mcp-monitoring = { version = "0.12.0", path = "mcp-monitoring" } -pulseengine-mcp-transport = { version = "0.12.0", path = "mcp-transport" } -pulseengine-mcp-cli = { version = "0.12.0", path = "mcp-cli" } -pulseengine-mcp-cli-derive = { version = "0.12.0", path = "mcp-cli-derive" } -pulseengine-mcp-server = { version = "0.12.0", path = "mcp-server" } -pulseengine-mcp-macros = { version = "0.12.0", path = "mcp-macros" } -pulseengine-mcp-external-validation = { version = "0.12.0", path = "mcp-external-validation" } +pulseengine-mcp-protocol = { version = "0.13.0", path = "mcp-protocol" } +pulseengine-mcp-logging = { version = "0.13.0", path = "mcp-logging" } +pulseengine-mcp-auth = { version = "0.13.0", path = "mcp-auth" } +pulseengine-mcp-security = { version = "0.13.0", path = "mcp-security" } +pulseengine-mcp-security-middleware = { version = "0.13.0", path = "mcp-security-middleware" } +pulseengine-mcp-monitoring = { version = "0.13.0", path = "mcp-monitoring" } +pulseengine-mcp-transport = { version = "0.13.0", path = "mcp-transport" } +pulseengine-mcp-cli = { version = "0.13.0", path = "mcp-cli" } +pulseengine-mcp-cli-derive = { version = "0.13.0", path = "mcp-cli-derive" } +pulseengine-mcp-server = { version = "0.13.0", path = "mcp-server" } +pulseengine-mcp-macros = { version = "0.13.0", path = "mcp-macros" } +pulseengine-mcp-external-validation = { version = "0.13.0", path = "mcp-external-validation" } [profile.release] opt-level = "s" diff --git a/mcp-macros/src/utils.rs b/mcp-macros/src/utils.rs index da7d17e4..558bc36d 100644 --- a/mcp-macros/src/utils.rs +++ b/mcp-macros/src/utils.rs @@ -158,14 +158,31 @@ pub fn generate_error_handling(return_type: &syn::ReturnType) -> TokenStream { if let Some(segment) = type_path.path.segments.last() { if segment.ident == "Result" { // It's already a Result, wrap it properly for the dispatch context + // Use JSON serialization for structured data, with fallback return quote! { match result { - Ok(value) => Ok(pulseengine_mcp_protocol::CallToolResult { - content: vec![pulseengine_mcp_protocol::Content::text(format!("{:?}", value))], - is_error: Some(false), - structured_content: None, - _meta: None, - }), + Ok(value) => { + // Try JSON serialization first (for structured data) + let (text_content, structured) = match serde_json::to_value(&value) { + Ok(json_value) => { + // Serialize as JSON string for text content + let text = serde_json::to_string(&value) + .unwrap_or_else(|_| format!("{:?}", value)); + (text, Some(json_value)) + } + Err(_) => { + // Fallback to Debug if not serializable + (format!("{:?}", value), None) + } + }; + + Ok(pulseengine_mcp_protocol::CallToolResult { + content: vec![pulseengine_mcp_protocol::Content::text(text_content)], + is_error: Some(false), + structured_content: structured, + _meta: None, + }) + } Err(e) => Err(pulseengine_mcp_protocol::Error::internal_error(e.to_string())), } }; @@ -173,14 +190,30 @@ pub fn generate_error_handling(return_type: &syn::ReturnType) -> TokenStream { } } - // Not a Result, wrap it with simple Display formatting + // Not a Result, wrap it with JSON serialization (preferred) or Display formatting quote! { - Ok(pulseengine_mcp_protocol::CallToolResult { - content: vec![pulseengine_mcp_protocol::Content::text(result.to_string())], - is_error: Some(false), - structured_content: None, - _meta: None, - }) + { + // Try JSON serialization first (for structured data) + let (text_content, structured) = match serde_json::to_value(&result) { + Ok(json_value) => { + // Serialize as JSON string for text content + let text = serde_json::to_string(&result) + .unwrap_or_else(|_| format!("{:?}", result)); + (text, Some(json_value)) + } + Err(_) => { + // Fallback to Debug if not serializable + (format!("{:?}", result), None) + } + }; + + Ok(pulseengine_mcp_protocol::CallToolResult { + content: vec![pulseengine_mcp_protocol::Content::text(text_content)], + is_error: Some(false), + structured_content: structured, + _meta: None, + }) + } } } } diff --git a/mcp-macros/tests/json_serialization_test.rs b/mcp-macros/tests/json_serialization_test.rs new file mode 100644 index 00000000..6cd062bc --- /dev/null +++ b/mcp-macros/tests/json_serialization_test.rs @@ -0,0 +1,436 @@ +//! Tests for JSON serialization of tool return values +//! +//! This test suite verifies that the #[mcp_tools] macro correctly serializes +//! structured return types as JSON instead of using Rust's Debug format. +//! +//! Addresses: https://github.com/pulseengine/mcp/issues/62 + +#![allow(dead_code)] + +use pulseengine_mcp_macros::{mcp_server, mcp_tools}; +use pulseengine_mcp_protocol::{CallToolRequestParam, Content}; +use pulseengine_mcp_server::McpServerBuilder; +use serde::{Deserialize, Serialize}; +use serde_json::json; + +/// Complex structured type for testing JSON serialization +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct SearchResult { + pub total_count: u32, + pub items: Vec, + pub has_more: bool, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct SearchItem { + pub id: u64, + pub title: String, + pub description: Option, + pub tags: Vec, +} + +/// Nested structured type +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct IssueSearchResult { + pub search_result: SearchResult, + pub query: String, + pub duration_ms: u64, +} + +/// Server with tools returning structured data +#[mcp_server(name = "JSON Serialization Test Server")] +#[derive(Clone, Default)] +struct JsonTestServer; + +#[mcp_tools] +impl JsonTestServer { + /// Tool returning a simple structured type + pub fn get_search_result(&self) -> SearchResult { + SearchResult { + total_count: 2, + items: vec![ + SearchItem { + id: 1, + title: "First Item".to_string(), + description: Some("Description of first item".to_string()), + tags: vec!["tag1".to_string(), "tag2".to_string()], + }, + SearchItem { + id: 2, + title: "Second Item".to_string(), + description: None, + tags: vec!["tag3".to_string()], + }, + ], + has_more: true, + } + } + + /// Tool returning a nested structured type + pub fn search_issues(&self, query: String) -> IssueSearchResult { + IssueSearchResult { + search_result: SearchResult { + total_count: 1, + items: vec![SearchItem { + id: 42, + title: format!("Issue matching '{query}'"), + description: Some("This is a test issue".to_string()), + tags: vec!["bug".to_string(), "high-priority".to_string()], + }], + has_more: false, + }, + query: query.clone(), + duration_ms: 150, + } + } + + /// Tool returning Result + pub fn get_result_type(&self, should_succeed: bool) -> Result { + if should_succeed { + Ok(SearchResult { + total_count: 1, + items: vec![SearchItem { + id: 999, + title: "Result type item".to_string(), + description: Some("From Result".to_string()), + tags: vec![], + }], + has_more: false, + }) + } else { + Err("Intentional failure".to_string()) + } + } + + /// Tool returning a simple string (should not break) + pub fn simple_string(&self) -> String { + "Simple text response".to_string() + } + + /// Tool returning a number (should serialize as JSON number) + pub fn simple_number(&self) -> i32 { + 42 + } + + /// Tool returning a bool + pub fn simple_bool(&self) -> bool { + true + } + + /// Tool returning a vector of strings + pub fn string_vector(&self) -> Vec { + vec![ + "item1".to_string(), + "item2".to_string(), + "item3".to_string(), + ] + } +} + +#[tokio::test] +async fn test_structured_return_serialization() { + let server = JsonTestServer::with_defaults(); + + // Call tool that returns structured data + let request = CallToolRequestParam { + name: "get_search_result".to_string(), + arguments: None, + }; + + let result = server + .call_tool(request) + .await + .expect("Tool should succeed"); + + // Verify the result + assert_eq!(result.is_error, Some(false)); + assert_eq!(result.content.len(), 1); + + // Extract the text content + let text_content = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + // Verify it's valid JSON (not Debug format) + let parsed: SearchResult = + serde_json::from_str(text_content).expect("Content should be valid JSON"); + + // Verify the structure + assert_eq!(parsed.total_count, 2); + assert_eq!(parsed.items.len(), 2); + assert_eq!(parsed.items[0].id, 1); + assert_eq!(parsed.items[0].title, "First Item"); + assert_eq!( + parsed.items[0].description, + Some("Description of first item".to_string()) + ); + assert_eq!(parsed.items[0].tags, vec!["tag1", "tag2"]); + assert!(parsed.has_more); + + // Verify structured_content is also populated (MCP 2025 spec) + assert!( + result.structured_content.is_some(), + "structured_content should be populated for structured types" + ); + + let structured = result.structured_content.unwrap(); + assert_eq!(structured["total_count"], 2); + assert_eq!(structured["items"][0]["id"], 1); +} + +#[tokio::test] +async fn test_nested_structured_return() { + let server = JsonTestServer::with_defaults(); + + let request = CallToolRequestParam { + name: "search_issues".to_string(), + arguments: Some(json!({ "query": "test query" })), + }; + + let result = server + .call_tool(request) + .await + .expect("Tool should succeed"); + + // Extract text content + let text_content = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + // Verify it's valid JSON + let parsed: IssueSearchResult = + serde_json::from_str(text_content).expect("Content should be valid JSON"); + + // Verify nested structure + assert_eq!(parsed.query, "test query"); + assert_eq!(parsed.duration_ms, 150); + assert_eq!(parsed.search_result.total_count, 1); + assert_eq!(parsed.search_result.items[0].id, 42); + assert_eq!( + parsed.search_result.items[0].title, + "Issue matching 'test query'" + ); + + // Verify it doesn't contain Debug format markers + assert!( + !text_content.contains("SearchResult {"), + "Should not contain Debug format: {text_content}" + ); + assert!( + !text_content.contains("IssueSearchResult {"), + "Should not contain Debug format: {text_content}" + ); +} + +#[tokio::test] +async fn test_result_type_success_serialization() { + let server = JsonTestServer::with_defaults(); + + let request = CallToolRequestParam { + name: "get_result_type".to_string(), + arguments: Some(json!({ "should_succeed": true })), + }; + + let result = server + .call_tool(request) + .await + .expect("Tool should succeed"); + + // Extract text content + let text_content = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + // Verify it's valid JSON + let parsed: SearchResult = + serde_json::from_str(text_content).expect("Content should be valid JSON"); + + assert_eq!(parsed.total_count, 1); + assert_eq!(parsed.items[0].id, 999); + assert_eq!(parsed.items[0].title, "Result type item"); + + // Verify structured_content + assert!(result.structured_content.is_some()); +} + +#[tokio::test] +async fn test_result_type_error() { + let server = JsonTestServer::with_defaults(); + + let request = CallToolRequestParam { + name: "get_result_type".to_string(), + arguments: Some(json!({ "should_succeed": false })), + }; + + let result = server.call_tool(request).await; + + // Should return an error + assert!(result.is_err(), "Should fail when should_succeed is false"); + + let error = result.unwrap_err(); + assert!(error.to_string().contains("Intentional failure")); +} + +#[tokio::test] +async fn test_simple_types_serialization() { + let server = JsonTestServer::with_defaults(); + + // Test string + let request = CallToolRequestParam { + name: "simple_string".to_string(), + arguments: None, + }; + let result = server.call_tool(request).await.expect("Should succeed"); + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + // String should be JSON-encoded (quoted) + let parsed: String = serde_json::from_str(text).expect("Should be valid JSON"); + assert_eq!(parsed, "Simple text response"); + + // Test number + let request = CallToolRequestParam { + name: "simple_number".to_string(), + arguments: None, + }; + let result = server.call_tool(request).await.expect("Should succeed"); + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + let parsed: i32 = serde_json::from_str(text).expect("Should be valid JSON"); + assert_eq!(parsed, 42); + + // Test bool + let request = CallToolRequestParam { + name: "simple_bool".to_string(), + arguments: None, + }; + let result = server.call_tool(request).await.expect("Should succeed"); + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + let parsed: bool = serde_json::from_str(text).expect("Should be valid JSON"); + assert!(parsed); +} + +#[tokio::test] +async fn test_vector_serialization() { + let server = JsonTestServer::with_defaults(); + + let request = CallToolRequestParam { + name: "string_vector".to_string(), + arguments: None, + }; + + let result = server.call_tool(request).await.expect("Should succeed"); + + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + // Should be JSON array + let parsed: Vec = serde_json::from_str(text).expect("Should be valid JSON array"); + assert_eq!(parsed, vec!["item1", "item2", "item3"]); + + // Should not contain Debug format + assert!(!text.contains("vec!["), "Should not contain Debug format"); +} + +#[tokio::test] +async fn test_no_debug_format_in_output() { + let server = JsonTestServer::with_defaults(); + + // Test all tools and verify none contain Debug format markers + let tools_to_test = vec![ + ("get_search_result", None), + ("search_issues", Some(json!({ "query": "test" }))), + ("get_result_type", Some(json!({ "should_succeed": true }))), + ]; + + for (tool_name, args) in tools_to_test { + let request = CallToolRequestParam { + name: tool_name.to_string(), + arguments: args, + }; + + let result = server + .call_tool(request) + .await + .expect("Tool should succeed"); + + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + // Verify no Debug format markers + let debug_markers = vec![ + "SearchResult {", + "SearchItem {", + "IssueSearchResult {", + ": String", + ": u32", + ": u64", + ": Vec<", + ]; + + for marker in debug_markers { + assert!( + !text.contains(marker), + "Tool '{tool_name}' output contains Debug format marker '{marker}': {text}" + ); + } + + // Verify it's valid JSON + serde_json::from_str::(text) + .unwrap_or_else(|_| panic!("Tool '{tool_name}' should return valid JSON")); + } +} + +#[tokio::test] +async fn test_structured_content_field_populated() { + let server = JsonTestServer::with_defaults(); + + let request = CallToolRequestParam { + name: "get_search_result".to_string(), + arguments: None, + }; + + let result = server + .call_tool(request) + .await + .expect("Tool should succeed"); + + // MCP 2025-06-18 spec: structured results should populate structured_content + assert!( + result.structured_content.is_some(), + "structured_content should be populated for structured types" + ); + + let structured = result.structured_content.unwrap(); + + // Verify structure matches the return type + assert_eq!(structured["total_count"], 2); + assert!(structured["items"].is_array()); + assert_eq!(structured["items"].as_array().unwrap().len(), 2); + assert_eq!(structured["has_more"], true); + + // Text content should also be valid JSON + let text = match &result.content[0] { + Content::Text { text, .. } => text, + _ => panic!("Expected text content"), + }; + + let text_parsed: serde_json::Value = + serde_json::from_str(text).expect("Text content should be valid JSON"); + + // structured_content and text content should represent the same data + assert_eq!(structured, text_parsed); +}