diff --git a/codex-rs/ext/skills/src/aliases.rs b/codex-rs/ext/skills/src/aliases.rs index 954877efcd..2c532a39bf 100644 --- a/codex-rs/ext/skills/src/aliases.rs +++ b/codex-rs/ext/skills/src/aliases.rs @@ -1,5 +1,71 @@ use std::collections::HashSet; +use serde::Deserialize; +use serde::Serialize; + +use crate::catalog::SkillSourceKind; + +fn alias_prefix(source: &SkillSourceKind) -> Option<&'static str> { + match source { + SkillSourceKind::Cloud => Some("c"), + SkillSourceKind::Executor => Some("e"), + SkillSourceKind::Host => Some("r"), + SkillSourceKind::Custom(_) => None, + } +} + +#[derive(Clone, Debug, Deserialize, Serialize)] +#[serde(from = "String", into = "String")] +pub(crate) struct SkillLocator { + value: String, + alias_source: Option, +} + +impl From for SkillLocator { + fn from(value: String) -> Self { + let alias_source = value.split_once('/').and_then(|(root, suffix)| { + if suffix.is_empty() { + return None; + } + let (prefix, index) = root.split_at_checked(1)?; + if index.is_empty() + || !index.bytes().all(|byte| byte.is_ascii_digit()) + || (index.len() > 1 && index.starts_with('0')) + { + return None; + } + index.parse::().ok()?; + [ + SkillSourceKind::Cloud, + SkillSourceKind::Executor, + SkillSourceKind::Host, + ] + .into_iter() + .find(|source| alias_prefix(source) == Some(prefix)) + }); + Self { + value, + alias_source, + } + } +} + +impl From for String { + fn from(locator: SkillLocator) -> Self { + locator.value + } +} + +impl SkillLocator { + pub(crate) fn as_str(&self) -> &str { + &self.value + } + + pub(crate) fn alias_source(&self) -> Option<&SkillSourceKind> { + self.alias_source.as_ref() + } +} + struct AliasRoot { name: String, value: String, @@ -10,7 +76,8 @@ pub(crate) struct AliasPlan { } impl AliasPlan { - pub(crate) fn build(prefix: &str, candidates: &[&str]) -> Option { + pub(crate) fn build(source: &SkillSourceKind, candidates: &[&str]) -> Option { + let prefix = alias_prefix(source)?; let mut roots = Vec::new(); let mut seen = HashSet::new(); diff --git a/codex-rs/ext/skills/src/aliases_tests.rs b/codex-rs/ext/skills/src/aliases_tests.rs index 814abef22c..5e388a2b20 100644 --- a/codex-rs/ext/skills/src/aliases_tests.rs +++ b/codex-rs/ext/skills/src/aliases_tests.rs @@ -1,6 +1,8 @@ use pretty_assertions::assert_eq; use super::AliasPlan; +use super::SkillLocator; +use crate::catalog::SkillSourceKind; #[test] fn assigns_aliases_in_first_seen_order() { @@ -10,7 +12,7 @@ fn assigns_aliases_in_first_seen_order() { "/skills/beta", "/skills/alpha", ]; - let plan = AliasPlan::build("r", &roots).expect("alias plan should build"); + let plan = AliasPlan::build(&SkillSourceKind::Host, &roots).expect("alias plan should build"); assert_eq!( vec!["- `r0` = `/skills/beta`", "- `r1` = `/skills/alpha`"], @@ -21,7 +23,8 @@ fn assigns_aliases_in_first_seen_order() { #[test] fn reuses_aliases_for_duplicate_roots() { let root = "/skills/shared"; - let plan = AliasPlan::build("r", &[root, root, root]).expect("alias plan should build"); + let plan = AliasPlan::build(&SkillSourceKind::Host, &[root, root, root]) + .expect("alias plan should build"); assert_eq!(vec!["- `r0` = `/skills/shared`"], plan.root_lines()); assert_eq!( @@ -36,31 +39,43 @@ fn reuses_aliases_for_duplicate_roots() { #[test] fn shortens_filesystem_and_skill_resource_locators() { - for (prefix, root, locator, expected) in [ + for (source, root, locator, expected) in [ ( - "r", + SkillSourceKind::Host, "/skills/shared", "/skills/shared/alpha/SKILL.md", "r0/alpha/SKILL.md", ), ( - "e", + SkillSourceKind::Executor, "skill://executor/workspace/skills", "skill://executor/workspace/skills/alpha/SKILL.md", "e0/alpha/SKILL.md", ), - ("c", "skill://plugin", "skill://plugin/alpha", "c0/alpha"), + ( + SkillSourceKind::Cloud, + "skill://plugin", + "skill://plugin/alpha", + "c0/alpha", + ), ] { - let plan = AliasPlan::build(prefix, &[root, root]).expect("alias plan should build"); + let plan = AliasPlan::build(&source, &[root, root]).expect("alias plan should build"); - assert_eq!(Some(expected.to_string()), plan.shorten(locator)); + let shortened = plan.shorten(locator).expect("known root"); + assert_eq!(shortened, expected); + let json = serde_json::to_string(&shortened).unwrap(); + let parsed: SkillLocator = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed.alias_source(), Some(&source)); + assert_eq!(parsed.as_str(), expected); + assert_eq!(serde_json::to_string(&parsed).unwrap(), json); } } #[test] fn rejects_unknown_roots_and_locators_outside_the_root() { let root = "/skills/shared"; - let plan = AliasPlan::build("r", &[root, root]).expect("alias plan should build"); + let plan = + AliasPlan::build(&SkillSourceKind::Host, &[root, root]).expect("alias plan should build"); assert_eq!(None, plan.shorten("/skills/other/alpha")); assert_eq!(None, plan.shorten("/skills/shared-other/alpha")); @@ -69,13 +84,14 @@ fn rejects_unknown_roots_and_locators_outside_the_root() { #[test] fn returns_none_when_no_roots_are_provided() { - assert!(AliasPlan::build("r", &[]).is_none()); + assert!(AliasPlan::build(&SkillSourceKind::Host, &[]).is_none()); } #[test] fn aliases_roots_used_by_only_one_skill() { let root = "/skills/singleton"; - let plan = AliasPlan::build("r", &[root]).expect("singleton root should produce an alias"); + let plan = AliasPlan::build(&SkillSourceKind::Host, &[root]) + .expect("singleton root should produce an alias"); assert_eq!(vec!["- `r0` = `/skills/singleton`"], plan.root_lines()); assert_eq!( @@ -83,3 +99,31 @@ fn aliases_roots_used_by_only_one_skill() { plan.shorten("/skills/singleton/alpha/SKILL.md") ); } + +#[test] +fn locator_serde_recognizes_aliases_and_preserves_package_strings() { + for (value, expected_source) in [ + ("c0/release", Some(SkillSourceKind::Cloud)), + ("e12/references/details.md", Some(SkillSourceKind::Executor)), + ("r1/release/SKILL.md", Some(SkillSourceKind::Host)), + ("skill://Plugin_123/release", None), + ("/skills/release/SKILL.md", None), + (r"C:\skills\release\SKILL.md", None), + ("cloud/release", None), + ("c/release", None), + ("c00/release", None), + ("e-1/release", None), + ("e+1/release", None), + ("c0", None), + ("c0/", None), + ("/release", None), + ("é0/release", None), + ("e999999999999999999999999999999/release", None), + ] { + let json = serde_json::to_string(value).unwrap(); + let locator: SkillLocator = serde_json::from_str(&json).unwrap(); + assert_eq!(locator.alias_source(), expected_source.as_ref(), "{value}"); + assert_eq!(locator.as_str(), value); + assert_eq!(serde_json::to_string(&locator).unwrap(), json); + } +} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index cfd65d61ba..f94fca18b2 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -985,13 +985,6 @@ pub(crate) fn build_alias_plan(entries: &[&SkillCatalogEntry]) -> Option "r", - SkillSourceKind::Executor => "e", - SkillSourceKind::Cloud => "c", - SkillSourceKind::Custom(_) => return None, - }; - let mut alias_ordered_entries = entries.to_vec(); alias_ordered_entries.sort_by_key(|entry| entry.alias_root_order().unwrap_or(usize::MAX)); let roots = match source { @@ -1005,7 +998,7 @@ pub(crate) fn build_alias_plan(entries: &[&SkillCatalogEntry]) -> Option>(); - AliasPlan::build(prefix, &roots) + AliasPlan::build(source, &roots) } fn render_skill_locator_with_aliases(entry: &SkillCatalogEntry, plan: &AliasPlan) -> String { diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index 65605ae52e..c502173b6f 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -11,7 +11,9 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; +use crate::aliases::SkillLocator; use crate::catalog::SkillResourceId; +use crate::catalog::SkillSourceKind; use crate::provider::MAX_SKILL_RESOURCE_CONTENT_BYTES; use crate::provider::SkillReadContext; use crate::provider::SkillReadRequest; @@ -36,7 +38,8 @@ const TOOL_NAME: &str = "read"; #[derive(Deserialize, JsonSchema)] struct ReadArgs { - package: String, + #[schemars(with = "String")] + package: SkillLocator, resource: Option, cursor: Option, } @@ -79,16 +82,21 @@ impl<'call> ToolExecutor> for ReadTool { Box::pin(async move { let args: ReadArgs = parse_args(&call)?; let response_byte_budget = call.response_byte_budget(MAX_SKILL_RESPONSE_BYTES); - validate_handle("package", &args.package, MAX_HANDLE_BYTES)?; + validate_handle("package", args.package.as_str(), MAX_HANDLE_BYTES)?; if let Some(resource) = args.resource.as_deref() { validate_handle("resource", resource, MAX_HANDLE_BYTES)?; } + let search_authorities: &[_] = match args.package.alias_source() { + Some(SkillSourceKind::Cloud) => &[SkillToolAuthoritySelector::Cloud], + Some(SkillSourceKind::Executor) => &[SkillToolAuthoritySelector::Executor], + Some(SkillSourceKind::Host | SkillSourceKind::Custom(_)) | None => &[ + SkillToolAuthoritySelector::Cloud, + SkillToolAuthoritySelector::Executor, + ], + }; let mut selected_skill = None; - for selector in [ - super::SkillToolAuthoritySelector::Cloud, - super::SkillToolAuthoritySelector::Executor, - ] { + for &selector in search_authorities { let catalog = self.context.catalog(&call.turn_id, selector).await; let alias_plan = build_alias_plan( &catalog @@ -99,11 +107,11 @@ impl<'call> ToolExecutor> for ReadTool { ); if let Some(entry) = catalog.entries.into_iter().find(|entry| { entry.enabled - && (entry.id.0 == args.package + && (entry.id.0 == args.package.as_str() || alias_plan .as_ref() .and_then(|plan| plan.shorten(&entry.id.0)) - .is_some_and(|alias| alias == args.package)) + .is_some_and(|alias| alias == args.package.as_str())) && SkillToolAuthority::from_authority(&entry.authority) .is_some_and(|authority| authority.selector() == selector) }) {