Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 93 additions & 8 deletions crates/tinymemory-core/src/sync/pipelines/composio/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand Down
81 changes: 81 additions & 0 deletions crates/tinymemory-core/src/sync/pipelines/composio/client_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}