Skip to content

fix: honor parent approvals for intercepted execs#24982

Open
bolinfest wants to merge 1 commit into
mainfrom
pr24982
Open

fix: honor parent approvals for intercepted execs#24982
bolinfest wants to merge 1 commit into
mainfrom
pr24982

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 28, 2026

Why

After the parent zsh-fork unified-exec session has an approved sandbox override, intercepted child execv(2) calls should not force the user through the same approval again just because the command crossed the zsh-fork boundary. For example, if the model requests curl google.com with sandbox_permissions=require_escalated and the user approves that parent command, the intercepted /usr/bin/curl should run with the approved unsandboxed execution semantics.

This is also the compelling guardian/auto-mode integration point. Instead of requiring Codex to predict and pre-author prefix_rule() entries for every possible segment of a complex shell command, zsh-fork interception gives guardian a decision point at each executed piece. A command like foo | bar | baz > quux can keep shell-owned behavior under the parent command's sandbox decision, while guardian can still independently evaluate the intercepted foo, bar, and baz executions and escalate only the pieces that need it.

Explicit exec-policy rules should remain authoritative for intercepted execs. A parent approval should cover unmatched sandbox-override prompts, not erase policy-driven decisions.

What Changed

  • Tracked whether the parent shell/unified-exec request had an approved sandbox override.
  • Converted only unmatched intercepted-exec prompts covered by that parent approval into Allow.
  • Preserved explicit exec-policy rule decisions for intercepted execs.
  • Forced preapproved with_additional_permissions requests through the server-side execution path so the resolved permission profile is applied consistently.
  • Stripped managed-network proxy environment variables when an intercepted command is rerun unsandboxed.

Verification

  • Added coverage for the parent-approval handoff logic.
  • Added coverage for a curl-style parent-approved require_escalated intercepted exec.
  • Added coverage proving parent approval does not suppress an explicit intercepted-exec policy prompt.
  • Added an end-to-end zsh-fork unified-exec integration test that approves require_escalated once and verifies shell-owned redirection can write through the approved parent session.
  • Re-ran focused codex-core zsh-fork and parent-approval tests.

@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from 2f1df9b to d592f5c Compare May 28, 2026 23:34
@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from 93fbaf3 to 90d6350 Compare May 31, 2026 20:54
@bolinfest bolinfest force-pushed the pr24981 branch 2 times, most recently from 12167bd to e7d52ab Compare May 31, 2026 21:12
@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from 59b62d4 to 8e53b89 Compare June 1, 2026 15:33
@bolinfest bolinfest force-pushed the pr24981 branch 2 times, most recently from 00a04d0 to 3429f08 Compare June 1, 2026 18:33
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 bolinfest force-pushed the pr24982 branch 2 times, most recently from 7e31d18 to 896feb9 Compare June 5, 2026 16:12
@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from a362b24 to c1d8b56 Compare June 5, 2026 16:34
@bolinfest bolinfest force-pushed the pr24981 branch 2 times, most recently from 4f00e2a to 8cbb1eb Compare June 5, 2026 16:56
@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from 6bc5a32 to e2f8f42 Compare June 7, 2026 16:38
bolinfest added a commit that referenced this pull request Jun 7, 2026
## 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 requested `sandbox_permissions=require_escalated`, or an
exec-policy `allow` rule 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 a
`launch_sandbox_permissions` value 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. `UseDefault` only escalates when an exec-policy
decision allows sandbox bypass, `RequireEscalated` escalates when
unsandboxed execution is allowed, and `WithAdditionalPermissions`
escalates through the bounded additional-permissions path instead of
being treated as a full unsandboxed override. Unsandboxed intercepted
execs now also rebuild the environment as `RequireEscalated`, which
strips managed-network proxy variables consistently with other
unsandboxed execution.

## Test Coverage

Most of the PR is tests. The new coverage verifies:

- unified exec preserves parent approval and exec-policy sandbox
decisions for zsh-fork launch selection;
- bounded `with_additional_permissions` remains sandboxed and
permission-profile based;
- denied-read profiles are not weakened by parent approval;
- explicit prompt rules still prompt for intercepted execs after the
parent command is approved;
- unsandboxed intercepted execs strip managed-network env vars.

No documentation update is needed; this is an internal approval/sandbox
correctness fix.





---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24981).
* #24982
* __->__ #24981
Base automatically changed from pr24981 to main June 7, 2026 18:33
@bolinfest bolinfest force-pushed the pr24982 branch 2 times, most recently from 92d1047 to 1b5f12f Compare June 8, 2026 14:34
@bolinfest bolinfest marked this pull request as ready for review June 8, 2026 16:00
@bolinfest bolinfest requested a review from a team as a code owner June 8, 2026 16:00
Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b5f12f44a

ℹ️ 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".

Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs Outdated
Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant