From 93f68577ed2ce5f48d7b3cdcfb614ce4536fdcd9 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Mon, 4 May 2026 12:37:38 -0700 Subject: [PATCH] Simplify provider default environment selection Have providers return a concrete default environment id after constructing their environment map, using None to disable the default. This removes the DefaultEnvironmentSelection tri-state while preserving legacy derived defaults through the trait's default implementation. Co-authored-by: Codex --- codex-rs/exec-server/src/environment.rs | 60 +++++++------------ .../exec-server/src/environment_provider.rs | 38 +++++++----- codex-rs/exec-server/src/lib.rs | 1 - 3 files changed, 43 insertions(+), 56 deletions(-) diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 433d51e777..b36ed33f3f 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -9,7 +9,6 @@ use crate::client::LazyRemoteExecServerClient; use crate::client::http_client::ReqwestHttpClient; use crate::client_api::ExecServerTransportParams; use crate::environment_provider::DefaultEnvironmentProvider; -use crate::environment_provider::DefaultEnvironmentSelection; use crate::environment_provider::EnvironmentProvider; use crate::environment_provider::normalize_exec_server_url; use crate::local_file_system::LocalFileSystem; @@ -76,7 +75,7 @@ impl EnvironmentManager { match Self::from_environments( HashMap::new(), local_runtime_paths, - DefaultEnvironmentSelection::Disabled, + /*default_environment*/ None, ) { Ok(manager) => manager, Err(err) => panic!("disabled test environment manager: {err}"), @@ -107,10 +106,11 @@ impl EnvironmentManager { ) -> 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, - provider.default_environment_selection(), + default_environment, ) { Ok(manager) => manager, Err(err) => panic!("default provider should create valid environments: {err}"), @@ -125,17 +125,15 @@ impl EnvironmentManager { where P: EnvironmentProvider + ?Sized, { - Self::from_provider_environments( - provider.get_environments(&local_runtime_paths).await?, - local_runtime_paths, - provider.default_environment_selection(), - ) + 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) } fn from_provider_environments( environments: HashMap, local_runtime_paths: ExecServerRuntimePaths, - default_selection: DefaultEnvironmentSelection, + default_environment: Option, ) -> Result { for id in environments.keys() { if id.is_empty() { @@ -145,36 +143,21 @@ impl EnvironmentManager { } } - Self::from_environments(environments, local_runtime_paths, default_selection) + Self::from_environments(environments, local_runtime_paths, default_environment) } fn from_environments( environments: HashMap, local_runtime_paths: ExecServerRuntimePaths, - default_selection: DefaultEnvironmentSelection, + default_environment: Option, ) -> Result { - // TODO: Stop deriving a default environment here once omitted - // environment attachment is owned by thread/session setup. - let default_environment = match default_selection { - DefaultEnvironmentSelection::Derived => { - 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 - } - } - DefaultEnvironmentSelection::Environment(environment_id) => { - if !environments.contains_key(&environment_id) { - return Err(ExecServerError::Protocol(format!( - "default environment `{environment_id}` is not configured" - ))); - } - Some(environment_id) - } - DefaultEnvironmentSelection::Disabled => None, - }; + if let Some(environment_id) = default_environment.as_ref() + && !environments.contains_key(environment_id) + { + return Err(ExecServerError::Protocol(format!( + "default environment `{environment_id}` is not configured" + ))); + } let local_environment = Arc::new(Environment::local(local_runtime_paths)); let environments = environments .into_iter() @@ -360,7 +343,6 @@ mod tests { use std::collections::HashMap; use std::sync::Arc; - use super::DefaultEnvironmentSelection; use super::Environment; use super::EnvironmentManager; use super::LOCAL_ENVIRONMENT_ID; @@ -467,7 +449,7 @@ mod tests { .expect("remote environment"), )]), test_runtime_paths(), - DefaultEnvironmentSelection::Derived, + Some(REMOTE_ENVIRONMENT_ID.to_string()), ) .expect("environment manager"); @@ -490,7 +472,7 @@ mod tests { let err = EnvironmentManager::from_provider_environments( HashMap::from([("".to_string(), Environment::default_for_tests())]), test_runtime_paths(), - DefaultEnvironmentSelection::Derived, + /*default_environment*/ None, ) .expect_err("empty id should fail"); @@ -515,7 +497,7 @@ mod tests { ), ]), test_runtime_paths(), - DefaultEnvironmentSelection::Environment("devbox".to_string()), + Some("devbox".to_string()), ) .expect("manager"); @@ -531,7 +513,7 @@ mod tests { Environment::default_for_tests(), )]), test_runtime_paths(), - DefaultEnvironmentSelection::Disabled, + /*default_environment*/ None, ) .expect("manager"); @@ -548,7 +530,7 @@ mod tests { Environment::default_for_tests(), )]), test_runtime_paths(), - DefaultEnvironmentSelection::Environment("missing".to_string()), + 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 eebfff6f13..1f85780622 100644 --- a/codex-rs/exec-server/src/environment_provider.rs +++ b/codex-rs/exec-server/src/environment_provider.rs @@ -22,18 +22,14 @@ pub trait EnvironmentProvider: Send + Sync { local_runtime_paths: &ExecServerRuntimePaths, ) -> Result, ExecServerError>; - fn default_environment_selection(&self) -> DefaultEnvironmentSelection { - DefaultEnvironmentSelection::Derived + fn default_environment_id( + &self, + environments: &HashMap, + ) -> Option { + derived_default_environment_id(environments) } } -#[derive(Clone, Debug, PartialEq, Eq)] -pub enum DefaultEnvironmentSelection { - Derived, - Environment(String), - Disabled, -} - /// Default provider backed by `CODEX_EXEC_SERVER_URL`. #[derive(Clone, Debug)] pub struct DefaultEnvironmentProvider { @@ -81,15 +77,28 @@ impl EnvironmentProvider for DefaultEnvironmentProvider { Ok(self.environments(local_runtime_paths)) } - fn default_environment_selection(&self) -> DefaultEnvironmentSelection { + fn default_environment_id( + &self, + environments: &HashMap, + ) -> Option { if normalize_exec_server_url(self.exec_server_url.clone()).1 { - DefaultEnvironmentSelection::Disabled + None } else { - DefaultEnvironmentSelection::Derived + derived_default_environment_id(environments) } } } +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 + } +} + 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), @@ -154,10 +163,7 @@ mod tests { assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); - assert_eq!( - provider.default_environment_selection(), - DefaultEnvironmentSelection::Disabled - ); + assert_eq!(provider.default_environment_id(&environments), None); } #[tokio::test] diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 1fd1ea9c1d..b36ab39d01 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -42,7 +42,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::DefaultEnvironmentSelection; pub use environment_provider::EnvironmentProvider; pub use fs_helper::CODEX_FS_HELPER_ARG1; pub use fs_helper_main::main as run_fs_helper_main;