mirror of
https://github.com/openai/codex.git
synced 2026-10-20 18:27:36 +00:00
Match Windows shell guidance to the executor platform (#41368)
## What changed - Include Windows safety guidance in the `exec_command` tool description when a single ready environment reports Windows as its executor platform. - Omit that guidance when the selected executor reports a non-Windows platform. - Preserve the host-based behavior when platform information is unavailable or the tool can target multiple environments. ## Testing - Cover Windows, non-Windows, unknown-platform, and multi-environment tool plans. - Verify remote environments expose guidance that matches their target OS. GitOrigin-RevId: 95e09790446c684ffaff2a0be077db426fff4618
This commit is contained in:
@@ -14,7 +14,10 @@ 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, /*include_shell_parameter*/ true,
|
||||
options,
|
||||
/*include_environment_id*/ false,
|
||||
/*include_shell_parameter*/ true,
|
||||
/*include_windows_shell_guidance*/ cfg!(windows),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -22,6 +25,7 @@ pub(crate) fn create_exec_command_tool_with_environment_id(
|
||||
options: CommandToolOptions,
|
||||
include_environment_id: bool,
|
||||
include_shell_parameter: bool,
|
||||
include_windows_shell_guidance: bool,
|
||||
) -> ToolSpec {
|
||||
let yield_time_ms_description = if cfg!(windows) {
|
||||
"Maximum time to wait before returning a session ID for a still-running command. Commands that finish sooner return immediately. For ordinary commands, omit this parameter to use the 10000 ms default. Effective range on Windows is 10000-30000 ms."
|
||||
@@ -90,7 +94,7 @@ pub(crate) fn create_exec_command_tool_with_environment_id(
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: "exec_command".to_string(),
|
||||
description: if cfg!(windows) {
|
||||
description: if include_windows_shell_guidance {
|
||||
format!(
|
||||
"Runs a command in a PTY, returning output or a session ID for ongoing interaction.\n\n{}",
|
||||
windows_shell_guidance()
|
||||
|
||||
@@ -107,6 +107,7 @@ fn exec_command_tool_can_hide_shell_parameter() {
|
||||
},
|
||||
/*include_environment_id*/ false,
|
||||
/*include_shell_parameter*/ false,
|
||||
/*include_windows_shell_guidance*/ cfg!(windows),
|
||||
);
|
||||
|
||||
assert!(!has_parameter(&tool, "shell"));
|
||||
|
||||
@@ -54,6 +54,7 @@ pub(crate) struct ExecCommandHandlerOptions {
|
||||
pub(crate) exec_permission_approvals_enabled: bool,
|
||||
pub(crate) include_environment_id: bool,
|
||||
pub(crate) include_shell_parameter: bool,
|
||||
pub(crate) include_windows_shell_guidance: bool,
|
||||
}
|
||||
|
||||
pub struct ExecCommandHandler {
|
||||
@@ -68,6 +69,7 @@ impl Default for ExecCommandHandler {
|
||||
exec_permission_approvals_enabled: false,
|
||||
include_environment_id: false,
|
||||
include_shell_parameter: true,
|
||||
include_windows_shell_guidance: cfg!(windows),
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -92,6 +94,7 @@ impl ToolExecutor<ToolInvocation> for ExecCommandHandler {
|
||||
},
|
||||
self.options.include_environment_id,
|
||||
self.options.include_shell_parameter,
|
||||
self.options.include_windows_shell_guidance,
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -1009,6 +1009,9 @@ fn add_core_tool_sources(context: &CoreToolPlanContext<'_>, registry: &mut ToolR
|
||||
turn_context,
|
||||
context.environments,
|
||||
),
|
||||
include_windows_shell_guidance: should_include_windows_shell_guidance(
|
||||
context.environments,
|
||||
),
|
||||
}));
|
||||
registry.add(WriteStdinHandler);
|
||||
}
|
||||
@@ -1055,6 +1058,25 @@ fn any_environment_allows_login_shell(environments: &TurnEnvironmentSnapshot) ->
|
||||
.any(|environment| environment.config().allow_login_shell)
|
||||
}
|
||||
|
||||
fn should_include_windows_shell_guidance(environments: &TurnEnvironmentSnapshot) -> bool {
|
||||
let mut environments = environments.turn_environments();
|
||||
let Some(environment) = environments.next() else {
|
||||
return false;
|
||||
};
|
||||
let executor_platform_os = if environments.next().is_none() {
|
||||
environment.executor_platform_os.as_deref()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
// One tool schema can target any ready environment. Multi-environment turns and legacy
|
||||
// executors without platform OS information preserve the host-derived guidance.
|
||||
match executor_platform_os {
|
||||
Some(platform_os) => platform_os == "windows",
|
||||
None => cfg!(windows),
|
||||
}
|
||||
}
|
||||
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
fn add_shell_tools(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistry) {
|
||||
let turn_context = context.turn_context;
|
||||
@@ -1079,6 +1101,7 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistr
|
||||
turn_context,
|
||||
context.environments,
|
||||
),
|
||||
include_windows_shell_guidance: should_include_windows_shell_guidance(context.environments),
|
||||
}));
|
||||
registry.add(WriteStdinHandler);
|
||||
}
|
||||
|
||||
@@ -499,6 +499,13 @@ fn has_parameter(spec: &ToolSpec, parameter_name: &str) -> bool {
|
||||
.is_some()
|
||||
}
|
||||
|
||||
fn has_windows_shell_guidance(spec: &ToolSpec) -> bool {
|
||||
let ToolSpec::Function(tool) = spec else {
|
||||
return false;
|
||||
};
|
||||
tool.description.contains("Windows safety rules:")
|
||||
}
|
||||
|
||||
fn apply_patch_accepts_environment_id(spec: &ToolSpec) -> bool {
|
||||
match spec {
|
||||
ToolSpec::Freeform(tool) if tool.name == "apply_patch" => {
|
||||
@@ -899,6 +906,45 @@ async fn shell_family_registers_only_unified_exec_tools() {
|
||||
assert!(has_parameter(plan.visible_spec("exec_command"), "shell"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn exec_command_guidance_follows_executor_platform_and_fallbacks() {
|
||||
let opposite_host_os = if cfg!(windows) { "linux" } else { "windows" };
|
||||
for (platform_os, multiple_environments, expect_windows_guidance) in [
|
||||
(Some("windows"), false, true),
|
||||
(Some("linux"), false, false),
|
||||
(None, false, cfg!(windows)),
|
||||
(Some(opposite_host_os), true, cfg!(windows)),
|
||||
] {
|
||||
let plan = probe(|turn| {
|
||||
set_features(turn, &[Feature::ShellTool, Feature::UnifiedExec]);
|
||||
set_feature(turn, Feature::ShellZshFork, /*enabled*/ false);
|
||||
update_turn_settings_for_test(turn, |settings| {
|
||||
Arc::make_mut(&mut settings.model_info).shell_type =
|
||||
ConfigShellToolType::UnifiedExec;
|
||||
});
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
else {
|
||||
panic!("primary environment should be ready");
|
||||
};
|
||||
environment.executor_platform_os = platform_os.map(str::to_string);
|
||||
if multiple_environments {
|
||||
duplicate_primary_environment(turn);
|
||||
}
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
has_windows_shell_guidance(plan.visible_spec("exec_command")),
|
||||
expect_windows_guidance,
|
||||
"unexpected guidance for executor platform {platform_os:?} with multiple_environments={multiple_environments}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn login_shell_parameter_follows_selected_environment() {
|
||||
for guardian in [false, true] {
|
||||
|
||||
@@ -327,7 +327,7 @@ async fn remote_test_env_can_connect_and_use_filesystem() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> {
|
||||
async fn remote_test_env_exposes_target_shell_and_exec_guidance_to_model() -> Result<()> {
|
||||
skip_if_no_remote_env!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
@@ -340,19 +340,26 @@ async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> {
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.disable(Feature::ShellTool)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let mut builder = test_codex();
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
test.submit_turn("report remote environment").await?;
|
||||
|
||||
let request = response_mock.single_request();
|
||||
let tools = tool_names(&request.body_json());
|
||||
assert!(!tools.contains(&"exec_command".to_string()));
|
||||
let body = request.body_json();
|
||||
let exec_command = body["tools"]
|
||||
.as_array()
|
||||
.context("tools should be an array")?
|
||||
.iter()
|
||||
.find(|tool| tool["name"] == "exec_command")
|
||||
.context("exec_command should be available")?;
|
||||
let has_windows_guidance = exec_command["description"]
|
||||
.as_str()
|
||||
.is_some_and(|description| description.contains("Windows safety rules:"));
|
||||
assert_eq!(
|
||||
has_windows_guidance,
|
||||
matches!(test_target_os(), TestTargetOs::Windows)
|
||||
);
|
||||
let environment_context = request
|
||||
.message_input_texts("user")
|
||||
.into_iter()
|
||||
|
||||
Reference in New Issue
Block a user