From db204ae2035021bb5f1e76004c13296853a139b6 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Fri, 5 Jun 2026 07:08:43 -0500 Subject: [PATCH] fix(update): make ensure_uv() survive the update boundary (no first-run crash) (#39780) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(update): make ensure_uv() survive the update boundary (no first-run crash) `hermes update` runs the `ensure_uv()` call site from the old, already-imported `hermes_cli.main` against the *freshly pulled* `managed_uv` (managed_uv is only ever lazily imported, so it loads from disk post-pull). `ensure_uv()`'s return arity flipped from a single path string to `(path, fresh_bootstrap)` (4df280d51) and back to a single string (fb853a178). Installs parked on a 2-tuple release unpack `uv_bin, fresh_bootstrap = ensure_uv()` against the new single-value module and crash the first update with `ValueError: not enough values to unpack (expected 2, got 1)` — inside the dependency-install step, *before* the PR #39763 subprocess hand-off can run. Return a `_UvResult` (a `str` subclass) that is usable as the bare path AND unpackable as `(path|None, fresh_bootstrap)`. Missing uv is `""` (falsy) instead of `None` so legacy 2-target call sites can unpack a failure without raising, while `if not uv_bin` keeps working for single-value callers. fresh_bootstrap is always False (the rebuild-venv path it gated was scrapped in fb853a178). * docs(update): correct the verified error string + mechanism for ensure_uv() A hermetic repro (old 2-target call site vs the freshly-pulled single-value module) shows the first-update crash is exactly the string from PR #39763's report: `ValueError: too many values to unpack (expected 2)` — not "not enough". The returned path is a plain `str`, which is iterable, so `uv_bin, fresh = ensure_uv()` walks its characters; the failure path's `None` return raises `TypeError: cannot unpack non-iterable NoneType`. Both are fixed by `_UvResult`. Comment/test wording updated to match; no behavior change. --- hermes_cli/managed_uv.py | 51 +++++++++++++++++++++++++---- tests/hermes_cli/test_managed_uv.py | 47 ++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 8 deletions(-) diff --git a/hermes_cli/managed_uv.py b/hermes_cli/managed_uv.py index 95cde0e7b..9be7f7cbb 100644 --- a/hermes_cli/managed_uv.py +++ b/hermes_cli/managed_uv.py @@ -51,15 +51,54 @@ def resolve_uv() -> Optional[str]: return None -def ensure_uv() -> Optional[str]: +class _UvResult(str): + """``ensure_uv()`` return value that survives an update boundary. + + ``ensure_uv()``'s arity has flipped between a single path string and a + ``(path, fresh_bootstrap)`` tuple across releases. ``hermes update`` runs + the call site from the *old*, already-imported ``hermes_cli.main`` against + this *freshly pulled* module, so the two can disagree on how many values + ``ensure_uv()`` returns. An install parked on a 2-tuple release runs + ``uv_bin, fresh_bootstrap = ensure_uv()`` against the single-value module + and crashes the first update: the returned path is a plain ``str``, which is + itself iterable, so the 2-target unpack walks its characters and raises + ``ValueError: too many values to unpack (expected 2)`` (and on the failure + path the ``None`` return raises ``TypeError: cannot unpack non-iterable + NoneType``). This wrapper answers to both conventions: + + uv_bin = ensure_uv() # behaves as the path str ("" when absent) + uv_bin, fresh = ensure_uv() # unpacks as (path|None, fresh_bootstrap) + + Missing uv is the empty string (falsy) instead of ``None`` so legacy + 2-target call sites can still unpack a failure without raising, while + ``if not uv_bin`` keeps working for single-value callers. + """ + + fresh_bootstrap: bool + + def __new__(cls, path: Optional[str], fresh: bool = False) -> "_UvResult": + self = super().__new__(cls, path or "") + self.fresh_bootstrap = fresh + return self + + def __iter__(self): + # Tuple-unpacking hook for legacy ``uv_bin, fresh = ensure_uv()`` sites. + # First element mirrors the historical contract: the path string, or + # ``None`` when uv is unavailable. + return iter(((str(self) or None), self.fresh_bootstrap)) + + +def ensure_uv() -> "_UvResult": """Return the managed uv path, installing it first if necessary. - On failure returns ``None`` (never raises) so callers can fall - back to pip gracefully. + Returns a :class:`_UvResult` (a ``str`` subclass) that is both usable + directly as the path and unpackable as ``(path, fresh_bootstrap)`` for + older call sites. On failure the result is falsy (``""``) — never raises — + so callers can fall back to pip gracefully. """ existing = resolve_uv() if existing: - return existing + return _UvResult(existing) target = managed_uv_path() target.parent.mkdir(parents=True, exist_ok=True) @@ -71,7 +110,7 @@ def ensure_uv() -> Optional[str]: except Exception as exc: logger.warning("Managed uv install failed: %s", exc) print(f" ✗ Failed to install managed uv: {exc}") - return None + return _UvResult(None) # Verify result = resolve_uv() @@ -85,7 +124,7 @@ def ensure_uv() -> Optional[str]: print(f" ✓ Managed uv installed ({version})") else: print(" ✗ Managed uv install appeared to succeed but binary not found") - return result + return _UvResult(result) def update_managed_uv() -> Optional[str]: diff --git a/tests/hermes_cli/test_managed_uv.py b/tests/hermes_cli/test_managed_uv.py index a9e9b92a6..8f55f1751 100644 --- a/tests/hermes_cli/test_managed_uv.py +++ b/tests/hermes_cli/test_managed_uv.py @@ -92,12 +92,55 @@ class TestEnsureUv: assert path == str(tmp_path / "bin" / "uv") mock_install.assert_called_once() - def test_install_failure_returns_none(self, tmp_path): + def test_install_failure_returns_falsy(self, tmp_path): with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ patch("hermes_cli.managed_uv._install_uv", side_effect=RuntimeError("network down")): from hermes_cli.managed_uv import ensure_uv path = ensure_uv() - assert path is None + # Failure is a falsy sentinel (not None) so legacy 2-target call + # sites can still unpack it without raising — see + # TestEnsureUvUpdateBoundary for why. + assert not path + + +class TestEnsureUvUpdateBoundary: + """``ensure_uv()`` must answer to both the single-value and the legacy + ``(path, fresh_bootstrap)`` call conventions. + + ``hermes update`` runs the call site from the old, already-imported + ``hermes_cli.main`` against the freshly pulled ``managed_uv``. A release + parked on a ``(path, fresh)`` tuple runs ``uv_bin, fresh = ensure_uv()`` + against the single-value module; the path is an iterable ``str`` so the + 2-target unpack walked its characters and raised + ``ValueError: too many values to unpack (expected 2)`` (root cause behind + PR #39763), or ``TypeError`` on the ``None`` failure path. The result must + therefore be usable as a bare path *and* unpackable as a 2-tuple, in both + the success and failure cases. + """ + + def test_success_usable_as_single_value(self, tmp_path): + _make_executable(tmp_path / "bin" / "uv") + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path): + from hermes_cli.managed_uv import ensure_uv + uv_bin = ensure_uv() + assert uv_bin == str(tmp_path / "bin" / "uv") + assert bool(uv_bin) is True + + def test_success_unpacks_as_legacy_two_tuple(self, tmp_path): + _make_executable(tmp_path / "bin" / "uv") + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path): + from hermes_cli.managed_uv import ensure_uv + uv_bin, fresh = ensure_uv() # old: uv_bin, fresh_bootstrap = ensure_uv() + assert uv_bin == str(tmp_path / "bin" / "uv") + assert fresh is False + + def test_failure_unpacks_without_raising(self, tmp_path): + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ + patch("hermes_cli.managed_uv._install_uv", side_effect=RuntimeError("network down")): + from hermes_cli.managed_uv import ensure_uv + uv_bin, fresh = ensure_uv() + assert uv_bin is None + assert fresh is False # ---------------------------------------------------------------------------