From d6bf21f1ba44869c681deed4de4132ad3cf4b659 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 21 May 2026 02:12:35 +0530 Subject: [PATCH 1/4] fix(integrations): distinguish mid-OAuth / expired / failed states in spawn gate (#2365) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` 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 #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. --- .../agent/agents/integrations_agent/prompt.rs | 2 + .../agent/agents/orchestrator/prompt.rs | 5 + src/openhuman/agent/agents/welcome/prompt.rs | 2 + .../agent/harness/subagent_runner/ops.rs | 4 + .../agent/harness/test_support_test.rs | 1 + src/openhuman/agent/prompts/types.rs | 13 ++ src/openhuman/composio/ops.rs | 50 ++++++ src/openhuman/composio/ops_test.rs | 1 + .../tools/impl/agent/spawn_subagent.rs | 159 +++++++++++++++++- src/openhuman/tools/orchestrator_tools.rs | 3 + 10 files changed, 233 insertions(+), 7 deletions(-) diff --git a/src/openhuman/agent/agents/integrations_agent/prompt.rs b/src/openhuman/agent/agents/integrations_agent/prompt.rs index 2913d220f0..deb38183ab 100644 --- a/src/openhuman/agent/agents/integrations_agent/prompt.rs +++ b/src/openhuman/agent/agents/integrations_agent/prompt.rs @@ -246,6 +246,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }]; let body = build(&ctx_with(&integrations, &[])).unwrap(); assert!(body.contains("## Connected Integrations")); @@ -265,6 +266,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: false, + non_active_status: None, }]; let body = build(&ctx_with(&integrations, &[])).unwrap(); assert!(!body.contains("## Connected Integrations")); diff --git a/src/openhuman/agent/agents/orchestrator/prompt.rs b/src/openhuman/agent/agents/orchestrator/prompt.rs index 874ee27184..63f3d5ef73 100644 --- a/src/openhuman/agent/agents/orchestrator/prompt.rs +++ b/src/openhuman/agent/agents/orchestrator/prompt.rs @@ -224,6 +224,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }]; let body = build(&ctx_with(&integrations)).unwrap(); assert!(body.contains("## Connected Integrations")); @@ -245,6 +246,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }]; let body = build(&ctx_with(&integrations)).unwrap(); assert!(body.contains("## Connected Integrations")); @@ -267,6 +269,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }, ConnectedIntegration { toolkit: "linear".into(), @@ -274,6 +277,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: false, + non_active_status: None, }, ]; let body = build(&ctx_with(&integrations)).unwrap(); @@ -289,6 +293,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: false, + non_active_status: None, }]; let body = build(&ctx_with(&integrations)).unwrap(); assert!(!body.contains("## Connected Integrations")); diff --git a/src/openhuman/agent/agents/welcome/prompt.rs b/src/openhuman/agent/agents/welcome/prompt.rs index 7c33661151..fa15d52eb3 100644 --- a/src/openhuman/agent/agents/welcome/prompt.rs +++ b/src/openhuman/agent/agents/welcome/prompt.rs @@ -181,6 +181,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }, ConnectedIntegration { toolkit: "notion".into(), @@ -188,6 +189,7 @@ mod tests { tools: Vec::new(), gated_tools: Vec::new(), connected: false, + non_active_status: None, }, ]; let body = build(&ctx_with(&integrations)).unwrap(); diff --git a/src/openhuman/agent/harness/subagent_runner/ops.rs b/src/openhuman/agent/harness/subagent_runner/ops.rs index 7be0d0d72a..f58bf33797 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops.rs @@ -689,6 +689,10 @@ async fn run_typed_mode( // the user pref and doesn't change per-spawn. gated_tools: cached_integration.gated_tools.clone(), connected: cached_integration.connected, + // Inherit the cached non-active status — this spawn + // path only fires on connected toolkits, but keep the + // field consistent with the source row for #2365. + non_active_status: cached_integration.non_active_status.clone(), }; let integration = &integration; // Fuzzy-filter the toolkit's actions against the task prompt diff --git a/src/openhuman/agent/harness/test_support_test.rs b/src/openhuman/agent/harness/test_support_test.rs index 424c59566a..e50bbceb03 100644 --- a/src/openhuman/agent/harness/test_support_test.rs +++ b/src/openhuman/agent/harness/test_support_test.rs @@ -1564,6 +1564,7 @@ async fn orchestrator_prompt_drives_composio_call_via_delegation_chain() { tools: Vec::new(), gated_tools: Vec::new(), connected: true, + non_active_status: None, }]; let ctx = { use crate::openhuman::context::prompt::{LearnedContextData, ToolCallFormat}; diff --git a/src/openhuman/agent/prompts/types.rs b/src/openhuman/agent/prompts/types.rs index d920c2d48a..c14ee025bd 100644 --- a/src/openhuman/agent/prompts/types.rs +++ b/src/openhuman/agent/prompts/types.rs @@ -112,6 +112,19 @@ pub struct ConnectedIntegration { /// and the orchestrator must point the user at Settings instead of /// attempting to delegate. pub connected: bool, + /// Raw upstream connection status when a connection row exists but + /// is not `ACTIVE` — e.g. `"INITIATED"`, `"INITIALIZING"`, + /// `"FAILED"`, `"EXPIRED"`. `None` means either the user is + /// `ACTIVE` (use `connected = true`) OR there is no connection + /// row at all (truly disconnected). + /// + /// Used by the `integrations_agent` spawn-gate to surface the + /// real reason a delegation can't proceed — see issue #2365 + /// ("Agent says Gmail is disconnected when sending email"). The + /// gate previously emitted the same "not authorized yet" message + /// regardless of whether OAuth was mid-flight, the token had + /// expired, or the user had simply never started the flow. + pub non_active_status: Option, } /// A toolkit action that exists in the catalog but is currently hidden from diff --git a/src/openhuman/composio/ops.rs b/src/openhuman/composio/ops.rs index e2f758633b..88d10d42c5 100644 --- a/src/openhuman/composio/ops.rs +++ b/src/openhuman/composio/ops.rs @@ -1668,6 +1668,51 @@ async fn fetch_connected_integrations_uncached( .filter(|toolkit| !toolkit.is_empty()) .collect(); + // Most-informative *non-active* status per toolkit slug. Lets the + // integrations_agent spawn-gate (#2365) emit a precise message + // when a connection row exists but isn't usable yet (`INITIATED` + // — OAuth still in progress) or any longer (`EXPIRED` / `FAILED`) + // — instead of the legacy generic "available but not authorized". + // + // Status priority (UI-actionability): + // 1. EXPIRED — reconnect path + // 2. FAILED / ERROR — reconnect path + // 3. INITIATED / INITIALIZING / PENDING — finish OAuth in browser + // 4. anything else — passes through verbatim + let non_active_status_by_slug: std::collections::HashMap = { + fn priority(status: &str) -> u8 { + let s = status.trim().to_ascii_uppercase(); + match s.as_str() { + "EXPIRED" => 4, + "FAILED" | "ERROR" => 3, + "INITIATED" | "INITIALIZING" | "PENDING" => 2, + _ => 1, + } + } + let mut map: std::collections::HashMap = + std::collections::HashMap::new(); + for conn in connections.iter().filter(|c| !c.is_active()) { + let slug = conn.normalized_toolkit(); + if slug.is_empty() { + continue; + } + // Don't override an ACTIVE-slug — those carry no non-active + // status from this map's perspective. + if connected_slugs.contains(&slug) { + continue; + } + let p = priority(&conn.status); + map.entry(slug) + .and_modify(|cur| { + if p > cur.0 { + *cur = (p, conn.status.clone()); + } + }) + .or_insert_with(|| (p, conn.status.clone())); + } + map.into_iter().map(|(k, (_, v))| (k, v)).collect() + }; + // Deduplicate the allowlist so a backend that returns duplicates // doesn't produce dual entries downstream. let mut unique_toolkits: Vec = allowlisted_toolkits.clone(); @@ -1764,6 +1809,11 @@ async fn fetch_connected_integrations_uncached( tools, gated_tools, connected, + non_active_status: if connected { + None + } else { + non_active_status_by_slug.get(slug).cloned() + }, }); } diff --git a/src/openhuman/composio/ops_test.rs b/src/openhuman/composio/ops_test.rs index bc365380d1..3ef637cee6 100644 --- a/src/openhuman/composio/ops_test.rs +++ b/src/openhuman/composio/ops_test.rs @@ -872,6 +872,7 @@ fn integration(toolkit: &str, connected: bool) -> ConnectedIntegration { tools: Vec::new(), gated_tools: Vec::new(), connected, + non_active_status: None, } } diff --git a/src/openhuman/tools/impl/agent/spawn_subagent.rs b/src/openhuman/tools/impl/agent/spawn_subagent.rs index 057eb7c9c7..91b5ff1ffb 100644 --- a/src/openhuman/tools/impl/agent/spawn_subagent.rs +++ b/src/openhuman/tools/impl/agent/spawn_subagent.rs @@ -352,13 +352,21 @@ impl Tool for SpawnSubagentTool { // doesn't render this as a failed tool call. // The model still reads the explanation and produces // an appropriate user-facing response. - return Ok(ToolResult::success(format!( - "Integration '{tk}' is available but the user has not \ - authorized it yet. Do NOT retry this spawn. Tell the user \ - the integration is available and ask them to authorize \ - '{tk}' in Connections → Integrations before retrying the \ - original request." - ))); + // + // Split (#2365) into 4 cases driven by the upstream + // status field on the most-informative connection + // row, instead of the legacy generic + // "not authorized yet" copy. Before this split, + // an OAuth-in-progress / expired / failed Gmail + // surfaced the same "you need to connect Gmail" + // message — which Settings UI contradicted (it + // shows the connection as initiated/expired), so + // users concluded the agent was confused. + let message = describe_unconnected_state( + &ci.toolkit, + ci.non_active_status.as_deref(), + ); + return Ok(ToolResult::success(message)); } Some(_) => { tracing::debug!( @@ -631,6 +639,63 @@ fn render_worker_thread_result( ) } +/// Build the user-facing explanation for an allowlisted-but-not-active +/// integration during an `integrations_agent` spawn (#2365). +/// +/// The single message that previously covered every cause ("available +/// but the user has not authorized it yet") looked confused to users +/// who had Gmail showing in Settings (because Settings reflects the +/// FE's optimistic post-OAuth view, while the spawn gate reads the +/// backend's authoritative status). We now pivot on the upstream +/// connection status: +/// +/// - `INITIATED` / `INITIALIZING` / `PENDING` — OAuth in progress; +/// ask the user to finish the flow in their browser. +/// - `EXPIRED` — token rolled over; reconnect. +/// - `FAILED` / `ERROR` — handshake didn't land; reconnect. +/// - any other non-active status — quote the upstream verbatim. +/// - `None` — no connection row at all (truly disconnected). +/// +/// Returns text the model reads literally; the orchestrator paraphrases +/// it into a user-facing reply. Keep the *intent* stable across +/// rewordings — the "Settings → Connections → {toolkit}" path is +/// load-bearing for the UI navigation tests. +pub(crate) fn describe_unconnected_state(toolkit: &str, status: Option<&str>) -> String { + match status.map(|s| s.trim().to_ascii_uppercase()).as_deref() { + Some("INITIATED") | Some("INITIALIZING") | Some("PENDING") => format!( + "Integration '{toolkit}' has an OAuth flow in progress but it hasn't reached \ + ACTIVE yet. Do NOT retry this spawn. Tell the user the authorization is \ + pending and ask them to finish the browser OAuth flow (Settings → \ + Connections → '{toolkit}') before retrying. If they already closed the \ + browser tab, they can restart the connection from the same Settings page." + ), + Some("EXPIRED") => format!( + "Integration '{toolkit}' is connected but the OAuth token has expired. \ + Do NOT retry this spawn. Tell the user the connection expired and ask \ + them to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' \ + before retrying the original request." + ), + Some("FAILED") | Some("ERROR") => format!( + "Integration '{toolkit}' has a previous OAuth attempt in a FAILED state. \ + Do NOT retry this spawn. Tell the user the connection failed and ask them \ + to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' before \ + retrying the original request." + ), + Some(other) if !other.is_empty() => format!( + "Integration '{toolkit}' has a connection row but its status is `{other}`, \ + which is not yet usable. Do NOT retry this spawn. Tell the user the \ + connection is in an unusable state and ask them to reconnect '{toolkit}' \ + at Settings → Connections → '{toolkit}'." + ), + _ => format!( + "Integration '{toolkit}' is available but the user has not authorized it \ + yet. Do NOT retry this spawn. Tell the user the integration is available \ + and ask them to authorize '{toolkit}' in Settings → Connections → \ + '{toolkit}' before retrying the original request." + ), + } +} + #[cfg(test)] mod tests { use super::*; @@ -887,4 +952,84 @@ mod tests { ))); assert!(out.contains("Valid toolkits")); } + + // ── #2365: describe_unconnected_state per upstream status ─────── + + #[test] + fn describe_unconnected_state_initiated_says_oauth_in_progress() { + let msg = describe_unconnected_state("gmail", Some("INITIATED")); + assert!( + msg.contains("OAuth flow in progress"), + "INITIATED must surface the in-progress wording: {msg}" + ); + assert!(msg.contains("Settings → Connections → 'gmail'")); + // The legacy "not authorized yet" copy must NOT leak into the + // pending-OAuth branch — that was the user-perception bug + // from #2365 (Settings UI showed Gmail connected, agent said + // "not authorized"). + assert!( + !msg.contains("has not authorized it yet"), + "INITIATED must not borrow the truly-disconnected copy: {msg}" + ); + } + + #[test] + fn describe_unconnected_state_pending_and_initializing_are_aliased() { + for status in ["PENDING", "INITIALIZING"] { + let msg = describe_unconnected_state("gmail", Some(status)); + assert!( + msg.contains("OAuth flow in progress"), + "{status} must hit the in-progress branch: {msg}" + ); + } + } + + #[test] + fn describe_unconnected_state_expired_says_reconnect() { + let msg = describe_unconnected_state("gmail", Some("EXPIRED")); + assert!(msg.contains("OAuth token has expired")); + assert!(msg.contains("reconnect 'gmail'")); + assert!(!msg.contains("OAuth flow in progress")); + } + + #[test] + fn describe_unconnected_state_failed_and_error_route_to_reconnect() { + for status in ["FAILED", "ERROR"] { + let msg = describe_unconnected_state("gmail", Some(status)); + assert!( + msg.contains("FAILED state"), + "{status} must classify as failed: {msg}" + ); + assert!(msg.contains("reconnect 'gmail'")); + } + } + + #[test] + fn describe_unconnected_state_quotes_unknown_status_verbatim() { + let msg = describe_unconnected_state("gmail", Some("DEAUTH_REQUIRED")); + assert!( + msg.contains("`DEAUTH_REQUIRED`"), + "unknown statuses must be quoted so triage can act on them: {msg}" + ); + } + + #[test] + fn describe_unconnected_state_none_is_truly_disconnected() { + let msg = describe_unconnected_state("gmail", None); + assert!( + msg.contains("has not authorized it yet"), + "None must hit the legacy never-connected copy: {msg}" + ); + assert!(msg.contains("Settings → Connections → 'gmail'")); + } + + #[test] + fn describe_unconnected_state_status_match_is_case_insensitive() { + // The status string flows in from Composio's wire format; we + // can't assume casing. The classifier must normalise. + let initiated = describe_unconnected_state("gmail", Some("initiated")); + assert!(initiated.contains("OAuth flow in progress")); + let expired = describe_unconnected_state("gmail", Some("Expired")); + assert!(expired.contains("OAuth token has expired")); + } } diff --git a/src/openhuman/tools/orchestrator_tools.rs b/src/openhuman/tools/orchestrator_tools.rs index 1ed23f4ac0..a7d456f70b 100644 --- a/src/openhuman/tools/orchestrator_tools.rs +++ b/src/openhuman/tools/orchestrator_tools.rs @@ -322,6 +322,7 @@ mod tests { tools: vec![], gated_tools: vec![], connected: true, + non_active_status: None, } } @@ -465,6 +466,7 @@ mod tests { tools: vec![], gated_tools: vec![], connected: false, // not connected — must not appear in the enum + non_active_status: None, }, integration("notion", "Read and write pages."), ]; @@ -539,6 +541,7 @@ mod tests { tools: vec![], gated_tools: vec![], connected: true, + non_active_status: None, }, integration("gmail", "Email."), ]; From 6e749326f2a32ffacf5e3ad8a4af0ca7eaa9fd11 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 21 May 2026 03:58:56 +0530 Subject: [PATCH 2/4] =?UTF-8?q?fix(integrations):=20address=20CodeRabbit?= =?UTF-8?q?=20on=20#2373=20=E2=80=94=20preserve=20verbatim=20status=20in?= =?UTF-8?q?=20unknown-status=20branch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../tools/impl/agent/spawn_subagent.rs | 62 ++++++++++++++++--- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/src/openhuman/tools/impl/agent/spawn_subagent.rs b/src/openhuman/tools/impl/agent/spawn_subagent.rs index 91b5ff1ffb..361fc8beef 100644 --- a/src/openhuman/tools/impl/agent/spawn_subagent.rs +++ b/src/openhuman/tools/impl/agent/spawn_subagent.rs @@ -661,7 +661,15 @@ fn render_worker_thread_result( /// rewordings — the "Settings → Connections → {toolkit}" path is /// load-bearing for the UI navigation tests. pub(crate) fn describe_unconnected_state(toolkit: &str, status: Option<&str>) -> String { - match status.map(|s| s.trim().to_ascii_uppercase()).as_deref() { + // Keep the original (trimmed) status separately so the + // unknown-status branch can quote it verbatim — CodeRabbit + // review on #2373: matching on the uppercased value AND + // formatting with that uppercased value broke the + // "quote upstream status verbatim" contract for mixed/lowercase + // wire shapes. + let trimmed = status.map(str::trim).filter(|s| !s.is_empty()); + let upper = trimmed.map(|s| s.to_ascii_uppercase()); + match upper.as_deref() { Some("INITIATED") | Some("INITIALIZING") | Some("PENDING") => format!( "Integration '{toolkit}' has an OAuth flow in progress but it hasn't reached \ ACTIVE yet. Do NOT retry this spawn. Tell the user the authorization is \ @@ -681,12 +689,18 @@ pub(crate) fn describe_unconnected_state(toolkit: &str, status: Option<&str>) -> to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' before \ retrying the original request." ), - Some(other) if !other.is_empty() => format!( - "Integration '{toolkit}' has a connection row but its status is `{other}`, \ - which is not yet usable. Do NOT retry this spawn. Tell the user the \ - connection is in an unusable state and ask them to reconnect '{toolkit}' \ - at Settings → Connections → '{toolkit}'." - ), + Some(_) => { + // Quote the *original* upstream status, not its uppercased + // form — preserves "DeauthRequired" / "needs_relink"-style + // mixed-case wire values for triage. + let raw = trimmed.unwrap_or(""); + format!( + "Integration '{toolkit}' has a connection row but its status is `{raw}`, \ + which is not yet usable. Do NOT retry this spawn. Tell the user the \ + connection is in an unusable state and ask them to reconnect '{toolkit}' \ + at Settings → Connections → '{toolkit}'." + ) + } _ => format!( "Integration '{toolkit}' is available but the user has not authorized it \ yet. Do NOT retry this spawn. Tell the user the integration is available \ @@ -1006,10 +1020,38 @@ mod tests { #[test] fn describe_unconnected_state_quotes_unknown_status_verbatim() { - let msg = describe_unconnected_state("gmail", Some("DEAUTH_REQUIRED")); + // Pin three shapes (uppercase / mixed / snake_case) so the + // verbatim-quoting contract can't silently drift back to + // echoing the matched (uppercased) value — that was the + // CodeRabbit finding on #2373. + for raw in ["DEAUTH_REQUIRED", "needs_relink", "PartialAuthRequired"] { + let msg = describe_unconnected_state("gmail", Some(raw)); + let expected = format!("`{raw}`"); + assert!( + msg.contains(&expected), + "unknown status `{raw}` must be quoted verbatim (not its uppercased form): {msg}" + ); + } + } + + #[test] + fn describe_unconnected_state_quotes_unknown_status_after_trimming_whitespace() { + // Whitespace-only / blank statuses must NOT hit the + // unknown-status branch — they collapse to the + // truly-disconnected legacy copy via the `filter(|s| + // !s.is_empty())` guard in `describe_unconnected_state`. + let blank = describe_unconnected_state("gmail", Some(" ")); + assert!( + blank.contains("has not authorized it yet"), + "whitespace-only status must collapse to legacy None branch: {blank}" + ); + // A real status with surrounding whitespace is quoted with + // the whitespace trimmed (not preserved verbatim — triage + // would not want padded backticks). + let padded = describe_unconnected_state("gmail", Some(" DeauthRequired ")); assert!( - msg.contains("`DEAUTH_REQUIRED`"), - "unknown statuses must be quoted so triage can act on them: {msg}" + padded.contains("`DeauthRequired`"), + "trimmed status must be quoted in original casing: {padded}" ); } From 7e6732d468e630e65ccc3def8e4060c2a3541d91 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Sat, 23 May 2026 00:24:06 +0530 Subject: [PATCH 3/4] =?UTF-8?q?fix(integrations):=20address=20review=20on?= =?UTF-8?q?=20#2373=20=E2=80=94=20verbatim=20FAILED/ERROR=20+=20state-tran?= =?UTF-8?q?sition=20tracing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/openhuman/composio/ops.rs | 42 +++++++++++++++-- .../tools/impl/agent/spawn_subagent.rs | 47 +++++++++++++++---- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/openhuman/composio/ops.rs b/src/openhuman/composio/ops.rs index 88d10d42c5..168bac0419 100644 --- a/src/openhuman/composio/ops.rs +++ b/src/openhuman/composio/ops.rs @@ -1702,15 +1702,50 @@ async fn fetch_connected_integrations_uncached( continue; } let p = priority(&conn.status); - map.entry(slug) + map.entry(slug.clone()) .and_modify(|cur| { if p > cur.0 { + tracing::debug!( + target: "composio", + toolkit = %slug, + previous_status = %cur.1, + previous_priority = cur.0, + new_status = %conn.status, + new_priority = p, + "[composio] non_active_status_by_slug: upgraded most-informative status" + ); *cur = (p, conn.status.clone()); + } else { + tracing::trace!( + target: "composio", + toolkit = %slug, + kept_status = %cur.1, + kept_priority = cur.0, + candidate_status = %conn.status, + candidate_priority = p, + "[composio] non_active_status_by_slug: kept higher-priority status" + ); } }) - .or_insert_with(|| (p, conn.status.clone())); + .or_insert_with(|| { + tracing::debug!( + target: "composio", + toolkit = %slug, + status = %conn.status, + priority = p, + "[composio] non_active_status_by_slug: first non-active row" + ); + (p, conn.status.clone()) + }); } - map.into_iter().map(|(k, (_, v))| (k, v)).collect() + let final_map: std::collections::HashMap = + map.into_iter().map(|(k, (_, v))| (k, v)).collect(); + tracing::debug!( + target: "composio", + entries = final_map.len(), + "[composio] non_active_status_by_slug: final map" + ); + final_map }; // Deduplicate the allowlist so a backend that returns duplicates @@ -1829,6 +1864,7 @@ async fn fetch_connected_integrations_uncached( tracing::debug!( toolkit = %ci.toolkit, connected = ci.connected, + non_active_status = ?ci.non_active_status, tool_count = ci.tools.len(), "[composio] integration overview" ); diff --git a/src/openhuman/tools/impl/agent/spawn_subagent.rs b/src/openhuman/tools/impl/agent/spawn_subagent.rs index 361fc8beef..5370bd7d6c 100644 --- a/src/openhuman/tools/impl/agent/spawn_subagent.rs +++ b/src/openhuman/tools/impl/agent/spawn_subagent.rs @@ -362,6 +362,12 @@ impl Tool for SpawnSubagentTool { // message — which Settings UI contradicted (it // shows the connection as initiated/expired), so // users concluded the agent was confused. + tracing::debug!( + target: "spawn_subagent", + toolkit = %ci.toolkit, + non_active_status = ?ci.non_active_status, + "[spawn_subagent] integrations_agent gate: toolkit not connected — emitting status-specific message" + ); let message = describe_unconnected_state( &ci.toolkit, ci.non_active_status.as_deref(), @@ -683,12 +689,19 @@ pub(crate) fn describe_unconnected_state(toolkit: &str, status: Option<&str>) -> them to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' \ before retrying the original request." ), - Some("FAILED") | Some("ERROR") => format!( - "Integration '{toolkit}' has a previous OAuth attempt in a FAILED state. \ - Do NOT retry this spawn. Tell the user the connection failed and ask them \ - to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' before \ - retrying the original request." - ), + Some("FAILED") | Some("ERROR") => { + // Quote the actual upstream label (FAILED / ERROR) instead of + // hard-coding "FAILED" — triage cross-references backend logs + // and a misquoted `ERROR` row showing up as "FAILED" wastes + // their time. graycyrus review on #2373. + let raw = trimmed.unwrap_or(""); + format!( + "Integration '{toolkit}' has a previous OAuth attempt in a `{raw}` state. \ + Do NOT retry this spawn. Tell the user the connection failed and ask them \ + to reconnect '{toolkit}' at Settings → Connections → '{toolkit}' before \ + retrying the original request." + ) + } Some(_) => { // Quote the *original* upstream status, not its uppercased // form — preserves "DeauthRequired" / "needs_relink"-style @@ -1010,14 +1023,32 @@ mod tests { fn describe_unconnected_state_failed_and_error_route_to_reconnect() { for status in ["FAILED", "ERROR"] { let msg = describe_unconnected_state("gmail", Some(status)); + let expected = format!("`{status}` state"); assert!( - msg.contains("FAILED state"), - "{status} must classify as failed: {msg}" + msg.contains(&expected), + "{status} must be quoted verbatim, not collapsed to a single label: {msg}" ); assert!(msg.contains("reconnect 'gmail'")); } } + #[test] + fn describe_unconnected_state_failed_and_error_preserve_original_casing() { + // Mixed-case wire values must round-trip through the FAILED / + // ERROR branch with their original casing intact — that's the + // whole point of graycyrus' review feedback. + let lower_failed = describe_unconnected_state("gmail", Some("failed")); + assert!( + lower_failed.contains("`failed` state"), + "lowercase `failed` must be quoted verbatim: {lower_failed}" + ); + let mixed_error = describe_unconnected_state("gmail", Some("Error")); + assert!( + mixed_error.contains("`Error` state"), + "mixed-case `Error` must be quoted verbatim: {mixed_error}" + ); + } + #[test] fn describe_unconnected_state_quotes_unknown_status_verbatim() { // Pin three shapes (uppercase / mixed / snake_case) so the From 36128cd3daac03f7cee98704dd23737ca3b1861e Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Sat, 23 May 2026 01:10:53 +0530 Subject: [PATCH 4/4] fix(tests): add non_active_status to ConnectedIntegration in merged session test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After merging upstream main (b2ae12ad — 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` 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 --- src/openhuman/agent/harness/session/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index fc33de6a87..7ddf47a889 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/src/openhuman/agent/harness/session/tests.rs @@ -257,6 +257,7 @@ fn set_connected_integrations_marks_session_initialized_and_updates_hash() { tools: vec![], gated_tools: vec![], connected: true, + non_active_status: None, }, ]);