Load executor skills without host path conversion (#29626)

## Why

After #28918, selected skill roots are `PathUri`, but the executor skill
provider still converts them to the app-server host's `AbsolutePathBuf`.
A foreign Windows root therefore cannot be discovered by a Unix host,
and the inverse has the same problem.

This PR keeps executor skill discovery and reads on the filesystem that
owns the selected root while reusing the existing skill rules.

## What changed

- Generalize the existing skill traversal to operate on `PathUri`
through `ExecutorFileSystem`, preserving its depth, directory, symlink,
and sibling-metadata concurrency behavior.
- Add a small environment skill loader that reuses the shared discovery,
frontmatter validation, dependency parsing, product policy, and
prompt-visibility rules.
- Keep the environment id and entrypoint `PathUri` in the skill catalog,
then route `skills.read` back through the same environment filesystem.
- Preserve the executor's path convention when deriving catalog handles,
including literal backslashes in POSIX filenames.
- Resolve plugin namespaces from nearby manifests through URI-native
filesystem reads.
- Cover foreign Windows roots, executor-owned reads, namespaces,
metadata, policy, and path identity.

```text
selected root (PathUri)
        |
        v
shared discovery over ExecutorFileSystem
        |
        v
environment-bound catalog entry --skills.read--> same ExecutorFileSystem
```

No second filesystem abstraction or duplicate traversal implementation
is introduced.

## Stack

1. #29614 — add lexical `PathUri` containment.
2. #29620 — share URI-native manifest path resolution.
3. #28918 — keep selected plugin roots and resources URI-native.
4. **This PR** — load executor skills without host path conversion.
5. #29628 — resolve executor MCP working directories without host path
conversion.
This commit is contained in:
jif
2026-06-23 23:26:06 +01:00
committed by GitHub
parent db6e676afc
commit 220f5b76b2
10 changed files with 520 additions and 200 deletions
@@ -1,3 +1,4 @@
#![recursion_limit = "256"]
#![allow(clippy::expect_used)]
use std::sync::Arc;
+1 -1
View File
@@ -20,7 +20,6 @@ codex-extension-api = { workspace = true }
codex-mcp = { workspace = true }
codex-protocol = { workspace = true }
codex-tools = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-path-uri = { workspace = true }
codex-utils-string = { workspace = true }
schemars = { workspace = true }
@@ -31,5 +30,6 @@ tracing = { workspace = true }
url = { workspace = true }
[dev-dependencies]
codex-utils-absolute-path = { workspace = true }
pretty_assertions = { workspace = true }
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
+4 -4
View File
@@ -1,5 +1,5 @@
use codex_core_skills::model::SkillDependencies;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
/// Source authority that owns a skill package and must be used to read it.
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
@@ -76,7 +76,7 @@ impl SkillResourceId {
pub fn environment(
id: impl Into<String>,
environment_id: impl Into<String>,
path: AbsolutePathBuf,
path: PathUri,
) -> Self {
Self {
id: id.into(),
@@ -91,7 +91,7 @@ impl SkillResourceId {
&self.id
}
pub(crate) fn environment_path(&self) -> Option<(&str, &AbsolutePathBuf)> {
pub(crate) fn environment_path(&self) -> Option<(&str, &PathUri)> {
self.environment_path
.as_ref()
.map(|resource| (resource.environment_id.as_str(), &resource.path))
@@ -101,7 +101,7 @@ impl SkillResourceId {
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
struct EnvironmentSkillResource {
environment_id: String,
path: AbsolutePathBuf,
path: PathUri,
}
/// Metadata shown in the always-visible skills catalog.
+23 -58
View File
@@ -1,15 +1,11 @@
use std::sync::Arc;
use codex_core_skills::SkillMetadata;
use codex_core_skills::filter_skill_load_outcome_for_product;
use codex_core_skills::loader::SkillRoot;
use codex_core_skills::loader::load_skills_from_roots;
use codex_core_skills::loader::EnvironmentSkillMetadata;
use codex_core_skills::loader::load_environment_skills_from_root;
use codex_exec_server::EnvironmentManager;
use codex_protocol::capabilities::CapabilityRootLocation;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use codex_utils_path_uri::PathConvention;
use crate::catalog::SkillAuthority;
use crate::catalog::SkillCatalog;
@@ -64,42 +60,17 @@ impl SkillProvider for ExecutorSkillProvider {
));
continue;
};
let root_path = match executor_absolute_path(&path) {
Ok(root_path) => root_path,
Err(err) => {
catalog.warnings.push(format!(
"Selected capability root `{selected_root_id}` has invalid path `{path}`: {err}"
));
continue;
}
};
let file_system = environment.get_filesystem();
let outcome = filter_skill_load_outcome_for_product(
load_skills_from_roots(
[SkillRoot {
path: root_path.clone(),
scope: SkillScope::User,
file_system: Arc::clone(&file_system),
plugin_id: None,
plugin_namespace: None,
plugin_root: None,
}],
/*plugin_skill_snapshots*/ None,
)
.await,
let outcome = load_environment_skills_from_root(
file_system.as_ref(),
&path,
self.restriction_product,
);
catalog.warnings.extend(outcome.errors.iter().map(|err| {
format!(
"Failed to load executor skill at {}: {}",
err.path.display(),
err.message
)
}));
for (skill, enabled) in outcome.skills_with_enabled() {
)
.await;
catalog.warnings.extend(outcome.warnings);
for skill in outcome.skills {
catalog.push_entry(catalog_entry_from_skill(
skill,
enabled,
&skill,
authority.clone(),
&selected_root_id,
&environment_id,
@@ -134,10 +105,9 @@ impl SkillProvider for ExecutorSkillProvider {
"executor skill resource references unavailable environment `{environment_id}`"
)));
};
let resource_path = PathUri::from_abs_path(resource_path);
let contents = environment
.get_filesystem()
.read_file_text(&resource_path, /*sandbox*/ None)
.read_file_text(resource_path, /*sandbox*/ None)
.await
.map_err(|err| {
SkillProviderError::new(format!(
@@ -159,19 +129,21 @@ impl SkillProvider for ExecutorSkillProvider {
}
fn catalog_entry_from_skill(
skill: &SkillMetadata,
enabled: bool,
skill: &EnvironmentSkillMetadata,
authority: SkillAuthority,
selected_root_id: &str,
environment_id: &str,
) -> SkillCatalogEntry {
let skill_path = skill.path_to_skills_md.to_string_lossy().into_owned();
let normalized_path = skill_path.replace('\\', "/");
let skill_path = skill.path_to_skills_md.inferred_native_path_string();
let normalized_path = match skill.path_to_skills_md.infer_path_convention() {
Some(PathConvention::Windows) => skill_path.replace('\\', "/"),
Some(PathConvention::Posix) | None => skill_path,
};
let display_path = format!(
"skill://{selected_root_id}/{}",
normalized_path.trim_start_matches('/')
);
let mut entry = SkillCatalogEntry::new(
let entry = SkillCatalogEntry::new(
SkillPackageId(display_path.clone()),
authority,
skill.name.clone(),
@@ -186,16 +158,9 @@ fn catalog_entry_from_skill(
.with_display_path(display_path)
.with_dependencies(skill.dependencies.clone());
if !enabled {
entry = entry.disabled();
if skill.allows_implicit_invocation() {
entry
} else {
entry.hidden_from_prompt()
}
if !skill.allows_implicit_invocation() {
entry = entry.hidden_from_prompt();
}
entry
}
fn executor_absolute_path(path: &PathUri) -> std::io::Result<AbsolutePathBuf> {
path.to_abs_path()
}
@@ -28,40 +28,48 @@ use pretty_assertions::assert_eq;
const SKILL_CONTENTS: &str =
"---\nname: synthetic\ndescription: Synthetic executor skill.\n---\n\nEXECUTOR_ONLY_BODY\n";
const PLUGIN_MANIFEST: &str = r#"{"name":"synthetic-plugin"}"#;
static NEXT_TEST_ROOT_ID: AtomicUsize = AtomicUsize::new(0);
struct SyntheticFileSystem {
alias_root: AbsolutePathBuf,
canonical_root: AbsolutePathBuf,
alias_root: PathUri,
canonical_root: PathUri,
has_plugin_manifest: bool,
}
impl SyntheticFileSystem {
fn path(&self, relative_path: &str) -> io::Result<PathUri> {
self.canonical_root
.join(relative_path)
.map_err(io::Error::other)
}
async fn canonicalize(&self, path: &PathUri) -> io::Result<PathUri> {
let path = path.to_abs_path()?;
if path == self.alias_root {
return Ok(PathUri::from_abs_path(&self.canonical_root));
if path == &self.alias_root {
return Ok(self.canonical_root.clone());
}
self.metadata(&path)?;
Ok(PathUri::from_abs_path(&path))
self.metadata(path)?;
Ok(path.clone())
}
async fn read_file(&self, path: &PathUri) -> io::Result<Vec<u8>> {
if path.to_abs_path()? == self.canonical_root.join("skill/SKILL.md") {
if path == &self.path("skill/SKILL.md")? {
Ok(SKILL_CONTENTS.as_bytes().to_vec())
} else if self.has_plugin_manifest && path == &self.path(".claude-plugin/plugin.json")? {
Ok(PLUGIN_MANIFEST.as_bytes().to_vec())
} else {
Err(io::Error::new(io::ErrorKind::NotFound, "not found"))
}
}
async fn read_directory(&self, path: &PathUri) -> io::Result<Vec<ReadDirectoryEntry>> {
let path = path.to_abs_path()?;
if path == self.canonical_root {
if path == &self.canonical_root {
Ok(vec![ReadDirectoryEntry {
file_name: "skill".to_string(),
is_directory: true,
is_file: false,
}])
} else if path == self.canonical_root.join("skill") {
} else if path == &self.path("skill")? {
Ok(vec![ReadDirectoryEntry {
file_name: "SKILL.md".to_string(),
is_directory: false,
@@ -72,12 +80,13 @@ impl SyntheticFileSystem {
}
}
fn metadata(&self, path: &AbsolutePathBuf) -> io::Result<FileMetadata> {
let skill_dir = self.canonical_root.join("skill");
let skill_path = skill_dir.join("SKILL.md");
fn metadata(&self, path: &PathUri) -> io::Result<FileMetadata> {
let skill_dir = self.path("skill")?;
let skill_path = self.path("skill/SKILL.md")?;
let manifest_path = self.path(".claude-plugin/plugin.json")?;
let (is_directory, is_file) = if path == &self.canonical_root || path == &skill_dir {
(true, false)
} else if path == &skill_path {
} else if path == &skill_path || self.has_plugin_manifest && path == &manifest_path {
(false, true)
} else {
return Err(io::Error::new(io::ErrorKind::NotFound, "not found"));
@@ -146,7 +155,7 @@ impl ExecutorFileSystem for SyntheticFileSystem {
path: &'a PathUri,
_sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, FileMetadata> {
Box::pin(async move { self.metadata(&path.to_abs_path()?) })
Box::pin(async move { self.metadata(path) })
}
fn read_directory<'a>(
@@ -193,8 +202,9 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() {
path: alias_root.clone(),
scope: SkillScope::User,
file_system: Arc::new(SyntheticFileSystem {
alias_root,
canonical_root: canonical_root.clone(),
alias_root: PathUri::from_abs_path(&alias_root),
canonical_root: PathUri::from_abs_path(&canonical_root),
has_plugin_manifest: false,
}),
plugin_id: None,
plugin_namespace: None,
@@ -221,13 +231,18 @@ 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 canonical_root = AbsolutePathBuf::from_absolute_path_checked(&test_root)
.expect("absolute skill root")
.canonicalize()
.expect("canonicalize skill root")
.to_string_lossy()
.replace('\\', "/");
let root_label = if cfg!(unix) {
r"root\identity"
} else {
"root-identity"
};
let test_root = create_local_skill_root(root_label).expect("create local skill root");
let selected_root = test_root.to_string_lossy().into_owned();
let selected_root = if cfg!(windows) {
selected_root.replace('\\', "/")
} else {
selected_root
};
let provider = ExecutorSkillProvider::new_with_restriction_product(
Arc::new(EnvironmentManager::default_for_tests()),
/*restriction_product*/ None,
@@ -268,14 +283,14 @@ async fn selected_root_id_distinguishes_identical_executor_paths() {
"root-a".to_string(),
format!(
"skill://root-a/{}/skill/SKILL.md",
canonical_root.trim_start_matches('/')
selected_root.trim_start_matches('/')
),
),
(
"root-b".to_string(),
format!(
"skill://root-b/{}/skill/SKILL.md",
canonical_root.trim_start_matches('/')
selected_root.trim_start_matches('/')
),
),
]