fix(composio): surface the error body instead of discarding it - #108
fix(composio): surface the error body instead of discarding it#108ntdatt812 wants to merge 2 commits into
Conversation
Both Composio request paths read the response body and threw it away:
let _ = response.bytes().await;
anyhow::bail!("Composio direct request failed with HTTP {status}");
Composio answers a failure with a structured error whose `message` and
`suggested_fix` name the actual problem and how to correct it. An entity-id
mismatch, for instance, says which id to use. Callers got a bare status line
and no route to a fix.
`describe_failure` now builds the message from the body, for the direct and
proxied paths alike. Only the known error fields are surfaced; an unrecognised
body is truncated rather than echoed whole, and truncation cuts on a char
boundary so a multi-byte body cannot panic the error path.
The retry needles are tightened in the same change, and that is not
housekeeping. `retryable_transport_error` classifies by substring, so once the
message carries the response body, a body that merely mentions "HTTP 503" would
turn a permanent 400 into a retry. The needles are now anchored on the
`failed with HTTP <code>` clause this module emits, which a body cannot forge.
`a_surfaced_body_cannot_forge_a_retryable_status` pins exactly that: with the
old loose needles it fails (10 passed / 1 failed), so surfacing the body
without this would have been a regression.
The existing `retry_classification_is_by_status_not_by_substring` still passes
unchanged — its own name is the property being strengthened here.
`cargo test -p tinymemory-core composio::client` — 11 passed.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
How this change flows3 changed behaviours across 10 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 23 further behaviours left out to keep the diagram readable. flowchart LR
n0["ComposioClient<br/>changed"]:::changed
n1["retryable_provider_error<br/>changed"]:::changed
n2["flat_proxy_response_remains_supported<br/>changed"]:::changed
n3["ActionExecutor"]:::impacted
n4["ExecuteResponse"]:::impacted
n5["execute"]:::impacted
n6["execute_direct"]:::impacted
n7["Result"]:::impacted
n8["assert"]:::impacted
n0 -->|implements| n3
n2 -->|calls| n8
n3 -->|uses| n4
n3 -->|uses| n7
n5 -->|calls| n1
n5 -->|uses| n4
n5 -->|calls| n6
n5 -->|uses| n7
n6 -->|uses| n4
n6 -->|uses| n7
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
Three spots in the previous commit were over the width rustfmt wants and would have failed the format gate.
|
Pushed
|
Both Composio request paths read the response body and threw it away:
Composio answers a failure with a structured error whose
messageandsuggested_fixname the actual problem and how to correct it. Reported downstream as tinyhumansai/openhuman#5731, where an entity-id mismatch surfaced as a bareHTTP 400 Bad Requestwhile the discarded body said:{"error":{"message":"Connected account user ID does not match the provided user ID.…", "slug":"ActionExecute_ConnectedAccountEntityIdMismatch", "suggested_fix":"The connected_account_id you provided belongs to a different entity.…"}}describe_failurenow builds the message from the body, for the direct and proxied paths alike — both discarded it, so one helper covers both.Only the known error fields are surfaced; an unrecognised body is truncated rather than echoed whole, and truncation cuts on a char boundary so a multi-byte body cannot panic the error path (there is a test for that).
The part that is not housekeeping
retryable_transport_errorclassifies by substring on the message. Once the message carries the response body, a body that merely mentionsHTTP 503would turn a permanent 400 into a retry — the exact class of bug the function's own doc comment records having fixed once already:So the needles are now anchored on the
failed with HTTP <code>clause this module emits, which a response body cannot forge.This is verified, not asserted.
a_surfaced_body_cannot_forge_a_retryable_statusfails against the old loose needles:Surfacing the body without this change would have been a regression.
The existing
retry_classification_is_by_status_not_by_substringpasses unchanged — its own name is the property being strengthened here, so I kept it exactly as it was rather than editing it to fit.Tests
Six new, alongside the five existing:
a_surfaced_body_cannot_forge_a_retryable_statusa_structured_error_body_reaches_the_messagea_bare_error_string_body_reaches_the_message{"error": "…"})an_unrecognised_body_is_truncatedtruncation_survives_multibyte_bodiesan_empty_body_leaves_the_status_line_alonecargo test -p tinymemory-core composio::client— 11 passed.Scope
This is the first of the two independent improvements the issue proposes — surfacing the body. It deliberately does not attempt the second (resolving the entity from
/connected_accountsinstead of trustingentity_id), which changes request construction and deserves its own review.