From 933fb5a54be104838ea1d4ab26797cafe06df521 Mon Sep 17 00:00:00 2001 From: Huskies Agent Date: Sun, 19 Jul 2026 18:06:49 +0000 Subject: [PATCH] huskies: merge 1232 bug Gateway chat bot crashes (CLI exit 1) on a sled MCP tool error instead of surfacing it --- server/src/gateway/tests.rs | 15 ++- server/src/http/gateway/mcp.rs | 202 +++++++++++++++++++++++++++--- server/src/service/gateway/mod.rs | 98 +++++++++++++++ 3 files changed, 296 insertions(+), 19 deletions(-) diff --git a/server/src/gateway/tests.rs b/server/src/gateway/tests.rs index a1e5c80c..19262310 100644 --- a/server/src/gateway/tests.rs +++ b/server/src/gateway/tests.rs @@ -1213,11 +1213,20 @@ async fn gateway_mcp_post_against_disconnected_sled_returns_error_response_fast( let body: serde_json::Value = resp.0.into_body().into_json().await.unwrap(); assert_eq!(body["id"], 3); + // Story 1232: a disconnected sled is a tool-call-time failure, not a + // protocol fault — it must come back as a non-fatal MCP tool_result + // (isError: true) so the calling `claude` CLI's MCP client treats it as + // a normal tool error instead of exiting non-zero, not a top-level + // JSON-RPC `error` object. assert!( - body.get("error").is_some(), - "Expected a JSON-RPC error for a disconnected sled; got: {body}" + body.get("error").is_none(), + "a disconnected sled must not produce a top-level JSON-RPC error; got: {body}" ); - let msg = body["error"]["message"].as_str().unwrap_or(""); + assert_eq!( + body["result"]["isError"], true, + "expected a non-fatal tool_result for a disconnected sled; got: {body}" + ); + let msg = body["result"]["content"][0]["text"].as_str().unwrap_or(""); assert!( msg.contains("offline-sled"), "error message must name the sled; got: {msg}" diff --git a/server/src/http/gateway/mcp.rs b/server/src/http/gateway/mcp.rs index 8b4513ba..decf450c 100644 --- a/server/src/http/gateway/mcp.rs +++ b/server/src/http/gateway/mcp.rs @@ -443,11 +443,7 @@ async fn proxy_and_respond_with_resolved_project( .status(StatusCode::OK) .header("Content-Type", "application/json") .body(Body::from(annotate_resolved_project(&resp_body, &active))), - Err(e) => to_json_response(JsonRpcResponse::error( - id, - -32603, - format!("proxy error: {e}"), - )), + Err(e) => tool_error_response(id, format!("Sled '{active}' is unavailable: {e}")), } } @@ -486,14 +482,22 @@ async fn proxy_and_respond_for_project( .status(StatusCode::OK) .header("Content-Type", "application/json") .body(Body::from(resp_body)), - Err(e) => to_json_response(JsonRpcResponse::error( - id, - -32603, - format!("proxy error: {e}"), - )), + Err(e) => tool_error_response(id, format!("Sled '{project}' is unavailable: {e}")), } } +/// Build a JSON-RPC **success** response shaped as an MCP tool_result error +/// (`isError: true`) rather than a top-level JSON-RPC protocol error. +/// +/// A down or version-mismatched sled is a tool-call-time failure, not a +/// protocol violation — the downstream `claude` CLI's MCP client must see a +/// normal tool_result so the model can report the failure and the turn +/// completes, instead of a top-level error that the CLI may treat as fatal +/// and exit non-zero (story 1232). +fn tool_error_response(id: Option, message: String) -> Response { + to_json_response(tool_error_json(id, message)) +} + /// Stream an MCP tool call to the active sled as SSE, re-emitting each `data:` /// event from the sled to the originating gateway client without buffering. /// @@ -502,7 +506,7 @@ async fn proxy_and_respond_for_project( async fn proxy_and_respond_sse(state: &GatewayState, bytes: &[u8], id: Option) -> Response { let url = match state.active_url().await { Ok(u) => u, - Err(e) => return sse_error_response(id, -32603, e.to_string()), + Err(e) => return sse_tool_error_response(id, e.to_string()), }; stream_mcp_call_sse(state, &url, bytes, id).await } @@ -537,7 +541,7 @@ async fn stream_mcp_call_sse( ) -> Response { let resp = match gateway::io::proxy_mcp_call_sse(&state.client, url, bytes).await { Ok(r) => r, - Err(e) => return sse_error_response(id, -32603, format!("proxy error: {e}")), + Err(e) => return sse_tool_error_response(id, format!("proxy error: {e}")), }; let id_for_error = id; @@ -563,9 +567,8 @@ async fn stream_mcp_call_sse( } } Err(e) => { - let err = JsonRpcResponse::error( + let err = tool_error_json( id_for_error.clone(), - -32603, format!("upstream disconnected: {e}"), ); let data = serde_json::to_string(&err).unwrap_or_default(); @@ -581,6 +584,32 @@ async fn stream_mcp_call_sse( .into_response() } +/// Build the JSON-RPC value used by [`tool_error_response`] and +/// [`sse_tool_error_response`] — extracted so the SSE mid-stream-disconnect +/// branch above can reuse the same non-fatal `isError` shape. +fn tool_error_json(id: Option, message: String) -> JsonRpcResponse { + JsonRpcResponse::success( + id, + json!({ + "content": [{ "type": "text", "text": message }], + "isError": true + }), + ) +} + +/// Build a minimal SSE response containing a single non-fatal MCP tool_result +/// error event (`isError: true`) — the SSE counterpart of +/// [`tool_error_response`], used when the initial proxy connection to the +/// sled fails (story 1232). +fn sse_tool_error_response(id: Option, message: String) -> Response { + let err = tool_error_json(id, message); + let data = serde_json::to_string(&err).unwrap_or_default(); + let stream = async_stream::stream! { + yield Event::message(data); + }; + SSE::new(stream).into_response() +} + /// Build a minimal SSE response containing a single JSON-RPC error event. fn sse_error_response(id: Option, code: i64, msg: String) -> Response { let err = JsonRpcResponse::error(id, code, msg); @@ -1760,9 +1789,14 @@ mod tests { proxy_and_respond_for_project(&state, "test-project", b"{}", Some(json!(1))).await; let bytes = resp.into_body().into_bytes().await.unwrap(); let parsed: Value = serde_json::from_slice(&bytes).unwrap(); - assert_eq!(parsed["error"]["code"], -32603); + // Story 1232: a known project with no live sled connection is a + // tool-call-time failure, not a protocol fault — it must come back as + // a non-fatal tool_result (isError: true), not a top-level JSON-RPC + // error, so the calling `claude` CLI treats it as a normal tool error. + assert!(parsed["error"].is_null(), "got: {parsed}"); + assert_eq!(parsed["result"]["isError"], true, "got: {parsed}"); assert!( - parsed["error"]["message"] + parsed["result"]["content"][0]["text"] .as_str() .unwrap() .contains("no live WS uplink"), @@ -1998,6 +2032,142 @@ mod tests { ); } + // ── story 1232: down/mismatched sled must not surface as a fatal + // top-level JSON-RPC error, which is what makes the calling `claude` CLI + // exit non-zero instead of completing the turn ──────────────────────── + + /// AC 1/2/3: a `tools/call` against a project with no live sled-uplink + /// connection at all (the "unreachable sled" case) must come back as a + /// normal, non-fatal MCP tool_result (`isError: true`), not a top-level + /// JSON-RPC `error` object — the CLI's MCP client treats the latter as a + /// protocol fault rather than something the model can react to. + #[tokio::test] + async fn unreachable_sled_returns_non_fatal_tool_result_not_top_level_error() { + let dir = tempfile::tempdir().unwrap(); + // No sled connection registered for "test-project" — proxy_active_mcp + // must fail with "no live WS uplink connection". + let state = make_test_state(dir.path()); + let app = poem::Route::new() + .at("/mcp", poem::post(gateway_mcp_post_handler)) + .data(state); + let cli = poem::test::TestClient::new(app); + + let rpc_body = serde_json::to_vec(&json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { + "name": "get_pipeline_status", + "arguments": {} + } + })) + .unwrap(); + let resp = cli + .post("/mcp") + .header("content-type", "application/json") + .body(rpc_body) + .send() + .await; + let parsed: Value = resp.0.into_body().into_json().await.unwrap(); + + assert!( + parsed["error"].is_null(), + "an unreachable sled must not produce a top-level JSON-RPC error: {parsed}" + ); + assert_eq!( + parsed["result"]["isError"], true, + "an unreachable sled must produce a tool_result with isError: true: {parsed}" + ); + let text = parsed["result"]["content"][0]["text"].as_str().unwrap(); + assert!( + text.contains("unavailable"), + "tool_result text should explain the sled is unavailable, got: {text}" + ); + } + + /// AC 1/2/3: a sled that IS connected but replies with a malformed or + /// version-mismatched MCP response (not a well-formed JSON-RPC object) + /// must also surface as a non-fatal tool_result, not garbage forwarded + /// verbatim to the calling `claude` CLI's MCP parser. + #[tokio::test] + async fn malformed_sled_response_returns_non_fatal_tool_result() { + let dir = tempfile::tempdir().unwrap(); + let mut projects = BTreeMap::new(); + projects.insert( + "test-project".to_string(), + ProjectEntry::with_url("http://127.0.0.1:3001"), + ); + let config = GatewayConfig { + projects, + sled_tokens: BTreeMap::new(), + release_channels: BTreeMap::new(), + }; + let state = Arc::new(GatewayState::new(config, dir.path().to_path_buf(), 3000).unwrap()); + + // Fake sled that answers every mcp_request with a payload that is + // valid JSON but not a well-formed JSON-RPC response — simulating a + // version-mismatched sled speaking an incompatible protocol shape. + let (tx, mut rx) = + tokio::sync::mpsc::unbounded_channel::(); + let in_flight: Arc< + tokio::sync::Mutex< + std::collections::HashMap>, + >, + > = Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new())); + let in_flight_task = Arc::clone(&in_flight); + tokio::spawn(async move { + while let Some(env) = rx.recv().await { + if let Some(sender) = in_flight_task.lock().await.remove(&env.req_id) { + let _ = sender.send(json!({ "unexpected": "shape", "no_jsonrpc_field": true })); + } + } + }); + state + .register_sled_connection( + "test-project".to_string(), + gateway::SledConnection { + tx, + last_heartbeat_ms: Arc::new(std::sync::atomic::AtomicI64::new( + chrono::Utc::now().timestamp_millis(), + )), + in_flight, + }, + ) + .await; + + let app = poem::Route::new() + .at("/mcp", poem::post(gateway_mcp_post_handler)) + .data(state); + let cli = poem::test::TestClient::new(app); + + let rpc_body = serde_json::to_vec(&json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { + "name": "get_pipeline_status", + "arguments": {} + } + })) + .unwrap(); + let resp = cli + .post("/mcp") + .header("content-type", "application/json") + .body(rpc_body) + .send() + .await; + let parsed: Value = resp.0.into_body().into_json().await.unwrap(); + + assert!( + parsed["error"].is_null(), + "a malformed sled response must not produce a top-level JSON-RPC error: {parsed}" + ); + assert_eq!( + parsed["result"]["isError"], true, + "a malformed sled response must produce a tool_result with isError: true: {parsed}" + ); + } + #[test] fn annotate_resolved_project_inserts_field_into_result_object() { let bytes = serde_json::to_vec(&json!({ diff --git a/server/src/service/gateway/mod.rs b/server/src/service/gateway/mod.rs index 9b2c509f..2628feeb 100644 --- a/server/src/service/gateway/mod.rs +++ b/server/src/service/gateway/mod.rs @@ -160,6 +160,12 @@ pub async fn proxy_mcp_via_ws( let timeout = std::time::Duration::from_millis(MCP_VIA_WS_TIMEOUT_MS); match tokio::time::timeout(timeout, rx).await { Ok(Ok(response_value)) => { + if !is_valid_jsonrpc_response(&response_value) { + return Err( + "sled returned a malformed/incompatible MCP response (possible version mismatch)" + .to_string(), + ); + } serde_json::to_vec(&response_value).map_err(|e| format!("serialise mcp_response: {e}")) } Ok(Err(_)) => Err("sled response channel dropped".to_string()), @@ -172,6 +178,21 @@ pub async fn proxy_mcp_via_ws( } } +/// Returns `true` when `value` looks like a well-formed JSON-RPC 2.0 response +/// (`jsonrpc: "2.0"` plus exactly one of `result`/`error`) — the shape the +/// downstream `claude` CLI's MCP client expects on every `mcp_response`. A +/// down or version-mismatched sled can send something else entirely (an +/// empty object, a request instead of a response, ...); forwarding that +/// verbatim risks the CLI's MCP parser crashing instead of surfacing a +/// normal tool error (story 1232). +fn is_valid_jsonrpc_response(value: &serde_json::Value) -> bool { + let Some(obj) = value.as_object() else { + return false; + }; + obj.get("jsonrpc").and_then(|v| v.as_str()) == Some("2.0") + && (obj.contains_key("result") ^ obj.contains_key("error")) +} + // ── Error type ────────────────────────────────────────────────────────────── /// Typed errors returned by `service::gateway` functions. @@ -880,6 +901,83 @@ mod tests { } } + // ── is_valid_jsonrpc_response / proxy_mcp_via_ws malformed handling + // (story 1232) ──────────────────────────────────────────────────────── + + #[test] + fn is_valid_jsonrpc_response_accepts_well_formed_result() { + assert!(is_valid_jsonrpc_response(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "result": { "content": [] } + }))); + } + + #[test] + fn is_valid_jsonrpc_response_accepts_well_formed_error() { + assert!(is_valid_jsonrpc_response(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "error": { "code": -32603, "message": "boom" } + }))); + } + + #[test] + fn is_valid_jsonrpc_response_rejects_missing_jsonrpc_field() { + assert!(!is_valid_jsonrpc_response(&serde_json::json!({ + "id": 1, + "result": {} + }))); + } + + #[test] + fn is_valid_jsonrpc_response_rejects_missing_result_and_error() { + assert!(!is_valid_jsonrpc_response(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1 + }))); + } + + #[test] + fn is_valid_jsonrpc_response_rejects_non_object() { + assert!(!is_valid_jsonrpc_response(&serde_json::json!( + "not an object" + ))); + assert!(!is_valid_jsonrpc_response(&serde_json::json!(null))); + } + + /// End-to-end: a sled that responds with a payload that is valid JSON but + /// not a well-formed JSON-RPC response (simulating a version-mismatched + /// sled) must make `proxy_mcp_via_ws` return `Err`, not `Ok` with garbage + /// bytes forwarded to the caller. + #[tokio::test] + async fn proxy_mcp_via_ws_errors_on_malformed_sled_response() { + let (tx, mut rx) = mpsc::unbounded_channel::(); + let in_flight: Arc< + TokioMutex>>, + > = Arc::new(TokioMutex::new(HashMap::new())); + let in_flight_task = Arc::clone(&in_flight); + tokio::spawn(async move { + while let Some(env) = rx.recv().await { + if let Some(sender) = in_flight_task.lock().await.remove(&env.req_id) { + let _ = sender.send(serde_json::json!({ "not": "jsonrpc" })); + } + } + }); + let conn = SledConnection { + tx, + last_heartbeat_ms: Arc::new(AtomicI64::new(chrono::Utc::now().timestamp_millis())), + in_flight, + }; + + let result = proxy_mcp_via_ws(&conn, b"{}").await; + let err = result.expect_err("malformed sled response must be surfaced as an error"); + assert!( + err.contains("malformed") || err.contains("incompatible"), + "error should explain the response was malformed/incompatible, got: {err}" + ); + } + #[test] fn gateway_uptime_secs_is_zero_or_positive_immediately_after_start() { // Just ensure it doesn't panic and returns a sane (small) value —