Skip to content

refactor: route zsh-fork through unified exec#13432

Open
bolinfest wants to merge 1 commit intomainfrom
pr13432
Open

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

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 2026

Why

When unified_exec and shell_zsh_fork are enabled together, unified-exec sessions need to stay on the configured zsh-fork binary so exec interception remains reliable for approvals, execpolicy, and escalation behavior.

In practice, those sessions could drift away from the configured zsh_path in a few ways:

  • routing could fall back to the wrong tool surface when both features were enabled
  • explicit exec_command.shell overrides could request a different shell
  • nested exec zsh calls could hop onto the host zsh
  • privileged login-shell sessions could surface startup helpers like /usr/libexec/path_helper as separate escalated execs
  • privileged exec_command sessions could let later write_stdin subcommands inherit that broader privilege instead of dropping back to turn-default policy
  • zsh-fork escalation-session resources could stay resident until the model polled again

That weakens the guarantees the zsh-fork path is supposed to provide and makes privileged interactive sessions much noisier than intended.

What Changed

  • Kept zsh-fork sessions on exec_command / unified exec when both features are enabled, and hid the shell parameter from the exec_command schema when the session is actively using zsh-fork (core/src/tools/spec.rs, core/src/tools/handlers/unified_exec.rs).
  • In active zsh-fork unified-exec sessions, ignore model-provided shell overrides and always launch the configured zsh_path (core/src/tools/handlers/unified_exec.rs).
  • Replaced permissive shell-shape scanning with deterministic parsing of the original shell argv (program, -c / -lc, script) before preparing the zsh-fork unified-exec path (core/src/tools/runtimes/shell/unix_escalation.rs, core/src/tools/runtimes/shell/zsh_fork_backend.rs, core/src/tools/runtimes/unified_exec.rs).
  • For privileged unified-exec zsh-fork launches, build an exact intercepted inner zsh -c ... target instead of heuristically matching later startup execs. When a matching shell snapshot exists for a login-shell request, that inner command now sources the snapshot, reapplies explicit env overrides, and then runs the original script. This preserves login-derived shell state without rerunning startup helpers under zsh-fork interception (core/src/tools/runtimes/mod.rs, core/src/tools/runtimes/shell/unix_escalation.rs).
  • Kept nested intercepted zsh execs on zsh-fork by:
    • recognizing approved host zsh installations
    • forcing escalation when an intercepted exec targets host zsh instead of the configured zsh_path
    • rewriting that intercepted program path back to the configured zsh_path
      (core/src/tools/runtimes/shell/unix_escalation.rs).
  • Kept skill lookup pinned to the turn cwd and re-read execpolicy for intercepted execs so approved amendments continue to apply to later matching subcommands in the same session (core/src/tools/runtimes/shell/unix_escalation.rs).
  • Ensured write_stdin subcommands in a privileged zsh-fork unified-exec TTY fall back to turn-default sandbox policy unless separately allowed, while still honoring matching prefix_rule() approvals (core/src/tools/runtimes/shell/unix_escalation.rs, core/tests/suite/unified_exec.rs).
  • Released zsh-fork escalation-session resources when unified-exec processes exit, even if the model never polls the session again (core/src/unified_exec/process.rs, core/src/tools/runtimes/shell/zsh_fork_backend.rs).
  • Expanded coverage for the routed unified-exec zsh-fork path:
    • app-server v2 turn-start coverage, including interrupt cleanup and the login-startup-helper regression (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, nested zsh rewrite coverage, and the privileged-write_stdin sandbox regression (core/tests/suite/unified_exec.rs)
    • unit tests for shell parsing, snapshot-backed exact matching, shell-override handling, and zsh rewrite predicates (core/src/tools/handlers/unified_exec_tests.rs, core/src/tools/runtimes/shell/unix_escalation_tests.rs)
  • Added Windows-safe #[cfg(unix)] usage around zsh-fork-only integration coverage.

Verification

  • cargo test -p codex-core build_exact_match_zsh_fork_reexec --lib
  • cargo test -p codex-core maybe_wrap_shell_lc_with_snapshot --lib
  • cargo test -p codex-core --test all unified_exec_zsh_fork_write_stdin_reverts_to_default_sandbox_and_honors_prefix_rule
  • cargo test -p codex-app-server --test all turn_start_shell_zsh_fork_login_startup_helper_does_not_prompt_separately_v2

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 checked pstree and confirmed that the Python process was still running under the configured zsh-fork shell rather than the host shell.

@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
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from f3f72e3 to e2d0232 Compare March 12, 2026 06:59
@bolinfest bolinfest force-pushed the pr13644 branch 2 times, most recently from 6764932 to 61fcd3e Compare March 12, 2026 07:09
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from 83eb97f to c7ce647 Compare March 12, 2026 07:19
@bolinfest bolinfest force-pushed the pr13432 branch 3 times, most recently from 75d57a9 to 6a3ad79 Compare March 12, 2026 15:29
@bolinfest bolinfest force-pushed the pr13644 branch 2 times, most recently from 1b7e3ec to b23aeee Compare March 13, 2026 16:43
@bolinfest bolinfest force-pushed the pr13432 branch 2 times, most recently from 2be2c19 to 374a9a6 Compare March 13, 2026 17:41
@bolinfest bolinfest force-pushed the pr13644 branch 5 times, most recently from 84ec355 to ab58df4 Compare March 13, 2026 19:57
.as_ref()
.map(|preamble| format!("{preamble}\n\n{}", shell_command.script))
.unwrap_or_else(|| shell_command.script.clone());
let exact_intercepted_command = vec![
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the exact match will create some issues... what if you don't have a shell snapshot available for example? Then we can still have a -lc

This will basically break the pre-approved path

req.sandbox_permissions,
SandboxPermissions::RequireEscalated
) {
let mut explicit_env_overrides = req.env.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the contract of the shell snapshot. It means you will restore something coming from before the shell_snapshot on top of the shell_snapshot

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