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 environment providers own default selection (#20665)

Browse Source 
## Why

The next PR in this stack introduces configured environments, where the
provider knows both which environments exist and which one should be
selected by default. The existing manager derived the default internally
by checking for the legacy `remote` and `local` ids, and it treated
"remote" as equivalent to "has a websocket URL." That does not work
cleanly for stdio-command remotes because they are remote environments
without an `exec_server_url`.

**Stack position:** this is PR 3 of 5. It is the environment-model
bridge between PR 2's transport enum and PR 4's TOML provider.

## What Changed

- Add `DefaultEnvironmentSelection` to the `EnvironmentProvider`
contract:
  - `Derived` preserves the old `remote`-then-`local` fallback behavior.
- `Environment(id)` lets a provider explicitly select a configured
default.
- `Disabled` lets a provider intentionally expose no default
environment.
- Move the legacy `CODEX_EXEC_SERVER_URL=none` default-disabling
behavior into `DefaultEnvironmentProvider`.
- Make `EnvironmentManager` validate explicit provider defaults and
return an error if the selected id is missing.
- Track `remote_transport` separately from `exec_server_url` so
stdio-command environments are still recognized as remote.
- Add `Environment::remote_stdio_shell_command(...)` for the TOML
provider added in the next PR.

## Stack

- 1. https://github.com/openai/codex/pull/20663 - Add stdio exec-server
listener
- 2. https://github.com/openai/codex/pull/20664 - Add stdio exec-server
client transport
- **3. This PR:** https://github.com/openai/codex/pull/20665 - Make
environment providers own default selection
- 4. https://github.com/openai/codex/pull/20666 - Add CODEX_HOME
environments TOML provider
- 5. https://github.com/openai/codex/pull/20667 - Load configured
environments from CODEX_HOME

Split from original draft: https://github.com/openai/codex/pull/20508

## Validation

Not run locally; this was split out of the original draft stack.

---------

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
starr-openai
2026-05-07 18:00:31 -07:00
committed by GitHub
Unverified
parent 79ad209ce6
commit 9669756b5f
2 changed files with 204 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/exec-server/src/environment.rs
+140 -48
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;
@@ -32,8 +34,8 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL";
/// shell/filesystem tool availability.
///
/// Remote environments create remote filesystem and execution backends that
/// lazy-connect to the configured exec-server on first use. The websocket is
/// not opened when the manager or environment is constructed.
/// lazy-connect to the configured exec-server on first use. The remote
/// transport is not opened when the manager or environment is constructed.
#[derive(Debug)]
pub struct EnvironmentManager {
default_environment: Option<String>,
@@ -72,9 +74,11 @@ impl EnvironmentManager {
/// Builds a test-only manager with environment access disabled.
pub fn disabled_for_tests(local_runtime_paths: ExecServerRuntimePaths) -> Self {
let mut manager = Self::from_environments(HashMap::new(), local_runtime_paths);
manager.default_environment = None;
manager
Self {
default_environment: None,
environments: HashMap::new(),
local_environment: Arc::new(Environment::local(local_runtime_paths)),
}
}
/// Builds a test-only manager from a raw exec-server URL value.
@@ -99,16 +103,11 @@ impl EnvironmentManager {
exec_server_url: Option<String>,
local_runtime_paths: ExecServerRuntimePaths,
) -> Self {
let environment_disabled = normalize_exec_server_url(exec_server_url.clone()).1;
let provider = DefaultEnvironmentProvider::new(exec_server_url);
let provider_environments = provider.environments(&local_runtime_paths);
let mut manager = Self::from_environments(provider_environments, local_runtime_paths);
if environment_disabled {
// TODO: Remove this legacy `CODEX_EXEC_SERVER_URL=none` crutch once
// environment attachment defaulting moves out of EnvironmentManager.
manager.default_environment = None;
match Self::from_provider(&provider, local_runtime_paths).await {
Ok(manager) => manager,
Err(err) => panic!("default provider should create valid environments: {err}"),
}
manager
}
/// Builds a manager from a provider-supplied startup snapshot.
@@ -119,16 +118,20 @@ impl EnvironmentManager {
where
P: EnvironmentProvider + ?Sized,
{
Self::from_provider_environments(
provider.get_environments(&local_runtime_paths).await?,
Self::from_provider_snapshot(
provider.snapshot(&local_runtime_paths).await?,
local_runtime_paths,
)
}
fn from_provider_environments(
environments: HashMap<String, Environment>,
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(
@@ -137,21 +140,16 @@ impl EnvironmentManager {
}
}
Ok(Self::from_environments(environments, local_runtime_paths))
}
fn from_environments(
environments: HashMap<String, Environment>,
local_runtime_paths: ExecServerRuntimePaths,
) -> Self {
// TODO: Stop deriving a default environment here once omitted
// environment attachment is owned by thread/session setup.
let default_environment = 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
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
@@ -159,11 +157,11 @@ impl EnvironmentManager {
.map(|(id, environment)| (id, Arc::new(environment)))
.collect();
Self {
Ok(Self {
default_environment,
environments,
local_environment,
}
})
}
/// Returns the default environment instance.
@@ -326,10 +324,28 @@ 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,
_local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot.clone())
}
}
fn test_runtime_paths() -> ExecServerRuntimePaths {
ExecServerRuntimePaths::new(
std::env::current_exe().expect("current exe"),
@@ -420,15 +436,20 @@ mod tests {
}
#[tokio::test]
async fn environment_manager_builds_from_provider_environments() {
let manager = EnvironmentManager::from_environments(
HashMap::from([(
REMOTE_ENVIRONMENT_ID.to_string(),
Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string()))
.expect("remote environment"),
)]),
test_runtime_paths(),
);
async fn environment_manager_builds_from_provider() {
let provider = TestEnvironmentProvider {
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!(
manager.default_environment_id(),
@@ -446,11 +467,15 @@ mod tests {
#[tokio::test]
async fn environment_manager_rejects_empty_environment_id() {
let err = EnvironmentManager::from_provider_environments(
HashMap::from([("".to_string(), Environment::default_for_tests())]),
test_runtime_paths(),
)
.expect_err("empty id should fail");
let provider = TestEnvironmentProvider {
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!(
err.to_string(),
@@ -458,6 +483,73 @@ mod tests {
);
}
#[tokio::test]
async fn environment_manager_uses_explicit_provider_default() {
let provider = TestEnvironmentProvider {
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())
.await
.expect("manager");
assert_eq!(manager.default_environment_id(), Some("devbox"));
assert!(manager.default_environment().expect("default").is_remote());
}
#[tokio::test]
async fn environment_manager_disables_provider_default() {
let provider = TestEnvironmentProvider {
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())
.await
.expect("manager");
assert_eq!(manager.default_environment_id(), None);
assert!(manager.default_environment().is_none());
assert!(manager.get_environment(LOCAL_ENVIRONMENT_ID).is_some());
}
#[tokio::test]
async fn environment_manager_rejects_unknown_provider_default() {
let provider = TestEnvironmentProvider {
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!(
err.to_string(),
"exec-server protocol error: default environment `missing` is not configured"
);
}
#[tokio::test]
async fn environment_manager_uses_provider_supplied_local_environment() {
let manager = EnvironmentManager::create_for_tests(
codex-rs/exec-server/src/environment_provider.rs
+64 -23
View File
@@ -11,16 +11,29 @@ use crate::environment::REMOTE_ENVIRONMENT_ID;
/// Lists the concrete environments available to Codex.
///
/// Implementations should return the provider-owned startup snapshot that
/// `EnvironmentManager` will cache. 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.
async 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>;
}
#[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`.
@@ -40,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 = normalize_exec_server_url(self.exec_server_url.clone()).0;
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(
@@ -57,17 +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 {
async fn get_environments(
async fn snapshot(
&self,
local_runtime_paths: &ExecServerRuntimePaths,
) -> Result<HashMap<String, Environment>, ExecServerError> {
Ok(self.environments(local_runtime_paths))
) -> Result<EnvironmentProviderSnapshot, ExecServerError> {
Ok(self.snapshot_inner(local_runtime_paths))
}
}
@@ -98,10 +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!(
@@ -109,42 +134,54 @@ mod tests {
Some(&runtime_paths)
);
assert!(!environments.contains_key(REMOTE_ENVIRONMENT_ID));
assert_eq!(
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
);
}
#[tokio::test]
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!(
snapshot.default,
EnvironmentDefault::EnvironmentId(LOCAL_ENVIRONMENT_ID.to_string())
);
}
#[tokio::test]
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!(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];
@@ -153,6 +190,10 @@ mod tests {
remote_environment.exec_server_url(),
Some("ws://127.0.0.1:8765")
);
assert_eq!(
snapshot.default,
EnvironmentDefault::EnvironmentId(REMOTE_ENVIRONMENT_ID.to_string())
);
}
#[tokio::test]
@@ -160,12 +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")
);
}