feat(search): add embedding-based semantic retrieval as a third backend (#35)#36
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds optional embedding support, persists/searches vectors in SQLite, rebuilds embeddings during index rebuild, exposes embedding/hybrid search via CLI and JSONL server with backend validation, and makes bundle member validation extension checks case-insensitive. ChangesSemantic Embedding Search
Bundle Validation Enhancement
Pre-commit tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vouch/server.py (1)
95-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
backend="substring"does not force substring search.At Line 95–108, explicit substring selection still executes FTS-first behavior. Please add a direct substring branch to honor the backend parameter contract.
🤖 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 95 - 108, The backend parameter currently doesn't short-circuit substring mode; add an explicit branch that checks if backend == "substring" and directly calls and returns store.search_substring(query, limit=limit) (using store and query/limit variables) before the try/except that falls back to substring; ensure the branch mirrors the return shape used by _search_embedding/_search_hybrid and the FTS path so callers get the same hit format and set backend to "substring" if your code relies on that variable.src/vouch/jsonl_server.py (1)
81-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
backend="substring"is ignored.When callers explicitly request substring search, execution still runs FTS-first fallback logic. Add an explicit substring branch so backend selection is deterministic.
Suggested patch
if backend == "embedding": return _search_embedding_jsonl(s, q, limit) if backend == "hybrid": return _search_hybrid_jsonl(s, q, limit) + if backend == "substring": + hits = s.search_substring(q, limit=limit) + return [ + {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": "substring"} + for k, i, sn, sc in hits + ]🤖 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 81 - 94, The backend="substring" case is ignored by the current FTS-first logic; add an explicit branch for backend == "substring" before trying index_db.search so the code calls s.search_substring(q, limit=limit) and returns those hits (and sets backend = "substring") immediately; keep the existing branches for "embedding" (_search_embedding_jsonl) and "hybrid" (_search_hybrid_jsonl) as-is, and preserve the try/except fallback that calls s.search_substring when index_db.search(s.kb_dir, q, limit=limit) fails or returns no hits.
🤖 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/capabilities.py`:
- Around line 60-65: The retrieval list currently appends "embedding" when
embeddings_available() returns true but doesn't include the "hybrid" option;
update the block that builds retrieval (the retrieval variable) so that when
from .embeddings import embeddings_available and embeddings_available() is true
you also append "hybrid" (in addition to "embedding") so capability advertising
matches kb.search supported backends; modify the try/except block around
embeddings_available() to append both "embedding" and "hybrid" to retrieval.
In `@src/vouch/index_db.py`:
- Around line 110-113: In the loop that iterates rows (the for kind, eid,
vec_json in rows block) guard against vector-dimension mismatches before calling
np.dot(q, v): after constructing v from vec_json, verify v.ndim == 1 and
v.shape[0] == q.shape[0] (or v.size == q.size) and if not, skip that row
(optionally logger.warning about kind/eid) and continue; alternatively wrap the
np.dot(q, v) in a try/except (catching ValueError) to skip problematic rows and
continue processing; keep the call to _snippet_for unchanged.
In `@src/vouch/server.py`:
- Around line 146-167: The fusion logic currently adds separate entries for the
same (kind, id) instead of combining them; update the loop that builds fused
(using seen, fused, fts_hits, emb_hits and the weight-based first loop) to key
results by (kind, id) and accumulate/merge scores into a single dict per key
(e.g., add weighted score to an existing entry if key in seen), pick or merge
the best snippet/snippet source and set backend to "hybrid", and only append a
new entry when the key is first encountered; after processing both fts_hits and
emb_hits sort fused by the combined "score" as before.
In `@tests/test_embeddings.py`:
- Around line 42-62: In test_index_and_search_embedding, there's a copy-paste
bug where index_db.index_embedding is called for pages with vec=p.id (a string)
instead of the computed vector; update the call to pass the vector variable vec
(the result of encode([f"{p.title} {p.body}"])[0].tolist()) so
index_db.index_embedding(conn, kind="page", id=p.id, vec=vec) uses the actual
embedding.
---
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 81-94: The backend="substring" case is ignored by the current
FTS-first logic; add an explicit branch for backend == "substring" before trying
index_db.search so the code calls s.search_substring(q, limit=limit) and returns
those hits (and sets backend = "substring") immediately; keep the existing
branches for "embedding" (_search_embedding_jsonl) and "hybrid"
(_search_hybrid_jsonl) as-is, and preserve the try/except fallback that calls
s.search_substring when index_db.search(s.kb_dir, q, limit=limit) fails or
returns no hits.
In `@src/vouch/server.py`:
- Around line 95-108: The backend parameter currently doesn't short-circuit
substring mode; add an explicit branch that checks if backend == "substring" and
directly calls and returns store.search_substring(query, limit=limit) (using
store and query/limit variables) before the try/except that falls back to
substring; ensure the branch mirrors the return shape used by
_search_embedding/_search_hybrid and the FTS path so callers get the same hit
format and set backend to "substring" if your code relies on that variable.
🪄 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: 3fcc76a8-1b80-4927-9206-dd77d0e4acce
📒 Files selected for processing (9)
pyproject.tomlsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/embeddings.pysrc/vouch/health.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/test_embeddings.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 103-105: Guard against empty or zero-norm query vectors before
normalizing: in the function that constructs q from query_vec (where you do q =
np.array(query_vec, dtype=np.float32) and q = q / np.linalg.norm(q)), check if
query_vec is empty or if np.linalg.norm(q) == 0 (or very close to 0) and return
an empty out list (out: list[tuple[str, str, str, float]] = []) or otherwise
short-circuit with appropriate return; update the code around q, query_vec and
the normalization to perform this check and avoid dividing by zero.
🪄 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: e9d6ba0a-ba19-4267-a2fc-b5cb34bcdc3f
📒 Files selected for processing (2)
src/vouch/embeddings.pysrc/vouch/index_db.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test_embeddings.py`:
- Around line 26-27: The test imports numpy directly which can hard-fail;
replace the direct import with a guarded import by calling
pytest.importorskip("numpy") and assigning its return to the np symbol before
gating sentence_transformers (i.e., ensure np is obtained via
pytest.importorskip("numpy") and keep the existing
pytest.importorskip("sentence_transformers") guard), so the test is skipped
cleanly when numpy is absent.
🪄 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: 276178cc-ca45-41a3-b442-27ccae72d01c
📒 Files selected for processing (1)
tests/test_embeddings.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bundle.py`:
- Around line 212-213: The early-return condition starting at the if not
any(path.endswith(ext) ...) line bypasses validation for many artifact
directories; remove this global extension gate and instead branch validation by
the file's subdirectory (use the path variable or derive subdir from it). For
example, enforce only .yaml/.yml for claims/, sources/, entities/; allow .md
only for documentation-like dirs; and explicitly reject unexpected extensions
for known artifact subdirs (and raise an import_check/import_apply validation
error for unknown subdirs). Update the logic where path is inspected so
import_check/import_apply continue to validate per-subdir rules rather than
skipping files by extension.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bundle.py`:
- Around line 216-217: The extension check using path.endswith(ext) is
case-sensitive and misses uppercase variants; update the conditional that tests
the variable path (the if block performing any(path.endswith(ext) for ext in
(".yaml", ".yml", ".md"))) to perform a case-insensitive comparison by
normalizing the filename (e.g., use path.lower() or path.casefold()) before
calling endswith so files like .YAML or .MD are correctly matched.
🪄 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: 688bbf9f-c98d-4dca-b474-9c95b5dd61e7
📒 Files selected for processing (6)
src/vouch/bundle.pysrc/vouch/capabilities.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/test_embeddings.py
|
Requesting changes. This is a solid MVP and I'd be happy to see it land, but there are a few bugs that I think need to be fixed first, plus one design choice that's going to hurt the next person who touches this. The biggest one is in The "hybrid" path isn't actually doing RRF either. In seen[key] = sc + rank_scorewhich mixes raw BM25 ( for item in fused:
item["score"] = seen[(item["kind"], item["id"])]is also a no-op — On storage: vectors are going through Related, and this one will bite you later: the schema declares A bunch of smaller stuff:
For tests, I'd want to see one that pins One last thing — the dripsmvcp series (#37–#44) is a much bigger rewrite of the same feature and addresses #4-#6 here. Worth a chat with that author about whether to land this as the MVP and stack their work on top, or close this and review their series. Probably shouldn't merge both. |
|
Fix the conflict or it will be closed in 12hr |
…vouchdev#35) MVP implementation: - src/vouch/embeddings.py - lazy-loaded sentence-transformers model - index_db.py - embeddings table + NumPy brute-force cosine search - server.py/jsonl_server.py - backend='embedding' and 'hybrid' support - cli.py - vouch search --embedding flag - health.py - embeddings rebuilt during vouch index - capabilities.py - advertises embedding backend when available - pyproject.toml - optional vouch[embeddings] dep
… guards, capabilities
- Use pytest.importorskip('numpy') instead of direct import
- Replace global extension gate with per-subdir validation
6bae7cc to
cbbd05d
Compare
2 participants
Summary
Adds a third search backend — embedding-based semantic retrieval — alongside the existing FTS5 and substring backends.
Closes #35
Changes
src/vouch/embeddings.py(new)all-MiniLM-L6-v2model viasentence-transformersembeddings_available())encode()with normalized embeddingssrc/vouch/index_db.pyembeddingstable instate.dbschemaindex_embedding()— store embedding vectorssearch_embeddings()— NumPy brute-force cosine similarity searchSearch dispatch
kb_search(MCP) and_h_search(JSONL) acceptbackendparam"embedding"— semantic search only"hybrid"— reciprocal rank fusion of FTS5 + embedding resultsvouch search --embedding <query>Index rebuild
vouch indexnow also regenerates embeddings (whensentence-transformersis installed)Packaging
pip install vouch[embeddings]Tests
test_embeddings.py— encode shape, index + search, relevance ranking, empty indexAcceptance criteria covered
backend="embedding"andbackend="hybrid"pip install vouch(no extras) still works with FTS5/substringvouch indexrebuilds embeddings (idempotent)Summary by CodeRabbit
New Features
Improvements
Chores