fix(update/windows): don't return _UvResult on Windows (subprocess argv crash) (#39820)
PR #39780 made ensure_uv() return a _UvResult — a str subclass whose __iter__ yields (path, fresh_bootstrap) so old `uv_bin, fresh = ensure_uv()` call sites survive the update boundary. That trick is unsafe on Windows. The dependency installer passes uv straight into the command list (`[uv_bin, "pip", "install", ...]`). On Windows, subprocess serializes argv via subprocess.list2cmdline, which iterates every entry *as a string* (`for c in arg`). Because _UvResult overrides __iter__, that iteration yields (path, fresh_bootstrap) instead of characters, injecting the bool into the command line and crashing the first update with: TypeError: sequence item 1: expected str instance, bool found This bites the common single-assignment caller (`uv_bin = ensure_uv()`) on its first update after #39780: the freshly pulled _UvResult flows into the old in-memory call site and into the argv. Reported in the field on a ~10-commits-behind Windows install. A single return value cannot satisfy both legacy 2-target unpacking and Windows char-iteration — both use the iterator protocol with contradictory results. So gate the wrapper to POSIX: Windows returns a plain str/None (the historical, subprocess-safe contract). POSIX keeps _UvResult and the #39780 update-boundary fix. Tests: list2cmdline canary proving _UvResult breaks Windows, plus Windows returns-plain-str and POSIX dual-contract coverage.
This commit is contained in:
parent
ca8c78e588
commit
d880b5be09
@ -72,6 +72,9 @@ class _UvResult(str):
|
|||||||
Missing uv is the empty string (falsy) instead of ``None`` so legacy
|
Missing uv is the empty string (falsy) instead of ``None`` so legacy
|
||||||
2-target call sites can still unpack a failure without raising, while
|
2-target call sites can still unpack a failure without raising, while
|
||||||
``if not uv_bin`` keeps working for single-value callers.
|
``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
|
fresh_bootstrap: bool
|
||||||
@ -88,17 +91,11 @@ class _UvResult(str):
|
|||||||
return iter(((str(self) or None), self.fresh_bootstrap))
|
return iter(((str(self) or None), self.fresh_bootstrap))
|
||||||
|
|
||||||
|
|
||||||
def ensure_uv() -> "_UvResult":
|
def _ensure_uv_path() -> Optional[str]:
|
||||||
"""Return the managed uv path, installing it first if necessary.
|
"""Resolve the managed uv path, installing it if necessary (plain ``str``/``None``)."""
|
||||||
|
|
||||||
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()
|
existing = resolve_uv()
|
||||||
if existing:
|
if existing:
|
||||||
return _UvResult(existing)
|
return existing
|
||||||
|
|
||||||
target = managed_uv_path()
|
target = managed_uv_path()
|
||||||
target.parent.mkdir(parents=True, exist_ok=True)
|
target.parent.mkdir(parents=True, exist_ok=True)
|
||||||
@ -110,7 +107,7 @@ def ensure_uv() -> "_UvResult":
|
|||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
logger.warning("Managed uv install failed: %s", exc)
|
logger.warning("Managed uv install failed: %s", exc)
|
||||||
print(f" ✗ Failed to install managed uv: {exc}")
|
print(f" ✗ Failed to install managed uv: {exc}")
|
||||||
return _UvResult(None)
|
return None
|
||||||
|
|
||||||
# Verify
|
# Verify
|
||||||
result = resolve_uv()
|
result = resolve_uv()
|
||||||
@ -124,6 +121,37 @@ def ensure_uv() -> "_UvResult":
|
|||||||
print(f" ✓ Managed uv installed ({version})")
|
print(f" ✓ Managed uv installed ({version})")
|
||||||
else:
|
else:
|
||||||
print(" ✗ Managed uv install appeared to succeed but binary not found")
|
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)
|
return _UvResult(result)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -105,7 +105,7 @@ class TestEnsureUv:
|
|||||||
|
|
||||||
class TestEnsureUvUpdateBoundary:
|
class TestEnsureUvUpdateBoundary:
|
||||||
"""``ensure_uv()`` must answer to both the single-value and the legacy
|
"""``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 update`` runs the call site from the old, already-imported
|
||||||
``hermes_cli.main`` against the freshly pulled ``managed_uv``. A release
|
``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
|
against the single-value module; the path is an iterable ``str`` so the
|
||||||
2-target unpack walked its characters and raised
|
2-target unpack walked its characters and raised
|
||||||
``ValueError: too many values to unpack (expected 2)`` (root cause behind
|
``ValueError: too many values to unpack (expected 2)`` (root cause behind
|
||||||
PR #39763), or ``TypeError`` on the ``None`` failure path. The result must
|
PR #39763), or ``TypeError`` on the ``None`` failure path. On POSIX the
|
||||||
therefore be usable as a bare path *and* unpackable as a 2-tuple, in both
|
result must therefore be usable as a bare path *and* unpackable as a
|
||||||
the success and failure cases.
|
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):
|
def test_success_usable_as_single_value(self, tmp_path):
|
||||||
_make_executable(tmp_path / "bin" / "uv")
|
_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
|
from hermes_cli.managed_uv import ensure_uv
|
||||||
uv_bin = ensure_uv()
|
uv_bin = ensure_uv()
|
||||||
assert uv_bin == str(tmp_path / "bin" / "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):
|
def test_success_unpacks_as_legacy_two_tuple(self, tmp_path):
|
||||||
_make_executable(tmp_path / "bin" / "uv")
|
_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
|
from hermes_cli.managed_uv import ensure_uv
|
||||||
uv_bin, fresh = ensure_uv() # old: uv_bin, fresh_bootstrap = ensure_uv()
|
uv_bin, fresh = ensure_uv() # old: uv_bin, fresh_bootstrap = ensure_uv()
|
||||||
assert uv_bin == str(tmp_path / "bin" / "uv")
|
assert uv_bin == str(tmp_path / "bin" / "uv")
|
||||||
@ -136,6 +142,7 @@ class TestEnsureUvUpdateBoundary:
|
|||||||
|
|
||||||
def test_failure_unpacks_without_raising(self, tmp_path):
|
def test_failure_unpacks_without_raising(self, tmp_path):
|
||||||
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"), \
|
||||||
patch("hermes_cli.managed_uv._install_uv", side_effect=RuntimeError("network down")):
|
patch("hermes_cli.managed_uv._install_uv", side_effect=RuntimeError("network down")):
|
||||||
from hermes_cli.managed_uv import ensure_uv
|
from hermes_cli.managed_uv import ensure_uv
|
||||||
uv_bin, fresh = ensure_uv()
|
uv_bin, fresh = ensure_uv()
|
||||||
@ -143,6 +150,52 @@ class TestEnsureUvUpdateBoundary:
|
|||||||
assert fresh is False
|
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
|
# update_managed_uv
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user