fix(doctor): web readiness reflects the selected provider's real state (#78412)
Salvaged from #78434 by @Slobaka (also the issue reporter; earlier than the competing #78436). hermes doctor no longer paints a green web check when the explicitly selected provider cannot initialize — web splits into per-capability rows (web search / web extract) resolved through the same registry resolvers the dispatchers use, with readiness from a true availability probe (_provider_is_ready). Keyless-tier integration on top of the salvage: - _provider_is_ready counts is_keyless_available() as ready — keyless mode is a working state, not a misconfiguration (zero-config installs and selected-keyless Tavily/Firecrawl show ok, not warn) - Tavily/Firecrawl gain is_keyless_available() (True only when explicitly selected — they stay out of the zero-config fallback) - doctor triggers plugin discovery before reading the registry (fresh doctor processes saw an empty registry and warned on everything) E2E: searxng-selected-without-URL warns (the #78412 repro); zero-config, tavily-keyless, firecrawl-keyless all read ok; parallel pinned paid without a key warns.
This commit is contained in:
@@ -295,6 +295,60 @@ def _doctor_tool_availability_detail(toolset: str) -> str:
|
||||
return ""
|
||||
|
||||
|
||||
def _doctor_web_capability_rows() -> list[tuple[str, str, str]]:
|
||||
"""Return doctor rows for web search/extract provider readiness (#78412).
|
||||
|
||||
Each row is ``(status, label, detail)`` where *status* is ``ok`` or ``warn``.
|
||||
Uses the same active-provider resolvers as the tools, but reports readiness
|
||||
from ``is_available()`` so an explicitly selected but unconfigured backend
|
||||
does not look healthy.
|
||||
"""
|
||||
rows: list[tuple[str, str, str]] = []
|
||||
try:
|
||||
from agent.web_search_registry import (
|
||||
get_active_extract_provider,
|
||||
get_active_search_provider,
|
||||
)
|
||||
from tools.web_tools import _ensure_web_plugins_loaded, _provider_is_ready
|
||||
|
||||
# Doctor runs in a fresh process — bundled web providers register
|
||||
# during plugin discovery, which nothing has triggered yet here.
|
||||
# Without this the registry is empty and every row reads
|
||||
# "no provider selected or registered" (idempotent, cheap on rerun).
|
||||
_ensure_web_plugins_loaded()
|
||||
except Exception:
|
||||
return rows
|
||||
|
||||
for capability, getter in (
|
||||
("web search", get_active_search_provider),
|
||||
("web extract", get_active_extract_provider),
|
||||
):
|
||||
try:
|
||||
provider = getter()
|
||||
except Exception:
|
||||
provider = None
|
||||
if provider is None:
|
||||
rows.append(
|
||||
(
|
||||
"warn",
|
||||
capability,
|
||||
"(no provider selected or registered)",
|
||||
)
|
||||
)
|
||||
continue
|
||||
name = getattr(provider, "name", None) or type(provider).__name__
|
||||
if _provider_is_ready(provider):
|
||||
rows.append(("ok", capability, f"({name})"))
|
||||
else:
|
||||
rows.append(
|
||||
(
|
||||
"warn",
|
||||
capability,
|
||||
f"({name} selected; provider not configured)",
|
||||
)
|
||||
)
|
||||
return rows
|
||||
|
||||
def _apply_doctor_tool_availability_overrides(available: list[str], unavailable: list[dict]) -> tuple[list[str], list[dict]]:
|
||||
"""Adjust runtime-gated tool availability for doctor diagnostics."""
|
||||
updated_available = list(available)
|
||||
@@ -2835,11 +2889,26 @@ def run_doctor(args):
|
||||
|
||||
available, unavailable = check_tool_availability()
|
||||
available, unavailable = _apply_doctor_tool_availability_overrides(available, unavailable)
|
||||
|
||||
|
||||
# Web is split into search/extract readiness rows so an explicitly
|
||||
# selected but unconfigured backend cannot look healthy (#78412).
|
||||
web_rows = []
|
||||
if "web" in available or any(item.get("name") == "web" for item in unavailable):
|
||||
web_rows = _doctor_web_capability_rows()
|
||||
if web_rows:
|
||||
available = [tid for tid in available if tid != "web"]
|
||||
unavailable = [item for item in unavailable if item.get("name") != "web"]
|
||||
|
||||
for tid in available:
|
||||
info = TOOLSET_REQUIREMENTS.get(tid, {})
|
||||
check_ok(info.get("name", tid), _doctor_tool_availability_detail(tid))
|
||||
|
||||
|
||||
for status, label, detail in web_rows:
|
||||
if status == "ok":
|
||||
check_ok(label, detail)
|
||||
else:
|
||||
check_warn(label, detail)
|
||||
|
||||
for item in unavailable:
|
||||
env_vars = item.get("missing_vars") or item.get("env_vars") or []
|
||||
if env_vars:
|
||||
@@ -2852,7 +2921,8 @@ def run_doctor(args):
|
||||
# current CLI platform. Default-off or explicitly disabled toolsets may
|
||||
# still show warnings above, but should not pollute the final summary.
|
||||
api_disabled = _missing_api_key_toolsets_for_summary(unavailable)
|
||||
if api_disabled:
|
||||
web_not_ready = any(status != "ok" for status, _, _ in web_rows)
|
||||
if api_disabled or web_not_ready:
|
||||
issues.append("Run 'hermes setup' to configure missing API keys for full tool access")
|
||||
except Exception as e:
|
||||
check_warn("Could not check tool availability", f"({e})")
|
||||
|
||||
@@ -504,6 +504,19 @@ class FirecrawlWebSearchProvider(WebSearchProvider):
|
||||
"""Return True when direct Firecrawl OR managed-gateway path is configured."""
|
||||
return check_firecrawl_api_key()
|
||||
|
||||
def is_keyless_available(self) -> bool:
|
||||
"""Firecrawl serves keyless cloud requests when explicitly selected.
|
||||
|
||||
Mirrors :func:`_is_explicit_firecrawl_selection` — keyless cloud
|
||||
mode is opt-in by selection, never part of the automatic
|
||||
zero-config fallback. Keeps doctor/readiness gates (#78412) from
|
||||
flagging a working selected-keyless Firecrawl setup as unconfigured.
|
||||
"""
|
||||
try:
|
||||
return _is_explicit_firecrawl_selection()
|
||||
except Exception: # noqa: BLE001 — config layer optional
|
||||
return False
|
||||
|
||||
def supports_search(self) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
@@ -159,6 +159,26 @@ class TavilyWebSearchProvider(WebSearchProvider):
|
||||
|
||||
return bool(get_provider_env("TAVILY_API_KEY"))
|
||||
|
||||
def is_keyless_available(self) -> bool:
|
||||
"""Tavily serves keyless requests when explicitly selected.
|
||||
|
||||
Keyless mode is opt-in by selection (X-Tavily-Access-Mode header),
|
||||
not part of the automatic zero-config fallback — so this only
|
||||
reports True when config actually routes a capability to Tavily.
|
||||
Keeps doctor/readiness gates (#78412) from flagging a working
|
||||
selected-keyless Tavily setup as unconfigured.
|
||||
"""
|
||||
import tools.web_tools as _wt
|
||||
|
||||
try:
|
||||
cfg = _wt._load_web_config()
|
||||
except Exception: # noqa: BLE001 — config layer optional
|
||||
return False
|
||||
return any(
|
||||
(cfg.get(key) or "").lower().strip() == "tavily"
|
||||
for key in ("backend", "search_backend", "extract_backend")
|
||||
)
|
||||
|
||||
def supports_search(self) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
@@ -77,6 +77,52 @@ class TestDoctorToolAvailabilitySummary:
|
||||
|
||||
assert [item["name"] for item in filtered] == ["web"]
|
||||
|
||||
def test_web_capability_rows_warn_when_selected_provider_not_ready(self, monkeypatch):
|
||||
"""#78412: selected firecrawl with is_available=False must warn."""
|
||||
class _Unavailable:
|
||||
name = "firecrawl"
|
||||
|
||||
def is_available(self):
|
||||
return False
|
||||
|
||||
unavailable = _Unavailable()
|
||||
monkeypatch.setattr(
|
||||
"agent.web_search_registry.get_active_search_provider",
|
||||
lambda: unavailable,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"agent.web_search_registry.get_active_extract_provider",
|
||||
lambda: unavailable,
|
||||
)
|
||||
|
||||
rows = doctor._doctor_web_capability_rows()
|
||||
assert rows
|
||||
assert all(status == "warn" for status, _, _ in rows)
|
||||
assert any("firecrawl selected; provider not configured" in detail for _, _, detail in rows)
|
||||
|
||||
def test_web_capability_rows_ok_when_provider_ready(self, monkeypatch):
|
||||
class _Ready:
|
||||
name = "ddgs"
|
||||
|
||||
def is_available(self):
|
||||
return True
|
||||
|
||||
ready = _Ready()
|
||||
monkeypatch.setattr(
|
||||
"agent.web_search_registry.get_active_search_provider",
|
||||
lambda: ready,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"agent.web_search_registry.get_active_extract_provider",
|
||||
lambda: ready,
|
||||
)
|
||||
|
||||
rows = doctor._doctor_web_capability_rows()
|
||||
assert rows == [
|
||||
("ok", "web search", "(ddgs)"),
|
||||
("ok", "web extract", "(ddgs)"),
|
||||
]
|
||||
|
||||
|
||||
class TestDoctorEnvFileEncoding:
|
||||
"""Regression for #18637 (bug 3): `hermes doctor` crashed on Windows
|
||||
|
||||
@@ -545,6 +545,54 @@ class TestCheckWebApiKey:
|
||||
from tools.web_tools import check_web_api_key
|
||||
assert check_web_api_key() is True
|
||||
|
||||
def test_explicit_unavailable_active_provider_is_not_ready(self):
|
||||
"""#78412: get_active_* may return a configured backend whose
|
||||
is_available() is False. check_web_api_key must still report False so
|
||||
doctor does not paint a green check for a backend that cannot run.
|
||||
"""
|
||||
class _UnavailableProvider:
|
||||
name = "firecrawl"
|
||||
|
||||
def is_available(self):
|
||||
return False
|
||||
|
||||
unavailable = _UnavailableProvider()
|
||||
with patch("tools.web_tools._load_web_config", return_value={"backend": "firecrawl"}), \
|
||||
patch("tools.web_tools._is_backend_available", return_value=False), \
|
||||
patch(
|
||||
"agent.web_search_registry.get_active_search_provider",
|
||||
return_value=unavailable,
|
||||
), \
|
||||
patch(
|
||||
"agent.web_search_registry.get_active_extract_provider",
|
||||
return_value=unavailable,
|
||||
):
|
||||
from tools.web_tools import check_web_api_key, _provider_is_ready
|
||||
assert _provider_is_ready(unavailable) is False
|
||||
assert check_web_api_key() is False
|
||||
|
||||
def test_explicit_available_active_provider_is_ready(self):
|
||||
"""Registry-selected available provider still lights the gate."""
|
||||
class _AvailableProvider:
|
||||
name = "custom-ok"
|
||||
|
||||
def is_available(self):
|
||||
return True
|
||||
|
||||
available = _AvailableProvider()
|
||||
with patch("tools.web_tools._load_web_config", return_value={"backend": "custom-ok"}), \
|
||||
patch("tools.web_tools._is_backend_available", return_value=False), \
|
||||
patch(
|
||||
"agent.web_search_registry.get_active_search_provider",
|
||||
return_value=available,
|
||||
), \
|
||||
patch(
|
||||
"agent.web_search_registry.get_active_extract_provider",
|
||||
return_value=None,
|
||||
):
|
||||
from tools.web_tools import check_web_api_key
|
||||
assert check_web_api_key() is True
|
||||
|
||||
|
||||
def test_web_requires_env_includes_exa_key():
|
||||
from tools.web_tools import _web_requires_env
|
||||
|
||||
@@ -1166,6 +1166,42 @@ async def web_extract_tool(
|
||||
|
||||
|
||||
# Convenience function to check Firecrawl credentials
|
||||
def _provider_is_ready(provider) -> bool:
|
||||
"""Return True when *provider* reports readiness without raising.
|
||||
|
||||
``get_active_*_provider()`` intentionally returns an explicitly configured
|
||||
backend even when ``is_available()`` is False so the dispatcher can emit a
|
||||
precise missing-credential error. Tool/doctor readiness gates must still
|
||||
require a true availability probe — otherwise ``hermes doctor`` paints a
|
||||
green ✓ for a backend that cannot run (issue #78412).
|
||||
|
||||
A provider that can serve anonymously (``is_keyless_available()`` — the
|
||||
Exa/Parallel free tier) IS ready: keyless mode is a working state, not a
|
||||
misconfiguration.
|
||||
"""
|
||||
if provider is None:
|
||||
return False
|
||||
try:
|
||||
if provider.is_available():
|
||||
return True
|
||||
except Exception as exc: # noqa: BLE001 — broken provider == not ready
|
||||
logger.debug(
|
||||
"web provider %r.is_available() raised during readiness check: %s",
|
||||
getattr(provider, "name", provider),
|
||||
exc,
|
||||
)
|
||||
return False
|
||||
try:
|
||||
return bool(provider.is_keyless_available())
|
||||
except Exception as exc: # noqa: BLE001 — broken provider == not ready
|
||||
logger.debug(
|
||||
"web provider %r.is_keyless_available() raised during readiness check: %s",
|
||||
getattr(provider, "name", provider),
|
||||
exc,
|
||||
)
|
||||
return False
|
||||
|
||||
|
||||
def check_web_api_key() -> bool:
|
||||
"""Check whether the configured web backend is available.
|
||||
|
||||
@@ -1185,15 +1221,14 @@ def check_web_api_key() -> bool:
|
||||
# unlike _get_backend() the probe order is irrelevant.
|
||||
if any(_is_backend_available(backend) for backend in _LEGACY_WEB_BACKENDS):
|
||||
return True
|
||||
# Any plugin-registered provider the registry considers active for either
|
||||
# capability. Delegating to the registry's own availability-filtered
|
||||
# resolvers keeps a single authority for "is a custom provider usable"
|
||||
# rather than re-implementing the walk here. This also covers the
|
||||
# keyless free tier (Parallel/Exa anonymous MCP endpoints): the registry
|
||||
# walk falls back to keyless-capable providers when nothing is keyed,
|
||||
# so a zero-credential install still lights the web tools up. Discovery
|
||||
# must run first — check_fn fires at tool-registration time, before any
|
||||
# dispatch has populated the registry.
|
||||
# Plugin-registered path: the active-provider resolvers return an explicit
|
||||
# config hit even when credentials are missing (so the tool can print a
|
||||
# precise "set FOO_API_KEY" error). Readiness still requires a true
|
||||
# availability probe — keyed (is_available) OR keyless-capable
|
||||
# (is_keyless_available; the Exa/Parallel anonymous free tier serves
|
||||
# zero-credential installs, so those count as ready). Discovery must run
|
||||
# first — check_fn fires at tool-registration time, before any dispatch
|
||||
# has populated the registry.
|
||||
try:
|
||||
_ensure_web_plugins_loaded()
|
||||
from agent.web_search_registry import (
|
||||
@@ -1202,14 +1237,13 @@ def check_web_api_key() -> bool:
|
||||
)
|
||||
|
||||
return (
|
||||
get_active_search_provider() is not None
|
||||
or get_active_extract_provider() is not None
|
||||
_provider_is_ready(get_active_search_provider())
|
||||
or _provider_is_ready(get_active_extract_provider())
|
||||
)
|
||||
except Exception as exc: # noqa: BLE001 — registry optional; never fatal
|
||||
logger.debug("web provider registry availability check failed: %s", exc)
|
||||
return False
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
"""
|
||||
Simple test/demo when run directly
|
||||
|
||||
Reference in New Issue
Block a user