diff --git a/tests/tools/test_browser_homebrew_paths.py b/tests/tools/test_browser_homebrew_paths.py new file mode 100644 index 000000000..3e2e76669 --- /dev/null +++ b/tests/tools/test_browser_homebrew_paths.py @@ -0,0 +1,259 @@ +"""Tests for macOS Homebrew PATH discovery in browser_tool.py.""" + +import json +import os +import subprocess +from pathlib import Path +from unittest.mock import patch, MagicMock, mock_open + +import pytest + +from tools.browser_tool import ( + _discover_homebrew_node_dirs, + _find_agent_browser, + _run_browser_command, + _SANE_PATH, +) + + +class TestSanePath: + """Verify _SANE_PATH includes Homebrew directories.""" + + def test_includes_homebrew_bin(self): + assert "/opt/homebrew/bin" in _SANE_PATH + + def test_includes_homebrew_sbin(self): + assert "/opt/homebrew/sbin" in _SANE_PATH + + def test_includes_standard_dirs(self): + assert "/usr/local/bin" in _SANE_PATH + assert "/usr/bin" in _SANE_PATH + assert "/bin" in _SANE_PATH + + +class TestDiscoverHomebrewNodeDirs: + """Tests for _discover_homebrew_node_dirs().""" + + def test_returns_empty_when_no_homebrew(self): + """Non-macOS systems without /opt/homebrew/opt should return empty.""" + with patch("os.path.isdir", return_value=False): + assert _discover_homebrew_node_dirs() == [] + + def test_finds_versioned_node_dirs(self): + """Should discover node@20/bin, node@24/bin etc.""" + entries = ["node@20", "node@24", "openssl", "node", "python@3.12"] + + def mock_isdir(p): + if p == "/opt/homebrew/opt": + return True + # node@20/bin and node@24/bin exist + if p in ( + "/opt/homebrew/opt/node@20/bin", + "/opt/homebrew/opt/node@24/bin", + ): + return True + return False + + with patch("os.path.isdir", side_effect=mock_isdir), \ + patch("os.listdir", return_value=entries): + result = _discover_homebrew_node_dirs() + + assert len(result) == 2 + assert "/opt/homebrew/opt/node@20/bin" in result + assert "/opt/homebrew/opt/node@24/bin" in result + + def test_excludes_plain_node(self): + """'node' (unversioned) should be excluded — covered by /opt/homebrew/bin.""" + with patch("os.path.isdir", return_value=True), \ + patch("os.listdir", return_value=["node"]): + result = _discover_homebrew_node_dirs() + assert result == [] + + def test_handles_oserror_gracefully(self): + """Should return empty list if listdir raises OSError.""" + with patch("os.path.isdir", return_value=True), \ + patch("os.listdir", side_effect=OSError("Permission denied")): + assert _discover_homebrew_node_dirs() == [] + + +class TestFindAgentBrowser: + """Tests for _find_agent_browser() Homebrew path search.""" + + def test_finds_in_current_path(self): + """Should return result from shutil.which if available on current PATH.""" + with patch("shutil.which", return_value="/usr/local/bin/agent-browser"): + assert _find_agent_browser() == "/usr/local/bin/agent-browser" + + def test_finds_in_homebrew_bin(self): + """Should search Homebrew dirs when not found on current PATH.""" + def mock_which(cmd, path=None): + if path and "/opt/homebrew/bin" in path and cmd == "agent-browser": + return "/opt/homebrew/bin/agent-browser" + return None + + with patch("shutil.which", side_effect=mock_which), \ + patch("os.path.isdir", return_value=True), \ + patch( + "tools.browser_tool._discover_homebrew_node_dirs", + return_value=[], + ): + result = _find_agent_browser() + assert result == "/opt/homebrew/bin/agent-browser" + + def test_finds_npx_in_homebrew(self): + """Should find npx in Homebrew paths as a fallback.""" + def mock_which(cmd, path=None): + if cmd == "agent-browser": + return None + if cmd == "npx": + if path and "/opt/homebrew/bin" in path: + return "/opt/homebrew/bin/npx" + return None + return None + + # Mock Path.exists() to prevent the local node_modules check from matching + 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) + + with patch("shutil.which", side_effect=mock_which), \ + patch("os.path.isdir", return_value=True), \ + 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 + + def mock_path_exists(self): + if "node_modules" in str(self) and "agent-browser" in str(self): + return False + return original_path_exists(self) + + with patch("shutil.which", return_value=None), \ + patch("os.path.isdir", return_value=False), \ + patch.object(Path, "exists", mock_path_exists), \ + patch( + "tools.browser_tool._discover_homebrew_node_dirs", + return_value=[], + ): + with pytest.raises(FileNotFoundError, match="agent-browser CLI not found"): + _find_agent_browser() + + +class TestRunBrowserCommandPathConstruction: + """Verify _run_browser_command() includes Homebrew node dirs in subprocess PATH.""" + + def test_subprocess_path_includes_homebrew_node_dirs(self, tmp_path): + """When _discover_homebrew_node_dirs returns dirs, they should appear + in the subprocess env PATH passed to Popen.""" + captured_env = {} + + # Create a mock Popen that captures the env dict + 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, + } + + # Write fake JSON output to the stdout temp file + fake_json = json.dumps({"success": True}) + stdout_file = tmp_path / "stdout" + stdout_file.write_text(fake_json) + + fake_homebrew_dirs = [ + "/opt/homebrew/opt/node@24/bin", + "/opt/homebrew/opt/node@20/bin", + ] + + # We need os.path.isdir to return True for our fake dirs + # but we also need real isdir for tmp_path operations + real_isdir = os.path.isdir + + def selective_isdir(p): + if p in fake_homebrew_dirs or p.startswith(str(tmp_path)): + return True + if "/opt/homebrew/" in p: + return True # _SANE_PATH dirs + return real_isdir(p) + + 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=fake_homebrew_dirs), \ + 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): + # The function reads from temp files for stdout/stderr + with patch("builtins.open", mock_open(read_data=fake_json)): + _run_browser_command("test-task", "navigate", ["https://example.com"]) + + # Verify Homebrew node dirs made it into the subprocess PATH + result_path = captured_env.get("PATH", "") + assert "/opt/homebrew/opt/node@24/bin" in result_path + assert "/opt/homebrew/opt/node@20/bin" in result_path + assert "/opt/homebrew/bin" in result_path # from _SANE_PATH + + def test_subprocess_path_includes_sane_path_homebrew(self, tmp_path): + """_SANE_PATH Homebrew entries should appear even without versioned node dirs.""" + 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(p): + if "/opt/homebrew/" in p: + return True + if p.startswith(str(tmp_path)): + return True + return real_isdir(p) + + 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 "/opt/homebrew/bin" in result_path + assert "/opt/homebrew/sbin" in result_path diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 94e3f97e0..b196cea78 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -288,3 +288,34 @@ class TestBlocklistCoverage: "DAYTONA_API_KEY", } assert extras.issubset(_HERMES_PROVIDER_ENV_BLOCKLIST) + + +class TestSanePathIncludesHomebrew: + """Verify _SANE_PATH includes macOS Homebrew directories.""" + + def test_sane_path_includes_homebrew_bin(self): + from tools.environments.local import _SANE_PATH + assert "/opt/homebrew/bin" in _SANE_PATH + + def test_sane_path_includes_homebrew_sbin(self): + from tools.environments.local import _SANE_PATH + assert "/opt/homebrew/sbin" in _SANE_PATH + + def test_make_run_env_appends_homebrew_on_minimal_path(self): + """When PATH is minimal (no /usr/bin), _make_run_env should append + _SANE_PATH which now includes Homebrew dirs.""" + from tools.environments.local import _make_run_env + minimal_env = {"PATH": "/some/custom/bin"} + with patch.dict(os.environ, minimal_env, clear=True): + result = _make_run_env({}) + assert "/opt/homebrew/bin" in result["PATH"] + assert "/opt/homebrew/sbin" in result["PATH"] + + def test_make_run_env_does_not_duplicate_on_full_path(self): + """When PATH already has /usr/bin, _make_run_env should not append.""" + from tools.environments.local import _make_run_env + full_env = {"PATH": "/usr/bin:/bin"} + with patch.dict(os.environ, full_env, clear=True): + result = _make_run_env({}) + # Should keep existing PATH unchanged + assert result["PATH"] == "/usr/bin:/bin" diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 5aea75f55..54780d45b 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -76,8 +76,35 @@ from tools.browser_providers.browser_use import BrowserUseProvider logger = logging.getLogger(__name__) -# Standard PATH entries for environments with minimal PATH (e.g. systemd services) -_SANE_PATH = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" +# 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" +) + + +def _discover_homebrew_node_dirs() -> list[str]: + """Find Homebrew versioned Node.js bin directories (e.g. node@20, node@24). + + When Node is installed via ``brew install node@24`` and NOT linked into + /opt/homebrew/bin, the binary lives only in /opt/homebrew/opt/node@24/bin/. + This function discovers those paths so they can be added to subprocess PATH. + """ + dirs: list[str] = [] + homebrew_opt = "/opt/homebrew/opt" + if not os.path.isdir(homebrew_opt): + return dirs + try: + for entry in os.listdir(homebrew_opt): + if entry.startswith("node") and entry != "node": + # e.g. node@20, node@24 + bin_dir = os.path.join(homebrew_opt, entry, "bin") + if os.path.isdir(bin_dir): + dirs.append(bin_dir) + except OSError: + pass + return dirs # Throttle screenshot cleanup to avoid repeated full directory scans. _last_screenshot_cleanup_by_dir: dict[str, float] = {} @@ -619,7 +646,8 @@ def _find_agent_browser() -> str: """ Find the agent-browser CLI executable. - Checks in order: PATH, local node_modules/.bin/, npx fallback. + Checks in order: current PATH, Homebrew/common bin dirs, Hermes-managed + node, local node_modules/.bin/, npx fallback. Returns: Path to agent-browser executable @@ -632,15 +660,36 @@ def _find_agent_browser() -> str: which_result = shutil.which("agent-browser") if which_result: 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 = Path(os.environ.get("HERMES_HOME", Path.home() / ".hermes")) + 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) + which_result = shutil.which("agent-browser", path=extended_path) + if which_result: + return which_result + # Check local node_modules/.bin/ (npm install in repo root) repo_root = Path(__file__).parent.parent local_bin = repo_root / "node_modules" / ".bin" / "agent-browser" if local_bin.exists(): return str(local_bin) - # Check common npx locations + # Check common npx locations (also search extended dirs) npx_path = shutil.which("npx") + if not npx_path and extra_dirs: + npx_path = shutil.which("npx", path=os.pathsep.join(extra_dirs)) if npx_path: return "npx agent-browser" @@ -742,13 +791,18 @@ def _run_browser_command( browser_env = {**os.environ} - # Ensure PATH includes Hermes-managed Node first, then standard system dirs. + # Ensure PATH includes Hermes-managed Node first, Homebrew versioned + # node dirs (for macOS ``brew install node@24``), then standard system dirs. hermes_home = Path(os.environ.get("HERMES_HOME", Path.home() / ".hermes")) 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] + [p for p in _SANE_PATH.split(":") if p] + candidate_dirs = ( + [hermes_node_bin] + + _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: diff --git a/tools/environments/local.py b/tools/environments/local.py index 914192f2d..0f913062a 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -254,7 +254,12 @@ def _clean_shell_noise(output: str) -> str: return result -_SANE_PATH = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" +# 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" +) def _make_run_env(env: dict) -> dict: