From 070499f534dc31de19284790444bdbec588f98c4 Mon Sep 17 00:00:00 2001 From: Fouad Matin <169186268+fouad-openai@users.noreply.github.com> Date: Fri, 16 May 2025 08:04:00 -0700 Subject: [PATCH 1/6] add: codex-mini-latest (#951) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 💽 --------- Co-authored-by: Trevor Creech --- codex-cli/src/cli.tsx | 2 +- .../chat/terminal-chat-response-item.tsx | 31 +++-- codex-cli/src/utils/agent/agent-loop.ts | 108 +++++++++++++++++- codex-cli/src/utils/config.ts | 2 +- codex-cli/src/utils/model-info.ts | 4 + codex-cli/tests/config.test.tsx | 2 +- .../disableResponseStorage.agentLoop.test.ts | 2 +- .../tests/disableResponseStorage.test.ts | 5 +- .../tests/model-utils-network-error.test.ts | 5 +- 9 files changed, 141 insertions(+), 20 deletions(-) diff --git a/codex-cli/src/cli.tsx b/codex-cli/src/cli.tsx index d53ed07f74..c009bb8ab3 100644 --- a/codex-cli/src/cli.tsx +++ b/codex-cli/src/cli.tsx @@ -56,7 +56,7 @@ const cli = meow( --version Print version and exit -h, --help Show usage and exit - -m, --model Model to use for completions (default: o4-mini) + -m, --model Model to use for completions (default: codex-mini-latest) -p, --provider Provider to use for completions (default: openai) -i, --image Path(s) to image files to include as input -v, --view Inspect a previously saved rollout instead of starting a session diff --git a/codex-cli/src/components/chat/terminal-chat-response-item.tsx b/codex-cli/src/components/chat/terminal-chat-response-item.tsx index bda4fea9db..bab4aa317f 100644 --- a/codex-cli/src/components/chat/terminal-chat-response-item.tsx +++ b/codex-cli/src/components/chat/terminal-chat-response-item.tsx @@ -19,6 +19,7 @@ import { parse, setOptions } from "marked"; import TerminalRenderer from "marked-terminal"; import path from "path"; import React, { useEffect, useMemo } from "react"; +import { formatCommandForDisplay } from "src/format-command.js"; import supportsHyperlinks from "supports-hyperlinks"; export default function TerminalChatResponseItem({ @@ -41,8 +42,12 @@ export default function TerminalChatResponseItem({ fileOpener={fileOpener} /> ); + // @ts-expect-error new item types aren't in SDK yet + case "local_shell_call": case "function_call": return ; + // @ts-expect-error new item types aren't in SDK yet + case "local_shell_call_output": case "function_call_output": return ( command - {details?.workdir ? ( - {` (${details?.workdir})`} - ) : ( - "" - )} + {workdir ? {` (${workdir})`} : ""} - $ {details?.cmdReadableText} + $ {cmdReadableText} ); @@ -190,7 +202,8 @@ function TerminalChatResponseToolCallOutput({ message, fullStdout, }: { - message: ResponseFunctionToolCallOutputItem; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + message: ResponseFunctionToolCallOutputItem | any; fullStdout: boolean; }) { const { output, metadata } = parseToolCallOutput(message.output); diff --git a/codex-cli/src/utils/agent/agent-loop.ts b/codex-cli/src/utils/agent/agent-loop.ts index 6f04401d5a..9198e7fd1c 100644 --- a/codex-cli/src/utils/agent/agent-loop.ts +++ b/codex-cli/src/utils/agent/agent-loop.ts @@ -8,6 +8,7 @@ import type { ResponseItem, ResponseCreateParams, FunctionTool, + Tool, } from "openai/resources/responses/responses.mjs"; import type { Reasoning } from "openai/resources.mjs"; @@ -84,7 +85,7 @@ type AgentLoopParams = { onLastResponseId: (lastResponseId: string) => void; }; -const shellTool: FunctionTool = { +const shellFunctionTool: FunctionTool = { type: "function", name: "shell", description: "Runs a shell command, and returns its output.", @@ -108,6 +109,11 @@ const shellTool: FunctionTool = { }, }; +const localShellTool: Tool = { + //@ts-expect-error - waiting on sdk + type: "local_shell", +}; + export class AgentLoop { private model: string; private provider: string; @@ -461,6 +467,73 @@ export class AgentLoop { return [outputItem, ...additionalItems]; } + private async handleLocalShellCall( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + item: any, + ): Promise> { + // If the agent has been canceled in the meantime we should not perform any + // additional work. Returning an empty array ensures that we neither execute + // the requested tool call nor enqueue any follow‑up input items. This keeps + // the cancellation semantics intuitive for users – once they interrupt a + // task no further actions related to that task should be taken. + if (this.canceled) { + return []; + } + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const outputItem: any = { + type: "local_shell_call_output", + // `call_id` is mandatory – ensure we never send `undefined` which would + // trigger the "No tool output found…" 400 from the API. + call_id: item.call_id, + output: "no function found", + }; + + // We intentionally *do not* remove this `callId` from the `pendingAborts` + // set right away. The output produced below is only queued up for the + // *next* request to the OpenAI API – it has not been delivered yet. If + // the user presses ESC‑ESC (i.e. invokes `cancel()`) in the small window + // between queuing the result and the actual network call, we need to be + // able to surface a synthetic `function_call_output` marked as + // "aborted". Keeping the ID in the set until the run concludes + // successfully lets the next `run()` differentiate between an aborted + // tool call (needs the synthetic output) and a completed one (cleared + // below in the `flush()` helper). + + // used to tell model to stop if needed + const additionalItems: Array = []; + + if (item.action.type !== "exec") { + throw new Error("Invalid action type"); + } + + const args = { + cmd: item.action.command, + workdir: item.action.working_directory, + timeoutInMillis: item.action.timeout_ms, + }; + + const { + outputText, + metadata, + additionalItems: additionalItemsFromExec, + } = await handleExecCommand( + args, + this.config, + this.approvalPolicy, + this.additionalWritableRoots, + this.getCommandConfirmation, + this.execAbortController?.signal, + ); + outputItem.output = JSON.stringify({ output: outputText, metadata }); + + if (additionalItemsFromExec) { + additionalItems.push(...additionalItemsFromExec); + } + + return [outputItem, ...additionalItems]; + } + public async run( input: Array, previousResponseId: string = "", @@ -545,6 +618,11 @@ export class AgentLoop { // `disableResponseStorage === true`. let transcriptPrefixLen = 0; + let tools: Array = [shellFunctionTool]; + if (this.model.startsWith("codex")) { + tools = [localShellTool]; + } + const stripInternalFields = ( item: ResponseInputItem, ): ResponseInputItem => { @@ -648,6 +726,8 @@ export class AgentLoop { if ( (item as ResponseInputItem).type === "function_call" || (item as ResponseInputItem).type === "reasoning" || + //@ts-expect-error - waiting on sdk + (item as ResponseInputItem).type === "local_shell_call" || ((item as ResponseInputItem).type === "message" && // eslint-disable-next-line @typescript-eslint/no-explicit-any (item as any).role === "user") @@ -748,7 +828,7 @@ export class AgentLoop { store: true, previous_response_id: lastResponseId || undefined, }), - tools: [shellTool], + tools: tools, // Explicitly tell the model it is allowed to pick whatever // tool it deems appropriate. Omitting this sometimes leads to // the model ignoring the available tools and responding with @@ -968,7 +1048,10 @@ export class AgentLoop { if (maybeReasoning.type === "reasoning") { maybeReasoning.duration_ms = Date.now() - thinkingStart; } - if (item.type === "function_call") { + if ( + item.type === "function_call" || + item.type === "local_shell_call" + ) { // Track outstanding tool call so we can abort later if needed. // The item comes from the streaming response, therefore it has // either `id` (chat) or `call_id` (responses) – we normalise @@ -1091,7 +1174,11 @@ export class AgentLoop { let reasoning: Reasoning | undefined; if (this.model.startsWith("o")) { reasoning = { effort: "high" }; - if (this.model === "o3" || this.model === "o4-mini") { + if ( + this.model === "o3" || + this.model === "o4-mini" || + this.model === "codex-mini-latest" + ) { reasoning.summary = "auto"; } } @@ -1130,7 +1217,7 @@ export class AgentLoop { store: true, previous_response_id: lastResponseId || undefined, }), - tools: [shellTool], + tools: tools, tool_choice: "auto", }); @@ -1492,6 +1579,17 @@ export class AgentLoop { // eslint-disable-next-line no-await-in-loop const result = await this.handleFunctionCall(item); turnInput.push(...result); + //@ts-expect-error - waiting on sdk + } else if (item.type === "local_shell_call") { + //@ts-expect-error - waiting on sdk + if (alreadyProcessedResponses.has(item.id)) { + continue; + } + //@ts-expect-error - waiting on sdk + alreadyProcessedResponses.add(item.id); + // eslint-disable-next-line no-await-in-loop + const result = await this.handleLocalShellCall(item); + turnInput.push(...result); } emitItem(item as ResponseItem); } diff --git a/codex-cli/src/utils/config.ts b/codex-cli/src/utils/config.ts index aba99b1352..95183937dd 100644 --- a/codex-cli/src/utils/config.ts +++ b/codex-cli/src/utils/config.ts @@ -43,7 +43,7 @@ if (!isVitest) { loadDotenv({ path: USER_WIDE_CONFIG_PATH }); } -export const DEFAULT_AGENTIC_MODEL = "o4-mini"; +export const DEFAULT_AGENTIC_MODEL = "codex-mini-latest"; export const DEFAULT_FULL_CONTEXT_MODEL = "gpt-4.1"; export const DEFAULT_APPROVAL_MODE = AutoApprovalMode.SUGGEST; export const DEFAULT_INSTRUCTIONS = ""; diff --git a/codex-cli/src/utils/model-info.ts b/codex-cli/src/utils/model-info.ts index bbe0cb36a9..50c899d09b 100644 --- a/codex-cli/src/utils/model-info.ts +++ b/codex-cli/src/utils/model-info.ts @@ -19,6 +19,10 @@ export const openAiModelInfo = { label: "o3 (2025-04-16)", maxContextLength: 200000, }, + "codex-mini-latest": { + label: "codex-mini-latest", + maxContextLength: 200000, + }, "o4-mini": { label: "o4 Mini", maxContextLength: 200000, diff --git a/codex-cli/tests/config.test.tsx b/codex-cli/tests/config.test.tsx index 05703e7ef1..55c2297fc0 100644 --- a/codex-cli/tests/config.test.tsx +++ b/codex-cli/tests/config.test.tsx @@ -67,7 +67,7 @@ test("loads default config if files don't exist", () => { }); // Keep the test focused on just checking that default model and instructions are loaded // so we need to make sure we check just these properties - expect(config.model).toBe("o4-mini"); + expect(config.model).toBe("codex-mini-latest"); expect(config.instructions).toBe(""); }); diff --git a/codex-cli/tests/disableResponseStorage.agentLoop.test.ts b/codex-cli/tests/disableResponseStorage.agentLoop.test.ts index b891e89ae9..7305ff98a7 100644 --- a/codex-cli/tests/disableResponseStorage.agentLoop.test.ts +++ b/codex-cli/tests/disableResponseStorage.agentLoop.test.ts @@ -29,7 +29,7 @@ describe.each([ ])("AgentLoop with disableResponseStorage=%s", ({ flag, title }) => { /* build a fresh config for each case */ const cfg: AppConfig = { - model: "o4-mini", + model: "codex-mini-latest", provider: "openai", instructions: "", disableResponseStorage: flag, diff --git a/codex-cli/tests/disableResponseStorage.test.ts b/codex-cli/tests/disableResponseStorage.test.ts index 83c2245044..e16fb447eb 100644 --- a/codex-cli/tests/disableResponseStorage.test.ts +++ b/codex-cli/tests/disableResponseStorage.test.ts @@ -21,7 +21,10 @@ describe("disableResponseStorage persistence", () => { mkdirSync(codexDir, { recursive: true }); // seed YAML with ZDR enabled - writeFileSync(yamlPath, "model: o4-mini\ndisableResponseStorage: true\n"); + writeFileSync( + yamlPath, + "model: codex-mini-latest\ndisableResponseStorage: true\n", + ); }); afterAll((): void => { diff --git a/codex-cli/tests/model-utils-network-error.test.ts b/codex-cli/tests/model-utils-network-error.test.ts index 537e7fdb35..9e2718baab 100644 --- a/codex-cli/tests/model-utils-network-error.test.ts +++ b/codex-cli/tests/model-utils-network-error.test.ts @@ -44,7 +44,10 @@ describe("model-utils – offline resilience", () => { "../src/utils/model-utils.js" ); - const supported = await isModelSupportedForResponses("openai", "o4-mini"); + const supported = await isModelSupportedForResponses( + "openai", + "codex-mini-latest", + ); expect(supported).toBe(true); }); From 30cbfdfa87287cfb422804540b17c75940417a07 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 16 May 2025 08:14:50 -0700 Subject: [PATCH 2/6] chore: update exec crate to use std::time instead of chrono (#952) When I originally wrote `elapsed.rs`, I realized we were using both `std::time` and `chrono` with no real benefit of having both. We should try to keep the `exec` subcommand trim (as it also buildable as a standalone executable), so this helps tighten things up. --- codex-rs/Cargo.lock | 1 - codex-rs/common/Cargo.toml | 3 +- codex-rs/common/src/elapsed.rs | 56 ++++++++++++++-------------- codex-rs/exec/src/event_processor.rs | 13 ++++--- 4 files changed, 36 insertions(+), 37 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 15bdf08b5e..7b12b654ef 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -503,7 +503,6 @@ dependencies = [ name = "codex-common" version = "0.0.0" dependencies = [ - "chrono", "clap", "codex-core", ] diff --git a/codex-rs/common/Cargo.toml b/codex-rs/common/Cargo.toml index ac0984e2a9..95e4a53182 100644 --- a/codex-rs/common/Cargo.toml +++ b/codex-rs/common/Cargo.toml @@ -7,11 +7,10 @@ edition = "2024" workspace = true [dependencies] -chrono = { version = "0.4.40", optional = true } clap = { version = "4", features = ["derive", "wrap_help"], optional = true } codex-core = { path = "../core" } [features] # Separate feature so that `clap` is not a mandatory dependency. cli = ["clap"] -elapsed = ["chrono"] +elapsed = [] diff --git a/codex-rs/common/src/elapsed.rs b/codex-rs/common/src/elapsed.rs index 72108f9dd0..e5e22a3ad1 100644 --- a/codex-rs/common/src/elapsed.rs +++ b/codex-rs/common/src/elapsed.rs @@ -1,18 +1,19 @@ -use chrono::Utc; +use std::time::Duration; +use std::time::Instant; /// Returns a string representing the elapsed time since `start_time` like /// "1m15s" or "1.50s". -pub fn format_elapsed(start_time: chrono::DateTime) -> String { - let elapsed = Utc::now().signed_duration_since(start_time); - format_time_delta(elapsed) +pub fn format_elapsed(start_time: Instant) -> String { + format_duration(start_time.elapsed()) } -fn format_time_delta(elapsed: chrono::TimeDelta) -> String { - let millis = elapsed.num_milliseconds(); - format_elapsed_millis(millis) -} - -pub fn format_duration(duration: std::time::Duration) -> String { +/// Convert a [`std::time::Duration`] into a human-readable, compact string. +/// +/// Formatting rules: +/// * < 1 s -> "{milli}ms" +/// * < 60 s -> "{sec:.2}s" (two decimal places) +/// * >= 60 s -> "{min}m{sec:02}s" +pub fn format_duration(duration: Duration) -> String { let millis = duration.as_millis() as i64; format_elapsed_millis(millis) } @@ -32,41 +33,40 @@ fn format_elapsed_millis(millis: i64) -> String { #[cfg(test)] mod tests { use super::*; - use chrono::Duration; #[test] - fn test_format_time_delta_subsecond() { + fn test_format_duration_subsecond() { // Durations < 1s should be rendered in milliseconds with no decimals. - let dur = Duration::milliseconds(250); - assert_eq!(format_time_delta(dur), "250ms"); + let dur = Duration::from_millis(250); + assert_eq!(format_duration(dur), "250ms"); // Exactly zero should still work. - let dur_zero = Duration::milliseconds(0); - assert_eq!(format_time_delta(dur_zero), "0ms"); + let dur_zero = Duration::from_millis(0); + assert_eq!(format_duration(dur_zero), "0ms"); } #[test] - fn test_format_time_delta_seconds() { + fn test_format_duration_seconds() { // Durations between 1s (inclusive) and 60s (exclusive) should be // printed with 2-decimal-place seconds. - let dur = Duration::milliseconds(1_500); // 1.5s - assert_eq!(format_time_delta(dur), "1.50s"); + let dur = Duration::from_millis(1_500); // 1.5s + assert_eq!(format_duration(dur), "1.50s"); // 59.999s rounds to 60.00s - let dur2 = Duration::milliseconds(59_999); - assert_eq!(format_time_delta(dur2), "60.00s"); + let dur2 = Duration::from_millis(59_999); + assert_eq!(format_duration(dur2), "60.00s"); } #[test] - fn test_format_time_delta_minutes() { + fn test_format_duration_minutes() { // Durations ≥ 1 minute should be printed mmss. - let dur = Duration::milliseconds(75_000); // 1m15s - assert_eq!(format_time_delta(dur), "1m15s"); + let dur = Duration::from_millis(75_000); // 1m15s + assert_eq!(format_duration(dur), "1m15s"); - let dur_exact = Duration::milliseconds(60_000); // 1m0s - assert_eq!(format_time_delta(dur_exact), "1m00s"); + let dur_exact = Duration::from_millis(60_000); // 1m0s + assert_eq!(format_duration(dur_exact), "1m00s"); - let dur_long = Duration::milliseconds(3_601_000); - assert_eq!(format_time_delta(dur_long), "60m01s"); + let dur_long = Duration::from_millis(3_601_000); + assert_eq!(format_duration(dur_long), "60m01s"); } } diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index f1f644cba7..4c8278cc59 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -17,6 +17,7 @@ use owo_colors::OwoColorize; use owo_colors::Style; use shlex::try_join; use std::collections::HashMap; +use std::time::Instant; /// This should be configurable. When used in CI, users may not want to impose /// a limit so they can see the full transcript. @@ -76,7 +77,7 @@ impl EventProcessor { struct ExecCommandBegin { command: Vec, - start_time: chrono::DateTime, + start_time: Instant, } /// Metadata captured when an `McpToolCallBegin` event is received. @@ -84,11 +85,11 @@ struct McpToolCallBegin { /// Formatted invocation string, e.g. `server.tool({"city":"sf"})`. invocation: String, /// Timestamp when the call started so we can compute duration later. - start_time: chrono::DateTime, + start_time: Instant, } struct PatchApplyBegin { - start_time: chrono::DateTime, + start_time: Instant, auto_approved: bool, } @@ -133,7 +134,7 @@ impl EventProcessor { call_id.clone(), ExecCommandBegin { command: command.clone(), - start_time: Utc::now(), + start_time: Instant::now(), }, ); ts_println!( @@ -208,7 +209,7 @@ impl EventProcessor { call_id.clone(), McpToolCallBegin { invocation: invocation.clone(), - start_time: Utc::now(), + start_time: Instant::now(), }, ); @@ -263,7 +264,7 @@ impl EventProcessor { self.call_id_to_patch.insert( call_id.clone(), PatchApplyBegin { - start_time: Utc::now(), + start_time: Instant::now(), auto_approved, }, ); From 316289d01dc7c4663085d11cc39f17416a782452 Mon Sep 17 00:00:00 2001 From: Fouad Matin <169186268+fouad-openai@users.noreply.github.com> Date: Fri, 16 May 2025 08:18:20 -0700 Subject: [PATCH 3/6] bump(version): 0.1.2505160811 `codex-mini-latest` (#953) ## `0.1.2505160811` - `codex-mini-latest` (#951) --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92707acfbf..e30f705019 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ You can install any of these versions: `npm install -g codex@version` +## `0.1.2505160811` + +- `codex-mini-latest` (#951) + ## `0.1.2505140839` ### 🪲 Bug Fixes From 7edfbae062ed0e8751c89a4bd69a1747d586cce6 Mon Sep 17 00:00:00 2001 From: hanson-openai Date: Fri, 16 May 2025 09:10:44 -0700 Subject: [PATCH 4/6] fix: diff command for filenames with special characters (#954) ## Summary - fix quoting issues in `/diff` to correctly handle files with special characters - add regression test for `getGitDiff` when filenames contain `$` - relax timeout in raw-exec-process-group test Fixes https://github.com/openai/codex/issues/943 ## Testing - `pnpm test` --- codex-cli/src/utils/get-diff.ts | 20 ++++++++----- .../tests/get-diff-special-chars.test.ts | 28 +++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 codex-cli/tests/get-diff-special-chars.test.ts diff --git a/codex-cli/src/utils/get-diff.ts b/codex-cli/src/utils/get-diff.ts index 9ac7844d2a..cd3d2040f1 100644 --- a/codex-cli/src/utils/get-diff.ts +++ b/codex-cli/src/utils/get-diff.ts @@ -1,4 +1,4 @@ -import { execSync } from "node:child_process"; +import { execSync, execFileSync } from "node:child_process"; // The objects thrown by `child_process.execSync()` are `Error` instances that // include additional, undocumented properties such as `status` (exit code) and @@ -89,12 +89,18 @@ export function getGitDiff(): { // // `git diff --color --no-index /dev/null ` exits with status 1 // when differences are found, so we capture stdout from the thrown - // error object instead of letting it propagate. - execSync(`git diff --color --no-index -- "${nullDevice}" "${file}"`, { - encoding: "utf8", - stdio: ["ignore", "pipe", "ignore"], - maxBuffer: 10 * 1024 * 1024, - }); + // error object instead of letting it propagate. Using `execFileSync` + // avoids shell interpolation issues with special characters in the + // path. + execFileSync( + "git", + ["diff", "--color", "--no-index", "--", nullDevice, file], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + maxBuffer: 10 * 1024 * 1024, + }, + ); } catch (err) { if ( isExecSyncError(err) && diff --git a/codex-cli/tests/get-diff-special-chars.test.ts b/codex-cli/tests/get-diff-special-chars.test.ts new file mode 100644 index 0000000000..e701fddfe2 --- /dev/null +++ b/codex-cli/tests/get-diff-special-chars.test.ts @@ -0,0 +1,28 @@ +import { mkdtempSync, writeFileSync, rmSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { execSync } from "child_process"; +import { describe, it, expect } from "vitest"; + +import { getGitDiff } from "../src/utils/get-diff.js"; + +describe("getGitDiff", () => { + it("handles untracked files with special characters", () => { + const repoDir = mkdtempSync(join(tmpdir(), "git-diff-test-")); + const prevCwd = process.cwd(); + try { + process.chdir(repoDir); + execSync("git init", { stdio: "ignore" }); + + const fileName = "a$b.txt"; + writeFileSync(join(repoDir, fileName), "hello\n"); + + const { isGitRepo, diff } = getGitDiff(); + expect(isGitRepo).toBe(true); + expect(diff).toContain(fileName); + } finally { + process.chdir(prevCwd); + rmSync(repoDir, { recursive: true, force: true }); + } + }); +}); From 84e01f4b6215dd2067447475a85a9a42b0211bb8 Mon Sep 17 00:00:00 2001 From: Sebastian Lund Date: Fri, 16 May 2025 12:12:16 -0400 Subject: [PATCH 5/6] fix: apply patch issue when using different cwd (#942) If you run a codex instance outside of the current working directory from where you launched the codex binary it won't be able to apply patches correctly, even if the sandbox policy allows it. This manifests weird behaviours, such as * Reading the same filename in the binary working directory, and overwriting it in the session working directory. e.g. if you have a `readme` in both folders it will overwrite the readme in the session working directory with the readme in the binary working directory *applied with the suggested patch*. * The LLM ends up in weird loops trying to verify and debug why the apply_patch won't work, and it can result in it applying patches by manually writing python or javascript if it figures out that either is supported by the system instead. I added a test-case to ensure that the patch contents are based on the cwd. ## Issue: mixing relative & absolute paths in apply_patch 1. The apply_patch tool use relative paths based on the session working directory. 2. `unified_diff_from_chunks` eventually ends up [reading the source file](https://github.com/reflectionai/codex/blob/main/codex-rs/apply-patch/src/lib.rs#L410) to figure out what the diff is, by using the relative path. 3. The changes are targeted using an absolute path derived from the current working directory. The end-result in case session working directory differs from the binary working directory: we get the diff for a file relative to the binary working directory, and apply it on a file in the session working directory. --- codex-rs/apply-patch/src/lib.rs | 96 +++++++++++++++++++++++++----- codex-rs/apply-patch/src/parser.rs | 12 ++++ 2 files changed, 92 insertions(+), 16 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index afabcea498..a144f0b41c 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -18,7 +18,7 @@ use thiserror::Error; use tree_sitter::Parser; use tree_sitter_bash::LANGUAGE as BASH; -#[derive(Debug, Error)] +#[derive(Debug, Error, PartialEq)] pub enum ApplyPatchError { #[error(transparent)] ParseError(#[from] ParseError), @@ -46,6 +46,12 @@ pub struct IoError { source: std::io::Error, } +impl PartialEq for IoError { + fn eq(&self, other: &Self) -> bool { + self.context == other.context && self.source.to_string() == other.source.to_string() + } +} + #[derive(Debug)] pub enum MaybeApplyPatch { Body(Vec), @@ -77,7 +83,7 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { } } -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum ApplyPatchFileChange { Add { content: String, @@ -106,7 +112,27 @@ pub enum MaybeApplyPatchVerified { NotApplyPatch, } -#[derive(Debug)] +impl PartialEq for MaybeApplyPatchVerified { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (MaybeApplyPatchVerified::Body(a), MaybeApplyPatchVerified::Body(b)) => a == b, + ( + MaybeApplyPatchVerified::ShellParseError(a), + MaybeApplyPatchVerified::ShellParseError(b), + ) => a.to_string() == b.to_string(), + ( + MaybeApplyPatchVerified::CorrectnessError(a), + MaybeApplyPatchVerified::CorrectnessError(b), + ) => a == b, + (MaybeApplyPatchVerified::NotApplyPatch, MaybeApplyPatchVerified::NotApplyPatch) => { + true + } + _ => false, + } + } +} + +#[derive(Debug, PartialEq)] /// ApplyPatchAction is the result of parsing an `apply_patch` command. By /// construction, all paths should be absolute paths. pub struct ApplyPatchAction { @@ -142,22 +168,16 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp MaybeApplyPatch::Body(hunks) => { let mut changes = HashMap::new(); for hunk in hunks { + let path = hunk.resolve_path(cwd); match hunk { - Hunk::AddFile { path, contents } => { - changes.insert( - cwd.join(path), - ApplyPatchFileChange::Add { - content: contents.clone(), - }, - ); + Hunk::AddFile { contents, .. } => { + changes.insert(path, ApplyPatchFileChange::Add { content: contents }); } - Hunk::DeleteFile { path } => { - changes.insert(cwd.join(path), ApplyPatchFileChange::Delete); + Hunk::DeleteFile { .. } => { + changes.insert(path, ApplyPatchFileChange::Delete); } Hunk::UpdateFile { - path, - move_path, - chunks, + move_path, chunks, .. } => { let ApplyPatchFileUpdate { unified_diff, @@ -169,7 +189,7 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp } }; changes.insert( - cwd.join(path), + path, ApplyPatchFileChange::Update { unified_diff, move_path: move_path.map(|p| cwd.join(p)), @@ -1137,4 +1157,48 @@ g "# ); } + + #[test] + fn test_apply_patch_should_resolve_absolute_paths_in_cwd() { + let session_dir = tempdir().unwrap(); + let relative_path = "source.txt"; + + // Note that we need this file to exist for the patch to be "verified" + // and parsed correctly. + let session_file_path = session_dir.path().join(relative_path); + fs::write(&session_file_path, "session directory content\n").unwrap(); + + let argv = vec![ + "apply_patch".to_string(), + r#"*** Begin Patch +*** Update File: source.txt +@@ +-session directory content ++updated session directory content +*** End Patch"# + .to_string(), + ]; + + let result = maybe_parse_apply_patch_verified(&argv, session_dir.path()); + + // Verify the patch contents - as otherwise we may have pulled contents + // from the wrong file (as we're using relative paths) + assert_eq!( + result, + MaybeApplyPatchVerified::Body(ApplyPatchAction { + changes: HashMap::from([( + session_dir.path().join(relative_path), + ApplyPatchFileChange::Update { + unified_diff: r#"@@ -1 +1 @@ +-session directory content ++updated session directory content +"# + .to_string(), + move_path: None, + new_content: "updated session directory content\n".to_string(), + }, + )]), + }) + ); + } } diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 40547b4231..391255defa 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -22,6 +22,7 @@ //! //! The parser below is a little more lenient than the explicit spec and allows for //! leading/trailing whitespace around patch markers. +use std::path::Path; use std::path::PathBuf; use thiserror::Error; @@ -64,6 +65,17 @@ pub enum Hunk { chunks: Vec, }, } + +impl Hunk { + pub fn resolve_path(&self, cwd: &Path) -> PathBuf { + match self { + Hunk::AddFile { path, .. } => cwd.join(path), + Hunk::DeleteFile { path } => cwd.join(path), + Hunk::UpdateFile { path, .. } => cwd.join(path), + } + } +} + use Hunk::*; #[derive(Debug, PartialEq)] From 02a569ac31fb8ca67044ae958b6c107331da1bb2 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 16 May 2025 09:22:31 -0700 Subject: [PATCH 6/6] feat: add support for file_opener option in Rust, similiar to #911 --- codex-rs/Cargo.lock | 3 + codex-rs/README.md | 13 ++ codex-rs/core/src/config.rs | 31 ++++- codex-rs/tui/Cargo.toml | 7 +- codex-rs/tui/src/chatwidget.rs | 6 +- codex-rs/tui/src/citation_regex.rs | 23 ++++ .../tui/src/conversation_history_widget.rs | 8 +- codex-rs/tui/src/history_cell.rs | 8 +- codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/markdown.rs | 129 +++++++++++++++++- 10 files changed, 215 insertions(+), 14 deletions(-) create mode 100644 codex-rs/tui/src/citation_regex.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 7b12b654ef..0bb478cbfc 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -629,8 +629,11 @@ dependencies = [ "codex-core", "color-eyre", "crossterm", + "lazy_static", "mcp-types", + "pretty_assertions", "ratatui", + "regex", "serde_json", "shlex", "strum 0.27.1", diff --git a/codex-rs/README.md b/codex-rs/README.md index 9fe9827bff..963df780d6 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -310,6 +310,19 @@ To disable this behavior, configure `[history]` as follows: persistence = "none" # "save-all" is the default value ``` +### file_opener + +Identifies the editor/URI scheme to use for hyperlinking citations in model output. If set, citations to files in the model output will be hyperlinked using the specified URI scheme so they can be ctrl/cmd-clicked from the terminal to open them. + +For example, if the model output includes a refrence such as `【F:/home/user/project/main.py†L42-L50】`, then this would be rewritten to link to the URI `vscode://file/home/user/project/main.py:42`. + +Note this is **not** a general editor setting (like `$EDITOR`), as it only accepts a fixed set of values: + +- `vscode` (default) +- `vscode-insiders` +- `windsurf` +- `cursor` + ### project_doc_max_bytes Maximum number of bytes to read from an `AGENTS.md` file to include in the instructions sent with the first turn of a session. Defaults to 32 KiB. diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index b63b51e036..b285a9ddf3 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -84,6 +84,10 @@ pub struct Config { /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. pub history: History, + + /// Optional URI-based file opener. If set, citations to files in the model + /// output will be hyperlinked using the specified URI scheme. + pub file_opener: Option, } /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. @@ -97,7 +101,7 @@ pub struct History { pub max_bytes: Option, } -#[derive(Deserialize, Debug, Clone, PartialEq, Default)] +#[derive(Deserialize, Debug, Copy, Clone, PartialEq, Default)] #[serde(rename_all = "kebab-case")] pub enum HistoryPersistence { /// Save all history entries to disk. @@ -107,6 +111,21 @@ pub enum HistoryPersistence { None, } +#[derive(Deserialize, Debug, Copy, Clone, PartialEq)] +pub enum UriBasedFileOpener { + #[serde(rename = "vscode")] + VsCode, + + #[serde(rename = "vscode-insiders")] + VsCodeInsiders, + + #[serde(rename = "windsurf")] + Windsurf, + + #[serde(rename = "cursor")] + Cursor, +} + /// Base config deserialized from ~/.codex/config.toml. #[derive(Deserialize, Debug, Clone, Default)] pub struct ConfigToml { @@ -158,6 +177,10 @@ pub struct ConfigToml { /// Settings that govern if and what will be written to `~/.codex/history.jsonl`. #[serde(default)] pub history: Option, + + /// Optional URI-based file opener. If set, citations to files in the model + /// output will be hyperlinked using the specified URI scheme. + pub file_opener: Option, } impl ConfigToml { @@ -351,6 +374,9 @@ impl Config { project_doc_max_bytes: cfg.project_doc_max_bytes.unwrap_or(PROJECT_DOC_MAX_BYTES), codex_home, history, + // TODO(mbolin): Check if user has VS Code installed before + // defaulting to it? + file_opener: cfg.file_opener.or(Some(UriBasedFileOpener::VsCode)), }; Ok(config) } @@ -686,6 +712,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: Some(UriBasedFileOpener::VsCode), }, o3_profile_config ); @@ -721,6 +748,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: Some(UriBasedFileOpener::VsCode), }; assert_eq!(expected_gpt3_profile_config, gpt3_profile_config); @@ -771,6 +799,7 @@ disable_response_storage = true project_doc_max_bytes: PROJECT_DOC_MAX_BYTES, codex_home: fixture.codex_home(), history: History::default(), + file_opener: Some(UriBasedFileOpener::VsCode), }; assert_eq!(expected_zdr_profile_config, zdr_profile_config); diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index fa075ada4a..8afd063aa8 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -22,11 +22,13 @@ codex-core = { path = "../core" } codex-common = { path = "../common", features = ["cli", "elapsed"] } color-eyre = "0.6.3" crossterm = { version = "0.28.1", features = ["bracketed-paste"] } +lazy_static = "1" mcp-types = { path = "../mcp-types" } ratatui = { version = "0.29.0", features = [ "unstable-widget-ref", "unstable-rendered-line-info", ] } +regex = "1" serde_json = "1" shlex = "1.3.0" strum = "0.27.1" @@ -44,4 +46,7 @@ tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } tui-input = "0.11.1" tui-markdown = "0.3.3" tui-textarea = "0.7.0" -uuid = { version = "1" } +uuid = "1" + +[dev-dependencies] +pretty_assertions = "1" diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 6771adb1fa..24d37c4c0a 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -209,11 +209,13 @@ impl ChatWidget<'_> { self.request_redraw(); } EventMsg::AgentMessage(AgentMessageEvent { message }) => { - self.conversation_history.add_agent_message(message); + self.conversation_history + .add_agent_message(&self.config, message); self.request_redraw(); } EventMsg::AgentReasoning(AgentReasoningEvent { text }) => { - self.conversation_history.add_agent_reasoning(text); + self.conversation_history + .add_agent_reasoning(&self.config, text); self.request_redraw(); } EventMsg::TaskStarted => { diff --git a/codex-rs/tui/src/citation_regex.rs b/codex-rs/tui/src/citation_regex.rs new file mode 100644 index 0000000000..354dae35ec --- /dev/null +++ b/codex-rs/tui/src/citation_regex.rs @@ -0,0 +1,23 @@ +#![allow(clippy::expect_used)] + +use regex::Regex; + +// This is defined in its own file so we can limit the scope of +// `allow(clippy::expect_used)` because we cannot scope it to the `lazy_static!` +// macro. +lazy_static::lazy_static! { + /// Regular expression that matches Codex-style source file citations such as: + /// + /// ```text + /// 【F:src/main.rs†L10-L20】 + /// ``` + /// + /// Capture groups: + /// 1. file path (anything except the dagger `†` symbol) + /// 2. start line number (digits) + /// 3. optional end line (digits or `?`) + #[allow(clippy::expect_used)] + pub(crate) static ref CITATION_REGEX: Regex = Regex::new( + r"【F:([^†]+)†L(\d+)(?:-L(\d+|\?))?】" + ).expect("failed to compile citation regex"); +} diff --git a/codex-rs/tui/src/conversation_history_widget.rs b/codex-rs/tui/src/conversation_history_widget.rs index 3d2d1cd59b..16fc3f4874 100644 --- a/codex-rs/tui/src/conversation_history_widget.rs +++ b/codex-rs/tui/src/conversation_history_widget.rs @@ -183,12 +183,12 @@ impl ConversationHistoryWidget { self.add_to_history(HistoryCell::new_user_prompt(message)); } - pub fn add_agent_message(&mut self, message: String) { - self.add_to_history(HistoryCell::new_agent_message(message)); + pub fn add_agent_message(&mut self, config: &Config, message: String) { + self.add_to_history(HistoryCell::new_agent_message(config, message)); } - pub fn add_agent_reasoning(&mut self, text: String) { - self.add_to_history(HistoryCell::new_agent_reasoning(text)); + pub fn add_agent_reasoning(&mut self, config: &Config, text: String) { + self.add_to_history(HistoryCell::new_agent_reasoning(config, text)); } pub fn add_background_event(&mut self, message: String) { diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 066ed335df..5ffeaeb5aa 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -155,19 +155,19 @@ impl HistoryCell { HistoryCell::UserPrompt { lines } } - pub(crate) fn new_agent_message(message: String) -> Self { + pub(crate) fn new_agent_message(config: &Config, message: String) -> Self { let mut lines: Vec> = Vec::new(); lines.push(Line::from("codex".magenta().bold())); - append_markdown(&message, &mut lines); + append_markdown(&message, &mut lines, config.file_opener, &config.cwd); lines.push(Line::from("")); HistoryCell::AgentMessage { lines } } - pub(crate) fn new_agent_reasoning(text: String) -> Self { + pub(crate) fn new_agent_reasoning(config: &Config, text: String) -> Self { let mut lines: Vec> = Vec::new(); lines.push(Line::from("thinking".magenta().italic())); - append_markdown(&text, &mut lines); + append_markdown(&text, &mut lines, config.file_opener, &config.cwd); lines.push(Line::from("")); HistoryCell::AgentReasoning { lines } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5e3ed9b6a0..a6849f62fb 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -19,6 +19,7 @@ mod app_event; mod app_event_sender; mod bottom_pane; mod chatwidget; +mod citation_regex; mod cli; mod conversation_history_widget; mod exec_command; diff --git a/codex-rs/tui/src/markdown.rs b/codex-rs/tui/src/markdown.rs index 9837f3e20c..73997c3d89 100644 --- a/codex-rs/tui/src/markdown.rs +++ b/codex-rs/tui/src/markdown.rs @@ -1,8 +1,36 @@ +use codex_core::config::UriBasedFileOpener; use ratatui::text::Line; use ratatui::text::Span; +use std::path::Path; +use std::path::PathBuf; -pub(crate) fn append_markdown(markdown_source: &str, lines: &mut Vec>) { - let markdown = tui_markdown::from_str(markdown_source); +use crate::citation_regex::CITATION_REGEX; + +/// Convert the provided markdown to [`ratatui`] [`Line`]s and append them to +/// `lines`. Before rendering we rewrite Codex-style file citations of the form +/// `【F:path/to/file†L10-L20】` into standard markdown hyperlinks that IDEs such +/// as VS Code can interpret when combined with the "URI-based file opener" +/// functionality. +/// +/// The specific URI scheme to use is determined by `file_opener` (e.g. +/// `vscode`, `vscode-insiders`, `cursor`, `windsurf`). When it is `None` the +/// citations are **left unchanged** so they still render as plain text. +pub(crate) fn append_markdown( + markdown_source: &str, + lines: &mut Vec>, + file_opener: Option, + cwd: &Path, +) { + // Perform citation rewrite *before* feeding the string to the markdown + // renderer. When `file_opener` is absent we bypass the transformation to + // avoid unnecessary allocations. + let processed_markdown: std::borrow::Cow<'_, str> = if let Some(scheme) = file_opener { + std::borrow::Cow::Owned(rewrite_file_citations(markdown_source, scheme, cwd)) + } else { + std::borrow::Cow::Borrowed(markdown_source) + }; + + let markdown = tui_markdown::from_str(&processed_markdown); // `tui_markdown` returns a `ratatui::text::Text` where every `Line` borrows // from the input `message` string. Since the `HistoryCell` stores its lines @@ -28,3 +56,100 @@ pub(crate) fn append_markdown(markdown_source: &str, lines: &mut Vec://file: +/// ``` +fn rewrite_file_citations(src: &str, scheme: UriBasedFileOpener, cwd: &Path) -> String { + // Map enum values to the corresponding URI scheme strings. + let scheme_str: &str = match scheme { + UriBasedFileOpener::VsCode => "vscode", + UriBasedFileOpener::VsCodeInsiders => "vscode-insiders", + UriBasedFileOpener::Windsurf => "windsurf", + UriBasedFileOpener::Cursor => "cursor", + }; + + CITATION_REGEX + .replace_all(src, |caps: ®ex::Captures<'_>| { + let file = &caps[1]; + let start_line = &caps[2]; + + // Resolve the path against `cwd` when it is relative. + let path = { + let p = Path::new(file); + if p.is_absolute() { + PathBuf::from(p) + } else { + cwd.join(p) + } + }; + + // VS Code expects forward slashes even on Windows because URIs use + // `/` as the path separator. + let abs = path.to_string_lossy().replace('\\', "/"); + + // Render as a normal markdown link so the downstream renderer emits + // the hyperlink escape sequence (when supported by the terminal). + format!("[{}]({}://file{}:{})", file, scheme_str, abs, start_line) + }) + .into_owned() +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn citation_is_rewritten_with_absolute_path() { + let markdown = "See 【F:/src/main.rs†L42-L50】 for details."; + let cwd = Path::new("/workspace"); + let result = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); + + assert_eq!( + "See [/src/main.rs](vscode://file/src/main.rs:42) for details.", + result + ); + } + + #[test] + fn citation_is_rewritten_with_relative_path() { + let markdown = "Refer to 【F:lib/mod.rs†L5】 here."; + let cwd = Path::new("/home/user/project"); + let result = rewrite_file_citations(markdown, UriBasedFileOpener::Cursor, cwd); + + assert_eq!( + "Refer to [lib/mod.rs](cursor://file/home/user/project/lib/mod.rs:5) here.", + result + ); + } + + #[test] + fn citation_unchanged_without_file_opener() { + let markdown = "Look at 【F:file.rs†L1】."; + let cwd = Path::new("/"); + let unchanged = rewrite_file_citations(markdown, UriBasedFileOpener::VsCode, cwd); + // The helper itself always rewrites – this test validates behaviour of + // append_markdown when `file_opener` is None. + let mut out = Vec::new(); + append_markdown(markdown, &mut out, None, cwd); + // Convert lines back to string for comparison. + let rendered: String = out + .iter() + .flat_map(|l| l.spans.iter()) + .map(|s| s.content.clone()) + .collect::>() + .join(""); + assert_eq!(markdown, rendered); + // Ensure helper rewrites. + assert_ne!(markdown, unchanged); + } +}