fix(updater): declare packaging as a runtime dependency, fix rc-vs-GA fallback - #1663
Conversation
… fallback `_is_newer` (duplicated in hal0.updater.updater and hal0.api.routes.updater) prefers `packaging.version.Version` for PEP 440 comparison but silently falls back to a naive digit-tuple compare when `packaging` can't be imported. `packaging` was only a transitive dep of the `dev` extra (via pytest) — a production venv built by the installer (no dev extras) never has it, so every real box hit the broken fallback. Reproduced: the fallback reads "1.0.0rc1" (the pip-normalised form `importlib.metadata.version` reports for an installed 1.0.0-rc.1) as digit-tuple (1, 0, 1) — same as "1.0.1" and *higher* than "1.0.0" (1, 0, 0) — so a 1.0.0rc1 box was never offered 1.0.0 or 1.0.1 GA. Fixes: - Add `packaging>=24` to [project].dependencies (pyproject.toml + relocked uv.lock) so it ships on every production install going forward. - Replace the naive digit-tuple fallback with `_naive_version_key`, which recognizes the common a/alpha/b/beta/c/rc/pre/preview prerelease forms (pip-normalised and tag-derived alike) and ranks them below their own final release, while preserving the existing nightly-timestamp digit behavior for anything that doesn't match. Honest caveat for the PR body: the old updater's version *check* runs in the OLD, already-installed code before the new tarball is even downloaded, so this dependency fix only protects updates initiated from a venv built *after* this change lands. A box already stuck on a pre-fix venv still needs the corrected fallback logic (also shipped here) to get itself unstuck via one more update. Tests: new fallback-path unit tests in tests/updater/test_updater.py force the `packaging` ImportError via monkeypatched `__import__` and cover 1.0.0rc1 vs 1.0.0, 1.0.0rc1 vs 1.0.1, and the 1.0.0-rc.1 tag form, plus a regression check that nightly timestamp ordering still works without packaging. Closes #1640 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5096d0a to
4ff215d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff215da17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except Exception: # packaging absent — keep the best-effort key compare | ||
| return _naive_version_key(candidate) > _naive_version_key(current) |
There was a problem hiding this comment.
Bootstrap installed RC clients before relying on this fallback
When upgrading an already-installed 1.0.0rc1 production venv—the population this fallback is intended to rescue—this branch is unreachable: /api/updates/check executes the old installed module before downloading or reinstalling anything, and the CLI exits at src/hal0/cli/update_commands.py:518 when that old comparison reports no update. Because the old tuple logic rejects 1.0.0-rc.2, 1.0.0, and 1.0.1, shipping this helper and the new dependency only protects fresh or already-bootstrapped installs; provide a bootstrap path that the old client can invoke, such as an explicit targeted update or installer flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — checked src/hal0/cli/update_commands.py:517 and the /api/updates/prepare route (src/hal0/api/routes/updater.py:735). Confirmed correct: this is unreachable for an already-stuck box via the default hal0 update path since the OLD check route (which calls the OLD, broken _is_newer) sets update_available=False and the CLI bails at line 518.
But the rescue path already exists and doesn't need new code: hal0 update --target <version> (line 429) sets target_version, which bypasses the update_available gate entirely (if not body.get("update_available") and not target_version), and /api/updates/prepare never calls _is_newer — it stages whatever version is requested directly. _print_check also always prints the real latest from the manifest fetch (independent of the broken comparison), so an operator running hal0 update --check on a stuck box still sees the true latest version to pass to --target.
Updating the PR body's caveat section to name this explicitly as the operator recovery path for boxes already stuck on a pre-fix venv, so it's documented rather than tribal knowledge. Not adding new code for this — the existing --target flag already covers it.
…1715) A box still on 1.0.0-rc.1 whose venv predates #1663 runs the old naive tuple fallback in _is_newer, which ranks 1.0.0rc1 above 1.0.0 -- so /api/updates/check reports update_available: false for the GA tag and plain 'hal0 update' exits before downloading anything. The corrected comparison shipped in rc.2 never gets a chance to run on such a box, since it's the box's own (old) code making the comparison. Verified existing rescue path: 'hal0 update --target <version>' bypasses the update_available gate (update_commands.py:429) and the prepare route never calls _is_newer, so it cleanly pulls the requested release. Boxes that update rc.1 -> rc.2 first are unaffected -- the rc-vs-rc comparison orders correctly even on the old fallback. Documented the recovery path in the update-and-rollback guide (a caution aside right next to the existing --target example) and added the one-liner to the 1.0.0 GA release notes' Known Issues. Closes #1687
Closes #1640
Summary
Release-blocker found in pre-rc.2 review: the updater's version comparison
_is_newer(duplicated inhal0.updater.updaterandhal0.api.routes.updater) preferspackaging.version.Versionfor PEP 440comparison but silently falls back to a naive digit-tuple compare when
packagingcan't be imported.packagingwas declared nowhere in[project].dependencies— it was only a transitive dep of thedevextra(pulled in by
pytest). A production venv built by the installer (no devextras) never has it, so every real box hits the broken fallback.
Repro (independently verified before fixing)
The fallback's digit-tuple parser reads
"1.0.0rc1"— the pip-normalisedform
importlib.metadata.version("hal0ai")reports for an installed1.0.0-rc.1— by stripping thercletters and keeping the1, producing(1, 0, 1). That's identical to_version_tuple("1.0.1")and greater than_version_tuple("1.0.0"). Result: a1.0.0rc1box is told1.0.0GA is"not newer" and
1.0.1GA is "not newer" (tied) — it would never be offeredeither.
Fix
pyproject.toml: addpackaging>=24to[project].dependencies(already an indirect dep of
dev, pinned loosely) and relockuv.lock.This is the durable fix — every install going forward ships
packaging._naive_version_key(new, in bothhal0/updater/updater.pyandhal0/api/routes/updater.py, mirroring the existing duplication): replacesthe raw
_version_tuplecompare in thepackaging-absent fallback branchof
_is_newer. It recognizes the commona/alpha/b/beta/c/rc/pre/previewprerelease forms (both pip-normalised
1.0.0rc1and tag-derived1.0.0-rc.1) and ranks a prerelease below its own final release. Anythingit doesn't recognize (nightly timestamp tags, malformed strings) keeps the
original digit-tuple behavior, so existing nightly monotonicity is
unaffected.
Honest caveat — who actually benefits, and who doesn't (updated after review)
The old updater's version check runs in the OLD, already-installed code
before the new release tarball is even downloaded — by the time new code
is running, the decision to offer the update has already been made. Concretely
(
hal0 update,src/hal0/cli/update_commands.py:507-520):GET /api/updates/check, served by the OLD, already-runningdaemon — OLD
_is_newer, OLD broken fallback.update_available: false, the CLI prints "nothing toapply" and exits at line 518, before any download, before any new code
ever runs.
So: an already-stuck
1.0.0rc1box on the OLD fallback cannot self-heal viathe plain
hal0 updatepath, no matter what this PR ships — the corrected_naive_version_keyfallback lives in the new code that box never reaches onits own. (An earlier draft of this section claimed otherwise; a codex review
comment on this PR caught that it's wrong — see the discussion thread.)
packaging>=24dependency fix protects updates initiated from a venvbuilt after this change lands (a fresh install, or a box that has
already updated once past this PR) — that population is fully fixed.
rescue, which already exists and needed no new code:
hal0 update --target <version>(e.g.hal0 update --target 1.0.0).--targetsetstarget_version, which bypasses theupdate_availablegate entirely (
update_commands.py:518), and/api/updates/preparenevercalls
_is_newer— it stages whatever version is requested directly. Runhal0 update --checkfirst to confirm the real latest version (_print_checkalways shows the true
latestfrom the manifest fetch, independent of thebroken comparison, even when it's mislabeled "up to date").
Once a box updates via
--target, it's running this PR's code and thepackagingdependency + corrected fallback, so every subsequent plainhal0 updatebehaves correctly again.Test plan
tests/updater/test_updater.pyforce thepackaging.versionimport to fail (monkeypatchedbuiltins.__import__)and exercise
_is_newer's fallback branch directly:-
1.0.0rc1vs1.0.0(both directions)-
1.0.0rc1vs1.0.1(both directions)- tag-derived
1.0.0-rc.1form (vs1.0.0,1.0.1, and1.0.0-rc.2)- nightly timestamp regression (unaffected by the fallback change)
HAL0_HOME=$(mktemp -d) uv run --extra dev pytest tests/updater tests/release -x— 335 passedtests/api/test_updater_routes.py— 50 passedruff checkon all changed files — cleanuv lock— minimal diff,packagingmoves frompytest-onlytransitive dep to a direct
hal0aidependencymypy— no new errors introduced (pre-existing errors elsewhere inupdater.py, unrelated to this change, confirmed present onmaintoo)🤖 Generated with Claude Code