refactor(browser): all cloud browser providers as image_gen-style plugins (#25214, salvages #25580)

agent/browser_provider.py

@@ -0,0 +1,175 @@
+"""
+Browser Provider ABC
+====================
+
+Defines the pluggable-backend interface for cloud browser providers
+(Browserbase, Browser Use, Firecrawl, …). Providers register instances via
+:meth:`PluginContext.register_browser_provider`; the active one (selected via
+``browser.cloud_provider`` in ``config.yaml``) services every cloud-mode
+``browser_*`` tool call.
+
+Providers live in ``<repo>/plugins/browser/<name>/`` (built-in, auto-loaded as
+``kind: backend``) or ``~/.hermes/plugins/browser/<name>/`` (user, opt-in via
+``plugins.enabled``).
+
+This ABC mirrors :class:`agent.web_search_provider.WebSearchProvider` (PR
+#25182) — same shape, same registration flow, same picker integration. The
+legacy in-tree ``tools.browser_providers.base.CloudBrowserProvider`` ABC was
+deleted in PR #25214 (this work) along with the per-vendor inline modules in
+``tools/browser_providers/``; the lifecycle contract documented below is
+preserved bit-for-bit so the tool wrapper (:mod:`tools.browser_tool`) does
+not have to translate.
+
+Session metadata contract (preserved from the legacy ``CloudBrowserProvider``)::
+
+    {
+        "session_name": str,        # unique name for agent-browser --session
+        "bb_session_id": str,       # provider session ID (for close/cleanup)
+        "cdp_url": str,             # CDP websocket URL
+        "features": dict,           # feature flags that were enabled
+        "external_call_id": str,    # optional, managed-gateway billing key
+    }
+
+``bb_session_id`` is a legacy key name kept verbatim for backward compat with
+:mod:`tools.browser_tool` — it holds the provider's session ID regardless of
+which provider is in use.
+"""
+
+from __future__ import annotations
+
+import abc
+from typing import Any, Dict
+
+
+# ---------------------------------------------------------------------------
+# ABC
+# ---------------------------------------------------------------------------
+
+
+class BrowserProvider(abc.ABC):
+    """Abstract base class for a cloud browser backend.
+
+    Subclasses must implement :meth:`name`, :meth:`is_available`, and the
+    three lifecycle methods: :meth:`create_session`, :meth:`close_session`,
+    :meth:`emergency_cleanup`.
+
+    The lifecycle shape preserves the legacy ``CloudBrowserProvider`` contract
+    bit-for-bit so the dispatcher in :mod:`tools.browser_tool` is a pure
+    registry lookup — no per-provider conditionals, no shape translation.
+    """
+
+    @property
+    @abc.abstractmethod
+    def name(self) -> str:
+        """Stable short identifier used in the ``browser.cloud_provider``
+        config key.
+
+        Lowercase, hyphens permitted to preserve existing user-visible names.
+        Examples: ``browserbase``, ``browser-use``, ``firecrawl``.
+        """
+
+    @property
+    def display_name(self) -> str:
+        """Human-readable label shown in ``hermes tools``. Defaults to ``name``."""
+        return self.name
+
+    @abc.abstractmethod
+    def is_available(self) -> bool:
+        """Return True when this provider can service calls.
+
+        Typically a cheap check (env var present, managed-gateway token
+        readable, optional Python dep importable). Must NOT make network
+        calls — this runs at tool-registration time and on every
+        ``hermes tools`` paint.
+
+        Mirrors the legacy ``CloudBrowserProvider.is_configured()`` method;
+        renamed for parity with :class:`agent.web_search_provider.WebSearchProvider`.
+        """
+
+    @abc.abstractmethod
+    def create_session(self, task_id: str) -> Dict[str, object]:
+        """Create a cloud browser session and return session metadata.
+
+        Must return a dict with at least::
+
+            {
+                "session_name": str,    # unique name for agent-browser --session
+                "bb_session_id": str,   # provider session ID (for close/cleanup)
+                "cdp_url": str,         # CDP websocket URL
+                "features": dict,       # feature flags that were enabled
+            }
+
+        ``bb_session_id`` is a legacy key name kept for backward compat with
+        the rest of :mod:`tools.browser_tool` — it holds the provider's
+        session ID regardless of which provider is in use.
+
+        May raise ``ValueError`` (missing credentials) or ``RuntimeError``
+        (network / API failure); the dispatcher surfaces these to the user.
+        """
+
+    @abc.abstractmethod
+    def close_session(self, session_id: str) -> bool:
+        """Release / terminate a cloud session by its provider session ID.
+
+        Returns True on success, False on failure. Should not raise — log and
+        return False on any exception so the dispatcher's cleanup loop keeps
+        moving across sessions.
+        """
+
+    @abc.abstractmethod
+    def emergency_cleanup(self, session_id: str) -> None:
+        """Best-effort session teardown during process exit.
+
+        Called from atexit / signal handlers. Must tolerate missing
+        credentials, network errors, etc. — log and move on. Must not raise.
+        """
+
+    def get_setup_schema(self) -> Dict[str, Any]:
+        """Return provider metadata for the ``hermes tools`` picker.
+
+        Used by :mod:`hermes_cli.tools_config` to inject this provider as a
+        row in the Browser Automation picker. Shape mirrors the existing
+        hardcoded entries in ``TOOL_CATEGORIES["browser"]``::
+
+            {
+                "name": "Browserbase",
+                "badge": "paid",
+                "tag": "Cloud browser with stealth and proxies",
+                "env_vars": [
+                    {"key": "BROWSERBASE_API_KEY",
+                     "prompt": "Browserbase API key",
+                     "url": "https://browserbase.com"},
+                ],
+                "post_setup": "agent_browser",
+            }
+
+        Default: minimal entry derived from :attr:`display_name`. Override to
+        expose API key prompts, badges, managed-Nous gating, and the
+        ``post_setup`` install hook.
+        """
+        return {
+            "name": self.display_name,
+            "badge": "",
+            "tag": "",
+            "env_vars": [],
+        }
+
+    # ------------------------------------------------------------------
+    # Backward-compat shims for the legacy CloudBrowserProvider API
+    # ------------------------------------------------------------------
+    #
+    # The pre-PR-#25214 ABC exposed ``is_configured()`` and ``provider_name()``;
+    # ``tools.browser_tool`` has ~6 callers that still use those names. Rather
+    # than churn every callsite (and break out-of-tree downstream code that
+    # subclassed CloudBrowserProvider), we expose the old names as thin
+    # delegations to the new API. Subclasses MUST implement :meth:`is_available`
+    # and :attr:`name`; they may override ``is_configured`` / ``provider_name``
+    # for compatibility with the legacy ABC but it is not required.
+
+    def is_configured(self) -> bool:
+        """Backward-compat alias for :meth:`is_available`."""
+        return self.is_available()
+
+    def provider_name(self) -> str:
+        """Backward-compat alias returning :attr:`display_name`."""
+        return self.display_name

agent/browser_registry.py

@@ -0,0 +1,223 @@
+"""
+Browser Provider Registry
+=========================
+
+Central map of registered cloud browser providers. Populated by plugins at
+import-time via :meth:`PluginContext.register_browser_provider`; consumed by
+:func:`tools.browser_tool._get_cloud_provider` to route each cloud-mode
+``browser_*`` tool call to the active backend.
+
+Active selection
+----------------
+The active provider is chosen by configuration with this precedence:
+
+1. ``browser.cloud_provider`` in ``config.yaml`` (explicit override).
+2. Legacy preference order — ``browser-use`` → ``browserbase`` — filtered by
+   availability. Matches the historic auto-detect order in
+   :func:`tools.browser_tool._get_cloud_provider` (Browser Use checked first
+   because it covers both the managed Nous gateway and direct API key path;
+   Browserbase as the older direct-credentials fallback). ``firecrawl`` is
+   intentionally NOT in the legacy walk — users only get Firecrawl as a
+   cloud browser when they explicitly set ``browser.cloud_provider:
+   firecrawl``, matching pre-migration behaviour where Firecrawl was never
+   auto-selected.
+3. Otherwise ``None`` — the dispatcher falls back to local browser mode.
+
+The explicit-config branch (rule 1) intentionally ignores ``is_available()``
+so the dispatcher surfaces a typed "X_API_KEY is not set" error to the user
+instead of silently switching backends. Matches the legacy
+:func:`tools.browser_tool._get_cloud_provider` behaviour for configured names.
+
+Note: there is no "capability" split here (unlike the web subsystem, which
+has search/extract/crawl). Every browser provider implements the full
+:class:`agent.browser_provider.BrowserProvider` lifecycle; the registry's
+job is purely selection, not capability routing.
+"""
+
+from __future__ import annotations
+
+import logging
+import threading
+from typing import Dict, List, Optional
+
+from agent.browser_provider import BrowserProvider
+
+logger = logging.getLogger(__name__)
+
+
+_providers: Dict[str, BrowserProvider] = {}
+_lock = threading.Lock()
+
+
+def register_provider(provider: BrowserProvider) -> None:
+    """Register a cloud browser provider.
+
+    Re-registration (same ``name``) overwrites the previous entry and logs
+    a debug message — makes hot-reload scenarios (tests, dev loops) behave
+    predictably.
+    """
+    if not isinstance(provider, BrowserProvider):
+        raise TypeError(
+            f"register_provider() expects a BrowserProvider instance, "
+            f"got {type(provider).__name__}"
+        )
+    name = provider.name
+    if not isinstance(name, str) or not name.strip():
+        raise ValueError("Browser provider .name must be a non-empty string")
+    with _lock:
+        existing = _providers.get(name)
+        _providers[name] = provider
+    if existing is not None:
+        logger.debug(
+            "Browser provider '%s' re-registered (was %r)",
+            name, type(existing).__name__,
+        )
+    else:
+        logger.debug(
+            "Registered browser provider '%s' (%s)",
+            name, type(provider).__name__,
+        )
+
+
+def list_providers() -> List[BrowserProvider]:
+    """Return all registered providers, sorted by name."""
+    with _lock:
+        items = list(_providers.values())
+    return sorted(items, key=lambda p: p.name)
+
+
+def get_provider(name: str) -> Optional[BrowserProvider]:
+    """Return the provider registered under *name*, or None."""
+    if not isinstance(name, str):
+        return None
+    with _lock:
+        return _providers.get(name.strip())
+
+
+# ---------------------------------------------------------------------------
+# Active-provider resolution
+# ---------------------------------------------------------------------------
+
+
+# Legacy auto-detect order — used when no ``browser.cloud_provider`` is set.
+# Matches the pre-migration walk in :func:`tools.browser_tool._get_cloud_provider`.
+# Firecrawl is intentionally absent so users with ``FIRECRAWL_API_KEY`` set
+# for web-extract don't get silently routed to a paid cloud browser. See
+# :func:`_resolve` for the full rationale.
+_LEGACY_PREFERENCE = (
+    "browser-use",
+    "browserbase",
+)
+
+
+def _resolve(configured: Optional[str]) -> Optional[BrowserProvider]:
+    """Resolve the active browser provider.
+
+    Resolution rules (in order):
+
+    1. **Explicit "local".** Returns None — the dispatcher disables cloud
+       mode entirely. Mirrors legacy short-circuit in
+       :func:`tools.browser_tool._get_cloud_provider`.
+    2. **Explicit config wins, ignoring availability.** If ``configured``
+       names a registered provider, return it even if its
+       :meth:`is_available` returns False — the dispatcher will surface a
+       precise "X_API_KEY is not set" error instead of silently routing
+       somewhere else.
+    3. **Legacy preference walk, filtered by availability.** Walk
+       :data:`_LEGACY_PREFERENCE` (``browser-use`` → ``browserbase``) looking
+       for a provider whose ``is_available()`` is True.
+
+    There is intentionally NO "single-eligible shortcut" rule here (unlike
+    :func:`agent.web_search_registry._resolve`). Pre-migration, the
+    auto-detect branch in ``tools.browser_tool._get_cloud_provider`` only
+    considered Browser Use and Browserbase; Firecrawl was reachable only
+    via an explicit ``browser.cloud_provider: firecrawl`` config key.
+    Preserving that gate matters because Firecrawl shares its API key with
+    the *web* extract plugin (``plugins/web/firecrawl/``), so users who set
+    ``FIRECRAWL_API_KEY`` for web extract must NOT get silently routed to a
+    paid cloud browser on a fresh install. Third-party browser-provider
+    plugins added under ``~/.hermes/plugins/browser/<vendor>/`` are subject
+    to the same gate — they must be explicitly configured to take effect.
+
+    Returns None when no provider is configured AND no available provider
+    matches the legacy preference; the dispatcher then falls back to local
+    browser mode.
+    """
+    with _lock:
+        snapshot = dict(_providers)
+
+    def _is_available_safe(p: BrowserProvider) -> bool:
+        """Wrap ``is_available()`` so a buggy provider doesn't kill resolution."""
+        try:
+            return bool(p.is_available())
+        except Exception as exc:  # noqa: BLE001
+            logger.warning(
+                "Browser provider %s.is_available() raised %s — treating as unavailable",
+                p.name, exc, exc_info=True,
+            )
+            return False
+
+    # 1. Explicit "local" short-circuit.
+    if configured == "local":
+        return None
+
+    # 2. Explicit config wins — return regardless of is_available() so the
+    #    user gets a precise downstream error message rather than a silent
+    #    backend switch. Matches _get_cloud_provider() in browser_tool.py.
+    if configured:
+        provider = snapshot.get(configured)
+        if provider is not None:
+            return provider
+        logger.debug(
+            "browser cloud_provider '%s' configured but not registered; "
+            "falling back to auto-detect",
+            configured,
+        )
+
+    # 3. Legacy preference walk — only providers in _LEGACY_PREFERENCE are
+    #    auto-eligible. Filtered by availability so we don't surface a
+    #    provider the user has no credentials for. See docstring for why
+    #    we do NOT fall back to "any single-eligible registered provider".
+    for legacy in _LEGACY_PREFERENCE:
+        provider = snapshot.get(legacy)
+        if provider is not None and _is_available_safe(provider):
+            return provider
+
+    return None
+
+
+def get_active_browser_provider() -> Optional[BrowserProvider]:
+    """Resolve the currently-active cloud browser provider.
+
+    Reads ``browser.cloud_provider`` from config.yaml; falls back per the
+    module docstring. Returns None for local mode or when no provider is
+    available.
+    """
+    try:
+        from hermes_cli.config import read_raw_config
+
+        cfg = read_raw_config()
+        browser_cfg = cfg.get("browser", {})
+    except Exception as exc:
+        logger.debug("Could not read browser config: %s", exc)
+        browser_cfg = {}
+
+    configured: Optional[str] = None
+    if isinstance(browser_cfg, dict) and "cloud_provider" in browser_cfg:
+        try:
+            from tools.tool_backend_helpers import normalize_browser_cloud_provider
+
+            configured = normalize_browser_cloud_provider(
+                browser_cfg.get("cloud_provider")
+            )
+        except Exception as exc:
+            logger.debug("normalize_browser_cloud_provider failed: %s", exc)
+            configured = None
+
+    return _resolve(configured)
+
+
+def _reset_for_tests() -> None:
+    """Clear the registry. **Test-only.**"""
+    with _lock:
+        _providers.clear()