From 9d7013eab084013572069b5d784cb78e78e35691 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 25 Feb 2026 10:31:37 -0800 Subject: [PATCH 1/3] Handle websocket timeout (#12791) Sometimes websockets will timeout with 400 error, ensure we retry it. --- .../src/endpoint/responses_websocket.rs | 75 ++++++++++++++----- .../core/tests/suite/client_websockets.rs | 38 ++++++++++ 2 files changed, 94 insertions(+), 19 deletions(-) diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index bdd32fbd5a..925f7d52d0 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -164,6 +164,8 @@ const X_CODEX_TURN_STATE_HEADER: &str = "x-codex-turn-state"; const X_MODELS_ETAG_HEADER: &str = "x-models-etag"; const X_REASONING_INCLUDED_HEADER: &str = "x-reasoning-included"; const OPENAI_MODEL_HEADER: &str = "openai-model"; +const WEBSOCKET_CONNECTION_LIMIT_REACHED_CODE: &str = "websocket_connection_limit_reached"; +const WEBSOCKET_CONNECTION_LIMIT_REACHED_MESSAGE: &str = "Responses websocket connection limit reached (60 minutes). Create a new websocket connection to continue."; pub struct ResponsesWebsocketConnection { stream: Arc>>, @@ -417,6 +419,12 @@ fn map_ws_error(err: WsError, url: &Url) -> ApiError { } } +#[derive(Debug, Deserialize)] +struct WrappedWebsocketError { + code: Option, + message: Option, +} + #[derive(Debug, Deserialize)] struct WrappedWebsocketErrorEvent { #[serde(rename = "type")] @@ -424,7 +432,7 @@ struct WrappedWebsocketErrorEvent { #[serde(alias = "status_code")] status: Option, #[serde(default)] - error: Option, + error: Option, #[serde(default)] headers: Option>, } @@ -437,7 +445,10 @@ fn parse_wrapped_websocket_error_event(payload: &str) -> Option Option { +fn map_wrapped_websocket_error_event( + event: WrappedWebsocketErrorEvent, + original_payload: String, +) -> Option { let WrappedWebsocketErrorEvent { status, error, @@ -445,28 +456,29 @@ fn map_wrapped_websocket_error_event(event: WrappedWebsocketErrorEvent) -> Optio .. } = event; + if let Some(error) = error.as_ref() + && let Some(code) = error.code.as_deref() + && code == WEBSOCKET_CONNECTION_LIMIT_REACHED_CODE + { + return Some(ApiError::Retryable { + message: error + .message + .clone() + .unwrap_or_else(|| WEBSOCKET_CONNECTION_LIMIT_REACHED_MESSAGE.to_string()), + delay: None, + }); + } + let status = StatusCode::from_u16(status?).ok()?; if status.is_success() { return None; } - let body = error.map(|error| { - serde_json::to_string_pretty(&serde_json::json!({ - "error": error - })) - .unwrap_or_else(|_| { - serde_json::json!({ - "error": error - }) - .to_string() - }) - }); - Some(ApiError::Transport(TransportError::Http { status, url: None, headers: headers.map(json_headers_to_http_headers), - body, + body: Some(original_payload), })) } @@ -551,7 +563,8 @@ async fn run_websocket_response_stream( Message::Text(text) => { trace!("websocket event: {text}"); if let Some(wrapped_error) = parse_wrapped_websocket_error_event(&text) - && let Some(error) = map_wrapped_websocket_error_event(wrapped_error) + && let Some(error) = + map_wrapped_websocket_error_event(wrapped_error, text.to_string()) { return Err(error); } @@ -639,7 +652,7 @@ mod tests { let wrapped_error = parse_wrapped_websocket_error_event(&payload) .expect("expected websocket error payload to be parsed"); - let api_error = map_wrapped_websocket_error_event(wrapped_error) + let api_error = map_wrapped_websocket_error_event(wrapped_error, payload) .expect("expected websocket error payload to map to ApiError"); let ApiError::Transport(TransportError::Http { @@ -699,7 +712,7 @@ mod tests { let wrapped_error = parse_wrapped_websocket_error_event(&payload) .expect("expected websocket error payload to be parsed"); - let api_error = map_wrapped_websocket_error_event(wrapped_error) + let api_error = map_wrapped_websocket_error_event(wrapped_error, payload) .expect("expected websocket error payload to map to ApiError"); let ApiError::Transport(TransportError::Http { status, body, .. }) = api_error else { panic!("expected ApiError::Transport(Http)"); @@ -710,6 +723,30 @@ mod tests { assert!(body.contains("Model does not support image inputs")); } + #[test] + fn parse_wrapped_websocket_error_event_with_connection_limit_maps_retryable() { + let payload = json!({ + "type": "error", + "status": 400, + "error": { + "type": "invalid_request_error", + "code": "websocket_connection_limit_reached", + "message": "Responses websocket connection limit reached (60 minutes). Create a new websocket connection to continue." + } + }) + .to_string(); + + let wrapped_error = parse_wrapped_websocket_error_event(&payload) + .expect("expected websocket error payload to be parsed"); + let api_error = map_wrapped_websocket_error_event(wrapped_error, payload) + .expect("expected websocket error payload to map to ApiError"); + let ApiError::Retryable { message, delay } = api_error else { + panic!("expected ApiError::Retryable"); + }; + assert_eq!(message, WEBSOCKET_CONNECTION_LIMIT_REACHED_MESSAGE); + assert_eq!(delay, None); + } + #[test] fn parse_wrapped_websocket_error_event_without_status_is_not_mapped() { let payload = json!({ @@ -727,7 +764,7 @@ mod tests { let wrapped_error = parse_wrapped_websocket_error_event(&payload) .expect("expected websocket error payload to be parsed"); - let api_error = map_wrapped_websocket_error_event(wrapped_error); + let api_error = map_wrapped_websocket_error_event(wrapped_error, payload); assert!(api_error.is_none()); } diff --git a/codex-rs/core/tests/suite/client_websockets.rs b/codex-rs/core/tests/suite/client_websockets.rs index 3dec0c4408..369d69ed20 100755 --- a/codex-rs/core/tests/suite/client_websockets.rs +++ b/codex-rs/core/tests/suite/client_websockets.rs @@ -923,6 +923,44 @@ async fn responses_websocket_invalid_request_error_with_status_is_forwarded() { server.shutdown().await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_websocket_connection_limit_error_reconnects_and_completes() { + skip_if_no_network!(); + + let websocket_connection_limit_error = json!({ + "type": "error", + "status": 400, + "error": { + "type": "invalid_request_error", + "code": "websocket_connection_limit_reached", + "message": "Responses websocket connection limit reached (60 minutes). Create a new websocket connection to continue." + } + }); + + let server = start_websocket_server(vec![ + vec![vec![websocket_connection_limit_error]], + vec![vec![ev_response_created("resp-1"), ev_completed("resp-1")]], + ]) + .await; + let mut builder = test_codex().with_config(|config| { + config.model_provider.request_max_retries = Some(0); + config.model_provider.stream_max_retries = Some(1); + }); + let test = builder + .build_with_websocket_server(&server) + .await + .expect("build websocket codex"); + + test.submit_turn("hello") + .await + .expect("submission should reconnect after websocket connection limit error"); + + let total_websocket_requests: usize = server.connections().iter().map(Vec::len).sum(); + assert_eq!(total_websocket_requests, 2); + + server.shutdown().await; +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn responses_websocket_appends_on_prefix() { skip_if_no_network!(); From 648a420cbf183f408f380aaa2da52089744f423f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 25 Feb 2026 11:05:27 -0800 Subject: [PATCH 2/3] fix: enforce sandbox envelope for zsh fork execution (#12800) ## Why Zsh fork execution was still able to bypass the `WorkspaceWrite` model in edge cases because the fork path reconstructed command execution without preserving sandbox wrappers, and command extraction only accepted shell invocations in a narrow positional shape. This can allow commands to run with broader filesystem access than expected, which breaks the sandbox safety model. ## What changed - Preserved the sandboxed `ExecRequest` produced by `attempt.env_for(...)` when entering the zsh fork path in [`unix_escalation.rs`](https://github.com/openai/codex/blob/main/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs). - Updated `CoreShellCommandExecutor` to execute the sandboxed command and working directory captured from `attempt.env_for(...)`, instead of re-running a freshly reconstructed shell command. - Made zsh-fork script extraction robust to wrapped invocations by scanning command arguments for `-c`/`-lc` rather than only matching the first positional form. - Added unit tests in `unix_escalation.rs` to lock in wrapper-tolerant parsing behavior and keep unsupported shell forms rejected. - Tightened the regression in [`skill_approval.rs`](https://github.com/openai/codex/blob/main/codex-rs/core/tests/suite/skill_approval.rs): - `shell_zsh_fork_still_enforces_workspace_write_sandbox` now uses an explicit `WorkspaceWrite` policy with `exclude_tmpdir_env_var: true` and `exclude_slash_tmp: true`. - The test attempts to write to `/tmp/...`, which is only reliably outside writable roots with those explicit exclusions set. ## Verification - Added and passed the new unit tests around `extract_shell_script` parsing behavior with wrapped command shapes. - `extract_shell_script_supports_wrapped_command_prefixes` - `extract_shell_script_rejects_unsupported_shell_invocation` - Verified the regression with the focused integration test: `shell_zsh_fork_still_enforces_workspace_write_sandbox`. ## Manual Testing Prior to this change, if I ran Codex via: ``` just codex --config zsh_path=/Users/mbolin/code/codex2/codex-rs/app-server/tests/suite/zsh --enable shell_zsh_fork ``` and asked: ``` what is the output of /bin/ps ``` it would run it, even though the default sandbox should prevent the agent from running `/bin/ps` because it is setuid on MacOS. But with this change, I now see the expected failure because it is blocked by the sandbox: ``` /bin/ps exited with status 1 and produced no output in this environment. ``` --- .../tools/runtimes/shell/unix_escalation.rs | 100 +++++++++++++----- codex-rs/core/tests/suite/skill_approval.rs | 88 +++++++++++++++ 2 files changed, 164 insertions(+), 24 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 7485825ab1..83f4fa32ac 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -72,14 +72,9 @@ pub(super) async fn try_run_zsh_fork( let sandbox_exec_request = attempt .env_for(spec, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; - // Keep env/network/sandbox metadata from `attempt.env_for()`, but build the - // script from the original shell argv. `attempt.env_for()` may wrap the - // command with `sandbox-exec` on macOS, and passing those wrapper flags - // (`-p`, `-D...`) through zsh breaks the zsh-fork path before subcommand - // approval runs. let crate::sandboxing::ExecRequest { - command: _sandbox_command, - cwd: _sandbox_cwd, + command, + cwd: sandbox_cwd, env: sandbox_env, network: sandbox_network, expiration: _sandbox_expiration, @@ -90,7 +85,7 @@ pub(super) async fn try_run_zsh_fork( justification, arg0, } = sandbox_exec_request; - let ParsedShellCommand { script, login } = extract_shell_script(command)?; + let ParsedShellCommand { script, login } = extract_shell_script(&command)?; let effective_timeout = Duration::from_millis( req.timeout_ms .unwrap_or(crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS), @@ -99,6 +94,8 @@ pub(super) async fn try_run_zsh_fork( ctx.session.services.exec_policy.current().as_ref().clone(), )); let command_executor = CoreShellCommandExecutor { + command, + cwd: sandbox_cwd, sandbox_policy, sandbox, env: sandbox_env, @@ -438,6 +435,8 @@ impl EscalationPolicy for CoreShellActionProvider { } struct CoreShellCommandExecutor { + command: Vec, + cwd: PathBuf, sandbox_policy: SandboxPolicy, sandbox: SandboxType, env: HashMap, @@ -452,8 +451,8 @@ struct CoreShellCommandExecutor { impl ShellCommandExecutor for CoreShellCommandExecutor { async fn run( &self, - command: Vec, - cwd: PathBuf, + _command: Vec, + _cwd: PathBuf, env: HashMap, cancel_rx: CancellationToken, ) -> anyhow::Result { @@ -466,8 +465,8 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { let result = crate::sandboxing::execute_env( crate::sandboxing::ExecRequest { - command, - cwd, + command: self.command.clone(), + cwd: self.cwd.clone(), env: exec_env, network: self.network.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), @@ -500,19 +499,20 @@ struct ParsedShellCommand { } fn extract_shell_script(command: &[String]) -> Result { - match command { - [_, flag, script, ..] if flag == "-c" => Ok(ParsedShellCommand { - script: script.clone(), - login: false, - }), - [_, flag, script, ..] if flag == "-lc" => Ok(ParsedShellCommand { - script: script.clone(), - login: true, - }), - _ => Err(ToolError::Rejected( - "unexpected shell command format for zsh-fork execution".to_string(), - )), + // Commands reaching zsh-fork can be wrapped by environment/sandbox helpers, so + // we search for the first `-c`/`-lc` triple anywhere in the argv rather + // than assuming it is the first positional form. + if let Some((script, login)) = command.windows(3).find_map(|parts| match parts { + [_, flag, script] if flag == "-c" => Some((script.to_owned(), false)), + [_, flag, script] if flag == "-lc" => Some((script.to_owned(), true)), + _ => None, + }) { + return Ok(ParsedShellCommand { script, login }); } + + Err(ToolError::Rejected( + "unexpected shell command format for zsh-fork execution".to_string(), + )) } fn map_exec_result( @@ -586,6 +586,58 @@ mod tests { ); } + #[test] + fn extract_shell_script_supports_wrapped_command_prefixes() { + assert_eq!( + extract_shell_script(&[ + "/usr/bin/env".into(), + "CODEX_EXECVE_WRAPPER=1".into(), + "/bin/zsh".into(), + "-lc".into(), + "echo hello".into() + ]) + .unwrap(), + ParsedShellCommand { + script: "echo hello".to_string(), + login: true, + } + ); + + assert_eq!( + extract_shell_script(&[ + "sandbox-exec".into(), + "-p".into(), + "sandbox_policy".into(), + "/bin/zsh".into(), + "-c".into(), + "pwd".into(), + ]) + .unwrap(), + ParsedShellCommand { + script: "pwd".to_string(), + login: false, + } + ); + } + + #[test] + fn extract_shell_script_rejects_unsupported_shell_invocation() { + let err = extract_shell_script(&[ + "sandbox-exec".into(), + "-fc".into(), + "echo not supported".into(), + ]) + .unwrap_err(); + assert!(matches!(err, super::ToolError::Rejected(_))); + assert_eq!( + match err { + super::ToolError::Rejected(reason) => reason, + _ => "".to_string(), + }, + "unexpected shell command format for zsh-fork execution" + ); + } + #[test] fn join_program_and_argv_replaces_original_argv_zero() { assert_eq!( diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index 147ecdd0c2..2756c03788 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -558,3 +558,91 @@ permissions: Ok(()) } + +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_still_enforces_workspace_write_sandbox() -> Result<()> { + use codex_config::Constrained; + use codex_protocol::protocol::AskForApproval; + + skip_if_no_network!(Ok(())); + + let Some(zsh_path) = find_test_zsh_path()? else { + return Ok(()); + }; + if !supports_exec_wrapper_intercept(&zsh_path) { + eprintln!( + "skipping zsh-fork sandbox test: zsh does not support EXEC_WRAPPER intercepts ({})", + zsh_path.display() + ); + return Ok(()); + } + let Ok(main_execve_wrapper_exe) = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper") + else { + eprintln!( + "skipping zsh-fork sandbox test: unable to resolve `codex-execve-wrapper` binary" + ); + return Ok(()); + }; + + let server = start_mock_server().await; + let tool_call_id = "zsh-fork-workspace-write-deny"; + let outside_path = "/tmp/codex-zsh-fork-workspace-write-deny.txt"; + let workspace_write_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let policy_for_config = workspace_write_policy.clone(); + let _ = fs::remove_file(outside_path); + let mut builder = test_codex() + .with_pre_build_hook(move |_| { + let _ = fs::remove_file(outside_path); + }) + .with_config(move |config| { + config.features.enable(Feature::ShellTool); + config.features.enable(Feature::ShellZshFork); + config.zsh_path = Some(zsh_path.clone()); + config.main_execve_wrapper_exe = Some(main_execve_wrapper_exe); + config.permissions.allow_login_shell = false; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); + config.permissions.sandbox_policy = Constrained::allow_any(policy_for_config); + }); + let test = builder.build(&server).await?; + + let command = format!("touch {outside_path}"); + let arguments = shell_command_arguments(&command)?; + let mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") + .await; + + submit_turn_with_policies( + &test, + "write outside workspace with zsh fork", + AskForApproval::Never, + workspace_write_policy, + ) + .await?; + + wait_for_turn_complete_without_skill_approval(&test).await; + + let call_output = mocks + .completion + .single_request() + .function_call_output(tool_call_id); + let output = call_output["output"].as_str().unwrap_or_default(); + assert!( + output.contains("Permission denied") + || output.contains("Operation not permitted") + || output.contains("Read-only file system"), + "expected sandbox denial, got output: {output:?}" + ); + assert!( + !Path::new(outside_path).exists(), + "command should not write outside workspace under WorkspaceWrite policy" + ); + + Ok(()) +} From dd829184040bc1ec1dc5299572311bd1548a8b4f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 25 Feb 2026 11:20:18 -0800 Subject: [PATCH 3/3] fix: make turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2 succeed on Bazel --- .../tests/suite/v2/turn_start_zsh_fork.rs | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) 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 340a5e156c..bdbb283432 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 @@ -13,6 +13,7 @@ use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::create_shell_command_sse_response; use app_test_support::to_response; +use codex_app_server_protocol::CommandAction; use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; @@ -542,7 +543,10 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() CommandExecutionApprovalDecision::Cancel, ]; let mut target_decision_index = 0; - while target_decision_index < target_decisions.len() { + let first_file_str = first_file.to_string_lossy().into_owned(); + let second_file_str = second_file.to_string_lossy().into_owned(); + let parent_shell_hint = format!("&& {}", &first_file_str); + while target_decision_index < target_decisions.len() || !saw_parent_approval { let server_req = timeout( DEFAULT_READ_TIMEOUT, mcp.read_stream_until_request_message(), @@ -558,16 +562,21 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() .command .as_deref() .expect("approval command should be present"); - let is_target_subcommand = (approval_command.starts_with("/bin/rm ") - || approval_command.starts_with("/usr/bin/rm ")) - && (approval_command.contains(&first_file.display().to_string()) - || approval_command.contains(&second_file.display().to_string())); + let has_first_file = approval_command.contains(&first_file_str); + let has_second_file = approval_command.contains(&second_file_str); + let mentions_rm_binary = + approval_command.contains("/bin/rm ") || approval_command.contains("/usr/bin/rm "); + let has_rm_action = params.command_actions.as_ref().is_some_and(|actions| { + actions.iter().any(|action| match action { + CommandAction::Read { name, .. } => name == "rm", + CommandAction::Unknown { command } => command.contains("rm"), + _ => false, + }) + }); + let is_target_subcommand = + (has_first_file != has_second_file) && (has_rm_action || mentions_rm_binary); + if is_target_subcommand { - assert!( - approval_command.contains(&first_file.display().to_string()) - || approval_command.contains(&second_file.display().to_string()), - "expected zsh subcommand approval for one of the rm commands, got: {approval_command}" - ); approved_subcommand_ids.push( params .approval_id @@ -577,7 +586,9 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() approved_subcommand_strings.push(approval_command.to_string()); } let is_parent_approval = approval_command.contains(&zsh_path.display().to_string()) - && approval_command.contains(&shell_command); + && (approval_command.contains(&shell_command) + || (has_first_file && has_second_file) + || approval_command.contains(&parent_shell_hint)); let decision = if is_target_subcommand { let decision = target_decisions[target_decision_index].clone(); target_decision_index += 1;