fix(composio): avoid nested auth retry#1791
Conversation
📝 WalkthroughWalkthroughPost-OAuth retry logic is centralized into ChangesComposio post-OAuth retry refactoring
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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/composio/auth_retry.rs (1)
57-60: ⚡ Quick winLog the empty-slug rejection path.
This new validation branch bails without a grep-friendly diagnostic, so whitespace-only slugs will be harder to trace than the rest of the flow. Add a
[composio][auth_retry]debug/trace event before returning.Proposed fix
let tool = slug.trim(); if tool.is_empty() { + tracing::debug!( + target: "composio", + raw_slug_len = slug.len(), + "[composio][auth_retry] rejecting empty tool slug" + ); anyhow::bail!("composio.execute_tool: tool slug must not be empty"); }As per coding guidelines,
src/**/*.rs: "uselog/tracingatdebugortracelevel for development-oriented diagnostics on new/changed flows, including logs at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths".🤖 Prompt for 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. In `@src/openhuman/composio/auth_retry.rs` around lines 57 - 60, The branch that trims and rejects an empty tool slug currently bails silently; add a tracing call before the bail so the rejection is grep-able and follows logging guidelines: in the auth_retry.rs code where slug is trimmed into let tool = slug.trim(); and before anyhow::bail!("composio.execute_tool: tool slug must not be empty"); emit a trace or debug event (e.g., tracing::debug! or tracing::trace!) with a clear tag like "[composio][auth_retry]" and include the original slug (or its whitespace-only indication) to aid debugging, then proceed to bail as before.
🤖 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.
Nitpick comments:
In `@src/openhuman/composio/auth_retry.rs`:
- Around line 57-60: The branch that trims and rejects an empty tool slug
currently bails silently; add a tracing call before the bail so the rejection is
grep-able and follows logging guidelines: in the auth_retry.rs code where slug
is trimmed into let tool = slug.trim(); and before
anyhow::bail!("composio.execute_tool: tool slug must not be empty"); emit a
trace or debug event (e.g., tracing::debug! or tracing::trace!) with a clear tag
like "[composio][auth_retry]" and include the original slug (or its
whitespace-only indication) to aid debugging, then proceed to bail as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54bb7193-305a-4e5d-a508-207b047a655c
📒 Files selected for processing (4)
src/openhuman/composio/auth_retry.rssrc/openhuman/composio/auth_retry_tests.rssrc/openhuman/composio/client.rssrc/openhuman/composio/client_tests.rs
💤 Files with no reviewable changes (1)
- src/openhuman/composio/auth_retry_tests.rs
Summary
execute_tool_with_post_oauth_retrytest-visible and preserve gateway error matching for wrapped/case-variedConnection error, try to authenticatepayloads.Test Plan
RUSTC=$(rustup which --toolchain 1.93.0 rustc) $(rustup which --toolchain 1.93.0 cargo) fmt --allRUSTC=$(rustup which --toolchain 1.93.0 rustc) $(rustup which --toolchain 1.93.0 cargo) test --manifest-path Cargo.toml -p openhuman openhuman::composio::auth_retry -- --nocaptureRUSTC=$(rustup which --toolchain 1.93.0 rustc) $(rustup which --toolchain 1.93.0 cargo) test --manifest-path Cargo.toml -p openhuman openhuman::composio::client -- --nocaptureRUSTC=$(rustup which --toolchain 1.93.0 rustc) $(rustup which --toolchain 1.93.0 cargo) test --manifest-path Cargo.toml -p openhuman openhuman::composio -- --nocaptureSummary by CodeRabbit
Bug Fixes
Tests