mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
core-skills: await bound path operations
This commit is contained in:
@@ -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<SkillMetadata>,
|
||||
) -> (
|
||||
HashMap<EnvironmentPathRef, SkillMetadata>,
|
||||
@@ -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<SkillMetadata> {
|
||||
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<String> {
|
||||
@@ -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<SkillMetadata> {
|
||||
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<EnvironmentPathRef> {
|
||||
async fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option<EnvironmentPathRef> {
|
||||
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)]
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<SkillRoot>) {
|
||||
});
|
||||
}
|
||||
|
||||
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,
|
||||
|
||||
@@ -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<EnvironmentPathRef>,
|
||||
) -> 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
|
||||
|
||||
@@ -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())
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user