Skip to content

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

Closed
kshitijk4poor wants to merge 9 commits into
NousResearch:mainfrom
kshitijk4poor:spike/browser-providers-plugin
Closed

refactor(browser): all cloud browser providers as image_gen-style plugins (closes #25214)#25580
kshitijk4poor wants to merge 9 commits into
NousResearch:mainfrom
kshitijk4poor:spike/browser-providers-plugin

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Implements #25214 — migrates the three cloud browser providers (Browserbase, Browser Use, Firecrawl) out of tools/browser_providers/ and into plugins/browser/<vendor>/, mirroring the architecture PR #25182 established for the web subsystem. Third-party browser backends can now drop in under ~/.hermes/plugins/browser/<vendor>/ and surface automatically in hermes tools — same UX as image_gen / video_gen / web plugins.

Scope — what moved

Provider Auth Capabilities
browserbase BROWSERBASE_API_KEY + BROWSERBASE_PROJECT_ID (direct only) Cloud browser w/ stealth, proxies, keep-alive
browser-use BROWSER_USE_API_KEY (direct) OR managed Nous gateway Cloud browser w/ remote execution + Nous-subscription billing
firecrawl FIRECRAWL_API_KEY (shared with the web firecrawl plugin) Cloud browser via /v2/browser endpoint

Each plugin is plugins/browser/<vendor>/{plugin.yaml,__init__.py,provider.py}.

What changes for users

Nothing. The setup wizard, hermes tools picker, per-provider env vars, and the dispatcher in tools/browser_tool.py all behave identically. The only visible difference is that user-installed browser plugins now light up in the picker the same way web/image-gen plugins do.

Architecture

Before:

  • Hardcoded class registry in tools/browser_tool.py:394:
    _PROVIDER_REGISTRY: Dict[str, type] = {
        "browserbase": BrowserbaseProvider,
        "browser-use": BrowserUseProvider,
        "firecrawl": FirecrawlProvider,
    }
  • ABC at tools/browser_providers/base.py::CloudBrowserProvider
  • 7 hardcoded TOOL_CATEGORIES["browser"]["providers"] rows in hermes_cli/tools_config.py

After:

  • ABC at agent/browser_provider.py::BrowserProvider (with is_configured() / provider_name() backward-compat aliases delegating to is_available() / display_name)
  • Registry at agent/browser_registry.py (register_provider / get_provider / list_providers / get_active_browser_provider)
  • Plugin facade: ctx.register_browser_provider() in hermes_cli/plugins.py
  • Dispatcher cutover in tools/browser_tool.py::_get_cloud_provider — explicit config goes through the registry, legacy class-name patches still drive the function (test-compat)
  • _plugin_browser_providers() injection in _visible_providers() for the Browser Automation picker

Registry resolution semantics

agent.browser_registry._resolve() follows two rules in order:

  1. Explicit config wins, ignoring availability. If browser.cloud_provider names a registered provider, return it even if its is_available() returns False — the dispatcher surfaces a typed credentials error instead of silently switching backends. Matches legacy _get_cloud_provider() behavior.
  2. Legacy preference walk, filtered by availability. Walk _LEGACY_PREFERENCE = ("browser-use", "browserbase") looking for a provider whose is_available() is True. Matches the historic auto-detect order.

No "single-eligible shortcut" (unlike agent.web_search_registry._resolve). Pre-migration, the auto-detect branch only considered Browser Use → Browserbase; Firecrawl was reachable solely via explicit browser.cloud_provider: firecrawl. Preserving that gate matters because FIRECRAWL_API_KEY is shared with the web firecrawl plugin — auto-routing a web-extract user to a paid cloud browser on a fresh install would be a real behavior regression.

Picker non-provider rows

TOOL_CATEGORIES["browser"]["providers"] keeps three hardcoded rows that aren't CloudBrowserProvider plugins:

  • Nous Subscription (Browser Use cloud) — managed Browser Use billed via Nous subscription. Uses the browser-use plugin as the underlying backend but has distinct setup UX (requires_nous_auth + override_env_vars).
  • Local Browser — headless Chromium, no CloudBrowserProvider.
  • Camofox — anti-detection local Firefox; _is_camofox_mode() short-circuits the cloud-provider dispatch path entirely. (Issue notes this is out-of-scope for Mirror web-provider plugin migration for browser providers #25214.)

Backward compatibility

The class names that tests/integration code reaches for are preserved as re-export shims on tools.browser_tool:

from agent.browser_provider import BrowserProvider as CloudBrowserProvider  # legacy alias
from plugins.browser.browserbase.provider import BrowserbaseBrowserProvider as BrowserbaseProvider
from plugins.browser.browser_use.provider import BrowserUseBrowserProvider as BrowserUseProvider
from plugins.browser.firecrawl.provider import FirecrawlBrowserProvider as FirecrawlProvider

The legacy _PROVIDER_REGISTRY dict stays on tools.browser_tool as a backward-compat shim for test fixtures that monkeypatch.setattr(browser_tool, "_PROVIDER_REGISTRY", ...) (the cache-policy regression tests for #22324). _is_legacy_provider_registry_overridden() detects test-time overrides by comparing against a frozen _DEFAULT_PROVIDER_REGISTRY snapshot.

Behavior-parity verification (subprocess sweep vs origin/main)

tests/plugins/browser/check_parity_vs_main.py runs _get_cloud_provider() across 13 config/env scenarios in subprocesses pinned to (a) origin/main and (b) this PR's worktree via sys.path.insert(). Compares the reduced shape (is_local, provider_name, is_available) per scenario.

Initial run caught a real regression: _get_cloud_provider could be reached from contexts that hadn't gone through model_tools (standalone scripts, certain unit-test paths, the parity harness) — without discover_plugins() having run, the registry was empty and the function silently dropped to local mode when the user had explicitly configured a cloud provider. Fixed in commit 00bfc2b58 (_ensure_browser_plugins_loaded() defensive call). Final state: PARITY OK across 13 scenarios.

Scenario origin/main this PR
no-config + no-env local local ✅
explicit local local local ✅
explicit browserbase + no creds Browserbase (is_available=False) Browserbase (is_available=False) ✅
explicit browserbase + creds Browserbase (is_available=True) Browserbase (is_available=True) ✅
explicit browser-use + no creds Browser Use (is_available=False) Browser Use (is_available=False) ✅
explicit browser-use + creds Browser Use Browser Use ✅
explicit firecrawl + no creds Firecrawl (is_available=False) Firecrawl (is_available=False) ✅
explicit firecrawl + creds Firecrawl Firecrawl ✅
no-config + BU creds Browser Use (auto-detect) Browser Use ✅
no-config + BB creds Browserbase (auto-detect) Browserbase ✅
no-config + both BU+BB Browser Use (legacy walk first) Browser Use ✅
no-config + FC only local (FC not in legacy walk) local
no-config + FC + BB Browserbase (FC skipped) Browserbase ✅

Testing

  • 3 existing test files migrated to new ABC + plugin paths:

    • test_managed_browserbase_and_modal.py_load_plugin_module() helper added; 5 _load_tool_module("tools.browser_providers.X") calls rerouted to _load_plugin_module("plugins.browser.X.provider"); class names updated to BrowserbaseBrowserProvider / BrowserUseBrowserProvider; is_configured()is_available() on the one assertion that cared about the rename.
    • test_browser_cloud_provider_cache.py — unchanged (drives the legacy _PROVIDER_REGISTRY path via the new override detection).
    • test_browser_cloud_fallback.py — unchanged.
  • New plugin-level test suite at tests/plugins/browser/test_browser_provider_plugins.py31 tests across 5 classes (mirrors PR refactor(web): all web providers as image_gen-style plugins #25182's tests/plugins/web/):

    • TestBundledPluginsRegister (8) — 3 plugins register, lifecycle methods overridden, picker schema valid
    • TestIsAvailable (4) — env-var sensitivity per plugin, incl. browserbase's two-key requirement
    • TestRegistryResolution (8) — explicit-config wins ignoring availability, legacy walk preference, firecrawl-never-auto-selected regression test
    • TestLegacyAbcAliases (6) — is_configured() / provider_name() shims delegate correctly
    • TestPickerIntegration (3) — _plugin_browser_providers() rows match registered plugins, carry post_setup hook + marker
  • tests/tools/ regression sweep: 5003 passed, 44 skipped, 12 failures — all pre-existing on origin/main (file-tool tests + test_matches_previous_manual_builtin_tool_set flaking on tools.video_generation_tool from feat(video_gen): unified video_generate tool with pluggable provider backends #25126). Verified by running same tests against origin/main — identical 12 failures, none in files this PR touches.

  • Self-review pass (3-agent parallel: code-reuse, code-quality, efficiency) caught and fixed 7 findings in commit 64df0b283:

    1. Dead _ = managed_nous_tools_enabled import-hider in browser_use/provider.py
    2. Wrong # pragma: no cover on ABC backward-compat aliases (they ARE covered)
    3. _is_legacy_provider_registry_overridden hardcoded-key landmine → frozen _DEFAULT_PROVIDER_REGISTRY snapshot
    4. Silent fallback when explicit cloud_provider not in registry → WARNING with actionable text
    5. Triple-redundant _LEGACY_PREFERENCE documentation → consolidated
    6. is_available() exception log: DEBUG → WARNING with exc_info=True
    7. Dead imports in parity harness script
  • Lint diff (ruff + ty vs base): ruff 0/0 new, ty 0 attributable new (the 9 reported are: 3× pre-existing sys.modules[...] = SimpleNamespace pattern in test stubs, 1× private function unresolved-import false-positive, 1× unsupported-operator carried from legacy browser_use.py:41 to plugin path, 1× ty internal panic on unrelated checkpoint_manager.py).

Diff stat

20 files changed, 1678 insertions(+), 172 deletions(-)

The big additions/deletions:

  • New: agent/browser_provider.py (181 LoC), agent/browser_registry.py (228 LoC), plugins/browser/* (3 plugins × ~3 files), tests/plugins/browser/test_browser_provider_plugins.py (379 LoC), tests/plugins/browser/check_parity_vs_main.py (276 LoC)
  • Modified: tools/browser_tool.py (+116 LoC — registry cutover + defensive plugin discovery + backward-compat shims), hermes_cli/plugins.py (+32 — register_browser_provider() facade), hermes_cli/tools_config.py (+74/-31 — _plugin_browser_providers() + dropped 3 hardcoded rows), tests/tools/test_managed_browserbase_and_modal.py (+62/-42 — _load_plugin_module() helper + path/class-name updates)
  • Deleted: tools/browser_providers/{__init__.py,base.py,browserbase.py,browser_use.py,firecrawl.py} (-630 LoC)

Commits

9 focused commits on kshitijk4poor/hermes-agent:spike/browser-providers-plugin:

Foundation
  199813bd  feat(browser): add BrowserProvider ABC mirroring web_search_provider template

Per-plugin migrations
  5bd7d392  feat(browser): browserbase plugin (spike — first migration)
  6f721883  feat(browser): browser-use + firecrawl plugins; drop single-eligible shortcut

Dispatcher cutover + picker + cleanup
  c3b45f98  refactor(browser): dispatch _get_cloud_provider through agent.browser_registry
  9434a356  feat(tools): mirror image_gen plugin-injection in Browser Automation picker
  94cf34f2  refactor(browser): delete tools/browser_providers/ directory; migrate tests

Tests + parity verification
  90cadb96  test(plugins/browser): coverage for the 3-plugin migration
  00bfc2b5  fix(browser): ensure plugin discovery before registry lookup; parity harness

Self-review pass
  64df0b28  fix(browser): self-review pass — dead-import, log levels, future-proofing

Closes #25214.

…template

Foundation commit for the browser-provider plugin migration (NousResearch#25214).
Mirrors the architecture established by PR NousResearch#25182 (web providers):

- agent/browser_provider.py — BrowserProvider ABC. Preserves the legacy
  CloudBrowserProvider lifecycle contract bit-for-bit (create_session,
  close_session, emergency_cleanup, session metadata shape) so the
  dispatcher in tools/browser_tool.py becomes a pure registry lookup.
  Renames is_configured() → is_available() for parity with WebSearchProvider.

- agent/browser_registry.py — selection registry with the same
  three-rule resolution as web_search_registry:
    1. Explicit config wins (returns even if is_available() == False so
       the dispatcher surfaces a precise credentials error)
    2. Single-eligible shortcut
    3. Legacy preference walk: browser-use → browserbase, filtered by
       availability. Firecrawl is intentionally NOT in the legacy walk
       (matches pre-migration behaviour — Firecrawl was only reachable
       via explicit browser.cloud_provider: firecrawl).

- hermes_cli/plugins.py — adds ctx.register_browser_provider() facade,
  one-liner mirror of register_web_search_provider().

No plugins registered yet; no dispatcher cutover yet. The next commits
move browserbase/browser-use/firecrawl into plugins/browser/<vendor>/
and switch tools/browser_tool.py over to the registry.
Migrates tools/browser_providers/browserbase.py → plugins/browser/browserbase/.
Direct credentials only (BROWSERBASE_API_KEY + BROWSERBASE_PROJECT_ID); same
session-creation, 402-handling, and feature-flag logic as the legacy
implementation. Renames is_configured() → is_available() to match the new
BrowserProvider ABC.

The legacy module tools/browser_providers/browserbase.py is NOT yet deleted
and tools/browser_tool.py still references the in-tree class. The dispatcher
cutover happens in a later commit so the plugin migration and the dispatcher
switch land as separate reviewable units.

Verified via plugin-discovery E2E:
  - browserbase registers as 'browserbase'
  - is_available() correctly tracks BROWSERBASE_API_KEY + BROWSERBASE_PROJECT_ID
  - _resolve('browserbase') returns the provider even when unavailable
    (so dispatcher surfaces a typed credentials error)
  - _resolve(None) returns the provider when it's the single eligible one
…shortcut

Migrates the remaining two cloud browser providers to plugins:

  plugins/browser/browser_use/    — dual auth (direct BROWSER_USE_API_KEY
                                    or managed Nous gateway), idempotency-
                                    key handling for retried managed-mode
                                    creates, x-external-call-id capture.
  plugins/browser/firecrawl/      — direct FIRECRAWL_API_KEY only;
                                    distinct from plugins/web/firecrawl/
                                    (same key, different endpoint).

Also drops the 'single-eligible shortcut' rule from
agent.browser_registry._resolve(). Was a copy-paste from
web_search_registry that would have introduced a real behavior change:
a user with only FIRECRAWL_API_KEY set (for web-extract) would silently
get routed to a paid Firecrawl cloud browser on a fresh install — not
matching origin/main, which only auto-detected between Browser Use and
Browserbase. Third-party browser plugins are subject to the same gate:
they require explicit `browser.cloud_provider` to take effect.

Verified end-to-end via plugin discovery:
  - 3 plugins register (browser-use, browserbase, firecrawl)
  - _resolve(None) with no creds: None (local mode)
  - _resolve(None) with only FIRECRAWL_API_KEY: None (matches main)
  - _resolve('firecrawl'): firecrawl (explicit wins)
  - _resolve(None) with BU+firecrawl: browser-use (legacy walk first hit)
  - _resolve(None) with all three: browser-use (legacy walk order)
…_registry

Switches tools.browser_tool's cloud-provider lookup from the hardcoded
_PROVIDER_REGISTRY class-instantiation pattern to the
agent.browser_registry singleton registry that plugins self-populate.

Changes:

- tools/browser_tool.py top imports: pull BrowserProvider from
  agent.browser_provider (re-exported as CloudBrowserProvider for legacy
  callers) and the three provider classes from plugins/browser/<vendor>/.
  Legacy class names (BrowserbaseProvider, BrowserUseProvider, FirecrawlProvider)
  remain on tools.browser_tool as re-export shims so existing test patches
  (monkeypatch.setattr(browser_tool, 'BrowserUseProvider', ...)) keep working.

- _get_cloud_provider() now consults agent.browser_registry.get_provider()
  for explicit-config lookups. The auto-detect fallback still uses
  BrowserUseProvider() / BrowserbaseProvider() at the module level so the
  cache-policy test fixtures (which patch those names) keep driving the
  function. Test-time _PROVIDER_REGISTRY overrides are detected by class
  identity and routed through the legacy factory-call path.

- agent/browser_provider.py: BrowserProvider grows is_configured() and
  provider_name() as thin backward-compat aliases for the legacy
  CloudBrowserProvider API. Subclasses MUST implement is_available() and
  name; the aliases delegate. This keeps ~6 caller sites in browser_tool.py
  working without churning them.

- tests/tools/test_managed_browserbase_and_modal.py: _install_fake_tools_package
  grows stubs for agent.browser_provider / agent.browser_registry /
  plugins.browser.<vendor>.provider so the test's spec-loader path
  (sys.modules-reset + reload-tool-from-disk) can satisfy tools.browser_tool's
  top-level imports.

Verified: all 23 existing tests in test_browser_cloud_*.py +
test_managed_browserbase_and_modal.py still pass post-cutover.

The legacy tools/browser_providers/ directory is NOT yet deleted; several
tests still _load_tool_module() those files via spec_from_file_location.
The deletion + test-path updates land in a later commit.
…picker

Drops the three hardcoded browser-provider rows (Browserbase, Browser Use,
Firecrawl) from TOOL_CATEGORIES['browser']['providers'] and replaces them
with runtime injection from agent.browser_registry — mirroring the
_plugin_web_search_providers() pattern PR NousResearch#25182 established for the
Web Search and Extract category.

Adds _plugin_browser_providers() helper in hermes_cli/tools_config.py
that walks list_providers() and builds a TOOL_CATEGORIES-shape dict per
provider via get_setup_schema(). The new visible_providers() hook calls
it for cat['name'] == 'Browser Automation'.

The three remaining hardcoded rows are non-provider UX setup-flow rows:
  - 'Nous Subscription (Browser Use cloud)' — managed Browser Use billed
    via Nous subscription; uses the browser-use plugin as the underlying
    backend but has distinct setup UX (requires_nous_auth gates it).
  - 'Local Browser' — headless Chromium, no CloudBrowserProvider.
  - 'Camofox' — anti-detection local Firefox; _is_camofox_mode()
    short-circuits the cloud-provider dispatch path entirely.

Verified the picker output matches pre-migration order/content:
  Local Browser, Camofox, Browser Use, Browserbase, Firecrawl
(with 'Nous Subscription' surfaced only when the user is Nous-authed,
unchanged from main).
… tests

The four files in tools/browser_providers/ (base.py, browserbase.py,
browser_use.py, firecrawl.py) have been migrated into
plugins/browser/<vendor>/provider.py over the previous commits. No
in-tree code references them anymore — the legacy class names
(BrowserbaseProvider / BrowserUseProvider / FirecrawlProvider) are
re-exported from tools.browser_tool as aliases to the plugin classes,
so existing test patches keep working.

Updates tests/tools/test_managed_browserbase_and_modal.py:
  - Adds _load_plugin_module() helper next to _load_tool_module().
  - Reroutes five _load_tool_module('tools.browser_providers.X', ...)
    calls to _load_plugin_module('plugins.browser.X.provider', ...).
  - Renames BrowserbaseProvider/BrowserUseProvider -> the new plugin
    class names (BrowserbaseBrowserProvider / BrowserUseBrowserProvider).
  - Updates is_configured() -> is_available() on the one assertion that
    cared about the rename (the others stay on is_configured() via the
    BrowserProvider ABC's backward-compat alias).

Net diff: -630 / +39 lines (tests + dead-code deletion). Verified
23/23 tests in test_browser_cloud_*.py + test_managed_browserbase_and_modal.py
still pass.

Closes the file-tree mismatch portion of NousResearch#25214. Remaining work:
new plugin-level test coverage under tests/plugins/browser/, behaviour
parity subprocess sweep vs origin/main, and full tests/tools/ regression
sweep before opening the PR.
Mirrors tests/plugins/web/test_web_search_provider_plugins.py from PR NousResearch#25182.
31 tests across 5 classes:

  TestBundledPluginsRegister (8 tests)
    - Three plugins register (browserbase, browser-use, firecrawl)
    - Each plugin's name + display_name accessible
    - get_setup_schema() returns picker-shaped dict with post_setup hook
    - All three lifecycle methods (create_session, close_session,
      emergency_cleanup) overridden on every plugin

  TestIsAvailable (4 tests)
    - browserbase needs BOTH BROWSERBASE_API_KEY and BROWSERBASE_PROJECT_ID
    - browserbase: api_key alone or project_id alone insufficient
    - browser-use satisfied by BROWSER_USE_API_KEY
    - firecrawl satisfied by FIRECRAWL_API_KEY

  TestRegistryResolution (8 tests) — most valuable, locks down
                                     pre-migration semantics:
    - _resolve(None) with no creds returns None (local mode)
    - _resolve('local') short-circuits to None
    - _resolve('browserbase') returns provider even when unavailable
      (so dispatcher surfaces typed credentials error)
    - _resolve('firecrawl') same: explicit-config wins
    - _resolve('unknown') falls through to auto-detect
    - Legacy walk picks browser-use over browserbase
    - browserbase-only configuration: browserbase wins
    - **Regression**: firecrawl is NEVER auto-selected even when
      single-eligible (preserves pre-migration gate; FIRECRAWL_API_KEY
      shared with web firecrawl must not silently route to paid cloud
      browser)

  TestLegacyAbcAliases (6 tests)
    - is_configured() delegates to is_available() for all three plugins
    - provider_name() returns display_name for all three plugins

  TestPickerIntegration (3 tests)
    - _plugin_browser_providers() exposes all three plugins as rows
    - Each row carries post_setup='agent_browser'
    - browser_plugin_name marker matches browser_provider

All tests use real imports — no mocking of provider classes — so the
suite catches drift in the ABC, registry, picker injection, and plugin
glue layer simultaneously.

31/31 passing.
…harness

Two changes that go together:

1. tools/browser_tool.py — add _ensure_browser_plugins_loaded() and call
   it from _get_cloud_provider() before consulting the registry. Normally
   model_tools triggers discover_plugins() as an import side-effect, but
   _get_cloud_provider() can be reached from contexts that haven't gone
   through model_tools (standalone scripts, certain unit-test paths, the
   new parity-sweep harness). Without the defensive call, the registry is
   empty and _registry_get_browser_provider() returns None — silently
   downgrading users to local mode when they explicitly configured a
   cloud provider with no credentials yet. The behavior-parity sweep
   below caught this as 4 scenario regressions (explicit-X-no-creds for
   all 3 providers, and explicit-firecrawl-with-creds).

2. tests/plugins/browser/check_parity_vs_main.py — subprocess harness
   that pins one Python invocation to origin/main and one to this PR's
   worktree via sys.path.insert(), runs _get_cloud_provider() across a
   13-scenario config matrix, and diffs the reduced shape tuple
   (is_local, provider_name, is_available). Provider_name pulls from
   provider.provider_name() which is the legacy CloudBrowserProvider
   API and remains as a backward-compat alias on the new BrowserProvider
   ABC, so the comparison is apples-to-apples regardless of class
   identity.

Final result: PARITY OK across 13 scenarios. The four observable
config/credential matrices that exercise the dispatcher all match
origin/main bit-for-bit:

  - no-config + no-env → local
  - explicit local + any env → local
  - explicit BB / BU / FC + no creds → provider returned with
    is_available()==False (so dispatcher surfaces typed credentials
    error; matches main exactly)
  - explicit BB / BU / FC + creds → provider returned with
    is_available()==True
  - no-config + BU creds → Browser Use
  - no-config + BB creds → Browserbase
  - no-config + both → Browser Use (legacy walk first hit)
  - no-config + FC only → local (firecrawl NOT in legacy walk)
  - no-config + FC + BB → Browserbase (legacy walk skips firecrawl)

Per the dev skill's "behavior-parity for refactor PRs" rule — without
this subprocess sweep, 31/31 unit tests pass while the production code
path is silently broken for users who type `browser.cloud_provider:
browserbase` and run a single browser command without prior model_tools
import. Caught + fixed before push.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets tool/browser Browser automation (CDP, Playwright) P3 Low — cosmetic, nice to have labels May 14, 2026
…fing

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.
@kshitijk4poor kshitijk4poor force-pushed the spike/browser-providers-plugin branch from 64df0b2 to e1bbee8 Compare May 14, 2026 19:40
teknium1 added a commit that referenced this pull request May 17, 2026
PR #25580 was authored before #2746 landed on main, so its plugin
versions of browser_use/browserbase/firecrawl ship without the
requests.RequestException → RuntimeError wrapping that 13c72fb added
to the legacy tools/browser_providers/ files for #2746. Cherry-picking
the PR + git rm'ing the legacy files (the migration's intent) would
silently revert that network-error fix.

Port the same try/except pattern into the three plugin create_session()
methods. Browser Use managed-mode keeps its raw-exception propagation
(idempotency-key retry semantics).

Co-authored-by: nidhi-singh02 <nidhi2894@gmail.com>
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #27403#27403

Your 9 commits were cherry-picked onto current main (was 357 commits behind) with your authorship preserved per-commit via rebase merge — they're on main now as c6e6909c74ff2c.

One follow-up commit on top: fix(plugins/browser): carry forward requests.RequestException wrapping (f36c89c) — your branch predates #2746 (commit 13c72fb) which added requests.RequestException → RuntimeError wrapping to the legacy files. Cherry-picking + deleting the legacy files would have silently reverted that fix, so I ported the try/except pattern into the plugin create_session() methods. Browser Use managed mode keeps its raw-exception propagation.

Closes #25214. Thanks for the careful migration — the parity harness and self-review pass were exactly what this needed.

@teknium1 teknium1 closed this May 17, 2026
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
PR NousResearch#25580 was authored before NousResearch#2746 landed on main, so its plugin
versions of browser_use/browserbase/firecrawl ship without the
requests.RequestException → RuntimeError wrapping that 13c72fb added
to the legacy tools/browser_providers/ files for NousResearch#2746. Cherry-picking
the PR + git rm'ing the legacy files (the migration's intent) would
silently revert that network-error fix.

Port the same try/except pattern into the three plugin create_session()
methods. Browser Use managed-mode keeps its raw-exception propagation
(idempotency-key retry semantics).

Co-authored-by: nidhi-singh02 <nidhi2894@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have tool/browser Browser automation (CDP, Playwright) type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mirror web-provider plugin migration for browser providers

3 participants