From 1f8cfbce387209fb71fe31dc15a7a96e060ae48d Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Tue, 31 Mar 2026 23:33:05 +0000 Subject: [PATCH] Prevent tool regressions by locking down dispatch-level edge cases The tools crate already covered several higher-level commands, but the public dispatch surface still lacked direct tests for shell and file operations plus several error-path behaviors. This change expands the existing lib.rs unit suite to cover the requested tools through `execute_tool`, adds deterministic temp-path helpers, and hardens assertions around invalid inputs and tricky offset/background behavior. Constraint: No new dependencies; coverage had to stay within the existing crate test structure Rejected: Split coverage into new integration tests under tests/ | would require broader visibility churn for little gain Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep future tool-coverage additions on the public dispatch surface unless a lower-level helper contract specifically needs direct testing Tested: cargo fmt --all; cargo clippy -p tools --all-targets --all-features -- -D warnings; cargo test -p tools Not-tested: Cross-platform shell/runtime differences beyond the current Linux-like CI environment --- rust/crates/tools/src/lib.rs | 474 +++++++++++++++++++++++++++++++++-- 1 file changed, 453 insertions(+), 21 deletions(-) diff --git a/rust/crates/tools/src/lib.rs b/rust/crates/tools/src/lib.rs index 7f67fe5..14590ac 100644 --- a/rust/crates/tools/src/lib.rs +++ b/rust/crates/tools/src/lib.rs @@ -2349,8 +2349,10 @@ fn parse_skill_description(contents: &str) -> Option { #[cfg(test)] mod tests { + use std::fs; use std::io::{Read, Write}; use std::net::{SocketAddr, TcpListener}; + use std::path::PathBuf; use std::sync::{Arc, Mutex, OnceLock}; use std::thread; use std::time::Duration; @@ -2363,6 +2365,14 @@ mod tests { LOCK.get_or_init(|| Mutex::new(())) } + fn temp_path(name: &str) -> PathBuf { + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("time") + .as_nanos(); + std::env::temp_dir().join(format!("clawd-tools-{unique}-{name}")) + } + #[test] fn exposes_mvp_tools() { let names = mvp_tool_specs() @@ -2432,6 +2442,40 @@ mod tests { assert!(titled_summary.contains("Title: Ignored")); } + #[test] + fn web_fetch_supports_plain_text_and_rejects_invalid_url() { + let server = TestServer::spawn(Arc::new(|request_line: &str| { + assert!(request_line.starts_with("GET /plain ")); + HttpResponse::text(200, "OK", "plain text response") + })); + + let result = execute_tool( + "WebFetch", + &json!({ + "url": format!("http://{}/plain", server.addr()), + "prompt": "Show me the content" + }), + ) + .expect("WebFetch should succeed for text content"); + + let output: serde_json::Value = serde_json::from_str(&result).expect("valid json"); + assert_eq!(output["url"], format!("http://{}/plain", server.addr())); + assert!(output["result"] + .as_str() + .expect("result") + .contains("plain text response")); + + let error = execute_tool( + "WebFetch", + &json!({ + "url": "not a url", + "prompt": "Summarize" + }), + ) + .expect_err("invalid URL should fail"); + assert!(error.contains("relative URL without a base") || error.contains("invalid")); + } + #[test] fn web_search_extracts_and_filters_results() { let server = TestServer::spawn(Arc::new(|request_line: &str| { @@ -2476,15 +2520,63 @@ mod tests { assert_eq!(content[0]["url"], "https://docs.rs/reqwest"); } + #[test] + fn web_search_handles_generic_links_and_invalid_base_url() { + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let server = TestServer::spawn(Arc::new(|request_line: &str| { + assert!(request_line.contains("GET /fallback?q=generic+links ")); + HttpResponse::html( + 200, + "OK", + r#" + + Example One + Duplicate Example One + Tokio Docs + + "#, + ) + })); + + std::env::set_var( + "CLAWD_WEB_SEARCH_BASE_URL", + format!("http://{}/fallback", server.addr()), + ); + let result = execute_tool( + "WebSearch", + &json!({ + "query": "generic links" + }), + ) + .expect("WebSearch fallback parsing should succeed"); + std::env::remove_var("CLAWD_WEB_SEARCH_BASE_URL"); + + let output: serde_json::Value = serde_json::from_str(&result).expect("valid json"); + let results = output["results"].as_array().expect("results array"); + let search_result = results + .iter() + .find(|item| item.get("content").is_some()) + .expect("search result block present"); + let content = search_result["content"].as_array().expect("content array"); + assert_eq!(content.len(), 2); + assert_eq!(content[0]["url"], "https://example.com/one"); + assert_eq!(content[1]["url"], "https://docs.rs/tokio"); + + std::env::set_var("CLAWD_WEB_SEARCH_BASE_URL", "://bad-base-url"); + let error = execute_tool("WebSearch", &json!({ "query": "generic links" })) + .expect_err("invalid base URL should fail"); + std::env::remove_var("CLAWD_WEB_SEARCH_BASE_URL"); + assert!(error.contains("relative URL without a base") || error.contains("empty host")); + } + #[test] fn todo_write_persists_and_returns_previous_state() { - let path = std::env::temp_dir().join(format!( - "clawd-tools-todos-{}.json", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("time") - .as_nanos() - )); + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let path = temp_path("todos.json"); std::env::set_var("CLAWD_TODO_STORE", &path); let first = execute_tool( @@ -2526,6 +2618,59 @@ mod tests { assert!(second_output["verificationNudgeNeeded"].is_null()); } + #[test] + fn todo_write_rejects_invalid_payloads_and_sets_verification_nudge() { + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let path = temp_path("todos-errors.json"); + std::env::set_var("CLAWD_TODO_STORE", &path); + + let empty = execute_tool("TodoWrite", &json!({ "todos": [] })) + .expect_err("empty todos should fail"); + assert!(empty.contains("todos must not be empty")); + + let too_many_active = execute_tool( + "TodoWrite", + &json!({ + "todos": [ + {"content": "One", "activeForm": "Doing one", "status": "in_progress"}, + {"content": "Two", "activeForm": "Doing two", "status": "in_progress"} + ] + }), + ) + .expect_err("multiple in-progress todos should fail"); + assert!(too_many_active.contains("zero or one todo items may be in_progress")); + + let blank_content = execute_tool( + "TodoWrite", + &json!({ + "todos": [ + {"content": " ", "activeForm": "Doing it", "status": "pending"} + ] + }), + ) + .expect_err("blank content should fail"); + assert!(blank_content.contains("todo content must not be empty")); + + let nudge = execute_tool( + "TodoWrite", + &json!({ + "todos": [ + {"content": "Write tests", "activeForm": "Writing tests", "status": "completed"}, + {"content": "Fix errors", "activeForm": "Fixing errors", "status": "completed"}, + {"content": "Ship branch", "activeForm": "Shipping branch", "status": "completed"} + ] + }), + ) + .expect("completed todos should succeed"); + std::env::remove_var("CLAWD_TODO_STORE"); + let _ = fs::remove_file(path); + + let output: serde_json::Value = serde_json::from_str(&nudge).expect("valid json"); + assert_eq!(output["verificationNudgeNeeded"], true); + } + #[test] fn skill_loads_local_skill_prompt() { let result = execute_tool( @@ -2599,13 +2744,10 @@ mod tests { #[test] fn agent_persists_handoff_metadata() { - let dir = std::env::temp_dir().join(format!( - "clawd-agent-store-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("time") - .as_nanos() - )); + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let dir = temp_path("agent-store"); std::env::set_var("CLAWD_AGENT_STORE", &dir); let result = execute_tool( @@ -2661,15 +2803,32 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + #[test] + fn agent_rejects_blank_required_fields() { + let missing_description = execute_tool( + "Agent", + &json!({ + "description": " ", + "prompt": "Inspect" + }), + ) + .expect_err("blank description should fail"); + assert!(missing_description.contains("description must not be empty")); + + let missing_prompt = execute_tool( + "Agent", + &json!({ + "description": "Inspect branch", + "prompt": " " + }), + ) + .expect_err("blank prompt should fail"); + assert!(missing_prompt.contains("prompt must not be empty")); + } + #[test] fn notebook_edit_replaces_inserts_and_deletes_cells() { - let path = std::env::temp_dir().join(format!( - "clawd-notebook-{}.ipynb", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("time") - .as_nanos() - )); + let path = temp_path("notebook.ipynb"); std::fs::write( &path, r#"{ @@ -2747,6 +2906,270 @@ mod tests { let _ = std::fs::remove_file(path); } + #[test] + fn notebook_edit_rejects_invalid_inputs() { + let text_path = temp_path("notebook.txt"); + fs::write(&text_path, "not a notebook").expect("write text file"); + let wrong_extension = execute_tool( + "NotebookEdit", + &json!({ + "notebook_path": text_path.display().to_string(), + "new_source": "print(1)\n" + }), + ) + .expect_err("non-ipynb file should fail"); + assert!(wrong_extension.contains("Jupyter notebook")); + let _ = fs::remove_file(&text_path); + + let empty_notebook = temp_path("empty.ipynb"); + fs::write( + &empty_notebook, + r#"{"cells":[],"metadata":{"kernelspec":{"language":"python"}},"nbformat":4,"nbformat_minor":5}"#, + ) + .expect("write empty notebook"); + + let missing_source = execute_tool( + "NotebookEdit", + &json!({ + "notebook_path": empty_notebook.display().to_string(), + "edit_mode": "insert" + }), + ) + .expect_err("insert without source should fail"); + assert!(missing_source.contains("new_source is required")); + + let missing_cell = execute_tool( + "NotebookEdit", + &json!({ + "notebook_path": empty_notebook.display().to_string(), + "edit_mode": "delete" + }), + ) + .expect_err("delete on empty notebook should fail"); + assert!(missing_cell.contains("Notebook has no cells to edit")); + let _ = fs::remove_file(empty_notebook); + } + + #[test] + fn bash_tool_reports_success_exit_failure_timeout_and_background() { + let success = execute_tool("bash", &json!({ "command": "printf 'hello'" })) + .expect("bash should succeed"); + let success_output: serde_json::Value = serde_json::from_str(&success).expect("json"); + assert_eq!(success_output["stdout"], "hello"); + assert_eq!(success_output["interrupted"], false); + + let failure = execute_tool("bash", &json!({ "command": "printf 'oops' >&2; exit 7" })) + .expect("bash failure should still return structured output"); + let failure_output: serde_json::Value = serde_json::from_str(&failure).expect("json"); + assert_eq!(failure_output["returnCodeInterpretation"], "exit_code:7"); + assert!(failure_output["stderr"] + .as_str() + .expect("stderr") + .contains("oops")); + + let timeout = execute_tool("bash", &json!({ "command": "sleep 1", "timeout": 10 })) + .expect("bash timeout should return output"); + let timeout_output: serde_json::Value = serde_json::from_str(&timeout).expect("json"); + assert_eq!(timeout_output["interrupted"], true); + assert_eq!(timeout_output["returnCodeInterpretation"], "timeout"); + assert!(timeout_output["stderr"] + .as_str() + .expect("stderr") + .contains("Command exceeded timeout")); + + let background = execute_tool( + "bash", + &json!({ "command": "sleep 1", "run_in_background": true }), + ) + .expect("bash background should succeed"); + let background_output: serde_json::Value = serde_json::from_str(&background).expect("json"); + assert!(background_output["backgroundTaskId"].as_str().is_some()); + assert_eq!(background_output["noOutputExpected"], true); + } + + #[test] + fn file_tools_cover_read_write_and_edit_behaviors() { + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let root = temp_path("fs-suite"); + fs::create_dir_all(&root).expect("create root"); + let original_dir = std::env::current_dir().expect("cwd"); + std::env::set_current_dir(&root).expect("set cwd"); + + let write_create = execute_tool( + "write_file", + &json!({ "path": "nested/demo.txt", "content": "alpha\nbeta\nalpha\n" }), + ) + .expect("write create should succeed"); + let write_create_output: serde_json::Value = + serde_json::from_str(&write_create).expect("json"); + assert_eq!(write_create_output["type"], "create"); + assert!(root.join("nested/demo.txt").exists()); + + let write_update = execute_tool( + "write_file", + &json!({ "path": "nested/demo.txt", "content": "alpha\nbeta\ngamma\n" }), + ) + .expect("write update should succeed"); + let write_update_output: serde_json::Value = + serde_json::from_str(&write_update).expect("json"); + assert_eq!(write_update_output["type"], "update"); + assert_eq!(write_update_output["originalFile"], "alpha\nbeta\nalpha\n"); + + let read_full = execute_tool("read_file", &json!({ "path": "nested/demo.txt" })) + .expect("read full should succeed"); + let read_full_output: serde_json::Value = serde_json::from_str(&read_full).expect("json"); + assert_eq!(read_full_output["file"]["content"], "alpha\nbeta\ngamma"); + assert_eq!(read_full_output["file"]["startLine"], 1); + + let read_slice = execute_tool( + "read_file", + &json!({ "path": "nested/demo.txt", "offset": 1, "limit": 1 }), + ) + .expect("read slice should succeed"); + let read_slice_output: serde_json::Value = serde_json::from_str(&read_slice).expect("json"); + assert_eq!(read_slice_output["file"]["content"], "beta"); + assert_eq!(read_slice_output["file"]["startLine"], 2); + + let read_past_end = execute_tool( + "read_file", + &json!({ "path": "nested/demo.txt", "offset": 50 }), + ) + .expect("read past EOF should succeed"); + let read_past_end_output: serde_json::Value = + serde_json::from_str(&read_past_end).expect("json"); + assert_eq!(read_past_end_output["file"]["content"], ""); + assert_eq!(read_past_end_output["file"]["startLine"], 4); + + let read_error = execute_tool("read_file", &json!({ "path": "missing.txt" })) + .expect_err("missing file should fail"); + assert!(!read_error.is_empty()); + + let edit_once = execute_tool( + "edit_file", + &json!({ "path": "nested/demo.txt", "old_string": "alpha", "new_string": "omega" }), + ) + .expect("single edit should succeed"); + let edit_once_output: serde_json::Value = serde_json::from_str(&edit_once).expect("json"); + assert_eq!(edit_once_output["replaceAll"], false); + assert_eq!( + fs::read_to_string(root.join("nested/demo.txt")).expect("read file"), + "omega\nbeta\ngamma\n" + ); + + execute_tool( + "write_file", + &json!({ "path": "nested/demo.txt", "content": "alpha\nbeta\nalpha\n" }), + ) + .expect("reset file"); + let edit_all = execute_tool( + "edit_file", + &json!({ + "path": "nested/demo.txt", + "old_string": "alpha", + "new_string": "omega", + "replace_all": true + }), + ) + .expect("replace all should succeed"); + let edit_all_output: serde_json::Value = serde_json::from_str(&edit_all).expect("json"); + assert_eq!(edit_all_output["replaceAll"], true); + assert_eq!( + fs::read_to_string(root.join("nested/demo.txt")).expect("read file"), + "omega\nbeta\nomega\n" + ); + + let edit_same = execute_tool( + "edit_file", + &json!({ "path": "nested/demo.txt", "old_string": "omega", "new_string": "omega" }), + ) + .expect_err("identical old/new should fail"); + assert!(edit_same.contains("must differ")); + + let edit_missing = execute_tool( + "edit_file", + &json!({ "path": "nested/demo.txt", "old_string": "missing", "new_string": "omega" }), + ) + .expect_err("missing substring should fail"); + assert!(edit_missing.contains("old_string not found")); + + std::env::set_current_dir(&original_dir).expect("restore cwd"); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn glob_and_grep_tools_cover_success_and_errors() { + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let root = temp_path("search-suite"); + fs::create_dir_all(root.join("nested")).expect("create root"); + let original_dir = std::env::current_dir().expect("cwd"); + std::env::set_current_dir(&root).expect("set cwd"); + + fs::write( + root.join("nested/lib.rs"), + "fn main() {}\nlet alpha = 1;\nlet alpha = 2;\n", + ) + .expect("write rust file"); + fs::write(root.join("nested/notes.txt"), "alpha\nbeta\n").expect("write txt file"); + + let globbed = execute_tool("glob_search", &json!({ "pattern": "nested/*.rs" })) + .expect("glob should succeed"); + let globbed_output: serde_json::Value = serde_json::from_str(&globbed).expect("json"); + assert_eq!(globbed_output["numFiles"], 1); + assert!(globbed_output["filenames"][0] + .as_str() + .expect("filename") + .ends_with("nested/lib.rs")); + + let glob_error = execute_tool("glob_search", &json!({ "pattern": "[" })) + .expect_err("invalid glob should fail"); + assert!(!glob_error.is_empty()); + + let grep_content = execute_tool( + "grep_search", + &json!({ + "pattern": "alpha", + "path": "nested", + "glob": "*.rs", + "output_mode": "content", + "-n": true, + "head_limit": 1, + "offset": 1 + }), + ) + .expect("grep content should succeed"); + let grep_content_output: serde_json::Value = + serde_json::from_str(&grep_content).expect("json"); + assert_eq!(grep_content_output["numFiles"], 0); + assert!(grep_content_output["appliedLimit"].is_null()); + assert_eq!(grep_content_output["appliedOffset"], 1); + assert!(grep_content_output["content"] + .as_str() + .expect("content") + .contains("let alpha = 2;")); + + let grep_count = execute_tool( + "grep_search", + &json!({ "pattern": "alpha", "path": "nested", "output_mode": "count" }), + ) + .expect("grep count should succeed"); + let grep_count_output: serde_json::Value = serde_json::from_str(&grep_count).expect("json"); + assert_eq!(grep_count_output["numMatches"], 3); + + let grep_error = execute_tool( + "grep_search", + &json!({ "pattern": "(alpha", "path": "nested" }), + ) + .expect_err("invalid regex should fail"); + assert!(!grep_error.is_empty()); + + std::env::set_current_dir(&original_dir).expect("restore cwd"); + let _ = fs::remove_dir_all(root); + } + #[test] fn sleep_waits_and_reports_duration() { let started = std::time::Instant::now(); @@ -3038,6 +3461,15 @@ printf 'pwsh:%s' "$1" } } + fn text(status: u16, reason: &'static str, body: &str) -> Self { + Self { + status, + reason, + content_type: "text/plain; charset=utf-8", + body: body.to_string(), + } + } + fn to_bytes(&self) -> Vec { format!( "HTTP/1.1 {} {}\r\nContent-Type: {}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}",