From a2521d6d07904804ec38d202fc09b690136763f3 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Thu, 27 Aug 2026 09:52:31 +0300 Subject: [PATCH 1/4] feat(skills): add read-side resource for DIAL Core /v2/skills Adds Skills/AsyncSkills covering the four read operations of Core's folder-as-resource API (epam/ai-dial-core#1633): get_metadata GET /v2/metadata/skills/{bucket}/{path} list_files GET /v2/metadata/skills/{bucket}/{path}/files[/{sub}] get_file GET /v2/skills/{bucket}/{path}/files/{filePath} download GET /v2/skills/{bucket}/{path} (application/zip) AsyncSkills also exposes stream_file/stream_download; the sync client has no stream method, mirroring Files. Groundwork, since the library assumed /v1 throughout: - API_V2_PREFIX and METADATA_V2_PREFIX constants - V2StorageResourceType joins the parser's resource-type union, and DialStorageResourceMixin gains a per-resource api_prefix - opt-in allow_bucket_root so "skills/{bucket}" parses, which Core's children listing accepts as an empty {path}; it stays opt-in because a two-segment path is ambiguous with "files/my-file.txt" - api_v2_url property, my_skills_home(), Skills wired into both clients file_path and path are validated before being concatenated onto the already-parsed api path, which never goes back through the url parser: "." and ".." segments and encoded separators are rejected. Segments are checked as they decode, since _percent_encode_relative_url normalizes with unquote before quoting, so "%2e%2e" would otherwise reach urljoin as ".." and retarget the request at another bucket. Also folds the byte-identical _files_error_processor and _prompts_error_processor into a shared storage_error_processor. Closes #136 Refs #135 --- README.md | 165 ++++++++ aidial_client/_client.py | 19 + aidial_client/_constants.py | 4 + aidial_client/helpers/storage_resource.py | 109 ++++- aidial_client/resources/__init__.py | 3 + aidial_client/resources/files.py | 46 +- aidial_client/resources/prompts.py | 38 +- aidial_client/resources/skills.py | 396 ++++++++++++++++++ aidial_client/types/metadata.py | 38 +- tests/helpers/test_storage_resource_parser.py | 89 ++++ tests/resources/skills/test_skill_download.py | 339 +++++++++++++++ tests/resources/skills/test_skill_metadata.py | 266 ++++++++++++ 12 files changed, 1438 insertions(+), 74 deletions(-) create mode 100644 aidial_client/resources/skills.py create mode 100644 tests/resources/skills/test_skill_download.py create mode 100644 tests/resources/skills/test_skill_metadata.py diff --git a/README.md b/README.md index 73a432d..d61e555 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,11 @@ - [Get Prompt](#get-prompt) - [Get Prompt Metadata](#get-prompt-metadata) - [Delete Prompt](#delete-prompt) + - [Skills](#skills) + - [Listing Skills](#listing-skills) + - [Listing Files in a Skill](#listing-files-in-a-skill) + - [Reading a File from a Skill](#reading-a-file-from-a-skill) + - [Downloading a Skill](#downloading-a-skill) - [Applications](#applications) - [List Applications](#list-applications) - [Get Application by Id](#get-application-by-id) @@ -795,6 +800,166 @@ client.prompts.delete("prompts/my-bucket/my-folder/my-prompt") await async_client.prompts.delete("prompts/my-bucket/my-folder/my-prompt") ``` +### Skills + +A DIAL *skill* is a folder-shaped resource served by DIAL Core's `/v2/skills` +API: a mandatory `SKILL.md` manifest plus an arbitrary hierarchy of bundled +files, addressed as a unit at `skills/{bucket}/{path}`. + +> [!NOTE] +> The `/v2/skills` endpoints are marked as preview in DIAL Core, so their +> contract may still change. The client currently supports the read +> operations; writes are tracked separately. + +#### Listing Skills + +Use `get_metadata()` to list the skills and grouping folders at a location. +Pass `my_skills_home()` to list the bucket root: + +```python +# Sync +listing = client.skills.get_metadata(client.my_skills_home()) +# Async +listing = await async_client.skills.get_metadata( + await async_client.my_skills_home() +) + +for item in listing.items or []: + # "ITEM" is a skill, "FOLDER" is a grouping folder + print(item.node_type, item.url) +``` + +Example of the response: + +```python +SkillMetadata( + name="writing", + parent_path=None, + bucket="my-bucket", + url="skills/my-bucket/writing/", + node_type="FOLDER", + resource_type="SKILL", + next_token=None, + items=[ + SkillItem( + name="tone-of-voice", + parent_path="writing", + bucket="my-bucket", + url="skills/my-bucket/writing/tone-of-voice", + node_type="ITEM", + resource_type="SKILL", + created_at=1724836229736, + updated_at=1724836248936, + author="user@example.com", + etag=None, + ) + ], +) +``` + +> [!NOTE] +> DIAL Core builds this listing without reading each skill's marker, so +> `etag` is always `None` here and the skill's `name`/`description` from +> `SKILL.md` are not included. Read `SKILL.md` itself if you need them. + +#### Listing Files in a Skill + +Use `list_files()` to enumerate what a skill contains. A page may hold fewer +entries than `limit`, so follow `next_token` until it is `None`: + +```python +skill = "skills/my-bucket/writing/tone-of-voice" + +token = None +while True: + page = client.skills.list_files( + skill, recursive=True, limit=1000, token=token + ) + for item in page.items or []: + print(item.url, item.etag) + token = page.next_token + if token is None: + break +``` + +Scope the listing to a subfolder with `path`: + +```python +page = await async_client.skills.list_files(skill, path="references") +``` + +Example of the response: + +```python +SkillFileMetadata( + name="files", + parent_path="writing/tone-of-voice", + bucket="my-bucket", + url="skills/my-bucket/writing/tone-of-voice/files/", + node_type="FOLDER", + resource_type="SKILL", + next_token=None, + items=[ + SkillFileItem( + name="SKILL.md", + parent_path="writing/tone-of-voice/files", + bucket="my-bucket", + url="skills/my-bucket/writing/tone-of-voice/files/SKILL.md", + node_type="ITEM", + resource_type="SKILL", + etag="9749fad13d6e7092a6337c4af9d83764", + updated_at=1724836248936, + ) + ], +) +``` + +#### Reading a File from a Skill + +Use `get_file()` with a path relative to the skill root: + +```python +# Sync +manifest = client.skills.get_file(skill, "SKILL.md") +print(manifest.get_content().decode()) + +# Async +manifest = await async_client.skills.get_file(skill, "SKILL.md") +schema = await async_client.skills.get_file( + skill, "references/api-schema.md" +) +await schema.awrite_to("api-schema.md") +``` + +The async client can stream instead, which avoids holding the file in memory: + +```python +async with async_client.skills.stream_file(skill, "assets/logo.png") as file: + await file.awrite_to("logo.png") +``` + +#### Downloading a Skill + +Use `download()` to fetch the whole skill as a ZIP archive: + +```python +# Sync +archive = client.skills.download(skill) +archive.write_to("tone-of-voice.zip") + +# Async, streamed +async with async_client.skills.stream_download(skill) as archive: + await archive.awrite_to("tone-of-voice.zip") +``` + +DIAL Core sends no `Content-Disposition` for this endpoint, so `filename` +defaults to the skill name with a `.zip` suffix (`tone-of-voice.zip` above). +The response's `ETag` header carries the skill's aggregate etag: + +```python +etag = archive.headers["etag"] +``` + ### Applications #### List Applications diff --git a/aidial_client/_client.py b/aidial_client/_client.py index 03eb914..496668b 100644 --- a/aidial_client/_client.py +++ b/aidial_client/_client.py @@ -16,6 +16,7 @@ ) from aidial_client._constants import ( API_PREFIX, + API_V2_PREFIX, DEFAULT_MAX_RETRIES, DEFAULT_TIMEOUT, OPENAI_PREFIX, @@ -73,6 +74,10 @@ def is_dial_url(self, absolute_url: str) -> bool: def api_url(self) -> str: return urljoin(self._base_url, API_PREFIX) + @property + def api_v2_url(self) -> str: + return urljoin(self._base_url, API_V2_PREFIX) + @property def base_url(self) -> str: return self._base_url @@ -110,6 +115,10 @@ def _init_resources(self) -> None: metadata=self.metadata, dial_api_url=self.api_url, ) + self.skills = resources.Skills( + http_client=self._http_client, + dial_api_url=self.api_v2_url, + ) self.deployments = resources.Deployments(http_client=self._http_client) self.application = resources.Application(http_client=self._http_client) self.toolset = resources.Toolset(http_client=self._http_client) @@ -149,6 +158,9 @@ def my_conversations_home(self) -> PurePosixPath: def my_prompts_home(self) -> PurePosixPath: return "prompts" / PurePosixPath(self.my_bucket()) + def my_skills_home(self) -> PurePosixPath: + return "skills" / PurePosixPath(self.my_bucket()) + def _get_my_appdata(self) -> AppData | None: return self.bucket.get_appdata() @@ -211,6 +223,10 @@ def _init_resources(self) -> None: metadata=self.metadata, dial_api_url=self.api_url, ) + self.skills = resources.AsyncSkills( + http_client=self._http_client, + dial_api_url=self.api_v2_url, + ) self.deployments = resources.AsyncDeployments( http_client=self._http_client ) @@ -254,6 +270,9 @@ async def my_conversations_home(self) -> PurePosixPath: async def my_prompts_home(self) -> PurePosixPath: return "prompts" / PurePosixPath(await self.my_bucket()) + async def my_skills_home(self) -> PurePosixPath: + return "skills" / PurePosixPath(await self.my_bucket()) + async def _get_my_appdata(self) -> AppData | None: return await self.bucket.get_appdata() diff --git a/aidial_client/_constants.py b/aidial_client/_constants.py index bdfc9c6..dcf6fa3 100644 --- a/aidial_client/_constants.py +++ b/aidial_client/_constants.py @@ -13,6 +13,10 @@ METADATA_PREFIX = urljoin(API_PREFIX, "metadata/") FILES_PREFIX = urljoin(API_PREFIX, "files/") +# DIAL Core exposes folder-shaped resources (agent skills) under /v2. +API_V2_PREFIX = "v2/" +METADATA_V2_PREFIX = urljoin(API_V2_PREFIX, "metadata/") + OPENAI_PREFIX = "openai/" APPLICATION_PREFIX = urljoin(OPENAI_PREFIX, "applications/") diff --git a/aidial_client/helpers/storage_resource.py b/aidial_client/helpers/storage_resource.py index d34075a..603894a 100644 --- a/aidial_client/helpers/storage_resource.py +++ b/aidial_client/helpers/storage_resource.py @@ -2,14 +2,28 @@ from typing import Literal, cast, get_args from urllib.parse import quote, unquote, urljoin, urlparse, urlsplit +import httpx + from aidial_client._compatibility.pydantic_v1 import BaseModel from aidial_client._constants import API_PREFIX -from aidial_client._exception import InvalidDialURLError, NotDialURLError +from aidial_client._exception import ( + DialException, + EtagMismatchError, + InvalidDialURLError, + NotDialURLError, + ResourceNotFoundError, +) from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none from aidial_client.helpers._url import enforce_trailing_slash StorageResourceType = Literal["files", "conversations", "prompts"] +"""Resource types served by the /v1 storage API.""" + +V2StorageResourceType = Literal["skills"] +"""Folder-shaped resource types served by the /v2 API.""" + +AnyStorageResourceType = StorageResourceType | V2StorageResourceType def _percent_encode_relative_url(url: str) -> str: @@ -28,12 +42,30 @@ def _percent_encode_relative_url(url: str) -> str: return "/".join(quote(unquote(seg), safe="") for seg in segments) +def storage_error_processor( + http_status_error: httpx.HTTPStatusError, +) -> DialException | None: + """ + Translate the status codes DIAL storage endpoints use for optimistic + concurrency and absence into the typed exception hierarchy. + """ + if http_status_error.response.status_code == 412: + return EtagMismatchError( + message=http_status_error.response.text, + ) + elif http_status_error.response.status_code == 404: + return ResourceNotFoundError( + message=http_status_error.response.text, + ) + return None + + def _is_directory(s: str) -> bool: return s[-1] == "/" class DialStorageResource(BaseModel): - resource_type: StorageResourceType + resource_type: AnyStorageResourceType """Bucket name, like 'my-bucket'""" bucket: str @@ -47,7 +79,10 @@ class DialStorageResource(BaseModel): """Path without api prefix, like 'files/my-bucket/my-folder/my-file.txt'""" api_path: str - """Path without bucket, like my-folder/'my-file.txt'""" + """ + Path without bucket, like 'my-folder/my-file.txt' + Empty string when the URL points at the bucket root + """ bucket_path: str """ @@ -61,17 +96,24 @@ def safe_parse_storage_resource( *, url: str, dial_api_url: str, - expected_resource_type: StorageResourceType | None = None, + expected_resource_type: AnyStorageResourceType | None = None, + api_prefix: str = API_PREFIX, + allow_bucket_root: bool = False, ) -> DialStorageResource | NotDialURLError | InvalidDialURLError: """ Parse the storage resource from the URL, that could be 1. Absolute: "https://dial.core/v1/files/my-bucket/my-file.txt" 2. Relative to API prefix: "files/my-bucket/my-file.txt" + + ``allow_bucket_root`` accepts a bucket-root URL like "skills/my-bucket". + It is opt-in because a two-segment path is ambiguous: "files/my-file.txt" + has the same shape and is a missing-bucket error. Only callers whose + endpoint accepts an empty path (DIAL Core's v2 metadata listing) enable it. """ dial_api_url = enforce_trailing_slash(dial_api_url) if url.startswith("/"): return InvalidDialURLError(f"Root-relative URL is forbidden: {url}") - if url.startswith(API_PREFIX): + if url.startswith(api_prefix): return InvalidDialURLError( f"API prefix as relative part is not allowed: {url}" ) @@ -90,10 +132,17 @@ def safe_parse_storage_resource( f" DIAL API URL {dial_api_parsed.path}" ) + # "{resource_type}/{bucket}" is the shortest addressable path. + if len(api_path.parents) < 2: + return InvalidDialURLError(f"Missing bucket in URL: {url}") + resource_path = api_path.parents[len(api_path.parents) - 2] parsed_resource_type = str(resource_path) - if parsed_resource_type not in get_args(StorageResourceType): + if parsed_resource_type not in ( + *get_args(StorageResourceType), + *get_args(V2StorageResourceType), + ): return InvalidDialURLError( f"Invalid resource type: {parsed_resource_type}" ) @@ -108,11 +157,22 @@ def safe_parse_storage_resource( ) if len(api_path.parents) < 3: - return InvalidDialURLError(f"Missing bucket in URL: {url}") + if not allow_bucket_root: + return InvalidDialURLError(f"Missing bucket in URL: {url}") + # The URL is "{resource_type}/{bucket}" — the bucket itself. + return DialStorageResource( + resource_type=cast(AnyStorageResourceType, parsed_resource_type), + absolute_url=absolute_url, + api_path=str(api_path), + bucket=api_path.name, + bucket_path="", + relative_url=str(url_path), + filename=None, + ) bucket_path = api_path.parents[len(api_path.parents) - 3] return DialStorageResource( - resource_type=cast(StorageResourceType, parsed_resource_type), + resource_type=cast(AnyStorageResourceType, parsed_resource_type), absolute_url=absolute_url, api_path=str(api_path), bucket=str(bucket_path.relative_to(resource_path)), @@ -126,12 +186,16 @@ def parse_storage_resource( *, url: str, dial_api_url: str, - expected_resource_type: StorageResourceType | None = None, + expected_resource_type: AnyStorageResourceType | None = None, + api_prefix: str = API_PREFIX, + allow_bucket_root: bool = False, ) -> DialStorageResource: result = safe_parse_storage_resource( url=url, dial_api_url=dial_api_url, expected_resource_type=expected_resource_type, + api_prefix=api_prefix, + allow_bucket_root=allow_bucket_root, ) if isinstance(result, NotDialURLError | InvalidDialURLError): raise result @@ -144,18 +208,26 @@ class DialStorageResourceMixin(BaseModel): - /v1/files - /v1/conversations - /v1/prompts + - /v2/skills """ - resource_type: StorageResourceType + resource_type: AnyStorageResourceType dial_api_url: str + api_prefix: str = API_PREFIX def get_storage_resource( - self, url: str | PurePosixPath + self, + url: str | PurePosixPath, + *, + allow_bucket_root: bool = False, ) -> DialStorageResource: """ Get the storage resource object from the URL Args: url (str | PurePosixPath): The URL to be processed. + allow_bucket_root (bool): Accept a bucket-root URL such as + "skills/my-bucket". Off by default, since a two-segment path + is otherwise a missing-bucket error. Returns: DialStorageResource: The storage resource object """ @@ -163,14 +235,23 @@ def get_storage_resource( url=str(url), dial_api_url=self.dial_api_url, expected_resource_type=self.resource_type, + api_prefix=self.api_prefix, + allow_bucket_root=allow_bucket_root, ) - def get_api_path(self, url: str | PurePosixPath) -> str: + def get_api_path( + self, + url: str | PurePosixPath, + *, + allow_bucket_root: bool = False, + ) -> str: """ Convert URL, that could relative or absolute, to relative, percent-encoded API path. """ - return self.get_storage_resource(url).api_path + return self.get_storage_resource( + url, allow_bucket_root=allow_bucket_root + ).api_path def get_display_name(self, url: str | PurePosixPath) -> str: """ @@ -190,7 +271,7 @@ def _prepare_download_request( options = FinalRequestOptions( method="GET", - url=urljoin(API_PREFIX, storage_resource.api_path), + url=urljoin(self.api_prefix, storage_resource.api_path), headers=remove_none( { "If-Match": etag_if_match, diff --git a/aidial_client/resources/__init__.py b/aidial_client/resources/__init__.py index 170b700..0c86271 100644 --- a/aidial_client/resources/__init__.py +++ b/aidial_client/resources/__init__.py @@ -17,6 +17,7 @@ from .chat import AsyncChat, Chat from .files import AsyncFiles, Files from .prompts import AsyncPrompts, Prompts +from .skills import AsyncSkills, Skills __all__ = [ "Chat", @@ -27,6 +28,8 @@ "AsyncFiles", "Prompts", "AsyncPrompts", + "Skills", + "AsyncSkills", "AsyncDeployments", "Deployments", "AsyncMetadata", diff --git a/aidial_client/resources/files.py b/aidial_client/resources/files.py index c7133ac..a83f835 100644 --- a/aidial_client/resources/files.py +++ b/aidial_client/resources/files.py @@ -7,18 +7,16 @@ import httpx from aidial_client._constants import API_PREFIX -from aidial_client._exception import ( - DialException, - EtagMismatchError, - ResourceNotFoundError, -) from aidial_client._internal_types._generic import NoneType from aidial_client._internal_types._http_request import ( FileTypes, FinalRequestOptions, ) from aidial_client._utils._dict import remove_none -from aidial_client.helpers.storage_resource import DialStorageResourceMixin +from aidial_client.helpers.storage_resource import ( + DialStorageResourceMixin, + storage_error_processor, +) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata from aidial_client.types.file import FileDownloadResponse @@ -38,20 +36,6 @@ def _move_copy_body( } -def _files_error_processor( - http_status_error: httpx.HTTPStatusError, -) -> DialException | None: - if http_status_error.response.status_code == 412: - return EtagMismatchError( - message=http_status_error.response.text, - ) - elif http_status_error.response.status_code == 404: - return ResourceNotFoundError( - message=http_status_error.response.text, - ) - return None - - class Files(Resource, DialStorageResourceMixin): metadata: Metadata resource_type: str = "files" @@ -76,7 +60,7 @@ def upload( } ), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) def download( @@ -88,7 +72,7 @@ def download( response = self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @@ -108,7 +92,7 @@ def delete( } ), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) def move_to( @@ -124,7 +108,7 @@ def move_to( url=urljoin(API_PREFIX, "ops/resource/move"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) def copy_to( @@ -140,7 +124,7 @@ def copy_to( url=urljoin(API_PREFIX, "ops/resource/copy"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) def get_metadata( @@ -182,7 +166,7 @@ async def upload( } ), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) async def download( @@ -194,7 +178,7 @@ async def download( response = await self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @@ -207,7 +191,7 @@ async def stream_download( options, filename = self._prepare_download_request(url, etag_if_match) async with self.http_client.stream( options=options, - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) as response: yield FileDownloadResponse(response=response, filename=filename) @@ -227,7 +211,7 @@ async def delete( } ), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) async def move_to( @@ -243,7 +227,7 @@ async def move_to( url=urljoin(API_PREFIX, "ops/resource/move"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) async def copy_to( @@ -259,7 +243,7 @@ async def copy_to( url=urljoin(API_PREFIX, "ops/resource/copy"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=_files_error_processor, + on_http_error=storage_error_processor, ) async def get_metadata( diff --git a/aidial_client/resources/prompts.py b/aidial_client/resources/prompts.py index 7aa6955..be16626 100644 --- a/aidial_client/resources/prompts.py +++ b/aidial_client/resources/prompts.py @@ -2,39 +2,21 @@ from typing import Any, Literal from urllib.parse import urljoin -import httpx - from aidial_client._compatibility.pydantic import PYDANTIC_V2 from aidial_client._constants import API_PREFIX -from aidial_client._exception import ( - DialException, - EtagMismatchError, - ResourceNotFoundError, -) from aidial_client._internal_types._generic import NoneType from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none -from aidial_client.helpers.storage_resource import DialStorageResourceMixin +from aidial_client.helpers.storage_resource import ( + DialStorageResourceMixin, + storage_error_processor, +) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata from aidial_client.types.metadata import PromptItem, PromptMetadata from aidial_client.types.prompt import Prompt -def _prompts_error_processor( - http_status_error: httpx.HTTPStatusError, -) -> DialException | None: - if http_status_error.response.status_code == 412: - return EtagMismatchError( - message=http_status_error.response.text, - ) - elif http_status_error.response.status_code == 404: - return ResourceNotFoundError( - message=http_status_error.response.text, - ) - return None - - def _prompt_to_json(prompt: Prompt) -> dict[str, Any]: if PYDANTIC_V2: return prompt.model_dump(by_alias=True) # type: ignore @@ -65,7 +47,7 @@ def save( } ), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) def get(self, url: str | PurePosixPath) -> Prompt: @@ -76,7 +58,7 @@ def get(self, url: str | PurePosixPath) -> Prompt: method="GET", url=urljoin(API_PREFIX, self.get_api_path(url)), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) def delete( @@ -95,7 +77,7 @@ def delete( } ), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: @@ -129,7 +111,7 @@ async def save( } ), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) async def get(self, url: str | PurePosixPath) -> Prompt: @@ -140,7 +122,7 @@ async def get(self, url: str | PurePosixPath) -> Prompt: method="GET", url=urljoin(API_PREFIX, self.get_api_path(url)), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) async def delete( @@ -159,7 +141,7 @@ async def delete( } ), ), - on_http_error=_prompts_error_processor, + on_http_error=storage_error_processor, ) async def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: diff --git a/aidial_client/resources/skills.py b/aidial_client/resources/skills.py new file mode 100644 index 0000000..c42cc5b --- /dev/null +++ b/aidial_client/resources/skills.py @@ -0,0 +1,396 @@ +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from pathlib import PurePosixPath +from urllib.parse import unquote, urljoin + +import httpx + +from aidial_client._constants import API_V2_PREFIX, METADATA_V2_PREFIX +from aidial_client._exception import InvalidDialURLError +from aidial_client._internal_types._http_request import FinalRequestOptions +from aidial_client._utils._dict import remove_none +from aidial_client.helpers.storage_resource import ( + DialStorageResourceMixin, + _percent_encode_relative_url, + storage_error_processor, +) +from aidial_client.resources.base import AsyncResource, Resource +from aidial_client.types.file import FileDownloadResponse +from aidial_client.types.metadata import SkillFileMetadata, SkillMetadata + +# DIAL Core reserves this path segment to keep the +# ".../{path}/files/{filePath}" grammar unambiguous. +FILES_SEGMENT = "files" + + +def _relative_path_segments(path: str, param: str) -> list[str]: + """ + Validate a path relative to the skill root and split it into segments. + + Unlike the ``url`` argument, this path is concatenated onto an + already-parsed api path and never goes back through the url parser, so + nothing else would catch a traversal segment. ``urljoin`` resolves "." and + ".." while building the request, which shifts the bucket segment: a + ``file_path`` of "../../../other-bucket/their-skill/files/SKILL.md" turns a + validated "skills/my-bucket/my-skill" into a request against + ``other-bucket``. Reject those segments instead. + + Each segment is checked *as it will decode*, because + ``_percent_encode_relative_url`` normalizes with ``unquote`` before quoting: + "%2e%2e" would otherwise pass a literal check here and still reach + ``urljoin`` as "..", and "%2f" would smuggle in a separator. + """ + if not path.strip(): + raise InvalidDialURLError(f"{param} must not be empty") + if path.startswith("/"): + raise InvalidDialURLError( + f"{param} must be relative to the skill root, got: {path}" + ) + + segments = path.split("/") + decoded = [unquote(segment) for segment in segments] + if any(segment in (".", "..") for segment in decoded): + raise InvalidDialURLError( + f'"." and ".." are not allowed in {param}, got: {path}' + ) + if any("/" in segment for segment in decoded): + raise InvalidDialURLError( + f"An encoded path separator is not allowed in {param}, got: {path}" + ) + # A trailing slash is allowed (it denotes a folder) but an interior empty + # segment is a malformed path. + if any(segment == "" for segment in segments[:-1]): + raise InvalidDialURLError(f"Empty path segment in {param}, got: {path}") + return segments + + +class SkillsMixin(DialStorageResourceMixin): + """ + URL and request shaping shared by the sync and async skills resources. + + A skill is a folder-shaped resource: the whole skill is addressed as a unit + at "skills/{bucket}/{path}", and its bundled files hang off + "skills/{bucket}/{path}/files/{filePath}". + """ + + resource_type: str = "skills" + api_prefix: str = API_V2_PREFIX + + def _files_path( + self, url: str | PurePosixPath, path: str | None = None + ) -> str: + api_path = f"{self.get_api_path(url)}/{FILES_SEGMENT}" + # None is the "unset" signal; "" goes through the same validation as + # file_path so the two entry points agree. + if path is None: + return api_path + + segments = _relative_path_segments(path, "path") + if segments[-1] == "": + # Scoping to a folder - drop the trailing empty segment. + segments = segments[:-1] + if not segments: + return api_path + relative = _percent_encode_relative_url("/".join(segments)) + return f"{api_path}/{relative}" + + @staticmethod + def _listing_params( + limit: int | None, + token: str | None, + recursive: bool | None, + ) -> dict[str, object]: + return remove_none( + {"limit": limit, "token": token, "recursive": recursive} + ) + + def _prepare_metadata_request( + self, + url: str | PurePosixPath, + *, + limit: int | None, + token: str | None, + recursive: bool | None, + ) -> FinalRequestOptions: + # Core lists the bucket root when {path} is empty, so a bucket-root + # url ("skills/my-bucket") is a valid target here. + api_path = self.get_api_path(url, allow_bucket_root=True) + return FinalRequestOptions( + method="GET", + url=urljoin(METADATA_V2_PREFIX, api_path), + params=self._listing_params(limit, token, recursive), + ) + + def _prepare_list_files_request( + self, + url: str | PurePosixPath, + *, + path: str | None, + limit: int | None, + token: str | None, + recursive: bool | None, + ) -> FinalRequestOptions: + return FinalRequestOptions( + method="GET", + url=urljoin(METADATA_V2_PREFIX, self._files_path(url, path)), + params=self._listing_params(limit, token, recursive), + ) + + def _prepare_get_file_request( + self, + url: str | PurePosixPath, + file_path: str, + etag_if_match: str | None, + ) -> tuple[FinalRequestOptions, str]: + segments = _relative_path_segments(file_path, "file_path") + if segments[-1] == "": + raise InvalidDialURLError( + f"file_path points to a directory, not a file: {file_path}" + ) + + relative = _percent_encode_relative_url("/".join(segments)) + api_path = f"{self.get_api_path(url)}/{FILES_SEGMENT}/{relative}" + options = FinalRequestOptions( + method="GET", + url=urljoin(API_V2_PREFIX, api_path), + headers=remove_none({"If-Match": etag_if_match}), + ) + return options, unquote(segments[-1]) + + def _prepare_download_archive_request( + self, + url: str | PurePosixPath, + etag_if_match: str | None, + ) -> tuple[FinalRequestOptions, str]: + api_path = self.get_api_path(url) + options = FinalRequestOptions( + method="GET", + url=urljoin(API_V2_PREFIX, api_path), + headers=remove_none({"If-Match": etag_if_match}), + ) + # Core answers application/zip without a Content-Disposition header, + # so name the archive after the skill. + filename = f"{unquote(PurePosixPath(api_path).name)}.zip" + return options, filename + + +class Skills(Resource, SkillsMixin): + def get_metadata( + self, + url: str | PurePosixPath, + *, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SkillMetadata: + """ + List the skills and grouping folders at ``url``. + + Pass a bucket-root url (``client.my_skills_home()``) to list the whole + bucket. Follow ``next_token`` until it is ``None`` to read every page. + """ + return self.http_client.request( + cast_to=SkillMetadata, + options=self._prepare_metadata_request( + url, limit=limit, token=token, recursive=recursive + ), + on_http_error=storage_error_processor, + ) + + def list_files( + self, + url: str | PurePosixPath, + *, + path: str | None = None, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SkillFileMetadata: + """ + List the files of the skill at ``url``, optionally scoped to the + ``path`` subfolder inside it. + + A page may hold fewer entries than ``limit``, so follow ``next_token`` + until it is ``None`` rather than assuming a single page is complete. + """ + return self.http_client.request( + cast_to=SkillFileMetadata, + options=self._prepare_list_files_request( + url, + path=path, + limit=limit, + token=token, + recursive=recursive, + ), + on_http_error=storage_error_processor, + ) + + def get_file( + self, + url: str | PurePosixPath, + file_path: str, + etag_if_match: str | None = None, + ) -> FileDownloadResponse: + """ + Download a single file bundled in the skill at ``url``. + + ``file_path`` is relative to the skill root, e.g. "SKILL.md" or + "references/api-schema.md". + """ + options, filename = self._prepare_get_file_request( + url, file_path, etag_if_match + ) + response = self.http_client.request( + cast_to=httpx.Response, + options=options, + on_http_error=storage_error_processor, + ) + return FileDownloadResponse(response=response, filename=filename) + + def download( + self, + url: str | PurePosixPath, + etag_if_match: str | None = None, + ) -> FileDownloadResponse: + """ + Download the whole skill at ``url`` as a ZIP archive. + """ + options, filename = self._prepare_download_archive_request( + url, etag_if_match + ) + response = self.http_client.request( + cast_to=httpx.Response, + options=options, + on_http_error=storage_error_processor, + ) + return FileDownloadResponse(response=response, filename=filename) + + +class AsyncSkills(AsyncResource, SkillsMixin): + async def get_metadata( + self, + url: str | PurePosixPath, + *, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SkillMetadata: + """ + List the skills and grouping folders at ``url``. + + Pass a bucket-root url (``await client.my_skills_home()``) to list the + whole bucket. Follow ``next_token`` until it is ``None`` to read every + page. + """ + return await self.http_client.request( + cast_to=SkillMetadata, + options=self._prepare_metadata_request( + url, limit=limit, token=token, recursive=recursive + ), + on_http_error=storage_error_processor, + ) + + async def list_files( + self, + url: str | PurePosixPath, + *, + path: str | None = None, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SkillFileMetadata: + """ + List the files of the skill at ``url``, optionally scoped to the + ``path`` subfolder inside it. + + A page may hold fewer entries than ``limit``, so follow ``next_token`` + until it is ``None`` rather than assuming a single page is complete. + """ + return await self.http_client.request( + cast_to=SkillFileMetadata, + options=self._prepare_list_files_request( + url, + path=path, + limit=limit, + token=token, + recursive=recursive, + ), + on_http_error=storage_error_processor, + ) + + async def get_file( + self, + url: str | PurePosixPath, + file_path: str, + etag_if_match: str | None = None, + ) -> FileDownloadResponse: + """ + Download a single file bundled in the skill at ``url``. + + ``file_path`` is relative to the skill root, e.g. "SKILL.md" or + "references/api-schema.md". + """ + options, filename = self._prepare_get_file_request( + url, file_path, etag_if_match + ) + response = await self.http_client.request( + cast_to=httpx.Response, + options=options, + on_http_error=storage_error_processor, + ) + return FileDownloadResponse(response=response, filename=filename) + + @asynccontextmanager + async def stream_file( + self, + url: str | PurePosixPath, + file_path: str, + etag_if_match: str | None = None, + ) -> AsyncIterator[FileDownloadResponse]: + """ + Stream a single file bundled in the skill at ``url``. + """ + options, filename = self._prepare_get_file_request( + url, file_path, etag_if_match + ) + async with self.http_client.stream( + options=options, + on_http_error=storage_error_processor, + ) as response: + yield FileDownloadResponse(response=response, filename=filename) + + async def download( + self, + url: str | PurePosixPath, + etag_if_match: str | None = None, + ) -> FileDownloadResponse: + """ + Download the whole skill at ``url`` as a ZIP archive. + """ + options, filename = self._prepare_download_archive_request( + url, etag_if_match + ) + response = await self.http_client.request( + cast_to=httpx.Response, + options=options, + on_http_error=storage_error_processor, + ) + return FileDownloadResponse(response=response, filename=filename) + + @asynccontextmanager + async def stream_download( + self, + url: str | PurePosixPath, + etag_if_match: str | None = None, + ) -> AsyncIterator[FileDownloadResponse]: + """ + Stream the whole skill at ``url`` as a ZIP archive. + """ + options, filename = self._prepare_download_archive_request( + url, etag_if_match + ) + async with self.http_client.stream( + options=options, + on_http_error=storage_error_processor, + ) as response: + yield FileDownloadResponse(response=response, filename=filename) diff --git a/aidial_client/types/metadata.py b/aidial_client/types/metadata.py index d188d87..0711dee 100644 --- a/aidial_client/types/metadata.py +++ b/aidial_client/types/metadata.py @@ -22,7 +22,7 @@ class Config: bucket: str url: str node_type: Literal["FOLDER", "ITEM"] - resource_type: Literal["FILE", "CONVERSATION", "PROMPT"] + resource_type: Literal["FILE", "CONVERSATION", "PROMPT", "SKILL"] class ResourceItemMetadata(BaseMetadata): @@ -69,3 +69,39 @@ class PromptMetadata(BaseMetadata): next_token: str | None = None items: list[PromptItem] | None resource_type: Literal["PROMPT"] + + +class SkillItem(ResourceItemMetadata): + """ + A node in the skills listing: a skill (ITEM) or a grouping folder (FOLDER). + + DIAL Core builds these from the folder marker's listing metadata without + reading the marker body, so no ``etag`` and no skill name/description are + carried here - they are available via a whole-resource GET. + """ + + node_type: Literal["FOLDER", "ITEM"] + resource_type: Literal["SKILL"] + + +class SkillMetadata(BaseMetadata): + node_type: Literal["FOLDER", "ITEM"] + resource_type: Literal["SKILL"] + next_token: str | None = None + items: list[SkillItem] | None = None + + +class SkillFileItem(ResourceItemMetadata): + """A file (ITEM) or a subfolder (FOLDER) inside a skill.""" + + node_type: Literal["FOLDER", "ITEM"] + resource_type: Literal["SKILL"] + content_length: int | None = None + content_type: str | None = None + + +class SkillFileMetadata(BaseMetadata): + node_type: Literal["FOLDER", "ITEM"] + resource_type: Literal["SKILL"] + next_token: str | None = None + items: list[SkillFileItem] | None = None diff --git a/tests/helpers/test_storage_resource_parser.py b/tests/helpers/test_storage_resource_parser.py index dba29a2..400a6ab 100644 --- a/tests/helpers/test_storage_resource_parser.py +++ b/tests/helpers/test_storage_resource_parser.py @@ -178,3 +178,92 @@ def test_parse_storage_resource_non_dial_ignore(): dial_api_url="https://dial.core/v1/", expected_resource_type="files", ) + + +@pytest.mark.parametrize( + "url, expected_api_path", + [ + ("skills/my-bucket/my-skill", "skills/my-bucket/my-skill"), + ("skills/my-bucket/group/my-skill", "skills/my-bucket/group/my-skill"), + ( + "https://dial.core/v2/skills/my-bucket/my-skill", + "skills/my-bucket/my-skill", + ), + ], +) +def test_parse_v2_skill_resource(url, expected_api_path): + result = parse_storage_resource( + url=url, + dial_api_url="https://dial.core/v2/", + expected_resource_type="skills", + api_prefix="v2/", + ) + assert result.resource_type == "skills" + assert result.bucket == "my-bucket" + assert result.api_path == expected_api_path + + +@pytest.mark.parametrize( + "url, dial_api_url, resource_type, api_prefix", + [ + ("skills/my-bucket", "https://dial.core/v2/", "skills", "v2/"), + ("skills/my-bucket/", "https://dial.core/v2/", "skills", "v2/"), + ("files/my-bucket", "https://dial.core/v1/", "files", "v1/"), + ], +) +def test_parse_bucket_root_when_allowed( + url, dial_api_url, resource_type, api_prefix +): + result = parse_storage_resource( + url=url, + dial_api_url=dial_api_url, + expected_resource_type=resource_type, + api_prefix=api_prefix, + allow_bucket_root=True, + ) + assert result.bucket == "my-bucket" + assert result.bucket_path == "" + assert result.filename is None + assert result.api_path == f"{resource_type}/my-bucket" + + +@pytest.mark.parametrize( + "url, dial_api_url, resource_type, api_prefix", + [ + ("skills/my-bucket", "https://dial.core/v2/", "skills", "v2/"), + ("files/my-bucket", "https://dial.core/v1/", "files", "v1/"), + ], +) +def test_parse_bucket_root_rejected_by_default( + url, dial_api_url, resource_type, api_prefix +): + # A two-segment path is ambiguous ("files/my-file.txt" has the same + # shape), so bucket-root parsing stays opt-in. + with pytest.raises(InvalidDialURLError, match="Missing bucket in URL"): + parse_storage_resource( + url=url, + dial_api_url=dial_api_url, + expected_resource_type=resource_type, + api_prefix=api_prefix, + ) + + +def test_parse_rejects_v2_api_prefix_as_relative_part(): + with pytest.raises( + InvalidDialURLError, match="API prefix as relative part" + ): + parse_storage_resource( + url="v2/skills/my-bucket/my-skill", + dial_api_url="https://dial.core/v2/", + expected_resource_type="skills", + api_prefix="v2/", + ) + + +def test_parse_rejects_skills_url_for_v1_resource(): + with pytest.raises(InvalidDialURLError, match="Invalid resource type"): + parse_storage_resource( + url="skills/my-bucket/my-skill", + dial_api_url="https://dial.core/v1/", + expected_resource_type="files", + ) diff --git a/tests/resources/skills/test_skill_download.py b/tests/resources/skills/test_skill_download.py new file mode 100644 index 0000000..dd17546 --- /dev/null +++ b/tests/resources/skills/test_skill_download.py @@ -0,0 +1,339 @@ +from typing import Any, cast +from unittest.mock import AsyncMock + +import httpx +import pytest + +from aidial_client import Dial +from aidial_client._client import AsyncDial +from aidial_client._exception import ( + DialException, + EtagMismatchError, + InvalidDialURLError, + ResourceNotFoundError, +) +from tests.client_mock import MockStreamIterator, get_client_mock + +SKILL_URL = "skills/test-bucket/writing/tone-of-voice" +ZIP_BYTES = b"PK\x03\x04fake-archive" + + +def _capturing_client( + captured: list[httpx.Request], + content: bytes, + headers: dict[str, str] | None = None, +) -> Dial: + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response( + status_code=200, + request=request, + content=content, + headers=headers or {}, + ) + response.request = request + return response + + client._http_client._internal_http_client.send = send_mock + return client + + +def test_download_whole_skill_as_zip(): + captured: list[httpx.Request] = [] + client = _capturing_client( + captured, + ZIP_BYTES, + {"content-type": "application/zip", "etag": "aggregate-etag"}, + ) + + response = client.skills.download(SKILL_URL) + + assert ( + captured[0].url.path == "/v2/skills/test-bucket/writing/tone-of-voice" + ) + assert response.get_content() == ZIP_BYTES + assert response.content_type == "application/zip" + assert response.headers["etag"] == "aggregate-etag" + # Core sends no Content-Disposition, so the archive is named after the skill. + assert response.filename == "tone-of-voice.zip" + + +def test_download_forwards_if_match(): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, ZIP_BYTES) + + client.skills.download(SKILL_URL, etag_if_match="aggregate-etag") + + assert captured[0].headers["if-match"] == "aggregate-etag" + + +def test_download_rejects_non_skill_url(): + client = _capturing_client([], ZIP_BYTES) + + with pytest.raises(InvalidDialURLError, match="Invalid resource type"): + client.skills.download("files/test-bucket/folder/file.txt") + + +def test_get_file_returns_bytes(): + captured: list[httpx.Request] = [] + client = _capturing_client( + captured, + b"---\nname: tone\ndescription: d\n---\nbody", + {"content-type": "text/markdown", "etag": "aggregate-etag"}, + ) + + response = client.skills.get_file(SKILL_URL, "SKILL.md") + + assert captured[0].url.path == ( + "/v2/skills/test-bucket/writing/tone-of-voice/files/SKILL.md" + ) + assert response.filename == "SKILL.md" + assert response.get_content().startswith(b"---") + + +def test_get_file_percent_encodes_relative_path(): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"schema") + + response = client.skills.get_file(SKILL_URL, "references/api schema.md") + + assert captured[0].url.raw_path.decode() == ( + "/v2/skills/test-bucket/writing/tone-of-voice" + "/files/references/api%20schema.md" + ) + # The filename stays human-readable. + assert response.filename == "api schema.md" + + +def test_get_file_accepts_already_encoded_path(): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"schema") + + client.skills.get_file(SKILL_URL, "references/api%20schema.md") + + assert captured[0].url.raw_path.decode() == ( + "/v2/skills/test-bucket/writing/tone-of-voice" + "/files/references/api%20schema.md" + ) + + +def test_get_file_preserves_non_utf8_content(): + payload = b"\x89PNG\r\n\x1a\n\xff\xfe" + client = _capturing_client([], payload, {"content-type": "image/png"}) + + response = client.skills.get_file(SKILL_URL, "assets/logo.png") + + assert response.get_content() == payload + + +@pytest.mark.parametrize( + "status_code, expected_exception", + [ + (404, ResourceNotFoundError), + (412, EtagMismatchError), + (403, DialException), + ], +) +def test_error_mapping(status_code, expected_exception): + client = get_client_mock( + status_code=status_code, json_mock={"error": {"message": "nope"}} + ) + + with pytest.raises(expected_exception) as exc_info: + client.skills.get_file(SKILL_URL, "SKILL.md") + + if status_code == 403: + assert exc_info.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_async_stream_download_streams_and_closes(): + captured: list[httpx.Request] = [] + captured_kwargs: list[dict[str, Any]] = [] + responses: list[httpx.Response] = [] + client = AsyncDial(api_key="dummy", base_url="http://dial.core") + client._get_my_bucket = cast(Any, AsyncMock(return_value="test-bucket")) + + async def send_mock( + request: httpx.Request, *, stream: bool = False, **kwargs: Any + ) -> httpx.Response: + captured.append(request) + captured_kwargs.append({"stream": stream, **kwargs}) + response = httpx.Response( + status_code=200, + request=request, + stream=MockStreamIterator(mock_chunks=[b"PK\x03\x04", b"rest"]), + ) + responses.append(response) + return response + + client._http_client._internal_http_client.send = cast(Any, send_mock) + + async with client.skills.stream_download(SKILL_URL) as response: + assert response.filename == "tone-of-voice.zip" + chunks = [chunk async for chunk in response] + assert b"".join(chunks) == b"PK\x03\x04rest" + + assert ( + captured[0].url.path == "/v2/skills/test-bucket/writing/tone-of-voice" + ) + assert captured_kwargs == [{"stream": True}] + assert responses[0].is_closed is True + + +@pytest.mark.asyncio +async def test_async_stream_file_streams_and_closes(): + captured: list[httpx.Request] = [] + responses: list[httpx.Response] = [] + client = AsyncDial(api_key="dummy", base_url="http://dial.core") + + async def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response( + status_code=200, + request=request, + stream=MockStreamIterator(mock_chunks=[b"# skill"]), + ) + responses.append(response) + return response + + client._http_client._internal_http_client.send = cast(Any, send_mock) + + async with client.skills.stream_file(SKILL_URL, "SKILL.md") as response: + assert response.filename == "SKILL.md" + assert b"".join([c async for c in response]) == b"# skill" + + assert captured[0].url.path == ( + "/v2/skills/test-bucket/writing/tone-of-voice/files/SKILL.md" + ) + assert responses[0].is_closed is True + + +@pytest.mark.parametrize( + "bad_path", + [ + "../../../victimbucket9f2/their-skill/files/SKILL.md", + "../SKILL.md", + "refs/../../../other/x.md", + "refs/./x.md", + ".", + "..", + # Percent-encoded dot segments: _percent_encode_relative_url + # unquotes before quoting, so these decode back to ".." and would + # otherwise slip past a literal check. + "%2e%2e/%2e%2e/%2e%2e/victimbucket9f2/s/files/SKILL.md", + "%2E%2E/SKILL.md", + "refs/%2e/x.md", + ".%2e/SKILL.md", + ], +) +def test_get_file_rejects_traversal_segments(bad_path): + # file_path is appended to an already-validated api path and never goes + # back through the url parser, so "." / ".." would shift the bucket + # segment and retarget the request at another bucket. + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): + client.skills.get_file(SKILL_URL, bad_path) + + assert captured == [] + + +@pytest.mark.parametrize( + "bad_path", + [ + "../../../victimbucket9f2/their-skill/files", + "refs/../../..", + "%2e%2e/%2e%2e/%2e%2e/victimbucket9f2/s/files", + ], +) +def test_list_files_rejects_traversal_segments(bad_path): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): + client.skills.list_files(SKILL_URL, path=bad_path) + + assert captured == [] + + +@pytest.mark.parametrize( + "bad_path, message", + [ + ("", "must not be empty"), + (" ", "must not be empty"), + ("/abs/path.md", "must be relative to the skill root"), + ("refs/", "points to a directory, not a file"), + ("a//b.md", "Empty path segment"), + ], +) +def test_get_file_rejects_malformed_path(bad_path, message): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + with pytest.raises(InvalidDialURLError, match=message): + client.skills.get_file(SKILL_URL, bad_path) + + assert captured == [] + + +@pytest.mark.asyncio +async def test_async_get_file_rejects_traversal(): + client = AsyncDial(api_key="dummy", base_url="http://dial.core") + + with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): + await client.skills.get_file(SKILL_URL, "../../../other/s/files/x.md") + + +@pytest.mark.parametrize( + "bad_path", + ["refs/a%2Fb.md", "a%2fb.md", "a%2f%2e%2e%2fb"], +) +def test_get_file_rejects_encoded_separator(bad_path): + # An encoded separator would decode into a real one, splitting a segment + # after validation and landing in the derived filename. + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + with pytest.raises(InvalidDialURLError, match="encoded path separator"): + client.skills.get_file(SKILL_URL, bad_path) + + assert captured == [] + + +def test_list_files_rejects_empty_path_like_get_file(): + # "" is validated the same way for both entry points; None means "unset". + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + with pytest.raises(InvalidDialURLError, match="path must not be empty"): + client.skills.list_files(SKILL_URL, path="") + + assert captured == [] + + +@pytest.mark.parametrize( + "good_path, expected_raw, expected_filename", + [ + ("SKILL.md", "SKILL.md", "SKILL.md"), + ("refs/api schema.md", "refs/api%20schema.md", "api schema.md"), + ("refs/api%20schema.md", "refs/api%20schema.md", "api schema.md"), + # A dot inside a segment is not a dot segment. + ("v1.2/notes.md", "v1.2/notes.md", "notes.md"), + ], +) +def test_get_file_accepts_legitimate_paths( + good_path, expected_raw, expected_filename +): + captured: list[httpx.Request] = [] + client = _capturing_client(captured, b"x") + + response = client.skills.get_file(SKILL_URL, good_path) + + assert captured[0].url.raw_path.decode() == ( + f"/v2/skills/test-bucket/writing/tone-of-voice/files/{expected_raw}" + ) + assert response.filename == expected_filename diff --git a/tests/resources/skills/test_skill_metadata.py b/tests/resources/skills/test_skill_metadata.py new file mode 100644 index 0000000..bff702a --- /dev/null +++ b/tests/resources/skills/test_skill_metadata.py @@ -0,0 +1,266 @@ +from typing import Any, cast +from unittest.mock import AsyncMock, Mock + +import httpx +import pytest + +from aidial_client import Dial +from aidial_client._client import AsyncDial +from aidial_client._exception import InvalidDialURLError + +SKILLS_LISTING_MOCK = { + "name": "writing", + "parentPath": None, + "bucket": "test-bucket", + "url": "skills/test-bucket/writing/", + "nodeType": "FOLDER", + "resourceType": "SKILL", + "items": [ + { + "name": "tone-of-voice", + "parentPath": "writing", + "bucket": "test-bucket", + "url": "skills/test-bucket/writing/tone-of-voice", + "nodeType": "ITEM", + "resourceType": "SKILL", + "createdAt": 1700000000000, + "updatedAt": 1700000001000, + "author": "someone", + }, + { + "name": "drafts", + "parentPath": "writing", + "bucket": "test-bucket", + "url": "skills/test-bucket/writing/drafts/", + "nodeType": "FOLDER", + "resourceType": "SKILL", + }, + ], + "nextToken": "next-page-token", +} + +SKILL_FILES_MOCK = { + "name": "files", + "parentPath": "tone-of-voice", + "bucket": "test-bucket", + "url": "skills/test-bucket/tone-of-voice/files/", + "nodeType": "FOLDER", + "resourceType": "SKILL", + "items": [ + { + "name": "SKILL.md", + "parentPath": "tone-of-voice/files", + "bucket": "test-bucket", + "url": "skills/test-bucket/tone-of-voice/files/SKILL.md", + "nodeType": "ITEM", + "resourceType": "SKILL", + "etag": "abc123", + "updatedAt": 1700000001000, + }, + { + "name": "references", + "parentPath": "tone-of-voice/files", + "bucket": "test-bucket", + "url": "skills/test-bucket/tone-of-voice/files/references/", + "nodeType": "FOLDER", + "resourceType": "SKILL", + }, + ], + "nextToken": None, +} + + +def _sync_client(captured: list[httpx.Request], payload: dict) -> Dial: + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response( + status_code=200, request=request, json=payload + ) + response.request = request + return response + + client._http_client._internal_http_client.send = send_mock + client._get_my_bucket = Mock(return_value="test-bucket") + return client + + +def _async_client(captured: list[httpx.Request], payload: dict) -> AsyncDial: + client = AsyncDial(api_key="dummy", base_url="http://dial.core") + + async def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response( + status_code=200, request=request, json=payload + ) + response.request = request + return response + + client._http_client._internal_http_client.send = cast(Any, send_mock) + client._get_my_bucket = cast(Any, AsyncMock(return_value="test-bucket")) + return client + + +def test_get_metadata_lists_bucket_root(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILLS_LISTING_MOCK) + + result = client.skills.get_metadata(client.my_skills_home()) + + assert captured[0].url.path == "/v2/metadata/skills/test-bucket" + assert result.resource_type == "SKILL" + assert result.next_token == "next-page-token" # noqa: S105 + + items = result.items or [] + assert [item.node_type for item in items] == ["ITEM", "FOLDER"] + # A skill node is an ITEM; Core omits the aggregate etag from the listing. + assert items[0].name == "tone-of-voice" + assert items[0].etag is None + assert items[0].author == "someone" + + +def test_get_metadata_passes_listing_params(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILLS_LISTING_MOCK) + + client.skills.get_metadata( + "skills/test-bucket/writing", + limit=1000, + token="page-2", # noqa: S106 + recursive=True, + ) + + request = captured[0] + assert request.url.path == "/v2/metadata/skills/test-bucket/writing" + assert dict(request.url.params) == { + "limit": "1000", + "token": "page-2", # noqa: S105 + "recursive": "true", + } + + +def test_get_metadata_omits_unset_params(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILLS_LISTING_MOCK) + + client.skills.get_metadata("skills/test-bucket/writing") + + assert dict(captured[0].url.params) == {} + + +def test_list_files_defaults_to_skill_root(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILL_FILES_MOCK) + + result = client.skills.list_files( + "skills/test-bucket/tone-of-voice", recursive=True, limit=1000 + ) + + request = captured[0] + assert ( + request.url.path + == "/v2/metadata/skills/test-bucket/tone-of-voice/files" + ) + assert dict(request.url.params) == {"limit": "1000", "recursive": "true"} + + items = result.items or [] + assert [item.node_type for item in items] == ["ITEM", "FOLDER"] + assert items[0].etag == "abc123" + assert result.next_token is None + + +def test_list_files_scopes_to_subfolder(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILL_FILES_MOCK) + + client.skills.list_files( + "skills/test-bucket/tone-of-voice", path="references/api schema" + ) + + assert captured[0].url.raw_path.decode() == ( + "/v2/metadata/skills/test-bucket/tone-of-voice" + "/files/references/api%20schema" + ) + + +def test_list_files_pagination_loop_terminates(): + pages = [ + {**SKILL_FILES_MOCK, "nextToken": "page-2"}, + {**SKILL_FILES_MOCK, "nextToken": None}, + ] + captured: list[httpx.Request] = [] + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + response = httpx.Response( + status_code=200, + request=request, + json=pages[len(captured)], + ) + captured.append(request) + response.request = request + return response + + client._http_client._internal_http_client.send = send_mock + + token = None + seen = 0 + while True: + page = client.skills.list_files( + "skills/test-bucket/tone-of-voice", token=token + ) + seen += len(page.items or []) + token = page.next_token + if token is None: + break + + assert len(captured) == 2 + assert seen == 4 + assert dict(captured[1].url.params) == {"token": "page-2"} + + +def test_get_metadata_rejects_non_skill_url(): + client = _sync_client([], SKILLS_LISTING_MOCK) + + with pytest.raises(InvalidDialURLError, match="Invalid resource type"): + client.skills.get_metadata("files/test-bucket/folder") + + +def test_list_files_rejects_bucket_root(): + # A bucket has no files of its own - only skills do. + client = _sync_client([], SKILL_FILES_MOCK) + + with pytest.raises(InvalidDialURLError, match="Missing bucket in URL"): + client.skills.list_files("skills/test-bucket") + + +@pytest.mark.asyncio +async def test_async_get_metadata_and_list_files(): + captured: list[httpx.Request] = [] + client = _async_client(captured, SKILLS_LISTING_MOCK) + + result = await client.skills.get_metadata(await client.my_skills_home()) + assert captured[0].url.path == "/v2/metadata/skills/test-bucket" + assert (result.items or [])[0].name == "tone-of-voice" + + await client.skills.list_files( + "skills/test-bucket/tone-of-voice", recursive=True + ) + assert ( + captured[1].url.path + == "/v2/metadata/skills/test-bucket/tone-of-voice/files" + ) + + +def test_list_files_accepts_folder_path_with_trailing_slash(): + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILL_FILES_MOCK) + + client.skills.list_files( + "skills/test-bucket/tone-of-voice", path="references/" + ) + + assert captured[0].url.path == ( + "/v2/metadata/skills/test-bucket/tone-of-voice/files/references" + ) From 3017bd844ab3e7b6ab148120e95222af8d5eaf8f Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 28 Aug 2026 11:59:04 +0300 Subject: [PATCH 2/4] fix(skills): align read requests and types with DIAL Core behaviour Follow-up on review of the /v2/skills read resource. Bucket-root listing was unroutable: the separator before {path} in Core's COMPLEX_RESOURCE_METADATA regex is literal, so an empty {path} only matches with a trailing slash, which api_path (a PurePosixPath) never carries. Emit one from _prepare_metadata_request; Core strips it back off {path}, so the deeper paths resolve to the same folder as before. Drop etag_if_match from the four reads. Neither ComplexResourceController .get nor .getFile calls ProxyUtil.etag, and neither operation declares an If-Match parameter or a 412 response, so the header was sent and silently ignored - advertising a precondition the server does not enforce. The /v1 reads do honour it, which is where the parameter was copied from. Drop content_length/content_type from SkillFileItem: listFiles builds plain ResourceItemMetadata entries, which carry neither field. Document that subfolders in the files listing are reported as "ITEM" too - listFiles never overrides the node type, so only the trailing "/" of the url marks a folder. node_type stays a union so an upstream fix does not become a parsing error here. Add test_skill_core_routes.py, which matches every built read URL against Core's own route regexes - the string assertions could not tell a well-formed url from a routable one. --- README.md | 11 ++ aidial_client/resources/skills.py | 48 +++--- aidial_client/types/metadata.py | 16 +- .../skills/test_skill_core_routes.py | 146 ++++++++++++++++++ tests/resources/skills/test_skill_download.py | 11 +- tests/resources/skills/test_skill_metadata.py | 17 +- 6 files changed, 209 insertions(+), 40 deletions(-) create mode 100644 tests/resources/skills/test_skill_core_routes.py diff --git a/README.md b/README.md index d61e555..d8763a3 100644 --- a/README.md +++ b/README.md @@ -888,6 +888,17 @@ Scope the listing to a subfolder with `path`: page = await async_client.skills.list_files(skill, path="references") ``` +> [!IMPORTANT] +> In this listing, use the trailing `/` of `url` to tell subfolders from +> files — not `node_type`. DIAL Core builds every entry as a plain item and +> never overrides its node type, so subfolders are reported as `"ITEM"` too. +> This differs from [Listing Skills](#listing-skills), where `node_type` +> does distinguish a skill from a grouping folder. +> +> ```python +> files = [item for item in page.items or [] if not item.url.endswith("/")] +> ``` + Example of the response: ```python diff --git a/aidial_client/resources/skills.py b/aidial_client/resources/skills.py index c42cc5b..6074018 100644 --- a/aidial_client/resources/skills.py +++ b/aidial_client/resources/skills.py @@ -115,9 +115,16 @@ def _prepare_metadata_request( # Core lists the bucket root when {path} is empty, so a bucket-root # url ("skills/my-bucket") is a valid target here. api_path = self.get_api_path(url, allow_bucket_root=True) + # This route always addresses a folder, and the separator after + # {bucket} in Core's route regex is literal: + # ^/v2/metadata/skills/(?[a-zA-Z0-9]+)/(?.*)$ + # so an empty {path} only matches with a trailing slash. api_path + # comes from PurePosixPath and never carries one. Core strips a + # trailing slash off {path} again, so appending it unconditionally + # leaves the deeper paths resolving to the same folder as before. return FinalRequestOptions( method="GET", - url=urljoin(METADATA_V2_PREFIX, api_path), + url=urljoin(METADATA_V2_PREFIX, f"{api_path}/"), params=self._listing_params(limit, token, recursive), ) @@ -140,7 +147,6 @@ def _prepare_get_file_request( self, url: str | PurePosixPath, file_path: str, - etag_if_match: str | None, ) -> tuple[FinalRequestOptions, str]: segments = _relative_path_segments(file_path, "file_path") if segments[-1] == "": @@ -150,23 +156,25 @@ def _prepare_get_file_request( relative = _percent_encode_relative_url("/".join(segments)) api_path = f"{self.get_api_path(url)}/{FILES_SEGMENT}/{relative}" + # No If-Match: unlike the /v1 reads, neither v2 read honours it - + # ComplexResourceController.getFile never calls ProxyUtil.etag, and + # the operation declares no If-Match parameter and no 412 response. options = FinalRequestOptions( method="GET", url=urljoin(API_V2_PREFIX, api_path), - headers=remove_none({"If-Match": etag_if_match}), ) return options, unquote(segments[-1]) def _prepare_download_archive_request( self, url: str | PurePosixPath, - etag_if_match: str | None, ) -> tuple[FinalRequestOptions, str]: api_path = self.get_api_path(url) + # See _prepare_get_file_request: Core ignores If-Match on this read + # too (ComplexResourceController.get). options = FinalRequestOptions( method="GET", url=urljoin(API_V2_PREFIX, api_path), - headers=remove_none({"If-Match": etag_if_match}), ) # Core answers application/zip without a Content-Disposition header, # so name the archive after the skill. @@ -229,7 +237,6 @@ def get_file( self, url: str | PurePosixPath, file_path: str, - etag_if_match: str | None = None, ) -> FileDownloadResponse: """ Download a single file bundled in the skill at ``url``. @@ -237,9 +244,7 @@ def get_file( ``file_path`` is relative to the skill root, e.g. "SKILL.md" or "references/api-schema.md". """ - options, filename = self._prepare_get_file_request( - url, file_path, etag_if_match - ) + options, filename = self._prepare_get_file_request(url, file_path) response = self.http_client.request( cast_to=httpx.Response, options=options, @@ -250,14 +255,11 @@ def get_file( def download( self, url: str | PurePosixPath, - etag_if_match: str | None = None, ) -> FileDownloadResponse: """ Download the whole skill at ``url`` as a ZIP archive. """ - options, filename = self._prepare_download_archive_request( - url, etag_if_match - ) + options, filename = self._prepare_download_archive_request(url) response = self.http_client.request( cast_to=httpx.Response, options=options, @@ -322,7 +324,6 @@ async def get_file( self, url: str | PurePosixPath, file_path: str, - etag_if_match: str | None = None, ) -> FileDownloadResponse: """ Download a single file bundled in the skill at ``url``. @@ -330,9 +331,7 @@ async def get_file( ``file_path`` is relative to the skill root, e.g. "SKILL.md" or "references/api-schema.md". """ - options, filename = self._prepare_get_file_request( - url, file_path, etag_if_match - ) + options, filename = self._prepare_get_file_request(url, file_path) response = await self.http_client.request( cast_to=httpx.Response, options=options, @@ -345,14 +344,11 @@ async def stream_file( self, url: str | PurePosixPath, file_path: str, - etag_if_match: str | None = None, ) -> AsyncIterator[FileDownloadResponse]: """ Stream a single file bundled in the skill at ``url``. """ - options, filename = self._prepare_get_file_request( - url, file_path, etag_if_match - ) + options, filename = self._prepare_get_file_request(url, file_path) async with self.http_client.stream( options=options, on_http_error=storage_error_processor, @@ -362,14 +358,11 @@ async def stream_file( async def download( self, url: str | PurePosixPath, - etag_if_match: str | None = None, ) -> FileDownloadResponse: """ Download the whole skill at ``url`` as a ZIP archive. """ - options, filename = self._prepare_download_archive_request( - url, etag_if_match - ) + options, filename = self._prepare_download_archive_request(url) response = await self.http_client.request( cast_to=httpx.Response, options=options, @@ -381,14 +374,11 @@ async def download( async def stream_download( self, url: str | PurePosixPath, - etag_if_match: str | None = None, ) -> AsyncIterator[FileDownloadResponse]: """ Stream the whole skill at ``url`` as a ZIP archive. """ - options, filename = self._prepare_download_archive_request( - url, etag_if_match - ) + options, filename = self._prepare_download_archive_request(url) async with self.http_client.stream( options=options, on_http_error=storage_error_processor, diff --git a/aidial_client/types/metadata.py b/aidial_client/types/metadata.py index 0711dee..132aef4 100644 --- a/aidial_client/types/metadata.py +++ b/aidial_client/types/metadata.py @@ -92,12 +92,22 @@ class SkillMetadata(BaseMetadata): class SkillFileItem(ResourceItemMetadata): - """A file (ITEM) or a subfolder (FOLDER) inside a skill.""" + """ + A file or a subfolder inside a skill. + + Use the trailing "/" of ``url`` to tell them apart, not ``node_type``: + DIAL Core builds every entry of this listing as a plain item and never + overrides its node type, so a subfolder is reported as ``"ITEM"`` too. + ``node_type`` is left as a union because that is a Core-side bug, and a + fix upstream should not turn into a parsing error here. + + Unlike the /v1 files listing, no ``content_length`` or ``content_type`` + is carried - Core copies only the etag, timestamps and author onto these + entries. + """ node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] - content_length: int | None = None - content_type: str | None = None class SkillFileMetadata(BaseMetadata): diff --git a/tests/resources/skills/test_skill_core_routes.py b/tests/resources/skills/test_skill_core_routes.py new file mode 100644 index 0000000..47a052b --- /dev/null +++ b/tests/resources/skills/test_skill_core_routes.py @@ -0,0 +1,146 @@ +""" +Pin the URLs the skills resource builds against DIAL Core's own route +regexes. + +The rest of the skills tests assert URL strings, which cannot catch a URL +that is well-formed but unroutable - a bucket-root listing without its +trailing slash matched every string assertion while Core would have answered +404. These patterns are copied verbatim from ai-dial-core's +``server/.../data/RouteTemplate.java``; keep them in sync when Core changes +them. + +The bucket here is alphanumeric on purpose: Core's ``[a-zA-Z0-9]+`` bucket +group rejects the hyphenated names used elsewhere in these tests, so a +hyphenated fixture would fail to match for the wrong reason. +""" + +import re +from collections.abc import Callable +from typing import Any +from unittest.mock import Mock + +import httpx +import pytest + +from aidial_client import Dial + +COMPLEX_RESOURCE = re.compile( + r"^/v2/skills/(?P[a-zA-Z0-9]+)" + r"/(?P[^/](?:[^/]|/(?=[^/])(?!files/))*)$" +) +COMPLEX_RESOURCE_FILE = re.compile( + r"^/v2/skills/(?P[a-zA-Z0-9]+)" + r"/(?P.+?)/files/(?P.+)$" +) +COMPLEX_RESOURCE_FILE_METADATA = re.compile( + r"^/v2/metadata/skills/(?P[a-zA-Z0-9]+)" + r"/(?P.+?)/files(?:/(?P.*))?$" +) +COMPLEX_RESOURCE_METADATA = re.compile( + r"^/v2/metadata/skills/(?P[a-zA-Z0-9]+)/(?P.*)$" +) + +BUCKET = "mybucket7a1" +SKILL = f"skills/{BUCKET}/writing/toneofvoice" + + +# Parses as both SkillMetadata and SkillFileMetadata; the binary reads cast +# to httpx.Response and ignore it. Only the request URL matters here. +EMPTY_LISTING = { + "bucket": BUCKET, + "url": f"skills/{BUCKET}/", + "nodeType": "FOLDER", + "resourceType": "SKILL", +} + + +def _client(captured: list[httpx.Request]) -> Dial: + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response( + status_code=200, request=request, json=EMPTY_LISTING + ) + response.request = request + return response + + client._http_client._internal_http_client.send = send_mock + client._get_my_bucket = Mock(return_value=BUCKET) + return client + + +def _route( + call: Callable[[Dial], object], +) -> tuple[str, dict[str, str | None]]: + captured: list[httpx.Request] = [] + call(_client(captured)) + path = captured[0].url.raw_path.decode() + + # Core checks the file routes before the whole-resource ones, so a path + # carrying a "/files/" segment must be reported as the file route. + for name, pattern in ( + ("COMPLEX_RESOURCE_FILE_METADATA", COMPLEX_RESOURCE_FILE_METADATA), + ("COMPLEX_RESOURCE_METADATA", COMPLEX_RESOURCE_METADATA), + ("COMPLEX_RESOURCE_FILE", COMPLEX_RESOURCE_FILE), + ("COMPLEX_RESOURCE", COMPLEX_RESOURCE), + ): + match = pattern.match(path) + if match: + return name, match.groupdict() + pytest.fail(f"{path} matches no /v2/skills route in DIAL Core") + + +def test_bucket_root_listing_is_routable(): + route, groups = _route( + lambda client: client.skills.get_metadata(client.my_skills_home()) + ) + + assert route == "COMPLEX_RESOURCE_METADATA" + assert groups["bucket"] == BUCKET + # An empty {path} is how Core lists the bucket root - and the separator + # before it is literal, so this only matches with the trailing slash. + assert groups["path"] == "" + + +def test_grouping_folder_listing_is_routable(): + route, groups = _route( + lambda client: client.skills.get_metadata(f"skills/{BUCKET}/writing") + ) + + assert route == "COMPLEX_RESOURCE_METADATA" + assert groups["path"] == "writing/" + + +def test_list_files_is_routable(): + route, groups = _route(lambda client: client.skills.list_files(SKILL)) + + assert route == "COMPLEX_RESOURCE_FILE_METADATA" + assert groups["path"] == "writing/toneofvoice" + assert groups["filePath"] is None + + +def test_list_files_subfolder_is_routable(): + route, groups = _route( + lambda client: client.skills.list_files(SKILL, path="references") + ) + + assert route == "COMPLEX_RESOURCE_FILE_METADATA" + assert groups["filePath"] == "references" + + +def test_get_file_is_routable(): + route, groups = _route( + lambda client: client.skills.get_file(SKILL, "SKILL.md") + ) + + assert route == "COMPLEX_RESOURCE_FILE" + assert groups["path"] == "writing/toneofvoice" + assert groups["filePath"] == "SKILL.md" + + +def test_download_is_routable(): + route, groups = _route(lambda client: client.skills.download(SKILL)) + + assert route == "COMPLEX_RESOURCE" + assert groups["path"] == "writing/toneofvoice" diff --git a/tests/resources/skills/test_skill_download.py b/tests/resources/skills/test_skill_download.py index dd17546..42b849f 100644 --- a/tests/resources/skills/test_skill_download.py +++ b/tests/resources/skills/test_skill_download.py @@ -60,13 +60,18 @@ def test_download_whole_skill_as_zip(): assert response.filename == "tone-of-voice.zip" -def test_download_forwards_if_match(): +def test_reads_send_no_if_match(): + # DIAL Core ignores If-Match on both v2 reads: neither + # ComplexResourceController.get nor .getFile calls ProxyUtil.etag, and + # neither operation declares the header or a 412 response. Sending it + # anyway would advertise a precondition the server does not enforce. captured: list[httpx.Request] = [] client = _capturing_client(captured, ZIP_BYTES) - client.skills.download(SKILL_URL, etag_if_match="aggregate-etag") + client.skills.download(SKILL_URL) + client.skills.get_file(SKILL_URL, "SKILL.md") - assert captured[0].headers["if-match"] == "aggregate-etag" + assert all("if-match" not in request.headers for request in captured) def test_download_rejects_non_skill_url(): diff --git a/tests/resources/skills/test_skill_metadata.py b/tests/resources/skills/test_skill_metadata.py index bff702a..1915b28 100644 --- a/tests/resources/skills/test_skill_metadata.py +++ b/tests/resources/skills/test_skill_metadata.py @@ -61,8 +61,11 @@ "name": "references", "parentPath": "tone-of-voice/files", "bucket": "test-bucket", + # A subfolder. Core builds every entry of the files listing as a + # plain item and never overrides the node type, so it reports + # "ITEM" here too - only the trailing "/" marks it as a folder. "url": "skills/test-bucket/tone-of-voice/files/references/", - "nodeType": "FOLDER", + "nodeType": "ITEM", "resourceType": "SKILL", }, ], @@ -108,7 +111,7 @@ def test_get_metadata_lists_bucket_root(): result = client.skills.get_metadata(client.my_skills_home()) - assert captured[0].url.path == "/v2/metadata/skills/test-bucket" + assert captured[0].url.path == "/v2/metadata/skills/test-bucket/" assert result.resource_type == "SKILL" assert result.next_token == "next-page-token" # noqa: S105 @@ -132,7 +135,7 @@ def test_get_metadata_passes_listing_params(): ) request = captured[0] - assert request.url.path == "/v2/metadata/skills/test-bucket/writing" + assert request.url.path == "/v2/metadata/skills/test-bucket/writing/" assert dict(request.url.params) == { "limit": "1000", "token": "page-2", # noqa: S105 @@ -165,10 +168,14 @@ def test_list_files_defaults_to_skill_root(): assert dict(request.url.params) == {"limit": "1000", "recursive": "true"} items = result.items or [] - assert [item.node_type for item in items] == ["ITEM", "FOLDER"] assert items[0].etag == "abc123" assert result.next_token is None + # Core reports subfolders of a skill as "ITEM" as well, so a caller has + # to key off the trailing "/" of the url instead. + assert [item.node_type for item in items] == ["ITEM", "ITEM"] + assert [item.url.endswith("/") for item in items] == [False, True] + def test_list_files_scopes_to_subfolder(): captured: list[httpx.Request] = [] @@ -241,7 +248,7 @@ async def test_async_get_metadata_and_list_files(): client = _async_client(captured, SKILLS_LISTING_MOCK) result = await client.skills.get_metadata(await client.my_skills_home()) - assert captured[0].url.path == "/v2/metadata/skills/test-bucket" + assert captured[0].url.path == "/v2/metadata/skills/test-bucket/" assert (result.items or [])[0].name == "tone-of-voice" await client.skills.list_files( From 49b31865a741e379ff1cd27d55a15a6f5cb20866 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 4 Sep 2026 17:21:55 +0300 Subject: [PATCH 3/4] refactor(skills)!: build request urls from chained references Addresses the review on #138. client.skills is no longer called with a hand-built url. It is an immutable reference narrowed step by step - client.skills / "writing" / "tone-of-voice" - so a url is assembled from validated segments instead of a string the caller can typo. References issue no request until a terminal call, and the sync and async clients build them identically. Also from the review: - rename API_PREFIX/METADATA_PREFIX to _V1, API_V2_PREFIX to API_PREFIX_V2, api_v2_url to api_url_v2, StorageResourceType to StorageResourceTypeV1 and V2StorageResourceType to StorageResourceTypeV2 - derive the api prefix from resource_type via assert_never, dropping both the mixin field and the parser parameter - correct node_type on skill file listings rather than documenting the bug, so callers need not know about it - report bucket_path as None at the bucket root, not "" - make storage_error_processor and FILES_SEGMENT private, rename allow_bucket_root to allow_empty_bucket_path On node_type: a non-recursive listing of a skill's files reports its subfolders as "nodeType": "ITEM", the same value as the files beside them, distinguished only by the trailing "/" on url. The client upgrades those to FOLDER and never derives the reverse - the absence of a trailing slash carries no information, since a listing root scoped to a subfolder is requested without one. Verified against a live Core in both modes; a recursive listing is flattened to leaf files and carries no folder entries at all. Both responses are pinned as fixtures. BREAKING CHANGE: the /v2/skills surface added earlier on this branch is replaced; get_metadata/list_files/get_file/download taking a url are now list/read/download on a reference, and my_skills_home() is gone. Nothing is released yet, so no published API changes. BREAKING CHANGE: path traversal is now rejected while parsing any storage url, not just skills. "files/b/../../other/x" used to resolve through urljoin and silently retarget another bucket; it now raises InvalidDialURLError. Segments are checked as they decode, so %2e%2e and %2f are caught too. --- CLAUDE.md | 4 +- README.md | 132 +++- aidial_client/_client.py | 26 +- aidial_client/_constants.py | 10 +- aidial_client/helpers/storage_resource.py | 160 +++-- aidial_client/resources/__init__.py | 13 +- aidial_client/resources/bucket.py | 6 +- aidial_client/resources/files.py | 47 +- aidial_client/resources/metadata.py | 14 +- aidial_client/resources/prompts.py | 33 +- aidial_client/resources/skills.py | 647 ++++++++++-------- aidial_client/types/metadata.py | 66 +- tests/helpers/test_storage_resource_mixin.py | 2 +- tests/helpers/test_storage_resource_parser.py | 123 +++- .../skills/test_skill_core_routes.py | 16 +- tests/resources/skills/test_skill_download.py | 53 +- tests/resources/skills/test_skill_metadata.py | 77 ++- tests/resources/skills/test_skill_refs.py | 475 +++++++++++++ 18 files changed, 1385 insertions(+), 519 deletions(-) create mode 100644 tests/resources/skills/test_skill_refs.py diff --git a/CLAUDE.md b/CLAUDE.md index 12fc306..93c9965 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,8 +28,8 @@ make coverage # Run tests with coverage report poetry run pytest tests/test_auth.py -k "test_name" # Linting & formatting -make lint # Run all linters (pyright, flake8, codespell, format check) -make format # Auto-format code (autoflake, isort, black) +make lint # Run all linters (ruff check, ruff format --check, pyright) +make format # Auto-format code (ruff check --fix, ruff format) ``` ## Dependency Compatibility Matrix diff --git a/README.md b/README.md index d8763a3..9b6176a 100644 --- a/README.md +++ b/README.md @@ -811,22 +811,55 @@ files, addressed as a unit at `skills/{bucket}/{path}`. > contract may still change. The client currently supports the read > operations; writes are tracked separately. +Unlike the other resources, `client.skills` is not called with a URL you build +yourself. It is a *reference* that you narrow step by step, and each step +returns a new reference: + +```python +skill = client.skills / "writing" / "tone-of-voice" +# equivalently: client.skills(path="writing/tone-of-voice") +``` + +References are immutable, validate every path segment as they are built, and +issue no request until a terminal call (`list()`, `read()`, `download()`, +`stream()`, `stream_download()`). Building one is identical for the sync and +async clients — only the terminal call is awaited. + +A reference points at your own bucket unless told otherwise. Use `bucket=` for +a shared bucket such as `public`, and `url=` to follow an entry returned by a +listing: + +```python +client.skills(bucket="public") / "demo" / "azure-resource-visualizer" + +appdata = client.my_appdata() +client.skills(bucket=appdata.user_bucket, path=f"appdata/{appdata.app_name}") +``` + #### Listing Skills -Use `get_metadata()` to list the skills and grouping folders at a location. -Pass `my_skills_home()` to list the bucket root: +`list()` returns the skills and grouping folders at the reference. With no +narrowing it lists your bucket root: ```python # Sync -listing = client.skills.get_metadata(client.my_skills_home()) +listing = client.skills.list() # Async -listing = await async_client.skills.get_metadata( - await async_client.my_skills_home() -) +listing = await async_client.skills.list() for item in listing.items or []: # "ITEM" is a skill, "FOLDER" is a grouping folder print(item.node_type, item.url) + + # Follow either one with url= + nested = client.skills(url=item.url) +``` + +Narrow first to list a grouping folder, and pass the listing options to the +terminal call: + +```python +page = (client.skills / "writing").list(recursive=True, limit=1000) ``` Example of the response: @@ -864,41 +897,30 @@ SkillMetadata( #### Listing Files in a Skill -Use `list_files()` to enumerate what a skill contains. A page may hold fewer -entries than `limit`, so follow `next_token` until it is `None`: +`skill.files` is a reference to the skill's bundled files. A page may hold +fewer entries than `limit`, so follow `next_token` until it is `None` — +building the reference once and varying only the token: ```python -skill = "skills/my-bucket/writing/tone-of-voice" +skill = client.skills / "writing" / "tone-of-voice" +files = skill.files token = None while True: - page = client.skills.list_files( - skill, recursive=True, limit=1000, token=token - ) + page = files.list(recursive=True, limit=1000, token=token) for item in page.items or []: - print(item.url, item.etag) + print(item.node_type, item.url) token = page.next_token if token is None: break ``` -Scope the listing to a subfolder with `path`: +Narrow to a subfolder the same way as anywhere else: ```python -page = await async_client.skills.list_files(skill, path="references") +page = await (async_skill.files / "references").list() ``` -> [!IMPORTANT] -> In this listing, use the trailing `/` of `url` to tell subfolders from -> files — not `node_type`. DIAL Core builds every entry as a plain item and -> never overrides its node type, so subfolders are reported as `"ITEM"` too. -> This differs from [Listing Skills](#listing-skills), where `node_type` -> does distinguish a skill from a grouping folder. -> -> ```python -> files = [item for item in page.items or [] if not item.url.endswith("/")] -> ``` - Example of the response: ```python @@ -918,48 +940,81 @@ SkillFileMetadata( url="skills/my-bucket/writing/tone-of-voice/files/SKILL.md", node_type="ITEM", resource_type="SKILL", - etag="9749fad13d6e7092a6337c4af9d83764", updated_at=1724836248936, - ) + ), + SkillFileItem( + # A subfolder, as returned by a non-recursive listing. Core sends + # node_type="ITEM"; the client corrects it from the trailing "/". + name="references", + parent_path="writing/tone-of-voice/files", + bucket="my-bucket", + url="skills/my-bucket/writing/tone-of-voice/files/references/", + node_type="FOLDER", + resource_type="SKILL", + ), ], ) ``` +> [!NOTE] +> A non-recursive listing returns the immediate subfolders, and DIAL Core +> reports them with `nodeType: "ITEM"` — the same value as the files beside +> them. The client derives `node_type` from the trailing `/` of `url`, so +> `node_type` is reliable here and you do not have to inspect urls yourself. + +> [!NOTE] +> The two modes answer different questions. `recursive=True` flattens the +> tree: every file at every depth, no folder entries at all, with +> `parent_path` showing where each file sits. A non-recursive listing returns +> only the immediate children. Empty folders never appear in either mode. + +Unlike the `/v1` files listing, these entries are sparse: no +`content_length`, no `content_type`, and in observed responses no `etag` +either. Folder entries carry no `updated_at`. Treat every field except +`name`, `url`, `node_type` and `resource_type` as optional here. + #### Reading a File from a Skill -Use `get_file()` with a path relative to the skill root: +Name the file with `path=`, relative to the skill root, then `read()`: ```python # Sync -manifest = client.skills.get_file(skill, "SKILL.md") +manifest = skill.files(path="SKILL.md").read() print(manifest.get_content().decode()) # Async -manifest = await async_client.skills.get_file(skill, "SKILL.md") -schema = await async_client.skills.get_file( - skill, "references/api-schema.md" -) +manifest = await async_skill.files(path="SKILL.md").read() +schema = await async_skill.files(path="references/api-schema.md").read() await schema.awrite_to("api-schema.md") ``` +An entry from a files listing can be followed directly, without slicing its +url apart: + +```python +for item in files.list().items or []: + if item.node_type == "ITEM": + content = skill.files(url=item.url).read() +``` + The async client can stream instead, which avoids holding the file in memory: ```python -async with async_client.skills.stream_file(skill, "assets/logo.png") as file: +async with async_skill.files(path="assets/logo.png").stream() as file: await file.awrite_to("logo.png") ``` #### Downloading a Skill -Use `download()` to fetch the whole skill as a ZIP archive: +`download()` fetches the whole skill as a ZIP archive: ```python # Sync -archive = client.skills.download(skill) +archive = skill.download() archive.write_to("tone-of-voice.zip") # Async, streamed -async with async_client.skills.stream_download(skill) as archive: +async with async_skill.stream_download() as archive: await archive.awrite_to("tone-of-voice.zip") ``` @@ -971,6 +1026,7 @@ The response's `ETag` header carries the skill's aggregate etag: etag = archive.headers["etag"] ``` + ### Applications #### List Applications diff --git a/aidial_client/_client.py b/aidial_client/_client.py index 496668b..fd26676 100644 --- a/aidial_client/_client.py +++ b/aidial_client/_client.py @@ -15,8 +15,8 @@ validate_auth, ) from aidial_client._constants import ( - API_PREFIX, - API_V2_PREFIX, + API_PREFIX_V1, + API_PREFIX_V2, DEFAULT_MAX_RETRIES, DEFAULT_TIMEOUT, OPENAI_PREFIX, @@ -72,11 +72,11 @@ def is_dial_url(self, absolute_url: str) -> bool: @property def api_url(self) -> str: - return urljoin(self._base_url, API_PREFIX) + return urljoin(self._base_url, API_PREFIX_V1) @property - def api_v2_url(self) -> str: - return urljoin(self._base_url, API_V2_PREFIX) + def api_url_v2(self) -> str: + return urljoin(self._base_url, API_PREFIX_V2) @property def base_url(self) -> str: @@ -115,9 +115,10 @@ def _init_resources(self) -> None: metadata=self.metadata, dial_api_url=self.api_url, ) - self.skills = resources.Skills( + self.skills = resources.SkillsRef( http_client=self._http_client, - dial_api_url=self.api_v2_url, + dial_api_url=self.api_url_v2, + resolve_bucket=self.my_bucket, ) self.deployments = resources.Deployments(http_client=self._http_client) self.application = resources.Application(http_client=self._http_client) @@ -158,9 +159,6 @@ def my_conversations_home(self) -> PurePosixPath: def my_prompts_home(self) -> PurePosixPath: return "prompts" / PurePosixPath(self.my_bucket()) - def my_skills_home(self) -> PurePosixPath: - return "skills" / PurePosixPath(self.my_bucket()) - def _get_my_appdata(self) -> AppData | None: return self.bucket.get_appdata() @@ -223,9 +221,10 @@ def _init_resources(self) -> None: metadata=self.metadata, dial_api_url=self.api_url, ) - self.skills = resources.AsyncSkills( + self.skills = resources.AsyncSkillsRef( http_client=self._http_client, - dial_api_url=self.api_v2_url, + dial_api_url=self.api_url_v2, + resolve_bucket=self.my_bucket, ) self.deployments = resources.AsyncDeployments( http_client=self._http_client @@ -270,9 +269,6 @@ async def my_conversations_home(self) -> PurePosixPath: async def my_prompts_home(self) -> PurePosixPath: return "prompts" / PurePosixPath(await self.my_bucket()) - async def my_skills_home(self) -> PurePosixPath: - return "skills" / PurePosixPath(await self.my_bucket()) - async def _get_my_appdata(self) -> AppData | None: return await self.bucket.get_appdata() diff --git a/aidial_client/_constants.py b/aidial_client/_constants.py index dcf6fa3..58243e6 100644 --- a/aidial_client/_constants.py +++ b/aidial_client/_constants.py @@ -9,13 +9,13 @@ ) INITIAL_RETRY_DELAY = 0.5 MAX_RETRY_DELAY = 8.0 -API_PREFIX = "v1/" -METADATA_PREFIX = urljoin(API_PREFIX, "metadata/") -FILES_PREFIX = urljoin(API_PREFIX, "files/") +API_PREFIX_V1 = "v1/" +METADATA_PREFIX_V1 = urljoin(API_PREFIX_V1, "metadata/") +FILES_PREFIX = urljoin(API_PREFIX_V1, "files/") # DIAL Core exposes folder-shaped resources (agent skills) under /v2. -API_V2_PREFIX = "v2/" -METADATA_V2_PREFIX = urljoin(API_V2_PREFIX, "metadata/") +API_PREFIX_V2 = "v2/" +METADATA_PREFIX_V2 = urljoin(API_PREFIX_V2, "metadata/") OPENAI_PREFIX = "openai/" diff --git a/aidial_client/helpers/storage_resource.py b/aidial_client/helpers/storage_resource.py index 603894a..15fe4a6 100644 --- a/aidial_client/helpers/storage_resource.py +++ b/aidial_client/helpers/storage_resource.py @@ -3,9 +3,10 @@ from urllib.parse import quote, unquote, urljoin, urlparse, urlsplit import httpx +from typing_extensions import assert_never from aidial_client._compatibility.pydantic_v1 import BaseModel -from aidial_client._constants import API_PREFIX +from aidial_client._constants import API_PREFIX_V1, API_PREFIX_V2 from aidial_client._exception import ( DialException, EtagMismatchError, @@ -17,13 +18,24 @@ from aidial_client._utils._dict import remove_none from aidial_client.helpers._url import enforce_trailing_slash -StorageResourceType = Literal["files", "conversations", "prompts"] +StorageResourceTypeV1 = Literal["files", "conversations", "prompts"] """Resource types served by the /v1 storage API.""" -V2StorageResourceType = Literal["skills"] +StorageResourceTypeV2 = Literal["skills"] """Folder-shaped resource types served by the /v2 API.""" -AnyStorageResourceType = StorageResourceType | V2StorageResourceType +AnyStorageResourceType = StorageResourceTypeV1 | StorageResourceTypeV2 + + +def api_prefix_for(resource_type: AnyStorageResourceType) -> str: + """The API prefix that serves ``resource_type``.""" + match resource_type: + case "files" | "conversations" | "prompts": + return API_PREFIX_V1 + case "skills": + return API_PREFIX_V2 + case _: + assert_never(resource_type) def _percent_encode_relative_url(url: str) -> str: @@ -42,7 +54,61 @@ def _percent_encode_relative_url(url: str) -> str: return "/".join(quote(unquote(seg), safe="") for seg in segments) -def storage_error_processor( +def split_relative_segments( + path: str, + param: str, + *, + allow_trailing_slash: bool = False, +) -> tuple[str, ...]: + """ + Split a relative path and reject any segment that would change which + resource the path addresses. + + Each segment is checked *as it will decode*, because + ``_percent_encode_relative_url`` normalizes with ``unquote`` before + quoting: "%2e%2e" would otherwise pass a literal check and still reach + ``urljoin`` as "..", and "%2f" would smuggle in a separator. ``urljoin`` + resolves "." and ".." while building a request, which shifts the bucket + segment - "../../../other-bucket/x" turns a validated "skills/my-bucket" + into a request against ``other-bucket``. + + ``allow_trailing_slash`` accepts one trailing empty segment, which is how + DIAL spells "this is a folder". It is off for paths given to a resource + reference, where the terminal call decides file vs folder. + """ + if not path.strip(): + raise InvalidDialURLError(f"{param} must not be empty") + if path.startswith("/"): + raise InvalidDialURLError(f"{param} must be relative, got: {path!r}") + + segments = tuple(path.split("/")) + if not allow_trailing_slash and path.endswith("/"): + raise InvalidDialURLError( + f"{param} must not end with '/', got: {path!r}. The terminal call" + " decides whether the reference names a file or a folder." + ) + + # A trailing slash marks a folder; it is not a segment of its own. + checked = segments[:-1] if path.endswith("/") else segments + for segment in checked: + decoded = unquote(segment) + if decoded == "": + raise InvalidDialURLError( + f"Empty path segment in {param}, got: {path!r}" + ) + if decoded in (".", ".."): + raise InvalidDialURLError( + f'"." and ".." are not allowed in {param}, got: {path!r}' + ) + if "/" in decoded: + raise InvalidDialURLError( + "An encoded path separator is not allowed in" + f" {param}, got: {path!r}" + ) + return segments + + +def _storage_error_processor( http_status_error: httpx.HTTPStatusError, ) -> DialException | None: """ @@ -61,7 +127,7 @@ def storage_error_processor( def _is_directory(s: str) -> bool: - return s[-1] == "/" + return s.endswith("/") class DialStorageResource(BaseModel): @@ -81,9 +147,9 @@ class DialStorageResource(BaseModel): """ Path without bucket, like 'my-folder/my-file.txt' - Empty string when the URL points at the bucket root + None when the URL points at the bucket root """ - bucket_path: str + bucket_path: str | None = None """ Filename, like 'my-file.txt' @@ -97,35 +163,46 @@ def safe_parse_storage_resource( url: str, dial_api_url: str, expected_resource_type: AnyStorageResourceType | None = None, - api_prefix: str = API_PREFIX, - allow_bucket_root: bool = False, + allow_empty_bucket_path: bool = False, ) -> DialStorageResource | NotDialURLError | InvalidDialURLError: """ Parse the storage resource from the URL, that could be 1. Absolute: "https://dial.core/v1/files/my-bucket/my-file.txt" 2. Relative to API prefix: "files/my-bucket/my-file.txt" - ``allow_bucket_root`` accepts a bucket-root URL like "skills/my-bucket". - It is opt-in because a two-segment path is ambiguous: "files/my-file.txt" - has the same shape and is a missing-bucket error. Only callers whose - endpoint accepts an empty path (DIAL Core's v2 metadata listing) enable it. + ``allow_empty_bucket_path`` accepts a URL that names a bucket and nothing + inside it, like "skills/my-bucket". It is opt-in because a two-segment path + is ambiguous: "files/my-file.txt" has the same shape and is a missing-bucket + error. A resource reference passes it unconditionally - it does not yet know + which call comes next, so the endpoint's own requirements are checked when + the request is built. """ dial_api_url = enforce_trailing_slash(dial_api_url) if url.startswith("/"): return InvalidDialURLError(f"Root-relative URL is forbidden: {url}") - if url.startswith(api_prefix): + if url.startswith((API_PREFIX_V1, API_PREFIX_V2)): return InvalidDialURLError( f"API prefix as relative part is not allowed: {url}" ) + # Reject traversal on the raw string, before urljoin below resolves it: + # _percent_encode_relative_url leaves ".." intact (quote treats "." as + # always-safe), so urljoin would silently retarget another bucket. + url_path = urlsplit(url).path.lstrip("/") + if url_path: + try: + split_relative_segments(url_path, "url", allow_trailing_slash=True) + except InvalidDialURLError as error: + return error + absolute_url = urljoin(dial_api_url, _percent_encode_relative_url(url)) url_parsed = urlparse(absolute_url) dial_api_parsed = urlparse(dial_api_url) if url_parsed.netloc != dial_api_parsed.netloc: return NotDialURLError(message=f"Provided URL is not DIAL URL: {url}") try: - url_path = PurePosixPath(url_parsed.path) - api_path = url_path.relative_to(dial_api_parsed.path) + url_path_parsed = PurePosixPath(url_parsed.path) + api_path = url_path_parsed.relative_to(dial_api_parsed.path) except ValueError: return InvalidDialURLError( f"Provided URL path {url_parsed.path} does not match with" @@ -140,8 +217,8 @@ def safe_parse_storage_resource( parsed_resource_type = str(resource_path) if parsed_resource_type not in ( - *get_args(StorageResourceType), - *get_args(V2StorageResourceType), + *get_args(StorageResourceTypeV1), + *get_args(StorageResourceTypeV2), ): return InvalidDialURLError( f"Invalid resource type: {parsed_resource_type}" @@ -157,16 +234,16 @@ def safe_parse_storage_resource( ) if len(api_path.parents) < 3: - if not allow_bucket_root: - return InvalidDialURLError(f"Missing bucket in URL: {url}") - # The URL is "{resource_type}/{bucket}" — the bucket itself. + if not allow_empty_bucket_path: + return InvalidDialURLError(f"Missing bucket path in URL: {url}") + # The URL is "{resource_type}/{bucket}" - the bucket itself. return DialStorageResource( resource_type=cast(AnyStorageResourceType, parsed_resource_type), absolute_url=absolute_url, api_path=str(api_path), bucket=api_path.name, - bucket_path="", - relative_url=str(url_path), + bucket_path=None, + relative_url=str(url_path_parsed), filename=None, ) @@ -177,8 +254,8 @@ def safe_parse_storage_resource( api_path=str(api_path), bucket=str(bucket_path.relative_to(resource_path)), bucket_path=str(api_path.relative_to(bucket_path)), - relative_url=str(url_path), - filename=url_path.name if not _is_directory(url) else None, + relative_url=str(url_path_parsed), + filename=url_path_parsed.name if not _is_directory(url) else None, ) @@ -187,15 +264,13 @@ def parse_storage_resource( url: str, dial_api_url: str, expected_resource_type: AnyStorageResourceType | None = None, - api_prefix: str = API_PREFIX, - allow_bucket_root: bool = False, + allow_empty_bucket_path: bool = False, ) -> DialStorageResource: result = safe_parse_storage_resource( url=url, dial_api_url=dial_api_url, expected_resource_type=expected_resource_type, - api_prefix=api_prefix, - allow_bucket_root=allow_bucket_root, + allow_empty_bucket_path=allow_empty_bucket_path, ) if isinstance(result, NotDialURLError | InvalidDialURLError): raise result @@ -213,21 +288,24 @@ class DialStorageResourceMixin(BaseModel): resource_type: AnyStorageResourceType dial_api_url: str - api_prefix: str = API_PREFIX + + def get_api_prefix(self) -> str: + """The API prefix serving this resource, implied by its type.""" + return api_prefix_for(self.resource_type) def get_storage_resource( self, url: str | PurePosixPath, *, - allow_bucket_root: bool = False, + allow_empty_bucket_path: bool = False, ) -> DialStorageResource: """ Get the storage resource object from the URL Args: url (str | PurePosixPath): The URL to be processed. - allow_bucket_root (bool): Accept a bucket-root URL such as - "skills/my-bucket". Off by default, since a two-segment path - is otherwise a missing-bucket error. + allow_empty_bucket_path (bool): Accept a URL naming a bucket and + nothing inside it, such as "skills/my-bucket". Off by default, + since a two-segment path is otherwise a missing-bucket error. Returns: DialStorageResource: The storage resource object """ @@ -235,27 +313,27 @@ def get_storage_resource( url=str(url), dial_api_url=self.dial_api_url, expected_resource_type=self.resource_type, - api_prefix=self.api_prefix, - allow_bucket_root=allow_bucket_root, + allow_empty_bucket_path=allow_empty_bucket_path, ) def get_api_path( self, url: str | PurePosixPath, *, - allow_bucket_root: bool = False, + allow_empty_bucket_path: bool = False, ) -> str: """ Convert URL, that could relative or absolute, to relative, percent-encoded API path. """ return self.get_storage_resource( - url, allow_bucket_root=allow_bucket_root + url, allow_empty_bucket_path=allow_empty_bucket_path ).api_path - def get_display_name(self, url: str | PurePosixPath) -> str: + def get_display_name(self, url: str | PurePosixPath) -> str | None: """ Get the display name of the resource from the URL + None when the URL points at the bucket root. """ return self.get_storage_resource(url).bucket_path @@ -271,7 +349,7 @@ def _prepare_download_request( options = FinalRequestOptions( method="GET", - url=urljoin(self.api_prefix, storage_resource.api_path), + url=urljoin(self.get_api_prefix(), storage_resource.api_path), headers=remove_none( { "If-Match": etag_if_match, diff --git a/aidial_client/resources/__init__.py b/aidial_client/resources/__init__.py index 0c86271..8859d9f 100644 --- a/aidial_client/resources/__init__.py +++ b/aidial_client/resources/__init__.py @@ -17,7 +17,12 @@ from .chat import AsyncChat, Chat from .files import AsyncFiles, Files from .prompts import AsyncPrompts, Prompts -from .skills import AsyncSkills, Skills +from .skills import ( + AsyncSkillFilesRef, + AsyncSkillsRef, + SkillFilesRef, + SkillsRef, +) __all__ = [ "Chat", @@ -28,8 +33,10 @@ "AsyncFiles", "Prompts", "AsyncPrompts", - "Skills", - "AsyncSkills", + "SkillsRef", + "AsyncSkillsRef", + "SkillFilesRef", + "AsyncSkillFilesRef", "AsyncDeployments", "Deployments", "AsyncMetadata", diff --git a/aidial_client/resources/bucket.py b/aidial_client/resources/bucket.py index caf7536..c1569d2 100644 --- a/aidial_client/resources/bucket.py +++ b/aidial_client/resources/bucket.py @@ -1,6 +1,6 @@ from urllib.parse import urljoin -from aidial_client._constants import API_PREFIX +from aidial_client._constants import API_PREFIX_V1 from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client.resources.base import AsyncResource, Resource from aidial_client.types.bucket import AppData, BucketResponse @@ -11,7 +11,7 @@ def get_raw(self) -> BucketResponse: return self.http_client.request( cast_to=BucketResponse, options=FinalRequestOptions( - method="GET", url=urljoin(API_PREFIX, "bucket") + method="GET", url=urljoin(API_PREFIX_V1, "bucket") ), ) @@ -31,7 +31,7 @@ async def get_raw(self) -> BucketResponse: return await self.http_client.request( cast_to=BucketResponse, options=FinalRequestOptions( - method="GET", url=urljoin(API_PREFIX, "bucket") + method="GET", url=urljoin(API_PREFIX_V1, "bucket") ), ) diff --git a/aidial_client/resources/files.py b/aidial_client/resources/files.py index a83f835..be079b8 100644 --- a/aidial_client/resources/files.py +++ b/aidial_client/resources/files.py @@ -6,7 +6,7 @@ import httpx -from aidial_client._constants import API_PREFIX +from aidial_client._constants import API_PREFIX_V1 from aidial_client._internal_types._generic import NoneType from aidial_client._internal_types._http_request import ( FileTypes, @@ -15,7 +15,8 @@ from aidial_client._utils._dict import remove_none from aidial_client.helpers.storage_resource import ( DialStorageResourceMixin, - storage_error_processor, + StorageResourceTypeV1, + _storage_error_processor, ) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata @@ -38,7 +39,7 @@ def _move_copy_body( class Files(Resource, DialStorageResourceMixin): metadata: Metadata - resource_type: str = "files" + resource_type: StorageResourceTypeV1 = "files" def upload( self, @@ -51,7 +52,7 @@ def upload( cast_to=FileItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), files={"file": file}, headers=remove_none( { @@ -60,7 +61,7 @@ def upload( } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def download( @@ -72,7 +73,7 @@ def download( response = self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @@ -85,14 +86,14 @@ def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), headers=remove_none( { "If-Match": etag_if_match, } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def move_to( @@ -105,10 +106,10 @@ def move_to( cast_to=NoneType, options=FinalRequestOptions( method="POST", - url=urljoin(API_PREFIX, "ops/resource/move"), + url=urljoin(API_PREFIX_V1, "ops/resource/move"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def copy_to( @@ -121,10 +122,10 @@ def copy_to( cast_to=NoneType, options=FinalRequestOptions( method="POST", - url=urljoin(API_PREFIX, "ops/resource/copy"), + url=urljoin(API_PREFIX_V1, "ops/resource/copy"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def get_metadata( @@ -144,7 +145,7 @@ def get_metadata( class AsyncFiles(AsyncResource, DialStorageResourceMixin): metadata: AsyncMetadata - resource_type: str = "files" + resource_type: StorageResourceTypeV1 = "files" async def upload( self, @@ -157,7 +158,7 @@ async def upload( cast_to=FileItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), files={"file": file}, headers=remove_none( { @@ -166,7 +167,7 @@ async def upload( } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def download( @@ -178,7 +179,7 @@ async def download( response = await self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @@ -191,7 +192,7 @@ async def stream_download( options, filename = self._prepare_download_request(url, etag_if_match) async with self.http_client.stream( options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) as response: yield FileDownloadResponse(response=response, filename=filename) @@ -204,14 +205,14 @@ async def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), headers=remove_none( { "If-Match": etag_if_match, } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def move_to( @@ -224,10 +225,10 @@ async def move_to( cast_to=NoneType, options=FinalRequestOptions( method="POST", - url=urljoin(API_PREFIX, "ops/resource/move"), + url=urljoin(API_PREFIX_V1, "ops/resource/move"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def copy_to( @@ -240,10 +241,10 @@ async def copy_to( cast_to=NoneType, options=FinalRequestOptions( method="POST", - url=urljoin(API_PREFIX, "ops/resource/copy"), + url=urljoin(API_PREFIX_V1, "ops/resource/copy"), json_data=_move_copy_body(self, source, destination, overwrite), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def get_metadata( diff --git a/aidial_client/resources/metadata.py b/aidial_client/resources/metadata.py index 0e12f16..799c3af 100644 --- a/aidial_client/resources/metadata.py +++ b/aidial_client/resources/metadata.py @@ -3,11 +3,11 @@ from typing_extensions import assert_never -from aidial_client._constants import METADATA_PREFIX +from aidial_client._constants import METADATA_PREFIX_V1 from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none from aidial_client.helpers.storage_resource import ( - StorageResourceType, + StorageResourceTypeV1, _percent_encode_relative_url, ) from aidial_client.resources.base import AsyncResource, Resource @@ -19,7 +19,7 @@ def _get_cast_to( - resource: StorageResourceType, + resource: StorageResourceTypeV1, ) -> type[FileMetadata] | type[ConversationMetadata] | type[PromptMetadata]: if resource == "files": return FileMetadata @@ -64,7 +64,7 @@ def get( def get( self, - resource: StorageResourceType, + resource: StorageResourceTypeV1, relative_url: str, *, limit: int | None = None, @@ -75,7 +75,7 @@ def get( options=FinalRequestOptions( method="GET", url=urljoin( - METADATA_PREFIX, + METADATA_PREFIX_V1, _percent_encode_relative_url(relative_url), ), params=remove_none({"limit": limit, "token": token}), @@ -116,7 +116,7 @@ async def get( async def get( self, - resource: StorageResourceType, + resource: StorageResourceTypeV1, relative_url: str, *, limit: int | None = None, @@ -127,7 +127,7 @@ async def get( options=FinalRequestOptions( method="GET", url=urljoin( - METADATA_PREFIX, + METADATA_PREFIX_V1, _percent_encode_relative_url(relative_url), ), params=remove_none({"limit": limit, "token": token}), diff --git a/aidial_client/resources/prompts.py b/aidial_client/resources/prompts.py index be16626..fc18db7 100644 --- a/aidial_client/resources/prompts.py +++ b/aidial_client/resources/prompts.py @@ -3,13 +3,14 @@ from urllib.parse import urljoin from aidial_client._compatibility.pydantic import PYDANTIC_V2 -from aidial_client._constants import API_PREFIX +from aidial_client._constants import API_PREFIX_V1 from aidial_client._internal_types._generic import NoneType from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none from aidial_client.helpers.storage_resource import ( DialStorageResourceMixin, - storage_error_processor, + StorageResourceTypeV1, + _storage_error_processor, ) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.resources.metadata import AsyncMetadata, Metadata @@ -25,7 +26,7 @@ def _prompt_to_json(prompt: Prompt) -> dict[str, Any]: class Prompts(Resource, DialStorageResourceMixin): metadata: Metadata - resource_type: str = "prompts" + resource_type: StorageResourceTypeV1 = "prompts" def save( self, @@ -38,7 +39,7 @@ def save( cast_to=PromptItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), json_data=_prompt_to_json(prompt), headers=remove_none( { @@ -47,7 +48,7 @@ def save( } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def get(self, url: str | PurePosixPath) -> Prompt: @@ -56,9 +57,9 @@ def get(self, url: str | PurePosixPath) -> Prompt: cast_to=Prompt, options=FinalRequestOptions( method="GET", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def delete( @@ -70,14 +71,14 @@ def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), headers=remove_none( { "If-Match": etag_if_match, } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: @@ -89,7 +90,7 @@ def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: class AsyncPrompts(AsyncResource, DialStorageResourceMixin): metadata: AsyncMetadata - resource_type: str = "prompts" + resource_type: StorageResourceTypeV1 = "prompts" async def save( self, @@ -102,7 +103,7 @@ async def save( cast_to=PromptItem, options=FinalRequestOptions( method="PUT", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), json_data=_prompt_to_json(prompt), headers=remove_none( { @@ -111,7 +112,7 @@ async def save( } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def get(self, url: str | PurePosixPath) -> Prompt: @@ -120,9 +121,9 @@ async def get(self, url: str | PurePosixPath) -> Prompt: cast_to=Prompt, options=FinalRequestOptions( method="GET", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def delete( @@ -134,14 +135,14 @@ async def delete( cast_to=NoneType, options=FinalRequestOptions( method="DELETE", - url=urljoin(API_PREFIX, self.get_api_path(url)), + url=urljoin(API_PREFIX_V1, self.get_api_path(url)), headers=remove_none( { "If-Match": etag_if_match, } ), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) async def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata: diff --git a/aidial_client/resources/skills.py b/aidial_client/resources/skills.py index 6074018..205462a 100644 --- a/aidial_client/resources/skills.py +++ b/aidial_client/resources/skills.py @@ -1,18 +1,38 @@ -from collections.abc import AsyncIterator +""" +Chained references to DIAL Core's ``/v2/skills`` API. + +A skill is a folder-shaped resource: the whole skill is addressed as a unit at +"skills/{bucket}/{path}", and its bundled files hang off +"skills/{bucket}/{path}/files/{filePath}". + +Rather than taking a hand-built URL per call, the resource is a reference that +is narrowed step by step - ``client.skills / "writing" / "tone-of-voice"`` - +so the URL is assembled from validated segments and cannot be typed wrong. +Each narrowing returns a new reference; references are immutable and issue no +requests until a terminal call (``list``/``read``/``download``/``stream``). + +The invariant that splits the two kinds of error: **constructing a reference +validates segment syntax; a terminal call validates that the reference has +enough path for its route.** +""" + +from collections.abc import AsyncIterator, Awaitable, Callable, Sequence from contextlib import asynccontextmanager -from pathlib import PurePosixPath -from urllib.parse import unquote, urljoin +from urllib.parse import unquote import httpx +from typing_extensions import Self, overload -from aidial_client._constants import API_V2_PREFIX, METADATA_V2_PREFIX +from aidial_client._constants import API_PREFIX_V2, METADATA_PREFIX_V2 from aidial_client._exception import InvalidDialURLError from aidial_client._internal_types._http_request import FinalRequestOptions from aidial_client._utils._dict import remove_none from aidial_client.helpers.storage_resource import ( DialStorageResourceMixin, + StorageResourceTypeV2, _percent_encode_relative_url, - storage_error_processor, + _storage_error_processor, + split_relative_segments, ) from aidial_client.resources.base import AsyncResource, Resource from aidial_client.types.file import FileDownloadResponse @@ -20,367 +40,456 @@ # DIAL Core reserves this path segment to keep the # ".../{path}/files/{filePath}" grammar unambiguous. -FILES_SEGMENT = "files" - - -def _relative_path_segments(path: str, param: str) -> list[str]: - """ - Validate a path relative to the skill root and split it into segments. - - Unlike the ``url`` argument, this path is concatenated onto an - already-parsed api path and never goes back through the url parser, so - nothing else would catch a traversal segment. ``urljoin`` resolves "." and - ".." while building the request, which shifts the bucket segment: a - ``file_path`` of "../../../other-bucket/their-skill/files/SKILL.md" turns a - validated "skills/my-bucket/my-skill" into a request against - ``other-bucket``. Reject those segments instead. - - Each segment is checked *as it will decode*, because - ``_percent_encode_relative_url`` normalizes with ``unquote`` before quoting: - "%2e%2e" would otherwise pass a literal check here and still reach - ``urljoin`` as "..", and "%2f" would smuggle in a separator. - """ - if not path.strip(): - raise InvalidDialURLError(f"{param} must not be empty") - if path.startswith("/"): - raise InvalidDialURLError( - f"{param} must be relative to the skill root, got: {path}" - ) +_FILES_SEGMENT = "files" - segments = path.split("/") - decoded = [unquote(segment) for segment in segments] - if any(segment in (".", "..") for segment in decoded): - raise InvalidDialURLError( - f'"." and ".." are not allowed in {param}, got: {path}' - ) - if any("/" in segment for segment in decoded): + +def _encode(segments: Sequence[str]) -> str: + """Percent-encode each segment, so none can contribute a separator.""" + return "/".join(_percent_encode_relative_url(seg) for seg in segments) + + +def _listing_params( + limit: int | None, + token: str | None, + recursive: bool | None, +) -> dict[str, object]: + return remove_none({"limit": limit, "token": token, "recursive": recursive}) + + +def _validate_bucket(bucket: str) -> str: + """A bucket is a URL segment too, so it gets the same treatment.""" + segments = split_relative_segments(bucket, "bucket") + if len(segments) != 1: raise InvalidDialURLError( - f"An encoded path separator is not allowed in {param}, got: {path}" - ) - # A trailing slash is allowed (it denotes a folder) but an interior empty - # segment is a malformed path. - if any(segment == "" for segment in segments[:-1]): - raise InvalidDialURLError(f"Empty path segment in {param}, got: {path}") - return segments - - -class SkillsMixin(DialStorageResourceMixin): - """ - URL and request shaping shared by the sync and async skills resources. - - A skill is a folder-shaped resource: the whole skill is addressed as a unit - at "skills/{bucket}/{path}", and its bundled files hang off - "skills/{bucket}/{path}/files/{filePath}". - """ - - resource_type: str = "skills" - api_prefix: str = API_V2_PREFIX - - def _files_path( - self, url: str | PurePosixPath, path: str | None = None - ) -> str: - api_path = f"{self.get_api_path(url)}/{FILES_SEGMENT}" - # None is the "unset" signal; "" goes through the same validation as - # file_path so the two entry points agree. - if path is None: - return api_path - - segments = _relative_path_segments(path, "path") - if segments[-1] == "": - # Scoping to a folder - drop the trailing empty segment. - segments = segments[:-1] - if not segments: - return api_path - relative = _percent_encode_relative_url("/".join(segments)) - return f"{api_path}/{relative}" - - @staticmethod - def _listing_params( - limit: int | None, - token: str | None, - recursive: bool | None, - ) -> dict[str, object]: - return remove_none( - {"limit": limit, "token": token, "recursive": recursive} + f"bucket must be a single path segment, got: {bucket!r}" ) + return segments[0] + + +class _RefCommon(DialStorageResourceMixin): + """State and URL parsing shared by both kinds of reference.""" - def _prepare_metadata_request( + resource_type: StorageResourceTypeV2 = "skills" + bucket: str | None = None + """None means "the caller's own bucket", resolved at terminal-call time.""" + + def _split_url(self, url: str) -> tuple[str, tuple[str, ...]]: + # allow_empty_bucket_path is unconditional: a reference does not know + # which terminal call comes next, so "skills/my-bucket" must parse and + # the shape guards decide later whether an empty path is acceptable. + parsed = self.get_storage_resource(url, allow_empty_bucket_path=True) + if parsed.bucket_path is None: + return parsed.bucket, () + return parsed.bucket, tuple(parsed.bucket_path.rstrip("/").split("/")) + + +class _SkillsRefBase(_RefCommon): + """A skill, a grouping folder, or the bucket root.""" + + path: tuple[str, ...] = () + + @overload + def __call__(self, *, path: str, bucket: str | None = None) -> Self: ... + @overload + def __call__(self, *, bucket: str) -> Self: ... + @overload + def __call__(self, *, url: str) -> Self: ... + + def __call__( self, - url: str | PurePosixPath, *, - limit: int | None, - token: str | None, - recursive: bool | None, - ) -> FinalRequestOptions: - # Core lists the bucket root when {path} is empty, so a bucket-root - # url ("skills/my-bucket") is a valid target here. - api_path = self.get_api_path(url, allow_bucket_root=True) - # This route always addresses a folder, and the separator after - # {bucket} in Core's route regex is literal: - # ^/v2/metadata/skills/(?[a-zA-Z0-9]+)/(?.*)$ - # so an empty {path} only matches with a trailing slash. api_path - # comes from PurePosixPath and never carries one. Core strips a - # trailing slash off {path} again, so appending it unconditionally - # leaves the deeper paths resolving to the same folder as before. - return FinalRequestOptions( + path: str | None = None, + bucket: str | None = None, + url: str | None = None, + ) -> Self: + if url is not None: + if path is not None or bucket is not None: + raise TypeError("url= cannot be combined with bucket= or path=") + new_bucket, segments = self._split_url(url) + return self.copy(update={"bucket": new_bucket, "path": segments}) + if path is None and bucket is None: + raise TypeError("one of url=, bucket= or path= is required") + + update: dict[str, object] = {} + if bucket is not None: + update["bucket"] = _validate_bucket(bucket) + if path is not None: + update["path"] = ( + *self.path, + *split_relative_segments(path, "path"), + ) + return self.copy(update=update) + + def __truediv__(self, path: str) -> Self: + return self(path=path) + + def __repr__(self) -> str: + bucket = self.bucket if self.bucket is not None else "" + return f"{type(self).__name__}('skills/{bucket}/{'/'.join(self.path)}')" + + def _require_skill_path(self, operation: str) -> None: + if not self.path: + raise InvalidDialURLError( + f"{operation} addresses one skill, but this reference points" + " at the bucket root. Descend to a skill first, e.g." + ' client.skills / "my-skill".' + ) + + def _metadata_url(self, bucket: str) -> str: + """``GET /v2/metadata/skills/{bucket}/{path}/`` - a folder listing. + + The separator after ``{bucket}`` in Core's route regex is literal, so + an empty ``{path}`` only matches with the trailing slash. Core strips a + trailing slash off ``{path}`` again, so appending it unconditionally + leaves deeper paths resolving to the same folder as before. + """ + encoded = _encode(self.path) + suffix = f"{encoded}/" if encoded else "" + return f"{METADATA_PREFIX_V2}skills/{bucket}/{suffix}" + + def _archive_url(self, bucket: str) -> tuple[FinalRequestOptions, str]: + """``GET /v2/skills/{bucket}/{path}`` - the skill as a ZIP archive. + + Core answers ``application/zip`` without a ``Content-Disposition`` + header, so name the archive after the skill. + """ + options = FinalRequestOptions( method="GET", - url=urljoin(METADATA_V2_PREFIX, f"{api_path}/"), - params=self._listing_params(limit, token, recursive), + url=f"{API_PREFIX_V2}skills/{bucket}/{_encode(self.path)}", ) + return options, f"{unquote(self.path[-1])}.zip" + + +class _SkillFilesRefBase(_RefCommon): + """The files bundled inside one skill, or a subfolder of them.""" + + skill_path: tuple[str, ...] = () + path: tuple[str, ...] = () + + @overload + def __call__(self, *, path: str) -> Self: ... + @overload + def __call__(self, *, url: str) -> Self: ... - def _prepare_list_files_request( + def __call__( self, - url: str | PurePosixPath, *, - path: str | None, - limit: int | None, - token: str | None, - recursive: bool | None, - ) -> FinalRequestOptions: - return FinalRequestOptions( - method="GET", - url=urljoin(METADATA_V2_PREFIX, self._files_path(url, path)), - params=self._listing_params(limit, token, recursive), + path: str | None = None, + url: str | None = None, + ) -> Self: + if url is not None: + if path is not None: + raise TypeError("url= cannot be combined with path=") + bucket, skill_path, file_path = self._split_files_url(url) + return self.copy( + update={ + "bucket": bucket, + "skill_path": skill_path, + "path": file_path, + } + ) + if path is None: + raise TypeError("one of url= or path= is required") + return self.copy( + update={ + "path": (*self.path, *split_relative_segments(path, "path")) + } ) - def _prepare_get_file_request( - self, - url: str | PurePosixPath, - file_path: str, - ) -> tuple[FinalRequestOptions, str]: - segments = _relative_path_segments(file_path, "file_path") - if segments[-1] == "": + def __truediv__(self, path: str) -> Self: + return self(path=path) + + def __repr__(self) -> str: + bucket = self.bucket if self.bucket is not None else "" + skill = "/".join(self.skill_path) + return ( + f"{type(self).__name__}('skills/{bucket}/{skill}" + f"/{_FILES_SEGMENT}/{'/'.join(self.path)}')" + ) + + def _split_files_url( + self, url: str + ) -> tuple[str, tuple[str, ...], tuple[str, ...]]: + """Split ``skills/{bucket}/{path}/files/{filePath}`` back into parts. + + The search starts at index 1 because Core's route requires at least + one segment before ``files`` (``(?.+?)/files/``), so a skill + named "files" resolves the same way here as it does there. + """ + bucket, segments = self._split_url(url) + try: + index = segments.index(_FILES_SEGMENT, 1) + except ValueError: + raise InvalidDialURLError( + f'url must address a file inside a skill ("…/{_FILES_SEGMENT}' + f'/…"), got: {url!r}' + ) from None + return bucket, segments[:index], segments[index + 1 :] + + def _require_skill_path(self, operation: str) -> None: + if not self.skill_path: raise InvalidDialURLError( - f"file_path points to a directory, not a file: {file_path}" + f"{operation} lists the files of one skill, but this" + " reference points at the bucket root. Descend to a skill" + ' first, e.g. (client.skills / "my-skill").files.' ) - relative = _percent_encode_relative_url("/".join(segments)) - api_path = f"{self.get_api_path(url)}/{FILES_SEGMENT}/{relative}" - # No If-Match: unlike the /v1 reads, neither v2 read honours it - - # ComplexResourceController.getFile never calls ProxyUtil.etag, and - # the operation declares no If-Match parameter and no 412 response. - options = FinalRequestOptions( - method="GET", - url=urljoin(API_V2_PREFIX, api_path), + def _require_file_path(self, operation: str) -> None: + self._require_skill_path(operation) + if not self.path: + raise InvalidDialURLError( + f"{operation} addresses one file, but no file path was given." + ' Use skill.files(path="SKILL.md") to name it.' + ) + + def _files_metadata_url(self, bucket: str) -> str: + """``GET /v2/metadata/skills/{b}/{p}/files[/{filePath}]``.""" + base = ( + f"{METADATA_PREFIX_V2}skills/{bucket}" + f"/{_encode(self.skill_path)}/{_FILES_SEGMENT}" ) - return options, unquote(segments[-1]) + tail = _encode(self.path) + return f"{base}/{tail}" if tail else base - def _prepare_download_archive_request( - self, - url: str | PurePosixPath, - ) -> tuple[FinalRequestOptions, str]: - api_path = self.get_api_path(url) - # See _prepare_get_file_request: Core ignores If-Match on this read - # too (ComplexResourceController.get). + def _file_url(self, bucket: str) -> tuple[FinalRequestOptions, str]: + """``GET /v2/skills/{b}/{p}/files/{filePath}`` - one bundled file.""" options = FinalRequestOptions( method="GET", - url=urljoin(API_V2_PREFIX, api_path), + url=( + f"{API_PREFIX_V2}skills/{bucket}" + f"/{_encode(self.skill_path)}/{_FILES_SEGMENT}" + f"/{_encode(self.path)}" + ), + ) + # The path is percent-encoded; return a human-readable filename. + return options, unquote(self.path[-1]) + + +class SkillsRef(Resource, _SkillsRefBase): + class Config: + arbitrary_types_allowed = True + allow_mutation = False + + resolve_bucket: Callable[[], str] + + def _bucket(self) -> str: + if self.bucket is not None: + return self.bucket + return self.resolve_bucket() + + @property + def files(self) -> "SkillFilesRef": + return SkillFilesRef( + http_client=self.http_client, + dial_api_url=self.dial_api_url, + bucket=self.bucket, + skill_path=self.path, + resolve_bucket=self.resolve_bucket, ) - # Core answers application/zip without a Content-Disposition header, - # so name the archive after the skill. - filename = f"{unquote(PurePosixPath(api_path).name)}.zip" - return options, filename - -class Skills(Resource, SkillsMixin): - def get_metadata( + def list( self, - url: str | PurePosixPath, *, limit: int | None = None, token: str | None = None, recursive: bool | None = None, ) -> SkillMetadata: """ - List the skills and grouping folders at ``url``. + List the skills and grouping folders this reference points at. - Pass a bucket-root url (``client.my_skills_home()``) to list the whole - bucket. Follow ``next_token`` until it is ``None`` to read every page. + Follow ``next_token`` until it is ``None`` to read every page. """ return self.http_client.request( cast_to=SkillMetadata, - options=self._prepare_metadata_request( - url, limit=limit, token=token, recursive=recursive + options=FinalRequestOptions( + method="GET", + url=self._metadata_url(self._bucket()), + params=_listing_params(limit, token, recursive), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) - def list_files( + def download(self) -> FileDownloadResponse: + """Download the whole skill as a ZIP archive.""" + self._require_skill_path("download()") + options, filename = self._archive_url(self._bucket()) + response = self.http_client.request( + cast_to=httpx.Response, + options=options, + on_http_error=_storage_error_processor, + ) + return FileDownloadResponse(response=response, filename=filename) + + +class SkillFilesRef(Resource, _SkillFilesRefBase): + class Config: + arbitrary_types_allowed = True + allow_mutation = False + + resolve_bucket: Callable[[], str] + + def _bucket(self) -> str: + if self.bucket is not None: + return self.bucket + return self.resolve_bucket() + + def list( self, - url: str | PurePosixPath, *, - path: str | None = None, limit: int | None = None, token: str | None = None, recursive: bool | None = None, ) -> SkillFileMetadata: """ - List the files of the skill at ``url``, optionally scoped to the - ``path`` subfolder inside it. + List the skill's files, optionally scoped to a subfolder. A page may hold fewer entries than ``limit``, so follow ``next_token`` - until it is ``None`` rather than assuming a single page is complete. + until it is ``None`` rather than assuming one page is complete. """ + self._require_skill_path("list()") return self.http_client.request( cast_to=SkillFileMetadata, - options=self._prepare_list_files_request( - url, - path=path, - limit=limit, - token=token, - recursive=recursive, + options=FinalRequestOptions( + method="GET", + url=self._files_metadata_url(self._bucket()), + params=_listing_params(limit, token, recursive), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) - def get_file( - self, - url: str | PurePosixPath, - file_path: str, - ) -> FileDownloadResponse: - """ - Download a single file bundled in the skill at ``url``. - - ``file_path`` is relative to the skill root, e.g. "SKILL.md" or - "references/api-schema.md". - """ - options, filename = self._prepare_get_file_request(url, file_path) + def read(self) -> FileDownloadResponse: + """Download the single file this reference names.""" + self._require_file_path("read()") + options, filename = self._file_url(self._bucket()) response = self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) - def download( - self, - url: str | PurePosixPath, - ) -> FileDownloadResponse: - """ - Download the whole skill at ``url`` as a ZIP archive. - """ - options, filename = self._prepare_download_archive_request(url) - response = self.http_client.request( - cast_to=httpx.Response, - options=options, - on_http_error=storage_error_processor, - ) - return FileDownloadResponse(response=response, filename=filename) +class AsyncSkillsRef(AsyncResource, _SkillsRefBase): + class Config: + arbitrary_types_allowed = True + allow_mutation = False -class AsyncSkills(AsyncResource, SkillsMixin): - async def get_metadata( - self, - url: str | PurePosixPath, - *, - limit: int | None = None, - token: str | None = None, - recursive: bool | None = None, - ) -> SkillMetadata: - """ - List the skills and grouping folders at ``url``. + resolve_bucket: Callable[[], Awaitable[str]] - Pass a bucket-root url (``await client.my_skills_home()``) to list the - whole bucket. Follow ``next_token`` until it is ``None`` to read every - page. - """ - return await self.http_client.request( - cast_to=SkillMetadata, - options=self._prepare_metadata_request( - url, limit=limit, token=token, recursive=recursive - ), - on_http_error=storage_error_processor, + async def _bucket(self) -> str: + if self.bucket is not None: + return self.bucket + return await self.resolve_bucket() + + @property + def files(self) -> "AsyncSkillFilesRef": + return AsyncSkillFilesRef( + http_client=self.http_client, + dial_api_url=self.dial_api_url, + bucket=self.bucket, + skill_path=self.path, + resolve_bucket=self.resolve_bucket, ) - async def list_files( + async def list( self, - url: str | PurePosixPath, *, - path: str | None = None, limit: int | None = None, token: str | None = None, recursive: bool | None = None, - ) -> SkillFileMetadata: + ) -> SkillMetadata: """ - List the files of the skill at ``url``, optionally scoped to the - ``path`` subfolder inside it. + List the skills and grouping folders this reference points at. - A page may hold fewer entries than ``limit``, so follow ``next_token`` - until it is ``None`` rather than assuming a single page is complete. + Follow ``next_token`` until it is ``None`` to read every page. """ return await self.http_client.request( - cast_to=SkillFileMetadata, - options=self._prepare_list_files_request( - url, - path=path, - limit=limit, - token=token, - recursive=recursive, + cast_to=SkillMetadata, + options=FinalRequestOptions( + method="GET", + url=self._metadata_url(await self._bucket()), + params=_listing_params(limit, token, recursive), ), - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) - async def get_file( - self, - url: str | PurePosixPath, - file_path: str, - ) -> FileDownloadResponse: - """ - Download a single file bundled in the skill at ``url``. - - ``file_path`` is relative to the skill root, e.g. "SKILL.md" or - "references/api-schema.md". - """ - options, filename = self._prepare_get_file_request(url, file_path) + async def download(self) -> FileDownloadResponse: + """Download the whole skill as a ZIP archive.""" + self._require_skill_path("download()") + options, filename = self._archive_url(await self._bucket()) response = await self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @asynccontextmanager - async def stream_file( - self, - url: str | PurePosixPath, - file_path: str, - ) -> AsyncIterator[FileDownloadResponse]: - """ - Stream a single file bundled in the skill at ``url``. - """ - options, filename = self._prepare_get_file_request(url, file_path) + async def stream_download(self) -> AsyncIterator[FileDownloadResponse]: + """Stream the whole skill as a ZIP archive.""" + self._require_skill_path("stream_download()") + options, filename = self._archive_url(await self._bucket()) async with self.http_client.stream( options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) as response: yield FileDownloadResponse(response=response, filename=filename) - async def download( + +class AsyncSkillFilesRef(AsyncResource, _SkillFilesRefBase): + class Config: + arbitrary_types_allowed = True + allow_mutation = False + + resolve_bucket: Callable[[], Awaitable[str]] + + async def _bucket(self) -> str: + if self.bucket is not None: + return self.bucket + return await self.resolve_bucket() + + async def list( self, - url: str | PurePosixPath, - ) -> FileDownloadResponse: + *, + limit: int | None = None, + token: str | None = None, + recursive: bool | None = None, + ) -> SkillFileMetadata: """ - Download the whole skill at ``url`` as a ZIP archive. + List the skill's files, optionally scoped to a subfolder. + + A page may hold fewer entries than ``limit``, so follow ``next_token`` + until it is ``None`` rather than assuming one page is complete. """ - options, filename = self._prepare_download_archive_request(url) + self._require_skill_path("list()") + return await self.http_client.request( + cast_to=SkillFileMetadata, + options=FinalRequestOptions( + method="GET", + url=self._files_metadata_url(await self._bucket()), + params=_listing_params(limit, token, recursive), + ), + on_http_error=_storage_error_processor, + ) + + async def read(self) -> FileDownloadResponse: + """Download the single file this reference names.""" + self._require_file_path("read()") + options, filename = self._file_url(await self._bucket()) response = await self.http_client.request( cast_to=httpx.Response, options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) return FileDownloadResponse(response=response, filename=filename) @asynccontextmanager - async def stream_download( - self, - url: str | PurePosixPath, - ) -> AsyncIterator[FileDownloadResponse]: - """ - Stream the whole skill at ``url`` as a ZIP archive. - """ - options, filename = self._prepare_download_archive_request(url) + async def stream(self) -> AsyncIterator[FileDownloadResponse]: + """Stream the single file this reference names.""" + self._require_file_path("stream()") + options, filename = self._file_url(await self._bucket()) async with self.http_client.stream( options=options, - on_http_error=storage_error_processor, + on_http_error=_storage_error_processor, ) as response: yield FileDownloadResponse(response=response, filename=filename) diff --git a/aidial_client/types/metadata.py b/aidial_client/types/metadata.py index 132aef4..956021e 100644 --- a/aidial_client/types/metadata.py +++ b/aidial_client/types/metadata.py @@ -1,9 +1,13 @@ -from typing import Literal +from typing import Any, Literal from aidial_client._compatibility.pydantic import PYDANTIC_V2 +from aidial_client._compatibility.pydantic_v1 import validator from aidial_client._internal_types._model import ExtraAllowModel from aidial_client._utils._alias import to_camel +if PYDANTIC_V2: + from pydantic import field_validator + class BaseMetadata(ExtraAllowModel): if PYDANTIC_V2: @@ -71,13 +75,22 @@ class PromptMetadata(BaseMetadata): resource_type: Literal["PROMPT"] +# Fixing the bug in DIAL Core: a non-recursive listing of a skill's files +# reports its subfolders with nodeType "ITEM". +# https://github.com/epam/ai-dial-core/issues/1912 +def _node_type_from_url(node_type: Any, url: Any) -> Any: + if not isinstance(url, str): + return node_type + return "FOLDER" if url.endswith("/") else "ITEM" + + class SkillItem(ResourceItemMetadata): """ A node in the skills listing: a skill (ITEM) or a grouping folder (FOLDER). - DIAL Core builds these from the folder marker's listing metadata without - reading the marker body, so no ``etag`` and no skill name/description are - carried here - they are available via a whole-resource GET. + ``node_type`` is taken from the response as-is. The bug worked around in + ``SkillFileItem`` was observed only on the file listing inside a skill, + not on this one. """ node_type: Literal["FOLDER", "ITEM"] @@ -95,23 +108,52 @@ class SkillFileItem(ResourceItemMetadata): """ A file or a subfolder inside a skill. - Use the trailing "/" of ``url`` to tell them apart, not ``node_type``: - DIAL Core builds every entry of this listing as a plain item and never - overrides its node type, so a subfolder is reported as ``"ITEM"`` too. - ``node_type`` is left as a union because that is a Core-side bug, and a - fix upstream should not turn into a parsing error here. + ``node_type`` is derived from ``url`` rather than taken from the + response - see ``_node_type_from_url``. A recursive listing is flattened + and contains no subfolder entries at all, so the two kinds only ever + appear together in a non-recursive one. - Unlike the /v1 files listing, no ``content_length`` or ``content_type`` - is carried - Core copies only the etag, timestamps and author onto these - entries. + Sparser than the /v1 files listing: no ``content_length``, no + ``content_type``, and in observed responses no ``etag`` either. + Subfolder entries carry no timestamps. """ node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] + if PYDANTIC_V2: + + @field_validator("node_type") + @classmethod + def _derive_node_type_v2(cls, value: Any, info: Any) -> Any: + return _node_type_from_url(value, info.data.get("url")) + + else: + + @validator("node_type") + def _derive_node_type_v1( # noqa: N805 + cls, value: Any, values: dict[str, Any] + ) -> Any: + return _node_type_from_url(value, values.get("url")) + class SkillFileMetadata(BaseMetadata): node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] next_token: str | None = None items: list[SkillFileItem] | None = None + + if PYDANTIC_V2: + + @field_validator("node_type") + @classmethod + def _derive_node_type_v2(cls, value: Any, info: Any) -> Any: + return _node_type_from_url(value, info.data.get("url")) + + else: + + @validator("node_type") + def _derive_node_type_v1( # noqa: N805 + cls, value: Any, values: dict[str, Any] + ) -> Any: + return _node_type_from_url(value, values.get("url")) diff --git a/tests/helpers/test_storage_resource_mixin.py b/tests/helpers/test_storage_resource_mixin.py index a12ce0c..f5d3a2d 100644 --- a/tests/helpers/test_storage_resource_mixin.py +++ b/tests/helpers/test_storage_resource_mixin.py @@ -104,7 +104,7 @@ def test_get_api_path_missing_bucket(resource_type, url): mixin = DialStorageResourceMixin( resource_type=resource_type, dial_api_url=DIAL_API_URL ) - with pytest.raises(InvalidDialURLError, match="Missing bucket in URL"): + with pytest.raises(InvalidDialURLError, match="Missing bucket path in URL"): mixin.get_api_path(url) diff --git a/tests/helpers/test_storage_resource_parser.py b/tests/helpers/test_storage_resource_parser.py index 400a6ab..c198610 100644 --- a/tests/helpers/test_storage_resource_parser.py +++ b/tests/helpers/test_storage_resource_parser.py @@ -196,7 +196,6 @@ def test_parse_v2_skill_resource(url, expected_api_path): url=url, dial_api_url="https://dial.core/v2/", expected_resource_type="skills", - api_prefix="v2/", ) assert result.resource_type == "skills" assert result.bucket == "my-bucket" @@ -204,47 +203,43 @@ def test_parse_v2_skill_resource(url, expected_api_path): @pytest.mark.parametrize( - "url, dial_api_url, resource_type, api_prefix", + "url, dial_api_url, resource_type", [ - ("skills/my-bucket", "https://dial.core/v2/", "skills", "v2/"), - ("skills/my-bucket/", "https://dial.core/v2/", "skills", "v2/"), - ("files/my-bucket", "https://dial.core/v1/", "files", "v1/"), + ("skills/my-bucket", "https://dial.core/v2/", "skills"), + ("skills/my-bucket/", "https://dial.core/v2/", "skills"), + ("files/my-bucket", "https://dial.core/v1/", "files"), ], ) -def test_parse_bucket_root_when_allowed( - url, dial_api_url, resource_type, api_prefix -): +def test_parse_bucket_root_when_allowed(url, dial_api_url, resource_type): result = parse_storage_resource( url=url, dial_api_url=dial_api_url, expected_resource_type=resource_type, - api_prefix=api_prefix, - allow_bucket_root=True, + allow_empty_bucket_path=True, ) assert result.bucket == "my-bucket" - assert result.bucket_path == "" + assert result.bucket_path is None assert result.filename is None assert result.api_path == f"{resource_type}/my-bucket" @pytest.mark.parametrize( - "url, dial_api_url, resource_type, api_prefix", + "url, dial_api_url, resource_type", [ - ("skills/my-bucket", "https://dial.core/v2/", "skills", "v2/"), - ("files/my-bucket", "https://dial.core/v1/", "files", "v1/"), + ("skills/my-bucket", "https://dial.core/v2/", "skills"), + ("files/my-bucket", "https://dial.core/v1/", "files"), ], ) def test_parse_bucket_root_rejected_by_default( - url, dial_api_url, resource_type, api_prefix + url, dial_api_url, resource_type ): # A two-segment path is ambiguous ("files/my-file.txt" has the same # shape), so bucket-root parsing stays opt-in. - with pytest.raises(InvalidDialURLError, match="Missing bucket in URL"): + with pytest.raises(InvalidDialURLError, match="Missing bucket path in URL"): parse_storage_resource( url=url, dial_api_url=dial_api_url, expected_resource_type=resource_type, - api_prefix=api_prefix, ) @@ -256,7 +251,6 @@ def test_parse_rejects_v2_api_prefix_as_relative_part(): url="v2/skills/my-bucket/my-skill", dial_api_url="https://dial.core/v2/", expected_resource_type="skills", - api_prefix="v2/", ) @@ -267,3 +261,96 @@ def test_parse_rejects_skills_url_for_v1_resource(): dial_api_url="https://dial.core/v1/", expected_resource_type="files", ) + + +@pytest.mark.parametrize( + "url, expected_bucket, expected_bucket_path", + [ + # The urls apps actually ship in their "skills" config: the shared + # public bucket, a nested grouping folder, and a generated bucket id. + # Core's bucket group is [a-zA-Z0-9]+, which both bucket forms satisfy. + ( + "skills/public/demo/azure-resource-visualizer", + "public", + "demo/azure-resource-visualizer", + ), + ( + "skills/public/all-three-conventionss", + "public", + "all-three-conventionss", + ), + ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/all-three-conventionss", + "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "all-three-conventionss", + ), + ], +) +def test_parse_real_world_skill_urls( + url, expected_bucket, expected_bucket_path +): + result = parse_storage_resource( + url=url, + dial_api_url="https://dial.core/v2/", + expected_resource_type="skills", + ) + assert result.bucket == expected_bucket + assert result.bucket_path == expected_bucket_path + + +@pytest.mark.parametrize( + "url, dial_api_url, resource_type", + [ + # urljoin resolves ".." while building the request, which silently + # retargets another bucket. The parser must reject it first - and it + # does so for /v1 resources too, not just skills. + ("files/my-bucket/../../other/x.txt", "https://dial.core/v1/", "files"), + ("files/my-bucket/../other/x.txt", "https://dial.core/v1/", "files"), + ("prompts/my-bucket/./p.txt", "https://dial.core/v1/", "prompts"), + ("skills/my-bucket/../other/skill", "https://dial.core/v2/", "skills"), + # Checked as it decodes: quote() leaves ".." intact, so a literal + # check on the raw string would miss these. + ( + "skills/my-bucket/%2e%2e/%2e%2e/other/skill", + "https://dial.core/v2/", + "skills", + ), + ("skills/my-bucket/.%2e/other", "https://dial.core/v2/", "skills"), + # An encoded separator would smuggle in an extra path segment. + ("skills/my-bucket/a%2fb", "https://dial.core/v2/", "skills"), + # Interior empty segment. + ("files/my-bucket//x.txt", "https://dial.core/v1/", "files"), + ( + "https://dial.core/v1/files/my-bucket/../other/x.txt", + "https://dial.core/v1/", + "files", + ), + ], +) +def test_parse_rejects_path_traversal(url, dial_api_url, resource_type): + with pytest.raises(InvalidDialURLError): + parse_storage_resource( + url=url, + dial_api_url=dial_api_url, + expected_resource_type=resource_type, + ) + + +@pytest.mark.parametrize( + "url", + [ + # A trailing slash is how DIAL spells "folder" - it must survive. + "files/my-bucket/relative_folder/", + "files/my-bucket/a/deep/folder/", + "https://dial.core/v1/files/my-bucket/relative_folder/", + ], +) +def test_parse_keeps_accepting_folder_urls(url): + result = parse_storage_resource( + url=url, + dial_api_url="https://dial.core/v1/", + expected_resource_type="files", + ) + assert result.bucket == "my-bucket" + assert result.filename is None diff --git a/tests/resources/skills/test_skill_core_routes.py b/tests/resources/skills/test_skill_core_routes.py index 47a052b..8b7761e 100644 --- a/tests/resources/skills/test_skill_core_routes.py +++ b/tests/resources/skills/test_skill_core_routes.py @@ -92,9 +92,7 @@ def _route( def test_bucket_root_listing_is_routable(): - route, groups = _route( - lambda client: client.skills.get_metadata(client.my_skills_home()) - ) + route, groups = _route(lambda client: client.skills.list()) assert route == "COMPLEX_RESOURCE_METADATA" assert groups["bucket"] == BUCKET @@ -104,16 +102,14 @@ def test_bucket_root_listing_is_routable(): def test_grouping_folder_listing_is_routable(): - route, groups = _route( - lambda client: client.skills.get_metadata(f"skills/{BUCKET}/writing") - ) + route, groups = _route(lambda client: (client.skills / "writing").list()) assert route == "COMPLEX_RESOURCE_METADATA" assert groups["path"] == "writing/" def test_list_files_is_routable(): - route, groups = _route(lambda client: client.skills.list_files(SKILL)) + route, groups = _route(lambda client: client.skills(url=SKILL).files.list()) assert route == "COMPLEX_RESOURCE_FILE_METADATA" assert groups["path"] == "writing/toneofvoice" @@ -122,7 +118,7 @@ def test_list_files_is_routable(): def test_list_files_subfolder_is_routable(): route, groups = _route( - lambda client: client.skills.list_files(SKILL, path="references") + lambda client: client.skills(url=SKILL).files(path="references").list() ) assert route == "COMPLEX_RESOURCE_FILE_METADATA" @@ -131,7 +127,7 @@ def test_list_files_subfolder_is_routable(): def test_get_file_is_routable(): route, groups = _route( - lambda client: client.skills.get_file(SKILL, "SKILL.md") + lambda client: client.skills(url=SKILL).files(path="SKILL.md").read() ) assert route == "COMPLEX_RESOURCE_FILE" @@ -140,7 +136,7 @@ def test_get_file_is_routable(): def test_download_is_routable(): - route, groups = _route(lambda client: client.skills.download(SKILL)) + route, groups = _route(lambda client: client.skills(url=SKILL).download()) assert route == "COMPLEX_RESOURCE" assert groups["path"] == "writing/toneofvoice" diff --git a/tests/resources/skills/test_skill_download.py b/tests/resources/skills/test_skill_download.py index 42b849f..f1646cf 100644 --- a/tests/resources/skills/test_skill_download.py +++ b/tests/resources/skills/test_skill_download.py @@ -48,7 +48,7 @@ def test_download_whole_skill_as_zip(): {"content-type": "application/zip", "etag": "aggregate-etag"}, ) - response = client.skills.download(SKILL_URL) + response = client.skills(url=SKILL_URL).download() assert ( captured[0].url.path == "/v2/skills/test-bucket/writing/tone-of-voice" @@ -68,8 +68,8 @@ def test_reads_send_no_if_match(): captured: list[httpx.Request] = [] client = _capturing_client(captured, ZIP_BYTES) - client.skills.download(SKILL_URL) - client.skills.get_file(SKILL_URL, "SKILL.md") + client.skills(url=SKILL_URL).download() + client.skills(url=SKILL_URL).files(path="SKILL.md").read() assert all("if-match" not in request.headers for request in captured) @@ -78,7 +78,7 @@ def test_download_rejects_non_skill_url(): client = _capturing_client([], ZIP_BYTES) with pytest.raises(InvalidDialURLError, match="Invalid resource type"): - client.skills.download("files/test-bucket/folder/file.txt") + client.skills(url="files/test-bucket/folder/file.txt") def test_get_file_returns_bytes(): @@ -89,7 +89,7 @@ def test_get_file_returns_bytes(): {"content-type": "text/markdown", "etag": "aggregate-etag"}, ) - response = client.skills.get_file(SKILL_URL, "SKILL.md") + response = client.skills(url=SKILL_URL).files(path="SKILL.md").read() assert captured[0].url.path == ( "/v2/skills/test-bucket/writing/tone-of-voice/files/SKILL.md" @@ -102,7 +102,11 @@ def test_get_file_percent_encodes_relative_path(): captured: list[httpx.Request] = [] client = _capturing_client(captured, b"schema") - response = client.skills.get_file(SKILL_URL, "references/api schema.md") + response = ( + client.skills(url=SKILL_URL) + .files(path="references/api schema.md") + .read() + ) assert captured[0].url.raw_path.decode() == ( "/v2/skills/test-bucket/writing/tone-of-voice" @@ -116,7 +120,7 @@ def test_get_file_accepts_already_encoded_path(): captured: list[httpx.Request] = [] client = _capturing_client(captured, b"schema") - client.skills.get_file(SKILL_URL, "references/api%20schema.md") + client.skills(url=SKILL_URL).files(path="references/api%20schema.md").read() assert captured[0].url.raw_path.decode() == ( "/v2/skills/test-bucket/writing/tone-of-voice" @@ -128,7 +132,7 @@ def test_get_file_preserves_non_utf8_content(): payload = b"\x89PNG\r\n\x1a\n\xff\xfe" client = _capturing_client([], payload, {"content-type": "image/png"}) - response = client.skills.get_file(SKILL_URL, "assets/logo.png") + response = client.skills(url=SKILL_URL).files(path="assets/logo.png").read() assert response.get_content() == payload @@ -147,7 +151,7 @@ def test_error_mapping(status_code, expected_exception): ) with pytest.raises(expected_exception) as exc_info: - client.skills.get_file(SKILL_URL, "SKILL.md") + client.skills(url=SKILL_URL).files(path="SKILL.md").read() if status_code == 403: assert exc_info.value.status_code == 403 @@ -176,7 +180,7 @@ async def send_mock( client._http_client._internal_http_client.send = cast(Any, send_mock) - async with client.skills.stream_download(SKILL_URL) as response: + async with client.skills(url=SKILL_URL).stream_download() as response: assert response.filename == "tone-of-voice.zip" chunks = [chunk async for chunk in response] assert b"".join(chunks) == b"PK\x03\x04rest" @@ -206,7 +210,9 @@ async def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: client._http_client._internal_http_client.send = cast(Any, send_mock) - async with client.skills.stream_file(SKILL_URL, "SKILL.md") as response: + async with ( + client.skills(url=SKILL_URL).files(path="SKILL.md").stream() as response + ): assert response.filename == "SKILL.md" assert b"".join([c async for c in response]) == b"# skill" @@ -242,7 +248,7 @@ def test_get_file_rejects_traversal_segments(bad_path): client = _capturing_client(captured, b"x") with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): - client.skills.get_file(SKILL_URL, bad_path) + client.skills(url=SKILL_URL).files(path=bad_path).read() assert captured == [] @@ -260,7 +266,7 @@ def test_list_files_rejects_traversal_segments(bad_path): client = _capturing_client(captured, b"x") with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): - client.skills.list_files(SKILL_URL, path=bad_path) + client.skills(url=SKILL_URL).files(path=bad_path).list() assert captured == [] @@ -270,8 +276,8 @@ def test_list_files_rejects_traversal_segments(bad_path): [ ("", "must not be empty"), (" ", "must not be empty"), - ("/abs/path.md", "must be relative to the skill root"), - ("refs/", "points to a directory, not a file"), + ("/abs/path.md", "must be relative"), + ("refs/", "must not end with"), ("a//b.md", "Empty path segment"), ], ) @@ -280,17 +286,19 @@ def test_get_file_rejects_malformed_path(bad_path, message): client = _capturing_client(captured, b"x") with pytest.raises(InvalidDialURLError, match=message): - client.skills.get_file(SKILL_URL, bad_path) + client.skills(url=SKILL_URL).files(path=bad_path).read() assert captured == [] @pytest.mark.asyncio async def test_async_get_file_rejects_traversal(): + # The reference is built synchronously, so a bad segment is rejected + # before anything is awaited - there is no request to intercept. client = AsyncDial(api_key="dummy", base_url="http://dial.core") with pytest.raises(InvalidDialURLError, match=r'"\." and "\.\."'): - await client.skills.get_file(SKILL_URL, "../../../other/s/files/x.md") + client.skills(url=SKILL_URL).files(path="../../../other/s/files/x.md") @pytest.mark.parametrize( @@ -304,18 +312,19 @@ def test_get_file_rejects_encoded_separator(bad_path): client = _capturing_client(captured, b"x") with pytest.raises(InvalidDialURLError, match="encoded path separator"): - client.skills.get_file(SKILL_URL, bad_path) + client.skills(url=SKILL_URL).files(path=bad_path).read() assert captured == [] -def test_list_files_rejects_empty_path_like_get_file(): - # "" is validated the same way for both entry points; None means "unset". +def test_rejects_empty_path(): + # Omitting path= keeps the reference where it is; passing "" is an error, + # not a way to say "no path". captured: list[httpx.Request] = [] client = _capturing_client(captured, b"x") with pytest.raises(InvalidDialURLError, match="path must not be empty"): - client.skills.list_files(SKILL_URL, path="") + client.skills(url=SKILL_URL).files(path="") assert captured == [] @@ -336,7 +345,7 @@ def test_get_file_accepts_legitimate_paths( captured: list[httpx.Request] = [] client = _capturing_client(captured, b"x") - response = client.skills.get_file(SKILL_URL, good_path) + response = client.skills(url=SKILL_URL).files(path=good_path).read() assert captured[0].url.raw_path.decode() == ( f"/v2/skills/test-bucket/writing/tone-of-voice/files/{expected_raw}" diff --git a/tests/resources/skills/test_skill_metadata.py b/tests/resources/skills/test_skill_metadata.py index 1915b28..020efbc 100644 --- a/tests/resources/skills/test_skill_metadata.py +++ b/tests/resources/skills/test_skill_metadata.py @@ -54,6 +54,8 @@ "url": "skills/test-bucket/tone-of-voice/files/SKILL.md", "nodeType": "ITEM", "resourceType": "SKILL", + # Observed responses carry no etag on these entries at all; + # this pins that one is parsed when a deployment does send it. "etag": "abc123", "updatedAt": 1700000001000, }, @@ -61,9 +63,8 @@ "name": "references", "parentPath": "tone-of-voice/files", "bucket": "test-bucket", - # A subfolder. Core builds every entry of the files listing as a - # plain item and never overrides the node type, so it reports - # "ITEM" here too - only the trailing "/" marks it as a folder. + # A subfolder, as a non-recursive listing reports one: Core + # sends "ITEM" and only the trailing "/" marks it as a folder. "url": "skills/test-bucket/tone-of-voice/files/references/", "nodeType": "ITEM", "resourceType": "SKILL", @@ -109,7 +110,7 @@ def test_get_metadata_lists_bucket_root(): captured: list[httpx.Request] = [] client = _sync_client(captured, SKILLS_LISTING_MOCK) - result = client.skills.get_metadata(client.my_skills_home()) + result = client.skills.list() assert captured[0].url.path == "/v2/metadata/skills/test-bucket/" assert result.resource_type == "SKILL" @@ -127,8 +128,7 @@ def test_get_metadata_passes_listing_params(): captured: list[httpx.Request] = [] client = _sync_client(captured, SKILLS_LISTING_MOCK) - client.skills.get_metadata( - "skills/test-bucket/writing", + (client.skills / "writing").list( limit=1000, token="page-2", # noqa: S106 recursive=True, @@ -147,7 +147,7 @@ def test_get_metadata_omits_unset_params(): captured: list[httpx.Request] = [] client = _sync_client(captured, SKILLS_LISTING_MOCK) - client.skills.get_metadata("skills/test-bucket/writing") + (client.skills / "writing").list() assert dict(captured[0].url.params) == {} @@ -156,8 +156,8 @@ def test_list_files_defaults_to_skill_root(): captured: list[httpx.Request] = [] client = _sync_client(captured, SKILL_FILES_MOCK) - result = client.skills.list_files( - "skills/test-bucket/tone-of-voice", recursive=True, limit=1000 + result = client.skills(url="skills/test-bucket/tone-of-voice").files.list( + recursive=True, limit=1000 ) request = captured[0] @@ -171,9 +171,9 @@ def test_list_files_defaults_to_skill_root(): assert items[0].etag == "abc123" assert result.next_token is None - # Core reports subfolders of a skill as "ITEM" as well, so a caller has - # to key off the trailing "/" of the url instead. - assert [item.node_type for item in items] == ["ITEM", "ITEM"] + # Core reports subfolders of a skill as "ITEM" too; the validator + # derives node_type from the url so callers do not have to. + assert [item.node_type for item in items] == ["ITEM", "FOLDER"] assert [item.url.endswith("/") for item in items] == [False, True] @@ -181,9 +181,9 @@ def test_list_files_scopes_to_subfolder(): captured: list[httpx.Request] = [] client = _sync_client(captured, SKILL_FILES_MOCK) - client.skills.list_files( - "skills/test-bucket/tone-of-voice", path="references/api schema" - ) + client.skills(url="skills/test-bucket/tone-of-voice").files( + path="references/api schema" + ).list() assert captured[0].url.raw_path.decode() == ( "/v2/metadata/skills/test-bucket/tone-of-voice" @@ -211,12 +211,12 @@ def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: client._http_client._internal_http_client.send = send_mock + files = client.skills(url="skills/test-bucket/tone-of-voice").files + token = None seen = 0 while True: - page = client.skills.list_files( - "skills/test-bucket/tone-of-voice", token=token - ) + page = files.list(token=token) seen += len(page.items or []) token = page.next_token if token is None: @@ -228,18 +228,26 @@ def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: def test_get_metadata_rejects_non_skill_url(): - client = _sync_client([], SKILLS_LISTING_MOCK) + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILLS_LISTING_MOCK) + # Rejected while the reference is built, so nothing reaches the wire. with pytest.raises(InvalidDialURLError, match="Invalid resource type"): - client.skills.get_metadata("files/test-bucket/folder") + client.skills(url="files/test-bucket/folder") + + assert captured == [] def test_list_files_rejects_bucket_root(): - # A bucket has no files of its own - only skills do. - client = _sync_client([], SKILL_FILES_MOCK) + # A bucket has no files of its own - only skills do. The reference is + # legal; the terminal call is what has too little path for its route. + captured: list[httpx.Request] = [] + client = _sync_client(captured, SKILL_FILES_MOCK) - with pytest.raises(InvalidDialURLError, match="Missing bucket in URL"): - client.skills.list_files("skills/test-bucket") + with pytest.raises(InvalidDialURLError, match="points at the bucket root"): + client.skills(url="skills/test-bucket").files.list() + + assert captured == [] @pytest.mark.asyncio @@ -247,12 +255,12 @@ async def test_async_get_metadata_and_list_files(): captured: list[httpx.Request] = [] client = _async_client(captured, SKILLS_LISTING_MOCK) - result = await client.skills.get_metadata(await client.my_skills_home()) + result = await client.skills.list() assert captured[0].url.path == "/v2/metadata/skills/test-bucket/" assert (result.items or [])[0].name == "tone-of-voice" - await client.skills.list_files( - "skills/test-bucket/tone-of-voice", recursive=True + await client.skills(url="skills/test-bucket/tone-of-voice").files.list( + recursive=True ) assert ( captured[1].url.path @@ -260,14 +268,15 @@ async def test_async_get_metadata_and_list_files(): ) -def test_list_files_accepts_folder_path_with_trailing_slash(): +def test_rejects_folder_path_with_trailing_slash(): + # A reference carries no trailing slash: list() vs read() is what decides + # whether it names a folder or a file. The old list_files(path="refs/") + # accepted this and dropped the slash silently. captured: list[httpx.Request] = [] client = _sync_client(captured, SKILL_FILES_MOCK) - client.skills.list_files( - "skills/test-bucket/tone-of-voice", path="references/" - ) + skill = client.skills(url="skills/test-bucket/tone-of-voice") + with pytest.raises(InvalidDialURLError, match="must not end with"): + skill.files(path="references/") - assert captured[0].url.path == ( - "/v2/metadata/skills/test-bucket/tone-of-voice/files/references" - ) + assert captured == [] diff --git a/tests/resources/skills/test_skill_refs.py b/tests/resources/skills/test_skill_refs.py new file mode 100644 index 0000000..70b611e --- /dev/null +++ b/tests/resources/skills/test_skill_refs.py @@ -0,0 +1,475 @@ +""" +What the chained-reference design itself adds, on top of the URLs the other +skills tests pin: equivalence of the ways to aim a reference, immutability, +laziness of the bucket lookup, and segment validation at construction time. +""" + +from typing import Any, cast +from unittest.mock import AsyncMock, Mock + +import httpx +import pytest + +from aidial_client import AsyncDial, Dial +from aidial_client._exception import InvalidDialURLError +from aidial_client.types.metadata import SkillFileItem, SkillFileMetadata + +BUCKET = "test-bucket" +SKILL_URL = f"skills/{BUCKET}/writing/tone-of-voice" +LISTING: dict[str, Any] = { + "bucket": BUCKET, + "url": f"skills/{BUCKET}/", + "nodeType": "FOLDER", + "resourceType": "SKILL", +} + + +def _client(captured: list[httpx.Request]) -> Dial: + client = Dial(api_key="dummy", base_url="http://dial.core") + + def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response(200, request=request, json=LISTING) + response.request = request + return response + + client._http_client._internal_http_client.send = send_mock + client._get_my_bucket = Mock(return_value=BUCKET) + return client + + +def _async_client(captured: list[httpx.Request]) -> AsyncDial: + client = AsyncDial(api_key="dummy", base_url="http://dial.core") + + async def send_mock(request: httpx.Request, **_: Any) -> httpx.Response: + captured.append(request) + response = httpx.Response(200, request=request, json=LISTING) + response.request = request + return response + + client._http_client._internal_http_client.send = cast(Any, send_mock) + client._get_my_bucket = cast(Any, AsyncMock(return_value=BUCKET)) + return client + + +def _path_of(build) -> str: + captured: list[httpx.Request] = [] + build(_client(captured)) + return captured[0].url.raw_path.decode() + + +# --- aiming a reference -------------------------------------------------- + + +def test_truediv_and_path_are_equivalent(): + by_operator = _path_of(lambda c: (c.skills / "a" / "b").list()) + by_keyword = _path_of(lambda c: c.skills(path="a/b").list()) + by_mixture = _path_of(lambda c: (c.skills / "a")(path="b").list()) + + assert by_operator == f"/v2/metadata/skills/{BUCKET}/a/b/" + assert by_operator == by_keyword == by_mixture + + +def test_path_descends_and_bucket_is_kept(): + assert ( + _path_of(lambda c: (c.skills / "writing")(bucket="public").list()) + == "/v2/metadata/skills/public/writing/" + ) + + +def test_url_replaces_both_bucket_and_path(): + assert ( + _path_of( + lambda c: c.skills(bucket="other", path="ignored")( + url="skills/elsewhere/deep/skill" + ).download() + ) + == "/v2/skills/elsewhere/deep/skill" + ) + + +def test_files_url_round_trips_a_listing_entry(): + # An entry of a files listing carries the "/files/" segment, so it cannot + # go back through skills(url=...) - the files reference parses it instead. + assert ( + _path_of( + lambda c: c.skills.files( + url=f"{SKILL_URL}/files/references/api.md" + ).read() + ) + == f"/v2/skills/{BUCKET}/writing/tone-of-voice/files/references/api.md" + ) + + +def test_files_url_splits_after_the_first_segment(): + # Core's route requires at least one segment before "files" + # ("(?.+?)/files/"), so a skill named "files" resolves the same way. + assert ( + _path_of( + lambda c: c.skills.files( + url=f"skills/{BUCKET}/files/files/a.md" + ).read() + ) + == f"/v2/skills/{BUCKET}/files/files/a.md" + ) + + +def test_files_url_without_a_files_segment_is_rejected(): + client = _client([]) + + with pytest.raises(InvalidDialURLError, match="must address a file"): + client.skills.files(url=SKILL_URL) + + +@pytest.mark.parametrize( + "call, message", + [ + (lambda ref: ref(url="x", bucket="b"), "cannot be combined"), + (lambda ref: ref(url="x", path="p"), "cannot be combined"), + (lambda ref: ref(), "is required"), + ], +) +def test_bad_argument_combinations_are_type_errors(call, message): + # The overloads already reject these statically, so reaching them is a + # programming error rather than a bad URL. + client = _client([]) + + with pytest.raises(TypeError, match=message): + call(client.skills) + + +# --- immutability and laziness ------------------------------------------- + + +def test_narrowing_leaves_the_original_untouched(): + captured: list[httpx.Request] = [] + client = _client(captured) + + root = client.skills + sub = root / "writing" + + assert root is not sub + assert root.path == () + assert sub.path == ("writing",) + + root.list() + assert captured[0].url.path == f"/v2/metadata/skills/{BUCKET}/" + + +@pytest.mark.parametrize( + "build", + [ + lambda c: c.skills, + lambda c: c.skills.files, + lambda c: _async_client([]).skills, + lambda c: _async_client([]).skills.files, + ], +) +def test_references_are_immutable(build): + reference = build(_client([])) + + with pytest.raises(TypeError): + reference.path = ("mutated",) + + +def test_building_a_reference_issues_no_requests(): + captured: list[httpx.Request] = [] + client = _client(captured) + + chain = (client.skills / "writing" / "tone-of-voice").files(path="refs") + + assert captured == [] + assert client._get_my_bucket.call_count == 0 + assert chain.skill_path == ("writing", "tone-of-voice") + + +def test_own_bucket_is_resolved_once_and_shared(): + client = _client([]) + + client.skills.list() + (client.skills / "writing").list() + + assert client._get_my_bucket.call_count == 1 + + +@pytest.mark.parametrize( + "build", + [ + lambda c: c.skills(bucket="public").list(), + lambda c: c.skills(url=SKILL_URL).download(), + ], +) +def test_explicit_bucket_skips_the_lookup(build): + client = _client([]) + + build(client) + + assert client._get_my_bucket.call_count == 0 + + +# --- shape guards run before any I/O ------------------------------------- + + +@pytest.mark.parametrize( + "call, message", + [ + (lambda c: c.skills.download(), "points at the bucket root"), + (lambda c: c.skills.files.list(), "points at the bucket root"), + ( + lambda c: c.skills(url=SKILL_URL).files.read(), + "no file path was given", + ), + ], +) +def test_shape_guards_precede_bucket_resolution(call, message): + captured: list[httpx.Request] = [] + client = _client(captured) + + with pytest.raises(InvalidDialURLError, match=message): + call(client) + + assert captured == [] + # The guard has to come first, or a doomed call still costs a round-trip. + assert client._get_my_bucket.call_count == 0 + + +# --- segment validation -------------------------------------------------- + + +@pytest.mark.parametrize( + "bad", + ["", " ", "/abs", "a//b", ".", "..", "%2e%2e", ".%2e", "a%2fb", "refs/"], +) +@pytest.mark.parametrize( + "aim", + [ + lambda ref, value: ref(path=value), + lambda ref, value: ref / value, + ], +) +def test_every_path_surface_validates_segments(aim, bad): + client = _client([]) + + with pytest.raises(InvalidDialURLError): + aim(client.skills, bad) + with pytest.raises(InvalidDialURLError): + aim(client.skills(url=SKILL_URL).files, bad) + + +@pytest.mark.parametrize("bad", ["", "a/b", "..", "a%2fb"]) +def test_bucket_is_validated_as_a_single_segment(bad): + client = _client([]) + + with pytest.raises(InvalidDialURLError): + client.skills(bucket=bad) + + +def test_root_reference_has_no_phantom_dot_segment(): + # PurePosixPath("") is PurePosixPath("."), which would put a "." in the + # URL. Segments are stored as a tuple precisely to avoid that. + client = _client([]) + + assert client.skills.path == () + assert _path_of(lambda c: c.skills.list()).endswith(f"/{BUCKET}/") + + +# --- filenames ----------------------------------------------------------- + + +def test_archive_is_named_after_the_skill(): + client = _client([]) + + assert client.skills(url=SKILL_URL).download().filename == ( + "tone-of-voice.zip" + ) + + +def test_read_returns_a_human_readable_filename(): + client = _client([]) + + response = ( + client.skills(url=SKILL_URL).files(path="refs/api%20schema.md").read() + ) + + assert response.filename == "api schema.md" + + +# --- async mirror -------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_async_chain_builds_without_awaiting(): + captured: list[httpx.Request] = [] + client = _async_client(captured) + + skill = client.skills / "writing" / "tone-of-voice" + files = skill.files(path="references") + assert captured == [] + + await files.list(recursive=True) + + assert captured[0].url.path == ( + f"/v2/metadata/skills/{BUCKET}/writing/tone-of-voice/files/references" + ) + assert client._get_my_bucket.await_count == 1 + + +# --- node_type derivation ------------------------------------------------ + +# Real listings captured from DIAL Core (2026-09-04), one per mode. A +# recursive listing flattens the tree to leaf files at every depth; a +# non-recursive one returns the immediate children, and reports its +# subfolders as "ITEM" with a trailing slash. Neither carries an etag. +REAL_RECURSIVE_LISTING: dict[str, Any] = { + "name": "files", + "parentPath": "all-three-conventionss", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/all-three-conventionss/files/" + ), + "nodeType": "FOLDER", + "resourceType": "SKILL", + "items": [ + { + "name": "SKILL.md", + "parentPath": "all-three-conventionss/files", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/all-three-conventionss/files/SKILL.md" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + "updatedAt": 1788267212576, + }, + { + "name": "regions.csv", + "parentPath": "all-three-conventionss/files/assets", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/all-three-conventionss/files/assets/regions.csv" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + "updatedAt": 1788267212609, + }, + { + "name": "extract.py", + "parentPath": "all-three-conventionss/files/scripts", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/all-three-conventionss/files/scripts/extract.py" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + "updatedAt": 1788267212641, + }, + ], +} + + +def test_real_recursive_listing_is_left_alone(): + page = SkillFileMetadata(**REAL_RECURSIVE_LISTING) + + assert page.node_type == "FOLDER" + assert [item.node_type for item in page.items or []] == ["ITEM"] * 3 + assert [item.name for item in page.items or []] == [ + "SKILL.md", + "regions.csv", + "extract.py", + ] + + +def test_trailing_slash_derives_folder(): + payload: dict[str, Any] = { + "bucket": BUCKET, + "url": f"{SKILL_URL}/files/references/", + "nodeType": "ITEM", + "resourceType": "SKILL", + } + + assert SkillFileItem(**payload).node_type == "FOLDER" + + +def test_no_trailing_slash_derives_item(): + # The derivation is symmetric, as the review asked for: the url is the + # only input. A listing root scoped to a subfolder is requested without + # a trailing slash, so this is the case to watch - see the PR thread. + payload: dict[str, Any] = { + "bucket": BUCKET, + "url": f"{SKILL_URL}/files/references", + "nodeType": "FOLDER", + "resourceType": "SKILL", + } + + assert SkillFileMetadata(**payload).node_type == "ITEM" + + +# The same skill listed non-recursively: files and directories side by side, +# every one of them "ITEM", directories distinguished only by trailing "/". +REAL_NON_RECURSIVE_LISTING: dict[str, Any] = { + "name": "files", + "parentPath": "skill-creator", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/skill-creator/files/" + ), + "nodeType": "FOLDER", + "resourceType": "SKILL", + "items": [ + { + "name": "SKILL.md", + "parentPath": "skill-creator/files", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/skill-creator/files/SKILL.md" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + "updatedAt": 1788530552986, + }, + { + "name": "agents", + "parentPath": "skill-creator/files", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/skill-creator/files/agents/" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + }, + { + "name": "scripts", + "parentPath": "skill-creator/files", + "bucket": "4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc", + "url": ( + "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" + "/skill-creator/files/scripts/" + ), + "nodeType": "ITEM", + "resourceType": "SKILL", + }, + ], +} + + +def test_real_non_recursive_listing_corrects_subfolders(): + page = SkillFileMetadata(**REAL_NON_RECURSIVE_LISTING) + + # Core sent "ITEM" for all three; the validator corrects the two + # directories from the trailing "/" of their url. + assert [(i.name, i.node_type) for i in page.items or []] == [ + ("SKILL.md", "ITEM"), + ("agents", "FOLDER"), + ("scripts", "FOLDER"), + ] + # Directory entries carry no timestamp, and nothing here carries an etag. + items = page.items or [] + assert [i.updated_at for i in items] == [1788530552986, None, None] + assert all(i.etag is None for i in items) From 31ea7338736655efc94e27ca89640ba39ebc4a4f Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Thu, 10 Sep 2026 12:30:20 +0300 Subject: [PATCH 4/4] chore: remove core fix --- README.md | 12 +-- aidial_client/types/metadata.py | 61 ++------------ tests/resources/skills/test_skill_metadata.py | 8 +- tests/resources/skills/test_skill_refs.py | 79 +++++++------------ 4 files changed, 38 insertions(+), 122 deletions(-) diff --git a/README.md b/README.md index 9b6176a..027fb05 100644 --- a/README.md +++ b/README.md @@ -943,8 +943,7 @@ SkillFileMetadata( updated_at=1724836248936, ), SkillFileItem( - # A subfolder, as returned by a non-recursive listing. Core sends - # node_type="ITEM"; the client corrects it from the trailing "/". + # A subfolder, as returned by a non-recursive listing. name="references", parent_path="writing/tone-of-voice/files", bucket="my-bucket", @@ -956,17 +955,12 @@ SkillFileMetadata( ) ``` -> [!NOTE] -> A non-recursive listing returns the immediate subfolders, and DIAL Core -> reports them with `nodeType: "ITEM"` — the same value as the files beside -> them. The client derives `node_type` from the trailing `/` of `url`, so -> `node_type` is reliable here and you do not have to inspect urls yourself. - > [!NOTE] > The two modes answer different questions. `recursive=True` flattens the > tree: every file at every depth, no folder entries at all, with > `parent_path` showing where each file sits. A non-recursive listing returns -> only the immediate children. Empty folders never appear in either mode. +> the immediate children, folders included, distinguished by +> `node_type == "FOLDER"`. Empty folders never appear in either mode. Unlike the `/v1` files listing, these entries are sparse: no `content_length`, no `content_type`, and in observed responses no `etag` diff --git a/aidial_client/types/metadata.py b/aidial_client/types/metadata.py index 956021e..9980e91 100644 --- a/aidial_client/types/metadata.py +++ b/aidial_client/types/metadata.py @@ -1,13 +1,9 @@ -from typing import Any, Literal +from typing import Literal from aidial_client._compatibility.pydantic import PYDANTIC_V2 -from aidial_client._compatibility.pydantic_v1 import validator from aidial_client._internal_types._model import ExtraAllowModel from aidial_client._utils._alias import to_camel -if PYDANTIC_V2: - from pydantic import field_validator - class BaseMetadata(ExtraAllowModel): if PYDANTIC_V2: @@ -75,23 +71,8 @@ class PromptMetadata(BaseMetadata): resource_type: Literal["PROMPT"] -# Fixing the bug in DIAL Core: a non-recursive listing of a skill's files -# reports its subfolders with nodeType "ITEM". -# https://github.com/epam/ai-dial-core/issues/1912 -def _node_type_from_url(node_type: Any, url: Any) -> Any: - if not isinstance(url, str): - return node_type - return "FOLDER" if url.endswith("/") else "ITEM" - - class SkillItem(ResourceItemMetadata): - """ - A node in the skills listing: a skill (ITEM) or a grouping folder (FOLDER). - - ``node_type`` is taken from the response as-is. The bug worked around in - ``SkillFileItem`` was observed only on the file listing inside a skill, - not on this one. - """ + """A node in the skills listing: a skill (ITEM) or a grouping folder.""" node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] @@ -106,12 +87,10 @@ class SkillMetadata(BaseMetadata): class SkillFileItem(ResourceItemMetadata): """ - A file or a subfolder inside a skill. + A file (ITEM) or a subfolder (FOLDER) inside a skill. - ``node_type`` is derived from ``url`` rather than taken from the - response - see ``_node_type_from_url``. A recursive listing is flattened - and contains no subfolder entries at all, so the two kinds only ever - appear together in a non-recursive one. + A recursive listing is flattened and contains no subfolder entries at + all, so the two kinds only ever appear together in a non-recursive one. Sparser than the /v1 files listing: no ``content_length``, no ``content_type``, and in observed responses no ``etag`` either. @@ -121,39 +100,9 @@ class SkillFileItem(ResourceItemMetadata): node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] - if PYDANTIC_V2: - - @field_validator("node_type") - @classmethod - def _derive_node_type_v2(cls, value: Any, info: Any) -> Any: - return _node_type_from_url(value, info.data.get("url")) - - else: - - @validator("node_type") - def _derive_node_type_v1( # noqa: N805 - cls, value: Any, values: dict[str, Any] - ) -> Any: - return _node_type_from_url(value, values.get("url")) - class SkillFileMetadata(BaseMetadata): node_type: Literal["FOLDER", "ITEM"] resource_type: Literal["SKILL"] next_token: str | None = None items: list[SkillFileItem] | None = None - - if PYDANTIC_V2: - - @field_validator("node_type") - @classmethod - def _derive_node_type_v2(cls, value: Any, info: Any) -> Any: - return _node_type_from_url(value, info.data.get("url")) - - else: - - @validator("node_type") - def _derive_node_type_v1( # noqa: N805 - cls, value: Any, values: dict[str, Any] - ) -> Any: - return _node_type_from_url(value, values.get("url")) diff --git a/tests/resources/skills/test_skill_metadata.py b/tests/resources/skills/test_skill_metadata.py index 020efbc..40b45e8 100644 --- a/tests/resources/skills/test_skill_metadata.py +++ b/tests/resources/skills/test_skill_metadata.py @@ -63,10 +63,9 @@ "name": "references", "parentPath": "tone-of-voice/files", "bucket": "test-bucket", - # A subfolder, as a non-recursive listing reports one: Core - # sends "ITEM" and only the trailing "/" marks it as a folder. + # A subfolder, as a non-recursive listing reports one. "url": "skills/test-bucket/tone-of-voice/files/references/", - "nodeType": "ITEM", + "nodeType": "FOLDER", "resourceType": "SKILL", }, ], @@ -171,8 +170,7 @@ def test_list_files_defaults_to_skill_root(): assert items[0].etag == "abc123" assert result.next_token is None - # Core reports subfolders of a skill as "ITEM" too; the validator - # derives node_type from the url so callers do not have to. + # A subfolder is a FOLDER and carries a trailing "/" on its url. assert [item.node_type for item in items] == ["ITEM", "FOLDER"] assert [item.url.endswith("/") for item in items] == [False, True] diff --git a/tests/resources/skills/test_skill_refs.py b/tests/resources/skills/test_skill_refs.py index 70b611e..273f8f3 100644 --- a/tests/resources/skills/test_skill_refs.py +++ b/tests/resources/skills/test_skill_refs.py @@ -12,7 +12,7 @@ from aidial_client import AsyncDial, Dial from aidial_client._exception import InvalidDialURLError -from aidial_client.types.metadata import SkillFileItem, SkillFileMetadata +from aidial_client.types.metadata import SkillFileMetadata BUCKET = "test-bucket" SKILL_URL = f"skills/{BUCKET}/writing/tone-of-voice" @@ -314,12 +314,11 @@ async def test_async_chain_builds_without_awaiting(): assert client._get_my_bucket.await_count == 1 -# --- node_type derivation ------------------------------------------------ +# --- listing shapes ------------------------------------------------------- # Real listings captured from DIAL Core (2026-09-04), one per mode. A # recursive listing flattens the tree to leaf files at every depth; a -# non-recursive one returns the immediate children, and reports its -# subfolders as "ITEM" with a trailing slash. Neither carries an etag. +# non-recursive one returns the immediate children. Neither carries an etag. REAL_RECURSIVE_LISTING: dict[str, Any] = { "name": "files", "parentPath": "all-three-conventionss", @@ -370,46 +369,10 @@ async def test_async_chain_builds_without_awaiting(): ], } - -def test_real_recursive_listing_is_left_alone(): - page = SkillFileMetadata(**REAL_RECURSIVE_LISTING) - - assert page.node_type == "FOLDER" - assert [item.node_type for item in page.items or []] == ["ITEM"] * 3 - assert [item.name for item in page.items or []] == [ - "SKILL.md", - "regions.csv", - "extract.py", - ] - - -def test_trailing_slash_derives_folder(): - payload: dict[str, Any] = { - "bucket": BUCKET, - "url": f"{SKILL_URL}/files/references/", - "nodeType": "ITEM", - "resourceType": "SKILL", - } - - assert SkillFileItem(**payload).node_type == "FOLDER" - - -def test_no_trailing_slash_derives_item(): - # The derivation is symmetric, as the review asked for: the url is the - # only input. A listing root scoped to a subfolder is requested without - # a trailing slash, so this is the case to watch - see the PR thread. - payload: dict[str, Any] = { - "bucket": BUCKET, - "url": f"{SKILL_URL}/files/references", - "nodeType": "FOLDER", - "resourceType": "SKILL", - } - - assert SkillFileMetadata(**payload).node_type == "ITEM" - - -# The same skill listed non-recursively: files and directories side by side, -# every one of them "ITEM", directories distinguished only by trailing "/". +# The same skill listed non-recursively: files and directories side by side. +# The directory nodeTypes here are post-fix (epam/ai-dial-core#1912) - the +# original capture reported them as "ITEM", which is what the client used to +# correct. Directories still carry no timestamps: that branch copies none. REAL_NON_RECURSIVE_LISTING: dict[str, Any] = { "name": "files", "parentPath": "skill-creator", @@ -441,7 +404,7 @@ def test_no_trailing_slash_derives_item(): "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" "/skill-creator/files/agents/" ), - "nodeType": "ITEM", + "nodeType": "FOLDER", "resourceType": "SKILL", }, { @@ -452,24 +415,36 @@ def test_no_trailing_slash_derives_item(): "skills/4T56XoBkFtVqFQFmwHtbkUbjx8zLC8Sypb3xrJH4MACc" "/skill-creator/files/scripts/" ), - "nodeType": "ITEM", + "nodeType": "FOLDER", "resourceType": "SKILL", }, ], } -def test_real_non_recursive_listing_corrects_subfolders(): +def test_recursive_listing_is_flat(): + page = SkillFileMetadata(**REAL_RECURSIVE_LISTING) + + # Leaf files at every depth, no folder entries, structure in parent_path. + assert page.node_type == "FOLDER" + assert [i.node_type for i in page.items or []] == ["ITEM"] * 3 + assert [i.name for i in page.items or []] == [ + "SKILL.md", + "regions.csv", + "extract.py", + ] + + +def test_non_recursive_listing_carries_subfolders(): page = SkillFileMetadata(**REAL_NON_RECURSIVE_LISTING) + items = page.items or [] - # Core sent "ITEM" for all three; the validator corrects the two - # directories from the trailing "/" of their url. - assert [(i.name, i.node_type) for i in page.items or []] == [ + # node_type comes straight from the response; nothing is rewritten. + assert [(i.name, i.node_type) for i in items] == [ ("SKILL.md", "ITEM"), ("agents", "FOLDER"), ("scripts", "FOLDER"), ] - # Directory entries carry no timestamp, and nothing here carries an etag. - items = page.items or [] + # Subfolder entries carry no timestamp, and nothing here carries an etag. assert [i.updated_at for i in items] == [1788530552986, None, None] assert all(i.etag is None for i in items)