Conversation
65be9c0 to
c04ba90
Compare
jif-oai
approved these changes
Mar 4, 2026
29419f7 to
1b5a00e
Compare
e95b1a1 to
58049ba
Compare
5c8732e to
8222c01
Compare
3960093 to
cbf6429
Compare
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
184d0e7 to
8b935f8
Compare
8cc6c06 to
deee4fd
Compare
033851e to
995ca7d
Compare
b792a0f to
d1b3a60
Compare
jif-oai
reviewed
Mar 6, 2026
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
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
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
When
unified_execandshell_zsh_forkare enabled together, execution must keep using the configuredzsh_pathso 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 nestedzshexecs. That breaks the guarantees the zsh-fork path is supposed to provide.What Changed
core/src/tools/spec.rs,core/src/tools/handlers/unified_exec.rs,core/src/memories/usage.rs, app-server suite updates).UnifiedExecBackendConfig::ZshFork, ignored model-provided shell overrides so the session starts from configuredzsh_path(core/src/tools/handlers/unified_exec.rs).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).zshexecs stay on configured zsh-fork:programiszshbut not configuredzsh_pathprogramto configuredzsh_pathbefore execution(
core/src/tools/runtimes/shell/unix_escalation.rs).cwdand 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).app-server/tests/suite/v2/turn_start_zsh_fork.rs)core/tests/suite/approvals.rs)core/tests/suite/skill_approval.rs)core/tests/suite/unified_exec.rs)core/src/tools/runtimes/shell/unix_escalation_tests.rs)#[cfg(unix)]import usage in zsh-fork integration tests.Verification
cargo test -p codex-core tools::runtimes::shell::unix_escalation::testscargo test -p codex-core --test all unified_exec_zsh_fork_keeps_python_repl_attached_to_zsh_sessioncargo test -p codex-core --test all unified_exec_zsh_fork_rewrites_nested_zsh_exec_to_configured_binaryManual Testing
Run Codex built from source with both features enabled as follows:
Then ask Codex to do something like:
Then tell it:
In my case, it replied with:
Then over in a plain shell, I check
pstree:Stack created with Sapling. Best reviewed with ReviewStack.