From 5988fe6cd5547d3620df1de889ac6007f5463b4d Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 18 Jul 2026 09:34:11 -0700 Subject: [PATCH] fix: widen headed-mode gate to config, add browser.headed default, tests, docs Follow-up to @vishnukool's #24064 salvage: - cleanup skip now uses _is_headed_mode() (config browser.headed OR AGENT_BROWSER_HEADED env) instead of env-var-only, with env fallback if browser_tool import fails - browser.headed added to DEFAULT_CONFIG (default false) - 14 regression tests: resolution precedence, cleanup skip, --headed argv injection (local vs cloud), VM cleanup unaffected - docs: Headed Mode section in browser.md --- agent/chat_completion_helpers.py | 8 +- hermes_cli/config.py | 1 + tests/tools/test_browser_headed_mode.py | 248 ++++++++++++++++++++ website/docs/user-guide/features/browser.md | 18 ++ 4 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 tests/tools/test_browser_headed_mode.py diff --git a/agent/chat_completion_helpers.py b/agent/chat_completion_helpers.py index 3bfe49781bf8..2912628b9882 100644 --- a/agent/chat_completion_helpers.py +++ b/agent/chat_completion_helpers.py @@ -2144,7 +2144,13 @@ def cleanup_task_resources(agent, task_id: str) -> None: if agent.verbose_logging: logger.warning(f"Failed to cleanup VM for task {task_id}: {e}") try: - if os.environ.get("AGENT_BROWSER_HEADED"): + headed = False + try: + from tools.browser_tool import _is_headed_mode + headed = _is_headed_mode() + except Exception: + headed = bool(os.environ.get("AGENT_BROWSER_HEADED")) + if headed: if agent.verbose_logging: logging.debug( f"Skipping per-turn cleanup_browser for headed session {task_id}; " diff --git a/hermes_cli/config.py b/hermes_cli/config.py index f7d83e09267c..55ad02afd3ee 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1304,6 +1304,7 @@ DEFAULT_CONFIG = { "inactivity_timeout": 120, "command_timeout": 30, # Timeout for browser commands in seconds (screenshot, navigate, etc.) "record_sessions": False, # Auto-record browser sessions as WebM videos + "headed": False, # Local mode: launch Chromium with a visible window (also skips per-turn cleanup so the window persists between turns; idle reaper still applies) "allow_private_urls": False, # Allow navigating to private/internal IPs (localhost, 192.168.x.x, etc.) # Browser engine for local mode. Passed as ``--engine `` to # agent-browser v0.25.3+. diff --git a/tests/tools/test_browser_headed_mode.py b/tests/tools/test_browser_headed_mode.py new file mode 100644 index 000000000000..a948463734f2 --- /dev/null +++ b/tests/tools/test_browser_headed_mode.py @@ -0,0 +1,248 @@ +"""Tests for headed browser mode: config/env resolution, --headed injection, +and the per-turn cleanup skip that keeps headed sessions alive between turns. + +Salvaged from PR #24064 (fixes #11020 lead bug). +""" + +import os +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + + +def _reset_headed_cache(): + """Reset the module-level headed-mode cache so tests start clean.""" + import tools.browser_tool as bt + bt._cached_headed_mode = None + bt._headed_mode_resolved = False + + +@pytest.fixture(autouse=True) +def _clean_headed_cache(): + _reset_headed_cache() + yield + _reset_headed_cache() + + +# --------------------------------------------------------------------------- +# _is_headed_mode resolution +# --------------------------------------------------------------------------- + +class TestIsHeadedMode: + def test_default_is_false(self): + from tools.browser_tool import _is_headed_mode + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("AGENT_BROWSER_HEADED", None) + with patch("hermes_cli.config.read_raw_config", return_value={}): + assert _is_headed_mode() is False + + def test_config_true(self): + from tools.browser_tool import _is_headed_mode + cfg = {"browser": {"headed": True}} + with patch("hermes_cli.config.read_raw_config", return_value=cfg): + assert _is_headed_mode() is True + + def test_config_string_true(self): + from tools.browser_tool import _is_headed_mode + cfg = {"browser": {"headed": "true"}} + with patch("hermes_cli.config.read_raw_config", return_value=cfg): + assert _is_headed_mode() is True + + def test_config_false_beats_missing_env(self): + from tools.browser_tool import _is_headed_mode + cfg = {"browser": {"headed": False}} + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("AGENT_BROWSER_HEADED", None) + with patch("hermes_cli.config.read_raw_config", return_value=cfg): + assert _is_headed_mode() is False + + def test_env_var_fallback(self): + from tools.browser_tool import _is_headed_mode + with patch.dict(os.environ, {"AGENT_BROWSER_HEADED": "1"}): + with patch("hermes_cli.config.read_raw_config", return_value={}): + assert _is_headed_mode() is True + + def test_env_var_garbage_is_false(self): + from tools.browser_tool import _is_headed_mode + with patch.dict(os.environ, {"AGENT_BROWSER_HEADED": "banana"}): + with patch("hermes_cli.config.read_raw_config", return_value={}): + assert _is_headed_mode() is False + + def test_caching(self): + from tools.browser_tool import _is_headed_mode + cfg = {"browser": {"headed": True}} + with patch("hermes_cli.config.read_raw_config", return_value=cfg) as mock_read: + assert _is_headed_mode() is True + assert _is_headed_mode() is True + assert mock_read.call_count == 1 + + +# --------------------------------------------------------------------------- +# Per-turn cleanup skip (agent/chat_completion_helpers.cleanup_task_resources) +# --------------------------------------------------------------------------- + +def _make_agent(verbose=False): + return SimpleNamespace(verbose_logging=verbose) + + +class TestCleanupTaskResourcesHeadedSkip: + def test_headless_still_cleans_browser(self): + from agent.chat_completion_helpers import cleanup_task_resources + with ( + patch("tools.browser_tool._is_headed_mode", return_value=False), + patch("run_agent.cleanup_vm"), + patch("run_agent.cleanup_browser") as mock_cb, + patch( + "agent.chat_completion_helpers.is_persistent_env", + return_value=False, + ), + ): + cleanup_task_resources(_make_agent(), "task-x") + mock_cb.assert_called_once_with("task-x") + + def test_headed_skips_browser_cleanup(self): + from agent.chat_completion_helpers import cleanup_task_resources + with ( + patch("tools.browser_tool._is_headed_mode", return_value=True), + patch("run_agent.cleanup_vm"), + patch("run_agent.cleanup_browser") as mock_cb, + patch( + "agent.chat_completion_helpers.is_persistent_env", + return_value=False, + ), + ): + cleanup_task_resources(_make_agent(), "task-x") + mock_cb.assert_not_called() + + def test_headed_env_var_fallback_when_import_fails(self): + """If browser_tool import blows up, the env var still gates the skip.""" + from agent.chat_completion_helpers import cleanup_task_resources + with ( + patch( + "tools.browser_tool._is_headed_mode", + side_effect=RuntimeError("boom"), + ), + patch.dict(os.environ, {"AGENT_BROWSER_HEADED": "1"}), + patch("run_agent.cleanup_vm"), + patch("run_agent.cleanup_browser") as mock_cb, + patch( + "agent.chat_completion_helpers.is_persistent_env", + return_value=False, + ), + ): + cleanup_task_resources(_make_agent(), "task-x") + mock_cb.assert_not_called() + + def test_headed_does_not_skip_vm_cleanup(self): + """Headed mode only affects the browser; VM teardown is untouched.""" + from agent.chat_completion_helpers import cleanup_task_resources + with ( + patch("tools.browser_tool._is_headed_mode", return_value=True), + patch("run_agent.cleanup_vm") as mock_vm, + patch("run_agent.cleanup_browser"), + patch( + "agent.chat_completion_helpers.is_persistent_env", + return_value=False, + ), + ): + cleanup_task_resources(_make_agent(), "task-x") + mock_vm.assert_called_once_with("task-x") + + +# --------------------------------------------------------------------------- +# --headed flag injection in local mode +# --------------------------------------------------------------------------- + +class TestHeadedFlagInjection: + def _run_and_capture(self, bt): + """Run a snapshot command with Popen mocked; return captured argv.""" + captured_cmds = [] + mock_proc = MagicMock() + mock_proc.wait.return_value = None + mock_proc.returncode = 0 + + def capture_popen(cmd, **kwargs): + captured_cmds.append(cmd) + return mock_proc + + mock_stdout = ( + '{"success": true, "data": {"snapshot": ' + '"- heading \\"Hi\\" [ref=e1]", "refs": {"e1": {}}}}' + ) + with patch("subprocess.Popen", side_effect=capture_popen), \ + patch("os.open", return_value=99), \ + patch("os.close"), \ + patch("os.unlink"), \ + patch("os.makedirs"), \ + patch("builtins.open", MagicMock(return_value=MagicMock( + __enter__=MagicMock(return_value=MagicMock( + read=MagicMock(return_value=mock_stdout))), + __exit__=MagicMock(return_value=False), + ))), \ + patch("tools.interrupt.is_interrupted", return_value=False), \ + patch("tools.browser_tool._write_owner_pid"): + bt._run_browser_command("task1", "snapshot", [], _engine_override="auto") + return captured_cmds + + @patch("tools.browser_tool._get_session_info") + @patch("tools.browser_tool._find_agent_browser", return_value="/usr/bin/agent-browser") + @patch("tools.browser_tool._is_local_mode", return_value=True) + @patch("tools.browser_tool._chromium_installed", return_value=True) + @patch("tools.browser_tool._get_cloud_provider", return_value=None) + @patch("tools.browser_tool._get_cdp_override", return_value="") + @patch("tools.browser_tool._is_camofox_mode", return_value=False) + def test_headed_flag_added_in_local_mode( + self, _camofox, _cdp, _cloud, _chromium, _local, _find, _session + ): + import tools.browser_tool as bt + bt._cached_headed_mode = True + bt._headed_mode_resolved = True + _session.return_value = {"session_name": "test-sess"} + + captured = self._run_and_capture(bt) + assert len(captured) == 1 + assert "--headed" in captured[0] + + @patch("tools.browser_tool._get_session_info") + @patch("tools.browser_tool._find_agent_browser", return_value="/usr/bin/agent-browser") + @patch("tools.browser_tool._is_local_mode", return_value=True) + @patch("tools.browser_tool._chromium_installed", return_value=True) + @patch("tools.browser_tool._get_cloud_provider", return_value=None) + @patch("tools.browser_tool._get_cdp_override", return_value="") + @patch("tools.browser_tool._is_camofox_mode", return_value=False) + def test_headed_flag_not_added_when_headless( + self, _camofox, _cdp, _cloud, _chromium, _local, _find, _session + ): + import tools.browser_tool as bt + bt._cached_headed_mode = False + bt._headed_mode_resolved = True + _session.return_value = {"session_name": "test-sess"} + + captured = self._run_and_capture(bt) + assert len(captured) == 1 + assert "--headed" not in captured[0] + + @patch("tools.browser_tool._get_session_info") + @patch("tools.browser_tool._find_agent_browser", return_value="/usr/bin/agent-browser") + @patch("tools.browser_tool._is_local_mode", return_value=True) + @patch("tools.browser_tool._chromium_installed", return_value=True) + @patch("tools.browser_tool._get_cloud_provider", return_value=None) + @patch("tools.browser_tool._get_cdp_override", return_value="") + @patch("tools.browser_tool._is_camofox_mode", return_value=False) + def test_headed_flag_not_added_in_cloud_mode( + self, _camofox, _cdp, _cloud, _chromium, _local, _find, _session + ): + """Cloud (CDP) sessions never get --headed — it's a local-only flag.""" + import tools.browser_tool as bt + bt._cached_headed_mode = True + bt._headed_mode_resolved = True + _session.return_value = { + "session_name": "test-sess", + "cdp_url": "wss://example.invalid/cdp", + } + + captured = self._run_and_capture(bt) + assert len(captured) == 1 + assert "--headed" not in captured[0] + assert "--cdp" in captured[0] diff --git a/website/docs/user-guide/features/browser.md b/website/docs/user-guide/features/browser.md index 0384a2e03bb2..167abcd6641d 100644 --- a/website/docs/user-guide/features/browser.md +++ b/website/docs/user-guide/features/browser.md @@ -631,6 +631,24 @@ browser: When enabled, recording starts automatically on the first `browser_navigate` and saves to `~/.hermes/browser_recordings/` when the session closes. Works in both local and cloud (Browserbase) modes. Recordings older than 72 hours are automatically cleaned up. +## Headed Mode (Visible Browser Window) + +By default, the local browser runs headless. Enable headed mode to get a visible Chromium window you can watch and interact with: + +```yaml +browser: + headed: true # default: false +``` + +Or via environment variable: `AGENT_BROWSER_HEADED=1`. + +Headed mode does two things: + +1. **Launches Chromium with a visible window** (passes `--headed` to agent-browser in local mode). +2. **Keeps the window open between turns.** Normally the browser session is cleaned up after every agent reply; in headed mode the per-turn cleanup is skipped so you can watch the agent work, intervene manually (sign-in challenges, CAPTCHAs), and keep login state warm across the conversation. + +Idle sessions are still reaped after `browser.inactivity_timeout` (default 120s of no browser activity), and all sessions are closed on shutdown. Headed mode only affects the local browser — cloud sessions (Browserbase) are unaffected. + ## Stealth Features Browserbase provides automatic stealth capabilities: