From 126a88af4af19734c3c56b3bad4e7ef302932cfa Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 29 May 2026 14:26:37 -0700 Subject: [PATCH] feat: gate unified exec zsh fork composition --- codex-rs/core/config.schema.json | 6 ++++ codex-rs/core/src/session/mod.rs | 1 - codex-rs/core/src/session/review.rs | 3 +- codex-rs/core/src/session/turn_context.rs | 4 +-- codex-rs/core/src/tools/spec_plan_tests.rs | 40 ++++++++++++++++++++++ codex-rs/features/src/lib.rs | 8 +++++ codex-rs/tools/src/lib.rs | 1 + codex-rs/tools/src/tool_config.rs | 21 ++++++++++-- codex-rs/tools/src/tool_config_tests.rs | 26 ++++++++++++-- 9 files changed, 99 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 1dd6bda8a4..38b6344319 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -614,6 +614,9 @@ "unified_exec": { "type": "boolean" }, + "unified_exec_zsh_fork": { + "type": "boolean" + }, "use_legacy_landlock": { "type": "boolean" }, @@ -4708,6 +4711,9 @@ "unified_exec": { "type": "boolean" }, + "unified_exec_zsh_fork": { + "type": "boolean" + }, "use_legacy_landlock": { "type": "boolean" }, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 8c535cd728..f242b2d918 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -362,7 +362,6 @@ use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; use codex_tools::ToolEnvironmentMode; use codex_tools::UnifiedExecShellMode; -use codex_tools::shell_command_backend_for_features; use codex_utils_absolute_path::AbsolutePathBuf; #[cfg(test)] use codex_utils_stream_parser::ProposedPlanSegment; diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 75b7539090..f2279bb03f 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -30,9 +30,8 @@ pub(super) async fn spawn_review_thread( .models_manager .list_models(RefreshStrategy::OnlineIfUncached) .await; - let shell_command_backend = shell_command_backend_for_features(review_features.get()); let unified_exec_shell_mode = UnifiedExecShellMode::for_session( - shell_command_backend, + codex_tools::unified_exec_uses_zsh_fork_for_features(review_features.get()), crate::tools::tool_user_shell_type(sess.services.user_shell.as_ref()), sess.services.shell_zsh_path.as_ref(), sess.services.main_execve_wrapper_exe.as_ref(), diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index dba5d16a75..865cc44eae 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -465,10 +465,8 @@ impl Session { let provider_for_context = create_model_provider(provider, auth_manager); let session_telemetry_for_context = session_telemetry; let available_models = models_manager.try_list_models().unwrap_or_default(); - let shell_command_backend = - shell_command_backend_for_features(per_turn_config.features.get()); let unified_exec_shell_mode = UnifiedExecShellMode::for_session( - shell_command_backend, + codex_tools::unified_exec_uses_zsh_fork_for_features(per_turn_config.features.get()), crate::tools::tool_user_shell_type(user_shell), shell_zsh_path, main_execve_wrapper_exe, diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index a274f934c4..d5c027d4fa 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -382,6 +382,46 @@ async fn shell_family_registers_visible_unified_exec_and_hidden_legacy_shell() { assert_eq!(plan.exposure("shell_command"), ToolExposure::Hidden); } +#[tokio::test] +async fn shell_zsh_fork_stays_standalone_until_unified_exec_composition_is_enabled() { + let standalone = probe(|turn| { + set_features(turn, &[Feature::ShellTool, Feature::UnifiedExec]); + set_feature(turn, Feature::ShellZshFork, /*enabled*/ true); + set_feature(turn, Feature::UnifiedExecZshFork, /*enabled*/ false); + turn.model_info.shell_type = ConfigShellToolType::ShellCommand; + }) + .await; + + standalone.assert_visible_contains(&["shell_command"]); + standalone.assert_visible_lacks(&["exec_command", "write_stdin"]); + standalone.assert_registered_contains(&["shell_command"]); + standalone.assert_registered_lacks(&["exec_command", "write_stdin"]); + + let composed = probe(|turn| { + set_features( + turn, + &[ + Feature::ShellTool, + Feature::UnifiedExec, + Feature::ShellZshFork, + Feature::UnifiedExecZshFork, + ], + ); + turn.model_info.shell_type = ConfigShellToolType::ShellCommand; + }) + .await; + + if codex_utils_pty::conpty_supported() { + composed.assert_visible_contains(&["exec_command", "write_stdin"]); + composed.assert_visible_lacks(&["shell_command"]); + composed.assert_registered_contains(&["exec_command", "write_stdin", "shell_command"]); + assert_eq!(composed.exposure("shell_command"), ToolExposure::Hidden); + } else { + composed.assert_visible_contains(&["shell_command"]); + composed.assert_visible_lacks(&["exec_command", "write_stdin"]); + } +} + #[tokio::test] async fn environment_count_controls_environment_backed_tools() { let no_environment = probe(|turn| { diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 116db0a5b5..65fa160cc4 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -90,6 +90,8 @@ pub enum Feature { UnifiedExec, /// Route shell tool execution through the zsh exec bridge. ShellZshFork, + /// Route unified exec through the zsh exec bridge when both are enabled. + UnifiedExecZshFork, /// Reflow transcript scrollback when the terminal is resized. TerminalResizeReflow, /// Stream structured progress while apply_patch input is being generated. @@ -739,6 +741,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, + FeatureSpec { + id: Feature::UnifiedExecZshFork, + key: "unified_exec_zsh_fork", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::ShellSnapshot, key: "shell_snapshot", diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 91fa5830a3..dc582d98b3 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -75,6 +75,7 @@ pub use tool_config::ZshForkConfig; pub use tool_config::request_user_input_available_modes; pub use tool_config::shell_command_backend_for_features; pub use tool_config::shell_type_for_model_and_features; +pub use tool_config::unified_exec_uses_zsh_fork_for_features; pub use tool_definition::ToolDefinition; pub use tool_discovery::DiscoverablePluginInfo; pub use tool_discovery::DiscoverableTool; diff --git a/codex-rs/tools/src/tool_config.rs b/codex-rs/tools/src/tool_config.rs index 3eaa73f189..5a1a0fbc05 100644 --- a/codex-rs/tools/src/tool_config.rs +++ b/codex-rs/tools/src/tool_config.rs @@ -41,6 +41,19 @@ pub fn shell_command_backend_for_features(features: &Features) -> ShellCommandBa } } +/// Returns whether unified exec should launch local zsh commands through the +/// zsh fork. +/// +/// `unified_exec_zsh_fork` is only a composition gate. It does not enable +/// either underlying shell mode on its own, so disabling `unified_exec` or +/// `shell_zsh_fork` keeps those features independently off. +pub fn unified_exec_uses_zsh_fork_for_features(features: &Features) -> bool { + features.enabled(Feature::ShellTool) + && features.enabled(Feature::UnifiedExec) + && features.enabled(Feature::ShellZshFork) + && features.enabled(Feature::UnifiedExecZshFork) +} + pub fn shell_type_for_model_and_features( model_info: &ModelInfo, features: &Features, @@ -58,7 +71,9 @@ pub fn shell_type_for_model_and_features( if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled - } else if features.enabled(Feature::ShellZshFork) { + } else if features.enabled(Feature::ShellZshFork) + && !unified_exec_uses_zsh_fork_for_features(features) + { ConfigShellToolType::ShellCommand } else if unified_exec_enabled { if codex_utils_pty::conpty_supported() { @@ -85,13 +100,13 @@ pub struct ZshForkConfig { impl UnifiedExecShellMode { pub fn for_session( - shell_command_backend: ShellCommandBackendConfig, + use_zsh_fork: bool, user_shell_type: ToolUserShellType, shell_zsh_path: Option<&PathBuf>, main_execve_wrapper_exe: Option<&PathBuf>, ) -> Self { if cfg!(unix) - && shell_command_backend == ShellCommandBackendConfig::ZshFork + && use_zsh_fork && matches!(user_shell_type, ToolUserShellType::Zsh) && let (Some(shell_zsh_path), Some(main_execve_wrapper_exe)) = (shell_zsh_path, main_execve_wrapper_exe) diff --git a/codex-rs/tools/src/tool_config_tests.rs b/codex-rs/tools/src/tool_config_tests.rs index 033bf07b10..e02d8e07d3 100644 --- a/codex-rs/tools/src/tool_config_tests.rs +++ b/codex-rs/tools/src/tool_config_tests.rs @@ -81,6 +81,12 @@ fn shell_type_is_derived_from_model_and_feature_gates() { ConfigShellToolType::ShellCommand ); + features.enable(Feature::UnifiedExecZshFork); + assert_eq!( + shell_type_for_model_and_features(&model, &features), + expected_unified_exec + ); + features.disable(Feature::ShellTool); assert_eq!( shell_type_for_model_and_features(&model, &features), @@ -109,6 +115,22 @@ fn shell_command_backend_requires_both_shell_tool_and_zsh_fork() { ); } +#[test] +fn unified_exec_uses_zsh_fork_only_when_composition_and_dependencies_are_enabled() { + let mut features = shell_features(); + assert_eq!(unified_exec_uses_zsh_fork_for_features(&features), false); + + features.enable(Feature::UnifiedExec); + features.enable(Feature::ShellZshFork); + assert_eq!(unified_exec_uses_zsh_fork_for_features(&features), false); + + features.enable(Feature::UnifiedExecZshFork); + assert_eq!(unified_exec_uses_zsh_fork_for_features(&features), true); + + features.disable(Feature::ShellTool); + assert_eq!(unified_exec_uses_zsh_fork_for_features(&features), false); +} + #[test] fn request_user_input_modes_follow_default_mode_feature() { let mut features = Features::with_defaults(); @@ -131,7 +153,7 @@ fn unified_exec_shell_mode_uses_zsh_fork_only_when_all_inputs_match() { let shell = exe.clone(); let mode = UnifiedExecShellMode::for_session( - ShellCommandBackendConfig::ZshFork, + /*use_zsh_fork*/ true, ToolUserShellType::Zsh, Some(&shell), Some(&exe), @@ -144,7 +166,7 @@ fn unified_exec_shell_mode_uses_zsh_fork_only_when_all_inputs_match() { assert_eq!( UnifiedExecShellMode::for_session( - ShellCommandBackendConfig::Classic, + /*use_zsh_fork*/ false, ToolUserShellType::Zsh, Some(&shell), Some(&exe),