diff --git a/hermes_cli/managed_uv.py b/hermes_cli/managed_uv.py index 9be7f7cbb43..78c8f469003 100644 --- a/hermes_cli/managed_uv.py +++ b/hermes_cli/managed_uv.py @@ -72,6 +72,9 @@ class _UvResult(str): 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. + + POSIX only. This wrapper is **never** returned on Windows — see + ``ensure_uv()`` for why the ``__iter__`` override is unsafe there. """ fresh_bootstrap: bool @@ -88,17 +91,11 @@ class _UvResult(str): return iter(((str(self) or None), self.fresh_bootstrap)) -def ensure_uv() -> "_UvResult": - """Return the managed uv path, installing it first if necessary. - - 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. - """ +def _ensure_uv_path() -> Optional[str]: + """Resolve the managed uv path, installing it if necessary (plain ``str``/``None``).""" existing = resolve_uv() if existing: - return _UvResult(existing) + return existing target = managed_uv_path() target.parent.mkdir(parents=True, exist_ok=True) @@ -110,7 +107,7 @@ def ensure_uv() -> "_UvResult": except Exception as exc: logger.warning("Managed uv install failed: %s", exc) print(f" ✗ Failed to install managed uv: {exc}") - return _UvResult(None) + return None # Verify result = resolve_uv() @@ -124,6 +121,37 @@ def ensure_uv() -> "_UvResult": print(f" ✓ Managed uv installed ({version})") else: print(" ✗ Managed uv install appeared to succeed but binary not found") + return result + + +def ensure_uv(): + """Return the managed uv path, installing it first if necessary. + + On **POSIX** the result is a :class:`_UvResult` (a ``str`` subclass) that is + both usable directly as the path *and* unpackable as + ``(path, fresh_bootstrap)`` for older call sites parked on a 2-tuple + release — see :class:`_UvResult` for the update-boundary rationale. + + On **Windows** we deliberately return a plain ``str``/``None`` instead. + ``subprocess`` there serializes the argv via ``subprocess.list2cmdline``, + which iterates every entry *as a string* (``for c in arg``). The dependency + installer passes uv straight into the command list (``[uv_bin, "pip", ...]``), + so a ``_UvResult`` — whose ``__iter__`` yields ``(path, fresh_bootstrap)`` + rather than characters — would inject the bool into the command line and + crash the install with ``TypeError: sequence item 1: expected str instance, + bool found``. A plain ``str`` matches the historical Windows contract and is + subprocess-safe. (A single value cannot satisfy both 2-target unpacking and + Windows char-iteration: both use the iterator protocol, with contradictory + results.) + + On failure the result is falsy — never raises — so callers can fall back to + pip gracefully. + """ + result = _ensure_uv_path() + if platform.system() == "Windows": + # See docstring: a str subclass with an overridden __iter__ is unsafe as + # a Windows subprocess argument. Hand back the plain path (or None). + return result return _UvResult(result) diff --git a/tests/hermes_cli/test_managed_uv.py b/tests/hermes_cli/test_managed_uv.py index 8f55f1751df..e0a33107917 100644 --- a/tests/hermes_cli/test_managed_uv.py +++ b/tests/hermes_cli/test_managed_uv.py @@ -105,7 +105,7 @@ class TestEnsureUv: class TestEnsureUvUpdateBoundary: """``ensure_uv()`` must answer to both the single-value and the legacy - ``(path, fresh_bootstrap)`` call conventions. + ``(path, fresh_bootstrap)`` call conventions — **on POSIX**. ``hermes update`` runs the call site from the old, already-imported ``hermes_cli.main`` against the freshly pulled ``managed_uv``. A release @@ -113,14 +113,19 @@ class TestEnsureUvUpdateBoundary: 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. + PR #39763), or ``TypeError`` on the ``None`` failure path. On POSIX the + result must therefore be usable as a bare path *and* unpackable as a + 2-tuple, in both the success and failure cases. + + The dual contract is intentionally **not** offered on Windows — see + ``TestEnsureUvWindowsSafe`` for why — so these tests pin ``platform.system`` + to a POSIX value. """ 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): + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ + patch("hermes_cli.managed_uv.platform.system", return_value="Linux"): from hermes_cli.managed_uv import ensure_uv uv_bin = ensure_uv() assert uv_bin == str(tmp_path / "bin" / "uv") @@ -128,7 +133,8 @@ class TestEnsureUvUpdateBoundary: 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): + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ + patch("hermes_cli.managed_uv.platform.system", return_value="Linux"): 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") @@ -136,6 +142,7 @@ class TestEnsureUvUpdateBoundary: 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.platform.system", return_value="Linux"), \ 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() @@ -143,6 +150,52 @@ class TestEnsureUvUpdateBoundary: assert fresh is False +class TestEnsureUvWindowsSafe: + """On Windows ``ensure_uv()`` must return a plain ``str``/``None``. + + ``subprocess`` on Windows serializes argv through + ``subprocess.list2cmdline``, which iterates every entry *as a string* + (``for c in arg``). The dependency installer feeds uv straight into the + command list (``[uv_bin, "pip", "install", ...]``). A ``str`` subclass + whose ``__iter__`` yields ``(path, fresh_bootstrap)`` instead of characters + therefore injects the bool into the command line and crashes the install + with ``TypeError: sequence item 1: expected str instance, bool found`` + (a real field report on a 10-commits-behind Windows install). A single + return value cannot serve both the legacy 2-tuple unpack and Windows + char-iteration — both use the iterator protocol — so Windows opts out of + the wrapper entirely. + """ + + def test_uvresult_would_break_windows_list2cmdline(self): + # Canary: this is *why* the wrapper is gated off Windows. If a future + # change makes _UvResult char-iterable (and thus list2cmdline-safe), + # the gate may be revisited. + import subprocess + from hermes_cli.managed_uv import _UvResult + with pytest.raises(TypeError): + subprocess.list2cmdline([_UvResult("C:\\hermes\\uv.exe"), "pip"]) + + def test_windows_returns_plain_str_safe_for_subprocess(self, tmp_path): + import subprocess + # On (mocked) Windows the managed binary is uv.exe. + _make_executable(tmp_path / "bin" / "uv.exe") + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ + patch("hermes_cli.managed_uv.platform.system", return_value="Windows"): + from hermes_cli.managed_uv import _UvResult, ensure_uv + uv_bin = ensure_uv() + assert type(uv_bin) is str and not isinstance(uv_bin, _UvResult) + # The exact operation that crashed in the field must now succeed. + cmdline = subprocess.list2cmdline([uv_bin, "pip", "install", "-e", "."]) + assert "pip" in cmdline and "install" in cmdline + + def test_windows_failure_returns_none(self, tmp_path): + with patch("hermes_cli.managed_uv.get_hermes_home", return_value=tmp_path), \ + patch("hermes_cli.managed_uv.platform.system", return_value="Windows"), \ + patch("hermes_cli.managed_uv._install_uv", side_effect=RuntimeError("network down")): + from hermes_cli.managed_uv import ensure_uv + assert ensure_uv() is None + + # --------------------------------------------------------------------------- # update_managed_uv # ---------------------------------------------------------------------------