fix(AI-3750): stop duplicate _ui_dist wheel entry (hatchling >= 1.30) - #623
Conversation
…builds hatchling's force-include for _ui_dist/ relies on .gitignore-based exclusion to avoid double-adding the path, but that exclusion needs a .git directory to run `git check-ignore` against. A git+ install that hands hatchling a plain exported tree (no .git) skips it, so _ui_dist/index.html gets added twice and the build aborts. Add an explicit wheel-target exclude so it works regardless of VCS state, and document the undocumented Python >=3.12 floor in the README install instructions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@claude review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a wheel-build failure that occurs when building from a VCS-less exported source tree (no .git directory), where src/keboola_agent_cli/_ui_dist/ could be collected twice (default package collection + force-include) and abort the build with a duplicate-archive-path error. It also adds a regression test to exercise that no-.git build path and documents the project’s Python version floor in the install instructions.
Changes:
- Prevent duplicate
_ui_dist/inclusion by explicitly excludingsrc/keboola_agent_cli/_ui_distfrom default wheel collection while keepingforce-includeas the single inclusion path. - Add an end-to-end test that builds a wheel in a
.git-less temporary project and asserts_ui_dist/index.htmlappears exactly once. - Document the Python >= 3.12 requirement (and
UV_PYTHON=3.12workaround) in the README install section.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pyproject.toml |
Adds an explicit wheel exclude to avoid duplicate _ui_dist/ entries in VCS-less builds while retaining force-include. |
tests/test_build_hook.py |
Adds an end-to-end regression test that builds a wheel without a .git directory and checks _ui_dist isn’t duplicated. |
README.md |
Documents Python >= 3.12 install requirement and a uv interpreter selection workaround. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Closing -- scope was wrong for this issue; redoing the actual fix (ai-kit docs cross-references) in keboola/ai-kit instead. |
Miro flagged that AI-3750 references in pyproject.toml/test docstrings aren't publicly accessible; the technical description stands on its own without them.
MiroCillik
left a comment
There was a problem hiding this comment.
Reviewed at 557ad04. The fix itself is sound and load-bearing — I reproduced the original failure by stripping the new exclude line and building the same fixture tree (ValueError: A second file is being added to the wheel archive at the same path: keboola_agent_cli/_ui_dist/index.html), and confirmed against hatchling 1.32 source that recurse_forced_files() applies no include/exclude filtering, so the new exclude cannot suppress the force-include. Also confirmed packages being set short-circuits default_file_selection_options, so exclude_spec's eager self.default_exclude() call has no side effect. The new test is a genuine regression test.
Four findings below — one is a docs bug that makes the documented workaround inert, two are about the root cause being misattributed (which also makes the regression test able to pass vacuously), one is test hygiene. Nothing blocking the one-line fix.
Thanks for dropping the AI-3750 refs in 557ad04 — that was on my list and it's already handled.
…d regression test Follow-up on review of #623. Four issues, none in the one-line `exclude` fix itself (which is load-bearing and verified). - README: `UV_PYTHON=3.12 curl ... | sh` assigns the var to `curl`, not `sh`, so neither the script nor the `uv` it invokes ever saw it -- the documented workaround for `does not satisfy Python>=3.12` was inert. Show the `| UV_PYTHON=3.12 sh` and `export` forms instead. - pyproject/test docstrings: the root cause was misattributed. Hatchling never runs `git check-ignore`; it parses the `.gitignore` *file* found by `locate_file(root, ".gitignore", boundary=".git")`, where `.git` is the boundary that STOPS the upward search rather than a prerequisite. A `git+` install ships a tracked `.gitignore` and excludes fine; the shape that actually failed is the sdist, whose `include` list omits it. - The regression test asserted "no `.git`", which guards the wrong invariant: with neither `.git` nor `.gitignore` in the fixture, hatchling searched every ancestor of `tmp_path`, so a `TMPDIR` under any checkout let an ancestor `.gitignore` supply the exclusion and the test passed with the fix reverted. Create an empty `.git` (the boundary) and assert no local `.gitignore`. Verified: reverting `exclude` now fails both with a normal TMPDIR and with TMPDIR beneath an ancestor `.gitignore`. - The module inherited `KBAGENT_SKIP_UI_BUILD` (exported in parts of CI), which makes `_bundle_ui` ship an empty `_ui_dist/`; 5 tests then failed on a green build with messages pointing elsewhere. Clear it in an autouse fixture. Also raise the wheel-build timeout to 300s for cold-cache Windows runners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MiroCillik
left a comment
There was a problem hiding this comment.
Second pass, at 354ee50. My four earlier findings are addressed (replies in each thread). Re-verified from scratch: removing the exclude line still reproduces ValueError: A second file is being added to the wheel archive at the same path, hatchling's recurse_forced_files is not subject to exclude, the sibling py.typed force-include does not collide (set_build_data reserves file-shaped force-include paths by exact match — which is exactly why the directory-shaped _ui_dist entry was the one that broke, so the root cause holds), and the pattern also covers nested SPA output (_ui_dist/assets/deep/c.css). 21 tests pass, ruff clean.
No correctness bug in the fix. Everything below is low severity, and three of them are against my own follow-up commit.
Two that can't be anchored inline (files/lines outside this diff):
-
scripts/hatch_build.py:18— step 3 of the module docstring now states the opposite of reality: it says the dist is copied into_ui_dist/"so hatchling's normal package collection picks it up (the dir is in.gitignore…)". After this PR normal collection explicitly skips_ui_distandforce-includeis the only way in. A maintainer trusting that sentence could drop the force-include entry and ship a UI-less wheel — only the Windowscheck_wheel_ui --expect-uistep would catch it. Worth updating in this PR since this PR is what made it false. -
pyproject.toml:92(sdistexclude) — the sdist target has no symmetric guard, and it does leak. I reproduced it:includehassrc/, so with_ui_dist/already populated on disk (an earlieruv pip install -e .) and no reachable.gitignore,uv build --sdistshipskeboola_cli-0.86.0/src/keboola_agent_cli/_ui_dist/index.html— a stale prebuilt SPA baked into the source distribution. Worth noting the hook is wheel-scoped, so an sdist build never creates_ui_distitself; a clean tree is unaffected (verified both ways). Not a crash, but adding"src/keboola_agent_cli/_ui_dist"to the sdistexcludemakes both targets VCS-state-independent for exactly the reason this PR gives for the wheel.
Process note: this lands without a version bump or changelog.py entry, so install.sh's preferred prebuilt-wheel path keeps serving v0.86.0 — whose sdist→wheel build is still broken. Only the git+ fallback benefits until the next release. Fine if a release is imminent; worth a deliberate decision if not.
|
@Matovidlo quick check before we act on this: you commented "Closing — scope was wrong for this issue" on 2026-08-20, but the PR stayed open and @MiroCillik has since done two thorough review passes (reproducing the bug and verifying the fix against hatchling internals, all findings addressed). The duplicate Are you OK with merging this PR as-is (decoupled from AI-3750)? We would like to include it in the upcoming 0.89.0 release. If you'd rather close it, say so and we'll take the |
padak
left a comment
There was a problem hiding this comment.
Second-opinion review (OpenAI Codex, gpt-5.6-sol, high reasoning) — verdict: MERGE WITH NITS.
No blocking correctness or security issues found. The hatchling fix is sound: normal package selection honors exclude, while force-include is processed separately and still adds _ui_dist exactly once.
Validation performed by the reviewer:
- Confirmed the pre-fix configuration reproduces the duplicate-entry exception.
- Confirmed the patched wheel contains one
_ui_dist/index.htmland onepy.typed. - Confirmed wheel-from-sdist, editable, and empty-UI builds succeed; the wheel-only exclusion does not alter sdist contents.
_ui_distabsence remains handled by the build hook creating an empty directory.
Nits (both overlap with @MiroCillik's open threads):
- LOW —
README.md:17:UV_PYTHON=3.12cannot solve an offline/download-disabled installation unless Python 3.12 is already installed or cached. Clarify: install Python 3.12 first, then useUV_PYTHON=3.12if uv does not select it automatically. - LOW —
tests/test_build_hook.py:329:test_wheel_builds_without_git_directoryactually creates an empty.gitdirectory to stop hatchling's upward.gitignoresearch. Rename to something liketest_wheel_builds_without_reachable_gitignore; hatchling does not invokegit check-ignore.
padak
left a comment
There was a problem hiding this comment.
Independent verification pass (fresh worktree, hatchling 1.32.0 source + empirical builds — no reliance on prior review threads). Verdict: TAKE WITH FIXES. The one-line exclude is correct, minimal and zero-regression; reproduced the failure on unfixed v0.89.0 main and confirmed the fix end-to-end, including a merged-tree build (git merge-tree is clean, 29 commits behind but no conflict, no version regression — the branch never touches the version line). Test is a genuine guard: fails pre-fix, passes post-fix, and the .git boundary fixture is load-bearing (removing it makes the test pass vacuously via an ancestor .gitignore, exactly as @MiroCillik found).
Two findings that are new to this thread:
-
The corrected root-cause comment is wrong about the sdist.
pyproject.toml(and two docstrings intests/test_build_hook.py) now claim the failing shape is "notably an sdist build, because[tool.hatch.build.targets.sdist].includedoes not ship.gitignore". Hatchling'sSdistBuilder.get_default_build_dataforce-includes the located VCS exclusion files regardless of theincludelist — verified end-to-end on unfixed main:uv build --sdistputs.gitignorein the tarball, and sdist→wheel builds fine. The accurate framing: any source tree where no.gitignoreis reachable — an export/vendored copy with dotfiles stripped, or a.gitboundary with.gitignoreremoved. This is worth fixing before merge; it replaced one wrong explanation with another, aimed at exactly the maintainer who next edits the sdistincludelist. -
No shipped install path currently hits the bug (prebuilt wheel ✓,
install.shgit+ fallback ✓ — uv clones tracked files incl..gitignore, release workflow ✓, sdist→wheel ✓ per above). So this is a latent-robustness fix, not a user-facing incident — which argues for correcting the justification, not for dropping the fix.
Also worth doing before a squash merge: the PR title and body still carry the refuted explanation ("VCS-less source builds", "uv tool install git+… on a source tree with no .git", "git check-ignore") and would become the permanent commit message on main; and the open nit about renaming test_wheel_builds_without_git_directory → test_wheel_builds_without_reachable_gitignore guards a proven false-negative trap, so it is worth taking.
@Matovidlo — the fix stands on its own merits for this repo regardless of the AI-3750 scope moving to ai-kit; happy to carry the comment/title corrections into a follow-up commit on this branch if you prefer, or take the whole thing forward in a fresh PR with you as co-author. Just say which.
…e test name Second review round on #623. All five findings were reproduced before fixing. - scripts/hatch_build.py: step 3 of the module docstring said the dist is copied into `_ui_dist/` "so hatchling's normal package collection picks it up". This PR makes that false -- collection now explicitly skips the dir and `force-include` is the only path in. Someone trusting the old wording could drop either half and ship a UI-less wheel or reintroduce the duplicate; only the Windows `check_wheel_ui --expect-ui` step catches the former. - pyproject.toml: give the sdist the symmetric guard. Reproduced the leak: `include` has `src/`, so with `_ui_dist/` already on disk (an earlier editable install) and no reachable `.gitignore`, `uv build --sdist` ships src/keboola_agent_cli/_ui_dist/index.html -- generated assets as source. A clean tree was never affected: the hook is wheel-scoped, so an sdist build never creates the dir. Downstream impact is nil either way because `_bundle_ui` rmtree's `_ui_dist` before every build, so a stale copy cannot reach a wheel; this is sdist hygiene, not a correctness fix. - tests: `test_wheel_builds_without_git_directory` now deliberately CREATES `.git` (the boundary that stops hatchling's upward `.gitignore` search), so the name said the opposite of the body and invited someone to "fix" the body into a false negative. Renamed to match what it covers. - tests: the autouse fixture's docstring claimed KBAGENT_SKIP_UI_BUILD is "exported by parts of CI". It is not -- ci.yml scopes it to one step's `env:`, the only occurrence in .github/ or the Makefile. The fixture still earns its place (a developer's exported shell flag breaks 5 tests here, measured); only the rationale was wrong. - README: the workaround hard-pinned `UV_PYTHON=3.12` while requires-python is >=3.12, so on a 3.13-only machine with downloads disabled -- exactly the stated precondition -- it failed where no pin would have worked. Verified end to end: sdist no longer carries `_ui_dist`; sdist -> wheel (the originally broken shape, no `.gitignore` in the tarball) builds with exactly one `_ui_dist/index.html` carrying fresh content; reverting the wheel `exclude` still fails the regression test both with a normal TMPDIR and with TMPDIR beneath an ancestor `.gitignore`. Full suite 5692 passed, 172 skipped; every `make check` gate passes except the pre-existing `changelog-check` staleness, which fails identically on main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both body-only findings from the last round are fixed in
Verified end to end: sdist no longer carries Two things for a human to decide — both pre-existing, neither caused by these commits:
That also corrects my process note from the last round: I said |
@padak's second review was right that the sdist justification is false -- hatchling force-includes the located VCS exclusion file, so `.gitignore` IS in the tarball (verified: keboola_cli-0.86.0/.gitignore). But his replacement framing, and the original `.git`/`git check-ignore` one, and my `no reachable .gitignore` one are all wrong too. Bisected instead of reasoned: clean clone of main, `.git` AND `.gitignore` present, prebuilt SPA on disk, wheel `exclude` removed: hatchling 1.27.0 builds OK hatchling 1.28.0 builds OK hatchling 1.29.0 builds OK hatchling 1.30.0 FAILS (duplicate) hatchling 1.31.0 FAILS hatchling 1.32.0 FAILS So VCS state is not the trigger: a directory-shaped `force-include` collides with `packages` collection whenever `_ui_dist/` exists at collection time, from hatchling 1.30.0 on, and `requires = ["hatchling"]` is unpinned. On 1.27.0 deleting `.gitignore` outright still builds fine, which rules the gitignore mechanism out as the explanation entirely. Corrected in all four places that carried a mechanism claim: the wheel `exclude` comment, the sdist `exclude` comment, the test module docstring and the class docstring. The comments now say what was measured and warn against relying on hatchling's gitignore exclusion, which is version- and layout-dependent (an ancestor `.gitignore` suppresses the duplicate in a minimal fixture; this repo's own root `.gitignore` does not). `scripts/hatch_build.py` needed no change -- its wording was already version-neutral. Renamed the test once more: `test_wheel_build_does_not_duplicate_ui_dist`, which states the invariant rather than a mechanism that turned out to be the wrong one. The `.git` boundary in the fixture stays and is still load-bearing for hermeticity (re-verified: removing it lets an ancestor `.gitignore` pass the test with the fix reverted). Also verified the fix is version-independent: exactly one `_ui_dist/index.html` on hatchling 1.27, 1.29, 1.30 and 1.32. Full suite 5692 passed, 172 skipped; ruff, typecheck, loc/version/ command-sync gates green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@padak — your finding 1 is fixed in You were right about the mechanism you tested. Hatchling force-includes the located VCS exclusion file, so But
And on 1.27.0, deleting That flips your finding 2. "No shipped install path currently hits the bug" doesn't hold: a clean clone of Fixed in
Your review-1 nits were already fixed in Two things I did not touch, both pre-existing: the branch is behind |
|
@MiroCillik — the bisect settles it, and it corrects my finding 2 as well: I claimed "no shipped install path currently hits the bug" after testing the shipped paths against the gitignore-reachability hypothesis — which your version table shows was never the operative variable. With hatchling unpinned and the collision keyed on I re-checked my review-1 asks against One suggestion for a follow-up (not this PR): From my side this PR is ready. @Matovidlo — with the root cause now bisected, all review threads addressed, and the title/body rewritten, the only thing missing is your word per your "Closing — scope was wrong" comment: OK to merge as-is (decoupled from AI-3750)? |
padak
left a comment
There was a problem hiding this comment.
All CI green (check, test 3.12/3.13, Windows wheel build). Clean test-merge against main -- no conflicts. No version bump and no changelog.py entry, matching the release process: the fix will be batched into the next release PR. All five open review threads carry a verified fix commit and have been resolved.
The root cause holds up on re-read: hatchling has two independent paths that can add a file to the wheel (packages collection and force-include), and from 1.30.0 the .gitignore-based exclusion no longer suppresses the first one. Since 'requires = ["hatchling"]' is unpinned, this is not a latent edge case -- it hits the ordinary release path. The unconditional 'exclude' glob is the right shape because recurse_forced_files() applies no include/exclude filtering, so force-include stays intact.
* chore(release): 0.90.0 Bumps pyproject.toml to 0.90.0 and adds the changelog entry covering every PR merged since v0.89.0 (#658, #662, #661, #663, #665, #666, #664, #668, #667, #623), resolves the vNEXT placeholders those PRs left behind, and adds the curated What's new reel for the release. * docs(web-server): keep the What's-new anchor stable across releases The '### What's-new popup *(since vNEXT)*' heading put the version gate in the heading itself, so resolving the placeholder to 0.90.0 changed the generated slug to 'whats-new-popup-since-0900' and broke the in-page link at line 138 -- and would have broken it again on every future release. Moved the '(since 0.90.0)' tag to the first body line: the anchor is now the stable 'whats-new-popup', the gate stays visible, and check_version_gates.py still sees it (it scans the whole file, not just headings).
Summary
uv build --wheelwheneversrc/keboola_agent_cli/_ui_dist/is populated at collection time.force-includecollides withpackages-based file selection from hatchling 1.30.0 onwards.requires = ["hatchling"]is unpinned, so every build takes the latest. This is not tied to VCS state: it reproduces on a clean checkout ofmainwith both.gitand.gitignorepresent.exclude = ["src/keboola_agent_cli/_ui_dist"]under[tool.hatch.build.targets.wheel]— a plain glob evaluated unconditionally, soforce-includeis the only path that adds it. Verified version-independent.tests/test_build_hook.py::TestForceIncludeNoDuplicate) that builds a real wheel; verified it fails on the pre-fix config and passes on the post-fix one.UV_PYTHONworkaround for whenuvdoesn't auto-fetch a matching interpreter.Root cause: what changed and why
This PR originally attributed the failure to a missing
.gitdirectory andgit check-ignore. Three successive explanations were wrong, and the record matters because the comments inpyproject.tomlare aimed at whoever next edits this build config:.gitdir; hatchling runsgit check-ignore.gitignore, notably an sdist (itsincludeomits.gitignore).gitignoreis in the tarball (caught by @padak).gitignoreis reachableBisected instead, on a clean clone of
mainwith.gitand.gitignorepresent, a prebuilt SPA on disk and the wheelexcluderemoved:On 1.27.0, deleting
.gitignoreoutright still builds fine — which rules the gitignore mechanism out as the explanation entirely. Consequence: this is not a latent edge case. Any wheel build with a prebuilt SPA on current hatchling hits it, including the ordinary release path.Filed against AI-3750. Note: this PR covers only Defects 2 & 3 from that issue (the wheel-build crash and the undocumented Python floor). Defect 1 (ai-kit's docs pointing at the wrong CLI /
developers.keboola.com/cli) lives entirely in the separatekeboola/ai-kitrepo — nothing in this repo references it, so there's nothing to fix here for that part.Test plan
excludefix and confirmed exactly one_ui_dist/index.htmlon hatchling 1.27, 1.29, 1.30 and 1.32 — the fix is version-independent.py.typedverified present exactly once (file-shaped force-includes were never affected: hatchling reserves their distribution path by exact match, which is why only the directory-shaped entry broke)..gitignorein the tarball) builds with one_ui_dist/index.htmlcarrying fresh content —_bundle_uirmtree's the dir first, so a stale copy can never reach a wheel._ui_dist/; reproduced the leak before the sdist guard and its absence after.TestForceIncludeNoDuplicate::test_wheel_build_does_not_duplicate_ui_dist; fails with the fix reverted both under a normalTMPDIRand under aTMPDIRbeneath an ancestor.gitignore(the fixture's.gitboundary is what makes that second case fail rather than pass vacuously).ruff check/ruff format --checkclean;typecheck,loc-check,version-check,command-sync-checkgreen.Known, pre-existing, not caused by this PR
main(now 0.89.0; branch base 0.86.0).maindoes not carry this fix. A test merge is clean — no conflicts.make changelog-checkfails here and identically onmainat this base: it queries live GitHub releases and finds nochangelog.pyentries for 0.87.0 / 0.88.0 / 0.89.0. Expect CI red on that gate until the branch picks upmain.Related issues
AI-3750
🤖 Generated with Claude Code