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`
This commit is contained in:
sayan-oai
2026-05-18 21:33:46 -07:00
committed by GitHub
Unverified
parent 7cdeab33d1
commit 1dd9bf9a74
10 changed files with 26 additions and 297 deletions
-4
View File
@@ -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?;
-4
View File
@@ -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?;
+1 -10
View File
@@ -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::<HashSet<_>>();
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),
}
}
+7 -76
View File
@@ -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"
)));
+1 -10
View File
@@ -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,
-2
View File
@@ -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;
-68
View File
@@ -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")];
+1 -113
View File
@@ -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<String> = 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<String>,
skill_name_counts_lower: &HashMap<String, usize>,
) -> Vec<connectors::AppInfo> {
let connectors: Vec<connectors::AppInfo> = connectors
.iter()
.filter(|connector| connector.is_enabled)
.cloned()
.collect::<Vec<_>>();
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::<HashSet<String>>();
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<String>,
allowed_connector_ids: &HashSet<String>,
connector_slug_counts: &HashMap<String, usize>,
skill_name_counts_lower: &HashMap<String, usize>,
) -> 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<ResponseItem>,
router: &ToolRouter,
@@ -1031,19 +950,9 @@ async fn run_sampling_request(
client_session: &mut ModelClientSession,
turn_metadata_header: Option<&str>,
input: Vec<ResponseItem>,
explicitly_enabled_connectors: &HashSet<String>,
skills_outcome: Option<&SkillLoadOutcome>,
cancellation_token: CancellationToken,
) -> CodexResult<SamplingRequestResult> {
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<String>,
skills_outcome: Option<&SkillLoadOutcome>,
cancellation_token: &CancellationToken,
) -> CodexResult<Arc<ToolRouter>> {
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,
);
@@ -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?;
+16 -6
View File
@@ -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(())