mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Restrict shell_command to a single local environment (#36329)
## What changed - Register the legacy `shell_command` handler only when exactly one local environment is available, including when unified exec keeps it hidden for compatibility. - Reserve the unnamespaced `shell_command` name so external tools cannot claim it when the built-in handler is unavailable. Namespaced tools with that name remain supported. ## Testing - Cover remote and multiple-environment tool plans, unified exec routing, and external tool registration for reserved and namespaced variants. GitOrigin-RevId: 93834539b31ac8a7fea072bfa4de792bcc3e053b
This commit is contained in:
@@ -384,7 +384,13 @@ impl ToolRegistry {
|
||||
}
|
||||
|
||||
pub(crate) fn register_external(&mut self, runtime: Arc<dyn CoreToolRuntime>) -> bool {
|
||||
match self.tools.entry(runtime.tool_name()) {
|
||||
let tool_name = runtime.tool_name();
|
||||
if tool_name.namespace.is_none() && tool_name.name == "shell_command" {
|
||||
tracing::warn!(tool_name = %tool_name, "skipping external tool with reserved name");
|
||||
return false;
|
||||
}
|
||||
|
||||
match self.tools.entry(tool_name) {
|
||||
Entry::Vacant(entry) => {
|
||||
entry.insert(runtime);
|
||||
true
|
||||
|
||||
@@ -234,6 +234,26 @@ fn registry_preserves_external_winners_and_trusted_synthetic_order() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn reserved_shell_command_rejects_external_runtimes_without_a_builtin() {
|
||||
let handler = |tool_name| Arc::new(TestHandler { tool_name }) as Arc<dyn CoreToolRuntime>;
|
||||
let shell_command_name = codex_tools::ToolName::plain("shell_command");
|
||||
let namespaced_shell_command_name =
|
||||
codex_tools::ToolName::namespaced("client", "shell_command");
|
||||
let mut registry = ToolRegistry::default();
|
||||
|
||||
assert!(!registry.register_external(handler(shell_command_name.clone())));
|
||||
assert!(registry.tool(&shell_command_name).is_none());
|
||||
|
||||
let namespaced_handler = handler(namespaced_shell_command_name.clone());
|
||||
assert!(registry.register_external(Arc::clone(&namespaced_handler)));
|
||||
assert!(
|
||||
registry
|
||||
.tool(&namespaced_shell_command_name)
|
||||
.is_some_and(|runtime| Arc::ptr_eq(&runtime, &namespaced_handler))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn readiness_selects_exact_tool_and_forwards_exposure_overrides() {
|
||||
let (session, _turn) = crate::session::tests::make_session_and_context().await;
|
||||
|
||||
@@ -728,6 +728,7 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistr
|
||||
let allow_login_shell = turn_context.config.permissions.allow_login_shell;
|
||||
let exec_permission_approvals_enabled = features.enabled(Feature::ExecPermissionApprovals);
|
||||
let include_environment_id = matches!(environment_mode, ToolEnvironmentMode::Multiple);
|
||||
let supports_shell_command = context.environments.single_local_environment().is_some();
|
||||
let shell_command_options = ShellCommandHandlerOptions {
|
||||
backend_config: shell_command_backend_for_features(features),
|
||||
allow_login_shell,
|
||||
@@ -747,18 +748,22 @@ fn add_shell_tools(context: &CoreToolPlanContext<'_>, registry: &mut ToolRegistr
|
||||
}));
|
||||
registry.add(WriteStdinHandler);
|
||||
|
||||
// Keep the legacy shell tool registered while unified exec is
|
||||
// model-visible.
|
||||
registry.add_with_exposure(
|
||||
ShellCommandHandler::new(shell_command_options),
|
||||
ToolExposure::Hidden,
|
||||
);
|
||||
if supports_shell_command {
|
||||
// Keep the legacy shell tool registered while unified exec is
|
||||
// model-visible.
|
||||
registry.add_with_exposure(
|
||||
ShellCommandHandler::new(shell_command_options),
|
||||
ToolExposure::Hidden,
|
||||
);
|
||||
}
|
||||
}
|
||||
ConfigShellToolType::Disabled => {}
|
||||
ConfigShellToolType::Default
|
||||
| ConfigShellToolType::Local
|
||||
| ConfigShellToolType::ShellCommand => {
|
||||
registry.add(ShellCommandHandler::new(shell_command_options));
|
||||
if supports_shell_command {
|
||||
registry.add(ShellCommandHandler::new(shell_command_options));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -658,6 +658,78 @@ async fn shell_family_registers_visible_unified_exec_and_hidden_legacy_shell() {
|
||||
assert!(has_parameter(plan.visible_spec("exec_command"), "shell"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn shell_command_is_not_registered_without_a_single_local_environment() {
|
||||
let remote_environment = probe(|turn| {
|
||||
set_feature(turn, Feature::ShellTool, /*enabled*/ true);
|
||||
set_feature(turn, Feature::UnifiedExec, /*enabled*/ false);
|
||||
turn.model_info.shell_type = ConfigShellToolType::ShellCommand;
|
||||
|
||||
let TurnEnvironmentState::Ready(environment) = turn
|
||||
.environments
|
||||
.environments
|
||||
.first_mut()
|
||||
.expect("primary environment")
|
||||
else {
|
||||
panic!("primary environment should be ready");
|
||||
};
|
||||
environment.environment_id = "remote".to_string();
|
||||
environment.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"),
|
||||
);
|
||||
})
|
||||
.await;
|
||||
remote_environment.assert_visible_lacks(&["shell_command", "exec_command", "write_stdin"]);
|
||||
remote_environment.assert_registered_lacks(&["shell_command", "exec_command", "write_stdin"]);
|
||||
|
||||
let multiple_local_environments = probe(|turn| {
|
||||
set_feature(turn, Feature::ShellTool, /*enabled*/ true);
|
||||
set_feature(turn, Feature::UnifiedExec, /*enabled*/ false);
|
||||
turn.model_info.shell_type = ConfigShellToolType::ShellCommand;
|
||||
duplicate_primary_environment(turn);
|
||||
})
|
||||
.await;
|
||||
multiple_local_environments.assert_visible_lacks(&["shell_command"]);
|
||||
multiple_local_environments.assert_registered_lacks(&["shell_command"]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dynamic_tools_cannot_reclaim_the_reserved_shell_command_name() {
|
||||
let plan = probe_with(
|
||||
duplicate_primary_environment,
|
||||
ToolPlanInputs {
|
||||
dynamic_tools: vec![
|
||||
dynamic_tool(
|
||||
/*namespace*/ None,
|
||||
"shell_command",
|
||||
/*defer_loading*/ false,
|
||||
),
|
||||
dynamic_tool(
|
||||
Some("client"),
|
||||
"shell_command",
|
||||
/*defer_loading*/ false,
|
||||
),
|
||||
],
|
||||
..ToolPlanInputs::default()
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
plan.assert_visible_lacks(&["shell_command"]);
|
||||
plan.assert_registered_lacks(&["shell_command"]);
|
||||
plan.assert_visible_contains(&["client"]);
|
||||
plan.assert_registered_contains(
|
||||
&[&ToolName::namespaced("client", "shell_command").to_string()],
|
||||
);
|
||||
assert_eq!(
|
||||
plan.namespace_function_names("client"),
|
||||
&["shell_command".to_string()]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn shell_zsh_fork_stays_standalone_until_unified_exec_composition_is_enabled() {
|
||||
let standalone = probe(|turn| {
|
||||
@@ -767,6 +839,8 @@ async fn zsh_fork_unified_exec_keeps_shell_parameter_when_remote_environment_ava
|
||||
.await;
|
||||
|
||||
plan.assert_visible_contains(&["exec_command", "write_stdin"]);
|
||||
plan.assert_visible_lacks(&["shell_command"]);
|
||||
plan.assert_registered_lacks(&["shell_command"]);
|
||||
assert!(has_parameter(plan.visible_spec("exec_command"), "shell"));
|
||||
assert!(has_parameter(
|
||||
plan.visible_spec("exec_command"),
|
||||
@@ -812,6 +886,8 @@ async fn environment_count_controls_environment_backed_tools() {
|
||||
"view_image",
|
||||
"request_permissions",
|
||||
]);
|
||||
multiple_environments.assert_visible_lacks(&["shell_command"]);
|
||||
multiple_environments.assert_registered_lacks(&["shell_command"]);
|
||||
assert!(has_parameter(
|
||||
multiple_environments.visible_spec("exec_command"),
|
||||
"environment_id"
|
||||
|
||||
@@ -272,11 +272,19 @@ async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> {
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let test = test_codex().build_with_auto_env(&server).await?;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.disable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
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(&"shell_command".to_string()));
|
||||
let environment_context = request
|
||||
.message_input_texts("user")
|
||||
.into_iter()
|
||||
@@ -1583,9 +1591,20 @@ async fn exec_command_routing_output(
|
||||
test.submit_turn_with_environments("route exec command", environments)
|
||||
.await?;
|
||||
|
||||
response_mock
|
||||
let output = response_mock
|
||||
.function_call_output_text(call_id)
|
||||
.with_context(|| format!("missing function_call_output for {call_id}"))
|
||||
.with_context(|| format!("missing function_call_output for {call_id}"))?;
|
||||
let request = response_mock
|
||||
.requests()
|
||||
.into_iter()
|
||||
.next()
|
||||
.context("initial model request should be recorded")?;
|
||||
let tools = tool_names(&request.body_json());
|
||||
assert!(tools.contains(&"exec_command".to_string()));
|
||||
assert!(tools.contains(&"write_stdin".to_string()));
|
||||
assert!(!tools.contains(&"shell_command".to_string()));
|
||||
|
||||
Ok(output)
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
|
||||
Reference in New Issue
Block a user