From 033d1d0741d697436d4d5527aaac8893c28b424c Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 29 May 2026 14:06:44 -0700 Subject: [PATCH] core-skills: await bound path operations --- codex-rs/core-skills/src/invocation_utils.rs | 37 ++++++------ .../core-skills/src/invocation_utils_tests.rs | 59 +++++++++++++++---- codex-rs/core-skills/src/loader.rs | 33 ++++++++--- codex-rs/core-skills/src/manager.rs | 6 +- codex-rs/core-skills/src/model.rs | 25 +++++--- codex-rs/core/src/skills.rs | 4 +- 6 files changed, 114 insertions(+), 50 deletions(-) diff --git a/codex-rs/core-skills/src/invocation_utils.rs b/codex-rs/core-skills/src/invocation_utils.rs index 621631db74..79363818dd 100644 --- a/codex-rs/core-skills/src/invocation_utils.rs +++ b/codex-rs/core-skills/src/invocation_utils.rs @@ -6,7 +6,7 @@ use crate::SkillMetadata; use codex_exec_server::EnvironmentPathRef; use codex_utils_absolute_path::AbsolutePathBuf; -pub(crate) fn build_implicit_skill_path_indexes( +pub(crate) async fn build_implicit_skill_path_indexes( skills: Vec, ) -> ( HashMap, @@ -15,12 +15,13 @@ pub(crate) fn build_implicit_skill_path_indexes( let mut by_scripts_dir = HashMap::new(); let mut by_skill_doc_path = HashMap::new(); for skill in skills { - let skill_doc_path = canonicalize_if_exists(&skill.source_path); + let skill_doc_path = canonicalize_if_exists(&skill.source_path).await; by_skill_doc_path.insert(skill_doc_path, skill.clone()); - if let Some(skill_dir) = skill.path_to_skills_md.parent() { - let scripts_dir = - canonicalize_if_exists(&skill.source_path.with_path(skill_dir.join("scripts"))); + if let Ok(Some(skill_dir)) = skill.source_path.parent().await + && let Ok(scripts_dir) = skill_dir.join(Path::new("scripts")).await + { + let scripts_dir = canonicalize_if_exists(&scripts_dir).await; by_scripts_dir.insert(scripts_dir, skill); } } @@ -28,19 +29,19 @@ pub(crate) fn build_implicit_skill_path_indexes( (by_scripts_dir, by_skill_doc_path) } -pub fn detect_implicit_skill_invocation_for_command( +pub async fn detect_implicit_skill_invocation_for_command( outcome: &SkillLoadOutcome, command: &str, workdir: &EnvironmentPathRef, ) -> Option { - let workdir = canonicalize_if_exists(workdir); + let workdir = canonicalize_if_exists(workdir).await; let tokens = tokenize_command(command); - if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir) { + if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir).await { return Some(candidate); } - detect_skill_doc_read(outcome, tokens.as_slice(), &workdir) + detect_skill_doc_read(outcome, tokens.as_slice(), &workdir).await } fn tokenize_command(command: &str) -> Vec { @@ -80,14 +81,14 @@ fn script_run_token(tokens: &[String]) -> Option<&str> { None } -fn detect_skill_script_run( +async fn detect_skill_script_run( outcome: &SkillLoadOutcome, tokens: &[String], workdir: &EnvironmentPathRef, ) -> Option { let script_token = script_run_token(tokens)?; let script_path = Path::new(script_token); - let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path)?); + let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path).await?).await; for path in script_path.path().ancestors() { let Ok(path) = AbsolutePathBuf::from_absolute_path(path) else { @@ -104,7 +105,7 @@ fn detect_skill_script_run( None } -fn detect_skill_doc_read( +async fn detect_skill_doc_read( outcome: &SkillLoadOutcome, tokens: &[String], workdir: &EnvironmentPathRef, @@ -118,7 +119,7 @@ fn detect_skill_doc_read( continue; } let path = Path::new(token); - let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path)?); + let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path).await?).await; if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) { return Some(candidate.clone()); } @@ -144,18 +145,20 @@ fn command_basename(command: &str) -> String { .to_string() } -fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option { +async fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option { if path.is_absolute() { AbsolutePathBuf::from_absolute_path(path) .ok() .map(|path| workdir.with_path(path)) } else { - workdir.join_relative(path) + workdir.join(path).await.ok() } } -fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef { - path.canonicalize_if_exists() +async fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef { + path.canonicalize(/*sandbox*/ None) + .await + .unwrap_or_else(|_| path.clone()) } #[cfg(test)] diff --git a/codex-rs/core-skills/src/invocation_utils_tests.rs b/codex-rs/core-skills/src/invocation_utils_tests.rs index 5087cc2ec1..b434e10cca 100644 --- a/codex-rs/core-skills/src/invocation_utils_tests.rs +++ b/codex-rs/core-skills/src/invocation_utils_tests.rs @@ -56,10 +56,11 @@ fn script_run_detection_excludes_python_c() { assert_eq!(script_run_token(&tokens).is_some(), false); } -#[test] -fn skill_doc_read_detection_matches_absolute_path() { +#[tokio::test] +async fn skill_doc_read_detection_matches_absolute_path() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); - let normalized_skill_doc_path = canonicalize_if_exists(&local_path_ref(skill_doc_path.clone())); + let normalized_skill_doc_path = + canonicalize_if_exists(&local_path_ref(skill_doc_path.clone())).await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), @@ -77,7 +78,8 @@ fn skill_doc_read_detection_matches_absolute_path() { &outcome, &tokens, &local_path_ref(test_path_buf("/tmp").abs()), - ); + ) + .await; assert_eq!( found.map(|value| value.name), @@ -85,12 +87,42 @@ fn skill_doc_read_detection_matches_absolute_path() { ); } -#[test] -fn skill_script_run_detection_matches_relative_path_from_skill_root() { +#[tokio::test] +async fn skill_doc_read_detection_matches_parent_relative_path() { + let skill_doc_path = test_path_buf("/tmp/project/.agents/skills/test-skill/SKILL.md").abs(); + let normalized_skill_doc_path = + canonicalize_if_exists(&local_path_ref(skill_doc_path.clone())).await; + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), + implicit_skills_by_doc_path: Arc::new(HashMap::from([(normalized_skill_doc_path, skill)])), + ..Default::default() + }; + let tokens = vec![ + "cat".to_string(), + "../.agents/skills/test-skill/SKILL.md".to_string(), + ]; + + let found = detect_skill_doc_read( + &outcome, + &tokens, + &local_path_ref(test_path_buf("/tmp/project/nested").abs()), + ) + .await; + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); +} + +#[tokio::test] +async fn skill_script_run_detection_matches_relative_path_from_skill_root() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); let scripts_dir = canonicalize_if_exists(&local_path_ref( test_path_buf("/tmp/skill-test/scripts").abs(), - )); + )) + .await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -106,7 +138,8 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { &outcome, &tokens, &local_path_ref(test_path_buf("/tmp/skill-test").abs()), - ); + ) + .await; assert_eq!( found.map(|value| value.name), @@ -114,12 +147,13 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() { ); } -#[test] -fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { +#[tokio::test] +async fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs(); let scripts_dir = canonicalize_if_exists(&local_path_ref( test_path_buf("/tmp/skill-test/scripts").abs(), - )); + )) + .await; let skill = test_skill_metadata(skill_doc_path); let outcome = SkillLoadOutcome { implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), @@ -135,7 +169,8 @@ fn skill_script_run_detection_matches_absolute_path_from_any_workdir() { &outcome, &tokens, &local_path_ref(test_path_buf("/tmp/other").abs()), - ); + ) + .await; assert_eq!( found.map(|value| value.name), diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 26a016c8aa..8c8a98c44f 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -168,10 +168,18 @@ where for root in roots { let fs = root.file_system; let root_path_ref = - canonicalize_for_skill_identity(&EnvironmentPathRef::new(Arc::clone(&fs), root.path)); - let plugin_root = root.plugin_root.map(|plugin_root| { - canonicalize_for_skill_identity(&EnvironmentPathRef::new(Arc::clone(&fs), plugin_root)) - }); + canonicalize_for_skill_identity(&EnvironmentPathRef::new(Arc::clone(&fs), root.path)) + .await; + let plugin_root = match root.plugin_root { + Some(plugin_root) => Some( + canonicalize_for_skill_identity(&EnvironmentPathRef::new( + Arc::clone(&fs), + plugin_root, + )) + .await, + ), + None => None, + }; let skills_before_root = outcome.skills.len(); discover_skills_under_root( &root_path_ref, @@ -470,8 +478,10 @@ fn dedupe_skill_roots_by_path(roots: &mut Vec) { }); } -fn canonicalize_for_skill_identity(path: &EnvironmentPathRef) -> EnvironmentPathRef { - path.canonicalize_if_exists() +async fn canonicalize_for_skill_identity(path: &EnvironmentPathRef) -> EnvironmentPathRef { + path.canonicalize(/*sandbox*/ None) + .await + .unwrap_or_else(|_| path.clone()) } async fn discover_skills_under_root( @@ -482,8 +492,11 @@ async fn discover_skills_under_root( outcome: &mut SkillLoadOutcome, ) { let fs = root.file_system(); - let root = canonicalize_for_skill_identity(root); - let plugin_root = plugin_root.map(canonicalize_for_skill_identity); + let root = canonicalize_for_skill_identity(root).await; + let plugin_root = match plugin_root { + Some(plugin_root) => Some(canonicalize_for_skill_identity(plugin_root).await), + None => None, + }; match fs.get_metadata(root.path(), /*sandbox*/ None).await { Ok(metadata) if metadata.is_directory => {} @@ -561,6 +574,7 @@ async fn discover_skills_under_root( Ok(_) => { let resolved_dir = canonicalize_for_skill_identity(&root.with_path(path.clone())) + .await .path() .clone(); enqueue_dir( @@ -588,6 +602,7 @@ async fn discover_skills_under_root( if metadata.is_directory { let resolved_dir = canonicalize_for_skill_identity(&root.with_path(path.clone())) + .await .path() .clone(); enqueue_dir( @@ -690,7 +705,7 @@ async fn parse_skill_file( )?; } - let resolved_path = canonicalize_for_skill_identity(path); + let resolved_path = canonicalize_for_skill_identity(path).await; Ok(SkillMetadata { name, diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index 0925f78fa1..369f21087d 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -190,7 +190,7 @@ impl SkillsManager { self.restriction_product, ); let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); - finalize_skill_outcome(outcome, disabled_paths) + finalize_skill_outcome(outcome, disabled_paths).await } pub fn clear_cache(&self) { @@ -294,13 +294,13 @@ fn config_skills_cache_key( } } -fn finalize_skill_outcome( +async fn finalize_skill_outcome( mut outcome: SkillLoadOutcome, disabled_paths: HashSet, ) -> SkillLoadOutcome { outcome.disabled_paths = disabled_paths; let (by_scripts_dir, by_doc_path) = - build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()); + build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()).await; outcome.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir); outcome.implicit_skills_by_doc_path = Arc::new(by_doc_path); outcome diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index d248564872..2d68283c63 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -172,21 +172,30 @@ mod tests { use codex_utils_absolute_path::test_support::PathBufExt; use pretty_assertions::assert_eq; - #[test] - fn environment_path_ref_joins_only_relative_paths() { + #[tokio::test] + async fn environment_path_ref_join_matches_absolute_path_buf() { 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()) + .join(Path::new("demo/SKILL.md")) + .await + .expect("join") + .path() + .clone(), + root_path.join("demo/SKILL.md").abs() ); - assert!( + assert_eq!( path_ref - .join_relative(std::env::temp_dir().join("SKILL.md").as_path()) - .is_none() + .join(std::env::temp_dir().join("SKILL.md").as_path()) + .await + .expect("join") + .path() + .clone(), + path_ref + .path() + .join(std::env::temp_dir().join("SKILL.md").as_path()) ); } } diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 0eacf780b0..526c425d62 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -54,7 +54,9 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( turn_context.turn_skills.outcome.as_ref(), command, workdir, - ) else { + ) + .await + else { return; }; let source_path = candidate.source_path.clone();