mirror of
https://github.com/openai/codex.git
synced 2026-05-28 06:55:01 +00:00
Build remote exec env from exec-server policy (#17216)
## Summary - add an exec-server `envPolicy` field; when present, the server starts from its own process env and applies the shell environment policy there - keep `env` as the exact environment for local/embedded starts, but make it an overlay for remote unified-exec starts - move the shell-environment-policy builder into `codex-config` so Core and exec-server share the inherit/filter/set/include behavior - overlay only runtime/sandbox/network deltas from Core onto the exec-server-derived env ## Why Remote unified exec was materializing the shell env inside Core and forwarding the whole map to exec-server, so remote processes could inherit the orchestrator machine's `HOME`, `PATH`, etc. This keeps the base env on the executor while preserving Core-owned runtime additions like `CODEX_THREAD_ID`, unified-exec defaults, network proxy env, and sandbox marker env. ## Validation - `just fmt` - `git diff --check` - `cargo test -p codex-exec-server --lib` - `cargo test -p codex-core --lib unified_exec::process_manager::tests` - `cargo test -p codex-core --lib exec_env::tests` - `cargo test -p codex-core --lib exec_env_tests` (compile-only; filter matched 0 tests) - `cargo test -p codex-config --lib shell_environment` (compile-only; filter matched 0 tests) - `just bazel-lock-update` ## Known local validation issue - `just bazel-lock-check` is not runnable in this checkout: it invokes `./scripts/check-module-bazel-lock.sh`, which is missing. --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: pakrym-oai <pakrym@openai.com>
This commit is contained in:
@@ -66,10 +66,11 @@ pub(crate) struct OutputHandles {
|
||||
/// Transport-specific process handle used by unified exec.
|
||||
enum ProcessHandle {
|
||||
Local(Box<ExecCommandSession>),
|
||||
Remote(Arc<dyn ExecProcess>),
|
||||
ExecServer(Arc<dyn ExecProcess>),
|
||||
}
|
||||
|
||||
/// Unified wrapper over local PTY sessions and exec-server-backed processes.
|
||||
/// Unified wrapper over directly spawned PTY sessions and exec-server-backed
|
||||
/// processes.
|
||||
pub(crate) struct UnifiedExecProcess {
|
||||
process_handle: ProcessHandle,
|
||||
output_tx: broadcast::Sender<Vec<u8>>,
|
||||
@@ -135,7 +136,7 @@ impl UnifiedExecProcess {
|
||||
.send(data.to_vec())
|
||||
.await
|
||||
.map_err(|_| UnifiedExecError::WriteToStdin),
|
||||
ProcessHandle::Remote(process_handle) => {
|
||||
ProcessHandle::ExecServer(process_handle) => {
|
||||
match process_handle.write(data.to_vec()).await {
|
||||
Ok(response) => match response.status {
|
||||
WriteStatus::Accepted => Ok(()),
|
||||
@@ -179,7 +180,7 @@ impl UnifiedExecProcess {
|
||||
let state = self.state_rx.borrow().clone();
|
||||
match &self.process_handle {
|
||||
ProcessHandle::Local(process_handle) => state.has_exited || process_handle.has_exited(),
|
||||
ProcessHandle::Remote(_) => state.has_exited,
|
||||
ProcessHandle::ExecServer(_) => state.has_exited,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -189,7 +190,7 @@ impl UnifiedExecProcess {
|
||||
ProcessHandle::Local(process_handle) => {
|
||||
state.exit_code.or_else(|| process_handle.exit_code())
|
||||
}
|
||||
ProcessHandle::Remote(_) => state.exit_code,
|
||||
ProcessHandle::ExecServer(_) => state.exit_code,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -198,7 +199,7 @@ impl UnifiedExecProcess {
|
||||
self.output_closed_notify.notify_waiters();
|
||||
match &self.process_handle {
|
||||
ProcessHandle::Local(process_handle) => process_handle.terminate(),
|
||||
ProcessHandle::Remote(process_handle) => {
|
||||
ProcessHandle::ExecServer(process_handle) => {
|
||||
let process_handle = Arc::clone(process_handle);
|
||||
tokio::spawn(async move {
|
||||
let _ = process_handle.terminate().await;
|
||||
@@ -331,14 +332,14 @@ impl UnifiedExecProcess {
|
||||
Ok(managed)
|
||||
}
|
||||
|
||||
pub(super) async fn from_remote_started(
|
||||
pub(super) async fn from_exec_server_started(
|
||||
started: StartedExecProcess,
|
||||
sandbox_type: SandboxType,
|
||||
) -> Result<Self, UnifiedExecError> {
|
||||
let process_handle = ProcessHandle::Remote(Arc::clone(&started.process));
|
||||
let process_handle = ProcessHandle::ExecServer(Arc::clone(&started.process));
|
||||
let mut managed = Self::new(process_handle, sandbox_type, /*spawn_lifecycle*/ None);
|
||||
let output_handles = managed.output_handles();
|
||||
managed.output_task = Some(Self::spawn_remote_output_task(
|
||||
managed.output_task = Some(Self::spawn_exec_server_output_task(
|
||||
started,
|
||||
output_handles,
|
||||
managed.output_tx.clone(),
|
||||
@@ -366,7 +367,7 @@ impl UnifiedExecProcess {
|
||||
Ok(managed)
|
||||
}
|
||||
|
||||
fn spawn_remote_output_task(
|
||||
fn spawn_exec_server_output_task(
|
||||
started: StartedExecProcess,
|
||||
output_handles: OutputHandles,
|
||||
output_tx: broadcast::Sender<Vec<u8>>,
|
||||
|
||||
@@ -11,9 +11,11 @@ use tokio::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
|
||||
use crate::exec_env::CODEX_THREAD_ID_ENV_VAR;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::exec_policy::ExecApprovalRequest;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
use crate::sandboxing::ExecServerEnvConfig;
|
||||
use crate::tools::context::ExecCommandToolOutput;
|
||||
use crate::tools::events::ToolEmitter;
|
||||
use crate::tools::events::ToolEventCtx;
|
||||
@@ -47,6 +49,7 @@ use crate::unified_exec::process::OutputBuffer;
|
||||
use crate::unified_exec::process::OutputHandles;
|
||||
use crate::unified_exec::process::SpawnLifecycleHandle;
|
||||
use crate::unified_exec::process::UnifiedExecProcess;
|
||||
use codex_config::types::ShellEnvironmentPolicy;
|
||||
use codex_protocol::protocol::ExecCommandSource;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_output_truncation::approx_token_count;
|
||||
@@ -89,6 +92,70 @@ fn apply_unified_exec_env(mut env: HashMap<String, String>) -> HashMap<String, S
|
||||
env
|
||||
}
|
||||
|
||||
fn exec_env_policy_from_shell_policy(
|
||||
policy: &ShellEnvironmentPolicy,
|
||||
) -> codex_exec_server::ExecEnvPolicy {
|
||||
codex_exec_server::ExecEnvPolicy {
|
||||
inherit: policy.inherit.clone(),
|
||||
ignore_default_excludes: policy.ignore_default_excludes,
|
||||
exclude: policy
|
||||
.exclude
|
||||
.iter()
|
||||
.map(std::string::ToString::to_string)
|
||||
.collect(),
|
||||
r#set: policy.r#set.clone(),
|
||||
include_only: policy
|
||||
.include_only
|
||||
.iter()
|
||||
.map(std::string::ToString::to_string)
|
||||
.collect(),
|
||||
}
|
||||
}
|
||||
|
||||
fn env_overlay_for_exec_server(
|
||||
request_env: &HashMap<String, String>,
|
||||
local_policy_env: &HashMap<String, String>,
|
||||
) -> HashMap<String, String> {
|
||||
request_env
|
||||
.iter()
|
||||
.filter(|(key, value)| local_policy_env.get(*key) != Some(*value))
|
||||
.map(|(key, value)| (key.clone(), value.clone()))
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn exec_server_env_for_request(
|
||||
request: &ExecRequest,
|
||||
) -> (
|
||||
Option<codex_exec_server::ExecEnvPolicy>,
|
||||
HashMap<String, String>,
|
||||
) {
|
||||
if let Some(exec_server_env_config) = &request.exec_server_env_config {
|
||||
(
|
||||
Some(exec_server_env_config.policy.clone()),
|
||||
env_overlay_for_exec_server(&request.env, &exec_server_env_config.local_policy_env),
|
||||
)
|
||||
} else {
|
||||
(None, request.env.clone())
|
||||
}
|
||||
}
|
||||
|
||||
fn exec_server_params_for_request(
|
||||
process_id: i32,
|
||||
request: &ExecRequest,
|
||||
tty: bool,
|
||||
) -> codex_exec_server::ExecParams {
|
||||
let (env_policy, env) = exec_server_env_for_request(request);
|
||||
codex_exec_server::ExecParams {
|
||||
process_id: exec_server_process_id(process_id).into(),
|
||||
argv: request.command.clone(),
|
||||
cwd: request.cwd.to_path_buf(),
|
||||
env_policy,
|
||||
env,
|
||||
tty,
|
||||
arg0: request.arg0.clone(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Borrowed process state prepared for a `write_stdin` or poll operation.
|
||||
struct PreparedProcessHandles {
|
||||
process: Arc<UnifiedExecProcess>,
|
||||
@@ -587,12 +654,7 @@ impl UnifiedExecProcessManager {
|
||||
mut spawn_lifecycle: SpawnLifecycleHandle,
|
||||
environment: &codex_exec_server::Environment,
|
||||
) -> Result<UnifiedExecProcess, UnifiedExecError> {
|
||||
let (program, args) = request
|
||||
.command
|
||||
.split_first()
|
||||
.ok_or(UnifiedExecError::MissingCommandLine)?;
|
||||
let inherited_fds = spawn_lifecycle.inherited_fds();
|
||||
|
||||
if environment.is_remote() {
|
||||
if !inherited_fds.is_empty() {
|
||||
return Err(UnifiedExecError::create_process(
|
||||
@@ -602,19 +664,17 @@ impl UnifiedExecProcessManager {
|
||||
|
||||
let started = environment
|
||||
.get_exec_backend()
|
||||
.start(codex_exec_server::ExecParams {
|
||||
process_id: exec_server_process_id(process_id).into(),
|
||||
argv: request.command.clone(),
|
||||
cwd: request.cwd.to_path_buf(),
|
||||
env: request.env.clone(),
|
||||
tty,
|
||||
arg0: request.arg0.clone(),
|
||||
})
|
||||
.start(exec_server_params_for_request(process_id, request, tty))
|
||||
.await
|
||||
.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
|
||||
return UnifiedExecProcess::from_remote_started(started, request.sandbox).await;
|
||||
spawn_lifecycle.after_spawn();
|
||||
return UnifiedExecProcess::from_exec_server_started(started, request.sandbox).await;
|
||||
}
|
||||
|
||||
let (program, args) = request
|
||||
.command
|
||||
.split_first()
|
||||
.ok_or(UnifiedExecError::MissingCommandLine)?;
|
||||
let spawn_result = if tty {
|
||||
codex_utils_pty::pty::spawn_process_with_inherited_fds(
|
||||
program,
|
||||
@@ -649,10 +709,20 @@ impl UnifiedExecProcessManager {
|
||||
cwd: AbsolutePathBuf,
|
||||
context: &UnifiedExecContext,
|
||||
) -> Result<(UnifiedExecProcess, Option<DeferredNetworkApproval>), UnifiedExecError> {
|
||||
let env = apply_unified_exec_env(create_env(
|
||||
let local_policy_env = create_env(
|
||||
&context.turn.shell_environment_policy,
|
||||
Some(context.session.conversation_id),
|
||||
));
|
||||
/*thread_id*/ None,
|
||||
);
|
||||
let mut env = local_policy_env.clone();
|
||||
env.insert(
|
||||
CODEX_THREAD_ID_ENV_VAR.to_string(),
|
||||
context.session.conversation_id.to_string(),
|
||||
);
|
||||
let env = apply_unified_exec_env(env);
|
||||
let exec_server_env_config = ExecServerEnvConfig {
|
||||
policy: exec_env_policy_from_shell_policy(&context.turn.shell_environment_policy),
|
||||
local_policy_env,
|
||||
};
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = UnifiedExecRuntime::new(
|
||||
self,
|
||||
@@ -680,6 +750,7 @@ impl UnifiedExecProcessManager {
|
||||
process_id: request.process_id,
|
||||
cwd,
|
||||
env,
|
||||
exec_server_env_config: Some(exec_server_env_config),
|
||||
explicit_env_overrides: context.turn.shell_environment_policy.r#set.clone(),
|
||||
network: request.network.clone(),
|
||||
tty: request.tty,
|
||||
|
||||
@@ -34,6 +34,92 @@ fn unified_exec_env_overrides_existing_values() {
|
||||
assert_eq!(env.get("PATH"), Some(&"/usr/bin".to_string()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn env_overlay_for_exec_server_keeps_runtime_changes_only() {
|
||||
let local_policy_env = HashMap::from([
|
||||
("HOME".to_string(), "/client-home".to_string()),
|
||||
("PATH".to_string(), "/client-path".to_string()),
|
||||
("SHELL_SET".to_string(), "policy".to_string()),
|
||||
]);
|
||||
let request_env = HashMap::from([
|
||||
("HOME".to_string(), "/client-home".to_string()),
|
||||
("PATH".to_string(), "/sandbox-path".to_string()),
|
||||
("SHELL_SET".to_string(), "policy".to_string()),
|
||||
("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),
|
||||
(
|
||||
"CODEX_SANDBOX_NETWORK_DISABLED".to_string(),
|
||||
"1".to_string(),
|
||||
),
|
||||
]);
|
||||
|
||||
assert_eq!(
|
||||
env_overlay_for_exec_server(&request_env, &local_policy_env),
|
||||
HashMap::from([
|
||||
("PATH".to_string(), "/sandbox-path".to_string()),
|
||||
("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),
|
||||
(
|
||||
"CODEX_SANDBOX_NETWORK_DISABLED".to_string(),
|
||||
"1".to_string()
|
||||
),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_server_params_use_env_policy_overlay_contract() {
|
||||
let request = ExecRequest {
|
||||
command: vec!["bash".to_string(), "-lc".to_string(), "true".to_string()],
|
||||
cwd: std::env::current_dir()
|
||||
.expect("current dir")
|
||||
.try_into()
|
||||
.expect("absolute path"),
|
||||
env: HashMap::from([
|
||||
("HOME".to_string(), "/client-home".to_string()),
|
||||
("PATH".to_string(), "/sandbox-path".to_string()),
|
||||
("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),
|
||||
]),
|
||||
exec_server_env_config: Some(ExecServerEnvConfig {
|
||||
policy: codex_exec_server::ExecEnvPolicy {
|
||||
inherit: codex_config::types::ShellEnvironmentPolicyInherit::Core,
|
||||
ignore_default_excludes: false,
|
||||
exclude: Vec::new(),
|
||||
r#set: HashMap::new(),
|
||||
include_only: Vec::new(),
|
||||
},
|
||||
local_policy_env: HashMap::from([
|
||||
("HOME".to_string(), "/client-home".to_string()),
|
||||
("PATH".to_string(), "/client-path".to_string()),
|
||||
]),
|
||||
}),
|
||||
network: None,
|
||||
expiration: crate::exec::ExecExpiration::DefaultTimeout,
|
||||
capture_policy: crate::exec::ExecCapturePolicy::ShellTool,
|
||||
sandbox: codex_sandboxing::SandboxType::None,
|
||||
windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled,
|
||||
windows_sandbox_private_desktop: false,
|
||||
sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
|
||||
file_system_sandbox_policy: codex_protocol::permissions::FileSystemSandboxPolicy::from(
|
||||
&codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
|
||||
),
|
||||
network_sandbox_policy: codex_protocol::permissions::NetworkSandboxPolicy::Restricted,
|
||||
windows_sandbox_filesystem_overrides: None,
|
||||
arg0: None,
|
||||
};
|
||||
|
||||
let params =
|
||||
exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true);
|
||||
|
||||
assert_eq!(params.process_id.as_str(), "123");
|
||||
assert!(params.env_policy.is_some());
|
||||
assert_eq!(
|
||||
params.env,
|
||||
HashMap::from([
|
||||
("PATH".to_string(), "/sandbox-path".to_string()),
|
||||
("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_server_process_id_matches_unified_exec_process_id() {
|
||||
assert_eq!(exec_server_process_id(/*process_id*/ 4321), "4321");
|
||||
|
||||
@@ -76,7 +76,7 @@ async fn remote_process(write_status: WriteStatus) -> UnifiedExecProcess {
|
||||
}),
|
||||
};
|
||||
|
||||
UnifiedExecProcess::from_remote_started(started, SandboxType::None)
|
||||
UnifiedExecProcess::from_exec_server_started(started, SandboxType::None)
|
||||
.await
|
||||
.expect("remote process should start")
|
||||
}
|
||||
@@ -133,7 +133,7 @@ async fn remote_process_waits_for_early_exit_event() {
|
||||
let _ = wake_tx.send(1);
|
||||
});
|
||||
|
||||
let process = UnifiedExecProcess::from_remote_started(started, SandboxType::None)
|
||||
let process = UnifiedExecProcess::from_exec_server_started(started, SandboxType::None)
|
||||
.await
|
||||
.expect("remote process should observe early exit");
|
||||
|
||||
|
||||
Reference in New Issue
Block a user