From 6c2e8fef1e09f3c4117147b2cd063606efb8f5ff Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 00:08:59 +0000 Subject: [PATCH 01/11] skills: add UC download client (list schemas, list bundle files, fetch bytes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the read-only client layer for `ucode configure skills` download mode: - `list_schema_skills` — finalized skill leaf names in a schema via the UC skills API (`/api/2.1/unity-catalog/skills`, paginated). Uses `bundle_name` as the leaf and `finalize_time` as the readiness signal. - `list_skill_files` — a skill bundle's files (relative paths), by recursively walking its UC Volume directory via the Files API. - `fetch_skill_file` — one bundle file's raw bytes via the Files API. - `_http_get_bytes` — raw-bytes GET helper (skill bundles may be binary). Library only; no CLI wiring yet. All functions return (value, reason) and reuse the existing `_http_get_json` error convention. Verified end-to-end against a staging workspace with a user OAuth token: all three endpoints (skills list, Volume directory listing, raw file bytes) resolve directly with the client token — no storage-credential vending required. Co-authored-by: Isaac --- src/ucode/databricks.py | 125 ++++++++++++++++++++++++++++ tests/test_databricks.py | 175 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 300 insertions(+) diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index 19f151e..1374f6d 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -290,6 +290,35 @@ def _http_post_json( return None, f"network error: {exc.reason}" +def _http_get_bytes(url: str, token: str, *, timeout: int = 10) -> tuple[bytes | None, str | None]: + """GET raw bytes. Returns (body, None) on success, (None, reason) on failure. + + Like `_http_get_json` but leaves the body undecoded, since skill bundles can + contain binary files. + """ + request = urllib_request.Request(url, headers={"Authorization": f"Bearer {token}"}) + try: + with urllib_request.urlopen(request, timeout=timeout) as response: + body = response.read() + _debug(f"GET {url}", f"HTTP 200, {len(body)} bytes") + return body, None + except urllib_error.HTTPError as exc: + detail = "" + try: + detail = exc.read().decode("utf-8", errors="replace") if exc.fp else "" + except Exception: + detail = "" + _debug(f"GET {url}", f"HTTP {exc.code} {exc.reason}") + reason = f"HTTP {exc.code} {exc.reason}" + excerpt = detail.strip()[:200] + if excerpt: + reason = f"{reason}: {excerpt}" + return None, reason + except urllib_error.URLError as exc: + _debug(f"GET {url}", f"URLError: {exc.reason}") + return None, f"network error: {exc.reason}" + + def get_current_user_name(workspace: str, token: str) -> str | None: """Return the current user's login (email) via SCIM `Me`, or None on failure. @@ -1378,6 +1407,102 @@ def build_skills_mcp_url(workspace: str, locations: list[str]) -> str: return base + "?" + urlencode([("schema", loc) for loc in locations]) +# --- Skill download (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. + + 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/..``). + """ + 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 + + +def list_schema_skills( + workspace: str, token: str, catalog: str, schema: str +) -> tuple[list[str], str | None]: + """List the finalized skill leaf names in ``.``. + + A non-None reason indicates the listing call itself failed. + """ + hostname = workspace_hostname(workspace) + base_url = f"https://{hostname}/api/2.1/unity-catalog/skills" + query = {"parent": f"schemas/{catalog}.{schema}"} + + leaves: list[str] = [] + page_token: str | None = None + while True: + if page_token: + query["page_token"] = page_token + payload, reason = _http_get_json(f"{base_url}?{urlencode(query)}", token, timeout=30) + if payload is None: + return [], reason + data = cast(dict, 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) + page_token = data.get("next_page_token") + if not page_token: + return leaves, None + + +def list_skill_files( + workspace: str, token: str, catalog: str, schema: str, leaf: str +) -> 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``). + 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}/" + + relative_paths: list[str] = [] + pending = [f"Volumes/{catalog}/{schema}/{leaf}"] + while pending: + directory = pending.pop() + page_token: str | None = None + while True: + url = f"{dirs_base}/{directory}" + if page_token: + url = f"{url}?{urlencode({'page_token': page_token})}" + payload, reason = _http_get_json(url, token, timeout=30) + if payload is None: + return [], reason + data = cast(dict, payload) if isinstance(payload, dict) else {} + for entry in data.get("contents") or []: + path = entry.get("path") if isinstance(entry, dict) else None + if not isinstance(path, str): + continue + if entry.get("is_directory"): + pending.append(path.strip("/")) + else: + relative_paths.append(path.removeprefix(volume_prefix)) + page_token = data.get("next_page_token") + if not page_token: + break + return relative_paths, None + + +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.""" + hostname = workspace_hostname(workspace) + url = f"https://{hostname}/api/2.0/fs/files/Volumes/{catalog}/{schema}/{leaf}/{relative_path}" + return _http_get_bytes(url, token, timeout=30) + + # Maps the gateway routing dialect a coding tool speaks to the Model Provider # Service `provider_type`s it can be backed by. claude speaks Anthropic's API, # which both the `anthropic` and `amazon_bedrock` provider types serve (Bedrock diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 212ca62..640a92b 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -693,6 +693,181 @@ def test_http_404_reason_surfaces_for_invalid_parent(self, monkeypatch): assert reason and reason.startswith("HTTP 404") +class TestListSchemaSkills: + def test_keeps_finalized_skills_and_uses_bundle_name(self, monkeypatch): + payload = { + "skills": [ + { + "name": "skills/main.default.pii-handling", + "bundle_name": "pii-handling", + "finalize_time": "2026-06-26T05:58:25Z", + }, + { + "name": "skills/main.default.triage", + "bundle_name": "triage", + "finalize_time": "2026-06-26T05:58:26Z", + }, + {"name": "skills/main.default.draft", "bundle_name": "draft"}, + ] + } + monkeypatch.setattr( + db_mod, "_http_get_json", lambda url, token, timeout=30: (payload, None) + ) + + leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["pii-handling", "triage"] + + def test_falls_back_to_resource_name_leaf(self, monkeypatch): + payload = { + "skills": [ + { + "name": "skills/main.default.pii-handling", + "finalize_time": "2026-06-26T05:58:25Z", + } + ] + } + monkeypatch.setattr( + db_mod, "_http_get_json", lambda url, token, timeout=30: (payload, None) + ) + + leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["pii-handling"] + + 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"}]}, + ] + captured_tokens = [] + + def fake_get(url, token, timeout=30): + captured_tokens.append("page_token=tok" in url) + return pages.pop(0), None + + monkeypatch.setattr(db_mod, "_http_get_json", fake_get) + + leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["a", "b"] + assert captured_tokens == [False, True] + + def test_targets_uc_skills_api_for_the_schema(self, monkeypatch): + captured = {} + + def fake_get(url, token, timeout=30): + captured["url"] = url + return {"skills": []}, None + + monkeypatch.setattr(db_mod, "_http_get_json", fake_get) + + db_mod.list_schema_skills(WS, "token", "main", "default") + + assert "/api/2.1/unity-catalog/skills?" in captured["url"] + assert "parent=schemas%2Fmain.default" in captured["url"] + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + db_mod, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 500 Server Error") + ) + + leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") + + assert leaves == [] + assert reason == "HTTP 500 Server Error" + + +class TestListSkillFiles: + def test_walks_nested_directories_into_relative_paths(self, monkeypatch): + # The Files API returns absolute `/Volumes/...` paths. + vol = "/Volumes/main/default/triage" + listings = { + "Volumes/main/default/triage": { + "contents": [ + {"path": f"{vol}/SKILL.md", "is_directory": False}, + {"path": f"{vol}/references/", "is_directory": True}, + ] + }, + "Volumes/main/default/triage/references": { + "contents": [{"path": f"{vol}/references/primary.md", "is_directory": False}] + }, + } + + def fake_get(url, token, timeout=30): + directory = url.split("/api/2.0/fs/directories/", 1)[1] + return listings[directory], None + + monkeypatch.setattr(db_mod, "_http_get_json", fake_get) + + paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") + + assert reason is None + assert sorted(paths) == ["SKILL.md", "references/primary.md"] + + def test_follows_pagination(self, monkeypatch): + vol = "/Volumes/main/default/triage" + pages = [ + { + "contents": [{"path": f"{vol}/a.md", "is_directory": False}], + "next_page_token": "tok", + }, + {"contents": [{"path": f"{vol}/b.md", "is_directory": False}]}, + ] + + monkeypatch.setattr( + db_mod, "_http_get_json", lambda url, token, timeout=30: (pages.pop(0), None) + ) + + paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") + + assert reason is None + assert sorted(paths) == ["a.md", "b.md"] + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + db_mod, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") + ) + + paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") + + assert paths == [] + assert reason == "HTTP 404 Not Found" + + +class TestFetchSkillFile: + def test_returns_raw_bytes_from_files_api(self, monkeypatch): + captured = {} + + def fake_get_bytes(url, token, timeout=30): + captured["url"] = url + return b"# SKILL\n", None + + monkeypatch.setattr(db_mod, "_http_get_bytes", fake_get_bytes) + + body, reason = db_mod.fetch_skill_file(WS, "token", "main", "default", "triage", "SKILL.md") + + 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" + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + db_mod, "_http_get_bytes", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") + ) + + body, reason = db_mod.fetch_skill_file(WS, "token", "main", "default", "triage", "gone.md") + + assert body is None + assert reason == "HTTP 404 Not Found" + + def _foundation_models_payload(names): return { "endpoints": [ From be00452704689bc6db23ebf8d37e13501c984a69 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 00:35:08 +0000 Subject: [PATCH 02/11] skills: add on-disk writer for downloaded skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New `skills_download.py` owns the filesystem side of `ucode configure skills` download mode: - `skill_dir_roots` — the `.claude/skills` + `.agents/skills` roots for a download, at user scope (`~/`) or a project `--path`. - `write_skill` — writes a skill's `{relpath: bytes}` bundle into every root, one flat `//` dir per skill. Re-downloading the same skill in a run rewrites silently; a different schema colliding on the same leaf prompts keep/overwrite (or overwrites under `--yes`). Rejects invalid leaf names and `..`/absolute paths in the bundle. Transport-agnostic (no HTTP) so it's unit-testable with a literal dict; the bytes come from the download client in `databricks.py`. No CLI wiring yet. Co-authored-by: Isaac --- src/ucode/skills_download.py | 92 +++++++++++++++++++++++++++++ tests/test_skills_download.py | 105 ++++++++++++++++++++++++++++++++++ 2 files changed, 197 insertions(+) create mode 100644 src/ucode/skills_download.py create mode 100644 tests/test_skills_download.py diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py new file mode 100644 index 0000000..b53976a --- /dev/null +++ b/src/ucode/skills_download.py @@ -0,0 +1,92 @@ +"""Write downloaded Unity Catalog skills to disk for coding agents to load. + +Skills are written flat, one directory per skill (``//SKILL.md`` plus +bundled files), into both `.claude/skills` and `.agents/skills` — the pair that +covers every skills-capable agent ucode configures. The bytes come from the +download client in `databricks.py`; this module owns only the filesystem side. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +from ucode.ui import print_warning, prompt_yes_no + +# Cross-agent skill directories: `.claude/skills` (Claude) and `.agents/skills` +# (the Agent Skills alias the other agents read). Both get the same skills. +SKILL_DIR_NAMES = (".claude/skills", ".agents/skills") + +# Agent Skills spec: a skill's directory name is its `name`, lowercase a-z 0-9 -. +_LEAF_PATTERN = re.compile(r"^[a-z0-9-]+$") + + +def skill_dir_roots(path: str | None) -> list[Path]: + """Skill directory roots for a download: user scope (``~/``) or ``path``.""" + base = Path(path).expanduser().resolve() if path else Path.home() + return [base / name for name in SKILL_DIR_NAMES] + + +def _is_valid_leaf(leaf: str) -> bool: + return bool(_LEAF_PATTERN.match(leaf)) + + +def _safe_relative_path(relative_path: str) -> Path | None: + """A file's path within its skill dir, or None if it escapes the dir. + + The Files API returns server-controlled paths, but ucode writes them to + disk, so reject absolute paths and any ``..`` traversal. + """ + path = Path(relative_path) + if path.is_absolute() or ".." in path.parts: + return None + return path + + +def write_skill( + roots: list[Path], + leaf: str, + files: dict[str, bytes], + *, + assume_yes: bool, + written: dict[str, str], + schema_ref: str, +) -> str: + """Write one skill's files into every root, resolving directory collisions. + + ``files`` maps each in-bundle relative path (including ``SKILL.md``) to its + bytes. ``written`` tracks leaves already written this run (``leaf -> + schema_ref``) so re-downloading the same skill is silent while a different + schema colliding on the same leaf prompts to keep or overwrite. ``--yes`` + (``assume_yes``) overwrites without prompting. + + Returns ``"written"``, ``"overwritten"``, ``"kept"``, or ``"skipped"``. + """ + if not _is_valid_leaf(leaf): + print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") + return "skipped" + + if leaf in written: + status = "overwritten" # same skill re-downloaded this run — rewrite silently + elif any((root / leaf).exists() for root in roots): + if not assume_yes and not prompt_yes_no( + f"A skill named `{leaf}` already exists. Overwrite it with `{schema_ref}.{leaf}`?" + ): + return "kept" + status = "overwritten" + else: + status = "written" + + for root in roots: + skill_dir = root / leaf + for relative_path, content in files.items(): + safe_path = _safe_relative_path(relative_path) + if safe_path is None: + print_warning(f"Skipping unsafe path in `{leaf}`: {relative_path}") + continue + destination = skill_dir / safe_path + destination.parent.mkdir(parents=True, exist_ok=True) + destination.write_bytes(content) + + written[leaf] = schema_ref + return status diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py new file mode 100644 index 0000000..bebdb7a --- /dev/null +++ b/tests/test_skills_download.py @@ -0,0 +1,105 @@ +"""Tests for skills_download.py — the on-disk skill writer (no network).""" + +from __future__ import annotations + +from pathlib import Path + +import ucode.skills_download as sd +from ucode.skills_download import skill_dir_roots, write_skill + + +class TestSkillDirRoots: + def test_user_scope_defaults_to_home(self): + roots = skill_dir_roots(None) + assert roots == [Path.home() / ".claude/skills", Path.home() / ".agents/skills"] + + def test_project_scope_roots_under_path(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + assert roots == [tmp_path / ".claude/skills", tmp_path / ".agents/skills"] + + +def _write(roots, leaf, files, *, assume_yes=False, written=None, schema_ref="main.default"): + if written is None: + written = {} + return write_skill( + roots, leaf, files, assume_yes=assume_yes, written=written, schema_ref=schema_ref + ) + + +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)"} + + status = _write(roots, "triage", files) + + assert status == "written" + 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_same_skill_twice_in_one_run_overwrites_silently(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + written: dict[str, str] = {} + + _write(roots, "triage", {"SKILL.md": b"v1"}, written=written) + monkeypatch.setattr(sd, "prompt_yes_no", _fail_if_called) + status = _write(roots, "triage", {"SKILL.md": b"v2"}, written=written) + + assert status == "overwritten" + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"v2" + + def test_cross_schema_collision_overwrites_with_yes(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + _write(roots, "triage", {"SKILL.md": b"from-main"}, schema_ref="main.default") + + monkeypatch.setattr(sd, "prompt_yes_no", _fail_if_called) + status = _write( + roots, "triage", {"SKILL.md": b"from-ml"}, assume_yes=True, schema_ref="ml.prod" + ) + + assert status == "overwritten" + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" + + def test_cross_schema_collision_prompt_keep(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + _write(roots, "triage", {"SKILL.md": b"from-main"}, schema_ref="main.default") + + monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) + status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, schema_ref="ml.prod") + + assert status == "kept" + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" + + def test_cross_schema_collision_prompt_overwrite(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + _write(roots, "triage", {"SKILL.md": b"from-main"}, schema_ref="main.default") + + monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) + status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, schema_ref="ml.prod") + + assert status == "overwritten" + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" + + def test_invalid_leaf_is_skipped(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + + status = _write(roots, "Bad_Name", {"SKILL.md": b"x"}) + + assert status == "skipped" + assert not (roots[0] / "Bad_Name").exists() + + def test_path_traversal_is_rejected(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + + status = _write( + roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} + ) + + assert status == "written" + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" + assert not (tmp_path / "escape.md").exists() + + +def _fail_if_called(_prompt): + raise AssertionError("prompt_yes_no should not be called") From f0ed2ed03c9521d3ba62e4e26f37ba215fd95e85 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 01:16:56 +0000 Subject: [PATCH 03/11] =?UTF-8?q?skills:=20simplify=20writer=20=E2=80=94?= =?UTF-8?q?=20required=20absolute=20--path,=20drop=20--yes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review feedback, simplify the download writer: - `skill_dir_roots` now requires an existing absolute `--path` (raises ValueError otherwise); no `~/` user-scope default. - Drop `--yes`/`assume_yes`; a cross-schema leaf collision always prompts. - Rename `written` -> `written_leaves`, extract `_write_bundle`, and flatten the collision decision into one readable conditional. Co-authored-by: Isaac --- src/ucode/skills_download.py | 75 ++++++++++++++++++++--------------- tests/test_skills_download.py | 34 ++++++---------- 2 files changed, 54 insertions(+), 55 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index b53976a..401da09 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -21,9 +21,16 @@ _LEAF_PATTERN = re.compile(r"^[a-z0-9-]+$") -def skill_dir_roots(path: str | None) -> list[Path]: - """Skill directory roots for a download: user scope (``~/``) or ``path``.""" - base = Path(path).expanduser().resolve() if path else Path.home() +def skill_dir_roots(project_dir: str) -> list[Path]: + """The ``.claude/skills`` and ``.agents/skills`` roots under ``project_dir``. + + ``project_dir`` must be an existing absolute directory. + """ + base = Path(project_dir) + if not base.is_absolute(): + raise ValueError(f"--path must be an absolute path, got `{project_dir}`.") + if not base.is_dir(): + raise ValueError(f"--path directory does not exist: `{project_dir}`.") return [base / name for name in SKILL_DIR_NAMES] @@ -32,7 +39,7 @@ def _is_valid_leaf(leaf: str) -> bool: def _safe_relative_path(relative_path: str) -> Path | None: - """A file's path within its skill dir, or None if it escapes the dir. + """A bundle file's path within its skill dir, or None if it escapes the dir. The Files API returns server-controlled paths, but ucode writes them to disk, so reject absolute paths and any ``..`` traversal. @@ -43,22 +50,32 @@ def _safe_relative_path(relative_path: str) -> Path | None: return path +def _write_bundle(skill_dir: Path, leaf: str, files: dict[str, bytes]) -> None: + for relative_path, content in files.items(): + safe_path = _safe_relative_path(relative_path) + if safe_path is None: + print_warning(f"Skipping unsafe path in `{leaf}`: {relative_path}") + continue + destination = skill_dir / safe_path + destination.parent.mkdir(parents=True, exist_ok=True) + destination.write_bytes(content) + + def write_skill( roots: list[Path], leaf: str, files: dict[str, bytes], *, - assume_yes: bool, - written: dict[str, str], + written_leaves: dict[str, str], schema_ref: str, ) -> str: - """Write one skill's files into every root, resolving directory collisions. + """Write one skill's bundle into every root, resolving directory collisions. ``files`` maps each in-bundle relative path (including ``SKILL.md``) to its - bytes. ``written`` tracks leaves already written this run (``leaf -> - schema_ref``) so re-downloading the same skill is silent while a different - schema colliding on the same leaf prompts to keep or overwrite. ``--yes`` - (``assume_yes``) overwrites without prompting. + bytes. ``written_leaves`` records what this run has already written + (``leaf -> schema_ref``); the caller shares one dict across the whole run so + re-downloading the same skill rewrites silently, while a *different* schema + landing on an existing leaf prompts to keep or overwrite it. Returns ``"written"``, ``"overwritten"``, ``"kept"``, or ``"skipped"``. """ @@ -66,27 +83,19 @@ def write_skill( print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") return "skipped" - if leaf in written: - status = "overwritten" # same skill re-downloaded this run — rewrite silently - elif any((root / leaf).exists() for root in roots): - if not assume_yes and not prompt_yes_no( - f"A skill named `{leaf}` already exists. Overwrite it with `{schema_ref}.{leaf}`?" - ): - return "kept" - status = "overwritten" - else: - status = "written" + already_on_disk = any((root / leaf).exists() for root in roots) + + # A collision is only a real conflict if this run didn't just write the leaf + # itself (re-downloading the same skill rewrites silently); ask before + # clobbering a skill some other schema or earlier run put there. + conflicts_with_existing = already_on_disk and leaf not in written_leaves + if conflicts_with_existing and not prompt_yes_no( + f"A skill named `{leaf}` already exists. Overwrite it with `{schema_ref}.{leaf}`?" + ): + return "kept" for root in roots: - skill_dir = root / leaf - for relative_path, content in files.items(): - safe_path = _safe_relative_path(relative_path) - if safe_path is None: - print_warning(f"Skipping unsafe path in `{leaf}`: {relative_path}") - continue - destination = skill_dir / safe_path - destination.parent.mkdir(parents=True, exist_ok=True) - destination.write_bytes(content) - - written[leaf] = schema_ref - return status + _write_bundle(root / leaf, leaf, files) + + written_leaves[leaf] = schema_ref + return "overwritten" if already_on_disk else "written" diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index bebdb7a..3261360 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -2,28 +2,30 @@ from __future__ import annotations -from pathlib import Path +import pytest import ucode.skills_download as sd from ucode.skills_download import skill_dir_roots, write_skill class TestSkillDirRoots: - def test_user_scope_defaults_to_home(self): - roots = skill_dir_roots(None) - assert roots == [Path.home() / ".claude/skills", Path.home() / ".agents/skills"] - - def test_project_scope_roots_under_path(self, tmp_path): + def test_roots_under_project_dir(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) assert roots == [tmp_path / ".claude/skills", tmp_path / ".agents/skills"] + def test_relative_path_rejected(self): + with pytest.raises(ValueError, match="absolute"): + skill_dir_roots("relative/dir") + + def test_missing_directory_rejected(self, tmp_path): + with pytest.raises(ValueError, match="does not exist"): + skill_dir_roots(str(tmp_path / "nope")) -def _write(roots, leaf, files, *, assume_yes=False, written=None, schema_ref="main.default"): + +def _write(roots, leaf, files, *, written=None, schema_ref="main.default"): if written is None: written = {} - return write_skill( - roots, leaf, files, assume_yes=assume_yes, written=written, schema_ref=schema_ref - ) + return write_skill(roots, leaf, files, written_leaves=written, schema_ref=schema_ref) class TestWriteSkill: @@ -49,18 +51,6 @@ def test_same_skill_twice_in_one_run_overwrites_silently(self, tmp_path, monkeyp assert status == "overwritten" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"v2" - def test_cross_schema_collision_overwrites_with_yes(self, tmp_path, monkeypatch): - roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"from-main"}, schema_ref="main.default") - - monkeypatch.setattr(sd, "prompt_yes_no", _fail_if_called) - status = _write( - roots, "triage", {"SKILL.md": b"from-ml"}, assume_yes=True, schema_ref="ml.prod" - ) - - assert status == "overwritten" - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" - def test_cross_schema_collision_prompt_keep(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) _write(roots, "triage", {"SKILL.md": b"from-main"}, schema_ref="main.default") From 8590c6b987a1c7e9e302255ede82e4859cf5333c Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 01:59:28 +0000 Subject: [PATCH 04/11] skills: always prompt on existing skill dir; drop per-run ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `write_skill` no longer tracks which leaves it wrote this run. Any pre-existing skill directory now prompts before overwrite, uniformly — including re-downloading the same skill. The per-run `written_leaves` ledger only made same-run duplicate leaves rewrite silently, but it added state and a subtle branch to cover a rare case, and it's fragile: ucode doesn't persist which schema wrote a given dir, and users can edit the filesystem, so "it's the same skill" was never reliable. Always asking is simpler and consistent. Co-authored-by: Isaac --- src/ucode/skills_download.py | 26 ++++++-------------------- tests/test_skills_download.py | 25 ++++--------------------- 2 files changed, 10 insertions(+), 41 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 401da09..151d38e 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -61,21 +61,13 @@ 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], - *, - written_leaves: dict[str, str], - schema_ref: str, -) -> str: - """Write one skill's bundle into every root, resolving directory collisions. +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, schema_ref: str) -> str: + """Write one skill's bundle into every root, prompting before overwriting. ``files`` maps each in-bundle relative path (including ``SKILL.md``) to its - bytes. ``written_leaves`` records what this run has already written - (``leaf -> schema_ref``); the caller shares one dict across the whole run so - re-downloading the same skill rewrites silently, while a *different* schema - landing on an existing leaf prompts to keep or overwrite it. + bytes. If the skill's directory already exists in any root, ask before + overwriting it — the on-disk skill may be from a different schema, so we + never clobber it without consent. Returns ``"written"``, ``"overwritten"``, ``"kept"``, or ``"skipped"``. """ @@ -84,12 +76,7 @@ def write_skill( return "skipped" already_on_disk = any((root / leaf).exists() for root in roots) - - # A collision is only a real conflict if this run didn't just write the leaf - # itself (re-downloading the same skill rewrites silently); ask before - # clobbering a skill some other schema or earlier run put there. - conflicts_with_existing = already_on_disk and leaf not in written_leaves - if conflicts_with_existing and not prompt_yes_no( + if already_on_disk and not prompt_yes_no( f"A skill named `{leaf}` already exists. Overwrite it with `{schema_ref}.{leaf}`?" ): return "kept" @@ -97,5 +84,4 @@ def write_skill( for root in roots: _write_bundle(root / leaf, leaf, files) - written_leaves[leaf] = schema_ref return "overwritten" if already_on_disk else "written" diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 3261360..506d812 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -22,10 +22,8 @@ def test_missing_directory_rejected(self, tmp_path): skill_dir_roots(str(tmp_path / "nope")) -def _write(roots, leaf, files, *, written=None, schema_ref="main.default"): - if written is None: - written = {} - return write_skill(roots, leaf, files, written_leaves=written, schema_ref=schema_ref) +def _write(roots, leaf, files, *, schema_ref="main.default"): + return write_skill(roots, leaf, files, schema_ref=schema_ref) class TestWriteSkill: @@ -40,18 +38,7 @@ def test_writes_bundle_into_every_root(self, tmp_path): assert (root / "triage/SKILL.md").read_bytes() == b"# skill" assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" - def test_same_skill_twice_in_one_run_overwrites_silently(self, tmp_path, monkeypatch): - roots = skill_dir_roots(str(tmp_path)) - written: dict[str, str] = {} - - _write(roots, "triage", {"SKILL.md": b"v1"}, written=written) - monkeypatch.setattr(sd, "prompt_yes_no", _fail_if_called) - status = _write(roots, "triage", {"SKILL.md": b"v2"}, written=written) - - assert status == "overwritten" - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"v2" - - def test_cross_schema_collision_prompt_keep(self, tmp_path, monkeypatch): + 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"}, schema_ref="main.default") @@ -61,7 +48,7 @@ def test_cross_schema_collision_prompt_keep(self, tmp_path, monkeypatch): assert status == "kept" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" - def test_cross_schema_collision_prompt_overwrite(self, tmp_path, monkeypatch): + 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"}, schema_ref="main.default") @@ -89,7 +76,3 @@ def test_path_traversal_is_rejected(self, tmp_path): assert status == "written" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists() - - -def _fail_if_called(_prompt): - raise AssertionError("prompt_yes_no should not be called") From c7d0b1d9e6b82147af2062b36a4738cbbd16aac7 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 02:10:42 +0000 Subject: [PATCH 05/11] =?UTF-8?q?skills:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?tighter=20docstrings,=20clearer=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Condense the module and write_skill docstrings. - Rename SKILL_DIR_NAMES -> SKILL_BASE_DIR_NAMES. - Rename _LEAF_PATTERN -> SKILL_NAME_PATTERN, drop its redundant comment. - Rename write_skill's schema_ref param -> location (it's the . string). Co-authored-by: Isaac --- src/ucode/skills_download.py | 35 ++++++++++++----------------------- tests/test_skills_download.py | 12 ++++++------ 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 151d38e..1f259b0 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -1,10 +1,4 @@ -"""Write downloaded Unity Catalog skills to disk for coding agents to load. - -Skills are written flat, one directory per skill (``//SKILL.md`` plus -bundled files), into both `.claude/skills` and `.agents/skills` — the pair that -covers every skills-capable agent ucode configures. The bytes come from the -download client in `databricks.py`; this module owns only the filesystem side. -""" +"""Write downloaded Unity Catalog skills to disk, one flat dir per skill.""" from __future__ import annotations @@ -13,12 +7,10 @@ from ucode.ui import print_warning, prompt_yes_no -# Cross-agent skill directories: `.claude/skills` (Claude) and `.agents/skills` -# (the Agent Skills alias the other agents read). Both get the same skills. -SKILL_DIR_NAMES = (".claude/skills", ".agents/skills") +# `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). +SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills") -# Agent Skills spec: a skill's directory name is its `name`, lowercase a-z 0-9 -. -_LEAF_PATTERN = re.compile(r"^[a-z0-9-]+$") +SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9-]+$") def skill_dir_roots(project_dir: str) -> list[Path]: @@ -31,11 +23,11 @@ def skill_dir_roots(project_dir: str) -> list[Path]: raise ValueError(f"--path must be an absolute path, got `{project_dir}`.") if not base.is_dir(): raise ValueError(f"--path directory does not exist: `{project_dir}`.") - return [base / name for name in SKILL_DIR_NAMES] + return [base / name for name in SKILL_BASE_DIR_NAMES] def _is_valid_leaf(leaf: str) -> bool: - return bool(_LEAF_PATTERN.match(leaf)) + return bool(SKILL_NAME_PATTERN.match(leaf)) def _safe_relative_path(relative_path: str) -> Path | None: @@ -61,15 +53,12 @@ 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], *, schema_ref: str) -> str: - """Write one skill's bundle into every root, prompting before overwriting. - - ``files`` maps each in-bundle relative path (including ``SKILL.md``) to its - bytes. If the skill's directory already exists in any root, ask before - overwriting it — the on-disk skill may be from a different schema, so we - never clobber it without consent. +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, location: str) -> str: + """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root. - Returns ``"written"``, ``"overwritten"``, ``"kept"``, or ``"skipped"``. + Prompts before overwriting an existing skill dir. ``location`` is the source + ``.``, shown in that prompt. Returns ``"written"``, + ``"overwritten"``, ``"kept"``, or ``"skipped"``. """ if not _is_valid_leaf(leaf): print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") @@ -77,7 +66,7 @@ def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, schema already_on_disk = any((root / leaf).exists() for root in roots) if already_on_disk and not prompt_yes_no( - f"A skill named `{leaf}` already exists. Overwrite it with `{schema_ref}.{leaf}`?" + f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" ): return "kept" diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 506d812..f57ab85 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -22,8 +22,8 @@ def test_missing_directory_rejected(self, tmp_path): skill_dir_roots(str(tmp_path / "nope")) -def _write(roots, leaf, files, *, schema_ref="main.default"): - return write_skill(roots, leaf, files, schema_ref=schema_ref) +def _write(roots, leaf, files, *, location="main.default"): + return write_skill(roots, leaf, files, location=location) class TestWriteSkill: @@ -40,20 +40,20 @@ def test_writes_bundle_into_every_root(self, tmp_path): 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"}, schema_ref="main.default") + _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) - status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, schema_ref="ml.prod") + status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") assert status == "kept" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" 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"}, schema_ref="main.default") + _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) - status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, schema_ref="ml.prod") + status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") assert status == "overwritten" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" From dce079ddc31b1b5745fe963059885099b7b7f493 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 02:34:46 +0000 Subject: [PATCH 06/11] skills: add fetch_skill_bundle to compose a skill's full download `fetch_skill_bundle(workspace, token, catalog, schema, leaf)` lists a skill's files then fetches each into a `{relpath: bytes}` map, all-or-nothing. This is the one-skill assembler the download command will call per leaf; keeping it in databricks.py (next to its list_skill_files/fetch_skill_file parts) keeps the writer transport-agnostic and the eventual command loop thin. Co-authored-by: Isaac --- src/ucode/databricks.py | 21 +++++++++++++++++++++ tests/test_databricks.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index 1374f6d..70d5f35 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1503,6 +1503,27 @@ def fetch_skill_file( return _http_get_bytes(url, token, timeout=30) +def fetch_skill_bundle( + workspace: str, token: str, catalog: str, schema: str, leaf: str +) -> tuple[dict[str, bytes] | None, str | None]: + """Fetch a whole skill bundle as ``{relative_path: bytes}``. + + Lists the skill's files then fetches each one. All-or-nothing: a non-None + 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) + 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) + if content is None: + return None, reason + bundle[relative_path] = content + return bundle, None + + # Maps the gateway routing dialect a coding tool speaks to the Model Provider # Service `provider_type`s it can be backed by. claude speaks Anthropic's API, # which both the `anthropic` and `amazon_bedrock` provider types serve (Bedrock diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 640a92b..1e3a164 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -868,6 +868,45 @@ def test_http_failure_propagates_reason(self, monkeypatch): assert reason == "HTTP 404 Not Found" +class TestFetchSkillBundle: + def test_assembles_relpath_to_bytes_map(self, monkeypatch): + contents = {"SKILL.md": b"# skill", "references/a.md": b"aaa"} + monkeypatch.setattr(db_mod, "list_skill_files", lambda *a, **k: (list(contents), None)) + monkeypatch.setattr( + db_mod, "fetch_skill_file", lambda ws, tok, c, s, leaf, rel: (contents[rel], None) + ) + + bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert reason is None + assert bundle == contents + + def test_listing_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr(db_mod, "list_skill_files", lambda *a, **k: ([], "HTTP 404 Not Found")) + + bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert bundle is None + assert reason == "HTTP 404 Not Found" + + def test_file_failure_aborts_whole_bundle(self, monkeypatch): + monkeypatch.setattr( + db_mod, "list_skill_files", lambda *a, **k: (["SKILL.md", "broken.md"], None) + ) + monkeypatch.setattr( + db_mod, + "fetch_skill_file", + lambda ws, tok, c, s, leaf, rel: ( + (b"ok", None) if rel == "SKILL.md" else (None, "HTTP 500 Server Error") + ), + ) + + bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert bundle is None + assert reason == "HTTP 500 Server Error" + + def _foundation_models_payload(names): return { "endpoints": [ From 96386aef9ce8553da654a692367b0baccae69b64 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 17:07:16 +0000 Subject: [PATCH 07/11] skills: wire download mode into configure skills Flip --mcp to optional and add a download-only --path. Without --mcp, configure skills downloads every skill in each --location schema into /.claude/skills + /.agents/skills and registers a schema-less skills MCP connection, leaving any prior --mcp scope intact. Adds configure_skills_download_command and the _skill_mcp_locations read accessor in mcp.py; download persists no disk state. Co-authored-by: Isaac --- AGENTS.md | 2 +- README.md | 24 +++++++++ src/ucode/cli.py | 27 ++++++---- src/ucode/mcp.py | 39 +++++++++++++++ tests/test_cli.py | 26 +++++++++- tests/test_mcp.py | 124 ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 230 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index deeb6c7..8dccc6f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Tests live in `tests/`. - Use Python 3.12+. - Keep changes scoped to the requested behavior. -- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, and presentation helpers in `ui.py`. +- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, filesystem writes for downloaded skills in `skills_download.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` and registers only the schema-less skills MCP connection. - Prefer existing helpers for config file writes, state persistence, UI messages, and Databricks authentication. - Add or update focused tests for behavior changes. - Do not modify generated or lock files unless the dependency graph intentionally changes. diff --git a/README.md b/README.md index dad25e0..a0a3b4a 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,28 @@ ucode configure --agents claude --mcp system.ai.slack `--mcp` also works without `--agents` for MCP-only clients (it configures just the workspace, then registers the servers); pass a comma-separated list to register several at once. +### Skills (optional) + +Configure Unity Catalog Skills for your coding tools with `ucode configure skills`. It has two +mutually-exclusive modes, both scoped by `--location .` (comma-separated for +multiple schemas): + +```bash +# Download mode (default): fetch every skill in the schema to disk. +ucode configure skills --location main.default --path /abs/project/dir + +# MCP mode: expose the schema's skills as MCP tools instead of downloading. +ucode configure skills --location main.default,ml.prod --mcp +``` + +- **Download mode** writes each skill flat as `/SKILL.md` (plus its bundled files) into both + `/.claude/skills/` and `/.agents/skills/`. `--path` is required and must be an + existing absolute directory. Any pre-existing skill dir prompts before it's overwritten. It then + registers a schema-less skills MCP connection (utility tools only), leaving any prior `--mcp` + scope untouched. +- **MCP mode** sets the connection's location set to exactly `` (override-only) and rebuilds + its `?schema=` URL; no files are downloaded and `--path` is rejected. + --- ## Other Commands @@ -118,6 +140,8 @@ then registers the servers); pass a comma-separated list to register several at | `ucode configure --profiles DEFAULT --use-pat` | Authenticate with the profile's personal access token — no browser login | | `ucode configure --skip-validate` | Write configs without sending a test message through each agent | | `ucode configure --agents claude --mcp system.ai.slack` | Configure an agent and register its Databricks MCP server(s) in one command | +| `ucode configure skills --location main.default --path ` | Download a schema's skills into `` and register a schema-less skills MCP connection | +| `ucode configure skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | ## Managed Local Files diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 865612d..5255fe6 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -55,6 +55,7 @@ MCP_CLIENTS, SKILLS_MCP_KIND, configure_mcp_command, + configure_skills_download_command, configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, @@ -1403,21 +1404,29 @@ def configure_skills( ], mcp: Annotated[ bool, - typer.Option( - "--mcp", help="Manage the skills MCP connection (required until download mode lands)." - ), + typer.Option("--mcp", help="Mutate the skills MCP connection instead of downloading."), ] = False, + path: Annotated[ + str | None, + typer.Option("--path", help="(download) Existing absolute project dir to download into."), + ] = None, ) -> None: """Configure Databricks Skills for your coding tools. - ``--location`` sets the skills MCP connection's scope to exactly the listed - schemas, replacing any previous set. + By default, downloads every skill in each ``--location`` schema into + ``--path`` and registers a schema-less MCP connection. With ``--mcp``, + instead sets the skills MCP connection's scope to exactly the listed schemas. """ try: - if not mcp: - raise RuntimeError("Download mode is not available yet; pass --mcp for now.") - configure_skills_mcp_command(_parse_skill_locations(location)) - except RuntimeError as exc: + if mcp: + if path is not None: + raise RuntimeError("--path is not valid with --mcp.") + configure_skills_mcp_command(_parse_skill_locations(location)) + else: + if path is None: + raise RuntimeError("--path is required for download.") + configure_skills_download_command(_parse_skill_locations(location), path=path) + except (RuntimeError, ValueError) as exc: print_err(str(exc)) raise typer.Exit(1) from None except KeyboardInterrupt: diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 9ab931f..7f1796f 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -31,15 +31,18 @@ build_mcp_service_url, build_skills_mcp_url, ensure_databricks_auth, + fetch_skill_bundle, get_databricks_token, list_databricks_apps, list_databricks_connections, list_genie_spaces, list_mcp_services, + list_schema_skills, list_uc_functions_catalog_schemas, list_vector_search_catalog_schemas, workspace_hostname, ) +from ucode.skills_download import skill_dir_roots, write_skill from ucode.state import load_full_state, load_state, save_state from ucode.ui import ( print_note, @@ -1427,3 +1430,39 @@ def configure_skills_mcp_command(locations: list[str]) -> int: workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") _update_skills_mcp(state, workspace, clients, locations) return 0 + + +def _skill_mcp_locations(state: dict) -> list[str]: + """The skills MCP connection's ``skill_locations``, or ``[]`` if none exists.""" + entry = next(iter(_skills_entries(list(state.get("mcp_servers") or []))), None) + return list((entry or {}).get("skill_locations") or []) + + +def configure_skills_download_command(locations: list[str], *, path: str) -> int: + """Download every skill in each schema to ``path`` and register the skills connection. + + Writes each skill's bundle into ``/.claude/skills`` and + ``/.agents/skills`` (prompting before overwriting an existing dir), then + registers/keeps the schema-less MCP connection. ``skill_locations`` is never + touched, so a prior ``--mcp`` set survives a download run. + """ + state = load_state() + workspace, profile, clients = _setup_mcp_clients(state, "Skills") + token = get_databricks_token(workspace, profile) + roots = skill_dir_roots(path) + + for location in locations: + catalog, schema = location.split(".") + leaves, reason = list_schema_skills(workspace, token, catalog, schema) + if reason: + print_warning(f"Skipping `{location}`: {reason}.") + continue + for leaf in leaves: + files, reason = fetch_skill_bundle(workspace, token, catalog, schema, leaf) + if reason or files is None: + print_warning(f"Skipping `{location}.{leaf}`: {reason}.") + continue + write_skill(roots, leaf, files, location=location) + + _update_skills_mcp(state, workspace, clients, _skill_mcp_locations(state)) + return 0 diff --git a/tests/test_cli.py b/tests/test_cli.py index c66c9c1..df6f9f8 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -364,11 +364,33 @@ def test_comma_location_yields_multiple_schemas(self): assert result.exit_code == 0, result.output mock_mcp.assert_called_once_with(["a.b", "c.d"]) - def test_without_mcp_is_not_implemented_exit_1(self): - with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: + def test_default_mode_dispatches_download_with_path(self): + with patch("ucode.cli.configure_skills_download_command") as mock_download: + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--path", "/tmp/skills"] + ) + assert result.exit_code == 0, result.output + mock_download.assert_called_once_with(["a.b"], path="/tmp/skills") + + def test_download_without_path_exit_1(self): + with patch("ucode.cli.configure_skills_download_command") as mock_download: result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) assert result.exit_code == 1 + assert "--path" in _strip_ansi(result.output) + mock_download.assert_not_called() + + def test_path_with_mcp_exit_1(self): + with ( + patch("ucode.cli.configure_skills_mcp_command") as mock_mcp, + patch("ucode.cli.configure_skills_download_command") as mock_download, + ): + result = runner.invoke( + app, ["configure", "skills", "--location", "a.b", "--mcp", "--path", "/tmp/skills"] + ) + assert result.exit_code == 1 + assert "--path" in _strip_ansi(result.output) mock_mcp.assert_not_called() + mock_download.assert_not_called() def test_three_part_location_exit_1(self): with patch("ucode.cli.configure_skills_mcp_command") as mock_mcp: diff --git a/tests/test_mcp.py b/tests/test_mcp.py index a520afa..85165d3 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1876,6 +1876,88 @@ def test_preserves_mcp_service_entries_across_set(self, monkeypatch): assert names.count(mcp.SKILLS_MCP_SERVER_NAME) == 1 +class TestSkillMcpLocations: + def test_reads_locations_off_skills_entry(self): + state = _skills_state(mcp._resolve_skills_mcp_servers(WS, ["claude"], ["A.a", "B.b"], [])) + assert mcp._skill_mcp_locations(state) == ["A.a", "B.b"] + + def test_empty_when_no_skills_entry(self): + assert mcp._skill_mcp_locations(_skills_state([])) == [] + assert mcp._skill_mcp_locations(_skills_state()) == [] + + +class TestConfigureSkillsDownloadCommand: + def _stub_download(self, monkeypatch, state, *, leaves, bundle=(None, None)): + saved_states: list[dict] = [] + written: list[tuple[str, str]] = [] + _stub_location_base(monkeypatch, state) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + monkeypatch.setattr(mcp, "skill_dir_roots", lambda path: [f"{path}/.claude/skills"]) + monkeypatch.setattr(mcp, "list_schema_skills", lambda *a, **kw: leaves) + monkeypatch.setattr(mcp, "fetch_skill_bundle", lambda *a, **kw: bundle) + monkeypatch.setattr( + mcp, + "write_skill", + lambda roots, leaf, files, *, location: written.append((location, leaf)) or "written", + ) + return saved_states, written + + def test_downloads_leaves_and_registers_schemaless_connection(self, monkeypatch): + saved_states, written = self._stub_download( + monkeypatch, + _skills_state(), + leaves=(["pii-handling", "triage"], None), + bundle=({"SKILL.md": b"body"}, None), + ) + + assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + + assert written == [("a.b", "pii-handling"), ("a.b", "triage")] + skills = _find_skills(saved_states[-1]["mcp_servers"]) + assert len(skills) == 1 + assert skills[0]["skill_locations"] == [] + assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" + + def test_preserves_prior_mcp_location_set(self, monkeypatch): + prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["X.x", "Y.y"], []) + state = _skills_state(prior) + self._stub_download( + monkeypatch, + state, + leaves=(["pii-handling"], None), + bundle=({"SKILL.md": b"body"}, None), + ) + + assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + + assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] + + def test_bundle_failure_warns_and_still_exits_0(self, monkeypatch): + saved_states, written = self._stub_download( + monkeypatch, + _skills_state(), + leaves=(["pii-handling"], None), + bundle=(None, "HTTP 500 Server Error"), + ) + + assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + + assert written == [] + assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == [] + + def test_list_failure_skips_location(self, monkeypatch): + saved_states, written = self._stub_download( + monkeypatch, + _skills_state(), + leaves=([], "HTTP 404 Not Found"), + ) + + assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + + assert written == [] + + class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): removed: list[tuple[str, str]] = [] @@ -1922,3 +2004,45 @@ def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkey "opencode": True, "copilot": True, } + + def test_removes_skills_registry_across_its_clients(self, monkeypatch): + removed: list[tuple[str, str]] = [] + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or ["user"], + ) + monkeypatch.setattr(mcp, "restore_file", lambda *a, **kw: False) + + skills_entry = mcp._resolve_skills_mcp_servers(WS, ["claude", "codex"], ["a.b"], [])[0] + mcp.revert_mcp_configs({"mcp_servers": [skills_entry]}) + + assert removed == [ + ("claude", mcp.SKILLS_MCP_SERVER_NAME), + ("codex", mcp.SKILLS_MCP_SERVER_NAME), + ] + + +class TestPurgeCrossWorkspaceSkillsEntry: + def test_drops_foreign_workspace_skills_entry(self, monkeypatch): + removed: list[tuple[str, str]] = [] + saved_states: list[dict] = [] + foreign = "https://other.databricks.com" + skills_entry = mcp._resolve_skills_mcp_servers(foreign, ["claude"], ["a.b"], [])[0] + # The skills URL carries a `?schema=` query; its host must still parse. + assert mcp._mcp_entry_url_host(skills_entry) == "other.databricks.com" + state = {"mcp_servers": [skills_entry]} + + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) + monkeypatch.setattr(mcp, "load_full_state", lambda: {}) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or ["user"], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + mcp.purge_cross_workspace_mcp_residue(state, WS) + + assert removed == [("claude", mcp.SKILLS_MCP_SERVER_NAME)] + assert state["mcp_servers"] == [] From 8f3959038a43d54a971b924af370cef0333dc269 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 18:10:35 +0000 Subject: [PATCH 08/11] =?UTF-8?q?skills:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?consolidate=20download=20in=20skills=5Fdownload.py?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move the UC skill-download client (list/fetch) and the download orchestration out of databricks.py and mcp.py into skills_download.py, so mcp.py keeps only MCP-connection glue and databricks.py only the shared HTTP transport helpers. - Make --path optional: omitted, downloads write under the home dir. - Parallelize per-skill bundle fetches; keep writes sequential (prompts). Add progress messages so users see skills being downloaded. - Drop write_skill's unused return value; it prints its own outcome. Co-authored-by: Isaac --- AGENTS.md | 2 +- README.md | 10 +- src/ucode/cli.py | 14 +- src/ucode/databricks.py | 117 -------------- src/ucode/mcp.py | 30 +--- src/ucode/skills_download.py | 209 +++++++++++++++++++++++-- tests/test_cli.py | 7 +- tests/test_databricks.py | 214 -------------------------- tests/test_mcp.py | 62 ++------ tests/test_skills_download.py | 279 ++++++++++++++++++++++++++++++++-- 10 files changed, 500 insertions(+), 444 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8dccc6f..7810cb9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,7 +20,7 @@ Tests live in `tests/`. - Use Python 3.12+. - Keep changes scoped to the requested behavior. -- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, filesystem writes for downloaded skills in `skills_download.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` and registers only the schema-less skills MCP connection. +- Follow the existing module boundaries: CLI orchestration in `cli.py`, agent-specific behavior in `agents/.py`, shared agent dispatch in `agents/__init__.py`, Databricks calls in `databricks.py`, skill download (UC fetch client + on-disk writer + download orchestration) in `skills_download.py`, MCP-connection state glue in `mcp.py`, and presentation helpers in `ui.py`. Skill download persists no disk state — it writes files to `--path` (or the home dir) and registers only the schema-less skills MCP connection. - Prefer existing helpers for config file writes, state persistence, UI messages, and Databricks authentication. - Add or update focused tests for behavior changes. - Do not modify generated or lock files unless the dependency graph intentionally changes. diff --git a/README.md b/README.md index a0a3b4a..346fb80 100644 --- a/README.md +++ b/README.md @@ -117,10 +117,10 @@ ucode configure skills --location main.default,ml.prod --mcp ``` - **Download mode** writes each skill flat as `/SKILL.md` (plus its bundled files) into both - `/.claude/skills/` and `/.agents/skills/`. `--path` is required and must be an - existing absolute directory. Any pre-existing skill dir prompts before it's overwritten. It then - registers a schema-less skills MCP connection (utility tools only), leaving any prior `--mcp` - scope untouched. + `.claude/skills/` and `.agents/skills/`. `--path` (an existing absolute directory) is optional; + when omitted, skills are written under your home directory. Any pre-existing skill dir prompts + before it's overwritten. It then registers a schema-less skills MCP connection (utility tools + only), leaving any prior `--mcp` scope untouched. - **MCP mode** sets the connection's location set to exactly `` (override-only) and rebuilds its `?schema=` URL; no files are downloaded and `--path` is rejected. @@ -140,7 +140,7 @@ ucode configure skills --location main.default,ml.prod --mcp | `ucode configure --profiles DEFAULT --use-pat` | Authenticate with the profile's personal access token — no browser login | | `ucode configure --skip-validate` | Write configs without sending a test message through each agent | | `ucode configure --agents claude --mcp system.ai.slack` | Configure an agent and register its Databricks MCP server(s) in one command | -| `ucode configure skills --location main.default --path ` | Download a schema's skills into `` and register a schema-less skills MCP connection | +| `ucode configure skills --location main.default [--path ]` | Download a schema's skills to disk (under ``, or your home dir) and register a schema-less skills MCP connection | | `ucode configure skills --location main.default --mcp` | Expose a schema's skills as MCP tools (override-only) instead of downloading | ## Managed Local Files diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 5255fe6..48415fa 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -1408,14 +1408,18 @@ def configure_skills( ] = False, path: Annotated[ str | None, - typer.Option("--path", help="(download) Existing absolute project dir to download into."), + typer.Option( + "--path", + help="(download) Existing absolute dir to download into; defaults to your home dir.", + ), ] = None, ) -> None: """Configure Databricks Skills for your coding tools. - By default, downloads every skill in each ``--location`` schema into - ``--path`` and registers a schema-less MCP connection. With ``--mcp``, - instead sets the skills MCP connection's scope to exactly the listed schemas. + By default, downloads every skill in each ``--location`` schema to disk + (under ``--path``, or your home dir when omitted) and registers a schema-less + MCP connection. With ``--mcp``, instead sets the skills MCP connection's scope + to exactly the listed schemas. """ try: if mcp: @@ -1423,8 +1427,6 @@ def configure_skills( raise RuntimeError("--path is not valid with --mcp.") configure_skills_mcp_command(_parse_skill_locations(location)) else: - if path is None: - raise RuntimeError("--path is required for download.") configure_skills_download_command(_parse_skill_locations(location), path=path) except (RuntimeError, ValueError) as exc: print_err(str(exc)) diff --git a/src/ucode/databricks.py b/src/ucode/databricks.py index 70d5f35..f9e3f1b 100644 --- a/src/ucode/databricks.py +++ b/src/ucode/databricks.py @@ -1407,123 +1407,6 @@ def build_skills_mcp_url(workspace: str, locations: list[str]) -> str: return base + "?" + urlencode([("schema", loc) for loc in locations]) -# --- Skill download (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. - - 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/..``). - """ - 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 - - -def list_schema_skills( - workspace: str, token: str, catalog: str, schema: str -) -> tuple[list[str], str | None]: - """List the finalized skill leaf names in ``.``. - - A non-None reason indicates the listing call itself failed. - """ - hostname = workspace_hostname(workspace) - base_url = f"https://{hostname}/api/2.1/unity-catalog/skills" - query = {"parent": f"schemas/{catalog}.{schema}"} - - leaves: list[str] = [] - page_token: str | None = None - while True: - if page_token: - query["page_token"] = page_token - payload, reason = _http_get_json(f"{base_url}?{urlencode(query)}", token, timeout=30) - if payload is None: - return [], reason - data = cast(dict, 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) - page_token = data.get("next_page_token") - if not page_token: - return leaves, None - - -def list_skill_files( - workspace: str, token: str, catalog: str, schema: str, leaf: str -) -> 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``). - 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}/" - - relative_paths: list[str] = [] - pending = [f"Volumes/{catalog}/{schema}/{leaf}"] - while pending: - directory = pending.pop() - page_token: str | None = None - while True: - url = f"{dirs_base}/{directory}" - if page_token: - url = f"{url}?{urlencode({'page_token': page_token})}" - payload, reason = _http_get_json(url, token, timeout=30) - if payload is None: - return [], reason - data = cast(dict, payload) if isinstance(payload, dict) else {} - for entry in data.get("contents") or []: - path = entry.get("path") if isinstance(entry, dict) else None - if not isinstance(path, str): - continue - if entry.get("is_directory"): - pending.append(path.strip("/")) - else: - relative_paths.append(path.removeprefix(volume_prefix)) - page_token = data.get("next_page_token") - if not page_token: - break - return relative_paths, None - - -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.""" - hostname = workspace_hostname(workspace) - url = f"https://{hostname}/api/2.0/fs/files/Volumes/{catalog}/{schema}/{leaf}/{relative_path}" - return _http_get_bytes(url, token, timeout=30) - - -def fetch_skill_bundle( - workspace: str, token: str, catalog: str, schema: str, leaf: str -) -> tuple[dict[str, bytes] | None, str | None]: - """Fetch a whole skill bundle as ``{relative_path: bytes}``. - - Lists the skill's files then fetches each one. All-or-nothing: a non-None - 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) - 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) - if content is None: - return None, reason - bundle[relative_path] = content - return bundle, None - - # Maps the gateway routing dialect a coding tool speaks to the Model Provider # Service `provider_type`s it can be backed by. claude speaks Anthropic's API, # which both the `anthropic` and `amazon_bedrock` provider types serve (Bedrock diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 7f1796f..0857e1d 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -31,18 +31,16 @@ build_mcp_service_url, build_skills_mcp_url, ensure_databricks_auth, - fetch_skill_bundle, get_databricks_token, list_databricks_apps, list_databricks_connections, list_genie_spaces, list_mcp_services, - list_schema_skills, list_uc_functions_catalog_schemas, list_vector_search_catalog_schemas, workspace_hostname, ) -from ucode.skills_download import skill_dir_roots, write_skill +from ucode.skills_download import download_skills from ucode.state import load_full_state, load_state, save_state from ucode.ui import ( print_note, @@ -1438,31 +1436,19 @@ def _skill_mcp_locations(state: dict) -> list[str]: return list((entry or {}).get("skill_locations") or []) -def configure_skills_download_command(locations: list[str], *, path: str) -> int: - """Download every skill in each schema to ``path`` and register the skills connection. +def configure_skills_download_command(locations: list[str], *, path: str | None) -> int: + """Download every skill in each schema to disk and register the skills connection. - Writes each skill's bundle into ``/.claude/skills`` and - ``/.agents/skills`` (prompting before overwriting an existing dir), then - registers/keeps the schema-less MCP connection. ``skill_locations`` is never - touched, so a prior ``--mcp`` set survives a download run. + Delegates the download to ``download_skills`` (files land under ``path`` or, + when ``path`` is None, the user home dir), then registers/keeps the + schema-less MCP connection. ``skill_locations`` is never touched, so a prior + ``--mcp`` set survives a download run. """ state = load_state() workspace, profile, clients = _setup_mcp_clients(state, "Skills") token = get_databricks_token(workspace, profile) - roots = skill_dir_roots(path) - for location in locations: - catalog, schema = location.split(".") - leaves, reason = list_schema_skills(workspace, token, catalog, schema) - if reason: - print_warning(f"Skipping `{location}`: {reason}.") - continue - for leaf in leaves: - files, reason = fetch_skill_bundle(workspace, token, catalog, schema, leaf) - if reason or files is None: - print_warning(f"Skipping `{location}.{leaf}`: {reason}.") - continue - write_skill(roots, leaf, files, location=location) + download_skills(workspace, token, locations, path) _update_skills_mcp(state, workspace, clients, _skill_mcp_locations(state)) return 0 diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 1f259b0..0023922 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -1,28 +1,159 @@ -"""Write downloaded Unity Catalog skills to disk, one flat dir per skill.""" +"""Download Unity Catalog skills and write them to disk, one flat dir per skill.""" from __future__ import annotations import re +from concurrent.futures import ThreadPoolExecutor, as_completed from pathlib import Path +from typing import cast +from urllib.parse import urlencode -from ucode.ui import print_warning, prompt_yes_no +from ucode.databricks import _http_get_bytes, _http_get_json, workspace_hostname +from ucode.ui import print_note, print_success, print_warning, prompt_yes_no # `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills") SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9-]+$") +# Parallel skill fetches per schema; writes stay sequential (they prompt). +_MAX_FETCH_WORKERS = 8 -def skill_dir_roots(project_dir: str) -> list[Path]: - """The ``.claude/skills`` and ``.agents/skills`` roots under ``project_dir``. - ``project_dir`` must be an existing absolute directory. +# --- 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. + + 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/..``). + """ + 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 + + +def list_schema_skills( + workspace: str, token: str, catalog: str, schema: str +) -> tuple[list[str], str | None]: + """List the finalized skill leaf names in ``.``. + + A non-None reason indicates the listing call itself failed. + """ + hostname = workspace_hostname(workspace) + base_url = f"https://{hostname}/api/2.1/unity-catalog/skills" + query = {"parent": f"schemas/{catalog}.{schema}"} + + leaves: list[str] = [] + page_token: str | None = None + while True: + if page_token: + query["page_token"] = page_token + payload, reason = _http_get_json(f"{base_url}?{urlencode(query)}", token, timeout=30) + if payload is None: + return [], reason + data = cast(dict, 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) + page_token = data.get("next_page_token") + if not page_token: + return leaves, None + + +def list_skill_files( + workspace: str, token: str, catalog: str, schema: str, leaf: str +) -> 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``). + 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}/" + + relative_paths: list[str] = [] + pending = [f"Volumes/{catalog}/{schema}/{leaf}"] + while pending: + directory = pending.pop() + page_token: str | None = None + while True: + url = f"{dirs_base}/{directory}" + if page_token: + url = f"{url}?{urlencode({'page_token': page_token})}" + payload, reason = _http_get_json(url, token, timeout=30) + if payload is None: + return [], reason + data = cast(dict, payload) if isinstance(payload, dict) else {} + for entry in data.get("contents") or []: + path = entry.get("path") if isinstance(entry, dict) else None + if not isinstance(path, str): + continue + if entry.get("is_directory"): + pending.append(path.strip("/")) + else: + relative_paths.append(path.removeprefix(volume_prefix)) + page_token = data.get("next_page_token") + if not page_token: + break + return relative_paths, None + + +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.""" + hostname = workspace_hostname(workspace) + url = f"https://{hostname}/api/2.0/fs/files/Volumes/{catalog}/{schema}/{leaf}/{relative_path}" + return _http_get_bytes(url, token, timeout=30) + + +def fetch_skill_bundle( + workspace: str, token: str, catalog: str, schema: str, leaf: str +) -> tuple[dict[str, bytes] | None, str | None]: + """Fetch a whole skill bundle as ``{relative_path: bytes}``. + + Lists the skill's files then fetches each one. All-or-nothing: a non-None + 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) + 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) + if content is None: + return None, reason + bundle[relative_path] = content + return bundle, None + + +# --- On-disk writer -------------------------------------------------------- + + +def skill_dir_roots(project_dir: str | None) -> list[Path]: + """The ``.claude/skills`` and ``.agents/skills`` roots to download into. + + ``project_dir`` must be an existing absolute directory when given; when + omitted, roots default to the user's home directory (user scope). """ - base = Path(project_dir) - if not base.is_absolute(): - raise ValueError(f"--path must be an absolute path, got `{project_dir}`.") - if not base.is_dir(): - raise ValueError(f"--path directory does not exist: `{project_dir}`.") + if project_dir is None: + base = Path.home() + else: + base = Path(project_dir) + if not base.is_absolute(): + raise ValueError(f"--path must be an absolute path, got `{project_dir}`.") + if not base.is_dir(): + raise ValueError(f"--path directory does not exist: `{project_dir}`.") return [base / name for name in SKILL_BASE_DIR_NAMES] @@ -53,24 +184,70 @@ 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) -> str: +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, location: str) -> None: """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root. Prompts before overwriting an existing skill dir. ``location`` is the source - ``.``, shown in that prompt. Returns ``"written"``, - ``"overwritten"``, ``"kept"``, or ``"skipped"``. + ``.``, 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 "skipped" + return already_on_disk = any((root / leaf).exists() for root in roots) if already_on_disk and not prompt_yes_no( f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" ): - return "kept" + print_note(f"Kept existing `{leaf}`.") + return for root in roots: _write_bundle(root / leaf, leaf, files) + verb = "Overwrote" if already_on_disk else "Downloaded" + print_success(f"{verb} `{leaf}` ({len(files)} file(s)).") + + +# --- Orchestration --------------------------------------------------------- + + +def _fetch_bundles( + workspace: str, token: str, catalog: str, schema: str, leaves: list[str] +) -> dict[str, tuple[dict[str, bytes] | None, str | None]]: + """Fetch every leaf's bundle concurrently, keyed by leaf name.""" + results: dict[str, tuple[dict[str, bytes] | None, str | None]] = {} + with ThreadPoolExecutor(max_workers=min(_MAX_FETCH_WORKERS, len(leaves))) as pool: + futures = { + pool.submit(fetch_skill_bundle, workspace, token, catalog, schema, leaf): leaf + for leaf in leaves + } + for future in as_completed(futures): + results[futures[future]] = future.result() + return results + + +def download_skills(workspace: str, token: str, locations: list[str], path: str | None) -> None: + """Download every skill in each ``.`` location to disk. + + Bundles are fetched concurrently per schema, then written sequentially (so + overwrite prompts don't interleave). A failure on one skill warns and skips + it without aborting the batch. + """ + roots = skill_dir_roots(path) + for location in locations: + catalog, schema = location.split(".") + leaves, reason = list_schema_skills(workspace, token, catalog, schema) + if reason: + print_warning(f"Skipping `{location}`: {reason}.") + continue + if not leaves: + print_note(f"No skills found in `{location}`.") + continue - return "overwritten" if already_on_disk else "written" + print_note(f"Downloading {len(leaves)} skill(s) from `{location}`...") + bundles = _fetch_bundles(workspace, token, catalog, schema, leaves) + for leaf in leaves: + files, reason = bundles[leaf] + if reason or files is None: + print_warning(f"Skipping `{location}.{leaf}`: {reason}.") + continue + write_skill(roots, leaf, files, location=location) diff --git a/tests/test_cli.py b/tests/test_cli.py index df6f9f8..8e7473d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -372,12 +372,11 @@ def test_default_mode_dispatches_download_with_path(self): assert result.exit_code == 0, result.output mock_download.assert_called_once_with(["a.b"], path="/tmp/skills") - def test_download_without_path_exit_1(self): + def test_default_mode_without_path_dispatches_download(self): with patch("ucode.cli.configure_skills_download_command") as mock_download: result = runner.invoke(app, ["configure", "skills", "--location", "a.b"]) - assert result.exit_code == 1 - assert "--path" in _strip_ansi(result.output) - mock_download.assert_not_called() + assert result.exit_code == 0, result.output + mock_download.assert_called_once_with(["a.b"], path=None) def test_path_with_mcp_exit_1(self): with ( diff --git a/tests/test_databricks.py b/tests/test_databricks.py index 1e3a164..212ca62 100644 --- a/tests/test_databricks.py +++ b/tests/test_databricks.py @@ -693,220 +693,6 @@ def test_http_404_reason_surfaces_for_invalid_parent(self, monkeypatch): assert reason and reason.startswith("HTTP 404") -class TestListSchemaSkills: - def test_keeps_finalized_skills_and_uses_bundle_name(self, monkeypatch): - payload = { - "skills": [ - { - "name": "skills/main.default.pii-handling", - "bundle_name": "pii-handling", - "finalize_time": "2026-06-26T05:58:25Z", - }, - { - "name": "skills/main.default.triage", - "bundle_name": "triage", - "finalize_time": "2026-06-26T05:58:26Z", - }, - {"name": "skills/main.default.draft", "bundle_name": "draft"}, - ] - } - monkeypatch.setattr( - db_mod, "_http_get_json", lambda url, token, timeout=30: (payload, None) - ) - - leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") - - assert reason is None - assert leaves == ["pii-handling", "triage"] - - def test_falls_back_to_resource_name_leaf(self, monkeypatch): - payload = { - "skills": [ - { - "name": "skills/main.default.pii-handling", - "finalize_time": "2026-06-26T05:58:25Z", - } - ] - } - monkeypatch.setattr( - db_mod, "_http_get_json", lambda url, token, timeout=30: (payload, None) - ) - - leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") - - assert reason is None - assert leaves == ["pii-handling"] - - 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"}]}, - ] - captured_tokens = [] - - def fake_get(url, token, timeout=30): - captured_tokens.append("page_token=tok" in url) - return pages.pop(0), None - - monkeypatch.setattr(db_mod, "_http_get_json", fake_get) - - leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") - - assert reason is None - assert leaves == ["a", "b"] - assert captured_tokens == [False, True] - - def test_targets_uc_skills_api_for_the_schema(self, monkeypatch): - captured = {} - - def fake_get(url, token, timeout=30): - captured["url"] = url - return {"skills": []}, None - - monkeypatch.setattr(db_mod, "_http_get_json", fake_get) - - db_mod.list_schema_skills(WS, "token", "main", "default") - - assert "/api/2.1/unity-catalog/skills?" in captured["url"] - assert "parent=schemas%2Fmain.default" in captured["url"] - - def test_http_failure_propagates_reason(self, monkeypatch): - monkeypatch.setattr( - db_mod, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 500 Server Error") - ) - - leaves, reason = db_mod.list_schema_skills(WS, "token", "main", "default") - - assert leaves == [] - assert reason == "HTTP 500 Server Error" - - -class TestListSkillFiles: - def test_walks_nested_directories_into_relative_paths(self, monkeypatch): - # The Files API returns absolute `/Volumes/...` paths. - vol = "/Volumes/main/default/triage" - listings = { - "Volumes/main/default/triage": { - "contents": [ - {"path": f"{vol}/SKILL.md", "is_directory": False}, - {"path": f"{vol}/references/", "is_directory": True}, - ] - }, - "Volumes/main/default/triage/references": { - "contents": [{"path": f"{vol}/references/primary.md", "is_directory": False}] - }, - } - - def fake_get(url, token, timeout=30): - directory = url.split("/api/2.0/fs/directories/", 1)[1] - return listings[directory], None - - monkeypatch.setattr(db_mod, "_http_get_json", fake_get) - - paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") - - assert reason is None - assert sorted(paths) == ["SKILL.md", "references/primary.md"] - - def test_follows_pagination(self, monkeypatch): - vol = "/Volumes/main/default/triage" - pages = [ - { - "contents": [{"path": f"{vol}/a.md", "is_directory": False}], - "next_page_token": "tok", - }, - {"contents": [{"path": f"{vol}/b.md", "is_directory": False}]}, - ] - - monkeypatch.setattr( - db_mod, "_http_get_json", lambda url, token, timeout=30: (pages.pop(0), None) - ) - - paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") - - assert reason is None - assert sorted(paths) == ["a.md", "b.md"] - - def test_http_failure_propagates_reason(self, monkeypatch): - monkeypatch.setattr( - db_mod, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") - ) - - paths, reason = db_mod.list_skill_files(WS, "token", "main", "default", "triage") - - assert paths == [] - assert reason == "HTTP 404 Not Found" - - -class TestFetchSkillFile: - def test_returns_raw_bytes_from_files_api(self, monkeypatch): - captured = {} - - def fake_get_bytes(url, token, timeout=30): - captured["url"] = url - return b"# SKILL\n", None - - monkeypatch.setattr(db_mod, "_http_get_bytes", fake_get_bytes) - - body, reason = db_mod.fetch_skill_file(WS, "token", "main", "default", "triage", "SKILL.md") - - 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" - - def test_http_failure_propagates_reason(self, monkeypatch): - monkeypatch.setattr( - db_mod, "_http_get_bytes", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") - ) - - body, reason = db_mod.fetch_skill_file(WS, "token", "main", "default", "triage", "gone.md") - - assert body is None - assert reason == "HTTP 404 Not Found" - - -class TestFetchSkillBundle: - def test_assembles_relpath_to_bytes_map(self, monkeypatch): - contents = {"SKILL.md": b"# skill", "references/a.md": b"aaa"} - monkeypatch.setattr(db_mod, "list_skill_files", lambda *a, **k: (list(contents), None)) - monkeypatch.setattr( - db_mod, "fetch_skill_file", lambda ws, tok, c, s, leaf, rel: (contents[rel], None) - ) - - bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") - - assert reason is None - assert bundle == contents - - def test_listing_failure_propagates_reason(self, monkeypatch): - monkeypatch.setattr(db_mod, "list_skill_files", lambda *a, **k: ([], "HTTP 404 Not Found")) - - bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") - - assert bundle is None - assert reason == "HTTP 404 Not Found" - - def test_file_failure_aborts_whole_bundle(self, monkeypatch): - monkeypatch.setattr( - db_mod, "list_skill_files", lambda *a, **k: (["SKILL.md", "broken.md"], None) - ) - monkeypatch.setattr( - db_mod, - "fetch_skill_file", - lambda ws, tok, c, s, leaf, rel: ( - (b"ok", None) if rel == "SKILL.md" else (None, "HTTP 500 Server Error") - ), - ) - - bundle, reason = db_mod.fetch_skill_bundle(WS, "token", "main", "default", "triage") - - assert bundle is None - assert reason == "HTTP 500 Server Error" - - def _foundation_models_payload(names): return { "endpoints": [ diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 85165d3..4d7e052 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1887,76 +1887,46 @@ def test_empty_when_no_skills_entry(self): class TestConfigureSkillsDownloadCommand: - def _stub_download(self, monkeypatch, state, *, leaves, bundle=(None, None)): + def _stub_download(self, monkeypatch, state): saved_states: list[dict] = [] - written: list[tuple[str, str]] = [] + download_calls: list[tuple] = [] _stub_location_base(monkeypatch, state) monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - monkeypatch.setattr(mcp, "skill_dir_roots", lambda path: [f"{path}/.claude/skills"]) - monkeypatch.setattr(mcp, "list_schema_skills", lambda *a, **kw: leaves) - monkeypatch.setattr(mcp, "fetch_skill_bundle", lambda *a, **kw: bundle) monkeypatch.setattr( mcp, - "write_skill", - lambda roots, leaf, files, *, location: written.append((location, leaf)) or "written", + "download_skills", + lambda ws, tok, locations, path: download_calls.append((ws, locations, path)), ) - return saved_states, written + return saved_states, download_calls - def test_downloads_leaves_and_registers_schemaless_connection(self, monkeypatch): - saved_states, written = self._stub_download( - monkeypatch, - _skills_state(), - leaves=(["pii-handling", "triage"], None), - bundle=({"SKILL.md": b"body"}, None), - ) + def test_downloads_and_registers_schemaless_connection(self, monkeypatch): + saved_states, download_calls = self._stub_download(monkeypatch, _skills_state()) assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 - assert written == [("a.b", "pii-handling"), ("a.b", "triage")] + assert download_calls == [(WS, ["a.b"], "/tmp/skills")] skills = _find_skills(saved_states[-1]["mcp_servers"]) assert len(skills) == 1 assert skills[0]["skill_locations"] == [] assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" + def test_none_path_threads_through(self, monkeypatch): + _, download_calls = self._stub_download(monkeypatch, _skills_state()) + + assert mcp.configure_skills_download_command(["a.b"], path=None) == 0 + + assert download_calls == [(WS, ["a.b"], None)] + def test_preserves_prior_mcp_location_set(self, monkeypatch): prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["X.x", "Y.y"], []) state = _skills_state(prior) - self._stub_download( - monkeypatch, - state, - leaves=(["pii-handling"], None), - bundle=({"SKILL.md": b"body"}, None), - ) + self._stub_download(monkeypatch, state) assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] - def test_bundle_failure_warns_and_still_exits_0(self, monkeypatch): - saved_states, written = self._stub_download( - monkeypatch, - _skills_state(), - leaves=(["pii-handling"], None), - bundle=(None, "HTTP 500 Server Error"), - ) - - assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 - - assert written == [] - assert _find_skills(saved_states[-1]["mcp_servers"])[0]["skill_locations"] == [] - - def test_list_failure_skips_location(self, monkeypatch): - saved_states, written = self._stub_download( - monkeypatch, - _skills_state(), - leaves=([], "HTTP 404 Not Found"), - ) - - assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 - - assert written == [] - class TestRevertMcpConfigs: def test_removes_cli_registered_servers_and_restores_copilot_config(self, monkeypatch): diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index f57ab85..cae037b 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -1,4 +1,5 @@ -"""Tests for skills_download.py — the on-disk skill writer (no network).""" +"""Tests for skills_download.py — the UC skill-download client, on-disk writer, +and download orchestration.""" from __future__ import annotations @@ -7,12 +8,226 @@ import ucode.skills_download as sd from ucode.skills_download import skill_dir_roots, write_skill +WS = "https://example.databricks.com" + + +class TestListSchemaSkills: + def test_keeps_finalized_skills_and_uses_bundle_name(self, monkeypatch): + payload = { + "skills": [ + { + "name": "skills/main.default.pii-handling", + "bundle_name": "pii-handling", + "finalize_time": "2026-06-26T05:58:25Z", + }, + { + "name": "skills/main.default.triage", + "bundle_name": "triage", + "finalize_time": "2026-06-26T05:58:26Z", + }, + {"name": "skills/main.default.draft", "bundle_name": "draft"}, + ] + } + monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + + leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["pii-handling", "triage"] + + def test_falls_back_to_resource_name_leaf(self, monkeypatch): + payload = { + "skills": [ + { + "name": "skills/main.default.pii-handling", + "finalize_time": "2026-06-26T05:58:25Z", + } + ] + } + monkeypatch.setattr(sd, "_http_get_json", lambda url, token, timeout=30: (payload, None)) + + leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["pii-handling"] + + 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"}]}, + ] + captured_tokens = [] + + def fake_get(url, token, timeout=30): + captured_tokens.append("page_token=tok" in url) + return pages.pop(0), None + + monkeypatch.setattr(sd, "_http_get_json", fake_get) + + leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert reason is None + assert leaves == ["a", "b"] + assert captured_tokens == [False, True] + + def test_targets_uc_skills_api_for_the_schema(self, monkeypatch): + captured = {} + + def fake_get(url, token, timeout=30): + captured["url"] = url + return {"skills": []}, None + + monkeypatch.setattr(sd, "_http_get_json", fake_get) + + sd.list_schema_skills(WS, "token", "main", "default") + + assert "/api/2.1/unity-catalog/skills?" in captured["url"] + assert "parent=schemas%2Fmain.default" in captured["url"] + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + sd, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 500 Server Error") + ) + + leaves, reason = sd.list_schema_skills(WS, "token", "main", "default") + + assert leaves == [] + assert reason == "HTTP 500 Server Error" + + +class TestListSkillFiles: + def test_walks_nested_directories_into_relative_paths(self, monkeypatch): + # The Files API returns absolute `/Volumes/...` paths. + vol = "/Volumes/main/default/triage" + listings = { + "Volumes/main/default/triage": { + "contents": [ + {"path": f"{vol}/SKILL.md", "is_directory": False}, + {"path": f"{vol}/references/", "is_directory": True}, + ] + }, + "Volumes/main/default/triage/references": { + "contents": [{"path": f"{vol}/references/primary.md", "is_directory": False}] + }, + } + + def fake_get(url, token, timeout=30): + directory = url.split("/api/2.0/fs/directories/", 1)[1] + return listings[directory], None + + monkeypatch.setattr(sd, "_http_get_json", fake_get) + + paths, reason = sd.list_skill_files(WS, "token", "main", "default", "triage") + + assert reason is None + assert sorted(paths) == ["SKILL.md", "references/primary.md"] + + def test_follows_pagination(self, monkeypatch): + vol = "/Volumes/main/default/triage" + pages = [ + { + "contents": [{"path": f"{vol}/a.md", "is_directory": False}], + "next_page_token": "tok", + }, + {"contents": [{"path": f"{vol}/b.md", "is_directory": False}]}, + ] + + monkeypatch.setattr( + sd, "_http_get_json", lambda url, token, timeout=30: (pages.pop(0), None) + ) + + paths, reason = sd.list_skill_files(WS, "token", "main", "default", "triage") + + assert reason is None + assert sorted(paths) == ["a.md", "b.md"] + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + sd, "_http_get_json", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") + ) + + paths, reason = sd.list_skill_files(WS, "token", "main", "default", "triage") + + assert paths == [] + assert reason == "HTTP 404 Not Found" + + +class TestFetchSkillFile: + def test_returns_raw_bytes_from_files_api(self, monkeypatch): + captured = {} + + def fake_get_bytes(url, token, timeout=30): + captured["url"] = url + return b"# SKILL\n", None + + monkeypatch.setattr(sd, "_http_get_bytes", fake_get_bytes) + + body, reason = sd.fetch_skill_file(WS, "token", "main", "default", "triage", "SKILL.md") + + 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" + + def test_http_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr( + sd, "_http_get_bytes", lambda url, token, timeout=30: (None, "HTTP 404 Not Found") + ) + + body, reason = sd.fetch_skill_file(WS, "token", "main", "default", "triage", "gone.md") + + assert body is None + assert reason == "HTTP 404 Not Found" + + +class TestFetchSkillBundle: + def test_assembles_relpath_to_bytes_map(self, monkeypatch): + contents = {"SKILL.md": b"# skill", "references/a.md": b"aaa"} + monkeypatch.setattr(sd, "list_skill_files", lambda *a, **k: (list(contents), None)) + monkeypatch.setattr( + sd, "fetch_skill_file", lambda ws, tok, c, s, leaf, rel: (contents[rel], None) + ) + + bundle, reason = sd.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert reason is None + assert bundle == contents + + def test_listing_failure_propagates_reason(self, monkeypatch): + monkeypatch.setattr(sd, "list_skill_files", lambda *a, **k: ([], "HTTP 404 Not Found")) + + bundle, reason = sd.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert bundle is None + assert reason == "HTTP 404 Not Found" + + def test_file_failure_aborts_whole_bundle(self, monkeypatch): + monkeypatch.setattr( + sd, "list_skill_files", lambda *a, **k: (["SKILL.md", "broken.md"], None) + ) + monkeypatch.setattr( + sd, + "fetch_skill_file", + lambda ws, tok, c, s, leaf, rel: ( + (b"ok", None) if rel == "SKILL.md" else (None, "HTTP 500 Server Error") + ), + ) + + bundle, reason = sd.fetch_skill_bundle(WS, "token", "main", "default", "triage") + + assert bundle is None + assert reason == "HTTP 500 Server Error" + class TestSkillDirRoots: def test_roots_under_project_dir(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) assert roots == [tmp_path / ".claude/skills", tmp_path / ".agents/skills"] + def test_defaults_to_home_when_omitted(self, tmp_path, monkeypatch): + monkeypatch.setattr(sd.Path, "home", classmethod(lambda cls: tmp_path)) + roots = skill_dir_roots(None) + assert roots == [tmp_path / ".claude/skills", tmp_path / ".agents/skills"] + def test_relative_path_rejected(self): with pytest.raises(ValueError, match="absolute"): skill_dir_roots("relative/dir") @@ -31,9 +246,8 @@ 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)"} - status = _write(roots, "triage", files) + _write(roots, "triage", files) - assert status == "written" for root in roots: assert (root / "triage/SKILL.md").read_bytes() == b"# skill" assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" @@ -43,9 +257,8 @@ def test_existing_skill_prompt_keep(self, tmp_path, monkeypatch): _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) - status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") + _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert status == "kept" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" def test_existing_skill_prompt_overwrite(self, tmp_path, monkeypatch): @@ -53,26 +266,66 @@ def test_existing_skill_prompt_overwrite(self, tmp_path, monkeypatch): _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) - status = _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") + _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert status == "overwritten" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" def test_invalid_leaf_is_skipped(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - status = _write(roots, "Bad_Name", {"SKILL.md": b"x"}) + _write(roots, "Bad_Name", {"SKILL.md": b"x"}) - assert status == "skipped" assert not (roots[0] / "Bad_Name").exists() def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - status = _write( - roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} - ) + _write(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) - assert status == "written" assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists() + + +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) + ) + bundles = { + "pii-handling": {"SKILL.md": b"pii"}, + "triage": {"SKILL.md": b"triage"}, + } + monkeypatch.setattr( + sd, "fetch_skill_bundle", lambda ws, tok, c, s, leaf: (bundles[leaf], None) + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + 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_list_failure_skips_location(self, tmp_path, monkeypatch): + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: ([], "HTTP 404 Not Found")) + called = [] + monkeypatch.setattr( + sd, "fetch_skill_bundle", lambda *a, **k: called.append(1) or (None, None) + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert called == [] + + 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, + "fetch_skill_bundle", + lambda ws, tok, c, s, leaf: ( + ({"SKILL.md": b"ok"}, None) if leaf == "good" else (None, "HTTP 500 Server Error") + ), + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok" + assert not (tmp_path / ".claude/skills/bad").exists() From d6272f9258de10d643c7a1da395b1e2acae41564 Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 19:49:07 +0000 Subject: [PATCH 09/11] skills: move download command out of mcp.py into skills_download.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit configure_skills_download_command now lives in skills_download.py alongside the client, writer, and orchestration. mcp.py keeps only the MCP-connection glue it exposes as setup_mcp_clients and register_schemaless_skills_connection, and no longer imports skills_download — the dependency flows one way (cli -> skills_download -> mcp -> databricks). Co-authored-by: Isaac --- src/ucode/cli.py | 2 +- src/ucode/mcp.py | 26 ++++++++----------------- src/ucode/skills_download.py | 25 +++++++++++++++++++++++- tests/test_mcp.py | 33 ++++++++++---------------------- tests/test_skills_download.py | 36 +++++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 43 deletions(-) diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 48415fa..975eb0b 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -55,11 +55,11 @@ MCP_CLIENTS, SKILLS_MCP_KIND, configure_mcp_command, - configure_skills_download_command, configure_skills_mcp_command, purge_cross_workspace_mcp_residue, revert_mcp_configs, ) +from ucode.skills_download import configure_skills_download_command from ucode.state import ( STATE_PATH, clear_state, diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 0857e1d..5f57860 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -40,7 +40,6 @@ list_vector_search_catalog_schemas, workspace_hostname, ) -from ucode.skills_download import download_skills from ucode.state import load_full_state, load_state, save_state from ucode.ui import ( print_note, @@ -1191,7 +1190,7 @@ def _resolve_location_mcp_servers( return [*working_servers, *_skills_entries(original_servers)] -def _setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: +def setup_mcp_clients(state: dict, section: str) -> tuple[str, str | None, list[str]]: """Validate the workspace, resolve configured MCP clients, and prepare auth. Returns ``(workspace, profile, clients)`` and prints the section header, the @@ -1254,7 +1253,7 @@ def configure_mcp_command(location: str | None = None, services: set[str] | None ) location = next(iter(schemas)) state = load_state() - workspace, profile, clients = _setup_mcp_clients(state, "MCP Servers") + workspace, profile, clients = setup_mcp_clients(state, "MCP Servers") original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: @@ -1425,7 +1424,7 @@ def configure_skills_mcp_command(locations: list[str]) -> int: """Set the skills MCP connection's ``skill_locations`` to exactly ``locations``, replacing any previous set.""" state = load_state() - workspace, _profile, clients = _setup_mcp_clients(state, "Skills MCP") + workspace, _profile, clients = setup_mcp_clients(state, "Skills MCP") _update_skills_mcp(state, workspace, clients, locations) return 0 @@ -1436,19 +1435,10 @@ def _skill_mcp_locations(state: dict) -> list[str]: return list((entry or {}).get("skill_locations") or []) -def configure_skills_download_command(locations: list[str], *, path: str | None) -> int: - """Download every skill in each schema to disk and register the skills connection. - - Delegates the download to ``download_skills`` (files land under ``path`` or, - when ``path`` is None, the user home dir), then registers/keeps the - schema-less MCP connection. ``skill_locations`` is never touched, so a prior - ``--mcp`` set survives a download run. - """ - state = load_state() - workspace, profile, clients = _setup_mcp_clients(state, "Skills") - token = get_databricks_token(workspace, profile) - - download_skills(workspace, token, locations, path) +def register_schemaless_skills_connection(state: dict, workspace: str, clients: list[str]) -> None: + """Register/keep the skills MCP connection without changing its schema set. + Download mode calls this after writing files: it preserves any prior + ``--mcp`` ``skill_locations`` and otherwise registers the bare schema-less + route (utility tools only).""" _update_skills_mcp(state, workspace, clients, _skill_mcp_locations(state)) - return 0 diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 0023922..76a7883 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -8,7 +8,14 @@ from typing import cast from urllib.parse import urlencode -from ucode.databricks import _http_get_bytes, _http_get_json, workspace_hostname +from ucode.databricks import ( + _http_get_bytes, + _http_get_json, + get_databricks_token, + workspace_hostname, +) +from ucode.mcp import register_schemaless_skills_connection, setup_mcp_clients +from ucode.state import load_state from ucode.ui import print_note, print_success, print_warning, prompt_yes_no # `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). @@ -251,3 +258,19 @@ def download_skills(workspace: str, token: str, locations: list[str], path: str print_warning(f"Skipping `{location}.{leaf}`: {reason}.") continue write_skill(roots, leaf, files, location=location) + + +def configure_skills_download_command(locations: list[str], *, path: str | None) -> int: + """Download every skill in each schema to disk and register the skills connection. + + Downloads to ``path`` (or the home dir when None), then registers/keeps the + schema-less MCP connection. ``skill_locations`` is never touched, so a prior + ``--mcp`` set survives a download run.""" + state = load_state() + workspace, profile, clients = setup_mcp_clients(state, "Skills") + token = get_databricks_token(workspace, profile) + + download_skills(workspace, token, locations, path) + + register_schemaless_skills_connection(state, workspace, clients) + return 0 diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 4d7e052..b428cea 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1886,44 +1886,31 @@ def test_empty_when_no_skills_entry(self): assert mcp._skill_mcp_locations(_skills_state()) == [] -class TestConfigureSkillsDownloadCommand: - def _stub_download(self, monkeypatch, state): +class TestRegisterSchemalessSkillsConnection: + def _stub(self, monkeypatch): saved_states: list[dict] = [] - download_calls: list[tuple] = [] - _stub_location_base(monkeypatch, state) monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "available_mcp_clients", lambda: ["claude"]) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - monkeypatch.setattr( - mcp, - "download_skills", - lambda ws, tok, locations, path: download_calls.append((ws, locations, path)), - ) - return saved_states, download_calls + return saved_states - def test_downloads_and_registers_schemaless_connection(self, monkeypatch): - saved_states, download_calls = self._stub_download(monkeypatch, _skills_state()) + def test_registers_bare_route_when_none_exists(self, monkeypatch): + saved_states = self._stub(monkeypatch) + state = _skills_state([]) - assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + mcp.register_schemaless_skills_connection(state, WS, ["claude"]) - assert download_calls == [(WS, ["a.b"], "/tmp/skills")] skills = _find_skills(saved_states[-1]["mcp_servers"]) assert len(skills) == 1 assert skills[0]["skill_locations"] == [] assert skills[0]["url"] == f"{WS}/ai-gateway/skills/" - def test_none_path_threads_through(self, monkeypatch): - _, download_calls = self._stub_download(monkeypatch, _skills_state()) - - assert mcp.configure_skills_download_command(["a.b"], path=None) == 0 - - assert download_calls == [(WS, ["a.b"], None)] - def test_preserves_prior_mcp_location_set(self, monkeypatch): + self._stub(monkeypatch) prior = mcp._resolve_skills_mcp_servers(WS, ["claude"], ["X.x", "Y.y"], []) state = _skills_state(prior) - self._stub_download(monkeypatch, state) - assert mcp.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + mcp.register_schemaless_skills_connection(state, WS, ["claude"]) assert _find_skills(state["mcp_servers"])[0]["skill_locations"] == ["X.x", "Y.y"] diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index cae037b..71c10e9 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -329,3 +329,39 @@ def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / ".claude/skills/bad").exists() + + +class TestConfigureSkillsDownloadCommand: + def _stub(self, monkeypatch): + calls: dict[str, object] = {} + monkeypatch.setattr(sd, "load_state", lambda: {"state": True}) + monkeypatch.setattr( + sd, "setup_mcp_clients", lambda state, section: (WS, "profile", ["claude"]) + ) + monkeypatch.setattr(sd, "get_databricks_token", lambda ws, profile: "token") + monkeypatch.setattr( + sd, + "download_skills", + lambda ws, tok, locations, path: calls.update(download=(ws, tok, locations, path)), + ) + monkeypatch.setattr( + sd, + "register_schemaless_skills_connection", + lambda state, ws, clients: calls.update(register=(ws, clients)), + ) + return calls + + def test_downloads_then_registers_connection(self, monkeypatch): + calls = self._stub(monkeypatch) + + assert sd.configure_skills_download_command(["a.b"], path="/tmp/skills") == 0 + + assert calls["download"] == (WS, "token", ["a.b"], "/tmp/skills") + assert calls["register"] == (WS, ["claude"]) + + def test_none_path_threads_through(self, monkeypatch): + calls = self._stub(monkeypatch) + + assert sd.configure_skills_download_command(["a.b"], path=None) == 0 + + assert calls["download"] == (WS, "token", ["a.b"], None) From 59b506fbfe35cb83fc042b79873f142b24e3268b Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 19:56:43 +0000 Subject: [PATCH 10/11] skills: show a progress bar and k/n summary during download Replace the per-skill [i/total] prefixes with a Rich progress bar that advances as concurrent fetches complete (transient, tty-only so CI logs stay clean), followed by a 'Downloaded k/n skill(s) from ' summary. Adds a reusable progress_bar helper to ui.py. Co-authored-by: Isaac --- src/ucode/skills_download.py | 37 ++++++++++++++++++++++------------- src/ucode/ui.py | 23 ++++++++++++++++++++++ tests/test_skills_download.py | 22 +++++++++++++++++++++ 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 76a7883..9c598d0 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -16,7 +16,7 @@ ) from ucode.mcp import register_schemaless_skills_connection, setup_mcp_clients from ucode.state import load_state -from ucode.ui import print_note, print_success, print_warning, prompt_yes_no +from ucode.ui import print_note, print_success, print_warning, progress_bar, prompt_yes_no # `.claude/skills` (Claude) + `.agents/skills` (the alias other agents read). SKILL_BASE_DIR_NAMES = (".claude/skills", ".agents/skills") @@ -191,27 +191,27 @@ 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) -> None: +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, location: str) -> bool: """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root. Prompts before overwriting an existing skill dir. ``location`` is the source - ``.``, shown in that prompt. + ``.``, shown in that prompt. Returns True if the skill was + written, False if it was skipped or kept. """ if not _is_valid_leaf(leaf): print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") - return + return False already_on_disk = any((root / leaf).exists() for root in roots) if already_on_disk and not prompt_yes_no( f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" ): print_note(f"Kept existing `{leaf}`.") - return + return False for root in roots: _write_bundle(root / leaf, leaf, files) - verb = "Overwrote" if already_on_disk else "Downloaded" - print_success(f"{verb} `{leaf}` ({len(files)} file(s)).") + return True # --- Orchestration --------------------------------------------------------- @@ -220,24 +220,31 @@ def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, locati def _fetch_bundles( workspace: str, token: str, catalog: str, schema: str, leaves: list[str] ) -> dict[str, tuple[dict[str, bytes] | None, str | None]]: - """Fetch every leaf's bundle concurrently, keyed by leaf name.""" + """Fetch every leaf's bundle concurrently, keyed by leaf name. + + Renders a ``k/n`` progress bar that advances as each fetch completes. + """ results: dict[str, tuple[dict[str, bytes] | None, str | None]] = {} - with ThreadPoolExecutor(max_workers=min(_MAX_FETCH_WORKERS, len(leaves))) as pool: + with ( + progress_bar(f"Fetching skills from {catalog}.{schema}", len(leaves)) as advance, + ThreadPoolExecutor(max_workers=min(_MAX_FETCH_WORKERS, len(leaves))) as pool, + ): futures = { pool.submit(fetch_skill_bundle, workspace, token, catalog, schema, leaf): leaf for leaf in leaves } for future in as_completed(futures): results[futures[future]] = future.result() + advance() return results def download_skills(workspace: str, token: str, locations: list[str], path: str | None) -> None: """Download every skill in each ``.`` location to disk. - Bundles are fetched concurrently per schema, then written sequentially (so - overwrite prompts don't interleave). A failure on one skill warns and skips - it without aborting the batch. + 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. """ roots = skill_dir_roots(path) for location in locations: @@ -250,14 +257,16 @@ def download_skills(workspace: str, token: str, locations: list[str], path: str print_note(f"No skills found in `{location}`.") continue - print_note(f"Downloading {len(leaves)} skill(s) from `{location}`...") bundles = _fetch_bundles(workspace, token, catalog, schema, leaves) + written = 0 for leaf in leaves: files, reason = bundles[leaf] if reason or files is None: print_warning(f"Skipping `{location}.{leaf}`: {reason}.") continue - write_skill(roots, leaf, files, location=location) + if write_skill(roots, leaf, files, location=location): + written += 1 + print_success(f"Downloaded {written}/{len(leaves)} skill(s) from `{location}`.") def configure_skills_download_command(locations: list[str], *, path: str | None) -> int: diff --git a/src/ucode/ui.py b/src/ucode/ui.py index c1b6ee3..cec7950 100644 --- a/src/ucode/ui.py +++ b/src/ucode/ui.py @@ -7,12 +7,14 @@ import textwrap import threading import time +from collections.abc import Callable, Iterator from contextlib import contextmanager from datetime import timedelta import questionary from rich.console import Console from rich.panel import Panel +from rich.progress import BarColumn, MofNCompleteColumn, Progress, TextColumn console = Console(highlight=False) err_console = Console(stderr=True, highlight=False) @@ -113,6 +115,27 @@ def spin() -> None: thread.join(timeout=1) +@contextmanager +def progress_bar(description: str, total: int) -> Iterator[Callable[[], None]]: + """Yield an ``advance()`` callback that drives a ``k/n`` progress bar. + + Falls back to no live bar off a tty (e.g. CI), so logs stay single-line. + """ + if total <= 0 or not sys.stdout.isatty(): + yield lambda: None + return + + with Progress( + TextColumn("[dim]{task.description}[/dim]"), + BarColumn(), + MofNCompleteColumn(), + console=console, + transient=True, + ) as progress: + task = progress.add_task(description, total=total) + yield lambda: progress.advance(task) + + def render_box_table( headers: list[str], rows: list[list[str]], diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 71c10e9..ebcc0df 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -330,6 +330,28 @@ def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): assert (tmp_path / ".claude/skills/good/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / ".claude/skills/bad").exists() + def test_prints_downloaded_count_summary(self, tmp_path, monkeypatch, capsys): + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["a", "b", "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)) + + assert "Downloaded 3/3 skill(s) from `main.default`" in capsys.readouterr().out + + 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, + "fetch_skill_bundle", + lambda ws, tok, c, s, leaf: ( + ({"SKILL.md": b"ok"}, None) if leaf == "good" else (None, "HTTP 500 Server Error") + ), + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert "Downloaded 1/2 skill(s) from `main.default`" in capsys.readouterr().out + class TestConfigureSkillsDownloadCommand: def _stub(self, monkeypatch): From bbe41495d781e569507c0199eb85a9bb57e2e04c Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 24 Jul 2026 21:22:19 +0000 Subject: [PATCH 11/11] skills: drop redundant cast and make _fetch_bundles self-contained - Remove the no-op cast(dict, payload) (the isinstance guard already narrows the type) and its now-unused typing.cast import. - Guard _fetch_bundles against empty leaves with an early return, so it no longer relies on the caller to avoid ThreadPoolExecutor(max_workers=0). Co-authored-by: Isaac --- src/ucode/skills_download.py | 7 ++++--- tests/test_skills_download.py | 7 +++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 9c598d0..db43108 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -5,7 +5,6 @@ import re from concurrent.futures import ThreadPoolExecutor, as_completed from pathlib import Path -from typing import cast from urllib.parse import urlencode from ucode.databricks import ( @@ -65,7 +64,7 @@ def list_schema_skills( payload, reason = _http_get_json(f"{base_url}?{urlencode(query)}", token, timeout=30) if payload is None: return [], reason - data = cast(dict, payload) if isinstance(payload, dict) else {} + 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: @@ -99,7 +98,7 @@ def list_skill_files( payload, reason = _http_get_json(url, token, timeout=30) if payload is None: return [], reason - data = cast(dict, payload) if isinstance(payload, dict) else {} + data = payload if isinstance(payload, dict) else {} for entry in data.get("contents") or []: path = entry.get("path") if isinstance(entry, dict) else None if not isinstance(path, str): @@ -224,6 +223,8 @@ def _fetch_bundles( Renders a ``k/n`` progress bar that advances as each fetch completes. """ + if not leaves: + return {} results: dict[str, tuple[dict[str, bytes] | None, str | None]] = {} with ( progress_bar(f"Fetching skills from {catalog}.{schema}", len(leaves)) as advance, diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index ebcc0df..47232d9 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -286,6 +286,13 @@ def test_path_traversal_is_rejected(self, tmp_path): assert not (tmp_path / "escape.md").exists() +class TestFetchBundles: + def test_empty_leaves_returns_empty_without_pool(self): + # min(workers, 0) would raise ValueError in ThreadPoolExecutor; the + # early return keeps _fetch_bundles safe regardless of caller. + assert sd._fetch_bundles(WS, "token", "main", "default", []) == {} + + class TestDownloadSkills: def test_fetches_and_writes_each_leaf(self, tmp_path, monkeypatch): monkeypatch.setattr(