Files
codex/codex-rs/core-plugins/src/loader_tests.rs
T
jif 17b9f4843e Extract shared plugin MCP config parsing (#27863)
## Why

We want a thread-selected plugin to eventually expose stdio MCP servers
that run on the executor owning that plugin.

The existing plugin MCP parser lived inside `core-plugins` and was
coupled to the host filesystem loader. Reusing it from an executor
provider would either duplicate MCP normalization or make the plugin
package layer own MCP runtime semantics. This PR creates the shared
MCP-owned boundary first.

In simple terms:

```text
plugin .mcp.json
        |
        v
shared parser in codex-mcp
        |
        +-- Declared placement: preserve current local-plugin behavior
        |
        +-- Environment placement: produce config bound to one executor
```

This builds on the authority-bound plugin descriptors from #27692. It
intentionally does not discover, register, or launch executor MCP
servers yet.

## What changed

- Moved plugin MCP file parsing and normalization from `core-plugins`
into `codex-mcp`.
- Kept support for both existing file shapes: a top-level server map and
an object containing `mcpServers`.
- Kept per-server failure isolation: one invalid server does not discard
valid siblings, while malformed top-level JSON still fails the whole
file.
- Updated the existing local plugin loader to use `Declared` placement,
preserving its current transport, OAuth, relative `cwd`, and error
behavior.
- Added `Environment` placement for the next stacked PR:
- the selected environment ID overrides anything declared by the plugin;
  - missing stdio `cwd` defaults to the plugin root;
- relative `cwd` is resolved beneath the plugin root and cannot traverse
outside it;
- bare or source-less environment-variable references resolve on a
non-local executor;
- explicit orchestrator environment-variable forwarding is rejected for
executor-owned plugins.

## User impact

None in this PR. Existing local plugin MCP loading follows the same
behavior through the shared parser. The executor placement mode is not
connected to thread startup until the follow-up registration PR.

## Assumptions

- A selected capability root's environment is authoritative. A plugin
cannot redirect its stdio process to the orchestrator or another
executor.
- Relative working directories belong under the plugin package root.
Explicit absolute working directories remain valid within the owning
environment.
- For a non-local executor, unqualified environment-variable names refer
to that executor. Reading an orchestrator variable requires an explicit
contract and is rejected for now.
- Parsing only produces normalized `McpServerConfig` values. Process
startup remains owned by the existing MCP runtime and connection
manager.

## Follow-ups

1. Add the executor MCP provider and catalog registration: read the
selected plugin's MCP config through the same executor filesystem,
support stdio only, freeze the result per active thread, apply managed
policy, and resolve name collisions as discovered plugin < selected
plugin < explicit config.
2. Install that provider in app-server and add an end-to-end test
proving `thread/start.selectedCapabilityRoots` launches and calls the
MCP tool on the selected executor, preserves the frozen registration
across refresh, and does not expose it to an unselected thread.
3. After the initial executor-stdio vertical, define
resume/fork/environment-replacement semantics, executor HTTP placement,
warning delivery, common MCP tool-context bounds, and move remaining MCP
source composition above core.

## Verification

- `cargo check -p codex-mcp -p codex-core-plugins --tests`
- `just bazel-lock-check`
- Added focused parser coverage for legacy local normalization, executor
authority, working-directory handling, and environment-variable
sourcing.
2026-06-12 15:10:05 +02:00

509 lines
14 KiB
Rust

use super::*;
use crate::manifest::load_plugin_manifest;
use crate::test_support::write_file;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerSource;
use codex_config::ConfigRequirements;
use codex_config::ConfigRequirementsToml;
use codex_plugin::PluginId;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
fn user_config_path(temp_dir: &TempDir, file_name: &str) -> AbsolutePathBuf {
AbsolutePathBuf::from_absolute_path(temp_dir.path().join(file_name))
.expect("test user config path should be absolute")
}
fn user_layer(path: AbsolutePathBuf, config: &str) -> ConfigLayerEntry {
ConfigLayerEntry::new(
ConfigLayerSource::User {
file: path,
profile: None,
},
toml::from_str(config).expect("user config toml"),
)
}
#[test]
fn configured_plugins_from_stack_merges_user_layers() {
let temp_dir = TempDir::new().expect("tempdir");
let stack = ConfigLayerStack::new(
vec![
user_layer(
user_config_path(&temp_dir, "config.toml"),
"[plugins.base]\nenabled = true\n",
),
user_layer(
user_config_path(&temp_dir, "work.config.toml"),
"[plugins.profile]\nenabled = false\n",
),
],
ConfigRequirements::default(),
ConfigRequirementsToml::default(),
)
.expect("valid config layer stack");
let plugins = configured_plugins_from_stack(&stack);
assert_eq!(
plugins,
HashMap::from([
(
"base".to_string(),
PluginConfig {
enabled: true,
mcp_servers: HashMap::new(),
},
),
(
"profile".to_string(),
PluginConfig {
enabled: false,
mcp_servers: HashMap::new(),
},
),
])
);
}
#[tokio::test]
async fn hooks_only_scope_shares_plugin_resolution_without_loading_other_capabilities() {
let temp_dir = TempDir::new().expect("tempdir");
let plugin_root = temp_dir.path().join("plugins/cache/test/valid/local");
write_file(
&plugin_root.join(".codex-plugin/plugin.json"),
r#"{"name":"valid"}"#,
);
write_file(
&plugin_root.join("skills/example/SKILL.md"),
"---\nname: example\ndescription: example skill\n---\n",
);
write_file(
&plugin_root.join(".mcp.json"),
r#"{"mcpServers":{"example":{"command":"echo"}}}"#,
);
write_file(
&plugin_root.join(".app.json"),
r#"{"apps":{"example":{"id":"connector_example"}}}"#,
);
write_file(
&plugin_root.join("hooks/hooks.json"),
r#"{
"hooks": {
"SessionStart": [
{
"hooks": [
{
"type": "command",
"command": "echo startup"
}
]
}
]
}
}"#,
);
let disabled_root = temp_dir.path().join("plugins/cache/test/disabled/local");
write_file(
&disabled_root.join(".codex-plugin/plugin.json"),
r#"{"name":"disabled"}"#,
);
write_file(
&disabled_root.join("hooks/hooks.json"),
r#"{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"echo disabled"}]}]}}"#,
);
let malformed_root = temp_dir.path().join("plugins/cache/test/malformed/local");
write_file(
&malformed_root.join(".codex-plugin/plugin.json"),
"not valid json",
);
let warning_root = temp_dir.path().join("plugins/cache/test/warning/local");
write_file(
&warning_root.join(".codex-plugin/plugin.json"),
r#"{"name":"warning"}"#,
);
write_file(&warning_root.join("hooks/hooks.json"), "not valid json");
let stack = ConfigLayerStack::new(
vec![user_layer(
user_config_path(&temp_dir, "config.toml"),
r#"
[plugins."valid@test"]
enabled = true
[plugins."disabled@test"]
enabled = false
[plugins.invalid]
enabled = true
[plugins."malformed@test"]
enabled = true
[plugins."missing@test"]
enabled = true
[plugins."warning@test"]
enabled = true
"#,
)],
ConfigRequirements::default(),
ConfigRequirementsToml::default(),
)
.expect("valid config layer stack");
let store = PluginStore::new(temp_dir.path().to_path_buf());
let full = load_plugins_from_layer_stack(
&stack,
HashMap::new(),
&store,
Some(Product::Codex),
/*prefer_remote_curated_conflicts*/ false,
)
.await;
let hooks_only = load_plugins_from_layer_stack_with_scope(
&stack,
HashMap::new(),
&store,
/*prefer_remote_curated_conflicts*/ false,
PluginLoadScope::HooksOnly,
)
.await;
let validation_state = |outcome: &PluginLoadOutcome<McpServerConfig>| {
outcome
.plugins()
.iter()
.map(|plugin| {
(
plugin.config_name.clone(),
plugin.enabled,
plugin.root.clone(),
plugin.error.clone(),
)
})
.collect::<Vec<_>>()
};
assert_eq!(validation_state(&hooks_only), validation_state(&full));
assert_eq!(
hooks_only.effective_plugin_hook_sources(),
full.effective_plugin_hook_sources()
);
assert_eq!(
hooks_only.effective_plugin_hook_warnings(),
full.effective_plugin_hook_warnings()
);
let full_valid = full
.plugins()
.iter()
.find(|plugin| plugin.config_name == "valid@test")
.expect("full load should include valid plugin");
assert!(full_valid.manifest_name.is_some());
assert!(!full_valid.skill_roots.is_empty());
assert!(!full_valid.mcp_servers.is_empty());
assert!(!full_valid.apps.is_empty());
let hooks_only_valid = hooks_only
.plugins()
.iter()
.find(|plugin| plugin.config_name == "valid@test")
.expect("hooks-only load should include valid plugin");
assert_eq!(hooks_only_valid.manifest_name, None);
assert!(hooks_only_valid.skill_roots.is_empty());
assert!(hooks_only_valid.mcp_servers.is_empty());
assert!(hooks_only_valid.apps.is_empty());
}
#[test]
fn curated_plugin_cache_version_shortens_full_git_sha() {
assert_eq!(
curated_plugin_cache_version("0123456789abcdef0123456789abcdef01234567"),
"01234567"
);
}
#[test]
fn curated_plugin_cache_version_preserves_non_git_sha_versions() {
assert_eq!(
curated_plugin_cache_version("export-backup"),
"export-backup"
);
assert_eq!(curated_plugin_cache_version("0123456"), "0123456");
}
fn plugin_id() -> PluginId {
PluginId::parse("demo-plugin@test-marketplace").expect("plugin id")
}
fn plugin_root() -> (tempfile::TempDir, AbsolutePathBuf) {
let tmp = tempfile::tempdir().expect("tempdir");
let plugin_root =
AbsolutePathBuf::try_from(tmp.path().join("demo-plugin")).expect("plugin root");
fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create manifest dir");
fs::create_dir_all(plugin_root.join("hooks")).expect("create hooks dir");
(tmp, plugin_root)
}
fn write_manifest(plugin_root: &AbsolutePathBuf, manifest: &str) {
fs::write(plugin_root.join(".codex-plugin/plugin.json"), manifest).expect("write manifest");
}
fn write_hook_file(plugin_root: &AbsolutePathBuf, relative_path: &str, event: &str, command: &str) {
fs::write(
plugin_root.join(relative_path),
format!(
r#"{{
"hooks": {{
"{event}": [
{{
"hooks": [{{ "type": "command", "command": "{command}" }}]
}}
]
}}
}}"#
),
)
.expect("write hooks");
}
fn load_sources(plugin_root: &AbsolutePathBuf) -> (Vec<PluginHookSource>, Vec<String>) {
let manifest = load_plugin_manifest(plugin_root.as_path()).expect("manifest");
let plugin_data_root = AbsolutePathBuf::try_from(
plugin_root
.as_path()
.parent()
.expect("plugin root parent")
.join("plugin-data"),
)
.expect("plugin data root");
load_plugin_hooks(
plugin_root,
&plugin_id(),
&plugin_data_root,
&manifest.paths,
)
}
fn assert_sources(sources: &[PluginHookSource], expected_relative_paths: &[&str]) {
assert_eq!(
sources
.iter()
.map(|source| source.plugin_id.clone())
.collect::<Vec<_>>(),
vec![plugin_id(); expected_relative_paths.len()]
);
assert_eq!(
sources
.iter()
.map(|source| source.source_relative_path.as_str())
.collect::<Vec<_>>(),
expected_relative_paths
);
assert_eq!(
sources
.iter()
.map(|source| source.hooks.handler_count())
.collect::<Vec<_>>(),
vec![1; expected_relative_paths.len()]
);
}
#[test]
fn load_plugin_hooks_discovers_default_hooks_file() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(&plugin_root, r#"{ "name": "demo-plugin" }"#);
fs::write(
plugin_root.join("hooks/hooks.json"),
r#"{
"hooks": {
"PreToolUse": [
{
"matcher": "Bash",
"hooks": [{ "type": "command", "command": "echo default" }]
}
]
}
}"#,
)
.expect("write hooks");
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(warnings, Vec::<String>::new());
assert_sources(&sources, &["hooks/hooks.json"]);
}
#[test]
fn load_plugin_hooks_supports_manifest_hook_path() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(
&plugin_root,
r#"{
"name": "demo-plugin",
"hooks": "./hooks/one.json"
}"#,
);
write_hook_file(&plugin_root, "hooks/one.json", "PreToolUse", "echo one");
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(warnings, Vec::<String>::new());
assert_sources(&sources, &["hooks/one.json"]);
}
#[test]
fn load_plugin_hooks_manifest_paths_replace_default_hooks_file() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(
&plugin_root,
r#"{
"name": "demo-plugin",
"hooks": ["./hooks/one.json", "./hooks/two.json"]
}"#,
);
write_hook_file(
&plugin_root,
"hooks/hooks.json",
"PreToolUse",
"echo ignored",
);
write_hook_file(&plugin_root, "hooks/one.json", "PreToolUse", "echo one");
write_hook_file(&plugin_root, "hooks/two.json", "PostToolUse", "echo two");
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(warnings, Vec::<String>::new());
assert_sources(&sources, &["hooks/one.json", "hooks/two.json"]);
}
#[test]
fn load_plugin_hooks_supports_inline_manifest_hooks() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(
&plugin_root,
r#"{
"name": "demo-plugin",
"hooks": {
"hooks": {
"SessionStart": [
{
"matcher": "startup",
"hooks": [{ "type": "command", "command": "echo inline" }]
}
]
}
}
}"#,
);
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(warnings, Vec::<String>::new());
assert_sources(&sources, &["plugin.json#hooks[0]"]);
}
#[test]
fn load_plugin_hooks_reports_invalid_hook_file() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(&plugin_root, r#"{ "name": "demo-plugin" }"#);
fs::write(plugin_root.join("hooks/hooks.json"), "{ not-json").expect("write invalid hooks");
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(sources, Vec::<PluginHookSource>::new());
assert_eq!(
warnings,
vec![format!(
"failed to parse plugin hooks config {}: key must be a string at line 1 column 3",
plugin_root.join("hooks/hooks.json").display()
)]
);
}
#[test]
fn load_plugin_hooks_supports_inline_manifest_hook_list() {
let (_tmp, plugin_root) = plugin_root();
write_manifest(
&plugin_root,
r#"{
"name": "demo-plugin",
"hooks": [
{
"hooks": {
"SessionStart": [
{
"hooks": [{ "type": "command", "command": "echo inline one" }]
}
]
}
},
{
"hooks": {
"Stop": [
{
"hooks": [{ "type": "command", "command": "echo inline two" }]
}
]
}
}
]
}"#,
);
let (sources, warnings) = load_sources(&plugin_root);
assert_eq!(warnings, Vec::<String>::new());
assert_sources(&sources, &["plugin.json#hooks[0]", "plugin.json#hooks[1]"]);
}
#[test]
fn materialize_git_subdir_uses_sparse_checkout() {
let codex_home = tempfile::tempdir().expect("create codex home");
let repo = tempfile::tempdir().expect("create git repo");
let plugin_dir = repo.path().join("plugins/toolkit");
fs::create_dir_all(&plugin_dir).expect("create plugin directory");
fs::create_dir_all(repo.path().join("plugins/other")).expect("create other plugin");
fs::write(plugin_dir.join("marker.txt"), "toolkit").expect("write plugin marker");
fs::write(repo.path().join("plugins/other/marker.txt"), "other").expect("write other marker");
fs::write(repo.path().join("root.txt"), "root").expect("write root marker");
run_git(&["init"], Some(repo.path())).expect("init git repo");
run_git(
&["config", "user.email", "test@example.com"],
Some(repo.path()),
)
.expect("configure git email");
run_git(&["config", "user.name", "Test User"], Some(repo.path())).expect("configure git name");
run_git(&["add", "."], Some(repo.path())).expect("stage git repo");
run_git(&["commit", "-m", "init"], Some(repo.path())).expect("commit git repo");
let materialized = materialize_marketplace_plugin_source(
codex_home.path(),
&MarketplacePluginSource::Git {
url: repo.path().display().to_string(),
path: Some("plugins/toolkit".to_string()),
ref_name: None,
sha: None,
},
)
.expect("materialize git source");
assert_eq!(
plugin_dir.file_name(),
materialized.path.as_path().file_name()
);
assert!(materialized.path.as_path().join("marker.txt").is_file());
let checkout_root = materialized
.path
.as_path()
.parent()
.and_then(Path::parent)
.expect("materialized path should be nested under checkout root");
assert!(!checkout_root.join("root.txt").exists());
assert!(!checkout_root.join("plugins/other/marker.txt").exists());
}