From 77347d268d96a528f8e1bc3bb3937fd65dd11d11 Mon Sep 17 00:00:00 2001 From: Fouad Matin <169186268+fouad-openai@users.noreply.github.com> Date: Wed, 14 May 2025 08:34:09 -0700 Subject: [PATCH] fix: gpt-4.1 apply_patch handling (#930) --- codex-cli/src/utils/agent/agent-loop.ts | 21 ++++++---- codex-cli/src/utils/agent/apply-patch.ts | 53 +++++++++++++++++++++++- codex-cli/src/utils/agent/exec.ts | 18 ++++++-- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/codex-cli/src/utils/agent/agent-loop.ts b/codex-cli/src/utils/agent/agent-loop.ts index 79d30484..6f04401d 100644 --- a/codex-cli/src/utils/agent/agent-loop.ts +++ b/codex-cli/src/utils/agent/agent-loop.ts @@ -29,6 +29,7 @@ import { setCurrentModel, setSessionId, } from "../session.js"; +import { applyPatchToolInstructions } from "./apply-patch.js"; import { handleExecCommand } from "./handle-exec-command.js"; import { HttpsProxyAgent } from "https-proxy-agent"; import { spawnSync } from "node:child_process"; @@ -702,13 +703,19 @@ export class AgentLoop { for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { try { let reasoning: Reasoning | undefined; - if (this.model.startsWith("o")) { - reasoning = { effort: this.config.reasoningEffort ?? "high" }; - if (this.model === "o3" || this.model === "o4-mini") { - reasoning.summary = "auto"; - } + let modelSpecificInstructions: string | undefined; + if (this.model.startsWith("o") || this.model.startsWith("codex")) { + reasoning = { effort: this.config.reasoningEffort ?? "medium" }; + reasoning.summary = "auto"; } - const mergedInstructions = [prefix, this.instructions] + if (this.model.startsWith("gpt-4.1")) { + modelSpecificInstructions = applyPatchToolInstructions; + } + const mergedInstructions = [ + prefix, + modelSpecificInstructions, + this.instructions, + ] .filter(Boolean) .join("\n"); @@ -1504,7 +1511,7 @@ if (spawnSync("rg", ["--version"], { stdio: "ignore" }).status === 0) { "- Always use rg instead of grep/ls -R because it is much faster and respects gitignore", ); } -const dynamicPrefix = dynamicLines.join("\n") + "\n\n"; +const dynamicPrefix = dynamicLines.join("\n"); const prefix = `You are operating as and within the Codex CLI, a terminal-based agentic coding assistant built by OpenAI. It wraps OpenAI models to enable natural language interaction with a local codebase. You are expected to be precise, safe, and helpful. You can: diff --git a/codex-cli/src/utils/agent/apply-patch.ts b/codex-cli/src/utils/agent/apply-patch.ts index 525404c3..0f66a8e0 100644 --- a/codex-cli/src/utils/agent/apply-patch.ts +++ b/codex-cli/src/utils/agent/apply-patch.ts @@ -550,7 +550,15 @@ export function text_to_patch( !(lines[0] ?? "").startsWith(PATCH_PREFIX.trim()) || lines[lines.length - 1] !== PATCH_SUFFIX.trim() ) { - throw new DiffError("Invalid patch text"); + let reason = "Invalid patch text: "; + if (lines.length < 2) { + reason += "Patch text must have at least two lines."; + } else if (!(lines[0] ?? "").startsWith(PATCH_PREFIX.trim())) { + reason += "Patch text must start with the correct patch prefix."; + } else if (lines[lines.length - 1] !== PATCH_SUFFIX.trim()) { + reason += "Patch text must end with the correct patch suffix."; + } + throw new DiffError(reason); } const parser = new Parser(orig, lines); parser.index = 1; @@ -762,3 +770,46 @@ if (import.meta.url === `file://${process.argv[1]}`) { } }); } + +export const applyPatchToolInstructions = ` +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-cli/src/utils/agent/exec.ts b/codex-cli/src/utils/agent/exec.ts index 58de9895..1f56dde1 100644 --- a/codex-cli/src/utils/agent/exec.ts +++ b/codex-cli/src/utils/agent/exec.ts @@ -9,11 +9,13 @@ import { execWithLandlock } from "./sandbox/landlock.js"; import { execWithSeatbelt } from "./sandbox/macos-seatbelt.js"; import { exec as rawExec } from "./sandbox/raw-exec.js"; import { formatCommandForDisplay } from "../../format-command.js"; +import { log } from "../logger/log.js"; import fs from "fs"; import os from "os"; import path from "path"; import { parse } from "shell-quote"; import { resolvePathAgainstWorkdir } from "src/approvals.js"; +import { PATCH_SUFFIX } from "src/parse-apply-patch.js"; const DEFAULT_TIMEOUT_MS = 10_000; // 10 seconds @@ -81,12 +83,22 @@ export function execApplyPatch( patchText: string, workdir: string | undefined = undefined, ): ExecResult { - // This is a temporary measure to understand what are the common base commands - // until we start persisting and uploading rollouts + // This find/replace is required from some models like 4.1 where the patch + // text is wrapped in quotes that breaks the apply_patch command. + let applyPatchInput = patchText + .replace(/('|")?<<('|")EOF('|")/, "") + .replace(/\*\*\* End Patch\nEOF('|")?/, "*** End Patch") + .trim(); + + if (!applyPatchInput.endsWith(PATCH_SUFFIX)) { + applyPatchInput += "\n" + PATCH_SUFFIX; + } + + log(`Applying patch: \`\`\`${applyPatchInput}\`\`\`\n\n`); try { const result = process_patch( - patchText, + applyPatchInput, (p) => fs.readFileSync(resolvePathAgainstWorkdir(p, workdir), "utf8"), (p, c) => { const resolvedPath = resolvePathAgainstWorkdir(p, workdir);