Change skill mounting behaviour

agent_instructions/containers.md

@@ -160,14 +160,20 @@ extra runtime support is needed.
 `skills` on a Responses-API or `POST /v1/containers` request is a tagged-union list per
 OpenAI's spec:
 
-- `{ "type": "skill_reference", "skill_id": "<uuid>", "version": "latest" }` —
-  resolves to a stored skill via `SkillService::get_by_id_and_org`. `version` accepts
-  `latest` only; anything else rejects with `unsupported_skill_version`.
+- `{ "type": "skill_reference", "skill_id": "<id>", "version": "latest" }` —
+  resolves a stored skill via `resolve_version_for_reference`. `skill_id` is a prefixed
+  id (`skill_…`), a bare UUID, or the skill's name slug. `version` is optional: omit for
+  the **default** version, `latest` for the newest, or a positive integer for that exact
+  version; anything else rejects with `unsupported_skill_version`. Files materialize
+  under `/skills/<name>-<version>/` (e.g. `/skills/csv-insights-1/`), mirroring OpenAI's
+  `<name>-<version>` container layout under Hadrian's `/skills` root.
 - `{ "type": "inline", "name": "...", "description": "...", "source": { "type": "base64",
   "media_type": "text/markdown", "data": "..." } }` — ephemeral. The decoded payload is
-  mounted as a single-file skill under `/skills/skill_inline_<hash>/SKILL.md`. The hash
-  is derived from `(name, content)` so foreground and background lanes mount the inline
-  skill at the same path.
+  mounted as a single-file skill under `/skills/<name>/SKILL.md`. `name` must be a valid
+  lowercase skill slug because it becomes the mount directory (written to the sandbox
+  unsanitized); a non-slug name rejects with `invalid_inline_skill`. The path is derived
+  purely from `name`, so foreground and background lanes mount the inline skill at the
+  same path.
 
 Skills attached at `POST /v1/containers` time are stored verbatim on the row's
 `skill_ids_json` column (the column name predates the typed enum; it now holds the full

openapi/hadrian.openapi.json

@@ -23999,7 +23999,7 @@
             "items": {
               "type": "object"
             },
-            "description": "**Hadrian Extension:** Skills to mount into the shell-tool\nsession. Hadrian treats skills as a top-level request field\nrather than threading them through the OpenAI shell-tool\n`environment` block, so they're accepted on every responses\nrequest regardless of whether the upstream provider has its own\nskills surface.\n\nMirrors OpenAI's typed shape — each entry is either a reference\nto a stored skill or an inline bundle:\n\n```json\n[\n  {\"type\": \"skill_reference\", \"skill_id\": \"skill_…\", \"version\": \"latest\"},\n  {\"type\": \"inline\", \"name\": \"extract-csv\", \"description\": \"...\",\n   \"source\": {\"type\": \"base64\", \"media_type\": \"text/markdown\", \"data\": \"...\"}}\n]\n```\n\nFor `skill_reference`, `skill_id` is a prefixed id (`skill_…`), a bare\nUUID, or the skill's name slug, owned by the caller's org. `version` is\noptional: omit for the skill's **default** version, `\"latest\"` for the\nnewest, or a positive integer for that exact version (default and latest\ncan differ).\n\nFor `inline`, the decoded `source.data` is mounted as an\nephemeral skill bundle: `text/markdown` is treated as the\n`SKILL.md` content; other media types are rejected today.\n\nEach resolved skill's `SKILL.md` is prepended to `instructions`\nso the model knows the skill is available; all skill files are\nmaterialized under `/skills/<skill_id>/` inside the sandbox."
+            "description": "**Hadrian Extension:** Skills to mount into the shell-tool\nsession. Hadrian treats skills as a top-level request field\nrather than threading them through the OpenAI shell-tool\n`environment` block, so they're accepted on every responses\nrequest regardless of whether the upstream provider has its own\nskills surface.\n\nMirrors OpenAI's typed shape — each entry is either a reference\nto a stored skill or an inline bundle:\n\n```json\n[\n  {\"type\": \"skill_reference\", \"skill_id\": \"skill_…\", \"version\": \"latest\"},\n  {\"type\": \"inline\", \"name\": \"extract-csv\", \"description\": \"...\",\n   \"source\": {\"type\": \"base64\", \"media_type\": \"text/markdown\", \"data\": \"...\"}}\n]\n```\n\nFor `skill_reference`, `skill_id` is a prefixed id (`skill_…`), a bare\nUUID, or the skill's name slug, owned by the caller's org. `version` is\noptional: omit for the skill's **default** version, `\"latest\"` for the\nnewest, or a positive integer for that exact version (default and latest\ncan differ).\n\nFor `inline`, the decoded `source.data` is mounted as an\nephemeral skill bundle: `text/markdown` is treated as the\n`SKILL.md` content; other media types are rejected today. The\n`name` must be a lowercase slug — it becomes the mount directory.\n\nEach resolved skill's `SKILL.md` is prepended to `instructions`\nso the model knows the skill is available; all skill files are\nmaterialized under `/skills/<name>-<version>/` (stored skills) or\n`/skills/<name>/` (inline skills) inside the sandbox. The path\nmirrors OpenAI's `<name>-<version>` container layout under\nHadrian's `/skills` root."
           },
           "sovereignty_requirements": {
             "oneOf": [

src/api_types/responses.rs

@@ -1800,8 +1800,8 @@ pub struct SkillReference {
     pub version: Option<String>,
 }
 
-/// Inline skill bundle. Mounted under `/skills/<synthetic-id>/` for
-/// the lifetime of the request (or the container, when supplied at
+/// Inline skill bundle. Mounted under `/skills/<name>/` for the
+/// lifetime of the request (or the container, when supplied at
 /// container creation).
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
@@ -2682,11 +2682,15 @@ pub struct CreateResponsesPayload {
     ///
     /// For `inline`, the decoded `source.data` is mounted as an
     /// ephemeral skill bundle: `text/markdown` is treated as the
-    /// `SKILL.md` content; other media types are rejected today.
+    /// `SKILL.md` content; other media types are rejected today. The
+    /// `name` must be a lowercase slug — it becomes the mount directory.
     ///
     /// Each resolved skill's `SKILL.md` is prepended to `instructions`
     /// so the model knows the skill is available; all skill files are
-    /// materialized under `/skills/<skill_id>/` inside the sandbox.
+    /// materialized under `/skills/<name>-<version>/` (stored skills) or
+    /// `/skills/<name>/` (inline skills) inside the sandbox. The path
+    /// mirrors OpenAI's `<name>-<version>` container layout under
+    /// Hadrian's `/skills` root.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     #[cfg_attr(feature = "utoipa", schema(value_type = Vec<Object>))]
     pub skills: Option<Vec<RequestSkill>>,

src/routes/api/chat.rs

@@ -1998,9 +1998,11 @@ pub async fn api_v1_responses(
         let code = match &e {
             SRE::InvalidId(_) | SRE::NotFound(_) | SRE::MissingOrg => "invalid_skill_reference",
             SRE::UnsupportedVersion(_) => "unsupported_skill_version",
-            SRE::InvalidBase64 { .. } | SRE::InvalidUtf8 { .. } | SRE::EmptyInlineName => {
-                "invalid_inline_skill"
-            }
+            SRE::InvalidBase64 { .. }
+            | SRE::InvalidUtf8 { .. }
+            | SRE::EmptyInlineName
+            | SRE::InvalidInlineName(_) => "invalid_inline_skill",
+            SRE::MountPathConflict { .. } => "skill_mount_conflict",
             SRE::UnsupportedMediaType { .. } => "unsupported_inline_skill_media_type",
             SRE::NoService => "skills_not_configured",
             SRE::Db(_) => "skill_lookup_failed",

src/routes/api/containers.rs

@@ -347,6 +347,17 @@ fn validate_skill_entry(entry: &crate::api_types::RequestSkill) -> Result<(), Ap
                     "inline skill `name` must be non-empty",
                 ));
             }
+            // The name becomes the `/skills/<name>` mount segment, written to
+            // the sandbox verbatim, so it must be a path-safe slug. Mirror the
+            // resolver's check so a bad name is rejected at creation rather than
+            // failing on every later request.
+            if crate::models::validate_skill_name(&inline.name).is_err() {
+                return Err(ApiError::new(
+                    StatusCode::BAD_REQUEST,
+                    "invalid_inline_skill",
+                    format!("inline skill `name` '{}' is not a valid slug", inline.name),
+                ));
+            }
             let crate::api_types::InlineSkillSource::Base64 { media_type, data } = &inline.source;
             if media_type != "text/markdown" {
                 return Err(ApiError::new(

src/runtimes/mod.rs

@@ -194,7 +194,7 @@ pub struct SecretMount {
 #[derive(Debug, Clone)]
 pub struct SkillMount {
     pub skill_id: String,
-    /// Mount path inside the container (e.g. `/skills/<skill_id>`).
+    /// Mount path inside the container (e.g. `/skills/<name>-<version>`).
     pub mount_path: String,
     /// Files to write under `mount_path`. Paths are relative to the
     /// mount point; the adapter is responsible for creating

src/services/responses_pipeline.rs

@@ -147,6 +147,18 @@ pub enum SkillResolutionError {
     InvalidUtf8 { name: String },
     #[error("inline skill name must be non-empty")]
     EmptyInlineName,
+    #[error(
+        "inline skill name `{0}` must be a lowercase slug (1-64 chars of a-z, 0-9, and hyphens; no leading, trailing, or consecutive hyphens)"
+    )]
+    InvalidInlineName(String),
+    #[error(
+        "skills `{first}` and `{second}` both resolve to mount path `{path}`; each skill must mount to a distinct directory (remove the duplicate or rename it)"
+    )]
+    MountPathConflict {
+        path: String,
+        first: String,
+        second: String,
+    },
 }
 
 /// Resolve `payload.skills` to mountable bundles and prepend each
@@ -172,13 +184,20 @@ pub async fn resolve_and_inject_skills(
         return Ok(Vec::new());
     }
 
-    let mut mounts = Vec::with_capacity(skills.len());
-    let mut preamble_sections = Vec::with_capacity(skills.len());
-
+    let mut resolved = Vec::with_capacity(skills.len());
     for entry in skills {
-        let mount = resolve_one_skill(state, org_id, entry).await?;
-        let preamble = mount.build_preamble();
-        preamble_sections.push(preamble);
+        resolved.push(resolve_one_skill(state, org_id, entry).await?);
+    }
+
+    // Two skills resolving to the same directory would clobber each
+    // other's files at mount time (and confuse the model's path hints),
+    // so reject the request rather than silently dropping one.
+    ensure_unique_mount_paths(&resolved)?;
+
+    let mut mounts = Vec::with_capacity(resolved.len());
+    let mut preamble_sections = Vec::with_capacity(resolved.len());
+    for mount in resolved {
+        preamble_sections.push(mount.build_preamble());
         mounts.push(mount.into_skill_mount());
     }
 
@@ -239,6 +258,25 @@ impl ResolvedSkill {
     }
 }
 
+/// Reject a skill set in which two entries resolve to the same mount
+/// directory. The list is tiny (a handful of skills per request), so a
+/// quadratic scan is cheaper than allocating a set.
+fn ensure_unique_mount_paths(resolved: &[ResolvedSkill]) -> Result<(), SkillResolutionError> {
+    for (i, skill) in resolved.iter().enumerate() {
+        if let Some(first) = resolved[..i]
+            .iter()
+            .find(|s| s.mount_path == skill.mount_path)
+        {
+            return Err(SkillResolutionError::MountPathConflict {
+                path: skill.mount_path.clone(),
+                first: first.name.clone(),
+                second: skill.name.clone(),
+            });
+        }
+    }
+    Ok(())
+}
+
 async fn resolve_one_skill(
     state: &AppState,
     org_id: Option<Uuid>,
@@ -281,7 +319,12 @@ async fn resolve_one_skill(
                 .map_err(|e| SkillResolutionError::Db(e.to_string()))?
                 .ok_or_else(|| SkillResolutionError::NotFound(reference.skill_id.clone()))?;
 
-            let mount_path = format!("/skills/{}", version.skill_id);
+            // Mount under `<name>-<version>` to mirror OpenAI's container
+            // layout (`/home/oai/skills/csv-insights-1`); we keep Hadrian's
+            // `/skills` root. The snapshot slug is path-safe (validated at
+            // creation) and the version suffix disambiguates multiple
+            // versions of the same skill mounted in one request.
+            let mount_path = format!("/skills/{}-{}", version.name, version.version_seq);
             let main_content = version
                 .files
                 .iter()
@@ -314,6 +357,12 @@ fn resolve_inline_skill(
     if inline.name.trim().is_empty() {
         return Err(SkillResolutionError::EmptyInlineName);
     }
+    // The name becomes the mount-path segment (`/skills/<name>`), which the
+    // runtime writes to the sandbox verbatim — unlike per-file paths it is
+    // not sanitized downstream. So it must be a path-safe slug, or a name
+    // like `../../etc/...` would escape the skills directory.
+    validate_skill_name(&inline.name)
+        .map_err(|_| SkillResolutionError::InvalidInlineName(inline.name.clone()))?;
     let crate::api_types::InlineSkillSource::Base64 { media_type, data } = &inline.source;
     if media_type != "text/markdown" {
         return Err(SkillResolutionError::UnsupportedMediaType {
@@ -331,17 +380,17 @@ fn resolve_inline_skill(
     let content = String::from_utf8(bytes).map_err(|_| SkillResolutionError::InvalidUtf8 {
         name: inline.name.clone(),
     })?;
-    // Synthetic per-request id derived from the payload hash so the
-    // mount path is stable across foreground/background dispatch but
-    // doesn't collide with stored skill UUIDs.
-    let synthetic_id = inline_skill_synthetic_id(&inline.name, &content);
-    let mount_path = format!("/skills/{synthetic_id}");
+    // Inline skills have no version, so the mount path is just the slug.
+    // Derived purely from `inline.name`, it's stable across the
+    // foreground / background dispatch lanes (both run off the same
+    // payload).
+    let mount_path = format!("/skills/{}", inline.name);
     let files = vec![MountedFile {
         relative_path: SKILL_MAIN_FILE.to_string(),
         content: Bytes::from(content.clone().into_bytes()),
     }];
     Ok(ResolvedSkill {
-        skill_id: synthetic_id,
+        skill_id: inline.name.clone(),
         name: inline.name.clone(),
         description: inline.description.clone(),
         mount_path,
@@ -350,18 +399,6 @@ fn resolve_inline_skill(
     })
 }
 
-/// Synthetic id used as the mount-path segment for inline skills.
-/// Hash-based so the same payload reuses the same path across the
-/// foreground / background lanes (otherwise the response that resumes
-/// from a background tick would mount the skill at a different path).
-fn inline_skill_synthetic_id(name: &str, content: &str) -> String {
-    use std::hash::{DefaultHasher, Hash, Hasher};
-    let mut hasher = DefaultHasher::new();
-    name.hash(&mut hasher);
-    content.hash(&mut hasher);
-    format!("skill_inline_{:016x}", hasher.finish())
-}
-
 /// Wrap a streaming Responses-API response with the full server-side
 /// pipeline: output guardrails, the server-executed tool loop
 /// (`file_search` / `web_search` / `shell`), and the persister.
@@ -880,8 +917,9 @@ mod skill_tests {
             },
         };
         let mount = resolve_inline_skill(&inline).expect("should resolve");
-        assert!(mount.skill_id.starts_with("skill_inline_"));
-        assert_eq!(mount.mount_path, format!("/skills/{}", mount.skill_id));
+        // Inline skills mount under their slug name, no version suffix.
+        assert_eq!(mount.skill_id, "useful");
+        assert_eq!(mount.mount_path, "/skills/useful");
         assert_eq!(mount.files.len(), 1);
         assert_eq!(mount.files[0].relative_path, SKILL_MAIN_FILE);
         assert_eq!(
@@ -910,6 +948,28 @@ mod skill_tests {
         );
     }
 
+    #[test]
+    fn inline_rejects_non_slug_name() {
+        // The name becomes the mount directory, so a path-traversal name
+        // must be rejected before it can escape `/skills/`.
+        let encoded = base64::engine::general_purpose::STANDARD.encode(b"# x");
+        for bad in ["../etc", "Has Space", "UPPER", "trailing-", "a--b"] {
+            let inline = InlineSkill {
+                name: bad.into(),
+                description: "x".into(),
+                source: InlineSkillSource::Base64 {
+                    media_type: "text/markdown".into(),
+                    data: encoded.clone(),
+                },
+            };
+            let err = resolve_inline_skill(&inline).expect_err("should reject");
+            assert!(
+                matches!(err, SkillResolutionError::InvalidInlineName(_)),
+                "name {bad:?} got: {err:?}"
+            );
+        }
+    }
+
     #[test]
     fn inline_rejects_invalid_base64() {
         let inline = InlineSkill {
@@ -924,12 +984,69 @@ mod skill_tests {
         assert!(matches!(err, SkillResolutionError::InvalidBase64 { .. }));
     }
 
+    fn resolved(name: &str, mount_path: &str) -> ResolvedSkill {
+        ResolvedSkill {
+            skill_id: name.into(),
+            name: name.into(),
+            description: "x".into(),
+            mount_path: mount_path.into(),
+            files: Vec::new(),
+            main_content: None,
+        }
+    }
+
+    #[test]
+    fn distinct_mount_paths_are_accepted() {
+        let set = [
+            resolved("foo", "/skills/foo-1"),
+            resolved("foo", "/skills/foo-2"),
+            resolved("bar", "/skills/bar"),
+        ];
+        assert!(ensure_unique_mount_paths(&set).is_ok());
+    }
+
+    #[test]
+    fn colliding_mount_paths_are_rejected() {
+        // A stored skill `foo` v1 and an inline skill literally named
+        // `foo-1` both want `/skills/foo-1`.
+        let set = [
+            resolved("foo", "/skills/foo-1"),
+            resolved("foo-1", "/skills/foo-1"),
+        ];
+        let err = ensure_unique_mount_paths(&set).expect_err("should conflict");
+        match err {
+            SkillResolutionError::MountPathConflict {
+                path,
+                first,
+                second,
+            } => {
+                assert_eq!(path, "/skills/foo-1");
+                assert_eq!(first, "foo");
+                assert_eq!(second, "foo-1");
+            }
+            other => panic!("unexpected error: {other:?}"),
+        }
+    }
+
     #[test]
-    fn inline_synthetic_id_stable_for_same_payload() {
-        let a = inline_skill_synthetic_id("foo", "bar");
-        let b = inline_skill_synthetic_id("foo", "bar");
-        assert_eq!(a, b);
-        let c = inline_skill_synthetic_id("foo", "different");
-        assert_ne!(a, c);
+    fn inline_mount_path_is_name_derived() {
+        // The inline mount path depends only on the name (so the
+        // foreground/background lanes agree), not on the content.
+        let resolve = |name: &str, body: &str| {
+            let encoded = base64::engine::general_purpose::STANDARD.encode(body.as_bytes());
+            resolve_inline_skill(&InlineSkill {
+                name: name.into(),
+                description: "x".into(),
+                source: InlineSkillSource::Base64 {
+                    media_type: "text/markdown".into(),
+                    data: encoded,
+                },
+            })
+            .expect("should resolve")
+            .mount_path
+        };
+        assert_eq!(resolve("foo", "bar"), "/skills/foo");
+        assert_eq!(resolve("foo", "different"), "/skills/foo");
+        assert_ne!(resolve("foo", "bar"), resolve("bar", "bar"));
     }
 }

src/services/shell_tool.rs

@@ -396,8 +396,9 @@ pub struct ShellToolHint {
     /// stdout/stderr fed back to the model are truncated past this many
     /// characters with a head+tail keep.
     pub max_output_chars: usize,
-    /// Names of skill bundles mounted under `/skills/<id>` for this
-    /// request.
+    /// Mount-directory names of skill bundles materialized under
+    /// `/skills/<name>-<version>` (or `/skills/<name>` for inline) for
+    /// this request.
     pub mounted_skill_ids: Vec<String>,
     /// Operator-supplied description of the container environment
     /// (`[features.server_tools.shell_limits].environment_description`):