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/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/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(&[ 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"] } 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* diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3bf6288fdf..936cd4ef98 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; @@ -65,6 +66,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 +404,7 @@ impl Session { call_id, command: command_for_display.clone(), cwd, + parsed_cmd: parse_command(&command_for_display), }), }; let event = Event { @@ -429,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 { @@ -502,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 @@ -1975,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 { @@ -2116,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(), @@ -2161,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, @@ -2177,8 +2169,18 @@ 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; + 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, 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/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/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, + }) + ); + } } 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/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/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/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) } 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" 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/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 2743ada547..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 { @@ -698,14 +704,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 { @@ -1077,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 344f025842..173ab64af2 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; @@ -46,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; @@ -57,13 +59,14 @@ struct RunningCommand { command: Vec, #[allow(dead_code)] cwd: PathBuf, + parsed_cmd: Vec, } 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, @@ -112,7 +115,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); @@ -129,7 +132,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)), ), @@ -208,7 +211,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(), @@ -230,7 +233,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)) } @@ -253,6 +256,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())); } @@ -296,6 +300,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 @@ -311,10 +325,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 +348,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 +366,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(); } @@ -442,6 +466,7 @@ impl ChatWidget<'_> { call_id, command, cwd, + parsed_cmd, }) => { self.finalize_active_stream(); // Ensure the status indicator is visible while the command runs. @@ -452,9 +477,54 @@ 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)); + 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, + exit_code, + duration: _, + stdout, + stderr, + }) => { + // Compute summary before moving stdout into the history cell. + let cmd = self.running_commands.remove(&call_id); + 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. + self.active_exec_cell = Some(HistoryCell::new_completed_exec_command( + cmd.command, + parsed_cmd, + CommandOutput { + exit_code, + stdout, + stderr, + }, + )); + } } EventMsg::ExecCommandOutputDelta(_) => { // TODO @@ -474,31 +544,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; - self.add_to_history(HistoryCell::new_completed_exec_command( - cmd.map(|cmd| cmd.command).unwrap_or_else(|| vec![call_id]), - 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: _, @@ -506,7 +557,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, @@ -515,7 +566,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 { @@ -624,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 } @@ -659,11 +715,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. @@ -745,7 +811,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); } } @@ -778,3 +844,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/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 3658dc7ed7..1236d4b550 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1,3 +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; @@ -8,6 +10,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; @@ -26,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; @@ -37,18 +38,13 @@ 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, pub(crate) stderr: String, } -struct FileSummary { - display_path: String, - added: usize, - removed: usize, -} - pub(crate) enum PatchEventType { ApprovalRequest, ApplyBegin { auto_approved: bool }, @@ -69,28 +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 }, + WelcomeMessage { + view: TextBlock, + }, /// Message from the user. - UserPrompt { view: TextBlock }, + 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(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, @@ -100,40 +105,60 @@ 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 `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 }, + 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 = 3; +const TOOL_CALL_MAX_LINES: usize = 5; fn title_case(s: &str) -> String { if s.is_empty() { @@ -160,6 +185,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 } @@ -170,15 +196,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(ExecCell { + command, + parsed, + output, + }) => HistoryCell::exec_command_lines(command, parsed, output.as_ref()), HistoryCell::CompletedMcpToolCallWithImageOutput { .. } => vec![ Line::from("tool result (image output omitted)"), Line::from(""), @@ -261,79 +290,104 @@ impl HistoryCell { } } - pub(crate) fn new_active_exec_command(command: Vec) -> Self { - let command_escaped = strip_bash_lc_and_escape(&command); + pub(crate) fn new_active_exec_command( + command: Vec, + parsed: Vec, + ) -> Self { + HistoryCell::new_exec_cell(command, parsed, None) + } - 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_completed_exec_command( + command: Vec, + parsed: Vec, + output: CommandOutput, + ) -> Self { + HistoryCell::new_exec_cell(command, parsed, Some(output)) + } - HistoryCell::ActiveExecCommand { - view: TextBlock::new(lines), + fn new_exec_cell( + command: Vec, + parsed: Vec, + output: Option, + ) -> Self { + HistoryCell::Exec(ExecCell { + command, + parsed, + output, + }) + } + + fn exec_command_lines( + command: &[String], + parsed: &[ParsedCommand], + output: Option<&CommandOutput>, + ) -> Vec> { + match parsed.is_empty() { + true => HistoryCell::new_exec_command_generic(command, output), + false => HistoryCell::new_parsed_command(parsed, output), } } - pub(crate) fn new_completed_exec_command(command: Vec, output: CommandOutput) -> Self { - let CommandOutput { - exit_code, - stdout, - stderr, - } = 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(output_lines(output, false, true)); - 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.push(Line::from("")); - - HistoryCell::CompletedExecCommand { - view: TextBlock::new(lines), - } + lines } pub(crate) fn new_active_mcp_tool_call(invocation: McpInvocation) -> Self { @@ -773,7 +827,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, @@ -791,15 +845,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), @@ -813,17 +859,15 @@ 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(output_lines( + Some(&CommandOutput { + exit_code: 1, + stdout: String::new(), + stderr, + }), + true, + true, + )); } lines.push(Line::from("")); @@ -842,131 +886,58 @@ impl WidgetRef for &HistoryCell { } } -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) - } +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![], }; - 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 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); } - 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" }; + // 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 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()); + 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); }); @@ -996,3 +967,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/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; 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", 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(),