diff --git a/codex-rs/core/src/tools/handlers/shell_spec.rs b/codex-rs/core/src/tools/handlers/shell_spec.rs index 361f73bc0..13c531eab 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 c11f34e92..b7c0db724 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 { @@ -88,6 +95,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.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 170c1bd72..6fa2e4068 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -7,6 +7,7 @@ use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; use crate::tools::registry::PostToolUsePayload; +use codex_exec_server::Environment; use codex_protocol::models::AdditionalPermissionProfile; use codex_tools::UnifiedExecShellMode; use serde::Deserialize; @@ -124,14 +125,33 @@ pub(crate) fn get_command( shell_type: shell.shell_type.clone(), }) } - UnifiedExecShellMode::ZshFork(zsh_fork_config) => Ok(ResolvedCommand { - command: vec![ - zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(), - if use_login_shell { "-lc" } else { "-c" }.to_string(), - args.cmd.clone(), - ], - shell_type: ShellType::Zsh, - }), + UnifiedExecShellMode::ZshFork(zsh_fork_config) => { + if args.shell.is_some() { + return Err( + "`shell` is not supported for local zsh-fork exec; omit `shell` to use zsh-fork, or target a remote environment where `shell` is supported.".to_string(), + ); + } + + Ok(ResolvedCommand { + command: vec![ + zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(), + if use_login_shell { "-lc" } else { "-c" }.to_string(), + args.cmd.clone(), + ], + shell_type: ShellType::Zsh, + }) + } + } +} + +pub(crate) fn shell_mode_for_environment( + turn_shell_mode: &UnifiedExecShellMode, + environment: &Environment, +) -> UnifiedExecShellMode { + if environment.is_remote() { + UnifiedExecShellMode::Direct + } else { + turn_shell_mode.clone() } } 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 83236881f..c26fa107c 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 @@ -38,12 +38,14 @@ use super::ExecCommandArgs; use super::ExecCommandEnvironmentArgs; use super::get_command; use super::post_unified_exec_tool_use_payload; +use super::shell_mode_for_environment; #[derive(Clone, Copy)] 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 +59,7 @@ impl Default for ExecCommandHandler { allow_login_shell: false, exec_permission_approvals_enabled: false, include_environment_id: false, + include_shell_parameter: true, }, } } @@ -81,6 +84,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, ) } @@ -140,10 +144,12 @@ impl ToolExecutor for ExecCommandHandler { ) .await; let process_id = manager.allocate_process_id().await; + let shell_mode = + shell_mode_for_environment(&turn.unified_exec_shell_mode, environment.as_ref()); let resolved_command = get_command( &args, session.user_shell(), - &turn.unified_exec_shell_mode, + &shell_mode, turn.config.permissions.allow_login_shell, ) .map_err(FunctionCallError::RespondToModel)?; @@ -260,6 +266,7 @@ impl ToolExecutor for ExecCommandHandler { cwd, sandbox_cwd: turn_environment.cwd.clone(), environment, + shell_mode, network: context.turn.network.clone(), tty, sandbox_permissions: effective_additional_permissions.sandbox_permissions, diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 0eb9ebbed..b33a0e85b 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -1,6 +1,7 @@ use super::*; use crate::shell::ShellType; use crate::shell::default_user_shell; +use codex_exec_server::Environment; use codex_tools::UnifiedExecShellMode; use codex_tools::ZshForkConfig; use codex_utils_absolute_path::AbsolutePathBuf; @@ -165,7 +166,7 @@ fn test_get_command_rejects_explicit_login_when_disallowed() -> anyhow::Result<( } #[test] -fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result<()> { +fn test_get_command_rejects_explicit_shell_in_zsh_fork_mode() -> anyhow::Result<()> { let json = r#"{"cmd": "echo hello", "shell": "/bin/bash"}"#; let args: ExecCommandArgs = parse_arguments(json)?; let shell_zsh_path = AbsolutePathBuf::from_absolute_path(if cfg!(windows) { @@ -174,7 +175,7 @@ fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< "/opt/codex/zsh" })?; let shell_mode = UnifiedExecShellMode::ZshFork(ZshForkConfig { - shell_zsh_path: shell_zsh_path.clone(), + shell_zsh_path, main_execve_wrapper_exe: AbsolutePathBuf::from_absolute_path(if cfg!(windows) { r"C:\opt\codex\codex-execve-wrapper" } else { @@ -182,23 +183,50 @@ fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< })?, }); - let resolved = get_command( + let err = get_command( &args, Arc::new(default_user_shell()), &shell_mode, /*allow_login_shell*/ true, ) - .map_err(anyhow::Error::msg)?; + .expect_err("explicit shell should be rejected"); + + assert!( + err.contains("`shell` is not supported for local zsh-fork exec"), + "unexpected error: {err}" + ); + Ok(()) +} + +#[tokio::test] +async fn shell_mode_for_environment_uses_direct_mode_for_remote_environments() -> anyhow::Result<()> +{ + let shell_zsh_path = AbsolutePathBuf::from_absolute_path(if cfg!(windows) { + r"C:\opt\codex\zsh" + } else { + "/opt/codex/zsh" + })?; + let shell_mode = UnifiedExecShellMode::ZshFork(ZshForkConfig { + shell_zsh_path, + main_execve_wrapper_exe: AbsolutePathBuf::from_absolute_path(if cfg!(windows) { + r"C:\opt\codex\codex-execve-wrapper" + } else { + "/opt/codex/codex-execve-wrapper" + })?, + }); + let local_environment = Environment::default_for_tests(); + let remote_environment = + Environment::create_for_tests(Some("ws://127.0.0.1:1/remote-exec-server".to_string()))?; assert_eq!( - resolved.command, - vec![ - shell_zsh_path.to_string_lossy().to_string(), - "-lc".to_string(), - "echo hello".to_string() - ] + shell_mode_for_environment(&shell_mode, &local_environment), + shell_mode ); - assert_eq!(resolved.shell_type, ShellType::Zsh); + assert_eq!( + shell_mode_for_environment(&shell_mode, &remote_environment), + UnifiedExecShellMode::Direct + ); + Ok(()) } diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 3c8abf8f1..c7d9ae5ae 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -73,6 +73,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; @@ -564,6 +565,7 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut Planne allow_login_shell, exec_permission_approvals_enabled, include_environment_id, + include_shell_parameter: unified_exec_should_include_shell_parameter(turn_context), })); planned_tools.add(WriteStdinHandler); @@ -580,6 +582,17 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut Planne } } +fn unified_exec_should_include_shell_parameter(turn_context: &TurnContext) -> bool { + !matches!( + &turn_context.unified_exec_shell_mode, + UnifiedExecShellMode::ZshFork(_) + ) || turn_context + .environments + .turn_environments + .iter() + .any(|environment| environment.environment.is_remote()) +} + fn add_mcp_resource_tools(context: &CoreToolPlanContext<'_>, planned_tools: &mut PlannedTools) { if context.mcp_tools.is_some() { planned_tools.add(ListMcpResourcesHandler); diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 3f832ed0a..e129e4dda 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -233,6 +233,21 @@ fn set_features(turn: &mut TurnContext, features: &[Feature]) { } } +fn zsh_fork_config_for_spec_plan_tests() -> codex_tools::ZshForkConfig { + let placeholder_exe = codex_utils_absolute_path::AbsolutePathBuf::try_from( + std::env::current_exe().expect("current exe path"), + ) + .expect("current exe should be absolute"); + + // Spec planning only checks whether the shell mode is ZshFork. These paths + // are never executed, so use a stable absolute placeholder instead of + // depending on packaged zsh-fork artifacts in schema tests. + codex_tools::ZshForkConfig { + shell_zsh_path: placeholder_exe.clone(), + main_execve_wrapper_exe: placeholder_exe, + } +} + fn update_config(turn: &mut TurnContext, update: impl FnOnce(&mut crate::config::Config)) { let mut config = (*turn.config).clone(); update(&mut config); @@ -406,6 +421,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] @@ -448,6 +464,79 @@ 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 plan = probe(|turn| { + set_features( + turn, + &[ + Feature::ShellTool, + Feature::UnifiedExec, + Feature::ShellZshFork, + Feature::UnifiedExecZshFork, + ], + ); + turn.unified_exec_shell_mode = + codex_tools::UnifiedExecShellMode::ZshFork(zsh_fork_config_for_spec_plan_tests()); + }) + .await; + + plan.assert_visible_contains(&["exec_command", "write_stdin"]); + assert!(!has_parameter(plan.visible_spec("exec_command"), "shell")); +} + +#[tokio::test] +async fn zsh_fork_unified_exec_keeps_shell_parameter_when_remote_environment_available() { + if !codex_utils_pty::conpty_supported() { + return; + } + + let plan = probe(|turn| { + set_features( + turn, + &[ + Feature::ShellTool, + Feature::UnifiedExec, + Feature::ShellZshFork, + Feature::UnifiedExecZshFork, + ], + ); + turn.unified_exec_shell_mode = + codex_tools::UnifiedExecShellMode::ZshFork(zsh_fork_config_for_spec_plan_tests()); + let remote_cwd = turn + .environments + .primary() + .expect("primary environment") + .cwd + .clone(); + turn.environments + .turn_environments + .push(crate::session::turn_context::TurnEnvironment { + environment_id: "remote".to_string(), + environment: Arc::new( + codex_exec_server::Environment::create_for_tests(Some( + "ws://127.0.0.1:1/remote-exec-server".to_string(), + )) + .expect("remote test environment"), + ), + cwd: remote_cwd, + shell: None, + }); + }) + .await; + + plan.assert_visible_contains(&["exec_command", "write_stdin"]); + assert!(has_parameter(plan.visible_spec("exec_command"), "shell")); + assert!(has_parameter( + plan.visible_spec("exec_command"), + "environment_id" + )); +} + #[tokio::test] async fn environment_count_controls_environment_backed_tools() { let no_environment = probe(|turn| { diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 6be32d26e..94a7ab7c7 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -30,6 +30,7 @@ use std::sync::Weak; use codex_exec_server::Environment; use codex_network_proxy::NetworkProxy; use codex_protocol::models::AdditionalPermissionProfile; +use codex_tools::UnifiedExecShellMode; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::TruncationPolicy; use rand::Rng; @@ -97,6 +98,7 @@ pub(crate) struct ExecCommandRequest { pub cwd: AbsolutePathBuf, pub sandbox_cwd: AbsolutePathBuf, pub environment: Arc, + pub shell_mode: UnifiedExecShellMode, pub network: Option, pub tty: bool, pub sandbox_permissions: SandboxPermissions, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 83c750ebc..563d486c6 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1010,8 +1010,7 @@ impl UnifiedExecProcessManager { local_policy_env, }; let mut orchestrator = ToolOrchestrator::new(); - let mut runtime = - UnifiedExecRuntime::new(self, context.turn.unified_exec_shell_mode.clone()); + let mut runtime = UnifiedExecRuntime::new(self, request.shell_mode.clone()); let file_system_sandbox_policy = context.turn.file_system_sandbox_policy(); let exec_approval_requirement = context .session diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 4d0597a84..9aa0e7381 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -179,6 +179,7 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() { .environments .primary_environment() .expect("primary environment"), + shell_mode: codex_tools::UnifiedExecShellMode::Direct, network: None, tty: true, sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,