From 721003c552747249bad413763f436c7f4b67e41d Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 6 Oct 2025 08:43:50 -0700 Subject: [PATCH 01/24] [MCP] Improve docs (#4811) Updated, expanded on, clarified, and deduplicated some MCP docs --- README.md | 2 +- codex-rs/README.md | 10 +++++-- codex-rs/docs/codex_mcp_interface.md | 4 +-- docs/advanced.md | 10 +------ docs/config.md | 44 +++++++++++++++++++++++----- 5 files changed, 48 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index f17f605e18..c9cc4e1fd4 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ You can also use Codex with an API key, but this requires [additional setup](./d ### Model Context Protocol (MCP) -Codex CLI supports [MCP servers](./docs/advanced.md#model-context-protocol-mcp). Enable by adding an `mcp_servers` section to your `~/.codex/config.toml`. +Codex can access MCP servers. To configure them, refer to the [config docs](./docs/config.md#mcp_servers). ### Configuration diff --git a/codex-rs/README.md b/codex-rs/README.md index f5a5b838fc..a05a8d48b1 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -23,9 +23,15 @@ Codex supports a rich set of configuration options. Note that the Rust CLI uses ### Model Context Protocol Support -Codex CLI functions as an MCP client that can connect to MCP servers on startup. See the [`mcp_servers`](../docs/config.md#mcp_servers) section in the configuration documentation for details. +#### MCP client -It is still experimental, but you can also launch Codex as an MCP _server_ by running `codex mcp-server`. Use the [`@modelcontextprotocol/inspector`](https://github.com/modelcontextprotocol/inspector) to try it out: +Codex CLI functions as an MCP client that allows the Codex CLI and IDE extension to connect to MCP servers on startup. See the [`configuration documentation`](../docs/config.md#mcp_servers) for details. + +#### MCP server (experimental) + +Codex can be launched as an MCP _server_ by running `codex mcp-server`. This allows _other_ MCP clients to use Codex as a tool for another agent. + +Use the [`@modelcontextprotocol/inspector`](https://github.com/modelcontextprotocol/inspector) to try it out: ```shell npx @modelcontextprotocol/inspector codex mcp-server diff --git a/codex-rs/docs/codex_mcp_interface.md b/codex-rs/docs/codex_mcp_interface.md index 1291d3ee9f..eb0442feb3 100644 --- a/codex-rs/docs/codex_mcp_interface.md +++ b/codex-rs/docs/codex_mcp_interface.md @@ -1,6 +1,6 @@ -# Codex MCP Interface [experimental] +# Codex MCP Server Interface [experimental] -This document describes Codex’s experimental MCP interface: a JSON‑RPC API that runs over the Model Context Protocol (MCP) transport to control a local Codex engine. +This document describes Codex’s experimental MCP server interface: a JSON‑RPC API that runs over the Model Context Protocol (MCP) transport to control a local Codex engine. - Status: experimental and subject to change without notice - Server binary: `codex mcp-server` (or `codex-mcp-server`) diff --git a/docs/advanced.md b/docs/advanced.md index d5012c2875..7c986f583a 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -16,15 +16,7 @@ See the Rust documentation on [`RUST_LOG`](https://docs.rs/env_logger/latest/env ## Model Context Protocol (MCP) -The Codex CLI can be configured to leverage MCP servers by defining an [`mcp_servers`](./config.md#mcp_servers) section in `~/.codex/config.toml`. It is intended to mirror how tools such as Claude and Cursor define `mcpServers` in their respective JSON config files, though the Codex format is slightly different since it uses TOML rather than JSON, e.g.: - -```toml -# IMPORTANT: the top-level key is `mcp_servers` rather than `mcpServers`. -[mcp_servers.server-name] -command = "npx" -args = ["-y", "mcp-server"] -env = { "API_KEY" = "value" } -``` +The Codex CLI and IDE extension is a MCP client which means that it can be configured to connect to MCP servers. For more information, refer to the [`config docs`](./config.md#connecting-to-mcp-servers). ## Using Codex as an MCP Server diff --git a/docs/config.md b/docs/config.md index f079adda6d..aa45acc8dd 100644 --- a/docs/config.md +++ b/docs/config.md @@ -335,18 +335,20 @@ Codex provides three main Approval Presets: You can further customize how Codex runs at the command line using the `--ask-for-approval` and `--sandbox` options. -## MCP Servers +## Connecting to MCP servers -You can configure Codex to use [MCP servers](https://modelcontextprotocol.io/about) to give Codex access to external applications, resources, or services such as [Playwright](https://github.com/microsoft/playwright-mcp), [Figma](https://www.figma.com/blog/design-context-everywhere-you-build/), [documentation](https://context7.com/), and [more](https://github.com/mcp?utm_source=blog-source&utm_campaign=mcp-registry-server-launch-2025). +You can configure Codex to use [MCP servers](https://modelcontextprotocol.io/about) to give Codex access to external applications, resources, or services. -### Server transport configuration +### Server configuration #### STDIO +[STDIO servers](https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#stdio) are MCP servers that you can launch directly via commands on your computer. + ```toml # The top-level table name must be `mcp_servers` # The sub-table name (`server-name` in this example) can be anything you would like. -[mcp_servers.server-name] +[mcp_servers.server_name] command = "npx" # Optional args = ["-y", "mcp-server"] @@ -354,21 +356,26 @@ args = ["-y", "mcp-server"] # A default whitelist of env vars will be propagated to the MCP server. # https://github.com/openai/codex/blob/main/codex-rs/rmcp-client/src/utils.rs#L82 env = { "API_KEY" = "value" } +# or +[mcp_servers.server_name.env] +API_KEY = "value" ``` #### Streamable HTTP +[Streamable HTTP servers](https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#streamable-http) enable Codex to talk to resources that are accessed via a http url (either on localhost or another domain). + ```toml # Streamable HTTP requires the experimental rmcp client experimental_use_rmcp_client = true [mcp_servers.figma] -url = "http://127.0.0.1:3845/mcp" +url = "https://mcp.linear.app/mcp" # Optional bearer token to be passed into an `Authorization: Bearer ` header -# Use this with caution because the token is in plaintext. +# Use this with caution because the token is in plaintext and can be read by Codex itself. bearer_token = "" ``` -Refer to the MCP CLI commands for oauth login +For oauth login, you must enable `experimental_use_rmcp_client = true` and then run `codex mcp login server_name` ### Other configuration options @@ -381,17 +388,25 @@ tool_timeout_sec = 30 ### Experimental RMCP client -Codex is transitioning to the [official Rust MCP SDK](https://github.com/modelcontextprotocol/rust-sdk) and new functionality such as streamable http servers will only work with the new client. +Codex is transitioning to the [official Rust MCP SDK](https://github.com/modelcontextprotocol/rust-sdk). + +The flag enabled OAuth support for streamable HTTP servers and uses a new STDIO client implementation. Please try and report issues with the new client. To enable it, add this to the top level of your `config.toml` ```toml experimental_use_rmcp_client = true + +[mcp_servers.server_name] +… ``` ### MCP CLI commands ```shell +# List all available commands +codex mcp --help + # Add a server (env can be repeated; `--` separates the launcher command) codex mcp add docs -- docs-server --port 4000 @@ -413,6 +428,19 @@ codex mcp login SERVER_NAME codex mcp logout SERVER_NAME ``` +## Examples of useful MCPs + +There is an ever growing list of useful MCP servers that can be helpful while you are working with Codex. + +Some of the most common MCPs we've seen are: + +- [Context7](https://github.com/upstash/context7) — connect to a wide range of up-to-date developer documentation +- Figma [Local](https://developers.figma.com/docs/figma-mcp-server/local-server-installation/) and [Remote](https://developers.figma.com/docs/figma-mcp-server/remote-server-installation/) - access to your Figma designs +- [Playwright](https://www.npmjs.com/package/@playwright/mcp) - control and inspect a browser using Playwright +- [Chrome Developer Tools](https://github.com/ChromeDevTools/chrome-devtools-mcp/) — control and inspect a Chrome browser +- [Sentry](https://docs.sentry.io/product/sentry-mcp/#codex) — access to your Sentry logs +- [GitHub](https://github.com/github/github-mcp-server) — Control over your GitHub account beyond what git allows (like controlling PRs, issues, etc.) + ## shell_environment_policy Codex spawns subprocesses (e.g. when executing a `local_shell` tool-call suggested by the assistant). By default it now passes **your full environment** to those subprocesses. You can tune this behavior via the **`shell_environment_policy`** block in `config.toml`: From 661663c98a43178bc54fc4e2923c1a7089a90e2c Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 6 Oct 2025 10:07:52 -0700 Subject: [PATCH 02/24] Fix event names in exec docs. (#4833) Fixes: https://github.com/openai/codex/issues --- docs/exec.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/exec.md b/docs/exec.md index a0a7caf9de..e0f695cd4c 100644 --- a/docs/exec.md +++ b/docs/exec.md @@ -30,25 +30,25 @@ Supported event types: Supported item types: -- `assistant_message` - assistant message. +- `agent_message` - assistant message. - `reasoning` - a summary of the assistant's thinking. - `command_execution` - assistant executing a command. - `file_change` - assistant making file changes. - `mcp_tool_call` - assistant calling an MCP tool. - `web_search` - assistant performing a web search. -Typically, an `assistant_message` is added at the end of the turn. +Typically, an `agent_message` is added at the end of the turn. Sample output: ```jsonl {"type":"thread.started","thread_id":"0199a213-81c0-7800-8aa1-bbab2a035a53"} {"type":"turn.started"} -{"type":"item.completed","item":{"id":"item_0","item_type":"reasoning","text":"**Searching for README files**"}} -{"type":"item.started","item":{"id":"item_1","item_type":"command_execution","command":"bash -lc ls","aggregated_output":"","status":"in_progress"}} -{"type":"item.completed","item":{"id":"item_1","item_type":"command_execution","command":"bash -lc ls","aggregated_output":"2025-09-11\nAGENTS.md\nCHANGELOG.md\ncliff.toml\ncodex-cli\ncodex-rs\ndocs\nexamples\nflake.lock\nflake.nix\nLICENSE\nnode_modules\nNOTICE\npackage.json\npnpm-lock.yaml\npnpm-workspace.yaml\nPNPM.md\nREADME.md\nscripts\nsdk\ntmp\n","exit_code":0,"status":"completed"}} -{"type":"item.completed","item":{"id":"item_2","item_type":"reasoning","text":"**Checking repository root for README**"}} -{"type":"item.completed","item":{"id":"item_3","item_type":"assistant_message","text":"Yep — there’s a `README.md` in the repository root."}} +{"type":"item.completed","item":{"id":"item_0","type":"reasoning","text":"**Searching for README files**"}} +{"type":"item.started","item":{"id":"item_1","type":"command_execution","command":"bash -lc ls","aggregated_output":"","status":"in_progress"}} +{"type":"item.completed","item":{"id":"item_1","type":"command_execution","command":"bash -lc ls","aggregated_output":"2025-09-11\nAGENTS.md\nCHANGELOG.md\ncliff.toml\ncodex-cli\ncodex-rs\ndocs\nexamples\nflake.lock\nflake.nix\nLICENSE\nnode_modules\nNOTICE\npackage.json\npnpm-lock.yaml\npnpm-workspace.yaml\nPNPM.md\nREADME.md\nscripts\nsdk\ntmp\n","exit_code":0,"status":"completed"}} +{"type":"item.completed","item":{"id":"item_2","type":"reasoning","text":"**Checking repository root for README**"}} +{"type":"item.completed","item":{"id":"item_3","type":"agent_message","text":"Yep — there’s a `README.md` in the repository root."}} {"type":"turn.completed","usage":{"input_tokens":24763,"cached_input_tokens":24448,"output_tokens":122}} ``` From 7e3a272b29559faabd689bc00a1b97045c745cd5 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 6 Oct 2025 10:39:26 -0700 Subject: [PATCH 03/24] Add a longer message to issue deduplicator and some logs (#4836) Logs are to diagnose why we're not filtering correctly. --- .github/workflows/issue-deduplicator.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/issue-deduplicator.yml b/.github/workflows/issue-deduplicator.yml index eed4d46167..2bac731e61 100644 --- a/.github/workflows/issue-deduplicator.yml +++ b/.github/workflows/issue-deduplicator.yml @@ -105,6 +105,10 @@ jobs: const issues = Array.isArray(parsed?.issues) ? parsed.issues : []; const currentIssueNumber = String(context.payload.issue.number); + + console.log(`Current issue number: ${currentIssueNumber}`); + console.log(issues); + const filteredIssues = issues.filter((value) => String(value) !== currentIssueNumber); if (filteredIssues.length === 0) { @@ -113,7 +117,8 @@ jobs: } const lines = [ - 'Potential duplicates detected:', + 'Potential duplicates detected. Please review them and close your issue if it is a duplicate.', + '', ...filteredIssues.map((value) => `- #${String(value)}`), '', '*Powered by [Codex Action](https://github.com/openai/codex-action)*']; From d73055c5b15f820590ccd7846c0ad9cc897e52dc Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 6 Oct 2025 14:41:16 -0700 Subject: [PATCH 04/24] [MCP] Fix the bearer token authorization header (#4846) `http_config.auth_header` automatically added `Bearer `. By adding it ourselves, we were sending `Bearer Bearer `. I confirmed that the GitHub MCP initialization 400s before and works now. I also optimized the oauth flow to not check the keyring if you explicitly pass in a bearer token. --- codex-rs/rmcp-client/src/rmcp_client.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index d19bf8fe72..676af9a5dd 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -120,14 +120,17 @@ impl RmcpClient { url: &str, bearer_token: Option, ) -> Result { - let initial_tokens = match load_oauth_tokens(server_name, url) { - Ok(tokens) => tokens, - Err(err) => { - warn!("failed to read tokens for server `{server_name}`: {err}"); - None - } + let initial_oauth_tokens = match bearer_token { + Some(_) => None, + None => match load_oauth_tokens(server_name, url) { + Ok(tokens) => tokens, + Err(err) => { + warn!("failed to read tokens for server `{server_name}`: {err}"); + None + } + }, }; - let transport = if let Some(initial_tokens) = initial_tokens.clone() { + let transport = if let Some(initial_tokens) = initial_oauth_tokens.clone() { let (transport, oauth_persistor) = create_oauth_transport_and_runtime(server_name, url, initial_tokens).await?; PendingTransport::StreamableHttpWithOAuth { @@ -137,7 +140,7 @@ impl RmcpClient { } else { let mut http_config = StreamableHttpClientTransportConfig::with_uri(url.to_string()); if let Some(bearer_token) = bearer_token { - http_config = http_config.auth_header(format!("Bearer {bearer_token}")); + http_config = http_config.auth_header(bearer_token); } let transport = StreamableHttpClientTransport::from_config(http_config); From 5833508a17c0738f9a14f45585a1d3eed0eade4d Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Mon, 6 Oct 2025 16:07:22 -0700 Subject: [PATCH 05/24] print codex resume note when quitting after codex resume (#4695) when exiting a session that was started with `codex resume`, the note about how to resume again wasn't being printed. thanks @aibrahim-oai for pointing out this issue! --- codex-rs/cli/src/main.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 04a7ff88f3..7e0036a57c 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -294,7 +294,8 @@ async fn cli_main(codex_linux_sandbox_exe: Option) -> anyhow::Result<() last, config_overrides, ); - codex_tui::run_main(interactive, codex_linux_sandbox_exe).await?; + let exit_info = codex_tui::run_main(interactive, codex_linux_sandbox_exe).await?; + print_exit_messages(exit_info); } Some(Subcommand::Login(mut login_cli)) => { prepend_config_flags( From b09f62a1c365dc75b6d663523bd89f54f24f2dd2 Mon Sep 17 00:00:00 2001 From: Colin Young Date: Mon, 6 Oct 2025 18:59:37 -0700 Subject: [PATCH 06/24] [Codex] Use Number instead of BigInt for TokenCountEvent (#4856) Adjust to use typescript number so reduce casting and normalizing code for VSCE since js supports up to 2^53-1 --- codex-rs/protocol/src/protocol.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index bdd1d3e241..3b6520be67 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -548,10 +548,15 @@ pub struct TaskStartedEvent { #[derive(Debug, Clone, Deserialize, Serialize, Default, TS)] pub struct TokenUsage { + #[ts(type = "number")] pub input_tokens: u64, + #[ts(type = "number")] pub cached_input_tokens: u64, + #[ts(type = "number")] pub output_tokens: u64, + #[ts(type = "number")] pub reasoning_output_tokens: u64, + #[ts(type = "number")] pub total_tokens: u64, } @@ -559,6 +564,7 @@ pub struct TokenUsage { pub struct TokenUsageInfo { pub total_token_usage: TokenUsage, pub last_token_usage: TokenUsage, + #[ts(type = "number | null")] pub model_context_window: Option, } @@ -634,8 +640,10 @@ pub struct RateLimitWindow { /// Percentage (0-100) of the window that has been consumed. pub used_percent: f64, /// Rolling window duration, in minutes. + #[ts(type = "number | null")] pub window_minutes: Option, /// Seconds until the window resets. + #[ts(type = "number | null")] pub resets_in_seconds: Option, } From 35a770e871353182fb07f268d732233f6aeb02f2 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 7 Oct 2025 01:56:39 -0700 Subject: [PATCH 07/24] Simplify request body assertions (#4845) We'll have a lot more test like these --- AGENTS.md | 25 +++ codex-rs/core/tests/common/responses.rs | 141 ++++++++++++--- codex-rs/core/tests/suite/cli_stream.rs | 20 +-- codex-rs/core/tests/suite/client.rs | 52 ++---- codex-rs/core/tests/suite/model_tools.rs | 24 +-- codex-rs/core/tests/suite/read_file.rs | 30 +--- codex-rs/core/tests/suite/tool_harness.rs | 101 +++-------- codex-rs/core/tests/suite/tools.rs | 191 ++++++++------------- codex-rs/core/tests/suite/view_image.rs | 83 ++------- codex-rs/exec/tests/suite/output_schema.rs | 10 +- 10 files changed, 285 insertions(+), 392 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index accfe447f7..832f1d6543 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,3 +73,28 @@ If you don’t have the tool: ### Test assertions - Tests should use pretty_assertions::assert_eq for clearer diffs. Import this at the top of the test module if it isn't already. + +### Integration tests (core) + +- Prefer the utilities in `core_test_support::responses` when writing end-to-end Codex tests. + +- All `mount_sse*` helpers return a `ResponseMock`; hold onto it so you can assert against outbound `/responses` POST bodies. +- Use `ResponseMock::single_request()` when a test should only issue one POST, or `ResponseMock::requests()` to inspect every captured `ResponsesRequest`. +- `ResponsesRequest` exposes helpers (`body_json`, `input`, `function_call_output`, `custom_tool_call_output`, `call_output`, `header`, `path`, `query_param`) so assertions can target structured payloads instead of manual JSON digging. +- Build SSE payloads with the provided `ev_*` constructors and the `sse(...)`. + +- Typical pattern: + + ```rust + let mock = responses::mount_sse_once(&server, responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), + responses::ev_completed("resp-1"), + ])).await; + + codex.submit(Op::UserTurn { ... }).await?; + + // Assert request body if needed. + let request = mock.single_request(); + // assert using request.function_call_output(call_id) or request.json_body() or other helpers. + ``` diff --git a/codex-rs/core/tests/common/responses.rs b/codex-rs/core/tests/common/responses.rs index 24ea824ac6..98b3eca1f4 100644 --- a/codex-rs/core/tests/common/responses.rs +++ b/codex-rs/core/tests/common/responses.rs @@ -1,11 +1,105 @@ +use std::sync::Arc; +use std::sync::Mutex; + use serde_json::Value; use wiremock::BodyPrintLimit; +use wiremock::Match; use wiremock::Mock; +use wiremock::MockBuilder; use wiremock::MockServer; use wiremock::Respond; use wiremock::ResponseTemplate; use wiremock::matchers::method; -use wiremock::matchers::path; +use wiremock::matchers::path_regex; + +#[derive(Debug, Clone)] +pub struct ResponseMock { + requests: Arc>>, +} + +impl ResponseMock { + fn new() -> Self { + Self { + requests: Arc::new(Mutex::new(Vec::new())), + } + } + + pub fn single_request(&self) -> ResponsesRequest { + let requests = self.requests.lock().unwrap(); + if requests.len() != 1 { + panic!("expected 1 request, got {}", requests.len()); + } + requests.first().unwrap().clone() + } + + pub fn requests(&self) -> Vec { + self.requests.lock().unwrap().clone() + } +} + +#[derive(Debug, Clone)] +pub struct ResponsesRequest(wiremock::Request); + +impl ResponsesRequest { + pub fn body_json(&self) -> Value { + self.0.body_json().unwrap() + } + + pub fn input(&self) -> Vec { + self.0.body_json::().unwrap()["input"] + .as_array() + .expect("input array not found in request") + .clone() + } + + pub fn function_call_output(&self, call_id: &str) -> Value { + self.call_output(call_id, "function_call_output") + } + + pub fn custom_tool_call_output(&self, call_id: &str) -> Value { + self.call_output(call_id, "custom_tool_call_output") + } + + pub fn call_output(&self, call_id: &str, call_type: &str) -> Value { + self.input() + .iter() + .find(|item| { + item.get("type").unwrap() == call_type && item.get("call_id").unwrap() == call_id + }) + .cloned() + .unwrap_or_else(|| panic!("function call output {call_id} item not found in request")) + } + + pub fn header(&self, name: &str) -> Option { + self.0 + .headers + .get(name) + .and_then(|v| v.to_str().ok()) + .map(str::to_string) + } + + pub fn path(&self) -> String { + self.0.url.path().to_string() + } + + pub fn query_param(&self, name: &str) -> Option { + self.0 + .url + .query_pairs() + .find(|(k, _)| k == name) + .map(|(_, v)| v.to_string()) + } +} + +impl Match for ResponseMock { + fn matches(&self, request: &wiremock::Request) -> bool { + self.requests + .lock() + .unwrap() + .push(ResponsesRequest(request.clone())); + true + } +} /// Build an SSE stream body from a list of JSON events. pub fn sse(events: Vec) -> String { @@ -161,34 +255,40 @@ pub fn sse_response(body: String) -> ResponseTemplate { .set_body_raw(body, "text/event-stream") } -pub async fn mount_sse_once_match(server: &MockServer, matcher: M, body: String) +fn base_mock() -> (MockBuilder, ResponseMock) { + let response_mock = ResponseMock::new(); + let mock = Mock::given(method("POST")) + .and(path_regex(".*/responses$")) + .and(response_mock.clone()); + (mock, response_mock) +} + +pub async fn mount_sse_once_match(server: &MockServer, matcher: M, body: String) -> ResponseMock where M: wiremock::Match + Send + Sync + 'static, { - Mock::given(method("POST")) - .and(path("/v1/responses")) - .and(matcher) + let (mock, response_mock) = base_mock(); + mock.and(matcher) .respond_with(sse_response(body)) .up_to_n_times(1) .mount(server) .await; + response_mock } -pub async fn mount_sse_once(server: &MockServer, body: String) { - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(sse_response(body)) - .expect(1) +pub async fn mount_sse_once(server: &MockServer, body: String) -> ResponseMock { + let (mock, response_mock) = base_mock(); + mock.respond_with(sse_response(body)) + .up_to_n_times(1) .mount(server) .await; + response_mock } -pub async fn mount_sse(server: &MockServer, body: String) { - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(sse_response(body)) - .mount(server) - .await; +pub async fn mount_sse(server: &MockServer, body: String) -> ResponseMock { + let (mock, response_mock) = base_mock(); + mock.respond_with(sse_response(body)).mount(server).await; + response_mock } pub async fn start_mock_server() -> MockServer { @@ -201,7 +301,7 @@ pub async fn start_mock_server() -> MockServer { /// Mounts a sequence of SSE response bodies and serves them in order for each /// POST to `/v1/responses`. Panics if more requests are received than bodies /// provided. Also asserts the exact number of expected calls. -pub async fn mount_sse_sequence(server: &MockServer, bodies: Vec) { +pub async fn mount_sse_sequence(server: &MockServer, bodies: Vec) -> ResponseMock { use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -228,10 +328,11 @@ pub async fn mount_sse_sequence(server: &MockServer, bodies: Vec) { responses: bodies, }; - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(responder) + let (mock, response_mock) = base_mock(); + mock.respond_with(responder) .expect(num_calls as u64) .mount(server) .await; + + response_mock } diff --git a/codex-rs/core/tests/suite/cli_stream.rs b/codex-rs/core/tests/suite/cli_stream.rs index 8fc36772ad..f9408d5a9c 100644 --- a/codex-rs/core/tests/suite/cli_stream.rs +++ b/codex-rs/core/tests/suite/cli_stream.rs @@ -106,16 +106,12 @@ async fn exec_cli_applies_experimental_instructions_file() { "data: {\"type\":\"response.created\",\"response\":{}}\n\n", "data: {\"type\":\"response.completed\",\"response\":{\"id\":\"r1\"}}\n\n" ); - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with( - ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse, "text/event-stream"), - ) - .expect(1) - .mount(&server) - .await; + let resp_mock = core_test_support::responses::mount_sse_once_match( + &server, + path("/v1/responses"), + sse.to_string(), + ) + .await; // Create a temporary instructions file with a unique marker we can assert // appears in the outbound request payload. @@ -164,8 +160,8 @@ async fn exec_cli_applies_experimental_instructions_file() { // Inspect the captured request and verify our custom base instructions were // included in the `instructions` field. - let request = &server.received_requests().await.unwrap()[0]; - let body = request.body_json::().unwrap(); + let request = resp_mock.single_request(); + let body = request.body_json(); let instructions = body .get("instructions") .and_then(|v| v.as_str()) diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index c49c38e30a..eb14dabb81 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -223,15 +223,9 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { // Mock server that will receive the resumed request let server = MockServer::start().await; - let first = ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse_completed("resp1"), "text/event-stream"); - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(first) - .expect(1) - .mount(&server) - .await; + let resp_mock = + responses::mount_sse_once_match(&server, path("/v1/responses"), sse_completed("resp1")) + .await; // Configure Codex to resume from our file let model_provider = ModelProviderInfo { @@ -277,8 +271,8 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { .unwrap(); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - let request = &server.received_requests().await.unwrap()[0]; - let request_body = request.body_json::().unwrap(); + let request = resp_mock.single_request(); + let request_body = request.body_json(); let expected_input = json!([ { "type": "message", @@ -372,18 +366,9 @@ async fn includes_base_instructions_override_in_request() { skip_if_no_network!(); // Mock server let server = MockServer::start().await; - - // First request – must NOT include `previous_response_id`. - let first = ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse_completed("resp1"), "text/event-stream"); - - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(first) - .expect(1) - .mount(&server) - .await; + let resp_mock = + responses::mount_sse_once_match(&server, path("/v1/responses"), sse_completed("resp1")) + .await; let model_provider = ModelProviderInfo { base_url: Some(format!("{}/v1", server.uri())), @@ -414,8 +399,8 @@ async fn includes_base_instructions_override_in_request() { wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - let request = &server.received_requests().await.unwrap()[0]; - let request_body = request.body_json::().unwrap(); + let request = resp_mock.single_request(); + let request_body = request.body_json(); assert!( request_body["instructions"] @@ -570,16 +555,9 @@ async fn includes_user_instructions_message_in_request() { skip_if_no_network!(); let server = MockServer::start().await; - let first = ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse_completed("resp1"), "text/event-stream"); - - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(first) - .expect(1) - .mount(&server) - .await; + let resp_mock = + responses::mount_sse_once_match(&server, path("/v1/responses"), sse_completed("resp1")) + .await; let model_provider = ModelProviderInfo { base_url: Some(format!("{}/v1", server.uri())), @@ -610,8 +588,8 @@ async fn includes_user_instructions_message_in_request() { wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - let request = &server.received_requests().await.unwrap()[0]; - let request_body = request.body_json::().unwrap(); + let request = resp_mock.single_request(); + let request_body = request.body_json(); assert!( !request_body["instructions"] diff --git a/codex-rs/core/tests/suite/model_tools.rs b/codex-rs/core/tests/suite/model_tools.rs index 6a7c576266..ee7b44d4b0 100644 --- a/codex-rs/core/tests/suite/model_tools.rs +++ b/codex-rs/core/tests/suite/model_tools.rs @@ -10,14 +10,11 @@ use codex_core::protocol::InputItem; use codex_core::protocol::Op; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id; +use core_test_support::responses; use core_test_support::skip_if_no_network; use core_test_support::wait_for_event; use tempfile::TempDir; -use wiremock::Mock; use wiremock::MockServer; -use wiremock::ResponseTemplate; -use wiremock::matchers::method; -use wiremock::matchers::path; fn sse_completed(id: &str) -> String { load_sse_fixture_with_id("tests/fixtures/completed_template.json", id) @@ -44,16 +41,7 @@ async fn collect_tool_identifiers_for_model(model: &str) -> Vec { let server = MockServer::start().await; let sse = sse_completed(model); - let template = ResponseTemplate::new(200) - .insert_header("content-type", "text/event-stream") - .set_body_raw(sse, "text/event-stream"); - - Mock::given(method("POST")) - .and(path("/v1/responses")) - .respond_with(template) - .expect(1) - .mount(&server) - .await; + let resp_mock = responses::mount_sse_once_match(&server, wiremock::matchers::any(), sse).await; let model_provider = ModelProviderInfo { base_url: Some(format!("{}/v1", server.uri())), @@ -93,13 +81,7 @@ async fn collect_tool_identifiers_for_model(model: &str) -> Vec { .unwrap(); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.unwrap(); - assert_eq!( - requests.len(), - 1, - "expected a single request for model {model}" - ); - let body = requests[0].body_json::().unwrap(); + let body = resp_mock.single_request().body_json(); tool_identifiers(&body) } diff --git a/codex-rs/core/tests/suite/read_file.rs b/codex-rs/core/tests/suite/read_file.rs index a6c8a7a181..fc5a94f91b 100644 --- a/codex-rs/core/tests/suite/read_file.rs +++ b/codex-rs/core/tests/suite/read_file.rs @@ -58,7 +58,7 @@ async fn read_file_tool_returns_requested_lines() -> anyhow::Result<()> { ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -79,36 +79,12 @@ async fn read_file_tool_returns_requested_lines() -> anyhow::Result<()> { wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.expect("recorded requests"); - let request_bodies = requests - .iter() - .map(|req| req.body_json::().unwrap()) - .collect::>(); - assert!( - !request_bodies.is_empty(), - "expected at least one request body" - ); - - let tool_output_item = request_bodies - .iter() - .find_map(|body| { - body.get("input") - .and_then(Value::as_array) - .and_then(|items| { - items.iter().find(|item| { - item.get("type").and_then(Value::as_str) == Some("function_call_output") - }) - }) - }) - .unwrap_or_else(|| { - panic!("function_call_output item not found in requests: {request_bodies:#?}") - }); - + let req = second_mock.single_request(); + let tool_output_item = req.function_call_output(call_id); assert_eq!( tool_output_item.get("call_id").and_then(Value::as_str), Some(call_id) ); - let output_text = tool_output_item .get("output") .and_then(|value| match value { diff --git a/codex-rs/core/tests/suite/tool_harness.rs b/codex-rs/core/tests/suite/tool_harness.rs index 14e0e1c892..eaefe7d9dc 100644 --- a/codex-rs/core/tests/suite/tool_harness.rs +++ b/codex-rs/core/tests/suite/tool_harness.rs @@ -27,16 +27,6 @@ use serde_json::Value; use serde_json::json; use wiremock::matchers::any; -fn function_call_output(body: &Value) -> Option<&Value> { - body.get("input") - .and_then(Value::as_array) - .and_then(|items| { - items.iter().find(|item| { - item.get("type").and_then(Value::as_str) == Some("function_call_output") - }) - }) -} - fn extract_output_text(item: &Value) -> Option<&str> { item.get("output").and_then(|value| match value { Value::String(text) => Some(text.as_str()), @@ -45,12 +35,6 @@ fn extract_output_text(item: &Value) -> Option<&str> { }) } -fn find_request_with_function_call_output(requests: &[Value]) -> Option<&Value> { - requests - .iter() - .find(|body| function_call_output(body).is_some()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_tool_executes_command_and_streams_output() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -81,7 +65,7 @@ async fn shell_tool_executes_command_and_streams_output() -> anyhow::Result<()> ev_assistant_message("msg-1", "all done"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -102,18 +86,9 @@ async fn shell_tool_executes_command_and_streams_output() -> anyhow::Result<()> wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.expect("recorded requests"); - assert!(!requests.is_empty(), "expected at least one POST request"); - - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); - let output_text = extract_output_text(output_item).expect("output text present"); + let req = second_mock.single_request(); + let output_item = req.function_call_output(call_id); + let output_text = extract_output_text(&output_item).expect("output text present"); let exec_output: Value = serde_json::from_str(output_text)?; assert_eq!(exec_output["metadata"]["exit_code"], 0); let stdout = exec_output["output"].as_str().expect("stdout field"); @@ -159,7 +134,7 @@ async fn update_plan_tool_emits_plan_update_event() -> anyhow::Result<()> { ev_assistant_message("msg-1", "plan acknowledged"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -197,22 +172,13 @@ async fn update_plan_tool_emits_plan_update_event() -> anyhow::Result<()> { assert!(saw_plan_update, "expected PlanUpdate event"); - let requests = server.received_requests().await.expect("recorded requests"); - assert!(!requests.is_empty(), "expected at least one POST request"); - - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); + let req = second_mock.single_request(); + let output_item = req.function_call_output(call_id); assert_eq!( output_item.get("call_id").and_then(Value::as_str), Some(call_id) ); - let output_text = extract_output_text(output_item).expect("output text present"); + let output_text = extract_output_text(&output_item).expect("output text present"); assert_eq!(output_text, "Plan updated"); Ok(()) @@ -251,7 +217,7 @@ async fn update_plan_tool_rejects_malformed_payload() -> anyhow::Result<()> { ev_assistant_message("msg-1", "malformed plan payload"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -286,22 +252,13 @@ async fn update_plan_tool_rejects_malformed_payload() -> anyhow::Result<()> { "did not expect PlanUpdate event for malformed payload" ); - let requests = server.received_requests().await.expect("recorded requests"); - assert!(!requests.is_empty(), "expected at least one POST request"); - - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); + let req = second_mock.single_request(); + let output_item = req.function_call_output(call_id); assert_eq!( output_item.get("call_id").and_then(Value::as_str), Some(call_id) ); - let output_text = extract_output_text(output_item).expect("output text present"); + let output_text = extract_output_text(&output_item).expect("output text present"); assert!( output_text.contains("failed to parse function arguments"), "expected parse error message in output text, got {output_text:?}" @@ -354,7 +311,7 @@ async fn apply_patch_tool_executes_and_emits_patch_events() -> anyhow::Result<() ev_assistant_message("msg-1", "patch complete"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -395,22 +352,13 @@ async fn apply_patch_tool_executes_and_emits_patch_events() -> anyhow::Result<() let patch_end_success = patch_end_success.expect("expected PatchApplyEnd event to capture success flag"); - let requests = server.received_requests().await.expect("recorded requests"); - assert!(!requests.is_empty(), "expected at least one POST request"); - - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); + let req = second_mock.single_request(); + let output_item = req.function_call_output(call_id); assert_eq!( output_item.get("call_id").and_then(Value::as_str), Some(call_id) ); - let output_text = extract_output_text(output_item).expect("output text present"); + let output_text = extract_output_text(&output_item).expect("output text present"); if let Ok(exec_output) = serde_json::from_str::(output_text) { let exit_code = exec_output["metadata"]["exit_code"] @@ -480,7 +428,7 @@ async fn apply_patch_reports_parse_diagnostics() -> anyhow::Result<()> { ev_assistant_message("msg-1", "failed"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let second_mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -501,22 +449,13 @@ async fn apply_patch_reports_parse_diagnostics() -> anyhow::Result<()> { wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.expect("recorded requests"); - assert!(!requests.is_empty(), "expected at least one POST request"); - - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); + let req = second_mock.single_request(); + let output_item = req.function_call_output(call_id); assert_eq!( output_item.get("call_id").and_then(Value::as_str), Some(call_id) ); - let output_text = extract_output_text(output_item).expect("output text present"); + let output_text = extract_output_text(&output_item).expect("output text present"); assert!( output_text.contains("apply_patch verification failed"), diff --git a/codex-rs/core/tests/suite/tools.rs b/codex-rs/core/tests/suite/tools.rs index 08826a1a72..27e709f2c3 100644 --- a/codex-rs/core/tests/suite/tools.rs +++ b/codex-rs/core/tests/suite/tools.rs @@ -15,6 +15,7 @@ use core_test_support::responses::ev_completed; use core_test_support::responses::ev_custom_tool_call; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; @@ -25,7 +26,6 @@ use core_test_support::wait_for_event; use regex_lite::Regex; use serde_json::Value; use serde_json::json; -use wiremock::Request; async fn submit_turn( test: &TestCodex, @@ -58,27 +58,6 @@ async fn submit_turn( Ok(()) } -fn request_bodies(requests: &[Request]) -> Result> { - requests - .iter() - .map(|req| Ok(serde_json::from_slice::(&req.body)?)) - .collect() -} - -fn collect_output_items<'a>(bodies: &'a [Value], ty: &str) -> Vec<&'a Value> { - let mut out = Vec::new(); - for body in bodies { - if let Some(items) = body.get("input").and_then(Value::as_array) { - for item in items { - if item.get("type").and_then(Value::as_str) == Some(ty) { - out.push(item); - } - } - } - } - out -} - fn tool_names(body: &Value) -> Vec { body.get("tools") .and_then(Value::as_array) @@ -107,18 +86,23 @@ async fn custom_tool_unknown_returns_custom_output_error() -> Result<()> { let call_id = "custom-unsupported"; let tool_name = "unsupported_tool"; - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), ev_custom_tool_call(call_id, tool_name, "\"payload\""), ev_completed("resp-1"), ]), + ) + .await; + let mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -128,13 +112,7 @@ async fn custom_tool_unknown_returns_custom_output_error() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let custom_items = collect_output_items(&bodies, "custom_tool_call_output"); - assert_eq!(custom_items.len(), 1, "expected single custom tool output"); - let item = custom_items[0]; - assert_eq!(item.get("call_id").and_then(Value::as_str), Some(call_id)); - + let item = mock.single_request().custom_tool_call_output(call_id); let output = item .get("output") .and_then(Value::as_str) @@ -170,7 +148,8 @@ async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { "timeout_ms": 1_000, }); - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), ev_function_call( @@ -180,6 +159,10 @@ async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { ), ev_completed("resp-1"), ]), + ) + .await; + let second_mock = mount_sse_once( + &server, sse(vec![ ev_response_created("resp-2"), ev_function_call( @@ -189,12 +172,16 @@ async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { ), ev_completed("resp-2"), ]), + ) + .await; + let third_mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-3"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -204,46 +191,23 @@ async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let function_outputs = collect_output_items(&bodies, "function_call_output"); - for item in &function_outputs { - let call_id = item - .get("call_id") - .and_then(Value::as_str) - .unwrap_or_default(); - assert!( - call_id == call_id_blocked || call_id == call_id_success, - "unexpected call id {call_id}" - ); - } - let policy = AskForApproval::Never; let expected_message = format!( "approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}" ); - let blocked_outputs: Vec<&Value> = function_outputs - .iter() - .filter(|item| item.get("call_id").and_then(Value::as_str) == Some(call_id_blocked)) - .copied() - .collect(); - assert!( - !blocked_outputs.is_empty(), - "expected at least one rejection output for {call_id_blocked}" + let blocked_item = second_mock + .single_request() + .function_call_output(call_id_blocked); + assert_eq!( + blocked_item.get("output").and_then(Value::as_str), + Some(expected_message.as_str()), + "unexpected rejection message" ); - for item in blocked_outputs { - assert_eq!( - item.get("output").and_then(Value::as_str), - Some(expected_message.as_str()), - "unexpected rejection message" - ); - } - let success_item = function_outputs - .iter() - .find(|item| item.get("call_id").and_then(Value::as_str) == Some(call_id_success)) - .expect("success output present"); + let success_item = third_mock + .single_request() + .function_call_output(call_id_success); let output_json: Value = serde_json::from_str( success_item .get("output") @@ -282,18 +246,23 @@ async fn local_shell_missing_ids_maps_to_function_output_error() -> Result<()> { } }); - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), local_shell_event, ev_completed("resp-1"), ]), + ) + .await; + let second_mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -303,15 +272,7 @@ async fn local_shell_missing_ids_maps_to_function_output_error() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let function_outputs = collect_output_items(&bodies, "function_call_output"); - assert_eq!( - function_outputs.len(), - 1, - "expected a single function output" - ); - let item = function_outputs[0]; + let item = second_mock.single_request().function_call_output(""); assert_eq!(item.get("call_id").and_then(Value::as_str), Some("")); assert_eq!( item.get("output").and_then(Value::as_str), @@ -329,7 +290,7 @@ async fn collect_tools(use_unified_exec: bool) -> Result> { ev_assistant_message("msg-1", "done"), ev_completed("resp-1"), ])]; - mount_sse_sequence(&server, responses).await; + let mock = mount_sse_sequence(&server, responses).await; let mut builder = test_codex().with_config(move |config| { config.use_experimental_unified_exec_tool = use_unified_exec; @@ -344,15 +305,8 @@ async fn collect_tools(use_unified_exec: bool) -> Result> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - assert_eq!( - requests.len(), - 1, - "expected a single request for tools collection" - ); - let bodies = request_bodies(&requests)?; - let first_body = bodies.first().expect("request body present"); - Ok(tool_names(first_body)) + let first_body = mock.single_request().body_json(); + Ok(tool_names(&first_body)) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -392,18 +346,23 @@ async fn shell_timeout_includes_timeout_prefix_and_metadata() -> Result<()> { "timeout_ms": timeout_ms, }); - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), ev_completed("resp-1"), ]), + ) + .await; + let second_mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -413,13 +372,7 @@ async fn shell_timeout_includes_timeout_prefix_and_metadata() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let function_outputs = collect_output_items(&bodies, "function_call_output"); - let timeout_item = function_outputs - .iter() - .find(|item| item.get("call_id").and_then(Value::as_str) == Some(call_id)) - .expect("timeout output present"); + let timeout_item = second_mock.single_request().function_call_output(call_id); let output_str = timeout_item .get("output") @@ -478,18 +431,23 @@ async fn shell_sandbox_denied_truncates_error_output() -> Result<()> { "timeout_ms": 1_000, }); - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), ev_completed("resp-1"), ]), + ) + .await; + let second_mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -499,13 +457,7 @@ async fn shell_sandbox_denied_truncates_error_output() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let function_outputs = collect_output_items(&bodies, "function_call_output"); - let denied_item = function_outputs - .iter() - .find(|item| item.get("call_id").and_then(Value::as_str) == Some(call_id)) - .expect("denied output present"); + let denied_item = second_mock.single_request().function_call_output(call_id); let output = denied_item .get("output") @@ -558,18 +510,23 @@ async fn shell_spawn_failure_truncates_exec_error() -> Result<()> { "timeout_ms": 1_000, }); - let responses = vec![ + mount_sse_once( + &server, sse(vec![ ev_response_created("resp-1"), ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), ev_completed("resp-1"), ]), + ) + .await; + let second_mock = mount_sse_once( + &server, sse(vec![ ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]), - ]; - mount_sse_sequence(&server, responses).await; + ) + .await; submit_turn( &test, @@ -579,13 +536,7 @@ async fn shell_spawn_failure_truncates_exec_error() -> Result<()> { ) .await?; - let requests = server.received_requests().await.expect("recorded requests"); - let bodies = request_bodies(&requests)?; - let function_outputs = collect_output_items(&bodies, "function_call_output"); - let failure_item = function_outputs - .iter() - .find(|item| item.get("call_id").and_then(Value::as_str) == Some(call_id)) - .expect("spawn failure output present"); + let failure_item = second_mock.single_request().function_call_output(call_id); let output = failure_item .get("output") diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 16913f4f93..bdb67ad631 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -22,16 +22,6 @@ use core_test_support::wait_for_event; use serde_json::Value; use wiremock::matchers::any; -fn function_call_output(body: &Value) -> Option<&Value> { - body.get("input") - .and_then(Value::as_array) - .and_then(|items| { - items.iter().find(|item| { - item.get("type").and_then(Value::as_str) == Some("function_call_output") - }) - }) -} - fn find_image_message(body: &Value) -> Option<&Value> { body.get("input") .and_then(Value::as_array) @@ -59,12 +49,6 @@ fn extract_output_text(item: &Value) -> Option<&str> { }) } -fn find_request_with_function_call_output(requests: &[Value]) -> Option<&Value> { - requests - .iter() - .find(|body| function_call_output(body).is_some()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -100,7 +84,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -137,25 +121,14 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { assert_eq!(tool_event.call_id, call_id); assert_eq!(tool_event.path, abs_path); - let requests = server.received_requests().await.expect("recorded requests"); - assert!( - requests.len() >= 2, - "expected at least two POST requests, got {}", - requests.len() - ); - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); + let body = mock.single_request().body_json(); + let output_item = mock.single_request().function_call_output(call_id); - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); - let output_text = extract_output_text(output_item).expect("output text present"); + let output_text = extract_output_text(&output_item).expect("output text present"); assert_eq!(output_text, "attached local image path"); - let image_message = find_image_message(body_with_tool_output) - .expect("pending input image message not included in request"); + let image_message = + find_image_message(&body).expect("pending input image message not included in request"); let image_url = image_message .get("content") .and_then(Value::as_array) @@ -210,7 +183,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -231,26 +204,14 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.expect("recorded requests"); - assert!( - requests.len() >= 2, - "expected at least two POST requests, got {}", - requests.len() - ); - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); - let output_text = extract_output_text(output_item).expect("output text present"); + let body_with_tool_output = mock.single_request().body_json(); + let output_item = mock.single_request().function_call_output(call_id); + let output_text = extract_output_text(&output_item).expect("output text present"); let expected_message = format!("image path `{}` is not a file", abs_path.display()); assert_eq!(output_text, expected_message); assert!( - find_image_message(body_with_tool_output).is_none(), + find_image_message(&body_with_tool_output).is_none(), "directory path should not produce an input_image message" ); @@ -287,7 +248,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]); - responses::mount_sse_once_match(&server, any(), second_response).await; + let mock = responses::mount_sse_once_match(&server, any(), second_response).await; let session_model = session_configured.model.clone(); @@ -308,21 +269,9 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await; - let requests = server.received_requests().await.expect("recorded requests"); - assert!( - requests.len() >= 2, - "expected at least two POST requests, got {}", - requests.len() - ); - let request_bodies = requests - .iter() - .map(|req| req.body_json::().expect("request json")) - .collect::>(); - - let body_with_tool_output = find_request_with_function_call_output(&request_bodies) - .expect("function_call_output item not found in requests"); - let output_item = function_call_output(body_with_tool_output).expect("tool output item"); - let output_text = extract_output_text(output_item).expect("output text present"); + let body_with_tool_output = mock.single_request().body_json(); + let output_item = mock.single_request().function_call_output(call_id); + let output_text = extract_output_text(&output_item).expect("output text present"); let expected_prefix = format!("unable to locate image at `{}`:", abs_path.display()); assert!( output_text.starts_with(&expected_prefix), @@ -330,7 +279,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { ); assert!( - find_image_message(body_with_tool_output).is_none(), + find_image_message(&body_with_tool_output).is_none(), "missing file should not produce an input_image message" ); diff --git a/codex-rs/exec/tests/suite/output_schema.rs b/codex-rs/exec/tests/suite/output_schema.rs index b054484f7c..913270eff4 100644 --- a/codex-rs/exec/tests/suite/output_schema.rs +++ b/codex-rs/exec/tests/suite/output_schema.rs @@ -28,7 +28,7 @@ async fn exec_includes_output_schema_in_request() -> anyhow::Result<()> { responses::ev_assistant_message("m1", "fixture hello"), responses::ev_completed("resp1"), ]); - responses::mount_sse_once_match(&server, any(), body).await; + let response_mock = responses::mount_sse_once_match(&server, any(), body).await; test.cmd_with_server(&server) .arg("--skip-git-repo-check") @@ -43,12 +43,8 @@ async fn exec_includes_output_schema_in_request() -> anyhow::Result<()> { .assert() .success(); - let requests = server - .received_requests() - .await - .expect("failed to capture requests"); - assert_eq!(requests.len(), 1, "expected exactly one request"); - let payload: Value = serde_json::from_slice(&requests[0].body)?; + let request = response_mock.single_request(); + let payload: Value = request.body_json(); let text = payload.get("text").expect("request missing text field"); let format = text .get("format") From b16c985ed23441f2937cfb576e8aebf4c411ae4e Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 7 Oct 2025 08:07:31 -0700 Subject: [PATCH 08/24] cli: fix zsh completion (#4692) Before this change: ``` tamird@L03G26TD12 codex-rs % codex zsh: do you wish to see all 3864 possibilities (1285 lines)? ``` After this change: ``` tamird@L03G26TD12 codex-rs % codex app-server -- [experimental] Run the app server apply a -- Apply the latest diff produced by Codex agent as a `git apply` to your local working tree cloud -- [EXPERIMENTAL] Browse tasks from Codex Cloud and apply changes locally completion -- Generate shell completion scripts debug -- Internal debugging commands exec e -- Run Codex non-interactively generate-ts -- Internal: generate TypeScript protocol bindings help -- Print this message or the help of the given subcommand(s) login -- Manage login logout -- Remove stored authentication credentials mcp -- [experimental] Run Codex as an MCP server and manage MCP servers mcp-server -- [experimental] Run the Codex MCP server (stdio transport) responses-api-proxy -- Internal: run the responses API proxy resume -- Resume a previous interactive session (picker by default; use --last to continue the most recent) ``` --- codex-rs/exec/src/cli.rs | 4 ++-- codex-rs/tui/src/cli.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index 323cc879fe..d264eb61b0 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -77,7 +77,7 @@ pub struct Cli { /// Initial instructions for the agent. If not provided as an argument (or /// if `-` is used), instructions are read from stdin. - #[arg(value_name = "PROMPT")] + #[arg(value_name = "PROMPT", value_hint = clap::ValueHint::Other)] pub prompt: Option, } @@ -99,7 +99,7 @@ pub struct ResumeArgs { pub last: bool, /// Prompt to send after resuming the session. If `-` is used, read from stdin. - #[arg(value_name = "PROMPT")] + #[arg(value_name = "PROMPT", value_hint = clap::ValueHint::Other)] pub prompt: Option, } diff --git a/codex-rs/tui/src/cli.rs b/codex-rs/tui/src/cli.rs index f0630a34c5..ee2a0355da 100644 --- a/codex-rs/tui/src/cli.rs +++ b/codex-rs/tui/src/cli.rs @@ -7,7 +7,7 @@ use std::path::PathBuf; #[command(version)] pub struct Cli { /// Optional user prompt to start the session. - #[arg(value_name = "PROMPT")] + #[arg(value_name = "PROMPT", value_hint = clap::ValueHint::Other)] pub prompt: Option, /// Optional image(s) to attach to the initial prompt. From 27f169bb914888630cfdb865b5f257c98d565237 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Fri, 3 Oct 2025 16:43:57 -0400 Subject: [PATCH 09/24] cloud-tasks: use workspace deps This seems to be the way. It made life easier when I was locally forking clap. --- codex-rs/cloud-tasks/Cargo.toml | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/codex-rs/cloud-tasks/Cargo.toml b/codex-rs/cloud-tasks/Cargo.toml index 13d7252760..46044fbb8d 100644 --- a/codex-rs/cloud-tasks/Cargo.toml +++ b/codex-rs/cloud-tasks/Cargo.toml @@ -11,10 +11,10 @@ path = "src/lib.rs" workspace = true [dependencies] -anyhow = "1" -base64 = "0.22" -chrono = { version = "0.4", features = ["serde"] } -clap = { version = "4", features = ["derive"] } +anyhow = { workspace = true } +base64 = { workspace = true } +chrono = { workspace = true, features = ["serde"] } +clap = { workspace = true, features = ["derive"] } codex-cloud-tasks-client = { path = "../cloud-tasks-client", features = [ "mock", "online", @@ -23,16 +23,16 @@ codex-common = { path = "../common", features = ["cli"] } codex-core = { path = "../core" } codex-login = { path = "../login" } codex-tui = { path = "../tui" } -crossterm = { version = "0.28.1", features = ["event-stream"] } -ratatui = { version = "0.29.0" } -reqwest = { version = "0.12", features = ["json"] } -serde = { version = "1", features = ["derive"] } -serde_json = "1" -tokio = { version = "1", features = ["macros", "rt-multi-thread"] } -tokio-stream = "0.1.17" -tracing = { version = "0.1.41", features = ["log"] } -tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } -unicode-width = "0.1" +crossterm = { workspace = true, features = ["event-stream"] } +ratatui = { workspace = true } +reqwest = { workspace = true, features = ["json"] } +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } +tokio-stream = { workspace = true } +tracing = { workspace = true, features = ["log"] } +tracing-subscriber = { workspace = true, features = ["env-filter"] } +unicode-width = { workspace = true } [dev-dependencies] -async-trait = "0.1" +async-trait = { workspace = true } From f2555422b91bea8cfb8629d893ebb3999bc30d8a Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 7 Oct 2025 10:12:38 -0700 Subject: [PATCH 10/24] Simplify parallel (#4829) make tool processing return a future and then collect futures. handle cleanup on Drop --- codex-rs/Cargo.lock | 1 + codex-rs/core/Cargo.toml | 2 +- codex-rs/core/src/codex.rs | 50 ++++++------ codex-rs/core/src/tools/parallel.rs | 119 ++++++++-------------------- 4 files changed, 58 insertions(+), 114 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 350773a3c5..1884dc7210 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -6019,6 +6019,7 @@ dependencies = [ "bytes", "futures-core", "futures-sink", + "futures-util", "pin-project-lite", "tokio", ] diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 367ccbcef2..4259e64fc4 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -61,7 +61,7 @@ tokio = { workspace = true, features = [ "rt-multi-thread", "signal", ] } -tokio-util = { workspace = true } +tokio-util = { workspace = true, features = ["rt"] } toml = { workspace = true } toml_edit = { workspace = true } tracing = { workspace = true, features = ["log"] } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 25f93d54d3..7d7bd60ac5 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -23,7 +23,9 @@ use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TaskStartedEvent; use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::TurnContextItem; +use futures::future::BoxFuture; use futures::prelude::*; +use futures::stream::FuturesOrdered; use mcp_types::CallToolResult; use serde_json; use serde_json::Value; @@ -2101,14 +2103,15 @@ async fn try_run_turn( sess.persist_rollout_items(&[rollout_item]).await; let mut stream = turn_context.client.clone().stream(&prompt).await?; - let mut output = Vec::new(); - let mut tool_runtime = ToolCallRuntime::new( + let tool_runtime = ToolCallRuntime::new( Arc::clone(&router), Arc::clone(&sess), Arc::clone(&turn_context), Arc::clone(&turn_diff_tracker), sub_id.to_string(), ); + let mut output: FuturesOrdered>> = + FuturesOrdered::new(); loop { // Poll the next item from the model stream. We must inspect *both* Ok and Err @@ -2116,9 +2119,8 @@ async fn try_run_turn( // `response.completed`) bubble up and trigger the caller's retry logic. let event = stream.next().await; let event = match event { - Some(event) => event, + Some(res) => res?, None => { - tool_runtime.abort_all(); return Err(CodexErr::Stream( "stream closed before response.completed".into(), None, @@ -2126,14 +2128,8 @@ async fn try_run_turn( } }; - let event = match event { - Ok(ev) => ev, - Err(e) => { - tool_runtime.abort_all(); - // Propagate the underlying stream error to the caller (run_turn), which - // will apply the configured `stream_max_retries` policy. - return Err(e); - } + let add_completed = &mut |response_item: ProcessedResponseItem| { + output.push_back(future::ready(Ok(response_item)).boxed()); }; match event { @@ -2143,14 +2139,18 @@ async fn try_run_turn( Ok(Some(call)) => { let payload_preview = call.payload.log_payload().into_owned(); tracing::info!("ToolCall: {} {}", call.tool_name, payload_preview); - let index = output.len(); - output.push(ProcessedResponseItem { - item, - response: None, - }); - tool_runtime - .handle_tool_call(call, index, output.as_mut_slice()) - .await?; + + let response = tool_runtime.handle_tool_call(call); + + output.push_back( + async move { + Ok(ProcessedResponseItem { + item, + response: Some(response.await?), + }) + } + .boxed(), + ); } Ok(None) => { let response = handle_non_tool_response_item( @@ -2160,7 +2160,7 @@ async fn try_run_turn( item.clone(), ) .await?; - output.push(ProcessedResponseItem { item, response }); + add_completed(ProcessedResponseItem { item, response }); } Err(FunctionCallError::MissingLocalShellCallId) => { let msg = "LocalShellCall without call_id or id"; @@ -2177,7 +2177,7 @@ async fn try_run_turn( success: None, }, }; - output.push(ProcessedResponseItem { + add_completed(ProcessedResponseItem { item, response: Some(response), }); @@ -2190,7 +2190,7 @@ async fn try_run_turn( success: None, }, }; - output.push(ProcessedResponseItem { + add_completed(ProcessedResponseItem { item, response: Some(response), }); @@ -2221,7 +2221,7 @@ async fn try_run_turn( sess.update_token_usage_info(sub_id, turn_context.as_ref(), token_usage.as_ref()) .await; - tool_runtime.resolve_pending(output.as_mut_slice()).await?; + let processed_items: Vec = output.try_collect().await?; let unified_diff = { let mut tracker = turn_diff_tracker.lock().await; @@ -2237,7 +2237,7 @@ async fn try_run_turn( } let result = TurnRunResult { - processed_items: output, + processed_items, total_token_usage: token_usage.clone(), }; diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index ff4104d0c1..26dfed8ecc 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -1,6 +1,8 @@ use std::sync::Arc; -use tokio::task::JoinHandle; +use tokio::sync::RwLock; +use tokio_util::either::Either; +use tokio_util::task::AbortOnDropHandle; use crate::codex::Session; use crate::codex::TurnContext; @@ -11,20 +13,13 @@ use crate::tools::router::ToolCall; use crate::tools::router::ToolRouter; use codex_protocol::models::ResponseInputItem; -use crate::codex::ProcessedResponseItem; - -struct PendingToolCall { - index: usize, - handle: JoinHandle>, -} - pub(crate) struct ToolCallRuntime { router: Arc, session: Arc, turn_context: Arc, tracker: SharedTurnDiffTracker, sub_id: String, - pending_calls: Vec, + parallel_execution: Arc>, } impl ToolCallRuntime { @@ -41,97 +36,45 @@ impl ToolCallRuntime { turn_context, tracker, sub_id, - pending_calls: Vec::new(), + parallel_execution: Arc::new(RwLock::new(())), } } - pub(crate) async fn handle_tool_call( - &mut self, + pub(crate) fn handle_tool_call( + &self, call: ToolCall, - output_index: usize, - output: &mut [ProcessedResponseItem], - ) -> Result<(), CodexErr> { + ) -> impl std::future::Future> { let supports_parallel = self.router.tool_supports_parallel(&call.tool_name); - if supports_parallel { - self.spawn_parallel(call, output_index); - } else { - self.resolve_pending(output).await?; - let response = self.dispatch_serial(call).await?; - let slot = output.get_mut(output_index).ok_or_else(|| { - CodexErr::Fatal(format!("tool output index {output_index} out of bounds")) - })?; - slot.response = Some(response); - } - Ok(()) - } - - pub(crate) fn abort_all(&mut self) { - while let Some(pending) = self.pending_calls.pop() { - pending.handle.abort(); - } - } - - pub(crate) async fn resolve_pending( - &mut self, - output: &mut [ProcessedResponseItem], - ) -> Result<(), CodexErr> { - while let Some(PendingToolCall { index, handle }) = self.pending_calls.pop() { - match handle.await { - Ok(Ok(response)) => { - if let Some(slot) = output.get_mut(index) { - slot.response = Some(response); - } - } - Ok(Err(FunctionCallError::Fatal(message))) => { - self.abort_all(); - return Err(CodexErr::Fatal(message)); - } - Ok(Err(other)) => { - self.abort_all(); - return Err(CodexErr::Fatal(other.to_string())); - } - Err(join_err) => { - self.abort_all(); - return Err(CodexErr::Fatal(format!( - "tool task failed to join: {join_err}" - ))); - } - } - } - - Ok(()) - } - - fn spawn_parallel(&mut self, call: ToolCall, index: usize) { let router = Arc::clone(&self.router); let session = Arc::clone(&self.session); let turn = Arc::clone(&self.turn_context); let tracker = Arc::clone(&self.tracker); let sub_id = self.sub_id.clone(); - let handle = tokio::spawn(async move { - router - .dispatch_tool_call(session, turn, tracker, sub_id, call) - .await - }); - self.pending_calls.push(PendingToolCall { index, handle }); - } + let lock = Arc::clone(&self.parallel_execution); - async fn dispatch_serial(&self, call: ToolCall) -> Result { - match self - .router - .dispatch_tool_call( - Arc::clone(&self.session), - Arc::clone(&self.turn_context), - Arc::clone(&self.tracker), - self.sub_id.clone(), - call, - ) - .await - { - Ok(response) => Ok(response), - Err(FunctionCallError::Fatal(message)) => Err(CodexErr::Fatal(message)), - Err(other) => Err(CodexErr::Fatal(other.to_string())), + let handle: AbortOnDropHandle> = + AbortOnDropHandle::new(tokio::spawn(async move { + let _guard = if supports_parallel { + Either::Left(lock.read().await) + } else { + Either::Right(lock.write().await) + }; + + router + .dispatch_tool_call(session, turn, tracker, sub_id, call) + .await + })); + + async move { + match handle.await { + Ok(Ok(response)) => Ok(response), + Ok(Err(FunctionCallError::Fatal(message))) => Err(CodexErr::Fatal(message)), + Ok(Err(other)) => Err(CodexErr::Fatal(other.to_string())), + Err(err) => Err(CodexErr::Fatal(format!( + "tool task failed to receive: {err:?}" + ))), + } } } } From 12fd2b4160ea1b8efa19db110749149944e560f0 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Tue, 7 Oct 2025 11:10:05 -0700 Subject: [PATCH 11/24] [TUI] Remove bottom padding (#4854) We don't need the bottom padding. It currently just makes the footer look off-centered. Before: image After: image --- codex-rs/tui/src/bottom_pane/mod.rs | 90 +++++++------------ ...er_fill_height_without_bottom_padding.snap | 11 +++ ...hidden_when_height_too_small_height_1.snap | 5 ++ ...hidden_when_height_too_small_height_2.snap | 6 ++ ...chatwidget__tests__chat_small_idle_h3.snap | 3 +- ...twidget__tests__chat_small_running_h2.snap | 3 +- ...twidget__tests__chat_small_running_h3.snap | 3 +- ...dget__tests__exec_approval_modal_exec.snap | 5 +- ...atwidget__tests__status_widget_active.snap | 2 +- ...sts__status_widget_and_approval_modal.snap | 2 +- 10 files changed, 66 insertions(+), 64 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_fill_height_without_bottom_padding.snap create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_1.snap create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_2.snap diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 6fe673a23b..db13a041cd 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -81,7 +81,7 @@ pub(crate) struct BottomPaneParams { } impl BottomPane { - const BOTTOM_PAD_LINES: u16 = 1; + const BOTTOM_PAD_LINES: u16 = 0; pub fn new(params: BottomPaneParams) -> Self { let enhanced_keys_supported = params.enhanced_keys_supported; Self { @@ -522,10 +522,29 @@ impl WidgetRef for &BottomPane { mod tests { use super::*; use crate::app_event::AppEvent; + use insta::assert_snapshot; use ratatui::buffer::Buffer; use ratatui::layout::Rect; use tokio::sync::mpsc::unbounded_channel; + fn snapshot_buffer(buf: &Buffer) -> String { + let mut lines = Vec::new(); + for y in 0..buf.area().height { + let mut row = String::new(); + for x in 0..buf.area().width { + row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); + } + lines.push(row); + } + lines.join("\n") + } + + fn render_snapshot(pane: &BottomPane, area: Rect) -> String { + let mut buf = Buffer::empty(area); + (&pane).render_ref(area, &mut buf); + snapshot_buffer(&buf) + } + fn exec_request() -> ApprovalRequest { ApprovalRequest::Exec { id: "1".to_string(), @@ -685,7 +704,7 @@ mod tests { } #[test] - fn bottom_padding_present_with_status_above_composer() { + fn status_and_composer_fill_height_without_bottom_padding() { let (tx_raw, _rx) = unbounded_channel::(); let tx = AppEventSender::new(tx_raw); let mut pane = BottomPane::new(BottomPaneParams { @@ -700,43 +719,21 @@ mod tests { // Activate spinner (status view replaces composer) with no live ring. pane.set_task_running(true); - // Use height == desired_height; expect 1 status row at top and 2 bottom padding rows. + // Use height == desired_height; expect spacer + status + composer rows without trailing padding. let height = pane.desired_height(30); assert!( height >= 3, - "expected at least 3 rows with bottom padding; got {height}" + "expected at least 3 rows to render spacer, status, and composer; got {height}" ); let area = Rect::new(0, 0, 30, height); - let mut buf = Buffer::empty(area); - (&pane).render_ref(area, &mut buf); - - // Row 1 contains the status header (row 0 is the spacer) - let mut top = String::new(); - for x in 0..area.width { - top.push(buf[(x, 1)].symbol().chars().next().unwrap_or(' ')); - } - assert!( - top.trim_start().starts_with("• Working"), - "expected top row to start with '• Working': {top:?}" - ); - assert!( - top.contains("Working"), - "expected Working header on top row: {top:?}" - ); - - // Last row should be blank padding; the row above should generally contain composer content. - let mut r_last = String::new(); - for x in 0..area.width { - r_last.push(buf[(x, height - 1)].symbol().chars().next().unwrap_or(' ')); - } - assert!( - r_last.trim().is_empty(), - "expected last row blank: {r_last:?}" + assert_snapshot!( + "status_and_composer_fill_height_without_bottom_padding", + render_snapshot(&pane, area) ); } #[test] - fn bottom_padding_shrinks_when_tiny() { + fn status_hidden_when_height_too_small() { let (tx_raw, _rx) = unbounded_channel::(); let tx = AppEventSender::new(tx_raw); let mut pane = BottomPane::new(BottomPaneParams { @@ -750,37 +747,18 @@ mod tests { pane.set_task_running(true); - // Height=2 → status on one row, composer on the other. + // Height=2 → composer takes the full space; status collapses when there is no room. let area2 = Rect::new(0, 0, 20, 2); - let mut buf2 = Buffer::empty(area2); - (&pane).render_ref(area2, &mut buf2); - let mut row0 = String::new(); - let mut row1 = String::new(); - for x in 0..area2.width { - row0.push(buf2[(x, 0)].symbol().chars().next().unwrap_or(' ')); - row1.push(buf2[(x, 1)].symbol().chars().next().unwrap_or(' ')); - } - let has_composer = row0.contains("Ask Codex") || row1.contains("Ask Codex"); - assert!( - has_composer, - "expected composer to be visible on one of the rows: row0={row0:?}, row1={row1:?}" - ); - assert!( - row0.contains("Working") || row1.contains("Working"), - "expected status header to be visible at height=2: row0={row0:?}, row1={row1:?}" + assert_snapshot!( + "status_hidden_when_height_too_small_height_2", + render_snapshot(&pane, area2) ); // Height=1 → no padding; single row is the composer (status hidden). let area1 = Rect::new(0, 0, 20, 1); - let mut buf1 = Buffer::empty(area1); - (&pane).render_ref(area1, &mut buf1); - let mut only = String::new(); - for x in 0..area1.width { - only.push(buf1[(x, 0)].symbol().chars().next().unwrap_or(' ')); - } - assert!( - only.contains("Ask Codex"), - "expected composer with no padding: {only:?}" + assert_snapshot!( + "status_hidden_when_height_too_small_height_1", + render_snapshot(&pane, area1) ); } } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_fill_height_without_bottom_padding.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_fill_height_without_bottom_padding.snap new file mode 100644 index 0000000000..ae96888f56 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_and_composer_fill_height_without_bottom_padding.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/bottom_pane/mod.rs +expression: "render_snapshot(&pane, area)" +--- + +• Working (0s • esc to interru + + +› Ask Codex to do anything + + ? for shortcuts diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_1.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_1.snap new file mode 100644 index 0000000000..310c32b40c --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_1.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/bottom_pane/mod.rs +expression: "render_snapshot(&pane, area1)" +--- +› Ask Codex to do an diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_2.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_2.snap new file mode 100644 index 0000000000..ea0beeedf3 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__tests__status_hidden_when_height_too_small_height_2.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/bottom_pane/mod.rs +expression: "render_snapshot(&pane, area2)" +--- + +› Ask Codex to do an diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h3.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h3.snap index 60ef79386d..adb00d4bc6 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h3.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_idle_h3.snap @@ -1,7 +1,8 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 1470 expression: terminal.backend() --- " " -"› Ask Codex to do anything " " " +"› Ask Codex to do anything " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h2.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h2.snap index 630d1a3bbe..4e1b74a2c6 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h2.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h2.snap @@ -1,6 +1,7 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 1500 expression: terminal.backend() --- -"• Thinking (0s • esc to interrupt) " +" " "› Ask Codex to do anything " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h3.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h3.snap index 60ef79386d..45d69ab1bd 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h3.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__chat_small_running_h3.snap @@ -1,7 +1,8 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 1500 expression: terminal.backend() --- " " +"• Thinking (0s • esc to interrupt) " "› Ask Codex to do anything " -" " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap index f52a0f38a2..f0cf3a2d33 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -1,9 +1,10 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 409 expression: "format!(\"{buf:?}\")" --- Buffer { - area: Rect { x: 0, y: 0, width: 80, height: 15 }, + area: Rect { x: 0, y: 0, width: 80, height: 14 }, content: [ " ", " ", @@ -19,7 +20,6 @@ Buffer { " 3. No, and tell Codex what to do differently esc ", " ", " Press enter to confirm or esc to cancel ", - " ", ], styles: [ x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, @@ -34,6 +34,5 @@ Buffer { x: 47, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, x: 50, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 0, y: 14, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, ] } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snap index 7c3253a2ea..e288807b00 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_active.snap @@ -1,5 +1,6 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 1577 expression: terminal.backend() --- " " @@ -9,4 +10,3 @@ expression: terminal.backend() "› Ask Codex to do anything " " " " ? for shortcuts " -" " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index d1951cd082..086a3a8c7e 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -1,5 +1,6 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 1548 expression: terminal.backend() --- " " @@ -16,4 +17,3 @@ expression: terminal.backend() " 3. No, and tell Codex what to do differently esc " " " " Press enter to confirm or esc to cancel " -" " From 75176dae703b38225dcc15435d287314cbefc137 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 7 Oct 2025 11:32:07 -0700 Subject: [PATCH 12/24] dynamic width for line numbers in diffs (#4664) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit instead of always reserving 6 spaces for the line number and gutter, we now dynamically adjust to the width of the longest number. Screenshot 2025-10-03 at 8 21 00 AM Screenshot 2025-10-03 at 8 21 21 AM Screenshot 2025-10-03 at 8 21 57 AM --- .../tui/src/bottom_pane/approval_overlay.rs | 7 +- ...ly_patch_manual_flow_history_approved.snap | 2 +- ...atwidget__tests__approval_modal_patch.snap | 8 +- codex-rs/tui/src/diff_render.rs | 116 ++++++++++++++---- codex-rs/tui/src/render/mod.rs | 1 + codex-rs/tui/src/render/renderable.rs | 26 ++++ ...__diff_render__tests__apply_add_block.snap | 4 +- ...iff_render__tests__apply_delete_block.snap | 6 +- ...er__tests__apply_multiple_files_block.snap | 6 +- ...iff_render__tests__apply_update_block.snap | 9 +- ..._block_line_numbers_three_digits_text.snap | 13 ++ ...der__tests__apply_update_block_manual.snap | 16 --- ...__apply_update_block_relativizes_path.snap | 7 +- ...__apply_update_block_wraps_long_lines.snap | 13 +- ...ly_update_block_wraps_long_lines_text.snap | 23 ++-- ...tests__apply_update_with_rename_block.snap | 9 +- ...f_render__tests__wrap_behavior_insert.snap | 4 +- ...ript_overlay_apply_patch_scroll_vt100.snap | 8 +- 18 files changed, 182 insertions(+), 96 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_line_numbers_three_digits_text.snap delete mode 100644 codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 7f52b11bfa..6585124c52 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -315,15 +315,16 @@ impl From for ApprovalRequestState { changes, } => { let mut header: Vec> = Vec::new(); - header.push(DiffSummary::new(changes, cwd).into()); if let Some(reason) = reason && !reason.is_empty() { - header.push(Box::new(Line::from(""))); header.push(Box::new( - Paragraph::new(reason.italic()).wrap(Wrap { trim: false }), + Paragraph::new(Line::from_iter(["Reason: ".into(), reason.italic()])) + .wrap(Wrap { trim: false }), )); + header.push(Box::new(Line::from(""))); } + header.push(DiffSummary::new(changes, cwd).into()); Self { variant: ApprovalVariant::ApplyPatch { id }, header: Box::new(ColumnRenderable::new(header)), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap index 6e22bcebaa..e139b51088 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap @@ -3,4 +3,4 @@ source: tui/src/chatwidget/tests.rs expression: lines_to_single_string(&approved_lines) --- • Added foo.txt (+1 -0) - 1 +hello + 1 +hello diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index ab88ffaf73..96dde8fb93 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -4,12 +4,12 @@ expression: terminal.backend().vt100().screen().contents() --- Would you like to make the following edits? + Reason: The model wants to apply changes + README.md (+2 -0) - 1 +hello - 2 +world - - The model wants to apply changes + 1 +hello + 2 +world › 1. Yes, proceed 2. No, and tell Codex what to do differently esc diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 6f5f7e7d8f..676b0559f8 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -13,13 +13,14 @@ use std::path::Path; use std::path::PathBuf; use crate::exec_command::relativize_to_home; +use crate::render::Insets; +use crate::render::line_utils::prefix_lines; use crate::render::renderable::ColumnRenderable; +use crate::render::renderable::InsetRenderable; use crate::render::renderable::Renderable; use codex_core::git_info::get_git_repo_root; use codex_core::protocol::FileChange; -const SPACES_AFTER_LINE_NUMBER: usize = 6; - // Internal representation for diff line rendering enum DiffLineType { Insert, @@ -65,7 +66,10 @@ impl From for Box { path.extend(render_line_count_summary(row.added, row.removed)); rows.push(Box::new(path)); rows.push(Box::new(RtLine::from(""))); - rows.push(Box::new(row.change)); + rows.push(Box::new(InsetRenderable::new( + Box::new(row.change), + Insets::tlbr(0, 2, 0, 0), + ))); } Box::new(ColumnRenderable::new(rows)) @@ -181,7 +185,9 @@ fn render_changes_block(rows: Vec, wrap_cols: usize, cwd: &Path) -> Vec, wrap_cols: usize, cwd: &Path) -> Vec>, width: usize) { match change { FileChange::Add { content } => { + let line_number_width = line_number_width(content.lines().count()); for (i, raw) in content.lines().enumerate() { out.extend(push_wrapped_diff_line( i + 1, DiffLineType::Insert, raw, width, + line_number_width, )); } } FileChange::Delete { content } => { + let line_number_width = line_number_width(content.lines().count()); for (i, raw) in content.lines().enumerate() { out.extend(push_wrapped_diff_line( i + 1, DiffLineType::Delete, raw, width, + line_number_width, )); } } FileChange::Update { unified_diff, .. } => { if let Ok(patch) = diffy::Patch::from_str(unified_diff) { + let mut max_line_number = 0; + for h in patch.hunks() { + let mut old_ln = h.old_range().start(); + let mut new_ln = h.new_range().start(); + for l in h.lines() { + match l { + diffy::Line::Insert(_) => { + max_line_number = max_line_number.max(new_ln); + new_ln += 1; + } + diffy::Line::Delete(_) => { + max_line_number = max_line_number.max(old_ln); + old_ln += 1; + } + diffy::Line::Context(_) => { + max_line_number = max_line_number.max(new_ln); + old_ln += 1; + new_ln += 1; + } + } + } + } + let line_number_width = line_number_width(max_line_number); let mut is_first_hunk = true; for h in patch.hunks() { if !is_first_hunk { - out.push(RtLine::from(vec![" ".into(), "⋮".dim()])); + let spacer = format!("{:width$} ", "", width = line_number_width.max(1)); + let spacer_span = RtSpan::styled(spacer, style_gutter()); + out.push(RtLine::from(vec![spacer_span, "⋮".dim()])); } is_first_hunk = false; @@ -229,6 +264,7 @@ fn render_change(change: &FileChange, out: &mut Vec>, width: usi DiffLineType::Insert, s, width, + line_number_width, )); new_ln += 1; } @@ -239,6 +275,7 @@ fn render_change(change: &FileChange, out: &mut Vec>, width: usi DiffLineType::Delete, s, width, + line_number_width, )); old_ln += 1; } @@ -249,6 +286,7 @@ fn render_change(change: &FileChange, out: &mut Vec>, width: usi DiffLineType::Context, s, width, + line_number_width, )); old_ln += 1; new_ln += 1; @@ -298,17 +336,15 @@ fn push_wrapped_diff_line( kind: DiffLineType, text: &str, width: usize, + line_number_width: usize, ) -> Vec> { - let indent = " "; let ln_str = line_number.to_string(); let mut remaining_text: &str = text; - // Reserve a fixed number of spaces after the line number so that content starts - // at a consistent column. Content includes a 1-character diff sign prefix - // ("+"/"-" for inserts/deletes, or a space for context lines) so alignment - // stays consistent across all diff lines. - let gap_after_ln = SPACES_AFTER_LINE_NUMBER.saturating_sub(ln_str.len()); - let prefix_cols = indent.len() + ln_str.len() + gap_after_ln; + // Reserve a fixed number of spaces (equal to the widest line number plus a + // trailing spacer) so the sign column stays aligned across the diff block. + let gutter_width = line_number_width.max(1); + let prefix_cols = gutter_width + 1; let mut first = true; let (sign_char, line_style) = match kind { @@ -332,8 +368,8 @@ fn push_wrapped_diff_line( remaining_text = rest; if first { - // Build gutter (indent + line number + spacing) as a dimmed span - let gutter = format!("{indent}{ln_str}{}", " ".repeat(gap_after_ln)); + // Build gutter (right-aligned line number plus spacer) as a dimmed span + let gutter = format!("{ln_str:>gutter_width$} "); // Content with a sign ('+'/'-'/' ') styled per diff kind let content = format!("{sign_char}{chunk}"); lines.push(RtLine::from(vec![ @@ -343,7 +379,7 @@ fn push_wrapped_diff_line( first = false; } else { // Continuation lines keep a space for the sign column so content aligns - let gutter = format!("{indent}{} ", " ".repeat(ln_str.len() + gap_after_ln)); + let gutter = format!("{:gutter_width$} ", ""); lines.push(RtLine::from(vec![ RtSpan::styled(gutter, style_gutter()), RtSpan::styled(chunk.to_string(), line_style), @@ -356,6 +392,14 @@ fn push_wrapped_diff_line( lines } +fn line_number_width(max_line_number: usize) -> usize { + if max_line_number == 0 { + 1 + } else { + max_line_number.to_string().len() + } +} + fn style_gutter() -> Style { Style::default().add_modifier(Modifier::DIM) } @@ -421,7 +465,8 @@ mod tests { let long_line = "this is a very long line that should wrap across multiple terminal columns and continue"; // Call the wrapping function directly so we can precisely control the width - let lines = push_wrapped_diff_line(1, DiffLineType::Insert, long_line, 80); + let lines = + push_wrapped_diff_line(1, DiffLineType::Insert, long_line, 80, line_number_width(1)); // Render into a small terminal to capture the visual layout snapshot_lines("wrap_behavior_insert", lines, 90, 8); @@ -442,11 +487,9 @@ mod tests { }, ); - for name in ["apply_update_block", "apply_update_block_manual"] { - let lines = diff_summary_for_tests(&changes); + let lines = diff_summary_for_tests(&changes); - snapshot_lines(name, lines, 80, 12); - } + snapshot_lines("apply_update_block", lines, 80, 12); } #[test] @@ -573,14 +616,37 @@ mod tests { }, ); - let mut lines = create_diff_summary(&changes, &PathBuf::from("/"), 28); - // Drop the combined header for this text-only snapshot - if !lines.is_empty() { - lines.remove(0); - } + let lines = create_diff_summary(&changes, &PathBuf::from("/"), 28); snapshot_lines_text("apply_update_block_wraps_long_lines_text", &lines); } + #[test] + fn ui_snapshot_apply_update_block_line_numbers_three_digits_text() { + let original = (1..=110).map(|i| format!("line {i}\n")).collect::(); + let modified = (1..=110) + .map(|i| { + if i == 100 { + format!("line {i} changed\n") + } else { + format!("line {i}\n") + } + }) + .collect::(); + let patch = diffy::create_patch(&original, &modified).to_string(); + + let mut changes: HashMap = HashMap::new(); + changes.insert( + PathBuf::from("hundreds.txt"), + FileChange::Update { + unified_diff: patch, + move_path: None, + }, + ); + + let lines = create_diff_summary(&changes, &PathBuf::from("/"), 80); + snapshot_lines_text("apply_update_block_line_numbers_three_digits_text", &lines); + } + #[test] fn ui_snapshot_apply_update_block_relativizes_path() { let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("/")); diff --git a/codex-rs/tui/src/render/mod.rs b/codex-rs/tui/src/render/mod.rs index 441c1d6b21..fe92eea91b 100644 --- a/codex-rs/tui/src/render/mod.rs +++ b/codex-rs/tui/src/render/mod.rs @@ -4,6 +4,7 @@ pub mod highlight; pub mod line_utils; pub mod renderable; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Insets { pub left: u16, pub top: u16, diff --git a/codex-rs/tui/src/render/renderable.rs b/codex-rs/tui/src/render/renderable.rs index 868d07264b..c55426337d 100644 --- a/codex-rs/tui/src/render/renderable.rs +++ b/codex-rs/tui/src/render/renderable.rs @@ -4,6 +4,9 @@ use ratatui::text::Line; use ratatui::widgets::Paragraph; use ratatui::widgets::WidgetRef; +use crate::render::Insets; +use crate::render::RectExt as _; + pub trait Renderable { fn render(&self, area: Rect, buf: &mut Buffer); fn desired_height(&self, width: u16) -> u16; @@ -100,3 +103,26 @@ impl ColumnRenderable { } } } + +pub struct InsetRenderable { + child: Box, + insets: Insets, +} + +impl Renderable for InsetRenderable { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.child.render(area.inset(self.insets), buf); + } + fn desired_height(&self, width: u16) -> u16 { + self.child + .desired_height(width - self.insets.left - self.insets.right) + + self.insets.top + + self.insets.bottom + } +} + +impl InsetRenderable { + pub fn new(child: Box, insets: Insets) -> Self { + Self { child, insets } + } +} diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_add_block.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_add_block.snap index 254f51b644..cd5aaf5fa0 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_add_block.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_add_block.snap @@ -3,8 +3,8 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- "• Added new_file.txt (+2 -0) " -" 1 +alpha " -" 2 +beta " +" 1 +alpha " +" 2 +beta " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_delete_block.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_delete_block.snap index 989200b228..edfdb2c05f 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_delete_block.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_delete_block.snap @@ -3,9 +3,9 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- "• Deleted tmp_delete_example.txt (+0 -3) " -" 1 -first " -" 2 -second " -" 3 -third " +" 1 -first " +" 2 -second " +" 3 -third " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_multiple_files_block.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_multiple_files_block.snap index 62e1f58e1b..62fc671d11 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_multiple_files_block.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_multiple_files_block.snap @@ -4,11 +4,11 @@ expression: terminal.backend() --- "• Edited 2 files (+2 -1) " " └ a.txt (+1 -1) " -" 1 -one " -" 1 +one changed " +" 1 -one " +" 1 +one changed " " " " └ b.txt (+1 -0) " -" 1 +new " +" 1 +new " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block.snap index 305120d388..8cc31efdde 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block.snap @@ -1,13 +1,12 @@ --- source: tui/src/diff_render.rs -assertion_line: 748 expression: terminal.backend() --- "• Edited example.txt (+1 -1) " -" 1 line one " -" 2 -line two " -" 2 +line two changed " -" 3 line three " +" 1 line one " +" 2 -line two " +" 2 +line two changed " +" 3 line three " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_line_numbers_three_digits_text.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_line_numbers_three_digits_text.snap new file mode 100644 index 0000000000..56058ee70e --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_line_numbers_three_digits_text.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/diff_render.rs +expression: text +--- +• Edited hundreds.txt (+1 -1) + 97 line 97 + 98 line 98 + 99 line 99 + 100 -line 100 + 100 +line 100 changed + 101 line 101 + 102 line 102 + 103 line 103 diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap deleted file mode 100644 index d188b2fd6f..0000000000 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap +++ /dev/null @@ -1,16 +0,0 @@ ---- -source: tui/src/diff_render.rs -expression: terminal.backend() ---- -"• Edited example.txt (+1 -1) " -" 1 line one " -" 2 -line two " -" 2 +line two changed " -" 3 line three " -" " -" " -" " -" " -" " -" " -" " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_relativizes_path.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_relativizes_path.snap index 825e50ebc9..a50f7700cf 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_relativizes_path.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_relativizes_path.snap @@ -1,12 +1,11 @@ --- source: tui/src/diff_render.rs -assertion_line: 748 expression: terminal.backend() --- "• Edited abs_old.rs → abs_new.rs (+1 -1) " -" 1 -X " -" 1 +X changed " -" 2 Y " +" 1 -X " +" 1 +X changed " +" 2 Y " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines.snap index d571b00638..72b9528e5c 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines.snap @@ -1,15 +1,14 @@ --- source: tui/src/diff_render.rs -assertion_line: 748 expression: terminal.backend() --- "• Edited long_example.txt (+1 -1) " -" 1 line 1 " -" 2 -short " -" 2 +short this_is_a_very_long_modified_line_that_should_wrap_acro " -" ss_multiple_terminal_columns_and_continue_even_further_beyond " -" _eighty_columns_to_force_multiple_wraps " -" 3 line 3 " +" 1 line 1 " +" 2 -short " +" 2 +short this_is_a_very_long_modified_line_that_should_wrap_across_m " +" ultiple_terminal_columns_and_continue_even_further_beyond_eighty_ " +" columns_to_force_multiple_wraps " +" 3 line 3 " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines_text.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines_text.snap index ea495c10cb..17c92c1b5d 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines_text.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_wraps_long_lines_text.snap @@ -2,15 +2,14 @@ source: tui/src/diff_render.rs expression: text --- - 1 1 - 2 -2 - 2 +added long line w - hich wraps and_if - _there_is_a_long_ - token_it_will_be_ - broken - 3 3 - 4 -4 - 4 +4 context line wh - ich also wraps ac - ross +• Edited wrap_demo.txt (+2 -2) + 1 1 + 2 -2 + 2 +added long line which + wraps and_if_there_i + s_a_long_token_it_wil + l_be_broken + 3 3 + 4 -4 + 4 +4 context line which + also wraps across diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_with_rename_block.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_with_rename_block.snap index 89c2ddf141..29b321011c 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_with_rename_block.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_with_rename_block.snap @@ -1,13 +1,12 @@ --- source: tui/src/diff_render.rs -assertion_line: 748 expression: terminal.backend() --- "• Edited old_name.rs → new_name.rs (+1 -1) " -" 1 A " -" 2 -B " -" 2 +B changed " -" 3 C " +" 1 A " +" 2 -B " +" 2 +B changed " +" 3 C " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap index b14dafaa3c..7532977cea 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__wrap_behavior_insert.snap @@ -2,8 +2,8 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- -" 1 +this is a very long line that should wrap across multiple terminal co " -" lumns and continue " +"1 +this is a very long line that should wrap across multiple terminal columns an " +" d continue " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap b/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap index 7beca4a4c2..1f086e81e4 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap @@ -4,12 +4,12 @@ expression: snapshot --- / T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / • Added foo.txt (+2 -0) - 1 +hello - 2 +world + 1 +hello + 2 +world • Added foo.txt (+2 -0) - 1 +hello - 2 +world + 1 +hello + 2 +world ─────────────────────────────────────────────────────────────────────────── 0% ─ ↑/↓ to scroll pgup/pgdn to page home/end to jump q to quit esc to edit prev From 4b0f5eb6a825578b43a8605ed07e9981009e70aa Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 7 Oct 2025 11:32:13 -0700 Subject: [PATCH 13/24] tui: wrapping bugfix (#4674) this fixes an issue where text lines with long words would sometimes overflow. - the default penalties for the OptimalFit algorithm allow overflowing in some cases. this seems insane to me, and i considered just banning the OptimalFit algorithm by disabling the 'smawk' feature on textwrap, but decided to keep it and just bump the overflow penalty to ~infinity since optimal fit does sometimes produce nicer wrappings. it's not clear this is worth it, though, and maybe we should just dump the optimal fit algorithm completely. - user history messages weren't rendering with the same wrap algorithm as used in the composer, which sometimes resulted in wrapping messages differently in the history vs. in the composer. --- codex-rs/tui/src/history_cell.rs | 3 ++- codex-rs/tui/src/wrapping.rs | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index e3a89da340..61640c3334 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -104,7 +104,8 @@ impl HistoryCell for UserHistoryCell { .lines() .map(|l| Line::from(l).style(style)) .collect::>(), - RtOptions::new(wrap_width as usize), + // Wrap algorithm matches textarea.rs. + RtOptions::new(wrap_width as usize).wrap_algorithm(textwrap::WrapAlgorithm::FirstFit), ); lines.push(Line::from("").style(style)); diff --git a/codex-rs/tui/src/wrapping.rs b/codex-rs/tui/src/wrapping.rs index da79f036b4..70ca2e46ca 100644 --- a/codex-rs/tui/src/wrapping.rs +++ b/codex-rs/tui/src/wrapping.rs @@ -2,6 +2,7 @@ use ratatui::text::Line; use ratatui::text::Span; use std::ops::Range; use textwrap::Options; +use textwrap::wrap_algorithms::Penalties; use crate::render::line_utils::push_owned_lines; @@ -90,7 +91,11 @@ impl<'a> RtOptions<'a> { subsequent_indent: Line::default(), break_words: true, word_separator: textwrap::WordSeparator::new(), - wrap_algorithm: textwrap::WrapAlgorithm::new(), + wrap_algorithm: textwrap::WrapAlgorithm::OptimalFit(Penalties { + // ~infinite overflow penalty, we never want to overflow a line. + overflow_penalty: usize::MAX / 4, + ..Default::default() + }), word_splitter: textwrap::WordSplitter::HyphenSplitter, } } From 338c2c873c10107a22b089ed56473338e80802c9 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 7 Oct 2025 19:32:49 +0100 Subject: [PATCH 14/24] bug: fix flaky test (#4878) Fix flaky test by warming up the tools --- codex-rs/core/tests/suite/tool_parallelism.rs | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/tests/suite/tool_parallelism.rs b/codex-rs/core/tests/suite/tool_parallelism.rs index e667df4369..0923f2f068 100644 --- a/codex-rs/core/tests/suite/tool_parallelism.rs +++ b/codex-rs/core/tests/suite/tool_parallelism.rs @@ -83,6 +83,16 @@ async fn read_file_tools_run_in_parallel() -> anyhow::Result<()> { let server = start_mock_server().await; let test = build_codex_with_test_tool(&server).await?; + let warmup_args = json!({ + "sleep_after_ms": 10, + "barrier": { + "id": "parallel-test-sync-warmup", + "participants": 2, + "timeout_ms": 1_000, + } + }) + .to_string(); + let parallel_args = json!({ "sleep_after_ms": 300, "barrier": { @@ -93,6 +103,17 @@ async fn read_file_tools_run_in_parallel() -> anyhow::Result<()> { }) .to_string(); + let warmup_first = sse(vec![ + json!({"type": "response.created", "response": {"id": "resp-warm-1"}}), + ev_function_call("warm-call-1", "test_sync_tool", &warmup_args), + ev_function_call("warm-call-2", "test_sync_tool", &warmup_args), + ev_completed("resp-warm-1"), + ]); + let warmup_second = sse(vec![ + ev_assistant_message("warm-msg-1", "warmup complete"), + ev_completed("resp-warm-2"), + ]); + let first_response = sse(vec![ json!({"type": "response.created", "response": {"id": "resp-1"}}), ev_function_call("call-1", "test_sync_tool", ¶llel_args), @@ -103,7 +124,13 @@ async fn read_file_tools_run_in_parallel() -> anyhow::Result<()> { ev_assistant_message("msg-1", "done"), ev_completed("resp-2"), ]); - mount_sse_sequence(&server, vec![first_response, second_response]).await; + mount_sse_sequence( + &server, + vec![warmup_first, warmup_second, first_response, second_response], + ) + .await; + + run_turn(&test, "warm up parallel tool").await?; let duration = run_turn_and_measure(&test, "exercise sync tool").await?; assert_parallel_duration(duration); From 226215f36d207286ee1e0d16fc2cea1a3e628973 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Tue, 7 Oct 2025 19:33:19 +0100 Subject: [PATCH 15/24] feat: `list_dir` tool (#4817) Add a tool to list_dir. It is useful because we can mark it as non-mutating and so use it in parallel --- codex-rs/core/src/model_family.rs | 3 +- codex-rs/core/src/tools/handlers/list_dir.rs | 476 +++++++++++++++++++ codex-rs/core/src/tools/handlers/mod.rs | 2 + codex-rs/core/src/tools/spec.rs | 58 +++ codex-rs/core/tests/suite/list_dir.rs | 460 ++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/tools.rs | 14 +- 7 files changed, 1001 insertions(+), 13 deletions(-) create mode 100644 codex-rs/core/src/tools/handlers/list_dir.rs create mode 100644 codex-rs/core/tests/suite/list_dir.rs diff --git a/codex-rs/core/src/model_family.rs b/codex-rs/core/src/model_family.rs index ed3230e2b3..2c66d9ad12 100644 --- a/codex-rs/core/src/model_family.rs +++ b/codex-rs/core/src/model_family.rs @@ -120,6 +120,7 @@ pub fn find_family_for_model(mut slug: &str) -> Option { base_instructions: GPT_5_CODEX_INSTRUCTIONS.to_string(), experimental_supported_tools: vec![ "read_file".to_string(), + "list_dir".to_string(), "test_sync_tool".to_string() ], supports_parallel_tool_calls: true, @@ -133,7 +134,7 @@ pub fn find_family_for_model(mut slug: &str) -> Option { reasoning_summary_format: ReasoningSummaryFormat::Experimental, base_instructions: GPT_5_CODEX_INSTRUCTIONS.to_string(), apply_patch_tool_type: Some(ApplyPatchToolType::Freeform), - experimental_supported_tools: vec!["read_file".to_string()], + experimental_supported_tools: vec!["read_file".to_string(), "list_dir".to_string()], supports_parallel_tool_calls: true, ) diff --git a/codex-rs/core/src/tools/handlers/list_dir.rs b/codex-rs/core/src/tools/handlers/list_dir.rs new file mode 100644 index 0000000000..bcea4a756d --- /dev/null +++ b/codex-rs/core/src/tools/handlers/list_dir.rs @@ -0,0 +1,476 @@ +use std::collections::VecDeque; +use std::ffi::OsStr; +use std::fs::FileType; +use std::path::Path; +use std::path::PathBuf; + +use async_trait::async_trait; +use codex_utils_string::take_bytes_at_char_boundary; +use serde::Deserialize; +use tokio::fs; + +use crate::function_tool::FunctionCallError; +use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolOutput; +use crate::tools::context::ToolPayload; +use crate::tools::registry::ToolHandler; +use crate::tools::registry::ToolKind; + +pub struct ListDirHandler; + +const MAX_ENTRY_LENGTH: usize = 500; +const INDENTATION_SPACES: usize = 2; + +fn default_offset() -> usize { + 1 +} + +fn default_limit() -> usize { + 25 +} + +fn default_depth() -> usize { + 2 +} + +#[derive(Deserialize)] +struct ListDirArgs { + dir_path: String, + #[serde(default = "default_offset")] + offset: usize, + #[serde(default = "default_limit")] + limit: usize, + #[serde(default = "default_depth")] + depth: usize, +} + +#[async_trait] +impl ToolHandler for ListDirHandler { + fn kind(&self) -> ToolKind { + ToolKind::Function + } + + async fn handle(&self, invocation: ToolInvocation) -> Result { + let ToolInvocation { payload, .. } = invocation; + + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { + return Err(FunctionCallError::RespondToModel( + "list_dir handler received unsupported payload".to_string(), + )); + } + }; + + let args: ListDirArgs = serde_json::from_str(&arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to parse function arguments: {err:?}" + )) + })?; + + let ListDirArgs { + dir_path, + offset, + limit, + depth, + } = args; + + if offset == 0 { + return Err(FunctionCallError::RespondToModel( + "offset must be a 1-indexed entry number".to_string(), + )); + } + + if limit == 0 { + return Err(FunctionCallError::RespondToModel( + "limit must be greater than zero".to_string(), + )); + } + + if depth == 0 { + return Err(FunctionCallError::RespondToModel( + "depth must be greater than zero".to_string(), + )); + } + + let path = PathBuf::from(&dir_path); + if !path.is_absolute() { + return Err(FunctionCallError::RespondToModel( + "dir_path must be an absolute path".to_string(), + )); + } + + let entries = list_dir_slice(&path, offset, limit, depth).await?; + let mut output = Vec::with_capacity(entries.len() + 1); + output.push(format!("Absolute path: {}", path.display())); + output.extend(entries); + Ok(ToolOutput::Function { + content: output.join("\n"), + success: Some(true), + }) + } +} + +async fn list_dir_slice( + path: &Path, + offset: usize, + limit: usize, + depth: usize, +) -> Result, FunctionCallError> { + let mut entries = Vec::new(); + collect_entries(path, Path::new(""), depth, &mut entries).await?; + + if entries.is_empty() { + return Ok(Vec::new()); + } + + let start_index = offset - 1; + if start_index >= entries.len() { + return Err(FunctionCallError::RespondToModel( + "offset exceeds directory entry count".to_string(), + )); + } + + let remaining_entries = entries.len() - start_index; + let capped_limit = limit.min(remaining_entries); + let end_index = start_index + capped_limit; + let mut selected_entries = entries[start_index..end_index].to_vec(); + selected_entries.sort_unstable_by(|a, b| a.name.cmp(&b.name)); + let mut formatted = Vec::with_capacity(selected_entries.len()); + + for entry in &selected_entries { + formatted.push(format_entry_line(entry)); + } + + if end_index < entries.len() { + formatted.push(format!("More than {capped_limit} entries found")); + } + + Ok(formatted) +} + +async fn collect_entries( + dir_path: &Path, + relative_prefix: &Path, + depth: usize, + entries: &mut Vec, +) -> Result<(), FunctionCallError> { + let mut queue = VecDeque::new(); + queue.push_back((dir_path.to_path_buf(), relative_prefix.to_path_buf(), depth)); + + while let Some((current_dir, prefix, remaining_depth)) = queue.pop_front() { + let mut read_dir = fs::read_dir(¤t_dir).await.map_err(|err| { + FunctionCallError::RespondToModel(format!("failed to read directory: {err}")) + })?; + + let mut dir_entries = Vec::new(); + + while let Some(entry) = read_dir.next_entry().await.map_err(|err| { + FunctionCallError::RespondToModel(format!("failed to read directory: {err}")) + })? { + let file_type = entry.file_type().await.map_err(|err| { + FunctionCallError::RespondToModel(format!("failed to inspect entry: {err}")) + })?; + + let file_name = entry.file_name(); + let relative_path = if prefix.as_os_str().is_empty() { + PathBuf::from(&file_name) + } else { + prefix.join(&file_name) + }; + + let display_name = format_entry_component(&file_name); + let display_depth = prefix.components().count(); + let sort_key = format_entry_name(&relative_path); + let kind = DirEntryKind::from(&file_type); + dir_entries.push(( + entry.path(), + relative_path, + kind, + DirEntry { + name: sort_key, + display_name, + depth: display_depth, + kind, + }, + )); + } + + dir_entries.sort_unstable_by(|a, b| a.3.name.cmp(&b.3.name)); + + for (entry_path, relative_path, kind, dir_entry) in dir_entries { + if kind == DirEntryKind::Directory && remaining_depth > 1 { + queue.push_back((entry_path, relative_path, remaining_depth - 1)); + } + entries.push(dir_entry); + } + } + + Ok(()) +} + +fn format_entry_name(path: &Path) -> String { + let normalized = path.to_string_lossy().replace("\\", "/"); + if normalized.len() > MAX_ENTRY_LENGTH { + take_bytes_at_char_boundary(&normalized, MAX_ENTRY_LENGTH).to_string() + } else { + normalized + } +} + +fn format_entry_component(name: &OsStr) -> String { + let normalized = name.to_string_lossy(); + if normalized.len() > MAX_ENTRY_LENGTH { + take_bytes_at_char_boundary(&normalized, MAX_ENTRY_LENGTH).to_string() + } else { + normalized.to_string() + } +} + +fn format_entry_line(entry: &DirEntry) -> String { + let indent = " ".repeat(entry.depth * INDENTATION_SPACES); + let mut name = entry.display_name.clone(); + match entry.kind { + DirEntryKind::Directory => name.push('/'), + DirEntryKind::Symlink => name.push('@'), + DirEntryKind::Other => name.push('?'), + DirEntryKind::File => {} + } + format!("{indent}{name}") +} + +#[derive(Clone)] +struct DirEntry { + name: String, + display_name: String, + depth: usize, + kind: DirEntryKind, +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum DirEntryKind { + Directory, + File, + Symlink, + Other, +} + +impl From<&FileType> for DirEntryKind { + fn from(file_type: &FileType) -> Self { + if file_type.is_symlink() { + DirEntryKind::Symlink + } else if file_type.is_dir() { + DirEntryKind::Directory + } else if file_type.is_file() { + DirEntryKind::File + } else { + DirEntryKind::Other + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[tokio::test] + async fn lists_directory_entries() { + let temp = tempdir().expect("create tempdir"); + let dir_path = temp.path(); + + let sub_dir = dir_path.join("nested"); + tokio::fs::create_dir(&sub_dir) + .await + .expect("create sub dir"); + + let deeper_dir = sub_dir.join("deeper"); + tokio::fs::create_dir(&deeper_dir) + .await + .expect("create deeper dir"); + + tokio::fs::write(dir_path.join("entry.txt"), b"content") + .await + .expect("write file"); + tokio::fs::write(sub_dir.join("child.txt"), b"child") + .await + .expect("write child"); + tokio::fs::write(deeper_dir.join("grandchild.txt"), b"grandchild") + .await + .expect("write grandchild"); + + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + let link_path = dir_path.join("link"); + symlink(dir_path.join("entry.txt"), &link_path).expect("create symlink"); + } + + let entries = list_dir_slice(dir_path, 1, 20, 3) + .await + .expect("list directory"); + + #[cfg(unix)] + let expected = vec![ + "entry.txt".to_string(), + "link@".to_string(), + "nested/".to_string(), + " child.txt".to_string(), + " deeper/".to_string(), + " grandchild.txt".to_string(), + ]; + + #[cfg(not(unix))] + let expected = vec![ + "entry.txt".to_string(), + "nested/".to_string(), + " child.txt".to_string(), + " deeper/".to_string(), + " grandchild.txt".to_string(), + ]; + + assert_eq!(entries, expected); + } + + #[tokio::test] + async fn errors_when_offset_exceeds_entries() { + let temp = tempdir().expect("create tempdir"); + let dir_path = temp.path(); + tokio::fs::create_dir(dir_path.join("nested")) + .await + .expect("create sub dir"); + + let err = list_dir_slice(dir_path, 10, 1, 2) + .await + .expect_err("offset exceeds entries"); + assert_eq!( + err, + FunctionCallError::RespondToModel("offset exceeds directory entry count".to_string()) + ); + } + + #[tokio::test] + async fn respects_depth_parameter() { + let temp = tempdir().expect("create tempdir"); + let dir_path = temp.path(); + let nested = dir_path.join("nested"); + let deeper = nested.join("deeper"); + tokio::fs::create_dir(&nested).await.expect("create nested"); + tokio::fs::create_dir(&deeper).await.expect("create deeper"); + tokio::fs::write(dir_path.join("root.txt"), b"root") + .await + .expect("write root"); + tokio::fs::write(nested.join("child.txt"), b"child") + .await + .expect("write nested"); + tokio::fs::write(deeper.join("grandchild.txt"), b"deep") + .await + .expect("write deeper"); + + let entries_depth_one = list_dir_slice(dir_path, 1, 10, 1) + .await + .expect("list depth 1"); + assert_eq!( + entries_depth_one, + vec!["nested/".to_string(), "root.txt".to_string(),] + ); + + let entries_depth_two = list_dir_slice(dir_path, 1, 20, 2) + .await + .expect("list depth 2"); + assert_eq!( + entries_depth_two, + vec![ + "nested/".to_string(), + " child.txt".to_string(), + " deeper/".to_string(), + "root.txt".to_string(), + ] + ); + + let entries_depth_three = list_dir_slice(dir_path, 1, 30, 3) + .await + .expect("list depth 3"); + assert_eq!( + entries_depth_three, + vec![ + "nested/".to_string(), + " child.txt".to_string(), + " deeper/".to_string(), + " grandchild.txt".to_string(), + "root.txt".to_string(), + ] + ); + } + + #[tokio::test] + async fn handles_large_limit_without_overflow() { + let temp = tempdir().expect("create tempdir"); + let dir_path = temp.path(); + tokio::fs::write(dir_path.join("alpha.txt"), b"alpha") + .await + .expect("write alpha"); + tokio::fs::write(dir_path.join("beta.txt"), b"beta") + .await + .expect("write beta"); + tokio::fs::write(dir_path.join("gamma.txt"), b"gamma") + .await + .expect("write gamma"); + + let entries = list_dir_slice(dir_path, 2, usize::MAX, 1) + .await + .expect("list without overflow"); + assert_eq!( + entries, + vec!["beta.txt".to_string(), "gamma.txt".to_string(),] + ); + } + + #[tokio::test] + async fn indicates_truncated_results() { + let temp = tempdir().expect("create tempdir"); + let dir_path = temp.path(); + + for idx in 0..40 { + let file = dir_path.join(format!("file_{idx:02}.txt")); + tokio::fs::write(file, b"content") + .await + .expect("write file"); + } + + let entries = list_dir_slice(dir_path, 1, 25, 1) + .await + .expect("list directory"); + assert_eq!(entries.len(), 26); + assert_eq!( + entries.last(), + Some(&"More than 25 entries found".to_string()) + ); + } + + #[tokio::test] + async fn bfs_truncation() -> anyhow::Result<()> { + let temp = tempdir()?; + let dir_path = temp.path(); + let nested = dir_path.join("nested"); + let deeper = nested.join("deeper"); + tokio::fs::create_dir(&nested).await?; + tokio::fs::create_dir(&deeper).await?; + tokio::fs::write(dir_path.join("root.txt"), b"root").await?; + tokio::fs::write(nested.join("child.txt"), b"child").await?; + tokio::fs::write(deeper.join("grandchild.txt"), b"deep").await?; + + let entries_depth_three = list_dir_slice(dir_path, 1, 3, 3).await?; + assert_eq!( + entries_depth_three, + vec![ + "nested/".to_string(), + " child.txt".to_string(), + "root.txt".to_string(), + "More than 3 entries found".to_string() + ] + ); + + Ok(()) + } +} diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index caa778c91e..d8cf29be72 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -1,5 +1,6 @@ pub mod apply_patch; mod exec_stream; +mod list_dir; mod mcp; mod plan; mod read_file; @@ -12,6 +13,7 @@ pub use plan::PLAN_TOOL; pub use apply_patch::ApplyPatchHandler; pub use exec_stream::ExecStreamHandler; +pub use list_dir::ListDirHandler; pub use mcp::McpHandler; pub use plan::PlanHandler; pub use read_file::ReadFileHandler; diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 51124d412f..08c3f60f27 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -356,6 +356,51 @@ fn create_read_file_tool() -> ToolSpec { }, }) } + +fn create_list_dir_tool() -> ToolSpec { + let mut properties = BTreeMap::new(); + properties.insert( + "dir_path".to_string(), + JsonSchema::String { + description: Some("Absolute path to the directory to list.".to_string()), + }, + ); + properties.insert( + "offset".to_string(), + JsonSchema::Number { + description: Some( + "The entry number to start listing from. Must be 1 or greater.".to_string(), + ), + }, + ); + properties.insert( + "limit".to_string(), + JsonSchema::Number { + description: Some("The maximum number of entries to return.".to_string()), + }, + ); + properties.insert( + "depth".to_string(), + JsonSchema::Number { + description: Some( + "The maximum directory depth to traverse. Must be 1 or greater.".to_string(), + ), + }, + ); + + ToolSpec::Function(ResponsesApiTool { + name: "list_dir".to_string(), + description: + "Lists entries in a local directory with 1-indexed entry numbers and simple type labels." + .to_string(), + strict: false, + parameters: JsonSchema::Object { + properties, + required: Some(vec!["dir_path".to_string()]), + additional_properties: Some(false.into()), + }, + }) +} /// TODO(dylan): deprecate once we get rid of json tool #[derive(Serialize, Deserialize)] pub(crate) struct ApplyPatchToolArgs { @@ -565,6 +610,7 @@ pub(crate) fn build_specs( use crate::exec_command::create_write_stdin_tool_for_responses_api; use crate::tools::handlers::ApplyPatchHandler; use crate::tools::handlers::ExecStreamHandler; + use crate::tools::handlers::ListDirHandler; use crate::tools::handlers::McpHandler; use crate::tools::handlers::PlanHandler; use crate::tools::handlers::ReadFileHandler; @@ -640,6 +686,16 @@ pub(crate) fn build_specs( builder.register_handler("read_file", read_file_handler); } + if config + .experimental_supported_tools + .iter() + .any(|tool| tool == "list_dir") + { + let list_dir_handler = Arc::new(ListDirHandler); + builder.push_spec_with_parallel_support(create_list_dir_tool(), true); + builder.register_handler("list_dir", list_dir_handler); + } + if config .experimental_supported_tools .iter() @@ -786,6 +842,7 @@ mod tests { assert!(!find_tool(&tools, "unified_exec").supports_parallel_tool_calls); assert!(find_tool(&tools, "read_file").supports_parallel_tool_calls); + assert!(find_tool(&tools, "list_dir").supports_parallel_tool_calls); } #[test] @@ -813,6 +870,7 @@ mod tests { .iter() .any(|tool| tool_name(&tool.spec) == "read_file") ); + assert!(tools.iter().any(|tool| tool_name(&tool.spec) == "list_dir")); } #[test] diff --git a/codex-rs/core/tests/suite/list_dir.rs b/codex-rs/core/tests/suite/list_dir.rs new file mode 100644 index 0000000000..1aa5a6484b --- /dev/null +++ b/codex-rs/core/tests/suite/list_dir.rs @@ -0,0 +1,460 @@ +#![cfg(not(target_os = "windows"))] + +use codex_core::protocol::AskForApproval; +use codex_core::protocol::EventMsg; +use codex_core::protocol::InputItem; +use codex_core::protocol::Op; +use codex_core::protocol::SandboxPolicy; +use codex_protocol::config_types::ReasoningSummary; +use core_test_support::responses; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use serde_json::Value; +use wiremock::matchers::any; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "disabled until we enable list_dir tool"] +async fn list_dir_tool_returns_entries() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + + let TestCodex { + codex, + cwd, + session_configured, + .. + } = test_codex().build(&server).await?; + + let dir_path = cwd.path().join("sample_dir"); + std::fs::create_dir(&dir_path)?; + std::fs::write(dir_path.join("alpha.txt"), "first file")?; + std::fs::create_dir(dir_path.join("nested"))?; + let dir_path = dir_path.to_string_lossy().to_string(); + + let call_id = "list-dir-call"; + let arguments = serde_json::json!({ + "dir_path": dir_path, + "offset": 1, + "limit": 2, + }) + .to_string(); + + let first_response = sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "list_dir", &arguments), + ev_completed("resp-1"), + ]); + responses::mount_sse_once_match(&server, any(), first_response).await; + + let second_response = sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]); + responses::mount_sse_once_match(&server, any(), second_response).await; + + let session_model = session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "list directory contents".into(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.expect("recorded requests"); + let request_bodies = requests + .iter() + .map(|req| req.body_json::().unwrap()) + .collect::>(); + assert!( + !request_bodies.is_empty(), + "expected at least one request body" + ); + + let tool_output_item = request_bodies + .iter() + .find_map(|body| { + body.get("input") + .and_then(Value::as_array) + .and_then(|items| { + items.iter().find(|item| { + item.get("type").and_then(Value::as_str) == Some("function_call_output") + }) + }) + }) + .unwrap_or_else(|| { + panic!("function_call_output item not found in requests: {request_bodies:#?}") + }); + + assert_eq!( + tool_output_item.get("call_id").and_then(Value::as_str), + Some(call_id) + ); + + let output_text = tool_output_item + .get("output") + .and_then(|value| match value { + Value::String(text) => Some(text.as_str()), + Value::Object(obj) => obj.get("content").and_then(Value::as_str), + _ => None, + }) + .expect("output text present"); + assert_eq!(output_text, "E1: [file] alpha.txt\nE2: [dir] nested"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "disabled until we enable list_dir tool"] +async fn list_dir_tool_depth_one_omits_children() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + + let TestCodex { + codex, + cwd, + session_configured, + .. + } = test_codex().build(&server).await?; + + let dir_path = cwd.path().join("depth_one"); + std::fs::create_dir(&dir_path)?; + std::fs::write(dir_path.join("alpha.txt"), "alpha")?; + std::fs::create_dir(dir_path.join("nested"))?; + std::fs::write(dir_path.join("nested").join("beta.txt"), "beta")?; + let dir_path = dir_path.to_string_lossy().to_string(); + + let call_id = "list-dir-depth1"; + let arguments = serde_json::json!({ + "dir_path": dir_path, + "offset": 1, + "limit": 10, + "depth": 1, + }) + .to_string(); + + let first_response = sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "list_dir", &arguments), + ev_completed("resp-1"), + ]); + responses::mount_sse_once_match(&server, any(), first_response).await; + + let second_response = sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]); + responses::mount_sse_once_match(&server, any(), second_response).await; + + let session_model = session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "list directory contents depth one".into(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.expect("recorded requests"); + let request_bodies = requests + .iter() + .map(|req| req.body_json::().unwrap()) + .collect::>(); + assert!( + !request_bodies.is_empty(), + "expected at least one request body" + ); + + let tool_output_item = request_bodies + .iter() + .find_map(|body| { + body.get("input") + .and_then(Value::as_array) + .and_then(|items| { + items.iter().find(|item| { + item.get("type").and_then(Value::as_str) == Some("function_call_output") + }) + }) + }) + .unwrap_or_else(|| { + panic!("function_call_output item not found in requests: {request_bodies:#?}") + }); + + assert_eq!( + tool_output_item.get("call_id").and_then(Value::as_str), + Some(call_id) + ); + + let output_text = tool_output_item + .get("output") + .and_then(|value| match value { + Value::String(text) => Some(text.as_str()), + Value::Object(obj) => obj.get("content").and_then(Value::as_str), + _ => None, + }) + .expect("output text present"); + assert_eq!(output_text, "E1: [file] alpha.txt\nE2: [dir] nested"); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "disabled until we enable list_dir tool"] +async fn list_dir_tool_depth_two_includes_children_only() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + + let TestCodex { + codex, + cwd, + session_configured, + .. + } = test_codex().build(&server).await?; + + let dir_path = cwd.path().join("depth_two"); + std::fs::create_dir(&dir_path)?; + std::fs::write(dir_path.join("alpha.txt"), "alpha")?; + let nested = dir_path.join("nested"); + std::fs::create_dir(&nested)?; + std::fs::write(nested.join("beta.txt"), "beta")?; + let deeper = nested.join("grand"); + std::fs::create_dir(&deeper)?; + std::fs::write(deeper.join("gamma.txt"), "gamma")?; + let dir_path_string = dir_path.to_string_lossy().to_string(); + + let call_id = "list-dir-depth2"; + let arguments = serde_json::json!({ + "dir_path": dir_path_string, + "offset": 1, + "limit": 10, + "depth": 2, + }) + .to_string(); + + let first_response = sse(vec![ + serde_json::json!({ + "type": "response.created", + "response": {"id": "resp-1"} + }), + ev_function_call(call_id, "list_dir", &arguments), + ev_completed("resp-1"), + ]); + responses::mount_sse_once_match(&server, any(), first_response).await; + + let second_response = sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]); + responses::mount_sse_once_match(&server, any(), second_response).await; + + let session_model = session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "list directory contents depth two".into(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.expect("recorded requests"); + let request_bodies = requests + .iter() + .map(|req| req.body_json::().unwrap()) + .collect::>(); + assert!( + !request_bodies.is_empty(), + "expected at least one request body" + ); + + let tool_output_item = request_bodies + .iter() + .find_map(|body| { + body.get("input") + .and_then(Value::as_array) + .and_then(|items| { + items.iter().find(|item| { + item.get("type").and_then(Value::as_str) == Some("function_call_output") + }) + }) + }) + .unwrap_or_else(|| { + panic!("function_call_output item not found in requests: {request_bodies:#?}") + }); + + assert_eq!( + tool_output_item.get("call_id").and_then(Value::as_str), + Some(call_id) + ); + + let output_text = tool_output_item + .get("output") + .and_then(|value| match value { + Value::String(text) => Some(text.as_str()), + Value::Object(obj) => obj.get("content").and_then(Value::as_str), + _ => None, + }) + .expect("output text present"); + assert_eq!( + output_text, + "E1: [file] alpha.txt\nE2: [dir] nested\nE3: [file] nested/beta.txt\nE4: [dir] nested/grand" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[ignore = "disabled until we enable list_dir tool"] +async fn list_dir_tool_depth_three_includes_grandchildren() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + + let TestCodex { + codex, + cwd, + session_configured, + .. + } = test_codex().build(&server).await?; + + let dir_path = cwd.path().join("depth_three"); + std::fs::create_dir(&dir_path)?; + std::fs::write(dir_path.join("alpha.txt"), "alpha")?; + let nested = dir_path.join("nested"); + std::fs::create_dir(&nested)?; + std::fs::write(nested.join("beta.txt"), "beta")?; + let deeper = nested.join("grand"); + std::fs::create_dir(&deeper)?; + std::fs::write(deeper.join("gamma.txt"), "gamma")?; + let dir_path_string = dir_path.to_string_lossy().to_string(); + + let call_id = "list-dir-depth3"; + let arguments = serde_json::json!({ + "dir_path": dir_path_string, + "offset": 1, + "limit": 10, + "depth": 3, + }) + .to_string(); + + let first_response = sse(vec![ + serde_json::json!({ + "type": "response.created", + "response": {"id": "resp-1"} + }), + ev_function_call(call_id, "list_dir", &arguments), + ev_completed("resp-1"), + ]); + responses::mount_sse_once_match(&server, any(), first_response).await; + + let second_response = sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]); + responses::mount_sse_once_match(&server, any(), second_response).await; + + let session_model = session_configured.model.clone(); + + codex + .submit(Op::UserTurn { + items: vec![InputItem::Text { + text: "list directory contents depth three".into(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: session_model, + effort: None, + summary: ReasoningSummary::Auto, + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = server.received_requests().await.expect("recorded requests"); + let request_bodies = requests + .iter() + .map(|req| req.body_json::().unwrap()) + .collect::>(); + assert!( + !request_bodies.is_empty(), + "expected at least one request body" + ); + + let tool_output_item = request_bodies + .iter() + .find_map(|body| { + body.get("input") + .and_then(Value::as_array) + .and_then(|items| { + items.iter().find(|item| { + item.get("type").and_then(Value::as_str) == Some("function_call_output") + }) + }) + }) + .unwrap_or_else(|| { + panic!("function_call_output item not found in requests: {request_bodies:#?}") + }); + + assert_eq!( + tool_output_item.get("call_id").and_then(Value::as_str), + Some(call_id) + ); + + let output_text = tool_output_item + .get("output") + .and_then(|value| match value { + Value::String(text) => Some(text.as_str()), + Value::Object(obj) => obj.get("content").and_then(Value::as_str), + _ => None, + }) + .expect("output text present"); + assert_eq!( + output_text, + "E1: [file] alpha.txt\nE2: [dir] nested\nE3: [file] nested/beta.txt\nE4: [dir] nested/grand\nE5: [file] nested/grand/gamma.txt" + ); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 2abbb6fcf7..04df9423f5 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -10,6 +10,7 @@ mod exec; mod exec_stream_events; mod fork_conversation; mod json_result; +mod list_dir; mod live_cli; mod model_overrides; mod model_tools; diff --git a/codex-rs/core/tests/suite/tools.rs b/codex-rs/core/tests/suite/tools.rs index 27e709f2c3..99c70dbd61 100644 --- a/codex-rs/core/tests/suite/tools.rs +++ b/codex-rs/core/tests/suite/tools.rs @@ -390,19 +390,9 @@ async fn shell_timeout_includes_timeout_prefix_and_metadata() -> Result<()> { ); let stdout = output_json["output"].as_str().unwrap_or_default(); - let timeout_pattern = r"(?s)^Total output lines: \d+ - -command timed out after (?P\d+) milliseconds -line -.*$"; - let captures = assert_regex_match(timeout_pattern, stdout); - let duration_ms = captures - .name("ms") - .and_then(|m| m.as_str().parse::().ok()) - .unwrap_or_default(); assert!( - duration_ms >= timeout_ms, - "expected duration >= configured timeout, got {duration_ms} (timeout {timeout_ms})" + stdout.contains("command timed out"), + "timeout output missing `command timed out`: {stdout}" ); } else { // Fallback: accept the signal classification path to deflake the test. From a0d56541cf123f20f90cbf3ceb5c2f7506a0321e Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 7 Oct 2025 11:34:05 -0700 Subject: [PATCH 16/24] tui: breathing spinner on true-color terms (#4853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit uses the same logic as shimmer_spans to render the `•` spinner. on terminals without true-color support, fall back to the existing `•/◦` blinking logic. https://github.com/user-attachments/assets/19db76f2-8fa2-440d-9fde-7bed67f4c4dc --- codex-rs/tui/src/exec_cell/render.rs | 15 +++++++++++---- codex-rs/tui/src/status_indicator_widget.rs | 9 +++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/codex-rs/tui/src/exec_cell/render.rs b/codex-rs/tui/src/exec_cell/render.rs index 9c1231f0cf..51824ddc3c 100644 --- a/codex-rs/tui/src/exec_cell/render.rs +++ b/codex-rs/tui/src/exec_cell/render.rs @@ -8,6 +8,7 @@ use crate::history_cell::HistoryCell; use crate::render::highlight::highlight_bash_to_lines; use crate::render::line_utils::prefix_lines; use crate::render::line_utils::push_owned_lines; +use crate::shimmer::shimmer_spans; use crate::wrapping::RtOptions; use crate::wrapping::word_wrap_line; use codex_ansi_escape::ansi_escape_line; @@ -116,10 +117,16 @@ pub(crate) fn output_lines( } pub(crate) fn spinner(start_time: Option) -> Span<'static> { - let blink_on = start_time - .map(|st| ((st.elapsed().as_millis() / 600) % 2) == 0) - .unwrap_or(false); - if blink_on { "•".into() } else { "◦".dim() } + let elapsed = start_time.map(|st| st.elapsed()).unwrap_or_default(); + if supports_color::on_cached(supports_color::Stream::Stdout) + .map(|level| level.has_16m) + .unwrap_or(false) + { + shimmer_spans("•")[0].clone() + } else { + let blink_on = (elapsed.as_millis() / 600).is_multiple_of(2); + if blink_on { "•".into() } else { "◦".dim() } + } } impl HistoryCell for ExecCell { diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index e60204cdb8..ce4f6eabd8 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -15,6 +15,7 @@ use ratatui::widgets::WidgetRef; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; +use crate::exec_cell::spinner; use crate::key_hint; use crate::shimmer::shimmer_spans; use crate::tui::FrameRequester; @@ -163,15 +164,11 @@ impl WidgetRef for StatusIndicatorWidget { let now = Instant::now(); let elapsed_duration = self.elapsed_duration_at(now); let pretty_elapsed = fmt_elapsed_compact(elapsed_duration.as_secs()); - let blink_on = (elapsed_duration.as_millis() / 600).is_multiple_of(2); // Plain rendering: no borders or padding so the live cell is visually indistinguishable from terminal scrollback. let mut spans = Vec::with_capacity(5); - if blink_on { - spans.push("• ".into()); - } else { - spans.push("◦ ".dim()); - } + spans.push(spinner(Some(self.last_resume_at))); + spans.push(" ".into()); spans.extend(shimmer_spans(&self.header)); spans.extend(vec![ " ".into(), From b016a3e7d8cf56a4352e933bd89f8127b7e5b7ee Mon Sep 17 00:00:00 2001 From: dedrisian-oai Date: Tue, 7 Oct 2025 12:47:00 -0700 Subject: [PATCH 17/24] Remove instruction hack for /review (#4896) We use to put the review prompt in the first user message as well to bypass statsig overrides, but now that's been resolved and instructions are being respected, so we're duplicating the review instructions. --- codex-rs/core/src/codex.rs | 2 +- codex-rs/core/tests/suite/review.rs | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7d7bd60ac5..d11a53b59a 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1586,7 +1586,7 @@ async fn spawn_review_thread( // Seed the child task with the review prompt as the initial user message. let input: Vec = vec![InputItem::Text { - text: format!("{base_instructions}\n\n---\n\nNow, here's your task: {review_prompt}"), + text: review_prompt, }]; let tc = Arc::new(review_turn_context); diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 66b67ae980..f3eeb3a310 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -445,7 +445,7 @@ async fn review_input_isolated_from_parent_history() { .await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; - // Assert the request `input` contains the environment context followed by the review prompt. + // Assert the request `input` contains the environment context followed by the user review prompt. let request = &server.received_requests().await.unwrap()[0]; let body = request.body_json::().unwrap(); let input = body["input"].as_array().expect("input array"); @@ -473,9 +473,14 @@ async fn review_input_isolated_from_parent_history() { assert_eq!(review_msg["role"].as_str().unwrap(), "user"); assert_eq!( review_msg["content"][0]["text"].as_str().unwrap(), - format!("{REVIEW_PROMPT}\n\n---\n\nNow, here's your task: Please review only this",) + review_prompt, + "user message should only contain the raw review prompt" ); + // Ensure the REVIEW_PROMPT rubric is sent via instructions. + let instructions = body["instructions"].as_str().expect("instructions string"); + assert_eq!(instructions, REVIEW_PROMPT); + // Also verify that a user interruption note was recorded in the rollout. codex.submit(Op::GetPath).await.unwrap(); let history_event = From 60f9e85c16230ad62b543f9c52065f3c886f93e9 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 7 Oct 2025 14:06:41 -0700 Subject: [PATCH 18/24] Set codex SDK TypeScript originator (#4894) ## Summary - ensure the TypeScript SDK sets CODEX_INTERNAL_ORIGINATOR_OVERRIDE to codex_sdk_ts when spawning the Codex CLI - extend the responses proxy test helper to capture request headers for assertions - add coverage that verifies Codex threads launched from the TypeScript SDK send the codex_sdk_ts originator header ## Testing - Not Run (not requested) ------ https://chatgpt.com/codex/tasks/task_i_68e561b125248320a487f129093d16e7 --- codex-rs/Cargo.lock | 2 +- codex-rs/core/src/default_client.rs | 28 +++++-------- codex-rs/exec/src/lib.rs | 2 +- codex-rs/exec/tests/suite/mod.rs | 1 + codex-rs/exec/tests/suite/originator.rs | 52 +++++++++++++++++++++++++ sdk/typescript/src/exec.ts | 6 +++ sdk/typescript/tests/responsesProxy.ts | 3 +- sdk/typescript/tests/run.test.ts | 24 ++++++++++++ 8 files changed, 97 insertions(+), 21 deletions(-) create mode 100644 codex-rs/exec/tests/suite/originator.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1884dc7210..3375f6d89f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -992,7 +992,7 @@ dependencies = [ "tokio-stream", "tracing", "tracing-subscriber", - "unicode-width 0.1.14", + "unicode-width 0.2.1", ] [[package]] diff --git a/codex-rs/core/src/default_client.rs b/codex-rs/core/src/default_client.rs index 5a6ea8d826..42690e04cc 100644 --- a/codex-rs/core/src/default_client.rs +++ b/codex-rs/core/src/default_client.rs @@ -20,7 +20,7 @@ use std::sync::OnceLock; /// The full user agent string is returned from the mcp initialize response. /// Parenthesis will be added by Codex. This should only specify what goes inside of the parenthesis. pub static USER_AGENT_SUFFIX: LazyLock>> = LazyLock::new(|| Mutex::new(None)); - +pub const DEFAULT_ORIGINATOR: &str = "codex_cli_rs"; pub const CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR: &str = "CODEX_INTERNAL_ORIGINATOR_OVERRIDE"; #[derive(Debug, Clone)] pub struct Originator { @@ -35,10 +35,11 @@ pub enum SetOriginatorError { AlreadyInitialized, } -fn init_originator_from_env() -> Originator { - let default = "codex_cli_rs"; +fn get_originator_value(provided: Option) -> Originator { let value = std::env::var(CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR) - .unwrap_or_else(|_| default.to_string()); + .ok() + .or(provided) + .unwrap_or(DEFAULT_ORIGINATOR.to_string()); match HeaderValue::from_str(&value) { Ok(header_value) => Originator { @@ -48,31 +49,22 @@ fn init_originator_from_env() -> Originator { Err(e) => { tracing::error!("Unable to turn originator override {value} into header value: {e}"); Originator { - value: default.to_string(), - header_value: HeaderValue::from_static(default), + value: DEFAULT_ORIGINATOR.to_string(), + header_value: HeaderValue::from_static(DEFAULT_ORIGINATOR), } } } } -fn build_originator(value: String) -> Result { - let header_value = - HeaderValue::from_str(&value).map_err(|_| SetOriginatorError::InvalidHeaderValue)?; - Ok(Originator { - value, - header_value, - }) -} - -pub fn set_default_originator(value: &str) -> Result<(), SetOriginatorError> { - let originator = build_originator(value.to_string())?; +pub fn set_default_originator(value: String) -> Result<(), SetOriginatorError> { + let originator = get_originator_value(Some(value)); ORIGINATOR .set(originator) .map_err(|_| SetOriginatorError::AlreadyInitialized) } pub fn originator() -> &'static Originator { - ORIGINATOR.get_or_init(init_originator_from_env) + ORIGINATOR.get_or_init(|| get_originator_value(None)) } pub fn get_codex_user_agent() -> String { diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 18e097782d..967da52b8a 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -48,7 +48,7 @@ use codex_core::default_client::set_default_originator; use codex_core::find_conversation_path_by_id_str; pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> anyhow::Result<()> { - if let Err(err) = set_default_originator("codex_exec") { + if let Err(err) = set_default_originator("codex_exec".to_string()) { tracing::warn!(?err, "Failed to set codex exec originator override {err:?}"); } diff --git a/codex-rs/exec/tests/suite/mod.rs b/codex-rs/exec/tests/suite/mod.rs index d04ecd2c93..052c43bf38 100644 --- a/codex-rs/exec/tests/suite/mod.rs +++ b/codex-rs/exec/tests/suite/mod.rs @@ -1,6 +1,7 @@ // Aggregates all former standalone integration tests as modules. mod apply_patch; mod auth_env; +mod originator; mod output_schema; mod resume; mod sandbox; diff --git a/codex-rs/exec/tests/suite/originator.rs b/codex-rs/exec/tests/suite/originator.rs new file mode 100644 index 0000000000..cf0954447a --- /dev/null +++ b/codex-rs/exec/tests/suite/originator.rs @@ -0,0 +1,52 @@ +#![cfg(not(target_os = "windows"))] +#![allow(clippy::expect_used, clippy::unwrap_used)] + +use core_test_support::responses; +use core_test_support::test_codex_exec::test_codex_exec; +use wiremock::matchers::header; + +/// Verify that when the server reports an error, `codex-exec` exits with a +/// non-zero status code so automation can detect failures. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn send_codex_exec_originator() -> anyhow::Result<()> { + let test = test_codex_exec(); + + let server = responses::start_mock_server().await; + let body = responses::sse(vec![ + responses::ev_response_created("response_1"), + responses::ev_assistant_message("response_1", "Hello, world!"), + responses::ev_completed("response_1"), + ]); + responses::mount_sse_once_match(&server, header("Originator", "codex_exec"), body).await; + + test.cmd_with_server(&server) + .arg("--skip-git-repo-check") + .arg("tell me something") + .assert() + .code(0); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn supports_originator_override() -> anyhow::Result<()> { + let test = test_codex_exec(); + + let server = responses::start_mock_server().await; + let body = responses::sse(vec![ + responses::ev_response_created("response_1"), + responses::ev_assistant_message("response_1", "Hello, world!"), + responses::ev_completed("response_1"), + ]); + responses::mount_sse_once_match(&server, header("Originator", "codex_exec_override"), body) + .await; + + test.cmd_with_server(&server) + .env("CODEX_INTERNAL_ORIGINATOR_OVERRIDE", "codex_exec_override") + .arg("--skip-git-repo-check") + .arg("tell me something") + .assert() + .code(0); + + Ok(()) +} diff --git a/sdk/typescript/src/exec.ts b/sdk/typescript/src/exec.ts index ac9e548b93..b1cf4c414a 100644 --- a/sdk/typescript/src/exec.ts +++ b/sdk/typescript/src/exec.ts @@ -23,6 +23,9 @@ export type CodexExecArgs = { outputSchemaFile?: string; }; +const INTERNAL_ORIGINATOR_ENV = "CODEX_INTERNAL_ORIGINATOR_OVERRIDE"; +const TYPESCRIPT_SDK_ORIGINATOR = "codex_sdk_ts"; + export class CodexExec { private executablePath: string; constructor(executablePath: string | null = null) { @@ -59,6 +62,9 @@ export class CodexExec { const env = { ...process.env, }; + if (!env[INTERNAL_ORIGINATOR_ENV]) { + env[INTERNAL_ORIGINATOR_ENV] = TYPESCRIPT_SDK_ORIGINATOR; + } if (args.baseUrl) { env.OPENAI_BASE_URL = args.baseUrl; } diff --git a/sdk/typescript/tests/responsesProxy.ts b/sdk/typescript/tests/responsesProxy.ts index 9f0060d37f..6b8ee9bcac 100644 --- a/sdk/typescript/tests/responsesProxy.ts +++ b/sdk/typescript/tests/responsesProxy.ts @@ -54,6 +54,7 @@ export type ResponsesApiRequest = { export type RecordedRequest = { body: string; json: ResponsesApiRequest; + headers: http.IncomingHttpHeaders; }; function formatSseEvent(event: SseEvent): string { @@ -90,7 +91,7 @@ export async function startResponsesTestProxy( if (req.method === "POST" && req.url === "/responses") { const body = await readRequestBody(req); const json = JSON.parse(body); - requests.push({ body, json }); + requests.push({ body, json, headers: { ...req.headers } }); const status = options.statusCode ?? 200; res.statusCode = status; diff --git a/sdk/typescript/tests/run.test.ts b/sdk/typescript/tests/run.test.ts index b711e5c506..de7ef5555d 100644 --- a/sdk/typescript/tests/run.test.ts +++ b/sdk/typescript/tests/run.test.ts @@ -345,6 +345,30 @@ describe("Codex", () => { await close(); } }); + + it("sets the codex sdk originator header", async () => { + const { url, close, requests } = await startResponsesTestProxy({ + statusCode: 200, + responseBodies: [sse(responseStarted(), assistantMessage("Hi!"), responseCompleted())], + }); + + try { + const client = new Codex({ codexPathOverride: codexExecPath, baseUrl: url, apiKey: "test" }); + + const thread = client.startThread(); + await thread.run("Hello, originator!"); + + expect(requests.length).toBeGreaterThan(0); + const originatorHeader = requests[0]!.headers["originator"]; + if (Array.isArray(originatorHeader)) { + expect(originatorHeader).toContain("codex_sdk_ts"); + } else { + expect(originatorHeader).toBe("codex_sdk_ts"); + } + } finally { + await close(); + } + }); it("throws ThreadRunError on turn failures", async () => { const { url, close } = await startResponsesTestProxy({ statusCode: 200, From 0e5d72cc57dad5c78c9a6120dc53faebbc599f63 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 7 Oct 2025 16:18:48 -0700 Subject: [PATCH 19/24] tui: bring the transcript closer to display mode (#4848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit before Screenshot 2025-10-06 at 3 06 52 PM after Screenshot 2025-10-06 at 3 07 02 PM --- codex-rs/tui/src/app_backtrack.rs | 3 +- codex-rs/tui/src/bottom_pane/chat_composer.rs | 3 +- .../src/bottom_pane/list_selection_view.rs | 3 +- codex-rs/tui/src/custom_terminal.rs | 34 ++-- codex-rs/tui/src/diff_render.rs | 2 +- codex-rs/tui/src/exec_cell/render.rs | 28 +-- codex-rs/tui/src/history_cell.rs | 168 ++++++++---------- codex-rs/tui/src/pager_overlay.rs | 114 +++++++----- codex-rs/tui/src/render/mod.rs | 10 +- codex-rs/tui/src/render/renderable.rs | 24 ++- codex-rs/tui/src/session_log.rs | 2 +- codex-rs/tui/src/streaming/controller.rs | 22 +-- codex-rs/tui/src/style.rs | 7 +- 13 files changed, 233 insertions(+), 187 deletions(-) diff --git a/codex-rs/tui/src/app_backtrack.rs b/codex-rs/tui/src/app_backtrack.rs index b5c1300b45..5528fa7bc4 100644 --- a/codex-rs/tui/src/app_backtrack.rs +++ b/codex-rs/tui/src/app_backtrack.rs @@ -134,8 +134,9 @@ impl App { /// Useful when switching sessions to ensure prior history remains visible. pub(crate) fn render_transcript_once(&mut self, tui: &mut tui::Tui) { if !self.transcript_cells.is_empty() { + let width = tui.terminal.last_known_screen_size.width; for cell in &self.transcript_cells { - tui.insert_history_lines(cell.transcript_lines()); + tui.insert_history_lines(cell.display_lines(width)); } } } diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index ad9770726a..2574ded461 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -38,7 +38,6 @@ use crate::bottom_pane::prompt_args::prompt_has_numeric_placeholders; use crate::slash_command::SlashCommand; use crate::slash_command::built_in_slash_commands; use crate::style::user_message_style; -use crate::terminal_palette; use codex_protocol::custom_prompts::CustomPrompt; use codex_protocol::custom_prompts::PROMPTS_CMD_PREFIX; @@ -1533,7 +1532,7 @@ impl WidgetRef for ChatComposer { } } } - let style = user_message_style(terminal_palette::default_bg()); + let style = user_message_style(); let mut block_rect = composer_rect; block_rect.y = composer_rect.y.saturating_sub(1); block_rect.height = composer_rect.height.saturating_add(1); diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 9f0ce3df24..238f74d6e4 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -20,7 +20,6 @@ use crate::render::RectExt as _; use crate::render::renderable::ColumnRenderable; use crate::render::renderable::Renderable; use crate::style::user_message_style; -use crate::terminal_palette; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; @@ -350,7 +349,7 @@ impl Renderable for ListSelectionView { .areas(area); Block::default() - .style(user_message_style(terminal_palette::default_bg())) + .style(user_message_style()) .render(content_area, buf); let header_height = self diff --git a/codex-rs/tui/src/custom_terminal.rs b/codex-rs/tui/src/custom_terminal.rs index aa79e980c1..bbd8900632 100644 --- a/codex-rs/tui/src/custom_terminal.rs +++ b/codex-rs/tui/src/custom_terminal.rs @@ -120,6 +120,8 @@ where /// Last known position of the cursor. Used to find the new area when the viewport is inlined /// and the terminal resized. pub last_known_cursor_pos: Position, + + use_custom_flush: bool, } impl Drop for Terminal @@ -158,6 +160,7 @@ where viewport_area: Rect::new(0, cursor_pos.y, 0, 0), last_known_screen_size: screen_size, last_known_cursor_pos: cursor_pos, + use_custom_flush: true, }) } @@ -190,15 +193,24 @@ where pub fn flush(&mut self) -> io::Result<()> { let previous_buffer = &self.buffers[1 - self.current]; let current_buffer = &self.buffers[self.current]; - let updates = diff_buffers(previous_buffer, current_buffer); - if let Some(DrawCommand::Put { x, y, .. }) = updates - .iter() - .rev() - .find(|cmd| matches!(cmd, DrawCommand::Put { .. })) - { - self.last_known_cursor_pos = Position { x: *x, y: *y }; + + if self.use_custom_flush { + let updates = diff_buffers(previous_buffer, current_buffer); + if let Some(DrawCommand::Put { x, y, .. }) = updates + .iter() + .rev() + .find(|cmd| matches!(cmd, DrawCommand::Put { .. })) + { + self.last_known_cursor_pos = Position { x: *x, y: *y }; + } + draw(&mut self.backend, updates.into_iter()) + } else { + let updates = previous_buffer.diff(current_buffer); + if let Some((x, y, _)) = updates.last() { + self.last_known_cursor_pos = Position { x: *x, y: *y }; + } + self.backend.draw(updates.into_iter()) } - draw(&mut self.backend, updates.into_iter()) } /// Updates the Terminal so that internal buffers match the requested area. @@ -408,11 +420,13 @@ fn diff_buffers<'a>(a: &'a Buffer, b: &'a Buffer) -> Vec> { let x = row .iter() - .rposition(|cell| cell.symbol() != " " || cell.bg != bg) + .rposition(|cell| { + cell.symbol() != " " || cell.bg != bg || cell.modifier != Modifier::empty() + }) .unwrap_or(0); last_nonblank_column[y as usize] = x as u16; - let (x_abs, y_abs) = a.pos_of(row_start + x + 1); if x < (a.area.width as usize).saturating_sub(1) { + let (x_abs, y_abs) = a.pos_of(row_start + x + 1); updates.push(DrawCommand::ClearToEnd { x: x_abs, y: y_abs, diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 676b0559f8..a4b7c87f73 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -67,7 +67,7 @@ impl From for Box { rows.push(Box::new(path)); rows.push(Box::new(RtLine::from(""))); rows.push(Box::new(InsetRenderable::new( - Box::new(row.change), + row.change, Insets::tlbr(0, 2, 0, 0), ))); } diff --git a/codex-rs/tui/src/exec_cell/render.rs b/codex-rs/tui/src/exec_cell/render.rs index 51824ddc3c..a3cc8cacae 100644 --- a/codex-rs/tui/src/exec_cell/render.rs +++ b/codex-rs/tui/src/exec_cell/render.rs @@ -11,6 +11,7 @@ use crate::render::line_utils::push_owned_lines; use crate::shimmer::shimmer_spans; use crate::wrapping::RtOptions; use crate::wrapping::word_wrap_line; +use crate::wrapping::word_wrap_lines; use codex_ansi_escape::ansi_escape_line; use codex_common::elapsed::format_duration; use codex_protocol::parse_command::ParsedCommand; @@ -138,17 +139,25 @@ impl HistoryCell for ExecCell { } } - fn transcript_lines(&self) -> Vec> { + fn desired_transcript_height(&self, width: u16) -> u16 { + self.transcript_lines(width).len() as u16 + } + + fn transcript_lines(&self, width: u16) -> Vec> { let mut lines: Vec> = vec![]; - for call in self.iter_calls() { - let cmd_display = strip_bash_lc_and_escape(&call.command); - for (i, part) in cmd_display.lines().enumerate() { - if i == 0 { - lines.push(vec!["$ ".magenta(), part.to_string().into()].into()); - } else { - lines.push(vec![" ".into(), part.to_string().into()].into()); - } + for (i, call) in self.iter_calls().enumerate() { + if i > 0 { + lines.push("".into()); } + let script = strip_bash_lc_and_escape(&call.command); + let highlighted_script = highlight_bash_to_lines(&script); + let cmd_display = word_wrap_lines( + &highlighted_script, + RtOptions::new(width as usize) + .initial_indent("$ ".magenta().into()) + .subsequent_indent(" ".into()), + ); + lines.extend(cmd_display); if let Some(output) = call.output.as_ref() { lines.extend(output.formatted_output.lines().map(ansi_escape_line)); @@ -167,7 +176,6 @@ impl HistoryCell for ExecCell { result.push_span(format!(" • {duration}").dim()); lines.push(result); } - lines.push("".into()); } lines } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 61640c3334..7c4d18e1a3 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -11,7 +11,6 @@ use crate::markdown::append_markdown; use crate::render::line_utils::line_to_static; use crate::render::line_utils::prefix_lines; use crate::style::user_message_style; -use crate::terminal_palette::default_bg; use crate::text_formatting::format_and_truncate_tool_result; use crate::ui_consts::LIVE_PREFIX_COLS; use crate::wrapping::RtOptions; @@ -56,10 +55,6 @@ use unicode_width::UnicodeWidthStr; pub(crate) trait HistoryCell: std::fmt::Debug + Send + Sync + Any { fn display_lines(&self, width: u16) -> Vec>; - fn transcript_lines(&self) -> Vec> { - self.display_lines(u16::MAX) - } - fn desired_height(&self, width: u16) -> u16 { Paragraph::new(Text::from(self.display_lines(width))) .wrap(Wrap { trim: false }) @@ -68,6 +63,29 @@ pub(crate) trait HistoryCell: std::fmt::Debug + Send + Sync + Any { .unwrap_or(0) } + fn transcript_lines(&self, width: u16) -> Vec> { + self.display_lines(width) + } + + fn desired_transcript_height(&self, width: u16) -> u16 { + let lines = self.transcript_lines(width); + // Workaround for ratatui bug: if there's only one line and it's whitespace-only, ratatui gives 2 lines. + if let [line] = &lines[..] + && line + .spans + .iter() + .all(|s| s.content.chars().all(char::is_whitespace)) + { + return 1; + } + + Paragraph::new(Text::from(lines)) + .wrap(Wrap { trim: false }) + .line_count(width) + .try_into() + .unwrap_or(0) + } + fn is_stream_continuation(&self) -> bool { false } @@ -92,12 +110,10 @@ impl HistoryCell for UserHistoryCell { fn display_lines(&self, width: u16) -> Vec> { let mut lines: Vec> = Vec::new(); - // Use ratatui-aware word wrapping and prefixing to avoid lifetime issues. - let wrap_width = width.saturating_sub(LIVE_PREFIX_COLS); // account for the ▌ prefix and trailing space + let wrap_width = width.saturating_sub(LIVE_PREFIX_COLS); - let style = user_message_style(default_bg()); + let style = user_message_style(); - // Use our ratatui wrapping helpers for correct styling and lifetimes. let wrapped = word_wrap_lines( &self .message @@ -113,13 +129,6 @@ impl HistoryCell for UserHistoryCell { lines.push(Line::from("").style(style)); lines } - - fn transcript_lines(&self) -> Vec> { - let mut lines: Vec> = Vec::new(); - lines.push("user".cyan().bold().into()); - lines.extend(self.message.lines().map(|l| l.to_string().into())); - lines - } } #[derive(Debug)] @@ -127,6 +136,7 @@ pub(crate) struct ReasoningSummaryCell { _header: String, content: String, citation_context: MarkdownCitationContext, + transcript_only: bool, } impl ReasoningSummaryCell { @@ -134,17 +144,17 @@ impl ReasoningSummaryCell { header: String, content: String, citation_context: MarkdownCitationContext, + transcript_only: bool, ) -> Self { Self { _header: header, content, citation_context, + transcript_only, } } -} -impl HistoryCell for ReasoningSummaryCell { - fn display_lines(&self, width: u16) -> Vec> { + fn lines(&self, width: u16) -> Vec> { let mut lines: Vec> = Vec::new(); append_markdown( &self.content, @@ -152,7 +162,7 @@ impl HistoryCell for ReasoningSummaryCell { &mut lines, self.citation_context.clone(), ); - let summary_style = Style::default().add_modifier(Modifier::DIM | Modifier::ITALIC); + let summary_style = Style::default().dim().italic(); let summary_lines = lines .into_iter() .map(|mut line| { @@ -172,19 +182,31 @@ impl HistoryCell for ReasoningSummaryCell { .subsequent_indent(" ".into()), ) } +} - fn transcript_lines(&self) -> Vec> { - let mut out: Vec> = Vec::new(); - out.push("thinking".magenta().bold().into()); - let mut lines = Vec::new(); - append_markdown( - &self.content, - None, - &mut lines, - self.citation_context.clone(), - ); - out.extend(lines); - out +impl HistoryCell for ReasoningSummaryCell { + fn display_lines(&self, width: u16) -> Vec> { + if self.transcript_only { + Vec::new() + } else { + self.lines(width) + } + } + + fn desired_height(&self, width: u16) -> u16 { + if self.transcript_only { + 0 + } else { + self.lines(width).len() as u16 + } + } + + fn transcript_lines(&self, width: u16) -> Vec> { + self.lines(width) + } + + fn desired_transcript_height(&self, width: u16) -> u16 { + self.lines(width).len() as u16 } } @@ -217,15 +239,6 @@ impl HistoryCell for AgentMessageCell { ) } - fn transcript_lines(&self) -> Vec> { - let mut out: Vec> = Vec::new(); - if self.is_first_line { - out.push("codex".magenta().bold().into()); - } - out.extend(self.lines.clone()); - out - } - fn is_stream_continuation(&self) -> bool { !self.is_first_line } @@ -248,21 +261,6 @@ impl HistoryCell for PlainHistoryCell { } } -#[derive(Debug)] -pub(crate) struct TranscriptOnlyHistoryCell { - lines: Vec>, -} - -impl HistoryCell for TranscriptOnlyHistoryCell { - fn display_lines(&self, _width: u16) -> Vec> { - Vec::new() - } - - fn transcript_lines(&self) -> Vec> { - self.lines.clone() - } -} - /// Cyan history cell line showing the current review status. pub(crate) fn new_review_status_line(message: String) -> PlainHistoryCell { PlainHistoryCell { @@ -1050,16 +1048,6 @@ pub(crate) fn new_view_image_tool_call(path: PathBuf, cwd: &Path) -> PlainHistor PlainHistoryCell { lines } } -pub(crate) fn new_reasoning_block( - full_reasoning_buffer: String, - config: &Config, -) -> TranscriptOnlyHistoryCell { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("thinking".magenta().italic())); - append_markdown(&full_reasoning_buffer, None, &mut lines, config); - TranscriptOnlyHistoryCell { lines } -} - pub(crate) fn new_reasoning_summary_block( full_reasoning_buffer: String, config: &Config, @@ -1085,12 +1073,18 @@ pub(crate) fn new_reasoning_summary_block( header_buffer, summary_buffer, config.into(), + false, )); } } } } - Box::new(new_reasoning_block(full_reasoning_buffer, config)) + Box::new(ReasoningSummaryCell::new( + "".to_string(), + full_reasoning_buffer, + config.into(), + true, + )) } #[derive(Debug)] @@ -1121,10 +1115,6 @@ impl HistoryCell for FinalMessageSeparator { vec![Line::from_iter(["─".repeat(width as usize).dim()])] } } - - fn transcript_lines(&self) -> Vec> { - vec![] - } } fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> { @@ -1188,7 +1178,14 @@ mod tests { } fn render_transcript(cell: &dyn HistoryCell) -> Vec { - render_lines(&cell.transcript_lines()) + render_lines(&cell.transcript_lines(u16::MAX)) + } + + #[test] + fn empty_agent_message_cell_transcript() { + let cell = AgentMessageCell::new(vec![Line::default()], false); + assert_eq!(cell.transcript_lines(80), vec![Line::from(" ")]); + assert_eq!(cell.desired_transcript_height(80), 1); } #[test] @@ -1883,10 +1880,7 @@ mod tests { assert_eq!(rendered_display, vec!["• Detailed reasoning goes here."]); let rendered_transcript = render_transcript(cell.as_ref()); - assert_eq!( - rendered_transcript, - vec!["thinking", "Detailed reasoning goes here."] - ); + assert_eq!(rendered_transcript, vec!["• Detailed reasoning goes here."]); } #[test] @@ -1898,7 +1892,7 @@ mod tests { new_reasoning_summary_block("Detailed reasoning goes here.".to_string(), &config); let rendered = render_transcript(cell.as_ref()); - assert_eq!(rendered, vec!["thinking", "Detailed reasoning goes here."]); + assert_eq!(rendered, vec!["• Detailed reasoning goes here."]); } #[test] @@ -1912,10 +1906,7 @@ mod tests { ); let rendered = render_transcript(cell.as_ref()); - assert_eq!( - rendered, - vec!["thinking", "**High level reasoning without closing"] - ); + assert_eq!(rendered, vec!["• **High level reasoning without closing"]); } #[test] @@ -1929,10 +1920,7 @@ mod tests { ); let rendered = render_transcript(cell.as_ref()); - assert_eq!( - rendered, - vec!["thinking", "High level reasoning without closing"] - ); + assert_eq!(rendered, vec!["• High level reasoning without closing"]); let cell = new_reasoning_summary_block( "**High level reasoning without closing**\n\n ".to_string(), @@ -1940,10 +1928,7 @@ mod tests { ); let rendered = render_transcript(cell.as_ref()); - assert_eq!( - rendered, - vec!["thinking", "High level reasoning without closing"] - ); + assert_eq!(rendered, vec!["• High level reasoning without closing"]); } #[test] @@ -1960,9 +1945,6 @@ mod tests { assert_eq!(rendered_display, vec!["• We should fix the bug next."]); let rendered_transcript = render_transcript(cell.as_ref()); - assert_eq!( - rendered_transcript, - vec!["thinking", "We should fix the bug next."] - ); + assert_eq!(rendered_transcript, vec!["• We should fix the bug next."]); } } diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index 7997625aae..46c6c19a18 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -3,9 +3,13 @@ use std::sync::Arc; use std::time::Duration; use crate::history_cell::HistoryCell; +use crate::history_cell::UserHistoryCell; use crate::key_hint; use crate::key_hint::KeyBinding; +use crate::render::Insets; +use crate::render::renderable::InsetRenderable; use crate::render::renderable::Renderable; +use crate::style::user_message_style; use crate::tui; use crate::tui::TuiEvent; use crossterm::event::KeyCode; @@ -13,6 +17,7 @@ use crossterm::event::KeyEvent; use ratatui::buffer::Buffer; use ratatui::buffer::Cell; use ratatui::layout::Rect; +use ratatui::style::Style; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; @@ -21,7 +26,6 @@ use ratatui::widgets::Clear; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; -use ratatui::widgets::Wrap; pub(crate) enum Overlay { Transcript(TranscriptOverlay), @@ -317,29 +321,30 @@ impl PagerView { } } -struct CachedParagraph { - paragraph: Paragraph<'static>, +/// A renderable that caches its desired height. +struct CachedRenderable { + renderable: Box, height: std::cell::Cell>, last_width: std::cell::Cell>, } -impl CachedParagraph { - fn new(paragraph: Paragraph<'static>) -> Self { +impl CachedRenderable { + fn new(renderable: Box) -> Self { Self { - paragraph, + renderable, height: std::cell::Cell::new(None), last_width: std::cell::Cell::new(None), } } } -impl Renderable for CachedParagraph { +impl Renderable for CachedRenderable { fn render(&self, area: Rect, buf: &mut Buffer) { - self.paragraph.render_ref(area, buf); + self.renderable.render(area, buf); } fn desired_height(&self, width: u16) -> u16 { if self.last_width.get() != Some(width) { - let height = self.paragraph.line_count(width) as u16; + let height = self.renderable.desired_height(width); self.height.set(Some(height)); self.last_width.set(Some(width)); } @@ -347,6 +352,23 @@ impl Renderable for CachedParagraph { } } +struct CellRenderable { + cell: Arc, + style: Style, +} + +impl Renderable for CellRenderable { + fn render(&self, area: Rect, buf: &mut Buffer) { + let p = + Paragraph::new(Text::from(self.cell.transcript_lines(area.width))).style(self.style); + p.render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + self.cell.desired_transcript_height(width) + } +} + pub(crate) struct TranscriptOverlay { view: PagerView, cells: Vec>, @@ -358,7 +380,7 @@ impl TranscriptOverlay { pub(crate) fn new(transcript_cells: Vec>) -> Self { Self { view: PagerView::new( - Self::render_cells_to_texts(&transcript_cells, None), + Self::render_cells(&transcript_cells, None), "T R A N S C R I P T".to_string(), usize::MAX, ), @@ -368,46 +390,46 @@ impl TranscriptOverlay { } } - fn render_cells_to_texts( + fn render_cells( cells: &[Arc], highlight_cell: Option, ) -> Vec> { - let mut texts: Vec> = Vec::new(); - let mut first = true; - for (idx, cell) in cells.iter().enumerate() { - let mut lines: Vec> = Vec::new(); - if !cell.is_stream_continuation() && !first { - lines.push(Line::from("")); - } - let cell_lines = if Some(idx) == highlight_cell { - cell.transcript_lines() - .into_iter() - .map(Stylize::reversed) - .collect() - } else { - cell.transcript_lines() - }; - lines.extend(cell_lines); - texts.push(Box::new(CachedParagraph::new( - Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }), - ))); - first = false; - } - texts + cells + .iter() + .enumerate() + .flat_map(|(i, c)| { + let mut v: Vec> = Vec::new(); + let mut cell_renderable = if c.as_any().is::() { + Box::new(CachedRenderable::new(Box::new(CellRenderable { + cell: c.clone(), + style: if highlight_cell == Some(i) { + user_message_style().reversed() + } else { + user_message_style() + }, + }))) as Box + } else { + Box::new(CachedRenderable::new(Box::new(CellRenderable { + cell: c.clone(), + style: Style::default(), + }))) as Box + }; + if !c.is_stream_continuation() && i > 0 { + cell_renderable = Box::new(InsetRenderable::new( + cell_renderable, + Insets::tlbr(1, 0, 0, 0), + )); + } + v.push(cell_renderable); + v + }) + .collect() } pub(crate) fn insert_cell(&mut self, cell: Arc) { let follow_bottom = self.view.is_scrolled_to_bottom(); - // Append as a new Text chunk (with a separating blank if needed) - let mut lines: Vec> = Vec::new(); - if !cell.is_stream_continuation() && !self.cells.is_empty() { - lines.push(Line::from("")); - } - lines.extend(cell.transcript_lines()); - self.view.renderables.push(Box::new(CachedParagraph::new( - Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }), - ))); self.cells.push(cell); + self.view.renderables = Self::render_cells(&self.cells, self.highlight_cell); if follow_bottom { self.view.scroll_offset = usize::MAX; } @@ -415,7 +437,7 @@ impl TranscriptOverlay { pub(crate) fn set_highlight_cell(&mut self, cell: Option) { self.highlight_cell = cell; - self.view.renderables = Self::render_cells_to_texts(&self.cells, self.highlight_cell); + self.view.renderables = Self::render_cells(&self.cells, self.highlight_cell); if let Some(idx) = self.highlight_cell { self.view.scroll_chunk_into_view(idx); } @@ -475,8 +497,8 @@ pub(crate) struct StaticOverlay { impl StaticOverlay { pub(crate) fn with_title(lines: Vec>, title: String) -> Self { Self::with_renderables( - vec![Box::new(CachedParagraph::new(Paragraph::new(Text::from( - lines, + vec![Box::new(CachedRenderable::new(Box::new(Paragraph::new( + Text::from(lines), ))))], title, ) @@ -585,7 +607,7 @@ mod tests { self.lines.clone() } - fn transcript_lines(&self) -> Vec> { + fn transcript_lines(&self, _width: u16) -> Vec> { self.lines.clone() } } diff --git a/codex-rs/tui/src/render/mod.rs b/codex-rs/tui/src/render/mod.rs index fe92eea91b..bf4fb553c5 100644 --- a/codex-rs/tui/src/render/mod.rs +++ b/codex-rs/tui/src/render/mod.rs @@ -38,11 +38,13 @@ pub trait RectExt { impl RectExt for Rect { fn inset(&self, insets: Insets) -> Rect { + let horizontal = insets.left.saturating_add(insets.right); + let vertical = insets.top.saturating_add(insets.bottom); Rect { - x: self.x + insets.left, - y: self.y + insets.top, - width: self.width - insets.left - insets.right, - height: self.height - insets.top - insets.bottom, + x: self.x.saturating_add(insets.left), + y: self.y.saturating_add(insets.top), + width: self.width.saturating_sub(horizontal), + height: self.height.saturating_sub(vertical), } } } diff --git a/codex-rs/tui/src/render/renderable.rs b/codex-rs/tui/src/render/renderable.rs index c55426337d..62c2720685 100644 --- a/codex-rs/tui/src/render/renderable.rs +++ b/codex-rs/tui/src/render/renderable.rs @@ -1,3 +1,5 @@ +use std::sync::Arc; + use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::text::Line; @@ -12,6 +14,12 @@ pub trait Renderable { fn desired_height(&self, width: u16) -> u16; } +impl From for Box { + fn from(value: R) -> Self { + Box::new(value) + } +} + impl Renderable for () { fn render(&self, _area: Rect, _buf: &mut Buffer) {} fn desired_height(&self, _width: u16) -> u16 { @@ -71,6 +79,15 @@ impl Renderable for Option { } } +impl Renderable for Arc { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.as_ref().render(area, buf); + } + fn desired_height(&self, width: u16) -> u16 { + self.as_ref().desired_height(width) + } +} + pub struct ColumnRenderable { children: Vec>, } @@ -122,7 +139,10 @@ impl Renderable for InsetRenderable { } impl InsetRenderable { - pub fn new(child: Box, insets: Insets) -> Self { - Self { child, insets } + pub fn new(child: impl Into>, insets: Insets) -> Self { + Self { + child: child.into(), + insets, + } } } diff --git a/codex-rs/tui/src/session_log.rs b/codex-rs/tui/src/session_log.rs index 75946edb5c..b2858e8f21 100644 --- a/codex-rs/tui/src/session_log.rs +++ b/codex-rs/tui/src/session_log.rs @@ -141,7 +141,7 @@ pub(crate) fn log_inbound_app_event(event: &AppEvent) { "ts": now_ts(), "dir": "to_tui", "kind": "insert_history_cell", - "lines": cell.transcript_lines().len(), + "lines": cell.transcript_lines(u16::MAX).len(), }); LOGGER.write_json_line(value); } diff --git a/codex-rs/tui/src/streaming/controller.rs b/codex-rs/tui/src/streaming/controller.rs index 372f296a80..319777ec73 100644 --- a/codex-rs/tui/src/streaming/controller.rs +++ b/codex-rs/tui/src/streaming/controller.rs @@ -90,6 +90,7 @@ mod tests { use super::*; use codex_core::config::Config; use codex_core::config::ConfigOverrides; + use pretty_assertions::assert_eq; async fn test_config() -> Config { let overrides = ConfigOverrides { @@ -195,7 +196,7 @@ mod tests { for d in deltas.iter() { ctrl.push(d); while let (Some(cell), idle) = ctrl.on_commit_tick() { - lines.extend(cell.transcript_lines()); + lines.extend(cell.transcript_lines(u16::MAX)); if idle { break; } @@ -203,21 +204,14 @@ mod tests { } // Finalize and flush remaining lines now. if let Some(cell) = ctrl.finalize() { - lines.extend(cell.transcript_lines()); + lines.extend(cell.transcript_lines(u16::MAX)); } - let mut flat = lines; - // Drop leading blank and header line if present. - if !flat.is_empty() && lines_to_plain_strings(&[flat[0].clone()])[0].is_empty() { - flat.remove(0); - } - if !flat.is_empty() { - let s0 = lines_to_plain_strings(&[flat[0].clone()])[0].clone(); - if s0 == "codex" { - flat.remove(0); - } - } - let streamed = lines_to_plain_strings(&flat); + let streamed: Vec<_> = lines_to_plain_strings(&lines) + .into_iter() + // skip • and 2-space indentation + .map(|s| s.chars().skip(2).collect::()) + .collect(); // Full render of the same source let source: String = deltas.iter().copied().collect(); diff --git a/codex-rs/tui/src/style.rs b/codex-rs/tui/src/style.rs index 90a8e7a85c..01861e1e15 100644 --- a/codex-rs/tui/src/style.rs +++ b/codex-rs/tui/src/style.rs @@ -1,12 +1,17 @@ use crate::color::blend; use crate::color::is_light; use crate::color::perceptual_distance; +use crate::terminal_palette::default_bg; use crate::terminal_palette::terminal_palette; use ratatui::style::Color; use ratatui::style::Style; +pub fn user_message_style() -> Style { + user_message_style_for(default_bg()) +} + /// Returns the style for a user-authored message using the provided terminal background. -pub fn user_message_style(terminal_bg: Option<(u8, u8, u8)>) -> Style { +pub fn user_message_style_for(terminal_bg: Option<(u8, u8, u8)>) -> Style { match terminal_bg { Some(bg) => Style::default().bg(user_message_bg(bg)), None => Style::default(), From b8b04514bcd69e1aea05209bb2fa94ea984d4c62 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Tue, 7 Oct 2025 16:20:12 -0700 Subject: [PATCH 20/24] feat(tui): switch to tree-sitter-highlight bash highlighting (#4666) use tree-sitter-highlight instead of custom logic over the tree-sitter tree to highlight bash. --- codex-rs/Cargo.lock | 18 +- codex-rs/Cargo.toml | 5 +- codex-rs/tui/Cargo.toml | 2 + codex-rs/tui/src/render/highlight.rs | 325 +++++++++++++++++---------- 4 files changed, 229 insertions(+), 121 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 3375f6d89f..f23890bf04 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1424,6 +1424,8 @@ dependencies = [ "tracing", "tracing-appender", "tracing-subscriber", + "tree-sitter-bash", + "tree-sitter-highlight", "unicode-segmentation", "unicode-width 0.2.1", "url", @@ -6261,9 +6263,9 @@ dependencies = [ [[package]] name = "tree-sitter" -version = "0.25.9" +version = "0.25.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ccd2a058a86cfece0bf96f7cce1021efef9c8ed0e892ab74639173e5ed7a34fa" +checksum = "78f873475d258561b06f1c595d93308a7ed124d9977cb26b148c2084a4a3cc87" dependencies = [ "cc", "regex", @@ -6283,6 +6285,18 @@ dependencies = [ "tree-sitter-language", ] +[[package]] +name = "tree-sitter-highlight" +version = "0.25.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adc5f880ad8d8f94e88cb81c3557024cf1a8b75e3b504c50481ed4f5a6006ff3" +dependencies = [ + "regex", + "streaming-iterator", + "thiserror 2.0.16", + "tree-sitter", +] + [[package]] name = "tree-sitter-language" version = "0.1.5" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index bcbe844509..ca40b1a536 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -175,8 +175,9 @@ tracing = "0.1.41" tracing-appender = "0.2.3" tracing-subscriber = "0.3.20" tracing-test = "0.2.5" -tree-sitter = "0.25.9" -tree-sitter-bash = "0.25.0" +tree-sitter = "0.25.10" +tree-sitter-bash = "0.25" +tree-sitter-highlight = "0.25.10" ts-rs = "11" unicode-segmentation = "1.12.0" unicode-width = "0.2" diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index f42f555c9e..2c6f32a20a 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -68,6 +68,8 @@ strum_macros = { workspace = true } supports-color = { workspace = true } tempfile = { workspace = true } textwrap = { workspace = true } +tree-sitter-highlight = { workspace = true } +tree-sitter-bash = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", diff --git a/codex-rs/tui/src/render/highlight.rs b/codex-rs/tui/src/render/highlight.rs index 393aa33705..e6d200cc3d 100644 --- a/codex-rs/tui/src/render/highlight.rs +++ b/codex-rs/tui/src/render/highlight.rs @@ -1,81 +1,146 @@ -use codex_core::bash::try_parse_bash; +use ratatui::style::Style; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; +use std::sync::OnceLock; +use tree_sitter_highlight::Highlight; +use tree_sitter_highlight::HighlightConfiguration; +use tree_sitter_highlight::HighlightEvent; +use tree_sitter_highlight::Highlighter; -/// Convert the full bash script into per-line styled content by first -/// computing operator-dimmed spans across the entire script, then splitting -/// by newlines and dimming heredoc body lines. Performs a single parse and -/// reuses it for both highlighting and heredoc detection. -pub(crate) fn highlight_bash_to_lines(script: &str) -> Vec> { - // Parse once; use the tree for both highlighting and heredoc body detection. - let spans: Vec> = if let Some(tree) = try_parse_bash(script) { - // Single walk: collect operator ranges and heredoc rows. - let root = tree.root_node(); - let mut cursor = root.walk(); - let mut stack = vec![root]; - let mut ranges: Vec<(usize, usize)> = Vec::new(); - while let Some(node) = stack.pop() { - if !node.is_named() && !node.is_extra() { - let kind = node.kind(); - let is_quote = matches!(kind, "\"" | "'" | "`"); - let is_whitespace = kind.trim().is_empty(); - if !is_quote && !is_whitespace { - ranges.push((node.start_byte(), node.end_byte())); - } - } else if node.kind() == "heredoc_body" { - ranges.push((node.start_byte(), node.end_byte())); - } - for child in node.children(&mut cursor) { - stack.push(child); - } - } - if ranges.is_empty() { - ranges.push((script.len(), script.len())); - } - ranges.sort_by_key(|(st, _)| *st); - let mut spans: Vec> = Vec::new(); - let mut i = 0usize; - for (start, end) in ranges.into_iter() { - let dim_start = start.max(i); - let dim_end = end; - if dim_start < dim_end { - if dim_start > i { - spans.push(script[i..dim_start].to_string().into()); - } - spans.push(script[dim_start..dim_end].to_string().dim()); - i = dim_end; - } - } - if i < script.len() { - spans.push(script[i..].to_string().into()); - } - spans - } else { - vec![script.to_string().into()] - }; - // Split spans into lines preserving style boundaries and highlights across newlines. - let mut lines: Vec> = vec![Line::from("")]; - for sp in spans { - let style = sp.style; - let text = sp.content.into_owned(); - for (i, part) in text.split('\n').enumerate() { - if i > 0 { - lines.push(Line::from("")); - } - if part.is_empty() { - continue; - } - let span = Span { - style, - content: std::borrow::Cow::Owned(part.to_string()), - }; - if let Some(last) = lines.last_mut() { - last.spans.push(span); - } +// Ref: https://github.com/tree-sitter/tree-sitter-bash/blob/master/queries/highlights.scm +#[derive(Copy, Clone)] +enum BashHighlight { + Comment, + Constant, + Embedded, + Function, + Keyword, + Number, + Operator, + Property, + String, +} + +impl BashHighlight { + const ALL: [Self; 9] = [ + Self::Comment, + Self::Constant, + Self::Embedded, + Self::Function, + Self::Keyword, + Self::Number, + Self::Operator, + Self::Property, + Self::String, + ]; + + const fn as_str(self) -> &'static str { + match self { + Self::Comment => "comment", + Self::Constant => "constant", + Self::Embedded => "embedded", + Self::Function => "function", + Self::Keyword => "keyword", + Self::Number => "number", + Self::Operator => "operator", + Self::Property => "property", + Self::String => "string", } } - lines + + fn style(self) -> Style { + match self { + Self::Comment | Self::Operator | Self::String => Style::default().dim(), + _ => Style::default(), + } + } +} + +static HIGHLIGHT_CONFIG: OnceLock = OnceLock::new(); + +fn highlight_names() -> &'static [&'static str] { + static NAMES: OnceLock<[&'static str; BashHighlight::ALL.len()]> = OnceLock::new(); + NAMES + .get_or_init(|| BashHighlight::ALL.map(BashHighlight::as_str)) + .as_slice() +} + +fn highlight_config() -> &'static HighlightConfiguration { + HIGHLIGHT_CONFIG.get_or_init(|| { + let language = tree_sitter_bash::LANGUAGE.into(); + #[expect(clippy::expect_used)] + let mut config = HighlightConfiguration::new( + language, + "bash", + tree_sitter_bash::HIGHLIGHT_QUERY, + "", + "", + ) + .expect("load bash highlight query"); + config.configure(highlight_names()); + config + }) +} + +fn highlight_for(highlight: Highlight) -> BashHighlight { + BashHighlight::ALL[highlight.0] +} + +fn push_segment(lines: &mut Vec>, segment: &str, style: Option