mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
cli: add package path from install context (#26189)
## Why Codex package installs include helper binaries in `codex-path`, such as the bundled `rg`. Package-layout launches should add that directory before user commands run, but standalone launches were missing it while npm launches only worked because `codex.js` had its own legacy `PATH` rewrite. That made npm and standalone package behavior diverge. Shell snapshot restoration can also reset `PATH` after runtime setup. Any package-owned `PATH` prepend has to be recorded as an explicit runtime override so shells, unified exec, and user-shell commands keep access to `codex-path` after a snapshot is sourced. ## Repro Before this change, a curl-installed package could contain `rg` under `codex-path` but still fail to put it on `PATH`: ```shell mkdir /tmp/test-codex-curl curl -fsSL https://chatgpt.com/codex/install.sh \ | CODEX_HOME=/tmp/test-codex-curl CODEX_NON_INTERACTIVE=1 sh /tmp/test-codex-curl/packages/standalone/current/bin/codex exec \ --skip-git-repo-check 'print `which -a rg`' find /tmp/test-codex-curl -name rg ``` The `which -a rg` output omitted the packaged helper even though `find` showed it under `/tmp/test-codex-curl/packages/standalone/releases/.../codex-path/rg`. The npm install path behaved differently only because `codex-cli/bin/codex.js` had legacy `PATH` rewriting: ```shell mkdir /tmp/test-codex-npm cd /tmp/test-codex-npm npm install @openai/codex ./node_modules/.bin/codex exec --skip-git-repo-check 'print `which -a rg`' ``` That printed the npm package's `vendor/<target>/codex-path/rg` first. This PR moves that behavior into Rust-side package launch setup so curl/standalone and npm/bun launches agree without JS rewriting `PATH`. ## What Changed - `codex-rs/arg0` now uses `InstallContext::current().package_layout.path_dir` to prepend the package helper directory before any threads are created. - Package helper `PATH` setup is independent from the temporary arg0 alias setup, so `codex-path` is still added even if CODEX_HOME tempdir, lock, or symlink setup fails. - `codex-rs/install-context` detects the canonical package layout we ship: `bin/`, `codex-resources/`, and `codex-path/` next to `codex-package.json`. - Shell, local unified exec, and user-shell runtimes now record package `codex-path` prepends in `explicit_env_overrides`, matching the existing zsh-fork behavior so shell snapshots cannot restore over the package helper path. - Remote unified exec requests do not receive the local app-server package path overlay. - `codex-cli/bin/codex.js` no longer computes or overrides `PATH`; it only locates the native binary in the canonical package layout and passes npm/bun management metadata. - Added regression tests for `PATH` ordering, package layout detection, and shell snapshot preservation of package path prepends. ## Verification - `node --check codex-cli/bin/codex.js` - `just test -p codex-install-context -p codex-arg0` - `just test -p codex-core user_shell_snapshot_preserves_package_path_prepend` - `just test -p codex-core tools::runtimes::tests` - `just bazel-lock-update` - `just bazel-lock-check` - `just fix -p codex-install-context -p codex-arg0 -p codex-core`
This commit is contained in:
committed by
GitHub
Unverified
parent
80b65e9945
commit
6bcccb0ee6
@@ -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<String, String>,
|
||||
exec_env_map: &mut HashMap<String, String>,
|
||||
) -> Vec<String> {
|
||||
#[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<String, String>,
|
||||
exec_env_map: &mut HashMap<String, String>,
|
||||
prepend_runtime_path: impl FnOnce(&mut HashMap<String, String>, &mut RuntimePathPrepends),
|
||||
) -> Vec<String> {
|
||||
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;
|
||||
|
||||
@@ -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())
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user