mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
* feat(tools): add production-grade coding tools, file history, and coding skills Add dedicated coding tools inspired by Claude Code's architecture to make IronClaw a more effective coding assistant: New tools: - GlobTool: fast file pattern matching via `glob` crate, sorted by mtime, with default exclusions (.git, node_modules, target, etc.) - GrepTool: content search wrapping ripgrep with 3 output modes (content, files_with_matches, count), pagination, and context lines - FileUndoTool: restore files to pre-modification state using in-memory file history snapshots Enhanced tools: - ReadFileTool: 10MB limit, 2000-line default, binary detection, device path blocking (/dev/zero, /proc/*/fd/*) - ApplyPatchTool: uniqueness validation (error on ambiguous matches), workspace path rejection, 10MB size limit, file history integration - WriteFileTool: file history integration for undo support Updated tool descriptions to guide LLM behavior (prefer apply_patch over write_file, always read before editing, use glob/grep instead of shell). New skills: - coding: best practices for code editing, search, and file operations - commit: git commit message generation workflow - review: code review workflow with structured checklist Shared infrastructure: - DEFAULT_EXCLUDED_DIRS constant in path_utils.rs - FileHistory module with SharedFileHistory for cross-tool snapshots 66 new tests covering all tools, edge cases, and regression scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: apply cargo fmt formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): address PR review — security, correctness, and robustness fixes - Move device path blocking after validate_path() to prevent traversal bypass - Add /proc/kcore, /proc/kmem to blocked paths - Reject absolute patterns and '..' in glob tool, add strip_prefix defense - Wrap glob sync I/O in spawn_blocking to avoid blocking tokio executor - Sort files_with_matches globally before pagination in grep tool - Add default exclusions for node_modules/target in grep tool - Inject ctx.extra_env into rg environment matching ShellTool policy - Use per-line strip_prefix for content mode path relativization - Change FileSnapshot.content_before to Vec<u8> for binary file support - Log snapshot errors with tracing::debug instead of silently discarding - Fix skill name mismatch: code-review → review to match directory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(skills): rename review skill directory to code-review Aligns the directory name with the manifest name (code-review) to prevent incorrect override/dedup behavior in the bundled-skill loader. The name stays "code-review" since other domains may also need review-type skills. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(tools): add file edit guards — staleness detection, fuzzy matching, encoding preservation Add file_edit_guard module with production-grade safeguards for file editing: - ReadFileState tracks file reads with mtime for staleness detection - 4-level fuzzy matching fallback (exact → whitespace-normalized → quote-normalized → both) - UTF-16LE BOM detection and line ending style preservation (LF/CRLF/CR) - Read-before-edit enforcement for ApplyPatch and WriteFile tools - No-op edit rejection (old_string == new_string) - Shared state injection via Arc<RwLock<>> across ReadFile, WriteFile, ApplyPatch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): address all PR review comments — session scoping, parallelism, security - Session-scoped state: ReadFileState and FileHistory now keyed by job_id so concurrent sessions sharing the same registry don't leak state (#2025) - Parallel metadata: grep files_with_matches uses JoinSet (max 64 concurrency) instead of sequential await per file for mtime sorting - Shared env allowlist: grep_tool imports SAFE_ENV_VARS from shell.rs (made pub(crate)) instead of maintaining a divergent copy - Glob traversal: uses Component::ParentDir check instead of substring ".." match, so patterns like "foo..bar" are no longer falsely rejected - UTF-16LE in read_file: binary detection skips null-byte check for files with UTF-16LE BOM; read_file uses encoding-aware read path - Partial flag: default 2000-line truncation now marks read as partial, preventing edits against unseen content - write_file guard softened: staleness check logs warning instead of hard error (full-file replacement has lower risk than apply_patch) - Updated e2e trace to include read_file before apply_patch - Updated expected tool list in schema validation tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): use async metadata instead of blocking path.exists() in write_file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): fix false-positive panic detection for lifetimes in char lexer The check_no_panics.py lexer misinterpreted Rust lifetimes ('static) as char literal starts, causing in_char state to persist across lines and hide all subsequent brace-delimited blocks — including #[cfg(test)] mod tests. Reset in_char at line boundaries since Rust char literals cannot span lines. https://claude.ai/code/session_012bJjER6L5zSAFd9BqYMUmC * test: verify MCP push works * test * chore: remove test file * style: apply cargo fmt to file.rs Collapse multi-line method chain to single line per rustfmt. https://claude.ai/code/session_012bJjER6L5zSAFd9BqYMUmC * style: apply cargo fmt to file.rs Collapse multi-line method chain to single line per rustfmt. https://claude.ai/code/session_012bJjER6L5zSAFd9BqYMUmC * fix(file-tools): harden fuzzy patch matching and undo * fix(ci): formatting + wasmtime 43 cache config compatibility After merging latest staging, cargo fmt had diffs in file tools and the wasmtime cache TOML format changed (v43 dropped the `enabled` field under `[cache]`). Also removes accidental .fmt-test artifact. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(file-tools): simplify strip_trailing_whitespace Remove redundant double-pass through .lines() — the first collect+join was a no-op since .lines() already handles line endings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): address PR review comments — security, correctness, tests - Add is_sensitive_path checks to GlobTool and GrepTool, matching the defense-in-depth posture of ReadFileTool/WriteFileTool/ListDirTool - Fix UTF-8 panicking byte-index slice in apply_patch error preview (old_string[..200] → chars().take(200)) - Add 10MB size guard on file_history snapshots to prevent memory exhaustion from snapshotting large files - Replace dead turn_number field with auto-incrementing sequence_number in FileHistory — callers no longer pass a hardcoded 0 - Fix glob mtime test flakiness by increasing sleep to 1100ms (above 1s filesystem granularity) - Fix emoji test to actually include emoji/non-ASCII content Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Zaki Manian <zaki@iqlusion.io>
171 lines
5.6 KiB
Rust
171 lines
5.6 KiB
Rust
//! Validates that all built-in tool schemas conform to OpenAI strict-mode rules.
|
|
//!
|
|
//! This catches the class of bugs where `required` keys aren't in `properties`,
|
|
//! properties are missing `type` (intentional freeform is allowed), or nested
|
|
//! objects/arrays are malformed.
|
|
//!
|
|
//! See: <https://github.com/nearai/ironclaw/issues/352> (QA plan, item 1.1)
|
|
|
|
use ironclaw::tools::validate_tool_schema;
|
|
use ironclaw::tools::{Tool, ToolRegistry};
|
|
|
|
/// Validate schemas of all tools registered via `register_builtin_tools()` and
|
|
/// `register_dev_tools()` (echo, time, json, http, shell, file tools).
|
|
///
|
|
/// These tools can be constructed without external dependencies (no DB, no
|
|
/// workspace, no extension manager). Tools requiring dependencies (memory, job,
|
|
/// skill, extension, routine) are validated individually below where test
|
|
/// construction helpers exist.
|
|
#[tokio::test]
|
|
async fn all_core_builtin_tool_schemas_are_valid() {
|
|
let registry = ToolRegistry::new();
|
|
registry.register_builtin_tools();
|
|
registry.register_dev_tools();
|
|
|
|
let tools = registry.all().await;
|
|
assert!(
|
|
!tools.is_empty(),
|
|
"registry should have tools after registration"
|
|
);
|
|
|
|
let mut all_errors = Vec::new();
|
|
for tool in &tools {
|
|
let schema = tool.parameters_schema();
|
|
let errors = validate_tool_schema(&schema, tool.name());
|
|
if !errors.is_empty() {
|
|
all_errors.push(format!(
|
|
"Tool '{}' has schema errors:\n {}",
|
|
tool.name(),
|
|
errors.join("\n ")
|
|
));
|
|
}
|
|
}
|
|
|
|
assert!(
|
|
all_errors.is_empty(),
|
|
"Tool schema validation failures:\n{}",
|
|
all_errors.join("\n\n")
|
|
);
|
|
}
|
|
|
|
/// Verify the exact set of tools registered by the core registration methods.
|
|
/// This guards against a new tool being added without schema validation coverage.
|
|
#[tokio::test]
|
|
async fn core_registration_covers_expected_tools() {
|
|
let registry = ToolRegistry::new();
|
|
registry.register_builtin_tools();
|
|
registry.register_dev_tools();
|
|
|
|
let mut names = registry.list().await;
|
|
names.sort();
|
|
|
|
let expected = &[
|
|
"apply_patch",
|
|
"echo",
|
|
"file_undo",
|
|
"glob",
|
|
"grep",
|
|
"http",
|
|
"json",
|
|
"list_dir",
|
|
"plan_update",
|
|
"read_file",
|
|
"shell",
|
|
"time",
|
|
"write_file",
|
|
];
|
|
|
|
assert_eq!(
|
|
names, expected,
|
|
"Core tool set changed. Update this test and ensure new tools have valid schemas."
|
|
);
|
|
}
|
|
|
|
/// Validate individual tool schemas that are known to use non-trivial patterns.
|
|
/// These are regression tests for specific bugs.
|
|
#[test]
|
|
fn json_tool_freeform_data_field_is_valid() {
|
|
// Regression: json tool's "data" field intentionally has no "type" for
|
|
// OpenAI compatibility (union types with arrays require "items").
|
|
let tool = ironclaw::tools::builtin::JsonTool;
|
|
let schema = tool.parameters_schema();
|
|
let errors = validate_tool_schema(&schema, "json");
|
|
assert!(errors.is_empty(), "json tool schema errors: {errors:?}");
|
|
|
|
// Verify the freeform pattern is still in place
|
|
let data = schema
|
|
.get("properties")
|
|
.and_then(|p| p.get("data"))
|
|
.expect("json tool should have 'data' property");
|
|
assert!(
|
|
data.get("type").is_none(),
|
|
"json.data should be freeform (no type) for OpenAI compatibility"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn http_tool_headers_array_is_valid() {
|
|
// Regression: http tool's "headers" is an array of {name, value} objects.
|
|
let tool = ironclaw::tools::builtin::HttpTool::new();
|
|
let schema = tool.parameters_schema();
|
|
let errors = validate_tool_schema(&schema, "http");
|
|
assert!(errors.is_empty(), "http tool schema errors: {errors:?}");
|
|
|
|
// Verify array structure
|
|
let headers = schema
|
|
.get("properties")
|
|
.and_then(|p| p.get("headers"))
|
|
.expect("http tool should have 'headers' property");
|
|
assert_eq!(
|
|
headers.get("type").and_then(|t| t.as_str()),
|
|
Some("array"),
|
|
"headers should be an array"
|
|
);
|
|
assert!(
|
|
headers.get("items").is_some(),
|
|
"headers array should have items defined"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn time_tool_schema_is_valid() {
|
|
let tool = ironclaw::tools::builtin::TimeTool;
|
|
let schema = tool.parameters_schema();
|
|
let errors = validate_tool_schema(&schema, "time");
|
|
assert!(errors.is_empty(), "time tool schema errors: {errors:?}");
|
|
}
|
|
|
|
#[test]
|
|
fn shell_tool_schema_is_valid() {
|
|
let tool = ironclaw::tools::builtin::ShellTool::new();
|
|
let schema = tool.parameters_schema();
|
|
let errors = validate_tool_schema(&schema, "shell");
|
|
assert!(errors.is_empty(), "shell tool schema errors: {errors:?}");
|
|
}
|
|
|
|
/// Validates that all core tools work correctly under a multi-threaded tokio runtime.
|
|
/// This catches sync-async boundary bugs like tokio::sync::RwLock::blocking_read()
|
|
/// panicking when called from within a multi-threaded runtime context.
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
async fn all_core_tools_work_in_multi_thread_runtime() {
|
|
let registry = ToolRegistry::new();
|
|
registry.register_builtin_tools();
|
|
registry.register_dev_tools();
|
|
|
|
let tools = registry.all().await;
|
|
assert!(
|
|
!tools.is_empty(),
|
|
"registry should have tools after registration"
|
|
);
|
|
|
|
for tool in &tools {
|
|
// These sync trait methods must not panic in multi-thread runtime
|
|
let _ = tool.name();
|
|
let _ = tool.description();
|
|
let _ = tool.parameters_schema();
|
|
let _ = tool.requires_approval(&serde_json::json!({}));
|
|
let _ = tool.requires_sanitization();
|
|
let _ = tool.domain();
|
|
}
|
|
}
|