mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Dedupe plugin MCPs by app declaration name (#27607)
## Context
This is the next step in the plugin auth-routing stack. The earlier PRs
make `PluginsManager` auth-aware and move the broad App/MCP surface
decision into that layer. This PR narrows the ChatGPT/SIWC behavior so
we only hide a plugin MCP server when it conflicts with an App
declaration of the same name.
In product terms: if a plugin exposes both an App route and MCP route
for `foo`, ChatGPT/SIWC sessions should use the App route for `foo`. If
the same plugin also exposes a separate MCP server like `foo2`, that MCP
server should remain available.
```json
// .app.json
{
"apps": {
"foo": {
"id": "connector_abc"
}
}
}
```
```json
// .mcp.json
{
"mcpServers": {
"foo": {
"url": "https://mcp.foo.com/mcp"
},
"foo2": {
"url": "https://mcp.foo2.com/mcp"
}
}
}
```
## Stack
- PR1: #27652 seed plugin manager auth at construction.
- PR2: #27459 route plugin surfaces by auth mode.
- PR3: #27607 dedupe plugin MCP servers by App declaration name.
- PR4: #27602 preserve plugin Apps in connector listings.
- PR5: #27461 skip install-time plugin MCP OAuth for matching App
routes.
## Summary
- Preserve App declaration names in loaded plugin metadata.
- Keep public effective App outputs as deduped connector IDs for
existing callers.
- For ChatGPT/SIWC, suppress only plugin MCP servers whose names match
declared App names.
## Validation
```bash
cargo fmt --all
cargo test -p codex-core-plugins plugin_auth_projection
cargo test -p codex-core-plugins effective_apps
cargo test -p codex-core-plugins read_plugin_for_config_installed_git_source_reads_from_cache_without_cloning
cargo test -p codex-core explicit_plugin_mentions_use_apps_for_chatgpt_dual_surface_plugins
cargo test -p codex-core explicit_plugin_mentions_keep_non_conflicting_mcp_for_chatgpt_auth
cargo test -p codex-app-server --test all plugin_install_filters_disallowed_apps_needing_auth
git diff --check
```
---------
Co-authored-by: Xin Lin <xl@openai.com>
This commit is contained in:
@@ -25,6 +25,7 @@ use codex_exec_server::LOCAL_FS;
|
||||
use codex_mcp::PluginMcpServerPlacement;
|
||||
use codex_mcp::parse_plugin_mcp_config;
|
||||
use codex_plugin::AppConnectorId;
|
||||
use codex_plugin::AppDeclaration;
|
||||
use codex_plugin::LoadedPlugin;
|
||||
use codex_plugin::PluginCapabilitySummary;
|
||||
use codex_plugin::PluginHookSource;
|
||||
@@ -32,6 +33,7 @@ use codex_plugin::PluginId;
|
||||
use codex_plugin::PluginIdError;
|
||||
use codex_plugin::PluginLoadOutcome;
|
||||
use codex_plugin::PluginTelemetryMetadata;
|
||||
use codex_plugin::app_connector_ids_from_declarations;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -63,12 +65,6 @@ pub struct PluginHookLoadOutcome {
|
||||
pub hook_load_warnings: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct PluginAppMetadata {
|
||||
pub id: AppConnectorId,
|
||||
pub category: Option<String>,
|
||||
}
|
||||
|
||||
enum PluginLoadScope<'a> {
|
||||
AllCapabilities {
|
||||
restriction_product: Option<Product>,
|
||||
@@ -835,15 +831,7 @@ fn default_mcp_config_paths(plugin_root: &Path) -> Vec<AbsolutePathBuf> {
|
||||
paths
|
||||
}
|
||||
|
||||
pub async fn load_plugin_apps(plugin_root: &Path) -> Vec<AppConnectorId> {
|
||||
load_plugin_app_metadata(plugin_root)
|
||||
.await
|
||||
.into_iter()
|
||||
.map(|app| app.id)
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub async fn load_plugin_app_metadata(plugin_root: &Path) -> Vec<PluginAppMetadata> {
|
||||
pub async fn load_plugin_apps(plugin_root: &Path) -> Vec<AppDeclaration> {
|
||||
if let Some(manifest) = load_plugin_manifest(plugin_root) {
|
||||
return load_apps_from_paths(
|
||||
plugin_root,
|
||||
@@ -854,13 +842,13 @@ pub async fn load_plugin_app_metadata(plugin_root: &Path) -> Vec<PluginAppMetada
|
||||
load_apps_from_paths(plugin_root, default_app_config_paths(plugin_root)).await
|
||||
}
|
||||
|
||||
pub fn plugin_app_metadata_from_value(value: &JsonValue) -> Vec<PluginAppMetadata> {
|
||||
pub fn plugin_app_declarations_from_value(value: &JsonValue) -> Vec<AppDeclaration> {
|
||||
let Ok(parsed) = serde_json::from_value::<PluginAppFile>(value.clone()) else {
|
||||
return Vec::new();
|
||||
};
|
||||
let mut apps = plugin_app_metadata_from_file(parsed, /*plugin_root*/ None);
|
||||
let mut apps = app_declarations_from_file(parsed, /*plugin_root*/ None);
|
||||
let mut seen_connector_ids = HashSet::new();
|
||||
apps.retain(|app| seen_connector_ids.insert(app.id.0.clone()));
|
||||
apps.retain(|app| seen_connector_ids.insert(app.connector_id.0.clone()));
|
||||
apps
|
||||
}
|
||||
|
||||
@@ -1000,8 +988,8 @@ fn append_plugin_hook_file(
|
||||
async fn load_apps_from_paths(
|
||||
plugin_root: &Path,
|
||||
app_config_paths: Vec<AbsolutePathBuf>,
|
||||
) -> Vec<PluginAppMetadata> {
|
||||
let mut apps = Vec::new();
|
||||
) -> Vec<AppDeclaration> {
|
||||
let mut app_declarations = Vec::new();
|
||||
for app_config_path in app_config_paths {
|
||||
let Ok(contents) = tokio::fs::read_to_string(app_config_path.as_path()).await else {
|
||||
continue;
|
||||
@@ -1017,21 +1005,19 @@ async fn load_apps_from_paths(
|
||||
}
|
||||
};
|
||||
|
||||
apps.extend(plugin_app_metadata_from_file(parsed, Some(plugin_root)));
|
||||
app_declarations.extend(app_declarations_from_file(parsed, Some(plugin_root)));
|
||||
}
|
||||
let mut seen_connector_ids = HashSet::new();
|
||||
apps.retain(|app| seen_connector_ids.insert(app.id.0.clone()));
|
||||
apps
|
||||
app_declarations
|
||||
}
|
||||
|
||||
fn plugin_app_metadata_from_file(
|
||||
fn app_declarations_from_file(
|
||||
parsed: PluginAppFile,
|
||||
plugin_root: Option<&Path>,
|
||||
) -> Vec<PluginAppMetadata> {
|
||||
) -> Vec<AppDeclaration> {
|
||||
parsed
|
||||
.apps
|
||||
.into_values()
|
||||
.filter_map(|app| {
|
||||
.into_iter()
|
||||
.filter_map(|(name, app)| {
|
||||
if app.id.trim().is_empty() {
|
||||
if let Some(plugin_root) = plugin_root {
|
||||
warn!(
|
||||
@@ -1041,18 +1027,22 @@ fn plugin_app_metadata_from_file(
|
||||
}
|
||||
None
|
||||
} else {
|
||||
Some(PluginAppMetadata {
|
||||
id: AppConnectorId(app.id),
|
||||
category: app
|
||||
.category
|
||||
.map(|category| category.trim().to_string())
|
||||
.filter(|category| !category.is_empty()),
|
||||
Some(AppDeclaration {
|
||||
name,
|
||||
connector_id: AppConnectorId(app.id),
|
||||
category: cleaned_app_category(app.category),
|
||||
})
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn cleaned_app_category(category: Option<String>) -> Option<String> {
|
||||
category
|
||||
.map(|category| category.trim().to_string())
|
||||
.filter(|category| !category.is_empty())
|
||||
}
|
||||
|
||||
pub async fn plugin_telemetry_metadata_from_root(
|
||||
plugin_id: &PluginId,
|
||||
plugin_root: &AbsolutePathBuf,
|
||||
@@ -1075,6 +1065,13 @@ pub async fn plugin_telemetry_metadata_from_root(
|
||||
mcp_server_names.sort_unstable();
|
||||
mcp_server_names.dedup();
|
||||
|
||||
let app_declarations = load_apps_from_paths(
|
||||
plugin_root.as_path(),
|
||||
plugin_app_config_paths(plugin_root.as_path(), manifest_paths),
|
||||
)
|
||||
.await;
|
||||
let app_connector_ids = app_connector_ids_from_declarations(&app_declarations);
|
||||
|
||||
PluginTelemetryMetadata {
|
||||
plugin_id: plugin_id.clone(),
|
||||
remote_plugin_id: None,
|
||||
@@ -1084,14 +1081,7 @@ pub async fn plugin_telemetry_metadata_from_root(
|
||||
description: None,
|
||||
has_skills,
|
||||
mcp_server_names,
|
||||
app_connector_ids: load_apps_from_paths(
|
||||
plugin_root.as_path(),
|
||||
plugin_app_config_paths(plugin_root.as_path(), manifest_paths),
|
||||
)
|
||||
.await
|
||||
.into_iter()
|
||||
.map(|app| app.id)
|
||||
.collect(),
|
||||
app_connector_ids,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user