openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-28 18:58:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

core-skills: bind loaded skill identity to environment paths

Browse Source
This commit is contained in:
starr-openai
2026-05-29 15:50:30 -07:00
parent 8c245774d9
commit 7b65b76bfc
23 changed files with 744 additions and 274 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server/src/request_processors/catalog_processor.rs
+2 -2
View File
@@ -17,12 +17,12 @@ const SKILLS_LIST_CWD_CONCURRENCY: usize = 5;
fn skills_to_info(
skills: &[codex_core::skills::SkillMetadata],
disabled_paths: &HashSet<AbsolutePathBuf>,
disabled_paths: &HashSet<codex_exec_server::EnvironmentPathRef>,
) -> Vec<codex_app_server_protocol::SkillMetadata> {
skills
.iter()
.map(|skill| {
let enabled = !disabled_paths.contains(&skill.path_to_skills_md);
let enabled = !disabled_paths.contains(&skill.source_path);
codex_app_server_protocol::SkillMetadata {
name: skill.name.clone(),
description: skill.description.clone(),
codex-rs/core-plugins/src/loader.rs
+4 -1
View File
@@ -665,7 +665,10 @@ pub async fn load_plugin_skills(
.into_iter()
.filter(|skill| skill.matches_product_restriction_for_product(restriction_product))
.collect::<Vec<_>>();
let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules);
let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules)
.into_iter()
.map(|path| path.path().clone())
.collect();
ResolvedPluginSkills {
skills,
codex-rs/core-skills/src/config_rules.rs
+16 -9
View File
@@ -5,6 +5,7 @@ use codex_config::ConfigLayerStack;
use codex_config::ConfigLayerStackOrdering;
use codex_config::SkillConfig;
use codex_config::SkillsConfig;
use codex_exec_server::EnvironmentPathRef;
use codex_utils_absolute_path::AbsolutePathBuf;
use tracing::warn;
@@ -71,28 +72,34 @@ pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> S
pub fn resolve_disabled_skill_paths(
skills: &[SkillMetadata],
rules: &SkillConfigRules,
) -> HashSet<AbsolutePathBuf> {
) -> HashSet<EnvironmentPathRef> {
let mut disabled_paths = HashSet::new();
for entry in &rules.entries {
match &entry.selector {
SkillConfigRuleSelector::Path(path) => {
if entry.enabled {
disabled_paths.remove(path);
} else {
disabled_paths.insert(path.clone());
for source_path in skills
.iter()
.filter(|skill| skill.path_to_skills_md == *path)
.map(|skill| skill.source_path.clone())
{
if entry.enabled {
disabled_paths.remove(&source_path);
} else {
disabled_paths.insert(source_path);
}
}
}
SkillConfigRuleSelector::Name(name) => {
for path in skills
for source_path in skills
.iter()
.filter(|skill| skill.name == *name)
.map(|skill| skill.path_to_skills_md.clone())
.map(|skill| skill.source_path.clone())
{
if entry.enabled {
disabled_paths.remove(&path);
disabled_paths.remove(&source_path);
} else {
disabled_paths.insert(path);
disabled_paths.insert(source_path);
}
}
}
codex-rs/core-skills/src/injection.rs
+50 -32
View File
@@ -1,7 +1,3 @@
use std::collections::HashMap;
use std::collections::HashSet;
use std::sync::Arc;
use crate::SkillLoadOutcome;
use crate::SkillMetadata;
use crate::build_skill_name_counts;
@@ -9,11 +5,13 @@ use codex_analytics::AnalyticsEventsClient;
use codex_analytics::InvocationType;
use codex_analytics::SkillInvocation;
use codex_analytics::TrackEventsContext;
use codex_exec_server::LOCAL_FS;
use codex_exec_server::EnvironmentPathRef;
use codex_otel::SessionTelemetry;
use codex_protocol::user_input::UserInput;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL;
use std::collections::HashMap;
use std::collections::HashSet;
#[derive(Debug, Default)]
pub struct SkillInjections {
@@ -30,7 +28,7 @@ pub struct SkillInjection {
pub async fn build_skill_injections(
mentioned_skills: &[SkillMetadata],
loaded_skills: Option<&SkillLoadOutcome>,
_loaded_skills: Option<&SkillLoadOutcome>,
otel: Option<&SessionTelemetry>,
analytics_client: &AnalyticsEventsClient,
tracking: TrackEventsContext,
@@ -46,13 +44,7 @@ pub async fn build_skill_injections(
let mut invocations = Vec::new();
for skill in mentioned_skills {
let fs = loaded_skills
.and_then(|outcome| outcome.file_system_for_skill(skill))
.unwrap_or_else(|| Arc::clone(&LOCAL_FS));
match fs
.read_file_text(&skill.path_to_skills_md, /*sandbox*/ None)
.await
{
match skill.source_path.read_to_string(/*sandbox*/ None).await {
Ok(contents) => {
emit_skill_injected_metric(otel, skill, "ok");
invocations.push(SkillInvocation {
@@ -115,7 +107,7 @@ fn emit_skill_injected_metric(
pub fn collect_explicit_skill_mentions(
inputs: &[UserInput],
skills: &[SkillMetadata],
disabled_paths: &HashSet<AbsolutePathBuf>,
disabled_paths: &HashSet<EnvironmentPathRef>,
connector_slug_counts: &HashMap<String, usize>,
) -> Vec<SkillMetadata> {
let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0;
@@ -128,7 +120,7 @@ pub fn collect_explicit_skill_mentions(
};
let mut selected: Vec<SkillMetadata> = Vec::new();
let mut seen_names: HashSet<String> = HashSet::new();
let mut seen_paths: HashSet<AbsolutePathBuf> = HashSet::new();
let mut seen_paths: HashSet<EnvironmentPathRef> = HashSet::new();
let mut blocked_plain_names: HashSet<String> = HashSet::new();
for input in inputs {
@@ -138,18 +130,23 @@ pub fn collect_explicit_skill_mentions(
continue;
};
if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) {
continue;
}
if let Some(skill) = selection_context
let matching_skills = selection_context
.skills
.iter()
.find(|skill| skill.path_to_skills_md == path)
.filter(|skill| {
skill.name == *name
&& skill.path_to_skills_md == path
&& !selection_context
.disabled_paths
.contains(&skill.source_path)
})
.collect::<Vec<_>>();
if let [skill] = matching_skills.as_slice()
&& !seen_paths.contains(&skill.source_path)
{
seen_paths.insert(skill.path_to_skills_md.clone());
seen_paths.insert(skill.source_path.clone());
seen_names.insert(skill.name.clone());
selected.push(skill.clone());
selected.push((*skill).clone());
}
}
}
@@ -173,7 +170,7 @@ pub fn collect_explicit_skill_mentions(
struct SkillSelectionContext<'a> {
skills: &'a [SkillMetadata],
disabled_paths: &'a HashSet<AbsolutePathBuf>,
disabled_paths: &'a HashSet<EnvironmentPathRef>,
skill_name_counts: &'a HashMap<String, usize>,
connector_slug_counts: &'a HashMap<String, usize>,
}
@@ -324,7 +321,7 @@ fn select_skills_from_mentions(
blocked_plain_names: &HashSet<String>,
mentions: &ToolMentions<'_>,
seen_names: &mut HashSet<String>,
seen_paths: &mut HashSet<AbsolutePathBuf>,
seen_paths: &mut HashSet<EnvironmentPathRef>,
selected: &mut Vec<SkillMetadata>,
) {
if mentions.is_empty() {
@@ -342,18 +339,39 @@ fn select_skills_from_mentions(
.map(normalize_skill_path)
.collect();
let mention_skill_path_counts = selection_context
.skills
.iter()
.filter(|skill| {
!selection_context
.disabled_paths
.contains(&skill.source_path)
})
.filter_map(|skill| {
let path = skill.path_to_skills_md.to_string_lossy();
mention_skill_paths
.contains(path.as_ref())
.then_some(path.into_owned())
})
.fold(HashMap::new(), |mut counts, path| {
*counts.entry(path).or_insert(0usize) += 1;
counts
});
for skill in selection_context.skills {
if selection_context
.disabled_paths
.contains(&skill.path_to_skills_md)
|| seen_paths.contains(&skill.path_to_skills_md)
.contains(&skill.source_path)
|| seen_paths.contains(&skill.source_path)
{
continue;
}
let path_str = skill.path_to_skills_md.to_string_lossy();
if mention_skill_paths.contains(path_str.as_ref()) {
seen_paths.insert(skill.path_to_skills_md.clone());
if mention_skill_paths.contains(path_str.as_ref())
&& mention_skill_path_counts.get(path_str.as_ref()) == Some(&1)
{
seen_paths.insert(skill.source_path.clone());
seen_names.insert(skill.name.clone());
selected.push(skill.clone());
}
@@ -362,8 +380,8 @@ fn select_skills_from_mentions(
for skill in selection_context.skills {
if selection_context
.disabled_paths
.contains(&skill.path_to_skills_md)
|| seen_paths.contains(&skill.path_to_skills_md)
.contains(&skill.source_path)
|| seen_paths.contains(&skill.source_path)
{
continue;
}
@@ -390,7 +408,7 @@ fn select_skills_from_mentions(
}
if seen_names.insert(skill.name.clone()) {
seen_paths.insert(skill.path_to_skills_md.clone());
seen_paths.insert(skill.source_path.clone());
selected.push(skill.clone());
}
}
codex-rs/core-skills/src/injection_tests.rs
+76 -4
View File
@@ -1,5 +1,6 @@
use super::*;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_exec_server::EnvironmentPathRef;
use codex_exec_server::LocalFileSystem;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_absolute_path::test_support::test_path_buf;
use pretty_assertions::assert_eq;
@@ -14,6 +15,7 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(test_path_buf(path).abs()),
path_to_skills_md: test_path_buf(path).abs(),
scope: codex_protocol::protocol::SkillScope::User,
plugin_id: None,
@@ -37,7 +39,7 @@ fn linked_skill_mention(name: &str, unix_path: &str) -> String {
fn collect_mentions(
inputs: &[UserInput],
skills: &[SkillMetadata],
disabled_paths: &HashSet<AbsolutePathBuf>,
disabled_paths: &HashSet<EnvironmentPathRef>,
connector_slug_counts: &HashMap<String, usize>,
) -> Vec<SkillMetadata> {
collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts)
@@ -193,6 +195,7 @@ fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fal
#[test]
fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() {
let alpha = make_skill("alpha-skill", "/tmp/alpha");
let disabled = HashSet::from([alpha.source_path.clone()]);
let skills = vec![alpha];
let inputs = vec![
UserInput::Text {
@@ -204,7 +207,6 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa
path: test_path_buf("/tmp/alpha"),
},
];
let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]);
let connector_counts = HashMap::new();
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
@@ -212,6 +214,53 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa
assert_eq!(selected, Vec::new());
}
#[test]
fn collect_explicit_skill_mentions_counts_only_enabled_duplicate_names() {
let alpha = make_skill("demo-skill", "/tmp/alpha");
let beta = SkillMetadata {
source_path: EnvironmentPathRef::new(
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
test_path_buf("/tmp/beta").abs(),
),
path_to_skills_md: test_path_buf("/tmp/beta").abs(),
..alpha.clone()
};
let disabled = HashSet::from([beta.source_path.clone()]);
let skills = vec![alpha.clone(), beta];
let inputs = vec![UserInput::Text {
text: "please run $demo-skill".to_string(),
text_elements: Vec::new(),
}];
let connector_counts = HashMap::new();
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
assert_eq!(selected, vec![alpha]);
}
#[test]
fn collect_explicit_skill_mentions_uses_structured_name_to_disambiguate_same_path() {
let first = make_skill("alpha-skill", "/tmp/shared");
let second = SkillMetadata {
name: "beta-skill".to_string(),
source_path: EnvironmentPathRef::new(
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
first.path_to_skills_md.clone(),
),
..first.clone()
};
let skills = vec![first, second.clone()];
let inputs = vec![UserInput::Skill {
name: "beta-skill".to_string(),
path: test_path_buf("/tmp/shared"),
}];
let connector_counts = HashMap::new();
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
assert_eq!(selected, vec![second]);
}
#[test]
fn collect_explicit_skill_mentions_dedupes_by_path() {
let alpha = make_skill("alpha-skill", "/tmp/alpha");
@@ -228,6 +277,29 @@ fn collect_explicit_skill_mentions_dedupes_by_path() {
assert_eq!(selected, vec![alpha]);
}
#[test]
fn collect_explicit_skill_mentions_skips_ambiguous_linked_path() {
let alpha = make_skill("demo-skill", "/tmp/shared");
let beta = SkillMetadata {
name: "demo-skill".to_string(),
source_path: EnvironmentPathRef::new(
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
alpha.path_to_skills_md.clone(),
),
..alpha.clone()
};
let skills = vec![alpha, beta];
let inputs = vec![UserInput::Text {
text: linked_skill_mention("demo-skill", "/tmp/shared"),
text_elements: Vec::new(),
}];
let connector_counts = HashMap::new();
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
assert_eq!(selected, Vec::new());
}
#[test]
fn collect_explicit_skill_mentions_skips_ambiguous_name() {
let alpha = make_skill("demo-skill", "/tmp/alpha");
@@ -297,12 +369,12 @@ fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict(
fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() {
let alpha = make_skill("demo-skill", "/tmp/alpha");
let beta = make_skill("demo-skill", "/tmp/beta");
let disabled = HashSet::from([alpha.source_path.clone()]);
let skills = vec![alpha, beta];
let inputs = vec![UserInput::Text {
text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/alpha")),
text_elements: Vec::new(),
}];
let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]);
let connector_counts = HashMap::new();
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
codex-rs/core-skills/src/invocation_utils.rs
+31 -13
View File
@@ -3,22 +3,24 @@ use std::path::Path;
use crate::SkillLoadOutcome;
use crate::SkillMetadata;
use codex_exec_server::EnvironmentPathRef;
use codex_utils_absolute_path::AbsolutePathBuf;
pub(crate) fn build_implicit_skill_path_indexes(
skills: Vec<SkillMetadata>,
) -> (
HashMap<AbsolutePathBuf, SkillMetadata>,
HashMap<AbsolutePathBuf, SkillMetadata>,
HashMap<EnvironmentPathRef, SkillMetadata>,
HashMap<EnvironmentPathRef, SkillMetadata>,
) {
let mut by_scripts_dir = HashMap::new();
let mut by_skill_doc_path = HashMap::new();
for skill in skills {
let skill_doc_path = canonicalize_if_exists(&skill.path_to_skills_md);
let skill_doc_path = canonicalize_if_exists(&skill.source_path);
by_skill_doc_path.insert(skill_doc_path, skill.clone());
if let Some(skill_dir) = skill.path_to_skills_md.parent() {
let scripts_dir = canonicalize_if_exists(&skill_dir.join("scripts"));
let scripts_dir =
canonicalize_if_exists(&skill.source_path.with_path(skill_dir.join("scripts")));
by_scripts_dir.insert(scripts_dir, skill);
}
}
@@ -29,7 +31,7 @@ pub(crate) fn build_implicit_skill_path_indexes(
pub fn detect_implicit_skill_invocation_for_command(
outcome: &SkillLoadOutcome,
command: &str,
workdir: &AbsolutePathBuf,
workdir: &EnvironmentPathRef,
) -> Option<SkillMetadata> {
let workdir = canonicalize_if_exists(workdir);
let tokens = tokenize_command(command);
@@ -81,14 +83,20 @@ fn script_run_token(tokens: &[String]) -> Option<&str> {
fn detect_skill_script_run(
outcome: &SkillLoadOutcome,
tokens: &[String],
workdir: &AbsolutePathBuf,
workdir: &EnvironmentPathRef,
) -> Option<SkillMetadata> {
let script_token = script_run_token(tokens)?;
let script_path = Path::new(script_token);
let script_path = canonicalize_if_exists(&workdir.join(script_path));
let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path)?);
for path in script_path.ancestors() {
if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&path) {
for path in script_path.path().ancestors() {
let Ok(path) = AbsolutePathBuf::from_absolute_path(path) else {
continue;
};
if let Some(candidate) = outcome
.implicit_skills_by_scripts_dir
.get(&script_path.with_path(path))
{
return Some(candidate.clone());
}
}
@@ -99,7 +107,7 @@ fn detect_skill_script_run(
fn detect_skill_doc_read(
outcome: &SkillLoadOutcome,
tokens: &[String],
workdir: &AbsolutePathBuf,
workdir: &EnvironmentPathRef,
) -> Option<SkillMetadata> {
if !command_reads_file(tokens) {
return None;
@@ -110,7 +118,7 @@ fn detect_skill_doc_read(
continue;
}
let path = Path::new(token);
let candidate_path = canonicalize_if_exists(&workdir.join(path));
let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path)?);
if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) {
return Some(candidate.clone());
}
@@ -136,8 +144,18 @@ fn command_basename(command: &str) -> String {
.to_string()
}
fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf {
path.canonicalize().unwrap_or_else(|_| path.clone())
fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option<EnvironmentPathRef> {
if path.is_absolute() {
AbsolutePathBuf::from_absolute_path(path)
.ok()
.map(|path| workdir.with_path(path))
} else {
workdir.join_relative(path)
}
}
fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef {
path.canonicalize_if_exists()
}
#[cfg(test)]
codex-rs/core-skills/src/invocation_utils_tests.rs
+27 -6
View File
@@ -19,6 +19,7 @@ fn test_skill_metadata(skill_doc_path: AbsolutePathBuf) -> SkillMetadata {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local((skill_doc_path).clone()),
path_to_skills_md: skill_doc_path,
scope: codex_protocol::protocol::SkillScope::User,
plugin_id: None,
@@ -29,6 +30,10 @@ fn test_path_display(unix_path: &str) -> String {
test_path_buf(unix_path).display().to_string()
}
fn local_path_ref(path: AbsolutePathBuf) -> codex_exec_server::EnvironmentPathRef {
codex_exec_server::EnvironmentPathRef::local(path)
}
#[test]
fn script_run_detection_matches_runner_plus_extension() {
let tokens = vec![
@@ -54,7 +59,7 @@ fn script_run_detection_excludes_python_c() {
#[test]
fn skill_doc_read_detection_matches_absolute_path() {
let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs();
let normalized_skill_doc_path = canonicalize_if_exists(&skill_doc_path);
let normalized_skill_doc_path = canonicalize_if_exists(&local_path_ref(skill_doc_path.clone()));
let skill = test_skill_metadata(skill_doc_path);
let outcome = SkillLoadOutcome {
implicit_skills_by_scripts_dir: Arc::new(HashMap::new()),
@@ -68,7 +73,11 @@ fn skill_doc_read_detection_matches_absolute_path() {
"|".to_string(),
"head".to_string(),
];
let found = detect_skill_doc_read(&outcome, &tokens, &test_path_buf("/tmp").abs());
let found = detect_skill_doc_read(
&outcome,
&tokens,
&local_path_ref(test_path_buf("/tmp").abs()),
);
assert_eq!(
found.map(|value| value.name),
@@ -79,7 +88,9 @@ fn skill_doc_read_detection_matches_absolute_path() {
#[test]
fn skill_script_run_detection_matches_relative_path_from_skill_root() {
let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs();
let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs());
let scripts_dir = canonicalize_if_exists(&local_path_ref(
test_path_buf("/tmp/skill-test/scripts").abs(),
));
let skill = test_skill_metadata(skill_doc_path);
let outcome = SkillLoadOutcome {
implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])),
@@ -91,7 +102,11 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() {
"scripts/fetch_comments.py".to_string(),
];
let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/skill-test").abs());
let found = detect_skill_script_run(
&outcome,
&tokens,
&local_path_ref(test_path_buf("/tmp/skill-test").abs()),
);
assert_eq!(
found.map(|value| value.name),
@@ -102,7 +117,9 @@ fn skill_script_run_detection_matches_relative_path_from_skill_root() {
#[test]
fn skill_script_run_detection_matches_absolute_path_from_any_workdir() {
let skill_doc_path = test_path_buf("/tmp/skill-test/SKILL.md").abs();
let scripts_dir = canonicalize_if_exists(&test_path_buf("/tmp/skill-test/scripts").abs());
let scripts_dir = canonicalize_if_exists(&local_path_ref(
test_path_buf("/tmp/skill-test/scripts").abs(),
));
let skill = test_skill_metadata(skill_doc_path);
let outcome = SkillLoadOutcome {
implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])),
@@ -114,7 +131,11 @@ fn skill_script_run_detection_matches_absolute_path_from_any_workdir() {
test_path_display("/tmp/skill-test/scripts/fetch_comments.py"),
];
let found = detect_skill_script_run(&outcome, &tokens, &test_path_buf("/tmp/other").abs());
let found = detect_skill_script_run(
&outcome,
&tokens,
&local_path_ref(test_path_buf("/tmp/other").abs()),
);
assert_eq!(
found.map(|value| value.name),
codex-rs/core-skills/src/loader.rs
+68 -46
View File
@@ -1,6 +1,5 @@
use crate::model::SkillDependencies;
use crate::model::SkillError;
use crate::model::SkillFileSystemsByPath;
use crate::model::SkillInterface;
use crate::model::SkillLoadOutcome;
use crate::model::SkillMetadata;
@@ -13,6 +12,7 @@ use codex_config::ConfigLayerStackOrdering;
use codex_config::default_project_root_markers;
use codex_config::merge_toml_values;
use codex_config::project_root_markers_from_config;
use codex_exec_server::EnvironmentPathRef;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_protocol::protocol::Product;
@@ -163,52 +163,48 @@ where
I: IntoIterator<Item = SkillRoot>,
{
let mut outcome = SkillLoadOutcome::default();
let mut skill_roots: Vec<AbsolutePathBuf> = Vec::new();
let mut skill_root_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf> = HashMap::new();
let mut file_systems_by_skill_path: HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>> =
HashMap::new();
let mut skill_roots: Vec<EnvironmentPathRef> = Vec::new();
let mut skill_root_by_path: HashMap<EnvironmentPathRef, EnvironmentPathRef> = HashMap::new();
for root in roots {
let root_path = canonicalize_for_skill_identity(&root.path);
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))
});
let skills_before_root = outcome.skills.len();
discover_skills_under_root(
fs.as_ref(),
&root_path,
&root_path_ref,
root.scope,
root.plugin_id.as_deref(),
root.plugin_root.as_ref(),
plugin_root.as_ref(),
&mut outcome,
)
.await;
for skill in &outcome.skills[skills_before_root..] {
if !skill_roots.contains(&root_path) {
skill_roots.push(root_path.clone());
if !skill_roots.contains(&root_path_ref) {
skill_roots.push(root_path_ref.clone());
}
skill_root_by_path
.entry(skill.path_to_skills_md.clone())
.or_insert_with(|| root_path.clone());
file_systems_by_skill_path
.entry(skill.path_to_skills_md.clone())
.or_insert_with(|| Arc::clone(&fs));
.entry(skill.source_path.clone())
.or_insert_with(|| root_path_ref.clone());
}
}
let mut seen: HashSet<AbsolutePathBuf> = HashSet::new();
let mut seen: HashSet<EnvironmentPathRef> = HashSet::new();
outcome
.skills
.retain(|skill| seen.insert(skill.path_to_skills_md.clone()));
let retained_skill_paths: HashSet<AbsolutePathBuf> = outcome
.retain(|skill| seen.insert(skill.source_path.clone()));
let retained_skill_paths: HashSet<EnvironmentPathRef> = outcome
.skills
.iter()
.map(|skill| skill.path_to_skills_md.clone())
.map(|skill| skill.source_path.clone())
.collect();
skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path));
let used_roots: HashSet<AbsolutePathBuf> = skill_root_by_path.values().cloned().collect();
let used_roots: HashSet<EnvironmentPathRef> = skill_root_by_path.values().cloned().collect();
skill_roots.retain(|root| used_roots.contains(root));
file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path));
outcome.skill_roots = skill_roots;
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
fn scope_rank(scope: SkillScope) -> u8 {
// Higher-priority scopes first (matches root scan order for dedupe).
@@ -465,31 +461,39 @@ fn dirs_between_project_root_and_cwd(
}
fn dedupe_skill_roots_by_path(roots: &mut Vec<SkillRoot>) {
let mut seen: HashSet<AbsolutePathBuf> = HashSet::new();
roots.retain(|root| seen.insert(root.path.clone()));
let mut seen: HashSet<EnvironmentPathRef> = HashSet::new();
roots.retain(|root| {
seen.insert(EnvironmentPathRef::new(
Arc::clone(&root.file_system),
root.path.clone(),
))
});
}
fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf {
path.canonicalize().unwrap_or_else(|_| path.clone())
fn canonicalize_for_skill_identity(path: &EnvironmentPathRef) -> EnvironmentPathRef {
path.canonicalize_if_exists()
}
async fn discover_skills_under_root(
fs: &dyn ExecutorFileSystem,
root: &AbsolutePathBuf,
root: &EnvironmentPathRef,
scope: SkillScope,
plugin_id: Option<&str>,
plugin_root: Option<&AbsolutePathBuf>,
plugin_root: Option<&EnvironmentPathRef>,
outcome: &mut SkillLoadOutcome,
) {
let fs = root.file_system();
let root = canonicalize_for_skill_identity(root);
let plugin_root = plugin_root.map(canonicalize_for_skill_identity);
match fs.get_metadata(&root, /*sandbox*/ None).await {
match fs.get_metadata(root.path(), /*sandbox*/ None).await {
Ok(metadata) if metadata.is_directory => {}
Ok(_) => return,
Err(err) if err.kind() == io::ErrorKind::NotFound => return,
Err(err) => {
error!("failed to stat skills root {}: {err:#}", root.display());
error!(
"failed to stat skills root {}: {err:#}",
root.path().display()
);
return;
}
}
@@ -520,9 +524,9 @@ async fn discover_skills_under_root(
);
let mut visited_dirs: HashSet<AbsolutePathBuf> = HashSet::new();
visited_dirs.insert(root.clone());
visited_dirs.insert(root.path().clone());
let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.clone(), 0)]);
let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.path().clone(), 0)]);
let mut truncated_by_dir_limit = false;
while let Some((dir, depth)) = queue.pop_front() {
@@ -555,7 +559,10 @@ async fn discover_skills_under_root(
}
match fs.read_directory(&path, /*sandbox*/ None).await {
Ok(_) => {
let resolved_dir = canonicalize_for_skill_identity(&path);
let resolved_dir =
canonicalize_for_skill_identity(&root.with_path(path.clone()))
.path()
.clone();
enqueue_dir(
&mut queue,
&mut visited_dirs,
@@ -580,7 +587,9 @@ async fn discover_skills_under_root(
}
if metadata.is_directory {
let resolved_dir = canonicalize_for_skill_identity(&path);
let resolved_dir = canonicalize_for_skill_identity(&root.with_path(path.clone()))
.path()
.clone();
enqueue_dir(
&mut queue,
&mut visited_dirs,
@@ -592,7 +601,14 @@ async fn discover_skills_under_root(
}
if metadata.is_file && file_name == SKILLS_FILENAME {
match parse_skill_file(fs, &path, scope, plugin_id, plugin_root.as_ref()).await {
match parse_skill_file(
&root.with_path(path.clone()),
scope,
plugin_id,
plugin_root.as_ref(),
)
.await
{
Ok(skill) => {
outcome.skills.push(skill);
}
@@ -613,20 +629,20 @@ async fn discover_skills_under_root(
tracing::warn!(
"skills scan truncated after {} directories (root: {})",
MAX_SKILLS_DIRS_PER_ROOT,
root.display()
root.path().display()
);
}
}
async fn parse_skill_file(
fs: &dyn ExecutorFileSystem,
path: &AbsolutePathBuf,
path: &EnvironmentPathRef,
scope: SkillScope,
plugin_id: Option<&str>,
plugin_root: Option<&AbsolutePathBuf>,
plugin_root: Option<&EnvironmentPathRef>,
) -> Result<SkillMetadata, SkillParseError> {
let fs = path.file_system();
let contents = fs
.read_file_text(path, /*sandbox*/ None)
.read_file_text(path.path(), /*sandbox*/ None)
.await
.map_err(SkillParseError::Read)?;
@@ -640,8 +656,8 @@ async fn parse_skill_file(
.as_deref()
.map(sanitize_single_line)
.filter(|value| !value.is_empty())
.unwrap_or_else(|| default_skill_name(path));
let name = namespaced_skill_name(fs, path, &base_name).await;
.unwrap_or_else(|| default_skill_name(path.path()));
let name = namespaced_skill_name(fs.as_ref(), path.path(), &base_name).await;
let description = parsed
.description
.as_deref()
@@ -657,7 +673,12 @@ async fn parse_skill_file(
interface,
dependencies,
policy,
} = load_skill_metadata(fs, path, plugin_root).await;
} = load_skill_metadata(
fs.as_ref(),
path.path(),
plugin_root.map(EnvironmentPathRef::path),
)
.await;
validate_len(&name, MAX_NAME_LEN, "name")?;
validate_len(&description, MAX_DESCRIPTION_LEN, "description")?;
@@ -678,7 +699,8 @@ async fn parse_skill_file(
interface,
dependencies,
policy,
path_to_skills_md: resolved_path,
path_to_skills_md: resolved_path.path().clone(),
source_path: resolved_path,
scope,
plugin_id: plugin_id.map(str::to_string),
})
codex-rs/core-skills/src/loader_tests.rs
+134 -38
View File
@@ -4,7 +4,9 @@ use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirements;
use codex_config::ConfigRequirementsToml;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_exec_server::LocalFileSystem;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::AbsolutePathBuf;
@@ -144,6 +146,16 @@ fn normalized(path: &Path) -> AbsolutePathBuf {
.abs()
}
fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot {
SkillRoot {
path,
scope,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
}
}
#[tokio::test]
async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin()
-> anyhow::Result<()> {
@@ -330,6 +342,7 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -468,6 +481,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() {
],
}),
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -524,6 +538,9 @@ interface:
}),
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
skill_path.as_path()
)),
path_to_skills_md: normalized(skill_path.as_path()),
scope: SkillScope::User,
plugin_id: None,
@@ -678,6 +695,7 @@ async fn accepts_icon_paths_under_assets_dir() {
}),
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -719,6 +737,7 @@ async fn ignores_invalid_brand_color() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -773,6 +792,7 @@ async fn ignores_default_prompt_over_max_length() {
}),
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -815,6 +835,7 @@ async fn drops_interface_when_icons_are_invalid() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -876,6 +897,7 @@ interface:
}),
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: Some("twilio-developer-kit@test".to_string()),
@@ -925,6 +947,7 @@ interface:
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: Some("twilio-developer-kit@test".to_string()),
@@ -970,6 +993,9 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&shared_skill_path
)),
path_to_skills_md: normalized(&shared_skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1030,6 +1056,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1048,14 +1075,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() {
fs::create_dir_all(admin_root.path()).unwrap();
symlink_dir(shared.path(), &admin_root.path().join("shared"));
let outcome = load_skills_from_roots([SkillRoot {
path: admin_root.path().abs(),
scope: SkillScope::Admin,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
}])
.await;
let outcome =
load_skills_from_roots([local_skill_root(admin_root.path().abs(), SkillScope::Admin)])
.await;
assert!(
outcome.errors.is_empty(),
@@ -1071,6 +1093,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&shared_skill_path
)),
path_to_skills_md: normalized(&shared_skill_path),
scope: SkillScope::Admin,
plugin_id: None,
@@ -1111,6 +1136,9 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&linked_skill_path
)),
path_to_skills_md: normalized(&linked_skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1130,14 +1158,8 @@ async fn system_scope_ignores_symlinked_subdir() {
fs::create_dir_all(&system_root).unwrap();
symlink_dir(shared.path(), &system_root.join("shared"));
let outcome = load_skills_from_roots([SkillRoot {
path: system_root.abs(),
scope: SkillScope::System,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
}])
.await;
let outcome =
load_skills_from_roots([local_skill_root(system_root.abs(), SkillScope::System)]).await;
assert!(
outcome.errors.is_empty(),
"unexpected errors: {:?}",
@@ -1164,14 +1186,8 @@ async fn respects_max_scan_depth_for_user_scope() {
);
let skills_root = codex_home.path().join("skills");
let outcome = load_skills_from_roots([SkillRoot {
path: skills_root.abs(),
scope: SkillScope::User,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
}])
.await;
let outcome =
load_skills_from_roots([local_skill_root(skills_root.abs(), SkillScope::User)]).await;
assert!(
outcome.errors.is_empty(),
@@ -1187,6 +1203,9 @@ async fn respects_max_scan_depth_for_user_scope() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&within_depth_path
)),
path_to_skills_md: normalized(&within_depth_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1215,6 +1234,7 @@ async fn loads_valid_skill() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1248,6 +1268,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1294,6 +1315,7 @@ async fn namespaces_plugin_skills_using_plugin_name() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: Some("sample@test".to_string()),
@@ -1326,6 +1348,7 @@ async fn loads_short_description_from_metadata() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1439,6 +1462,7 @@ async fn loads_skills_from_repo_root() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1475,6 +1499,7 @@ async fn loads_skills_from_agents_dir_without_codex_dir() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1529,6 +1554,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&nested_skill_path
)),
path_to_skills_md: normalized(&nested_skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1540,6 +1568,9 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&root_skill_path
)),
path_to_skills_md: normalized(&root_skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1580,6 +1611,7 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1594,20 +1626,8 @@ async fn deduplicates_by_path_preferring_first_root() {
let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo");
let outcome = load_skills_from_roots([
SkillRoot {
path: root.path().abs(),
scope: SkillScope::Repo,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
},
SkillRoot {
path: root.path().abs(),
scope: SkillScope::User,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_root: None,
},
local_skill_root(root.path().abs(), SkillScope::Repo),
local_skill_root(root.path().abs(), SkillScope::User),
])
.await;
@@ -1625,6 +1645,7 @@ async fn deduplicates_by_path_preferring_first_root() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1632,6 +1653,71 @@ async fn deduplicates_by_path_preferring_first_root() {
);
}
#[tokio::test]
async fn preserves_same_absolute_skill_path_from_distinct_file_systems() {
let root = tempfile::tempdir().expect("tempdir");
let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo");
let shared_path = root.path().abs();
let first_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let second_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let outcome = load_skills_from_roots([
SkillRoot {
path: shared_path.clone(),
scope: SkillScope::Repo,
file_system: first_file_system,
plugin_id: None,
plugin_root: None,
},
SkillRoot {
path: shared_path,
scope: SkillScope::Repo,
file_system: second_file_system,
plugin_id: None,
plugin_root: None,
},
])
.await;
assert_eq!(
outcome
.skills
.iter()
.map(|skill| skill.path_to_skills_md.clone())
.collect::<Vec<_>>(),
vec![normalized(&skill_path), normalized(&skill_path)]
);
assert_ne!(outcome.skills[0].source_path, outcome.skills[1].source_path);
}
#[test]
fn dedupe_skill_roots_preserves_same_path_from_distinct_file_systems() {
let root = tempfile::tempdir().expect("tempdir");
let shared_path = root.path().abs();
let first_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let second_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let mut roots = vec![
SkillRoot {
path: shared_path.clone(),
scope: SkillScope::Repo,
file_system: first_file_system,
plugin_id: None,
plugin_root: None,
},
SkillRoot {
path: shared_path,
scope: SkillScope::Repo,
file_system: second_file_system,
plugin_id: None,
plugin_root: None,
},
];
dedupe_skill_roots_by_path(&mut roots);
assert_eq!(roots.len(), 2);
}
#[tokio::test]
async fn keeps_duplicate_names_from_repo_and_user() {
let codex_home = tempfile::tempdir().expect("tempdir");
@@ -1667,6 +1753,9 @@ async fn keeps_duplicate_names_from_repo_and_user() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&repo_skill_path
)),
path_to_skills_md: normalized(&repo_skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1678,6 +1767,9 @@ async fn keeps_duplicate_names_from_repo_and_user() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(
&user_skill_path
)),
path_to_skills_md: normalized(&user_skill_path),
scope: SkillScope::User,
plugin_id: None,
@@ -1741,6 +1833,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local((first_path).clone()),
path_to_skills_md: first_path,
scope: SkillScope::Repo,
plugin_id: None,
@@ -1752,6 +1845,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local((second_path).clone()),
path_to_skills_md: second_path,
scope: SkillScope::Repo,
plugin_id: None,
@@ -1824,6 +1918,7 @@ async fn loads_skills_when_cwd_is_file_in_repo() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::Repo,
plugin_id: None,
@@ -1883,6 +1978,7 @@ async fn loads_skills_from_system_cache_when_present() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(normalized(&skill_path)),
path_to_skills_md: normalized(&skill_path),
scope: SkillScope::System,
plugin_id: None,
codex-rs/core-skills/src/manager.rs
+18 -11
View File
@@ -4,6 +4,7 @@ use std::sync::Arc;
use std::sync::RwLock;
use codex_config::ConfigLayerStack;
use codex_exec_server::EnvironmentPathRef;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
@@ -52,7 +53,7 @@ pub struct SkillsManager {
codex_home: AbsolutePathBuf,
restriction_product: Option<Product>,
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, SkillLoadOutcome>>,
cache_by_cwd: RwLock<HashMap<EnvironmentPathRef, SkillLoadOutcome>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
}
@@ -146,10 +147,12 @@ impl SkillsManager {
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
let use_cwd_cache = fs.is_some();
if use_cwd_cache
&& !force_reload
&& let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd)
let cwd_path_ref = fs
.as_ref()
.map(|fs| EnvironmentPathRef::new(Arc::clone(fs), input.cwd.clone()));
if !force_reload
&& let Some(cwd_path_ref) = cwd_path_ref.as_ref()
&& let Some(outcome) = self.cached_outcome_for_cwd(cwd_path_ref)
{
return outcome;
}
@@ -167,12 +170,12 @@ impl SkillsManager {
}
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let outcome = self.build_skill_outcome(roots, &skill_config_rules).await;
if use_cwd_cache {
if let Some(cwd_path_ref) = cwd_path_ref {
let mut cache = self
.cache_by_cwd
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(input.cwd.clone(), outcome.clone());
cache.insert(cwd_path_ref, outcome.clone());
}
outcome
}
@@ -213,7 +216,7 @@ impl SkillsManager {
info!("skills cache cleared ({cleared} entries)");
}
fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option<SkillLoadOutcome> {
fn cached_outcome_for_cwd(&self, cwd: &EnvironmentPathRef) -> Option<SkillLoadOutcome> {
match self.cache_by_cwd.read() {
Ok(cache) => cache.get(cwd).cloned(),
Err(err) => err.into_inner().get(cwd).cloned(),
@@ -240,7 +243,7 @@ impl SkillsManager {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct ConfigSkillsCacheKey {
roots: Vec<(AbsolutePathBuf, u8, Option<String>)>,
roots: Vec<(EnvironmentPathRef, u8, Option<String>)>,
skill_config_rules: SkillConfigRules,
}
@@ -280,7 +283,11 @@ fn config_skills_cache_key(
SkillScope::System => 2,
SkillScope::Admin => 3,
};
(root.path.clone(), scope_rank, root.plugin_id.clone())
(
EnvironmentPathRef::new(Arc::clone(&root.file_system), root.path.clone()),
scope_rank,
root.plugin_id.clone(),
)
})
.collect(),
skill_config_rules: skill_config_rules.clone(),
@@ -289,7 +296,7 @@ fn config_skills_cache_key(
fn finalize_skill_outcome(
mut outcome: SkillLoadOutcome,
disabled_paths: HashSet<AbsolutePathBuf>,
disabled_paths: HashSet<EnvironmentPathRef>,
) -> SkillLoadOutcome {
outcome.disabled_paths = disabled_paths;
let (by_scripts_dir, by_doc_path) =
codex-rs/core-skills/src/manager_tests.rs
+190 -13
View File
@@ -7,7 +7,10 @@ use codex_config::CONFIG_TOML_FILE;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirementsToml;
use codex_exec_server::EnvironmentPathRef;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_exec_server::LocalFileSystem;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_absolute_path::test_support::PathExt;
@@ -71,6 +74,10 @@ fn plugin_skill_root_for_skill_path(skill_path: &Path, plugin_id: &str) -> Plugi
}
fn test_skill(name: &str, path: PathBuf) -> SkillMetadata {
let path = path
.abs()
.canonicalize()
.expect("skill path should canonicalize");
SkillMetadata {
name: name.to_string(),
description: "test".to_string(),
@@ -78,10 +85,8 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata {
interface: None,
dependencies: None,
policy: None,
path_to_skills_md: path
.abs()
.canonicalize()
.expect("skill path should canonicalize"),
source_path: EnvironmentPathRef::local(path.clone()),
path_to_skills_md: path,
scope: SkillScope::User,
plugin_id: None,
}
@@ -221,6 +226,51 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() {
assert_eq!(outcome2.skills, outcome1.skills);
}
#[tokio::test]
async fn skills_for_config_keeps_cache_entries_bound_to_file_system() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let config_layer_stack = config_stack(&codex_home, "");
let repo_skill_dir = cwd.path().join(".agents/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 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 first_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let second_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let first = skills_manager
.skills_for_config(&skills_input, Some(first_file_system))
.await;
let second = skills_manager
.skills_for_config(&skills_input, Some(second_file_system))
.await;
let first_skill = first
.skills
.iter()
.find(|skill| skill.name == "repo-skill")
.expect("repo skill should load");
let second_skill = second
.skills
.iter()
.find(|skill| skill.name == "repo-skill")
.expect("repo skill should load");
assert_ne!(first_skill.source_path, second_skill.source_path);
}
#[tokio::test]
async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
let codex_home = tempfile::tempdir().expect("tempdir");
@@ -382,7 +432,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() {
.abs();
assert_eq!(skill.path_to_skills_md, skill_path);
assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md));
assert!(outcome.disabled_paths.contains(&skill.source_path));
assert!(
!outcome
.allowed_skills_for_implicit_invocation()
@@ -616,6 +666,59 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() {
);
}
#[tokio::test]
async fn skills_for_cwd_keeps_cache_entries_bound_to_file_system() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let config_layer_stack = config_stack(&codex_home, "");
let repo_skill_dir = cwd.path().join(".agents/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 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 first_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let second_file_system: Arc<dyn ExecutorFileSystem> = Arc::new(LocalFileSystem::unsandboxed());
let first = skills_manager
.skills_for_cwd(
&skills_input,
/*force_reload*/ false,
Some(first_file_system),
)
.await;
let second = skills_manager
.skills_for_cwd(
&skills_input,
/*force_reload*/ false,
Some(second_file_system),
)
.await;
let first_skill = first
.skills
.iter()
.find(|skill| skill.name == "repo-skill")
.expect("repo skill should load");
let second_skill = second
.skills
.iter()
.find(|skill| skill.name == "repo-skill")
.expect("repo skill should load");
assert_ne!(first_skill.source_path, second_skill.source_path);
}
#[cfg_attr(windows, ignore)]
#[test]
fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() {
@@ -682,10 +785,12 @@ fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill(
let skill_config_rules = skill_config_rules_from_stack(&stack);
assert_eq!(
resolve_disabled_skill_paths(&[skill], &skill_config_rules),
HashSet::from([skill_path
.abs()
.canonicalize()
.expect("skill path should canonicalize")])
HashSet::from([EnvironmentPathRef::local(
skill_path
.abs()
.canonicalize()
.expect("skill path should canonicalize"),
)])
);
}
@@ -715,10 +820,82 @@ fn disabled_paths_for_skills_disables_matching_name_selectors() {
let skill_config_rules = skill_config_rules_from_stack(&stack);
assert_eq!(
resolve_disabled_skill_paths(&[skill], &skill_config_rules),
HashSet::from([skill_path
.abs()
.canonicalize()
.expect("skill path should canonicalize")])
HashSet::from([EnvironmentPathRef::local(
skill_path
.abs()
.canonicalize()
.expect("skill path should canonicalize"),
)])
);
}
#[cfg_attr(windows, ignore)]
#[test]
fn path_rules_apply_to_every_matching_source_path() {
let tempdir = tempfile::tempdir().expect("tempdir");
let skill_path = write_demo_skill(&tempdir);
let first = test_skill("demo-skill", skill_path.clone());
let second = SkillMetadata {
source_path: EnvironmentPathRef::new(
Arc::new(LocalFileSystem::unsandboxed()),
first.path_to_skills_md.clone(),
),
..first.clone()
};
let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml"))
.expect("user config path should be absolute");
let disabled_layer = ConfigLayerEntry::new(
ConfigLayerSource::User {
file: user_file.clone(),
profile: None,
},
toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false))
.expect("user layer toml"),
);
let disabled_stack = ConfigLayerStack::new(
vec![disabled_layer],
Default::default(),
ConfigRequirementsToml::default(),
)
.expect("valid config layer stack");
let disabled_paths = resolve_disabled_skill_paths(
&[first.clone(), second.clone()],
&skill_config_rules_from_stack(&disabled_stack),
);
assert_eq!(
disabled_paths,
HashSet::from([first.source_path.clone(), second.source_path.clone()])
);
let reenabled_layer = ConfigLayerEntry::new(
ConfigLayerSource::SessionFlags,
toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ true))
.expect("session layer toml"),
);
let reenabled_stack = ConfigLayerStack::new(
vec![
ConfigLayerEntry::new(
ConfigLayerSource::User {
file: user_file,
profile: None,
},
toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false))
.expect("user layer toml"),
),
reenabled_layer,
],
Default::default(),
ConfigRequirementsToml::default(),
)
.expect("valid config layer stack");
assert_eq!(
resolve_disabled_skill_paths(
&[first, second],
&skill_config_rules_from_stack(&reenabled_stack),
),
HashSet::new()
);
}
codex-rs/core-skills/src/mention_counts.rs
+3 -3
View File
@@ -2,17 +2,17 @@ use std::collections::HashMap;
use std::collections::HashSet;
use super::SkillMetadata;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_exec_server::EnvironmentPathRef;
/// Counts how often each skill name appears (exact and ASCII-lowercase), excluding disabled paths.
pub fn build_skill_name_counts(
skills: &[SkillMetadata],
disabled_paths: &HashSet<AbsolutePathBuf>,
disabled_paths: &HashSet<EnvironmentPathRef>,
) -> (HashMap<String, usize>, HashMap<String, usize>) {
let mut exact_counts: HashMap<String, usize> = HashMap::new();
let mut lower_counts: HashMap<String, usize> = HashMap::new();
for skill in skills {
if disabled_paths.contains(&skill.path_to_skills_md) {
if disabled_paths.contains(&skill.source_path) {
continue;
}
*exact_counts.entry(skill.name.clone()).or_insert(0) += 1;
codex-rs/core-skills/src/model.rs
+43 -63
View File
@@ -1,12 +1,10 @@
use std::collections::HashMap;
use std::collections::HashSet;
use std::fmt;
use std::sync::Arc;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::EnvironmentPathRef;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashMap;
use std::collections::HashSet;
use std::sync::Arc;
#[derive(Debug, Clone, PartialEq)]
pub struct SkillMetadata {
@@ -16,8 +14,10 @@ pub struct SkillMetadata {
pub interface: Option<SkillInterface>,
pub dependencies: Option<SkillDependencies>,
pub policy: Option<SkillPolicy>,
/// Path to the SKILLS.md file that declares this skill.
/// Best-effort resolved raw path used for display, wire payloads, and config selectors.
pub path_to_skills_md: AbsolutePathBuf,
/// Same resolved path bound to the filesystem that loaded this skill.
pub source_path: EnvironmentPathRef,
pub scope: SkillScope,
pub plugin_id: Option<String>,
}
@@ -89,17 +89,16 @@ pub struct SkillError {
pub struct SkillLoadOutcome {
pub skills: Vec<SkillMetadata>,
pub errors: Vec<SkillError>,
pub disabled_paths: HashSet<AbsolutePathBuf>,
pub(crate) skill_roots: Vec<AbsolutePathBuf>,
pub(crate) skill_root_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath,
pub(crate) implicit_skills_by_scripts_dir: Arc<HashMap<AbsolutePathBuf, SkillMetadata>>,
pub(crate) implicit_skills_by_doc_path: Arc<HashMap<AbsolutePathBuf, SkillMetadata>>,
pub disabled_paths: HashSet<EnvironmentPathRef>,
pub(crate) skill_roots: Vec<EnvironmentPathRef>,
pub(crate) skill_root_by_path: Arc<HashMap<EnvironmentPathRef, EnvironmentPathRef>>,
pub(crate) implicit_skills_by_scripts_dir: Arc<HashMap<EnvironmentPathRef, SkillMetadata>>,
pub(crate) implicit_skills_by_doc_path: Arc<HashMap<EnvironmentPathRef, SkillMetadata>>,
}
impl SkillLoadOutcome {
pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool {
!self.disabled_paths.contains(&skill.path_to_skills_md)
!self.disabled_paths.contains(&skill.source_path)
}
pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool {
@@ -119,49 +118,6 @@ impl SkillLoadOutcome {
.iter()
.map(|skill| (skill, self.is_skill_enabled(skill)))
}
pub(crate) fn file_system_for_skill(
&self,
skill: &SkillMetadata,
) -> Option<Arc<dyn ExecutorFileSystem>> {
self.file_systems_by_skill_path
.get(&skill.path_to_skills_md)
}
}
#[derive(Clone, Default)]
pub(crate) struct SkillFileSystemsByPath {
values: Arc<HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>>>,
}
impl SkillFileSystemsByPath {
pub(crate) fn new(values: HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>>) -> Self {
Self {
values: Arc::new(values),
}
}
fn get(&self, path: &AbsolutePathBuf) -> Option<Arc<dyn ExecutorFileSystem>> {
self.values.get(path).map(Arc::clone)
}
fn retain_paths(&mut self, paths: &HashSet<AbsolutePathBuf>) {
self.values = Arc::new(
self.values
.iter()
.filter(|(path, _)| paths.contains(*path))
.map(|(path, fs)| (path.clone(), Arc::clone(fs)))
.collect(),
);
}
}
impl fmt::Debug for SkillFileSystemsByPath {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("SkillFileSystemsByPath")
.field("len", &self.values.len())
.finish()
}
}
pub fn filter_skill_load_outcome_for_product(
@@ -171,14 +127,11 @@ pub fn filter_skill_load_outcome_for_product(
outcome
.skills
.retain(|skill| skill.matches_product_restriction_for_product(restriction_product));
let retained_paths: HashSet<AbsolutePathBuf> = outcome
let retained_paths: HashSet<EnvironmentPathRef> = outcome
.skills
.iter()
.map(|skill| skill.path_to_skills_md.clone())
.map(|skill| skill.source_path.clone())
.collect();
outcome
.file_systems_by_skill_path
.retain_paths(&retained_paths);
outcome.skill_root_by_path = Arc::new(
outcome
.skill_root_by_path
@@ -187,7 +140,7 @@ pub fn filter_skill_load_outcome_for_product(
.map(|(path, root)| (path.clone(), root.clone()))
.collect(),
);
let retained_roots: HashSet<AbsolutePathBuf> =
let retained_roots: HashSet<EnvironmentPathRef> =
outcome.skill_root_by_path.values().cloned().collect();
outcome
.skill_roots
@@ -210,3 +163,30 @@ pub fn filter_skill_load_outcome_for_product(
);
outcome
}
#[cfg(test)]
mod tests {
use std::path::Path;
use codex_exec_server::EnvironmentPathRef;
use codex_utils_absolute_path::test_support::PathBufExt;
use pretty_assertions::assert_eq;
#[test]
fn environment_path_ref_joins_only_relative_paths() {
let root_path = std::env::temp_dir().join("skills");
let path_ref = EnvironmentPathRef::local(root_path.abs());
assert_eq!(
path_ref
.join_relative(Path::new("demo/SKILL.md"))
.map(|path_ref| path_ref.path().clone()),
Some(root_path.join("demo/SKILL.md").abs())
);
assert!(
path_ref
.join_relative(std::env::temp_dir().join("SKILL.md").as_path())
.is_none()
);
}
}
codex-rs/core-skills/src/render.rs
+29 -18
View File
@@ -5,6 +5,7 @@ use std::path::Path;
use crate::model::SkillLoadOutcome;
use crate::model::SkillMetadata;
use codex_exec_server::EnvironmentPathRef;
use codex_otel::SessionTelemetry;
use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC;
use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC;
@@ -665,8 +666,8 @@ struct SkillPathAliases {
struct AliasPlan {
aliases: SkillPathAliases,
root_aliases: HashMap<AbsolutePathBuf, String>,
alias_root_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf>,
root_aliases: HashMap<EnvironmentPathRef, String>,
alias_root_by_path: HashMap<EnvironmentPathRef, EnvironmentPathRef>,
table_cost: usize,
}
@@ -677,7 +678,7 @@ fn build_alias_plan(
) -> Option<AliasPlan> {
let skill_paths = skills
.iter()
.map(|skill| skill.path_to_skills_md.clone())
.map(|skill| skill.source_path.clone())
.collect::<HashSet<_>>();
let skill_root_by_path = outcome
.skill_root_by_path
@@ -736,9 +737,9 @@ fn build_alias_plan(
}
fn ordered_alias_roots(
used_roots: &[AbsolutePathBuf],
alias_root_by_skill_root: &HashMap<AbsolutePathBuf, AbsolutePathBuf>,
) -> Option<Vec<AbsolutePathBuf>> {
used_roots: &[EnvironmentPathRef],
alias_root_by_skill_root: &HashMap<EnvironmentPathRef, EnvironmentPathRef>,
) -> Option<Vec<EnvironmentPathRef>> {
let mut seen = HashSet::new();
let mut alias_roots = Vec::new();
for root in used_roots {
@@ -751,10 +752,10 @@ fn ordered_alias_roots(
}
fn alias_root_for_skill_root(
root: &AbsolutePathBuf,
root: &EnvironmentPathRef,
plugin_version_skill_counts: &HashMap<AbsolutePathBuf, usize>,
) -> AbsolutePathBuf {
let Some(plugin_version_base) = plugin_version_base(root.as_path()) else {
) -> EnvironmentPathRef {
let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) else {
return root.clone();
};
let skill_count = plugin_version_skill_counts
@@ -764,16 +765,18 @@ fn alias_root_for_skill_root(
if skill_count > 1 {
root.clone()
} else {
plugin_marketplace_base(root.as_path()).unwrap_or_else(|| root.clone())
plugin_marketplace_base(root.path().as_path())
.map(|path| root.with_path(path))
.unwrap_or_else(|| root.clone())
}
}
fn plugin_version_skill_counts_for_skill_roots<'a>(
skill_roots: impl Iterator<Item = &'a AbsolutePathBuf>,
skill_roots: impl Iterator<Item = &'a EnvironmentPathRef>,
) -> HashMap<AbsolutePathBuf, usize> {
let mut counts = HashMap::new();
for root in skill_roots {
if let Some(plugin_version_base) = plugin_version_base(root.as_path()) {
if let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) {
let count = counts.entry(plugin_version_base).or_insert(0usize);
*count = count.saturating_add(1);
}
@@ -793,12 +796,12 @@ fn aliased_metadata_overhead_cost(
.saturating_sub(budget.cost(&absolute_body))
}
fn build_skill_root_lines(roots: &[AbsolutePathBuf]) -> Vec<String> {
fn build_skill_root_lines(roots: &[EnvironmentPathRef]) -> Vec<String> {
roots
.iter()
.enumerate()
.map(|(index, root)| {
let root_str = root.to_string_lossy().replace('\\', "/");
let root_str = root.path().to_string_lossy().replace('\\', "/");
format!("- `r{index}` = `{root_str}`")
})
.collect()
@@ -840,12 +843,12 @@ fn render_skill_path_with_aliases(skill: &SkillMetadata, plan: &AliasPlan) -> St
}
fn outcome_relative_skill_path(skill: &SkillMetadata, plan: &AliasPlan) -> Option<String> {
let alias_root = plan.alias_root_by_path.get(&skill.path_to_skills_md)?;
let alias_root = plan.alias_root_by_path.get(&skill.source_path)?;
let alias = plan.root_aliases.get(alias_root)?;
let relative_path = skill
.path_to_skills_md
.as_path()
.strip_prefix(alias_root.as_path())
.strip_prefix(alias_root.path().as_path())
.ok()?;
let relative_path = relative_path.to_string_lossy().replace('\\', "/");
Some(format!("{alias}/{relative_path}"))
@@ -920,6 +923,9 @@ mod tests {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(),
),
path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(),
scope,
plugin_id: None,
@@ -948,6 +954,10 @@ mod tests {
skills: Vec<SkillMetadata>,
roots: Vec<AbsolutePathBuf>,
) -> SkillLoadOutcome {
let roots = roots
.into_iter()
.map(codex_exec_server::EnvironmentPathRef::local)
.collect::<Vec<_>>();
let skill_root_by_path = skills
.iter()
.filter_map(|skill| {
@@ -957,9 +967,9 @@ mod tests {
skill
.path_to_skills_md
.as_path()
.starts_with(root.as_path())
.starts_with(root.path().as_path())
})
.map(|root| (skill.path_to_skills_md.clone(), root.clone()))
.map(|root| (skill.source_path.clone(), root.clone()))
})
.collect::<HashMap<_, _>>();
SkillLoadOutcome {
@@ -1507,6 +1517,7 @@ mod tests {
fn skill_with_path(name: &str, path: &AbsolutePathBuf) -> SkillMetadata {
let mut skill = make_skill(name, SkillScope::User);
skill.path_to_skills_md = path.clone();
skill.source_path = codex_exec_server::EnvironmentPathRef::local(path.clone());
skill
}
}
codex-rs/core/src/session/tests.rs
+21
View File
@@ -7224,6 +7224,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
scope: SkillScope::Admin,
plugin_id: None,
@@ -7235,6 +7238,9 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
scope: SkillScope::Repo,
plugin_id: None,
@@ -7271,6 +7277,9 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() {
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
scope: SkillScope::Repo,
plugin_id: None,
@@ -7316,6 +7325,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/alpha-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/alpha-skill/SKILL.md").abs(),
scope: SkillScope::Repo,
plugin_id: None,
@@ -7327,6 +7339,9 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/beta-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/beta-skill/SKILL.md").abs(),
scope: SkillScope::Repo,
plugin_id: None,
@@ -7375,6 +7390,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
scope: SkillScope::Admin,
plugin_id: None,
@@ -7386,6 +7404,9 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil
interface: None,
dependencies: None,
policy: None,
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
),
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
scope: SkillScope::Repo,
plugin_id: None,
codex-rs/core/src/session/turn_context.rs
+2 -1
View File
@@ -2,6 +2,7 @@ use super::*;
use crate::SkillLoadOutcome;
use crate::config::GhostSnapshotConfig;
use crate::environment_selection::ResolvedTurnEnvironments;
use crate::skills::EnvironmentPathRef;
use codex_model_provider::SharedModelProvider;
use codex_model_provider::create_model_provider;
use codex_protocol::SessionId;
@@ -18,7 +19,7 @@ use std::sync::atomic::Ordering;
#[derive(Clone, Debug)]
pub(crate) struct TurnSkillsContext {
pub(crate) outcome: Arc<SkillLoadOutcome>,
pub(crate) implicit_invocation_seen_skills: Arc<Mutex<HashSet<String>>>,
pub(crate) implicit_invocation_seen_skills: Arc<Mutex<HashSet<EnvironmentPathRef>>>,
}
impl TurnSkillsContext {
codex-rs/core/src/skills.rs
+4 -12
View File
@@ -4,8 +4,7 @@ use crate::session::turn_context::TurnContext;
use codex_analytics::InvocationType;
use codex_analytics::SkillInvocation;
use codex_analytics::build_track_events_context;
use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::AbsolutePathBuf;
pub use codex_exec_server::EnvironmentPathRef;
use codex_utils_plugins::PluginSkillRoot;
pub use codex_core_skills::SkillError;
@@ -49,7 +48,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
sess: &Session,
turn_context: &TurnContext,
command: &str,
workdir: &AbsolutePathBuf,
workdir: &EnvironmentPathRef,
) {
let Some(candidate) = detect_implicit_skill_invocation_for_command(
turn_context.turn_skills.outcome.as_ref(),
@@ -58,6 +57,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
) else {
return;
};
let source_path = candidate.source_path.clone();
let invocation = SkillInvocation {
skill_name: candidate.name,
skill_scope: candidate.scope,
@@ -65,22 +65,14 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
plugin_id: candidate.plugin_id,
invocation_type: InvocationType::Implicit,
};
let skill_scope = match invocation.skill_scope {
SkillScope::User => "user",
SkillScope::Repo => "repo",
SkillScope::System => "system",
SkillScope::Admin => "admin",
};
let skill_path = invocation.skill_path.to_string_lossy();
let skill_name = invocation.skill_name.clone();
let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}");
let inserted = {
let mut seen_skills = turn_context
.turn_skills
.implicit_invocation_seen_skills
.lock()
.await;
seen_skills.insert(seen_key)
seen_skills.insert(source_path)
};
if !inserted {
return;
codex-rs/core/src/tools/handlers/shell/shell_command.rs
+10
View File
@@ -167,6 +167,16 @@ impl ToolExecutor<ToolInvocation> for ShellCommandHandler {
let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?;
#[allow(deprecated)]
let workdir = turn.resolve_path(params.workdir.clone());
let workdir = turn
.environments
.primary()
.map(|environment| {
crate::skills::EnvironmentPathRef::new(
environment.environment.get_filesystem(),
workdir.clone(),
)
})
.unwrap_or_else(|| crate::skills::EnvironmentPathRef::local(workdir.clone()));
maybe_emit_implicit_skill_invocation(
session.as_ref(),
turn.as_ref(),
codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs
+2 -1
View File
@@ -132,11 +132,12 @@ impl ToolExecutor<ToolInvocation> for ExecCommandHandler {
let fs = environment.get_filesystem();
let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?;
let hook_command = args.cmd.clone();
let workdir = crate::skills::EnvironmentPathRef::new(Arc::clone(&fs), cwd.clone());
maybe_emit_implicit_skill_invocation(
session.as_ref(),
context.turn.as_ref(),
&hook_command,
&cwd,
&workdir,
)
.await;
let process_id = manager.allocate_process_id().await;
codex-rs/tui/src/bottom_pane/chat_composer.rs
+5
View File
@@ -6255,6 +6255,7 @@ mod tests {
dependencies: None,
policy: None,
path_to_skills_md: skill_path.clone(),
source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()),
scope: crate::test_support::skill_scope_user(),
plugin_id: None,
}]));
@@ -6298,6 +6299,7 @@ mod tests {
dependencies: None,
policy: None,
path_to_skills_md: skill_path.clone(),
source_path: codex_exec_server::EnvironmentPathRef::local(skill_path.clone()),
scope: crate::test_support::skill_scope_repo(),
plugin_id: None,
}]));
@@ -6390,6 +6392,9 @@ mod tests {
dependencies: None,
policy: None,
path_to_skills_md: test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(),
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(),
),
scope: crate::test_support::skill_scope_repo(),
plugin_id: None,
}]));
codex-rs/tui/src/bottom_pane/mod.rs
+3
View File
@@ -2577,6 +2577,9 @@ mod tests {
dependencies: None,
policy: None,
path_to_skills_md: test_path_buf("/tmp/test-skill/SKILL.md").abs(),
source_path: codex_exec_server::EnvironmentPathRef::local(
test_path_buf("/tmp/test-skill/SKILL.md").abs(),
),
scope: crate::test_support::skill_scope_user(),
plugin_id: None,
}]),
codex-rs/tui/src/chatwidget/skills.rs
+3
View File
@@ -242,6 +242,9 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> Option<SkillMetadata
}),
policy: None,
path_to_skills_md: skill.path.clone(),
// TUI only uses listed skills for display and raw-path mention payloads. `skills/list`
// does not carry source authority yet, so this placeholder must not be used for reads.
source_path: codex_exec_server::EnvironmentPathRef::local(skill.path.clone()),
scope,
plugin_id: None,
})
codex-rs/tui/src/chatwidget/tests/composer_submission.rs
+3 -1
View File
@@ -548,7 +548,8 @@ async fn submission_prefers_selected_duplicate_skill_path() {
interface: None,
dependencies: None,
policy: None,
path_to_skills_md: repo_skill_path,
path_to_skills_md: repo_skill_path.clone(),
source_path: codex_exec_server::EnvironmentPathRef::local(repo_skill_path),
scope: crate::test_support::skill_scope_repo(),
plugin_id: None,
},
@@ -560,6 +561,7 @@ async fn submission_prefers_selected_duplicate_skill_path() {
dependencies: None,
policy: None,
path_to_skills_md: user_skill_path.clone(),
source_path: codex_exec_server::EnvironmentPathRef::local(user_skill_path.clone()),
scope: crate::test_support::skill_scope_user(),
plugin_id: None,
},