From 83b7372e0f477f2b9f2080ed3bb855697394176b Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 29 May 2026 12:18:30 -0700 Subject: [PATCH 1/2] core-skills: separate selected and local skill root authority --- codex-rs/app-server/src/skills_watcher.rs | 2 +- codex-rs/core-plugins/src/loader.rs | 14 +- codex-rs/core-plugins/src/manager.rs | 4 +- codex-rs/core-skills/src/loader.rs | 327 ++++++++++++---------- codex-rs/core-skills/src/loader_tests.rs | 95 +++++-- codex-rs/core-skills/src/manager.rs | 137 ++++++--- codex-rs/core-skills/src/manager_tests.rs | 174 +++++++++--- 7 files changed, 498 insertions(+), 255 deletions(-) diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 57edb89c7ff..e43237ba446 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -115,7 +115,7 @@ impl SkillsWatcher { .await .into_iter() .map(|root| WatchPath { - path: root.path.into_path_buf(), + path: root.path.path().to_path_buf(), recursive: true, }) .collect(); diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index cf51d4958ed..4ae3a0a250e 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -19,7 +19,7 @@ use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; -use codex_exec_server::LOCAL_FS; +use codex_exec_server::EnvironmentPathRef; use codex_plugin::AppConnectorId; use codex_plugin::LoadedPlugin; use codex_plugin::PluginCapabilitySummary; @@ -40,7 +40,6 @@ use std::collections::HashSet; use std::fs; use std::path::Path; use std::process::Command; -use std::sync::Arc; use tempfile::TempDir; use tracing::warn; @@ -564,8 +563,10 @@ async fn load_plugin( .or_else(|| Some(manifest.name.clone())); loaded_plugin.manifest_description = manifest.description.clone(); loaded_plugin.skill_roots = plugin_skill_roots(&plugin_root, manifest_paths); + // TODO: Support multi-env plugin skill loading. Plugin discovery is local-only today, so bind + // plugin skill reads to the local environment instead of the selected turn environment. let resolved_skills = load_plugin_skills( - &plugin_root, + EnvironmentPathRef::local(plugin_root.clone()), &loaded_plugin_id, manifest_paths, restriction_product, @@ -642,18 +643,17 @@ impl ResolvedPluginSkills { } pub async fn load_plugin_skills( - plugin_root: &AbsolutePathBuf, + plugin_root: EnvironmentPathRef, plugin_id: &PluginId, manifest_paths: &PluginManifestPaths, restriction_product: Option, skill_config_rules: &SkillConfigRules, ) -> ResolvedPluginSkills { - let roots = plugin_skill_roots(plugin_root, manifest_paths) + let roots = plugin_skill_roots(plugin_root.path(), manifest_paths) .into_iter() .map(|path| SkillRoot { - path, + path: plugin_root.with_path(path), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some(plugin_id.as_key()), plugin_root: Some(plugin_root.clone()), }) diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index ec3e74dd16c..5d4b497951d 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -1419,8 +1419,10 @@ impl PluginsManager { manifest.interface.clone(), marketplace_category, ); + // TODO: Support multi-env plugin skill loading. Marketplace plugins are installed and + // scanned locally today, so bind plugin skill reads to the local environment. let resolved_skills = load_plugin_skills( - &source_path, + codex_exec_server::EnvironmentPathRef::local(source_path.clone()), &plugin_id, &manifest.paths, self.restriction_product, diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 8c8a98c44fe..8bfa934b474 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -14,7 +14,6 @@ use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; use codex_exec_server::EnvironmentPathRef; use codex_exec_server::ExecutorFileSystem; -use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -151,11 +150,10 @@ impl fmt::Display for SkillParseError { impl Error for SkillParseError {} pub struct SkillRoot { - pub path: AbsolutePathBuf, + pub path: EnvironmentPathRef, pub scope: SkillScope, - pub file_system: Arc, pub plugin_id: Option, - pub plugin_root: Option, + pub plugin_root: Option, } pub async fn load_skills_from_roots(roots: I) -> SkillLoadOutcome @@ -166,18 +164,9 @@ where let mut skill_roots: Vec = Vec::new(); let mut skill_root_by_path: HashMap = HashMap::new(); for root in roots { - let fs = root.file_system; - let root_path_ref = - canonicalize_for_skill_identity(&EnvironmentPathRef::new(Arc::clone(&fs), root.path)) - .await; - let plugin_root = match root.plugin_root { - Some(plugin_root) => Some( - canonicalize_for_skill_identity(&EnvironmentPathRef::new( - Arc::clone(&fs), - plugin_root, - )) - .await, - ), + let root_path_ref = canonicalize_for_skill_identity(&root.path).await; + let plugin_root = match root.plugin_root.as_ref() { + Some(plugin_root) => Some(canonicalize_for_skill_identity(plugin_root).await), None => None, }; let skills_before_root = outcome.skills.len(); @@ -235,18 +224,20 @@ where } pub(crate) async fn skill_roots( - fs: Option>, + env_path: Option<&EnvironmentPathRef>, + local_file_system: Option<&Arc>, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, plugin_skill_roots: Vec, extra_skill_roots: Vec, ) -> Vec { + // `env_path` owns workspace/repo-relative skill discovery for the selected environment. + // `local_file_system` owns client-local system/user/plugin roots. let home_dir = home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); skill_roots_with_home_dir( - fs, + env_path, + local_file_system, config_layer_stack, - cwd, home_dir.as_ref(), plugin_skill_roots, extra_skill_roots, @@ -255,38 +246,47 @@ pub(crate) async fn skill_roots( } async fn skill_roots_with_home_dir( - fs: Option>, + env_path: Option<&EnvironmentPathRef>, + local_file_system: Option<&Arc>, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, extra_skill_roots: Vec, ) -> Vec { - let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, fs.clone()); - roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot { - path: root.path, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some(root.plugin_id), - plugin_root: Some(root.plugin_root), - })); - roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot { - path, - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - })); - roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); + // Assemble one precedence-ordered root list from the two authorities above, then dedupe before + // any reads happen so downstream loading stays oblivious to local-vs-selected-env routing. + let mut roots = env_path + .into_iter() + .flat_map(|env_path| repo_config_skill_roots(env_path, config_layer_stack)) + .collect::>(); + roots.extend(local_skill_roots( + local_file_system, + config_layer_stack, + home_dir, + plugin_skill_roots, + extra_skill_roots, + )); + if let Some(env_path) = env_path { + roots.extend(repo_agents_skill_roots(env_path, config_layer_stack).await); + } dedupe_skill_roots_by_path(&mut roots); roots } -fn skill_roots_from_layer_stack_inner( +fn local_skill_roots( + local_file_system: Option<&Arc>, config_layer_stack: &ConfigLayerStack, home_dir: Option<&AbsolutePathBuf>, - repo_fs: Option>, + plugin_skill_roots: Vec, + extra_skill_roots: Vec, ) -> Vec { + // These roots are absolute paths on the client machine, not paths inside the selected + // workspace environment. Bind them to the local-authority filesystem so remote turns keep + // loading local installed/bundled/plugin skills without reading those paths remotely. + let Some(local_file_system) = local_file_system else { + // Some focused tests intentionally omit local authority to prove repo-only loading. + return Vec::new(); + }; let mut roots = Vec::new(); for layer in config_layer_stack.get_layers( @@ -298,24 +298,17 @@ fn skill_roots_from_layer_stack_inner( }; match &layer.name { - ConfigLayerSource::Project { .. } => { - if let Some(repo_fs) = &repo_fs { - roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), - scope: SkillScope::Repo, - file_system: Arc::clone(repo_fs), - plugin_id: None, - plugin_root: None, - }); - } - } + ConfigLayerSource::Project { .. } => {} ConfigLayerSource::User { .. } => { // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward - // compatibility. + // compatibility. These are client-local absolute paths even when the active repo + // environment is remote, so bind them to the available local filesystem only. roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: EnvironmentPathRef::new( + Arc::clone(local_file_system), + config_folder.join(SKILLS_DIR_NAME), + ), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -323,9 +316,11 @@ fn skill_roots_from_layer_stack_inner( // `$HOME/.agents/skills` (user-installed skills). if let Some(home_dir) = home_dir { roots.push(SkillRoot { - path: home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + path: EnvironmentPathRef::new( + Arc::clone(local_file_system), + home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + ), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -334,9 +329,11 @@ fn skill_roots_from_layer_stack_inner( // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a // special case (not a config layer). roots.push(SkillRoot { - path: system_cache_root_dir(&config_folder), + path: EnvironmentPathRef::new( + Arc::clone(local_file_system), + system_cache_root_dir(&config_folder), + ), scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -345,9 +342,11 @@ fn skill_roots_from_layer_stack_inner( // The system config layer lives under `/etc/codex/` on Unix, so treat // `/etc/codex/skills` as admin-scoped skills. roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: EnvironmentPathRef::new( + Arc::clone(local_file_system), + config_folder.join(SKILLS_DIR_NAME), + ), scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -359,28 +358,77 @@ fn skill_roots_from_layer_stack_inner( } } + roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot { + // Plugin discovery is currently local-only; multi-env plugin skill loading should be an + // explicit future change rather than inheriting the selected repo environment here. + path: EnvironmentPathRef::new(Arc::clone(local_file_system), root.path), + scope: SkillScope::User, + plugin_id: Some(root.plugin_id), + plugin_root: Some(EnvironmentPathRef::new( + Arc::clone(local_file_system), + root.plugin_root, + )), + })); + roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot { + path: EnvironmentPathRef::new(Arc::clone(local_file_system), path), + scope: SkillScope::User, + plugin_id: None, + plugin_root: None, + })); + roots } +fn repo_config_skill_roots( + env_path: &EnvironmentPathRef, + config_layer_stack: &ConfigLayerStack, +) -> Vec { + // Project config layers describe workspace-local `.codex/skills` directories. Their absolute + // paths only make sense inside the selected environment, so keep them bound to `env_path` + // rather than the optional local filesystem used for client-local system/user/plugin roots. + config_layer_stack + .get_layers( + ConfigLayerStackOrdering::HighestPrecedenceFirst, + /*include_disabled*/ true, + ) + .into_iter() + .filter_map(|layer| match &layer.name { + ConfigLayerSource::Project { .. } => layer.config_folder().map(|config_folder| { + SkillRoot { + // Project and repo `.agents` roots belong to the selected environment because + // their absolute paths are only meaningful within that environment's cwd/repo. + path: env_path.with_path(config_folder.join(SKILLS_DIR_NAME)), + scope: SkillScope::Repo, + plugin_id: None, + plugin_root: None, + } + }), + ConfigLayerSource::User { .. } + | ConfigLayerSource::System { .. } + | ConfigLayerSource::Mdm { .. } + | ConfigLayerSource::SessionFlags + | ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } + | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => None, + }) + .collect() +} + async fn repo_agents_skill_roots( - fs: Option>, + env_path: &EnvironmentPathRef, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, ) -> Vec { - let Some(fs) = fs else { - return Vec::new(); - }; + // Discover repo `.agents/skills` folders by walking from the selected environment's project + // root to its cwd; this must never consult the local filesystem for remote workspaces. let project_root_markers = project_root_markers_from_stack(config_layer_stack); - let project_root = find_project_root(fs.as_ref(), cwd, &project_root_markers).await; - let dirs = dirs_between_project_root_and_cwd(cwd, &project_root); + let project_root = find_project_root(env_path, &project_root_markers).await; + let dirs = dirs_between_project_root_and_cwd(env_path.path(), project_root.path()); let mut roots = Vec::new(); for dir in dirs { - let agents_skills = dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); - match fs.get_metadata(&agents_skills, /*sandbox*/ None).await { + let agents_skills = env_path.with_path(dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME)); + match agents_skills.metadata(/*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => roots.push(SkillRoot { path: agents_skills, scope: SkillScope::Repo, - file_system: Arc::clone(&fs), plugin_id: None, plugin_root: None, }), @@ -389,7 +437,7 @@ async fn repo_agents_skill_roots( Err(err) => { tracing::warn!( "failed to stat repo skills root {}: {err:#}", - agents_skills.display() + agents_skills.path().display() ); } } @@ -420,24 +468,23 @@ fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec } async fn find_project_root( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, + cwd: &EnvironmentPathRef, project_root_markers: &[String], -) -> AbsolutePathBuf { +) -> EnvironmentPathRef { if project_root_markers.is_empty() { return cwd.clone(); } - for ancestor in cwd.ancestors() { + for ancestor in cwd.path().ancestors() { for marker in project_root_markers { - let marker_path = ancestor.join(marker); - match fs.get_metadata(&marker_path, /*sandbox*/ None).await { - Ok(_) => return ancestor, + let marker_path = cwd.with_path(ancestor.join(marker)); + match marker_path.metadata(/*sandbox*/ None).await { + Ok(_) => return cwd.with_path(ancestor), Err(err) if err.kind() == io::ErrorKind::NotFound => {} Err(err) => { tracing::warn!( "failed to stat project root marker {}: {err:#}", - marker_path.display() + marker_path.path().display() ); } } @@ -470,12 +517,7 @@ fn dirs_between_project_root_and_cwd( fn dedupe_skill_roots_by_path(roots: &mut Vec) { let mut seen: HashSet = HashSet::new(); - roots.retain(|root| { - seen.insert(EnvironmentPathRef::new( - Arc::clone(&root.file_system), - root.path.clone(), - )) - }); + roots.retain(|root| seen.insert(root.path.clone())); } async fn canonicalize_for_skill_identity(path: &EnvironmentPathRef) -> EnvironmentPathRef { @@ -491,14 +533,13 @@ async fn discover_skills_under_root( plugin_root: Option<&EnvironmentPathRef>, outcome: &mut SkillLoadOutcome, ) { - let fs = root.file_system(); let root = canonicalize_for_skill_identity(root).await; let plugin_root = match plugin_root { Some(plugin_root) => Some(canonicalize_for_skill_identity(plugin_root).await), None => None, }; - match fs.get_metadata(root.path(), /*sandbox*/ None).await { + match root.metadata(/*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => {} Ok(_) => return, Err(err) if err.kind() == io::ErrorKind::NotFound => return, @@ -512,10 +553,10 @@ async fn discover_skills_under_root( } fn enqueue_dir( - queue: &mut VecDeque<(AbsolutePathBuf, usize)>, + queue: &mut VecDeque<(EnvironmentPathRef, usize)>, visited_dirs: &mut HashSet, truncated_by_dir_limit: &mut bool, - path: AbsolutePathBuf, + path: EnvironmentPathRef, depth: usize, ) { if depth > MAX_SCAN_DEPTH { @@ -525,7 +566,7 @@ async fn discover_skills_under_root( *truncated_by_dir_limit = true; return; } - if visited_dirs.insert(path.clone()) { + if visited_dirs.insert(path.path().clone()) { queue.push_back((path, depth)); } } @@ -539,14 +580,14 @@ async fn discover_skills_under_root( let mut visited_dirs: HashSet = HashSet::new(); visited_dirs.insert(root.path().clone()); - let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.path().clone(), 0)]); + let mut queue: VecDeque<(EnvironmentPathRef, usize)> = VecDeque::from([(root.clone(), 0)]); let mut truncated_by_dir_limit = false; while let Some((dir, depth)) = queue.pop_front() { - let entries = match fs.read_directory(&dir, /*sandbox*/ None).await { + let entries = match dir.read_directory(/*sandbox*/ None).await { Ok(entries) => entries, Err(e) => { - error!("failed to read skills dir {}: {e:#}", dir.display()); + error!("failed to read skills dir {}: {e:#}", dir.path().display()); continue; } }; @@ -557,11 +598,16 @@ async fn discover_skills_under_root( continue; } - let path = dir.join(&file_name); - let metadata = match fs.get_metadata(&path, /*sandbox*/ None).await { + let Ok(path) = dir.join(Path::new(&file_name)).await else { + continue; + }; + let metadata = match path.metadata(/*sandbox*/ None).await { Ok(metadata) => metadata, Err(e) => { - error!("failed to stat skills path {}: {e:#}", path.display()); + error!( + "failed to stat skills path {}: {e:#}", + path.path().display() + ); continue; } }; @@ -570,13 +616,9 @@ async fn discover_skills_under_root( if !follow_symlinks { continue; } - match fs.read_directory(&path, /*sandbox*/ None).await { + match path.read_directory(/*sandbox*/ None).await { Ok(_) => { - let resolved_dir = - canonicalize_for_skill_identity(&root.with_path(path.clone())) - .await - .path() - .clone(); + let resolved_dir = canonicalize_for_skill_identity(&path).await; enqueue_dir( &mut queue, &mut visited_dirs, @@ -593,7 +635,7 @@ async fn discover_skills_under_root( Err(err) => { error!( "failed to read skills symlink dir {}: {err:#}", - path.display() + path.path().display() ); } } @@ -601,10 +643,7 @@ async fn discover_skills_under_root( } if metadata.is_directory { - let resolved_dir = canonicalize_for_skill_identity(&root.with_path(path.clone())) - .await - .path() - .clone(); + let resolved_dir = canonicalize_for_skill_identity(&path).await; enqueue_dir( &mut queue, &mut visited_dirs, @@ -616,21 +655,14 @@ async fn discover_skills_under_root( } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file( - &root.with_path(path.clone()), - scope, - plugin_id, - plugin_root.as_ref(), - ) - .await - { + match parse_skill_file(&path, scope, plugin_id, plugin_root.as_ref()).await { Ok(skill) => { outcome.skills.push(skill); } Err(err) => { if scope != SkillScope::System { outcome.errors.push(SkillError { - path: path.clone(), + path: path.path().clone(), message: err.to_string(), }); } @@ -650,16 +682,16 @@ async fn discover_skills_under_root( } async fn parse_skill_file( - path: &EnvironmentPathRef, + skill_path: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, plugin_root: Option<&EnvironmentPathRef>, ) -> Result { - let fs = path.file_system(); - let contents = fs - .read_file_text(path.path(), /*sandbox*/ None) + let contents = skill_path + .read_to_string(/*sandbox*/ None) .await .map_err(SkillParseError::Read)?; + let path = skill_path.path(); let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?; @@ -671,8 +703,8 @@ async fn parse_skill_file( .as_deref() .map(sanitize_single_line) .filter(|value| !value.is_empty()) - .unwrap_or_else(|| default_skill_name(path.path())); - let name = namespaced_skill_name(fs.as_ref(), path.path(), &base_name).await; + .unwrap_or_else(|| default_skill_name(path)); + let name = namespaced_skill_name(skill_path.file_system().as_ref(), path, &base_name).await; let description = parsed .description .as_deref() @@ -688,12 +720,7 @@ async fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata( - fs.as_ref(), - path.path(), - plugin_root.map(EnvironmentPathRef::path), - ) - .await; + } = load_skill_metadata(skill_path, plugin_root).await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -705,7 +732,7 @@ async fn parse_skill_file( )?; } - let resolved_path = canonicalize_for_skill_identity(path).await; + let resolved_path = canonicalize_for_skill_identity(skill_path).await; Ok(SkillMetadata { name, @@ -745,18 +772,22 @@ async fn namespaced_skill_name( } async fn load_skill_metadata( - fs: &dyn ExecutorFileSystem, - skill_path: &AbsolutePathBuf, - plugin_root: Option<&AbsolutePathBuf>, + skill_path: &EnvironmentPathRef, + plugin_root: Option<&EnvironmentPathRef>, ) -> LoadedSkillMetadata { // Fail open: optional metadata should not block loading SKILL.md. - let Some(skill_dir) = skill_path.parent() else { + let Ok(Some(skill_dir)) = skill_path.parent().await else { return LoadedSkillMetadata::default(); }; - let metadata_path = skill_dir - .join(SKILLS_METADATA_DIR) - .join(SKILLS_METADATA_FILENAME); - match fs.get_metadata(&metadata_path, /*sandbox*/ None).await { + let Ok(metadata_path) = skill_dir.join( + Path::new(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME) + .as_path(), + ) + .await else { + return LoadedSkillMetadata::default(); + }; + match metadata_path.metadata(/*sandbox*/ None).await { Ok(metadata) if metadata.is_file => {} Ok(_) => return LoadedSkillMetadata::default(), Err(error) if error.kind() == io::ErrorKind::NotFound => { @@ -765,19 +796,19 @@ async fn load_skill_metadata( Err(error) => { tracing::warn!( "ignoring {path}: failed to stat {label}: {error}", - path = metadata_path.display(), + path = metadata_path.path().display(), label = SKILLS_METADATA_FILENAME ); return LoadedSkillMetadata::default(); } } - let contents = match fs.read_file_text(&metadata_path, /*sandbox*/ None).await { + let contents = match metadata_path.read_to_string(/*sandbox*/ None).await { Ok(contents) => contents, Err(error) => { tracing::warn!( "ignoring {path}: failed to read {label}: {error}", - path = metadata_path.display(), + path = metadata_path.path().display(), label = SKILLS_METADATA_FILENAME ); return LoadedSkillMetadata::default(); @@ -785,13 +816,13 @@ async fn load_skill_metadata( }; let parsed: SkillMetadataFile = { - let _guard = AbsolutePathBufGuard::new(skill_dir.as_path()); + let _guard = AbsolutePathBufGuard::new(skill_dir.path().as_path()); match serde_yaml::from_str(&contents) { Ok(parsed) => parsed, Err(error) => { tracing::warn!( "ignoring {path}: invalid {label}: {error}", - path = metadata_path.display(), + path = metadata_path.path().display(), label = SKILLS_METADATA_FILENAME ); return LoadedSkillMetadata::default(); @@ -805,7 +836,11 @@ async fn load_skill_metadata( policy, } = parsed; LoadedSkillMetadata { - interface: resolve_interface(interface, &skill_dir, plugin_root), + interface: resolve_interface( + interface, + skill_dir.path(), + plugin_root.map(EnvironmentPathRef::path), + ), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), } @@ -1098,10 +1133,12 @@ pub(crate) async fn skill_roots_from_layer_stack( cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, ) -> Vec { + let path_ref = EnvironmentPathRef::new(fs, cwd.clone()); + let local_file_system = path_ref.file_system(); skill_roots_with_home_dir( - Some(fs), + Some(&path_ref), + Some(&local_file_system), config_layer_stack, - cwd, home_dir, Vec::new(), Vec::new(), diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index aa3cf2e8b8a..d90e8368ae8 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -146,11 +146,14 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } +fn local_env_path(path: AbsolutePathBuf) -> EnvironmentPathRef { + EnvironmentPathRef::local(path) +} + fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot { SkillRoot { - path, + path: local_env_path(path), scope, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, } @@ -199,7 +202,7 @@ async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_sys ) .await .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) + .map(|root| (root.scope, root.path.path().to_path_buf())) .collect::>(); assert_eq!( @@ -268,7 +271,7 @@ async fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyh ) .await .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) + .map(|root| (root.scope, root.path.path().to_path_buf())) .collect::>(); assert_eq!( @@ -290,6 +293,57 @@ async fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyh Ok(()) } +#[tokio::test] +async fn skill_roots_bind_repo_and_local_roots_to_their_own_file_systems() -> anyhow::Result<()> { + let codex_home = tempfile::tempdir()?; + let project_root = codex_home.path().join("workspace"); + fs::create_dir_all(project_root.join(".git"))?; + fs::create_dir_all(project_root.join(REPO_ROOT_CONFIG_DIR_NAME))?; + let cfg = make_config_for_cwd(&codex_home, project_root.clone()).await; + + let env_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let local_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let env_path = EnvironmentPathRef::new(env_file_system.clone(), cfg.cwd.clone()); + let roots = super::skill_roots( + Some(&env_path), + Some(&local_file_system), + &cfg.config_layer_stack, + Vec::new(), + Vec::new(), + ) + .await; + + let repo_root = roots + .iter() + .find(|root| root.scope == SkillScope::Repo) + .expect("repo root"); + let user_root = roots + .iter() + .find(|root| root.scope == SkillScope::User) + .expect("user root"); + assert!(Arc::ptr_eq(&repo_root.path.file_system(), &env_file_system)); + assert!(Arc::ptr_eq( + &user_root.path.file_system(), + &local_file_system + )); + + let roots_without_local = super::skill_roots( + Some(&env_path), + /*local_file_system*/ None, + &cfg.config_layer_stack, + Vec::new(), + Vec::new(), + ) + .await; + assert!( + roots_without_local + .iter() + .all(|root| root.scope == SkillScope::Repo) + ); + + Ok(()) +} + #[tokio::test] async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { let tmp = tempfile::tempdir()?; @@ -867,11 +921,10 @@ interface: let plugin_root_abs = plugin_root.abs(); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: local_env_path(plugin_root.join("skills").abs()), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_root: Some(plugin_root_abs.clone()), + plugin_root: Some(local_env_path(plugin_root_abs.clone())), }]) .await; @@ -925,11 +978,10 @@ interface: ); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: local_env_path(plugin_root.join("skills").abs()), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_root: Some(plugin_root.abs()), + plugin_root: Some(local_env_path(plugin_root.abs())), }]) .await; @@ -1293,11 +1345,10 @@ async fn namespaces_plugin_skills_using_plugin_name() { .unwrap(); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: local_env_path(plugin_root.join("skills").abs()), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some("sample@test".to_string()), - plugin_root: Some(plugin_root.abs()), + plugin_root: Some(local_env_path(plugin_root.abs())), }]) .await; @@ -1663,16 +1714,14 @@ async fn preserves_same_absolute_skill_path_from_distinct_file_systems() { let outcome = load_skills_from_roots([ SkillRoot { - path: shared_path.clone(), + path: EnvironmentPathRef::new(first_file_system, shared_path.clone()), scope: SkillScope::Repo, - file_system: first_file_system, plugin_id: None, plugin_root: None, }, SkillRoot { - path: shared_path, + path: EnvironmentPathRef::new(second_file_system, shared_path), scope: SkillScope::Repo, - file_system: second_file_system, plugin_id: None, plugin_root: None, }, @@ -1698,16 +1747,14 @@ fn dedupe_skill_roots_preserves_same_path_from_distinct_file_systems() { let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); let mut roots = vec![ SkillRoot { - path: shared_path.clone(), + path: EnvironmentPathRef::new(first_file_system, shared_path.clone()), scope: SkillScope::Repo, - file_system: first_file_system, plugin_id: None, plugin_root: None, }, SkillRoot { - path: shared_path, + path: EnvironmentPathRef::new(second_file_system, shared_path), scope: SkillScope::Repo, - file_system: second_file_system, plugin_id: None, plugin_root: None, }, @@ -1991,10 +2038,12 @@ async fn skill_roots_include_admin_with_lowest_priority() { let codex_home = tempfile::tempdir().expect("tempdir"); let cfg = make_config(&codex_home).await; + let cwd = local_env_path(cfg.cwd.clone()); + let local_file_system = cwd.file_system(); let scopes: Vec = super::skill_roots( - Some(Arc::clone(&LOCAL_FS)), + Some(&cwd), + Some(&local_file_system), &cfg.config_layer_stack, - &cfg.cwd, Vec::new(), Vec::new(), ) diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 369f21087d0..a46a7c12b11 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -4,8 +4,6 @@ use std::sync::Arc; use std::sync::RwLock; use codex_config::ConfigLayerStack; -use codex_exec_server::EnvironmentPathRef; -use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -24,15 +22,31 @@ use crate::loader::skill_roots; use crate::system::install_system_skills; use crate::system::uninstall_system_skills; use codex_config::SkillsConfig; +use codex_exec_server::EnvironmentPathRef; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::LOCAL_FS; -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct SkillsLoadInput { pub cwd: AbsolutePathBuf, + local_file_system: Option>, pub effective_skill_roots: Vec, pub config_layer_stack: ConfigLayerStack, pub bundled_skills_enabled: bool, } +impl std::fmt::Debug for SkillsLoadInput { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SkillsLoadInput") + .field("cwd", &self.cwd) + .field("has_local_file_system", &self.local_file_system.is_some()) + .field("effective_skill_roots", &self.effective_skill_roots) + .field("config_layer_stack", &self.config_layer_stack) + .field("bundled_skills_enabled", &self.bundled_skills_enabled) + .finish() + } +} + impl SkillsLoadInput { pub fn new( cwd: AbsolutePathBuf, @@ -42,18 +56,41 @@ impl SkillsLoadInput { ) -> Self { Self { cwd, + local_file_system: Some(Arc::clone(&LOCAL_FS)), effective_skill_roots, config_layer_stack, bundled_skills_enabled, } } + + #[cfg(test)] + fn with_local_file_system( + mut self, + local_file_system: Option>, + ) -> Self { + self.local_file_system = local_file_system; + self + } + + fn authorities( + &self, + fs: Option>, + ) -> ( + Option, + Option>, + ) { + ( + fs.map(|fs| EnvironmentPathRef::new(fs, self.cwd.clone())), + self.local_file_system.clone(), + ) + } } pub struct SkillsManager { codex_home: AbsolutePathBuf, restriction_product: Option, extra_roots: RwLock>, - cache_by_cwd: RwLock>, + cache_by_path_ref: RwLock>, cache_by_config: RwLock>, } @@ -71,7 +108,7 @@ impl SkillsManager { codex_home, restriction_product, extra_roots: RwLock::new(Vec::new()), - cache_by_cwd: RwLock::new(HashMap::new()), + cache_by_path_ref: RwLock::new(HashMap::new()), cache_by_config: RwLock::new(HashMap::new()), }; if !bundled_skills_enabled { @@ -127,10 +164,11 @@ impl SkillsManager { input: &SkillsLoadInput, fs: Option>, ) -> Vec { + let (env_path, local_file_system) = input.authorities(fs); let mut roots = skill_roots( - fs, + env_path.as_ref(), + local_file_system.as_ref(), &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -147,20 +185,21 @@ impl SkillsManager { force_reload: bool, fs: Option>, ) -> SkillLoadOutcome { - let cwd_path_ref = fs - .as_ref() - .map(|fs| EnvironmentPathRef::new(Arc::clone(fs), input.cwd.clone())); + let (env_path, local_file_system) = input.authorities(fs); + let path_ref_cache_key = SkillsPathCacheKey { + env_path: env_path.clone(), + local_file_system: local_file_system.as_ref().map(ExecutorFileSystemRef::new), + }; if !force_reload - && let Some(cwd_path_ref) = cwd_path_ref.as_ref() - && let Some(outcome) = self.cached_outcome_for_cwd(cwd_path_ref) + && let Some(outcome) = self.cached_outcome_for_path_ref(&path_ref_cache_key) { return outcome; } let mut roots = skill_roots( - fs.clone(), + env_path.as_ref(), + local_file_system.as_ref(), &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -170,13 +209,11 @@ impl SkillsManager { } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; - if let Some(cwd_path_ref) = cwd_path_ref { - let mut cache = self - .cache_by_cwd - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(cwd_path_ref, outcome.clone()); - } + let mut cache = self + .cache_by_path_ref + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(path_ref_cache_key, outcome.clone()); outcome } @@ -194,9 +231,9 @@ impl SkillsManager { } pub fn clear_cache(&self) { - let cleared_cwd = { + let cleared_path_refs = { let mut cache = self - .cache_by_cwd + .cache_by_path_ref .write() .unwrap_or_else(std::sync::PoisonError::into_inner); let cleared = cache.len(); @@ -212,14 +249,17 @@ impl SkillsManager { cache.clear(); cleared }; - let cleared = cleared_cwd + cleared_config; + let cleared = cleared_path_refs + cleared_config; info!("skills cache cleared ({cleared} entries)"); } - fn cached_outcome_for_cwd(&self, cwd: &EnvironmentPathRef) -> Option { - match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), + fn cached_outcome_for_path_ref( + &self, + cache_key: &SkillsPathCacheKey, + ) -> Option { + match self.cache_by_path_ref.read() { + Ok(cache) => cache.get(cache_key).cloned(), + Err(err) => err.into_inner().get(cache_key).cloned(), } } @@ -241,6 +281,41 @@ impl SkillsManager { } } +#[derive(Clone, Debug, Eq, Hash, PartialEq)] +struct SkillsPathCacheKey { + env_path: Option, + local_file_system: Option, +} + +#[derive(Clone)] +struct ExecutorFileSystemRef(Arc); + +impl ExecutorFileSystemRef { + fn new(file_system: &Arc) -> Self { + Self(Arc::clone(file_system)) + } +} + +impl PartialEq for ExecutorFileSystemRef { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.0, &other.0) + } +} + +impl Eq for ExecutorFileSystemRef {} + +impl std::hash::Hash for ExecutorFileSystemRef { + fn hash(&self, state: &mut H) { + std::hash::Hash::hash(&(Arc::as_ptr(&self.0) as *const () as usize), state); + } +} + +impl std::fmt::Debug for ExecutorFileSystemRef { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ExecutorFileSystemRef").finish() + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { roots: Vec<(EnvironmentPathRef, u8, Option)>, @@ -283,11 +358,7 @@ fn config_skills_cache_key( SkillScope::System => 2, SkillScope::Admin => 3, }; - ( - EnvironmentPathRef::new(Arc::clone(&root.file_system), root.path.clone()), - scope_rank, - root.plugin_id.clone(), - ) + (root.path.clone(), scope_rank, root.plugin_id.clone()) }) .collect(), skill_config_rules: skill_config_rules.clone(), diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index fcd5e9c828e..29090ba5f8f 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -92,6 +92,23 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { } } +fn local_skills_input( + cwd: AbsolutePathBuf, + effective_skill_roots: Vec, + config_layer_stack: ConfigLayerStack, +) -> SkillsLoadInput { + SkillsLoadInput::new( + cwd, + effective_skill_roots, + config_layer_stack.clone(), + bundled_skills_enabled_from_stack(&config_layer_stack), + ) +} + +fn local_file_system() -> Option> { + Some(Arc::clone(&LOCAL_FS)) +} + fn write_demo_skill(tempdir: &TempDir) -> PathBuf { let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); fs::create_dir_all(skill_path.parent().expect("skill path should have parent")) @@ -169,14 +186,13 @@ async fn skills_for_config_with_stack( config_layer_stack: &ConfigLayerStack, effective_skill_roots: &[PluginSkillRoot], ) -> SkillLoadOutcome { - let skills_input = SkillsLoadInput::new( + let skills_input = local_skills_input( cwd.path().abs(), effective_skill_roots.to_vec(), config_layer_stack.clone(), - bundled_skills_enabled_from_stack(config_layer_stack), ); skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS))) + .skills_for_config(&skills_input, local_file_system()) .await } @@ -282,17 +298,12 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { /*bundled_skills_enabled*/ true, ); - let skills_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let empty_outcome = skills_manager .skills_for_cwd( &skills_input, /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; assert!( @@ -316,7 +327,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { .skills_for_cwd( &skills_input, /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; assert!( @@ -331,7 +342,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { .skills_for_cwd( &skills_input, /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; assert_eq!(replaced_outcome.errors, Vec::new()); @@ -471,12 +482,7 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); - let skills_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let skills_manager = SkillsManager::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, @@ -486,7 +492,7 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { .skills_for_cwd( &skills_input, /*force_reload*/ true, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; @@ -505,7 +511,7 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { } #[tokio::test] -async fn skills_for_cwd_without_fs_skips_repo_roots() { +async fn skills_for_cwd_without_local_fs_skips_local_roots() { let codex_home = tempfile::tempdir().expect("tempdir"); let cwd = tempfile::tempdir().expect("tempdir"); let repo_dot_codex = cwd.path().join(".codex"); @@ -534,12 +540,66 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() { ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); - let skills_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), + let skills_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack.clone()) + .with_local_file_system(/*local_file_system*/ None); + let skills_manager = SkillsManager::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + + let outcome = skills_manager + .skills_for_cwd( + &skills_input, + /*force_reload*/ true, + local_file_system(), + ) + .await; + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors ); + let loaded_names = outcome + .skills + .iter() + .map(|skill| skill.name.as_str()) + .collect::>(); + assert!(!loaded_names.contains("user-skill")); + assert!(loaded_names.contains("repo-skill")); +} + +#[tokio::test] +async fn skills_for_cwd_without_env_path_still_loads_local_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let repo_dot_codex = cwd.path().join(".codex"); + fs::create_dir_all(&repo_dot_codex).expect("create repo config dir"); + + write_user_skill(&codex_home, "user", "user-skill", "from local user root"); + let repo_skill_dir = repo_dot_codex.join("skills/repo"); + fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir"); + fs::write( + repo_skill_dir.join("SKILL.md"), + "---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n", + ) + .expect("write repo skill"); + + let config_layer_stack = ConfigLayerStack::new( + vec![ + user_config_layer(&codex_home, ""), + ConfigLayerEntry::new( + ConfigLayerSource::Project { + dot_codex_folder: repo_dot_codex.abs(), + }, + toml::Value::Table(toml::map::Map::new()), + ), + ], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + let skills_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack); let skills_manager = SkillsManager::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, @@ -615,17 +675,12 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { /*bundled_skills_enabled*/ true, ); let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; - let base_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let base_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let outcome_a = skills_manager .skills_for_cwd( &base_input, /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; assert!( @@ -641,7 +696,7 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { .skills_for_cwd( &base_input, /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; assert!( @@ -652,11 +707,7 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { ); let outcome_reloaded = skills_manager - .skills_for_cwd( - &base_input, - /*force_reload*/ true, - Some(Arc::clone(&LOCAL_FS)), - ) + .skills_for_cwd(&base_input, /*force_reload*/ true, local_file_system()) .await; assert!( outcome_reloaded @@ -934,6 +985,44 @@ fn disabled_paths_for_skills_allows_name_selector_to_override_path_selector() { ); } +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_disables_same_raw_path_in_each_file_system() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = write_demo_skill(&tempdir); + let mut second_skill = test_skill("demo-skill", skill_path.clone()); + second_skill.source_path = EnvironmentPathRef::new( + Arc::new(codex_exec_server::LocalFileSystem::unsandboxed()), + second_skill.path_to_skills_md.clone(), + ); + let local_skill = test_skill("demo-skill", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: user_file, + profile: None, + }, + toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) + .expect("user layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths( + &[second_skill.clone(), local_skill.clone()], + &skill_config_rules + ), + HashSet::from([second_skill.source_path, local_skill.source_path]) + ); +} + #[cfg_attr(windows, ignore)] #[tokio::test] async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { @@ -956,18 +1045,13 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let parent_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - parent_stack.clone(), - bundled_skills_enabled_from_stack(&parent_stack), - ); + let parent_input = local_skills_input(cwd.path().abs(), Vec::new(), parent_stack.clone()); let parent_outcome = skills_manager .skills_for_cwd( &parent_input, /*force_reload*/ true, - Some(Arc::clone(&LOCAL_FS)), + local_file_system(), ) .await; let parent_skill = parent_outcome From 33c234ac3f32f0ba7c07062a7f46f3c998184ce2 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 29 May 2026 13:49:36 -0700 Subject: [PATCH 2/2] core-skills: skip cwd cache without env path --- codex-rs/core-skills/src/loader.rs | 14 ++++--- codex-rs/core-skills/src/manager.rs | 19 ++++++---- codex-rs/core-skills/src/manager_tests.rs | 46 +++++++++++++++++++++++ 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 8bfa934b474..8a018324732 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -779,12 +779,14 @@ async fn load_skill_metadata( let Ok(Some(skill_dir)) = skill_path.parent().await else { return LoadedSkillMetadata::default(); }; - let Ok(metadata_path) = skill_dir.join( - Path::new(SKILLS_METADATA_DIR) - .join(SKILLS_METADATA_FILENAME) - .as_path(), - ) - .await else { + let Ok(metadata_path) = skill_dir + .join( + Path::new(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME) + .as_path(), + ) + .await + else { return LoadedSkillMetadata::default(); }; match metadata_path.metadata(/*sandbox*/ None).await { diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index a46a7c12b11..a92d582f480 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -186,12 +186,13 @@ impl SkillsManager { fs: Option>, ) -> SkillLoadOutcome { let (env_path, local_file_system) = input.authorities(fs); - let path_ref_cache_key = SkillsPathCacheKey { + let path_ref_cache_key = env_path.as_ref().map(|_| SkillsPathCacheKey { env_path: env_path.clone(), local_file_system: local_file_system.as_ref().map(ExecutorFileSystemRef::new), - }; + }); if !force_reload - && let Some(outcome) = self.cached_outcome_for_path_ref(&path_ref_cache_key) + && let Some(path_ref_cache_key) = path_ref_cache_key.as_ref() + && let Some(outcome) = self.cached_outcome_for_path_ref(path_ref_cache_key) { return outcome; } @@ -209,11 +210,13 @@ impl SkillsManager { } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; - let mut cache = self - .cache_by_path_ref - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(path_ref_cache_key, outcome.clone()); + if let Some(path_ref_cache_key) = path_ref_cache_key { + let mut cache = self + .cache_by_path_ref + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(path_ref_cache_key, outcome.clone()); + } outcome } diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 29090ba5f8f..0545fc54e49 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -770,6 +770,52 @@ async fn skills_for_cwd_keeps_cache_entries_bound_to_file_system() { assert_ne!(first_skill.source_path, second_skill.source_path); } +#[tokio::test] +async fn skills_for_cwd_skips_cache_without_environment_path() { + let manager_codex_home = tempfile::tempdir().expect("tempdir"); + let first_codex_home = tempfile::tempdir().expect("tempdir"); + let second_codex_home = tempfile::tempdir().expect("tempdir"); + let first_cwd = tempfile::tempdir().expect("tempdir"); + let second_cwd = tempfile::tempdir().expect("tempdir"); + write_user_skill(&first_codex_home, "first", "first-skill", "first"); + write_user_skill(&second_codex_home, "second", "second-skill", "second"); + let skills_manager = SkillsManager::new( + manager_codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + let first_input = local_skills_input( + first_cwd.path().abs(), + Vec::new(), + config_stack(&first_codex_home, ""), + ); + let second_input = local_skills_input( + second_cwd.path().abs(), + Vec::new(), + config_stack(&second_codex_home, ""), + ); + + let first = skills_manager + .skills_for_cwd(&first_input, /*force_reload*/ false, /*fs*/ None) + .await; + let second = skills_manager + .skills_for_cwd(&second_input, /*force_reload*/ false, /*fs*/ None) + .await; + + assert!(first.skills.iter().any(|skill| skill.name == "first-skill")); + assert!( + second + .skills + .iter() + .any(|skill| skill.name == "second-skill") + ); + assert!( + second + .skills + .iter() + .all(|skill| skill.name != "first-skill") + ); +} + #[cfg_attr(windows, ignore)] #[test] fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() {