From 61f228d3430b23a0dc3e1f4edc7ae8956d53c3df Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 31 Jul 2026 01:03:08 +0000 Subject: [PATCH 1/2] skills: prompt before fetching so declined skills aren't downloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Download mode fetched every skill's bytes up front, then prompted to overwrite existing dirs at write time — so declining a skill threw away an already-completed download. Move the overwrite prompt and invalid-name check ahead of the fetch: split write_skill into should_download_skill (the disk-only decision, extracting existing_skill_on_disk) and a pure write_skill, and filter each schema's leaves through the decision before _fetch_bundles runs. The per-schema parallel fetch and the sequential location loop are unchanged, so cross-location same-leaf overwrite prompting still works. --- src/ucode/skills_download.py | 57 ++++++++++++++++++--------- tests/test_skills_download.py | 72 ++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 40 deletions(-) diff --git a/src/ucode/skills_download.py b/src/ucode/skills_download.py index 124721e..ac51e59 100644 --- a/src/ucode/skills_download.py +++ b/src/ucode/skills_download.py @@ -197,27 +197,36 @@ def _write_bundle(skill_dir: Path, leaf: str, files: dict[str, bytes]) -> None: destination.write_bytes(content) -def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes], *, location: str) -> bool: - """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root. +def existing_skill_on_disk(roots: list[Path], leaf: str) -> bool: + """Whether ``leaf`` already has a skill directory under any root.""" + return any((root / leaf).exists() for root in roots) - Prompts before overwriting an existing skill dir. ``location`` is the source - ``.``, shown in that prompt. Returns True if the skill was - written, False if it was skipped or kept. + +def should_download_skill(roots: list[Path], leaf: str, *, location: str) -> bool: + """Whether ``leaf`` should be fetched and written into ``roots``. + + Applies the disk-only checks that need no bundle bytes, so a declined or + invalid skill is never downloaded: skips invalid leaf names, and prompts + before overwriting a skill already on disk (``location`` is the source + ``.`` shown in that prompt). """ if not _is_valid_leaf(leaf): print_warning(f"Skipping `{leaf}`: not a valid skill name (lowercase a-z, 0-9, -).") return False - already_on_disk = any((root / leaf).exists() for root in roots) - if already_on_disk and not prompt_yes_no( + if existing_skill_on_disk(roots, leaf) and not prompt_yes_no( f"A skill named `{leaf}` already exists. Overwrite it with `{location}.{leaf}`?" ): print_note(f"Kept existing `{leaf}`.") return False + return True + + +def write_skill(roots: list[Path], leaf: str, files: dict[str, bytes]) -> None: + """Write ``leaf``'s bundle (``{relpath: bytes}``) into every root.""" for root in roots: _write_bundle(root / leaf, leaf, files) - return True # --- Orchestration --------------------------------------------------------- @@ -256,12 +265,21 @@ def download_skills( ) -> None: """Download every skill in each ``.`` location to disk. - Bundles are fetched concurrently (with a progress bar) per schema, then - written sequentially so overwrite prompts don't interleave. A failure on one - skill warns and skips it without aborting the batch. - - When ``skills`` is given, only those leaf names are downloaded; names absent - from a schema warn and are skipped. ``None`` downloads the whole schema. + Locations are processed one at a time, and each runs three stages: + + 1. **List** the schema's skill leaves. When ``skills`` is given, restrict to + those leaf names; names absent from the schema warn and are skipped, and + ``None`` keeps the whole schema. + 2. **Decide** which to download via ``should_download_skill`` (skips invalid + names and prompts before overwriting a skill already on disk), so a + declined skill is never fetched. + 3. **Fetch** the survivors' bundles concurrently (with a progress bar) and + **write** them. + + Finishing one location before starting the next means a skill written for an + earlier location is already on disk when a same-named skill in a later + location reaches its decide stage, so the overwrite prompt still fires. A + failure on one skill warns and skips it without aborting the batch. """ roots = skill_dir_roots(path) roots_display = " and ".join(str(root) for root in roots) @@ -286,15 +304,18 @@ def download_skills( print_note(f"No skills found in `{location}`.") continue - bundles = _fetch_bundles(workspace, token, catalog, schema, leaves) + to_download = [ + leaf for leaf in leaves if should_download_skill(roots, leaf, location=location) + ] + bundles = _fetch_bundles(workspace, token, catalog, schema, to_download) written = 0 - for leaf in leaves: + for leaf in to_download: files, reason = bundles[leaf] if reason or files is None: print_warning(f"Skipping `{location}.{leaf}`: {reason}.") continue - if write_skill(roots, leaf, files, location=location): - written += 1 + write_skill(roots, leaf, files) + written += 1 console.print() print_success( f"Downloaded {written}/{len(leaves)} skill(s) from `{location}` in {roots_display}." diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index bd5e54d..89ca449 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -6,7 +6,12 @@ import pytest import ucode.skills_download as sd -from ucode.skills_download import skill_dir_roots, write_skill +from ucode.skills_download import ( + existing_skill_on_disk, + should_download_skill, + skill_dir_roots, + write_skill, +) WS = "https://example.databricks.com" @@ -237,50 +242,56 @@ def test_missing_directory_rejected(self, tmp_path): skill_dir_roots(str(tmp_path / "nope")) -def _write(roots, leaf, files, *, location="main.default"): - return write_skill(roots, leaf, files, location=location) - - -class TestWriteSkill: - def test_writes_bundle_into_every_root(self, tmp_path): +class TestShouldDownloadSkill: + def test_new_skill_is_downloaded(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - files = {"SKILL.md": b"# skill", "scripts/run.py": b"print(1)"} - _write(roots, "triage", files) - - for root in roots: - assert (root / "triage/SKILL.md").read_bytes() == b"# skill" - assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" + assert should_download_skill(roots, "triage", location="main.default") def test_existing_skill_prompt_keep(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") + write_skill(roots, "triage", {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) - _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-main" + assert not should_download_skill(roots, "triage", location="ml.prod") def test_existing_skill_prompt_overwrite(self, tmp_path, monkeypatch): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"from-main"}, location="main.default") + write_skill(roots, "triage", {"SKILL.md": b"from-main"}) monkeypatch.setattr(sd, "prompt_yes_no", lambda _: True) - _write(roots, "triage", {"SKILL.md": b"from-ml"}, location="ml.prod") - assert (roots[0] / "triage/SKILL.md").read_bytes() == b"from-ml" + assert should_download_skill(roots, "triage", location="ml.prod") def test_invalid_leaf_is_skipped(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "Bad_Name", {"SKILL.md": b"x"}) + assert not should_download_skill(roots, "Bad_Name", location="main.default") - assert not (roots[0] / "Bad_Name").exists() + def test_existing_skill_on_disk_checks_every_root(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + assert not existing_skill_on_disk(roots, "triage") + + (roots[1] / "triage").mkdir(parents=True) + assert existing_skill_on_disk(roots, "triage") + + +class TestWriteSkill: + def test_writes_bundle_into_every_root(self, tmp_path): + roots = skill_dir_roots(str(tmp_path)) + files = {"SKILL.md": b"# skill", "scripts/run.py": b"print(1)"} + + write_skill(roots, "triage", files) + + for root in roots: + assert (root / "triage/SKILL.md").read_bytes() == b"# skill" + assert (root / "triage/scripts/run.py").read_bytes() == b"print(1)" def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - _write(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) + write_skill(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists() @@ -322,6 +333,23 @@ def test_list_failure_skips_location(self, tmp_path, monkeypatch): assert called == [] + def test_declined_skill_is_not_fetched(self, tmp_path, monkeypatch): + roots = skill_dir_roots(str(tmp_path)) + write_skill(roots, "triage", {"SKILL.md": b"kept"}) + monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["triage"], None)) + monkeypatch.setattr(sd, "prompt_yes_no", lambda _: False) + fetched = [] + monkeypatch.setattr( + sd, + "fetch_skill_bundle", + lambda ws, tok, c, s, leaf: fetched.append(leaf) or ({"SKILL.md": b"new"}, None), + ) + + sd.download_skills(WS, "token", ["main.default"], str(tmp_path)) + + assert fetched == [] + assert (roots[0] / "triage/SKILL.md").read_bytes() == b"kept" + def test_bundle_failure_skips_that_skill_only(self, tmp_path, monkeypatch): monkeypatch.setattr(sd, "list_schema_skills", lambda *a, **k: (["good", "bad"], None)) monkeypatch.setattr( From 829d83a203cf5780a5ac2a9014921f74aade9c3e Mon Sep 17 00:00:00 2001 From: Xiang Shen Date: Fri, 31 Jul 2026 01:46:34 +0000 Subject: [PATCH 2/2] skills: clarify download_skills docstring and ruff format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review nit — restructure the download_skills docstring into explicit list/decide/fetch stages. Run ruff format to wrap an over-length line in tests (fixes the test_ruff_format CI check). --- tests/test_skills_download.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_skills_download.py b/tests/test_skills_download.py index 89ca449..8c0a039 100644 --- a/tests/test_skills_download.py +++ b/tests/test_skills_download.py @@ -291,7 +291,9 @@ def test_writes_bundle_into_every_root(self, tmp_path): def test_path_traversal_is_rejected(self, tmp_path): roots = skill_dir_roots(str(tmp_path)) - write_skill(roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"}) + write_skill( + roots, "triage", {"SKILL.md": b"ok", "../escape.md": b"nope", "/abs.md": b"nope"} + ) assert (roots[0] / "triage/SKILL.md").read_bytes() == b"ok" assert not (tmp_path / "escape.md").exists()