From f17270ccd4be9c9ad2d3c2426ed250ccd6e5c0e4 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2026 09:19:24 -0700 Subject: [PATCH] cli: add package path from install context --- codex-cli/bin/codex.js | 37 +++------- codex-rs/Cargo.lock | 2 + codex-rs/arg0/Cargo.toml | 4 ++ codex-rs/arg0/src/lib.rs | 104 ++++++++++++++++++++++------ codex-rs/install-context/src/lib.rs | 94 ++++++++++++++++++++++++- 5 files changed, 188 insertions(+), 53 deletions(-) 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<()> { diff --git a/codex-rs/install-context/src/lib.rs b/codex-rs/install-context/src/lib.rs index d92a80cbef..1bef56c1c4 100644 --- a/codex-rs/install-context/src/lib.rs +++ b/codex-rs/install-context/src/lib.rs @@ -6,6 +6,8 @@ use std::sync::OnceLock; use codex_utils_absolute_path::AbsolutePathBuf; const BIN_DIRNAME: &str = "bin"; +const LEGACY_BINARY_DIRNAME: &str = "codex"; +const LEGACY_PATH_DIRNAME: &str = "path"; const PACKAGE_METADATA_FILENAME: &str = "codex-package.json"; const PATH_DIRNAME: &str = "codex-path"; const RELEASES_DIRNAME: &str = "releases"; @@ -184,11 +186,17 @@ 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(name) if name == OsStr::new(LEGACY_BINARY_DIRNAME) => { + Self::from_legacy_binary_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; @@ -201,6 +209,18 @@ impl CodexPackageLayout { bin_dir, }) } + + fn from_legacy_binary_dir(bin_dir: AbsolutePathBuf) -> Option { + let package_dir = bin_dir.parent()?; + let path_dir = existing_dir(package_dir.join(LEGACY_PATH_DIRNAME))?; + + Some(Self { + resources_dir: existing_dir(package_dir.join(RESOURCES_DIRNAME)), + path_dir: Some(path_dir), + package_dir, + bin_dir, + }) + } } fn install_method_from_exe( @@ -511,6 +531,74 @@ mod tests { Ok(()) } + #[test] + fn npm_managed_legacy_package_keeps_path_dir() -> std::io::Result<()> { + let package_dir = tempfile::tempdir()?; + let bin_dir = package_dir.path().join(LEGACY_BINARY_DIRNAME); + let path_dir = package_dir.path().join(LEGACY_PATH_DIRNAME); + fs::create_dir_all(&bin_dir)?; + fs::create_dir_all(&path_dir)?; + let exe_path = bin_dir.join(if cfg!(windows) { "codex.exe" } else { "codex" }); + fs::write(&exe_path, "")?; + fs::write(path_dir.join(default_rg_command()), "")?; + let canonical_package_dir = + AbsolutePathBuf::from_absolute_path(package_dir.path().canonicalize()?)?; + let canonical_bin_dir = AbsolutePathBuf::from_absolute_path(bin_dir.canonicalize()?)?; + let canonical_path_dir = AbsolutePathBuf::from_absolute_path(path_dir.canonicalize()?)?; + + let context = InstallContext::from_exe_with_codex_home( + /*is_macos*/ false, + /*current_exe*/ Some(&exe_path), + /*managed_by_npm*/ true, + /*managed_by_bun*/ false, + /*codex_home*/ None, + ); + assert_eq!( + context, + InstallContext { + method: InstallMethod::Npm, + package_layout: Some(CodexPackageLayout { + package_dir: canonical_package_dir, + bin_dir: canonical_bin_dir, + resources_dir: None, + path_dir: Some(canonical_path_dir.clone()), + }), + } + ); + assert_eq!( + context.rg_command(), + canonical_path_dir + .join(default_rg_command()) + .into_path_buf() + ); + Ok(()) + } + + #[test] + fn legacy_package_layout_requires_path_dir() -> std::io::Result<()> { + let package_dir = tempfile::tempdir()?; + let bin_dir = package_dir.path().join(LEGACY_BINARY_DIRNAME); + fs::create_dir_all(&bin_dir)?; + let exe_path = bin_dir.join(if cfg!(windows) { "codex.exe" } else { "codex" }); + fs::write(&exe_path, "")?; + + let context = InstallContext::from_exe_with_codex_home( + /*is_macos*/ false, + /*current_exe*/ Some(&exe_path), + /*managed_by_npm*/ true, + /*managed_by_bun*/ false, + /*codex_home*/ None, + ); + assert_eq!( + context, + InstallContext { + method: InstallMethod::Npm, + package_layout: None, + } + ); + Ok(()) + } + #[test] fn standalone_package_rg_falls_back_when_codex_path_is_missing() -> std::io::Result<()> { let package_dir = tempfile::tempdir()?;