From 95e809c135344747008ae84c7b328399d29f1946 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Thu, 2 Apr 2026 17:02:56 -0700 Subject: [PATCH 1/6] Auto-trust cwd on thread start (#16492) - Persist trusted cwd state during thread/start when the resolved sandbox is elevated. - Add app-server coverage for trusted root resolution and confirm turn/start does not mutate trust. --- codex-rs/app-server/README.md | 2 +- .../app-server/src/codex_message_processor.rs | 59 ++++- .../app-server/tests/suite/v2/thread_start.rs | 215 +++++++++++++++++- .../app-server/tests/suite/v2/turn_start.rs | 61 +++++ 4 files changed, 324 insertions(+), 13 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 43cdbf08e4..fdd61fba48 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -132,7 +132,7 @@ Example with notification opt-out: ## API Overview -- `thread/start` — create a new thread; emits `thread/started` (including the current `thread.status`) and auto-subscribes you to turn/item events for that thread. +- `thread/start` — create a new thread; emits `thread/started` (including the current `thread.status`) and auto-subscribes you to turn/item events for that thread. When the request includes a `cwd` and the resolved sandbox is `workspace-write` or full access, app-server also marks that project as trusted in the user `config.toml`. - `thread/resume` — reopen an existing thread by id so subsequent `turn/start` calls append to it. - `thread/fork` — fork an existing thread into a new thread id by copying the stored history; if the source thread is currently mid-turn, the fork records the same interruption marker as `turn/interrupt` instead of inheriting an unmarked partial turn suffix. The returned `thread.forkedFromId` points at the source thread when known. Accepts `ephemeral: true` for an in-memory temporary fork, emits `thread/started` (including the current `thread.status`), and auto-subscribes you to turn/item events for the new thread. - `thread/list` — page through stored rollouts; supports cursor-based pagination and optional `modelProviders`, `sourceKinds`, `archived`, `cwd`, and `searchTerm` filters. Each returned `thread` includes `status` (`ThreadStatus`), defaulting to `notLoaded` when the thread is not currently loaded. diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index e5d51dd474..7910c417cc 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -235,6 +235,7 @@ use codex_features::Feature; use codex_features::Stage; use codex_feedback::CodexFeedback; use codex_git_utils::git_diff_to_remote; +use codex_git_utils::resolve_root_git_project_for_trust; use codex_login::AuthManager; use codex_login::AuthMode as CoreAuthMode; use codex_login::CLIENT_ID; @@ -255,6 +256,7 @@ use codex_protocol::ThreadId; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ForcedLoginMethod; use codex_protocol::config_types::Personality; +use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::dynamic_tools::DynamicToolSpec as CoreDynamicToolSpec; use codex_protocol::items::TurnItem; @@ -2190,10 +2192,11 @@ impl CodexMessageProcessor { experimental_raw_events: bool, request_trace: Option, ) { - let config = match derive_config_from_params( + let requested_cwd = typesafe_overrides.cwd.clone(); + let mut config = match derive_config_from_params( &cli_overrides, - config_overrides, - typesafe_overrides, + config_overrides.clone(), + typesafe_overrides.clone(), &cloud_requirements, &listener_task_context.codex_home, &runtime_feature_enablement, @@ -2211,6 +2214,56 @@ impl CodexMessageProcessor { } }; + if requested_cwd.is_some() + && !config.active_project.is_trusted() + && matches!( + config.permissions.sandbox_policy.get(), + codex_protocol::protocol::SandboxPolicy::WorkspaceWrite { .. } + | codex_protocol::protocol::SandboxPolicy::DangerFullAccess + | codex_protocol::protocol::SandboxPolicy::ExternalSandbox { .. } + ) + { + let trust_target = resolve_root_git_project_for_trust(config.cwd.as_path()) + .unwrap_or_else(|| config.cwd.to_path_buf()); + if let Err(err) = codex_core::config::set_project_trust_level( + &listener_task_context.codex_home, + trust_target.as_path(), + TrustLevel::Trusted, + ) { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to persist trusted project state: {err}"), + data: None, + }; + listener_task_context + .outgoing + .send_error(request_id, error) + .await; + return; + } + + config = match derive_config_from_params( + &cli_overrides, + config_overrides, + typesafe_overrides, + &cloud_requirements, + &listener_task_context.codex_home, + &runtime_feature_enablement, + ) + .await + { + Ok(config) => config, + Err(err) => { + let error = config_load_error(&err); + listener_task_context + .outgoing + .send_error(request_id, error) + .await; + return; + } + }; + } + let dynamic_tools = dynamic_tools.unwrap_or_default(); let core_dynamic_tools = if dynamic_tools.is_empty() { Vec::new() diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 979c36e119..3b116f8ed9 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -4,12 +4,14 @@ use app_test_support::McpProcess; use app_test_support::create_mock_responses_server_repeating_assistant; use app_test_support::to_response; use app_test_support::write_chatgpt_auth; +use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::McpServerStartupState; use codex_app_server_protocol::McpServerStatusUpdatedNotification; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SandboxMode; use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::ThreadStartResponse; @@ -17,6 +19,7 @@ use codex_app_server_protocol::ThreadStartedNotification; use codex_app_server_protocol::ThreadStatus; use codex_app_server_protocol::ThreadStatusChangedNotification; use codex_core::config::set_project_trust_level; +use codex_git_utils::resolve_root_git_project_for_trust; use codex_login::AuthCredentialsStoreMode; use codex_login::REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR; use codex_protocol::config_types::ServiceTier; @@ -48,7 +51,7 @@ async fn thread_start_creates_thread_and_emits_started() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri())?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; // Start server and initialize. let mut mcp = McpProcess::new(codex_home.path()).await?; @@ -231,7 +234,7 @@ async fn thread_start_respects_project_config_from_cwd() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri())?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; let workspace = TempDir::new()?; let project_config_dir = workspace.path().join(".codex"); @@ -272,7 +275,7 @@ async fn thread_start_accepts_flex_service_tier() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri())?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -300,7 +303,7 @@ async fn thread_start_accepts_metrics_service_name() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri())?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -327,7 +330,7 @@ async fn thread_start_accepts_metrics_service_name() -> Result<()> { async fn thread_start_ephemeral_remains_pathless() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml(codex_home.path(), &server.uri())?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -584,16 +587,210 @@ async fn thread_start_surfaces_cloud_requirements_load_errors() -> Result<()> { Ok(()) } -// Helper to create a config.toml pointing at the mock model server. -fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { +#[tokio::test] +async fn thread_start_with_elevated_sandbox_trusts_project_and_followup_loads_project_config() +-> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + + let workspace = TempDir::new()?; + let project_config_dir = workspace.path().join(".codex"); + std::fs::create_dir_all(&project_config_dir)?; + std::fs::write( + project_config_dir.join("config.toml"), + r#" +model_reasoning_effort = "high" +"#, + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let first_request = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + sandbox: Some(SandboxMode::WorkspaceWrite), + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(first_request)), + ) + .await??; + + let second_request = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + ..Default::default() + }) + .await?; + let second_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_request)), + ) + .await??; + let ThreadStartResponse { + approval_policy, + reasoning_effort, + .. + } = to_response::(second_response)?; + + assert_eq!(approval_policy, AskForApproval::OnRequest); + assert_eq!(reasoning_effort, Some(ReasoningEffort::High)); + + let config_toml = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + let trusted_root = resolve_root_git_project_for_trust(workspace.path()) + .unwrap_or_else(|| workspace.path().to_path_buf()); + assert!(config_toml.contains(&trusted_root.display().to_string())); + assert!(config_toml.contains("trust_level = \"trusted\"")); + + Ok(()) +} + +#[tokio::test] +async fn thread_start_with_nested_git_cwd_trusts_repo_root() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + + let repo_root = TempDir::new()?; + std::fs::create_dir(repo_root.path().join(".git"))?; + let nested = repo_root.path().join("nested/project"); + std::fs::create_dir_all(&nested)?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(nested.display().to_string()), + sandbox: Some(SandboxMode::WorkspaceWrite), + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + + let config_toml = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + let trusted_root = + resolve_root_git_project_for_trust(&nested).expect("git root should resolve"); + assert!(config_toml.contains(&trusted_root.display().to_string())); + assert!(!config_toml.contains(&nested.display().to_string())); + + Ok(()) +} + +#[tokio::test] +async fn thread_start_with_read_only_sandbox_does_not_persist_project_trust() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + + let workspace = TempDir::new()?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + + let config_toml = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + assert!(!config_toml.contains("trust_level = \"trusted\"")); + assert!(!config_toml.contains(&workspace.path().display().to_string())); + + Ok(()) +} + +#[tokio::test] +async fn thread_start_skips_trust_write_when_project_is_already_trusted() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + + let workspace = TempDir::new()?; + let project_config_dir = workspace.path().join(".codex"); + std::fs::create_dir_all(&project_config_dir)?; + std::fs::write( + project_config_dir.join("config.toml"), + r#" +model_reasoning_effort = "high" +"#, + )?; + set_project_trust_level(codex_home.path(), workspace.path(), TrustLevel::Trusted)?; + let config_before = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + sandbox: Some(SandboxMode::WorkspaceWrite), + ..Default::default() + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let ThreadStartResponse { + approval_policy, + reasoning_effort, + .. + } = to_response::(response)?; + + assert_eq!(approval_policy, AskForApproval::OnRequest); + assert_eq!(reasoning_effort, Some(ReasoningEffort::High)); + + let config_after = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + assert_eq!(config_after, config_before); + + Ok(()) +} + +fn create_config_toml_without_approval_policy( + codex_home: &Path, + server_uri: &str, +) -> std::io::Result<()> { + create_config_toml_with_optional_approval_policy( + codex_home, server_uri, /*approval_policy*/ None, + ) +} + +fn create_config_toml_with_optional_approval_policy( + codex_home: &Path, + server_uri: &str, + approval_policy: Option<&str>, +) -> std::io::Result<()> { let config_toml = codex_home.join("config.toml"); + let approval_policy = approval_policy + .map(|policy| format!("approval_policy = \"{policy}\"\n")) + .unwrap_or_default(); std::fs::write( config_toml, format!( r#" model = "mock-model" -approval_policy = "never" -sandbox_mode = "read-only" +{approval_policy}sandbox_mode = "read-only" model_provider = "mock_provider" 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 3155a4be9c..b99d1cb73e 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -2523,6 +2523,67 @@ async fn command_execution_notifications_include_process_id() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_with_elevated_override_does_not_persist_project_trust() -> Result<()> { + let responses = vec![create_final_assistant_message_sse_response("Done")?]; + let server = create_mock_responses_server_sequence_unchecked(responses).await; + + let codex_home = TempDir::new()?; + create_config_toml( + codex_home.path(), + &server.uri(), + "never", + &BTreeMap::from([(Feature::Personality, true)]), + )?; + + let workspace = TempDir::new()?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let thread_request = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().display().to_string()), + ..Default::default() + }) + .await?; + let thread_response: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_request)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(thread_response)?; + + let turn_request = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + cwd: Some(workspace.path().to_path_buf()), + sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::DangerFullAccess), + input: vec![V2UserInput::Text { + text: "Hello".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_request)), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let config_toml = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + assert!(!config_toml.contains("trust_level = \"trusted\"")); + assert!(!config_toml.contains(&workspace.path().display().to_string())); + + Ok(()) +} + // Helper to create a config.toml pointing at the mock model server. fn create_config_toml( codex_home: &Path, From cb9fb562a46c2217ff0386bb487932db828b3cda Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 2 Apr 2026 17:05:45 -0700 Subject: [PATCH 2/6] fix: address unused variable on windows (#16633) This slipped in during https://github.com/openai/codex/pull/16578. I am still working on getting Windows working properly with Bazel on PRs. --- codex-rs/tui/src/tui.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 78d9adb789..fc5d350527 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -569,11 +569,10 @@ impl Tui { terminal.invalidate_viewport(); } - let area = terminal.viewport_area; - // Update the y position for suspending so Ctrl-Z can place the cursor correctly. #[cfg(unix)] { + let area = terminal.viewport_area; let inline_area_bottom = if self.alt_screen_active.load(Ordering::Relaxed) { self.alt_saved_viewport .map(|r| r.bottom().saturating_sub(1)) From 862158b9e990f06c64419258f7ec661b94a4f8a1 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 2 Apr 2026 17:28:47 -0700 Subject: [PATCH 3/6] app-server: make thread/shellCommand tests shell-aware (#16635) ## Why `thread/shellCommand` executes the raw command string through the current user shell, which is PowerShell on Windows. The two v2 app-server tests in `app-server/tests/suite/v2/thread_shell_command.rs` used POSIX `printf`, so Bazel CI on Windows failed with `printf` not being recognized as a PowerShell command. For reference, the user-shell task wraps commands with the active shell before execution: [`core/src/tasks/user_shell.rs`](https://github.com/openai/codex/blob/7a3eec6fdb356bd71f80582119eb829179ff0da1/codex-rs/core/src/tasks/user_shell.rs#L120-L126). ## What Changed Added a test-local helper that builds a shell-appropriate output command and expected newline sequence from `default_user_shell()`: - PowerShell: `Write-Output '...'` with `\r\n` - Cmd: `echo ...` with `\r\n` - POSIX shells: `printf '%s\n' ...` with `\n` Both `thread_shell_command_runs_as_standalone_turn_and_persists_history` and `thread_shell_command_uses_existing_active_turn` now use that helper. ## Verification - `cargo test -p codex-app-server thread_shell_command` --- .../tests/suite/v2/thread_shell_command.rs | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs index 4e8a40fca2..d5e6903272 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_shell_command.rs @@ -26,6 +26,7 @@ use codex_app_server_protocol::TurnCompletedNotification; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::UserInput as V2UserInput; +use codex_core::shell::default_user_shell; use codex_features::FEATURES; use codex_features::Feature; use pretty_assertions::assert_eq; @@ -67,11 +68,12 @@ async fn thread_shell_command_runs_as_standalone_turn_and_persists_history() -> ) .await??; let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + let (shell_command, expected_output) = current_shell_output_command("hello from bang")?; let shell_id = mcp .send_thread_shell_command_request(ThreadShellCommandParams { thread_id: thread.id.clone(), - command: "printf 'hello from bang\\n'".to_string(), + command: shell_command, }) .await?; let shell_resp: JSONRPCResponse = timeout( @@ -93,7 +95,7 @@ async fn thread_shell_command_runs_as_standalone_turn_and_persists_history() -> assert_eq!(status, &CommandExecutionStatus::InProgress); let delta = wait_for_command_execution_output_delta(&mut mcp, &command_id).await?; - assert_eq!(delta.delta, "hello from bang\n"); + assert_eq!(delta.delta, expected_output); let completed = wait_for_command_execution_completed(&mut mcp, Some(&command_id)).await?; let ThreadItem::CommandExecution { @@ -110,7 +112,7 @@ async fn thread_shell_command_runs_as_standalone_turn_and_persists_history() -> assert_eq!(id, &command_id); assert_eq!(source, &CommandExecutionSource::UserShell); assert_eq!(status, &CommandExecutionStatus::Completed); - assert_eq!(aggregated_output.as_deref(), Some("hello from bang\n")); + assert_eq!(aggregated_output.as_deref(), Some(expected_output.as_str())); assert_eq!(*exit_code, Some(0)); timeout( @@ -147,7 +149,7 @@ async fn thread_shell_command_runs_as_standalone_turn_and_persists_history() -> }; assert_eq!(source, &CommandExecutionSource::UserShell); assert_eq!(status, &CommandExecutionStatus::Completed); - assert_eq!(aggregated_output.as_deref(), Some("hello from bang\n")); + assert_eq!(aggregated_output.as_deref(), Some(expected_output.as_str())); Ok(()) } @@ -196,6 +198,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { ) .await??; let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + let (shell_command, expected_output) = current_shell_output_command("active turn bang")?; let turn_id = mcp .send_turn_start_request(TurnStartParams { @@ -240,7 +243,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { let shell_id = mcp .send_thread_shell_command_request(ThreadShellCommandParams { thread_id: thread.id.clone(), - command: "printf 'active turn bang\\n'".to_string(), + command: shell_command, }) .await?; let shell_resp: JSONRPCResponse = timeout( @@ -269,7 +272,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { unreachable!("helper returns command execution item"); }; assert_eq!(source, &CommandExecutionSource::UserShell); - assert_eq!(aggregated_output.as_deref(), Some("active turn bang\n")); + assert_eq!(aggregated_output.as_deref(), Some(expected_output.as_str())); mcp.send_response( request_id, @@ -309,7 +312,7 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { source: CommandExecutionSource::UserShell, aggregated_output, .. - } if aggregated_output.as_deref() == Some("active turn bang\n") + } if aggregated_output.as_deref() == Some(expected_output.as_str()) ) }), "expected active-turn shell command to be persisted on the existing turn" @@ -318,6 +321,24 @@ async fn thread_shell_command_uses_existing_active_turn() -> Result<()> { Ok(()) } +fn current_shell_output_command(text: &str) -> Result<(String, String)> { + let command_and_output = match default_user_shell().name() { + "powershell" => { + let escaped_text = text.replace('\'', "''"); + ( + format!("Write-Output '{escaped_text}'"), + format!("{text}\r\n"), + ) + } + "cmd" => (format!("echo {text}"), format!("{text}\r\n")), + _ => { + let quoted_text = shlex::try_quote(text)?; + (format!("printf '%s\\n' {quoted_text}"), format!("{text}\n")) + } + }; + Ok(command_and_output) +} + async fn wait_for_command_execution_started( mcp: &mut McpProcess, expected_id: Option<&str>, From beb3978a3ba15c631d6844a967b05e0b68a3a196 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 2 Apr 2026 17:33:07 -0700 Subject: [PATCH 4/6] test: use cmd.exe for ProviderAuthScript on Windows (#16629) ## Why The Windows `ProviderAuthScript` test helpers do not need PowerShell. Running them through `cmd.exe` is enough to emit the next fixture token and rotate `tokens.txt`, and it avoids a PowerShell-specific dependency in these tests. ## What changed - Replaced the Windows `print-token.ps1` fixtures with `print-token.cmd` in `codex-rs/core/src/models_manager/manager_tests.rs` and `codex-rs/login/src/auth/auth_tests.rs`. - Switched the failing external-auth helper in `codex-rs/login/src/auth/auth_tests.rs` from `powershell.exe -Command 'exit 1'` to `cmd.exe /d /s /c 'exit /b 1'`. - Updated Windows timeout comments so they no longer call out PowerShell specifically. ## Verification - `cargo test -p codex-login` - `cargo test -p codex-core` (fails in unrelated `core/src/config/config_tests.rs` assertions in this checkout) --- .../core/src/models_manager/manager_tests.rs | 33 +++++++------ codex-rs/login/src/auth/auth_tests.rs | 46 +++++++++++-------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/codex-rs/core/src/models_manager/manager_tests.rs b/codex-rs/core/src/models_manager/manager_tests.rs index 6ab1c267bb..48c01f198c 100644 --- a/codex-rs/core/src/models_manager/manager_tests.rs +++ b/codex-rs/core/src/models_manager/manager_tests.rs @@ -112,10 +112,12 @@ impl ProviderAuthScript { fn new(tokens: &[&str]) -> std::io::Result { let tempdir = tempfile::tempdir()?; let tokens_file = tempdir.path().join("tokens.txt"); + // `cmd.exe`'s `set /p` treats LF-only input as one line, so use CRLF on Windows. + let token_line_ending = if cfg!(windows) { "\r\n" } else { "\n" }; let mut token_file_contents = String::new(); for token in tokens { token_file_contents.push_str(token); - token_file_contents.push('\n'); + token_file_contents.push_str(token_line_ending); } std::fs::write(&tokens_file, token_file_contents)?; @@ -142,23 +144,28 @@ mv tokens.next tokens.txt #[cfg(windows)] let (command, args) = { - let script_path = tempdir.path().join("print-token.ps1"); + let script_path = tempdir.path().join("print-token.cmd"); std::fs::write( &script_path, - r#"$lines = @(Get-Content -Path tokens.txt) -if ($lines.Count -eq 0) { exit 1 } -Write-Output $lines[0] -$lines | Select-Object -Skip 1 | Set-Content -Path tokens.txt + r#"@echo off +setlocal EnableExtensions DisableDelayedExpansion +set "first_line=" + ModelProviderAuthInfo { let timeout_ms = if cfg!(windows) { - // `powershell.exe` startup can be slow on loaded Windows CI workers + // Process startup can be slow on loaded Windows CI workers. 10_000 } else { 2_000 diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index 3ee877872f..fb7da63e79 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -355,10 +355,12 @@ impl ProviderAuthScript { fn new(tokens: &[&str]) -> std::io::Result { let tempdir = tempfile::tempdir()?; let token_file = tempdir.path().join("tokens.txt"); + // `cmd.exe`'s `set /p` treats LF-only input as one line, so use CRLF on Windows. + let token_line_ending = if cfg!(windows) { "\r\n" } else { "\n" }; let mut token_file_contents = String::new(); for token in tokens { token_file_contents.push_str(token); - token_file_contents.push('\n'); + token_file_contents.push_str(token_line_ending); } std::fs::write(&token_file, token_file_contents)?; @@ -385,23 +387,28 @@ mv tokens.next tokens.txt #[cfg(windows)] let (command, args) = { - let script_path = tempdir.path().join("print-token.ps1"); + let script_path = tempdir.path().join("print-token.cmd"); std::fs::write( &script_path, - r#"$lines = @(Get-Content -Path tokens.txt) -if ($lines.Count -eq 0) { exit 1 } -Write-Output $lines[0] -$lines | Select-Object -Skip 1 | Set-Content -Path tokens.txt + r#"@echo off +setlocal EnableExtensions DisableDelayedExpansion +set "first_line=" + Date: Thu, 2 Apr 2026 17:33:42 -0700 Subject: [PATCH 5/6] build: fix Bazel lzma-sys wiring (#16634) This seems to be required to fix bazel builds on an applied devbox ## Summary - add the Bazel `xz` module - wire `lzma-sys` directly to `@xz//:lzma` and disable its build script - refresh `MODULE.bazel.lock` ## Validation - `just bazel-lock-update` - `just bazel-lock-check` - `bazel run //codex-rs/cli:codex --run_under="cd $PWD &&" -- --version` - `just bazel-codex --version` Co-authored-by: Codex --- MODULE.bazel | 8 ++++++-- MODULE.bazel.lock | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 43f133db64..9ef6cbac89 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -234,14 +234,18 @@ crate.annotation( inject_repo(crate, "zlib") -# TODO(zbarsky): Enable annotation after fixing windows arm64 builds. +bazel_dep(name = "xz", version = "5.4.5.bcr.8") + crate.annotation( crate = "lzma-sys", - gen_build_script = "on", + gen_build_script = "off", + deps = ["@xz//:lzma"], ) bazel_dep(name = "openssl", version = "3.5.4.bcr.0") +inject_repo(crate, "xz") + crate.annotation( build_script_data = [ "@openssl//:gen_dir", diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index e1a08359b2..7ff0d76ad7 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -228,6 +228,8 @@ "https://bcr.bazel.build/modules/upb/0.0.0-20220923-a547704/MODULE.bazel": "7298990c00040a0e2f121f6c32544bab27d4452f80d9ce51349b1a28f3005c43", "https://bcr.bazel.build/modules/with_cfg.bzl/0.12.0/MODULE.bazel": "b573395fe63aef4299ba095173e2f62ccfee5ad9bbf7acaa95dba73af9fc2b38", "https://bcr.bazel.build/modules/with_cfg.bzl/0.12.0/source.json": "3f3fbaeafecaf629877ad152a2c9def21f8d330d91aa94c5dc75bbb98c10b8b8", + "https://bcr.bazel.build/modules/xz/5.4.5.bcr.8/MODULE.bazel": "e48a69bd54053c2ec5fffc2a29fb70122afd3e83ab6c07068f63bc6553fa57cc", + "https://bcr.bazel.build/modules/xz/5.4.5.bcr.8/source.json": "bd7e928ccd63505b44f4784f7bbf12cc11f9ff23bf3ca12ff2c91cd74846099e", "https://bcr.bazel.build/modules/zlib/1.2.11/MODULE.bazel": "07b389abc85fdbca459b69e2ec656ae5622873af3f845e1c9d80fe179f3effa0", "https://bcr.bazel.build/modules/zlib/1.3.1.bcr.5/MODULE.bazel": "eec517b5bbe5492629466e11dae908d043364302283de25581e3eb944326c4ca", "https://bcr.bazel.build/modules/zlib/1.3.1.bcr.8/MODULE.bazel": "772c674bb78a0342b8caf32ab5c25085c493ca4ff08398208dcbe4375fe9f776", From 4c8f6330853e97d44a37a0914591e07a21a395da Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 2 Apr 2026 21:56:10 -0700 Subject: [PATCH 6/6] refactor: move shell and snapshot code out of codex-core --- codex-rs/Cargo.lock | 21 + codex-rs/Cargo.toml | 2 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/src/codex.rs | 29 +- codex-rs/core/src/codex_tests.rs | 2 - .../core/src/environment_context_tests.rs | 18 +- codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/shell.rs | 415 +------------ codex-rs/core/src/shell_detect.rs | 24 - codex-rs/core/src/shell_snapshot.rs | 505 +--------------- codex-rs/core/src/shell_snapshot_tests.rs | 389 +------------ codex-rs/core/src/state/service.rs | 5 +- .../core/src/tools/handlers/shell_tests.rs | 36 +- .../core/src/tools/handlers/unified_exec.rs | 9 +- codex-rs/core/src/tools/runtimes/mod.rs | 2 +- codex-rs/core/src/tools/runtimes/mod_tests.rs | 39 +- codex-rs/core/src/tools/spec.rs | 13 - codex-rs/core/src/tools/spec_tests.rs | 8 +- codex-rs/shell-command/Cargo.toml | 1 + codex-rs/shell-command/src/bash.rs | 4 +- codex-rs/shell-command/src/lib.rs | 2 - codex-rs/shell-command/src/powershell.rs | 4 +- codex-rs/shell-command/src/shell_detect.rs | 32 -- codex-rs/shell/BUILD.bazel | 7 + codex-rs/shell/Cargo.toml | 33 ++ codex-rs/shell/src/lib.rs | 385 +++++++++++++ codex-rs/{core => shell}/src/shell_tests.rs | 52 +- codex-rs/shell/src/snapshot.rs | 543 ++++++++++++++++++ codex-rs/shell/src/snapshot_tests.rs | 367 ++++++++++++ codex-rs/tools/Cargo.toml | 1 + codex-rs/tools/src/lib.rs | 1 - codex-rs/tools/src/tool_config.rs | 16 +- codex-rs/tools/src/tool_config_tests.rs | 3 +- 33 files changed, 1472 insertions(+), 1498 deletions(-) delete mode 100644 codex-rs/core/src/shell_detect.rs delete mode 100644 codex-rs/shell-command/src/shell_detect.rs create mode 100644 codex-rs/shell/BUILD.bazel create mode 100644 codex-rs/shell/Cargo.toml create mode 100644 codex-rs/shell/src/lib.rs rename codex-rs/{core => shell}/src/shell_tests.rs (77%) create mode 100644 codex-rs/shell/src/snapshot.rs create mode 100644 codex-rs/shell/src/snapshot_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c59b8d9c4d..373d116ac2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1866,6 +1866,7 @@ dependencies = [ "codex-rollout", "codex-sandboxing", "codex-secrets", + "codex-shell", "codex-shell-command", "codex-shell-escalation", "codex-state", @@ -2571,6 +2572,24 @@ dependencies = [ "tracing", ] +[[package]] +name = "codex-shell" +version = "0.0.0" +dependencies = [ + "anyhow", + "codex-otel", + "codex-protocol", + "codex-utils-pty", + "libc", + "pretty_assertions", + "serde", + "tempfile", + "tokio", + "tracing", + "uuid", + "which 8.0.0", +] + [[package]] name = "codex-shell-command" version = "0.0.0" @@ -2578,6 +2597,7 @@ dependencies = [ "anyhow", "base64 0.22.1", "codex-protocol", + "codex-shell", "codex-utils-absolute-path", "once_cell", "pretty_assertions", @@ -2671,6 +2691,7 @@ dependencies = [ "codex-code-mode", "codex-features", "codex-protocol", + "codex-shell", "codex-utils-absolute-path", "codex-utils-pty", "pretty_assertions", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 2d80bbc69e..319e41e9fa 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -24,6 +24,7 @@ members = [ "config", "shell-command", "shell-escalation", + "shell", "skills", "core", "core-skills", @@ -149,6 +150,7 @@ codex-sandboxing = { path = "sandboxing" } codex-secrets = { path = "secrets" } codex-shell-command = { path = "shell-command" } codex-shell-escalation = { path = "shell-escalation" } +codex-shell = { path = "shell" } codex-skills = { path = "skills" } codex-state = { path = "state" } codex-stdio-to-uds = { path = "stdio-to-uds" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index ad82b6c086..9519166d6f 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -51,6 +51,7 @@ codex-protocol = { workspace = true } codex-rollout = { workspace = true } codex-rmcp-client = { workspace = true } codex-sandboxing = { workspace = true } +codex-shell = { workspace = true } codex-state = { workspace = true } codex-terminal-detection = { workspace = true } codex-tools = { workspace = true } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 73f5c6ed7d..6836221dc9 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -290,6 +290,7 @@ use crate::rollout::policy::EventPersistenceMode; use crate::session_startup_prewarm::SessionStartupPrewarmHandle; use crate::shell; use crate::shell_snapshot::ShellSnapshot; +use crate::shell_snapshot::spawn_stale_snapshot_cleanup; use crate::skills_watcher::SkillsWatcher; use crate::skills_watcher::SkillsWatcherEvent; use crate::state::ActiveTurn; @@ -1401,7 +1402,7 @@ impl Session { windows_sandbox_level: session_configuration.windows_sandbox_level, }) .with_unified_exec_shell_mode_for_session( - crate::tools::spec::tool_user_shell_type(user_shell), + user_shell.shell_type, shell_zsh_path, main_execve_wrapper_exe, ) @@ -1751,25 +1752,19 @@ impl Session { shell::default_user_shell() }; // Create the mutable state for the Session. - let shell_snapshot_tx = if config.features.enabled(Feature::ShellSnapshot) { + if config.features.enabled(Feature::ShellSnapshot) { if let Some(snapshot) = session_configuration.inherited_shell_snapshot.clone() { - let (tx, rx) = watch::channel(Some(snapshot)); - default_shell.shell_snapshot = rx; - tx + default_shell.set_shell_snapshot(Some(snapshot)); } else { - ShellSnapshot::start_snapshotting( + default_shell.start_snapshotting( config.codex_home.clone(), conversation_id, session_configuration.cwd.to_path_buf(), - &mut default_shell, session_telemetry.clone(), - ) + ); + spawn_stale_snapshot_cleanup(config.codex_home.clone(), conversation_id); } - } else { - let (tx, rx) = watch::channel(None); - default_shell.shell_snapshot = rx; - tx - }; + } let thread_name = match session_index::find_thread_name_by_id(&config.codex_home, &conversation_id) .instrument(info_span!( @@ -1885,7 +1880,6 @@ impl Session { hooks, rollout: Mutex::new(rollout_recorder), user_shell: Arc::new(default_shell), - shell_snapshot_tx, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), @@ -2326,14 +2320,13 @@ impl Session { return; } - ShellSnapshot::refresh_snapshot( + self.services.user_shell.refresh_snapshot( codex_home.to_path_buf(), self.conversation_id, next_cwd.to_path_buf(), - self.services.user_shell.as_ref().clone(), - self.services.shell_snapshot_tx.clone(), self.services.session_telemetry.clone(), ); + spawn_stale_snapshot_cleanup(codex_home.to_path_buf(), self.conversation_id); } pub(crate) async fn update_settings( @@ -5525,7 +5518,7 @@ async fn spawn_review_thread( windows_sandbox_level: parent_turn_context.windows_sandbox_level, }) .with_unified_exec_shell_mode_for_session( - crate::tools::spec::tool_user_shell_type(sess.services.user_shell.as_ref()), + sess.services.user_shell.shell_type, sess.services.shell_zsh_path.as_ref(), sess.services.main_execve_wrapper_exe.as_ref(), ) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index c939120739..2d89994761 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2677,7 +2677,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { }), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), - shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: auth_manager.clone(), @@ -3513,7 +3512,6 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( }), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), - shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), diff --git a/codex-rs/core/src/environment_context_tests.rs b/codex-rs/core/src/environment_context_tests.rs index 073f1fe169..57ce05e611 100644 --- a/codex-rs/core/src/environment_context_tests.rs +++ b/codex-rs/core/src/environment_context_tests.rs @@ -5,11 +5,7 @@ use core_test_support::test_path_buf; use pretty_assertions::assert_eq; fn fake_shell() -> Shell { - Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - } + Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")) } #[test] @@ -219,11 +215,7 @@ fn equals_except_shell_compares_cwd_differences() { fn equals_except_shell_ignores_shell() { let context1 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Bash, - shell_path: "/bin/bash".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + Shell::new(ShellType::Bash, "/bin/bash".into()), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -231,11 +223,7 @@ fn equals_except_shell_ignores_shell() { ); let context2 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Zsh, - shell_path: "/bin/zsh".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + Shell::new(ShellType::Zsh, "/bin/zsh".into()), /*current_date*/ None, /*timezone*/ None, /*network*/ None, diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 8ffc7acfb7..bdb9154d12 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -72,7 +72,6 @@ mod sandbox_tags; pub mod sandboxing; mod session_prefix; mod session_startup_prewarm; -mod shell_detect; pub mod skills; pub(crate) use skills::SkillError; pub(crate) use skills::SkillInjections; diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 48c760d667..25bf7d59cb 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -1,409 +1,6 @@ -use crate::shell_detect::detect_shell_type; -use crate::shell_snapshot::ShellSnapshot; -use serde::Deserialize; -use serde::Serialize; -use std::path::PathBuf; -use std::sync::Arc; -use tokio::sync::watch; - -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub enum ShellType { - Zsh, - Bash, - PowerShell, - Sh, - Cmd, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Shell { - pub(crate) shell_type: ShellType, - pub(crate) shell_path: PathBuf, - #[serde( - skip_serializing, - skip_deserializing, - default = "empty_shell_snapshot_receiver" - )] - pub(crate) shell_snapshot: watch::Receiver>>, -} - -impl Shell { - pub fn name(&self) -> &'static str { - match self.shell_type { - ShellType::Zsh => "zsh", - ShellType::Bash => "bash", - ShellType::PowerShell => "powershell", - ShellType::Sh => "sh", - ShellType::Cmd => "cmd", - } - } - - /// Takes a string of shell and returns the full list of command args to - /// use with `exec()` to run the shell command. - pub fn derive_exec_args(&self, command: &str, use_login_shell: bool) -> Vec { - match self.shell_type { - ShellType::Zsh | ShellType::Bash | ShellType::Sh => { - let arg = if use_login_shell { "-lc" } else { "-c" }; - vec![ - self.shell_path.to_string_lossy().to_string(), - arg.to_string(), - command.to_string(), - ] - } - ShellType::PowerShell => { - let mut args = vec![self.shell_path.to_string_lossy().to_string()]; - if !use_login_shell { - args.push("-NoProfile".to_string()); - } - - args.push("-Command".to_string()); - args.push(command.to_string()); - args - } - ShellType::Cmd => { - let mut args = vec![self.shell_path.to_string_lossy().to_string()]; - args.push("/c".to_string()); - args.push(command.to_string()); - args - } - } - } - - /// Return the shell snapshot if existing. - pub fn shell_snapshot(&self) -> Option> { - self.shell_snapshot.borrow().clone() - } -} - -pub(crate) fn empty_shell_snapshot_receiver() -> watch::Receiver>> { - let (_tx, rx) = watch::channel(None); - rx -} - -impl PartialEq for Shell { - fn eq(&self, other: &Self) -> bool { - self.shell_type == other.shell_type && self.shell_path == other.shell_path - } -} - -impl Eq for Shell {} - -#[cfg(unix)] -fn get_user_shell_path() -> Option { - let uid = unsafe { libc::getuid() }; - use std::ffi::CStr; - use std::mem::MaybeUninit; - use std::ptr; - - let mut passwd = MaybeUninit::::uninit(); - - // We cannot use getpwuid here: it returns pointers into libc-managed - // storage, which is not safe to read concurrently on all targets (the musl - // static build used by the CLI can segfault when parallel callers race on - // that buffer). getpwuid_r keeps the passwd data in caller-owned memory. - let suggested_buffer_len = unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) }; - let buffer_len = usize::try_from(suggested_buffer_len) - .ok() - .filter(|len| *len > 0) - .unwrap_or(1024); - let mut buffer = vec![0; buffer_len]; - - loop { - let mut result = ptr::null_mut(); - let status = unsafe { - libc::getpwuid_r( - uid, - passwd.as_mut_ptr(), - buffer.as_mut_ptr().cast(), - buffer.len(), - &mut result, - ) - }; - - if status == 0 { - if result.is_null() { - return None; - } - - let passwd = unsafe { passwd.assume_init_ref() }; - if passwd.pw_shell.is_null() { - return None; - } - - let shell_path = unsafe { CStr::from_ptr(passwd.pw_shell) } - .to_string_lossy() - .into_owned(); - return Some(PathBuf::from(shell_path)); - } - - if status != libc::ERANGE { - return None; - } - - // Retry with a larger buffer until libc can materialize the passwd entry. - let new_len = buffer.len().checked_mul(2)?; - if new_len > 1024 * 1024 { - return None; - } - buffer.resize(new_len, 0); - } -} - -#[cfg(not(unix))] -fn get_user_shell_path() -> Option { - None -} - -fn file_exists(path: &PathBuf) -> Option { - if std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) { - Some(PathBuf::from(path)) - } else { - None - } -} - -fn get_shell_path( - shell_type: ShellType, - provided_path: Option<&PathBuf>, - binary_name: &str, - fallback_paths: &[&str], -) -> Option { - // If exact provided path exists, use it - if provided_path.and_then(file_exists).is_some() { - return provided_path.cloned(); - } - - // Check if the shell we are trying to load is user's default shell - // if just use it - let default_shell_path = get_user_shell_path(); - if let Some(default_shell_path) = default_shell_path - && detect_shell_type(&default_shell_path) == Some(shell_type) - && file_exists(&default_shell_path).is_some() - { - return Some(default_shell_path); - } - - if let Ok(path) = which::which(binary_name) { - return Some(path); - } - - for path in fallback_paths { - //check exists - if let Some(path) = file_exists(&PathBuf::from(path)) { - return Some(path); - } - } - - None -} - -const ZSH_FALLBACK_PATHS: &[&str] = &["/bin/zsh"]; - -fn get_zsh_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Zsh, path, "zsh", ZSH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Zsh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -const BASH_FALLBACK_PATHS: &[&str] = &["/bin/bash"]; - -fn get_bash_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Bash, path, "bash", BASH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Bash, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -const SH_FALLBACK_PATHS: &[&str] = &["/bin/sh"]; - -fn get_sh_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Sh, path, "sh", SH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Sh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -// Note the `pwsh` and `powershell` fallback paths are where the respective -// shells are commonly installed on GitHub Actions Windows runners, but may not -// be present on all Windows machines: -// https://docs.github.com/en/actions/tutorials/build-and-test-code/powershell - -#[cfg(windows)] -const PWSH_FALLBACK_PATHS: &[&str] = &[r#"C:\Program Files\PowerShell\7\pwsh.exe"#]; -#[cfg(not(windows))] -const PWSH_FALLBACK_PATHS: &[&str] = &["/usr/local/bin/pwsh"]; - -#[cfg(windows)] -const POWERSHELL_FALLBACK_PATHS: &[&str] = - &[r#"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"#]; -#[cfg(not(windows))] -const POWERSHELL_FALLBACK_PATHS: &[&str] = &[]; - -fn get_powershell_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::PowerShell, path, "pwsh", PWSH_FALLBACK_PATHS) - .or_else(|| { - get_shell_path( - ShellType::PowerShell, - path, - "powershell", - POWERSHELL_FALLBACK_PATHS, - ) - }); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::PowerShell, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn get_cmd_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Cmd, path, "cmd", &[]); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Cmd, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn ultimate_fallback_shell() -> Shell { - if cfg!(windows) { - Shell { - shell_type: ShellType::Cmd, - shell_path: PathBuf::from("cmd.exe"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } else { - Shell { - shell_type: ShellType::Sh, - shell_path: PathBuf::from("/bin/sh"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } -} - -pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> Shell { - detect_shell_type(shell_path) - .and_then(|shell_type| get_shell(shell_type, Some(shell_path))) - .unwrap_or(ultimate_fallback_shell()) -} - -pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { - match shell_type { - ShellType::Zsh => get_zsh_shell(path), - ShellType::Bash => get_bash_shell(path), - ShellType::PowerShell => get_powershell_shell(path), - ShellType::Sh => get_sh_shell(path), - ShellType::Cmd => get_cmd_shell(path), - } -} - -pub fn default_user_shell() -> Shell { - default_user_shell_from_path(get_user_shell_path()) -} - -fn default_user_shell_from_path(user_shell_path: Option) -> Shell { - if cfg!(windows) { - get_shell(ShellType::PowerShell, /*path*/ None).unwrap_or(ultimate_fallback_shell()) - } else { - let user_default_shell = user_shell_path - .and_then(|shell| detect_shell_type(&shell)) - .and_then(|shell_type| get_shell(shell_type, /*path*/ None)); - - let shell_with_fallback = if cfg!(target_os = "macos") { - user_default_shell - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - } else { - user_default_shell - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - }; - - shell_with_fallback.unwrap_or(ultimate_fallback_shell()) - } -} - -#[cfg(test)] -mod detect_shell_type_tests { - use super::*; - - #[test] - fn test_detect_shell_type() { - assert_eq!( - detect_shell_type(&PathBuf::from("zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell")), - Some(ShellType::PowerShell) - ); - assert_eq!(detect_shell_type(&PathBuf::from("fish")), None); - assert_eq!(detect_shell_type(&PathBuf::from("other")), None); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from(if cfg!(windows) { - "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" - } else { - "/usr/local/bin/pwsh" - })), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/usr/local/bin/pwsh")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/sh")), - Some(ShellType::Sh) - ); - assert_eq!(detect_shell_type(&PathBuf::from("sh")), Some(ShellType::Sh)); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd")), - Some(ShellType::Cmd) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd.exe")), - Some(ShellType::Cmd) - ); - } -} - -#[cfg(test)] -#[cfg(unix)] -#[path = "shell_tests.rs"] -mod tests; +pub use codex_shell::Shell; +pub use codex_shell::ShellType; +pub use codex_shell::default_user_shell; +pub use codex_shell::detect_shell_type; +pub use codex_shell::get_shell; +pub use codex_shell::get_shell_by_model_provided_path; diff --git a/codex-rs/core/src/shell_detect.rs b/codex-rs/core/src/shell_detect.rs deleted file mode 100644 index 3595ab3469..0000000000 --- a/codex-rs/core/src/shell_detect.rs +++ /dev/null @@ -1,24 +0,0 @@ -use crate::shell::ShellType; -use std::path::Path; -use std::path::PathBuf; - -pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option { - match shell_path.as_os_str().to_str() { - Some("zsh") => Some(ShellType::Zsh), - Some("sh") => Some(ShellType::Sh), - Some("cmd") => Some(ShellType::Cmd), - Some("bash") => Some(ShellType::Bash), - Some("pwsh") => Some(ShellType::PowerShell), - Some("powershell") => Some(ShellType::PowerShell), - _ => { - let shell_name = shell_path.file_stem(); - if let Some(shell_name) = shell_name { - let shell_name_path = Path::new(shell_name); - if shell_name_path != Path::new(shell_path) { - return detect_shell_type(&shell_name_path.to_path_buf()); - } - } - None - } - } -} diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 29b50cb9e8..096f3cd6e8 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -1,490 +1,17 @@ use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; -use std::process::Stdio; -use std::sync::Arc; -use std::time::Duration; use std::time::SystemTime; use crate::rollout::list::find_thread_path_by_id_str; -use crate::shell::Shell; -use crate::shell::ShellType; -use crate::shell::get_shell; -use anyhow::Context; use anyhow::Result; -use anyhow::anyhow; -use anyhow::bail; -use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; +pub use codex_shell::SNAPSHOT_DIR; +pub use codex_shell::SNAPSHOT_RETENTION; +pub use codex_shell::ShellSnapshot; +use codex_shell::remove_snapshot_file; +pub use codex_shell::snapshot_session_id_from_file_name; use tokio::fs; -use tokio::process::Command; -use tokio::sync::watch; -use tokio::time::timeout; -use tracing::Instrument; -use tracing::info_span; - -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct ShellSnapshot { - pub path: PathBuf, - pub cwd: PathBuf, -} - -const SNAPSHOT_TIMEOUT: Duration = Duration::from_secs(10); -const SNAPSHOT_RETENTION: Duration = Duration::from_secs(60 * 60 * 24 * 3); // 3 days retention. -const SNAPSHOT_DIR: &str = "shell_snapshots"; -const EXCLUDED_EXPORT_VARS: &[&str] = &["PWD", "OLDPWD"]; - -impl ShellSnapshot { - pub fn start_snapshotting( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - shell: &mut Shell, - session_telemetry: SessionTelemetry, - ) -> watch::Sender>> { - let (shell_snapshot_tx, shell_snapshot_rx) = watch::channel(None); - shell.shell_snapshot = shell_snapshot_rx; - - Self::spawn_snapshot_task( - codex_home, - session_id, - session_cwd, - shell.clone(), - shell_snapshot_tx.clone(), - session_telemetry, - ); - - shell_snapshot_tx - } - - pub fn refresh_snapshot( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - shell: Shell, - shell_snapshot_tx: watch::Sender>>, - session_telemetry: SessionTelemetry, - ) { - Self::spawn_snapshot_task( - codex_home, - session_id, - session_cwd, - shell, - shell_snapshot_tx, - session_telemetry, - ); - } - - fn spawn_snapshot_task( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - snapshot_shell: Shell, - shell_snapshot_tx: watch::Sender>>, - session_telemetry: SessionTelemetry, - ) { - let snapshot_span = info_span!("shell_snapshot", thread_id = %session_id); - tokio::spawn( - async move { - let timer = session_telemetry.start_timer("codex.shell_snapshot.duration_ms", &[]); - let snapshot = ShellSnapshot::try_new( - &codex_home, - session_id, - session_cwd.as_path(), - &snapshot_shell, - ) - .await - .map(Arc::new); - let success = snapshot.is_ok(); - let success_tag = if success { "true" } else { "false" }; - let _ = timer.map(|timer| timer.record(&[("success", success_tag)])); - let mut counter_tags = vec![("success", success_tag)]; - if let Some(failure_reason) = snapshot.as_ref().err() { - counter_tags.push(("failure_reason", *failure_reason)); - } - session_telemetry.counter("codex.shell_snapshot", /*inc*/ 1, &counter_tags); - let _ = shell_snapshot_tx.send(snapshot.ok()); - } - .instrument(snapshot_span), - ); - } - - async fn try_new( - codex_home: &Path, - session_id: ThreadId, - session_cwd: &Path, - shell: &Shell, - ) -> std::result::Result { - // File to store the snapshot - let extension = match shell.shell_type { - ShellType::PowerShell => "ps1", - _ => "sh", - }; - let nonce = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .map(|duration| duration.as_nanos()) - .unwrap_or(0); - let path = codex_home - .join(SNAPSHOT_DIR) - .join(format!("{session_id}.{nonce}.{extension}")); - let temp_path = codex_home - .join(SNAPSHOT_DIR) - .join(format!("{session_id}.tmp-{nonce}")); - - // Clean the (unlikely) leaked snapshot files. - let codex_home = codex_home.to_path_buf(); - let cleanup_session_id = session_id; - tokio::spawn(async move { - if let Err(err) = cleanup_stale_snapshots(&codex_home, cleanup_session_id).await { - tracing::warn!("Failed to clean up shell snapshots: {err:?}"); - } - }); - - // Make the new snapshot. - let temp_path = - match write_shell_snapshot(shell.shell_type.clone(), &temp_path, session_cwd).await { - Ok(path) => { - tracing::info!("Shell snapshot successfully created: {}", path.display()); - path - } - Err(err) => { - tracing::warn!( - "Failed to create shell snapshot for {}: {err:?}", - shell.name() - ); - return Err("write_failed"); - } - }; - - let temp_snapshot = Self { - path: temp_path.clone(), - cwd: session_cwd.to_path_buf(), - }; - - if let Err(err) = validate_snapshot(shell, &temp_snapshot.path, session_cwd).await { - tracing::error!("Shell snapshot validation failed: {err:?}"); - remove_snapshot_file(&temp_snapshot.path).await; - return Err("validation_failed"); - } - - if let Err(err) = fs::rename(&temp_snapshot.path, &path).await { - tracing::warn!("Failed to finalize shell snapshot: {err:?}"); - remove_snapshot_file(&temp_snapshot.path).await; - return Err("write_failed"); - } - - Ok(Self { - path, - cwd: session_cwd.to_path_buf(), - }) - } -} - -impl Drop for ShellSnapshot { - fn drop(&mut self) { - if let Err(err) = std::fs::remove_file(&self.path) { - tracing::warn!( - "Failed to delete shell snapshot at {:?}: {err:?}", - self.path - ); - } - } -} - -async fn write_shell_snapshot( - shell_type: ShellType, - output_path: &Path, - cwd: &Path, -) -> Result { - if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { - bail!("Shell snapshot not supported yet for {shell_type:?}"); - } - let shell = get_shell(shell_type.clone(), /*path*/ None) - .with_context(|| format!("No available shell for {shell_type:?}"))?; - - let raw_snapshot = capture_snapshot(&shell, cwd).await?; - let snapshot = strip_snapshot_preamble(&raw_snapshot)?; - - if let Some(parent) = output_path.parent() { - let parent_display = parent.display(); - fs::create_dir_all(parent) - .await - .with_context(|| format!("Failed to create snapshot parent {parent_display}"))?; - } - - let snapshot_path = output_path.display(); - fs::write(output_path, snapshot) - .await - .with_context(|| format!("Failed to write snapshot to {snapshot_path}"))?; - - Ok(output_path.to_path_buf()) -} - -async fn capture_snapshot(shell: &Shell, cwd: &Path) -> Result { - let shell_type = shell.shell_type.clone(); - match shell_type { - ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script(), cwd).await, - ShellType::Bash => run_shell_script(shell, &bash_snapshot_script(), cwd).await, - ShellType::Sh => run_shell_script(shell, &sh_snapshot_script(), cwd).await, - ShellType::PowerShell => run_shell_script(shell, powershell_snapshot_script(), cwd).await, - ShellType::Cmd => bail!("Shell snapshotting is not yet supported for {shell_type:?}"), - } -} - -fn strip_snapshot_preamble(snapshot: &str) -> Result { - let marker = "# Snapshot file"; - let Some(start) = snapshot.find(marker) else { - bail!("Snapshot output missing marker {marker}"); - }; - - Ok(snapshot[start..].to_string()) -} - -async fn validate_snapshot(shell: &Shell, snapshot_path: &Path, cwd: &Path) -> Result<()> { - let snapshot_path_display = snapshot_path.display(); - let script = format!("set -e; . \"{snapshot_path_display}\""); - run_script_with_timeout( - shell, - &script, - SNAPSHOT_TIMEOUT, - /*use_login_shell*/ false, - cwd, - ) - .await - .map(|_| ()) -} - -async fn run_shell_script(shell: &Shell, script: &str, cwd: &Path) -> Result { - run_script_with_timeout( - shell, - script, - SNAPSHOT_TIMEOUT, - /*use_login_shell*/ true, - cwd, - ) - .await -} - -async fn run_script_with_timeout( - shell: &Shell, - script: &str, - snapshot_timeout: Duration, - use_login_shell: bool, - cwd: &Path, -) -> Result { - let args = shell.derive_exec_args(script, use_login_shell); - let shell_name = shell.name(); - - // Handler is kept as guard to control the drop. The `mut` pattern is required because .args() - // returns a ref of handler. - let mut handler = Command::new(&args[0]); - handler.args(&args[1..]); - handler.stdin(Stdio::null()); - handler.current_dir(cwd); - #[cfg(unix)] - unsafe { - handler.pre_exec(|| { - codex_utils_pty::process_group::detach_from_tty()?; - Ok(()) - }); - } - handler.kill_on_drop(true); - let output = timeout(snapshot_timeout, handler.output()) - .await - .map_err(|_| anyhow!("Snapshot command timed out for {shell_name}"))? - .with_context(|| format!("Failed to execute {shell_name}"))?; - - if !output.status.success() { - let status = output.status; - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("Snapshot command exited with status {status}: {stderr}"); - } - - Ok(String::from_utf8_lossy(&output.stdout).into_owned()) -} - -fn excluded_exports_regex() -> String { - EXCLUDED_EXPORT_VARS.join("|") -} - -fn zsh_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [[ -n "$ZDOTDIR" ]]; then - rc="$ZDOTDIR/.zshrc" -else - rc="$HOME/.zshrc" -fi -[[ -r "$rc" ]] && . "$rc" -print '# Snapshot file' -print '# Unset all aliases to avoid conflicts with functions' -print 'unalias -a 2>/dev/null || true' -print '# Functions' -functions -print '' -setopt_count=$(setopt | wc -l | tr -d ' ') -print "# setopts $setopt_count" -setopt | sed 's/^/setopt /' -print '' -alias_count=$(alias -L | wc -l | tr -d ' ') -print "# aliases $alias_count" -alias -L -print '' -export_lines=$(export -p | awk ' -/^(export|declare -x|typeset -x) / { - line=$0 - name=line - sub(/^(export|declare -x|typeset -x) /, "", name) - sub(/=.*/, "", name) - if (name ~ /^(EXCLUDED_EXPORTS)$/) { - next - } - if (name ~ /^[A-Za-z_][A-Za-z0-9_]*$/) { - print line - } -}') -export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') -print "# exports $export_count" -if [[ -n "$export_lines" ]]; then - print -r -- "$export_lines" -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn bash_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [ -z "$BASH_ENV" ] && [ -r "$HOME/.bashrc" ]; then - . "$HOME/.bashrc" -fi -echo '# Snapshot file' -echo '# Unset all aliases to avoid conflicts with functions' -unalias -a 2>/dev/null || true -echo '# Functions' -declare -f -echo '' -bash_opts=$(set -o | awk '$2=="on"{print $1}') -bash_opt_count=$(printf '%s\n' "$bash_opts" | sed '/^$/d' | wc -l | tr -d ' ') -echo "# setopts $bash_opt_count" -if [ -n "$bash_opts" ]; then - printf 'set -o %s\n' $bash_opts -fi -echo '' -alias_count=$(alias -p | wc -l | tr -d ' ') -echo "# aliases $alias_count" -alias -p -echo '' -export_lines=$( - while IFS= read -r name; do - if [[ "$name" =~ ^(EXCLUDED_EXPORTS)$ ]]; then - continue - fi - if [[ ! "$name" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then - continue - fi - declare -xp "$name" 2>/dev/null || true - done < <(compgen -e) -) -export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') -echo "# exports $export_count" -if [ -n "$export_lines" ]; then - printf '%s\n' "$export_lines" -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn sh_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [ -n "$ENV" ] && [ -r "$ENV" ]; then - . "$ENV" -fi -echo '# Snapshot file' -echo '# Unset all aliases to avoid conflicts with functions' -unalias -a 2>/dev/null || true -echo '# Functions' -if command -v typeset >/dev/null 2>&1; then - typeset -f -elif command -v declare >/dev/null 2>&1; then - declare -f -fi -echo '' -if set -o >/dev/null 2>&1; then - sh_opts=$(set -o | awk '$2=="on"{print $1}') - sh_opt_count=$(printf '%s\n' "$sh_opts" | sed '/^$/d' | wc -l | tr -d ' ') - echo "# setopts $sh_opt_count" - if [ -n "$sh_opts" ]; then - printf 'set -o %s\n' $sh_opts - fi -else - echo '# setopts 0' -fi -echo '' -if alias >/dev/null 2>&1; then - alias_count=$(alias | wc -l | tr -d ' ') - echo "# aliases $alias_count" - alias - echo '' -else - echo '# aliases 0' -fi -if export -p >/dev/null 2>&1; then - export_lines=$(export -p | awk ' -/^(export|declare -x|typeset -x) / { - line=$0 - name=line - sub(/^(export|declare -x|typeset -x) /, "", name) - sub(/=.*/, "", name) - if (name ~ /^(EXCLUDED_EXPORTS)$/) { - next - } - if (name ~ /^[A-Za-z_][A-Za-z0-9_]*$/) { - print line - } -}') - export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') - echo "# exports $export_count" - if [ -n "$export_lines" ]; then - printf '%s\n' "$export_lines" - fi -else - export_count=$(env | sort | awk -F= '$1 ~ /^[A-Za-z_][A-Za-z0-9_]*$/ { count++ } END { print count }') - echo "# exports $export_count" - env | sort | while IFS='=' read -r key value; do - case "$key" in - ""|[0-9]*|*[!A-Za-z0-9_]*|EXCLUDED_EXPORTS) continue ;; - esac - escaped=$(printf "%s" "$value" | sed "s/'/'\"'\"'/g") - printf "export %s='%s'\n" "$key" "$escaped" - done -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn powershell_snapshot_script() -> &'static str { - r##"$ErrorActionPreference = 'Stop' -Write-Output '# Snapshot file' -Write-Output '# Unset all aliases to avoid conflicts with functions' -Write-Output 'Remove-Item Alias:* -ErrorAction SilentlyContinue' -Write-Output '# Functions' -Get-ChildItem Function: | ForEach-Object { - "function {0} {{`n{1}`n}}" -f $_.Name, $_.Definition -} -Write-Output '' -$aliases = Get-Alias -Write-Output ("# aliases " + $aliases.Count) -$aliases | ForEach-Object { - "Set-Alias -Name {0} -Value {1}" -f $_.Name, $_.Definition -} -Write-Output '' -$envVars = Get-ChildItem Env: -Write-Output ("# exports " + $envVars.Count) -$envVars | ForEach-Object { - $escaped = $_.Value -replace "'", "''" - "`$env:{0}='{1}'" -f $_.Name, $escaped -} -"## -} /// Removes shell snapshots that either lack a matching session rollout file or /// whose rollouts have not been updated within the retention window. @@ -547,22 +74,12 @@ pub async fn cleanup_stale_snapshots(codex_home: &Path, active_session_id: Threa Ok(()) } -async fn remove_snapshot_file(path: &Path) { - if let Err(err) = fs::remove_file(path).await { - tracing::warn!("Failed to delete shell snapshot at {:?}: {err:?}", path); - } -} - -fn snapshot_session_id_from_file_name(file_name: &str) -> Option<&str> { - let (stem, extension) = file_name.rsplit_once('.')?; - match extension { - "sh" | "ps1" => Some( - stem.split_once('.') - .map_or(stem, |(session_id, _generation)| session_id), - ), - _ if extension.starts_with("tmp-") => Some(stem), - _ => None, - } +pub(crate) fn spawn_stale_snapshot_cleanup(codex_home: PathBuf, active_session_id: ThreadId) { + tokio::spawn(async move { + if let Err(err) = cleanup_stale_snapshots(&codex_home, active_session_id).await { + tracing::warn!("Failed to clean up shell snapshots: {err:?}"); + } + }); } #[cfg(test)] diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index ff700ff7a6..79b27f2c23 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -1,101 +1,26 @@ use super::*; +use anyhow::Result; use pretty_assertions::assert_eq; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; +use std::path::Path; +use std::path::PathBuf; #[cfg(unix)] -use std::process::Command; -#[cfg(target_os = "linux")] -use std::process::Command as StdCommand; - +use std::time::Duration; +#[cfg(unix)] +use std::time::SystemTime; use tempfile::tempdir; -#[cfg(unix)] -struct BlockingStdinPipe { - original: i32, - write_end: i32, -} - -#[cfg(unix)] -impl BlockingStdinPipe { - fn install() -> Result { - let mut fds = [0i32; 2]; - if unsafe { libc::pipe(fds.as_mut_ptr()) } == -1 { - return Err(std::io::Error::last_os_error()).context("create stdin pipe"); - } - - let original = unsafe { libc::dup(libc::STDIN_FILENO) }; - if original == -1 { - let err = std::io::Error::last_os_error(); - unsafe { - libc::close(fds[0]); - libc::close(fds[1]); - } - return Err(err).context("dup stdin"); - } - - if unsafe { libc::dup2(fds[0], libc::STDIN_FILENO) } == -1 { - let err = std::io::Error::last_os_error(); - unsafe { - libc::close(fds[0]); - libc::close(fds[1]); - libc::close(original); - } - return Err(err).context("replace stdin"); - } - - unsafe { - libc::close(fds[0]); - } - - Ok(Self { - original, - write_end: fds[1], - }) - } -} - -#[cfg(unix)] -impl Drop for BlockingStdinPipe { - fn drop(&mut self) { - unsafe { - libc::dup2(self.original, libc::STDIN_FILENO); - libc::close(self.original); - libc::close(self.write_end); - } - } -} - -#[cfg(not(target_os = "windows"))] -fn assert_posix_snapshot_sections(snapshot: &str) { - assert!(snapshot.contains("# Snapshot file")); - assert!(snapshot.contains("aliases ")); - assert!(snapshot.contains("exports ")); - assert!( - snapshot.contains("PATH"), - "snapshot should capture a PATH export" - ); - assert!(snapshot.contains("setopts ")); -} - -async fn get_snapshot(shell_type: ShellType) -> Result { - let dir = tempdir()?; - let path = dir.path().join("snapshot.sh"); - write_shell_snapshot(shell_type, &path, dir.path()).await?; - let content = fs::read_to_string(&path).await?; - Ok(content) -} - -#[test] -fn strip_snapshot_preamble_removes_leading_output() { - let snapshot = "noise\n# Snapshot file\nexport PATH=/bin\n"; - let cleaned = strip_snapshot_preamble(snapshot).expect("snapshot marker exists"); - assert_eq!(cleaned, "# Snapshot file\nexport PATH=/bin\n"); -} - -#[test] -fn strip_snapshot_preamble_requires_marker() { - let result = strip_snapshot_preamble("missing header"); - assert!(result.is_err()); +async fn write_rollout_stub(codex_home: &Path, session_id: ThreadId) -> Result { + let dir = codex_home + .join("sessions") + .join("2025") + .join("01") + .join("01"); + fs::create_dir_all(&dir).await?; + let path = dir.join(format!("rollout-2025-01-01T00-00-00-{session_id}.jsonl")); + fs::write(&path, "").await?; + Ok(path) } #[test] @@ -120,286 +45,6 @@ fn snapshot_file_name_parser_supports_legacy_and_suffixed_names() { ); } -#[cfg(unix)] -#[test] -fn bash_snapshot_filters_invalid_exports() -> Result<()> { - let output = Command::new("/bin/bash") - .arg("-c") - .arg(bash_snapshot_script()) - .env("BASH_ENV", "/dev/null") - .env("VALID_NAME", "ok") - .env("PWD", "/tmp/stale") - .env("NEXTEST_BIN_EXE_codex-write-config-schema", "/path/to/bin") - .env("BAD-NAME", "broken") - .output()?; - - assert!(output.status.success()); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("VALID_NAME")); - assert!(!stdout.contains("PWD=/tmp/stale")); - assert!(!stdout.contains("NEXTEST_BIN_EXE_codex-write-config-schema")); - assert!(!stdout.contains("BAD-NAME")); - - Ok(()) -} - -#[cfg(unix)] -#[test] -fn bash_snapshot_preserves_multiline_exports() -> Result<()> { - let multiline_cert = "-----BEGIN CERTIFICATE-----\nabc\n-----END CERTIFICATE-----"; - let output = Command::new("/bin/bash") - .arg("-c") - .arg(bash_snapshot_script()) - .env("BASH_ENV", "/dev/null") - .env("MULTILINE_CERT", multiline_cert) - .output()?; - - assert!(output.status.success()); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!( - stdout.contains("MULTILINE_CERT=") || stdout.contains("MULTILINE_CERT"), - "snapshot should include the multiline export name" - ); - - let dir = tempdir()?; - let snapshot_path = dir.path().join("snapshot.sh"); - std::fs::write(&snapshot_path, stdout.as_bytes())?; - - let validate = Command::new("/bin/bash") - .arg("-c") - .arg("set -e; . \"$1\"") - .arg("bash") - .arg(&snapshot_path) - .env("BASH_ENV", "/dev/null") - .output()?; - - assert!( - validate.status.success(), - "snapshot validation failed: {}", - String::from_utf8_lossy(&validate.stderr) - ); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn try_new_creates_and_deletes_snapshot_file() -> Result<()> { - let dir = tempdir()?; - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let snapshot = ShellSnapshot::try_new(dir.path(), ThreadId::new(), dir.path(), &shell) - .await - .expect("snapshot should be created"); - let path = snapshot.path.clone(); - assert!(path.exists()); - assert_eq!(snapshot.cwd, dir.path().to_path_buf()); - - drop(snapshot); - - assert!(!path.exists()); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn try_new_uses_distinct_generation_paths() -> Result<()> { - let dir = tempdir()?; - let session_id = ThreadId::new(); - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let initial_snapshot = ShellSnapshot::try_new(dir.path(), session_id, dir.path(), &shell) - .await - .expect("initial snapshot should be created"); - let refreshed_snapshot = ShellSnapshot::try_new(dir.path(), session_id, dir.path(), &shell) - .await - .expect("refreshed snapshot should be created"); - let initial_path = initial_snapshot.path.clone(); - let refreshed_path = refreshed_snapshot.path.clone(); - - assert_ne!(initial_path, refreshed_path); - assert_eq!(initial_path.exists(), true); - assert_eq!(refreshed_path.exists(), true); - - drop(initial_snapshot); - - assert_eq!(initial_path.exists(), false); - assert_eq!(refreshed_path.exists(), true); - - drop(refreshed_snapshot); - - assert_eq!(refreshed_path.exists(), false); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn snapshot_shell_does_not_inherit_stdin() -> Result<()> { - let _stdin_guard = BlockingStdinPipe::install()?; - - let dir = tempdir()?; - let home = dir.path(); - let read_status_path = home.join("stdin-read-status"); - let read_status_display = read_status_path.display(); - // Persist the startup `read` exit status so the test can assert whether - // bash saw EOF on stdin after the snapshot process exits. - let bashrc = format!("read -t 1 -r ignored\nprintf '%s' \"$?\" > \"{read_status_display}\"\n"); - fs::write(home.join(".bashrc"), bashrc).await?; - - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let home_display = home.display(); - let script = format!( - "HOME=\"{home_display}\"; export HOME; {}", - bash_snapshot_script() - ); - let output = run_script_with_timeout( - &shell, - &script, - Duration::from_secs(2), - /*use_login_shell*/ true, - home, - ) - .await - .context("run snapshot command")?; - let read_status = fs::read_to_string(&read_status_path) - .await - .context("read stdin probe status")?; - - assert_eq!( - read_status, "1", - "expected shell startup read to see EOF on stdin; status={read_status:?}" - ); - - assert!( - output.contains("# Snapshot file"), - "expected snapshot marker in output; output={output:?}" - ); - - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn timed_out_snapshot_shell_is_terminated() -> Result<()> { - use std::process::Stdio; - use tokio::time::Duration as TokioDuration; - use tokio::time::Instant; - use tokio::time::sleep; - - let dir = tempdir()?; - let pid_path = dir.path().join("pid"); - let script = format!("echo $$ > \"{}\"; sleep 30", pid_path.display()); - - let shell = Shell { - shell_type: ShellType::Sh, - shell_path: PathBuf::from("/bin/sh"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let err = run_script_with_timeout( - &shell, - &script, - Duration::from_secs(1), - /*use_login_shell*/ true, - dir.path(), - ) - .await - .expect_err("snapshot shell should time out"); - assert!( - err.to_string().contains("timed out"), - "expected timeout error, got {err:?}" - ); - - let pid = fs::read_to_string(&pid_path) - .await - .expect("snapshot shell writes its pid before timing out") - .trim() - .parse::()?; - - let deadline = Instant::now() + TokioDuration::from_secs(1); - loop { - let kill_status = StdCommand::new("kill") - .arg("-0") - .arg(pid.to_string()) - .stderr(Stdio::null()) - .stdout(Stdio::null()) - .status()?; - if !kill_status.success() { - break; - } - if Instant::now() >= deadline { - panic!("timed out snapshot shell is still alive after grace period"); - } - sleep(TokioDuration::from_millis(50)).await; - } - - Ok(()) -} - -#[cfg(target_os = "macos")] -#[tokio::test] -async fn macos_zsh_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Zsh).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn linux_bash_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Bash).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn linux_sh_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Sh).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "windows")] -#[ignore] -#[tokio::test] -async fn windows_powershell_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::PowerShell).await?; - assert!(snapshot.contains("# Snapshot file")); - assert!(snapshot.contains("aliases ")); - assert!(snapshot.contains("exports ")); - Ok(()) -} - -async fn write_rollout_stub(codex_home: &Path, session_id: ThreadId) -> Result { - let dir = codex_home - .join("sessions") - .join("2025") - .join("01") - .join("01"); - fs::create_dir_all(&dir).await?; - let path = dir.join(format!("rollout-2025-01-01T00-00-00-{session_id}.jsonl")); - fs::write(&path, "").await?; - Ok(path) -} - #[tokio::test] async fn cleanup_stale_snapshots_removes_orphans_and_keeps_live() -> Result<()> { let dir = tempdir()?; @@ -476,7 +121,7 @@ fn set_file_mtime(path: &Path, age: Duration) -> Result<()> { .saturating_sub(age.as_secs()); let tv_sec = now .try_into() - .map_err(|_| anyhow!("Snapshot mtime is out of range for libc::timespec"))?; + .map_err(|_| anyhow::anyhow!("Snapshot mtime is out of range for libc::timespec"))?; let ts = libc::timespec { tv_sec, tv_nsec: 0 }; let times = [ts, ts]; let c_path = std::ffi::CString::new(path.as_os_str().as_bytes())?; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index a7c384202f..f8e8311e28 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use crate::RolloutRecorder; use crate::SkillsManager; use crate::agent::AgentControl; @@ -22,9 +20,9 @@ use codex_mcp::mcp_connection_manager::McpConnectionManager; use codex_otel::SessionTelemetry; use codex_rollout::state_db::StateDbHandle; use std::path::PathBuf; +use std::sync::Arc; use tokio::sync::Mutex; use tokio::sync::RwLock; -use tokio::sync::watch; use tokio_util::sync::CancellationToken; pub(crate) struct SessionServices { @@ -39,7 +37,6 @@ pub(crate) struct SessionServices { pub(crate) hooks: Hooks, pub(crate) rollout: Mutex>, pub(crate) user_shell: Arc, - pub(crate) shell_snapshot_tx: watch::Sender>>, pub(crate) show_raw_agent_reasoning: bool, pub(crate) exec_policy: Arc, pub(crate) auth_manager: Arc, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index fee47c2147..40bee160c2 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -9,7 +9,6 @@ use crate::exec_env::create_env; use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; -use crate::shell_snapshot::ShellSnapshot; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; @@ -22,42 +21,25 @@ use codex_shell_command::powershell::try_find_powershell_executable_blocking; use codex_shell_command::powershell::try_find_pwsh_executable_blocking; use serde_json::json; use tokio::sync::Mutex; -use tokio::sync::watch; /// The logic for is_known_safe_command() has heuristics for known shells, /// so we must ensure the commands generated by [ShellCommandHandler] can be /// recognized as safe if the `command` is safe. #[test] fn commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_command() { - let bash_shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let bash_shell = Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")); assert_safe(&bash_shell, "ls -la"); - let zsh_shell = Shell { - shell_type: ShellType::Zsh, - shell_path: PathBuf::from("/bin/zsh"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let zsh_shell = Shell::new(ShellType::Zsh, PathBuf::from("/bin/zsh")); 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(), - }; + let powershell = Shell::new(ShellType::PowerShell, path.to_path_buf()); 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(), - }; + let pwsh = Shell::new(ShellType::PowerShell, path.to_path_buf()); assert_safe(&pwsh, "ls -Name"); } } @@ -124,15 +106,7 @@ async fn shell_command_handler_to_exec_params_uses_session_shell_and_turn_contex #[test] fn shell_command_handler_respects_explicit_login_flag() { - let (_tx, shell_snapshot) = watch::channel(Some(Arc::new(ShellSnapshot { - path: PathBuf::from("/tmp/snapshot.sh"), - cwd: PathBuf::from("/tmp"), - }))); - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot, - }; + let shell = Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")); let login_command = ShellCommandHandler::base_command( &shell, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index dfe8c42752..435e41f5e3 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -390,11 +390,10 @@ pub(crate) fn get_command( match shell_mode { UnifiedExecShellMode::Direct => { - let model_shell = args.shell.as_ref().map(|shell_str| { - let mut shell = get_shell_by_model_provided_path(&PathBuf::from(shell_str)); - shell.shell_snapshot = crate::shell::empty_shell_snapshot_receiver(); - shell - }); + let model_shell = args + .shell + .as_ref() + .map(|shell_str| get_shell_by_model_provided_path(&PathBuf::from(shell_str))); let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); Ok(shell.derive_exec_args(&args.cmd, use_login_shell)) } diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 53890a89b7..d15328672b 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -38,7 +38,7 @@ pub(crate) fn build_sandbox_command( /// POSIX-only helper: for commands produced by `Shell::derive_exec_args` /// for Bash/Zsh/sh of the form `[shell_path, "-lc", "