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`.
This commit is contained in:
Michael Bolin
2025-04-28 21:15:41 -07:00
committed by GitHub
parent d09dbba7ec
commit b9bba09819
2 changed files with 53 additions and 31 deletions

View File

@@ -86,6 +86,8 @@ pub enum ApplyPatchFileChange {
Update {
unified_diff: String,
move_path: Option<PathBuf>,
/// 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<String, ApplyPatchError> {
) -> std::result::Result<ApplyPatchFileUpdate, ApplyPatchError> {
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<String, ApplyPatchError> {
) -> std::result::Result<ApplyPatchFileUpdate, ApplyPatchError> {
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();