crashlabs/huskies
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 37 Wiki Activity

huskies: merge 1232 bug Gateway chat bot crashes (CLI exit 1) on a sled MCP tool error instead of surfacing it

Browse Source
This commit is contained in:
Huskies Agent
2026-07-19 18:12:33 +00:00
parent 7b3990430e
commit 933fb5a54b
3 changed files with 296 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/gateway/tests.rs
+12 -3
View File
@@ -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}"
server/src/http/gateway/mcp.rs
+186 -16
View File
@@ -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<Value>, 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<Value>) -> 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<Value>, 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<Value>, 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<Value>, 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::<crate::sled_uplink::UplinkEnvelope>();
let in_flight: Arc<
tokio::sync::Mutex<
std::collections::HashMap<String, tokio::sync::oneshot::Sender<Value>>,
>,
> = 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!({
server/src/service/gateway/mod.rs
+98
View File
@@ -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::<crate::sled_uplink::UplinkEnvelope>();
let in_flight: Arc<
TokioMutex<HashMap<String, tokio::sync::oneshot::Sender<serde_json::Value>>>,
> = 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 —