fix(integrations): distinguish mid-OAuth / expired / failed states in spawn gate (#2365)#2373
Conversation
… spawn gate (tinyhumansai#2365) The `integrations_agent` spawn-gate used to emit the same "available but the user has not authorized it yet" copy regardless of WHY a Composio connection wasn't usable — whether the OAuth was still in flight (`INITIATED`), the token had rolled over (`EXPIRED`), or the handshake had failed outright (`FAILED`). Users who saw Gmail showing as connected in Settings would then see the agent say "Gmail isn't connected", concluded the app was broken, and opened the issue. Settings UI reflects the FE's optimistic post-OAuth view; the spawn-gate reads the backend's authoritative `list_connections` status. When those diverge — most commonly because OAuth never reached `ACTIVE` — the user-facing message has to be precise enough that the user can act on the actual situation, not retry the same flow. Wire path - `ConnectedIntegration` gains a `non_active_status: Option<String>` field carrying the most-informative upstream status for toolkits in the backend allowlist that lack an ACTIVE connection row. `EXPIRED > FAILED/ERROR > INITIATED/INITIALIZING/PENDING > other`. - `fetch_connected_integrations_uncached` builds a per-slug map from the raw `connections` vec (filtered to non-active rows that don't have a competing ACTIVE row for the same slug) and feeds the prioritised status into every emitted `ConnectedIntegration`. - `spawn_subagent`'s integrations_agent gate routes through a new `describe_unconnected_state(toolkit, status)` helper instead of the inline literal. Five distinct messages: INITIATED/INITIALIZING/PENDING → "finish the browser OAuth flow" EXPIRED → "reconnect — token expired" FAILED/ERROR → "reconnect — previous attempt failed" other non-empty status → quoted verbatim for triage None (no row at all) → legacy "never authorized" copy - All 14 `ConnectedIntegration { ... }` literal sites updated to carry the new field. Test fixtures all use `None`. Tests (in `spawn_subagent::tests`, 7 new, 13 passing in the targeted filter; 57 passing in `composio::ops`) - INITIATED / PENDING / INITIALIZING all route to the OAuth-in-progress branch and explicitly do NOT borrow the legacy "has not authorized it yet" wording — that was the actual user-perception bug from tinyhumansai#2365 (Settings showed Gmail connected, agent said "not authorized"). - EXPIRED → reconnect copy with `OAuth token has expired`. - FAILED / ERROR → reconnect copy with `FAILED state`. - Unknown status (e.g. `DEAUTH_REQUIRED`) is quoted verbatim so triage can act on it without needing a code change. - None → preserves the legacy never-connected copy. - Case-insensitive matching (`initiated`, `Expired`) routes the same way as the canonical uppercase form, since Composio's wire shape isn't case-stable. Acceptance criteria touched - [x] No false disconnected response: connections in the mid-OAuth / expired / failed states are now described with their actual state. - [x] Scope issue surfaced: scope-mismatch errors continue to flow through the existing `composio::error_mapping` `InsufficientScope` path; this PR doesn't regress that. - [x] Connection state consistent: the gate now reads the same backend status the FE Settings UI reads. - [x] Regression safety: 7 new tests + 6 pre-existing integrations_agent gate tests still pass. Out of scope (separate PRs / not needed) - No FE change: the spawn-gate output bubbles up to the orchestrator, which paraphrases for the user. - No `is_active` change: pre-existing semantics (only ACTIVE / CONNECTED count as usable) are preserved; the new field only describes the *non-active* case. - No new RPC: the new field rides along the existing `ConnectedIntegration` payload used by the agent harness.
|
Caution Review failedFailed to post review comments 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:
📝 WalkthroughWalkthroughAdds ConnectedIntegration.non_active_status, computes and attaches it in composio integration overviews, preserves it during spawn refreshes, and uses it to produce status-specific user messages; test fixtures and unit tests updated accordingly. ChangesIntegration Status Tracking and User Messaging
Sequence DiagramsequenceDiagram
participant Composio
participant IntegrationOverviewBuilder
participant CachedIntegrationStore
participant SpawnSubagent
participant UserClient
Composio->>IntegrationOverviewBuilder: provide backend connections list
IntegrationOverviewBuilder->>IntegrationOverviewBuilder: compute non_active_status_by_slug (priority)
IntegrationOverviewBuilder->>CachedIntegrationStore: build ConnectedIntegration entries with non_active_status
UserClient->>SpawnSubagent: request integrations_agent spawn for toolkit
SpawnSubagent->>CachedIntegrationStore: read cached ConnectedIntegration
SpawnSubagent->>SpawnSubagent: copy non_active_status into refreshed record
SpawnSubagent->>SpawnSubagent: call describe_unconnected_state(toolkit, non_active_status)
SpawnSubagent->>UserClient: return ToolResult message explaining unconnected state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/openhuman/tools/impl/agent/spawn_subagent.rs (1)
355-369: ⚡ Quick winAdd a debug log for non-active status classification before early return.
This branch is a state-transition gate with user-visible behavior changes, but it currently returns without emitting status context. A single structured
tracing::debug!here would make regressions much easier to triage.As per coding guidelines: "Use
log/tracingatdebugortracelevel on ... state transitions, and any branch that is hard to infer from tests alone."🤖 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/tools/impl/agent/spawn_subagent.rs` around lines 355 - 369, Before the early return that builds and returns the user-visible message, add a tracing::debug! call to record the non-active status classification and relevant context (e.g., include ci.non_active_status.as_deref(), ci.toolkit identifier/state if available) so state transitions are observable; place this debug log immediately before the call to describe_unconnected_state(...) / the return of ToolResult::success(message) in the same block where describe_unconnected_state and ci.non_active_status are referenced.src/openhuman/composio/ops.rs (1)
1671-1714: ⚡ Quick winAdd trace/debug context for chosen non-active status per toolkit.
This new priority-selection path is a state-interpretation branch, but logs currently don’t expose the selected
non_active_status. Including it in the integration overview/debug path would make spawn-gate regressions much faster to triage.Proposed patch
@@ for ci in &integrations { tracing::debug!( toolkit = %ci.toolkit, connected = ci.connected, + non_active_status = ?ci.non_active_status, tool_count = ci.tools.len(), "[composio] integration overview" ); }As per coding guidelines: "Use
log/tracingatdebugortracelevel on ... state transitions, and any branch that is hard to infer from tests alone."Also applies to: 1812-1816
🤖 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/ops.rs` around lines 1671 - 1714, The non_active_status_by_slug construction doesn't emit any debug/trace info about which non-active status was chosen per toolkit slug; add a debug-level log (using tracing::debug! or log::debug!) at the points where you insert or update the map entry (inside the map.entry(...).and_modify(...) / .or_insert_with(...) flow) to record the slug, the candidate status (conn.status) and the chosen status after comparison, and also emit a final trace/debug of the resulting non_active_status_by_slug map after collection so the integration overview can show the selected non-active status per toolkit; reference variables/functions: non_active_status_by_slug, connections, connected_slugs, conn.normalized_toolkit(), conn.status, and the local priority(...) helper.
🤖 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/tools/impl/agent/spawn_subagent.rs`:
- Around line 663-689: The function describe_unconnected_state uppercases the
incoming status for matching then echoes that uppercased value back in the
Some(other) branch, breaking the requirement to preserve the original verbatim
status; fix by first capturing the trimmed original status (e.g., let orig =
status.map(|s| s.trim().to_string())), compute an uppercase version for matching
(e.g., let up = orig.as_deref().map(|s| s.to_ascii_uppercase())), then perform
the match on up.as_deref() but when formatting the unknown-status message use
the original trimmed string (orig.as_deref().unwrap_or("<empty>")) instead of
the uppercased value so describe_unconnected_state returns the upstream status
verbatim in the Some(other) arm.
---
Nitpick comments:
In `@src/openhuman/composio/ops.rs`:
- Around line 1671-1714: The non_active_status_by_slug construction doesn't emit
any debug/trace info about which non-active status was chosen per toolkit slug;
add a debug-level log (using tracing::debug! or log::debug!) at the points where
you insert or update the map entry (inside the map.entry(...).and_modify(...) /
.or_insert_with(...) flow) to record the slug, the candidate status
(conn.status) and the chosen status after comparison, and also emit a final
trace/debug of the resulting non_active_status_by_slug map after collection so
the integration overview can show the selected non-active status per toolkit;
reference variables/functions: non_active_status_by_slug, connections,
connected_slugs, conn.normalized_toolkit(), conn.status, and the local
priority(...) helper.
In `@src/openhuman/tools/impl/agent/spawn_subagent.rs`:
- Around line 355-369: Before the early return that builds and returns the
user-visible message, add a tracing::debug! call to record the non-active status
classification and relevant context (e.g., include
ci.non_active_status.as_deref(), ci.toolkit identifier/state if available) so
state transitions are observable; place this debug log immediately before the
call to describe_unconnected_state(...) / the return of
ToolResult::success(message) in the same block where describe_unconnected_state
and ci.non_active_status are referenced.
🪄 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: a907bcd8-ec55-4b3f-9bb0-1c8c4e627498
📒 Files selected for processing (10)
src/openhuman/agent/agents/integrations_agent/prompt.rssrc/openhuman/agent/agents/orchestrator/prompt.rssrc/openhuman/agent/agents/welcome/prompt.rssrc/openhuman/agent/harness/subagent_runner/ops.rssrc/openhuman/agent/harness/test_support_test.rssrc/openhuman/agent/prompts/types.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/ops_test.rssrc/openhuman/tools/impl/agent/spawn_subagent.rssrc/openhuman/tools/orchestrator_tools.rs
… verbatim status in unknown-status branch
CodeRabbit major finding on src/openhuman/tools/impl/agent/spawn_subagent.rs.
The previous `describe_unconnected_state` uppercased the status
once and then used the uppercased value in BOTH the match arms AND
the unknown-status format string, so a mixed-case wire value like
`DeauthRequired` was echoed back as `DEAUTH_REQUIRED` — breaking
the "quote unknown statuses verbatim" contract.
Fix
- Capture the trimmed original separately:
let trimmed = status.map(str::trim).filter(|s| !s.is_empty());
let upper = trimmed.map(|s| s.to_ascii_uppercase());
- Match on `upper.as_deref()` for the known branches.
- In the unknown-status branch, format with `trimmed.unwrap_or("")`
so the upstream casing is preserved verbatim.
- `filter(|s| !s.is_empty())` collapses whitespace-only inputs to
the truly-disconnected `_` branch (instead of formatting "" into
the unknown-status template).
Tests
- Expanded `describe_unconnected_state_quotes_unknown_status_verbatim`
to pin three shapes (uppercase / snake_case / PascalCase) so a
silent drift back to echoing the uppercased value fails CI.
- New `describe_unconnected_state_quotes_unknown_status_after_trimming_whitespace`
pins:
* blank/whitespace input collapses to the legacy `None` branch
* padded status is trimmed but its casing is preserved
- All other tests unchanged and still pass.
`cargo test --lib describe_unconnected_state` → 8 passed, 0 failed
(7 pre-existing + 1 new).
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — graycyrus
Solid PR. The spawn-gate now correctly distinguishes mid-OAuth / expired / failed / truly-disconnected states instead of conflating them into a single misleading message. The priority map in fetch_connected_integrations_uncached is well-designed (EXPIRED > FAILED/ERROR > INITIATED/PENDING > other), and the CodeRabbit verbatim-quoting fix in 6e74932 looks correct.
CI: all green. Coverage gate: passing. Tests: 7 new, 13 total — good coverage of all branches including case-insensitivity and whitespace trimming.
Issue #2365 alignment
The core repro (Settings shows Gmail connected → agent says "not authorized") is resolved for the cases this PR targets (INITIATED, EXPIRED, FAILED, ERROR). One acceptance criterion — "If Gmail is connected but lacks send scopes, the UI/agent clearly says scopes are missing" — isn't addressed here, but that would be a different code path (action-execution, not spawn-gate). Fine to track separately.
Change summary
| File | What changed |
|---|---|
prompts/types.rs |
New non_active_status: Option<String> on ConnectedIntegration |
composio/ops.rs |
Builds non_active_status_by_slug map with priority logic |
spawn_subagent.rs |
New describe_unconnected_state() replaces legacy single message |
| 7 other files | Test fixtures + field propagation |
1 minor finding below — no blockers.
| to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' before \ | ||
| retrying the original request." | ||
| ), | ||
| Some(_) => { |
There was a problem hiding this comment.
[minor] When status is ERROR, this message says "a previous OAuth attempt in a FAILED state" — which misquotes the upstream status. For the known FAILED/ERROR pair this is cosmetic, but it could confuse triage if they're cross-referencing backend logs that show ERROR.
Consider interpolating the actual matched category or using a neutral phrasing like "in a failed/error state":
Some("FAILED") | Some("ERROR") => format!(
"Integration '{toolkit}' has a previous OAuth attempt that did not succeed. \
Do NOT retry this spawn. ..."
)Non-blocking — the user-facing model paraphrases this anyway.
Review note — overlap with merged #2414Issue #2365 now has two PRs linked. #2414 (by @aqilaziz) was already merged and closed the issue — it fixed the specific case where Gmail is connected but lacks send scopes, surfacing "missing permissions" instead of the generic "not connected" message. This PR (#2373) is still valuable — it covers a different slice of the same problem that #2414 doesn't handle:
Without this PR, users whose Gmail shows as INITIATED/EXPIRED/FAILED in Settings will still get the confusing "not authorized yet" message from the agent. Since #2414 already merged and touched some of the same files (spawn_subagent, prompts, composio ops), this branch likely needs a rebase. Pushing a conflict resolution now. |
|
@CodeGhost21 — heads up, 3 CI checks are failing after the main merge, but none are caused by your changes:
All Rust checks pass (fmt, clippy, core tests, core coverage, Tauri shell tests, Tauri coverage) — your actual changes are green. That said, can you take a look and confirm these are indeed pre-existing on main? A quick check would be to see if the same Frontend Unit Tests / i18n Coverage jobs are also failing on other recent PRs or on main itself. If they are, this PR is clear to review as-is. |
…LED/ERROR + state-transition tracing
graycyrus review: the FAILED/ERROR branch hard-coded "in a FAILED state",
which misquoted upstream `ERROR` rows and wasted triage time when cross-
referencing backend logs. Now quotes the actual trimmed upstream label
(`failed`, `Error`, etc.) in backticks, matching the unknown-status
branch's verbatim-quoting contract. Test updated; new test pins
mixed-case round-trip.
CodeRabbit nitpicks: two state-transition gates were silent. Added
tracing::debug! calls so regressions are easy to trace end-to-end
(matches CLAUDE.md "Debug logging" rule):
- `spawn_subagent.rs` integrations_agent gate: log toolkit + non_active_status
immediately before emitting the status-specific message.
- `composio/ops.rs` `non_active_status_by_slug` build: log each
insert / upgrade / kept candidate, and the final map size. Also
include `non_active_status` in the existing per-toolkit integration
overview debug line.
Tests: cargo test --lib describe_unconnected_state → 9 passed (8 prior + 1 new).
cargo test --lib composio::ops → 57 passed.
cargo test --lib integrations_agent → 7 passed.
…ession test After merging upstream main (b2ae12a — perf(agent): prewarm session integrations) into this branch, the new test `set_connected_integrations_marks_session_initialized_and_updates_hash` constructs a `ConnectedIntegration` literal without the `non_active_status: Option<String>` field that this PR added to the struct. Auto-merge with main therefore failed CI compile with E0063 at session/tests.rs:254 even though both sides compiled in isolation — a classic silent semantic conflict. Add `non_active_status: None` to the merged-in literal so the merged tree compiles. No behavior change: the test asserts initialization / hash bookkeeping and does not touch the new field. Tests: - cargo test --lib describe_unconnected_state -> 9 passed (PR coverage) - cargo test --lib set_connected_integrations_marks_session -> 1 passed (the merged-in regression test) - cargo check --tests -p openhuman -> clean
3 participants
Summary
INITIATED/INITIALIZING/PENDING), expired (EXPIRED), failed (FAILED/ERROR), and truly-never-connected states in theintegrations_agentspawn-gate so the agent's reply to "send me an email" matches the actual Composio connection status.ConnectedIntegration.non_active_status, populated from rawlist_connectionsrows infetch_connected_integrations_uncached.describe_unconnected_state(toolkit, status)helper emits one of 5 distinct messages instead of the legacy single "available but not authorized yet" copy that conflated all causes.Problem
Repro: Settings → Connections shows Gmail as connected. User asks the agent to send an email. Orchestrator delegates to
integrations_agent(toolkit="gmail", …).spawn_subagent's gate refreshes fromfetch_connected_integrations_statusand finds gmail in the allowlist withconnected: false(becauseis_active()accepts onlyACTIVE/CONNECTED). Gate returns the legacy generic "not authorized yet" copy. Agent paraphrases as "Gmail isn't connected" — which contradicts what Settings shows.The actual cause could be any of: OAuth still pending (
INITIATED), token expired (EXPIRED), handshake failed (FAILED) — each with a different remediation. The legacy message conflated all of them.Solution
ConnectedIntegrationcarries the non-active statusnon_active_status: Option<String>onprompts::types::ConnectedIntegration.fetch_connected_integrations_uncached(src/openhuman/composio/ops.rs) builds a per-slug map from the rawconnectionsvec, filtered to non-active rows that don't have a competing ACTIVE row for the same slug. Status prioritisedEXPIRED > FAILED/ERROR > INITIATED/INITIALIZING/PENDING > otherso the most-actionable label wins.ConnectedIntegration { … }literal sites updated to carry the new field (test fixtures useNone).Gate emits a specific message
spawn_subagent's integrations_agent branch routes throughdescribe_unconnected_state(toolkit, status):INITIATED/INITIALIZING/PENDINGEXPIREDSettings → Connections → '<toolkit>'FAILED/ERRORNone(no row at all)Submission Checklist
cargo test --lib describe_unconnected_state non_active_status integrations_agent→ 13 passed.describe_unconnected_statehas at least one focused test; the newnon_active_status_by_slugbuild path is exercised through the existing composio::ops integration tests.## Related— N/A: no matrix row touched.Closes #NNNin the## Relatedsection — see below.Impact
integrations_agentspawn-gate.non_active_statusis computed once perfetch_connected_integrations_uncachedcycle (cached). No new IO; no new error surfaces.ConnectedIntegrationgained one optional field; all in-repo constructors updated.Tests
cargo test --lib describe_unconnected_state non_active_status integrations_agent→ 13 passed, 0 failed (7 new).cargo test --lib composio::ops→ 57 passed (sanity-check the new build path).describe_unconnected_state_initiated_says_oauth_in_progressdescribe_unconnected_state_pending_and_initializing_are_aliaseddescribe_unconnected_state_expired_says_reconnectdescribe_unconnected_state_failed_and_error_route_to_reconnectdescribe_unconnected_state_quotes_unknown_status_verbatimDEAUTH_REQUIRED) is quoted so triage can read it from logsdescribe_unconnected_state_none_is_truly_disconnectedNonepreserves the legacy never-connected copydescribe_unconnected_state_status_match_is_case_insensitiveinitiated/Expiredroute the same way as the canonical uppercase formsCI flakes
The "Rust Tauri Coverage" job is currently red on
core_process::tests::ensure_running_*— those tests live inapp/src-tauri/src/core_process_tests.rs, which this PR does NOT modify. The failure pattern ("core process did not become ready" cascading into "env lock poisoned") is a known port-binding flake in CI. A no-op re-run of just that job should pass; the PR's own changed-line tests are all green.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2365-integration-gate-states(branched fromorigin/mainafter fresh fetch)Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend changes.pnpm typecheck— N/A: no TypeScript changes.cargo test --lib describe_unconnected_state non_active_status integrations_agent→ 13 passed, 0 failed.cargo fmt --manifest-path Cargo.tomlapplied;cargo checkclean.app/src-tauri/changes. The Tauri Coverage CI red is on an unrelatedcore_processport-binding flake.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
None(no row) keeps the legacy "never authorized" copy. TheSettings → Connections → '<toolkit>'remediation literal is preserved verbatim so existing UI-navigation tests pass.initiated,Expired, etc.) locked in by_status_match_is_case_insensitive.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests