Make environment provider snapshots path-free (#21794)

## Summary
- make EnvironmentProvider::snapshot path-free and keep providers
focused on provider-owned remote environments
- let provider snapshots request local inclusion via include_local, with
environments.toml including local and CODEX_EXEC_SERVER_URL excluding
local
- move reserved local environment construction into EnvironmentManager
using ExecServerRuntimePaths

Follow-up to https://github.com/openai/codex/pull/20667

## Testing
- just fmt
- git diff --check
- devbox: bazel build --bes_backend= --bes_results_url=
//codex-rs/exec-server:exec-server
- devbox: bazel test --bes_backend= --bes_results_url=
//codex-rs/exec-server:exec-server-unit-tests

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
starr-openai
2026-05-08 15:30:00 -07:00
committed by GitHub
Unverified
parent 24111790f0
commit dac108f2f1
6 changed files with 187 additions and 206 deletions
+83 -54
View File
@@ -142,10 +142,7 @@ impl EnvironmentManager {
where
P: EnvironmentProvider + ?Sized,
{
Self::from_provider_snapshot(
provider.snapshot(&local_runtime_paths).await?,
local_runtime_paths,
)
Self::from_provider_snapshot(provider.snapshot().await?, local_runtime_paths)
}
fn from_provider_snapshot(
@@ -155,14 +152,28 @@ impl EnvironmentManager {
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let mut environment_map = HashMap::with_capacity(environments.len());
let mut environment_map =
HashMap::with_capacity(environments.len() + usize::from(include_local));
let local_environment = Arc::new(Environment::local(local_runtime_paths));
if include_local {
environment_map.insert(
LOCAL_ENVIRONMENT_ID.to_string(),
Arc::clone(&local_environment),
);
}
for (id, environment) in environments {
if id.is_empty() {
return Err(ExecServerError::Protocol(
"environment id cannot be empty".to_string(),
));
}
if id == LOCAL_ENVIRONMENT_ID {
return Err(ExecServerError::Protocol(format!(
"environment id `{LOCAL_ENVIRONMENT_ID}` is reserved for EnvironmentManager"
)));
}
if environment_map
.insert(id.clone(), Arc::new(environment))
.is_some()
@@ -183,8 +194,6 @@ impl EnvironmentManager {
Some(environment_id)
}
};
let local_environment = Arc::new(Environment::local(local_runtime_paths));
Ok(Self {
default_environment,
environments: environment_map,
@@ -399,10 +408,7 @@ mod tests {
#[async_trait::async_trait]
impl EnvironmentProvider for TestEnvironmentProvider {
async fn snapshot(
&self,
_local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
async fn snapshot(&self) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot.clone())
}
}
@@ -431,14 +437,15 @@ mod tests {
let environment = manager.default_environment().expect("default environment");
assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID));
assert!(!environment.is_remote());
assert!(
!manager
assert!(Arc::ptr_eq(
&environment,
&manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
.is_remote()
);
));
assert!(Arc::ptr_eq(&environment, &manager.local_environment()));
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
assert!(!environment.is_remote());
}
#[tokio::test]
@@ -473,12 +480,7 @@ mod tests {
.get_environment(REMOTE_ENVIRONMENT_ID)
.expect("remote environment")
));
assert!(
!manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
.is_remote()
);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(!manager.local_environment().is_remote());
}
@@ -506,6 +508,7 @@ mod tests {
.expect("remote environment"),
)],
default: EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string()),
include_local: false,
},
};
let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths())
@@ -532,6 +535,7 @@ mod tests {
snapshot: EnvironmentProviderSnapshot {
environments: vec![("".to_string(), Environment::default_for_tests())],
default: EnvironmentDefault::Disabled,
include_local: false,
},
};
let err = EnvironmentManager::from_provider(&provider, test_runtime_paths())
@@ -544,22 +548,39 @@ mod tests {
);
}
#[tokio::test]
async fn environment_manager_rejects_provider_supplied_local_environment() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::default_for_tests(),
)],
default: EnvironmentDefault::Disabled,
include_local: false,
},
};
let err = EnvironmentManager::from_provider(&provider, test_runtime_paths())
.await
.expect_err("local id should fail");
assert_eq!(
err.to_string(),
"exec-server protocol error: environment id `local` is reserved for EnvironmentManager"
);
}
#[tokio::test]
async fn environment_manager_uses_explicit_provider_default() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![
(
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"),
),
],
environments: vec![(
"devbox".to_string(),
Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string()))
.expect("remote environment"),
)],
default: EnvironmentDefault::EnvironmentId("devbox".to_string()),
include_local: true,
},
};
let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths())
@@ -579,10 +600,12 @@ mod tests {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
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: EnvironmentDefault::Disabled,
include_local: true,
},
};
let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths())
@@ -591,7 +614,12 @@ mod tests {
assert_eq!(manager.default_environment_id(), None);
assert!(manager.default_environment().is_none());
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_some());
assert!(Arc::ptr_eq(
&manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment"),
&manager.local_environment()
));
}
#[tokio::test]
@@ -599,10 +627,12 @@ mod tests {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
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: EnvironmentDefault::EnvironmentId("missing".to_string()),
include_local: true,
},
};
let err = EnvironmentManager::from_provider(&provider, test_runtime_paths())
@@ -616,20 +646,23 @@ mod tests {
}
#[tokio::test]
async fn environment_manager_uses_provider_supplied_local_environment() {
async fn environment_manager_includes_local_for_default_provider_without_url() {
let manager = EnvironmentManager::create_for_tests(
/*exec_server_url*/ None,
test_runtime_paths(),
)
.await;
let environment = manager.default_environment().expect("default environment");
assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID));
let provider_local = manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("provider local environment");
assert!(!provider_local.is_remote());
assert!(!manager.local_environment().is_remote());
assert!(!Arc::ptr_eq(&provider_local, &manager.local_environment()));
assert!(Arc::ptr_eq(
&environment,
&manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
));
assert!(Arc::ptr_eq(&environment, &manager.local_environment()));
assert!(!environment.is_remote());
}
#[tokio::test]
@@ -641,7 +674,7 @@ mod tests {
)
.await;
let environment = manager.default_environment().expect("default environment");
let environment = manager.local_environment();
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
let manager = EnvironmentManager::create_for_tests(
@@ -652,7 +685,7 @@ mod tests {
.clone(),
)
.await;
let environment = manager.default_environment().expect("default environment");
let environment = manager.local_environment();
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
}
@@ -665,20 +698,16 @@ mod tests {
}
#[tokio::test]
async fn environment_manager_keeps_default_provider_local_lookup_when_default_disabled() {
async fn environment_manager_omits_default_provider_local_lookup_when_default_disabled() {
let manager =
EnvironmentManager::create_for_tests(Some("none".to_string()), test_runtime_paths())
.await;
assert!(manager.default_environment().is_none());
assert_eq!(manager.default_environment_id(), None);
assert!(
!manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
.is_remote()
);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
assert!(!manager.local_environment().is_remote());
}
#[tokio::test]
@@ -2,7 +2,6 @@ use async_trait::async_trait;
use crate::Environment;
use crate::ExecServerError;
use crate::ExecServerRuntimePaths;
use crate::environment::CODEX_EXEC_SERVER_URL_ENV_VAR;
use crate::environment::LOCAL_ENVIRONMENT_ID;
use crate::environment::REMOTE_ENVIRONMENT_ID;
@@ -11,21 +10,20 @@ use crate::environment::REMOTE_ENVIRONMENT_ID;
///
/// Implementations own a startup snapshot containing both the available
/// environment list in configured order and the default environment
/// selection. Providers that want the local environment to be addressable by
/// id should include it explicitly in the returned list.
/// selection. Providers should only return provider-owned remote environments;
/// `include_local` controls whether `EnvironmentManager` should add the local
/// environment to the snapshot.
#[async_trait]
pub trait EnvironmentProvider: Send + Sync {
/// Returns the provider-owned environment startup snapshot.
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError>;
async fn snapshot(&self) -> Result<EnvironmentProviderSnapshot, ExecServerError>;
}
#[derive(Clone, Debug)]
pub struct EnvironmentProviderSnapshot {
pub environments: Vec<(String, Environment)>,
pub default: EnvironmentDefault,
pub include_local: bool,
}
#[derive(Clone, Debug, PartialEq, Eq)]
@@ -51,26 +49,21 @@ impl DefaultEnvironmentProvider {
Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok())
}
pub(crate) fn snapshot_inner(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> EnvironmentProviderSnapshot {
let mut environments = vec![(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::local(local_runtime_paths.clone()),
)];
pub(crate) fn snapshot_inner(&self) -> EnvironmentProviderSnapshot {
let mut environments = Vec::new();
let (exec_server_url, disabled) = normalize_exec_server_url(self.exec_server_url.clone());
if let Some(exec_server_url) = exec_server_url {
environments.push((
REMOTE_ENVIRONMENT_ID.to_string(),
Environment::remote_inner(exec_server_url, Some(local_runtime_paths.clone())),
Environment::remote_inner(exec_server_url, /*local_runtime_paths*/ None),
));
}
let has_remote = environments
.iter()
.any(|(id, _environment)| id == REMOTE_ENVIRONMENT_ID);
let include_local = !disabled && !has_remote;
let default = if disabled {
EnvironmentDefault::Disabled
} else if has_remote {
@@ -82,17 +75,15 @@ impl DefaultEnvironmentProvider {
EnvironmentProviderSnapshot {
environments,
default,
include_local,
}
}
}
#[async_trait]
impl EnvironmentProvider for DefaultEnvironmentProvider {
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot_inner(local_runtime_paths))
async fn snapshot(&self) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot_inner())
}
}
@@ -111,35 +102,20 @@ mod tests {
use pretty_assertions::assert_eq;
use super::*;
use crate::ExecServerRuntimePaths;
fn test_runtime_paths() -> ExecServerRuntimePaths {
ExecServerRuntimePaths::new(
std::env::current_exe().expect("current exe"),
/*codex_linux_sandbox_exe*/ None,
)
.expect("runtime paths")
}
#[tokio::test]
async fn default_provider_returns_local_environment_when_url_is_missing() {
async fn default_provider_requests_local_environment_when_url_is_missing() {
let provider = DefaultEnvironmentProvider::new(/*exec_server_url*/ None);
let runtime_paths = test_runtime_paths();
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let environments: HashMap<_, _> = environments.into_iter().collect();
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert_eq!(
environments[LOCAL_ENVIRONMENT_ID].local_runtime_paths(),
Some(&runtime_paths)
);
assert!(include_local);
assert!(!environments.contains_key(LOCAL_ENVIRONMENT_ID));
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(
default,
@@ -148,20 +124,18 @@ mod tests {
}
#[tokio::test]
async fn default_provider_returns_local_environment_when_url_is_empty() {
async fn default_provider_requests_local_environment_when_url_is_empty() {
let provider = DefaultEnvironmentProvider::new(Some(String::new()));
let runtime_paths = test_runtime_paths();
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let environments: HashMap<_, _> = environments.into_iter().collect();
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(include_local);
assert!(!environments.contains_key(LOCAL_ENVIRONMENT_ID));
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(
default,
@@ -170,20 +144,18 @@ mod tests {
}
#[tokio::test]
async fn default_provider_returns_local_environment_for_none_value() {
async fn default_provider_omits_local_environment_for_none_value() {
let provider = DefaultEnvironmentProvider::new(Some("none".to_string()));
let runtime_paths = test_runtime_paths();
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let environments: HashMap<_, _> = environments.into_iter().collect();
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!include_local);
assert!(!environments.contains_key(LOCAL_ENVIRONMENT_ID));
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(default, EnvironmentDefault::Disabled);
}
@@ -191,18 +163,16 @@ mod tests {
#[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
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let environments: HashMap<_, _> = environments.into_iter().collect();
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!include_local);
assert!(!environments.contains_key(LOCAL_ENVIRONMENT_ID));
let remote_environment = &environments[REMOTE_ENVIRONMENT_ID];
assert!(remote_environment.is_remote());
assert_eq!(
@@ -218,11 +188,7 @@ mod tests {
#[tokio::test]
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
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let environments: HashMap<_, _> = snapshot.environments.into_iter().collect();
assert_eq!(
+24 -48
View File
@@ -11,7 +11,6 @@ use crate::DefaultEnvironmentProvider;
use crate::Environment;
use crate::EnvironmentProvider;
use crate::ExecServerError;
use crate::ExecServerRuntimePaths;
use crate::client_api::ExecServerTransportParams;
use crate::client_api::StdioExecServerCommand;
use crate::environment::LOCAL_ENVIRONMENT_ID;
@@ -78,21 +77,14 @@ impl TomlEnvironmentProvider {
#[async_trait]
impl EnvironmentProvider for TomlEnvironmentProvider {
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
let mut environments = Vec::with_capacity(self.environments.len() + 1);
environments.push((
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::local(local_runtime_paths.clone()),
));
async fn snapshot(&self) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
let mut environments = Vec::with_capacity(self.environments.len());
for (id, transport_params) in &self.environments {
environments.push((
id.clone(),
Environment::remote_with_transport(
transport_params.clone(),
Some(local_runtime_paths.clone()),
/*local_runtime_paths*/ None,
),
));
}
@@ -100,6 +92,7 @@ impl EnvironmentProvider for TomlEnvironmentProvider {
Ok(EnvironmentProviderSnapshot {
environments,
default: self.default.clone(),
include_local: true,
})
}
}
@@ -292,16 +285,8 @@ mod tests {
use super::*;
fn test_runtime_paths() -> ExecServerRuntimePaths {
ExecServerRuntimePaths::new(
std::env::current_exe().expect("current exe"),
/*codex_linux_sandbox_exe*/ None,
)
.expect("runtime paths")
}
#[tokio::test]
async fn toml_provider_adds_implicit_local_and_configured_environments() {
async fn toml_provider_includes_local_and_adds_configured_environments() {
let provider = TomlEnvironmentProvider::new(EnvironmentsToml {
default: Some("ssh-dev".to_string()),
environments: vec![
@@ -326,27 +311,22 @@ mod tests {
],
})
.expect("provider");
let runtime_paths = test_runtime_paths();
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let EnvironmentProviderSnapshot {
environments,
default,
include_local,
} = snapshot;
let environment_ids: Vec<_> = environments
.iter()
.map(|(id, _environment)| id.as_str())
.collect();
assert_eq!(
environment_ids,
vec![LOCAL_ENVIRONMENT_ID, "devbox", "ssh-dev"]
);
assert_eq!(environment_ids, vec!["devbox", "ssh-dev"]);
let environments: HashMap<_, _> = environments.into_iter().collect();
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(include_local);
assert!(!environments.contains_key(LOCAL_ENVIRONMENT_ID));
assert_eq!(
environments["devbox"].exec_server_url(),
Some("ws://127.0.0.1:8765")
@@ -362,11 +342,9 @@ mod tests {
#[tokio::test]
async fn toml_provider_default_omitted_selects_local() {
let provider = TomlEnvironmentProvider::new(EnvironmentsToml::default()).expect("provider");
let snapshot = provider
.snapshot(&test_runtime_paths())
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
assert!(snapshot.include_local);
assert_eq!(
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
@@ -380,11 +358,9 @@ mod tests {
environments: Vec::new(),
})
.expect("provider");
let snapshot = provider
.snapshot(&test_runtime_paths())
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
assert!(snapshot.include_local);
assert_eq!(snapshot.default, EnvironmentDefault::Disabled);
}
@@ -681,17 +657,15 @@ default = "none"
let provider =
environment_provider_from_codex_home(codex_home.path()).expect("environment provider");
let snapshot = provider
.snapshot(&test_runtime_paths())
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let environment_ids: Vec<_> = snapshot
.environments
.into_iter()
.map(|(id, _environment)| id)
.collect();
assert!(environment_ids.contains(&LOCAL_ENVIRONMENT_ID.to_string()));
assert!(snapshot.include_local);
assert!(!environment_ids.contains(&LOCAL_ENVIRONMENT_ID.to_string()));
assert_eq!(snapshot.default, EnvironmentDefault::Disabled);
}
@@ -702,16 +676,18 @@ default = "none"
let provider =
environment_provider_from_codex_home(codex_home.path()).expect("environment provider");
let snapshot = provider
.snapshot(&test_runtime_paths())
.await
.expect("environments");
let snapshot = provider.snapshot().await.expect("environments");
let environment_ids: Vec<_> = snapshot
.environments
.into_iter()
.map(|(id, _environment)| id)
.collect();
assert!(environment_ids.contains(&LOCAL_ENVIRONMENT_ID.to_string()));
assert!(snapshot.include_local);
assert!(!environment_ids.contains(&LOCAL_ENVIRONMENT_ID.to_string()));
assert_eq!(
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
);
}
}