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 6f17e4614f..b27476f836 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -513,15 +513,14 @@ 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() + let environment_manager = self.thread_manager.environment_manager(); + let local_fs = environment_manager + .try_local_environment() .map(|environment| environment.get_filesystem()); let mut data = futures::stream::iter(cwds.into_iter().enumerate()) .map(|(index, cwd)| { let config = &config; - let fs = fs.clone(); + let local_fs = local_fs.clone(); let plugins_manager = &plugins_manager; let skills_manager = &skills_manager; async move { @@ -542,6 +541,23 @@ impl CatalogRequestProcessor { ); } }; + let local_path_ref = local_fs.clone().map(|file_system| { + codex_core::skills::EnvironmentPathRef::new( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + file_system, + cwd_abs.clone(), + ) + }); + let Some(path_ref) = local_path_ref else { + return ( + index, + codex_app_server_protocol::SkillsListEntry { + cwd, + skills: Vec::new(), + errors: Vec::new(), + }, + ); + }; let effective_skill_roots = if workspace_codex_plugins_enabled { let plugins_input = config.plugins_config_input(); plugins_manager @@ -554,13 +570,13 @@ impl CatalogRequestProcessor { Vec::new() }; let skills_input = codex_core::skills::SkillsLoadInput::new( - cwd_abs.clone(), + path_ref, 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 57edb89c7f..9844e8fec4 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -7,6 +7,7 @@ use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::SkillsChangedNotification; use codex_core::ThreadManager; use codex_core::config::Config; +use codex_core::skills::EnvironmentPathRef; use codex_core::skills::SkillsLoadInput; use codex_core::skills::SkillsManager; use codex_file_watcher::FileWatcher; @@ -100,22 +101,27 @@ impl SkillsWatcher { return WatchRegistration::default(); } + let root_path_ref = EnvironmentPathRef::new( + environment_selection.environment_id.clone(), + environment.get_filesystem(), + config.cwd.clone(), + ); 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(), + root_path_ref, 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 9e0ef7c471..4fc7cfa235 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; @@ -565,7 +564,7 @@ async fn load_plugin( loaded_plugin.manifest_description = manifest.description.clone(); loaded_plugin.skill_roots = plugin_skill_roots(&plugin_root, manifest_paths); let resolved_skills = load_plugin_skills( - &plugin_root, + EnvironmentPathRef::local(plugin_root.clone()), &loaded_plugin_id, manifest_paths, restriction_product, @@ -642,18 +641,17 @@ impl ResolvedPluginSkills { } pub async fn load_plugin_skills( - plugin_root: &AbsolutePathBuf, + plugin_root: EnvironmentPathRef, plugin_id: &PluginId, manifest_paths: &PluginManifestPaths, restriction_product: Option, skill_config_rules: &SkillConfigRules, ) -> ResolvedPluginSkills { - let roots = plugin_skill_roots(plugin_root, manifest_paths) + let roots = plugin_skill_roots(plugin_root.path(), manifest_paths) .into_iter() .map(|path| SkillRoot { - path, + path: plugin_root.with_path(path), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some(plugin_id.as_key()), plugin_root: Some(plugin_root.clone()), }) diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index ec3e74dd16..e387607c85 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -1420,7 +1420,7 @@ impl PluginsManager { marketplace_category, ); let resolved_skills = load_plugin_skills( - &source_path, + codex_exec_server::EnvironmentPathRef::local(source_path.clone()), &plugin_id, &manifest.paths, self.restriction_product, diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 8663737ee5..98e3448106 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -13,8 +13,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 +151,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 @@ -168,12 +167,12 @@ where let mut file_systems_by_skill_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 fs = root_path_ref.file_system(); 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(), @@ -231,18 +230,16 @@ where } pub(crate) async fn skill_roots( - fs: Option>, + path_ref: &EnvironmentPathRef, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, plugin_skill_roots: Vec, extra_skill_roots: Vec, ) -> Vec { let home_dir = home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); skill_roots_with_home_dir( - fs, + path_ref, config_layer_stack, - cwd, home_dir.as_ref(), plugin_skill_roots, extra_skill_roots, @@ -251,29 +248,26 @@ pub(crate) async fn skill_roots( } async fn skill_roots_with_home_dir( - fs: Option>, + path_ref: &EnvironmentPathRef, 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()); + let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, path_ref); roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot { - path: root.path, + path: path_ref.with_path(root.path), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: Some(root.plugin_id), - plugin_root: Some(root.plugin_root), + plugin_root: Some(path_ref.with_path(root.plugin_root)), })); roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot { - path, + path: path_ref.with_path(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); + roots.extend(repo_agents_skill_roots(path_ref, config_layer_stack).await); dedupe_skill_roots_by_path(&mut roots); roots } @@ -281,7 +275,7 @@ async fn skill_roots_with_home_dir( fn skill_roots_from_layer_stack_inner( config_layer_stack: &ConfigLayerStack, home_dir: Option<&AbsolutePathBuf>, - repo_fs: Option>, + path_ref: &EnvironmentPathRef, ) -> Vec { let mut roots = Vec::new(); @@ -295,23 +289,19 @@ 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, - }); - } + roots.push(SkillRoot { + path: path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), + scope: SkillScope::Repo, + plugin_id: None, + plugin_root: None, + }); } ConfigLayerSource::User { .. } => { // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward // compatibility. roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -319,9 +309,9 @@ 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: path_ref + .with_path(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 +320,8 @@ 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: path_ref.with_path(system_cache_root_dir(&config_folder)), scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -341,9 +330,8 @@ 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: path_ref.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }); @@ -359,24 +347,19 @@ fn skill_roots_from_layer_stack_inner( } async fn repo_agents_skill_roots( - fs: Option>, + path_ref: &EnvironmentPathRef, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, ) -> Vec { - let Some(fs) = fs else { - return Vec::new(); - }; 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(path_ref, &project_root_markers).await; + let dirs = dirs_between_project_root_and_cwd(path_ref.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 = path_ref.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 +368,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 +399,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,7 +447,7 @@ 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())); } @@ -474,31 +456,35 @@ fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { } 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 +494,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 +506,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 +526,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 +544,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 +564,7 @@ async fn discover_skills_under_root( Err(err) => { error!( "failed to read skills symlink dir {}: {err:#}", - path.display() + path.path().display() ); } } @@ -580,7 +572,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 +584,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 +605,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 +633,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 +649,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")?; @@ -708,18 +700,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 metadata_path = skill_dir - .join(SKILLS_METADATA_DIR) - .join(SKILLS_METADATA_FILENAME); - match fs.get_metadata(&metadata_path, /*sandbox*/ None).await { + let Some(metadata_path) = skill_dir.join_relative( + Path::new(SKILLS_METADATA_DIR) + .join(SKILLS_METADATA_FILENAME) + .as_path(), + ) else { + return LoadedSkillMetadata::default(); + }; + match metadata_path.metadata(/*sandbox*/ None).await { Ok(metadata) if metadata.is_file => {} Ok(_) => return LoadedSkillMetadata::default(), Err(error) if error.kind() == io::ErrorKind::NotFound => { @@ -728,19 +723,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 +743,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 +763,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,15 +1060,13 @@ pub(crate) async fn skill_roots_from_layer_stack( cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, ) -> Vec { - skill_roots_with_home_dir( - Some(fs), - config_layer_stack, - cwd, - home_dir, - Vec::new(), - Vec::new(), - ) - .await + let path_ref = EnvironmentPathRef::new( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + fs, + cwd.clone(), + ); + skill_roots_with_home_dir(&path_ref, config_layer_stack, home_dir, Vec::new(), Vec::new()) + .await } #[cfg(test)] diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 69e357d93d..8ea015d030 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,6 +4,7 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -144,6 +145,23 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } +fn env_path(path: AbsolutePathBuf) -> EnvironmentPathRef { + EnvironmentPathRef::new( + LOCAL_ENVIRONMENT_ID.to_string(), + Arc::clone(&LOCAL_FS), + path, + ) +} + +fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot { + SkillRoot { + path: 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 +205,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 +274,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!( @@ -846,11 +864,10 @@ interface: let plugin_root_abs = plugin_root.abs(); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: 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(env_path(plugin_root_abs.clone())), }]) .await; @@ -903,11 +920,10 @@ interface: ); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: 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(env_path(plugin_root.abs())), }]) .await; @@ -1048,14 +1064,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(), @@ -1130,14 +1141,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 +1169,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(), @@ -1272,11 +1271,10 @@ async fn namespaces_plugin_skills_using_plugin_name() { .unwrap(); let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), + path: 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(env_path(plugin_root.abs())), }]) .await; @@ -1595,16 +1593,14 @@ async fn deduplicates_by_path_preferring_first_root() { let outcome = load_skills_from_roots([ SkillRoot { - path: root.path().abs(), + path: 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: env_path(root.path().abs()), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), plugin_id: None, plugin_root: None, }, @@ -1895,17 +1891,13 @@ async fn skill_roots_include_admin_with_lowest_priority() { let codex_home = tempfile::tempdir().expect("tempdir"); let cfg = make_config(&codex_home).await; - let scopes: Vec = super::skill_roots( - Some(Arc::clone(&LOCAL_FS)), - &cfg.config_layer_stack, - &cfg.cwd, - Vec::new(), - Vec::new(), - ) - .await - .into_iter() - .map(|root| root.scope) - .collect(); + let cwd = env_path(cfg.cwd.clone()); + let scopes: Vec = + super::skill_roots(&cwd, &cfg.config_layer_stack, Vec::new(), Vec::new()) + .await + .into_iter() + .map(|root| root.scope) + .collect(); let mut expected = vec![SkillScope::User, SkillScope::System]; if home_dir().is_some() { expected.insert(1, SkillScope::User); diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 185eaa9320..71739c65b8 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,10 +22,11 @@ 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; #[derive(Debug, Clone)] pub struct SkillsLoadInput { - pub cwd: AbsolutePathBuf, + pub path_ref: EnvironmentPathRef, pub effective_skill_roots: Vec, pub config_layer_stack: ConfigLayerStack, pub bundled_skills_enabled: bool, @@ -34,13 +34,13 @@ pub struct SkillsLoadInput { impl SkillsLoadInput { pub fn new( - cwd: AbsolutePathBuf, + path_ref: EnvironmentPathRef, effective_skill_roots: Vec, config_layer_stack: ConfigLayerStack, bundled_skills_enabled: bool, ) -> Self { Self { - cwd, + path_ref, effective_skill_roots, config_layer_stack, bundled_skills_enabled, @@ -52,7 +52,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 +70,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 +100,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 +117,10 @@ 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.path_ref, &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -144,20 +135,17 @@ 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 = input.path_ref.clone(); + 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.path_ref, &input.config_layer_stack, - &input.cwd, input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -167,13 +155,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 +177,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 +195,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, + path_ref: &EnvironmentPathRef, + ) -> Option { + match self.cache_by_path_ref.read() { + Ok(cache) => cache.get(path_ref).cloned(), + Err(err) => err.into_inner().get(path_ref).cloned(), } } @@ -240,7 +229,7 @@ impl SkillsManager { #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { - roots: Vec<(AbsolutePathBuf, u8, Option)>, + roots: Vec<(EnvironmentPathRef, u8, Option)>, skill_config_rules: SkillConfigRules, } diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 713b94d7b1..7c78143583 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/manager_tests.rs @@ -7,6 +7,7 @@ use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; +use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::LOCAL_FS; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; @@ -87,6 +88,28 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { } } +fn skill_root_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef { + EnvironmentPathRef::new( + LOCAL_ENVIRONMENT_ID.to_string(), + Arc::clone(&LOCAL_FS), + 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( + path_ref, + 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 +187,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] @@ -421,23 +441,14 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { ConfigRequirementsToml::default(), ) .expect("valid config layer stack"); - let skills_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); + let skills_input = local_skills_input(cwd.path().abs(), Vec::new(), config_layer_stack.clone()); let skills_manager = SkillsManager::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); 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!( @@ -454,65 +465,6 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { assert!(loaded_names.contains("repo-skill")); } -#[tokio::test] -async fn skills_for_cwd_without_fs_skips_repo_roots() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let cwd = tempfile::tempdir().expect("tempdir"); - let repo_dot_codex = cwd.path().join(".codex"); - fs::create_dir_all(&repo_dot_codex).expect("create repo config dir"); - - write_user_skill(&codex_home, "user", "user-skill", "from local user root"); - let repo_skill_dir = repo_dot_codex.join("skills/repo"); - fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir"); - fs::write( - repo_skill_dir.join("SKILL.md"), - "---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n", - ) - .expect("write repo skill"); - - let config_layer_stack = ConfigLayerStack::new( - vec![ - user_config_layer(&codex_home, ""), - ConfigLayerEntry::new( - ConfigLayerSource::Project { - dot_codex_folder: repo_dot_codex.abs(), - }, - toml::Value::Table(toml::map::Map::new()), - ), - ], - Default::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"); - let skills_input = SkillsLoadInput::new( - cwd.path().abs(), - Vec::new(), - config_layer_stack.clone(), - bundled_skills_enabled_from_stack(&config_layer_stack), - ); - 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, /*fs*/ None) - .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_config_excludes_bundled_skills_when_disabled_in_config() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -565,18 +517,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 +531,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 +541,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 @@ -779,19 +714,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/model.rs b/codex-rs/core-skills/src/model.rs index fc8e9f5917..832af06bb4 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -1,12 +1,11 @@ -use std::collections::HashMap; -use std::collections::HashSet; -use std::fmt; -use std::sync::Arc; - use codex_exec_server::ExecutorFileSystem; 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::fmt; +use std::sync::Arc; #[derive(Debug, Clone, PartialEq)] pub struct SkillMetadata { @@ -210,3 +209,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/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index 5461323a36..8e20d09506 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -421,13 +421,13 @@ 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::environment_path( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + Arc::clone(&codex_exec_server::LOCAL_FS), + config.cwd.clone(), + ); + let skills_input = skills_load_input_from_config(&config, cwd, 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 749212f70a..0922dd95a0 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/lib.rs b/codex-rs/core/src/lib.rs index aa5784d4fe..da60668e68 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -92,6 +92,7 @@ pub(crate) use skills::build_skill_injections; pub(crate) use skills::build_skill_name_counts; pub(crate) use skills::collect_explicit_skill_mentions; pub(crate) use skills::default_skill_metadata_budget; +pub(crate) use skills::environment_path_ref; pub(crate) use skills::injection; pub(crate) use skills::manager; pub(crate) use skills::maybe_emit_implicit_skill_invocation; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index c9e1807fe6..47b65cf207 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1,6 +1,7 @@ use super::input_queue::InputQueue; use super::*; use crate::config::ConstraintError; +use crate::environment_path; use crate::goals::GoalRuntimeState; use crate::skills::SkillError; use crate::state::ActiveTurn; @@ -447,20 +448,30 @@ 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| { + environment_path( + environment.environment_id.clone(), + environment.environment.get_filesystem(), + environment.cwd.clone(), + ) + }); + let Some(path_ref) = path_ref else { + return Vec::new(); + }; 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, 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 62321756db..971c05eb10 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4127,9 +4127,16 @@ 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, + crate::environment_path( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + Arc::clone(&skill_fs), + parent_config.cwd.clone(), + ), + Vec::new(), + ), /*force_reload*/ true, - Some(Arc::clone(&skill_fs)), ) .await; let parent_skill = parent_outcome @@ -4692,13 +4699,17 @@ 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 cwd = crate::environment_path( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + environment.get_filesystem(), + per_turn_config.cwd.clone(), + ); let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skill_fs = environment.get_filesystem(); + crate::skills_load_input_from_config(&per_turn_config, cwd, 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 +6546,17 @@ where .plugins_for_config(&per_turn_config.plugins_config_input()) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); + let cwd = crate::environment_path( + codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(), + environment.get_filesystem(), + per_turn_config.cwd.clone(), + ); let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skill_fs = environment.get_filesystem(); + crate::skills_load_input_from_config(&per_turn_config, cwd, 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); diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index dba5d16a75..5253fe7d3e 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -1,6 +1,7 @@ use super::*; use crate::SkillLoadOutcome; use crate::config::GhostSnapshotConfig; +use crate::environment_path; use crate::environment_selection::ResolvedTurnEnvironments; use codex_model_provider::SharedModelProvider; use codex_model_provider::create_model_provider; @@ -678,21 +679,32 @@ impl Session { &per_turn_config.to_models_manager_config(), ) .await; - let plugin_outcome = self - .services - .plugins_manager - .plugins_for_config(&per_turn_config.plugins_config_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_outcome = Arc::new( - self.services - .skills_manager - .skills_for_config(&skills_input, fs) - .await, - ); + let path_ref = primary_turn_environment.map(|turn_environment| { + environment_path( + turn_environment.environment_id.clone(), + turn_environment.environment.get_filesystem(), + turn_environment.cwd.clone(), + ) + }); + let skills_outcome = if let Some(path_ref) = path_ref { + let plugins_input = per_turn_config.plugins_config_input(); + let plugin_outcome = self + .services + .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(&per_turn_config, path_ref, effective_skill_roots); + Arc::new( + self.services + .skills_manager + .skills_for_config(&skills_input) + .await, + ) + } else { + Arc::new(SkillLoadOutcome::default()) + }; let goal_tools_supported = !per_turn_config.ephemeral && self.state_db().is_some(); let mut turn_context: TurnContext = Self::make_turn_context( self.thread_id(), diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index d540cd6d43..86aaf004ea 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,16 +38,25 @@ pub use codex_core_skills::system; pub(crate) fn skills_load_input_from_config( config: &Config, + path_ref: EnvironmentPathRef, effective_skill_roots: Vec, ) -> SkillsLoadInput { SkillsLoadInput::new( - config.cwd.clone(), + path_ref, effective_skill_roots, config.config_layer_stack.clone(), config.bundled_skills_enabled(), ) } +pub(crate) fn environment_path( + environment_id: String, + file_system: Arc, + path: AbsolutePathBuf, +) -> EnvironmentPathRef { + EnvironmentPathRef::new(environment_id, file_system, path) +} + pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, turn_context: &TurnContext, 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 0000000000..64d9f327d7 --- /dev/null +++ b/codex-rs/exec-server/src/environment_path.rs @@ -0,0 +1,355 @@ +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_ENVIRONMENT_ID; +use crate::LOCAL_FS; +use crate::ReadDirectoryEntry; + +/// Binds an absolute path to the executor filesystem and environment that owns it. +#[derive(Clone)] +pub struct EnvironmentPathRef { + environment_id: String, + file_system: Arc, + path: AbsolutePathBuf, +} + +impl EnvironmentPathRef { + pub fn new( + environment_id: String, + file_system: Arc, + path: AbsolutePathBuf, + ) -> Self { + Self { + environment_id, + file_system, + path, + } + } + + pub fn local(path: AbsolutePathBuf) -> Self { + Self::new( + LOCAL_ENVIRONMENT_ID.to_string(), + Arc::clone(&LOCAL_FS), + path, + ) + } + + pub fn path(&self) -> &AbsolutePathBuf { + &self.path + } + + pub fn environment_id(&self) -> &str { + &self.environment_id + } + + 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( + self.environment_id.clone(), + Arc::clone(&self.file_system), + path, + ) + } +} + +impl PartialEq for EnvironmentPathRef { + fn eq(&self, other: &Self) -> bool { + self.environment_id == other.environment_id + && 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) { + self.environment_id.hash(state); + (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("environment_id", &self.environment_id) + .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("local".to_string(), 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_environment_id_and_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("local".to_string(), same_file_system.clone(), path.clone()); + let same = EnvironmentPathRef::new("local".to_string(), same_file_system, path.clone()); + let different_env = + EnvironmentPathRef::new("remote".to_string(), file_system.clone(), path.clone()); + let different_fs = + EnvironmentPathRef::new("local".to_string(), different_file_system, path); + + assert_eq!(left, same); + assert_ne!(left, different_env); + assert_ne!(left, different_fs); + + let set = HashSet::from([left, same, different_env, 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 f2d16f8fac..01b530ba34 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;