mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
refactor: hide shell override for zsh fork unified exec (#24980)
## Why When unified exec is configured to launch through the zsh fork, local commands should not let the model override the shell binary with the `shell` parameter. The configured zsh fork is the mechanism that makes `execv(2)` interception reliable, so exposing `shell` for local zsh-fork execution would create a confusing API surface and undermine the composition. Remote environments are different: zsh-fork interception is local-only, so remote unified-exec calls must keep direct unified-exec behavior and still expose `shell` when a remote environment can be selected. ## What Changed - Taught the `exec_command` schema builder to omit the `shell` parameter when requested. - Hid `shell` from the unified-exec tool schema only when zsh-fork unified exec applies to all selectable environments. - Kept `shell` visible when any remote environment can be targeted, because those calls run through direct unified exec. - Made unified exec choose the effective shell mode per selected environment: local environments keep zsh-fork mode, remote environments use direct mode. - Left direct unified-exec behavior unchanged, including support for model-specified shells there. ## Verification - Added schema coverage showing `exec_command` can hide `shell`. - Added planner coverage showing zsh-fork unified exec hides `shell` for local-only execution while direct unified exec still exposes it. - Added planner coverage showing `shell` remains visible when a remote environment is available. - Added handler coverage showing remote environments use direct unified-exec shell mode instead of zsh-fork mode. - Ran the focused `codex-core` shell-parameter and zsh-fork tests. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24980). * #24982 * #24981 * __->__ #24980
This commit is contained in:
committed by
GitHub
Unverified
parent
d6748f741a
commit
feb9eddc51
@@ -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(),
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<ToolInvocation> 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<ToolInvocation> 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<ToolInvocation> 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,
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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| {
|
||||
|
||||
@@ -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<Environment>,
|
||||
pub shell_mode: UnifiedExecShellMode,
|
||||
pub network: Option<NetworkProxy>,
|
||||
pub tty: bool,
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user