Feat/hybrid skill retrieval - #64528
Conversation
b743b8d to
035ce7e
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thank you for tackling skill-index growth. The underlying problem is present on current origin/main (agent/prompt_builder.py:1669-1676 renders each skill description), but this implementation needs substantial rework.
Problems
agent/conversation_loop.py:371calls_build_system_prompt(..., query_text=...);run_agent.py:3752-3755accepts onlysystem_message, and this PR does not change that interface. The default-enabled path therefore raisesTypeError.- The actual prompt-builder integration is not in the production module: this PR adds
agent/prompt_builder.patchandagent/prompt_builder_patch.py, whileagent/prompt_builder.pyis unchanged. The new test attests/agent/test_skill_retrieval.py:373calls an unsupportedquery_textparameter. agent/turn_context.py:378-379clears the cached system prompt every turn. This breaks the session-stable prompt invariant documented inAGENTS.md:19-23andagent/system_prompt.py:519-532.invalidation_hooks.pycontains suggested snippets, but the claimed mutation files are not changed, so no index invalidation is actually wired.
Suggested changes
- Rework the design around the cache invariant; do not vary or rebuild the persisted system prompt per message.
- Integrate any retained retrieval logic into the real prompt construction path and cover it with an end-to-end temporary-
HERMES_HOMEtest.
Automated hermes-sweeper review.
| agent._cached_system_prompt = agent._build_system_prompt(system_message) | ||
| # prompt, or query-based semantic retrieval is active) — build from scratch. | ||
| if query_text is not None: | ||
| agent._cached_system_prompt = agent._build_system_prompt(system_message, query_text=query_text) |
There was a problem hiding this comment.
AIAgent._build_system_prompt still accepts only system_message (run_agent.py:3752-3755), and this PR does not modify it. With semantic search enabled this call raises TypeError; wire the complete production interface before passing query_text.
| _semantic_enabled = True | ||
|
|
||
| if _semantic_enabled: | ||
| agent._cached_system_prompt = None |
There was a problem hiding this comment.
Clearing the session prompt on every turn defeats the prefix-cache contract. AGENTS.md:19-23 and agent/system_prompt.py:519-532 require a byte-stable system prompt for the conversation; this needs a design that keeps dynamic retrieval out of the persisted system prompt.
| if clear_fn: | ||
| clear_fn(clear_snapshot=False) | ||
| # Without query_text the full-index path runs unchanged. | ||
| result = build_skills_system_prompt(query_text=None) |
There was a problem hiding this comment.
The production agent.prompt_builder.build_skills_system_prompt is not changed by this PR and has no query_text parameter, so this integration test fails rather than testing the feature. Apply the implementation to the production module and exercise the actual call path.
035ce7e to
4f3c2f5
Compare
|
Hi @teknium1 — thanks for the detailed review. I've reworked this based on your feedback and pushed the fix to this branch (latest commit 4f3c2f54e). Summary of what changed: build_skills_system_prompt no longer takes any query-dependent input. Its cache key is unaffected by the current turn's message, so the system prompt stays byte-identical across turns in a session, satisfying the invariant in AGENTS.md/agent/system_prompt.py. When semantic search is enabled, it renders skills names-only. Ran the full test suite locally; only pre-existing Windows-specific failures unrelated to this change (SQLite file-lock teardown timing, os.symlink privilege requirements, and a local dotfiles repo tripping workspace-detection tests) — all reproduced identically against unmodified upstream/main. Let me know if there's anything else you'd like adjusted. |
…fix-cache invariant - Extracted BM25/embedding retrieval logic from skills hub into agent/skill_retrieval.py - Refactored build_skills_system_prompt to omit query_text entirely, preserving system prompt byte identity - Injected retrieved_skills_context dynamically into the per-turn user message via API content sidecar - Plumbed index cache invalidation hooks into skill creation/deletion and sync-from-disk paths Addresses review comments on PR NousResearch#64528
e67f749 to
83e8db8
Compare
SummaryOne PR directly addresses #34823. #64528 adds per-message BM25/optional embedding retrieval, injects Top-K skill descriptions into the API-bound user turn, and renders the stable system-prompt index names-only, but the current diff also contains unresolved conflict markers and unrelated large-scale config duplication. Related pull requests
DuplicatesNo duplicate PR exists in this triaged set; contributor discussion identifies issue #34823 as a duplicate of #17649, relates it to #22620, and cites closed PR #18316 as an earlier implementation attempt. Suggested consolidationKeep #64528 open with a salvage path as the recorded best available fix: retain the per-turn sidecar injection, names-only stable prompt index, BM25 retrieval, and mutation invalidation, while removing conflict artifacts and duplicated fields/tests, restoring imported config defaults, and validating the real conversation path. This follows the visible keep_open review rather than proposing closure or integration over its blocking concerns. Complex graphflowchart 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
I34823(["issue #34823 (open)"])
P64528["PR #64528 (open)"]
P64528 -->|best fix| I34823
class I34823 open
class P64528 open
class P64528 best
class P64528 target
click I34823 "https://github.com/NousResearch/hermes-agent/issues/34823"
click P64528 "https://github.com/NousResearch/hermes-agent/pull/64528"
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 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 222 kB of PR diffs, 4 kB of issue/PR text, 4 kB of discussion (3 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
@teknium1 Thanks for the detailed review — pushed a round of fixes addressing all four points: Caching invariant: Removed the per-turn _cached_system_prompt = None resets from system_prompt.py and agent_runtime_helpers.py. The system prompt now stays byte-stable for the whole conversation — retrieval no longer touches it. Retrieved skills are instead injected as a sidecar (retrieved_skills_context) into the per-turn API content, not the persisted prompt. Also cleaned up: resolved the leftover merge conflict markers, removed a duplicated preflight_compression_blocked field on TurnContext, restored the DEFAULT_CONFIG import in hermes_cli/config.py (was inlined to ~2,500 lines during conflict resolution), and deduped the integration tests. Added test_system_prompt_byte_identity_invariant — asserts the cached system prompt is identical across two turns with different queries, while the injected retrieved_skills_context varies with the query. Full suite: 31/31 passing. Ready for another look whenever you have time. |
What does this PR do?
This PR introduces Hybrid Semantic Per-Message Skill Retrieval for the prompt assembly pipeline.
Previously, the system prompt included the descriptions of every available skill for each request. As the number of skills grows, this increases prompt size and unnecessary context. This change retrieves only the skills that are most relevant to the current user message while keeping the remaining skills discoverable through the existing skill_view tool.
The retrieval pipeline combines:
BM25 for lexical keyword matching
Dense semantic embeddings using EmbeddingIndex
Reciprocal Rank Fusion (RRF) to combine lexical and semantic rankings
Retrieved skill metadata is stored in a thread-safe SQLite database (WAL mode enabled) to support efficient indexing and concurrent access.
The prompt is now rendered using a two-tier structure:
Top-K relevant skills → Included with their full descriptions.
Remaining skills → Listed by name only, allowing the agent to fetch their details on demand using skill_view.
Related Issue
Fixes #34823
Type of Change
✨ New feature (non-breaking change that adds functionality)
✅ Tests (adding or improving test coverage)
Changes Made
Skill Retrieval
Added agent/skill_retrieval.py implementing:
BM25Scorer
EmbeddingIndex
ReciprocalRankFusion (RRF)
Added asynchronous indexing support for efficient skill indexing.
Prompt Assembly
Updated agent/prompt_builder.py to:
Perform hybrid skill retrieval for each user message.
Generate a two-tier prompt containing:
Full descriptions for the highest-ranked skills.
Names-only entries for all remaining skills.
Conversation Flow
Updated:
agent/conversation_loop.py
run_agent.py
to pass the current user message into the prompt builder and trigger index invalidation when required.
Configuration
Updated hermes_cli/config.py to add configuration and validation for semantic skill retrieval.
Index Invalidation
Added index invalidation when the available skill set changes by updating:
invalidation_hooks.py
tools/skills_sync.py
tools/skill_manager_tool.py
This ensures retrieval indices remain synchronized after skills are added, updated, deleted, or synced.
Tests
Added tests/agent/test_skill_retrieval.py containing 29 unit and integration tests covering:
BM25 scoring
Embedding retrieval
Reciprocal Rank Fusion
Prompt generation
Index invalidation
End-to-end retrieval behavior
Test Scripts
Added cross-platform test runners:
scripts/run_tests.sh
scripts/run_tests.ps1
How to Test
Run the test suite:
Unix/macOS
./scripts/run_tests.sh -v
Windows (PowerShell)
.\scripts\run_tests.ps1 -v
Verify that:
All tests pass successfully.
Only the most relevant skills are included with full descriptions in the generated system prompt.
Remaining skills are listed by name only.
Updating, deleting, or syncing skills correctly invalidates and rebuilds the retrieval index.
Existing prompt generation behavior remains unchanged when retrieval is disabled.