feat: add read-side resource for DIAL Core /v2/skills - #138
Conversation
adubovik
left a comment
There was a problem hiding this comment.
I think we need to rethink the API. Currently, it's too fragile - the user must construct the URLs manually and make sure not to make any errors. Instead, we can build the URL while chaining the methods. That will guarantee that the URL is valid by construction. Here is the proposal:
List skills and grouping folders
listing = client.skills.list()
# GET /v2/metadata/skills/my-bucket/
listing = client.skills(bucket="other-bucket").list()
# GET /v2/metadata/skills/other-bucket/
listing = client.skills(path="sub/folder").list()
# GET /v2/metadata/skills/my-bucket/sub/folder
listing = (client.skills / "sub" / "folder").list()
# GET /v2/metadata/skills/my-bucket/sub/folder
listing = client.skills.list(recursive=True, limit=1000, token=page_token)
# GET /v2/metadata/skills/my-bucket/?recursive=true&limit=1000&token=...Follow an item from a listing
for item in client.skills.list().items or []:
skill = client.skills(url=item.url) # override: bucket + path from the item
manifest = skill.files(path="SKILL.md").read()
# GET /v2/skills/my-bucket/sub/folder/my-skill/files/SKILL.mdurl= is also how a grouping folder from that same listing is descended into:
folder = client.skills(url=item.url)
nested = folder.list()
# GET /v2/metadata/skills/my-bucket/sub/folder
deeper = folder(path="my-skill").files.list()
# GET /v2/metadata/skills/my-bucket/sub/folder/my-skill/filesList the files inside a skill
skill = client.skills(path="sub/folder/my-skill")
listing = skill.files.list()
# GET /v2/metadata/skills/my-bucket/sub/folder/my-skill/files
listing = skill.files(path="references").list()
# GET /v2/metadata/skills/my-bucket/sub/folder/my-skill/files/references
listing = (skill.files / "references").list(recursive=True)
# GET /v2/metadata/skills/my-bucket/sub/folder/my-skill/files/references?recursive=trueRead one file
manifest = skill.files(path="SKILL.md").read()
# GET /v2/skills/my-bucket/sub/folder/my-skill/files/SKILL.md
schema = skill.files(path="references/api schema.md").read()
# GET /v2/skills/my-bucket/sub/folder/my-skill/files/references/api%20schema.mdDownload a whole skill
archive = skill.download()
# GET /v2/skills/my-bucket/sub/folder/my-skill -> application/zipAsync
References build synchronously; only the terminal call is awaited.
skill = async_client.skills(path="sub/folder/my-skill")
listing = await skill.files.list()
# GET /v2/metadata/skills/my-bucket/sub/folder/my-skill/files
async with skill.files(path="assets/logo.png").stream() as file:
await file.awrite_to("logo.png")
# GET /v2/skills/my-bucket/sub/folder/my-skill/files/assets/logo.png
async with skill.stream_download() as archive:
await archive.awrite_to("my-skill.zip")
# GET /v2/skills/my-bucket/sub/folder/my-skillGrouping folders
client.skills.folder(path="sub/folder").create()
# PUT /v2/skills/my-bucket/sub/folder/
client.skills.folder(path="sub/folder").delete()
# DELETE /v2/skills/my-bucket/sub/folder/ (Core: only if empty)
client.skills.folder(url=item.url).delete()
# DELETE /v2/skills/my-bucket/sub/folder/Write and delete a single file
skill.files(path="references/api-schema.md").write(content)
# PUT /v2/skills/my-bucket/sub/folder/my-skill/files/references/api-schema.md
skill.files(path="references/api-schema.md").delete()
# DELETE /v2/skills/my-bucket/sub/folder/my-skill/files/references/api-schema.mdReplace and delete a whole skill
skill.replace(files={"SKILL.md": manifest, "scripts/run.sh": script})
# PUT /v2/skills/my-bucket/sub/folder/my-skill (multipart/form-data, one part per file)
skill.replace(files=..., etag_if_match=etag)
# PUT /v2/skills/my-bucket/sub/folder/my-skill + If-Match
skill.delete()
# DELETE /v2/skills/my-bucket/sub/folder/my-skillTyping sketch
class SkillsRef:
@overload
def __call__(self, *, path: str, bucket: str | None = None) -> "SkillsRef": ...
@overload
def __call__(self, *, bucket: str) -> "SkillsRef": ...
@overload
def __call__(self, *, url: str) -> "SkillsRef": ...
def __truediv__(self, path: str) -> "SkillsRef": ... # == self(path=path)
@property
def files(self) -> "SkillFilesRef": ...
@overload
def folder(self, *, path: str) -> "SkillFolderRef": ...
@overload
def folder(self, *, url: str) -> "SkillFolderRef": ...
def list(
self,
*,
limit: int | None = None,
token: str | None = None,
recursive: bool | None = None,
) -> SkillMetadata: ...
def download(self) -> FileDownloadResponse: ...
def replace(
self, files: Mapping[str, FileTypes], *, etag_if_match: str | None = None
) -> None: ...
def delete(self, *, etag_if_match: str | None = None) -> None: ...
class SkillFilesRef:
def __call__(self, *, path: str) -> "SkillFilesRef": ...
def __truediv__(self, path: str) -> "SkillFilesRef": ...
def list(
self,
*,
limit: int | None = None,
token: str | None = None,
recursive: bool | None = None,
) -> SkillFileMetadata: ...
def read(self) -> FileDownloadResponse: ...
def write(self, content: FileTypes) -> None: ...
def delete(self) -> None: ...
class SkillFolderRef:
def create(self) -> None: ...
def delete(self) -> None: ...Each reference is immutable and carries (bucket: str | None, path: PurePosixPath).
__call__ and __truediv__ return a new instance; the async client mirrors the
three classes with awaitable terminal methods.
Segment validation
path= and url= are the only path-taking surfaces, so validation lives in one
place. A segment is rejected when it is empty, or when it decodes to ., .., or
contains a separator — checked after unquote, so %2e%2e and %2f are caught.
Nothing is ever resolved with urljoin against an accumulated path, so no input can
shift the bucket segment.
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.
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
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.
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.
72e647e to
fc1282b
Compare
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.
fc1282b to
49b3186
Compare
Applicable issues
Description of changes
Adds a
skillsresource covering the read side of DIAL Core's/v2/skillsAPI, introduced by epam/ai-dial-core#1633 ("DIAL Folder As Resource"). A skill is a folder-shaped resource: a.dial-resourcemarker pointing at an immutablev/{versionId}/subtree, containing a mandatorySKILL.mdplus arbitrary bundled files.The write side (
PUT/DELETEfor whole resources, grouping folders and single files) is deliberately out of scope and tracked in #137 — it carries open questions (multipart part-naming, ETag-only responses,If-Matchcreate-vs-overwrite semantics) that would have held up a complete, useful read capability.Note
The surface was redesigned mid-review. It originally took a hand-built url on every call; per review feedback it is now an immutable chained reference, so a url is assembled from validated segments and cannot be typed wrong. The API section below reflects the current design. #137's API sketch still describes the old shape and needs rewriting before the write side is picked up.
API
client.skillsis a reference, narrowed step by step. Each step returns a new reference; references are immutable, validate every path segment as they are built, and issue no request until a terminal call..list(*, limit, token, recursive)GET /v2/metadata/skills/{bucket}/{path}.files.list(*, limit, token, recursive)GET /v2/metadata/skills/{bucket}/{path}/files[/{sub}].files(path=…).read()GET /v2/skills/{bucket}/{path}/files/{filePath}.download()GET /v2/skills/{bucket}/{path}→application/zipThree ways to aim a reference:
path=//to descend,bucket=to switch bucket (yours by default, resolved lazily),url=to follow an entry from a listing.AsyncSkillsRef/AsyncSkillFilesRefmirror this: references build synchronously, only the terminal call is awaited. They addstream()andstream_download(). The sync client has nostreammethod, so sync stays non-streaming — the same asymmetry that already exists forFiles.No read takes
etag_if_match: Core honoursIf-MatchonGET /v1/files/...but not on either/v2read, so offering the argument would have been a silent no-op. Pinned bytest_reads_send_no_if_match.Groundwork
The library assumed
/v1throughout, so this is not just one more resource class:API_PREFIX_V2/METADATA_PREFIX_V2constants; the v1 pair renamed toAPI_PREFIX_V1/METADATA_PREFIX_V1to match.StorageResourceTypeV2 = Literal["skills"]joins the parser's resource-type union. The v1StorageResourceTypeV1is left intact soMetadata._get_cast_to'sassert_neverstays exhaustive.resource_typeviaapi_prefix_for()+assert_never, rather than passed around —DialStorageResourceMixinand the parser both lost theirapi_prefixparameter.allow_empty_bucket_path, soskills/{bucket}parses — Core's children listing accepts an empty{path}. It is opt-in rather than automatic because a two-segment path is ambiguous:files/my-file.txthas the same shape and must stay a missing-bucket error (test_get_api_path_missing_bucketpins this). References pass it unconditionally, since a reference does not know which terminal call comes next; whether an empty path is acceptable is decided by the terminal call's own guard.DialStorageResource.bucket_pathisNoneat the bucket root rather than"".api_url_v2property, and the four reference classes wired into both_init_resources.my_skills_home()is gone — a reference already defaults to the caller's bucket.Path validation
Segments are rejected when empty, or when — after
unquote— they are.,.., or contain a separator. Checking post-decode is what catches%2e%2e(whichquoteleaves as.., since.is always-safe) and%2f.Validation lives in one place,
split_relative_segments, used by both the parser and the references. That placement is the fix for a real hole:urljoinresolves.and..while building a request, which shifts the bucket segment.Warning
Behaviour change on already-released resources. Traversal is now rejected while parsing any storage url, not only skills.
client.files.download("files/b/../../other/x")and the equivalent onprompts/conversationsused to resolve throughurljoinand silently retarget another bucket; they now raiseInvalidDialURLError. Covered bytest_parse_rejects_path_traversal. A trailing/still means "folder" and is unaffected.The other breaking change is scoped to this branch: the
/v2/skillssurface added by the first two commits is replaced by the reference form above. Nothing is released yet, so no published API changes.node_typeon the file listingA non-recursive listing of a skill's files reported subfolders with
nodeType: "ITEM"— the same value as the files beside them, distinguished only by a trailing/. Reported as epam/ai-dial-core#1912 and already fixed upstream (#1914).This PR ships a field validator deriving
node_typefromurlso callers never see the discrepancy. The fix landed ondevelopmentabout four minutes after Core0.47.1was cut, so it is in no release yet and the workaround cannot be dropped without breaking anyone on a released Core. Removal is tracked in #140.A recursive listing is unaffected — it flattens to leaf files with no folder entries at all. Both captured responses are pinned as fixtures.
Drive-by
_files_error_processorand_prompts_error_processorwere byte-identical and skills needed a third, so they are folded into a shared_storage_error_processorinhelpers/storage_resource.py.Notes for reviewers
/v2/skillsoperation is markedx-preview: truein Core'sopen_api_core.yaml; the README says so.ai-dial-core@developmentand against captured live responses rather than the OpenAPI doc — notably that the ZIP response carries noContent-Disposition(hence the derived filename) and that neither metadata listing returns anetagon the container.etagand no skill name/description. The file listing carries noetageither, on any entry, and subfolder entries carry no timestamps./v2single-file read returns the skill's aggregate etag, not the file's own — caching one file against it invalidates whenever any file in the skill changes.test_skill_core_routes.pyasserts every built url against Core's own route regexes, copied fromRouteTemplate.java. String-equality tests cannot catch a url that is well-formed but unroutable, which is how the bucket-root trailing slash was found.make lintfails identically on a cleandevelopment(the nox lint venv lacks test deps for pyright);ruffandpyrightare clean in the project venv.Checklist
README.mdupdated with sync + async usage and sample response objectsresources/__init__.pyBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.