skills: prompt before fetching so declined skills aren't downloaded - #253
Open
xsh310 wants to merge 2 commits into
Open
skills: prompt before fetching so declined skills aren't downloaded#253xsh310 wants to merge 2 commits into
xsh310 wants to merge 2 commits into
Conversation
xsh310
commented
Jul 31, 2026
xsh310
marked this pull request as ready for review
July 31, 2026 01:49
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.
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).
xsh310
force-pushed
the
skills-dedup-before-fetch-main
branch
from
July 31, 2026 22:23
03c30d7 to
829d83a
Compare
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
1 participant
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.
Problem
In
ucode configure skillsdownload mode, the overwrite dedup prompt runs after the bytes are already downloaded.download_skillsfetches every skill's full bundle up front via_fetch_bundles, then prompts per-skill at write time insidewrite_skill. Declining a skill (or hitting an invalid leaf name) throws away a download that already completed — wasted work that scales with how much the user already has on disk (the common re-download case).Change
Move the disk-only checks (overwrite prompt + invalid-name skip) ahead of the fetch:
existing_skill_on_disk(roots, leaf)— a small reusable existence check.write_skillintoshould_download_skill(roots, leaf, *, location)(the decision — invalid-name skip + overwrite prompt, needs no bytes) and a purewrite_skill(roots, leaf, files)writer.download_skills, filter each schema's leaves throughshould_download_skillfirst, then fetch only the survivors.The per-schema parallel fetch (
_fetch_bundlesThreadPoolExecutor) and the sequentialfor location in locationsloop are unchanged. Because locations are still processed one at a time, a same-named skill from a later location still sees the earlier one on disk and prompts — cross-location overwrite behavior is preserved.Tests
test_declined_skill_is_not_fetchedassertsfetch_skill_bundleis never called for a declined skill and the existing copy is untouched (the point of the change).TestShouldDownloadSkill;TestWriteSkillnow covers the pure writer; added a directexisting_skill_on_disktest.uv run pytest tests/test_skills_download.py→ 33 passed.uv run ruff check→ clean.This pull request and its description were written by Isaac.