chore(sync): merge upstream/main into main#32
Conversation
## 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
…tream-main-20260305100117
![kontext-review[bot]](https://avatars.githubusercontent.com/in/2989619?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verification certificate:
- Verdict: REQUEST_CHANGES
- Depth: full
- Confidence: 0.75
- Certificate valid: yes
- Scenarios traced: 1
- Verification bounds:
- Changed files and explicitly traced call paths
Inline findings posted:
- [LOW][correctness] Architectural finding (
codex-rs/shell-escalation/src/unix/escalate_server.rs:116) - [LOW][correctness] Architectural finding (
codex-rs/shell-escalation/src/unix/stopwatch.rs:45)
Reviewer summary:
- The PR successfully merges upstream/main into main.
- It introduces
zsh_fork_backendfor bothshellandunified_execruntimes. - It properly manages the lifecycle of the escalation session and the client socket.
- The code is well-written, follows best practices, and correctly implements the intended behavior.
Scores and diagnostics
-
Architecture: 7.0/10
-
Organization: 7.0/10
-
Cleanliness: 7.0/10
-
Overall: 7.0/10
-
Candidate findings: 2
-
Published inline: 2
-
Published summary-only: 0
-
Suppressed findings: 0
-
Verification verdict: REQUEST_CHANGES
-
Model:
gemini-3.1-pro-preview -
Omitted files: 0
Generated at 2026-03-05T09:02:20.462Z.
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for EscalationSession { | ||
| fn drop(&mut self) { | ||
| self.close_client_socket(); |
There was a problem hiding this comment.
[LOW][correctness] Architectural finding
Impact: Can reduce maintainability or correctness if left unresolved.
Suggested fix: Apply a targeted refactor and add/adjust tests where needed.
Evidence: The EscalationSession correctly manages the lifecycle of the escalation task and the client socket.
Evidence citations:
- codex-rs/shell-escalation/src/unix/escalate_server.rs:116
| limit: None, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[LOW][correctness] Architectural finding
Impact: Can reduce maintainability or correctness if left unresolved.
Suggested fix: Apply a targeted refactor and add/adjust tests where needed.
Evidence: The Stopwatch::unlimited correctly returns a cancellation token that is never cancelled.
Evidence citations:
- codex-rs/shell-escalation/src/unix/stopwatch.rs:45
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| pub async fn execute_exec_request_with_after_spawn( | ||
| exec_request: ExecRequest, | ||
| stdout_stream: Option<StdoutStream>, | ||
| after_spawn: Option<Box<dyn FnOnce() + Send>>, | ||
| ) -> crate::error::Result<ExecToolCallOutput> { | ||
| let effective_policy = exec_request.sandbox_policy.clone(); | ||
| execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn).await |
There was a problem hiding this comment.
🟡 AGENTS.md violation: execute_exec_request_with_after_spawn is a small helper referenced only once
The repository's AGENTS.md mandates: "Do not create small helper methods that are referenced only once." The new function execute_exec_request_with_after_spawn at codex-rs/core/src/sandboxing/mod.rs:428-434 is a trivial 3-line wrapper around execute_exec_request and has exactly one call site at codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs:768. The caller could inline the two lines (let effective_policy = exec_request.sandbox_policy.clone(); execute_exec_request(...)) directly instead of going through this wrapper.
Prompt for agents
Remove the single-use helper `execute_exec_request_with_after_spawn` from codex-rs/core/src/sandboxing/mod.rs (lines 428-434). Instead, at the one call site in codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs:768, inline the logic: clone the sandbox_policy from the exec_request, then call `crate::exec::execute_exec_request(exec_request, &effective_policy, stdout_stream, after_spawn)` directly. This satisfies the AGENTS.md rule: Do not create small helper methods that are referenced only once.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Hard-cutover sync: merge latest upstream/main into main using a real merge commit (no squash).