diff --git a/codex-rs/apply-patch/apply_patch_tool_instructions.md b/codex-rs/apply-patch/apply_patch_tool_instructions.md new file mode 100644 index 0000000000..3c51d9cfbf --- /dev/null +++ b/codex-rs/apply-patch/apply_patch_tool_instructions.md @@ -0,0 +1,40 @@ +To edit files, ALWAYS use the `shell` tool with `apply_patch` CLI. `apply_patch` effectively allows you to execute a diff/patch against a file, but the format of the diff specification is unique to this task, so pay careful attention to these instructions. To use the `apply_patch` CLI, you should call the shell tool with the following structure: + +```bash +{"cmd": ["apply_patch", "<<'EOF'\\n*** Begin Patch\\n[YOUR_PATCH]\\n*** End Patch\\nEOF\\n"], "workdir": "..."} +``` + +Where [YOUR_PATCH] is the actual content of your patch, specified in the following V4A diff format. + +*** [ACTION] File: [path/to/file] -> ACTION can be one of Add, Update, or Delete. +For each snippet of code that needs to be changed, repeat the following: +[context_before] -> See below for further instructions on context. +- [old_code] -> Precede the old code with a minus sign. ++ [new_code] -> Precede the new, replacement code with a plus sign. +[context_after] -> See below for further instructions on context. + +For instructions on [context_before] and [context_after]: +- By default, show 3 lines of code immediately above and 3 lines immediately below each change. If a change is within 3 lines of a previous change, do NOT duplicate the first change’s [context_after] lines in the second change’s [context_before] lines. +- If 3 lines of context is insufficient to uniquely identify the snippet of code within the file, use the @@ operator to indicate the class or function to which the snippet belongs. For instance, we might have: +@@ class BaseClass +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] + +- If a code block is repeated so many times in a class or function such that even a single `@@` statement and 3 lines of context cannot uniquely identify the snippet of code, you can use multiple `@@` statements to jump to the right context. For instance: + +@@ class BaseClass +@@ def method(): +[3 lines of pre-context] +- [old_code] ++ [new_code] +[3 lines of post-context] + +Note, then, that we do not use line numbers in this diff format, as the context is enough to uniquely identify code. An example of a message that you might pass as "input" to this function, in order to apply a patch, is shown below. + +```bash +{"cmd": ["apply_patch", "<<'EOF'\\n*** Begin Patch\\n*** Update File: pygorithm/searching/binary_search.py\\n@@ class BaseClass\\n@@ def search():\\n- pass\\n+ raise NotImplementedError()\\n@@ class Subclass\\n@@ def search():\\n- pass\\n+ raise NotImplementedError()\\n*** End Patch\\nEOF\\n"], "workdir": "..."} +``` + +File references can only be relative, NEVER ABSOLUTE. After the apply_patch command is run, it will always say "Done!", regardless of whether the patch was successfully applied or not. However, you can determine if there are issue and errors by looking at any warnings or logging lines printed BEFORE the "Done!" is output. diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index fcbc97b4f6..ff9840abc7 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; @@ -61,8 +63,29 @@ pub enum MaybeApplyPatch { NotApplyPatch, } +#[allow(clippy::unwrap_used)] pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { - match argv { + // Clean up heredoc quoting issues and ensure proper suffix for some model outputs. + #[allow(clippy::unwrap_used)] + 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('\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 +642,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)] @@ -689,6 +715,7 @@ PATCH"#, } } + #[test] fn test_add_file_hunk_creates_file_with_contents() { let dir = tempdir().unwrap(); diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 391255defa..a8764b09a5 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,19 @@ 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("*** Something else\n*** End Patch"), + Err(InvalidPatchError( + "Patch text must start with the correct patch prefix.".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..ba320f022d 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([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)]