[apply-patch] Handle multiple context lines #2596

dylan-hurd-oai · 2025-08-22T22:45:19Z

Summary

Fixes #2578 - the following example would be considered incorrect, despite it being valid according to our system instructions and our official Lark grammar.

*** Begin Patch
*** Update File: src/app.py
@@ class BaseClass
@@     def method():
- old_line
+ new_line
*** End Patch

Testing

Added unit tests

dylan-hurd-oai · 2025-08-23T17:14:23Z

codex-rs/apply-patch/src/lib.rs

+                    original_lines.len() - 1
+                } else {
+                    original_lines.len()
+                }


@bolinfest this more nuanced handling feels more correct to me but would love your opinion here

codex-rs/exec/tests/apply_patch.rs

github-actions · 2025-08-23T17:20:40Z

Summary

Enables multi-line context anchors in apply-patch hunks and anchors pure additions at the last matched context (or EOF appropriately). Updates parser API (change_context → context_lines) with unit tests and adds an end-to-end parsing test. Also tightens Wiremock path matching in exec tests.

Notable changes

codex-rs/apply-patch/src/parser.rs: Parses multiple @@ context markers sequentially into context_lines, updates error offsets, and adjusts tests accordingly.
codex-rs/apply-patch/src/lib.rs: Seeks each context line in order and refines insertion index for pure additions; adds tests for single and multi-context anchors.
codex-rs/exec/tests/apply_patch.rs: Uncomments .and(path("/v1/responses")) in existing tests and adds a new e2e test covering multi-context grammar.

Review

Solid, focused improvement with clear API update and good test coverage; the insertion logic and ordered context search look correct and idiomatic.

Suggestions

codex-rs/exec/tests/apply_patch.rs (new e2e test hunk): First mock keeps the path matcher commented; consider enabling .and(path("/v1/responses")) for consistency with the other two mocks.
codex-rs/exec/tests/apply_patch.rs (new e2e test hunk): Comment says “create test.md” but the file is app.py; update the comment to avoid confusion.
codex-rs/apply-patch/src/lib.rs (pure addition branch): Nice behavior; consider adding a test where pure addition uses EOF with and without trailing newline to lock in both cases.

View workflow run

chatgpt-codex-connector

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

chatgpt-codex-connector · 2025-08-23T20:31:42Z

codex-rs/apply-patch/src/lib.rs

    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,
-                false,
-            ) {
-                line_index = idx + 1;
-            } else {
-                return Err(ApplyPatchError::ComputeReplacements(format!(
-                    "Failed to find context '{}' in {}",
-                    ctx_line,
-                    path.display()
-                )));
+        // If a chunk has context lines, we use seek_sequence to find each in order,
+        // then adjust our `line_index` to continue from there.
+        if !chunk.context_lines.is_empty() {
+            let total = chunk.context_lines.len();
+            for (i, ctx_line) in chunk.context_lines.iter().enumerate() {
+                if let Some(idx) = seek_sequence::seek_sequence(
+                    original_lines,
+                    std::slice::from_ref(ctx_line),
+                    line_index,
+                    false,
+                ) {
+                    line_index = idx + 1;
+                } else {
+                    return Err(ApplyPatchError::ComputeReplacements(format!(
+                        "Failed to find context {}/{}: '{}' in {}",
+                        i + 1,
+                        total,
+                        ctx_line,
+                        path.display()
+                    )));
+                }
            }
        }

        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(|s| s.is_empty()) {
-                original_lines.len() - 1
+            // Pure addition (no old lines).
+            // Prefer to insert at the matched context anchor if one exists and
+            // the hunk is not explicitly marked as end-of-file.
+            let insertion_idx = if chunk.is_end_of_file {
+                if original_lines.last().is_some_and(|s| s.is_empty()) {
+                    original_lines.len() - 1
+                } else {
+                    original_lines.len()
+                }
+            } else if !chunk.context_lines.is_empty() {
+                // Insert immediately after the last matched context line.
+                line_index
            } else {
-                original_lines.len()
+                // No context provided: fall back to appending at the end (before
+                // the trailing empty line if present).
+                if original_lines.last().is_some_and(|s| s.is_empty()) {
+                    original_lines.len() - 1
+                } else {
+                    original_lines.len()
+                }
            };
            replacements.push((insertion_idx, 0, chunk.new_lines.clone()));


[P1] Failing to apply multiple chunks under same outer context

compute_replacements advances line_index past each processed chunk and then searches subsequent context lines starting from that position (lib.rs:529‑579). With multi‑level context lines, two chunks in the same class (e.g., methods A and B each prefixed with @@ class Foo) cause the second search to begin after the class declaration so it can’t be found, aborting the patch. This prevents valid multi‑chunk patches within one scope.

Useful? React with 👍 / 👎.

bolinfest

Overall LGTM, just some small suggestions!

bolinfest · 2025-08-25T23:32:52Z

codex-rs/apply-patch/src/lib.rs

+            let insertion_idx = if chunk.is_end_of_file {
+                if original_lines.last().is_some_and(|s| s.is_empty()) {
+                    original_lines.len() - 1
+                } else {
+                    original_lines.len()
+                }
+            } else if !chunk.context_lines.is_empty() {
+                // Insert immediately after the last matched context line.
+                line_index


Is this correct?

Suggested change

let insertion_idx = if chunk.is_end_of_file {

if original_lines.last().is_some_and(|s| s.is_empty()) {

original_lines.len() - 1

} else {

original_lines.len()

}

} else if !chunk.context_lines.is_empty() {

// Insert immediately after the last matched context line.

line_index

let insertion_idx = if !chunk.is_end_of_file && !chunk.context_lines.is_empty() {

// Insert immediately after the last matched context line.

line_index

} else if original_lines.last().is_some_and(|s| s.is_empty()) {

original_lines.len() - 1

} else {

original_lines.len()

};

bolinfest · 2025-08-25T23:35:39Z

codex-rs/exec/tests/fixtures/sse_apply_patch_context_update.json

+      "output": []
+    }
+  }
+]


newline at eof?

bolinfest · 2025-08-25T23:35:53Z

codex-rs/exec/tests/suite/apply_patch.rs

@@ -106,3 +106,41 @@ async fn test_apply_patch_freeform_tool() -> anyhow::Result<()> {
    );
    Ok(())
 }
+
+#[cfg(not(target_os = "windows"))]


Are you sure these don't work on Windows?

dylan-hurd-oai commented Aug 23, 2025

View reviewed changes

dylan-hurd-oai force-pushed the codex--apply-patch-context-fix branch from 0aa030e to bd8deb1 Compare August 23, 2025 17:16

dylan-hurd-oai added the codex-rust-review Perform a detailed review of Rust changes. label Aug 23, 2025

dylan-hurd-oai commented Aug 23, 2025

View reviewed changes

codex-rs/exec/tests/apply_patch.rs Outdated Show resolved Hide resolved

github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Aug 23, 2025

github-actions bot added codex-rust-review-completed and removed codex-rust-review-in-progress labels Aug 23, 2025

dylan-hurd-oai requested a review from bolinfest August 23, 2025 17:57

dylan-hurd-oai marked this pull request as ready for review August 23, 2025 20:21

chatgpt-codex-connector bot reviewed Aug 23, 2025

View reviewed changes

dylan-hurd-oai force-pushed the codex--apply-patch-context-fix branch from bd8deb1 to eadc5f3 Compare August 25, 2025 01:24

[apply-patch] Handle multiple context lines

72d8bde

dylan-hurd-oai force-pushed the codex--apply-patch-context-fix branch from eadc5f3 to 72d8bde Compare August 25, 2025 22:10

bolinfest approved these changes Aug 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[apply-patch] Handle multiple context lines #2596

[apply-patch] Handle multiple context lines #2596

dylan-hurd-oai commented Aug 22, 2025 •

edited

Loading

Uh oh!

dylan-hurd-oai Aug 23, 2025

Uh oh!

Uh oh!

github-actions bot commented Aug 23, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Aug 23, 2025

Uh oh!

bolinfest left a comment

Uh oh!

bolinfest Aug 25, 2025

Uh oh!

bolinfest Aug 25, 2025

Uh oh!

bolinfest Aug 25, 2025

Uh oh!

Uh oh!

[apply-patch] Handle multiple context lines #2596

Are you sure you want to change the base?

[apply-patch] Handle multiple context lines #2596

Conversation

dylan-hurd-oai commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

dylan-hurd-oai Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 23, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

bolinfest Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

bolinfest Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

bolinfest Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dylan-hurd-oai commented Aug 22, 2025 •

edited

Loading