diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index fe4fe584d..645f39684 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -699,7 +699,13 @@ fn derive_new_contents_from_chunks( } }; - let original_lines: Vec = build_lines_from_contents(&original_contents); + 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 replacements = compute_replacements(&original_lines, path, chunks)?; let new_lines = apply_replacements(original_lines, &replacements); @@ -707,67 +713,13 @@ fn derive_new_contents_from_chunks( if !new_lines.last().is_some_and(String::is_empty) { new_lines.push(String::new()); } - let new_contents = build_contents_from_lines(&original_contents, &new_lines); + let new_contents = new_lines.join("\n"); Ok(AppliedPatch { original_contents, new_contents, }) } -// TODO(dylan-hurd-oai): I think we can migrate to just use `contents.lines()` -// across all platforms. -fn build_lines_from_contents(contents: &str) -> Vec { - if cfg!(windows) { - contents.lines().map(String::from).collect() - } else { - let mut lines: Vec = 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 lines.last().is_some_and(String::is_empty) { - lines.pop(); - } - - lines - } -} - -fn build_contents_from_lines(original_contents: &str, lines: &[String]) -> String { - if cfg!(windows) { - // for now, only compute this if we're on Windows. - let uses_crlf = contents_uses_crlf(original_contents); - if uses_crlf { - lines.join("\r\n") - } else { - lines.join("\n") - } - } else { - lines.join("\n") - } -} - -/// Detects whether the source file uses Windows CRLF line endings consistently. -/// We only consider a file CRLF-formatted if every newline is part of a -/// CRLF sequence. This avoids rewriting an LF-formatted file that merely -/// contains embedded sequences of "\r\n". -/// -/// Returns `true` if the file uses CRLF line endings, `false` otherwise. -fn contents_uses_crlf(contents: &str) -> bool { - let bytes = contents.as_bytes(); - let mut n_newlines = 0usize; - let mut n_crlf = 0usize; - for i in 0..bytes.len() { - if bytes[i] == b'\n' { - n_newlines += 1; - if i > 0 && bytes[i - 1] == b'\r' { - n_crlf += 1; - } - } - } - - n_newlines > 0 && n_crlf == n_newlines -} - /// 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)`. @@ -1414,72 +1366,6 @@ PATCH"#, assert_eq!(contents, "a\nB\nc\nd\nE\nf\ng\n"); } - /// Ensure CRLF line endings are preserved for updated files on Windows‑style inputs. - #[cfg(windows)] - #[test] - fn test_preserve_crlf_line_endings_on_update() { - let dir = tempdir().unwrap(); - let path = dir.path().join("crlf.txt"); - - // Original file uses CRLF (\r\n) endings. - std::fs::write(&path, b"a\r\nb\r\nc\r\n").unwrap(); - - // Replace `b` -> `B` and append `d`. - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ - a --b -+B -@@ - c -+d -*** End of File"#, - path.display() - )); - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); - - let out = std::fs::read(&path).unwrap(); - // Expect all CRLF endings; count occurrences of CRLF and ensure there are 4 lines. - let content = String::from_utf8_lossy(&out); - assert!(content.contains("\r\n")); - // No bare LF occurrences immediately preceding a non-CR: the text should not contain "a\nb". - assert!(!content.contains("a\nb")); - // Validate exact content sequence with CRLF delimiters. - assert_eq!(content, "a\r\nB\r\nc\r\nd\r\n"); - } - - /// Ensure CRLF inputs with embedded carriage returns in the content are preserved. - #[cfg(windows)] - #[test] - fn test_preserve_crlf_embedded_carriage_returns_on_append() { - let dir = tempdir().unwrap(); - let path = dir.path().join("crlf_cr_content.txt"); - - // Original file: first line has a literal '\r' in the content before the CRLF terminator. - std::fs::write(&path, b"foo\r\r\nbar\r\n").unwrap(); - - // Append a new line without modifying existing ones. - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ -+BAZ -*** End of File"#, - path.display() - )); - - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); - - let out = std::fs::read(&path).unwrap(); - // CRLF endings must be preserved and the extra CR in "foo\r\r" must not be collapsed. - assert_eq!(out.as_slice(), b"foo\r\r\nbar\r\nBAZ\r\n"); - } - #[test] fn test_pure_addition_chunk_followed_by_removal() { let dir = tempdir().unwrap(); @@ -1665,37 +1551,6 @@ PATCH"#, assert_eq!(expected, diff); } - /// For LF-only inputs with a trailing newline ensure that the helper used - /// on Windows-style builds drops the synthetic trailing empty element so - /// replacements behave like standard `diff` line numbering. - #[test] - fn test_derive_new_contents_lf_trailing_newline() { - let dir = tempdir().unwrap(); - let path = dir.path().join("lf_trailing_newline.txt"); - fs::write(&path, "foo\nbar\n").unwrap(); - - let patch = wrap_patch(&format!( - r#"*** Update File: {} -@@ - foo --bar -+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 AppliedPatch { new_contents, .. } = - derive_new_contents_from_chunks(&path, chunks).unwrap(); - - assert_eq!(new_contents, "foo\nBAR\n"); - } - #[test] fn test_unified_diff_insert_at_eof() { // Insert a new line at end‑of‑file. diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 70c8aa4fa..880e74d95 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -1250,94 +1250,3 @@ async fn apply_patch_change_context_disambiguates_target( assert_eq!(contents, "fn a\nx=10\ny=2\nfn b\nx=11\ny=20\n"); Ok(()) } - -/// Ensure that applying a patch can update a CRLF file with unicode characters. -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -#[test_case(ApplyPatchModelOutput::Freeform)] -#[test_case(ApplyPatchModelOutput::Function)] -#[test_case(ApplyPatchModelOutput::Shell)] -#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)] -#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)] -async fn apply_patch_cli_updates_unicode_characters( - model_output: ApplyPatchModelOutput, -) -> Result<()> { - skip_if_no_network!(Ok(())); - - let harness = apply_patch_harness().await?; - - let target = harness.path("unicode.txt"); - fs::write(&target, "first ⚠️\nsecond ❌\nthird 🔥\n")?; - - let patch = format!( - r#"*** Begin Patch -*** Update File: {} -@@ - first ⚠️ --second ❌ -+SECOND ✅ -@@ - third 🔥 -+FOURTH -*** End of File -*** End Patch"#, - target.display() - ); - let call_id = "apply-unicode-update"; - mount_apply_patch(&harness, call_id, patch.as_str(), "ok", model_output).await; - - harness - .submit("update unicode characters via apply_patch CLI") - .await?; - - let file_contents = fs::read(&target)?; - let content = String::from_utf8_lossy(&file_contents); - assert_eq!(content, "first ⚠️\nSECOND ✅\nthird 🔥\nFOURTH\n"); - Ok(()) -} - -/// Ensure that applying a patch via the CLI preserves CRLF line endings for -/// Windows-style inputs even when updating the file contents. -#[cfg(windows)] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -#[test_case(ApplyPatchModelOutput::Freeform)] -#[test_case(ApplyPatchModelOutput::Function)] -#[test_case(ApplyPatchModelOutput::Shell)] -#[test_case(ApplyPatchModelOutput::ShellViaHeredoc)] -#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc)] -async fn apply_patch_cli_updates_crlf_file_preserves_line_endings( - model_output: ApplyPatchModelOutput, -) -> Result<()> { - skip_if_no_network!(Ok(())); - - let harness = apply_patch_harness().await?; - - let target = harness.path("crlf.txt"); - fs::write(&target, b"first\r\nsecond\r\nthird\r\n")?; - - let patch = format!( - r#"*** Begin Patch -*** Update File: {} -@@ - first --second -+SECOND -@@ - third -+FOURTH -*** End of File -*** End Patch"#, - target.display() - ); - let call_id = "apply-crlf-update"; - mount_apply_patch(&harness, call_id, patch.as_str(), "ok", model_output).await; - - harness - .submit("update crlf file via apply_patch CLI") - .await?; - - let file_contents = fs::read(&target)?; - let content = String::from_utf8_lossy(&file_contents); - assert!(content.contains("\r\n")); - assert_eq!(content, "first\r\nSECOND\r\nthird\r\nFOURTH\r\n"); - Ok(()) -}