fix: reliable network tests and improved tool error messages - #626
Conversation
…ror (#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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…n-issue-448.-do-we-have
…merge main - Gate `use std::path::PathBuf` with `#[cfg(unix)]` in sandbox/container.rs since all usages are inside `#[cfg(unix)]` functions, fixing Windows clippy error (unused import) - Add two regression tests for `enable_compilation_cache` (#448): one verifies explicit cache directory creation and TOML config, another verifies label-based isolation between engines - Resolve merge conflict in test.yml to include both windows-build and wasm-wit-compat jobs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Format tool errors as "Tool '<name>' failed: <reason>" instead of the bare "Error: <reason>" 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 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's stability and diagnostic capabilities. It improves the reliability of network-related tests by using dedicated test IP ranges, making them more consistent across diverse network environments. Additionally, it refines how tool errors are reported, providing more actionable information to the agent and facilitating better decision-making. A specific fix for WASM compilation caching on Windows also prevents potential file locking conflicts, contributing to overall system robustness. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces two main improvements: making network failure tests more reliable by using TEST-NET-1 IPs, and enhancing tool error messages to include the tool name for better debugging by the LLM. The changes are well-implemented and include corresponding tests. I've provided a suggestion to further improve the error message formatting for better maintainability and effectiveness.
Note: Security Review did not run due to the size of the PR.
| Err(e) => format!( | ||
| "Tool '{}' failed: {}", | ||
| tc.name, e | ||
| ), |
There was a problem hiding this comment.
The new error message format introduces redundancy. The e.to_string() call for a ToolError often produces a string that already includes the tool's name (e.g., Tool http execution failed: ...). Prepended with Tool '{}' failed: , this results in a verbose and repetitive message like Tool 'http' failed: Tool error: Tool http execution failed: connection refused, which could be confusing for the LLM. To make the error message clearer and more concise, consider extracting just the core reason from the error.
Err(e) => {
let reason = match e {
crate::error::Error::Tool(
crate::error::ToolError::ExecutionFailed { reason, .. },
) => reason,
other => other.to_string(),
};
format!("Tool '{}' failed: {}", tc.name, reason)
},References
- Create specific error variants for different failure modes to provide semantically correct and clear error messages. This comment aims to improve the clarity and conciseness of error messages, aligning with the goal of providing semantically correct and clear error messages.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
) * fix(wasm): use per-engine cache dirs on Windows to avoid file lock error (nearai#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 nearai#448 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 nearai#444 (takeover from hobostay) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: include tool name in error messages sent to LLM Format tool errors as "Tool '<name>' failed: <reason>" instead of the bare "Error: <reason>" 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 nearai#487 (takeover from lustsazeus-lab, PR nearai#530) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve cargo fmt formatting in dispatcher Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
) * fix(wasm): use per-engine cache dirs on Windows to avoid file lock error (nearai#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 nearai#448 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 nearai#444 (takeover from hobostay) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: include tool name in error messages sent to LLM Format tool errors as "Tool '<name>' failed: <reason>" instead of the bare "Error: <reason>" 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 nearai#487 (takeover from lustsazeus-lab, PR nearai#530) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve cargo fmt formatting in dispatcher Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Tool '<name>' failed: <reason>instead of bareError: <reason>so the LLM can identify which tool failed and try alternatives. Does not short-circuit the agent loop. (closes bug: agent returns raw "[Called tool ...]" text when all tool call attempts fail #487, takeover from @lustsazeus-lab PR fix: generate user-friendly error message when all tool calls fail #530)Test plan
test_search_returns_error_on_network_failurepasses with tightened assertion (noerror.contains("error")tautology)health_with_unreachable_url_is_falseuses TEST-NET-1test_tool_error_format_includes_tool_nameregression test for error formatcargo clippy --all --all-featurescleanGenerated with Claude Code