fix(signal): review comments

Author: ibhagwan

src/config.rs

@@ -541,11 +541,9 @@ bind = "127.0.0.1"
             guild_id: None,
             workspace_id: workspace_id.map(String::from),
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users,
-            group_allowed_users: vec![],
         }
     }
 
@@ -1315,11 +1313,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         assert_eq!(binding.runtime_adapter_key(), "telegram:sales");
     }
@@ -1333,11 +1329,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         assert!(binding.uses_default_adapter());
     }
@@ -1366,11 +1360,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         let message = test_inbound_message("telegram", None);
         assert!(binding_adapter_matches(&binding, &message));
@@ -1385,11 +1377,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         let message = test_inbound_message("telegram", Some("telegram:support"));
         assert!(binding_adapter_matches(&binding, &message));
@@ -1404,11 +1394,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         let message = test_inbound_message("telegram", None);
         assert!(!binding_adapter_matches(&binding, &message));
@@ -1423,11 +1411,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         let message = test_inbound_message("telegram", Some("telegram:support"));
         assert!(!binding_adapter_matches(&binding, &message));
@@ -1442,11 +1428,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         };
         let message = test_inbound_message("telegram", Some("telegram:sales"));
         assert!(!binding_adapter_matches(&binding, &message));
@@ -1481,11 +1465,10 @@ association_max_per_pass = 55
                 guild_id: None,
                 workspace_id: None,
                 chat_id: None,
-                group_ids: vec![],
+
                 channel_ids: vec![],
                 require_mention: false,
                 dm_allowed_users: vec![],
-                group_allowed_users: vec![],
             },
             Binding {
                 agent_id: "support-agent".into(),
@@ -1494,11 +1477,10 @@ association_max_per_pass = 55
                 guild_id: None,
                 workspace_id: None,
                 chat_id: None,
-                group_ids: vec![],
+
                 channel_ids: vec![],
                 require_mention: false,
                 dm_allowed_users: vec![],
-                group_allowed_users: vec![],
             },
         ];
         assert!(validate_named_messaging_adapters(&messaging, &bindings).is_ok());
@@ -1527,11 +1509,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         }];
         assert!(validate_named_messaging_adapters(&messaging, &bindings).is_err());
     }
@@ -1592,11 +1572,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         }];
         assert!(validate_named_messaging_adapters(&messaging, &bindings).is_err());
     }
@@ -1630,11 +1608,9 @@ association_max_per_pass = 55
             guild_id: None,
             workspace_id: None,
             chat_id: None,
-            group_ids: vec![],
             channel_ids: vec![],
             require_mention: false,
             dm_allowed_users: vec![],
-            group_allowed_users: vec![],
         }];
         assert!(validate_named_messaging_adapters(&messaging, &bindings).is_err());
     }

src/config/load.rs

@@ -2153,15 +2153,16 @@ impl Config {
                 let account = std::env::var("SIGNAL_ACCOUNT")
                     .ok()
                     .or_else(|| s.account.as_deref().and_then(resolve_env_value));
+                let has_default_credentials = http_url.is_some() && account.is_some();
 
-                if (http_url.is_none() || account.is_none())
-                    && !instances.iter().any(|inst| inst.enabled)
+                if !has_default_credentials
+                    && !instances.iter().any(|instance| instance.enabled)
                 {
                     return None;
                 }
 
                 Some(SignalConfig {
-                    enabled: s.enabled,
+                    enabled: s.enabled && has_default_credentials,
                     http_url: http_url.unwrap_or_default(),
                     account: account.unwrap_or_default(),
                     instances,

src/messaging/signal.rs

@@ -1079,7 +1079,13 @@ async fn sse_listener(
                     "signal SSE returned error, retrying in {:?}",
                     retry_delay
                 );
-                tokio::time::sleep(retry_delay).await;
+                tokio::select! {
+                    _ = tokio::time::sleep(retry_delay) => {}
+                    _ = shutdown_rx.recv() => {
+                        tracing::info!("signal SSE listener shutting down during retry");
+                        return;
+                    }
+                }
                 retry_delay = (retry_delay * 2).min(SSE_MAX_BACKOFF);
                 continue;
             }
@@ -1089,7 +1095,13 @@ async fn sse_listener(
                     "signal SSE connect error, retrying in {:?}",
                     retry_delay
                 );
-                tokio::time::sleep(retry_delay).await;
+                tokio::select! {
+                    _ = tokio::time::sleep(retry_delay) => {}
+                    _ = shutdown_rx.recv() => {
+                        tracing::info!("signal SSE listener shutting down during retry");
+                        return;
+                    }
+                }
                 retry_delay = (retry_delay * 2).min(SSE_MAX_BACKOFF);
                 continue;
             }
@@ -1229,7 +1241,13 @@ async fn sse_listener(
         }
 
         tracing::debug!("signal SSE stream ended, reconnecting with backoff...");
-        tokio::time::sleep(retry_delay).await;
+        tokio::select! {
+            _ = tokio::time::sleep(retry_delay) => {}
+            _ = shutdown_rx.recv() => {
+                tracing::info!("signal SSE listener shutting down during reconnect");
+                return;
+            }
+        }
         retry_delay = (retry_delay * 2).min(SSE_MAX_BACKOFF);
     }
 }

src/messaging/target.rs

@@ -341,13 +341,15 @@ fn json_value_to_string(value: &serde_json::Value) -> Option<String> {
 fn extract_signal_adapter_from_channel_id(channel_id: &str) -> String {
     let parts: Vec<&str> = channel_id.split(':').collect();
     match parts.as_slice() {
-        // Named adapter: signal:{instance}:{target}
-        // target must start with "uuid:", "group:", or "+" to confirm it's a real target
-        ["signal", instance, target_part, ..]
-            if target_part.starts_with("uuid:")
-                || target_part.starts_with("group:")
-                || target_part.starts_with('+') =>
-        {
+        // Named adapter: signal:{instance}:uuid:{uuid}, signal:{instance}:group:{id}
+        // or signal:{instance}:e164:+{phone}
+        ["signal", instance, "uuid", ..]
+        | ["signal", instance, "group", ..]
+        | ["signal", instance, "e164", ..] => {
+            format!("signal:{instance}")
+        }
+        // Named adapter: signal:{instance}:+{phone}
+        ["signal", instance, phone, ..] if phone.starts_with('+') => {
             format!("signal:{instance}")
         }
         // Default adapter: signal:{target}
@@ -386,6 +388,11 @@ pub fn parse_signal_target_parts(parts: &[&str]) -> Option<BroadcastTarget> {
                 target,
             })
         }
+        // Single-part targets: delegate to normalize_signal_target for bare UUIDs/phones
+        [single] => normalize_signal_target(single).map(|target| BroadcastTarget {
+            adapter: "signal".to_string(),
+            target,
+        }),
         // Named adapter: signal:instance:uuid:xxx, signal:instance:group:xxx
         [instance, "uuid", uuid] => Some(BroadcastTarget {
             adapter: format!("signal:{instance}"),
@@ -409,6 +416,11 @@ pub fn parse_signal_target_parts(parts: &[&str]) -> Option<BroadcastTarget> {
                 target,
             })
         }
+        // Named adapter with single-part target: delegate to normalize_signal_target
+        [instance, single] => normalize_signal_target(single).map(|target| BroadcastTarget {
+            adapter: format!("signal:{instance}"),
+            target,
+        }),
         _ => None,
     }
 }

src/tools/send_message_to_another_channel.rs

@@ -144,13 +144,27 @@ impl Tool for SendMessageTool {
             "send_message_to_another_channel tool called"
         );
 
-        // Check for explicit Signal target (named adapter only)
-        // Only treat as explicit if it's a named adapter (e.g., "signal:work:..."),
-        // not the default "signal:..." forms which should route via current_adapter.
-        // Default "signal:" targets fall through to channel lookup or current_adapter below.
-        if let Some(explicit_target) = parse_explicit_signal_target(&args.target) {
-            // Only proceed if this is a named adapter (not the default "signal")
-            if explicit_target.adapter != "signal" {
+        // Check for explicit Signal target
+        // Treat as explicit if it has a valid Signal target form (uuid:, group:, +)
+        // regardless of whether it's the default "signal" adapter or a named one.
+        if let Some(mut explicit_target) = parse_explicit_signal_target(&args.target) {
+            if explicit_target.adapter == "signal"
+                && let Some(current_adapter) = self
+                    .current_adapter
+                    .as_ref()
+                    .filter(|adapter| adapter.starts_with("signal"))
+            {
+                explicit_target.adapter = current_adapter.clone();
+            }
+
+            // Check if this is an explicit Signal target form
+            let is_explicit_signal_target = explicit_target.target.starts_with("uuid:")
+                || explicit_target.target.starts_with("group:")
+                || explicit_target.target.starts_with("e164:")
+                || explicit_target.target.starts_with('+');
+
+            // Broadcast if it's a named adapter OR an explicit Signal target form
+            if explicit_target.adapter != "signal" || is_explicit_signal_target {
                 self.messaging_manager
                     .broadcast(
                         &explicit_target.adapter,
@@ -180,7 +194,7 @@ impl Tool for SendMessageTool {
                     platform: explicit_target.adapter,
                 });
             }
-            // If adapter is "signal" (default), fall through to use current_adapter
+            // If adapter is "signal" and not an explicit target form, fall through to use current_adapter
         }
 
         // Check for explicit email target