Skip to content

feat(tool_search): optional embedding reranker for progressive tool disclosure - #35457

Open
davidgut1982 wants to merge 1 commit into
NousResearch:mainfrom
davidgut1982:feat/tool-search-hybrid-rerank
Open

feat(tool_search): optional embedding reranker for progressive tool disclosure#35457
davidgut1982 wants to merge 1 commit into
NousResearch:mainfrom
davidgut1982:feat/tool-search-hybrid-rerank

Conversation

@davidgut1982

@davidgut1982 davidgut1982 commented May 30, 2026

Copy link
Copy Markdown
Contributor

What

Adds optional embedding-based reranker for semantic tool discovery on top of BM25 lexical search. When enabled, all tool descriptions are embedded once per process using nomic-embed-text-v2-moe (MD5-cached), then per-query tool candidates are reranked by cosine similarity. Implements progressive tool disclosure: when a profile exceeds the activation threshold, the full catalog (~54k tokens) is deferred behind tool_search stubs (~2.3k tokens) and tools are fetched on demand. Two reranking modes: pure cosine or Reciprocal Rank Fusion (RRF k=10).

Files modified: tools/tool_search.py (reranker + progressive disclosure), tests/tools/test_tool_search.py (new tests), website/docs/user-guide/features/tool-search.md (updated).

Why

BM25 lexical matching fails on semantic queries ("remind me tonight" vs "create_calendar_event"). Embedding reranker recovers those cases. Large tool catalogs consume 34-67% of a 131k context window. Progressive disclosure defers the catalog and reduces visible tools from 226 → 4, freeing 95.8% of tool-definition tokens.

Tests

pytest tests/tools/test_tool_search.py -v

53 tests pass: BM25 fallback, RRF exact-score, limit contract, dimension-mismatch, prefix payload, cache invalidation. Offline eval suite shows R@5 improvement from 0.634 (BM25) → 0.810 (with reranker).

Platforms tested

Linux (CT/LXC environment, Python 3.13)

@alt-glitch alt-glitch added type/feature New feature or request comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have labels May 30, 2026
@davidgut1982

Copy link
Copy Markdown
Contributor Author

Per-Scope Cache Improvement Added

Cherry-picked commit 09d86e6 (fix(tool_search): per-scope reranker cache) onto this PR. This adds critical multi-agent support:

  • Replaces single-slot module-level reranker singleton with a bounded scope-keyed cache (max 8 entries, FIFO eviction)
  • Each distinct toolset-scope (keyed by md5(endpoint + model + tool_names)) retains its own EmbeddingReranker instance + embedding cache
  • Key benefit: Concurrent sub-agents operating on different toolsets no longer evict each other's embedding cache, eliminating redundant endpoint calls

New test coverage:

  • TestEmbedCacheInvalidation.test_concurrent_scopes_do_not_share_reranker — proves scope B creation does NOT evict scope A's instance or cache
  • TestEmbedCacheInvalidation.test_reranker_cache_evicts_oldest_scope_when_full — validates FIFO eviction when cache is full

This is essential for orchestrator patterns where multiple concurrent agents with different MCP toolsets need to avoid thrashing the embedding endpoint.

@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 for the optional reranker work. The semantic-retrieval premise remains valid: current main's tools/tool_search.py:378-418 is BM25 plus a substring fallback only.

Problems

  • tools/tool_search.py:863-903 is a one-entry global reranker cache. A second toolset scope replaces the first, so it does not provide the per-scope retention described in the PR discussion.
  • RerankerConfig.top_k is parsed at tools/tool_search.py:131 and documented at website/docs/user-guide/features/tool-search.md:221, but reranking uses only the call limit at tools/tool_search.py:804.
  • The added example places api_key in config.yaml (website/docs/user-guide/features/tool-search.md:224), while repository documentation requires credentials in .env.

Suggested changes

  • Implement and test a bounded scope-keyed cache, including A → B → A reuse.
  • Remove top_k or implement and test its precedence.
  • Route endpoint credentials through the established secret path.
  • Preserve current-main deferred-call validation from 37df7ff01671685dae4e2d7204180beda7747a02 during salvage.

Automated hermes-sweeper review.

Comment thread tools/tool_search.py Outdated
# Lazily constructed on first use; None means reranker is disabled or not
# yet built. The reranker embeds tool texts; the cache lives here across
# search calls within the same process, invalidated when the catalog changes.
_reranker: Optional[EmbeddingReranker] = None

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.

This is a single global cache slot, so a request for scope B replaces scope A's reranker and cache. That contradicts the PR discussion's claimed bounded per-scope cache; use a scope-keyed bounded map and add an A → B → A reuse test.

Comment thread tools/tool_search.py
# search_default_limit is already applied by dispatch_tool_search before
# calling search_catalog; returning more than limit here violates the
# search_catalog(limit=N) contract and over-returns to the model.
top_k = limit

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.

RerankerConfig.top_k is parsed and documented but is not consulted here; this makes the documented setting a no-op. Please either remove it or define and test its precedence relative to the tool-call limit.

top_k: 5 # results to return (should match search_default_limit)
query_prefix: "search_query: " # nomic task prefix for queries
doc_prefix: "search_document: " # nomic task prefix for tool docs
api_key: "" # optional bearer token

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.

Please do not document bearer credentials in config.yaml. Hermes' documented convention is .env for API keys and tokens; route this through the established secret-resolution/setup path instead.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 13, 2026
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 13, 2026
@davidgut1982
davidgut1982 force-pushed the feat/tool-search-hybrid-rerank branch 2 times, most recently from e8136eb to ea64142 Compare July 14, 2026 23:07
@davidgut1982

Copy link
Copy Markdown
Contributor Author

Reworked and rebased onto current main. Addressing all three points:

  1. Global cache -> scope-keyed bounded map. Replaced the single global reranker slot with a bounded OrderedDict (FIFO, max 8) keyed per scope, so a second toolset no longer evicts the first. Added tests proving A -> B -> A reuses scope A without rebuilding, that concurrent scopes are both retained, and FIFO eviction at capacity. While here I also hardened the key: it now includes mode/rrf_k/query_prefix/doc_prefix (so a config hot-reload cannot serve a stale-mode reranker) with a NUL separator (tool names containing commas cannot collide), and the fast-path read uses an atomic dict.get() to avoid a TOCTOU against concurrent eviction.

  2. top_k removed. It was parsed and documented but never consulted (rerank honors the caller limit), so it is gone from RerankerConfig, the docs, and the tests.

  3. Credentials via .env. api_key now resolves from HERMES_EMBED_API_KEY in .env (added to .env.example) with an explicit config value still allowed as an override; the docs no longer put the token in config.yaml. api_key is also excluded from the dataclass repr to avoid leaking it in logs.

Also removed a couple of stray /tmp local-path references from the docs/comments. 64 tests pass, ruff clean.

@davidgut1982
davidgut1982 force-pushed the feat/tool-search-hybrid-rerank branch 5 times, most recently from 2a02f9b to 9ad68ad Compare July 19, 2026 03:00
…isclosure

Adds an optional embedding-based reranker over the existing BM25 tool search,
gated behind config (disabled by default). Reworked per maintainer review:

- Scope-keyed bounded reranker cache. Replaces the previous single global
  reranker slot (which let a second toolset scope evict the first) with a
  bounded OrderedDict keyed on endpoint+model+mode+rrf_k+prefixes+tool-names
  (FIFO eviction, max 8). A -> B -> A reuses scope A's instance without
  rebuilding. Cache key uses a NUL separator so tool names containing commas
  cannot collide, and includes the behavior-affecting config fields so a
  config hot-reload cannot serve a stale-mode reranker. Fast-path read uses an
  atomic dict.get() (no in/getitem TOCTOU against concurrent eviction).
- Removed the dead RerankerConfig.top_k field (reranking honors the caller's
  limit; top_k was parsed and documented but never consulted).
- Endpoint credentials go through .env (HERMES_EMBED_API_KEY) per the repo
  convention rather than config.yaml; an explicit config value still overrides.
  api_key is excluded from the dataclass repr to avoid token leakage in logs.

Tests: scope-keyed cache reuse/eviction (A->B->A, concurrent scopes, FIFO),
config parsing, rerank invocation + fallback, RRF math, prefix validation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants