mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Carry shell invocation metadata and report executor PATH directories (#49360)
## What changed - Carry the selected shell and login mode through unified exec using `ShellInvocation`. Use this metadata for shell snapshots and credential brokerage instead of inferring it from command arguments. - Add `prependPathDirs` to exec-server environment metadata, populated from the executor's package PATH directory. Default to an empty list for older servers and omit empty lists from serialized output. ## Testing Extend environment metadata round-trip coverage for `prependPathDirs` and the remote exec-server integration test to verify the packaged `codex-path` directory is reported. GitOrigin-RevId: c571c068effcb305d196a701ad9a335ec7a3f5a2
This commit is contained in:
committed by
copyberry
parent
1983c48fd1
commit
995138d71a
Generated
+1
@@ -3250,6 +3250,7 @@ version = "0.0.0"
|
||||
dependencies = [
|
||||
"base64 0.22.1",
|
||||
"codex-file-system",
|
||||
"codex-install-context",
|
||||
"codex-mxc-sandbox",
|
||||
"codex-network-proxy",
|
||||
"codex-protocol",
|
||||
|
||||
@@ -187,6 +187,8 @@ metrics_exporter = {{ otlp-http = {{ endpoint = "{collector_url}/v1/metrics", pr
|
||||
std::fs::create_dir(&bin_dir)?;
|
||||
let executable = bin_dir.join(format!("codex{}", std::env::consts::EXE_SUFFIX));
|
||||
copy_executable(&codex_utils_cargo_bin::cargo_bin("codex")?, &executable)?;
|
||||
let codex_path_dir = package.path().join("codex-path");
|
||||
std::fs::create_dir(&codex_path_dir)?;
|
||||
let manifest = package.path().join("codex-package.json");
|
||||
std::fs::write(&manifest, r#"{"version":"1.2.3-alpha.4"}"#)?;
|
||||
|
||||
@@ -250,10 +252,19 @@ metrics_exporter = {{ otlp-http = {{ endpoint = "{collector_url}/v1/metrics", pr
|
||||
.context("remote harness did not connect")???;
|
||||
|
||||
let environment_info = client.environment_info().await?;
|
||||
assert_eq!(
|
||||
environment_info
|
||||
.prepend_path_dirs
|
||||
.iter()
|
||||
.map(|path| std::fs::canonicalize(path.inferred_native_path_string()))
|
||||
.collect::<std::io::Result<Vec<_>>>()?,
|
||||
vec![codex_path_dir.canonicalize()?]
|
||||
);
|
||||
let expected_info = EnvironmentInfo {
|
||||
executor_version: "1.2.3-alpha.4".to_string(),
|
||||
// The build identity belongs to the spawned CLI, not this test process.
|
||||
provider_id: environment_info.provider_id.clone(),
|
||||
prepend_path_dirs: environment_info.prepend_path_dirs.clone(),
|
||||
..EnvironmentInfo::local()
|
||||
};
|
||||
assert_eq!(environment_info, expected_info);
|
||||
|
||||
@@ -76,6 +76,23 @@ impl Shell {
|
||||
}
|
||||
}
|
||||
|
||||
/// The shell and startup mode Codex chose for an exec command.
|
||||
#[derive(Debug, Clone)]
|
||||
pub(crate) struct ShellInvocation {
|
||||
pub(crate) shell: Shell,
|
||||
pub(crate) use_login_shell: bool,
|
||||
}
|
||||
|
||||
impl ShellInvocation {
|
||||
pub(crate) fn is_posix_login(&self) -> bool {
|
||||
self.use_login_shell
|
||||
&& matches!(
|
||||
self.shell.shell_type,
|
||||
ShellType::Bash | ShellType::Zsh | ShellType::Sh
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(all(test, unix))]
|
||||
fn ultimate_fallback_shell() -> Shell {
|
||||
codex_shell_command::shell_detect::ultimate_fallback_shell().into()
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::ShellInvocation;
|
||||
use crate::shell::ShellType;
|
||||
use crate::shell::get_shell_by_model_provided_path;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
@@ -74,7 +75,7 @@ fn default_tty() -> bool {
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct ResolvedCommand {
|
||||
pub(crate) command: Vec<String>,
|
||||
pub(crate) shell_type: ShellType,
|
||||
pub(crate) shell: ShellInvocation,
|
||||
}
|
||||
|
||||
fn post_unified_exec_tool_use_payload(
|
||||
@@ -112,35 +113,31 @@ pub(crate) fn get_command(
|
||||
None => allow_login_shell,
|
||||
};
|
||||
|
||||
match shell_mode {
|
||||
UnifiedExecShellMode::Direct => {
|
||||
let model_shell = args
|
||||
.shell
|
||||
.as_ref()
|
||||
.map(|shell_str| get_shell_by_model_provided_path(&PathBuf::from(shell_str)));
|
||||
let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref());
|
||||
Ok(ResolvedCommand {
|
||||
command: shell.derive_exec_args(&args.cmd, use_login_shell),
|
||||
shell_type: shell.shell_type,
|
||||
})
|
||||
}
|
||||
let shell = match shell_mode {
|
||||
UnifiedExecShellMode::Direct => args
|
||||
.shell
|
||||
.as_ref()
|
||||
.map(|shell_str| get_shell_by_model_provided_path(&PathBuf::from(shell_str)))
|
||||
.unwrap_or_else(|| session_shell.as_ref().clone()),
|
||||
UnifiedExecShellMode::ZshFork(zsh_fork_config) => {
|
||||
if args.shell.is_some() {
|
||||
return Err(
|
||||
"`shell` is not supported for local zsh-fork exec; omit `shell` to use zsh-fork, or target a remote environment where `shell` is supported.".to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
Ok(ResolvedCommand {
|
||||
command: vec![
|
||||
zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(),
|
||||
if use_login_shell { "-lc" } else { "-c" }.to_string(),
|
||||
args.cmd.clone(),
|
||||
],
|
||||
Shell {
|
||||
shell_type: ShellType::Zsh,
|
||||
})
|
||||
shell_path: zsh_fork_config.shell_zsh_path.as_path().to_path_buf(),
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
Ok(ResolvedCommand {
|
||||
command: shell.derive_exec_args(&args.cmd, use_login_shell),
|
||||
shell: ShellInvocation {
|
||||
shell,
|
||||
use_login_shell,
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn shell_mode_for_environment(
|
||||
|
||||
@@ -297,7 +297,6 @@ impl ExecCommandHandler {
|
||||
)
|
||||
.map_err(FunctionCallError::RespondToModel)?;
|
||||
let command = resolved_command.command;
|
||||
let shell_type = resolved_command.shell_type;
|
||||
let ExecCommandArgs {
|
||||
mut tty,
|
||||
yield_time_ms,
|
||||
@@ -418,7 +417,7 @@ impl ExecCommandHandler {
|
||||
let process_id = manager.allocate_process_id().await;
|
||||
let request = ExecCommandRequest {
|
||||
command,
|
||||
shell_type,
|
||||
shell: resolved_command.shell,
|
||||
hook_command: hook_command.clone(),
|
||||
process_id,
|
||||
yield_time_ms,
|
||||
|
||||
@@ -130,7 +130,7 @@ fn test_get_command_resolves_powershell_by_type() -> anyhow::Result<()> {
|
||||
resolved.command,
|
||||
expected_shell.derive_exec_args("echo hello", /*use_login_shell*/ true)
|
||||
);
|
||||
assert_eq!(resolved.shell_type, expected_shell.shell_type);
|
||||
assert_eq!(resolved.shell.shell, expected_shell);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -14,6 +14,7 @@ use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecServerEnvConfig;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::shell::ShellInvocation;
|
||||
use crate::shell::ShellType;
|
||||
use crate::shell_snapshot::ShellSnapshotSandbox;
|
||||
use crate::shell_snapshot::snapshot_read_permissions;
|
||||
@@ -58,7 +59,6 @@ use codex_tools::UnifiedExecShellMode;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::collections::HashMap;
|
||||
use std::io;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
|
||||
@@ -74,7 +74,7 @@ const REMOTE_NETWORK_POLICY_DECISION_MARGIN: Duration = Duration::from_secs(10);
|
||||
#[derive(Clone, Debug)]
|
||||
pub struct UnifiedExecRequest {
|
||||
pub command: Vec<String>,
|
||||
pub shell_type: ShellType,
|
||||
pub(crate) shell: ShellInvocation,
|
||||
pub hook_command: String,
|
||||
pub process_id: i32,
|
||||
pub cwd: PathUri,
|
||||
@@ -271,6 +271,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
ctx: &ToolCtx,
|
||||
) -> Result<UnifiedExecAttempt, ToolError> {
|
||||
let base_command = &req.command;
|
||||
let requested_shell = &req.shell.shell;
|
||||
let windows_sandbox_proxy_settings_mode = ctx.session.windows_sandbox_proxy_settings_mode;
|
||||
let session_shell = ctx.session.user_shell();
|
||||
let environment_shell = req
|
||||
@@ -306,19 +307,11 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
"credential brokerage does not yet support shell_zsh_fork; disable shell_zsh_fork to use the broker".to_string(),
|
||||
));
|
||||
}
|
||||
let requested_shell = (credential_broker_available
|
||||
&& (req.shell_type != environment_shell.shell_type
|
||||
|| base_command.first().is_some_and(|path| {
|
||||
path != environment_shell.shell_path.to_string_lossy().as_ref()
|
||||
})))
|
||||
.then(|| {
|
||||
base_command.first().map(|path| crate::shell::Shell {
|
||||
shell_type: req.shell_type,
|
||||
shell_path: PathBuf::from(path),
|
||||
})
|
||||
})
|
||||
.flatten();
|
||||
let shell = requested_shell.as_ref().unwrap_or(environment_shell);
|
||||
let shell = if credential_broker_available {
|
||||
requested_shell
|
||||
} else {
|
||||
environment_shell
|
||||
};
|
||||
let shell_snapshot = if environment_is_remote
|
||||
|| credential_broker_available
|
||||
&& launch_sandbox_permissions.requires_escalated_permissions()
|
||||
@@ -348,7 +341,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
let shell_snapshot_location = shell_snapshot.as_ref().map(|snapshot| snapshot.path());
|
||||
let mut env = exec_env_for_sandbox_permissions(&req.env, launch_sandbox_permissions);
|
||||
let snapshot_credential_context = if let Some(snapshot) = shell_snapshot.as_ref()
|
||||
&& (managed_network.is_some() || base_command.get(1).is_some_and(|flag| flag == "-lc"))
|
||||
&& (managed_network.is_some() || req.shell.is_posix_login())
|
||||
{
|
||||
Some(
|
||||
snapshot
|
||||
@@ -576,7 +569,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
// preparation is a no-op for PowerShell on other platforms.
|
||||
let command = prepare_powershell_command_for_windows_sandbox(
|
||||
&command,
|
||||
Some(&req.shell_type),
|
||||
Some(&requested_shell.shell_type),
|
||||
attempt.sandbox_requested,
|
||||
executor_windows_sandbox_selection(
|
||||
attempt.windows_sandbox_type,
|
||||
@@ -585,7 +578,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
),
|
||||
environment_is_remote,
|
||||
);
|
||||
let command = if matches!(req.shell_type, ShellType::PowerShell) {
|
||||
let command = if matches!(requested_shell.shell_type, ShellType::PowerShell) {
|
||||
prefix_powershell_script_with_utf8(&command)
|
||||
} else {
|
||||
command
|
||||
@@ -761,6 +754,7 @@ mod tests {
|
||||
use crate::config::PermissionProfileSnapshot;
|
||||
use crate::environment_selection::EnvironmentConfigOrigin;
|
||||
use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS;
|
||||
use crate::shell::Shell;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::LOCAL_ENVIRONMENT_ID;
|
||||
@@ -864,7 +858,13 @@ mod tests {
|
||||
let runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct);
|
||||
let request = UnifiedExecRequest {
|
||||
command: vec!["pwd".to_string()],
|
||||
shell_type: ShellType::Sh,
|
||||
shell: ShellInvocation {
|
||||
shell: Shell {
|
||||
shell_type: ShellType::Sh,
|
||||
shell_path: "sh".into(),
|
||||
},
|
||||
use_login_shell: false,
|
||||
},
|
||||
hook_command: "pwd".to_string(),
|
||||
process_id: 1000,
|
||||
cwd: cwd.into(),
|
||||
@@ -967,7 +967,13 @@ mod tests {
|
||||
.expect("current dir is absolute");
|
||||
UnifiedExecRequest {
|
||||
command: vec!["zsh".to_string(), "-c".to_string(), "echo hi".to_string()],
|
||||
shell_type: ShellType::Zsh,
|
||||
shell: ShellInvocation {
|
||||
shell: Shell {
|
||||
shell_type: ShellType::Zsh,
|
||||
shell_path: "zsh".into(),
|
||||
},
|
||||
use_login_shell: false,
|
||||
},
|
||||
hook_command: "echo hi".to_string(),
|
||||
process_id: 1000,
|
||||
cwd: cwd.clone().into(),
|
||||
|
||||
@@ -43,7 +43,7 @@ use crate::session::session::Session;
|
||||
use crate::session::step_context::StepContext;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::session::turn_context::TurnEnvironment;
|
||||
use crate::shell::ShellType;
|
||||
use crate::shell::ShellInvocation;
|
||||
use crate::tools::network_approval::DeferredNetworkApproval;
|
||||
use codex_core_plugins::PluginMetricsSidecar;
|
||||
|
||||
@@ -113,7 +113,7 @@ impl UnifiedExecContext {
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct ExecCommandRequest {
|
||||
pub command: Vec<String>,
|
||||
pub shell_type: ShellType,
|
||||
pub shell: ShellInvocation,
|
||||
pub hook_command: String,
|
||||
pub process_id: i32,
|
||||
pub yield_time_ms: u64,
|
||||
|
||||
@@ -1505,7 +1505,7 @@ impl UnifiedExecProcessManager {
|
||||
.await;
|
||||
let req = UnifiedExecToolRequest {
|
||||
command: request.command.clone(),
|
||||
shell_type: request.shell_type,
|
||||
shell: request.shell.clone(),
|
||||
hook_command: request.hook_command.clone(),
|
||||
process_id: request.process_id,
|
||||
cwd,
|
||||
|
||||
@@ -1,4 +1,7 @@
|
||||
use super::*;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::ShellInvocation;
|
||||
use crate::shell::ShellType;
|
||||
use crate::unified_exec::clamp_yield_time;
|
||||
use codex_network_proxy::ManagedNetworkSandboxContext;
|
||||
use core_test_support::assert_regex_match;
|
||||
@@ -459,7 +462,13 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() {
|
||||
"-lc".to_string(),
|
||||
"echo before".to_string(),
|
||||
],
|
||||
shell_type: crate::shell::ShellType::Sh,
|
||||
shell: ShellInvocation {
|
||||
shell: Shell {
|
||||
shell_type: ShellType::Sh,
|
||||
shell_path: "sh".into(),
|
||||
},
|
||||
use_login_shell: true,
|
||||
},
|
||||
hook_command: "echo before".to_string(),
|
||||
process_id: 123,
|
||||
yield_time_ms: 1000,
|
||||
|
||||
@@ -179,15 +179,12 @@ pub(super) fn shell_snapshot_request(
|
||||
|| !request.turn_environment.shell_snapshot_v2_supported
|
||||
|| request.turn_environment.selection.cwd != *cwd
|
||||
|| !matches!(request.shell_mode, UnifiedExecShellMode::Direct)
|
||||
|| !matches!(
|
||||
request.shell_type,
|
||||
ShellType::Bash | ShellType::Zsh | ShellType::Sh
|
||||
)
|
||||
|| request.command.get(1).is_none_or(|flag| flag != "-lc")
|
||||
|| !request.shell.is_posix_login()
|
||||
{
|
||||
return None;
|
||||
}
|
||||
|
||||
let shell = &request.shell.shell;
|
||||
Some(ShellSnapshotRequest {
|
||||
scope_id: format!(
|
||||
"{}:{}",
|
||||
@@ -195,8 +192,8 @@ pub(super) fn shell_snapshot_request(
|
||||
request.turn_environment.selection.environment_id
|
||||
),
|
||||
shell: ShellInfo {
|
||||
name: request.shell_type.name().to_string(),
|
||||
path: request.command.first()?.clone(),
|
||||
name: shell.name().to_string(),
|
||||
path: shell.shell_path.to_string_lossy().into_owned(),
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ workspace = true
|
||||
[dependencies]
|
||||
base64 = { workspace = true }
|
||||
codex-file-system = { workspace = true }
|
||||
codex-install-context = { workspace = true }
|
||||
codex-network-proxy = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
codex-shell-command = { workspace = true }
|
||||
|
||||
@@ -6,6 +6,7 @@ use codex_file_system::FileSystemSandboxContext;
|
||||
pub use codex_file_system::WalkOptions;
|
||||
pub use codex_file_system::WalkOutcome;
|
||||
use codex_file_system::WireFileSystemSandboxContext;
|
||||
use codex_install_context::InstallContext;
|
||||
use codex_network_proxy::ManagedNetworkSandboxContext;
|
||||
use codex_network_proxy::RemoteNetworkProxyLaunchConfig;
|
||||
use codex_protocol::ThreadId;
|
||||
@@ -116,6 +117,9 @@ pub struct EnvironmentInfo {
|
||||
/// Operating system reported by the executor; absent for legacy exec-servers.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub platform_os: Option<String>,
|
||||
/// Executor directories to prepend to `PATH` when missing, in priority order.
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub prepend_path_dirs: Vec<PathUri>,
|
||||
/// Executor-local default directories for resolving `:tmpdir`, when reported.
|
||||
/// On Windows, a command's `TEMP` or `TMP` overrides take precedence.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
@@ -252,6 +256,13 @@ impl EnvironmentInfo {
|
||||
cwd: cwd.and_then(|cwd| PathUri::from_host_native_path(cwd).ok()),
|
||||
user_home_dir: PathUri::from_host_native_path("~").ok(),
|
||||
platform_os: Some(std::env::consts::OS.to_string()),
|
||||
prepend_path_dirs: InstallContext::current()
|
||||
.package_layout
|
||||
.as_ref()
|
||||
.and_then(|layout| layout.path_dir.as_ref())
|
||||
.into_iter()
|
||||
.map(PathUri::from_abs_path)
|
||||
.collect(),
|
||||
temporary_directories: Some(temporary_directories),
|
||||
temp_dir,
|
||||
capabilities: EnvironmentCapabilities {
|
||||
@@ -1372,6 +1383,7 @@ mod tests {
|
||||
cwd: None,
|
||||
user_home_dir: None,
|
||||
platform_os: None,
|
||||
prepend_path_dirs: Vec::new(),
|
||||
temporary_directories: None,
|
||||
temp_dir: None,
|
||||
capabilities: EnvironmentCapabilities::default(),
|
||||
@@ -1447,6 +1459,7 @@ mod tests {
|
||||
"cwd": null,
|
||||
"userHomeDir": "file:///C:/Users/remote",
|
||||
"platformOs": "windows",
|
||||
"prependPathDirs": ["file:///C:/tools/bin", "file:///D:/tools/bin"],
|
||||
"temporaryDirectories": ["file:///C:/Temp", "file:///D:/Temp"],
|
||||
"capabilities": {
|
||||
"networkProxyLaunch": false,
|
||||
|
||||
Reference in New Issue
Block a user