From 9ecac8b2182b1edcafed2636971639db6981e41c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 28 May 2026 15:54:02 -0700 Subject: [PATCH] refactor: hide shell override for zsh fork unified exec --- .../core/src/tools/handlers/shell_spec.rs | 19 +++++++---- .../src/tools/handlers/shell_spec_tests.rs | 22 ++++++++++++ .../handlers/unified_exec/exec_command.rs | 3 ++ codex-rs/core/src/tools/spec_plan.rs | 5 +++ codex-rs/core/src/tools/spec_plan_tests.rs | 34 +++++++++++++++++++ 5 files changed, 76 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/shell_spec.rs b/codex-rs/core/src/tools/handlers/shell_spec.rs index 43b78e3afe..7393eeee59 100644 --- a/codex-rs/core/src/tools/handlers/shell_spec.rs +++ b/codex-rs/core/src/tools/handlers/shell_spec.rs @@ -13,12 +13,15 @@ pub struct CommandToolOptions { #[cfg(test)] pub fn create_exec_command_tool(options: CommandToolOptions) -> ToolSpec { - create_exec_command_tool_with_environment_id(options, /*include_environment_id*/ false) + create_exec_command_tool_with_environment_id( + options, /*include_environment_id*/ false, /*include_shell_parameter*/ true, + ) } pub(crate) fn create_exec_command_tool_with_environment_id( options: CommandToolOptions, include_environment_id: bool, + include_shell_parameter: bool, ) -> ToolSpec { let mut properties = BTreeMap::from([ ( @@ -32,12 +35,6 @@ pub(crate) fn create_exec_command_tool_with_environment_id( .to_string(), )), ), - ( - "shell".to_string(), - JsonSchema::string(Some( - "Shell binary to launch. Defaults to the user's default shell.".to_string(), - )), - ), ( "tty".to_string(), JsonSchema::boolean(Some( @@ -58,6 +55,14 @@ pub(crate) fn create_exec_command_tool_with_environment_id( )), ), ]); + if include_shell_parameter { + properties.insert( + "shell".to_string(), + JsonSchema::string(Some( + "Shell binary to launch. Defaults to the user's default shell.".to_string(), + )), + ); + } if options.allow_login_shell { properties.insert( "login".to_string(), diff --git a/codex-rs/core/src/tools/handlers/shell_spec_tests.rs b/codex-rs/core/src/tools/handlers/shell_spec_tests.rs index 4774815fb2..1d1349ccb4 100644 --- a/codex-rs/core/src/tools/handlers/shell_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_spec_tests.rs @@ -6,6 +6,13 @@ fn windows_shell_guidance_description() -> String { format!("\n\n{}", windows_shell_guidance()) } +fn has_parameter(tool: &ToolSpec, parameter_name: &str) -> bool { + serde_json::to_value(tool) + .expect("tool spec should serialize") + .pointer(&format!("/parameters/properties/{parameter_name}")) + .is_some() +} + #[test] fn exec_command_tool_matches_expected_spec() { let tool = create_exec_command_tool(CommandToolOptions { @@ -89,6 +96,21 @@ fn exec_command_tool_matches_expected_spec() { ); } +#[test] +fn exec_command_tool_can_hide_shell_parameter() { + let tool = create_exec_command_tool_with_environment_id( + CommandToolOptions { + allow_login_shell: true, + exec_permission_approvals_enabled: false, + }, + /*include_environment_id*/ false, + /*include_shell_parameter*/ false, + ); + + assert!(!has_parameter(&tool, "shell")); + assert!(has_parameter(&tool, "cmd")); +} + #[test] fn write_stdin_tool_matches_expected_spec() { let tool = create_write_stdin_tool(); diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 83236881f3..72ac23691a 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -44,6 +44,7 @@ pub(crate) struct ExecCommandHandlerOptions { pub(crate) allow_login_shell: bool, pub(crate) exec_permission_approvals_enabled: bool, pub(crate) include_environment_id: bool, + pub(crate) include_shell_parameter: bool, } pub struct ExecCommandHandler { @@ -57,6 +58,7 @@ impl Default for ExecCommandHandler { allow_login_shell: false, exec_permission_approvals_enabled: false, include_environment_id: false, + include_shell_parameter: true, }, } } @@ -81,6 +83,7 @@ impl ToolExecutor for ExecCommandHandler { exec_permission_approvals_enabled: self.options.exec_permission_approvals_enabled, }, self.options.include_environment_id, + self.options.include_shell_parameter, ) } diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index bb7baf4bd8..50935ccd70 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -72,6 +72,7 @@ use codex_tools::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolOutput; use codex_tools::ToolSpec; +use codex_tools::UnifiedExecShellMode; use codex_tools::can_request_original_image_detail; use codex_tools::collect_code_mode_exec_prompt_tool_definitions; use codex_tools::collect_request_plugin_install_entries; @@ -566,6 +567,10 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut Planne allow_login_shell, exec_permission_approvals_enabled, include_environment_id, + include_shell_parameter: !matches!( + &turn_context.unified_exec_shell_mode, + UnifiedExecShellMode::ZshFork(_) + ), })); planned_tools.add(WriteStdinHandler); diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index d5c027d4fa..af2f3c9162 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -380,6 +380,7 @@ async fn shell_family_registers_visible_unified_exec_and_hidden_legacy_shell() { plan.assert_visible_lacks(&["shell_command"]); plan.assert_registered_contains(&["exec_command", "write_stdin", "shell_command"]); assert_eq!(plan.exposure("shell_command"), ToolExposure::Hidden); + assert!(has_parameter(plan.visible_spec("exec_command"), "shell")); } #[tokio::test] @@ -422,6 +423,39 @@ async fn shell_zsh_fork_stays_standalone_until_unified_exec_composition_is_enabl } } +#[tokio::test] +async fn zsh_fork_unified_exec_hides_shell_parameter() { + if !codex_utils_pty::conpty_supported() { + return; + } + + let exe = codex_utils_absolute_path::AbsolutePathBuf::try_from( + std::env::current_exe().expect("current exe path"), + ) + .expect("current exe should be absolute"); + + let plan = probe(|turn| { + set_features( + turn, + &[ + Feature::ShellTool, + Feature::UnifiedExec, + Feature::ShellZshFork, + Feature::UnifiedExecZshFork, + ], + ); + turn.unified_exec_shell_mode = + codex_tools::UnifiedExecShellMode::ZshFork(codex_tools::ZshForkConfig { + shell_zsh_path: exe.clone(), + main_execve_wrapper_exe: exe, + }); + }) + .await; + + plan.assert_visible_contains(&["exec_command", "write_stdin"]); + assert!(!has_parameter(plan.visible_spec("exec_command"), "shell")); +} + #[tokio::test] async fn environment_count_controls_environment_backed_tools() { let no_environment = probe(|turn| {