feat(retrieval): wire HRR structural-query lane into retrieve_v2 (#152) - #503
Merged
Merged
Conversation
Mirrors VocabBridgeCache and BM25IndexCache: lazy build, store- invalidation subscription, drop-on-mutate. Long-running consumers (retrieve_v2 with use_hrr_structural=True, future bench harness) should pass an explicit cache to amortise the per-belief HRR encode cost across queries instead of rebuilding per call. Module-level docstring updated to reflect the wiring intent; substrate-only framing was leftover from the deferred-integration state. The actual retrieve_v2 routing lands in the next commit. Tests: lazy_build_then_reuse, invalidate_on_store_mutation, explicit_invalidate_drops_index. Existing 18 unit tests + 2 perf- gated unchanged.
The substrate (HRRStructIndex, parse_structural_marker) and the flag resolver (is_hrr_structural_enabled) were shipped at v1.7.0 but never connected: the flag was a phantom — setting it ON changed no behavior because no retrieval entry point imported the index or called the marker parser. PR audit on main HEAD f547779 found zero call sites outside hrr_index.py itself. This commit closes the loop. retrieve_v2 grows two kwargs (use_hrr_structural, hrr_struct_index_cache) and a routing branch that fires BEFORE the vocab-bridge rewrite — a '<KIND>:<target_id>' marker must not be munged into bag-of-words expansions. On marker hit + flag ON the HRR lane returns directly with locks pinned and the top-K HRR-ranked beliefs packed against the budget; on miss (non-marker query, marker with unknown target, or flag OFF) the call falls through to the textual lane unchanged. Per spec the lanes are parallel, never blended. Default-OFF preserved (matches #154 policy until #437 reproducibility harness clears). Locked beliefs continue to pin to head and bypass the budget per the existing public-API contract; HRR results are de-duped against the locked set so the pin invariant holds. Tests: 118 retrieval-area tests pass (test_retrieve_v2, test_retrieval_cache, test_retrieval_l0_first, test_retrieval_smoke, test_retrieval_token_budget, test_retrieve_doc_anchors, test_retrieve_v2_temporal_sort, test_compression_integration, test_vocab_bridge, test_vocab_bridge_integration, test_clustering). Integration tests for the new routing land in the next commit.
…152) Six tests proving the wiring is real (not just module-shipped): - IT1 marker + flag ON returns HRR-ranked beliefs (b1 -CONTRADICTS-> b2 surfaces b1 for query 'CONTRADICTS:b2') - IT2 marker + flag OFF falls through to textual lane (b1 absent because textual BM25 over the literal string 'CONTRADICTS:b2' cannot match content 'content of b1' — proves the structural lane was the only path that could surface it) - IT3 non-marker query + flag ON byte-identical to flag OFF (the flag is a no-op on normal text queries, preserving default-OFF posture for non-marker traffic) - IT4 marker + unknown target falls through to textual gracefully (no exception, valid RetrievalResult) - IT5 explicit cache reuses the index across calls (cache._index identity stable across two retrieve_v2 calls) - IT6 locks pin to head when the structural lane fires (locked_ids invariant preserved) Each test is end-to-end through retrieve_v2 — exercises the full kwarg surface, the routing branch, _route_structural_query, HRRStructIndex.probe, and the budget pack. Full non-bench-gate suite: 2924 passed, 26 skipped.
…#152) The CONFIG.md entry from when the substrate landed (#152 close at 2026-04-28) said only 'lane is implemented and stays opt-in pending the #154 benchmark gate' — true at the module level, misleading at the integration level. Setting the flag did nothing then. Now it does, so the doc reflects what actually happens. Adds: - Routing diagram (structural-marker hit -> HRRStructIndex; miss -> textual lane). - The supported edge-type kinds + the case-sensitivity rule. - A 5-row example table covering hit, miss-by-text, miss-by-empty- target, miss-by-unknown-target. - The locked-pin invariant + budget pack semantics. - The HRRStructIndexCache amortisation guidance for long-running callers. - The precedence chain and the default-on flip gate (#154 bench).
Reviewer's GuideWires the previously shipped HRR structural-query substrate into retrieve_v2 by adding a structural routing branch and cache, plus tests and documentation, so structural markers can be served via an HRR lane in parallel to the existing textual lane while remaining default-off. Sequence diagram for retrieve_v2 HRR structural-query routingsequenceDiagram
actor Client
participant RetrieveV2 as retrieve_v2
participant FlagResolver as is_hrr_structural_enabled
participant HRRRouter as _route_structural_query
participant MarkerParser as parse_structural_marker
participant Cache as HRRStructIndexCache
participant Index as HRRStructIndex
participant Store as MemoryStore
participant TextualLane as textual_lane
Client->>RetrieveV2: retrieve_v2(query, use_hrr_structural, hrr_struct_index_cache, ...)
RetrieveV2->>FlagResolver: is_hrr_structural_enabled(use_hrr_structural)
FlagResolver-->>RetrieveV2: enabled_flag
alt HRR structural enabled
RetrieveV2->>HRRRouter: _route_structural_query(Store, query, cache, top_k, include_locked, budget)
HRRRouter->>MarkerParser: parse_structural_marker(query)
MarkerParser-->>HRRRouter: (kind, target_id) or None
alt query is structural marker
alt cache is None
HRRRouter->>Index: HRRStructIndex()
HRRRouter->>Index: build(Store)
else cache provided
HRRRouter->>Cache: get()
Cache-->>HRRRouter: Index
end
HRRRouter->>Index: probe(kind, target_id, top_k)
Index-->>HRRRouter: hits
alt hits not empty
HRRRouter->>Store: list_locked_beliefs()
Store-->>HRRRouter: locked_beliefs
HRRRouter->>Store: get_belief(belief_id) for each hit
Store-->>HRRRouter: belief or None
HRRRouter-->>RetrieveV2: RetrievalResult(beliefs, locked_ids)
RetrieveV2-->>Client: RetrievalResult
else no hits
HRRRouter-->>RetrieveV2: None
RetrieveV2->>TextualLane: run textual retrieval stack
TextualLane-->>RetrieveV2: RetrievalResult
RetrieveV2-->>Client: RetrievalResult
end
else not a marker
HRRRouter-->>RetrieveV2: None
RetrieveV2->>TextualLane: run textual retrieval stack
TextualLane-->>RetrieveV2: RetrievalResult
RetrieveV2-->>Client: RetrievalResult
end
else HRR structural disabled
RetrieveV2->>TextualLane: run textual retrieval stack
TextualLane-->>RetrieveV2: RetrievalResult
RetrieveV2-->>Client: RetrievalResult
end
Class diagram for HRRStructIndexCache and related retrieval typesclassDiagram
class MemoryStore {
+add_invalidation_callback(callback)
+list_locked_beliefs() list~Belief~
+get_belief(belief_id: str) Belief
}
class HRRStructIndex {
+dim: int
+build(store: MemoryStore, store_path: str, seed: int) void
+probe(kind: str, target_id: str, top_k: int) list~tuple~
}
class HRRStructIndexCache {
+store: MemoryStore
+dim: int
+store_path: str
+seed: int
-_index: HRRStructIndex
-_subscribed: bool
+__post_init__() void
+get() HRRStructIndex
+invalidate() void
}
class Belief {
+id: str
}
class RetrievalResult {
+beliefs: list~Belief~
+locked_ids: list~str~
}
MemoryStore "1" --> "*" Belief : stores
MemoryStore "1" --> "*" HRRStructIndexCache : used_by
HRRStructIndexCache "1" o-- "0..1" HRRStructIndex : caches
HRRStructIndex "1" --> "1" MemoryStore : built_from
RetrievalResult "1" --> "*" Belief : beliefs
RetrievalResult "1" --> "*" Belief : locked_via_ids
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
author-Kulili
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds HRR structural-marker routing to ChangesHRR Structural-Query Retrieval Lane
🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
[claim:review:Toug:2026-05-08T20:37:45Z] |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_route_structural_query, the non-cached path constructsHRRStructIndex()with only default parameters, whileHRRStructIndexCache.get()wires throughdim/store_path/seed; if callers rely on non-default dimensions or reproducible seeding you may want to expose equivalent configuration for the non-cached path to avoid subtle behavioral drift between the two. - Several tests (e.g.
test_explicit_cache_reuses_index_across_calls) assert againstcache._indexdirectly; consider exposing a small public introspection method/property onHRRStructIndexCacheso tests don’t need to reach into a private attribute.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_route_structural_query`, the non-cached path constructs `HRRStructIndex()` with only default parameters, while `HRRStructIndexCache.get()` wires through `dim/store_path/seed`; if callers rely on non-default dimensions or reproducible seeding you may want to expose equivalent configuration for the non-cached path to avoid subtle behavioral drift between the two.
- Several tests (e.g. `test_explicit_cache_reuses_index_across_calls`) assert against `cache._index` directly; consider exposing a small public introspection method/property on `HRRStructIndexCache` so tests don’t need to reach into a private attribute.
## Individual Comments
### Comment 1
<location path="tests/test_hrr_struct_index.py" line_range="281-286" />
<code_context>
+ assert "b6" in second.belief_ids
+
+
+def test_cache_explicit_invalidate_drops_index() -> None:
+ s = _toy_store()
+ cache = HRRStructIndexCache(store=s, dim=256, seed=7)
+ cache.get()
+ cache.invalidate()
+ assert cache._index is None
+
+
</code_context>
<issue_to_address>
**nitpick (testing):** Avoid asserting on the cache's private `_index` attribute or add a small public helper for this
This test relies on `cache._index`, which is a private implementation detail and makes `HRRStructIndexCache` harder to refactor safely. Instead, consider asserting via the public API—for example, compare the object returned by `cache.get()` before and after `invalidate()`. If you need to assert the empty state directly, you could add a small read-only helper (e.g. `is_built` or `peek_index`) and use that in the test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+281
to
+286
| def test_cache_explicit_invalidate_drops_index() -> None: | ||
| s = _toy_store() | ||
| cache = HRRStructIndexCache(store=s, dim=256, seed=7) | ||
| cache.get() | ||
| cache.invalidate() | ||
| assert cache._index is None |
There was a problem hiding this comment.
nitpick (testing): Avoid asserting on the cache's private _index attribute or add a small public helper for this
This test relies on cache._index, which is a private implementation detail and makes HRRStructIndexCache harder to refactor safely. Instead, consider asserting via the public API—for example, compare the object returned by cache.get() before and after invalidate(). If you need to assert the empty state directly, you could add a small read-only helper (e.g. is_built or peek_index) and use that in the test.
Owner
Author
|
Reviewed (Toug, non-author per session-mutex labels — 4 atomic signed commits (672c719, b32e918, 7f0facc, 6a1f04e — all G). Code:
Tests:
Docs (6a1f04e): CI: required gates SUCCESS at Discretion: clean. FF: confirmed ( Cross-PR note: PR #502 (mine, post-#494 doc_linker leaf-module cleanup) is open. No file overlap — #503 only touches Approving. Proceeding with local FF push merge. |
Owner
Author
|
[release:review:Toug:2026-05-08T20:39:41Z] |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the loop on #152: the substrate (
HRRStructIndex,parse_structural_marker,is_hrr_structural_enabled) was shipped at v1.7.0 but the integration intoretrieve_v2was deferred and never followed up.use_hrr_structuralwas a phantom flag — setting it ON changed no behavior becauseHRRIndexwas never imported intoretrieval.py. Audit onmainHEADf547779:This PR connects them.
Commits
feat(hrr_index): HRRStructIndexCache for warm callers— mirrorsVocabBridgeCache/BM25IndexCache. Lazy build, store-invalidation subscription, drop-on-mutate. Long-running consumers (interactive shells, future bench harness,retrieve_v2with explicit cache) amortise the per-belief HRR encode cost across queries instead of rebuilding per call. 3 unit tests.feat(retrieval): wire HRR structural lane into retrieve_v2—retrieve_v2growsuse_hrr_structuralandhrr_struct_index_cachekwargs and a routing branch that fires before the vocab-bridge rewrite (a<KIND>:<target_id>marker must not be munged into bag-of-words expansions). On marker hit + flag ON the HRR lane returns directly with locks pinned and HRR-ranked beliefs packed against the budget; on miss the call falls through to the textual lane unchanged. Per spec the lanes are parallel, never blended.test(retrieval): integration tests for HRR structural-query routing— 6 end-to-end tests throughretrieve_v2: (IT1) marker + flag ON returns HRR-ranked beliefs; (IT2) marker + flag OFF falls through to textual; (IT3) non-marker query + flag ON byte-identical to flag OFF; (IT4) marker with unknown target falls through gracefully; (IT5) explicit cache reuses the index across calls; (IT6) locks pin to head when structural lane fires.docs(retrieval): un-phantom use_hrr_structural with syntax cheatsheet—CONFIG.mdreflects the actual routing behavior + a 5-row example table covering hit / miss-by-text / miss-by-empty-target / miss-by-unknown-target / case-sensitivity.Acceptance status
hrr_index.py, full unit testsis_hrr_structural_enabled)retrieve_v2use_hrr_structural/hrr_struct_index_cachekwargs + routing branchVerification
tests/test_hrr_struct_index.py— 21 passed, 2 skipped (perf-gated).tests/test_retrieve_v2_hrr_structural.py— 6 passed.git diff origin/main...HEAD→ empty.Default-flip plan
Stays default-OFF behind
use_hrr_structural. The #154 composition tracker (currently 7/11 per #474 v2.1 umbrella) gates the flip; lab-side bench evidence required.Out of scope
aelf:search "<KIND>:<id>"— current contract is library-API-only viaretrieve_v2(use_hrr_structural=True). CLI exposure can land separately if/when the operator wants the marker syntax in the user-visible search verb.tests/test_hrr_struct_index.pyalready has perf-gated tests for the index itself; the end-to-endretrieve_v2latency is dominated by the HRR matvec which has its own AC7 budget.Summary by Sourcery
Wire the HRR structural-query lane into retrieve_v2 and add a cache for the HRR structural index, with tests and documentation updates.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
retrieve_v2for improved retrieval precision.use_hrr_structuralparameter to enable/disable structural queries.Documentation
use_hrr_structuralconfiguration documentation with behavioral specifications and usage guidance.Tests