Skip to content

Commit ab0a2e0

Browse files
nick-stebbingsclaude
nick-stebbings
and
claude
authored
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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 290d925 commit ab0a2e0

5 files changed

Lines changed: 95 additions & 35 deletions

File tree

‎src/tools/mcp/client.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,7 @@ mod tests {
527527
fn test_mcp_request_list_tools() {
528528
let req = McpRequest::list_tools(1);
529529
assert_eq!(req.method, "tools/list");
530-
assert_eq!(req.id, 1);
530+
assert_eq!(req.id, Some(1));
531531
}
532532

533533
#[test]
@@ -777,7 +777,7 @@ mod tests {
777777
async fn test_non_http_transport_skips_401_retry() {
778778
let response = McpResponse {
779779
jsonrpc: "2.0".to_string(),
780-
id: 1,
780+
id: Some(1),
781781
result: Some(serde_json::json!({"tools": []})),
782782
error: None,
783783
};

‎src/tools/mcp/protocol.rs‎

Lines changed: 75 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,19 @@
11
//! MCP protocol types.
22
3-
use serde::{Deserialize, Serialize};
3+
use serde::{Deserialize, Deserializer, Serialize};
4+
5+
/// Flexibly deserialize a JSON-RPC id that may be a number, string, or null.
6+
fn deserialize_flexible_id<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error>
7+
where
8+
D: Deserializer<'de>,
9+
{
10+
let value: Option<serde_json::Value> = Option::deserialize(deserializer)?;
11+
match value {
12+
Some(serde_json::Value::Number(n)) => Ok(n.as_u64()),
13+
Some(serde_json::Value::String(s)) => Ok(s.parse::<u64>().ok()),
14+
_ => Ok(None),
15+
}
16+
}
417

518
/// MCP protocol version.
619
pub const PROTOCOL_VERSION: &str = "2024-11-05";
@@ -80,8 +93,9 @@ impl McpTool {
8093
pub struct McpRequest {
8194
/// JSON-RPC version.
8295
pub jsonrpc: String,
83-
/// Request ID.
84-
pub id: u64,
96+
/// Request ID (None for notifications per JSON-RPC spec).
97+
#[serde(skip_serializing_if = "Option::is_none")]
98+
pub id: Option<u64>,
8599
/// Method name.
86100
pub method: String,
87101
/// Request parameters.
@@ -94,7 +108,7 @@ impl McpRequest {
94108
pub fn new(id: u64, method: impl Into<String>, params: Option<serde_json::Value>) -> Self {
95109
Self {
96110
jsonrpc: "2.0".to_string(),
97-
id,
111+
id: Some(id),
98112
method: method.into(),
99113
params,
100114
}
@@ -120,15 +134,11 @@ impl McpRequest {
120134
}
121135

122136
/// Create an initialized notification (sent after initialize).
123-
///
124-
/// Note: JSON-RPC 2.0 notifications should omit the `id` field entirely.
125-
/// We set `id: 0` because `McpRequest` uses `u64` (not `Option<u64>`).
126-
/// Most MCP servers tolerate this; a proper fix would use a separate
127-
/// `McpNotification` type or make `id` optional with `skip_serializing_if`.
137+
/// Per JSON-RPC spec, notifications MUST NOT have an id field.
128138
pub fn initialized_notification() -> Self {
129139
Self {
130140
jsonrpc: "2.0".to_string(),
131-
id: 0,
141+
id: None,
132142
method: "notifications/initialized".to_string(),
133143
params: None,
134144
}
@@ -157,8 +167,9 @@ impl McpRequest {
157167
pub struct McpResponse {
158168
/// JSON-RPC version.
159169
pub jsonrpc: String,
160-
/// Request ID.
161-
pub id: u64,
170+
/// Request ID (may be missing for notifications or non-standard for errors).
171+
#[serde(deserialize_with = "deserialize_flexible_id")]
172+
pub id: Option<u64>,
162173
/// Result (on success).
163174
#[serde(skip_serializing_if = "Option::is_none")]
164175
pub result: Option<serde_json::Value>,
@@ -361,7 +372,7 @@ mod tests {
361372
fn test_initialize_request() {
362373
let req = McpRequest::initialize(42);
363374
assert_eq!(req.jsonrpc, "2.0");
364-
assert_eq!(req.id, 42);
375+
assert_eq!(req.id, Some(42));
365376
assert_eq!(req.method, "initialize");
366377

367378
let params = req.params.expect("initialize must have params");
@@ -385,7 +396,7 @@ mod tests {
385396
fn test_call_tool_request() {
386397
let args = serde_json::json!({"query": "rust async"});
387398
let req = McpRequest::call_tool(7, "search", args.clone());
388-
assert_eq!(req.id, 7);
399+
assert_eq!(req.id, Some(7));
389400
assert_eq!(req.method, "tools/call");
390401

391402
let params = req.params.expect("call_tool must have params");
@@ -401,7 +412,7 @@ mod tests {
401412
"result": { "tools": [] }
402413
});
403414
let resp: McpResponse = serde_json::from_value(json).expect("deserialize");
404-
assert_eq!(resp.id, 1);
415+
assert_eq!(resp.id, Some(1));
405416
assert!(resp.result.is_some());
406417
assert!(resp.error.is_none());
407418
}
@@ -630,6 +641,55 @@ mod tests {
630641
assert_eq!(serialized, "slow");
631642
}
632643

644+
#[test]
645+
fn test_notification_serializes_without_id_field() {
646+
// JSON-RPC 2.0 spec: notifications MUST NOT have an "id" field.
647+
let notif = McpRequest::initialized_notification();
648+
let json = serde_json::to_value(&notif).expect("serialize notification");
649+
assert!(
650+
json.get("id").is_none(),
651+
"notifications must not contain an 'id' field per JSON-RPC 2.0 spec"
652+
);
653+
assert_eq!(json.get("method").unwrap(), "notifications/initialized");
654+
}
655+
656+
#[test]
657+
fn test_response_with_string_id() {
658+
// Some MCP servers return id as a string instead of a number.
659+
let json = serde_json::json!({
660+
"jsonrpc": "2.0",
661+
"id": "42",
662+
"result": {}
663+
});
664+
let resp: McpResponse = serde_json::from_value(json).expect("deserialize string id");
665+
assert_eq!(resp.id, Some(42));
666+
}
667+
668+
#[test]
669+
fn test_response_with_null_id() {
670+
// JSON-RPC error responses may have a null id.
671+
let json = serde_json::json!({
672+
"jsonrpc": "2.0",
673+
"id": null,
674+
"error": { "code": -32700, "message": "Parse error" }
675+
});
676+
let resp: McpResponse = serde_json::from_value(json).expect("deserialize null id");
677+
assert_eq!(resp.id, None);
678+
}
679+
680+
#[test]
681+
fn test_response_with_non_numeric_string_id() {
682+
// Some servers send non-numeric string ids — these should parse as None.
683+
let json = serde_json::json!({
684+
"jsonrpc": "2.0",
685+
"id": "not-a-number",
686+
"result": {}
687+
});
688+
let resp: McpResponse =
689+
serde_json::from_value(json).expect("deserialize non-numeric string id");
690+
assert_eq!(resp.id, None);
691+
}
692+
633693
#[test]
634694
fn test_mcp_tool_roundtrip_preserves_schema() {
635695
// Simulate what list_tools returns from a real MCP server

‎src/tools/mcp/stdio_transport.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ impl McpTransport for StdioMcpTransport {
124124
// so we don't miss a fast response from the child.
125125
{
126126
let mut pending = self.pending.lock().await;
127-
pending.insert(request.id, tx);
127+
pending.insert(request.id.unwrap_or(0), tx);
128128
}
129129

130130
// Write the request to stdin.
@@ -133,7 +133,7 @@ impl McpTransport for StdioMcpTransport {
133133
if let Err(e) = write_jsonrpc_line(&mut *stdin, request).await {
134134
// Remove the pending entry on write failure.
135135
let mut pending = self.pending.lock().await;
136-
pending.remove(&request.id);
136+
pending.remove(&request.id.unwrap_or(0));
137137
return Err(e);
138138
}
139139
}
@@ -145,18 +145,18 @@ impl McpTransport for StdioMcpTransport {
145145
Ok(Err(_)) => {
146146
// Sender was dropped (reader task ended). Clean up pending entry.
147147
let mut pending = self.pending.lock().await;
148-
pending.remove(&request.id);
148+
pending.remove(&request.id.unwrap_or(0));
149149
Err(ToolError::ExternalService(format!(
150-
"[{}] MCP server closed connection before responding to request {}",
150+
"[{}] MCP server closed connection before responding to request {:?}",
151151
self.server_name, request.id
152152
)))
153153
}
154154
Err(_) => {
155155
// Timeout: remove the pending entry.
156156
let mut pending = self.pending.lock().await;
157-
pending.remove(&request.id);
157+
pending.remove(&request.id.unwrap_or(0));
158158
Err(ToolError::ExternalService(format!(
159-
"[{}] Timeout waiting for response to request {} after {:?}",
159+
"[{}] Timeout waiting for response to request {:?} after {:?}",
160160
self.server_name, request.id, timeout
161161
)))
162162
}

‎src/tools/mcp/transport.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ pub fn spawn_jsonrpc_reader<R: AsyncBufRead + Unpin + Send + 'static>(
9797
}
9898
};
9999

100-
let id = response.id;
100+
let id = response.id.unwrap_or(0);
101101
let mut map = pending.lock().await;
102102
if let Some(tx) = map.remove(&id) {
103103
// Ignore send error — the receiver may have been dropped (timeout).
@@ -123,7 +123,7 @@ mod tests {
123123
async fn test_write_jsonrpc_line_serializes_and_flushes() {
124124
let request = McpRequest {
125125
jsonrpc: "2.0".into(),
126-
id: 1,
126+
id: Some(1),
127127
method: "test/method".into(),
128128
params: None,
129129
};
@@ -146,7 +146,7 @@ mod tests {
146146
async fn test_spawn_jsonrpc_reader_dispatches_response() {
147147
let response = McpResponse {
148148
jsonrpc: "2.0".into(),
149-
id: 42,
149+
id: Some(42),
150150
result: Some(serde_json::json!({"tools": []})),
151151
error: None,
152152
};
@@ -165,7 +165,7 @@ mod tests {
165165
let handle = spawn_jsonrpc_reader(reader, pending.clone(), "test".into());
166166

167167
let resp = rx.await.expect("should receive response");
168-
assert_eq!(resp.id, 42);
168+
assert_eq!(resp.id, Some(42));
169169
assert!(resp.result.is_some());
170170

171171
handle.await.expect("reader task should finish");
@@ -189,7 +189,7 @@ mod tests {
189189
let resp = rx
190190
.await
191191
.expect("should receive response despite earlier invalid line");
192-
assert_eq!(resp.id, 7);
192+
assert_eq!(resp.id, Some(7));
193193

194194
handle.await.expect("reader task should finish");
195195
}

‎src/tools/mcp/unix_transport.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ impl McpTransport for UnixMcpTransport {
9797
// so we don't miss a fast response from the server.
9898
{
9999
let mut pending = self.pending.lock().await;
100-
pending.insert(request.id, tx);
100+
pending.insert(request.id.unwrap_or(0), tx);
101101
}
102102

103103
// Write the request to the socket.
@@ -106,7 +106,7 @@ impl McpTransport for UnixMcpTransport {
106106
if let Err(e) = write_jsonrpc_line(&mut *writer, request).await {
107107
// Remove the pending entry on write failure.
108108
let mut pending = self.pending.lock().await;
109-
pending.remove(&request.id);
109+
pending.remove(&request.id.unwrap_or(0));
110110
return Err(e);
111111
}
112112
}
@@ -118,18 +118,18 @@ impl McpTransport for UnixMcpTransport {
118118
Ok(Err(_)) => {
119119
// Sender was dropped (reader task ended). Clean up pending entry.
120120
let mut pending = self.pending.lock().await;
121-
pending.remove(&request.id);
121+
pending.remove(&request.id.unwrap_or(0));
122122
Err(ToolError::ExternalService(format!(
123-
"[{}] MCP server closed connection before responding to request {}",
123+
"[{}] MCP server closed connection before responding to request {:?}",
124124
self.server_name, request.id
125125
)))
126126
}
127127
Err(_) => {
128128
// Timeout: remove the pending entry.
129129
let mut pending = self.pending.lock().await;
130-
pending.remove(&request.id);
130+
pending.remove(&request.id.unwrap_or(0));
131131
Err(ToolError::ExternalService(format!(
132-
"[{}] Timeout waiting for response to request {} after {:?}",
132+
"[{}] Timeout waiting for response to request {:?} after {:?}",
133133
self.server_name, request.id, timeout
134134
)))
135135
}
@@ -237,7 +237,7 @@ mod tests {
237237
let headers = HashMap::new();
238238
let response = transport.send(&request, &headers).await.expect("send");
239239

240-
assert_eq!(response.id, 42);
240+
assert_eq!(response.id, Some(42));
241241
assert!(response.result.is_some());
242242
assert!(response.error.is_none());
243243

0 commit comments

Comments
 (0)