From 5086768859657ab27456b71267baba8b34aa6da4 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Mon, 4 May 2026 13:12:50 -0700 Subject: [PATCH] Inline provider manager construction Remove the private from_provider_parts helper. EnvironmentManager::from_provider now performs the provider read, validation, and manager construction directly, and tests use a small provider implementation instead of bypassing that path. Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 111 ++++++++++++++---------- 1 file changed, 64 insertions(+), 47 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index dc4bd64989..3c70374bad 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -73,9 +73,10 @@ impl EnvironmentManager { /// Builds a test-only manager with environment access disabled. pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { - match Self::from_provider_parts(HashMap::new(), None, local_runtime_paths) { - Ok(manager) => manager, - Err(err) => panic!("disabled test environment manager: {err}"), + Self { + default_environment: None, + environments: HashMap::new(), + local_environment: Arc::new(Environment::local(local_runtime_paths)), } } @@ -102,11 +103,7 @@ impl EnvironmentManager { local_runtime_paths: ExecServerRuntimePaths, ) -> Self { let provider = DefaultEnvironmentProvider::new(exec_server_url); - match Self::from_provider_parts( - provider.environments(&local_runtime_paths), - provider.default_id(), - local_runtime_paths, - ) { + match Self::from_provider(&provider, local_runtime_paths).await { Ok(manager) => manager, Err(err) => panic!("default provider should create valid environments: {err}"), } @@ -122,14 +119,6 @@ impl EnvironmentManager { { 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_parts( - environments: HashMap, - default_environment_id: Option, - local_runtime_paths: ExecServerRuntimePaths, - ) -> Result { for id in environments.keys() { if id.is_empty() { return Err(ExecServerError::Protocol( @@ -334,10 +323,33 @@ mod tests { use super::EnvironmentManager; use super::LOCAL_ENVIRONMENT_ID; use super::REMOTE_ENVIRONMENT_ID; + use crate::EnvironmentProvider; + use crate::ExecServerError; use crate::ExecServerRuntimePaths; use crate::ProcessId; + use async_trait::async_trait; use pretty_assertions::assert_eq; + #[derive(Clone)] + struct TestEnvironmentProvider { + environments: HashMap, + default_environment_id: Option, + } + + #[async_trait] + impl EnvironmentProvider for TestEnvironmentProvider { + async fn get_environments( + &self, + _local_runtime_paths: &ExecServerRuntimePaths, + ) -> Result, ExecServerError> { + Ok(self.environments.clone()) + } + + fn default_environment_id(&self) -> Option { + self.default_environment_id.clone() + } + } + fn test_runtime_paths() -> ExecServerRuntimePaths { ExecServerRuntimePaths::new( std::env::current_exe().expect("current exe"), @@ -428,17 +440,18 @@ mod tests { } #[tokio::test] - async fn environment_manager_builds_from_provider_parts() { - let manager = EnvironmentManager::from_provider_parts( - HashMap::from([( + async fn environment_manager_builds_from_provider() { + let provider = TestEnvironmentProvider { + environments: 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"); + default_environment_id: Some(REMOTE_ENVIRONMENT_ID.to_string()), + }; + let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) + .await + .expect("environment manager"); assert_eq!( manager.default_environment_id(), @@ -456,12 +469,13 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_empty_environment_id() { - let err = EnvironmentManager::from_provider_parts( - HashMap::from([("".to_string(), Environment::default_for_tests())]), - None, - test_runtime_paths(), - ) - .expect_err("empty id should fail"); + let provider = TestEnvironmentProvider { + environments: HashMap::from([("".to_string(), Environment::default_for_tests())]), + default_environment_id: None, + }; + let err = EnvironmentManager::from_provider(&provider, test_runtime_paths()) + .await + .expect_err("empty id should fail"); assert_eq!( err.to_string(), @@ -471,8 +485,8 @@ mod tests { #[tokio::test] async fn environment_manager_uses_explicit_provider_default() { - let manager = EnvironmentManager::from_provider_parts( - HashMap::from([ + let provider = TestEnvironmentProvider { + environments: HashMap::from([ ( LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(), @@ -483,10 +497,11 @@ mod tests { .expect("remote environment"), ), ]), - Some("devbox".to_string()), - test_runtime_paths(), - ) - .expect("manager"); + default_environment_id: Some("devbox".to_string()), + }; + let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) + .await + .expect("manager"); assert_eq!(manager.default_environment_id(), Some("devbox")); assert!(manager.default_environment().expect("default").is_remote()); @@ -494,15 +509,16 @@ mod tests { #[tokio::test] async fn environment_manager_disables_provider_default() { - let manager = EnvironmentManager::from_provider_parts( - HashMap::from([( + let provider = TestEnvironmentProvider { + environments: HashMap::from([( LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(), )]), - None, - test_runtime_paths(), - ) - .expect("manager"); + default_environment_id: None, + }; + let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths()) + .await + .expect("manager"); assert_eq!(manager.default_environment_id(), None); assert!(manager.default_environment().is_none()); @@ -511,15 +527,16 @@ mod tests { #[tokio::test] async fn environment_manager_rejects_unknown_provider_default() { - let err = EnvironmentManager::from_provider_parts( - HashMap::from([( + let provider = TestEnvironmentProvider { + environments: HashMap::from([( LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(), )]), - Some("missing".to_string()), - test_runtime_paths(), - ) - .expect_err("unknown default should fail"); + default_environment_id: Some("missing".to_string()), + }; + let err = EnvironmentManager::from_provider(&provider, test_runtime_paths()) + .await + .expect_err("unknown default should fail"); assert_eq!( err.to_string(),