agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] unify apply patch parsing (#27913)

Browse Source 
## Why

`apply_patch` maintained separate batch and streaming parsers for the
same patch grammar. That duplicated the parsing rules and allowed final
execution to disagree with the live streamed preview.

## What changed

- Make `StreamingPatchParser` the single owner of hunk and environment
ID parsing.
- Keep heredoc and outer patch-boundary normalization in the existing
`parse_patch` wrapper, preserving its public API.
- Reject non-whitespace content after `*** End Patch` and preserve
separator handling after `*** End of File`.
- Reject duplicate environment ID preambles explicitly.
- Remove the duplicate batch hunk parser and its implementation-specific
tests.

The change removes 201 net lines while retaining focused coverage for
the unified parser's boundary behavior.

## Validation

- `just test -p codex-apply-patch`
- Compared a 24-hour corpus of 2,788,059 observed `apply_patch` payloads
against the previous batch parser. All 2,779,502 accepted payloads
produced identical hunks, canonical patch text, and environment IDs; the
remaining 8,557 payloads were rejected by both parsers, with zero
acceptance or payload mismatches.
This commit is contained in:
pakrym-oai
2026-06-12 11:54:28 -07:00
committed by GitHub
Unverified
parent 9600afb1a1
commit e26f734f91
2 changed files with 138 additions and 339 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/apply-patch/src/parser.rs
+34 -328
View File
@@ -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<ApplyPatchArgs, ParseError> {
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<Hunk> = 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<ApplyPatchArgs, Pars
})
}
fn parse_environment_id_preamble<'a>(
hunk_lines: &'a [&'a str],
) -> Result<(Option<String>, &'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 `<<EOF`
@@ -239,7 +215,7 @@ fn check_patch_boundaries_strict<'a>(
/// 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();
codex-rs/apply-patch/src/streaming_parser.rs
+104 -11
View File
@@ -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 {
@@ -29,6 +29,7 @@ pub struct StreamingPatchParser {
struct StreamingParserState {
mode: StreamingParserMode,
hunks: Vec<Hunk>,
environment_id: Option<String>,
}
#[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> {
@@ -84,6 +82,23 @@ impl StreamingPatchParser {
}
fn handle_hunk_headers_and_end_patch(&mut self, trimmed: &str) -> Result<bool, ParseError> {
if matches!(self.state.mode, StreamingParserMode::StartedPatch)
&& 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(
"apply_patch environment_id cannot be empty".to_string(),
));
}
self.state.environment_id = Some(environment_id.to_string());
return Ok(true);
}
if trimmed == END_PATCH_MARKER {
self.ensure_update_hunk_is_not_empty(trimmed)?;
self.state.mode = StreamingParserMode::EndedPatch;
@@ -170,9 +185,6 @@ impl StreamingPatchParser {
))
}
StreamingParserMode::StartedPatch => {
if self.is_environment_id_preamble_line(line) {
return Ok(());
}
if self.handle_hunk_headers_and_end_patch(trimmed)? {
return Ok(());
}
@@ -222,6 +234,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 +395,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 +497,24 @@ 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: 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"),
Ok(vec![])
Err(InvalidPatchError(
"apply_patch environment_id cannot be empty".to_string(),
))
);
}
@@ -637,6 +686,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 +806,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();