mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Make selected plugin roots URI-native (#28918)
## Why Selected capability roots belong to the executor filesystem, not the app-server host. Converting their path strings into the host's native `Path` breaks whenever the two machines use different path conventions, such as a Windows executor behind a Unix app-server. This PR establishes `PathUri` as the selected-plugin boundary so the executor remains authoritative for its paths. ## What changed - Require `selectedCapabilityRoots[].location.path` to be a canonical `file:` URI and deserialize it directly as `PathUri`; native path strings are rejected. - Update the app-server schema, generated TypeScript, examples, and request coverage for the URI contract. - Keep selected roots, resolved plugin locations, manifest paths, and manifest resources as `PathUri`. - Inspect and read plugin roots and manifests only through the selected environment's `ExecutorFileSystem`. - Parse executor manifests with the shared URI-native parser from #29620 instead of projecting them onto the host filesystem. - Enforce resource containment lexically and preserve the root URI's POSIX or Windows path convention. - Cover foreign Windows plugin roots and URI-native manifest resources. ```text thread/start selectedCapabilityRoots[].location.path = "file:///C:/plugins/demo" | PathUri v ExecutorFileSystem | +--> plugin.json +--> manifest resources ``` This PR stops at the shared selected-plugin representation. The next two PRs remove the remaining host-path projections in the skill and MCP consumers. ## Stack 1. #29614 — add lexical `PathUri` containment. 2. #29620 — share URI-native manifest path resolution. 3. **This PR** — keep selected plugin roots and resources URI-native. 4. #29626 — load executor skills without host path conversion. 5. #29628 — resolve executor MCP working directories without host path conversion.
This commit is contained in:
@@ -8,9 +8,10 @@ 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 codex_utils_path_uri::PathUriParseError;
|
||||
use std::io;
|
||||
use std::path::PathBuf;
|
||||
use thiserror::Error;
|
||||
|
||||
const DEFAULT_MCP_CONFIG_FILE: &str = ".mcp.json";
|
||||
@@ -25,14 +26,24 @@ pub(super) enum ExecutorPluginMcpProviderError {
|
||||
#[error("failed to read MCP config for selected plugin `{plugin_id}` at `{path}`: {source}")]
|
||||
ReadConfig {
|
||||
plugin_id: String,
|
||||
path: AbsolutePathBuf,
|
||||
path: PathUri,
|
||||
#[source]
|
||||
source: io::Error,
|
||||
},
|
||||
#[error(
|
||||
"failed to resolve MCP config path `{relative_path}` below selected plugin `{plugin_id}` at `{root}`: {source}"
|
||||
)]
|
||||
InvalidConfigPath {
|
||||
plugin_id: String,
|
||||
root: PathUri,
|
||||
relative_path: &'static str,
|
||||
#[source]
|
||||
source: PathUriParseError,
|
||||
},
|
||||
#[error("failed to parse MCP config for selected plugin `{plugin_id}` at `{path}`: {source}")]
|
||||
ParseConfig {
|
||||
plugin_id: String,
|
||||
path: AbsolutePathBuf,
|
||||
path: PathUri,
|
||||
#[source]
|
||||
source: serde_json::Error,
|
||||
},
|
||||
@@ -52,7 +63,7 @@ impl ExecutorPluginMcpProvider {
|
||||
|
||||
async fn load_from_file_system(
|
||||
plugin: &ResolvedPlugin,
|
||||
plugin_root: &AbsolutePathBuf,
|
||||
plugin_root: &PathUri,
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
) -> Result<Vec<(String, McpServerConfig)>, ExecutorPluginMcpProviderError> {
|
||||
let ResolvedPluginLocation::Environment { environment_id, .. } = plugin.location();
|
||||
@@ -61,10 +72,9 @@ async fn load_from_file_system(
|
||||
Some(PluginManifestMcpServers::Path(PluginResourceLocator::Environment {
|
||||
path, ..
|
||||
})) => {
|
||||
let config_uri = PathUri::from_abs_path(path);
|
||||
(
|
||||
file_system
|
||||
.read_file_text(&config_uri, /*sandbox*/ None)
|
||||
.read_file_text(path, /*sandbox*/ None)
|
||||
.await
|
||||
.map_err(|source| ExecutorPluginMcpProviderError::ReadConfig {
|
||||
plugin_id: plugin_id.to_string(),
|
||||
@@ -74,15 +84,21 @@ async fn load_from_file_system(
|
||||
path.clone(),
|
||||
)
|
||||
}
|
||||
Some(PluginManifestMcpServers::Object(object_config)) => (
|
||||
object_config.clone(),
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
),
|
||||
Some(PluginManifestMcpServers::Object(object_config)) => {
|
||||
let PluginResourceLocator::Environment { path, .. } = plugin.manifest_path();
|
||||
(object_config.clone(), path.clone())
|
||||
}
|
||||
None => {
|
||||
let config_path = plugin_root.join(DEFAULT_MCP_CONFIG_FILE);
|
||||
let config_uri = PathUri::from_abs_path(&config_path);
|
||||
let config_path = plugin_root
|
||||
.join(DEFAULT_MCP_CONFIG_FILE)
|
||||
.map_err(|source| ExecutorPluginMcpProviderError::InvalidConfigPath {
|
||||
plugin_id: plugin_id.to_string(),
|
||||
root: plugin_root.clone(),
|
||||
relative_path: DEFAULT_MCP_CONFIG_FILE,
|
||||
source,
|
||||
})?;
|
||||
let contents = match file_system
|
||||
.read_file_text(&config_uri, /*sandbox*/ None)
|
||||
.read_file_text(&config_path, /*sandbox*/ None)
|
||||
.await
|
||||
{
|
||||
Ok(contents) => contents,
|
||||
@@ -100,8 +116,9 @@ async fn load_from_file_system(
|
||||
(contents, config_path)
|
||||
}
|
||||
};
|
||||
let plugin_root_path = PathBuf::from(plugin_root.inferred_native_path_string());
|
||||
let parsed = parse_plugin_mcp_config(
|
||||
plugin_root.as_path(),
|
||||
plugin_root_path.as_path(),
|
||||
&contents,
|
||||
PluginMcpServerPlacement::Environment { environment_id },
|
||||
)
|
||||
|
||||
@@ -156,7 +156,8 @@ async fn reads_declared_config_only_through_executor_file_system() {
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let servers = load_from_file_system(&plugin, &plugin_root, &file_system)
|
||||
let plugin_root_uri = PathUri::from_abs_path(&plugin_root);
|
||||
let servers = load_from_file_system(&plugin, &plugin_root_uri, &file_system)
|
||||
.await
|
||||
.expect("load executor MCP config");
|
||||
|
||||
@@ -210,7 +211,8 @@ async fn reads_manifest_object_config_without_executor_file_system_access() {
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let servers = load_from_file_system(&plugin, &plugin_root, &file_system)
|
||||
let plugin_root_uri = PathUri::from_abs_path(&plugin_root);
|
||||
let servers = load_from_file_system(&plugin, &plugin_root_uri, &file_system)
|
||||
.await
|
||||
.expect("load manifest object executor MCP config");
|
||||
|
||||
@@ -259,7 +261,8 @@ async fn missing_default_config_is_empty() {
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let servers = load_from_file_system(&plugin, &plugin_root, &file_system)
|
||||
let plugin_root_uri = PathUri::from_abs_path(&plugin_root);
|
||||
let servers = load_from_file_system(&plugin, &plugin_root_uri, &file_system)
|
||||
.await
|
||||
.expect("missing default config should be ignored");
|
||||
|
||||
@@ -283,7 +286,8 @@ async fn malformed_declared_config_is_an_error() {
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let err = load_from_file_system(&plugin, &plugin_root, &file_system)
|
||||
let plugin_root_uri = PathUri::from_abs_path(&plugin_root);
|
||||
let err = load_from_file_system(&plugin, &plugin_root_uri, &file_system)
|
||||
.await
|
||||
.expect_err("malformed declared config should fail");
|
||||
|
||||
@@ -297,20 +301,70 @@ async fn malformed_declared_config_is_an_error() {
|
||||
};
|
||||
assert_eq!(
|
||||
(plugin_id, path),
|
||||
("selected-root".to_string(), config_path.clone())
|
||||
(
|
||||
"selected-root".to_string(),
|
||||
PathUri::from_abs_path(&config_path)
|
||||
)
|
||||
);
|
||||
assert_eq!(reads(&file_system), vec![config_path]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn malformed_manifest_object_config_reports_actual_manifest_path() {
|
||||
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 plugin = resolved_plugin(
|
||||
&plugin_root,
|
||||
Some(PluginManifestMcpServers::Object("{not-json".to_string())),
|
||||
);
|
||||
let file_system = SyntheticExecutorFileSystem {
|
||||
config_path: plugin_root.join(DEFAULT_MCP_CONFIG_FILE),
|
||||
config_contents: None,
|
||||
reads: Mutex::new(Vec::new()),
|
||||
};
|
||||
|
||||
let plugin_root_uri = PathUri::from_abs_path(&plugin_root);
|
||||
let err = load_from_file_system(&plugin, &plugin_root_uri, &file_system)
|
||||
.await
|
||||
.expect_err("malformed manifest object config should fail");
|
||||
|
||||
let ExecutorPluginMcpProviderError::ParseConfig {
|
||||
plugin_id,
|
||||
path,
|
||||
source: _,
|
||||
} = err
|
||||
else {
|
||||
panic!("expected parse error");
|
||||
};
|
||||
assert_eq!(
|
||||
(plugin_id, path),
|
||||
(
|
||||
"selected-root".to_string(),
|
||||
PathUri::from_abs_path(&plugin_root.join(".claude-plugin/plugin.json"))
|
||||
)
|
||||
);
|
||||
assert_eq!(reads(&file_system), Vec::new());
|
||||
}
|
||||
|
||||
fn resolved_plugin(
|
||||
plugin_root: &AbsolutePathBuf,
|
||||
mcp_servers: Option<PluginManifestMcpServers<AbsolutePathBuf>>,
|
||||
) -> ResolvedPlugin {
|
||||
let plugin_root_uri = PathUri::from_abs_path(plugin_root);
|
||||
let mcp_servers = mcp_servers.map(|mcp_servers| match mcp_servers {
|
||||
PluginManifestMcpServers::Path(path) => {
|
||||
PluginManifestMcpServers::Path(PathUri::from_abs_path(&path))
|
||||
}
|
||||
PluginManifestMcpServers::Object(config) => PluginManifestMcpServers::Object(config),
|
||||
});
|
||||
ResolvedPlugin::from_environment(
|
||||
"selected-root".to_string(),
|
||||
"executor-test".to_string(),
|
||||
plugin_root.clone(),
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
plugin_root_uri.clone(),
|
||||
plugin_root_uri
|
||||
.join(".claude-plugin/plugin.json")
|
||||
.expect("manifest URI"),
|
||||
PluginManifest {
|
||||
name: "demo-plugin".to_string(),
|
||||
version: None,
|
||||
|
||||
@@ -9,6 +9,7 @@ use codex_extension_api::McpServerContribution;
|
||||
use codex_extension_api::McpServerContributionContext;
|
||||
use codex_protocol::capabilities::CapabilityRootLocation;
|
||||
use codex_protocol::capabilities::SelectedCapabilityRoot;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
|
||||
@@ -59,7 +60,7 @@ command = "expected-command"
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
let contributions = selected_plugin_contributions(&config, plugin_root.path()).await;
|
||||
let contributions = selected_plugin_contributions(&config, plugin_root.path()).await?;
|
||||
|
||||
assert_eq!(
|
||||
contributions,
|
||||
@@ -93,7 +94,7 @@ command = "expected-command"
|
||||
async fn selected_plugin_contributions(
|
||||
config: &Config,
|
||||
plugin_root: &std::path::Path,
|
||||
) -> Vec<ContributionSummary> {
|
||||
) -> Result<Vec<ContributionSummary>, Box<dyn std::error::Error>> {
|
||||
let mut builder = ExtensionRegistryBuilder::new();
|
||||
codex_mcp_extension::install_executor_plugins(
|
||||
&mut builder,
|
||||
@@ -105,12 +106,12 @@ async fn selected_plugin_contributions(
|
||||
id: "selected-root".to_string(),
|
||||
location: CapabilityRootLocation::Environment {
|
||||
environment_id: LOCAL_ENVIRONMENT_ID.to_string(),
|
||||
path: plugin_root.to_string_lossy().into_owned(),
|
||||
path: PathUri::from_host_native_path(plugin_root)?,
|
||||
},
|
||||
}]);
|
||||
codex_mcp_extension::initialize_executor_plugin_thread_data(&mut thread_init);
|
||||
|
||||
registry.mcp_server_contributors()[0]
|
||||
Ok(registry.mcp_server_contributors()[0]
|
||||
.contribute(McpServerContributionContext::for_thread(
|
||||
config,
|
||||
&thread_init,
|
||||
@@ -135,5 +136,5 @@ async fn selected_plugin_contributions(
|
||||
panic!("expected selected plugin contribution")
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_core_skills::SkillMetadata;
|
||||
@@ -197,13 +196,6 @@ fn catalog_entry_from_skill(
|
||||
entry
|
||||
}
|
||||
|
||||
fn executor_absolute_path(path: &str) -> std::io::Result<AbsolutePathBuf> {
|
||||
let path = PathBuf::from(path);
|
||||
if !path.is_absolute() {
|
||||
return Err(std::io::Error::new(
|
||||
std::io::ErrorKind::InvalidInput,
|
||||
"executor path must be absolute",
|
||||
));
|
||||
}
|
||||
AbsolutePathBuf::from_absolute_path_checked(path)
|
||||
fn executor_absolute_path(path: &PathUri) -> std::io::Result<AbsolutePathBuf> {
|
||||
path.to_abs_path()
|
||||
}
|
||||
|
||||
@@ -222,7 +222,6 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() {
|
||||
#[tokio::test]
|
||||
async fn selected_root_id_distinguishes_identical_executor_paths() {
|
||||
let test_root = create_local_skill_root("root-identity").expect("create local skill root");
|
||||
let root_path = test_root.to_string_lossy().into_owned();
|
||||
let canonical_root = AbsolutePathBuf::from_absolute_path_checked(&test_root)
|
||||
.expect("absolute skill root")
|
||||
.canonicalize()
|
||||
@@ -242,7 +241,7 @@ async fn selected_root_id_distinguishes_identical_executor_paths() {
|
||||
id: id.to_string(),
|
||||
location: CapabilityRootLocation::Environment {
|
||||
environment_id: "local".to_string(),
|
||||
path: root_path.clone(),
|
||||
path: PathUri::from_host_native_path(&test_root).expect("skill root URI"),
|
||||
},
|
||||
})
|
||||
.collect(),
|
||||
|
||||
@@ -48,6 +48,7 @@ use codex_skills_extension::provider::SkillProviderFuture;
|
||||
use codex_skills_extension::provider::SkillReadRequest;
|
||||
use codex_skills_extension::provider::SkillSearchRequest;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
type TestResult = Result<(), Box<dyn std::error::Error>>;
|
||||
@@ -171,7 +172,7 @@ async fn selected_executor_catalog_is_context_and_selected_entrypoint_is_turn_in
|
||||
id: "lint-fix".to_string(),
|
||||
location: CapabilityRootLocation::Environment {
|
||||
environment_id: "env-1".to_string(),
|
||||
path: "/skills/lint-fix".to_string(),
|
||||
path: PathUri::parse("file:///skills/lint-fix").expect("skill root URI"),
|
||||
},
|
||||
}]);
|
||||
let session_source = SessionSource::Cli;
|
||||
@@ -494,7 +495,7 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te
|
||||
id: id.to_string(),
|
||||
location: CapabilityRootLocation::Environment {
|
||||
environment_id: "env-1".to_string(),
|
||||
path: path.to_string(),
|
||||
path: PathUri::parse(&format!("file://{path}")).expect("skill root URI"),
|
||||
},
|
||||
})
|
||||
.collect::<Vec<_>>(),
|
||||
|
||||
Reference in New Issue
Block a user