diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index 1a43ce7e1f..4a8813f03f 100755 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -83,21 +83,14 @@ 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"), - }; + return binaryPath; } const legacyPath = legacyBinaryPath(vendorRoot); if (existsSync(legacyPath)) { - return { - binaryPath: legacyPath, - pathDir: path.join(packageRoot, "path"), - }; + return legacyPath; } return null; @@ -124,7 +117,7 @@ if (!nativePackage) { ); } -const { binaryPath, pathDir } = nativePackage; +const binaryPath = nativePackage; // 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 +125,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 +150,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 2183d91b64..e9c171d778 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 7ee21a770e..55526b4d06 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 87940f1182..5a385a8fe0 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)] @@ -285,10 +287,11 @@ 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. +/// This temporary directory, followed by the package-managed `codex-path` +/// directory when present, is prepended to the PATH environment variable so +/// that `apply_patch` and bundled package helpers can be on the PATH without +/// requiring the user to install separate executables, 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 @@ -371,23 +374,8 @@ 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(), - }; + let updated_path_env_var = + path_env_with_codex_entries(path, InstallContext::current(), std::env::var_os("PATH")); unsafe { std::env::set_var("PATH", updated_path_env_var); @@ -420,6 +408,39 @@ pub fn prepend_path_entry_for_codex_aliases() -> std::io::Result, +) -> OsString { + #[cfg(unix)] + const PATH_SEPARATOR: &str = ":"; + + #[cfg(windows)] + const PATH_SEPARATOR: &str = ";"; + + let package_path_dir = install_context + .package_layout + .as_ref() + .and_then(|package_layout| package_layout.path_dir.as_ref()); + let capacity = arg0_dir.as_os_str().len() + + package_path_dir.map_or(0, |path_dir| 1 + path_dir.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(arg0_dir); + if let Some(path_dir) = package_path_dir { + path_env_var.push(PATH_SEPARATOR); + path_env_var.push(path_dir.as_path()); + } + 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<()> { let entries = match std::fs::read_dir(temp_root) { Ok(entries) => entries, @@ -475,6 +496,11 @@ 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; @@ -513,6 +539,42 @@ mod tests { Ok(()) } + #[test] + fn path_env_includes_arg0_dir_package_path_dir_and_existing_path() -> 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()), + }), + }; + + let updated_path = super::path_env_with_codex_entries( + &arg0_dir, + &install_context, + Some(existing_dir.as_os_str().to_owned()), + ); + + assert_eq!( + std::env::split_paths(&updated_path).collect::>(), + vec![arg0_dir, path_dir.as_path().to_path_buf(), existing_dir,], + ); + Ok(()) + } + #[cfg(unix)] #[test] fn run_main_with_arg0_guard_keeps_aliases_alive_until_main_returns() -> anyhow::Result<()> {