agent/codex
1
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

tools: remove is_mutating dispatch gating (#22382)

Browse Source 
## 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`
This commit is contained in:
pakrym-oai
2026-05-12 15:44:54 -07:00
committed by GitHub
Unverified
parent 5e3ee5eddf
commit 862b2122ee
18 changed files with 11 additions and 142 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/Cargo.lock
Generated
-1
View File
@@ -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",
codex-rs/Cargo.toml
-1
View File
@@ -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" }
codex-rs/core/Cargo.toml
-1
View File
@@ -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 }
codex-rs/core/src/session/mod.rs
-1
View File
@@ -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;
codex-rs/core/src/session/review.rs
-1
View File
@@ -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,
codex-rs/core/src/session/tests.rs
-4
View File
@@ -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]
codex-rs/core/src/session/turn_context.rs
-3
View File
@@ -89,7 +89,6 @@ pub struct TurnContext {
pub(crate) final_output_json_schema: Option<Value>,
pub(crate) codex_self_exe: Option<PathBuf>,
pub(crate) codex_linux_sandbox_exe: Option<PathBuf>,
pub(crate) tool_call_gate: Arc<ReadinessFlag>,
pub(crate) truncation_policy: TruncationPolicy,
pub(crate) dynamic_tools: Vec<DynamicToolSpec>,
pub(crate) turn_metadata_state: Arc<TurnMetadataState>,
@@ -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,
codex-rs/core/src/tools/handlers/apply_patch.rs
-4
View File
@@ -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<Box<dyn ToolArgumentDiffConsumer>> {
Some(Box::<ApplyPatchArgumentDiffConsumer>::default())
}
codex-rs/core/src/tools/handlers/dynamic.rs
-4
View File
@@ -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<Self::Output, FunctionCallError> {
let ToolInvocation {
session,
codex-rs/core/src/tools/handlers/extension_tools.rs
+1 -6
View File
@@ -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<PreToolUsePayload> {
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 {
codex-rs/core/src/tools/handlers/shell/container_exec.rs
-11
View File
@@ -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::<ShellToolCallParams>(arguments)
.map(|params| !is_known_safe_command(&params.command))
.unwrap_or(true)
}
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload> {
shell_function_pre_tool_use_payload(invocation)
}
codex-rs/core/src/tools/handlers/shell/local_shell.rs
+2 -11
View File
@@ -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(&params.command)
fn supports_parallel_tool_calls(&self) -> bool {
self.include_spec
}
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload> {
codex-rs/core/src/tools/handlers/shell/shell_command.rs
+2 -24
View File
@@ -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::<ShellCommandToolCallParams>(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(), &params.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<PreToolUsePayload> {
codex-rs/core/src/tools/handlers/shell/shell_handler.rs
+2 -13
View File
@@ -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::<ShellToolCallParams>(arguments)
.map(|params| !is_known_safe_command(&params.command))
.unwrap_or(true)
fn supports_parallel_tool_calls(&self) -> bool {
self.options.is_some()
}
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload> {
codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs
+2 -27
View File
@@ -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::<ExecCommandArgs>(arguments) else {
return true;
};
let command = match get_command(
&params,
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<PreToolUsePayload> {
codex-rs/core/src/tools/handlers/unified_exec/write_stdin.rs
-4
View File
@@ -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,
codex-rs/core/src/tools/registry.rs
+2 -25
View File
@@ -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<Output = bool> + 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<PreToolUsePayload>;
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<PreToolUsePayload> {
ToolHandler::pre_tool_use_payload(self, invocation)
}
@@ -305,7 +287,8 @@ impl ToolRegistry {
}
pub(crate) fn supports_parallel_tool_calls(&self, name: &ToolName) -> Option<bool> {
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();
codex-rs/core/src/tools/router_tests.rs
-1
View File
@@ -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),