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(),