Conversation
Summary: - prepend the configured zsh fork executable directory to PATH for zsh-fork shell-command and unified-exec launches - preserve that PATH value through shell snapshot wrapping so #!/usr/bin/env zsh can resolve to the packaged fork - cover duplicate PATH entries and snapshot preservation in codex-core runtime tests Test Plan: - just fmt - cargo test -p codex-core apply_zsh_fork_path_prepend - cargo test -p codex-core maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend - just fix -p codex-core Notes: - cargo test -p codex-core currently aborts in unrelated thread_manager::tests::resume_and_fork_do_not_restore_thread_environments_from_rollout with a stack overflow; rerunning that single test reproduces the same stack overflow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#23756 makes packaged Codex builds include and default to the bundled zsh fork. The follow-up runtime behavior is that zsh-fork executions should also put the folder containing that fork at the front of
PATH, so scripts that use#!/usr/bin/env zshresolve to the packaged fork instead of the systemzsh.Shell snapshots make this slightly more subtle: a snapshot can restore an older
PATHafter the child shell starts. This PR treats the zsh fork PATH prepend as an explicit environment override so snapshot wrapping preserves it.What Changed
PATHand records the resulting value as an explicit override.shell_commandlaunches and unified-exec zsh-fork launches before sandbox command construction.PATHentries and for preserving the zsh fork prepend through shell snapshot wrapping.Testing
cargo test -p codex-core apply_zsh_fork_path_prependcargo test -p codex-core maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prependjust fix -p codex-coreI also tried
cargo test -p codex-core, but it currently aborts in unrelatedthread_manager::tests::resume_and_fork_do_not_restore_thread_environments_from_rolloutwith a stack overflow. Rerunning that single test reproduces the same stack overflow.Stack created with Sapling. Best reviewed with ReviewStack.