feat(web): multi-source fallback chain and search_engine parameter - #53158
feat(web): multi-source fallback chain and search_engine parameter#53158Icather wants to merge 2 commits into
Conversation
8f82584 to
4c35056
Compare
New: fallback path transparency note (2026-06-28)Added a {
"_fallback_path": "Search started with \"baidu\" (returned 0 results or failed) and ultimately succeeded via \"serper\".",
"_fallback_trace": ["baidu: returned 0 results", "serpapi: not available (missing key)"],
...
}The model can present this path to the user for transparency — no more silent "baidu returned 0, silently fell back to another engine." Implementation is minimal: 18 lines in |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the fallback work; current main still returns the selected provider’s response directly, so the underlying reliability gap is real (tools/web_tools.py:686-723).
Problems
tools/web_tools.py:306on this PR uses onlyweb.backendas the auto-chain primary. Current main givesweb.search_backendper-capability precedence (hermes_cli/config.py:1260-1264), so a separate search override would be ignored.tools/web_tools.py:317-324assumeslist_providers()returns registration order, but the registry sorts by provider name (agent/web_search_registry.py:78-82). The implicit chain is therefore alphabetical rather than the documented order.tools/web_tools.py:1484-1488describes configured engine “choices,” but the schema is a free-form string and_get_valid_engine_names()is not used to create an enum.- The source-text test at
tests/tools/test_web_fallback_chain.py:177-188is an implementation/change detector rather than a behavioral regression test.
Suggested changes
- Rework this atop current registry resolution, beginning the search chain from
_get_search_backend()and preserving the disabled-plugin diagnostic covered intests/tools/test_web_providers.py:599-623. - Add behavioral coverage for divergent
web.search_backend/web.backendvalues and deterministic fallback ordering.
This is an automated hermes-sweeper review.
93fe358 to
be03f6b
Compare
Icather
left a comment
There was a problem hiding this comment.
Rebased onto current main. All four review items addressed:
- _get_fallback_chain() primary uses _get_search_backend() (respects per-capability web.search_backend override, works with divergent web.backend/web.search_backend configs)
- Dropped misleading "registration order" assumption — registry sorts alphabetically, chain is explicit
- _get_valid_engine_names() exposed via search_engine schema parameter
- Disabled-plugin diagnostic preserved in fallback exhaustion path
- Tests: de-hardcoding test removed, replaced with behavioral coverage for divergent backends, deterministic ordering, and provider availability gating
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Five PRs address or reference the provider-fallback problem. The supplied diffs verify that #53151 and #53158 add search-provider selection and runtime fallback, while #56916, #73631, and #74410 report broader search/extract-chain designs whose current diffs were not supplied and therefore cannot be compared at implementation level here.
Related pull requests
- #53151 [closed]
duplicate— (+807/-29) — closed, still relevant as an earlier broad implementation: the supplied diff adds an automatic search fallback chain, explicitsearch_enginedispatch, and nine provider plugins, but its closure reason and any supersession relationship are not established by the evidence. - #53158
related— (+471/-31) — keep open with a salvage path: the supplied diff contains the focused search fallback dispatcher and behavioral tests. Consistent with the contributor keep_open review on #53158, salvage the search-only chain after rebasing and resolving the reviewed configuration-precedence, deterministic-ordering, schema-exposure, and test-quality requirements against current main. - #56916 [closed]
related— (+455/-89) — closed, still relevant as a competing reported configuration design: its body describes capability-specific search and extraction chains with legacy scalar compatibility, but no diff or closure rationale is supplied, so neither implementation details nor supersession by another PR are verified. - #73631
related— (+1861/-117) — keep open review noted, but current implementation is unverified here: the body reports explicit search/extract chains and per-URL extraction fallback. Despite the keep_open review on #73631 and the author's report that commit80ea1efbcfixes schema exposure, no supplied diff or Verify verdict confirms that fix, so this PR cannot yet be selected as the consolidation base. - #74410
related— (+695/-11) — keep open review noted, but current implementation is unverified here: the body reports primary-plus-fallback search/extract lists. The contributor keep_open review identifies that ordinary extraction error rows do not trigger fallback; without a supplied updated diff, that blocking contract issue remains unverified.
Duplicates
#53151 and #53158 substantially overlap on automatic search fallback and explicit engine selection, but the evidence does not establish a formal duplicate or supersession relationship. #56916, #73631, and #74410 overlap on configurable search/extract fallback chains, while their differing reported configuration contracts and extraction semantics prevent a verified duplicate designation without their diffs.
Suggested consolidation
Author action: rebase #53158 onto main, or split out its supplied, reviewable search-only fallback dispatcher and behavioral tests as the salvageable part. Do not select #73631 from the author-reported 80ea1efbc fix alone, and do not close #73631 or #74410 over their contributor keep_open reviews until updated diffs verify the schema-exposure fix and returned-error-row extraction fallback respectively; no additional duplicate closures are supported by the present evidence.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup53151 ["PRs duplicating each other"]
P53151["PR #53151 (closed)"]
P53158["PR #53158 (open)"]
end
class P53151 closed
class P53158 open
class P53158 target
click P53151 "https://github.com/NousResearch/hermes-agent/pull/53151"
click P53158 "https://github.com/NousResearch/hermes-agent/pull/53158"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Diffs were read for 2 of 5 PRs (rest unavailable); Assessment working set: 64 kB of PR diffs, 13 kB of issue/PR text, 13 kB of discussion (14 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Delta since our previous triage comment
@richkapp explicitly corrected the review chronology for #73631: the fix was pushed before the independent review, but a subsequent read-only review against exact head 80ea1efbc96fea9b80623bf89572bbbd14b9957b returned PUBLISH. That new report says the contributor-raised schema-exposure gap is resolved for both explicit chains and that GitHub CI is 38/38, strengthening—but not replacing—the required maintainer review because the updated diff is not supplied here.
Changed pull requests
- #73631
related— (+1861/-117) — consolidation candidate pending maintainer review: despite the visible keep_open review on #73631, @richkapp reports that exact-head review verified the requestedcheck_web_api_key()schema exposure for bothsearch_backendsandextract_backends, with 38/38 CI checks passing; this addresses the reported blocker, though the fix is not independently diff-verifiable from this record.
Suggested consolidation
The consolidation direction is unchanged: retain #73631 as the preferred candidate, now with stronger exact-head evidence, and require normal maintainer review before merge.
Complex graph unchanged since our previous triage comment.
Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Diffs were read for 2 of 5 PRs (rest unavailable); Assessment working set: 64 kB of PR diffs, 13 kB of issue/PR text, 14 kB of discussion (15 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Rebased onto current main; addresses review feedback and CI failures: - Public error envelope matches main's contract: all-backends-unavailable raises through the top-level "Error searching web: ..." handler with a setup hint (e.g. FIRECRAWL_API_KEY); per-backend diagnostics stay in logs and the model-facing _fallback_trace, never in the error string - Auto mode seeds the fallback chain from _get_search_backend() (respects per-capability web.search_backend override); a live backend that returns a failure result or raises stays authoritative (main's single-backend contract), with the one-shot keyless rescue integrated on both paths - WEB_SEARCH_SCHEMA gains a search_engine property whose enum is built from _get_valid_engine_names(); the registry handler forwards search_engine only when the model supplied it - code_execution_tool stub synced (schema drift test green) - tests/tools/test_web_fallback_chain.py: deterministic rescue-off fixture, updated for the authoritative-failure semantics
be03f6b to
7b0ad9a
Compare
|
Thanks for the thorough review — rebased onto current main ( Review items:
CI failures also fixed (all previously-red checks now green locally):
Local run: 195 passed (web_fallback_chain, keyless rescue/fallback, web_tools_config, web_providers, registry, schema-drift). The only local skips are |
…riority Addresses review: - Seed the provider list from agent.web_search_registry.list_providers() (search-capable) instead of tools.web_tools._get_fallback_chain(), so this command stands alone and does not depend on the companion fallback-chain PR (NousResearch#53158). The previous import was swallowed and the command always reported "no providers" with an empty fallback_backends config. - Tests now patch agent.web_search_registry.get_provider — the module where _provider_display_name imports from — instead of a non-existent hermes_cli.tools_config.get_provider binding (AttributeError). - Dropped the source-text assertion (reads the source file looking for hardcoded labels); registry-driven behavior is covered by mock-based tests instead. - Added a test for registry-seeded initial order + reorder write-back.
…ack chain The availability gate in _search_with_fallback treated a missing key as a fallback trigger, but keyless-ring vendors (e.g. Tavily) can serve requests without one. main's dispatcher never gates the configured backend on is_available() for this reason — the provider's own search() routes through the ring. Fall back only when the provider is neither available nor keyless-available (is_keyless_available()). Fixes the CI failure in tests/tools/test_web_tools_tavily.py:: test_search_keyless_dispatch (KeyError: 'success' — the call returned the "no backend available" envelope instead of routing through the ring).
What does this PR do?
When a search provider fails, Hermes previously returned an error immediately — no retry with another backend. This adds a configurable fallback chain and a
search_engineparameter so the model picks engines and Hermes retries when the configured backend is unavailable. Works with all currently-registered backends and auto-extends to new providers.Rebased onto current
mainand updated for the keyless-rescue mechanism landed in the meantime.Related Issue
Follows the intent of #35690 (closed without review) and extends it with dynamic engine names from the registry.
Type of Change
Changes Made
web_search_tool(query, limit, search_engine="auto")— newsearch_engineparameter;"auto"walks the fallback chain, explicit names run a single backend with no fallback_get_fallback_chain()— seeds from_get_search_backend()(per-capabilityweb.search_backendoverride), thenweb.fallback_backendsfrom config (user-ordered), then remaining registered providers_search_with_fallback()— skips backends that are unregistered or unavailable (missing key/package); a live backend that returns a failure result or raises stays authoritative (main's single-backend contract) with the one-shot keyless rescue integrated on both the explicit and auto paths{"error": "Error searching web: ..."}with a setup hint (e.g.FIRECRAWL_API_KEY). Per-backend diagnostics never leak into the public error string — they stay in the log and the model-facing_fallback_traceWEB_SEARCH_SCHEMA—search_engineproperty withenumbuilt dynamically from_get_valid_engine_names()(currently-available providers +"auto"); registry handler forwards it only when supplied_get_registered_backend_names()/_get_valid_engine_names()— dynamically sourced from the plugin registry, no hardcoded liststools/code_execution_tool.py—web_searchsandbox stub synced with the newsearch_engineparameterTests
tests/tools/test_web_fallback_chain.py— behavior tests for chain ordering, first-success return, skip-unavailable, authoritative-failure semantics, engine-name sets (deterministic rescue-off fixture)How to Test
BRAVE_SEARCH_API_KEYweb.fallback_backends: [brave-free, ddgs]in config.yamlweb_search→ should use brave-freeBRAVE_SEARCH_API_KEYandpip install ddgs→ should fall back to ddgsweb_searchwithsearch_engine: "ddgs"→ pinned to ddgs only{"error": "Error searching web: No web search backend available. Set a provider API key (e.g. FIRECRAWL_API_KEY) or run \hermes tools`."}`Checklist
Code
pytest tests/ -qand all tests passtests/tools/test_web_fallback_chain.py)Documentation & Housekeeping