diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index 1a43ce7e1..5cc519418 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -75,45 +75,25 @@ if (!platformPackage) { throw new Error(`Unsupported target triple: ${targetTriple}`); } -const codexBinaryName = process.platform === "win32" ? "codex.exe" : "codex"; -const localVendorRoot = path.join(__dirname, "..", "vendor"); -const packageBinaryPath = (vendorRoot) => - path.join(vendorRoot, targetTriple, "bin", codexBinaryName); -const legacyBinaryPath = (vendorRoot) => - path.join(vendorRoot, targetTriple, "codex", codexBinaryName); - -function resolveNativePackage(vendorRoot) { - const packageRoot = path.join(vendorRoot, targetTriple); - const binaryPath = packageBinaryPath(vendorRoot); - if (existsSync(binaryPath)) { - return { - binaryPath, - pathDir: path.join(packageRoot, "codex-path"), - }; +function findCodexExecutable() { + let vendorRoot; + try { + const packageJsonPath = require.resolve(`${platformPackage}/package.json`); + vendorRoot = path.join(path.dirname(packageJsonPath), "vendor"); + } catch { + vendorRoot = path.join(__dirname, "..", "vendor"); } - const legacyPath = legacyBinaryPath(vendorRoot); - if (existsSync(legacyPath)) { - return { - binaryPath: legacyPath, - pathDir: path.join(packageRoot, "path"), - }; - } - - return null; -} - -let nativePackage; -try { - const packageJsonPath = require.resolve(`${platformPackage}/package.json`); - nativePackage = resolveNativePackage( - path.join(path.dirname(packageJsonPath), "vendor"), + const codexExecutable = path.join( + vendorRoot, + targetTriple, + "bin", + process.platform === "win32" ? "codex.exe" : "codex", ); -} catch { - nativePackage = resolveNativePackage(localVendorRoot); -} + if (existsSync(codexExecutable)) { + return codexExecutable; + } -if (!nativePackage) { const packageManager = detectPackageManager(); const updateCommand = packageManager === "bun" @@ -124,7 +104,7 @@ if (!nativePackage) { ); } -const { binaryPath, pathDir } = nativePackage; +const binaryPath = findCodexExecutable(); // Use an asynchronous spawn instead of spawnSync so that Node is able to // respond to signals (e.g. Ctrl-C / SIGINT) while the native binary is @@ -132,16 +112,6 @@ const { binaryPath, pathDir } = nativePackage; // and guarantees that when either the child terminates or the parent // receives a fatal signal, both processes exit in a predictable manner. -function getUpdatedPath(newDirs) { - const pathSep = process.platform === "win32" ? ";" : ":"; - const existingPath = process.env.PATH || ""; - const updatedPath = [ - ...newDirs, - ...existingPath.split(pathSep).filter(Boolean), - ].join(pathSep); - return updatedPath; -} - /** * Use heuristics to detect the package manager that was used to install Codex * in order to give the user a hint about how to update it. @@ -167,19 +137,15 @@ function detectPackageManager() { return userAgent ? "npm" : null; } -const additionalDirs = []; -if (existsSync(pathDir)) { - additionalDirs.push(pathDir); -} -const updatedPath = getUpdatedPath(additionalDirs); - -const env = { ...process.env, PATH: updatedPath }; const packageManagerEnvVar = detectPackageManager() === "bun" ? "CODEX_MANAGED_BY_BUN" : "CODEX_MANAGED_BY_NPM"; -env[packageManagerEnvVar] = "1"; -env.CODEX_MANAGED_PACKAGE_ROOT = realpathSync(path.join(__dirname, "..")); +const env = { + ...process.env, + [packageManagerEnvVar]: "1", + CODEX_MANAGED_PACKAGE_ROOT: realpathSync(path.join(__dirname, "..")), +}; const child = spawn(binaryPath, process.argv.slice(2), { stdio: "inherit", diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2183d91b6..e9c171d77 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2145,12 +2145,14 @@ dependencies = [ "anyhow", "codex-apply-patch", "codex-exec-server", + "codex-install-context", "codex-linux-sandbox", "codex-sandboxing", "codex-shell-escalation", "codex-utils-absolute-path", "codex-utils-home-dir", "dotenvy", + "pretty_assertions", "tempfile", "tokio", ] diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index 7ee21a770..55526b4d0 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -16,6 +16,7 @@ workspace = true anyhow = { workspace = true } codex-apply-patch = { workspace = true } codex-exec-server = { workspace = true } +codex-install-context = { workspace = true } codex-linux-sandbox = { workspace = true } codex-sandboxing = { workspace = true } codex-shell-escalation = { workspace = true } @@ -24,3 +25,6 @@ codex-utils-home-dir = { workspace = true } dotenvy = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } + +[dev-dependencies] +pretty_assertions = { workspace = true } diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 87940f118..ba254d57a 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -1,3 +1,4 @@ +use std::ffi::OsString; use std::fs::File; use std::future::Future; use std::path::Path; @@ -5,6 +6,7 @@ use std::path::PathBuf; use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; use codex_exec_server::CODEX_FS_HELPER_ARG1; +use codex_install_context::InstallContext; use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0; use codex_utils_home_dir::find_codex_home; #[cfg(unix)] @@ -138,13 +140,36 @@ pub fn arg0_dispatch() -> Option { // before creating any threads/the Tokio runtime. load_dotenv(); - match prepend_path_entry_for_codex_aliases() { - Ok(path_entry) => Some(path_entry), + let (path_entry_guard, updated_path_env_var) = prepare_path_env_var_with_aliases( + InstallContext::current(), + std::env::var_os("PATH"), + prepare_path_entry_for_codex_aliases, + ); + if let Some(updated_path_env_var) = updated_path_env_var { + // It is safe to call set_var() because our process is single-threaded at + // this point in its execution. + unsafe { + std::env::set_var("PATH", updated_path_env_var); + } + } + path_entry_guard +} + +fn prepare_path_env_var_with_aliases( + install_context: &InstallContext, + existing_path: Option, + prepare_aliases: impl FnOnce(Option) -> std::io::Result<(Arg0PathEntryGuard, OsString)>, +) -> (Option, Option) { + let package_path = path_env_with_package_path_dir(install_context, existing_path.clone()); + let path_for_aliases = package_path.clone().or(existing_path); + + match prepare_aliases(path_for_aliases) { + Ok((path_entry, updated_path_env_var)) => (Some(path_entry), Some(updated_path_env_var)), Err(err) => { // It is possible that Codex will proceed successfully even if - // updating the PATH fails, so warn the user and move on. - eprintln!("WARNING: proceeding, even though we could not update PATH: {err}"); - None + // creating helper aliases fails, so warn the user and move on. + eprintln!("WARNING: proceeding, even though we could not create PATH aliases: {err}"); + (None, package_path) } } } @@ -285,15 +310,16 @@ where /// - WINDOWS: `apply_patch.bat` batch script to invoke the current executable /// with the hidden `--codex-run-as-apply-patch` flag. /// -/// This temporary directory is prepended to the PATH environment variable so -/// that `apply_patch` can be on the PATH without requiring the user to -/// install a separate `apply_patch` executable, simplifying the deployment of -/// Codex CLI. +/// Returns the temporary directory guard and the PATH value that prepends the +/// temporary directory so `apply_patch` can be on the PATH without requiring the +/// user to install a separate executable, simplifying the deployment of Codex +/// CLI. /// Note: In debug builds the temp-dir guard is disabled to ease local testing. /// -/// IMPORTANT: This function modifies the PATH environment variable, so it MUST -/// be called before multiple threads are spawned. -pub fn prepend_path_entry_for_codex_aliases() -> std::io::Result { +/// IMPORTANT: Callers must update PATH before multiple threads are spawned. +fn prepare_path_entry_for_codex_aliases( + existing_path: Option, +) -> std::io::Result<(Arg0PathEntryGuard, OsString)> { let codex_home = find_codex_home()?; #[cfg(not(debug_assertions))] { @@ -371,27 +397,7 @@ pub fn prepend_path_entry_for_codex_aliases() -> std::io::Result { - let mut path_env_var = - std::ffi::OsString::with_capacity(path.as_os_str().len() + 1 + existing_path.len()); - path_env_var.push(path); - path_env_var.push(PATH_SEPARATOR); - path_env_var.push(existing_path); - path_env_var - } - None => path.as_os_str().to_owned(), - }; - - unsafe { - std::env::set_var("PATH", updated_path_env_var); - } + let updated_path_env_var = path_env_with_entry(path, existing_path); let paths = Arg0DispatchPaths { codex_self_exe: std::env::current_exe().ok(), @@ -417,7 +423,41 @@ pub fn prepend_path_entry_for_codex_aliases() -> std::io::Result, +) -> Option { + let path_dir = install_context + .package_layout + .as_ref() + .and_then(|package_layout| package_layout.path_dir.as_ref())?; + Some(path_env_with_entry(path_dir.as_path(), existing_path)) +} + +fn path_env_with_entry(path_entry: &Path, existing_path: Option) -> OsString { + #[cfg(unix)] + const PATH_SEPARATOR: &str = ":"; + + #[cfg(windows)] + const PATH_SEPARATOR: &str = ";"; + + let capacity = path_entry.as_os_str().len() + + existing_path + .as_ref() + .map_or(0, |existing_path| 1 + existing_path.len()); + let mut path_env_var = OsString::with_capacity(capacity); + path_env_var.push(path_entry); + if let Some(existing_path) = existing_path { + path_env_var.push(PATH_SEPARATOR); + path_env_var.push(existing_path); + } + path_env_var } fn janitor_cleanup(temp_root: &Path) -> std::io::Result<()> { @@ -475,12 +515,25 @@ mod tests { use super::run_main_with_arg0_guard; #[cfg(unix)] use anyhow::ensure; + use codex_install_context::CodexPackageLayout; + use codex_install_context::InstallContext; + use codex_install_context::InstallMethod; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; use std::fs; use std::fs::File; use std::path::Path; use std::path::PathBuf; use tempfile::TempDir; + struct PackagePathTestFixture { + _temp_dir: TempDir, + arg0_dir: PathBuf, + existing_dir: PathBuf, + install_context: InstallContext, + path_dir: AbsolutePathBuf, + } + fn create_lock(dir: &Path) -> std::io::Result { let lock_path = dir.join(LOCK_FILENAME); File::options() @@ -491,6 +544,37 @@ mod tests { .open(lock_path) } + fn package_path_test_fixture() -> anyhow::Result { + let temp_dir = TempDir::new()?; + let arg0_dir = temp_dir.path().join("arg0"); + let package_dir = temp_dir.path().join("package"); + let bin_dir = package_dir.join("bin"); + let path_dir = package_dir.join("codex-path"); + let existing_dir = temp_dir.path().join("existing-bin"); + fs::create_dir_all(&arg0_dir)?; + fs::create_dir_all(&bin_dir)?; + fs::create_dir_all(&path_dir)?; + fs::create_dir_all(&existing_dir)?; + let path_dir = AbsolutePathBuf::from_absolute_path(path_dir.canonicalize()?)?; + let install_context = InstallContext { + method: InstallMethod::Other, + package_layout: Some(CodexPackageLayout { + package_dir: AbsolutePathBuf::from_absolute_path(package_dir.canonicalize()?)?, + bin_dir: AbsolutePathBuf::from_absolute_path(bin_dir.canonicalize()?)?, + resources_dir: None, + path_dir: Some(path_dir.clone()), + }), + }; + + Ok(PackagePathTestFixture { + _temp_dir: temp_dir, + arg0_dir, + existing_dir, + install_context, + path_dir, + }) + } + #[test] fn linux_sandbox_exe_path_prefers_codex_linux_sandbox_alias() -> std::io::Result<()> { let temp_dir = TempDir::new()?; @@ -513,6 +597,63 @@ mod tests { Ok(()) } + #[test] + fn path_env_can_prepend_package_path_before_arg0_alias_dir() -> anyhow::Result<()> { + let fixture = package_path_test_fixture()?; + + let package_path = super::path_env_with_package_path_dir( + &fixture.install_context, + Some(fixture.existing_dir.as_os_str().to_owned()), + ) + .expect("package path dir should update PATH"); + let updated_path = super::path_env_with_entry(&fixture.arg0_dir, Some(package_path)); + + assert_eq!( + std::env::split_paths(&updated_path).collect::>(), + vec![ + fixture.arg0_dir, + fixture.path_dir.as_path().to_path_buf(), + fixture.existing_dir + ], + ); + Ok(()) + } + + #[test] + fn package_path_survives_arg0_alias_setup_failure() -> anyhow::Result<()> { + let fixture = package_path_test_fixture()?; + + let (path_entry_guard, updated_path_env_var) = super::prepare_path_env_var_with_aliases( + &fixture.install_context, + Some(fixture.existing_dir.as_os_str().to_owned()), + |path_for_aliases| { + assert_eq!( + std::env::split_paths( + &path_for_aliases.expect("package PATH should be passed to alias setup") + ) + .collect::>(), + vec![ + fixture.path_dir.as_path().to_path_buf(), + fixture.existing_dir.clone() + ], + ); + Err(std::io::Error::other("alias setup failed")) + }, + ); + + assert!(path_entry_guard.is_none()); + let updated_path_env_var = + updated_path_env_var.expect("package PATH should survive alias setup failure"); + assert_eq!( + std::env::split_paths(&updated_path_env_var).collect::>(), + vec![ + fixture.path_dir.as_path().to_path_buf(), + fixture.existing_dir + ], + ); + Ok(()) + } + #[cfg(unix)] #[test] fn run_main_with_arg0_guard_keeps_aliases_alive_until_main_returns() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index e016a725c..6affc3d03 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -1,9 +1,11 @@ +use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; use codex_async_utils::CancelErr; use codex_async_utils::OrCancelExt; use codex_network_proxy::PROXY_ACTIVE_ENV_KEY; +use codex_utils_absolute_path::AbsolutePathBuf; use tokio_util::sync::CancellationToken; use tracing::error; use uuid::Uuid; @@ -15,8 +17,12 @@ use crate::exec_env::create_env; use crate::sandboxing::ExecRequest; use crate::session::TurnInput; use crate::session::turn_context::TurnContext; +use crate::shell::Shell; use crate::state::TaskKind; use crate::tools::format_exec_output_str; +use crate::tools::runtimes::RuntimePathPrepends; +#[cfg(unix)] +use crate::tools::runtimes::apply_package_path_prepend; use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::runtimes::strip_managed_proxy_env; use crate::turn_timing::now_unix_timestamp_ms; @@ -131,13 +137,13 @@ pub(crate) async fn execute_user_shell_command( if exec_env_map.contains_key(PROXY_ACTIVE_ENV_KEY) { strip_managed_proxy_env(&mut exec_env_map); } - let exec_command = maybe_wrap_shell_lc_with_snapshot( + let exec_command = prepare_user_shell_exec_command( &display_command, session_shell.as_ref(), #[allow(deprecated)] &turn_context.cwd, &turn_context.shell_environment_policy.r#set, - &exec_env_map, + &mut exec_env_map, ); let call_id = Uuid::new_v4().to_string(); @@ -328,6 +334,68 @@ pub(crate) async fn execute_user_shell_command( } } +fn prepare_user_shell_exec_command( + display_command: &[String], + session_shell: &Shell, + cwd: &AbsolutePathBuf, + shell_environment_set: &HashMap, + exec_env_map: &mut HashMap, +) -> Vec { + #[cfg(unix)] + { + prepare_user_shell_exec_command_with_path_prepend( + display_command, + session_shell, + cwd, + shell_environment_set, + exec_env_map, + apply_package_path_prepend, + ) + } + + #[cfg(not(unix))] + { + maybe_wrap_shell_lc_with_snapshot( + display_command, + session_shell, + cwd, + shell_environment_set, + exec_env_map, + // On non-Unix targets, arg0 has already prepended the package path + // to the process PATH before create_env() builds exec_env_map. + // RuntimePathPrepends is only needed for Unix shell snapshot replay. + &RuntimePathPrepends::default(), + ) + } +} + +/// Prepares a user-shell command after adding runtime-owned PATH entries. +/// +/// The callback mutates the live exec environment for commands that are not +/// wrapped with a shell snapshot and records only the runtime-owned entries so +/// snapshot wrapping can reapply them after restoring the user's snapshot PATH. +#[cfg(unix)] +fn prepare_user_shell_exec_command_with_path_prepend( + display_command: &[String], + session_shell: &Shell, + cwd: &AbsolutePathBuf, + shell_environment_set: &HashMap, + exec_env_map: &mut HashMap, + prepend_runtime_path: impl FnOnce(&mut HashMap, &mut RuntimePathPrepends), +) -> Vec { + let explicit_env_overrides = shell_environment_set.clone(); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + prepend_runtime_path(exec_env_map, &mut runtime_path_prepends); + maybe_wrap_shell_lc_with_snapshot( + display_command, + session_shell, + cwd, + &explicit_env_overrides, + exec_env_map, + &runtime_path_prepends, + ) +} + async fn persist_user_shell_output( session: &Session, turn_context: &TurnContext, @@ -351,3 +419,7 @@ async fn persist_user_shell_output( .inject_no_new_turn(vec![output_item], Some(turn_context)) .await; } + +#[cfg(all(test, unix))] +#[path = "user_shell_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/tasks/user_shell_tests.rs b/codex-rs/core/src/tasks/user_shell_tests.rs new file mode 100644 index 000000000..442c5cc61 --- /dev/null +++ b/codex-rs/core/src/tasks/user_shell_tests.rs @@ -0,0 +1,71 @@ +use super::*; +use crate::shell::Shell; +use crate::shell::ShellType; +use crate::shell_snapshot::ShellSnapshot; +use core_test_support::PathExt; +use pretty_assertions::assert_eq; +use std::path::PathBuf; +use std::process::Command; +use tokio::sync::watch; + +fn shell_with_snapshot( + shell_type: ShellType, + shell_path: &str, + snapshot_path: AbsolutePathBuf, + snapshot_cwd: AbsolutePathBuf, +) -> Shell { + let (_tx, shell_snapshot) = watch::channel(Some(Arc::new(ShellSnapshot { + path: snapshot_path, + cwd: snapshot_cwd, + }))); + Shell { + shell_type, + shell_path: PathBuf::from(shell_path), + shell_snapshot, + } +} + +#[test] +fn user_shell_snapshot_preserves_package_path_prepend() { + let dir = tempfile::tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport PATH='/snapshot/bin'\n", + ) + .expect("write snapshot"); + let session_shell = shell_with_snapshot( + ShellType::Bash, + "/bin/bash", + snapshot_path.abs(), + dir.path().abs(), + ); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s' \"$PATH\"".to_string(), + ]; + let package_path_dir = dir.path().join("codex-path"); + let mut env = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); + let rewritten = prepare_user_shell_exec_command_with_path_prepend( + &command, + &session_shell, + &dir.path().abs(), + &HashMap::new(), + &mut env, + |env, runtime_path_prepends| { + runtime_path_prepends.prepend(env, package_path_dir.as_path()); + }, + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env("PATH", env.get("PATH").expect("PATH should be set")) + .output() + .expect("run rewritten command"); + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + format!("{}:/snapshot/bin", package_path_dir.display()) + ); +} diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index b237f44c4..29d323b63 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -10,6 +10,8 @@ use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; use crate::tools::sandboxing::ToolError; +#[cfg(unix)] +use codex_install_context::InstallContext; #[cfg(target_os = "macos")] use codex_network_proxy::CODEX_PROXY_GIT_SSH_COMMAND_MARKER; use codex_network_proxy::CUSTOM_CA_ENV_KEYS; @@ -86,17 +88,87 @@ pub(crate) fn strip_managed_proxy_env(env: &mut HashMap) { } } +/// Prepends `path_entry` to `PATH`, removing duplicate and empty existing +/// entries. +/// +/// Returns the updated `PATH` value when `env` was changed. Returns `None` when +/// `path_entry` is empty, leaving `env` untouched so an empty entry does not add +/// the current working directory to command lookup. #[cfg(unix)] -fn prepend_path_entry(env: &mut HashMap, path_entry: &str) -> String { - let updated_path = match env.get("PATH") { - Some(path) if !path.is_empty() => std::iter::once(path_entry) - .chain(path.split(':').filter(|entry| *entry != path_entry)) +fn prepend_path_entry(env: &mut HashMap, path_entry: &str) -> Option { + if path_entry.is_empty() { + None + } else { + let updated_path = match env.get("PATH") { + Some(path) if !path.is_empty() => std::iter::once(path_entry) + .chain( + path.split(':') + .filter(|entry| !entry.is_empty() && *entry != path_entry), + ) + .collect::>() + .join(":"), + _ => path_entry.to_string(), + }; + env.insert("PATH".to_string(), updated_path.clone()); + Some(updated_path) + } +} + +/// PATH entries owned by Codex runtime setup. +/// +/// These are applied to the live exec environment immediately and replayed after +/// restoring a shell snapshot, unless the user explicitly overrides `PATH`. +#[derive(Debug, Default, Eq, PartialEq)] +pub(crate) struct RuntimePathPrepends { + entries: Vec, +} + +impl RuntimePathPrepends { + #[cfg(unix)] + pub(crate) fn prepend(&mut self, env: &mut HashMap, path_entry: &Path) { + let path_entry = path_entry.to_string_lossy().to_string(); + if prepend_path_entry(env, &path_entry).is_some() { + self.entries.retain(|entry| entry != &path_entry); + self.entries.push(path_entry); + } + } + + fn shell_exports_after_snapshot( + &self, + explicit_env_overrides: &HashMap, + ) -> String { + if explicit_env_overrides.contains_key("PATH") { + return String::new(); + } + + self.entries + .iter() + .filter(|entry| !entry.is_empty()) + .map(|entry| { + let entry = shell_single_quote(entry); + format!( + "if [ -n \"${{PATH:-}}\" ]; then export PATH='{entry}':\"$PATH\"; else export PATH='{entry}'; fi" + ) + }) .collect::>() - .join(":"), - _ => path_entry.to_string(), + .join("\n") + } +} + +#[cfg(unix)] +pub(crate) fn apply_package_path_prepend( + env: &mut HashMap, + runtime_path_prepends: &mut RuntimePathPrepends, +) { + let Some(path_dir) = InstallContext::current() + .package_layout + .as_ref() + .and_then(|package_layout| package_layout.path_dir.as_ref()) + else { + return; }; - env.insert("PATH".to_string(), updated_path.clone()); - updated_path + + runtime_path_prepends.prepend(env, path_dir.as_path()); } #[cfg(unix)] @@ -107,21 +179,19 @@ pub(crate) fn prepend_zsh_fork_bin_to_path( let zsh_bin_dir = shell_zsh_path .parent() .map(|path| path.to_string_lossy().to_string())?; - Some(prepend_path_entry(env, &zsh_bin_dir)) + prepend_path_entry(env, &zsh_bin_dir) } #[cfg(unix)] pub(crate) fn apply_zsh_fork_path_prepend( env: &mut HashMap, - explicit_env_overrides: &mut HashMap, + runtime_path_prepends: &mut RuntimePathPrepends, shell_zsh_path: &Path, ) { - let Some(updated_path) = prepend_zsh_fork_bin_to_path(env, shell_zsh_path) else { + let Some(zsh_bin_dir) = shell_zsh_path.parent() else { return; }; - // Snapshot wrapping restores explicit overrides after sourcing the shell - // snapshot, so capture this PATH override there as well. - explicit_env_overrides.insert("PATH".to_string(), updated_path); + runtime_path_prepends.prepend(env, zsh_bin_dir); } pub(crate) fn disable_powershell_profile_for_elevated_windows_sandbox( @@ -172,12 +242,17 @@ pub(crate) fn disable_powershell_profile_for_elevated_windows_sandbox( /// environment. We need access to both so snapshot restore logic can preserve /// runtime-only vars like `CODEX_THREAD_ID` without pretending they came from /// the explicit override policy. +/// +/// `runtime_path_prepends` contains Codex-owned PATH entries already applied to +/// the live `env`; snapshot wrapping replays them after restoring the snapshot +/// PATH unless the user explicitly overrides `PATH`. pub(crate) fn maybe_wrap_shell_lc_with_snapshot( command: &[String], session_shell: &Shell, cwd: &AbsolutePathBuf, explicit_env_overrides: &HashMap, env: &HashMap, + runtime_path_prepends: &RuntimePathPrepends, ) -> Vec { if cfg!(windows) { return command.to_vec(); @@ -219,8 +294,14 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( } let (override_captures, override_exports) = build_override_exports(&override_env); let (proxy_captures, proxy_exports) = build_proxy_env_exports(); + let runtime_path_prepend_exports = + runtime_path_prepends.shell_exports_after_snapshot(explicit_env_overrides); let override_captures = join_shell_blocks([override_captures, proxy_captures]); - let override_exports = join_shell_blocks([override_exports, proxy_exports]); + let override_exports = join_shell_blocks([ + override_exports, + proxy_exports, + runtime_path_prepend_exports, + ]); let rewritten_script = if override_exports.is_empty() { format!( "if . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index 369f301cd..011da46b3 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -166,23 +166,106 @@ fn explicit_escalation_preserves_user_ca_env() { ); } +#[cfg(unix)] +#[test] +fn runtime_path_prepends_records_runtime_path_prepend() { + let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + + runtime_path_prepends.prepend(&mut env, PathBuf::from("/package/codex-path").as_path()); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/package/codex-path:/usr/bin:/bin"), + "runtime PATH prepend should update the live exec environment" + ); + assert_eq!( + runtime_path_prepends.entries, + vec!["/package/codex-path"], + "runtime PATH prepend should be recorded for snapshot replay" + ); +} + +#[cfg(unix)] +#[test] +fn runtime_path_prepends_drops_empty_path_entries() { + let mut env = HashMap::from([( + "PATH".to_string(), + ":/usr/bin:/package/codex-path::/bin:".to_string(), + )]); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + + runtime_path_prepends.prepend(&mut env, PathBuf::from("/package/codex-path").as_path()); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/package/codex-path:/usr/bin:/bin"), + "empty PATH entries should be dropped instead of preserving current-directory lookup" + ); + assert_eq!( + runtime_path_prepends.entries, + vec!["/package/codex-path"], + "deduped runtime PATH prepend should still be recorded once" + ); +} + +#[cfg(unix)] +#[test] +fn runtime_path_prepends_ignores_empty_path_entry() { + let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + + runtime_path_prepends.prepend(&mut env, PathBuf::new().as_path()); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/usr/bin:/bin"), + "empty runtime PATH prepend should leave PATH unchanged" + ); + assert_eq!( + runtime_path_prepends, + RuntimePathPrepends::default(), + "empty runtime PATH prepend should not be recorded for snapshot replay" + ); +} + +#[cfg(unix)] +#[test] +fn prepend_zsh_fork_bin_to_path_ignores_empty_parent() { + let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); + + let result = prepend_zsh_fork_bin_to_path(&mut env, PathBuf::from("zsh").as_path()); + + assert_eq!( + result, None, + "zsh fork helper should not report a PATH update for an empty parent" + ); + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/usr/bin:/bin"), + "zsh fork helper should leave PATH unchanged when the parent is empty" + ); +} + #[cfg(unix)] #[test] fn apply_zsh_fork_path_prepend_uses_shell_parent() { let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); - let mut explicit_env_overrides = HashMap::new(); + let mut runtime_path_prepends = RuntimePathPrepends::default(); apply_zsh_fork_path_prepend( &mut env, - &mut explicit_env_overrides, + &mut runtime_path_prepends, PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), ); let expected = "/package/codex-resources/zsh/bin:/usr/bin:/bin"; assert_eq!(env.get("PATH").map(String::as_str), Some(expected)); assert_eq!( - explicit_env_overrides.get("PATH").map(String::as_str), - Some(expected) + runtime_path_prepends, + RuntimePathPrepends { + entries: vec!["/package/codex-resources/zsh/bin".to_string()] + } ); } @@ -194,11 +277,11 @@ fn apply_zsh_fork_path_prepend_moves_existing_shell_parent_to_front() { "/usr/bin:/package/codex-resources/zsh/bin:/bin:/package/codex-resources/zsh/bin" .to_string(), )]); - let mut explicit_env_overrides = HashMap::new(); + let mut runtime_path_prepends = RuntimePathPrepends::default(); apply_zsh_fork_path_prepend( &mut env, - &mut explicit_env_overrides, + &mut runtime_path_prepends, PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), ); @@ -206,6 +289,12 @@ fn apply_zsh_fork_path_prepend_moves_existing_shell_parent_to_front() { env.get("PATH").map(String::as_str), Some("/package/codex-resources/zsh/bin:/usr/bin:/bin") ); + assert_eq!( + runtime_path_prepends, + RuntimePathPrepends { + entries: vec!["/package/codex-resources/zsh/bin".to_string()] + } + ); } #[test] @@ -250,6 +339,7 @@ fn maybe_wrap_shell_lc_with_snapshot_bootstraps_in_user_shell() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert_eq!(rewritten[0], "/bin/zsh"); @@ -281,6 +371,7 @@ fn maybe_wrap_shell_lc_with_snapshot_escapes_single_quotes() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert!(rewritten[2].contains(r#"exec '/bin/bash' -c 'echo '"'"'hello'"'"''"#)); @@ -309,6 +400,7 @@ fn maybe_wrap_shell_lc_with_snapshot_uses_bash_bootstrap_shell() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert_eq!(rewritten[0], "/bin/bash"); @@ -340,6 +432,7 @@ fn maybe_wrap_shell_lc_with_snapshot_uses_sh_bootstrap_shell() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert_eq!(rewritten[0], "/bin/sh"); @@ -373,6 +466,7 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_trailing_args() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert!( @@ -408,6 +502,7 @@ fn maybe_wrap_shell_lc_with_snapshot_skips_when_cwd_mismatch() { &command_cwd.abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert_eq!(rewritten, command); @@ -437,6 +532,7 @@ fn maybe_wrap_shell_lc_with_snapshot_accepts_dot_alias_cwd() { &command_cwd.abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); assert_eq!(rewritten[0], "/bin/zsh"); @@ -473,6 +569,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_explicit_override_precedence() { &dir.path().abs(), &explicit_env_overrides, &HashMap::from([("TEST_ENV_SNAPSHOT".to_string(), "worktree".to_string())]), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -513,6 +610,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_codex_thread_id_from_env() { &dir.path().abs(), &HashMap::new(), &HashMap::from([("CODEX_THREAD_ID".to_string(), "nested-thread".to_string())]), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -555,6 +653,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_proxy_env_from_process_env() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -612,6 +711,7 @@ fn maybe_wrap_shell_lc_with_snapshot_refreshes_codex_proxy_git_ssh_command() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -657,6 +757,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_custom_git_ssh_command() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -703,6 +804,7 @@ fn maybe_wrap_shell_lc_with_snapshot_clears_stale_codex_git_ssh_command_without_ &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -740,6 +842,7 @@ fn maybe_wrap_shell_lc_with_snapshot_keeps_user_proxy_env_when_proxy_inactive() &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let mut command = Command::new(&rewritten[0]); command.args(&rewritten[1..]); @@ -793,6 +896,7 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_live_env_when_snapshot_proxy_activ "HTTP_PROXY".to_string(), "http://user.proxy:8080".to_string(), )]), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -835,6 +939,7 @@ fn maybe_wrap_shell_lc_with_snapshot_keeps_snapshot_path_without_override() { &dir.path().abs(), &HashMap::new(), &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -872,6 +977,7 @@ fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() { &dir.path().abs(), &explicit_env_overrides, &HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -883,6 +989,84 @@ fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() { assert_eq!(String::from_utf8_lossy(&output.stdout), "/worktree/bin"); } +#[cfg(unix)] +#[test] +fn maybe_wrap_shell_lc_with_snapshot_preserves_package_path_prepend() -> anyhow::Result<()> { + let (stdout, package_path_dir) = + run_snapshot_path_probe_with_runtime_path_prepend(HashMap::new())?; + + assert_eq!( + stdout, + format!("{}:/snapshot/bin", package_path_dir.display()), + "package path prepend should replay ahead of snapshot PATH" + ); + Ok(()) +} + +#[cfg(unix)] +#[test] +fn maybe_wrap_shell_lc_with_snapshot_applies_runtime_path_prepend_after_explicit_path_override() +-> anyhow::Result<()> { + let (stdout, package_path_dir) = run_snapshot_path_probe_with_runtime_path_prepend( + HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]), + )?; + + assert_eq!( + stdout, + format!("{}:/worktree/bin", package_path_dir.display()), + "explicit PATH override should suppress snapshot PATH while preserving runtime prepend" + ); + Ok(()) +} + +#[cfg(unix)] +fn run_snapshot_path_probe_with_runtime_path_prepend( + explicit_env_overrides: HashMap, +) -> anyhow::Result<(String, PathBuf)> { + let dir = tempdir()?; + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport PATH='/snapshot/bin'\n", + )?; + let session_shell = shell_with_snapshot( + ShellType::Bash, + "/bin/bash", + snapshot_path.abs(), + dir.path().abs(), + ); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s' \"$PATH\"".to_string(), + ]; + let package_path_dir = dir.path().join("codex-path"); + let mut env = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + runtime_path_prepends.prepend(&mut env, package_path_dir.as_path()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + &dir.path().abs(), + &explicit_env_overrides, + &env, + &runtime_path_prepends, + ); + let path = env + .get("PATH") + .ok_or_else(|| anyhow::anyhow!("PATH should be set"))?; + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env("PATH", path) + .output()?; + + assert!(output.status.success(), "command failed: {output:?}"); + Ok(( + String::from_utf8_lossy(&output.stdout).into_owned(), + package_path_dir, + )) +} + #[cfg(unix)] #[test] fn maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend() { @@ -912,14 +1096,16 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend() { .join("zsh"); let zsh_bin_dir = zsh_path.parent().expect("zsh path should have parent"); let mut env = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); - let mut explicit_env_overrides = HashMap::new(); - apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, zsh_path.as_path()); + let explicit_env_overrides = HashMap::new(); + let mut runtime_path_prepends = RuntimePathPrepends::default(); + apply_zsh_fork_path_prepend(&mut env, &mut runtime_path_prepends, zsh_path.as_path()); let rewritten = maybe_wrap_shell_lc_with_snapshot( &command, &session_shell, &dir.path().abs(), &explicit_env_overrides, &env, + &runtime_path_prepends, ); let output = Command::new(&rewritten[0]) .args(&rewritten[1..]) @@ -930,7 +1116,8 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend() { assert!(output.status.success(), "command failed: {output:?}"); assert_eq!( String::from_utf8_lossy(&output.stdout), - format!("{}:/worktree/bin", zsh_bin_dir.display()) + format!("{}:/snapshot/bin", zsh_bin_dir.display()), + "zsh fork path prepend should replay ahead of snapshot PATH" ); } @@ -967,6 +1154,7 @@ fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() { "OPENAI_API_KEY".to_string(), "super-secret-value".to_string(), )]), + &RuntimePathPrepends::default(), ); assert!(!rewritten[2].contains("super-secret-value")); @@ -1012,6 +1200,7 @@ fn maybe_wrap_shell_lc_with_snapshot_preserves_unset_override_variables() { &dir.path().abs(), &explicit_env_overrides, &HashMap::new(), + &RuntimePathPrepends::default(), ); let output = Command::new(&rewritten[0]) diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 0380f1840..13b65f044 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -20,6 +20,7 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +use crate::tools::runtimes::RuntimePathPrepends; #[cfg(unix)] use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; @@ -249,22 +250,29 @@ impl ToolRuntime for ShellRuntime { let env = exec_env_for_sandbox_permissions(&req.env, sandbox_permissions); let explicit_env_overrides = req.explicit_env_overrides.clone(); #[cfg(unix)] - let (env, explicit_env_overrides) = { + let (env, runtime_path_prepends) = { let mut env = env; - let mut explicit_env_overrides = explicit_env_overrides; + let mut runtime_path_prepends = RuntimePathPrepends::default(); + crate::tools::runtimes::apply_package_path_prepend( + &mut env, + &mut runtime_path_prepends, + ); if self.backend == ShellRuntimeBackend::ShellCommandZshFork && let Some(shell_zsh_path) = ctx.session.services.shell_zsh_path.as_deref() { - apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, shell_zsh_path); + apply_zsh_fork_path_prepend(&mut env, &mut runtime_path_prepends, shell_zsh_path); } - (env, explicit_env_overrides) + (env, runtime_path_prepends) }; + #[cfg(not(unix))] + let runtime_path_prepends = RuntimePathPrepends::default(); let command = maybe_wrap_shell_lc_with_snapshot( &req.command, session_shell.as_ref(), &req.cwd, &explicit_env_overrides, &env, + &runtime_path_prepends, ); let command = disable_powershell_profile_for_elevated_windows_sandbox( &command, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index e49c92572..bf04a6fe3 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -17,6 +17,7 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +use crate::tools::runtimes::RuntimePathPrepends; #[cfg(unix)] use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; @@ -278,20 +279,28 @@ impl<'a> ToolRuntime for UnifiedExecRunt if let Some(network) = managed_network { network.apply_to_env(&mut env); } + let environment_is_remote = req.environment.is_remote(); let explicit_env_overrides = req.explicit_env_overrides.clone(); #[cfg(unix)] - let explicit_env_overrides = { - let mut explicit_env_overrides = explicit_env_overrides; + let runtime_path_prepends = { + let mut runtime_path_prepends = RuntimePathPrepends::default(); + if !environment_is_remote { + crate::tools::runtimes::apply_package_path_prepend( + &mut env, + &mut runtime_path_prepends, + ); + } if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { apply_zsh_fork_path_prepend( &mut env, - &mut explicit_env_overrides, + &mut runtime_path_prepends, zsh_fork_config.shell_zsh_path.as_path(), ); } - explicit_env_overrides + runtime_path_prepends }; - let environment_is_remote = req.environment.is_remote(); + #[cfg(not(unix))] + let runtime_path_prepends = RuntimePathPrepends::default(); let command = if environment_is_remote { base_command.to_vec() } else { @@ -301,6 +310,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt &req.cwd, &explicit_env_overrides, &env, + &runtime_path_prepends, ) }; let command = disable_powershell_profile_for_elevated_windows_sandbox( diff --git a/codex-rs/install-context/src/lib.rs b/codex-rs/install-context/src/lib.rs index d92a80cbe..63694c5d6 100644 --- a/codex-rs/install-context/src/lib.rs +++ b/codex-rs/install-context/src/lib.rs @@ -184,11 +184,14 @@ impl InstallContext { impl CodexPackageLayout { fn from_exe(exe_path: &Path) -> Option { let canonical_exe = canonical_absolute_path(exe_path)?; - let bin_dir = canonical_exe.parent()?; - if bin_dir.file_name() != Some(OsStr::new(BIN_DIRNAME)) { - return None; + let exe_dir = canonical_exe.parent()?; + match exe_dir.file_name() { + Some(name) if name == OsStr::new(BIN_DIRNAME) => Self::from_package_bin_dir(exe_dir), + Some(_) | None => None, } + } + fn from_package_bin_dir(bin_dir: AbsolutePathBuf) -> Option { let package_dir = bin_dir.parent()?; if !package_dir.join(PACKAGE_METADATA_FILENAME).is_file() { return None;