From 101eabf6af3fe4bb8f7d24bc12a2ccfbfe55a2aa Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 7 May 2026 18:28:31 -0700 Subject: [PATCH] codex: address apply_patch grammar review feedback Co-authored-by: Codex --- codex-rs/apply-patch/src/streaming_parser.rs | 59 +++++++++++++++---- .../core/src/tools/handlers/apply_patch.lark | 3 +- .../src/tools/handlers/apply_patch_spec.rs | 12 ++-- .../tools/handlers/apply_patch_spec_tests.rs | 16 +++-- 4 files changed, 66 insertions(+), 24 deletions(-) diff --git a/codex-rs/apply-patch/src/streaming_parser.rs b/codex-rs/apply-patch/src/streaming_parser.rs index 4a5b5328db..095b642d77 100644 --- a/codex-rs/apply-patch/src/streaming_parser.rs +++ b/codex-rs/apply-patch/src/streaming_parser.rs @@ -19,8 +19,6 @@ use ParseError::*; #[derive(Debug, Default, Clone)] pub struct StreamingPatchParser { - allow_environment_id: bool, - saw_environment_id: bool, line_buffer: String, state: StreamingParserState, line_number: usize, @@ -32,11 +30,15 @@ struct StreamingParserState { hunks: Vec, } -#[derive(Debug, Default, Clone)] +#[derive(Debug, Clone)] enum StreamingParserMode { - #[default] - NotStarted, - StartedPatch, + NotStarted { + allow_environment_id: bool, + }, + StartedPatch { + allow_environment_id: bool, + saw_environment_id: bool, + }, AddFile, DeleteFile, UpdateFile { @@ -45,9 +47,29 @@ enum StreamingParserMode { EndedPatch, } +impl Default for StreamingParserMode { + fn default() -> Self { + Self::NotStarted { + allow_environment_id: false, + } + } +} + impl StreamingPatchParser { pub fn set_allow_environment_id(&mut self, allow_environment_id: bool) { - self.allow_environment_id = allow_environment_id; + match &mut self.state.mode { + StreamingParserMode::NotStarted { + allow_environment_id: current, + } + | StreamingParserMode::StartedPatch { + allow_environment_id: current, + .. + } => *current = allow_environment_id, + StreamingParserMode::AddFile + | StreamingParserMode::DeleteFile + | StreamingParserMode::UpdateFile { .. } + | StreamingParserMode::EndedPatch => {} + } } fn ensure_update_hunk_is_not_empty(&self, line: &str) -> Result<(), ParseError> { @@ -158,18 +180,26 @@ impl StreamingPatchParser { fn process_line(&mut self, line: &str) -> Result<(), ParseError> { let trimmed = line.trim(); match self.state.mode.clone() { - StreamingParserMode::NotStarted => { + StreamingParserMode::NotStarted { + allow_environment_id, + } => { if trimmed == BEGIN_PATCH_MARKER { - self.state.mode = StreamingParserMode::StartedPatch; + self.state.mode = StreamingParserMode::StartedPatch { + allow_environment_id, + saw_environment_id: false, + }; return Ok(()); } Err(InvalidPatchError( "The first line of the patch must be '*** Begin Patch'".to_string(), )) } - StreamingParserMode::StartedPatch => { - if self.allow_environment_id - && !self.saw_environment_id + StreamingParserMode::StartedPatch { + allow_environment_id, + saw_environment_id, + } => { + if allow_environment_id + && !saw_environment_id && let Some(environment_id) = trimmed.strip_prefix(ENVIRONMENT_ID_MARKER) { if environment_id.trim().is_empty() { @@ -177,7 +207,10 @@ impl StreamingPatchParser { "Environment ID cannot be empty".to_string(), )); } - self.saw_environment_id = true; + self.state.mode = StreamingParserMode::StartedPatch { + allow_environment_id, + saw_environment_id: true, + }; return Ok(()); } if self.handle_hunk_headers_and_end_patch(trimmed)? { diff --git a/codex-rs/core/src/tools/handlers/apply_patch.lark b/codex-rs/core/src/tools/handlers/apply_patch.lark index c61987d02a..5aa41b0af7 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.lark +++ b/codex-rs/core/src/tools/handlers/apply_patch.lark @@ -1,6 +1,5 @@ -start: begin_patch environment_id? hunk+ end_patch +start: begin_patch hunk+ end_patch begin_patch: "*** Begin Patch" LF -environment_id: "*** Environment ID: " /(.+)/ LF end_patch: "*** End Patch" LF? hunk: add_hunk | delete_hunk | update_hunk diff --git a/codex-rs/core/src/tools/handlers/apply_patch_spec.rs b/codex-rs/core/src/tools/handlers/apply_patch_spec.rs index f2fe6b9239..bab96dc6f3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_spec.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_spec.rs @@ -10,6 +10,7 @@ use std::collections::BTreeMap; const APPLY_PATCH_LARK_GRAMMAR: &str = include_str!("apply_patch.lark"); const APPLY_PATCH_WITH_ENV_START: &str = "start: begin_patch environment_id? hunk+ end_patch"; const APPLY_PATCH_STRICT_START: &str = "start: begin_patch hunk+ end_patch"; +const APPLY_PATCH_BEGIN_PATCH_RULE: &str = "begin_patch: \"*** Begin Patch\" LF\n"; const APPLY_PATCH_ENVIRONMENT_RULE: &str = "environment_id: \"*** Environment ID: \" /(.+)/ LF\n"; const APPLY_PATCH_JSON_TOOL_DESCRIPTION: &str = r#"Use the `apply_patch` tool to edit files. @@ -109,11 +110,14 @@ pub fn create_apply_patch_freeform_tool(multi_environment: bool) -> ToolSpec { r#type: "grammar".to_string(), syntax: "lark".to_string(), definition: if multi_environment { - APPLY_PATCH_LARK_GRAMMAR.to_string() - } else { APPLY_PATCH_LARK_GRAMMAR - .replace(APPLY_PATCH_WITH_ENV_START, APPLY_PATCH_STRICT_START) - .replace(APPLY_PATCH_ENVIRONMENT_RULE, "") + .replace(APPLY_PATCH_STRICT_START, APPLY_PATCH_WITH_ENV_START) + .replace( + APPLY_PATCH_BEGIN_PATCH_RULE, + &format!("{APPLY_PATCH_BEGIN_PATCH_RULE}{APPLY_PATCH_ENVIRONMENT_RULE}"), + ) + } else { + APPLY_PATCH_LARK_GRAMMAR.to_string() }, }, }) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs index ee9d43ecad..73fbdf5907 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs @@ -3,6 +3,15 @@ use codex_tools::JsonSchema; use pretty_assertions::assert_eq; use std::collections::BTreeMap; +fn multi_environment_apply_patch_grammar() -> String { + APPLY_PATCH_LARK_GRAMMAR + .replace(APPLY_PATCH_STRICT_START, APPLY_PATCH_WITH_ENV_START) + .replace( + APPLY_PATCH_BEGIN_PATCH_RULE, + &format!("{APPLY_PATCH_BEGIN_PATCH_RULE}{APPLY_PATCH_ENVIRONMENT_RULE}"), + ) +} + #[test] fn create_apply_patch_freeform_tool_matches_expected_spec() { assert_eq!( @@ -13,7 +22,7 @@ fn create_apply_patch_freeform_tool_matches_expected_spec() { format: FreeformToolFormat { r#type: "grammar".to_string(), syntax: "lark".to_string(), - definition: APPLY_PATCH_LARK_GRAMMAR.to_string(), + definition: multi_environment_apply_patch_grammar(), }, }) ); @@ -59,10 +68,7 @@ fn strict_apply_patch_tools_do_not_advertise_multi_environment() { let ToolSpec::Freeform(freeform) = freeform else { panic!("expected freeform tool"); }; - let expected_strict = APPLY_PATCH_LARK_GRAMMAR - .replace(APPLY_PATCH_WITH_ENV_START, APPLY_PATCH_STRICT_START) - .replace(APPLY_PATCH_ENVIRONMENT_RULE, ""); - assert_eq!(freeform.format.definition, expected_strict); + assert_eq!(freeform.format.definition, APPLY_PATCH_LARK_GRAMMAR); assert_eq!(freeform.description, APPLY_PATCH_FREEFORM_TOOL_DESCRIPTION); let json_tool = create_apply_patch_json_tool(/*multi_environment*/ false);