From a3f832130371acd4c94259fb74a059e27ebbfc03 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Fri, 8 May 2026 01:00:39 +0530 Subject: [PATCH] fix(integrations): propagate backend error body in non-2xx responses (#1296) (#1330) Co-authored-by: Claude Opus 4.7 (1M context) --- src/openhuman/composio/client.rs | 15 +- src/openhuman/composio/client_tests.rs | 36 +++- src/openhuman/integrations/client.rs | 69 +++++++- src/openhuman/integrations/client_tests.rs | 186 +++++++++++++++++++++ 4 files changed, 296 insertions(+), 10 deletions(-) create mode 100644 src/openhuman/integrations/client_tests.rs diff --git a/src/openhuman/composio/client.rs b/src/openhuman/composio/client.rs index af30503c7..482e6b421 100644 --- a/src/openhuman/composio/client.rs +++ b/src/openhuman/composio/client.rs @@ -317,15 +317,24 @@ impl ComposioClient { let status = resp.status(); if !status.is_success() { let body_text = resp.text().await.unwrap_or_default(); + let detail = crate::openhuman::integrations::client::extract_error_detail( + &body_text, + crate::openhuman::integrations::client::MAX_ERROR_BODY_LEN, + ); + // Use the same UTF-8-safe truncation for the debug-log preview + // — direct byte-slicing (`&body_text[..len.min(300)]`) panics + // when the cutoff lands inside a multibyte codepoint. + let logged_body = + crate::openhuman::integrations::client::extract_error_detail(&body_text, 300); tracing::debug!( "[composio] DELETE {} → {} body={}", url, status, - &body_text[..body_text.len().min(300)] + logged_body ); let status_str = status.as_u16().to_string(); crate::core::observability::report_error( - format!("Backend returned {} for DELETE {}", status, url).as_str(), + format!("Backend returned {status} for DELETE {url}: {detail}").as_str(), "composio", "delete", &[ @@ -334,7 +343,7 @@ impl ComposioClient { ("failure", "non_2xx"), ], ); - anyhow::bail!("Backend returned {} for DELETE {}", status, url); + anyhow::bail!("Backend returned {status} for DELETE {url}: {detail}"); } let envelope: Envelope = resp.json().await?; diff --git a/src/openhuman/composio/client_tests.rs b/src/openhuman/composio/client_tests.rs index 6494a0151..5672bfccc 100644 --- a/src/openhuman/composio/client_tests.rs +++ b/src/openhuman/composio/client_tests.rs @@ -482,5 +482,39 @@ async fn disable_trigger_surfaces_non_2xx_status() { let base = start_mock_backend(app).await; let client = build_client_for(base); let err = client.disable_trigger("ti_x").await.unwrap_err(); - assert!(err.to_string().contains("404"), "unexpected: {err}"); + let msg = err.to_string(); + assert!(msg.contains("404"), "expected status 404, got: {msg}"); + // Phase A (#1296): raw_delete must propagate the envelope's `error` + // field so callers can tell *why* the backend rejected the call. + assert!( + msg.contains("no"), + "expected envelope error detail in message, got: {msg}" + ); +} + +#[tokio::test] +async fn delete_connection_surfaces_envelope_error_detail() { + // Direct cover of the `raw_delete` envelope-error path used by + // `delete_connection` — proves the backend message ("Connection + // not found") makes it into the propagated bail message rather + // than being discarded with the body. Mirror of the `post`/`get` + // envelope tests in `integrations/client_tests.rs`. + let app = Router::new().route( + "/agent-integrations/composio/connections/{id}", + axum::routing::delete(|Path(_id): Path| async move { + ( + StatusCode::BAD_REQUEST, + Json(json!({"success": false, "error": "Connection not found"})), + ) + }), + ); + let base = start_mock_backend(app).await; + let client = build_client_for(base); + let err = client.delete_connection("missing-id").await.unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("Connection not found"), + "expected backend error detail in message, got: {msg}" + ); + assert!(msg.contains("400"), "expected status 400, got: {msg}"); } diff --git a/src/openhuman/integrations/client.rs b/src/openhuman/integrations/client.rs index c31a1c557..74892e42a 100644 --- a/src/openhuman/integrations/client.rs +++ b/src/openhuman/integrations/client.rs @@ -5,6 +5,57 @@ use std::error::Error as _; use std::sync::Arc; use std::time::Duration; +/// Maximum length (in bytes) of backend error body included in propagated +/// errors. Keep this bounded — error messages flow through tracing/Sentry and +/// are surfaced in user-facing toasts, neither of which want a 100KB blob. +pub(crate) const MAX_ERROR_BODY_LEN: usize = 500; + +/// Extract a human-readable failure detail from a backend error response body. +/// +/// The backend wraps every error response in +/// `{ "success": false, "error": "" }` (see +/// `backend-openhuman/src/middlewares/errorHandler.ts`). When the body parses +/// as that envelope, return the inner `error` string verbatim — it is the +/// authoritative failure message (e.g. `"Insufficient balance"`, +/// `"Toolkit \"X\" is not enabled"`). +/// +/// Otherwise (non-JSON body, missing `error` field) fall back to the raw +/// text truncated to `max_bytes` at a UTF-8 char boundary so callers always +/// get *something* to grep for, without unbounded memory in error paths. +pub(crate) fn extract_error_detail(body: &str, max_bytes: usize) -> String { + if body.is_empty() { + return "".to_string(); + } + if let Ok(v) = serde_json::from_str::(body) { + if let Some(msg) = v.get("error").and_then(|e| e.as_str()) { + let trimmed = msg.trim(); + if !trimmed.is_empty() { + return truncate_at_char_boundary(trimmed, max_bytes); + } + } + } + truncate_at_char_boundary(body, max_bytes) +} + +fn truncate_at_char_boundary(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + // Reserve space for the trailing `…` so the returned string never + // exceeds `max` bytes. Without this, a 500-byte cap could return + // 503 bytes (500 raw + 3-byte ellipsis), breaking the hard cap that + // Sentry tag values and user-facing toasts rely on. + let ellipsis_len = '…'.len_utf8(); + if max < ellipsis_len { + return String::new(); + } + let mut end = max - ellipsis_len; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + format!("{}…", &s[..end]) +} + /// Shared client for all integration tools. Holds backend URL, auth token, /// a reusable `reqwest::Client`, and a lazily-fetched pricing cache. pub struct IntegrationClient { @@ -80,10 +131,11 @@ impl IntegrationClient { let status = resp.status(); if !status.is_success() { - let _body_text = resp.text().await.unwrap_or_default(); + let body_text = resp.text().await.unwrap_or_default(); + let detail = extract_error_detail(&body_text, MAX_ERROR_BODY_LEN); let status_str = status.as_u16().to_string(); crate::core::observability::report_error( - format!("Backend returned {} for POST {}", status, url).as_str(), + format!("Backend returned {status} for POST {url}: {detail}").as_str(), "integrations", "post", &[ @@ -92,7 +144,7 @@ impl IntegrationClient { ("failure", "non_2xx"), ], ); - anyhow::bail!("Backend returned {} for POST {}", status, url); + anyhow::bail!("Backend returned {status} for POST {url}: {detail}"); } let envelope: BackendResponse = resp.json().await?; @@ -143,10 +195,11 @@ impl IntegrationClient { let status = resp.status(); if !status.is_success() { - let _body_text = resp.text().await.unwrap_or_default(); + let body_text = resp.text().await.unwrap_or_default(); + let detail = extract_error_detail(&body_text, MAX_ERROR_BODY_LEN); let status_str = status.as_u16().to_string(); crate::core::observability::report_error( - format!("Backend returned {} for GET {}", status, url).as_str(), + format!("Backend returned {status} for GET {url}: {detail}").as_str(), "integrations", "get", &[ @@ -155,7 +208,7 @@ impl IntegrationClient { ("failure", "non_2xx"), ], ); - anyhow::bail!("Backend returned {} for GET {}", status, url); + anyhow::bail!("Backend returned {status} for GET {url}: {detail}"); } let envelope: BackendResponse = resp.json().await?; @@ -251,3 +304,7 @@ pub fn build_client(config: &crate::openhuman::config::Config) -> Option"); +} + +#[test] +fn extract_error_detail_truncates_long_non_json_bodies_at_char_boundary() { + // Multi-byte UTF-8 (€ = 3 bytes). Building a string longer than `max` + // ensures truncate_at_char_boundary backs off until it lands on a + // valid char boundary instead of slicing inside a code point. + let body = "€".repeat(200); // 600 bytes + let out = extract_error_detail(&body, 50); + assert!(out.ends_with('…'), "expected ellipsis, got: {out}"); + // Hard cap check: the returned string MUST NOT exceed `max` bytes + // including the ellipsis. Earlier the helper appended `…` after + // slicing to `max`, which leaked 3 bytes past the advertised cap; + // CR flagged this. Now the cap is strict. + assert!( + out.len() <= 50, + "output ({} bytes) exceeded advertised cap of 50", + out.len() + ); +} + +#[test] +fn extract_error_detail_with_max_below_ellipsis_returns_empty() { + // Edge case: when `max` is smaller than the ellipsis byte length + // (3 bytes), there's no room for any content + ellipsis, so the + // helper must return an empty string rather than panic or emit a + // partial codepoint. + let body = "€".repeat(10); + assert_eq!(extract_error_detail(&body, 2), ""); +} + +#[test] +fn extract_error_detail_envelope_missing_error_field_falls_back() { + let body = r#"{"success":false}"#; + // No `error` key — fall back to truncated raw body so the caller + // still has *something* to grep for. + assert_eq!(extract_error_detail(body, 500), body); +} + +#[test] +fn extract_error_detail_envelope_blank_error_falls_back() { + let body = r#"{"success":false,"error":" "}"#; + assert_eq!(extract_error_detail(body, 500), body); +} + +// ── Integration: HTTP error propagation through `post`/`get` ────── + +async fn start_mock_backend(app: Router) -> String { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + format!("http://127.0.0.1:{}", addr.port()) +} + +fn client_for(base: String) -> IntegrationClient { + IntegrationClient::new(base, "test-token".into()) +} + +#[tokio::test] +async fn post_400_propagates_backend_error_envelope_message() { + // Mirror the real backend BadRequestError shape from + // `backend-openhuman/src/middlewares/errorHandler.ts` — the 400 + // body is JSON `{ success:false, error:"" }`. + let app = Router::new().route( + "/agent-integrations/composio/execute", + post(|| async { + ( + StatusCode::BAD_REQUEST, + Json(json!({ "success": false, "error": "Insufficient balance" })), + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .post::( + "/agent-integrations/composio/execute", + &json!({ "tool": "GMAIL_FETCH_EMAILS" }), + ) + .await + .expect_err("400 must surface as Err"); + let msg = format!("{err:#}"); + assert!( + msg.contains("Insufficient balance"), + "expected backend error in propagated message, got: {msg}" + ); + assert!(msg.contains("400"), "expected status code, got: {msg}"); +} + +#[tokio::test] +async fn post_500_propagates_html_body_truncated() { + let app = Router::new().route( + "/foo", + post(|| async { + ( + StatusCode::INTERNAL_SERVER_ERROR, + "upstream blew up", + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .post::("/foo", &json!({})) + .await + .expect_err("500 must surface as Err"); + let msg = format!("{err:#}"); + assert!( + msg.contains("upstream blew up"), + "expected raw body in propagated message, got: {msg}" + ); +} + +#[tokio::test] +async fn get_403_propagates_backend_error_envelope_message() { + let app = Router::new().route( + "/agent-integrations/composio/connections", + get(|| async { + ( + StatusCode::FORBIDDEN, + Json(json!({ "success": false, "error": "Toolkit \"x\" is not enabled" })), + ) + .into_response() + }), + ); + let base = start_mock_backend(app).await; + let client = client_for(base); + let err = client + .get::("/agent-integrations/composio/connections") + .await + .expect_err("403 must surface as Err"); + let msg = format!("{err:#}"); + assert!( + msg.contains("Toolkit \"x\" is not enabled"), + "expected backend error in propagated message, got: {msg}" + ); + assert!(msg.contains("403"), "expected status code, got: {msg}"); +}