Merge pull request #108 from ntdatt812/fix/composio-surface-error-body

fix(composio): surface the error body instead of discarding it

Author: senamakel

crates/tinymemory-core/src/sync/pipelines/composio/client.rs

@@ -186,8 +186,8 @@ impl ComposioClient {
             .map_err(|error| anyhow::anyhow!("Composio direct transport error: {error}"))?;
         let status = response.status();
         if !status.is_success() {
-            let _ = response.bytes().await;
-            anyhow::bail!("Composio direct request failed with HTTP {status}");
+            let body = response.text().await.unwrap_or_default();
+            anyhow::bail!(describe_failure("direct", status, &body));
         }
         let raw: serde_json::Value = decode_response(response, "direct").await?;
         Ok(decode_direct_response(raw))
@@ -218,8 +218,8 @@ impl ComposioClient {
             .map_err(|error| anyhow::anyhow!("Composio proxy transport error: {error}"))?;
         let status = response.status();
         if !status.is_success() {
-            let _ = response.bytes().await;
-            anyhow::bail!("Composio proxy request failed with HTTP {status}");
+            let body = response.text().await.unwrap_or_default();
+            anyhow::bail!(describe_failure("proxy", status, &body));
         }
         let raw: serde_json::Value = response
             .json()
@@ -287,17 +287,102 @@ fn retryable_provider_error(error: Option<&str>) -> bool {
 /// needle that both status-bail messages also matched.
 fn retryable_transport_error(error: &anyhow::Error) -> bool {
     let message = error.to_string();
+    // Anchored on the status clause this module actually emits. A bare
+    // "HTTP 429" needle would now be forgeable: the failure message carries the
+    // response body, and a body that merely mentions another status must not
+    // turn a permanent 400 into a retry.
     [
-        "HTTP 429",
-        "HTTP 502",
-        "HTTP 503",
-        "HTTP 504",
+        "failed with HTTP 429",
+        "failed with HTTP 502",
+        "failed with HTTP 503",
+        "failed with HTTP 504",
         "transport error",
     ]
     .iter()
     .any(|needle| message.contains(needle))
 }
 
+/// Longest body snippet echoed for a response whose shape we do not recognise.
+const FAILURE_BODY_LIMIT: usize = 400;
+
+/// Describe a non-2xx Composio response, using the body rather than throwing it away.
+///
+/// Composio answers with a structured error whose `message` and `suggested_fix`
+/// name the actual problem and how to correct it — an entity-id mismatch says
+/// which id to use instead. Discarding it left callers with a bare status line
+/// and no route to a fix.
+///
+/// The `failed with HTTP {status}` clause is load-bearing: [`retryable_transport_error`]
+/// keys off it, so it stays first and stays verbatim.
+///
+/// Only the known error fields are surfaced. An unrecognised body is truncated
+/// instead of echoed whole, so an unexpected payload cannot pour arbitrary
+/// content into logs.
+fn describe_failure(surface: &str, status: reqwest::StatusCode, body: &str) -> String {
+    let head = format!("Composio {surface} request failed with HTTP {status}");
+    match failure_detail(body) {
+        Some(detail) => format!("{head}: {detail}"),
+        None => head,
+    }
+}
+
+/// Pull the human-meaningful part out of a Composio error body.
+fn failure_detail(body: &str) -> Option<String> {
+    let trimmed = body.trim();
+    if trimmed.is_empty() {
+        return None;
+    }
+
+    if let Ok(parsed) = serde_json::from_str::<serde_json::Value>(trimmed) {
+        if let Some(detail) = structured_detail(&parsed) {
+            return Some(detail);
+        }
+    }
+
+    Some(truncate(trimmed, FAILURE_BODY_LIMIT))
+}
+
+/// `{"error": {"message": .., "slug": .., "suggested_fix": ..}}`, or a bare
+/// `{"error": "..."}`.
+fn structured_detail(parsed: &serde_json::Value) -> Option<String> {
+    let error = parsed.get("error")?;
+
+    if let Some(text) = error.as_str() {
+        let text = text.trim();
+        return (!text.is_empty()).then(|| truncate(text, FAILURE_BODY_LIMIT));
+    }
+
+    let field = |name: &str| {
+        error
+            .get(name)
+            .and_then(serde_json::Value::as_str)
+            .map(str::trim)
+            .filter(|value| !value.is_empty())
+    };
+
+    let message = field("message")?;
+    let mut detail = truncate(message, FAILURE_BODY_LIMIT);
+    if let Some(slug) = field("slug") {
+        detail.push_str(&format!(" [{slug}]"));
+    }
+    if let Some(fix) = field("suggested_fix") {
+        detail.push_str(&format!(
+            " — suggested fix: {}",
+            truncate(fix, FAILURE_BODY_LIMIT)
+        ));
+    }
+    Some(detail)
+}
+
+/// Cut on a char boundary so a multi-byte body cannot panic the error path.
+fn truncate(value: &str, limit: usize) -> String {
+    if value.chars().count() <= limit {
+        return value.to_owned();
+    }
+    let kept: String = value.chars().take(limit).collect();
+    format!("{kept}…")
+}
+
 async fn decode_response(
     response: reqwest::Response,
     mode: &str,

crates/tinymemory-core/src/sync/pipelines/composio/client_tests.rs

@@ -80,3 +80,84 @@ fn flat_proxy_response_remains_supported() {
     assert!(response.successful);
     assert_eq!(response.data["items"], serde_json::json!([1]));
 }
+
+/// The failure message now carries the response body, so a body that merely
+/// mentions another status must not turn a permanent failure into a retry.
+/// This is the hazard the needles were tightened against.
+#[test]
+fn a_surfaced_body_cannot_forge_a_retryable_status() {
+    let retry = |m: &str| retryable_transport_error(&anyhow::anyhow!("{m}"));
+    assert!(!retry(
+        "Composio direct request failed with HTTP 400 Bad Request: upstream said HTTP 503"
+    ));
+    assert!(!retry(
+        "Composio proxy request failed with HTTP 401 Unauthorized: retry after HTTP 429"
+    ));
+    // The real ones still classify.
+    assert!(retry(
+        "Composio direct request failed with HTTP 429 Too Many Requests: slow down"
+    ));
+}
+
+/// Composio's structured error names the problem and how to fix it. This is the
+/// payload from the report, verbatim.
+#[test]
+fn a_structured_error_body_reaches_the_message() {
+    let body = r#"{"error":{"message":"Connected account user ID does not match the provided user ID.","code":1812,"slug":"ActionExecute_ConnectedAccountEntityIdMismatch","status":400,"suggested_fix":"The connected_account_id you provided belongs to a different entity."}}"#;
+    let message = describe_failure("direct", reqwest::StatusCode::BAD_REQUEST, body);
+
+    assert!(
+        message.starts_with("Composio direct request failed with HTTP 400"),
+        "the status clause must stay first and verbatim: {message}"
+    );
+    assert!(message.contains("Connected account user ID does not match"));
+    assert!(message.contains("ActionExecute_ConnectedAccountEntityIdMismatch"));
+    assert!(
+        message.contains("belongs to a different entity"),
+        "the suggested fix is the part that turns a dead end into an action: {message}"
+    );
+}
+
+/// A bare `{"error": "..."}` string body is the other shape Composio returns.
+#[test]
+fn a_bare_error_string_body_reaches_the_message() {
+    let body = r#"{"error":"You have exceeded your credits limit.","tag":"NO_MORE_CREDITS"}"#;
+    let message = describe_failure("proxy", reqwest::StatusCode::PAYMENT_REQUIRED, body);
+    assert!(message.contains("exceeded your credits limit"), "{message}");
+}
+
+/// An unrecognised body is echoed but bounded, so an unexpected payload cannot
+/// pour arbitrary content into the logs.
+#[test]
+fn an_unrecognised_body_is_truncated() {
+    let body = "x".repeat(5_000);
+    let message = describe_failure("direct", reqwest::StatusCode::BAD_GATEWAY, &body);
+    assert!(
+        message.contains('…'),
+        "expected an elision marker: {message}"
+    );
+    assert!(
+        message.chars().count() < 600,
+        "the message grew to {} chars",
+        message.chars().count()
+    );
+}
+
+/// Truncation must cut on a char boundary — a multi-byte body must not panic
+/// the error path.
+#[test]
+fn truncation_survives_multibyte_bodies() {
+    let body = "é".repeat(5_000);
+    let message = describe_failure("direct", reqwest::StatusCode::BAD_GATEWAY, &body);
+    assert!(message.contains('…'), "{message}");
+}
+
+/// No body, no change: the status line stands on its own as before.
+#[test]
+fn an_empty_body_leaves_the_status_line_alone() {
+    let message = describe_failure("direct", reqwest::StatusCode::NOT_FOUND, "   ");
+    assert_eq!(
+        message,
+        "Composio direct request failed with HTTP 404 Not Found"
+    );
+}