diff --git a/src/openhuman/agent/agents/integrations_agent/prompt.rs b/src/openhuman/agent/agents/integrations_agent/prompt.rs index 2913d220f..deb38183a 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 874ee2718..63f3d5ef7 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 7c3366115..fa15d52eb 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 7be0d0d72..f58bf3379 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 424c59566..e50bbceb0 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 d920c2d48..c14ee025b 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 e2f758633..88d10d42c 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 bc365380d..3ef637cee 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 057eb7c9c..361fc8bee 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,77 @@ 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 { + // 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 \ + 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(_) => { + // 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 \ + and ask them to authorize '{toolkit}' in Settings → Connections → \ + '{toolkit}' before retrying the original request." + ), + } +} + #[cfg(test)] mod tests { use super::*; @@ -887,4 +966,112 @@ 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() { + // 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!( + padded.contains("`DeauthRequired`"), + "trimmed status must be quoted in original casing: {padded}" + ); + } + + #[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 1ed23f4ac..a7d456f70 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."), ];