diff --git a/src/openhuman/channels/controllers/mod.rs b/src/openhuman/channels/controllers/mod.rs index deb1b3cde..809da0532 100644 --- a/src/openhuman/channels/controllers/mod.rs +++ b/src/openhuman/channels/controllers/mod.rs @@ -13,3 +13,8 @@ pub use schemas::{ all_controller_schemas as all_channels_controller_schemas, all_registered_controllers as all_channels_registered_controllers, }; + +/// Cross-module helpers from the channel controller layer that callers +/// outside the controller registry need (e.g. the welcome agent's +/// onboarding status snapshot). +pub use ops::connected_channel_slugs; diff --git a/src/openhuman/channels/controllers/ops.rs b/src/openhuman/channels/controllers/ops.rs index a5393d456..0e3188aba 100644 --- a/src/openhuman/channels/controllers/ops.rs +++ b/src/openhuman/channels/controllers/ops.rs @@ -420,12 +420,15 @@ pub async fn channel_status( config: &Config, channel_id: Option<&str>, ) -> Result>, String> { - // List all stored credentials with "channel:" prefix. - let stored = credentials::ops::list_provider_credentials(config, Some("channel:".to_string())) + // List all stored credentials with "channel:" prefix. Uses the + // prefix-match helper because channel credentials are keyed as + // `channel::` and no single literal value matches them + // through `list_provider_credentials`'s exact-match filter. + let stored = credentials::ops::list_provider_credentials_by_prefix(config, "channel:") .await .map_err(|e| format!("failed to list credentials: {e}"))?; - let stored_providers: Vec = stored.value.iter().map(|p| p.provider.clone()).collect(); + let stored_providers: Vec = stored.iter().map(|p| p.provider.clone()).collect(); let defs = match channel_id { Some(id) => { @@ -453,6 +456,92 @@ pub async fn channel_status( Ok(RpcOutcome::new(entries, vec![])) } +/// Return the slugs of all messaging channels currently connected, +/// merging the two storage layers OpenHuman uses for connection state. +/// +/// Two equally-authoritative sources exist today: +/// +/// * `config.channels_config.` — the legacy TOML field set by +/// credential-mode connects that need a runtime listener +/// (`bot_token` / `webhook` / `oauth`). These trigger +/// `restart_required = true` on the connect call. +/// * Provider credentials keyed `channel::` — set by the +/// newer managed-DM and OAuth flows that don't materialise a TOML +/// block but do persist a credential marker. +/// +/// Until both stores merge, any caller that only reads one will report +/// stale state to the user (e.g. the agent will say "Telegram not +/// connected" right after a managed-DM link succeeds — issue #1149). +/// This helper centralises the merge so every consumer agrees. +pub async fn connected_channel_slugs(config: &Config) -> Result, String> { + use std::collections::BTreeSet; + + let mut slugs: BTreeSet = BTreeSet::new(); + + // Layer 1: credential-mode channels written to TOML config. + let cc = &config.channels_config; + if cc.telegram.is_some() { + slugs.insert("telegram".to_string()); + } + if cc.discord.is_some() { + slugs.insert("discord".to_string()); + } + if cc.slack.is_some() { + slugs.insert("slack".to_string()); + } + if cc.mattermost.is_some() { + slugs.insert("mattermost".to_string()); + } + if cc.email.is_some() { + slugs.insert("email".to_string()); + } + if cc.whatsapp.is_some() { + slugs.insert("whatsapp".to_string()); + } + if cc.signal.is_some() { + slugs.insert("signal".to_string()); + } + if cc.matrix.is_some() { + slugs.insert("matrix".to_string()); + } + if cc.imessage.is_some() { + slugs.insert("imessage".to_string()); + } + if cc.irc.is_some() { + slugs.insert("irc".to_string()); + } + if cc.lark.is_some() { + slugs.insert("lark".to_string()); + } + if cc.dingtalk.is_some() { + slugs.insert("dingtalk".to_string()); + } + if cc.linq.is_some() { + slugs.insert("linq".to_string()); + } + if cc.qq.is_some() { + slugs.insert("qq".to_string()); + } + + // Layer 2: managed-DM / OAuth channels stored only as credentials + // under `channel::`. + let stored = credentials::ops::list_provider_credentials_by_prefix(config, "channel:") + .await + .map_err(|e| format!("failed to list channel credentials: {e}"))?; + for entry in &stored { + // provider format: "channel::" — extract slug. + if let Some(rest) = entry.provider.strip_prefix("channel:") { + if let Some((slug, _mode)) = rest.split_once(':') { + if !slug.is_empty() { + slugs.insert(slug.to_string()); + } + } + } + } + + Ok(slugs.into_iter().collect()) +} + /// Test a channel connection without persisting credentials. pub async fn test_channel( _config: &Config, diff --git a/src/openhuman/channels/controllers/ops_tests.rs b/src/openhuman/channels/controllers/ops_tests.rs index 1f7ed6fd7..669e04532 100644 --- a/src/openhuman/channels/controllers/ops_tests.rs +++ b/src/openhuman/channels/controllers/ops_tests.rs @@ -360,3 +360,123 @@ async fn disconnect_imessage_clears_runtime_config() { .and_then(|v| v.get("imessage")); assert!(im_entry.is_none(), "imessage config should be cleared"); } + +// --------------------------------------------------------------------------- +// Issue #1149: managed-DM / OAuth channels are stored only in the credential +// layer (`channel::`), not in `channels_config.`. Both +// `channel_status` and `connected_channel_slugs` must surface them so the +// chat agent stops reporting "Telegram not connected" right after a +// managed-DM link succeeds. +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn channel_status_reports_managed_dm_credential_as_connected() { + let (_tmp, config) = isolated_test_config(); + + // Simulate the post-link state: `telegram_login_check` stored a + // credential marker under `channel:telegram:managed_dm` with no + // corresponding `channels_config.telegram` block. + crate::openhuman::credentials::ops::store_provider_credentials( + &config, + "channel:telegram:managed_dm", + None, + Some("managed".to_string()), + Some(serde_json::json!({ "linked": true })), + Some(true), + ) + .await + .expect("seed managed-DM credential"); + + let result = channel_status(&config, Some("telegram")) + .await + .expect("channel_status should succeed"); + + let managed_dm = result + .value + .iter() + .find(|e| e.auth_mode == ChannelAuthMode::ManagedDm) + .expect("managed_dm entry"); + assert!( + managed_dm.connected, + "managed-DM credential should report connected: {:?}", + result.value + ); + assert!(managed_dm.has_credentials); +} + +#[tokio::test] +async fn connected_channel_slugs_merges_credentials_and_config() { + let (_tmp, mut config) = isolated_test_config(); + + // Layer 1: TOML-resident channel (e.g. discord bot_token). + config.channels_config.discord = Some(DiscordConfig { + bot_token: "tok".to_string(), + guild_id: None, + channel_id: None, + allowed_users: vec![], + listen_to_bots: false, + mention_only: false, + }); + + // Layer 2: credential-only channel (telegram managed_dm). + crate::openhuman::credentials::ops::store_provider_credentials( + &config, + "channel:telegram:managed_dm", + None, + Some("managed".to_string()), + Some(serde_json::json!({ "linked": true })), + Some(true), + ) + .await + .expect("seed managed-DM credential"); + + let slugs = connected_channel_slugs(&config) + .await + .expect("connected_channel_slugs should succeed"); + + assert!(slugs.contains(&"discord".to_string()), "got {slugs:?}"); + assert!(slugs.contains(&"telegram".to_string()), "got {slugs:?}"); +} + +#[tokio::test] +async fn connected_channel_slugs_dedupes_when_both_layers_present() { + let (_tmp, mut config) = isolated_test_config(); + + config.channels_config.discord = Some(DiscordConfig { + bot_token: "tok".to_string(), + guild_id: None, + channel_id: None, + allowed_users: vec![], + listen_to_bots: false, + mention_only: false, + }); + + // Same slug appears in both layers — should collapse to one entry. + crate::openhuman::credentials::ops::store_provider_credentials( + &config, + "channel:discord:managed_dm", + None, + Some("managed".to_string()), + Some(serde_json::json!({ "linked": true })), + Some(true), + ) + .await + .expect("seed managed-DM credential"); + + let slugs = connected_channel_slugs(&config) + .await + .expect("connected_channel_slugs should succeed"); + + let discord_count = slugs.iter().filter(|s| *s == "discord").count(); + assert_eq!(discord_count, 1, "discord should appear once: {slugs:?}"); +} + +#[tokio::test] +async fn connected_channel_slugs_empty_when_nothing_configured() { + let (_tmp, config) = isolated_test_config(); + let slugs = connected_channel_slugs(&config).await.unwrap(); + assert!( + slugs.is_empty(), + "fresh config should yield no channels: {slugs:?}" + ); +} diff --git a/src/openhuman/credentials/ops.rs b/src/openhuman/credentials/ops.rs index 46520295f..b396127bf 100644 --- a/src/openhuman/credentials/ops.rs +++ b/src/openhuman/credentials/ops.rs @@ -479,6 +479,35 @@ pub async fn list_provider_credentials( Ok(RpcOutcome::single_log(items, "provider credentials listed")) } +/// List credentials whose provider key starts with `prefix`. +/// +/// Pure prefix variant of [`list_provider_credentials`] for namespaces +/// that group multiple providers under a common stem (e.g. +/// `"channel:"` covers `channel:telegram:managed_dm`, +/// `channel:slack:bot_token`, …). The exact-match filter on +/// `list_provider_credentials` cannot express this without enumerating +/// every concrete provider key up front. +pub async fn list_provider_credentials_by_prefix( + config: &Config, + prefix: &str, +) -> Result, String> { + let auth = AuthService::from_config(config); + let profiles = auth.load_profiles().map_err(|e| e.to_string())?; + let mut items = profiles + .profiles + .values() + .filter(|profile| profile.provider != APP_SESSION_PROVIDER) + .filter(|profile| profile.provider.starts_with(prefix)) + .map(summarize_auth_profile) + .collect::>(); + items.sort_by(|a, b| { + a.provider + .cmp(&b.provider) + .then_with(|| a.profile_name.cmp(&b.profile_name)) + }); + Ok(items) +} + pub async fn oauth_connect( config: &Config, provider: &str, diff --git a/src/openhuman/credentials/ops_tests.rs b/src/openhuman/credentials/ops_tests.rs index 3df7df086..b9a20ba0f 100644 --- a/src/openhuman/credentials/ops_tests.rs +++ b/src/openhuman/credentials/ops_tests.rs @@ -390,3 +390,64 @@ async fn auth_get_me_errors_without_session() { let err = auth_get_me(&config).await.unwrap_err(); assert!(err.contains("session JWT required")); } + +// ── list_provider_credentials_by_prefix ─────────────────────── + +/// Issue #1149 root-cause regression: the exact-match filter on +/// `list_provider_credentials` cannot enumerate provider keys grouped +/// under a common stem (e.g. `channel:telegram:managed_dm`, +/// `channel:slack:bot_token`). The prefix variant fixes that — without +/// it, `channel_status` always returned `connected: false`. +#[tokio::test] +async fn list_provider_credentials_by_prefix_matches_namespaced_keys() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + for provider in [ + "channel:telegram:managed_dm", + "channel:slack:bot_token", + "skill:notion", + ] { + store_provider_credentials( + &config, + provider, + None, + Some("token-x".to_string()), + None, + Some(true), + ) + .await + .expect("seed credential"); + } + + let channels = list_provider_credentials_by_prefix(&config, "channel:") + .await + .expect("prefix list should succeed"); + let providers: Vec<&str> = channels.iter().map(|p| p.provider.as_str()).collect(); + + assert_eq!(channels.len(), 2, "got {providers:?}"); + assert!(providers.contains(&"channel:slack:bot_token")); + assert!(providers.contains(&"channel:telegram:managed_dm")); +} + +#[tokio::test] +async fn list_provider_credentials_by_prefix_returns_empty_when_no_match() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + store_provider_credentials( + &config, + "skill:notion", + None, + Some("token-x".to_string()), + None, + Some(true), + ) + .await + .expect("seed credential"); + + let result = list_provider_credentials_by_prefix(&config, "channel:") + .await + .expect("prefix list should succeed"); + assert!(result.is_empty(), "got {result:?}"); +} diff --git a/src/openhuman/tools/impl/agent/check_onboarding_status.rs b/src/openhuman/tools/impl/agent/check_onboarding_status.rs index fbb3c246e..6fd7091ad 100644 --- a/src/openhuman/tools/impl/agent/check_onboarding_status.rs +++ b/src/openhuman/tools/impl/agent/check_onboarding_status.rs @@ -125,6 +125,7 @@ async fn check_status() -> anyhow::Result { state.ready_to_complete, &state.ready_to_complete_reason, &state.composio_connected_toolkits, + &state.connected_channels, &webview_logins, ); diff --git a/src/openhuman/tools/impl/agent/onboarding_status.rs b/src/openhuman/tools/impl/agent/onboarding_status.rs index a0e180d65..ce0d5e88e 100644 --- a/src/openhuman/tools/impl/agent/onboarding_status.rs +++ b/src/openhuman/tools/impl/agent/onboarding_status.rs @@ -130,57 +130,12 @@ pub(crate) fn detect_auth(config: &Config) -> (bool, Value) { /// for that provider. See `openhuman::webview_accounts`. /// * `exchange_count` / `ready_to_complete` / `ready_to_complete_reason` /// — the gate the finalizer enforces. -/// Walk `config.channels_config` and return the connected messaging-channel -/// slugs in a stable order. Shared between `build_status_snapshot` and -/// `format_status_markdown` so the channel list can't drift between the -/// JSON and markdown views. -fn detect_channels(config: &Config) -> Vec<&'static str> { - let cc = &config.channels_config; - let mut out: Vec<&'static str> = Vec::new(); - if cc.telegram.is_some() { - out.push("telegram"); - } - if cc.discord.is_some() { - out.push("discord"); - } - if cc.slack.is_some() { - out.push("slack"); - } - if cc.mattermost.is_some() { - out.push("mattermost"); - } - if cc.email.is_some() { - out.push("email"); - } - if cc.whatsapp.is_some() { - out.push("whatsapp"); - } - if cc.signal.is_some() { - out.push("signal"); - } - if cc.matrix.is_some() { - out.push("matrix"); - } - if cc.imessage.is_some() { - out.push("imessage"); - } - if cc.irc.is_some() { - out.push("irc"); - } - if cc.lark.is_some() { - out.push("lark"); - } - if cc.dingtalk.is_some() { - out.push("dingtalk"); - } - if cc.linq.is_some() { - out.push("linq"); - } - if cc.qq.is_some() { - out.push("qq"); - } - out -} +// Channel detection now lives in +// `crate::openhuman::channels::controllers::ops::connected_channel_slugs` +// (precomputed in `compute_state` because it needs to read the +// credential store), so the welcome-agent surface honors managed-DM / +// OAuth connections that don't materialise a TOML +// `channels_config.` block (issue #1149). pub(crate) fn build_status_snapshot( config: &Config, @@ -189,10 +144,11 @@ pub(crate) fn build_status_snapshot( ready_to_complete: bool, ready_to_complete_reason: &str, composio_connected_toolkits: &[String], + connected_channels: &[String], webview_logins: Value, ) -> Value { let (is_authenticated, auth_source) = detect_auth(config); - let channels_connected = detect_channels(config); + let channels_connected: Vec<&str> = connected_channels.iter().map(|s| s.as_str()).collect(); let composio_enabled = config.composio.enabled; let delegate_agents: Vec<&str> = config.agents.keys().map(|s| s.as_str()).collect(); @@ -246,10 +202,11 @@ pub(crate) fn format_status_markdown( ready_to_complete: bool, ready_to_complete_reason: &str, composio_connected_toolkits: &[String], + connected_channels: &[String], webview_logins: &Value, ) -> String { let (is_authenticated, auth_source) = detect_auth(config); - let channels = detect_channels(config); + let channels: Vec<&str> = connected_channels.iter().map(|s| s.as_str()).collect(); let active_channel = config .channels_config @@ -321,6 +278,11 @@ pub(crate) struct OnboardingState { pub is_authenticated: bool, pub exchange_count: u32, pub composio_connected_toolkits: Vec, + /// Slugs of messaging channels currently connected, merged across + /// the legacy `channels_config.` TOML store and the + /// `channel::` credential store. Computed in + /// [`compute_state`] (issue #1149). + pub connected_channels: Vec, pub onboarding_status: &'static str, pub ready_to_complete: bool, pub ready_to_complete_reason: String, @@ -340,6 +302,22 @@ pub(crate) async fn compute_state( .collect(); let composio_connections = composio_connected_toolkits.len() as u32; + // Merge legacy `channels_config.` with the credential store + // so managed-DM / OAuth channels (e.g. Telegram managed_dm) report + // as connected to the welcome agent (issue #1149). Best-effort — + // a credential-store read failure logs and falls back to empty + // rather than masking the rest of the snapshot. + let connected_channels = + crate::openhuman::channels::controllers::connected_channel_slugs(config) + .await + .unwrap_or_else(|err| { + tracing::warn!( + error = %err, + "[onboarding] connected_channel_slugs failed; reporting empty channel list" + ); + Vec::new() + }); + let onboarding_status = if !is_authenticated { "unauthenticated" } else if config.chat_onboarding_completed { @@ -365,6 +343,7 @@ pub(crate) async fn compute_state( is_authenticated, exchange_count, composio_connected_toolkits, + connected_channels, onboarding_status, ready_to_complete, ready_to_complete_reason, @@ -385,6 +364,7 @@ mod tests { false, "no_apps_connected", &[], + &[], json!({"gmail": false}), ); assert_eq!(snap["onboarding_status"], "pending"); @@ -400,6 +380,8 @@ mod tests { 0 ); assert_eq!(snap["webview_logins"]["gmail"], false); + assert!(snap["channels_connected"].is_array()); + assert_eq!(snap["channels_connected"].as_array().unwrap().len(), 0); } #[test] @@ -412,6 +394,7 @@ mod tests { false, "no_apps_connected", &["gmail".to_string(), "github".to_string()], + &[], json!({"gmail": true, "whatsapp": false}), ); let toolkits = snap["composio_connected_toolkits"].as_array().unwrap(); @@ -421,6 +404,51 @@ mod tests { assert_eq!(snap["webview_logins"]["whatsapp"], false); } + /// Issue #1149: managed-DM / OAuth channels are stored in the + /// credential layer, not in `channels_config`. The snapshot must + /// reflect them so the welcome agent doesn't say "Telegram not + /// connected" right after a managed-DM link succeeds. + #[test] + fn build_status_snapshot_surfaces_credential_only_channels() { + let config = Config::default(); + // `channels_config.telegram` is None — the channel was linked + // via the managed-DM flow which only writes a credential entry. + // The merged slug list (built upstream by `compute_state`) is + // what `build_status_snapshot` consumes. + let snap = build_status_snapshot( + &config, + "pending", + 1, + false, + "no_apps_connected", + &[], + &["telegram".to_string()], + json!({}), + ); + let channels = snap["channels_connected"].as_array().unwrap(); + assert_eq!(channels.len(), 1); + assert_eq!(channels[0], "telegram"); + } + + #[test] + fn format_status_markdown_surfaces_credential_only_channels() { + let config = Config::default(); + let md = format_status_markdown( + &config, + "pending", + 1, + false, + "no_apps_connected", + &[], + &["telegram".to_string()], + &json!({}), + ); + assert!( + md.contains("**channels:** telegram"), + "markdown should surface credential-stored channel: {md}" + ); + } + #[test] fn detect_auth_on_default_config_is_unauthenticated() { let config = Config::default(); diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index c2bc73fa8..ca5cb6c8e 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -3273,3 +3273,111 @@ async fn external_process_with_guessed_token_is_rejected() { rpc_join.abort(); } + +/// End-to-end coverage for issue #1149: storing a managed-DM channel +/// credential under `channel::` and immediately observing +/// `connected:true` from `openhuman.channels_status`. +/// +/// Before the fix, `channels_status` always returned `connected:false` +/// because the underlying `list_provider_credentials` call used an +/// exact-match filter (`provider == "channel:"`) that never matched +/// the real credential keys (`channel:telegram:managed_dm`, +/// `channel:slack:bot_token`, …). The user could connect Telegram in +/// the UI but the chat / Settings page would still report it +/// disconnected on the next reload. +/// +/// This test exercises the full RPC wire path so a regression in +/// either the prefix helper or the channels controller is caught at +/// the transport layer, not just at the unit level. +#[tokio::test] +async fn channels_status_reflects_managed_dm_credential_e2e() { + let _env_lock = json_rpc_e2e_env_lock(); + let tmp = tempdir().expect("tempdir"); + let home = tmp.path(); + let openhuman_home = home.join(".openhuman"); + + let _home_guard = EnvVarGuard::set_to_path("HOME", home); + let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); + let _backend_url_guard = EnvVarGuard::unset("BACKEND_URL"); + let _vite_backend_guard = EnvVarGuard::unset("VITE_BACKEND_URL"); + + let (mock_addr, mock_join) = serve_on_ephemeral(mock_upstream_router()).await; + let mock_origin = format!("http://{}", mock_addr); + write_min_config(&openhuman_home, &mock_origin); + + let (rpc_addr, rpc_join) = serve_on_ephemeral(build_core_http_router(false)).await; + let rpc_base = format!("http://{}", rpc_addr); + tokio::time::sleep(Duration::from_millis(100)).await; + + // ── 1. baseline: telegram should report disconnected ──────────────────── + let baseline = post_json_rpc( + &rpc_base, + 7001, + "openhuman.channels_status", + json!({ "channel": "telegram" }), + ) + .await; + let baseline_outer = assert_no_jsonrpc_error(&baseline, "channels_status (baseline)"); + let baseline_result = baseline_outer.get("result").unwrap_or(baseline_outer); + let baseline_entries = baseline_result + .as_array() + .unwrap_or_else(|| panic!("expected array: {baseline_result}")); + let baseline_managed = baseline_entries + .iter() + .find(|e| e.get("auth_mode").and_then(Value::as_str) == Some("managed_dm")) + .expect("managed_dm entry should exist for telegram"); + assert_eq!( + baseline_managed.get("connected").and_then(Value::as_bool), + Some(false), + "fresh config should report telegram managed_dm disconnected: {baseline_managed}" + ); + + // ── 2. simulate a successful managed-DM link by storing the credential + // marker the way `telegram_login_check` does in production ───────── + let store = post_json_rpc( + &rpc_base, + 7002, + "openhuman.auth_store_provider_credentials", + json!({ + "provider": "channel:telegram:managed_dm", + "profile": "default", + "token": "managed", + "fields": { "linked": true }, + "setActive": true, + }), + ) + .await; + assert_no_jsonrpc_error(&store, "auth_store_provider_credentials"); + + // ── 3. channels_status must now report telegram managed_dm connected ─ + let after = post_json_rpc( + &rpc_base, + 7003, + "openhuman.channels_status", + json!({ "channel": "telegram" }), + ) + .await; + let after_outer = assert_no_jsonrpc_error(&after, "channels_status (after link)"); + let after_result = after_outer.get("result").unwrap_or(after_outer); + let after_entries = after_result + .as_array() + .unwrap_or_else(|| panic!("expected array: {after_result}")); + let after_managed = after_entries + .iter() + .find(|e| e.get("auth_mode").and_then(Value::as_str) == Some("managed_dm")) + .expect("managed_dm entry should exist for telegram"); + assert_eq!( + after_managed.get("connected").and_then(Value::as_bool), + Some(true), + "managed-DM credential should surface as connected: {after_managed}" + ); + assert_eq!( + after_managed + .get("has_credentials") + .and_then(Value::as_bool), + Some(true) + ); + + mock_join.abort(); + rpc_join.abort(); +}