Repository navigation
Fix #104: scope _vec_qualifiers kNN to the caller's candidate ids - #120
Merged
Merged
Conversation
_vec_qualifiers issued an unfiltered global k=50 kNN against reflection_embeddings / semantic_embeddings. Both vec tables are global across projects, and retire only UPDATEs status without deleting the vector, so a caller's own candidates could be crowded out of the top-50 slice by other projects' or retired reflections -- and the returned rank was against the global neighbourhood, not the caller's competitors. Now the caller passes its already-computed candidate id list into _vec_qualifiers. We scale k with max(len(candidates), 50) and filter the kNN in Python by the candidate set, matching the sibling pattern established in ReflectionService._vec_ranks (reflection.py). vec_r / vec_s ranks are now dense over the caller's actual candidates, which means their RRF vec contribution reflects position among competitors rather than global neighbours. Empty candidate lists short-circuit without touching the DB. Existing 536 service tests and 84 hook tests pass unchanged. Added test_orphan_vector_row_does_not_leak_via_vec_gate as a regression that locks in the candidate scoping via a direct _vec_qualifiers call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…bal scope
SqliteBackend.relevance_ranks (sqlite.py:353, :359) intentionally wants
the unfiltered rank map -- it's the "server-side gate" analogue used by
retrieve_relevant's agentcore-mode branch (relevant.py:230-234 notes
this callsite is never used from the sqlite branch, but it stays for
protocol completeness). Making candidate_ids required broke pyright on
those two calls.
candidate_ids is now defaulted to None ("no scope filter, keep the
existing global k=50 behaviour"). retrieve_relevant continues to pass
its actual candidate list so its own vec scoping is enforced; [] still
short-circuits explicitly. Regression test still passes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Closes #104
Summary
_vec_qualifiersissued an unfiltered globalk=50kNN againstreflection_embeddings/semantic_embeddings. Both vec tables are global across projects, andretireonly UPDATEs status without deleting the vector, so a caller's own candidates could be crowded out of the top-50 slice by other projects' or retired reflections. Even when they weren't fully crowded out, the rank returned invec_r/vec_swas against the global neighbourhood, not the caller's competitors — which then fed a distorted RRF vec contribution.This PR mirrors the pattern already used by
ReflectionService._vec_ranks(reflection.py) — the sibling the issue explicitly references:_vec_qualifiersgains acandidate_ids: list[str]parameter. Empty list short-circuits without touching the DB.k = max(len(candidate_ids), 50)so the window scales with how much the caller actually has to rank.set(candidate_ids)in Python (sqlite-vec kNN accepts onlyMATCH ? AND k = ?, no extra predicates), so vec ranks are dense over the caller's own set.retrieve_relevantgathersrefl_candidate_idsandsem_candidate_idsfrombuckets/semanticbefore the vec query, and reuses the samerefl_bucket_orderlist for the candidate build loop below.Behaviorally: injection selection benefits because the vec-rank a candidate contributes to RRF now reflects position among competitors instead of position among global neighbours. Orphan or cross-project vector rows no longer appear in
vec_r/vec_sat all.Tests
tests/services/(536 tests) +tests/hooks/(84 tests) pass unchanged.test_orphan_vector_row_does_not_leak_via_vec_gateintests/services/test_relevant.py— a direct_vec_qualifiersregression asserting (a) emptycandidate_idsshort-circuits to{}and (b) an orphan vec row is filtered out when the caller's candidate list doesn't include it.Confidence
90% — the fix is exactly what the issue proposes ("Restrict the kNN with the candidate id set … and scale
kwith candidate count, matching_fuse_by_relevance"), matches an established sibling pattern that already has test coverage, and is a strict improvement: it doesn't materially reduce recall in the normal case (small candidate sets still see k=50 kNN) and it tightens the rank signal in every case.Deliberately out of scope: the more principled fix of a vec0 partition/aux column filter on project + status, which would need a migration and touches the schema. That would fully solve the pathological 60-neighbours-at-cos-0.99 reproduction cited in the issue; this PR does not, and I noted it as a follow-up direction.
This PR was generated by a scheduled Claude routine.
Generated by Claude Code