diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 533c28d6f7..ee8960d88f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2098,6 +2098,7 @@ dependencies = [ "async-trait", "base64 0.22.1", "codex-app-server-protocol", + "codex-config", "codex-protocol", "codex-sandboxing", "codex-utils-absolute-path", @@ -2107,6 +2108,7 @@ dependencies = [ "pretty_assertions", "serde", "serde_json", + "serial_test", "tempfile", "test-case", "thiserror 2.0.18", diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 08a51108ae..9e4e603b69 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -1303,6 +1303,11 @@ pub(crate) async fn apply_bespoke_event_handling( .await; } EventMsg::ContextCompacted(..) => { + // Core still fans out this deprecated event for legacy clients; + // v2 clients receive the canonical ContextCompaction item instead. + if matches!(api_version, ApiVersion::V2) { + return; + } let notification = ContextCompactedNotification { thread_id: conversation_id.to_string(), turn_id: event_turn_id.clone(), @@ -1599,6 +1604,17 @@ pub(crate) async fn apply_bespoke_event_handling( .await; } EventMsg::ExecCommandBegin(exec_command_begin_event) => { + if matches!(api_version, ApiVersion::V2) + && matches!( + exec_command_begin_event.source, + codex_protocol::protocol::ExecCommandSource::UnifiedExecInteraction + ) + { + // TerminalInteraction is the v2 surface for unified exec + // stdin/poll events. Suppress the legacy CommandExecution + // item so clients do not render the same wait twice. + return; + } let item_id = exec_command_begin_event.call_id.clone(); let command_actions = exec_command_begin_event .parsed_cmd @@ -1702,6 +1718,17 @@ pub(crate) async fn apply_bespoke_event_handling( .command_execution_started .remove(&call_id); } + if matches!(api_version, ApiVersion::V2) + && matches!( + exec_command_end_event.source, + codex_protocol::protocol::ExecCommandSource::UnifiedExecInteraction + ) + { + // The paired begin event is suppressed above; keep the + // completion out of v2 as well so no orphan legacy item is + // emitted for unified exec interactions. + return; + } let item = build_command_execution_end_item(&exec_command_end_event); diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 1b178fcdae..ffed561984 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -14,6 +14,7 @@ pub mod profile_toml; mod project_root_markers; mod requirements_exec_policy; pub mod schema; +pub mod shell_environment; mod skills_config; mod state; pub mod types; diff --git a/codex-rs/config/src/shell_environment.rs b/codex-rs/config/src/shell_environment.rs new file mode 100644 index 0000000000..80fe0da426 --- /dev/null +++ b/codex-rs/config/src/shell_environment.rs @@ -0,0 +1,123 @@ +use crate::types::EnvironmentVariablePattern; +use crate::types::ShellEnvironmentPolicy; +use crate::types::ShellEnvironmentPolicyInherit; +use std::collections::HashMap; +use std::collections::HashSet; + +pub const CODEX_THREAD_ID_ENV_VAR: &str = "CODEX_THREAD_ID"; + +/// Construct a shell environment from the supplied process environment and +/// shell-environment policy. +pub fn create_env( + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap { + create_env_from_vars(std::env::vars(), policy, thread_id) +} + +pub fn create_env_from_vars( + vars: I, + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap +where + I: IntoIterator, +{ + let mut env_map = populate_env(vars, policy, thread_id); + + if cfg!(target_os = "windows") { + // This is a workaround to address the failures we are seeing in the + // following tests when run via Bazel on Windows: + // + // ``` + // suite::shell_command::unicode_output::with_login + // suite::shell_command::unicode_output::without_login + // ``` + // + // Currently, we can only reproduce these failures in CI, which makes + // iteration times long, so we include this quick fix for now to unblock + // getting the Windows Bazel build running. + if !env_map.keys().any(|k| k.eq_ignore_ascii_case("PATHEXT")) { + env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); + } + } + env_map +} + +pub fn populate_env( + vars: I, + policy: &ShellEnvironmentPolicy, + thread_id: Option<&str>, +) -> HashMap +where + I: IntoIterator, +{ + // Step 1 - determine the starting set of variables based on the + // `inherit` strategy. + let mut env_map: HashMap = match policy.inherit { + ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), + ShellEnvironmentPolicyInherit::None => HashMap::new(), + ShellEnvironmentPolicyInherit::Core => { + let core_vars: HashSet<&str> = COMMON_CORE_VARS + .iter() + .copied() + .chain(PLATFORM_CORE_VARS.iter().copied()) + .collect(); + let is_core_var = |name: &str| { + if cfg!(target_os = "windows") { + core_vars + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(name)) + } else { + core_vars.contains(name) + } + }; + vars.into_iter().filter(|(k, _)| is_core_var(k)).collect() + } + }; + + // Internal helper - does `name` match any pattern in `patterns`? + let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool { + patterns.iter().any(|pattern| pattern.matches(name)) + }; + + // Step 2 - Apply the default exclude if not disabled. + if !policy.ignore_default_excludes { + let default_excludes = vec![ + EnvironmentVariablePattern::new_case_insensitive("*KEY*"), + EnvironmentVariablePattern::new_case_insensitive("*SECRET*"), + EnvironmentVariablePattern::new_case_insensitive("*TOKEN*"), + ]; + env_map.retain(|k, _| !matches_any(k, &default_excludes)); + } + + // Step 3 - Apply custom excludes. + if !policy.exclude.is_empty() { + env_map.retain(|k, _| !matches_any(k, &policy.exclude)); + } + + // Step 4 - Apply user-provided overrides. + for (key, val) in &policy.r#set { + env_map.insert(key.clone(), val.clone()); + } + + // Step 5 - If include_only is non-empty, keep only the matching vars. + if !policy.include_only.is_empty() { + env_map.retain(|k, _| matches_any(k, &policy.include_only)); + } + + // Step 6 - Populate the thread ID environment variable when provided. + if let Some(thread_id) = thread_id { + env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string()); + } + + env_map +} + +const COMMON_CORE_VARS: &[&str] = &["PATH", "SHELL", "TMPDIR", "TEMP", "TMP"]; + +#[cfg(target_os = "windows")] +const PLATFORM_CORE_VARS: &[&str] = &["PATHEXT", "USERNAME", "USERPROFILE"]; + +#[cfg(unix)] +const PLATFORM_CORE_VARS: &[&str] = &["HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME", "USER"]; diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index a1880c3be1..64cfe85d08 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -658,7 +658,7 @@ impl From for codex_app_server_protocol::SandboxSettings } } -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default, JsonSchema)] #[serde(rename_all = "kebab-case")] pub enum ShellEnvironmentPolicyInherit { /// "Core" environment variables for the platform. On UNIX, this would diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index a3a674cb60..7311df2808 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -6838,12 +6838,6 @@ async fn run_sampling_request( let base_instructions = sess.get_base_instructions().await; - let prompt = build_prompt( - input, - router.as_ref(), - turn_context.as_ref(), - base_instructions, - ); let tool_runtime = ToolCallRuntime::new( Arc::clone(&router), Arc::clone(&sess), @@ -6861,7 +6855,21 @@ async fn run_sampling_request( ) .await; let mut retries = 0; + let mut initial_input = Some(input); loop { + let prompt_input = if let Some(input) = initial_input.take() { + input + } else { + sess.clone_history() + .await + .for_prompt(&turn_context.model_info.input_modalities) + }; + let prompt = build_prompt( + prompt_input, + router.as_ref(), + turn_context.as_ref(), + base_instructions.clone(), + ); let err = match try_run_sampling_request( tool_runtime.clone(), Arc::clone(&sess), @@ -7668,7 +7676,8 @@ async fn try_run_sampling_request( }; let event = match event { - Some(res) => res?, + Some(Ok(event)) => event, + Some(Err(err)) => break Err(err), None => { break Err(CodexErr::Stream( "stream closed before response.completed".into(), @@ -7739,9 +7748,14 @@ async fn try_run_sampling_request( | ResponseItem::Other => false, }; - let output_result = handle_output_item_done(&mut ctx, item, previously_active_item) - .instrument(handle_responses) - .await?; + let output_result = + match handle_output_item_done(&mut ctx, item, previously_active_item) + .instrument(handle_responses) + .await + { + Ok(output_result) => output_result, + Err(err) => break Err(err), + }; if let Some(tool_future) = output_result.tool_future { in_flight.push_back(tool_future); } diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index a205b2041c..0ae1f55690 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -1525,7 +1525,7 @@ fn config_defaults_to_file_cli_auth_store_mode() -> std::io::Result<()> { } #[test] -fn config_honors_explicit_keyring_auth_store_mode() -> std::io::Result<()> { +fn config_resolves_explicit_keyring_auth_store_mode() -> std::io::Result<()> { let codex_home = TempDir::new()?; let cfg = ConfigToml { cli_auth_credentials_store: Some(AuthCredentialsStoreMode::Keyring), @@ -1540,14 +1540,17 @@ fn config_honors_explicit_keyring_auth_store_mode() -> std::io::Result<()> { assert_eq!( config.cli_auth_credentials_store_mode, - AuthCredentialsStoreMode::Keyring, + resolve_cli_auth_credentials_store_mode( + AuthCredentialsStoreMode::Keyring, + env!("CARGO_PKG_VERSION"), + ), ); Ok(()) } #[test] -fn config_defaults_to_auto_oauth_store_mode() -> std::io::Result<()> { +fn config_resolves_default_oauth_store_mode() -> std::io::Result<()> { let codex_home = TempDir::new()?; let cfg = ConfigToml::default(); @@ -1559,12 +1562,66 @@ fn config_defaults_to_auto_oauth_store_mode() -> std::io::Result<()> { assert_eq!( config.mcp_oauth_credentials_store_mode, - OAuthCredentialsStoreMode::Auto, + resolve_mcp_oauth_credentials_store_mode( + OAuthCredentialsStoreMode::Auto, + env!("CARGO_PKG_VERSION"), + ), ); Ok(()) } +#[test] +fn local_dev_builds_force_file_cli_auth_store_modes() { + assert_eq!( + resolve_cli_auth_credentials_store_mode( + AuthCredentialsStoreMode::Keyring, + LOCAL_DEV_BUILD_VERSION, + ), + AuthCredentialsStoreMode::File, + ); + assert_eq!( + resolve_cli_auth_credentials_store_mode( + AuthCredentialsStoreMode::Auto, + LOCAL_DEV_BUILD_VERSION, + ), + AuthCredentialsStoreMode::File, + ); + assert_eq!( + resolve_cli_auth_credentials_store_mode( + AuthCredentialsStoreMode::Ephemeral, + LOCAL_DEV_BUILD_VERSION, + ), + AuthCredentialsStoreMode::Ephemeral, + ); + assert_eq!( + resolve_cli_auth_credentials_store_mode(AuthCredentialsStoreMode::Keyring, "1.2.3"), + AuthCredentialsStoreMode::Keyring, + ); +} + +#[test] +fn local_dev_builds_force_file_mcp_oauth_store_modes() { + assert_eq!( + resolve_mcp_oauth_credentials_store_mode( + OAuthCredentialsStoreMode::Keyring, + LOCAL_DEV_BUILD_VERSION, + ), + OAuthCredentialsStoreMode::File, + ); + assert_eq!( + resolve_mcp_oauth_credentials_store_mode( + OAuthCredentialsStoreMode::Auto, + LOCAL_DEV_BUILD_VERSION, + ), + OAuthCredentialsStoreMode::File, + ); + assert_eq!( + resolve_mcp_oauth_credentials_store_mode(OAuthCredentialsStoreMode::Keyring, "1.2.3"), + OAuthCredentialsStoreMode::Keyring, + ); +} + #[test] fn feedback_enabled_defaults_to_true() -> std::io::Result<()> { let codex_home = TempDir::new()?; @@ -1922,7 +1979,10 @@ async fn managed_config_overrides_oauth_store_mode() -> anyhow::Result<()> { )?; assert_eq!( final_config.mcp_oauth_credentials_store_mode, - OAuthCredentialsStoreMode::Keyring, + resolve_mcp_oauth_credentials_store_mode( + OAuthCredentialsStoreMode::Keyring, + env!("CARGO_PKG_VERSION"), + ), ); Ok(()) @@ -4514,7 +4574,10 @@ fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> { cwd: fixture.cwd(), cli_auth_credentials_store_mode: Default::default(), mcp_servers: Constrained::allow_any(HashMap::new()), - mcp_oauth_credentials_store_mode: Default::default(), + mcp_oauth_credentials_store_mode: resolve_mcp_oauth_credentials_store_mode( + Default::default(), + LOCAL_DEV_BUILD_VERSION, + ), mcp_oauth_callback_port: None, mcp_oauth_callback_url: None, model_providers: fixture.model_provider_map.clone(), @@ -4660,7 +4723,10 @@ fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> { cwd: fixture.cwd(), cli_auth_credentials_store_mode: Default::default(), mcp_servers: Constrained::allow_any(HashMap::new()), - mcp_oauth_credentials_store_mode: Default::default(), + mcp_oauth_credentials_store_mode: resolve_mcp_oauth_credentials_store_mode( + Default::default(), + LOCAL_DEV_BUILD_VERSION, + ), mcp_oauth_callback_port: None, mcp_oauth_callback_url: None, model_providers: fixture.model_provider_map.clone(), @@ -4804,7 +4870,10 @@ fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> { cwd: fixture.cwd(), cli_auth_credentials_store_mode: Default::default(), mcp_servers: Constrained::allow_any(HashMap::new()), - mcp_oauth_credentials_store_mode: Default::default(), + mcp_oauth_credentials_store_mode: resolve_mcp_oauth_credentials_store_mode( + Default::default(), + LOCAL_DEV_BUILD_VERSION, + ), mcp_oauth_callback_port: None, mcp_oauth_callback_url: None, model_providers: fixture.model_provider_map.clone(), @@ -4934,7 +5003,10 @@ fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> { cwd: fixture.cwd(), cli_auth_credentials_store_mode: Default::default(), mcp_servers: Constrained::allow_any(HashMap::new()), - mcp_oauth_credentials_store_mode: Default::default(), + mcp_oauth_credentials_store_mode: resolve_mcp_oauth_credentials_store_mode( + Default::default(), + LOCAL_DEV_BUILD_VERSION, + ), mcp_oauth_callback_port: None, mcp_oauth_callback_url: None, model_providers: fixture.model_provider_map.clone(), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 9b2b06e0a8..d6d08c4d90 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -124,6 +124,7 @@ pub(crate) const PROJECT_DOC_MAX_BYTES: usize = 32 * 1024; // 32 KiB pub(crate) const DEFAULT_AGENT_MAX_THREADS: Option = Some(6); pub(crate) const DEFAULT_AGENT_MAX_DEPTH: i32 = 1; pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option = None; +const LOCAL_DEV_BUILD_VERSION: &str = "0.0.0"; pub const CONFIG_TOML_FILE: &str = "config.toml"; @@ -141,6 +142,32 @@ fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option { } } +fn resolve_cli_auth_credentials_store_mode( + configured: AuthCredentialsStoreMode, + package_version: &str, +) -> AuthCredentialsStoreMode { + match (package_version, configured) { + ( + LOCAL_DEV_BUILD_VERSION, + AuthCredentialsStoreMode::Keyring | AuthCredentialsStoreMode::Auto, + ) => AuthCredentialsStoreMode::File, + (_, mode) => mode, + } +} + +fn resolve_mcp_oauth_credentials_store_mode( + configured: OAuthCredentialsStoreMode, + package_version: &str, +) -> OAuthCredentialsStoreMode { + match (package_version, configured) { + ( + LOCAL_DEV_BUILD_VERSION, + OAuthCredentialsStoreMode::Keyring | OAuthCredentialsStoreMode::Auto, + ) => OAuthCredentialsStoreMode::File, + (_, mode) => mode, + } +} + #[cfg(test)] pub(crate) fn test_config() -> Config { let codex_home = tempfile::tempdir().expect("create temp dir"); @@ -2014,11 +2041,17 @@ impl Config { include_environment_context, // The config.toml omits "_mode" because it's a config file. However, "_mode" // is important in code to differentiate the mode from the store implementation. - cli_auth_credentials_store_mode: cfg.cli_auth_credentials_store.unwrap_or_default(), + cli_auth_credentials_store_mode: resolve_cli_auth_credentials_store_mode( + cfg.cli_auth_credentials_store.unwrap_or_default(), + env!("CARGO_PKG_VERSION"), + ), mcp_servers, // The config.toml omits "_mode" because it's a config file. However, "_mode" // is important in code to differentiate the mode from the store implementation. - mcp_oauth_credentials_store_mode: cfg.mcp_oauth_credentials_store.unwrap_or_default(), + mcp_oauth_credentials_store_mode: resolve_mcp_oauth_credentials_store_mode( + cfg.mcp_oauth_credentials_store.unwrap_or_default(), + env!("CARGO_PKG_VERSION"), + ), mcp_oauth_callback_port: cfg.mcp_oauth_callback_port, mcp_oauth_callback_url: cfg.mcp_oauth_callback_url.clone(), model_providers, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 75033b13eb..1dcd897f09 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -349,6 +349,7 @@ pub(crate) async fn execute_exec_request( command, cwd, env, + exec_server_env_config: _, network, expiration, capture_policy, diff --git a/codex-rs/core/src/exec_env.rs b/codex-rs/core/src/exec_env.rs index a50fcc2538..ad94bc51a0 100644 --- a/codex-rs/core/src/exec_env.rs +++ b/codex-rs/core/src/exec_env.rs @@ -1,11 +1,10 @@ +#[cfg(test)] use codex_config::types::EnvironmentVariablePattern; use codex_config::types::ShellEnvironmentPolicy; -use codex_config::types::ShellEnvironmentPolicyInherit; use codex_protocol::ThreadId; use std::collections::HashMap; -use std::collections::HashSet; -pub const CODEX_THREAD_ID_ENV_VAR: &str = "CODEX_THREAD_ID"; +pub use codex_config::shell_environment::CODEX_THREAD_ID_ENV_VAR; /// Construct an environment map based on the rules in the specified policy. The /// resulting map can be passed directly to `Command::envs()` after calling @@ -21,9 +20,11 @@ pub fn create_env( policy: &ShellEnvironmentPolicy, thread_id: Option, ) -> HashMap { - create_env_from_vars(std::env::vars(), policy, thread_id) + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::create_env(policy, thread_id.as_deref()) } +#[cfg(all(test, target_os = "windows"))] fn create_env_from_vars( vars: I, policy: &ShellEnvironmentPolicy, @@ -32,35 +33,11 @@ fn create_env_from_vars( where I: IntoIterator, { - let mut env_map = populate_env(vars, policy, thread_id); - - if cfg!(target_os = "windows") { - // This is a workaround to address the failures we are seeing in the - // following tests when run via Bazel on Windows: - // - // ``` - // suite::shell_command::unicode_output::with_login - // suite::shell_command::unicode_output::without_login - // ``` - // - // Currently, we can only reproduce these failures in CI, which makes - // iteration times long, so we include this quick fix for now to unblock - // getting the Windows Bazel build running. - if !env_map.keys().any(|k| k.eq_ignore_ascii_case("PATHEXT")) { - env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); - } - } - env_map + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::create_env_from_vars(vars, policy, thread_id.as_deref()) } -const COMMON_CORE_VARS: &[&str] = &["PATH", "SHELL", "TMPDIR", "TEMP", "TMP"]; - -#[cfg(target_os = "windows")] -const PLATFORM_CORE_VARS: &[&str] = &["PATHEXT", "USERNAME", "USERPROFILE"]; - -#[cfg(unix)] -const PLATFORM_CORE_VARS: &[&str] = &["HOME", "LANG", "LC_ALL", "LC_CTYPE", "LOGNAME", "USER"]; - +#[cfg(test)] fn populate_env( vars: I, policy: &ShellEnvironmentPolicy, @@ -69,66 +46,8 @@ fn populate_env( where I: IntoIterator, { - // Step 1 – determine the starting set of variables based on the - // `inherit` strategy. - let mut env_map: HashMap = match policy.inherit { - ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), - ShellEnvironmentPolicyInherit::None => HashMap::new(), - ShellEnvironmentPolicyInherit::Core => { - let core_vars: HashSet<&str> = COMMON_CORE_VARS - .iter() - .copied() - .chain(PLATFORM_CORE_VARS.iter().copied()) - .collect(); - let is_core_var = |name: &str| { - if cfg!(target_os = "windows") { - core_vars - .iter() - .any(|allowed| allowed.eq_ignore_ascii_case(name)) - } else { - core_vars.contains(name) - } - }; - vars.into_iter().filter(|(k, _)| is_core_var(k)).collect() - } - }; - - // Internal helper – does `name` match **any** pattern in `patterns`? - let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool { - patterns.iter().any(|pattern| pattern.matches(name)) - }; - - // Step 2 – Apply the default exclude if not disabled. - if !policy.ignore_default_excludes { - let default_excludes = vec![ - EnvironmentVariablePattern::new_case_insensitive("*KEY*"), - EnvironmentVariablePattern::new_case_insensitive("*SECRET*"), - EnvironmentVariablePattern::new_case_insensitive("*TOKEN*"), - ]; - env_map.retain(|k, _| !matches_any(k, &default_excludes)); - } - - // Step 3 – Apply custom excludes. - if !policy.exclude.is_empty() { - env_map.retain(|k, _| !matches_any(k, &policy.exclude)); - } - - // Step 4 – Apply user-provided overrides. - for (key, val) in &policy.r#set { - env_map.insert(key.clone(), val.clone()); - } - - // Step 5 – If include_only is non-empty, keep *only* the matching vars. - if !policy.include_only.is_empty() { - env_map.retain(|k, _| matches_any(k, &policy.include_only)); - } - - // Step 6 – Populate the thread ID environment variable when provided. - if let Some(thread_id) = thread_id { - env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string()); - } - - env_map + let thread_id = thread_id.map(|thread_id| thread_id.to_string()); + codex_config::shell_environment::populate_env(vars, policy, thread_id.as_deref()) } #[cfg(test)] diff --git a/codex-rs/core/src/memories/mod.rs b/codex-rs/core/src/memories/mod.rs index 194c27c907..ecb6c05df2 100644 --- a/codex-rs/core/src/memories/mod.rs +++ b/codex-rs/core/src/memories/mod.rs @@ -65,7 +65,7 @@ mod phase_one { /// Phase 2 (aka `Consolidation`). mod phase_two { /// Default model used for phase 2. - pub(super) const MODEL: &str = "gpt-5.3-codex"; + pub(super) const MODEL: &str = "gpt-5.4"; /// Default reasoning effort used for phase 2. pub(super) const REASONING_EFFORT: super::ReasoningEffort = super::ReasoningEffort::Medium; /// Lease duration (seconds) for phase-2 consolidation job ownership. diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index 2377dbcaf4..d4e83a6637 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -33,11 +33,18 @@ pub(crate) struct ExecOptions { pub(crate) capture_policy: ExecCapturePolicy, } +#[derive(Clone, Debug)] +pub(crate) struct ExecServerEnvConfig { + pub(crate) policy: codex_exec_server::ExecEnvPolicy, + pub(crate) local_policy_env: HashMap, +} + #[derive(Debug)] pub struct ExecRequest { pub command: Vec, pub cwd: AbsolutePathBuf, pub env: HashMap, + pub(crate) exec_server_env_config: Option, pub network: Option, pub expiration: ExecExpiration, pub capture_policy: ExecCapturePolicy, @@ -72,6 +79,7 @@ impl ExecRequest { command, cwd, env, + exec_server_env_config: None, network, expiration, capture_policy, @@ -121,6 +129,7 @@ impl ExecRequest { command, cwd, env, + exec_server_env_config: None, network, expiration, capture_policy, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index e0a7703879..f909175871 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -162,6 +162,7 @@ pub(crate) async fn execute_user_shell_command( command: exec_command.clone(), cwd: cwd.clone(), env: exec_env_map, + exec_server_env_config: None, network: turn_context.network.clone(), // TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we // should use that instead of an "arbitrarily large" timeout here. diff --git a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs index 8e4bfb5b59..523b1ed357 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs @@ -2,11 +2,10 @@ use super::*; use crate::agent::control::SpawnAgentForkMode; use crate::agent::control::SpawnAgentOptions; use crate::agent::control::render_input_preview; -use crate::agent::role::DEFAULT_ROLE_NAME; -use crate::agent::role::apply_role_to_config; - use crate::agent::exceeds_thread_spawn_depth_limit; use crate::agent::next_thread_spawn_depth; +use crate::agent::role::DEFAULT_ROLE_NAME; +use crate::agent::role::apply_role_to_config; pub(crate) struct Handler; @@ -61,17 +60,25 @@ impl ToolHandler for Handler { .await; let mut config = build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; - apply_requested_spawn_agent_model_overrides( - &session, - turn.as_ref(), - &mut config, - args.model.as_deref(), - args.reasoning_effort, - ) - .await?; - apply_role_to_config(&mut config, role_name) - .await - .map_err(FunctionCallError::RespondToModel)?; + if args.fork_context { + reject_full_fork_spawn_overrides( + role_name, + args.model.as_deref(), + args.reasoning_effort, + )?; + } else { + apply_requested_spawn_agent_model_overrides( + &session, + turn.as_ref(), + &mut config, + args.model.as_deref(), + args.reasoning_effort, + ) + .await?; + apply_role_to_config(&mut config, role_name) + .await + .map_err(FunctionCallError::RespondToModel)?; + } apply_spawn_agent_runtime_overrides(&mut config, turn.as_ref())?; apply_spawn_agent_overrides(&mut config, child_depth); diff --git a/codex-rs/core/src/tools/handlers/multi_agents_common.rs b/codex-rs/core/src/tools/handlers/multi_agents_common.rs index 2078c229b9..9c2740d48a 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_common.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_common.rs @@ -225,7 +225,9 @@ fn build_agent_shared_config(turn: &TurnContext) -> Result Result, + model: Option<&str>, + reasoning_effort: Option, +) -> Result<(), FunctionCallError> { + if agent_type.is_some() || model.is_some() || reasoning_effort.is_some() { + return Err(FunctionCallError::RespondToModel( + "Full-history forked agents inherit the parent agent type, model, and reasoning effort; omit agent_type, model, and reasoning_effort, or spawn without fork_context/fork_turns=all.".to_string(), + )); + } + Ok(()) +} + /// Copies runtime-only turn state onto a child config before it is handed to `AgentControl`. /// /// These values are chosen by the live turn rather than persisted config, so leaving them stale diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index 733d998536..9eb0679bfb 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -2,6 +2,7 @@ use super::*; use crate::CodexThread; use crate::ThreadManager; use crate::codex::make_session_and_context; +use crate::config::AgentRoleConfig; use crate::config::DEFAULT_AGENT_MAX_DEPTH; use crate::function_tool::FunctionCallError; use crate::session_prefix::format_subagent_notification_message; @@ -28,6 +29,7 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputBody; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; +use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::AgentStatus; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -89,6 +91,36 @@ fn thread_manager() -> ThreadManager { ) } +async fn install_role_with_model_override(turn: &mut TurnContext) -> String { + let role_name = "fork-context-role".to_string(); + tokio::fs::create_dir_all(&turn.config.codex_home) + .await + .expect("codex home should be created"); + let role_config_path = turn.config.codex_home.join("fork-context-role.toml"); + tokio::fs::write( + &role_config_path, + r#"model = "gpt-5-role-override" +model_provider = "ollama" +model_reasoning_effort = "minimal" +"#, + ) + .await + .expect("role config should be written"); + + let mut config = (*turn.config).clone(); + config.agent_roles.insert( + role_name.clone(), + AgentRoleConfig { + description: Some("Role with model overrides".to_string()), + config_file: Some(role_config_path), + nickname_candidates: None, + }, + ); + turn.config = Arc::new(config); + + role_name +} + fn history_contains_inter_agent_communication( history_items: &[ResponseItem], expected: &InterAgentCommunication, @@ -365,6 +397,215 @@ async fn spawn_agent_uses_explorer_role_and_preserves_approval_policy() { assert_eq!(snapshot.model_provider_id, "ollama"); } +#[tokio::test] +async fn spawn_agent_fork_context_rejects_agent_type_override() { + let (mut session, mut turn) = make_session_and_context().await; + let role_name = install_role_with_model_override(&mut turn).await; + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + let err = SpawnAgentHandler + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "agent_type": role_name, + "fork_context": true + })), + )) + .await + .expect_err("fork_context should reject agent_type overrides"); + + assert_eq!( + err, + FunctionCallError::RespondToModel( + "Full-history forked agents inherit the parent agent type, model, and reasoning effort; omit agent_type, model, and reasoning_effort, or spawn without fork_context/fork_turns=all.".to_string(), + ) + ); +} + +#[tokio::test] +async fn spawn_agent_fork_context_rejects_child_model_overrides() { + let (mut session, turn) = make_session_and_context().await; + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + + let err = SpawnAgentHandler + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "model": "gpt-5-child-override", + "reasoning_effort": "low", + "fork_context": true + })), + )) + .await + .expect_err("forked spawn should reject child model overrides"); + + assert_eq!( + err, + FunctionCallError::RespondToModel( + "Full-history forked agents inherit the parent agent type, model, and reasoning effort; omit agent_type, model, and reasoning_effort, or spawn without fork_context/fork_turns=all.".to_string(), + ) + ); +} + +#[tokio::test] +async fn multi_agent_v2_spawn_fork_turns_all_rejects_agent_type_override() { + let (mut session, mut turn) = make_session_and_context().await; + let role_name = install_role_with_model_override(&mut turn).await; + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + let mut config = (*turn.config).clone(); + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + let turn = TurnContext { + config: Arc::new(config), + ..turn + }; + + let err = SpawnAgentHandlerV2 + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "task_name": "fork_context_v2", + "agent_type": role_name, + "fork_turns": "all" + })), + )) + .await + .expect_err("fork_turns=all should reject agent_type overrides"); + + assert_eq!( + err, + FunctionCallError::RespondToModel( + "Full-history forked agents inherit the parent agent type, model, and reasoning effort; omit agent_type, model, and reasoning_effort, or spawn without fork_context/fork_turns=all.".to_string(), + ) + ); +} + +#[tokio::test] +async fn multi_agent_v2_spawn_fork_turns_rejects_child_model_overrides() { + let (mut session, mut turn) = make_session_and_context().await; + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + let mut config = (*turn.config).clone(); + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + turn.config = Arc::new(config); + + let err = SpawnAgentHandlerV2 + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "task_name": "fork_context_v2", + "model": "gpt-5-child-override", + "reasoning_effort": "low", + "fork_turns": "all" + })), + )) + .await + .expect_err("forked spawn should reject child model overrides"); + + assert_eq!( + err, + FunctionCallError::RespondToModel( + "Full-history forked agents inherit the parent agent type, model, and reasoning effort; omit agent_type, model, and reasoning_effort, or spawn without fork_context/fork_turns=all.".to_string(), + ) + ); +} + +#[tokio::test] +async fn multi_agent_v2_spawn_partial_fork_turns_allows_agent_type_override() { + let (mut session, mut turn) = make_session_and_context().await; + let role_name = install_role_with_model_override(&mut turn).await; + let manager = thread_manager(); + let root = manager + .start_thread((*turn.config).clone()) + .await + .expect("root thread should start"); + session.services.agent_control = manager.agent_control(); + session.conversation_id = root.thread_id; + let mut config = (*turn.config).clone(); + config + .features + .enable(Feature::MultiAgentV2) + .expect("test config should allow feature update"); + let turn = TurnContext { + config: Arc::new(config), + ..turn + }; + + let output = SpawnAgentHandlerV2 + .handle(invocation( + Arc::new(session), + Arc::new(turn), + "spawn_agent", + function_payload(json!({ + "message": "inspect this repo", + "task_name": "partial_fork", + "agent_type": role_name, + "fork_turns": "1" + })), + )) + .await + .expect("partial fork should allow agent_type overrides"); + let (content, _) = expect_text_output(output); + let result: serde_json::Value = + serde_json::from_str(&content).expect("spawn_agent result should be json"); + assert_eq!(result["task_name"], "/root/partial_fork"); + let agent_id = manager + .captured_ops() + .into_iter() + .map(|(thread_id, _)| thread_id) + .find(|thread_id| *thread_id != root.thread_id) + .expect("spawned agent should receive an op"); + let snapshot = manager + .get_thread(agent_id) + .await + .expect("spawned agent thread should exist") + .config_snapshot() + .await; + + assert_eq!(snapshot.model, "gpt-5-role-override"); + assert_eq!(snapshot.model_provider_id, "ollama"); + assert_eq!(snapshot.reasoning_effort, Some(ReasoningEffort::Minimal)); +} + #[tokio::test] async fn spawn_agent_returns_agent_id_without_task_name() { let (mut session, turn) = make_session_and_context().await; diff --git a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs index 3c475e7906..03287b20a8 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_v2/spawn.rs @@ -70,17 +70,25 @@ impl ToolHandler for Handler { .await; let mut config = build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?; - apply_requested_spawn_agent_model_overrides( - &session, - turn.as_ref(), - &mut config, - args.model.as_deref(), - args.reasoning_effort, - ) - .await?; - apply_role_to_config(&mut config, role_name) - .await - .map_err(FunctionCallError::RespondToModel)?; + if matches!(fork_mode, Some(SpawnAgentForkMode::FullHistory)) { + reject_full_fork_spawn_overrides( + role_name, + args.model.as_deref(), + args.reasoning_effort, + )?; + } else { + apply_requested_spawn_agent_model_overrides( + &session, + turn.as_ref(), + &mut config, + args.model.as_deref(), + args.reasoning_effort, + ) + .await?; + apply_role_to_config(&mut config, role_name) + .await + .map_err(FunctionCallError::RespondToModel)?; + } apply_spawn_agent_runtime_overrides(&mut config, turn.as_ref())?; apply_spawn_agent_overrides(&mut config, child_depth); config.developer_instructions = Some( 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 07c78d2e79..0bb4263d26 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -126,6 +126,7 @@ pub(super) async fn try_run_zsh_fork( command, cwd: sandbox_cwd, env: sandbox_env, + exec_server_env_config: _, network: sandbox_network, expiration: _sandbox_expiration, capture_policy: _capture_policy, @@ -734,6 +735,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { command: self.command.clone(), cwd: self.cwd.clone(), env: exec_env, + exec_server_env_config: None, network: self.network.clone(), expiration: ExecExpiration::Cancellation(cancel_rx), capture_policy: ExecCapturePolicy::ShellTool, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 94d370fbcd..cb0f328769 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -10,6 +10,7 @@ use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; +use crate::sandboxing::ExecServerEnvConfig; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::tools::network_approval::NetworkApprovalMode; @@ -52,6 +53,7 @@ pub struct UnifiedExecRequest { pub process_id: i32, pub cwd: AbsolutePathBuf, pub env: HashMap, + pub exec_server_env_config: Option, pub explicit_env_overrides: HashMap, pub network: Option, pub tty: bool, @@ -237,9 +239,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; - let exec_env = attempt + let mut exec_env = attempt .env_for(command, options, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; + exec_env.exec_server_env_config = req.exec_server_env_config.clone(); match zsh_fork_backend::maybe_prepare_unified_exec( req, attempt, @@ -294,9 +297,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt expiration: ExecExpiration::DefaultTimeout, capture_policy: ExecCapturePolicy::ShellTool, }; - let exec_env = attempt + let mut exec_env = attempt .env_for(command, options, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; + exec_env.exec_server_env_config = req.exec_server_env_config.clone(); let Some(environment) = ctx.turn.environment.as_ref() else { return Err(ToolError::Rejected( "exec_command is unavailable in this session".to_string(), diff --git a/codex-rs/core/src/unified_exec/process.rs b/codex-rs/core/src/unified_exec/process.rs index 007214c254..62a60b0a8f 100644 --- a/codex-rs/core/src/unified_exec/process.rs +++ b/codex-rs/core/src/unified_exec/process.rs @@ -66,10 +66,11 @@ pub(crate) struct OutputHandles { /// Transport-specific process handle used by unified exec. enum ProcessHandle { Local(Box), - Remote(Arc), + ExecServer(Arc), } -/// Unified wrapper over local PTY sessions and exec-server-backed processes. +/// Unified wrapper over directly spawned PTY sessions and exec-server-backed +/// processes. pub(crate) struct UnifiedExecProcess { process_handle: ProcessHandle, output_tx: broadcast::Sender>, @@ -135,7 +136,7 @@ impl UnifiedExecProcess { .send(data.to_vec()) .await .map_err(|_| UnifiedExecError::WriteToStdin), - ProcessHandle::Remote(process_handle) => { + ProcessHandle::ExecServer(process_handle) => { match process_handle.write(data.to_vec()).await { Ok(response) => match response.status { WriteStatus::Accepted => Ok(()), @@ -179,7 +180,7 @@ impl UnifiedExecProcess { let state = self.state_rx.borrow().clone(); match &self.process_handle { ProcessHandle::Local(process_handle) => state.has_exited || process_handle.has_exited(), - ProcessHandle::Remote(_) => state.has_exited, + ProcessHandle::ExecServer(_) => state.has_exited, } } @@ -189,7 +190,7 @@ impl UnifiedExecProcess { ProcessHandle::Local(process_handle) => { state.exit_code.or_else(|| process_handle.exit_code()) } - ProcessHandle::Remote(_) => state.exit_code, + ProcessHandle::ExecServer(_) => state.exit_code, } } @@ -198,7 +199,7 @@ impl UnifiedExecProcess { self.output_closed_notify.notify_waiters(); match &self.process_handle { ProcessHandle::Local(process_handle) => process_handle.terminate(), - ProcessHandle::Remote(process_handle) => { + ProcessHandle::ExecServer(process_handle) => { let process_handle = Arc::clone(process_handle); tokio::spawn(async move { let _ = process_handle.terminate().await; @@ -331,14 +332,14 @@ impl UnifiedExecProcess { Ok(managed) } - pub(super) async fn from_remote_started( + pub(super) async fn from_exec_server_started( started: StartedExecProcess, sandbox_type: SandboxType, ) -> Result { - let process_handle = ProcessHandle::Remote(Arc::clone(&started.process)); + let process_handle = ProcessHandle::ExecServer(Arc::clone(&started.process)); let mut managed = Self::new(process_handle, sandbox_type, /*spawn_lifecycle*/ None); let output_handles = managed.output_handles(); - managed.output_task = Some(Self::spawn_remote_output_task( + managed.output_task = Some(Self::spawn_exec_server_output_task( started, output_handles, managed.output_tx.clone(), @@ -366,7 +367,7 @@ impl UnifiedExecProcess { Ok(managed) } - fn spawn_remote_output_task( + fn spawn_exec_server_output_task( started: StartedExecProcess, output_handles: OutputHandles, output_tx: broadcast::Sender>, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 9d77ac4e97..20cc7c5f7f 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -11,9 +11,11 @@ use tokio::time::Duration; use tokio::time::Instant; use tokio_util::sync::CancellationToken; +use crate::exec_env::CODEX_THREAD_ID_ENV_VAR; use crate::exec_env::create_env; use crate::exec_policy::ExecApprovalRequest; use crate::sandboxing::ExecRequest; +use crate::sandboxing::ExecServerEnvConfig; use crate::tools::context::ExecCommandToolOutput; use crate::tools::events::ToolEmitter; use crate::tools::events::ToolEventCtx; @@ -47,6 +49,7 @@ use crate::unified_exec::process::OutputBuffer; use crate::unified_exec::process::OutputHandles; use crate::unified_exec::process::SpawnLifecycleHandle; use crate::unified_exec::process::UnifiedExecProcess; +use codex_config::types::ShellEnvironmentPolicy; use codex_protocol::protocol::ExecCommandSource; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::approx_token_count; @@ -89,6 +92,70 @@ fn apply_unified_exec_env(mut env: HashMap) -> HashMap codex_exec_server::ExecEnvPolicy { + codex_exec_server::ExecEnvPolicy { + inherit: policy.inherit.clone(), + ignore_default_excludes: policy.ignore_default_excludes, + exclude: policy + .exclude + .iter() + .map(std::string::ToString::to_string) + .collect(), + r#set: policy.r#set.clone(), + include_only: policy + .include_only + .iter() + .map(std::string::ToString::to_string) + .collect(), + } +} + +fn env_overlay_for_exec_server( + request_env: &HashMap, + local_policy_env: &HashMap, +) -> HashMap { + request_env + .iter() + .filter(|(key, value)| local_policy_env.get(*key) != Some(*value)) + .map(|(key, value)| (key.clone(), value.clone())) + .collect() +} + +fn exec_server_env_for_request( + request: &ExecRequest, +) -> ( + Option, + HashMap, +) { + if let Some(exec_server_env_config) = &request.exec_server_env_config { + ( + Some(exec_server_env_config.policy.clone()), + env_overlay_for_exec_server(&request.env, &exec_server_env_config.local_policy_env), + ) + } else { + (None, request.env.clone()) + } +} + +fn exec_server_params_for_request( + process_id: i32, + request: &ExecRequest, + tty: bool, +) -> codex_exec_server::ExecParams { + let (env_policy, env) = exec_server_env_for_request(request); + codex_exec_server::ExecParams { + process_id: exec_server_process_id(process_id).into(), + argv: request.command.clone(), + cwd: request.cwd.to_path_buf(), + env_policy, + env, + tty, + arg0: request.arg0.clone(), + } +} + /// Borrowed process state prepared for a `write_stdin` or poll operation. struct PreparedProcessHandles { process: Arc, @@ -587,12 +654,7 @@ impl UnifiedExecProcessManager { mut spawn_lifecycle: SpawnLifecycleHandle, environment: &codex_exec_server::Environment, ) -> Result { - let (program, args) = request - .command - .split_first() - .ok_or(UnifiedExecError::MissingCommandLine)?; let inherited_fds = spawn_lifecycle.inherited_fds(); - if environment.is_remote() { if !inherited_fds.is_empty() { return Err(UnifiedExecError::create_process( @@ -602,19 +664,17 @@ impl UnifiedExecProcessManager { let started = environment .get_exec_backend() - .start(codex_exec_server::ExecParams { - process_id: exec_server_process_id(process_id).into(), - argv: request.command.clone(), - cwd: request.cwd.to_path_buf(), - env: request.env.clone(), - tty, - arg0: request.arg0.clone(), - }) + .start(exec_server_params_for_request(process_id, request, tty)) .await .map_err(|err| UnifiedExecError::create_process(err.to_string()))?; - return UnifiedExecProcess::from_remote_started(started, request.sandbox).await; + spawn_lifecycle.after_spawn(); + return UnifiedExecProcess::from_exec_server_started(started, request.sandbox).await; } + let (program, args) = request + .command + .split_first() + .ok_or(UnifiedExecError::MissingCommandLine)?; let spawn_result = if tty { codex_utils_pty::pty::spawn_process_with_inherited_fds( program, @@ -649,10 +709,20 @@ impl UnifiedExecProcessManager { cwd: AbsolutePathBuf, context: &UnifiedExecContext, ) -> Result<(UnifiedExecProcess, Option), UnifiedExecError> { - let env = apply_unified_exec_env(create_env( + let local_policy_env = create_env( &context.turn.shell_environment_policy, - Some(context.session.conversation_id), - )); + /*thread_id*/ None, + ); + let mut env = local_policy_env.clone(); + env.insert( + CODEX_THREAD_ID_ENV_VAR.to_string(), + context.session.conversation_id.to_string(), + ); + let env = apply_unified_exec_env(env); + let exec_server_env_config = ExecServerEnvConfig { + policy: exec_env_policy_from_shell_policy(&context.turn.shell_environment_policy), + local_policy_env, + }; let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new( self, @@ -680,6 +750,7 @@ impl UnifiedExecProcessManager { process_id: request.process_id, cwd, env, + exec_server_env_config: Some(exec_server_env_config), explicit_env_overrides: context.turn.shell_environment_policy.r#set.clone(), network: request.network.clone(), tty: request.tty, diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 6897f4a5fd..2c829cdd0e 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -34,6 +34,92 @@ fn unified_exec_env_overrides_existing_values() { assert_eq!(env.get("PATH"), Some(&"/usr/bin".to_string())); } +#[test] +fn env_overlay_for_exec_server_keeps_runtime_changes_only() { + let local_policy_env = HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/client-path".to_string()), + ("SHELL_SET".to_string(), "policy".to_string()), + ]); + let request_env = HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/sandbox-path".to_string()), + ("SHELL_SET".to_string(), "policy".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ( + "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), + "1".to_string(), + ), + ]); + + assert_eq!( + env_overlay_for_exec_server(&request_env, &local_policy_env), + HashMap::from([ + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ( + "CODEX_SANDBOX_NETWORK_DISABLED".to_string(), + "1".to_string() + ), + ]) + ); +} + +#[test] +fn exec_server_params_use_env_policy_overlay_contract() { + let request = ExecRequest { + command: vec!["bash".to_string(), "-lc".to_string(), "true".to_string()], + cwd: std::env::current_dir() + .expect("current dir") + .try_into() + .expect("absolute path"), + env: HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ]), + exec_server_env_config: Some(ExecServerEnvConfig { + policy: codex_exec_server::ExecEnvPolicy { + inherit: codex_config::types::ShellEnvironmentPolicyInherit::Core, + ignore_default_excludes: false, + exclude: Vec::new(), + r#set: HashMap::new(), + include_only: Vec::new(), + }, + local_policy_env: HashMap::from([ + ("HOME".to_string(), "/client-home".to_string()), + ("PATH".to_string(), "/client-path".to_string()), + ]), + }), + network: None, + expiration: crate::exec::ExecExpiration::DefaultTimeout, + capture_policy: crate::exec::ExecCapturePolicy::ShellTool, + sandbox: codex_sandboxing::SandboxType::None, + windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, + sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: codex_protocol::permissions::FileSystemSandboxPolicy::from( + &codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ), + network_sandbox_policy: codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + windows_sandbox_filesystem_overrides: None, + arg0: None, + }; + + let params = + exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true); + + assert_eq!(params.process_id.as_str(), "123"); + assert!(params.env_policy.is_some()); + assert_eq!( + params.env, + HashMap::from([ + ("PATH".to_string(), "/sandbox-path".to_string()), + ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), + ]) + ); +} + #[test] fn exec_server_process_id_matches_unified_exec_process_id() { assert_eq!(exec_server_process_id(/*process_id*/ 4321), "4321"); diff --git a/codex-rs/core/src/unified_exec/process_tests.rs b/codex-rs/core/src/unified_exec/process_tests.rs index 2816d3370d..065d65bc97 100644 --- a/codex-rs/core/src/unified_exec/process_tests.rs +++ b/codex-rs/core/src/unified_exec/process_tests.rs @@ -76,7 +76,7 @@ async fn remote_process(write_status: WriteStatus) -> UnifiedExecProcess { }), }; - UnifiedExecProcess::from_remote_started(started, SandboxType::None) + UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) .await .expect("remote process should start") } @@ -133,7 +133,7 @@ async fn remote_process_waits_for_early_exit_event() { let _ = wake_tx.send(1); }); - let process = UnifiedExecProcess::from_remote_started(started, SandboxType::None) + let process = UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) .await .expect("remote process should observe early exit"); diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 25570dc71b..251089ce36 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -15,6 +15,7 @@ arc-swap = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } codex-app-server-protocol = { workspace = true } +codex-config = { workspace = true } codex-protocol = { workspace = true } codex-sandboxing = { workspace = true } codex-utils-absolute-path = { workspace = true } @@ -42,5 +43,6 @@ uuid = { workspace = true, features = ["v4"] } anyhow = { workspace = true } codex-utils-cargo-bin = { workspace = true } pretty_assertions = { workspace = true } +serial_test = { workspace = true } tempfile = { workspace = true } test-case = "3.3.1" diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index a118468f5b..77ead87a84 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -343,6 +343,7 @@ mod tests { process_id: ProcessId::from("default-env-proc"), argv: vec!["true".to_string()], cwd: std::env::current_dir().expect("read current dir"), + env_policy: None, env: Default::default(), tty: false, arg0: None, diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 81bb8a6cd6..a73182697a 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -42,6 +42,7 @@ pub use process::ExecProcess; pub use process::StartedExecProcess; pub use process_id::ProcessId; pub use protocol::ExecClosedNotification; +pub use protocol::ExecEnvPolicy; pub use protocol::ExecExitedNotification; pub use protocol::ExecOutputDeltaNotification; pub use protocol::ExecOutputStream; diff --git a/codex-rs/exec-server/src/local_process.rs b/codex-rs/exec-server/src/local_process.rs index 5a5a6a6e66..bf38aa360b 100644 --- a/codex-rs/exec-server/src/local_process.rs +++ b/codex-rs/exec-server/src/local_process.rs @@ -5,6 +5,9 @@ use std::time::Duration; use async_trait::async_trait; use codex_app_server_protocol::JSONRPCErrorError; +use codex_config::shell_environment; +use codex_config::types::EnvironmentVariablePattern; +use codex_config::types::ShellEnvironmentPolicy; use codex_utils_pty::ExecCommandSession; use codex_utils_pty::TerminalSize; use tokio::sync::Mutex; @@ -19,6 +22,7 @@ use crate::ProcessId; use crate::StartedExecProcess; use crate::protocol::EXEC_CLOSED_METHOD; use crate::protocol::ExecClosedNotification; +use crate::protocol::ExecEnvPolicy; use crate::protocol::ExecExitedNotification; use crate::protocol::ExecOutputDeltaNotification; use crate::protocol::ExecOutputStream; @@ -150,12 +154,13 @@ impl LocalProcess { process_map.insert(process_id.clone(), ProcessEntry::Starting); } + let env = child_env(¶ms); let spawned_result = if params.tty { codex_utils_pty::spawn_pty_process( program, args, params.cwd.as_path(), - ¶ms.env, + &env, ¶ms.arg0, TerminalSize::default(), ) @@ -165,7 +170,7 @@ impl LocalProcess { program, args, params.cwd.as_path(), - ¶ms.env, + &env, ¶ms.arg0, ) .await @@ -375,6 +380,36 @@ impl LocalProcess { } } +fn child_env(params: &ExecParams) -> HashMap { + let Some(env_policy) = ¶ms.env_policy else { + return params.env.clone(); + }; + + let policy = shell_environment_policy(env_policy); + let mut env = shell_environment::create_env(&policy, /*thread_id*/ None); + env.extend(params.env.clone()); + env +} + +fn shell_environment_policy(env_policy: &ExecEnvPolicy) -> ShellEnvironmentPolicy { + ShellEnvironmentPolicy { + inherit: env_policy.inherit.clone(), + ignore_default_excludes: env_policy.ignore_default_excludes, + exclude: env_policy + .exclude + .iter() + .map(|pattern| EnvironmentVariablePattern::new_case_insensitive(pattern)) + .collect(), + r#set: env_policy.r#set.clone(), + include_only: env_policy + .include_only + .iter() + .map(|pattern| EnvironmentVariablePattern::new_case_insensitive(pattern)) + .collect(), + use_profile: false, + } +} + #[async_trait] impl ExecBackend for LocalProcess { async fn start(&self, params: ExecParams) -> Result { @@ -618,3 +653,56 @@ fn notification_sender(inner: &Inner) -> Option { .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } + +#[cfg(test)] +mod tests { + use super::*; + use codex_config::types::ShellEnvironmentPolicyInherit; + + fn test_exec_params(env: HashMap) -> ExecParams { + ExecParams { + process_id: ProcessId::from("env-test"), + argv: vec!["true".to_string()], + cwd: std::path::PathBuf::from("/tmp"), + env_policy: None, + env, + tty: false, + arg0: None, + } + } + + #[test] + fn child_env_defaults_to_exact_env() { + let params = test_exec_params(HashMap::from([("ONLY_THIS".to_string(), "1".to_string())])); + + assert_eq!( + child_env(¶ms), + HashMap::from([("ONLY_THIS".to_string(), "1".to_string())]) + ); + } + + #[test] + fn child_env_applies_policy_then_overlay() { + let mut params = test_exec_params(HashMap::from([ + ("OVERLAY".to_string(), "overlay".to_string()), + ("POLICY_SET".to_string(), "overlay-wins".to_string()), + ])); + params.env_policy = Some(ExecEnvPolicy { + inherit: ShellEnvironmentPolicyInherit::None, + ignore_default_excludes: true, + exclude: Vec::new(), + r#set: HashMap::from([("POLICY_SET".to_string(), "policy".to_string())]), + include_only: Vec::new(), + }); + + let mut expected = HashMap::from([ + ("OVERLAY".to_string(), "overlay".to_string()), + ("POLICY_SET".to_string(), "overlay-wins".to_string()), + ]); + if cfg!(target_os = "windows") { + expected.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); + } + + assert_eq!(child_env(¶ms), expected); + } +} diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index b353637627..0ccb9794a6 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use crate::FileSystemSandboxContext; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use codex_config::types::ShellEnvironmentPolicyInherit; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use serde::Serialize; @@ -64,11 +65,23 @@ pub struct ExecParams { pub process_id: ProcessId, pub argv: Vec, pub cwd: PathBuf, + #[serde(default)] + pub env_policy: Option, pub env: HashMap, pub tty: bool, pub arg0: Option, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ExecEnvPolicy { + pub inherit: ShellEnvironmentPolicyInherit, + pub ignore_default_excludes: bool, + pub exclude: Vec, + pub r#set: HashMap, + pub include_only: Vec, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ExecResponse { diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 5474099460..321bf243a9 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -27,6 +27,7 @@ fn exec_params_with_argv(process_id: &str, argv: Vec) -> ExecParams { process_id: ProcessId::from(process_id), argv, cwd: std::env::current_dir().expect("cwd"), + env_policy: None, env: inherited_path_env(), tty: false, arg0: None, diff --git a/codex-rs/exec-server/src/server/processor.rs b/codex-rs/exec-server/src/server/processor.rs index 6ea6a55bd1..87b9704502 100644 --- a/codex-rs/exec-server/src/server/processor.rs +++ b/codex-rs/exec-server/src/server/processor.rs @@ -390,6 +390,7 @@ mod tests { process_id, argv: sleep_then_print_argv(), cwd: std::env::current_dir().expect("cwd"), + env_policy: None, env, tty: false, arg0: None, diff --git a/codex-rs/exec-server/tests/common/exec_server.rs b/codex-rs/exec-server/tests/common/exec_server.rs index 0b6eec5447..fc11f87a05 100644 --- a/codex-rs/exec-server/tests/common/exec_server.rs +++ b/codex-rs/exec-server/tests/common/exec_server.rs @@ -11,6 +11,7 @@ use codex_app_server_protocol::RequestId; use codex_utils_cargo_bin::cargo_bin; use futures::SinkExt; use futures::StreamExt; +use tempfile::TempDir; use tokio::io::AsyncBufReadExt; use tokio::io::BufReader; use tokio::process::Child; @@ -26,6 +27,7 @@ const CONNECT_RETRY_INTERVAL: Duration = Duration::from_millis(25); const EVENT_TIMEOUT: Duration = Duration::from_secs(5); pub(crate) struct ExecServerHarness { + _codex_home: TempDir, child: Child, websocket_url: String, websocket: tokio_tungstenite::WebSocketStream< @@ -42,16 +44,20 @@ impl Drop for ExecServerHarness { pub(crate) async fn exec_server() -> anyhow::Result { let binary = cargo_bin("codex")?; + let codex_home = TempDir::new()?; let mut child = Command::new(binary); child.args(["exec-server", "--listen", "ws://127.0.0.1:0"]); child.stdin(Stdio::null()); child.stdout(Stdio::piped()); child.stderr(Stdio::inherit()); + child.kill_on_drop(true); + child.env("CODEX_HOME", codex_home.path()); let mut child = child.spawn()?; let websocket_url = read_listen_url_from_stdout(&mut child).await?; let (websocket, _) = connect_websocket_when_ready(&websocket_url).await?; Ok(ExecServerHarness { + _codex_home: codex_home, child, websocket_url, websocket, @@ -140,6 +146,9 @@ impl ExecServerHarness { pub(crate) async fn shutdown(&mut self) -> anyhow::Result<()> { self.child.start_kill()?; + timeout(CONNECT_TIMEOUT, self.child.wait()) + .await + .map_err(|_| anyhow!("timed out waiting for exec-server shutdown"))??; Ok(()) } diff --git a/codex-rs/exec-server/tests/exec_process.rs b/codex-rs/exec-server/tests/exec_process.rs index ab232ccd3e..afe0c2e356 100644 --- a/codex-rs/exec-server/tests/exec_process.rs +++ b/codex-rs/exec-server/tests/exec_process.rs @@ -51,6 +51,7 @@ async fn assert_exec_process_starts_and_exits(use_remote: bool) -> Result<()> { process_id: ProcessId::from("proc-1"), argv: vec!["true".to_string()], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -127,6 +128,7 @@ async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { "sleep 0.05; printf 'session output\\n'".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -156,6 +158,7 @@ async fn assert_exec_process_write_then_read(use_remote: bool) -> Result<()> { "import sys; line = sys.stdin.readline(); sys.stdout.write(f'from-stdin:{line}'); sys.stdout.flush()".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: true, arg0: None, @@ -192,6 +195,7 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( "printf 'queued output\\n'".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -210,6 +214,8 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// Serialize tests that launch a real exec-server process through the full CLI. +#[serial_test::serial(remote_exec_server)] async fn remote_exec_process_reports_transport_disconnect() -> Result<()> { let mut context = create_process_context(/*use_remote*/ true).await?; let session = context @@ -222,6 +228,7 @@ async fn remote_exec_process_reports_transport_disconnect() -> Result<()> { "sleep 10".to_string(), ], cwd: std::env::current_dir()?, + env_policy: /*env_policy*/ None, env: Default::default(), tty: false, arg0: None, @@ -255,6 +262,8 @@ async fn remote_exec_process_reports_transport_disconnect() -> Result<()> { #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// Serialize tests that launch a real exec-server process through the full CLI. +#[serial_test::serial(remote_exec_server)] async fn exec_process_starts_and_exits(use_remote: bool) -> Result<()> { assert_exec_process_starts_and_exits(use_remote).await } @@ -262,6 +271,8 @@ async fn exec_process_starts_and_exits(use_remote: bool) -> Result<()> { #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// Serialize tests that launch a real exec-server process through the full CLI. +#[serial_test::serial(remote_exec_server)] async fn exec_process_streams_output(use_remote: bool) -> Result<()> { assert_exec_process_streams_output(use_remote).await } @@ -269,6 +280,8 @@ async fn exec_process_streams_output(use_remote: bool) -> Result<()> { #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// Serialize tests that launch a real exec-server process through the full CLI. +#[serial_test::serial(remote_exec_server)] async fn exec_process_write_then_read(use_remote: bool) -> Result<()> { assert_exec_process_write_then_read(use_remote).await } @@ -276,6 +289,8 @@ async fn exec_process_write_then_read(use_remote: bool) -> Result<()> { #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// Serialize tests that launch a real exec-server process through the full CLI. +#[serial_test::serial(remote_exec_server)] async fn exec_process_preserves_queued_events_before_subscribe(use_remote: bool) -> Result<()> { assert_exec_process_preserves_queued_events_before_subscribe(use_remote).await } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e7ef6a0606..5531f4a4c6 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2193,20 +2193,11 @@ impl ChatWidget { self.request_redraw(); } + #[cfg(test)] fn on_agent_message(&mut self, message: String) { self.finalize_completed_assistant_message(Some(&message)); } - fn on_context_compacted(&mut self) { - self.flush_answer_stream_with_separator(); - self.handle_stream_finished(); - self.add_to_history(history_cell::new_info_event( - "Context compacted".to_owned(), - /*hint*/ None, - )); - self.request_redraw(); - } - fn on_agent_message_delta(&mut self, delta: String) { self.handle_streaming_delta(delta); } @@ -4565,10 +4556,14 @@ impl ChatWidget { pub(crate) fn handle_request_user_input_now(&mut self, ev: RequestUserInputEvent) { self.flush_answer_stream_with_separator(); - self.notify(Notification::UserInputRequested { - question_count: ev.questions.len(), - summary: Notification::user_input_request_summary(&ev.questions), - }); + let question_count = ev.questions.len(); + let summary = Notification::user_input_request_summary(&ev.questions); + let title = match (question_count, summary.as_deref()) { + (1, Some(summary)) => summary.to_string(), + (1, None) => "Question requested".to_string(), + (count, _) => format!("{count} questions requested"), + }; + self.notify(Notification::PlanModePrompt { title }); self.bottom_pane.push_user_input_request(ev); self.request_redraw(); } @@ -5930,7 +5925,7 @@ impl ChatWidget { self.exit_review_mode_after_item(); } ThreadItem::ContextCompaction { .. } => { - self.on_agent_message("Context compacted".to_owned()); + self.add_info_message("Context compacted".to_string(), /*hint*/ None); } ThreadItem::HookPrompt { .. } => {} ThreadItem::CollabAgentToolCall { @@ -6254,7 +6249,7 @@ impl ChatWidget { | ServerNotification::WindowsWorldWritableWarning(_) | ServerNotification::WindowsSandboxSetupCompleted(_) | ServerNotification::AccountLoginCompleted(_) => {} - ServerNotification::ContextCompacted(_) => self.on_context_compacted(), + ServerNotification::ContextCompacted(_) => {} } } @@ -6739,7 +6734,7 @@ impl ChatWidget { self.on_entered_review_mode(review_request, from_replay) } EventMsg::ExitedReviewMode(review) => self.on_exited_review_mode(review), - EventMsg::ContextCompacted(_) => self.on_context_compacted(), + EventMsg::ContextCompacted(_) => {} EventMsg::CollabAgentSpawnBegin(CollabAgentSpawnBeginEvent { call_id, model, @@ -10678,26 +10673,11 @@ impl Renderable for ChatWidget { #[derive(Debug)] enum Notification { - AgentTurnComplete { - response: String, - }, - ExecApprovalRequested { - command: String, - }, - EditApprovalRequested { - cwd: PathBuf, - changes: Vec, - }, - ElicitationRequested { - server_name: String, - }, - PlanModePrompt { - title: String, - }, - UserInputRequested { - question_count: usize, - summary: Option, - }, + AgentTurnComplete { response: String }, + ExecApprovalRequested { command: String }, + EditApprovalRequested { cwd: PathBuf, changes: Vec }, + ElicitationRequested { server_name: String }, + PlanModePrompt { title: String }, } impl Notification { @@ -10730,14 +10710,6 @@ impl Notification { Notification::PlanModePrompt { title } => { format!("Plan mode prompt: {title}") } - Notification::UserInputRequested { - question_count, - summary, - } => match (*question_count, summary.as_deref()) { - (1, Some(summary)) => format!("Question requested: {summary}"), - (1, None) => "Question requested".to_string(), - (count, _) => format!("Questions requested: {count}"), - }, } } @@ -10748,7 +10720,6 @@ impl Notification { | Notification::EditApprovalRequested { .. } | Notification::ElicitationRequested { .. } => "approval-requested", Notification::PlanModePrompt { .. } => "plan-mode-prompt", - Notification::UserInputRequested { .. } => "user-input-requested", } } @@ -10758,8 +10729,7 @@ impl Notification { Notification::ExecApprovalRequested { .. } | Notification::EditApprovalRequested { .. } | Notification::ElicitationRequested { .. } - | Notification::PlanModePrompt { .. } - | Notification::UserInputRequested { .. } => 1, + | Notification::PlanModePrompt { .. } => 1, } } diff --git a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs index ecdb15a398..083cb8069a 100644 --- a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs @@ -265,25 +265,6 @@ fn plan_mode_prompt_notification_uses_dedicated_type_name() { ); } -#[test] -fn user_input_requested_notification_uses_dedicated_type_name() { - let notification = Notification::UserInputRequested { - question_count: 1, - summary: Some("Reasoning scope".to_string()), - }; - - assert!(notification.allowed_for(&Notifications::Custom(vec![ - "user-input-requested".to_string(), - ]))); - assert!(!notification.allowed_for(&Notifications::Custom(vec![ - "approval-requested".to_string(), - ]))); - assert_eq!( - notification.display(), - "Question requested: Reasoning scope" - ); -} - #[tokio::test] async fn open_plan_implementation_prompt_sets_pending_notification() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await; @@ -331,7 +312,7 @@ async fn agent_turn_complete_does_not_override_pending_plan_mode_prompt_notifica } #[tokio::test] -async fn user_input_notification_overrides_pending_agent_turn_complete_notification() { +async fn request_user_input_notification_overrides_pending_agent_turn_complete_notification() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await; chat.notify(Notification::AgentTurnComplete { @@ -355,10 +336,7 @@ async fn user_input_notification_overrides_pending_agent_turn_complete_notificat assert_matches!( chat.pending_notification, - Some(Notification::UserInputRequested { - question_count: 1, - summary: Some(ref summary), - }) if summary == "Reasoning scope" + Some(Notification::PlanModePrompt { ref title }) if title == "Reasoning scope" ); } @@ -366,7 +344,7 @@ async fn user_input_notification_overrides_pending_agent_turn_complete_notificat async fn handle_request_user_input_sets_pending_notification() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.1-codex-max")).await; chat.config.tui_notifications.notifications = - Notifications::Custom(vec!["user-input-requested".to_string()]); + Notifications::Custom(vec!["plan-mode-prompt".to_string()]); chat.handle_request_user_input_now(RequestUserInputEvent { call_id: "call-1".to_string(), @@ -386,10 +364,7 @@ async fn handle_request_user_input_sets_pending_notification() { assert_matches!( chat.pending_notification, - Some(Notification::UserInputRequested { - question_count: 1, - summary: Some(ref summary), - }) if summary == "Reasoning scope" + Some(Notification::PlanModePrompt { ref title }) if title == "Reasoning scope" ); } diff --git a/codex-rs/tui/src/status/card.rs b/codex-rs/tui/src/status/card.rs index 340f933689..20b4c6ed8d 100644 --- a/codex-rs/tui/src/status/card.rs +++ b/codex-rs/tui/src/status/card.rs @@ -457,11 +457,21 @@ impl StatusHistoryCell { resets_at, } => { let percent_remaining = (100.0 - percent_used).clamp(0.0, 100.0); - let value_spans = vec![ + let summary = format_status_limit_summary(percent_remaining); + let full_value_spans = vec![ Span::from(render_status_limit_progress_bar(percent_remaining)), Span::from(" "), - Span::from(format_status_limit_summary(percent_remaining)), + Span::from(summary.clone()), ]; + // On narrow terminals, keep the percentage visible rather than + // letting the fixed-width progress bar crowd out the reset time. + let value_spans = if line_display_width(&Line::from(full_value_spans.clone())) + <= formatter.value_width(available_inner_width) + { + full_value_spans + } else { + vec![Span::from(summary)] + }; let base_spans = formatter.full_spans(row.label.as_str(), value_spans); let base_line = Line::from(base_spans.clone()); @@ -477,7 +487,21 @@ impl StatusHistoryCell { lines.push(Line::from(inline_spans)); } else { lines.push(base_line); - lines.push(formatter.continuation(vec![resets_span])); + let reset_text = format!("(resets {resets_at})"); + let reset_width = formatter.value_width(available_inner_width).max(1); + let wrap_options = + textwrap::Options::new(reset_width).break_words(false); + // Reset timestamps are the actionable part of this row, so wrap them + // onto continuation lines instead of truncating partial times/dates. + lines.extend( + textwrap::wrap(reset_text.as_str(), wrap_options) + .into_iter() + .map(|wrapped| { + formatter.continuation(vec![ + Span::from(wrapped.into_owned()).dim(), + ]) + }), + ); } } else { lines.push(base_line);