From 1dd9bf9a74d66729d0e4169f57df5bc5dc762e77 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Mon, 18 May 2026 21:33:46 -0700 Subject: [PATCH] Remove explicit connector tool undeferral (#23390) ## Summary - remove the explicit-connector carveout that kept mentioned app tools directly exposed instead of deferred - keep the surviving explicit-mention reconstruction only for analytics, preserving `codex_app_mentioned` and `codex_app_used.invoke_type` - trim the now-unused prompt/tool-exposure plumbing and refresh coverage around always-defer behavior ## Verification - `just fmt` - `cargo test -p codex-analytics` - `cargo test -p codex-core` *(one transient timeout in `shell_snapshot::tests::macos_zsh_snapshot_includes_sections`; isolated rerun passed)* - `cargo test -p codex-core --lib shell_snapshot::tests::macos_zsh_snapshot_includes_sections` - `cargo test -p codex-core --test all explicit_app_mentions_respect_always_defer` - `cargo test -p codex-core --lib mcp_tool_exposure::tests::always_defer_feature_defers_apps_too` - `just fix -p codex-analytics` - `just fix -p codex-core` --- codex-rs/core/src/compact_remote.rs | 4 - codex-rs/core/src/compact_remote_v2.rs | 4 - codex-rs/core/src/mcp_tool_exposure.rs | 11 +- codex-rs/core/src/mcp_tool_exposure_test.rs | 83 ++------------ codex-rs/core/src/prompt_debug.rs | 11 +- codex-rs/core/src/session/mod.rs | 2 - codex-rs/core/src/session/tests.rs | 68 ----------- codex-rs/core/src/session/turn.rs | 114 +------------------ codex-rs/core/src/session_startup_prewarm.rs | 4 - codex-rs/core/tests/suite/search_tool.rs | 22 +++- 10 files changed, 26 insertions(+), 297 deletions(-) diff --git a/codex-rs/core/src/compact_remote.rs b/codex-rs/core/src/compact_remote.rs index bc684647e..c7ba1a314 100644 --- a/codex-rs/core/src/compact_remote.rs +++ b/codex-rs/core/src/compact_remote.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::sync::Arc; use crate::Prompt; @@ -174,9 +173,6 @@ async fn run_remote_compact_task_inner_impl( let tool_router = built_tools( sess.as_ref(), turn_context.as_ref(), - &prompt_input, - &HashSet::new(), - /*skills_outcome*/ None, &CancellationToken::new(), ) .await?; diff --git a/codex-rs/core/src/compact_remote_v2.rs b/codex-rs/core/src/compact_remote_v2.rs index 3f5368a96..2e261b2e9 100644 --- a/codex-rs/core/src/compact_remote_v2.rs +++ b/codex-rs/core/src/compact_remote_v2.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::sync::Arc; use crate::Prompt; @@ -180,9 +179,6 @@ async fn run_remote_compact_task_inner_impl( let tool_router = built_tools( sess.as_ref(), turn_context.as_ref(), - &prompt_input, - &HashSet::new(), - /*skills_outcome*/ None, &CancellationToken::new(), ) .await?; diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index 0bf696acd..8f8be1107 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -18,7 +18,6 @@ pub(crate) struct McpToolExposure { pub(crate) fn build_mcp_tool_exposure( all_mcp_tools: &[McpToolInfo], connectors: Option<&[connectors::AppInfo]>, - explicitly_enabled_connectors: &[connectors::AppInfo], config: &Config, tools_config: &ToolsConfig, ) -> McpToolExposure { @@ -44,16 +43,8 @@ pub(crate) fn build_mcp_tool_exposure( }; } - let direct_tools = - filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); - let direct_tool_names = direct_tools - .iter() - .map(McpToolInfo::canonical_tool_name) - .collect::>(); - deferred_tools.retain(|tool| !direct_tool_names.contains(&tool.canonical_tool_name())); - McpToolExposure { - direct_tools, + direct_tools: Vec::new(), deferred_tools: (!deferred_tools.is_empty()).then_some(deferred_tools), } } diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index f9468d898..06c9d1255 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -120,13 +120,8 @@ async fn directly_exposes_small_effective_tool_sets() { let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); - let exposure = build_mcp_tool_exposure( - &mcp_tools, - /*connectors*/ None, - &[], - &config, - &tools_config, - ); + let exposure = + build_mcp_tool_exposure(&mcp_tools, /*connectors*/ None, &config, &tools_config); assert_eq!(tool_names(&exposure.direct_tools), tool_names(&mcp_tools)); assert!(exposure.deferred_tools.is_none()); @@ -138,13 +133,8 @@ async fn searches_large_effective_tool_sets() { let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD); - let exposure = build_mcp_tool_exposure( - &mcp_tools, - /*connectors*/ None, - &[], - &config, - &tools_config, - ); + let exposure = + build_mcp_tool_exposure(&mcp_tools, /*connectors*/ None, &config, &tools_config); assert!(exposure.direct_tools.is_empty()); let deferred_tools = exposure @@ -155,58 +145,7 @@ async fn searches_large_effective_tool_sets() { } #[tokio::test] -async fn directly_exposes_explicit_apps_without_deferred_overlap() { - let config = test_config().await; - let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; - let mut mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); - mcp_tools.push(make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - "mcp__codex_apps__calendar", - "_create_event", - Some("calendar"), - Some("Calendar"), - )); - let connectors = vec![make_connector("calendar", "Calendar")]; - - let exposure = build_mcp_tool_exposure( - &mcp_tools, - Some(connectors.as_slice()), - connectors.as_slice(), - &config, - &tools_config, - ); - - let direct_tool_names = tool_names(&exposure.direct_tools); - assert_eq!( - direct_tool_names, - HashSet::from([ToolName::namespaced( - "mcp__codex_apps__calendar", - "_create_event" - )]) - ); - assert_eq!( - exposure.deferred_tools.as_ref().map(Vec::len), - Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1) - ); - let deferred_tools = exposure - .deferred_tools - .as_ref() - .expect("large tool sets should be discoverable through tool_search"); - let deferred_tool_names = tool_names(deferred_tools); - assert!( - direct_tool_names.is_disjoint(&deferred_tool_names), - "direct tools should not also be deferred: {direct_tool_names:?}" - ); - assert!(!deferred_tool_names.contains(&ToolName::namespaced( - "mcp__codex_apps__calendar", - "_create_event" - ))); - assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool_0"))); -} - -#[tokio::test] -async fn always_defer_feature_preserves_explicit_apps() { +async fn always_defer_feature_defers_apps_too() { let mut config = test_config().await; config .features @@ -236,26 +175,18 @@ async fn always_defer_feature_preserves_explicit_apps() { let exposure = build_mcp_tool_exposure( &mcp_tools, Some(connectors.as_slice()), - connectors.as_slice(), &config, &tools_config, ); - let direct_tool_names = tool_names(&exposure.direct_tools); - assert_eq!( - direct_tool_names, - HashSet::from([ToolName::namespaced( - "mcp__codex_apps__calendar", - "_create_event" - )]) - ); + assert!(exposure.direct_tools.is_empty()); let deferred_tools = exposure .deferred_tools .as_ref() .expect("MCP tools should be discoverable through tool_search"); let deferred_tool_names = tool_names(deferred_tools); assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool"))); - assert!(!deferred_tool_names.contains(&ToolName::namespaced( + assert!(deferred_tool_names.contains(&ToolName::namespaced( "mcp__codex_apps__calendar", "_create_event" ))); diff --git a/codex-rs/core/src/prompt_debug.rs b/codex-rs/core/src/prompt_debug.rs index 72fa5b4e4..c6386d7c7 100644 --- a/codex-rs/core/src/prompt_debug.rs +++ b/codex-rs/core/src/prompt_debug.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::sync::Arc; use codex_exec_server::EnvironmentManager; @@ -86,15 +85,7 @@ pub(crate) async fn build_prompt_input_from_session( .clone_history() .await .for_prompt(&turn_context.model_info.input_modalities); - let router = built_tools( - sess, - turn_context.as_ref(), - &prompt_input, - &HashSet::new(), - Some(turn_context.turn_skills.outcome.as_ref()), - &CancellationToken::new(), - ) - .await?; + let router = built_tools(sess, turn_context.as_ref(), &CancellationToken::new()).await?; let base_instructions = sess.get_base_instructions().await; let prompt = build_prompt( prompt_input, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index cb848286c..99c2f39b6 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -215,8 +215,6 @@ pub(crate) use self::session::SessionSettingsUpdate; use self::turn::AssistantMessageStreamParsers; #[cfg(test)] use self::turn::collect_explicit_app_ids_from_skill_items; -#[cfg(test)] -use self::turn::filter_connectors_for_input; use self::turn::realtime_text_for_event; use self::turn_context::TurnContext; use self::turn_context::TurnSkillsContext; diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 2363f02cc..37b46b6da 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -1497,74 +1497,6 @@ disabled_tools = [ ); } -#[test] -fn filter_connectors_for_input_skips_duplicate_slug_mentions() { - let connectors = vec![ - make_connector("one", "Foo Bar"), - make_connector("two", "Foo-Bar"), - ]; - let input = vec![user_message("use $foo-bar")]; - let explicitly_enabled_connectors = HashSet::new(); - let skill_name_counts_lower = HashMap::new(); - - let selected = filter_connectors_for_input( - &connectors, - &input, - &explicitly_enabled_connectors, - &skill_name_counts_lower, - ); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn filter_connectors_for_input_skips_when_skill_name_conflicts() { - let connectors = vec![make_connector("one", "Todoist")]; - let input = vec![user_message("use $todoist")]; - let explicitly_enabled_connectors = HashSet::new(); - let skill_name_counts_lower = HashMap::from([("todoist".to_string(), 1)]); - - let selected = filter_connectors_for_input( - &connectors, - &input, - &explicitly_enabled_connectors, - &skill_name_counts_lower, - ); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn filter_connectors_for_input_skips_disabled_connectors() { - let mut connector = make_connector("calendar", "Calendar"); - connector.is_enabled = false; - let input = vec![user_message("use $calendar")]; - let explicitly_enabled_connectors = HashSet::new(); - let selected = filter_connectors_for_input( - &[connector], - &input, - &explicitly_enabled_connectors, - &HashMap::new(), - ); - - assert_eq!(selected, Vec::new()); -} - -#[test] -fn filter_connectors_for_input_skips_plugin_mentions() { - let connectors = vec![make_connector("figma", "Figma")]; - let input = vec![user_message("use [@figma](plugin://figma@openai-curated)")]; - let explicitly_enabled_connectors = HashSet::new(); - let selected = filter_connectors_for_input( - &connectors, - &input, - &explicitly_enabled_connectors, - &HashMap::new(), - ); - - assert_eq!(selected, Vec::new()); -} - #[test] fn collect_explicit_app_ids_from_skill_items_includes_linked_mentions() { let connectors = vec![make_connector("calendar", "Calendar")]; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 24788d4d9..27d35504f 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -4,14 +4,12 @@ use std::sync::Arc; use std::sync::atomic::Ordering; use crate::SkillInjections; -use crate::SkillLoadOutcome; use crate::build_skill_injections; use crate::client::ModelClientSession; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::collect_explicit_skill_mentions; use crate::compact::InitialContextInjection; -use crate::compact::collect_user_messages; use crate::compact::run_inline_auto_compact_task; use crate::compact::should_use_remote_compact_task; use crate::compact_remote::run_inline_remote_auto_compact_task; @@ -221,7 +219,6 @@ pub(crate) async fn run_turn( track_turn_resolved_config_analytics(&sess, &turn_context, &input).await; - let skills_outcome = Some(turn_context.turn_skills.outcome.as_ref()); let mut last_agent_message: Option = None; let mut stop_hook_active = false; // Although from the perspective of codex.rs, TurnDiffTracker has the lifecycle of a Task which contains @@ -331,8 +328,6 @@ pub(crate) async fn run_turn( &mut client_session, turn_metadata_header.as_deref(), sampling_request_input, - &explicitly_enabled_connectors, - skills_outcome, cancellation_token.child_token(), ) .await @@ -918,82 +913,6 @@ pub(super) fn collect_explicit_app_ids_from_skill_items( connector_ids } -pub(super) fn filter_connectors_for_input( - connectors: &[connectors::AppInfo], - input: &[ResponseItem], - explicitly_enabled_connectors: &HashSet, - skill_name_counts_lower: &HashMap, -) -> Vec { - let connectors: Vec = connectors - .iter() - .filter(|connector| connector.is_enabled) - .cloned() - .collect::>(); - if connectors.is_empty() { - return Vec::new(); - } - - let user_messages = collect_user_messages(input); - if user_messages.is_empty() && explicitly_enabled_connectors.is_empty() { - return Vec::new(); - } - - let mentions = collect_tool_mentions_from_messages(&user_messages); - let mention_names_lower = mentions - .plain_names - .iter() - .map(|name| name.to_ascii_lowercase()) - .collect::>(); - - let connector_slug_counts = build_connector_slug_counts(&connectors); - let mut allowed_connector_ids = explicitly_enabled_connectors.clone(); - for path in mentions - .paths - .iter() - .filter(|path| tool_kind_for_path(path) == ToolMentionKind::App) - { - if let Some(connector_id) = app_id_from_path(path) { - allowed_connector_ids.insert(connector_id.to_string()); - } - } - - connectors - .into_iter() - .filter(|connector| { - connector_inserted_in_messages( - connector, - &mention_names_lower, - &allowed_connector_ids, - &connector_slug_counts, - skill_name_counts_lower, - ) - }) - .collect() -} - -fn connector_inserted_in_messages( - connector: &connectors::AppInfo, - mention_names_lower: &HashSet, - allowed_connector_ids: &HashSet, - connector_slug_counts: &HashMap, - skill_name_counts_lower: &HashMap, -) -> bool { - if allowed_connector_ids.contains(&connector.id) { - return true; - } - - let mention_slug = codex_connectors::metadata::connector_mention_slug(connector); - let connector_count = connector_slug_counts - .get(&mention_slug) - .copied() - .unwrap_or(0); - let skill_count = skill_name_counts_lower - .get(&mention_slug) - .copied() - .unwrap_or(0); - connector_count == 1 && skill_count == 0 && mention_names_lower.contains(&mention_slug) -} - pub(crate) fn build_prompt( input: Vec, router: &ToolRouter, @@ -1031,19 +950,9 @@ async fn run_sampling_request( client_session: &mut ModelClientSession, turn_metadata_header: Option<&str>, input: Vec, - explicitly_enabled_connectors: &HashSet, - skills_outcome: Option<&SkillLoadOutcome>, cancellation_token: CancellationToken, ) -> CodexResult { - let router = built_tools( - sess.as_ref(), - turn_context.as_ref(), - &input, - explicitly_enabled_connectors, - skills_outcome, - &cancellation_token, - ) - .await?; + let router = built_tools(sess.as_ref(), turn_context.as_ref(), &cancellation_token).await?; let base_instructions = sess.get_base_instructions().await; @@ -1173,9 +1082,6 @@ async fn run_sampling_request( pub(crate) async fn built_tools( sess: &Session, turn_context: &TurnContext, - input: &[ResponseItem], - explicitly_enabled_connectors: &HashSet, - skills_outcome: Option<&SkillLoadOutcome>, cancellation_token: &CancellationToken, ) -> CodexResult> { let mcp_connection_manager = sess.services.mcp_connection_manager.read().await; @@ -1191,9 +1097,6 @@ pub(crate) async fn built_tools( .plugins_for_config(&turn_context.config.plugins_config_input()) .await; - let mut effective_explicitly_enabled_connectors = explicitly_enabled_connectors.clone(); - effective_explicitly_enabled_connectors.extend(sess.get_connector_selection().await); - let apps_enabled = turn_context.apps_enabled(); let accessible_connectors = apps_enabled.then(|| connectors::accessible_connectors_from_mcp_tools(&all_mcp_tools)); @@ -1245,24 +1148,9 @@ pub(crate) async fn built_tools( None }; - let explicitly_enabled = 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 - }); - - filter_connectors_for_input( - connectors, - input, - &effective_explicitly_enabled_connectors, - &skill_name_counts_lower, - ) - } else { - Vec::new() - }; let mcp_tool_exposure = build_mcp_tool_exposure( &all_mcp_tools, connectors.as_deref(), - explicitly_enabled.as_slice(), &turn_context.config, &turn_context.tools_config, ); diff --git a/codex-rs/core/src/session_startup_prewarm.rs b/codex-rs/core/src/session_startup_prewarm.rs index 0fac4b1c9..ca21e3ea7 100644 --- a/codex-rs/core/src/session_startup_prewarm.rs +++ b/codex-rs/core/src/session_startup_prewarm.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -235,9 +234,6 @@ async fn schedule_startup_prewarm_inner( let startup_router = built_tools( session.as_ref(), startup_turn_context.as_ref(), - &[], - &HashSet::new(), - /*skills_outcome*/ None, &startup_cancellation_token, ) .await?; diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index 51f100d3c..8a092d760 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -335,7 +335,7 @@ async fn search_tool_hides_apps_tools_without_search() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn explicit_app_mentions_expose_apps_tools_without_search() -> Result<()> { +async fn explicit_app_mentions_respect_always_defer() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -350,7 +350,13 @@ async fn explicit_app_mentions_expose_apps_tools_without_search() -> Result<()> ) .await; - let mut builder = configured_builder(apps_server.chatgpt_base_url.clone()); + let mut builder = + configured_builder(apps_server.chatgpt_base_url.clone()).with_config(|config| { + config + .features + .enable(Feature::ToolSearchAlwaysDeferMcpTools) + .expect("test config should allow feature update"); + }); let test = builder.build(&server).await?; test.submit_turn_with_approval_and_permission_profile( @@ -362,18 +368,22 @@ async fn explicit_app_mentions_expose_apps_tools_without_search() -> Result<()> let body = mock.single_request().body_json(); let tools = tool_names(&body); + assert!( + tools.iter().any(|name| name == TOOL_SEARCH_TOOL_NAME), + "explicit app mentions should leave app tools deferred when always-defer is active: {tools:?}" + ); assert!( namespace_child_tool( &body, SEARCH_CALENDAR_NAMESPACE, SEARCH_CALENDAR_CREATE_TOOL ) - .is_some(), - "expected explicit app mention to expose create tool, got tools: {tools:?}" + .is_none(), + "explicit app mentions should not directly expose create tool, got tools: {tools:?}" ); assert!( - namespace_child_tool(&body, SEARCH_CALENDAR_NAMESPACE, SEARCH_CALENDAR_LIST_TOOL).is_some(), - "expected explicit app mention to expose list tool, got tools: {tools:?}" + namespace_child_tool(&body, SEARCH_CALENDAR_NAMESPACE, SEARCH_CALENDAR_LIST_TOOL).is_none(), + "explicit app mentions should not directly expose list tool, got tools: {tools:?}" ); Ok(())