agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

Make local environment optional in EnvironmentManager (#23369)

Browse Source 
## Summary
- make `EnvironmentManager` local environment/runtime paths optional
- simplify constructor surface around snapshot materialization
- rename local env accessors to `require_local_environment` /
`try_local_environment`

## Validation
- devbox Bazel build for touched crate surfaces
- `//codex-rs/exec-server:exec-server-unit-tests`
- `//codex-rs/app-server-client:app-server-client-unit-tests`
- filtered touched `//codex-rs/core:core-unit-tests` cases
This commit is contained in:
starr-openai
2026-05-19 12:55:34 -07:00
committed by GitHub
parent 7f4d7ae3a4
commit 5c43a64e2b
26 changed files with 853 additions and 298 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/exec-server/src/environment.rs
+153 -176
View File
@@ -27,12 +27,11 @@ 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 snapshot.
/// while configured 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
/// environment available through `local_environment()`. Callers use
/// `default_environment().is_some()` as the signal for model-facing
/// the default environment unset and omitting the local environment. Callers
/// use `default_environment().is_some()` as the signal for model-facing
/// shell/filesystem tool availability.
///
/// Remote environments create remote filesystem and execution backends that
@@ -42,25 +41,13 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL";
pub struct EnvironmentManager {
default_environment: Option<String>,
environments: RwLock<HashMap<String, Arc<Environment>>>,
local_environment: Arc<Environment>,
local_environment: Option<Arc<Environment>>,
local_runtime_paths: Option<ExecServerRuntimePaths>,
}
pub const LOCAL_ENVIRONMENT_ID: &str = "local";
pub const REMOTE_ENVIRONMENT_ID: &str = "remote";
#[derive(Clone, Debug)]
pub struct EnvironmentManagerArgs {
pub local_runtime_paths: ExecServerRuntimePaths,
}
impl EnvironmentManagerArgs {
pub fn new(local_runtime_paths: ExecServerRuntimePaths) -> Self {
Self {
local_runtime_paths,
}
}
}
impl EnvironmentManager {
/// Builds a test-only manager without configured sandbox helper paths.
pub fn default_for_tests() -> Self {
@@ -70,37 +57,29 @@ impl EnvironmentManager {
LOCAL_ENVIRONMENT_ID.to_string(),
Arc::new(Environment::default_for_tests()),
)])),
local_environment: Arc::new(Environment::default_for_tests()),
local_environment: Some(Arc::new(Environment::default_for_tests())),
local_runtime_paths: None,
}
}
/// Builds a test-only manager with environment access disabled.
pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self {
/// Builds a manager with no configured execution environments.
pub fn without_environments() -> Self {
Self {
default_environment: None,
environments: RwLock::new(HashMap::new()),
local_environment: Arc::new(Environment::local(local_runtime_paths)),
local_environment: None,
local_runtime_paths: None,
}
}
/// Builds a test-only manager from a raw exec-server URL value.
pub async fn create_for_tests(
exec_server_url: Option<String>,
local_runtime_paths: ExecServerRuntimePaths,
local_runtime_paths: Option<ExecServerRuntimePaths>,
) -> Self {
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 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).await
}
/// Builds a manager from `CODEX_HOME` and local runtime paths used when
/// creating local filesystem helpers.
///
@@ -109,27 +88,27 @@ impl EnvironmentManager {
/// `CODEX_EXEC_SERVER_URL` behavior.
pub async fn from_codex_home(
codex_home: impl AsRef<std::path::Path>,
local_runtime_paths: ExecServerRuntimePaths,
local_runtime_paths: Option<ExecServerRuntimePaths>,
) -> Result<Self, ExecServerError> {
let provider = environment_provider_from_codex_home(codex_home.as_ref())?;
Self::from_provider(provider.as_ref(), local_runtime_paths).await
Self::from_snapshot(provider.snapshot().await?, local_runtime_paths)
}
/// Builds a manager from the legacy environment-variable provider without
/// reading user config files from `CODEX_HOME`.
pub async fn from_env(
local_runtime_paths: ExecServerRuntimePaths,
local_runtime_paths: Option<ExecServerRuntimePaths>,
) -> Result<Self, ExecServerError> {
let provider = DefaultEnvironmentProvider::from_env();
Self::from_provider(&provider, local_runtime_paths).await
Self::from_snapshot(provider.snapshot().await?, local_runtime_paths)
}
async fn from_default_provider_url(
exec_server_url: Option<String>,
local_runtime_paths: ExecServerRuntimePaths,
local_runtime_paths: Option<ExecServerRuntimePaths>,
) -> Self {
let provider = DefaultEnvironmentProvider::new(exec_server_url);
match Self::from_provider(&provider, local_runtime_paths).await {
match Self::from_snapshot(provider.snapshot_inner(), local_runtime_paths) {
Ok(manager) => manager,
Err(err) => panic!("default provider should create valid environments: {err}"),
}
@@ -143,26 +122,15 @@ impl EnvironmentManager {
) -> Self {
let mut snapshot = DefaultEnvironmentProvider::new(exec_server_url).snapshot_inner();
snapshot.include_local = true;
match Self::from_provider_snapshot(snapshot, local_runtime_paths) {
match Self::from_snapshot(snapshot, Some(local_runtime_paths)) {
Ok(manager) => manager,
Err(err) => panic!("test provider with local should create valid environments: {err}"),
}
}
/// 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,
{
Self::from_provider_snapshot(provider.snapshot().await?, local_runtime_paths)
}
fn from_provider_snapshot(
fn from_snapshot(
snapshot: EnvironmentProviderSnapshot,
local_runtime_paths: ExecServerRuntimePaths,
local_runtime_paths: Option<ExecServerRuntimePaths>,
) -> Result<Self, ExecServerError> {
let EnvironmentProviderSnapshot {
environments,
@@ -171,13 +139,21 @@ impl EnvironmentManager {
} = snapshot;
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 {
let local_environment = if include_local {
let local_runtime_paths = local_runtime_paths.clone().ok_or_else(|| {
ExecServerError::Protocol(
"local environment requires configured runtime paths".to_string(),
)
})?;
let local_environment = Arc::new(Environment::local(local_runtime_paths));
environment_map.insert(
LOCAL_ENVIRONMENT_ID.to_string(),
Arc::clone(&local_environment),
);
}
Some(local_environment)
} else {
None
};
for (id, environment) in environments {
if id.is_empty() {
return Err(ExecServerError::Protocol(
@@ -213,6 +189,7 @@ impl EnvironmentManager {
default_environment,
environments: RwLock::new(environment_map),
local_environment,
local_runtime_paths,
})
}
@@ -248,9 +225,15 @@ impl EnvironmentManager {
environment_ids
}
/// Returns the local environment instance used for internal runtime work.
pub fn local_environment(&self) -> Arc<Environment> {
Arc::clone(&self.local_environment)
/// Returns the local environment instance when one is configured.
pub fn try_local_environment(&self) -> Option<Arc<Environment>> {
self.local_environment.as_ref().map(Arc::clone)
}
/// Returns the default environment or local environment when either exists.
pub fn default_or_local_environment(&self) -> Option<Arc<Environment>> {
self.default_environment()
.or_else(|| self.try_local_environment())
}
/// Returns a named environment instance.
@@ -285,10 +268,8 @@ impl EnvironmentManager {
"remote environment requires an exec-server url".to_string(),
));
};
let environment = Environment::remote_inner(
exec_server_url,
self.local_environment.local_runtime_paths.clone(),
);
let environment =
Environment::remote_inner(exec_server_url, self.local_runtime_paths.clone());
self.environments
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner)
@@ -452,25 +433,12 @@ 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 crate::environment_provider::EnvironmentDefault;
use crate::environment_provider::EnvironmentProviderSnapshot;
use pretty_assertions::assert_eq;
struct TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot,
}
#[async_trait::async_trait]
impl EnvironmentProvider for TestEnvironmentProvider {
async fn snapshot(&self) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot.clone())
}
}
fn test_runtime_paths() -> ExecServerRuntimePaths {
ExecServerRuntimePaths::new(
std::env::current_exe().expect("current exe"),
@@ -479,6 +447,10 @@ mod tests {
.expect("runtime paths")
}
fn assert_local_environment_unavailable(manager: &EnvironmentManager) {
assert!(manager.try_local_environment().is_none());
}
#[tokio::test]
async fn create_local_environment_does_not_connect() {
let environment = Environment::create(/*exec_server_url*/ None, test_runtime_paths())
@@ -491,7 +463,8 @@ mod tests {
#[tokio::test]
async fn environment_manager_normalizes_empty_url() {
let manager =
EnvironmentManager::create_for_tests(Some(String::new()), test_runtime_paths()).await;
EnvironmentManager::create_for_tests(Some(String::new()), Some(test_runtime_paths()))
.await;
let environment = manager.default_environment().expect("default environment");
assert_eq!(manager.default_environment_id(), Some(LOCAL_ENVIRONMENT_ID));
@@ -501,18 +474,22 @@ mod tests {
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
));
assert!(Arc::ptr_eq(&environment, &manager.local_environment()));
assert!(Arc::ptr_eq(
&environment,
&manager.try_local_environment().expect("local environment")
));
assert!(manager.try_local_environment().is_some());
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
assert!(!environment.is_remote());
}
#[tokio::test]
async fn disabled_environment_manager_has_no_default_but_keeps_explicit_local_environment() {
let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths());
async fn disabled_environment_manager_has_no_default_or_local_environment() {
let manager = EnvironmentManager::without_environments();
assert!(manager.default_environment().is_none());
assert_eq!(manager.default_environment_id(), None);
assert!(!manager.local_environment().is_remote());
assert_local_environment_unavailable(&manager);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
}
@@ -521,7 +498,7 @@ mod tests {
async fn environment_manager_reports_remote_url() {
let manager = EnvironmentManager::create_for_tests(
Some("ws://127.0.0.1:8765".to_string()),
test_runtime_paths(),
Some(test_runtime_paths()),
)
.await;
@@ -539,7 +516,7 @@ mod tests {
.expect("remote environment")
));
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(!manager.local_environment().is_remote());
assert_local_environment_unavailable(&manager);
}
#[tokio::test]
@@ -557,20 +534,17 @@ mod tests {
}
#[tokio::test]
async fn environment_manager_builds_from_provider() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
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()),
include_local: false,
},
async fn environment_manager_builds_from_snapshot() {
let snapshot = EnvironmentProviderSnapshot {
environments: vec![(
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()),
include_local: false,
};
let manager = EnvironmentManager::from_provider(&provider, test_runtime_paths())
.await
let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect("environment manager");
assert_eq!(
@@ -584,20 +558,17 @@ mod tests {
.is_remote()
);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(!manager.local_environment().is_remote());
assert_local_environment_unavailable(&manager);
}
#[tokio::test]
async fn environment_manager_rejects_empty_environment_id() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![("".to_string(), Environment::default_for_tests())],
default: EnvironmentDefault::Disabled,
include_local: false,
},
let 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())
.await
let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect_err("empty id should fail");
assert_eq!(
@@ -608,18 +579,15 @@ 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 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
let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect_err("local id should fail");
assert_eq!(
@@ -630,19 +598,16 @@ mod tests {
#[tokio::test]
async fn environment_manager_uses_explicit_provider_default() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
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 snapshot = EnvironmentProviderSnapshot {
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())
.await
let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect("manager");
assert_eq!(manager.default_environment_id(), Some("devbox"));
@@ -655,19 +620,16 @@ mod tests {
#[tokio::test]
async fn environment_manager_disables_provider_default() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
"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 snapshot = EnvironmentProviderSnapshot {
environments: vec![(
"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())
.await
let manager = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect("manager");
assert_eq!(manager.default_environment_id(), None);
@@ -676,25 +638,22 @@ mod tests {
&manager
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment"),
&manager.local_environment()
&manager.try_local_environment().expect("local environment")
));
}
#[tokio::test]
async fn environment_manager_rejects_unknown_provider_default() {
let provider = TestEnvironmentProvider {
snapshot: EnvironmentProviderSnapshot {
environments: vec![(
"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 snapshot = EnvironmentProviderSnapshot {
environments: vec![(
"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())
.await
let err = EnvironmentManager::from_snapshot(snapshot, Some(test_runtime_paths()))
.expect_err("unknown default should fail");
assert_eq!(
@@ -707,7 +666,7 @@ mod tests {
async fn environment_manager_includes_local_for_default_provider_without_url() {
let manager = EnvironmentManager::create_for_tests(
/*exec_server_url*/ None,
test_runtime_paths(),
Some(test_runtime_paths()),
)
.await;
@@ -719,7 +678,10 @@ mod tests {
.get_environment(LOCAL_ENVIRONMENT_ID)
.expect("local environment")
));
assert!(Arc::ptr_eq(&environment, &manager.local_environment()));
assert!(Arc::ptr_eq(
&environment,
&manager.try_local_environment().expect("local environment")
));
assert!(!environment.is_remote());
}
@@ -728,44 +690,59 @@ mod tests {
let runtime_paths = test_runtime_paths();
let manager = EnvironmentManager::create_for_tests(
/*exec_server_url*/ None,
runtime_paths.clone(),
Some(runtime_paths.clone()),
)
.await;
let environment = manager.local_environment();
let environment = manager.try_local_environment().expect("local environment");
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
let manager = EnvironmentManager::create_for_tests(
environment.exec_server_url().map(str::to_owned),
environment
.local_runtime_paths()
.expect("local runtime paths")
.clone(),
Some(
environment
.local_runtime_paths()
.expect("local runtime paths")
.clone(),
),
)
.await;
let environment = manager.local_environment();
let environment = manager.try_local_environment().expect("local environment");
assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths));
}
#[tokio::test]
async fn disabled_environment_manager_has_no_default_environment() {
let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths());
assert!(manager.default_environment().is_none());
assert_eq!(manager.default_environment_id(), None);
}
#[tokio::test]
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;
let manager = EnvironmentManager::create_for_tests(
Some("none".to_string()),
Some(test_runtime_paths()),
)
.await;
assert!(manager.default_environment().is_none());
assert_eq!(manager.default_environment_id(), None);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none());
assert!(!manager.local_environment().is_remote());
assert_local_environment_unavailable(&manager);
}
#[tokio::test]
async fn environment_manager_snapshot_without_local_environment_disables_local_default() {
let mut snapshot = EnvironmentProviderSnapshot {
environments: Vec::new(),
default: EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string()),
include_local: true,
};
snapshot.include_local = false;
snapshot.default = EnvironmentDefault::Disabled;
let manager =
EnvironmentManager::from_snapshot(snapshot, /*local_runtime_paths*/ None)
.expect("environment manager");
assert!(manager.default_environment().is_none());
assert_eq!(manager.default_environment_id(), None);
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_none());
assert_local_environment_unavailable(&manager);
}
#[tokio::test]
@@ -777,7 +754,7 @@ mod tests {
#[tokio::test]
async fn environment_manager_upserts_named_remote_environment() {
let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths());
let manager = EnvironmentManager::without_environments();
manager
.upsert_environment("executor-a".to_string(), "ws://127.0.0.1:8765".to_string())
@@ -802,7 +779,7 @@ mod tests {
#[tokio::test]
async fn environment_manager_rejects_empty_remote_environment_url() {
let manager = EnvironmentManager::disabled_for_tests(test_runtime_paths());
let manager = EnvironmentManager::without_environments();
let err = manager
.upsert_environment("executor-a".to_string(), String::new())