mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-06-02 07:11:49 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

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

Browse source 
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.
This commit is contained in:
kshitijk4poor 2026-05-14 14:45:29 +05:30 • committed by Teknium
parent 1bb6f03724
commit c74ff2c8ef
7 changed files with 62 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

66
tools/browser_tool.py
View file

@ -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: <name>``,
# mirroring the firecrawl gate documented on
# :data:`agent.browser_registry._LEGACY_PREFERENCE`.
try:
fallback_provider = BrowserUseProvider()
if fallback_provider.is_configured():