Route skill reads by alias source (#55303)

## Why

Skill aliases already identify their source, but `skills.read` searched the cloud catalog before the executor catalog even for executor aliases.

## What changed

Parse package locators to recognize cloud (`c0/...`), executor (`e0/...`), and host (`r0/...`) aliases. Restrict cloud and executor alias lookups to the matching catalog, while retaining the existing search order for other package strings.

Share the source-to-prefix mapping between alias generation and parsing, and preserve the string representation and tool schema for package locators.

## Testing

Extend alias tests to cover source recognition, JSON round trips, and malformed alias strings, including leading zeros, missing suffixes, non-ASCII prefixes, and overflowing indices.

GitOrigin-RevId: f835e47a244aa0cf9eb95980ea23f4d7fcedd511

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<SkillSourceKind>,
+}
+
+impl From<String> 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::<usize>().ok()?;
+            [
+                SkillSourceKind::Cloud,
+                SkillSourceKind::Executor,
+                SkillSourceKind::Host,
+            ]
+            .into_iter()
+            .find(|source| alias_prefix(source) == Some(prefix))
+        });
+        Self {
+            value,
+            alias_source,
+        }
+    }
+}
+
+impl From<SkillLocator> 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<Self> {
+    pub(crate) fn build(source: &SkillSourceKind, candidates: &[&str]) -> Option<Self> {
+        let prefix = alias_prefix(source)?;
         let mut roots = Vec::new();
         let mut seen = HashSet::new();
 

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);
+    }
+}

codex-rs/ext/skills/src/render.rs

@@ -985,13 +985,6 @@ pub(crate) fn build_alias_plan(entries: &[&SkillCatalogEntry]) -> Option<AliasPl
     if entries.iter().any(|entry| &entry.authority.kind != source) {
         return None;
     }
-    let prefix = match source {
-        SkillSourceKind::Host => "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<AliasPl
     };
     let roots = roots.iter().map(String::as_str).collect::<Vec<_>>();
 
-    AliasPlan::build(prefix, &roots)
+    AliasPlan::build(source, &roots)
 }
 
 fn render_skill_locator_with_aliases(entry: &SkillCatalogEntry, plan: &AliasPlan) -> String {

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<String>,
     cursor: Option<String>,
 }
@@ -79,16 +82,21 @@ impl<'call> ToolExecutor<ToolCall<'call>> 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<ToolCall<'call>> 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)
                 }) {