feat(tools): production-grade coding tools, file history, and skills

[ilblackdragon]

Summary

• Add glob tool for fast file pattern matching (sorted by mtime, excludes .git/node_modules/target)
• Add grep tool wrapping ripgrep with structured output modes (content/files_with_matches/count), pagination, and context lines
• Add file_undo tool backed by in-memory FileHistory snapshots (supports binary files via Vec<u8>)
• Enhance read_file with device path blocking (on resolved paths), binary detection, size limits
• Enhance apply_patch with uniqueness validation and file history integration
• Add bundled skills: coding, commit, code-review

Security hardening (from review)

• Device/proc path blocking runs after validate_path() on resolved paths, preventing traversal bypass
• Glob tool rejects absolute patterns and .. segments; strip_prefix defense-in-depth on all matches
• Glob sync I/O wrapped in spawn_blocking
• Grep tool injects ctx.extra_env matching ShellTool policy
• Content mode path relativization uses per-line strip_prefix (not global string replace)
• files_with_matches sorts globally before pagination

Supersedes #1841 (rebranched from staging to avoid v2-architecture merge conflicts).

Test plan

• 64 unit tests pass across glob, grep, file, file_history modules
• cargo fmt --all -- --check clean
• cargo clippy --all-features zero warnings
• check_no_panics.py passes

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR expands the toolchain for “coding agent” workflows by adding dedicated search/discovery tools (glob, grep), file-level undo via in-memory snapshots, and tightening/standardizing filesystem safety behaviors. It also bundles several skills to guide agent behavior for coding, commits, and code review.

Changes:

• Add new built-in tools: glob (pattern-based file discovery) and grep (ripgrep-backed search with structured outputs).
• Add file_undo backed by a shared in-memory FileHistory, and integrate snapshotting into write_file / apply_patch.
• Harden and extend filesystem tools (read_file size/line limits, binary detection, device/proc blocking; apply_patch uniqueness validation; shared excluded-dir list).

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/registry.rs	Registers new dev tools and injects shared FileHistory into file-modifying tools.
src/tools/builtin/path_utils.rs	Introduces DEFAULT_EXCLUDED_DIRS constant for consistent filesystem-walk exclusions.
src/tools/builtin/mod.rs	Exposes new built-in modules/tools (glob/grep/file_history).
src/tools/builtin/grep_tool.rs	Implements grep tool with multiple output modes, truncation, and pagination.
src/tools/builtin/glob_tool.rs	Implements glob tool with mtime sorting, default exclusions, and sandbox checks.
src/tools/builtin/file.rs	Enhances read_file safety/limits; integrates file history into write_file / apply_patch; adds apply_patch validation.
src/tools/builtin/file_history.rs	Adds FileHistory snapshot store and file_undo tool implementation.
skills/commit/SKILL.md	Adds “commit” skill guidance for commit-message workflow.
skills/coding/SKILL.md	Adds “coding” skill guidance emphasizing disciplined tool usage.
skills/code-review/SKILL.md	Adds “code-review” skill guidance for review workflow.
Cargo.toml	Adds glob dependency.
Cargo.lock	Locks glob dependency for the workspace.

Comments suppressed due to low confidence (1)

src/tools/builtin/file.rs:218

• lines[start_line..end_line] can panic when offset is greater than total_lines (e.g., offset=9999 on a short file), because start_line becomes > total_lines and the slice is out of bounds. Clamp start_line to total_lines (or return an empty result / InvalidParameters) before slicing to avoid crashing the tool.

            (total_lines, false)
        };

        let selected_lines: Vec<String> = lines[start_line..end_line]
            .iter()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment says this uses the “same policy as ShellTool”, but the allowed env var list here is much narrower than ShellTool’s SAFE_ENV_VARS (e.g., missing PWD, LC_MESSAGES, XDG_* and toolchain vars). Either reuse the ShellTool allowlist (or move it to a shared helper) or update the comment/policy so they can’t silently diverge.

[ilblackdragon]

Fixed — removed the local copy. grep_tool now imports SAFE_ENV_VARS directly from shell.rs (made pub(crate)) so they can't diverge.

[Copilot]

In files_with_matches mode this does an awaited tokio::fs::metadata call per match line, sequentially. For large result sets this can be very slow. Consider sorting without mtimes (or only fetching mtimes for the page being returned), or parallelize metadata collection (e.g., stream + buffer_unordered) / run it in spawn_blocking.

Suggested change

	// Collect all file entries with mtime, sort globally, then paginate
	let all_lines = &lines;
	let mut file_entries: Vec<(String, SystemTime)> =
	Vec::with_capacity(all_lines.len());
	for line in all_lines {
	let path = line.trim();
	if path.is_empty() {
	continue;
	}
	let mtime = tokio::fs::metadata(path)
	.await
	.and_then(|m| m.modified())
	.unwrap_or(UNIX_EPOCH);
	let relative = std::path::Path::new(path)
	.strip_prefix(&search_path)
	.map(|p| p.to_string_lossy().into_owned())
	.unwrap_or_else(|_| path.to_string());
	file_entries.push((relative, mtime));
	// Collect all file entries with mtime, sort globally, then paginate.
	// Metadata lookup is done with bounded parallelism to avoid
	// sequentially awaiting one filesystem call per matched file.
	let all_paths: Vec<String> = lines
	.iter()
	.map(|line| line.trim())
	.filter(|path| !path.is_empty())
	.map(|path| path.to_string())
	.collect();
	let mut file_entries: Vec<(String, SystemTime)> =
	Vec::with_capacity(all_paths.len());
	let mut join_set = tokio::task::JoinSet::new();
	let mut pending = all_paths.into_iter();
	let max_concurrency = 64usize;
	while join_set.len() < max_concurrency {
	let Some(path) = pending.next() else {
	break;
	};
	let search_path = search_path.clone();
	join_set.spawn(async move {
	let mtime = tokio::fs::metadata(&path)
	.await
	.and_then(|m| m.modified())
	.unwrap_or(UNIX_EPOCH);
	let relative = std::path::Path::new(&path)
	.strip_prefix(&search_path)
	.map(|p| p.to_string_lossy().into_owned())
	.unwrap_or_else(|_| path.clone());
	(relative, mtime)
	});
	}
	while let Some(join_result) = join_set.join_next().await {
	if let Ok(entry) = join_result {
	file_entries.push(entry);
	}
	if let Some(path) = pending.next() {
	let search_path = search_path.clone();
	join_set.spawn(async move {
	let mtime = tokio::fs::metadata(&path)
	.await
	.and_then(|m| m.modified())
	.unwrap_or(UNIX_EPOCH);
	let relative = std::path::Path::new(&path)
	.strip_prefix(&search_path)
	.map(|p| p.to_string_lossy().into_owned())
	.unwrap_or_else(|_| path.clone());
	(relative, mtime)
	});
	}

[ilblackdragon]

Fixed — replaced sequential await loop with tokio::task::JoinSet bounded to 64 concurrent metadata lookups.

[Copilot]

pattern.contains("..") rejects any pattern containing the substring ".." (e.g. foo..bar), even when it’s not a parent-directory segment. If the intent is to block traversal segments, consider checking parsed path components for Component::ParentDir (handling both / and \\) rather than a substring match.

[ilblackdragon]

Fixed — now uses std::path::Component::ParentDir check instead of substring match, so patterns like foo..bar are no longer falsely rejected.

[gemini-code-assist]

Code Review

This pull request introduces several new development tools, including GlobTool for pattern-based file discovery, GrepTool for content searching, and a FileHistory system with FileUndoTool for reverting changes. It also adds skill definitions for code review, coding best practices, and git commits. Existing tools like ReadFileTool and ApplyPatchTool were enhanced with security features such as blocked device paths, binary file detection, and uniqueness validation. Review feedback suggests utilizing dedicated path manipulation APIs instead of string operations for relativizing file paths to ensure correctness.

[gemini-code-assist]

Relativizing paths by stripping a prefix is prone to errors if the prefix is not exactly matched. Consider using pathdiff::diff_paths or path.strip_prefix on the path object itself rather than string manipulation to ensure correctness.

[ilblackdragon]

The code already uses std::path::Path::new(path).strip_prefix(&search_path) (Path object, not string manipulation) in files_with_matches and count modes. The content mode uses string prefix stripping which is safe since the prefix is the search_path directory with trailing slash.

[serrrfirat]

Medium Severity

apply_patch now supports UTF-16LE text via the encoding-aware helpers, but read_file still probes for NUL bytes and then falls back to read_to_string() as UTF-8.

That means valid UTF-16LE text files will be rejected as "binary" here, and because apply_patch now requires read_file first, those files become impossible to patch through the intended flow.

read_file should reuse the same encoding-aware read path as apply_patch, then render the decoded text with line numbers.

[ilblackdragon]

Fixed — binary detection now checks for UTF-16LE BOM (FF FE) before the null-byte check, and read_file uses the encoding-aware read path from file_edit_guard::read_file_with_encoding() so UTF-16LE files are properly decoded.

[serrrfirat]

High Severity

This records a default read_file call as a full read even when the response was truncated to the first 2000 lines.

partial is only derived from offset/limit, but the new default cap also produces a partial view. After reading the first 2000 lines of a 3000-line file, apply_patch / write_file will pass the guard and allow edits against unseen content.

partial should also be set when truncated_by_default is true, and there should be a test that a default-truncated read blocks later edits until the full file is read.

[ilblackdragon]

Fixed — partial is now set when has_explicit_range || truncated_by_default, so the default 2000-line cap correctly marks the read as partial and blocks edits until the full file is read.

[serrrfirat]

Medium Severity

files_with_matches says it sorts globally by mtime before pagination, but by this point the raw rg output has already been truncated to 64KB.

So on large result sets you're only sorting the first chunk of ripgrep output, not the full match set. Newer files that fall after the first 64KB can never be returned, even if they should have been first after the mtime sort.

This mode needs a non-truncating collection path before the sort, or a different truncation strategy that happens after the full file list has been gathered.

[ilblackdragon]

Good catch on the 64KB truncation happening before mtime sort. This is a known limitation — ripgrep itself streams output and we truncate at the collection layer. For the common case (< 64KB of matched file paths) the sort is correct. For pathological cases the user can narrow the search pattern. Will note this as a known limitation.

[serrrfirat]

High Severity

read_state and file_history are being allocated once inside register_dev_tools() and then shared through the registry. In this app, the registry itself is shared across sessions/threads, so the new stateful behavior is no longer scoped to a single conversation.

That creates two correctness problems:

1. A read in session A can satisfy the "read before edit" guard for session B if they touch the same path.
2. file_undo can restore a snapshot created by a different session, even though the tool description says it only undoes changes from the current session.

This needs to be keyed by session/thread/job, or injected from per-session context instead of living in registry-global state.

[ilblackdragon]

Fixed — both ReadFileState and FileHistory are now keyed by job_id (from JobContext). A read in session A cannot satisfy the guard for session B, and file_undo only restores snapshots from the same job. Added test_read_state_isolated_across_jobs test to verify.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 16 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

src/tools/builtin/file.rs:226

• offset is documented as 1-indexed, but start_line is computed directly from offset without clamping to total_lines. If the caller passes an offset beyond EOF, lines[start_line..end_line] will panic (start > end). Consider clamping start_line to total_lines (and returning 0 lines shown) so out-of-range offsets are handled safely.

        let start_line = if offset > 0 {
            offset.saturating_sub(1)
        } else {
            0
        };

        let (end_line, truncated_by_default) = if let Some(lim) = limit {
            ((start_line + lim as usize).min(total_lines), false)
        } else if !has_explicit_range && total_lines > DEFAULT_LINE_LIMIT {
            // Apply default 2000-line limit when no offset/limit specified
            (DEFAULT_LINE_LIMIT.min(total_lines), true)
        } else {
            (total_lines, false)
        };

        let selected_lines: Vec<String> = lines[start_line..end_line]
            .iter()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Device/proc blocking relies only on the resolved path from validate_path(). Because validate_path() canonicalizes existing absolute paths, /proc/self/fd/* will typically resolve to something like /dev/pts/* (or a pipe), which bypasses the /proc/.../fd check and may block/hang reads. Consider also checking the original path_str (pre-canonicalization) for /proc/*/fd/ (and similar like /dev/fd/) and rejecting those regardless of how they resolve.

[ilblackdragon]

Good point about canonicalization resolving /proc/self/fd/* to /dev/pts/*. The current check runs on the resolved path which catches the common cases. Adding pre-canonicalization checks would improve defense-in-depth — will address in a follow-up.

[Copilot]

write_file snapshots the entire existing file contents before overwriting. There’s no size guard here, so overwriting a very large file could read it fully into memory and store it in FileHistory, causing high memory usage/DoS. Suggest checking existing file size (metadata.len) before snapshotting, and either (a) refuse, (b) cap snapshots to a maximum size, or (c) store an on-disk temp snapshot instead of in-memory for large files.

Suggested change

	// Snapshot existing file before overwriting (for file_undo)
	if let Some(ref history) = self.file_history {
	let mut h = history.write().await;
	if let Err(e) = h.snapshot(ctx.job_id, &path, "write_file", 0).await {
	tracing::debug!("file_history snapshot failed before write_file: {}", e);
	// Snapshot existing file before overwriting (for file_undo).
	// Guard against unbounded memory usage by skipping snapshots for large files.
	const MAX_HISTORY_SNAPSHOT_SIZE_BYTES: u64 = 10 * 1024 * 1024;
	if let Some(ref history) = self.file_history {
	let mut h = history.write().await;
	match fs::metadata(&path).await {
	Ok(metadata) if metadata.is_file() => {
	if metadata.len() <= MAX_HISTORY_SNAPSHOT_SIZE_BYTES {
	if let Err(e) = h.snapshot(ctx.job_id, &path, "write_file", 0).await {
	tracing::debug!("file_history snapshot failed before write_file: {}", e);
	}
	} else {
	tracing::debug!(
	"skipping file_history snapshot before write_file for {}: file size {} exceeds limit {}",
	path.display(),
	metadata.len(),
	MAX_HISTORY_SNAPSHOT_SIZE_BYTES
	);
	}
	}
	Ok(_) => {
	if let Err(e) = h.snapshot(ctx.job_id, &path, "write_file", 0).await {
	tracing::debug!("file_history snapshot failed before write_file: {}", e);
	}
	}
	Err(_) => {
	if let Err(e) = h.snapshot(ctx.job_id, &path, "write_file", 0).await {
	tracing::debug!("file_history snapshot failed before write_file: {}", e);
	}
	}

[ilblackdragon]

Valid concern. The snapshot is bounded by MAX_PATCH_SIZE (10MB) for apply_patch, but write_file doesn't have the same guard on the existing file. Will add a size cap on snapshots in a follow-up — files over a threshold should skip snapshotting or use on-disk temp files.

[Copilot]

path.exists() performs blocking filesystem I/O via std, and this runs inside an async execute() method. Prefer using tokio::fs::try_exists() / fs::metadata().await.is_ok() (you already call fs::metadata below) to avoid blocking the async runtime.

[ilblackdragon]

Good catch — replaced the blocking path.exists() with the fs::metadata(&path).await call that immediately follows it. The metadata result now serves double duty (existence check + mtime).

[Copilot]

Fuzzy matching + replace_all can produce incorrect behavior. match_count is computed on normalized content, but the replacement uses content.replace(&actual_old_string, ...) where actual_old_string is a single concrete substring from the first match. If multiple fuzzy matches exist with different original formatting (e.g., differing trailing whitespace / quote variants), match_count may be >1 but replace() may update fewer occurrences, and replacements will be reported incorrectly. Consider either rejecting replace_all when match_method != Exact, or implementing a replacement strategy that replaces all normalized matches while preserving original spans, and compute replacements from the actual number of edits performed.

[ilblackdragon]

Good edge case. When match_method != Exact, the actual_old_string is derived from the first match and may not match all normalized variants. For now, replace_all with fuzzy matching will replace however many instances match the concrete string. Rejecting replace_all for non-exact matches would be safer — will evaluate in a follow-up.

[Copilot]

In files_with_matches mode, ripgrep typically returns paths relative to the current working directory (or to the search root), but this code calls tokio::fs::metadata(&path) on the raw string. If path is relative, metadata will be resolved relative to the process CWD, which can fail and force mtime=UNIX_EPOCH, breaking sort-by-mtime and potentially mis-ordering results. Consider resolving each path against search_path (e.g., let abs = if Path::new(&path).is_absolute() { ... } else { search_path.join(&path) }) before metadata() and before strip_prefix.

[ilblackdragon]

We pass --path-separator / and the full absolute search_path to rg, so output paths are absolute. But you're right that defensively resolving relative paths would be more robust. Will address.

[Copilot]

Similar to files_with_matches, count mode assumes file strings can be strip_prefix'd against search_path, but ripgrep output is often relative, so strip_prefix(&search_path) will fail and absolute/odd paths may be returned. Consider resolving file against search_path first (or using pathdiff-style relativization from an absolute path) so returned paths are consistently relative to the search root.

[ilblackdragon]

Same as above — rg receives an absolute search path so output is absolute, but defensive resolution would be better. Will address together with the files_with_matches fix.

[Copilot]

This tool shells out to rg, but the repo’s sandbox container image (docker/sandbox.Dockerfile) doesn’t appear to install ripgrep. If grep is intended to run inside that sandbox (ToolDomain::Container), it will always fail at runtime with the “rg is not installed” error. Consider adding ripgrep to the sandbox image build (or otherwise guaranteeing rg is present in the execution environment) so the tool is usable in production.

[ilblackdragon]

Good point — ripgrep needs to be added to the sandbox Dockerfile. Will address in a separate PR for the container image.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[ilblackdragon]

Updates after latest staging merge

Merged latest staging and addressed the following:

Fixed in this push

1. CI formatting fixed — cargo fmt diffs from merge resolved
2. Wasmtime 43 cache config — removed enabled = true from TOML (wasmtime 43 dropped that field); updated test assertions
3. Removed .fmt-test — accidental test artifact deleted
4. Simplified strip_trailing_whitespace — removed redundant double .lines() pass in file_edit_guard.rs

Self-review items status

• Remove /.fmt-test — Done
• turn_number plumbing — Still always 0 from callers. Low priority since it's used in undo output and tests but doesn't affect correctness. Will address when we add proper turn tracking.
• Total-bytes cap in file_history — Deferred to follow-up (tracked)
• Simplify strip_trailing_whitespace — Done
• grep mtime sort optimization — Deferred to follow-up

Pre-existing test failures

tool_error_recovery and workspace_semantic_search in e2e_advanced_traces fail on staging too (before our branch). The trace fixtures expect tool call counts that don't match the current agent dispatcher behavior — unrelated to this PR.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

old_string[..200] can panic on non-ASCII input because Rust string slicing requires a UTF-8 char boundary. If a user provides an old_string containing multi-byte characters and length > 200 bytes, this will crash the tool. Truncate the preview on a char boundary (e.g., walk back to is_char_boundary, or build the preview via chars().take(...)).

Suggested change

	let preview = if old_string.len() > 200 {
	format!("{}...", &old_string[..200])
	let preview = if old_string.chars().count() > 200 {
	format!("{}...", old_string.chars().take(200).collect::<String>())

[ilblackdragon]

Fixed in 489612d — now uses old_string.chars().take(200).collect::<String>() for char-boundary-safe truncation.

[Copilot]

write_file snapshots the existing file by reading it fully into memory with no size guard. Overwriting a very large file can cause high memory usage (and the snapshot queue holds up to 50 entries), which is a realistic resource-exhaustion/DoS vector. Add a maximum snapshot size (or skip snapshotting / store snapshots on disk) based on file metadata before calling FileHistory::snapshot.

[ilblackdragon]

Fixed in 489612d — added MAX_SNAPSHOT_FILE_SIZE (10MB) check in FileHistory::snapshot(). Files exceeding the limit are silently skipped with a debug log.

[Copilot]

FileHistory::snapshot is always called with turn_number = 0 here, so file_undo will report restored_from_turn: 0 regardless of when the edit happened. If turn numbers aren’t available at tool-execution time, consider removing/renaming this field (e.g., to restored_from_snapshot_id), making it optional, or plumbing a real turn counter into the tool context so this metadata is accurate.

Suggested change

	// Snapshot before modification (for file_undo)
	if let Some(ref history) = self.file_history {
	let mut h = history.write().await;
	if let Err(e) = h.snapshot(ctx.job_id, &path, "apply_patch", 0).await {
	tracing::debug!("file_history snapshot failed before apply_patch: {}", e);
	}
	}
	// Snapshot before modification (for file_undo) is intentionally skipped here.
	// This path does not have an accurate turn number available, and passing a
	// hard-coded value would record misleading `restored_from_turn` metadata.

[ilblackdragon]

Fixed in 489612d — turn_number parameter removed entirely. FileHistory now auto-assigns monotonically increasing sequence_number values.

[serrrfirat]

Medium Severity — Missing is_sensitive_path check

GlobTool does not check is_sensitive_path() on the search root or matched paths. The other file tools (ReadFileTool, WriteFileTool, ApplyPatchTool, ListDirTool) all guard against credential directories (.ssh, .aws, .gnupg, etc.), but GlobTool skips this check.

This means {"pattern": "*", "path": "~/.ssh"} would enumerate files in sensitive directories. Not a blocker since GlobTool only returns paths (not content) and read_file would still block reading them, but it is a defense-in-depth gap.

Suggested fix: Add is_sensitive_path(&search_root) after validate_path, and filter matched paths with is_sensitive_path inside the spawn_blocking loop (mirroring ListDirTool's skip logic).

[ilblackdragon]

Fixed in 489612d — added is_sensitive_path check on the search root after validate_path, plus per-match filtering inside the spawn_blocking loop (mirroring ListDirTool's skip logic).

[serrrfirat]

Medium Severity — Missing is_sensitive_path check

GrepTool does not check is_sensitive_path() on the search path. Same defense-in-depth gap as the GlobTool comment, but slightly higher risk here because GrepTool returns file content (in content and count modes), not just paths.

Example: {"pattern": "AKIA", "path": "/home/user/.aws"} could surface AWS access key IDs in grep output.

Not a blocker — requires the LLM to actively target credential directories, and sandboxed deployments mitigate this — but worth closing the gap for parity with the other file tools.

Suggested fix: Add is_sensitive_path(&search_path) check after validate_path, consistent with ReadFileTool and ListDirTool.

[ilblackdragon]

Fixed in 489612d — added is_sensitive_path check on the search path after validate_path, consistent with the other file tools.

[serrrfirat]

LGTM — solid work. Path validation, env scrubbing, fuzzy matching, and the read-before-edit guard are all well-implemented.

Two inline comments on missing is_sensitive_path checks in GlobTool and GrepTool — defense-in-depth gaps, not blockers. Can be addressed in a follow-up.

[claude]

Code review

Found 8 issues:

1. [HIGH:87] Unbounded memory growth in FileHistory - up to 50 snapshots × 10MB = 500MB per registry instance

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file_history.rs#L22-L27

The FileHistory stores snapshots in a VecDeque with max 50 snapshots. While there's protection against files >10MB, under concurrent load with many job_ids, old entries aren't cleaned up from the ReadFileState HashMap. At production scale (10K+ short-lived jobs), this accumulates unbounded memory. Recommendation: attach read-state to JobContext lifecycle for cleanup or implement LRU eviction.

2. [MEDIUM:75] Off-by-one error in fuzzy byte offset mapping for patch application

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file_edit_guard.rs#L164-L177

In map_normalized_char_to_original_byte(), when handling the newline after a line:

if normalized_char_idx == normalized_seen + 1 {
    return Some(original_byte + 1);  // original_byte is at line.len(), not the newline
}

This returns a byte position pointing after the newline, not at the newline itself. This causes fuzzy-matched patches with replace_all=true to extract incorrect byte ranges, potentially corrupting files when the old_string has trailing whitespace differences.

3. [MEDIUM:70] Path traversal TOCTOU between validation and write

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file.rs#L159-L175

The validate_path() function canonicalizes symlinks, but between validation and the actual write in apply_patch or write_file, an attacker could replace a real file with a symlink. Mitigations exist (layered validation), but recommend re-validating immediately before write or using O_NOFOLLOW.

4. [MEDIUM:65] Unbounded allocation in ripgrep metadata collection spawns excessive tasks

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/grep_tool.rs#L312-L353

For files_with_matches mode, metadata is collected with 64 concurrent tasks BEFORE truncation to max_results. A malicious glob could match thousands of files and spike FD/memory usage. Recommendation: truncate results before metadata collection or lower concurrency bound.

5. [MEDIUM:60] Missing HashSet optimization for path exclusion checks

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/glob_tool.rs#L42-L51

The DEFAULT_EXCLUDED_DIRS is a slice (&[&str]), searched linearly. With 200 glob results × 50 path components × O(6) lookup, this is acceptable now but should use phf::Map or once_cell::Lazy<HashSet> for production scale.

6. [MEDIUM:50] UTF-16LE decoder silently discards incomplete final byte

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file_edit_guard.rs#L408-L411

The decoder uses chunks_exact(2) which silently drops any remaining odd byte:

let u16s: Vec<u16> = data
    .chunks_exact(2)
    .map(|pair| u16::from_le_bytes([pair[0], pair[1]]))
    .collect();

If a UTF-16LE file has an odd number of bytes after BOM stripping, the final byte is lost. Recommendation: validate (data.len() % 2) == 0 or use chunks(2) with error handling.

7. [LOW:65] Silent failures in file history snapshots only logged at debug level

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file.rs#L223-L230

File snapshot failures in apply_patch and write_file are caught and logged at debug level, allowing edits to proceed with partial undo capability. This is intentional (per comments), but users won't know file history is broken. Recommendation: consider a warning-level log or capability flag.

8. [LOW:50] ReadFileState HashMap grows unbounded with job_ids

https://github.com/anthropics/ironclaw/blob/45cece552df2517c64fb14ab8a73c714d79f429b/src/tools/builtin/file_edit_guard.rs#L26-L50

The ReadFileState::update_mtime() silently does nothing if a file was never read. Combined with unbounded HashMap growth, long-running services accumulate entries. Recommendation: add max-entries bound or LRU eviction, or attach state to JobContext for lifecycle management.