From bff4435c8058b06850f5d5f2b1c09d8ebe6ce21b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 10 Aug 2025 14:19:27 -0700 Subject: [PATCH 01/16] docs: update the docs to explain how to authenticate on a headless machine (#2121) Users on "headless" machines, such as WSL users, are understandable having trouble authenticating successfully. To date, I have been providing one-off user support on issues such as https://github.com/openai/codex/issues/2000, but we need a more detailed explanation that we can link to so that users can self-serve. This PR aims to provide detailed information that we can link to in response to user issues going forward. That said, it would also be helpful if we employed heuristics to detect this issue at runtime, and/or we should just link to these docs as part of the `codex login` flow. --- README.md | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 89063b7e7b..0c01654d66 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,9 @@ - [Quickstart](#quickstart) - [Installing and running Codex CLI](#installing-and-running-codex-cli) - [Using Codex with your ChatGPT plan](#using-codex-with-your-chatgpt-plan) - - [Connecting through VPS or remote](#connecting-through-vps-or-remote) + - [Connecting on a "Headless" Machine](#connecting-on-a-headless-machine) + - [Authenticate locally and copy your credentials to the "headless" machine](#authenticate-locally-and-copy-your-credentials-to-the-headless-machine) + - [Connecting through VPS or remote](#connecting-through-vps-or-remote) - [Usage-based billing alternative: Use an OpenAI API key](#usage-based-billing-alternative-use-an-openai-api-key) - [Choosing Codex's level of autonomy](#choosing-codexs-level-of-autonomy) - [**1. Read/write**](#1-readwrite) @@ -99,17 +101,47 @@ Each archive contains a single entry with the platform baked into the name (e.g. Codex CLI login

-After you run `codex` select Sign in with ChatGPT. You'll need a Plus, Pro, or Team ChatGPT account, and will get access to our latest models, including `gpt-5`, at no extra cost to your plan. (Enterprise is coming soon.) +Run `codex` and select **Sign in with ChatGPT**. You'll need a Plus, Pro, or Team ChatGPT account, and will get access to our latest models, including `gpt-5`, at no extra cost to your plan. (Enterprise is coming soon.) -> Important: If you've used the Codex CLI before, you'll need to follow these steps to migrate from usage-based billing with your API key: +> Important: If you've used the Codex CLI before, follow these steps to migrate from usage-based billing with your API key: > -> 1. Update the CLI with `codex update` and ensure `codex --version` is greater than 0.13 -> 2. Ensure that there is no `OPENAI_API_KEY` environment variable set. (Check that `env | grep 'OPENAI_API_KEY'` returns empty) +> 1. Update the CLI and ensure `codex --version` is `0.20.0` or later +> 2. Delete `~/.codex/auth.json` (this should be `C:\Users\USERNAME\.codex\auth.json` on Windows) > 3. Run `codex login` again If you encounter problems with the login flow, please comment on [this issue](https://github.com/openai/codex/issues/1243). -### Connecting through VPS or remote +### Connecting on a "Headless" Machine + +Today, the login process entails running a server on `localhost:1455`. If you are on a "headless" server, such as a Docker container or are `ssh`'d into a remote machine, loading `localhost:1455` in the browser on your local machine will not automatically connect to the webserver running on the _headless_ machine, so you must use one of the following workarounds: + +#### Authenticate locally and copy your credentials to the "headless" machine + +The easiest solution is likely to run through the `codex login` process on your local machine such that `localhost:1455` _is_ accessible in your web browser. When you complete the authentication process, an `auth.json` file should be available at `$CODEX_HOME/auth.json` (on Mac/Linux, `$CODEX_HOME` defaults to `~/.codex` whereas on Windows, it defaults to `%USERPROFILE%\.codex`). + +Because the `auth.json` file is not tied to a specific host, once you complete the authentication flow locally, you can copy the `$CODEX_HOME/auth.json` file to the headless machine and then `codex` should "just work" on that machine. Note to copy a file to a Docker container, you can do: + +```shell +# substitute MY_CONTAINER with the name or id of your Docker container: +CONTAINER_HOME=$(docker exec MY_CONTAINER printenv HOME) +docker exec MY_CONTAINER mkdir -p "$CONTAINER_HOME/.codex" +docker cp auth.json MY_CONTAINER:"$CONTAINER_HOME/.codex/auth.json" +``` + +whereas if you are `ssh`'d into a remote machine, you likely want to use [`scp`](https://en.wikipedia.org/wiki/Secure_copy_protocol): + +```shell +ssh user@remote 'mkdir -p ~/.codex' +scp ~/.codex/auth.json user@remote:~/.codex/auth.json +``` + +or try this one-liner: + +```shell +ssh user@remote 'mkdir -p ~/.codex && cat > ~/.codex/auth.json' < ~/.codex/auth.json +``` + +#### Connecting through VPS or remote If you run Codex on a remote machine (VPS/server) without a local browser, the login helper starts a server on `localhost:1455` on the remote host. To complete login in your local browser, forward that port to your machine before starting the login flow: From f146981b734a6cc2de8eceebb94cac8f53b3f694 Mon Sep 17 00:00:00 2001 From: Yaroslav Date: Mon, 11 Aug 2025 03:57:39 +0300 Subject: [PATCH 02/16] =?UTF-8?q?feat:=20add=20JSON=20schema=20sanitizatio?= =?UTF-8?q?n=20for=20MCP=20tools=20to=20ensure=20compatibil=E2=80=A6=20(#1?= =?UTF-8?q?975)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …ity with internal JsonSchema enum Closes: #1973 Co-authored-by: Dylan Hurd --- codex-rs/core/src/openai_tools.rs | 334 +++++++++++++++++++++++++++++- 1 file changed, 333 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/openai_tools.rs b/codex-rs/core/src/openai_tools.rs index 1c92c07c12..ad794c38f1 100644 --- a/codex-rs/core/src/openai_tools.rs +++ b/codex-rs/core/src/openai_tools.rs @@ -1,5 +1,6 @@ use serde::Deserialize; use serde::Serialize; +use serde_json::Value as JsonValue; use serde_json::json; use std::collections::BTreeMap; use std::collections::HashMap; @@ -81,6 +82,8 @@ pub(crate) enum JsonSchema { #[serde(skip_serializing_if = "Option::is_none")] description: Option, }, + /// MCP schema allows "number" | "integer" for Number + #[serde(alias = "integer")] Number { #[serde(skip_serializing_if = "Option::is_none")] description: Option, @@ -296,7 +299,13 @@ pub(crate) fn mcp_tool_to_openai_tool( input_schema.properties = Some(serde_json::Value::Object(serde_json::Map::new())); } - let serialized_input_schema = serde_json::to_value(input_schema)?; + // Serialize to a raw JSON value so we can sanitize schemas coming from MCP + // servers. Some servers omit the top-level or nested `type` in JSON + // Schemas (e.g. using enum/anyOf), or use unsupported variants like + // `integer`. Our internal JsonSchema is a small subset and requires + // `type`, so we coerce/sanitize here for compatibility. + let mut serialized_input_schema = serde_json::to_value(input_schema)?; + sanitize_json_schema(&mut serialized_input_schema); let input_schema = serde_json::from_value::(serialized_input_schema)?; Ok(ResponsesApiTool { @@ -307,6 +316,120 @@ pub(crate) fn mcp_tool_to_openai_tool( }) } +/// Sanitize a JSON Schema (as serde_json::Value) so it can fit our limited +/// JsonSchema enum. This function: +/// - Ensures every schema object has a "type". If missing, infers it from +/// common keywords (properties => object, items => array, enum/const/format => string) +/// and otherwise defaults to "string". +/// - Fills required child fields (e.g. array items, object properties) with +/// permissive defaults when absent. +fn sanitize_json_schema(value: &mut JsonValue) { + match value { + JsonValue::Bool(_) => { + // JSON Schema boolean form: true/false. Coerce to an accept-all string. + *value = json!({ "type": "string" }); + } + JsonValue::Array(arr) => { + for v in arr.iter_mut() { + sanitize_json_schema(v); + } + } + JsonValue::Object(map) => { + // First, recursively sanitize known nested schema holders + if let Some(props) = map.get_mut("properties") { + if let Some(props_map) = props.as_object_mut() { + for (_k, v) in props_map.iter_mut() { + sanitize_json_schema(v); + } + } + } + if let Some(items) = map.get_mut("items") { + sanitize_json_schema(items); + } + // Some schemas use oneOf/anyOf/allOf - sanitize their entries + for combiner in ["oneOf", "anyOf", "allOf", "prefixItems"] { + if let Some(v) = map.get_mut(combiner) { + sanitize_json_schema(v); + } + } + + // Normalize/ensure type + let mut ty = map + .get("type") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()); + + // If type is an array (union), pick first supported; else leave to inference + if ty.is_none() { + if let Some(JsonValue::Array(types)) = map.get("type") { + for t in types { + if let Some(tt) = t.as_str() { + if matches!( + tt, + "object" | "array" | "string" | "number" | "integer" | "boolean" + ) { + ty = Some(tt.to_string()); + break; + } + } + } + } + } + + // Infer type if still missing + if ty.is_none() { + if map.contains_key("properties") + || map.contains_key("required") + || map.contains_key("additionalProperties") + { + ty = Some("object".to_string()); + } else if map.contains_key("items") || map.contains_key("prefixItems") { + ty = Some("array".to_string()); + } else if map.contains_key("enum") + || map.contains_key("const") + || map.contains_key("format") + { + ty = Some("string".to_string()); + } else if map.contains_key("minimum") + || map.contains_key("maximum") + || map.contains_key("exclusiveMinimum") + || map.contains_key("exclusiveMaximum") + || map.contains_key("multipleOf") + { + ty = Some("number".to_string()); + } + } + // If we still couldn't infer, default to string + let ty = ty.unwrap_or_else(|| "string".to_string()); + map.insert("type".to_string(), JsonValue::String(ty.to_string())); + + // Ensure object schemas have properties map + if ty == "object" { + if !map.contains_key("properties") { + map.insert( + "properties".to_string(), + JsonValue::Object(serde_json::Map::new()), + ); + } + // If additionalProperties is an object schema, sanitize it too. + // Leave booleans as-is, since JSON Schema allows boolean here. + if let Some(ap) = map.get_mut("additionalProperties") { + let is_bool = matches!(ap, JsonValue::Bool(_)); + if !is_bool { + sanitize_json_schema(ap); + } + } + } + + // Ensure array schemas have items + if ty == "array" && !map.contains_key("items") { + map.insert("items".to_string(), json!({ "type": "string" })); + } + } + _ => {} + } +} + /// Returns a list of OpenAiTools based on the provided config and MCP tools. /// Note that the keys of mcp_tools should be fully qualified names. See /// [`McpConnectionManager`] for more details. @@ -351,6 +474,7 @@ pub(crate) fn get_openai_tools( mod tests { use crate::model_family::find_family_for_model; use mcp_types::ToolInputSchema; + use pretty_assertions::assert_eq; use super::*; @@ -497,4 +621,212 @@ mod tests { }) ); } + + #[test] + fn test_mcp_tool_property_missing_type_defaults_to_string() { + let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); + let config = ToolsConfig::new( + &model_family, + AskForApproval::Never, + SandboxPolicy::ReadOnly, + false, + ); + + let tools = get_openai_tools( + &config, + Some(HashMap::from([( + "dash/search".to_string(), + mcp_types::Tool { + name: "search".to_string(), + input_schema: ToolInputSchema { + properties: Some(serde_json::json!({ + "query": { + "description": "search query" + } + })), + required: None, + r#type: "object".to_string(), + }, + output_schema: None, + title: None, + annotations: None, + description: Some("Search docs".to_string()), + }, + )])), + ); + + assert_eq_tool_names(&tools, &["shell", "dash/search"]); + + assert_eq!( + tools[1], + OpenAiTool::Function(ResponsesApiTool { + name: "dash/search".to_string(), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "query".to_string(), + JsonSchema::String { + description: Some("search query".to_string()) + } + )]), + required: None, + additional_properties: None, + }, + description: "Search docs".to_string(), + strict: false, + }) + ); + } + + #[test] + fn test_mcp_tool_integer_normalized_to_number() { + let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); + let config = ToolsConfig::new( + &model_family, + AskForApproval::Never, + SandboxPolicy::ReadOnly, + false, + ); + + let tools = get_openai_tools( + &config, + Some(HashMap::from([( + "dash/paginate".to_string(), + mcp_types::Tool { + name: "paginate".to_string(), + input_schema: ToolInputSchema { + properties: Some(serde_json::json!({ + "page": { "type": "integer" } + })), + required: None, + r#type: "object".to_string(), + }, + output_schema: None, + title: None, + annotations: None, + description: Some("Pagination".to_string()), + }, + )])), + ); + + assert_eq_tool_names(&tools, &["shell", "dash/paginate"]); + assert_eq!( + tools[1], + OpenAiTool::Function(ResponsesApiTool { + name: "dash/paginate".to_string(), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "page".to_string(), + JsonSchema::Number { description: None } + )]), + required: None, + additional_properties: None, + }, + description: "Pagination".to_string(), + strict: false, + }) + ); + } + + #[test] + fn test_mcp_tool_array_without_items_gets_default_string_items() { + let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); + let config = ToolsConfig::new( + &model_family, + AskForApproval::Never, + SandboxPolicy::ReadOnly, + false, + ); + + let tools = get_openai_tools( + &config, + Some(HashMap::from([( + "dash/tags".to_string(), + mcp_types::Tool { + name: "tags".to_string(), + input_schema: ToolInputSchema { + properties: Some(serde_json::json!({ + "tags": { "type": "array" } + })), + required: None, + r#type: "object".to_string(), + }, + output_schema: None, + title: None, + annotations: None, + description: Some("Tags".to_string()), + }, + )])), + ); + + assert_eq_tool_names(&tools, &["shell", "dash/tags"]); + assert_eq!( + tools[1], + OpenAiTool::Function(ResponsesApiTool { + name: "dash/tags".to_string(), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "tags".to_string(), + JsonSchema::Array { + items: Box::new(JsonSchema::String { description: None }), + description: None + } + )]), + required: None, + additional_properties: None, + }, + description: "Tags".to_string(), + strict: false, + }) + ); + } + + #[test] + fn test_mcp_tool_anyof_defaults_to_string() { + let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); + let config = ToolsConfig::new( + &model_family, + AskForApproval::Never, + SandboxPolicy::ReadOnly, + false, + ); + + let tools = get_openai_tools( + &config, + Some(HashMap::from([( + "dash/value".to_string(), + mcp_types::Tool { + name: "value".to_string(), + input_schema: ToolInputSchema { + properties: Some(serde_json::json!({ + "value": { "anyOf": [ { "type": "string" }, { "type": "number" } ] } + })), + required: None, + r#type: "object".to_string(), + }, + output_schema: None, + title: None, + annotations: None, + description: Some("AnyOf Value".to_string()), + }, + )])), + ); + + assert_eq_tool_names(&tools, &["shell", "dash/value"]); + assert_eq!( + tools[1], + OpenAiTool::Function(ResponsesApiTool { + name: "dash/value".to_string(), + parameters: JsonSchema::Object { + properties: BTreeMap::from([( + "value".to_string(), + JsonSchema::String { description: None } + )]), + required: None, + additional_properties: None, + }, + description: "AnyOf Value".to_string(), + strict: false, + }) + ); + } } From 9d8d7d8704083fea6cefe6e7a6863f9775807385 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Sun, 10 Aug 2025 21:32:56 -0700 Subject: [PATCH 03/16] Middle-truncate tool output and show more lines (#2096) Command output can contain important bits of information at the beginning or end. This shows a bit more output and truncates in the middle. This will work better paired with https://github.com/openai/codex/pull/2095 which will omit output for simple successful reads/searches/etc. CleanShot 2025-08-09 at 13 01 05 ------ https://chatgpt.com/codex/tasks/task_i_68978cd19f9c832cac4975e44dcd99a0 --- codex-rs/tui/src/history_cell.rs | 78 ++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 3658dc7ed7..43144a648a 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -133,7 +133,7 @@ pub(crate) enum HistoryCell { PatchApplyResult { view: TextBlock }, } -const TOOL_CALL_MAX_LINES: usize = 3; +const TOOL_CALL_MAX_LINES: usize = 5; fn title_case(s: &str) -> String { if s.is_empty() { @@ -194,6 +194,48 @@ impl HistoryCell { .unwrap_or(0) } + fn output_lines(src: &str) -> Vec> { + let lines: Vec<&str> = src.lines().collect(); + let total = lines.len(); + let limit = TOOL_CALL_MAX_LINES; + + let mut out = Vec::new(); + + let head_end = total.min(limit); + for (i, raw) in lines[..head_end].iter().enumerate() { + let mut line = ansi_escape_line(raw); + let prefix = if i == 0 { " ⎿ " } else { " " }; + line.spans.insert(0, prefix.into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + // If we will ellipsize less than the limit, just show it. + let show_ellipsis = total > 2 * limit; + if show_ellipsis { + let omitted = total - 2 * limit; + out.push(Line::from(format!("… +{omitted} lines"))); + } + + let tail_start = if show_ellipsis { + total - limit + } else { + head_end + }; + for raw in lines[tail_start..].iter() { + let mut line = ansi_escape_line(raw); + line.spans.insert(0, " ".into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + out + } + pub(crate) fn new_session_info( config: &Config, event: SessionConfiguredEvent, @@ -308,27 +350,7 @@ impl HistoryCell { } let src = if exit_code == 0 { stdout } else { stderr }; - - let mut lines_iter = src.lines(); - for (idx, raw) in lines_iter.by_ref().take(TOOL_CALL_MAX_LINES).enumerate() { - let mut line = ansi_escape_line(raw); - let prefix = if idx == 0 { " ⎿ " } else { " " }; - line.spans.insert(0, prefix.into()); - line.spans.iter_mut().for_each(|span| { - span.style = span.style.add_modifier(Modifier::DIM); - }); - lines.push(line); - } - let remaining = lines_iter.count(); - if remaining > 0 { - let mut more = Line::from(format!("... +{remaining} lines")); - // Continuation/ellipsis is treated as a subsequent line for prefixing - more.spans.insert(0, " ".into()); - more.spans.iter_mut().for_each(|span| { - span.style = span.style.add_modifier(Modifier::DIM); - }); - lines.push(more); - } + lines.extend(Self::output_lines(&src)); lines.push(Line::from("")); HistoryCell::CompletedExecCommand { @@ -813,17 +835,7 @@ impl HistoryCell { lines.push(Line::from("✘ Failed to apply patch".magenta().bold())); if !stderr.trim().is_empty() { - let mut iter = stderr.lines(); - for (i, raw) in iter.by_ref().take(TOOL_CALL_MAX_LINES).enumerate() { - let prefix = if i == 0 { " ⎿ " } else { " " }; - let s = format!("{prefix}{raw}"); - lines.push(ansi_escape_line(&s).dim()); - } - let remaining = iter.count(); - if remaining > 0 { - lines.push(Line::from("")); - lines.push(Line::from(format!("... +{remaining} lines")).dim()); - } + lines.extend(Self::output_lines(&stderr)); } lines.push(Line::from("")); From a191945ed696c2e0a61d33995a474e5fd9446b24 Mon Sep 17 00:00:00 2001 From: ae Date: Mon, 11 Aug 2025 07:19:15 -0700 Subject: [PATCH 04/16] fix: token usage display and context calculation (#2117) - I had a recent conversation where the one-liner showed using 11M tokens! But looking into it 10M were cached. So I looked into it and I think we had a regression here. -> - Use blended total tokens for chat composer usage display - Compute remaining context using tokens_in_context_window helper ------ https://chatgpt.com/codex/tasks/task_i_68981a16c0a4832cbf416017390930e5 --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 2743ada547..09ff8b7ab3 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -698,14 +698,15 @@ impl WidgetRef for &ChatComposer { let token_usage = &token_usage_info.total_token_usage; hint.push(Span::from(" ")); hint.push( - Span::from(format!("{} tokens used", token_usage.total_tokens)) + Span::from(format!("{} tokens used", token_usage.blended_total())) .style(Style::default().add_modifier(Modifier::DIM)), ); let last_token_usage = &token_usage_info.last_token_usage; if let Some(context_window) = token_usage_info.model_context_window { let percent_remaining: u8 = if context_window > 0 { let percent = 100.0 - - (last_token_usage.total_tokens as f32 / context_window as f32 + - (last_token_usage.tokens_in_context_window() as f32 + / context_window as f32 * 100.0); percent.clamp(0.0, 100.0) as u8 } else { From c61911524d839f5d56842faee0c46f6ef52d4387 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 11 Aug 2025 09:08:21 -0700 Subject: [PATCH 05/16] chore(deps): bump tokio-util from 0.7.15 to 0.7.16 in /codex-rs (#2155) Bumps [tokio-util](https://github.com/tokio-rs/tokio) from 0.7.15 to 0.7.16.
Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=tokio-util&package-manager=cargo&previous-version=0.7.15&new-version=0.7.16)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- codex-rs/Cargo.lock | 4 ++-- codex-rs/core/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 4eddf7bd7b..5dbc2421fa 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4781,9 +4781,9 @@ dependencies = [ [[package]] name = "tokio-util" -version = "0.7.15" +version = "0.7.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "66a539a9ad6d5d281510d5bd368c973d636c02dbf8a67300bfb6b950696ad7df" +checksum = "14307c986784f72ef81c89db7d9e28d6ac26d16213b109ea501696195e6e3ce5" dependencies = [ "bytes", "futures-core", diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 006a218abf..0f03c3b647 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -46,7 +46,7 @@ tokio = { version = "1", features = [ "rt-multi-thread", "signal", ] } -tokio-util = "0.7.14" +tokio-util = "0.7.16" toml = "0.9.4" toml_edit = "0.23.3" tracing = { version = "0.1.41", features = ["log"] } From 0aa7efe05b28f07f35570ce2fa0bad406782c444 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 11 Aug 2025 10:35:03 -0700 Subject: [PATCH 06/16] Trace RAW sse events (#2056) For easier parsing. --- codex-rs/core/src/client.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 0caf1170a6..ad08782b6d 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -403,6 +403,8 @@ async fn process_sse( } }; + trace!("SSE event: {}", sse.data); + let event: SseEvent = match serde_json::from_str(&sse.data) { Ok(event) => event, Err(e) => { @@ -411,7 +413,6 @@ async fn process_sse( } }; - trace!(?event, "SSE event"); match event.kind.as_str() { // Individual output item finalised. Forward immediately so the // rest of the agent can stream assistant text/functions *live* From fa0a879444a13b1ed1445cc21b78a4a740ac37f0 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Mon, 11 Aug 2025 10:41:23 -0700 Subject: [PATCH 07/16] show feedback message after /Compact command (#2162) This PR updates ChatWidget to ensure that when AgentMessage, AgentReasoning, or AgentReasoningRawContent events arrive without any streamed deltas, the final text from the event is rendered before the stream is finalized. Previously, these handlers ignored the event text in such cases, relying solely on prior deltas. image --- codex-rs/tui/src/chatwidget.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 344f025842..0362a678b1 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -311,10 +311,12 @@ impl ChatWidget<'_> { self.request_redraw(); } - EventMsg::AgentMessage(AgentMessageEvent { message: _ }) => { - // Final assistant answer: commit all remaining rows and close with - // a blank line. Use the final text if provided, otherwise rely on - // streamed deltas already in the builder. + EventMsg::AgentMessage(AgentMessageEvent { message }) => { + // AgentMessage: if no deltas were streamed, render the final text. + if self.current_stream != Some(StreamKind::Answer) && !message.is_empty() { + self.begin_stream(StreamKind::Answer); + self.stream_push_and_maybe_commit(&message); + } self.finalize_stream(StreamKind::Answer); self.request_redraw(); } @@ -332,8 +334,12 @@ impl ChatWidget<'_> { self.stream_push_and_maybe_commit(&delta); self.request_redraw(); } - EventMsg::AgentReasoning(AgentReasoningEvent { text: _ }) => { - // Final reasoning: commit remaining rows and close with a blank. + EventMsg::AgentReasoning(AgentReasoningEvent { text }) => { + // Final reasoning: if no deltas were streamed, render the final text. + if self.current_stream != Some(StreamKind::Reasoning) && !text.is_empty() { + self.begin_stream(StreamKind::Reasoning); + self.stream_push_and_maybe_commit(&text); + } self.finalize_stream(StreamKind::Reasoning); self.request_redraw(); } @@ -346,8 +352,12 @@ impl ChatWidget<'_> { self.stream_push_and_maybe_commit(&delta); self.request_redraw(); } - EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text: _ }) => { - // Finalize the raw reasoning stream just like the summarized reasoning event. + EventMsg::AgentReasoningRawContent(AgentReasoningRawContentEvent { text }) => { + // Final raw reasoning content: if no deltas were streamed, render the final text. + if self.current_stream != Some(StreamKind::Reasoning) && !text.is_empty() { + self.begin_stream(StreamKind::Reasoning); + self.stream_push_and_maybe_commit(&text); + } self.finalize_stream(StreamKind::Reasoning); self.request_redraw(); } From 7f6408720b66204a4ac8b2432515de7447acaa21 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 11 Aug 2025 11:26:15 -0700 Subject: [PATCH 08/16] [1/3] Parse exec commands and format them more nicely in the UI (#2095) # Note for reviewers The bulk of this PR is in in the new file, `parse_command.rs`. This file is designed to be written TDD and implemented with Codex. Do not worry about reviewing the code, just review the unit tests (if you want). If any cases are missing, we'll add more tests and have Codex fix them. I think the best approach will be to land and iterate. I have some follow-ups I want to do after this lands. The next PR after this will let us merge (and dedupe) multiple sequential cells of the same such as multiple read commands. The deduping will also be important because the model often reads the same file multiple times in a row in chunks === This PR formats common commands like reading, formatting, testing, etc more nicely: It tries to extract things like file names, tests and falls back to the cmd if it doesn't. It also only shows stdout/err if the command failed. CleanShot 2025-08-09 at 16 05 15 CleanShot 2025-08-09 at 16 05 32 CleanShot 2025-08-09 at 16 05 56 2 Part 2: https://github.com/openai/codex/pull/2097 Part 3: https://github.com/openai/codex/pull/2110 --- AGENTS.md | 2 + codex-rs/core/src/codex.rs | 2 + codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/parse_command.rs | 2045 +++++++++++++++++ codex-rs/core/src/protocol.rs | 2 + .../src/event_processor_with_human_output.rs | 1 + codex-rs/mcp-server/src/mcp_protocol.rs | 4 +- codex-rs/tui/src/chatwidget.rs | 16 +- codex-rs/tui/src/history_cell.rs | 231 +- codex-rs/tui/src/user_approval_widget.rs | 4 +- 10 files changed, 2225 insertions(+), 83 deletions(-) create mode 100644 codex-rs/core/src/parse_command.rs diff --git a/AGENTS.md b/AGENTS.md index 5c3f659c35..af25482795 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,6 +2,8 @@ In the codex-rs folder where the rust code lives: +- Crate names are prefixed with `codex-`. For examole, the `core` folder's crate is named `codex-core` +- When using format! and you can inline variables into {}, always do that. - Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. - You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. - Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate. diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3bf6288fdf..3fcac51bdc 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -65,6 +65,7 @@ use crate::models::ResponseItem; use crate::models::ShellToolCallParams; use crate::openai_tools::ToolsConfig; use crate::openai_tools::get_openai_tools; +use crate::parse_command::parse_command; use crate::plan_tool::handle_update_plan; use crate::project_doc::get_user_instructions; use crate::protocol::AgentMessageDeltaEvent; @@ -402,6 +403,7 @@ impl Session { call_id, command: command_for_display.clone(), cwd, + parsed_cmd: parse_command(&command_for_display), }), }; let event = Event { diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index c728bd3125..b36689f057 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -28,6 +28,7 @@ mod mcp_connection_manager; mod mcp_tool_call; mod message_history; mod model_provider_info; +pub mod parse_command; pub use model_provider_info::BUILT_IN_OSS_MODEL_PROVIDER_ID; pub use model_provider_info::ModelProviderInfo; pub use model_provider_info::WireApi; diff --git a/codex-rs/core/src/parse_command.rs b/codex-rs/core/src/parse_command.rs new file mode 100644 index 0000000000..01dc6e3227 --- /dev/null +++ b/codex-rs/core/src/parse_command.rs @@ -0,0 +1,2045 @@ +use crate::bash::try_parse_bash; +use crate::bash::try_parse_word_only_commands_sequence; +use serde::Deserialize; +use serde::Serialize; +use shlex::split as shlex_split; + +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub enum ParsedCommand { + Read { + cmd: Vec, + name: String, + }, + ListFiles { + cmd: Vec, + path: Option, + }, + Search { + cmd: Vec, + query: Option, + path: Option, + }, + Format { + cmd: Vec, + tool: Option, + targets: Option>, + }, + Test { + cmd: Vec, + }, + Lint { + cmd: Vec, + tool: Option, + targets: Option>, + }, + Unknown { + cmd: Vec, + }, +} + +/// DO NOT REVIEW THIS CODE BY HAND +/// This parsing code is quite complex and not easy to hand-modify. +/// The easiest way to iterate is to add unit tests and have Codex fix the implementation. +/// To encourage this, the tests have been put directly below this function rather than at the bottom of the +/// +/// Parses metadata out of an arbitrary command. +/// These commands are model driven and could include just about anything. +/// The parsing is slightly lossy due to the ~infinite expressiveness of an arbitrary command. +/// The goal of the parsed metadata is to be able to provide the user with a human readable gis +/// of what it is doing. +pub fn parse_command(command: &[String]) -> Vec { + // Parse and then collapse consecutive duplicate commands to avoid redundant summaries. + let parsed = parse_command_impl(command); + let mut deduped: Vec = Vec::with_capacity(parsed.len()); + for cmd in parsed.into_iter() { + if deduped.last().is_some_and(|prev| prev == &cmd) { + continue; + } + deduped.push(cmd); + } + deduped +} + +#[cfg(test)] +#[allow(clippy::items_after_test_module)] +/// Tests are at the top to encourage using TDD + Codex to fix the implementation. +mod tests { + use super::*; + + fn shlex_split_safe(s: &str) -> Vec { + shlex_split(s).unwrap_or_else(|| s.split_whitespace().map(|s| s.to_string()).collect()) + } + + fn vec_str(args: &[&str]) -> Vec { + args.iter().map(|s| s.to_string()).collect() + } + + fn assert_parsed(args: &[String], expected: Vec) { + let out = parse_command(args); + assert_eq!(out, expected); + } + + #[test] + fn git_status_is_unknown() { + assert_parsed( + &vec_str(&["git", "status"]), + vec![ParsedCommand::Unknown { + cmd: vec_str(&["git", "status"]), + }], + ); + } + + #[test] + fn handles_complex_bash_command_head() { + let inner = + "rg --version && node -v && pnpm -v && rg --files | wc -l && rg --files | head -n 40"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ + // Expect commands in left-to-right execution order + ParsedCommand::Search { + cmd: vec_str(&["rg", "--version"]), + query: None, + path: None, + }, + ParsedCommand::Unknown { + cmd: vec_str(&["node", "-v"]), + }, + ParsedCommand::Unknown { + cmd: vec_str(&["pnpm", "-v"]), + }, + ParsedCommand::Search { + cmd: vec_str(&["rg", "--files"]), + query: None, + path: None, + }, + ParsedCommand::Unknown { + cmd: vec_str(&["head", "-n", "40"]), + }, + ], + ); + } + + #[test] + fn supports_searching_for_navigate_to_route() -> anyhow::Result<()> { + let inner = "rg -n \"navigate-to-route\" -S"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Search { + cmd: shlex_split_safe(inner), + query: Some("navigate-to-route".to_string()), + path: None, + }], + ); + Ok(()) + } + + #[test] + fn handles_complex_bash_command() { + let inner = "rg -n \"BUG|FIXME|TODO|XXX|HACK\" -S | head -n 200"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ + ParsedCommand::Search { + cmd: vec_str(&["rg", "-n", "BUG|FIXME|TODO|XXX|HACK", "-S"]), + query: Some("BUG|FIXME|TODO|XXX|HACK".to_string()), + path: None, + }, + ParsedCommand::Unknown { + cmd: vec_str(&["head", "-n", "200"]), + }, + ], + ); + } + + #[test] + fn supports_rg_files_with_path_and_pipe() { + let inner = "rg --files webview/src | sed -n"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Search { + cmd: vec_str(&["rg", "--files", "webview/src"]), + query: None, + path: Some("webview".to_string()), + }], + ); + } + + #[test] + fn supports_rg_files_then_head() { + let inner = "rg --files | head -n 50"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ + ParsedCommand::Search { + cmd: vec_str(&["rg", "--files"]), + query: None, + path: None, + }, + ParsedCommand::Unknown { + cmd: vec_str(&["head", "-n", "50"]), + }, + ], + ); + } + + #[test] + fn supports_cat() { + let inner = "cat webview/README.md"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "README.md".to_string(), + }], + ); + } + + #[test] + fn supports_ls_with_pipe() { + let inner = "ls -la | sed -n '1,120p'"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::ListFiles { + cmd: vec_str(&["ls", "-la"]), + path: None, + }], + ); + } + + #[test] + fn supports_head_n() { + let inner = "head -n 50 Cargo.toml"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "Cargo.toml".to_string(), + }], + ); + } + + #[test] + fn supports_cat_sed_n() { + let inner = "cat tui/Cargo.toml | sed -n '1,200p'"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "Cargo.toml".to_string(), + }], + ); + } + + #[test] + fn supports_tail_n_plus() { + let inner = "tail -n +522 README.md"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "README.md".to_string(), + }], + ); + } + + #[test] + fn supports_tail_n_last_lines() { + let inner = "tail -n 30 README.md"; + let out = parse_command(&vec_str(&["bash", "-lc", inner])); + assert_eq!( + out, + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "README.md".to_string(), + }] + ); + } + + #[test] + fn supports_npm_run_build_is_unknown() { + assert_parsed( + &vec_str(&["npm", "run", "build"]), + vec![ParsedCommand::Unknown { + cmd: vec_str(&["npm", "run", "build"]), + }], + ); + } + + #[test] + fn supports_npm_run_with_forwarded_args() { + assert_parsed( + &vec_str(&[ + "npm", + "run", + "lint", + "--", + "--max-warnings", + "0", + "--format", + "json", + ]), + vec![ParsedCommand::Lint { + cmd: vec_str(&[ + "npm", + "run", + "lint", + "--", + "--max-warnings", + "0", + "--format", + "json", + ]), + tool: Some("npm-script:lint".to_string()), + targets: None, + }], + ); + } + + #[test] + fn supports_grep_recursive_current_dir() { + assert_parsed( + &vec_str(&["grep", "-R", "CODEX_SANDBOX_ENV_VAR", "-n", "."]), + vec![ParsedCommand::Search { + cmd: vec_str(&["grep", "-R", "CODEX_SANDBOX_ENV_VAR", "-n", "."]), + query: Some("CODEX_SANDBOX_ENV_VAR".to_string()), + path: Some(".".to_string()), + }], + ); + } + + #[test] + fn supports_grep_recursive_specific_file() { + assert_parsed( + &vec_str(&[ + "grep", + "-R", + "CODEX_SANDBOX_ENV_VAR", + "-n", + "core/src/spawn.rs", + ]), + vec![ParsedCommand::Search { + cmd: vec_str(&[ + "grep", + "-R", + "CODEX_SANDBOX_ENV_VAR", + "-n", + "core/src/spawn.rs", + ]), + query: Some("CODEX_SANDBOX_ENV_VAR".to_string()), + path: Some("spawn.rs".to_string()), + }], + ); + } + + #[test] + fn supports_grep_query_with_slashes_not_shortened() { + // Query strings may contain slashes and should not be shortened to the basename. + // Previously, grep queries were passed through short_display_path, which is incorrect. + assert_parsed( + &shlex_split_safe("grep -R src/main.rs -n ."), + vec![ParsedCommand::Search { + cmd: vec_str(&["grep", "-R", "src/main.rs", "-n", "."]), + query: Some("src/main.rs".to_string()), + path: Some(".".to_string()), + }], + ); + } + + #[test] + fn supports_grep_weird_backtick_in_query() { + assert_parsed( + &shlex_split_safe("grep -R COD`EX_SANDBOX -n"), + vec![ParsedCommand::Search { + cmd: vec_str(&["grep", "-R", "COD`EX_SANDBOX", "-n"]), + query: Some("COD`EX_SANDBOX".to_string()), + path: None, + }], + ); + } + + #[test] + fn supports_cd_and_rg_files() { + assert_parsed( + &shlex_split_safe("cd codex-rs && rg --files"), + vec![ + ParsedCommand::Unknown { + cmd: vec_str(&["cd", "codex-rs"]), + }, + ParsedCommand::Search { + cmd: vec_str(&["rg", "--files"]), + query: None, + path: None, + }, + ], + ); + } + + #[test] + fn echo_then_cargo_test_sequence() { + assert_parsed( + &shlex_split_safe("echo Running tests... && cargo test --all-features --quiet"), + vec![ParsedCommand::Test { + cmd: vec_str(&["cargo", "test", "--all-features", "--quiet"]), + }], + ); + } + + #[test] + fn supports_cargo_fmt_and_test_with_config() { + assert_parsed( + &shlex_split_safe( + "cargo fmt -- --config imports_granularity=Item && cargo test -p core --all-features", + ), + vec![ + ParsedCommand::Format { + cmd: shlex_split_safe("cargo fmt -- --config imports_granularity=Item"), + tool: Some("cargo fmt".to_string()), + targets: None, + }, + ParsedCommand::Test { + cmd: vec_str(&["cargo", "test", "-p", "core", "--all-features"]), + }, + ], + ); + } + + #[test] + fn recognizes_rustfmt_and_clippy() { + assert_parsed( + &shlex_split_safe("rustfmt src/main.rs"), + vec![ParsedCommand::Format { + cmd: vec_str(&["rustfmt", "src/main.rs"]), + tool: Some("rustfmt".to_string()), + targets: Some(vec!["src/main.rs".to_string()]), + }], + ); + + assert_parsed( + &shlex_split_safe("cargo clippy -p core --all-features -- -D warnings"), + vec![ParsedCommand::Lint { + cmd: vec_str(&[ + "cargo", + "clippy", + "-p", + "core", + "--all-features", + "--", + "-D", + "warnings", + ]), + tool: Some("cargo clippy".to_string()), + targets: None, + }], + ); + } + + #[test] + fn recognizes_pytest_go_and_tools() { + assert_parsed( + &shlex_split_safe( + "pytest -k 'Login and not slow' tests/test_login.py::TestLogin::test_ok", + ), + vec![ParsedCommand::Test { + cmd: vec_str(&[ + "pytest", + "-k", + "Login and not slow", + "tests/test_login.py::TestLogin::test_ok", + ]), + }], + ); + + assert_parsed( + &shlex_split_safe("go fmt ./..."), + vec![ParsedCommand::Format { + cmd: vec_str(&["go", "fmt", "./..."]), + tool: Some("go fmt".to_string()), + targets: Some(vec!["./...".to_string()]), + }], + ); + + assert_parsed( + &shlex_split_safe("go test ./pkg -run TestThing"), + vec![ParsedCommand::Test { + cmd: vec_str(&["go", "test", "./pkg", "-run", "TestThing"]), + }], + ); + + assert_parsed( + &shlex_split_safe("eslint . --max-warnings 0"), + vec![ParsedCommand::Lint { + cmd: vec_str(&["eslint", ".", "--max-warnings", "0"]), + tool: Some("eslint".to_string()), + targets: Some(vec![".".to_string()]), + }], + ); + + assert_parsed( + &shlex_split_safe("prettier -w ."), + vec![ParsedCommand::Format { + cmd: vec_str(&["prettier", "-w", "."]), + tool: Some("prettier".to_string()), + targets: Some(vec![".".to_string()]), + }], + ); + } + + #[test] + fn recognizes_jest_and_vitest_filters() { + assert_parsed( + &shlex_split_safe("jest -t 'should work' src/foo.test.ts"), + vec![ParsedCommand::Test { + cmd: vec_str(&["jest", "-t", "should work", "src/foo.test.ts"]), + }], + ); + + assert_parsed( + &shlex_split_safe("vitest -t 'runs' src/foo.test.tsx"), + vec![ParsedCommand::Test { + cmd: vec_str(&["vitest", "-t", "runs", "src/foo.test.tsx"]), + }], + ); + } + + #[test] + fn recognizes_npx_and_scripts() { + assert_parsed( + &shlex_split_safe("npx eslint src"), + vec![ParsedCommand::Lint { + cmd: vec_str(&["npx", "eslint", "src"]), + tool: Some("eslint".to_string()), + targets: Some(vec!["src".to_string()]), + }], + ); + + assert_parsed( + &shlex_split_safe("npx prettier -c ."), + vec![ParsedCommand::Format { + cmd: vec_str(&["npx", "prettier", "-c", "."]), + tool: Some("prettier".to_string()), + targets: Some(vec![".".to_string()]), + }], + ); + + assert_parsed( + &shlex_split_safe("pnpm run lint -- --max-warnings 0"), + vec![ParsedCommand::Lint { + cmd: vec_str(&["pnpm", "run", "lint", "--", "--max-warnings", "0"]), + tool: Some("pnpm-script:lint".to_string()), + targets: None, + }], + ); + + assert_parsed( + &shlex_split_safe("npm test"), + vec![ParsedCommand::Test { + cmd: vec_str(&["npm", "test"]), + }], + ); + + assert_parsed( + &shlex_split_safe("yarn test"), + vec![ParsedCommand::Test { + cmd: vec_str(&["yarn", "test"]), + }], + ); + } + + // ---- is_small_formatting_command unit tests ---- + #[test] + fn small_formatting_always_true_commands() { + for cmd in [ + "wc", "tr", "cut", "sort", "uniq", "xargs", "tee", "column", "awk", + ] { + assert!(is_small_formatting_command(&shlex_split_safe(cmd))); + assert!(is_small_formatting_command(&shlex_split_safe(&format!( + "{cmd} -x" + )))); + } + } + + #[test] + fn head_behavior() { + // No args -> small formatting + assert!(is_small_formatting_command(&vec_str(&["head"]))); + // Numeric count only -> not considered small formatting by implementation + assert!(!is_small_formatting_command(&shlex_split_safe( + "head -n 40" + ))); + // With explicit file -> not small formatting + assert!(!is_small_formatting_command(&shlex_split_safe( + "head -n 40 file.txt" + ))); + // File only (no count) -> treated as small formatting by implementation + assert!(is_small_formatting_command(&vec_str(&["head", "file.txt"]))); + } + + #[test] + fn tail_behavior() { + // No args -> small formatting + assert!(is_small_formatting_command(&vec_str(&["tail"]))); + // Numeric with plus offset -> not small formatting + assert!(!is_small_formatting_command(&shlex_split_safe( + "tail -n +10" + ))); + assert!(!is_small_formatting_command(&shlex_split_safe( + "tail -n +10 file.txt" + ))); + // Numeric count + assert!(!is_small_formatting_command(&shlex_split_safe( + "tail -n 30" + ))); + assert!(!is_small_formatting_command(&shlex_split_safe( + "tail -n 30 file.txt" + ))); + // File only -> small formatting by implementation + assert!(is_small_formatting_command(&vec_str(&["tail", "file.txt"]))); + } + + #[test] + fn sed_behavior() { + // Plain sed -> small formatting + assert!(is_small_formatting_command(&vec_str(&["sed"]))); + // sed -n (no file) -> still small formatting + assert!(is_small_formatting_command(&vec_str(&["sed", "-n", "10p"]))); + // Valid range with file -> not small formatting + assert!(!is_small_formatting_command(&shlex_split_safe( + "sed -n 10p file.txt" + ))); + assert!(!is_small_formatting_command(&shlex_split_safe( + "sed -n 1,200p file.txt" + ))); + // Invalid ranges with file -> small formatting + assert!(is_small_formatting_command(&shlex_split_safe( + "sed -n p file.txt" + ))); + assert!(is_small_formatting_command(&shlex_split_safe( + "sed -n +10p file.txt" + ))); + } + + #[test] + fn empty_tokens_is_not_small() { + let empty: Vec = Vec::new(); + assert!(!is_small_formatting_command(&empty)); + } + + #[test] + fn supports_nl_then_sed_reading() { + let inner = "nl -ba core/src/parse_command.rs | sed -n '1200,1720p'"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "parse_command.rs".to_string(), + }], + ); + } + + #[test] + fn supports_sed_n() { + let inner = "sed -n '2000,2200p' tui/src/history_cell.rs"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(inner), + name: "history_cell.rs".to_string(), + }], + ); + } + + #[test] + fn filters_out_printf() { + let inner = + r#"printf "\n===== ansi-escape/Cargo.toml =====\n"; cat -- ansi-escape/Cargo.toml"#; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Read { + cmd: shlex_split_safe("cat -- ansi-escape/Cargo.toml"), + name: "Cargo.toml".to_string(), + }], + ); + } + + #[test] + fn drops_yes_in_pipelines() { + // Inside bash -lc, `yes | rg --files` should focus on the primary command. + let inner = "yes | rg --files"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Search { + cmd: vec_str(&["rg", "--files"]), + query: None, + path: None, + }], + ); + } + + #[test] + fn supports_sed_n_then_nl_as_search() { + // Ensure `sed -n '' | nl -ba` is summarized as a search for that file. + let args = shlex_split_safe( + "sed -n '260,640p' exec/src/event_processor_with_human_output.rs | nl -ba", + ); + assert_parsed( + &args, + vec![ParsedCommand::Read { + cmd: shlex_split_safe( + "sed -n '260,640p' exec/src/event_processor_with_human_output.rs", + ), + name: "event_processor_with_human_output.rs".to_string(), + }], + ); + } + + #[test] + fn preserves_rg_with_spaces() { + assert_parsed( + &shlex_split_safe("yes | rg -n 'foo bar' -S"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg -n 'foo bar' -S"), + query: Some("foo bar".to_string()), + path: None, + }], + ); + } + + #[test] + fn ls_with_glob() { + assert_parsed( + &shlex_split_safe("ls -I '*.test.js'"), + vec![ParsedCommand::ListFiles { + cmd: shlex_split_safe("ls -I '*.test.js'"), + path: None, + }], + ); + } + + #[test] + fn trim_on_semicolon() { + assert_parsed( + &shlex_split_safe("rg foo ; echo done"), + vec![ + ParsedCommand::Search { + cmd: shlex_split_safe("rg foo"), + query: Some("foo".to_string()), + path: None, + }, + ParsedCommand::Unknown { + cmd: shlex_split_safe("echo done"), + }, + ], + ); + } + + #[test] + fn split_on_or_connector() { + // Ensure we split commands on the logical OR operator as well. + assert_parsed( + &shlex_split_safe("rg foo || echo done"), + vec![ + ParsedCommand::Search { + cmd: shlex_split_safe("rg foo"), + query: Some("foo".to_string()), + path: None, + }, + ParsedCommand::Unknown { + cmd: shlex_split_safe("echo done"), + }, + ], + ); + } + + #[test] + fn strips_true_in_sequence() { + // `true` should be dropped from parsed sequences + assert_parsed( + &shlex_split_safe("true && rg --files"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }], + ); + + assert_parsed( + &shlex_split_safe("rg --files && true"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }], + ); + } + + #[test] + fn strips_true_inside_bash_lc() { + let inner = "true && rg --files"; + assert_parsed( + &vec_str(&["bash", "-lc", inner]), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }], + ); + + let inner2 = "rg --files || true"; + assert_parsed( + &vec_str(&["bash", "-lc", inner2]), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }], + ); + } + + #[test] + fn shorten_path_on_windows() { + assert_parsed( + &shlex_split_safe(r#"cat "pkg\src\main.rs""#), + vec![ParsedCommand::Read { + cmd: shlex_split_safe(r#"cat "pkg\src\main.rs""#), + name: "main.rs".to_string(), + }], + ); + } + + #[test] + fn head_with_no_space() { + assert_parsed( + &shlex_split_safe("bash -lc 'head -n50 Cargo.toml'"), + vec![ParsedCommand::Read { + cmd: shlex_split_safe("head -n50 Cargo.toml"), + name: "Cargo.toml".to_string(), + }], + ); + } + + #[test] + fn bash_dash_c_pipeline_parsing() { + // Ensure -c is handled similarly to -lc by normalization + let inner = "rg --files | head -n 1"; + assert_parsed( + &shlex_split_safe(inner), + vec![ + ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }, + ParsedCommand::Unknown { + cmd: shlex_split_safe("head -n 1"), + }, + ], + ); + } + + #[test] + fn tail_with_no_space() { + assert_parsed( + &shlex_split_safe("bash -lc 'tail -n+10 README.md'"), + vec![ParsedCommand::Read { + cmd: shlex_split_safe("tail -n+10 README.md"), + name: "README.md".to_string(), + }], + ); + } + + #[test] + fn pnpm_test_is_parsed_as_test() { + assert_parsed( + &shlex_split_safe("pnpm test"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("pnpm test"), + }], + ); + } + + #[test] + fn pnpm_exec_vitest_is_unknown() { + // From commands_combined: cd codex-cli && pnpm exec vitest run tests/... --threads=false --passWithNoTests + let inner = "cd codex-cli && pnpm exec vitest run tests/file-tag-utils.test.ts --threads=false --passWithNoTests"; + assert_parsed( + &shlex_split_safe(inner), + vec![ + ParsedCommand::Unknown { + cmd: shlex_split_safe("cd codex-cli"), + }, + ParsedCommand::Unknown { + cmd: shlex_split_safe( + "pnpm exec vitest run tests/file-tag-utils.test.ts --threads=false --passWithNoTests", + ), + }, + ], + ); + } + + #[test] + fn cargo_test_with_crate() { + assert_parsed( + &shlex_split_safe("cargo test -p codex-core parse_command::"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("cargo test -p codex-core parse_command::"), + }], + ); + } + + #[test] + fn cargo_test_with_crate_2() { + assert_parsed( + &shlex_split_safe( + "cd core && cargo test -q parse_command::tests::bash_dash_c_pipeline_parsing parse_command::tests::fd_file_finder_variants", + ), + vec![ParsedCommand::Test { + cmd: shlex_split_safe( + "cargo test -q parse_command::tests::bash_dash_c_pipeline_parsing parse_command::tests::fd_file_finder_variants", + ), + }], + ); + } + + #[test] + fn cargo_test_with_crate_3() { + assert_parsed( + &shlex_split_safe("cd core && cargo test -q parse_command::tests"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("cargo test -q parse_command::tests"), + }], + ); + } + + #[test] + fn cargo_test_with_crate_4() { + assert_parsed( + &shlex_split_safe("cd core && cargo test --all-features parse_command -- --nocapture"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("cargo test --all-features parse_command -- --nocapture"), + }], + ); + } + + // Additional coverage for other common tools/frameworks + #[test] + fn recognizes_black_and_ruff() { + // black formats Python code + assert_parsed( + &shlex_split_safe("black src"), + vec![ParsedCommand::Format { + cmd: shlex_split_safe("black src"), + tool: Some("black".to_string()), + targets: Some(vec!["src".to_string()]), + }], + ); + + // ruff check is a linter; ensure we collect targets + assert_parsed( + &shlex_split_safe("ruff check ."), + vec![ParsedCommand::Lint { + cmd: shlex_split_safe("ruff check ."), + tool: Some("ruff".to_string()), + targets: Some(vec![".".to_string()]), + }], + ); + + // ruff format is a formatter + assert_parsed( + &shlex_split_safe("ruff format pkg/"), + vec![ParsedCommand::Format { + cmd: shlex_split_safe("ruff format pkg/"), + tool: Some("ruff".to_string()), + targets: Some(vec!["pkg/".to_string()]), + }], + ); + } + + #[test] + fn recognizes_pnpm_monorepo_test_and_npm_format_script() { + // pnpm -r test in a monorepo should still parse as a test action + assert_parsed( + &shlex_split_safe("pnpm -r test"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("pnpm -r test"), + }], + ); + + // npm run format should be recognized as a format action + assert_parsed( + &shlex_split_safe("npm run format -- -w ."), + vec![ParsedCommand::Format { + cmd: shlex_split_safe("npm run format -- -w ."), + tool: Some("npm-script:format".to_string()), + targets: None, + }], + ); + } + + #[test] + fn yarn_test_is_parsed_as_test() { + assert_parsed( + &shlex_split_safe("yarn test"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("yarn test"), + }], + ); + } + + #[test] + fn pytest_file_only_and_go_run_regex() { + // pytest invoked with a file path should be captured as a filter + assert_parsed( + &shlex_split_safe("pytest tests/test_example.py"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("pytest tests/test_example.py"), + }], + ); + + // go test with -run regex should capture the filter + assert_parsed( + &shlex_split_safe("go test ./... -run '^TestFoo$'"), + vec![ParsedCommand::Test { + cmd: shlex_split_safe("go test ./... -run '^TestFoo$'"), + }], + ); + } + + #[test] + fn grep_with_query_and_path() { + assert_parsed( + &shlex_split_safe("grep -R TODO src"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("grep -R TODO src"), + query: Some("TODO".to_string()), + path: Some("src".to_string()), + }], + ); + } + + #[test] + fn rg_with_equals_style_flags() { + assert_parsed( + &shlex_split_safe("rg --colors=never -n foo src"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --colors=never -n foo src"), + query: Some("foo".to_string()), + path: Some("src".to_string()), + }], + ); + } + + #[test] + fn cat_with_double_dash_and_sed_ranges() { + // cat -- should be treated as a read of that file + assert_parsed( + &shlex_split_safe("cat -- ./-strange-file-name"), + vec![ParsedCommand::Read { + cmd: shlex_split_safe("cat -- ./-strange-file-name"), + name: "-strange-file-name".to_string(), + }], + ); + + // sed -n should be treated as a read of + assert_parsed( + &shlex_split_safe("sed -n '12,20p' Cargo.toml"), + vec![ParsedCommand::Read { + cmd: shlex_split_safe("sed -n '12,20p' Cargo.toml"), + name: "Cargo.toml".to_string(), + }], + ); + } + + #[test] + fn drop_trailing_nl_in_pipeline() { + // When an `nl` stage has only flags, it should be dropped from the summary + assert_parsed( + &shlex_split_safe("rg --files | nl -ba"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("rg --files"), + query: None, + path: None, + }], + ); + } + + #[test] + fn ls_with_time_style_and_path() { + assert_parsed( + &shlex_split_safe("ls --time-style=long-iso ./dist"), + vec![ParsedCommand::ListFiles { + cmd: shlex_split_safe("ls --time-style=long-iso ./dist"), + // short_display_path drops "dist" and shows "." as the last useful segment + path: Some(".".to_string()), + }], + ); + } + + #[test] + fn eslint_with_config_path_and_target() { + assert_parsed( + &shlex_split_safe("eslint -c .eslintrc.json src"), + vec![ParsedCommand::Lint { + cmd: shlex_split_safe("eslint -c .eslintrc.json src"), + tool: Some("eslint".to_string()), + targets: Some(vec!["src".to_string()]), + }], + ); + } + + #[test] + fn npx_eslint_with_config_path_and_target() { + assert_parsed( + &shlex_split_safe("npx eslint -c .eslintrc src"), + vec![ParsedCommand::Lint { + cmd: shlex_split_safe("npx eslint -c .eslintrc src"), + tool: Some("eslint".to_string()), + targets: Some(vec!["src".to_string()]), + }], + ); + } + + #[test] + fn fd_file_finder_variants() { + assert_parsed( + &shlex_split_safe("fd -t f src/"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("fd -t f src/"), + query: None, + path: Some("src".to_string()), + }], + ); + + // fd with query and path should capture both + assert_parsed( + &shlex_split_safe("fd main src"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("fd main src"), + query: Some("main".to_string()), + path: Some("src".to_string()), + }], + ); + } + + #[test] + fn find_basic_name_filter() { + assert_parsed( + &shlex_split_safe("find . -name '*.rs'"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("find . -name '*.rs'"), + query: Some("*.rs".to_string()), + path: Some(".".to_string()), + }], + ); + } + + #[test] + fn find_type_only_path() { + assert_parsed( + &shlex_split_safe("find src -type f"), + vec![ParsedCommand::Search { + cmd: shlex_split_safe("find src -type f"), + query: None, + path: Some("src".to_string()), + }], + ); + } +} + +pub fn parse_command_impl(command: &[String]) -> Vec { + let normalized = normalize_tokens(command); + + if let Some(commands) = parse_bash_lc_commands(command, &normalized) { + return commands; + } + + let parts = if contains_connectors(&normalized) { + split_on_connectors(&normalized) + } else { + vec![normalized.clone()] + }; + + // Preserve left-to-right execution order for all commands, including bash -c/-lc + // so summaries reflect the order they will run. + + // Map each pipeline segment to its parsed summary. + let mut parsed: Vec = parts + .iter() + .map(|tokens| summarize_main_tokens(tokens)) + .collect(); + + // If a pipeline ends with `nl` using only flags (e.g., `| nl -ba`), drop it so the + // main action (e.g., a sed range over a file) is surfaced cleanly. + if parsed.len() >= 2 { + let has_and_and = normalized.iter().any(|t| t == "&&"); + let contains_test = parsed + .iter() + .any(|pc| matches!(pc, ParsedCommand::Test { .. })); + parsed.retain(|pc| match pc { + ParsedCommand::Unknown { cmd } => { + if let Some(first) = cmd.first() { + // Drop cosmetic echo segments in chained commands + if has_and_and && first == "echo" { + return false; + } + // In non-bash chained commands, ignore directory changes like `cd foo` + // when the sequence includes a recognized test command. Preserve `cd` + // for other sequences (e.g., followed by a search command). + if has_and_and && contains_test && first == "cd" { + return false; + } + // Drop no-op commands like `true` + if cmd.len() == 1 && first == "true" { + return false; + } + if first == "nl" { + // Treat `nl` without an explicit file operand as formatting-only. + return cmd.iter().skip(1).any(|a| !a.starts_with('-')); + } + } + true + } + _ => true, + }); + } + + // Also drop standalone `true` commands when not part of a chained `&&` context above + parsed.retain(|pc| match pc { + ParsedCommand::Unknown { cmd } => { + !(cmd.len() == 1 && cmd.first().is_some_and(|s| s == "true")) + } + _ => true, + }); + + parsed +} + +/// Validates that this is a `sed -n 123,123p` command. +fn is_valid_sed_n_arg(arg: Option<&str>) -> bool { + let s = match arg { + Some(s) => s, + None => return false, + }; + let core = match s.strip_suffix('p') { + Some(rest) => rest, + None => return false, + }; + let parts: Vec<&str> = core.split(',').collect(); + match parts.as_slice() { + [num] => !num.is_empty() && num.chars().all(|c| c.is_ascii_digit()), + [a, b] => { + !a.is_empty() + && !b.is_empty() + && a.chars().all(|c| c.is_ascii_digit()) + && b.chars().all(|c| c.is_ascii_digit()) + } + _ => false, + } +} + +/// Normalize a command by: +/// - Removing `yes`/`no`/`bash -c`/`bash -lc` prefixes. +/// - Splitting on `|` and `&&`/`||`/`; +fn normalize_tokens(cmd: &[String]) -> Vec { + match cmd { + [first, pipe, rest @ ..] if (first == "yes" || first == "y") && pipe == "|" => { + // Do not re-shlex already-tokenized input; just drop the prefix. + rest.to_vec() + } + [first, pipe, rest @ ..] if (first == "no" || first == "n") && pipe == "|" => { + // Do not re-shlex already-tokenized input; just drop the prefix. + rest.to_vec() + } + [bash, flag, script] if bash == "bash" && (flag == "-c" || flag == "-lc") => { + shlex_split(script) + .unwrap_or_else(|| vec!["bash".to_string(), flag.clone(), script.clone()]) + } + _ => cmd.to_vec(), + } +} + +fn contains_connectors(tokens: &[String]) -> bool { + tokens + .iter() + .any(|t| t == "&&" || t == "||" || t == "|" || t == ";") +} + +fn split_on_connectors(tokens: &[String]) -> Vec> { + let mut out: Vec> = Vec::new(); + let mut cur: Vec = Vec::new(); + for t in tokens { + if t == "&&" || t == "||" || t == "|" || t == ";" { + if !cur.is_empty() { + out.push(std::mem::take(&mut cur)); + } + } else { + cur.push(t.clone()); + } + } + if !cur.is_empty() { + out.push(cur); + } + out +} + +fn trim_at_connector(tokens: &[String]) -> Vec { + let idx = tokens + .iter() + .position(|t| t == "|" || t == "&&" || t == "||" || t == ";") + .unwrap_or(tokens.len()); + tokens[..idx].to_vec() +} + +/// Shorten a path to the last component, excluding `build`/`dist`/`node_modules`/`src`. +/// It also pulls out a useful path from a directory such as: +/// - webview/src -> webview +/// - foo/src/ -> foo +/// - packages/app/node_modules/ -> app +fn short_display_path(path: &str) -> String { + // Normalize separators and drop any trailing slash for display. + let normalized = path.replace('\\', "/"); + let trimmed = normalized.trim_end_matches('/'); + let mut parts = trimmed.split('/').rev().filter(|p| { + !p.is_empty() && *p != "build" && *p != "dist" && *p != "node_modules" && *p != "src" + }); + parts + .next() + .map(|s| s.to_string()) + .unwrap_or_else(|| trimmed.to_string()) +} + +// Skip values consumed by specific flags and ignore --flag=value style arguments. +fn skip_flag_values<'a>(args: &'a [String], flags_with_vals: &[&str]) -> Vec<&'a String> { + let mut out: Vec<&'a String> = Vec::new(); + let mut skip_next = false; + for (i, a) in args.iter().enumerate() { + if skip_next { + skip_next = false; + continue; + } + if a == "--" { + // From here on, everything is positional operands; push the rest and break. + for rest in &args[i + 1..] { + out.push(rest); + } + break; + } + if a.starts_with("--") && a.contains('=') { + // --flag=value form: treat as a flag taking a value; skip entirely. + continue; + } + if flags_with_vals.contains(&a.as_str()) { + // This flag consumes the next argument as its value. + if i + 1 < args.len() { + skip_next = true; + } + continue; + } + out.push(a); + } + out +} + +/// Common flags for ESLint that take a following value and should not be +/// considered positional targets. +const ESLINT_FLAGS_WITH_VALUES: &[&str] = &[ + "-c", + "--config", + "--parser", + "--parser-options", + "--rulesdir", + "--plugin", + "--max-warnings", + "--format", +]; + +fn collect_non_flag_targets(args: &[String]) -> Option> { + let mut targets = Vec::new(); + let mut skip_next = false; + for (i, a) in args.iter().enumerate() { + if a == "--" { + break; + } + if skip_next { + skip_next = false; + continue; + } + if a == "-p" + || a == "--package" + || a == "--features" + || a == "-C" + || a == "--config" + || a == "--config-path" + || a == "--out-dir" + || a == "-o" + || a == "--run" + || a == "--max-warnings" + || a == "--format" + { + if i + 1 < args.len() { + skip_next = true; + } + continue; + } + if a.starts_with('-') { + continue; + } + targets.push(a.clone()); + } + if targets.is_empty() { + None + } else { + Some(targets) + } +} + +fn collect_non_flag_targets_with_flags( + args: &[String], + flags_with_vals: &[&str], +) -> Option> { + let targets: Vec = skip_flag_values(args, flags_with_vals) + .into_iter() + .filter(|a| !a.starts_with('-')) + .cloned() + .collect(); + if targets.is_empty() { + None + } else { + Some(targets) + } +} + +fn is_pathish(s: &str) -> bool { + s == "." + || s == ".." + || s.starts_with("./") + || s.starts_with("../") + || s.contains('/') + || s.contains('\\') +} + +fn parse_fd_query_and_path(tail: &[String]) -> (Option, Option) { + let args_no_connector = trim_at_connector(tail); + // fd has several flags that take values (e.g., -t/--type, -e/--extension). + // Skip those values when extracting positional operands. + let candidates = skip_flag_values( + &args_no_connector, + &[ + "-t", + "--type", + "-e", + "--extension", + "-E", + "--exclude", + "--search-path", + ], + ); + let non_flags: Vec<&String> = candidates + .into_iter() + .filter(|p| !p.starts_with('-')) + .collect(); + match non_flags.as_slice() { + [one] => { + if is_pathish(one) { + (None, Some(short_display_path(one))) + } else { + (Some((*one).clone()), None) + } + } + [q, p, ..] => (Some((*q).clone()), Some(short_display_path(p))), + _ => (None, None), + } +} + +fn parse_find_query_and_path(tail: &[String]) -> (Option, Option) { + let args_no_connector = trim_at_connector(tail); + // First positional argument (excluding common unary operators) is the root path + let mut path: Option = None; + for a in &args_no_connector { + if !a.starts_with('-') && *a != "!" && *a != "(" && *a != ")" { + path = Some(short_display_path(a)); + break; + } + } + // Extract a common name/path/regex pattern if present + let mut query: Option = None; + let mut i = 0; + while i < args_no_connector.len() { + let a = &args_no_connector[i]; + if a == "-name" || a == "-iname" || a == "-path" || a == "-regex" { + if i + 1 < args_no_connector.len() { + query = Some(args_no_connector[i + 1].clone()); + } + break; + } + i += 1; + } + (query, path) +} + +fn classify_npm_like(tool: &str, tail: &[String], full_cmd: &[String]) -> Option { + let mut r = tail; + if tool == "pnpm" && r.first().map(|s| s.as_str()) == Some("-r") { + r = &r[1..]; + } + let mut script_name: Option = None; + if r.first().map(|s| s.as_str()) == Some("run") { + script_name = r.get(1).cloned(); + } else { + let is_test_cmd = (tool == "npm" && r.first().map(|s| s.as_str()) == Some("t")) + || ((tool == "npm" || tool == "pnpm" || tool == "yarn") + && r.first().map(|s| s.as_str()) == Some("test")); + if is_test_cmd { + script_name = Some("test".to_string()); + } + } + if let Some(name) = script_name { + let lname = name.to_lowercase(); + if lname == "test" || lname == "unit" || lname == "jest" || lname == "vitest" { + return Some(ParsedCommand::Test { + cmd: full_cmd.to_vec(), + }); + } + if lname == "lint" || lname == "eslint" { + return Some(ParsedCommand::Lint { + cmd: full_cmd.to_vec(), + tool: Some(format!("{tool}-script:{name}")), + targets: None, + }); + } + if lname == "format" || lname == "fmt" || lname == "prettier" { + return Some(ParsedCommand::Format { + cmd: full_cmd.to_vec(), + tool: Some(format!("{tool}-script:{name}")), + targets: None, + }); + } + } + None +} + +fn parse_bash_lc_commands( + original: &[String], + normalized: &[String], +) -> Option> { + let [bash, flag, script] = original else { + return None; + }; + if bash != "bash" || flag != "-lc" { + return None; + } + if let Some(tree) = try_parse_bash(script) { + if let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script) { + if !all_commands.is_empty() { + let script_tokens = shlex_split(script) + .unwrap_or_else(|| vec!["bash".to_string(), flag.clone(), script.clone()]); + // Strip small formatting helpers (e.g., head/tail/awk/wc/etc) so we + // bias toward the primary command when pipelines are present. + // First, drop obvious small formatting helpers (e.g., wc/awk/etc). + let had_multiple_commands = all_commands.len() > 1; + // The bash AST walker yields commands in right-to-left order for + // connector/pipeline sequences. Reverse to reflect actual execution order. + let mut filtered_commands = drop_small_formatting_commands(all_commands); + filtered_commands.reverse(); + if filtered_commands.is_empty() { + return Some(vec![ParsedCommand::Unknown { + cmd: normalized.to_vec(), + }]); + } + let mut commands: Vec = filtered_commands + .into_iter() + .map(|tokens| summarize_main_tokens(&tokens)) + .collect(); + // Drop no-op `true` commands + commands.retain(|pc| match pc { + ParsedCommand::Unknown { cmd } => { + !(cmd.len() == 1 && cmd.first().is_some_and(|s| s == "true")) + } + _ => true, + }); + commands = maybe_collapse_cat_sed(commands, &script_tokens); + if commands.len() == 1 { + // If we reduced to a single command, attribute the full original script + // for clearer UX in file-reading and listing scenarios, or when there were + // no connectors in the original script. For search commands that came from + // a pipeline (e.g. `rg --files | sed -n`), keep only the primary command. + let had_connectors = had_multiple_commands + || script_tokens + .iter() + .any(|t| t == "|" || t == "&&" || t == "||" || t == ";"); + commands = commands + .into_iter() + .map(|pc| match pc { + ParsedCommand::Read { name, cmd } => { + if had_connectors { + let has_pipe = script_tokens.iter().any(|t| t == "|"); + let has_sed_n = script_tokens.windows(2).any(|w| { + w.first().map(|s| s.as_str()) == Some("sed") + && w.get(1).map(|s| s.as_str()) == Some("-n") + }); + if has_pipe && has_sed_n { + ParsedCommand::Read { + cmd: script_tokens.clone(), + name, + } + } else { + ParsedCommand::Read { cmd, name } + } + } else { + ParsedCommand::Read { + cmd: script_tokens.clone(), + name, + } + } + } + ParsedCommand::ListFiles { path, cmd } => { + if had_connectors { + ParsedCommand::ListFiles { cmd, path } + } else { + ParsedCommand::ListFiles { + cmd: script_tokens.clone(), + path, + } + } + } + ParsedCommand::Search { cmd, query, path } => { + if had_connectors { + ParsedCommand::Search { cmd, query, path } + } else { + ParsedCommand::Search { + cmd: script_tokens.clone(), + query, + path, + } + } + } + ParsedCommand::Format { tool, targets, .. } => ParsedCommand::Format { + cmd: script_tokens.clone(), + tool, + targets, + }, + ParsedCommand::Test { .. } => ParsedCommand::Test { + cmd: script_tokens.clone(), + }, + ParsedCommand::Lint { tool, targets, .. } => ParsedCommand::Lint { + cmd: script_tokens.clone(), + tool, + targets, + }, + ParsedCommand::Unknown { .. } => ParsedCommand::Unknown { + cmd: script_tokens.clone(), + }, + }) + .collect(); + } + return Some(commands); + } + } + } + Some(vec![ParsedCommand::Unknown { + cmd: normalized.to_vec(), + }]) +} + +/// Return true if this looks like a small formatting helper in a pipeline. +/// Examples: `head -n 40`, `tail -n +10`, `wc -l`, `awk ...`, `cut ...`, `tr ...`. +/// We try to keep variants that clearly include a file path (e.g. `tail -n 30 file`). +fn is_small_formatting_command(tokens: &[String]) -> bool { + if tokens.is_empty() { + return false; + } + let cmd = tokens[0].as_str(); + match cmd { + // Always formatting; typically used in pipes. + // `nl` is special-cased below to allow `nl ` to be treated as a read command. + "wc" | "tr" | "cut" | "sort" | "uniq" | "xargs" | "tee" | "column" | "awk" | "yes" + | "printf" => true, + "head" => { + // Treat as formatting when no explicit file operand is present. + // Common forms: `head -n 40`, `head -c 100`. + // Keep cases like `head -n 40 file`. + tokens.len() < 3 + } + "tail" => { + // Treat as formatting when no explicit file operand is present. + // Common forms: `tail -n +10`, `tail -n 30`. + // Keep cases like `tail -n 30 file`. + tokens.len() < 3 + } + "sed" => { + // Keep `sed -n file` (treated as a file read elsewhere); + // otherwise consider it a formatting helper in a pipeline. + tokens.len() < 4 + || !(tokens[1] == "-n" && is_valid_sed_n_arg(tokens.get(2).map(|s| s.as_str()))) + } + _ => false, + } +} + +fn drop_small_formatting_commands(mut commands: Vec>) -> Vec> { + commands.retain(|tokens| !is_small_formatting_command(tokens)); + commands +} + +fn maybe_collapse_cat_sed( + commands: Vec, + script_tokens: &[String], +) -> Vec { + if commands.len() < 2 { + return commands; + } + let drop_leading_sed = match (&commands[0], &commands[1]) { + (ParsedCommand::Unknown { cmd: sed_cmd }, ParsedCommand::Read { cmd: cat_cmd, .. }) => { + let is_sed_n = sed_cmd.first().map(|s| s.as_str()) == Some("sed") + && sed_cmd.get(1).map(|s| s.as_str()) == Some("-n") + && is_valid_sed_n_arg(sed_cmd.get(2).map(|s| s.as_str())) + && sed_cmd.len() == 3; + let is_cat_file = + cat_cmd.first().map(|s| s.as_str()) == Some("cat") && cat_cmd.len() == 2; + is_sed_n && is_cat_file + } + _ => false, + }; + if drop_leading_sed { + if let ParsedCommand::Read { name, .. } = &commands[1] { + return vec![ParsedCommand::Read { + cmd: script_tokens.to_vec(), + name: name.clone(), + }]; + } + } + commands +} + +fn summarize_main_tokens(main_cmd: &[String]) -> ParsedCommand { + match main_cmd.split_first() { + // (sed-specific logic handled below in dedicated arm returning Read) + Some((head, tail)) + if head == "cargo" && tail.first().map(|s| s.as_str()) == Some("fmt") => + { + ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("cargo fmt".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + Some((head, tail)) + if head == "cargo" && tail.first().map(|s| s.as_str()) == Some("clippy") => + { + ParsedCommand::Lint { + cmd: main_cmd.to_vec(), + tool: Some("cargo clippy".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + Some((head, tail)) + if head == "cargo" && tail.first().map(|s| s.as_str()) == Some("test") => + { + ParsedCommand::Test { + cmd: main_cmd.to_vec(), + } + } + Some((head, tail)) if head == "rustfmt" => ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("rustfmt".to_string()), + targets: collect_non_flag_targets(tail), + }, + Some((head, tail)) if head == "go" && tail.first().map(|s| s.as_str()) == Some("fmt") => { + ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("go fmt".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + Some((head, tail)) if head == "go" && tail.first().map(|s| s.as_str()) == Some("test") => { + ParsedCommand::Test { + cmd: main_cmd.to_vec(), + } + } + Some((head, _)) if head == "pytest" => ParsedCommand::Test { + cmd: main_cmd.to_vec(), + }, + Some((head, tail)) if head == "eslint" => { + // Treat configuration flags with values (e.g. `-c .eslintrc`) as non-targets. + let targets = collect_non_flag_targets_with_flags(tail, ESLINT_FLAGS_WITH_VALUES); + ParsedCommand::Lint { + cmd: main_cmd.to_vec(), + tool: Some("eslint".to_string()), + targets, + } + } + Some((head, tail)) if head == "prettier" => ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("prettier".to_string()), + targets: collect_non_flag_targets(tail), + }, + Some((head, tail)) if head == "black" => ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("black".to_string()), + targets: collect_non_flag_targets(tail), + }, + Some((head, tail)) + if head == "ruff" && tail.first().map(|s| s.as_str()) == Some("check") => + { + ParsedCommand::Lint { + cmd: main_cmd.to_vec(), + tool: Some("ruff".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + Some((head, tail)) + if head == "ruff" && tail.first().map(|s| s.as_str()) == Some("format") => + { + ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("ruff".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + Some((head, _)) if (head == "jest" || head == "vitest") => ParsedCommand::Test { + cmd: main_cmd.to_vec(), + }, + Some((head, tail)) + if head == "npx" && tail.first().map(|s| s.as_str()) == Some("eslint") => + { + let targets = collect_non_flag_targets_with_flags(&tail[1..], ESLINT_FLAGS_WITH_VALUES); + ParsedCommand::Lint { + cmd: main_cmd.to_vec(), + tool: Some("eslint".to_string()), + targets, + } + } + Some((head, tail)) + if head == "npx" && tail.first().map(|s| s.as_str()) == Some("prettier") => + { + ParsedCommand::Format { + cmd: main_cmd.to_vec(), + tool: Some("prettier".to_string()), + targets: collect_non_flag_targets(&tail[1..]), + } + } + // NPM-like scripts including yarn + Some((tool, tail)) if (tool == "pnpm" || tool == "npm" || tool == "yarn") => { + if let Some(cmd) = classify_npm_like(tool, tail, main_cmd) { + cmd + } else { + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + } + Some((head, tail)) if head == "ls" => { + // Avoid treating option values as paths (e.g., ls -I "*.test.js"). + let candidates = skip_flag_values( + tail, + &[ + "-I", + "-w", + "--block-size", + "--format", + "--time-style", + "--color", + "--quoting-style", + ], + ); + let path = candidates + .into_iter() + .find(|p| !p.starts_with('-')) + .map(|p| short_display_path(p)); + ParsedCommand::ListFiles { + cmd: main_cmd.to_vec(), + path, + } + } + Some((head, tail)) if head == "rg" => { + let args_no_connector = trim_at_connector(tail); + let has_files_flag = args_no_connector.iter().any(|a| a == "--files"); + let non_flags: Vec<&String> = args_no_connector + .iter() + .filter(|p| !p.starts_with('-')) + .collect(); + let (query, path) = if has_files_flag { + (None, non_flags.first().map(|s| short_display_path(s))) + } else { + ( + non_flags.first().cloned().map(|s| s.to_string()), + non_flags.get(1).map(|s| short_display_path(s)), + ) + }; + ParsedCommand::Search { + cmd: main_cmd.to_vec(), + query, + path, + } + } + Some((head, tail)) if head == "fd" => { + let (query, path) = parse_fd_query_and_path(tail); + ParsedCommand::Search { + cmd: main_cmd.to_vec(), + query, + path, + } + } + Some((head, tail)) if head == "find" => { + // Basic find support: capture path and common name filter + let (query, path) = parse_find_query_and_path(tail); + ParsedCommand::Search { + cmd: main_cmd.to_vec(), + query, + path, + } + } + Some((head, tail)) if head == "grep" => { + let args_no_connector = trim_at_connector(tail); + let non_flags: Vec<&String> = args_no_connector + .iter() + .filter(|p| !p.starts_with('-')) + .collect(); + // Do not shorten the query: grep patterns may legitimately contain slashes + // and should be preserved verbatim. Only paths should be shortened. + let query = non_flags.first().cloned().map(|s| s.to_string()); + let path = non_flags.get(1).map(|s| short_display_path(s)); + ParsedCommand::Search { + cmd: main_cmd.to_vec(), + query, + path, + } + } + Some((head, tail)) if head == "cat" => { + // Support both `cat ` and `cat -- ` forms. + let effective_tail: &[String] = if tail.first().map(|s| s.as_str()) == Some("--") { + &tail[1..] + } else { + tail + }; + if effective_tail.len() == 1 { + let name = short_display_path(&effective_tail[0]); + ParsedCommand::Read { + cmd: main_cmd.to_vec(), + name, + } + } else { + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + } + Some((head, tail)) if head == "head" => { + // Support `head -n 50 file` and `head -n50 file` forms. + let has_valid_n = match tail.split_first() { + Some((first, rest)) if first == "-n" => rest + .first() + .is_some_and(|n| n.chars().all(|c| c.is_ascii_digit())), + Some((first, _)) if first.starts_with("-n") => { + first[2..].chars().all(|c| c.is_ascii_digit()) + } + _ => false, + }; + if has_valid_n { + // Build candidates skipping the numeric value consumed by `-n` when separated. + let mut candidates: Vec<&String> = Vec::new(); + let mut i = 0; + while i < tail.len() { + if i == 0 && tail[i] == "-n" && i + 1 < tail.len() { + let n = &tail[i + 1]; + if n.chars().all(|c| c.is_ascii_digit()) { + i += 2; + continue; + } + } + candidates.push(&tail[i]); + i += 1; + } + if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) { + let name = short_display_path(p); + return ParsedCommand::Read { + cmd: main_cmd.to_vec(), + name, + }; + } + } + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + Some((head, tail)) if head == "tail" => { + // Support `tail -n +10 file` and `tail -n+10 file` forms. + let has_valid_n = match tail.split_first() { + Some((first, rest)) if first == "-n" => rest.first().is_some_and(|n| { + let s = n.strip_prefix('+').unwrap_or(n); + !s.is_empty() && s.chars().all(|c| c.is_ascii_digit()) + }), + Some((first, _)) if first.starts_with("-n") => { + let v = &first[2..]; + let s = v.strip_prefix('+').unwrap_or(v); + !s.is_empty() && s.chars().all(|c| c.is_ascii_digit()) + } + _ => false, + }; + if has_valid_n { + // Build candidates skipping the numeric value consumed by `-n` when separated. + let mut candidates: Vec<&String> = Vec::new(); + let mut i = 0; + while i < tail.len() { + if i == 0 && tail[i] == "-n" && i + 1 < tail.len() { + let n = &tail[i + 1]; + let s = n.strip_prefix('+').unwrap_or(n); + if !s.is_empty() && s.chars().all(|c| c.is_ascii_digit()) { + i += 2; + continue; + } + } + candidates.push(&tail[i]); + i += 1; + } + if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) { + let name = short_display_path(p); + return ParsedCommand::Read { + cmd: main_cmd.to_vec(), + name, + }; + } + } + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + Some((head, tail)) if head == "nl" => { + // Avoid treating option values as paths (e.g., nl -s " "). + let candidates = skip_flag_values(tail, &["-s", "-w", "-v", "-i", "-b"]); + if let Some(p) = candidates.into_iter().find(|p| !p.starts_with('-')) { + let name = short_display_path(p); + ParsedCommand::Read { + cmd: main_cmd.to_vec(), + name, + } + } else { + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + } + Some((head, tail)) + if head == "sed" + && tail.len() >= 3 + && tail[0] == "-n" + && is_valid_sed_n_arg(tail.get(1).map(|s| s.as_str())) => + { + if let Some(path) = tail.get(2) { + let name = short_display_path(path); + ParsedCommand::Read { + cmd: main_cmd.to_vec(), + name, + } + } else { + ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + } + } + } + // Other commands + _ => ParsedCommand::Unknown { + cmd: main_cmd.to_vec(), + }, + } +} diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 9008ad307d..4972f10d98 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -21,6 +21,7 @@ use crate::config_types::ReasoningEffort as ReasoningEffortConfig; use crate::config_types::ReasoningSummary as ReasoningSummaryConfig; use crate::message_history::HistoryEntry; use crate::model_provider_info::ModelProviderInfo; +use crate::parse_command::ParsedCommand; use crate::plan_tool::UpdatePlanArgs; /// Submission Queue Entry - requests from user @@ -579,6 +580,7 @@ pub struct ExecCommandBeginEvent { pub command: Vec, /// The command's working directory if not the default cwd for the agent. pub cwd: PathBuf, + pub parsed_cmd: Vec, } #[derive(Debug, Clone, Deserialize, Serialize)] diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index a2ae813183..1d35dcb73f 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -255,6 +255,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { call_id, command, cwd, + parsed_cmd: _, }) => { self.call_id_to_command.insert( call_id.clone(), diff --git a/codex-rs/mcp-server/src/mcp_protocol.rs b/codex-rs/mcp-server/src/mcp_protocol.rs index 2f8858a37b..0528e18a39 100644 --- a/codex-rs/mcp-server/src/mcp_protocol.rs +++ b/codex-rs/mcp-server/src/mcp_protocol.rs @@ -936,6 +936,7 @@ mod tests { call_id: "c1".into(), command: vec!["bash".into(), "-lc".into(), "echo hi".into()], cwd: std::path::PathBuf::from("/work"), + parsed_cmd: vec![], }), }; @@ -947,7 +948,8 @@ mod tests { "type": "exec_command_begin", "call_id": "c1", "command": ["bash", "-lc", "echo hi"], - "cwd": "/work" + "cwd": "/work", + "parsed_cmd": [] } } }); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 0362a678b1..a3a51aba44 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use codex_core::codex_wrapper::CodexConversation; use codex_core::codex_wrapper::init_codex; use codex_core::config::Config; +use codex_core::parse_command::ParsedCommand; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::AgentReasoningDeltaEvent; @@ -57,6 +58,7 @@ struct RunningCommand { command: Vec, #[allow(dead_code)] cwd: PathBuf, + parsed_cmd: Vec, } pub(crate) struct ChatWidget<'a> { @@ -452,6 +454,7 @@ impl ChatWidget<'_> { call_id, command, cwd, + parsed_cmd, }) => { self.finalize_active_stream(); // Ensure the status indicator is visible while the command runs. @@ -462,9 +465,11 @@ impl ChatWidget<'_> { RunningCommand { command: command.clone(), cwd: cwd.clone(), + parsed_cmd: parsed_cmd.clone(), }, ); - self.active_history_cell = Some(HistoryCell::new_active_exec_command(command)); + self.active_history_cell = + Some(HistoryCell::new_active_exec_command(command, parsed_cmd)); } EventMsg::ExecCommandOutputDelta(_) => { // TODO @@ -494,14 +499,19 @@ impl ChatWidget<'_> { // Compute summary before moving stdout into the history cell. let cmd = self.running_commands.remove(&call_id); self.active_history_cell = None; + let (command, parsed_cmd) = match cmd { + Some(cmd) => (cmd.command, cmd.parsed_cmd), + None => (vec![call_id], vec![]), + }; self.add_to_history(HistoryCell::new_completed_exec_command( - cmd.map(|cmd| cmd.command).unwrap_or_else(|| vec![call_id]), + command, + parsed_cmd, CommandOutput { exit_code, stdout, stderr, }, - )); + )) } EventMsg::McpToolCallBegin(McpToolCallBeginEvent { call_id: _, diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 43144a648a..3149de3551 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1,3 +1,4 @@ +use crate::colors::LIGHT_BLUE; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::slash_command::SlashCommand; @@ -8,6 +9,7 @@ use codex_ansi_escape::ansi_escape_line; use codex_common::create_config_summary_entries; use codex_common::elapsed::format_duration; use codex_core::config::Config; +use codex_core::parse_command::ParsedCommand; use codex_core::plan_tool::PlanItemArg; use codex_core::plan_tool::StepStatus; use codex_core::plan_tool::UpdatePlanArgs; @@ -160,6 +162,7 @@ impl HistoryCell { /// Return a cloned, plain representation of the cell's lines suitable for /// one‑shot insertion into the terminal scrollback. Image cells are /// represented with a simple placeholder for now. + /// These lines are also rendered directly by ratatui wrapped in a Paragraph. pub(crate) fn plain_lines(&self) -> Vec> { match self { HistoryCell::WelcomeMessage { view } @@ -194,48 +197,6 @@ impl HistoryCell { .unwrap_or(0) } - fn output_lines(src: &str) -> Vec> { - let lines: Vec<&str> = src.lines().collect(); - let total = lines.len(); - let limit = TOOL_CALL_MAX_LINES; - - let mut out = Vec::new(); - - let head_end = total.min(limit); - for (i, raw) in lines[..head_end].iter().enumerate() { - let mut line = ansi_escape_line(raw); - let prefix = if i == 0 { " ⎿ " } else { " " }; - line.spans.insert(0, prefix.into()); - line.spans.iter_mut().for_each(|span| { - span.style = span.style.add_modifier(Modifier::DIM); - }); - out.push(line); - } - - // If we will ellipsize less than the limit, just show it. - let show_ellipsis = total > 2 * limit; - if show_ellipsis { - let omitted = total - 2 * limit; - out.push(Line::from(format!("… +{omitted} lines"))); - } - - let tail_start = if show_ellipsis { - total - limit - } else { - head_end - }; - for raw in lines[tail_start..].iter() { - let mut line = ansi_escape_line(raw); - line.spans.insert(0, " ".into()); - line.spans.iter_mut().for_each(|span| { - span.style = span.style.add_modifier(Modifier::DIM); - }); - out.push(line); - } - - out - } - pub(crate) fn new_session_info( config: &Config, event: SessionConfiguredEvent, @@ -303,59 +264,99 @@ impl HistoryCell { } } - pub(crate) fn new_active_exec_command(command: Vec) -> Self { - let command_escaped = strip_bash_lc_and_escape(&command); - - let mut lines: Vec> = Vec::new(); - let mut iter = command_escaped.lines(); - if let Some(first) = iter.next() { - lines.push(Line::from(vec![ - "▌ ".cyan(), - "Running command ".magenta(), - first.to_string().into(), - ])); - } else { - lines.push(Line::from(vec!["▌ ".cyan(), "Running command".magenta()])); - } - for cont in iter { - lines.push(Line::from(cont.to_string())); - } - lines.push(Line::from("")); - + pub(crate) fn new_active_exec_command( + command: Vec, + parsed: Vec, + ) -> Self { + let lines = HistoryCell::exec_command_lines(&command, &parsed, None); HistoryCell::ActiveExecCommand { view: TextBlock::new(lines), } } - pub(crate) fn new_completed_exec_command(command: Vec, output: CommandOutput) -> Self { - let CommandOutput { - exit_code, - stdout, - stderr, - } = output; + pub(crate) fn new_completed_exec_command( + command: Vec, + parsed: Vec, + output: CommandOutput, + ) -> Self { + let lines = HistoryCell::exec_command_lines(&command, &parsed, Some(&output)); + HistoryCell::CompletedExecCommand { + view: TextBlock::new(lines), + } + } + fn exec_command_lines( + command: &[String], + parsed: &[ParsedCommand], + output: Option<&CommandOutput>, + ) -> Vec> { + if parsed.is_empty() { + HistoryCell::new_exec_command_generic(command, output) + } else { + HistoryCell::new_parsed_command(parsed, output) + } + } + + fn new_parsed_command( + parsed_commands: &[ParsedCommand], + output: Option<&CommandOutput>, + ) -> Vec> { + let mut lines: Vec = vec![Line::from("⚙︎ Working")]; + + for (i, parsed) in parsed_commands.iter().enumerate() { + let str = match parsed { + ParsedCommand::Read { name, .. } => format!("📖 {name}"), + ParsedCommand::ListFiles { cmd, path } => match path { + Some(p) => format!("📂 {p}"), + None => format!("📂 {}", shlex_join_safe(cmd)), + }, + ParsedCommand::Search { query, path, cmd } => match (query, path) { + (Some(q), Some(p)) => format!("🔎 {q} in {p}"), + (Some(q), None) => format!("🔎 {q}"), + (None, Some(p)) => format!("🔎 {p}"), + (None, None) => format!("🔎 {}", shlex_join_safe(cmd)), + }, + ParsedCommand::Format { .. } => "✨ Formatting".to_string(), + ParsedCommand::Test { cmd } => format!("🧪 {}", shlex_join_safe(cmd)), + ParsedCommand::Lint { cmd, .. } => format!("🧹 {}", shlex_join_safe(cmd)), + ParsedCommand::Unknown { cmd } => format!("⌨️ {}", shlex_join_safe(cmd)), + }; + + let prefix = if i == 0 { " L " } else { " " }; + lines.push(Line::from(vec![ + Span::styled(prefix, Style::default().add_modifier(Modifier::DIM)), + Span::styled(str, Style::default().fg(LIGHT_BLUE)), + ])); + } + + lines.extend(output_lines(output, true, false)); + lines.push(Line::from("")); + + lines + } + + fn new_exec_command_generic( + command: &[String], + output: Option<&CommandOutput>, + ) -> Vec> { let mut lines: Vec> = Vec::new(); - let command_escaped = strip_bash_lc_and_escape(&command); + let command_escaped = strip_bash_lc_and_escape(command); let mut cmd_lines = command_escaped.lines(); if let Some(first) = cmd_lines.next() { lines.push(Line::from(vec![ - "⚡ Ran command ".magenta(), + "⚡ Running ".to_string().magenta(), first.to_string().into(), ])); } else { - lines.push(Line::from("⚡ Ran command".magenta())); + lines.push(Line::from("⚡ Running".to_string().magenta())); } for cont in cmd_lines { lines.push(Line::from(cont.to_string())); } - let src = if exit_code == 0 { stdout } else { stderr }; - lines.extend(Self::output_lines(&src)); - lines.push(Line::from("")); + lines.extend(output_lines(output, false, true)); - HistoryCell::CompletedExecCommand { - view: TextBlock::new(lines), - } + lines } pub(crate) fn new_active_mcp_tool_call(invocation: McpInvocation) -> Self { @@ -835,7 +836,15 @@ impl HistoryCell { lines.push(Line::from("✘ Failed to apply patch".magenta().bold())); if !stderr.trim().is_empty() { - lines.extend(Self::output_lines(&stderr)); + lines.extend(output_lines( + Some(&CommandOutput { + exit_code: 1, + stdout: String::new(), + stderr, + }), + true, + true, + )); } lines.push(Line::from("")); @@ -854,6 +863,67 @@ impl WidgetRef for &HistoryCell { } } +fn output_lines( + output: Option<&CommandOutput>, + only_err: bool, + include_angle_pipe: bool, +) -> Vec> { + let CommandOutput { + exit_code, + stdout, + stderr, + } = match output { + Some(output) if only_err && output.exit_code == 0 => return vec![], + Some(output) => output, + None => return vec![], + }; + + let src = if *exit_code == 0 { stdout } else { stderr }; + let lines: Vec<&str> = src.lines().collect(); + let total = lines.len(); + let limit = TOOL_CALL_MAX_LINES; + + let mut out = Vec::new(); + + let head_end = total.min(limit); + for (i, raw) in lines[..head_end].iter().enumerate() { + let mut line = ansi_escape_line(raw); + let prefix = if i == 0 && include_angle_pipe { + " ⎿ " + } else { + " " + }; + line.spans.insert(0, prefix.into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + // If we will ellipsize less than the limit, just show it. + let show_ellipsis = total > 2 * limit; + if show_ellipsis { + let omitted = total - 2 * limit; + out.push(Line::from(format!("… +{omitted} lines"))); + } + + let tail_start = if show_ellipsis { + total - limit + } else { + head_end + }; + for raw in lines[tail_start..].iter() { + let mut line = ansi_escape_line(raw); + line.spans.insert(0, " ".into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + out +} + fn create_diff_summary(title: &str, changes: HashMap) -> Vec> { let mut files: Vec = Vec::new(); @@ -1008,3 +1078,10 @@ fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> { ]; Line::from(invocation_spans) } + +fn shlex_join_safe(command: &[String]) -> String { + match shlex::try_join(command.iter().map(|s| s.as_str())) { + Ok(cmd) => cmd, + Err(_) => command.join(" "), + } +} diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 966b8d68f9..d9cd709b3e 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -247,7 +247,7 @@ impl UserApprovalWidget<'_> { match decision { ReviewDecision::Approved => { lines.push(Line::from(vec![ - "✓ ".fg(Color::Green), + "✔ ".fg(Color::Green), "You ".into(), "approved".bold(), " codex to run ".into(), @@ -258,7 +258,7 @@ impl UserApprovalWidget<'_> { } ReviewDecision::ApprovedForSession => { lines.push(Line::from(vec![ - "✓ ".fg(Color::Green), + "✔ ".fg(Color::Green), "You ".into(), "approved".bold(), " codex to run ".into(), From c6b46fe2202dde895e7870594242a96376faeac2 Mon Sep 17 00:00:00 2001 From: Dylan Date: Mon, 11 Aug 2025 11:38:47 -0700 Subject: [PATCH 09/16] [mcp-server] Support CodexToolCallApprovalPolicy::OnRequest (#2187) ## Summary #1865 added `AskForApproval::OnRequest`, but missed adding it to our custom struct in `mcp-server`. This adds the missing configuration ## Testing - [x] confirmed locally --- codex-rs/mcp-server/src/codex_tool_config.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index 899451a50d..4af3e29c48 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -34,7 +34,7 @@ pub struct CodexToolCallParam { pub cwd: Option, /// Approval policy for shell commands generated by the model: - /// `untrusted`, `on-failure`, `never`. + /// `untrusted`, `on-failure`, `on-request`, `never`. #[serde(default, skip_serializing_if = "Option::is_none")] pub approval_policy: Option, @@ -63,6 +63,7 @@ pub struct CodexToolCallParam { pub enum CodexToolCallApprovalPolicy { Untrusted, OnFailure, + OnRequest, Never, } @@ -71,6 +72,7 @@ impl From for AskForApproval { match value { CodexToolCallApprovalPolicy::Untrusted => AskForApproval::UnlessTrusted, CodexToolCallApprovalPolicy::OnFailure => AskForApproval::OnFailure, + CodexToolCallApprovalPolicy::OnRequest => AskForApproval::OnRequest, CodexToolCallApprovalPolicy::Never => AskForApproval::Never, } } @@ -244,10 +246,11 @@ mod tests { "type": "object", "properties": { "approval-policy": { - "description": "Approval policy for shell commands generated by the model: `untrusted`, `on-failure`, `never`.", + "description": "Approval policy for shell commands generated by the model: `untrusted`, `on-failure`, `on-request`, `never`.", "enum": [ "untrusted", "on-failure", + "on-request", "never" ], "type": "string" From b76a562c49b04e85854ec0e9681d2358a80f0527 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 11 Aug 2025 11:43:58 -0700 Subject: [PATCH 10/16] [2/3] Retain the TUI last exec history cell so that it can be updated by the next tool call (#2097) Right now, every time an exec ends, we emit it to history which makes it immutable. In order to be able to update or merge successive tool calls (which will be useful after https://github.com/openai/codex/pull/2095), we need to retain it as the active cell. This also changes the cell to contain the metadata necessary to render it so it can be updated rather than baking in the final text lines when the cell is created. Part 1: https://github.com/openai/codex/pull/2095 Part 3: https://github.com/openai/codex/pull/2110 --- codex-rs/tui/src/chatwidget.rs | 96 +++++++++++++++++++++----------- codex-rs/tui/src/history_cell.rs | 47 +++++++++------- 2 files changed, 89 insertions(+), 54 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index a3a51aba44..1c81b2cf56 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -65,7 +65,7 @@ pub(crate) struct ChatWidget<'a> { app_event_tx: AppEventSender, codex_op_tx: UnboundedSender, bottom_pane: BottomPane<'a>, - active_history_cell: Option, + active_exec_cell: Option, config: Config, initial_user_message: Option, total_token_usage: TokenUsage, @@ -114,7 +114,7 @@ fn create_initial_user_message(text: String, image_paths: Vec) -> Optio impl ChatWidget<'_> { fn interrupt_running_task(&mut self) { if self.bottom_pane.is_task_running() { - self.active_history_cell = None; + self.active_exec_cell = None; self.bottom_pane.clear_ctrl_c_quit_hint(); self.submit_op(Op::Interrupt); self.bottom_pane.set_task_running(false); @@ -131,7 +131,7 @@ impl ChatWidget<'_> { fn layout_areas(&self, area: Rect) -> [Rect; 2] { Layout::vertical([ Constraint::Max( - self.active_history_cell + self.active_exec_cell .as_ref() .map_or(0, |c| c.desired_height(area.width)), ), @@ -210,7 +210,7 @@ impl ChatWidget<'_> { has_input_focus: true, enhanced_keys_supported, }), - active_history_cell: None, + active_exec_cell: None, config, initial_user_message: create_initial_user_message( initial_prompt.unwrap_or_default(), @@ -232,7 +232,7 @@ impl ChatWidget<'_> { pub fn desired_height(&self, width: u16) -> u16 { self.bottom_pane.desired_height(width) + self - .active_history_cell + .active_exec_cell .as_ref() .map_or(0, |c| c.desired_height(width)) } @@ -255,6 +255,7 @@ impl ChatWidget<'_> { } fn add_to_history(&mut self, cell: HistoryCell) { + self.flush_active_exec_cell(); self.app_event_tx .send(AppEvent::InsertHistory(cell.plain_lines())); } @@ -298,6 +299,16 @@ impl ChatWidget<'_> { pub(crate) fn handle_codex_event(&mut self, event: Event) { let Event { id, msg } = event; + + match msg { + EventMsg::AgentMessageDelta(_) + | EventMsg::AgentReasoningDelta(_) + | EventMsg::ExecCommandOutputDelta(_) => {} + _ => { + tracing::info!("handle_codex_event: {:?}", msg); + } + } + match msg { EventMsg::SessionConfigured(event) => { self.bottom_pane @@ -456,6 +467,8 @@ impl ChatWidget<'_> { cwd, parsed_cmd, }) => { + // TODO: merge this into the active exec call. + self.flush_active_exec_cell(); self.finalize_active_stream(); // Ensure the status indicator is visible while the command runs. self.bottom_pane @@ -468,9 +481,37 @@ impl ChatWidget<'_> { parsed_cmd: parsed_cmd.clone(), }, ); - self.active_history_cell = + self.active_exec_cell = Some(HistoryCell::new_active_exec_command(command, parsed_cmd)); } + EventMsg::ExecCommandEnd(ExecCommandEndEvent { + call_id, + exit_code, + duration: _, + stdout, + stderr, + }) => { + // Compute summary before moving stdout into the history cell. + let cmd = self.running_commands.remove(&call_id); + let parsed_cmd = match &cmd { + Some(RunningCommand { parsed_cmd, .. }) => parsed_cmd.clone(), + _ => vec![], + }; + if let Some(cmd) = cmd { + // Replace the active running cell with the finalized result, + // but keep it as the active cell so it can be merged with + // subsequent commands before being committed. + self.active_exec_cell = Some(HistoryCell::new_completed_exec_command( + cmd.command, + parsed_cmd, + CommandOutput { + exit_code, + stdout, + stderr, + }, + )); + } + } EventMsg::ExecCommandOutputDelta(_) => { // TODO } @@ -489,36 +530,12 @@ impl ChatWidget<'_> { self.add_to_history(HistoryCell::new_patch_apply_failure(event.stderr)); } } - EventMsg::ExecCommandEnd(ExecCommandEndEvent { - call_id, - exit_code, - duration: _, - stdout, - stderr, - }) => { - // Compute summary before moving stdout into the history cell. - let cmd = self.running_commands.remove(&call_id); - self.active_history_cell = None; - let (command, parsed_cmd) = match cmd { - Some(cmd) => (cmd.command, cmd.parsed_cmd), - None => (vec![call_id], vec![]), - }; - self.add_to_history(HistoryCell::new_completed_exec_command( - command, - parsed_cmd, - CommandOutput { - exit_code, - stdout, - stderr, - }, - )) - } EventMsg::McpToolCallBegin(McpToolCallBeginEvent { call_id: _, invocation, }) => { self.finalize_active_stream(); - self.add_to_history(HistoryCell::new_active_mcp_tool_call(invocation)); + self.active_exec_cell = Some(HistoryCell::new_active_mcp_tool_call(invocation)); } EventMsg::McpToolCallEnd(McpToolCallEndEvent { call_id: _, @@ -526,7 +543,7 @@ impl ChatWidget<'_> { invocation, result, }) => { - self.add_to_history(HistoryCell::new_completed_mcp_tool_call( + let completed = HistoryCell::new_completed_mcp_tool_call( 80, invocation, duration, @@ -535,7 +552,8 @@ impl ChatWidget<'_> { .map(|r| r.is_error.unwrap_or(false)) .unwrap_or(false), result, - )); + ); + self.active_exec_cell = Some(completed); } EventMsg::GetHistoryEntryResponse(event) => { let codex_core::protocol::GetHistoryEntryResponseEvent { @@ -679,11 +697,21 @@ impl ChatWidget<'_> { // Ensure the waiting status is visible (composer replaced). self.bottom_pane .update_status_text("waiting for model".to_string()); + self.flush_active_exec_cell(); self.emit_stream_header(kind); } } + fn flush_active_exec_cell(&mut self) { + if let Some(active) = self.active_exec_cell.take() { + self.app_event_tx + .send(AppEvent::InsertHistory(active.plain_lines())); + } + } + fn stream_push_and_maybe_commit(&mut self, delta: &str) { + self.flush_active_exec_cell(); + self.live_builder.push_fragment(delta); // Commit overflow rows (small batches) while keeping the last N rows visible. @@ -765,7 +793,7 @@ impl WidgetRef for &ChatWidget<'_> { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let [active_cell_area, bottom_pane_area] = self.layout_areas(area); (&self.bottom_pane).render(bottom_pane_area, buf); - if let Some(cell) = &self.active_history_cell { + if let Some(cell) = &self.active_exec_cell { cell.render_ref(active_cell_area, buf); } } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 3149de3551..6b9c5df7d3 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -81,12 +81,11 @@ pub(crate) enum HistoryCell { /// Message from the user. UserPrompt { view: TextBlock }, - // AgentMessage and AgentReasoning variants were unused and have been removed. - /// An exec tool call that has not finished yet. - ActiveExecCommand { view: TextBlock }, - - /// Completed exec tool call. - CompletedExecCommand { view: TextBlock }, + Exec { + command: Vec, + parsed: Vec, + output: Option, + }, /// An MCP tool call that has not finished yet. ActiveMcpToolCall { view: TextBlock }, @@ -123,7 +122,7 @@ pub(crate) enum HistoryCell { SessionInfo { view: TextBlock }, /// A pending code patch that is awaiting user approval. Mirrors the - /// behaviour of `ActiveExecCommand` so the user sees *what* patch the + /// behaviour of `ExecCell` so the user sees *what* patch the /// model wants to apply before being prompted to approve or deny it. PendingPatch { view: TextBlock }, @@ -173,15 +172,18 @@ impl HistoryCell { | HistoryCell::PromptsOutput { view } | HistoryCell::ErrorEvent { view } | HistoryCell::SessionInfo { view } - | HistoryCell::CompletedExecCommand { view } | HistoryCell::CompletedMcpToolCall { view } | HistoryCell::PendingPatch { view } | HistoryCell::PlanUpdate { view } | HistoryCell::PatchApplyResult { view } - | HistoryCell::ActiveExecCommand { view, .. } | HistoryCell::ActiveMcpToolCall { view, .. } => { view.lines.iter().map(line_to_static).collect() } + HistoryCell::Exec { + command, + parsed, + output, + } => HistoryCell::exec_command_lines(command, parsed, output.as_ref()), HistoryCell::CompletedMcpToolCallWithImageOutput { .. } => vec![ Line::from("tool result (image output omitted)"), Line::from(""), @@ -268,10 +270,7 @@ impl HistoryCell { command: Vec, parsed: Vec, ) -> Self { - let lines = HistoryCell::exec_command_lines(&command, &parsed, None); - HistoryCell::ActiveExecCommand { - view: TextBlock::new(lines), - } + HistoryCell::new_exec_cell(command, parsed, None) } pub(crate) fn new_completed_exec_command( @@ -279,9 +278,18 @@ impl HistoryCell { parsed: Vec, output: CommandOutput, ) -> Self { - let lines = HistoryCell::exec_command_lines(&command, &parsed, Some(&output)); - HistoryCell::CompletedExecCommand { - view: TextBlock::new(lines), + HistoryCell::new_exec_cell(command, parsed, Some(output)) + } + + fn new_exec_cell( + command: Vec, + parsed: Vec, + output: Option, + ) -> Self { + HistoryCell::Exec { + command, + parsed, + output, } } @@ -290,10 +298,9 @@ impl HistoryCell { parsed: &[ParsedCommand], output: Option<&CommandOutput>, ) -> Vec> { - if parsed.is_empty() { - HistoryCell::new_exec_command_generic(command, output) - } else { - HistoryCell::new_parsed_command(parsed, output) + match parsed.is_empty() { + true => HistoryCell::new_exec_command_generic(command, output), + false => HistoryCell::new_parsed_command(parsed, output), } } From 0cf57e1f42b54fb1ed9e6af6fdfd18da106e4298 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 11 Aug 2025 11:52:05 -0700 Subject: [PATCH 11/16] Include output truncation message in tool call results (#2183) To avoid model being confused about incomplete output. --- codex-rs/core/src/codex.rs | 58 ++++++++--------- codex-rs/core/src/exec.rs | 52 +++++++++++++--- codex-rs/core/src/shell.rs | 2 +- codex-rs/core/tests/exec.rs | 76 ++++++++++++++++++++--- codex-rs/core/tests/exec_stream_events.rs | 6 +- codex-rs/linux-sandbox/tests/landlock.rs | 6 +- 6 files changed, 146 insertions(+), 54 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3fcac51bdc..2dd48bf366 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -51,6 +51,7 @@ use crate::exec::ExecParams; use crate::exec::ExecToolCallOutput; use crate::exec::SandboxType; use crate::exec::StdoutStream; +use crate::exec::StreamOutput; use crate::exec::process_exec_tool_call; use crate::exec_env::create_env; use crate::mcp_connection_manager::McpConnectionManager; @@ -431,8 +432,8 @@ impl Session { // Because stdout and stderr could each be up to 100 KiB, we send // truncated versions. const MAX_STREAM_OUTPUT: usize = 5 * 1024; // 5KiB - let stdout = stdout.chars().take(MAX_STREAM_OUTPUT).collect(); - let stderr = stderr.chars().take(MAX_STREAM_OUTPUT).collect(); + let stdout = stdout.text.chars().take(MAX_STREAM_OUTPUT).collect(); + let stderr = stderr.text.chars().take(MAX_STREAM_OUTPUT).collect(); let msg = if is_apply_patch { EventMsg::PatchApplyEnd(PatchApplyEndEvent { @@ -504,8 +505,8 @@ impl Session { Err(e) => { output_stderr = ExecToolCallOutput { exit_code: -1, - stdout: String::new(), - stderr: get_error_message_ui(e), + stdout: StreamOutput::new(String::new()), + stderr: StreamOutput::new(get_error_message_ui(e)), duration: Duration::default(), }; &output_stderr @@ -1977,19 +1978,10 @@ async fn handle_container_exec_with_params( match output_result { Ok(output) => { - let ExecToolCallOutput { - exit_code, - stdout, - stderr, - duration, - } = &output; + let ExecToolCallOutput { exit_code, .. } = &output; let is_success = *exit_code == 0; - let content = format_exec_output( - if is_success { stdout } else { stderr }, - *exit_code, - *duration, - ); + let content = format_exec_output(&output); ResponseInputItem::FunctionCallOutput { call_id: call_id.clone(), output: FunctionCallOutputPayload { @@ -2118,19 +2110,10 @@ async fn handle_sandbox_error( match retry_output_result { Ok(retry_output) => { - let ExecToolCallOutput { - exit_code, - stdout, - stderr, - duration, - } = &retry_output; + let ExecToolCallOutput { exit_code, .. } = &retry_output; let is_success = *exit_code == 0; - let content = format_exec_output( - if is_success { stdout } else { stderr }, - *exit_code, - *duration, - ); + let content = format_exec_output(&retry_output); ResponseInputItem::FunctionCallOutput { call_id: call_id.clone(), @@ -2163,7 +2146,14 @@ async fn handle_sandbox_error( } /// Exec output is a pre-serialized JSON payload -fn format_exec_output(output: &str, exit_code: i32, duration: Duration) -> String { +fn format_exec_output(exec_output: &ExecToolCallOutput) -> String { + let ExecToolCallOutput { + exit_code, + stdout, + stderr, + duration, + } = exec_output; + #[derive(Serialize)] struct ExecMetadata { exit_code: i32, @@ -2179,10 +2169,20 @@ fn format_exec_output(output: &str, exit_code: i32, duration: Duration) -> Strin // round to 1 decimal place let duration_seconds = ((duration.as_secs_f32()) * 10.0).round() / 10.0; + let is_success = *exit_code == 0; + let output = if is_success { stdout } else { stderr }; + + let mut formatted_output = output.text.clone(); + if let Some(truncated_after_lines) = output.truncated_after_lines { + formatted_output.push_str(&format!( + "\n\n[Output truncated after {truncated_after_lines} lines: too many lines or bytes.]", + )); + } + let payload = ExecOutput { - output, + output: &formatted_output, metadata: ExecMetadata { - exit_code, + exit_code: *exit_code, duration_seconds, }, }; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 10606b6821..c964466f78 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -130,8 +130,8 @@ pub async fn process_exec_tool_call( let duration = start.elapsed(); match raw_output_result { Ok(raw_output) => { - let stdout = String::from_utf8_lossy(&raw_output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&raw_output.stderr).to_string(); + let stdout = raw_output.stdout.from_utf8_lossy(); + let stderr = raw_output.stderr.from_utf8_lossy(); #[cfg(target_family = "unix")] match raw_output.exit_status.signal() { @@ -146,7 +146,9 @@ pub async fn process_exec_tool_call( if exit_code != 0 && is_likely_sandbox_denied(sandbox_type, exit_code) { return Err(CodexErr::Sandbox(SandboxErr::Denied( - exit_code, stdout, stderr, + exit_code, + stdout.text, + stderr.text, ))); } @@ -243,18 +245,41 @@ fn is_likely_sandbox_denied(sandbox_type: SandboxType, exit_code: i32) -> bool { true } +#[derive(Debug)] +pub struct StreamOutput { + pub text: T, + pub truncated_after_lines: Option, +} #[derive(Debug)] pub struct RawExecToolCallOutput { pub exit_status: ExitStatus, - pub stdout: Vec, - pub stderr: Vec, + pub stdout: StreamOutput>, + pub stderr: StreamOutput>, +} + +impl StreamOutput { + pub fn new(text: String) -> Self { + Self { + text, + truncated_after_lines: None, + } + } +} + +impl StreamOutput> { + pub fn from_utf8_lossy(&self) -> StreamOutput { + StreamOutput { + text: String::from_utf8_lossy(&self.text).to_string(), + truncated_after_lines: self.truncated_after_lines, + } + } } #[derive(Debug)] pub struct ExecToolCallOutput { pub exit_code: i32, - pub stdout: String, - pub stderr: String, + pub stdout: StreamOutput, + pub stderr: StreamOutput, pub duration: Duration, } @@ -363,7 +388,7 @@ async fn read_capped( max_lines: usize, stream: Option, is_stderr: bool, -) -> io::Result> { +) -> io::Result>> { let mut buf = Vec::with_capacity(max_output.min(8 * 1024)); let mut tmp = [0u8; 8192]; @@ -413,7 +438,16 @@ async fn read_capped( // Continue reading to EOF to avoid back-pressure, but discard once caps are hit. } - Ok(buf) + let truncated = remaining_lines == 0 || remaining_bytes == 0; + + Ok(StreamOutput { + text: buf, + truncated_after_lines: if truncated { + Some((max_lines - remaining_lines) as u32) + } else { + None + }, + }) } #[cfg(unix)] diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index de0764f75e..cc58eb7460 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -230,7 +230,7 @@ mod tests { assert_eq!(output.exit_code, 0, "input: {input:?} output: {output:?}"); if let Some(expected) = expected_output { assert_eq!( - output.stdout, expected, + output.stdout.text, expected, "input: {input:?} output: {output:?}" ); } diff --git a/codex-rs/core/tests/exec.rs b/codex-rs/core/tests/exec.rs index f1b9e78e67..9bead7ef60 100644 --- a/codex-rs/core/tests/exec.rs +++ b/codex-rs/core/tests/exec.rs @@ -1,10 +1,11 @@ #![cfg(target_os = "macos")] -#![expect(clippy::expect_used)] +#![expect(clippy::unwrap_used, clippy::expect_used)] use std::collections::HashMap; use std::sync::Arc; use codex_core::exec::ExecParams; +use codex_core::exec::ExecToolCallOutput; use codex_core::exec::SandboxType; use codex_core::exec::process_exec_tool_call; use codex_core::protocol::SandboxPolicy; @@ -12,14 +13,20 @@ use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; use tempfile::TempDir; use tokio::sync::Notify; +use codex_core::error::Result; + use codex_core::get_platform_sandbox; -async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>, should_be_ok: bool) { +fn skip_test() -> bool { if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); - return; + return true; } + false +} + +async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result { let sandbox_type = get_platform_sandbox().expect("should be able to get sandbox type"); assert_eq!(sandbox_type, SandboxType::MacosSeatbelt); @@ -35,31 +42,82 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>, should_be_ok: bool) { let ctrl_c = Arc::new(Notify::new()); let policy = SandboxPolicy::new_read_only_policy(); - let result = process_exec_tool_call(params, sandbox_type, ctrl_c, &policy, &None, None).await; - - assert!(result.is_ok() == should_be_ok); + process_exec_tool_call(params, sandbox_type, ctrl_c, &policy, &None, None).await } /// Command succeeds with exit code 0 normally #[tokio::test] async fn exit_code_0_succeeds() { + if skip_test() { + return; + } + let tmp = TempDir::new().expect("should be able to create temp dir"); let cmd = vec!["echo", "hello"]; - run_test_cmd(tmp, cmd, true).await + let output = run_test_cmd(tmp, cmd).await.unwrap(); + assert_eq!(output.stdout.text, "hello\n"); + assert_eq!(output.stderr.text, ""); + assert_eq!(output.stdout.truncated_after_lines, None); +} + +/// Command succeeds with exit code 0 normally +#[tokio::test] +async fn truncates_output_lines() { + if skip_test() { + return; + } + + let tmp = TempDir::new().expect("should be able to create temp dir"); + let cmd = vec!["seq", "300"]; + + #[expect(clippy::unwrap_used)] + let output = run_test_cmd(tmp, cmd).await.unwrap(); + + let expected_output = (1..=256) + .map(|i| format!("{i}\n")) + .collect::>() + .join(""); + assert_eq!(output.stdout.text, expected_output); + assert_eq!(output.stdout.truncated_after_lines, Some(256)); +} + +/// Command succeeds with exit code 0 normally +#[tokio::test] +async fn truncates_output_bytes() { + if skip_test() { + return; + } + + let tmp = TempDir::new().expect("should be able to create temp dir"); + // each line is 1000 bytes + let cmd = vec!["bash", "-lc", "seq 15 | awk '{printf \"%-1000s\\n\", $0}'"]; + + let output = run_test_cmd(tmp, cmd).await.unwrap(); + + assert_eq!(output.stdout.text.len(), 10240); + assert_eq!(output.stdout.truncated_after_lines, Some(10)); } /// Command not found returns exit code 127, this is not considered a sandbox error #[tokio::test] async fn exit_command_not_found_is_ok() { + if skip_test() { + return; + } + let tmp = TempDir::new().expect("should be able to create temp dir"); let cmd = vec!["/bin/bash", "-c", "nonexistent_command_12345"]; - run_test_cmd(tmp, cmd, true).await + run_test_cmd(tmp, cmd).await.unwrap(); } /// Writing a file fails and should be considered a sandbox error #[tokio::test] async fn write_file_fails_as_sandbox_error() { + if skip_test() { + return; + } + let tmp = TempDir::new().expect("should be able to create temp dir"); let path = tmp.path().join("test.txt"); let cmd = vec![ @@ -67,5 +125,5 @@ async fn write_file_fails_as_sandbox_error() { path.to_str().expect("should be able to get path"), ]; - run_test_cmd(tmp, cmd, false).await; + assert!(run_test_cmd(tmp, cmd).await.is_err()); } diff --git a/codex-rs/core/tests/exec_stream_events.rs b/codex-rs/core/tests/exec_stream_events.rs index 534b25513a..36632afd2a 100644 --- a/codex-rs/core/tests/exec_stream_events.rs +++ b/codex-rs/core/tests/exec_stream_events.rs @@ -76,7 +76,7 @@ async fn test_exec_stdout_stream_events_echo() { }; assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "hello-world\n"); + assert_eq!(result.stdout.text, "hello-world\n"); let streamed = collect_stdout_events(rx); // We should have received at least the same contents (possibly in one chunk) @@ -128,8 +128,8 @@ async fn test_exec_stderr_stream_events_echo() { }; assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, ""); - assert_eq!(result.stderr, "oops\n"); + assert_eq!(result.stdout.text, ""); + assert_eq!(result.stderr.text, "oops\n"); // Collect only stderr delta events let mut err = Vec::new(); diff --git a/codex-rs/linux-sandbox/tests/landlock.rs b/codex-rs/linux-sandbox/tests/landlock.rs index 96298c6563..c7081dbca2 100644 --- a/codex-rs/linux-sandbox/tests/landlock.rs +++ b/codex-rs/linux-sandbox/tests/landlock.rs @@ -72,8 +72,8 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { .unwrap(); if res.exit_code != 0 { - println!("stdout:\n{}", res.stdout); - println!("stderr:\n{}", res.stderr); + println!("stdout:\n{}", res.stdout.text); + println!("stderr:\n{}", res.stderr.text); panic!("exit code: {}", res.exit_code); } } @@ -164,7 +164,7 @@ async fn assert_network_blocked(cmd: &[&str]) { .await; let (exit_code, stdout, stderr) = match result { - Ok(output) => (output.exit_code, output.stdout, output.stderr), + Ok(output) => (output.exit_code, output.stdout.text, output.stderr.text), Err(CodexErr::Sandbox(SandboxErr::Denied(exit_code, stdout, stderr))) => { (exit_code, stdout, stderr) } From 85e4f564a33a3a877fc4f279bf870c7ef9ed2645 Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Mon, 11 Aug 2025 12:31:34 -0700 Subject: [PATCH 12/16] Chores: Refactor approval Patch UI. Stack: [1/2] (#2049) - Moved the logic for the apply patch in its own file Stack: #2050 -> #2049 --- codex-rs/tui/src/diff_render.rs | 152 ++++++++++++++++++++++++++++++ codex-rs/tui/src/history_cell.rs | 155 +------------------------------ codex-rs/tui/src/lib.rs | 1 + 3 files changed, 156 insertions(+), 152 deletions(-) create mode 100644 codex-rs/tui/src/diff_render.rs diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs new file mode 100644 index 0000000000..f536681732 --- /dev/null +++ b/codex-rs/tui/src/diff_render.rs @@ -0,0 +1,152 @@ +use ratatui::style::Color; +use ratatui::style::Modifier; +use ratatui::style::Style; +use ratatui::text::Line as RtLine; +use ratatui::text::Span as RtSpan; +use std::collections::HashMap; +use std::path::PathBuf; + +use codex_core::protocol::FileChange; + +struct FileSummary { + display_path: String, + added: usize, + removed: usize, +} + +pub(crate) fn create_diff_summary( + title: &str, + changes: HashMap, +) -> Vec> { + let mut files: Vec = Vec::new(); + + // Count additions/deletions from a unified diff body + let count_from_unified = |diff: &str| -> (usize, usize) { + if let Ok(patch) = diffy::Patch::from_str(diff) { + let mut adds = 0usize; + let mut dels = 0usize; + for hunk in patch.hunks() { + for line in hunk.lines() { + match line { + diffy::Line::Insert(_) => adds += 1, + diffy::Line::Delete(_) => dels += 1, + _ => {} + } + } + } + (adds, dels) + } else { + let mut adds = 0usize; + let mut dels = 0usize; + for l in diff.lines() { + if l.starts_with("+++") || l.starts_with("---") || l.starts_with("@@") { + continue; + } + match l.as_bytes().first() { + Some(b'+') => adds += 1, + Some(b'-') => dels += 1, + _ => {} + } + } + (adds, dels) + } + }; + + for (path, change) in &changes { + use codex_core::protocol::FileChange::*; + match change { + Add { content } => { + let added = content.lines().count(); + files.push(FileSummary { + display_path: path.display().to_string(), + added, + removed: 0, + }); + } + Delete => { + let removed = std::fs::read_to_string(path) + .ok() + .map(|s| s.lines().count()) + .unwrap_or(0); + files.push(FileSummary { + display_path: path.display().to_string(), + added: 0, + removed, + }); + } + Update { + unified_diff, + move_path, + } => { + let (added, removed) = count_from_unified(unified_diff); + let display_path = if let Some(new_path) = move_path { + format!("{} → {}", path.display(), new_path.display()) + } else { + path.display().to_string() + }; + files.push(FileSummary { + display_path, + added, + removed, + }); + } + } + } + + let file_count = files.len(); + let total_added: usize = files.iter().map(|f| f.added).sum(); + let total_removed: usize = files.iter().map(|f| f.removed).sum(); + let noun = if file_count == 1 { "file" } else { "files" }; + + let mut out: Vec> = Vec::new(); + + // Header + let mut header_spans: Vec> = Vec::new(); + header_spans.push(RtSpan::styled( + title.to_owned(), + Style::default() + .fg(Color::Magenta) + .add_modifier(Modifier::BOLD), + )); + header_spans.push(RtSpan::raw(" to ")); + header_spans.push(RtSpan::raw(format!("{file_count} {noun} "))); + header_spans.push(RtSpan::raw("(")); + header_spans.push(RtSpan::styled( + format!("+{total_added}"), + Style::default().fg(Color::Green), + )); + header_spans.push(RtSpan::raw(" ")); + header_spans.push(RtSpan::styled( + format!("-{total_removed}"), + Style::default().fg(Color::Red), + )); + header_spans.push(RtSpan::raw(")")); + out.push(RtLine::from(header_spans)); + + // Dimmed per-file lines with prefix + for (idx, f) in files.iter().enumerate() { + let mut spans: Vec> = Vec::new(); + spans.push(RtSpan::raw(f.display_path.clone())); + spans.push(RtSpan::raw(" (")); + spans.push(RtSpan::styled( + format!("+{}", f.added), + Style::default().fg(Color::Green), + )); + spans.push(RtSpan::raw(" ")); + spans.push(RtSpan::styled( + format!("-{}", f.removed), + Style::default().fg(Color::Red), + )); + spans.push(RtSpan::raw(")")); + + let mut line = RtLine::from(spans); + let prefix = if idx == 0 { " ⎿ " } else { " " }; + line.spans.insert(0, prefix.into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + out +} diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 6b9c5df7d3..29b62ecb3e 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1,4 +1,5 @@ use crate::colors::LIGHT_BLUE; +use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::slash_command::SlashCommand; @@ -28,8 +29,6 @@ use ratatui::prelude::*; use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::style::Style; -use ratatui::text::Line as RtLine; -use ratatui::text::Span as RtSpan; use ratatui::widgets::Paragraph; use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; @@ -45,12 +44,6 @@ pub(crate) struct CommandOutput { pub(crate) stderr: String, } -struct FileSummary { - display_path: String, - added: usize, - removed: usize, -} - pub(crate) enum PatchEventType { ApprovalRequest, ApplyBegin { auto_approved: bool }, @@ -803,7 +796,7 @@ impl HistoryCell { event_type: PatchEventType, changes: HashMap, ) -> Self { - let title = match event_type { + let title = match &event_type { PatchEventType::ApprovalRequest => "proposed patch", PatchEventType::ApplyBegin { auto_approved: true, @@ -821,15 +814,7 @@ impl HistoryCell { } }; - let summary_lines = create_diff_summary(title, changes); - - let mut lines: Vec> = Vec::new(); - - for line in summary_lines { - lines.push(line); - } - - lines.push(Line::from("")); + let lines: Vec> = create_diff_summary(title, changes); HistoryCell::PendingPatch { view: TextBlock::new(lines), @@ -931,140 +916,6 @@ fn output_lines( out } -fn create_diff_summary(title: &str, changes: HashMap) -> Vec> { - let mut files: Vec = Vec::new(); - - // Count additions/deletions from a unified diff body - let count_from_unified = |diff: &str| -> (usize, usize) { - if let Ok(patch) = diffy::Patch::from_str(diff) { - let mut adds = 0usize; - let mut dels = 0usize; - for hunk in patch.hunks() { - for line in hunk.lines() { - match line { - diffy::Line::Insert(_) => adds += 1, - diffy::Line::Delete(_) => dels += 1, - _ => {} - } - } - } - (adds, dels) - } else { - let mut adds = 0usize; - let mut dels = 0usize; - for l in diff.lines() { - if l.starts_with("+++") || l.starts_with("---") || l.starts_with("@@") { - continue; - } - match l.as_bytes().first() { - Some(b'+') => adds += 1, - Some(b'-') => dels += 1, - _ => {} - } - } - (adds, dels) - } - }; - - for (path, change) in &changes { - use codex_core::protocol::FileChange::*; - match change { - Add { content } => { - let added = content.lines().count(); - files.push(FileSummary { - display_path: path.display().to_string(), - added, - removed: 0, - }); - } - Delete => { - let removed = std::fs::read_to_string(path) - .ok() - .map(|s| s.lines().count()) - .unwrap_or(0); - files.push(FileSummary { - display_path: path.display().to_string(), - added: 0, - removed, - }); - } - Update { - unified_diff, - move_path, - } => { - let (added, removed) = count_from_unified(unified_diff); - let display_path = if let Some(new_path) = move_path { - format!("{} → {}", path.display(), new_path.display()) - } else { - path.display().to_string() - }; - files.push(FileSummary { - display_path, - added, - removed, - }); - } - } - } - - let file_count = files.len(); - let total_added: usize = files.iter().map(|f| f.added).sum(); - let total_removed: usize = files.iter().map(|f| f.removed).sum(); - let noun = if file_count == 1 { "file" } else { "files" }; - - let mut out: Vec> = Vec::new(); - - // Header - let mut header_spans: Vec> = Vec::new(); - header_spans.push(RtSpan::styled( - title.to_owned(), - Style::default() - .fg(Color::Magenta) - .add_modifier(Modifier::BOLD), - )); - header_spans.push(RtSpan::raw(" to ")); - header_spans.push(RtSpan::raw(format!("{file_count} {noun} "))); - header_spans.push(RtSpan::raw("(")); - header_spans.push(RtSpan::styled( - format!("+{total_added}"), - Style::default().fg(Color::Green), - )); - header_spans.push(RtSpan::raw(" ")); - header_spans.push(RtSpan::styled( - format!("-{total_removed}"), - Style::default().fg(Color::Red), - )); - header_spans.push(RtSpan::raw(")")); - out.push(RtLine::from(header_spans)); - - // Dimmed per-file lines with prefix - for (idx, f) in files.iter().enumerate() { - let mut spans: Vec> = Vec::new(); - spans.push(RtSpan::raw(f.display_path.clone())); - spans.push(RtSpan::raw(" (")); - spans.push(RtSpan::styled( - format!("+{}", f.added), - Style::default().fg(Color::Green), - )); - spans.push(RtSpan::raw(" ")); - spans.push(RtSpan::styled( - format!("-{}", f.removed), - Style::default().fg(Color::Red), - )); - spans.push(RtSpan::raw(")")); - - let mut line = RtLine::from(spans); - let prefix = if idx == 0 { " ⎿ " } else { " " }; - line.spans.insert(0, prefix.into()); - line.spans.iter_mut().for_each(|span| { - span.style = span.style.add_modifier(Modifier::DIM); - }); - out.push(line); - } - - out -} - fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> { let args_str = invocation .arguments diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index e15a235a71..27c850ca61 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -31,6 +31,7 @@ mod citation_regex; mod cli; mod colors; pub mod custom_terminal; +mod diff_render; mod exec_command; mod file_search; mod get_git_diff; From 4368f075d0bcd5323044dfb7b28b6e689d7832e8 Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Mon, 11 Aug 2025 12:40:12 -0700 Subject: [PATCH 13/16] [3/3] Merge sequential exec commands (#2110) This PR merges and dedupes sequential exec cells so they stack neatly on sequential lines rather than separate blocks. This is particularly useful because the model will often sed 200 lines of a file multiple times in a row and this nicely collapses them. https://github.com/user-attachments/assets/04cccda5-e2ba-4a97-a613-4547587aa15c Part 1: https://github.com/openai/codex/pull/2095 Part 2: https://github.com/openai/codex/pull/2097 --- codex-rs/tui/src/chatwidget.rs | 267 ++++++++++++++++++++++++++++++- codex-rs/tui/src/history_cell.rs | 81 +++++++--- 2 files changed, 315 insertions(+), 33 deletions(-) diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 1c81b2cf56..d03a51bd70 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -47,6 +47,7 @@ use crate::bottom_pane::BottomPaneParams; use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::InputResult; use crate::history_cell::CommandOutput; +use crate::history_cell::ExecCell; use crate::history_cell::HistoryCell; use crate::history_cell::PatchEventType; use crate::live_wrap::RowBuilder; @@ -467,8 +468,6 @@ impl ChatWidget<'_> { cwd, parsed_cmd, }) => { - // TODO: merge this into the active exec call. - self.flush_active_exec_cell(); self.finalize_active_stream(); // Ensure the status indicator is visible while the command runs. self.bottom_pane @@ -481,8 +480,19 @@ impl ChatWidget<'_> { parsed_cmd: parsed_cmd.clone(), }, ); - self.active_exec_cell = - Some(HistoryCell::new_active_exec_command(command, parsed_cmd)); + let active_exec_cell = self.active_exec_cell.take(); + let merge_result = merge_cells(&command, &parsed_cmd, &active_exec_cell); + self.active_exec_cell = match merge_result { + MergeResult::Merge(cell) => Some(cell), + MergeResult::Drop => active_exec_cell, + MergeResult::NewCell(cell) => { + if let Some(active) = active_exec_cell { + self.app_event_tx + .send(AppEvent::InsertHistory(active.plain_lines())); + } + Some(cell) + } + } } EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, @@ -493,11 +503,15 @@ impl ChatWidget<'_> { }) => { // Compute summary before moving stdout into the history cell. let cmd = self.running_commands.remove(&call_id); - let parsed_cmd = match &cmd { - Some(RunningCommand { parsed_cmd, .. }) => parsed_cmd.clone(), - _ => vec![], - }; if let Some(cmd) = cmd { + // Preserve any merged parsed commands already present on the + // active cell; otherwise, fall back to this command's parsed. + let parsed_cmd = match &self.active_exec_cell { + Some(HistoryCell::Exec(ExecCell { parsed, .. })) if !parsed.is_empty() => { + parsed.clone() + } + _ => cmd.parsed_cmd.clone(), + }; // Replace the active running cell with the finalized result, // but keep it as the active cell so it can be merged with // subsequent commands before being committed. @@ -826,3 +840,240 @@ fn add_token_usage(current_usage: &TokenUsage, new_usage: &TokenUsage) -> TokenU total_tokens: current_usage.total_tokens + new_usage.total_tokens, } } + +enum MergeResult { + Merge(HistoryCell), + Drop, + NewCell(HistoryCell), +} + +// Determine whether to and how to merge two consecutive exec cells. +fn merge_cells( + new_command: &[String], + new_parsed: &[ParsedCommand], + active_exec_cell: &Option, +) -> MergeResult { + let ExecCell { + command: _existing_command, + parsed: existing_parsed, + output: existing_output, + } = match active_exec_cell { + Some(HistoryCell::Exec(cell)) => cell, + _ => { + // There is no existing exec cell. + return MergeResult::NewCell(HistoryCell::new_active_exec_command( + new_command.to_vec(), + new_parsed.to_vec(), + )); + } + }; + let existing_last = existing_parsed.last(); + let new_last = new_parsed.last(); + + // Drop the first command if it is a read and matches the last command. + // This is a common pattern the model does and it simplifies the output to dedupe. + let drop_first = if let ( + Some(ParsedCommand::Read { + name: existing_name, + .. + }), + Some(ParsedCommand::Read { name: new_name, .. }), + ) = (existing_last, new_last) + { + existing_name == new_name + } else { + false + }; + + if drop_first && new_parsed.len() == 1 { + // There is only one command and it was deduped. + return MergeResult::Drop; + } + let existing_exit_code = existing_output.as_ref().map(|o| o.exit_code); + if let Some(code) = existing_exit_code { + if code != 0 { + // If the previous command failed, don't merge so the user can see stderr. + // Start a fresh cell for the new command instead of duplicating the old one. + return MergeResult::NewCell(HistoryCell::new_active_exec_command( + new_command.to_vec(), + new_parsed.to_vec(), + )); + } + } + + let mut merged_parsed = existing_parsed.to_vec(); + if drop_first { + merged_parsed.extend(new_parsed[1..].to_vec()); + } else { + merged_parsed.extend(new_parsed.to_vec()); + } + + MergeResult::Merge(HistoryCell::new_active_exec_command( + new_command.to_vec(), + merged_parsed, + )) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::history_cell::CommandOutput; + + fn read_cmd(name: &str) -> ParsedCommand { + ParsedCommand::Read { + cmd: vec!["cat".to_string(), name.to_string()], + name: name.to_string(), + } + } + + fn unknown_cmd(cmd: &str) -> ParsedCommand { + ParsedCommand::Unknown { + cmd: cmd.split_whitespace().map(|s| s.to_string()).collect(), + } + } + + #[test] + fn when_no_active_exec_cell_creates_new_cell() { + let new_command = vec!["echo".to_string(), "hi".to_string()]; + let new_parsed = vec![read_cmd("a")]; + + let result = merge_cells(&new_command, &new_parsed, &None); + + match result { + MergeResult::NewCell(cell) => match cell { + HistoryCell::Exec(ExecCell { + command, + parsed, + output, + }) => { + assert_eq!(command, new_command); + assert_eq!(parsed, new_parsed); + assert!(output.is_none()); + } + _ => panic!("expected Exec cell"), + }, + _ => panic!("expected NewCell"), + } + } + + #[test] + fn drops_duplicate_trailing_read_when_new_has_only_one_read() { + // existing last = Read("foo"), new last = Read("foo"), new_parsed.len() == 1 + let active = Some(HistoryCell::new_active_exec_command( + vec!["bash".into(), "-lc".into(), "cat foo".into()], + vec![read_cmd("foo")], + )); + let new_command = vec!["cat".into(), "foo".into()]; + let new_parsed = vec![read_cmd("foo")]; + + let result = merge_cells(&new_command, &new_parsed, &active); + match result { + MergeResult::Drop => {} + _ => panic!("expected Drop"), + } + } + + #[test] + fn does_not_merge_when_previous_command_failed() { + // existing exit_code != 0 forces starting a fresh cell + let active = Some(HistoryCell::new_completed_exec_command( + vec!["bash".into(), "-lc".into(), "cat bar".into()], + vec![read_cmd("bar")], + CommandOutput { + exit_code: 1, + stdout: String::new(), + stderr: "err".into(), + }, + )); + // Ensure drop_first condition is false (different name) + let new_command = vec!["cat".into(), "baz".into()]; + let new_parsed = vec![read_cmd("baz")]; + + let result = merge_cells(&new_command, &new_parsed, &active); + match result { + MergeResult::NewCell(cell) => match cell { + HistoryCell::Exec(ExecCell { + command, parsed, .. + }) => { + assert_eq!(command, new_command); + assert_eq!(parsed, new_parsed); + } + _ => panic!("expected Exec cell"), + }, + _ => panic!("expected NewCell"), + } + } + + #[test] + fn merges_with_drop_first_true_when_new_len_gt_one() { + // existing last Read("file.txt"), new starts with same Read then more + let active = Some(HistoryCell::new_active_exec_command( + vec!["cat".into(), "file.txt".into()], + vec![read_cmd("file.txt")], + )); + let new_command = vec!["bash".into(), "-lc".into(), "sed -n 1,20p file.txt".into()]; + // Place the duplicate Read as the LAST element to satisfy drop_first condition + let leading = unknown_cmd("tail -n 20"); + let new_parsed = vec![leading.clone(), read_cmd("file.txt")]; + + let result = merge_cells(&new_command, &new_parsed, &active); + match result { + MergeResult::Merge(cell) => match cell { + HistoryCell::Exec(ExecCell { + command, parsed, .. + }) => { + assert_eq!(command, new_command); + // Expect existing parsed + new_parsed[1..] + assert_eq!(parsed.len(), 2); + match (&parsed[0], &parsed[1]) { + ( + ParsedCommand::Read { name, .. }, + ParsedCommand::Read { name: n2, .. }, + ) => { + assert_eq!(name, "file.txt"); + assert_eq!(n2, "file.txt"); + } + _ => panic!("unexpected parsed commands"), + } + } + _ => panic!("expected Exec cell"), + }, + _ => panic!("expected Merge"), + } + } + + #[test] + fn merges_without_drop_first_when_last_commands_differ() { + // existing last Read("file1.txt"), new last Read("file2.txt"); should concatenate + let active = Some(HistoryCell::new_active_exec_command( + vec!["cat".into(), "file1.txt".into()], + vec![read_cmd("file1.txt")], + )); + let new_command = vec!["bash".into(), "-lc".into(), "cat file2.txt".into()]; + let t2 = read_cmd("file2.txt"); + let extra = unknown_cmd("echo done"); + let new_parsed = vec![t2.clone(), extra.clone()]; + + let result = merge_cells(&new_command, &new_parsed, &active); + match result { + MergeResult::Merge(cell) => match cell { + HistoryCell::Exec(ExecCell { + command, parsed, .. + }) => { + assert_eq!(command, new_command); + assert_eq!(parsed.len(), 3); + match (&parsed[0], &parsed[1], &parsed[2]) { + (ParsedCommand::Read { name: n1, .. }, p2, p3) => { + assert_eq!(n1, "file1.txt"); + assert_eq!(p2, &t2); + assert_eq!(p3, &extra); + } + _ => panic!("unexpected parsed commands"), + } + } + _ => panic!("expected Exec cell"), + }, + _ => panic!("expected Merge"), + } + } +} diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 29b62ecb3e..1236d4b550 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -38,6 +38,7 @@ use std::path::PathBuf; use std::time::Duration; use tracing::error; +#[derive(Clone)] pub(crate) struct CommandOutput { pub(crate) exit_code: i32, pub(crate) stdout: String, @@ -64,27 +65,37 @@ fn line_to_static(line: &Line) -> Line<'static> { } } +pub(crate) struct ExecCell { + pub(crate) command: Vec, + pub(crate) parsed: Vec, + pub(crate) output: Option, +} + /// Represents an event to display in the conversation history. Returns its /// `Vec>` representation to make it easier to display in a /// scrollable list. pub(crate) enum HistoryCell { /// Welcome message. - WelcomeMessage { view: TextBlock }, - - /// Message from the user. - UserPrompt { view: TextBlock }, - - Exec { - command: Vec, - parsed: Vec, - output: Option, + WelcomeMessage { + view: TextBlock, }, + /// Message from the user. + UserPrompt { + view: TextBlock, + }, + + Exec(ExecCell), + /// An MCP tool call that has not finished yet. - ActiveMcpToolCall { view: TextBlock }, + ActiveMcpToolCall { + view: TextBlock, + }, /// Completed MCP tool call where we show the result serialized as JSON. - CompletedMcpToolCall { view: TextBlock }, + CompletedMcpToolCall { + view: TextBlock, + }, /// Completed MCP tool call where the result is an image. /// Admittedly, [mcp_types::CallToolResult] can have multiple content types, @@ -94,37 +105,57 @@ pub(crate) enum HistoryCell { // resized version avoids doing the potentially expensive rescale twice // because the scroll-view first calls `height()` for layouting and then // `render_window()` for painting. - CompletedMcpToolCallWithImageOutput { _image: DynamicImage }, + CompletedMcpToolCallWithImageOutput { + _image: DynamicImage, + }, /// Background event. - BackgroundEvent { view: TextBlock }, + BackgroundEvent { + view: TextBlock, + }, /// Output from the `/diff` command. - GitDiffOutput { view: TextBlock }, + GitDiffOutput { + view: TextBlock, + }, /// Output from the `/status` command. - StatusOutput { view: TextBlock }, + StatusOutput { + view: TextBlock, + }, /// Output from the `/prompts` command. - PromptsOutput { view: TextBlock }, + PromptsOutput { + view: TextBlock, + }, /// Error event from the backend. - ErrorEvent { view: TextBlock }, + ErrorEvent { + view: TextBlock, + }, /// Info describing the newly-initialized session. - SessionInfo { view: TextBlock }, + SessionInfo { + view: TextBlock, + }, /// A pending code patch that is awaiting user approval. Mirrors the /// behaviour of `ExecCell` so the user sees *what* patch the /// model wants to apply before being prompted to approve or deny it. - PendingPatch { view: TextBlock }, + PendingPatch { + view: TextBlock, + }, /// A human‑friendly rendering of the model's current plan and step /// statuses provided via the `update_plan` tool. - PlanUpdate { view: TextBlock }, + PlanUpdate { + view: TextBlock, + }, /// Result of applying a patch (success or failure) with optional output. - PatchApplyResult { view: TextBlock }, + PatchApplyResult { + view: TextBlock, + }, } const TOOL_CALL_MAX_LINES: usize = 5; @@ -172,11 +203,11 @@ impl HistoryCell { | HistoryCell::ActiveMcpToolCall { view, .. } => { view.lines.iter().map(line_to_static).collect() } - HistoryCell::Exec { + HistoryCell::Exec(ExecCell { command, parsed, output, - } => HistoryCell::exec_command_lines(command, parsed, output.as_ref()), + }) => HistoryCell::exec_command_lines(command, parsed, output.as_ref()), HistoryCell::CompletedMcpToolCallWithImageOutput { .. } => vec![ Line::from("tool result (image output omitted)"), Line::from(""), @@ -279,11 +310,11 @@ impl HistoryCell { parsed: Vec, output: Option, ) -> Self { - HistoryCell::Exec { + HistoryCell::Exec(ExecCell { command, parsed, output, - } + }) } fn exec_command_lines( From 5f8984aa7d550955eb5f894d5c29adc2b9901da2 Mon Sep 17 00:00:00 2001 From: Dylan Date: Mon, 11 Aug 2025 13:11:04 -0700 Subject: [PATCH 14/16] [apply-patch] Support applypatch command string (#2186) ## Summary GPT-OSS and `gpt-5-mini` have training artifacts that cause the models to occasionally use `applypatch` instead of `apply_patch`. I think long-term we'll want to provide `apply_patch` as a first class tool, but for now let's silently handle this case to avoid hurting model performance ## Testing - [x] Added unit test --- codex-rs/apply-patch/src/lib.rs | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 61b1b68f9e..262d219d6d 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -82,8 +82,9 @@ pub struct ApplyPatchArgs { } pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { + const APPLY_PATCH_COMMANDS: [&str; 2] = ["apply_patch", "applypatch"]; match argv { - [cmd, body] if cmd == "apply_patch" => match parse_patch(body) { + [cmd, body] if APPLY_PATCH_COMMANDS.contains(&cmd.as_str()) => match parse_patch(body) { Ok(source) => MaybeApplyPatch::Body(source), Err(e) => MaybeApplyPatch::PatchParseError(e), }, @@ -722,6 +723,31 @@ mod tests { } } + #[test] + fn test_literal_applypatch() { + let args = strs_to_strings(&[ + "applypatch", + r#"*** Begin Patch +*** Add File: foo ++hi +*** End Patch +"#, + ]); + + match maybe_parse_apply_patch(&args) { + MaybeApplyPatch::Body(ApplyPatchArgs { hunks, patch: _ }) => { + assert_eq!( + hunks, + vec![Hunk::AddFile { + path: PathBuf::from("foo"), + contents: "hi\n".to_string() + }] + ); + } + result => panic!("expected MaybeApplyPatch::Body got {result:?}"), + } + } + #[test] fn test_heredoc() { let args = strs_to_strings(&[ From a48372ce5dbfdef94fdb91a173df93119a5aee90 Mon Sep 17 00:00:00 2001 From: ae Date: Mon, 11 Aug 2025 14:15:41 -0700 Subject: [PATCH 15/16] feat: add a /mention slash command (#2114) - To help people discover @mentions. - Command just places a @ in the composer. - #2115 then improves the behavior of @mentions with empty queries. --- codex-rs/tui/src/app.rs | 5 ++ codex-rs/tui/src/bottom_pane/chat_composer.rs | 46 +++++++++++++++++++ codex-rs/tui/src/bottom_pane/mod.rs | 5 ++ codex-rs/tui/src/chatwidget.rs | 4 ++ codex-rs/tui/src/slash_command.rs | 2 + 5 files changed, 62 insertions(+) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index a97948f3ea..4a0adb8de7 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -366,6 +366,11 @@ impl App<'_> { widget.add_diff_output(text); } } + SlashCommand::Mention => { + if let AppState::Chat { widget } = &mut self.app_state { + widget.insert_str("@"); + } + } SlashCommand::Status => { if let AppState::Chat { widget } = &mut self.app_state { widget.add_status_output(); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 09ff8b7ab3..78506f572c 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -198,6 +198,12 @@ impl ChatComposer { self.set_has_focus(has_focus); } + pub(crate) fn insert_str(&mut self, text: &str) { + self.textarea.insert_str(text); + self.sync_command_popup(); + self.sync_file_search_popup(); + } + /// Handle a key event coming from the main UI. pub fn handle_key_event(&mut self, key_event: KeyEvent) -> (InputResult, bool) { let result = match &mut self.active_popup { @@ -1078,6 +1084,46 @@ mod tests { } } + #[test] + fn slash_mention_dispatches_command_and_inserts_at() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + use std::sync::mpsc::TryRecvError; + + let (tx, rx) = std::sync::mpsc::channel(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new(true, sender, false); + + for ch in ['/', 'm', 'e', 'n', 't', 'i', 'o', 'n'] { + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE)); + } + + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + match result { + InputResult::None => {} + InputResult::Submitted(text) => { + panic!("expected command dispatch, but composer submitted literal text: {text}") + } + } + assert!(composer.textarea.is_empty(), "composer should be cleared"); + + match rx.try_recv() { + Ok(AppEvent::DispatchCommand(cmd)) => { + assert_eq!(cmd.command(), "mention"); + composer.insert_str("@"); + } + Ok(_other) => panic!("unexpected app event"), + Err(TryRecvError::Empty) => panic!("expected a DispatchCommand event for '/mention'"), + Err(TryRecvError::Disconnected) => { + panic!("app event channel disconnected") + } + } + assert_eq!(composer.textarea.text(), "@"); + } + #[test] fn test_multiple_pastes_submission() { use crossterm::event::KeyCode; diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 0c8610470c..4606f9b8ee 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -196,6 +196,11 @@ impl BottomPane<'_> { } } + pub(crate) fn insert_str(&mut self, text: &str) { + self.composer.insert_str(text); + self.request_redraw(); + } + /// Update the status indicator text. Prefer replacing the composer with /// the StatusIndicatorView so the input pane shows a single-line status /// like: `▌ Working waiting for model`. diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index d03a51bd70..173ab64af2 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -676,6 +676,10 @@ impl ChatWidget<'_> { self.submit_user_message(text.into()); } + pub(crate) fn insert_str(&mut self, text: &str) { + self.bottom_pane.insert_str(text); + } + pub(crate) fn token_usage(&self) -> &TokenUsage { &self.total_token_usage } diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index e58ab8521e..0de1f6fae6 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -16,6 +16,7 @@ pub enum SlashCommand { Init, Compact, Diff, + Mention, Status, Prompts, Logout, @@ -33,6 +34,7 @@ impl SlashCommand { SlashCommand::Compact => "summarize conversation to prevent hitting the context limit", SlashCommand::Quit => "exit Codex", SlashCommand::Diff => "show git diff (including untracked files)", + SlashCommand::Mention => "mention a file", SlashCommand::Status => "show current session configuration and token usage", SlashCommand::Prompts => "show example prompts", SlashCommand::Logout => "log out of Codex", From 178cf1e884a00f80e30acaca00d99130e6ef51de Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 11 Aug 2025 14:51:01 -0700 Subject: [PATCH 16/16] fix: take ExecToolCallOutput by value to avoid clone() --- codex-rs/core/src/codex.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 2dd48bf366..936cd4ef98 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1981,7 +1981,7 @@ async fn handle_container_exec_with_params( let ExecToolCallOutput { exit_code, .. } = &output; let is_success = *exit_code == 0; - let content = format_exec_output(&output); + let content = format_exec_output(output); ResponseInputItem::FunctionCallOutput { call_id: call_id.clone(), output: FunctionCallOutputPayload { @@ -2113,7 +2113,7 @@ async fn handle_sandbox_error( let ExecToolCallOutput { exit_code, .. } = &retry_output; let is_success = *exit_code == 0; - let content = format_exec_output(&retry_output); + let content = format_exec_output(retry_output); ResponseInputItem::FunctionCallOutput { call_id: call_id.clone(), @@ -2146,7 +2146,7 @@ async fn handle_sandbox_error( } /// Exec output is a pre-serialized JSON payload -fn format_exec_output(exec_output: &ExecToolCallOutput) -> String { +fn format_exec_output(exec_output: ExecToolCallOutput) -> String { let ExecToolCallOutput { exit_code, stdout, @@ -2169,10 +2169,10 @@ fn format_exec_output(exec_output: &ExecToolCallOutput) -> String { // round to 1 decimal place let duration_seconds = ((duration.as_secs_f32()) * 10.0).round() / 10.0; - let is_success = *exit_code == 0; + let is_success = exit_code == 0; let output = if is_success { stdout } else { stderr }; - let mut formatted_output = output.text.clone(); + let mut formatted_output = output.text; if let Some(truncated_after_lines) = output.truncated_after_lines { formatted_output.push_str(&format!( "\n\n[Output truncated after {truncated_after_lines} lines: too many lines or bytes.]", @@ -2182,7 +2182,7 @@ fn format_exec_output(exec_output: &ExecToolCallOutput) -> String { let payload = ExecOutput { output: &formatted_output, metadata: ExecMetadata { - exit_code: *exit_code, + exit_code, duration_seconds, }, };