fix(observability,composio): demote direct-mode Composio 401/Invalid API key noise (Sentry TAURI-RUST-X9)#2481
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughComposio direct-mode authentication failures marked with ChangesComposio Direct-Mode Auth Observability
sequenceDiagram
participant Client as Client (direct mode)
participant ComposioOp as composio op (list/authorize/tools)
participant Reporter as report_composio_op_error
participant Classifier as is_provider_user_state_message
participant Sentry as Sentry/Observability
Client->>ComposioOp: invoke direct operation
ComposioOp->>Reporter: report_composio_op_error(operation, &error)
Reporter->>Classifier: pass rendered error message for classification
Classifier->>Sentry: classification result (e.g., ProviderUserState) forwarded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Heads-up to reviewers: the 3 frontend failures on this PR ( Root cause: PR #2378 (Add German locale support, merged 2026-05-21) shipped without 20 keys that PR #2280 ( Verification:
This PR's own quality gates are clean:
Happy to rebase once a German-backfill PR lands on Sentry-Issue: TAURI-RUST-X9 (15,732 events / 22h, single user, |
|
@oxoxDev this PR has merge conflicts with main — please rebase/resolve before review. |
b21d43b to
d1d8f1d
Compare
…ites (Sentry TAURI-RUST-X9) Direct mode shipped via PR tinyhumansai#1825 (2026-05-15) but the mode-aware error paths in `src/openhuman/composio/{ops,tools,periodic}.rs` only routed the backend branch through `report_composio_op_error`. The `ComposioClientKind::Direct` branches wrapped the inner error with `format!("[composio-direct] … failed: {e:#}")` and skipped the classifier hook entirely, so the dominant failure shape `Composio v3 connected_accounts failed: HTTP 401: Invalid API key: ak_…` bypassed `report_error_or_expected` and leaked to Sentry on every UI 5 s poll plus every server-side `periodic.rs` tick. Sentry TAURI-RUST-X9 captured 15,732 events in ~22 h from a single user. This commit restores symmetric error routing — each direct-mode `.map_err` closure now calls `report_composio_op_error("<op>", &e)` with the same op-name string the backend branch uses. The `report_composio_op_error` helper visibility is widened from private to `pub(super)` so `tools.rs` and `periodic.rs` can reuse it without inlining the classifier logic. Patch 2 (next commit) adds the matching arm to `expected_error_kind` so the wire shape demotes to `ProviderUserState` (info breadcrumb, no Sentry event) instead of escaping as an unclassified error. Files touched: - src/openhuman/composio/ops.rs — direct branches in `composio_list_connections`, `composio_authorize`, and both `composio_list_tools` call sites (prefetch + `list_tools` itself). `report_composio_op_error` visibility widened to `pub(super)`. - src/openhuman/composio/tools.rs — `ComposioListConnectionsTool::execute` direct branch. (The other tool `execute` direct branches either refuse the verb or return an empty response with no network call, so no hook to add.) - src/openhuman/composio/periodic.rs — `run_one_tick` direct branch on the server-side scheduler. Sentry-Issue: TAURI-RUST-X9
…as expected user-state (Sentry TAURI-RUST-X9) Adds a new arm to `is_provider_user_state_message` that recognizes the direct-mode composio v3 auth-rejection wire shape: `[composio-direct] <op> failed: Composio v3 … failed: HTTP 401: Invalid API key: ak_…` The matcher is gated on a combined anchor — the `[composio-direct]` prefix (added by ops.rs / tools.rs / periodic.rs direct branches in the previous commit) AND one of (`HTTP 401`, `Invalid API key`). This discriminates against backend-mode 401s (which carry the `Backend returned 401` shape and route through the failure-tag flow with `status="401"`) while still catching every direct-mode call site that emits the new prefix. The arm routes to the existing `ProviderUserState` variant — the same bucket used for the composio toolkit-not-enabled / OAuth-scope cases — so the wire shape demotes to an info breadcrumb via `report_expected_message` with `kind="provider_user_state"` instead of firing a Sentry event. Together with the symmetric `report_composio_op_error` hooks added in the previous commit, this closes the leak for Sentry TAURI-RUST-X9 (~15.7 k events / ~22 h on a single user with a bad direct-mode key). Sentry-Issue: TAURI-RUST-X9
…er + ops routing Adds five tests in `src/core/observability.rs` and four tests in `src/openhuman/composio/ops_test.rs` that lock the TAURI-RUST-X9 (tinyhumansai#1166) leak-fix in place. ## Observability classifier tests - `classifies_composio_direct_invalid_api_key_as_provider_user_state` pins the verbatim Sentry title body (`[composio-direct] list_connections failed: Composio v3 connected_accounts failed: HTTP 401: Invalid API key: ak_VsUvq*****`) to `Some(ProviderUserState)` so the dominant wire shape demotes to an info breadcrumb. - `classifies_composio_direct_invalid_api_key_for_other_ops` covers the parallel direct-mode op-name prefixes (`list_tools` prefetch, `authorize`, `list_tools`) so a future op added to the family doesn't silently bypass the matcher. - `classifies_composio_direct_with_invalid_api_key_only_no_http_401` pins the OR semantics of the combined anchor — body matches either `HTTP 401` or `Invalid API key`. - `does_not_classify_unrelated_http_401_as_composio_direct_user_state` is the discrimination test: backend-mode 401 (`Backend returned 401 …`) and unrelated provider 401s (`GitHub API error: HTTP 401`) MUST NOT match the new arm. - `does_not_classify_composio_direct_500_as_user_state` pins that a real bug shape (500 with no auth body) still falls through to `None`, preserving Sentry signal for genuine backend faults. ## Composio ops tests - `composio_direct_invalid_api_key_classifies_as_provider_user_state` re-asserts the classifier contract from the composio side so a future classifier regression surfaces in both crates. - `composio_direct_invalid_api_key_failure_tag_is_non_2xx` pins the failure-tag side of the path: even if the classifier ever stops matching, the tag stays `non_2xx` so the `before_send` filter has a consistent input. - `composio_direct_invalid_api_key_extract_status_returns_none` pins the contract that `extract_backend_returned_status` only parses the integrations-layer `Backend returned <status>` rendering, NOT the direct-mode `HTTP 401` shape. Documents the boundary so a future extension to cover both shapes comes with its own test. - `composio_direct_500_does_not_demote` re-asserts the discrimination contract from the composio side. ## Spy infrastructure (deviation from plan) The plan suggested extending an `report_composio_op_error` spy in ops_test.rs to assert the hook was invoked on the direct branch. No such spy infrastructure exists in the current codebase — the existing ops_test pinning tests only exercise `classify_composio_failure_tag` and `extract_backend_returned_status` directly. Rather than introduce a Sentry test client + mock plumbing for this single commit, the tests above pin the **observable contract** (`expected_error_kind` for the canonical Sentry body + `classify_composio_failure_tag` + status extraction) which is what the leak-fix actually changes from the caller's perspective. The direct-branch call sites were verified by inspection in patch 1 of tinyhumansai#1166 (every `.map_err` on the direct path now calls `report_composio_op_error(<op>, &e)`). Sentry-Issue: TAURI-RUST-X9
d1d8f1d to
549a4ad
Compare
|
Rebased onto upstream/main New tip:
PR now MERGEABLE. The 3 frontend failures persist — confirmed pre-existing main breakage per earlier comment; main |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/composio/ops.rs`:
- Around line 246-255: The report_composio_op_error calls in the direct-mode
branches are logging the original error before adding the “[composio-direct]”
anchor, so the classifier misses demotion; update each direct-mode error mapping
(the ones wrapping direct_list_connections, authorize, the direct list_tools
prefetch and the direct list_tools call) to first prepend or format the error
with the “[composio-direct]” anchor and then pass that prefixed error into
report_composio_op_error, e.g. create a prefixed_error =
format!("[composio-direct] {e:#}") (or otherwise attach the anchor) and call
report_composio_op_error("list_connections", &prefixed_error) before returning
the formatted error string; apply the same pattern to the authorize and
list_tools direct branches referenced in the comment.
In `@src/openhuman/composio/periodic.rs`:
- Around line 178-190: The periodic Direct branch (ComposioClientKind::Direct)
currently calls super::ops::report_composio_op_error with the raw error `e`,
which bypasses the composio-direct classifier; instead, build a rendered message
containing the `[composio-direct]` anchor (e.g. let msg =
format!("[composio-direct] list_connections (direct): {e:#}")), call
super::ops::report_composio_op_error("list_connections", &msg) with that string,
and then return Err(msg) (i.e. propagate the same rendered message via the
map_err return) so the periodic 401s are classified the same as UI failures;
update the direct_list_connections(direct).await.map_err closure to construct
and use that anchored message rather than passing `e` directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eea3e84a-6e18-472c-9876-5c17f589b4a2
📒 Files selected for processing (5)
src/core/observability.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/periodic.rssrc/openhuman/composio/tools.rs
…e reporting The `report_composio_op_error` hook in tinyhumansai#1166 was called with the unprefixed inner error, so the classifier arm in `is_provider_user_state_message` that gates on `[composio-direct]` never matched — the demotion fix only worked in the assert-side tests, not in the live error path. Render the full `[composio-direct] …` message FIRST, pass that to the reporter, then return the same string. Applies to all 5 direct-mode call sites: - ops.rs: list_connections, authorize, list_tools prefetch, list_tools - periodic.rs: run_one_tick list_connections (direct) - tools.rs: ComposioListConnectionsTool::execute direct branch Addresses CodeRabbit comments r3288995437 and r3288995447 on PR tinyhumansai#2481.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/composio/tools.rs`:
- Around line 512-519: The formatted direct-mode error string loses inner error
details because it uses "{e}" when building `rendered` in the
`composio_list_connections` failure path; change the interpolation to use the
alternate/debug display "{e:#}" so the full error chain (e.g. "HTTP 401: Invalid
API key ...") is preserved before calling
`super::ops::report_composio_op_error("list_connections", &rendered)` and
returning `anyhow::anyhow!("{rendered}")`, ensuring the reported message
includes the inner anchors expected by the observability tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30b69478-d0a7-4273-8350-39f83965192b
📒 Files selected for processing (4)
src/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/composio/periodic.rssrc/openhuman/composio/tools.rs
…ering
Use `{e:#}` instead of `{e}` so the inner anyhow chain (carrying
`HTTP 401: Invalid API key …`) survives into the rendered message that
`report_composio_op_error` passes to the classifier. Matches the other
4 direct-mode call sites in ops.rs and periodic.rs.
Addresses CodeRabbit comment r3291771563 on PR tinyhumansai#2481.
Summary
expected_error_kindarm for the canonical[composio-direct] … HTTP 401 / Invalid API keyshape so user-supplied bad API keys land atdebuginstead of Sentry.TAURI-RUST-X9(15,732 events in ~22h from a single user onopenhuman@0.54.0).Problem
Composio direct mode (shipped via #1825, 2026-05-15) added per-tenant
list_connections/authorize/list_toolscall sites on the agent-integrations path, but the direct branches did not wire their error closures throughreport_composio_op_error()the way the backend branches do. The classifier ladder insrc/core/observability.rs::expected_error_kindwas therefore never consulted for direct-mode errors, so an invalid user API key (ak_…) returned a raw 401 to the JSON-RPC layer where it captured to Sentry on every 5 s UI poll and every server-sidecomposio::periodictick.Result: Sentry
TAURI-RUST-X9accumulated 15,732 events in ~22 hours from a single affected user (releaseopenhuman@0.54.0+c25fc8e5fd3e). Bug-shape was user-config (their own key was bad), not a runtime fault — it is exactly the kind of shape the existing classifier ladder is designed to demote.Two compounding gaps:
.map_errclosures never calledreport_composio_op_erroreven though the backend branch in the same handler did. The classifier never had a chance to see the message.expected_error_kindhad no arm for the[composio-direct] … HTTP 401 / Invalid API keyshape, so the message would still have classified as a real error → Sentry capture.Solution
Three micro-commits, each independently revertible.
Commit 1 —
fix(composio): restore symmetric reportingAdd
report_composio_op_error(op, &e)to every direct-mode.map_errclosure that was missing it, mirroring the backend branch pattern exactly:src/openhuman/composio/ops.rs—composio_list_connections(direct branch),composio_authorize(direct branch),composio_list_toolsprefetch + main call (direct branches). Widenedreport_composio_op_errorfrom private topub(super)so siblings (tools, periodic) can call it.src/openhuman/composio/tools.rs—ComposioListConnectionsTool::executedirect branch (the only tool-side direct branch that issues a network call against the user's tenant).src/openhuman/composio/periodic.rs—run_one_tickdirect branch.composio_delete_connectionalready routes through the unifiedresolve_clientpath, so the single.map_erratops.rs:362covers both modes; no change needed.Commit 2 —
fix(observability): new classifier armAdd a matcher in
is_provider_user_state_message(src/core/observability.rs) that recognizes the[composio-direct]caller prefix combined with one ofHTTP 401/Invalid API key. Routes to the existingExpectedErrorKind::ProviderUserStatevariant (no new enum variants), which downgrades todebuglog and skips Sentry.The combined prefix anchor (
[composio-direct]is unique to our own callers insrc/openhuman/composio/*) keeps the matcher discriminating — unrelated 401s elsewhere in the codebase are unaffected.Commit 3 —
test: pin the contractsrc/core/observability.rscovering: canonical Sentry-body match, sibling op-name match,Invalid API keywithoutHTTP 401, negative case (unrelated 401 stays unclassified), negative case (composio-direct 500 stays unclassified).src/openhuman/composio/ops_test.rspinning the classifier + failure-tag (non_2xx) + status-extraction contract on the canonical body, plus a 500-doesn't-demote regression test.Rejected alternatives
[composio-direct]log line entirely. Hurts debuggability when users report "integrations missing". Demoting todebugvia the expected-kind branch is the right trade.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.sentry.tinyhumans.aiissueTAURI-RUST-X9); no GitHub issue to close.Sentry-Issue:header in## Relatedso the post-merge resolver hook can flip it.Impact
src/openhuman/composio/*,src/core/observability.rs). No frontend / Tauri shell changes. Desktop only.tauri-rustproject leak (15.7 k events / 22 h from a single user) and pre-empts the same shape from every other direct-mode call site that was bypassing the classifier.Related
composio::periodic::run_one_tickalso lacks thereport_composio_op_errorhook on itslist_connections (backend)arm. Out of scope here (plan was direct-only) but worth a follow-up issue.Sentry-Issue: TAURI-RUST-X9
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/sentry-1166-composio-direct-auth-symmetryb21d43be(tip — 3 micro-commits ahead ofupstream/main@c204a53d)Validation Run
app/changes in this PR (Rust core + observability only).cargo test -p openhuman --lib openhuman::composio::ops(61 passed),cargo test -p openhuman --lib core::observability(84 passed),cargo test -p openhuman --lib openhuman::composio::periodic(6 passed),cargo test -p openhuman --lib openhuman::composio::tools(31 passed).cargo fmt --checkclean;cargo check --manifest-path Cargo.tomlclean.cargo check --manifest-path app/src-tauri/Cargo.tomlclean (no Tauri-shell changes in this PR; checked for parity).Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Invalid API keyerrors no longer reach Sentry. They surface indebuglogs and the existing user-visible "not connected" UI state remains the actionable surface for the user.Parity Contract
report_composio_op_error→report_error_or_expected→expected_error_kindpipeline that the backend branch already used is now applied uniformly to the direct branch.report_composio_op_errorpinning tests atops_test.rs:1376+still pass. The new arm inis_provider_user_state_messagereuses the existingExpectedErrorKind::ProviderUserStatevariant — no enum surface change, no dispatcher branches changed.Summary by CodeRabbit
Bug Fixes
Tests