mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Support object-valued plugin MCP manifests (#28580)
## Summary
This fixes plugin manifest parsing for MCP servers declared as an object
directly in `plugin.json`.
Before this change, Codex modeled `mcpServers` as only a string path,
for example:
```json
{
"name": "counter-sample",
"version": "1.1.1",
"mcpServers": "./.mcp.json"
}
```
Some migrated plugins instead provide the server map directly in the
manifest:
```json
{
"name": "counter-sample",
"version": "1.1.1",
"description": "Plugin that declares MCP servers in the manifest",
"mcpServers": {
"counter": {
"type": "http",
"url": "https://sample.example/counter/mcp"
}
}
}
```
That object form previously failed during install/load with an error
like:
```text
failed to parse plugin manifest: invalid type: map, expected a string
```
## What changed
- Add a manifest representation for `mcpServers` as either
`Path(Resource)` or `Object(map)`.
- Parse `plugin.json` `mcpServers` as either a string path or an object.
- Route object-valued MCP server maps through the existing plugin MCP
config parser instead of adding a second parser.
- Apply existing per-plugin MCP server policy to object-valued MCP
servers the same way as file-backed MCP servers.
- Include object-valued MCP server names in plugin telemetry/capability
metadata.
- Support object-valued MCP config for executor plugins without
requiring a `.mcp.json` filesystem read.
- Update the bundled plugin-creator validator and `plugin-json-spec.md`
so generated-plugin validation accepts the same object-valued shape.
## Compatibility
Existing plugin manifests that use `"mcpServers": "./.mcp.json"`
continue to work. Plugins can now also use the object shape shown above.
## Tests
Added coverage for the new manifest attribute shape at the install,
normal load, telemetry, and executor-provider layers:
- `install_accepts_manifest_mcp_server_objects`
- `load_plugins_loads_manifest_mcp_server_objects`
- `plugin_telemetry_metadata_uses_manifest_mcp_server_objects`
- `reads_manifest_object_config_without_executor_file_system_access`
Also smoke-tested the plugin-creator validator against both supported
forms:
- `mcpServers` as a direct object in `plugin.json`
- `mcpServers` as `"./.mcp.json"` with a companion `.mcp.json`
## Validation
- `just test -p codex-plugin`
- `just test -p codex-core-plugins`
- `just test -p codex-mcp-extension`
- `just bazel-lock-update`
- `just bazel-lock-check`
- `just fmt`
- `git diff --check`
- Focused rename/object-form rerun: `just test -p codex-core-plugins
manager::tests::load_plugins_loads_manifest_mcp_server_objects
manager::tests::plugin_telemetry_metadata_uses_manifest_mcp_server_objects
store::tests::install_accepts_manifest_mcp_server_objects`
- Focused executor rerun: `just test -p codex-mcp-extension
executor_plugin::provider::tests::reads_manifest_object_config_without_executor_file_system_access`
- `python3
codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py
/private/tmp/codex-validator-object`
- `python3
codex-rs/skills/src/assets/samples/plugin-creator/scripts/validate_plugin.py
/private/tmp/codex-validator-path`
This commit is contained in:
committed by
GitHub
Unverified
parent
6f77491e95
commit
1883dedc0e
@@ -2,6 +2,7 @@ use crate::app_mcp_routing::apply_app_mcp_routing_policy;
|
||||
use crate::app_mcp_routing::apps_route_available;
|
||||
use crate::is_openai_curated_marketplace_name;
|
||||
use crate::manifest::PluginManifestHooks;
|
||||
use crate::manifest::PluginManifestMcpServers;
|
||||
use crate::manifest::PluginManifestPaths;
|
||||
use crate::manifest::load_plugin_manifest;
|
||||
use crate::marketplace::MarketplacePluginSource;
|
||||
@@ -724,25 +725,12 @@ async fn load_plugin(
|
||||
let has_enabled_skills = resolved_skills.has_enabled_skills();
|
||||
loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths;
|
||||
loaded_plugin.has_enabled_skills = has_enabled_skills;
|
||||
let mut mcp_servers = HashMap::new();
|
||||
for mcp_config_path in plugin_mcp_config_paths(plugin_root.as_path(), manifest_paths) {
|
||||
let plugin_mcp =
|
||||
load_mcp_servers_from_file(plugin_root.as_path(), &mcp_config_path).await;
|
||||
for (name, mut config) in plugin_mcp.mcp_servers {
|
||||
if let Some(policy) = plugin.mcp_servers.get(&name) {
|
||||
apply_plugin_mcp_server_policy(&mut config, policy);
|
||||
}
|
||||
if mcp_servers.insert(name.clone(), config).is_some() {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
path = %mcp_config_path.display(),
|
||||
server = name,
|
||||
"plugin MCP file overwrote an earlier server definition"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
loaded_plugin.mcp_servers = mcp_servers;
|
||||
loaded_plugin.mcp_servers = load_plugin_mcp_servers_from_manifest(
|
||||
plugin_root.as_path(),
|
||||
manifest_paths,
|
||||
Some(&plugin.mcp_servers),
|
||||
)
|
||||
.await;
|
||||
loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await;
|
||||
}
|
||||
PluginLoadScope::HooksOnly => {}
|
||||
@@ -853,7 +841,7 @@ fn plugin_mcp_config_paths(
|
||||
plugin_root: &Path,
|
||||
manifest_paths: &PluginManifestPaths,
|
||||
) -> Vec<AbsolutePathBuf> {
|
||||
if let Some(path) = &manifest_paths.mcp_servers {
|
||||
if let Some(PluginManifestMcpServers::Path(path)) = &manifest_paths.mcp_servers {
|
||||
return vec![path.clone()];
|
||||
}
|
||||
default_mcp_config_paths(plugin_root)
|
||||
@@ -1094,15 +1082,14 @@ pub async fn plugin_telemetry_metadata_from_root(
|
||||
|
||||
let manifest_paths = &manifest.paths;
|
||||
let has_skills = !plugin_skill_roots(plugin_root, manifest_paths).is_empty();
|
||||
let mut mcp_server_names = Vec::new();
|
||||
for path in plugin_mcp_config_paths(plugin_root.as_path(), manifest_paths) {
|
||||
mcp_server_names.extend(
|
||||
load_mcp_servers_from_file(plugin_root.as_path(), &path)
|
||||
.await
|
||||
.mcp_servers
|
||||
.into_keys(),
|
||||
);
|
||||
}
|
||||
let mut mcp_server_names = load_plugin_mcp_servers_from_manifest(
|
||||
plugin_root.as_path(),
|
||||
manifest_paths,
|
||||
/*plugin_policy*/ None,
|
||||
)
|
||||
.await
|
||||
.into_keys()
|
||||
.collect::<Vec<_>>();
|
||||
mcp_server_names.sort_unstable();
|
||||
mcp_server_names.dedup();
|
||||
|
||||
@@ -1151,11 +1138,49 @@ async fn load_declared_plugin_mcp_servers(plugin_root: &Path) -> HashMap<String,
|
||||
return HashMap::new();
|
||||
};
|
||||
|
||||
load_plugin_mcp_servers_from_manifest(plugin_root, &manifest.paths, /*plugin_policy*/ None)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn load_plugin_mcp_servers_from_manifest(
|
||||
plugin_root: &Path,
|
||||
manifest_paths: &PluginManifestPaths,
|
||||
plugin_policy: Option<&HashMap<String, PluginMcpServerConfig>>,
|
||||
) -> HashMap<String, McpServerConfig> {
|
||||
let mut mcp_servers = HashMap::new();
|
||||
for mcp_config_path in plugin_mcp_config_paths(plugin_root, &manifest.paths) {
|
||||
let plugin_mcp = load_mcp_servers_from_file(plugin_root, &mcp_config_path).await;
|
||||
for (name, config) in plugin_mcp.mcp_servers {
|
||||
mcp_servers.entry(name).or_insert(config);
|
||||
match &manifest_paths.mcp_servers {
|
||||
Some(PluginManifestMcpServers::Object(object_servers)) => {
|
||||
let plugin_mcp = load_mcp_servers_from_manifest_object(plugin_root, object_servers);
|
||||
for (name, mut config) in plugin_mcp.mcp_servers {
|
||||
if let Some(policy) = plugin_policy.and_then(|policy| policy.get(&name)) {
|
||||
apply_plugin_mcp_server_policy(&mut config, policy);
|
||||
}
|
||||
if mcp_servers.insert(name.clone(), config).is_some() {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
server = name,
|
||||
"plugin manifest MCP object overwrote an earlier server definition"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Some(PluginManifestMcpServers::Path(_)) | None => {
|
||||
for mcp_config_path in plugin_mcp_config_paths(plugin_root, manifest_paths) {
|
||||
let plugin_mcp = load_mcp_servers_from_file(plugin_root, &mcp_config_path).await;
|
||||
for (name, mut config) in plugin_mcp.mcp_servers {
|
||||
if let Some(policy) = plugin_policy.and_then(|policy| policy.get(&name)) {
|
||||
apply_plugin_mcp_server_policy(&mut config, policy);
|
||||
}
|
||||
if mcp_servers.insert(name.clone(), config).is_some() {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
path = %mcp_config_path.display(),
|
||||
server = name,
|
||||
"plugin MCP file overwrote an earlier server definition"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1212,6 +1237,37 @@ async fn load_mcp_servers_from_file(
|
||||
}
|
||||
}
|
||||
|
||||
fn load_mcp_servers_from_manifest_object(
|
||||
plugin_root: &Path,
|
||||
object_config: &str,
|
||||
) -> PluginMcpDiscovery {
|
||||
let parsed = match parse_plugin_mcp_config(
|
||||
plugin_root,
|
||||
object_config,
|
||||
PluginMcpServerPlacement::Declared,
|
||||
) {
|
||||
Ok(parsed) => parsed,
|
||||
Err(err) => {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
"failed to parse plugin manifest MCP object: {err}"
|
||||
);
|
||||
return PluginMcpDiscovery::default();
|
||||
}
|
||||
};
|
||||
for error in parsed.errors {
|
||||
warn!(
|
||||
plugin = %plugin_root.display(),
|
||||
server = error.name,
|
||||
error = error.message,
|
||||
"failed to parse plugin manifest MCP object server"
|
||||
);
|
||||
}
|
||||
PluginMcpDiscovery {
|
||||
mcp_servers: parsed.servers.into_iter().collect(),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
struct PluginMcpDiscovery {
|
||||
mcp_servers: HashMap<String, McpServerConfig>,
|
||||
|
||||
@@ -682,6 +682,70 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn load_plugins_loads_manifest_mcp_server_objects() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
let plugin_root = codex_home
|
||||
.path()
|
||||
.join("plugins/cache")
|
||||
.join("test/counter-sample/local");
|
||||
|
||||
write_file(
|
||||
&plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{
|
||||
"name": "counter-sample",
|
||||
"version": "1.1.1",
|
||||
"description": "Plugin that declares MCP servers in the manifest",
|
||||
"mcpServers": {
|
||||
"counter": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/counter/mcp"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
);
|
||||
|
||||
let config_toml = r#"
|
||||
[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."counter-sample@test"]
|
||||
enabled = true
|
||||
"#;
|
||||
let outcome =
|
||||
load_plugins_from_config(config_toml, codex_home.path(), /*auth_mode*/ None).await;
|
||||
|
||||
assert_eq!(outcome.plugins()[0].error, None);
|
||||
assert_eq!(
|
||||
outcome.plugins()[0].mcp_servers,
|
||||
HashMap::from([(
|
||||
"counter".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::StreamableHttp {
|
||||
url: "https://sample.example/counter/mcp".to_string(),
|
||||
bearer_token_env_var: None,
|
||||
http_headers: None,
|
||||
env_http_headers: None,
|
||||
},
|
||||
environment_id: "local".to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)])
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn load_plugins_applies_plugin_mcp_server_policy() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
@@ -1098,6 +1162,47 @@ async fn plugin_telemetry_metadata_uses_default_mcp_config_path() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plugin_telemetry_metadata_uses_manifest_mcp_server_objects() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
let plugin_root = codex_home
|
||||
.path()
|
||||
.join("plugins/cache")
|
||||
.join("test/counter-sample/local");
|
||||
|
||||
write_file(
|
||||
&plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{
|
||||
"name": "counter-sample",
|
||||
"version": "1.1.1",
|
||||
"mcpServers": {
|
||||
"counter": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/counter/mcp"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
);
|
||||
|
||||
let metadata = plugin_telemetry_metadata_from_root(
|
||||
&PluginId::parse("counter-sample@test").expect("plugin id should parse"),
|
||||
&plugin_root.abs(),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
metadata.capability_summary,
|
||||
Some(PluginCapabilitySummary {
|
||||
config_name: "counter-sample@test".to_string(),
|
||||
display_name: "counter-sample".to_string(),
|
||||
description: None,
|
||||
has_skills: false,
|
||||
mcp_server_names: vec!["counter".to_string()],
|
||||
app_connector_ids: Vec::new(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn capability_summary_sanitizes_plugin_descriptions_to_one_line() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
|
||||
@@ -12,6 +12,8 @@ const MAX_DEFAULT_PROMPT_LEN: usize = 128;
|
||||
pub type PluginManifest = codex_plugin::manifest::PluginManifest<AbsolutePathBuf>;
|
||||
pub type PluginManifestHooks = codex_plugin::manifest::PluginManifestHooks<AbsolutePathBuf>;
|
||||
pub type PluginManifestInterface = codex_plugin::manifest::PluginManifestInterface<AbsolutePathBuf>;
|
||||
pub type PluginManifestMcpServers =
|
||||
codex_plugin::manifest::PluginManifestMcpServers<AbsolutePathBuf>;
|
||||
pub type PluginManifestPaths = codex_plugin::manifest::PluginManifestPaths<AbsolutePathBuf>;
|
||||
|
||||
#[derive(Debug, Default, Deserialize)]
|
||||
@@ -30,7 +32,7 @@ struct RawPluginManifest {
|
||||
#[serde(default)]
|
||||
skills: Option<RawPluginManifestPath>,
|
||||
#[serde(default)]
|
||||
mcp_servers: Option<String>,
|
||||
mcp_servers: Option<RawPluginManifestMcpServers>,
|
||||
#[serde(default)]
|
||||
apps: Option<String>,
|
||||
#[serde(default)]
|
||||
@@ -97,6 +99,14 @@ enum RawPluginManifestPath {
|
||||
Invalid(JsonValue),
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
#[serde(untagged)]
|
||||
enum RawPluginManifestMcpServers {
|
||||
Path(String),
|
||||
Object(std::collections::BTreeMap<String, JsonValue>),
|
||||
Invalid(JsonValue),
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
#[serde(untagged)]
|
||||
enum RawPluginManifestHooks {
|
||||
@@ -221,7 +231,7 @@ pub(crate) fn parse_plugin_manifest(
|
||||
keywords,
|
||||
paths: PluginManifestPaths {
|
||||
skills: resolve_manifest_path_value(plugin_root, "skills", skills.as_ref()),
|
||||
mcp_servers: resolve_manifest_path(plugin_root, "mcpServers", mcp_servers.as_deref()),
|
||||
mcp_servers: resolve_manifest_mcp_servers(plugin_root, mcp_servers),
|
||||
apps: resolve_manifest_path(plugin_root, "apps", apps.as_deref()),
|
||||
hooks: resolve_manifest_hooks(plugin_root, hooks),
|
||||
},
|
||||
@@ -259,6 +269,32 @@ fn resolve_manifest_hooks(
|
||||
}
|
||||
}
|
||||
|
||||
fn resolve_manifest_mcp_servers(
|
||||
plugin_root: &Path,
|
||||
mcp_servers: Option<RawPluginManifestMcpServers>,
|
||||
) -> Option<PluginManifestMcpServers> {
|
||||
match mcp_servers? {
|
||||
RawPluginManifestMcpServers::Path(path) => {
|
||||
resolve_manifest_path(plugin_root, "mcpServers", Some(&path))
|
||||
.map(PluginManifestMcpServers::Path)
|
||||
}
|
||||
RawPluginManifestMcpServers::Object(servers) => match serde_json::to_string(&servers) {
|
||||
Ok(servers) => Some(PluginManifestMcpServers::Object(servers)),
|
||||
Err(err) => {
|
||||
tracing::warn!("ignoring mcpServers: failed to serialize object: {err}");
|
||||
None
|
||||
}
|
||||
},
|
||||
RawPluginManifestMcpServers::Invalid(value) => {
|
||||
tracing::warn!(
|
||||
"ignoring mcpServers: expected a string or object; found {}",
|
||||
json_value_type(&value)
|
||||
);
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn resolve_interface_asset_path(
|
||||
plugin_root: &Path,
|
||||
field: &'static str,
|
||||
|
||||
@@ -71,6 +71,46 @@ fn install_copies_plugin_into_default_marketplace() {
|
||||
assert!(installed_path.join("skills/SKILL.md").is_file());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn install_accepts_manifest_mcp_server_objects() {
|
||||
let tmp = tempdir().unwrap();
|
||||
let plugin_root = tmp.path().join("counter-sample");
|
||||
fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap();
|
||||
fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{
|
||||
"name": "counter-sample",
|
||||
"version": "1.1.1",
|
||||
"mcpServers": {
|
||||
"counter": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/counter/mcp"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
let plugin_id = PluginId::new("counter-sample".to_string(), "debug".to_string()).unwrap();
|
||||
|
||||
let result = PluginStore::new(tmp.path().to_path_buf())
|
||||
.install(
|
||||
AbsolutePathBuf::try_from(plugin_root).unwrap(),
|
||||
plugin_id.clone(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let installed_path = tmp.path().join("plugins/cache/debug/counter-sample/1.1.1");
|
||||
assert_eq!(
|
||||
result,
|
||||
PluginInstallResult {
|
||||
plugin_id,
|
||||
plugin_version: "1.1.1".to_string(),
|
||||
installed_path: AbsolutePathBuf::try_from(installed_path.clone()).unwrap(),
|
||||
}
|
||||
);
|
||||
assert!(installed_path.join(".codex-plugin/plugin.json").is_file());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn install_uses_manifest_name_for_destination_and_key() {
|
||||
let tmp = tempdir().unwrap();
|
||||
|
||||
@@ -7,6 +7,7 @@ use codex_mcp::parse_plugin_mcp_config;
|
||||
use codex_plugin::PluginResourceLocator;
|
||||
use codex_plugin::ResolvedPlugin;
|
||||
use codex_plugin::ResolvedPluginLocation;
|
||||
use codex_plugin::manifest::PluginManifestMcpServers;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::io;
|
||||
@@ -56,26 +57,47 @@ async fn load_from_file_system(
|
||||
) -> Result<Vec<(String, McpServerConfig)>, ExecutorPluginMcpProviderError> {
|
||||
let ResolvedPluginLocation::Environment { environment_id, .. } = plugin.location();
|
||||
let plugin_id = plugin.selected_root_id();
|
||||
let (config_path, is_default) = match plugin.manifest().paths.mcp_servers.as_ref() {
|
||||
Some(PluginResourceLocator::Environment { path, .. }) => (path.clone(), false),
|
||||
None => (plugin_root.join(DEFAULT_MCP_CONFIG_FILE), true),
|
||||
};
|
||||
let config_uri = PathUri::from_abs_path(&config_path);
|
||||
|
||||
let contents = match file_system
|
||||
.read_file_text(&config_uri, /*sandbox*/ None)
|
||||
.await
|
||||
{
|
||||
Ok(contents) => contents,
|
||||
Err(source) if is_default && source.kind() == io::ErrorKind::NotFound => {
|
||||
return Ok(Vec::new());
|
||||
let (contents, config_path) = match plugin.manifest().paths.mcp_servers.as_ref() {
|
||||
Some(PluginManifestMcpServers::Path(PluginResourceLocator::Environment {
|
||||
path, ..
|
||||
})) => {
|
||||
let config_uri = PathUri::from_abs_path(path);
|
||||
(
|
||||
file_system
|
||||
.read_file_text(&config_uri, /*sandbox*/ None)
|
||||
.await
|
||||
.map_err(|source| ExecutorPluginMcpProviderError::ReadConfig {
|
||||
plugin_id: plugin_id.to_string(),
|
||||
path: path.clone(),
|
||||
source,
|
||||
})?,
|
||||
path.clone(),
|
||||
)
|
||||
}
|
||||
Err(source) => {
|
||||
return Err(ExecutorPluginMcpProviderError::ReadConfig {
|
||||
plugin_id: plugin_id.to_string(),
|
||||
path: config_path.clone(),
|
||||
source,
|
||||
});
|
||||
Some(PluginManifestMcpServers::Object(object_config)) => (
|
||||
object_config.clone(),
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
),
|
||||
None => {
|
||||
let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE);
|
||||
let config_uri = PathUri::from_abs_path(&config_path);
|
||||
let contents = match file_system
|
||||
.read_file_text(&config_uri, /*sandbox*/ None)
|
||||
.await
|
||||
{
|
||||
Ok(contents) => contents,
|
||||
Err(source) if source.kind() == io::ErrorKind::NotFound => {
|
||||
return Ok(Vec::new());
|
||||
}
|
||||
Err(source) => {
|
||||
return Err(ExecutorPluginMcpProviderError::ReadConfig {
|
||||
plugin_id: plugin_id.to_string(),
|
||||
path: config_path.clone(),
|
||||
source,
|
||||
});
|
||||
}
|
||||
};
|
||||
(contents, config_path)
|
||||
}
|
||||
};
|
||||
let parsed = parse_plugin_mcp_config(
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_exec_server::ReadDirectoryEntry;
|
||||
use codex_exec_server::RemoveOptions;
|
||||
use codex_plugin::ResolvedPlugin;
|
||||
use codex_plugin::manifest::PluginManifest;
|
||||
use codex_plugin::manifest::PluginManifestMcpServers;
|
||||
use codex_plugin::manifest::PluginManifestPaths;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -144,7 +145,10 @@ async fn reads_declared_config_only_through_executor_file_system() {
|
||||
.expect("absolute plugin root");
|
||||
assert!(!plugin_root.as_path().exists());
|
||||
let config_path = plugin_root.join("config/mcp.json");
|
||||
let plugin = resolved_plugin(&plugin_root, Some(config_path.clone()));
|
||||
let plugin = resolved_plugin(
|
||||
&plugin_root,
|
||||
Some(PluginManifestMcpServers::Path(config_path.clone())),
|
||||
);
|
||||
let file_system = SyntheticExecutorFileSystem {
|
||||
config_path: config_path.clone(),
|
||||
config_contents: Some(MCP_CONFIG_CONTENTS),
|
||||
@@ -187,6 +191,60 @@ async fn reads_declared_config_only_through_executor_file_system() {
|
||||
assert_eq!(reads(&file_system), vec![config_path]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reads_manifest_object_config_without_executor_file_system_access() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let plugin_root = AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("plugin"))
|
||||
.expect("absolute plugin root");
|
||||
let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE);
|
||||
let plugin = resolved_plugin(
|
||||
&plugin_root,
|
||||
Some(PluginManifestMcpServers::Object(
|
||||
r#"{"counter":{"command":"counter-mcp","environment_id":"local"}}"#.to_string(),
|
||||
)),
|
||||
);
|
||||
let file_system = SyntheticExecutorFileSystem {
|
||||
config_path,
|
||||
config_contents: None,
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let servers = load_from_file_system(&plugin, &plugin_root, &file_system)
|
||||
.await
|
||||
.expect("load manifest object executor MCP config");
|
||||
|
||||
assert_eq!(
|
||||
servers,
|
||||
vec![(
|
||||
"counter".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: "counter-mcp".to_string(),
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
env_vars: Vec::new(),
|
||||
cwd: Some(plugin_root.to_path_buf()),
|
||||
},
|
||||
environment_id: "executor-test".to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]
|
||||
);
|
||||
assert_eq!(reads(&file_system), Vec::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn missing_default_config_is_empty() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
@@ -214,7 +272,10 @@ async fn malformed_declared_config_is_an_error() {
|
||||
let plugin_root = AbsolutePathBuf::from_absolute_path_checked(temp_dir.path().join("plugin"))
|
||||
.expect("absolute plugin root");
|
||||
let config_path = plugin_root.join("mcp.json");
|
||||
let plugin = resolved_plugin(&plugin_root, Some(config_path.clone()));
|
||||
let plugin = resolved_plugin(
|
||||
&plugin_root,
|
||||
Some(PluginManifestMcpServers::Path(config_path.clone())),
|
||||
);
|
||||
let file_system = SyntheticExecutorFileSystem {
|
||||
config_path: config_path.clone(),
|
||||
config_contents: Some("{not-json"),
|
||||
@@ -242,7 +303,7 @@ async fn malformed_declared_config_is_an_error() {
|
||||
|
||||
fn resolved_plugin(
|
||||
plugin_root: &AbsolutePathBuf,
|
||||
mcp_servers: Option<AbsolutePathBuf>,
|
||||
mcp_servers: Option<PluginManifestMcpServers<AbsolutePathBuf>>,
|
||||
) -> ResolvedPlugin {
|
||||
ResolvedPlugin::from_environment(
|
||||
"selected-root".to_string(),
|
||||
|
||||
@@ -18,11 +18,18 @@ pub struct PluginManifest<Resource> {
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct PluginManifestPaths<Resource> {
|
||||
pub skills: Option<Resource>,
|
||||
pub mcp_servers: Option<Resource>,
|
||||
pub mcp_servers: Option<PluginManifestMcpServers<Resource>>,
|
||||
pub apps: Option<Resource>,
|
||||
pub hooks: Option<PluginManifestHooks<Resource>>,
|
||||
}
|
||||
|
||||
/// MCP server declarations embedded in or referenced by a plugin manifest.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum PluginManifestMcpServers<Resource> {
|
||||
Path(Resource),
|
||||
Object(String),
|
||||
}
|
||||
|
||||
/// Hook declarations embedded in or referenced by a plugin manifest.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum PluginManifestHooks<Resource> {
|
||||
@@ -109,6 +116,15 @@ impl<Resource> PluginManifest<Resource> {
|
||||
Some(PluginManifestHooks::Inline(hooks)) => Some(PluginManifestHooks::Inline(hooks)),
|
||||
None => None,
|
||||
};
|
||||
let mcp_servers = match mcp_servers {
|
||||
Some(PluginManifestMcpServers::Path(path)) => {
|
||||
Some(PluginManifestMcpServers::Path(map(path)?))
|
||||
}
|
||||
Some(PluginManifestMcpServers::Object(servers)) => {
|
||||
Some(PluginManifestMcpServers::Object(servers))
|
||||
}
|
||||
None => None,
|
||||
};
|
||||
let interface = match interface {
|
||||
Some(interface) => {
|
||||
let PluginManifestInterface {
|
||||
@@ -157,7 +173,7 @@ impl<Resource> PluginManifest<Resource> {
|
||||
keywords,
|
||||
paths: PluginManifestPaths {
|
||||
skills: skills.map(&mut map).transpose()?,
|
||||
mcp_servers: mcp_servers.map(&mut map).transpose()?,
|
||||
mcp_servers,
|
||||
apps: apps.map(&mut map).transpose()?,
|
||||
hooks,
|
||||
},
|
||||
|
||||
@@ -4,6 +4,7 @@ use super::ResolvedPluginError;
|
||||
use crate::manifest::PluginManifest;
|
||||
use crate::manifest::PluginManifestHooks;
|
||||
use crate::manifest::PluginManifestInterface;
|
||||
use crate::manifest::PluginManifestMcpServers;
|
||||
use crate::manifest::PluginManifestPaths;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -37,7 +38,7 @@ fn environment_descriptor_binds_every_manifest_resource() {
|
||||
keywords: Vec::new(),
|
||||
paths: PluginManifestPaths {
|
||||
skills: Some(skills.clone()),
|
||||
mcp_servers: Some(mcp_servers.clone()),
|
||||
mcp_servers: Some(PluginManifestMcpServers::Path(mcp_servers.clone())),
|
||||
apps: Some(apps.clone()),
|
||||
hooks: Some(PluginManifestHooks::Paths(vec![hooks.clone()])),
|
||||
},
|
||||
@@ -71,7 +72,10 @@ fn environment_descriptor_binds_every_manifest_resource() {
|
||||
keywords: Vec::new(),
|
||||
paths: PluginManifestPaths {
|
||||
skills: Some(resource("executor-1", skills)),
|
||||
mcp_servers: Some(resource("executor-1", mcp_servers)),
|
||||
mcp_servers: Some(PluginManifestMcpServers::Path(resource(
|
||||
"executor-1",
|
||||
mcp_servers,
|
||||
))),
|
||||
apps: Some(resource("executor-1", apps)),
|
||||
hooks: Some(PluginManifestHooks::Paths(vec![resource(
|
||||
"executor-1",
|
||||
@@ -100,7 +104,7 @@ fn environment_descriptor_rejects_resources_outside_package_root() {
|
||||
keywords: Vec::new(),
|
||||
paths: PluginManifestPaths {
|
||||
skills: None,
|
||||
mcp_servers: Some(outside.clone()),
|
||||
mcp_servers: Some(PluginManifestMcpServers::Path(outside.clone())),
|
||||
apps: None,
|
||||
hooks: None,
|
||||
},
|
||||
|
||||
@@ -62,10 +62,31 @@
|
||||
- `keywords` (`array` of `string`): Search/discovery tags.
|
||||
- `skills` (`string`): Relative path to skill directories/files.
|
||||
- `hooks` (`string`): Hook config path.
|
||||
- `mcpServers` (`string`): MCP config path.
|
||||
- `mcpServers` (`string` or `object`): MCP config path, or an object whose keys are MCP server names and whose values are MCP server config objects.
|
||||
- `apps` (`string`): App manifest path for plugin integrations.
|
||||
- `interface` (`object`): Interface/UX metadata block for plugin presentation.
|
||||
|
||||
`mcpServers` may be declared as a companion file path:
|
||||
|
||||
```json
|
||||
{
|
||||
"mcpServers": "./.mcp.json"
|
||||
}
|
||||
```
|
||||
|
||||
Or as an object directly in `plugin.json`:
|
||||
|
||||
```json
|
||||
{
|
||||
"mcpServers": {
|
||||
"counter": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/counter/mcp"
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### `interface` fields
|
||||
|
||||
- `displayName` (`string`): User-facing title shown for the plugin.
|
||||
@@ -91,7 +112,7 @@
|
||||
### Path conventions and defaults
|
||||
|
||||
- Path values should be relative and begin with `./`.
|
||||
- `skills`, `hooks`, and `mcpServers` are supplemented on top of default component discovery; they do not replace defaults.
|
||||
- `skills`, `hooks`, and string-valued `mcpServers` are supplemented on top of default component discovery; they do not replace defaults.
|
||||
- Custom path values must follow the plugin root convention and naming/namespacing rules.
|
||||
- This repo’s scaffold writes `.codex-plugin/plugin.json`; treat that as the manifest location this skill generates.
|
||||
|
||||
@@ -186,8 +207,9 @@ personal marketplace unless the caller explicitly requests a repo-local destinat
|
||||
present.
|
||||
- `composerIcon`, `logo`, and `screenshots` must point to real files inside the plugin archive when
|
||||
present.
|
||||
- `apps` and `mcpServers` should appear in `plugin.json` only when `.app.json` and `.mcp.json`
|
||||
actually exist.
|
||||
- `apps` should appear in `plugin.json` only when `.app.json` actually exists.
|
||||
- `mcpServers` may point to `.mcp.json` or contain the MCP server object directly in
|
||||
`plugin.json`.
|
||||
- Validation rejects unsupported manifest fields such as `hooks`, so the scaffold keeps them out of
|
||||
generated manifests.
|
||||
- Run `scripts/validate_plugin.py <plugin-path>` before handing back a generated plugin. It adds one
|
||||
|
||||
@@ -126,18 +126,13 @@ def validate_manifest_shape(
|
||||
|
||||
validate_optional_contract_path(manifest, "skills", "skills", errors)
|
||||
validate_optional_contract_path(manifest, "apps", ".app.json", errors)
|
||||
validate_optional_contract_path(manifest, "mcpServers", ".mcp.json", errors)
|
||||
validate_manifest_mcp_servers(plugin_root, manifest, errors)
|
||||
|
||||
if manifest.get("apps") is not None:
|
||||
validate_app_manifest(
|
||||
plugin_root / ".app.json",
|
||||
errors,
|
||||
)
|
||||
if manifest.get("mcpServers") is not None:
|
||||
validate_mcp_manifest(
|
||||
plugin_root / ".mcp.json",
|
||||
errors,
|
||||
)
|
||||
validate_skill_manifests(plugin_root, errors)
|
||||
|
||||
interface = require_object(manifest, "interface", errors)
|
||||
@@ -286,6 +281,32 @@ def validate_optional_contract_path(
|
||||
errors.append(f"plugin.json field `{key}` must resolve to `{expected}`")
|
||||
|
||||
|
||||
def validate_manifest_mcp_servers(
|
||||
plugin_root: Path,
|
||||
manifest: dict[str, Any],
|
||||
errors: list[str],
|
||||
) -> None:
|
||||
value = manifest.get("mcpServers")
|
||||
if value is None:
|
||||
return
|
||||
if isinstance(value, str):
|
||||
validate_optional_contract_path(manifest, "mcpServers", ".mcp.json", errors)
|
||||
validate_mcp_manifest(
|
||||
plugin_root / ".mcp.json",
|
||||
errors,
|
||||
)
|
||||
return
|
||||
if isinstance(value, dict):
|
||||
validate_mcp_server_entries(
|
||||
value,
|
||||
"plugin.json field `mcpServers`",
|
||||
"plugin.json field `mcpServers`",
|
||||
errors,
|
||||
)
|
||||
return
|
||||
errors.append("plugin.json field `mcpServers` must be a string path or object")
|
||||
|
||||
|
||||
def normalize_contract_path(raw_path: str) -> str | None:
|
||||
path = Path(raw_path)
|
||||
if path.is_absolute():
|
||||
@@ -326,14 +347,28 @@ def validate_mcp_manifest(path: Path, errors: list[str]) -> None:
|
||||
return
|
||||
reject_companion_unknown_fields(payload, {"mcpServers"}, "`.mcp.json`", errors)
|
||||
servers = payload.get("mcpServers")
|
||||
validate_mcp_server_entries(
|
||||
servers,
|
||||
"`.mcp.json`",
|
||||
"`.mcp.json` field `mcpServers`",
|
||||
errors,
|
||||
)
|
||||
|
||||
|
||||
def validate_mcp_server_entries(
|
||||
servers: Any,
|
||||
source_label: str,
|
||||
field_label: str,
|
||||
errors: list[str],
|
||||
) -> None:
|
||||
if not isinstance(servers, dict):
|
||||
errors.append("`.mcp.json` field `mcpServers` must be an object")
|
||||
errors.append(f"{field_label} must be an object")
|
||||
return
|
||||
for key, value in servers.items():
|
||||
if not isinstance(key, str) or not key.strip():
|
||||
errors.append("`.mcp.json` server names must be non-empty strings")
|
||||
errors.append(f"{source_label} server names must be non-empty strings")
|
||||
if not isinstance(value, dict):
|
||||
errors.append(f"`.mcp.json` server `{key}` must be an object")
|
||||
errors.append(f"{source_label} server `{key}` must be an object")
|
||||
|
||||
|
||||
def load_companion_json_object(
|
||||
|
||||
Reference in New Issue
Block a user