From a1105a00872a0d5f94e75e5841590325f003be2d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 6 Aug 2025 17:11:58 -0700 Subject: [PATCH 1/2] feat: update UI for showing a diff --- codex-rs/tui/src/history_cell.rs | 236 +++++++++++++++++++++++++++---- 1 file changed, 205 insertions(+), 31 deletions(-) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index c577ce17a0..dee9e1273a 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -618,30 +618,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 +685,67 @@ impl WidgetRef for &HistoryCell { } } -fn create_diff_summary(changes: HashMap) -> Vec { +fn create_diff_summary(changes: HashMap) -> Vec> { // 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 = Vec::new(); + let mut summaries: Vec> = 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, } => { 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())), + ])); } - summaries.extend(unified_diff.lines().map(|s| s.to_string())); + + // Render unified diff as inline diff with line numbers. + summaries.extend(format_inline_diff(unified_diff)); } } } @@ -740,6 +753,167 @@ fn create_diff_summary(changes: HashMap) -> Vec { 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> { + // 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> = 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 +} + +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 From 497b41d1a11698c95ba5944a250a1b55215e1607 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 6 Aug 2025 17:54:07 -0700 Subject: [PATCH 2/2] feat: show diff for apply_patch in a nicer way --- codex-rs/core/src/apply_patch.rs | 138 +++++++++++++++++- codex-rs/core/src/protocol.rs | 41 ++++++ codex-rs/core/src/turn_diff_tracker.rs | 7 + .../src/event_processor_with_human_output.rs | 1 + codex-rs/mcp-server/tests/codex_tool.rs | 1 + codex-rs/tui/src/app.rs | 1 + codex-rs/tui/src/history_cell.rs | 97 +++++++++++- 7 files changed, 277 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index dc11aed023..0410a517a4 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/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> { + let mut hunks: Vec = Vec::new(); + let mut cur: Option = 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 { let mut writable_roots = Vec::new(); if cfg!(target_os = "macos") { diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 052806dd97..b141e42aa5 100644 --- a/codex-rs/core/src/protocol.rs +++ b/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, +} + #[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, + /// 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>, }, } diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs index 7026d7bb32..07b675d46c 100644 --- a/codex-rs/core/src/turn_diff_tracker.rs +++ b/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); diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 6b03ed7882..c2735f6f9b 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/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!( diff --git a/codex-rs/mcp-server/tests/codex_tool.rs b/codex-rs/mcp-server/tests/codex_tool.rs index fc992a8cd1..dfab97f2b9 100644 --- a/codex-rs/mcp-server/tests/codex_tool.rs +++ b/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, }, ); diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 23e12be38f..21bf3c74a2 100644 --- a/codex-rs/tui/src/app.rs +++ b/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, }, ), ]), diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index dee9e1273a..3bd78ff4e5 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/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; @@ -719,6 +721,7 @@ fn create_diff_summary(changes: HashMap) -> Vec { if let Some(new_path) = move_path { summaries.push(Line::from(vec![ @@ -744,8 +747,13 @@ fn create_diff_summary(changes: HashMap) -> Vec Vec> { } // 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('+') - { + 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), @@ -829,6 +841,81 @@ fn format_inline_diff(unified_diff: &str) -> Vec> { 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> { + // 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> = 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