fix(tests): replace hardcoded /tmp paths with tempdir + add 300 unit tests (#659)

* test: add unit tests across 20 modules for coverage push

Add 300+ unit tests covering config, context, evaluation, extensions,
LLM, secrets, tools/builder, and tools/mcp modules. All tests are
pure unit tests (no mocks) exercising serde roundtrips, edge cases,
error paths, and business logic.

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

* fix(tests): replace hardcoded /tmp paths with tempfile::tempdir

The e2e_metrics_test::test_metrics_collected_from_tool_trace test was
failing because setup_test_dir() created /tmp/ironclaw_metrics_test but
the fixture referenced /tmp/ironclaw_e2e_test/hello.txt (path mismatch).

Added LlmTrace::replace_paths() to substitute fixture paths at runtime,
then converted all 12 test files from hardcoded /tmp/ironclaw_* paths to
tempfile::tempdir(). Tests are now isolated, parallel-safe, and leave no
debris on disk.

Regression test: test_metrics_collected_from_tool_trace now passes
consistently regardless of prior /tmp state.

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

---------

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

src/tools/builder/core.rs

@@ -1026,24 +1026,384 @@ impl Tool for BuildSoftwareTool {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use crate::tools::builder::core::*;
 
     #[test]
-    fn test_language_extensions() {
+    fn test_language_extension_all_variants() {
         assert_eq!(Language::Rust.extension(), "rs");
         assert_eq!(Language::Python.extension(), "py");
         assert_eq!(Language::TypeScript.extension(), "ts");
+        assert_eq!(Language::JavaScript.extension(), "js");
+        assert_eq!(Language::Go.extension(), "go");
+        assert_eq!(Language::Bash.extension(), "sh");
     }
 
     #[test]
-    fn test_build_commands() {
-        assert!(Language::Rust.build_command("/tmp/project").is_some());
-        assert!(Language::Python.build_command("/tmp/project").is_none());
+    fn test_language_build_command_compiled_returns_some() {
+        let dir = "/tmp/project";
+        let rust_cmd = Language::Rust.build_command(dir);
+        assert!(rust_cmd.is_some());
+        assert!(rust_cmd.unwrap().contains("cargo build"));
+
+        let ts_cmd = Language::TypeScript.build_command(dir);
+        assert!(ts_cmd.is_some());
+        assert!(ts_cmd.unwrap().contains("npm run build"));
+
+        let go_cmd = Language::Go.build_command(dir);
+        assert!(go_cmd.is_some());
+        assert!(go_cmd.unwrap().contains("go build"));
     }
 
     #[test]
-    fn test_software_type_serialization() {
-        let json = serde_json::to_string(&SoftwareType::WasmTool).unwrap();
-        assert_eq!(json, "\"wasm_tool\"");
+    fn test_language_build_command_interpreted_returns_none() {
+        let dir = "/tmp/project";
+        assert!(Language::Python.build_command(dir).is_none());
+        assert!(Language::JavaScript.build_command(dir).is_none());
+        assert!(Language::Bash.build_command(dir).is_none());
+    }
+
+    #[test]
+    fn test_language_build_command_includes_project_dir() {
+        let dir = "/home/user/my_project";
+        for lang in [Language::Rust, Language::TypeScript, Language::Go] {
+            let cmd = lang.build_command(dir);
+            assert!(
+                cmd.as_ref().unwrap().contains(dir),
+                "{:?} build command should contain project dir",
+                lang
+            );
+        }
+    }
+
+    #[test]
+    fn test_language_test_command_all_variants_non_empty() {
+        let dir = "/tmp/project";
+        let all_languages = [
+            Language::Rust,
+            Language::Python,
+            Language::TypeScript,
+            Language::JavaScript,
+            Language::Go,
+            Language::Bash,
+        ];
+        for lang in all_languages {
+            let cmd = lang.test_command(dir);
+            assert!(
+                !cmd.is_empty(),
+                "{:?} test command should not be empty",
+                lang
+            );
+            assert!(
+                cmd.contains(dir),
+                "{:?} test command should contain project dir",
+                lang
+            );
+        }
+    }
+
+    #[test]
+    fn test_language_test_command_specific_tools() {
+        let dir = "/tmp/p";
+        assert!(Language::Rust.test_command(dir).contains("cargo test"));
+        assert!(Language::Python.test_command(dir).contains("pytest"));
+        assert!(Language::TypeScript.test_command(dir).contains("npm test"));
+        assert!(Language::JavaScript.test_command(dir).contains("npm test"));
+        assert!(Language::Go.test_command(dir).contains("go test"));
+        assert!(Language::Bash.test_command(dir).contains("shellcheck"));
+    }
+
+    #[test]
+    fn test_software_type_serde_roundtrip() {
+        let variants = [
+            SoftwareType::WasmTool,
+            SoftwareType::CliBinary,
+            SoftwareType::Library,
+            SoftwareType::Script,
+            SoftwareType::WebService,
+        ];
+        let expected_strings = [
+            "\"wasm_tool\"",
+            "\"cli_binary\"",
+            "\"library\"",
+            "\"script\"",
+            "\"web_service\"",
+        ];
+        for (variant, expected) in variants.iter().zip(expected_strings.iter()) {
+            let json = serde_json::to_string(variant).unwrap();
+            assert_eq!(&json, expected, "serialization mismatch for {:?}", variant);
+            let deserialized: SoftwareType = serde_json::from_str(&json).unwrap();
+            assert_eq!(
+                &deserialized, variant,
+                "roundtrip mismatch for {:?}",
+                variant
+            );
+        }
+    }
+
+    #[test]
+    fn test_language_serde_roundtrip() {
+        let variants = [
+            Language::Rust,
+            Language::Python,
+            Language::TypeScript,
+            Language::JavaScript,
+            Language::Go,
+            Language::Bash,
+        ];
+        let expected_strings = [
+            "\"rust\"",
+            "\"python\"",
+            "\"type_script\"",
+            "\"java_script\"",
+            "\"go\"",
+            "\"bash\"",
+        ];
+        for (variant, expected) in variants.iter().zip(expected_strings.iter()) {
+            let json = serde_json::to_string(variant).unwrap();
+            assert_eq!(&json, expected, "serialization mismatch for {:?}", variant);
+            let deserialized: Language = serde_json::from_str(&json).unwrap();
+            assert_eq!(
+                &deserialized, variant,
+                "roundtrip mismatch for {:?}",
+                variant
+            );
+        }
+    }
+
+    #[test]
+    fn test_build_requirement_serde_roundtrip() {
+        let req = BuildRequirement {
+            name: "my_tool".into(),
+            description: "A tool that does stuff".into(),
+            software_type: SoftwareType::WasmTool,
+            language: Language::Rust,
+            input_spec: Some("JSON object with 'query' field".into()),
+            output_spec: Some("JSON object with 'result' field".into()),
+            dependencies: vec!["serde".into(), "reqwest".into()],
+            capabilities: vec!["http".into(), "workspace".into()],
+        };
+        let json = serde_json::to_string(&req).unwrap();
+        let deserialized: BuildRequirement = serde_json::from_str(&json).unwrap();
+        assert_eq!(deserialized.name, req.name);
+        assert_eq!(deserialized.description, req.description);
+        assert_eq!(deserialized.software_type, req.software_type);
+        assert_eq!(deserialized.language, req.language);
+        assert_eq!(deserialized.input_spec, req.input_spec);
+        assert_eq!(deserialized.output_spec, req.output_spec);
+        assert_eq!(deserialized.dependencies, req.dependencies);
+        assert_eq!(deserialized.capabilities, req.capabilities);
+    }
+
+    #[test]
+    fn test_build_requirement_serde_optional_fields_none() {
+        let req = BuildRequirement {
+            name: "minimal".into(),
+            description: "Bare minimum".into(),
+            software_type: SoftwareType::Script,
+            language: Language::Bash,
+            input_spec: None,
+            output_spec: None,
+            dependencies: vec![],
+            capabilities: vec![],
+        };
+        let json = serde_json::to_string(&req).unwrap();
+        let deserialized: BuildRequirement = serde_json::from_str(&json).unwrap();
+        assert!(deserialized.input_spec.is_none());
+        assert!(deserialized.output_spec.is_none());
+        assert!(deserialized.dependencies.is_empty());
+        assert!(deserialized.capabilities.is_empty());
+    }
+
+    #[test]
+    fn test_builder_config_default_sensible_values() {
+        let config = BuilderConfig::default();
+        assert!(config.max_iterations > 0, "max_iterations must be positive");
+        assert!(!config.timeout.is_zero(), "timeout must be non-zero");
+        assert!(
+            config.timeout.as_secs() >= 60,
+            "timeout should be at least 60 seconds"
+        );
+        assert!(config.validate_wasm, "validate_wasm should default to true");
+        assert!(config.run_tests, "run_tests should default to true");
+        assert!(config.auto_register, "auto_register should default to true");
+        assert!(
+            !config.cleanup_on_failure,
+            "cleanup_on_failure should default to false for debugging"
+        );
+        assert!(
+            config.wasm_output_dir.is_none(),
+            "wasm_output_dir should default to None"
+        );
+        assert!(
+            config
+                .build_dir
+                .to_string_lossy()
+                .contains("ironclaw-builds"),
+            "build_dir should contain 'ironclaw-builds'"
+        );
+    }
+
+    #[test]
+    fn test_build_phase_serde_roundtrip() {
+        let variants = [
+            BuildPhase::Analyzing,
+            BuildPhase::Scaffolding,
+            BuildPhase::Implementing,
+            BuildPhase::Building,
+            BuildPhase::Testing,
+            BuildPhase::Fixing,
+            BuildPhase::Validating,
+            BuildPhase::Registering,
+            BuildPhase::Packaging,
+            BuildPhase::Complete,
+            BuildPhase::Failed,
+        ];
+        for variant in &variants {
+            let json = serde_json::to_string(variant).unwrap();
+            let deserialized: BuildPhase = serde_json::from_str(&json).unwrap();
+            assert_eq!(
+                &deserialized, variant,
+                "roundtrip mismatch for {:?}",
+                variant
+            );
+        }
+    }
+
+    #[test]
+    fn test_build_result_serde_success() {
+        let result = BuildResult {
+            build_id: Uuid::nil(),
+            requirement: BuildRequirement {
+                name: "test_tool".into(),
+                description: "test".into(),
+                software_type: SoftwareType::WasmTool,
+                language: Language::Rust,
+                input_spec: None,
+                output_spec: None,
+                dependencies: vec![],
+                capabilities: vec![],
+            },
+            artifact_path: PathBuf::from("/tmp/test.wasm"),
+            logs: vec![],
+            success: true,
+            error: None,
+            started_at: Utc::now(),
+            completed_at: Utc::now(),
+            iterations: 3,
+            validation_warnings: vec![],
+            tests_passed: 5,
+            tests_failed: 0,
+            registered: true,
+        };
+        let json = serde_json::to_string(&result).unwrap();
+        let deserialized: BuildResult = serde_json::from_str(&json).unwrap();
+        assert!(deserialized.success);
+        assert!(deserialized.error.is_none());
+        assert_eq!(deserialized.iterations, 3);
+        assert_eq!(deserialized.tests_passed, 5);
+        assert_eq!(deserialized.tests_failed, 0);
+        assert!(deserialized.registered);
+    }
+
+    #[test]
+    fn test_build_result_serde_failure() {
+        let result = BuildResult {
+            build_id: Uuid::nil(),
+            requirement: BuildRequirement {
+                name: "broken".into(),
+                description: "fails".into(),
+                software_type: SoftwareType::CliBinary,
+                language: Language::Go,
+                input_spec: None,
+                output_spec: None,
+                dependencies: vec![],
+                capabilities: vec![],
+            },
+            artifact_path: PathBuf::from("/tmp/broken"),
+            logs: vec![],
+            success: false,
+            error: Some("compilation error: undefined reference".into()),
+            started_at: Utc::now(),
+            completed_at: Utc::now(),
+            iterations: 10,
+            validation_warnings: vec!["missing export".into()],
+            tests_passed: 2,
+            tests_failed: 3,
+            registered: false,
+        };
+        let json = serde_json::to_string(&result).unwrap();
+        let deserialized: BuildResult = serde_json::from_str(&json).unwrap();
+        assert!(!deserialized.success);
+        assert_eq!(
+            deserialized.error.as_deref(),
+            Some("compilation error: undefined reference")
+        );
+        assert_eq!(deserialized.iterations, 10);
+        assert_eq!(deserialized.validation_warnings.len(), 1);
+        assert_eq!(deserialized.tests_passed, 2);
+        assert_eq!(deserialized.tests_failed, 3);
+        assert!(!deserialized.registered);
+    }
+
+    #[test]
+    fn test_build_result_default_fields_from_json() {
+        // Verify #[serde(default)] fields can be omitted in JSON
+        let json = serde_json::json!({
+            "build_id": "00000000-0000-0000-0000-000000000000",
+            "requirement": {
+                "name": "x",
+                "description": "y",
+                "software_type": "script",
+                "language": "bash",
+                "input_spec": null,
+                "output_spec": null,
+                "dependencies": [],
+                "capabilities": []
+            },
+            "artifact_path": "/tmp/x.sh",
+            "logs": [],
+            "success": true,
+            "error": null,
+            "started_at": "2025-01-01T00:00:00Z",
+            "completed_at": "2025-01-01T00:01:00Z",
+            "iterations": 1
+        });
+        let result: BuildResult = serde_json::from_value(json).unwrap();
+        assert_eq!(result.validation_warnings, Vec::<String>::new());
+        assert_eq!(result.tests_passed, 0);
+        assert_eq!(result.tests_failed, 0);
+        assert!(!result.registered);
+    }
+
+    #[test]
+    fn test_build_log_serde_roundtrip() {
+        let log = BuildLog {
+            timestamp: Utc::now(),
+            phase: BuildPhase::Building,
+            message: "Running cargo build".into(),
+            details: Some("cargo build --release 2>&1".into()),
+        };
+        let json = serde_json::to_string(&log).unwrap();
+        let deserialized: BuildLog = serde_json::from_str(&json).unwrap();
+        assert_eq!(deserialized.phase, BuildPhase::Building);
+        assert_eq!(deserialized.message, "Running cargo build");
+        assert_eq!(
+            deserialized.details.as_deref(),
+            Some("cargo build --release 2>&1")
+        );
+    }
+
+    #[test]
+    fn test_build_log_serde_details_none() {
+        let log = BuildLog {
+            timestamp: Utc::now(),
+            phase: BuildPhase::Complete,
+            message: "Done".into(),
+            details: None,
+        };
+        let json = serde_json::to_string(&log).unwrap();
+        let deserialized: BuildLog = serde_json::from_str(&json).unwrap();
+        assert!(deserialized.details.is_none());
+        assert_eq!(deserialized.phase, BuildPhase::Complete);
     }
 }

src/tools/builder/templates.rs

@@ -498,4 +498,163 @@ mod tests {
         assert_eq!(template.name, "WASM HTTP Tool");
         assert!(!template.files.is_empty());
     }
+
+    #[test]
+    fn test_render_no_variables() {
+        let engine = TemplateEngine::new();
+        let input = "Hello, world! No placeholders here.";
+        assert_eq!(engine.render(input), input);
+    }
+
+    #[test]
+    fn test_render_variable_not_found() {
+        let mut engine = TemplateEngine::new();
+        engine.set("name", "ironclaw");
+        let input = "Name: {{name}}, Missing: {{missing}}";
+        assert_eq!(engine.render(input), "Name: ironclaw, Missing: {{missing}}");
+    }
+
+    #[test]
+    fn test_render_multiple_replacements_of_same_variable() {
+        let mut engine = TemplateEngine::new();
+        engine.set("x", "42");
+        assert_eq!(engine.render("{{x}} + {{x}} = 2*{{x}}"), "42 + 42 = 2*42");
+    }
+
+    #[test]
+    fn test_set_overwrites_existing_variable() {
+        let mut engine = TemplateEngine::new();
+        engine.set("color", "red");
+        assert_eq!(engine.render("{{color}}"), "red");
+        engine.set("color", "blue");
+        assert_eq!(engine.render("{{color}}"), "blue");
+    }
+
+    #[test]
+    fn test_render_template_all_files() {
+        let mut engine = TemplateEngine::new();
+        engine.set("name", "my_tool");
+        engine.set("description", "does stuff");
+
+        let template = Template::get(TemplateType::CliBinary);
+        let rendered = engine.render_template(&template);
+
+        assert_eq!(rendered.len(), template.files.len());
+        // Paths should have variables substituted
+        for (path, _content) in &rendered {
+            assert!(!path.contains("{{name}}"));
+        }
+        // Content should have variables substituted
+        for (_path, content) in &rendered {
+            assert!(!content.contains("{{name}}"));
+            assert!(!content.contains("{{description}}"));
+        }
+    }
+
+    #[test]
+    fn test_all_template_types_return_non_empty() {
+        let all_types = [
+            TemplateType::WasmHttpTool,
+            TemplateType::WasmTransformTool,
+            TemplateType::WasmComputeTool,
+            TemplateType::CliBinary,
+            TemplateType::PythonScript,
+            TemplateType::BashScript,
+        ];
+        for tt in all_types {
+            let t = Template::get(tt);
+            assert!(!t.name.is_empty(), "{:?} has empty name", tt);
+            assert!(!t.description.is_empty(), "{:?} has empty description", tt);
+            assert!(!t.files.is_empty(), "{:?} has no files", tt);
+            for f in &t.files {
+                assert!(
+                    !f.content.is_empty(),
+                    "{:?} file {:?} has empty content",
+                    tt,
+                    f.path
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn test_template_type_serde_roundtrip() {
+        let all_types = [
+            TemplateType::WasmHttpTool,
+            TemplateType::WasmTransformTool,
+            TemplateType::WasmComputeTool,
+            TemplateType::CliBinary,
+            TemplateType::PythonScript,
+            TemplateType::BashScript,
+        ];
+        for tt in all_types {
+            let json = serde_json::to_string(&tt).unwrap();
+            let back: TemplateType = serde_json::from_str(&json).unwrap();
+            assert_eq!(back, tt, "roundtrip failed for {:?} (json: {})", tt, json);
+        }
+    }
+
+    #[test]
+    fn test_each_template_has_at_least_one_required_file() {
+        let all_types = [
+            TemplateType::WasmHttpTool,
+            TemplateType::WasmTransformTool,
+            TemplateType::WasmComputeTool,
+            TemplateType::CliBinary,
+            TemplateType::PythonScript,
+            TemplateType::BashScript,
+        ];
+        for tt in all_types {
+            let t = Template::get(tt);
+            let required_count = t.files.iter().filter(|f| f.is_required).count();
+            assert!(required_count >= 1, "{:?} has no required files", tt);
+        }
+    }
+
+    #[test]
+    fn test_template_file_extensions() {
+        // WASM and CLI templates should have Cargo.toml and .rs files
+        for tt in [
+            TemplateType::WasmHttpTool,
+            TemplateType::WasmTransformTool,
+            TemplateType::WasmComputeTool,
+            TemplateType::CliBinary,
+        ] {
+            let t = Template::get(tt);
+            let paths: Vec<&str> = t.files.iter().map(|f| f.path).collect();
+            assert!(
+                paths.iter().any(|p| p.ends_with("Cargo.toml")),
+                "{:?} missing Cargo.toml",
+                tt
+            );
+            assert!(
+                paths.iter().any(|p| p.ends_with(".rs")),
+                "{:?} missing .rs file",
+                tt
+            );
+        }
+
+        // Python template should have a .py file
+        let py = Template::get(TemplateType::PythonScript);
+        assert!(py.files.iter().any(|f| f.path.ends_with(".py")));
+
+        // Bash template should have a .sh file
+        let bash = Template::get(TemplateType::BashScript);
+        assert!(bash.files.iter().any(|f| f.path.ends_with(".sh")));
+    }
+
+    #[test]
+    fn test_python_and_bash_templates_have_name_in_path() {
+        let py = Template::get(TemplateType::PythonScript);
+        assert!(
+            py.files.iter().any(|f| f.path.contains("{{name}}")),
+            "PythonScript template should have {{{{name}}}} in a file path"
+        );
+
+        let bash = Template::get(TemplateType::BashScript);
+        assert!(
+            bash.files.iter().any(|f| f.path.contains("{{name}}")),
+            "BashScript template should have {{{{name}}}} in a file path"
+        );
+    }
 }

src/tools/builder/validation.rs

@@ -315,5 +315,163 @@ mod tests {
         );
     }
 
-    // Note: Full WASM parsing tests would require actual WASM binaries
+    #[test]
+    fn test_validate_bytes_invalid_bytes() {
+        let validator = WasmValidator::new();
+        let garbage = b"this is not a wasm module at all";
+        let result = validator.validate_bytes(garbage).unwrap();
+        assert!(!result.is_valid);
+        assert!(
+            result
+                .errors
+                .iter()
+                .any(|e| matches!(e, ValidationError::InvalidModule(_)))
+        );
+    }
+
+    #[test]
+    fn test_validate_bytes_empty() {
+        let validator = WasmValidator::new();
+        let result = validator.validate_bytes(b"").unwrap();
+        assert!(!result.is_valid);
+        assert!(
+            result
+                .errors
+                .iter()
+                .any(|e| matches!(e, ValidationError::InvalidModule(_)))
+        );
+    }
+
+    #[test]
+    fn test_validate_bytes_minimal_wasm_missing_run_export() {
+        let validator = WasmValidator::new();
+        // Minimal valid WASM: magic number + version
+        let minimal_wasm = b"\x00asm\x01\x00\x00\x00";
+        let result = validator.validate_bytes(minimal_wasm).unwrap();
+        assert!(!result.is_valid);
+        assert!(
+            result
+                .errors
+                .iter()
+                .any(|e| matches!(e, ValidationError::MissingExport(name) if name == "run"))
+        );
+        assert_eq!(result.size_bytes, 8);
+    }
+
+    #[test]
+    fn test_validation_result_is_valid_when_no_errors() {
+        let result = ValidationResult {
+            is_valid: true,
+            errors: vec![],
+            warnings: vec!["some warning".to_string()],
+            exports: vec![],
+            imports: vec![],
+            size_bytes: 0,
+        };
+        assert!(result.is_valid);
+        assert!(result.errors.is_empty());
+    }
+
+    #[test]
+    fn test_validation_result_is_invalid_when_errors_present() {
+        let result = ValidationResult {
+            is_valid: false,
+            errors: vec![ValidationError::MissingExport("run".to_string())],
+            warnings: vec![],
+            exports: vec![],
+            imports: vec![],
+            size_bytes: 0,
+        };
+        assert!(!result.is_valid);
+        assert_eq!(result.errors.len(), 1);
+    }
+
+    #[test]
+    fn test_validation_error_display() {
+        let io_err =
+            ValidationError::IoError(std::io::Error::new(std::io::ErrorKind::NotFound, "gone"));
+        assert!(io_err.to_string().contains("Failed to read WASM file"));
+
+        let invalid = ValidationError::InvalidModule("bad magic".to_string());
+        assert!(invalid.to_string().contains("Invalid WASM module"));
+        assert!(invalid.to_string().contains("bad magic"));
+
+        let missing = ValidationError::MissingExport("run".to_string());
+        assert!(missing.to_string().contains("Missing required export"));
+        assert!(missing.to_string().contains("run"));
+
+        let sig = ValidationError::InvalidSignature {
+            name: "run".to_string(),
+            expected: "() -> i32".to_string(),
+            actual: "() -> ()".to_string(),
+        };
+        assert!(sig.to_string().contains("Invalid export signature"));
+        assert!(sig.to_string().contains("run"));
+
+        let disallowed = ValidationError::DisallowedImport {
+            module: "evil".to_string(),
+            name: "hack".to_string(),
+        };
+        assert!(disallowed.to_string().contains("disallowed import"));
+        assert!(disallowed.to_string().contains("evil::hack"));
+
+        let too_large = ValidationError::TooLarge {
+            size: 200,
+            max: 100,
+        };
+        assert!(too_large.to_string().contains("200"));
+        assert!(too_large.to_string().contains("100"));
+
+        let other = ValidationError::Other("something broke".to_string());
+        assert!(other.to_string().contains("something broke"));
+    }
+
+    #[test]
+    fn test_export_kind_equality() {
+        assert_eq!(ExportKind::Function, ExportKind::Function);
+        assert_eq!(ExportKind::Memory, ExportKind::Memory);
+        assert_eq!(ExportKind::Table, ExportKind::Table);
+        assert_eq!(ExportKind::Global, ExportKind::Global);
+        assert_ne!(ExportKind::Function, ExportKind::Memory);
+        assert_ne!(ExportKind::Table, ExportKind::Global);
+    }
+
+    #[test]
+    fn test_import_kind_equality() {
+        assert_eq!(ImportKind::Function, ImportKind::Function);
+        assert_eq!(ImportKind::Memory, ImportKind::Memory);
+        assert_eq!(ImportKind::Table, ImportKind::Table);
+        assert_eq!(ImportKind::Global, ImportKind::Global);
+        assert_ne!(ImportKind::Function, ImportKind::Global);
+        assert_ne!(ImportKind::Memory, ImportKind::Table);
+    }
+
+    #[test]
+    fn test_validate_bytes_exceeds_max_size() {
+        let validator = WasmValidator::new().with_max_size(4);
+        // 8 bytes, over the 4-byte limit
+        let minimal_wasm = b"\x00asm\x01\x00\x00\x00";
+        let result = validator.validate_bytes(minimal_wasm).unwrap();
+        assert!(!result.is_valid);
+        assert!(
+            result
+                .errors
+                .iter()
+                .any(|e| matches!(e, ValidationError::TooLarge { size: 8, max: 4 }))
+        );
+    }
+
+    #[test]
+    fn test_with_max_size_then_validate_over_limit() {
+        let validator = WasmValidator::new().with_max_size(16);
+        let oversized = vec![0u8; 32];
+        let result = validator.validate_bytes(&oversized).unwrap();
+        assert!(!result.is_valid);
+        assert!(
+            result
+                .errors
+                .iter()
+                .any(|e| matches!(e, ValidationError::TooLarge { size: 32, max: 16 }))
+        );
+    }
 }

src/tools/builtin/extension_tools.rs

@@ -671,8 +671,8 @@ mod tests {
             Arc::new(ToolRegistry::new()),
             None,
             None,
-            std::path::PathBuf::from("/tmp/ironclaw-test-tools"),
-            std::path::PathBuf::from("/tmp/ironclaw-test-channels"),
+            std::env::temp_dir().join("ironclaw-test-tools"),
+            std::env::temp_dir().join("ironclaw-test-channels"),
             None,
             "test".to_string(),
             None,

src/tools/mcp/auth.rs

@@ -858,4 +858,310 @@ mod tests {
         assert!(url.contains("owner=user"));
         assert!(url.contains("state=abc123"));
     }
+
+    #[test]
+    fn test_pkce_challenge_s256_is_correct_sha256() {
+        let pkce = PkceChallenge::generate();
+
+        // Recompute the S256 challenge from scratch and compare.
+        let mut hasher = Sha256::new();
+        hasher.update(pkce.verifier.as_bytes());
+        let expected = URL_SAFE_NO_PAD.encode(hasher.finalize());
+
+        assert_eq!(pkce.challenge, expected);
+    }
+
+    #[test]
+    fn test_build_authorization_url_empty_scopes_no_scope_param() {
+        let url = build_authorization_url(
+            "https://auth.example.com/authorize",
+            "client-123",
+            "http://localhost:9876/callback",
+            &[],
+            None,
+            &HashMap::new(),
+        );
+
+        // With no scopes, the URL must not contain a scope parameter at all.
+        assert!(!url.contains("scope="));
+    }
+
+    #[test]
+    fn test_build_authorization_url_special_characters_are_encoded() {
+        let url = build_authorization_url(
+            "https://auth.example.com/authorize",
+            "client id&evil=true",
+            "http://localhost:9876/call back?x=1",
+            &[],
+            None,
+            &HashMap::new(),
+        );
+
+        // Spaces and ampersands in client_id must be percent-encoded.
+        assert!(url.contains("client_id=client%20id%26evil%3Dtrue"));
+        // Spaces and question marks in redirect_uri must be percent-encoded.
+        assert!(url.contains("redirect_uri=http%3A%2F%2Flocalhost%3A9876%2Fcall%20back%3Fx%3D1"));
+    }
+
+    #[test]
+    fn test_protected_resource_metadata_serde_roundtrip_full() {
+        let meta = ProtectedResourceMetadata {
+            resource: "https://mcp.example.com".to_string(),
+            authorization_servers: vec![
+                "https://auth1.example.com".to_string(),
+                "https://auth2.example.com".to_string(),
+            ],
+            scopes_supported: vec!["read".to_string(), "write".to_string()],
+        };
+
+        let json = serde_json::to_string(&meta).unwrap();
+        let deserialized: ProtectedResourceMetadata = serde_json::from_str(&json).unwrap();
+
+        assert_eq!(deserialized.resource, meta.resource);
+        assert_eq!(
+            deserialized.authorization_servers,
+            meta.authorization_servers
+        );
+        assert_eq!(deserialized.scopes_supported, meta.scopes_supported);
+    }
+
+    #[test]
+    fn test_protected_resource_metadata_serde_roundtrip_minimal() {
+        // Only required field, optional vecs should default to empty.
+        let json = r#"{"resource": "https://mcp.example.com"}"#;
+        let meta: ProtectedResourceMetadata = serde_json::from_str(json).unwrap();
+
+        assert_eq!(meta.resource, "https://mcp.example.com");
+        assert!(meta.authorization_servers.is_empty());
+        assert!(meta.scopes_supported.is_empty());
+    }
+
+    #[test]
+    fn test_authorization_server_metadata_serde_roundtrip_all_fields() {
+        let meta = AuthorizationServerMetadata {
+            issuer: "https://auth.example.com".to_string(),
+            authorization_endpoint: "https://auth.example.com/authorize".to_string(),
+            token_endpoint: "https://auth.example.com/token".to_string(),
+            registration_endpoint: Some("https://auth.example.com/register".to_string()),
+            response_types_supported: vec!["code".to_string()],
+            grant_types_supported: vec![
+                "authorization_code".to_string(),
+                "refresh_token".to_string(),
+            ],
+            code_challenge_methods_supported: vec!["S256".to_string()],
+            scopes_supported: vec!["openid".to_string(), "profile".to_string()],
+        };
+
+        let json = serde_json::to_string(&meta).unwrap();
+        let rt: AuthorizationServerMetadata = serde_json::from_str(&json).unwrap();
+
+        assert_eq!(rt.issuer, meta.issuer);
+        assert_eq!(rt.authorization_endpoint, meta.authorization_endpoint);
+        assert_eq!(rt.token_endpoint, meta.token_endpoint);
+        assert_eq!(rt.registration_endpoint, meta.registration_endpoint);
+        assert_eq!(rt.response_types_supported, meta.response_types_supported);
+        assert_eq!(rt.grant_types_supported, meta.grant_types_supported);
+        assert_eq!(
+            rt.code_challenge_methods_supported,
+            meta.code_challenge_methods_supported
+        );
+        assert_eq!(rt.scopes_supported, meta.scopes_supported);
+    }
+
+    #[test]
+    fn test_authorization_server_metadata_serde_without_registration() {
+        let json = r#"{
+            "issuer": "https://auth.example.com",
+            "authorization_endpoint": "https://auth.example.com/authorize",
+            "token_endpoint": "https://auth.example.com/token"
+        }"#;
+
+        let meta: AuthorizationServerMetadata = serde_json::from_str(json).unwrap();
+        assert_eq!(meta.issuer, "https://auth.example.com");
+        assert!(meta.registration_endpoint.is_none());
+        assert!(meta.response_types_supported.is_empty());
+        assert!(meta.grant_types_supported.is_empty());
+    }
+
+    #[test]
+    fn test_client_registration_request_serialization() {
+        let req = ClientRegistrationRequest {
+            client_name: "IronClaw".to_string(),
+            redirect_uris: vec!["http://localhost:9876/callback".to_string()],
+            grant_types: vec![
+                "authorization_code".to_string(),
+                "refresh_token".to_string(),
+            ],
+            response_types: vec!["code".to_string()],
+            token_endpoint_auth_method: "none".to_string(),
+        };
+
+        let value: serde_json::Value = serde_json::to_value(&req).unwrap();
+
+        assert_eq!(value["client_name"], "IronClaw");
+        assert_eq!(value["redirect_uris"][0], "http://localhost:9876/callback");
+        assert_eq!(value["grant_types"][0], "authorization_code");
+        assert_eq!(value["grant_types"][1], "refresh_token");
+        assert_eq!(value["response_types"][0], "code");
+        assert_eq!(value["token_endpoint_auth_method"], "none");
+    }
+
+    #[test]
+    fn test_client_registration_response_deserialization_full() {
+        let json = r#"{
+            "client_id": "abc-123",
+            "client_secret": "s3cret",
+            "client_secret_expires_at": 1700000000,
+            "registration_access_token": "reg-tok",
+            "registration_client_uri": "https://auth.example.com/register/abc-123"
+        }"#;
+
+        let resp: ClientRegistrationResponse = serde_json::from_str(json).unwrap();
+
+        assert_eq!(resp.client_id, "abc-123");
+        assert_eq!(resp.client_secret.as_deref(), Some("s3cret"));
+        assert_eq!(resp.client_secret_expires_at, Some(1700000000));
+        assert_eq!(resp.registration_access_token.as_deref(), Some("reg-tok"));
+        assert_eq!(
+            resp.registration_client_uri.as_deref(),
+            Some("https://auth.example.com/register/abc-123")
+        );
+    }
+
+    #[test]
+    fn test_client_registration_response_deserialization_minimal() {
+        let json = r#"{"client_id": "xyz-789"}"#;
+
+        let resp: ClientRegistrationResponse = serde_json::from_str(json).unwrap();
+
+        assert_eq!(resp.client_id, "xyz-789");
+        assert!(resp.client_secret.is_none());
+        assert!(resp.client_secret_expires_at.is_none());
+        assert!(resp.registration_access_token.is_none());
+        assert!(resp.registration_client_uri.is_none());
+    }
+
+    #[test]
+    fn test_access_token_construction() {
+        let token = AccessToken {
+            access_token: "at-abc".to_string(),
+            token_type: "Bearer".to_string(),
+            expires_in: Some(3600),
+            refresh_token: Some("rt-xyz".to_string()),
+            scope: Some("read write".to_string()),
+        };
+
+        assert_eq!(token.access_token, "at-abc");
+        assert_eq!(token.token_type, "Bearer");
+        assert_eq!(token.expires_in, Some(3600));
+        assert_eq!(token.refresh_token.as_deref(), Some("rt-xyz"));
+        assert_eq!(token.scope.as_deref(), Some("read write"));
+
+        // Also test with no optional fields.
+        let minimal = AccessToken {
+            access_token: "tok".to_string(),
+            token_type: "bearer".to_string(),
+            expires_in: None,
+            refresh_token: None,
+            scope: None,
+        };
+        assert!(minimal.expires_in.is_none());
+        assert!(minimal.refresh_token.is_none());
+        assert!(minimal.scope.is_none());
+    }
+
+    #[test]
+    fn test_token_response_to_access_token_pattern() {
+        // TokenResponse is private, but we can test the conversion pattern
+        // by deserializing JSON the same way exchange_code_for_token does.
+        let json = r#"{
+            "access_token": "eyJ-token",
+            "token_type": "Bearer",
+            "expires_in": 7200,
+            "refresh_token": "refresh-me",
+            "scope": "openid profile"
+        }"#;
+
+        // Deserialize via the same struct path the production code uses.
+        let resp: serde_json::Value = serde_json::from_str(json).unwrap();
+        let token = AccessToken {
+            access_token: resp["access_token"].as_str().unwrap().to_string(),
+            token_type: resp["token_type"].as_str().unwrap().to_string(),
+            expires_in: resp["expires_in"].as_u64(),
+            refresh_token: resp["refresh_token"].as_str().map(String::from),
+            scope: resp["scope"].as_str().map(String::from),
+        };
+
+        assert_eq!(token.access_token, "eyJ-token");
+        assert_eq!(token.token_type, "Bearer");
+        assert_eq!(token.expires_in, Some(7200));
+        assert_eq!(token.refresh_token.as_deref(), Some("refresh-me"));
+        assert_eq!(token.scope.as_deref(), Some("openid profile"));
+
+        // Without optional fields.
+        let minimal_json = r#"{"access_token": "tok", "token_type": "bearer"}"#;
+        let resp: serde_json::Value = serde_json::from_str(minimal_json).unwrap();
+        let token = AccessToken {
+            access_token: resp["access_token"].as_str().unwrap().to_string(),
+            token_type: resp["token_type"].as_str().unwrap().to_string(),
+            expires_in: resp["expires_in"].as_u64(),
+            refresh_token: resp["refresh_token"].as_str().map(String::from),
+            scope: resp["scope"].as_str().map(String::from),
+        };
+        assert!(token.expires_in.is_none());
+        assert!(token.refresh_token.is_none());
+        assert!(token.scope.is_none());
+    }
+
+    #[test]
+    fn test_auth_error_display_strings() {
+        let cases: Vec<(AuthError, &str)> = vec![
+            (
+                AuthError::NotSupported,
+                "Server does not support OAuth authorization",
+            ),
+            (
+                AuthError::DiscoveryFailed("timeout".to_string()),
+                "Failed to discover authorization endpoints: timeout",
+            ),
+            (
+                AuthError::AuthorizationDenied,
+                "Authorization denied by user",
+            ),
+            (
+                AuthError::TokenExchangeFailed("bad code".to_string()),
+                "Token exchange failed: bad code",
+            ),
+            (
+                AuthError::RefreshFailed("expired".to_string()),
+                "Token expired and refresh failed: expired",
+            ),
+            (AuthError::NoToken, "No access token available"),
+            (
+                AuthError::Timeout,
+                "Timeout waiting for authorization callback",
+            ),
+            (
+                AuthError::PortUnavailable,
+                "Could not bind to callback port",
+            ),
+            (
+                AuthError::Http("connection refused".to_string()),
+                "HTTP error: connection refused",
+            ),
+            (
+                AuthError::Secrets("decrypt failed".to_string()),
+                "Secrets error: decrypt failed",
+            ),
+        ];
+
+        for (error, expected) in cases {
+            let display = error.to_string();
+            assert_eq!(
+                display, expected,
+                "AuthError display mismatch for {:?}",
+                error
+            );
+        }
+    }
 }

src/tools/mcp/client.rs

@@ -583,4 +583,161 @@ mod tests {
         assert!(client.session_manager.is_none());
         assert!(client.secrets.is_none());
     }
+
+    #[test]
+    fn test_extract_server_name_with_port() {
+        assert_eq!(
+            extract_server_name("http://example.com:3000"),
+            "example_com"
+        );
+    }
+
+    #[test]
+    fn test_extract_server_name_with_path() {
+        assert_eq!(
+            extract_server_name("http://api.server.io/v2/mcp"),
+            "api_server_io"
+        );
+    }
+
+    #[test]
+    fn test_extract_server_name_with_query_params() {
+        assert_eq!(
+            extract_server_name("http://mcp.example.com/endpoint?token=abc&v=1"),
+            "mcp_example_com"
+        );
+    }
+
+    #[test]
+    fn test_extract_server_name_https() {
+        assert_eq!(
+            extract_server_name("https://secure.mcp.dev"),
+            "secure_mcp_dev"
+        );
+    }
+
+    #[test]
+    fn test_extract_server_name_ip_address() {
+        assert_eq!(
+            extract_server_name("http://192.168.1.100:9090/mcp"),
+            "192_168_1_100"
+        );
+    }
+
+    #[test]
+    fn test_new_defaults() {
+        let client = McpClient::new("http://localhost:9999");
+        assert_eq!(client.server_url(), "http://localhost:9999");
+        assert_eq!(client.server_name(), "localhost");
+        assert!(client.session_manager.is_none());
+        assert!(client.secrets.is_none());
+        assert_eq!(client.user_id, "default");
+    }
+
+    #[test]
+    fn test_new_with_name_uses_custom_name() {
+        let client = McpClient::new_with_name("my-server", "http://localhost:8080");
+        assert_eq!(client.server_name(), "my-server");
+        assert_eq!(client.server_url(), "http://localhost:8080");
+        assert_eq!(client.user_id, "default");
+        assert!(client.session_manager.is_none());
+        assert!(client.secrets.is_none());
+    }
+
+    #[test]
+    fn test_server_name_accessor() {
+        let client = McpClient::new("https://tools.example.org/mcp");
+        assert_eq!(client.server_name(), "tools_example_org");
+    }
+
+    #[test]
+    fn test_server_url_accessor() {
+        let url = "https://tools.example.org/mcp?v=2";
+        let client = McpClient::new(url);
+        assert_eq!(client.server_url(), url);
+    }
+
+    #[test]
+    fn test_clone_preserves_fields() {
+        let client = McpClient::new_with_name("cloned-server", "http://localhost:5555");
+        // Bump the request ID a few times
+        client.next_request_id();
+        client.next_request_id();
+
+        let cloned = client.clone();
+        assert_eq!(cloned.server_url(), "http://localhost:5555");
+        assert_eq!(cloned.server_name(), "cloned-server");
+        assert_eq!(cloned.user_id, "default");
+        // The atomic counter value is copied
+        assert_eq!(cloned.next_id.load(Ordering::SeqCst), 3);
+    }
+
+    #[tokio::test]
+    async fn test_clone_resets_tools_cache() {
+        let client = McpClient::new("http://localhost:5555");
+        // The clone implementation resets tools_cache to None
+        let cloned = client.clone();
+        let cache = cloned.tools_cache.read().await;
+        assert!(cache.is_none());
+    }
+
+    #[test]
+    fn test_next_request_id_monotonically_increasing() {
+        let client = McpClient::new("http://localhost:1234");
+        let id1 = client.next_request_id();
+        let id2 = client.next_request_id();
+        let id3 = client.next_request_id();
+        assert_eq!(id1, 1);
+        assert_eq!(id2, 2);
+        assert_eq!(id3, 3);
+    }
+
+    #[test]
+    fn test_mcp_tool_requires_approval_destructive() {
+        use crate::tools::mcp::protocol::{McpTool, McpToolAnnotations};
+
+        let tool = McpTool {
+            name: "delete_all".to_string(),
+            description: "Deletes everything".to_string(),
+            input_schema: serde_json::json!({"type": "object"}),
+            annotations: Some(McpToolAnnotations {
+                destructive_hint: true,
+                side_effects_hint: false,
+                read_only_hint: false,
+                execution_time_hint: None,
+            }),
+        };
+        assert!(tool.requires_approval());
+    }
+
+    #[test]
+    fn test_mcp_tool_no_approval_when_not_destructive() {
+        use crate::tools::mcp::protocol::{McpTool, McpToolAnnotations};
+
+        let tool = McpTool {
+            name: "read_data".to_string(),
+            description: "Reads data".to_string(),
+            input_schema: serde_json::json!({"type": "object"}),
+            annotations: Some(McpToolAnnotations {
+                destructive_hint: false,
+                side_effects_hint: true,
+                read_only_hint: false,
+                execution_time_hint: None,
+            }),
+        };
+        assert!(!tool.requires_approval());
+    }
+
+    #[test]
+    fn test_mcp_tool_no_approval_when_no_annotations() {
+        use crate::tools::mcp::protocol::McpTool;
+
+        let tool = McpTool {
+            name: "simple_tool".to_string(),
+            description: "A simple tool".to_string(),
+            input_schema: serde_json::json!({"type": "object"}),
+            annotations: None,
+        };
+        assert!(!tool.requires_approval());
+    }
 }

src/tools/mcp/protocol.rs

@@ -352,6 +352,279 @@ mod tests {
         assert!(tool.input_schema["properties"].is_object());
     }
 
+    #[test]
+    fn test_initialize_request() {
+        let req = McpRequest::initialize(42);
+        assert_eq!(req.jsonrpc, "2.0");
+        assert_eq!(req.id, 42);
+        assert_eq!(req.method, "initialize");
+
+        let params = req.params.expect("initialize must have params");
+        assert_eq!(params["protocolVersion"], PROTOCOL_VERSION);
+        assert!(params["capabilities"].is_object());
+        assert!(params["capabilities"]["roots"].is_object());
+        assert!(params["capabilities"]["sampling"].is_object());
+        assert_eq!(params["clientInfo"]["name"], "ironclaw");
+        assert!(params["clientInfo"]["version"].is_string());
+    }
+
+    #[test]
+    fn test_initialized_notification() {
+        let req = McpRequest::initialized_notification();
+        assert_eq!(req.jsonrpc, "2.0");
+        assert_eq!(req.method, "notifications/initialized");
+        assert!(req.params.is_none());
+    }
+
+    #[test]
+    fn test_call_tool_request() {
+        let args = serde_json::json!({"query": "rust async"});
+        let req = McpRequest::call_tool(7, "search", args.clone());
+        assert_eq!(req.id, 7);
+        assert_eq!(req.method, "tools/call");
+
+        let params = req.params.expect("call_tool must have params");
+        assert_eq!(params["name"], "search");
+        assert_eq!(params["arguments"], args);
+    }
+
+    #[test]
+    fn test_mcp_response_deserialize_success() {
+        let json = serde_json::json!({
+            "jsonrpc": "2.0",
+            "id": 1,
+            "result": { "tools": [] }
+        });
+        let resp: McpResponse = serde_json::from_value(json).expect("deserialize");
+        assert_eq!(resp.id, 1);
+        assert!(resp.result.is_some());
+        assert!(resp.error.is_none());
+    }
+
+    #[test]
+    fn test_mcp_response_deserialize_error() {
+        let json = serde_json::json!({
+            "jsonrpc": "2.0",
+            "id": 2,
+            "error": {
+                "code": -32601,
+                "message": "Method not found"
+            }
+        });
+        let resp: McpResponse = serde_json::from_value(json).expect("deserialize");
+        assert!(resp.result.is_none());
+        let err = resp.error.expect("should have error");
+        assert_eq!(err.code, -32601);
+        assert_eq!(err.message, "Method not found");
+        assert!(err.data.is_none());
+    }
+
+    #[test]
+    fn test_mcp_error_roundtrip() {
+        let err = McpError {
+            code: -32600,
+            message: "Invalid Request".to_string(),
+            data: Some(serde_json::json!({"detail": "missing field"})),
+        };
+        let serialized = serde_json::to_string(&err).expect("serialize");
+        let deserialized: McpError = serde_json::from_str(&serialized).expect("deserialize");
+        assert_eq!(deserialized.code, err.code);
+        assert_eq!(deserialized.message, err.message);
+        assert_eq!(deserialized.data, err.data);
+    }
+
+    #[test]
+    fn test_initialize_result_full() {
+        let json = serde_json::json!({
+            "protocolVersion": "2024-11-05",
+            "capabilities": {
+                "tools": { "listChanged": true },
+                "resources": { "subscribe": true, "listChanged": false },
+                "prompts": { "listChanged": true },
+                "logging": {}
+            },
+            "serverInfo": {
+                "name": "test-server",
+                "version": "1.2.3"
+            },
+            "instructions": "Use this server for testing."
+        });
+        let result: InitializeResult = serde_json::from_value(json).expect("deserialize");
+        assert_eq!(result.protocol_version.as_deref(), Some("2024-11-05"));
+
+        let tools_cap = result.capabilities.tools.expect("has tools capability");
+        assert!(tools_cap.list_changed);
+
+        let res_cap = result
+            .capabilities
+            .resources
+            .expect("has resources capability");
+        assert!(res_cap.subscribe);
+        assert!(!res_cap.list_changed);
+
+        let prompts_cap = result.capabilities.prompts.expect("has prompts capability");
+        assert!(prompts_cap.list_changed);
+
+        assert!(result.capabilities.logging.is_some());
+
+        let info = result.server_info.expect("has server info");
+        assert_eq!(info.name, "test-server");
+        assert_eq!(info.version.as_deref(), Some("1.2.3"));
+        assert_eq!(
+            result.instructions.as_deref(),
+            Some("Use this server for testing.")
+        );
+    }
+
+    #[test]
+    fn test_content_block_as_text() {
+        let text_block = ContentBlock::Text {
+            text: "hello".to_string(),
+        };
+        assert_eq!(text_block.as_text(), Some("hello"));
+
+        let image_block = ContentBlock::Image {
+            data: "base64data".to_string(),
+            mime_type: "image/png".to_string(),
+        };
+        assert!(image_block.as_text().is_none());
+
+        let resource_block = ContentBlock::Resource {
+            uri: "file:///tmp/a.txt".to_string(),
+            mime_type: Some("text/plain".to_string()),
+            text: Some("content".to_string()),
+        };
+        assert!(resource_block.as_text().is_none());
+    }
+
+    #[test]
+    fn test_content_block_serde_tagged_union() {
+        let text_block = ContentBlock::Text {
+            text: "hi".to_string(),
+        };
+        let json = serde_json::to_value(&text_block).expect("serialize");
+        assert_eq!(json["type"], "text");
+        assert_eq!(json["text"], "hi");
+
+        let image_block = ContentBlock::Image {
+            data: "abc".to_string(),
+            mime_type: "image/jpeg".to_string(),
+        };
+        let json = serde_json::to_value(&image_block).expect("serialize");
+        assert_eq!(json["type"], "image");
+        assert_eq!(json["data"], "abc");
+        assert_eq!(json["mime_type"], "image/jpeg");
+
+        let resource_block = ContentBlock::Resource {
+            uri: "file:///x".to_string(),
+            mime_type: None,
+            text: None,
+        };
+        let json = serde_json::to_value(&resource_block).expect("serialize");
+        assert_eq!(json["type"], "resource");
+        assert_eq!(json["uri"], "file:///x");
+    }
+
+    #[test]
+    fn test_call_tool_result_is_error() {
+        let success: CallToolResult = serde_json::from_value(serde_json::json!({
+            "content": [{"type": "text", "text": "done"}],
+            "is_error": false
+        }))
+        .expect("deserialize");
+        assert!(!success.is_error);
+        assert_eq!(success.content.len(), 1);
+
+        let failure: CallToolResult = serde_json::from_value(serde_json::json!({
+            "content": [{"type": "text", "text": "boom"}],
+            "is_error": true
+        }))
+        .expect("deserialize");
+        assert!(failure.is_error);
+    }
+
+    #[test]
+    fn test_call_tool_result_is_error_defaults_false() {
+        let result: CallToolResult = serde_json::from_value(serde_json::json!({
+            "content": []
+        }))
+        .expect("deserialize");
+        assert!(!result.is_error);
+    }
+
+    #[test]
+    fn test_requires_approval_with_destructive_hint() {
+        let tool = McpTool {
+            name: "delete_all".to_string(),
+            description: "Deletes everything".to_string(),
+            input_schema: default_input_schema(),
+            annotations: Some(McpToolAnnotations {
+                destructive_hint: true,
+                ..Default::default()
+            }),
+        };
+        assert!(tool.requires_approval());
+    }
+
+    #[test]
+    fn test_requires_approval_without_destructive_hint() {
+        let tool = McpTool {
+            name: "read_file".to_string(),
+            description: "Reads a file".to_string(),
+            input_schema: default_input_schema(),
+            annotations: Some(McpToolAnnotations {
+                destructive_hint: false,
+                read_only_hint: true,
+                ..Default::default()
+            }),
+        };
+        assert!(!tool.requires_approval());
+    }
+
+    #[test]
+    fn test_requires_approval_no_annotations() {
+        let tool = McpTool {
+            name: "ping".to_string(),
+            description: "Ping".to_string(),
+            input_schema: default_input_schema(),
+            annotations: None,
+        };
+        assert!(!tool.requires_approval());
+    }
+
+    #[test]
+    fn test_mcp_tool_annotations_defaults() {
+        let annotations = McpToolAnnotations::default();
+        assert!(!annotations.destructive_hint);
+        assert!(!annotations.side_effects_hint);
+        assert!(!annotations.read_only_hint);
+        assert!(annotations.execution_time_hint.is_none());
+    }
+
+    #[test]
+    fn test_execution_time_hint_serde() {
+        // Fast
+        let json = serde_json::json!("fast");
+        let hint: ExecutionTimeHint = serde_json::from_value(json).expect("deserialize fast");
+        assert_eq!(hint, ExecutionTimeHint::Fast);
+        let serialized = serde_json::to_value(hint).expect("serialize fast");
+        assert_eq!(serialized, "fast");
+
+        // Medium
+        let json = serde_json::json!("medium");
+        let hint: ExecutionTimeHint = serde_json::from_value(json).expect("deserialize medium");
+        assert_eq!(hint, ExecutionTimeHint::Medium);
+        let serialized = serde_json::to_value(hint).expect("serialize medium");
+        assert_eq!(serialized, "medium");
+
+        // Slow
+        let json = serde_json::json!("slow");
+        let hint: ExecutionTimeHint = serde_json::from_value(json).expect("deserialize slow");
+        assert_eq!(hint, ExecutionTimeHint::Slow);
+        let serialized = serde_json::to_value(hint).expect("serialize slow");
+        assert_eq!(serialized, "slow");
+    }
+
     #[test]
     fn test_mcp_tool_roundtrip_preserves_schema() {
         // Simulate what list_tools returns from a real MCP server

src/tools/mcp/session.rs

@@ -283,4 +283,108 @@ mod tests {
         assert!(servers.contains(&"notion".to_string()));
         assert!(servers.contains(&"github".to_string()));
     }
+
+    #[test]
+    fn test_update_session_id_none_leaves_id_unchanged() {
+        let mut session = McpSession::new("https://mcp.example.com");
+        session.session_id = Some("existing-id".to_string());
+
+        session.update_session_id(None);
+
+        assert_eq!(session.session_id, Some("existing-id".to_string()));
+    }
+
+    #[test]
+    fn test_touch_updates_last_activity() {
+        let mut session = McpSession::new("https://mcp.example.com");
+        // Push last_activity into the past so we can observe the change.
+        session.last_activity = std::time::Instant::now() - std::time::Duration::from_secs(60);
+        let before = session.last_activity;
+
+        session.touch();
+
+        assert!(session.last_activity > before);
+    }
+
+    #[test]
+    fn test_with_idle_timeout() {
+        let manager = McpSessionManager::with_idle_timeout(42);
+        assert_eq!(manager.max_idle_secs, 42);
+    }
+
+    #[tokio::test]
+    async fn test_get_session_id_nonexistent_returns_none() {
+        let manager = McpSessionManager::new();
+        assert!(manager.get_session_id("ghost").await.is_none());
+    }
+
+    #[tokio::test]
+    async fn test_update_session_id_nonexistent_is_noop() {
+        let manager = McpSessionManager::new();
+        // Should not panic or create a session.
+        manager
+            .update_session_id("ghost", Some("id".to_string()))
+            .await;
+        assert!(manager.active_servers().await.is_empty());
+    }
+
+    #[tokio::test]
+    async fn test_mark_initialized_nonexistent_is_noop() {
+        let manager = McpSessionManager::new();
+        manager.mark_initialized("ghost").await;
+        assert!(manager.active_servers().await.is_empty());
+    }
+
+    #[tokio::test]
+    async fn test_touch_nonexistent_is_noop() {
+        let manager = McpSessionManager::new();
+        manager.touch("ghost").await;
+        assert!(manager.active_servers().await.is_empty());
+    }
+
+    #[tokio::test]
+    async fn test_cleanup_stale_removes_only_stale() {
+        // Use a 5-second idle timeout so we can fake staleness easily.
+        let manager = McpSessionManager::with_idle_timeout(5);
+
+        manager
+            .get_or_create("fresh", "https://fresh.example.com")
+            .await;
+        manager
+            .get_or_create("stale1", "https://stale1.example.com")
+            .await;
+        manager
+            .get_or_create("stale2", "https://stale2.example.com")
+            .await;
+
+        // Push the two stale sessions into the past.
+        {
+            let mut sessions = manager.sessions.write().await;
+            let past = std::time::Instant::now() - std::time::Duration::from_secs(60);
+            sessions.get_mut("stale1").unwrap().last_activity = past;
+            sessions.get_mut("stale2").unwrap().last_activity = past;
+        }
+
+        let removed = manager.cleanup_stale().await;
+        assert_eq!(removed, 2);
+
+        let remaining = manager.active_servers().await;
+        assert_eq!(remaining.len(), 1);
+        assert!(remaining.contains(&"fresh".to_string()));
+    }
+
+    #[tokio::test]
+    async fn test_terminate_nonexistent_is_noop() {
+        let manager = McpSessionManager::new();
+        // Should not panic.
+        manager.terminate("ghost").await;
+        assert!(manager.active_servers().await.is_empty());
+    }
+
+    #[test]
+    fn test_default_trait_impl() {
+        let manager = McpSessionManager::default();
+        // Default should match new(), which uses 1800s idle timeout.
+        assert_eq!(manager.max_idle_secs, 1800);
+    }
 }