chore(retrieval,history,recall): review fixes and test coverage [skip-tag]#41
Merged
Merged
Conversation
…-tag]
Apply fixes surfaced during a focused review of `sdk/retrieval`, plus the
parallel `sdk/history` and `sdk/recall` follow-ups that share the same
explanation / capability surface. No release-relevant API changes — kept
under [skip-tag] so the per-module patch bumper does not roll a tag for
this maintenance batch.
retrieval (high-priority correctness)
- HybridShortCircuit now forwards SearchRequest.Debug into HybridRequest
and merges the backend's SearchExecution into the pipeline's response
so callers observe one consistent explanation regardless of the
short-circuit path.
- Pipeline.Run emits SearchExecution.Lanes in a deterministic order
(well-known LaneKeys first, then lexical) to keep dashboards / golden
tests stable across runs.
- pickFinalish now shallow-copies when lifting Reranked/Fused into Final
so downstream in-place mutators (BM25Boost, TimeDecay, EntityBoost,
...) cannot retroactively corrupt the snapshots the caller can still
inspect.
- BM25Boost initialises min/max with +/-Inf for clearer intent and
removes the i==0 special case.
- MemoryIndex.Search drops MinScore on the hybrid (RRF) branch — RRF
scores live on a different scale than raw BM25/cosine — and
SearchRequest.MinScore documents the single-modality contract.
- liftRecall sheds dead ShortCircuit guard; LTM factory expresses the
"BM25 is a boost, not a recall expander" rule via an addBM25Boost
local for readability.
retrieval (small but visible)
- normalizeJSONish coerces every numeric kind to float64 so
Filter.Eq{"score": 5} matches metadata decoded as float64(5).
- PartialError reports only failing rows and emits a stable empty form.
- journal.Wrap exposes the inner backend's optional sub-interfaces
(DocGetter / Filterable / DeletableByFilter / Droppable / Iterable /
Snapshottable / Hybridable / Vectorizable) and fans bulk
DeleteByFilter / Drop into per-doc OpDelete events so the audit log
stays complete.
- EmbedQuery cache gains MaxEntries (default 1024) with TTL-aware
eviction; pass -1 to disable caching outright.
- Entity recall lane scores by overlap count instead of constant 1.0,
giving WeightedFusion / RRF a real signal.
- pipeline/stages_rerank.go switches to math.Sqrt; rerank stages drop
the hand-rolled Newton iteration.
- types.go documents every Filter field and the reserved "_content" key.
- sdkx/retrieval/{postgres,sqlite}: List doc notes the in-memory scan
cost and points bulk export at Iterate; the new offset >= len(all)
guard short-circuits stale page tokens.
history / recall
- history.{compactor,store,tools,archive,doc}: same review pass —
refines compactor coordination, adds dedicated coordinator /
token / tools tests, and folds deprecated knobs into a focused
deprecated.go surface.
- recall.{recall,save,sweeper,merger,memory,telemetry}: introduces
namespace_registry as the single authority for ns -> backend lookup,
hardens save / sweep semantics, and surfaces telemetry counters.
- Adds regression tests for explain output, save context, sweep, and
merger so the contract surface is locked in.
tests
- New: retrieval/{errors,filter_match,explain}_test.go,
pipeline/{stages_boost,stages_query,pipeline}_test.go additions for
lane-order stability, hybrid Debug forwarding, and pickFinalish
isolation; journal/wrap_test.go covers sub-interface delegation and
bulk-delete journaling.
- New: history/{coordinator,store,summary_store,token,tools}_test.go;
recall/{merger,namespace_registry,recall_explain,save_context,
sweeper,test_helpers_regression}_test.go.
Verified: go vet ./sdk/... ./sdkx/... and go test ./sdk/... ./sdkx/...
both pass on the work tree.
Made-with: Cursor
…our [skip-tag] The test's clock closure captured a local `now` variable that the recall worker goroutine read while the test goroutine wrote it between Save calls, which CI's -race build flagged. Wrap the value in atomic.Pointer[time.Time] so reads from the worker and writes from the test are synchronised. Made-with: Cursor
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
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.
Summary
Apply review fixes across
sdk/retrieval,sdk/history, andsdk/recall, plus the supporting tests. This is a maintenance batch with no release-relevant API changes —[skip-tag]is set in the title soauto-tag.yml's patch bumper does not roll a tag for this merge.retrieval — high priority correctness
Debug. PreviouslySearchRequest.Debugwas dropped on the native-hybrid path; backends that honourCapabilities.Debugnow receive it and the pipeline merges theirSearchExecutioninto the response so callers observe one consistent explanation regardless of which path ran.Pipeline.RunemitsSearchExecution.Lanesby well-knownLaneKeyfirst, then lexical, so dashboards / golden tests don't flake from Go map iteration order.pickFinalishno longer aliasesReranked/Fused. Lifting now shallow-copies, so in-place rescoring stages (BM25Boost,TimeDecay,EntityBoost, ...) cannot retroactively corrupt earlier snapshots.BM25Boostuses ±Inf seeds for clearer min/max semantics.MemoryIndex.SearchdropsMinScoreon the hybrid (RRF) branch. RRF scores live on a different scale than raw BM25/cosine;SearchRequest.MinScorenow documents the single-modality contract. Hybrid filtering should usepipeline.ScoreThreshold.liftRecallshort-circuit guard; LTM factory expresses the "BM25 is a boost, not a recall expander" rule via a cleareraddBM25Boostlocal.retrieval — small but visible
normalizeJSONishcoerces every numeric kind tofloat64—Filter.Eq{"score": 5}now matches metadata decoded asfloat64(5).PartialErrorreports only failing rows; empty form is now stable.journal.Wrapprojects the inner backend's optional sub-interfaces (DocGetter/Filterable/DeletableByFilter/Droppable/Iterable/Snapshottable/Hybridable/Vectorizable) and fans bulkDeleteByFilter/Dropinto per-docOpDeleteevents so the audit log stays complete.EmbedQuerycache gainsMaxEntries(default 1024) with TTL-aware eviction;-1disables caching.1.0), givingWeightedFusion/ RRF a real signal.stages_rerank.goswitches tomath.Sqrt; the hand-rolled Newton iteration is gone.types.godocuments everyFilterfield and the reserved_contentkey.sdkx/retrieval/{postgres,sqlite}: List doc notes the in-memory scan cost and points bulk export atIterate; the newoffset >= len(all)guard short-circuits stale page tokens.history / recall
history.{compactor,store,tools,archive,doc}— same review pass: refines compactor coordination, adds dedicated coordinator / token / tools tests, folds deprecated knobs into a focuseddeprecated.gosurface.recall.{recall,save,sweeper,merger,memory,telemetry}— introducesnamespace_registryas the single authority for ns → backend lookup, hardens save / sweep semantics, surfaces telemetry counters.Tests added
errors_test.go,filter_match_test.go,explain_test.go, pluspipeline/{stages_boost,stages_query,pipeline}_test.goadditions for lane-order stability, hybridDebugforwarding, andpickFinalishisolation;journal/wrap_test.gocovers sub-interface delegation and bulk-delete journaling.coordinator_test.go,store_test.go,summary_store_test.go,token_test.go,tools_test.go.merger_test.go,namespace_registry_test.go,recall_explain_test.go,save_context_test.go,sweeper_test.go,test_helpers_regression_test.go.Test plan
go vet ./sdk/... ./sdkx/...go test ./sdk/... ./sdkx/...Made with Cursor