Extract shared skill-download core and add selective download - #578
Merged
Merged
Conversation
xsh310
added this pull request to stack #576
September 11, 2026 23:15
xsh310
commented
Sep 11, 2026
xsh310
marked this pull request as ready for review
September 12, 2026 07:13
xsh310
force-pushed
the
skills-picker-download-core
branch
2 times, most recently
from
September 14, 2026 03:08
274f6b0 to
5a90185
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 14, 2026 20:09
5a90185 to
a6fbd13
Compare
sunishsheth2009
approved these changes
Sep 14, 2026
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 14, 2026 22:32
a6fbd13 to
eda0255
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 14, 2026 23:20
eda0255 to
0571612
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 00:53
0571612 to
1d7543c
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 01:17
1d7543c to
69292f2
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 01:41
69292f2 to
202fd05
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 03:30
202fd05 to
3f6aa4e
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 03:40
3f6aa4e to
21a7d8d
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 04:42
21a7d8d to
aaa780b
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 14:43
aaa780b to
155e401
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 18:17
155e401 to
def026f
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 19:56
def026f to
e1a77be
Compare
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 15, 2026 21:19
e1a77be to
0aa946e
Compare
Refactor skills_download.py so the download mechanics (collision dedup, overwrite prompt, parallel fetch, write) become one reusable core, ahead of the interactive picker (PR D) which selects individual skills across schemas. - SkillRef now carries catalog/schema and exposes fqn, so a ref is fully self-contained; _skill_ref parses all three from the API name. - Extract _download_refs(refs, roots) -> (written, total), the shared core. should_download_skill and _reject_bundle_name_collisions read the source off the ref (ref.fqn), yielding identical messages, so the core is location-agnostic and handles a cross-schema set. - _fetch_bundles keys results by fqn (a securable is unique only within its schema) and takes its progress-bar label from the caller. - Rename download_skills to download_skills_from_schema_locations; it calls _download_refs per location and keeps the per-location summary. - download_managed_skills_on_launch (the other _fetch_bundles caller) moves to the new keying; behavior unchanged. - Add get_skill(fqn) (GetSkill, NOT_FOUND to None) and download_selected_skills(fqns, path); both unwired this PR. No behavior change on any existing path. New unit tests cover _download_refs (including cross-schema dedup), get_skill, and download_selected_skills. Co-authored-by: Isaac <no-reply@databricks.com>
bundle_name is server-provided and names the on-disk skill directory (a write sink, and the future ug skill remove delete sink), so guard it client-side against separators, .. traversal, and absolute paths rather than relying only on FinalizeSkill's server-side naming rules. Co-authored-by: Isaac <no-reply@databricks.com>
xsh310
force-pushed
the
skills-picker-download-core
branch
from
September 16, 2026 00:51
0aa946e to
30cb6b5
Compare
xsh310
added a commit
that referenced
this pull request
Sep 16, 2026
## What PR D (final) of the `ug skill add` interactive-picker stack: it wires the feature. Running `ug skill add` with no `--mcp`, `--location`, or `--skills` on an interactive terminal now opens a picker of the workspace's skills to download, mirroring `ug mcp add`'s picker exactly. Design doc: `ug_skill_add_download_interactive_picker_design.md` (PR table, row D). Builds on PR A (picker primitives), PR B (`walk_catalog_schemas`), and PR C (`download_selected_skills`). ## Changes All in `skills_download.py` (discovery + picker glue) and `cli.py` (routing): - **`list_all_skills`** — workspace-wide skill walk via `walk_catalog_schemas` (catalogs → schemas → skills in parallel under a wall-clock deadline), flattening and streaming each schema's finalized skills by FQN. - **Picker glue** — `_skill_download_choice` (value = FQN, title flags on-disk bundles, which stay selectable so a re-download can update them), `_skills_download_background_loader` (runs `list_all_skills` on the token the command already fetched and streams discovered skills into the open picker as choices), and `prompt_for_skill_download_choices` (`scrolling_checkbox` with `loading_noun="skills"`). - **`configure_skills_download_picker_command`** — opens the picker immediately, streams skills in behind it, then `download_selected_skills` + `register_schemaless_skills_connection` on save. Ctrl-C downloads nothing and leaves the connection untouched. - **`skills_add`** — opens the picker when `--mcp`, `--location`, and `--skills` are all omitted on an interactive terminal (via `_stdin_is_interactive`). The typed `--location` / `--skills` paths are unchanged, and a non-interactive run (piped/CI) still errors with `--location is required`. - **README** — documents the no-argument picker. ## Edge cases (handled by the reused primitives) - **Empty / streaming:** the picker opens on an empty list and fills in via the background loader. - **Deadline / partial results:** `walk_catalog_schemas` returns what it found once the budget is exceeded. - **Permission / discovery failure:** discovery runs on the picker's daemon thread, so a 403 or transient failure just stops the stream and the picker stays open. - **Ctrl-C:** exit 0, nothing downloaded, no registration. ## Testing `uv run ruff check .`, `ruff format --check src tests`, `ty check src`, and `uv run pytest` all pass (the 2 pre-existing e2e failures in `test_e2e_user_agent` and `test_claude_smart_routing_v2` are unrelated and also fail on the base branch). New unit tests cover `list_all_skills` (flatten, stream, dedupe by FQN, progress, deadline, walk-failure reason), the picker glue (choice on-disk flag, background loader streaming, prompt selection and cancel), the picker command (download+register, Ctrl-C no-op), and CLI routing (interactive opens the picker, `--skills` stays on the typed path, non-interactive errors). Stacked on #578. https://github.com/user-attachments/assets/e5140d3c-cdf0-4746-b7cf-ffe93826cfe3 This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
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
2 participants
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
PR C of the
ug skill addinteractive-picker stack. It reshapesskills_download.pyso the download mechanics become one reusable core, and adds the (still unwired) functions the picker will call in PR D. No behavior changes on any existing path.Design doc:
ug_skill_add_download_interactive_picker_design.md(PR table, row C).Changes
SkillRefcarries its full location. Newcatalog/schemafields plus anfqnproperty, parsed once from the APInamein_skill_ref, so a ref is self-contained and can be downloaded on its own._download_refs(refs, roots) -> (written, total), the shared core: collision dedup, overwrite prompt, parallel fetch, write.should_download_skilland_reject_bundle_name_collisionsnow read the source off the ref (ref.fqn) instead of an explicitlocationarg, which produces byte-identical messages and lets the core serve a flat, cross-schema set._fetch_bundleskeys results by FQN (a securable name is unique only within its schema) and takes its progress-bar label from the caller.download_skillstodownload_skills_from_schema_locations, now calling_download_refsper location and keeping the exact per-locationDownloaded k/n from <location>summary.get_skill(fqn)(GetSkill at/2.1/unity-catalog/skills/<fqn>, reusing_skill_ref; NOT_FOUND toNone) anddownload_selected_skills(fqns, path)(one_download_refspass over the flat selection). Both are dead code until PR D wires the picker.Deviations from the design doc
download_managed_skills_on_launchis a second caller of_fetch_bundlesthe doc did not mention; it is updated to the new keying (behavior-preserving).should_download_skill/_reject_bundle_name_collisionsas reused unchanged, but making the core location-agnostic means they read the source off the ref. The emitted messages are identical (<catalog>.<schema>.<securable>equalsref.fqn).Testing
uv run ruff check .,ruff format --check src tests, anduv run pytestall pass (the 2 pre-existing e2e failures intest_e2e_user_agentandtest_claude_smart_routing_v2are unrelated and also fail on the base branch). New unit tests cover_download_refs(including cross-schema dedup),get_skill(NOT_FOUND toNone, unfinalized toNone), anddownload_selected_skills.Stacked on #574 (
skills-picker-walk).This pull request and its description were written by Isaac.