feat(tools): hermes tools web reorder — interactive search provider priority - #53160
feat(tools): hermes tools web reorder — interactive search provider priority#53160Icather wants to merge 1 commit into
Conversation
7a645c1 to
29d6c33
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding a CLI-oriented configuration surface.
Problems
- This command writes
web.fallback_backends(hermes_cli/tools_config.py:3439in this PR), but currentmainresolves web search fromweb.search_backend/web.backendonly (tools/web_tools.py:273-308;agent/web_search_registry.py:281-288). The_get_fallback_chainimport at PR line 3391 is absent on currentmain; its exception is swallowed at line 3397, so a normal empty configuration reports no providers. tests/hermes_cli/test_web_reorder.py:26and:34patchhermes_cli.tools_config.get_provider, while the implementation importsget_providerlocally at PRtools_config.py:3462. Those patch targets do not exist.
Suggested changes
- Land or explicitly depend on the actual fallback-chain resolver first, then add a temp-
HERMES_HOMEintegration test proving this command changes runtime fallback order. - Patch
agent.web_search_registry.get_provider(or introduce a real module-level binding) and replace the source-text test attest_web_reorder.py:11-21with behavior coverage.
Automated hermes-sweeper review.
| if not current: | ||
| # Populate from auto-discovered providers | ||
| try: | ||
| from tools.web_tools import _get_fallback_chain |
There was a problem hiding this comment.
_get_fallback_chain is not present on current main; this ImportError is swallowed below, so with the normal empty fallback_backends setting the command always reports no providers. Please make the fallback resolver/config consumer a prerequisite and test the real runtime path.
| def test_falls_back_on_unregistered(self): | ||
| """When provider is not in registry, falls back to title case.""" | ||
| from hermes_cli.tools_config import _provider_display_name | ||
| with patch("hermes_cli.tools_config.get_provider") as mock_gp: |
There was a problem hiding this comment.
get_provider is imported locally inside _provider_display_name, not bound on hermes_cli.tools_config, so this patch() target raises AttributeError. Patch agent.web_search_registry.get_provider, or change the implementation to use a module-level binding.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address interactive reordering of web-search providers: #53152 combines the CLI with fallback-chain logic and multiple provider plugins, while #53160 isolates the CLI and tests but still writes a configuration key that current main does not consume. No Verify verdict is recorded for either PR.
Related pull requests
- #53152 [closed]
duplicate— (+936/-29) — broader overlapping implementation, with no Verify verdict: it adds the reorder command together with fallback-chain runtime support and numerous provider plugins. Although closed for an unspecified reason, it remains relevant as the only diff here that connectsweb.fallback_backendsto runtime search behavior. - #53160
related— (+198/-0) — keep open with a salvage path, with no Verify verdict: it provides the narrower reorder UI and registry-derived labels, but current main does not consumeweb.fallback_backends, the missing_get_fallback_chainimport is silently swallowed, and two tests patch a nonexistent module-levelget_providerbinding. This follows the contributor keep_open review on #53160 while making its documented blockers explicit.
Duplicates
#53152 and #53160 substantially duplicate the same hermes tools web reorder parser and configuration-writing flow; #53152 additionally includes the fallback-chain runtime and provider implementations, whereas #53160 adds focused but currently defective tests.
Suggested consolidation
Keep #53160 open with a salvage path: first land or explicitly depend on a runtime fallback-chain resolver, then add a temporary-HERMES_HOME integration test proving that the saved order changes runtime provider selection, patch agent.web_search_registry.get_provider or introduce a real module-level binding, and replace the source-text assertion with behavioral coverage. Keep #53152 closed because no evidence establishes why it was closed or that it was formally superseded; use its fallback-chain portion only as an implementation reference for resolving #53160's runtime gap.
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 Dup53152 ["PRs duplicating each other"]
P53152["PR #53152 (closed)"]
P53160["PR #53160 (open)"]
end
class P53152 closed
class P53160 open
class P53160 target
click P53152 "https://github.com/NousResearch/hermes-agent/pull/53152"
click P53160 "https://github.com/NousResearch/hermes-agent/pull/53160"
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 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 55 kB of PR diffs, 4 kB of issue/PR text, 6 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
…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.
655d7ff to
b596a7e
Compare
|
Rebased onto current main and addressed both items:
Also dropped the source-text assertion ( Local: |
|
The |
What does this PR do?
Search providers land in registration order with no interactive way to re-prioritize them for the fallback chain. This adds
hermes tools web reorder— an interactive CLI command that lists the current order, accepts a space-separated reorder from the user, validates it, and writes back toweb.fallback_backendsin config.yaml.Related Issue
Companion to the web search fallback chain work (#53158).
Type of Change
Changes Made
hermes_cli/tools_config.py—_tools_web_reorder()(~80 lines): interactive numbered-list prompt, completeness validation, config write viasave_config()hermes_cli/tools_config.py—_provider_display_name()helper: reads display names fromget_provider(name).display_name— zero hardcoded labelshermes_cli/subcommands/tools.py—hermes tools web reordersub-subparser registrationTests added
tests/hermes_cli/test_web_reorder.py— 7 tests: de-hardcoding verification, registry-based display names, fallback for unregistered, validation of invalid/duplicate/out-of-range input, empty-input preservationHow to Test
hermes tools web reorder3 1 2to reorder → should validate and saveconfig.yaml→web.fallback_backendsreflects the new orderChecklist
Code
pytest tests/ -qand all tests passtests/hermes_cli/test_web_reorder.py)Documentation & Housekeeping
cli-config.yaml.example— or N/A (no new config keys)