From 6b2c572c7a2e3cd662afaf7ee62c8a202e32ae53 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 12 Jun 2026 10:04:45 -0700 Subject: [PATCH 1/4] unify apply patch parsing --- codex-rs/apply-patch/src/parser.rs | 362 ++----------------- codex-rs/apply-patch/src/streaming_parser.rs | 95 ++++- 2 files changed, 121 insertions(+), 336 deletions(-) diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index b3b2337c392e..5854f2616f8f 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -24,6 +24,7 @@ //! The parser below is a little more lenient than the explicit spec and allows for //! leading/trailing whitespace around patch markers. use crate::ApplyPatchArgs; +use crate::streaming_parser::StreamingPatchParser; use codex_utils_absolute_path::AbsolutePathBuf; #[cfg(test)] use codex_utils_absolute_path::test_support::PathBufExt; @@ -33,7 +34,6 @@ use std::path::PathBuf; use thiserror::Error; pub(crate) const BEGIN_PATCH_MARKER: &str = "*** Begin Patch"; -pub(crate) const ENVIRONMENT_ID_MARKER: &str = "*** Environment ID: "; pub(crate) const END_PATCH_MARKER: &str = "*** End Patch"; pub(crate) const ADD_FILE_MARKER: &str = "*** Add File: "; pub(crate) const DELETE_FILE_MARKER: &str = "*** Delete File: "; @@ -107,6 +107,7 @@ impl Hunk { } } +#[cfg(test)] use Hunk::*; #[derive(Debug, PartialEq, Clone)] @@ -175,21 +176,16 @@ enum ParseMode { fn parse_patch_text(patch: &str, mode: ParseMode) -> Result { let lines: Vec<&str> = patch.trim().lines().collect(); - let (patch_lines, hunk_lines) = match mode { + let patch_lines = match mode { ParseMode::Strict => check_patch_boundaries_strict(&lines)?, ParseMode::Lenient => check_patch_boundaries_lenient(&lines)?, }; - let (environment_id, mut remaining_lines, mut line_number) = - parse_environment_id_preamble(hunk_lines)?; - let mut hunks: Vec = Vec::new(); - while !remaining_lines.is_empty() { - let (hunk, hunk_lines) = parse_one_hunk(remaining_lines, line_number)?; - hunks.push(hunk); - line_number += hunk_lines; - remaining_lines = &remaining_lines[hunk_lines..] - } let patch = patch_lines.join("\n"); + let mut parser = StreamingPatchParser::default(); + parser.push_delta(&patch)?; + let hunks = parser.finish()?; + let environment_id = parser.environment_id().map(str::to_owned); Ok(ApplyPatchArgs { hunks, patch, @@ -198,36 +194,16 @@ fn parse_patch_text(patch: &str, mode: ParseMode) -> Result( - hunk_lines: &'a [&'a str], -) -> Result<(Option, &'a [&'a str], usize), ParseError> { - let Some(first_line) = hunk_lines.first() else { - return Ok((None, hunk_lines, 2)); - }; - let Some(environment_id) = first_line.trim_start().strip_prefix(ENVIRONMENT_ID_MARKER) else { - return Ok((None, hunk_lines, 2)); - }; - let environment_id = environment_id.trim(); - if environment_id.is_empty() { - return Err(InvalidPatchError( - "apply_patch environment_id cannot be empty".to_string(), - )); - } - Ok((Some(environment_id.to_string()), &hunk_lines[1..], 3)) -} - /// Checks the start and end lines of the patch text for `apply_patch`, /// returning an error if they do not match the expected markers. -fn check_patch_boundaries_strict<'a>( - lines: &'a [&'a str], -) -> Result<(&'a [&'a str], &'a [&'a str]), ParseError> { +fn check_patch_boundaries_strict<'a>(lines: &'a [&'a str]) -> Result<&'a [&'a str], ParseError> { let (first_line, last_line) = match lines { [] => (None, None), [first] => (Some(first), Some(first)), [first, .., last] => (Some(first), Some(last)), }; check_start_and_end_lines_strict(first_line, last_line)?; - Ok((lines, &lines[1..lines.len() - 1])) + Ok(lines) } /// If we are in lenient mode, we check if the first line starts with `<( /// contents, excluding the heredoc markers. fn check_patch_boundaries_lenient<'a>( original_lines: &'a [&'a str], -) -> Result<(&'a [&'a str], &'a [&'a str]), ParseError> { +) -> Result<&'a [&'a str], ParseError> { let original_parse_error = match check_patch_boundaries_strict(original_lines) { Ok(lines) => return Ok(lines), Err(e) => e, @@ -281,300 +257,6 @@ fn check_start_and_end_lines_strict( } } -/// Attempts to parse a single hunk from the start of lines. -/// Returns the parsed hunk and the number of lines parsed (or a ParseError). -fn parse_one_hunk(lines: &[&str], line_number: usize) -> Result<(Hunk, usize), ParseError> { - let first_line = lines[0].trim(); - if let Some(path) = first_line.strip_prefix(ADD_FILE_MARKER) { - let mut contents = String::new(); - let mut parsed_lines = 1; - for add_line in &lines[1..] { - if let Some(line_to_add) = add_line.strip_prefix('+') { - contents.push_str(line_to_add); - contents.push('\n'); - parsed_lines += 1; - } else { - break; - } - } - return Ok(( - AddFile { - path: PathBuf::from(path), - contents, - }, - parsed_lines, - )); - } else if let Some(path) = first_line.strip_prefix(DELETE_FILE_MARKER) { - return Ok(( - DeleteFile { - path: PathBuf::from(path), - }, - 1, - )); - } else if let Some(path) = first_line.strip_prefix(UPDATE_FILE_MARKER) { - let mut remaining_lines = &lines[1..]; - let mut parsed_lines = 1; - let move_path = remaining_lines - .first() - .and_then(|x| x.strip_prefix(MOVE_TO_MARKER)); - - if move_path.is_some() { - remaining_lines = &remaining_lines[1..]; - parsed_lines += 1; - } - - let mut chunks = Vec::new(); - while !remaining_lines.is_empty() { - if remaining_lines[0].trim().is_empty() { - parsed_lines += 1; - remaining_lines = &remaining_lines[1..]; - continue; - } - - if remaining_lines[0].starts_with('*') { - break; - } - - let (chunk, chunk_lines) = parse_update_file_chunk( - remaining_lines, - line_number + parsed_lines, - chunks.is_empty(), - )?; - chunks.push(chunk); - parsed_lines += chunk_lines; - remaining_lines = &remaining_lines[chunk_lines..] - } - - if chunks.is_empty() { - return Err(InvalidHunkError { - message: format!( - "Update file hunk for path '{}' is empty", - Path::new(path).display() - ), - line_number, - }); - } - - return Ok(( - UpdateFile { - path: PathBuf::from(path), - move_path: move_path.map(PathBuf::from), - chunks, - }, - parsed_lines, - )); - } - - Err(InvalidHunkError { - message: format!( - "'{first_line}' is not a valid hunk header. Valid hunk headers: '*** Add File: {{path}}', '*** Delete File: {{path}}', '*** Update File: {{path}}'" - ), - line_number, - }) -} - -fn parse_update_file_chunk( - lines: &[&str], - line_number: usize, - allow_missing_context: bool, -) -> Result<(UpdateFileChunk, usize), ParseError> { - if lines.is_empty() { - return Err(InvalidHunkError { - message: "Update hunk does not contain any lines".to_string(), - line_number, - }); - } - let (change_context, start_index) = if lines[0] == EMPTY_CHANGE_CONTEXT_MARKER { - (None, 1) - } else if let Some(context) = lines[0].strip_prefix(CHANGE_CONTEXT_MARKER) { - (Some(context.to_string()), 1) - } else { - if !allow_missing_context { - return Err(InvalidHunkError { - message: format!( - "Expected update hunk to start with a @@ context marker, got: '{}'", - lines[0] - ), - line_number, - }); - } - (None, 0) - }; - if start_index >= lines.len() { - return Err(InvalidHunkError { - message: "Update hunk does not contain any lines".to_string(), - line_number: line_number + 1, - }); - } - let mut chunk = UpdateFileChunk { - change_context, - old_lines: Vec::new(), - new_lines: Vec::new(), - is_end_of_file: false, - }; - let mut parsed_lines = 0; - for line in &lines[start_index..] { - match *line { - EOF_MARKER => { - if parsed_lines == 0 { - return Err(InvalidHunkError { - message: "Update hunk does not contain any lines".to_string(), - line_number: line_number + 1, - }); - } - chunk.is_end_of_file = true; - parsed_lines += 1; - break; - } - line_contents => { - match line_contents.chars().next() { - None => { - // Interpret this as an empty line. - chunk.old_lines.push(String::new()); - chunk.new_lines.push(String::new()); - } - Some(' ') => { - chunk.old_lines.push(line_contents[1..].to_string()); - chunk.new_lines.push(line_contents[1..].to_string()); - } - Some('+') => { - chunk.new_lines.push(line_contents[1..].to_string()); - } - Some('-') => { - chunk.old_lines.push(line_contents[1..].to_string()); - } - _ => { - if parsed_lines == 0 { - return Err(InvalidHunkError { - message: format!( - "Unexpected line found in update hunk: '{line_contents}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)" - ), - line_number: line_number + 1, - }); - } - // Assume this is the start of the next hunk. - break; - } - } - parsed_lines += 1; - } - } - } - - Ok((chunk, parsed_lines + start_index)) -} - -#[test] -fn test_parse_one_hunk() { - assert_eq!( - parse_one_hunk(&["bad"], /*line_number*/ 234), - Err(InvalidHunkError { - message: "'bad' is not a valid hunk header. \ - Valid hunk headers: '*** Add File: {path}', '*** Delete File: {path}', '*** Update File: {path}'".to_string(), - line_number: 234 - }) - ); -} - -#[test] -fn test_update_file_chunk() { - assert_eq!( - parse_update_file_chunk( - &["bad"], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Err(InvalidHunkError { - message: "Expected update hunk to start with a @@ context marker, got: 'bad'" - .to_string(), - line_number: 123 - }) - ); - assert_eq!( - parse_update_file_chunk( - &["@@"], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Err(InvalidHunkError { - message: "Update hunk does not contain any lines".to_string(), - line_number: 124 - }) - ); - assert_eq!( - parse_update_file_chunk( - &["@@", "bad"], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Err(InvalidHunkError { - message: "Unexpected line found in update hunk: 'bad'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)".to_string(), - line_number: 124 - }) - ); - assert_eq!( - parse_update_file_chunk( - &["@@", "*** End of File"], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Err(InvalidHunkError { - message: "Update hunk does not contain any lines".to_string(), - line_number: 124 - }) - ); - assert_eq!( - parse_update_file_chunk( - &[ - "@@ change_context", - "", - " context", - "-remove", - "+add", - " context2", - "*** End Patch", - ], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Ok(( - UpdateFileChunk { - change_context: Some("change_context".to_string()), - old_lines: vec![ - String::new(), - "context".to_string(), - "remove".to_string(), - "context2".to_string(), - ], - new_lines: vec![ - String::new(), - "context".to_string(), - "add".to_string(), - "context2".to_string(), - ], - is_end_of_file: false, - }, - 6, - )) - ); - assert_eq!( - parse_update_file_chunk( - &["@@", "+line", "*** End of File"], - /*line_number*/ 123, - /*allow_missing_context*/ false, - ), - Ok(( - UpdateFileChunk { - change_context: None, - old_lines: Vec::new(), - new_lines: vec!["line".to_string()], - is_end_of_file: true, - }, - 3, - )) - ); -} - #[test] fn test_parse_patch() { assert_eq!( @@ -725,6 +407,30 @@ fn test_parse_patch() { ); } +#[test] +fn test_parse_patch_preserves_end_of_file_marker() { + let patch = + "*** Begin Patch\n*** Update File: file.txt\n@@\n+quux\n*** End of File\n\n*** End Patch"; + assert_eq!( + parse_patch(patch), + Ok(ApplyPatchArgs { + hunks: vec![UpdateFile { + path: PathBuf::from("file.txt"), + move_path: None, + chunks: vec![UpdateFileChunk { + change_context: None, + old_lines: Vec::new(), + new_lines: vec!["quux".to_string()], + is_end_of_file: true, + }], + }], + patch: patch.to_string(), + workdir: None, + environment_id: None, + }) + ); +} + #[test] fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() { let dir = tempfile::tempdir().unwrap(); diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index 86084af0b8c1..a6a23c5e2c95 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -29,6 +29,7 @@ pub struct StreamingPatchParser { struct StreamingParserState { mode: StreamingParserMode, hunks: Vec, + environment_id: Option, } #[derive(Debug, Default, Clone, Copy)] @@ -45,11 +46,8 @@ enum StreamingParserMode { } impl StreamingPatchParser { - // The live streaming parser only needs to keep the patch preview flowing. - // Environment selection and validation happen on the final tool invocation, - // so here we just tolerate and skip the optional preamble line. - fn is_environment_id_preamble_line(&self, line: &str) -> bool { - line.starts_with(ENVIRONMENT_ID_MARKER) + pub fn environment_id(&self) -> Option<&str> { + self.state.environment_id.as_deref() } fn ensure_update_hunk_is_not_empty(&self, line: &str) -> Result<(), ParseError> { @@ -170,7 +168,17 @@ impl StreamingPatchParser { )) } StreamingParserMode::StartedPatch => { - if self.is_environment_id_preamble_line(line) { + if self.line_number == 2 + && let Some(environment_id) = + line.trim_start().strip_prefix(ENVIRONMENT_ID_MARKER) + { + let environment_id = environment_id.trim(); + if environment_id.is_empty() { + return Err(InvalidPatchError( + "apply_patch environment_id cannot be empty".to_string(), + )); + } + self.state.environment_id = Some(environment_id.to_string()); return Ok(()); } if self.handle_hunk_headers_and_end_patch(trimmed)? { @@ -222,6 +230,22 @@ impl StreamingPatchParser { move_path, chunks, .. }) = self.state.hunks.last_mut() { + if chunks.last().is_some_and(|chunk| chunk.is_end_of_file) { + if update_line.is_empty() { + return Ok(()); + } + if update_line != EMPTY_CHANGE_CONTEXT_MARKER + && !update_line.starts_with(CHANGE_CONTEXT_MARKER) + { + return Err(InvalidHunkError { + message: format!( + "Expected update hunk to start with a @@ context marker, got: '{line}'" + ), + line_number: self.line_number, + }); + } + } + if chunks.is_empty() && move_path.is_none() && let Some(move_to_path) = update_line.strip_prefix(MOVE_TO_MARKER) @@ -367,7 +391,15 @@ impl StreamingPatchParser { line_number: self.line_number, }) } - StreamingParserMode::EndedPatch => Ok(()), + StreamingParserMode::EndedPatch => { + if trimmed.is_empty() { + Ok(()) + } else { + Err(InvalidPatchError( + "The last line of the patch must be '*** End Patch'".to_string(), + )) + } + } } } } @@ -461,11 +493,14 @@ mod tests { contents: "hello\n".to_string(), }]) ); + assert_eq!(parser.environment_id(), Some("remote")); let mut parser = StreamingPatchParser::default(); assert_eq!( parser.push_delta("*** Begin Patch\n*** Environment ID: \n"), - Ok(vec![]) + Err(InvalidPatchError( + "apply_patch environment_id cannot be empty".to_string(), + )) ); } @@ -637,6 +672,26 @@ mod tests { ); } + #[test] + fn test_streaming_patch_parser_ignores_empty_lines_after_end_of_file() { + let mut parser = StreamingPatchParser::default(); + assert_eq!( + parser.push_delta( + "*** Begin Patch\n*** Update File: file.txt\n@@\n+quux\n*** End of File\n\n*** End Patch\n", + ), + Ok(vec![UpdateFile { + path: PathBuf::from("file.txt"), + move_path: None, + chunks: vec![UpdateFileChunk { + change_context: None, + old_lines: Vec::new(), + new_lines: vec!["quux".to_string()], + is_end_of_file: true, + }], + }]) + ); + } + #[test] fn test_streaming_patch_parser_matches_line_ending_behavior() { let mut parser = StreamingPatchParser::default(); @@ -737,6 +792,30 @@ mod tests { ); } + #[test] + fn test_streaming_patch_parser_rejects_content_after_end_patch() { + let mut parser = StreamingPatchParser::default(); + assert_eq!( + parser.push_delta( + "*** Begin Patch\n*** Add File: file.txt\n+hello\n*** End Patch\nextra\n", + ), + Err(InvalidPatchError( + "The last line of the patch must be '*** End Patch'".to_string(), + )) + ); + + let mut parser = StreamingPatchParser::default(); + assert_eq!( + parser.push_delta( + "*** Begin Patch\n*** Add File: file.txt\n+hello\n*** End Patch\n \t\n", + ), + Ok(vec![AddFile { + path: PathBuf::from("file.txt"), + contents: "hello\n".to_string(), + }]) + ); + } + #[test] fn test_streaming_patch_parser_returns_errors() { let mut parser = StreamingPatchParser::default(); From e263eedd3acbd3852ae139e88b82893dc8e6b46b Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 12 Jun 2026 10:12:58 -0700 Subject: [PATCH 2/4] centralize apply patch control lines --- codex-rs/apply-patch/src/streaming_parser.rs | 34 +++++++++++--------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index a6a23c5e2c95..2c06841ec921 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -81,7 +81,24 @@ impl StreamingPatchParser { Ok(()) } - fn handle_hunk_headers_and_end_patch(&mut self, trimmed: &str) -> Result { + fn handle_hunk_headers_and_end_patch(&mut self, line: &str) -> Result { + if self.line_number == 2 + && let Some(environment_id) = line.trim_start().strip_prefix(ENVIRONMENT_ID_MARKER) + { + let environment_id = environment_id.trim(); + if environment_id.is_empty() { + return Err(InvalidPatchError( + "apply_patch environment_id cannot be empty".to_string(), + )); + } + self.state.environment_id = Some(environment_id.to_string()); + return Ok(true); + } + let trimmed = if matches!(self.state.mode, StreamingParserMode::StartedPatch) { + line.trim() + } else { + line + }; if trimmed == END_PATCH_MARKER { self.ensure_update_hunk_is_not_empty(trimmed)?; self.state.mode = StreamingParserMode::EndedPatch; @@ -168,20 +185,7 @@ impl StreamingPatchParser { )) } StreamingParserMode::StartedPatch => { - if self.line_number == 2 - && let Some(environment_id) = - line.trim_start().strip_prefix(ENVIRONMENT_ID_MARKER) - { - let environment_id = environment_id.trim(); - if environment_id.is_empty() { - return Err(InvalidPatchError( - "apply_patch environment_id cannot be empty".to_string(), - )); - } - self.state.environment_id = Some(environment_id.to_string()); - return Ok(()); - } - if self.handle_hunk_headers_and_end_patch(trimmed)? { + if self.handle_hunk_headers_and_end_patch(line)? { return Ok(()); } Err(InvalidHunkError { From 312807de7287117c496988a4fac00e976c0bd3c9 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 12 Jun 2026 10:21:50 -0700 Subject: [PATCH 3/4] simplify environment marker parsing --- codex-rs/apply-patch/src/streaming_parser.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index 2c06841ec921..c8c1748f2d6f 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -16,7 +16,7 @@ use crate::parser::UpdateFileChunk; use Hunk::*; use ParseError::*; -const ENVIRONMENT_ID_MARKER: &str = "*** Environment ID: "; +const ENVIRONMENT_ID_MARKER: &str = "*** Environment ID:"; #[derive(Debug, Default, Clone)] pub struct StreamingPatchParser { @@ -81,9 +81,10 @@ impl StreamingPatchParser { Ok(()) } - fn handle_hunk_headers_and_end_patch(&mut self, line: &str) -> Result { - if self.line_number == 2 - && let Some(environment_id) = line.trim_start().strip_prefix(ENVIRONMENT_ID_MARKER) + fn handle_hunk_headers_and_end_patch(&mut self, trimmed: &str) -> Result { + if matches!(self.state.mode, StreamingParserMode::StartedPatch) + && self.state.environment_id.is_none() + && let Some(environment_id) = trimmed.strip_prefix(ENVIRONMENT_ID_MARKER) { let environment_id = environment_id.trim(); if environment_id.is_empty() { @@ -94,11 +95,6 @@ impl StreamingPatchParser { self.state.environment_id = Some(environment_id.to_string()); return Ok(true); } - let trimmed = if matches!(self.state.mode, StreamingParserMode::StartedPatch) { - line.trim() - } else { - line - }; if trimmed == END_PATCH_MARKER { self.ensure_update_hunk_is_not_empty(trimmed)?; self.state.mode = StreamingParserMode::EndedPatch; @@ -185,7 +181,7 @@ impl StreamingPatchParser { )) } StreamingParserMode::StartedPatch => { - if self.handle_hunk_headers_and_end_patch(line)? { + if self.handle_hunk_headers_and_end_patch(trimmed)? { return Ok(()); } Err(InvalidHunkError { From 4dbf491551b67756e6657fe0145b384ebef71ade Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 12 Jun 2026 10:24:56 -0700 Subject: [PATCH 4/4] reject duplicate patch environments --- codex-rs/apply-patch/src/streaming_parser.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index c8c1748f2d6f..7b447fce3c3c 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -83,9 +83,13 @@ impl StreamingPatchParser { fn handle_hunk_headers_and_end_patch(&mut self, trimmed: &str) -> Result { if matches!(self.state.mode, StreamingParserMode::StartedPatch) - && self.state.environment_id.is_none() && let Some(environment_id) = trimmed.strip_prefix(ENVIRONMENT_ID_MARKER) { + if self.state.environment_id.is_some() { + return Err(InvalidPatchError( + "apply_patch environment_id cannot be specified more than once".to_string(), + )); + } let environment_id = environment_id.trim(); if environment_id.is_empty() { return Err(InvalidPatchError( @@ -495,6 +499,16 @@ mod tests { ); assert_eq!(parser.environment_id(), Some("remote")); + let mut parser = StreamingPatchParser::default(); + assert_eq!( + parser.push_delta( + "*** Begin Patch\n*** Environment ID: first\n*** Environment ID: second\n", + ), + Err(InvalidPatchError( + "apply_patch environment_id cannot be specified more than once".to_string(), + )) + ); + let mut parser = StreamingPatchParser::default(); assert_eq!( parser.push_delta("*** Begin Patch\n*** Environment ID: \n"),