Merge 2388a941f5 into sapling-pr-archive-bolinfest

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)]

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<Vec<Hunk>, 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<Hunk> = 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!(

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<Reasoning> = 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,

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)]