diff --git a/crates/tinymemory-core/src/sync/pipelines/composio/client.rs b/crates/tinymemory-core/src/sync/pipelines/composio/client.rs index b7505f7..a3dd900 100644 --- a/crates/tinymemory-core/src/sync/pipelines/composio/client.rs +++ b/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 { + let trimmed = body.trim(); + if trimmed.is_empty() { + return None; + } + + if let Ok(parsed) = serde_json::from_str::(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 { + 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, diff --git a/crates/tinymemory-core/src/sync/pipelines/composio/client_tests.rs b/crates/tinymemory-core/src/sync/pipelines/composio/client_tests.rs index 8ce7e6f..2209586 100644 --- a/crates/tinymemory-core/src/sync/pipelines/composio/client_tests.rs +++ b/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" + ); +}