diff --git a/codex-rs/apply-patch/src/file_update.rs b/codex-rs/apply-patch/src/file_update.rs new file mode 100644 index 0000000000..d700570264 --- /dev/null +++ b/codex-rs/apply-patch/src/file_update.rs @@ -0,0 +1,322 @@ +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::FileSystemSandboxContext; +use codex_utils_path_uri::PathUri; +use similar::TextDiff; + +use crate::ApplyPatchError; +use crate::ApplyPatchFileUpdateMode; +use crate::IoError; +use crate::UpdateFileChunk; +use crate::seek_sequence; +use crate::text_file::Replacement; +use crate::text_file::SourceFile; + +#[cfg(test)] +#[path = "file_update_tests.rs"] +mod tests; + +pub(crate) struct AppliedPatch { + pub(crate) original_contents: String, + pub(crate) new_contents: String, +} + +/// Return *only* the new file contents (joined into a single `String`) after +/// applying the chunks to the file at `path`. +pub(crate) async fn derive_new_contents_from_chunks( + path: &PathUri, + chunks: &[UpdateFileChunk], + update_file_mode: ApplyPatchFileUpdateMode, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + let original_contents = fs.read_file_text(path, sandbox).await.map_err(|err| { + ApplyPatchError::IoError(IoError { + context: format!( + "Failed to read file to update {}", + path.inferred_native_path_string() + ), + source: err, + }) + })?; + + let path_text = path.inferred_native_path_string(); + let new_contents = match update_file_mode { + ApplyPatchFileUpdateMode::NormalizeToLf => { + let mut original_lines = original_contents + .split('\n') + .map(String::from) + .collect::>(); + + // Drop the trailing empty element that results from the final newline so + // that line counts match the behaviour of standard `diff`. + if original_lines.last().is_some_and(String::is_empty) { + original_lines.pop(); + } + + let replacements = + compute_replacements(&original_lines, &path_text, chunks, update_file_mode)?; + let mut new_lines = apply_replacements(original_lines, &replacements); + if !new_lines.last().is_some_and(String::is_empty) { + new_lines.push(String::new()); + } + new_lines.join("\n") + } + ApplyPatchFileUpdateMode::PreserveLineEndings => { + let mut source_file = SourceFile::parse(&original_contents); + let original_lines = source_file.line_texts(); + let replacements = + compute_replacements(&original_lines, &path_text, chunks, update_file_mode)?; + source_file.apply_replacements(&replacements); + source_file.into_contents() + } + }; + Ok(AppliedPatch { + original_contents, + new_contents, + }) +} + +/// Compute a list of replacements needed to transform `original_lines` into the +/// new lines, given the patch `chunks`. Each replacement is returned as +/// `(start_index, old_len, new_lines)`. +fn compute_replacements( + original_lines: &[String], + path: &str, + chunks: &[UpdateFileChunk], + update_file_mode: ApplyPatchFileUpdateMode, +) -> std::result::Result, ApplyPatchError> { + let mut replacements: Vec = Vec::new(); + let mut line_index: usize = 0; + + for chunk in chunks { + // If a chunk has a `change_context`, we use seek_sequence to find it, then + // adjust our `line_index` to continue from there. + if let Some(ctx_line) = &chunk.change_context { + if let Some(idx) = seek_sequence::seek_sequence( + original_lines, + std::slice::from_ref(ctx_line), + line_index, + /*eof*/ false, + update_file_mode, + ) { + line_index = idx + 1; + } else { + return Err(ApplyPatchError::ComputeReplacements(format!( + "Failed to find context '{ctx_line}' in {path}" + ))); + } + } + + if chunk.old_lines.is_empty() { + // Preserve the legacy split representation's handling of a final + // empty line. `SourceFile` only exposes real source lines, so its + // insertion point is always after the final line. + let insertion_idx = match update_file_mode { + ApplyPatchFileUpdateMode::NormalizeToLf => { + if original_lines.last().is_some_and(String::is_empty) { + original_lines.len() - 1 + } else { + original_lines.len() + } + } + ApplyPatchFileUpdateMode::PreserveLineEndings => original_lines.len(), + }; + replacements.push((insertion_idx, 0, chunk.new_lines.clone())); + continue; + } + + // Otherwise, try to match the existing lines in the file with the old lines + // from the chunk. If found, schedule that region for replacement. + // Attempt to locate the `old_lines` verbatim within the file. In many + // real‑world diffs the last element of `old_lines` is an *empty* string + // representing the terminating newline of the region being replaced. + // This sentinel is not present in `original_lines` because `SourceFile` + // stores the terminator on the preceding line rather than as an extra + // trailing element. If a direct search fails and the pattern ends with + // an empty string, retry without that final element so modifications + // touching the end‑of‑file can be located reliably. + + let mut pattern: &[String] = &chunk.old_lines; + let mut found = seek_sequence::seek_sequence( + original_lines, + pattern, + line_index, + chunk.is_end_of_file, + update_file_mode, + ); + + let mut new_slice: &[String] = &chunk.new_lines; + + if found.is_none() && pattern.last().is_some_and(String::is_empty) { + // Retry without the trailing empty line which represents the final + // newline in the file. + pattern = &pattern[..pattern.len() - 1]; + if new_slice.last().is_some_and(String::is_empty) { + new_slice = &new_slice[..new_slice.len() - 1]; + } + + found = seek_sequence::seek_sequence( + original_lines, + pattern, + line_index, + chunk.is_end_of_file, + update_file_mode, + ); + } + + if let Some(start_idx) = found { + match update_file_mode { + ApplyPatchFileUpdateMode::NormalizeToLf => { + replacements.push((start_idx, pattern.len(), new_slice.to_vec())); + } + ApplyPatchFileUpdateMode::PreserveLineEndings => { + // Context lines occur in both sides of a patch chunk. Keep those + // original lines in place so their exact contents and terminators + // survive, especially when the file has mixed line endings. + let mut old_start = 0; + let mut new_start = 0; + for &(old_context, new_context) in &chunk.context_line_indices { + // A trailing empty context line can be removed from `pattern` + // and `new_slice` above when it represents the final newline. + if old_context >= pattern.len() || new_context >= new_slice.len() { + break; + } + if old_start != old_context || new_start != new_context { + replacements.push(( + start_idx + old_start, + old_context - old_start, + new_slice[new_start..new_context].to_vec(), + )); + } + old_start = old_context + 1; + new_start = new_context + 1; + } + if old_start != pattern.len() || new_start != new_slice.len() { + replacements.push(( + start_idx + old_start, + pattern.len() - old_start, + new_slice[new_start..].to_vec(), + )); + } + } + } + line_index = start_idx + pattern.len(); + } else { + return Err(ApplyPatchError::ComputeReplacements(format!( + "Failed to find expected lines in {}:\n{}", + path, + chunk.old_lines.join("\n"), + ))); + } + } + + replacements.sort_by_key(|(index, _, _)| *index); + + Ok(replacements) +} + +/// Apply the `(start_index, old_len, new_lines)` replacements to `original_lines`, +/// returning the modified file contents as a vector of lines. +fn apply_replacements(mut lines: Vec, replacements: &[Replacement]) -> Vec { + // We must apply replacements in descending order so that earlier replacements + // don't shift the positions of later ones. + for (start_idx, old_len, new_segment) in replacements.iter().rev() { + let start_idx = *start_idx; + let old_len = *old_len; + + // Remove old lines. + for _ in 0..old_len { + if start_idx < lines.len() { + lines.remove(start_idx); + } + } + + // Insert new lines. + for (offset, new_line) in new_segment.iter().enumerate() { + lines.insert(start_idx + offset, new_line.clone()); + } + } + + lines +} + +/// Intended result of a file update for apply_patch. +#[derive(Debug, Eq, PartialEq)] +pub struct ApplyPatchFileUpdate { + pub(crate) unified_diff: String, + pub(crate) original_content: String, + pub(crate) content: String, +} + +pub async fn unified_diff_from_chunks( + path: &PathUri, + chunks: &[UpdateFileChunk], + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + unified_diff_from_chunks_with_mode( + path, + chunks, + ApplyPatchFileUpdateMode::default(), + fs, + sandbox, + ) + .await +} + +pub(crate) async fn unified_diff_from_chunks_with_mode( + path: &PathUri, + chunks: &[UpdateFileChunk], + update_file_mode: ApplyPatchFileUpdateMode, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + unified_diff_from_chunks_with_context_and_mode( + path, + chunks, + /*context*/ 1, + update_file_mode, + fs, + sandbox, + ) + .await +} + +pub async fn unified_diff_from_chunks_with_context( + path: &PathUri, + chunks: &[UpdateFileChunk], + context: usize, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + unified_diff_from_chunks_with_context_and_mode( + path, + chunks, + context, + ApplyPatchFileUpdateMode::default(), + fs, + sandbox, + ) + .await +} + +async fn unified_diff_from_chunks_with_context_and_mode( + path: &PathUri, + chunks: &[UpdateFileChunk], + context: usize, + update_file_mode: ApplyPatchFileUpdateMode, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + let AppliedPatch { + original_contents, + new_contents, + } = derive_new_contents_from_chunks(path, chunks, update_file_mode, fs, sandbox).await?; + let text_diff = TextDiff::from_lines(&original_contents, &new_contents); + let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); + Ok(ApplyPatchFileUpdate { + unified_diff, + original_content: original_contents, + content: new_contents, + }) +} diff --git a/codex-rs/apply-patch/src/file_update_tests.rs b/codex-rs/apply-patch/src/file_update_tests.rs new file mode 100644 index 0000000000..968082e26d --- /dev/null +++ b/codex-rs/apply-patch/src/file_update_tests.rs @@ -0,0 +1,278 @@ +use super::*; +use crate::Hunk; +use crate::apply_patch; +use crate::parse_patch; +use codex_exec_server::LOCAL_FS; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; +use std::fs; +use tempfile::tempdir; + +fn wrap_patch(body: &str) -> String { + format!("*** Begin Patch\n{body}\n*** End Patch") +} + +#[tokio::test] +async fn test_unified_diff() { + // Start with a file containing four lines. + let dir = tempdir().unwrap(); + let path = dir.path().join("multi.txt"); + fs::write(&path, "foo\nbar\nbaz\nqux\n").unwrap(); + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ + foo +-bar ++BAR +@@ + baz +-qux ++QUX"#, + path.display() + )); + let patch = parse_patch(&patch).unwrap(); + + let update_file_chunks = match patch.hunks.as_slice() { + [Hunk::UpdateFile { chunks, .. }] => chunks, + _ => panic!("Expected a single UpdateFile hunk"), + }; + let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); + let diff = unified_diff_from_chunks( + &path_uri, + update_file_chunks, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .unwrap(); + let expected_diff = r#"@@ -1,4 +1,4 @@ + foo +-bar ++BAR + baz +-qux ++QUX +"#; + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + original_content: "foo\nbar\nbaz\nqux\n".to_string(), + content: "foo\nBAR\nbaz\nQUX\n".to_string(), + }; + assert_eq!(expected, diff); +} + +#[tokio::test] +async fn test_unified_diff_first_line_replacement() { + // Replace the very first line of the file. + let dir = tempdir().unwrap(); + let path = dir.path().join("first.txt"); + fs::write(&path, "foo\nbar\nbaz\n").unwrap(); + + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ +-foo ++FOO + bar +"#, + path.display() + )); + + let patch = parse_patch(&patch).unwrap(); + let chunks = match patch.hunks.as_slice() { + [Hunk::UpdateFile { chunks, .. }] => chunks, + _ => panic!("Expected a single UpdateFile hunk"), + }; + + let resolved_path = PathUri::from_host_native_path(&path).expect("absolute test path"); + let diff = unified_diff_from_chunks( + &resolved_path, + chunks, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .unwrap(); + let expected_diff = r#"@@ -1,2 +1,2 @@ +-foo ++FOO + bar +"#; + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + original_content: "foo\nbar\nbaz\n".to_string(), + content: "FOO\nbar\nbaz\n".to_string(), + }; + assert_eq!(expected, diff); +} + +#[tokio::test] +async fn test_unified_diff_last_line_replacement() { + // Replace the very last line of the file. + let dir = tempdir().unwrap(); + let path = dir.path().join("last.txt"); + fs::write(&path, "foo\nbar\nbaz\n").unwrap(); + + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ + foo + bar +-baz ++BAZ +"#, + path.display() + )); + + let patch = parse_patch(&patch).unwrap(); + let chunks = match patch.hunks.as_slice() { + [Hunk::UpdateFile { chunks, .. }] => chunks, + _ => panic!("Expected a single UpdateFile hunk"), + }; + + let resolved_path = PathUri::from_host_native_path(&path).expect("absolute test path"); + let diff = unified_diff_from_chunks( + &resolved_path, + chunks, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await + .unwrap(); + let expected_diff = r#"@@ -2,2 +2,2 @@ + bar +-baz ++BAZ +"#; + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + original_content: "foo\nbar\nbaz\n".to_string(), + content: "foo\nbar\nBAZ\n".to_string(), + }; + assert_eq!(expected, diff); +} + +#[tokio::test] +async fn test_unified_diff_insert_at_eof() { + // Insert a new line at end-of-file. + let dir = tempdir().unwrap(); + let path = dir.path().join("insert.txt"); + fs::write(&path, "foo\nbar\nbaz\n").unwrap(); + + let patch = wrap_patch(&format!( + r#"*** Update File: {} +@@ ++quux +*** End of File +"#, + path.display() + )); + + let patch = parse_patch(&patch).unwrap(); + let chunks = match patch.hunks.as_slice() { + [Hunk::UpdateFile { chunks, .. }] => chunks, + _ => panic!("Expected a single UpdateFile hunk"), + }; + + let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); + let diff = + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + .await + .unwrap(); + let expected_diff = r#"@@ -3 +3,2 @@ + baz ++quux +"#; + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + original_content: "foo\nbar\nbaz\n".to_string(), + content: "foo\nbar\nbaz\nquux\n".to_string(), + }; + assert_eq!(expected, diff); +} + +#[tokio::test] +async fn test_unified_diff_interleaved_changes() { + // Original file with six lines. + let dir = tempdir().unwrap(); + let path = dir.path().join("interleaved.txt"); + fs::write(&path, "a\nb\nc\nd\ne\nf\n").unwrap(); + + // Patch replaces two separate lines and appends a new one at EOF using + // three distinct chunks. + let patch_body = format!( + r#"*** Update File: {} +@@ + a +-b ++B +@@ + d +-e ++E +@@ + f ++g +*** End of File"#, + path.display() + ); + let patch = wrap_patch(&patch_body); + + // Extract chunks then build the unified diff. + let parsed = parse_patch(&patch).unwrap(); + let chunks = match parsed.hunks.as_slice() { + [Hunk::UpdateFile { chunks, .. }] => chunks, + _ => panic!("Expected a single UpdateFile hunk"), + }; + + let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); + let diff = + unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) + .await + .unwrap(); + + let expected_diff = r#"@@ -1,6 +1,7 @@ + a +-b ++B + c + d +-e ++E + f ++g +"#; + + let expected = ApplyPatchFileUpdate { + unified_diff: expected_diff.to_string(), + original_content: "a\nb\nc\nd\ne\nf\n".to_string(), + content: "a\nB\nc\nd\nE\nf\ng\n".to_string(), + }; + + assert_eq!(expected, diff); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + 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 + .unwrap(); + let contents = fs::read_to_string(path).unwrap(); + assert_eq!( + contents, + r#"a +B +c +d +E +f +g +"# + ); +} diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 8ac2084e4f..b1731d081f 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -13,12 +13,13 @@ use crate::ApplyPatchArgs; use crate::ApplyPatchError; use crate::ApplyPatchFileChange; use crate::ApplyPatchFileUpdate; +use crate::ApplyPatchFileUpdateMode; use crate::IoError; use crate::MaybeApplyPatchVerified; use crate::parser::Hunk; use crate::parser::ParseError; use crate::parser::parse_patch; -use crate::unified_diff_from_chunks; +use crate::unified_diff_from_chunks_with_mode; use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use std::str::Utf8Error; @@ -143,6 +144,25 @@ pub async fn maybe_parse_apply_patch_verified( cwd: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, +) -> MaybeApplyPatchVerified { + maybe_parse_apply_patch_verified_with_mode( + argv, + cwd, + ApplyPatchFileUpdateMode::default(), + fs, + sandbox, + ) + .await +} + +/// Parses and verifies an `apply_patch` invocation using the selected +/// file-update mode. +pub async fn maybe_parse_apply_patch_verified_with_mode( + argv: &[String], + cwd: &PathUri, + update_file_mode: ApplyPatchFileUpdateMode, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, ) -> MaybeApplyPatchVerified { // Detect a raw patch body passed directly as the command or as the body of a shell // script. In these cases, report an explicit error rather than applying the patch. @@ -158,7 +178,9 @@ pub async fn maybe_parse_apply_patch_verified( } match maybe_parse_apply_patch(argv, cwd) { - MaybeApplyPatch::Body(args) => verify_apply_patch_args(args, cwd, fs, sandbox).await, + MaybeApplyPatch::Body(args) => { + verify_apply_patch_args_with_mode(args, cwd, update_file_mode, fs, sandbox).await + } MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), MaybeApplyPatch::NotApplyPatch => MaybeApplyPatchVerified::NotApplyPatch, @@ -171,7 +193,19 @@ pub async fn verify_apply_patch_args( fs: &dyn ExecutorFileSystem, sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, ) -> MaybeApplyPatchVerified { - match try_verify_apply_patch_args(args, cwd, fs, sandbox).await { + verify_apply_patch_args_with_mode(args, cwd, ApplyPatchFileUpdateMode::default(), fs, sandbox) + .await +} + +/// Verifies parsed patch arguments using the selected file-update mode. +pub async fn verify_apply_patch_args_with_mode( + args: ApplyPatchArgs, + cwd: &PathUri, + update_file_mode: ApplyPatchFileUpdateMode, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, +) -> MaybeApplyPatchVerified { + match try_verify_apply_patch_args(args, cwd, update_file_mode, fs, sandbox).await { Ok(action) => MaybeApplyPatchVerified::Body(action), Err(err) => MaybeApplyPatchVerified::CorrectnessError(err), } @@ -180,6 +214,7 @@ pub async fn verify_apply_patch_args( async fn try_verify_apply_patch_args( args: ApplyPatchArgs, cwd: &PathUri, + update_file_mode: ApplyPatchFileUpdateMode, fs: &dyn ExecutorFileSystem, sandbox: Option<&codex_exec_server::FileSystemSandboxContext>, ) -> Result { @@ -217,7 +252,14 @@ async fn try_verify_apply_patch_args( unified_diff, content: contents, .. - } = unified_diff_from_chunks(&path, &chunks, fs, sandbox).await?; + } = unified_diff_from_chunks_with_mode( + &path, + &chunks, + update_file_mode, + fs, + sandbox, + ) + .await?; changes.insert( path, ApplyPatchFileChange::Update { @@ -233,6 +275,7 @@ async fn try_verify_apply_patch_args( } Ok(ApplyPatchAction { changes, + update_file_mode, patch, cwd: effective_cwd, }) @@ -856,6 +899,7 @@ PATCH"#, new_content: "updated session directory content\n".to_string(), }, )]), + update_file_mode: ApplyPatchFileUpdateMode::default(), patch: argv[1].clone(), cwd: PathUri::from_host_native_path(session_dir.path()) .expect("absolute test path"), diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 5cd3364ff2..5b5fac0683 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -1,8 +1,10 @@ +mod file_update; mod invocation; mod parser; mod seek_sequence; mod standalone_executable; mod streaming_parser; +mod text_file; use std::collections::HashMap; use std::io; @@ -21,14 +23,21 @@ pub use parser::ParseError; use parser::ParseError::*; pub use parser::UpdateFileChunk; pub use parser::parse_patch; -use similar::TextDiff; pub use streaming_parser::StreamingPatchParser; use thiserror::Error; +use file_update::AppliedPatch; +pub use file_update::ApplyPatchFileUpdate; +use file_update::derive_new_contents_from_chunks; +pub use file_update::unified_diff_from_chunks; +pub use file_update::unified_diff_from_chunks_with_context; +pub(crate) use file_update::unified_diff_from_chunks_with_mode; pub use invocation::MaybeApplyPatch; pub use invocation::maybe_parse_apply_patch; pub use invocation::maybe_parse_apply_patch_verified; +pub use invocation::maybe_parse_apply_patch_verified_with_mode; pub use invocation::verify_apply_patch_args; +pub use invocation::verify_apply_patch_args_with_mode; pub use standalone_executable::main; use crate::invocation::ExtractHeredocError; @@ -42,6 +51,30 @@ use crate::invocation::ExtractHeredocError; /// surface. pub const CODEX_CORE_APPLY_PATCH_ARG1: &str = "--codex-run-as-apply-patch"; +/// Internal environment variable used to carry the selected update mode +/// through the arg0-dispatched standalone executable. +pub const CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR: &str = + "CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS"; + +/// Controls how updates reconstruct the target file after matching a patch. +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub enum ApplyPatchFileUpdateMode { + /// Preserve the historical behavior of normalizing updated files to LF. + #[default] + NormalizeToLf, + /// Preserve existing line endings and use the file's preferred ending for new lines. + PreserveLineEndings, +} + +/// Reads the update mode selected for an arg0-dispatched `apply_patch` process. +#[doc(hidden)] +pub fn apply_patch_file_update_mode_from_env() -> ApplyPatchFileUpdateMode { + match std::env::var(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR).as_deref() { + Ok("1") => ApplyPatchFileUpdateMode::PreserveLineEndings, + _ => ApplyPatchFileUpdateMode::NormalizeToLf, + } +} + #[derive(Debug, Error, PartialEq)] pub enum ApplyPatchError { #[error(transparent)] @@ -140,6 +173,8 @@ pub enum MaybeApplyPatchVerified { pub struct ApplyPatchAction { changes: HashMap, + update_file_mode: ApplyPatchFileUpdateMode, + /// The raw patch argument that can be used to apply the patch. i.e., if the /// original arg was parsed in "lenient" mode with a /// heredoc, this should be the value without the heredoc wrapper. @@ -159,6 +194,11 @@ impl ApplyPatchAction { &self.changes } + /// Returns the update mode selected while the patch was verified. + pub fn update_file_mode(&self) -> ApplyPatchFileUpdateMode { + self.update_file_mode + } + /// Should be used exclusively for testing. (Not worth the overhead of /// creating a feature flag for this.) pub fn new_add_for_test(path: &PathUri, content: String) -> Self { @@ -175,6 +215,7 @@ impl ApplyPatchAction { #[expect(clippy::expect_used)] Self { changes, + update_file_mode: ApplyPatchFileUpdateMode::default(), cwd: path.parent().expect("path should have parent"), patch, } @@ -282,6 +323,29 @@ pub async fn apply_patch( stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, +) -> Result { + apply_patch_with_mode( + patch, + ApplyPatchFileUpdateMode::default(), + cwd, + stdout, + stderr, + fs, + sandbox, + ) + .await +} + +/// Applies the patch using the selected file-update mode and prints the result +/// to stdout/stderr. +pub async fn apply_patch_with_mode( + patch: &str, + update_file_mode: ApplyPatchFileUpdateMode, + cwd: &PathUri, + stdout: &mut impl std::io::Write, + stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, ) -> Result { let hunks = match parse_patch(patch) { Ok(source) => source.hunks, @@ -310,7 +374,7 @@ pub async fn apply_patch( } }; - apply_hunks(&hunks, cwd, stdout, stderr, fs, sandbox).await + apply_hunks_with_mode(&hunks, update_file_mode, cwd, stdout, stderr, fs, sandbox).await } /// Applies hunks and continues to update stdout/stderr @@ -321,9 +385,32 @@ pub async fn apply_hunks( stderr: &mut impl std::io::Write, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, +) -> Result { + apply_hunks_with_mode( + hunks, + ApplyPatchFileUpdateMode::default(), + cwd, + stdout, + stderr, + fs, + sandbox, + ) + .await +} + +/// Applies hunks using the selected file-update mode and continues to update +/// stdout/stderr. +async fn apply_hunks_with_mode( + hunks: &[Hunk], + update_file_mode: ApplyPatchFileUpdateMode, + cwd: &PathUri, + stdout: &mut impl std::io::Write, + stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, + sandbox: Option<&FileSystemSandboxContext>, ) -> Result { let mut delta = AppliedPatchDelta::empty(); - match apply_hunks_to_files(hunks, cwd, fs, sandbox, &mut delta).await { + match apply_hunks_to_files(hunks, update_file_mode, cwd, fs, sandbox, &mut delta).await { Ok(affected_paths) => { print_summary(&affected_paths, stdout).map_err(|error| { ApplyPatchFailure::new(ApplyPatchError::from(error), delta.clone()) @@ -362,6 +449,7 @@ pub struct AffectedPaths { /// Returns an error if the patch could not be applied. async fn apply_hunks_to_files( hunks: &[Hunk], + update_file_mode: ApplyPatchFileUpdateMode, cwd: &PathUri, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, @@ -470,7 +558,14 @@ async fn apply_hunks_to_files( let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(&path_uri, chunks, fs, sandbox).await?; + } = derive_new_contents_from_chunks( + &path_uri, + chunks, + update_file_mode, + fs, + sandbox, + ) + .await?; if let Some(dest) = move_path { let dest_uri = cwd.join(&dest.to_string_lossy())?; let overwritten_move_content = @@ -666,207 +761,6 @@ async fn write_file_with_missing_parent_retry( } } -struct AppliedPatch { - original_contents: String, - new_contents: String, -} - -/// Return *only* the new file contents (joined into a single `String`) after -/// applying the chunks to the file at `path`. -async fn derive_new_contents_from_chunks( - path: &PathUri, - chunks: &[UpdateFileChunk], - fs: &dyn ExecutorFileSystem, - sandbox: Option<&FileSystemSandboxContext>, -) -> std::result::Result { - let original_contents = fs.read_file_text(path, sandbox).await.map_err(|err| { - ApplyPatchError::IoError(IoError { - context: format!( - "Failed to read file to update {}", - path.inferred_native_path_string() - ), - source: err, - }) - })?; - - let mut original_lines: Vec = original_contents.split('\n').map(String::from).collect(); - - // Drop the trailing empty element that results from the final newline so - // that line counts match the behaviour of standard `diff`. - if original_lines.last().is_some_and(String::is_empty) { - original_lines.pop(); - } - - let path_text = path.inferred_native_path_string(); - let replacements = compute_replacements(&original_lines, &path_text, chunks)?; - let new_lines = apply_replacements(original_lines, &replacements); - let mut new_lines = new_lines; - if !new_lines.last().is_some_and(String::is_empty) { - new_lines.push(String::new()); - } - let new_contents = new_lines.join("\n"); - Ok(AppliedPatch { - original_contents, - new_contents, - }) -} - -/// Compute a list of replacements needed to transform `original_lines` into the -/// new lines, given the patch `chunks`. Each replacement is returned as -/// `(start_index, old_len, new_lines)`. -fn compute_replacements( - original_lines: &[String], - path: &str, - chunks: &[UpdateFileChunk], -) -> std::result::Result)>, ApplyPatchError> { - let mut replacements: Vec<(usize, usize, Vec)> = Vec::new(); - let mut line_index: usize = 0; - - for chunk in chunks { - // If a chunk has a `change_context`, we use seek_sequence to find it, then - // adjust our `line_index` to continue from there. - if let Some(ctx_line) = &chunk.change_context { - if let Some(idx) = seek_sequence::seek_sequence( - original_lines, - std::slice::from_ref(ctx_line), - line_index, - /*eof*/ false, - ) { - line_index = idx + 1; - } else { - return Err(ApplyPatchError::ComputeReplacements(format!( - "Failed to find context '{ctx_line}' in {path}" - ))); - } - } - - if chunk.old_lines.is_empty() { - // Pure addition (no old lines). We'll add them at the end or just - // before the final empty line if one exists. - let insertion_idx = if original_lines.last().is_some_and(String::is_empty) { - original_lines.len() - 1 - } else { - original_lines.len() - }; - replacements.push((insertion_idx, 0, chunk.new_lines.clone())); - continue; - } - - // Otherwise, try to match the existing lines in the file with the old lines - // from the chunk. If found, schedule that region for replacement. - // Attempt to locate the `old_lines` verbatim within the file. In many - // real‑world diffs the last element of `old_lines` is an *empty* string - // representing the terminating newline of the region being replaced. - // This sentinel is not present in `original_lines` because we strip the - // trailing empty slice emitted by `split('\n')`. If a direct search - // fails and the pattern ends with an empty string, retry without that - // final element so that modifications touching the end‑of‑file can be - // located reliably. - - let mut pattern: &[String] = &chunk.old_lines; - let mut found = - seek_sequence::seek_sequence(original_lines, pattern, line_index, chunk.is_end_of_file); - - let mut new_slice: &[String] = &chunk.new_lines; - - if found.is_none() && pattern.last().is_some_and(String::is_empty) { - // Retry without the trailing empty line which represents the final - // newline in the file. - pattern = &pattern[..pattern.len() - 1]; - if new_slice.last().is_some_and(String::is_empty) { - new_slice = &new_slice[..new_slice.len() - 1]; - } - - found = seek_sequence::seek_sequence( - original_lines, - pattern, - line_index, - chunk.is_end_of_file, - ); - } - - if let Some(start_idx) = found { - replacements.push((start_idx, pattern.len(), new_slice.to_vec())); - line_index = start_idx + pattern.len(); - } else { - return Err(ApplyPatchError::ComputeReplacements(format!( - "Failed to find expected lines in {}:\n{}", - path, - chunk.old_lines.join("\n"), - ))); - } - } - - replacements.sort_by_key(|(index, _, _)| *index); - - Ok(replacements) -} - -/// Apply the `(start_index, old_len, new_lines)` replacements to `original_lines`, -/// returning the modified file contents as a vector of lines. -fn apply_replacements( - mut lines: Vec, - replacements: &[(usize, usize, Vec)], -) -> Vec { - // We must apply replacements in descending order so that earlier replacements - // don't shift the positions of later ones. - for (start_idx, old_len, new_segment) in replacements.iter().rev() { - let start_idx = *start_idx; - let old_len = *old_len; - - // Remove old lines. - for _ in 0..old_len { - if start_idx < lines.len() { - lines.remove(start_idx); - } - } - - // Insert new lines. - for (offset, new_line) in new_segment.iter().enumerate() { - lines.insert(start_idx + offset, new_line.clone()); - } - } - - lines -} - -/// Intended result of a file update for apply_patch. -#[derive(Debug, Eq, PartialEq)] -pub struct ApplyPatchFileUpdate { - unified_diff: String, - original_content: String, - content: String, -} - -pub async fn unified_diff_from_chunks( - path: &PathUri, - chunks: &[UpdateFileChunk], - fs: &dyn ExecutorFileSystem, - sandbox: Option<&FileSystemSandboxContext>, -) -> std::result::Result { - unified_diff_from_chunks_with_context(path, chunks, /*context*/ 1, fs, sandbox).await -} - -pub async fn unified_diff_from_chunks_with_context( - path: &PathUri, - chunks: &[UpdateFileChunk], - context: usize, - fs: &dyn ExecutorFileSystem, - sandbox: Option<&FileSystemSandboxContext>, -) -> std::result::Result { - let AppliedPatch { - original_contents, - new_contents, - } = derive_new_contents_from_chunks(path, chunks, fs, sandbox).await?; - let text_diff = TextDiff::from_lines(&original_contents, &new_contents); - let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); - Ok(ApplyPatchFileUpdate { - unified_diff, - original_content: original_contents, - content: new_contents, - }) -} - /// Print the summary of changes in git-style format. /// Write a summary of changes to the given writer. pub fn print_summary( @@ -892,7 +786,6 @@ mod tests { use codex_exec_server::LOCAL_FS; use pretty_assertions::assert_eq; use std::fs; - use std::string::ToString; use tempfile::tempdir; /// Helper to construct a patch with the given body. @@ -1368,271 +1261,6 @@ mod tests { assert_eq!(String::from_utf8(stderr).unwrap(), ""); } - #[tokio::test] - async fn test_unified_diff() { - // Start with a file containing four lines. - let dir = tempdir().unwrap(); - let path = dir.path().join("multi.txt"); - fs::write(&path, "foo\nbar\nbaz\nqux\n").unwrap(); - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ - foo --bar -+BAR -@@ - baz --qux -+QUX"#, - path.display() - )); - let patch = parse_patch(&patch).unwrap(); - - let update_file_chunks = match patch.hunks.as_slice() { - [Hunk::UpdateFile { chunks, .. }] => chunks, - _ => panic!("Expected a single UpdateFile hunk"), - }; - let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); - let diff = unified_diff_from_chunks( - &path_uri, - update_file_chunks, - LOCAL_FS.as_ref(), - /*sandbox*/ None, - ) - .await - .unwrap(); - let expected_diff = r#"@@ -1,4 +1,4 @@ - foo --bar -+BAR - baz --qux -+QUX -"#; - let expected = ApplyPatchFileUpdate { - unified_diff: expected_diff.to_string(), - original_content: "foo\nbar\nbaz\nqux\n".to_string(), - content: "foo\nBAR\nbaz\nQUX\n".to_string(), - }; - assert_eq!(expected, diff); - } - - #[tokio::test] - async fn test_unified_diff_first_line_replacement() { - // Replace the very first line of the file. - let dir = tempdir().unwrap(); - let path = dir.path().join("first.txt"); - fs::write(&path, "foo\nbar\nbaz\n").unwrap(); - - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ --foo -+FOO - bar -"#, - path.display() - )); - - let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.hunks.as_slice() { - [Hunk::UpdateFile { chunks, .. }] => chunks, - _ => panic!("Expected a single UpdateFile hunk"), - }; - - let resolved_path = PathUri::from_host_native_path(&path).expect("absolute test path"); - let diff = unified_diff_from_chunks( - &resolved_path, - chunks, - LOCAL_FS.as_ref(), - /*sandbox*/ None, - ) - .await - .unwrap(); - let expected_diff = r#"@@ -1,2 +1,2 @@ --foo -+FOO - bar -"#; - let expected = ApplyPatchFileUpdate { - unified_diff: expected_diff.to_string(), - original_content: "foo\nbar\nbaz\n".to_string(), - content: "FOO\nbar\nbaz\n".to_string(), - }; - assert_eq!(expected, diff); - } - - #[tokio::test] - async fn test_unified_diff_last_line_replacement() { - // Replace the very last line of the file. - let dir = tempdir().unwrap(); - let path = dir.path().join("last.txt"); - fs::write(&path, "foo\nbar\nbaz\n").unwrap(); - - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ - foo - bar --baz -+BAZ -"#, - path.display() - )); - - let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.hunks.as_slice() { - [Hunk::UpdateFile { chunks, .. }] => chunks, - _ => panic!("Expected a single UpdateFile hunk"), - }; - - let resolved_path = PathUri::from_host_native_path(&path).expect("absolute test path"); - let diff = unified_diff_from_chunks( - &resolved_path, - chunks, - LOCAL_FS.as_ref(), - /*sandbox*/ None, - ) - .await - .unwrap(); - let expected_diff = r#"@@ -2,2 +2,2 @@ - bar --baz -+BAZ -"#; - let expected = ApplyPatchFileUpdate { - unified_diff: expected_diff.to_string(), - original_content: "foo\nbar\nbaz\n".to_string(), - content: "foo\nbar\nBAZ\n".to_string(), - }; - assert_eq!(expected, diff); - } - - #[tokio::test] - async fn test_unified_diff_insert_at_eof() { - // Insert a new line at end‑of‑file. - let dir = tempdir().unwrap(); - let path = dir.path().join("insert.txt"); - fs::write(&path, "foo\nbar\nbaz\n").unwrap(); - - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ -+quux -*** End of File -"#, - path.display() - )); - - let patch = parse_patch(&patch).unwrap(); - let chunks = match patch.hunks.as_slice() { - [Hunk::UpdateFile { chunks, .. }] => chunks, - _ => panic!("Expected a single UpdateFile hunk"), - }; - - let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); - let diff = - unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) - .await - .unwrap(); - let expected_diff = r#"@@ -3 +3,2 @@ - baz -+quux -"#; - let expected = ApplyPatchFileUpdate { - unified_diff: expected_diff.to_string(), - original_content: "foo\nbar\nbaz\n".to_string(), - content: "foo\nbar\nbaz\nquux\n".to_string(), - }; - assert_eq!(expected, diff); - } - - #[tokio::test] - async fn test_unified_diff_interleaved_changes() { - // Original file with six lines. - let dir = tempdir().unwrap(); - let path = dir.path().join("interleaved.txt"); - fs::write(&path, "a\nb\nc\nd\ne\nf\n").unwrap(); - - // Patch replaces two separate lines and appends a new one at EOF using - // three distinct chunks. - let patch_body = format!( - r#"*** Update File: {} -@@ - a --b -+B -@@ - d --e -+E -@@ - f -+g -*** End of File"#, - path.display() - ); - let patch = wrap_patch(&patch_body); - - // Extract chunks then build the unified diff. - let parsed = parse_patch(&patch).unwrap(); - let chunks = match parsed.hunks.as_slice() { - [Hunk::UpdateFile { chunks, .. }] => chunks, - _ => panic!("Expected a single UpdateFile hunk"), - }; - - let path_uri = PathUri::from_host_native_path(&path).expect("absolute test path"); - let diff = - unified_diff_from_chunks(&path_uri, chunks, LOCAL_FS.as_ref(), /*sandbox*/ None) - .await - .unwrap(); - - let expected_diff = r#"@@ -1,6 +1,7 @@ - a --b -+B - c - d --e -+E - f -+g -"#; - - let expected = ApplyPatchFileUpdate { - unified_diff: expected_diff.to_string(), - original_content: "a\nb\nc\nd\ne\nf\n".to_string(), - content: "a\nB\nc\nd\nE\nf\ng\n".to_string(), - }; - - assert_eq!(expected, diff); - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - 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 - .unwrap(); - let contents = fs::read_to_string(path).unwrap(); - assert_eq!( - contents, - r#"a -B -c -d -E -f -g -"# - ); - } - #[cfg(unix)] #[tokio::test] async fn test_apply_patch_fails_on_write_error() { diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index ec2d97c477..c400d075a6 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -111,7 +111,7 @@ impl Hunk { #[cfg(test)] use Hunk::*; -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, Default, PartialEq, Clone)] pub struct UpdateFileChunk { /// A single line of context used to narrow down the position of the chunk /// (this is usually a class, method, or function definition.) @@ -122,11 +122,26 @@ pub struct UpdateFileChunk { pub old_lines: Vec, pub new_lines: Vec, + /// Pairs of indices into `old_lines` and `new_lines` that identify lines + /// parsed as context rather than inferred to be equal by their contents. + pub context_line_indices: Vec<(usize, usize)>, + /// If set to true, `old_lines` must occur at the end of the source file. /// (Tolerance around trailing newlines should be encouraged.) pub is_end_of_file: bool, } +impl UpdateFileChunk { + /// Adds a context line to both sides while recording its corresponding + /// indices so it remains distinguishable from identical changed lines. + pub(crate) fn push_context_line(&mut self, line: String) { + self.context_line_indices + .push((self.old_lines.len(), self.new_lines.len())); + self.old_lines.push(line.clone()); + self.new_lines.push(line); + } +} + pub fn parse_patch(patch: &str) -> Result { let mode = if PARSE_IN_STRICT_MODE { ParseMode::Strict @@ -345,6 +360,7 @@ fn test_parse_patch() { change_context: Some("def f():".to_string()), old_lines: vec![" pass".to_string()], new_lines: vec![" return 123".to_string()], + context_line_indices: vec![], is_end_of_file: false }] } @@ -372,6 +388,7 @@ fn test_parse_patch() { change_context: None, old_lines: vec![], new_lines: vec!["line".to_string()], + context_line_indices: vec![], is_end_of_file: false }], }, @@ -402,6 +419,7 @@ fn test_parse_patch() { change_context: None, old_lines: vec!["import foo".to_string()], new_lines: vec!["import foo".to_string(), "bar".to_string()], + context_line_indices: vec![(0, 0)], is_end_of_file: false, }], }] @@ -422,6 +440,7 @@ fn test_parse_patch_preserves_end_of_file_marker() { change_context: None, old_lines: Vec::new(), new_lines: vec!["quux".to_string()], + context_line_indices: vec![], is_end_of_file: true, }], }], @@ -470,6 +489,7 @@ fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() { change_context: None, old_lines: vec!["old".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false }] }, @@ -548,6 +568,7 @@ fn test_parse_patch_lenient() { change_context: None, old_lines: vec!["import foo".to_string()], new_lines: vec!["import foo".to_string(), "bar".to_string()], + context_line_indices: vec![(0, 0)], is_end_of_file: false, }], }]; diff --git a/codex-rs/apply-patch/src/seek_sequence.rs b/codex-rs/apply-patch/src/seek_sequence.rs index 3555963120..9934fa55e7 100644 --- a/codex-rs/apply-patch/src/seek_sequence.rs +++ b/codex-rs/apply-patch/src/seek_sequence.rs @@ -14,6 +14,7 @@ pub(crate) fn seek_sequence( pattern: &[String], start: usize, eof: bool, + update_file_mode: crate::ApplyPatchFileUpdateMode, ) -> Option { if pattern.is_empty() { return Some(start); @@ -27,7 +28,11 @@ pub(crate) fn seek_sequence( return None; } let search_start = if eof && lines.len() >= pattern.len() { - lines.len() - pattern.len() + let eof_start = lines.len() - pattern.len(); + match update_file_mode { + crate::ApplyPatchFileUpdateMode::NormalizeToLf => eof_start, + crate::ApplyPatchFileUpdateMode::PreserveLineEndings => eof_start.max(start), + } } else { start }; @@ -112,6 +117,7 @@ pub(crate) fn seek_sequence( #[cfg(test)] mod tests { use super::seek_sequence; + use crate::ApplyPatchFileUpdateMode; use std::string::ToString; fn to_vec(strings: &[&str]) -> Vec { @@ -123,7 +129,13 @@ mod tests { let lines = to_vec(&["foo", "bar", "baz"]); let pattern = to_vec(&["bar", "baz"]); assert_eq!( - seek_sequence(&lines, &pattern, /*start*/ 0, /*eof*/ false), + seek_sequence( + &lines, + &pattern, + /*start*/ 0, + /*eof*/ false, + ApplyPatchFileUpdateMode::NormalizeToLf, + ), Some(1) ); } @@ -134,7 +146,13 @@ mod tests { // Pattern omits trailing whitespace. let pattern = to_vec(&["foo", "bar"]); assert_eq!( - seek_sequence(&lines, &pattern, /*start*/ 0, /*eof*/ false), + seek_sequence( + &lines, + &pattern, + /*start*/ 0, + /*eof*/ false, + ApplyPatchFileUpdateMode::NormalizeToLf, + ), Some(0) ); } @@ -145,7 +163,13 @@ mod tests { // Pattern omits any additional whitespace. let pattern = to_vec(&["foo", "bar"]); assert_eq!( - seek_sequence(&lines, &pattern, /*start*/ 0, /*eof*/ false), + seek_sequence( + &lines, + &pattern, + /*start*/ 0, + /*eof*/ false, + ApplyPatchFileUpdateMode::NormalizeToLf, + ), Some(0) ); } @@ -156,7 +180,13 @@ mod tests { let pattern = to_vec(&["too", "many", "lines"]); // Should not panic – must return None when pattern cannot possibly fit. assert_eq!( - seek_sequence(&lines, &pattern, /*start*/ 0, /*eof*/ false), + seek_sequence( + &lines, + &pattern, + /*start*/ 0, + /*eof*/ false, + ApplyPatchFileUpdateMode::NormalizeToLf, + ), None ); } diff --git a/codex-rs/apply-patch/src/standalone_executable.rs b/codex-rs/apply-patch/src/standalone_executable.rs index 384b2ecee2..c977fb74c2 100644 --- a/codex-rs/apply-patch/src/standalone_executable.rs +++ b/codex-rs/apply-patch/src/standalone_executable.rs @@ -67,8 +67,10 @@ pub fn run_main() -> i32 { }; // TODO(anp): Discover the standalone executable cwd as PathUri directly. let cwd = codex_utils_path_uri::PathUri::from_abs_path(&cwd); - match runtime.block_on(crate::apply_patch( + let update_file_mode = crate::apply_patch_file_update_mode_from_env(); + match runtime.block_on(crate::apply_patch_with_mode( &patch_arg, + update_file_mode, &cwd, &mut stdout, &mut stderr, diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index 7b447fce3c..ff1b2f82fe 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -274,12 +274,7 @@ impl StreamingPatchParser { } if update_line == EMPTY_CHANGE_CONTEXT_MARKER { - chunks.push(UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }); + chunks.push(UpdateFileChunk::default()); self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number }; return Ok(()); } @@ -287,9 +282,7 @@ impl StreamingPatchParser { if let Some(change_context) = update_line.strip_prefix(CHANGE_CONTEXT_MARKER) { chunks.push(UpdateFileChunk { change_context: Some(change_context.to_string()), - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, + ..UpdateFileChunk::default() }); self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number }; return Ok(()); @@ -313,16 +306,10 @@ impl StreamingPatchParser { if line.is_empty() { if chunks.is_empty() { - chunks.push(UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }); + chunks.push(UpdateFileChunk::default()); } if let Some(chunk) = chunks.last_mut() { - chunk.old_lines.push(String::new()); - chunk.new_lines.push(String::new()); + chunk.push_context_line(String::new()); } self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number }; return Ok(()); @@ -330,16 +317,10 @@ impl StreamingPatchParser { if let Some(line_to_add) = line.strip_prefix(' ') { if chunks.is_empty() { - chunks.push(UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }); + chunks.push(UpdateFileChunk::default()); } if let Some(chunk) = chunks.last_mut() { - chunk.old_lines.push(line_to_add.to_string()); - chunk.new_lines.push(line_to_add.to_string()); + chunk.push_context_line(line_to_add.to_string()); } self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number }; return Ok(()); @@ -347,12 +328,7 @@ impl StreamingPatchParser { if let Some(line_to_add) = line.strip_prefix('+') { if chunks.is_empty() { - chunks.push(UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }); + chunks.push(UpdateFileChunk::default()); } if let Some(chunk) = chunks.last_mut() { chunk.new_lines.push(line_to_add.to_string()); @@ -363,12 +339,7 @@ impl StreamingPatchParser { if let Some(line_to_remove) = line.strip_prefix('-') { if chunks.is_empty() { - chunks.push(UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }); + chunks.push(UpdateFileChunk::default()); } if let Some(chunk) = chunks.last_mut() { chunk.old_lines.push(line_to_remove.to_string()); @@ -445,6 +416,7 @@ mod tests { change_context: None, old_lines: vec!["old".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false, }], }]) @@ -635,12 +607,14 @@ mod tests { change_context: None, old_lines: vec!["old a".to_string(), "*** Update File: b.txt".to_string()], new_lines: vec!["new a".to_string(), "*** Update File: b.txt".to_string()], + context_line_indices: vec![(1, 1)], is_end_of_file: false, }, UpdateFileChunk { change_context: None, old_lines: vec!["old b".to_string()], new_lines: vec!["new b".to_string()], + context_line_indices: vec![], is_end_of_file: false, }, ], @@ -680,6 +654,7 @@ mod tests { String::new(), "context after".to_string(), ], + context_line_indices: vec![(0, 0), (1, 1), (2, 2)], is_end_of_file: false, }], }]) @@ -700,6 +675,7 @@ mod tests { change_context: None, old_lines: Vec::new(), new_lines: vec!["quux".to_string()], + context_line_indices: vec![], is_end_of_file: true, }], }]) @@ -718,6 +694,7 @@ mod tests { change_context: None, old_lines: vec!["old".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false, }], }]) @@ -733,6 +710,7 @@ mod tests { change_context: None, old_lines: vec!["old\r".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false, }], }]) @@ -769,6 +747,7 @@ mod tests { change_context: None, old_lines: vec!["old".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false, }], }]) @@ -782,6 +761,7 @@ mod tests { change_context: None, old_lines: vec!["old".to_string()], new_lines: vec!["new".to_string()], + context_line_indices: vec![], is_end_of_file: false, }], }]) diff --git a/codex-rs/apply-patch/src/text_file.rs b/codex-rs/apply-patch/src/text_file.rs new file mode 100644 index 0000000000..b7d598ca97 --- /dev/null +++ b/codex-rs/apply-patch/src/text_file.rs @@ -0,0 +1,121 @@ +pub(super) type Replacement = (usize, usize, Vec); + +#[derive(Clone, Copy)] +enum LineEnding { + Lf, + CrLf, + Cr, +} + +impl LineEnding { + fn as_str(self) -> &'static str { + match self { + Self::Lf => "\n", + Self::CrLf => "\r\n", + Self::Cr => "\r", + } + } +} + +struct SourceLine { + text: String, + ending: Option, +} + +pub(super) struct SourceFile { + lines: Vec, + preferred_ending: LineEnding, +} + +impl SourceFile { + /// Splits contents into logical lines while retaining each line ending. + /// + /// The first existing ending becomes the preferred style for inserted + /// lines; files without an ending default to LF. + pub(super) fn parse(contents: &str) -> Self { + let mut lines = Vec::new(); + let mut preferred_ending = None; + let mut line_start = 0; + let mut cursor = 0; + + while cursor < contents.len() { + let (ending, ending_len) = match contents.as_bytes()[cursor] { + b'\r' if contents.as_bytes().get(cursor + 1) == Some(&b'\n') => { + (LineEnding::CrLf, 2) + } + b'\r' => (LineEnding::Cr, 1), + b'\n' => (LineEnding::Lf, 1), + _ => { + cursor += 1; + continue; + } + }; + preferred_ending.get_or_insert(ending); + lines.push(SourceLine { + text: contents[line_start..cursor].to_string(), + ending: Some(ending), + }); + cursor += ending_len; + line_start = cursor; + } + + if line_start < contents.len() { + lines.push(SourceLine { + text: contents[line_start..].to_string(), + ending: None, + }); + } + + Self { + lines, + preferred_ending: preferred_ending.unwrap_or(LineEnding::Lf), + } + } + + pub(super) fn line_texts(&self) -> Vec { + self.lines.iter().map(|line| line.text.clone()).collect() + } + + /// Rebuilds the file from source-ordered, non-overlapping replacements. + /// + /// Unchanged lines retain their original endings, inserted lines use the + /// preferred ending, and every resulting line receives an ending to match + /// apply-patch's historical trailing-newline behavior. + pub(super) fn apply_replacements(&mut self, replacements: &[Replacement]) { + let mut source_lines = std::mem::take(&mut self.lines).into_iter(); + let mut new_lines = Vec::new(); + let mut source_index = 0; + + for (start_idx, old_len, new_segment) in replacements { + debug_assert!(*start_idx >= source_index); + for line in source_lines.by_ref().take(*start_idx - source_index) { + new_lines.push(line); + } + for _ in source_lines.by_ref().take(*old_len) {} + new_lines.extend(new_segment.iter().map(|text| SourceLine { + text: text.clone(), + ending: Some(self.preferred_ending), + })); + source_index = start_idx + old_len; + } + new_lines.extend(source_lines); + self.lines = new_lines; + + // Updates have historically added a trailing newline. This also gives + // an unterminated last line an ending if an insertion moved it inward. + for line in &mut self.lines { + line.ending.get_or_insert(self.preferred_ending); + } + } + + pub(super) fn into_contents(self) -> String { + let mut contents = String::new(); + for line in self.lines { + contents.push_str(&line.text); + if let Some(ending) = line.ending { + contents.push_str(ending.as_str()); + } + } + contents + } +} diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/.gitattributes b/codex-rs/apply-patch/tests/fixtures/scenarios/.gitattributes index a42a20ddc5..3961a5e66a 100644 --- a/codex-rs/apply-patch/tests/fixtures/scenarios/.gitattributes +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/.gitattributes @@ -1 +1,5 @@ ** text eol=lf +023_preserves_crlf_line_endings/input/*.txt -diff -text +023_preserves_crlf_line_endings/expected/*.txt -diff -text +024_preserves_mixed_line_endings/input/*.txt -diff -text +024_preserves_mixed_line_endings/expected/*.txt -diff -text diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/expected/lines.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/expected/lines.txt new file mode 100644 index 0000000000..43321636e6 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/expected/lines.txt @@ -0,0 +1,4 @@ +ONE +two +between +three diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/input/lines.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/input/lines.txt new file mode 100644 index 0000000000..e1587ff9ea --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/input/lines.txt @@ -0,0 +1,3 @@ +one +two +three diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/patch.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/patch.txt new file mode 100644 index 0000000000..6df72a09cf --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/023_preserves_crlf_line_endings/patch.txt @@ -0,0 +1,9 @@ +*** Begin Patch +*** Update File: lines.txt +@@ +-one ++ONE + two ++between + three +*** End Patch diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/expected/lines.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/expected/lines.txt new file mode 100644 index 0000000000..967a20a3a7 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/expected/lines.txt @@ -0,0 +1,3 @@ +one +two THREE +four diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/input/lines.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/input/lines.txt new file mode 100644 index 0000000000..92a67d3b1c --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/input/lines.txt @@ -0,0 +1,3 @@ +one +two three +four diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/patch.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/patch.txt new file mode 100644 index 0000000000..a97b2f35a4 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/024_preserves_mixed_line_endings/patch.txt @@ -0,0 +1,9 @@ +*** Begin Patch +*** Update File: lines.txt +@@ + one + two +-three ++THREE + four +*** End Patch diff --git a/codex-rs/apply-patch/tests/suite/scenarios.rs b/codex-rs/apply-patch/tests/suite/scenarios.rs index 3beb7d652d..b15e697664 100644 --- a/codex-rs/apply-patch/tests/suite/scenarios.rs +++ b/codex-rs/apply-patch/tests/suite/scenarios.rs @@ -1,3 +1,4 @@ +use codex_apply_patch::CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR; use codex_utils_cargo_bin::repo_root; use pretty_assertions::assert_eq; use std::collections::BTreeMap; @@ -44,6 +45,7 @@ fn run_apply_patch_scenario(dir: &Path) -> anyhow::Result<()> { // final filesystem state, which we compare below. Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?) .arg(patch) + .env(CODEX_APPLY_PATCH_PRESERVE_LINE_ENDINGS_ENV_VAR, "1") .current_dir(tmp.path()) .output()?; diff --git a/codex-rs/apply-patch/tests/suite/tool.rs b/codex-rs/apply-patch/tests/suite/tool.rs index 8499d0fb90..7b994c0152 100644 --- a/codex-rs/apply-patch/tests/suite/tool.rs +++ b/codex-rs/apply-patch/tests/suite/tool.rs @@ -1,4 +1,10 @@ use assert_cmd::Command; +use codex_apply_patch::AppliedPatchDelta; +use codex_apply_patch::ApplyPatchFailure; +use codex_apply_patch::ApplyPatchFileUpdateMode; +use codex_apply_patch::apply_patch_with_mode; +use codex_exec_server::LOCAL_FS; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; @@ -11,6 +17,48 @@ fn run_apply_patch_in_dir(dir: &Path, patch: &str) -> anyhow::Result, + stderr: &mut Vec, +) -> Result { + apply_patch_with_mode( + patch, + ApplyPatchFileUpdateMode::PreserveLineEndings, + cwd, + stdout, + stderr, + LOCAL_FS.as_ref(), + /*sandbox*/ None, + ) + .await +} + +async fn assert_apply_patch_updates_file( + file_name: &str, + original: &[u8], + patch: &str, + expected: &[u8], +) -> anyhow::Result<()> { + let tmp = tempdir()?; + let target_path = tmp.path().join(file_name); + fs::write(&target_path, original)?; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let cwd = PathUri::from_host_native_path(tmp.path())?; + run_apply_patch_preserving_line_endings(&cwd, patch, &mut stdout, &mut stderr).await?; + + assert_eq!( + String::from_utf8(stdout)?, + format!("Success. Updated the following files:\nM {file_name}\n") + ); + assert_eq!(stderr, Vec::::new()); + assert_eq!(fs::read(target_path)?, expected); + Ok(()) +} + fn apply_patch_command(dir: &Path) -> anyhow::Result { let mut cmd = Command::new(codex_utils_cargo_bin::cargo_bin("apply_patch")?); cmd.current_dir(dir); @@ -64,6 +112,147 @@ fn test_apply_patch_cli_applies_multiple_chunks() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn test_apply_patch_rejects_overlapping_end_of_file_chunks() -> anyhow::Result<()> { + let tmp = tempdir()?; + let target_path = tmp.path().join("overlapping.txt"); + fs::write(&target_path, "one\n")?; + + let patch = "*** Begin Patch\n*** Update File: overlapping.txt\n@@\n-one\n+first\n@@\n-one\n+second\n*** End of File\n*** End Patch"; + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let cwd = PathUri::from_host_native_path(tmp.path())?; + let error = run_apply_patch_preserving_line_endings(&cwd, patch, &mut stdout, &mut stderr) + .await + .expect_err("overlapping chunks should fail"); + + assert!(error.delta().is_empty()); + assert_eq!(stdout, Vec::::new()); + assert_eq!( + String::from_utf8(stderr)?, + format!( + "Failed to find expected lines in {}:\none\n", + target_path.display() + ) + ); + + assert_eq!(fs::read_to_string(target_path)?, "one\n"); + Ok(()) +} + +#[test] +fn test_apply_patch_cli_allows_overlapping_eof_chunks_in_legacy_mode() -> anyhow::Result<()> { + let tmp = tempdir()?; + let target_path = tmp.path().join("overlapping.txt"); + fs::write(&target_path, "one\n")?; + + let patch = "*** Begin Patch\n*** Update File: overlapping.txt\n@@\n-one\n+first\n@@\n-one\n+second\n*** End of File\n*** End Patch"; + + run_apply_patch_in_dir(tmp.path(), patch)? + .success() + .stdout("Success. Updated the following files:\nM overlapping.txt\n"); + + assert_eq!(fs::read_to_string(target_path)?, "first\n"); + Ok(()) +} + +#[tokio::test] +async fn test_apply_patch_preserves_crlf_from_target_file() -> anyhow::Result<()> { + let patch = "*** Begin Patch\n*** Update File: crlf.txt\n@@\n-one\n+uno\n@@\n two\n+\n+between\n three\n*** End Patch"; + + assert_apply_patch_updates_file( + "crlf.txt", + b"one\r\ntwo\r\nthree\r\n", + patch, + b"uno\r\ntwo\r\n\r\nbetween\r\nthree\r\n", + ) + .await +} + +#[tokio::test] +async fn test_apply_patch_appends_after_trailing_blank_crlf_line() -> anyhow::Result<()> { + let patch = "*** Begin Patch\n*** Update File: trailing_blank.txt\n@@\n+new\n*** End Patch"; + + assert_apply_patch_updates_file( + "trailing_blank.txt", + b"a\r\n\r\n", + patch, + b"a\r\n\r\nnew\r\n", + ) + .await +} + +#[test] +fn test_apply_patch_cli_uses_legacy_line_handling_by_default() -> anyhow::Result<()> { + let tmp = tempdir()?; + let target_path = tmp.path().join("crlf.txt"); + fs::write(&target_path, b"one\r\n")?; + let patch = "*** Begin Patch\n*** Update File: crlf.txt\n@@\n-one\n+uno\n*** End Patch"; + + run_apply_patch_in_dir(tmp.path(), patch)?.success(); + + assert_eq!(fs::read(target_path)?, b"uno\n"); + Ok(()) +} + +#[tokio::test] +async fn test_apply_patch_preserves_cr_from_target_file() -> anyhow::Result<()> { + let patch = "*** Begin Patch\n*** Update File: cr.txt\n@@\n-one\n+uno\n@@\n two\n+\n+between\n three\n*** End Patch"; + + assert_apply_patch_updates_file( + "cr.txt", + b"one\rtwo\rthree\r", + patch, + b"uno\rtwo\r\rbetween\rthree\r", + ) + .await +} + +#[tokio::test] +async fn test_apply_patch_preserves_change_order_with_repeated_lines() -> anyhow::Result<()> { + let patch = + "*** Begin Patch\n*** Update File: repeated.txt\n@@\n-a\n-b\n+b\n+b\n+a\n*** End Patch"; + + assert_apply_patch_updates_file("repeated.txt", b"a\nb\n", patch, b"b\nb\na\n").await +} + +#[tokio::test] +async fn test_apply_patch_preserves_repeated_context_line_ending() -> anyhow::Result<()> { + let patch = + "*** Begin Patch\n*** Update File: repeated_context.txt\n@@\n-same\n same\n*** End Patch"; + + assert_apply_patch_updates_file("repeated_context.txt", b"same\r\nsame\n", patch, b"same\n") + .await +} + +#[tokio::test] +async fn test_apply_patch_preserves_untouched_mixed_line_endings() -> anyhow::Result<()> { + let patch = "*** Begin Patch\n*** Update File: mixed.txt\n@@\n one\n two\n-three\n+THREE\n four\n*** End Patch"; + + assert_apply_patch_updates_file( + "mixed.txt", + b"one\r\ntwo\rthree\nfour\r\n", + patch, + b"one\r\ntwo\rTHREE\r\nfour\r\n", + ) + .await +} + +#[tokio::test] +async fn test_apply_patch_uses_crlf_for_new_trailing_newline() -> anyhow::Result<()> { + let patch = + "*** Begin Patch\n*** Update File: no_trailing_newline.txt\n@@\n-one\n+ONE\n*** End Patch"; + + assert_apply_patch_updates_file( + "no_trailing_newline.txt", + b"one\r\ntwo", + patch, + b"ONE\r\ntwo\r\n", + ) + .await +} + #[test] fn test_apply_patch_cli_moves_file_to_new_directory() -> anyhow::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 7951d2cb05..50ec7cff18 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -129,8 +129,10 @@ pub fn arg0_dispatch() -> Option { Err(_) => std::process::exit(1), }; let cwd = cwd.into(); - match runtime.block_on(codex_apply_patch::apply_patch( + let update_file_mode = codex_apply_patch::apply_patch_file_update_mode_from_env(); + match runtime.block_on(codex_apply_patch::apply_patch_with_mode( &patch_arg, + update_file_mode, &cwd, &mut stdout, &mut stderr,