From a1709a444da697b4a9d71acbfeabf4d273702d93 Mon Sep 17 00:00:00 2001 From: Bryan Eastes Date: Tue, 7 Jul 2026 11:37:01 -0700 Subject: [PATCH] Reject duplicate file entries in apply_patch --- codex-rs/apply-patch/src/invocation.rs | 35 ++++++++++++ codex-rs/apply-patch/src/lib.rs | 74 ++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 8ac2084e4f..5c861ccb36 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -15,6 +15,7 @@ use crate::ApplyPatchFileChange; use crate::ApplyPatchFileUpdate; use crate::IoError; use crate::MaybeApplyPatchVerified; +use crate::ensure_unique_hunk_paths; use crate::parser::Hunk; use crate::parser::ParseError; use crate::parser::parse_patch; @@ -194,6 +195,7 @@ async fn try_verify_apply_patch_args( .map(|dir| cwd.join(dir)) .transpose()? .unwrap_or_else(|| cwd.clone()); + ensure_unique_hunk_paths(&hunks, &effective_cwd)?; let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(&effective_cwd)?; @@ -526,6 +528,39 @@ mod tests { ); } + #[tokio::test] + async fn test_verified_rejects_duplicate_hunk_paths() { + let dir = tempdir().unwrap(); + let path = dir.path().join("source.txt"); + fs::write(&path, "first\nsecond\n").unwrap(); + let argv = vec![ + "apply_patch".to_string(), + r#"*** Begin Patch +*** Update File: source.txt +@@ +-first ++FIRST +*** Update File: source.txt +@@ +-second ++SECOND +*** End Patch"# + .to_string(), + ]; + + let cwd = PathUri::from_host_native_path(dir.path()).expect("absolute test path"); + let result = + maybe_parse_apply_patch_verified(&argv, &cwd, LOCAL_FS.as_ref(), /*sandbox*/ None) + .await; + + assert_eq!( + result, + MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::DuplicateFilePath( + path.display().to_string() + )) + ); + } + #[tokio::test] async fn test_literal() { let args = strs_to_strings(&[ diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 81ad1f0781..8200109ebe 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -5,6 +5,7 @@ mod standalone_executable; mod streaming_parser; use std::collections::HashMap; +use std::collections::HashSet; use std::io; use std::path::PathBuf; @@ -57,6 +58,10 @@ pub enum ApplyPatchError { "patch detected without explicit call to apply_patch. Rerun as [\"apply_patch\", \"\"]" )] ImplicitInvocation, + /// A patch had multiple hunks for a single source path, which cannot be + /// represented faithfully in the approval payload. + #[error("patch contains multiple hunks for the same file: {0}")] + DuplicateFilePath(String), } impl From for ApplyPatchError { @@ -320,6 +325,13 @@ pub async fn apply_hunks( fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> Result { + if let Err(error) = ensure_unique_hunk_paths(hunks, cwd) { + writeln!(stderr, "{error}") + .map_err(ApplyPatchError::from) + .map_err(ApplyPatchFailure::without_delta)?; + return Err(ApplyPatchFailure::without_delta(error)); + } + let mut delta = AppliedPatchDelta::empty(); match apply_hunks_to_files(hunks, cwd, fs, sandbox, &mut delta).await { Ok(affected_paths) => { @@ -346,6 +358,22 @@ pub async fn apply_hunks( } } +pub(crate) fn ensure_unique_hunk_paths( + hunks: &[Hunk], + cwd: &PathUri, +) -> Result<(), ApplyPatchError> { + let mut seen = HashSet::new(); + for hunk in hunks { + let path = hunk.resolve_path(cwd)?; + if !seen.insert(path.clone()) { + return Err(ApplyPatchError::DuplicateFilePath( + path.inferred_native_path_string(), + )); + } + } + Ok(()) +} + /// Applies each parsed patch hunk to the filesystem. /// Returns an error if any of the changes could not be applied. /// Tracks file paths affected by applying a patch, preserving the path spelling @@ -934,6 +962,52 @@ mod tests { assert_eq!(contents, "ab\ncd\n"); } + #[tokio::test] + async fn test_apply_patch_rejects_duplicate_hunk_paths_before_writing() { + let dir = tempdir().unwrap(); + let path = dir.path().join("source.txt"); + fs::write(&path, "first\nsecond\n").unwrap(); + let patch = wrap_patch( + r#"*** Update File: source.txt +@@ +-first ++FIRST +*** Update File: source.txt +@@ +-second ++SECOND"#, + ); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = apply_patch( + &patch, + &PathUri::from_host_native_path(dir.path()).expect("absolute test path"), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await; + + let expected_error = ApplyPatchError::DuplicateFilePath(path.display().to_string()); + assert_eq!( + result + .expect_err("duplicate paths should be rejected") + .into_parts(), + (expected_error, AppliedPatchDelta::empty()) + ); + assert_eq!(String::from_utf8(stdout).unwrap(), ""); + assert_eq!( + String::from_utf8(stderr).unwrap(), + format!( + "patch contains multiple hunks for the same file: {}\n", + path.display() + ) + ); + assert_eq!(fs::read_to_string(path).unwrap(), "first\nsecond\n"); + } + #[tokio::test] async fn test_apply_patch_hunks_accept_relative_and_absolute_paths() { let dir = tempdir().unwrap();