Match streaming apply_patch parser errors

This commit is contained in:
Akshay Nathan
2026-04-30 12:42:56 -07:00
parent 6bd78a51b5
commit 903c56aa87
4 changed files with 420 additions and 187 deletions

1
codex-rs/Cargo.lock generated
View File

@@ -1993,6 +1993,7 @@ dependencies = [
"codex-utils-absolute-path",
"codex-utils-cargo-bin",
"pretty_assertions",
"serde_json",
"similar",
"tempfile",
"thiserror 2.0.18",

View File

@@ -30,4 +30,5 @@ assert_cmd = { workspace = true }
assert_matches = { workspace = true }
codex-utils-cargo-bin = { workspace = true }
pretty_assertions = { workspace = true }
serde_json = { workspace = true }
tempfile = { workspace = true }

View File

@@ -0,0 +1,124 @@
use std::fs::File;
use std::io::BufRead;
use std::io::BufReader;
use codex_apply_patch::Hunk;
use codex_apply_patch::StreamingPatchParser;
use codex_apply_patch::parse_patch;
use serde_json::Value;
#[derive(Debug)]
enum ParseOutcome {
Ok(Vec<Hunk>),
Err(String),
}
fn normal_parse(patch: &str) -> ParseOutcome {
parse_patch(patch)
.map(|args| ParseOutcome::Ok(args.hunks))
.unwrap_or_else(|err| ParseOutcome::Err(err.to_string()))
}
fn streaming_parse(patch: &str) -> ParseOutcome {
let mut parser = StreamingPatchParser::default();
parser
.push_delta(patch)
.and_then(|_| parser.finish())
.map(ParseOutcome::Ok)
.unwrap_or_else(|err| ParseOutcome::Err(err.to_string()))
}
fn mismatch_kind(normal: &ParseOutcome, streaming: &ParseOutcome) -> Option<&'static str> {
match (normal, streaming) {
(ParseOutcome::Ok(normal), ParseOutcome::Ok(streaming)) if normal == streaming => None,
(ParseOutcome::Err(normal), ParseOutcome::Err(streaming)) if normal == streaming => None,
(ParseOutcome::Ok(_), ParseOutcome::Err(_)) => Some("Old OK, new error"),
(ParseOutcome::Err(_), ParseOutcome::Ok(_)) => Some("Old error, new OK"),
(ParseOutcome::Ok(_), ParseOutcome::Ok(_)) => Some("Both OK, different parse"),
(ParseOutcome::Err(_), ParseOutcome::Err(_)) => Some("Both error, different error"),
}
}
fn decode_patch(raw: &str) -> String {
if raw.contains("\\n") {
let wrapped = format!("\"{raw}\"");
if let Ok(decoded) = serde_json::from_str::<String>(&wrapped) {
return decoded;
}
}
if raw.starts_with('"')
&& let Ok(decoded) = serde_json::from_str::<String>(raw)
{
return decoded;
}
raw.to_string()
}
fn summarize_patch(patch: &str) -> String {
let mut out = String::new();
for (i, line) in patch.lines().take(80).enumerate() {
out.push_str(&format!("{:>4}: {line}\n", i + 1));
}
if patch.lines().count() > 80 {
out.push_str("... truncated after 80 lines ...\n");
}
out
}
fn main() {
let mut args = std::env::args().skip(1);
let path = args
.next()
.expect("usage: compare_streaming_parser <jsonl> [start_line]");
let start_line = args
.next()
.map(|line| line.parse::<usize>().expect("start_line must be a number"))
.unwrap_or(1);
let file = File::open(&path).expect("failed to open jsonl");
let reader = BufReader::new(file);
let mut total = 0usize;
let mut both_error = 0usize;
for line in reader.lines() {
total += 1;
if total < start_line {
continue;
}
let line = line.expect("failed to read line");
let value: Value = serde_json::from_str(&line).expect("invalid jsonl row");
let request_id = value
.get("request_id")
.and_then(Value::as_str)
.unwrap_or("<missing request_id>");
let raw = value
.get("patch_payload_escaped")
.and_then(Value::as_str)
.unwrap_or("");
let patch = decode_patch(raw);
let normal = normal_parse(&patch);
let streaming = streaming_parse(&patch);
if let Some(kind) = mismatch_kind(&normal, &streaming) {
println!("first mismatch at row {total}");
println!("result: {kind}");
println!("request_id: {request_id}");
println!("raw payload bytes: {}", raw.len());
println!("decoded patch bytes: {}", patch.len());
println!("\nnormal parser:\n{normal:#?}");
println!("\nstreaming parser:\n{streaming:#?}");
println!("\npatch preview:\n{}", summarize_patch(&patch));
return;
}
if matches!(normal, ParseOutcome::Err(_)) {
both_error += 1;
}
if total % 10_000 == 0 {
eprintln!("checked {total} rows ({both_error} rows where both parsers errored)");
}
}
println!("checked all {total} rows; no parser result mismatches");
if both_error > 0 {
println!("{both_error} rows produced an error in both parsers");
}
}

View File

@@ -36,66 +36,89 @@ enum StreamingParserMode {
StartedPatch,
AddFile,
DeleteFile,
UpdateFile,
UpdateFile {
hunk_line_number: usize,
},
EndedPatch,
}
fn handle_hunk_headers_and_end_patch(
trimmed: &str,
hunks: &mut Vec<Hunk>,
) -> Result<Option<StreamingParserMode>, String> {
if trimmed == END_PATCH_MARKER {
ensure_update_hunk_is_not_empty(hunks)?;
return Ok(Some(StreamingParserMode::EndedPatch));
}
if let Some(path) = trimmed.strip_prefix(ADD_FILE_MARKER) {
ensure_update_hunk_is_not_empty(hunks)?;
hunks.push(AddFile {
path: PathBuf::from(path),
contents: String::new(),
});
return Ok(Some(StreamingParserMode::AddFile));
}
if let Some(path) = trimmed.strip_prefix(DELETE_FILE_MARKER) {
ensure_update_hunk_is_not_empty(hunks)?;
hunks.push(DeleteFile {
path: PathBuf::from(path),
});
return Ok(Some(StreamingParserMode::DeleteFile));
}
if let Some(path) = trimmed.strip_prefix(UPDATE_FILE_MARKER) {
ensure_update_hunk_is_not_empty(hunks)?;
hunks.push(UpdateFile {
path: PathBuf::from(path),
move_path: None,
chunks: Vec::new(),
});
return Ok(Some(StreamingParserMode::UpdateFile));
}
Ok(None)
}
fn ensure_update_hunk_is_not_empty(hunks: &[Hunk]) -> Result<(), String> {
if let Some(UpdateFile { chunks, .. }) = hunks.last()
&& (chunks.is_empty()
|| chunks
.last()
.is_some_and(|chunk| chunk.old_lines.is_empty() && chunk.new_lines.is_empty()))
{
return Err("Update hunk does not contain any lines".to_string());
}
Ok(())
}
impl StreamingPatchParser {
fn ensure_update_hunk_is_not_empty(&self, line: &str) -> Result<(), ParseError> {
if let Some(UpdateFile { path, chunks, .. }) = self.state.hunks.last() {
if chunks.is_empty()
&& let StreamingParserMode::UpdateFile { hunk_line_number } = self.state.mode
{
return Err(InvalidHunkError {
message: format!("Update file hunk for path '{}' is empty", path.display()),
line_number: hunk_line_number,
});
}
if chunks
.last()
.is_some_and(|chunk| chunk.old_lines.is_empty() && chunk.new_lines.is_empty())
{
if line == END_PATCH_MARKER {
return Err(InvalidHunkError {
message: "Update hunk does not contain any lines".to_string(),
line_number: self.line_number,
});
}
return Err(InvalidHunkError {
message: format!(
"Unexpected line found in update hunk: '{line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)"
),
line_number: self.line_number,
});
}
}
Ok(())
}
fn handle_hunk_headers_and_end_patch(&mut self, trimmed: &str) -> Result<bool, ParseError> {
if trimmed == END_PATCH_MARKER {
self.ensure_update_hunk_is_not_empty(trimmed)?;
self.state.mode = StreamingParserMode::EndedPatch;
return Ok(true);
}
if let Some(path) = trimmed.strip_prefix(ADD_FILE_MARKER) {
self.ensure_update_hunk_is_not_empty(trimmed)?;
self.state.hunks.push(AddFile {
path: PathBuf::from(path),
contents: String::new(),
});
self.state.mode = StreamingParserMode::AddFile;
return Ok(true);
}
if let Some(path) = trimmed.strip_prefix(DELETE_FILE_MARKER) {
self.ensure_update_hunk_is_not_empty(trimmed)?;
self.state.hunks.push(DeleteFile {
path: PathBuf::from(path),
});
self.state.mode = StreamingParserMode::DeleteFile;
return Ok(true);
}
if let Some(path) = trimmed.strip_prefix(UPDATE_FILE_MARKER) {
self.ensure_update_hunk_is_not_empty(trimmed)?;
self.state.hunks.push(UpdateFile {
path: PathBuf::from(path),
move_path: None,
chunks: Vec::new(),
});
self.state.mode = StreamingParserMode::UpdateFile {
hunk_line_number: self.line_number,
};
return Ok(true);
}
Ok(false)
}
pub fn push_delta(&mut self, delta: &str) -> Result<Vec<Hunk>, ParseError> {
for ch in delta.chars() {
if ch == '\n' {
let line = std::mem::take(&mut self.line_buffer);
let state = std::mem::take(&mut self.state);
let mut line = std::mem::take(&mut self.line_buffer);
line.truncate(line.strip_suffix('\r').map_or(line.len(), str::len));
self.line_number += 1;
self.state =
Self::process_line(state, line.trim_end_matches('\r'), self.line_number)?;
self.process_line(&line)?;
} else {
self.line_buffer.push(ch);
}
@@ -106,7 +129,14 @@ impl StreamingPatchParser {
pub fn finish(&mut self) -> Result<Vec<Hunk>, ParseError> {
if !self.line_buffer.is_empty() {
self.push_delta("\n")?;
let line = std::mem::take(&mut self.line_buffer);
self.line_number += 1;
if line.trim() == END_PATCH_MARKER {
self.ensure_update_hunk_is_not_empty(line.trim())?;
self.state.mode = StreamingParserMode::EndedPatch;
} else {
self.process_line(&line)?;
}
}
if !matches!(self.state.mode, StreamingParserMode::EndedPatch) {
@@ -118,155 +148,127 @@ impl StreamingPatchParser {
Ok(self.state.hunks.clone())
}
fn process_line(
state: StreamingParserState,
line: &str,
line_number: usize,
) -> Result<StreamingParserState, ParseError> {
fn process_line(&mut self, line: &str) -> Result<(), ParseError> {
let trimmed = line.trim();
let StreamingParserState {
mut mode,
mut hunks,
} = state;
mode = match mode {
match self.state.mode.clone() {
StreamingParserMode::NotStarted => {
if trimmed == BEGIN_PATCH_MARKER {
return Ok(StreamingParserState {
mode: StreamingParserMode::StartedPatch,
hunks,
});
self.state.mode = StreamingParserMode::StartedPatch;
return Ok(());
}
return Err(InvalidPatchError(
Err(InvalidPatchError(
"The first line of the patch must be '*** Begin Patch'".to_string(),
));
))
}
StreamingParserMode::StartedPatch => {
if let Some(mode) =
handle_hunk_headers_and_end_patch(trimmed, &mut hunks).map_err(|message| {
InvalidHunkError {
message,
line_number,
}
})?
{
return Ok(StreamingParserState { mode, hunks });
if self.handle_hunk_headers_and_end_patch(trimmed)? {
return Ok(());
}
return Err(InvalidHunkError {
Err(InvalidHunkError {
message: format!(
"'{trimmed}' is not a valid hunk header. Valid hunk headers: '*** Add File: {{path}}', '*** Delete File: {{path}}', '*** Update File: {{path}}'"
),
line_number,
});
line_number: self.line_number,
})
}
StreamingParserMode::AddFile => {
if let Some(mode) =
handle_hunk_headers_and_end_patch(trimmed, &mut hunks).map_err(|message| {
InvalidHunkError {
message,
line_number,
}
})?
{
return Ok(StreamingParserState { mode, hunks });
if self.handle_hunk_headers_and_end_patch(trimmed)? {
return Ok(());
}
if let Some(line_to_add) = line.strip_prefix('+')
&& let Some(AddFile { contents, .. }) = hunks.last_mut()
&& let Some(AddFile { contents, .. }) = self.state.hunks.last_mut()
{
contents.push_str(line_to_add);
contents.push('\n');
return Ok(StreamingParserState {
mode: StreamingParserMode::AddFile,
hunks,
});
return Ok(());
}
return Err(InvalidHunkError {
message: format!(
"Unexpected line found in add file hunk: '{line}'. Every line should start with '+'"
),
line_number,
});
}
StreamingParserMode::DeleteFile => {
if let Some(mode) =
handle_hunk_headers_and_end_patch(trimmed, &mut hunks).map_err(|message| {
InvalidHunkError {
message,
line_number,
}
})?
{
return Ok(StreamingParserState { mode, hunks });
}
return Err(InvalidHunkError {
Err(InvalidHunkError {
message: format!(
"'{trimmed}' is not a valid hunk header. Valid hunk headers: '*** Add File: {{path}}', '*** Delete File: {{path}}', '*** Update File: {{path}}'"
),
line_number,
});
line_number: self.line_number,
})
}
StreamingParserMode::UpdateFile => {
StreamingParserMode::DeleteFile => {
if self.handle_hunk_headers_and_end_patch(trimmed)? {
return Ok(());
}
Err(InvalidHunkError {
message: format!(
"'{trimmed}' is not a valid hunk header. Valid hunk headers: '*** Add File: {{path}}', '*** Delete File: {{path}}', '*** Update File: {{path}}'"
),
line_number: self.line_number,
})
}
StreamingParserMode::UpdateFile { hunk_line_number } => {
let update_line = line.trim_end();
if let Some(mode) = handle_hunk_headers_and_end_patch(update_line, &mut hunks)
.map_err(|message| InvalidHunkError {
message,
line_number,
})?
{
return Ok(StreamingParserState { mode, hunks });
if self.handle_hunk_headers_and_end_patch(update_line)? {
return Ok(());
}
if let Some(UpdateFile {
move_path, chunks, ..
}) = hunks.last_mut()
}) = self.state.hunks.last_mut()
{
if chunks.is_empty()
&& move_path.is_none()
&& let Some(move_to_path) = update_line.strip_prefix(MOVE_TO_MARKER)
{
*move_path = Some(PathBuf::from(move_to_path));
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if (update_line == EMPTY_CHANGE_CONTEXT_MARKER
|| update_line.starts_with(CHANGE_CONTEXT_MARKER))
&& chunks.last().is_some_and(|chunk| {
chunk.old_lines.is_empty() && chunk.new_lines.is_empty()
})
{
return Err(InvalidHunkError {
message: format!(
"Unexpected line found in update hunk: '{line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)"
),
line_number: self.line_number,
});
}
match update_line {
EMPTY_CHANGE_CONTEXT_MARKER => {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
}
line => {
if let Some(change_context) = line.strip_prefix(CHANGE_CONTEXT_MARKER) {
chunks.push(UpdateFileChunk {
change_context: Some(change_context.to_string()),
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
}
}
if update_line == EMPTY_CHANGE_CONTEXT_MARKER {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if let Some(change_context) = update_line.strip_prefix(CHANGE_CONTEXT_MARKER) {
chunks.push(UpdateFileChunk {
change_context: Some(change_context.to_string()),
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if update_line == EOF_MARKER {
if chunks.last().is_some_and(|chunk| {
chunk.old_lines.is_empty() && chunk.new_lines.is_empty()
}) {
return Err(InvalidHunkError {
message: "Update hunk does not contain any lines".to_string(),
line_number: self.line_number,
});
}
if let Some(chunk) = chunks.last_mut() {
chunk.is_end_of_file = true;
}
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if line.is_empty() {
@@ -282,10 +284,8 @@ impl StreamingPatchParser {
chunk.old_lines.push(String::new());
chunk.new_lines.push(String::new());
}
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if let Some(line_to_add) = line.strip_prefix(' ') {
@@ -301,10 +301,8 @@ impl StreamingPatchParser {
chunk.old_lines.push(line_to_add.to_string());
chunk.new_lines.push(line_to_add.to_string());
}
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if let Some(line_to_add) = line.strip_prefix('+') {
@@ -319,10 +317,8 @@ impl StreamingPatchParser {
if let Some(chunk) = chunks.last_mut() {
chunk.new_lines.push(line_to_add.to_string());
}
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if let Some(line_to_remove) = line.strip_prefix('-') {
@@ -337,22 +333,30 @@ impl StreamingPatchParser {
if let Some(chunk) = chunks.last_mut() {
chunk.old_lines.push(line_to_remove.to_string());
}
return Ok(StreamingParserState {
mode: StreamingParserMode::UpdateFile,
hunks,
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}
if chunks.last().is_some_and(|chunk| {
!chunk.old_lines.is_empty() || !chunk.new_lines.is_empty()
}) {
return Err(InvalidHunkError {
message: format!(
"Expected update hunk to start with a @@ context marker, got: '{line}'"
),
line_number: self.line_number,
});
}
}
return Err(InvalidHunkError {
Err(InvalidHunkError {
message: format!(
"Unexpected line found in update hunk: '{line}'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)"
),
line_number,
});
line_number: self.line_number,
})
}
StreamingParserMode::EndedPatch => mode,
};
Ok(StreamingParserState { mode, hunks })
StreamingParserMode::EndedPatch => Ok(()),
}
}
}
@@ -595,6 +599,39 @@ mod tests {
);
}
#[test]
fn test_streaming_patch_parser_matches_line_ending_behavior() {
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta("*** Begin Patch\r\n*** Update File: file.txt\r\n@@\r\n-old\r\n+new\r\n*** End Patch\r\n"),
Ok(vec![UpdateFile {
path: PathBuf::from("file.txt"),
move_path: None,
chunks: vec![UpdateFileChunk {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
is_end_of_file: false,
}],
}])
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta("*** Begin Patch\r\n*** Update File: file.txt\r\n@@\r\n-old\r\r\n+new\r\n*** End Patch\r\n"),
Ok(vec![UpdateFile {
path: PathBuf::from("file.txt"),
move_path: None,
chunks: vec![UpdateFileChunk {
change_context: None,
old_lines: vec!["old\r".to_string()],
new_lines: vec!["new".to_string()],
is_end_of_file: false,
}],
}])
);
}
#[test]
fn test_streaming_patch_parser_finish_processes_final_line_without_newline() {
let mut parser = StreamingPatchParser::default();
@@ -612,6 +649,36 @@ mod tests {
contents: "hello\n".to_string(),
}])
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta(
"*** Begin Patch\n*** Update File: file.txt\n@@\n-old\n+new\n *** End Patch",
),
Ok(vec![UpdateFile {
path: PathBuf::from("file.txt"),
move_path: None,
chunks: vec![UpdateFileChunk {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
is_end_of_file: false,
}],
}])
);
assert_eq!(
parser.finish(),
Ok(vec![UpdateFile {
path: PathBuf::from("file.txt"),
move_path: None,
chunks: vec![UpdateFileChunk {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
is_end_of_file: false,
}],
}])
);
}
#[test]
@@ -657,9 +724,8 @@ mod tests {
assert_eq!(
parser.push_delta("*** Begin Patch\n*** Add File: file.txt\nbad\n"),
Err(InvalidHunkError {
message:
"Unexpected line found in add file hunk: 'bad'. Every line should start with '+'"
.to_string(),
message: "'bad' is not a valid hunk header. Valid hunk headers: '*** Add File: {path}', '*** Delete File: {path}', '*** Update File: {path}'"
.to_string(),
line_number: 3,
})
);
@@ -678,8 +744,8 @@ mod tests {
assert_eq!(
parser.push_delta("*** Begin Patch\n*** Update File: file.txt\n*** End Patch\n"),
Err(InvalidHunkError {
message: "Update hunk does not contain any lines".to_string(),
line_number: 3,
message: "Update file hunk for path 'file.txt' is empty".to_string(),
line_number: 2,
})
);
@@ -689,8 +755,8 @@ mod tests {
"*** Begin Patch\n*** Update File: old.txt\n*** Move to: new.txt\n*** Delete File: other.txt\n",
),
Err(InvalidHunkError {
message: "Update hunk does not contain any lines".to_string(),
line_number: 4,
message: "Update file hunk for path 'old.txt' is empty".to_string(),
line_number: 2,
})
);
@@ -702,5 +768,46 @@ mod tests {
line_number: 4,
})
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta("*** Begin Patch\n*** Update File: file.txt\n@@\n*** End of File\n"),
Err(InvalidHunkError {
message: "Update hunk does not contain any lines".to_string(),
line_number: 4,
})
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta("*** Begin Patch\n*** Update File: file.txt\n@@\n@@\n"),
Err(InvalidHunkError {
message: "Unexpected line found in update hunk: '@@'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)"
.to_string(),
line_number: 4,
})
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta("*** Begin Patch\n*** Update File: file.txt\n@@\n-old\nbad\n"),
Err(InvalidHunkError {
message: "Expected update hunk to start with a @@ context marker, got: 'bad'"
.to_string(),
line_number: 5,
})
);
let mut parser = StreamingPatchParser::default();
assert_eq!(
parser.push_delta(
"*** Begin Patch\n*** Update File: file.txt\n@@\n*** Update File: other.txt\n",
),
Err(InvalidHunkError {
message: "Unexpected line found in update hunk: '*** Update File: other.txt'. Every line should start with ' ' (context line), '+' (added line), or '-' (removed line)"
.to_string(),
line_number: 4,
})
);
}
}