fix: use std::sync::RwLock in MessageTool to avoid runtime panic (#411)

* fix: use std::sync::RwLock in MessageTool to avoid runtime panic

The `requires_approval` method is synchronous but was using
`tokio::sync::RwLock` with `.await` which requires blocking the
runtime. This caused a panic:
"Cannot block the current thread from within a runtime"

Changes:
- Replace `tokio::sync::RwLock` with `std::sync::RwLock` for
  `default_channel` and `default_target` fields
- Use `unwrap_or_else(|e| e.into_inner())` to gracefully handle
  poisoned locks (recovers instead of panicking)
- Update all usages from `.read().await` to `.read().unwrap_or_else()`

The locks are short-held (just cloning strings), making std::sync::RwLock
appropriate for sync methods called from async contexts.

Fixes: "Cannot block the current thread from within a runtime" panic
when the LLM tries to send a message via the message tool.

Co-Authored-By: Claude Opus 4.6 <[email protected]>

* fix: address code review feedback for MessageTool RwLock fix

- Fix formatting (long lines broken up per rustfmt)
- Add regression test that demonstrates the panic with tokio::sync::RwLock
  and passes with std::sync::RwLock when calling requires_approval()
  (sync method) from async context

Co-Authored-By: Claude Opus 4.6 <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>

src/tools/builtin/message.rs

@@ -3,10 +3,9 @@
 //! Allows the agent to proactively message users on any connected channel.
 
 use std::path::PathBuf;
-use std::sync::Arc;
+use std::sync::{Arc, RwLock};
 
 use async_trait::async_trait;
-use tokio::sync::RwLock;
 
 use crate::bootstrap::ironclaw_base_dir;
 use crate::channels::{ChannelManager, OutgoingResponse};
@@ -19,6 +18,7 @@ use crate::tools::tool::{
 pub struct MessageTool {
     channel_manager: Arc<ChannelManager>,
     /// Default channel for current conversation (set per-turn).
+    /// Uses std::sync::RwLock because requires_approval() is sync and called from async context.
     default_channel: Arc<RwLock<Option<String>>>,
     /// Default target (user_id or group_id) for current conversation (set per-turn).
     default_target: Arc<RwLock<Option<String>>>,
@@ -48,8 +48,14 @@ impl MessageTool {
     /// Set the default channel and target for the current conversation turn.
     /// Call this before each agent turn with the incoming message's channel/target.
     pub async fn set_context(&self, channel: Option<String>, target: Option<String>) {
-        *self.default_channel.write().await = channel;
-        *self.default_target.write().await = target;
+        *self
+            .default_channel
+            .write()
+            .unwrap_or_else(|e| e.into_inner()) = channel;
+        *self
+            .default_target
+            .write()
+            .unwrap_or_else(|e| e.into_inner()) = target;
     }
 }
 
@@ -106,24 +112,32 @@ impl Tool for MessageTool {
         let channel = if let Some(c) = params.get("channel").and_then(|v| v.as_str()) {
             c.to_string()
         } else {
-            self.default_channel.read().await.clone().ok_or_else(|| {
-                ToolError::ExecutionFailed(
-                    "No channel specified and no active conversation. Provide channel parameter."
-                        .to_string(),
-                )
-            })?
+            self.default_channel
+                .read()
+                .unwrap_or_else(|e| e.into_inner())
+                .clone()
+                .ok_or_else(|| {
+                    ToolError::ExecutionFailed(
+                        "No channel specified and no active conversation. Provide channel parameter."
+                            .to_string(),
+                    )
+                })?
         };
 
         // Get target: use param or fall back to default
         let target = if let Some(t) = params.get("target").and_then(|v| v.as_str()) {
             t.to_string()
         } else {
-            self.default_target.read().await.clone().ok_or_else(|| {
-                ToolError::ExecutionFailed(
-                    "No target specified and no active conversation. Provide target parameter."
-                        .to_string(),
-                )
-            })?
+            self.default_target
+                .read()
+                .unwrap_or_else(|e| e.into_inner())
+                .clone()
+                .ok_or_else(|| {
+                    ToolError::ExecutionFailed(
+                        "No target specified and no active conversation. Provide target parameter."
+                            .to_string(),
+                    )
+                })?
         };
 
         let attachments: Vec<String> = match params.get("attachments") {
@@ -199,7 +213,10 @@ impl Tool for MessageTool {
         let param_channel = params.get("channel").and_then(|v| v.as_str());
         if let Some(channel) = param_channel {
             // Check if it differs from the default channel
-            let default_channel = self.default_channel.blocking_read();
+            let default_channel = self
+                .default_channel
+                .read()
+                .unwrap_or_else(|e| e.into_inner());
             if let Some(default) = default_channel.as_ref()
                 && channel != default
             {
@@ -515,4 +532,42 @@ mod tests {
             err
         );
     }
+
+    /// Regression test: requires_approval() is a sync method called from async context.
+    /// With tokio::sync::RwLock, this would panic with:
+    ///   "Cannot block the current thread from within a runtime"
+    /// because blocking_read() cannot be called inside an async runtime.
+    /// With std::sync::RwLock, it works correctly since std locks are safe
+    /// for short-held locks in sync methods called from async contexts.
+    #[tokio::test]
+    async fn requires_approval_works_from_async_context() {
+        let tool = MessageTool::new(Arc::new(ChannelManager::new()));
+
+        // Set context asynchronously (simulating real usage pattern)
+        tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string()))
+            .await;
+
+        // Call requires_approval (sync method) from async context.
+        // This is the critical test: with tokio::sync::RwLock::blocking_read(),
+        // this would panic. With std::sync::RwLock::read(), it works.
+        let approval = tool.requires_approval(&serde_json::json!({
+            "content": "hello",
+            "channel": "telegram"
+        }));
+        // Different channel from default -> Always
+        assert!(matches!(approval, ApprovalRequirement::Always));
+
+        // No channel specified (uses default) -> UnlessAutoApproved
+        let approval = tool.requires_approval(&serde_json::json!({
+            "content": "hello"
+        }));
+        assert!(matches!(approval, ApprovalRequirement::UnlessAutoApproved));
+
+        // Explicit channel (even if same as default) -> Always
+        let approval = tool.requires_approval(&serde_json::json!({
+            "content": "hello",
+            "channel": "signal"
+        }));
+        assert!(matches!(approval, ApprovalRequirement::Always));
+    }
 }