Skip to content

Commit 448fb6a

Browse files
authored
fix: clarify the value of SkillMetadata.path (#12729)
Rename `SkillMetadata.path` to `SkillMetadata.path_to_skills_md` for clarity. Would ideally change the type to `AbsolutePathBuf`, but that can be done later.
1 parent 63c2ac9 commit 448fb6a

File tree

14 files changed

+80
-73
lines changed

14 files changed

+80
-73
lines changed

codex-rs/app-server/src/codex_message_processor.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6526,7 +6526,7 @@ fn skills_to_info(
65266526
skills
65276527
.iter()
65286528
.map(|skill| {
6529-
let enabled = !disabled_paths.contains(&skill.path);
6529+
let enabled = !disabled_paths.contains(&skill.path_to_skills_md);
65306530
codex_app_server_protocol::SkillMetadata {
65316531
name: skill.name.clone(),
65326532
description: skill.description.clone(),
@@ -6557,7 +6557,7 @@ fn skills_to_info(
65576557
.collect(),
65586558
}
65596559
}),
6560-
path: skill.path.clone(),
6560+
path: skill.path_to_skills_md.clone(),
65616561
scope: skill.scope.into(),
65626562
enabled,
65636563
}

codex-rs/core/src/codex.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4734,9 +4734,9 @@ fn skills_to_info(
47344734
.collect(),
47354735
}
47364736
}),
4737-
path: skill.path.clone(),
4737+
path: skill.path_to_skills_md.clone(),
47384738
scope: skill.scope,
4739-
enabled: !disabled_paths.contains(&skill.path),
4739+
enabled: !disabled_paths.contains(&skill.path_to_skills_md),
47404740
})
47414741
.collect()
47424742
}

codex-rs/core/src/mcp/skill_dependencies.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ mod tests {
434434
dependencies: Some(SkillDependencies { tools }),
435435
policy: None,
436436
permissions: None,
437-
path: PathBuf::from("skill"),
437+
path_to_skills_md: PathBuf::from("skill"),
438438
scope: SkillScope::User,
439439
}
440440
}

codex-rs/core/src/mentions.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ pub(crate) fn build_skill_name_counts(
5555
let mut exact_counts: HashMap<String, usize> = HashMap::new();
5656
let mut lower_counts: HashMap<String, usize> = HashMap::new();
5757
for skill in skills {
58-
if disabled_paths.contains(&skill.path) {
58+
if disabled_paths.contains(&skill.path_to_skills_md) {
5959
continue;
6060
}
6161
*exact_counts.entry(skill.name.clone()).or_insert(0) += 1;

codex-rs/core/src/skills/injection.rs

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,18 @@ pub(crate) async fn build_skill_injections(
3737
let mut invocations = Vec::new();
3838

3939
for skill in mentioned_skills {
40-
match fs::read_to_string(&skill.path).await {
40+
match fs::read_to_string(&skill.path_to_skills_md).await {
4141
Ok(contents) => {
4242
emit_skill_injected_metric(otel, skill, "ok");
4343
invocations.push(SkillInvocation {
4444
skill_name: skill.name.clone(),
4545
skill_scope: skill.scope,
46-
skill_path: skill.path.clone(),
46+
skill_path: skill.path_to_skills_md.clone(),
4747
invocation_type: InvocationType::Explicit,
4848
});
4949
result.items.push(ResponseItem::from(SkillInstructions {
5050
name: skill.name.clone(),
51-
path: skill.path.to_string_lossy().into_owned(),
51+
path: skill.path_to_skills_md.to_string_lossy().into_owned(),
5252
contents,
5353
}));
5454
}
@@ -57,7 +57,7 @@ pub(crate) async fn build_skill_injections(
5757
let message = format!(
5858
"Failed to load skill {name} at {path}: {err:#}",
5959
name = skill.name,
60-
path = skill.path.display()
60+
path = skill.path_to_skills_md.display()
6161
);
6262
result.warnings.push(message);
6363
}
@@ -121,9 +121,9 @@ pub(crate) fn collect_explicit_skill_mentions(
121121
if let Some(skill) = selection_context
122122
.skills
123123
.iter()
124-
.find(|skill| skill.path.as_path() == path.as_path())
124+
.find(|skill| skill.path_to_skills_md.as_path() == path.as_path())
125125
{
126-
seen_paths.insert(skill.path.clone());
126+
seen_paths.insert(skill.path_to_skills_md.clone());
127127
seen_names.insert(skill.name.clone());
128128
selected.push(skill.clone());
129129
}
@@ -304,23 +304,27 @@ fn select_skills_from_mentions(
304304
.collect();
305305

306306
for skill in selection_context.skills {
307-
if selection_context.disabled_paths.contains(&skill.path)
308-
|| seen_paths.contains(&skill.path)
307+
if selection_context
308+
.disabled_paths
309+
.contains(&skill.path_to_skills_md)
310+
|| seen_paths.contains(&skill.path_to_skills_md)
309311
{
310312
continue;
311313
}
312314

313-
let path_str = skill.path.to_string_lossy();
315+
let path_str = skill.path_to_skills_md.to_string_lossy();
314316
if mention_skill_paths.contains(path_str.as_ref()) {
315-
seen_paths.insert(skill.path.clone());
317+
seen_paths.insert(skill.path_to_skills_md.clone());
316318
seen_names.insert(skill.name.clone());
317319
selected.push(skill.clone());
318320
}
319321
}
320322

321323
for skill in selection_context.skills {
322-
if selection_context.disabled_paths.contains(&skill.path)
323-
|| seen_paths.contains(&skill.path)
324+
if selection_context
325+
.disabled_paths
326+
.contains(&skill.path_to_skills_md)
327+
|| seen_paths.contains(&skill.path_to_skills_md)
324328
{
325329
continue;
326330
}
@@ -347,7 +351,7 @@ fn select_skills_from_mentions(
347351
}
348352

349353
if seen_names.insert(skill.name.clone()) {
350-
seen_paths.insert(skill.path.clone());
354+
seen_paths.insert(skill.path_to_skills_md.clone());
351355
selected.push(skill.clone());
352356
}
353357
}
@@ -479,7 +483,7 @@ mod tests {
479483
dependencies: None,
480484
policy: None,
481485
permissions: None,
482-
path: PathBuf::from(path),
486+
path_to_skills_md: PathBuf::from(path),
483487
scope: codex_protocol::protocol::SkillScope::User,
484488
}
485489
}

codex-rs/core/src/skills/invocation_utils.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,10 @@ pub(crate) fn build_implicit_skill_path_indexes(
3232
let mut by_scripts_dir = HashMap::new();
3333
let mut by_skill_doc_path = HashMap::new();
3434
for skill in skills {
35-
let skill_doc_path = normalize_path(skill.path.as_path());
35+
let skill_doc_path = normalize_path(skill.path_to_skills_md.as_path());
3636
by_skill_doc_path.insert(skill_doc_path, skill.clone());
3737

38-
if let Some(skill_dir) = skill.path.parent() {
38+
if let Some(skill_dir) = skill.path_to_skills_md.parent() {
3939
let scripts_dir = normalize_path(&skill_dir.join("scripts"));
4040
by_scripts_dir.insert(scripts_dir, skill);
4141
}
@@ -118,7 +118,7 @@ pub(crate) async fn ensure_skill_approval_for_command(
118118

119119
let cache_key = SkillApprovalCacheKey {
120120
skill_name: skill.name.clone(),
121-
skill_path: skill.path.clone(),
121+
skill_path: skill.path_to_skills_md.clone(),
122122
skill_scope: skill.scope,
123123
};
124124
let already_approved = {
@@ -162,7 +162,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
162162
let invocation = SkillInvocation {
163163
skill_name: candidate.name,
164164
skill_scope: candidate.scope,
165-
skill_path: candidate.path,
165+
skill_path: candidate.path_to_skills_md,
166166
invocation_type: InvocationType::Implicit,
167167
};
168168
let skill_scope = match invocation.skill_scope {
@@ -337,7 +337,7 @@ mod tests {
337337
dependencies: None,
338338
policy: None,
339339
permissions: None,
340-
path: skill_doc_path,
340+
path_to_skills_md: skill_doc_path,
341341
scope: codex_protocol::protocol::SkillScope::User,
342342
}
343343
}

0 commit comments

Comments
 (0)