Add a TurnDiffTracker to create a unified diff for an entire turn #1770

gpeal · 2025-07-31T23:42:44Z

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

It will pick up all changes to files that the model touched including if they prettier or another command that updates them.
It will not pick up changes made by the user or other agents to files it didn't modify.

Cons

It will pick up changes that the user made to a file that the model also touched
It will not pick up changes to codegen or files that were not modified with apply_patch

github-actions · 2025-07-31T23:48:52Z

Summary

Adds TurnDiffTracker and a new TurnDiff event to aggregate and stream the unified diff for all file changes in a user turn; integrates tracker throughout the codex execution flow and moves tempfile to non-dev deps.

Notes

New turn_diff_tracker.rs (≈480 LoC) with tests.
TurnDiffEvent variant added to EventMsg; event emitted after every command / patch.
Codex flow updated (on_exec_command_* helpers) to drive the tracker.
tempfile promoted to [dependencies], ChatGPT-specific header removal in client.

Review

Nice feature – delivers running diffs and keeps core compile-clean. A few suggestions:

PR body lacks motivation/UX demo; please add a brief rationale and, if possible, a GIF or screenshot of the streamed diff.
Keep [dependencies] alpha-sorted: place tempfile before tree-sitter-bash.
turn_diff_tracker.rs is large; consider splitting helpers/tests into a sub-module to aid navigation.
Minor style: some if let chains in on_patch_begin could be flattened with pattern-matching for clarity.
Please update CHANGELOG and any external protocol docs to mention the new TurnDiff event.

Otherwise, looks solid – great work!

View workflow run

codex-rs/core/Cargo.toml

bolinfest · 2025-08-01T16:41:09Z

codex-rs/core/src/protocol.rs

@@ -525,6 +527,11 @@ pub struct PatchApplyEndEvent {
    pub success: bool,
 }

+#[derive(Debug, Clone, Deserialize, Serialize)]
+pub struct TurnDiffEvent {
+    pub unified_diff: String,


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)?

What guarantees, if any, can we make about the paths in the unified_diff: will they all be absolute paths?

We don't currently need this because we parse the whole unified diff. Could we add it if/when we need it?

bolinfest · 2025-08-01T16:48:22Z

codex-rs/core/src/codex.rs

@@ -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,


Hmm, what would happen if we wanted to support parallel tool calls at one point. This would be a problem, no?

How do you think it would break?

Because only one tool call could take ownership of TurnDiffTracker.

I think we can solve this then. Maybe a Mutex or a channel or something, right?

Yes, though also, if we introduce a struct TurnContext as mentioned above, that may also force the move to Mutex. But yes, does not have to be done in this PR.

nornagon-openai · 2025-08-01T16:48:56Z

codex-rs/core/src/turn_diff_tracker.rs

+/// 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.


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.

Good idea, gave it a shot and I think it's cleaner.

codex-rs/core/src/turn_diff_tracker.rs

bolinfest · 2025-08-01T16:57:12Z

codex-rs/core/src/turn_diff_tracker.rs

+    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,


Suggested change

_ => id,

None => id,

I'm still unclear why ext is added.

codex-rs/core/src/turn_diff_tracker.rs

bolinfest · 2025-08-01T17:08:30Z

codex-rs/core/src/turn_diff_tracker.rs

+    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 ") {


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)

Fully removed in the new version

codex-rs/core/src/turn_diff_tracker.rs

bolinfest · 2025-08-01T17:12:45Z

codex-rs/core/src/turn_diff_tracker.rs

+        fs::write(&file, "foo\n").unwrap();
+        acc.update_and_get_unified_diff().unwrap();
+        let first = acc.unified_diff.clone().unwrap();
+        assert!(first.contains("+foo"));


Instead of contains() checks, can these all be full assert_eq!() checks?

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!

bolinfest · 2025-08-01T17:13:43Z

codex-rs/core/src/turn_diff_tracker.rs

+
+#[cfg(test)]
+mod tests {
+    #![allow(clippy::unwrap_used)]


Can you also test that paths with spaces work as intended?

codex-rs/core/src/turn_diff_tracker.rs

bolinfest · 2025-08-01T17:20:55Z

codex-rs/core/src/turn_diff_tracker.rs

+        Ok(())
+    }
+
+    /// Recompute the aggregated unified diff by comparing all baseline snapshots against


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.

The version I'm about to push uses similar and does it all in-memory

bolinfest

I have a lot of comments, but they are generally small, so please fix and merge!

codex-rs/core/Cargo.toml

bolinfest · 2025-08-04T02:45:51Z

codex-rs/core/src/turn_diff_tracker.rs

+            };
+        }
+
+        Ok(())


I don't see a ? or a place where the Err variant is constructed, so does this need to return Result?

bolinfest · 2025-08-04T02:46:36Z

codex-rs/core/src/codex.rs

-                changes,
-            }),
+            }) => {
+                let _ = turn_diff_tracker.on_patch_begin(&changes);


If this doesn't have to return Result, then let _ can go away, of course, but depending on what sort of Err we expect, perhaps we should at least warn!() or error!()?

bolinfest · 2025-08-04T02:47:22Z

codex-rs/core/src/codex.rs

@@ -392,8 +402,10 @@ impl Session {
        let _ = self.tx_event.send(event).await;
    }

-    async fn notify_exec_command_end(
+    #[allow(clippy::too_many_arguments)]


We should maybe introduce a struct in a follow-up PR.

bolinfest · 2025-08-04T02:49:08Z

codex-rs/core/src/codex.rs

@@ -1163,6 +1193,7 @@ async fn run_task(sess: Arc<Session>, sub_id: String, input: Vec<InputItem>) {

 async fn run_turn(
    sess: &Session,
+    turn_diff_tracker: &mut TurnDiffTracker,


We'll probably want a struct TurnContext or somesuch in the near future.

bolinfest · 2025-08-04T03:08:45Z

codex-rs/core/src/turn_diff_tracker.rs

+
+const ZERO_OID: &str = "0000000000000000000000000000000000000000";
+const REGULAR_MODE: &str = "100644";
+#[cfg(unix)]


Though even on Windows, this has to be readable (and preserved?) in a Git tree object, no?

bolinfest · 2025-08-04T03:11:58Z

codex-rs/core/src/turn_diff_tracker.rs

+fn symlink_blob_bytes(path: &Path) -> Option<Vec<u8>> {
+    use std::os::unix::ffi::OsStrExt;
+    let target = std::fs::read_link(path).ok()?;
+    Some(target.as_os_str().as_bytes().to_vec())


One interesting operating system fact is that the contents of a symlink do not have to be a path to a file: you can just use it for arbitrary data storage. (As such, I think the max number of bytes you can store in a symlink is PATH_MAX, though.)

I knew of one project that did this to save a system call because readlink() is one system call but open() plus read() for a regular file is two?

bolinfest · 2025-08-04T03:12:56Z

codex-rs/core/src/turn_diff_tracker.rs

+fn git_blob_sha1_hex_bytes(data: &[u8]) -> String {
+    // Git blob hash is sha1 of: "blob <len>\0<data>"
+    let header = format!("blob {}\0", data.len());
+    use sha1::Digest;


Returning this type is slightly stronger since you don't have to verify the integrity of the String contents elsewhere.

bolinfest · 2025-08-04T03:13:59Z

codex-rs/core/src/turn_diff_tracker.rs

+            let mode = file_mode_for_path(&file).unwrap_or_else(|| REGULAR_MODE.to_string());
+            let right_oid = git_blob_sha1_hex("foo\n");
+            format!(
+                "diff --git a/<TMP>/a.txt b/<TMP>/a.txt\nnew file mode {mode}\nindex {ZERO_OID}..{right_oid}\n--- /dev/null\n+++ b/<TMP>/a.txt\n@@ -0,0 +1 @@\n+foo\n",


r# for better readability?

bolinfest · 2025-08-04T03:14:59Z

codex-rs/core/src/turn_diff_tracker.rs

+        // No file existed initially; create only dest
+        fs::write(&dest, "hello\n").unwrap();
+        let diff = acc.get_unified_diff().unwrap().unwrap();
+        assert!(diff.contains("new file mode"));


We can't assert_eq!() here?

gpeal added 8 commits July 30, 2025 21:36

First shot

ea4224b

WIP temp repo

3793670

Cleanup

38e0461

Emit TurnDiff

756708a

Tests pass

b249e50

Updated for non-git

eeaf2ca

Merge remote-tracking branch 'origin/main' into gpeal/patch-accumulator

fa861e5

Emit events. Builds

50c408b

gpeal added the codex-rust-review Perform a detailed review of Rust changes. label Jul 31, 2025

github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Jul 31, 2025

Spelling

c8e3763

github-actions bot added codex-rust-review-completed and removed codex-rust-review-in-progress labels Jul 31, 2025

gpeal marked this pull request as ready for review July 31, 2025 23:51

bolinfest reviewed Aug 1, 2025

View reviewed changes

nornagon-openai reviewed Aug 1, 2025

View reviewed changes

bolinfest requested changes Aug 1, 2025

View reviewed changes

gpeal added 10 commits August 1, 2025 14:35

Migrated to similar

e3f8b12

assert_eq

6bd7ef3

Added index

feeb266

Find git root works

69fb639

Add sha1

75bab53

Remove field

ffdc6fb

Merge remote-tracking branch 'origin/main' into gpeal/patch-accumulator

3425753

Get actual oid

de85ee8

Migrate to BaselineFileInfo

c52f7ef

Better handling for binary files and windows

02a7221

gpeal added 2 commits August 2, 2025 18:44

Merge remote-tracking branch 'origin/main' into gpeal/patch-accumulator

4dc6364

CI

df0f792

gpeal force-pushed the gpeal/patch-accumulator branch 2 times, most recently from b366916 to 89fba63 Compare August 3, 2025 14:22

Improved symlink handling

7d354d7

gpeal force-pushed the gpeal/patch-accumulator branch from 89fba63 to 7d354d7 Compare August 3, 2025 14:22

gpeal requested a review from bolinfest August 3, 2025 14:22

gpeal added 2 commits August 3, 2025 21:02

Clippy

592e253

Merge remote-tracking branch 'origin/main' into gpeal/patch-accumulator

d9e9063

bolinfest approved these changes Aug 4, 2025

View reviewed changes

+                          };
+                      }
+                      Ok(())

Add a TurnDiffTracker to create a unified diff for an entire turn #1770

Are you sure you want to change the base?

Add a TurnDiffTracker to create a unified diff for an entire turn #1770

Conversation

gpeal commented Jul 31, 2025 • edited by bolinfest Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 31, 2025

Summary

Notes

Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gpeal commented Jul 31, 2025 •

edited by bolinfest

Loading