diff --git a/src/llm-coding-tools-core/src/error.rs b/src/llm-coding-tools-core/src/error.rs
index 49468e3b..bdf351a7 100644
--- a/src/llm-coding-tools-core/src/error.rs
+++ b/src/llm-coding-tools-core/src/error.rs
@@ -59,39 +59,3 @@ impl From<globset::Error> for ToolError {
         ToolError::InvalidPattern(e.to_string())
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn tool_error_displays_io_error() {
-        let io_err = std::io::Error::new(std::io::ErrorKind::NotFound, "file not found");
-        let err: ToolError = io_err.into();
-        assert!(err.to_string().contains("I/O error"));
-    }
-
-    #[test]
-    fn tool_error_displays_invalid_path() {
-        let err = ToolError::InvalidPath("not absolute".into());
-        assert!(err.to_string().contains("invalid path"));
-    }
-
-    #[test]
-    fn tool_error_from_glob_pattern_error() {
-        let glob_err = globset::Glob::new("[invalid").unwrap_err();
-        let err: ToolError = glob_err.into();
-        assert!(matches!(err, ToolError::InvalidPattern(_)));
-    }
-
-    #[test]
-    fn timeout_with_kill_failure_displays_both_contexts() {
-        let err = ToolError::TimeoutWithKillFailure {
-            message: "command timed out after 100ms".into(),
-            kill_error: "permission denied".into(),
-        };
-        let display = err.to_string();
-        assert!(display.contains("command timed out after 100ms"));
-        assert!(display.contains("kill failed: permission denied"));
-    }
-}
diff --git a/src/llm-coding-tools-core/src/internal/hash64.rs b/src/llm-coding-tools-core/src/internal/hash64.rs
index 6c96ea78..9e353af3 100644
--- a/src/llm-coding-tools-core/src/internal/hash64.rs
+++ b/src/llm-coding-tools-core/src/internal/hash64.rs
@@ -37,21 +37,22 @@ pub(crate) fn hash_u64_bytes(bytes: &[u8]) -> Hash64 {
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    #[test]
-    fn hash_is_deterministic() {
-        let hash1 = hash_u64("bash");
-        let hash2 = hash_u64("bash");
-        assert_eq!(hash1, hash2);
-    }
-
-    #[test]
-    fn different_inputs_produce_different_hashes() {
-        let h1 = hash_u64("bash");
-        let h2 = hash_u64("read");
-        let h3 = hash_u64("write");
-        assert_ne!(h1, h2);
-        assert_ne!(h1, h3);
-        assert_ne!(h2, h3);
+    use rstest::rstest;
+
+    /// Verifies that the hash function is deterministic for identical inputs
+    /// and produces different hashes for different inputs.
+    #[rstest]
+    #[case::same_input("bash", "bash", true)]
+    #[case::different_inputs("bash", "read", false)]
+    #[case::different_inputs_2("bash", "write", false)]
+    #[case::different_inputs_3("read", "write", false)]
+    fn hash_properties(#[case] a: &str, #[case] b: &str, #[case] should_equal: bool) {
+        let hash_a = hash_u64(a);
+        let hash_b = hash_u64(b);
+        if should_equal {
+            assert_eq!(hash_a, hash_b);
+        } else {
+            assert_ne!(hash_a, hash_b);
+        }
     }
 }
diff --git a/src/llm-coding-tools-core/src/models/provider_type.rs b/src/llm-coding-tools-core/src/models/provider_type.rs
index 1b18e262..81fd9013 100644
--- a/src/llm-coding-tools-core/src/models/provider_type.rs
+++ b/src/llm-coding-tools-core/src/models/provider_type.rs
@@ -60,19 +60,24 @@ impl ProviderType {
 #[cfg(test)]
 mod tests {
     use super::ProviderType;
+    use rstest::rstest;
 
     #[test]
     fn unknown_is_default_variant() {
         assert_eq!(ProviderType::default(), ProviderType::Unknown);
     }
 
-    #[test]
-    fn azure_requires_base_url() {
-        assert!(ProviderType::Azure.requires_base_url());
-    }
-
-    #[test]
-    fn ollama_does_not_require_api_key() {
-        assert!(!ProviderType::Ollama.requires_api_key());
+    /// Verifies that provider type flags return expected values.
+    #[rstest]
+    #[case::azure_requires_base_url(ProviderType::Azure, true, true)]
+    #[case::ollama_no_api_key(ProviderType::Ollama, false, false)]
+    #[case::openai_requires_api_key(ProviderType::OpenAiCompletions, false, true)]
+    fn provider_type_flags(
+        #[case] provider: ProviderType,
+        #[case] requires_base_url: bool,
+        #[case] requires_api_key: bool,
+    ) {
+        assert_eq!(provider.requires_base_url(), requires_base_url);
+        assert_eq!(provider.requires_api_key(), requires_api_key);
     }
 }
diff --git a/src/llm-coding-tools-core/src/output.rs b/src/llm-coding-tools-core/src/output.rs
index e22ca449..616345cb 100644
--- a/src/llm-coding-tools-core/src/output.rs
+++ b/src/llm-coding-tools-core/src/output.rs
@@ -59,38 +59,39 @@ impl From<WebFetchOutput> for ToolOutput {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use rstest::rstest;
 
-    #[test]
-    fn tool_output_new_creates_non_truncated() {
-        let output = ToolOutput::new("content");
-        assert_eq!(output.content, "content");
-        assert!(!output.truncated);
-    }
-
-    #[test]
-    fn tool_output_truncated_marks_truncated() {
-        let output = ToolOutput::truncated("partial");
-        assert!(output.truncated);
-    }
-
-    #[test]
-    fn tool_output_from_string() {
-        let output: ToolOutput = "hello".into();
-        assert_eq!(output.content, "hello");
-    }
-
-    #[test]
-    fn tool_output_serializes_without_truncated_when_false() {
-        let output = ToolOutput::new("content");
-        let json = serde_json::to_string(&output).unwrap();
-        assert!(!json.contains("truncated"));
+    /// Verifies that ToolOutput constructors correctly set the truncated flag.
+    #[rstest]
+    #[case::new_creates_non_truncated(false, "content")]
+    #[case::truncated_marks_truncated(true, "partial")]
+    fn tool_output_creation(#[case] is_truncated: bool, #[case] content: &str) {
+        let output = if is_truncated {
+            ToolOutput::truncated(content)
+        } else {
+            ToolOutput::new(content)
+        };
+        assert_eq!(output.content, content);
+        assert_eq!(output.truncated, is_truncated);
     }
 
-    #[test]
-    fn tool_output_serializes_with_truncated_when_true() {
-        let output = ToolOutput::truncated("content");
+    /// Verifies that the truncated field is only serialized when true.
+    /// ToolOutput uses `#[serde(skip_serializing_if)]` to omit the field
+    /// when false, producing cleaner JSON output.
+    ///
+    /// We verify this behaviour specifically to ensure the LLM does not receive
+    /// unnecessary tokens for default values that provide no information.
+    #[rstest]
+    #[case::without_truncated_when_false(false)]
+    #[case::with_truncated_when_true(true)]
+    fn tool_output_serialization(#[case] truncated: bool) {
+        let output = if truncated {
+            ToolOutput::truncated("content")
+        } else {
+            ToolOutput::new("content")
+        };
         let json = serde_json::to_string(&output).unwrap();
-        assert!(json.contains("truncated"));
+        assert_eq!(json.contains("truncated"), truncated);
     }
 
     #[cfg(any(feature = "tokio", feature = "blocking"))]
diff --git a/src/llm-coding-tools-core/src/path/allowed.rs b/src/llm-coding-tools-core/src/path/allowed.rs
index ae2774b9..a2772d77 100644
--- a/src/llm-coding-tools-core/src/path/allowed.rs
+++ b/src/llm-coding-tools-core/src/path/allowed.rs
@@ -125,6 +125,7 @@ impl PathResolver for AllowedPathResolver {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use rstest::rstest;
     use std::fs;
     use tempfile::TempDir;
 
@@ -136,64 +137,43 @@ mod tests {
         dir
     }
 
-    #[test]
-    fn resolves_relative_path_in_allowed_dir() {
-        let dir = setup_test_dir();
-        let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
-
-        let result = resolver.resolve("file.txt");
-        assert!(result.is_ok());
-        assert!(result.unwrap().ends_with("file.txt"));
-    }
-
-    #[test]
-    fn resolves_nested_path() {
+    /// Verifies that valid paths resolve successfully, including both existing
+    /// files and new files that don't exist yet (important for write operations).
+    #[rstest]
+    #[case::existing_file_in_root("file.txt", "file.txt")] // exists: created by setup_test_dir()
+    #[case::nested_existing_file("subdir/nested.txt", "nested.txt")] // exists: created by setup_test_dir()
+    #[case::new_file_in_root("new_file.txt", "new_file.txt")] // does NOT exist: tests write path resolution
+    #[case::new_file_in_subdir("subdir/new_file.txt", "new_file.txt")] // does NOT exist: tests write path resolution
+    fn resolves_valid_paths_successfully(
+        #[case] input_path: &str,
+        #[case] expected_filename: &str,
+    ) {
         let dir = setup_test_dir();
         let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
 
-        let result = resolver.resolve("subdir/nested.txt");
-        assert!(result.is_ok());
+        let result = resolver.resolve(input_path);
+        let resolved = result.expect("path should resolve successfully");
+        assert!(
+            resolved.ends_with(expected_filename),
+            "resolved path should end with '{expected_filename}'"
+        );
     }
 
-    #[test]
-    fn rejects_path_traversal() {
+    /// Verifies that path traversal attempts are blocked regardless of
+    /// how the escape is constructed.
+    #[rstest]
+    #[case::parent_traversal("../../../etc/passwd")]
+    #[case::nested_parent_traversal("subdir/../../../new_file.txt")]
+    fn rejects_paths_that_escape_allowed_directory(#[case] input_path: &str) {
         let dir = setup_test_dir();
         let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
 
-        let result = resolver.resolve("../../../etc/passwd");
-        assert!(result.is_err());
-        assert!(result
-            .unwrap_err()
-            .to_string()
-            .contains("not within allowed"));
-    }
-
-    #[test]
-    fn allows_non_existent_path_for_write() {
-        let dir = setup_test_dir();
-        let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
-
-        let result = resolver.resolve("new_file.txt");
-        assert!(result.is_ok());
-    }
-
-    #[test]
-    fn allows_nested_non_existent_path() {
-        let dir = setup_test_dir();
-        let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
-
-        let result = resolver.resolve("subdir/new_file.txt");
-        assert!(result.is_ok());
-    }
-
-    #[test]
-    fn rejects_non_existent_path_outside_allowed() {
-        let dir = setup_test_dir();
-        let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
-
-        // Parent traversal in non-existent path
-        let result = resolver.resolve("subdir/../../../new_file.txt");
-        assert!(result.is_err());
+        let result = resolver.resolve(input_path);
+        let err = result.expect_err("path should be rejected");
+        assert!(
+            err.to_string().contains("not within allowed"),
+            "error should mention 'not within allowed'"
+        );
     }
 
     #[test]
@@ -212,14 +192,15 @@ mod tests {
     }
 
     #[test]
-    fn returns_canonical_path() {
+    fn returns_canonical_path_without_dotdots() {
         let dir = setup_test_dir();
         let resolver = AllowedPathResolver::new(vec![dir.path().to_path_buf()]).unwrap();
 
-        let result = resolver.resolve("subdir/../file.txt");
-        assert!(result.is_ok());
-        // Should resolve to the canonical path without ../
-        let resolved = result.unwrap();
-        assert!(!resolved.to_string_lossy().contains(".."));
+        // Path with ".." should be normalized
+        let resolved = resolver.resolve("subdir/../file.txt").unwrap();
+        assert!(
+            !resolved.to_string_lossy().contains(".."),
+            "canonical path should not contain '..'"
+        );
     }
 }
diff --git a/src/llm-coding-tools-core/src/permissions.rs b/src/llm-coding-tools-core/src/permissions.rs
index 0c3d8f56..90a69bcf 100644
--- a/src/llm-coding-tools-core/src/permissions.rs
+++ b/src/llm-coding-tools-core/src/permissions.rs
@@ -7,7 +7,7 @@
 //! - Evaluation is **last-match-wins**.
 //! - If nothing matches, the result is [`PermissionAction::Deny`].
 //!
-//! Matching behavior:
+//! Matching behaviour:
 //! - Permission key: exact match (case-sensitive), no wildcard expansion.
 //! - Subject pattern: wildcard matching with `*` (many chars) and `?` (one char).
 //!
@@ -296,70 +296,156 @@ fn wildcard_match_impl(input: &[u8], pattern: &[u8]) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    // ===== Wildcard matching tests =====
-
-    #[test]
-    fn wildcard_star_matches_everything() {
-        assert!(wildcard_match("anything", "*"));
-        assert!(wildcard_match("", "*"));
-        assert!(wildcard_match("a/b/c", "*"));
-    }
-
-    #[test]
-    fn wildcard_exact_match() {
-        assert!(wildcard_match("bash", "bash"));
-        assert!(!wildcard_match("bash", "task"));
-    }
-
-    #[test]
-    fn wildcard_prefix_star() {
-        assert!(wildcard_match("orchestrator-builder", "orchestrator-*"));
-        assert!(wildcard_match("orchestrator-", "orchestrator-*"));
-        assert!(!wildcard_match("other-builder", "orchestrator-*"));
-    }
-
-    #[test]
-    fn wildcard_suffix_star() {
-        assert!(wildcard_match("pre-bash", "*-bash"));
-        assert!(wildcard_match("-bash", "*-bash"));
-        assert!(!wildcard_match("bash-post", "*-bash"));
-    }
-
-    #[test]
-    fn wildcard_middle_star() {
-        assert!(wildcard_match("a-middle-z", "a-*-z"));
-        assert!(wildcard_match("a--z", "a-*-z"));
-        assert!(!wildcard_match("a-middle", "a-*-z"));
-    }
-
-    #[test]
-    fn wildcard_question_mark() {
-        assert!(wildcard_match("test", "te?t"));
-        assert!(wildcard_match("teat", "te?t"));
-        assert!(!wildcard_match("teest", "te?t"));
-        assert!(!wildcard_match("tet", "te?t"));
-    }
-
-    #[test]
-    fn wildcard_multiple_stars() {
-        assert!(wildcard_match("a/b/c", "*/*"));
-        assert!(wildcard_match("abc", "*a*c*"));
-    }
-
-    #[test]
-    fn wildcard_empty_input() {
-        assert!(wildcard_match("", "*"));
-        assert!(!wildcard_match("", "?"));
-        assert!(!wildcard_match("", "a"));
-    }
-
-    // ===== Rule tests =====
-
-    #[test]
-    fn rule_preserves_pattern_casing() {
-        let rule = Rule::new("BASH", "PATTERN", PermissionAction::Allow);
-        assert_eq!(rule.pattern(), "PATTERN");
+    use rstest::rstest;
+
+    /// Verifies that wildcard patterns match inputs correctly for various
+    /// pattern types: star (*), question mark (?), and combinations.
+    #[rstest]
+    // Star matches any string including empty
+    #[case::star_matches_anything(
+        "anything",  // input: any non-empty string
+        "*",         // pattern: single star matches everything
+        true         // matches: yes
+    )]
+    #[case::star_matches_empty(
+        "",          // input: empty string
+        "*",         // pattern: star matches empty too
+        true         // matches: yes
+    )]
+    // Exact match requires identical strings
+    #[case::exact_match(
+        "bash",      // input: exact string
+        "bash",      // pattern: identical string
+        true         // matches: yes (exact match)
+    )]
+    #[case::exact_mismatch(
+        "bash",      // input: one string
+        "task",      // pattern: different string
+        false        // matches: no (not identical)
+    )]
+    // Prefix patterns match from start
+    #[case::prefix_star(
+        "orchestrator-builder", // input: starts with prefix
+        "orchestrator-*",       // pattern: prefix + star suffix
+        true                    // matches: yes (prefix matches)
+    )]
+    #[case::prefix_star_match_empty_suffix(
+        "orchestrator-",  // input: prefix only, empty suffix
+        "orchestrator-*", // pattern: prefix + star
+        true              // matches: yes (star matches empty)
+    )]
+    #[case::prefix_star_no_match(
+        "other-builder",  // input: different prefix
+        "orchestrator-*", // pattern: specific prefix required
+        false             // matches: no (wrong prefix)
+    )]
+    // Suffix patterns match from end
+    #[case::suffix_star(
+        "pre-bash",  // input: ends with suffix
+        "*-bash",    // pattern: star prefix + suffix
+        true         // matches: yes (suffix matches)
+    )]
+    #[case::suffix_star_match_empty_prefix(
+        "-bash",     // input: suffix only, empty prefix
+        "*-bash",    // pattern: star + suffix
+        true         // matches: yes (star matches empty)
+    )]
+    #[case::suffix_star_no_match(
+        "bash-post", // input: different ending
+        "*-bash",    // pattern: specific suffix required
+        false        // matches: no (wrong suffix)
+    )]
+    // Middle star matches substring
+    #[case::middle_star(
+        "a-middle-z", // input: middle substring present
+        "a-*-z",      // pattern: prefix + star + suffix
+        true          // matches: yes (middle exists)
+    )]
+    #[case::middle_star_empty_middle(
+        "a--z",     // input: empty middle (adjacent delimiters)
+        "a-*-z",    // pattern: prefix + star + suffix
+        true        // matches: yes (star matches empty)
+    )]
+    #[case::middle_star_no_match(
+        "a-middle", // input: missing suffix
+        "a-*-z",    // pattern: requires suffix
+        false       // matches: no (no suffix)
+    )]
+    // Question mark matches exactly one character
+    #[case::question_mark(
+        "test", // input: one char at position 3
+        "te?t", // pattern: te + ? + t
+        true    // matches: yes (s matches ?)
+    )]
+    #[case::question_mark_another(
+        "teat", // input: different char at position 3
+        "te?t", // pattern: te + ? + t
+        true    // matches: yes (a matches ?)
+    )]
+    #[case::question_mark_too_many(
+        "teest", // input: two chars at position 3
+        "te?t",  // pattern: exactly one char required
+        false    // matches: no (too many chars)
+    )]
+    #[case::question_mark_too_few(
+        "tet",  // input: zero chars at position 3
+        "te?t", // pattern: exactly one char required
+        false   // matches: no (too few chars)
+    )]
+    // Multiple stars work together
+    #[case::multiple_stars_path(
+        "a/b/c", // input: path with slash
+        "*/*",   // pattern: two stars match components
+        true     // matches: yes
+    )]
+    #[case::multiple_stars_complex(
+        "abc",   // input: surrounded chars
+        "*a*c*", // pattern: stars on both sides
+        true     // matches: yes
+    )]
+    // Empty input edge cases
+    #[case::empty_input_with_star(
+        "",    // input: empty string
+        "*",   // pattern: star matches empty
+        true   // matches: yes
+    )]
+    #[case::empty_input_with_question(
+        "",    // input: empty string
+        "?",   // pattern: requires one char
+        false  // matches: no (empty has zero)
+    )]
+    #[case::empty_input_with_literal(
+        "",    // input: empty string
+        "a",   // pattern: requires literal "a"
+        false  // matches: no (not equal)
+    )]
+    fn wildcard_pattern_matching(
+        #[case] input: &str,
+        #[case] pattern: &str,
+        #[case] should_match: bool,
+    ) {
+        assert_eq!(wildcard_match(input, pattern), should_match);
+    }
+
+    /// Verifies that Rule preserves casing and computes correct hashes.
+    #[rstest]
+    #[case::pattern_casing(
+        "BASH",      // permission: uppercase permission key
+        "PATTERN",   // pattern: uppercase pattern string
+        "PATTERN"    // expected: pattern unchanged (preserves casing)
+    )]
+    #[case::lowercase_pattern(
+        "bash",      // permission: lowercase permission key
+        "pattern",   // pattern: lowercase pattern string
+        "pattern"    // expected: pattern unchanged (preserves casing)
+    )]
+    fn rule_preserves_casing(
+        #[case] permission: &str,
+        #[case] pattern: &str,
+        #[case] expected: &str,
+    ) {
+        let rule = Rule::new(permission, pattern, PermissionAction::Allow);
+        assert_eq!(rule.pattern(), expected);
     }
 
     #[test]
@@ -381,99 +467,144 @@ mod tests {
         assert_eq!(std::mem::size_of::<Rule>(), 32);
     }
 
-    // ===== Ruleset tests =====
-
-    #[test]
-    fn ruleset_evaluate_default_deny() {
-        let ruleset = Ruleset::new();
-        assert_eq!(ruleset.evaluate("bash", "anything"), PermissionAction::Deny);
-    }
-
-    #[test]
-    fn ruleset_evaluate_simple_allow() {
-        let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("bash", "*", PermissionAction::Allow));
-
-        assert_eq!(
-            ruleset.evaluate("bash", "anything"),
-            PermissionAction::Allow
-        );
-        assert_eq!(ruleset.evaluate("task", "anything"), PermissionAction::Deny);
-    }
-
-    #[test]
-    fn ruleset_evaluate_last_match_wins() {
+    /// Verifies that Ruleset evaluates rules correctly, including default deny
+    /// and last-match-wins behaviour.
+    #[rstest]
+    // Default is deny when no rules match
+    #[case::default_deny(
+        vec![],                // rules: empty ruleset
+        ("bash", "anything"),  // evaluate: permission="bash", subject="anything"
+        PermissionAction::Deny // expected: default deny (no matching rules)
+    )]
+    // Simple allow rule permits matching permission
+    #[case::simple_allow(
+        vec![("bash", "*", PermissionAction::Allow)], // rules: allow all for "bash" permission
+        ("bash", "anything"),                         // evaluate: matches permission="bash"
+        PermissionAction::Allow                       // expected: allow (rule matches)
+    )]
+    // Non-matching permission is still denied
+    #[case::simple_allow_nonmatch(
+        vec![("bash", "*", PermissionAction::Allow)], // rules: allow all for "bash" only
+        ("task", "anything"),                         // evaluate: different permission="task"
+        PermissionAction::Deny                        // expected: deny (no matching rules)
+    )]
+    // Last match wins when multiple rules apply
+    #[case::last_match_wins_allow(
+        vec![
+            ("task", "*", PermissionAction::Deny),              // rules[0]: deny all task
+            ("task", "orchestrator-*", PermissionAction::Allow) // rules[1]: allow orchestrator-*
+        ],
+        ("task", "orchestrator-builder"),                       // evaluate: matches both rules
+        PermissionAction::Allow                                 // expected: allow (rules[1] wins)
+    )]
+    #[case::last_match_wins_deny(
+        vec![
+            ("task", "orchestrator-*", PermissionAction::Allow),  // rules[0]: allow orchestrator-*
+            ("task", "*", PermissionAction::Deny)                 // rules[1]: deny all task
+        ],
+        ("task", "orchestrator-builder"),                         // evaluate: matches both rules
+        PermissionAction::Deny                                    // expected: deny (rules[1] wins)
+    )]
+    // Only first rule applies when second doesn't match
+    #[case::partial_match_first_rule(
+        vec![
+            ("task", "*", PermissionAction::Deny),              // rules[0]: deny all task (matches)
+            ("task", "orchestrator-*", PermissionAction::Allow) // rules[1]: allow orchestrator-* (doesn't match)
+        ],
+        ("task", "random-agent"),                              // evaluate: only matches rules[0]
+        PermissionAction::Deny                                 // expected: deny (only first rule matches)
+    )]
+    fn ruleset_evaluate(
+        #[case] rules: Vec<(&str, &str, PermissionAction)>,
+        #[case] (permission, target): (&str, &str),
+        #[case] expected: PermissionAction,
+    ) {
         let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("task", "*", PermissionAction::Deny));
-        ruleset.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow));
-
-        // "orchestrator-builder" matches both rules, but last one wins
-        assert_eq!(
-            ruleset.evaluate("task", "orchestrator-builder"),
-            PermissionAction::Allow
-        );
-        // "random-agent" only matches first rule
-        assert_eq!(
-            ruleset.evaluate("task", "random-agent"),
-            PermissionAction::Deny
-        );
-    }
-
-    #[test]
-    fn ruleset_evaluate_case_sensitive() {
-        let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("BASH", "*", PermissionAction::Allow));
-
-        assert_eq!(ruleset.evaluate("BASH", "test"), PermissionAction::Allow);
-        assert_eq!(ruleset.evaluate("bash", "test"), PermissionAction::Deny);
-        assert_eq!(ruleset.evaluate("Bash", "test"), PermissionAction::Deny);
-    }
-
-    #[test]
-    fn ruleset_evaluate_pattern_case_sensitive() {
-        let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("task", "AGENT-*", PermissionAction::Allow));
-
-        assert_eq!(
-            ruleset.evaluate("task", "AGENT-foo"),
-            PermissionAction::Allow
-        );
-        assert_eq!(
-            ruleset.evaluate("task", "agent-foo"),
-            PermissionAction::Deny
-        );
-        assert_eq!(
-            ruleset.evaluate("task", "Agent-Bar"),
-            PermissionAction::Deny
-        );
-    }
-
-    #[test]
-    fn ruleset_evaluate_permission_exact_match_only() {
-        // Wildcards in permission key should NOT match multiple tools
+        for (perm, pat, action) in rules {
+            ruleset.push(Rule::new(perm, pat, action));
+        }
+        assert_eq!(ruleset.evaluate(permission, target), expected);
+    }
+
+    /// Verifies that permission keys and patterns are case-sensitive.
+    #[rstest]
+    #[case::permission_uppercase(
+        vec![("BASH", "*", PermissionAction::Allow)], // rules: allow all for "BASH" permission
+        ("BASH", "test"),                             // evaluate: exact case match
+        PermissionAction::Allow                       // expected: allow (exact case match)
+    )]
+    #[case::permission_lowercase_no_match(
+        vec![("BASH", "*", PermissionAction::Allow)], // rules: allow all for "BASH" only
+        ("bash", "test"),                             // evaluate: different case "bash"
+        PermissionAction::Deny                        // expected: deny (case mismatch)
+    )]
+    #[case::pattern_uppercase(
+        vec![("task", "AGENT-*", PermissionAction::Allow)], // rules: allow AGENT-* pattern
+        ("task", "AGENT-foo"),                              // evaluate: exact case match
+        PermissionAction::Allow                             // expected: allow (exact case match)
+    )]
+    #[case::pattern_lowercase_no_match(
+        vec![("task", "AGENT-*", PermissionAction::Allow)], // rules: allow AGENT-* only
+        ("task", "agent-foo"),                              // evaluate: different case "agent-foo"
+        PermissionAction::Deny                              // expected: deny (case mismatch)
+    )]
+    fn ruleset_evaluate_case_sensitive(
+        #[case] rules: Vec<(&str, &str, PermissionAction)>,
+        #[case] (permission, target): (&str, &str),
+        #[case] expected: PermissionAction,
+    ) {
         let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("*", "*", PermissionAction::Allow));
-
-        // A rule with permission "*" only matches permission key "*", not "bash"
-        assert_eq!(ruleset.evaluate("bash", "anything"), PermissionAction::Deny);
-        assert_eq!(ruleset.evaluate("*", "anything"), PermissionAction::Allow);
+        for (perm, pat, action) in rules {
+            ruleset.push(Rule::new(perm, pat, action));
+        }
+        assert_eq!(ruleset.evaluate(permission, target), expected);
     }
 
-    #[test]
-    fn ruleset_evaluate_permission_no_wildcard_expansion() {
-        // Rule with "bash*" permission should NOT match "bash-extended"
+    /// Verifies that wildcards in the first parameter of Rule::new() (permission)
+    /// do NOT match other keys.
+    ///
+    /// The permission field requires exact match only - "*" is treated as a literal
+    /// string, not a wildcard. So a rule with permission="*" only matches when the
+    /// evaluated permission is exactly "*", not "bash" or any other value.
+    ///
+    /// This differs from the second parameter (pattern/target) which does support
+    /// wildcards.
+    #[rstest]
+    #[case::star_permission_not_bash(
+        vec![("*", "*", PermissionAction::Allow)], // rules: allow all for permission="*" only
+        ("bash", "anything"),                      // evaluate: different permission="bash"
+        PermissionAction::Deny                     // expected: deny (no match, must be exact)
+    )]
+    #[case::star_permission_matches_star(
+        vec![("*", "*", PermissionAction::Allow)], // rules: allow all for permission="*"
+        ("*", "anything"),                         // evaluate: exact match on permission="*"
+        PermissionAction::Allow                    // expected: allow (exact permission match)
+    )]
+    #[case::bash_star_permission_not_bash(
+        vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*"
+        ("bash", "anything"),                          // evaluate: different permission="bash"
+        PermissionAction::Deny                         // expected: deny (must be exact)
+    )]
+    #[case::bash_star_permission_not_bash_extended(
+        vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*"
+        ("bash-extended", "anything"),                 // evaluate: different permission="bash-extended"
+        PermissionAction::Deny                         // expected: deny (must be exact)
+    )]
+    #[case::bash_star_permission_matches_exact(
+        vec![("bash*", "*", PermissionAction::Allow)], // rules: allow all for permission="bash*"
+        ("bash*", "anything"),                         // evaluate: exact match on permission="bash*"
+        PermissionAction::Allow                        // expected: allow (exact permission match)
+    )]
+    fn ruleset_evaluate_permission_exact_match_only(
+        #[case] rules: Vec<(&str, &str, PermissionAction)>,
+        #[case] (permission, target): (&str, &str),
+        #[case] expected: PermissionAction,
+    ) {
         let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("bash*", "*", PermissionAction::Allow));
-
-        assert_eq!(ruleset.evaluate("bash", "anything"), PermissionAction::Deny);
-        assert_eq!(
-            ruleset.evaluate("bash-extended", "anything"),
-            PermissionAction::Deny
-        );
-        assert_eq!(
-            ruleset.evaluate("bash*", "anything"),
-            PermissionAction::Allow
-        );
+        for (perm, pat, action) in rules {
+            ruleset.push(Rule::new(perm, pat, action));
+        }
+        assert_eq!(ruleset.evaluate(permission, target), expected);
     }
 
     #[test]
@@ -510,26 +641,4 @@ mod tests {
         let combined = Ruleset::merged([&r1, &r2]);
         assert_eq!(combined.evaluate("a", "x"), PermissionAction::Allow);
     }
-
-    #[test]
-    fn ruleset_precedence_specific_overrides_wildcard_when_specific_is_last() {
-        let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("task", "*", PermissionAction::Deny));
-        ruleset.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow));
-        assert_eq!(
-            ruleset.evaluate("task", "orchestrator-review"),
-            PermissionAction::Allow
-        );
-    }
-
-    #[test]
-    fn ruleset_precedence_wildcard_overrides_specific_when_wildcard_is_last() {
-        let mut ruleset = Ruleset::new();
-        ruleset.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow));
-        ruleset.push(Rule::new("task", "*", PermissionAction::Deny));
-        assert_eq!(
-            ruleset.evaluate("task", "orchestrator-review"),
-            PermissionAction::Deny
-        );
-    }
 }
diff --git a/src/llm-coding-tools-core/src/tools/glob.rs b/src/llm-coding-tools-core/src/tools/glob.rs
index aa8ad8da..76a09f85 100644
--- a/src/llm-coding-tools-core/src/tools/glob.rs
+++ b/src/llm-coding-tools-core/src/tools/glob.rs
@@ -130,6 +130,7 @@ pub fn glob_files<R: PathResolver>(
 mod tests {
     use super::*;
     use crate::path::AbsolutePathResolver;
+    use rstest::rstest;
     use std::fs::{self, File, FileTimes};
     use std::io::Write;
     use std::time::{Duration, SystemTime};
@@ -149,20 +150,80 @@ mod tests {
         dir
     }
 
-    #[test]
-    fn glob_matches_pattern() {
+    /// Verifies that glob patterns correctly include or exclude files based on
+    /// both pattern matching and gitignore rules.
+    #[rstest]
+    #[case::matches_rs_extension("**/*.rs", "lib.rs", true)]
+    #[case::excludes_gitignored_target("**/*", "target", false)]
+    fn glob_pattern_includes_or_excludes_files(
+        #[case] pattern: &str,
+        #[case] needle: &str,
+        #[case] should_find: bool,
+    ) {
         let dir = create_test_tree();
         let resolver = AbsolutePathResolver;
-        let result = glob_files(&resolver, "**/*.rs", dir.path().to_str().unwrap(), 1000).unwrap();
-        assert!(result.files.iter().any(|f| f.ends_with("lib.rs")));
+
+        let result = glob_files(&resolver, pattern, dir.path().to_str().unwrap(), 1000).unwrap();
+
+        let found = result.files.iter().any(|f| f.contains(needle));
+
+        assert_eq!(
+            found, should_find,
+            "pattern={pattern}, needle={needle}, files={:?}",
+            result.files
+        );
     }
 
-    #[test]
-    fn glob_respects_gitignore() {
-        let dir = create_test_tree();
-        let resolver = AbsolutePathResolver;
-        let result = glob_files(&resolver, "**/*", dir.path().to_str().unwrap(), 1000).unwrap();
-        assert!(!result.files.iter().any(|f| f.contains("target")));
+    /// Verifies that optional JSON fields are only serialized when they contain
+    /// meaningful data. GlobOutput uses `#[serde(skip_serializing_if)]` to omit
+    /// `partial` when false and `errors` when empty, producing cleaner JSON output.
+    ///
+    /// Test matrix:
+    /// - Case 1: partial=true, has errors → both fields appear in JSON
+    /// - Case 2: partial=false, no errors → neither field appears in JSON
+    ///
+    /// We verify this behaviour specifically to ensure the LLM does not receive
+    /// unnecessary tokens for default values that provide no information.
+    #[rstest]
+    #[case::partial_with_errors(true, vec!["walk error: permission denied"])]
+    #[case::clean_results_no_optional_fields(false, vec![])]
+    fn glob_output_serialization_omits_default_fields(
+        #[case] partial: bool,     // Whether walk encountered errors
+        #[case] errors: Vec<&str>, // Error messages from walk
+    ) {
+        // Save error count before consuming the vec for assertions later
+        let error_count = errors.len();
+
+        // Compute expected field presence from input values
+        let expected_partial_present = partial;
+        let expected_errors_present = !errors.is_empty();
+
+        let output = GlobOutput {
+            files: vec!["src/lib.rs".to_string()],
+            truncated: false,
+            partial,
+            errors: errors.into_iter().map(String::from).collect(),
+        };
+
+        let json = serde_json::to_value(&output).unwrap();
+
+        // Verify presence/absence of optional fields based on their values
+        assert_eq!(
+            json.get("partial").is_some(),
+            expected_partial_present,
+            "partial field presence mismatch in JSON: {json}"
+        );
+        assert_eq!(
+            json.get("errors").is_some(),
+            expected_errors_present,
+            "errors field presence mismatch in JSON: {json}"
+        );
+
+        // When errors are present, verify they serialized correctly
+        if expected_errors_present {
+            let error_values = json.get("errors").unwrap().as_array().unwrap();
+            assert_eq!(error_values.len(), error_count);
+        }
     }
 
     #[test]
@@ -198,6 +259,7 @@ mod tests {
             .position(|path| path.ends_with("older.txt"))
             .unwrap();
 
+        // Newer files should appear before older ones in the results
         assert!(
             newer_index < older_index,
             "expected newer file before older: {:?}",
@@ -212,44 +274,14 @@ mod tests {
         let resolver = AbsolutePathResolver;
         let result = glob_files(&resolver, "**/*.rs", dir.path().to_str().unwrap(), 1000).unwrap();
 
-        // Verify matching works with forward-slash patterns
+        // The only .rs file in our test tree is src/lib.rs
         assert_eq!(result.files.len(), 1);
         assert!(result.files[0].ends_with("lib.rs"));
 
-        // Verify returned paths use forward slashes (critical for Windows)
+        // Returned paths must always use forward slashes (important on Windows)
         for path in &result.files {
             assert!(!path.contains('\\'), "expected forward slashes: {path}");
         }
         assert!(result.files.iter().any(|f| f.contains('/')));
     }
-
-    #[test]
-    fn glob_output_serializes_partial_metadata() {
-        let output = GlobOutput {
-            files: vec!["src/lib.rs".to_string()],
-            truncated: false,
-            partial: true,
-            errors: vec!["walk error: denied".to_string()],
-        };
-
-        let json = serde_json::to_value(&output).unwrap();
-
-        assert_eq!(json["partial"], true);
-        assert_eq!(json["errors"][0], "walk error: denied");
-    }
-
-    #[test]
-    fn glob_output_omits_partial_metadata_when_not_partial() {
-        let output = GlobOutput {
-            files: vec!["src/lib.rs".to_string()],
-            truncated: false,
-            partial: false,
-            errors: Vec::new(),
-        };
-
-        let json = serde_json::to_value(&output).unwrap();
-
-        assert!(json.get("partial").is_none());
-        assert!(json.get("errors").is_none());
-    }
 }
diff --git a/src/llm-coding-tools-core/src/tools/grep.rs b/src/llm-coding-tools-core/src/tools/grep.rs
index f6006b68..a080e22b 100644
--- a/src/llm-coding-tools-core/src/tools/grep.rs
+++ b/src/llm-coding-tools-core/src/tools/grep.rs
@@ -264,90 +264,178 @@ mod tests {
     use rstest::rstest;
     use tempfile::tempdir;
 
-    #[test]
-    fn grep_finds_matches() {
-        let temp = tempdir().unwrap();
-        std::fs::write(temp.path().join("match.txt"), "hello world").unwrap();
-        let resolver = AbsolutePathResolver;
-
-        let result =
-            grep_search(&resolver, "hello", None, temp.path().to_str().unwrap(), 10).unwrap();
-
-        assert_eq!(result.files.len(), 1);
-        assert_eq!(result.match_count, 1);
-    }
-
-    #[test]
-    fn grep_respects_glob_filter() {
+    /// Verifies that grep search returns the expected number of files and matches
+    /// for different file layouts and optional glob filters.
+    #[rstest]
+    #[case::single_file_no_filter(
+        vec![("match.txt", "hello world")], // files: 1 file with 1 match
+        "hello",                            // pattern: matches 1 place
+        None::<&str>,                       // filter: none (search all)
+        1,                                  // expected: 1 file matched
+        1                                   // expected: 1 total match
+    )]
+    #[case::glob_filters_to_rs_only(
+        vec![("match.rs", "hello"), ("match.txt", "hello")], // files: 2 files, same pattern
+        "hello",                                             // pattern: matches 1 place in each
+        Some("*.rs"),                                        // filter: only .rs files
+        1,                                                   // expected: 1 file matched (.rs only)
+        1                                                    // expected: 1 total match
+    )]
+    #[case::no_matches_found(
+        vec![("notes.txt", "goodbye")], // files: 1 file with no match
+        "hello",                        // pattern: matches 0 places
+        None::<&str>,                   // filter: none (search all)
+        0,                              // expected: 0 files matched
+        0                               // expected: 0 total matches
+    )]
+    #[case::multiple_files_multiple_matches(
+        vec![("a.txt", "hello\nworld"), ("b.txt", "hello\nhello")], // files: 2 files
+        "hello",                                                    // pattern: matches 1+2 places
+        None::<&str>,                                               // filter: none (search all)
+        2,                                                          // expected: 2 files matched
+        3                                                           // expected: 3 total matches
+    )]
+    #[case::glob_excludes_everything(
+        vec![("match.rs", "hello")], // files: 1 file that would match
+        "hello",                     // pattern: matches 0 places (file excluded)
+        Some("*.py"),                // filter: only .py files (excludes .rs)
+        0,                           // expected: 0 files matched (all excluded)
+        0                            // expected: 0 total matches
+    )]
+    fn grep_search_finds_expected_matches(
+        #[case] files: Vec<(&str, &str)>,
+        #[case] pattern: &str,
+        #[case] include: Option<&str>,
+        #[case] expected_file_count: usize,
+        #[case] expected_match_count: usize,
+    ) {
         let temp = tempdir().unwrap();
-        std::fs::write(temp.path().join("match.rs"), "hello").unwrap();
-        std::fs::write(temp.path().join("match.txt"), "hello").unwrap();
+        for (name, content) in files {
+            std::fs::write(temp.path().join(name), content).unwrap();
+        }
         let resolver = AbsolutePathResolver;
 
         let result = grep_search(
             &resolver,
-            "hello",
-            Some("*.rs"),
+            pattern,
+            include,
             temp.path().to_str().unwrap(),
             10,
         )
         .unwrap();
 
-        assert_eq!(result.files.len(), 1);
-        assert!(result.files[0].path.ends_with(".rs"));
+        assert_eq!(result.files.len(), expected_file_count);
+        assert_eq!(result.match_count, expected_match_count);
+
+        // Verify glob filtering works correctly
+        if let Some(glob) = include {
+            for file in &result.files {
+                assert!(file
+                    .path
+                    .ends_with(glob.trim_start_matches('*').trim_start_matches('.')));
+            }
+        }
     }
 
-    #[test]
-    fn grep_format_includes_partial_marker() {
+    /// Verifies that format output displays correct status markers for different
+    /// combinations of partial results and truncation flags.
+    #[rstest]
+    #[case::partial_only(true, false, true, false)]
+    #[case::truncated_only(false, true, false, true)]
+    #[case::both_flags(true, true, true, true)]
+    #[case::neither_flag(false, false, false, false)]
+    fn grep_format_displays_status_markers(
+        #[case] partial: bool,
+        #[case] truncated: bool,
+        #[case] expect_partial_msg: bool,
+        #[case] expect_truncated_msg: bool,
+    ) {
+        let errors = if partial {
+            vec!["walk error: denied".to_string()]
+        } else {
+            Vec::new()
+        };
+
         let output = GrepOutput {
             files: Vec::new(),
             match_count: 0,
-            truncated: false,
-            partial: true,
-            errors: vec!["walk error: denied".to_string()],
+            truncated,
+            partial,
+            errors,
         };
 
         let formatted = output.format::<true>(10, DEFAULT_MAX_LINE_LENGTH);
 
-        assert!(formatted.contains("Partial results"));
+        assert_eq!(formatted.contains("Partial results"), expect_partial_msg);
+        assert_eq!(
+            formatted.contains("Results truncated"),
+            expect_truncated_msg
+        );
     }
 
-    #[test]
-    fn collect_file_matches_reports_error_for_missing_file() {
+    /// Verifies how collect_file_matches handles various file edge cases like
+    /// missing files and binary content.
+    #[rstest]
+    #[case::missing_file(
+        "missing.txt", // file: does not exist on disk
+        true,          // expect_error: search fails (file not found)
+        0              // expected_match_count: 0 matches (search never ran)
+    )]
+    #[case::binary_file(
+        "binary.bin",  // file: binary with null bytes
+        false,         // expect_error: no error (binary skipped gracefully)
+        0              // expected_match_count: 0 matches (binary not searched)
+    )]
+    fn collect_file_matches_handles_edge_cases(
+        #[case] file_name: &str,
+        #[case] expect_error: bool,
+        #[case] expected_match_count: usize,
+    ) {
         let temp = tempdir().unwrap();
-        let missing = temp.path().join("missing.txt");
+        let target_path = temp.path().join(file_name);
+
+        // Create binary file if testing binary detection
+        if file_name == "binary.bin" {
+            std::fs::write(&target_path, b"hello\x00world").unwrap();
+        }
+
         let matcher = RegexMatcher::new("hello").unwrap();
         let mut searcher = SearcherBuilder::new()
             .binary_detection(BinaryDetection::quit(0))
             .build();
 
-        let result = collect_file_matches(&matcher, &mut searcher, &missing);
-
-        assert!(result.matches.is_empty());
-        assert!(result.error.is_some());
-    }
-
-    #[test]
-    fn grep_marks_results_partial_when_walker_reports_error() {
-        let temp = tempdir().unwrap();
-        let missing_root = temp.path().join("missing-root");
-        let resolver = AbsolutePathResolver;
-
-        let result =
-            grep_search(&resolver, "hello", None, missing_root.to_str().unwrap(), 10).unwrap();
+        let result = collect_file_matches(&matcher, &mut searcher, &target_path);
 
-        assert!(result.partial);
-        assert_eq!(result.match_count, 0);
-        assert!(!result.truncated);
-        assert!(!result.errors.is_empty());
+        assert_eq!(result.matches.len(), expected_match_count);
+        assert_eq!(result.error.is_some(), expect_error);
     }
 
+    /// Verifies that line truncation in formatted output behaves correctly for
+    /// different line lengths and line number settings.
     #[rstest]
-    #[case(true, 6, "L1: abc...")] // With line numbers, truncates to "abc..."
-    #[case(false, 4, "  a...")] // Without line numbers, at min limit "  a..."
-    fn grep_format_truncates_lines_with_ellipsis(
-        #[case] with_line_numbers: bool,
+    #[case::with_line_numbers_short(
+        6,           // max_len: line "abcdefghij" (10 chars) truncated to 6
+        true,        // with_line_numbers: yes, shows "L1: " prefix
+        "L1: abc..." // expected: truncated with line number prefix
+    )]
+    #[case::without_line_numbers_short(
+        4,        // max_len: line truncated to 4 chars
+        false,    // with_line_numbers: no prefix
+        "  a..."  // expected: truncated without line number prefix
+    )]
+    #[case::no_truncation_when_fits(
+        200,             // max_len: larger than line length (10 chars)
+        true,            // with_line_numbers: yes
+        "L1: abcdefghij" // expected: full line preserved, no truncation
+    )]
+    #[case::exact_boundary_no_truncation(
+        10,              // max_len: exactly matches line length (10 chars)
+        true,            // with_line_numbers: yes
+        "L1: abcdefghij" // expected: full line preserved, boundary not exceeded
+    )]
+    fn grep_format_handles_line_truncation(
         #[case] max_len: usize,
+        #[case] with_line_numbers: bool,
         #[case] expected: &str,
     ) {
         let output = GrepOutput {
@@ -370,6 +458,7 @@ mod tests {
         } else {
             output.format::<false>(10, max_len)
         };
+
         assert!(
             formatted.contains(expected),
             "Expected '{}' in:\n{}",
@@ -377,4 +466,20 @@ mod tests {
             formatted
         );
     }
+
+    #[test]
+    fn grep_marks_results_partial_when_walker_reports_error() {
+        let temp = tempdir().unwrap();
+        let missing_root = temp.path().join("missing-root");
+        let resolver = AbsolutePathResolver;
+
+        let result =
+            grep_search(&resolver, "hello", None, missing_root.to_str().unwrap(), 10).unwrap();
+
+        // Walker errors should mark results as partial but not truncated
+        assert!(result.partial);
+        assert!(!result.truncated);
+        assert!(!result.errors.is_empty());
+        assert_eq!(result.match_count, 0);
+    }
 }
diff --git a/src/llm-coding-tools-core/src/tools/todo.rs b/src/llm-coding-tools-core/src/tools/todo.rs
index ec929ec7..2b6cedc6 100644
--- a/src/llm-coding-tools-core/src/tools/todo.rs
+++ b/src/llm-coding-tools-core/src/tools/todo.rs
@@ -118,6 +118,7 @@ pub fn read_todos(state: &TodoState) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use rstest::rstest;
 
     fn make_todo(id: &str, status: TodoStatus) -> Todo {
         Todo {
@@ -128,6 +129,36 @@ mod tests {
         }
     }
 
+    /// Verifies that write_todos rejects todos with empty id or content.
+    #[rstest]
+    #[case::empty_id("", "Task", "id")]
+    #[case::empty_content("1", "  ", "content")]
+    fn write_validates_required_fields(
+        #[case] id: &str,
+        #[case] content: &str,
+        #[case] _field_name: &str,
+    ) {
+        let state = TodoState::new();
+        let todo = Todo {
+            id: id.to_string(),
+            content: content.to_string(),
+            status: TodoStatus::Pending,
+            priority: TodoPriority::Low,
+        };
+        let result = write_todos(&state, vec![todo]);
+        assert!(matches!(result, Err(ToolError::Validation(_))));
+    }
+
+    /// Verifies that each status variant returns the correct icon string.
+    #[rstest]
+    #[case::pending(TodoStatus::Pending, "[ ]")]
+    #[case::in_progress(TodoStatus::InProgress, "[>]")]
+    #[case::completed(TodoStatus::Completed, "[x]")]
+    #[case::cancelled(TodoStatus::Cancelled, "[-]")]
+    fn status_icons(#[case] status: TodoStatus, #[case] expected: &str) {
+        assert_eq!(status.icon(), expected);
+    }
+
     #[test]
     fn write_and_read_todos() {
         let state = TodoState::new();
@@ -166,40 +197,6 @@ mod tests {
         assert!(output.contains("Task b"));
     }
 
-    #[test]
-    fn write_validates_empty_id() {
-        let state = TodoState::new();
-        let todo = Todo {
-            id: "".to_string(),
-            content: "Task".to_string(),
-            status: TodoStatus::Pending,
-            priority: TodoPriority::Low,
-        };
-        let result = write_todos(&state, vec![todo]);
-        assert!(matches!(result, Err(ToolError::Validation(_))));
-    }
-
-    #[test]
-    fn write_validates_empty_content() {
-        let state = TodoState::new();
-        let todo = Todo {
-            id: "1".to_string(),
-            content: "  ".to_string(),
-            status: TodoStatus::Pending,
-            priority: TodoPriority::Low,
-        };
-        let result = write_todos(&state, vec![todo]);
-        assert!(matches!(result, Err(ToolError::Validation(_))));
-    }
-
-    #[test]
-    fn status_icons_are_correct() {
-        assert_eq!(TodoStatus::Pending.icon(), "[ ]");
-        assert_eq!(TodoStatus::InProgress.icon(), "[>]");
-        assert_eq!(TodoStatus::Completed.icon(), "[x]");
-        assert_eq!(TodoStatus::Cancelled.icon(), "[-]");
-    }
-
     #[test]
     fn status_serde_roundtrip() {
         let json = serde_json::to_string(&TodoStatus::InProgress).unwrap();
diff --git a/src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs b/src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs
index 8f210989..a6ebe031 100644
--- a/src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs
+++ b/src/llm-coding-tools-core/src/tools/webfetch/blocking_impl.rs
@@ -115,6 +115,7 @@ pub fn fetch_url(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use rstest::rstest;
 
     fn test_client() -> reqwest::blocking::Client {
         reqwest::blocking::Client::builder()
@@ -122,6 +123,25 @@ mod tests {
             .expect("client build failed")
     }
 
+    /// Verifies that invalid timeout values are rejected before making any
+    /// network request. The URL is intentionally unreachable.
+    #[rstest]
+    #[case::zero_timeout(0, 10_000)]
+    #[case::exceeds_max(11_000, 10_000)]
+    fn rejects_invalid_timeout(#[case] timeout_ms: u32, #[case] max_timeout_ms: u32) {
+        let client = test_client();
+
+        let result = fetch_url(
+            &client,
+            "http://localhost:1",
+            timeout_ms,
+            max_timeout_ms,
+            5 * 1024 * 1024,
+        );
+
+        assert!(matches!(result, Err(ToolError::Validation(_))));
+    }
+
     #[test]
     fn fetches_plain_text() {
         // Use httpbin.org for blocking tests since wiremock is async-only
@@ -158,24 +178,4 @@ mod tests {
             assert!(matches!(e, ToolError::Http(_)));
         }
     }
-
-    #[test]
-    fn rejects_timeout_zero() {
-        let client = test_client();
-        let result = fetch_url(&client, "http://localhost:1", 0, 10_000, 5 * 1024 * 1024);
-        assert!(matches!(result, Err(ToolError::Validation(_))));
-    }
-
-    #[test]
-    fn rejects_timeout_exceeding_max() {
-        let client = test_client();
-        let result = fetch_url(
-            &client,
-            "http://localhost:1",
-            11_000,
-            10_000,
-            5 * 1024 * 1024,
-        );
-        assert!(matches!(result, Err(ToolError::Validation(_))));
-    }
 }