fix(browser): ensure plugin discovery before registry lookup; parity 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.
This commit is contained in:
kshitijk4poor 2026-05-14 14:27:21 +05:30 committed by Teknium
parent fec0a0da98
commit 1bb6f03724
2 changed files with 298 additions and 0 deletions

View file

@ -465,6 +465,25 @@ def _is_legacy_provider_registry_overridden() -> bool:
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.
@ -509,6 +528,9 @@ def _get_cloud_provider() -> Optional[CloudBrowserProvider]:
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)
except Exception:
logger.warning(