diff --git a/.githooks/pre-commit b/.githooks/pre-commit index a3dbafa61..5ce940e00 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,4 +1,4 @@ -#!/bin/bash +#!/usr/bin/env bash set -euo pipefail diff --git a/Cargo.lock b/Cargo.lock index ffcc64c5e..13b48c4ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1222,7 +1222,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a6139a8597ed92cf816dfb33f5dd6cf0bb93a6adc938f11039f371bc5bcd26c3" dependencies = [ "chrono", - "phf", + "phf 0.12.1", ] [[package]] @@ -2671,6 +2671,15 @@ dependencies = [ "serde", ] +[[package]] +name = "emojis" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50c1c1870b766fc398e5f0526498d09c94b6de15be5fd769a28bbc804fb1b05d" +dependencies = [ + "phf 0.13.1", +] + [[package]] name = "encode_unicode" version = "1.0.0" @@ -6241,7 +6250,16 @@ version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "913273894cec178f401a31ec4b656318d95473527be05c0752cc41cdc32be8b7" dependencies = [ - "phf_shared", + "phf_shared 0.12.1", +] + +[[package]] +name = "phf" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c1562dc717473dbaa4c1f85a36410e03c047b2e7df7f45ee938fbef64ae7fadf" +dependencies = [ + "phf_shared 0.13.1", ] [[package]] @@ -6253,6 +6271,15 @@ dependencies = [ "siphasher", ] +[[package]] +name = "phf_shared" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e57fef6bc5981e38c2ce2d63bfa546861309f875b8a75f092d1d54ae2d64f266" +dependencies = [ + "siphasher", +] + [[package]] name = "pin-project" version = "1.1.10" @@ -8230,6 +8257,7 @@ dependencies = [ "daemonize", "dialoguer", "dirs", + "emojis", "fastembed", "futures", "hex", @@ -8271,6 +8299,7 @@ dependencies = [ "tokio", "tokio-stream", "tokio-test", + "tokio-tungstenite 0.28.0", "toml 0.8.23", "toml_edit 0.22.27", "tower-http", diff --git a/Cargo.toml b/Cargo.toml index ba7a53a3c..1bed533d3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -93,9 +93,13 @@ async-trait = "0.1" # Slack slack-morphism = { version = "2.17", features = ["hyper"] } +emojis = "0.8" # TLS (shared crypto backend for slack-morphism, reqwest, teloxide) rustls = { version = "0.23", default-features = false, features = ["ring"] } +# slack-morphism enables tokio-tungstenite/rustls-native-certs, but tokio-tungstenite 0.28 +# restructured features — rustls-tls-native-roots is now needed to activate actual TLS. +tokio-tungstenite = { version = "0.28", features = ["rustls-tls-native-roots"] } # Telegram teloxide = { version = "0.17", default-features = false, features = ["rustls"] } diff --git a/src/config.rs b/src/config.rs index 20e3394d7..889c4f902 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1025,7 +1025,15 @@ impl SlackPermissions { filter }; - let dm_allowed_users = slack.dm_allowed_users.clone(); + let mut dm_allowed_users = slack.dm_allowed_users.clone(); + + for binding in &slack_bindings { + for id in &binding.dm_allowed_users { + if !dm_allowed_users.contains(id) { + dm_allowed_users.push(id.clone()); + } + } + } Self { workspace_filter, @@ -4335,6 +4343,105 @@ bind = "127.0.0.1" assert_eq!(config.api.bind, "[::]"); } + /// Helper to build a minimal `SlackConfig` for permission tests. + fn slack_config_with_dm_users(dm_allowed_users: Vec) -> SlackConfig { + SlackConfig { + enabled: true, + bot_token: "xoxb-test".into(), + app_token: "xapp-test".into(), + dm_allowed_users, + commands: vec![], + } + } + + /// Helper to build a Slack binding with optional dm_allowed_users. + fn slack_binding(workspace_id: Option<&str>, dm_allowed_users: Vec) -> Binding { + Binding { + agent_id: "test-agent".into(), + channel: "slack".into(), + guild_id: None, + workspace_id: workspace_id.map(String::from), + chat_id: None, + channel_ids: vec![], + require_mention: false, + dm_allowed_users, + } + } + + #[test] + fn slack_permissions_merges_dm_users_from_config_and_bindings() { + let config = slack_config_with_dm_users(vec!["U001".into(), "U002".into()]); + let bindings = vec![slack_binding( + Some("T1"), + vec!["U003".into(), "U004".into()], + )]; + let perms = SlackPermissions::from_config(&config, &bindings); + assert_eq!(perms.dm_allowed_users, vec!["U001", "U002", "U003", "U004"]); + } + + #[test] + fn slack_permissions_deduplicates_dm_users() { + let config = slack_config_with_dm_users(vec!["U001".into(), "U002".into()]); + let bindings = vec![slack_binding( + Some("T1"), + vec!["U002".into(), "U003".into()], + )]; + let perms = SlackPermissions::from_config(&config, &bindings); + // U002 appears in both config and binding — should appear only once + assert_eq!(perms.dm_allowed_users, vec!["U001", "U002", "U003"]); + } + + #[test] + fn slack_permissions_empty_dm_users_stays_empty() { + let config = slack_config_with_dm_users(vec![]); + let bindings = vec![slack_binding(Some("T1"), vec![])]; + let perms = SlackPermissions::from_config(&config, &bindings); + assert!(perms.dm_allowed_users.is_empty()); + } + + #[test] + fn slack_permissions_merges_dm_users_from_multiple_bindings() { + let config = slack_config_with_dm_users(vec!["U001".into()]); + let bindings = vec![ + slack_binding(Some("T1"), vec!["U002".into()]), + slack_binding(Some("T2"), vec!["U003".into()]), + ]; + let perms = SlackPermissions::from_config(&config, &bindings); + assert_eq!(perms.dm_allowed_users, vec!["U001", "U002", "U003"]); + } + + #[test] + fn slack_permissions_ignores_non_slack_bindings() { + let config = slack_config_with_dm_users(vec!["U001".into()]); + let mut discord_binding = slack_binding(Some("T1"), vec!["U099".into()]); + discord_binding.channel = "discord".into(); + let perms = SlackPermissions::from_config(&config, &[discord_binding]); + // U099 should not appear — that binding is for discord, not slack + assert_eq!(perms.dm_allowed_users, vec!["U001"]); + } + + #[test] + fn slack_permissions_workspace_filter_from_bindings() { + let config = slack_config_with_dm_users(vec![]); + let bindings = vec![ + slack_binding(Some("T1"), vec![]), + slack_binding(Some("T2"), vec![]), + ]; + let perms = SlackPermissions::from_config(&config, &bindings); + assert_eq!( + perms.workspace_filter, + Some(vec!["T1".to_string(), "T2".to_string()]) + ); + } + + #[test] + fn slack_permissions_no_workspace_filter_when_none_specified() { + let config = slack_config_with_dm_users(vec![]); + let bindings = vec![slack_binding(None, vec![])]; + let perms = SlackPermissions::from_config(&config, &bindings); + assert!(perms.workspace_filter.is_none()); + } + #[test] fn test_cron_timezone_resolution_precedence() { let _lock = env_test_lock() diff --git a/src/messaging/slack.rs b/src/messaging/slack.rs index 51e09375e..2aa1e3cbf 100644 --- a/src/messaging/slack.rs +++ b/src/messaging/slack.rs @@ -117,12 +117,29 @@ async fn handle_push_event( ) -> UserCallbackResult<()> { match event.event { SlackEventCallbackBody::Message(msg) => { + let channel = msg + .origin + .channel + .as_ref() + .map(|c| c.0.as_str()) + .unwrap_or("none"); + let sender = msg + .sender + .user + .as_ref() + .map(|u| u.0.as_str()) + .unwrap_or("none"); + let subtype = msg.subtype.as_ref().map(|s| format!("{:?}", s)); + tracing::debug!(channel, sender, ?subtype, "slack push event: message"); handle_message_event(msg, &event.team_id, client, states).await } SlackEventCallbackBody::AppMention(mention) => { handle_app_mention_event(mention, &event.team_id, client, states).await } - _ => Ok(()), + _ => { + tracing::debug!(event_type = ?std::mem::discriminant(&event.event), "slack push event: unhandled"); + Ok(()) + } } } @@ -166,32 +183,46 @@ async fn handle_message_event( let ts = msg_event.origin.ts.0.clone(); let perms = adapter_state.permissions.load(); + let is_dm = channel_id.starts_with('D'); - // DM filter - if channel_id.starts_with('D') { + // DM filter — allowed DMs skip workspace/channel filters entirely + if is_dm { if perms.dm_allowed_users.is_empty() { + tracing::debug!(channel_id, "DM dropped: dm_allowed_users is empty"); return Ok(()); } - if let Some(ref uid) = user_id - && !perms.dm_allowed_users.contains(uid) + if let Some(ref sender_id) = user_id + && !perms.dm_allowed_users.contains(sender_id) { + tracing::debug!( + channel_id, + user_id = sender_id.as_str(), + "DM dropped: user not in dm_allowed_users" + ); return Ok(()); } + tracing::info!( + channel_id, + ?user_id, + "DM permitted, bypassing channel filter" + ); } - // Workspace filter - if let Some(ref filter) = perms.workspace_filter - && !filter.contains(&team_id_str) - { - return Ok(()); - } + if !is_dm { + // Workspace filter + if let Some(ref filter) = perms.workspace_filter + && !filter.contains(&team_id_str) + { + return Ok(()); + } - // Channel filter - if let Some(allowed) = perms.channel_filter.get(&team_id_str) - && !allowed.is_empty() - && !allowed.contains(&channel_id) - { - return Ok(()); + // Channel filter + if let Some(allowed) = perms.channel_filter.get(&team_id_str) + && !allowed.is_empty() + && !allowed.contains(&channel_id) + { + return Ok(()); + } } let conversation_id = if let Some(ref thread_ts) = msg_event.origin.thread_ts { @@ -1499,10 +1530,38 @@ fn split_message(text: &str, max_len: usize) -> Vec { chunks } -/// Sanitize an emoji name for Slack reactions (strip colons, lowercase). +/// Convert an emoji input to a Slack reaction short-code name. +/// +/// Handles three input forms: +/// 1. Unicode emoji (e.g. "👍") → looked up via the `emojis` crate → "thumbsup" +/// 2. Colon-wrapped short-code (e.g. ":thumbsup:") → stripped to "thumbsup" +/// 3. Plain short-code (e.g. "thumbsup") → passed through as-is fn sanitize_reaction_name(emoji: &str) -> String { - emoji - .trim() + let trimmed = emoji.trim(); + if let Some(emoji) = emojis::get(trimmed) { + if let Some(shortcode) = emoji.shortcode() { + // Note: shortcodes come from gemoji (GitHub's set) which may not match Slack's + // shortcode names for uncommon emojis. Common emojis (thumbsup, heart, etc.) are + // consistent across both sets. + tracing::debug!( + unicode = trimmed, + shortcode, + "resolved unicode emoji to shortcode" + ); + return shortcode.to_string(); + } + // Unicode emoji matched but has no shortcode — use the emoji's name as fallback. + // Raw unicode would be rejected by Slack's reactions API. + let name = emoji.name().replace(' ', "_").to_lowercase(); + tracing::warn!( + unicode = trimmed, + fallback_name = %name, + "emoji matched but has no shortcode, using name as fallback" + ); + return name; + } + // Fall back to stripping colons and lowercasing (handles ":thumbsup:" and "thumbsup"). + trimmed .trim_start_matches(':') .trim_end_matches(':') .to_lowercase() @@ -1522,3 +1581,80 @@ fn resolve_slack_user_identity(user: &SlackUser, user_id: &str) -> SlackUserIden username, } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn sanitize_reaction_name_unicode_emoji_with_shortcode() { + // gemoji maps 👍 to "+1" — verify we get the shortcode, not the unicode back + let result = sanitize_reaction_name("\u{1F44D}"); // 👍 + assert_eq!( + result, "+1", + "should resolve unicode thumbs-up to its gemoji shortcode" + ); + } + + #[test] + fn sanitize_reaction_name_unicode_heart() { + let result = sanitize_reaction_name("\u{2764}\u{FE0F}"); // ❤️ + assert_eq!(result, "heart"); + } + + #[test] + fn sanitize_reaction_name_colon_wrapped_shortcode() { + let result = sanitize_reaction_name(":thumbsup:"); + assert_eq!(result, "thumbsup"); + } + + #[test] + fn sanitize_reaction_name_plain_shortcode() { + let result = sanitize_reaction_name("thumbsup"); + assert_eq!(result, "thumbsup"); + } + + #[test] + fn sanitize_reaction_name_colon_wrapped_uppercased() { + let result = sanitize_reaction_name(":ThumbsUp:"); + assert_eq!(result, "thumbsup"); + } + + #[test] + fn sanitize_reaction_name_whitespace_trimmed() { + let result = sanitize_reaction_name(" :fire: "); + // After trim, this won't match emojis::get (it's a shortcode string), + // so falls through to colon-stripping path + assert_eq!(result, "fire"); + } + + #[test] + fn sanitize_reaction_name_unicode_emoji_without_shortcode() { + // The emojis crate may have entries without shortcodes. + // Find one programmatically to keep the test resilient. + let emoji_without_shortcode = emojis::iter().find(|e| e.shortcode().is_none()); + if let Some(emoji) = emoji_without_shortcode { + let result = sanitize_reaction_name(emoji.as_str()); + let expected = emoji.name().replace(' ', "_").to_lowercase(); + assert_eq!( + result, expected, + "emoji without shortcode should fall back to name with underscores" + ); + } + // If all emojis have shortcodes, the fallback path is untestable + // with real data, but the code path still exists for safety. + } + + #[test] + fn sanitize_reaction_name_custom_slack_emoji() { + // Custom Slack emojis come as plain names like "partyparrot" + let result = sanitize_reaction_name("partyparrot"); + assert_eq!(result, "partyparrot"); + } + + #[test] + fn sanitize_reaction_name_custom_with_colons() { + let result = sanitize_reaction_name(":partyparrot:"); + assert_eq!(result, "partyparrot"); + } +} diff --git a/src/tools/browser.rs b/src/tools/browser.rs index 2d912b69e..f6e692f9b 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -5,7 +5,6 @@ //! ref system for LLM-friendly element addressing. use crate::config::BrowserConfig; - use chromiumoxide::browser::{Browser, BrowserConfig as ChromeConfig}; use chromiumoxide::page::ScreenshotParams; use chromiumoxide_cdp::cdp::browser_protocol::accessibility::{