Skip to content

refactor: route zsh-fork through unified exec#13432

Open
bolinfest wants to merge 1 commit intopr13644from
pr13432
Open

refactor: route zsh-fork through unified exec#13432
bolinfest wants to merge 1 commit intopr13644from
pr13432

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 2026

Why

When unified_exec and shell_zsh_fork are enabled together, execution must keep using the configured zsh_path so exec interception remains reliable for approvals, policy, and escalation behavior.

In practice, unified-exec sessions could drift to host zsh (for example /bin/zsh) via shell overrides or nested zsh execs. That breaks the guarantees the zsh-fork path is supposed to provide.

What Changed

  • Kept routing on unified exec when both features are enabled (core/src/tools/spec.rs, core/src/tools/handlers/unified_exec.rs, core/src/memories/usage.rs, app-server suite updates).
  • For UnifiedExecBackendConfig::ZshFork, ignored model-provided shell overrides so the session starts from configured zsh_path (core/src/tools/handlers/unified_exec.rs).
  • Replaced permissive shell-shape scanning in zsh-fork preparation with deterministic parsing of the original shell command (program, -c/-lc, script) (core/src/tools/runtimes/shell/unix_escalation.rs, core/src/tools/runtimes/shell/zsh_fork_backend.rs, core/src/tools/runtimes/unified_exec.rs).
  • Ensured nested intercepted zsh execs stay on configured zsh-fork:
    • force escalation when intercepted program is zsh but not configured zsh_path
    • rewrite that intercepted program to configured zsh_path before execution
      (core/src/tools/runtimes/shell/unix_escalation.rs).
  • Kept skill lookup pinned to turn cwd and re-read execpolicy for intercepted execs so approved amendments apply to later matching subcommands in the same session (core/src/tools/runtimes/shell/unix_escalation.rs).
  • Expanded tests for the routed unified-exec zsh-fork path:
    • app-server v2 turn-start coverage, including interrupt cleanup (app-server/tests/suite/v2/turn_start_zsh_fork.rs)
    • approval/amendment behavior (core/tests/suite/approvals.rs)
    • skill prompt and turn-cwd behavior (core/tests/suite/skill_approval.rs)
    • interactive unified-exec behavior including executable-path assertions and nested zsh rewrite coverage (core/tests/suite/unified_exec.rs)
    • unit tests for deterministic shell parsing and zsh rewrite predicates (core/src/tools/runtimes/shell/unix_escalation_tests.rs)
  • Added Windows-safe #[cfg(unix)] import usage in zsh-fork integration tests.

Verification

  • cargo test -p codex-core tools::runtimes::shell::unix_escalation::tests
  • cargo test -p codex-core --test all unified_exec_zsh_fork_keeps_python_repl_attached_to_zsh_session
  • cargo test -p codex-core --test all unified_exec_zsh_fork_rewrites_nested_zsh_exec_to_configured_binary

Manual Testing

Run Codex built from source with both features enabled as follows:

just codex --enable unified_exec --enable shell_zsh_fork --config zsh_path=$(realpath codex-rs/app-server/tests/suite/zsh)

Then ask Codex to do something like:

Start a new instance of a shell using exec_command where it just runs `ls` but keep it around.

Then tell it:

Run python3 in the shell. Once the REPL starts, run `import os; print(os.getpid())` and tell me what you get.

In my case, it replied with:

python3 started in the existing shell session, and import os; print(os.getpid()) printed:

55692

Then over in a plain shell, I check pstree:

$ pstree -p 55692
-+= 00001 root /sbin/launchd
 \-+= 01681 mbolin /Applications/iTerm.app/Contents/MacOS/iTerm2
   \-+- 01814 mbolin /Users/mbolin/Library/Application Support/iTerm2/iTermServer-3.6.6 /Users/mbolin/Library/Application Support/iTerm2/iterm2-daemon-1.socket
     \-+= 35040 root /usr/bin/login -fpl mbolin /Applications/iTerm.app/Contents/MacOS/ShellLauncher --launch_shell
       \-+= 35041 mbolin -zsh
         \-+= 54453 mbolin just codex --enable unified_exec --enable shell_zsh_fork --config zsh_path=/Users/mbolin/code/codex4/codex-rs/app-server/tests/suite/zsh
           \-+- 54463 mbolin target/debug/codex --enable unified_exec --enable shell_zsh_fork --config zsh_path=/Users/mbolin/code/codex4/codex-rs/app-server/tests/suite/zsh
             \-+= 55386 mbolin /Users/mbolin/code/codex4/codex-rs/app-server/tests/suite/zsh
               \--- 55692 mbolin python3

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr13392 March 4, 2026 06:42
@bolinfest bolinfest requested a review from jif-oai March 4, 2026 06:44
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from 65be9c0 to c04ba90 Compare March 4, 2026 20:11
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from 29419f7 to 1b5a00e Compare March 5, 2026 06:31
@bolinfest bolinfest force-pushed the pr13392 branch 2 times, most recently from e95b1a1 to 58049ba Compare March 5, 2026 06:58
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from 5c8732e to 8222c01 Compare March 5, 2026 07:45
@bolinfest bolinfest force-pushed the pr13392 branch 2 times, most recently from 3960093 to cbf6429 Compare March 5, 2026 08:33
Base automatically changed from pr13392 to main March 5, 2026 08:55
bolinfest added a commit that referenced this pull request Mar 5, 2026
## Why

`shell_zsh_fork` already provides stronger guarantees around which
executables receive elevated permissions. To reuse that machinery from
unified exec without pushing Unix-specific escalation details through
generic runtime code, the escalation bootstrap and session lifetime
handling need a cleaner boundary.

That boundary also needs to be safe for long-lived sessions: when an
intercepted shell session is closed or pruned, any in-flight approval
workers and any already-approved escalated child they spawned must be
torn down with the session, and the inherited escalation socket must not
leak into unrelated subprocesses.

## What Changed

- Extracted a reusable `EscalationSession` and
`EscalateServer::start_session(...)` in `shell-escalation` so callers
can get the wrapper/socket env overlay and keep the escalation server
alive without immediately running a one-shot command.
- Documented that `EscalationSession::env()` and
`ShellCommandExecutor::run(...)` exchange only that env overlay, which
callers must merge into their own base shell environment.
- Clarified the prepared-exec helper boundary in `core` by naming the
new helper APIs around `ExecRequest`, while keeping the legacy
`execute_env(...)` entrypoints as thin compatibility wrappers for
existing callers that still use the older naming.
- Added a small post-spawn hook on the prepared execution path so the
parent copy of the inheritable escalation socket is closed immediately
after both the existing one-shot shell-command spawn and the
unified-exec spawn.
- Made session teardown explicit with session-scoped cancellation:
dropping an `EscalationSession` or canceling its parent request now
stops intercept workers, and the server-spawned escalated child uses
`kill_on_drop(true)` so teardown cannot orphan an already-approved
child.
- Added `UnifiedExecBackendConfig` plumbing through `ToolsConfig`, a
`shell::zsh_fork_backend` facade, and an opaque unified-exec
spawn-lifecycle hook so unified exec can prepare a wrapped `zsh -c/-lc`
request without storing `EscalationSession` directly in generic
process/runtime code.
- Kept the existing `shell_command` zsh-fork behavior intact on top of
the new bootstrap path. Tool selection is unchanged in this PR: when
`shell_zsh_fork` is enabled, `ShellCommand` still wins over
`exec_command`.

## Verification

- `cargo test -p codex-shell-escalation`
  - includes coverage for `start_session_exposes_wrapper_env_overlay`
  - includes coverage for `exec_closes_parent_socket_after_shell_spawn`
- includes coverage for
`dropping_session_aborts_intercept_workers_and_kills_spawned_child`
- `cargo test -p codex-core
shell_zsh_fork_prefers_shell_command_over_unified_exec`
- `cargo test -p codex-core --test all
shell_zsh_fork_prompts_for_skill_script_execution`


---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13392).
* #13432
* __->__ #13392
@bolinfest bolinfest force-pushed the pr13432 branch 3 times, most recently from 184d0e7 to 8b935f8 Compare March 6, 2026 00:06
@bolinfest bolinfest changed the base branch from main to pr13644 March 6, 2026 00:06
ethriel3695 pushed a commit to ethriel3695/codex that referenced this pull request Mar 6, 2026
## Why

`shell_zsh_fork` already provides stronger guarantees around which
executables receive elevated permissions. To reuse that machinery from
unified exec without pushing Unix-specific escalation details through
generic runtime code, the escalation bootstrap and session lifetime
handling need a cleaner boundary.

That boundary also needs to be safe for long-lived sessions: when an
intercepted shell session is closed or pruned, any in-flight approval
workers and any already-approved escalated child they spawned must be
torn down with the session, and the inherited escalation socket must not
leak into unrelated subprocesses.

## What Changed

- Extracted a reusable `EscalationSession` and
`EscalateServer::start_session(...)` in `shell-escalation` so callers
can get the wrapper/socket env overlay and keep the escalation server
alive without immediately running a one-shot command.
- Documented that `EscalationSession::env()` and
`ShellCommandExecutor::run(...)` exchange only that env overlay, which
callers must merge into their own base shell environment.
- Clarified the prepared-exec helper boundary in `core` by naming the
new helper APIs around `ExecRequest`, while keeping the legacy
`execute_env(...)` entrypoints as thin compatibility wrappers for
existing callers that still use the older naming.
- Added a small post-spawn hook on the prepared execution path so the
parent copy of the inheritable escalation socket is closed immediately
after both the existing one-shot shell-command spawn and the
unified-exec spawn.
- Made session teardown explicit with session-scoped cancellation:
dropping an `EscalationSession` or canceling its parent request now
stops intercept workers, and the server-spawned escalated child uses
`kill_on_drop(true)` so teardown cannot orphan an already-approved
child.
- Added `UnifiedExecBackendConfig` plumbing through `ToolsConfig`, a
`shell::zsh_fork_backend` facade, and an opaque unified-exec
spawn-lifecycle hook so unified exec can prepare a wrapped `zsh -c/-lc`
request without storing `EscalationSession` directly in generic
process/runtime code.
- Kept the existing `shell_command` zsh-fork behavior intact on top of
the new bootstrap path. Tool selection is unchanged in this PR: when
`shell_zsh_fork` is enabled, `ShellCommand` still wins over
`exec_command`.

## Verification

- `cargo test -p codex-shell-escalation`
  - includes coverage for `start_session_exposes_wrapper_env_overlay`
  - includes coverage for `exec_closes_parent_socket_after_shell_spawn`
- includes coverage for
`dropping_session_aborts_intercept_workers_and_kills_spawned_child`
- `cargo test -p codex-core
shell_zsh_fork_prefers_shell_command_over_unified_exec`
- `cargo test -p codex-core --test all
shell_zsh_fork_prompts_for_skill_script_execution`


---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13392).
* openai#13432
* __->__ openai#13392
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants