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;