From 4ac78a5b1f704579e31dfdde7708b968e21ca74c Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Fri, 6 Mar 2026 21:54:12 -0800 Subject: [PATCH] fix: reliable network tests and improved tool error messages (#626) * fix(wasm): use per-engine cache dirs on Windows to avoid file lock error (#448) On Windows, multiple wasmtime Engine instances sharing the default compilation cache directory hit OS error 33 (ERROR_LOCK_VIOLATION) because Windows holds exclusive file locks on memory-mapped cache files. This is especially triggered when the Telegram channel WASM module is loaded at startup and then hot-activated via the Extensions UI. Fix by giving each engine its own cache subdirectory on Windows (~/.cache/ironclaw/wasmtime-tools/ and wasmtime-channels/). On Unix the shared default cache continues to work as before. Also adds Windows CI jobs (cargo check + clippy across all feature flag combinations) to catch Windows-specific issues going forward. Closes #448 Co-Authored-By: Claude Opus 4.6 * fix: silence Windows clippy warnings for platform-gated code Gate PathBuf import behind #[cfg(unix)] in container.rs (only used in Unix socket path), suppress unused_mut on conflicts Vec in channels.rs (mutations are platform-gated), and add cfg gates on keychain constants and hex_to_bytes that are only used on macOS/Linux. Co-Authored-By: Claude Opus 4.6 * fix: escape directory path in TOML cache config to prevent injection Use double-quoted TOML strings with backslash and double-quote escaping for the cache directory path, preventing breakage or injection when paths contain special characters (e.g. single quotes on Unix, backslashes on Windows). Co-Authored-By: Claude Opus 4.6 * fix: resolve cargo fmt formatting errors Fix import ordering in container.rs and line wrapping in runtime.rs to pass the CI formatting check. Co-Authored-By: Claude Opus 4.6 * fix(ci): restore Path import for all platforms, keep PathBuf unix-only Path is used in non-cfg-gated functions (lines 148, 244) so it must be available on all platforms. Only PathBuf is unix-specific. Co-Authored-By: Claude Opus 4.6 * fix: use RFC 5737 TEST-NET-1 IPs for reliable network failure tests Replace localhost/loopback addresses with 192.0.2.1 (TEST-NET-1) in network failure tests so they work consistently behind HTTP proxies. Tighten the catalog.rs error assertion to avoid matching any string containing "error". Closes #444 (takeover from hobostay) Co-Authored-By: Claude Opus 4.6 * fix: include tool name in error messages sent to LLM Format tool errors as "Tool '' failed: " instead of the bare "Error: " so the LLM can identify which tool failed and reason about alternatives. Does not short-circuit the agent loop -- errors still flow back to the LLM for reasoning. Closes #487 (takeover from lustsazeus-lab, PR #530) Co-Authored-By: Claude Opus 4.6 * fix: resolve cargo fmt formatting in dispatcher Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/agent/dispatcher.rs | 23 ++++++++++++++++++++++- src/skills/catalog.rs | 14 +++++++++++--- src/tunnel/custom.rs | 8 ++++++-- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 95d8d711..94f69ae6 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -708,7 +708,7 @@ impl Agent { sanitized.was_modified, ) } - Err(e) => format!("Error: {}", e), + Err(e) => format!("Tool '{}' failed: {}", tc.name, e), }; context_messages.push(ChatMessage::tool_result( @@ -2028,4 +2028,25 @@ mod tests { let result = super::strip_internal_tool_call_text(input); assert_eq!(result, input); } + + #[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); + assert!( + formatted.contains("Tool 'http' failed:"), + "Error should identify the tool by name, got: {formatted}" + ); + assert!( + formatted.contains("connection refused"), + "Error should include the underlying reason, got: {formatted}" + ); + } } diff --git a/src/skills/catalog.rs b/src/skills/catalog.rs index 2a2b69dc..93584f5f 100644 --- a/src/skills/catalog.rs +++ b/src/skills/catalog.rs @@ -457,12 +457,20 @@ mod tests { #[tokio::test] async fn test_search_returns_error_on_network_failure() { - // Point at an invalid URL to trigger a network error - let catalog = SkillCatalog::with_url("http://127.0.0.1:1"); + // Use RFC 5737 TEST-NET-1 (192.0.2.0/24) for reliable failure even behind proxies. + let catalog = SkillCatalog::with_url("http://192.0.2.1:9999"); let outcome = catalog.search("test").await; assert!(outcome.results.is_empty()); assert!(outcome.error.is_some()); - assert!(outcome.error.unwrap().contains("Registry unreachable")); + let error = outcome.error.unwrap(); + assert!( + error.contains("Registry unreachable") + || error.contains("connect") + || error.contains("502") + || error.contains("503") + || error.contains("504"), + "Expected connection or gateway error, got: {error}", + ); } #[tokio::test] diff --git a/src/tunnel/custom.rs b/src/tunnel/custom.rs index 888cb698..1cb71b0f 100644 --- a/src/tunnel/custom.rs +++ b/src/tunnel/custom.rs @@ -214,12 +214,16 @@ mod tests { #[tokio::test] async fn health_with_unreachable_url_is_false() { + // Use RFC 5737 TEST-NET-1 (192.0.2.0/24) for reliable failure even behind proxies. let tunnel = CustomTunnel::new( "sleep 1".into(), - Some("http://127.0.0.1:9/healthz".into()), + Some("http://192.0.2.1:9999/healthz".into()), None, ); - assert!(!tunnel.health_check().await); + assert!( + !tunnel.health_check().await, + "Health check should fail for unreachable URL" + ); } #[test]