From 38c442ca7f1383a2f6ce80e038518b266325ba37 Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Fri, 13 Feb 2026 09:32:46 -0800 Subject: [PATCH] core: limit search_tool_bm25 to Apps and clarify discovery guidance (#11669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Limit `search_tool_bm25` indexing to `codex_apps` tools only, so non-Apps MCP servers are no longer discoverable through this search path. - Move search-tool discovery guidance into the `search_tool_bm25` tool description (via template include) instead of injecting it as a separate developer message. - Update Apps discovery guidance wording to clarify when to use `search_tool_bm25` for Apps-backed systems (for example Slack, Google Drive, Jira, Notion) and when to call tools directly. - Remove dead `core` helper code (`filter_codex_apps_mcp_tools` and `codex_apps_connector_id`) that is no longer used after the tool-selection refactor. - Update `core` search-tool tests to assert codex-apps-only behavior and to validate guidance from the tool description. ## Validation - ✅ `just fmt` - ✅ `cargo test -p codex-core search_tool` - ⚠️ `cargo test -p codex-core` was attempted, but the run repeatedly stalled on `tools::js_repl::tests::js_repl_can_attach_image_via_view_image_tool`. ## Tickets - None --- codex-rs/core/src/codex.rs | 103 +++++++------- codex-rs/core/src/mcp_connection_manager.rs | 32 +++-- .../src/tools/handlers/search_tool_bm25.rs | 14 +- codex-rs/core/src/tools/js_repl/mod.rs | 1 + codex-rs/core/src/tools/router.rs | 8 +- codex-rs/core/src/tools/spec.rs | 130 +++++++++++++++--- .../search_tool/developer_instructions.md | 29 ---- .../templates/search_tool/tool_description.md | 30 ++++ codex-rs/core/tests/suite/search_tool.rs | 116 +++++++++------- 9 files changed, 287 insertions(+), 176 deletions(-) delete mode 100644 codex-rs/core/templates/search_tool/developer_instructions.md create mode 100644 codex-rs/core/templates/search_tool/tool_description.md diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index d21ad0d0a..09d46af33 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -522,9 +522,6 @@ pub(crate) struct Session { next_internal_sub_id: AtomicU64, } -const SEARCH_TOOL_DEVELOPER_INSTRUCTIONS: &str = - include_str!("../templates/search_tool/developer_instructions.md"); - /// The context needed for a single turn of the thread. #[derive(Debug)] pub(crate) struct TurnContext { @@ -2509,11 +2506,6 @@ impl Session { if let Some(developer_instructions) = turn_context.developer_instructions.as_deref() { items.push(DeveloperInstructions::new(developer_instructions.to_string()).into()); } - if turn_context.tools_config.search_tool { - items.push( - DeveloperInstructions::new(SEARCH_TOOL_DEVELOPER_INSTRUCTIONS.to_string()).into(), - ); - } // Add developer instructions for memories. if let Some(memory_prompt) = build_memory_tool_developer_instructions(&turn_context.config.codex_home).await @@ -4549,14 +4541,15 @@ fn collect_explicit_app_ids_from_skill_items( } fn filter_connectors_for_input( - connectors: Vec, + connectors: &[connectors::AppInfo], input: &[ResponseItem], explicitly_enabled_connectors: &HashSet, skill_name_counts_lower: &HashMap, ) -> Vec { - let connectors = connectors - .into_iter() + let connectors: Vec = connectors + .iter() .filter(|connector| connector.is_enabled) + .cloned() .collect::>(); if connectors.is_empty() { return Vec::new(); @@ -4624,7 +4617,7 @@ fn connector_inserted_in_messages( } fn filter_codex_apps_mcp_tools( - mut mcp_tools: HashMap, + mcp_tools: &HashMap, connectors: &[connectors::AppInfo], ) -> HashMap { let allowed: HashSet<&str> = connectors @@ -4632,17 +4625,19 @@ fn filter_codex_apps_mcp_tools( .map(|connector| connector.id.as_str()) .collect(); - mcp_tools.retain(|_, tool| { - if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { - return true; - } - let Some(connector_id) = codex_apps_connector_id(tool) else { - return false; - }; - allowed.contains(connector_id) - }); - mcp_tools + .iter() + .filter(|(_, tool)| { + if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { + return true; + } + let Some(connector_id) = codex_apps_connector_id(tool) else { + return false; + }; + allowed.contains(connector_id) + }) + .map(|(name, tool)| (name.clone(), tool.clone())) + .collect() } fn codex_apps_connector_id(tool: &crate::mcp_connection_manager::ToolInfo) -> Option<&str> { @@ -4804,42 +4799,45 @@ async fn built_tools( let mut effective_explicitly_enabled_connectors = explicitly_enabled_connectors.clone(); effective_explicitly_enabled_connectors.extend(sess.get_connector_selection().await); - let connectors_for_tools = if turn_context.config.features.enabled(Feature::Apps) { - let skill_name_counts_lower = skills_outcome.map_or_else(HashMap::new, |outcome| { - build_skill_name_counts(&outcome.skills, &outcome.disabled_paths).1 - }); - let connectors = connectors::with_app_enabled_state( + let connectors = if turn_context.features.enabled(Feature::Apps) { + Some(connectors::with_app_enabled_state( connectors::accessible_connectors_from_mcp_tools(&mcp_tools), &turn_context.config, - ); - Some(filter_connectors_for_input( - connectors, - input, - &effective_explicitly_enabled_connectors, - &skill_name_counts_lower, )) } else { None }; - if turn_context.config.features.enabled(Feature::Apps) { + if let Some(connectors) = connectors.as_ref() { + let skill_name_counts_lower = skills_outcome.map_or_else(HashMap::new, |outcome| { + build_skill_name_counts(&outcome.skills, &outcome.disabled_paths).1 + }); + + let explicitly_enabled = filter_connectors_for_input( + connectors, + input, + &effective_explicitly_enabled_connectors, + &skill_name_counts_lower, + ); + let mut selected_mcp_tools = if let Some(selected_tools) = sess.get_mcp_tool_selection().await { - filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tools) + filter_mcp_tools_by_name(&mcp_tools, &selected_tools) } else { HashMap::new() }; - if let Some(connectors) = connectors_for_tools.as_ref() { - let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, connectors); - selected_mcp_tools.extend(apps_mcp_tools); - } + let apps_mcp_tools = + filter_codex_apps_mcp_tools_only(&mcp_tools, explicitly_enabled.as_ref()); + selected_mcp_tools.extend(apps_mcp_tools); mcp_tools = selected_mcp_tools; - } else if let Some(connectors) = connectors_for_tools.as_ref() { - mcp_tools = filter_codex_apps_mcp_tools(mcp_tools, connectors); } + let app_tools = connectors + .as_ref() + .map(|connectors| filter_codex_apps_mcp_tools(&mcp_tools, connectors)); + Ok(Arc::new(ToolRouter::from_config( &turn_context.tools_config, Some( @@ -4848,6 +4846,7 @@ async fn built_tools( .map(|(name, tool)| (name, tool.tool)) .collect(), ), + app_tools, turn_context.dynamic_tools.as_slice(), ))) } @@ -5803,7 +5802,7 @@ mod tests { let skill_name_counts_lower = HashMap::new(); let selected = filter_connectors_for_input( - connectors, + &connectors, &input, &explicitly_enabled_connectors, &skill_name_counts_lower, @@ -5820,7 +5819,7 @@ mod tests { let skill_name_counts_lower = HashMap::from([("todoist".to_string(), 1)]); let selected = filter_connectors_for_input( - connectors, + &connectors, &input, &explicitly_enabled_connectors, &skill_name_counts_lower, @@ -5836,7 +5835,7 @@ mod tests { let input = vec![user_message("use $calendar")]; let explicitly_enabled_connectors = HashSet::new(); let selected = filter_connectors_for_input( - vec![connector], + &[connector], &input, &explicitly_enabled_connectors, &HashMap::new(), @@ -5910,17 +5909,16 @@ mod tests { ), ]); - let mut selected_mcp_tools = - filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tool_names); + let mut selected_mcp_tools = filter_mcp_tools_by_name(&mcp_tools, &selected_tool_names); let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); let explicitly_enabled_connectors = HashSet::new(); let connectors = filter_connectors_for_input( - connectors, + &connectors, &[user_message("run the selected tools")], &explicitly_enabled_connectors, &HashMap::new(), ); - let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, &connectors); + let apps_mcp_tools = filter_codex_apps_mcp_tools_only(&mcp_tools, &connectors); selected_mcp_tools.extend(apps_mcp_tools); let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); @@ -5953,17 +5951,16 @@ mod tests { ), ]); - let mut selected_mcp_tools = - filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tool_names); + let mut selected_mcp_tools = filter_mcp_tools_by_name(&mcp_tools, &selected_tool_names); let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); let explicitly_enabled_connectors = HashSet::new(); let connectors = filter_connectors_for_input( - connectors, + &connectors, &[user_message("use $calendar and then echo the response")], &explicitly_enabled_connectors, &HashMap::new(), ); - let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, &connectors); + let apps_mcp_tools = filter_codex_apps_mcp_tools_only(&mcp_tools, &connectors); selected_mcp_tools.extend(apps_mcp_tools); let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); @@ -7627,6 +7624,7 @@ mod tests { .list_all_tools() .await }; + let app_tools = Some(tools.clone()); let router = ToolRouter::from_config( &turn_context.tools_config, Some( @@ -7635,6 +7633,7 @@ mod tests { .map(|(name, tool)| (name, tool.tool)) .collect(), ), + app_tools, turn_context.dynamic_tools.as_slice(), ); let item = ResponseItem::CustomToolCall { diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 5b9d3b997..af2003375 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -882,7 +882,7 @@ fn filter_tools(tools: Vec, filter: ToolFilter) -> Vec { } pub(crate) fn filter_codex_apps_mcp_tools_only( - mut mcp_tools: HashMap, + mcp_tools: &HashMap, connectors: &[crate::connectors::AppInfo], ) -> HashMap { let allowed: HashSet<&str> = connectors @@ -890,26 +890,32 @@ pub(crate) fn filter_codex_apps_mcp_tools_only( .map(|connector| connector.id.as_str()) .collect(); - mcp_tools.retain(|_, tool| { - if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { - return false; - } - let Some(connector_id) = tool.connector_id.as_deref() else { - return false; - }; - allowed.contains(connector_id) - }); - mcp_tools + .iter() + .filter(|(_, tool)| { + if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { + return false; + } + let Some(connector_id) = tool.connector_id.as_deref() else { + return false; + }; + allowed.contains(connector_id) + }) + .map(|(name, tool)| (name.clone(), tool.clone())) + .collect() } pub(crate) fn filter_mcp_tools_by_name( - mut mcp_tools: HashMap, + mcp_tools: &HashMap, selected_tools: &[String], ) -> HashMap { let allowed: HashSet<&str> = selected_tools.iter().map(String::as_str).collect(); - mcp_tools.retain(|name, _| allowed.contains(name.as_str())); + mcp_tools + .iter() + .filter(|(name, _)| allowed.contains(name.as_str())) + .map(|(name, tool)| (name.clone(), tool.clone())) + .collect() } fn normalize_codex_apps_tool_title( diff --git a/codex-rs/core/src/tools/handlers/search_tool_bm25.rs b/codex-rs/core/src/tools/handlers/search_tool_bm25.rs index 54dd5c3dd..37fb34267 100644 --- a/codex-rs/core/src/tools/handlers/search_tool_bm25.rs +++ b/codex-rs/core/src/tools/handlers/search_tool_bm25.rs @@ -208,7 +208,7 @@ fn filter_codex_apps_mcp_tools( mcp_tools.retain(|_, tool| { if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { - return true; + return false; } tool.connector_id @@ -308,7 +308,7 @@ mod tests { } #[test] - fn filter_codex_apps_mcp_tools_keeps_non_apps_and_enabled_apps() { + fn filter_codex_apps_mcp_tools_keeps_enabled_apps_only() { let mcp_tools = HashMap::from([ make_tool( "mcp__codex_apps__calendar_create_event", @@ -334,13 +334,7 @@ mod tests { .collect(); filtered.sort(); - assert_eq!( - filtered, - vec![ - "mcp__codex_apps__drive_search".to_string(), - "mcp__rmcp__echo".to_string(), - ] - ); + assert_eq!(filtered, vec!["mcp__codex_apps__drive_search".to_string()]); } #[test] @@ -361,6 +355,6 @@ mod tests { .collect(); filtered.sort(); - assert_eq!(filtered, vec!["mcp__rmcp__echo".to_string()]); + assert_eq!(filtered, Vec::::new()); } } diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 8b362ab8f..f6fd8188e 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -653,6 +653,7 @@ impl JsReplManager { .map(|(name, tool)| (name, tool.tool)) .collect(), ), + None, exec.turn.dynamic_tools.as_slice(), ); diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 1cc8b9177..1f1b7f3e0 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -2,6 +2,7 @@ use crate::client_common::tools::ToolSpec; use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; +use crate::mcp_connection_manager::ToolInfo; use crate::sandboxing::SandboxPermissions; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::context::ToolInvocation; @@ -43,9 +44,10 @@ impl ToolRouter { pub fn from_config( config: &ToolsConfig, mcp_tools: Option>, + app_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> Self { - let builder = build_specs(config, mcp_tools, dynamic_tools); + let builder = build_specs(config, mcp_tools, app_tools, dynamic_tools); let (specs, registry) = builder.build(); Self { registry, specs } @@ -238,6 +240,7 @@ mod tests { .await .list_all_tools() .await; + let app_tools = Some(mcp_tools.clone()); let router = ToolRouter::from_config( &turn.tools_config, Some( @@ -246,6 +249,7 @@ mod tests { .map(|(name, tool)| (name, tool.tool)) .collect(), ), + app_tools, turn.dynamic_tools.as_slice(), ); @@ -289,6 +293,7 @@ mod tests { .await .list_all_tools() .await; + let app_tools = Some(mcp_tools.clone()); let router = ToolRouter::from_config( &turn.tools_config, Some( @@ -297,6 +302,7 @@ mod tests { .map(|(name, tool)| (name, tool.tool)) .collect(), ), + app_tools, turn.dynamic_tools.as_slice(), ); diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 82e34298c..06181ae0f 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -5,6 +5,7 @@ use crate::client_common::tools::ResponsesApiTool; use crate::client_common::tools::ToolSpec; use crate::features::Feature; use crate::features::Features; +use crate::mcp_connection_manager::ToolInfo; use crate::tools::handlers::PLAN_TOOL; use crate::tools::handlers::SEARCH_TOOL_BM25_DEFAULT_LIMIT; use crate::tools::handlers::apply_patch::create_apply_patch_freeform_tool; @@ -27,6 +28,9 @@ use serde_json::json; use std::collections::BTreeMap; use std::collections::HashMap; +const SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE: &str = + include_str!("../../templates/search_tool/tool_description.md"); + #[derive(Debug, Clone)] pub(crate) struct ToolsConfig { pub shell_type: ConfigShellToolType, @@ -880,12 +884,12 @@ fn create_grep_files_tool() -> ToolSpec { }) } -fn create_search_tool_bm25_tool() -> ToolSpec { +fn create_search_tool_bm25_tool(app_tools: &HashMap) -> ToolSpec { let properties = BTreeMap::from([ ( "query".to_string(), JsonSchema::String { - description: Some("Search query for MCP tools.".to_string()), + description: Some("Search query for apps tools.".to_string()), }, ), ( @@ -897,10 +901,20 @@ fn create_search_tool_bm25_tool() -> ToolSpec { }, ), ]); + let mut app_names = app_tools + .values() + .filter_map(|tool| tool.connector_name.clone()) + .collect::>(); + app_names.sort(); + app_names.dedup(); + let app_names = app_names.join(", "); + + let description = + SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str()); ToolSpec::Function(ResponsesApiTool { name: "search_tool_bm25".to_string(), - description: "Searches MCP tool metadata with BM25 and exposes matching tools for the next model call.".to_string(), + description, strict: false, parameters: JsonSchema::Object { properties, @@ -1390,6 +1404,7 @@ fn sanitize_json_schema(value: &mut JsonValue) { pub(crate) fn build_specs( config: &ToolsConfig, mcp_tools: Option>, + app_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { use crate::tools::handlers::ApplyPatchHandler; @@ -1488,8 +1503,10 @@ pub(crate) fn build_specs( builder.register_handler("request_user_input", request_user_input_handler); } - if config.search_tool { - builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(), true); + if config.search_tool + && let Some(app_tools) = app_tools + { + builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(&app_tools), true); builder.register_handler("search_tool_bm25", search_tool_handler); } @@ -1795,7 +1812,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Live), }); - let (tools, _) = build_specs(&config, None, &[]).build(); + let (tools, _) = build_specs(&config, None, None, &[]).build(); // Build actual map name -> spec use std::collections::BTreeMap; @@ -1860,7 +1877,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert_contains_tool_names( &tools, &[ @@ -1885,7 +1902,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert!( !tools.iter().any(|t| t.spec.name() == "request_user_input"), "request_user_input should be disabled when collaboration_modes feature is off" @@ -1897,7 +1914,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert_contains_tool_names(&tools, &["request_user_input"]); } @@ -1913,7 +1930,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert!( !tools.iter().any(|tool| tool.spec.name() == "js_repl"), @@ -1938,7 +1955,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert_contains_tool_names(&tools, &["js_repl", "js_repl_reset"]); } @@ -1956,7 +1973,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); let filtered = filter_tools_for_model( tools.iter().map(|tool| tool.spec.clone()).collect(), &tools_config, @@ -1994,7 +2011,7 @@ mod tests { "additionalProperties": false }), }]; - let (tools, _) = build_specs(&tools_config, None, &dynamic_tools).build(); + let (tools, _) = build_specs(&tools_config, None, None, &dynamic_tools).build(); assert!( tools.iter().any(|tool| tool.spec.name() == "dynamic_echo"), "expected dynamic tool in full router specs" @@ -2025,7 +2042,7 @@ mod tests { features, web_search_mode, }); - let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), &[]).build(); + let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), None, &[]).build(); let tool_names = tools.iter().map(|t| t.spec.name()).collect::>(); assert_eq!(&tool_names, &expected_tools,); } @@ -2058,7 +2075,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); let tool = find_tool(&tools, "web_search"); assert_eq!( @@ -2081,7 +2098,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Live), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); let tool = find_tool(&tools, "web_search"); assert_eq!( @@ -2306,7 +2323,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Live), }); - let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), &[]).build(); + let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), None, &[]).build(); // Only check the shell variant and a couple of core tools. let mut subset = vec!["exec_command", "write_stdin", "update_plan"]; @@ -2329,7 +2346,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert!(find_tool(&tools, "exec_command").supports_parallel_tool_calls); assert!(!find_tool(&tools, "write_stdin").supports_parallel_tool_calls); @@ -2353,7 +2370,7 @@ mod tests { features: &features, web_search_mode: Some(WebSearchMode::Cached), }); - let (tools, _) = build_specs(&tools_config, None, &[]).build(); + let (tools, _) = build_specs(&tools_config, None, None, &[]).build(); assert!( tools @@ -2409,6 +2426,7 @@ mod tests { }), ), )])), + None, &[], ) .build(); @@ -2486,7 +2504,7 @@ mod tests { ), ]); - let (tools, _) = build_specs(&tools_config, Some(tools_map), &[]).build(); + let (tools, _) = build_specs(&tools_config, Some(tools_map), None, &[]).build(); // Only assert that the MCP tools themselves are sorted by fully-qualified name. let mcp_names: Vec<_> = tools @@ -2502,6 +2520,73 @@ mod tests { assert_eq!(mcp_names, expected); } + #[test] + fn search_tool_description_includes_only_codex_apps_connector_names() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let mut features = Features::with_defaults(); + features.enable(Feature::Apps); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + }); + + let (tools, _) = build_specs( + &tools_config, + Some(HashMap::from([ + ( + "mcp__codex_apps__calendar_create_event".to_string(), + mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + ), + ( + "mcp__rmcp__echo".to_string(), + mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), + ), + ])), + Some(HashMap::from([ + ( + "mcp__codex_apps__calendar_create_event".to_string(), + ToolInfo { + server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "calendar_create_event".to_string(), + tool: mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + }, + ), + ( + "mcp__rmcp__echo".to_string(), + ToolInfo { + server_name: "rmcp".to_string(), + tool_name: "echo".to_string(), + tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), + connector_id: None, + connector_name: None, + }, + ), + ])), + &[], + ) + .build(); + + let search_tool = find_tool(&tools, "search_tool_bm25"); + let ToolSpec::Function(ResponsesApiTool { description, .. }) = &search_tool.spec else { + panic!("expected function tool"); + }; + assert!(description.contains("Calendar")); + assert!(!description.contains("mcp__rmcp__echo")); + } + #[test] fn test_mcp_tool_property_missing_type_defaults_to_string() { let config = test_config(); @@ -2530,6 +2615,7 @@ mod tests { }), ), )])), + None, &[], ) .build(); @@ -2581,6 +2667,7 @@ mod tests { }), ), )])), + None, &[], ) .build(); @@ -2631,6 +2718,7 @@ mod tests { }), ), )])), + None, &[], ) .build(); @@ -2685,6 +2773,7 @@ mod tests { }), ), )])), + None, &[], ) .build(); @@ -2811,6 +2900,7 @@ Examples of valid command strings: }), ), )])), + None, &[], ) .build(); diff --git a/codex-rs/core/templates/search_tool/developer_instructions.md b/codex-rs/core/templates/search_tool/developer_instructions.md deleted file mode 100644 index 5ff6a83e5..000000000 --- a/codex-rs/core/templates/search_tool/developer_instructions.md +++ /dev/null @@ -1,29 +0,0 @@ -# MCP tool discovery - -When `search_tool_bm25` is available, MCP tools (`mcp__...`) are hidden until you search for them. - -Follow this workflow: - -1. Call `search_tool_bm25` with: - - `query` (required): focused terms that describe the capability you need. - - `limit` (optional): maximum number of tools to return (default `8`). -2. Use the returned `tools` list to decide which MCP tools are relevant. -3. Matching tools are added to `active_selected_tools`. Only tools in `active_selected_tools` are available for the remainder of the current turn. -4. Repeated searches in the same turn are additive: new matches are unioned into `active_selected_tools`. -5. `active_selected_tools` resets at the start of the next turn. - -Notes: -- Core tools remain available without searching. -- If you are unsure, start with `limit` between 5 and 10 to see a broader set of tools. -- `query` is matched against MCP tool metadata fields: - - `name` - - `tool_name` - - `server_name` - - `title` - - `description` - - `connector_name` - - `connector_id` - - input schema property keys (`input_keys`) -- When the user asks to search/lookup/query any external system (logs, tickets, metrics, Slack, etc.), you must call `search_tool_bm25` first before running any shell command or repo search. -- Only use shell commands if (a) MCP tools for that system are not available or not sufficient, and (b) the user explicitly wants a local file/CLI search. -- If unsure which system/tool applies, ask a clarifying question after checking MCP tools. diff --git a/codex-rs/core/templates/search_tool/tool_description.md b/codex-rs/core/templates/search_tool/tool_description.md new file mode 100644 index 000000000..ce4268177 --- /dev/null +++ b/codex-rs/core/templates/search_tool/tool_description.md @@ -0,0 +1,30 @@ +# Apps tool discovery + +Searches over apps tool metadata with BM25 and exposes matching tools for the next model call. + +When `search_tool_bm25` is available, apps tools (`mcp__codex_apps__...`) are hidden until you search for them. + +Follow this workflow: + +1. Call `search_tool_bm25` with: + - `query` (required): focused terms that describe the capability you need. + - `limit` (optional): maximum number of tools to return (default `8`). +2. Use the returned `tools` list to decide which Apps tools are relevant. +3. Matching tools are added to available `tools` and available for the remainder of the current turn. +4. Repeated searches in the same turn are additive: new matches are unioned into `tools`. + +Notes: +- Core tools remain available without searching. +- If you are unsure, start with `limit` between 5 and 10 to see a broader set of tools. +- `query` is matched against Apps tool metadata fields: + - `name` + - `tool_name` + - `server_name` + - `title` + - `description` + - `connector_name` + - `connector_id` + - input schema property keys (`input_keys`) +- Use `search_tool_bm25` when the user asks to work with one of apps ({{app_names}}) and the exact tool name is not already known. +- If the needed app is already explicit in the prompt (for example an `apps://...` mention) or already present in the current `tools` list, you can call that tool directly. +- Do not use `search_tool_bm25` for non-apps/local tasks (filesystem, repo search, or shell-only workflows). diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index c819c03b2..d40c12a42 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -25,8 +25,8 @@ use serde_json::Value; use serde_json::json; const SEARCH_TOOL_INSTRUCTION_SNIPPETS: [&str; 2] = [ - "MCP tools (`mcp__...`) are hidden until you search for them.", - "Matching tools are added to `active_selected_tools`.", + "apps tools (`mcp__codex_apps__...`) are hidden until you search for them.", + "Matching tools are added to available `tools` and available for the remainder of the current turn.", ]; fn tool_names(body: &Value) -> Vec { @@ -46,30 +46,20 @@ fn tool_names(body: &Value) -> Vec { .unwrap_or_default() } -fn developer_messages(body: &Value) -> Vec { - body.get("input") +fn search_tool_description(body: &Value) -> Option { + body.get("tools") .and_then(Value::as_array) - .map(|items| { - items - .iter() - .filter_map(|item| { - if item.get("role").and_then(Value::as_str) != Some("developer") { - return None; - } - let content = item.get("content").and_then(Value::as_array)?; - let texts: Vec<&str> = content - .iter() - .filter_map(|entry| entry.get("text").and_then(Value::as_str)) - .collect(); - if texts.is_empty() { - None - } else { - Some(texts.join("\n")) - } - }) - .collect() + .and_then(|tools| { + tools.iter().find_map(|tool| { + if tool.get("name").and_then(Value::as_str) == Some("search_tool_bm25") { + tool.get("description") + .and_then(Value::as_str) + .map(str::to_string) + } else { + None + } + }) }) - .unwrap_or_default() } fn search_tool_output_payload(request: &ResponsesRequest, call_id: &str) -> Value { @@ -95,6 +85,16 @@ fn active_selected_tools(payload: &Value) -> Vec { .collect() } +fn search_result_tools(payload: &Value) -> Vec<&Value> { + payload + .get("tools") + .and_then(Value::as_array) + .map(Vec::as_slice) + .unwrap_or_default() + .iter() + .collect() +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn search_tool_flag_adds_tool() -> Result<()> { skip_if_no_network!(Ok(())); @@ -133,7 +133,7 @@ async fn search_tool_flag_adds_tool() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn search_tool_adds_developer_instructions() -> Result<()> { +async fn search_tool_adds_discovery_instructions_to_tool_description() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -160,14 +160,13 @@ async fn search_tool_adds_developer_instructions() -> Result<()> { .await?; let body = mock.single_request().body_json(); - let developer_texts = developer_messages(&body); + let search_tool_description = + search_tool_description(&body).expect("search tool description should be present"); assert!( - developer_texts.iter().any(|text| { - SEARCH_TOOL_INSTRUCTION_SNIPPETS - .iter() - .all(|snippet| text.contains(snippet)) - }), - "developer instructions should include search tool workflow: {developer_texts:?}" + SEARCH_TOOL_INSTRUCTION_SNIPPETS + .iter() + .all(|snippet| search_tool_description.contains(snippet)), + "search tool description should include search tool workflow: {search_tool_description:?}" ); Ok(()) @@ -331,12 +330,12 @@ async fn search_tool_selection_persists_within_turn_and_resets_next_turn() -> Re let second_tools = tool_names(&requests[1].body_json()); assert!( - second_tools.iter().any(|name| name == "mcp__rmcp__echo"), - "second request should include selected MCP tool: {second_tools:?}" + !second_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "second request should not include rmcp MCP tools after search: {second_tools:?}" ); assert!( !second_tools.iter().any(|name| name == "mcp__rmcp__image"), - "second request should only include selected MCP tool: {second_tools:?}" + "second request should not include rmcp MCP tools after search: {second_tools:?}" ); let search_output_payload = search_tool_output_payload(&requests[1], call_id); @@ -344,9 +343,18 @@ async fn search_tool_selection_persists_within_turn_and_resets_next_turn() -> Re search_output_payload.get("selected_tools").is_none(), "selected_tools should not be returned: {search_output_payload:?}" ); - assert_eq!( - active_selected_tools(&search_output_payload), - vec!["mcp__rmcp__echo".to_string()], + for tool in search_result_tools(&search_output_payload) { + assert_eq!( + tool.get("server").and_then(Value::as_str), + Some("codex_apps"), + "search results should only include codex_apps tools: {search_output_payload:?}" + ); + } + assert!( + !active_selected_tools(&search_output_payload) + .iter() + .any(|tool_name| tool_name.starts_with("mcp__rmcp__")), + "search should not add rmcp tools to active selection: {search_output_payload:?}" ); let third_tools = tool_names(&requests[2].body_json()); @@ -453,22 +461,22 @@ async fn search_tool_selection_unions_results_within_turn() -> Result<()> { let second_tools = tool_names(&requests[1].body_json()); assert!( - second_tools.iter().any(|name| name == "mcp__rmcp__echo"), - "second request should include echo after first search: {second_tools:?}" + !second_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "second request should not include rmcp tools after first search: {second_tools:?}" ); assert!( !second_tools.iter().any(|name| name == "mcp__rmcp__image"), - "second request should not include image before second search runs: {second_tools:?}" + "second request should not include rmcp tools after first search: {second_tools:?}" ); let third_tools = tool_names(&requests[2].body_json()); assert!( - third_tools.iter().any(|name| name == "mcp__rmcp__echo"), - "third request should still include echo: {third_tools:?}" + !third_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "third request should not include rmcp tools: {third_tools:?}" ); assert!( - third_tools.iter().any(|name| name == "mcp__rmcp__image"), - "third request should include image after second search: {third_tools:?}" + !third_tools.iter().any(|name| name == "mcp__rmcp__image"), + "third request should not include rmcp tools: {third_tools:?}" ); let second_search_payload = search_tool_output_payload(&requests[2], second_call_id); @@ -476,12 +484,18 @@ async fn search_tool_selection_unions_results_within_turn() -> Result<()> { second_search_payload.get("selected_tools").is_none(), "selected_tools should not be returned: {second_search_payload:?}" ); - assert_eq!( - active_selected_tools(&second_search_payload), - vec![ - "mcp__rmcp__echo".to_string(), - "mcp__rmcp__image".to_string(), - ], + for tool in search_result_tools(&second_search_payload) { + assert_eq!( + tool.get("server").and_then(Value::as_str), + Some("codex_apps"), + "search results should only include codex_apps tools: {second_search_payload:?}" + ); + } + assert!( + !active_selected_tools(&second_search_payload) + .iter() + .any(|tool_name| tool_name.starts_with("mcp__rmcp__")), + "search should not add rmcp tools to active selection: {second_search_payload:?}" ); Ok(())