skills: add download mode to ucode configure skills (client, writer, CLI wiring)#237
Merged
Merged
Conversation
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
force-pushed
the
skills-download-client
branch
from
July 24, 2026 17:23
952c8a3 to
df68c78
Compare
ucode configure skills (client, writer, CLI wiring)
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
commented
Jul 24, 2026
xsh310
marked this pull request as ready for review
July 24, 2026 20:57
…h bytes) 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
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 `<root>/<leaf>/` 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
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
`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
- 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 <catalog>.<schema> string). Co-authored-by: Isaac
`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
Flip --mcp to optional and add a download-only --path. Without --mcp, configure skills downloads every skill in each --location schema into <path>/.claude/skills + <path>/.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
- 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
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
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 <location>' summary. Adds a reusable progress_bar helper to ui.py. Co-authored-by: Isaac
- 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
xsh310
force-pushed
the
skills-download-client
branch
from
July 24, 2026 22:43
63b9516 to
bbe4149
Compare
AarushiShah-db
approved these changes
Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes download mode for
ucode configure skills, end to end. Downloads every Unity Catalog skill in a schema to local disk and registers a schema-less skills MCP connection. This PR covers the whole path: the read-only client, the on-disk writer, the CLI wiring, and docs.Download client + writer + orchestration (
src/ucode/skills_download.py, new module):list_schema_skills(...)returns finalized skill leaf names in a schema via the UC skills API (GET /api/2.1/unity-catalog/skills?parent=schemas/<cat>.<sch>, paginated). Usesbundle_nameas the leaf and the presence offinalize_timeas the readiness signal.list_skill_files(...)returns a bundle's files as paths relative to the skill directory, by recursively walking its UC Volume directory (GET /api/2.0/fs/directories/Volumes/<cat>/<sch>/<leaf>, paginated).fetch_skill_file(...)returns one file's raw bytes;fetch_skill_bundle(...)composes list + fetch into{relpath: bytes}, all or nothing (any failure returns a reason and no bundle, so a partial skill is never written).skill_dir_roots(path)returns the.claude/skills+.agents/skillsroots.pathis optional: given, it must be an existing absolute directory; omitted, roots default to the home directory (user scope).write_skill(...)writes a skill's{relpath: bytes}bundle into every root, one flat<root>/<leaf>/dir per skill. It prompts before overwriting any pre-existing skill dir (ucode does not track which schema wrote a dir, so it always asks). Rejects invalid leaf names and../absolute paths in the bundle.download_skills(...)fetches a schema's bundles concurrently (with ak/nprogress bar), then writes them sequentially so overwrite prompts do not interleave. A per-skill failure warns and skips without aborting the batch, and it prints aDownloaded k/n skill(s)summary.configure_skills_download_command(locations, *, path)is the command entry point: resolves clients + token, callsdownload_skills, then registers or keeps the schema-less MCP connection.Shared transport (
src/ucode/databricks.py):_http_get_bytes, a raw-bytes GET helper next to_http_get_json, since bundles can contain binary files.MCP glue (
src/ucode/mcp.py):setup_mcp_clients(promoted from private) andregister_schemaless_skills_connection, which preserves any prior--mcpskill_locationsand otherwise registers the bare schema-less route.mcp.pyholds no download logic; the dependency flows one way (clitoskills_downloadtomcptodatabricks).CLI (
src/ucode/cli.py):--mcpis optional and--pathis download-only. Download is the default mode;--mcpopts into the connection-only mode;--pathwith--mcpis rejected.skill_locationsis never touched by download, so a prior--mcpscope survives a download run.Docs (
README.md,AGENTS.md): a Skills subsection and Other Commands rows covering both modes, the optional--pathwith its home-dir default, the flat layout, the collision prompt, and the schema-less connection; module-boundary note forskills_download.py.Test plan
Unit tests (no network, mocked HTTP / patched
prompt_yes_no):tests/test_skills_download.py: the client (list_schema_skillsfinalized-only filter + pagination,list_skill_filesrecursive walk + pagination,fetch_skill_file,fetch_skill_bundleall-or-nothing); the writer (skill_dir_rootsproject + home-default + reject relative/missing,write_skillinto both roots, nested files, collision prompt keep/overwrite, invalid-leaf skip,../absolute path rejection); the orchestrator (download_skillsfetch-and-write, list-failure skip, per-skill bundle-failure skip,k/nsummary counts only written skills); and the command wiring (configure_skills_download_commanddownloads then registers, threads aNonepath).tests/test_mcp.py:_skill_mcp_locations,register_schemaless_skills_connection(registers the bare route, preserves a prior--mcpset), plus revert/purge lifecycle coverage for the skills entry.tests/test_cli.py: two-mode dispatch (default downloads with and without--path,--mcpsets the connection,--path+--mcprejected, malformed--location).Full suite: 952 passing, 39 skipped, 1 pre-existing unrelated
test_e2e_user_agentfailure (spawns a liveclaudesubprocess; also fails on the base commit).ruff check,ruff format --check, andty check src/all clean (these are the CI gates intest_lint.py).Live end-to-end against a staging workspace (
eng-ml-inference.staging, user OAuth token, locationxsh.uc_skill_test). Drove the realconfigure_skills_download_commandwith only the agent-config writes and state persistence patched out, so the network fetch, the write, and the connection registration all ran for real. Three scenarios on the latest commit:--path <dir>, fresh):pii-handlingdownloaded with its full nested bundle (SKILL.md+ 5references/*.md+ 2templates/*) into both.claude/skills/and.agents/skills/(16 files total);SKILL.mdhad real frontmatter, not a stub. The progress bar advanced0/3to3/3during the concurrent fetch and cleared. Summary:Downloaded 1/3 skill(s) from xsh.uc_skill_test. Schema-less connection registered (URL.../ai-gateway/skills/,skill_locations: [], clients[claude, codex]).A skill named 'pii-handling' already exists. Overwrite it with 'xsh.uc_skill_test.pii-handling'?before writing, confirming the collision path.--pathomitted,Path.home()pointed at a temp dir): the same 16 files landed under the home-dir roots, confirming the optional---pathdefault.In all three, the two other listed leaves (
xsh-format,dataset-boundary) have no Volume in staging and returned a realHTTP 404 Volume ... does not exist; each was skipped with a warning and the batch continued (exit 0). Scratch dirs were removed afterward and real ucode state was never written.The earlier client-only e2e also resolved a real design question: ucode's client-side OAuth token reads all three endpoints directly, so no
GenerateTemporaryStorageCredentialsvending is needed. It corrected two assumptions unit tests alone would miss: the deployed endpoint is/api/2.1/unity-catalog/skills(not themanaged-catalog-aigovpath), and readiness isfinalize_time(there is nostatusfield).Notes