From bebd4f851631e65e0ca0eaa1266ad4bce8aad701 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Tue, 26 May 2026 23:11:28 -0700 Subject: [PATCH] fix(cli): restrict uv-tool-install detection to running interpreter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on PR #29703 flagged two issues with the `uv tool list` fallback in `is_uv_tool_install`: 1. False positive: `uv tool list` returns the *machine*'s installed tools, not the active install. A regular pip/venv Hermes on a host that also has `uv tool install hermes-agent` available would be misclassified as a uv-tool install, and `hermes update` would upgrade the wrong copy. 2. Overhead: the subprocess call (up to a 15s timeout) was triggered even from `recommended_update_command_for_method`, which just computes a display string. Restrict detection to properties of the running interpreter (`sys.prefix` and `sys.executable` — both can carry the uv-tool layout marker depending on entry point). Drop the `uv tool list` fallback and the `uv_path` parameter entirely. `_cmd_update_pip` now also surfaces a clear hint when the runtime looks like a uv-tool install but `uv` is missing from PATH, instead of silently falling back to `python -m pip`. --- hermes_cli/config.py | 59 +++++----- hermes_cli/main.py | 12 +- tests/hermes_cli/test_uv_tool_update.py | 143 +++++++++++++++--------- 3 files changed, 122 insertions(+), 92 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 40aedf625..f5985556c 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -329,39 +329,31 @@ def stamp_install_method(method: str) -> None: pass -def is_uv_tool_install(uv_path: Optional[str] = None) -> bool: - """Return True when Hermes is installed via ``uv tool install hermes-agent``. +def is_uv_tool_install() -> bool: + """Return True when the *running* Hermes lives in a ``uv tool`` layout. - ``uv tool`` installs live outside any virtualenv, so ``uv pip install`` - (the previous update path) fails with ``No virtual environment found``. - The fast path inspects ``sys.prefix`` for the standard uv tool layout - (``.../uv/tools/hermes-agent/...``); the authoritative fallback shells - out to ``uv tool list``. Returns False on any error so callers fall - back to the legacy pip path. + ``uv tool install hermes-agent`` places the install at + ``.../uv/tools/hermes-agent/...`` (default ``~/.local/share/uv/tools``, + or ``$UV_TOOL_DIR/...``). Such installs live outside any virtualenv, so + ``uv pip install`` fails with ``No virtual environment found`` and the + update path must use ``uv tool upgrade`` instead. + + Detection is intentionally restricted to properties of the running + interpreter (``sys.prefix`` / ``sys.executable``). We deliberately do + NOT consult ``uv tool list``: it would also return True when + ``hermes-agent`` happens to be uv-tool-installed on the machine while + the *active* Hermes is a regular pip/venv install, causing + ``hermes update`` to upgrade the wrong copy. It would also block on a + subprocess call (~seconds) just to compute a recommendation string. """ - prefix = os.path.normpath(sys.prefix).replace(os.sep, "/").lower() - if "/uv/tools/hermes-agent/" in prefix + "/": + def _has_uv_tool_marker(path: str) -> bool: + norm = os.path.normpath(path).replace(os.sep, "/").lower() + return "/uv/tools/hermes-agent/" in norm + "/" + + if _has_uv_tool_marker(sys.prefix): + return True + if _has_uv_tool_marker(sys.executable or ""): return True - if uv_path is None: - import shutil - uv_path = shutil.which("uv") - if not uv_path: - return False - try: - result = subprocess.run( - [uv_path, "tool", "list"], - capture_output=True, - text=True, - timeout=15, - ) - except (OSError, subprocess.SubprocessError): - return False - if result.returncode != 0: - return False - for line in result.stdout.splitlines(): - tokens = line.strip().split() - if tokens and tokens[0] == "hermes-agent": - return True return False @@ -374,11 +366,10 @@ def recommended_update_command_for_method(method: str) -> str: if method == "docker": return "docker pull nousresearch/hermes-agent:latest" if method == "pip": + if is_uv_tool_install(): + return "uv tool upgrade hermes-agent" import shutil - uv = shutil.which("uv") - if uv: - if is_uv_tool_install(uv): - return "uv tool upgrade hermes-agent" + if shutil.which("uv"): return "uv pip install --upgrade hermes-agent" return "pip install --upgrade hermes-agent" return "hermes update" diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 50e24fc83..86b525469 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8977,11 +8977,13 @@ def _cmd_update_pip(args): print("→ Checking PyPI for updates...") uv = shutil.which("uv") - if uv: - if is_uv_tool_install(uv): - cmd = [uv, "tool", "upgrade", "hermes-agent"] - else: - cmd = [uv, "pip", "install", "--upgrade", "hermes-agent"] + if is_uv_tool_install(): + if not uv: + print("✗ Detected a uv-tool install but `uv` is not on PATH; install uv and retry.") + sys.exit(1) + cmd = [uv, "tool", "upgrade", "hermes-agent"] + elif uv: + cmd = [uv, "pip", "install", "--upgrade", "hermes-agent"] else: cmd = [sys.executable, "-m", "pip", "install", "--upgrade", "hermes-agent"] diff --git a/tests/hermes_cli/test_uv_tool_update.py b/tests/hermes_cli/test_uv_tool_update.py index 4e0978873..b51fefe3b 100644 --- a/tests/hermes_cli/test_uv_tool_update.py +++ b/tests/hermes_cli/test_uv_tool_update.py @@ -6,6 +6,10 @@ environment found``. ``is_uv_tool_install`` should detect this layout and both the user-facing recommended command and the actual ``_cmd_update_pip`` subprocess invocation should switch to ``uv tool upgrade hermes-agent``. + +Detection is restricted to properties of the running interpreter +(``sys.prefix`` / ``sys.executable``) so a pip/venv install on a machine +that also has ``uv tool install hermes-agent`` does not get misclassified. """ from __future__ import annotations @@ -26,68 +30,59 @@ class TestIsUvToolInstall: from hermes_cli import config with patch.object(config.sys, "prefix", "/home/user/.local/share/uv/tools/hermes-agent"): - assert config.is_uv_tool_install("uv") is True + assert config.is_uv_tool_install() is True - def test_returns_true_when_uv_tool_list_includes_hermes_agent(self): - from hermes_cli import config - - completed = subprocess.CompletedProcess( - ["uv", "tool", "list"], - 0, - stdout="hermes-agent v0.14.0\n- hermes\n- hermes-bot\nblack v23.0.0\n- black\n", - stderr="", - ) - with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("subprocess.run", return_value=completed) as mock_run: - assert config.is_uv_tool_install("/usr/local/bin/uv") is True - mock_run.assert_called_once() - assert mock_run.call_args[0][0] == ["/usr/local/bin/uv", "tool", "list"] - - def test_returns_false_when_uv_tool_list_lacks_hermes_agent(self): - from hermes_cli import config - - completed = subprocess.CompletedProcess( - ["uv", "tool", "list"], 0, stdout="black v23.0.0\n- black\nruff v0.5.0\n- ruff\n", stderr="" - ) - with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("subprocess.run", return_value=completed): - assert config.is_uv_tool_install("uv") is False - - def test_returns_false_when_uv_tool_list_fails(self): - from hermes_cli import config - - completed = subprocess.CompletedProcess(["uv", "tool", "list"], 2, stdout="", stderr="oops") - with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("subprocess.run", return_value=completed): - assert config.is_uv_tool_install("uv") is False - - def test_returns_false_when_subprocess_raises(self): + def test_returns_true_when_sys_executable_matches_uv_tool_layout(self): + """Some uv-tool layouts surface the marker on ``sys.executable`` (bin/python).""" from hermes_cli import config with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("subprocess.run", side_effect=subprocess.TimeoutExpired(["uv"], 15)): - assert config.is_uv_tool_install("uv") is False + patch.object( + config.sys, + "executable", + "/home/user/.local/share/uv/tools/hermes-agent/bin/python", + ): + assert config.is_uv_tool_install() is True - def test_returns_false_when_no_uv_available(self): + def test_returns_false_when_neither_prefix_nor_executable_matches(self): from hermes_cli import config with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("shutil.which", return_value=None): + patch.object(config.sys, "executable", "/usr/bin/python3"): assert config.is_uv_tool_install() is False - def test_indented_alias_line_does_not_false_positive(self): - """A tool whose alias line is ``- hermes-agent`` shouldn't match.""" + def test_does_not_consult_uv_tool_list(self): + """Detection must NOT shell out: ``uv tool list`` would false-positive + when the active install is pip/venv but the machine also has + ``uv tool install hermes-agent`` somewhere on disk. Copilot review on + PR #29703 flagged this; the fix is to never call ``uv tool list`` + from the detection path.""" from hermes_cli import config - completed = subprocess.CompletedProcess( - ["uv", "tool", "list"], - 0, - stdout="some-other-tool v1.0.0\n- hermes-agent\n", - stderr="", - ) with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ - patch("subprocess.run", return_value=completed): - assert config.is_uv_tool_install("uv") is False + patch.object(config.sys, "executable", "/usr/bin/python3"), \ + patch("subprocess.run") as mock_run: + assert config.is_uv_tool_install() is False + mock_run.assert_not_called() + + def test_case_insensitive_match(self): + """Match must be case-insensitive — Windows paths preserve case + (e.g. ``...AppData\\Local\\UV\\Tools\\hermes-agent``) and a case-sensitive + check would miss them. We exercise the lower-cased compare path here + without monkey-patching ``os.sep``, which would break the whole suite.""" + from hermes_cli import config + + with patch.object( + config.sys, "prefix", "/HOME/USER/.local/share/UV/Tools/hermes-agent" + ): + assert config.is_uv_tool_install() is True + + def test_handles_empty_executable(self): + from hermes_cli import config + + with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ + patch.object(config.sys, "executable", ""): + assert config.is_uv_tool_install() is False # --------------------------------------------------------------------------- @@ -104,6 +99,16 @@ class TestRecommendedUpdateCommandForUvTool: cmd = config.recommended_update_command_for_method("pip") assert cmd == "uv tool upgrade hermes-agent" + def test_uv_tool_install_recommends_uv_tool_upgrade_even_without_uv_on_path(self): + """Recommendation reflects the *install method*, not whether ``uv`` is + currently on PATH — the user needs to know the right command to run.""" + from hermes_cli import config + + with patch("shutil.which", return_value=None), \ + patch.object(config, "is_uv_tool_install", return_value=True): + cmd = config.recommended_update_command_for_method("pip") + assert cmd == "uv tool upgrade hermes-agent" + def test_uv_pip_install_keeps_legacy_recommendation(self): """Existing behavior: uv is on PATH but Hermes is a regular pip install.""" from hermes_cli import config @@ -114,12 +119,28 @@ class TestRecommendedUpdateCommandForUvTool: assert cmd == "uv pip install --upgrade hermes-agent" def test_no_uv_falls_back_to_plain_pip(self): - from hermes_cli.config import recommended_update_command_for_method + from hermes_cli import config - with patch("shutil.which", return_value=None): - cmd = recommended_update_command_for_method("pip") + with patch("shutil.which", return_value=None), \ + patch.object(config, "is_uv_tool_install", return_value=False): + cmd = config.recommended_update_command_for_method("pip") assert cmd == "pip install --upgrade hermes-agent" + def test_recommendation_does_not_spawn_subprocess(self): + """Computing the recommendation string must be cheap — no ``uv tool list`` + spawn. Copilot review on PR #29703 flagged the prior subprocess hop + as adding overhead and a multi-second timeout window for what is + purely a display string.""" + from hermes_cli import config + + with patch.object(config.sys, "prefix", "/some/unrelated/venv"), \ + patch.object(config.sys, "executable", "/usr/bin/python3"), \ + patch("shutil.which", return_value="/usr/local/bin/uv"), \ + patch("subprocess.run") as mock_run: + cmd = config.recommended_update_command_for_method("pip") + mock_run.assert_not_called() + assert cmd == "uv pip install --upgrade hermes-agent" + # --------------------------------------------------------------------------- # _cmd_update_pip subprocess command @@ -162,7 +183,8 @@ class TestCmdUpdatePipUsesUvTool: from hermes_cli.main import _cmd_update_pip mock_run.return_value = subprocess.CompletedProcess(["pip"], 0, stdout="", stderr="") - with patch("shutil.which", return_value=None): + with patch("shutil.which", return_value=None), \ + patch("hermes_cli.config.is_uv_tool_install", return_value=False): _cmd_update_pip(SimpleNamespace()) cmd = mock_run.call_args[0][0] @@ -178,3 +200,18 @@ class TestCmdUpdatePipUsesUvTool: with pytest.raises(SystemExit) as exc_info: _cmd_update_pip(SimpleNamespace()) assert exc_info.value.code == 1 + + @patch("subprocess.run") + def test_uv_tool_install_without_uv_on_path_exits_with_hint(self, mock_run): + """If the running interpreter looks like a uv-tool install but ``uv`` is + somehow missing from PATH, surface a clear hint instead of silently + falling back to ``python -m pip``, which would either fail (no venv) + or upgrade the wrong copy.""" + from hermes_cli.main import _cmd_update_pip + + with patch("shutil.which", return_value=None), \ + patch("hermes_cli.config.is_uv_tool_install", return_value=True): + with pytest.raises(SystemExit) as exc_info: + _cmd_update_pip(SimpleNamespace()) + assert exc_info.value.code == 1 + mock_run.assert_not_called()