apply_patch: StreamingPatchParser::finish() accepts a patch that never began

What issue are you seeing?

StreamingPatchParser::finish() (codex-rs/apply-patch/src/streaming_parser.rs:154-173) special-cases a final buffered line equal to *** End Patch, so that input ending without a trailing newline is still terminated:

if line.trim() == END_PATCH_MARKER {
    self.ensure_update_hunk_is_not_empty(line.trim())?;
    self.state.mode = StreamingParserMode::EndedPatch;   // no mode check
} else {
    self.process_line(&line)?;
}

The branch sets mode = EndedPatch directly, bypassing process_line's state machine, so it applies from any mode. Every other line — including an identical *** End Patch that happens to be followed by \n — goes through process_line, which validates the current mode.

The only difference between accepting and rejecting these inputs is a trailing newline.

Case A — a patch with no *** Begin Patch:

let mut parser = StreamingPatchParser::default();
parser.push_delta("*** End Patch");   // no trailing newline
parser.finish()

• Expected: Err(InvalidPatchError("The first line of the patch must be '*** Begin Patch'"))
• Actual: Ok(vec![]) — accepted as a valid, empty patch

Add one newline (push_delta("*** End Patch\n")) and the same parser rejects it correctly via the NotStarted arm at lines 178-186.

Case B — content after the patch already ended:

let mut parser = StreamingPatchParser::default();
parser.push_delta("*** Begin Patch\n*** Add File: f.txt\n+x\n*** End Patch\n");
parser.push_delta("*** End Patch");   // no trailing newline
parser.finish()

• Expected: Err(InvalidPatchError("The last line of the patch must be '*** End Patch'"))
• Actual: Ok(vec![AddFile { .. }]) — the duplicate trailing marker is silently swallowed

The existing test test_streaming_patch_parser_rejects_content_after_end_patch (line 790) asserts precisely this rejection for "...*** End Patch\nextra\n". The same rejection is bypassed when the trailing content is itself *** End Patch and lacks a newline.

Why this looks unintended

finish()'s only stated job (lines 166-170) is to enforce that the patch ended with *** End Patch. It is not documented or tested as a way to start or re-end a patch. No existing test exercises finish() from NotStarted or EndedPatch mode — the three finish() tests (lines 731, 756, 782) all run from AddFile/UpdateFile mode, where the special case is genuinely needed (the test at line 741 relies on it to treat a trailing *** End Patch as the terminator rather than a context line).

Impact

Scoped, and worth stating plainly: parse_patch is unaffected, because check_patch_boundaries_strict (parser.rs:215-223) independently rejects a first line that is not *** Begin Patch. The reachable path is the public StreamingPatchParser API used by codex-rs/core/src/tools/handlers/apply_patch.rs:147 (finish_update_on_complete), where finish() is the validation gate producing the model-facing failed to parse apply_patch: ... error. This is a validation gate that fails open, not a patch-application exploit.

Possible fix

Gate the special case on the patch actually being open, so NotStarted and EndedPatch fall through to process_line and produce the same errors the newline-terminated form does.

A branch with the fix and two regression tests is at https://github.com/Shivansh1205/codex/tree/fix/streaming-parser-finish-mode — both tests fail before the change and pass after, and the full codex-apply-patch suite (67 unit + 3 integration tests) passes, including test_streaming_patch_parser_handles_trailing_end_patch_without_newline, which the special case exists to serve.

Environment

Observed on main at commit 36f0dbe796. Verified with rustc 1.93.0 (repo pins 1.95.0).

🤖 Generated with Claude Code

Start in codex-rs/apply-patch/src/streaming_parser.rs:154-173 and read the finish() state handling alongside process_line(). Run the streaming parser tests, including test_streaming_patch_parser_rejects_content_after_end_patch and the finish() tests near lines 731, 756, and 782. Done means unterminated, not-started, and already-ended inputs are rejected consistently while the no-trailing-newline terminator case still passes.