From ab0a2e05de57c78bc078373626d1946aa360fac6 Mon Sep 17 00:00:00 2001 From: Nick Stebbings <47646783+nick-stebbings@users.noreply.github.com> Date: Tue, 10 Mar 2026 04:37:42 +1300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20JSON-RPC=20spec=20compliance=20?= =?UTF-8?q?=E2=80=94=20flexible=20id,=20correct=20notification=20format=20?= =?UTF-8?q?(#685)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): JSON-RPC spec compliance — flexible id, correct notification format - McpRequest.id is now Option with skip_serializing_if, so notifications omit the id field as required by JSON-RPC 2.0 spec. Previously sent id: 0 which violates the spec. - McpResponse.id uses flexible deserialization that accepts number, string, or null — fixes interop with non-standard MCP servers that return string ids or missing id fields on error responses. Co-Authored-By: Claude Opus 4.6 * Fix review feedback: remove serde(default) from McpResponse.id, fix test assertions - Remove #[serde(default)] from McpResponse.id so notifications (no id field) don't incorrectly parse as responses — prevents DoS/spoofing via SSE - Update test assertions to use Some(value) after id became Option Co-Authored-By: Claude Opus 4.6 * fix: update new transport files for Option id after rebase Upstream #721 added stdio/unix/transport modules that use McpRequest.id and McpResponse.id as u64. After our rebase (which changes id to Option), these need .unwrap_or(0) for HashMap keys and Some() wrapping in tests. Co-Authored-By: Claude Opus 4.6 * test: add regression tests for JSON-RPC spec compliance Tests for notification serialization without id field, flexible id deserialization (string, null, non-numeric). Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/tools/mcp/client.rs | 4 +- src/tools/mcp/protocol.rs | 90 ++++++++++++++++++++++++++------ src/tools/mcp/stdio_transport.rs | 12 ++--- src/tools/mcp/transport.rs | 10 ++-- src/tools/mcp/unix_transport.rs | 14 ++--- 5 files changed, 95 insertions(+), 35 deletions(-) diff --git a/src/tools/mcp/client.rs b/src/tools/mcp/client.rs index aa14189b..cd74d572 100644 --- a/src/tools/mcp/client.rs +++ b/src/tools/mcp/client.rs @@ -527,7 +527,7 @@ mod tests { fn test_mcp_request_list_tools() { let req = McpRequest::list_tools(1); assert_eq!(req.method, "tools/list"); - assert_eq!(req.id, 1); + assert_eq!(req.id, Some(1)); } #[test] @@ -777,7 +777,7 @@ mod tests { async fn test_non_http_transport_skips_401_retry() { let response = McpResponse { jsonrpc: "2.0".to_string(), - id: 1, + id: Some(1), result: Some(serde_json::json!({"tools": []})), error: None, }; diff --git a/src/tools/mcp/protocol.rs b/src/tools/mcp/protocol.rs index cb55ef94..f8071e09 100644 --- a/src/tools/mcp/protocol.rs +++ b/src/tools/mcp/protocol.rs @@ -1,6 +1,19 @@ //! MCP protocol types. -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; + +/// Flexibly deserialize a JSON-RPC id that may be a number, string, or null. +fn deserialize_flexible_id<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let value: Option = Option::deserialize(deserializer)?; + match value { + Some(serde_json::Value::Number(n)) => Ok(n.as_u64()), + Some(serde_json::Value::String(s)) => Ok(s.parse::().ok()), + _ => Ok(None), + } +} /// MCP protocol version. pub const PROTOCOL_VERSION: &str = "2024-11-05"; @@ -80,8 +93,9 @@ impl McpTool { pub struct McpRequest { /// JSON-RPC version. pub jsonrpc: String, - /// Request ID. - pub id: u64, + /// Request ID (None for notifications per JSON-RPC spec). + #[serde(skip_serializing_if = "Option::is_none")] + pub id: Option, /// Method name. pub method: String, /// Request parameters. @@ -94,7 +108,7 @@ impl McpRequest { pub fn new(id: u64, method: impl Into, params: Option) -> Self { Self { jsonrpc: "2.0".to_string(), - id, + id: Some(id), method: method.into(), params, } @@ -120,15 +134,11 @@ impl McpRequest { } /// Create an initialized notification (sent after initialize). - /// - /// Note: JSON-RPC 2.0 notifications should omit the `id` field entirely. - /// We set `id: 0` because `McpRequest` uses `u64` (not `Option`). - /// Most MCP servers tolerate this; a proper fix would use a separate - /// `McpNotification` type or make `id` optional with `skip_serializing_if`. + /// Per JSON-RPC spec, notifications MUST NOT have an id field. pub fn initialized_notification() -> Self { Self { jsonrpc: "2.0".to_string(), - id: 0, + id: None, method: "notifications/initialized".to_string(), params: None, } @@ -157,8 +167,9 @@ impl McpRequest { pub struct McpResponse { /// JSON-RPC version. pub jsonrpc: String, - /// Request ID. - pub id: u64, + /// Request ID (may be missing for notifications or non-standard for errors). + #[serde(deserialize_with = "deserialize_flexible_id")] + pub id: Option, /// Result (on success). #[serde(skip_serializing_if = "Option::is_none")] pub result: Option, @@ -361,7 +372,7 @@ mod tests { fn test_initialize_request() { let req = McpRequest::initialize(42); assert_eq!(req.jsonrpc, "2.0"); - assert_eq!(req.id, 42); + assert_eq!(req.id, Some(42)); assert_eq!(req.method, "initialize"); let params = req.params.expect("initialize must have params"); @@ -385,7 +396,7 @@ mod tests { fn test_call_tool_request() { let args = serde_json::json!({"query": "rust async"}); let req = McpRequest::call_tool(7, "search", args.clone()); - assert_eq!(req.id, 7); + assert_eq!(req.id, Some(7)); assert_eq!(req.method, "tools/call"); let params = req.params.expect("call_tool must have params"); @@ -401,7 +412,7 @@ mod tests { "result": { "tools": [] } }); let resp: McpResponse = serde_json::from_value(json).expect("deserialize"); - assert_eq!(resp.id, 1); + assert_eq!(resp.id, Some(1)); assert!(resp.result.is_some()); assert!(resp.error.is_none()); } @@ -630,6 +641,55 @@ mod tests { assert_eq!(serialized, "slow"); } + #[test] + fn test_notification_serializes_without_id_field() { + // JSON-RPC 2.0 spec: notifications MUST NOT have an "id" field. + let notif = McpRequest::initialized_notification(); + let json = serde_json::to_value(¬if).expect("serialize notification"); + assert!( + json.get("id").is_none(), + "notifications must not contain an 'id' field per JSON-RPC 2.0 spec" + ); + assert_eq!(json.get("method").unwrap(), "notifications/initialized"); + } + + #[test] + fn test_response_with_string_id() { + // Some MCP servers return id as a string instead of a number. + let json = serde_json::json!({ + "jsonrpc": "2.0", + "id": "42", + "result": {} + }); + let resp: McpResponse = serde_json::from_value(json).expect("deserialize string id"); + assert_eq!(resp.id, Some(42)); + } + + #[test] + fn test_response_with_null_id() { + // JSON-RPC error responses may have a null id. + let json = serde_json::json!({ + "jsonrpc": "2.0", + "id": null, + "error": { "code": -32700, "message": "Parse error" } + }); + let resp: McpResponse = serde_json::from_value(json).expect("deserialize null id"); + assert_eq!(resp.id, None); + } + + #[test] + fn test_response_with_non_numeric_string_id() { + // Some servers send non-numeric string ids — these should parse as None. + let json = serde_json::json!({ + "jsonrpc": "2.0", + "id": "not-a-number", + "result": {} + }); + let resp: McpResponse = + serde_json::from_value(json).expect("deserialize non-numeric string id"); + assert_eq!(resp.id, None); + } + #[test] fn test_mcp_tool_roundtrip_preserves_schema() { // Simulate what list_tools returns from a real MCP server diff --git a/src/tools/mcp/stdio_transport.rs b/src/tools/mcp/stdio_transport.rs index c1b79762..95deb8b3 100644 --- a/src/tools/mcp/stdio_transport.rs +++ b/src/tools/mcp/stdio_transport.rs @@ -124,7 +124,7 @@ impl McpTransport for StdioMcpTransport { // so we don't miss a fast response from the child. { let mut pending = self.pending.lock().await; - pending.insert(request.id, tx); + pending.insert(request.id.unwrap_or(0), tx); } // Write the request to stdin. @@ -133,7 +133,7 @@ impl McpTransport for StdioMcpTransport { if let Err(e) = write_jsonrpc_line(&mut *stdin, request).await { // Remove the pending entry on write failure. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); return Err(e); } } @@ -145,18 +145,18 @@ impl McpTransport for StdioMcpTransport { Ok(Err(_)) => { // Sender was dropped (reader task ended). Clean up pending entry. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); Err(ToolError::ExternalService(format!( - "[{}] MCP server closed connection before responding to request {}", + "[{}] MCP server closed connection before responding to request {:?}", self.server_name, request.id ))) } Err(_) => { // Timeout: remove the pending entry. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); Err(ToolError::ExternalService(format!( - "[{}] Timeout waiting for response to request {} after {:?}", + "[{}] Timeout waiting for response to request {:?} after {:?}", self.server_name, request.id, timeout ))) } diff --git a/src/tools/mcp/transport.rs b/src/tools/mcp/transport.rs index 98c4a478..e5030b28 100644 --- a/src/tools/mcp/transport.rs +++ b/src/tools/mcp/transport.rs @@ -97,7 +97,7 @@ pub fn spawn_jsonrpc_reader( } }; - let id = response.id; + let id = response.id.unwrap_or(0); let mut map = pending.lock().await; if let Some(tx) = map.remove(&id) { // Ignore send error — the receiver may have been dropped (timeout). @@ -123,7 +123,7 @@ mod tests { async fn test_write_jsonrpc_line_serializes_and_flushes() { let request = McpRequest { jsonrpc: "2.0".into(), - id: 1, + id: Some(1), method: "test/method".into(), params: None, }; @@ -146,7 +146,7 @@ mod tests { async fn test_spawn_jsonrpc_reader_dispatches_response() { let response = McpResponse { jsonrpc: "2.0".into(), - id: 42, + id: Some(42), result: Some(serde_json::json!({"tools": []})), error: None, }; @@ -165,7 +165,7 @@ mod tests { let handle = spawn_jsonrpc_reader(reader, pending.clone(), "test".into()); let resp = rx.await.expect("should receive response"); - assert_eq!(resp.id, 42); + assert_eq!(resp.id, Some(42)); assert!(resp.result.is_some()); handle.await.expect("reader task should finish"); @@ -189,7 +189,7 @@ mod tests { let resp = rx .await .expect("should receive response despite earlier invalid line"); - assert_eq!(resp.id, 7); + assert_eq!(resp.id, Some(7)); handle.await.expect("reader task should finish"); } diff --git a/src/tools/mcp/unix_transport.rs b/src/tools/mcp/unix_transport.rs index 07ef7f17..bf5865c6 100644 --- a/src/tools/mcp/unix_transport.rs +++ b/src/tools/mcp/unix_transport.rs @@ -97,7 +97,7 @@ impl McpTransport for UnixMcpTransport { // so we don't miss a fast response from the server. { let mut pending = self.pending.lock().await; - pending.insert(request.id, tx); + pending.insert(request.id.unwrap_or(0), tx); } // Write the request to the socket. @@ -106,7 +106,7 @@ impl McpTransport for UnixMcpTransport { if let Err(e) = write_jsonrpc_line(&mut *writer, request).await { // Remove the pending entry on write failure. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); return Err(e); } } @@ -118,18 +118,18 @@ impl McpTransport for UnixMcpTransport { Ok(Err(_)) => { // Sender was dropped (reader task ended). Clean up pending entry. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); Err(ToolError::ExternalService(format!( - "[{}] MCP server closed connection before responding to request {}", + "[{}] MCP server closed connection before responding to request {:?}", self.server_name, request.id ))) } Err(_) => { // Timeout: remove the pending entry. let mut pending = self.pending.lock().await; - pending.remove(&request.id); + pending.remove(&request.id.unwrap_or(0)); Err(ToolError::ExternalService(format!( - "[{}] Timeout waiting for response to request {} after {:?}", + "[{}] Timeout waiting for response to request {:?} after {:?}", self.server_name, request.id, timeout ))) } @@ -237,7 +237,7 @@ mod tests { let headers = HashMap::new(); let response = transport.send(&request, &headers).await.expect("send"); - assert_eq!(response.id, 42); + assert_eq!(response.id, Some(42)); assert!(response.result.is_some()); assert!(response.error.is_none());