diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index b36ed33f3f..9fec3372f4 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -10,6 +10,7 @@ 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; @@ -72,11 +73,11 @@ impl EnvironmentManager { /// Builds a test-only manager with environment access disabled. pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { - match Self::from_environments( - HashMap::new(), - local_runtime_paths, - /*default_environment*/ None, - ) { + let snapshot = EnvironmentProviderSnapshot { + environments: HashMap::new(), + default_environment_id: None, + }; + match Self::from_provider_snapshot(snapshot, local_runtime_paths) { Ok(manager) => manager, Err(err) => panic!("disabled test environment manager: {err}"), } @@ -105,13 +106,8 @@ impl EnvironmentManager { local_runtime_paths: ExecServerRuntimePaths, ) -> Self { let provider = DefaultEnvironmentProvider::new(exec_server_url); - let provider_environments = provider.environments(&local_runtime_paths); - let default_environment = provider.default_environment_id(&provider_environments); - match Self::from_environments( - provider_environments, - local_runtime_paths, - default_environment, - ) { + let snapshot = provider.snapshot(&local_runtime_paths); + match Self::from_provider_snapshot(snapshot, local_runtime_paths) { Ok(manager) => manager, Err(err) => panic!("default provider should create valid environments: {err}"), } @@ -125,16 +121,20 @@ impl EnvironmentManager { where P: EnvironmentProvider + ?Sized, { - let environments = provider.get_environments(&local_runtime_paths).await?; - let default_environment = provider.default_environment_id(&environments); - Self::from_provider_environments(environments, local_runtime_paths, default_environment) + let snapshot = provider + .get_environment_snapshot(&local_runtime_paths) + .await?; + Self::from_provider_snapshot(snapshot, local_runtime_paths) } - fn from_provider_environments( - environments: HashMap, + fn from_provider_snapshot( + snapshot: EnvironmentProviderSnapshot, local_runtime_paths: ExecServerRuntimePaths, - default_environment: Option, ) -> Result { + let EnvironmentProviderSnapshot { + environments, + default_environment_id, + } = snapshot; for id in environments.keys() { if id.is_empty() { return Err(ExecServerError::Protocol( @@ -143,15 +143,7 @@ impl EnvironmentManager { } } - Self::from_environments(environments, local_runtime_paths, default_environment) - } - - fn from_environments( - environments: HashMap, - local_runtime_paths: ExecServerRuntimePaths, - default_environment: Option, - ) -> Result { - if let Some(environment_id) = default_environment.as_ref() + if let Some(environment_id) = default_environment_id.as_ref() && !environments.contains_key(environment_id) { return Err(ExecServerError::Protocol(format!( @@ -165,7 +157,7 @@ impl EnvironmentManager { .collect(); Ok(Self { - default_environment, + default_environment: default_environment_id, environments, local_environment, }) @@ -441,15 +433,17 @@ mod tests { } #[tokio::test] - async fn environment_manager_builds_from_provider_environments() { - let manager = EnvironmentManager::from_environments( - HashMap::from([( - REMOTE_ENVIRONMENT_ID.to_string(), - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - )]), + 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()), + }, test_runtime_paths(), - Some(REMOTE_ENVIRONMENT_ID.to_string()), ) .expect("environment manager"); @@ -469,10 +463,12 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_empty_environment_id() { - let err = EnvironmentManager::from_provider_environments( - HashMap::from([("".to_string(), Environment::default_for_tests())]), + let err = EnvironmentManager::from_provider_snapshot( + EnvironmentProviderSnapshot { + environments: HashMap::from([("".to_string(), Environment::default_for_tests())]), + default_environment_id: None, + }, test_runtime_paths(), - /*default_environment*/ None, ) .expect_err("empty id should fail"); @@ -484,20 +480,22 @@ mod tests { #[tokio::test] async fn environment_manager_uses_explicit_provider_default() { - let manager = EnvironmentManager::from_provider_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"), - ), - ]), + 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()), + }, test_runtime_paths(), - Some("devbox".to_string()), ) .expect("manager"); @@ -507,13 +505,15 @@ mod tests { #[tokio::test] async fn environment_manager_disables_provider_default() { - let manager = EnvironmentManager::from_provider_environments( - HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - )]), + let manager = EnvironmentManager::from_provider_snapshot( + EnvironmentProviderSnapshot { + environments: HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + Environment::default_for_tests(), + )]), + default_environment_id: None, + }, test_runtime_paths(), - /*default_environment*/ None, ) .expect("manager"); @@ -524,13 +524,15 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_unknown_provider_default() { - let err = EnvironmentManager::from_provider_environments( - HashMap::from([( - LOCAL_ENVIRONMENT_ID.to_string(), - Environment::default_for_tests(), - )]), + 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()), + }, test_runtime_paths(), - Some("missing".to_string()), ) .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 1f85780622..39226cd0d1 100644 --- a/codex-rs/exec-server/src/environment_provider.rs +++ b/codex-rs/exec-server/src/environment_provider.rs @@ -17,17 +17,15 @@ use crate::environment::REMOTE_ENVIRONMENT_ID; #[async_trait] pub trait EnvironmentProvider: Send + Sync { /// Returns the environments available for a new manager. - async fn get_environments( + async fn get_environment_snapshot( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> Result, ExecServerError>; + ) -> Result; +} - fn default_environment_id( - &self, - environments: &HashMap, - ) -> Option { - derived_default_environment_id(environments) - } +pub struct EnvironmentProviderSnapshot { + pub environments: HashMap, + pub default_environment_id: Option, } /// Default provider backed by `CODEX_EXEC_SERVER_URL`. @@ -47,15 +45,15 @@ impl DefaultEnvironmentProvider { Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok()) } - pub(crate) fn environments( + pub(crate) fn snapshot( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> HashMap { + ) -> EnvironmentProviderSnapshot { 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; + 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( @@ -64,28 +62,26 @@ impl DefaultEnvironmentProvider { ); } - environments + let default_environment_id = if disabled { + None + } else { + derived_default_environment_id(&environments) + }; + + EnvironmentProviderSnapshot { + environments, + default_environment_id, + } } } #[async_trait] impl EnvironmentProvider for DefaultEnvironmentProvider { - async fn get_environments( + async fn get_environment_snapshot( &self, local_runtime_paths: &ExecServerRuntimePaths, - ) -> Result, ExecServerError> { - Ok(self.environments(local_runtime_paths)) - } - - fn default_environment_id( - &self, - environments: &HashMap, - ) -> Option { - if normalize_exec_server_url(self.exec_server_url.clone()).1 { - None - } else { - derived_default_environment_id(environments) - } + ) -> Result { + Ok(self.snapshot(local_runtime_paths)) } } @@ -126,10 +122,11 @@ 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 environments = provider - .get_environments(&runtime_paths) + let snapshot = provider + .get_environment_snapshot(&runtime_paths) .await - .expect("environments"); + .expect("environment snapshot"); + let environments = snapshot.environments; assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert_eq!( @@ -143,10 +140,11 @@ 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 environments = provider - .get_environments(&runtime_paths) + let snapshot = provider + .get_environment_snapshot(&runtime_paths) .await - .expect("environments"); + .expect("environment snapshot"); + let environments = snapshot.environments; assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); @@ -156,24 +154,26 @@ 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 environments = provider - .get_environments(&runtime_paths) + let snapshot = provider + .get_environment_snapshot(&runtime_paths) .await - .expect("environments"); + .expect("environment snapshot"); + let environments = snapshot.environments; assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); - assert_eq!(provider.default_environment_id(&environments), None); + assert_eq!(snapshot.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 environments = provider - .get_environments(&runtime_paths) + let snapshot = provider + .get_environment_snapshot(&runtime_paths) .await - .expect("environments"); + .expect("environment snapshot"); + let environments = snapshot.environments; assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); let remote_environment = &environments[REMOTE_ENVIRONMENT_ID]; @@ -188,10 +188,11 @@ 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 environments = provider - .get_environments(&runtime_paths) + let snapshot = provider + .get_environment_snapshot(&runtime_paths) .await - .expect("environments"); + .expect("environment snapshot"); + let environments = snapshot.environments; assert_eq!( environments[REMOTE_ENVIRONMENT_ID].exec_server_url(),