# NousResearch/hermes-agent · Refactor cloud browser providers as plugin architecture ## Open now - Nothing open on GitHub. These items concern migrating cloud browser providers to follow an image_gen-style plugin pattern. The work involves restructuring browser provider integrations similarly to how image generation providers are organized. ## TL;DR - 3 member(s): 0 open issue(s), 1 closed issue(s), 0 open PR(s), 1 merged PR(s), 1 PR(s) closed without merging. - A fix was merged to main in PR #27403. Verify it addresses your case. - **Recommended:** read PR #27403 (and any competing PRs below) before filing. Confirm the merged change covers your symptom. ## Maintainer resolution verdict - Internal-tracker verdict over this cluster's issue members: `resolved` · deciding PR(s): `pr-27403`. This is distinct from the fix status above. Weigh it yourself. ## How this cluster was grouped - Link evidence (`STRUCTURAL_SPANNING`): every member is connected by structural evidence: shared changed files, or a closing/superseded reference. - Pairwise link density `1.00`: the share of member pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36. `1.00` means every pair is linked. ## Closed / duplicate issues - #25214: Mirror web-provider plugin migration for browser providers ## Prior & competing PRs - **#27403: refactor(browser): all cloud browser providers as image_gen-style plugins (#25214, salvages #25580)**. merged to main · fix tier `verified` - maintainer direction: category `merged` - files: `agent/browser_provider.py`, `agent/browser_registry.py`, `hermes_cli/plugins.py`, `hermes_cli/tools_config.py`, `plugins/browser/browser_use/__init__.py`, `plugins/browser/browser_use/plugin.yaml`, `plugins/browser/browser_use/provider.py`, `plugins/browser/browserbase/__init__.py`, `plugins/browser/browserbase/plugin.yaml`, `plugins/browser/browserbase/provider.py`, `plugins/browser/firecrawl/__init__.py`, `plugins/browser/firecrawl/plugin.yaml`, `plugins/browser/firecrawl/provider.py`, `tests/plugins/browser/__init__.py`, `tests/plugins/browser/check_parity_vs_main.py`, `tests/plugins/browser/test_browser_provider_plugins.py`, `tests/tools/test_managed_browserbase_and_modal.py`, `tools/browser_providers/__init__.py`, `tools/browser_providers/base.py`, `tools/browser_tool.py` - fix hunks: - `agent/browser_provider.py` ```diff @@ -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 ``/plugins/browser//`` (built-in, auto-loaded as +``kind: backend``) or ``~/.hermes/plugins/browser//`` (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` ```diff @@ -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//`` 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() ``` - `hermes_cli/plugins.py` ```diff @@ -608,6 +608,38 @@ def register_web_search_provider(self, provider) -> None: self.manifest.name, provider.name, ) + # -- browser provider registration --------------------------------------- + + def register_browser_provider(self, provider) -> None: + """Register a cloud browser backend. + + ``provider`` must be an instance of + :class:`agent.browser_provider.BrowserProvider`. The + ``provider.name`` attribute is what ``browser.cloud_provider`` in + ``config.yaml`` matches against when routing cloud-mode + ``browser_*`` tool calls. + + Mirrors :meth:`register_web_search_provider` exactly — same + registration shape, same gating, same logging. The browser + subsystem's dispatcher (:func:`tools.browser_tool._get_cloud_provider`) + consults the registry built up by these calls. + """ + from agent.browser_provider import BrowserProvider + from agent.browser_registry import register_provider as _register_browser_provider + + if not isinstance(provider, BrowserProvider): + logger.warning( + "Plugin '%s' tried to register a browser provider that does " + "not inherit from BrowserProvider. Ignoring.", + self.manifest.name, + ) + return + _register_browser_provider(provider) + logger.info( + "Plugin '%s' registered browser provider: %s", + self.manifest.name, provider.name, + ) + # -- platform adapter registration --------------------------------------- def register_platform( ``` - `hermes_cli/tools_config.py` ```diff @@ -378,6 +378,17 @@ def _get_plugin_toolset_keys() -> set: "browser": { "name": "Browser Automation", "icon": "🌐", + # Per-provider rows for Browserbase, Browser Use, and Firecrawl are + # injected at runtime from plugins.browser..provider via + # _plugin_browser_providers() in _visible_providers(). Only + # non-provider UX setup-flow rows remain here: + # - "Nous Subscription (Browser Use cloud)" — managed Browser Use + # billed via Nous subscription (requires_nous_auth + + # override_env_vars). Uses the browser-use plugin as the + # underlying backend but has a distinct setup UX. + # - "Local Browser" — non-cloud option, no CloudBrowserProvider. + # - "Camofox" — anti-detection local Firefox; short-circuits the + # cloud-provider dispatch path via _is_camofox_mode(). "providers": [ { "name": "Nous Subscription (Browser Use cloud)", ``` ```diff @@ -398,37 +409,6 @@ def _get_plugin_toolset_keys() -> set: "browser_provider": "local", "post_setup": "agent_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"}, - {"key": "BROWSERBASE_PROJECT_ID", "prompt": "Browserbase project ID"}, - ], - "browser_provider": "browserbase", - "post_setup": "agent_browser", - }, - { - "name": "Browser Use", - "badge": "paid", - "tag": "Cloud browser with remote execution", - "env_vars": [ - {"key": "BROWSER_USE_API_KEY", "prompt": "Browser Use API key", "url": "https://browser-use.com"}, - ], - "browser_provider": "browser-use", - "post_setup": "agent_browser", - }, - { - "name": "Firecrawl", - "badge": "paid", - "tag": "Cloud browser with remote execution", - "env_vars": [ - {"key": "FIRECRAWL_API_KEY", "prompt": "Firecrawl API key", "url": "https://firecrawl.dev"}, - ], - "browser_provider": "firecrawl", - "post_setup": "agent_browser", - }, { "name": "Camofox", "badge": "free · local", ``` ```diff @@ -1662,6 +1642,61 @@ def _plugin_web_search_providers() -> list[dict]: return rows +# Mirror of _plugin_web_search_providers for cloud browser backends. After +# PR #25214, Browserbase / Browser Use / Firecrawl live as plugins under +# plugins/browser//; this helper is the sole source of provider rows +# for those three in the "Browser Automation" picker. The hardcoded +# ``TOOL_CATEGORIES["browser"]`` entries that drove the category before +# were deleted in the same PR; only non-provider UX setup-flow rows remain +# ("Nous Subscription", "Local Browser", "Camofox") — see the comment block +# in ``TOOL_CATEGORIES["browser"]`` for why each one stays hardcoded. +def _plugin_browser_providers() -> list[dict]: + """Build picker-row dicts from plugin-registered cloud browser providers. + + Each returned dict mirrors the legacy ``TOOL_CATEGORIES["browser"]`` + schema (``name`` / ``badge`` / ``tag`` / ``env_vars`` / + ``browser_provider`` / ``post_setup``) so the picker behaves identically + whether a provider was hardcoded or plugin-registered. + + Populates ``browser_provider`` (the legacy config key written to + ``browser.cloud_provider``) and a ``browser_plugin_name`` marker so + setup / write paths can route through the registry when they want to. + """ + try: + from agent.browser_registry import list_providers as _list_browser_providers + from hermes_cli.plugins import _ensure_plugins_discovered + + _ensure_plugins_discovered() + providers = _list_browser_providers() + except Exception: + return [] + + rows: list[dict] = [] + for provider in providers: + name = getattr(provider, "name", None) + if not name: + continue + try: + schema = provider.get_setup_schema() + except Exception: + continue + if not isinstance(schema, dict): + continue + row = { + "name": schema.get("name", provider.display_name), + "badge": schema.get("badge", ""), + "tag": schema.get("tag", ""), + "env_vars": schema.get("env_vars", []), + "browser_provider": name, + "browser_plugin_name": name, + } + # Pass-through optional fields the schema can opt into. + if schema.get("post_setup"): + row["post_setup"] = schema["post_setup"] + rows.append(row) + return rows + + def _visible_providers(cat: dict, config: dict) -> list[dict]: """Return provider entries visible for the current auth/config state.""" features = get_nous_subscription_features(config) ``` ```diff @@ -1691,6 +1726,14 @@ def _visible_providers(cat: dict, config: dict) -> list[dict]: if cat.get("name") == "Web Search & Extract": visible.extend(_plugin_web_search_providers()) + # Inject plugin-registered cloud browser backends. After PR #25214, + # Browserbase / Browser Use / Firecrawl are the plugin-supplied rows; + # the hardcoded "Nous Subscription" / "Local Browser" / "Camofox" rows + # stay because they're non-provider UX setup flows (subscription auth, + # local fallback, and the REST-API anti-detection backend respectively). + if cat.get("name") == "Browser Automation": + visible.extend(_plugin_browser_providers()) + return visible ``` - `plugins/browser/browser_use/__init__.py` ```diff @@ -0,0 +1,14 @@ +"""Browser Use cloud browser plugin — bundled, auto-loaded. + +Mirrors the ``plugins/web//`` layout: ``provider.py`` holds the +provider class; ``__init__.py::register`` instantiates and registers it. +""" + +from __future__ import annotations + +from plugins.browser.browser_use.provider import BrowserUseBrowserProvider + + +def register(ctx) -> None: + """Register the Browser Use provider with the plugin context.""" + ctx.register_browser_provider(BrowserUseBrowserProvider()) ``` - `plugins/browser/browser_use/plugin.yaml` ```diff @@ -0,0 +1,7 @@ +name: browser-browser-use +version: 1.0.0 +description: "Browser Use (https://browser-use.com) cloud browser backend. Supports both direct BROWSER_USE_API_KEY and the managed Nous tool gateway. Also powers the 'Nous Subscription' UX flow that bills usage to a Nous subscription." +author: NousResearch +kind: backend +provides_browser_providers: + - browser-use ``` - `plugins/browser/browser_use/provider.py` ```diff @@ -1,4 +1,32 @@ -"""Browser Use cloud browser provider.""" +"""Browser Use cloud browser provider — plugin form. + +Subclasses :class:`agent.browser_provider.BrowserProvider` (the plugin-facing +ABC introduced in PR #25214). The legacy in-tree module +``tools.browser_providers.browser_use`` was removed in the same PR; this file +is now the canonical implementation. + +Browser Use is the only browser backend with dual auth: a direct +``BROWSER_USE_API_KEY`` for self-billed users, or the managed Nous tool +gateway (which Hermes uses to bill Browser Use sessions to a Nous +subscription). The dispatch order — direct API key first, managed gateway +second — preserves the pre-migration behaviour in +``tools.browser_providers.browser_use.BrowserUseProvider._get_config_or_none``. + +Config keys this provider responds to:: + + browser: + cloud_provider: "browser-use" # explicit selection + tool_gateway: + browser: "gateway" # optional: prefer managed gateway + # even when BROWSER_USE_API_KEY is set + +Auth env vars (one of):: + + BROWSER_USE_API_KEY=... # https://browser-use.com + # OR a managed Nous gateway entry (configured via 'hermes setup') +""" + +from __future__ import annotations import logging import os ``` ```diff @@ -8,11 +36,14 @@ import requests -from tools.browser_providers.base import CloudBrowserProvider -from tools.managed_tool_gateway import resolve_managed_tool_gateway -from tools.tool_backend_helpers import managed_nous_tools_enabled, prefers_gateway +from agent.browser_provider import BrowserProvider logger = logging.getLogger(__name__) + +# Idempotency tracking for managed-mode session creation. The managed Nous +# gateway returns 409 "already in progress" on retried POSTs; we forward the +# original idempotency key so the gateway can deduplicate. Cleared on +# success or terminal failure. _pending_create_keys: Dict[str, str] = {} _pending_create_keys_lock = threading.Lock() ``` ```diff @@ -38,6 +69,16 @@ def _clear_pending_create_key(task_id: str) -> None: def _should_preserve_pending_create_key(response: requests.Response) -> bool: + """Decide whether to keep the idempotency key after a failed create. + + Preserve the key when the failure looks retryable (5xx) OR when the + gateway reports the original request is still in flight (409 "already + in progress") — in either case, retrying with the same key lets the + gateway deduplicate. + + Drop the key on any other 4xx (auth failure, bad request, etc.) — those + won't succeed by being retried. + """ if response.status_code >= 500: return True ``` ```diff @@ -60,13 +101,24 @@ def _should_preserve_pending_create_key(response: requests.Response) -> bool: return "already in progress" in message -class BrowserUseProvider(CloudBrowserProvider): - """Browser Use (https://browser-use.com) cloud browser backend.""" +class BrowserUseBrowserProvider(BrowserProvider): + """Browser Use (https://browser-use.com) cloud browser backend. + + Dual auth: prefers a direct BROWSER_USE_API_KEY when set, falling back + to the managed Nous tool gateway when ``tool_gateway.browser`` config + routes through it. Setting ``tool_gateway.browser: gateway`` flips the + order so managed billing wins even when BROWSER_USE_API_KEY is present. + """ - def provider_name(self) -> str: + @property + def name(self) -> str: + return "browser-use" + + @property + def display_name(self) -> str: return "Browser Use" - def is_configured(self) -> bool: + def is_available(self) -> bool: return self._get_config_or_none() is not None # ------------------------------------------------------------------ ``` ```diff @@ -74,6 +126,14 @@ def is_configured(self) -> bool: # ------------------------------------------------------------------ def _get_config_or_none(self) -> Optional[Dict[str, Any]]: + # Import here to avoid a hard dependency at module-import time — + # managed_tool_gateway pulls in the Nous auth stack which can be + # heavy and is not needed for direct-API-key users. + from tools.managed_tool_gateway import resolve_managed_tool_gateway + from tools.tool_backend_helpers import prefers_gateway + + # Direct API key wins unless the user has explicitly opted into the + # managed Nous gateway via ``tool_gateway.browser: gateway``. api_key = os.environ.get("BROWSER_USE_API_KEY") if api_key and not prefers_gateway("browser"): return { ``` ```diff @@ -93,6 +153,8 @@ def _get_config_or_none(self) -> Optional[Dict[str, Any]]: } def _get_config(self) -> Dict[str, Any]: + from tools.tool_backend_helpers import managed_nous_tools_enabled + config = self._get_config_or_none() if config is None: message = ( ``` ```diff @@ -111,11 +173,10 @@ def _get_config(self) -> Dict[str, Any]: # ------------------------------------------------------------------ def _headers(self, config: Dict[str, Any]) -> Dict[str, str]: - headers = { + return { "Content-Type": "application/json", "X-Browser-Use-API-Key": [redacted:secret-assignment]"api_key"], } - return headers def create_session(self, task_id: str) -> Dict[str, object]: config = self._get_config() ``` ```diff @@ -166,7 +227,9 @@ def create_session(self, task_id: str) -> Dict[str, object]: if managed_mode: _clear_pending_create_key(task_id) session_name = f"hermes_{task_id}_{uuid.uuid4().hex[:8]}" - external_call_id = response.headers.get("x-external-call-id") if managed_mode else None + external_call_id = ( + response.headers.get("x-external-call-id") if managed_mode else None + ) logger.info("Created Browser Use session %s", session_name) ``` ```diff @@ -184,7 +247,9 @@ def close_session(self, session_id: str) -> bool: try: config = self._get_config() except ValueError: - logger.warning("Cannot close Browser Use session %s — missing credentials", session_id) + logger.warning( + "Cannot close Browser Use session %s — missing credentials", session_id + ) return False try: ``` ```diff @@ -212,7 +277,10 @@ def close_session(self, session_id: str) -> bool: def emergency_cleanup(self, session_id: str) -> None: config = self._get_config_or_none() if config is None: - logger.warning("Cannot emergency-cleanup Browser Use session %s — missing credentials", session_id) + logger.warning( + "Cannot emergency-cleanup Browser Use session %s — missing credentials", + session_id, + ) return try: requests.patch( ``` ```diff @@ -222,4 +290,21 @@ def emergency_cleanup(self, session_id: str) -> None: timeout=5, ) except Exception as e: - logger.debug("Emergency cleanup failed for Browser Use session %s: %s", session_id, e) + logger.debug( + "Emergency cleanup failed for Browser Use session %s: %s", session_id, e + ) + + def get_setup_schema(self) -> Dict[str, Any]: + return { + "name": "Browser Use", + "badge": "paid", + "tag": "Cloud browser with remote execution", + "env_vars": [ + { + "key": "BROWSER_USE_API_KEY", + "prompt": "Browser Use API key", + "url": "https://browser-use.com", + }, + ], + "post_setup": "agent_browser", + } ``` - `plugins/browser/browserbase/__init__.py` ```diff @@ -0,0 +1,15 @@ +"""Browserbase cloud browser plugin — bundled, auto-loaded. + +Mirrors the ``plugins/web//`` and ``plugins/image_gen/openai/`` +layout: ``provider.py`` holds the provider class; ``__init__.py::register`` +instantiates and registers it via the plugin context. +""" + +from __future__ import annotations + +from plugins.browser.browserbase.provider import BrowserbaseBrowserProvider + + +def register(ctx) -> None: + """Register the Browserbase provider with the plugin context.""" + ctx.register_browser_provider(BrowserbaseBrowserProvider()) ``` - `plugins/browser/browserbase/plugin.yaml` ```diff @@ -0,0 +1,7 @@ +name: browser-browserbase +version: 1.0.0 +description: "Browserbase (https://browserbase.com) cloud browser backend. Requires BROWSERBASE_API_KEY + BROWSERBASE_PROJECT_ID. Supports stealth, proxies, and keep-alive sessions; auto-falls-back when paid features are unavailable." +author: NousResearch +kind: backend +provides_browser_providers: + - browserbase ``` - `plugins/browser/browserbase/provider.py` ```diff @@ -1,4 +1,35 @@ -"""Browserbase cloud browser provider (direct credentials only).""" +"""Browserbase cloud browser provider — plugin form. + +Subclasses :class:`agent.browser_provider.BrowserProvider` (the plugin-facing +ABC introduced in PR #25214). The legacy in-tree module +``tools.browser_providers.browserbase`` was removed in the same PR; this file +is now the canonical implementation. + +Browserbase requires direct ``BROWSERBASE_API_KEY`` and ``BROWSERBASE_PROJECT_ID`` +credentials. Managed Nous gateway support has been removed — the Nous +subscription now routes through Browser Use instead (see +``plugins/browser/browser_use/``). + +Config keys this provider responds to:: + + browser: + cloud_provider: "browserbase" + +Auth env vars:: + + BROWSERBASE_API_KEY=... # https://browserbase.com + BROWSERBASE_PROJECT_ID=... + +Optional feature knobs:: + + BROWSERBASE_BASE_URL=... # default https://api.browserbase.com + BROWSERBASE_PROXIES=true # default true + BROWSERBASE_ADVANCED_STEALTH=false + BROWSERBASE_KEEP_ALIVE=true # default true + BROWSERBASE_SESSION_TIMEOUT=... (ms, integer) +""" + +from __future__ import annotations import logging import os ``` ```diff @@ -7,27 +38,31 @@ import requests -from tools.browser_providers.base import CloudBrowserProvider +from agent.browser_provider import BrowserProvider logger = logging.getLogger(__name__) -class BrowserbaseProvider(CloudBrowserProvider): +class BrowserbaseBrowserProvider(BrowserProvider): """Browserbase (https://browserbase.com) cloud browser backend. - This provider requires direct BROWSERBASE_API_KEY and BROWSERBASE_PROJECT_ID - credentials. Managed Nous gateway support has been removed — the Nous - subscription now routes through Browser Use instead. + Direct credentials only — managed-Nous-gateway support lives on the + Browser Use provider now. """ - def provider_name(self) -> str: + @property + def name(self) -> str: + return "browserbase" + + @property + def display_name(self) -> str: return "Browserbase" - def is_configured(self) -> bool: + def is_available(self) -> bool: return self._get_config_or_none() is not None # ------------------------------------------------------------------ - # Session lifecycle + # Config resolution # ------------------------------------------------------------------ def _get_config_or_none(self) -> Optional[Dict[str, Any]]: ``` ```diff @@ -37,7 +72,9 @@ def _get_config_or_none(self) -> Optional[Dict[str, Any]]: return { "api_key": [redacted:secret-assignment] "project_id": project_id, - "base_url": os.environ.get("BROWSERBASE_BASE_URL", "https://api.browserbase.com").rstrip("/"), + "base_url": os.environ.get( + "BROWSERBASE_BASE_URL", "https://api.browserbase.com" + ).rstrip("/"), } return None ``` ```diff @@ -50,13 +87,21 @@ def _get_config(self) -> Dict[str, Any]: ) return config + # ------------------------------------------------------------------ + # Session lifecycle + # ------------------------------------------------------------------ + def create_session(self, task_id: str) -> Dict[str, object]: config = self._get_config() # Optional env-var knobs enable_proxies = os.environ.get("BROWSERBASE_PROXIES", "true").lower() != "false" - enable_advanced_stealth = os.environ.get("BROWSERBASE_ADVANCED_STEALTH", "false").lower() == "true" - enable_keep_alive = os.environ.get("BROWSERBASE_KEEP_ALIVE", "true").lower() != "false" + enable_advanced_stealth = ( + os.environ.get("BROWSERBASE_ADVANCED_STEALTH", "false").lower() == "true" + ) + enable_keep_alive = ( + os.environ.get("BROWSERBASE_KEEP_ALIVE", "true").lower() != "false" + ) custom_timeout_ms = os.environ.get("BROWSERBASE_SESSION_TIMEOUT") features_enabled = { ``` ```diff @@ -78,7 +123,9 @@ def create_session(self, task_id: str) -> Dict[str, object]: if timeout_val > 0: session_config["timeout"] = timeout_val except ValueError: - logger.warning("Invalid BROWSERBASE_SESSION_TIMEOUT value: %s", custom_timeout_ms) + logger.warning( + "Invalid BROWSERBASE_SESSION_TIMEOUT value: %s", custom_timeout_ms + ) if enable_proxies: session_config["proxies"] = True ``` ```diff @@ -156,7 +203,9 @@ def create_session(self, task_id: str) -> Dict[str, object]: features_enabled["custom_timeout"] = True feature_str = ", ".join(k for k, v in features_enabled.items() if v) - logger.info("Created Browserbase session %s with features: %s", session_name, feature_str) + logger.info( + "Created Browserbase session %s with features: %s", session_name, feature_str + ) return { "session_name": session_name, ``` ```diff @@ -169,7 +218,9 @@ def close_session(self, session_id: str) -> bool: try: config = self._get_config() except ValueError: - logger.warning("Cannot close Browserbase session %s — missing credentials", session_id) + logger.warning( + "Cannot close Browserbase session %s — missing credentials", session_id + ) return False try: ``` ```diff @@ -203,7 +254,10 @@ def close_session(self, session_id: str) -> bool: def emergency_cleanup(self, session_id: str) -> None: config = self._get_config_or_none() if config is None: - logger.warning("Cannot emergency-cleanup Browserbase session %s — missing credentials", session_id) + logger.warning( + "Cannot emergency-cleanup Browserbase session %s — missing credentials", + session_id, + ) return try: requests.post( ``` ```diff @@ -219,4 +273,25 @@ def emergency_cleanup(self, session_id: str) -> None: timeout=5, ) except Exception as e: - logger.debug("Emergency cleanup failed for Browserbase session %s: %s", session_id, e) + logger.debug( + "Emergency cleanup failed for Browserbase session %s: %s", session_id, e + ) + + def get_setup_schema(self) -> Dict[str, Any]: + return { + "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", + }, + { + "key": "BROWSERBASE_PROJECT_ID", + "prompt": "Browserbase project ID", + }, + ], + "post_setup": "agent_browser", + } ``` - `plugins/browser/firecrawl/__init__.py` ```diff @@ -0,0 +1,16 @@ +"""Firecrawl cloud browser plugin — bundled, auto-loaded. + +Distinct from ``plugins/web/firecrawl/`` (the web search/extract/crawl +plugin); both share the FIRECRAWL_API_KEY but speak to different endpoints +(``/v2/browser`` here vs ``/v2/search`` / ``/v2/scrape`` / ``/v2/crawl`` +over there). +""" + +from __future__ import annotations + +from plugins.browser.firecrawl.provider import FirecrawlBrowserProvider + + +def register(ctx) -> None: + """Register the Firecrawl cloud-browser provider with the plugin context.""" + ctx.register_browser_provider(FirecrawlBrowserProvider()) ``` - `plugins/browser/firecrawl/plugin.yaml` ```diff @@ -0,0 +1,7 @@ +name: browser-firecrawl +version: 1.0.0 +description: "Firecrawl (https://firecrawl.dev) cloud browser backend. Requires FIRECRAWL_API_KEY. Distinct from the firecrawl WEB search/extract plugin — the two share an API key but operate on different endpoints." +author: NousResearch +kind: backend +provides_browser_providers: + - firecrawl ``` - `plugins/browser/firecrawl/provider.py` ```diff @@ -1,26 +1,61 @@ -"""Firecrawl cloud browser provider.""" +"""Firecrawl cloud browser provider — plugin form. + +Subclasses :class:`agent.browser_provider.BrowserProvider` (the plugin-facing +ABC introduced in PR #25214). The legacy in-tree module +``tools.browser_providers.firecrawl`` was removed in the same PR; this file +is now the canonical implementation. + +This is the cloud-browser path — distinct from the firecrawl WEB plugin at +``plugins/web/firecrawl/`` which handles search/extract/crawl on +``/v2/search`` / ``/v2/scrape`` / ``/v2/crawl``. The two plugins share the +``FIRECRAWL_API_KEY`` env var but talk to different endpoints (this one +hits ``/v2/browser``). + +Config keys this provider responds to:: + + browser: + cloud_provider: "firecrawl" # explicit selection only — not in the + # legacy auto-detect walk + +Auth env vars:: + + FIRECRAWL_API_KEY=... # https://firecrawl.dev + FIRECRAWL_API_URL=... # optional override (default https://api.firecrawl.dev) + FIRECRAWL_BROWSER_TTL=... # optional, default 300 seconds +""" + +from __future__ import annotations import logging import os import uuid -from typing import Dict +from typing import Any, Dict import requests -from tools.browser_providers.base import CloudBrowserProvider +from agent.browser_provider import BrowserProvider logger = logging.getLogger(__name__) _BASE_URL = "https://api.firecrawl.dev" -class FirecrawlProvider(CloudBrowserProvider): - """Firecrawl (https://firecrawl.dev) cloud browser backend.""" +class FirecrawlBrowserProvider(BrowserProvider): + """Firecrawl (https://firecrawl.dev) cloud browser backend. - def provider_name(self) -> str: + Cloud-browser path only — search/extract/crawl live in the separate + ``plugins/web/firecrawl/`` plugin. + """ + + @property + def name(self) -> str: + return "firecrawl" + + @property + def display_name(self) -> str: return "Firecrawl" - def is_configured(self) -> bool: + def is_available(self) -> bool: return bool(os.environ.get("FIRECRAWL_API_KEY")) # ------------------------------------------------------------------ ``` ```diff @@ -100,13 +135,34 @@ def close_session(self, session_id: str) -> bool: return False def emergency_cleanup(self, session_id: str) -> None: + if not self.is_available(): + logger.warning( + "Cannot emergency-cleanup Firecrawl session %s — missing credentials", + session_id, + ) + return try: requests.delete( f"{self._api_url()}/v2/browser/{session_id}", headers=self._headers(), timeout=5, ) - except ValueError: - logger.warning("Cannot emergency-cleanup Firecrawl session %s — missing credentials", session_id) except Exception as e: - logger.debug("Emergency cleanup failed for Firecrawl session %s: %s", session_id, e) + logger.debug( + "Emergency cleanup failed for Firecrawl session %s: %s", session_id, e + ) + + def get_setup_schema(self) -> Dict[str, Any]: + return { + "name": "Firecrawl", + "badge": "paid", + "tag": "Cloud browser with remote execution", + "env_vars": [ + { + "key": "FIRECRAWL_API_KEY", + "prompt": "Firecrawl API key", + "url": "https://firecrawl.dev", + }, + ], + "post_setup": "agent_browser", + } ``` - `tests/plugins/browser/check_parity_vs_main.py` ```diff @@ -0,0 +1,273 @@ +"""Behavior-parity check for the browser-provider plugin migration (#25214). + +Spawns one subprocess per (version, scenario) cell — pinned to either +origin/main (legacy in-tree providers + class-instantiation lookup) or +this PR's worktree (plugin-based registry) via `sys.path[0]`. Each +subprocess clears all browser-related env vars + writes a config.yaml, +loads `tools.browser_tool._get_cloud_provider()`, and emits a reduced +"shape tuple" {is_local, provider_name, is_available} as JSON. + +The parent process diffs the shapes per scenario. A diff means the +migration introduced an observable behaviour change vs origin/main — +which would be a real regression for users on the existing config keys. + +Run from the PR worktree: + + cd ~/.hermes/hermes-agent/.worktrees/browser-providers-plugin + python tests/plugins/browser/check_parity_vs_main.py +""" +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[3] + + +# Pin one path to current main, one to the PR worktree. +# ``REPO_ROOT`` is ``.../.worktrees/browser-providers-plugin``; the main +# checkout lives two levels up at ``~/.hermes/hermes-agent``. +MAIN_DIR = REPO_ROOT.parent.parent # ~/.hermes/hermes-agent +PR_DIR = REPO_ROOT # the worktree we're in +assert (MAIN_DIR / "tools" / "browser_tool.py").exists(), ( + f"MAIN_DIR={MAIN_DIR} doesn't look like a hermes-agent checkout" +) +assert (PR_DIR / "tools" / "browser_tool.py").exists(), ( + f"PR_DIR={PR_DIR} doesn't look like a hermes-agent checkout" +) + + +# Reduced shape comparison — exact instance addresses obviously differ +# between subprocesses, so we compare the parts that matter for users. +SUBPROCESS_SCRIPT = r""" +import json, os, sys, tempfile +sys.path.insert(0, sys.argv[1]) + +# Isolated HERMES_HOME for the config write. +home = tempfile.mkdtemp() +os.environ["HERMES_HOME"] = home + +# Clear every browser-related env var so is_available() is deterministic. +for k in ( + "BROWSERBASE_API_KEY", "BROWSERBASE_PROJECT_ID", "BROWSERBASE_BASE_URL", + "BROWSER_USE_API_KEY", "BROWSER_USE_GATEWAY_URL", + "FIRECRAWL_API_KEY", "FIRECRAWL_API_URL", "FIRECRAWL_BROWSER_TTL", + "TOOL_GATEWAY_DOMAIN", "TOOL_GATEWAY_USER_TOKEN", +): + os.environ.pop(k, None) + +# Apply per-scenario env (passed as JSON via argv[2]). +scenario_env = json.loads(sys.argv[2]) +os.environ.update(scenario_env) + +# Apply per-scenario config (passed as YAML body via argv[3]). +config_yaml = sys.argv[3] +config_path = os.path.join(home, "config.yaml") +with open(config_path, "w") as f: + f.write(config_yaml) + +# Fresh import — must not have any browser modules cached. +for name in list(sys.modules): + if name.startswith("tools.") or name.startswith("agent.") or name.startswith("plugins."): + sys.modules.pop(name, None) + +from tools.browser_tool import _get_cloud_provider, _is_local_mode + +provider = _get_cloud_provider() + +# Pull the human-readable backend name via the API that exists on BOTH +# legacy (origin/main: CloudBrowserProvider.provider_name()) and the new +# ABC (BrowserProvider exposes provider_name() as a backward-compat alias +# returning display_name). Both shapes resolve to the same string — +# 'Browserbase' / 'Browser Use' / 'Firecrawl' — so we can compare safely. +provider_name = None +is_available = None +if provider is not None: + pn = getattr(provider, "provider_name", None) + if callable(pn): + provider_name = pn() + elif isinstance(pn, str): + provider_name = pn + is_conf = getattr(provider, "is_configured", None) + if callable(is_conf): + is_available = bool(is_conf()) + +shape = { + "is_local": _is_local_mode(), + "provider_name": provider_name, + "is_available": is_available, +} +print(json.dumps(shape)) +""" + + +SCENARIOS: list[tuple[str, str, dict[str, str]]] = [ + # (label, config.yaml body, extra env vars) + ("no-config-no-env", "", {}), + ("explicit-local-no-env", "browser:\n cloud_provider: local\n", {}), + ( + "explicit-browserbase-no-creds", + "browser:\n cloud_provider: browserbase\n", + {}, + ), + ( + "explicit-browserbase-with-creds", + "browser:\n cloud_provider: browserbase\n", + {"BROWSERBASE_API_KEY": "x", "BROWSERBASE_PROJECT_ID": "y"}, + ), + ( + "explicit-browser-use-no-creds", + "browser:\n cloud_provider: browser-use\n", + {}, + ), + ( + "explicit-browser-use-with-creds", + "browser:\n cloud_provider: browser-use\n", + {"BROWSER_USE_API_KEY": "k"}, + ), + ( + "explicit-firecrawl-no-creds", + "browser:\n cloud_provider: firecrawl\n", + {}, + ), + ( + "explicit-firecrawl-with-creds", + "browser:\n cloud_provider: firecrawl\n", + {"FIRECRAWL_API_KEY": "k"}, + ), + ( + "no-config-bu-creds", + "", + {"BROWSER_USE_API_KEY": "k"}, + ), + ( + "no-config-bb-creds", + "", + {"BROWSERBASE_API_KEY": "x", "BROWSERBASE_PROJECT_ID": "y"}, + ), + ( + "no-config-both-creds", + "", + { + "BROWSER_USE_API_KEY": "k", + "BROWSERBASE_API_KEY": "x", + "BROWSERBASE_PROJECT_ID": "y", + }, + ), + ( + "no-config-firecrawl-only", + "", + {"FIRECRAWL_API_KEY": "k"}, + ), + ( + "no-config-firecrawl-and-bb", + "", + { + "FIRECRAWL_API_KEY": "k", + "BROWSERBASE_API_KEY": "x", + "BROWSERBASE_PROJECT_ID": "y", + }, + ), +] + + +def _run_scenario(repo_path: Path, label: str, config_yaml: str, env: dict) -> dict: + """Run one (version, scenario) cell. Returns the shape dict.""" + venv_python = repo_path / ".venv" / "bin" / "python" + if not venv_python.exists(): + # Worktrees share the main repo's venv. + venv_python = MAIN_DIR / ".venv" / "bin" / "python" + if not venv_python.exists(): + venv_python = Path("python3") + + out = subprocess.run( + [ + str(venv_python), + "-c", + SUBPROCESS_SCRIPT, + str(repo_path), + json.dumps(env), + config_yaml, + ], + capture_output=True, + text=True, + timeout=30, + ) + if out.returncode != 0: + return { + "error": "subprocess failed", + "stdout": out.stdout, + "stderr": out.stderr[-500:], + } + try: + return json.loads(out.stdout.strip().splitlines()[-1]) + except Exception as exc: + return {"error": f"could not parse output: {exc}", "stdout": out.stdout} + + +def _reduce_for_comparison(shape: dict) -> dict: + """Reduce a shape dict to the parts that matter for user-visible parity. + + We compare ``(is_local, provider_name, is_available)`` — the trio that + decides what the dispatcher does with each tool call. ``provider_name`` + is the legacy ``provider_name()`` return value ('Browserbase' / 'Browser + Use' / 'Firecrawl'), which is identical between legacy and plugin + classes (the plugin's ``display_name`` matches the legacy + ``provider_name()`` return). + """ + return { + "is_local": shape.get("is_local"), + "provider_name": shape.get("provider_name"), + "is_available": shape.get("is_available"), + } + + +def main() -> int: + print(f"main: {MAIN_DIR}") + print(f"pr: {PR_DIR}") + print() + + failures: list[str] = [] + errors: list[str] = [] + for label, config_yaml, env in SCENARIOS: + main_shape = _run_scenario(MAIN_DIR, label, config_yaml, env) + pr_shape = _run_scenario(PR_DIR, label, config_yaml, env) + + if "error" in main_shape or "error" in pr_shape: + print(f" [ERR ] {label}: subprocess failed") + print(f" main: {main_shape}") + print(f" pr: {pr_shape}") + errors.append(label) + continue + + main_reduced = _reduce_for_comparison(main_shape) + pr_reduced = _reduce_for_comparison(pr_shape) + + if main_reduced == pr_reduced: + print(f" [OK] {label}: {main_reduced}") + else: + print(f" [FAIL] {label}") + print(f" main: {main_reduced}") + print(f" pr: {pr_reduced}") + failures.append(label) + + print() + if errors: + print(f"SUBPROCESS ERRORS in {len(errors)} scenario(s):") + for e in errors: + print(f" - {e}") + if failures: + print(f"BEHAVIOUR REGRESSION in {len(failures)} scenario(s):") + for f in failures: + print(f" - {f}") + if failures or errors: + return 1 + print(f"PARITY OK across {len(SCENARIOS)} scenarios.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) ``` - `tests/plugins/browser/test_browser_provider_plugins.py` ```diff @@ -0,0 +1,379 @@ +"""Plugin-side tests for the browser provider migration (PR #25214). + +Covers: + +- All three bundled plugins (browserbase, browser-use, firecrawl) + instantiate and self-report the expected ABC defaults. +- Each plugin's ``is_available()`` correctly reflects env-var presence. +- The browser_registry resolves an active provider in the documented + scenarios: + * explicit config wins ignoring availability (so dispatcher surfaces + a typed credentials error) + * legacy preference walk: browser-use → browserbase (filtered by + availability) + * firecrawl is NOT in the legacy walk — explicit-only + * unknown name falls through to auto-detect + * ``local`` short-circuits to None + +These tests use *real* imports from the plugin modules — no mocking of +provider classes themselves — so the test catches drift in the ABC +interface, the registry, and the plugin glue layer simultaneously. +Mirrors ``tests/plugins/web/test_web_search_provider_plugins.py`` from +PR #25182. +""" +from __future__ import annotations + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _clear_browser_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Strip every browser-provider env var so is_available() returns False.""" + for k in ( + "BROWSERBASE_API_KEY", + "BROWSERBASE_PROJECT_ID", + "BROWSERBASE_BASE_URL", + "BROWSER_USE_API_KEY", + "BROWSER_USE_GATEWAY_URL", + "FIRECRAWL_API_KEY", + "FIRECRAWL_API_URL", + "FIRECRAWL_BROWSER_TTL", + "TOOL_GATEWAY_DOMAIN", + "TOOL_GATEWAY_USER_TOKEN", + ): + monkeypatch.delenv(k, raising=False) + + +def _ensure_plugins_loaded() -> None: + """Idempotently load plugins so the registry is populated.""" + from hermes_cli.plugins import _ensure_plugins_discovered + + _ensure_plugins_discovered() + + +# --------------------------------------------------------------------------- +# Per-test isolation +# --------------------------------------------------------------------------- + + [redacted:email](autouse=True) +def _isolate_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Each test starts with a clean browser-provider env.""" + _clear_browser_env(monkeypatch) + + +# --------------------------------------------------------------------------- +# Bundled plugins register +# --------------------------------------------------------------------------- + + +class TestBundledPluginsRegister: + """All three bundled browser plugins discover and register correctly.""" + + def test_all_three_plugins_present_in_registry(self) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import list_providers + + names = sorted(p.name for p in list_providers()) + assert names == ["browser-use", "browserbase", "firecrawl"] + + @pytest.mark.parametrize( + "plugin_name,expected_display", + [ + ("browserbase", "Browserbase"), + ("browser-use", "Browser Use"), + ("firecrawl", "Firecrawl"), + ], + ) + def test_each_plugin_has_name_and_display_name( + self, plugin_name: str, expected_display: str + ) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + provider = get_provider(plugin_name) + assert provider is not None, f"plugin {plugin_name!r} not registered" + assert provider.name == plugin_name + assert provider.display_name == expected_display + + @pytest.mark.parametrize( + "plugin_name", + ["browserbase", "browser-use", "firecrawl"], + ) + def test_each_plugin_has_setup_schema(self, plugin_name: str) -> None: + """``get_setup_schema()`` returns a dict the picker can consume.""" + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + provider = get_provider(plugin_name) + assert provider is not None + schema = provider.get_setup_schema() + assert isinstance(schema, dict) + assert "name" in schema + assert "env_vars" in schema + # Every cloud-browser plugin needs the agent-browser post-setup hook + # so the picker auto-installs the CLI on selection. + assert schema.get("post_setup") == "agent_browser" + + @pytest.mark.parametrize( + "plugin_name", + ["browserbase", "browser-use", "firecrawl"], + ) + def test_each_plugin_implements_full_lifecycle(self, plugin_name: str) -> None: + """The ABC's three lifecycle methods are all overridden.""" + _ensure_plugins_loaded() + from agent.browser_provider import BrowserProvider + from agent.browser_registry import get_provider + + provider = get_provider(plugin_name) + assert provider is not None + # Each method must be a real override, not the ABC's NotImplementedError + # default — we check by comparing the function reference. + assert type(provider).create_session is not BrowserProvider.create_session + assert type(provider).close_session is not BrowserProvider.close_session + assert ( + type(provider).emergency_cleanup is not BrowserProvider.emergency_cleanup + ) + + +# --------------------------------------------------------------------------- +# is_available() behavior +# --------------------------------------------------------------------------- + + +class TestIsAvailable: + """Each plugin's ``is_available()`` reflects env-var presence accurately.""" + + def test_browserbase_requires_both_api_key_and_project_id( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider("browserbase") + assert p is not None + assert p.is_available() is False + + # API key alone is insufficient. + monkeypatch.setenv("BROWSERBASE_API_KEY", "key") + assert p.is_available() is False + + # Both env vars set → available. + monkeypatch.setenv("BROWSERBASE_PROJECT_ID", "proj") + assert p.is_available() is True + + def test_browserbase_project_id_alone_insufficient( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider("browserbase") + assert p is not None + monkeypatch.setenv("BROWSERBASE_PROJECT_ID", "proj") + assert p.is_available() is False + + def test_browser_use_satisfied_by_api_key( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider("browser-use") + assert p is not None + assert p.is_available() is False + monkeypatch.setenv("BROWSER_USE_API_KEY", "key") + assert p.is_available() is True + + def test_firecrawl_requires_api_key(self, monkeypatch: pytest.MonkeyPatch) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider("firecrawl") + assert p is not None + assert p.is_available() is False + monkeypatch.setenv("FIRECRAWL_API_KEY", "key") + assert p.is_available() is True + + +# --------------------------------------------------------------------------- +# Registry resolution semantics +# --------------------------------------------------------------------------- + + +class TestRegistryResolution: + """``_resolve()`` implements the documented three-rule precedence.""" + + def test_resolve_none_with_no_creds_returns_none(self) -> None: + """No config, no env → local mode (None).""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + assert _resolve(None) is None + + def test_explicit_local_returns_none(self) -> None: + """``cloud_provider: local`` is a positive choice; short-circuits to None.""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + assert _resolve("local") is None + + def test_explicit_browserbase_returns_provider_even_when_unavailable(self) -> None: + """Rule 1: explicit-config wins even when credentials are missing. + + This is critical — the dispatcher needs to surface a typed + credentials error rather than silently switching backends. + """ + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + provider = _resolve("browserbase") + assert provider is not None + assert provider.name == "browserbase" + assert provider.is_available() is False # confirms "ignoring availability" + + def test_explicit_firecrawl_returns_provider_even_when_unavailable(self) -> None: + """Firecrawl behaves the same as browserbase under explicit config.""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + provider = _resolve("firecrawl") + assert provider is not None + assert provider.name == "firecrawl" + + def test_explicit_unknown_falls_back_to_auto_detect(self) -> None: + """Rule 1 miss: unknown name → fall through to legacy walk.""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + # With no credentials anywhere, auto-detect should also fail. + assert _resolve("not-a-real-provider") is None + + def test_legacy_walk_prefers_browser_use_over_browserbase( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Rule 3: walk order is browser-use → browserbase.""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + # Both available — browser-use should win. + monkeypatch.setenv("BROWSER_USE_API_KEY", "k1") + monkeypatch.setenv("BROWSERBASE_API_KEY", "k2") + monkeypatch.setenv("BROWSERBASE_PROJECT_ID", "p") + + provider = _resolve(None) + assert provider is not None + assert provider.name == "browser-use" + + def test_legacy_walk_falls_through_to_browserbase( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Rule 3: browser-use unavailable → browserbase picked.""" + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + monkeypatch.setenv("BROWSERBASE_API_KEY", "k") + monkeypatch.setenv("BROWSERBASE_PROJECT_ID", "p") + + provider = _resolve(None) + assert provider is not None + assert provider.name == "browserbase" + + def test_firecrawl_not_in_legacy_walk_even_when_only_one_available( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Regression: firecrawl is NEVER auto-selected even when single-eligible. + + Pre-PR-#25214, the dispatcher only auto-detected between Browser Use + and Browserbase; firecrawl was reachable solely via explicit + config. We preserve that gate because FIRECRAWL_API_KEY is shared + with the *web* firecrawl plugin — auto-routing a web-extract user + to a paid cloud browser would be a real behaviour regression. + """ + _ensure_plugins_loaded() + from agent.browser_registry import _resolve + + monkeypatch.setenv("FIRECRAWL_API_KEY", "k") + + # Only firecrawl is_available() — but it's not in the legacy walk. + assert _resolve(None) is None + + +# --------------------------------------------------------------------------- +# Legacy ABC backward-compat aliases (is_configured / provider_name) +# --------------------------------------------------------------------------- + + +class TestLegacyAbcAliases: + """is_configured() and provider_name() delegate to the new API.""" + + @pytest.mark.parametrize( + "plugin_name", + ["browserbase", "browser-use", "firecrawl"], + ) + def test_is_configured_delegates_to_is_available(self, plugin_name: str) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider(plugin_name) + assert p is not None + assert p.is_configured() is p.is_available() + + @pytest.mark.parametrize( + "plugin_name,expected_label", + [ + ("browserbase", "Browserbase"), + ("browser-use", "Browser Use"), + ("firecrawl", "Firecrawl"), + ], + ) + def test_provider_name_returns_display_name( + self, plugin_name: str, expected_label: str + ) -> None: + _ensure_plugins_loaded() + from agent.browser_registry import get_provider + + p = get_provider(plugin_name) + assert p is not None + assert p.provider_name() == expected_label + + +# --------------------------------------------------------------------------- +# Picker integration +# --------------------------------------------------------------------------- + + +class TestPickerIntegration: + """`_plugin_browser_providers()` exposes all three plugins as picker rows.""" + + def test_picker_rows_match_registered_plugins(self) -> None: + _ensure_plugins_loaded() + from hermes_cli.tools_config import _plugin_browser_providers + + rows = _plugin_browser_providers() + names = sorted(r.get("browser_provider") for r in rows) + assert names == ["browser-use", "browserbase", "firecrawl"] + + def test_picker_rows_carry_post_setup_hook(self) -> None: + """Every browser plugin row has post_setup='agent_browser' so + selecting it triggers the agent-browser CLI install.""" + _ensure_plugins_loaded() + from hermes_cli.tools_config import _plugin_browser_providers + + for row in _plugin_browser_providers(): + assert row.get("post_setup") == "agent_browser", ( + f"plugin row {row['browser_provider']!r} missing post_setup hook" + ) + + def test_picker_rows_carry_browser_plugin_name_marker(self) -> None: + """`browser_plugin_name` matches `browser_provider` so downstream + code can route through the registry when it wants to.""" + _ensure_plugins_loaded() + from hermes_cli.tools_config import _plugin_browser_providers + + for row in _plugin_browser_providers(): + assert row.get("browser_plugin_name") == row.get("browser_provider") ``` - `tests/tools/test_managed_browserbase_and_modal.py` ```diff @@ -10,7 +10,9 @@ import pytest -TOOLS_DIR = Path(__file__).resolve().parents[2] / "tools" +REPO_ROOT = Path(__file__).resolve().parents[2] +TOOLS_DIR = REPO_ROOT / "tools" +PLUGINS_DIR = REPO_ROOT / "plugins" def _load_tool_module(module_name: str, filename: str): ``` ```diff @@ -22,6 +24,21 @@ def _load_tool_module(module_name: str, filename: str): return module +def _load_plugin_module(module_name: str, relpath: str): + """Load a plugin module by file path from ``plugins/``. + + Mirror of :func:`_load_tool_module` for the plugin tree. Used by tests + that exercise the per-vendor browser plugins' session-lifecycle + behaviour after the PR #25214 migration. + """ + spec = spec_from_file_location(module_name, PLUGINS_DIR / relpath) + assert spec and spec.loader + module = module_from_spec(spec) + sys.modules[module_name] = module + spec.loader.exec_module(module) + return module + + def _reset_modules(prefixes: tuple[str, ...]): for name in list(sys.modules): if name.startswith(prefixes): ``` ```diff @@ -76,6 +93,48 @@ def _install_fake_tools_package(): call_llm=lambda *args, **kwargs: "", ) + # Stubs for the browser-provider plugin layer introduced in PR #25214. + # The fake `agent` package has an empty __path__ so real submodules + # aren't reachable; we install just enough stand-ins to satisfy + # ``tools.browser_tool``'s top-level imports. The actual lifecycle + # tests instantiate the real plugin classes via _load_tool_module + # below, so the stubs only need to satisfy import + isinstance. + class _StubBrowserProvider: + """Minimal BrowserProvider stub for ``from agent.browser_provider import BrowserProvider``.""" + + sys.modules["agent.browser_provider"] = types.SimpleNamespace( + BrowserProvider=_StubBrowserProvider, + ) + sys.modules["agent.browser_registry"] = types.SimpleNamespace( + get_provider=lambda name: None, + list_providers=lambda: [], + register_provider=lambda provider: None, + _resolve=lambda configured: None, + ) + + # Plugin module stubs — the real plugin classes are loaded from disk by + # the lifecycle tests below via _load_tool_module(). For the import + # phase, we just need the class names to exist on the right module path. + plugins_package = types.ModuleType("plugins") + plugins_package.__path__ = [] # type: ignore[attr-defined] + sys.modules["plugins"] = plugins_package + plugins_browser_package = types.ModuleType("plugins.browser") + plugins_browser_package.__path__ = [] # type: ignore[attr-defined] + sys.modules["plugins.browser"] = plugins_browser_package + + for _name, _classname in ( + ("browserbase", "BrowserbaseBrowserProvider"), + ("browser_use", "BrowserUseBrowserProvider"), + ("firecrawl", "FirecrawlBrowserProvider"), + ): + _vendor_pkg = types.ModuleType(f"plugins.browser.{_name}") + _vendor_pkg.__path__ = [] # type: ignore[attr-defined] + sys.modules[f"plugins.browser.{_name}"] = _vendor_pkg + _provider_stub_cls = type(_classname, (_StubBrowserProvider,), {}) + sys.modules[f"plugins.browser.{_name}.provider"] = types.SimpleNamespace( + **{_classname: _provider_stub_cls}, + ) + sys.modules["tools.managed_tool_gateway"] = _load_tool_module( "tools.managed_tool_gateway", "managed_tool_gateway.py", ``` ```diff @@ -157,13 +216,13 @@ def test_browserbase_does_not_use_gateway_only_configuration(): }) with patch.dict(os.environ, env, clear=True): - browserbase_module = _load_tool_module( - "tools.browser_providers.browserbase", - "browser_providers/browserbase.py", + browserbase_module = _load_plugin_module( + "plugins.browser.browserbase.provider", + "browser/browserbase/provider.py", ) - provider = browserbase_module.BrowserbaseProvider() + provider = browserbase_module.BrowserbaseBrowserProvider() - assert provider.is_configured() is False + assert provider.is_available() is False def test_browser_use_managed_gateway_adds_idempotency_key_and_persists_external_call_id(): ``` ```diff @@ -188,13 +247,13 @@ def json(self): } with patch.dict(os.environ, env, clear=True): - browser_use_module = _load_tool_module( - "tools.browser_providers.browser_use", - "browser_providers/browser_use.py", + browser_use_module = _load_plugin_module( + "plugins.browser.browser_use.provider", + "browser/browser_use/provider.py", ) with patch.object(browser_use_module.requests, "post", return_value=_Response()) as post: - provider = browser_use_module.BrowserUseProvider() + provider = browser_use_module.BrowserUseBrowserProvider() session = provider.create_session("task-browser-use-managed") sent_headers = post.call_args.kwargs["headers"] ``` ```diff @@ -228,11 +287,11 @@ def json(self): } with patch.dict(os.environ, env, clear=True): - browser_use_module = _load_tool_module( - "tools.browser_providers.browser_use", - "browser_providers/browser_use.py", + browser_use_module = _load_plugin_module( + "plugins.browser.browser_use.provider", + "browser/browser_use/provider.py", ) - provider = browser_use_module.BrowserUseProvider() + provider = browser_use_module.BrowserUseBrowserProvider() timeout = browser_use_module.requests.Timeout("timed out") with patch.object( ``` ```diff @@ -290,11 +349,11 @@ def json(self): } with patch.dict(os.environ, env, clear=True): - browser_use_module = _load_tool_module( - "tools.browser_providers.browser_use", - "browser_providers/browser_use.py", + browser_use_module = _load_plugin_module( + "plugins.browser.browser_use.provider", + "browser/browser_use/provider.py", ) - provider = browser_use_module.BrowserUseProvider() + provider = browser_use_module.BrowserUseBrowserProvider() with patch.object( browser_use_module.requests, ``` ```diff @@ -337,11 +396,11 @@ def json(self): } with patch.dict(os.environ, env, clear=True): - browser_use_module = _load_tool_module( - "tools.browser_providers.browser_use", - "browser_providers/browser_use.py", + browser_use_module = _load_plugin_module( + "plugins.browser.browser_use.provider", + "browser/browser_use/provider.py", ) - provider = browser_use_module.BrowserUseProvider() + provider = browser_use_module.BrowserUseBrowserProvider() with patch.object(browser_use_module.requests, "post", side_effect=[_Response(), _Response()]) as post: provider.create_session("task-browser-use-new") ``` - `/dev/null` ```diff @@ -1,10 +0,0 @@ -"""Cloud browser provider abstraction. - -Import the ABC so callers can do:: - - from tools.browser_providers import CloudBrowserProvider -""" - -from tools.browser_providers.base import CloudBrowserProvider - -__all__ = ["CloudBrowserProvider"] ``` - `/dev/null` ```diff @@ -1,59 +0,0 @@ -"""Abstract base class for cloud browser providers.""" - -from abc import ABC, abstractmethod -from typing import Dict - - -class CloudBrowserProvider(ABC): - """Interface for cloud browser backends (Browserbase, Steel, etc.). - - Implementations live in sibling modules and are registered in - ``browser_tool._PROVIDER_REGISTRY``. The user selects a provider via - ``hermes setup`` / ``hermes tools``; the choice is persisted as - ``config["browser"]["cloud_provider"]``. - """ - - @abstractmethod - def provider_name(self) -> str: - """Short, human-readable name shown in logs and diagnostics.""" - - @abstractmethod - def is_configured(self) -> bool: - """Return True when all required env vars / credentials are present. - - Called at tool-registration time (``check_browser_requirements``) to - gate availability. Must be cheap — no network calls. - """ - - @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 browser_tool.py — it holds the provider's session ID - regardless of which provider is in use. - """ - - @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. - """ - - @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. - """ ``` - `tools/browser_tool.py` ```diff @@ -83,10 +83,24 @@ except Exception: _is_safe_url = lambda url: False # noqa: E731 — fail-closed: block all if safety module unavailable _is_always_blocked_url = lambda url: True # noqa: E731 — fail-closed on the floor too -from tools.browser_providers.base import CloudBrowserProvider -from tools.browser_providers.browserbase import BrowserbaseProvider -from tools.browser_providers.browser_use import BrowserUseProvider -from tools.browser_providers.firecrawl import FirecrawlProvider +# Browser-provider ABC + registry — PR #25214 moved the per-vendor providers +# (Browserbase / Browser Use / Firecrawl) out of ``tools/browser_providers/`` +# and into ``plugins/browser//``. The dispatcher consults the +# registry; the legacy class names are re-exported below as backward-compat +# shims for callers that import them from this module. +from agent.browser_provider import BrowserProvider as CloudBrowserProvider # noqa: F401 (legacy alias) +from agent.browser_registry import ( # noqa: F401 (test-patchable surface) + get_provider as _registry_get_browser_provider, +) +from plugins.browser.browserbase.provider import ( # noqa: F401 (legacy import surface) + BrowserbaseBrowserProvider as BrowserbaseProvider, +) +from plugins.browser.browser_use.provider import ( # noqa: F401 + BrowserUseBrowserProvider as BrowserUseProvider, +) +from plugins.browser.firecrawl.provider import ( # noqa: F401 + FirecrawlBrowserProvider as FirecrawlProvider, +) from tools.tool_backend_helpers import normalize_browser_cloud_provider # Camofox local anti-detection browser backend (optional). ``` ```diff @@ -391,12 +405,29 @@ def _stop_cdp_supervisor(task_id: str) -> None: # ============================================================================ # Cloud Provider Registry # ============================================================================ +# +# Per-vendor browser providers (Browserbase / Browser Use / Firecrawl) live as +# plugins under ``plugins/browser//`` and self-register through +# :mod:`agent.browser_registry` at plugin-discovery time. The legacy +# class-name registry below is preserved as a backward-compat shim so test +# fixtures that ``monkeypatch.setattr(browser_tool, "_PROVIDER_REGISTRY", ...)`` +# keep working — but ``_get_cloud_provider()`` now consults +# :mod:`agent.browser_registry` for the actual lookup. +# +# When the test patches ``_PROVIDER_REGISTRY``, we honour it (so the cache +# unit tests still drive the function); otherwise the registry-backed path +# wins. This keeps the test surface stable while letting third-party +# plugins drop in under ``~/.hermes/plugins/browser//``. _PROVIDER_REGISTRY: Dict[str, type] = { "browserbase": BrowserbaseProvider, "browser-use": BrowserUseProvider, "firecrawl": FirecrawlProvider, } +# Frozen copy of the import-time _PROVIDER_REGISTRY, used by +# ``_is_legacy_provider_registry_overridden`` to detect test-time +# monkeypatching. NEVER mutate this dict. +_DEFAULT_PROVIDER_REGISTRY: Dict[str, type] = dict(_PROVIDER_REGISTRY) _cached_cloud_provider: Optional[CloudBrowserProvider] = None _cloud_provider_resolved = False ``` ```diff @@ -411,13 +442,65 @@ def _stop_cdp_supervisor(task_id: str) -> None: _browser_engine_resolved = False +def _is_legacy_provider_registry_overridden() -> bool: + """Return True when a test has patched ``_PROVIDER_REGISTRY`` to a custom value. + + Detected by spotting any registered class that *isn't* the canonical + plugin-backed class for that name. Tests that + ``monkeypatch.setattr(browser_tool, "_PROVIDER_REGISTRY", ...)`` install + custom factories (`exploding_factory`, `lambda: fake_provider`, etc.); + those entries fail the canonical-class identity check below. + + Note: a future maintainer adding a 4th built-in provider only needs to + extend ``_DEFAULT_PROVIDER_REGISTRY`` below — they do NOT need to update + a hardcoded set of keys here. The detection just compares each registered + value against the corresponding canonical class. + """ + try: + for key, default_cls in _DEFAULT_PROVIDER_REGISTRY.items(): + if _PROVIDER_REGISTRY.get(key) is not default_cls: + return True + # Extra keys not in the default registry → also an override. + return len(_PROVIDER_REGISTRY) != len(_DEFAULT_PROVIDER_REGISTRY) + except Exception: + return False + + +def _ensure_browser_plugins_loaded() -> None: + """Idempotently trigger plugin discovery so the browser registry is populated. + + Normally `model_tools` is imported early in any session and that + triggers `discover_plugins()` as a side effect. But `_get_cloud_provider` + can be called from contexts that haven't gone through `model_tools` — + standalone scripts, certain unit-test paths, the parity-sweep harness. + Make discovery idempotent and side-effect-only here so users always + see registered plugins regardless of import order. Cheap: subsequent + calls early-return inside `_ensure_plugins_discovered`. + """ + try: + from hermes_cli.plugins import _ensure_plugins_discovered + + _ensure_plugins_discovered() + except Exception as exc: + logger.debug("Browser plugin discovery failed (non-fatal): %s", exc) + + def _get_cloud_provider() -> Optional[CloudBrowserProvider]: """Return the configured cloud browser provider, or None for local mode. Reads ``config["browser"]["cloud_provider"]`` once and caches the result for the process lifetime. An explicit ``local`` provider disables cloud - fallback. If unset, fall back to Browserbase when direct or managed - Browserbase credentials are available. + fallback. If unset, fall back to Browser Use (managed Nous gateway or + direct API key) and then Browserbase (direct credentials only) — the + historic auto-detect order, now expressed as the + :data:`agent.browser_registry._LEGACY_PREFERENCE` walk. + + Selection routes through :mod:`agent.browser_registry` so third-party + browser plugins (``~/.hermes/plugins/browser//``) participate + in explicit-config resolution. Test fixtures that override + ``_PROVIDER_REGISTRY`` or ``BrowserUseProvider`` / ``BrowserbaseProvider`` + on this module still drive the function — see + ``_is_legacy_provider_registry_overridden``. """ global _cached_cloud_provider, _cloud_provider_resolved if _cloud_provider_resolved: ``` ```diff @@ -437,9 +520,33 @@ def _get_cloud_provider() -> Optional[CloudBrowserProvider]: _cached_cloud_provider = None _cloud_provider_resolved = True return None - if provider_key and provider_key in _PROVIDER_REGISTRY: + if provider_key: try: - resolved = _PROVIDER_REGISTRY[provider_key]() + if _is_legacy_provider_registry_overridden(): + # Test fixture path: honour the patched dict so the + # cache-policy unit tests keep working. + factory = _PROVIDER_REGISTRY.get(provider_key) + if factory is not None: + resolved = factory() + else: + # Ensure plugins are discovered so the registry is + # populated. Idempotent — cheap on subsequent calls. + _ensure_browser_plugins_loaded() + resolved = _registry_get_browser_provider(provider_key) + if resolved is None: + # Explicit config name unknown to the registry — + # might be a typo, an uninstalled plugin, or a + # registry-population failure. Warn the user + # (legacy code would have surfaced a typed + # credentials error via direct class instantiation; + # post-migration we surface this WARNING instead). + logger.warning( + "browser.cloud_provider=%r is not a registered " + "browser plugin; falling back to auto-detect " + "(install the corresponding plugin or fix the " + "config key spelling).", + provider_key, + ) except Exception: logger.warning( "Failed to instantiate explicit cloud_provider %r; will retry on next call", ``` ```diff @@ -453,8 +560,15 @@ def _get_cloud_provider() -> Optional[CloudBrowserProvider]: logger.debug("Could not read cloud_provider from config: %s", e) if resolved is None: - # Prefer Browser Use (managed Nous gateway or direct API key), - # fall back to Browserbase (direct credentials only). + # Auto-detect path: Browser Use first (managed Nous gateway or + # direct API key), then Browserbase (direct credentials). Uses + # the legacy class names imported at the top of this module so + # tests that ``monkeypatch.setattr(browser_tool, "BrowserUseProvider", ...)`` + # keep driving this branch deterministically. Third-party browser + # plugins are intentionally NOT reachable from auto-detect — they + # participate only via explicit ``browser.cloud_provider: ``, + # mirroring the firecrawl gate documented on + # :data:`agent.browser_registry._LEGACY_PREFERENCE`. try: fallback_provider = BrowserUseProvider() if fallback_provider.is_configured(): ``` - **#25580: refactor(browser): all cloud browser providers as image_gen-style plugins (closes #25214)**. closed without merge - maintainer direction: category `merged_via`; implication `superseded`; refs `c6e6909e5`, `c74ff2c8e`, `f36c89cd5`, `#27403`, `#2746`, `#25214` ## Fix status - Merged to main in PR #27403 (cited above). No release data is tracked. Verify downstream. - Fix last verified (merged) `2026-05-17`.