From 9ddbb8b10e5cccd0e8237d2bb4383845da7553b3 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Mon, 4 May 2026 13:03:44 -0700 Subject: [PATCH] Split provider environments from default id Remove the EnvironmentProviderSnapshot wrapper. Providers now expose environments and the selected default id directly, while EnvironmentManager validates that the default id exists in the returned environment map. Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 117 ++++++++---------- .../exec-server/src/environment_provider.rs | 94 +++++++------- 2 files changed, 93 insertions(+), 118 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 9fec3372f4..dc4bd64989 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -10,7 +10,6 @@ use crate::client::http_client::ReqwestHttpClient; use crate::client_api::ExecServerTransportParams; use crate::environment_provider::DefaultEnvironmentProvider; use crate::environment_provider::EnvironmentProvider; -use crate::environment_provider::EnvironmentProviderSnapshot; use crate::environment_provider::normalize_exec_server_url; use crate::local_file_system::LocalFileSystem; use crate::local_process::LocalProcess; @@ -24,7 +23,8 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; /// /// `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. +/// while provider-based construction accepts a provider-supplied environment +/// list and default id. /// /// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving /// the default environment unset while still keeping an explicit local @@ -73,11 +73,7 @@ impl EnvironmentManager { /// Builds a test-only manager with environment access disabled. pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { - let snapshot = EnvironmentProviderSnapshot { - environments: HashMap::new(), - default_environment_id: None, - }; - match Self::from_provider_snapshot(snapshot, local_runtime_paths) { + match Self::from_provider_parts(HashMap::new(), None, local_runtime_paths) { Ok(manager) => manager, Err(err) => panic!("disabled test environment manager: {err}"), } @@ -106,14 +102,17 @@ impl EnvironmentManager { local_runtime_paths: ExecServerRuntimePaths, ) -> Self { let provider = DefaultEnvironmentProvider::new(exec_server_url); - let snapshot = provider.snapshot(&local_runtime_paths); - match Self::from_provider_snapshot(snapshot, local_runtime_paths) { + match Self::from_provider_parts( + provider.environments(&local_runtime_paths), + provider.default_id(), + local_runtime_paths, + ) { Ok(manager) => manager, Err(err) => panic!("default provider should create valid environments: {err}"), } } - /// Builds a manager from a provider-supplied startup snapshot. + /// Builds a manager from a provider-supplied environment list and default. pub async fn from_provider

( provider: &P, local_runtime_paths: ExecServerRuntimePaths, @@ -121,20 +120,16 @@ impl EnvironmentManager { where P: EnvironmentProvider + ?Sized, { - let snapshot = provider - .get_environment_snapshot(&local_runtime_paths) - .await?; - Self::from_provider_snapshot(snapshot, local_runtime_paths) + let environments = provider.get_environments(&local_runtime_paths).await?; + let default_environment_id = provider.default_environment_id(); + Self::from_provider_parts(environments, default_environment_id, local_runtime_paths) } - fn from_provider_snapshot( - snapshot: EnvironmentProviderSnapshot, + fn from_provider_parts( + environments: HashMap, + default_environment_id: Option, local_runtime_paths: ExecServerRuntimePaths, ) -> Result { - let EnvironmentProviderSnapshot { - environments, - default_environment_id, - } = snapshot; for id in environments.keys() { if id.is_empty() { return Err(ExecServerError::Protocol( @@ -433,16 +428,14 @@ mod tests { } #[tokio::test] - async fn environment_manager_builds_from_provider_snapshot() { - let manager = EnvironmentManager::from_provider_snapshot( - EnvironmentProviderSnapshot { - environments: HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )]), - default_environment_id: Some(REMOTE_ENVIRONMENT_ID.to_string()), - }, + async fn environment_manager_builds_from_provider_parts() { + let manager = EnvironmentManager::from_provider_parts( + HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + )]), + Some(REMOTE_ENVIRONMENT_ID.to_string()), test_runtime_paths(), ) .expect("environment manager"); @@ -463,11 +456,9 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_empty_environment_id() { - let err = EnvironmentManager::from_provider_snapshot( - EnvironmentProviderSnapshot { - environments: HashMap::from([("".to_string(), Environment::default_for_tests())]), - default_environment_id: None, - }, + let err = EnvironmentManager::from_provider_parts( + HashMap::from([("".to_string(), Environment::default_for_tests())]), + None, test_runtime_paths(), ) .expect_err("empty id should fail"); @@ -480,21 +471,19 @@ mod tests { #[tokio::test] async fn environment_manager_uses_explicit_provider_default() { - let manager = EnvironmentManager::from_provider_snapshot( - EnvironmentProviderSnapshot { - environments: HashMap::from([ - ( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - ), - ( - "devbox".to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - ), - ]), - default_environment_id: Some("devbox".to_string()), - }, + let manager = EnvironmentManager::from_provider_parts( + HashMap::from([ + ( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::default_for_tests(), + ), + ( + "devbox".to_string(), + Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) + .expect("remote environment"), + ), + ]), + Some("devbox".to_string()), test_runtime_paths(), ) .expect("manager"); @@ -505,14 +494,12 @@ mod tests { #[tokio::test] async fn environment_manager_disables_provider_default() { - let manager = EnvironmentManager::from_provider_snapshot( - EnvironmentProviderSnapshot { - environments: HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - )]), - default_environment_id: None, - }, + let manager = EnvironmentManager::from_provider_parts( + HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::default_for_tests(), + )]), + None, test_runtime_paths(), ) .expect("manager"); @@ -524,14 +511,12 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_unknown_provider_default() { - let err = EnvironmentManager::from_provider_snapshot( - EnvironmentProviderSnapshot { - environments: HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - )]), - default_environment_id: Some("missing".to_string()), - }, + let err = EnvironmentManager::from_provider_parts( + HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::default_for_tests(), + )]), + Some("missing".to_string()), test_runtime_paths(), ) .expect_err("unknown default should fail"); diff --git a/codex-rs/exec-server/src/environment_provider.rs b/codex-rs/exec-server/src/environment_provider.rs index 39226cd0d1..57f4340498 100644 --- a/codex-rs/exec-server/src/environment_provider.rs +++ b/codex-rs/exec-server/src/environment_provider.rs @@ -11,21 +11,20 @@ use crate::environment::REMOTE_ENVIRONMENT_ID; /// Lists the concrete environments available to Codex. /// -/// 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. +/// Implementations own both the available environment list and the default +/// environment id. 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 environments available for a new manager. - async fn get_environment_snapshot( + async fn get_environments( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> Result; -} + ) -> Result, ExecServerError>; -pub struct EnvironmentProviderSnapshot { - pub environments: HashMap, - pub default_environment_id: Option, + /// Returns the provider-selected default environment id, or `None` to + /// disable model-facing environment access. + fn default_environment_id(&self) -> Option; } /// Default provider backed by `CODEX_EXEC_SERVER_URL`. @@ -45,15 +44,15 @@ impl DefaultEnvironmentProvider { Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok()) } - pub(crate) fn snapshot( + pub(crate) fn environments( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> EnvironmentProviderSnapshot { + ) -> HashMap { let mut environments = HashMap::from([( LOCAL_ENVIRONMENT_ID.to_string(), Environment::local(local_runtime_paths.clone()), )]); - let (exec_server_url, disabled) = normalize_exec_server_url(self.exec_server_url.clone()); + let (exec_server_url, _disabled) = normalize_exec_server_url(self.exec_server_url.clone()); if let Some(exec_server_url) = exec_server_url { environments.insert( @@ -62,36 +61,32 @@ impl DefaultEnvironmentProvider { ); } - let default_environment_id = if disabled { - None - } else { - derived_default_environment_id(&environments) - }; + environments + } - EnvironmentProviderSnapshot { - environments, - default_environment_id, + pub(crate) fn default_id(&self) -> Option { + let (exec_server_url, disabled) = normalize_exec_server_url(self.exec_server_url.clone()); + if disabled { + None + } else if exec_server_url.is_some() { + Some(REMOTE_ENVIRONMENT_ID.to_string()) + } else { + Some(LOCAL_ENVIRONMENT_ID.to_string()) } } } #[async_trait] impl EnvironmentProvider for DefaultEnvironmentProvider { - async fn get_environment_snapshot( + async fn get_environments( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> Result { - Ok(self.snapshot(local_runtime_paths)) + ) -> Result, ExecServerError> { + Ok(self.environments(local_runtime_paths)) } -} -fn derived_default_environment_id(environments: &HashMap) -> Option { - 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 { - None + fn default_environment_id(&self) -> Option { + self.default_id() } } @@ -122,11 +117,10 @@ mod tests { 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 snapshot = provider - .get_environment_snapshot(&runtime_paths) + let environments = provider + .get_environments(&runtime_paths) .await - .expect("environment snapshot"); - let environments = snapshot.environments; + .expect("environments"); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert_eq!( @@ -140,11 +134,10 @@ mod tests { 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 snapshot = provider - .get_environment_snapshot(&runtime_paths) + let environments = provider + .get_environments(&runtime_paths) .await - .expect("environment snapshot"); - let environments = snapshot.environments; + .expect("environments"); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); @@ -154,26 +147,24 @@ mod tests { 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 snapshot = provider - .get_environment_snapshot(&runtime_paths) + let environments = provider + .get_environments(&runtime_paths) .await - .expect("environment snapshot"); - let environments = snapshot.environments; + .expect("environments"); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); - assert_eq!(snapshot.default_environment_id, None); + assert_eq!(provider.default_environment_id(), None); } #[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 snapshot = provider - .get_environment_snapshot(&runtime_paths) + let environments = provider + .get_environments(&runtime_paths) .await - .expect("environment snapshot"); - let environments = snapshot.environments; + .expect("environments"); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); let remote_environment = &environments[REMOTE_ENVIRONMENT_ID]; @@ -188,11 +179,10 @@ mod tests { 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 snapshot = provider - .get_environment_snapshot(&runtime_paths) + let environments = provider + .get_environments(&runtime_paths) .await - .expect("environment snapshot"); - let environments = snapshot.environments; + .expect("environments"); assert_eq!( environments[REMOTE_ENVIRONMENT_ID].exec_server_url(),