From f2cf3843ec5593f7bea206bc3976ad617f50a070 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Sun, 3 May 2026 02:30:44 -0500 Subject: [PATCH] feat(tools): inline unified-diff in edit_file / write_file results (#505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `edit_file` and `write_file` now capture the file contents before and after the mutation, generate a unified diff with `similar`, and emit it at the head of the `ToolResult` body. The TUI's existing `output_looks_like_diff` detector (history.rs:1335) sees the `@@` header in the first 5 lines and routes the payload through `diff_render::render_diff`, which already renders unified diffs with line numbers and coloured `+`/`-` gutters. The model also benefits — it sees exactly which lines changed instead of just `Replaced N occurrence(s)` or `Wrote N bytes`. Identical content produces an empty diff, in which case the body falls back to `\n(no changes)`. ### What's wired - New `crates/tui/src/tools/diff_format.rs` exposes `make_unified_diff(path, old, new) -> String` using `similar::TextDiff::from_lines(...).unified_diff().context_radius(3)`. - `WriteFileTool::execute` snapshots prior contents (or empty for new files), writes, then emits `\n` where summary is `Wrote N bytes to PATH` for existing files and `Created PATH (N bytes)` for new ones. - `EditFileTool::execute` snapshots, replaces, writes, emits `\nReplaced N occurrence(s) in PATH`. - `similar = "2"` added to `crates/tui/Cargo.toml`. Pure-Rust, no C deps; v2.7.0 in Cargo.lock. ### Tests - 4 unit tests in `diff_format::tests` covering identical inputs, replacement, new-file (against empty), and presence of the `@@` header in the first 5 lines (so the TUI detector trips). - Existing `test_write_file_tool` / `test_edit_file_tool` updated to assert both the summary line and the unified-diff body (`--- a/`, `-old`, `+new`). ### Verification cargo fmt --all -- --check ✓ cargo clippy --workspace --all-targets --all-features --locked -- -D warnings ✓ cargo test --workspace --all-features --locked ✓ (1824 + supporting; was 1820) Closes #505 Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 7 +++ crates/tui/Cargo.toml | 1 + crates/tui/src/tools/diff_format.rs | 77 +++++++++++++++++++++++++++++ crates/tui/src/tools/file.rs | 67 ++++++++++++++++++++----- crates/tui/src/tools/mod.rs | 1 + 5 files changed, 142 insertions(+), 11 deletions(-) create mode 100644 crates/tui/src/tools/diff_format.rs diff --git a/Cargo.lock b/Cargo.lock index 7c6e8864..bb2d7aa4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1269,6 +1269,7 @@ dependencies = [ "sha2 0.10.9", "shellexpand", "shlex", + "similar", "starlark", "tar", "tempfile", @@ -4607,6 +4608,12 @@ version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e320a6c5ad31d271ad523dcf3ad13e2767ad8b1cb8f047f75a8aeaf8da139da2" +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "siphasher" version = "1.0.1" diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index cd501fe0..0b03fdf8 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -39,6 +39,7 @@ indicatif = "0.18.0" ratatui = "0.29" regex = "1.11" reqwest = { version = "0.13.1", default-features = false, features = ["blocking", "json", "stream", "multipart", "native-tls", "http2"] } +similar = "2" rustyline = "15.0.0" serde = { version = "1.0.228", features = ["derive"] } serde_json = { version = "1.0.149", features = ["preserve_order"] } diff --git a/crates/tui/src/tools/diff_format.rs b/crates/tui/src/tools/diff_format.rs new file mode 100644 index 00000000..bb3549b2 --- /dev/null +++ b/crates/tui/src/tools/diff_format.rs @@ -0,0 +1,77 @@ +//! Build unified-diff strings for tool results. +//! +//! `edit_file` and `write_file` capture the file contents before and after +//! the mutation and emit a unified diff at the head of their `ToolResult` +//! output. The TUI's `output_looks_like_diff` detector then routes the +//! payload through `diff_render::render_diff`, which renders it with line +//! numbers and coloured `+`/`-` gutters (#505). +//! +//! The diff is also a strict UX upgrade for the model — it sees exactly +//! which lines changed instead of a one-line summary. + +use similar::TextDiff; + +/// Build a unified diff between `old` and `new` keyed at `path`. +/// +/// Returns an empty string when the inputs are byte-identical so callers +/// can skip the "no changes" header. The output uses git-style `--- a/...` +/// / `+++ b/...` headers and three lines of context — matching the format +/// the TUI's `diff_render::render_diff` already understands. +#[must_use] +pub fn make_unified_diff(path: &str, old: &str, new: &str) -> String { + if old == new { + return String::new(); + } + let a = format!("a/{path}"); + let b = format!("b/{path}"); + let diff = TextDiff::from_lines(old, new); + diff.unified_diff() + .context_radius(3) + .header(&a, &b) + .to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn identical_inputs_emit_empty_diff() { + let s = "hello\nworld\n"; + assert!(make_unified_diff("foo.txt", s, s).is_empty()); + } + + #[test] + fn replacement_emits_minus_plus_pair() { + let old = "alpha\nbeta\ngamma\n"; + let new = "alpha\nBETA\ngamma\n"; + let diff = make_unified_diff("foo.txt", old, new); + assert!(diff.contains("--- a/foo.txt"), "{diff}"); + assert!(diff.contains("+++ b/foo.txt"), "{diff}"); + assert!(diff.contains("-beta"), "{diff}"); + assert!(diff.contains("+BETA"), "{diff}"); + } + + #[test] + fn new_file_renders_against_empty_old() { + let new = "first line\nsecond line\n"; + let diff = make_unified_diff("new.txt", "", new); + assert!(diff.contains("--- a/new.txt"), "{diff}"); + assert!(diff.contains("+++ b/new.txt"), "{diff}"); + assert!(diff.contains("+first line"), "{diff}"); + assert!(diff.contains("+second line"), "{diff}"); + } + + #[test] + fn diff_contains_hunk_header_so_tui_renders_it() { + // The TUI detector scans the first 5 lines for `@@`. Make sure the + // unified diff puts a hunk header within that window so the + // diff-aware renderer kicks in (#505). + let diff = make_unified_diff("foo.txt", "a\n", "b\n"); + let head: Vec<&str> = diff.lines().take(5).collect(); + assert!( + head.iter().any(|line| line.starts_with("@@")), + "expected hunk header in first 5 lines; got {head:?}" + ); + } +} diff --git a/crates/tui/src/tools/file.rs b/crates/tui/src/tools/file.rs index 5a56d59f..1218a26c 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -3,6 +3,7 @@ //! These tools provide safe file system operations within the workspace, //! with path validation to prevent escaping the workspace boundary. +use super::diff_format::make_unified_diff; use super::spec::{ ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, optional_str, required_str, @@ -228,6 +229,15 @@ impl ToolSpec for WriteFileTool { let file_path = context.resolve_path(path_str)?; + // Snapshot the existing contents (if any) before we overwrite — used + // to render an inline diff in the tool result. + let existed_before = file_path.exists(); + let prior_contents = if existed_before { + fs::read_to_string(&file_path).unwrap_or_default() + } else { + String::new() + }; + // Create parent directories if needed if let Some(parent) = file_path.parent() { fs::create_dir_all(parent).map_err(|e| { @@ -243,11 +253,20 @@ impl ToolSpec for WriteFileTool { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) })?; - Ok(ToolResult::success(format!( - "Wrote {} bytes to {}", - file_content.len(), - file_path.display() - ))) + let display = file_path.display().to_string(); + let diff = make_unified_diff(&display, &prior_contents, file_content); + let summary = if existed_before { + format!("Wrote {} bytes to {}", file_content.len(), display) + } else { + format!("Created {} ({} bytes)", display, file_content.len()) + }; + let body = if diff.is_empty() { + format!("{summary}\n(no changes)") + } else { + format!("{diff}\n{summary}") + }; + + Ok(ToolResult::success(body)) } } @@ -324,11 +343,16 @@ impl ToolSpec for EditFileTool { ToolError::execution_failed(format!("Failed to write {}: {}", file_path.display(), e)) })?; - Ok(ToolResult::success(format!( - "Replaced {} occurrence(s) in {}", - count, - file_path.display() - ))) + let display = file_path.display().to_string(); + let diff = make_unified_diff(&display, &contents, &updated); + let summary = format!("Replaced {count} occurrence(s) in {display}"); + let body = if diff.is_empty() { + format!("{summary}\n(no textual changes)") + } else { + format!("{diff}\n{summary}") + }; + + Ok(ToolResult::success(body)) } } @@ -527,7 +551,15 @@ mod tests { .expect("execute"); assert!(result.success); - assert!(result.content.contains("Wrote")); + // New file → "Created …" summary; the unified diff above the summary + // primes the TUI's diff-aware renderer (#505). + assert!(result.content.contains("Created"), "{}", result.content); + assert!(result.content.contains("--- a/"), "{}", result.content); + assert!( + result.content.contains("+test content"), + "{}", + result.content + ); // Verify file was written let written = fs::read_to_string(tmp.path().join("output.txt")).expect("read"); @@ -575,6 +607,19 @@ mod tests { assert!(result.success); assert!(result.content.contains("2 occurrence(s)")); + // Inline diff (#505) — the unified diff lands above the summary + // line so the TUI's diff-aware renderer kicks in. + assert!(result.content.contains("--- a/"), "{}", result.content); + assert!( + result.content.contains("-hello world hello"), + "{}", + result.content + ); + assert!( + result.content.contains("+hi world hi"), + "{}", + result.content + ); // Verify edit was applied let edited = fs::read_to_string(&test_file).expect("read"); diff --git a/crates/tui/src/tools/mod.rs b/crates/tui/src/tools/mod.rs index 1394521e..9ea0826a 100644 --- a/crates/tui/src/tools/mod.rs +++ b/crates/tui/src/tools/mod.rs @@ -4,6 +4,7 @@ pub mod apply_patch; pub mod approval_cache; pub mod automation; pub mod diagnostics; +pub mod diff_format; pub mod file; pub mod file_search; pub mod finance;