From d45513ce5ad99d37638f355601f075d55860001e Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Tue, 7 Apr 2026 13:35:40 -0700 Subject: [PATCH] fix(core) revert Command line in unified exec output (#17031) ## Summary https://github.com/openai/codex/pull/13860 changed the serialized output format of Unified Exec. This PR reverts those changes and some related test changes ## Testing - [x] Update tests --------- Co-authored-by: Codex --- codex-rs/core/src/tools/context.rs | 7 -- codex-rs/core/src/tools/context_tests.rs | 9 +- codex-rs/core/tests/suite/unified_exec.rs | 118 +++++++++++----------- 3 files changed, 63 insertions(+), 71 deletions(-) diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 7c486640e..01dd0548b 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -364,13 +364,6 @@ impl ExecCommandToolOutput { fn response_text(&self) -> String { let mut sections = Vec::new(); - if let Some(command) = &self.session_command { - sections.push(format!( - "Command: {}", - codex_shell_command::parse_command::shlex_join(command) - )); - } - if !self.chunk_id.is_empty() { sections.push(format!("Chunk ID: {}", self.chunk_id)); } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index f7641a1db..07fcfda98 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -249,11 +249,7 @@ fn exec_command_tool_output_formats_truncated_response() { process_id: None, exit_code: Some(0), original_token_count: Some(10), - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "rm -rf /tmp/example.sqlite".to_string(), - ]), + session_command: None, } .to_response_item("call-42", &payload); @@ -267,8 +263,7 @@ fn exec_command_tool_output_formats_truncated_response() { .expect("exec output should serialize as text"); assert_regex_match( r#"(?sx) - ^Command:\ /bin/zsh\ -lc\ 'rm\ -rf\ /tmp/example\.sqlite' - \nChunk\ ID:\ abc123 + ^Chunk\ ID:\ abc123 \nWall\ time:\ \d+\.\d{4}\ seconds \nProcess\ exited\ with\ code\ 0 \nOriginal\ token\ count:\ 10 diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index a165db045..7470717a0 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::ffi::OsStr; use std::fs; +use std::sync::OnceLock; use anyhow::Context; use anyhow::Result; @@ -33,6 +34,7 @@ use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; use pretty_assertions::assert_eq; +use regex_lite::Regex; use serde_json::Value; use serde_json::json; use tokio::time::Duration; @@ -57,49 +59,65 @@ struct ParsedUnifiedExecOutput { #[allow(clippy::expect_used)] fn parse_unified_exec_output(raw: &str) -> Result { - let cleaned = raw.replace("\r\n", "\n"); - let (metadata, output) = cleaned - .rsplit_once("\nOutput:") + static OUTPUT_REGEX: OnceLock = OnceLock::new(); + let regex = OUTPUT_REGEX.get_or_init(|| { + Regex::new(concat!( + r#"(?s)^(?:Total output lines: \d+\n\n)?"#, + r#"(?:Chunk ID: (?P[^\n]+)\n)?"#, + r#"Wall time: (?P-?\d+(?:\.\d+)?) seconds\n"#, + r#"(?:Process exited with code (?P-?\d+)\n)?"#, + r#"(?:Process running with session ID (?P-?\d+)\n)?"#, + r#"(?:Original token count: (?P\d+)\n)?"#, + r#"Output:\n?(?P.*)$"#, + )) + .expect("valid unified exec output regex") + }); + + let cleaned = raw.trim_matches('\r'); + let captures = regex + .captures(cleaned) .ok_or_else(|| anyhow::anyhow!("missing Output section in unified exec output {raw}"))?; - let output = output.strip_prefix('\n').unwrap_or(output); - let mut chunk_id = None; - let mut wall_time_seconds = None; - let mut process_id = None; - let mut exit_code = None; - let mut original_token_count = None; + let chunk_id = captures + .name("chunk_id") + .map(|value| value.as_str().to_string()); - for line in metadata.lines() { - if let Some(value) = line.strip_prefix("Chunk ID: ") { - chunk_id = Some(value.to_string()); - } else if let Some(value) = line.strip_prefix("Wall time: ") { - let value = value.strip_suffix(" seconds").ok_or_else(|| { - anyhow::anyhow!("invalid wall time line in unified exec output: {line}") - })?; - wall_time_seconds = Some( - value - .parse::() - .context("failed to parse wall time seconds")?, - ); - } else if let Some(value) = line.strip_prefix("Process exited with code ") { - exit_code = Some( - value - .parse::() - .context("failed to parse exit code from unified exec output")?, - ); - } else if let Some(value) = line.strip_prefix("Process running with session ID ") { - process_id = Some(value.to_string()); - } else if let Some(value) = line.strip_prefix("Original token count: ") { - original_token_count = Some( - value - .parse::() - .context("failed to parse original token count from unified exec output")?, - ); - } - } + let wall_time_seconds = captures + .name("wall_time") + .expect("wall_time group present") + .as_str() + .parse::() + .context("failed to parse wall time seconds")?; - let wall_time_seconds = wall_time_seconds - .ok_or_else(|| anyhow::anyhow!("missing wall time in unified exec output {raw}"))?; + let exit_code = captures + .name("exit_code") + .map(|value| { + value + .as_str() + .parse::() + .context("failed to parse exit code from unified exec output") + }) + .transpose()?; + + let process_id = captures + .name("process_id") + .map(|value| value.as_str().to_string()); + + let original_token_count = captures + .name("original_token_count") + .map(|value| { + value + .as_str() + .parse::() + .context("failed to parse original token count from unified exec output") + }) + .transpose()?; + + let output = captures + .name("output") + .expect("output group present") + .as_str() + .to_string(); Ok(ParsedUnifiedExecOutput { chunk_id, @@ -107,7 +125,7 @@ fn parse_unified_exec_output(raw: &str) -> Result { process_id, exit_code, original_token_count, - output: output.to_string(), + output, }) } @@ -2223,22 +2241,8 @@ PY let large_output = outputs.get(call_id).expect("missing large output summary"); let output_text = large_output.output.replace("\r\n", "\n"); - assert!( - output_text.starts_with("Total output lines: "), - "expected large output summary header, got {output_text:?}" - ); - assert!( - output_text.contains("…") && output_text.contains("tokens truncated"), - "expected truncation marker in large output summary, got {output_text:?}" - ); - assert!( - output_text.contains("token token \ntoken token \ntoken token \n"), - "expected preserved output prefix in large output summary, got {output_text:?}" - ); - assert!( - output_text.ends_with("token token ") || output_text.ends_with("token token \n"), - "expected preserved output suffix in large output summary, got {output_text:?}" - ); + let truncated_pattern = r"(?s)^Total output lines: \d+\n\n(token token \n){5,}.*…\d+ tokens truncated….*(token token \n){5,}$"; + assert_regex_match(truncated_pattern, &output_text); let original_tokens = large_output .original_token_count @@ -2323,7 +2327,7 @@ async fn unified_exec_runs_under_sandbox() -> Result<()> { let outputs = collect_tool_outputs(&bodies)?; let output = outputs.get(call_id).expect("missing output"); - assert_eq!(output.output.trim_end_matches(['\r', '\n']), "hello"); + assert_regex_match("hello[\r\n]+", &output.output); Ok(()) }