Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions codex-rs/core/src/tools/handlers/shell_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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([
(
Expand All @@ -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(
Expand All @@ -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(),
Expand Down
22 changes: 22 additions & 0 deletions codex-rs/core/src/tools/handlers/shell_spec_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down
36 changes: 28 additions & 8 deletions codex-rs/core/src/tools/handlers/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
},
}
}
Expand All @@ -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,
)
}

Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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,
Expand Down
50 changes: 39 additions & 11 deletions codex-rs/core/src/tools/handlers/unified_exec_tests.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -174,31 +175,58 @@ 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 {
"/opt/codex/codex-execve-wrapper"
})?,
});

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(())
}

Expand Down
13 changes: 13 additions & 0 deletions codex-rs/core/src/tools/spec_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still leaves shell as a silent no-op in mixed-env sessions: once any remote env exists we expose it, but local execs still ignore it under zsh-for k

Should we reject shell for local targets instead of silently dropping it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[codex] Agreed. This is now changed on #24980: local zsh-fork unified exec rejects an explicit shell instead of silently ignoring it, with an error telling the model to omit shell or target a remote environment. Remote targets still map to direct unified exec via shell_mode_for_environment(), so their shell parameter remains supported.

!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);
Expand Down
Loading
Loading