fix(cli): apply Termux carve-out to doctor --live's npx browser probe
_browser_available()'s npx rung was missing the bare-npx-on-Termux
guard its sibling probes (dep_ensure, nous_subscription) already
apply, so it could report the browser probe available on Termux when
local mode would actually reject the bare npx fallback and fail on
first use.
Also adds argv-level coverage for the two real npx launch sites
(_run_browser_command, _run_chrome_fallback_command) and an
end-to-end test proving _find_agent_browser's lazy-install fallback
and ensure_dependency("browser")'s npx check terminate without
recursion.
This commit is contained in:
@@ -93,12 +93,21 @@ def _browser_available() -> bool:
|
||||
pass
|
||||
# agent-browser resolves lazily via npx on the default install (#43564),
|
||||
# invisible to the PATH/node_modules probes above. Mirror the rung
|
||||
# hermes_cli.doctor uses so this probe can't diverge from it.
|
||||
# hermes_cli.doctor uses so this probe can't diverge from it, including
|
||||
# the Termux carve-out (bare npx is too fragile to advertise as ready
|
||||
# there — see check_browser_requirements).
|
||||
try:
|
||||
from tools.browser_tool import _find_agent_browser, _is_npx_agent_browser_sentinel
|
||||
return _is_npx_agent_browser_sentinel(_find_agent_browser(validate=False))
|
||||
from tools.browser_tool import (
|
||||
_find_agent_browser,
|
||||
_is_npx_agent_browser_sentinel,
|
||||
_requires_real_termux_browser_install,
|
||||
)
|
||||
browser_cmd = _find_agent_browser(validate=False)
|
||||
except Exception:
|
||||
return False
|
||||
if not _is_npx_agent_browser_sentinel(browser_cmd):
|
||||
return False
|
||||
return not _requires_real_termux_browser_install(browser_cmd)
|
||||
|
||||
|
||||
def _launch_browser_probe(timeout: float) -> tuple:
|
||||
|
||||
@@ -58,6 +58,46 @@ def test_has_npx_agent_browser_false_when_nothing_resolves():
|
||||
assert _has_npx_agent_browser() is False
|
||||
|
||||
|
||||
def test_find_agent_browser_lazy_install_cycle_terminates(monkeypatch):
|
||||
"""tools.browser_tool._find_agent_browser's "nothing found" branch calls
|
||||
ensure_dependency("browser"), whose "browser" check now includes
|
||||
_has_npx_agent_browser() -> _find_agent_browser(validate=False) again.
|
||||
That nested call must NOT be able to trigger another ensure_dependency
|
||||
call (only validate=True does that) — verifying the cycle is bounded to
|
||||
one extra rescan, not unbounded recursion, using the real functions on
|
||||
both sides rather than mocking the cycle away."""
|
||||
import shutil
|
||||
import tools.browser_tool as bt
|
||||
from hermes_cli import dep_ensure
|
||||
|
||||
monkeypatch.setattr(bt, "_cached_agent_browser", None)
|
||||
monkeypatch.setattr(bt, "_agent_browser_resolved", False)
|
||||
monkeypatch.setattr(shutil, "which", lambda *a, **k: None)
|
||||
monkeypatch.setattr(bt, "_resolve_npx_bin", lambda: None)
|
||||
monkeypatch.setattr(dep_ensure, "_has_system_browser", lambda: False)
|
||||
monkeypatch.setattr(dep_ensure, "_has_hermes_agent_browser", lambda: False)
|
||||
monkeypatch.setattr(dep_ensure, "_find_install_script", lambda *a, **k: (None, None))
|
||||
|
||||
real_find_agent_browser = bt._find_agent_browser
|
||||
validate_calls = []
|
||||
|
||||
def counting_find_agent_browser(*, validate=True):
|
||||
validate_calls.append(validate)
|
||||
return real_find_agent_browser(validate=validate)
|
||||
|
||||
monkeypatch.setattr(bt, "_find_agent_browser", counting_find_agent_browser)
|
||||
|
||||
with pytest.raises(FileNotFoundError):
|
||||
bt._find_agent_browser(validate=True)
|
||||
|
||||
# One outer validate=True call, plus exactly one bounded nested
|
||||
# validate=False rescan from _has_npx_agent_browser inside
|
||||
# ensure_dependency's "browser" check — not unbounded recursion, and not
|
||||
# a second ensure_dependency("browser") call (which would show up as a
|
||||
# second `True` in this list).
|
||||
assert validate_calls == [True, False]
|
||||
|
||||
|
||||
@pytest.mark.windows_only
|
||||
def test_ensure_dependency_uses_powershell_on_windows(tmp_path):
|
||||
"""``windows_only``: the assertion is that we shell out to a real
|
||||
|
||||
@@ -219,6 +219,18 @@ class TestBrowserAvailableNpxRung:
|
||||
|
||||
assert _real_browser_available() is False
|
||||
|
||||
def test_false_on_termux_local_bare_npx(self, monkeypatch, tmp_path):
|
||||
"""On Termux in local mode the bare npx fallback is too fragile to
|
||||
advertise as ready — must not diverge from dep_ensure/nous_subscription's
|
||||
same carve-out."""
|
||||
self._block_path_and_node_modules_checks(monkeypatch, tmp_path)
|
||||
import tools.browser_tool as bt
|
||||
|
||||
monkeypatch.setattr(bt, "_find_agent_browser", lambda **_kw: "npx agent-browser")
|
||||
monkeypatch.setattr(bt, "_requires_real_termux_browser_install", lambda cmd: True)
|
||||
|
||||
assert _real_browser_available() is False
|
||||
|
||||
|
||||
class TestFailureIsolation:
|
||||
def test_one_probe_raising_does_not_stop_others(self, monkeypatch):
|
||||
|
||||
@@ -13,6 +13,8 @@ from tools.browser_tool import (
|
||||
_discover_homebrew_node_dirs,
|
||||
_find_agent_browser,
|
||||
_run_browser_command,
|
||||
_run_chrome_fallback_command,
|
||||
AGENT_BROWSER_NPX_SPEC,
|
||||
_SANE_PATH,
|
||||
check_browser_requirements,
|
||||
)
|
||||
@@ -349,6 +351,59 @@ class TestRunBrowserCommandPathConstruction:
|
||||
]
|
||||
|
||||
|
||||
def test_npx_sentinel_resolves_via_resolve_npx_bin_with_pinned_spec(self, tmp_path):
|
||||
"""When _find_agent_browser resolves the npx sentinel, the cmd prefix
|
||||
must come from _resolve_npx_bin() (not a bare shutil.which("npx"), which
|
||||
could let a broken system npx shadow a healthy Hermes-managed one) and
|
||||
use the pinned agent-browser npx spec, not a bare "agent-browser"."""
|
||||
captured_cmd = None
|
||||
|
||||
mock_proc = MagicMock()
|
||||
mock_proc.returncode = 0
|
||||
mock_proc.wait.return_value = 0
|
||||
|
||||
def capture_popen(cmd, **kwargs):
|
||||
nonlocal captured_cmd
|
||||
captured_cmd = cmd
|
||||
return mock_proc
|
||||
|
||||
fake_session = {
|
||||
"session_name": "test-session",
|
||||
"session_id": "test-id",
|
||||
"cdp_url": None,
|
||||
}
|
||||
fake_json = json.dumps({"success": True})
|
||||
hermes_home = str(tmp_path / "hermes-home")
|
||||
|
||||
with patch("tools.browser_tool._find_agent_browser", return_value="npx agent-browser"), \
|
||||
patch("tools.browser_tool._resolve_npx_bin", return_value="/opt/hermes/node/bin/npx"), \
|
||||
patch("tools.browser_tool._chromium_installed", return_value=True), \
|
||||
patch("tools.browser_tool._get_session_info", return_value=fake_session), \
|
||||
patch("tools.browser_tool._socket_safe_tmpdir", return_value=str(tmp_path)), \
|
||||
patch("tools.browser_tool._discover_homebrew_node_dirs", return_value=[]), \
|
||||
patch("hermes_constants.Path.home", return_value=tmp_path), \
|
||||
patch("subprocess.Popen", side_effect=capture_popen), \
|
||||
patch("os.open", return_value=99), \
|
||||
patch("os.close"), \
|
||||
patch("tools.interrupt.is_interrupted", return_value=False), \
|
||||
patch.dict(
|
||||
os.environ,
|
||||
{
|
||||
"PATH": "/usr/bin:/bin",
|
||||
"HOME": "/home/test",
|
||||
"HERMES_HOME": hermes_home,
|
||||
},
|
||||
clear=True,
|
||||
):
|
||||
with patch("builtins.open", mock_open(read_data=fake_json)):
|
||||
_run_browser_command("test-task", "navigate", ["https://example.com"])
|
||||
|
||||
assert captured_cmd is not None
|
||||
assert captured_cmd[:4] == [
|
||||
"/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC,
|
||||
]
|
||||
assert captured_cmd[4:8] == ["--session", "test-session", "--json", "navigate"]
|
||||
|
||||
def test_subprocess_path_includes_termux_fallback_dirs(self, tmp_path):
|
||||
"""Termux fallback dirs should survive browser PATH rebuilding."""
|
||||
captured_env = {}
|
||||
@@ -397,3 +452,41 @@ class TestRunBrowserCommandPathConstruction:
|
||||
result_path = captured_env.get("PATH", "")
|
||||
assert "/data/data/com.termux/files/usr/bin" in result_path
|
||||
assert "/data/data/com.termux/files/usr/sbin" in result_path
|
||||
|
||||
|
||||
class TestRunChromeFallbackCommandNpxResolution:
|
||||
"""_run_chrome_fallback_command builds its own npx cmd prefix independently
|
||||
of _run_browser_command's — it must resolve npx the same way (via
|
||||
_resolve_npx_bin(), not a bare shutil.which("npx")) and use the pinned
|
||||
agent-browser npx spec."""
|
||||
|
||||
def test_npx_sentinel_resolves_via_resolve_npx_bin_with_pinned_spec(self, tmp_path):
|
||||
captured_cmds = []
|
||||
|
||||
mock_proc = MagicMock()
|
||||
mock_proc.returncode = 0
|
||||
mock_proc.wait.return_value = 0
|
||||
|
||||
def capture_popen(cmd, **kwargs):
|
||||
captured_cmds.append(cmd)
|
||||
return mock_proc
|
||||
|
||||
url_result = {"success": True, "data": {"result": "https://example.com"}}
|
||||
|
||||
with patch("tools.browser_tool._run_browser_command", return_value=url_result), \
|
||||
patch("tools.browser_tool._find_agent_browser", return_value="npx agent-browser"), \
|
||||
patch("tools.browser_tool._resolve_npx_bin", return_value="/opt/hermes/node/bin/npx"), \
|
||||
patch("tools.browser_tool._chromium_installed", return_value=True), \
|
||||
patch("tools.browser_tool._running_in_docker", return_value=False), \
|
||||
patch("tools.browser_tool._socket_safe_tmpdir", return_value=str(tmp_path)), \
|
||||
patch("subprocess.Popen", side_effect=capture_popen):
|
||||
_run_chrome_fallback_command("test-task", "navigate", ["https://example.com"], timeout=10)
|
||||
|
||||
assert captured_cmds, "expected at least one Popen call for the chrome-fallback session"
|
||||
first_cmd = captured_cmds[0]
|
||||
assert first_cmd[:4] == [
|
||||
"/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC,
|
||||
]
|
||||
assert first_cmd[4] == "--engine" and first_cmd[5] == "chrome"
|
||||
assert first_cmd[6] == "--session" and first_cmd[7].startswith("h_cfb_")
|
||||
assert first_cmd[8] == "--json"
|
||||
|
||||
Reference in New Issue
Block a user