diff --git a/agent_instructions/containers.md b/agent_instructions/containers.md index 7e2c27c8..a6ea2dfd 100644 --- a/agent_instructions/containers.md +++ b/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": "", "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": "", "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/-/` (e.g. `/skills/csv-insights-1/`), mirroring OpenAI's + `-` 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_/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//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 diff --git a/openapi/hadrian.openapi.json b/openapi/hadrian.openapi.json index c1e03025..c7265c2e 100644 --- a/openapi/hadrian.openapi.json +++ b/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//` 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/-/` (stored skills) or\n`/skills//` (inline skills) inside the sandbox. The path\nmirrors OpenAI's `-` container layout under\nHadrian's `/skills` root." }, "sovereignty_requirements": { "oneOf": [ diff --git a/src/api_types/responses.rs b/src/api_types/responses.rs index ddef0543..c3ee4625 100644 --- a/src/api_types/responses.rs +++ b/src/api_types/responses.rs @@ -1800,8 +1800,8 @@ pub struct SkillReference { pub version: Option, } -/// Inline skill bundle. Mounted under `/skills//` for -/// the lifetime of the request (or the container, when supplied at +/// Inline skill bundle. Mounted under `/skills//` 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//` inside the sandbox. + /// materialized under `/skills/-/` (stored skills) or + /// `/skills//` (inline skills) inside the sandbox. The path + /// mirrors OpenAI's `-` container layout under + /// Hadrian's `/skills` root. #[serde(default, skip_serializing_if = "Option::is_none")] #[cfg_attr(feature = "utoipa", schema(value_type = Vec))] pub skills: Option>, diff --git a/src/routes/api/chat.rs b/src/routes/api/chat.rs index 1bc81095..2fa2feb3 100644 --- a/src/routes/api/chat.rs +++ b/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", diff --git a/src/routes/api/containers.rs b/src/routes/api/containers.rs index a31fc736..d164fbcb 100644 --- a/src/routes/api/containers.rs +++ b/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/` 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( diff --git a/src/runtimes/mod.rs b/src/runtimes/mod.rs index 9d7bea66..093f5040 100644 --- a/src/runtimes/mod.rs +++ b/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/`). + /// Mount path inside the container (e.g. `/skills/-`). pub mount_path: String, /// Files to write under `mount_path`. Paths are relative to the /// mount point; the adapter is responsible for creating diff --git a/src/services/responses_pipeline.rs b/src/services/responses_pipeline.rs index 16b4585d..24d2fe57 100644 --- a/src/services/responses_pipeline.rs +++ b/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, @@ -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 `-` 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/`), 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")); } } diff --git a/src/services/shell_tool.rs b/src/services/shell_tool.rs index 1c656d27..c99ac4bc 100644 --- a/src/services/shell_tool.rs +++ b/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/` for this - /// request. + /// Mount-directory names of skill bundles materialized under + /// `/skills/-` (or `/skills/` for inline) for + /// this request. pub mounted_skill_ids: Vec, /// Operator-supplied description of the container environment /// (`[features.server_tools.shell_limits].environment_description`): diff --git a/ui/src/api/openapi.json b/ui/src/api/openapi.json index c1e03025..c7265c2e 100644 --- a/ui/src/api/openapi.json +++ b/ui/src/api/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//` 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/-/` (stored skills) or\n`/skills//` (inline skills) inside the sandbox. The path\nmirrors OpenAI's `-` container layout under\nHadrian's `/skills` root." }, "sovereignty_requirements": { "oneOf": [