diff --git a/channels-src/feishu/Cargo.lock b/channels-src/feishu/Cargo.lock index 60f68fcc..4e95f3fe 100644 --- a/channels-src/feishu/Cargo.lock +++ b/channels-src/feishu/Cargo.lock @@ -44,6 +44,7 @@ version = "0.1.0" dependencies = [ "serde", "serde_json", + "subtle", "wit-bindgen", ] @@ -208,6 +209,12 @@ dependencies = [ "smallvec", ] +[[package]] +name = "subtle" +version = "2.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" + [[package]] name = "syn" version = "2.0.117" diff --git a/channels-src/feishu/Cargo.toml b/channels-src/feishu/Cargo.toml index 53b9357d..95762410 100644 --- a/channels-src/feishu/Cargo.toml +++ b/channels-src/feishu/Cargo.toml @@ -15,6 +15,7 @@ wit-bindgen = "0.36" # Serialization serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +subtle = "2.6" # Exclude from parent workspace (this is a standalone WASM component) diff --git a/channels-src/feishu/feishu.capabilities.json b/channels-src/feishu/feishu.capabilities.json index a228cc4e..cf344d74 100644 --- a/channels-src/feishu/feishu.capabilities.json +++ b/channels-src/feishu/feishu.capabilities.json @@ -27,7 +27,7 @@ { "name": "feishu_verification_token", "prompt": "Enter your Feishu/Lark Verification Token (from Event Subscription webhook settings)", - "optional": true + "optional": false } ], "setup_url": "https://open.feishu.cn/app" @@ -63,13 +63,15 @@ }, "webhook": { "secret_header": "X-Feishu-Verification-Token", - "secret_name": "feishu_verification_token" + "secret_name": "feishu_verification_token", + "managed_by_host": false } } }, "config": { "app_id": null, "app_secret": null, + "verification_token": null, "api_base": "https://open.feishu.cn", "owner_id": null, "dm_policy": "pairing", diff --git a/channels-src/feishu/src/lib.rs b/channels-src/feishu/src/lib.rs index 62440d2c..647f5fa5 100644 --- a/channels-src/feishu/src/lib.rs +++ b/channels-src/feishu/src/lib.rs @@ -23,7 +23,8 @@ //! - App credentials (app_id, app_secret) are injected by the host into //! the config JSON during startup for token exchange //! - Bearer token for API calls is obtained via token exchange and cached -//! - Verification token validated by host for webhook requests +//! - Webhook requests must be authenticated by the host or by a matching +//! Feishu verification token in the request body // Generate bindings from the WIT file wit_bindgen::generate!({ @@ -32,6 +33,7 @@ wit_bindgen::generate!({ }); use serde::{Deserialize, Serialize}; +use subtle::ConstantTimeEq; // Re-export generated types use exports::near::agent::channel::{ @@ -50,6 +52,7 @@ const ALLOW_FROM_PATH: &str = "allow_from"; const API_BASE_PATH: &str = "api_base"; const APP_ID_PATH: &str = "app_id"; const APP_SECRET_PATH: &str = "app_secret"; +const VERIFICATION_TOKEN_PATH: &str = "verification_token"; const TOKEN_PATH: &str = "tenant_access_token"; const TOKEN_EXPIRY_PATH: &str = "token_expiry"; @@ -102,6 +105,10 @@ struct FeishuEventHeader { /// Tenant key. #[serde(default)] tenant_key: Option, + + /// Verification token for v2 event payloads. + #[serde(default)] + token: Option, } /// Message receive event payload (im.message.receive_v1). @@ -251,6 +258,9 @@ struct FeishuConfig { /// Feishu App Secret (for token exchange). app_secret: Option, + /// Feishu Event Subscription verification token. + verification_token: Option, + /// API base URL. Defaults to "https://open.feishu.cn" (use /// "https://open.larksuite.com" for Lark international). #[serde(default = "default_api_base")] @@ -300,6 +310,9 @@ impl Guest for FeishuChannel { if let Some(ref app_secret) = config.app_secret { let _ = channel_host::workspace_write(APP_SECRET_PATH, app_secret); } + if let Some(ref verification_token) = config.verification_token { + let _ = channel_host::workspace_write(VERIFICATION_TOKEN_PATH, verification_token); + } if let Some(owner_id) = &config.owner_id { let _ = channel_host::workspace_write(OWNER_ID_PATH, owner_id); @@ -376,6 +389,23 @@ impl Guest for FeishuChannel { } }; + let configured_token = + channel_host::workspace_read(VERIFICATION_TOKEN_PATH).filter(|token| !token.is_empty()); + if !is_authenticated_webhook( + req.secret_validated, + configured_token.as_deref(), + request_verification_token(&event), + ) { + channel_host::log( + channel_host::LogLevel::Warn, + "Rejecting unauthenticated Feishu webhook request", + ); + return json_response( + 401, + serde_json::json!({"error": "Webhook authentication failed"}), + ); + } + // Handle URL verification challenge (initial webhook setup). if event.event_type.as_deref() == Some("url_verification") { if let Some(challenge) = &event.challenge { @@ -839,6 +869,31 @@ fn json_response(status: u16, body: serde_json::Value) -> OutgoingHttpResponse { } } +fn is_authenticated_webhook( + secret_validated: bool, + configured_token: Option<&str>, + request_token: Option<&str>, +) -> bool { + if secret_validated { + return true; + } + + match (configured_token, request_token) { + (Some(expected), Some(provided)) => { + bool::from(expected.as_bytes().ct_eq(provided.as_bytes())) + } + _ => false, + } +} + +fn request_verification_token(event: &FeishuEvent) -> Option<&str> { + event + .header + .as_ref() + .and_then(|header| header.token.as_deref()) + .or(event.token.as_deref()) +} + #[cfg(test)] mod tests { use super::*; @@ -862,7 +917,10 @@ mod tests { fn parse_token_response_rejects_missing_token() { let json = r#"{"code": 0, "msg": "ok", "expire": 7200}"#; let result: Result = serde_json::from_str(json); - assert!(result.is_err(), "should fail when tenant_access_token is missing"); + assert!( + result.is_err(), + "should fail when tenant_access_token is missing" + ); } #[test] @@ -894,4 +952,64 @@ mod tests { assert_eq!(resp.code, 10003); assert!(resp.tenant_access_token.is_empty()); } + + #[test] + fn webhook_auth_requires_host_auth_or_matching_verification_token() { + assert!( + !is_authenticated_webhook(false, None, Some("token")), + "requests without any configured verification mechanism must be rejected" + ); + assert!( + !is_authenticated_webhook(false, Some("expected"), None), + "requests missing the Feishu token must be rejected when host auth did not pass" + ); + assert!( + !is_authenticated_webhook(false, Some("expected"), Some("wrong")), + "requests with the wrong Feishu token must be rejected" + ); + assert!( + is_authenticated_webhook(false, Some("expected"), Some("expected")), + "matching Feishu verification token should authenticate the request" + ); + assert!( + is_authenticated_webhook(true, None, None), + "host-authenticated requests should still be accepted" + ); + assert!( + is_authenticated_webhook(true, Some("expected"), Some("wrong")), + "host authentication should take precedence over body token checks" + ); + } + + #[test] + fn request_verification_token_prefers_v2_header_token() { + let event: FeishuEvent = serde_json::from_str( + r#"{ + "schema": "2.0", + "header": { + "event_id": "evt_123", + "event_type": "im.message.receive_v1", + "token": "header-token" + }, + "event": {} + }"#, + ) + .unwrap(); + + assert_eq!(request_verification_token(&event), Some("header-token")); + } + + #[test] + fn request_verification_token_falls_back_to_top_level_token() { + let event: FeishuEvent = serde_json::from_str( + r#"{ + "type": "url_verification", + "challenge": "abc", + "token": "top-level-token" + }"#, + ) + .unwrap(); + + assert_eq!(request_verification_token(&event), Some("top-level-token")); + } } 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/wasm/loader.rs b/src/channels/wasm/loader.rs index 6329428f..41ecfac1 100644 --- a/src/channels/wasm/loader.rs +++ b/src/channels/wasm/loader.rs @@ -317,6 +317,14 @@ impl LoadedChannel { .map(|f| f.webhook_secret_name()) .unwrap_or_else(|| format!("{}_webhook_secret", self.channel.channel_name())) } + + /// Whether the host should enforce generic webhook-secret validation. + pub fn webhook_secret_managed_by_host(&self) -> bool { + self.capabilities_file + .as_ref() + .map(|f| f.webhook_secret_managed_by_host()) + .unwrap_or(true) + } } /// Results from loading multiple channels. diff --git a/src/channels/wasm/schema.rs b/src/channels/wasm/schema.rs index b5081426..f2ca6674 100644 --- a/src/channels/wasm/schema.rs +++ b/src/channels/wasm/schema.rs @@ -185,6 +185,19 @@ impl ChannelCapabilitiesFile { .and_then(|w| w.secret_name.clone()) .unwrap_or_else(|| format!("{}_webhook_secret", self.name)) } + + /// Whether the host should enforce generic webhook-secret validation. + /// + /// Defaults to true. Channels can opt out when they validate the shared + /// secret themselves using provider-specific request body fields. + pub fn webhook_secret_managed_by_host(&self) -> bool { + self.capabilities + .channel + .as_ref() + .and_then(|c| c.webhook.as_ref()) + .and_then(|w| w.managed_by_host) + .unwrap_or(true) + } } /// Schema for channel capabilities. @@ -302,6 +315,14 @@ pub struct WebhookSchema { /// Secret name in secrets store for HMAC-SHA256 signing (Slack-style). #[serde(default)] pub hmac_secret_name: Option, + + /// Whether the host/router should enforce generic webhook-secret + /// validation before the channel sees the request. + /// + /// Default: true. Set to false when the provider sends the shared secret + /// in a provider-specific request field rather than the configured header. + #[serde(default)] + pub managed_by_host: Option, } /// Setup configuration schema. @@ -611,6 +632,25 @@ mod tests { Some("X-Telegram-Bot-Api-Secret-Token") ); assert_eq!(file.webhook_secret_name(), "telegram_webhook_secret"); + assert!(file.webhook_secret_managed_by_host()); + } + + #[test] + fn test_webhook_schema_can_disable_host_managed_secret_validation() { + let json = r#"{ + "name": "feishu", + "capabilities": { + "channel": { + "webhook": { + "secret_name": "feishu_verification_token", + "managed_by_host": false + } + } + } + }"#; + + let file = ChannelCapabilitiesFile::from_json(json).unwrap(); + assert!(!file.webhook_secret_managed_by_host()); } #[test] diff --git a/src/channels/wasm/setup.rs b/src/channels/wasm/setup.rs index 7f0bb8fb..84df615f 100644 --- a/src/channels/wasm/setup.rs +++ b/src/channels/wasm/setup.rs @@ -139,13 +139,18 @@ async fn register_channel( }; let secret_header = loaded.webhook_secret_header().map(|s| s.to_string()); + let host_webhook_secret = if loaded.webhook_secret_managed_by_host() { + webhook_secret.clone() + } else { + None + }; let webhook_path = format!("/webhook/{}", channel_name); let endpoints = vec![RegisteredEndpoint { channel_name: channel_name.clone(), path: webhook_path, methods: vec!["POST".to_string()], - require_secret: webhook_secret.is_some(), + require_secret: host_webhook_secret.is_some(), }]; let channel_arc = Arc::new(loaded.channel.with_owner_actor_id(owner_actor_id.clone())); @@ -205,7 +210,7 @@ async fn register_channel( tracing::info!( channel = %channel_name, - has_webhook_secret = webhook_secret.is_some(), + has_webhook_secret = host_webhook_secret.is_some(), secret_header = ?secret_header, "Registering channel with router" ); @@ -214,7 +219,7 @@ async fn register_channel( .register( Arc::clone(&channel_arc), endpoints, - webhook_secret.clone(), + host_webhook_secret.clone(), secret_header, ) .await; @@ -392,8 +397,9 @@ pub async fn inject_channel_credentials( /// placeholders in URLs and headers, so this function fills config fields /// that map to secret names. /// -/// Mapping: for a channel named "feishu", secrets `feishu_app_id` and -/// `feishu_app_secret` are injected as config keys `app_id` and `app_secret`. +/// Mapping: for a channel named "feishu", secrets `feishu_app_id`, +/// `feishu_app_secret`, and `feishu_verification_token` are injected as config +/// keys `app_id`, `app_secret`, and `verification_token`. async fn inject_channel_secrets_into_config( channel_name: &str, secrets_store: &Option>, @@ -404,6 +410,7 @@ async fn inject_channel_secrets_into_config( "feishu" => &[ ("app_id", "feishu_app_id"), ("app_secret", "feishu_app_secret"), + ("verification_token", "feishu_verification_token"), ], _ => return, }; 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); } }