mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-26 06:01:49 +00:00
fix(plugins): filter resolution by is_available() in web + image_gen registries
Both web_search_registry._resolve() and image_gen_registry.get_active_provider()
walked their registered providers and returned the first one matching the
capability flag — without checking whether that provider was actually
usable. On a fresh install with no credentials at all, this meant
get_active_search_provider() returned `brave-free` (legacy preference
order) even though BRAVE_SEARCH_API_KEY was unset, leading the
dispatcher to surface a "BRAVE_SEARCH_API_KEY is not set" error for a
provider the user never chose. Same bug shape in image_gen for FAL.
Resolution semantics now match tools.web_tools._get_backend():
1. Explicit config name wins, ignoring is_available() — the dispatcher
surfaces a precise "X_API_KEY is not set" error rather than silently
switching backends. Matches user expectation: "I configured X, tell
me what's wrong with X."
2. Fallback (no explicit config) walks the legacy preference order
filtered by is_available() — pick the highest-priority backend the
user actually has credentials for.
is_available() is wrapped in a try/except so a buggy provider doesn't
brick resolution.
E2E verified:
- No creds + no config: get_active_search_provider() -> None
- Explicit brave-free + no key: get_active_search_provider() -> brave-free
(and .is_available() correctly reports False)
This fix was identified during the spike (#25182 finding #1) and is
fold-in to the same PR rather than a follow-up.
This commit is contained in:
parent
6b219f5af6
commit
0a7cbd3342
2 changed files with 80 additions and 9 deletions
|
|
@ -77,6 +77,17 @@ def get_active_provider() -> Optional[ImageGenProvider]:
|
||||||
|
|
||||||
Reads ``image_gen.provider`` from config.yaml; falls back per the
|
Reads ``image_gen.provider`` from config.yaml; falls back per the
|
||||||
module docstring.
|
module docstring.
|
||||||
|
|
||||||
|
**Availability semantics** (mirrors :mod:`agent.web_search_registry`):
|
||||||
|
|
||||||
|
- When ``image_gen.provider`` is explicitly set, the configured
|
||||||
|
provider is returned even if :meth:`ImageGenProvider.is_available`
|
||||||
|
reports False — the dispatcher surfaces a precise "X_API_KEY is not
|
||||||
|
set" error rather than silently switching backends.
|
||||||
|
- When ``image_gen.provider`` is unset, the fallback path (single-
|
||||||
|
provider shortcut and the FAL legacy preference) is filtered by
|
||||||
|
``is_available()`` so we don't pick a provider the user has no
|
||||||
|
credentials for.
|
||||||
"""
|
"""
|
||||||
configured: Optional[str] = None
|
configured: Optional[str] = None
|
||||||
try:
|
try:
|
||||||
|
|
@ -94,6 +105,17 @@ def get_active_provider() -> Optional[ImageGenProvider]:
|
||||||
with _lock:
|
with _lock:
|
||||||
snapshot = dict(_providers)
|
snapshot = dict(_providers)
|
||||||
|
|
||||||
|
def _is_available_safe(p: ImageGenProvider) -> bool:
|
||||||
|
"""Wrap ``is_available()`` so a buggy provider doesn't kill resolution."""
|
||||||
|
try:
|
||||||
|
return bool(p.is_available())
|
||||||
|
except Exception as exc: # noqa: BLE001
|
||||||
|
logger.debug("image_gen provider %s.is_available() raised %s", p.name, exc)
|
||||||
|
return False
|
||||||
|
|
||||||
|
# 1. Explicit config wins — return regardless of is_available() so the
|
||||||
|
# user gets a precise downstream error message rather than a silent
|
||||||
|
# backend switch.
|
||||||
if configured:
|
if configured:
|
||||||
provider = snapshot.get(configured)
|
provider = snapshot.get(configured)
|
||||||
if provider is not None:
|
if provider is not None:
|
||||||
|
|
@ -103,13 +125,16 @@ def get_active_provider() -> Optional[ImageGenProvider]:
|
||||||
configured,
|
configured,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Fallback: single-provider case
|
# 2. Fallback: single registered provider — but only if it's actually
|
||||||
if len(snapshot) == 1:
|
# available (no credentials = don't surface it as "active").
|
||||||
return next(iter(snapshot.values()))
|
available = [p for p in snapshot.values() if _is_available_safe(p)]
|
||||||
|
if len(available) == 1:
|
||||||
|
return available[0]
|
||||||
|
|
||||||
# Fallback: prefer legacy FAL for backward compat
|
# 3. Fallback: prefer legacy FAL for backward compat, when available.
|
||||||
if "fal" in snapshot:
|
fal = snapshot.get("fal")
|
||||||
return snapshot["fal"]
|
if fal is not None and _is_available_safe(fal):
|
||||||
|
return fal
|
||||||
|
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -114,7 +114,31 @@ _LEGACY_PREFERENCE = ("brave-free", "firecrawl", "searxng", "ddgs")
|
||||||
|
|
||||||
|
|
||||||
def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearchProvider]:
|
def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearchProvider]:
|
||||||
"""Resolve the active provider for a capability ("search" | "extract")."""
|
"""Resolve the active provider for a capability ("search" | "extract").
|
||||||
|
|
||||||
|
Resolution rules (in order):
|
||||||
|
|
||||||
|
1. **Explicit config wins, ignoring availability.** If
|
||||||
|
``web.{capability}_backend`` or ``web.backend`` names a registered
|
||||||
|
provider that supports *capability*, return it even if its
|
||||||
|
:meth:`is_available` returns False — the dispatcher will surface a
|
||||||
|
precise "X_API_KEY is not set" error to the user instead of silently
|
||||||
|
routing somewhere else. Matches legacy
|
||||||
|
:func:`tools.web_tools._get_backend` behavior for configured names.
|
||||||
|
|
||||||
|
2. **Single-provider shortcut.** When only one registered provider
|
||||||
|
supports *capability* AND ``is_available()`` reports True, return it.
|
||||||
|
|
||||||
|
3. **Legacy preference walk, filtered by availability.** Walk the
|
||||||
|
:data:`_LEGACY_PREFERENCE` order looking for a provider whose
|
||||||
|
``supports_<capability>()`` is True AND whose ``is_available()`` is
|
||||||
|
True. This is the path that fires when no config key is set — pick
|
||||||
|
the highest-priority backend the user actually has credentials for.
|
||||||
|
|
||||||
|
Returns None when no provider is configured AND no available provider
|
||||||
|
matches the legacy preference; the dispatcher then returns a "set up a
|
||||||
|
provider" error to the user.
|
||||||
|
"""
|
||||||
with _lock:
|
with _lock:
|
||||||
snapshot = dict(_providers)
|
snapshot = dict(_providers)
|
||||||
|
|
||||||
|
|
@ -125,6 +149,17 @@ def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearc
|
||||||
return bool(p.supports_extract())
|
return bool(p.supports_extract())
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
def _is_available_safe(p: WebSearchProvider) -> bool:
|
||||||
|
"""Wrap ``is_available()`` so a buggy provider doesn't kill resolution."""
|
||||||
|
try:
|
||||||
|
return bool(p.is_available())
|
||||||
|
except Exception as exc: # noqa: BLE001
|
||||||
|
logger.debug("provider %s.is_available() raised %s", p.name, exc)
|
||||||
|
return False
|
||||||
|
|
||||||
|
# 1. Explicit config wins — return regardless of is_available() so the
|
||||||
|
# user gets a precise downstream error message rather than a silent
|
||||||
|
# backend switch. Matches _get_backend() in web_tools.py.
|
||||||
if configured:
|
if configured:
|
||||||
provider = snapshot.get(configured)
|
provider = snapshot.get(configured)
|
||||||
if provider is not None and _capable(provider):
|
if provider is not None and _capable(provider):
|
||||||
|
|
@ -140,13 +175,24 @@ def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearc
|
||||||
configured, capability,
|
configured, capability,
|
||||||
)
|
)
|
||||||
|
|
||||||
eligible = [p for p in snapshot.values() if _capable(p)]
|
# 2. + 3. Fallback path — filter by availability so we don't surface
|
||||||
|
# a provider the user has no credentials for. Without this filter,
|
||||||
|
# brave-free's slot in the legacy preference order would make it
|
||||||
|
# the "active" provider on a fresh install with no API keys at all.
|
||||||
|
eligible = [
|
||||||
|
p for p in snapshot.values()
|
||||||
|
if _capable(p) and _is_available_safe(p)
|
||||||
|
]
|
||||||
if len(eligible) == 1:
|
if len(eligible) == 1:
|
||||||
return eligible[0]
|
return eligible[0]
|
||||||
|
|
||||||
for legacy in _LEGACY_PREFERENCE:
|
for legacy in _LEGACY_PREFERENCE:
|
||||||
provider = snapshot.get(legacy)
|
provider = snapshot.get(legacy)
|
||||||
if provider is not None and _capable(provider):
|
if (
|
||||||
|
provider is not None
|
||||||
|
and _capable(provider)
|
||||||
|
and _is_available_safe(provider)
|
||||||
|
):
|
||||||
return provider
|
return provider
|
||||||
|
|
||||||
return None
|
return None
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue