From f67c4e875442de5314c403d93095e88cdee80f58 Mon Sep 17 00:00:00 2001 From: Ben Hoverter <32376575+benhoverter@users.noreply.github.com> Date: Wed, 29 Apr 2026 04:40:36 -0700 Subject: [PATCH] fix(channels): key router default-agent map on user_id, not channel_id (#1123) * fix(channels): key router on user, not channel (Discord/Slack) Discord and Slack adapters set sender.platform_id to the channel/conversation ID (needed for the send path), so router.resolve(channel, sender.platform_id, ..) was matching peer_id bindings against the channel ID and never finding the user-keyed binding. The sender_user_id() helper already existed but only the rate-limit/authz paths used it; the routing reads did not. Read-path fix: - discord.rs / slack.rs: stash author/user ID in metadata["sender_user_id"] - bridge.rs: route the text and audio paths through sender_user_id(message) - bridge.rs: thread user_id through handle_command() so the 6 CLI slash-command resolves (/new, /compact, /model, /stop, /usage, /think) also key on user. Tests updated for the new signature. Write-path follow-up (set_user_default + broadcast routing) deferred to a separate commit so this change can be validated in isolation. * fix(channels): close write-path keying gap; broadcast user-scoped Completes the router keying fix started in 6a90aa0. The read path resolves on user_id, but four write sites and the broadcast lookup were still keyed on sender.platform_id (channel ID on Discord/Slack), producing the split-keying state that surfaced in GAP-008. - 4 x set_user_default writes (text/audio fallback, /agent existing, /agent spawned) now key on sender_user_id(message) - 2 x broadcast lookups (has_broadcast / resolve_broadcast) switched to sender_user_id(message), matching the upstream test's intent (router.rs:521-547 keys on "vip_user", not a channel) - boot-time log warning on Discord/Slack adapter start: any pre-existing /agent default may need to be re-run once - new test test_handle_command_agent_select_keys_on_user_id_not_ platform_id locks in the round-trip --- crates/openfang-channels/src/bridge.rs | 154 ++++++++++++++++++++---- crates/openfang-channels/src/discord.rs | 3 + crates/openfang-channels/src/slack.rs | 3 + 3 files changed, 136 insertions(+), 24 deletions(-) diff --git a/crates/openfang-channels/src/bridge.rs b/crates/openfang-channels/src/bridge.rs index 522f4b67..29e70252 100644 --- a/crates/openfang-channels/src/bridge.rs +++ b/crates/openfang-channels/src/bridge.rs @@ -421,6 +421,26 @@ impl BridgeManager { adapter: Arc, ) -> Result<(), Box> { let stream = adapter.start().await?; + + // Migration note for Discord/Slack: prior versions keyed `/agent ` + // selections on the channel ID rather than the user. `user_defaults` is + // in-memory only, so the daemon restart that loads this binary already + // wipes any stale entries — but log a one-line nudge so users know to + // re-run `/agent ` if their previous selection appears to have + // gone away. See `set_user_default` call sites in `dispatch_message` + // and `handle_command` for the keying fix. + match adapter.name() { + "discord" | "slack" => { + info!( + adapter = adapter.name(), + "Channel adapter starting: per-user `/agent ` defaults are \ + in-memory and reset on daemon restart. If a previous selection \ + no longer takes effect, re-run `/agent ` once." + ); + } + _ => {} + } + let handle = self.handle.clone(); let router = self.router.clone(); let rate_limiter = self.rate_limiter.clone(); @@ -797,7 +817,15 @@ async fn dispatch_message( // Handle commands first (early return) if let ChannelContent::Command { ref name, ref args } = message.content { - let result = handle_command(name, args, handle, router, &message.sender).await; + let result = handle_command( + name, + args, + handle, + router, + &message.sender, + sender_user_id(message), + ) + .await; send_response(adapter, &message.sender, result, thread_id, output_format).await; return; } @@ -881,7 +909,15 @@ async fn dispatch_message( }; if is_channel_command(cmd) { - let result = handle_command(cmd, &args, handle, router, &message.sender).await; + let result = handle_command( + cmd, + &args, + handle, + router, + &message.sender, + sender_user_id(message), + ) + .await; send_response(adapter, &message.sender, result, thread_id, output_format).await; return; } @@ -889,8 +925,12 @@ async fn dispatch_message( } // Check broadcast routing first - if router.has_broadcast(&message.sender.platform_id) { - let targets = router.resolve_broadcast(&message.sender.platform_id); + // Broadcast lookup is keyed on the user, matching the read path's + // sender_user_id() resolution. On Discord/Slack `sender.platform_id` is the + // channel ID, so keying on it would collide with channel routing — see the + // companion fix on `set_user_default` writes below. + if router.has_broadcast(sender_user_id(message)) { + let targets = router.resolve_broadcast(sender_user_id(message)); if !targets.is_empty() { // RBAC check applies to broadcast too if let Err(denied) = handle @@ -958,10 +998,12 @@ async fn dispatch_message( } } - // Route to agent (standard path) + // Route to agent (standard path). + // Use sender_user_id() so user-keyed bindings (peer_id) match for adapters like + // Discord/Slack where sender.platform_id is the channel ID, not the user ID. let agent_id = router.resolve( &message.channel, - &message.sender.platform_id, + sender_user_id(message), message.sender.openfang_user.as_deref(), ); @@ -980,8 +1022,10 @@ async fn dispatch_message( }; match fallback { Some(id) => { - // Auto-set this as the user's default so future messages route directly - router.set_user_default(message.sender.platform_id.clone(), id); + // Auto-set this as the user's default so future messages route directly. + // Key on sender_user_id() (not platform_id) so Discord/Slack — where + // platform_id is the channel — store per-user, matching the read path. + router.set_user_default(sender_user_id(message).to_string(), id); id } None => { @@ -1399,10 +1443,11 @@ async fn dispatch_with_blocks( lifecycle_reactions: bool, prefix_style: PrefixStyle, ) { - // Route to agent (same logic as text path) + // Route to agent (same logic as text path). + // Use sender_user_id() so user-keyed bindings match for Discord/Slack. let agent_id = router.resolve( &message.channel, - &message.sender.platform_id, + sender_user_id(message), message.sender.openfang_user.as_deref(), ); @@ -1420,7 +1465,9 @@ async fn dispatch_with_blocks( }; match fallback { Some(id) => { - router.set_user_default(message.sender.platform_id.clone(), id); + // Key on sender_user_id() (not platform_id) so Discord/Slack — where + // platform_id is the channel — store per-user, matching the read path. + router.set_user_default(sender_user_id(message).to_string(), id); id } None => { @@ -1609,12 +1656,19 @@ async fn dispatch_with_blocks( } /// Handle a bot command (returns the response text). +/// +/// `user_id` is the platform user ID (e.g. Discord author ID, Slack user ID). +/// For adapters that set `sender.platform_id` to the channel/conversation ID +/// (Discord, Slack), callers must pass `sender_user_id(message)` here so that +/// per-user agent routing works correctly. For adapters where platform_id is +/// already the user (CLI, Telegram DM), the two are equivalent. async fn handle_command( name: &str, args: &[String], handle: &Arc, router: &Arc, sender: &ChannelUser, + user_id: &str, ) -> String { // Canonicalise through the unified command registry: aliases resolve to // their canonical name and matching is case-insensitive. If the command @@ -1668,14 +1722,17 @@ async fn handle_command( let agent_name = &args[0]; match handle.find_agent_by_name(agent_name).await { Ok(Some(agent_id)) => { - router.set_user_default(sender.platform_id.clone(), agent_id); + // Key on user_id (the param wired in by the Discord/Slack call sites + // via sender_user_id(message)) — not sender.platform_id, which is the + // channel ID on those adapters. Matches the read-path resolution. + router.set_user_default(user_id.to_string(), agent_id); format!("Now talking to agent: {agent_name}") } Ok(None) => { // Try to spawn it match handle.spawn_agent_by_name(agent_name).await { Ok(agent_id) => { - router.set_user_default(sender.platform_id.clone(), agent_id); + router.set_user_default(user_id.to_string(), agent_id); format!("Spawned and connected to agent: {agent_name}") } Err(e) => { @@ -1690,7 +1747,7 @@ async fn handle_command( // Need to resolve the user's current agent let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1704,7 +1761,7 @@ async fn handle_command( "compact" => { let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1718,7 +1775,7 @@ async fn handle_command( "model" => { let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1742,7 +1799,7 @@ async fn handle_command( "stop" => { let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1756,7 +1813,7 @@ async fn handle_command( "usage" => { let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1770,7 +1827,7 @@ async fn handle_command( "think" => { let agent_id = router.resolve( &crate::types::ChannelType::CLI, - &sender.platform_id, + user_id, sender.openfang_user.as_deref(), ); match agent_id { @@ -1939,10 +1996,10 @@ mod tests { openfang_user: None, }; - let result = handle_command("agents", &[], &handle, &router, &sender).await; + let result = handle_command("agents", &[], &handle, &router, &sender, "user1").await; assert!(result.contains("coder")); - let result = handle_command("help", &[], &handle, &router, &sender).await; + let result = handle_command("help", &[], &handle, &router, &sender, "user1").await; assert!(result.contains("/agents")); } @@ -1960,8 +2017,15 @@ mod tests { }; // Select existing agent - let result = - handle_command("agent", &["coder".to_string()], &handle, &router, &sender).await; + let result = handle_command( + "agent", + &["coder".to_string()], + &handle, + &router, + &sender, + "user1", + ) + .await; assert!(result.contains("Now talking to agent: coder")); // Verify router was updated @@ -1969,6 +2033,48 @@ mod tests { assert_eq!(resolved, Some(agent_id)); } + /// Discord/Slack-shaped: sender.platform_id is the *channel* id, user_id is + /// the actual user. After /agent , the default must be stored under + /// user_id and resolvable by user_id — NOT by the channel id. This is the + /// "split-keying" fix the read path has and the write path now matches. + #[tokio::test] + async fn test_handle_command_agent_select_keys_on_user_id_not_platform_id() { + let agent_id = AgentId::new(); + let handle: Arc = Arc::new(MockHandle { + agents: Mutex::new(vec![(agent_id, "coder".to_string())]), + }); + let router = Arc::new(AgentRouter::new()); + // Discord-shape: platform_id is the channel, the real user is in user_id. + let sender = ChannelUser { + platform_id: "channel-123".to_string(), + display_name: "Test".to_string(), + openfang_user: None, + }; + let user_id = "user-789"; + + let result = handle_command( + "agent", + &["coder".to_string()], + &handle, + &router, + &sender, + user_id, + ) + .await; + assert!(result.contains("Now talking to agent: coder")); + + // Resolves under the user's id (correct). + let by_user = router.resolve(&ChannelType::Discord, user_id, None); + assert_eq!(by_user, Some(agent_id), "should resolve by user_id"); + + // Does NOT resolve under the channel id (the bug we just fixed). + let by_channel = router.resolve(&ChannelType::Discord, "channel-123", None); + assert_eq!( + by_channel, None, + "must NOT resolve by sender.platform_id (channel id)" + ); + } + #[tokio::test] async fn test_handle_command_agent_without_args_lists_agents() { let agent_id = AgentId::new(); @@ -1982,7 +2088,7 @@ mod tests { openfang_user: None, }; - let result = handle_command("agent", &[], &handle, &router, &sender).await; + let result = handle_command("agent", &[], &handle, &router, &sender, "user1").await; assert!(result.contains("Usage: /agent ")); assert!(result.contains("coder")); } diff --git a/crates/openfang-channels/src/discord.rs b/crates/openfang-channels/src/discord.rs index cffa63a8..7d43e53f 100644 --- a/crates/openfang-channels/src/discord.rs +++ b/crates/openfang-channels/src/discord.rs @@ -636,6 +636,9 @@ async fn parse_discord_message( if was_mentioned { metadata.insert("was_mentioned".to_string(), serde_json::json!(true)); } + // Stash the Discord author ID so the router can key bindings on user, not channel. + // (`sender.platform_id` below is the channel ID, used for the send path.) + metadata.insert("sender_user_id".to_string(), serde_json::json!(author_id)); Some(ChannelMessage { channel: ChannelType::Discord, diff --git a/crates/openfang-channels/src/slack.rs b/crates/openfang-channels/src/slack.rs index f39d5e70..b4e81ee6 100644 --- a/crates/openfang-channels/src/slack.rs +++ b/crates/openfang-channels/src/slack.rs @@ -501,6 +501,9 @@ async fn parse_slack_event( // Check if the bot was @-mentioned (for group_policy = "mention_only") let mut metadata = HashMap::new(); + // Stash the Slack user ID so the router can key bindings on user, not channel. + // (`sender.platform_id` below is the channel ID, used for the send path.) + metadata.insert("sender_user_id".to_string(), serde_json::json!(user_id)); if event_type == "app_mention" { metadata.insert("was_mentioned".to_string(), serde_json::Value::Bool(true)); }