From 1526b2ce16b146c9d8127a47188c1dca73f432f8 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 28 Apr 2026 11:44:40 -0700 Subject: [PATCH 1/5] Add environment provider snapshot Separate provider-owned environment configuration listing from EnvironmentManager's concrete environment cache. The default provider preserves the existing CODEX_EXEC_SERVER_URL behavior while the manager keeps local environment ownership internal. Co-authored-by: Codex --- codex-rs/app-server-client/src/lib.rs | 17 +- codex-rs/app-server/src/lib.rs | 15 +- codex-rs/core/src/connectors.rs | 2 +- codex-rs/core/src/environment_selection.rs | 15 +- codex-rs/core/src/prompt_debug.rs | 4 +- codex-rs/core/src/thread_manager_tests.rs | 7 +- codex-rs/core/tests/common/test_codex.rs | 18 +- codex-rs/core/tests/suite/skills.rs | 13 +- codex-rs/exec-server/src/environment.rs | 237 ++++++++++++------ .../exec-server/src/environment_provider.rs | 220 ++++++++++++++++ codex-rs/exec-server/src/lib.rs | 5 + codex-rs/exec/src/lib.rs | 6 +- codex-rs/mcp-server/src/lib.rs | 15 +- codex-rs/tui/src/lib.rs | 31 +-- 14 files changed, 457 insertions(+), 148 deletions(-) create mode 100644 codex-rs/exec-server/src/environment_provider.rs diff --git a/codex-rs/app-server-client/src/lib.rs b/codex-rs/app-server-client/src/lib.rs index 1e7a84efe7ff..8165fb367344 100644 --- a/codex-rs/app-server-client/src/lib.rs +++ b/codex-rs/app-server-client/src/lib.rs @@ -2029,14 +2029,17 @@ mod tests { #[tokio::test] async fn runtime_start_args_forward_environment_manager() { let config = Arc::new(build_test_config().await); - let environment_manager = Arc::new(EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("ws://127.0.0.1:8765".to_string()), - local_runtime_paths: ExecServerRuntimePaths::new( - std::env::current_exe().expect("current exe"), - /*codex_linux_sandbox_exe*/ None, + let environment_manager = Arc::new( + EnvironmentManager::create_for_tests( + Some("ws://127.0.0.1:8765".to_string()), + ExecServerRuntimePaths::new( + std::env::current_exe().expect("current exe"), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"), ) - .expect("runtime paths"), - })); + .await, + ); let runtime_args = InProcessClientStartArgs { arg0_paths: Arg0DispatchPaths::default(), diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index de8960810122..44fe776dd61a 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -414,12 +414,15 @@ pub async fn run_main_with_transport_options( auth: AppServerWebsocketAuthSettings, runtime_options: AppServerRuntimeOptions, ) -> IoResult<()> { - let environment_manager = Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::from_env( - ExecServerRuntimePaths::from_optional_paths( - arg0_paths.codex_self_exe.clone(), - arg0_paths.codex_linux_sandbox_exe.clone(), - )?, - ))); + let environment_manager = Arc::new( + EnvironmentManager::new(EnvironmentManagerArgs::new( + ExecServerRuntimePaths::from_optional_paths( + arg0_paths.codex_self_exe.clone(), + arg0_paths.codex_linux_sandbox_exe.clone(), + )?, + )) + .await, + ); let (transport_event_tx, mut transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let (outgoing_tx, mut outgoing_rx) = mpsc::channel::(CHANNEL_CAPACITY); diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index d06ea24609f7..7d325abb43b2 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -201,7 +201,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status( config.codex_linux_sandbox_exe.clone(), )?; let environment_manager = - EnvironmentManager::new(EnvironmentManagerArgs::from_env(local_runtime_paths)); + EnvironmentManager::new(EnvironmentManagerArgs::new(local_runtime_paths)).await; list_accessible_connectors_from_mcp_tools_with_environment_manager( config, force_refetch, diff --git a/codex-rs/core/src/environment_selection.rs b/codex-rs/core/src/environment_selection.rs index 7f93ba384c5c..a33aae92b094 100644 --- a/codex-rs/core/src/environment_selection.rs +++ b/codex-rs/core/src/environment_selection.rs @@ -61,7 +61,6 @@ pub(crate) fn selected_primary_environment( #[cfg(test)] mod tests { - use codex_exec_server::EnvironmentManagerArgs; use codex_exec_server::ExecServerRuntimePaths; use codex_exec_server::REMOTE_ENVIRONMENT_ID; use codex_protocol::protocol::TurnEnvironmentSelection; @@ -81,10 +80,11 @@ mod tests { #[tokio::test] async fn default_thread_environment_selections_use_manager_default_id() { let cwd = AbsolutePathBuf::current_dir().expect("cwd"); - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("ws://127.0.0.1:8765".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = EnvironmentManager::create_for_tests( + Some("ws://127.0.0.1:8765".to_string()), + test_runtime_paths(), + ) + .await; assert_eq!( default_thread_environment_selections(&manager, &cwd), @@ -98,10 +98,7 @@ mod tests { #[tokio::test] async fn default_thread_environment_selections_empty_when_default_disabled() { let cwd = AbsolutePathBuf::current_dir().expect("cwd"); - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("none".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); assert_eq!( default_thread_environment_selections(&manager, &cwd), diff --git a/codex-rs/core/src/prompt_debug.rs b/codex-rs/core/src/prompt_debug.rs index f782c0d232e0..cc1203c2db38 100644 --- a/codex-rs/core/src/prompt_debug.rs +++ b/codex-rs/core/src/prompt_debug.rs @@ -45,9 +45,7 @@ pub async fn build_prompt_input( .features .enabled(Feature::DefaultModeRequestUserInput), }, - Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::from_env( - local_runtime_paths, - ))), + Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::new(local_runtime_paths)).await), /*analytics_events_client*/ None, ); let thread = thread_manager.start_thread(config).await?; diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 9307927b036a..e63ade3462e1 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -53,11 +53,8 @@ fn disabled_environment_manager_for_tests() -> Arc Result<()> { codex_core::test_support::auth_manager_from_auth(CodexAuth::from_api_key("dummy")), SessionSource::Exec, CollaborationModesConfig::default(), - Arc::new(EnvironmentManager::new( - codex_exec_server::EnvironmentManagerArgs { - exec_server_url: Some("none".to_string()), - local_runtime_paths: ExecServerRuntimePaths::new( - std::env::current_exe()?, - /*codex_linux_sandbox_exe*/ None, - )?, - }, + Arc::new(EnvironmentManager::disabled_for_tests( + ExecServerRuntimePaths::new( + std::env::current_exe()?, + /*codex_linux_sandbox_exe*/ None, + )?, )), /*analytics_events_client*/ None, ); diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index b934ec8d933e..87449b87f05f 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -7,6 +7,10 @@ use crate::ExecutorFileSystem; use crate::HttpClient; use crate::client::LazyRemoteExecServerClient; use crate::client::http_client::ReqwestHttpClient; +use crate::environment_provider::DefaultEnvironmentProvider; +use crate::environment_provider::EnvironmentConfigurations; +use crate::environment_provider::EnvironmentProvider; +use crate::environment_provider::normalize_exec_server_url; use crate::local_file_system::LocalFileSystem; use crate::local_process::LocalProcess; use crate::process::ExecBackend; @@ -17,11 +21,9 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; /// Owns the execution/filesystem environments available to the Codex runtime. /// -/// `EnvironmentManager` is a shared registry for concrete environments. It -/// always creates a local environment under [`LOCAL_ENVIRONMENT_ID`]. When -/// `CODEX_EXEC_SERVER_URL` is set to a websocket URL, it also creates a remote -/// environment under [`REMOTE_ENVIRONMENT_ID`] and makes that the default -/// environment. Otherwise the local environment is the default. +/// `EnvironmentManager` is a shared registry for concrete environments. Its +/// default constructor preserves the legacy `CODEX_EXEC_SERVER_URL` behavior +/// while provider-based construction accepts a provider-supplied snapshot. /// /// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving /// the default environment unset while still keeping the local environment @@ -43,21 +45,12 @@ pub const REMOTE_ENVIRONMENT_ID: &str = "remote"; #[derive(Clone, Debug)] pub struct EnvironmentManagerArgs { - pub exec_server_url: Option, pub local_runtime_paths: ExecServerRuntimePaths, } impl EnvironmentManagerArgs { pub fn new(local_runtime_paths: ExecServerRuntimePaths) -> Self { Self { - exec_server_url: None, - local_runtime_paths, - } - } - - pub fn from_env(local_runtime_paths: ExecServerRuntimePaths) -> Self { - Self { - exec_server_url: std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok(), local_runtime_paths, } } @@ -75,35 +68,93 @@ impl EnvironmentManager { } } - /// Builds a manager from the raw `CODEX_EXEC_SERVER_URL` value and local - /// runtime paths used when creating local filesystem helpers. - pub fn new(args: EnvironmentManagerArgs) -> Self { + /// Builds a test-only manager with environment access disabled. + pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { + let mut manager = + Self::from_configurations(EnvironmentConfigurations::empty(), local_runtime_paths); + manager.default_environment = None; + manager + } + + /// Builds a test-only manager from a raw exec-server URL value. + pub async fn create_for_tests( + exec_server_url: Option, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { + Self::from_default_provider_url(exec_server_url, local_runtime_paths).await + } + + /// Builds a manager from `CODEX_EXEC_SERVER_URL` and local runtime paths + /// used when creating local filesystem helpers. + pub async fn new(args: EnvironmentManagerArgs) -> Self { let EnvironmentManagerArgs { - exec_server_url, local_runtime_paths, } = args; - let (exec_server_url, environment_disabled) = normalize_exec_server_url(exec_server_url); - let mut environments = HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - Arc::new(Environment::local(local_runtime_paths.clone())), - )]); - let default_environment = if environment_disabled { - None + let exec_server_url = std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok(); + Self::from_default_provider_url(exec_server_url, local_runtime_paths).await + } + + async fn from_default_provider_url( + exec_server_url: Option, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { + let environment_disabled = normalize_exec_server_url(exec_server_url.clone()).1; + let provider = DefaultEnvironmentProvider::new(exec_server_url); + let mut manager = Self::from_configurations( + provider.environment_configurations().await, + local_runtime_paths, + ); + if environment_disabled { + // TODO: Remove this legacy `CODEX_EXEC_SERVER_URL=none` crutch once + // environment attachment defaulting moves out of EnvironmentManager. + manager.default_environment = None; + } + manager + } + + /// Builds a manager from a provider-supplied startup snapshot. + pub async fn from_provider

( + provider: &P, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Result + where + P: EnvironmentProvider + ?Sized, + { + Ok(Self::from_configurations( + provider.get_environments().await?, + local_runtime_paths, + )) + } + + fn from_configurations( + environment_configurations: EnvironmentConfigurations, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { + let environment_configurations = environment_configurations.into_environments(); + // TODO: Stop deriving a default environment here once omitted + // environment attachment is owned by thread/session setup. + let default_environment = if environment_configurations.contains_key(REMOTE_ENVIRONMENT_ID) + { + REMOTE_ENVIRONMENT_ID } else { - match exec_server_url { - Some(exec_server_url) => { - environments.insert( - REMOTE_ENVIRONMENT_ID.to_string(), - Arc::new(Environment::remote(exec_server_url, local_runtime_paths)), - ); - Some(REMOTE_ENVIRONMENT_ID.to_string()) - } - None => Some(LOCAL_ENVIRONMENT_ID.to_string()), - } - }; + LOCAL_ENVIRONMENT_ID + } + .to_string(); + let mut environments = HashMap::new(); + for (id, configuration) in environment_configurations { + let environment = Environment::remote_inner( + configuration.exec_server_url, + Some(local_runtime_paths.clone()), + ); + environments.insert(id, Arc::new(environment)); + } + environments.insert( + LOCAL_ENVIRONMENT_ID.to_string(), + Arc::new(Environment::local(local_runtime_paths)), + ); Self { - default_environment, + default_environment: Some(default_environment), environments, } } @@ -216,10 +267,6 @@ impl Environment { } } - fn remote(exec_server_url: String, local_runtime_paths: ExecServerRuntimePaths) -> Self { - Self::remote_inner(exec_server_url, Some(local_runtime_paths)) - } - fn remote_inner( exec_server_url: String, local_runtime_paths: Option, @@ -264,22 +311,17 @@ impl Environment { } } -fn normalize_exec_server_url(exec_server_url: Option) -> (Option, bool) { - match exec_server_url.as_deref().map(str::trim) { - None | Some("") => (None, false), - Some(url) if url.eq_ignore_ascii_case("none") => (None, true), - Some(url) => (Some(url.to_string()), false), - } -} #[cfg(test)] mod tests { + use std::collections::HashMap; use std::sync::Arc; use super::Environment; use super::EnvironmentManager; - use super::EnvironmentManagerArgs; use super::LOCAL_ENVIRONMENT_ID; use super::REMOTE_ENVIRONMENT_ID; + use crate::EnvironmentConfiguration; + use crate::EnvironmentConfigurations; use crate::ExecServerRuntimePaths; use crate::ProcessId; use pretty_assertions::assert_eq; @@ -303,10 +345,8 @@ mod tests { #[tokio::test] async fn environment_manager_normalizes_empty_url() { - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some(String::new()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = + EnvironmentManager::create_for_tests(Some(String::new()), test_runtime_paths()).await; let environment = manager.default_environment().expect("default environment"); assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID)); @@ -321,11 +361,8 @@ mod tests { } #[tokio::test] - async fn environment_manager_treats_none_value_as_disabled() { - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("none".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + async fn disabled_environment_manager_has_no_default_but_keeps_local_lookup() { + let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); @@ -340,10 +377,11 @@ mod tests { #[tokio::test] async fn environment_manager_reports_remote_url() { - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("ws://127.0.0.1:8765".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = EnvironmentManager::create_for_tests( + Some("ws://127.0.0.1:8765".to_string()), + test_runtime_paths(), + ) + .await; let environment = manager.default_environment().expect("default environment"); assert_eq!( @@ -380,34 +418,80 @@ mod tests { )); } + #[tokio::test] + async fn environment_manager_builds_from_provider_configurations() { + let manager = EnvironmentManager::from_configurations( + EnvironmentConfigurations::new(HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { + exec_server_url: "ws://127.0.0.1:8765".to_string(), + }, + )])) + .expect("environment configurations"), + test_runtime_paths(), + ); + + assert_eq!( + manager.default_environment_id(), + Some(REMOTE_ENVIRONMENT_ID) + ); + assert!( + manager + .get_environment(REMOTE_ENVIRONMENT_ID) + .expect("remote environment") + .is_remote() + ); + assert!( + !manager + .get_environment(LOCAL_ENVIRONMENT_ID) + .expect("local environment") + .is_remote() + ); + } + + #[tokio::test] + async fn environment_manager_inserts_local_environment() { + let manager = EnvironmentManager::from_configurations( + EnvironmentConfigurations::empty(), + test_runtime_paths(), + ); + + assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID)); + assert!( + !manager + .get_environment(LOCAL_ENVIRONMENT_ID) + .expect("local environment") + .is_remote() + ); + } + #[tokio::test] async fn environment_manager_carries_local_runtime_paths() { let runtime_paths = test_runtime_paths(); - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: None, - local_runtime_paths: runtime_paths.clone(), - }); + let manager = EnvironmentManager::create_for_tests( + /*exec_server_url*/ None, + runtime_paths.clone(), + ) + .await; let environment = manager.default_environment().expect("default environment"); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: environment.exec_server_url().map(str::to_owned), - local_runtime_paths: environment + let manager = EnvironmentManager::create_for_tests( + environment.exec_server_url().map(str::to_owned), + environment .local_runtime_paths() .expect("local runtime paths") .clone(), - }); + ) + .await; let environment = manager.default_environment().expect("default environment"); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); } #[tokio::test] async fn disabled_environment_manager_has_no_default_environment() { - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("none".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); @@ -415,10 +499,7 @@ mod tests { #[tokio::test] async fn environment_manager_keeps_local_lookup_when_default_disabled() { - let manager = EnvironmentManager::new(EnvironmentManagerArgs { - exec_server_url: Some("none".to_string()), - local_runtime_paths: test_runtime_paths(), - }); + let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); diff --git a/codex-rs/exec-server/src/environment_provider.rs b/codex-rs/exec-server/src/environment_provider.rs new file mode 100644 index 000000000000..c1fdf7f79377 --- /dev/null +++ b/codex-rs/exec-server/src/environment_provider.rs @@ -0,0 +1,220 @@ +use std::collections::HashMap; + +use async_trait::async_trait; + +use crate::ExecServerError; +use crate::environment::CODEX_EXEC_SERVER_URL_ENV_VAR; +use crate::environment::REMOTE_ENVIRONMENT_ID; + +/// Provider-supplied environment definition consumed by `EnvironmentManager`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EnvironmentConfiguration { + pub exec_server_url: String, +} + +/// Provider-supplied environment snapshot consumed by `EnvironmentManager`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct EnvironmentConfigurations { + environments: HashMap, +} + +impl EnvironmentConfigurations { + pub fn new( + mut environments: HashMap, + ) -> Result { + for (id, configuration) in &mut environments { + if id.is_empty() { + return Err(ExecServerError::Protocol( + "environment configuration id cannot be empty".to_string(), + )); + } + + match normalize_exec_server_url(Some(configuration.exec_server_url.clone())) { + (Some(exec_server_url), false) => { + configuration.exec_server_url = exec_server_url; + } + (None, false) | (None, true) | (Some(_), true) => { + return Err(ExecServerError::Protocol(format!( + "environment configuration `{id}` must set a remote exec-server URL" + ))); + } + } + } + + Ok(Self { environments }) + } + + pub fn empty() -> Self { + Self { + environments: HashMap::new(), + } + } + + pub(crate) fn remote(exec_server_url: String) -> Self { + Self { + environments: HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { exec_server_url }, + )]), + } + } + + pub(crate) fn into_environments(self) -> HashMap { + self.environments + } +} + +/// Lists the concrete environment configurations available to Codex. +/// +/// Implementations should return the provider-owned portion of the startup +/// snapshot that `EnvironmentManager` will cache. The local environment is +/// always supplied by `EnvironmentManager`. +#[async_trait] +pub trait EnvironmentProvider: Send + Sync { + /// Returns the environment configurations available for a new manager. + async fn get_environments(&self) -> Result; +} + +/// Default provider backed by `CODEX_EXEC_SERVER_URL`. +#[derive(Clone, Debug)] +pub struct DefaultEnvironmentProvider { + exec_server_url: Option, +} + +impl DefaultEnvironmentProvider { + /// Builds a provider from an already-read raw `CODEX_EXEC_SERVER_URL` value. + pub fn new(exec_server_url: Option) -> Self { + Self { exec_server_url } + } + + /// Builds a provider by reading `CODEX_EXEC_SERVER_URL`. + pub fn from_env() -> Self { + Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok()) + } + + pub(crate) async fn environment_configurations(&self) -> EnvironmentConfigurations { + let exec_server_url = normalize_exec_server_url(self.exec_server_url.clone()).0; + + if let Some(exec_server_url) = exec_server_url { + EnvironmentConfigurations::remote(exec_server_url) + } else { + EnvironmentConfigurations::empty() + } + } +} + +#[async_trait] +impl EnvironmentProvider for DefaultEnvironmentProvider { + async fn get_environments(&self) -> Result { + Ok(self.environment_configurations().await) + } +} + +pub(crate) fn normalize_exec_server_url(exec_server_url: Option) -> (Option, bool) { + match exec_server_url.as_deref().map(str::trim) { + None | Some("") => (None, false), + Some(url) if url.eq_ignore_ascii_case("none") => (None, true), + Some(url) => (Some(url.to_string()), false), + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::*; + + #[tokio::test] + async fn default_provider_returns_no_provider_environment_when_url_is_missing() { + let provider = DefaultEnvironmentProvider::new(/*exec_server_url*/ None); + + assert_eq!( + provider.get_environments().await.expect("environments"), + EnvironmentConfigurations::empty() + ); + } + + #[tokio::test] + async fn default_provider_returns_no_provider_environment_when_url_is_empty() { + let provider = DefaultEnvironmentProvider::new(Some(String::new())); + + assert_eq!( + provider.get_environments().await.expect("environments"), + EnvironmentConfigurations::empty() + ); + } + + #[tokio::test] + async fn default_provider_returns_no_provider_environment_for_none_value() { + let provider = DefaultEnvironmentProvider::new(Some("none".to_string())); + + assert_eq!( + provider.get_environments().await.expect("environments"), + EnvironmentConfigurations::empty() + ); + } + + #[tokio::test] + async fn default_provider_adds_remote_environment_for_websocket_url() { + let provider = DefaultEnvironmentProvider::new(Some("ws://127.0.0.1:8765".to_string())); + + assert_eq!( + provider.get_environments().await.expect("environments"), + EnvironmentConfigurations::new(HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { + exec_server_url: "ws://127.0.0.1:8765".to_string(), + }, + )])) + .expect("environment configurations") + ); + } + + #[test] + fn environment_configurations_rejects_empty_exec_server_url() { + let err = EnvironmentConfigurations::new(HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { + exec_server_url: String::new(), + }, + )])) + .expect_err("empty URL should fail"); + + assert_eq!( + err.to_string(), + "exec-server protocol error: environment configuration `remote` must set a remote exec-server URL" + ); + } + + #[test] + fn environment_configurations_rejects_disabled_exec_server_url() { + let err = EnvironmentConfigurations::new(HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { + exec_server_url: "none".to_string(), + }, + )])) + .expect_err("disabled URL should fail"); + + assert_eq!( + err.to_string(), + "exec-server protocol error: environment configuration `remote` must set a remote exec-server URL" + ); + } + + #[test] + fn environment_configurations_normalizes_exec_server_url() { + let configurations = EnvironmentConfigurations::new(HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + EnvironmentConfiguration { + exec_server_url: " ws://127.0.0.1:8765 ".to_string(), + }, + )])) + .expect("environment configurations"); + + assert_eq!( + configurations, + EnvironmentConfigurations::remote("ws://127.0.0.1:8765".to_string()) + ); + } +} diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index e739b056443a..ab41c4750ca7 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -2,6 +2,7 @@ mod client; mod client_api; mod connection; mod environment; +mod environment_provider; mod fs_helper; mod fs_helper_main; mod fs_sandbox; @@ -38,6 +39,10 @@ pub use environment::EnvironmentManager; pub use environment::EnvironmentManagerArgs; pub use environment::LOCAL_ENVIRONMENT_ID; pub use environment::REMOTE_ENVIRONMENT_ID; +pub use environment_provider::DefaultEnvironmentProvider; +pub use environment_provider::EnvironmentConfiguration; +pub use environment_provider::EnvironmentConfigurations; +pub use environment_provider::EnvironmentProvider; pub use fs_helper::CODEX_FS_HELPER_ARG1; pub use fs_helper_main::main as run_fs_helper_main; pub use local_file_system::LOCAL_FS; diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index d4044a728d86..e6fe5abe0cd3 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -502,9 +502,9 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result cloud_requirements: run_cloud_requirements, feedback: CodexFeedback::new(), log_db: None, - environment_manager: std::sync::Arc::new(EnvironmentManager::new( - EnvironmentManagerArgs::from_env(local_runtime_paths), - )), + environment_manager: std::sync::Arc::new( + EnvironmentManager::new(EnvironmentManagerArgs::new(local_runtime_paths)).await, + ), config_warnings, session_source: SessionSource::Exec, enable_codex_api_key_env: true, diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index 95fc4341ee78..bb54ffcc53f8 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -60,12 +60,15 @@ pub async fn run_main( arg0_paths: Arg0DispatchPaths, cli_config_overrides: CliConfigOverrides, ) -> IoResult<()> { - let environment_manager = Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::from_env( - ExecServerRuntimePaths::from_optional_paths( - arg0_paths.codex_self_exe.clone(), - arg0_paths.codex_linux_sandbox_exe.clone(), - )?, - ))); + let environment_manager = Arc::new( + EnvironmentManager::new(EnvironmentManagerArgs::new( + ExecServerRuntimePaths::from_optional_paths( + arg0_paths.codex_self_exe.clone(), + arg0_paths.codex_linux_sandbox_exe.clone(), + )?, + )) + .await, + ); // Parse CLI overrides once and derive the base Config eagerly so later // components do not need to work with raw TOML values. let cli_kv_overrides = cli_config_overrides.parse_overrides().map_err(|e| { diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index d09876739858..9ef4c4994157 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -745,12 +745,15 @@ pub async fn run_main( } }; - let environment_manager = Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::from_env( - ExecServerRuntimePaths::from_optional_paths( - arg0_paths.codex_self_exe.clone(), - arg0_paths.codex_linux_sandbox_exe.clone(), - )?, - ))); + let environment_manager = Arc::new( + EnvironmentManager::new(EnvironmentManagerArgs::new( + ExecServerRuntimePaths::from_optional_paths( + arg0_paths.codex_self_exe.clone(), + arg0_paths.codex_linux_sandbox_exe.clone(), + )?, + )) + .await, + ); let cwd = cli.cwd.clone(); let config_cwd = config_cwd_for_app_server_target(cwd.as_deref(), &app_server_target, &environment_manager)?; @@ -2017,14 +2020,14 @@ mod tests { Path::new("/definitely/not/local/to/this/test") }; let target = AppServerTarget::Embedded; - let environment_manager = - EnvironmentManager::new(codex_exec_server::EnvironmentManagerArgs { - exec_server_url: Some("ws://127.0.0.1:8765".to_string()), - local_runtime_paths: ExecServerRuntimePaths::new( - std::env::current_exe().expect("current exe"), - /*codex_linux_sandbox_exe*/ None, - )?, - }); + let environment_manager = EnvironmentManager::create_for_tests( + Some("ws://127.0.0.1:8765".to_string()), + ExecServerRuntimePaths::new( + std::env::current_exe().expect("current exe"), + /*codex_linux_sandbox_exe*/ None, + )?, + ) + .await; let config_cwd = config_cwd_for_app_server_target(Some(remote_only_cwd), &target, &environment_manager)?; From 869b20927d9d91fbf390b39de77727cfe2012432 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 28 Apr 2026 17:42:23 -0700 Subject: [PATCH 2/5] Return environments from provider Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 161 +++++++------ .../exec-server/src/environment_provider.rs | 222 +++++++----------- codex-rs/exec-server/src/lib.rs | 2 - 3 files changed, 170 insertions(+), 215 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 87449b87f05f..5f093b492f9e 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -8,7 +8,6 @@ use crate::HttpClient; use crate::client::LazyRemoteExecServerClient; use crate::client::http_client::ReqwestHttpClient; use crate::environment_provider::DefaultEnvironmentProvider; -use crate::environment_provider::EnvironmentConfigurations; use crate::environment_provider::EnvironmentProvider; use crate::environment_provider::normalize_exec_server_url; use crate::local_file_system::LocalFileSystem; @@ -26,8 +25,8 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; /// while provider-based construction accepts a provider-supplied snapshot. /// /// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving -/// the default environment unset while still keeping the local environment -/// available for internal callers by id. Callers use +/// the default environment unset while still keeping an explicit local +/// environment available through `local_environment()`. Callers use /// `default_environment().is_some()` as the signal for model-facing /// shell/filesystem tool availability. /// @@ -38,6 +37,7 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; pub struct EnvironmentManager { default_environment: Option, environments: HashMap>, + local_environment: Arc, } pub const LOCAL_ENVIRONMENT_ID: &str = "local"; @@ -65,13 +65,14 @@ impl EnvironmentManager { LOCAL_ENVIRONMENT_ID.to_string(), Arc::new(Environment::default_for_tests()), )]), + local_environment: Arc::new(Environment::default_for_tests()), } } /// Builds a test-only manager with environment access disabled. pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { let mut manager = - Self::from_configurations(EnvironmentConfigurations::empty(), local_runtime_paths); + Self::from_environments(HashMap::new(), local_runtime_paths).expect("empty manager"); manager.default_environment = None; manager } @@ -100,10 +101,9 @@ impl EnvironmentManager { ) -> Self { let environment_disabled = normalize_exec_server_url(exec_server_url.clone()).1; let provider = DefaultEnvironmentProvider::new(exec_server_url); - let mut manager = Self::from_configurations( - provider.environment_configurations().await, - local_runtime_paths, - ); + let provider_environments = provider.environments(&local_runtime_paths); + let mut manager = Self::from_environments(provider_environments, local_runtime_paths) + .expect("default provider returns valid environment ids"); if environment_disabled { // TODO: Remove this legacy `CODEX_EXEC_SERVER_URL=none` crutch once // environment attachment defaulting moves out of EnvironmentManager. @@ -120,43 +120,44 @@ impl EnvironmentManager { where P: EnvironmentProvider + ?Sized, { - Ok(Self::from_configurations( - provider.get_environments().await?, + Self::from_environments( + provider.get_environments(&local_runtime_paths).await?, local_runtime_paths, - )) + ) } - fn from_configurations( - environment_configurations: EnvironmentConfigurations, + fn from_environments( + environments: HashMap, local_runtime_paths: ExecServerRuntimePaths, - ) -> Self { - let environment_configurations = environment_configurations.into_environments(); + ) -> Result { + for id in environments.keys() { + if id.is_empty() { + return Err(ExecServerError::Protocol( + "environment id cannot be empty".to_string(), + )); + } + } + // TODO: Stop deriving a default environment here once omitted // environment attachment is owned by thread/session setup. - let default_environment = if environment_configurations.contains_key(REMOTE_ENVIRONMENT_ID) - { - REMOTE_ENVIRONMENT_ID + let default_environment = if environments.contains_key(REMOTE_ENVIRONMENT_ID) { + Some(REMOTE_ENVIRONMENT_ID.to_string()) + } else if environments.contains_key(LOCAL_ENVIRONMENT_ID) { + Some(LOCAL_ENVIRONMENT_ID.to_string()) } else { - LOCAL_ENVIRONMENT_ID - } - .to_string(); - let mut environments = HashMap::new(); - for (id, configuration) in environment_configurations { - let environment = Environment::remote_inner( - configuration.exec_server_url, - Some(local_runtime_paths.clone()), - ); - environments.insert(id, Arc::new(environment)); - } - environments.insert( - LOCAL_ENVIRONMENT_ID.to_string(), - Arc::new(Environment::local(local_runtime_paths)), - ); - - Self { - default_environment: Some(default_environment), + None + }; + let local_environment = Arc::new(Environment::local(local_runtime_paths)); + let environments = environments + .into_iter() + .map(|(id, environment)| (id, Arc::new(environment))) + .collect(); + + Ok(Self { + default_environment, environments, - } + local_environment, + }) } /// Returns the default environment instance. @@ -173,10 +174,7 @@ impl EnvironmentManager { /// Returns the local environment instance used for internal runtime work. pub fn local_environment(&self) -> Arc { - match self.get_environment(LOCAL_ENVIRONMENT_ID) { - Some(environment) => environment, - None => unreachable!("EnvironmentManager always has a local environment"), - } + Arc::clone(&self.local_environment) } /// Returns a named environment instance. @@ -255,7 +253,7 @@ impl Environment { }) } - fn local(local_runtime_paths: ExecServerRuntimePaths) -> Self { + pub(crate) fn local(local_runtime_paths: ExecServerRuntimePaths) -> Self { Self { exec_server_url: None, exec_backend: Arc::new(LocalProcess::default()), @@ -267,7 +265,7 @@ impl Environment { } } - fn remote_inner( + pub(crate) fn remote_inner( exec_server_url: String, local_runtime_paths: Option, ) -> Self { @@ -320,8 +318,6 @@ mod tests { use super::EnvironmentManager; use super::LOCAL_ENVIRONMENT_ID; use super::REMOTE_ENVIRONMENT_ID; - use crate::EnvironmentConfiguration; - use crate::EnvironmentConfigurations; use crate::ExecServerRuntimePaths; use crate::ProcessId; use pretty_assertions::assert_eq; @@ -361,17 +357,13 @@ mod tests { } #[tokio::test] - async fn disabled_environment_manager_has_no_default_but_keeps_local_lookup() { + async fn disabled_environment_manager_has_no_default_but_keeps_explicit_local_environment() { let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); - assert!( - !manager - .get_environment(LOCAL_ENVIRONMENT_ID) - .expect("local environment") - .is_remote() - ); + assert!(!manager.local_environment().is_remote()); + assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); } @@ -402,6 +394,7 @@ mod tests { .expect("local environment") .is_remote() ); + assert!(!manager.local_environment().is_remote()); } #[tokio::test] @@ -419,17 +412,16 @@ mod tests { } #[tokio::test] - async fn environment_manager_builds_from_provider_configurations() { - let manager = EnvironmentManager::from_configurations( - EnvironmentConfigurations::new(HashMap::from([( + async fn environment_manager_builds_from_provider_environments() { + let manager = EnvironmentManager::from_environments( + HashMap::from([( REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { - exec_server_url: "ws://127.0.0.1:8765".to_string(), - }, - )])) - .expect("environment configurations"), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )]), test_runtime_paths(), - ); + ) + .expect("environment manager"); assert_eq!( manager.default_environment_id(), @@ -441,28 +433,39 @@ mod tests { .expect("remote environment") .is_remote() ); - assert!( - !manager - .get_environment(LOCAL_ENVIRONMENT_ID) - .expect("local environment") - .is_remote() - ); + assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none()); + assert!(!manager.local_environment().is_remote()); } #[tokio::test] - async fn environment_manager_inserts_local_environment() { - let manager = EnvironmentManager::from_configurations( - EnvironmentConfigurations::empty(), + async fn environment_manager_rejects_empty_environment_id() { + let err = EnvironmentManager::from_environments( + HashMap::from([("".to_string(), Environment::default_for_tests())]), test_runtime_paths(), + ) + .expect_err("empty id should fail"); + + assert_eq!( + err.to_string(), + "exec-server protocol error: environment id cannot be empty" ); + } + + #[tokio::test] + async fn environment_manager_uses_provider_supplied_local_environment() { + let manager = EnvironmentManager::create_for_tests( + /*exec_server_url*/ None, + test_runtime_paths(), + ) + .await; assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID)); - assert!( - !manager - .get_environment(LOCAL_ENVIRONMENT_ID) - .expect("local environment") - .is_remote() - ); + let provider_local = manager + .get_environment(LOCAL_ENVIRONMENT_ID) + .expect("provider local environment"); + assert!(!provider_local.is_remote()); + assert!(!manager.local_environment().is_remote()); + assert!(!Arc::ptr_eq(&provider_local, &manager.local_environment())); } #[tokio::test] @@ -498,8 +501,10 @@ mod tests { } #[tokio::test] - async fn environment_manager_keeps_local_lookup_when_default_disabled() { - let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths()); + async fn environment_manager_keeps_default_provider_local_lookup_when_default_disabled() { + let manager = + EnvironmentManager::create_for_tests(Some("none".to_string()), test_runtime_paths()) + .await; assert!(manager.default_environment().is_none()); assert_eq!(manager.default_environment_id(), None); diff --git a/codex-rs/exec-server/src/environment_provider.rs b/codex-rs/exec-server/src/environment_provider.rs index c1fdf7f79377..7c8db07e85e5 100644 --- a/codex-rs/exec-server/src/environment_provider.rs +++ b/codex-rs/exec-server/src/environment_provider.rs @@ -2,77 +2,25 @@ use std::collections::HashMap; use async_trait::async_trait; +use crate::Environment; use crate::ExecServerError; +use crate::ExecServerRuntimePaths; use crate::environment::CODEX_EXEC_SERVER_URL_ENV_VAR; +use crate::environment::LOCAL_ENVIRONMENT_ID; use crate::environment::REMOTE_ENVIRONMENT_ID; -/// Provider-supplied environment definition consumed by `EnvironmentManager`. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct EnvironmentConfiguration { - pub exec_server_url: String, -} - -/// Provider-supplied environment snapshot consumed by `EnvironmentManager`. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct EnvironmentConfigurations { - environments: HashMap, -} - -impl EnvironmentConfigurations { - pub fn new( - mut environments: HashMap, - ) -> Result { - for (id, configuration) in &mut environments { - if id.is_empty() { - return Err(ExecServerError::Protocol( - "environment configuration id cannot be empty".to_string(), - )); - } - - match normalize_exec_server_url(Some(configuration.exec_server_url.clone())) { - (Some(exec_server_url), false) => { - configuration.exec_server_url = exec_server_url; - } - (None, false) | (None, true) | (Some(_), true) => { - return Err(ExecServerError::Protocol(format!( - "environment configuration `{id}` must set a remote exec-server URL" - ))); - } - } - } - - Ok(Self { environments }) - } - - pub fn empty() -> Self { - Self { - environments: HashMap::new(), - } - } - - pub(crate) fn remote(exec_server_url: String) -> Self { - Self { - environments: HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { exec_server_url }, - )]), - } - } - - pub(crate) fn into_environments(self) -> HashMap { - self.environments - } -} - -/// Lists the concrete environment configurations available to Codex. +/// Lists the concrete environments available to Codex. /// -/// Implementations should return the provider-owned portion of the startup -/// snapshot that `EnvironmentManager` will cache. The local environment is -/// always supplied by `EnvironmentManager`. +/// Implementations should return the provider-owned startup snapshot that +/// `EnvironmentManager` will cache. Providers that want the local environment to +/// be addressable by id should include it explicitly in the returned map. #[async_trait] pub trait EnvironmentProvider: Send + Sync { - /// Returns the environment configurations available for a new manager. - async fn get_environments(&self) -> Result; + /// Returns the environments available for a new manager. + async fn get_environments( + &self, + local_runtime_paths: &ExecServerRuntimePaths, + ) -> Result, ExecServerError>; } /// Default provider backed by `CODEX_EXEC_SERVER_URL`. @@ -92,21 +40,34 @@ impl DefaultEnvironmentProvider { Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok()) } - pub(crate) async fn environment_configurations(&self) -> EnvironmentConfigurations { + pub(crate) fn environments( + &self, + local_runtime_paths: &ExecServerRuntimePaths, + ) -> HashMap { + let mut environments = HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::local(local_runtime_paths.clone()), + )]); let exec_server_url = normalize_exec_server_url(self.exec_server_url.clone()).0; if let Some(exec_server_url) = exec_server_url { - EnvironmentConfigurations::remote(exec_server_url) - } else { - EnvironmentConfigurations::empty() + environments.insert( + REMOTE_ENVIRONMENT_ID.to_string(), + Environment::remote_inner(exec_server_url, Some(local_runtime_paths.clone())), + ); } + + environments } } #[async_trait] impl EnvironmentProvider for DefaultEnvironmentProvider { - async fn get_environments(&self) -> Result { - Ok(self.environment_configurations().await) + async fn get_environments( + &self, + local_runtime_paths: &ExecServerRuntimePaths, + ) -> Result, ExecServerError> { + Ok(self.environments(local_runtime_paths)) } } @@ -123,98 +84,89 @@ mod tests { use pretty_assertions::assert_eq; use super::*; + use crate::ExecServerRuntimePaths; + + fn test_runtime_paths() -> ExecServerRuntimePaths { + ExecServerRuntimePaths::new( + std::env::current_exe().expect("current exe"), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths") + } #[tokio::test] - async fn default_provider_returns_no_provider_environment_when_url_is_missing() { + async fn default_provider_returns_local_environment_when_url_is_missing() { let provider = DefaultEnvironmentProvider::new(/*exec_server_url*/ None); + let runtime_paths = test_runtime_paths(); + let environments = provider + .get_environments(&runtime_paths) + .await + .expect("environments"); + assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert_eq!( - provider.get_environments().await.expect("environments"), - EnvironmentConfigurations::empty() + environments[LOCAL_ENVIRONMENT_ID].local_runtime_paths(), + Some(&runtime_paths) ); + assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); } #[tokio::test] - async fn default_provider_returns_no_provider_environment_when_url_is_empty() { + async fn default_provider_returns_local_environment_when_url_is_empty() { let provider = DefaultEnvironmentProvider::new(Some(String::new())); + let runtime_paths = test_runtime_paths(); + let environments = provider + .get_environments(&runtime_paths) + .await + .expect("environments"); - assert_eq!( - provider.get_environments().await.expect("environments"), - EnvironmentConfigurations::empty() - ); + assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); + assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); } #[tokio::test] - async fn default_provider_returns_no_provider_environment_for_none_value() { + async fn default_provider_returns_local_environment_for_none_value() { let provider = DefaultEnvironmentProvider::new(Some("none".to_string())); + let runtime_paths = test_runtime_paths(); + let environments = provider + .get_environments(&runtime_paths) + .await + .expect("environments"); - assert_eq!( - provider.get_environments().await.expect("environments"), - EnvironmentConfigurations::empty() - ); + assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); + assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); } #[tokio::test] async fn default_provider_adds_remote_environment_for_websocket_url() { let provider = DefaultEnvironmentProvider::new(Some("ws://127.0.0.1:8765".to_string())); - + let runtime_paths = test_runtime_paths(); + let environments = provider + .get_environments(&runtime_paths) + .await + .expect("environments"); + + assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); + let remote_environment = &environments[REMOTE_ENVIRONMENT_ID]; + assert!(remote_environment.is_remote()); assert_eq!( - provider.get_environments().await.expect("environments"), - EnvironmentConfigurations::new(HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { - exec_server_url: "ws://127.0.0.1:8765".to_string(), - }, - )])) - .expect("environment configurations") + remote_environment.exec_server_url(), + Some("ws://127.0.0.1:8765") ); } - #[test] - fn environment_configurations_rejects_empty_exec_server_url() { - let err = EnvironmentConfigurations::new(HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { - exec_server_url: String::new(), - }, - )])) - .expect_err("empty URL should fail"); - - assert_eq!( - err.to_string(), - "exec-server protocol error: environment configuration `remote` must set a remote exec-server URL" - ); - } - - #[test] - fn environment_configurations_rejects_disabled_exec_server_url() { - let err = EnvironmentConfigurations::new(HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { - exec_server_url: "none".to_string(), - }, - )])) - .expect_err("disabled URL should fail"); - - assert_eq!( - err.to_string(), - "exec-server protocol error: environment configuration `remote` must set a remote exec-server URL" - ); - } - - #[test] - fn environment_configurations_normalizes_exec_server_url() { - let configurations = EnvironmentConfigurations::new(HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - EnvironmentConfiguration { - exec_server_url: " ws://127.0.0.1:8765 ".to_string(), - }, - )])) - .expect("environment configurations"); + #[tokio::test] + async fn default_provider_normalizes_exec_server_url() { + let provider = DefaultEnvironmentProvider::new(Some(" ws://127.0.0.1:8765 ".to_string())); + let runtime_paths = test_runtime_paths(); + let environments = provider + .get_environments(&runtime_paths) + .await + .expect("environments"); assert_eq!( - configurations, - EnvironmentConfigurations::remote("ws://127.0.0.1:8765".to_string()) + environments[REMOTE_ENVIRONMENT_ID].exec_server_url(), + Some("ws://127.0.0.1:8765") ); } } diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index ab41c4750ca7..1550653d94b8 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -40,8 +40,6 @@ pub use environment::EnvironmentManagerArgs; pub use environment::LOCAL_ENVIRONMENT_ID; pub use environment::REMOTE_ENVIRONMENT_ID; pub use environment_provider::DefaultEnvironmentProvider; -pub use environment_provider::EnvironmentConfiguration; -pub use environment_provider::EnvironmentConfigurations; pub use environment_provider::EnvironmentProvider; pub use fs_helper::CODEX_FS_HELPER_ARG1; pub use fs_helper_main::main as run_fs_helper_main; From 6415cb9a7c744a1a37323b1b0a3de2b80cc5b033 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 28 Apr 2026 18:50:36 -0700 Subject: [PATCH 3/5] Avoid provider construction expects Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 5f093b492f9e..5cbf9c141c3d 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -71,8 +71,7 @@ impl EnvironmentManager { /// Builds a test-only manager with environment access disabled. pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { - let mut manager = - Self::from_environments(HashMap::new(), local_runtime_paths).expect("empty manager"); + let mut manager = Self::from_environments(HashMap::new(), local_runtime_paths); manager.default_environment = None; manager } @@ -102,8 +101,7 @@ impl EnvironmentManager { let environment_disabled = normalize_exec_server_url(exec_server_url.clone()).1; let provider = DefaultEnvironmentProvider::new(exec_server_url); let provider_environments = provider.environments(&local_runtime_paths); - let mut manager = Self::from_environments(provider_environments, local_runtime_paths) - .expect("default provider returns valid environment ids"); + let mut manager = Self::from_environments(provider_environments, local_runtime_paths); if environment_disabled { // TODO: Remove this legacy `CODEX_EXEC_SERVER_URL=none` crutch once // environment attachment defaulting moves out of EnvironmentManager. @@ -120,13 +118,13 @@ impl EnvironmentManager { where P: EnvironmentProvider + ?Sized, { - Self::from_environments( + Self::from_provider_environments( provider.get_environments(&local_runtime_paths).await?, local_runtime_paths, ) } - fn from_environments( + fn from_provider_environments( environments: HashMap, local_runtime_paths: ExecServerRuntimePaths, ) -> Result { @@ -138,6 +136,13 @@ impl EnvironmentManager { } } + Ok(Self::from_environments(environments, local_runtime_paths)) + } + + fn from_environments( + environments: HashMap, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { // TODO: Stop deriving a default environment here once omitted // environment attachment is owned by thread/session setup. let default_environment = if environments.contains_key(REMOTE_ENVIRONMENT_ID) { @@ -153,11 +158,11 @@ impl EnvironmentManager { .map(|(id, environment)| (id, Arc::new(environment))) .collect(); - Ok(Self { + Self { default_environment, environments, local_environment, - }) + } } /// Returns the default environment instance. @@ -439,7 +444,7 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_empty_environment_id() { - let err = EnvironmentManager::from_environments( + let err = EnvironmentManager::from_provider_environments( HashMap::from([("".to_string(), Environment::default_for_tests())]), test_runtime_paths(), ) From 24aa8c1ac7dadcb614dbe2cbd7028c9259bded5d Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 28 Apr 2026 18:56:16 -0700 Subject: [PATCH 4/5] Fix provider environment test construction Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 5cbf9c141c3d..855989dafbc2 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -425,8 +425,7 @@ mod tests { .expect("remote environment"), )]), test_runtime_paths(), - ) - .expect("environment manager"); + ); assert_eq!( manager.default_environment_id(), From 9ef6ff7716279913930834ba4e01c89a8e837779 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 28 Apr 2026 19:04:13 -0700 Subject: [PATCH 5/5] Fix explicit local environment test Co-authored-by: Codex --- codex-rs/core/src/thread_manager_tests.rs | 25 ++++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index e63ade3462e1..54d7ea4f7211 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -47,17 +47,6 @@ fn assistant_msg(text: &str) -> ResponseItem { } } -fn disabled_environment_manager_for_tests() -> Arc { - let runtime_paths = codex_exec_server::ExecServerRuntimePaths::new( - std::env::current_exe().expect("current exe path"), - /*codex_linux_sandbox_exe*/ None, - ) - .expect("runtime paths"); - Arc::new(codex_exec_server::EnvironmentManager::disabled_for_tests( - runtime_paths, - )) -} - fn contextual_user_interrupted_marker() -> ResponseItem { interrupted_turn_history_marker(InterruptedTurnHistoryMarker::ContextualUser) .expect("contextual-user interrupted marker should be enabled") @@ -304,11 +293,23 @@ async fn start_thread_accepts_explicit_environment_when_default_environment_is_d config.cwd = config.codex_home.abs(); std::fs::create_dir_all(&config.codex_home).expect("create codex home"); + let runtime_paths = codex_exec_server::ExecServerRuntimePaths::new( + std::env::current_exe().expect("current exe path"), + /*codex_linux_sandbox_exe*/ None, + ) + .expect("runtime paths"); + let environment_manager = Arc::new( + codex_exec_server::EnvironmentManager::create_for_tests( + Some("none".to_string()), + runtime_paths, + ) + .await, + ); let manager = ThreadManager::with_models_provider_and_home_for_tests( CodexAuth::from_api_key("dummy"), config.model_provider.clone(), config.codex_home.to_path_buf(), - disabled_environment_manager_for_tests(), + environment_manager, ); let thread = manager