Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d531b0a3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
baefa8f to
f17270c
Compare
viyatb-oai
left a comment
There was a problem hiding this comment.
A few places where the package PATH can still be lost or broadened unexpectedly.
viyatb-oai
left a comment
There was a problem hiding this comment.
The original comments are addressed. I found two remaining cases in the new snapshot-preservation path.
44920dc to
acb5aa0
Compare
482fc53 to
5dac4a3
Compare
viyatb-oai
left a comment
There was a problem hiding this comment.
Nothing blocking from my side. The latest change fixes the snapshot PATH issue by replaying only Codex-owned prepends after snapshot restoration, while preserving explicit PATH overrides and keeping remote unified exec local-path-free.
2f518f9 to
6582ff0
Compare
781cd39 to
dce9f86
Compare
Why
Codex package installs include helper binaries in
codex-path, such as the bundledrg. Package-layout launches should add that directory before user commands run, but standalone launches were missing it while npm launches only worked becausecodex.jshad its own legacyPATHrewrite. That made npm and standalone package behavior diverge.Shell snapshot restoration can also reset
PATHafter runtime setup. Any package-ownedPATHprepend has to be recorded as an explicit runtime override so shells, unified exec, and user-shell commands keep access tocodex-pathafter a snapshot is sourced.Repro
Before this change, a curl-installed package could contain
rgundercodex-pathbut still fail to put it onPATH:The
which -a rgoutput omitted the packaged helper even thoughfindshowed it under/tmp/test-codex-curl/packages/standalone/releases/.../codex-path/rg.The npm install path behaved differently only because
codex-cli/bin/codex.jshad legacyPATHrewriting:That printed the npm package's
vendor/<target>/codex-path/rgfirst. This PR moves that behavior into Rust-side package launch setup so curl/standalone and npm/bun launches agree without JS rewritingPATH.What Changed
codex-rs/arg0now usesInstallContext::current().package_layout.path_dirto prepend the package helper directory before any threads are created.PATHsetup is independent from the temporary arg0 alias setup, socodex-pathis still added even if CODEX_HOME tempdir, lock, or symlink setup fails.codex-rs/install-contextdetects the canonical package layout we ship:bin/,codex-resources/, andcodex-path/next tocodex-package.json.codex-pathprepends inexplicit_env_overrides, matching the existing zsh-fork behavior so shell snapshots cannot restore over the package helper path.codex-cli/bin/codex.jsno longer computes or overridesPATH; it only locates the native binary in the canonical package layout and passes npm/bun management metadata.PATHordering, package layout detection, and shell snapshot preservation of package path prepends.Verification
node --check codex-cli/bin/codex.jsjust test -p codex-install-context -p codex-arg0just test -p codex-core user_shell_snapshot_preserves_package_path_prependjust test -p codex-core tools::runtimes::testsjust bazel-lock-updatejust bazel-lock-checkjust fix -p codex-install-context -p codex-arg0 -p codex-core