Skip to content

fix(web-tools): ensure plugins loaded before backend selection for plugin-registered providers - #67309

Open
mblauser wants to merge 3 commits into
NousResearch:mainfrom
mblauser:fix/plugin-provider-load-order
Open

fix(web-tools): ensure plugins loaded before backend selection for plugin-registered providers#67309
mblauser wants to merge 3 commits into
NousResearch:mainfrom
mblauser:fix/plugin-provider-load-order

Conversation

@mblauser

@mblauser mblauser commented Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown

What does this PR do?

Plugin-registered web providers (any provider contributed via the plugin system rather than bundled into core) are correctly discovered and correctly registered, but they are unreachable from backend selection in tools/web_tools.py, because backend selection can run before plugin discovery has populated the registry. The user-facing symptom is a silent fallback to the shared web.backend, which for extract-only plugin providers means landing on a search-only backend (e.g. searxng) and returning a "search-only backend" error for web_extract.

The specific fail-case: the hermes-plugin-crawl4ai (https://github.com/mblauser/hermes-plugin-crawl4ai) plugin, set as web.extract_backend: crawl4ai, which every diagnostic said was working (installs cleanly, loads cleanly, registers itself, reports supports_extract() = True and is_available() = True), yet every extraction fell through to searxng with "SearXNG is a search-only backend and cannot extract URL content."

This is worth fixing because the plugin pattern is the project's intended extension path for web providers (per AGENTS.md: "capability lives at the edges"). A plugin that loads and registers correctly but is silently unreachable from selection is a confusing failure mode. Every diagnostic the user has says "working", and the tool still errors out.

The fix moves _ensure_web_plugins_loaded() into the two chokepoint functions (_is_backend_available() and _get_backend()) that all backend-resolution paths pass through, rather than placing it in one specific caller. This covers the full bug class (all sibling call paths) not just the symptom hit.

Related Issue

No existing issue tracks this. A related PR (#67110) addresses the narrower symptom for extract backends only. This PR supersedes that approach by fixing at the shared chokepoint, covering all backend resolution paths including _get_backend(), check_web_api_key, and any future callers.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/web_tools.py: added _ensure_web_plugins_loaded() at the top of _is_backend_available() (line ~324) and _get_backend() (line ~230). Removed the call previously added at _get_capability_backend() from the initial attempt (net +2 lines, -3 lines).

How to Test

Prerequisites: a plugin-registered web provider (e.g. crawl4ai) configured as web.extract_backend with a non-plugin web.backend (e.g. searxng).

  1. Run backend selection check:

    python -c "from tools.web_tools import _get_extract_backend; print(_get_extract_backend())"

    Expected: crawl4ai (the plugin-registered provider), not searxng.

  2. Run availability check:

    python -c "from tools.web_tools import _is_backend_available; print(_is_backend_available('crawl4ai'))"

    Expected: True.

  3. Run end-to-end extraction:

    python -c "import asyncio, json; from tools.web_tools import web_extract_tool; r = json.loads(asyncio.run(web_extract_tool(['https://example.com']))); print('error:', r['results'][0]['error'])"

    Expected: no error, title returned.

  4. Verify regression (search still works via the configured web.backend):

    python -c "import json; from tools.web_tools import web_search_tool; r = json.loads(web_search_tool('test', limit=1)); print('success:', r['success'], 'results:', len(r['data']['web']))"

    Expected: success: True with at least 1 result.

Root cause (for reviewers)

The order in which things load is the problem:

 1. web_extract_tool() is called
 2.   → _get_extract_backend()
 3.     → _get_capability_backend()
 4.       → _is_backend_available('crawl4ai')     ← runs FIRST
 5.         → _registered_web_provider_available() ← registry is EMPTY
 6.           → returns False (plugin not found)
 7.         → falls through to legacy env-var probes; nothing matches
 8.       → falls back to shared _get_backend() → 'searxng'
 9.     → crawl4ai is never selected
10.  _ensure_web_plugins_loaded()               ← runs AFTER selection
11.    → crawl4ai finally registered... too late

Line 10 (_ensure_web_plugins_loaded()) is already called inside web_search_tool() and web_extract_tool(), but both sit after the backend-selection call. The existing placement makes dispatch work once a backend is chosen, but not the choice itself. That is the gap this fix corrects.

For backends in _LEGACY_WEB_BACKENDS (the bundled providers), _is_backend_available() uses a cheap hardcoded env-var probe, so they never hit this. For anything else (i.e. any plugin-registered provider) it delegates to the registry, which is empty at that point. The fix moves plugin discovery before registry reads, so the order becomes correct.

Why this approach over a narrower fix

An earlier sibling PR (#67110) adds _ensure_web_plugins_loaded() only to the extract-backend resolution path. This PR goes further by fixing at the two chokepoints the file already documents as the shared resolution paths:

  • _is_backend_available(): its docstring calls it "the single chokepoint through which _get_backend, _get_capability_backend, and check_web_api_key all resolve availability"
  • _get_backend(): the shared fallback that any caller reaches when no per-capability override is set or when an override fails availability

This covers every documented caller, not just extract, and satisfies the AGENTS.md rubric: fixes must "fix the whole bug class (sibling call paths included) not just the one site the reporter hit."

Why this is low risk

_ensure_web_plugins_loaded() is idempotent and caches after the first call; subsequent calls are a boolean check. The xai probe comment at line 349 notes _is_backend_available() runs on every hermes tools repaint, so the cache matters and it holds. No change to selection semantics, fallback priority, or bundled-provider behavior. Backends in _LEGACY_WEB_BACKENDS are resolved via hardcoded env-var probes and never touch the registry.

Verification

End-to-end against a live crawl4ai instance with web.extract_backend: crawl4ai and web.backend: searxng:

Check Before fix After fix
_get_extract_backend() returns searxng (wrong) crawl4ai (correct)
_is_backend_available('crawl4ai') False True
web_extract_tool('https://example.com') error success
web_search_tool('test', limit=2) via searxng works still works

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(web-tools): ensure plugins loaded before backend selection for plugin-registered providers)
  • I searched for existing PRs to make sure this isn't a duplicate. Related PR fix(web): load plugins before resolving extract backend #67110 addresses a narrower scope (extract only); this PR covers the full bug class and supersedes it.
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (untested; relied on manual end-to-end verification against a live instance)
  • I've added tests for my changes (not included in this PR; the verification steps above were run manually)
  • I've tested on my platform: Ubuntu 24.04 (self-hosted)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) -- this PR body documents the fix reasoning. No README or docstring changes needed.
  • N/A -- cli-config.yaml.example (no config changes)
  • N/A -- CONTRIBUTING.md or AGENTS.md (no architecture or workflow changes)
  • N/A -- cross-platform impact (change is in platform-neutral Python)
  • N/A -- tool descriptions/schemas (no tool behavior changes)

Files changed

tools/web_tools.py: +2 lines, −3 lines (revert of first attempt)

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets comp/plugins Plugin system and bundled plugins tool/web Web search and extraction P3 Low — cosmetic, nice to have labels Jul 19, 2026
@alt-glitch

alt-glitch commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #67110 and merged #34563. #67110 moves discovery ahead of the extract dispatch only; this PR loads it at the shared backend and availability gates, so the scopes are overlapping but not duplicate.

teknium1
teknium1 reviewed Jul 19, 2026
View reviewed changes

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the premise is confirmed on current main. web_extract_tool() resolves _get_extract_backend() at tools/web_tools.py:858, while plugin discovery occurs only afterward at tools/web_tools.py:867; configured non-legacy providers therefore consult an empty registry through _is_backend_available() (tools/web_tools.py:304-308, 323-327). The proposed calls at the shared gates match the issue, and _ensure_plugins_discovered() is idempotent (hermes_cli/plugins.py:1286-1287, 2330-2337).

Problems

  • The diff has no regression test. The closest custom-provider coverage pre-registers its fake provider in tests/tools/test_web_tools_config.py:753-759, so it does not verify discovery occurs before backend selection.

Suggested changes

  • Add a cold-registry regression test that has _ensure_web_plugins_loaded() register an available fake configured provider, then asserts _get_extract_backend() selects it.

Automated hermes-sweeper review.

Comment thread tools/web_tools.py
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 19, 2026
@mblauser mblauser mentioned this pull request Jul 19, 2026
Open
2 tasks
@mblauser

mblauser commented Jul 20, 2026

Copy link
Copy Markdown
Author

I've addressed the review feedback by adding the requested cold-registry regression test. Whenever convenient, would someone please approve the workflow run so the CI can execute and verify the changes? No rush; happy to wait for your review timeline.

@mblauser
mblauser requested a review from teknium1 July 27, 2026 19:51
@mblauser

mblauser commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

This PR was flagged at P3 "Low - cosmetic, nice to have" but the underlying blocking bug is more than cosmetic. It prevents custom web_extract plugins such as hermes-plugin-crawl4ai for a self-hosted crawl4ai from loading. This simple fix solves that.

@GottZ GottZ mentioned this pull request Jul 28, 2026
Open
@mblauser
test(web-tools): replace cold-registry regression test with verified …
f8d74bd 
…suite

The test added in 029b5bf did not exercise the fix. It called
register_provider() directly after _reset_for_tests(), so the registry was
pre-populated by the test rather than by discovery, and it never engaged
_ensure_web_plugins_loaded() at all. It passed with both fix lines removed,
which made it a coverage illusion rather than a regression guard.

Replace it with tests/tools/test_plugin_discovery_ordering.py:

- test_discovery_precedes_registry_read, parametrized over
  _is_backend_available, _get_backend and _get_extract_backend. Spies both
  registry helpers (_registered_web_provider and
  _registered_web_provider_available, since the two chokepoints use
  different ones) plus discovery, and asserts discovery precedes the
  registry read. Enforces the ordering invariant rather than this specific
  fix, so a future registry consumer that skips discovery also fails.
- Two cold-registry selection tests covering the capability-override path
  and the plain web.backend path, pinning each of the two fix lines
  independently.
- A negative test so discovery running does not make an unregistered
  backend name pass.
- A re-entrancy test covering a plugin whose import-time code calls back
  into _is_backend_available() during its own registration. Skips when the
  fix is absent.
- One integration-marked test running real discovery, so the mocked tests
  are not resting on an unverified premise.

Verified against the tree with and without the fix: the ordering and
selection tests fail without it and pass with it, and reverting either fix
line individually fails only the tests covering that line.

Also ran the full set of tests/tools/ files that import or exercise
tools.web_tools (16 files, 386 tests) with the fix applied: all pass, no
unrelated regressions.

No production code changes in this commit.
@mblauser

mblauser commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Thanks for pointing out the errors in the test methodology. The test pre-populated the registry itself rather than letting discovery do it, so it never exercised the ordering this PR changes. I have replaced it with a suite that I verified fails without the fix.

Why the fix sits at the two shared gates

Some context on how this got to where it is, because the shape of the fix is the main thing worth reviewing and it only makes sense against what came before it.

#34563 introduced _ensure_web_plugins_loaded() and wired it at the dispatch sites. That fixed the symptom but left a gap inside the dispatch functions, where backend resolution runs before the discovery call. Everything here is built directly on that work.

#67110 found that exact gap in web_extract_tool. Credit where it belongs, since it took someone actually tracing the call order to spot it, and that diagnosis is what this PR acts on.

The place where I would push for merging this PR is the wider scope that it fixes. Moving the call earlier in web_extract_tool fixes the extract path. But the same ordering hazard exists on every other path that reads the provider registry, and there are three of them:

  • _get_backend() reads the registry at web_tools.py:231 through _registered_web_provider()
  • _is_backend_available() reads it at web_tools.py:325 through _registered_web_provider_available()
  • check_web_api_key() reaches the registry through _is_backend_available()

Those are two distinct registry reads through two distinct helpers, which is easy to miss (I missed it myself while writing the tests, see below). Fixing the extract caller leaves the search path and the availability gate still resolving against a possibly empty registry. If we fix this too narrowly, we would be back here for the third time with a third narrow patch.

Putting the call at _get_backend() and _is_backend_available() covers all three entry points at once, because every resolution path in the module funnels through one or both of them. That is not a cleverer fix, it is just a fix at the join instead of at the leaves. The cost is two lines and one extra boolean check per call, since _ensure_web_plugins_loaded() short circuits after the first invocation via the _discovered guard in PluginManager.discover_and_load().

I am not attached to this particular placement if you see a reason it is wrong. If there is a hot path where even the guard check matters, or if you would rather this live one level up, I am happy to move it. The property I care about is that no registry consumer can be added later without discovery having run first, and the ordering test below is written to enforce that property rather than this specific implementation of it.

The replacement tests

Five classes, eight tests, one marked integration per the existing marker convention in pyproject.toml:

  • test_discovery_precedes_registry_read, parametrized across _is_backend_available, _get_backend, and _get_extract_backend. Spies on discovery and on both registry helpers, then asserts discovery happened before the registry was read. This is the one that generalizes: a future registry consumer that forgets the call fails here without anyone needing to remember this bug existed.
  • Two cold registry selection tests that check the actual outcome (the plugin provider gets selected), one through the capability override path and one through the plain web.backend path. These pin the two fix lines separately. More on why that matters in a second.
  • A negative test, so that discovery running does not accidentally make an unregistered backend name pass. Guards against the availability check degrading into an unconditional yes.
  • A re-entrancy test. A plugin's import time code can legitimately call back into _is_backend_available() while deciding whether to register itself, and adding this call to two frequently hit functions makes that path more reachable than it was. It's designed to validate that the re-entrancy guard in discover_and_load() terminates cleanly when a plugin calls back into the availability check during its own registration. Without the fix there's no re-entrant path to exercise.
  • One integration test that runs real, unmocked discovery and asserts a bundled provider actually lands in the registry. The other tests mock _ensure_web_plugins_loaded(), which proves the call ordering but would still pass if discovery itself were broken. This one closes that gap so the premise is tested rather than assumed.

What I checked, and one thing this caught

Ran the suite against the tree with the fix applied and with it reverted:

  • Without the fix, the ordering and cold registry selection tests fail. The failure is the actual reported symptom: _get_extract_backend() returns the default instead of the configured plugin provider.
  • With the fix, all eight pass.
  • Reverting only the _get_backend() line and leaving the other in place fails exactly two tests, the two that cover that specific line, and nothing else. So neither line can be deleted later without a test noticing. This was worth checking, because my first draft of the suite would have let the _get_backend() line be removed silently.

I also ran every test in tests/tools/ that actually imports or exercises tools.web_tools (16 files, 386 tests, found by grepping for the import rather than guessing) with the fix applied: all pass, no unrelated regressions.

That last point is also how I found a bug in my own test. My first ordering spy only wrapped _registered_web_provider_available(), which meant the _get_backend case was passing for the wrong reason, since _get_backend() reads the registry through _registered_web_provider() instead. Running it against the fixed tree surfaced that as an unexpected failure. Both helpers are spied now. Mentioning it because it is the same category of mistake as the original test: something that looks like it is testing the thing but is not, and the only reliable way I know to catch it is to run the test against code where it is supposed to fail.

@mblauser

mblauser commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks to #58320 for flagging the flawed test methodology in the earlier commit on this PR. That's been corrected: see the latest commit and comment above. The fix itself is ready and just waiting on review and merge. Once that happens, it closes out the plugin discovery ordering piece of the wider web-plugin issues. #58320's specific ask around clearer failure messaging is a separate, still-open concern and deserves its own follow-up.

@GottZ GottZ mentioned this pull request Aug 3, 2026
Open
7 tasks
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Twenty-four PRs address or reference three linked causes: cold-registry discovery ordering, hardcoded provider-selection/tool gates, and non-actionable web-tool failures. #34563 and #57779 are merged reference implementations for dispatcher discovery and registry-aware provider selection, while #67309, #67110, #71791, and #72646 cover the remaining selection-before-discovery gap and #58359 separately covers failure guidance.

Related pull requests

Duplicates

#27584, #27700, #28202, and #69144 overlap on discovery-before-dispatch, with #34563 as the merged reference. #31829, #31887, #32465, #33516, #33902, #36094, #38375, #52057, #56201, and #56761 overlap with the provider-gate class implemented by #57779; #67110, #67309, #71791, and the web-selection portion of #72646 overlap on discovery-before-selection.

Suggested consolidation

Keep #67309 open with a salvage path: retain the shared-gate fix and its corrected cold-registry tests, then obtain an independent run/review against current main. Close #67110 as a narrower duplicate of #67309; despite the keep_open review on #71791, its diff duplicates that ordering fix while adding a second discovery wrapper and omitting check_web_api_key() coverage, so close it as a duplicate of #67309; keep #72646 open only to split and preserve its distinct display fix while addressing its check_web_api_key() review, and keep #58359 open only after replacing the unsupported cache/browser guidance, adding focused tests, and removing the unrelated Himalaya edit.

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
    I27683(["issue #27683 (closed)"])
    subgraph Dup67309 ["PRs duplicating each other"]
        P67309["PR #67309 (open)"]
        P71791["PR #71791 (open)"]
    end
    P67309 -.->|partial| I27683
    class I27683 closed
    class P67309 open
    class P71791 open
    class P67309 target
    click I27683 "https://github.com/NousResearch/hermes-agent/issues/27683"
    click P67309 "https://github.com/NousResearch/hermes-agent/pull/67309"
    click P71791 "https://github.com/NousResearch/hermes-agent/pull/71791"
Loading

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 24 pull requests and 8 issues in this complex. Each diff was read against this issue; Assessment working set: 147 kB of PR diffs, 118 kB of issue/PR text, 64 kB of discussion (79 comments), 96 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@GottZ GottZ mentioned this pull request Aug 3, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teknium1 teknium1 Awaiting requested review from teknium1

Assignees

No one assigned

Labels

comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/web Web search and extraction type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mblauser @alt-glitch @GottZ @teknium1