From ef293f7ac9d756f793f3e952a790f9bec16a6eeb Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 31 Jul 2026 13:54:11 +0000 Subject: [PATCH] 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 --- codex-rs/core/src/tools/registry.rs | 8 ++- codex-rs/core/src/tools/registry_tests.rs | 20 ++++++ codex-rs/core/src/tools/spec_plan.rs | 19 ++++-- codex-rs/core/src/tools/spec_plan_tests.rs | 76 ++++++++++++++++++++++ codex-rs/core/tests/suite/remote_env.rs | 25 ++++++- 5 files changed, 137 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 64b8e819d5..a3871064e5 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -384,7 +384,13 @@ impl ToolRegistry { } pub(crate) fn register_external(&mut self, runtime: Arc) -> 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 diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index 2e79693314..061c251fa6 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -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; + 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; diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 32d5d6b786..c120bba8bf 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -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)); + } } } } diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 8410a1add3..19a05b0365 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -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" diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 6caa24a873..06a1a4fc4f 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -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)]