From c74ff2c8effce1615074820b03e0d13997c62bb5 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 14 May 2026 14:45:29 +0530 Subject: [PATCH] =?UTF-8?q?fix(browser):=20self-review=20pass=20=E2=80=94?= =?UTF-8?q?=20dead-import,=20log=20levels,=20future-proofing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses findings from two self-review passes pre-merge. First pass (3-agent parallel review): 1. plugins/browser/browser_use/provider.py: drop the ``_ = managed_nous_tools_enabled`` dead-import-hider in _get_config_or_none(). The import was actively misleading — the helper IS used in _get_config() (separate method, separate import), not here. The "keep static analysis happy" comment was wrong about what the helper does in this scope. 2. agent/browser_provider.py: drop ``pragma: no cover`` from is_configured() / provider_name() backward-compat aliases. They ARE covered by ``TestLegacyAbcAliases`` — the pragma would have masked future regressions. 3. tools/browser_tool.py: refactor _is_legacy_provider_registry_overridden() to compare against a module-frozen _DEFAULT_PROVIDER_REGISTRY snapshot instead of hardcoded set of 3 keys. Future maintainers adding a 4th built-in provider now just extend _PROVIDER_REGISTRY; the override detection adapts automatically. Previously the hardcoded ``set(...) != {"browserbase", "browser-use", "firecrawl"}`` would flip True forever on any 4-key registry, silently routing every install onto the legacy fixture path. 4. tools/browser_tool.py: when explicit ``browser.cloud_provider`` is set but the registry has no matching plugin (typo, uninstalled plugin, discovery failure), emit a WARNING with actionable text instead of silently falling through to auto-detect. Legacy code surfaced a typed credentials error via direct class instantiation; this log restores the signal in the post-migration path. 5. agent/browser_registry.py: trim the triple-redundant _LEGACY_PREFERENCE documentation. Module docstring + 13-line block-comment + 5-line inline comment was repeating the same point. Kept the docstring and trimmed the block-comment to 5 lines. 6. agent/browser_registry.py: upgrade is_available()-raised logging from DEBUG to WARNING with exc_info=True. A provider's availability check throwing is unusual enough that users debugging "no cloud provider" need the traceback in logs. 7. tests/plugins/browser/check_parity_vs_main.py: drop dead top-level imports (os, shutil, tempfile — only referenced inside the SUBPROCESS_SCRIPT string literal that runs in a child process). Second pass (architecture + claim-verification review): 8. tools/browser_tool.py: rewrite the inline comment in _get_cloud_provider auto-detect branch. Prior text claimed it "routes through the plugin registry's legacy preference walk so third-party plugins still get a chance to be selected when they're explicitly configured" — false on both counts. The branch uses module-level legacy class aliases (BrowserUseProvider / BrowserbaseProvider) directly; third-party plugins are intentionally reachable only via explicit ``browser.cloud_provider``. Corrected comment now matches behaviour and cross-references _LEGACY_PREFERENCE for the firecrawl gate rationale. 9. tools/browser_tool.py + tests/tools/test_managed_browserbase_and_modal.py: drop the unused ``get_active_browser_provider as _registry_get_active_browser_provider`` alias from the ``from agent.browser_registry import ...`` block. It was never referenced; matching test-stub line in the agent.browser_registry SimpleNamespace also dropped. ``get_provider`` is still imported (used by the explicit-config dispatch path at line 535). 10. plugins/browser/firecrawl/provider.py: align emergency_cleanup() with the early-guard pattern used in browserbase + browser_use plugins. Previously firecrawl tried the DELETE and relied on ``_headers()`` raising ValueError to trip a "missing credentials" warning; same final outcome but a different control flow that read like a bug to a maintainer skimming the three modules. Now: if is_available() is False, log+return early — identical shape to the other two providers. Verification: 54/54 unit tests + 13/13 parity scenarios still pass. --- agent/browser_provider.py | 4 +- agent/browser_registry.py | 23 +++---- plugins/browser/browser_use/provider.py | 11 +--- plugins/browser/firecrawl/provider.py | 11 ++-- tests/plugins/browser/check_parity_vs_main.py | 3 - .../test_managed_browserbase_and_modal.py | 1 - tools/browser_tool.py | 66 ++++++++++++------- 7 files changed, 62 insertions(+), 57 deletions(-) diff --git a/agent/browser_provider.py b/agent/browser_provider.py index 338dfcd6b0..75e88e584f 100644 --- a/agent/browser_provider.py +++ b/agent/browser_provider.py @@ -166,10 +166,10 @@ class BrowserProvider(abc.ABC): # 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: # pragma: no cover - trivial delegation + def is_configured(self) -> bool: """Backward-compat alias for :meth:`is_available`.""" return self.is_available() - def provider_name(self) -> str: # pragma: no cover - trivial delegation + def provider_name(self) -> str: """Backward-compat alias returning :attr:`display_name`.""" return self.display_name diff --git a/agent/browser_registry.py b/agent/browser_registry.py index 7b5b8b99b5..db608744b3 100644 --- a/agent/browser_registry.py +++ b/agent/browser_registry.py @@ -99,19 +99,11 @@ def get_provider(name: str) -> Optional[BrowserProvider]: # --------------------------------------------------------------------------- -# Legacy preference order — preserves behaviour for users who set no -# ``browser.cloud_provider`` config key. Matches the historic auto-detect -# order in :func:`tools.browser_tool._get_cloud_provider` (Browser Use first -# because it covers both managed Nous gateway and direct API key; Browserbase -# second as the older direct-credentials fallback). Filtered by -# ``is_available()`` at walk time so we don't surface a provider the user -# has no credentials for. -# -# Note: ``firecrawl`` is intentionally absent. Pre-migration, the auto-detect -# branch only considered Browser Use → Browserbase; Firecrawl was reachable -# only via an explicit ``browser.cloud_provider: firecrawl`` config key. -# Preserving that gate prevents users with a ``FIRECRAWL_API_KEY`` set for -# web-extract from accidentally getting routed to a (paid) cloud browser. +# 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", @@ -159,7 +151,10 @@ def _resolve(configured: Optional[str]) -> Optional[BrowserProvider]: try: return bool(p.is_available()) except Exception as exc: # noqa: BLE001 - logger.debug("provider %s.is_available() raised %s", p.name, exc) + 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. diff --git a/plugins/browser/browser_use/provider.py b/plugins/browser/browser_use/provider.py index 82bd2420ca..8c5af5f9f0 100644 --- a/plugins/browser/browser_use/provider.py +++ b/plugins/browser/browser_use/provider.py @@ -130,9 +130,10 @@ class BrowserUseBrowserProvider(BrowserProvider): # 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 managed_nous_tools_enabled, prefers_gateway + from tools.tool_backend_helpers import prefers_gateway - # 1. Direct API key path (unless user explicitly 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 { @@ -141,16 +142,10 @@ class BrowserUseBrowserProvider(BrowserProvider): "managed_mode": False, } - # 2. Managed Nous gateway path. managed = resolve_managed_tool_gateway("browser-use") if managed is None: return None - # Hold reference to managed_nous_tools_enabled so static analysis - # doesn't flag the import as unused — the helper is consulted by - # _get_config() below to compose a more accurate error message. - _ = managed_nous_tools_enabled - return { "api_key": managed.nous_user_token, "base_url": managed.gateway_origin.rstrip("/"), diff --git a/plugins/browser/firecrawl/provider.py b/plugins/browser/firecrawl/provider.py index a3f74d3211..498e4ffad9 100644 --- a/plugins/browser/firecrawl/provider.py +++ b/plugins/browser/firecrawl/provider.py @@ -130,17 +130,18 @@ class FirecrawlBrowserProvider(BrowserProvider): 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 diff --git a/tests/plugins/browser/check_parity_vs_main.py b/tests/plugins/browser/check_parity_vs_main.py index 11652e94af..b706ce3e9c 100644 --- a/tests/plugins/browser/check_parity_vs_main.py +++ b/tests/plugins/browser/check_parity_vs_main.py @@ -19,11 +19,8 @@ Run from the PR worktree: from __future__ import annotations import json -import os -import shutil import subprocess import sys -import tempfile from pathlib import Path diff --git a/tests/tools/test_managed_browserbase_and_modal.py b/tests/tools/test_managed_browserbase_and_modal.py index 3d0d7b3419..d88789706b 100644 --- a/tests/tools/test_managed_browserbase_and_modal.py +++ b/tests/tools/test_managed_browserbase_and_modal.py @@ -106,7 +106,6 @@ def _install_fake_tools_package(): BrowserProvider=_StubBrowserProvider, ) sys.modules["agent.browser_registry"] = types.SimpleNamespace( - get_active_browser_provider=lambda: None, get_provider=lambda name: None, list_providers=lambda: [], register_provider=lambda provider: None, diff --git a/tools/browser_tool.py b/tools/browser_tool.py index b089ed9213..fb96649cb3 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -90,7 +90,6 @@ except Exception: # 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_active_browser_provider as _registry_get_active_browser_provider, get_provider as _registry_get_browser_provider, ) from plugins.browser.browserbase.provider import ( # noqa: F401 (legacy import surface) @@ -425,6 +424,10 @@ _PROVIDER_REGISTRY: Dict[str, type] = { "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 @@ -442,25 +445,23 @@ _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 comparing identity with the module-level defaults dict - populated above. Tests that ``monkeypatch.setattr(browser_tool, - "_PROVIDER_REGISTRY", ...)`` swap in a new object; identity differs - even when the contents happen to match. Used by ``_get_cloud_provider`` - to honour test-time overrides (which expect a factory-callable shape) - instead of routing through the plugin registry. + 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. """ - # The module-level _PROVIDER_REGISTRY is built once at import time. A test - # that swaps it via monkeypatch creates a new dict; we detect that via - # the registered class identities, not by ``is`` on the dict itself - # (the patch may install a dict whose values happen to be the same - # classes; treat that as "not overridden"). try: - return ( - _PROVIDER_REGISTRY.get("browserbase") is not BrowserbaseProvider - or _PROVIDER_REGISTRY.get("browser-use") is not BrowserUseProvider - or _PROVIDER_REGISTRY.get("firecrawl") is not FirecrawlProvider - or set(_PROVIDER_REGISTRY.keys()) != {"browserbase", "browser-use", "firecrawl"} - ) + 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 @@ -532,6 +533,20 @@ def _get_cloud_provider() -> Optional[CloudBrowserProvider]: # 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", @@ -545,12 +560,15 @@ def _get_cloud_provider() -> Optional[CloudBrowserProvider]: logger.debug("Could not read cloud_provider from config: %s", e) if resolved is None: - # Auto-detect path. When tests have patched the per-class names - # on this module (BrowserUseProvider / BrowserbaseProvider), honour - # them — the test_browser_cloud_provider_cache test relies on this. - # Otherwise route through the plugin registry's legacy preference - # walk so third-party plugins still get a chance to be selected - # when they're explicitly configured. + # 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():