mirror of
https://github.com/openai/codex.git
synced 2026-05-01 03:42:05 +03:00
refactor: make unified-exec zsh-fork state explicit (#14633)
## Why
The unified-exec path was carrying zsh-fork state in a partially
flattened way.
First, the decision about whether zsh-fork was active came from feature
selection in `ToolsConfig`, while the real prerequisites lived in
session state. That left the handler and runtime defending against
partially configured cases later.
Second, once zsh-fork was active, its two runtime-only paths were
threaded through the runtime as separate arguments even though they form
one coherent piece of configuration.
This change keeps unified-exec on a single session-derived source of
truth and bundles the zsh-fork-specific paths into a named config type
so the runtime can pass them around as one unit.
In particular, this PR introduces this enum so the `ZshFork` variant can
carry the appropriate state with it:
```rust
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum UnifiedExecShellMode {
Direct,
ZshFork(ZshForkConfig),
}
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct ZshForkConfig {
pub(crate) shell_zsh_path: AbsolutePathBuf,
pub(crate) main_execve_wrapper_exe: AbsolutePathBuf,
}
```
This cleanup was done in preparation for
https://github.com/openai/codex/pull/13432.
## What Changed
- Replaced the feature-only `UnifiedExecBackendConfig` split with
`UnifiedExecShellMode` in `codex-rs/core/src/tools/spec.rs`.
- Derived the unified-exec mode from session-backed inputs when building
turn `ToolsConfig`, and preserved that mode across model switches and
review turns.
- Introduced `ZshForkConfig`, which stores the resolved zsh-fork
`AbsolutePathBuf` values for the configured `zsh` binary and `execve`
wrapper.
- Threaded `ZshForkConfig` through unified-exec command construction and
the zsh-fork preparation path so zsh-fork-specific runtime code consumes
a single config object instead of separate path arguments.
- Added focused tests for constructing zsh-fork mode only when session
prerequisites are available, and updated the zsh-fork expectations to be
target-platform aware.
## Testing
- `cargo test -p codex-core zsh_fork --lib`
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/14633).
* #13432
* __->__ #14633
This commit is contained in:
@@ -8,6 +8,8 @@ use crate::features::Features;
|
||||
use crate::mcp_connection_manager::ToolInfo;
|
||||
use crate::models_manager::collaboration_mode_presets::CollaborationModesConfig;
|
||||
use crate::original_image_detail::can_request_original_image_detail;
|
||||
use crate::shell::Shell;
|
||||
use crate::shell::ShellType;
|
||||
use crate::tools::code_mode::PUBLIC_TOOL_NAME;
|
||||
use crate::tools::code_mode::WAIT_TOOL_NAME;
|
||||
use crate::tools::code_mode::is_code_mode_nested_tool;
|
||||
@@ -46,12 +48,14 @@ use codex_protocol::openai_models::WebSearchToolType;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use serde_json::Value as JsonValue;
|
||||
use serde_json::json;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
|
||||
const TOOL_SEARCH_DESCRIPTION_TEMPLATE: &str =
|
||||
include_str!("../../templates/search_tool/tool_description.md");
|
||||
@@ -203,10 +207,46 @@ pub enum ShellCommandBackendConfig {
|
||||
ZshFork,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
|
||||
pub enum UnifiedExecBackendConfig {
|
||||
#[derive(Debug, Clone, Eq, PartialEq)]
|
||||
pub enum UnifiedExecShellMode {
|
||||
Direct,
|
||||
ZshFork,
|
||||
ZshFork(ZshForkConfig),
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Eq, PartialEq)]
|
||||
pub struct ZshForkConfig {
|
||||
pub(crate) shell_zsh_path: AbsolutePathBuf,
|
||||
pub(crate) main_execve_wrapper_exe: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
impl UnifiedExecShellMode {
|
||||
pub fn for_session(
|
||||
shell_command_backend: ShellCommandBackendConfig,
|
||||
user_shell: &Shell,
|
||||
shell_zsh_path: Option<&PathBuf>,
|
||||
main_execve_wrapper_exe: Option<&PathBuf>,
|
||||
) -> Self {
|
||||
if cfg!(unix)
|
||||
&& shell_command_backend == ShellCommandBackendConfig::ZshFork
|
||||
&& matches!(user_shell.shell_type, ShellType::Zsh)
|
||||
&& let (Some(shell_zsh_path), Some(main_execve_wrapper_exe)) =
|
||||
(shell_zsh_path, main_execve_wrapper_exe)
|
||||
&& let (Ok(shell_zsh_path), Ok(main_execve_wrapper_exe)) = (
|
||||
AbsolutePathBuf::try_from(shell_zsh_path.as_path())
|
||||
.inspect_err(|e| tracing::warn!("Failed to convert shell_zsh_path `{shell_zsh_path:?}`: {e:?}")),
|
||||
AbsolutePathBuf::try_from(main_execve_wrapper_exe.as_path()).inspect_err(|e| {
|
||||
tracing::warn!("Failed to convert main_execve_wrapper_exe `{main_execve_wrapper_exe:?}`: {e:?}")
|
||||
}),
|
||||
)
|
||||
{
|
||||
Self::ZshFork(ZshForkConfig {
|
||||
shell_zsh_path,
|
||||
main_execve_wrapper_exe,
|
||||
})
|
||||
} else {
|
||||
Self::Direct
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
@@ -214,7 +254,7 @@ pub(crate) struct ToolsConfig {
|
||||
pub available_models: Vec<ModelPreset>,
|
||||
pub shell_type: ConfigShellToolType,
|
||||
shell_command_backend: ShellCommandBackendConfig,
|
||||
pub unified_exec_backend: UnifiedExecBackendConfig,
|
||||
pub unified_exec_shell_mode: UnifiedExecShellMode,
|
||||
pub allow_login_shell: bool,
|
||||
pub apply_patch_tool_type: Option<ApplyPatchToolType>,
|
||||
pub web_search_mode: Option<WebSearchMode>,
|
||||
@@ -300,13 +340,6 @@ impl ToolsConfig {
|
||||
} else {
|
||||
ShellCommandBackendConfig::Classic
|
||||
};
|
||||
let unified_exec_backend =
|
||||
if features.enabled(Feature::ShellTool) && features.enabled(Feature::ShellZshFork) {
|
||||
UnifiedExecBackendConfig::ZshFork
|
||||
} else {
|
||||
UnifiedExecBackendConfig::Direct
|
||||
};
|
||||
|
||||
let unified_exec_allowed = unified_exec_allowed_in_environment(
|
||||
cfg!(target_os = "windows"),
|
||||
sandbox_policy,
|
||||
@@ -353,7 +386,7 @@ impl ToolsConfig {
|
||||
available_models: available_models_ref.to_vec(),
|
||||
shell_type,
|
||||
shell_command_backend,
|
||||
unified_exec_backend,
|
||||
unified_exec_shell_mode: UnifiedExecShellMode::Direct,
|
||||
allow_login_shell: true,
|
||||
apply_patch_tool_type,
|
||||
web_search_mode: *web_search_mode,
|
||||
@@ -390,6 +423,29 @@ impl ToolsConfig {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_unified_exec_shell_mode(
|
||||
mut self,
|
||||
unified_exec_shell_mode: UnifiedExecShellMode,
|
||||
) -> Self {
|
||||
self.unified_exec_shell_mode = unified_exec_shell_mode;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_unified_exec_shell_mode_for_session(
|
||||
mut self,
|
||||
user_shell: &Shell,
|
||||
shell_zsh_path: Option<&PathBuf>,
|
||||
main_execve_wrapper_exe: Option<&PathBuf>,
|
||||
) -> Self {
|
||||
self.unified_exec_shell_mode = UnifiedExecShellMode::for_session(
|
||||
self.shell_command_backend,
|
||||
user_shell,
|
||||
shell_zsh_path,
|
||||
main_execve_wrapper_exe,
|
||||
);
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_web_search_config(mut self, web_search_config: Option<WebSearchConfig>) -> Self {
|
||||
self.web_search_config = web_search_config;
|
||||
self
|
||||
|
||||
Reference in New Issue
Block a user