test: add 26 tests for multi-thread safety, db CRUD, concurrency, errors (#442)

* 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]>

* test: comprehensive testing improvements and fix MessageTool blocking_read panic

Fix tokio::sync::RwLock::blocking_read() panic in MessageTool::requires_approval()
under multi-threaded tokio runtimes by switching to std::sync::RwLock with poison
recovery. Add 26 new tests across 4 tiers:

Tier 1 - Multi-thread runtime safety:
- Fix MessageTool to use std::sync::RwLock instead of tokio::sync::RwLock
- 4 multi-thread tests for MessageTool::requires_approval() scenarios
- 1 multi-thread test for HttpTool credential-dependent approval
- 1 structural test exercising all core tool sync trait methods under multi-thread runtime

Tier 2 - Database CRUD coverage:
- Settings lifecycle (CRUD, bulk ops)
- Tool failure tracking (record, broken list, repair)
- Routine lifecycle (create, get, list, update, delete, runs)
- LLM call recording
- Sandbox job lifecycle (create, get, update, list, mode)
- Job events (save, list, limit)
- Estimation snapshot round-trip

Tier 3 - Concurrency:
- ToolRegistry concurrent register + read under 4-worker runtime

Tier 4 - Error coverage:
- Display tests for all 8 error variants
- From conversion tests for top-level Error enum

Supersedes the fix in PR #411 with the same bug fix plus comprehensive test coverage.

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

* fix: remove trailing whitespace in registry.rs

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

---------

Co-authored-by: Jerome Revillard <[email protected]>
Co-authored-by: Claude Opus 4.6 <[email protected]>
Co-authored-by: Illia Polosukhin <[email protected]>

src/tools/builtin/http.rs

@@ -998,4 +998,44 @@ mod tests {
         let params = serde_json::json!({"method": "GET"});
         assert_eq!(extract_host_from_params(&params), None);
     }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn requires_approval_multi_thread_no_panic() {
+        use crate::secrets::CredentialMapping;
+        use crate::tools::wasm::SharedCredentialRegistry;
+
+        // Test with credential registry (uses std::sync::RwLock - should be safe)
+        let registry = Arc::new(SharedCredentialRegistry::new());
+        registry.add_mappings(vec![CredentialMapping::bearer("test_key", "api.test.com")]);
+
+        let tool = HttpTool::new().with_credentials(
+            registry,
+            Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new(
+                crate::secrets::SecretsCrypto::new(secrecy::SecretString::from(
+                    "0123456789abcdef0123456789abcdef".to_string(),
+                ))
+                .unwrap(),
+            ))),
+        );
+
+        // These calls should not panic in multi-thread runtime
+        let params_no_auth = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data"
+        });
+        let _ = tool.requires_approval(&params_no_auth);
+
+        let params_with_cred = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.test.com/v1/models"
+        });
+        let _ = tool.requires_approval(&params_with_cred);
+
+        let params_with_auth = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com",
+            "headers": {"Authorization": "Bearer token"}
+        });
+        let _ = tool.requires_approval(&params_with_auth);
+    }
 }

src/tools/builtin/message.rs

@@ -533,41 +533,53 @@ mod tests {
         );
     }
 
-    /// 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()));
+    // ── Multi-thread runtime safety tests ─────────────────────────────
 
-        // Set context asynchronously (simulating real usage pattern)
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn requires_approval_no_channel_multi_thread() {
+        let tool = MessageTool::new(Arc::new(ChannelManager::new()));
+        // No channel set, no channel param - should not panic in multi-thread runtime
+        let result = tool.requires_approval(&serde_json::json!({"content": "hello"}));
+        assert_eq!(result, ApprovalRequirement::UnlessAutoApproved);
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn requires_approval_with_context_multi_thread() {
+        let tool = MessageTool::new(Arc::new(ChannelManager::new()));
         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!({
+        // No channel param - uses default, less risky
+        let result = tool.requires_approval(&serde_json::json!({"content": "hello"}));
+        assert_eq!(result, ApprovalRequirement::UnlessAutoApproved);
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn requires_approval_cross_channel_multi_thread() {
+        let tool = MessageTool::new(Arc::new(ChannelManager::new()));
+        tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string()))
+            .await;
+
+        // Different channel than default requires approval
+        let result = tool.requires_approval(&serde_json::json!({
             "content": "hello",
             "channel": "telegram"
         }));
-        // Different channel from default -> Always
-        assert!(matches!(approval, ApprovalRequirement::Always));
+        assert_eq!(result, ApprovalRequirement::Always);
+    }
 
-        // No channel specified (uses default) -> UnlessAutoApproved
-        let approval = tool.requires_approval(&serde_json::json!({
-            "content": "hello"
-        }));
-        assert!(matches!(approval, ApprovalRequirement::UnlessAutoApproved));
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn requires_approval_same_channel_explicit_multi_thread() {
+        let tool = MessageTool::new(Arc::new(ChannelManager::new()));
+        tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string()))
+            .await;
 
-        // Explicit channel (even if same as default) -> Always
-        let approval = tool.requires_approval(&serde_json::json!({
+        // Explicit channel that matches default still returns Always
+        // (existing behavior: any explicit channel param triggers Always)
+        let result = tool.requires_approval(&serde_json::json!({
             "content": "hello",
             "channel": "signal"
         }));
-        assert!(matches!(approval, ApprovalRequirement::Always));
+        assert_eq!(result, ApprovalRequirement::Always);
     }
 }

src/tools/registry.rs

@@ -765,6 +765,44 @@ mod tests {
         assert_ne!(desc, "EVIL SHADOW");
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+    async fn concurrent_register_and_read_no_panic() {
+        use std::sync::Arc as StdArc;
+
+        let registry = StdArc::new(ToolRegistry::new());
+        registry.register_builtin_tools();
+
+        // Spawn concurrent readers and check they don't panic
+        let mut handles = Vec::new();
+
+        // Readers
+        for _ in 0..10 {
+            let reg = StdArc::clone(&registry);
+            handles.push(tokio::spawn(async move {
+                let tools = reg.all().await;
+                assert!(!tools.is_empty());
+                let names = reg.list().await;
+                assert!(!names.is_empty());
+                let _ = reg.get("echo").await;
+                let _ = reg.has("echo").await;
+                let _ = reg.tool_definitions().await;
+            }));
+        }
+
+        // Concurrent register attempts (will be rejected as shadowing)
+        for _ in 0..5 {
+            let reg = StdArc::clone(&registry);
+            handles.push(tokio::spawn(async move {
+                // This will be rejected (echo is protected) but should not panic
+                reg.register(Arc::new(EchoTool)).await;
+            }));
+        }
+
+        for handle in handles {
+            handle.await.expect("task should not panic");
+        }
+    }
+
     #[tokio::test]
     async fn test_tool_definitions_sorted_alphabetically() {
         // Create tools with names that would NOT be alphabetical if inserted in this order.