clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-06-01 19:02:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

Split provider environments from default id

Browse Source 
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 <noreply@openai.com>
This commit is contained in:
starr-openai
2026-05-04 13:03:44 -07:00
parent 7e7d8698c3
commit 9ddbb8b10e
2 changed files with 93 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

117
codex-rs/exec-server/src/environment.rs
View File

@@ -10,7 +10,6 @@ use crate::client::http_client::ReqwestHttpClient;
use crate::client_api::ExecServerTransportParams; use crate::client_api::ExecServerTransportParams;
use crate::environment_provider::DefaultEnvironmentProvider; use crate::environment_provider::DefaultEnvironmentProvider;
use crate::environment_provider::EnvironmentProvider; use crate::environment_provider::EnvironmentProvider;
use crate::environment_provider::EnvironmentProviderSnapshot;
use crate::environment_provider::normalize_exec_server_url; use crate::environment_provider::normalize_exec_server_url;
use crate::local_file_system::LocalFileSystem; use crate::local_file_system::LocalFileSystem;
use crate::local_process::LocalProcess; 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 /// `EnvironmentManager` is a shared registry for concrete environments. Its
/// default constructor preserves the legacy `CODEX_EXEC_SERVER_URL` behavior /// 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 /// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving
/// the default environment unset while still keeping an explicit local /// 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. /// Builds a test-only manager with environment access disabled.
pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self { pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self {
let snapshot = EnvironmentProviderSnapshot { match Self::from_provider_parts(HashMap::new(), None, local_runtime_paths) {
environments: HashMap::new(),
default_environment_id: None,
};
match Self::from_provider_snapshot(snapshot, local_runtime_paths) {
Ok(manager) => manager, Ok(manager) => manager,
Err(err) => panic!("disabled test environment manager: {err}"), Err(err) => panic!("disabled test environment manager: {err}"),
} }
@@ -106,14 +102,17 @@ impl EnvironmentManager {
local_runtime_paths: ExecServerRuntimePaths, local_runtime_paths: ExecServerRuntimePaths,
) -> Self { ) -> Self {
let provider = DefaultEnvironmentProvider::new(exec_server_url); let provider = DefaultEnvironmentProvider::new(exec_server_url);
let snapshot = provider.snapshot(&local_runtime_paths); match Self::from_provider_parts(
match Self::from_provider_snapshot(snapshot, local_runtime_paths) { provider.environments(&local_runtime_paths),
provider.default_id(),
local_runtime_paths,
) {
Ok(manager) => manager, Ok(manager) => manager,
Err(err) => panic!("default provider should create valid environments: {err}"), 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<P>( pub async fn from_provider<P>(
provider: &P, provider: &P,
local_runtime_paths: ExecServerRuntimePaths, local_runtime_paths: ExecServerRuntimePaths,
@@ -121,20 +120,16 @@ impl EnvironmentManager {
where where
P: EnvironmentProvider + ?Sized, P: EnvironmentProvider + ?Sized,
{ {
let snapshot = provider let environments = provider.get_environments(&local_runtime_paths).await?;
.get_environment_snapshot(&local_runtime_paths) let default_environment_id = provider.default_environment_id();
.await?; Self::from_provider_parts(environments, default_environment_id, local_runtime_paths)
Self::from_provider_snapshot(snapshot, local_runtime_paths)
} }
fn from_provider_snapshot( fn from_provider_parts(
snapshot: EnvironmentProviderSnapshot, environments: HashMap<String, Environment>,
default_environment_id: Option<String>,
local_runtime_paths: ExecServerRuntimePaths, local_runtime_paths: ExecServerRuntimePaths,
) -> Result<Self, ExecServerError> { ) -> Result<Self, ExecServerError> {
let EnvironmentProviderSnapshot {
environments,
default_environment_id,
} = snapshot;
for id in environments.keys() { for id in environments.keys() {
if id.is_empty() { if id.is_empty() {
return Err(ExecServerError::Protocol( return Err(ExecServerError::Protocol(
@@ -433,16 +428,14 @@ mod tests {
} }
#[tokio::test] #[tokio::test]
async fn environment_manager_builds_from_provider_snapshot() { async fn environment_manager_builds_from_provider_parts() {
let manager = EnvironmentManager::from_provider_snapshot( let manager = EnvironmentManager::from_provider_parts(
EnvironmentProviderSnapshot { HashMap::from([(
environments: HashMap::from([( REMOTE_ENVIRONMENT_ID.to_string(),
REMOTE_ENVIRONMENT_ID.to_string(), Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string()))
Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) .expect("remote environment"),
.expect("remote environment"), )]),
)]), Some(REMOTE_ENVIRONMENT_ID.to_string()),
default_environment_id: Some(REMOTE_ENVIRONMENT_ID.to_string()),
},
test_runtime_paths(), test_runtime_paths(),
) )
.expect("environment manager"); .expect("environment manager");
@@ -463,11 +456,9 @@ mod tests {
#[tokio::test] #[tokio::test]
async fn environment_manager_rejects_empty_environment_id() { async fn environment_manager_rejects_empty_environment_id() {
let err = EnvironmentManager::from_provider_snapshot( let err = EnvironmentManager::from_provider_parts(
EnvironmentProviderSnapshot { HashMap::from([("".to_string(), Environment::default_for_tests())]),
environments: HashMap::from([("".to_string(), Environment::default_for_tests())]), None,
default_environment_id: None,
},
test_runtime_paths(), test_runtime_paths(),
) )
.expect_err("empty id should fail"); .expect_err("empty id should fail");
@@ -480,21 +471,19 @@ mod tests {
#[tokio::test] #[tokio::test]
async fn environment_manager_uses_explicit_provider_default() { async fn environment_manager_uses_explicit_provider_default() {
let manager = EnvironmentManager::from_provider_snapshot( let manager = EnvironmentManager::from_provider_parts(
EnvironmentProviderSnapshot { HashMap::from([
environments: HashMap::from([ (
( LOCAL_ENVIRONMENT_ID.to_string(),
LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(),
Environment::default_for_tests(), ),
), (
( "devbox".to_string(),
"devbox".to_string(), Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string()))
Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) .expect("remote environment"),
.expect("remote environment"), ),
), ]),
]), Some("devbox".to_string()),
default_environment_id: Some("devbox".to_string()),
},
test_runtime_paths(), test_runtime_paths(),
) )
.expect("manager"); .expect("manager");
@@ -505,14 +494,12 @@ mod tests {
#[tokio::test] #[tokio::test]
async fn environment_manager_disables_provider_default() { async fn environment_manager_disables_provider_default() {
let manager = EnvironmentManager::from_provider_snapshot( let manager = EnvironmentManager::from_provider_parts(
EnvironmentProviderSnapshot { HashMap::from([(
environments: HashMap::from([( LOCAL_ENVIRONMENT_ID.to_string(),
LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(),
Environment::default_for_tests(), )]),
)]), None,
default_environment_id: None,
},
test_runtime_paths(), test_runtime_paths(),
) )
.expect("manager"); .expect("manager");
@@ -524,14 +511,12 @@ mod tests {
#[tokio::test] #[tokio::test]
async fn environment_manager_rejects_unknown_provider_default() { async fn environment_manager_rejects_unknown_provider_default() {
let err = EnvironmentManager::from_provider_snapshot( let err = EnvironmentManager::from_provider_parts(
EnvironmentProviderSnapshot { HashMap::from([(
environments: HashMap::from([( LOCAL_ENVIRONMENT_ID.to_string(),
LOCAL_ENVIRONMENT_ID.to_string(), Environment::default_for_tests(),
Environment::default_for_tests(), )]),
)]), Some("missing".to_string()),
default_environment_id: Some("missing".to_string()),
},
test_runtime_paths(), test_runtime_paths(),
) )
.expect_err("unknown default should fail"); .expect_err("unknown default should fail");

94
codex-rs/exec-server/src/environment_provider.rs
View File

@@ -11,21 +11,20 @@ use crate::environment::REMOTE_ENVIRONMENT_ID;
/// Lists the concrete environments available to Codex. /// Lists the concrete environments available to Codex.
/// ///
/// Implementations should return the provider-owned startup snapshot that /// Implementations own both the available environment list and the default
/// `EnvironmentManager` will cache. Providers that want the local environment to /// environment id. Providers that want the local environment to be addressable
/// be addressable by id should include it explicitly in the returned map. /// by id should include it explicitly in the returned map.
#[async_trait] #[async_trait]
pub trait EnvironmentProvider: Send + Sync { pub trait EnvironmentProvider: Send + Sync {
/// Returns the environments available for a new manager. /// Returns the environments available for a new manager.
async fn get_environment_snapshot( async fn get_environments(
&self, &self,
local_runtime_paths: &ExecServerRuntimePaths, local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError>; ) -> Result<HashMap<String, Environment>, ExecServerError>;
}
pub struct EnvironmentProviderSnapshot { /// Returns the provider-selected default environment id, or `None` to
pub environments: HashMap<String, Environment>, /// disable model-facing environment access.
pub default_environment_id: Option<String>, fn default_environment_id(&self) -> Option<String>;
} }
/// Default provider backed by `CODEX_EXEC_SERVER_URL`. /// 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()) Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok())
} }
pub(crate) fn snapshot( pub(crate) fn environments(
&self, &self,
local_runtime_paths: &ExecServerRuntimePaths, local_runtime_paths: &ExecServerRuntimePaths,
) -> EnvironmentProviderSnapshot { ) -> HashMap<String, Environment> {
let mut environments = HashMap::from([( let mut environments = HashMap::from([(
LOCAL_ENVIRONMENT_ID.to_string(), LOCAL_ENVIRONMENT_ID.to_string(),
Environment::local(local_runtime_paths.clone()), 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 { if let Some(exec_server_url) = exec_server_url {
environments.insert( environments.insert(
@@ -62,36 +61,32 @@ impl DefaultEnvironmentProvider {
); );
} }
let default_environment_id = if disabled { environments
None }
} else {
derived_default_environment_id(&environments)
};
EnvironmentProviderSnapshot { pub(crate) fn default_id(&self) -> Option<String> {
environments, let (exec_server_url, disabled) = normalize_exec_server_url(self.exec_server_url.clone());
default_environment_id, 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] #[async_trait]
impl EnvironmentProvider for DefaultEnvironmentProvider { impl EnvironmentProvider for DefaultEnvironmentProvider {
async fn get_environment_snapshot( async fn get_environments(
&self, &self,
local_runtime_paths: &ExecServerRuntimePaths, local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError> { ) -> Result<HashMap<String, Environment>, ExecServerError> {
Ok(self.snapshot(local_runtime_paths)) Ok(self.environments(local_runtime_paths))
} }
}
fn derived_default_environment_id(environments: &HashMap<String, Environment>) -> Option<String> { fn default_environment_id(&self) -> Option<String> {
if environments.contains_key(REMOTE_ENVIRONMENT_ID) { self.default_id()
Some(REMOTE_ENVIRONMENT_ID.to_string())
} else if environments.contains_key(LOCAL_ENVIRONMENT_ID) {
Some(LOCAL_ENVIRONMENT_ID.to_string())
} else {
None
} }
} }
@@ -122,11 +117,10 @@ mod tests {
async fn default_provider_returns_local_environment_when_url_is_missing() { async fn default_provider_returns_local_environment_when_url_is_missing() {
let provider = DefaultEnvironmentProvider::new(/*exec_server_url*/ None); let provider = DefaultEnvironmentProvider::new(/*exec_server_url*/ None);
let runtime_paths = test_runtime_paths(); let runtime_paths = test_runtime_paths();
let snapshot = provider let environments = provider
.get_environment_snapshot(&runtime_paths) .get_environments(&runtime_paths)
.await .await
.expect("environment snapshot"); .expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert_eq!( assert_eq!(
@@ -140,11 +134,10 @@ mod tests {
async fn default_provider_returns_local_environment_when_url_is_empty() { async fn default_provider_returns_local_environment_when_url_is_empty() {
let provider = DefaultEnvironmentProvider::new(Some(String::new())); let provider = DefaultEnvironmentProvider::new(Some(String::new()));
let runtime_paths = test_runtime_paths(); let runtime_paths = test_runtime_paths();
let snapshot = provider let environments = provider
.get_environment_snapshot(&runtime_paths) .get_environments(&runtime_paths)
.await .await
.expect("environment snapshot"); .expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
@@ -154,26 +147,24 @@ mod tests {
async fn default_provider_returns_local_environment_for_none_value() { async fn default_provider_returns_local_environment_for_none_value() {
let provider = DefaultEnvironmentProvider::new(Some("none".to_string())); let provider = DefaultEnvironmentProvider::new(Some("none".to_string()));
let runtime_paths = test_runtime_paths(); let runtime_paths = test_runtime_paths();
let snapshot = provider let environments = provider
.get_environment_snapshot(&runtime_paths) .get_environments(&runtime_paths)
.await .await
.expect("environment snapshot"); .expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID)); assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(snapshot.default_environment_id, None); assert_eq!(provider.default_environment_id(), None);
} }
#[tokio::test] #[tokio::test]
async fn default_provider_adds_remote_environment_for_websocket_url() { async fn default_provider_adds_remote_environment_for_websocket_url() {
let provider = DefaultEnvironmentProvider::new(Some("ws://127.0.0.1:8765".to_string())); let provider = DefaultEnvironmentProvider::new(Some("ws://127.0.0.1:8765".to_string()));
let runtime_paths = test_runtime_paths(); let runtime_paths = test_runtime_paths();
let snapshot = provider let environments = provider
.get_environment_snapshot(&runtime_paths) .get_environments(&runtime_paths)
.await .await
.expect("environment snapshot"); .expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote()); assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
let remote_environment = &environments[REMOTE_ENVIRONMENT_ID]; let remote_environment = &environments[REMOTE_ENVIRONMENT_ID];
@@ -188,11 +179,10 @@ mod tests {
async fn default_provider_normalizes_exec_server_url() { async fn default_provider_normalizes_exec_server_url() {
let provider = DefaultEnvironmentProvider::new(Some(" ws://127.0.0.1:8765 ".to_string())); let provider = DefaultEnvironmentProvider::new(Some(" ws://127.0.0.1:8765 ".to_string()));
let runtime_paths = test_runtime_paths(); let runtime_paths = test_runtime_paths();
let snapshot = provider let environments = provider
.get_environment_snapshot(&runtime_paths) .get_environments(&runtime_paths)
.await .await
.expect("environment snapshot"); .expect("environments");
let environments = snapshot.environments;
assert_eq!( assert_eq!(
environments[REMOTE_ENVIRONMENT_ID].exec_server_url(), environments[REMOTE_ENVIRONMENT_ID].exec_server_url(),