From 2388a941f573cb0a04c94d9da950108eba099609 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 17 May 2025 10:53:01 -0700 Subject: [PATCH] fix: provide tolerance for apply_patch tool --- codex-rs/apply-patch/src/lib.rs | 26 ++++++++++++++++++- codex-rs/apply-patch/src/parser.rs | 29 ++++++++++++---------- codex-rs/core/src/client.rs | 40 ++++++++++++++++++++++++++---- codex-rs/core/src/client_common.rs | 2 +- 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index fcbc97b4f6..763968e330 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -8,11 +8,13 @@ use std::str::Utf8Error; use anyhow::Context; use anyhow::Result; +use parser::END_PATCH_MARKER; pub use parser::Hunk; pub use parser::ParseError; use parser::ParseError::*; use parser::UpdateFileChunk; pub use parser::parse_patch; +use regex::Regex; use similar::TextDiff; use thiserror::Error; use tree_sitter::LanguageError; @@ -62,7 +64,26 @@ pub enum MaybeApplyPatch { } pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { - match argv { + // Clean up heredoc quoting issues and ensure proper suffix for some model outputs. + let argv = { + if argv.len() == 3 && argv[0] == "bash" && argv[1] == "-lc" { + let mut script = argv[2].clone(); + // Remove quoted heredoc markers that can break parsing. + let re_start = Regex::new(r#"(['"])?<<(['"])?EOF(['"]?)"#).unwrap(); + let re_end = Regex::new(r#"\*\*\* End Patch\nEOF(['"])?"#).unwrap(); + script = re_start.replace_all(&script, "").to_string(); + script = re_end.replace_all(&script, "*** End Patch").to_string(); + script = script.trim().to_string(); + if !script.ends_with(END_PATCH_MARKER) { + script.push_str("\n"); + script.push_str(END_PATCH_MARKER); + } + vec![argv[0].clone(), argv[1].clone(), script] + } else { + argv.to_vec() + } + }; + match argv.as_slice() { [cmd, body] if cmd == "apply_patch" => match parse_patch(body) { Ok(hunks) => MaybeApplyPatch::Body(hunks), Err(e) => MaybeApplyPatch::PatchParseError(e), @@ -619,6 +640,9 @@ pub fn print_summary( Ok(()) } +/// Detailed instructions for gpt-4.1 on how to use the `apply_patch` tool. +pub const APPLY_PATCH_TOOL_INSTRUCTIONS: &str = include_str!("../apply_patch_tool_instructions.md"); + #[cfg(test)] mod tests { #![allow(clippy::unwrap_used)] diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 391255defa..8f99436ea1 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -28,7 +28,7 @@ use std::path::PathBuf; use thiserror::Error; const BEGIN_PATCH_MARKER: &str = "*** Begin Patch"; -const END_PATCH_MARKER: &str = "*** End Patch"; +pub(crate) const END_PATCH_MARKER: &str = "*** End Patch"; const ADD_FILE_MARKER: &str = "*** Add File: "; const DELETE_FILE_MARKER: &str = "*** Delete File: "; const UPDATE_FILE_MARKER: &str = "*** Update File: "; @@ -96,16 +96,19 @@ pub struct UpdateFileChunk { pub fn parse_patch(patch: &str) -> Result, ParseError> { let lines: Vec<&str> = patch.trim().lines().collect(); - if lines.is_empty() || lines[0] != BEGIN_PATCH_MARKER { - return Err(InvalidPatchError(String::from( - "The first line of the patch must be '*** Begin Patch'", - ))); - } - let last_line_index = lines.len() - 1; - if lines[last_line_index] != END_PATCH_MARKER { - return Err(InvalidPatchError(String::from( - "The last line of the patch must be '*** End Patch'", - ))); + let last_line_index = lines.len().saturating_sub(1); + if lines.len() < 2 + || lines[0] != BEGIN_PATCH_MARKER + || lines[last_line_index] != END_PATCH_MARKER + { + let reason = if lines.len() < 2 { + "Patch text must have at least two lines." + } else if lines[0] != BEGIN_PATCH_MARKER { + "Patch text must start with the correct patch prefix." + } else { + "Patch text must end with the correct patch suffix." + }; + return Err(InvalidPatchError(reason.to_string())); } let mut hunks: Vec = Vec::new(); let mut remaining_lines = &lines[1..last_line_index]; @@ -314,13 +317,13 @@ fn test_parse_patch() { assert_eq!( parse_patch("bad"), Err(InvalidPatchError( - "The first line of the patch must be '*** Begin Patch'".to_string() + "Patch text must have at least two lines.".to_string() )) ); assert_eq!( parse_patch("*** Begin Patch\nbad"), Err(InvalidPatchError( - "The last line of the patch must be '*** End Patch'".to_string() + "Patch text must end with the correct patch suffix.".to_string() )) ); assert_eq!( diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 57534e2f9a..f59c297c11 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -21,6 +21,7 @@ use tracing::warn; use crate::chat_completions::AggregateStreamExt; use crate::chat_completions::stream_chat_completions; +use crate::client_common::BASE_INSTRUCTIONS; use crate::client_common::Payload; use crate::client_common::Prompt; use crate::client_common::Reasoning; @@ -37,6 +38,8 @@ use crate::model_provider_info::ModelProviderInfo; use crate::model_provider_info::WireApi; use crate::models::ResponseItem; use crate::util::backoff; +use codex_apply_patch::APPLY_PATCH_TOOL_INSTRUCTIONS; +use std::borrow::Cow; /// When serialized as JSON, this produces a valid "Tool" in the OpenAI /// Responses API. @@ -181,7 +184,37 @@ impl ModelClient { debug!("tools_json: {}", serde_json::to_string_pretty(&tools_json)?); - let full_instructions = prompt.get_full_instructions(); + // Model-specific instructions and reasoning adjustments. + let mut model_specific_instructions: Option<&str> = None; + let mut reasoning: Option = None; + if self.model.starts_with("o") || self.model.starts_with("codex") { + reasoning = Some(Reasoning { + effort: "medium", + summary: Some(Summary::Auto), + }); + } + if self.model.starts_with("gpt-4.1") { + model_specific_instructions = Some(APPLY_PATCH_TOOL_INSTRUCTIONS); + } + let full_instructions = { + match &prompt.instructions { + Some(user_instructions) => { + let mut parts = vec![BASE_INSTRUCTIONS]; + if let Some(msi) = model_specific_instructions { + parts.push(msi); + } + parts.push(user_instructions); + Cow::Owned(parts.join("\n")) + } + None => { + if let Some(msi) = model_specific_instructions { + Cow::Owned(vec![BASE_INSTRUCTIONS, msi].join("\n")) + } else { + Cow::Borrowed(BASE_INSTRUCTIONS) + } + } + } + }; let payload = Payload { model: &self.model, instructions: &full_instructions, @@ -189,10 +222,7 @@ impl ModelClient { tools: &tools_json, tool_choice: "auto", parallel_tool_calls: false, - reasoning: Some(Reasoning { - effort: "high", - summary: Some(Summary::Auto), - }), + reasoning, previous_response_id: prompt.prev_id.clone(), store: prompt.store, stream: true, diff --git a/codex-rs/core/src/client_common.rs b/codex-rs/core/src/client_common.rs index 8eb8074b1e..4900a6638f 100644 --- a/codex-rs/core/src/client_common.rs +++ b/codex-rs/core/src/client_common.rs @@ -11,7 +11,7 @@ use tokio::sync::mpsc; /// The `instructions` field in the payload sent to a model should always start /// with this content. -const BASE_INSTRUCTIONS: &str = include_str!("../prompt.md"); +pub(crate) const BASE_INSTRUCTIONS: &str = include_str!("../prompt.md"); /// API request payload for a single model turn. #[derive(Default, Debug, Clone)]