From a69d757cd8ef8310001186865911b69e4b4175e5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 23 Sep 2026 10:10:14 +0000 Subject: [PATCH] Emit command lifecycle events for unified exec launch failures (#47529) ## Why When unified exec cannot create a process, command lifecycle events are missing, leaving clients without a command failure notification. ## What changed Emit start and failure events for terminal process-creation errors in both regular and zsh-fork execution. Report no process ID, an exit code of `-1`, zero duration, and the launch diagnostic, while retaining plugin attribution. Once event publication starts, let it finish even if the caller is cancelled. Preserve approval rejection handling and retryable sandbox denials. ## Testing Extend app-server tests to cover pipe and PTY launch failures, plugin attribution on failed launches, and zsh-fork launch failures after approval. Assert failure metadata and check for duplicate lifecycle notifications while retaining approval-decline coverage. GitOrigin-RevId: 26ed191fc69b903ea4710dd1a6c3b031e8caa4eb --- CHANGELOG.md | 4 + .../app-server/tests/suite/v2/turn_start.rs | 66 +++++++++++++--- .../tests/suite/v2/turn_start_zsh_fork.rs | 50 +++++++++++-- .../core/src/tools/runtimes/unified_exec.rs | 14 +++- .../src/tools/runtimes/unified_exec/launch.rs | 75 +++++++++++++++++++ 5 files changed, 185 insertions(+), 24 deletions(-) create mode 100644 codex-rs/core/src/tools/runtimes/unified_exec/launch.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eb564c560..cc9e645e40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1,5 @@ The changelog can be found on the [releases page](https://github.com/openai/codex/releases). + +## Unreleased + +- Fix missing command lifecycle events when unified exec cannot create a process, preserving approval rejection and sandbox retry behavior. Thanks @Marvinthebored ([#44557](https://github.com/openai/codex/issues/44557)). diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 4179365c1e..57a0184665 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -5495,9 +5495,13 @@ async fn run_turn_start_file_change_approval_rejection_v2( Ok(()) } +#[cfg_attr(not(windows), test_case(None; "started"))] +#[test_case(Some(json!({"cmd": "echo unreachable", "workdir": "missing-work-directory", "tty": false})); "pipe_launch_failure")] +#[test_case(Some(json!({"cmd": "echo unreachable\u{0}", "tty": true, "login": false})); "pty_launch_failure")] #[tokio::test] -#[cfg_attr(windows, ignore = "process id reporting differs on Windows")] -async fn command_execution_notifications_include_process_id() -> Result<()> { +async fn command_execution_notifications_include_process_id( + launch_failure_args: Option, +) -> Result<()> { // TODO(anp): Add target-Windows process-id expectations for remote executors. skip_if_wine_exec!( Ok(()), @@ -5505,8 +5509,18 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { ); skip_if_no_network!(Ok(())); + let launch_failed = launch_failure_args.is_some(); + let command = if let Some(args) = launch_failure_args { + responses::sse(vec![ + responses::ev_response_created("launch"), + responses::ev_function_call("uexec-1", "exec_command", &args.to_string()), + responses::ev_completed("launch"), + ]) + } else { + create_exec_command_sse_response("uexec-1")? + }; let responses = vec![ - create_exec_command_sse_response("uexec-1")?, + command, create_final_assistant_message_sse_response("done")?, ]; let server = create_mock_responses_server_sequence(responses).await; @@ -5514,6 +5528,8 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { MockResponsesConfig::new(&server.uri()) .with_sandbox_mode("danger-full-access") .enable_feature(Feature::UnifiedExec) + .disable_feature(Feature::ShellZshFork) + .disable_feature(Feature::ShellSnapshot) .write(codex_home.path())?; let mut mcp = TestAppServer::builder() @@ -5564,7 +5580,7 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { }; assert_eq!(id, "uexec-1"); assert_eq!(status, CommandExecutionStatus::InProgress); - let started_process_id = started_process_id.expect("process id should be present"); + assert_eq!(started_process_id.is_none(), launch_failed); let completed_command = timeout(DEFAULT_READ_TIMEOUT, async { loop { @@ -5581,6 +5597,8 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { process_id: completed_process_id, status: completed_status, exit_code, + duration_ms, + aggregated_output, .. } = completed_command else { @@ -5594,15 +5612,22 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { ), "unexpected command execution status: {completed_status:?}" ); - if completed_status == CommandExecutionStatus::Completed { + if launch_failed { + assert_eq!( + (completed_status, exit_code, duration_ms), + (CommandExecutionStatus::Failed, Some(-1), Some(0)) + ); + assert!( + aggregated_output + .context("launch diagnostic")? + .starts_with("Failed to create unified exec process:") + ); + } else if completed_status == CommandExecutionStatus::Completed { assert_eq!(exit_code, Some(0)); } else { assert!(exit_code.is_some(), "expected exit_code for failed command"); } - assert_eq!( - completed_process_id.as_deref(), - Some(started_process_id.as_str()) - ); + assert_eq!(completed_process_id, started_process_id); timeout( DEFAULT_READ_TIMEOUT, @@ -5610,16 +5635,30 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { ) .await??; + for method in mcp.pending_notification_methods() { + let notification = mcp.read_stream_until_notification_message(&method).await?; + assert_ne!( + notification.params.context("notification params")?["item"]["id"], + "uexec-1" + ); + } + Ok(()) } #[cfg_attr(windows, ignore = "plugin attribution fixture is Unix-only")] +#[test_case(CommandExecutionStatus::Completed; "completed")] +#[test_case(CommandExecutionStatus::Failed; "launch_failure")] #[tokio::test] -async fn command_execution_notifications_include_trusted_plugin_id() -> Result<()> { +async fn command_execution_notifications_include_trusted_plugin_id( + expected_status: CommandExecutionStatus, +) -> Result<()> { skip_if_no_network!(Ok(())); skip_if_wine_exec!(Ok(()), "plugin attribution fixture is Unix-only"); let codex_home = TempDir::new()?; + let missing_cwd = codex_home.path().join("missing-work-directory"); + let launch_failed = expected_status == CommandExecutionStatus::Failed; let curated_sha = "0123456789abcdef0123456789abcdef01234567"; let plugin_root = codex_home .path() @@ -5661,7 +5700,7 @@ async fn command_execution_notifications_include_trusted_plugin_id() -> Result<( "/bin/sh".to_string(), script_path.to_string_lossy().into_owned(), ], - /*workdir*/ None, + launch_failed.then_some(missing_cwd.as_path()), /*timeout_ms*/ None, "plugin-command", )?, @@ -5672,7 +5711,10 @@ async fn command_execution_notifications_include_trusted_plugin_id() -> Result<( .with_approval_policy("on-request") .with_sandbox_mode("danger-full-access") .enable_feature(Feature::Plugins) + .enable_feature(Feature::UnifiedExec) .disable_feature(Feature::RemotePlugin) + .disable_feature(Feature::ShellZshFork) + .disable_feature(Feature::ShellSnapshot) .with_extra_config("[plugins.\"google-calendar@openai-api-curated\"]\nenabled = true") .write(codex_home.path())?; @@ -5733,7 +5775,7 @@ async fn command_execution_notifications_include_trusted_plugin_id() -> Result<( if method == "item/started" { assert_eq!(status, CommandExecutionStatus::InProgress); } else { - assert_eq!(status, CommandExecutionStatus::Completed); + assert_eq!(status, expected_status); } } diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index 863ccb901b..d97e66fb67 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -38,6 +38,7 @@ use std::collections::BTreeMap; use std::path::Path; use std::path::PathBuf; use tempfile::TempDir; +use test_case::test_case; use tokio::time::timeout; #[cfg(windows)] @@ -169,8 +170,12 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { Ok(()) } +#[test_case(CommandExecutionApprovalDecision::Decline; "declined")] +#[test_case(CommandExecutionApprovalDecision::Accept; "launch_failure")] #[tokio::test] -async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { +async fn turn_start_shell_zsh_fork_exec_approval_v2( + decision: CommandExecutionApprovalDecision, +) -> Result<()> { // TODO(anp): Remove after zsh-fork fixtures can run in the selected remote environment. skip_if_remote!( Ok(()), @@ -190,6 +195,8 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { }; eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); + let launch_failed = matches!(decision, CommandExecutionApprovalDecision::Accept); + let missing_cwd = workspace.join("missing-work-directory"); let responses = vec![ create_escalated_command_execution_sse_response( vec![ @@ -197,7 +204,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { "-c".to_string(), "print(42)".to_string(), ], - /*workdir*/ None, + launch_failed.then_some(missing_cwd.as_path()), Some(5000), "call-zsh-fork-decline", )?, @@ -209,6 +216,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { &server.uri(), "on-request", &BTreeMap::from([ + (Feature::UnifiedExec, true), (Feature::ShellZshFork, true), (Feature::ShellSnapshot, false), ]), @@ -253,9 +261,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { mcp.send_response( request_id, - serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: CommandExecutionApprovalDecision::Decline, - })?, + serde_json::to_value(CommandExecutionRequestApprovalResponse { decision })?, ) .await?; @@ -280,6 +286,8 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { id, status, exit_code, + process_id, + duration_ms, aggregated_output, .. } = completed_command_execution @@ -287,9 +295,24 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { unreachable!("loop ensures we break on command execution items"); }; assert_eq!(id, "call-zsh-fork-decline"); - assert_eq!(status, CommandExecutionStatus::Declined); - assert!(exit_code.is_none()); - assert!(aggregated_output.is_none()); + assert_eq!(process_id, None); + if launch_failed { + assert_eq!( + (status, exit_code, duration_ms), + (CommandExecutionStatus::Failed, Some(-1), Some(0)) + ); + assert!( + aggregated_output + .expect("launch diagnostic") + .starts_with("Failed to create unified exec process:") + ); + } else { + assert_eq!( + (status, exit_code, duration_ms), + (CommandExecutionStatus::Declined, None, None) + ); + assert!(aggregated_output.is_none()); + } timeout( DEFAULT_READ_TIMEOUT, @@ -297,6 +320,17 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { ) .await??; + let mut command_events = Vec::new(); + for method in mcp.pending_notification_methods() { + let notification = mcp.read_stream_until_notification_message(&method).await?; + if notification.params.expect("notification params")["item"]["id"] + == "call-zsh-fork-decline" + { + command_events.push(method); + } + } + assert_eq!(command_events, ["item/started"]); + Ok(()) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 354e9324d9..722b8dffc1 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -61,6 +61,10 @@ use std::path::PathBuf; use std::time::Duration; use tokio_util::sync::CancellationToken; +mod launch; + +use launch::with_launch_failure_events; + // Allow 5s for Guardian cleanup and 5s for controller processing after review. const REMOTE_NETWORK_POLICY_DECISION_MARGIN: Duration = Duration::from_secs(10); @@ -653,7 +657,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt .to_string(), )); } - let mut process = self + let process = self .manager .open_session_with_prepared_exec_env( req.process_id, @@ -674,7 +678,8 @@ impl<'a> ToolRuntime for UnifiedExecRunt })) } other => ToolError::Rejected(other.to_string()), - })?; + }); + let mut process = with_launch_failure_events(process, req, ctx).await?; process._shell_snapshot = shell_snapshot; return Ok(UnifiedExecAttempt { process, @@ -703,7 +708,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt error @ ToolError::Codex(_) => error, })?; let options = unified_exec_options(attempt.network_denial_cancellation_token.clone()); - let mut process = self + let process = self .manager .open_session_with_exec_env( req.process_id, @@ -721,7 +726,8 @@ impl<'a> ToolRuntime for UnifiedExecRunt Box::new(NoopSpawnLifecycle), req.turn_environment.environment.as_ref(), ) - .await?; + .await; + let mut process = with_launch_failure_events(process, req, ctx).await?; process._shell_snapshot = shell_snapshot; Ok(UnifiedExecAttempt { process, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec/launch.rs b/codex-rs/core/src/tools/runtimes/unified_exec/launch.rs new file mode 100644 index 0000000000..0e292ffd54 --- /dev/null +++ b/codex-rs/core/src/tools/runtimes/unified_exec/launch.rs @@ -0,0 +1,75 @@ +//! Reports terminal process-creation failures as command attempts without a process handle. +//! Approval rejections retain their own lifecycle, and sandbox denials remain retryable. +//! Once started, failure event publication survives caller cancellation. + +use super::UnifiedExecRequest; +use crate::tools::events::ToolEmitter; +use crate::tools::events::ToolEventCtx; +use crate::tools::events::ToolEventFailure; +use crate::tools::events::ToolEventStage; +use crate::tools::sandboxing::ToolCtx; +use crate::tools::sandboxing::ToolError; +use crate::unified_exec::UnifiedExecProcess; +use codex_protocol::protocol::ExecCommandSource; +use std::sync::Arc; +use tracing::Instrument; + +pub(super) async fn with_launch_failure_events( + result: Result, + req: &UnifiedExecRequest, + ctx: &ToolCtx, +) -> Result { + let Err(ToolError::Rejected(message)) = &result else { + return result; + }; + let plugin_attribution = if req.turn_environment.environment.is_remote() { + let file_system = req + .turn_environment + .environment + .get_filesystem_without_reconnect(); + ctx.step_context + .turn + .plugin_attribution_for_executor_command(&req.command, &req.cwd, file_system.as_ref()) + .await + } else { + req.cwd.to_abs_path().ok().and_then(|cwd| { + ctx.step_context + .turn + .plugin_attribution_for_command(&req.command, &cwd) + }) + }; + let emitter = ToolEmitter::unified_exec( + &req.command, + req.cwd.clone(), + ExecCommandSource::UnifiedExecStartup, + /*process_id*/ None, + plugin_attribution, + ); + let session = Arc::clone(&ctx.session); + let step_context = Arc::clone(&ctx.step_context); + let call_id = ctx.call_id.clone(); + let message = message.clone(); + // Dropping the join handle on cancellation leaves both lifecycle events running. + let publish = async move { + let model_context = step_context.model_context(); + let mut event_ctx = ToolEventCtx::new( + session.as_ref(), + step_context.turn.as_ref(), + &step_context.settings.model_info, + &call_id, + /*turn_diff_tracker*/ None, + ); + event_ctx.model_context = Some(&model_context); + emitter.emit(event_ctx, ToolEventStage::Begin).await; + emitter + .emit( + event_ctx, + ToolEventStage::Failure(ToolEventFailure::Message(message)), + ) + .await; + }; + if let Err(err) = tokio::spawn(publish.in_current_span()).await { + tracing::warn!(%err, "failed to publish unified exec launch failure"); + } + result +}