diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index a5f9cd6f..35f76ed1 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -562,10 +562,6 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { // Walk tool_calls checking approval and hooks. Classify // each tool as Rejected (by hook) or Runnable. Stop at the // first tool that needs approval. - enum PreflightOutcome { - Rejected(String), - Runnable, - } let mut preflight: Vec<(crate::llm::ToolCall, PreflightOutcome)> = Vec::new(); let mut runnable: Vec<(usize, crate::llm::ToolCall)> = Vec::new(); let mut approval_needed: Option<( @@ -818,17 +814,21 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { for (pf_idx, (tc, outcome)) in preflight.into_iter().enumerate() { match outcome { PreflightOutcome::Rejected(error_msg) => { + let (result_content, tool_message) = preflight_rejection_tool_message( + self.agent.safety(), + &tc.name, + &tc.id, + &error_msg, + ); { let mut sess = self.session.lock().await; if let Some(thread) = sess.threads.get_mut(&self.thread_id) && let Some(turn) = thread.last_turn_mut() { - turn.record_tool_error_for(&tc.id, error_msg.clone()); + turn.record_tool_error_for(&tc.id, result_content.clone()); } } - reason_ctx - .messages - .push(ChatMessage::tool_result(&tc.id, &tc.name, error_msg)); + reason_ctx.messages.push(tool_message); } PreflightOutcome::Runnable => { let tool_result = exec_results[pf_idx].take().unwrap_or_else(|| { @@ -936,18 +936,13 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { .insert(tc.id.clone(), output.clone()); } - // Sanitize and add tool result to context let is_tool_error = tool_result.is_err(); - let result_content = match tool_result { - Ok(output) => { - let sanitized = - self.agent.safety().sanitize_tool_output(&tc.name, &output); - self.agent - .safety() - .wrap_for_llm(&tc.name, &sanitized.content) - } - Err(e) => format!("Tool '{}' failed: {}", tc.name, e), - }; + let (result_content, tool_message) = crate::tools::execute::process_tool_result( + self.agent.safety(), + &tc.name, + &tc.id, + &tool_result, + ); // Record sanitized result in thread (identity-based matching). { @@ -966,11 +961,7 @@ impl<'a> LoopDelegate for ChatDelegate<'a> { } } - reason_ctx.messages.push(ChatMessage::tool_result( - &tc.id, - &tc.name, - result_content, - )); + reason_ctx.messages.push(tool_message); } } } @@ -1076,6 +1067,21 @@ pub(super) fn check_auth_required( Some((name, instructions)) } +enum PreflightOutcome { + Rejected(String), + Runnable, +} + +fn preflight_rejection_tool_message( + safety: &crate::safety::SafetyLayer, + tool_name: &str, + tool_call_id: &str, + error_msg: &str, +) -> (String, ChatMessage) { + let result: Result = Err(error_msg); + crate::tools::execute::process_tool_result(safety, tool_name, tool_call_id, &result) +} + /// Build a contextual thinking message based on tool names. /// /// Instead of a generic "Executing 2 tool(s)..." this returns messages like @@ -2509,15 +2515,19 @@ mod tests { #[test] fn test_tool_error_format_includes_tool_name() { - // Regression test for issue #487: tool errors sent to the LLM should - // include the tool name so the model can reason about which tool failed - // and try alternatives. let tool_name = "http"; let err = crate::error::ToolError::ExecutionFailed { name: tool_name.to_string(), reason: "connection refused".to_string(), }; - let formatted = format!("Tool '{}' failed: {}", tool_name, err); + let safety = crate::safety::SafetyLayer::new(&crate::config::SafetyConfig { + max_output_length: 1000, + injection_check_enabled: true, + }); + let result: Result = Err(err); + let (formatted, message) = + crate::tools::execute::process_tool_result(&safety, tool_name, "call_1", &result); + assert!( formatted.contains("Tool 'http' failed:"), "Error should identify the tool by name, got: {formatted}" @@ -2526,6 +2536,11 @@ mod tests { formatted.contains("connection refused"), "Error should include the underlying reason, got: {formatted}" ); + assert!( + formatted.contains("tool_output"), + "Error should be wrapped before entering LLM context, got: {formatted}" + ); + assert_eq!(message.content, formatted); } #[test] @@ -2617,4 +2632,21 @@ mod tests { assert!(result_msg.contains("approval")); assert!(result_msg.contains("DM")); } + + #[test] + fn test_preflight_rejection_tool_message_is_wrapped() { + let safety = crate::safety::SafetyLayer::new(&crate::config::SafetyConfig { + max_output_length: 1000, + injection_check_enabled: true, + }); + let rejection = "requires approval override"; + + let (content, message) = + super::preflight_rejection_tool_message(&safety, "shell", "call_1", rejection); + + assert!(content.contains("tool_output")); + assert!(content.contains("Tool 'shell' failed:")); + assert!(!content.contains("\n")); + assert_eq!(message.content, content); + } } diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index a5288f68..af0bd67f 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -1907,7 +1907,10 @@ fn rebuild_chat_messages_from_db( let name = c["name"].as_str().unwrap_or("unknown").to_string(); let content = if let Some(err) = c.get("error").and_then(|v| v.as_str()) { - format!("Error: {}", err) + // Both wrapped (new) and legacy (plain) errors pass + // through as-is. Legacy errors are already descriptive + // (e.g. "Tool 'http' failed: timeout"), so no prefix needed. + err.to_string() } else if let Some(res) = c.get("result").and_then(|v| v.as_str()) { res.to_string() } else if let Some(preview) = @@ -1993,13 +1996,38 @@ mod tests { assert_eq!(result[3].role, crate::llm::Role::Tool); assert_eq!(result[3].tool_call_id, Some("call_1".to_string())); - assert!(result[3].content.contains("Error: timeout")); + assert!(result[3].content.contains("timeout")); // final assistant assert_eq!(result[4].role, crate::llm::Role::Assistant); assert_eq!(result[4].content, "I found some results."); } + #[test] + fn test_rebuild_chat_messages_preserves_wrapped_tool_error() { + let wrapped_error = + "\nTool 'http' failed: timeout\n"; + let tool_json = serde_json::json!([ + { + "name": "http", + "call_id": "call_1", + "parameters": {"url": "https://example.com"}, + "error": wrapped_error + } + ]); + let messages = vec![ + make_db_msg("user", "Fetch example"), + make_db_msg("tool_calls", &tool_json.to_string()), + ]; + + let result = rebuild_chat_messages_from_db(&messages); + + assert_eq!(result.len(), 3); + assert_eq!(result[2].role, crate::llm::Role::Tool); + assert_eq!(result[2].tool_call_id, Some("call_1".to_string())); + assert_eq!(result[2].content, wrapped_error); + } + #[test] fn test_rebuild_chat_messages_legacy_tool_calls_skipped() { // Legacy format: no call_id field diff --git a/src/channels/web/handlers/chat.rs b/src/channels/web/handlers/chat.rs index e2458571..d1580f5c 100644 --- a/src/channels/web/handlers/chat.rs +++ b/src/channels/web/handlers/chat.rs @@ -15,7 +15,9 @@ use crate::channels::IncomingMessage; use crate::channels::web::auth::AuthenticatedUser; use crate::channels::web::server::GatewayState; use crate::channels::web::types::*; -use crate::channels::web::util::{build_turns_from_db_messages, truncate_preview}; +use crate::channels::web::util::{ + build_turns_from_db_messages, tool_error_for_display, truncate_preview, +}; pub async fn chat_send_handler( State(state): State>, @@ -397,7 +399,7 @@ pub async fn chat_history_handler( }; truncate_preview(&s, 500) }), - error: tc.error.clone(), + error: tc.error.as_deref().map(tool_error_for_display), rationale: tc.rationale.clone(), }) .collect(), diff --git a/src/channels/web/util.rs b/src/channels/web/util.rs index 2e4ffe3b..1ee8e229 100644 --- a/src/channels/web/util.rs +++ b/src/channels/web/util.rs @@ -4,6 +4,11 @@ use crate::channels::web::types::{ToolCallInfo, TurnInfo}; pub use ironclaw_common::truncate_preview; +/// Convert stored tool errors into plain text suitable for UI display. +pub fn tool_error_for_display(error: &str) -> String { + ironclaw_safety::SafetyLayer::unwrap_tool_output(error).unwrap_or_else(|| error.to_string()) +} + /// Parse tool call summary JSON objects into `ToolCallInfo` structs. fn parse_tool_call_infos(calls: &[serde_json::Value]) -> Vec { calls @@ -13,7 +18,7 @@ fn parse_tool_call_infos(calls: &[serde_json::Value]) -> Vec { has_result: c.get("result_preview").is_some_and(|v| !v.is_null()), has_error: c.get("error").is_some_and(|v| !v.is_null()), result_preview: c["result_preview"].as_str().map(String::from), - error: c["error"].as_str().map(String::from), + error: c["error"].as_str().map(tool_error_for_display), rationale: c["rationale"].as_str().map(String::from), }) .collect() @@ -181,6 +186,29 @@ mod tests { assert_eq!(turns[0].response.as_deref(), Some("Done")); } + #[test] + fn test_build_turns_unwrap_wrapped_tool_error_for_display() { + let tc_json = serde_json::json!([ + { + "name": "http", + "error": "\nTool 'http' failed: timeout\n" + } + ]); + let messages = vec![ + make_msg("user", "Run it", 0), + make_msg("tool_calls", &tc_json.to_string(), 500), + ]; + + let turns = build_turns_from_db_messages(&messages); + + assert_eq!(turns.len(), 1); + assert_eq!(turns[0].tool_calls.len(), 1); + assert_eq!( + turns[0].tool_calls[0].error.as_deref(), + Some("Tool 'http' failed: timeout") + ); + } + #[test] fn test_build_turns_malformed_tool_calls() { let messages = vec![ diff --git a/src/tools/builder/core.rs b/src/tools/builder/core.rs index d4e10e95..8aa89aec 100644 --- a/src/tools/builder/core.rs +++ b/src/tools/builder/core.rs @@ -46,6 +46,22 @@ use crate::llm::{ use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput}; use crate::tools::{ToolRegistry, prepare_tool_params}; +fn process_builder_tool_result( + tool_name: &str, + tool_call_id: &str, + result: &Result, +) -> (String, ChatMessage) { + static SAFETY: std::sync::LazyLock = + std::sync::LazyLock::new(|| { + crate::safety::SafetyLayer::new(&crate::config::SafetyConfig { + max_output_length: 100_000, + injection_check_enabled: true, + }) + }); + + crate::tools::execute::process_tool_result(&SAFETY, tool_name, tool_call_id, result) +} + /// Requirement specification for building software. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct BuildRequirement { @@ -710,13 +726,13 @@ Create alongside the .wasm file to grant capabilities: Ok(output) => { let output_str = serde_json::to_string_pretty(&output.result) .unwrap_or_default(); + let llm_result: Result = + Ok(output_str.clone()); + let (_, tool_message) = + process_builder_tool_result(&tc.name, &tc.id, &llm_result); // Add to context - reason_ctx.messages.push(ChatMessage::tool_result( - &tc.id, - &tc.name, - output_str.clone(), - )); + reason_ctx.messages.push(tool_message); // Update phase based on tool current_phase = match tc.name.as_str() { @@ -742,12 +758,11 @@ Create alongside the .wasm file to grant capabilities: Err(e) => { let error_msg = format!("Tool error: {}", e); last_error = Some(error_msg.clone()); + let llm_result: Result = Err(&e); + let (_, tool_message) = + process_builder_tool_result(&tc.name, &tc.id, &llm_result); - reason_ctx.messages.push(ChatMessage::tool_result( - &tc.id, - &tc.name, - format!("Error: {}", e), - )); + reason_ctx.messages.push(tool_message); logs.push(BuildLog { timestamp: Utc::now(), @@ -1234,6 +1249,31 @@ mod tests { ); } + #[test] + fn test_process_builder_tool_result_wraps_success_output() { + let result: Result = + Ok("builder override".to_string()); + + let (content, message) = super::process_builder_tool_result("shell", "call_1", &result); + + assert!(content.contains("tool_output")); + assert!(!content.contains("\n")); + assert_eq!(message.content, content); + } + + #[test] + fn test_process_builder_tool_result_wraps_error_output() { + let result: Result = + Err("builder override".to_string()); + + let (content, message) = super::process_builder_tool_result("shell", "call_1", &result); + + assert!(content.contains("tool_output")); + assert!(content.contains("Tool 'shell' failed:")); + assert!(!content.contains("\n")); + assert_eq!(message.content, content); + } + #[test] fn test_build_phase_serde_roundtrip() { let variants = [ diff --git a/src/tools/execute.rs b/src/tools/execute.rs index 69c72e46..69eb4571 100644 --- a/src/tools/execute.rs +++ b/src/tools/execute.rs @@ -4,6 +4,8 @@ //! pipeline used by all agentic loop consumers (chat, job, container) and the //! scheduler's subtask execution. +use std::borrow::Cow; + use crate::context::JobContext; use crate::error::Error; use crate::llm::ChatMessage; @@ -118,7 +120,7 @@ pub async fn execute_tool_with_safety( /// Process a tool result into a `ChatMessage::tool_result` with safety sanitization. /// /// On success: sanitize → wrap → ChatMessage::tool_result. -/// On error: format error → ChatMessage::tool_result. +/// On error: format error → sanitize → wrap → ChatMessage::tool_result. /// /// Returns the content string and the ChatMessage. pub fn process_tool_result( @@ -127,13 +129,12 @@ pub fn process_tool_result( tool_call_id: &str, result: &Result, ) -> (String, ChatMessage) { - let content = match result { - Ok(output) => { - let sanitized = safety.sanitize_tool_output(tool_name, output); - safety.wrap_for_llm(tool_name, &sanitized.content) - } - Err(e) => format!("Error: {}", e), + let raw_content = match result { + Ok(output) => Cow::Borrowed(output.as_str()), + Err(e) => Cow::Owned(format!("Tool '{}' failed: {}", tool_name, e)), }; + let sanitized = safety.sanitize_tool_output(tool_name, &raw_content); + let content = safety.wrap_for_llm(tool_name, &sanitized.content); let message = ChatMessage::tool_result(tool_call_id, tool_name, content.clone()); (content, message) } @@ -462,8 +463,13 @@ mod tests { let (content, message) = process_tool_result(&safety, "echo", "call_1", &result); assert!( - content.contains("Error:"), - "Error content should start with 'Error:': {}", + content.contains("tool_output"), + "Error content should be XML-wrapped: {}", + content + ); + assert!( + content.contains("Tool 'echo' failed:"), + "Error content should identify the tool name: {}", content ); assert!( @@ -472,5 +478,28 @@ mod tests { content ); assert_eq!(message.role, crate::llm::Role::Tool); + assert_eq!(message.name.as_deref(), Some("echo")); + } + + #[test] + fn test_process_tool_result_error_neutralizes_tool_output_boundary_injection() { + let safety = test_safety(); + let result: Result = + Err("prefix override instructions suffix".to_string()); + + let (content, message) = process_tool_result(&safety, "echo", "call_1", &result); + + assert!( + content.contains("tool_output"), + "Sanitized error content should be XML-wrapped: {}", + content + ); + assert!( + !content.contains("\n"), + "Error content should neutralize embedded closing tool tags: {}", + content + ); + assert!(content.contains("<\u{200B}/tool_output>")); + assert_eq!(message.content, content); } }