Conversation
This was referenced May 28, 2026
bolinfest
force-pushed
the
pr24981
branch
2 times, most recently
from
8c71c82 to
12167bd
Compare
bolinfest
force-pushed
the
pr24980
branch
2 times, most recently
from
9bc8a51 to
79a708f
Compare
bolinfest
added a commit
that referenced
this pull request
Jun 1, 2026
## Why `shell_zsh_fork` and unified exec need to remain independently controllable for enterprise rollouts, but we also need a third mode that composes them. That composed mode is intended to preserve unified exec command lifecycle support while letting the zsh fork provide more accurate `execv(2)` interception. Enabling `unified_exec_zsh_fork` by itself is intentionally not sufficient. It is a composition gate, not a dependency-enabling shortcut: - `unified_exec` selects the PTY-backed unified exec tool. - `shell_zsh_fork` opts into the zsh fork backend. - `unified_exec_zsh_fork` only allows those two already-enabled modes to be composed so local zsh unified exec commands can launch through the zsh fork. This separation is deliberate. Enterprises and staged rollouts must be able to enable or disable unified exec and zsh-fork independently. If `unified_exec_zsh_fork` implied either dependency, then enabling one under-development composition flag would silently activate a shell backend that the configured feature set left disabled. This PR introduces only the configuration and planning gate for that composition. Existing `shell_zsh_fork` behavior continues to use the standalone shell tool unless the new composition feature is explicitly enabled alongside both dependencies. ## What Changed - Added the under-development feature flag `unified_exec_zsh_fork`. - Added `UnifiedExecFeatureMode` so the three input feature flags collapse into `Disabled`, `Direct`, or `ZshFork` mode before tool planning. - Updated tool selection so zsh-fork composition requires `unified_exec`, `shell_zsh_fork`, and `unified_exec_zsh_fork`. - Kept the existing standalone zsh-fork shell tool behavior when only `shell_zsh_fork` is enabled. - Updated config schema output for the new feature flag. ## Verification - Added feature and tool-config coverage for the new gate. - Added planner coverage proving `shell_zsh_fork` remains standalone until composition is explicitly enabled. - Ran focused tests for `codex-features`, `codex-tools`, and the affected `codex-core` planner case. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24979). * #24982 * #24981 * #24980 * __->__ #24979
bolinfest
added a commit
that referenced
this pull request
Jun 1, 2026
## Why When unified exec is configured to launch through the zsh fork, local commands should not let the model override the shell binary with the `shell` parameter. The configured zsh fork is the mechanism that makes `execv(2)` interception reliable, so exposing `shell` for local zsh-fork execution would create a confusing API surface and undermine the composition. Remote environments are different: zsh-fork interception is local-only, so remote unified-exec calls must keep direct unified-exec behavior and still expose `shell` when a remote environment can be selected. ## What Changed - Taught the `exec_command` schema builder to omit the `shell` parameter when requested. - Hid `shell` from the unified-exec tool schema only when zsh-fork unified exec applies to all selectable environments. - Kept `shell` visible when any remote environment can be targeted, because those calls run through direct unified exec. - Made unified exec choose the effective shell mode per selected environment: local environments keep zsh-fork mode, remote environments use direct mode. - Left direct unified-exec behavior unchanged, including support for model-specified shells there. ## Verification - Added schema coverage showing `exec_command` can hide `shell`. - Added planner coverage showing zsh-fork unified exec hides `shell` for local-only execution while direct unified exec still exposes it. - Added planner coverage showing `shell` remains visible when a remote environment is available. - Added handler coverage showing remote environments use direct unified-exec shell mode instead of zsh-fork mode. - Ran the focused `codex-core` shell-parameter and zsh-fork tests. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24980). * #24982 * #24981 * __->__ #24980
bolinfest
force-pushed
the
pr24981
branch
2 times, most recently
from
47b40d8 to
0ff0b67
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ff0b679e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } = &req.exec_approval_requirement | ||
| { | ||
| ExecApprovalRequirement::Skip { | ||
| bypass_sandbox: false, |
Contributor
There was a problem hiding this comment.
When UnifiedExecShellMode::ZshFork is configured but the request cannot be prepared by the zsh-fork backend, the code later logs a fallback and runs through the normal direct path; however this line has already cleared bypass_sandbox from an ExecPolicy Allow. In that fallback scenario, policy-approved commands that previously selected SandboxType::None are now launched inside the sandbox and can fail on writes/network that the rule explicitly allowed, so either restore the flag on fallback or only clear it after the zsh-fork trampoline is definitely used.
Useful? React with 👍 / 👎.
bolinfest
changed the title
bolinfest
force-pushed
the
pr24981
branch
5 times, most recently
from
4f00e2a to
8cbb1eb
Compare
bolinfest
changed the title
viyatb-oai
approved these changes
Jun 5, 2026
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
2 participants
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
This PR fixes approval sandbox semantics in the unified-exec path. The zsh-fork runtime exposed the bug because the shell can do meaningful work before any intercepted child
execv(2)exists: redirections, builtins, globbing, and pipeline setup all happen in the launch process. If the model requestedsandbox_permissions=require_escalated, or an exec-policyallowrule explicitly bypassed the sandbox, that approved sandbox decision needs to be preserved for the launch path and for intercepted execs that use the same approval machinery.The behavior is not only about zsh fork. The production changes are in shared approval/escalation code, so they also affect non-zsh-fork intercepted exec paths that go through the same sandbox decision logic. The narrow intent is to preserve the approval decision while still keeping denied-read profiles and bounded additional-permission requests sandboxed.
Production Changes
codex-rs/core/src/tools/runtimes/unified_exec.rs: derives alaunch_sandbox_permissionsvalue from the requested sandbox permissions and the runtime filesystem policy, then uses that value for managed-network/env setup and launch sandbox selection. This keeps full approval or policy-bypass decisions visible to the first unified-exec attempt, while still preventing a full sandbox override from discarding denied-read restrictions. Direct unified exec keeps the same decision surface; the important difference is that zsh-fork launch setup no longer accidentally loses the approved parent sandbox decision.codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs: makes intercepted-exec escalation selection explicit for the three sandbox permission modes.UseDefaultonly escalates when an exec-policy decision allows sandbox bypass,RequireEscalatedescalates when unsandboxed execution is allowed, andWithAdditionalPermissionsescalates through the bounded additional-permissions path instead of being treated as a full unsandboxed override. Unsandboxed intercepted execs now also rebuild the environment asRequireEscalated, which strips managed-network proxy variables consistently with other unsandboxed execution.Test Coverage
Most of the PR is tests. The new coverage verifies:
with_additional_permissionsremains sandboxed and permission-profile based;No documentation update is needed; this is an internal approval/sandbox correctness fix.
Stack created with Sapling. Best reviewed with ReviewStack.