mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
refactor: prepare unified exec for zsh-fork backend (#13392)
## 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
This commit is contained in:
committed by
GitHub
Unverified
parent
1ce1712aeb
commit
b4cb989563
@@ -49,6 +49,10 @@ pub(crate) fn set_deterministic_process_ids_for_tests(enabled: bool) {
|
||||
}
|
||||
|
||||
pub(crate) use errors::UnifiedExecError;
|
||||
pub(crate) use process::NoopSpawnLifecycle;
|
||||
#[cfg(unix)]
|
||||
pub(crate) use process::SpawnLifecycle;
|
||||
pub(crate) use process::SpawnLifecycleHandle;
|
||||
pub(crate) use process::UnifiedExecProcess;
|
||||
|
||||
pub(crate) const MIN_YIELD_TIME_MS: u64 = 250;
|
||||
|
||||
@@ -24,6 +24,17 @@ use super::UNIFIED_EXEC_OUTPUT_MAX_TOKENS;
|
||||
use super::UnifiedExecError;
|
||||
use super::head_tail_buffer::HeadTailBuffer;
|
||||
|
||||
pub(crate) trait SpawnLifecycle: std::fmt::Debug + Send + Sync {
|
||||
fn after_spawn(&mut self) {}
|
||||
}
|
||||
|
||||
pub(crate) type SpawnLifecycleHandle = Box<dyn SpawnLifecycle>;
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub(crate) struct NoopSpawnLifecycle;
|
||||
|
||||
impl SpawnLifecycle for NoopSpawnLifecycle {}
|
||||
|
||||
pub(crate) type OutputBuffer = Arc<Mutex<HeadTailBuffer>>;
|
||||
pub(crate) struct OutputHandles {
|
||||
pub(crate) output_buffer: OutputBuffer,
|
||||
@@ -44,6 +55,7 @@ pub(crate) struct UnifiedExecProcess {
|
||||
output_drained: Arc<Notify>,
|
||||
output_task: JoinHandle<()>,
|
||||
sandbox_type: SandboxType,
|
||||
_spawn_lifecycle: SpawnLifecycleHandle,
|
||||
}
|
||||
|
||||
impl UnifiedExecProcess {
|
||||
@@ -51,6 +63,7 @@ impl UnifiedExecProcess {
|
||||
process_handle: ExecCommandSession,
|
||||
initial_output_rx: tokio::sync::broadcast::Receiver<Vec<u8>>,
|
||||
sandbox_type: SandboxType,
|
||||
spawn_lifecycle: SpawnLifecycleHandle,
|
||||
) -> Self {
|
||||
let output_buffer = Arc::new(Mutex::new(HeadTailBuffer::default()));
|
||||
let output_notify = Arc::new(Notify::new());
|
||||
@@ -92,6 +105,7 @@ impl UnifiedExecProcess {
|
||||
output_drained,
|
||||
output_task,
|
||||
sandbox_type,
|
||||
_spawn_lifecycle: spawn_lifecycle,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -196,13 +210,14 @@ impl UnifiedExecProcess {
|
||||
pub(super) async fn from_spawned(
|
||||
spawned: SpawnedPty,
|
||||
sandbox_type: SandboxType,
|
||||
spawn_lifecycle: SpawnLifecycleHandle,
|
||||
) -> Result<Self, UnifiedExecError> {
|
||||
let SpawnedPty {
|
||||
session: process_handle,
|
||||
output_rx,
|
||||
mut exit_rx,
|
||||
} = spawned;
|
||||
let managed = Self::new(process_handle, output_rx, sandbox_type);
|
||||
let managed = Self::new(process_handle, output_rx, sandbox_type, spawn_lifecycle);
|
||||
|
||||
let exit_ready = matches!(exit_rx.try_recv(), Ok(_) | Err(TryRecvError::Closed));
|
||||
|
||||
|
||||
@@ -49,6 +49,7 @@ use crate::unified_exec::generate_chunk_id;
|
||||
use crate::unified_exec::head_tail_buffer::HeadTailBuffer;
|
||||
use crate::unified_exec::process::OutputBuffer;
|
||||
use crate::unified_exec::process::OutputHandles;
|
||||
use crate::unified_exec::process::SpawnLifecycleHandle;
|
||||
use crate::unified_exec::process::UnifiedExecProcess;
|
||||
use crate::unified_exec::resolve_max_tokens;
|
||||
|
||||
@@ -528,6 +529,7 @@ impl UnifiedExecProcessManager {
|
||||
&self,
|
||||
env: &ExecRequest,
|
||||
tty: bool,
|
||||
mut spawn_lifecycle: SpawnLifecycleHandle,
|
||||
) -> Result<UnifiedExecProcess, UnifiedExecError> {
|
||||
let (program, args) = env
|
||||
.command
|
||||
@@ -555,7 +557,8 @@ impl UnifiedExecProcessManager {
|
||||
};
|
||||
let spawned =
|
||||
spawn_result.map_err(|err| UnifiedExecError::create_process(err.to_string()))?;
|
||||
UnifiedExecProcess::from_spawned(spawned, env.sandbox).await
|
||||
spawn_lifecycle.after_spawn();
|
||||
UnifiedExecProcess::from_spawned(spawned, env.sandbox, spawn_lifecycle).await
|
||||
}
|
||||
|
||||
pub(super) async fn open_session_with_sandbox(
|
||||
@@ -569,7 +572,8 @@ impl UnifiedExecProcessManager {
|
||||
Some(context.session.conversation_id),
|
||||
));
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = UnifiedExecRuntime::new(self);
|
||||
let mut runtime =
|
||||
UnifiedExecRuntime::new(self, context.turn.tools_config.unified_exec_backend);
|
||||
let exec_approval_requirement = context
|
||||
.session
|
||||
.services
|
||||
|
||||
Reference in New Issue
Block a user