From b9bba098197dff6f1dd2d6f64b87befc2dafbd0f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 28 Apr 2025 21:15:41 -0700 Subject: [PATCH] fix: eliminate runtime dependency on patch(1) for apply_patch (#718) When processing an `apply_patch` tool call, we were already computing the new file content in order to compute the unified diff. Before this PR, we were shelling out to `patch(1)` to apply the unified diff once the user accepted the change, but this updates the code to just retain the new file content and use it to write the file when the user accepts. This simplifies deployment because it no longer assumes `patch(1)` is on the host. Note this change is internal to the Codex agent and does not affect `protocol.rs`. --- codex-rs/apply-patch/src/lib.rs | 56 +++++++++++++++++++++++++++------ codex-rs/core/src/codex.rs | 28 ++++------------- 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index bd9e4044..090eab18 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -86,6 +86,8 @@ pub enum ApplyPatchFileChange { Update { unified_diff: String, move_path: Option, + /// new_content that will result after the unified_diff is applied. + new_content: String, }, } @@ -126,7 +128,10 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif move_path, chunks, } => { - let unified_diff = match unified_diff_from_chunks(&path, &chunks) { + let ApplyPatchFileUpdate { + unified_diff, + content: contents, + } = match unified_diff_from_chunks(&path, &chunks) { Ok(diff) => diff, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError(e); @@ -137,6 +142,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif ApplyPatchFileChange::Update { unified_diff, move_path, + new_content: contents, }, ); } @@ -516,10 +522,17 @@ fn apply_replacements( lines } +/// Intended result of a file update for apply_patch. +#[derive(Debug, Eq, PartialEq)] +pub struct ApplyPatchFileUpdate { + unified_diff: String, + content: String, +} + pub fn unified_diff_from_chunks( path: &Path, chunks: &[UpdateFileChunk], -) -> std::result::Result { +) -> std::result::Result { unified_diff_from_chunks_with_context(path, chunks, 1) } @@ -527,13 +540,17 @@ pub fn unified_diff_from_chunks_with_context( path: &Path, chunks: &[UpdateFileChunk], context: usize, -) -> std::result::Result { +) -> std::result::Result { let AppliedPatch { original_contents, new_contents, } = derive_new_contents_from_chunks(path, chunks)?; let text_diff = TextDiff::from_lines(&original_contents, &new_contents); - Ok(text_diff.unified_diff().context_radius(context).to_string()) + let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); + Ok(ApplyPatchFileUpdate { + unified_diff, + content: new_contents, + }) } /// Print the summary of changes in git-style format. @@ -898,7 +915,11 @@ PATCH"#, -qux +QUX "#; - assert_eq!(expected_diff, diff); + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + content: "foo\nBAR\nbaz\nQUX\n".to_string(), + }; + assert_eq!(expected, diff); } #[test] @@ -930,7 +951,11 @@ PATCH"#, +FOO bar "#; - assert_eq!(expected_diff, diff); + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + content: "FOO\nbar\nbaz\n".to_string(), + }; + assert_eq!(expected, diff); } #[test] @@ -963,7 +988,11 @@ PATCH"#, -baz +BAZ "#; - assert_eq!(expected_diff, diff); + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + content: "foo\nbar\nBAZ\n".to_string(), + }; + assert_eq!(expected, diff); } #[test] @@ -993,7 +1022,11 @@ PATCH"#, baz +quux "#; - assert_eq!(expected_diff, diff); + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + content: "foo\nbar\nbaz\nquux\n".to_string(), + }; + assert_eq!(expected, diff); } #[test] @@ -1032,7 +1065,7 @@ PATCH"#, let diff = unified_diff_from_chunks(&path, chunks).unwrap(); - let expected = r#"@@ -1,6 +1,7 @@ + let expected_diff = r#"@@ -1,6 +1,7 @@ a -b +B @@ -1044,6 +1077,11 @@ PATCH"#, +g "#; + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + content: "a\nB\nc\nd\nE\nf\ng\n".to_string(), + }; + assert_eq!(expected, diff); let mut stdout = Vec::new(); diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 2f80e505..edeaef99 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -3,8 +3,6 @@ use std::collections::HashSet; use std::io::Write; use std::path::Path; use std::path::PathBuf; -use std::process::Command; -use std::process::Stdio; use std::sync::Arc; use std::sync::Mutex; @@ -1346,6 +1344,7 @@ fn convert_apply_patch_to_protocol( ApplyPatchFileChange::Update { unified_diff, move_path, + new_content: _new_content, } => FileChange::Update { unified_diff: unified_diff.clone(), move_path: move_path.clone(), @@ -1400,28 +1399,10 @@ fn apply_changes_from_apply_patch( deleted.push(path.clone()); } ApplyPatchFileChange::Update { - unified_diff, + unified_diff: _unified_diff, move_path, + new_content, } => { - // TODO(mbolin): `patch` is not guaranteed to be available. - // Allegedly macOS provides it, but minimal Linux installs - // might omit it. - Command::new("patch") - .arg(path) - .arg("-p0") - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .stdin(Stdio::piped()) - .spawn() - .and_then(|mut child| { - let mut stdin = child.stdin.take().unwrap(); - stdin.write_all(unified_diff.as_bytes())?; - stdin.flush()?; - // Drop stdin to send EOF. - drop(stdin); - child.wait() - }) - .with_context(|| format!("Failed to apply patch to {}", path.display()))?; if let Some(move_path) = move_path { if let Some(parent) = move_path.parent() { if !parent.as_os_str().is_empty() { @@ -1433,11 +1414,14 @@ fn apply_changes_from_apply_patch( })?; } } + std::fs::rename(path, move_path) .with_context(|| format!("Failed to rename file {}", path.display()))?; + std::fs::write(move_path, new_content)?; modified.push(move_path.clone()); deleted.push(path.clone()); } else { + std::fs::write(path, new_content)?; modified.push(path.clone()); } }