fix(integrations): propagate backend error body in non-2xx responses (#1296) (#1330)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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<T> = resp.json().await?;

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<String>| 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}");
 }

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": "<msg>" }` (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 "<empty body>".to_string();
+    }
+    if let Ok(v) = serde_json::from_str::<serde_json::Value>(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<T> = 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<T> = resp.json().await?;
@@ -251,3 +304,7 @@ pub fn build_client(config: &crate::openhuman::config::Config) -> Option<Arc<Int
         }
     }
 }
+
+#[cfg(test)]
+#[path = "client_tests.rs"]
+mod tests;

src/openhuman/integrations/client_tests.rs

@@ -0,0 +1,186 @@
+//! Tests for the shared integrations HTTP client.
+//!
+//! Focus: backend error body propagation. Pre-fix, non-2xx responses
+//! discarded the body (`let _body_text = …`) leaving callers with a
+//! generic `"Backend returned 400 …"` message — see #1296. These tests
+//! lock in the new behaviour where `extract_error_detail` pulls the
+//! envelope's `error` field (or falls back to truncated raw text) and
+//! the bail message includes it.
+
+use super::*;
+use axum::{
+    http::StatusCode,
+    response::IntoResponse,
+    routing::{get, post},
+    Json, Router,
+};
+use serde_json::json;
+
+// ── Unit: `extract_error_detail` ──────────────────────────────────
+
+#[test]
+fn extract_error_detail_envelope_returns_inner_message() {
+    let body = r#"{"success":false,"error":"Insufficient balance"}"#;
+    assert_eq!(extract_error_detail(body, 500), "Insufficient balance");
+}
+
+#[test]
+fn extract_error_detail_envelope_trims_whitespace() {
+    let body = r#"{"success":false,"error":"   Toolkit \"foo\" is not enabled   "}"#;
+    assert_eq!(
+        extract_error_detail(body, 500),
+        "Toolkit \"foo\" is not enabled"
+    );
+}
+
+#[test]
+fn extract_error_detail_falls_back_for_non_json_body() {
+    let body = "<html>500 internal error</html>";
+    assert_eq!(extract_error_detail(body, 500), body);
+}
+
+#[test]
+fn extract_error_detail_handles_empty_body() {
+    assert_eq!(extract_error_detail("", 500), "<empty body>");
+}
+
+#[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:"<msg>" }`.
+    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::<serde_json::Value>(
+            "/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,
+                "<html>upstream blew up</html>",
+            )
+                .into_response()
+        }),
+    );
+    let base = start_mock_backend(app).await;
+    let client = client_for(base);
+    let err = client
+        .post::<serde_json::Value>("/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::<serde_json::Value>("/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}");
+}