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

Represent provider defaults with snapshots

Browse Source 
Keep EnvironmentManager construction async to preserve caller behavior while moving provider-owned default selection into a single snapshot object.

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
starr-openai
2026-05-06 12:04:08 -07:00
parent a2ef8e05b5
commit dc926a56c7
12 changed files with 228 additions and 159 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -9,7 +9,9 @@ use crate::client::LazyRemoteExecServerClient;
use crate::client::http_client::ReqwestHttpClient;
use crate::client_api::ExecServerTransportParams;
use crate::environment_provider::DefaultEnvironmentProvider;
use crate::environment_provider::EnvironmentDefault;
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;
@@ -23,8 +25,7 @@ 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 environment
/// list and default id.
/// while provider-based construction accepts a provider-supplied snapshot.
///
/// Setting `CODEX_EXEC_SERVER_URL=none` disables environment access by leaving
/// the default environment unset while still keeping an explicit local
@@ -81,44 +82,56 @@ impl EnvironmentManager {
}
/// Builds a test-only manager from a raw exec-server URL value.
pub fn create_for_tests(
pub async fn create_for_tests(
exec_server_url: Option<String>,
local_runtime_paths: ExecServerRuntimePaths,
) -> Self {
Self::from_default_provider_url(exec_server_url, local_runtime_paths)
Self::from_default_provider_url(exec_server_url, local_runtime_paths).await
}
/// Builds a manager from `CODEX_EXEC_SERVER_URL` and local runtime paths
/// used when creating local filesystem helpers.
pub fn new(args: EnvironmentManagerArgs) -> Self {
pub async fn new(args: EnvironmentManagerArgs) -> Self {
let EnvironmentManagerArgs {
local_runtime_paths,
} = args;
let exec_server_url = std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok();
Self::from_default_provider_url(exec_server_url, local_runtime_paths)
Self::from_default_provider_url(exec_server_url, local_runtime_paths).await
}
fn from_default_provider_url(
async fn from_default_provider_url(
exec_server_url: Option<String>,
local_runtime_paths: ExecServerRuntimePaths,
) -> Self {
let provider = DefaultEnvironmentProvider::new(exec_server_url);
match Self::from_provider(&provider, 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}"),
}
}
/// Builds a manager from a provider-supplied environment list and default.
pub fn from_provider<P>(
/// Builds a manager from a provider-supplied startup snapshot.
pub async fn from_provider<P>(
provider: &P,
local_runtime_paths: ExecServerRuntimePaths,
) -> Result<Self, ExecServerError>
where
P: EnvironmentProvider + ?Sized,
{
let environments = provider.get_environments(&local_runtime_paths)?;
let default_environment_id = provider.default_environment_id();
Self::from_provider_snapshot(
provider.snapshot(&local_runtime_paths).await?,
local_runtime_paths,
)
}
fn from_provider_snapshot(
snapshot: EnvironmentProviderSnapshot,
local_runtime_paths: ExecServerRuntimePaths,
) -> Result<Self, ExecServerError> {
let EnvironmentProviderSnapshot {
environments,
default,
} = snapshot;
for id in environments.keys() {
if id.is_empty() {
return Err(ExecServerError::Protocol(
@@ -127,13 +140,17 @@ impl EnvironmentManager {
}
}
if let Some(environment_id) = default_environment_id.as_ref()
&& !environments.contains_key(environment_id)
{
return Err(ExecServerError::Protocol(format!(
"default environment `{environment_id}` is not configured"
)));
}
let default_environment = match default {
EnvironmentDefault::Disabled => None,
EnvironmentDefault::EnvironmentId(environment_id) => {
if !environments.contains_key(&environment_id) {
return Err(ExecServerError::Protocol(format!(
"default environment `{environment_id}` is not configured"
)));
}
Some(environment_id)
}
};
let local_environment = Arc::new(Environment::local(local_runtime_paths));
let environments = environments
.into_iter()
@@ -141,7 +158,7 @@ impl EnvironmentManager {
.collect();
Ok(Self {
default_environment: default_environment_id,
default_environment,
environments,
local_environment,
})
@@ -311,23 +328,21 @@ mod tests {
use crate::ExecServerError;
use crate::ExecServerRuntimePaths;
use crate::ProcessId;
use crate::environment_provider::EnvironmentDefault;
use crate::environment_provider::EnvironmentProviderSnapshot;
use pretty_assertions::assert_eq;
struct TestEnvironmentProvider {
environments: HashMap<String, Environment>,
default_environment_id: Option<String>,
snapshot: EnvironmentProviderSnapshot,
}
#[async_trait::async_trait]
impl EnvironmentProvider for TestEnvironmentProvider {
fn get_environments(
async fn snapshot(
&self,
_local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<HashMap<String, Environment>, ExecServerError> {
Ok(self.environments.clone())
}
fn default_environment_id(&self) -> Option<String> {
self.default_environment_id.clone()
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot.clone())
}
}
@@ -351,7 +366,7 @@ mod tests {
#[tokio::test]
async fn environment_manager_normalizes_empty_url() {
let manager =
EnvironmentManager::create_for_tests(Some(String::new()), test_runtime_paths());
EnvironmentManager::create_for_tests(Some(String::new()), test_runtime_paths()).await;
let environment = manager.default_environment().expect("default environment");
assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID));
@@ -381,7 +396,8 @@ mod tests {
let manager = EnvironmentManager::create_for_tests(
Some("ws://127.0.0.1:8765".to_string()),
test_runtime_paths(),
);
)
.await;
let environment = manager.default_environment().expect("default environment");
assert_eq!(
@@ -422,14 +438,17 @@ mod tests {
#[tokio::test]
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"),
)]),
default_environment_id: Some(REMOTE_ENVIRONMENT_ID.to_string()),
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: EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string()),
},
};
let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths())
.await
.expect("environment manager");
assert_eq!(
@@ -449,10 +468,13 @@ mod tests {
#[tokio::test]
async fn environment_manager_rejects_empty_environment_id() {
let provider = TestEnvironmentProvider {
environments: HashMap::from([("".to_string(), Environment::default_for_tests())]),
default_environment_id: None,
snapshot: EnvironmentProviderSnapshot {
environments: HashMap::from([("".to_string(), Environment::default_for_tests())]),
default: EnvironmentDefault::Disabled,
},
};
let err = EnvironmentManager::from_provider(&provider, test_runtime_paths())
.await
.expect_err("empty id should fail");
assert_eq!(
@@ -464,21 +486,24 @@ mod tests {
#[tokio::test]
async fn environment_manager_uses_explicit_provider_default() {
let provider = TestEnvironmentProvider {
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()),
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: EnvironmentDefault::EnvironmentId("devbox".to_string()),
},
};
let manager =
EnvironmentManager::from_provider(&provider, test_runtime_paths()).expect("manager");
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());
@@ -487,14 +512,17 @@ mod tests {
#[tokio::test]
async fn environment_manager_disables_provider_default() {
let provider = TestEnvironmentProvider {
environments: HashMap::from([(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::default_for_tests(),
)]),
default_environment_id: None,
snapshot: EnvironmentProviderSnapshot {
environments: HashMap::from([(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::default_for_tests(),
)]),
default: EnvironmentDefault::Disabled,
},
};
let manager =
EnvironmentManager::from_provider(&provider, test_runtime_paths()).expect("manager");
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());
@@ -504,13 +532,16 @@ mod tests {
#[tokio::test]
async fn environment_manager_rejects_unknown_provider_default() {
let provider = TestEnvironmentProvider {
environments: HashMap::from([(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::default_for_tests(),
)]),
default_environment_id: Some("missing".to_string()),
snapshot: EnvironmentProviderSnapshot {
environments: HashMap::from([(
LOCAL_ENVIRONMENT_ID.to_string(),
Environment::default_for_tests(),
)]),
default: EnvironmentDefault::EnvironmentId("missing".to_string()),
},
};
let err = EnvironmentManager::from_provider(&provider, test_runtime_paths())
.await
.expect_err("unknown default should fail");
assert_eq!(
@@ -524,7 +555,8 @@ mod tests {
let manager = EnvironmentManager::create_for_tests(
/*exec_server_url*/ None,
test_runtime_paths(),
);
)
.await;
assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID));
let provider_local = manager
@@ -541,7 +573,8 @@ mod tests {
let manager = EnvironmentManager::create_for_tests(
/*exec_server_url*/ None,
runtime_paths.clone(),
);
)
.await;
let environment = manager.default_environment().expect("default environment");
@@ -552,7 +585,8 @@ mod tests {
.local_runtime_paths()
.expect("local runtime paths")
.clone(),
);
)
.await;
let environment = manager.default_environment().expect("default environment");
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
}
@@ -568,7 +602,8 @@ mod tests {
#[tokio::test]
async fn environment_manager_keeps_default_provider_local_lookup_when_default_disabled() {
let manager =
EnvironmentManager::create_for_tests(Some("none".to_string()), test_runtime_paths());
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);

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

@@ -1,5 +1,7 @@
use std::collections::HashMap;
use async_trait::async_trait;
use crate::Environment;
use crate::ExecServerError;
use crate::ExecServerRuntimePaths;
@@ -9,19 +11,29 @@ use crate::environment::REMOTE_ENVIRONMENT_ID;
/// Lists the concrete environments available to Codex.
///
/// 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.
/// Implementations own a startup snapshot containing both the available
/// environment list and default environment selection. 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.
fn get_environments(
/// Returns the provider-owned environment startup snapshot.
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<HashMap<String, Environment>, ExecServerError>;
) -> Result<EnvironmentProviderSnapshot, ExecServerError>;
}
/// Returns the provider-selected default environment id, or `None` to
/// disable model-facing environment access.
fn default_environment_id(&self) -> Option<String>;
#[derive(Clone, Debug)]
pub struct EnvironmentProviderSnapshot {
pub environments: HashMap<String, Environment>,
pub default: EnvironmentDefault,
}
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum EnvironmentDefault {
Disabled,
EnvironmentId(String),
}
/// Default provider backed by `CODEX_EXEC_SERVER_URL`.
@@ -41,15 +53,15 @@ impl DefaultEnvironmentProvider {
Self::new(std::env::var(CODEX_EXEC_SERVER_URL_ENV_VAR).ok())
}
pub(crate) fn environments(
pub(crate) fn snapshot_inner(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> HashMap<String, Environment> {
) -> EnvironmentProviderSnapshot {
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(
@@ -58,32 +70,28 @@ impl DefaultEnvironmentProvider {
);
}
environments
let default = if disabled {
EnvironmentDefault::Disabled
} else if environments.contains_key(REMOTE_ENVIRONMENT_ID) {
EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string())
} else {
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
};
EnvironmentProviderSnapshot {
environments,
default,
}
}
}
#[async_trait]
impl EnvironmentProvider for DefaultEnvironmentProvider {
fn get_environments(
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<HashMap<String, Environment>, ExecServerError> {
Ok(self.environments(local_runtime_paths))
}
fn default_environment_id(&self) -> Option<String> {
let (exec_server_url, disabled) = normalize_exec_server_url(self.exec_server_url.clone());
if disabled {
return None;
}
Some(
if exec_server_url.is_some() {
REMOTE_ENVIRONMENT_ID
} else {
LOCAL_ENVIRONMENT_ID
}
.to_string(),
)
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot_inner(local_runtime_paths))
}
}
@@ -114,9 +122,11 @@ 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 environments = provider
.get_environments(&runtime_paths)
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert_eq!(
@@ -125,8 +135,8 @@ mod tests {
);
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(
provider.default_environment_id(),
Some(LOCAL_ENVIRONMENT_ID.to_string())
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
);
}
@@ -134,15 +144,17 @@ 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 environments = provider
.get_environments(&runtime_paths)
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(
provider.default_environment_id(),
Some(LOCAL_ENVIRONMENT_ID.to_string())
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
);
}
@@ -150,22 +162,26 @@ 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 environments = provider
.get_environments(&runtime_paths)
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(provider.default_environment_id(), None);
assert_eq!(snapshot.default, EnvironmentDefault::Disabled);
}
#[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 environments = provider
.get_environments(&runtime_paths)
let snapshot = provider
.snapshot(&runtime_paths)
.await
.expect("environments");
let environments = snapshot.environments;
assert!(!environments[LOCAL_ENVIRONMENT_ID].is_remote());
let remote_environment = &environments[REMOTE_ENVIRONMENT_ID];
@@ -175,8 +191,8 @@ mod tests {
Some("ws://127.0.0.1:8765")
);
assert_eq!(
provider.default_environment_id(),
Some(REMOTE_ENVIRONMENT_ID.to_string())
snapshot.default,
EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string())
);
}
@@ -185,11 +201,12 @@ mod tests {
let provider = DefaultEnvironmentProvider::new(Some(" ws://127.0.0.1:8765 ".to_string()));
let runtime_paths = test_runtime_paths();
let environments = provider
.get_environments(&runtime_paths)
.snapshot(&runtime_paths)
.await
.expect("environments");
assert_eq!(
environments[REMOTE_ENVIRONMENT_ID].exec_server_url(),
environments.environments[REMOTE_ENVIRONMENT_ID].exec_server_url(),
Some("ws://127.0.0.1:8765")
);
}