From 56c34ac4f73d145cef6eb3847855573a13e56588 Mon Sep 17 00:00:00 2001 From: adybag14-cyber <252811164+adybag14-cyber@users.noreply.github.com> Date: Tue, 14 Apr 2026 16:47:36 -0700 Subject: [PATCH] fix(browser): add termux PATH fallbacks Refactor browser tool PATH construction to include Termux directories (/data/data/com.termux/files/usr/bin, /data/data/com.termux/files/usr/sbin) so agent-browser and npx are discoverable on Android/Termux. Extracts _browser_candidate_path_dirs() and _merge_browser_path() helpers to centralize PATH construction shared between _find_agent_browser() and _run_browser_command(), replacing duplicated inline logic. Also fixes os.pathsep usage (was hardcoded ':') for cross-platform correctness. Cherry-picked from PR #9846. --- tests/tools/test_browser_homebrew_paths.py | 105 +++++++++++++++++++-- tools/browser_tool.py | 86 +++++++++-------- 2 files changed, 145 insertions(+), 46 deletions(-) diff --git a/tests/tools/test_browser_homebrew_paths.py b/tests/tools/test_browser_homebrew_paths.py index b54f4abb8..772a0b46b 100644 --- a/tests/tools/test_browser_homebrew_paths.py +++ b/tests/tools/test_browser_homebrew_paths.py @@ -31,18 +31,25 @@ def _clear_browser_caches(): class TestSanePath: - """Verify _SANE_PATH includes Homebrew directories.""" + """Verify _SANE_PATH includes fallback directories used by browser_tool.""" + + def test_includes_termux_bin(self): + assert "/data/data/com.termux/files/usr/bin" in _SANE_PATH.split(os.pathsep) + + def test_includes_termux_sbin(self): + assert "/data/data/com.termux/files/usr/sbin" in _SANE_PATH.split(os.pathsep) def test_includes_homebrew_bin(self): - assert "/opt/homebrew/bin" in _SANE_PATH + assert "/opt/homebrew/bin" in _SANE_PATH.split(os.pathsep) def test_includes_homebrew_sbin(self): - assert "/opt/homebrew/sbin" in _SANE_PATH + assert "/opt/homebrew/sbin" in _SANE_PATH.split(os.pathsep) def test_includes_standard_dirs(self): - assert "/usr/local/bin" in _SANE_PATH - assert "/usr/bin" in _SANE_PATH - assert "/bin" in _SANE_PATH + path_parts = _SANE_PATH.split(os.pathsep) + assert "/usr/local/bin" in path_parts + assert "/usr/bin" in path_parts + assert "/bin" in path_parts class TestDiscoverHomebrewNodeDirs: @@ -143,6 +150,44 @@ class TestFindAgentBrowser: result = _find_agent_browser() assert result == "npx agent-browser" + def test_finds_npx_in_termux_fallback_path(self): + """Should find npx when only Termux fallback dirs are available.""" + def mock_which(cmd, path=None): + if cmd == "agent-browser": + return None + if cmd == "npx": + if path and "/data/data/com.termux/files/usr/bin" in path: + return "/data/data/com.termux/files/usr/bin/npx" + return None + return None + + original_path_exists = Path.exists + + def mock_path_exists(self): + if "node_modules" in str(self) and "agent-browser" in str(self): + return False + return original_path_exists(self) + + real_isdir = os.path.isdir + + def selective_isdir(path): + if path in ( + "/data/data/com.termux/files/usr/bin", + "/data/data/com.termux/files/usr/sbin", + ): + return True + return real_isdir(path) + + with patch("shutil.which", side_effect=mock_which), \ + patch("os.path.isdir", side_effect=selective_isdir), \ + patch.object(Path, "exists", mock_path_exists), \ + patch( + "tools.browser_tool._discover_homebrew_node_dirs", + return_value=[], + ): + result = _find_agent_browser() + assert result == "npx agent-browser" + def test_raises_when_not_found(self): """Should raise FileNotFoundError when nothing works.""" original_path_exists = Path.exists @@ -399,3 +444,51 @@ class TestRunBrowserCommandPathConstruction: result_path = captured_env.get("PATH", "") assert "/opt/homebrew/bin" in result_path assert "/opt/homebrew/sbin" in result_path + + def test_subprocess_path_includes_termux_fallback_dirs(self, tmp_path): + """Termux fallback dirs should survive browser PATH rebuilding.""" + captured_env = {} + + mock_proc = MagicMock() + mock_proc.returncode = 0 + mock_proc.wait.return_value = 0 + + def capture_popen(cmd, **kwargs): + captured_env.update(kwargs.get("env", {})) + return mock_proc + + fake_session = { + "session_name": "test-session", + "session_id": "test-id", + "cdp_url": None, + } + + fake_json = json.dumps({"success": True}) + real_isdir = os.path.isdir + + def selective_isdir(path): + if path in ( + "/data/data/com.termux/files/usr/bin", + "/data/data/com.termux/files/usr/sbin", + ): + return True + if path.startswith(str(tmp_path)): + return True + return real_isdir(path) + + with patch("tools.browser_tool._find_agent_browser", return_value="/usr/local/bin/agent-browser"), \ + 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("os.path.isdir", side_effect=selective_isdir), \ + 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"}, clear=True): + with patch("builtins.open", mock_open(read_data=fake_json)): + _run_browser_command("test-task", "navigate", ["https://example.com"]) + + 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 diff --git a/tools/browser_tool.py b/tools/browser_tool.py index fd6562575..03be84e02 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -94,11 +94,21 @@ except ImportError: logger = logging.getLogger(__name__) # Standard PATH entries for environments with minimal PATH (e.g. systemd services). -# Includes macOS Homebrew paths (/opt/homebrew/* for Apple Silicon). -_SANE_PATH = ( - "/opt/homebrew/bin:/opt/homebrew/sbin:" - "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" +# Includes Android/Termux and macOS Homebrew locations needed for agent-browser, +# npx, node, and Android's glibc runner (grun). +_SANE_PATH_DIRS = ( + "/data/data/com.termux/files/usr/bin", + "/data/data/com.termux/files/usr/sbin", + "/opt/homebrew/bin", + "/opt/homebrew/sbin", + "/usr/local/sbin", + "/usr/local/bin", + "/usr/sbin", + "/usr/bin", + "/sbin", + "/bin", ) +_SANE_PATH = os.pathsep.join(_SANE_PATH_DIRS) @functools.lru_cache(maxsize=1) @@ -123,6 +133,28 @@ def _discover_homebrew_node_dirs() -> tuple[str, ...]: pass return tuple(dirs) + +def _browser_candidate_path_dirs() -> list[str]: + """Return ordered browser CLI PATH candidates shared by discovery and execution.""" + hermes_home = get_hermes_home() + hermes_node_bin = str(hermes_home / "node" / "bin") + return [hermes_node_bin, *list(_discover_homebrew_node_dirs()), *_SANE_PATH_DIRS] + + +def _merge_browser_path(existing_path: str = "") -> str: + """Prepend browser-specific PATH fallbacks without reordering existing entries.""" + path_parts = [p for p in (existing_path or "").split(os.pathsep) if p] + existing_parts = set(path_parts) + prefix_parts: list[str] = [] + + for part in _browser_candidate_path_dirs(): + if not part or part in existing_parts or part in prefix_parts: + continue + if os.path.isdir(part): + prefix_parts.append(part) + + return os.pathsep.join(prefix_parts + path_parts) + # Throttle screenshot cleanup to avoid repeated full directory scans. _last_screenshot_cleanup_by_dir: dict[str, float] = {} @@ -895,21 +927,10 @@ def _find_agent_browser() -> str: _agent_browser_resolved = True return which_result - # Build an extended search PATH including Homebrew and Hermes-managed dirs. - # This covers macOS where the process PATH may not include Homebrew paths. - extra_dirs: list[str] = [] - for d in ["/opt/homebrew/bin", "/usr/local/bin"]: - if os.path.isdir(d): - extra_dirs.append(d) - extra_dirs.extend(_discover_homebrew_node_dirs()) - - hermes_home = get_hermes_home() - hermes_node_bin = str(hermes_home / "node" / "bin") - if os.path.isdir(hermes_node_bin): - extra_dirs.append(hermes_node_bin) - - if extra_dirs: - extended_path = os.pathsep.join(extra_dirs) + # Build an extended search PATH including Hermes-managed Node, macOS + # versioned Homebrew installs, and fallback system dirs like Termux. + extended_path = _merge_browser_path("") + if extended_path: which_result = shutil.which("agent-browser", path=extended_path) if which_result: _cached_agent_browser = which_result @@ -924,10 +945,10 @@ def _find_agent_browser() -> str: _agent_browser_resolved = True return _cached_agent_browser - # Check common npx locations (also search extended dirs) + # Check common npx locations (also search the extended fallback PATH) npx_path = shutil.which("npx") - if not npx_path and extra_dirs: - npx_path = shutil.which("npx", path=os.pathsep.join(extra_dirs)) + if not npx_path and extended_path: + npx_path = shutil.which("npx", path=extended_path) if npx_path: _cached_agent_browser = "npx agent-browser" _agent_browser_resolved = True @@ -1046,24 +1067,9 @@ def _run_browser_command( browser_env = {**os.environ} - # Ensure PATH includes Hermes-managed Node first, Homebrew versioned - # node dirs (for macOS ``brew install node@24``), then standard system dirs. - hermes_home = get_hermes_home() - hermes_node_bin = str(hermes_home / "node" / "bin") - - existing_path = browser_env.get("PATH", "") - path_parts = [p for p in existing_path.split(":") if p] - candidate_dirs = ( - [hermes_node_bin] - + list(_discover_homebrew_node_dirs()) - + [p for p in _SANE_PATH.split(":") if p] - ) - - for part in reversed(candidate_dirs): - if os.path.isdir(part) and part not in path_parts: - path_parts.insert(0, part) - - browser_env["PATH"] = ":".join(path_parts) + # Ensure subprocesses inherit the same browser-specific PATH fallbacks + # used during CLI discovery. + browser_env["PATH"] = _merge_browser_path(browser_env.get("PATH", "")) browser_env["AGENT_BROWSER_SOCKET_DIR"] = task_socket_dir # Use temp files for stdout/stderr instead of pipes.