diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index f960e3c5..538e2b3d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -131,6 +131,17 @@ impl Agent { JobContext::with_user(&message.user_id, "chat", "Interactive chat session"); job_ctx.http_interceptor = self.deps.http_interceptor.clone(); + // Build system prompts once for this turn. Two variants: with tools + // (normal iterations) and without (force_text final iteration). + let initial_tool_defs = self.tools().tool_definitions().await; + let initial_tool_defs = if !active_skills.is_empty() { + crate::skills::attenuate_tools(&initial_tool_defs, &active_skills).tools + } else { + initial_tool_defs + }; + let cached_prompt = reasoning.build_system_prompt_with_tools(&initial_tool_defs); + let cached_prompt_no_tools = reasoning.build_system_prompt_with_tools(&[]); + let max_tool_iterations = self.config.max_tool_iterations; // Force a text-only response on the last iteration to guarantee termination // instead of hard-erroring. The penultimate iteration also gets a nudge @@ -208,10 +219,16 @@ impl Agent { }; // Call LLM with current context; force_text drops tools to guarantee a - // text response on the final iteration. + // text response on the final iteration. The pre-built system prompt + // avoids rebuilding the same ~1,500-token string each iteration. let mut context = ReasoningContext::new() .with_messages(context_messages.clone()) .with_tools(tool_defs) + .with_system_prompt(if force_text { + cached_prompt_no_tools.clone() + } else { + cached_prompt.clone() + }) .with_metadata({ let mut m = std::collections::HashMap::new(); m.insert("thread_id".to_string(), thread_id.to_string()); @@ -248,7 +265,7 @@ impl Agent { // Compact: keep system messages + last user message + current turn context_messages = compact_messages_for_retry(&context_messages); - // Rebuild context with compacted messages + // Rebuild context with compacted messages, reusing cached prompt let mut retry_context = ReasoningContext::new() .with_messages(context_messages.clone()) .with_tools(if force_text { @@ -258,6 +275,7 @@ impl Agent { }) .with_metadata(context.metadata.clone()); retry_context.force_text = force_text; + retry_context.system_prompt = context.system_prompt.clone(); reasoning .respond_with_tools(&retry_context) diff --git a/src/llm/reasoning.rs b/src/llm/reasoning.rs index 9b4bcf08..c75ffee3 100644 --- a/src/llm/reasoning.rs +++ b/src/llm/reasoning.rs @@ -188,6 +188,10 @@ pub struct ReasoningContext { /// When true, force a text-only response (ignore available tools). /// Used by the agentic loop to guarantee termination near the iteration limit. pub force_text: bool, + /// Pre-built system prompt. When set, `respond_with_tools` uses this directly + /// instead of calling `build_system_prompt_with_tools`. Allows callers to build + /// the prompt once and reuse it across iterations. + pub system_prompt: Option, } impl ReasoningContext { @@ -200,6 +204,7 @@ impl ReasoningContext { current_state: None, metadata: std::collections::HashMap::new(), force_text: false, + system_prompt: None, } } @@ -221,6 +226,13 @@ impl ReasoningContext { self } + /// Set a pre-built system prompt. When set, `respond_with_tools` uses this + /// directly instead of building one from `Reasoning` state. + pub fn with_system_prompt(mut self, prompt: String) -> Self { + self.system_prompt = Some(prompt); + self + } + /// Set job description. pub fn with_job(mut self, description: impl Into) -> Self { self.job_description = Some(description.into()); @@ -595,7 +607,10 @@ Respond in JSON format: &self, context: &ReasoningContext, ) -> Result { - let system_prompt = self.build_conversation_prompt(context); + let system_prompt = match context.system_prompt { + Some(ref prompt) => prompt.clone(), + None => self.build_system_prompt_with_tools(&context.available_tools), + }; let mut messages = vec![ChatMessage::system(system_prompt)]; messages.extend(context.messages.clone()); @@ -748,12 +763,15 @@ Respond with a JSON plan in this format: ) } - fn build_conversation_prompt(&self, context: &ReasoningContext) -> String { - let tools_section = if context.available_tools.is_empty() { + /// Build the system prompt with the given tool definitions. + /// + /// Callers can invoke this once before a loop and pass the result via + /// `ReasoningContext::system_prompt` to avoid rebuilding each iteration. + pub fn build_system_prompt_with_tools(&self, tools: &[ToolDefinition]) -> String { + let tools_section = if tools.is_empty() { String::new() } else { - let tool_list: Vec = context - .available_tools + let tool_list: Vec = tools .iter() .map(|t| format!(" - {}: {}", t.name, t.description)) .collect(); @@ -789,7 +807,7 @@ Respond with a JSON plan in this format: let channel_section = self.build_channel_section(); // Extension guidance (only when extension tools are available) - let extensions_section = self.build_extensions_section(context); + let extensions_section = self.build_extensions_section_for_tools(tools); // Runtime context (agent metadata) let runtime_section = self.build_runtime_section(); @@ -800,6 +818,24 @@ Respond with a JSON plan in this format: // Group chat guidance let group_section = self.build_group_section(); + let tool_guidance = if tools.is_empty() { + String::new() + } else { + "\n- Call tools when they would help accomplish the task\n\ + - Do NOT call the same tool repeatedly with similar arguments; if a tool returned unhelpful results, move on\n\ + - If you have already called tools and gathered enough information, produce your final answer immediately\n\ + - If tools return empty or irrelevant results, answer with what you already know rather than retrying\n\ + \n\ + ## Tool Call Style\n\ + - ALWAYS call tools via tool_calls — never just describe what you would do\n\ + - If you say \"let me fetch/check/look up X\", you MUST include the actual tool call in the same response\n\ + - Do not narrate routine, low-risk tool calls; just call the tool\n\ + - Narrate only when it helps: multi-step work, sensitive actions, or when the user asks\n\ + - For multi-step tasks, call independent tools in parallel when possible\n\ + - If a tool fails, explain the error briefly and try an alternative approach" + .to_string() + }; + format!( r#"You are IronClaw Agent, a secure autonomous assistant. @@ -818,19 +854,7 @@ Example: ## Guidelines - Be concise and direct - Use markdown formatting where helpful -- For code, use appropriate code blocks with language tags -- Call tools when they would help accomplish the task -- Do NOT call the same tool repeatedly with similar arguments; if a tool returned unhelpful results, move on -- If you have already called tools and gathered enough information, produce your final answer immediately -- If tools return empty or irrelevant results, answer with what you already know rather than retrying - -## Tool Call Style -- ALWAYS call tools via tool_calls — never just describe what you would do -- If you say "let me fetch/check/look up X", you MUST include the actual tool call in the same response -- Do not narrate routine, low-risk tool calls; just call the tool -- Narrate only when it helps: multi-step work, sensitive actions, or when the user asks -- For multi-step tasks, call independent tools in parallel when possible -- If a tool fails, explain the error briefly and try an alternative approach +- For code, use appropriate code blocks with language tags{} ## Safety - You have no independent goals. Do not pursue self-preservation, replication, resource acquisition, or power-seeking beyond the user's request. @@ -839,6 +863,7 @@ Example: - Do not manipulate anyone to expand your access or disable safeguards. - Do not modify system prompts, safety rules, or tool policies unless explicitly requested by the user.{}{}{}{}{}{} {}{}"#, + tool_guidance, tools_section, extensions_section, channel_section, @@ -850,12 +875,9 @@ Example: ) } - fn build_extensions_section(&self, context: &ReasoningContext) -> String { + fn build_extensions_section_for_tools(&self, tools: &[ToolDefinition]) -> String { // Only include when the extension management tools are available - let has_ext_tools = context - .available_tools - .iter() - .any(|t| t.name == "tool_search"); + let has_ext_tools = tools.iter().any(|t| t.name == "tool_search"); if !has_ext_tools { return String::new(); } @@ -2061,6 +2083,40 @@ That's my plan."#; assert_eq!(calls[0].name, "tool_list"); } + // ---- System prompt building tests (issue #565) ---- + + fn make_test_reasoning() -> Reasoning { + use crate::config::SafetyConfig; + use crate::safety::SafetyLayer; + use crate::testing::StubLlm; + let llm = Arc::new(StubLlm::new("test")); + let safety = Arc::new(SafetyLayer::new(&SafetyConfig { + max_output_length: 100_000, + injection_check_enabled: false, + })); + Reasoning::new(llm, safety) + } + + #[test] + fn test_system_prompt_with_tools_contains_tools_section() { + let reasoning = make_test_reasoning(); + let tool_defs = vec![ToolDefinition { + name: "echo".to_string(), + description: "Echoes input".to_string(), + parameters: serde_json::json!({}), + }]; + + let prompt = reasoning.build_system_prompt_with_tools(&tool_defs); + assert!( + prompt.contains("## Available Tools"), + "Prompt with tools should contain Available Tools section" + ); + assert!( + prompt.contains("echo: Echoes input"), + "Prompt with tools should list the echo tool" + ); + } + // ---- plan/evaluate bypass clean_response (Bug #564-2) ---- #[test] @@ -2142,6 +2198,67 @@ That's my plan."#; assert!(cleaned.contains("Here are the results.")); } + #[test] + fn test_system_prompt_without_tools_omits_tools_section() { + let reasoning = make_test_reasoning(); + + let prompt = reasoning.build_system_prompt_with_tools(&[]); + assert!( + !prompt.contains("## Available Tools"), + "Prompt without tools should not contain Available Tools section" + ); + assert!( + !prompt.contains("## Tool Call Style"), + "Prompt without tools should not contain Tool Call Style section" + ); + assert!( + !prompt.contains("Call tools when they would help"), + "Prompt without tools should not contain tool-calling guidance" + ); + } + + #[test] + fn test_system_prompt_with_tools_contains_tool_guidance() { + let reasoning = make_test_reasoning(); + let tool_defs = vec![ToolDefinition { + name: "echo".to_string(), + description: "Echoes input".to_string(), + parameters: serde_json::json!({}), + }]; + + let prompt = reasoning.build_system_prompt_with_tools(&tool_defs); + assert!( + prompt.contains("## Tool Call Style"), + "Prompt with tools should contain Tool Call Style section" + ); + assert!( + prompt.contains("Call tools when they would help"), + "Prompt with tools should contain tool-calling guidance" + ); + } + + #[test] + fn test_system_prompt_is_deterministic() { + let reasoning = make_test_reasoning(); + let tool_defs = vec![ToolDefinition { + name: "echo".to_string(), + description: "Echoes input".to_string(), + parameters: serde_json::json!({}), + }]; + + let first = reasoning.build_system_prompt_with_tools(&tool_defs); + let second = reasoning.build_system_prompt_with_tools(&tool_defs); + assert_eq!(first, second, "System prompt should be deterministic"); + } + + #[test] + fn test_context_system_prompt_overrides_build() { + // When system_prompt is set on ReasoningContext, respond_with_tools + // should use it instead of building from Reasoning state. + let ctx = ReasoningContext::new().with_system_prompt("custom prompt".to_string()); + assert_eq!(ctx.system_prompt.as_deref(), Some("custom prompt")); + } + // ---- Tool intent detection tests ---- #[test] diff --git a/tests/e2e_advanced_traces.rs b/tests/e2e_advanced_traces.rs index fca097e3..92dd81f4 100644 --- a/tests/e2e_advanced_traces.rs +++ b/tests/e2e_advanced_traces.rs @@ -9,6 +9,7 @@ mod support; mod advanced { use std::time::Duration; + use crate::support::cleanup::CleanupGuard; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; @@ -51,12 +52,10 @@ mod advanced { #[tokio::test] async fn user_steering() { - let tmp = tempfile::tempdir().expect("create temp dir"); - let test_file = tmp.path().join("ironclaw_steer_test.txt"); - - let mut trace = LlmTrace::from_file(format!("{FIXTURES}/steering.json")).unwrap(); - trace.replace_paths("/tmp/ironclaw_steer_test.txt", test_file.to_str().unwrap()); + let _cleanup = CleanupGuard::new().file("/tmp/ironclaw_steer_test.txt"); + let _ = std::fs::remove_file("/tmp/ironclaw_steer_test.txt"); + let trace = LlmTrace::from_file(format!("{FIXTURES}/steering.json")).unwrap(); let rig = TestRigBuilder::new() .with_trace(trace.clone()) .build() @@ -68,7 +67,8 @@ mod advanced { assert!(!all_responses[1].is_empty(), "Turn 2: no response"); // Extra: verify file on disk after steering. - let content = std::fs::read_to_string(&test_file).expect("steer test file should exist"); + let content = std::fs::read_to_string("/tmp/ironclaw_steer_test.txt") + .expect("steer test file should exist"); assert_eq!( content, "goodbye", "File should contain 'goodbye' after steering" @@ -91,16 +91,10 @@ mod advanced { #[tokio::test] async fn tool_error_recovery() { - let tmp = tempfile::tempdir().expect("create temp dir"); - let test_file = tmp.path().join("ironclaw_recovery_test.txt"); - - let mut trace = - LlmTrace::from_file(format!("{FIXTURES}/tool_error_recovery.json")).unwrap(); - trace.replace_paths( - "/tmp/ironclaw_recovery_test.txt", - test_file.to_str().unwrap(), - ); + let _cleanup = CleanupGuard::new().file("/tmp/ironclaw_recovery_test.txt"); + let _ = std::fs::remove_file("/tmp/ironclaw_recovery_test.txt"); + let trace = LlmTrace::from_file(format!("{FIXTURES}/tool_error_recovery.json")).unwrap(); let rig = TestRigBuilder::new().with_trace(trace).build().await; rig.send_message("Write 'recovered successfully' to a file for me.") @@ -118,7 +112,8 @@ mod advanced { ); // The second write should have succeeded on disk. - let content = std::fs::read_to_string(&test_file).expect("recovery file should exist"); + let content = std::fs::read_to_string("/tmp/ironclaw_recovery_test.txt") + .expect("recovery file should exist"); assert_eq!(content, "recovered successfully"); // At least one write should have completed with success=true. @@ -137,18 +132,18 @@ mod advanced { #[tokio::test] async fn long_tool_chain() { - let tmp = tempfile::tempdir().expect("create temp dir"); - let test_dir = tmp.path().join("ironclaw_chain_test"); - std::fs::create_dir_all(&test_dir).unwrap(); - - let mut trace = LlmTrace::from_file(format!("{FIXTURES}/long_tool_chain.json")).unwrap(); - trace.replace_paths("/tmp/ironclaw_chain_test", test_dir.to_str().unwrap()); + let test_dir = "/tmp/ironclaw_chain_test"; + let _cleanup = CleanupGuard::new().dir(test_dir); + let _ = std::fs::remove_dir_all(test_dir); + std::fs::create_dir_all(test_dir).unwrap(); + let trace = LlmTrace::from_file(format!("{FIXTURES}/long_tool_chain.json")).unwrap(); let rig = TestRigBuilder::new().with_trace(trace).build().await; rig.send_message( - "Create a daily log, update it with afternoon activities, \ - write an end-of-day summary, then read both files and give me a report.", + "Create a daily log at /tmp/ironclaw_chain_test/log.md, \ + update it with afternoon activities, write an end-of-day summary, \ + then read both files and give me a report.", ) .await; let responses = rig.wait_for_responses(1, TIMEOUT).await; @@ -164,15 +159,16 @@ mod advanced { ); // Verify files on disk. - let log = std::fs::read_to_string(test_dir.join("log.md")).expect("log.md should exist"); + let log = + std::fs::read_to_string(format!("{test_dir}/log.md")).expect("log.md should exist"); assert!( log.contains("Afternoon"), "log.md missing Afternoon section" ); assert!(log.contains("PR #42"), "log.md missing PR #42"); - let summary = - std::fs::read_to_string(test_dir.join("summary.md")).expect("summary.md should exist"); + let summary = std::fs::read_to_string(format!("{test_dir}/summary.md")) + .expect("summary.md should exist"); assert!( summary.contains("accomplishments"), "summary.md missing accomplishments" @@ -394,136 +390,4 @@ mod advanced { rig.verify_trace_expects(&trace, &responses); rig.shutdown(); } - - // ----------------------------------------------------------------------- - // 7. Tool intent nudge — model recovers after nudge - // ----------------------------------------------------------------------- - - #[tokio::test] - async fn tool_intent_nudge_recovery() { - let trace = - LlmTrace::from_file(format!("{FIXTURES}/tool_intent_nudge_recovery.json")).unwrap(); - let rig = TestRigBuilder::new() - .with_trace(trace.clone()) - .build() - .await; - - rig.send_message("Search for the config file.").await; - let responses = rig.wait_for_responses(1, TIMEOUT).await; - - rig.verify_trace_expects(&trace, &responses); - - // The nudge should have caused the model to actually call a tool. - let started = rig.tool_calls_started(); - assert!( - started.iter().any(|s| s == "echo"), - "expected echo tool call after nudge, got: {started:?}" - ); - - // Verify the nudge was injected: the TraceLlm request_hint on step 2 - // requires "tool_calls mechanism" in the last user message. If the hint - // didn't match, TraceLlm logs a warning but doesn't fail -- so also - // check captured requests directly. - let trace_llm = rig.trace_llm().expect("trace_llm should exist"); - assert_eq!( - trace_llm.hint_mismatches(), - 0, - "nudge message should have been injected before the tool-call step" - ); - - rig.shutdown(); - } - - // ----------------------------------------------------------------------- - // 8. Tool intent nudge — caps at 2 nudges - // ----------------------------------------------------------------------- - - #[tokio::test] - async fn tool_intent_nudge_cap() { - let trace = LlmTrace::from_file(format!("{FIXTURES}/tool_intent_nudge_cap.json")).unwrap(); - let rig = TestRigBuilder::new() - .with_trace(trace.clone()) - .build() - .await; - - rig.send_message("Fetch the project data for me.").await; - let responses = rig.wait_for_responses(1, TIMEOUT).await; - - rig.verify_trace_expects(&trace, &responses); - - // Exactly 3 LLM calls: nudge after 1st, nudge after 2nd, 3rd text - // returned as-is (cap of 2 nudges reached). - let trace_llm = rig.trace_llm().expect("trace_llm should exist"); - let captured = trace_llm.captured_requests(); - assert_eq!( - captured.len(), - 3, - "expected exactly 3 LLM calls (2 nudged + 1 returned), got {}", - captured.len() - ); - - // Verify both nudges fired: calls 2 and 3 should have the nudge - // message as the last user message. - for call_idx in [1usize, 2] { - let msgs = &captured[call_idx]; - let last_user = msgs - .iter() - .rev() - .find(|m| matches!(m.role, ironclaw::llm::Role::User)); - assert!( - last_user.is_some_and(|m| m.content.contains("tool_calls mechanism")), - "call {} should have the nudge as last user message", - call_idx + 1 - ); - } - - // No tools should have been called (model never issued tool_calls). - let started = rig.tool_calls_started(); - assert!( - started.is_empty(), - "no tools should be called when model keeps narrating, got: {started:?}" - ); - - rig.shutdown(); - } - - // ----------------------------------------------------------------------- - // 9. Tool intent nudge — no false positive on conversational "let me explain" - // ----------------------------------------------------------------------- - - #[tokio::test] - async fn tool_intent_no_false_positive() { - let trace = - LlmTrace::from_file(format!("{FIXTURES}/tool_intent_no_false_positive.json")).unwrap(); - let rig = TestRigBuilder::new() - .with_trace(trace.clone()) - .build() - .await; - - rig.send_message("How does auth work?").await; - let responses = rig.wait_for_responses(1, TIMEOUT).await; - - rig.verify_trace_expects(&trace, &responses); - - // "Let me explain" should NOT trigger a nudge, so the TraceLlm should - // have been called exactly once (the text response) with no extra nudge - // messages injected. - let trace_llm = rig.trace_llm().expect("trace_llm should exist"); - let captured = trace_llm.captured_requests(); - assert_eq!( - captured.len(), - 1, - "expected exactly 1 LLM call (no nudge), got {}", - captured.len() - ); - - // No tools should have been called. - let started = rig.tool_calls_started(); - assert!( - started.is_empty(), - "no tools should be called for a conversational response, got: {started:?}" - ); - - rig.shutdown(); - } } diff --git a/tests/e2e_metrics_test.rs b/tests/e2e_metrics_test.rs index 15786cc8..5af612c3 100644 --- a/tests/e2e_metrics_test.rs +++ b/tests/e2e_metrics_test.rs @@ -11,10 +11,18 @@ mod tests { use std::time::Duration; use crate::support::assertions::assert_all_tools_succeeded; + use crate::support::cleanup::CleanupGuard; use crate::support::metrics::{RunResult, ScenarioResult, compare_runs}; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; + const TEST_DIR: &str = "/tmp/ironclaw_metrics_test"; + + fn setup_test_dir() { + let _ = std::fs::remove_dir_all(TEST_DIR); + std::fs::create_dir_all(TEST_DIR).expect("failed to create test directory"); + } + /// Verify that metrics are collected from a simple text-only trace. #[tokio::test] async fn test_metrics_collected_from_text_trace() { @@ -78,14 +86,14 @@ mod tests { /// Verify that metrics capture tool calls from a file write/read flow. #[tokio::test] async fn test_metrics_collected_from_tool_trace() { - let tmp = tempfile::tempdir().expect("create temp dir"); + setup_test_dir(); + let _cleanup = CleanupGuard::new().dir(TEST_DIR); - let mut trace = LlmTrace::from_file(concat!( + let trace = LlmTrace::from_file(concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/file_write_read.json" )) .expect("failed to load file_write_read.json"); - trace.replace_paths("/tmp/ironclaw_e2e_test", tmp.path().to_str().unwrap()); let rig = TestRigBuilder::new().with_trace(trace).build().await; diff --git a/tests/e2e_spot_checks.rs b/tests/e2e_spot_checks.rs index a6ef6dbb..5723f73b 100644 --- a/tests/e2e_spot_checks.rs +++ b/tests/e2e_spot_checks.rs @@ -11,6 +11,7 @@ mod support; mod spot_tests { use std::time::Duration; + use crate::support::cleanup::CleanupGuard; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; @@ -97,12 +98,10 @@ mod spot_tests { #[tokio::test] async fn spot_chain_write_read() { - let tmp = tempfile::tempdir().unwrap(); - let test_file = tmp.path().join("ironclaw_spot_test.txt"); - - let mut trace = LlmTrace::from_file(format!("{FIXTURES}/chain_write_read.json")).unwrap(); - trace.replace_paths("/tmp/ironclaw_spot_test.txt", test_file.to_str().unwrap()); + let _cleanup = CleanupGuard::new().file("/tmp/ironclaw_spot_test.txt"); + let _ = std::fs::remove_file("/tmp/ironclaw_spot_test.txt"); + let trace = LlmTrace::from_file(format!("{FIXTURES}/chain_write_read.json")).unwrap(); let rig = TestRigBuilder::new() .with_trace(trace.clone()) .build() @@ -118,7 +117,8 @@ mod spot_tests { rig.verify_trace_expects(&trace, &responses); // Extra: verify file on disk (can't express in expects). - let content = std::fs::read_to_string(&test_file).expect("file should exist"); + let content = + std::fs::read_to_string("/tmp/ironclaw_spot_test.txt").expect("file should exist"); assert_eq!(content, "ironclaw spot check"); rig.shutdown(); @@ -166,12 +166,10 @@ mod spot_tests { #[tokio::test] async fn spot_memory_save_recall() { - let tmp = tempfile::tempdir().unwrap(); - let test_file = tmp.path().join("bench-meeting.md"); - - let mut trace = LlmTrace::from_file(format!("{FIXTURES}/memory_save_recall.json")).unwrap(); - trace.replace_paths("/tmp/bench-meeting.md", test_file.to_str().unwrap()); + let _cleanup = CleanupGuard::new().file("/tmp/bench-meeting.md"); + let _ = std::fs::remove_file("/tmp/bench-meeting.md"); + let trace = LlmTrace::from_file(format!("{FIXTURES}/memory_save_recall.json")).unwrap(); let rig = TestRigBuilder::new() .with_trace(trace.clone()) .build() diff --git a/tests/e2e_tool_coverage.rs b/tests/e2e_tool_coverage.rs index 4d390916..be460f3a 100644 --- a/tests/e2e_tool_coverage.rs +++ b/tests/e2e_tool_coverage.rs @@ -10,9 +10,19 @@ mod support; mod tests { use std::time::Duration; + use crate::support::cleanup::CleanupGuard; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; + const TEST_DIR_BASE: &str = "/tmp/ironclaw_coverage_test"; + + fn setup_test_dir(suffix: &str) -> String { + let dir = format!("{TEST_DIR_BASE}_{suffix}"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("failed to create test directory"); + dir + } + // ----------------------------------------------------------------------- // json tool // ----------------------------------------------------------------------- @@ -84,21 +94,16 @@ mod tests { #[tokio::test] async fn test_list_dir() { - let tmp = tempfile::tempdir().expect("failed to create tempdir"); - let test_dir = tmp.path().join("test_dir"); - std::fs::create_dir_all(&test_dir).unwrap(); - std::fs::write(test_dir.join("file_a.txt"), "content a").unwrap(); - std::fs::write(test_dir.join("file_b.txt"), "content b").unwrap(); + let test_dir = setup_test_dir("list_dir"); + let _cleanup = CleanupGuard::new().dir(&test_dir); + std::fs::write(format!("{test_dir}/file_a.txt"), "content a").unwrap(); + std::fs::write(format!("{test_dir}/file_b.txt"), "content b").unwrap(); - let mut trace = LlmTrace::from_file(concat!( + let trace = LlmTrace::from_file(concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/coverage/list_dir.json" )) .expect("failed to load list_dir.json"); - trace.replace_paths( - "/tmp/ironclaw_coverage_test_list_dir", - test_dir.to_str().unwrap(), - ); let rig = TestRigBuilder::new() .with_trace(trace.clone()) @@ -118,19 +123,14 @@ mod tests { #[tokio::test] async fn test_apply_patch_chain() { - let tmp = tempfile::tempdir().expect("failed to create tempdir"); - let test_dir = tmp.path().join("test_dir"); - std::fs::create_dir_all(&test_dir).unwrap(); + let test_dir = setup_test_dir("apply_patch"); + let _cleanup = CleanupGuard::new().dir(&test_dir); - let mut trace = LlmTrace::from_file(concat!( + let trace = LlmTrace::from_file(concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/coverage/apply_patch_chain.json" )) .expect("failed to load apply_patch_chain.json"); - trace.replace_paths( - "/tmp/ironclaw_coverage_test_apply_patch", - test_dir.to_str().unwrap(), - ); let rig = TestRigBuilder::new() .with_trace(trace.clone()) @@ -143,7 +143,7 @@ mod tests { rig.verify_trace_expects(&trace, &responses); // Extra: verify the patch was applied on disk. - let content = std::fs::read_to_string(test_dir.join("patch_target.txt")) + let content = std::fs::read_to_string(format!("{test_dir}/patch_target.txt")) .expect("patch_target.txt should exist"); assert!( content.contains("PATCHED"), diff --git a/tests/e2e_trace_file_tools.rs b/tests/e2e_trace_file_tools.rs index 2cf6bab7..f6f96b4e 100644 --- a/tests/e2e_trace_file_tools.rs +++ b/tests/e2e_trace_file_tools.rs @@ -8,21 +8,29 @@ mod support; mod tests { use std::time::Duration; + use crate::support::cleanup::CleanupGuard; use crate::support::test_rig::TestRigBuilder; use crate::support::trace_llm::LlmTrace; + const TEST_DIR: &str = "/tmp/ironclaw_e2e_test"; + const TEST_FILE: &str = "/tmp/ironclaw_e2e_test/hello.txt"; const EXPECTED_CONTENT: &str = "Hello, E2E test!"; + fn setup_test_dir() { + let _ = std::fs::remove_dir_all(TEST_DIR); + std::fs::create_dir_all(TEST_DIR).expect("failed to create test directory"); + } + #[tokio::test] async fn test_file_write_and_read_flow() { - let tmp = tempfile::tempdir().expect("create temp dir"); + setup_test_dir(); + let _cleanup = CleanupGuard::new().dir(TEST_DIR); let fixture_path = concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/file_write_read.json" ); - let mut trace = LlmTrace::from_file(fixture_path).expect("failed to load trace fixture"); - trace.replace_paths("/tmp/ironclaw_e2e_test", tmp.path().to_str().unwrap()); + let trace = LlmTrace::from_file(fixture_path).expect("failed to load trace fixture"); let rig = TestRigBuilder::new() .with_trace(trace.clone()) @@ -36,8 +44,8 @@ mod tests { rig.verify_trace_expects(&trace, &responses); // Extra: verify file on disk (can't express in expects). - let file_content = std::fs::read_to_string(tmp.path().join("hello.txt")) - .expect("hello.txt should exist after write_file"); + let file_content = + std::fs::read_to_string(TEST_FILE).expect("hello.txt should exist after write_file"); assert_eq!(file_content, EXPECTED_CONTENT); rig.shutdown(); diff --git a/tests/e2e_worker_coverage.rs b/tests/e2e_worker_coverage.rs index 005d21c3..a2d3988c 100644 --- a/tests/e2e_worker_coverage.rs +++ b/tests/e2e_worker_coverage.rs @@ -91,17 +91,22 @@ mod tests { #[tokio::test] async fn tool_error_feedback() { + // Use a tempdir for the recovery file. The fixture's recovery path + // is updated to write here via the test_dir variable. let tmp = tempfile::tempdir().expect("create temp dir"); + let test_dir = tmp.path().to_str().expect("tempdir path"); - let mut trace = LlmTrace::from_file(concat!( + // Patch the fixture's recovery path to use our tempdir. + let fixture_str = std::fs::read_to_string(concat!( env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/llm_traces/worker/tool_error_feedback.json" )) - .expect("failed to load tool_error_feedback.json"); - trace.replace_paths( - "/tmp/ironclaw_error_feedback_test", - tmp.path().to_str().unwrap(), + .expect("read fixture"); + let fixture_str = fixture_str.replace( + "/tmp/ironclaw_error_feedback_test/recovered.txt", + &format!("{test_dir}/recovered.txt"), ); + let trace: LlmTrace = serde_json::from_str(&fixture_str).expect("parse patched fixture"); let rig = TestRigBuilder::new() .with_trace(trace.clone()) @@ -115,7 +120,7 @@ mod tests { rig.verify_trace_expects(&trace, &responses); // Verify the recovery file exists in the tempdir. - let content = std::fs::read_to_string(tmp.path().join("recovered.txt")) + let content = std::fs::read_to_string(format!("{test_dir}/recovered.txt")) .expect("recovered.txt should exist"); assert!( content.contains("recovered"), diff --git a/tests/heartbeat_integration.rs b/tests/heartbeat_integration.rs index f609b769..227f59f9 100644 --- a/tests/heartbeat_integration.rs +++ b/tests/heartbeat_integration.rs @@ -1,4 +1,4 @@ -#![cfg(all(feature = "postgres", feature = "integration"))] +#![cfg(feature = "postgres")] //! Heartbeat integration test. //! //! Exercises the heartbeat system in isolation: connects to the real diff --git a/tests/openai_compat_integration.rs b/tests/openai_compat_integration.rs index b4fc3b12..d9dd3745 100644 --- a/tests/openai_compat_integration.rs +++ b/tests/openai_compat_integration.rs @@ -9,8 +9,9 @@ use std::time::Duration; use async_trait::async_trait; use rust_decimal::Decimal; -use ironclaw::channels::web::server::GatewayState; -use ironclaw::channels::web::test_helpers::TestGatewayBuilder; +use ironclaw::channels::web::server::{GatewayState, start_server}; +use ironclaw::channels::web::sse::SseManager; +use ironclaw::channels::web::ws::WsConnectionTracker; use ironclaw::error::LlmError; use ironclaw::llm::{ CompletionRequest, CompletionResponse, FinishReason, LlmProvider, ToolCompletionRequest, @@ -188,11 +189,37 @@ async fn start_test_server() -> (SocketAddr, Arc, Arc, ) -> (SocketAddr, Arc) { - TestGatewayBuilder::new() - .llm_provider(llm_provider) - .start(AUTH_TOKEN) + let state = Arc::new(GatewayState { + msg_tx: tokio::sync::RwLock::new(None), + sse: SseManager::new(), + workspace: None, + session_manager: None, + log_broadcaster: None, + log_level_handle: None, + extension_manager: None, + tool_registry: None, + store: None, + job_manager: None, + prompt_queue: None, + scheduler: None, + user_id: "test-user".to_string(), + shutdown_tx: tokio::sync::RwLock::new(None), + ws_tracker: Some(Arc::new(WsConnectionTracker::new())), + llm_provider: Some(llm_provider), + skill_registry: None, + skill_catalog: None, + chat_rate_limiter: ironclaw::channels::web::server::RateLimiter::new(30, 60), + registry_entries: Vec::new(), + cost_guard: None, + startup_time: std::time::Instant::now(), + }); + + let addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + let bound_addr = start_server(addr, state.clone(), AUTH_TOKEN.to_string()) .await - .expect("Failed to start test server") + .expect("Failed to start test server"); + + (bound_addr, state) } fn client() -> reqwest::Client { @@ -651,10 +678,35 @@ async fn test_models_no_auth() { #[tokio::test] async fn test_no_llm_provider_returns_503() { // Create state WITHOUT llm_provider - let (bound_addr, _state) = TestGatewayBuilder::new() - .start(AUTH_TOKEN) + let state = Arc::new(GatewayState { + msg_tx: tokio::sync::RwLock::new(None), + sse: SseManager::new(), + workspace: None, + session_manager: None, + log_broadcaster: None, + log_level_handle: None, + extension_manager: None, + tool_registry: None, + store: None, + job_manager: None, + prompt_queue: None, + scheduler: None, + user_id: "test-user".to_string(), + shutdown_tx: tokio::sync::RwLock::new(None), + ws_tracker: Some(Arc::new(WsConnectionTracker::new())), + llm_provider: None, // No LLM! + skill_registry: None, + skill_catalog: None, + chat_rate_limiter: ironclaw::channels::web::server::RateLimiter::new(30, 60), + registry_entries: Vec::new(), + cost_guard: None, + startup_time: std::time::Instant::now(), + }); + + let addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + let bound_addr = start_server(addr, state, AUTH_TOKEN.to_string()) .await - .expect("Failed to start test server"); + .unwrap(); let url = format!("http://{}/v1/chat/completions", bound_addr); let resp = client() diff --git a/tests/support/trace_llm.rs b/tests/support/trace_llm.rs index 0559ab5b..804e8eab 100644 --- a/tests/support/trace_llm.rs +++ b/tests/support/trace_llm.rs @@ -236,38 +236,6 @@ impl LlmTrace { Ok(trace) } - /// Replace all occurrences of `old` with `new` in tool call arguments, - /// text content, and user input throughout the trace. - /// - /// Used to substitute hardcoded fixture paths (e.g. `/tmp/ironclaw_test`) - /// with dynamic `tempfile::tempdir()` paths so tests don't collide. - pub fn replace_paths(&mut self, old: &str, new: &str) { - for turn in &mut self.turns { - if turn.user_input.contains(old) { - turn.user_input = turn.user_input.replace(old, new); - } - for step in &mut turn.steps { - match &mut step.response { - TraceResponse::ToolCalls { tool_calls, .. } => { - for tc in tool_calls { - replace_in_json_value(&mut tc.arguments, old, new); - } - } - TraceResponse::Text { content, .. } => { - if content.contains(old) { - *content = content.replace(old, new); - } - } - TraceResponse::UserInput { content } => { - if content.contains(old) { - *content = content.replace(old, new); - } - } - } - } - } - } - /// Return only the playable steps from the raw steps (text + tool_calls), /// skipping `user_input` markers. Only meaningful for recorded traces that /// were deserialized from a flat `steps` array. @@ -280,28 +248,6 @@ impl LlmTrace { } } -/// Recursively replace `old` with `new` in all string values within a JSON tree. -fn replace_in_json_value(value: &mut serde_json::Value, old: &str, new: &str) { - match value { - serde_json::Value::String(s) => { - if s.contains(old) { - *s = s.replace(old, new); - } - } - serde_json::Value::Object(map) => { - for v in map.values_mut() { - replace_in_json_value(v, old, new); - } - } - serde_json::Value::Array(arr) => { - for v in arr { - replace_in_json_value(v, old, new); - } - } - _ => {} - } -} - // --------------------------------------------------------------------------- // TraceLlm provider // --------------------------------------------------------------------------- diff --git a/tests/support_unit_tests.rs b/tests/support_unit_tests.rs index 76ef5d6d..645746ea 100644 --- a/tests/support_unit_tests.rs +++ b/tests/support_unit_tests.rs @@ -96,46 +96,42 @@ mod cleanup_tests { #[test] fn cleanup_guard_removes_file() { - let tmp = tempfile::tempdir().unwrap(); - let path = tmp.path().join("cleanup_guard_test.txt"); - std::fs::write(&path, "test").unwrap(); - let path_str = path.to_str().unwrap().to_string(); + let path = "/tmp/ironclaw_cleanup_guard_test.txt"; + std::fs::write(path, "test").unwrap(); { - let _guard = CleanupGuard::new().file(path_str); - assert!(path.exists()); + let _guard = CleanupGuard::new().file(path); + assert!(std::path::Path::new(path).exists()); } - assert!(!path.exists()); + assert!(!std::path::Path::new(path).exists()); } #[test] fn cleanup_guard_removes_dir() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("cleanup_guard_test_dir"); - std::fs::create_dir_all(&dir).unwrap(); - std::fs::write(dir.join("file.txt"), "test").unwrap(); - let dir_str = dir.to_str().unwrap().to_string(); + let dir = "/tmp/ironclaw_cleanup_guard_test_dir"; + std::fs::create_dir_all(dir).unwrap(); + std::fs::write(format!("{dir}/file.txt"), "test").unwrap(); { - let _guard = CleanupGuard::new().dir(dir_str); - assert!(dir.exists()); + let _guard = CleanupGuard::new().dir(dir); + assert!(std::path::Path::new(dir).exists()); } - assert!(!dir.exists()); + assert!(!std::path::Path::new(dir).exists()); } #[test] fn cleanup_guard_file_does_not_remove_dir() { - let tmp = tempfile::tempdir().unwrap(); - let dir = tmp.path().join("cleanup_guard_file_not_dir"); - std::fs::create_dir_all(&dir).unwrap(); - let dir_str = dir.to_str().unwrap().to_string(); + let dir = "/tmp/ironclaw_cleanup_guard_file_not_dir"; + std::fs::create_dir_all(dir).unwrap(); { // Registering a directory path as .file() should not remove it // (remove_file fails on directories). - let _guard = CleanupGuard::new().file(dir_str); + let _guard = CleanupGuard::new().file(dir); } assert!( - dir.exists(), + std::path::Path::new(dir).exists(), "dir should still exist when registered as file" ); + // Clean up manually. + let _ = std::fs::remove_dir_all(dir); } } diff --git a/tests/workspace_integration.rs b/tests/workspace_integration.rs index 54882ec1..dddd95e9 100644 --- a/tests/workspace_integration.rs +++ b/tests/workspace_integration.rs @@ -1,4 +1,4 @@ -#![cfg(all(feature = "postgres", feature = "integration"))] +#![cfg(feature = "postgres")] //! Integration tests for the workspace module. //! //! Requires a running PostgreSQL with pgvector extension. @@ -21,6 +21,18 @@ fn get_pool() -> deadpool_postgres::Pool { .expect("Failed to create pool") } +/// Try to get a connection, returning None if Postgres is unreachable. +/// Tests call this to skip gracefully in CI where no database is available. +async fn try_connect(pool: &deadpool_postgres::Pool) -> Option<()> { + match pool.get().await { + Ok(_) => Some(()), + Err(e) => { + eprintln!("skipping: database unavailable ({e})"); + None + } + } +} + async fn cleanup_user(pool: &deadpool_postgres::Pool, user_id: &str) { let conn = pool.get().await.expect("Failed to get connection"); conn.execute( @@ -34,6 +46,9 @@ async fn cleanup_user(pool: &deadpool_postgres::Pool, user_id: &str) { #[tokio::test] async fn test_workspace_write_and_read() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_write_read"; cleanup_user(&pool, user_id).await; @@ -59,6 +74,9 @@ async fn test_workspace_write_and_read() { #[tokio::test] async fn test_workspace_append() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_append"; cleanup_user(&pool, user_id).await; @@ -86,6 +104,9 @@ async fn test_workspace_append() { #[tokio::test] async fn test_workspace_nested_paths() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_nested"; cleanup_user(&pool, user_id).await; @@ -131,6 +152,9 @@ async fn test_workspace_nested_paths() { #[tokio::test] async fn test_workspace_delete() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_delete"; cleanup_user(&pool, user_id).await; @@ -155,6 +179,9 @@ async fn test_workspace_delete() { #[tokio::test] async fn test_workspace_memory_operations() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_memory_ops"; cleanup_user(&pool, user_id).await; @@ -183,6 +210,9 @@ async fn test_workspace_memory_operations() { #[tokio::test] async fn test_workspace_daily_log() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_daily_log"; cleanup_user(&pool, user_id).await; @@ -209,6 +239,9 @@ async fn test_workspace_daily_log() { #[tokio::test] async fn test_workspace_fts_search() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_fts_search"; cleanup_user(&pool, user_id).await; @@ -267,6 +300,9 @@ async fn test_workspace_fts_search() { #[tokio::test] async fn test_workspace_hybrid_search_with_mock_embeddings() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_hybrid_search"; cleanup_user(&pool, user_id).await; @@ -306,6 +342,9 @@ async fn test_workspace_hybrid_search_with_mock_embeddings() { #[tokio::test] async fn test_workspace_list_all() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_list_all"; cleanup_user(&pool, user_id).await; @@ -331,6 +370,9 @@ async fn test_workspace_list_all() { #[tokio::test] async fn test_workspace_system_prompt() { let pool = get_pool(); + if try_connect(&pool).await.is_none() { + return; + } let user_id = "test_system_prompt"; cleanup_user(&pool, user_id).await; diff --git a/tests/ws_gateway_integration.rs b/tests/ws_gateway_integration.rs index e95f7a23..0016ba4e 100644 --- a/tests/ws_gateway_integration.rs +++ b/tests/ws_gateway_integration.rs @@ -20,9 +20,10 @@ use tokio_tungstenite::tungstenite::Message; use tokio_tungstenite::tungstenite::client::IntoClientRequest; use ironclaw::channels::IncomingMessage; -use ironclaw::channels::web::server::GatewayState; -use ironclaw::channels::web::test_helpers::TestGatewayBuilder; +use ironclaw::channels::web::server::{GatewayState, start_server}; +use ironclaw::channels::web::sse::SseManager; use ironclaw::channels::web::types::SseEvent; +use ironclaw::channels::web::ws::WsConnectionTracker; const AUTH_TOKEN: &str = "test-token-12345"; const TIMEOUT: Duration = Duration::from_secs(5); @@ -36,13 +37,37 @@ async fn start_test_server() -> ( ) { let (agent_tx, agent_rx) = mpsc::channel(64); - let (addr, state) = TestGatewayBuilder::new() - .msg_tx(agent_tx) - .start(AUTH_TOKEN) + let state = Arc::new(GatewayState { + msg_tx: tokio::sync::RwLock::new(Some(agent_tx)), + sse: SseManager::new(), + workspace: None, + session_manager: None, + log_broadcaster: None, + log_level_handle: None, + extension_manager: None, + tool_registry: None, + store: None, + job_manager: None, + prompt_queue: None, + scheduler: None, + user_id: "test-user".to_string(), + shutdown_tx: tokio::sync::RwLock::new(None), + ws_tracker: Some(Arc::new(WsConnectionTracker::new())), + llm_provider: None, + skill_registry: None, + skill_catalog: None, + chat_rate_limiter: ironclaw::channels::web::server::RateLimiter::new(30, 60), + registry_entries: Vec::new(), + cost_guard: None, + startup_time: std::time::Instant::now(), + }); + + let addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + let bound_addr = start_server(addr, state.clone(), AUTH_TOKEN.to_string()) .await .expect("Failed to start test server"); - (addr, state, agent_rx) + (bound_addr, state, agent_rx) } /// Connect a WebSocket client with auth token in query parameter.