feat: show diff for apply_patch in a nicer way

codex-rs/core/src/apply_patch.rs

@@ -1,6 +1,9 @@
 use crate::codex::Session;
 use crate::models::FunctionCallOutputPayload;
 use crate::models::ResponseInputItem;
+use crate::protocol::DiffHunk;
+use crate::protocol::DiffLine;
+use crate::protocol::DiffLineKind;
 use crate::protocol::FileChange;
 use crate::protocol::ReviewDecision;
 use crate::safety::SafetyCheck;
@@ -119,16 +122,143 @@ pub(crate) fn convert_apply_patch_to_protocol(
                 unified_diff,
                 move_path,
                 new_content: _new_content,
-            } => FileChange::Update {
-                unified_diff: unified_diff.clone(),
-                move_path: move_path.clone(),
-            },
+            } => {
+                let hunks = parse_unified_diff_to_hunks(unified_diff).unwrap_or_default();
+                FileChange::Update {
+                    unified_diff: unified_diff.clone(),
+                    move_path: move_path.clone(),
+                    hunks: if hunks.is_empty() { None } else { Some(hunks) },
+                }
+            }
         };
         result.insert(path.clone(), protocol_change);
     }
     result
 }
 
+/// Parse a unified diff string into structured hunks. The input is expected to
+/// contain one or more hunk headers (lines starting with "@@") followed by hunk
+/// bodies. Lines starting with '+++', '---' (file headers) are ignored.
+fn parse_unified_diff_to_hunks(src: &str) -> Option<Vec<DiffHunk>> {
+    let mut hunks: Vec<DiffHunk> = Vec::new();
+    let mut cur: Option<DiffHunk> = None;
+
+    for line in src.lines() {
+        if line.starts_with("---") || line.starts_with("+++") {
+            // File headers: ignore.
+            continue;
+        }
+
+        if let Some((old_start, old_count, new_start, new_count)) = parse_hunk_header(line) {
+            // Flush previous hunk
+            if let Some(h) = cur.take() {
+                hunks.push(h);
+            }
+            cur = Some(DiffHunk {
+                old_start: old_start as u32,
+                old_count: old_count as u32,
+                new_start: new_start as u32,
+                new_count: new_count as u32,
+                lines: Vec::new(),
+            });
+            continue;
+        }
+
+        if let Some(h) = cur.as_mut() {
+            // Classify by prefix; store text without the prefix when present.
+            let (kind, text) = if let Some(rest) = line.strip_prefix('+') {
+                (DiffLineKind::Add, rest.to_string())
+            } else if let Some(rest) = line.strip_prefix('-') {
+                (DiffLineKind::Delete, rest.to_string())
+            } else if let Some(rest) = line.strip_prefix(' ') {
+                (DiffLineKind::Context, rest.to_string())
+            } else {
+                // Non-standard line inside hunk; keep as context with full text.
+                (DiffLineKind::Context, line.to_string())
+            };
+            h.lines.push(DiffLine { kind, text });
+        }
+    }
+
+    if let Some(h) = cur.take() {
+        hunks.push(h);
+    }
+
+    Some(hunks)
+}
+
+// Lightweight parsing of a unified diff hunk header of the form:
+// @@ -oldStart,oldCount +newStart,newCount @@
+// Counts may be omitted which implies 1.
+fn parse_hunk_header(line: &str) -> Option<(u64, u64, u64, u64)> {
+    if !line.starts_with("@@") {
+        return None;
+    }
+    let bytes = line.as_bytes();
+    let mut i = 2usize;
+    // skip spaces
+    while i < bytes.len() && bytes[i].is_ascii_whitespace() {
+        i += 1;
+    }
+    if i >= bytes.len() || bytes[i] != b'-' {
+        return None;
+    }
+    i += 1;
+    let (old_start, c1) = parse_uint(&bytes[i..]);
+    if c1 == 0 {
+        return None;
+    }
+    i += c1;
+    let mut old_count = 1u64;
+    if i < bytes.len() && bytes[i] == b',' {
+        i += 1;
+        let (n, c) = parse_uint(&bytes[i..]);
+        if c == 0 {
+            return None;
+        }
+        old_count = n;
+        i += c;
+    }
+    while i < bytes.len() && bytes[i].is_ascii_whitespace() {
+        i += 1;
+    }
+    if i >= bytes.len() || bytes[i] != b'+' {
+        return None;
+    }
+    i += 1;
+    let (new_start, c2) = parse_uint(&bytes[i..]);
+    if c2 == 0 {
+        return None;
+    }
+    i += c2;
+    let mut new_count = 1u64;
+    if i < bytes.len() && bytes[i] == b',' {
+        i += 1;
+        let (n, c) = parse_uint(&bytes[i..]);
+        if c == 0 {
+            return None;
+        }
+        new_count = n;
+        i += c;
+    }
+    Some((old_start, old_count, new_start, new_count))
+}
+
+fn parse_uint(s: &[u8]) -> (u64, usize) {
+    let mut i = 0usize;
+    let mut n: u64 = 0;
+    while i < s.len() {
+        let b = s[i];
+        if b.is_ascii_digit() {
+            n = n * 10 + (b - b'0') as u64;
+            i += 1;
+        } else {
+            break;
+        }
+    }
+    (n, i)
+}
+
 pub(crate) fn get_writable_roots(cwd: &Path) -> Vec<PathBuf> {
     let mut writable_roots = Vec::new();
     if cfg!(target_os = "macos") {

codex-rs/core/src/protocol.rs

@@ -626,6 +626,41 @@ pub struct PatchApplyEndEvent {
     pub success: bool,
 }
 
+/// Kind of a single line in a diff hunk body.
+#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
+#[serde(rename_all = "snake_case")]
+pub enum DiffLineKind {
+    /// Line was inserted (prefixed with '+').
+    Add,
+    /// Line was deleted (prefixed with '-').
+    Delete,
+    /// Context line (no prefix or a single space in unified diff output).
+    Context,
+}
+
+/// A single line within a diff hunk.
+#[derive(Debug, Clone, Deserialize, Serialize)]
+pub struct DiffLine {
+    pub kind: DiffLineKind,
+    /// Text content of the line without the unified diff prefix character.
+    pub text: String,
+}
+
+/// Structured representation of a unified diff hunk.
+#[derive(Debug, Clone, Deserialize, Serialize)]
+pub struct DiffHunk {
+    /// 1-based starting line number in the original file.
+    pub old_start: u32,
+    /// Number of lines in the original file that the hunk covers.
+    pub old_count: u32,
+    /// 1-based starting line number in the new file.
+    pub new_start: u32,
+    /// Number of lines in the new file that the hunk covers.
+    pub new_count: u32,
+    /// Lines within this hunk in unified diff order.
+    pub lines: Vec<DiffLine>,
+}
+
 #[derive(Debug, Clone, Deserialize, Serialize)]
 pub struct TurnDiffEvent {
     pub unified_diff: String,
@@ -685,8 +720,14 @@ pub enum FileChange {
     },
     Delete,
     Update {
+        /// Unified diff for this file change. Retained for compatibility with
+        /// older clients that do not support structured hunks.
         unified_diff: String,
         move_path: Option<PathBuf>,
+        /// Optional structured representation of the diff hunks to avoid
+        /// requiring clients to parse unified diff syntax.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        hunks: Option<Vec<DiffHunk>>,
     },
 }
 

codex-rs/core/src/turn_diff_tracker.rs

@@ -546,6 +546,7 @@ index {ZERO_OID}..{right_oid}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: None,
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&update_changes);
@@ -616,6 +617,7 @@ index {left_oid}..{ZERO_OID}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: Some(dest.clone()),
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&mv_changes);
@@ -656,6 +658,7 @@ index {left_oid}..{right_oid}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: Some(dest.clone()),
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&mv_changes);
@@ -678,6 +681,7 @@ index {left_oid}..{right_oid}
             FileChange::Update {
                 unified_diff: "".into(),
                 move_path: Some(dest.clone()),
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&mv);
@@ -718,6 +722,7 @@ index {ZERO_OID}..{right_oid}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: None,
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&update_a);
@@ -793,6 +798,7 @@ index {left_oid_b}..{ZERO_OID}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: None,
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&update_changes);
@@ -859,6 +865,7 @@ index {ZERO_OID}..{right_oid}
             FileChange::Update {
                 unified_diff: "".to_owned(),
                 move_path: None,
+                hunks: None,
             },
         )]);
         acc.on_patch_begin(&update_changes);

codex-rs/exec/src/event_processor_with_human_output.rs

@@ -397,6 +397,7 @@ impl EventProcessor for EventProcessorWithHumanOutput {
                         FileChange::Update {
                             unified_diff,
                             move_path,
+                            ..
                         } => {
                             let header = if let Some(dest) = move_path {
                                 format!(

codex-rs/mcp-server/tests/codex_tool.rs

@@ -236,6 +236,7 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> {
         FileChange::Update {
             unified_diff: "@@ -1 +1 @@\n-original content\n+modified content\n".to_string(),
             move_path: None,
+            hunks: None,
         },
     );
 

codex-rs/tui/src/app.rs

@@ -412,6 +412,7 @@ impl App<'_> {
                                             FileChange::Update {
                                                 unified_diff: "+test\n-test2".to_string(),
                                                 move_path: None,
+                                                hunks: None,
                                             },
                                         ),
                                     ]),

codex-rs/tui/src/history_cell.rs

@@ -11,6 +11,8 @@ use codex_core::config::Config;
 use codex_core::plan_tool::PlanItemArg;
 use codex_core::plan_tool::StepStatus;
 use codex_core::plan_tool::UpdatePlanArgs;
+use codex_core::protocol::DiffHunk;
+use codex_core::protocol::DiffLineKind;
 use codex_core::protocol::FileChange;
 use codex_core::protocol::McpInvocation;
 use codex_core::protocol::SessionConfiguredEvent;
@@ -618,30 +620,7 @@ impl HistoryCell {
         lines.push(Line::from(title.magenta().bold()));
 
         for line in summary_lines {
-            if line.starts_with('+') {
-                lines.push(line.green().into());
-            } else if line.starts_with('-') {
-                lines.push(line.red().into());
-            } else if let Some(space_idx) = line.find(' ') {
-                let kind_owned = line[..space_idx].to_string();
-                let rest_owned = line[space_idx + 1..].to_string();
-
-                let style_for = |fg: Color| Style::default().fg(fg).add_modifier(Modifier::BOLD);
-
-                let styled_kind = match kind_owned.as_str() {
-                    "A" => RtSpan::styled(kind_owned.clone(), style_for(Color::Green)),
-                    "D" => RtSpan::styled(kind_owned.clone(), style_for(Color::Red)),
-                    "M" => RtSpan::styled(kind_owned.clone(), style_for(Color::Yellow)),
-                    "R" | "C" => RtSpan::styled(kind_owned.clone(), style_for(Color::Cyan)),
-                    _ => RtSpan::raw(kind_owned.clone()),
-                };
-
-                let styled_line =
-                    RtLine::from(vec![styled_kind, RtSpan::raw(" "), RtSpan::raw(rest_owned)]);
-                lines.push(styled_line);
-            } else {
-                lines.push(Line::from(line));
-            }
+            lines.push(line);
         }
 
         lines.push(Line::from(""));
@@ -708,31 +687,73 @@ impl WidgetRef for &HistoryCell {
     }
 }
 
-fn create_diff_summary(changes: HashMap<PathBuf, FileChange>) -> Vec<String> {
+fn create_diff_summary(changes: HashMap<PathBuf, FileChange>) -> Vec<Line<'static>> {
     // Build a concise, human‑readable summary list similar to the
     // `git status` short format so the user can reason about the
     // patch without scrolling.
-    let mut summaries: Vec<String> = Vec::new();
+    let mut summaries: Vec<Line<'static>> = Vec::new();
     for (path, change) in &changes {
         use codex_core::protocol::FileChange::*;
         match change {
             Add { content } => {
                 let added = content.lines().count();
-                summaries.push(format!("A {} (+{added})", path.display()));
+                summaries.push(Line::from(vec![
+                    RtSpan::styled(
+                        "A",
+                        Style::default()
+                            .fg(Color::Green)
+                            .add_modifier(Modifier::BOLD),
+                    ),
+                    RtSpan::raw(" "),
+                    RtSpan::raw(format!("{} (+{added})", path.display())),
+                ]));
             }
             Delete => {
-                summaries.push(format!("D {}", path.display()));
+                summaries.push(Line::from(vec![
+                    RtSpan::styled(
+                        "D",
+                        Style::default().fg(Color::Red).add_modifier(Modifier::BOLD),
+                    ),
+                    RtSpan::raw(" "),
+                    RtSpan::raw(format!("{}", path.display())),
+                ]));
             }
             Update {
                 unified_diff,
                 move_path,
+                hunks,
             } => {
                 if let Some(new_path) = move_path {
-                    summaries.push(format!("R {} → {}", path.display(), new_path.display(),));
+                    summaries.push(Line::from(vec![
+                        RtSpan::styled(
+                            "R",
+                            Style::default()
+                                .fg(Color::Cyan)
+                                .add_modifier(Modifier::BOLD),
+                        ),
+                        RtSpan::raw(" "),
+                        RtSpan::raw(format!("{} → {}", path.display(), new_path.display())),
+                    ]));
                 } else {
-                    summaries.push(format!("M {}", path.display(),));
+                    summaries.push(Line::from(vec![
+                        RtSpan::styled(
+                            "M",
+                            Style::default()
+                                .fg(Color::Yellow)
+                                .add_modifier(Modifier::BOLD),
+                        ),
+                        RtSpan::raw(" "),
+                        RtSpan::raw(format!("{}", path.display())),
+                    ]));
+                }
+
+                // Prefer structured hunks if provided; otherwise, fall back
+                // to parsing the unified diff string.
+                if let Some(hunks) = hunks {
+                    summaries.extend(format_inline_hunks(hunks));
+                } else {
+                    summaries.extend(format_inline_diff(unified_diff));
                 }
-                summaries.extend(unified_diff.lines().map(|s| s.to_string()));
             }
         }
     }
@@ -740,6 +761,246 @@ fn create_diff_summary(changes: HashMap<PathBuf, FileChange>) -> Vec<String> {
     summaries
 }
 
+/// Convert a unified diff (only hunk bodies expected) into inline diff lines
+/// with old/new line numbers and colored +/- content.
+fn format_inline_diff(unified_diff: &str) -> Vec<Line<'static>> {
+    // First pass: compute max widths for old/new line numbers from hunk headers
+    let mut max_old: usize = 0;
+    let mut max_new: usize = 0;
+    for l in unified_diff.lines() {
+        if let Some(caps) = parse_hunk_header(l) {
+            let (o_start, o_count, n_start, n_count) = caps;
+            let o_end = o_start + o_count.saturating_sub(1);
+            let n_end = n_start + n_count.saturating_sub(1);
+            max_old = max_old.max(o_end as usize);
+            max_new = max_new.max(n_end as usize);
+        }
+    }
+    let old_w = std::cmp::max(2, num_width(max_old as u64));
+    let new_w = std::cmp::max(2, num_width(max_new as u64));
+
+    let mut old_ln: i64 = 0;
+    let mut new_ln: i64 = 0;
+    let mut out: Vec<Line<'static>> = Vec::new();
+
+    for raw in unified_diff.lines() {
+        if let Some((o_start, _o_count, n_start, _n_count)) = parse_hunk_header(raw) {
+            old_ln = o_start as i64;
+            new_ln = n_start as i64;
+            out.push(Line::from(RtSpan::styled(
+                raw.to_string(),
+                Style::default().fg(Color::Cyan),
+            )));
+            continue;
+        }
+
+        // Prepare number columns and content coloring based on first char
+        let (left_num, right_num, content_style, advance_old, advance_new): (
+            String,
+            String,
+            Style,
+            i64,
+            i64,
+        ) = if raw.starts_with('+') {
+            (
+                " ".repeat(old_w),
+                format!("{:>width$}", new_ln, width = new_w),
+                Style::default().fg(Color::Green),
+                0,
+                1,
+            )
+        } else if raw.starts_with('-') {
+            (
+                format!("{:>width$}", old_ln, width = old_w),
+                " ".repeat(new_w),
+                Style::default().fg(Color::Red),
+                1,
+                0,
+            )
+        } else {
+            (
+                format!("{:>width$}", old_ln, width = old_w),
+                format!("{:>width$}", new_ln, width = new_w),
+                Style::default(),
+                1,
+                1,
+            )
+        };
+
+        let num_style = Style::default().fg(Color::DarkGray);
+        let sep = RtSpan::styled(" | ", num_style);
+        let left = RtSpan::styled(left_num, num_style);
+        let right = RtSpan::styled(right_num, num_style);
+        let content = RtSpan::styled(raw.to_string(), content_style);
+        out.push(RtLine::from(vec![left, sep.clone(), right, sep, content]));
+
+        old_ln += advance_old;
+        new_ln += advance_new;
+    }
+
+    out
+}
+
+/// Render structured hunks into inline diff with numbered columns, similar to
+/// `format_inline_diff` but without parsing unified diff text.
+fn format_inline_hunks(hunks: &[DiffHunk]) -> Vec<Line<'static>> {
+    // Compute max widths for columns based on hunk ranges.
+    let mut max_old_end: u32 = 0;
+    let mut max_new_end: u32 = 0;
+    for h in hunks {
+        let o_end = h.old_start.saturating_add(h.old_count.saturating_sub(1));
+        let n_end = h.new_start.saturating_add(h.new_count.saturating_sub(1));
+        max_old_end = max_old_end.max(o_end);
+        max_new_end = max_new_end.max(n_end);
+    }
+    let old_w = std::cmp::max(2, num_width(max_old_end as u64));
+    let new_w = std::cmp::max(2, num_width(max_new_end as u64));
+
+    let mut out: Vec<Line<'static>> = Vec::new();
+    let num_style = Style::default().fg(Color::DarkGray);
+
+    for h in hunks {
+        // Hunk header line
+        let header = format!(
+            "@@ -{},{} +{},{} @@",
+            h.old_start, h.old_count, h.new_start, h.new_count
+        );
+        out.push(Line::from(RtSpan::styled(header, Style::default().fg(Color::Cyan))));
+
+        let mut old_ln = h.old_start as i64;
+        let mut new_ln = h.new_start as i64;
+
+        for l in &h.lines {
+            let (left_num, right_num, content_style, advance_old, advance_new, prefix) =
+                match l.kind {
+                    DiffLineKind::Add => (
+                        " ".repeat(old_w),
+                        format!("{:>width$}", new_ln, width = new_w),
+                        Style::default().fg(Color::Green),
+                        0,
+                        1,
+                        '+',
+                    ),
+                    DiffLineKind::Delete => (
+                        format!("{:>width$}", old_ln, width = old_w),
+                        " ".repeat(new_w),
+                        Style::default().fg(Color::Red),
+                        1,
+                        0,
+                        '-',
+                    ),
+                    DiffLineKind::Context => (
+                        format!("{:>width$}", old_ln, width = old_w),
+                        format!("{:>width$}", new_ln, width = new_w),
+                        Style::default(),
+                        1,
+                        1,
+                        ' ',
+                    ),
+                };
+
+            let sep = RtSpan::styled(" | ", num_style);
+            let left = RtSpan::styled(left_num, num_style);
+            let right = RtSpan::styled(right_num, num_style);
+            let mut content = String::with_capacity(l.text.len() + 1);
+            content.push(prefix);
+            content.push_str(&l.text);
+            let content = RtSpan::styled(content, content_style);
+            out.push(RtLine::from(vec![left, sep.clone(), right, sep, content]));
+
+            old_ln += advance_old;
+            new_ln += advance_new;
+        }
+    }
+
+    out
+}
+
+fn parse_hunk_header(line: &str) -> Option<(u64, u64, u64, u64)> {
+    // @@ -oldStart,oldCount +newStart,newCount @@
+    // counts may be omitted which implies 1
+    if !line.starts_with("@@") {
+        return None;
+    }
+    let mut old_start: u64 = 0;
+    let mut old_count: u64 = 1;
+    let mut new_start: u64 = 0;
+    let mut new_count: u64 = 1;
+    // A simple, robust parse without regex
+    // Find "-" then space then "+" then space then "@@"
+    let bytes = line.as_bytes();
+    // Find first '-' after '@@'
+    let mut i = 2;
+    while i < bytes.len() && bytes[i].is_ascii_whitespace() {
+        i += 1;
+    }
+    if i >= bytes.len() || bytes[i] != b'-' {
+        return None;
+    }
+    i += 1;
+    // parse oldStart
+    let (n, consumed) = parse_uint(&bytes[i..]);
+    if consumed == 0 {
+        return None;
+    }
+    old_start = n;
+    i += consumed;
+    // optional ,oldCount
+    if i < bytes.len() && bytes[i] == b',' {
+        i += 1;
+        let (n2, c2) = parse_uint(&bytes[i..]);
+        if c2 == 0 {
+            return None;
+        }
+        old_count = n2;
+        i += c2;
+    }
+    // skip spaces
+    while i < bytes.len() && bytes[i].is_ascii_whitespace() {
+        i += 1;
+    }
+    if i >= bytes.len() || bytes[i] != b'+' {
+        return None;
+    }
+    i += 1;
+    let (n3, c3) = parse_uint(&bytes[i..]);
+    if c3 == 0 {
+        return None;
+    }
+    new_start = n3;
+    i += c3;
+    if i < bytes.len() && bytes[i] == b',' {
+        i += 1;
+        let (n4, c4) = parse_uint(&bytes[i..]);
+        if c4 == 0 {
+            return None;
+        }
+        new_count = n4;
+        i += c4;
+    }
+    Some((old_start, old_count, new_start, new_count))
+}
+
+fn parse_uint(s: &[u8]) -> (u64, usize) {
+    let mut i = 0usize;
+    let mut n: u64 = 0;
+    while i < s.len() {
+        let b = s[i];
+        if b.is_ascii_digit() {
+            n = n * 10 + (b - b'0') as u64;
+            i += 1;
+        } else {
+            break;
+        }
+    }
+    (n, i)
+}
+
+fn num_width(n: u64) -> usize {
+    // Simple and exact decimal width
+    n.to_string().len()
+}
+
 fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> {
     let args_str = invocation
         .arguments