feat(embeddings): hybrid fusion strategies (RRF, weighted, normalized)#41
Merged
plind-junior merged 18 commits intoMay 21, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements embedding-first semantic search across the knowledge base by adding SQLite-backed vector persistence, query caching, result-list fusion, ingest-time embedding hooks, and multi-backend search routing with optional hybrid mode combining semantic and keyword search. ChangesSemantic Search Implementation
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Store as KBStore
participant IndexDB as index_db
participant Cache as cache
participant Embedder as Embedder
App->>Store: put_claim(text)
Store->>Store: _embed_and_store(kind, id, text)
Store->>IndexDB: search_semantic requires embeddings
Note over Store: Compute content_hash, resolve embedder
Store->>Embedder: encode(text)
Embedder-->>Store: embedding vector
Store->>Cache: lookup_query_vec(text)
Cache-->>Store: None (cache miss)
Store->>IndexDB: put_embedding(vector, hash, model)
IndexDB->>IndexDB: Store in embedding_index
IndexDB->>IndexDB: Update embedding metadata
sequenceDiagram
participant Client as Client
participant Server as server.kb_search
participant IndexDB as index_db
participant Fusion as fusion
Client->>Server: kb_search(query, backend="hybrid")
Server->>IndexDB: search_semantic(query)
IndexDB->>IndexDB: Cache or encode query
IndexDB-->>Server: semantic results
Server->>IndexDB: search(query) [FTS5]
IndexDB-->>Server: fts5 results
Server->>Fusion: rrf_fuse(semantic, fts5)
Fusion->>Fusion: Accumulate reciprocal ranks
Fusion->>Fusion: Deduplicate by key
Fusion-->>Server: fused results
Server-->>Client: {backend: "hybrid", hits: [...]}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vouch/index_db.py (1)
89-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reset()should also clear embedding tables.
reset()claims to drop everything, but it currently leavesembedding_index,query_embedding_cache,embedding_dupes, and embedding meta intact. That can return stale semantic hits after a rebuild.Proposed fix
def reset(kb_dir: Path) -> None: """Drop everything; the rebuild caller re-populates.""" with open_db(kb_dir) as conn: conn.executescript( - "DELETE FROM claims_fts; DELETE FROM pages_fts; DELETE FROM entities_fts;" + "DELETE FROM claims_fts; " + "DELETE FROM pages_fts; " + "DELETE FROM entities_fts; " + "DELETE FROM embedding_index; " + "DELETE FROM query_embedding_cache; " + "DELETE FROM embedding_dupes; " + "DELETE FROM index_meta WHERE key LIKE 'embedding_%';" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/index_db.py` around lines 89 - 94, The reset function (reset) currently deletes only FT tables and must also clear all embedding-related tables to avoid stale semantic hits; update the block that opens the DB via open_db and the conn.executescript call to also DELETE FROM embedding_index, query_embedding_cache, embedding_dupes and the embedding metadata table (e.g., embedding_meta or whatever the repo uses) in the same script so the rebuild starts from a clean embedding state; keep the changes inside the same transaction via conn.executescript and ensure the SQL uses DELETE FROM <table>;.
🧹 Nitpick comments (5)
src/vouch/server.py (1)
77-134: 🏗️ Heavy liftConsider extracting shared search-routing logic used by MCP and JSONL servers.
The backend-selection/fusion code is duplicated and already diverged across transports. Moving this into one internal helper would prevent future contract drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/server.py` around lines 77 - 134, Extract the backend-selection and fusion logic from kb_search into a shared internal helper (e.g., _route_kb_search or kb_search_impl) that takes (query, limit, backend, min_score, store, index_db) and returns the same dict shape used by kb_search (_to_dicts-like output); the helper should call index_db.search_semantic, index_db.search, store.search_substring, and embeddings.fusion.rrf_fuse as currently done, handle exceptions and backend-specific empty-return behavior, and raise on unknown backend—then have kb_search call that helper (so the JSONL server can reuse it) to remove the duplicated routing code.tests/embeddings/test_search.py (1)
98-137: ⚡ Quick winAdd direct integration coverage for
backend="hybrid"andmin_scorebehavior.Current tests validate default routing, but they don’t assert the hybrid execution path or threshold propagation, which are central to this change set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_search.py` around lines 98 - 137, Add tests that explicitly exercise the hybrid path and min_score propagation: create a new test (or extend existing tests) that uses jsonl_server.handle_request (and/or server.kb_search) with params including "backend":"hybrid" and a "min_score" threshold, seed two claims (one high-sim, one low-sim) via store.put_claim, call health.rebuild_index(store), then assert the response backend is "hybrid" and that results respect min_score (i.e., low-scoring claim is excluded when min_score is high and included when min_score is low). Use the existing symbols jsonl_server.handle_request, server.kb_search, health.rebuild_index, store.put_claim and index_db.search_semantic to locate where to add the assertions.docs/superpowers/specs/2026-05-20-semantic-search-design.md (1)
33-47: 💤 Low valueAdd language identifier to the fenced code block.
The directory tree structure should specify a language identifier (e.g.,
textor leave it asplaintext) to satisfy markdown linting rules.📝 Proposed fix
-``` +```text src/vouch/embeddings/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md` around lines 33 - 47, The fenced code block showing the embeddings directory tree is missing a language identifier; update the opening fence (the triple backticks before "src/vouch/embeddings/") to include a language label such as "text" or "plaintext" (e.g., ```text) so the markdown linter accepts the block while keeping the directory listing unchanged.tests/embeddings/test_integration.py (2)
37-43: 💤 Low valueConsider adding norm check for consistency.
The
test_st_mpnet_loads_and_encodestest verifies that embeddings are normalized to unit length (line 21), but this test doesn't include the same check. SinceSTMinilmEmbeddernormalizes embeddings in its implementation, adding a norm assertion would improve test consistency.✅ Optional: Add norm verification
def test_st_minilm_loads_and_encodes() -> None: from vouch.embeddings.st_minilm import STMinilmEmbedder e = STMinilmEmbedder() vec = e.encode("hello world") assert vec.shape == (384,) assert vec.dtype == np.float32 + assert abs(float(np.linalg.norm(vec)) - 1.0) < 1e-3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_integration.py` around lines 37 - 43, The test test_st_minilm_loads_and_encodes currently checks shape and dtype but omits verifying that embeddings are unit-normalized; update the test to compute the vector norm from STMinilmEmbedder().encode (call e.encode("hello world")) and assert the norm is approximately 1.0 (use np.linalg.norm and np.isclose or an equivalent assertion) to match the normalization check in test_st_mpnet_loads_and_encodes.
46-53: 💤 Low valueConsider adding norm check for consistency.
Similar to the other integration tests, adding a norm assertion would verify that
FastembedBgeEmbedderproduces unit-length embeddings as expected.✅ Optional: Add norm verification
def test_fastembed_bge_loads_and_encodes() -> None: pytest.importorskip("fastembed") from vouch.embeddings.fastembed_bge import FastembedBgeEmbedder e = FastembedBgeEmbedder() vec = e.encode("hello world") assert vec.shape == (384,) assert vec.dtype == np.float32 + assert abs(float(np.linalg.norm(vec)) - 1.0) < 1e-3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_integration.py` around lines 46 - 53, Update the integration test test_fastembed_bge_loads_and_encodes to also verify embedding normalization: after creating FastembedBgeEmbedder and calling e.encode("hello world") (the vec variable), compute the L2 norm of vec and assert it is close to 1.0 (use an appropriate tolerance) to ensure FastembedBgeEmbedder produces unit-length embeddings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/embeddings/cache.py`:
- Line 10: The module-level unconditional "import numpy as np" in
src/vouch/embeddings/cache.py causes mypy to fail when numpy isn't installed;
move the numpy import into the local scope where it's actually used (e.g.,
inside any functions/methods that call numpy in this module) or wrap it with a
TYPE_CHECKING guard so only type-checking imports occur (use
typing.TYPE_CHECKING and import numpy under that guard), and update usages
accordingly (refer to the module-level import and any functions in cache.py that
call numpy to locate where to move/guard the import).
In `@src/vouch/embeddings/fastembed_bge.py`:
- Line 11: CI mypy fails because src/vouch/embeddings/fastembed_bge.py imports
numpy unconditionally but the CI only installs [dev] deps; either have the CI
install the optional [embeddings-fast] extras before running mypy (update the
workflow step that installs dependencies to include extras) or add mypy config
to ignore missing imports (e.g., add an appropriate [mypy] section in
pyproject.toml with ignore_missing_imports = True or per-module settings for the
fastembed_bge module) so imports like the numpy import in fastembed_bge.py do
not cause type-check failures.
In `@src/vouch/index_db.py`:
- Around line 340-344: lookup_query_vec currently returns cached vectors keyed
only by query text which can be stale if the embedder model or embedding
dimensionality changes; update the logic around lookup_query_vec,
cache_query_vec and the embedder usage so that you validate the cached vector
against the current embedder (e.g., check an associated model identifier and
length/dimension) and treat it as invalid if it doesn't match. Specifically,
when calling lookup_query_vec(kb_dir, query=query) verify the returned vec has
the expected dimension (and/or stored model id) matching embedder.encode; if it
fails the check, call embedder.encode(query) to produce a fresh vector and then
cache_query_vec(kb_dir, query=query, vec=qvec, metadata={model_id, dim}) before
passing qvec into search_embedding.
In `@src/vouch/jsonl_server.py`:
- Around line 79-83: The code reads backend_arg and currently falls through to
returning an empty hits list for invalid values; update the validation around
backend_arg (the code that assigns backend_arg and sets used) to explicitly
check against the allowed backend options (e.g., include "auto" and the known
backend names used elsewhere) and reject unknown values instead of silently
returning hits: when backend_arg is not in the allowed set, return a clear error
(HTTP 400 / raise ValueError) indicating the invalid backend instead of
proceeding, ensuring consumers see consistent MCP-like behavior; adjust the code
that currently initializes hits and used to only proceed after this validation.
- Around line 99-103: In the hybrid branch (where backend_arg == "hybrid") apply
the same min_score filtering and FTS error handling as other backends: call
index_db.search_semantic(...) into emb and index_db.search(...) into fts inside
a try/except so any exception from index_db.search is caught and handled (e.g.,
log the error and set fts = [] to avoid failing the request), then filter both
emb and fts by the min_score parameter before passing them to rrf_fuse(emb, fts,
limit=limit). Ensure you reference the existing symbols:
index_db.search_semantic, index_db.search, rrf_fuse, emb, fts, hits, min_score,
q, and limit when making these changes.
In `@src/vouch/server.py`:
- Around line 126-131: In the hybrid branch (the block handling backend ==
"hybrid"), make the FTS call resilient and apply the semantic min_score: wrap
the index_db.search(store.kb_dir, query, limit=limit * 2) call in a try/except
and fall back to an empty list if it raises, then call
index_db.search_semantic(...) as before but filter its results using the
provided min_score threshold before passing them into rrf_fuse; after rrf_fuse,
also filter the fused hits by score >= min_score (or the same score key your
semantic results use) and then return _to_dicts(filtered_hits, "hybrid"). Ensure
you reference rrf_fuse, index_db.search_semantic, index_db.search, and _to_dicts
when making the changes.
In `@src/vouch/storage.py`:
- Around line 483-505: The embedding/indexing hook can still raise after the
caller persisted the file; wrap the whole block that calls
_index_db.get_embedding, get_embedder(), embedder.encode(...), the with
_index_db.open_db(...) / _index_db.put_embedding(...),
_index_db.set_embedding_meta(...), and the dedup check_and_log(...) in a single
try/except Exception as e so any DB/encode/dedup errors are caught and logged
rather than propagated; log the exception with context (e.g., which kb_dir,
kind, id and that embedding/indexing failed) and then return silently so the
write path never aborts.
- Around line 502-503: The mypy failure is caused by importing an untyped local
module (.embeddings.dedup) in storage.py; to fix quickly, suppress the mypy
import error by appending a type ignore to the local import line (from
.embeddings.dedup import check_and_log # type: ignore[import-untyped]) so CI
passes, or alternatively make the dedup module a typed package (add py.typed and
type annotations) if you prefer a permanent fix; target the import of
check_and_log in storage.py for the change.
---
Outside diff comments:
In `@src/vouch/index_db.py`:
- Around line 89-94: The reset function (reset) currently deletes only FT tables
and must also clear all embedding-related tables to avoid stale semantic hits;
update the block that opens the DB via open_db and the conn.executescript call
to also DELETE FROM embedding_index, query_embedding_cache, embedding_dupes and
the embedding metadata table (e.g., embedding_meta or whatever the repo uses) in
the same script so the rebuild starts from a clean embedding state; keep the
changes inside the same transaction via conn.executescript and ensure the SQL
uses DELETE FROM <table>;.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 33-47: The fenced code block showing the embeddings directory tree
is missing a language identifier; update the opening fence (the triple backticks
before "src/vouch/embeddings/") to include a language label such as "text" or
"plaintext" (e.g., ```text) so the markdown linter accepts the block while
keeping the directory listing unchanged.
In `@src/vouch/server.py`:
- Around line 77-134: Extract the backend-selection and fusion logic from
kb_search into a shared internal helper (e.g., _route_kb_search or
kb_search_impl) that takes (query, limit, backend, min_score, store, index_db)
and returns the same dict shape used by kb_search (_to_dicts-like output); the
helper should call index_db.search_semantic, index_db.search,
store.search_substring, and embeddings.fusion.rrf_fuse as currently done, handle
exceptions and backend-specific empty-return behavior, and raise on unknown
backend—then have kb_search call that helper (so the JSONL server can reuse it)
to remove the duplicated routing code.
In `@tests/embeddings/test_integration.py`:
- Around line 37-43: The test test_st_minilm_loads_and_encodes currently checks
shape and dtype but omits verifying that embeddings are unit-normalized; update
the test to compute the vector norm from STMinilmEmbedder().encode (call
e.encode("hello world")) and assert the norm is approximately 1.0 (use
np.linalg.norm and np.isclose or an equivalent assertion) to match the
normalization check in test_st_mpnet_loads_and_encodes.
- Around line 46-53: Update the integration test
test_fastembed_bge_loads_and_encodes to also verify embedding normalization:
after creating FastembedBgeEmbedder and calling e.encode("hello world") (the vec
variable), compute the L2 norm of vec and assert it is close to 1.0 (use an
appropriate tolerance) to ensure FastembedBgeEmbedder produces unit-length
embeddings.
In `@tests/embeddings/test_search.py`:
- Around line 98-137: Add tests that explicitly exercise the hybrid path and
min_score propagation: create a new test (or extend existing tests) that uses
jsonl_server.handle_request (and/or server.kb_search) with params including
"backend":"hybrid" and a "min_score" threshold, seed two claims (one high-sim,
one low-sim) via store.put_claim, call health.rebuild_index(store), then assert
the response backend is "hybrid" and that results respect min_score (i.e.,
low-scoring claim is excluded when min_score is high and included when min_score
is low). Use the existing symbols jsonl_server.handle_request, server.kb_search,
health.rebuild_index, store.put_claim and index_db.search_semantic to locate
where to add the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae6a6c73-d807-4fbe-a4c7-575cf396576b
📒 Files selected for processing (21)
docs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/test_core.pytests/embeddings/test_fusion.pytests/embeddings/test_integration.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
ec626ee to
e0b1e96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/embeddings/test_search.py`:
- Line 9: The test module imports MockEmbedder (from tests.embeddings._fakes
import MockEmbedder) which triggers a ModuleNotFoundError at collection when
numpy is not installed; guard the import by calling pytest.importorskip("numpy")
at the top of tests/embeddings/test_search.py before importing MockEmbedder (or
replace the direct import with pytest.importorskip("numpy") followed by the
import) so the entire test module is skipped if numpy is unavailable.
In `@tests/embeddings/test_storage.py`:
- Around line 7-8: Replace the unconditional NumPy import in
tests/embeddings/test_storage.py with a pytest.importorskip call so tests are
skipped at collection when NumPy isn't installed; locate the top-level "import
numpy as np" and change it to call pytest.importorskip("numpy") (and assign to a
local name like np if needed) so the module uses pytest.importorskip to detect
and skip the tests gracefully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e504035-8e0c-4a5a-b596-aaae31bd36c1
📒 Files selected for processing (11)
pyproject.tomlsrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fusion.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/test_fusion.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
|
|
||
| import pytest | ||
|
|
||
| from tests.embeddings._fakes import MockEmbedder |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check test module import order:"
sed -n '1,30p' tests/embeddings/test_search.py
echo "Confirm numpy dependency in fake embedder:"
sed -n '1,80p' tests/embeddings/_fakes.py
echo "Inspect dependency declarations for numpy:"
fd -i 'pyproject.toml' -x rg -n 'numpy|optional-dependencies|dependencies|pytest' {}Repository: vouchdev/vouch
Length of output: 2439
Guard test module import when numpy is unavailable.
MockEmbedder imports numpy at the module level in _fakes.py. When this test imports MockEmbedder at line 9, pytest collection fails with ModuleNotFoundError if numpy is not installed, preventing the entire test suite from loading. Gate the import with pytest.importorskip() to skip the test module instead of crashing collection.
💡 Suggested patch
import pytest
-from tests.embeddings._fakes import MockEmbedder
+pytest.importorskip("numpy")
+from tests.embeddings._fakes import MockEmbedder
from vouch import index_db📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from tests.embeddings._fakes import MockEmbedder | |
| import pytest | |
| pytest.importorskip("numpy") | |
| from tests.embeddings._fakes import MockEmbedder | |
| from vouch import index_db |
🧰 Tools
🪛 GitHub Actions: ci / 1_test (py3.11).txt
[error] 9-9: pytest collection failed due to import error while importing MockEmbedder from tests.embeddings._fakes: ModuleNotFoundError: No module named 'numpy'.
🪛 GitHub Actions: ci / 2_test (py3.13).txt
[error] 9-9: Pytest collection failed due to missing dependency in imported test utilities: ModuleNotFoundError: No module named 'numpy'
🪛 GitHub Actions: ci / 3_test (py3.12).txt
[error] 9-9: pytest collection failed due to missing dependency: ModuleNotFoundError: No module named 'numpy' (indirectly from tests/embeddings/_fakes.py).
🪛 GitHub Actions: ci / test (py3.11)
[error] 9-9: pytest collection failed because referenced fakes import fails with ImportError: No module named 'numpy'.
🪛 GitHub Actions: ci / test (py3.12)
[error] 9-9: pytest import error during test collection due to missing dependency: ModuleNotFoundError: No module named 'numpy'.
🪛 GitHub Actions: ci / test (py3.13)
[error] 9-9: pytest collection failed because tests/embeddings/_fakes.py imports numpy, but ModuleNotFoundError: No module named 'numpy'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/embeddings/test_search.py` at line 9, The test module imports
MockEmbedder (from tests.embeddings._fakes import MockEmbedder) which triggers a
ModuleNotFoundError at collection when numpy is not installed; guard the import
by calling pytest.importorskip("numpy") at the top of
tests/embeddings/test_search.py before importing MockEmbedder (or replace the
direct import with pytest.importorskip("numpy") followed by the import) so the
entire test module is skipped if numpy is unavailable.
Comment on lines
+7
to
+8
| import numpy as np | ||
| import pytest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read it
cat -n tests/embeddings/test_storage.pyRepository: vouchdev/vouch
Length of output: 7258
🏁 Script executed:
# Look for CI configuration files
git ls-files | grep -E '\.(yml|yaml|toml|cfg|ini)$' | head -20Repository: vouchdev/vouch
Length of output: 1014
🏁 Script executed:
# Check pyproject.toml or setup.py for optional dependencies
fd -e toml -e py | grep -E '(pyproject|setup)' | head -5Repository: vouchdev/vouch
Length of output: 73
🏁 Script executed:
cat -n pyproject.toml | head -100Repository: vouchdev/vouch
Length of output: 3397
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: vouchdev/vouch
Length of output: 1859
Use pytest.importorskip to skip tests gracefully when NumPy isn't installed.
Line 7 imports NumPy unconditionally, causing test collection to fail when the [embeddings] extra is not installed. The default CI job installs only the [dev] extra (as documented in pyproject.toml lines 79–84), so NumPy is unavailable during collection.
Suggested fix
-import numpy as np
import pytest
+np = pytest.importorskip("numpy")🧰 Tools
🪛 GitHub Actions: ci / 1_test (py3.11).txt
[error] 7-7: pytest collection failed due to import error: ModuleNotFoundError: No module named 'numpy' (import numpy as np).
🪛 GitHub Actions: ci / 2_test (py3.13).txt
[error] 7-7: Pytest collection failed due to missing dependency: ModuleNotFoundError: No module named 'numpy'
🪛 GitHub Actions: ci / 3_test (py3.12).txt
[error] 7-7: pytest collection failed: ModuleNotFoundError: No module named 'numpy' (import numpy as np).
🪛 GitHub Actions: ci / test (py3.11)
[error] 7-7: pytest collection failed due to ImportError: No module named 'numpy' while importing the test module.
🪛 GitHub Actions: ci / test (py3.12)
[error] 7-7: pytest import error during test collection: ModuleNotFoundError: No module named 'numpy'.
🪛 GitHub Actions: ci / test (py3.13)
[error] 7-7: pytest collection failed due to missing dependency: ModuleNotFoundError: No module named 'numpy' (import numpy as np).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/embeddings/test_storage.py` around lines 7 - 8, Replace the
unconditional NumPy import in tests/embeddings/test_storage.py with a
pytest.importorskip call so tests are skipped at collection when NumPy isn't
installed; locate the top-level "import numpy as np" and change it to call
pytest.importorskip("numpy") (and assign to a local name like np if needed) so
the module uses pytest.importorskip to detect and skip the tests gracefully.
4 tasks
Collaborator
|
LGTM with tiny nits. Fix CI and check comments from codrabbits. I think thoes are all valid |
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
e0b1e96 to
56bd11b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/embeddings/test_search.py (1)
9-9:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard the MockEmbedder import to prevent pytest collection failures.
This issue was flagged in a previous review and is blocking CI across all Python versions. The
MockEmbedderimport triggers aModuleNotFoundErrorfor numpy during pytest collection when the optional embedding dependencies are not installed, preventing the entire test suite from loading.🔧 Apply the guard immediately to unblock CI
import pytest +pytest.importorskip("numpy") from tests.embeddings._fakes import MockEmbedder from vouch import index_db🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_search.py` at line 9, Guard the problematic import of MockEmbedder from tests.embeddings._fakes to avoid ModuleNotFoundError during collection: wrap the import in a safe guard (use pytest.importorskip for the optional dependency like "numpy" or a try/except ImportError around the from tests.embeddings._fakes import MockEmbedder and set a fallback/skip behavior) so tests/embeddings/test_search.py does not fail to import when embedding extras are not installed; locate the import statement referencing MockEmbedder in that file and replace it with the guarded import pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/index_db.py`:
- Around line 40-67: The reset() routine currently only removes the three FTS
tables and leaves the new embedding tables intact, causing stale
embedding_index, query_embedding_cache, index_meta and embedding_dupes rows to
persist; update reset() to also clear those derived tables by running the
appropriate DELETE/DELETE FROM or DROP/CREATE statements for embedding_index,
query_embedding_cache, embedding_dupes and index_meta (and any associated
indexes) so reindexing starts from a clean slate, ensuring you reference the
reset() function and the table names embedding_index, query_embedding_cache,
embedding_dupes, and index_meta when making the change.
- Around line 311-314: The loop computing scores uses a raw dot product (score =
float(q @ vec)) which biases by magnitude; change it to compute cosine
similarity by dividing the dot product by the product of vector norms (i.e.,
score = (q · vec) / (||q|| * ||vec||)). Ensure you handle zero-length vectors
safely (skip or treat norm zero as yielding score below min_score). Update the
code where _blob_to_vec is used and the loop that iterates over rows
(referencing variables kind, id_, blob, dim, vec, q, min_score) so the fallback
path matches sqlite-vec cosine rankings.
In `@src/vouch/storage.py`:
- Around line 482-489: The code currently skips re-embedding when
content_hash(text) equals existing[1], which allows embeddings from a different
model to persist; change the check to also verify the active embedder identity
before returning: call get_embedder() (or its identifier like
embedder.name/model_id) and compare it to the embedder id stored in the existing
record returned by _index_db.get_embedding (augment _index_db.get_embedding to
include stored_embedder if it doesn't already). Only return early if both the
content hash and the stored embedder id match the current embedder; otherwise
proceed to re-embed and overwrite the stored embedding and embedder id.
---
Duplicate comments:
In `@tests/embeddings/test_search.py`:
- Line 9: Guard the problematic import of MockEmbedder from
tests.embeddings._fakes to avoid ModuleNotFoundError during collection: wrap the
import in a safe guard (use pytest.importorskip for the optional dependency like
"numpy" or a try/except ImportError around the from tests.embeddings._fakes
import MockEmbedder and set a fallback/skip behavior) so
tests/embeddings/test_search.py does not fail to import when embedding extras
are not installed; locate the import statement referencing MockEmbedder in that
file and replace it with the guarded import pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00e7dbdb-accc-4e6c-ad70-0bda5402746c
📒 Files selected for processing (11)
src/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fusion.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/conftest.pytests/embeddings/test_fusion.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
✅ Files skipped from review due to trivial changes (2)
- tests/embeddings/conftest.py
- src/vouch/embeddings/init.py
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
56bd11b to
6543417
Compare
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
6543417 to
4ed27e6
Compare
…embedding Per CodeRabbit Critical on PR vouchdev#39/40/43: SQLite does not allow a column alias defined in the SELECT list to be referenced in the WHERE clause of the same query. The old form SELECT ... 1.0 - vec_distance_cosine(vec, ?) AS score ... WHERE ... AND score >= ? raised `no such column: score` at runtime; the catch-all `except sqlite3.OperationalError` silently swallowed it and made every call fall through to the NumPy brute-force path, which works but defeats the whole point of installing sqlite-vec. Wrap the projection in a subquery so the WHERE clause sees the alias. No behavior change for callers that have already been using the fallback (NumPy still produces correct results); the ANN code path now actually runs when sqlite-vec is loaded.
The KBStore._embed_and_store helper lazily imports vouch.embeddings.dedup (added in Phase 6). On Phase 3's branch that module does not yet exist, so mypy treats it as untyped and errors out. Mark the import with `# type: ignore[import-not-found,import-untyped, unused-ignore]` so the type check passes here and stays silent once Phase 6 lands.
The hybrid backend branch in kb_search (server.py) and _h_search (jsonl_server.py) lazily imports vouch.embeddings.fusion (added in Phase 5). On Phase 4's branch that module doesn't exist yet, so mypy emits import-untyped. Mark each import with `# type: ignore` and let ruff auto-format the line.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
4ed27e6 to
379297a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
src/vouch/storage.py (2)
482-509:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
_embed_and_store()truly best-effort.The docstring says embedding failures are swallowed, but
get_embedding(),encode(), the DB writes,set_embedding_meta(), andcheck_and_log()can still raise after the artifact file has already been persisted. That breaks the “embeddings are an enhancement” contract and makes create/update calls report failure after a successful durable write. Wrap the remaining embedding/indexing work in a broadtry/except Exception, log the failure withkind/id, and return silently here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/storage.py` around lines 482 - 509, _wrap the remainder of _embed_and_store after obtaining h and ensuring the artifact file exists in a broad try/except Exception block so any error from get_embedder, embedder.encode, _index_db.put_embedding, _index_db.set_embedding_meta, or check_and_log does not propagate; on exception log a concise error including kind and id and then return silently, preserving the already-persisted artifact and keeping embedding/indexing strictly best-effort (refer to symbols: _embed_and_store, get_embedder, embedder.encode, _index_db.put_embedding, _index_db.set_embedding_meta, check_and_log).
482-488:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't skip re-embedding on hash alone.
The early return only checks
content_hash, so switching the active embedder can leave an old vector on disk for the same text. That mixes document embeddings from one model with queries from another and will skew semantic ranking. Compare the stored model identity toget_embedder()before returning early.Suggested fix
- h = content_hash(text) - existing = _index_db.get_embedding(self.kb_dir, kind=kind, id=id) - if existing is not None and existing[1] == h: - return try: embedder = get_embedder() except KeyError: return + h = content_hash(text) + existing = _index_db.get_embedding(self.kb_dir, kind=kind, id=id) + if ( + existing is not None + and existing[1] == h + and existing[2] == embedder.name + ): + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/storage.py` around lines 482 - 488, The early-return after computing h currently only checks content_hash(text) and can skip re-embedding when the active embedder changed; modify the logic in the block that fetches existing = _index_db.get_embedding(self.kb_dir, kind=kind, id=id) so that you also obtain the current embedder identity from get_embedder() (e.g., a model_name or id property) and compare it to the stored model identity in existing (the record returned by get_embedding); only return early if both the stored content hash equals h and the stored model identity matches the current embedder; otherwise fall through to re-embed and update the index.src/vouch/jsonl_server.py (2)
99-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same hybrid safeguards as the other backends.
The hybrid branch ignores
min_scoreon the semantic leg and letsindex_db.search()exceptions abort the request. That makeshybridless reliable thanauto/fts5for the same KB state.Suggested fix
- emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2) - fts = index_db.search(s.kb_dir, q, limit=limit * 2) + emb = index_db.search_semantic( + s.kb_dir, q, limit=limit * 2, min_score=min_score, + ) + try: + fts = index_db.search(s.kb_dir, q, limit=limit * 2) + except Exception: + fts = [] hits = rrf_fuse(emb, fts, limit=limit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/jsonl_server.py` around lines 99 - 105, The hybrid branch (backend_arg == "hybrid") currently calls index_db.search_semantic and index_db.search without the same safeguards as other backends: it must apply the min_score cutoff to the semantic results and guard index_db.search with the same try/except fallback logic. Modify the hybrid block to (1) call index_db.search_semantic(..., limit=limit*2) and filter out semantic hits below min_score before passing to rrf_fuse, and (2) wrap index_db.search(...) in the same try/except used elsewhere so exceptions don’t abort the request (fall back to an empty FTS result on error). Ensure you still pass the filtered semantic list and the safe FTS list into rrf_fuse(..., limit=limit).
79-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown
backendvalues up front.Invalid backend names currently fall through to a successful empty response, which hides client bugs and diverges from
server.kb_search(). Validatebackend_argagainst the supported set and raiseValueErrorfor anything else.Suggested fix
backend_arg = p.get("backend", "auto") + valid_backends = {"auto", "embedding", "fts5", "substring", "hybrid"} + if backend_arg not in valid_backends: + raise ValueError(f"unknown backend: {backend_arg}") min_score = float(p.get("min_score", 0.0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/jsonl_server.py` around lines 79 - 83, Reject unknown backend values by validating backend_arg against the canonical supported set used by server.kb_search(); if backend_arg not in that set (and not the special "auto"), raise a ValueError. Update the code around the backend_arg/used assignment to fetch or reference the same supported-backends collection used by server.kb_search() (or define a small SUPPORTED_BACKENDS set if one doesn’t exist), then perform an if backend_arg not in SUPPORTED_BACKENDS: raise ValueError(f"unknown backend: {backend_arg}") before continuing and setting used.src/vouch/server.py (1)
126-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror
min_scoreand FTS fallback inhybrid.The explicit hybrid branch bypasses the semantic threshold and can still fail on
index_db.search()exceptions, even though the other backend paths already treat FTS errors as empty hits. Apply the same guards here before fusing.Suggested fix
- emb = index_db.search_semantic(store.kb_dir, query, limit=limit * 2) - fts = index_db.search(store.kb_dir, query, limit=limit * 2) + emb = index_db.search_semantic( + store.kb_dir, query, limit=limit * 2, min_score=min_score, + ) + try: + fts = index_db.search(store.kb_dir, query, limit=limit * 2) + except Exception: + fts = [] hits = rrf_fuse(emb, fts, limit=limit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/server.py` around lines 126 - 133, In the hybrid branch where you call index_db.search_semantic and index_db.search and then rrf_fuse, apply the same semantic min_score filtering and FTS fallback used by other backends: filter the semantic results from index_db.search_semantic by the existing min_score variable before fusion, and wrap index_db.search in a try/except so any exception yields an empty FTS hits list instead of bubbling up; then call rrf_fuse(emb_filtered, fts_or_empty, limit=limit) and return _to_dicts(hits, "hybrid"). Ensure you reference the same symbols used here: index_db.search_semantic, index_db.search, rrf_fuse, min_score, and _to_dicts.tests/embeddings/test_search.py (1)
7-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSkip this module cleanly when NumPy isn't installed.
Importing
MockEmbedderat module load time makes pytest collection fail when the optionalnumpydependency is absent. Gate the import withpytest.importorskip("numpy")so CI skips this module instead of hard-failing collection.Suggested fix
import pytest +pytest.importorskip("numpy") from tests.embeddings._fakes import MockEmbedder🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_search.py` around lines 7 - 10, The test module imports MockEmbedder at import time which causes collection to fail when numpy is not present; call pytest.importorskip("numpy") at the top of the module before importing MockEmbedder so pytest will skip the whole module cleanly, then import MockEmbedder from tests.embeddings._fakes (leave vouch.index_db import as-is); ensure the importorskip call precedes any use/import of MockEmbedder or other numpy-dependent test utilities.src/vouch/index_db.py (3)
344-348:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate cached query vector against current embedding metadata.
Lines 344-348 reuse cached vectors keyed only by query text; model/dimension drift can produce stale vectors and potential shape errors downstream.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/index_db.py` around lines 344 - 348, The cached query vector returned by lookup_query_vec(query) must be validated against the current embedder metadata before reuse: call the embedder's metadata (e.g., model name and embedding dimension from embedder or embedder.metadata) and verify the cached vector's stored model/dimension match; if they differ or the vector shape is incorrect, discard the cached vector, re-run embedder.encode(query) and call cache_query_vec to store the new vector, then proceed to search_embedding; update the logic around lookup_query_vec, embedder.encode, cache_query_vec, and search_embedding to perform this validation and recaching.
315-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPython fallback scoring is not cosine-consistent.
At Line 317, raw dot product can skew ranking by vector magnitude versus the sqlite-vec cosine path. Normalize consistently in fallback scoring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/index_db.py` around lines 315 - 318, The fallback scoring currently uses raw dot product (score = float(q @ vec)) which is inconsistent with sqlite-vec's cosine scoring; change the fallback in the loop that iterates over rows to compute cosine similarity instead by normalizing both the query vector q and the document vector produced by _blob_to_vec (or divide the dot by (||q|| * ||vec||)), precompute ||q|| once, guard against zero norms (skip or set score to 0 if either norm is zero), and then compare that normalized cosine score against min_score so ranking matches the sqlite-vec path; update references to q, _blob_to_vec, vec, score, and min_score accordingly.
89-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reset()doesn’t clear embedding-derived tables.Line 93 only clears FTS tables; embedding tables/meta/cache remain and can leak stale semantic state after rebuilds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/index_db.py` around lines 89 - 94, The reset(kb_dir) function currently only deletes FTS tables (claims_fts, pages_fts, entities_fts) but leaves embedding-derived tables and meta/cache behind; update reset (the function using open_db) to also clear all embedding-related tables and metadata/cache (for example embeddings, embedding_meta, embedding_cache and any *_embeddings tables or other embedding index tables your schema uses) by executing DELETE (or DROP/CREATE as appropriate) against those tables in the same DB transaction, and optionally run VACUUM or equivalent cleanup after to remove leftover state.tests/embeddings/test_storage.py (1)
7-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
pytest.importorskip("numpy")for optional dependency safety.Line 7 performs an unconditional NumPy import; this can fail collection in environments that don’t install embeddings extras.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_storage.py` around lines 7 - 8, The unconditional "import numpy as np" should be replaced with a conditional import using pytest.importorskip to avoid test collection failures when the optional numpy dependency isn't installed; update the top of tests/embeddings/test_storage.py by calling pytest.importorskip("numpy") (or assigning numpy = pytest.importorskip("numpy")) instead of the bare "import numpy as np" so the tests are skipped gracefully when numpy is unavailable.src/vouch/embeddings/cache.py (1)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard NumPy import behind optional-dependency boundaries.
Line 10 still imports NumPy unconditionally; this can break CI/type-check/collection in environments without embeddings extras. Move NumPy imports into local call sites (or a
TYPE_CHECKINGpattern) so optional embeddings code remains import-safe.#!/bin/bash # Verify whether numpy is guaranteed in default/dev install paths and CI checks. set -euo pipefail fd -HI 'pyproject.toml|requirements.*|constraints.*|ci.yml|*.yaml|*.yml' rg -n "numpy|embeddings|dev|mypy|pytest" pyproject.toml .github/workflows/ci.yml rg -n "import numpy as np" src/vouch/embeddings/cache.py tests/embeddings/test_storage.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/cache.py` at line 10, Top-level "import numpy as np" in src.vouch.embeddings.cache must be removed and guarded: delete the unconditional import and instead import numpy locally inside the functions/methods that actually use it (e.g., any functions that manipulate arrays or call np.* in this module); for type hints use "from typing import TYPE_CHECKING" and put "if TYPE_CHECKING: import numpy as np" so annotations remain valid without requiring numpy at import time. Ensure all references to np within the file still work by adding the local import at each call site or the start of the function that needs it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 33-47: The fenced code block that lists files under
"src/vouch/embeddings/" is missing a language tag which triggers markdownlint
MD040; update the opening fence to include an explicit language (e.g., replace
``` with ```text or ```bash) so the block is recognized as plain text, and scan
the rest of the file for other ``` blocks (e.g., any README examples or config
snippets) and add appropriate language identifiers (text/sql/toml/bash) to each.
In `@src/vouch/embeddings/fusion.py`:
- Around line 22-27: The rrf_fuse function can divide by zero when k is
negative; add an upfront validation in rrf_fuse to ensure k is non-negative (or
more strictly that k >= 0) and raise a ValueError with a clear message if
invalid, so the loop that uses 1.0 / (k + rank) never encounters (k + rank) ==
0; update any docstring or error text accordingly and keep the rest of the logic
(including use of _key and limit) unchanged.
---
Duplicate comments:
In `@src/vouch/embeddings/cache.py`:
- Line 10: Top-level "import numpy as np" in src.vouch.embeddings.cache must be
removed and guarded: delete the unconditional import and instead import numpy
locally inside the functions/methods that actually use it (e.g., any functions
that manipulate arrays or call np.* in this module); for type hints use "from
typing import TYPE_CHECKING" and put "if TYPE_CHECKING: import numpy as np" so
annotations remain valid without requiring numpy at import time. Ensure all
references to np within the file still work by adding the local import at each
call site or the start of the function that needs it.
In `@src/vouch/index_db.py`:
- Around line 344-348: The cached query vector returned by
lookup_query_vec(query) must be validated against the current embedder metadata
before reuse: call the embedder's metadata (e.g., model name and embedding
dimension from embedder or embedder.metadata) and verify the cached vector's
stored model/dimension match; if they differ or the vector shape is incorrect,
discard the cached vector, re-run embedder.encode(query) and call
cache_query_vec to store the new vector, then proceed to search_embedding;
update the logic around lookup_query_vec, embedder.encode, cache_query_vec, and
search_embedding to perform this validation and recaching.
- Around line 315-318: The fallback scoring currently uses raw dot product
(score = float(q @ vec)) which is inconsistent with sqlite-vec's cosine scoring;
change the fallback in the loop that iterates over rows to compute cosine
similarity instead by normalizing both the query vector q and the document
vector produced by _blob_to_vec (or divide the dot by (||q|| * ||vec||)),
precompute ||q|| once, guard against zero norms (skip or set score to 0 if
either norm is zero), and then compare that normalized cosine score against
min_score so ranking matches the sqlite-vec path; update references to q,
_blob_to_vec, vec, score, and min_score accordingly.
- Around line 89-94: The reset(kb_dir) function currently only deletes FTS
tables (claims_fts, pages_fts, entities_fts) but leaves embedding-derived tables
and meta/cache behind; update reset (the function using open_db) to also clear
all embedding-related tables and metadata/cache (for example embeddings,
embedding_meta, embedding_cache and any *_embeddings tables or other embedding
index tables your schema uses) by executing DELETE (or DROP/CREATE as
appropriate) against those tables in the same DB transaction, and optionally run
VACUUM or equivalent cleanup after to remove leftover state.
In `@src/vouch/jsonl_server.py`:
- Around line 99-105: The hybrid branch (backend_arg == "hybrid") currently
calls index_db.search_semantic and index_db.search without the same safeguards
as other backends: it must apply the min_score cutoff to the semantic results
and guard index_db.search with the same try/except fallback logic. Modify the
hybrid block to (1) call index_db.search_semantic(..., limit=limit*2) and filter
out semantic hits below min_score before passing to rrf_fuse, and (2) wrap
index_db.search(...) in the same try/except used elsewhere so exceptions don’t
abort the request (fall back to an empty FTS result on error). Ensure you still
pass the filtered semantic list and the safe FTS list into rrf_fuse(...,
limit=limit).
- Around line 79-83: Reject unknown backend values by validating backend_arg
against the canonical supported set used by server.kb_search(); if backend_arg
not in that set (and not the special "auto"), raise a ValueError. Update the
code around the backend_arg/used assignment to fetch or reference the same
supported-backends collection used by server.kb_search() (or define a small
SUPPORTED_BACKENDS set if one doesn’t exist), then perform an if backend_arg not
in SUPPORTED_BACKENDS: raise ValueError(f"unknown backend: {backend_arg}")
before continuing and setting used.
In `@src/vouch/server.py`:
- Around line 126-133: In the hybrid branch where you call
index_db.search_semantic and index_db.search and then rrf_fuse, apply the same
semantic min_score filtering and FTS fallback used by other backends: filter the
semantic results from index_db.search_semantic by the existing min_score
variable before fusion, and wrap index_db.search in a try/except so any
exception yields an empty FTS hits list instead of bubbling up; then call
rrf_fuse(emb_filtered, fts_or_empty, limit=limit) and return _to_dicts(hits,
"hybrid"). Ensure you reference the same symbols used here:
index_db.search_semantic, index_db.search, rrf_fuse, min_score, and _to_dicts.
In `@src/vouch/storage.py`:
- Around line 482-509: _wrap the remainder of _embed_and_store after obtaining h
and ensuring the artifact file exists in a broad try/except Exception block so
any error from get_embedder, embedder.encode, _index_db.put_embedding,
_index_db.set_embedding_meta, or check_and_log does not propagate; on exception
log a concise error including kind and id and then return silently, preserving
the already-persisted artifact and keeping embedding/indexing strictly
best-effort (refer to symbols: _embed_and_store, get_embedder, embedder.encode,
_index_db.put_embedding, _index_db.set_embedding_meta, check_and_log).
- Around line 482-488: The early-return after computing h currently only checks
content_hash(text) and can skip re-embedding when the active embedder changed;
modify the logic in the block that fetches existing =
_index_db.get_embedding(self.kb_dir, kind=kind, id=id) so that you also obtain
the current embedder identity from get_embedder() (e.g., a model_name or id
property) and compare it to the stored model identity in existing (the record
returned by get_embedding); only return early if both the stored content hash
equals h and the stored model identity matches the current embedder; otherwise
fall through to re-embed and update the index.
In `@tests/embeddings/test_search.py`:
- Around line 7-10: The test module imports MockEmbedder at import time which
causes collection to fail when numpy is not present; call
pytest.importorskip("numpy") at the top of the module before importing
MockEmbedder so pytest will skip the whole module cleanly, then import
MockEmbedder from tests.embeddings._fakes (leave vouch.index_db import as-is);
ensure the importorskip call precedes any use/import of MockEmbedder or other
numpy-dependent test utilities.
In `@tests/embeddings/test_storage.py`:
- Around line 7-8: The unconditional "import numpy as np" should be replaced
with a conditional import using pytest.importorskip to avoid test collection
failures when the optional numpy dependency isn't installed; update the top of
tests/embeddings/test_storage.py by calling pytest.importorskip("numpy") (or
assigning numpy = pytest.importorskip("numpy")) instead of the bare "import
numpy as np" so the tests are skipped gracefully when numpy is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af8c2c97-c9a6-490e-9ca5-e8c1fbd52779
📒 Files selected for processing (23)
docs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/vouch/bundle.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/conftest.pytests/embeddings/test_core.pytests/embeddings/test_fusion.pytests/embeddings/test_integration.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
✅ Files skipped from review due to trivial changes (1)
- tests/embeddings/init.py
Comment on lines
+33
to
+47
| ``` | ||
| src/vouch/embeddings/ | ||
| __init__.py # public API: encode, search, register | ||
| base.py # Embedder ABC + adapter registry | ||
| st_mpnet.py # default impl (sentence-transformers all-mpnet-base-v2) | ||
| st_minilm.py # alternative impl | ||
| fastembed_bge.py # alternative impl (no-torch path via fastembed) | ||
| cache.py # query embedding LRU + content-hash skip cache | ||
| rerank.py # cross-encoder reranker (ms-marco-MiniLM-L6-v2) | ||
| hyde.py # Hypothetical Document Embedding query expansion | ||
| dedup.py # cosine-threshold duplicate detection at ingest | ||
| fusion.py # RRF, weighted-sum, normalized-cosine fusion strategies | ||
| eval.py # recall@k / MRR / nDCG harness | ||
| migration.py # model-identity check + backfill orchestration | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
The fence starting at Line 33 is missing a language tag (MD040), which can fail docs linting in CI. Add explicit languages (e.g., text, sql, toml, bash) for each fenced block in this file.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md` around lines 33
- 47, The fenced code block that lists files under "src/vouch/embeddings/" is
missing a language tag which triggers markdownlint MD040; update the opening
fence to include an explicit language (e.g., replace ``` with ```text or
```bash) so the block is recognized as plain text, and scan the rest of the file
for other ``` blocks (e.g., any README examples or config snippets) and add
appropriate language identifiers (text/sql/toml/bash) to each.
379297a to
993e7da
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/vouch/embeddings/fusion.py (1)
22-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
kto prevent division-by-zero in RRF.
k < 0can make(k + rank)equal zero (e.g.,k=-1, first item), causing a runtime crash.Proposed fix
def rrf_fuse(a: Hits, b: Hits, *, limit: int = 10, k: int = 60) -> Hits: """Reciprocal Rank Fusion: score = sum(1 / (k + rank)) across lists.""" + if k < 0: + raise ValueError("k must be >= 0") scores: dict[tuple[str, str], float] = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/fusion.py` around lines 22 - 27, The rrf_fuse function can divide by zero when k is negative (1.0 / (k + rank)); add validation at the start of rrf_fuse to ensure k is non-negative (or at least k + 1 > 0) and raise a clear ValueError if invalid. Update the function that accepts parameter k (rrf_fuse) to validate k before the loop that uses rank and the expression 1.0/(k + rank) so the division can never receive zero or a negative denominator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/embeddings/fusion.py`:
- Around line 22-23: The rrf_fuse function currently slices results with a
negative limit which yields unexpected behavior; add a guard at the start of
rrf_fuse (checking the limit parameter) so that if limit <= 0 you return an
empty Hits object (or raise a ValueError if that API is preferred) instead of
performing slicing; apply the same guard to the other fusion functions in this
module (the other fuse implementations referenced around the other ranges) so
all functions consistently handle negative or zero limit values by returning no
results or failing fast.
In `@src/vouch/jsonl_server.py`:
- Line 100: The import of rrf_fuse in the import block is not formatted/sorted
per project lint rules (Ruff I001); update the import formatting and order to
match the module import group conventions and type-ignore comment style (for
example ensure "from .embeddings.fusion import rrf_fuse # type:
ignore[import-not-found,import-untyped]" is placed in the correct import section
and any unused-ignore is removed), or simply run "ruff check --fix" to auto-fix
the import formatting in src/vouch/jsonl_server.py so the rrf_fuse import
complies with the project's import styling rules.
In `@src/vouch/server.py`:
- Line 127: Import line "from .embeddings.fusion import rrf_fuse" in
src/vouch/server.py is unsorted causing Ruff I001; fix by sorting/formatting the
module imports (or running the auto-fixer) so import ordering complies with
ruff/isort rules, preserving the type-ignore comment (type:
ignore[import-not-found,import-untyped,unused-ignore]) and the symbol rrf_fuse;
run `ruff check --fix` (or `ruff format`/`isort`) and commit the updated import
block.
---
Duplicate comments:
In `@src/vouch/embeddings/fusion.py`:
- Around line 22-27: The rrf_fuse function can divide by zero when k is negative
(1.0 / (k + rank)); add validation at the start of rrf_fuse to ensure k is
non-negative (or at least k + 1 > 0) and raise a clear ValueError if invalid.
Update the function that accepts parameter k (rrf_fuse) to validate k before the
loop that uses rank and the expression 1.0/(k + rank) so the division can never
receive zero or a negative denominator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8aba9b3f-8cd2-4e99-91d1-8c8fd6de8ad1
📒 Files selected for processing (4)
src/vouch/embeddings/fusion.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/embeddings/test_fusion.py
dripsmvcp
force-pushed
the
feat/semantic-search-phase-5-fusion
branch
from
993e7da to
f6b8427
Compare
… hook Bundles the substantive correctness fixes flagged by CodeRabbit on PR vouchdev#41. CI was already green; these are latent bugs the review caught. storage._embed_and_store - Re-embed when the active embedder model changes, not only when the content hash changes. The previous short-circuit kept stale vectors on disk after a model swap, mixing document embeddings from one model with queries from another. - Truly best-effort: catch any exception during encode/put/meta/dedup so a hook failure can't bubble back to the caller and leave an artifact persisted-but-API-errored. index_db.reset - Also clears embedding_index, query_embedding_cache, embedding_dupes, and embedding_* keys from index_meta. Otherwise `vouch index` left orphaned hits behind after artifacts were removed. index_db.search_embedding (Python fallback) - Normalize the stored vector by its own L2 norm before scoring, so rankings match the sqlite-vec `1 - vec_distance_cosine` path. The previous raw dot product made magnitude leak into the score. index_db.search_semantic - Invalidate the query-vector cache entry when the embedder dim no longer matches what's on disk; otherwise a model swap returned stale vectors living in the wrong space. server.kb_search + jsonl_server._h_search (hybrid branch) - Hybrid now passes min_score into search_semantic (consistent with embedding/auto branches) and wraps the FTS lookup in try/except so an FTS5 error doesn't fail the whole request. - JSONL transport rejects unknown backend values with a clear error instead of silently returning []; matches MCP transport behavior. embeddings.fusion - rrf_fuse validates limit >= 0 and k >= 0 (k = -rank would divide by zero). weighted_fuse validates limit >= 0. Tests - Three new fusion guard tests cover the negative-limit and negative-k paths.
2 tasks
plind-junior
added a commit
that referenced
this pull request
May 22, 2026
… hook Bundles the substantive correctness fixes flagged by CodeRabbit on PR #41. CI was already green; these are latent bugs the review caught. storage._embed_and_store - Re-embed when the active embedder model changes, not only when the content hash changes. The previous short-circuit kept stale vectors on disk after a model swap, mixing document embeddings from one model with queries from another. - Truly best-effort: catch any exception during encode/put/meta/dedup so a hook failure can't bubble back to the caller and leave an artifact persisted-but-API-errored. index_db.reset - Also clears embedding_index, query_embedding_cache, embedding_dupes, and embedding_* keys from index_meta. Otherwise `vouch index` left orphaned hits behind after artifacts were removed. index_db.search_embedding (Python fallback) - Normalize the stored vector by its own L2 norm before scoring, so rankings match the sqlite-vec `1 - vec_distance_cosine` path. The previous raw dot product made magnitude leak into the score. index_db.search_semantic - Invalidate the query-vector cache entry when the embedder dim no longer matches what's on disk; otherwise a model swap returned stale vectors living in the wrong space. server.kb_search + jsonl_server._h_search (hybrid branch) - Hybrid now passes min_score into search_semantic (consistent with embedding/auto branches) and wraps the FTS lookup in try/except so an FTS5 error doesn't fail the whole request. - JSONL transport rejects unknown backend values with a clear error instead of silently returning []; matches MCP transport behavior. embeddings.fusion - rrf_fuse validates limit >= 0 and k >= 0 (k = -rank would divide by zero). weighted_fuse validates limit >= 0. Tests - Three new fusion guard tests cover the negative-limit and negative-k paths.
plind-junior
added a commit
that referenced
this pull request
May 22, 2026
…sion feat(embeddings): hybrid fusion strategies (RRF, weighted, normalized)
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
2 participants
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
Phase 5: enable the
backend="hybrid"code path stubbed out in Phase 4. Adds three fusion strategies — Reciprocal Rank Fusion (default), min-max-normalized weighted blend, and equal-weight normalized — for combining FTS5 and embedding result lists into a single ranked list.Tracks Task 17 in the plan.
Commit
ec626eesrc/vouch/embeddings/fusion.pywithrrf_fuse,weighted_fuse,normalized_fuse; tests intests/embeddings/test_fusion.pyDesign notes
RRF default with k=60 — Cormack et al.'s canonical parameter-free fusion. Robust against score-scale differences between BM25 (negative log-odds) and cosine similarity (
[-1, 1]).Weighted fuse min-max-normalizes each list first, then linearly blends with caller-supplied weights. Useful when one signal is known to be higher-quality for a workload.
Snippet coalescing — fused hits inherit the snippet from whichever input list had one (embedding hits have
""snippets since the backend returns IDs and cosine only; FTS5 supplies the snippet).Test Plan
.venv/bin/pytest tests/embeddings/test_fusion.py -v→ 4 passed (RRF top-of-both-lists, empty inputs, weight dominance, zero-score normalization).venv/bin/pytest tests/embeddings -v→ no regression.venv/bin/python -m ruff check src/vouch/embeddings/fusion.py tests/embeddings/test_fusion.py→ cleanWhat's NOT in this PR
hybridbranch ofkb.search/_h_search(added in Phase 4) — no CLI flag exposes it yet. Phase 8 addsvouch search --backend hybrid.Summary by CodeRabbit
New Features
Tests