fix(mcp): JSON-RPC spec compliance — flexible id, correct notification format (#685)

* fix(mcp): JSON-RPC spec compliance — flexible id, correct notification format

- McpRequest.id is now Option<u64> 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 <[email protected]>

* 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<u64>

Co-Authored-By: Claude Opus 4.6 <[email protected]>

* fix: update new transport files for Option<u64> 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<u64>), these need .unwrap_or(0) for HashMap keys and Some()
wrapping in tests.

Co-Authored-By: Claude Opus 4.6 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>

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,
         };

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<Option<u64>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    let value: Option<serde_json::Value> = Option::deserialize(deserializer)?;
+    match value {
+        Some(serde_json::Value::Number(n)) => Ok(n.as_u64()),
+        Some(serde_json::Value::String(s)) => Ok(s.parse::<u64>().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<u64>,
     /// Method name.
     pub method: String,
     /// Request parameters.
@@ -94,7 +108,7 @@ impl McpRequest {
     pub fn new(id: u64, method: impl Into<String>, params: Option<serde_json::Value>) -> 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<u64>`).
-    /// 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<u64>,
     /// Result (on success).
     #[serde(skip_serializing_if = "Option::is_none")]
     pub result: Option<serde_json::Value>,
@@ -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(&notif).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

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
                 )))
             }

src/tools/mcp/transport.rs

@@ -97,7 +97,7 @@ pub fn spawn_jsonrpc_reader<R: AsyncBufRead + Unpin + Send + 'static>(
                 }
             };
 
-            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");
     }

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());