From c9e46ed639db547e84f543e9230a2391df34ecbc Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 11 May 2026 22:26:33 -0700 Subject: [PATCH] [codex] Make handlers own parallel tool support (#22254) ## Why `ToolRouter::tool_supports_parallel()` was still consulting configured specs when a handler lookup missed, even though parallel schedulability is really a property of the executable handler. Keeping that metadata on `ConfiguredToolSpec` duplicated state between the model-visible spec layer and the runtime handler layer. This change makes handlers the sole source of truth for parallel tool support and removes the extra spec wrapper that only existed to carry duplicated metadata. ## What changed - removed `ConfiguredToolSpec` and store plain `ToolSpec` values in the registry/router builder path - changed `ToolRouter::tool_supports_parallel()` to consult only the handler registry and fall back to `false` - simplified spec collection and test helpers to operate directly on `ToolSpec` - updated router/spec tests to cover handler-owned parallel behavior and the no-handler fallback ## Validation - `cargo test -p codex-tools` - `cargo test -p codex-core mcp_parallel_support_uses_handler_data` - `cargo test -p codex-core deferred_responses_api_tool_serializes_with_defer_loading` - `cargo test -p codex-core tools_without_handlers_do_not_support_parallel` - `cargo test -p codex-core request_plugin_install_can_be_registered_without_search_tool` ## Docs No documentation updates needed. --- codex-rs/core/src/tools/registry.rs | 17 ++-- codex-rs/core/src/tools/registry_tests.rs | 2 +- codex-rs/core/src/tools/router.rs | 44 ++------- codex-rs/core/src/tools/router_tests.rs | 34 ++++++- codex-rs/core/src/tools/spec_plan.rs | 30 ++---- codex-rs/core/src/tools/spec_plan_tests.rs | 106 +++++++++------------ codex-rs/core/src/tools/spec_tests.rs | 23 +++-- codex-rs/tools/src/lib.rs | 1 - codex-rs/tools/src/tool_spec.rs | 19 ---- codex-rs/tools/src/tool_spec_tests.rs | 24 ----- 10 files changed, 115 insertions(+), 185 deletions(-) diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index 25f2d11e9a..235cb6fef3 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -25,7 +25,6 @@ use crate::util::error_or_panic; use codex_protocol::models::ResponseInputItem; use codex_protocol::protocol::EventMsg; use codex_tool_api::ToolBundle as ExtensionToolBundle; -use codex_tools::ConfiguredToolSpec; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_utils_readiness::Readiness; @@ -554,7 +553,7 @@ impl ToolRegistry { pub struct ToolRegistryBuilder { handlers: HashMap>, - specs: Vec, + specs: Vec, code_mode_enabled: bool, } @@ -567,14 +566,13 @@ impl ToolRegistryBuilder { } } - pub(crate) fn push_spec(&mut self, spec: ToolSpec, supports_parallel_tool_calls: bool) { + pub(crate) fn push_spec(&mut self, spec: ToolSpec) { let spec = if self.code_mode_enabled { codex_tools::augment_tool_spec_for_code_mode(spec) } else { spec }; - self.specs - .push(ConfiguredToolSpec::new(spec, supports_parallel_tool_calls)); + self.specs.push(spec); } pub fn register_handler(&mut self, handler: Arc) @@ -588,8 +586,7 @@ impl ToolRegistryBuilder { } if let Some(spec) = handler.spec() { - let supports_parallel_tool_calls = handler.supports_parallel_tool_calls(); - self.push_spec(spec, supports_parallel_tool_calls); + self.push_spec(spec); } let handler: Arc = handler; @@ -612,17 +609,17 @@ impl ToolRegistryBuilder { return; } }; - self.push_spec(spec.clone(), /*supports_parallel_tool_calls*/ false); + self.push_spec(spec.clone()); let handler: Arc = Arc::new(BundledToolHandler::new(bundle, spec)); self.handlers.insert(tool_name, handler); } - pub(crate) fn specs(&self) -> &[ConfiguredToolSpec] { + pub(crate) fn specs(&self) -> &[ToolSpec] { &self.specs } - pub fn build(self) -> (Vec, ToolRegistry) { + pub fn build(self) -> (Vec, ToolRegistry) { let registry = ToolRegistry::new(self.handlers); (self.specs, registry) } diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index be5223a117..d6acb80fb4 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -71,7 +71,7 @@ fn register_handler_adds_handler_and_augments_specs_for_code_mode() { assert_eq!(specs.len(), 1); assert_eq!( - specs[0].spec, + specs[0], codex_tools::augment_tool_spec_for_code_mode(create_get_goal_tool()) ); assert!(registry.has_handler(&codex_tools::ToolName::plain(GET_GOAL_TOOL_NAME))); diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 51fd40a268..ed3f1a720b 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -16,7 +16,6 @@ use codex_protocol::models::ResponseItem; use codex_protocol::models::SearchToolCallParams; use codex_protocol::models::ShellToolCallParams; use codex_tool_api::ToolBundle as ExtensionToolBundle; -use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverableTool; use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; @@ -38,7 +37,7 @@ pub struct ToolCall { pub struct ToolRouter { registry: ToolRegistry, - specs: Vec, + specs: Vec, model_visible_specs: Vec, } @@ -78,17 +77,14 @@ impl ToolRouter { .collect::>(); let model_visible_specs = specs .iter() - .filter_map(|configured_tool| { + .filter_map(|spec| { if config.code_mode_only_enabled - && codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) + && codex_code_mode::is_code_mode_nested_tool(spec.name()) { return None; } - filter_deferred_dynamic_tool_spec( - configured_tool.spec.clone(), - &deferred_dynamic_tools, - ) + filter_deferred_dynamic_tool_spec(spec.clone(), &deferred_dynamic_tools) }) .collect(); @@ -100,10 +96,7 @@ impl ToolRouter { } pub fn specs(&self) -> Vec { - self.specs - .iter() - .map(|config| config.spec.clone()) - .collect() + self.specs.clone() } pub fn model_visible_specs(&self) -> Vec { @@ -111,16 +104,16 @@ impl ToolRouter { } pub fn find_spec(&self, tool_name: &ToolName) -> Option { - self.specs.iter().find_map(|config| match &config.spec { + self.specs.iter().find_map(|spec| match spec { ToolSpec::Function(tool) if tool_name.namespace.is_none() && tool.name == tool_name.name => { - Some(config.spec.clone()) + Some(spec.clone()) } ToolSpec::Freeform(tool) if tool_name.namespace.is_none() && tool.name == tool_name.name => { - Some(config.spec.clone()) + Some(spec.clone()) } ToolSpec::Namespace(namespace) => namespace.tools.iter().find_map(|tool| match tool { ResponsesApiNamespaceTool::Function(tool) @@ -142,29 +135,10 @@ impl ToolRouter { self.registry.create_diff_consumer(tool_name) } - fn configured_tool_supports_parallel(&self, tool_name: &ToolName) -> bool { - if tool_name.namespace.is_some() { - return false; - } - - self.specs - .iter() - .filter(|config| config.supports_parallel_tool_calls) - .any(|config| match &config.spec { - ToolSpec::Function(tool) => tool.name == tool_name.name.as_str(), - ToolSpec::Freeform(tool) => tool.name == tool_name.name.as_str(), - ToolSpec::Namespace(_) - | ToolSpec::ToolSearch { .. } - | ToolSpec::LocalShell {} - | ToolSpec::ImageGeneration { .. } - | ToolSpec::WebSearch { .. } => false, - }) - } - pub fn tool_supports_parallel(&self, call: &ToolCall) -> bool { self.registry .supports_parallel_tool_calls(&call.tool_name) - .unwrap_or_else(|| self.configured_tool_supports_parallel(&call.tool_name)) + .unwrap_or(false) } #[instrument(level = "trace", skip_all, err)] diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index dc06f7ed02..b3fe087fcc 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -157,7 +157,7 @@ async fn build_tool_call_uses_namespace_for_registry_name() -> anyhow::Result<() } #[tokio::test] -async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> { +async fn mcp_parallel_support_uses_handler_data() -> anyhow::Result<()> { let (_, turn) = make_session_and_context().await; let router = ToolRouter::from_config( &turn.tools_config, @@ -184,14 +184,14 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> }, ); - let deferred_call = ToolCall { + let call = ToolCall { tool_name: ToolName::namespaced("mcp__echo__", "query_with_delay"), - call_id: "call-deferred".to_string(), + call_id: "call-handler".to_string(), payload: ToolPayload::Function { arguments: "{}".to_string(), }, }; - assert!(router.tool_supports_parallel(&deferred_call)); + assert!(router.tool_supports_parallel(&call)); let different_server_call = ToolCall { tool_name: ToolName::namespaced("mcp__hello_echo__", "query_with_delay"), @@ -205,6 +205,32 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> Ok(()) } +#[tokio::test] +async fn tools_without_handlers_do_not_support_parallel() -> anyhow::Result<()> { + let (_, turn) = make_session_and_context().await; + let router = ToolRouter::from_config( + &turn.tools_config, + ToolRouterParams { + deferred_mcp_tools: None, + mcp_tools: None, + unavailable_called_tools: Vec::new(), + discoverable_tools: None, + extension_tool_bundles: Vec::new(), + dynamic_tools: turn.dynamic_tools.as_slice(), + }, + ); + + assert!(!router.tool_supports_parallel(&ToolCall { + tool_name: ToolName::plain("web_search"), + call_id: "call-web-search".to_string(), + payload: ToolPayload::Function { + arguments: "{}".to_string(), + }, + })); + + Ok(()) +} + #[tokio::test] async fn model_visible_specs_filter_deferred_dynamic_tools() -> anyhow::Result<()> { let (_, turn) = 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 b07a572ef7..69456a36ef 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -97,12 +97,8 @@ pub fn build_tool_registry_builder( ..params }, ); - let mut enabled_tools = collect_code_mode_exec_prompt_tool_definitions( - nested_builder - .specs() - .iter() - .map(|configured_tool| &configured_tool.spec), - ); + let mut enabled_tools = + collect_code_mode_exec_prompt_tool_definitions(nested_builder.specs().iter()); enabled_tools .sort_by(|left, right| compare_code_mode_tools(left, right, &namespace_descriptions)); builder.register_handler(Arc::new(CodeModeExecuteHandler::new( @@ -277,14 +273,11 @@ pub fn build_tool_registry_builder( web_search_config: config.web_search_config.as_ref(), web_search_tool_type: config.web_search_tool_type, }) { - builder.push_spec(web_search_tool, /*supports_parallel_tool_calls*/ false); + builder.push_spec(web_search_tool); } if config.image_gen_tool { - builder.push_spec( - create_image_generation_tool("png"), - /*supports_parallel_tool_calls*/ false, - ); + builder.push_spec(create_image_generation_tool("png")); } if config.environment_mode.has_environment() { @@ -391,14 +384,11 @@ pub fn build_tool_registry_builder( } if config.namespace_tools && !tools.is_empty() { - builder.push_spec( - ToolSpec::Namespace(ResponsesApiNamespace { - name: namespace, - description, - tools, - }), - /*supports_parallel_tool_calls*/ false, - ); + builder.push_spec(ToolSpec::Namespace(ResponsesApiNamespace { + name: namespace, + description, + tools, + })); } } } @@ -422,7 +412,7 @@ pub fn build_tool_registry_builder( for spec in coalesce_loadable_tool_specs(dynamic_tool_specs) { let spec = spec.into(); if config.namespace_tools || !matches!(spec, ToolSpec::Namespace(_)) { - builder.push_spec(spec, /*supports_parallel_tool_calls*/ false); + builder.push_spec(spec); } } diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 8079f91da1..a4132391a2 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -47,7 +47,6 @@ use codex_tool_api::ToolBundle as ExtensionToolBundle; use codex_tool_api::ToolExecutor; use codex_tool_api::ToolFuture; use codex_tools::AdditionalProperties; -use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverablePluginInfo; use codex_tools::DiscoverableTool; use codex_tools::FreeformTool; @@ -132,7 +131,7 @@ fn extension_tools_do_not_replace_builtin_tools() { ); assert_eq!( - find_tool(&tools, "update_plan").spec, + find_tool(&tools, "update_plan").clone(), create_update_plan_tool() ); assert_eq!( @@ -171,7 +170,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() { let mut duplicate_names = Vec::new(); for tool in &tools { let name = tool.name().to_string(); - if actual.insert(name.clone(), tool.spec.clone()).is_some() { + if actual.insert(name.clone(), tool.clone()).is_some() { duplicate_names.push(name); } } @@ -365,7 +364,7 @@ fn test_build_specs_collab_tools_enabled() { assert_lacks_tool_name(&tools, "list_agents"); let spawn_agent = find_tool(&tools, "spawn_agent"); - let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &spawn_agent.spec else { + let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = spawn_agent else { panic!("spawn_agent should be a function tool"); }; let (properties, _) = expect_object_schema(parameters); @@ -459,7 +458,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { parameters, output_schema, .. - }) = &spawn_agent.spec + }) = spawn_agent else { panic!("spawn_agent should be a function tool"); }; @@ -483,7 +482,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { parameters, output_schema, .. - }) = &send_message.spec + }) = send_message else { panic!("send_message should be a function tool"); }; @@ -503,7 +502,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { parameters, output_schema, .. - }) = &followup_task.spec + }) = followup_task else { panic!("followup_task should be a function tool"); }; @@ -522,7 +521,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { parameters, output_schema, .. - }) = &wait_agent.spec + }) = wait_agent else { panic!("wait_agent should be a function tool"); }; @@ -543,7 +542,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { parameters, output_schema, .. - }) = &list_agents.spec + }) = list_agents else { panic!("list_agents should be a function tool"); }; @@ -660,7 +659,7 @@ fn view_image_tool_omits_detail_without_original_detail_support() { &[], ); let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME); - let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &view_image.spec else { + let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = view_image else { panic!("view_image should be a function tool"); }; let (properties, _) = expect_object_schema(parameters); @@ -690,7 +689,7 @@ fn view_image_tool_includes_detail_with_original_detail_support() { &[], ); let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME); - let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &view_image.spec else { + let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = view_image else { panic!("view_image should be a function tool"); }; let (properties, _) = expect_object_schema(parameters); @@ -841,7 +840,7 @@ fn request_user_input_description_reflects_default_mode_feature_flag() { ); let request_user_input_tool = find_tool(&tools, REQUEST_USER_INPUT_TOOL_NAME); assert_eq!( - request_user_input_tool.spec, + request_user_input_tool.clone(), request_user_input_tool_spec(&request_user_input_available_modes(&features)) ); @@ -864,7 +863,7 @@ fn request_user_input_description_reflects_default_mode_feature_flag() { ); let request_user_input_tool = find_tool(&tools, REQUEST_USER_INPUT_TOOL_NAME); assert_eq!( - request_user_input_tool.spec, + request_user_input_tool.clone(), request_user_input_tool_spec(&request_user_input_available_modes(&features)) ); } @@ -912,7 +911,7 @@ fn request_permissions_requires_feature_flag() { ); let request_permissions_tool = find_tool(&tools, "request_permissions"); assert_eq!( - request_permissions_tool.spec, + request_permissions_tool.clone(), create_request_permissions_tool(request_permissions_tool_description()) ); } @@ -973,7 +972,7 @@ fn image_generation_tools_require_feature_and_supported_model() { assert!( !default_tools .iter() - .any(|tool| tool.spec.name() == "image_generation"), + .any(|tool| tool.name() == "image_generation"), "image_generation should be disabled when the feature is disabled" ); @@ -996,7 +995,7 @@ fn image_generation_tools_require_feature_and_supported_model() { assert_contains_tool_names(&supported_tools, &["image_generation"]); let image_generation_tool = find_tool(&supported_tools, "image_generation"); assert_eq!( - serde_json::to_value(&image_generation_tool.spec).expect("serialize image tool"), + serde_json::to_value(image_generation_tool).expect("serialize image tool"), serde_json::json!({ "type": "image_generation", "output_format": "png" @@ -1020,9 +1019,7 @@ fn image_generation_tools_require_feature_and_supported_model() { &[], ); assert!( - !tools - .iter() - .any(|tool| tool.spec.name() == "image_generation"), + !tools.iter().any(|tool| tool.name() == "image_generation"), "image_generation should be disabled for unsupported models" ); } @@ -1052,7 +1049,7 @@ fn web_search_mode_cached_sets_external_web_access_false() { let tool = find_tool(&tools, "web_search"); assert_eq!( - tool.spec, + tool.clone(), ToolSpec::WebSearch { external_web_access: Some(false), filters: None, @@ -1088,7 +1085,7 @@ fn web_search_mode_live_sets_external_web_access_true() { let tool = find_tool(&tools, "web_search"); assert_eq!( - tool.spec, + tool.clone(), ToolSpec::WebSearch { external_web_access: Some(true), filters: None, @@ -1138,7 +1135,7 @@ fn web_search_config_is_forwarded_to_tool_spec() { let tool = find_tool(&tools, "web_search"); assert_eq!( - tool.spec, + tool.clone(), ToolSpec::WebSearch { external_web_access: Some(true), filters: web_search_config @@ -1179,7 +1176,7 @@ fn web_search_tool_type_text_and_image_sets_search_content_types() { let tool = find_tool(&tools, "web_search"); assert_eq!( - tool.spec, + tool.clone(), ToolSpec::WebSearch { external_web_access: Some(true), filters: None, @@ -1214,7 +1211,7 @@ fn mcp_resource_tools_are_hidden_without_mcp_servers() { assert!( !tools.iter().any(|tool| matches!( - tool.spec.name(), + tool.name(), "list_mcp_resources" | "list_mcp_resource_templates" | "read_mcp_resource" )), "MCP resource tools should be omitted when no MCP servers are configured" @@ -1277,8 +1274,7 @@ fn test_parallel_support_flags() { &[], ); - assert!(find_tool(&tools, "exec_command").supports_parallel_tool_calls); - assert!(!find_tool(&tools, "write_stdin").supports_parallel_tool_calls); + assert_contains_tool_names(&tools, &["exec_command", "write_stdin"]); } #[test] @@ -1505,7 +1501,7 @@ fn test_build_specs_mcp_namespace_description_falls_back_when_missing() { ); let namespace_tool = find_tool(&tools, "test_server/"); - let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + let ToolSpec::Namespace(namespace) = namespace_tool else { panic!("expected namespace tool"); }; assert_eq!( @@ -1631,7 +1627,7 @@ fn search_tool_description_lists_each_mcp_source_once() { ); let search_tool = find_tool(&tools, TOOL_SEARCH_TOOL_NAME); - let ToolSpec::ToolSearch { description, .. } = &search_tool.spec else { + let ToolSpec::ToolSearch { description, .. } = search_tool else { panic!("expected tool_search tool"); }; let description = description.as_str(); @@ -1810,7 +1806,7 @@ fn search_tool_registers_for_deferred_dynamic_tools() { ); let search_tool = find_tool(&tools, TOOL_SEARCH_TOOL_NAME); - let ToolSpec::ToolSearch { description, .. } = &search_tool.spec else { + let ToolSpec::ToolSearch { description, .. } = search_tool else { panic!("expected tool_search tool"); }; assert!(description.contains("- Dynamic tools: Tools provided by the current Codex thread.")); @@ -1959,11 +1955,9 @@ fn request_plugin_install_can_be_registered_without_search_tool() { assert_contains_tool_names(&tools, &[REQUEST_PLUGIN_INSTALL_TOOL_NAME]); let request_plugin_install = find_tool(&tools, REQUEST_PLUGIN_INSTALL_TOOL_NAME); - assert!(request_plugin_install.supports_parallel_tool_calls); assert_lacks_tool_name(&tools, TOOL_SEARCH_TOOL_NAME); - let ToolSpec::Function(ResponsesApiTool { description, .. }) = &request_plugin_install.spec - else { + let ToolSpec::Function(ResponsesApiTool { description, .. }) = request_plugin_install else { panic!("expected function tool"); }; assert!(description.contains( @@ -2030,7 +2024,7 @@ fn request_plugin_install_description_lists_discoverable_tools() { description, parameters, .. - }) = &request_plugin_install.spec + }) = request_plugin_install else { panic!("expected function tool"); }; @@ -2247,7 +2241,7 @@ fn code_mode_augments_builtin_tool_descriptions_with_typed_sample() { &[], ); let ToolSpec::Function(ResponsesApiTool { description, .. }) = - &find_tool(&tools, VIEW_IMAGE_TOOL_NAME).spec + find_tool(&tools, VIEW_IMAGE_TOOL_NAME) else { panic!("expected function tool"); }; @@ -2282,8 +2276,7 @@ fn code_mode_only_exec_description_includes_full_nested_tool_details() { /*deferred_mcp_tools*/ None, &[], ); - let ToolSpec::Freeform(FreeformTool { description, .. }) = &find_tool(&tools, "exec").spec - else { + let ToolSpec::Freeform(FreeformTool { description, .. }) = find_tool(&tools, "exec") else { panic!("expected freeform tool"); }; @@ -2326,8 +2319,7 @@ fn code_mode_only_exec_description_includes_extension_tool_details() { &extension_tool_bundles, &[], ); - let ToolSpec::Freeform(FreeformTool { description, .. }) = &find_tool(&tools, "exec").spec - else { + let ToolSpec::Freeform(FreeformTool { description, .. }) = find_tool(&tools, "exec") else { panic!("expected freeform tool"); }; @@ -2358,8 +2350,7 @@ fn code_mode_exec_description_omits_nested_tool_details_when_not_code_mode_only( /*deferred_mcp_tools*/ None, &[], ); - let ToolSpec::Freeform(FreeformTool { description, .. }) = &find_tool(&tools, "exec").spec - else { + let ToolSpec::Freeform(FreeformTool { description, .. }) = find_tool(&tools, "exec") else { panic!("expected freeform tool"); }; @@ -2417,7 +2408,7 @@ fn build_specs( mcp_tools: Option>, deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], -) -> (Vec, ToolRegistry) { +) -> (Vec, ToolRegistry) { build_specs_with_discoverable_tools( config, mcp_tools, @@ -2435,7 +2426,7 @@ fn build_specs_with_discoverable_tools( discoverable_tools: Option>, extension_tool_bundles: &[codex_tool_api::ToolBundle], dynamic_tools: &[DynamicToolSpec], -) -> (Vec, ToolRegistry) { +) -> (Vec, ToolRegistry) { build_specs_with_optional_tool_namespaces( config, mcp_tools, @@ -2455,7 +2446,7 @@ fn build_specs_with_optional_tool_namespaces( discoverable_tools: Option>, extension_tool_bundles: &[codex_tool_api::ToolBundle], dynamic_tools: &[DynamicToolSpec], -) -> (Vec, ToolRegistry) { +) -> (Vec, ToolRegistry) { let mcp_tool_inputs = mcp_tools.as_ref().map(|mcp_tools| { mcp_tools .iter() @@ -2636,12 +2627,12 @@ fn deferred_mcp_tool( } } -fn assert_contains_tool_names(tools: &[ConfiguredToolSpec], expected_subset: &[&str]) { +fn assert_contains_tool_names(tools: &[ToolSpec], expected_subset: &[&str]) { use std::collections::HashSet; let mut names = HashSet::new(); let mut duplicates = Vec::new(); - for name in tools.iter().map(ConfiguredToolSpec::name) { + for name in tools.iter().map(ToolSpec::name) { if !names.insert(name) { duplicates.push(name); } @@ -2658,11 +2649,8 @@ fn assert_contains_tool_names(tools: &[ConfiguredToolSpec], expected_subset: &[& } } -fn assert_lacks_tool_name(tools: &[ConfiguredToolSpec], expected_absent: &str) { - let names = tools - .iter() - .map(ConfiguredToolSpec::name) - .collect::>(); +fn assert_lacks_tool_name(tools: &[ToolSpec], expected_absent: &str) { + let names = tools.iter().map(ToolSpec::name).collect::>(); assert!( !names.contains(&expected_absent), "expected tool {expected_absent} to be absent; had: {names:?}" @@ -2692,7 +2680,7 @@ fn wait_agent_timeout_options() -> WaitAgentTimeoutOptions { } } -fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a ConfiguredToolSpec { +fn find_tool<'a>(tools: &'a [ToolSpec], expected_name: &str) -> &'a ToolSpec { tools .iter() .find(|tool| tool.name() == expected_name) @@ -2700,12 +2688,12 @@ fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a Co } fn assert_process_tool_environment_id( - tools: &[ConfiguredToolSpec], + tools: &[ToolSpec], expected_name: &str, expected_present: bool, ) { let tool = find_tool(tools, expected_name); - let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &tool.spec else { + let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = tool else { panic!("expected function tool {expected_name}"); }; let (properties, _) = expect_object_schema(parameters); @@ -2716,9 +2704,9 @@ fn assert_process_tool_environment_id( ); } -fn assert_apply_patch_environment_id(tools: &[ConfiguredToolSpec], expected_present: bool) { +fn assert_apply_patch_environment_id(tools: &[ToolSpec], expected_present: bool) { let tool = find_tool(tools, "apply_patch"); - let ToolSpec::Freeform(FreeformTool { format, .. }) = &tool.spec else { + let ToolSpec::Freeform(FreeformTool { format, .. }) = tool else { panic!("expected freeform apply_patch tool"); }; assert_eq!( @@ -2729,12 +2717,12 @@ fn assert_apply_patch_environment_id(tools: &[ConfiguredToolSpec], expected_pres } fn find_namespace_function_tool<'a>( - tools: &'a [ConfiguredToolSpec], + tools: &'a [ToolSpec], expected_namespace: &str, expected_name: &str, ) -> &'a ResponsesApiTool { let namespace_tool = find_tool(tools, expected_namespace); - let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + let ToolSpec::Namespace(namespace) = namespace_tool else { panic!("expected namespace tool {expected_namespace}"); }; namespace @@ -2747,9 +2735,9 @@ fn find_namespace_function_tool<'a>( .unwrap_or_else(|| panic!("expected tool {expected_namespace}{expected_name} in namespace")) } -fn namespace_function_names(tools: &[ConfiguredToolSpec], expected_namespace: &str) -> Vec { +fn namespace_function_names(tools: &[ToolSpec], expected_namespace: &str) -> Vec { let namespace_tool = find_tool(tools, expected_namespace); - let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + let ToolSpec::Namespace(namespace) = namespace_tool else { panic!("expected namespace tool {expected_namespace}"); }; namespace diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index c97d3ccdeb..0086542859 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -17,7 +17,6 @@ use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; use codex_protocol::protocol::SessionSource; use codex_tools::AdditionalProperties; -use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverableTool; use codex_tools::JsonSchema; use codex_tools::LoadableToolSpec; @@ -163,11 +162,11 @@ fn deferred_responses_api_tool_serializes_with_defer_loading() { } // Avoid order-based assertions; compare via set containment instead. -fn assert_contains_tool_names(tools: &[ConfiguredToolSpec], expected_subset: &[&str]) { +fn assert_contains_tool_names(tools: &[ToolSpec], expected_subset: &[&str]) { use std::collections::HashSet; let mut names = HashSet::new(); let mut duplicates = Vec::new(); - for name in tools.iter().map(ConfiguredToolSpec::name) { + for name in tools.iter().map(ToolSpec::name) { if !names.insert(name) { duplicates.push(name); } @@ -194,7 +193,7 @@ fn shell_tool_name(config: &ToolsConfig) -> Option<&'static str> { } } -fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a ConfiguredToolSpec { +fn find_tool<'a>(tools: &'a [ToolSpec], expected_name: &str) -> &'a ToolSpec { tools .iter() .find(|tool| tool.name() == expected_name) @@ -202,12 +201,12 @@ fn find_tool<'a>(tools: &'a [ConfiguredToolSpec], expected_name: &str) -> &'a Co } fn find_namespace_function_tool<'a>( - tools: &'a [ConfiguredToolSpec], + tools: &'a [ToolSpec], expected_namespace: &str, expected_name: &str, ) -> &'a ResponsesApiTool { let namespace_tool = find_tool(tools, expected_namespace); - let ToolSpec::Namespace(namespace) = &namespace_tool.spec else { + let ToolSpec::Namespace(namespace) = namespace_tool else { panic!("expected namespace tool {expected_namespace}"); }; namespace @@ -249,7 +248,7 @@ fn multi_agent_v2_spawn_agent_description(tools_config: &ToolsConfig) -> String ) .build(); let spawn_agent = find_tool(&tools, "spawn_agent"); - let ToolSpec::Function(ResponsesApiTool { description, .. }) = &spawn_agent.spec else { + let ToolSpec::Function(ResponsesApiTool { description, .. }) = spawn_agent else { panic!("spawn_agent should be a function tool"); }; description.clone() @@ -326,7 +325,7 @@ async fn get_memory_requires_feature_flag() { ) .build(); assert!( - !tools.iter().any(|t| t.spec.name() == "get_memory"), + !tools.iter().any(|t| t.name() == "get_memory"), "get_memory should be disabled when memory_tool feature is off" ); } @@ -778,7 +777,7 @@ async fn multi_agent_v2_wait_agent_schema_uses_configured_min_timeout() { ) .build(); let wait_agent = find_tool(&tools, "wait_agent"); - let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = &wait_agent.spec else { + let ToolSpec::Function(ResponsesApiTool { parameters, .. }) = wait_agent else { panic!("wait_agent should be a function tool"); }; let timeout_description = parameters @@ -867,7 +866,7 @@ async fn search_tool_description_handles_no_enabled_mcp_tools() { ) .build(); let search_tool = find_tool(&tools, TOOL_SEARCH_TOOL_NAME); - let ToolSpec::ToolSearch { description, .. } = &search_tool.spec else { + let ToolSpec::ToolSearch { description, .. } = search_tool else { panic!("expected tool_search tool"); }; @@ -916,7 +915,7 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio ) .build(); let search_tool = find_tool(&tools, TOOL_SEARCH_TOOL_NAME); - let ToolSpec::ToolSearch { description, .. } = &search_tool.spec else { + let ToolSpec::ToolSearch { description, .. } = search_tool else { panic!("expected tool_search tool"); }; @@ -1123,7 +1122,7 @@ async fn unavailable_mcp_tools_are_exposed_as_dummy_function_tools() { description, parameters, .. - }) = &tool.spec + }) = tool else { panic!("unavailable MCP tool should be exposed as a function tool"); }; diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index d0a1794cbc..8c63e6e631 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -76,7 +76,6 @@ pub use tool_discovery::collect_request_plugin_install_entries; pub use tool_discovery::collect_tool_search_source_infos; pub use tool_discovery::filter_request_plugin_install_discoverable_tools_for_client; pub use tool_discovery::tool_search_result_source_to_loadable_tool_spec; -pub use tool_spec::ConfiguredToolSpec; pub use tool_spec::ResponsesApiWebSearchFilters; pub use tool_spec::ResponsesApiWebSearchUserLocation; pub use tool_spec::ToolSpec; diff --git a/codex-rs/tools/src/tool_spec.rs b/codex-rs/tools/src/tool_spec.rs index be8d00d08a..2a0183a905 100644 --- a/codex-rs/tools/src/tool_spec.rs +++ b/codex-rs/tools/src/tool_spec.rs @@ -75,25 +75,6 @@ impl From for ToolSpec { } } -#[derive(Debug, Clone, PartialEq)] -pub struct ConfiguredToolSpec { - pub spec: ToolSpec, - pub supports_parallel_tool_calls: bool, -} - -impl ConfiguredToolSpec { - pub fn new(spec: ToolSpec, supports_parallel_tool_calls: bool) -> Self { - Self { - spec, - supports_parallel_tool_calls, - } - } - - pub fn name(&self) -> &str { - self.spec.name() - } -} - /// Returns JSON values that are compatible with Function Calling in the /// Responses API: /// https://platform.openai.com/docs/guides/function-calling?api-mode=responses diff --git a/codex-rs/tools/src/tool_spec_tests.rs b/codex-rs/tools/src/tool_spec_tests.rs index 0b98625626..6d2017e553 100644 --- a/codex-rs/tools/src/tool_spec_tests.rs +++ b/codex-rs/tools/src/tool_spec_tests.rs @@ -1,4 +1,3 @@ -use super::ConfiguredToolSpec; use super::ResponsesApiNamespace; use super::ResponsesApiWebSearchFilters; use super::ResponsesApiWebSearchUserLocation; @@ -92,29 +91,6 @@ fn tool_spec_name_covers_all_variants() { ); } -#[test] -fn configured_tool_spec_name_delegates_to_tool_spec() { - assert_eq!( - ConfiguredToolSpec::new( - ToolSpec::Function(ResponsesApiTool { - name: "lookup_order".to_string(), - description: "Look up an order".to_string(), - strict: false, - defer_loading: None, - parameters: JsonSchema::object( - BTreeMap::new(), - /*required*/ None, - /*additional_properties*/ None - ), - output_schema: None, - }), - /*supports_parallel_tool_calls*/ true, - ) - .name(), - "lookup_order" - ); -} - #[test] fn web_search_config_converts_to_responses_api_types() { assert_eq!(