diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 41469bb8d4..a61f7b329a 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -200,11 +200,9 @@ def _browser_cfg(key: str, default, parse, log_label: str): def _cached_browser_cfg(cache_name: str, flag_name: str, key: str, default, parse, log_label: str): - """Process-cached ``_browser_cfg`` read (cache cleared by ``cleanup_all_browsers``). - - The value is stored BEFORE the resolved flag flips so a concurrent reader can - never observe ``resolved=True`` with a ``None`` cache. - """ + """Process-cached ``_browser_cfg`` read (cleared by ``cleanup_all_browsers``). The value is + stored BEFORE the resolved flag flips so a concurrent reader never sees ``resolved=True`` + with a ``None`` cache.""" g = globals() if g[flag_name] and g[cache_name] is not None: return g[cache_name] @@ -315,11 +313,8 @@ _PRIVATE_HOST_SUFFIXES = (".localhost", ".local", ".lan", ".internal") def _url_is_private(url: str) -> bool: """True when the URL's host is (or resolves to) a private/LAN/loopback/CGNAT address. - - Routing oracle only: DNS failures are NOT private (fall through to the - configured backend, which surfaces the DNS error). Obvious names short-circuit - the DNS hop; bare ``localhost`` resolves via /etc/hosts otherwise. - """ + Routing oracle only: DNS failures are NOT private (the configured backend surfaces the + error); obvious names short-circuit the DNS hop.""" import ipaddress import socket from urllib.parse import urlparse @@ -350,13 +345,10 @@ def _url_is_private(url: str) -> bool: def _navigation_session_key(task_id: str, url: str) -> str: - """Session key that should handle ``url`` for ``task_id``. - - ``f"{task_id}::local"`` (hybrid routing: local Chromium sidecar while the cloud - session keeps serving public URLs) only when ALL hold: cloud provider - configured, ``browser.auto_local_for_private_urls`` on, private URL, no CDP - override (it owns the whole session), Camofox off (already local-only). - """ + """Session key that should handle ``url`` for ``task_id``: ``f"{task_id}::local"`` (hybrid + local sidecar while the cloud session keeps serving public URLs) only when ALL hold — + cloud provider configured, ``browser.auto_local_for_private_urls`` on, private URL, no + CDP override (it owns the whole session), Camofox off (already local-only).""" if task_id is None: task_id = "default" hybrid = ( @@ -387,10 +379,8 @@ def _session_info_owned_by_task(session_info: Dict[str, Any], task_id: str, sess def _last_session_key(task_id: str) -> str: """Session key a non-nav tool must use: the one that served the task's last navigation. - - If that session was cleaned up or its ownership no longer matches, fail closed by - dropping the stale binding rather than recreating or mutating the wrong browser. - """ + If it was cleaned up or ownership no longer matches, fail closed by dropping the stale + binding rather than recreating or mutating the wrong browser.""" if task_id is None: task_id = "default" recorded_key = _last_active_session_key.get(task_id) @@ -686,15 +676,11 @@ def _secret_url_error(url: str) -> Optional[dict]: def _url_policy_error(url: str, *, auto_local: bool = False) -> Optional[dict]: - """Backend-aware URL checks on an already-normalized URL; None if allowed. - - Ordered floors: (1) credential-like query params refused for cloud backends - (third-party readers), allowed for local and the hybrid sidecar; (2) cloud - metadata / IMDS refused UNCONDITIONALLY for every backend (a local Chromium on - a cloud VM still reaches the host IMDS); (3) private addresses refused unless - local, auto-routed to the sidecar, or ``browser.allow_private_urls``; - (4) website policy allow/deny lists. - """ + """Backend-aware URL checks on an already-normalized URL; None if allowed. Ordered floors: + (1) credential-like query params refused for cloud backends (third-party readers); + (2) cloud metadata / IMDS refused UNCONDITIONALLY (a local Chromium on a cloud VM still + reaches the host IMDS); (3) private addresses refused unless local, sidecar-routed, or + ``browser.allow_private_urls``; (4) website policy allow/deny lists.""" local = _is_local_backend() sensitive_query_key = _sensitive_query_param_name(url) if sensitive_query_key and not local and not auto_local: @@ -737,13 +723,10 @@ _BOT_DETECTION_TITLE_PATTERNS = ( def _post_redirect_block(nav_session_key: str, url: str, final_url: str, auto_local_this_nav: bool) -> Optional[str]: - """Post-redirect SSRF check; blocked JSON payload or None. - - A redirect onto a private/internal address would let later snapshots read - internal content, so the page is navigated to about:blank first. The metadata - floor fires for every backend; the private-address check is skipped for local - backends, the hybrid sidecar, and ``browser.allow_private_urls``. - """ + """Post-redirect SSRF check; blocked JSON payload or None. The page is moved to about:blank + first so later snapshots can't read the internal content. The metadata floor fires for + every backend; the private-address check is skipped for local, the sidecar, and + ``browser.allow_private_urls``.""" if not final_url or final_url == url: return None if _is_always_blocked_url(final_url): @@ -762,11 +745,8 @@ def _post_redirect_block(nav_session_key: str, url: str, final_url: str, auto_lo def _snapshot_fields(snap_result: Dict[str, Any]) -> Dict[str, Any]: - """``snapshot`` + ``element_count`` response fields from a successful snapshot result. - - Oversized snapshots truncate at line boundaries; the full tree is stored to - cache/web with a read_file paging note (same pattern as web_extract — no LLM). - """ + """``snapshot`` + ``element_count`` fields from a successful snapshot result; oversized + snapshots truncate at line boundaries with the full tree stored for read_file paging.""" data = snap_result.get("data", {}) snapshot_text = data.get("snapshot", "") refs = data.get("refs", {}) @@ -795,11 +775,8 @@ def _attach_auto_snapshot(response: Dict[str, Any], nav_session_key: str) -> Non def browser_navigate(url: str, task_id: Optional[str] = None) -> str: """Navigate to ``url``; JSON with title, compact snapshot and, on first nav, stealth features. - - Hybrid routing decides BEFORE the safety checks whether this URL goes to a local - Chromium sidecar; the cloud provider never sees the URL then, so the - private-address checks are relaxed for it. - """ + Hybrid routing decides BEFORE the safety checks whether this URL goes to a local sidecar + (the cloud provider never sees it then, so the private-address checks are relaxed).""" url, safety_error = _secret_url_error_normalized(url) if safety_error is not None: return json.dumps(safety_error) @@ -1103,12 +1080,9 @@ def _eval_result_or_blocked(effective_task_id: str, parsed: Any, result: Dict[st def _eval_supervisor_fast_path(effective_task_id: str, expression: str) -> Optional[str]: """``Runtime.evaluate`` on the CDP supervisor's persistent WebSocket (no subprocess cost). - - Returns tool JSON when the supervisor gave a definitive answer (a value, a - blocked private page, or a real JS-side exception — NOT retried through the - subprocess, that would just reproduce it slower), or None to fall through to the - subprocess path (no supervisor, supervisor-side failure, import error). - """ + Tool JSON when the supervisor gave a definitive answer (value, blocked page, or a real + JS-side exception — NOT retried via subprocess, that would just reproduce it slower); + None to fall through to the subprocess path.""" try: from tools.browser_supervisor import SUPERVISOR_REGISTRY # type: ignore[import-not-found] supervisor = SUPERVISOR_REGISTRY.get(effective_task_id) @@ -1147,12 +1121,9 @@ def _eval_failure_response(result: Dict[str, Any]) -> str: def _browser_eval(expression: str, task_id: Optional[str] = None) -> str: - """Evaluate a JavaScript expression in the page context and return the result. - - Private-network guard, both halves gated on the same condition: the literal - pre-scan closes direct fetches (``fetch('http://127.0.0.1/...')`` never updates - ``location.href``); the post-eval page-URL recheck closes navigate-then-read. - """ + """Evaluate JS in the page context. Private-network guard in two halves: the literal + pre-scan closes direct fetches (they never update ``location.href``); the post-eval + page-URL recheck closes navigate-then-read.""" effective_task_id = _last_session_key(task_id or "default") if _eval_ssrf_guard_active(effective_task_id): @@ -1309,13 +1280,9 @@ def _capture_vision_screenshot(effective_task_id: str, annotate: bool, screensho def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] = None) -> Union[str, Dict[str, Any]]: - """Screenshot the current page for visual inspection (CAPTCHAs, images, layouts). - - Native-vision models get the screenshot attached to the conversation (multimodal - tool-result envelope); otherwise the auxiliary vision model returns a text - analysis. The file is saved persistently and its path returned (MEDIA:). - ``annotate`` overlays numbered [N] labels on interactive elements. - """ + """Screenshot the current page for visual inspection. Native-vision models get the image + attached to the conversation; otherwise the auxiliary vision model returns a text + analysis. The file is kept and its path returned (MEDIA:).""" if _is_camofox_mode(): return _camofox("camofox_vision", question, annotate, task_id) @@ -1324,14 +1291,12 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] screenshots_dir = get_hermes_dir("cache/screenshots", "browser_screenshots") screenshot_path = screenshots_dir / f"browser_screenshot_{uuid_mod.uuid4().hex}.png" effective_task_id = _last_session_key(task_id or "default") - blocked = _blocked_private_page_content(effective_task_id) if blocked is not None: return blocked _lp_prerouted, _lp_fallback_warning, screenshot_path = _lightpanda_vision_preroute( - effective_task_id, annotate, screenshot_path, - ) + effective_task_id, annotate, screenshot_path) result: Dict[str, Any] = {} try: screenshots_dir.mkdir(parents=True, exist_ok=True) @@ -1340,11 +1305,9 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] effective_task_id, annotate, screenshot_path, _lp_prerouted) if error is not None: return error - - # Native image routing for the active main model: attach the screenshot - # directly instead of describing it through an aux vision LLM (no information loss). + # Native image routing: attach the screenshot directly instead of describing it + # through an aux vision LLM (no information loss). from tools.vision_tools import _should_use_native_vision_fast_path - if _should_use_native_vision_fast_path(): return _native_vision_result(screenshot_path, question, annotate, result, _lp_fallback_warning) @@ -1355,10 +1318,9 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] if annotate and result.get("data", {}).get("annotations"): response_data["annotations"] = result["data"]["annotations"] return _dumps(response_data) - except Exception as e: - # Keep a captured screenshot — the failure is in the analysis, not the - # capture, and deleting it loses evidence. The 24-hour cleanup bounds disk growth. + # Keep a captured screenshot — the failure is in the analysis, not the capture, + # and deleting it loses evidence. The 24-hour cleanup bounds disk growth. logger.warning("browser_vision failed: %s", e, exc_info=True) error_info = _err(f"Error during vision analysis: {str(e)}") if screenshot_path.exists(): diff --git a/tools/browser_tool_lifecycle.py b/tools/browser_tool_lifecycle.py index dfb434c933..396e951982 100644 --- a/tools/browser_tool_lifecycle.py +++ b/tools/browser_tool_lifecycle.py @@ -1,9 +1,7 @@ -"""Browser session lifecycle: inactivity janitor, orphan reaper, per-session teardown, atexit emergency cleanup. +"""Browser session lifecycle: inactivity janitor, orphan reaper, per-session teardown, atexit cleanup. -Split out of ``tools/browser_tool.py``; every name is re-imported there. Origin -symbols and module state are read/written through ``_bt`` (the origin module, -the :data:`tools.browser_tool_origin.origin` proxy) so -``patch("tools.browser_tool.X")`` is honoured and no import cycle exists. +Split out of ``tools/browser_tool.py`` (every name re-imported there); origin symbols +are read through the ``_bt`` proxy so ``patch("tools.browser_tool.X")`` is honoured. """ import contextlib @@ -189,14 +187,12 @@ def _write_owner_pid(socket_dir: str, session_name: str) -> None: def _verify_reapable_browser_daemon(daemon_pid: int, socket_dir: str, session_name: str) -> bool: - """Confirm a live PID is genuinely *this* session's agent-browser daemon. + """Confirm a live PID is genuinely *this* session's agent-browser daemon (fail-closed). - The ``.pid`` file lives in a world-writable temp dir and is written by the - daemon: a same-user actor can plant one pointing at a victim PID, or a recycled - PID can land on an unrelated process — and reaping is a *tree* kill. Two - checks must pass: (1) identity — ``agent-browser`` in name or cmdline; - (2) binding — the socket dir in the cmdline or ``AGENT_BROWSER_SOCKET_DIR`` in - its environ (the real spoof defense). Fail-closed on any ambiguity. + The ``.pid`` file sits in a world-writable temp dir: a planted or recycled PID + would turn the tree-kill into an arbitrary-process DoS. Both must pass: + (1) identity — ``agent-browser`` in name/cmdline; (2) binding — the socket dir in + the cmdline or ``AGENT_BROWSER_SOCKET_DIR`` in its environ (the real spoof defense). """ def refuse(reason: str, *args) -> bool: _bt.logger.warning("Refusing to reap browser daemon PID %d (session %s): " + reason, @@ -236,11 +232,8 @@ def _verify_reapable_browser_daemon(daemon_pid: int, socket_dir: str, def _socket_dir_idle_seconds(socket_dir: str) -> Optional[float]: """Seconds since anything in ``socket_dir`` was last written; None if unknown (fail safe). - - Every command writes ``_stdout_`` there, so the newest mtime is a - last-activity marker surviving hermes restarts. The dir's own mtime is not - enough — rewriting an existing file doesn't touch it — so entries are scanned. - """ + Every command rewrites ``_stdout_`` there — a restart-proof activity marker — and + rewriting doesn't touch the dir mtime, so entries are scanned too.""" try: latest = os.path.getmtime(socket_dir) except OSError: @@ -295,13 +288,11 @@ def _terminate_verified_daemon(daemon_pid: int, session_name: str, log) -> bool: def _reap_socket_dir(socket_dir: str, session_name: str, tracked_names: set) -> bool: """Reap one ``agent-browser-`` dir if orphaned; True when a daemon was killed. - Ownership priority: (1) a live ``owner_pid`` means another hermes process owns - it — leave it alone UNLESS it is untracked here and idle past - ``BROWSER_ORPHAN_GRACE_SECONDS`` (owner-alive alone made leaked daemons - immortal); (2) no owner_pid (legacy) falls back to this process's tracking. - A pidless dir is only stale after the grace period — deleting it immediately - races the creator's first stdout open. The daemon PID is identity-verified - before a tree-kill and refused without a start-time fingerprint. + A live ``owner_pid`` means another hermes process owns it — leave it UNLESS untracked + here and idle past ``BROWSER_ORPHAN_GRACE_SECONDS`` (owner-alive alone made leaked + daemons immortal); no owner_pid (legacy) falls back to this process's tracking. A + pidless dir is only stale after the grace period (deleting it immediately races the + creator's first stdout open). The PID is identity-verified before any tree-kill. """ owner_pid, owner_alive = _bt._owner_pid_alive(socket_dir, session_name) if owner_alive is True: @@ -349,13 +340,9 @@ def _reap_socket_dir(socket_dir: str, session_name: str, tracked_names: set) -> def _reap_orphaned_browser_sessions(): - """Kill agent-browser daemons whose owning hermes process is gone. - - An unclean exit (SIGKILL, crash, gateway restart) loses ``_active_sessions`` - but node + Chromium keep running. Scans the tmp dir for ``agent-browser-*`` - socket dirs and applies ``_reap_socket_dir``'s ownership rules. Safe from any - context — atexit, cleanup thread, or on demand. - """ + """Kill agent-browser daemons whose owning hermes process is gone (an unclean exit loses + ``_active_sessions`` but node + Chromium keep running). Scans the tmp dir for + ``agent-browser-*`` socket dirs; safe from any context.""" import glob # Lightpanda servers keep their own records (no socket dir); sweep them with the @@ -429,9 +416,8 @@ def _stop_browser_cleanup_thread(): def _update_session_activity(task_id: str): - """Touch the activity timestamp; records the owning Hermes home on first sight so - the process-global janitor tears down under the owner's scope. Deliberately does - NOT reset ``_cleanup_failures`` — only a successful cleanup does.""" + """Touch the activity timestamp and record the owning Hermes home on first sight (the + janitor tears down under the owner's scope). Does NOT reset ``_cleanup_failures``.""" with _bt._cleanup_lock: _bt._session_last_activity[task_id] = time.time() _bt._session_owner_homes.setdefault(task_id, str(get_hermes_home())) @@ -440,12 +426,10 @@ def _update_session_activity(task_id: str): def _kill_process_tree(proc: "subprocess.Popen") -> None: """Best-effort kill of *proc* and every descendant; never raises. - ``Popen.kill()`` only signals the direct child; npm/npx helpers and - agent-browser's detached daemon grandchild keep a capture pipe open so - ``communicate()`` never sees EOF (no non-blocking read on Windows), so the whole - tree must go. No grace period: the caller already burned its timeout. - Delegates to :func:`agent.deadline.kill_process_tree`, falling back to - :func:`_legacy_kill_process_tree` on any failure. + ``Popen.kill()`` only signals the direct child; npm/npx helpers and the detached + daemon grandchild keep a capture pipe open so ``communicate()`` never sees EOF, so + the whole tree must go (no grace: the caller already burned its timeout). Delegates + to :func:`agent.deadline.kill_process_tree`, falling back to the legacy kill. """ try: from agent.deadline import kill_process_tree as _deadline_kill_tree @@ -457,8 +441,7 @@ def _kill_process_tree(proc: "subprocess.Popen") -> None: def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None: """Local tree-kill (SIGTERM then SIGKILL to the process group) — fallback when - agent.deadline is unavailable. Differs from hermes_cli._subprocess_compat's - group-leader-only variant, and tests pin this sequence.""" + agent.deadline is unavailable; tests pin this signal sequence.""" if os.name == "nt": try: subprocess.run(["taskkill", "/PID", str(proc.pid), "/T", "/F"], @@ -530,12 +513,9 @@ def _cleanup_old_recordings(max_age_hours=72): def _drop_last_active_binding(task_id: str) -> None: - """Drop stale last-active ownership after cleaning ``task_id``. - - A bare task drops its binding; a sidecar drops it only if that sidecar was - still the recorded owner — a later click can't resurrect a cleaned sidecar on - about:blank while a primary-session binding is preserved. - """ + """Drop stale last-active ownership after cleaning ``task_id``: a bare task always, a + sidecar only if it was still the recorded owner (a later click must not resurrect a + cleaned sidecar while a primary-session binding is preserved).""" bare_task_id = _bt._bare_task_id_for_session_key(task_id) if bare_task_id == task_id or _bt._last_active_session_key.get(bare_task_id) == task_id: _bt._last_active_session_key.pop(bare_task_id, None) @@ -558,8 +538,8 @@ def cleanup_browser(task_id: Optional[str] = None) -> None: def _kill_verified_daemon(socket_dir: str, session_name: str) -> bool: - """Tree-kill the daemon in ``/.pid`` if verifiably ours - (identity check + start-time fingerprint). True when a kill was issued. Never raises.""" + """Tree-kill the daemon in ``/.pid`` if verifiably ours; True when + a kill was issued. Never raises.""" pid_file = os.path.join(socket_dir, f"{session_name}.pid") if not os.path.isfile(pid_file): return False @@ -579,11 +559,8 @@ def _kill_verified_daemon(socket_dir: str, session_name: str) -> bool: def _release_session_resources(task_id: str, session_info: Dict[str, Any]) -> None: - """Untrack ``task_id``, close its cloud provider session, kill its daemon. - - Unconditional tail of ``_cleanup_single_browser_session``; also the whole of the - janitor's force-reap path, which skips the polite ``close`` that kept failing. - """ + """Untrack ``task_id``, close its cloud provider session, kill its daemon — the + unconditional tail of a teardown, and the whole of the janitor's force-reap path.""" bb_session_id = session_info.get("bb_session_id", "unknown") _forget_session_tracking(task_id, session=True) @@ -604,8 +581,7 @@ def _release_session_resources(task_id: str, session_info: Dict[str, Any]) -> No def _force_reap_browser_session(task_id: str) -> None: - """Janitor last resort after repeated failures: skip the failing ``close`` - round-trips, go straight to ``_release_session_resources``.""" + """Janitor last resort: skip the failing ``close`` round-trips, release resources directly.""" _bt._stop_cdp_supervisor(task_id) with _bt._cleanup_lock: session_info = _bt._active_sessions.get(task_id) @@ -618,12 +594,10 @@ def _force_reap_browser_session(task_id: str) -> None: def _cleanup_single_browser_session(task_id: str) -> None: """Reap a single browser session by its exact session key.""" - # Stop the CDP supervisor FIRST so our WebSocket closes before the backend - # tears down the CDP endpoint. - _bt._stop_cdp_supervisor(task_id) + _bt._stop_cdp_supervisor(task_id) # close our WebSocket BEFORE the backend tears down the endpoint - # Camofox: skip the full close when managed persistence is on — the profile - # (session cookies) must survive across tasks; the inactivity reaper still frees idle resources. + # Camofox: managed persistence keeps the profile (cookies) across tasks; skip the full + # close then — the inactivity reaper still frees idle resources. if _bt._is_camofox_mode(): def _camofox_cleanup(): from tools.browser_camofox import camofox_close, camofox_soft_cleanup @@ -634,8 +608,7 @@ def _cleanup_single_browser_session(task_id: str) -> None: _bt.logger.debug("cleanup_browser called for task_id: %s", task_id) _bt.logger.debug("Active sessions: %s", list(_bt._active_sessions.keys())) - # Look up (under lock) but don't remove yet — _run_browser_command needs the - # entry to build the close command. + # Look up but don't remove yet — _run_browser_command needs the entry for ``close``. with _bt._cleanup_lock: session_info = _bt._active_sessions.get(task_id) @@ -646,9 +619,8 @@ def _cleanup_single_browser_session(task_id: str) -> None: _bt.logger.debug("Found session for task %s: bb_session_id=%s", task_id, session_info.get("bb_session_id", "unknown")) _bt._maybe_stop_recording(task_id) # saves the file before close - # Lightpanda sessions are processes Hermes spawned (no daemon to ``close``). - # An expired cloud CDP URL cannot accept a close, and feeding it through - # _get_session_info() would try to renew the session recursively mid-cleanup. + # Lightpanda sessions have no daemon to ``close``; an expired cloud CDP URL cannot + # accept one and would make _get_session_info() renew the session mid-cleanup. if (session_info.get("features") or {}).get("lightpanda"): try: from tools.browser_lightpanda import stop_lightpanda @@ -675,8 +647,7 @@ def cleanup_all_browsers() -> None: for task_id in task_ids: _bt.cleanup_browser(task_id) - # Tear down CDP supervisors for all tasks so background threads exit. - try: + try: # tear down CDP supervisors so background threads exit from tools.browser_supervisor import SUPERVISOR_REGISTRY # type: ignore[import-not-found] SUPERVISOR_REGISTRY.stop_all() except Exception: diff --git a/tools/browser_tool_origin.py b/tools/browser_tool_origin.py index f12f3c1472..3f34887852 100644 --- a/tools/browser_tool_origin.py +++ b/tools/browser_tool_origin.py @@ -1,9 +1,8 @@ """Origin-module lookup shared by the ``tools.browser_tool_*`` extraction modules. -Extracted code must keep reading its origin's symbols (helpers, module state, -``logger``) *through* ``tools.browser_tool`` so ``patch("tools.browser_tool.X")`` -in tests is honoured. The extraction modules must not import ``tools.browser_tool`` -at import time (cycle), so they call :func:`origin_module` lazily per call. +Extracted code must read its origin's symbols *through* ``tools.browser_tool`` so +``patch("tools.browser_tool.X")`` is honoured, and must not import it at import time +(cycle) — hence the lazy :func:`origin_module` and the :data:`origin` proxy. """ import sys @@ -13,9 +12,8 @@ _ORIGIN_NAME = "tools.browser_tool" class _NamespaceView: - """Live attribute view over a module namespace whose module object is gone (a - test purged ``sys.modules`` after importing the origin): the old code still runs - with its own globals, so reads/writes must hit *those*, not a fresh re-import.""" + """Live view over a module namespace whose module object is gone (a test purged + ``sys.modules``): the old code still runs with its own globals, so hit *those*.""" __slots__ = ("_g",) @@ -43,15 +41,11 @@ def _module_for_globals(g: dict): def origin_module(_depth: int = 2): - """Return the ``tools.browser_tool`` instance the *calling* moved function belongs to. - - Resolution order, mirroring what in-file code would see: (1) the nearest - enclosing frame executing ``tools.browser_tool`` code — binds to that exact - module copy even after a test purged/reloaded ``sys.modules``; (2) a - ``tools.browser_tool`` module referenced from a calling frame's globals; - (3) ``sys.modules`` / the ``tools`` package attribute / a fresh import. - ``_depth`` is the frame of the moved function's caller. - """ + """The ``tools.browser_tool`` instance the *calling* moved function belongs to, mirroring + what in-file code would see: (1) the nearest enclosing frame executing origin code (the + exact module copy, even after a test purged/reloaded ``sys.modules``); (2) an origin + module referenced from a calling frame's globals; (3) ``sys.modules`` / ``tools`` + package attribute / fresh import. ``_depth`` is the frame of the moved function's caller.""" try: start = sys._getframe(_depth) except ValueError: # called directly by the interpreter (atexit callback) @@ -78,9 +72,8 @@ def origin_module(_depth: int = 2): class _OriginProxy: - """Module-level ``_bt`` stand-in: every attribute read/write forwards to the - origin module resolved at that moment, so moved functions need no per-call - ``_bt = _origin()`` line.""" + """Module-level ``_bt`` stand-in: every attribute read/write forwards to the origin + module resolved at that moment.""" __slots__ = () diff --git a/tools/browser_tool_session.py b/tools/browser_tool_session.py index 3f86e095f5..c999de236f 100644 --- a/tools/browser_tool_session.py +++ b/tools/browser_tool_session.py @@ -1,11 +1,8 @@ """agent-browser session management: daemon spawn, per-backend session creation -(local/lightpanda/cdp/cloud), cached session lookup, command execution with timeout -handling and output interpretation. +(local/lightpanda/cdp/cloud), cached lookup, command execution + output interpretation. -Split out of ``tools/browser_tool.py``; every name is re-imported there. Origin -symbols and module state are read/written through ``_bt`` (the origin module, -the :data:`tools.browser_tool_origin.origin` proxy) so -``patch("tools.browser_tool.X")`` is honoured and no import cycle exists. +Split out of ``tools/browser_tool.py`` (every name re-imported there); origin symbols +are read through the ``_bt`` proxy so ``patch("tools.browser_tool.X")`` is honoured. """ import json @@ -21,9 +18,8 @@ from tools.browser_tool_origin import origin as _bt _DOCKER_PULL = "docker pull ghcr.io/nousresearch/hermes-agent:latest" _CHROMIUM_INSTALL = "npx agent-browser install --with-deps (or: npx playwright install --with-deps chromium)" -_CHROMIUM_MISSING_DOCKER_HINT = ( - f"Chromium browser is missing. You're running in Docker — pull the latest image to get the bundled Chromium: {_DOCKER_PULL}" -) +_CHROMIUM_MISSING_DOCKER_HINT = ("Chromium browser is missing. You're running in Docker — pull the latest image " + f"to get the bundled Chromium: {_DOCKER_PULL}") _CHROMIUM_MISSING_HINT = f"Chromium browser is missing. Install it with: {_CHROMIUM_INSTALL}" @@ -42,11 +38,8 @@ def _needs_chromium_sandbox_bypass() -> bool: def _apply_chromium_sandbox_args(browser_env: Dict[str, str]) -> None: """Add required Chromium sandbox flags without overriding user settings.""" - if ( - "AGENT_BROWSER_ARGS" not in browser_env - and "AGENT_BROWSER_CHROME_FLAGS" not in browser_env - and _bt._needs_chromium_sandbox_bypass() - ): + if ("AGENT_BROWSER_ARGS" not in browser_env and "AGENT_BROWSER_CHROME_FLAGS" not in browser_env + and _bt._needs_chromium_sandbox_bypass()): _bt.logger.debug("browser: sandbox bypass needed (root/docker/AppArmor userns) — injecting --no-sandbox") browser_env["AGENT_BROWSER_ARGS"] = "--no-sandbox,--disable-dev-shm-usage" @@ -95,14 +88,12 @@ def _format_browser_timeout_error( def _agent_browser_argv(browser_cmd: str) -> list: - """Command prefix to invoke agent-browser (concrete binary or npx sentinel). + """Command prefix to invoke agent-browser (concrete binary, or the npx sentinel expanded). - Concrete paths stay a single argv item; only the npx sentinel expands. npx is - resolved through the same PATH cascade as ``_find_agent_browser`` (a bare - ``shutil.which("npx")`` would let a broken system npx shadow a healthy managed - one); if absent the bare name gives a readable ``FileNotFoundError: 'npx'``. - ``--ignore-scripts``: AGENT_BROWSER_NPX_SPEC is a floating range — a compromised - future patch must not run install-time scripts. + npx is resolved through the same PATH cascade as ``_find_agent_browser`` (a bare + ``which("npx")`` would let a broken system npx shadow a healthy managed one); if + absent the bare name gives a readable ``FileNotFoundError``. ``--ignore-scripts``: + the spec is a floating range — a compromised future patch must not run install scripts. """ if _bt._is_npx_agent_browser_sentinel(browser_cmd): _npx_bin = _bt._resolve_npx_bin() or "npx" @@ -111,12 +102,9 @@ def _agent_browser_argv(browser_cmd: str) -> list: def _prepare_session_socket_dir(session_name: str) -> str: - """Create the per-session socket dir and claim it with our PID. - - Per-session dirs keep parallel workers from fighting over the default socket - path. The owner_pid file is written BEFORE first use: another hermes process's - orphan reaper rmtree's any ownerless agent-browser-* dir in the shared tmpdir. - """ + """Create the per-session socket dir (parallel workers must not share one) and claim it + with our PID BEFORE first use — another hermes process's orphan reaper rmtree's any + ownerless agent-browser-* dir in the shared tmpdir.""" socket_dir = os.path.join(_bt._socket_safe_tmpdir(), f"agent-browser-{session_name}") os.makedirs(socket_dir, mode=0o700, exist_ok=True) _bt._write_owner_pid(socket_dir, session_name) @@ -124,10 +112,9 @@ def _prepare_session_socket_dir(session_name: str) -> str: def _agent_browser_command_env(socket_dir: str) -> Dict[str, str]: - """Credential-scrubbed env for one agent-browser command: discovery-time PATH - fallbacks, the session socket dir, and daemon-side idle self-termination - (``AGENT_BROWSER_IDLE_TIMEOUT_MS``, agent-browser 0.24+) mirroring the Python - janitor — unless the user set it explicitly.""" + """Credential-scrubbed env for one command: PATH fallbacks, the session socket dir, and + daemon-side idle self-termination (agent-browser 0.24+) mirroring the Python janitor + unless the user set ``AGENT_BROWSER_IDLE_TIMEOUT_MS`` explicitly.""" env = _bt._build_browser_env() env["PATH"] = _bt._merge_browser_path(env.get("PATH", "")) env["AGENT_BROWSER_SOCKET_DIR"] = socket_dir @@ -137,14 +124,12 @@ def _agent_browser_command_env(socket_dir: str) -> Dict[str, str]: def _popen_agent_browser(argv: List[str], env: Dict[str, str], socket_dir: str, tag: str) -> "subprocess.Popen": - """Spawn agent-browser with stdout/stderr redirected to ``socket_dir/_stdout_``. + """Spawn agent-browser with stdout/stderr redirected to ``socket_dir/_std{out,err}_``. - Temp files instead of pipes: the CLI forks a daemon that inherits its fds, so - with pipes ``communicate()`` never sees EOF until the timeout. Windows: - CREATE_NO_WINDOW only (NOT CREATE_NEW_PROCESS_GROUP — on 3.11 it cancels - asyncio's running task and surfaces as KeyboardInterrupt), STARTF_USESTDHANDLES - so the child gets ONLY our three handles (leaked console handles make the Rust - daemon grandchild die silently), close_fds=True for the rest. + Temp files, not pipes: the CLI forks a daemon that inherits its fds, so pipes never + see EOF until the timeout. Windows: CREATE_NO_WINDOW only (CREATE_NEW_PROCESS_GROUP + cancels asyncio's running task on 3.11), STARTF_USESTDHANDLES + close_fds so the child + gets ONLY our three handles (leaked console handles kill the Rust daemon grandchild). """ fds = [os.open(os.path.join(socket_dir, f"_{slot}_{tag}"), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) for slot in ("stdout", "stderr")] @@ -169,10 +154,10 @@ def _session_record(prefix: str, cdp_url: Optional[str], features: Dict[str, Any def _create_local_session(task_id: str, allow_real_profile: bool = True) -> Dict[str, str]: """Local Chromium session; consented real-profile CDP attach when allowed. - Real-profile fails closed on resolver/launch errors — a consented user must - never be silently downgraded to a throwaway. The hybrid private-URL sidecar - passes ``allow_real_profile=False``: handing the user's cookie jar to an - arbitrary internal host the model chose is a larger, unconsented exposure. + Real-profile fails closed on resolver/launch errors (a consented user must never be + silently downgraded to a throwaway). The hybrid private-URL sidecar passes + ``allow_real_profile=False``: the user's cookie jar must not reach an arbitrary + internal host the model chose. """ if allow_real_profile: cdp_url, err = _bt._real_profile_cdp() @@ -183,9 +168,8 @@ def _create_local_session(task_id: str, allow_real_profile: bool = True) -> Dict _bt.logger.info("Created real-profile local session %s for task %s", info["session_name"], task_id) return info - # Browser Use mode drives whatever CDP endpoint it is handed; with - # ``browser.engine: lightpanda`` that is a Hermes-spawned ``lightpanda serve``. - # The built-in tools never reach this branch (hidden in Browser Use mode). + # Browser Use mode + ``browser.engine: lightpanda`` drives a Hermes-spawned + # ``lightpanda serve`` (the built-in tools are hidden in that mode). if _bt._is_browser_use_cli_mode() and _bt._using_lightpanda_engine(): return _bt._create_lightpanda_session(task_id) @@ -226,11 +210,8 @@ def _create_cdp_session(task_id: str, cdp_url: str) -> Dict[str, str]: def _create_cloud_session_or_fallback(task_id: str, provider) -> Dict[str, Any]: - """Cloud session; fall back to local Chromium (marked degraded) on failure. - - Some providers (Browser-Use v3) return an HTTP CDP discovery URL instead of a - raw websocket endpoint, so ``cdp_url`` is resolved here. - """ + """Cloud session; fall back to local Chromium (marked degraded) on failure. ``cdp_url`` + is resolved here because some providers return an HTTP discovery URL, not a websocket.""" try: session_info = provider.create_session(task_id) if not session_info or not isinstance(session_info, dict): @@ -256,10 +237,7 @@ def _create_cloud_session_or_fallback(task_id: str, provider) -> Dict[str, Any]: def _create_session_for_key(task_id: str, force_local: bool) -> Dict[str, Any]: """Fresh session for ``task_id`` (runs OUTSIDE the lock: cloud mode makes a network call). - - Precedence: CDP override > hybrid local sidecar > cloud provider > local. The - hybrid sidecar NEVER gets the real profile (see ``_create_local_session``). - """ + Precedence: CDP override > hybrid local sidecar (never real-profile) > cloud > local.""" cdp_override = _bt._get_cdp_override() if cdp_override and not force_local: return _bt._create_cdp_session(task_id, cdp_override) @@ -272,13 +250,9 @@ def _create_session_for_key(task_id: str, force_local: bool) -> Dict[str, Any]: def _get_session_info(task_id: Optional[str] = None) -> Dict[str, Any]: - """Get or create session info for a session key (thread-safe). - - A ``::local``-suffixed key (hybrid sidecar) forces local Chromium even with a - cloud provider configured. Also starts the inactivity thread and touches - activity tracking. Returns ``session_name`` (always) plus ``bb_session_id`` / - ``cdp_url`` for cloud sessions. - """ + """Get or create session info for a session key (thread-safe); also starts the + inactivity thread and touches activity. A ``::local`` key forces local Chromium + even with a cloud provider configured.""" if task_id is None: task_id = "default" @@ -289,20 +263,16 @@ def _get_session_info(task_id: Optional[str] = None) -> Dict[str, Any]: existing_session = _bt._active_sessions.get(task_id) def _replacement_after_teardown() -> Optional[Dict[str, Any]]: - # Teardown removes the activity entry; the replacement must be tracked by - # the reaper like an initial session. Another thread may already have - # recycled and re-created it — return that live one instead of a third. + # Teardown removes the activity entry; re-touch so the reaper tracks the + # replacement. Another thread may already have re-created it — reuse that. _bt._update_session_activity(task_id) with _bt._cleanup_lock: replacement = _bt._active_sessions.get(task_id) - if replacement is not None and replacement is not existing_session: - return replacement - return None + return replacement if replacement is not None and replacement is not existing_session else None if existing_session is not None: - # Suspect-session recycle: a previous command timeout marked this cached - # session via the SuspectableBackend adapter; the expensive recycle lives - # here at next use, not on the timeout path (mark must stay cheap). + # Suspect recycle: a command timeout marked this session; the expensive recycle + # lives here at next use, not on the timeout path (mark must stay cheap). if not _bt._browser_session_backend(task_id).ensure_healthy(): replacement = _replacement_after_teardown() if replacement is not None: @@ -321,20 +291,16 @@ def _get_session_info(task_id: Optional[str] = None) -> Dict[str, Any]: session_info = _bt._create_session_for_key(task_id, force_local) with _bt._cleanup_lock: - # Another thread may have created a session during the network call; use - # it to avoid leaking orphan cloud sessions. - if task_id in _bt._active_sessions: + if task_id in _bt._active_sessions: # created concurrently during the network call — don't leak ours return _bt._active_sessions[task_id] session_info = dict(session_info) session_info.setdefault("session_key", task_id) session_info.setdefault("owner_task_id", _bt._bare_task_id_for_session_key(task_id)) _bt._active_sessions[task_id] = session_info - # A brand-new session is healthy by definition — drop any stale suspect flag. - _bt._suspect_browser_sessions.pop(task_id, None) + _bt._suspect_browser_sessions.pop(task_id, None) # brand-new session is healthy by definition - # Lazy-start the CDP supervisor (idempotent; swallows errors). Skip for local - # sidecars (no CDP URL) and Lightpanda sessions (Browser Use mode hides the - # browser_* tools that consume supervisor state; it would just idle a second CDP connection). + # Lazy-start the CDP supervisor (idempotent). Skip local sidecars (no CDP URL) and + # Lightpanda sessions (Browser Use mode hides the tools that consume supervisor state). if not force_local and not (session_info.get("features") or {}).get("lightpanda"): _bt._ensure_cdp_supervisor(task_id) @@ -389,12 +355,9 @@ def _read_browser_daemon_pid(task_socket_dir: str, session_name: str) -> Optiona def _browser_daemon_responsive(task_socket_dir: str, probe_timeout_s: float = 1.0) -> bool: - """Cheap liveness probe: connect to the daemon's unix control socket. - - A successful connect proves the accept loop is alive (the command wedged on the - page/CDP side). Windows uses named pipes — no probe possible, so report - unresponsive (tree-kill + respawn is the safe recovery). - """ + """Cheap liveness probe: a connect to the daemon's unix control socket proves the accept + loop is alive (the command wedged page/CDP-side). Windows named pipes can't be probed → + report unresponsive (tree-kill + respawn is the safe recovery).""" if os.name == "nt": return False import socket as socket_mod @@ -417,17 +380,14 @@ def _browser_daemon_responsive(task_socket_dir: str, probe_timeout_s: float = 1. def _handle_browser_command_timeout(task_id: str, session_info: Dict[str, Any], task_socket_dir: str) -> None: - """Recover session state after a browser command timeout. + """Recover session state after a command timeout. - * Cloud / CDP: no local daemon to probe — replace the stuck client generation - now (fresh ``session_name``, same ``bb_session_id`` so cloud cleanup works). - * Local daemon alive (PID live, identity-verified, control socket accepts): only - the *command* wedged; mark suspect and let next use recycle via ``ensure_healthy``. - * Local daemon wedged/dead: it cannot service a clean close and its Chromium - children would leak — tree-kill and evict now. - - Both local branches ``mark_suspect`` first (cheap, lock-free) so the - poisoned-cache invariant holds even if eviction races another thread's replacement. + Cloud/CDP: no daemon to probe — replace the stuck client generation now (same + ``bb_session_id`` so cloud cleanup works). Local daemon alive (PID live, verified, + socket accepts): only the command wedged — mark suspect, recycle at next use. + Local daemon wedged/dead: tree-kill and evict now (Chromium children would leak). + Both local branches ``mark_suspect`` first so the poisoned-cache invariant holds + even if eviction races another thread's replacement. """ if session_info.get("bb_session_id") or session_info.get("cdp_url"): _bt._discard_timed_out_browser_session(task_id, session_info, task_socket_dir) @@ -457,13 +417,9 @@ def _handle_browser_command_timeout(task_id: str, session_info: Dict[str, Any], def _interpret_browser_command_output(command: str, stdout: str, stderr: str, returncode: int) -> Dict[str, Any]: - """Turn a finished agent-browser process's output into a result dict. - - Empty stdout with rc=0 is a broken state (stale daemon) and is reported as - failure rather than a silent success — except for ``_EMPTY_OK_COMMANDS``. - Non-JSON output is an error, except ``screenshot`` where the saved path is - recovered from the prose. - """ + """Finished agent-browser process output → result dict. Empty stdout with rc=0 is a + broken state (stale daemon) reported as failure except for ``_EMPTY_OK_COMMANDS``; + non-JSON output is an error except ``screenshot``, whose path is recovered from prose.""" if stderr and stderr.strip(): level = logging.WARNING if returncode != 0 else logging.DEBUG _bt.logger.log(level, "browser '%s' stderr: %s", command, stderr.strip()[:500]) @@ -502,9 +458,8 @@ def _interpret_browser_command_output(command: str, stdout: str, stderr: str, re def _browser_command_preflight() -> Dict[str, Any]: - """Fail fast before spawning: missing CLI, Termux install gap, interrupt, or no - Chromium in local mode (else every call hangs for command_timeout). Returns an - error result, or ``{"browser_cmd": path}`` on success.""" + """Fail fast before spawning (missing CLI, Termux gap, interrupt, no Chromium in local + mode — else every call hangs for command_timeout). Error result, or ``{"browser_cmd": path}``.""" try: browser_cmd = _bt._find_agent_browser() except FileNotFoundError as e: @@ -587,11 +542,8 @@ def _run_browser_command( _engine_override: Optional[str] = None, ) -> Dict[str, Any]: """Run one agent-browser CLI command against the task's session; returns its parsed JSON. - - ``timeout=None`` reads ``browser.command_timeout`` (default 30s). - ``_engine_override`` forces an engine for this call only (the Lightpanda - fallback uses it to retry with Chrome without touching global state). - """ + ``timeout=None`` reads ``browser.command_timeout``; ``_engine_override`` forces an engine + for this call only (Lightpanda fallback retries with Chrome without touching global state).""" if timeout is None: timeout = _bt._safe_command_timeout() args = args or []