From 06c84a5c77215e844acec7efe7c90ad9a40ba509 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 5 Mar 2026 20:39:04 -0800 Subject: [PATCH] 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 * 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 * fix: remove trailing whitespace in registry.rs Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Jerome Revillard Co-authored-by: Claude Opus 4.6 Co-authored-by: Illia Polosukhin --- src/error.rs | 144 ++++++++ src/testing.rs | 579 ++++++++++++++++++++++++++++++++ src/tools/builtin/http.rs | 40 +++ src/tools/builtin/message.rs | 60 ++-- src/tools/registry.rs | 38 +++ tests/tool_schema_validation.rs | 26 ++ 6 files changed, 863 insertions(+), 24 deletions(-) diff --git a/src/error.rs b/src/error.rs index 4c746122..973e0150 100644 --- a/src/error.rs +++ b/src/error.rs @@ -422,3 +422,147 @@ pub enum RoutineError { /// Result type alias for the agent. pub type Result = std::result::Result; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn config_error_display() { + let err = ConfigError::MissingEnvVar("DATABASE_URL".to_string()); + let msg = err.to_string(); + assert!( + msg.contains("DATABASE_URL"), + "Should mention the variable name: {msg}" + ); + + let err = ConfigError::MissingRequired { + key: "llm.model".to_string(), + hint: "Set LLM_MODEL env var".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("llm.model"), "Should mention the key: {msg}"); + assert!( + msg.contains("Set LLM_MODEL"), + "Should include the hint: {msg}" + ); + + let err = ConfigError::InvalidValue { + key: "port".to_string(), + message: "must be a number".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("port"), "Should mention the key: {msg}"); + } + + #[test] + fn database_error_display() { + let err = DatabaseError::NotFound { + entity: "conversation".to_string(), + id: "abc-123".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("conversation"), "Should mention entity: {msg}"); + assert!(msg.contains("abc-123"), "Should mention id: {msg}"); + + let err = DatabaseError::Query("syntax error near SELECT".to_string()); + assert!(err.to_string().contains("syntax error")); + } + + #[test] + fn channel_error_display() { + let err = ChannelError::StartupFailed { + name: "telegram".to_string(), + reason: "invalid token".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("telegram"), "Should mention channel: {msg}"); + assert!( + msg.contains("invalid token"), + "Should mention reason: {msg}" + ); + } + + #[test] + fn llm_error_display() { + let err = LlmError::ContextLengthExceeded { + used: 100_000, + limit: 50_000, + }; + let msg = err.to_string(); + assert!(msg.contains("100000"), "Should mention used tokens: {msg}"); + assert!(msg.contains("50000"), "Should mention limit: {msg}"); + + let err = LlmError::RateLimited { + provider: "openai".to_string(), + retry_after: Some(Duration::from_secs(30)), + }; + let msg = err.to_string(); + assert!(msg.contains("openai"), "Should mention provider: {msg}"); + } + + #[test] + fn job_error_display() { + let err = JobError::MaxJobsExceeded { max: 5 }; + let msg = err.to_string(); + assert!(msg.contains("5"), "Should mention max: {msg}"); + + let id = Uuid::new_v4(); + let err = JobError::NotFound { id }; + let msg = err.to_string(); + assert!( + msg.contains(&id.to_string()), + "Should mention job id: {msg}" + ); + } + + #[test] + fn safety_error_display() { + let err = SafetyError::InjectionDetected { + pattern: "SYSTEM:".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("SYSTEM:"), "Should mention pattern: {msg}"); + } + + #[test] + fn workspace_error_display() { + let err = WorkspaceError::DocumentNotFound { + doc_type: "notes".to_string(), + user_id: "user1".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("notes"), "Should mention doc_type: {msg}"); + assert!(msg.contains("user1"), "Should mention user_id: {msg}"); + } + + #[test] + fn routine_error_display() { + let err = RoutineError::InvalidCron { + reason: "bad format".to_string(), + }; + let msg = err.to_string(); + assert!(msg.contains("bad format"), "Should mention reason: {msg}"); + } + + #[test] + fn top_level_error_from_conversions() { + let config_err = ConfigError::MissingEnvVar("TEST".to_string()); + let err: Error = config_err.into(); + assert!(matches!(err, Error::Config(_))); + + let db_err = DatabaseError::Query("test".to_string()); + let err: Error = db_err.into(); + assert!(matches!(err, Error::Database(_))); + + let job_err = JobError::MaxJobsExceeded { max: 1 }; + let err: Error = job_err.into(); + assert!(matches!(err, Error::Job(_))); + + let safety_err = SafetyError::ValidationFailed { + reason: "test".to_string(), + }; + let err: Error = safety_err.into(); + assert!(matches!(err, Error::Safety(_))); + } +} diff --git a/src/testing.rs b/src/testing.rs index d0bc2e6a..7c36dc98 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -652,4 +652,583 @@ mod tests { assert_eq!(response.content, "hello world"); assert_eq!(response.finish_reason, FinishReason::Stop); } + + // === Database CRUD coverage for untested trait methods === + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_settings_crud() { + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + // Initially no setting + let val = db.get_setting("user1", "theme").await.expect("get"); + assert!(val.is_none()); + + // Set a value + db.set_setting("user1", "theme", &serde_json::json!("dark")) + .await + .expect("set"); + + // Read it back + let val = db + .get_setting("user1", "theme") + .await + .expect("get") + .expect("should exist"); + assert_eq!(val, serde_json::json!("dark")); + + // Update it + db.set_setting("user1", "theme", &serde_json::json!("light")) + .await + .expect("set update"); + let val = db + .get_setting("user1", "theme") + .await + .expect("get") + .expect("should exist"); + assert_eq!(val, serde_json::json!("light")); + + // List settings + let all = db.list_settings("user1").await.expect("list"); + assert_eq!(all.len(), 1); + + // Delete + let deleted = db.delete_setting("user1", "theme").await.expect("delete"); + assert!(deleted); + + let val = db.get_setting("user1", "theme").await.expect("get"); + assert!(val.is_none()); + + // Delete non-existent + let deleted = db.delete_setting("user1", "theme").await.expect("delete"); + assert!(!deleted); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_settings_bulk_operations() { + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + // Initially no settings + let has = db.has_settings("bulk_user").await.expect("has_settings"); + assert!(!has); + + // Set all settings at once + let mut settings = std::collections::HashMap::new(); + settings.insert("key1".to_string(), serde_json::json!("value1")); + settings.insert("key2".to_string(), serde_json::json!(42)); + db.set_all_settings("bulk_user", &settings) + .await + .expect("set_all"); + + // Has settings should now be true + let has = db.has_settings("bulk_user").await.expect("has_settings"); + assert!(has); + + // Get all settings + let all = db.get_all_settings("bulk_user").await.expect("get_all"); + assert_eq!(all.len(), 2); + assert_eq!(all["key1"], serde_json::json!("value1")); + assert_eq!(all["key2"], serde_json::json!(42)); + + // Get full setting row + let full = db + .get_setting_full("bulk_user", "key1") + .await + .expect("get_full") + .expect("should exist"); + assert_eq!(full.key, "key1"); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_tool_failure_tracking() { + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + // Record some failures + db.record_tool_failure("bad_tool", "connection refused") + .await + .expect("record 1"); + db.record_tool_failure("bad_tool", "timeout") + .await + .expect("record 2"); + db.record_tool_failure("bad_tool", "parse error") + .await + .expect("record 3"); + + // Get broken tools (threshold = 2, should include bad_tool with 3 failures) + let broken = db.get_broken_tools(2).await.expect("get broken"); + assert!(!broken.is_empty()); + let found = broken.iter().find(|b| b.name == "bad_tool"); + assert!(found.is_some(), "bad_tool should be in broken tools list"); + + // Mark as repaired + db.mark_tool_repaired("bad_tool") + .await + .expect("mark repaired"); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_routine_crud() { + use crate::agent::routine::{ + NotifyConfig, Routine, RoutineAction, RoutineGuardrails, RoutineRun, RunStatus, Trigger, + }; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + let routine_id = uuid::Uuid::new_v4(); + let routine = Routine { + id: routine_id, + name: "test-routine".to_string(), + description: "A test routine".to_string(), + user_id: "user1".to_string(), + enabled: true, + trigger: Trigger::Cron { + schedule: "0 * * * *".to_string(), + }, + action: RoutineAction::Lightweight { + prompt: "Check status".to_string(), + context_paths: vec![], + max_tokens: 500, + }, + guardrails: RoutineGuardrails { + cooldown: std::time::Duration::from_secs(60), + max_concurrent: 1, + dedup_window: None, + }, + notify: NotifyConfig { + channel: None, + user: "user1".to_string(), + on_attention: true, + on_failure: true, + on_success: false, + }, + last_run_at: None, + next_fire_at: None, + run_count: 0, + consecutive_failures: 0, + state: serde_json::json!({}), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + }; + + // Create + db.create_routine(&routine).await.expect("create routine"); + + // Get by ID + let fetched = db + .get_routine(routine_id) + .await + .expect("get routine") + .expect("should exist"); + assert_eq!(fetched.name, "test-routine"); + assert!(fetched.enabled); + + // Get by name + let by_name = db + .get_routine_by_name("user1", "test-routine") + .await + .expect("get by name") + .expect("should exist"); + assert_eq!(by_name.id, routine_id); + + // List routines for user + let list = db.list_routines("user1").await.expect("list routines"); + assert_eq!(list.len(), 1); + + // List all routines + let all = db.list_all_routines().await.expect("list all"); + assert!(!all.is_empty()); + + // Update routine (disable + change description) + let mut updated = fetched; + updated.enabled = false; + updated.description = "Updated description".to_string(); + db.update_routine(&updated).await.expect("update routine"); + + let re_fetched = db + .get_routine(routine_id) + .await + .expect("get") + .expect("exists"); + assert!(!re_fetched.enabled); + assert_eq!(re_fetched.description, "Updated description"); + + // Create a routine run + let run_id = uuid::Uuid::new_v4(); + let run = RoutineRun { + id: run_id, + routine_id, + trigger_type: "cron".to_string(), + trigger_detail: Some("0 * * * *".to_string()), + started_at: chrono::Utc::now(), + completed_at: None, + status: RunStatus::Running, + result_summary: None, + tokens_used: None, + job_id: None, + created_at: chrono::Utc::now(), + }; + db.create_routine_run(&run).await.expect("create run"); + + // List runs + let runs = db + .list_routine_runs(routine_id, 10) + .await + .expect("list runs"); + assert_eq!(runs.len(), 1); + assert!(matches!(runs[0].status, RunStatus::Running)); + + // Complete the run + db.complete_routine_run(run_id, RunStatus::Ok, Some("All good"), Some(150)) + .await + .expect("complete run"); + + let runs = db + .list_routine_runs(routine_id, 10) + .await + .expect("list runs after complete"); + assert!(matches!(runs[0].status, RunStatus::Ok)); + + // Delete + let deleted = db.delete_routine(routine_id).await.expect("delete"); + assert!(deleted); + + // Delete non-existent + let deleted = db.delete_routine(routine_id).await.expect("delete again"); + assert!(!deleted); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_routine_runtime_update() { + use crate::agent::routine::{ + NotifyConfig, Routine, RoutineAction, RoutineGuardrails, Trigger, + }; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + let routine_id = uuid::Uuid::new_v4(); + let routine = Routine { + id: routine_id, + name: "runtime-test".to_string(), + description: "Test runtime update".to_string(), + user_id: "user1".to_string(), + enabled: true, + trigger: Trigger::Manual, + action: RoutineAction::Lightweight { + prompt: "test".to_string(), + context_paths: vec![], + max_tokens: 100, + }, + guardrails: RoutineGuardrails { + cooldown: std::time::Duration::from_secs(0), + max_concurrent: 1, + dedup_window: None, + }, + notify: NotifyConfig { + channel: None, + user: "user1".to_string(), + on_attention: false, + on_failure: false, + on_success: false, + }, + last_run_at: None, + next_fire_at: None, + run_count: 0, + consecutive_failures: 0, + state: serde_json::json!({}), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + }; + db.create_routine(&routine).await.expect("create"); + + let now = chrono::Utc::now(); + db.update_routine_runtime( + routine_id, + now, + Some(now + chrono::TimeDelta::seconds(3600)), + 5, + 2, + &serde_json::json!({"last_result": "ok"}), + ) + .await + .expect("update runtime"); + + let fetched = db + .get_routine(routine_id) + .await + .expect("get") + .expect("exists"); + assert_eq!(fetched.run_count, 5); + assert_eq!(fetched.consecutive_failures, 2); + assert!(fetched.last_run_at.is_some()); + assert!(fetched.next_fire_at.is_some()); + + // Cleanup + db.delete_routine(routine_id).await.expect("delete"); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_llm_call_recording() { + use crate::history::LlmCallRecord; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + let record = LlmCallRecord { + job_id: None, + conversation_id: None, + provider: "openai", + model: "gpt-4", + input_tokens: 100, + output_tokens: 50, + cost: Decimal::new(5, 3), // 0.005 + purpose: Some("test"), + }; + + let call_id = db.record_llm_call(&record).await.expect("record llm call"); + assert!(!call_id.is_nil()); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_sandbox_job_lifecycle() { + use crate::history::SandboxJobRecord; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + let job_id = uuid::Uuid::new_v4(); + let job = SandboxJobRecord { + id: job_id, + task: "Build a test tool".to_string(), + status: "creating".to_string(), + user_id: "user1".to_string(), + project_dir: "/workspace/test".to_string(), + success: None, + failure_reason: None, + created_at: chrono::Utc::now(), + started_at: None, + completed_at: None, + credential_grants_json: "[]".to_string(), + }; + + // Create + db.save_sandbox_job(&job).await.expect("save sandbox job"); + + // Get + let fetched = db + .get_sandbox_job(job_id) + .await + .expect("get") + .expect("should exist"); + assert_eq!(fetched.task, "Build a test tool"); + assert_eq!(fetched.status, "creating"); + + // Update status to running + db.update_sandbox_job_status( + job_id, + "running", + None, + None, + Some(chrono::Utc::now()), + None, + ) + .await + .expect("update to running"); + + // Update to completed + db.update_sandbox_job_status( + job_id, + "completed", + Some(true), + Some("Done"), + None, + Some(chrono::Utc::now()), + ) + .await + .expect("update to completed"); + + let fetched = db + .get_sandbox_job(job_id) + .await + .expect("get") + .expect("should exist"); + assert_eq!(fetched.status, "completed"); + assert_eq!(fetched.success, Some(true)); + + // List + let all = db.list_sandbox_jobs().await.expect("list"); + assert!(!all.is_empty()); + + // Summary + let summary = db.sandbox_job_summary().await.expect("summary"); + assert!(summary.total >= 1); + + // Per-user list + let user_jobs = db + .list_sandbox_jobs_for_user("user1") + .await + .expect("user list"); + assert!(!user_jobs.is_empty()); + + // Ownership check + let belongs = db + .sandbox_job_belongs_to_user(job_id, "user1") + .await + .expect("belongs check"); + assert!(belongs); + let not_belongs = db + .sandbox_job_belongs_to_user(job_id, "other_user") + .await + .expect("belongs check"); + assert!(!not_belongs); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_sandbox_job_mode() { + use crate::history::SandboxJobRecord; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + let job_id = uuid::Uuid::new_v4(); + let job = SandboxJobRecord { + id: job_id, + task: "Mode test".to_string(), + status: "creating".to_string(), + user_id: "user1".to_string(), + project_dir: "/workspace".to_string(), + success: None, + failure_reason: None, + created_at: chrono::Utc::now(), + started_at: None, + completed_at: None, + credential_grants_json: "[]".to_string(), + }; + db.save_sandbox_job(&job).await.expect("save"); + + // Default mode + let mode = db.get_sandbox_job_mode(job_id).await.expect("get mode"); + // Default is "worker" per schema or NULL + assert!(mode.is_none() || mode.as_deref() == Some("worker")); + + // Update mode + db.update_sandbox_job_mode(job_id, "claude_code") + .await + .expect("update mode"); + let mode = db + .get_sandbox_job_mode(job_id) + .await + .expect("get mode") + .expect("should have mode"); + assert_eq!(mode, "claude_code"); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_job_events() { + use crate::history::SandboxJobRecord; + + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + // Create a sandbox job first (foreign key) + let job_id = uuid::Uuid::new_v4(); + let job = SandboxJobRecord { + id: job_id, + task: "Event test".to_string(), + status: "running".to_string(), + user_id: "user1".to_string(), + project_dir: "/workspace".to_string(), + success: None, + failure_reason: None, + created_at: chrono::Utc::now(), + started_at: Some(chrono::Utc::now()), + completed_at: None, + credential_grants_json: "[]".to_string(), + }; + db.save_sandbox_job(&job).await.expect("save job"); + + // Save events + db.save_job_event( + job_id, + "tool_call", + &serde_json::json!({"tool": "shell", "args": {"command": "ls"}}), + ) + .await + .expect("save event 1"); + + db.save_job_event( + job_id, + "tool_result", + &serde_json::json!({"output": "file1.txt\nfile2.txt"}), + ) + .await + .expect("save event 2"); + + db.save_job_event( + job_id, + "llm_response", + &serde_json::json!({"content": "Found 2 files"}), + ) + .await + .expect("save event 3"); + + // List all events + let events = db.list_job_events(job_id, None).await.expect("list events"); + assert_eq!(events.len(), 3); + + // List with limit + let events = db + .list_job_events(job_id, Some(2)) + .await + .expect("list events limited"); + assert_eq!(events.len(), 2); + } + + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_estimation_snapshot_round_trip() { + let harness = TestHarnessBuilder::new().build().await; + let db = &harness.db; + + // Create a job first + let job_ctx = crate::context::JobContext::with_user("user1", "Estimate test", "testing"); + let job_id = job_ctx.job_id; + db.save_job(&job_ctx).await.expect("save job"); + + // Save estimation snapshot + let snap_id = db + .save_estimation_snapshot( + job_id, + "code_generation", + &["shell".to_string(), "write_file".to_string()], + Decimal::new(50, 2), // 0.50 + 120, + Decimal::new(500, 2), // 5.00 + ) + .await + .expect("save snapshot"); + assert!(!snap_id.is_nil()); + + // Update with actuals + db.update_estimation_actuals( + snap_id, + Decimal::new(45, 2), // 0.45 + 110, + Some(Decimal::new(600, 2)), // 6.00 + ) + .await + .expect("update actuals"); + } } diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index d19aacfd..0fbdd1de 100644 --- a/src/tools/builtin/http.rs +++ b/src/tools/builtin/http.rs @@ -998,4 +998,44 @@ mod tests { let params = serde_json::json!({"method": "GET"}); assert_eq!(extract_host_from_params(¶ms), 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(¶ms_no_auth); + + let params_with_cred = serde_json::json!({ + "method": "GET", + "url": "https://api.test.com/v1/models" + }); + let _ = tool.requires_approval(¶ms_with_cred); + + let params_with_auth = serde_json::json!({ + "method": "GET", + "url": "https://api.example.com", + "headers": {"Authorization": "Bearer token"} + }); + let _ = tool.requires_approval(¶ms_with_auth); + } } diff --git a/src/tools/builtin/message.rs b/src/tools/builtin/message.rs index 78592ad4..532b41e4 100644 --- a/src/tools/builtin/message.rs +++ b/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); } } diff --git a/src/tools/registry.rs b/src/tools/registry.rs index 62f1b05c..5809305e 100644 --- a/src/tools/registry.rs +++ b/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(®istry); + 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(®istry); + 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. diff --git a/tests/tool_schema_validation.rs b/tests/tool_schema_validation.rs index 8f1495cd..07218bb8 100644 --- a/tests/tool_schema_validation.rs +++ b/tests/tool_schema_validation.rs @@ -138,3 +138,29 @@ fn shell_tool_schema_is_valid() { let errors = validate_tool_schema(&schema, "shell"); assert!(errors.is_empty(), "shell tool schema errors: {errors:?}"); } + +/// Validates that all core tools work correctly under a multi-threaded tokio runtime. +/// This catches sync-async boundary bugs like tokio::sync::RwLock::blocking_read() +/// panicking when called from within a multi-threaded runtime context. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn all_core_tools_work_in_multi_thread_runtime() { + let registry = ToolRegistry::new(); + registry.register_builtin_tools(); + registry.register_dev_tools(); + + let tools = registry.all().await; + assert!( + !tools.is_empty(), + "registry should have tools after registration" + ); + + for tool in &tools { + // These sync trait methods must not panic in multi-thread runtime + let _ = tool.name(); + let _ = tool.description(); + let _ = tool.parameters_schema(); + let _ = tool.requires_approval(&serde_json::json!({})); + let _ = tool.requires_sanitization(); + let _ = tool.domain(); + } +}