Skip to content

runtime: prepend zsh fork bin dir to PATH#23768

Merged
bolinfest merged 1 commit into
mainfrom
pr23768
May 28, 2026
Merged

runtime: prepend zsh fork bin dir to PATH#23768
bolinfest merged 1 commit into
mainfrom
pr23768

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 20, 2026

Why

#23756 makes packaged Codex builds include and default to the bundled zsh fork. The important reason to put that fork's directory at the front of PATH is to keep executable-level escalation working after a command leaves the original shell and later re-enters zsh through env.

The expected chain is:

  1. The zsh fork runs the top-level shell command.
  2. That command launches another program, such as python3, while inheriting the EXEC_WRAPPER environment and the escalation socket fd.
  3. That program spawns a shell script whose shebang is #!/usr/bin/env zsh rather than #!/bin/zsh, and it does not close the escalation fd.
  4. /usr/bin/env resolves zsh through PATH, so it must find the packaged zsh fork before the system zsh.
  5. Commands inside that nested script are intercepted by the zsh fork and can still request escalation from Codex.

If PATH resolves zsh to the system shell instead, the nested script loses zsh-fork exec interception. Commands that should request escalation can then run only in the original sandbox, or fail there, without Codex ever receiving the approval request.

Shell snapshots make this slightly more subtle: a snapshot can restore an older PATH after the child shell starts. This PR treats the zsh fork PATH prepend as an explicit environment override so snapshot wrapping preserves it.

What Changed

  • Added shared zsh-fork runtime helpers that prepend the configured zsh executable parent directory to PATH without duplicate entries.
  • Applied the zsh fork PATH prepend to both zsh-fork shell_command launches and unified-exec zsh-fork launches before sandbox command construction.
  • Kept the shell-command zsh-fork backend API narrow: it derives the configured zsh path from session services and rebuilds its sandbox environment from req.env, rather than accepting a second, competing environment map or a separately threaded bin dir.
  • Kept Unix-only zsh-fork PATH mutation out of Windows clippy-visible mutability.
  • Added coverage for duplicate PATH entries, for preserving the zsh fork prepend through shell snapshot wrapping, and for the nested python3 -> #!/usr/bin/env zsh escalation flow.

Testing

  • just fmt
  • just fix -p codex-core

I left final test validation to CI after the latest review-comment cleanup. Before that cleanup, just test -p codex-core zsh_fork passed locally for the zsh-fork-focused tests.

@bolinfest bolinfest requested a review from a team as a code owner May 20, 2026 22:18
@bolinfest bolinfest changed the base branch from main to pr23756 May 20, 2026 22:18
@bolinfest bolinfest force-pushed the pr23768 branch 2 times, most recently from 113d454 to 94e0873 Compare May 22, 2026 17:08
bolinfest added a commit that referenced this pull request May 22, 2026
## Why

The package builder already fetches `rg` from a checked-in DotSlash
manifest. The zsh packaging work needs the same
fetch/cache/size-check/SHA-256/extract path for another manifest, but
keeping that refactor inside the zsh PR makes the review harder to
follow.

This PR factors the existing `rg`-specific implementation into a
reusable helper with no intended behavior change for `rg` packaging.

## What Changed

- Added `scripts/codex_package/dotslash.py` for checked-in DotSlash
manifest parsing, archive download, cache reuse, size validation,
SHA-256 validation, and member extraction.
- Updated `scripts/codex_package/ripgrep.py` to delegate to the shared
helper.
- Preserved the existing `rg` manifest path, cache key, destination
filename, and executable-bit behavior.

## Testing

- `python3 -m py_compile scripts/codex_package/dotslash.py
scripts/codex_package/ripgrep.py scripts/codex_package/cli.py
scripts/codex_package/layout.py scripts/codex_package/zsh.py`
- `python3 -m unittest discover scripts/codex_package`


---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24129).
* #23768
* #23756
* __->__ #24129
@bolinfest bolinfest force-pushed the pr23756 branch 2 times, most recently from 4d32f20 to 85e446e Compare May 22, 2026 22:08
@bolinfest bolinfest force-pushed the pr23768 branch 2 times, most recently from 813eb7c to 2180101 Compare May 22, 2026 23:12
@bolinfest bolinfest force-pushed the pr23756 branch 2 times, most recently from 7180f76 to eefd649 Compare May 23, 2026 00:30
bolinfest added a commit that referenced this pull request May 23, 2026
## Why

The package layout gives Codex a stable place for runtime helpers that
should travel with the entrypoint. `shell_zsh_fork` still required users
to configure `zsh_path` manually, even though we already publish
prebuilt zsh fork artifacts.

This PR builds on #24129 and uses the shared DotSlash artifact fetcher
to include the zsh fork in Codex packages when a matching target
artifact exists. Packaged Codex builds can then discover the bundled
fork automatically; the user/profile `zsh_path` override is removed so
the feature uses the package-managed artifact instead of a legacy path
knob.

## What Changed

- Added `scripts/codex_package/codex-zsh`, a checked-in DotSlash
manifest for the current macOS arm64 and Linux zsh fork artifacts.
- Taught `scripts/build_codex_package.py` to fetch the matching zsh fork
artifact and install it at `codex-resources/zsh/bin/zsh` when available
for the selected target.
- Added package layout validation for the optional bundled zsh resource.
- Added `InstallContext::bundled_zsh_path()` and
`InstallContext::bundled_zsh_bin_dir()` for package-layout resource
discovery.
- Threaded the packaged zsh path through config loading as the runtime
`zsh_path` for packaged installs, and removed the config/profile/CLI
override path.
- Kept the packaged default zsh override typed as `AbsolutePathBuf`
until the existing runtime `Config::zsh_path` boundary.
- Updated app-server zsh-fork integration tests to spawn
`codex-app-server` from a temporary package layout with
`codex-resources/zsh/bin/zsh`, matching the new packaged discovery path
instead of setting `zsh_path` in config.
- Switched package executable copying from metadata-preserving `copy2()`
to `copyfile()` plus explicit executable bits, which avoids macOS
file-flag failures when local smoke tests use system binaries as inputs.

## Testing

To verify that the `zsh` executable from the Codex package is picked up
correctly, first I ran:

```shell
./scripts/build_codex_package.py
```

which created:

```
/private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/
```

so then I ran:

```
/private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/bin/codex exec --enable shell_zsh_fork 'run `echo $0`'
```

which reported the following, as expected:

```
/private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/codex-resources/zsh/bin/zsh
```



---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23756).
* #23768
* __->__ #23756
Base automatically changed from pr23756 to main May 23, 2026 00:54
Comment thread codex-rs/core/src/tools/runtimes/mod.rs Outdated
Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs Outdated
Comment thread codex-rs/core/src/tools/runtimes/shell.rs Outdated
@bolinfest bolinfest force-pushed the pr23768 branch 2 times, most recently from c03fc26 to d55fc34 Compare May 28, 2026 19:28
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
- keep Unix-only zsh-fork PATH mutation out of Windows clippy-visible mutability
- cover duplicate PATH entries and snapshot preservation in codex-core runtime tests

Test Plan:
- just fmt
- just fix -p codex-core
- cargo test -p codex-core zsh_fork_path_prepend

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.
@bolinfest bolinfest merged commit bc10e5b into main May 28, 2026
46 checks passed
@bolinfest bolinfest deleted the pr23768 branch May 28, 2026 21:10
@github-actions github-actions Bot locked and limited conversation to collaborators May 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants