diff --git a/.codex/flaky-test-triage.md b/.codex/flaky-test-triage.md index 74aa93c7f4..b9d3b6b9e0 100644 --- a/.codex/flaky-test-triage.md +++ b/.codex/flaky-test-triage.md @@ -67,6 +67,10 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron - Cap the Windows PowerShell AST parser subprocess at `5s` and kill it on timeout so a hung `pwsh` startup fails closed instead of stalling `exec_policy::tests::verify_approval_requirement_for_unsafe_powershell_command` until nextest aborts the shard. - Add a Windows-only regression test that launches a deliberately sleeping fake PowerShell executable and asserts the parser returns `Failed` within the same `5s` cap. - Merge `origin/main` into the PR branch to clear a newly dirty merge state after GitHub stopped enqueuing any `pull_request` workflows for two consecutive heads and created only the `CLA Assistant` suite. + - Run `23169801581` on head `9192637e4` narrowed the remaining failures back to two Windows-only shell-command tests after every `Lint/Build` lane and every non-Windows `Tests` lane passed: `all::suite::apply_patch_cli::apply_patch_cli_can_use_shell_command_output_as_patch_input` on Windows x64 and `tools::handlers::shell::tests::commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_command` on Windows arm64. + - Add a test-only `user_shell_override` seam through `ThreadManager`, `SessionConfiguration`, and `TestCodexBuilder` so the affected integration tests can pin `cmd.exe` on Windows without mutating live session state after startup. + - Use that override in the Windows-flaky `apply_patch_cli` and websocket shell-chain tests, while still executing the file-read command under `powershell.exe` inside `cmd.exe` where the scenario needs PowerShell output rather than PowerShell process startup. + - Remove the live PowerShell executable discovery from `core/src/tools/handlers/shell_tests.rs`; the PowerShell wrapper safety coverage already lives in `codex-shell-command`, and this core handler regression test only needs to verify that derived commands still match `is_known_safe_command`. - Rationale: these failures are test-harness flakes, not product behaviors. The fixes keep the assertions intact and remove environment-sensitive startup and ordering hazards instead of stretching timeouts. ## Constraints diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index d30e5a3eaf..4aad4eb895 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -371,6 +371,7 @@ pub(crate) struct CodexSpawnArgs { pub(crate) persist_extended_history: bool, pub(crate) metrics_service_name: Option, pub(crate) inherited_shell_snapshot: Option>, + pub(crate) user_shell_override: Option, pub(crate) parent_trace: Option, } @@ -422,6 +423,7 @@ impl Codex { persist_extended_history, metrics_service_name, inherited_shell_snapshot, + user_shell_override, parent_trace: _, } = args; let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); @@ -576,6 +578,7 @@ impl Codex { dynamic_tools, persist_extended_history, inherited_shell_snapshot, + user_shell_override, }; // Generate a unique ID for the lifetime of this Codex session. @@ -1036,6 +1039,7 @@ pub(crate) struct SessionConfiguration { dynamic_tools: Vec, persist_extended_history: bool, inherited_shell_snapshot: Option>, + user_shell_override: Option, } impl SessionConfiguration { @@ -1616,7 +1620,11 @@ impl Session { ); let use_zsh_fork_shell = config.features.enabled(Feature::ShellZshFork); - let mut default_shell = if use_zsh_fork_shell { + let mut default_shell = if let Some(user_shell_override) = + session_configuration.user_shell_override.clone() + { + user_shell_override + } else if use_zsh_fork_shell { let zsh_path = config.zsh_path.as_ref().ok_or_else(|| { anyhow::anyhow!( "zsh fork feature enabled, but `zsh_path` is not configured; set `zsh_path` in config.toml" diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 1142612ae3..9f175029b4 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -88,6 +88,7 @@ pub(crate) async fn run_codex_thread_interactive( persist_extended_history: false, metrics_service_name: None, inherited_shell_snapshot: None, + user_shell_override: None, parent_trace: None, }) .await?; diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 37c397c717..f52a6a61fb 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -1563,6 +1563,7 @@ async fn set_rate_limits_retains_previous_credits() { dynamic_tools: Vec::new(), persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, }; let mut state = SessionState::new(session_configuration); @@ -1660,6 +1661,7 @@ async fn set_rate_limits_updates_plan_type_when_present() { dynamic_tools: Vec::new(), persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, }; let mut state = SessionState::new(session_configuration); @@ -2015,6 +2017,7 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati dynamic_tools: Vec::new(), persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, } } @@ -2245,6 +2248,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { dynamic_tools: Vec::new(), persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, }; let (tx_event, _rx_event) = async_channel::unbounded(); @@ -2339,6 +2343,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { dynamic_tools: Vec::new(), persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, }; let per_turn_config = Session::build_per_turn_config(&session_configuration); let model_info = ModelsManager::construct_model_info_offline_for_tests( @@ -3129,6 +3134,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( dynamic_tools, persist_extended_history: false, inherited_shell_snapshot: None, + user_shell_override: None, }; let per_turn_config = Session::build_per_turn_config(&session_configuration); let model_info = ModelsManager::construct_model_info_offline_for_tests( diff --git a/codex-rs/core/src/codex_tests_guardian.rs b/codex-rs/core/src/codex_tests_guardian.rs index 8c96407f50..20f09e759f 100644 --- a/codex-rs/core/src/codex_tests_guardian.rs +++ b/codex-rs/core/src/codex_tests_guardian.rs @@ -452,6 +452,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { persist_extended_history: false, metrics_service_name: None, inherited_shell_snapshot: None, + user_shell_override: None, parent_trace: None, }) .await diff --git a/codex-rs/core/src/test_support.rs b/codex-rs/core/src/test_support.rs index 12ba7cda82..c2aad83df4 100644 --- a/codex-rs/core/src/test_support.rs +++ b/codex-rs/core/src/test_support.rs @@ -64,6 +64,33 @@ pub fn thread_manager_with_models_provider_and_home( ThreadManager::with_models_provider_and_home_for_tests(auth, provider, codex_home) } +pub async fn start_thread_with_user_shell_override( + thread_manager: &ThreadManager, + config: Config, + user_shell_override: crate::shell::Shell, +) -> crate::error::Result { + thread_manager + .start_thread_with_user_shell_override_for_tests(config, user_shell_override) + .await +} + +pub async fn resume_thread_from_rollout_with_user_shell_override( + thread_manager: &ThreadManager, + config: Config, + rollout_path: PathBuf, + auth_manager: Arc, + user_shell_override: crate::shell::Shell, +) -> crate::error::Result { + thread_manager + .resume_thread_from_rollout_with_user_shell_override_for_tests( + config, + rollout_path, + auth_manager, + user_shell_override, + ) + .await +} + pub fn models_manager_with_provider( codex_home: PathBuf, auth_manager: Arc, diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 31088b9c42..2404e27f57 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -376,6 +376,7 @@ impl ThreadManager { persist_extended_history, metrics_service_name, parent_trace, + None, )) .await } @@ -415,6 +416,48 @@ impl ThreadManager { persist_extended_history, None, parent_trace, + None, + )) + .await + } + + pub(crate) async fn start_thread_with_user_shell_override_for_tests( + &self, + config: Config, + user_shell_override: crate::shell::Shell, + ) -> CodexResult { + Box::pin(self.state.spawn_thread( + config, + InitialHistory::New, + Arc::clone(&self.state.auth_manager), + self.agent_control(), + Vec::new(), + false, + None, + None, + Some(user_shell_override), + )) + .await + } + + pub(crate) async fn resume_thread_from_rollout_with_user_shell_override_for_tests( + &self, + config: Config, + rollout_path: PathBuf, + auth_manager: Arc, + user_shell_override: crate::shell::Shell, + ) -> CodexResult { + let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?; + Box::pin(self.state.spawn_thread( + config, + initial_history, + auth_manager, + self.agent_control(), + Vec::new(), + false, + None, + None, + Some(user_shell_override), )) .await } @@ -500,6 +543,7 @@ impl ThreadManager { persist_extended_history, None, parent_trace, + None, )) .await } @@ -585,6 +629,7 @@ impl ThreadManagerState { metrics_service_name, inherited_shell_snapshot, None, + None, )) .await } @@ -609,6 +654,7 @@ impl ThreadManagerState { None, inherited_shell_snapshot, None, + None, )) .await } @@ -633,6 +679,7 @@ impl ThreadManagerState { None, inherited_shell_snapshot, None, + None, )) .await } @@ -649,6 +696,7 @@ impl ThreadManagerState { persist_extended_history: bool, metrics_service_name: Option, parent_trace: Option, + user_shell_override: Option, ) -> CodexResult { Box::pin(self.spawn_thread_with_source( config, @@ -661,6 +709,7 @@ impl ThreadManagerState { metrics_service_name, None, parent_trace, + user_shell_override, )) .await } @@ -678,6 +727,7 @@ impl ThreadManagerState { metrics_service_name: Option, inherited_shell_snapshot: Option>, parent_trace: Option, + user_shell_override: Option, ) -> CodexResult { let watch_registration = self .file_watcher @@ -699,6 +749,7 @@ impl ThreadManagerState { persist_extended_history, metrics_service_name, inherited_shell_snapshot, + user_shell_override, parent_trace, }) .await?; diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index b69f3be230..6d5c35cda5 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -7,8 +7,6 @@ use pretty_assertions::assert_eq; use crate::codex::make_session_and_context; use crate::exec_env::create_env; use crate::is_safe_command::is_known_safe_command; -use crate::powershell::try_find_powershell_executable_blocking; -use crate::powershell::try_find_pwsh_executable_blocking; use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; @@ -34,24 +32,8 @@ fn commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_c shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; assert_safe(&zsh_shell, "ls -la"); - - if let Some(path) = try_find_powershell_executable_blocking() { - let powershell = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - assert_safe(&powershell, "ls -Name"); - } - - if let Some(path) = try_find_pwsh_executable_blocking() { - let pwsh = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - assert_safe(&pwsh, "ls -Name"); - } + // PowerShell wrapper safety lives in codex-shell-command, which already + // covers real executable discovery and parser behavior directly. } fn assert_safe(shell: &Shell, command: &str) { diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index d86a9c01d6..1359f5f413 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -13,6 +13,8 @@ use codex_core::built_in_model_providers; use codex_core::config::Config; use codex_core::features::Feature; use codex_core::models_manager::collaboration_mode_presets::CollaborationModesConfig; +use codex_core::shell::Shell; +use codex_core::shell::get_shell_by_model_provided_path; use codex_protocol::config_types::ServiceTier; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::protocol::AskForApproval; @@ -64,6 +66,7 @@ pub struct TestCodexBuilder { auth: CodexAuth, pre_build_hooks: Vec>, home: Option>, + user_shell_override: Option, } impl TestCodexBuilder { @@ -100,6 +103,19 @@ impl TestCodexBuilder { self } + pub fn with_user_shell(mut self, user_shell: Shell) -> Self { + self.user_shell_override = Some(user_shell); + self + } + + pub fn with_windows_cmd_shell(self) -> Self { + if cfg!(windows) { + self.with_user_shell(get_shell_by_model_provided_path(&PathBuf::from("cmd.exe"))) + } else { + self + } + } + pub async fn build(&mut self, server: &wiremock::MockServer) -> anyhow::Result { let home = match self.home.clone() { Some(home) => home, @@ -194,9 +210,23 @@ impl TestCodexBuilder { ) }; let thread_manager = Arc::new(thread_manager); + let user_shell_override = self.user_shell_override.clone(); - let new_conversation = match resume_from { - Some(path) => { + let new_conversation = match (resume_from, user_shell_override) { + (Some(path), Some(user_shell_override)) => { + let auth_manager = codex_core::test_support::auth_manager_from_auth(auth); + Box::pin( + codex_core::test_support::resume_thread_from_rollout_with_user_shell_override( + thread_manager.as_ref(), + config.clone(), + path, + auth_manager, + user_shell_override, + ), + ) + .await? + } + (Some(path), None) => { let auth_manager = codex_core::test_support::auth_manager_from_auth(auth); Box::pin(thread_manager.resume_thread_from_rollout( config.clone(), @@ -206,7 +236,17 @@ impl TestCodexBuilder { )) .await? } - None => Box::pin(thread_manager.start_thread(config.clone())).await?, + (None, Some(user_shell_override)) => { + Box::pin( + codex_core::test_support::start_thread_with_user_shell_override( + thread_manager.as_ref(), + config.clone(), + user_shell_override, + ), + ) + .await? + } + (None, None) => Box::pin(thread_manager.start_thread(config.clone())).await?, }; Ok(TestCodex { @@ -552,6 +592,7 @@ pub fn test_codex() -> TestCodexBuilder { auth: CodexAuth::from_api_key("dummy"), pre_build_hooks: vec![], home: None, + user_shell_override: None, } } diff --git a/codex-rs/core/tests/suite/agent_websocket.rs b/codex-rs/core/tests/suite/agent_websocket.rs index 45752f1826..6b38ca2b45 100644 --- a/codex-rs/core/tests/suite/agent_websocket.rs +++ b/codex-rs/core/tests/suite/agent_websocket.rs @@ -35,7 +35,7 @@ async fn websocket_test_codex_shell_chain() -> Result<()> { ]]) .await; - let mut builder = test_codex(); + let mut builder = test_codex().with_windows_cmd_shell(); let test = builder.build_with_websocket_server(&server).await?; test.submit_turn_with_policy( @@ -183,7 +183,7 @@ async fn websocket_v2_test_codex_shell_chain() -> Result<()> { ]]) .await; - let mut builder = test_codex().with_config(|config| { + let mut builder = test_codex().with_windows_cmd_shell().with_config(|config| { config .features .enable(Feature::ResponsesWebsocketsV2) diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 28fdd0b83e..761c2c4b0e 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -740,7 +740,9 @@ async fn apply_patch_shell_command_heredoc_with_cd_updates_relative_workdir() -> async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result<()> { skip_if_no_network!(Ok(())); - let harness = apply_patch_harness_with(|builder| builder.with_model("gpt-5.1")).await?; + let harness = + apply_patch_harness_with(|builder| builder.with_model("gpt-5.1").with_windows_cmd_shell()) + .await?; let source_contents = "line1\nnaïve café\nline3\n"; let source_path = harness.path("source.txt"); @@ -786,7 +788,7 @@ async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result match call_num { 0 => { let command = if cfg!(windows) { - "Get-Content -Encoding utf8 source.txt" + r#"powershell.exe -NoLogo -NoProfile -Command "Get-Content -Encoding utf8 source.txt""# } else { "cat source.txt" }; @@ -807,9 +809,7 @@ async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result let body_json: serde_json::Value = request.body_json().expect("request body should be json"); let read_output = function_call_output_text(&body_json, &self.read_call_id); - eprintln!("read_output: \n{read_output}"); let stdout = stdout_from_shell_output(&read_output); - eprintln!("stdout: \n{stdout}"); let patch_lines = stdout .lines() .map(|line| format!("+{line}")) @@ -819,8 +819,6 @@ async fn apply_patch_cli_can_use_shell_command_output_as_patch_input() -> Result "*** Begin Patch\n*** Add File: target.txt\n{patch_lines}\n*** End Patch" ); - eprintln!("patch: \n{patch}"); - let body = sse(vec![ ev_response_created("resp-2"), ev_apply_patch_custom_tool_call(&self.apply_call_id, &patch),