Add a TurnDiffTracker to create a unified diff for an entire turn #1770
Conversation
github-actions
bot
added
codex-rust-review-in-progress
and removed
codex-rust-review
SummaryAdds Notes
ReviewNice feature – delivers running diffs and keeps core compile-clean. A few suggestions:
Otherwise, looks solid – great work! |
github-actions
bot
added
codex-rust-review-completed
and removed
codex-rust-review-in-progress
labels
| @@ -525,6 +527,11 @@ pub struct PatchApplyEndEvent { | |||
| pub success: bool, | |||
| } | |||
|
|
|||
| #[derive(Debug, Clone, Deserialize, Serialize)] | |||
| pub struct TurnDiffEvent { | |||
| pub unified_diff: String, | |||
There was a problem hiding this comment.
I feel like this would be easier to work with programmatically if this were keyed by path, more like changes in PatchApplyBeginEvent. Maybe for a full add or a full delete for an individual file, we still want the unified diff, but it's nice to have added/modified/removed metadata for each path so it's easy to build a compact summary for the diff (maybe with +/- line counts)?
There was a problem hiding this comment.
What guarantees, if any, can we make about the paths in the unified_diff: will they all be absolute paths?
There was a problem hiding this comment.
We don't currently need this because we parse the whole unified diff. Could we add it if/when we need it?
| @@ -1471,6 +1528,7 @@ fn maybe_run_with_user_profile(params: ExecParams, sess: &Session) -> ExecParams | |||
| async fn handle_container_exec_with_params( | |||
| params: ExecParams, | |||
| sess: &Session, | |||
| turn_diff_tracker: &mut TurnDiffTracker, | |||
There was a problem hiding this comment.
Hmm, what would happen if we wanted to support parallel tool calls at one point. This would be a problem, no?
There was a problem hiding this comment.
How do you think it would break?
There was a problem hiding this comment.
Because only one tool call could take ownership of TurnDiffTracker.
There was a problem hiding this comment.
I think we can solve this then. Maybe a Mutex or a channel or something, right?
| /// 1. Create a temp directory to store baseline snapshots of files when they are first seen. | ||
| /// 2. When a path is first observed, copy its current contents into the baseline dir if it exists on disk. | ||
| /// For new additions, do not create a baseline file so that diffs are shown as proper additions (using /dev/null). | ||
| /// 3. Keep a stable internal filename (uuid + same extension) per external path for path rewrite in diffs. | ||
| /// 4. To compute the aggregated unified diff, compare each baseline snapshot to the current file on disk using | ||
| /// `git diff --no-index` and rewrite paths to external paths. |
There was a problem hiding this comment.
I feel like https://crates.io/crates/similar or something would be a cleaner approach than maintaining a tmp dir; everything could be in-memory and there would be no need to rewrite anything.
There was a problem hiding this comment.
Good idea, gave it a shot and I think it's cleaner.
| let id = Uuid::new_v4().to_string(); | ||
| match path.extension().and_then(|e| e.to_str()) { | ||
| Some(ext) if !ext.is_empty() => format!("{id}.{ext}"), | ||
| _ => id, |
There was a problem hiding this comment.
| _ => id, | |
| None => id, |
| fn rewrite_diff_paths(&self, diff: &str) -> String { | ||
| let mut out = String::new(); | ||
| for line in diff.lines() { | ||
| if let Some(rest) = line.strip_prefix("diff --git ") { |
There was a problem hiding this comment.
This gets indented pretty far, so maybe it's worth moving to a helper function that takes (&mut String, temp_name_to_current_external, temp_name_to_baseline_external)
There was a problem hiding this comment.
Fully removed in the new version
| fs::write(&file, "foo\n").unwrap(); | ||
| acc.update_and_get_unified_diff().unwrap(); | ||
| let first = acc.unified_diff.clone().unwrap(); | ||
| assert!(first.contains("+foo")); |
There was a problem hiding this comment.
Instead of contains() checks, can these all be full assert_eq!() checks?
There was a problem hiding this comment.
It wasn't trivial because it included the TempDir path (I originally tried to do it that way). If I switch to similar, I can change this though!
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| #![allow(clippy::unwrap_used)] |
There was a problem hiding this comment.
Can you also test that paths with spaces work as intended?
| Ok(()) | ||
| } | ||
|
|
||
| /// Recompute the aggregated unified diff by comparing all baseline snapshots against |
There was a problem hiding this comment.
Can you expand this comment? I don't have a great mental model of the structure you're trying to set up for the ultimate git diff call.
I want to understand why this isn't something simpler like diff -u backed-up-file current-file.
There was a problem hiding this comment.
The version I'm about to push uses similar and does it all in-memory
gpeal
force-pushed
the
gpeal/patch-accumulator
branch
2 times, most recently
from
b366916 to
89fba63
Compare
gpeal
force-pushed
the
gpeal/patch-accumulator
branch
from
89fba63 to
7d354d7
Compare
This lets us show an accumulating diff across all patches in a turn. Refer to the docs for TurnDiffTracker for implementation details.
There are multiple ways this could have been done and this felt like the right tradeoff between reliability and completeness:
Pros
Cons