fix(web): correct 'disabled plugin' diagnosis for web backends (#59573)

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.
This commit is contained in:
Teknium 2026-07-06 04:38:17 -07:00 committed by GitHub
parent d84a2af3d4
commit 27f74b26c5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 397 additions and 33 deletions

View file

@ -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.<capability>_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/<vendor>`` 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.

View file

@ -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)

View file

@ -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

View file

@ -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()

View file

@ -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,