From 862b2122ee970fd3e124f0957f16fe9fd275fedc Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 12 May 2026 15:44:54 -0700 Subject: [PATCH] tools: remove is_mutating dispatch gating (#22382) ## Why Tool dispatch had two serialization mechanisms: - `supports_parallel_tool_calls` decides whether a tool participates in the shared parallel-execution lock. - `is_mutating` separately gated some calls inside dispatch. That second hook no longer carried its weight. The remaining parallel-support flag is already the per-tool concurrency policy, so keeping a second mutating gate made dispatch harder to follow and left behind extra session plumbing that only existed for that path. ## What changed - Removed `is_mutating` from tool handlers and deleted the `tool_call_gate` path that existed only to support it. - Simplified dispatch and routing to rely on the existing per-tool `supports_parallel_tool_calls` boolean. - Dropped the now-unused handler overrides and related session/test scaffolding. - Kept the router/parallel tests focused on the surviving per-tool behavior. - Removed the unused `codex-utils-readiness` dependency from `codex-core` as a follow-up fix for `cargo shear`. ## Testing - `cargo test -p codex-core parallel_support_does_not_match_namespaced_local_tool_names` - `cargo test -p codex-core mcp_parallel_support_uses_handler_data` - `cargo test -p codex-core tools_without_handlers_do_not_support_parallel` --- codex-rs/Cargo.lock | 1 - codex-rs/Cargo.toml | 1 - codex-rs/core/Cargo.toml | 1 - codex-rs/core/src/session/mod.rs | 1 - codex-rs/core/src/session/review.rs | 1 - codex-rs/core/src/session/tests.rs | 4 --- codex-rs/core/src/session/turn_context.rs | 3 -- .../core/src/tools/handlers/apply_patch.rs | 4 --- codex-rs/core/src/tools/handlers/dynamic.rs | 4 --- .../src/tools/handlers/extension_tools.rs | 7 +---- .../tools/handlers/shell/container_exec.rs | 11 ------- .../src/tools/handlers/shell/local_shell.rs | 13 ++------- .../src/tools/handlers/shell/shell_command.rs | 26 ++--------------- .../src/tools/handlers/shell/shell_handler.rs | 15 ++-------- .../handlers/unified_exec/exec_command.rs | 29 ++----------------- .../handlers/unified_exec/write_stdin.rs | 4 --- codex-rs/core/src/tools/registry.rs | 27 ++--------------- codex-rs/core/src/tools/router_tests.rs | 1 - 18 files changed, 11 insertions(+), 142 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 30d4bf364..afb3cbbed 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2516,7 +2516,6 @@ dependencies = [ "codex-utils-path", "codex-utils-plugins", "codex-utils-pty", - "codex-utils-readiness", "codex-utils-stream-parser", "codex-utils-string", "codex-utils-template", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 5a21e9899..3b9a0f0d7 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -228,7 +228,6 @@ codex-utils-output-truncation = { path = "utils/output-truncation" } codex-utils-path = { path = "utils/path-utils" } codex-utils-plugins = { path = "utils/plugins" } codex-utils-pty = { path = "utils/pty" } -codex-utils-readiness = { path = "utils/readiness" } codex-utils-rustls-provider = { path = "utils/rustls-provider" } codex-utils-sandbox-summary = { path = "utils/sandbox-summary" } codex-utils-sleep-inhibitor = { path = "utils/sleep-inhibitor" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 86e7c07ad..ec82a53d0 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -70,7 +70,6 @@ codex-utils-output-truncation = { workspace = true } codex-utils-path = { workspace = true } codex-utils-plugins = { workspace = true } codex-utils-pty = { workspace = true } -codex-utils-readiness = { workspace = true } codex-utils-string = { workspace = true } codex-utils-stream-parser = { workspace = true } codex-utils-template = { workspace = true } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ea0196a23..8739eb063 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -358,7 +358,6 @@ use codex_tools::ToolEnvironmentMode; use codex_tools::ToolsConfig; use codex_tools::ToolsConfigParams; use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_readiness::ReadinessFlag; #[cfg(test)] use codex_utils_stream_parser::ProposedPlanSegment; diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 7d4b1b736..b0113cbdb 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -146,7 +146,6 @@ pub(super) async fn spawn_review_thread( final_output_json_schema: None, codex_self_exe: parent_turn_context.codex_self_exe.clone(), codex_linux_sandbox_exe: parent_turn_context.codex_linux_sandbox_exe.clone(), - tool_call_gate: Arc::new(ReadinessFlag::new()), dynamic_tools: parent_turn_context.dynamic_tools.clone(), truncation_policy: model_info.truncation_policy.into(), turn_metadata_state, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 705139cfd..4b847295b 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -2837,10 +2837,6 @@ async fn turn_context_with_model_updates_model_fields() { updated.truncation_policy, expected_model_info.truncation_policy.into() ); - assert!(!Arc::ptr_eq( - &updated.tool_call_gate, - &turn_context.tool_call_gate - )); } #[test] diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 5a8885e0f..79d07ede0 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -89,7 +89,6 @@ pub struct TurnContext { pub(crate) final_output_json_schema: Option, pub(crate) codex_self_exe: Option, pub(crate) codex_linux_sandbox_exe: Option, - pub(crate) tool_call_gate: Arc, pub(crate) truncation_policy: TruncationPolicy, pub(crate) dynamic_tools: Vec, pub(crate) turn_metadata_state: Arc, @@ -272,7 +271,6 @@ impl TurnContext { final_output_json_schema: self.final_output_json_schema.clone(), codex_self_exe: self.codex_self_exe.clone(), codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.clone(), - tool_call_gate: Arc::new(ReadinessFlag::new()), truncation_policy, dynamic_tools: self.dynamic_tools.clone(), turn_metadata_state: self.turn_metadata_state.clone(), @@ -571,7 +569,6 @@ impl Session { final_output_json_schema: None, codex_self_exe: per_turn_config.codex_self_exe.clone(), codex_linux_sandbox_exe: per_turn_config.codex_linux_sandbox_exe.clone(), - tool_call_gate: Arc::new(ReadinessFlag::new()), truncation_policy: model_info.truncation_policy.into(), dynamic_tools: session_configuration.dynamic_tools.clone(), turn_metadata_state, diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 85b2c979b..da70b3985 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -311,10 +311,6 @@ impl ToolHandler for ApplyPatchHandler { matches!(payload, ToolPayload::Custom { .. }) } - async fn is_mutating(&self, _invocation: &ToolInvocation) -> bool { - true - } - fn create_diff_consumer(&self) -> Option> { Some(Box::::default()) } diff --git a/codex-rs/core/src/tools/handlers/dynamic.rs b/codex-rs/core/src/tools/handlers/dynamic.rs index 59450a837..7cc9e61e3 100644 --- a/codex-rs/core/src/tools/handlers/dynamic.rs +++ b/codex-rs/core/src/tools/handlers/dynamic.rs @@ -35,10 +35,6 @@ impl ToolHandler for DynamicToolHandler { self.tool_name.clone() } - async fn is_mutating(&self, _invocation: &ToolInvocation) -> bool { - true - } - async fn handle(&self, invocation: ToolInvocation) -> Result { let ToolInvocation { session, diff --git a/codex-rs/core/src/tools/handlers/extension_tools.rs b/codex-rs/core/src/tools/handlers/extension_tools.rs index f76708478..da0a93515 100644 --- a/codex-rs/core/src/tools/handlers/extension_tools.rs +++ b/codex-rs/core/src/tools/handlers/extension_tools.rs @@ -83,10 +83,6 @@ impl ToolHandler for BundledToolHandler { self.arguments_from_payload(payload).is_some() } - async fn is_mutating(&self, _invocation: &ToolInvocation) -> bool { - true - } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { let arguments = self.arguments_from_payload(&invocation.payload)?; Some(PreToolUsePayload { @@ -189,7 +185,7 @@ mod tests { } #[tokio::test] - async fn exposes_generic_hook_payloads_and_is_conservatively_mutating() { + async fn exposes_generic_hook_payloads() { let bundle = codex_tool_api::ToolBundle::new( codex_tool_api::FunctionToolSpec { name: "extension_echo".to_string(), @@ -225,7 +221,6 @@ mod tests { value: json!({ "ok": true }), }; - assert!(ToolHandler::is_mutating(&handler, &invocation).await); assert_eq!( ToolHandler::pre_tool_use_payload(&handler, &invocation), Some(PreToolUsePayload { diff --git a/codex-rs/core/src/tools/handlers/shell/container_exec.rs b/codex-rs/core/src/tools/handlers/shell/container_exec.rs index cb09a5150..515b8ca7d 100644 --- a/codex-rs/core/src/tools/handlers/shell/container_exec.rs +++ b/codex-rs/core/src/tools/handlers/shell/container_exec.rs @@ -1,5 +1,4 @@ use codex_protocol::models::ShellToolCallParams; -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ToolName; use crate::function_tool::FunctionCallError; @@ -33,16 +32,6 @@ impl ToolHandler for ContainerExecHandler { matches!(payload, ToolPayload::Function { .. }) } - async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let ToolPayload::Function { arguments } = &invocation.payload else { - return true; - }; - - serde_json::from_str::(arguments) - .map(|params| !is_known_safe_command(¶ms.command)) - .unwrap_or(true) - } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { shell_function_pre_tool_use_payload(invocation) } diff --git a/codex-rs/core/src/tools/handlers/shell/local_shell.rs b/codex-rs/core/src/tools/handlers/shell/local_shell.rs index 6265be428..9312b6913 100644 --- a/codex-rs/core/src/tools/handlers/shell/local_shell.rs +++ b/codex-rs/core/src/tools/handlers/shell/local_shell.rs @@ -1,4 +1,3 @@ -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ToolName; use crate::function_tool::FunctionCallError; @@ -42,20 +41,12 @@ impl ToolHandler for LocalShellHandler { self.include_spec.then(create_local_shell_tool) } - fn supports_parallel_tool_calls(&self) -> bool { - self.include_spec - } - fn matches_kind(&self, payload: &ToolPayload) -> bool { matches!(payload, ToolPayload::LocalShell { .. }) } - async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let ToolPayload::LocalShell { params } = &invocation.payload else { - return true; - }; - - !is_known_safe_command(¶ms.command) + fn supports_parallel_tool_calls(&self) -> bool { + self.include_spec } fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index f5ecd8066..aa88da95d 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -1,6 +1,5 @@ use codex_protocol::ThreadId; use codex_protocol::models::ShellCommandToolCallParams; -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ShellCommandBackendConfig; use codex_tools::ToolName; @@ -141,33 +140,12 @@ impl ToolHandler for ShellCommandHandler { }) } - fn supports_parallel_tool_calls(&self) -> bool { - self.options.is_some() - } - fn matches_kind(&self, payload: &ToolPayload) -> bool { matches!(payload, ToolPayload::Function { .. }) } - async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let ToolPayload::Function { arguments } = &invocation.payload else { - return true; - }; - - serde_json::from_str::(arguments) - .map(|params| { - let use_login_shell = match Self::resolve_use_login_shell( - params.login, - invocation.turn.tools_config.allow_login_shell, - ) { - Ok(use_login_shell) => use_login_shell, - Err(_) => return true, - }; - let shell = invocation.session.user_shell(); - let command = Self::base_command(shell.as_ref(), ¶ms.command, use_login_shell); - !is_known_safe_command(&command) - }) - .unwrap_or(true) + fn supports_parallel_tool_calls(&self) -> bool { + self.options.is_some() } fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { diff --git a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs index 39422734f..fdcb86230 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_handler.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_handler.rs @@ -1,6 +1,5 @@ use codex_protocol::ThreadId; use codex_protocol::models::ShellToolCallParams; -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ToolName; use crate::exec::ExecCapturePolicy; @@ -74,22 +73,12 @@ impl ToolHandler for ShellHandler { self.options.map(create_shell_tool) } - fn supports_parallel_tool_calls(&self) -> bool { - self.options.is_some() - } - fn matches_kind(&self, payload: &ToolPayload) -> bool { matches!(payload, ToolPayload::Function { .. }) } - async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let ToolPayload::Function { arguments } = &invocation.payload else { - return true; - }; - - serde_json::from_str::(arguments) - .map(|params| !is_known_safe_command(¶ms.command)) - .unwrap_or(true) + fn supports_parallel_tool_calls(&self) -> bool { + self.options.is_some() } fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 131181326..3a69b3c63 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -26,7 +26,6 @@ use crate::unified_exec::generate_chunk_id; use codex_features::Feature; use codex_otel::SessionTelemetry; use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC; -use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_utils_output_truncation::approx_token_count; @@ -85,36 +84,12 @@ impl ToolHandler for ExecCommandHandler { )) } - fn supports_parallel_tool_calls(&self) -> bool { - true - } - fn matches_kind(&self, payload: &ToolPayload) -> bool { matches!(payload, ToolPayload::Function { .. }) } - async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let ToolPayload::Function { arguments } = &invocation.payload else { - tracing::error!( - "This should never happen, invocation payload is wrong: {:?}", - invocation.payload - ); - return true; - }; - - let Ok(params) = parse_arguments::(arguments) else { - return true; - }; - let command = match get_command( - ¶ms, - invocation.session.user_shell(), - &invocation.turn.tools_config.unified_exec_shell_mode, - invocation.turn.tools_config.allow_login_shell, - ) { - Ok(command) => command, - Err(_) => return true, - }; - !is_known_safe_command(&command) + fn supports_parallel_tool_calls(&self) -> bool { + true } fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { diff --git a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs index 3250a9d0a..455d7b641 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs @@ -45,10 +45,6 @@ impl ToolHandler for WriteStdinHandler { matches!(payload, ToolPayload::Function { .. }) } - async fn is_mutating(&self, _invocation: &ToolInvocation) -> bool { - true - } - fn post_tool_use_payload( &self, invocation: &ToolInvocation, diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 235cb6fef..01b3caa34 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -27,7 +27,6 @@ use codex_protocol::protocol::EventMsg; use codex_tool_api::ToolBundle as ExtensionToolBundle; use codex_tools::ToolName; use codex_tools::ToolSpec; -use codex_utils_readiness::Readiness; use futures::future::BoxFuture; use serde_json::Value; use tracing::warn; @@ -62,17 +61,6 @@ pub trait ToolHandler: Send + Sync { async { Vec::new() } } - /// Returns `true` if the [ToolInvocation] *might* mutate the environment of the - /// user (through file system, OS operations, ...). - /// This function must remains defensive and return `true` if a doubt exist on the - /// exact effect of a ToolInvocation. - fn is_mutating( - &self, - _invocation: &ToolInvocation, - ) -> impl std::future::Future + Send { - async { false } - } - fn post_tool_use_payload( &self, _invocation: &ToolInvocation, @@ -185,8 +173,6 @@ trait AnyToolHandler: Send + Sync { fn matches_kind(&self, payload: &ToolPayload) -> bool; - fn is_mutating<'a>(&'a self, invocation: &'a ToolInvocation) -> BoxFuture<'a, bool>; - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; fn with_updated_hook_input( @@ -219,10 +205,6 @@ where ToolHandler::matches_kind(self, payload) } - fn is_mutating<'a>(&'a self, invocation: &'a ToolInvocation) -> BoxFuture<'a, bool> { - Box::pin(ToolHandler::is_mutating(self, invocation)) - } - fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { ToolHandler::pre_tool_use_payload(self, invocation) } @@ -305,7 +287,8 @@ impl ToolRegistry { } pub(crate) fn supports_parallel_tool_calls(&self, name: &ToolName) -> Option { - Some(self.handler(name)?.supports_parallel_tool_calls()) + let handler = self.handler(name)?; + Some(handler.supports_parallel_tool_calls()) } #[expect( @@ -424,7 +407,6 @@ impl ToolRegistry { } } - let is_mutating = handler.is_mutating(&invocation).await; let response_cell = tokio::sync::Mutex::new(None); let invocation_for_tool = invocation.clone(); let log_payload = invocation.payload.log_payload(); @@ -440,11 +422,6 @@ impl ToolRegistry { let handler = handler.clone(); let response_cell = &response_cell; async move { - if is_mutating { - tracing::trace!("waiting for tool gate"); - invocation_for_tool.turn.tool_call_gate.wait_ready().await; - tracing::trace!("tool gate released"); - } match handler.handle_any(invocation_for_tool).await { Ok(result) => { let preview = result.result.log_preview(); diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index b3fe087fc..e78bd9402 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -361,7 +361,6 @@ async fn extension_tool_bundles_are_model_visible_and_dispatchable() -> anyhow:: call_id: "call-extension".to_string(), })? .expect("function_call should produce a tool call"); - let result = router .dispatch_tool_call_with_code_mode_result( Arc::new(session),