fix(composio): retry post-oauth action readiness error

[Zavianx]

Summary

• retry Composio action execution once when the post-OAuth readiness error is returned
• keep real auth/scope failures unchanged by matching only Connection error, try to authenticate
• add client tests for the retry path and non-retry auth error path

Fixes #1688

Tests

• cargo fmt --all -- --check
• cargo clippy -p openhuman
• cargo test -p openhuman
• cargo llvm-cov -p openhuman --lcov --output-path /tmp/lcov-core.info
• git diff --check
• pre-push: pnpm format:check, lint, compile, rust:check

Summary by CodeRabbit

• Improvements

  • Tool execution now retries once for specific post‑OAuth readiness errors, with a short configurable delay.

• Refactor

  • Facet routing was refactored to support routing into arbitrary buffers and stricter validation/clamping of facet fields.

• Tests

  • Added end‑to‑end tests for retry vs non‑retry OAuth scenarios.
  • Reworked event tests to use probe-based subscriptions for deterministic collection.
  • Restart test now uses unique per‑run identifiers and robust event matching.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a single-retry (10s) for Composio execute calls when a normalized post‑OAuth readiness error is returned; centralizes the POST, adds a matching predicate and constants, and introduces tests. Also replaces ad-hoc test event capture with probe-based, timeout-driven collection and updates a restart test to use unique IDs.

Changes

Post-OAuth Retry Mechanism

Layer / File(s)	Summary
Retry configuration and error matching src/openhuman/composio/client.rs	Duration import and private constants define a 10-second retry delay and exact-match readiness error strings. is_post_oauth_auth_readiness_error normalizes (trim, lowercase) ComposioExecuteResponse.error and matches against the configured strings.
Execute tool with retry src/openhuman/composio/client.rs	execute_tool now builds the request body and delegates to execute_tool_with_post_oauth_retry, which posts to /agent-integrations/composio/execute, inspects the response, waits 10s and retries the POST exactly once when the readiness predicate matches. post_execute_tool centralizes the POST.
Retry behavior tests src/openhuman/composio/client_tests.rs	Adds axum::extract::State and atomic counters to support stateful mock endpoints. Two tests verify: a single-retry succeeds when the first response is a readiness error (attempts == 2), and no retry occurs for other auth errors (attempts == 1, error preserved).

Test probe refactor (agent triage)

Layer / File(s)	Summary
Probe-based test imports src/openhuman/agent/triage/escalation.rs	Replaces Arc/Mutex/sleep imports with tokio::sync::broadcast and tokio::time::{timeout, Duration} to support probe channels and timeout-driven collection.
Subscribe/collect helpers & drop test src/openhuman/agent/triage/escalation.rs	Adds helpers to subscribe to event topics, forward matching events to an unbounded channel, and collect events by external_id until a predicate or timeout; used by the drop-only apply_decision test.
Refactor acknowledge/react/escalate tests src/openhuman/agent/triage/escalation.rs	Refactors apply_decision_acknowledge_only_publishes_evaluated, apply_decision_react_failure_publishes_failed_event, and apply_decision_escalate_failure_publishes_failed_event to use the probe receiver and deterministic collection instead of shared-memory + sleep.

Restart test update

Layer / File(s)	Summary
Restart test unique IDs src/openhuman/service/restart.rs	Adds a global RESTART_TEST_ID (AtomicUsize) and uses unique source/reason strings per test run.
Receive-loop assertion src/openhuman/service/restart.rs	Updates service_restart_publishes_restart_event to loop on probe rx.recv() until it observes the matching (source, reason) tuple or times out.

Summary-facets buffer routing

Layer / File(s)	Summary
Buffer-into routing helper src/openhuman/learning/extract/summary_facets.rs	route_facets_to_buffer delegates to route_facets_to_buffer_into, which validates facets, computes a single timestamp, maps class/cue/confidence, constructs LearningCandidate with EvidenceRef::DocumentChunk, and pushes into the given candidate::Buffer.
Local-buffer facet tests src/openhuman/learning/extract/summary_facets.rs	Tests updated to use local candidate::Buffer instances and assert drops for unknown class or empty evidence, default cue_family behavior, and that valid facets push expected candidates and evidence.

Sequence Diagram

sequenceDiagram
  participant Client as execute_tool
  participant Composio as /agent-integrations/composio/execute
  participant Timer as tokio::time::sleep
  Client->>Composio: POST execute (initial)
  Composio-->>Client: ComposioExecuteResponse (error or success)
  alt readiness error matched
    Client->>Timer: sleep POST_OAUTH_ACTION_RETRY_DELAY
    Timer-->>Client: wake
    Client->>Composio: POST execute (retry)
    Composio-->>Client: ComposioExecuteResponse (final)
  end

Loading

Estimated code review effort:
🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs:

• tinyhumansai/openhuman#1460: Related changes to route_facets_to_buffer and facet routing logic in summary facets.

Suggested reviewers:

• senamakel

"I'm a rabbit in code, quick on my paws,
I sniffed a retry for OAuth's small pause.
Ten seconds I wait, then hop once more,
The call leaps through doors it couldn't before.
🐇✨"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Changes include out-of-scope test harness modifications unrelated to #1688: deterministic event capture in escalation tests and restart test refactoring added beyond the retry implementation.	Remove or separately justify the test harness changes in escalation.rs and restart.rs that address event-bus stability but are unrelated to the Composio retry fix. Keep only client.rs and client_tests.rs changes aligned with #1688.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title 'fix(composio): retry post-oauth action readiness error' directly summarizes the main change: adding retry logic for Composio's post-OAuth readiness error.
Linked Issues check	✅ Passed	Changes implement the recommended solution (A) from issue #1688: retry detection of specific error string 'Connection error, try to authenticate' with configurable delay and single retry in action-execution path.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/client.rs`:
- Around line 171-191: The execute_tool_with_post_oauth_retry function only
emits a warn on the retry path; add debug/trace diagnostics to make both paths
grep-traceable: log entry/attempt start (with tool and retry_delay_ms) before
calling post_execute_tool, log the branch decision (whether
is_post_oauth_auth_readiness_error returned true/false) at debug/trace, and log
the retry dispatch and final outcome (success/error) after the second
post_execute_tool call; use tracing::debug or tracing::trace for these messages
and include the function name, tool, retry_delay.as_millis(), and the
result/error so callers can trace both retry and non-retry flows, referencing
execute_tool_with_post_oauth_retry, post_execute_tool, and
is_post_oauth_auth_readiness_error.

🪄 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: f1903aea-84aa-49ec-b524-206141d41b5f

📥 Commits

Reviewing files that changed from the base of the PR and between 2672706 and 5b901e6.

📒 Files selected for processing (2)

• src/openhuman/composio/client.rs
• src/openhuman/composio/client_tests.rs

[coderabbitai]

♻️ Duplicate comments (1)

src/openhuman/composio/client.rs (1)

171-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add debug-level diagnostics for both retry and non-retry paths.

The retry helper only logs at warn level for the retry branch. Per coding guidelines, new flows should include debug/trace logs at entry/exit, branch decisions, external calls, and retry dispatch/outcome.

As per coding guidelines, "In Rust, use log / tracing at debug or trace level 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/client.rs` around lines 171 - 191, In
execute_tool_with_post_oauth_retry add debug-level tracing at function entry
(including tool and retry_delay), before calling post_execute_tool, after
receiving the first response (including whether
is_post_oauth_auth_readiness_error returned true/false), and after the retry
call showing final outcome; also log the decision to sleep/retry with
retry_delay and the result of post_execute_tool for both attempts (use the
existing post_execute_tool and is_post_oauth_auth_readiness_error identifiers to
locate insertion points).

🧹 Nitpick comments (2)

src/openhuman/composio/client.rs (1)

193-197: 💤 Low value

Consider adding trace-level log for POST body visibility.

For deep debugging, a tracing::trace! log showing the request body (with PII redacted) before calling inner.post would make the retry flow fully grep-traceable under the [composio] prefix. Since IntegrationClient already logs HTTP calls, this is optional.

Optional trace log

 async fn post_execute_tool(&self, body: &serde_json::Value) -> Result<ComposioExecuteResponse> {
+    tracing::trace!(body = %body, "[composio] post_execute_tool");
     self.inner
         .post::<ComposioExecuteResponse>("/agent-integrations/composio/execute", body)
         .await
 }

🤖 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/client.rs` around lines 193 - 197, Add a trace-level
log in post_execute_tool before calling self.inner.post so the POST body is
visible in traces under the [composio] prefix; call tracing::trace! (or
tracing::instrument target) with a short message and the serialized body but
ensure any PII is redacted (e.g., redact fields or a redact_body helper) and
include context like the endpoint ("/agent-integrations/composio/execute") so
retries are fully traceable for the post_execute_tool ->
self.inner.post::<ComposioExecuteResponse> flow.

src/openhuman/agent/triage/escalation.rs (1)

252-262: 💤 Low value

Consider documenting the name parameter purpose.

The subscribe_probe function accepts a name parameter that's passed to global().unwrap().on(name, ...). Each test uses a unique string like "triage-escalation-drop", but it's unclear whether this is:

• The actual event topic that apply_decision publishes to
• A subscription identifier (with all DomainEvents going to all subscribers)
• Something else

Since the tests reportedly pass and external_id filtering provides isolation, this works correctly. However, a comment clarifying the parameter's purpose would improve maintainability.

🤖 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/agent/triage/escalation.rs` around lines 252 - 262, Document
that the name parameter of subscribe_probe denotes the event topic passed to
global().unwrap().on (i.e., the specific topic/key that publishers like
apply_decision will emit to), not merely a local subscription id; update the
subscribe_probe doc comment to state that subscribers only receive events
published to that topic, that tests use unique names like
"triage-escalation-drop" to avoid collisions, and mention that further isolation
is achieved by filtering on DomainEvent.external_id so readers understand why
tests pass.

🤖 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.

Duplicate comments:
In `@src/openhuman/composio/client.rs`:
- Around line 171-191: In execute_tool_with_post_oauth_retry add debug-level
tracing at function entry (including tool and retry_delay), before calling
post_execute_tool, after receiving the first response (including whether
is_post_oauth_auth_readiness_error returned true/false), and after the retry
call showing final outcome; also log the decision to sleep/retry with
retry_delay and the result of post_execute_tool for both attempts (use the
existing post_execute_tool and is_post_oauth_auth_readiness_error identifiers to
locate insertion points).

---

Nitpick comments:
In `@src/openhuman/agent/triage/escalation.rs`:
- Around line 252-262: Document that the name parameter of subscribe_probe
denotes the event topic passed to global().unwrap().on (i.e., the specific
topic/key that publishers like apply_decision will emit to), not merely a local
subscription id; update the subscribe_probe doc comment to state that
subscribers only receive events published to that topic, that tests use unique
names like "triage-escalation-drop" to avoid collisions, and mention that
further isolation is achieved by filtering on DomainEvent.external_id so readers
understand why tests pass.

In `@src/openhuman/composio/client.rs`:
- Around line 193-197: Add a trace-level log in post_execute_tool before calling
self.inner.post so the POST body is visible in traces under the [composio]
prefix; call tracing::trace! (or tracing::instrument target) with a short
message and the serialized body but ensure any PII is redacted (e.g., redact
fields or a redact_body helper) and include context like the endpoint
("/agent-integrations/composio/execute") so retries are fully traceable for the
post_execute_tool -> self.inner.post::<ComposioExecuteResponse> flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8a128f71-9ec5-47a6-be4f-98b185984163

📥 Commits

Reviewing files that changed from the base of the PR and between 5b901e6 and f9b8367.

📒 Files selected for processing (4)

• src/openhuman/agent/triage/escalation.rs
• src/openhuman/composio/client.rs
• src/openhuman/composio/client_tests.rs
• src/openhuman/service/restart.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openhuman/composio/client_tests.rs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/openhuman/learning/extract/summary_facets.rs (1)

92-103: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use first non-empty evidence chunk instead of only first()

Line 92 currently accepts only the first array entry, so a facet like ["", "chunk-2"] is dropped even though it has valid evidence. That contradicts the stated contract and can silently lose valid facets.

Proposed fix

-        let chunk_id = match facet.evidence_chunks.first() {
-            Some(id) if !id.is_empty() => id.clone(),
+        let chunk_id = match facet.evidence_chunks.iter().find(|id| !id.is_empty()) {
+            Some(id) => id.clone(),
             _ => {
                 tracing::debug!(
                     "[learning::extract::summary_facets] dropping facet key={} \
                      (no evidence_chunks) source_id={}",
                     facet.key,
                     source_id
                 );
                 continue;
             }
         };

🤖 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/learning/extract/summary_facets.rs` around lines 92 - 103, The
code currently takes facet.evidence_chunks.first() and drops the facet if that
first entry is empty; change this to scan facet.evidence_chunks for the first
non-empty string and use that as chunk_id (e.g., find(|id| !id.is_empty()) or
equivalent) so facets like ["", "chunk-2"] are preserved; update the chunk_id
match from using first() to using the iterator search and keep the same
debug/logging and continue behavior when no non-empty entries exist.

🤖 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.

Outside diff comments:
In `@src/openhuman/learning/extract/summary_facets.rs`:
- Around line 92-103: The code currently takes facet.evidence_chunks.first() and
drops the facet if that first entry is empty; change this to scan
facet.evidence_chunks for the first non-empty string and use that as chunk_id
(e.g., find(|id| !id.is_empty()) or equivalent) so facets like ["", "chunk-2"]
are preserved; update the chunk_id match from using first() to using the
iterator search and keep the same debug/logging and continue behavior when no
non-empty entries exist.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a5805fbb-8bcb-453a-8fd9-e347005f344a

📥 Commits

Reviewing files that changed from the base of the PR and between 6d9cd1d and d383b1f.

📒 Files selected for processing (2)

• src/openhuman/agent/triage/escalation.rs
• src/openhuman/learning/extract/summary_facets.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openhuman/agent/triage/escalation.rs