diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1382c61b4..ea79776bd 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2015,6 +2015,7 @@ dependencies = [ "codex-analytics", "codex-app-server-protocol", "codex-config", + "codex-exec-server", "codex-instructions", "codex-login", "codex-otel", @@ -3075,10 +3076,13 @@ dependencies = [ name = "codex-utils-plugins" version = "0.0.0" dependencies = [ + "codex-exec-server", "codex-login", + "codex-utils-absolute-path", "serde", "serde_json", "tempfile", + "tokio", ] [[package]] diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 1864509e8..f31aafeac 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -6292,6 +6292,23 @@ impl CodexMessageProcessor { }; let skills_manager = self.thread_manager.skills_manager(); let plugins_manager = self.thread_manager.plugins_manager(); + let fs = match self.thread_manager.environment_manager().current().await { + Ok(Some(environment)) => Some(environment.get_filesystem()), + Ok(None) => None, + Err(err) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to create environment: {err}"), + data: None, + }, + ) + .await; + return; + } + }; let cli_overrides = self.current_cli_overrides(); let mut data = Vec::new(); for cwd in cwds { @@ -6349,7 +6366,12 @@ impl CodexMessageProcessor { config.bundled_skills_enabled(), ); let outcome = skills_manager - .skills_for_cwd_with_extra_user_roots(&skills_input, force_reload, extra_roots) + .skills_for_cwd_with_extra_user_roots( + &skills_input, + force_reload, + extra_roots, + fs.clone(), + ) .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/tests/suite/v2/skills_list.rs b/codex-rs/app-server/tests/suite/v2/skills_list.rs index 51dae76b0..8675b3a42 100644 --- a/codex-rs/app-server/tests/suite/v2/skills_list.rs +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -11,6 +11,7 @@ use codex_app_server_protocol::SkillsListExtraRootsForCwd; use codex_app_server_protocol::SkillsListParams; use codex_app_server_protocol::SkillsListResponse; use codex_app_server_protocol::ThreadStartParams; +use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; use pretty_assertions::assert_eq; use tempfile::TempDir; use tokio::time::timeout; @@ -64,6 +65,56 @@ async fn skills_list_includes_skills_from_per_cwd_extra_user_roots() -> Result<( Ok(()) } +#[tokio::test] +async fn skills_list_skips_cwd_roots_when_environment_disabled() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&codex_home, "home-skill")?; + write_skill(&extra_root, "extra-skill")?; + + let mut mcp = McpProcess::new_with_env( + codex_home.path(), + &[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("none"))], + ) + .await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let SkillsListResponse { data } = to_response(response)?; + assert_eq!(data.len(), 1); + assert_eq!(data[0].cwd, cwd.path().to_path_buf()); + assert_eq!(data[0].errors, Vec::new()); + assert!( + data[0] + .skills + .iter() + .any(|skill| skill.name == "home-skill") + ); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.name != "extra-skill") + ); + Ok(()) +} + #[tokio::test] async fn skills_list_rejects_relative_extra_user_roots() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 35c9ed0a5..bc73909f9 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -17,6 +17,7 @@ anyhow = { workspace = true } codex-analytics = { workspace = true } codex-app-server-protocol = { workspace = true } codex-config = { workspace = true } +codex-exec-server = { workspace = true } codex-instructions = { workspace = true } codex-login = { workspace = true } codex-otel = { workspace = true } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 444942389..1ccd447cf 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -1,19 +1,21 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::sync::Arc; +use crate::SkillLoadOutcome; use crate::SkillMetadata; use crate::build_skill_name_counts; use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; use codex_analytics::SkillInvocation; use codex_analytics::TrackEventsContext; +use codex_exec_server::LOCAL_FS; use codex_instructions::SkillInstructions; use codex_otel::SessionTelemetry; use codex_protocol::models::ResponseItem; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; -use tokio::fs; #[derive(Debug, Default)] pub struct SkillInjections { @@ -23,6 +25,7 @@ pub struct SkillInjections { pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], + loaded_skills: Option<&SkillLoadOutcome>, otel: Option<&SessionTelemetry>, analytics_client: &AnalyticsEventsClient, tracking: TrackEventsContext, @@ -38,7 +41,13 @@ pub async fn build_skill_injections( let mut invocations = Vec::new(); for skill in mentioned_skills { - match fs::read_to_string(&skill.path_to_skills_md).await { + 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 + { Ok(contents) => { emit_skill_injected_metric(otel, skill, "ok"); invocations.push(SkillInvocation { diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index b8303df1a..2cae6a4b0 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,5 +1,6 @@ use crate::model::SkillDependencies; use crate::model::SkillError; +use crate::model::SkillFileSystemsByPath; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; @@ -12,22 +13,24 @@ 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::ExecutorFileSystem; +use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_plugins::plugin_namespace_for_skill_path; use dirs::home_dir; -use dunce::canonicalize as canonicalize_path; use serde::Deserialize; +use std::collections::HashMap; use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; use std::fmt; -use std::fs; +use std::io; use std::path::Component; -use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use toml::Value as TomlValue; use tracing::error; @@ -148,21 +151,38 @@ impl Error for SkillParseError {} pub struct SkillRoot { pub path: AbsolutePathBuf, pub scope: SkillScope, + pub file_system: Arc, } -pub fn load_skills_from_roots(roots: I) -> SkillLoadOutcome +pub async fn load_skills_from_roots(roots: I) -> SkillLoadOutcome where I: IntoIterator, { let mut outcome = SkillLoadOutcome::default(); + let mut file_systems_by_skill_path: HashMap> = + HashMap::new(); for root in roots { - discover_skills_under_root(&root.path, root.scope, &mut outcome); + let fs = root.file_system; + let skills_before_root = outcome.skills.len(); + discover_skills_under_root(fs.as_ref(), &root.path, root.scope, &mut outcome).await; + for skill in &outcome.skills[skills_before_root..] { + file_systems_by_skill_path + .entry(skill.path_to_skills_md.clone()) + .or_insert_with(|| Arc::clone(&fs)); + } } let mut seen: HashSet = HashSet::new(); outcome .skills .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); + let retained_skill_paths: HashSet = outcome + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect(); + file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(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). @@ -184,7 +204,8 @@ where outcome } -pub(crate) fn skill_roots( +pub(crate) async fn skill_roots( + fs: Option>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, plugin_skill_roots: Vec, @@ -192,25 +213,29 @@ pub(crate) fn skill_roots( let home_dir = home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); skill_roots_with_home_dir( + fs, config_layer_stack, cwd, home_dir.as_ref(), plugin_skill_roots, ) + .await } -fn skill_roots_with_home_dir( +async fn skill_roots_with_home_dir( + fs: Option>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, ) -> Vec { - let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir); + let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, fs.clone()); roots.extend(plugin_skill_roots.into_iter().map(|path| SkillRoot { path, scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), })); - roots.extend(repo_agents_skill_roots(config_layer_stack, cwd)); + roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); dedupe_skill_roots_by_path(&mut roots); roots } @@ -218,6 +243,7 @@ fn skill_roots_with_home_dir( fn skill_roots_from_layer_stack_inner( config_layer_stack: &ConfigLayerStack, home_dir: Option<&AbsolutePathBuf>, + repo_fs: Option>, ) -> Vec { let mut roots = Vec::new(); @@ -231,10 +257,13 @@ fn skill_roots_from_layer_stack_inner( match &layer.name { ConfigLayerSource::Project { .. } => { - roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), - scope: SkillScope::Repo, - }); + 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), + }); + } } ConfigLayerSource::User { .. } => { // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward @@ -242,6 +271,7 @@ fn skill_roots_from_layer_stack_inner( roots.push(SkillRoot { path: config_folder.join(SKILLS_DIR_NAME), scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), }); // `$HOME/.agents/skills` (user-installed skills). @@ -249,6 +279,7 @@ fn skill_roots_from_layer_stack_inner( roots.push(SkillRoot { path: home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), }); } @@ -257,6 +288,7 @@ fn skill_roots_from_layer_stack_inner( roots.push(SkillRoot { path: system_cache_root_dir(&config_folder), scope: SkillScope::System, + file_system: Arc::clone(&LOCAL_FS), }); } ConfigLayerSource::System { .. } => { @@ -265,6 +297,7 @@ fn skill_roots_from_layer_stack_inner( roots.push(SkillRoot { path: config_folder.join(SKILLS_DIR_NAME), scope: SkillScope::Admin, + file_system: Arc::clone(&LOCAL_FS), }); } ConfigLayerSource::Mdm { .. } @@ -277,21 +310,34 @@ fn skill_roots_from_layer_stack_inner( roots } -fn repo_agents_skill_roots( +async fn repo_agents_skill_roots( + fs: Option>, 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(cwd, &project_root_markers); + 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 mut roots = Vec::new(); for dir in dirs { let agents_skills = dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); - if agents_skills.is_dir() { - roots.push(SkillRoot { + match fs.get_metadata(&agents_skills, /*sandbox*/ None).await { + Ok(metadata) if metadata.is_directory => roots.push(SkillRoot { path: agents_skills, scope: SkillScope::Repo, - }); + file_system: Arc::clone(&fs), + }), + Ok(_) => {} + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => { + tracing::warn!( + "failed to stat repo skills root {}: {err:#}", + agents_skills.display() + ); + } } } roots @@ -319,16 +365,27 @@ fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec } } -fn find_project_root(cwd: &AbsolutePathBuf, project_root_markers: &[String]) -> AbsolutePathBuf { +async fn find_project_root( + fs: &dyn ExecutorFileSystem, + cwd: &AbsolutePathBuf, + project_root_markers: &[String], +) -> AbsolutePathBuf { if project_root_markers.is_empty() { return cwd.clone(); } - for path in cwd.ancestors() { + for ancestor in cwd.ancestors() { for marker in project_root_markers { - let marker_path = path.join(marker); - if marker_path.exists() { - return path; + let marker_path = ancestor.join(marker); + match fs.get_metadata(&marker_path, /*sandbox*/ None).await { + Ok(_) => return ancestor, + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(err) => { + tracing::warn!( + "failed to stat project root marker {}: {err:#}", + marker_path.display() + ); + } } } } @@ -362,24 +419,33 @@ fn dedupe_skill_roots_by_path(roots: &mut Vec) { roots.retain(|root| seen.insert(root.path.clone())); } -fn discover_skills_under_root( +fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { + path.canonicalize().unwrap_or_else(|_| path.clone()) +} + +async fn discover_skills_under_root( + fs: &dyn ExecutorFileSystem, root: &AbsolutePathBuf, scope: SkillScope, outcome: &mut SkillLoadOutcome, ) { - let Ok(root) = canonicalize_path(root.as_path()) else { - return; - }; + let root = canonicalize_for_skill_identity(root); - if !root.is_dir() { - return; + match fs.get_metadata(&root, /*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()); + return; + } } fn enqueue_dir( - queue: &mut VecDeque<(PathBuf, usize)>, - visited_dirs: &mut HashSet, + queue: &mut VecDeque<(AbsolutePathBuf, usize)>, + visited_dirs: &mut HashSet, truncated_by_dir_limit: &mut bool, - path: PathBuf, + path: AbsolutePathBuf, depth: usize, ) { if depth > MAX_SCAN_DEPTH { @@ -400,14 +466,14 @@ fn discover_skills_under_root( SkillScope::Repo | SkillScope::User | SkillScope::Admin ); - let mut visited_dirs: HashSet = HashSet::new(); + let mut visited_dirs: HashSet = HashSet::new(); visited_dirs.insert(root.clone()); - let mut queue: VecDeque<(PathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let mut queue: VecDeque<(AbsolutePathBuf, 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_dir(&dir) { + let entries = match fs.read_directory(&dir, /*sandbox*/ None).await { Ok(entries) => entries, Err(e) => { error!("failed to read skills dir {}: {e:#}", dir.display()); @@ -415,65 +481,53 @@ fn discover_skills_under_root( } }; - for entry in entries.flatten() { - let path = match AbsolutePathBuf::from_absolute_path_checked(entry.path()) { - Ok(path) => path, - Err(err) => { - error!("failed to normalize skills entry path: {err:#}"); - continue; - } - }; - let file_name = match path.file_name().and_then(|f| f.to_str()) { - Some(name) => name, - None => continue, - }; - + for entry in entries { + let file_name = entry.file_name; if file_name.starts_with('.') { continue; } - let Ok(file_type) = entry.file_type() else { - continue; + let path = dir.join(&file_name); + let metadata = match fs.get_metadata(&path, /*sandbox*/ None).await { + Ok(metadata) => metadata, + Err(e) => { + error!("failed to stat skills path {}: {e:#}", path.display()); + continue; + } }; - if file_type.is_symlink() { + if metadata.is_symlink { if !follow_symlinks { continue; } - - // Follow the symlink to determine what it points to. - let metadata = match fs::metadata(&path) { - Ok(metadata) => metadata, - Err(e) => { + match fs.read_directory(&path, /*sandbox*/ None).await { + Ok(_) => { + let resolved_dir = canonicalize_for_skill_identity(&path); + enqueue_dir( + &mut queue, + &mut visited_dirs, + &mut truncated_by_dir_limit, + resolved_dir, + depth + 1, + ); + } + Err(err) + if matches!( + err.kind(), + io::ErrorKind::NotADirectory | io::ErrorKind::NotFound + ) => {} + Err(err) => { error!( - "failed to stat skills entry {} (symlink): {e:#}", + "failed to read skills symlink dir {}: {err:#}", path.display() ); - continue; } - }; - - if metadata.is_dir() { - let Ok(resolved_dir) = canonicalize_path(&path) else { - continue; - }; - enqueue_dir( - &mut queue, - &mut visited_dirs, - &mut truncated_by_dir_limit, - resolved_dir, - depth + 1, - ); - continue; } - continue; } - if file_type.is_dir() { - let Ok(resolved_dir) = canonicalize_path(&path) else { - continue; - }; + if metadata.is_directory { + let resolved_dir = canonicalize_for_skill_identity(&path); enqueue_dir( &mut queue, &mut visited_dirs, @@ -484,15 +538,15 @@ fn discover_skills_under_root( continue; } - if file_type.is_file() && file_name == SKILLS_FILENAME { - match parse_skill_file(&path, scope) { + if metadata.is_file && file_name == SKILLS_FILENAME { + match parse_skill_file(fs, &path, scope).await { Ok(skill) => { outcome.skills.push(skill); } Err(err) => { if scope != SkillScope::System { outcome.errors.push(SkillError { - path, + path: path.clone(), message: err.to_string(), }); } @@ -511,11 +565,15 @@ fn discover_skills_under_root( } } -fn parse_skill_file( +async fn parse_skill_file( + fs: &dyn ExecutorFileSystem, path: &AbsolutePathBuf, scope: SkillScope, ) -> Result { - let contents = fs::read_to_string(path.as_path()).map_err(SkillParseError::Read)?; + let contents = fs + .read_file_text(path, /*sandbox*/ None) + .await + .map_err(SkillParseError::Read)?; let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?; @@ -527,8 +585,8 @@ fn parse_skill_file( .as_deref() .map(sanitize_single_line) .filter(|value| !value.is_empty()) - .unwrap_or_else(|| default_skill_name(path.as_path())); - let name = namespaced_skill_name(path.as_path(), &base_name); + .unwrap_or_else(|| default_skill_name(path)); + let name = namespaced_skill_name(fs, path, &base_name).await; let description = parsed .description .as_deref() @@ -544,7 +602,7 @@ fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata(path); + } = load_skill_metadata(fs, path).await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -556,7 +614,7 @@ fn parse_skill_file( )?; } - let resolved_path = path.canonicalize().map_err(SkillParseError::Read)?; + let resolved_path = canonicalize_for_skill_identity(path); Ok(SkillMetadata { name, @@ -570,22 +628,33 @@ fn parse_skill_file( }) } -fn default_skill_name(path: &Path) -> String { +fn default_skill_name(path: &AbsolutePathBuf) -> String { path.parent() - .and_then(Path::file_name) - .and_then(|name| name.to_str()) - .map(sanitize_single_line) + .and_then(|parent| { + parent + .file_name() + .and_then(|name| name.to_str()) + .map(sanitize_single_line) + }) .filter(|value| !value.is_empty()) .unwrap_or_else(|| "skill".to_string()) } -fn namespaced_skill_name(path: &Path, base_name: &str) -> String { - plugin_namespace_for_skill_path(path) +async fn namespaced_skill_name( + fs: &dyn ExecutorFileSystem, + path: &AbsolutePathBuf, + base_name: &str, +) -> String { + plugin_namespace_for_skill_path(fs, path) + .await .map(|namespace| format!("{namespace}:{base_name}")) .unwrap_or_else(|| base_name.to_string()) } -fn load_skill_metadata(skill_path: &AbsolutePathBuf) -> LoadedSkillMetadata { +async fn load_skill_metadata( + fs: &dyn ExecutorFileSystem, + skill_path: &AbsolutePathBuf, +) -> LoadedSkillMetadata { // Fail open: optional metadata should not block loading SKILL.md. let Some(skill_dir) = skill_path.parent() else { return LoadedSkillMetadata::default(); @@ -593,11 +662,23 @@ fn load_skill_metadata(skill_path: &AbsolutePathBuf) -> LoadedSkillMetadata { let metadata_path = skill_dir .join(SKILLS_METADATA_DIR) .join(SKILLS_METADATA_FILENAME); - if !metadata_path.as_path().exists() { - return LoadedSkillMetadata::default(); + match fs.get_metadata(&metadata_path, /*sandbox*/ None).await { + Ok(metadata) if metadata.is_file => {} + Ok(_) => return LoadedSkillMetadata::default(), + Err(error) if error.kind() == io::ErrorKind::NotFound => { + return LoadedSkillMetadata::default(); + } + Err(error) => { + tracing::warn!( + "ignoring {path}: failed to stat {label}: {error}", + path = metadata_path.display(), + label = SKILLS_METADATA_FILENAME + ); + return LoadedSkillMetadata::default(); + } } - let contents = match fs::read_to_string(metadata_path.as_path()) { + let contents = match fs.read_file_text(&metadata_path, /*sandbox*/ None).await { Ok(contents) => contents, Err(error) => { tracing::warn!( @@ -863,12 +944,13 @@ fn extract_frontmatter(contents: &str) -> Option { Some(frontmatter_lines.join("\n")) } #[cfg(test)] -pub(crate) fn skill_roots_from_layer_stack( +pub(crate) async fn skill_roots_from_layer_stack( + fs: Arc, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, ) -> Vec { - skill_roots_with_home_dir(config_layer_stack, cwd, home_dir, Vec::new()) + skill_roots_with_home_dir(Some(fs), config_layer_stack, cwd, home_dir, 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 9c7561a77..a12f09f80 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,14 +4,18 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::PathExt; +use dunce::canonicalize as canonicalize_path; use pretty_assertions::assert_eq; +use std::fs; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use tempfile::TempDir; use toml::Value as TomlValue; @@ -113,14 +117,18 @@ async fn make_config_for_cwd(codex_home: &TempDir, cwd: PathBuf) -> TestConfig { } } -fn load_skills_for_test(config: &TestConfig) -> SkillLoadOutcome { +async fn load_skills_for_test(config: &TestConfig) -> SkillLoadOutcome { // Keep unit tests hermetic by never scanning the real `$HOME/.agents/skills`. - super::load_skills_from_roots(super::skill_roots_with_home_dir( - &config.config_layer_stack, - &config.cwd, - /*home_dir*/ None, - Vec::new(), - )) + super::load_skills_from_roots( + super::skill_roots_from_layer_stack( + Arc::clone(&LOCAL_FS), + &config.config_layer_stack, + &config.cwd, + /*home_dir*/ None, + ) + .await, + ) + .await } fn mark_as_git_repo(dir: &Path) { @@ -135,8 +143,8 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } -#[test] -fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() +#[tokio::test] +async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() -> anyhow::Result<()> { let tmp = tempfile::tempdir()?; @@ -167,10 +175,16 @@ fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to )?; let home_folder_abs = home_folder.abs(); - let got = skill_roots_from_layer_stack(&stack, &home_folder_abs, Some(&home_folder_abs)) - .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) - .collect::>(); + let got = skill_roots_from_layer_stack( + Arc::clone(&LOCAL_FS), + &stack, + &home_folder_abs, + Some(&home_folder_abs), + ) + .await + .into_iter() + .map(|root| (root.scope, root.path.to_path_buf())) + .collect::>(); assert_eq!( got, @@ -191,8 +205,8 @@ fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to Ok(()) } -#[test] -fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Result<()> { +#[tokio::test] +async fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Result<()> { let tmp = tempfile::tempdir()?; let home_folder = tmp.path().join("home"); @@ -227,10 +241,16 @@ fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Re let home_folder_abs = home_folder.abs(); let project_root_abs = project_root.abs(); - let got = skill_roots_from_layer_stack(&stack, &project_root_abs, Some(&home_folder_abs)) - .into_iter() - .map(|root| (root.scope, root.path.to_path_buf())) - .collect::>(); + let got = skill_roots_from_layer_stack( + Arc::clone(&LOCAL_FS), + &stack, + &project_root_abs, + Some(&home_folder_abs), + ) + .await + .into_iter() + .map(|root| (root.scope, root.path.to_path_buf())) + .collect::>(); assert_eq!( got, @@ -251,8 +271,8 @@ fn skill_roots_from_layer_stack_includes_disabled_project_layers() -> anyhow::Re Ok(()) } -#[test] -fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { +#[tokio::test] +async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { let tmp = tempfile::tempdir()?; let home_folder = tmp.path().join("home"); @@ -278,11 +298,14 @@ fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()> { ); let home_folder_abs = home_folder.abs(); - let outcome = load_skills_from_roots(skill_roots_from_layer_stack( + let roots = skill_roots_from_layer_stack( + Arc::clone(&LOCAL_FS), &stack, &home_folder_abs, Some(&home_folder_abs), - )); + ) + .await; + let outcome = load_skills_from_roots(roots).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -396,7 +419,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -474,7 +497,7 @@ interface: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -523,7 +546,7 @@ policy: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -555,7 +578,7 @@ policy: {} ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -594,7 +617,7 @@ policy: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -632,7 +655,7 @@ async fn accepts_icon_paths_under_assets_dir() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -679,7 +702,7 @@ async fn ignores_invalid_brand_color() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -725,7 +748,7 @@ async fn ignores_default_prompt_over_max_length() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -773,7 +796,7 @@ async fn drops_interface_when_icons_are_invalid() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -817,7 +840,7 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -852,7 +875,7 @@ async fn ignores_symlinked_skill_file_for_user_scope() { symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -876,7 +899,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -898,9 +921,9 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() { ); } -#[test] +#[tokio::test] #[cfg(unix)] -fn loads_skills_via_symlinked_subdir_for_admin_scope() { +async fn loads_skills_via_symlinked_subdir_for_admin_scope() { let admin_root = tempfile::tempdir().expect("tempdir"); let shared = tempfile::tempdir().expect("tempdir"); @@ -912,7 +935,9 @@ fn loads_skills_via_symlinked_subdir_for_admin_scope() { let outcome = load_skills_from_roots([SkillRoot { path: admin_root.path().abs(), scope: SkillScope::Admin, - }]); + file_system: Arc::clone(&LOCAL_FS), + }]) + .await; assert!( outcome.errors.is_empty(), @@ -951,7 +976,7 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { symlink_dir(shared.path(), &repo_skills_root.join("shared")); let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -988,7 +1013,9 @@ async fn system_scope_ignores_symlinked_subdir() { let outcome = load_skills_from_roots([SkillRoot { path: system_root.abs(), scope: SkillScope::System, - }]); + file_system: Arc::clone(&LOCAL_FS), + }]) + .await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1018,7 +1045,9 @@ async fn respects_max_scan_depth_for_user_scope() { let outcome = load_skills_from_roots([SkillRoot { path: skills_root.abs(), scope: SkillScope::User, - }]); + file_system: Arc::clone(&LOCAL_FS), + }]) + .await; assert!( outcome.errors.is_empty(), @@ -1046,7 +1075,7 @@ async fn loads_valid_skill() { let skill_path = write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1077,7 +1106,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -1118,7 +1147,9 @@ async fn namespaces_plugin_skills_using_plugin_name() { let outcome = load_skills_from_roots([SkillRoot { path: plugin_root.join("skills").abs(), scope: SkillScope::User, - }]); + file_system: Arc::clone(&LOCAL_FS), + }]) + .await; assert!( outcome.errors.is_empty(), @@ -1150,7 +1181,7 @@ async fn loads_short_description_from_metadata() { fs::write(&skill_path, contents).unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1183,7 +1214,7 @@ async fn enforces_short_description_length_limits() { fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( @@ -1212,7 +1243,7 @@ async fn skips_hidden_and_invalid() { fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( @@ -1230,7 +1261,7 @@ async fn enforces_length_limits() { write_skill(&codex_home, "max-len", "max-len", &max_desc); let cfg = make_config(&codex_home).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1240,7 +1271,7 @@ async fn enforces_length_limits() { let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1); write_skill(&codex_home, "too-long", "too-long", &too_long_desc); - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert_eq!(outcome.skills.len(), 1); assert_eq!(outcome.errors.len(), 1); assert!( @@ -1262,7 +1293,7 @@ async fn loads_skills_from_repo_root() { let skill_path = write_skill_at(&skills_root, "repo", "repo-skill", "from repo"); let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1297,7 +1328,7 @@ async fn loads_skills_from_agents_dir_without_codex_dir() { ); let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1349,7 +1380,7 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() { let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1399,7 +1430,7 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() { let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1430,12 +1461,15 @@ async fn deduplicates_by_path_preferring_first_root() { SkillRoot { path: root.path().abs(), scope: SkillScope::Repo, + file_system: Arc::clone(&LOCAL_FS), }, SkillRoot { path: root.path().abs(), scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), }, - ]); + ]) + .await; assert!( outcome.errors.is_empty(), @@ -1476,7 +1510,7 @@ async fn keeps_duplicate_names_from_repo_and_user() { let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1539,7 +1573,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() { ); let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), @@ -1601,7 +1635,7 @@ async fn repo_skills_search_does_not_escape_repo_root() { let cfg = make_config_for_cwd(&codex_home, repo_dir).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1630,7 +1664,7 @@ async fn loads_skills_when_cwd_is_file_in_repo() { let cfg = make_config_for_cwd(&codex_home, file_path).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1670,7 +1704,7 @@ async fn non_git_repo_skills_search_does_not_walk_parents() { let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1688,7 +1722,7 @@ async fn loads_skills_from_system_cache_when_present() { let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - let outcome = load_skills_for_test(&cfg); + let outcome = load_skills_for_test(&cfg).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1714,10 +1748,16 @@ 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(&cfg.config_layer_stack, &cfg.cwd, Vec::new()) - .into_iter() - .map(|root| root.scope) - .collect(); + let scopes: Vec = super::skill_roots( + Some(Arc::clone(&LOCAL_FS)), + &cfg.config_layer_stack, + &cfg.cwd, + 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 bfefc5c7d..b7b7a4b64 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -4,6 +4,7 @@ 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; @@ -85,15 +86,19 @@ 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 fn skills_for_config(&self, input: &SkillsLoadInput) -> SkillLoadOutcome { - let roots = self.skill_roots_for_config(input); + pub async fn skills_for_config( + &self, + input: &SkillsLoadInput, + fs: Option>, + ) -> SkillLoadOutcome { + let roots = self.skill_roots_for_config(input, fs).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) { return outcome; } - let outcome = self.build_skill_outcome(roots, &skill_config_rules); + let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; let mut cache = self .cache_by_config .write() @@ -102,12 +107,18 @@ impl SkillsManager { outcome } - pub fn skill_roots_for_config(&self, input: &SkillsLoadInput) -> Vec { + pub async fn skill_roots_for_config( + &self, + input: &SkillsLoadInput, + fs: Option>, + ) -> Vec { let mut roots = skill_roots( + fs, &input.config_layer_stack, &input.cwd, input.effective_skill_roots.clone(), - ); + ) + .await; if !input.bundled_skills_enabled { roots.retain(|root| root.scope != SkillScope::System); } @@ -118,12 +129,9 @@ impl SkillsManager { &self, input: &SkillsLoadInput, force_reload: bool, + fs: Option>, ) -> SkillLoadOutcome { - if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) { - return outcome; - } - - self.skills_for_cwd_with_extra_user_roots(input, force_reload, &[]) + self.skills_for_cwd_with_extra_user_roots(input, force_reload, &[], fs) .await } @@ -132,46 +140,56 @@ impl SkillsManager { input: &SkillsLoadInput, force_reload: bool, extra_user_roots: &[AbsolutePathBuf], + fs: Option>, ) -> SkillLoadOutcome { - if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) { + let use_cwd_cache = fs.is_some(); + if use_cwd_cache + && !force_reload + && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) + { return outcome; } - let normalized_extra_user_roots = normalize_extra_user_roots(extra_user_roots); let mut roots = skill_roots( + fs.clone(), &input.config_layer_stack, &input.cwd, input.effective_skill_roots.clone(), - ); + ) + .await; if !bundled_skills_enabled_from_stack(&input.config_layer_stack) { roots.retain(|root| root.scope != SkillScope::System); } - roots.extend( - normalized_extra_user_roots - .iter() - .cloned() - .map(|path| SkillRoot { - path, - scope: SkillScope::User, - }), - ); + if let Some(fs) = fs { + roots.extend( + normalize_extra_user_roots(extra_user_roots) + .into_iter() + .map(|path| SkillRoot { + path, + scope: SkillScope::User, + file_system: Arc::clone(&fs), + }), + ); + } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); - let outcome = self.build_skill_outcome(roots, &skill_config_rules); - let mut cache = self - .cache_by_cwd - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(input.cwd.clone(), outcome.clone()); + 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()); + } outcome } - fn build_skill_outcome( + async fn build_skill_outcome( &self, roots: Vec, skill_config_rules: &SkillConfigRules, ) -> SkillLoadOutcome { let outcome = crate::filter_skill_load_outcome_for_product( - load_skills_from_roots(roots), + load_skills_from_roots(roots).await, self.restriction_product, ); let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/manager_tests.rs index 2ba1463a3..73800a51d 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_FS; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::PathExt; @@ -15,6 +16,7 @@ use pretty_assertions::assert_eq; use std::collections::HashSet; use std::fs; use std::path::PathBuf; +use std::sync::Arc; use tempfile::TempDir; fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { @@ -136,22 +138,21 @@ enabled = {enabled} ) } -fn skills_for_config_with_stack( +async fn skills_for_config_with_stack( skills_manager: &SkillsManager, cwd: &TempDir, config_layer_stack: &ConfigLayerStack, - effective_skill_roots: &[PathBuf], + effective_skill_roots: &[AbsolutePathBuf], ) -> SkillLoadOutcome { let skills_input = SkillsLoadInput::new( cwd.path().abs(), - effective_skill_roots - .iter() - .map(codex_utils_absolute_path::test_support::PathBufExt::abs) - .collect(), + effective_skill_roots.to_vec(), config_layer_stack.clone(), bundled_skills_enabled_from_stack(config_layer_stack), ); - skills_manager.skills_for_config(&skills_input) + skills_manager + .skills_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS))) + .await } #[test] @@ -184,7 +185,8 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { ); write_user_skill(&codex_home, "a", "skill-a", "from a"); - let outcome1 = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]); + let outcome1 = + skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; assert!( outcome1.skills.iter().any(|s| s.name == "skill-a"), "expected skill-a to be discovered" @@ -193,7 +195,8 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { // Write a new skill after the first call; the second call should reuse the config-aware cache // entry because the effective skill config is unchanged. write_user_skill(&codex_home, "b", "skill-b", "from b"); - let outcome2 = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]); + let outcome2 = + skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; assert_eq!(outcome2.errors, outcome1.errors); assert_eq!(outcome2.skills, outcome1.skills); } @@ -218,7 +221,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() { .parent() .and_then(std::path::Path::parent) .expect("plugin skill should live under a skills root") - .to_path_buf(); + .abs(); let skills_manager = SkillsManager::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, @@ -229,7 +232,8 @@ async fn skills_for_config_disables_plugin_skills_by_name() { &cwd, &config_layer_stack, &[plugin_skill_root], - ); + ) + .await; let skill = outcome .skills .iter() @@ -259,7 +263,7 @@ async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]); + let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; write_user_skill(&extra_root, "x", "extra-skill", "from extra root"); let extra_root_path = extra_root.path().abs(); @@ -274,6 +278,7 @@ async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { &base_input, /*force_reload*/ true, std::slice::from_ref(&extra_root_path), + Some(Arc::clone(&LOCAL_FS)), ) .await; assert!( @@ -298,12 +303,150 @@ async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { bundled_skills_enabled_from_stack(&config_layer_stack), ); let outcome_without_extra = skills_manager - .skills_for_cwd(&base_input, /*force_reload*/ false) + .skills_for_cwd( + &base_input, + /*force_reload*/ false, + Some(Arc::clone(&LOCAL_FS)), + ) .await; assert_eq!(outcome_without_extra.skills, outcome_with_extra.skills); assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors); } +#[tokio::test] +async fn skills_for_cwd_loads_repo_user_and_extra_roots_with_local_fs() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root = 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"); + write_user_skill(&extra_root, "extra", "extra-skill", "from extra 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_with_extra_user_roots( + &skills_input, + /*force_reload*/ true, + &[extra_root.path().abs()], + Some(Arc::clone(&LOCAL_FS)), + ) + .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")); + assert!(loaded_names.contains("extra-skill")); +} + +#[tokio::test] +async fn skills_for_cwd_without_fs_skips_repo_and_extra_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root = 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"); + write_user_skill(&extra_root, "extra", "extra-skill", "from extra 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_with_extra_user_roots( + &skills_input, + /*force_reload*/ true, + &[extra_root.path().abs()], + /*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")); + assert!(!loaded_names.contains("extra-skill")); +} + #[tokio::test] async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -330,7 +473,8 @@ async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { ) .expect("rewrite bundled skill"); - let outcome = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]); + let outcome = + skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; assert!( outcome .skills @@ -356,7 +500,7 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]); + let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a"); write_user_skill(&extra_root_b, "x", "extra-skill-b", "from extra root b"); @@ -373,6 +517,7 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { &base_input, /*force_reload*/ true, std::slice::from_ref(&extra_root_a_path), + Some(Arc::clone(&LOCAL_FS)), ) .await; assert!( @@ -394,6 +539,7 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { &base_input, /*force_reload*/ false, std::slice::from_ref(&extra_root_b_path), + Some(Arc::clone(&LOCAL_FS)), ) .await; assert!( @@ -414,6 +560,7 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { &base_input, /*force_reload*/ true, std::slice::from_ref(&extra_root_b_path), + Some(Arc::clone(&LOCAL_FS)), ) .await; assert!( @@ -600,7 +747,11 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() ); let parent_outcome = skills_manager - .skills_for_cwd(&parent_input, /*force_reload*/ true) + .skills_for_cwd( + &parent_input, + /*force_reload*/ true, + Some(Arc::clone(&LOCAL_FS)), + ) .await; let parent_skill = parent_outcome .skills @@ -609,7 +760,8 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() .expect("demo skill should be discovered"); assert_eq!(parent_outcome.is_skill_enabled(parent_skill), false); - let child_outcome = skills_for_config_with_stack(&skills_manager, &cwd, &child_stack, &[]); + let child_outcome = + skills_for_config_with_stack(&skills_manager, &cwd, &child_stack, &[]).await; let child_skill = child_outcome .skills .iter() diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index a3523d2f4..eb9a6f132 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -1,7 +1,9 @@ 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; @@ -87,6 +89,7 @@ pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, pub disabled_paths: HashSet, + pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, pub(crate) implicit_skills_by_scripts_dir: Arc>, pub(crate) implicit_skills_by_doc_path: Arc>, } @@ -113,6 +116,49 @@ 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( @@ -122,6 +168,14 @@ 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 + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect(); + outcome + .file_systems_by_skill_path + .retain_paths(&retained_paths); outcome.implicit_skills_by_scripts_dir = Arc::new( outcome .implicit_skills_by_scripts_dir diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index 2bb10744a..69aa10adb 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -658,7 +658,12 @@ enabled = false let plugin_outcome = plugins_manager.plugins_for_config(&config).await; let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = skills_load_input_from_config(&config, effective_skill_roots); - let outcome = skills_manager.skills_for_config(&skills_input); + let outcome = skills_manager + .skills_for_config( + &skills_input, + Some(Arc::clone(&codex_exec_server::LOCAL_FS)), + ) + .await; let skill = outcome .skills .iter() diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f16f76dad..95e932b1d 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -496,10 +496,17 @@ impl Codex { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); + let environment = environment_manager + .current() + .await + .map_err(|err| CodexErr::Fatal(format!("failed to create environment: {err}")))?; + let fs = environment + .as_ref() + .map(|environment| environment.get_filesystem()); let plugin_outcome = plugins_manager.plugins_for_config(&config).await; let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = skills_load_input_from_config(&config, effective_skill_roots); - let loaded_skills = skills_manager.skills_for_config(&skills_input); + let loaded_skills = skills_manager.skills_for_config(&skills_input, fs).await; for err in &loaded_skills.errors { error!( @@ -544,10 +551,6 @@ impl Codex { config.startup_warnings.push(message); } - let environment = environment_manager - .current() - .await - .map_err(|err| CodexErr::Fatal(format!("failed to create environment: {err}")))?; let user_instructions = get_user_instructions(&config, environment.as_deref()).await; let exec_policy = if crate::guardian::is_guardian_reviewer_source(&session_source) { @@ -2670,10 +2673,16 @@ impl Session { .await; let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots); + let fs = self + .services + .environment + .as_ref() + .map(|environment| environment.get_filesystem()); let skills_outcome = Arc::new( self.services .skills_manager - .skills_for_config(&skills_input), + .skills_for_config(&skills_input, fs) + .await, ); let mut turn_context: TurnContext = Self::make_turn_context( self.conversation_id, @@ -5398,6 +5407,11 @@ mod handlers { let skills_manager = &sess.services.skills_manager; let plugins_manager = &sess.services.plugins_manager; + let fs = sess + .services + .environment + .as_ref() + .map(|environment| environment.get_filesystem()); let config = sess.get_config().await; let codex_home = sess.codex_home().await; let mut skills = Vec::new(); @@ -5454,7 +5468,7 @@ mod handlers { config.bundled_skills_enabled(), ); let outcome = skills_manager - .skills_for_cwd(&skills_input, force_reload) + .skills_for_cwd(&skills_input, force_reload, fs.clone()) .await; let errors = super::errors_to_info(&outcome.errors); let skills_metadata = super::skills_to_info(&outcome.skills, &outcome.disabled_paths); @@ -6232,6 +6246,7 @@ pub(crate) async fn run_turn( warnings: skill_warnings, } = build_skill_injections( &mentioned_skills, + skills_outcome, Some(&session_telemetry), &sess.services.analytics_events_client, tracking.clone(), diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index bf213a645..6de8909e6 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2450,12 +2450,19 @@ async fn new_default_turn_uses_config_aware_skills_for_role_overrides() { ) .expect("write skill"); + let skill_fs = session + .services + .environment + .as_ref() + .map(|environment| environment.get_filesystem()) + .unwrap_or_else(|| std::sync::Arc::clone(&codex_exec_server::LOCAL_FS)); let parent_outcome = session .services .skills_manager .skills_for_cwd( &crate::skills_load_input_from_config(&parent_config, Vec::new()), /*force_reload*/ true, + Some(Arc::clone(&skill_fs)), ) .await; let parent_skill = parent_outcome @@ -2845,7 +2852,13 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skills_outcome = Arc::new(services.skills_manager.skills_for_config(&skills_input)); + let skill_fs = environment.get_filesystem(); + let skills_outcome = Arc::new( + services + .skills_manager + .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .await, + ); let turn_context = Session::make_turn_context( conversation_id, Some(Arc::clone(&auth_manager)), @@ -3691,7 +3704,13 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); - let skills_outcome = Arc::new(services.skills_manager.skills_for_config(&skills_input)); + let skill_fs = environment.get_filesystem(); + let skills_outcome = Arc::new( + services + .skills_manager + .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .await, + ); let turn_context = Arc::new(Session::make_turn_context( conversation_id, Some(Arc::clone(&auth_manager)), diff --git a/codex-rs/core/src/file_watcher.rs b/codex-rs/core/src/file_watcher.rs index f8f0e4b11..bfa01f8c9 100644 --- a/codex-rs/core/src/file_watcher.rs +++ b/codex-rs/core/src/file_watcher.rs @@ -252,6 +252,16 @@ pub struct WatchRegistration { watched_paths: Vec, } +impl Default for WatchRegistration { + fn default() -> Self { + Self { + file_watcher: std::sync::Weak::new(), + subscriber_id: 0, + watched_paths: Vec::new(), + } + } +} + impl Drop for WatchRegistration { fn drop(&mut self) { if let Some(file_watcher) = self.file_watcher.upgrade() { diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index 4f6090fd7..a735180ab 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -46,6 +46,7 @@ use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::MergeStrategy; use codex_config::types::McpServerConfig; use codex_config::types::PluginConfig; +use codex_exec_server::LOCAL_FS; use codex_features::Feature; use codex_login::AuthManager; use codex_login::CodexAuth; @@ -1031,7 +1032,8 @@ impl PluginsManager { manifest_paths, self.restriction_product, &skill_config_rules, - ); + ) + .await; let apps = load_apps_from_paths( source_path.as_path(), plugin_app_config_paths(source_path.as_path(), manifest_paths), @@ -1751,7 +1753,8 @@ async fn load_plugin( manifest_paths, restriction_product, skill_config_rules, - ); + ) + .await; let has_enabled_skills = resolved_skills.has_enabled_skills(); loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths; loaded_plugin.has_enabled_skills = has_enabled_skills; @@ -1795,20 +1798,21 @@ impl ResolvedPluginSkills { } } -fn load_plugin_skills( +async fn load_plugin_skills( plugin_root: &AbsolutePathBuf, manifest_paths: &PluginManifestPaths, restriction_product: Option, skill_config_rules: &SkillConfigRules, ) -> ResolvedPluginSkills { - let outcome = load_skills_from_roots( - plugin_skill_roots(plugin_root, manifest_paths) - .into_iter() - .map(|path| SkillRoot { - path, - scope: SkillScope::User, - }), - ); + let roots = plugin_skill_roots(plugin_root, manifest_paths) + .into_iter() + .map(|path| SkillRoot { + path, + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + }) + .collect::>(); + let outcome = load_skills_from_roots(roots).await; let had_errors = !outcome.errors.is_empty(); let skills = outcome .skills diff --git a/codex-rs/core/src/skills_watcher.rs b/codex-rs/core/src/skills_watcher.rs index dd7d21cfc..9473282b1 100644 --- a/codex-rs/core/src/skills_watcher.rs +++ b/codex-rs/core/src/skills_watcher.rs @@ -59,12 +59,14 @@ impl SkillsWatcher { config: &Config, skills_manager: &SkillsManager, plugins_manager: &PluginsManager, + fs: Option>, ) -> WatchRegistration { let plugin_outcome = plugins_manager.plugins_for_config(config).await; let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = skills_load_input_from_config(config, effective_skill_roots); let roots = skills_manager - .skill_roots_for_config(&skills_input) + .skill_roots_for_config(&skills_input, fs) + .await .into_iter() .map(|root| WatchPath { path: root.path.into_path_buf(), diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 06aa1bc7b..b8ad75b49 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -365,6 +365,10 @@ impl ThreadManager { self.state.mcp_manager.clone() } + pub fn environment_manager(&self) -> Arc { + self.state.environment_manager.clone() + } + pub fn get_models_manager(&self) -> Arc { self.state.models_manager.clone() } @@ -903,14 +907,24 @@ impl ThreadManagerState { parent_trace: Option, user_shell_override: Option, ) -> CodexResult { - let watch_registration = self - .skills_watcher - .register_config( - &config, - self.skills_manager.as_ref(), - self.plugins_manager.as_ref(), - ) - .await; + let environment = self + .environment_manager + .current() + .await + .map_err(|err| CodexErr::Fatal(format!("failed to create environment: {err}")))?; + let watch_registration = match environment.as_ref() { + Some(environment) if !environment.is_remote() => { + self.skills_watcher + .register_config( + &config, + self.skills_manager.as_ref(), + self.plugins_manager.as_ref(), + Some(environment.get_filesystem()), + ) + .await + } + Some(_) | None => crate::file_watcher::WatchRegistration::default(), + }; let CodexSpawnOk { codex, thread_id, .. } = Codex::spawn(CodexSpawnArgs { diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 388618b56..1b2ac7164 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -2,10 +2,19 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; +use codex_core::ThreadManager; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::EnvironmentManager; +use codex_exec_server::ExecutorFileSystem; +use codex_login::CodexAuth; +use codex_models_manager::collaboration_mode_presets::CollaborationModesConfig; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Op; use codex_protocol::protocol::SandboxPolicy; +use codex_protocol::protocol::SessionSource; use codex_protocol::user_input::UserInput; +use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::load_default_config_for_test; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; @@ -14,16 +23,39 @@ use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; use std::fs; use std::path::Path; +use std::sync::Arc; +use tempfile::TempDir; -fn write_skill(home: &Path, name: &str, description: &str, body: &str) -> std::path::PathBuf { - let skill_dir = home.join("skills").join(name); - fs::create_dir_all(&skill_dir).unwrap(); +async fn write_repo_skill( + cwd: AbsolutePathBuf, + fs: Arc, + name: &str, + description: &str, + body: &str, +) -> Result<()> { + let skill_dir = cwd.join(".agents").join("skills").join(name); + fs.create_directory( + &skill_dir, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, + ) + .await?; let contents = format!("---\nname: {name}\ndescription: {description}\n---\n\n{body}\n"); let path = skill_dir.join("SKILL.md"); - fs::write(&path, contents).unwrap(); - path + fs.write_file(&path, contents.into_bytes(), /*sandbox*/ None) + .await?; + Ok(()) +} + +fn write_home_skill(codex_home: &Path, dir: &str, name: &str, description: &str) -> Result<()> { + let skill_dir = codex_home.join("skills").join(dir); + fs::create_dir_all(&skill_dir)?; + let contents = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skill_dir.join("SKILL.md"), contents)?; + Ok(()) } fn system_skill_md_path(home: impl AsRef, name: &str) -> std::path::PathBuf { @@ -40,13 +72,18 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let server = start_mock_server().await; let skill_body = "skill body"; - let mut builder = test_codex().with_pre_build_hook(|home| { - write_skill(home, "demo", "demo skill", skill_body); + let mut builder = test_codex().with_workspace_setup(move |cwd, fs| async move { + write_repo_skill(cwd, fs, "demo", "demo skill", skill_body).await }); - let test = builder.build(&server).await?; + let test = builder.build_remote_aware(&server).await?; - let skill_path = test.codex_home_path().join("skills/demo/SKILL.md"); - let skill_path = std::fs::canonicalize(skill_path)?; + let skill_path = test + .config + .cwd + .join(".agents/skills/demo/SKILL.md") + .canonicalize() + .unwrap_or_else(|_| test.config.cwd.join(".agents/skills/demo/SKILL.md")) + .to_path_buf(); let mock = mount_sse_once( &server, @@ -72,7 +109,7 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { }, ], final_output_json_schema: None, - cwd: test.cwd_path().to_path_buf(), + cwd: test.config.cwd.to_path_buf(), approval_policy: AskForApproval::Never, approvals_reviewer: None, sandbox_policy: SandboxPolicy::DangerFullAccess, @@ -106,6 +143,138 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_skills_includes_repo_and_home_skills_remote_aware() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex() + .with_pre_build_hook(|home| { + write_home_skill(home, "home-demo", "home-demo", "from home") + .expect("write home skill"); + }) + .with_workspace_setup(|cwd, fs| async move { + write_repo_skill(cwd, fs, "repo-demo", "from repo", "# Body").await + }); + let test = builder.build_remote_aware(&server).await?; + + test.codex + .submit(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }) + .await?; + let response = + core_test_support::wait_for_event_match(test.codex.as_ref(), |event| match event { + codex_protocol::protocol::EventMsg::ListSkillsResponse(response) => { + Some(response.clone()) + } + _ => None, + }) + .await; + + let cwd = test.config.cwd.as_path(); + let skills = response + .skills + .iter() + .find(|entry| entry.cwd.as_path() == cwd) + .map(|entry| entry.skills.clone()) + .unwrap_or_default(); + + let repo_skill = skills + .iter() + .find(|skill| skill.name == "repo-demo") + .expect("expected repo skill"); + assert_eq!(repo_skill.scope, codex_protocol::protocol::SkillScope::Repo); + let repo_path = repo_skill.path.to_string_lossy().replace('\\', "/"); + assert!( + repo_path.ends_with("/.agents/skills/repo-demo/SKILL.md"), + "unexpected repo skill path: {repo_path}" + ); + + let home_skill = skills + .iter() + .find(|skill| skill.name == "home-demo") + .expect("expected home skill"); + assert_eq!(home_skill.scope, codex_protocol::protocol::SkillScope::User); + let home_path = home_skill.path.to_string_lossy().replace('\\', "/"); + assert!( + home_path.ends_with("/skills/home-demo/SKILL.md"), + "unexpected home skill path: {home_path}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_skills_skips_cwd_roots_when_environment_disabled() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + write_home_skill( + codex_home.path(), + "home-disabled", + "home-disabled", + "from home", + )?; + let repo_skill_dir = cwd + .path() + .join(".agents") + .join("skills") + .join("repo-disabled"); + fs::create_dir_all(&repo_skill_dir)?; + fs::write( + repo_skill_dir.join("SKILL.md"), + "---\nname: repo-disabled\ndescription: from repo\n---\n\n# Body\n", + )?; + let mut config = load_default_config_for_test(&codex_home).await; + config.cwd = AbsolutePathBuf::from_absolute_path_checked(cwd.path())?; + + let thread_manager = ThreadManager::new( + &config, + codex_core::test_support::auth_manager_from_auth(CodexAuth::from_api_key("dummy")), + SessionSource::Exec, + CollaborationModesConfig::default(), + Arc::new(EnvironmentManager::new(Some("none".to_string()))), + /*analytics_events_client*/ None, + ); + let new_thread = thread_manager.start_thread(config.clone()).await?; + let cwd = config.cwd.to_path_buf(); + + new_thread + .thread + .submit(Op::ListSkills { + cwds: vec![cwd.clone()], + force_reload: true, + }) + .await?; + let response = + core_test_support::wait_for_event_match(new_thread.thread.as_ref(), |event| match event { + codex_protocol::protocol::EventMsg::ListSkillsResponse(response) => { + Some(response.clone()) + } + _ => None, + }) + .await; + + assert_eq!(response.skills.len(), 1); + assert_eq!(response.skills[0].cwd, cwd); + assert_eq!(response.skills[0].errors.len(), 0); + assert!( + response.skills[0] + .skills + .iter() + .any(|skill| skill.name == "home-disabled") + ); + assert!( + response.skills[0] + .skills + .iter() + .all(|skill| skill.name != "repo-disabled") + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn skill_load_errors_surface_in_session_configured() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/utils/plugins/Cargo.toml b/codex-rs/utils/plugins/Cargo.toml index 69b75a653..983f4526e 100644 --- a/codex-rs/utils/plugins/Cargo.toml +++ b/codex-rs/utils/plugins/Cargo.toml @@ -14,9 +14,12 @@ path = "src/lib.rs" workspace = true [dependencies] +codex-exec-server = { workspace = true } codex-login = { workspace = true } +codex-utils-absolute-path = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } [dev-dependencies] tempfile = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt"] } diff --git a/codex-rs/utils/plugins/src/plugin_namespace.rs b/codex-rs/utils/plugins/src/plugin_namespace.rs index 6688ae046..adb709c7f 100644 --- a/codex-rs/utils/plugins/src/plugin_namespace.rs +++ b/codex-rs/utils/plugins/src/plugin_namespace.rs @@ -1,7 +1,7 @@ //! Resolve plugin namespace from skill file paths by walking ancestors for `plugin.json`. -use std::fs; -use std::path::Path; +use codex_exec_server::ExecutorFileSystem; +use codex_utils_absolute_path::AbsolutePathBuf; /// Relative path from a plugin root to its manifest file. pub const PLUGIN_MANIFEST_PATH: &str = ".codex-plugin/plugin.json"; @@ -13,12 +13,19 @@ struct RawPluginManifestName { name: String, } -fn plugin_manifest_name(plugin_root: &Path) -> Option { +async fn plugin_manifest_name( + fs: &dyn ExecutorFileSystem, + plugin_root: &AbsolutePathBuf, +) -> Option { let manifest_path = plugin_root.join(PLUGIN_MANIFEST_PATH); - if !manifest_path.is_file() { - return None; + match fs.get_metadata(&manifest_path, /*sandbox*/ None).await { + Ok(metadata) if metadata.is_file => {} + Ok(_) | Err(_) => return None, } - let contents = fs::read_to_string(&manifest_path).ok()?; + let contents = fs + .read_file_text(&manifest_path, /*sandbox*/ None) + .await + .ok()?; let RawPluginManifestName { name: raw_name } = serde_json::from_str(&contents).ok()?; Some( plugin_root @@ -32,9 +39,12 @@ fn plugin_manifest_name(plugin_root: &Path) -> Option { /// Returns the plugin manifest `name` for the nearest ancestor of `path` that contains a valid /// plugin manifest (same `name` rules as full manifest loading in codex-core). -pub fn plugin_namespace_for_skill_path(path: &Path) -> Option { +pub async fn plugin_namespace_for_skill_path( + fs: &dyn ExecutorFileSystem, + path: &AbsolutePathBuf, +) -> Option { for ancestor in path.ancestors() { - if let Some(name) = plugin_manifest_name(ancestor) { + if let Some(name) = plugin_manifest_name(fs, &ancestor).await { return Some(name); } } @@ -44,11 +54,13 @@ pub fn plugin_namespace_for_skill_path(path: &Path) -> Option { #[cfg(test)] mod tests { use super::plugin_namespace_for_skill_path; + use codex_exec_server::LOCAL_FS; + use codex_utils_absolute_path::test_support::PathBufExt; use std::fs; use tempfile::tempdir; - #[test] - fn uses_manifest_name() { + #[tokio::test] + async fn uses_manifest_name() { let tmp = tempdir().expect("tempdir"); let plugin_root = tmp.path().join("plugins/sample"); let skill_path = plugin_root.join("skills/search/SKILL.md"); @@ -63,7 +75,7 @@ mod tests { fs::write(&skill_path, "---\ndescription: search\n---\n").expect("write skill"); assert_eq!( - plugin_namespace_for_skill_path(&skill_path), + plugin_namespace_for_skill_path(LOCAL_FS.as_ref(), &skill_path.abs()).await, Some("sample".to_string()) ); }