diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index 6f17e4614f4..4cf19259b3e 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -17,12 +17,12 @@ const SKILLS_LIST_CWD_CONCURRENCY: usize = 5; fn skills_to_info( skills: &[codex_core::skills::SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, ) -> Vec { skills .iter() .map(|skill| { - let enabled = !disabled_paths.contains(&skill.path_to_skills_md); + let enabled = !disabled_paths.contains(&skill.source_path); codex_app_server_protocol::SkillMetadata { name: skill.name.clone(), description: skill.description.clone(), @@ -513,15 +513,17 @@ impl CatalogRequestProcessor { .await; let skills_manager = self.thread_manager.skills_manager(); let plugins_manager = self.thread_manager.plugins_manager(); - let fs = self - .thread_manager - .environment_manager() - .default_environment() - .map(|environment| environment.get_filesystem()); + let environment_manager = self.thread_manager.environment_manager(); + // TODO: Reintroduce env-scoped `skills/list` only after cwd/config-layer discovery can run + // against the selected environment and the design decides which non-repo roots stay local + // versus follow that environment. + let selected_environment = environment_manager.default_environment(); + let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS)); let mut data = futures::stream::iter(cwds.into_iter().enumerate()) .map(|(index, cwd)| { let config = &config; - let fs = fs.clone(); + let local_file_system = local_file_system.clone(); + let selected_environment = selected_environment.clone(); let plugins_manager = &plugins_manager; let skills_manager = &skills_manager; async move { @@ -554,13 +556,19 @@ impl CatalogRequestProcessor { Vec::new() }; let skills_input = codex_core::skills::SkillsLoadInput::new( - cwd_abs.clone(), + selected_environment.as_ref().map(|environment| { + codex_exec_server::EnvironmentPathRef::new( + environment.get_filesystem(), + cwd_abs.clone(), + ) + }), + local_file_system, effective_skill_roots, config_layer_stack, config.bundled_skills_enabled(), ); let outcome = skills_manager - .skills_for_cwd(&skills_input, force_reload, fs) + .skills_for_cwd(&skills_input, force_reload) .await; let errors = errors_to_info(&outcome.errors); let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths); diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 57edb89c7ff..e87efcad3bc 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -9,6 +9,7 @@ use codex_core::ThreadManager; use codex_core::config::Config; use codex_core::skills::SkillsLoadInput; use codex_core::skills::SkillsManager; +use codex_exec_server::EnvironmentPathRef; use codex_file_watcher::FileWatcher; use codex_file_watcher::FileWatcherSubscriber; use codex_file_watcher::Receiver; @@ -100,22 +101,26 @@ impl SkillsWatcher { return WatchRegistration::default(); } + let root_path_ref = + EnvironmentPathRef::new(environment.get_filesystem(), config.cwd.clone()); + let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS)); let plugins_input = config.plugins_config_input(); let plugins_manager = thread_manager.plugins_manager(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let skills_input = SkillsLoadInput::new( - config.cwd.clone(), + Some(root_path_ref), + local_file_system, plugin_outcome.effective_plugin_skill_roots(), config.config_layer_stack.clone(), config.bundled_skills_enabled(), ); let roots = thread_manager .skills_manager() - .skill_roots_for_config(&skills_input, Some(environment.get_filesystem())) + .skill_roots_for_config(&skills_input) .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 9e0ef7c471e..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()), }) @@ -665,7 +665,10 @@ pub async fn load_plugin_skills( .into_iter() .filter(|skill| skill.matches_product_restriction_for_product(restriction_product)) .collect::>(); - let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules); + let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules) + .into_iter() + .map(|path| path.path().clone()) + .collect(); ResolvedPluginSkills { skills, 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/config_rules.rs b/codex-rs/core-skills/src/config_rules.rs index 92ad2ab1a68..65d3c401077 100644 --- a/codex-rs/core-skills/src/config_rules.rs +++ b/codex-rs/core-skills/src/config_rules.rs @@ -5,6 +5,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigLayerStackOrdering; use codex_config::SkillConfig; use codex_config::SkillsConfig; +use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use tracing::warn; @@ -71,28 +72,34 @@ pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> S pub fn resolve_disabled_skill_paths( skills: &[SkillMetadata], rules: &SkillConfigRules, -) -> HashSet { +) -> HashSet { let mut disabled_paths = HashSet::new(); for entry in &rules.entries { match &entry.selector { SkillConfigRuleSelector::Path(path) => { - if entry.enabled { - disabled_paths.remove(path); - } else { - disabled_paths.insert(path.clone()); + for source_path in skills + .iter() + .filter(|skill| skill.path_to_skills_md == *path) + .map(|skill| skill.source_path.clone()) + { + if entry.enabled { + disabled_paths.remove(&source_path); + } else { + disabled_paths.insert(source_path); + } } } SkillConfigRuleSelector::Name(name) => { - for path in skills + for source_path in skills .iter() .filter(|skill| skill.name == *name) - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) { if entry.enabled { - disabled_paths.remove(&path); + disabled_paths.remove(&source_path); } else { - disabled_paths.insert(path); + disabled_paths.insert(source_path); } } } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 45979fa5c49..ef22e435764 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,7 +1,3 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::sync::Arc; - use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::build_skill_name_counts; @@ -9,11 +5,13 @@ use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::TrackEventsContext; -use codex_exec_server::LOCAL_FS; +use codex_exec_server::EnvironmentPathRef; use codex_otel::SessionTelemetry; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; +use std::collections::HashMap; +use std::collections::HashSet; #[derive(Debug, Default)] pub struct SkillInjections { @@ -30,7 +28,7 @@ pub struct SkillInjection { pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], - loaded_skills: Option<&SkillLoadOutcome>, + _loaded_skills: Option<&SkillLoadOutcome>, otel: Option<&SessionTelemetry>, analytics_client: &AnalyticsEventsClient, tracking: TrackEventsContext, @@ -46,13 +44,7 @@ pub async fn build_skill_injections( let mut invocations = Vec::new(); for skill in mentioned_skills { - let fs = loaded_skills - .and_then(|outcome| outcome.file_system_for_skill(skill)) - .unwrap_or_else(|| Arc::clone(&LOCAL_FS)); - match fs - .read_file_text(&skill.path_to_skills_md, /*sandbox*/ None) - .await - { + match skill.source_path.read_to_string(/*sandbox*/ None).await { Ok(contents) => { emit_skill_injected_metric(otel, skill, "ok"); invocations.push(SkillInvocation { @@ -115,7 +107,7 @@ fn emit_skill_injected_metric( pub fn collect_explicit_skill_mentions( inputs: &[UserInput], skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0; @@ -128,7 +120,7 @@ pub fn collect_explicit_skill_mentions( }; let mut selected: Vec = Vec::new(); let mut seen_names: HashSet = HashSet::new(); - let mut seen_paths: HashSet = HashSet::new(); + let mut seen_paths: HashSet = HashSet::new(); let mut blocked_plain_names: HashSet = HashSet::new(); for input in inputs { @@ -138,18 +130,22 @@ pub fn collect_explicit_skill_mentions( continue; }; - if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) { - continue; - } - - if let Some(skill) = selection_context + let matching_skills = selection_context .skills .iter() - .find(|skill| skill.path_to_skills_md == path) + .filter(|skill| { + skill.path_to_skills_md == path + && !selection_context + .disabled_paths + .contains(&skill.source_path) + }) + .collect::>(); + if let [skill] = matching_skills.as_slice() + && !seen_paths.contains(&skill.source_path) { - seen_paths.insert(skill.path_to_skills_md.clone()); + seen_paths.insert(skill.source_path.clone()); seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); + selected.push((*skill).clone()); } } } @@ -173,7 +169,7 @@ pub fn collect_explicit_skill_mentions( struct SkillSelectionContext<'a> { skills: &'a [SkillMetadata], - disabled_paths: &'a HashSet, + disabled_paths: &'a HashSet, skill_name_counts: &'a HashMap, connector_slug_counts: &'a HashMap, } @@ -324,7 +320,7 @@ fn select_skills_from_mentions( blocked_plain_names: &HashSet, mentions: &ToolMentions<'_>, seen_names: &mut HashSet, - seen_paths: &mut HashSet, + seen_paths: &mut HashSet, selected: &mut Vec, ) { if mentions.is_empty() { @@ -342,18 +338,39 @@ fn select_skills_from_mentions( .map(normalize_skill_path) .collect(); + let mention_skill_path_counts = selection_context + .skills + .iter() + .filter(|skill| { + !selection_context + .disabled_paths + .contains(&skill.source_path) + }) + .filter_map(|skill| { + let path = skill.path_to_skills_md.to_string_lossy(); + mention_skill_paths + .contains(path.as_ref()) + .then_some(path.into_owned()) + }) + .fold(HashMap::new(), |mut counts, path| { + *counts.entry(path).or_insert(0usize) += 1; + counts + }); + for skill in selection_context.skills { if selection_context .disabled_paths - .contains(&skill.path_to_skills_md) - || seen_paths.contains(&skill.path_to_skills_md) + .contains(&skill.source_path) + || seen_paths.contains(&skill.source_path) { continue; } let path_str = skill.path_to_skills_md.to_string_lossy(); - if mention_skill_paths.contains(path_str.as_ref()) { - seen_paths.insert(skill.path_to_skills_md.clone()); + if mention_skill_paths.contains(path_str.as_ref()) + && mention_skill_path_counts.get(path_str.as_ref()) == Some(&1) + { + seen_paths.insert(skill.source_path.clone()); seen_names.insert(skill.name.clone()); selected.push(skill.clone()); } @@ -362,8 +379,8 @@ fn select_skills_from_mentions( for skill in selection_context.skills { if selection_context .disabled_paths - .contains(&skill.path_to_skills_md) - || seen_paths.contains(&skill.path_to_skills_md) + .contains(&skill.source_path) + || seen_paths.contains(&skill.source_path) { continue; } @@ -390,7 +407,7 @@ fn select_skills_from_mentions( } if seen_names.insert(skill.name.clone()) { - seen_paths.insert(skill.path_to_skills_md.clone()); + seen_paths.insert(skill.source_path.clone()); selected.push(skill.clone()); } } diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 78aa1958952..822c0f3821b 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -1,5 +1,6 @@ use super::*; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_exec_server::EnvironmentPathRef; +use codex_exec_server::LocalFileSystem; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; @@ -14,6 +15,7 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(test_path_buf(path).abs()), path_to_skills_md: test_path_buf(path).abs(), scope: codex_protocol::protocol::SkillScope::User, plugin_id: None, @@ -37,7 +39,7 @@ fn linked_skill_mention(name: &str, unix_path: &str) -> String { fn collect_mentions( inputs: &[UserInput], skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts) @@ -193,6 +195,7 @@ fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fal #[test] fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); + let disabled = HashSet::from([alpha.source_path.clone()]); let skills = vec![alpha]; let inputs = vec![ UserInput::Text { @@ -204,7 +207,6 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa path: test_path_buf("/tmp/alpha"), }, ]; - let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]); let connector_counts = HashMap::new(); let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); @@ -228,6 +230,29 @@ fn collect_explicit_skill_mentions_dedupes_by_path() { assert_eq!(selected, vec![alpha]); } +#[test] +fn collect_explicit_skill_mentions_skips_ambiguous_linked_path() { + let alpha = make_skill("demo-skill", "/tmp/shared"); + let beta = SkillMetadata { + name: "demo-skill".to_string(), + source_path: EnvironmentPathRef::new( + std::sync::Arc::new(LocalFileSystem::unsandboxed()), + alpha.path_to_skills_md.clone(), + ), + ..alpha.clone() + }; + let skills = vec![alpha, beta]; + let inputs = vec![UserInput::Text { + text: linked_skill_mention("demo-skill", "/tmp/shared"), + text_elements: Vec::new(), + }]; + let connector_counts = HashMap::new(); + + let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts); + + assert_eq!(selected, Vec::new()); +} + #[test] fn collect_explicit_skill_mentions_skips_ambiguous_name() { let alpha = make_skill("demo-skill", "/tmp/alpha"); @@ -297,12 +322,12 @@ fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict( fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() { let alpha = make_skill("demo-skill", "/tmp/alpha"); let beta = make_skill("demo-skill", "/tmp/beta"); + let disabled = HashSet::from([alpha.source_path.clone()]); let skills = vec![alpha, beta]; let inputs = vec![UserInput::Text { text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/alpha")), text_elements: Vec::new(), }]; - let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]); let connector_counts = HashMap::new(); let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts); diff --git a/codex-rs/core-skills/src/invocation_utils.rs b/codex-rs/core-skills/src/invocation_utils.rs index 4c9d0a4119e..4632c4ef4f6 100644 --- a/codex-rs/core-skills/src/invocation_utils.rs +++ b/codex-rs/core-skills/src/invocation_utils.rs @@ -3,22 +3,24 @@ use std::path::Path; use crate::SkillLoadOutcome; use crate::SkillMetadata; +use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; pub(crate) fn build_implicit_skill_path_indexes( skills: Vec, ) -> ( - HashMap, - HashMap, + HashMap, + HashMap, ) { let mut by_scripts_dir = HashMap::new(); let mut by_skill_doc_path = HashMap::new(); for skill in skills { - let skill_doc_path = canonicalize_if_exists(&skill.path_to_skills_md); + let skill_doc_path = canonicalize_if_exists(&skill.source_path); by_skill_doc_path.insert(skill_doc_path, skill.clone()); if let Some(skill_dir) = skill.path_to_skills_md.parent() { - let scripts_dir = canonicalize_if_exists(&skill_dir.join("scripts")); + let scripts_dir = + canonicalize_if_exists(&skill.source_path.with_path(skill_dir.join("scripts"))); by_scripts_dir.insert(scripts_dir, skill); } } @@ -29,7 +31,7 @@ pub(crate) fn build_implicit_skill_path_indexes( pub fn detect_implicit_skill_invocation_for_command( outcome: &SkillLoadOutcome, command: &str, - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { let workdir = canonicalize_if_exists(workdir); let tokens = tokenize_command(command); @@ -81,14 +83,20 @@ fn script_run_token(tokens: &[String]) -> Option<&str> { fn detect_skill_script_run( outcome: &SkillLoadOutcome, tokens: &[String], - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { let script_token = script_run_token(tokens)?; let script_path = Path::new(script_token); - let script_path = canonicalize_if_exists(&workdir.join(script_path)); + let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path)?); - for path in script_path.ancestors() { - if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&path) { + for path in script_path.path().ancestors() { + let Ok(path) = AbsolutePathBuf::from_absolute_path(path) else { + continue; + }; + if let Some(candidate) = outcome + .implicit_skills_by_scripts_dir + .get(&script_path.with_path(path)) + { return Some(candidate.clone()); } } @@ -99,7 +107,7 @@ fn detect_skill_script_run( fn detect_skill_doc_read( outcome: &SkillLoadOutcome, tokens: &[String], - workdir: &AbsolutePathBuf, + workdir: &EnvironmentPathRef, ) -> Option { if !command_reads_file(tokens) { return None; @@ -110,7 +118,7 @@ fn detect_skill_doc_read( continue; } let path = Path::new(token); - let candidate_path = canonicalize_if_exists(&workdir.join(path)); + let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path)?); if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { return Some(candidate.clone()); } @@ -136,8 +144,24 @@ fn command_basename(command: &str) -> String { .to_string() } -fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) +fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option { + if path.is_absolute() { + AbsolutePathBuf::from_absolute_path(path) + .ok() + .map(|path| workdir.with_path(path)) + } else { + workdir.join_relative(path) + } +} + +fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef { + // TODO: Canonicalize through the bound executor filesystem once it exposes a realpath API. + // The local fallback below cannot resolve remote-only paths. + path.with_path( + path.path() + .canonicalize() + .unwrap_or_else(|_| path.path().clone()), + ) } #[cfg(test)] diff --git a/codex-rs/core-skills/src/invocation_utils_tests.rs b/codex-rs/core-skills/src/invocation_utils_tests.rs index f6e3883c16d..5087cc2ec10 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/core-skills/src/invocation_utils_tests.rs @@ -19,6 +19,7 @@ fn test_skill_metadata(skill_doc_path: AbsolutePathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((skill_doc_path).clone()), path_to_skills_md: skill_doc_path, scope: codex_protocol::protocol::SkillScope::User, plugin_id: None, @@ -29,6 +30,10 @@ fn test_path_display(unix_path: &str) -> String { test_path_buf(unix_path).display().to_string() } +fn local_path_ref(path: AbsolutePathBuf) -> codex_exec_server::EnvironmentPathRef { + codex_exec_server::EnvironmentPathRef::local(path) +} + #[test] fn script_run_detection_matches_runner_plus_extension() { let tokens = vec![ @@ -54,7 +59,7 @@ fn script_run_detection_excludes_python_c() { #[test] fn skill_doc_read_detection_matches_absolute_path() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let normalized_skill_doc_path = canonicalize_if_exists(&skill_doc_path); + let normalized_skill_doc_path = canonicalize_if_exists(&local_path_ref(skill_doc_path.clone())); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), @@ -68,7 +73,11 @@ fn skill_doc_read_detection_matches_absolute_path() { "|".to_string(), "head".to_string(), ]; - let found = detect_skill_doc_read(&outcome, &tokens, &test_path_buf("/tmp").abs()); + let found = detect_skill_doc_read( + &outcome, + &tokens, + &local_path_ref(test_path_buf("/tmp").abs()), + ); assert_eq!( found.map(|value| value.name), @@ -79,7 +88,9 @@ fn skill_doc_read_detection_matches_absolute_path() { #[test] fn skill_script_run_detection_matches_relative_path_from_skill_root() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs()); + let scripts_dir = canonicalize_if_exists(&local_path_ref( + test_path_buf("/tmp/skill-test/scripts").abs(), + )); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -91,7 +102,11 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { "scripts/fetch_comments.py".to_string(), ]; - let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/skill-test").abs()); + let found = detect_skill_script_run( + &outcome, + &tokens, + &local_path_ref(test_path_buf("/tmp/skill-test").abs()), + ); assert_eq!( found.map(|value| value.name), @@ -102,7 +117,9 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { #[test] fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs()); + let scripts_dir = canonicalize_if_exists(&local_path_ref( + test_path_buf("/tmp/skill-test/scripts").abs(), + )); let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -114,7 +131,11 @@ fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { test_path_display("/tmp/skill-test/scripts/fetch_comments.py"), ]; - let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/other").abs()); + let found = detect_skill_script_run( + &outcome, + &tokens, + &local_path_ref(test_path_buf("/tmp/other").abs()), + ); assert_eq!( found.map(|value| value.name), diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 8663737ee56..408c0850489 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,6 +1,5 @@ use crate::model::SkillDependencies; use crate::model::SkillError; -use crate::model::SkillFileSystemsByPath; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; @@ -13,8 +12,8 @@ use codex_config::ConfigLayerStackOrdering; use codex_config::default_project_root_markers; 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 @@ -163,17 +161,14 @@ where I: IntoIterator, { let mut outcome = SkillLoadOutcome::default(); - let mut skill_roots: Vec = Vec::new(); - let mut skill_root_by_path: HashMap = HashMap::new(); - let mut file_systems_by_skill_path: HashMap> = - HashMap::new(); + let mut skill_roots: Vec = Vec::new(); + let mut skill_root_by_path: HashMap = HashMap::new(); for root in roots { - let root_path = canonicalize_for_skill_identity(&root.path); - let fs = root.file_system; + let root_path = canonicalize_for_skill_identity(root.path.path()); + let root_path_ref = root.path.with_path(root_path.clone()); let skills_before_root = outcome.skills.len(); discover_skills_under_root( - fs.as_ref(), - &root_path, + &root_path_ref, root.scope, root.plugin_id.as_deref(), root.plugin_root.as_ref(), @@ -181,34 +176,29 @@ where ) .await; for skill in &outcome.skills[skills_before_root..] { - if !skill_roots.contains(&root_path) { - skill_roots.push(root_path.clone()); + if !skill_roots.contains(&root_path_ref) { + skill_roots.push(root_path_ref.clone()); } skill_root_by_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| root_path.clone()); - file_systems_by_skill_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| Arc::clone(&fs)); + .entry(skill.source_path.clone()) + .or_insert_with(|| root_path_ref.clone()); } } - let mut seen: HashSet = HashSet::new(); + let mut seen: HashSet = HashSet::new(); outcome .skills - .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); - let retained_skill_paths: HashSet = outcome + .retain(|skill| seen.insert(skill.source_path.clone())); + let retained_skill_paths: HashSet = outcome .skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect(); skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); - let used_roots: HashSet = skill_root_by_path.values().cloned().collect(); + let used_roots: HashSet = skill_root_by_path.values().cloned().collect(); skill_roots.retain(|root| used_roots.contains(root)); - file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); outcome.skill_roots = skill_roots; outcome.skill_root_by_path = Arc::new(skill_root_by_path); - outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); fn scope_rank(scope: SkillScope) -> u8 { // Higher-priority scopes first (matches root scan order for dedupe). @@ -231,18 +221,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, @@ -251,38 +243,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( @@ -294,24 +295,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, }); @@ -319,9 +313,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, }); @@ -330,9 +326,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, }); @@ -341,9 +339,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, }); @@ -355,28 +355,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, }), @@ -385,7 +434,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() ); } } @@ -416,24 +465,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() ); } } @@ -465,40 +513,46 @@ fn dirs_between_project_root_and_cwd( } fn dedupe_skill_roots_by_path(roots: &mut Vec) { - let mut seen: HashSet = HashSet::new(); + let mut seen: HashSet = HashSet::new(); roots.retain(|root| seen.insert(root.path.clone())); } fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { + // TODO: Canonicalize through the bound executor filesystem once it exposes a realpath API. + // The local fallback below cannot resolve remote-only paths. path.canonicalize().unwrap_or_else(|_| path.clone()) } async fn discover_skills_under_root( - fs: &dyn ExecutorFileSystem, - root: &AbsolutePathBuf, + root: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, - plugin_root: Option<&AbsolutePathBuf>, + plugin_root: Option<&EnvironmentPathRef>, outcome: &mut SkillLoadOutcome, ) { - let root = canonicalize_for_skill_identity(root); - let plugin_root = plugin_root.map(canonicalize_for_skill_identity); + let root = root.with_path(canonicalize_for_skill_identity(root.path())); + let plugin_root = plugin_root.map(|plugin_root| { + plugin_root.with_path(canonicalize_for_skill_identity(plugin_root.path())) + }); - match fs.get_metadata(&root, /*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, Err(err) => { - error!("failed to stat skills root {}: {err:#}", root.display()); + error!( + "failed to stat skills root {}: {err:#}", + root.path().display() + ); return; } } 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 { @@ -508,7 +562,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)); } } @@ -520,16 +574,16 @@ async fn discover_skills_under_root( ); let mut visited_dirs: HashSet = HashSet::new(); - visited_dirs.insert(root.clone()); + visited_dirs.insert(root.path().clone()); - let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.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; } }; @@ -540,11 +594,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 Some(path) = dir.join_relative(Path::new(&file_name)) 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; } }; @@ -553,9 +612,10 @@ 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(&path); + let resolved_dir = + path.with_path(canonicalize_for_skill_identity(path.path())); enqueue_dir( &mut queue, &mut visited_dirs, @@ -572,7 +632,7 @@ async fn discover_skills_under_root( Err(err) => { error!( "failed to read skills symlink dir {}: {err:#}", - path.display() + path.path().display() ); } } @@ -580,7 +640,7 @@ async fn discover_skills_under_root( } if metadata.is_directory { - let resolved_dir = canonicalize_for_skill_identity(&path); + let resolved_dir = path.with_path(canonicalize_for_skill_identity(path.path())); enqueue_dir( &mut queue, &mut visited_dirs, @@ -592,14 +652,14 @@ async fn discover_skills_under_root( } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file(fs, &path, 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(), }); } @@ -613,22 +673,22 @@ async fn discover_skills_under_root( tracing::warn!( "skills scan truncated after {} directories (root: {})", MAX_SKILLS_DIRS_PER_ROOT, - root.display() + root.path().display() ); } } async fn parse_skill_file( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, + skill_path: &EnvironmentPathRef, scope: SkillScope, plugin_id: Option<&str>, - plugin_root: Option<&AbsolutePathBuf>, + plugin_root: Option<&EnvironmentPathRef>, ) -> Result { - let contents = fs - .read_file_text(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)?; @@ -641,7 +701,7 @@ async fn parse_skill_file( .map(sanitize_single_line) .filter(|value| !value.is_empty()) .unwrap_or_else(|| default_skill_name(path)); - let name = namespaced_skill_name(fs, path, &base_name).await; + let name = namespaced_skill_name(skill_path.file_system().as_ref(), path, &base_name).await; let description = parsed .description .as_deref() @@ -657,7 +717,7 @@ async fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata(fs, path, plugin_root).await; + } = load_skill_metadata(skill_path, plugin_root).await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -678,7 +738,8 @@ async fn parse_skill_file( interface, dependencies, policy, - path_to_skills_md: resolved_path, + path_to_skills_md: resolved_path.clone(), + source_path: skill_path.with_path(resolved_path), scope, plugin_id: plugin_id.map(str::to_string), }) @@ -708,18 +769,21 @@ 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 Some(skill_dir) = skill_path.parent_dir() else { + return LoadedSkillMetadata::default(); + }; + let Some(metadata_path) = skill_dir.join_relative( + Path::new(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME) + .as_path(), + ) 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 { + 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 => { @@ -728,19 +792,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(); @@ -748,13 +812,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(); @@ -768,7 +832,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), } @@ -1061,10 +1129,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 69e357d93db..252bc55cb4d 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,7 +4,9 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; +use codex_exec_server::LocalFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -144,6 +146,19 @@ 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: local_env_path(path), + scope, + plugin_id: None, + plugin_root: None, + } +} + #[tokio::test] async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() -> anyhow::Result<()> { @@ -187,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!( @@ -256,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!( @@ -278,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()?; @@ -330,6 +396,7 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -468,6 +535,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() { ], }), policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -524,6 +592,9 @@ interface: }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + skill_path.as_path() + )), path_to_skills_md: normalized(skill_path.as_path()), scope: SkillScope::User, plugin_id: None, @@ -678,6 +749,7 @@ async fn accepts_icon_paths_under_assets_dir() { }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -719,6 +791,7 @@ async fn ignores_invalid_brand_color() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -773,6 +846,7 @@ async fn ignores_default_prompt_over_max_length() { }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -815,6 +889,7 @@ async fn drops_interface_when_icons_are_invalid() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -846,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; @@ -876,6 +950,7 @@ interface: }), dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), @@ -903,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; @@ -925,6 +999,7 @@ interface: interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("twilio-developer-kit@test".to_string()), @@ -970,6 +1045,9 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &shared_skill_path + )), path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::User, plugin_id: None, @@ -1030,6 +1108,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1048,14 +1127,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { fs::create_dir_all(admin_root.path()).unwrap(); symlink_dir(shared.path(), &admin_root.path().join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: admin_root.path().abs(), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(admin_root.path().abs(), SkillScope::Admin)]) + .await; assert!( outcome.errors.is_empty(), @@ -1071,6 +1145,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &shared_skill_path + )), path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::Admin, plugin_id: None, @@ -1111,6 +1188,9 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &linked_skill_path + )), path_to_skills_md: normalized(&linked_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1130,14 +1210,8 @@ async fn system_scope_ignores_symlinked_subdir() { fs::create_dir_all(&system_root).unwrap(); symlink_dir(shared.path(), &system_root.join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: system_root.abs(), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(system_root.abs(), SkillScope::System)]).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1164,14 +1238,8 @@ async fn respects_max_scan_depth_for_user_scope() { ); let skills_root = codex_home.path().join("skills"); - let outcome = load_skills_from_roots([SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_root: None, - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(skills_root.abs(), SkillScope::User)]).await; assert!( outcome.errors.is_empty(), @@ -1187,6 +1255,9 @@ async fn respects_max_scan_depth_for_user_scope() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &within_depth_path + )), path_to_skills_md: normalized(&within_depth_path), scope: SkillScope::User, plugin_id: None, @@ -1215,6 +1286,7 @@ async fn loads_valid_skill() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1248,6 +1320,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1272,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; @@ -1294,6 +1366,7 @@ async fn namespaces_plugin_skills_using_plugin_name() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: Some("sample@test".to_string()), @@ -1326,6 +1399,7 @@ async fn loads_short_description_from_metadata() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, plugin_id: None, @@ -1439,6 +1513,7 @@ async fn loads_skills_from_repo_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1475,6 +1550,7 @@ async fn loads_skills_from_agents_dir_without_codex_dir() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1529,6 +1605,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &nested_skill_path + )), path_to_skills_md: normalized(&nested_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1540,6 +1619,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &root_skill_path + )), path_to_skills_md: normalized(&root_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1580,6 +1662,7 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1595,16 +1678,14 @@ async fn deduplicates_by_path_preferring_first_root() { let outcome = load_skills_from_roots([ SkillRoot { - path: root.path().abs(), + path: local_env_path(root.path().abs()), scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }, SkillRoot { - path: root.path().abs(), + path: local_env_path(root.path().abs()), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }, @@ -1625,6 +1706,7 @@ async fn deduplicates_by_path_preferring_first_root() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1632,6 +1714,41 @@ async fn deduplicates_by_path_preferring_first_root() { ); } +#[tokio::test] +async fn preserves_same_absolute_skill_path_from_distinct_file_systems() { + let root = tempfile::tempdir().expect("tempdir"); + let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); + let shared_path = root.path().abs(); + let first_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + let second_file_system: Arc = Arc::new(LocalFileSystem::unsandboxed()); + + let outcome = load_skills_from_roots([ + SkillRoot { + path: EnvironmentPathRef::new(first_file_system, shared_path.clone()), + scope: SkillScope::Repo, + plugin_id: None, + plugin_root: None, + }, + SkillRoot { + path: EnvironmentPathRef::new(second_file_system, shared_path), + scope: SkillScope::Repo, + plugin_id: None, + plugin_root: None, + }, + ]) + .await; + + assert_eq!( + outcome + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect::>(), + vec![normalized(&skill_path), normalized(&skill_path)] + ); + assert_ne!(outcome.skills[0].source_path, outcome.skills[1].source_path); +} + #[tokio::test] async fn keeps_duplicate_names_from_repo_and_user() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -1667,6 +1784,9 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &repo_skill_path + )), path_to_skills_md: normalized(&repo_skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1678,6 +1798,9 @@ async fn keeps_duplicate_names_from_repo_and_user() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized( + &user_skill_path + )), path_to_skills_md: normalized(&user_skill_path), scope: SkillScope::User, plugin_id: None, @@ -1741,6 +1864,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((first_path).clone()), path_to_skills_md: first_path, scope: SkillScope::Repo, plugin_id: None, @@ -1752,6 +1876,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local((second_path).clone()), path_to_skills_md: second_path, scope: SkillScope::Repo, plugin_id: None, @@ -1824,6 +1949,7 @@ async fn loads_skills_when_cwd_is_file_in_repo() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, plugin_id: None, @@ -1883,6 +2009,7 @@ async fn loads_skills_from_system_cache_when_present() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)), path_to_skills_md: normalized(&skill_path), scope: SkillScope::System, plugin_id: None, @@ -1895,10 +2022,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 185eaa93206..0cbaa02ed40 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -4,7 +4,6 @@ use std::sync::Arc; use std::sync::RwLock; use codex_config::ConfigLayerStack; -use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -23,24 +22,42 @@ 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; -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct SkillsLoadInput { - pub cwd: AbsolutePathBuf, + pub env_path: Option, + /// Local user/system/plugin skill roots are read from this filesystem for now. + pub 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("env_path", &self.env_path) + .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, + env_path: Option, + local_file_system: Option>, effective_skill_roots: Vec, config_layer_stack: ConfigLayerStack, bundled_skills_enabled: bool, ) -> Self { Self { - cwd, + env_path, + local_file_system, effective_skill_roots, config_layer_stack, bundled_skills_enabled, @@ -52,7 +69,7 @@ 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>, } @@ -70,7 +87,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 { @@ -100,12 +117,8 @@ impl SkillsManager { /// This path uses a cache keyed by the effective skill-relevant config state rather than just /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen /// to share a directory. - pub async fn skills_for_config( - &self, - input: &SkillsLoadInput, - fs: Option>, - ) -> SkillLoadOutcome { - let roots = self.skill_roots_for_config(input, fs).await; + pub async fn skills_for_config(&self, input: &SkillsLoadInput) -> SkillLoadOutcome { + let roots = self.skill_roots_for_config(input).await; let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let cache_key = config_skills_cache_key(&roots, &skill_config_rules); if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { @@ -121,15 +134,11 @@ impl SkillsManager { outcome } - pub async fn skill_roots_for_config( - &self, - input: &SkillsLoadInput, - fs: Option>, - ) -> Vec { + pub async fn skill_roots_for_config(&self, input: &SkillsLoadInput) -> Vec { let mut roots = skill_roots( - fs, + input.env_path.as_ref(), + input.local_file_system.as_ref(), &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -144,20 +153,24 @@ impl SkillsManager { &self, input: &SkillsLoadInput, force_reload: bool, - fs: Option>, ) -> SkillLoadOutcome { - let use_cwd_cache = fs.is_some(); - if use_cwd_cache - && !force_reload - && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) + let path_ref_cache_key = SkillsPathCacheKey { + env_path: input.env_path.clone(), + local_file_system: input + .local_file_system + .as_ref() + .map(ExecutorFileSystemRef::new), + }; + if !force_reload + && let Some(outcome) = self.cached_outcome_for_path_ref(&path_ref_cache_key) { return outcome; } let mut roots = skill_roots( - fs.clone(), + input.env_path.as_ref(), + input.local_file_system.as_ref(), &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -167,13 +180,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 use_cwd_cache { - let mut cache = self - .cache_by_cwd - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(input.cwd.clone(), 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 } @@ -191,9 +202,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(); @@ -209,14 +220,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: &AbsolutePathBuf) -> 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(), } } @@ -238,9 +252,44 @@ 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<(AbsolutePathBuf, u8, Option)>, + roots: Vec<(EnvironmentPathRef, u8, Option)>, skill_config_rules: SkillConfigRules, } @@ -289,7 +338,7 @@ fn config_skills_cache_key( fn finalize_skill_outcome( mut outcome: SkillLoadOutcome, - disabled_paths: HashSet, + disabled_paths: HashSet, ) -> SkillLoadOutcome { outcome.disabled_paths = disabled_paths; let (by_scripts_dir, by_doc_path) = diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 713b94d7b1c..52901decb0c 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -71,6 +71,10 @@ fn plugin_skill_root_for_skill_path(skill_path: &Path, plugin_id: &str) -> Plugi } fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { + let path = path + .abs() + .canonicalize() + .expect("skill path should canonicalize"); SkillMetadata { name: name.to_string(), description: "test".to_string(), @@ -78,15 +82,32 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { interface: None, dependencies: None, policy: None, - path_to_skills_md: path - .abs() - .canonicalize() - .expect("skill path should canonicalize"), + source_path: EnvironmentPathRef::local(path.clone()), + path_to_skills_md: path, scope: SkillScope::User, plugin_id: None, } } +fn skill_root_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { + EnvironmentPathRef::local(path) +} + +fn local_skills_input( + cwd: AbsolutePathBuf, + effective_skill_roots: Vec, + config_layer_stack: ConfigLayerStack, +) -> SkillsLoadInput { + let path_ref = skill_root_path_ref(cwd); + SkillsLoadInput::new( + Some(path_ref), + Some(Arc::clone(&LOCAL_FS)), + effective_skill_roots, + config_layer_stack.clone(), + bundled_skills_enabled_from_stack(&config_layer_stack), + ) +} + 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")) @@ -164,15 +185,12 @@ 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))) - .await + skills_manager.skills_for_config(&skills_input).await } #[test] @@ -232,18 +250,9 @@ 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)), - ) + .skills_for_cwd(&skills_input, /*force_reload*/ false) .await; assert!( empty_outcome @@ -263,11 +272,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { skills_manager.set_extra_roots(vec![extra_skills_root.abs()]); let runtime_outcome = skills_manager - .skills_for_cwd( - &skills_input, - /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), - ) + .skills_for_cwd(&skills_input, /*force_reload*/ false) .await; assert!( runtime_outcome @@ -278,11 +283,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { skills_manager.set_extra_roots(vec![extra_root.path().join("missing-skills").abs()]); let replaced_outcome = skills_manager - .skills_for_cwd( - &skills_input, - /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), - ) + .skills_for_cwd(&skills_input, /*force_reload*/ false) .await; assert_eq!(replaced_outcome.errors, Vec::new()); assert!( @@ -382,7 +383,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() { .abs(); assert_eq!(skill.path_to_skills_md, skill_path); - assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md)); + assert!(outcome.disabled_paths.contains(&skill.source_path)); assert!( !outcome .allowed_skills_for_implicit_invocation() @@ -407,6 +408,60 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ) .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.clone()); + 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) + .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_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"); + 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, ""), @@ -422,7 +477,8 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ) .expect("valid config layer stack"); let skills_input = SkillsLoadInput::new( - cwd.path().abs(), + Some(skill_root_path_ref(cwd.path().abs())), + /*local_file_system*/ None, Vec::new(), config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), @@ -433,11 +489,7 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ); let outcome = skills_manager - .skills_for_cwd( - &skills_input, - /*force_reload*/ true, - Some(Arc::clone(&LOCAL_FS)), - ) + .skills_for_cwd(&skills_input, /*force_reload*/ true) .await; assert!( @@ -450,12 +502,12 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { .iter() .map(|skill| skill.name.as_str()) .collect::>(); - assert!(loaded_names.contains("user-skill")); + assert!(!loaded_names.contains("user-skill")); assert!(loaded_names.contains("repo-skill")); } #[tokio::test] -async fn skills_for_cwd_without_fs_skips_repo_roots() { +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"); @@ -485,7 +537,8 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() { ) .expect("valid config layer stack"); let skills_input = SkillsLoadInput::new( - cwd.path().abs(), + /*env_path*/ None, + Some(Arc::clone(&LOCAL_FS)), Vec::new(), config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), @@ -496,7 +549,7 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() { ); let outcome = skills_manager - .skills_for_cwd(&skills_input, /*force_reload*/ true, /*fs*/ None) + .skills_for_cwd(&skills_input, /*force_reload*/ true) .await; assert!( @@ -565,18 +618,9 @@ 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)), - ) + .skills_for_cwd(&base_input, /*force_reload*/ false) .await; assert!( outcome_a @@ -588,11 +632,7 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { write_user_skill(&codex_home, "late", "late-skill", "added after cache"); let outcome_b = skills_manager - .skills_for_cwd( - &base_input, - /*force_reload*/ false, - Some(Arc::clone(&LOCAL_FS)), - ) + .skills_for_cwd(&base_input, /*force_reload*/ false) .await; assert!( outcome_b @@ -602,11 +642,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) .await; assert!( outcome_reloaded @@ -682,10 +718,12 @@ fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill( let skill_config_rules = skill_config_rules_from_stack(&stack); assert_eq!( resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) + HashSet::from([EnvironmentPathRef::local( + skill_path + .abs() + .canonicalize() + .expect("skill path should canonicalize"), + )]) ); } @@ -715,10 +753,12 @@ fn disabled_paths_for_skills_disables_matching_name_selectors() { let skill_config_rules = skill_config_rules_from_stack(&stack); assert_eq!( resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) + HashSet::from([EnvironmentPathRef::local( + skill_path + .abs() + .canonicalize() + .expect("skill path should canonicalize"), + )]) ); } @@ -757,6 +797,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() { @@ -779,19 +857,10 @@ 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)), - ) + .skills_for_cwd(&parent_input, /*force_reload*/ true) .await; let parent_skill = parent_outcome .skills diff --git a/codex-rs/core-skills/src/mention_counts.rs b/codex-rs/core-skills/src/mention_counts.rs index b7482ca36ec..2eabc07c6fe 100644 --- a/codex-rs/core-skills/src/mention_counts.rs +++ b/codex-rs/core-skills/src/mention_counts.rs @@ -2,17 +2,17 @@ use std::collections::HashMap; use std::collections::HashSet; use super::SkillMetadata; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_exec_server::EnvironmentPathRef; /// Counts how often each skill name appears (exact and ASCII-lowercase), excluding disabled paths. pub fn build_skill_name_counts( skills: &[SkillMetadata], - disabled_paths: &HashSet, + disabled_paths: &HashSet, ) -> (HashMap, HashMap) { let mut exact_counts: HashMap = HashMap::new(); let mut lower_counts: HashMap = HashMap::new(); for skill in skills { - if disabled_paths.contains(&skill.path_to_skills_md) { + if disabled_paths.contains(&skill.source_path) { continue; } *exact_counts.entry(skill.name.clone()).or_insert(0) += 1; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index fc8e9f5917d..91871f9842e 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -1,12 +1,10 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::fmt; -use std::sync::Arc; - -use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::EnvironmentPathRef; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; +use std::collections::HashSet; +use std::sync::Arc; #[derive(Debug, Clone, PartialEq)] pub struct SkillMetadata { @@ -18,6 +16,8 @@ pub struct SkillMetadata { pub policy: Option, /// Path to the SKILLS.md file that declares this skill. pub path_to_skills_md: AbsolutePathBuf, + /// Bound path to the SKILL.md file that declares this skill. + pub source_path: EnvironmentPathRef, pub scope: SkillScope, pub plugin_id: Option, } @@ -89,17 +89,16 @@ pub struct SkillError { pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, - pub disabled_paths: HashSet, - pub(crate) skill_roots: Vec, - pub(crate) skill_root_by_path: Arc>, - pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, - pub(crate) implicit_skills_by_scripts_dir: Arc>, - pub(crate) implicit_skills_by_doc_path: Arc>, + pub disabled_paths: HashSet, + pub(crate) skill_roots: Vec, + pub(crate) skill_root_by_path: Arc>, + pub(crate) implicit_skills_by_scripts_dir: Arc>, + pub(crate) implicit_skills_by_doc_path: Arc>, } impl SkillLoadOutcome { pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { - !self.disabled_paths.contains(&skill.path_to_skills_md) + !self.disabled_paths.contains(&skill.source_path) } pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool { @@ -119,49 +118,6 @@ impl SkillLoadOutcome { .iter() .map(|skill| (skill, self.is_skill_enabled(skill))) } - - pub(crate) fn file_system_for_skill( - &self, - skill: &SkillMetadata, - ) -> Option> { - self.file_systems_by_skill_path - .get(&skill.path_to_skills_md) - } -} - -#[derive(Clone, Default)] -pub(crate) struct SkillFileSystemsByPath { - values: Arc>>, -} - -impl SkillFileSystemsByPath { - pub(crate) fn new(values: HashMap>) -> Self { - Self { - values: Arc::new(values), - } - } - - fn get(&self, path: &AbsolutePathBuf) -> Option> { - self.values.get(path).map(Arc::clone) - } - - fn retain_paths(&mut self, paths: &HashSet) { - self.values = Arc::new( - self.values - .iter() - .filter(|(path, _)| paths.contains(*path)) - .map(|(path, fs)| (path.clone(), Arc::clone(fs))) - .collect(), - ); - } -} - -impl fmt::Debug for SkillFileSystemsByPath { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("SkillFileSystemsByPath") - .field("len", &self.values.len()) - .finish() - } } pub fn filter_skill_load_outcome_for_product( @@ -171,14 +127,11 @@ pub fn filter_skill_load_outcome_for_product( outcome .skills .retain(|skill| skill.matches_product_restriction_for_product(restriction_product)); - let retained_paths: HashSet = outcome + let retained_paths: HashSet = outcome .skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect(); - outcome - .file_systems_by_skill_path - .retain_paths(&retained_paths); outcome.skill_root_by_path = Arc::new( outcome .skill_root_by_path @@ -187,7 +140,7 @@ pub fn filter_skill_load_outcome_for_product( .map(|(path, root)| (path.clone(), root.clone())) .collect(), ); - let retained_roots: HashSet = + let retained_roots: HashSet = outcome.skill_root_by_path.values().cloned().collect(); outcome .skill_roots @@ -210,3 +163,30 @@ pub fn filter_skill_load_outcome_for_product( ); outcome } + +#[cfg(test)] +mod tests { + use std::path::Path; + + use codex_exec_server::EnvironmentPathRef; + use codex_utils_absolute_path::test_support::PathBufExt; + use pretty_assertions::assert_eq; + + #[test] + fn environment_path_ref_joins_only_relative_paths() { + let root_path = std::env::temp_dir().join("skills"); + let path_ref = EnvironmentPathRef::local(root_path.abs()); + + assert_eq!( + path_ref + .join_relative(Path::new("demo/SKILL.md")) + .map(|path_ref| path_ref.path().clone()), + Some(root_path.join("demo/SKILL.md").abs()) + ); + assert!( + path_ref + .join_relative(std::env::temp_dir().join("SKILL.md").as_path()) + .is_none() + ); + } +} diff --git a/codex-rs/core-skills/src/render.rs b/codex-rs/core-skills/src/render.rs index 28617fb6c42..2da51d94e6b 100644 --- a/codex-rs/core-skills/src/render.rs +++ b/codex-rs/core-skills/src/render.rs @@ -5,6 +5,7 @@ use std::path::Path; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; +use codex_exec_server::EnvironmentPathRef; use codex_otel::SessionTelemetry; use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC; use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC; @@ -665,8 +666,8 @@ struct SkillPathAliases { struct AliasPlan { aliases: SkillPathAliases, - root_aliases: HashMap, - alias_root_by_path: HashMap, + root_aliases: HashMap, + alias_root_by_path: HashMap, table_cost: usize, } @@ -677,7 +678,7 @@ fn build_alias_plan( ) -> Option { let skill_paths = skills .iter() - .map(|skill| skill.path_to_skills_md.clone()) + .map(|skill| skill.source_path.clone()) .collect::>(); let skill_root_by_path = outcome .skill_root_by_path @@ -736,9 +737,9 @@ fn build_alias_plan( } fn ordered_alias_roots( - used_roots: &[AbsolutePathBuf], - alias_root_by_skill_root: &HashMap, -) -> Option> { + used_roots: &[EnvironmentPathRef], + alias_root_by_skill_root: &HashMap, +) -> Option> { let mut seen = HashSet::new(); let mut alias_roots = Vec::new(); for root in used_roots { @@ -751,10 +752,10 @@ fn ordered_alias_roots( } fn alias_root_for_skill_root( - root: &AbsolutePathBuf, + root: &EnvironmentPathRef, plugin_version_skill_counts: &HashMap, -) -> AbsolutePathBuf { - let Some(plugin_version_base) = plugin_version_base(root.as_path()) else { +) -> EnvironmentPathRef { + let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) else { return root.clone(); }; let skill_count = plugin_version_skill_counts @@ -764,16 +765,18 @@ fn alias_root_for_skill_root( if skill_count > 1 { root.clone() } else { - plugin_marketplace_base(root.as_path()).unwrap_or_else(|| root.clone()) + plugin_marketplace_base(root.path().as_path()) + .map(|path| root.with_path(path)) + .unwrap_or_else(|| root.clone()) } } fn plugin_version_skill_counts_for_skill_roots<'a>( - skill_roots: impl Iterator, + skill_roots: impl Iterator, ) -> HashMap { let mut counts = HashMap::new(); for root in skill_roots { - if let Some(plugin_version_base) = plugin_version_base(root.as_path()) { + if let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) { let count = counts.entry(plugin_version_base).or_insert(0usize); *count = count.saturating_add(1); } @@ -793,12 +796,12 @@ fn aliased_metadata_overhead_cost( .saturating_sub(budget.cost(&absolute_body)) } -fn build_skill_root_lines(roots: &[AbsolutePathBuf]) -> Vec { +fn build_skill_root_lines(roots: &[EnvironmentPathRef]) -> Vec { roots .iter() .enumerate() .map(|(index, root)| { - let root_str = root.to_string_lossy().replace('\\', "/"); + let root_str = root.path().to_string_lossy().replace('\\', "/"); format!("- `r{index}` = `{root_str}`") }) .collect() @@ -840,12 +843,12 @@ fn render_skill_path_with_aliases(skill: &SkillMetadata, plan: &AliasPlan) -> St } fn outcome_relative_skill_path(skill: &SkillMetadata, plan: &AliasPlan) -> Option { - let alias_root = plan.alias_root_by_path.get(&skill.path_to_skills_md)?; + let alias_root = plan.alias_root_by_path.get(&skill.source_path)?; let alias = plan.root_aliases.get(alias_root)?; let relative_path = skill .path_to_skills_md .as_path() - .strip_prefix(alias_root.as_path()) + .strip_prefix(alias_root.path().as_path()) .ok()?; let relative_path = relative_path.to_string_lossy().replace('\\', "/"); Some(format!("{alias}/{relative_path}")) @@ -920,6 +923,9 @@ mod tests { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), + ), path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), scope, plugin_id: None, @@ -948,6 +954,10 @@ mod tests { skills: Vec, roots: Vec, ) -> SkillLoadOutcome { + let roots = roots + .into_iter() + .map(codex_exec_server::EnvironmentPathRef::local) + .collect::>(); let skill_root_by_path = skills .iter() .filter_map(|skill| { @@ -957,9 +967,9 @@ mod tests { skill .path_to_skills_md .as_path() - .starts_with(root.as_path()) + .starts_with(root.path().as_path()) }) - .map(|root| (skill.path_to_skills_md.clone(), root.clone())) + .map(|root| (skill.source_path.clone(), root.clone())) }) .collect::>(); SkillLoadOutcome { @@ -1507,6 +1517,7 @@ mod tests { fn skill_with_path(name: &str, path: &AbsolutePathBuf) -> SkillMetadata { let mut skill = make_skill(name, SkillScope::User); skill.path_to_skills_md = path.clone(); + skill.source_path = codex_exec_server::EnvironmentPathRef::local(path.clone()); skill } } diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index 5461323a366..4935515c48b 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -421,13 +421,17 @@ enabled = false let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(&config, effective_skill_roots); - let outcome = skills_manager - .skills_for_config( - &skills_input, - Some(Arc::clone(&codex_exec_server::LOCAL_FS)), - ) - .await; + let cwd = crate::skills::EnvironmentPathRef::new( + Arc::clone(&codex_exec_server::LOCAL_FS), + config.cwd.clone(), + ); + let skills_input = skills_load_input_from_config( + &config, + Some(cwd), + Some(Arc::clone(&codex_exec_server::LOCAL_FS)), + effective_skill_roots, + ); + let outcome = skills_manager.skills_for_config(&skills_input).await; let skill = outcome .skills .iter() diff --git a/codex-rs/core/src/environment_selection.rs b/codex-rs/core/src/environment_selection.rs index 749212f70af..0922dd95a0c 100644 --- a/codex-rs/core/src/environment_selection.rs +++ b/codex-rs/core/src/environment_selection.rs @@ -2,7 +2,6 @@ use std::collections::HashSet; use std::sync::Arc; use codex_exec_server::EnvironmentManager; -use codex_exec_server::ExecutorFileSystem; use codex_protocol::error::CodexErr; use codex_protocol::error::Result as CodexResult; use codex_protocol::protocol::TurnEnvironmentSelection; @@ -45,11 +44,6 @@ impl ResolvedTurnEnvironments { self.primary() .map(|environment| Arc::clone(&environment.environment)) } - - pub(crate) fn primary_filesystem(&self) -> Option> { - self.primary() - .map(|environment| environment.environment.get_filesystem()) - } } pub(crate) fn resolve_environment_selections( diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index c9e1807fe6f..58173f9c38e 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -447,20 +447,31 @@ async fn warm_plugins_and_skills_for_session_init( skills_manager: Arc, environments: Vec, ) -> Vec { - let fs = crate::environment_selection::resolve_environment_selections( + let resolved_environments = crate::environment_selection::resolve_environment_selections( environment_manager.as_ref(), &environments, ) - .ok() - .and_then(|resolved| resolved.primary_filesystem()); + .ok(); + let path_ref = resolved_environments + .as_ref() + .and_then(crate::environment_selection::ResolvedTurnEnvironments::primary) + .map(|environment| { + crate::skills::EnvironmentPathRef::new( + environment.environment.get_filesystem(), + environment.cwd.clone(), + ) + }); + let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS)); let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(config.as_ref(), effective_skill_roots); - skills_manager - .skills_for_config(&skills_input, fs) - .await - .errors + let skills_input = skills_load_input_from_config( + config.as_ref(), + path_ref, + local_file_system, + effective_skill_roots, + ); + skills_manager.skills_for_config(&skills_input).await.errors } impl Session { diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index aa2586cae4f..4b82006ee60 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4127,9 +4127,20 @@ async fn new_default_turn_uses_config_aware_skills_for_role_overrides() { .services .skills_manager .skills_for_cwd( - &crate::skills_load_input_from_config(&parent_config, Vec::new()), + &crate::skills_load_input_from_config( + &parent_config, + Some(crate::skills::EnvironmentPathRef::new( + Arc::clone(&skill_fs), + parent_config.cwd.clone(), + )), + session + .services + .environment_manager + .try_local_environment() + .map(|environment| environment.get_filesystem()), + Vec::new(), + ), /*force_reload*/ true, - Some(Arc::clone(&skill_fs)), ) .await; let parent_skill = parent_outcome @@ -4692,13 +4703,23 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { .plugins_for_config(&per_turn_config.plugins_config_input()) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skill_fs = environment.get_filesystem(); + let cwd = crate::skills::EnvironmentPathRef::new( + environment.get_filesystem(), + per_turn_config.cwd.clone(), + ); + let skills_input = crate::skills_load_input_from_config( + &per_turn_config, + Some(cwd), + services + .environment_manager + .try_local_environment() + .map(|environment| environment.get_filesystem()), + effective_skill_roots, + ); let skills_outcome = Arc::new( services .skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .skills_for_config(&skills_input) .await, ); let turn_environments = turn_environments_for_tests(&environment, &session_configuration.cwd); @@ -6535,13 +6556,23 @@ where .plugins_for_config(&per_turn_config.plugins_config_input()) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skill_fs = environment.get_filesystem(); + let cwd = crate::skills::EnvironmentPathRef::new( + environment.get_filesystem(), + per_turn_config.cwd.clone(), + ); + let skills_input = crate::skills_load_input_from_config( + &per_turn_config, + Some(cwd), + services + .environment_manager + .try_local_environment() + .map(|environment| environment.get_filesystem()), + effective_skill_roots, + ); let skills_outcome = Arc::new( services .skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .skills_for_config(&skills_input) .await, ); let turn_environments = turn_environments_for_tests(&environment, &session_configuration.cwd); @@ -7224,6 +7255,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/admin-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(), scope: SkillScope::Admin, plugin_id: None, @@ -7235,6 +7269,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7271,6 +7308,9 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() { interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7316,6 +7356,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/alpha-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/alpha-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7327,6 +7370,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/beta-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/beta-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, @@ -7375,6 +7421,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/admin-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(), scope: SkillScope::Admin, plugin_id: None, @@ -7386,6 +7435,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil interface: None, dependencies: None, policy: None, + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo-skill/SKILL.md").abs(), + ), path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(), scope: SkillScope::Repo, plugin_id: None, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 638bb911986..9b102cc7e99 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -700,19 +700,32 @@ impl Session { &per_turn_config.to_models_manager_config(), ) .await; + let path_ref = primary_turn_environment.map(|turn_environment| { + crate::skills::EnvironmentPathRef::new( + turn_environment.environment.get_filesystem(), + turn_environment.cwd.clone(), + ) + }); + // Workspace/repo skill roots use the selected turn environment path above when present, + // while user/system/plugin skill roots still read from the local-authority filesystem. + let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS)); + let plugins_input = per_turn_config.plugins_config_input(); let plugin_outcome = self .services .plugins_manager - .plugins_for_config(&per_turn_config.plugins_config_input()) + .plugins_for_config(&plugins_input) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let fs = primary_turn_environment - .map(|turn_environment| turn_environment.environment.get_filesystem()); + let skills_input = skills_load_input_from_config( + &per_turn_config, + path_ref, + local_file_system, + effective_skill_roots, + ); let skills_outcome = Arc::new( self.services .skills_manager - .skills_for_config(&skills_input, fs) + .skills_for_config(&skills_input) .await, ); let goal_tools_supported = !per_turn_config.ephemeral && self.state_db().is_some(); diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index d540cd6d439..38e1e43d2da 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -4,9 +4,12 @@ use crate::session::turn_context::TurnContext; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::build_track_events_context; +pub use codex_exec_server::EnvironmentPathRef; +use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::PluginSkillRoot; +use std::sync::Arc; pub use codex_core_skills::SkillError; pub use codex_core_skills::SkillLoadOutcome; @@ -35,10 +38,13 @@ pub use codex_core_skills::system; pub(crate) fn skills_load_input_from_config( config: &Config, + env_path: Option, + local_file_system: Option>, effective_skill_roots: Vec, ) -> SkillsLoadInput { SkillsLoadInput::new( - config.cwd.clone(), + env_path, + local_file_system, effective_skill_roots, config.config_layer_stack.clone(), config.bundled_skills_enabled(), @@ -51,10 +57,17 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( command: &str, workdir: &AbsolutePathBuf, ) { + let workdir = turn_context + .environments + .primary() + .map(|environment| { + EnvironmentPathRef::new(environment.environment.get_filesystem(), workdir.clone()) + }) + .unwrap_or_else(|| EnvironmentPathRef::local(workdir.clone())); let Some(candidate) = detect_implicit_skill_invocation_for_command( turn_context.turn_skills.outcome.as_ref(), command, - workdir, + &workdir, ) else { return; }; diff --git a/codex-rs/exec-server/src/environment_path.rs b/codex-rs/exec-server/src/environment_path.rs new file mode 100644 index 00000000000..591412ce421 --- /dev/null +++ b/codex-rs/exec-server/src/environment_path.rs @@ -0,0 +1,325 @@ +use std::fmt; +use std::hash::Hash; +use std::hash::Hasher; +use std::io; +use std::path::Path; +use std::sync::Arc; + +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::ExecutorFileSystem; +use crate::FileMetadata; +use crate::FileSystemSandboxContext; +use crate::LOCAL_FS; +use crate::ReadDirectoryEntry; + +/// Binds an absolute path to the executor filesystem that owns it. +#[derive(Clone)] +pub struct EnvironmentPathRef { + file_system: Arc, + path: AbsolutePathBuf, +} + +impl EnvironmentPathRef { + pub fn new(file_system: Arc, path: AbsolutePathBuf) -> Self { + Self { file_system, path } + } + + pub fn local(path: AbsolutePathBuf) -> Self { + Self::new(Arc::clone(&LOCAL_FS), path) + } + + pub fn path(&self) -> &AbsolutePathBuf { + &self.path + } + + pub fn file_system(&self) -> Arc { + Arc::clone(&self.file_system) + } + + pub async fn read_to_string( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result { + self.file_system.read_file_text(&self.path, sandbox).await + } + + pub async fn metadata( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result { + self.file_system.get_metadata(&self.path, sandbox).await + } + + pub async fn read_directory( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result> { + self.file_system.read_directory(&self.path, sandbox).await + } + + pub fn join_relative(&self, relative: &Path) -> Option { + relative + .is_relative() + .then(|| self.with_path(self.path.join(relative))) + } + + pub fn parent_dir(&self) -> Option { + self.path.parent().map(|path| self.with_path(path)) + } + + pub fn with_path(&self, path: AbsolutePathBuf) -> Self { + Self::new(Arc::clone(&self.file_system), path) + } +} + +impl PartialEq for EnvironmentPathRef { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.file_system, &other.file_system) && self.path == other.path + } +} + +impl Eq for EnvironmentPathRef {} + +impl Hash for EnvironmentPathRef { + fn hash(&self, state: &mut H) { + (Arc::as_ptr(&self.file_system) as *const () as usize).hash(state); + self.path.hash(state); + } +} + +impl fmt::Debug for EnvironmentPathRef { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("EnvironmentPathRef") + .field("path", &self.path) + .finish() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use async_trait::async_trait; + use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::NetworkSandboxPolicy; + use codex_utils_absolute_path::test_support::PathBufExt; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::sync::Mutex; + + #[derive(Clone, Debug, Eq, PartialEq)] + enum RecordedMethod { + ReadFileText, + Metadata, + ReadDirectory, + } + + #[derive(Clone, Debug, Eq, PartialEq)] + struct RecordedCall { + method: RecordedMethod, + path: AbsolutePathBuf, + sandbox: Option, + } + + #[derive(Default)] + struct RecordingFileSystem { + calls: Mutex>, + } + + impl RecordingFileSystem { + fn recorded_calls(&self) -> Vec { + match self.calls.lock() { + Ok(calls) => calls.clone(), + Err(err) => err.into_inner().clone(), + } + } + + fn push_call(&self, call: RecordedCall) { + match self.calls.lock() { + Ok(mut calls) => calls.push(call), + Err(err) => err.into_inner().push(call), + } + } + } + + #[async_trait] + impl ExecutorFileSystem for RecordingFileSystem { + async fn read_file( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result> { + self.push_call(RecordedCall { + method: RecordedMethod::ReadFileText, + path: path.clone(), + sandbox: sandbox.cloned(), + }); + Ok(b"skill contents".to_vec()) + } + + async fn write_file( + &self, + _path: &AbsolutePathBuf, + _contents: Vec, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result<()> { + unreachable!("write_file should not be called") + } + + async fn create_directory( + &self, + _path: &AbsolutePathBuf, + _create_directory_options: crate::CreateDirectoryOptions, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result<()> { + unreachable!("create_directory should not be called") + } + + async fn get_metadata( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result { + self.push_call(RecordedCall { + method: RecordedMethod::Metadata, + path: path.clone(), + sandbox: sandbox.cloned(), + }); + Ok(FileMetadata { + is_directory: true, + is_file: false, + is_symlink: false, + created_at_ms: 1, + modified_at_ms: 2, + }) + } + + async fn read_directory( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result> { + self.push_call(RecordedCall { + method: RecordedMethod::ReadDirectory, + path: path.clone(), + sandbox: sandbox.cloned(), + }); + Ok(vec![ReadDirectoryEntry { + file_name: "SKILL.md".to_string(), + is_directory: false, + is_file: true, + }]) + } + + async fn remove( + &self, + _path: &AbsolutePathBuf, + _remove_options: crate::RemoveOptions, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result<()> { + unreachable!("remove should not be called") + } + + async fn copy( + &self, + _source_path: &AbsolutePathBuf, + _destination_path: &AbsolutePathBuf, + _copy_options: crate::CopyOptions, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result<()> { + unreachable!("copy should not be called") + } + } + + fn restricted_sandbox() -> FileSystemSandboxContext { + FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + ), + ) + } + + #[tokio::test] + async fn environment_path_ref_forwards_sandbox_to_file_system_methods() { + let path = std::env::temp_dir().join("skills/demo").abs(); + let file_system = Arc::new(RecordingFileSystem::default()); + let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone()); + let sandbox = restricted_sandbox(); + + assert_eq!( + path_ref + .read_to_string(Some(&sandbox)) + .await + .expect("read skill contents"), + "skill contents".to_string() + ); + assert_eq!( + path_ref + .metadata(Some(&sandbox)) + .await + .expect("read metadata"), + FileMetadata { + is_directory: true, + is_file: false, + is_symlink: false, + created_at_ms: 1, + modified_at_ms: 2, + } + ); + assert_eq!( + path_ref + .read_directory(Some(&sandbox)) + .await + .expect("read directory"), + vec![ReadDirectoryEntry { + file_name: "SKILL.md".to_string(), + is_directory: false, + is_file: true, + }] + ); + assert_eq!( + file_system.recorded_calls(), + vec![ + RecordedCall { + method: RecordedMethod::ReadFileText, + path: path.clone(), + sandbox: Some(sandbox.clone()), + }, + RecordedCall { + method: RecordedMethod::Metadata, + path: path.clone(), + sandbox: Some(sandbox.clone()), + }, + RecordedCall { + method: RecordedMethod::ReadDirectory, + path, + sandbox: Some(sandbox), + }, + ] + ); + } + + #[test] + fn environment_path_ref_equality_and_hash_include_file_system_identity() { + let path = std::env::temp_dir().join("skills/demo").abs(); + let file_system = Arc::new(RecordingFileSystem::default()); + let same_file_system: Arc = file_system.clone(); + let different_file_system: Arc = + Arc::new(RecordingFileSystem::default()); + + let left = EnvironmentPathRef::new(same_file_system.clone(), path.clone()); + let same = EnvironmentPathRef::new(same_file_system, path.clone()); + let different_path = EnvironmentPathRef::new(file_system, path.parent().unwrap()); + let different_fs = EnvironmentPathRef::new(different_file_system, path); + + assert_eq!(left, same); + assert_ne!(left, different_path); + assert_ne!(left, different_fs); + + let set = HashSet::from([left, same, different_path, different_fs]); + assert_eq!(set.len(), 3); + } +} diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index f2d16f8fac9..01b530ba341 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -3,6 +3,7 @@ mod client_api; mod client_transport; mod connection; mod environment; +mod environment_path; mod environment_provider; mod environment_toml; mod fs_helper; @@ -43,6 +44,7 @@ pub use environment::Environment; pub use environment::EnvironmentManager; pub use environment::LOCAL_ENVIRONMENT_ID; pub use environment::REMOTE_ENVIRONMENT_ID; +pub use environment_path::EnvironmentPathRef; pub use environment_provider::DefaultEnvironmentProvider; pub use environment_provider::EnvironmentProvider; pub use fs_helper::CODEX_FS_HELPER_ARG1; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 721824ed61f..b8d3ff6323c 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -6255,6 +6255,7 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: skill_path.clone(), + source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()), scope: crate::test_support::skill_scope_user(), plugin_id: None, }])); @@ -6298,6 +6299,7 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: skill_path.clone(), + source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()), scope: crate::test_support::skill_scope_repo(), plugin_id: None, }])); @@ -6390,6 +6392,9 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(), + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(), + ), scope: crate::test_support::skill_scope_repo(), plugin_id: None, }])); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index fa92914670a..8e686a799dc 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -2577,6 +2577,9 @@ mod tests { dependencies: None, policy: None, path_to_skills_md: test_path_buf("/tmp/test-skill/SKILL.md").abs(), + source_path: codex_exec_server::EnvironmentPathRef::local( + test_path_buf("/tmp/test-skill/SKILL.md").abs(), + ), scope: crate::test_support::skill_scope_user(), plugin_id: None, }]), diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index 687ce564166..5b32f2662ff 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -242,6 +242,9 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> Option