fix: make JsonRpcError id optional per MCP spec

DaleSeo · DaleSeo · commit b3858c038f69 · 2026-05-07T11:15:26.000-04:00
diff --git a/crates/rmcp/src/model.rs b/crates/rmcp/src/model.rs
@@ -461,13 +461,16 @@ pub struct JsonRpcResponse<R = JsonObject> {
 #[expect(clippy::exhaustive_structs, reason = "intentionally exhaustive")]
 pub struct JsonRpcError {
     pub jsonrpc: JsonRpcVersion2_0,
-    pub id: RequestId,
+    // MCP 2025-11-25 §Error Responses: `id` is optional and omitted when the
+    // server cannot read the request id (e.g. parse error / invalid request).
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub id: Option<RequestId>,
     pub error: ErrorData,
 }
 
 impl JsonRpcError {
     /// Create a new JsonRpcError.
-    pub fn new(id: RequestId, error: ErrorData) -> Self {
+    pub fn new(id: Option<RequestId>, error: ErrorData) -> Self {
         Self {
             jsonrpc: JsonRpcVersion2_0,
             id,
@@ -601,7 +604,7 @@ impl<Req, Resp, Not> JsonRpcMessage<Req, Resp, Not> {
         })
     }
     #[inline]
-    pub const fn error(error: ErrorData, id: RequestId) -> Self {
+    pub const fn error(error: ErrorData, id: Option<RequestId>) -> Self {
         JsonRpcMessage::Error(JsonRpcError {
             jsonrpc: JsonRpcVersion2_0,
             id,
@@ -633,15 +636,15 @@ impl<Req, Resp, Not> JsonRpcMessage<Req, Resp, Not> {
             _ => None,
         }
     }
-    pub fn into_error(self) -> Option<(ErrorData, RequestId)> {
+    pub fn into_error(self) -> Option<(ErrorData, Option<RequestId>)> {
         match self {
             JsonRpcMessage::Error(e) => Some((e.error, e.id)),
             _ => None,
         }
     }
-    pub fn into_result(self) -> Option<(Result<Resp, ErrorData>, RequestId)> {
+    pub fn into_result(self) -> Option<(Result<Resp, ErrorData>, Option<RequestId>)> {
         match self {
-            JsonRpcMessage::Response(r) => Some((Ok(r.result), r.id)),
+            JsonRpcMessage::Response(r) => Some((Ok(r.result), Some(r.id))),
             JsonRpcMessage::Error(e) => Some((Err(e.error), e.id)),
 
             _ => None,
diff --git a/crates/rmcp/src/service.rs b/crates/rmcp/src/service.rs
@@ -881,7 +881,7 @@ where
                 Event::ToSink(m) => {
                     if let Some(id) = match &m {
                         JsonRpcMessage::Response(response) => Some(&response.id),
-                        JsonRpcMessage::Error(error) => Some(&error.id),
+                        JsonRpcMessage::Error(error) => error.id.as_ref(),
                         _ => None,
                     } {
                         if let Some(ct) = local_ct_pool.remove(id) {
@@ -971,7 +971,7 @@ where
                                 }
                                 Err(error) => {
                                     tracing::warn!(%id, ?error, "response error");
-                                    JsonRpcMessage::error(error, id)
+                                    JsonRpcMessage::error(error, Some(id))
                                 }
                             };
                             let _send_result = sink.send(response).await;
@@ -1028,6 +1028,12 @@ where
                     }
                 }
                 Event::PeerMessage(JsonRpcMessage::Error(JsonRpcError { error, id, .. })) => {
+                    let Some(id) = id else {
+                        // MCP error responses without an id (e.g. Parse error / Invalid Request)
+                        // can't be routed back to a pending request — log and drop.
+                        tracing::debug!(?error, "received id-less peer error");
+                        continue;
+                    };
                     if let Some(responder) = local_responder_pool.remove(&id) {
                         let _response_result = responder.send(Err(ServiceError::McpError(error)));
                         if let Err(_error) = _response_result {
diff --git a/crates/rmcp/src/service/server.rs b/crates/rmcp/src/service/server.rs
@@ -219,7 +219,7 @@ where
         }
         Err(e) => {
             transport
-                .send(ServerJsonRpcMessage::error(e.clone(), id))
+                .send(ServerJsonRpcMessage::error(e.clone(), Some(id)))
                 .await
                 .map_err(|error| {
                     ServerInitializeError::transport::<T>(error, "sending error response")
diff --git a/crates/rmcp/src/transport/async_rw.rs b/crates/rmcp/src/transport/async_rw.rs
@@ -4,7 +4,7 @@ use futures::SinkExt;
 use serde::{Serialize, de::DeserializeOwned};
 use thiserror::Error;
 use tokio::{
-    io::{AsyncBufReadExt, AsyncRead, AsyncWrite, AsyncWriteExt, BufReader},
+    io::{AsyncBufReadExt, AsyncRead, AsyncWrite, BufReader},
     sync::Mutex,
 };
 use tokio_util::{
@@ -13,7 +13,10 @@ use tokio_util::{
 };
 
 use super::{IntoTransport, Transport};
-use crate::service::{RxJsonRpcMessage, ServiceRole, TxJsonRpcMessage};
+use crate::{
+    model::ErrorData,
+    service::{RxJsonRpcMessage, ServiceRole, TxJsonRpcMessage},
+};
 
 #[non_exhaustive]
 pub enum TransportAdapterAsyncRW {}
@@ -143,10 +146,11 @@ where
                     tracing::debug!("Parse error on incoming message: {e}");
                     let mut write = self.write.lock().await;
                     let framed = write.as_mut()?;
-                    let inner = framed.get_mut();
-                    if inner.write_all(PARSE_ERROR_RESPONSE).await.is_err()
-                        || inner.flush().await.is_err()
-                    {
+                    let response = TxJsonRpcMessage::<Role>::error(
+                        ErrorData::parse_error("Parse error", None),
+                        None,
+                    );
+                    if framed.send(response).await.is_err() {
                         return None;
                     }
                 }
@@ -208,12 +212,6 @@ fn without_carriage_return(s: &[u8]) -> &[u8] {
 /// UTF-8 byte order mark. RFC 8259 §8.1 allows JSON parsers to ignore a leading BOM.
 const UTF8_BOM: &[u8; 3] = b"\xEF\xBB\xBF";
 
-// JSON-RPC 2.0 §5.1: https://www.jsonrpc.org/specification#error_object
-// Hardcoded bytes because `RequestId` has no `Null` variant — we can't
-// build an `id: null` JsonRpcError through the typed codec.
-const PARSE_ERROR_RESPONSE: &[u8] =
-    b"{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32700,\"message\":\"Parse error\"},\"id\":null}\n";
-
 /// Check if a method is a standard MCP method (request, response, or notification).
 /// This includes both requests and notifications defined in the MCP specification.
 ///
@@ -621,7 +619,7 @@ mod test {
     #[cfg(feature = "server")]
     #[tokio::test]
     async fn receive_recovers_from_parse_error() {
-        use tokio::io::AsyncReadExt;
+        use tokio::io::{AsyncReadExt, AsyncWriteExt};
 
         use crate::{RoleServer, transport::Transport};
 
@@ -645,10 +643,20 @@ mod test {
             .await
             .expect("transport should recover and yield the next valid message");
 
-        let mut reply = vec![0u8; PARSE_ERROR_RESPONSE.len()];
-        client_r.read_exact(&mut reply).await.unwrap();
+        // Read one line back from the peer side and parse as JSON.
+        let mut reply_buf = Vec::new();
+        let mut peer = tokio::io::BufReader::new(&mut client_r);
+        peer.read_until(b'\n', &mut reply_buf).await.unwrap();
+        let reply: serde_json::Value = serde_json::from_slice(&reply_buf).unwrap();
 
-        assert_eq!(reply, PARSE_ERROR_RESPONSE);
+        // Per MCP 2025-11-25: id is omitted when the server can't read the request id.
+        assert_eq!(
+            reply,
+            serde_json::json!({
+                "jsonrpc": "2.0",
+                "error": {"code": -32700, "message": "Parse error"},
+            })
+        );
         assert_eq!(
             serde_json::to_value(&received).unwrap()["method"],
             "notifications/initialized",
diff --git a/crates/rmcp/src/transport/streamable_http_server/session/local.rs b/crates/rmcp/src/transport/streamable_http_server/session/local.rs
@@ -523,14 +523,12 @@ impl LocalSessionWorker {
                 }
             }
             ServerJsonRpcMessage::Error(json_rpc_error) => {
-                if let Some(id) = self
-                    .resource_router
-                    .get(&ResourceKey::McpRequestId(json_rpc_error.id.clone()))
-                {
-                    OutboundChannel::RequestWise {
-                        id: *id,
-                        close: true,
-                    }
+                if let Some(id) = json_rpc_error.id.clone().and_then(|rid| {
+                    self.resource_router
+                        .get(&ResourceKey::McpRequestId(rid))
+                        .copied()
+                }) {
+                    OutboundChannel::RequestWise { id, close: true }
                 } else {
                     OutboundChannel::Common
                 }
@@ -1041,8 +1039,7 @@ impl Worker for LocalSessionWorker {
                             Some(ResourceKey::McpRequestId(request_id))
                         }
                         crate::model::JsonRpcMessage::Error(json_rpc_error) => {
-                            let request_id = json_rpc_error.id.clone();
-                            Some(ResourceKey::McpRequestId(request_id))
+                            json_rpc_error.id.clone().map(ResourceKey::McpRequestId)
                         }
                         _ => {
                             None
diff --git a/crates/rmcp/tests/test_message_schema/client_json_rpc_message_schema.json b/crates/rmcp/tests/test_message_schema/client_json_rpc_message_schema.json
@@ -862,15 +862,21 @@
           "$ref": "#/definitions/ErrorData"
         },
         "id": {
-          "$ref": "#/definitions/NumberOrString"
+          "anyOf": [
+            {
+              "$ref": "#/definitions/NumberOrString"
+            },
+            {
+              "type": "null"
+            }
+          ]
         },
         "jsonrpc": {
           "$ref": "#/definitions/JsonRpcVersion2_0"
         }
       },
       "required": [
         "jsonrpc",
-        "id",
         "error"
       ]
     },
diff --git a/crates/rmcp/tests/test_message_schema/client_json_rpc_message_schema_current.json b/crates/rmcp/tests/test_message_schema/client_json_rpc_message_schema_current.json
@@ -862,15 +862,21 @@
           "$ref": "#/definitions/ErrorData"
         },
         "id": {
-          "$ref": "#/definitions/NumberOrString"
+          "anyOf": [
+            {
+              "$ref": "#/definitions/NumberOrString"
+            },
+            {
+              "type": "null"
+            }
+          ]
         },
         "jsonrpc": {
           "$ref": "#/definitions/JsonRpcVersion2_0"
         }
       },
       "required": [
         "jsonrpc",
-        "id",
         "error"
       ]
     },
diff --git a/crates/rmcp/tests/test_message_schema/server_json_rpc_message_schema.json b/crates/rmcp/tests/test_message_schema/server_json_rpc_message_schema.json
@@ -1281,15 +1281,21 @@
           "$ref": "#/definitions/ErrorData"
         },
         "id": {
-          "$ref": "#/definitions/NumberOrString"
+          "anyOf": [
+            {
+              "$ref": "#/definitions/NumberOrString"
+            },
+            {
+              "type": "null"
+            }
+          ]
         },
         "jsonrpc": {
           "$ref": "#/definitions/JsonRpcVersion2_0"
         }
       },
       "required": [
         "jsonrpc",
-        "id",
         "error"
       ]
     },
diff --git a/crates/rmcp/tests/test_message_schema/server_json_rpc_message_schema_current.json b/crates/rmcp/tests/test_message_schema/server_json_rpc_message_schema_current.json
@@ -1281,15 +1281,21 @@
           "$ref": "#/definitions/ErrorData"
         },
         "id": {
-          "$ref": "#/definitions/NumberOrString"
+          "anyOf": [
+            {
+              "$ref": "#/definitions/NumberOrString"
+            },
+            {
+              "type": "null"
+            }
+          ]
         },
         "jsonrpc": {
           "$ref": "#/definitions/JsonRpcVersion2_0"
         }
       },
       "required": [
         "jsonrpc",
-        "id",
         "error"
       ]
     },

Original file line number	Diff line number	Diff line change
`@@ -219,7 +219,7 @@ where`
`219`	`219`	`}`
`220`	`220`	`Err(e) => {`
`221`	`221`	`transport`
`222`		`- .send(ServerJsonRpcMessage::error(e.clone(), id))`
	`222`	`+ .send(ServerJsonRpcMessage::error(e.clone(), Some(id)))`
`223`	`223`	`.await`
`224`	`224`	`.map_err(\|error\| {`
`225`	`225`	`ServerInitializeError::transport::<T>(error, "sending error response")`