clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-04-24 22:54:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat: show diff for apply_patch in a nicer way

Browse Source
This commit is contained in:
Michael Bolin
2025-08-06 17:54:07 -07:00
parent a1105a0087
commit 497b41d1a1
7 changed files with 277 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

138
codex-rs/core/src/apply_patch.rs
View File

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

41
codex-rs/core/src/protocol.rs
View File

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

7
codex-rs/core/src/turn_diff_tracker.rs
View File

@@ -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);

1
codex-rs/exec/src/event_processor_with_human_output.rs
View File

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

1
codex-rs/mcp-server/tests/codex_tool.rs
View File

@@ -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,
},
);

1
codex-rs/tui/src/app.rs
View File

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

97
codex-rs/tui/src/history_cell.rs
View File

@@ -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<PathBuf, FileChange>) -> Vec<Line<'stati
Update {
unified_diff,
move_path,
hunks,
} => {
if let Some(new_path) = move_path {
summaries.push(Line::from(vec![
@@ -744,8 +747,13 @@ fn create_diff_summary(changes: HashMap<PathBuf, FileChange>) -> Vec<Line<'stati
]));
}
// Render unified diff as inline diff with line numbers.
summaries.extend(format_inline_diff(unified_diff));
// 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));
}
}
}
}
@@ -787,9 +795,13 @@ fn format_inline_diff(unified_diff: &str) -> Vec<Line<'static>> {
}
// 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<Line<'static>> {
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