From 0a7cbd33424732694cc6e7b886376ee221613e03 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 14 May 2026 00:06:29 +0530 Subject: [PATCH] fix(plugins): filter resolution by is_available() in web + image_gen registries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- agent/image_gen_registry.py | 37 ++++++++++++++++++++----- agent/web_search_registry.py | 52 +++++++++++++++++++++++++++++++++--- 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/agent/image_gen_registry.py b/agent/image_gen_registry.py index 715133231cb..5d14a6f1ece 100644 --- a/agent/image_gen_registry.py +++ b/agent/image_gen_registry.py @@ -77,6 +77,17 @@ def get_active_provider() -> Optional[ImageGenProvider]: Reads ``image_gen.provider`` from config.yaml; falls back per the 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 try: @@ -94,6 +105,17 @@ def get_active_provider() -> Optional[ImageGenProvider]: with _lock: 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: provider = snapshot.get(configured) if provider is not None: @@ -103,13 +125,16 @@ def get_active_provider() -> Optional[ImageGenProvider]: configured, ) - # Fallback: single-provider case - if len(snapshot) == 1: - return next(iter(snapshot.values())) + # 2. Fallback: single registered provider — but only if it's actually + # available (no credentials = don't surface it as "active"). + 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 - if "fal" in snapshot: - return snapshot["fal"] + # 3. Fallback: prefer legacy FAL for backward compat, when available. + fal = snapshot.get("fal") + if fal is not None and _is_available_safe(fal): + return fal return None diff --git a/agent/web_search_registry.py b/agent/web_search_registry.py index 8f1e884b3cf..45f2a0f8883 100644 --- a/agent/web_search_registry.py +++ b/agent/web_search_registry.py @@ -114,7 +114,31 @@ _LEGACY_PREFERENCE = ("brave-free", "firecrawl", "searxng", "ddgs") 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_()`` 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: snapshot = dict(_providers) @@ -125,6 +149,17 @@ def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearc return bool(p.supports_extract()) 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: provider = snapshot.get(configured) if provider is not None and _capable(provider): @@ -140,13 +175,24 @@ def _resolve(configured: Optional[str], *, capability: str) -> Optional[WebSearc 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: return eligible[0] for legacy in _LEGACY_PREFERENCE: 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 None