From 415e7c682c603c8f1f9364c9f6bf7556d47c672d Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 31 Jul 2026 01:03:08 +0000 Subject: [PATCH 1/7] skills: prompt before fetching so declined skills aren't downloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Download mode fetched every skill's bytes up front, then prompted to overwrite existing dirs at write time — so declining a skill threw away an already-completed download. Move the overwrite prompt and invalid-name check ahead of the fetch: split write_skill into should_download_skill (the disk-only decision, extracting existing_skill_on_disk) and a pure write_skill, and filter each schema's leaves through the decision before _fetch_bundles runs. The per-schema parallel fetch and the sequential location loop are unchanged, so cross-location same-leaf overwrite prompting still works. --- src/ucode/skills_download.py | 57 ++++++++++++++++++--------- tests/test_skills_download.py | 72 ++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 40 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 124721e..ac51e59 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -197,27 +197,36 @@ def _write_bundle(skill_dir: Path, leaf: str, files: dict[str, bytes]) -> None: destination.write_bytes(content) -def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, location: str) -> bool: - """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root. +def existing_skill_on_disk(roots: list[Path], leaf: str) -> bool: + """Whether ``leaf`` already has a skill directory under any root.""" + return any((root / leaf).exists() for root in roots) - Prompts before overwriting an existing skill dir. ``location`` is the source - ``.``, shown in that prompt. Returns True if the skill was - written, False if it was skipped or kept. + +def should_download_skill(roots: list[Path], leaf: str, *, location: str) -> bool: + """Whether ``leaf`` should be fetched and written into ``roots``. + + Applies the disk-only checks that need no bundle bytes, so a declined or + invalid skill is never downloaded: skips invalid leaf names, and prompts + before overwriting a skill already on disk (``location`` is the source + ``.`` shown in that prompt). """ if not _is_valid_leaf(leaf): print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") return False - already_on_disk = any((root / leaf).exists() for root in roots) - if already_on_disk and not prompt_yes_no( + if existing_skill_on_disk(roots, leaf) and not prompt_yes_no( f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" ): print_note(f"Kept existing `{leaf}`.") return False + return True + + +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes]) -> None: + """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root.""" for root in roots: _write_bundle(root / leaf, leaf, files) - return True # --- Orchestration --------------------------------------------------------- @@ -256,12 +265,21 @@ def download_skills( ) -> None: """Download every skill in each ``.`` location to disk. - Bundles are fetched concurrently (with a progress bar) per schema, then - written sequentially so overwrite prompts don't interleave. A failure on one - skill warns and skips it without aborting the batch. - - When ``skills`` is given, only those leaf names are downloaded; names absent - from a schema warn and are skipped. ``None`` downloads the whole schema. + Locations are processed one at a time, and each runs three stages: + + 1. **List** the schema's skill leaves. When ``skills`` is given, restrict to + those leaf names; names absent from the schema warn and are skipped, and + ``None`` keeps the whole schema. + 2. **Decide** which to download via ``should_download_skill`` (skips invalid + names and prompts before overwriting a skill already on disk), so a + declined skill is never fetched. + 3. **Fetch** the survivors' bundles concurrently (with a progress bar) and + **write** them. + + Finishing one location before starting the next means a skill written for an + earlier location is already on disk when a same-named skill in a later + location reaches its decide stage, so the overwrite prompt still fires. A + failure on one skill warns and skips it without aborting the batch. """ roots = skill_dir_roots(path) roots_display = " and ".join(str(root) for root in roots) @@ -286,15 +304,18 @@ def download_skills( print_note(f"No skills found in `{location}`.") continue - bundles = _fetch_bundles(workspace, token, catalog, schema, leaves) + to_download = [ + leaf for leaf in leaves if should_download_skill(roots, leaf, location=location) + ] + bundles = _fetch_bundles(workspace, token, catalog, schema, to_download) written = 0 - for leaf in leaves: + for leaf in to_download: files, reason = bundles[leaf] if reason or files is None: print_warning(f"Skipping `{location}.{leaf}`: {reason}.") continue - if write_skill(roots, leaf, files, location=location): - written += 1 + write_skill(roots, leaf, files) + written += 1 console.print() print_success( f"Downloaded {written}/{len(leaves)} skill(s) from `{location}` in {roots_display}." diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index bd5e54d..89ca449 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -6,7 +6,12 @@ import pytest import ucode.skills_download as sd -from ucode.skills_download import skill_dir_roots, write_skill +from ucode.skills_download import ( + existing_skill_on_disk, + should_download_skill, + skill_dir_roots, + write_skill, +) WS = "https://example.databricks.com" @@ -237,50 +242,56 @@ def test_missing_directory_rejected(self, tmp_path): skill_dir_roots(str(tmp_path / "nope")) -def _write(roots, leaf, files, *, location="main.default"): - return write_skill(roots, leaf, files, location=location) - - -class TestWriteSkill: - def test_writes_bundle_into_every_root(self, tmp_path): +class TestShouldDownloadSkill: + def test_new_skill_is_downloaded(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - files = {"SKILL.md": b"# skill", "scripts/run.py": b"print(1)"} - _write(roots, "triage", files) - - for root in roots: - assert (root / "triage/SKILL.md").read_bytes() == b"# skill" - assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" + assert should_download_skill(roots, "triage", location="main.default") def test_existing_skill_prompt_keep(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") + write_skill(roots, "triage", {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) - _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" + assert not should_download_skill(roots, "triage", location="ml.prod") def test_existing_skill_prompt_overwrite(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") + write_skill(roots, "triage", {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) - _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" + assert should_download_skill(roots, "triage", location="ml.prod") def test_invalid_leaf_is_skipped(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "Bad_Name", {"SKILL.md": b"x"}) + assert not should_download_skill(roots, "Bad_Name", location="main.default") - assert not (roots[0] / "Bad_Name").exists() + def test_existing_skill_on_disk_checks_every_root(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + assert not existing_skill_on_disk(roots, "triage") + + (roots[1] / "triage").mkdir(parents=True) + assert existing_skill_on_disk(roots, "triage") + + +class TestWriteSkill: + def test_writes_bundle_into_every_root(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + files = {"SKILL.md": b"# skill", "scripts/run.py": b"print(1)"} + + write_skill(roots, "triage", files) + + for root in roots: + assert (root / "triage/SKILL.md").read_bytes() == b"# skill" + assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) + write_skill(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists() @@ -322,6 +333,23 @@ def test_list_failure_skips_location(self, tmp_path, monkeypatch): assert called == [] + def test_declined_skill_is_not_fetched(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + write_skill(roots, "triage", {"SKILL.md": b"kept"}) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["triage"], None)) + monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) + fetched = [] + monkeypatch.setattr( + sd, + "fetch_skill_bundle", + lambda ws, tok, c, s, leaf: fetched.append(leaf) or ({"SKILL.md": b"new"}, None), + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert fetched == [] + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"kept" + def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["good", "bad"], None)) monkeypatch.setattr( From e0567556e7fd642cbcde00b098774768eb3898d0 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 31 Jul 2026 01:46:34 +0000 Subject: [PATCH 2/7] skills: clarify download_skills docstring and ruff format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review nit — restructure the download_skills docstring into explicit list/decide/fetch stages. Run ruff format to wrap an over-length line in tests (fixes the test_ruff_format CI check). --- tests/test_skills_download.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 89ca449..8c0a039 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -291,7 +291,9 @@ def test_writes_bundle_into_every_root(self, tmp_path): def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) + write_skill( + roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} + ) assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists() From 0768e1a99898e61125fc9c3b8664ed3c1809666b Mon Sep 17 00:00:00 2001 From: xiang-shen_data Date: Thu, 6 Aug 2026 20:41:58 +0000 Subject: [PATCH 3/7] skills: read skill bundles from the Files API `/Skills` place The download client read bundle bytes from `/Volumes///`, but skills have no backing UC Volume, so every fetch failed against a real workspace: listing succeeded via the 2.1 skills API, then each file 404'd with "Volume ... does not exist". Point both the directory walk and the file fetch at the `Skills` place instead. The old path survived because the tests mocked the HTTP layer and asserted the `/Volumes/...` URLs, so they encoded the bug. Updated those and pinned the listing URL with a test, which was previously unasserted. Verified end to end against xsh.bb-0806 on eng-ml-inference.staging (the workspace where both Skills flags are on): download_skills writes each bundle, nested files included, into .claude/skills and .agents/skills. Co-authored-by: Isaac --- src/ucode/skills_download.py | 17 +++++++++++------ tests/test_skills_download.py | 35 ++++++++++++++++++++++++----------- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index ac51e59..ddd44c7 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -29,6 +29,8 @@ SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9-]+$") +SKILL_FILES_API_PREFIX = "Skills" + # Parallel skill fetches per schema; writes stay sequential (they prompt). _MAX_FETCH_WORKERS = 8 @@ -86,15 +88,15 @@ def list_skill_files( ) -> tuple[list[str], str | None]: """List a skill bundle's files, as paths relative to the skill directory. - Recursively walks the skill's UC Volume directory (including ``SKILL.md``). + Recursively walks the skill's Files API directory (including ``SKILL.md``). A non-None reason indicates the listing call itself failed. """ hostname = workspace_hostname(workspace) dirs_base = f"https://{hostname}/api/2.0/fs/directories" - volume_prefix = f"/Volumes/{catalog}/{schema}/{leaf}/" + skill_prefix = f"/{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}/" relative_paths: list[str] = [] - pending = [f"Volumes/{catalog}/{schema}/{leaf}"] + pending = [f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}"] while pending: directory = pending.pop() page_token: str | None = None @@ -113,7 +115,7 @@ def list_skill_files( if entry.get("is_directory"): pending.append(path.strip("/")) else: - relative_paths.append(path.removeprefix(volume_prefix)) + relative_paths.append(path.removeprefix(skill_prefix)) page_token = data.get("next_page_token") if not page_token: break @@ -123,9 +125,12 @@ def list_skill_files( def fetch_skill_file( workspace: str, token: str, catalog: str, schema: str, leaf: str, relative_path: str ) -> tuple[bytes | None, str | None]: - """Fetch one skill bundle file's raw bytes from its UC Volume.""" + """Fetch one skill bundle file's raw bytes from the Files API.""" hostname = workspace_hostname(workspace) - url = f"https://{hostname}/api/2.0/fs/files/Volumes/{catalog}/{schema}/{leaf}/{relative_path}" + url = ( + f"https://{hostname}/api/2.0/fs/files/" + f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}/{relative_path}" + ) return _http_get_bytes(url, token, timeout=30) diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 8c0a039..cdd1687 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -101,18 +101,31 @@ def test_http_failure_propagates_reason(self, monkeypatch): class TestListSkillFiles: + def test_lists_under_the_skills_place(self, monkeypatch): + captured = {} + + def fake_get(url, token, timeout=30): + captured["url"] = url + return {"contents": []}, None + + monkeypatch.setattr(sd, "_http_get_json", fake_get) + + sd.list_skill_files(WS, "token", "main", "default", "triage") + + assert captured["url"] == f"{WS}/api/2.0/fs/directories/Skills/main/default/triage" + def test_walks_nested_directories_into_relative_paths(self, monkeypatch): - # The Files API returns absolute `/Volumes/...` paths. - vol = "/Volumes/main/default/triage" + # The Files API returns absolute paths. + skill = "/Skills/main/default/triage" listings = { - "Volumes/main/default/triage": { + "Skills/main/default/triage": { "contents": [ - {"path": f"{vol}/SKILL.md", "is_directory": False}, - {"path": f"{vol}/references/", "is_directory": True}, + {"path": f"{skill}/SKILL.md", "is_directory": False}, + {"path": f"{skill}/references/", "is_directory": True}, ] }, - "Volumes/main/default/triage/references": { - "contents": [{"path": f"{vol}/references/primary.md", "is_directory": False}] + "Skills/main/default/triage/references": { + "contents": [{"path": f"{skill}/references/primary.md", "is_directory": False}] }, } @@ -128,13 +141,13 @@ def fake_get(url, token, timeout=30): assert sorted(paths) == ["SKILL.md", "references/primary.md"] def test_follows_pagination(self, monkeypatch): - vol = "/Volumes/main/default/triage" + skill = "/Skills/main/default/triage" pages = [ { - "contents": [{"path": f"{vol}/a.md", "is_directory": False}], + "contents": [{"path": f"{skill}/a.md", "is_directory": False}], "next_page_token": "tok", }, - {"contents": [{"path": f"{vol}/b.md", "is_directory": False}]}, + {"contents": [{"path": f"{skill}/b.md", "is_directory": False}]}, ] monkeypatch.setattr( @@ -171,7 +184,7 @@ def fake_get_bytes(url, token, timeout=30): assert reason is None assert body == b"# SKILL\n" - assert captured["url"] == f"{WS}/api/2.0/fs/files/Volumes/main/default/triage/SKILL.md" + assert captured["url"] == f"{WS}/api/2.0/fs/files/Skills/main/default/triage/SKILL.md" def test_http_failure_propagates_reason(self, monkeypatch): monkeypatch.setattr( From fe1295b87173408a3bc1daccfa95f6d84f5b8bb0 Mon Sep 17 00:00:00 2001 From: xiang-shen_data Date: Thu, 6 Aug 2026 21:35:28 +0000 Subject: [PATCH 4/7] skills: address bundles by securable, name dirs by bundle name A skill has two names that are not interchangeable. The securable leaf of `skills/..` is the only one the Files API resolves, while `bundle_name` is set at finalize from the bundle's SKILL.md frontmatter and is what an agent looks for on disk. They coincide only when a skill was created under a securable matching its frontmatter. `_skill_bundle_name` preferred `bundle_name` and used it for both jobs, so a skill whose two names differ 404'd: Path '/Skills/xsh/bb-0806/task-triage' did not resolve to a Unity Catalog skill. where the securable is `task-prioritizer` and the frontmatter says `name: task-triage`. Replace it with a frozen `SkillRef` carrying both, so the type makes the distinction explicit rather than leaving it to a bare string: fetches take `ref.securable`, directories and dedup use `ref.bundle`. `should_download_skill` now validates both names, since each reaches a URL or the filesystem. `--skill` matches either name, so whichever a user knows works. Verified against xsh.bb-0806 on eng-ml-inference.staging: 5/5 skills download (was 4/5), and the divergent one lands in `task-triage/` with a SKILL.md whose frontmatter matches the directory. Co-authored-by: Isaac --- src/ucode/skills_download.py | 162 ++++++++++++++++++++-------------- tests/test_skills_download.py | 141 +++++++++++++++++++++++------ 2 files changed, 209 insertions(+), 94 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index ddd44c7..5d41fc6 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -4,6 +4,7 @@ import re from concurrent.futures import ThreadPoolExecutor, as_completed +from dataclasses import dataclass from pathlib import Path from urllib.parse import urlencode @@ -38,26 +39,43 @@ # --- Download client (UC skills API + Files API) --------------------------- -def _skill_bundle_name(skill: dict) -> str | None: - """The downloadable leaf name of a skill, or None if it isn't finalized. +@dataclass(frozen=True) +class SkillRef: + """A downloadable skill's two names, which are not interchangeable. - Only finalized skills (those with a ``finalize_time``) have bundle content - to download. ``bundle_name`` is the leaf; fall back to the last dotted - segment of the resource ``name`` (``skills/..``). + ``securable`` is the UC leaf of ``skills/..`` and is the only + name the Files API resolves, so it addresses the bytes. ``bundle`` is the + ``name:`` an agent reads from the bundle's SKILL.md frontmatter, so it names + the on-disk directory. They differ whenever a skill was created under a + securable that doesn't match its frontmatter. + """ + + securable: str + bundle: str + + +def _skill_ref(skill: dict) -> SkillRef | None: + """A finalized skill's ``SkillRef``, or None if it has no bundle to download. + + Only finalized skills (those with a ``finalize_time``) have bundle content. + ``bundle_name`` is set at finalize from the SKILL.md frontmatter; when it is + absent, the securable leaf doubles as the directory name. """ if not skill.get("finalize_time"): return None - bundle_name = skill.get("bundle_name") - if isinstance(bundle_name, str) and bundle_name: - return bundle_name name = skill.get("name") - return name.rsplit(".", 1)[-1] if isinstance(name, str) else None + if not isinstance(name, str) or not name: + return None + securable = name.rsplit(".", 1)[-1] + bundle_name = skill.get("bundle_name") + bundle = bundle_name if isinstance(bundle_name, str) and bundle_name else securable + return SkillRef(securable=securable, bundle=bundle) def list_schema_skills( workspace: str, token: str, catalog: str, schema: str -) -> tuple[list[str], str | None]: - """List the finalized skill leaf names in ``.``. +) -> tuple[list[SkillRef], str | None]: + """List the finalized skills in ``.``. A non-None reason indicates the listing call itself failed. """ @@ -65,7 +83,7 @@ def list_schema_skills( base_url = f"https://{hostname}/api/2.1/unity-catalog/skills" query = {"parent": f"schemas/{catalog}.{schema}"} - leaves: list[str] = [] + refs: list[SkillRef] = [] page_token: str | None = None while True: if page_token: @@ -75,28 +93,29 @@ def list_schema_skills( return [], reason data = payload if isinstance(payload, dict) else {} for skill in data.get("skills") or []: - leaf = _skill_bundle_name(skill) if isinstance(skill, dict) else None - if leaf: - leaves.append(leaf) + ref = _skill_ref(skill) if isinstance(skill, dict) else None + if ref: + refs.append(ref) page_token = data.get("next_page_token") if not page_token: - return leaves, None + return refs, None def list_skill_files( - workspace: str, token: str, catalog: str, schema: str, leaf: str + workspace: str, token: str, catalog: str, schema: str, securable: str ) -> tuple[list[str], str | None]: """List a skill bundle's files, as paths relative to the skill directory. Recursively walks the skill's Files API directory (including ``SKILL.md``). - A non-None reason indicates the listing call itself failed. + Takes the securable leaf, the only name the Files API resolves. A non-None + reason indicates the listing call itself failed. """ hostname = workspace_hostname(workspace) dirs_base = f"https://{hostname}/api/2.0/fs/directories" - skill_prefix = f"/{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}/" + skill_prefix = f"/{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{securable}/" relative_paths: list[str] = [] - pending = [f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}"] + pending = [f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{securable}"] while pending: directory = pending.pop() page_token: str | None = None @@ -123,19 +142,19 @@ def list_skill_files( def fetch_skill_file( - workspace: str, token: str, catalog: str, schema: str, leaf: str, relative_path: str + workspace: str, token: str, catalog: str, schema: str, securable: str, relative_path: str ) -> tuple[bytes | None, str | None]: """Fetch one skill bundle file's raw bytes from the Files API.""" hostname = workspace_hostname(workspace) url = ( f"https://{hostname}/api/2.0/fs/files/" - f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{leaf}/{relative_path}" + f"{SKILL_FILES_API_PREFIX}/{catalog}/{schema}/{securable}/{relative_path}" ) return _http_get_bytes(url, token, timeout=30) def fetch_skill_bundle( - workspace: str, token: str, catalog: str, schema: str, leaf: str + workspace: str, token: str, catalog: str, schema: str, securable: str ) -> tuple[dict[str, bytes] | None, str | None]: """Fetch a whole skill bundle as ``{relative_path: bytes}``. @@ -143,12 +162,14 @@ def fetch_skill_bundle( reason (and None bundle) means the listing or any file fetch failed, so a partially-downloaded skill is never written to disk. """ - relative_paths, reason = list_skill_files(workspace, token, catalog, schema, leaf) + relative_paths, reason = list_skill_files(workspace, token, catalog, schema, securable) if reason: return None, reason bundle: dict[str, bytes] = {} for relative_path in relative_paths: - content, reason = fetch_skill_file(workspace, token, catalog, schema, leaf, relative_path) + content, reason = fetch_skill_file( + workspace, token, catalog, schema, securable, relative_path + ) if content is None: return None, reason bundle[relative_path] = content @@ -202,58 +223,68 @@ def _write_bundle(skill_dir: Path, leaf: str, files: dict[str, bytes]) -> None: destination.write_bytes(content) -def existing_skill_on_disk(roots: list[Path], leaf: str) -> bool: - """Whether ``leaf`` already has a skill directory under any root.""" - return any((root / leaf).exists() for root in roots) +def existing_skill_on_disk(roots: list[Path], bundle: str) -> bool: + """Whether ``bundle`` already has a skill directory under any root.""" + return any((root / bundle).exists() for root in roots) -def should_download_skill(roots: list[Path], leaf: str, *, location: str) -> bool: - """Whether ``leaf`` should be fetched and written into ``roots``. +def should_download_skill(roots: list[Path], ref: SkillRef, *, location: str) -> bool: + """Whether ``ref`` should be fetched and written into ``roots``. Applies the disk-only checks that need no bundle bytes, so a declined or - invalid skill is never downloaded: skips invalid leaf names, and prompts - before overwriting a skill already on disk (``location`` is the source - ``.`` shown in that prompt). + invalid skill is never downloaded: skips names that are unsafe to use as a + URL or directory segment, and prompts before overwriting a skill already on + disk (``location`` is the source ``.`` shown in that + prompt). Dedup keys on the bundle name, since that is the directory an agent + would load. """ - if not _is_valid_leaf(leaf): - print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") - return False - - if existing_skill_on_disk(roots, leaf) and not prompt_yes_no( - f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" + for name in (ref.securable, ref.bundle): + if not _is_valid_leaf(name): + print_warning(f"Skipping `{name}`: not a valid skill name (lowercase a-z, 0-9, -).") + return False + + if existing_skill_on_disk(roots, ref.bundle) and not prompt_yes_no( + f"A skill named `{ref.bundle}` already exists. " + f"Overwrite it with `{location}.{ref.securable}`?" ): - print_note(f"Kept existing `{leaf}`.") + print_note(f"Kept existing `{ref.bundle}`.") return False return True -def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes]) -> None: - """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root.""" +def write_skill(roots: list[Path], ref: SkillRef, files: dict[str, bytes]) -> None: + """Write ``ref``'s bundle (``{relpath: bytes}``) into every root. + + The directory is named for the bundle, so it matches the ``name:`` an agent + reads from the written SKILL.md. + """ for root in roots: - _write_bundle(root / leaf, leaf, files) + _write_bundle(root / ref.bundle, ref.bundle, files) # --- Orchestration --------------------------------------------------------- def _fetch_bundles( - workspace: str, token: str, catalog: str, schema: str, leaves: list[str] + workspace: str, token: str, catalog: str, schema: str, refs: list[SkillRef] ) -> dict[str, tuple[dict[str, bytes] | None, str | None]]: - """Fetch every leaf's bundle concurrently, keyed by leaf name. + """Fetch every skill's bundle concurrently, keyed by securable leaf. Renders a ``k/n`` progress bar that advances as each fetch completes. """ - if not leaves: + if not refs: return {} results: dict[str, tuple[dict[str, bytes] | None, str | None]] = {} with ( - progress_bar(f"Fetching skills from {catalog}.{schema}", len(leaves)) as advance, - ThreadPoolExecutor(max_workers=min(_MAX_FETCH_WORKERS, len(leaves))) as pool, + progress_bar(f"Fetching skills from {catalog}.{schema}", len(refs)) as advance, + ThreadPoolExecutor(max_workers=min(_MAX_FETCH_WORKERS, len(refs))) as pool, ): futures = { - pool.submit(fetch_skill_bundle, workspace, token, catalog, schema, leaf): leaf - for leaf in leaves + pool.submit( + fetch_skill_bundle, workspace, token, catalog, schema, ref.securable + ): ref.securable + for ref in refs } for future in as_completed(futures): results[futures[future]] = future.result() @@ -272,9 +303,10 @@ def download_skills( Locations are processed one at a time, and each runs three stages: - 1. **List** the schema's skill leaves. When ``skills`` is given, restrict to - those leaf names; names absent from the schema warn and are skipped, and - ``None`` keeps the whole schema. + 1. **List** the schema's finalized skills. When ``skills`` is given, restrict + to those names, matching either the securable leaf or the bundle name so + whichever a user knows works; names matching neither warn and are skipped, + and ``None`` keeps the whole schema. 2. **Decide** which to download via ``should_download_skill`` (skips invalid names and prompts before overwriting a skill already on disk), so a declined skill is never fetched. @@ -290,40 +322,38 @@ def download_skills( roots_display = " and ".join(str(root) for root in roots) for location in locations: catalog, schema = location.split(".") - leaves, reason = list_schema_skills(workspace, token, catalog, schema) + refs, reason = list_schema_skills(workspace, token, catalog, schema) if reason: print_warning(f"Skipping `{location}`: {reason}.") continue if skills is not None: - unknown = skills - set(leaves) + unknown = skills - {name for ref in refs for name in (ref.securable, ref.bundle)} if unknown: print_warning( f"Skipping requested skill(s) not found in `{location}`: " f"{', '.join(sorted(unknown))}." ) - leaves = [leaf for leaf in leaves if leaf in skills] - if not leaves: + refs = [ref for ref in refs if skills & {ref.securable, ref.bundle}] + if not refs: print_note(f"No requested skills to download from `{location}`.") continue - if not leaves: + if not refs: print_note(f"No skills found in `{location}`.") continue - to_download = [ - leaf for leaf in leaves if should_download_skill(roots, leaf, location=location) - ] + to_download = [ref for ref in refs if should_download_skill(roots, ref, location=location)] bundles = _fetch_bundles(workspace, token, catalog, schema, to_download) written = 0 - for leaf in to_download: - files, reason = bundles[leaf] + for ref in to_download: + files, reason = bundles[ref.securable] if reason or files is None: - print_warning(f"Skipping `{location}.{leaf}`: {reason}.") + print_warning(f"Skipping `{location}.{ref.securable}`: {reason}.") continue - write_skill(roots, leaf, files) + write_skill(roots, ref, files) written += 1 console.print() print_success( - f"Downloaded {written}/{len(leaves)} skill(s) from `{location}` in {roots_display}." + f"Downloaded {written}/{len(refs)} skill(s) from `{location}` in {roots_display}." ) diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index cdd1687..c6c651e 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -7,6 +7,7 @@ import ucode.skills_download as sd from ucode.skills_download import ( + SkillRef, existing_skill_on_disk, should_download_skill, skill_dir_roots, @@ -16,8 +17,13 @@ WS = "https://example.databricks.com" +def ref(securable: str, bundle: str | None = None) -> SkillRef: + """A SkillRef whose two names match unless a differing bundle is given.""" + return SkillRef(securable=securable, bundle=bundle or securable) + + class TestListSchemaSkills: - def test_keeps_finalized_skills_and_uses_bundle_name(self, monkeypatch): + def test_keeps_finalized_skills_only(self, monkeypatch): payload = { "skills": [ { @@ -35,12 +41,31 @@ def test_keeps_finalized_skills_and_uses_bundle_name(self, monkeypatch): } monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) - leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + refs, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert refs == [ref("pii-handling"), ref("triage")] + + def test_keeps_both_names_when_bundle_differs_from_securable(self, monkeypatch): + # bundle_name comes from the bundle's SKILL.md frontmatter, so it can + # differ from the securable it was created under. + payload = { + "skills": [ + { + "name": "skills/main.default.task-prioritizer", + "bundle_name": "task-triage", + "finalize_time": "2026-06-26T05:58:25Z", + } + ] + } + monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + + refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None - assert leaves == ["pii-handling", "triage"] + assert refs == [SkillRef(securable="task-prioritizer", bundle="task-triage")] - def test_falls_back_to_resource_name_leaf(self, monkeypatch): + def test_bundle_falls_back_to_securable_leaf(self, monkeypatch): payload = { "skills": [ { @@ -51,15 +76,27 @@ def test_falls_back_to_resource_name_leaf(self, monkeypatch): } monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) - leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + refs, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert refs == [ref("pii-handling")] + + def test_skips_skills_without_a_resource_name(self, monkeypatch): + payload = {"skills": [{"bundle_name": "orphan", "finalize_time": "t"}]} + monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + + refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None - assert leaves == ["pii-handling"] + assert refs == [] def test_follows_pagination(self, monkeypatch): pages = [ - {"skills": [{"bundle_name": "a", "finalize_time": "t"}], "next_page_token": "tok"}, - {"skills": [{"bundle_name": "b", "finalize_time": "t"}]}, + { + "skills": [{"name": "skills/main.default.a", "finalize_time": "t"}], + "next_page_token": "tok", + }, + {"skills": [{"name": "skills/main.default.b", "finalize_time": "t"}]}, ] captured_tokens = [] @@ -69,10 +106,10 @@ def fake_get(url, token, timeout=30): monkeypatch.setattr(sd, "_http_get_json", fake_get) - leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None - assert leaves == ["a", "b"] + assert refs == [ref("a"), ref("b")] assert captured_tokens == [False, True] def test_targets_uc_skills_api_for_the_schema(self, monkeypatch): @@ -259,28 +296,38 @@ class TestShouldDownloadSkill: def test_new_skill_is_downloaded(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - assert should_download_skill(roots, "triage", location="main.default") + assert should_download_skill(roots, ref("triage"), location="main.default") def test_existing_skill_prompt_keep(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, "triage", {"SKILL.md": b"from-main"}) + write_skill(roots, ref("triage"), {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) - assert not should_download_skill(roots, "triage", location="ml.prod") + assert not should_download_skill(roots, ref("triage"), location="ml.prod") def test_existing_skill_prompt_overwrite(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, "triage", {"SKILL.md": b"from-main"}) + write_skill(roots, ref("triage"), {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) - assert should_download_skill(roots, "triage", location="ml.prod") + assert should_download_skill(roots, ref("triage"), location="ml.prod") def test_invalid_leaf_is_skipped(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - assert not should_download_skill(roots, "Bad_Name", location="main.default") + assert not should_download_skill(roots, ref("Bad_Name"), location="main.default") + + def test_either_unsafe_name_is_skipped(self, tmp_path): + # Both names reach the filesystem or a URL, so both must be validated. + roots = skill_dir_roots(str(tmp_path)) + + unsafe_bundle = SkillRef(securable="ok-name", bundle="../escape") + unsafe_securable = SkillRef(securable="../escape", bundle="ok-name") + + assert not should_download_skill(roots, unsafe_bundle, location="main.default") + assert not should_download_skill(roots, unsafe_securable, location="main.default") def test_existing_skill_on_disk_checks_every_root(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) @@ -295,7 +342,7 @@ def test_writes_bundle_into_every_root(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) files = {"SKILL.md": b"# skill", "scripts/run.py": b"print(1)"} - write_skill(roots, "triage", files) + write_skill(roots, ref("triage"), files) for root in roots: assert (root / "triage/SKILL.md").read_bytes() == b"# skill" @@ -305,7 +352,7 @@ def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) write_skill( - roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} + roots, ref("triage"), {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} ) assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" @@ -322,7 +369,7 @@ def test_empty_leaves_returns_empty_without_pool(self): class TestDownloadSkills: def test_fetches_and_writes_each_leaf(self, tmp_path, monkeypatch): monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: (["pii-handling", "triage"], None) + sd, "list_schema_skills", lambda *a, **k: ([ref("pii-handling"), ref("triage")], None) ) bundles = { "pii-handling": {"SKILL.md": b"pii"}, @@ -337,6 +384,38 @@ def test_fetches_and_writes_each_leaf(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/pii-handling/SKILL.md").read_bytes() == b"pii" assert (tmp_path / ".agents/skills/triage/SKILL.md").read_bytes() == b"triage" + def test_fetches_by_securable_and_writes_under_bundle_name(self, tmp_path, monkeypatch): + # The Files API resolves only the securable, while an agent loads the + # directory matching the bundle's SKILL.md `name:`. + diverging = SkillRef(securable="task-prioritizer", bundle="task-triage") + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([diverging], None)) + fetched = [] + monkeypatch.setattr( + sd, + "fetch_skill_bundle", + lambda ws, tok, c, s, securable: ( + fetched.append(securable) or ({"SKILL.md": b"name: task-triage"}, None) + ), + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert fetched == ["task-prioritizer"] + for base in (".claude/skills", ".agents/skills"): + assert (tmp_path / base / "task-triage/SKILL.md").read_bytes() == b"name: task-triage" + assert not (tmp_path / base / "task-prioritizer").exists() + + def test_skill_filter_matches_either_name(self, tmp_path, monkeypatch): + diverging = SkillRef(securable="task-prioritizer", bundle="task-triage") + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([diverging], None)) + monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"ok"}, None)) + + for requested in ("task-prioritizer", "task-triage"): + target = tmp_path / requested + target.mkdir() + sd.download_skills(WS, "token", ["main.default"], str(target), {requested}) + assert (target / ".claude/skills/task-triage/SKILL.md").exists() + def test_list_failure_skips_location(self, tmp_path, monkeypatch): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([], "HTTP 404 Not Found")) called = [] @@ -350,8 +429,8 @@ def test_list_failure_skips_location(self, tmp_path, monkeypatch): def test_declined_skill_is_not_fetched(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, "triage", {"SKILL.md": b"kept"}) - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["triage"], None)) + write_skill(roots, ref("triage"), {"SKILL.md": b"kept"}) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([ref("triage")], None)) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) fetched = [] monkeypatch.setattr( @@ -366,7 +445,9 @@ def test_declined_skill_is_not_fetched(self, tmp_path, monkeypatch): assert (roots[0] / "triage/SKILL.md").read_bytes() == b"kept" def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["good", "bad"], None)) + monkeypatch.setattr( + sd, "list_schema_skills", lambda *a, **k: ([ref("good"), ref("bad")], None) + ) monkeypatch.setattr( sd, "fetch_skill_bundle", @@ -381,7 +462,9 @@ def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): assert not (tmp_path / ".claude/skills/bad").exists() def test_prints_downloaded_count_and_roots_summary(self, tmp_path, monkeypatch, capsys): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["a", "b", "c"], None)) + monkeypatch.setattr( + sd, "list_schema_skills", lambda *a, **k: ([ref("a"), ref("b"), ref("c")], None) + ) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None)) sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) @@ -393,7 +476,9 @@ def test_prints_downloaded_count_and_roots_summary(self, tmp_path, monkeypatch, assert "".join(expected.split()) in printed def test_summary_counts_only_written_skills(self, tmp_path, monkeypatch, capsys): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["good", "bad"], None)) + monkeypatch.setattr( + sd, "list_schema_skills", lambda *a, **k: ([ref("good"), ref("bad")], None) + ) monkeypatch.setattr( sd, "fetch_skill_bundle", @@ -408,7 +493,7 @@ def test_summary_counts_only_written_skills(self, tmp_path, monkeypatch, capsys) def test_skill_filter_downloads_only_matching_leaves(self, tmp_path, monkeypatch): monkeypatch.setattr( - sd, "list_schema_skills", lambda *a, **k: (["pii-handling", "triage"], None) + sd, "list_schema_skills", lambda *a, **k: ([ref("pii-handling"), ref("triage")], None) ) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None)) @@ -418,7 +503,7 @@ def test_skill_filter_downloads_only_matching_leaves(self, tmp_path, monkeypatch assert not (tmp_path / ".claude/skills/pii-handling").exists() def test_skill_filter_warns_on_unknown_and_downloads_rest(self, tmp_path, monkeypatch, capsys): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["triage"], None)) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([ref("triage")], None)) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None)) sd.download_skills(WS, "token", ["main.default"], str(tmp_path), {"triage", "ghost"}) @@ -428,7 +513,7 @@ def test_skill_filter_warns_on_unknown_and_downloads_rest(self, tmp_path, monkey assert (tmp_path / ".claude/skills/triage/SKILL.md").read_bytes() == b"x" def test_empty_skill_filter_downloads_nothing(self, tmp_path, monkeypatch, capsys): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["triage"], None)) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([ref("triage")], None)) called = [] monkeypatch.setattr( sd, "fetch_skill_bundle", lambda *a, **k: called.append(1) or ({"SKILL.md": b"x"}, None) @@ -452,7 +537,7 @@ def test_empty_schema_reports_no_skills_found(self, tmp_path, monkeypatch, capsy assert "No skills found in `main.default`." in capsys.readouterr().out def test_none_skill_filter_downloads_everything(self, tmp_path, monkeypatch): - monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["a", "b"], None)) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([ref("a"), ref("b")], None)) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"x"}, None)) sd.download_skills(WS, "token", ["main.default"], str(tmp_path), None) From 410f21f7cd2dd5f87c56e182f7259a3525d52ca5 Mon Sep 17 00:00:00 2001 From: xiang-shen_data Date: Thu, 6 Aug 2026 21:43:20 +0000 Subject: [PATCH 5/7] skills: name SkillRef fields for the API, scope --skill to the securable Rename `SkillRef.securable`/`.bundle` to `securable_name`/`bundle_name` so both read as the API fields they come from, rather than leaving `bundle` to be mistaken for the bundle itself. Narrow `--skill` to match the securable name only. It selects which skills to download, and the securable is what identifies a skill in UC, so accepting the bundle name too gave one skill two selectors with no gain. Requesting a bundle name now reports it as not found, alongside the existing unknown-name warning. Verified against xsh.bb-0806 on eng-ml-inference.staging: the whole schema still downloads 5/5, `--skill task-prioritizer` downloads it into `task-triage/`, and `--skill task-triage` is reported as not found. Co-authored-by: Isaac --- src/ucode/cli.py | 6 ++-- src/ucode/skills_download.py | 60 ++++++++++++++++++----------------- tests/test_skills_download.py | 38 ++++++++++++---------- 3 files changed, 56 insertions(+), 48 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index f541079..e4ce205 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1907,9 +1907,9 @@ def configure_skills( str | None, typer.Option( "--skill", - help="(download) Download only this comma-separated subset of skills (by leaf " - "name, e.g. `my-skill`) from the schema, instead of every skill. Requires a " - "single --location; not valid with --mcp.", + help="(download) Download only this comma-separated subset of skills (by " + "securable name, e.g. `my-skill`) from the schema, instead of every skill. " + "Requires a single --location; not valid with --mcp.", ), ] = None, ) -> None: diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 5d41fc6..44836b1 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -43,15 +43,15 @@ class SkillRef: """A downloadable skill's two names, which are not interchangeable. - ``securable`` is the UC leaf of ``skills/..`` and is the only - name the Files API resolves, so it addresses the bytes. ``bundle`` is the - ``name:`` an agent reads from the bundle's SKILL.md frontmatter, so it names - the on-disk directory. They differ whenever a skill was created under a - securable that doesn't match its frontmatter. + ``securable_name`` is the UC leaf of ``skills/..`` and is the + only name the Files API resolves, so it addresses the bytes and identifies the + skill. ``bundle_name`` is the ``name:`` an agent reads from the bundle's + SKILL.md frontmatter, so it names the on-disk directory. They differ whenever + a skill was created under a securable that doesn't match its frontmatter. """ - securable: str - bundle: str + securable_name: str + bundle_name: str def _skill_ref(skill: dict) -> SkillRef | None: @@ -59,17 +59,19 @@ def _skill_ref(skill: dict) -> SkillRef | None: Only finalized skills (those with a ``finalize_time``) have bundle content. ``bundle_name`` is set at finalize from the SKILL.md frontmatter; when it is - absent, the securable leaf doubles as the directory name. + absent, the securable name doubles as the directory name. """ if not skill.get("finalize_time"): return None name = skill.get("name") if not isinstance(name, str) or not name: return None - securable = name.rsplit(".", 1)[-1] + securable_name = name.rsplit(".", 1)[-1] bundle_name = skill.get("bundle_name") - bundle = bundle_name if isinstance(bundle_name, str) and bundle_name else securable - return SkillRef(securable=securable, bundle=bundle) + return SkillRef( + securable_name=securable_name, + bundle_name=bundle_name if isinstance(bundle_name, str) and bundle_name else securable_name, + ) def list_schema_skills( @@ -223,9 +225,9 @@ def _write_bundle(skill_dir: Path, leaf: str, files: dict[str, bytes]) -> None: destination.write_bytes(content) -def existing_skill_on_disk(roots: list[Path], bundle: str) -> bool: - """Whether ``bundle`` already has a skill directory under any root.""" - return any((root / bundle).exists() for root in roots) +def existing_skill_on_disk(roots: list[Path], bundle_name: str) -> bool: + """Whether ``bundle_name`` already has a skill directory under any root.""" + return any((root / bundle_name).exists() for root in roots) def should_download_skill(roots: list[Path], ref: SkillRef, *, location: str) -> bool: @@ -238,16 +240,16 @@ def should_download_skill(roots: list[Path], ref: SkillRef, *, location: str) -> prompt). Dedup keys on the bundle name, since that is the directory an agent would load. """ - for name in (ref.securable, ref.bundle): + for name in (ref.securable_name, ref.bundle_name): if not _is_valid_leaf(name): print_warning(f"Skipping `{name}`: not a valid skill name (lowercase a-z, 0-9, -).") return False - if existing_skill_on_disk(roots, ref.bundle) and not prompt_yes_no( - f"A skill named `{ref.bundle}` already exists. " - f"Overwrite it with `{location}.{ref.securable}`?" + if existing_skill_on_disk(roots, ref.bundle_name) and not prompt_yes_no( + f"A skill named `{ref.bundle_name}` already exists. " + f"Overwrite it with `{location}.{ref.securable_name}`?" ): - print_note(f"Kept existing `{ref.bundle}`.") + print_note(f"Kept existing `{ref.bundle_name}`.") return False return True @@ -260,7 +262,7 @@ def write_skill(roots: list[Path], ref: SkillRef, files: dict[str, bytes]) -> No reads from the written SKILL.md. """ for root in roots: - _write_bundle(root / ref.bundle, ref.bundle, files) + _write_bundle(root / ref.bundle_name, ref.bundle_name, files) # --- Orchestration --------------------------------------------------------- @@ -282,8 +284,8 @@ def _fetch_bundles( ): futures = { pool.submit( - fetch_skill_bundle, workspace, token, catalog, schema, ref.securable - ): ref.securable + fetch_skill_bundle, workspace, token, catalog, schema, ref.securable_name + ): ref.securable_name for ref in refs } for future in as_completed(futures): @@ -304,9 +306,9 @@ def download_skills( Locations are processed one at a time, and each runs three stages: 1. **List** the schema's finalized skills. When ``skills`` is given, restrict - to those names, matching either the securable leaf or the bundle name so - whichever a user knows works; names matching neither warn and are skipped, - and ``None`` keeps the whole schema. + to those securable names (the name that identifies a skill in UC); names + absent from the schema warn and are skipped, and ``None`` keeps the whole + schema. 2. **Decide** which to download via ``should_download_skill`` (skips invalid names and prompts before overwriting a skill already on disk), so a declined skill is never fetched. @@ -327,13 +329,13 @@ def download_skills( print_warning(f"Skipping `{location}`: {reason}.") continue if skills is not None: - unknown = skills - {name for ref in refs for name in (ref.securable, ref.bundle)} + unknown = skills - {ref.securable_name for ref in refs} if unknown: print_warning( f"Skipping requested skill(s) not found in `{location}`: " f"{', '.join(sorted(unknown))}." ) - refs = [ref for ref in refs if skills & {ref.securable, ref.bundle}] + refs = [ref for ref in refs if ref.securable_name in skills] if not refs: print_note(f"No requested skills to download from `{location}`.") continue @@ -345,9 +347,9 @@ def download_skills( bundles = _fetch_bundles(workspace, token, catalog, schema, to_download) written = 0 for ref in to_download: - files, reason = bundles[ref.securable] + files, reason = bundles[ref.securable_name] if reason or files is None: - print_warning(f"Skipping `{location}.{ref.securable}`: {reason}.") + print_warning(f"Skipping `{location}.{ref.securable_name}`: {reason}.") continue write_skill(roots, ref, files) written += 1 diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index c6c651e..ed55583 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -17,9 +17,9 @@ WS = "https://example.databricks.com" -def ref(securable: str, bundle: str | None = None) -> SkillRef: - """A SkillRef whose two names match unless a differing bundle is given.""" - return SkillRef(securable=securable, bundle=bundle or securable) +def ref(securable_name: str, bundle_name: str | None = None) -> SkillRef: + """A SkillRef whose two names match unless a differing bundle name is given.""" + return SkillRef(securable_name=securable_name, bundle_name=bundle_name or securable_name) class TestListSchemaSkills: @@ -63,7 +63,7 @@ def test_keeps_both_names_when_bundle_differs_from_securable(self, monkeypatch): refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None - assert refs == [SkillRef(securable="task-prioritizer", bundle="task-triage")] + assert refs == [SkillRef(securable_name="task-prioritizer", bundle_name="task-triage")] def test_bundle_falls_back_to_securable_leaf(self, monkeypatch): payload = { @@ -323,8 +323,8 @@ def test_either_unsafe_name_is_skipped(self, tmp_path): # Both names reach the filesystem or a URL, so both must be validated. roots = skill_dir_roots(str(tmp_path)) - unsafe_bundle = SkillRef(securable="ok-name", bundle="../escape") - unsafe_securable = SkillRef(securable="../escape", bundle="ok-name") + unsafe_bundle = SkillRef(securable_name="ok-name", bundle_name="../escape") + unsafe_securable = SkillRef(securable_name="../escape", bundle_name="ok-name") assert not should_download_skill(roots, unsafe_bundle, location="main.default") assert not should_download_skill(roots, unsafe_securable, location="main.default") @@ -387,14 +387,14 @@ def test_fetches_and_writes_each_leaf(self, tmp_path, monkeypatch): def test_fetches_by_securable_and_writes_under_bundle_name(self, tmp_path, monkeypatch): # The Files API resolves only the securable, while an agent loads the # directory matching the bundle's SKILL.md `name:`. - diverging = SkillRef(securable="task-prioritizer", bundle="task-triage") + diverging = SkillRef(securable_name="task-prioritizer", bundle_name="task-triage") monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([diverging], None)) fetched = [] monkeypatch.setattr( sd, "fetch_skill_bundle", - lambda ws, tok, c, s, securable: ( - fetched.append(securable) or ({"SKILL.md": b"name: task-triage"}, None) + lambda ws, tok, c, s, securable_name: ( + fetched.append(securable_name) or ({"SKILL.md": b"name: task-triage"}, None) ), ) @@ -405,16 +405,22 @@ def test_fetches_by_securable_and_writes_under_bundle_name(self, tmp_path, monke assert (tmp_path / base / "task-triage/SKILL.md").read_bytes() == b"name: task-triage" assert not (tmp_path / base / "task-prioritizer").exists() - def test_skill_filter_matches_either_name(self, tmp_path, monkeypatch): - diverging = SkillRef(securable="task-prioritizer", bundle="task-triage") + def test_skill_filter_matches_securable_name_only(self, tmp_path, monkeypatch): + # `--skill` selects by the name that identifies the skill in UC, so the + # bundle name is not a selector even when it differs. + diverging = SkillRef(securable_name="task-prioritizer", bundle_name="task-triage") monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([diverging], None)) monkeypatch.setattr(sd, "fetch_skill_bundle", lambda *a, **k: ({"SKILL.md": b"ok"}, None)) - for requested in ("task-prioritizer", "task-triage"): - target = tmp_path / requested - target.mkdir() - sd.download_skills(WS, "token", ["main.default"], str(target), {requested}) - assert (target / ".claude/skills/task-triage/SKILL.md").exists() + selected = tmp_path / "by-securable" + selected.mkdir() + sd.download_skills(WS, "token", ["main.default"], str(selected), {"task-prioritizer"}) + assert (selected / ".claude/skills/task-triage/SKILL.md").exists() + + ignored = tmp_path / "by-bundle" + ignored.mkdir() + sd.download_skills(WS, "token", ["main.default"], str(ignored), {"task-triage"}) + assert not (ignored / ".claude/skills").exists() def test_list_failure_skips_location(self, tmp_path, monkeypatch): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([], "HTTP 404 Not Found")) From 94fd4af2e6869601b799b9f499cd22a2fa6de873 Mon Sep 17 00:00:00 2001 From: xiang-shen_data Date: Thu, 6 Aug 2026 21:57:21 +0000 Subject: [PATCH 6/7] skills: skip and warn when a finalized skill is missing either name Addresses review on _skill_ref: drop the bundle_name fallback and warn instead. A finalized skill is expected to carry both names -- `name` is immutable from CreateSkill, and FinalizeSkill is the sole writer of `bundle_name` -- so either one missing is an anomaly, not a case to paper over. Substituting the securable name for a missing bundle_name guessed a directory name that may not match the bundle's SKILL.md `name:`, which would silently hide the skill from the agent meant to load it. Now both names are required and a skill missing either is skipped with a warning naming the missing field(s). An unfinalized skill is still skipped quietly, since having no bundle yet is a normal in-progress state rather than an anomaly. Also correct the SkillRef docstring: finalize validates the frontmatter name for emptiness, length, and control characters, but never compares it to the securable, which is why the two can legitimately differ. Extract `_non_empty_str` so both names narrow from the untyped API payload without repeating the isinstance dance (also keeps `ty` happy). Co-authored-by: Isaac --- src/ucode/skills_download.py | 46 +++++++++++++++++++++++----------- tests/test_skills_download.py | 47 ++++++++++++++++++++++++----------- 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 44836b1..3226f25 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -46,32 +46,50 @@ class SkillRef: ``securable_name`` is the UC leaf of ``skills/..`` and is the only name the Files API resolves, so it addresses the bytes and identifies the skill. ``bundle_name`` is the ``name:`` an agent reads from the bundle's - SKILL.md frontmatter, so it names the on-disk directory. They differ whenever - a skill was created under a securable that doesn't match its frontmatter. + SKILL.md frontmatter, so it names the on-disk directory. Finalize does not + require the two to match, so a skill created under a securable that differs + from its frontmatter carries both. """ securable_name: str bundle_name: str +def _non_empty_str(value: object) -> str | None: + """``value`` when it is a non-empty string, else None.""" + return value if isinstance(value, str) and value else None + + def _skill_ref(skill: dict) -> SkillRef | None: - """A finalized skill's ``SkillRef``, or None if it has no bundle to download. + """A finalized skill's ``SkillRef``, or None if it cannot be downloaded. - Only finalized skills (those with a ``finalize_time``) have bundle content. - ``bundle_name`` is set at finalize from the SKILL.md frontmatter; when it is - absent, the securable name doubles as the directory name. + A skill without a ``finalize_time`` has no bundle content yet and is skipped + quietly, since that is a normal in-progress state. + + A finalized skill is expected to carry both names: ``name`` is immutable from + creation, and finalize is the sole writer of ``bundle_name``. One missing is + therefore an anomaly, so warn and skip rather than substituting the other + name -- the two are not interchangeable, and guessing a directory name that + doesn't match the bundle's SKILL.md ``name:`` would hide the skill from the + agent meant to load it. """ if not skill.get("finalize_time"): return None - name = skill.get("name") - if not isinstance(name, str) or not name: + + name = _non_empty_str(skill.get("name")) + bundle_name = _non_empty_str(skill.get("bundle_name")) + if name is None or bundle_name is None: + missing = " or ".join( + field + for field, value in (("name", name), ("bundle_name", bundle_name)) + if value is None + ) + print_warning( + f"Skipping `{name or ''}`: the skills API returned no {missing}." + ) return None - securable_name = name.rsplit(".", 1)[-1] - bundle_name = skill.get("bundle_name") - return SkillRef( - securable_name=securable_name, - bundle_name=bundle_name if isinstance(bundle_name, str) and bundle_name else securable_name, - ) + + return SkillRef(securable_name=name.rsplit(".", 1)[-1], bundle_name=bundle_name) def list_schema_skills( diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index ed55583..5948ec6 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -65,38 +65,57 @@ def test_keeps_both_names_when_bundle_differs_from_securable(self, monkeypatch): assert reason is None assert refs == [SkillRef(securable_name="task-prioritizer", bundle_name="task-triage")] - def test_bundle_falls_back_to_securable_leaf(self, monkeypatch): - payload = { - "skills": [ - { - "name": "skills/main.default.pii-handling", - "finalize_time": "2026-06-26T05:58:25Z", - } - ] - } + @pytest.mark.parametrize( + ("skill", "expected_missing"), + [ + ({"name": "skills/main.default.pii-handling"}, "bundle_name"), + ({"name": "skills/main.default.pii-handling", "bundle_name": ""}, "bundle_name"), + ({"bundle_name": "orphan"}, "name"), + ({}, "name or bundle_name"), + ], + ids=["no-bundle-name", "blank-bundle-name", "no-resource-name", "neither"], + ) + def test_skips_and_warns_when_a_name_is_missing(self, skill, expected_missing, monkeypatch): + # Finalize owns bundle_name and `name` is immutable from creation, so a + # finalized skill missing either is an anomaly worth surfacing. + payload = {"skills": [{**skill, "finalize_time": "2026-06-26T05:58:25Z"}]} monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + warnings = [] + monkeypatch.setattr(sd, "print_warning", warnings.append) refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None - assert refs == [ref("pii-handling")] + assert refs == [] + assert len(warnings) == 1 + assert f"no {expected_missing}." in warnings[0] - def test_skips_skills_without_a_resource_name(self, monkeypatch): - payload = {"skills": [{"bundle_name": "orphan", "finalize_time": "t"}]} + def test_unfinalized_skill_is_skipped_without_a_warning(self, monkeypatch): + # An unfinalized skill simply has no bundle yet, which is not an anomaly. + payload = {"skills": [{"name": "skills/main.default.draft"}]} monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + warnings = [] + monkeypatch.setattr(sd, "print_warning", warnings.append) refs, reason = sd.list_schema_skills(WS, "token", "main", "default") assert reason is None assert refs == [] + assert warnings == [] def test_follows_pagination(self, monkeypatch): pages = [ { - "skills": [{"name": "skills/main.default.a", "finalize_time": "t"}], + "skills": [ + {"name": "skills/main.default.a", "bundle_name": "a", "finalize_time": "t"} + ], "next_page_token": "tok", }, - {"skills": [{"name": "skills/main.default.b", "finalize_time": "t"}]}, + { + "skills": [ + {"name": "skills/main.default.b", "bundle_name": "b", "finalize_time": "t"} + ] + }, ] captured_tokens = [] From 3f36cfd5a2d9dae6c636542cbfc8b1c8651f84d8 Mon Sep 17 00:00:00 2001 From: xiang-shen_data Date: Thu, 6 Aug 2026 22:44:41 +0000 Subject: [PATCH 7/7] skills: drop siblings that claim the same bundle name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only the securable name is unique within a schema. `bundle_name` is parsed from each bundle's SKILL.md frontmatter and never compared against its siblings, so one schema can hold two finalized skills claiming the same directory. The decide stage runs before any write, so neither sibling saw the other on disk and both passed. Both then wrote to the same directory, whichever finished last won, and `written` counted both: ✔ Downloaded 2/2 skill(s) # one surviving directory, no prompt Reduce each location's skills to the first claimant of a bundle name and warn about the rest, naming the winner and how to resolve it. Runs before the decide stage, so a dropped sibling is never fetched and the summary's denominator counts only skills that can reach disk. Now: ! Skipping `main.default.skill-b`: its bundle name `foo` is already claimed by `main.default.skill-a`. Rename one skill's SKILL.md `name:` to download both. ✔ Downloaded 1/1 skill(s) Keeping the first rather than prompting matches the existing treatment of unusable skills, which warn and skip; a prompt here would ask the user to choose between two skills they cannot tell apart from the directory name alone. The guard is per location, so a same-named skill from a *later* location still reaches the overwrite prompt as before -- covered by a test, since that is the behavior most at risk of regressing here. Co-authored-by: Isaac --- src/ucode/skills_download.py | 31 +++++++++++++++++++++- tests/test_skills_download.py | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 3226f25..f93550e 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -312,6 +312,31 @@ def _fetch_bundles( return results +def _reject_bundle_name_collisions(refs: list[SkillRef], *, location: str) -> list[SkillRef]: + """``refs`` with any later skill that repeats an earlier one's bundle name dropped. + + Only the securable name is unique within a schema; ``bundle_name`` comes from + each bundle's SKILL.md frontmatter and is never checked against its siblings, + so one schema can hold two skills claiming the same directory. Writing both + would land them on top of each other, leaving whichever finished last with no + sign the other was lost, so keep the first and warn about the rest. + """ + kept: list[SkillRef] = [] + claimed: dict[str, str] = {} + for ref in refs: + winner = claimed.get(ref.bundle_name) + if winner is not None: + print_warning( + f"Skipping `{location}.{ref.securable_name}`: its bundle name " + f"`{ref.bundle_name}` is already claimed by `{location}.{winner}`. " + "Rename one skill's SKILL.md `name:` to download both." + ) + continue + claimed[ref.bundle_name] = ref.securable_name + kept.append(ref) + return kept + + def download_skills( workspace: str, token: str, @@ -326,7 +351,8 @@ def download_skills( 1. **List** the schema's finalized skills. When ``skills`` is given, restrict to those securable names (the name that identifies a skill in UC); names absent from the schema warn and are skipped, and ``None`` keeps the whole - schema. + schema. Siblings claiming one directory are then reduced to the first (see + ``_reject_bundle_name_collisions``). 2. **Decide** which to download via ``should_download_skill`` (skips invalid names and prompts before overwriting a skill already on disk), so a declined skill is never fetched. @@ -360,6 +386,9 @@ def download_skills( if not refs: print_note(f"No skills found in `{location}`.") continue + # Before the decide stage, so a dropped sibling is never fetched and the + # summary's denominator counts only skills that can reach disk. + refs = _reject_bundle_name_collisions(refs, location=location) to_download = [ref for ref in refs if should_download_skill(roots, ref, location=location)] bundles = _fetch_bundles(workspace, token, catalog, schema, to_download) diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 5948ec6..1b35530 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -403,6 +403,56 @@ def test_fetches_and_writes_each_leaf(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/pii-handling/SKILL.md").read_bytes() == b"pii" assert (tmp_path / ".agents/skills/triage/SKILL.md").read_bytes() == b"triage" + def test_sibling_bundle_name_collision_keeps_the_first(self, tmp_path, monkeypatch): + # Only the securable name is unique in a schema, so two siblings can claim + # one directory. Writing both would silently lose one. + colliding = [SkillRef("skill-a", "foo"), SkillRef("skill-b", "foo")] + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (colliding, None)) + bodies = {"skill-a": b"FROM A", "skill-b": b"FROM B"} + fetched = [] + monkeypatch.setattr( + sd, + "fetch_skill_bundle", + lambda ws, tok, c, s, securable_name: ( + fetched.append(securable_name) or ({"SKILL.md": bodies[securable_name]}, None) + ), + ) + warnings = [] + monkeypatch.setattr(sd, "print_warning", warnings.append) + monkeypatch.setattr( + sd, "prompt_yes_no", lambda msg: pytest.fail(f"unexpected prompt: {msg}") + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + # The loser is dropped before the fetch, not after paying for it. + assert fetched == ["skill-a"] + assert (tmp_path / ".claude/skills/foo/SKILL.md").read_bytes() == b"FROM A" + assert [d.name for d in (tmp_path / ".claude/skills").iterdir()] == ["foo"] + assert len(warnings) == 1 + assert "skill-b" in warnings[0] and "already claimed by" in warnings[0] + + def test_same_bundle_name_across_locations_still_prompts(self, tmp_path, monkeypatch): + # The collision guard is per location, so a later location's same-named + # skill must still reach the overwrite prompt rather than being dropped. + by_location = { + "main.default": [SkillRef("skill-a", "foo")], + "ml.prod": [SkillRef("skill-b", "foo")], + } + monkeypatch.setattr( + sd, "list_schema_skills", lambda ws, tok, c, s: (by_location[f"{c}.{s}"], None) + ) + monkeypatch.setattr( + sd, "fetch_skill_bundle", lambda ws, tok, c, s, sn: ({"SKILL.md": sn.encode()}, None) + ) + prompts = [] + monkeypatch.setattr(sd, "prompt_yes_no", lambda msg: bool(prompts.append(msg)) or True) + + sd.download_skills(WS, "token", ["main.default", "ml.prod"], str(tmp_path)) + + assert len(prompts) == 1 + assert (tmp_path / ".claude/skills/foo/SKILL.md").read_bytes() == b"skill-b" + def test_fetches_by_securable_and_writes_under_bundle_name(self, tmp_path, monkeypatch): # The Files API resolves only the securable, while an agent loads the # directory matching the bundle's SKILL.md `name:`.