mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
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.
This commit is contained in:
adybag14-cyber
Teknium
parent
3ca7417c2a
commit
56c34ac4f7
2 changed files with 145 additions and 46 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue