mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Register app tools independently of the connector list (#36900)
## What changed - Gate Codex Apps MCP tool registration on whether apps are enabled instead of requiring each tool's connector to appear in the accessible connector list. - Continue enforcing model-visibility and app-tool policy checks, including the requirement for connector metadata. - Use the merged connector catalog for tool-suggestion discovery. ## Testing - Cover app-tool registration from catalog metadata, including synthetic links, source ordering, and the apps-disabled case. GitOrigin-RevId: eef4eb03da9738d3a165dd6cfda0d3c1844d51e7
This commit is contained in:
@@ -12,23 +12,23 @@ use tracing::instrument;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::config::Config;
|
||||
use crate::connectors;
|
||||
use crate::tools::handlers::McpHandler;
|
||||
use crate::tools::registry::ToolRegistry;
|
||||
|
||||
#[instrument(level = "trace", skip_all)]
|
||||
pub(crate) fn append_mcp_tools(
|
||||
all_mcp_tools: &[McpToolInfo],
|
||||
connectors: Option<&[connectors::AppInfo]>,
|
||||
config: &Config,
|
||||
apps_enabled: bool,
|
||||
search_tool_enabled: bool,
|
||||
registry: &mut ToolRegistry,
|
||||
) -> HashSet<ToolName> {
|
||||
// Keep regular MCP tools first; Apps tools also require connector and policy checks.
|
||||
let non_app_tools = filter_non_codex_apps_mcp_tools_only(all_mcp_tools);
|
||||
let app_tools = connectors
|
||||
let app_tools = apps_enabled
|
||||
.then(|| filter_codex_apps_mcp_tools(all_mcp_tools, config))
|
||||
.into_iter()
|
||||
.flat_map(move |connectors| filter_codex_apps_mcp_tools(all_mcp_tools, connectors, config));
|
||||
.flatten();
|
||||
let exposure = if search_tool_enabled {
|
||||
ToolExposure::Deferred
|
||||
} else {
|
||||
@@ -61,13 +61,8 @@ fn filter_non_codex_apps_mcp_tools_only(
|
||||
|
||||
fn filter_codex_apps_mcp_tools<'a>(
|
||||
mcp_tools: &'a [McpToolInfo],
|
||||
connectors: &'a [connectors::AppInfo],
|
||||
config: &'a Config,
|
||||
) -> impl Iterator<Item = &'a McpToolInfo> + 'a {
|
||||
let allowed: HashSet<&str> = connectors
|
||||
.iter()
|
||||
.map(|connector| connector.id.as_str())
|
||||
.collect();
|
||||
let app_tool_policy = AppToolPolicyEvaluator::new(&config.config_layer_stack);
|
||||
|
||||
mcp_tools.iter().filter(move |tool| {
|
||||
@@ -81,18 +76,15 @@ fn filter_codex_apps_mcp_tools<'a>(
|
||||
return false;
|
||||
};
|
||||
let annotations = tool.tool.annotations.as_ref();
|
||||
allowed.contains(connector_id)
|
||||
&& app_tool_policy
|
||||
.policy(AppToolPolicyInput {
|
||||
connector_id: Some(connector_id),
|
||||
tool_name: &tool.tool.name,
|
||||
tool_title: tool.tool.title.as_deref(),
|
||||
destructive_hint: annotations
|
||||
.and_then(|annotations| annotations.destructive_hint),
|
||||
open_world_hint: annotations
|
||||
.and_then(|annotations| annotations.open_world_hint),
|
||||
})
|
||||
.enabled
|
||||
app_tool_policy
|
||||
.policy(AppToolPolicyInput {
|
||||
connector_id: Some(connector_id),
|
||||
tool_name: &tool.tool.name,
|
||||
tool_title: tool.tool.title.as_deref(),
|
||||
destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint),
|
||||
open_world_hint: annotations.and_then(|annotations| annotations.open_world_hint),
|
||||
})
|
||||
.enabled
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -15,29 +15,8 @@ use crate::config::CONFIG_TOML_FILE;
|
||||
use crate::config::Config;
|
||||
use crate::config::ConfigBuilder;
|
||||
use crate::config::test_config;
|
||||
use crate::connectors::AppInfo;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn make_connector(id: &str, name: &str) -> AppInfo {
|
||||
AppInfo {
|
||||
id: id.to_string(),
|
||||
name: name.to_string(),
|
||||
description: None,
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
icon_assets: None,
|
||||
icon_dark_assets: None,
|
||||
distribution_channel: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
install_url: None,
|
||||
is_accessible: true,
|
||||
is_enabled: true,
|
||||
plugin_display_names: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
fn make_mcp_tool(
|
||||
server_name: &str,
|
||||
tool_name: &str,
|
||||
@@ -93,15 +72,15 @@ fn expected_runtimes(
|
||||
|
||||
fn runtimes_by_name(
|
||||
tools: &[ToolInfo],
|
||||
connectors: Option<&[AppInfo]>,
|
||||
config: &Config,
|
||||
apps_enabled: bool,
|
||||
search_tool_enabled: bool,
|
||||
) -> HashMap<ToolName, ToolExposure> {
|
||||
let mut registry = ToolRegistry::default();
|
||||
append_mcp_tools(
|
||||
tools,
|
||||
connectors,
|
||||
config,
|
||||
apps_enabled,
|
||||
search_tool_enabled,
|
||||
&mut registry,
|
||||
);
|
||||
@@ -127,7 +106,7 @@ async fn directly_exposes_effective_tool_sets_when_search_is_unavailable() {
|
||||
let mcp_tools = numbered_mcp_tools(/*count*/ 2);
|
||||
|
||||
let runtimes = runtimes_by_name(
|
||||
&mcp_tools, /*connectors*/ None, &config, /*search_tool_enabled*/ false,
|
||||
&mcp_tools, &config, /*apps_enabled*/ false, /*search_tool_enabled*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
@@ -198,13 +177,8 @@ async fn excludes_tools_hidden_from_model_exposure() {
|
||||
visible_app_tool.clone(),
|
||||
hidden_app_tool,
|
||||
];
|
||||
let connectors = vec![make_connector("calendar", "Calendar")];
|
||||
|
||||
let runtimes = runtimes_by_name(
|
||||
&mcp_tools,
|
||||
Some(connectors.as_slice()),
|
||||
&config,
|
||||
/*search_tool_enabled*/ false,
|
||||
&mcp_tools, &config, /*apps_enabled*/ true, /*search_tool_enabled*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
@@ -213,6 +187,83 @@ async fn excludes_tools_hidden_from_model_exposure() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_tool_registration_uses_trusted_catalog_metadata_and_preserves_source_order() {
|
||||
let config = test_config().await;
|
||||
let app_tool = make_mcp_tool(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"calendar_list_events",
|
||||
"mcp__codex_apps__calendar",
|
||||
"list_events",
|
||||
Some("calendar"),
|
||||
Some("Calendar"),
|
||||
);
|
||||
let missing_connector_id = make_mcp_tool(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"unknown_tool",
|
||||
"mcp__codex_apps__unknown",
|
||||
"unknown",
|
||||
/*connector_id*/ None,
|
||||
/*connector_name*/ None,
|
||||
);
|
||||
let mut synthetic_app_tool = make_mcp_tool(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"gmail_batch_read_email",
|
||||
"mcp__codex_apps__gmail",
|
||||
"batch_read_email",
|
||||
Some("gmail"),
|
||||
Some("Gmail"),
|
||||
);
|
||||
synthetic_app_tool.tool.meta = Some(MetaObject(
|
||||
serde_json::json!({ "_codex_apps": { "synthetic_link": true } })
|
||||
.as_object()
|
||||
.expect("metadata should be an object")
|
||||
.clone(),
|
||||
));
|
||||
let regular_tool = make_mcp_tool(
|
||||
"rmcp",
|
||||
"regular_tool",
|
||||
"mcp__rmcp",
|
||||
"regular_tool",
|
||||
/*connector_id*/ None,
|
||||
/*connector_name*/ None,
|
||||
);
|
||||
let mcp_tools = [
|
||||
app_tool.clone(),
|
||||
missing_connector_id,
|
||||
synthetic_app_tool.clone(),
|
||||
regular_tool.clone(),
|
||||
];
|
||||
let mut registry = ToolRegistry::default();
|
||||
|
||||
append_mcp_tools(
|
||||
&mcp_tools,
|
||||
&config,
|
||||
/*apps_enabled*/ true,
|
||||
/*search_tool_enabled*/ false,
|
||||
&mut registry,
|
||||
);
|
||||
|
||||
let registered_names = registry
|
||||
.entries()
|
||||
.map(|entry| entry.runtime.tool_name())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
registered_names,
|
||||
vec![
|
||||
regular_tool.canonical_tool_name(),
|
||||
app_tool.canonical_tool_name(),
|
||||
synthetic_app_tool.canonical_tool_name(),
|
||||
]
|
||||
);
|
||||
assert_eq!(
|
||||
runtimes_by_name(
|
||||
&mcp_tools, &config, /*apps_enabled*/ false, /*search_tool_enabled*/ false,
|
||||
),
|
||||
expected_runtimes(&[regular_tool], ToolExposure::Direct)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn applies_per_tool_app_policy_across_the_exposure_build() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
@@ -248,14 +299,9 @@ enabled = true
|
||||
Some("calendar"),
|
||||
Some("Calendar"),
|
||||
);
|
||||
let connectors = vec![make_connector("calendar", "Calendar")];
|
||||
|
||||
let mcp_tools = [enabled_tool.clone(), disabled_tool];
|
||||
let runtimes = runtimes_by_name(
|
||||
&mcp_tools,
|
||||
Some(connectors.as_slice()),
|
||||
&config,
|
||||
/*search_tool_enabled*/ false,
|
||||
&mcp_tools, &config, /*apps_enabled*/ true, /*search_tool_enabled*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
@@ -270,7 +316,7 @@ async fn defers_effective_tool_sets_when_search_is_available() {
|
||||
let mcp_tools = numbered_mcp_tools(/*count*/ 2);
|
||||
|
||||
let runtimes = runtimes_by_name(
|
||||
&mcp_tools, /*connectors*/ None, &config, /*search_tool_enabled*/ true,
|
||||
&mcp_tools, &config, /*apps_enabled*/ false, /*search_tool_enabled*/ true,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
@@ -300,13 +346,8 @@ async fn defers_apps_and_non_app_mcp_tools() {
|
||||
Some("Calendar"),
|
||||
),
|
||||
];
|
||||
let connectors = vec![make_connector("calendar", "Calendar")];
|
||||
|
||||
let runtimes = runtimes_by_name(
|
||||
&mcp_tools,
|
||||
Some(connectors.as_slice()),
|
||||
&config,
|
||||
/*search_tool_enabled*/ true,
|
||||
&mcp_tools, &config, /*apps_enabled*/ true, /*search_tool_enabled*/ true,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
|
||||
@@ -1476,10 +1476,6 @@ pub(crate) async fn built_tools(
|
||||
let apps_enabled = turn_context.apps_enabled();
|
||||
let accessible_connectors =
|
||||
apps_enabled.then(|| connectors::accessible_connectors_from_mcp_tools(all_mcp_tools));
|
||||
let accessible_connectors_with_enabled_state =
|
||||
accessible_connectors.as_ref().map(|connectors| {
|
||||
connectors::with_app_enabled_state(connectors.clone(), &turn_context.config)
|
||||
});
|
||||
let connectors = if apps_enabled {
|
||||
let connectors = codex_connectors::merge::merge_plugin_connectors_with_accessible(
|
||||
connector_snapshot
|
||||
@@ -1514,9 +1510,7 @@ pub(crate) async fn built_tools(
|
||||
.collect::<Vec<_>>();
|
||||
async {
|
||||
if apps_enabled && tool_suggest_is_enabled {
|
||||
if let Some(accessible_connectors) =
|
||||
accessible_connectors_with_enabled_state.as_ref()
|
||||
{
|
||||
if let Some(accessible_connectors) = connectors.as_ref() {
|
||||
match connectors::list_tool_suggest_discoverable_tools_with_auth(
|
||||
&turn_context.config,
|
||||
sess.services.plugins_manager.as_ref(),
|
||||
@@ -1556,7 +1550,7 @@ pub(crate) async fn built_tools(
|
||||
turn_context,
|
||||
environments,
|
||||
mcp,
|
||||
connectors.as_deref(),
|
||||
apps_enabled,
|
||||
step_store,
|
||||
tool_suggest_candidates.as_ref(),
|
||||
))
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
use crate::agent::exceeds_thread_spawn_depth_limit;
|
||||
use crate::agent::next_thread_spawn_depth;
|
||||
use crate::connectors::AppInfo;
|
||||
use crate::environment_selection::TurnEnvironmentSnapshot;
|
||||
use crate::mcp_tool_exposure::append_mcp_tools;
|
||||
use crate::session::session::Session;
|
||||
@@ -119,7 +118,7 @@ pub(crate) fn build_tool_router(
|
||||
turn_context: &TurnContext,
|
||||
environments: &TurnEnvironmentSnapshot,
|
||||
mcp: &codex_mcp::McpBinding,
|
||||
connectors: Option<&[AppInfo]>,
|
||||
apps_enabled: bool,
|
||||
step_store: &ExtensionData,
|
||||
tool_suggest_candidates: Option<&crate::tools::router::ToolSuggestCandidates>,
|
||||
) -> ToolRouter {
|
||||
@@ -147,8 +146,8 @@ pub(crate) fn build_tool_router(
|
||||
} else {
|
||||
let registered_mcp_tools = append_mcp_tools(
|
||||
mcp.tools(),
|
||||
connectors,
|
||||
&turn_context.config,
|
||||
apps_enabled,
|
||||
search_tool_enabled(turn_context),
|
||||
&mut registry,
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user