From 27f74b26c59ec06d9c72b5a9ed33633c1a7a9b24 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 6 Jul 2026 04:38:17 -0700 Subject: [PATCH] fix(web): correct 'disabled plugin' diagnosis for web backends (#59573) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a bundled web provider (firecrawl, tavily, exa, ...) is listed in plugins.disabled, its provider never registers and the web_search/ web_extract dispatchers emitted the misleading "No web extract provider configured. Set web.extract_backend to ..." — even though the backend was configured correctly. The real fix is to re-enable the plugin. - web_tools.py + web_search_registry.py: when the configured backend names a disabled bundled web plugin, both dispatchers now point the user at the actual cause (re-enable the plugin) instead of a wrong config hint. - plugins_cmd.py cmd_enable: enabling by canonical key now also clears the manifest-name alias (web-firecrawl) from plugins.disabled, so the suggested command actually re-enables the plugin ('explicit disable wins' matches on the name too). - plugins_cmd.py cmd_toggle / _run_composite_ui / _run_composite_fallback: the interactive 'hermes plugins' menu now persists the canonical key (web/firecrawl), never the bare manifest name — the drift that put the offending entry in plugins.disabled in the first place. Follow-up to #59518 (which fixed web credential resolution, a different cause). Fixes the disabled-plugin symptom reported after that PR. --- agent/web_search_registry.py | 59 ++++++++ hermes_cli/plugins_cmd.py | 90 ++++++++---- .../test_plugins_cmd_enable_disable_nested.py | 98 +++++++++++++ tests/tools/test_web_providers.py | 130 ++++++++++++++++++ tools/web_tools.py | 53 ++++++- 5 files changed, 397 insertions(+), 33 deletions(-) diff --git a/agent/web_search_registry.py b/agent/web_search_registry.py index 079c755787c4..45832cb5488d 100644 --- a/agent/web_search_registry.py +++ b/agent/web_search_registry.py @@ -219,6 +219,65 @@ def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearc return None +def _disabled_web_plugin_for(configured: Optional[str] = None, *, capability: Optional[str] = None) -> Optional[str]: + """Return the plugin key of a *disabled* bundled web plugin that would + have provided the configured backend, or None. + + When a user sets ``web.extract_backend: firecrawl`` (or the search + equivalent) but also lists ``web-firecrawl`` in ``plugins.disabled``, + the provider never registers and the dispatcher would otherwise emit a + misleading "No web extract provider configured. Set web.extract_backend + to ..." error — even though the backend IS configured correctly. The + real fix is to re-enable the plugin. This helper detects that case so + the dispatcher can point the user at the actual cause (issue #40190 + follow-up: pi314's disabled-plugin symptom). + + Pass ``capability`` ("search" | "extract") to resolve the configured + name straight from ``config.yaml`` (``web._backend`` → + ``web.backend``). This is more reliable than the resolved backend the + dispatcher fell back to, since a disabled provider fails the + ``_is_backend_available`` gate and the dispatcher silently drops to + the shared default. An explicit ``configured`` name still wins when + given. + + Matching is by convention: bundled web plugins live under the + ``web/`` key with the provider ``name`` differing only in + hyphen/underscore (``brave-free`` provider ⇄ ``web/brave_free`` key, + ``firecrawl`` ⇄ ``web/firecrawl``). We normalize both sides before + comparing so every bundled provider is covered without hardcoding a + per-vendor table. + """ + def _norm(s: str) -> str: + return s.strip().lower().replace("-", "_") + + if not configured and capability in ("search", "extract"): + configured = ( + _read_config_key("web", f"{capability}_backend") + or _read_config_key("web", "backend") + ) + if not configured: + return None + + want = _norm(configured) + try: + from hermes_cli.plugins import get_plugin_manager + + pm = get_plugin_manager() + for key, loaded in pm._plugins.items(): + if not isinstance(key, str) or not key.startswith("web/"): + continue + if loaded.enabled: + continue + if loaded.error != "disabled via config": + continue + vendor = key.split("/", 1)[1] + if _norm(vendor) == want: + return key + except Exception as exc: # noqa: BLE001 — diagnostics are best-effort + logger.debug("disabled-web-plugin lookup failed: %s", exc) + return None + + def get_active_search_provider() -> Optional[WebSearchProvider]: """Resolve the currently-active web search provider. diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index fc66810489ef..7d6284426ee3 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -839,10 +839,21 @@ def cmd_enable(name: str, allow_tool_override: Optional[bool] = None) -> None: if not already_enabled: enabled.add(key) disabled.discard(key) - # Drop any legacy bare-name entry so the two don't drift out of sync. + # Drop every alias of this plugin from the disabled list so an + # explicit disable under a different form can't keep it off. The + # loader's disable check matches on BOTH the canonical key + # (``web/firecrawl``) AND the manifest name (``web-firecrawl``); + # a stale entry under either form makes "explicit disable wins" + # (plugins.py) silently veto this enable. Discard the key, its + # bare leaf, and the manifest name. (#40190 follow-up.) bare = key.split("/")[-1] if bare != key: disabled.discard(bare) + for entry in _discover_all_plugins(): + # entry = (name, version, description, source, dir_path, key) + if entry[5] == key: + disabled.discard(entry[0]) + break _save_enabled_set(enabled) _save_disabled_set(disabled) console.print( @@ -1289,7 +1300,15 @@ def cmd_toggle() -> None: enabled_set = _get_enabled_set() disabled_set = _get_disabled_set() - plugin_names = [] + # Track by CANONICAL KEY (``key``), not the manifest name. The loader + # (PluginManager) and ``cmd_enable``/``cmd_disable`` all gate on the + # canonical key (``web/firecrawl``), while the manifest name may differ + # (``web-firecrawl``). Persisting the bare name here caused the two + # forms to drift: the menu would write ``web-firecrawl`` to + # plugins.disabled, but ``hermes plugins enable web/firecrawl`` cleared + # only the key — so "explicit disable wins" kept a bundled backend off + # forever (pi314's #40190 symptom). Keys keep every surface aligned. + plugin_keys = [] plugin_labels = [] plugin_selected = set() @@ -1297,10 +1316,17 @@ def cmd_toggle() -> None: label = f"{name} \u2014 {description}" if description else name if source == "bundled": label = f"{label} [bundled]" - plugin_names.append(name) + plugin_keys.append(key) plugin_labels.append(label) - # Selected (enabled) when in enabled-set AND not in disabled-set - if (name in enabled_set or key in enabled_set) and name not in disabled_set and key not in disabled_set: + # Selected (enabled) when in enabled-set AND not in disabled-set. + # Accept the legacy bare name on either side for back-compat with + # existing configs written before this normalization. + is_on = ( + (key in enabled_set or name in enabled_set) + and key not in disabled_set + and name not in disabled_set + ) + if is_on: plugin_selected.add(i) # -- Provider categories -- @@ -1311,7 +1337,7 @@ def cmd_toggle() -> None: ("Context Engine", current_context, _configure_context_engine), ] - has_plugins = bool(plugin_names) + has_plugins = bool(plugin_keys) has_categories = bool(categories) if not has_plugins and not has_categories: @@ -1327,20 +1353,20 @@ def cmd_toggle() -> None: # Launch the composite curses UI try: import curses - _run_composite_ui(curses, plugin_names, plugin_labels, plugin_selected, + _run_composite_ui(curses, plugin_keys, plugin_labels, plugin_selected, disabled_set, categories, console) except ImportError: - _run_composite_fallback(plugin_names, plugin_labels, plugin_selected, + _run_composite_fallback(plugin_keys, plugin_labels, plugin_selected, disabled_set, categories, console) -def _run_composite_ui(curses, plugin_names, plugin_labels, plugin_selected, +def _run_composite_ui(curses, plugin_keys, plugin_labels, plugin_selected, disabled, categories, console): """Custom curses screen with checkboxes + category action rows.""" from hermes_cli.curses_ui import flush_stdin chosen = set(plugin_selected) - n_plugins = len(plugin_names) + n_plugins = len(plugin_keys) # Total rows: plugins + separator + categories # separator is not navigable n_categories = len(categories) @@ -1555,18 +1581,24 @@ def _run_composite_ui(curses, plugin_names, plugin_labels, plugin_selected, curses.wrapper(_draw) flush_stdin() - # Persist general plugin changes. The new allow-list is the set of - # plugin names that were checked; anything not checked is explicitly - # disabled (written to disabled-list) so it remains off even if the - # plugin code does something clever like auto-enable in the future. + # Persist by canonical key. Unchecked plugins are written to the + # disabled-list so they stay off even if a future plugin auto-enables + # itself — but we ONLY ever write the canonical key (never the bare + # manifest name), so the disabled-list can't drift out of sync with + # what ``cmd_enable`` clears or what PluginManager gates on (#40190). new_enabled: set = set() new_disabled: set = set(disabled) # preserve existing disabled state for unseen plugins - for i, name in enumerate(plugin_names): + for i, key in enumerate(plugin_keys): + bare = key.split("/")[-1] if i in chosen: - new_enabled.add(name) - new_disabled.discard(name) + new_enabled.add(key) + new_disabled.discard(key) + # Drop any stale legacy bare-leaf disable so re-enabling here + # fully clears the plugin from the disabled-list. + if bare != key: + new_disabled.discard(bare) else: - new_disabled.add(name) + new_disabled.add(key) prev_enabled = _get_enabled_set() enabled_changed = new_enabled != prev_enabled @@ -1577,7 +1609,7 @@ def _run_composite_ui(curses, plugin_names, plugin_labels, plugin_selected, _save_disabled_set(new_disabled) console.print( f"\n[green]\u2713[/green] General plugins: {len(new_enabled)} enabled, " - f"{len(plugin_names) - len(new_enabled)} disabled." + f"{len(plugin_keys) - len(new_enabled)} disabled." ) elif n_plugins > 0: console.print("\n[dim]General plugins unchanged.[/dim]") @@ -1595,7 +1627,7 @@ def _run_composite_ui(curses, plugin_names, plugin_labels, plugin_selected, console.print() -def _run_composite_fallback(plugin_names, plugin_labels, plugin_selected, +def _run_composite_fallback(plugin_keys, plugin_labels, plugin_selected, disabled, categories, console): """Text-based fallback for the composite plugins UI.""" from hermes_cli.colors import Colors, color @@ -1603,7 +1635,7 @@ def _run_composite_fallback(plugin_names, plugin_labels, plugin_selected, print(color("\n Plugins", Colors.YELLOW)) # General plugins - if plugin_names: + if plugin_keys: chosen = set(plugin_selected) print(color("\n General Plugins", Colors.YELLOW)) print(color(" Toggle by number, Enter to confirm.\n", Colors.DIM)) @@ -1618,20 +1650,26 @@ def _run_composite_fallback(plugin_names, plugin_labels, plugin_selected, if not val: break idx = int(val) - 1 - if 0 <= idx < len(plugin_names): + if 0 <= idx < len(plugin_keys): chosen.symmetric_difference_update({idx}) except (ValueError, KeyboardInterrupt, EOFError): return print() + # Persist by canonical key only — never the bare manifest name — so + # the disabled-list stays aligned with cmd_enable / PluginManager + # (#40190). new_enabled: set = set() new_disabled: set = set(disabled) - for i, name in enumerate(plugin_names): + for i, key in enumerate(plugin_keys): + bare = key.split("/")[-1] if i in chosen: - new_enabled.add(name) - new_disabled.discard(name) + new_enabled.add(key) + new_disabled.discard(key) + if bare != key: + new_disabled.discard(bare) else: - new_disabled.add(name) + new_disabled.add(key) prev_enabled = _get_enabled_set() if new_enabled != prev_enabled or new_disabled != disabled: _save_enabled_set(new_enabled) diff --git a/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py b/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py index b001c32d5c64..c140a56e62f6 100644 --- a/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py +++ b/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py @@ -140,6 +140,44 @@ class TestEnableDisableNested: saved = mock_save_en.call_args[0][0] assert "observability/nemo_relay" in saved + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_enable_clears_manifest_name_alias_from_disabled( + self, mock_en, mock_save_en, mock_save_dis, + mock_user, mock_bundled, tmp_path, + ): + """#40190 follow-up: enabling by canonical key must clear a stale + disable entry recorded under the *manifest name*. + + The web providers ship with a manifest name that differs from the + key (``web-firecrawl`` vs ``web/firecrawl``). A user who ran + ``hermes plugins disable web-firecrawl`` gets ``web-firecrawl`` in + ``plugins.disabled``. Since the loader's disable check matches on + the manifest name too, ``enable web/firecrawl`` must remove that + entry or "explicit disable wins" keeps the plugin off. + """ + from hermes_cli.plugins_cmd import cmd_enable + _make_category_plugin(tmp_path, "web", "firecrawl", { + "name": "web-firecrawl", "version": "1.0.0", + "description": "firecrawl", "kind": "backend", + }) + mock_user.return_value = tmp_path + mock_bundled.return_value = tmp_path / "nonexistent" + # Disabled under the manifest name (neither key nor bare leaf). + with patch( + "hermes_cli.plugins_cmd._get_disabled_set", + return_value={"web-firecrawl"}, + ): + cmd_enable("web/firecrawl", allow_tool_override=False) + + saved_en = mock_save_en.call_args[0][0] + saved_dis = mock_save_dis.call_args[0][0] + assert "web/firecrawl" in saved_en + assert "web-firecrawl" not in saved_dis # manifest-name alias cleared + @patch("hermes_cli.plugins.get_bundled_plugins_dir") @patch("hermes_cli.plugins_cmd._plugins_dir") @patch("hermes_cli.plugins_cmd._save_disabled_set") @@ -338,3 +376,63 @@ class TestEnableToolOverrideConsent: cmd_enable("trusted_bundled") mock_set_flag.assert_not_called() + + +class TestCompositeMenuWritesCanonicalKey: + """#40190 follow-up: the interactive `hermes plugins` menu must persist + the CANONICAL KEY (``web/firecrawl``), never the bare manifest name + (``web-firecrawl``), so its disabled-list entries stay aligned with what + ``cmd_enable`` clears and what PluginManager gates on. Writing the bare + name is what silently vetoed a bundled backend forever (pi314). + """ + + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_fallback_unchecked_plugin_disables_by_key_not_name( + self, mock_en, mock_save_en, mock_save_dis, + ): + from hermes_cli.plugins_cmd import _run_composite_fallback + from rich.console import Console + + # key differs from the manifest name, mirroring web/firecrawl. + plugin_keys = ["web/firecrawl"] + plugin_labels = ["web-firecrawl — firecrawl [bundled]"] + plugin_selected = set() # unchecked → should be disabled + + # First input() toggles nothing (blank Enter confirms immediately), + # second (category prompt) is skipped with blank Enter. + with patch("builtins.input", return_value=""): + _run_composite_fallback( + plugin_keys, plugin_labels, plugin_selected, + set(), [], Console(), + ) + + saved_dis = mock_save_dis.call_args[0][0] + assert "web/firecrawl" in saved_dis # canonical key persisted + assert "web-firecrawl" not in saved_dis # never the bare name + + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_fallback_checked_plugin_enables_by_key_and_clears_aliases( + self, mock_en, mock_save_en, mock_save_dis, + ): + from hermes_cli.plugins_cmd import _run_composite_fallback + from rich.console import Console + + plugin_keys = ["web/firecrawl"] + plugin_labels = ["web-firecrawl — firecrawl [bundled]"] + plugin_selected = {0} # checked → enabled + + # Pre-existing stale bare-leaf disable should be cleared on enable. + with patch("builtins.input", return_value=""): + _run_composite_fallback( + plugin_keys, plugin_labels, plugin_selected, + {"firecrawl"}, [], Console(), + ) + + saved_en = mock_save_en.call_args[0][0] + saved_dis = mock_save_dis.call_args[0][0] + assert "web/firecrawl" in saved_en + assert "firecrawl" not in saved_dis # stale bare-leaf alias cleared diff --git a/tests/tools/test_web_providers.py b/tests/tools/test_web_providers.py index f71b1f3b7d8a..8a62093b0a7d 100644 --- a/tests/tools/test_web_providers.py +++ b/tests/tools/test_web_providers.py @@ -492,3 +492,133 @@ class TestDispatchersTriggerPluginDiscovery: finally: restore() + +class TestDisabledPluginDiagnostic: + """#40190 follow-up: when the configured web backend names a bundled + web plugin the user put in ``plugins.disabled``, the dispatcher must + tell the user to re-enable the plugin instead of the misleading + "No web extract provider configured. Set web.extract_backend to ..." + (they already set it correctly — the provider just isn't loaded). + """ + + def _clear_registry(self): + from agent import web_search_registry + + with web_search_registry._lock: + original = dict(web_search_registry._providers) + web_search_registry._providers.clear() + + def _restore(): + with web_search_registry._lock: + web_search_registry._providers.clear() + web_search_registry._providers.update(original) + + return _restore + + class _FakeLoaded: + def __init__(self, enabled, error): + self.enabled = enabled + self.error = error + + def _patch_manager(self, monkeypatch, plugins_map): + """Point ``get_plugin_manager()`` at a stub whose ``_plugins`` + dict is ``plugins_map`` so ``_disabled_web_plugin_for`` sees the + simulated disabled/enabled state without touching real config.""" + import hermes_cli.plugins as plugins_mod + + class _StubMgr: + _plugins = plugins_map + + monkeypatch.setattr(plugins_mod, "get_plugin_manager", lambda: _StubMgr()) + + def test_disabled_web_plugin_for_matches_by_key(self, monkeypatch): + from agent.web_search_registry import _disabled_web_plugin_for + + self._patch_manager(monkeypatch, { + "web/firecrawl": self._FakeLoaded(False, "disabled via config"), + "web/ddgs": self._FakeLoaded(True, None), + }) + assert _disabled_web_plugin_for("firecrawl") == "web/firecrawl" + # Enabled plugin is not a match + assert _disabled_web_plugin_for("ddgs") is None + # Unknown name is not a match + assert _disabled_web_plugin_for("nope") is None + + def test_disabled_web_plugin_for_normalizes_hyphens(self, monkeypatch): + from agent.web_search_registry import _disabled_web_plugin_for + + self._patch_manager(monkeypatch, { + "web/brave_free": self._FakeLoaded(False, "disabled via config"), + }) + # config name uses a hyphen; plugin key uses an underscore + assert _disabled_web_plugin_for("brave-free") == "web/brave_free" + + def test_disabled_web_plugin_for_ignores_non_disabled_errors(self, monkeypatch): + from agent.web_search_registry import _disabled_web_plugin_for + + self._patch_manager(monkeypatch, { + # a plugin that failed to import is NOT "disabled via config" + "web/exa": self._FakeLoaded(False, "ImportError: boom"), + }) + assert _disabled_web_plugin_for("exa") is None + + def test_extract_tool_reports_disabled_plugin(self, monkeypatch): + import asyncio + + from tools import web_tools + + restore = self._clear_registry() + try: + monkeypatch.setattr(web_tools, "_ensure_web_plugins_loaded", lambda: None) + monkeypatch.setattr( + web_tools, "_load_web_config", + lambda: {"extract_backend": "firecrawl"}, + ) + import agent.web_search_registry as wsr + monkeypatch.setattr( + wsr, "_read_config_key", + lambda *path: "firecrawl" if path == ("web", "extract_backend") else None, + ) + self._patch_manager(monkeypatch, { + "web/firecrawl": self._FakeLoaded(False, "disabled via config"), + }) + result = json.loads( + asyncio.new_event_loop().run_until_complete( + web_tools.web_extract_tool(["https://example.com"]) + ) + ) + err = result["error"] + assert "disabled" in err + assert "web/firecrawl" in err + assert "hermes plugins enable" in err + # Must NOT tell them to set extract_backend (already set) + assert "Set web.extract_backend to firecrawl" not in err + finally: + restore() + + def test_search_tool_reports_disabled_plugin(self, monkeypatch): + from tools import web_tools + + restore = self._clear_registry() + try: + monkeypatch.setattr(web_tools, "_ensure_web_plugins_loaded", lambda: None) + monkeypatch.setattr( + web_tools, "_load_web_config", + lambda: {"search_backend": "firecrawl"}, + ) + import agent.web_search_registry as wsr + monkeypatch.setattr( + wsr, "_read_config_key", + lambda *path: "firecrawl" if path == ("web", "search_backend") else None, + ) + self._patch_manager(monkeypatch, { + "web/firecrawl": self._FakeLoaded(False, "disabled via config"), + }) + result = json.loads(web_tools.web_search_tool("hello", limit=1)) + err = result["error"] + assert "disabled" in err + assert "web/firecrawl" in err + assert "No web search provider configured" not in err + finally: + restore() + diff --git a/tools/web_tools.py b/tools/web_tools.py index 5fa1e8728327..409b46fa606f 100644 --- a/tools/web_tools.py +++ b/tools/web_tools.py @@ -661,6 +661,7 @@ def web_search_tool(query: str, limit: int = 5) -> str: from agent.web_search_registry import ( get_active_search_provider, get_provider as _wsp_get_provider, + _disabled_web_plugin_for, ) backend = _get_search_backend() @@ -672,13 +673,29 @@ def web_search_tool(query: str, limit: int = 5) -> str: provider = get_active_search_provider() if provider is None: - response_data = { - "success": False, - "error": ( - "No web search provider configured. " - "Run `hermes tools` to set one up." - ), - } + # A bundled web plugin the user explicitly disabled looks + # identical to "no provider" here — point at the real cause + # (re-enable the plugin) rather than a generic setup hint. + disabled_key = _disabled_web_plugin_for(capability="search") + if disabled_key: + _vendor = disabled_key.split("/", 1)[-1] + response_data = { + "success": False, + "error": ( + f"web.search_backend is set to '{_vendor}', but its " + f"plugin ('{disabled_key}') is disabled in config. " + f"Re-enable it with `hermes plugins enable {disabled_key}` " + "(or remove it from plugins.disabled)." + ), + } + else: + response_data = { + "success": False, + "error": ( + "No web search provider configured. " + "Run `hermes tools` to set one up." + ), + } else: logger.info( "Web search via %s: '%s' (limit: %d)", @@ -814,6 +831,7 @@ async def web_extract_tool( from agent.web_search_registry import ( get_active_extract_provider, get_provider as _wsp_get_provider, + _disabled_web_plugin_for, ) provider = _wsp_get_provider(backend) if backend else None @@ -839,6 +857,27 @@ async def web_extract_tool( ) provider = get_active_extract_provider() if provider is None: + # If the configured backend is a bundled web plugin the + # user explicitly disabled, the backend is set correctly + # and the real fix is to re-enable the plugin — say so + # instead of telling them to set web.extract_backend + # (which they already did). #40190 follow-up. + disabled_key = _disabled_web_plugin_for(capability="extract") + if disabled_key: + _vendor = disabled_key.split("/", 1)[-1] + return json.dumps( + { + "success": False, + "error": ( + f"web.extract_backend is set to '{_vendor}', " + f"but its plugin ('{disabled_key}') is disabled " + "in config. Re-enable it with " + f"`hermes plugins enable {disabled_key}` " + "(or remove it from plugins.disabled)." + ), + }, + ensure_ascii=False, + ) return json.dumps( { "success": False,