feat(embeddings): semantic-primary kb.search and JSONL parity - #40
Merged
plind-junior merged 16 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:
📝 WalkthroughWalkthroughAdds an embedding-first semantic search system: embedder registry and adapters, query embedding cache, SQLite vector persistence with optional sqlite-vec ANN, write-time embedding persistence across artifacts, backend-selectable search (semantic/FTS5/hybrid), tests, and a design specification. ChangesSemantic Embedding Search System
Sequence DiagramssequenceDiagram
participant KB as KBStore.put_claim
participant Embed as _embed_and_store
participant Model as Embedder.encode
participant IndexDB as index_db.put_embedding
KB->>Embed: (kind=claim, id, text)
Embed->>Embed: compute content_hash(text)
Embed->>IndexDB: get_embedding(kind, id)
IndexDB-->>Embed: existing_entry or None
alt hash unchanged
Embed-->>KB: no-op (skip)
else hash changed
Embed->>Model: encode(text)
Model-->>Embed: normalized vector
Embed->>IndexDB: put_embedding(vec, content_hash, model_meta)
IndexDB-->>Embed: stored
end
Embed-->>KB: done
sequenceDiagram
participant Client as caller (kb_search/jsonl)
participant Server as server/jsonl_server
participant Cache as embeddings.cache
participant Embed as Embedder
participant Index as index_db.search_embedding
Client->>Server: query, backend=auto
Server->>Cache: lookup_query_vec(query)
Cache-->>Server: cached_vec or miss
alt cache miss
Server->>Embed: encode(query)
Embed-->>Server: query_vec
Server->>Cache: cache_query_vec(query, query_vec)
end
Server->>Index: search_embedding(query_vec, min_score)
Index-->>Server: ranked results
Server-->>Client: {backend: used, hits: [...]}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
🧹 Nitpick comments (2)
src/vouch/embeddings/base.py (1)
48-50: ⚡ Quick winGuard against accidental registry key collisions.
register()currently overwrites existing factories silently. A collision check avoids hard-to-debug adapter resolution changes.♻️ Proposed refactor
def register(name: str, factory: Callable[[], Embedder]) -> None: """Register an Embedder factory under `name`. Idempotent.""" - _REGISTRY[name] = factory + existing = _REGISTRY.get(name) + if existing is not None and existing is not factory: + raise ValueError(f"embedder already registered under {name!r}") + _REGISTRY.setdefault(name, factory)🤖 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/base.py` around lines 48 - 50, The register function (_REGISTRY and register) currently overwrites existing entries silently; change register(name, factory) to check _REGISTRY for an existing key and raise an appropriate exception (e.g., ValueError) or log+no-op if name already exists to prevent accidental collisions; update the implementation of register to perform this guard (check name in _REGISTRY before assignment) and include a clear error message referencing the conflicting name and possibly the existing factory to aid debugging.src/vouch/index_db.py (1)
248-271: 💤 Low valueConnection ID caching may cause spurious cache hits.
id(conn)returns the memory address, which Python can reuse after a connection is closed. A new connection might get the same ID as a previously closed one, causing the function to skip loading the extension. The downstreamexcept sqlite3.OperationalErrorfallback mitigates this, but it could cause unnecessary client-side brute-force searches.Consider using a
WeakValueDictionarykeyed by connection, or attaching a loaded flag directly to the connection object.🤖 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 248 - 271, The cache uses id(conn) via _sqlite_vec_loaded in _load_sqlite_vec which can be reused after a connection is closed; replace this fragile id-based set with a safer approach such as a WeakKeyDictionary keyed by the actual sqlite3.Connection object or by attaching a boolean attribute to the conn object (e.g., conn._sqlite_vec_loaded) to mark that the extension was loaded; update _load_sqlite_vec to check and set that per-connection marker (or check/insert into the WeakKeyDictionary) instead of using id(conn) so closed connections won't cause spurious cache 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 `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 213-227: The test-file count in the Test plan is inconsistent: the
heading "## 13. Test plan (~900 LOC across 9 files)"/acceptance criteria says 9
files but the table under it lists 10 test files; pick a canonical count (either
9 or 10) and make the doc consistent by either removing the extra row from the
table or updating the heading/acceptance text to reflect the true number, and
apply the same change to the other occurrence referenced (lines 234-235). Locate
the block titled "## 13. Test plan" and the following table rows
(tests/test_embeddings_*.py) and adjust the heading/count or the table entries
so both match.
- Around line 33-47: The fenced code block showing the directory listing for
"src/vouch/embeddings/" lacks a language identifier and triggers markdownlint
MD040; edit the block in
docs/superpowers/specs/2026-05-20-semantic-search-design.md that contains the
listing (lines with "src/vouch/embeddings/" and filenames like "__init__.py",
"base.py", "st_mpnet.py", etc.) and add a language tag (e.g., ```text)
immediately after the opening backticks so the block becomes fenced with a
language identifier.
In `@src/vouch/embeddings/__init__.py`:
- Around line 22-29: Replace the broad suppress(ImportError) blocks in
src/vouch/embeddings/__init__.py that auto-import st_mpnet, st_minilm, and
fastembed_bge with explicit try/except ModuleNotFoundError logic: for each
import (from . import st_mpnet, st_minilm, fastembed_bge) wrap in try and catch
ModuleNotFoundError as e, define an allowed_optional_deps set (e.g.,
{"numpy","torch","transformers","sentence_transformers","bge"} tuned to the
adapter), and if e.name is in that set silently skip registration, otherwise
re-raise the exception so unexpected ImportError causes a failure.
In `@src/vouch/jsonl_server.py`:
- Around line 79-108: Validate the backend_arg value after the hybrid block in
jsonl_server.py by checking it is one of the allowed backends ("auto",
"embedding", "fts5", "substring", "hybrid"); if it is not, raise
ValueError(f"unknown backend: {backend_arg}") so the handler mirrors server.py
behavior and does not silently return empty results; locate the validation near
the variables backend_arg/used and the search calls (index_db.search_semantic,
index_db.search, s.search_substring, rrf_fuse) and add the check just after the
hybrid branch before returning the hits list.
In `@tests/embeddings/_fakes.py`:
- Around line 27-42: The encode method currently decodes arbitrary 4-byte
sequences as IEEE754 floats which can produce NaN/Inf; change
MockEmbedder.encode to derive finite deterministic floats by interpreting each
4-byte chunk as an unsigned integer (use int.from_bytes on the chunk), map that
integer into the [-1, 1] range (e.g. val / (2**32 - 1) scaled to [-1,1]), assign
into out, then normalize as before; locate and update the encode function
(variables: out, seed, i, dim) to replace struct.unpack("<f", ...) with this
integer-to-float mapping to ensure stable finite embeddings.
---
Nitpick comments:
In `@src/vouch/embeddings/base.py`:
- Around line 48-50: The register function (_REGISTRY and register) currently
overwrites existing entries silently; change register(name, factory) to check
_REGISTRY for an existing key and raise an appropriate exception (e.g.,
ValueError) or log+no-op if name already exists to prevent accidental
collisions; update the implementation of register to perform this guard (check
name in _REGISTRY before assignment) and include a clear error message
referencing the conflicting name and possibly the existing factory to aid
debugging.
In `@src/vouch/index_db.py`:
- Around line 248-271: The cache uses id(conn) via _sqlite_vec_loaded in
_load_sqlite_vec which can be reused after a connection is closed; replace this
fragile id-based set with a safer approach such as a WeakKeyDictionary keyed by
the actual sqlite3.Connection object or by attaching a boolean attribute to the
conn object (e.g., conn._sqlite_vec_loaded) to mark that the extension was
loaded; update _load_sqlite_vec to check and set that per-connection marker (or
check/insert into the WeakKeyDictionary) instead of using id(conn) so closed
connections won't cause spurious cache hits.
🪄 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: 54aba106-32f7-4450-9b17-a47720901a51
📒 Files selected for processing (19)
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/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_integration.pytests/embeddings/test_search.pytests/embeddings/test_storage.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 a language identifier to the fenced block.
This trips markdownlint MD040 and can break renderer syntax handling in docs tooling.
📝 Proposed fix
-```
+```text
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</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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 directory listing for
"src/vouch/embeddings/" lacks a language identifier and triggers markdownlint
MD040; edit the block in
docs/superpowers/specs/2026-05-20-semantic-search-design.md that contains the
listing (lines with "src/vouch/embeddings/" and filenames like "init.py",
"base.py", "st_mpnet.py", etc.) and add a language tag (e.g., ```text)
immediately after the opening backticks so the block becomes fenced with a
language identifier.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+213
to
+227
| ## 13. Test plan (~900 LOC across 9 files) | ||
|
|
||
| | File | Covers | | ||
| |---|---| | ||
| | `tests/test_embeddings_core.py` | Embedder ABC, registry, content-hash skip, batched encode, lazy load | | ||
| | `tests/test_embeddings_storage.py` | vec0 + sqlite-vec round trip + NumPy fallback parity | | ||
| | `tests/test_embeddings_search.py` | Semantic primary, FTS5 fallback, lexical-disjoint regression | | ||
| | `tests/test_embeddings_fusion.py` | RRF, weighted, normalized fusion strategies correctness | | ||
| | `tests/test_embeddings_rerank.py` | Cross-encoder rerank changes top-K order on a known pair | | ||
| | `tests/test_embeddings_hyde.py` | HyDE expansion improves recall on terse queries | | ||
| | `tests/test_embeddings_dedup.py` | Threshold ledger + audit event | | ||
| | `tests/test_embeddings_migration.py` | Model-version mismatch + backfill flow | | ||
| | `tests/test_embeddings_eval.py` | recall@k / MRR / nDCG correctness on synthetic ground truth | | ||
| | `tests/test_embeddings_cli.py` | CLI flag routing | | ||
|
|
There was a problem hiding this comment.
Fix test-file count inconsistency in the spec.
The heading/acceptance criteria say 9 test files, but the table lists 10. Keep one canonical count to avoid ambiguous release checks.
📝 Proposed fix
-## 13. Test plan (~900 LOC across 9 files)
+## 13. Test plan (~900 LOC across 10 files)
@@
-- [ ] All 9 test files pass; `ruff` and `mypy` clean.
+- [ ] All 10 test files pass; `ruff` and `mypy` clean.Also applies to: 234-235
🤖 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 213
- 227, The test-file count in the Test plan is inconsistent: the heading "## 13.
Test plan (~900 LOC across 9 files)"/acceptance criteria says 9 files but the
table under it lists 10 test files; pick a canonical count (either 9 or 10) and
make the doc consistent by either removing the extra row from the table or
updating the heading/acceptance text to reflect the true number, and apply the
same change to the other occurrence referenced (lines 234-235). Locate the block
titled "## 13. Test plan" and the following table rows
(tests/test_embeddings_*.py) and adjust the heading/count or the table entries
so both match.
Comment on lines
+22
to
+29
| with suppress(ImportError): | ||
| from . import st_mpnet # noqa: F401 | ||
|
|
||
| with suppress(ImportError): | ||
| from . import st_minilm # noqa: F401 | ||
|
|
||
| with suppress(ImportError): | ||
| from . import fastembed_bge # noqa: F401 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/vouch/embeddings/__init__.pyRepository: vouchdev/vouch
Length of output: 1261
🏁 Script executed:
cat -n src/vouch/embeddings/st_mpnet.pyRepository: vouchdev/vouch
Length of output: 1776
🏁 Script executed:
cat -n src/vouch/embeddings/st_minilm.pyRepository: vouchdev/vouch
Length of output: 1579
🏁 Script executed:
cat -n src/vouch/embeddings/fastembed_bge.pyRepository: vouchdev/vouch
Length of output: 1702
🏁 Script executed:
cat -n src/vouch/embeddings/base.pyRepository: vouchdev/vouch
Length of output: 2349
🏁 Script executed:
python3 << 'EOF'
# Test ModuleNotFoundError behavior
import sys
# Test 1: Check ModuleNotFoundError attributes
try:
import nonexistent_module
except ModuleNotFoundError as e:
print(f"Test 1 - exc.name: {e.name}")
print(f"Test 1 - exc type: {type(e).__name__}")
print(f"Test 1 - Is ImportError subclass: {isinstance(e, ImportError)}")
# Test 2: Nested import - missing dependency
# Create a test module that tries to import a missing package
test_code = """
import nonexistent_dependency
class Foo: pass
"""
import tempfile
import os
with tempfile.TemporaryDirectory() as tmpdir:
test_file = os.path.join(tmpdir, "test_mod.py")
with open(test_file, 'w') as f:
f.write(test_code)
sys.path.insert(0, tmpdir)
try:
import test_mod
except ModuleNotFoundError as e:
print(f"\nTest 2 - Nested import exc.name: {e.name}")
root = (e.name or "").split(".")[0]
print(f"Test 2 - root package: {root}")
print("\nConclusion: ModuleNotFoundError.name exists and is useful for filtering")
EOFRepository: vouchdev/vouch
Length of output: 350
🏁 Script executed:
find . -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" | head -5Repository: vouchdev/vouch
Length of output: 75
🏁 Script executed:
cat -n pyproject.toml | grep -A 30 "\[project"Repository: vouchdev/vouch
Length of output: 2437
Narrow exception handling for adapter auto-imports.
suppress(ImportError) is too broad and masks both intended and unintended errors. If numpy (a hard dependency of the [embeddings] extra) goes missing, the adapter silently fails to register instead of raising an error. Any unexpected ImportError in adapter code is also silently suppressed.
Use ModuleNotFoundError with an explicit set of allowed optional dependencies instead. This approach catches only the intended missing-dependency scenario and re-raises if something unexpected fails.
🔧 Proposed fix
-from contextlib import suppress
+from importlib import import_module
@@
-# Auto-register the default adapter if sentence-transformers is installed.
-with suppress(ImportError):
- from . import st_mpnet # noqa: F401
-
-with suppress(ImportError):
- from . import st_minilm # noqa: F401
-
-with suppress(ImportError):
- from . import fastembed_bge # noqa: F401
+def _import_optional(module: str, allowed_missing: set[str]) -> None:
+ try:
+ import_module(f"{__name__}.{module}")
+ except ModuleNotFoundError as exc:
+ root = (exc.name or "").split(".")[0]
+ if root not in allowed_missing:
+ raise
+
+
+_import_optional("st_mpnet", {"sentence_transformers"})
+_import_optional("st_minilm", {"sentence_transformers"})
+_import_optional("fastembed_bge", {"fastembed", "onnxruntime"})🤖 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/__init__.py` around lines 22 - 29, Replace the broad
suppress(ImportError) blocks in src/vouch/embeddings/__init__.py that
auto-import st_mpnet, st_minilm, and fastembed_bge with explicit try/except
ModuleNotFoundError logic: for each import (from . import st_mpnet, st_minilm,
fastembed_bge) wrap in try and catch ModuleNotFoundError as e, define an
allowed_optional_deps set (e.g.,
{"numpy","torch","transformers","sentence_transformers","bge"} tuned to the
adapter), and if e.name is in that set silently skip registration, otherwise
re-raise the exception so unexpected ImportError causes a failure.
Comment on lines
+79
to
108
| backend_arg = p.get("backend", "auto") | ||
| min_score = float(p.get("min_score", 0.0)) | ||
| hits: list[tuple[str, str, str, float]] = [] | ||
| used = backend_arg | ||
|
|
||
| if backend_arg in ("auto", "embedding"): | ||
| hits = index_db.search_semantic( | ||
| s.kb_dir, q, limit=limit, min_score=min_score, | ||
| ) | ||
| if hits: | ||
| used = "embedding" | ||
| if not hits and backend_arg in ("auto", "fts5"): | ||
| try: | ||
| hits = index_db.search(s.kb_dir, q, limit=limit) | ||
| used = "fts5" if hits else used | ||
| except Exception: | ||
| hits = [] | ||
| if not hits and backend_arg in ("auto", "substring"): | ||
| hits = s.search_substring(q, limit=limit) | ||
| backend = "substring" | ||
| used = "substring" | ||
| if backend_arg == "hybrid": | ||
| from .embeddings.fusion import rrf_fuse | ||
| emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2) | ||
| fts = index_db.search(s.kb_dir, q, limit=limit * 2) | ||
| hits = rrf_fuse(emb, fts, limit=limit) | ||
| used = "hybrid" | ||
|
|
||
| return [ | ||
| {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": backend} | ||
| {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used} | ||
| for k, i, sn, sc in hits |
There was a problem hiding this comment.
Missing validation for unknown backend values.
Unlike server.py which raises ValueError(f"unknown backend: {backend}") for invalid values, this handler silently accepts any backend string and returns empty results. This inconsistency could confuse callers and mask configuration errors.
🐛 Proposed fix: add validation after hybrid block
if backend_arg == "hybrid":
from .embeddings.fusion import rrf_fuse
emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2)
fts = index_db.search(s.kb_dir, q, limit=limit * 2)
hits = rrf_fuse(emb, fts, limit=limit)
used = "hybrid"
+
+ if backend_arg not in ("auto", "embedding", "fts5", "substring", "hybrid"):
+ raise ValueError(f"unknown backend: {backend_arg}")
return [
{"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used}
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 79 - 108, Validate the backend_arg
value after the hybrid block in jsonl_server.py by checking it is one of the
allowed backends ("auto", "embedding", "fts5", "substring", "hybrid"); if it is
not, raise ValueError(f"unknown backend: {backend_arg}") so the handler mirrors
server.py behavior and does not silently return empty results; locate the
validation near the variables backend_arg/used and the search calls
(index_db.search_semantic, index_db.search, s.search_substring, rrf_fuse) and
add the check just after the hybrid branch before returning the hits list.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-4-read-integration
branch
from
May 20, 2026 05:30
e8b7f02 to
d029f26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/vouch/jsonl_server.py (1)
100-102: ⚡ Quick winAdd
py.typedmarker toembeddingspackage to fix mypy errors.The pipeline mypy errors indicate
vouch.embeddings.fusionis missing apy.typedmarker. The# type: ignorecomment is correctly placed but mypy still flags it because the module exists without type stubs. Add an emptypy.typedfile tosrc/vouch/embeddings/to mark it as a typed package.🤖 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 100 - 102, The embeddings package (vouch.embeddings) is missing a py.typed marker which causes mypy to ignore the # type: ignore workaround for imports like vouch.embeddings.fusion (used for rrf_fuse); fix it by adding an empty py.typed file to the embeddings package directory (src/vouch/embeddings/) so mypy recognizes the package as typed and the import warnings are resolved.
🤖 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`:
- Around line 15-16: The cache key function _query_key currently hashes only the
query string which allows stale vectors to be reused across different embedding
models/versions/dimensions; change _query_key to incorporate embedding model
metadata (e.g., model name, model_version and embedding_dim) into the key (e.g.,
concatenate or JSON-serialize [query, model, version, dim] in a stable order
before hashing) and update every call site that uses _query_key (and the other
similar helpers at the other occurrences) to pass the model metadata coming from
the embedding call path (ensure callers in functions handling embeddings supply
the same model/version/dim values so cached entries are namespaced by model
identity).
In `@src/vouch/index_db.py`:
- Around line 311-314: The loop computing fallback scores can crash when stored
embeddings have a different dimension than the query; before computing score =
float(q @ vec) (where vec is produced by _blob_to_vec and q and min_score are in
scope), check that vec length or shape matches the query dimension (e.g.,
len(vec) == q.shape[0] or vec.size == q.size) and skip that row if it doesn't
match (optionally log or count mismatches), then only compute and compare score
>= min_score for matching-dimension embeddings.
---
Nitpick comments:
In `@src/vouch/jsonl_server.py`:
- Around line 100-102: The embeddings package (vouch.embeddings) is missing a
py.typed marker which causes mypy to ignore the # type: ignore workaround for
imports like vouch.embeddings.fusion (used for rrf_fuse); fix it by adding an
empty py.typed file to the embeddings package directory (src/vouch/embeddings/)
so mypy recognizes the package as typed and the import warnings are resolved.
🪄 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: c8f3964a-114b-4061-bb0c-a70ef9221bbc
📒 Files selected for processing (9)
pyproject.tomlsrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
Comment on lines
+15
to
+16
| def _query_key(query: str) -> str: | ||
| return hashlib.sha256(query.encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
Namespace query-cache keys by embedding model metadata.
Cache identity is query-only today, so model/version/dim changes can reuse stale vectors for the same text. That can silently degrade ranking or break downstream scoring when dimensions differ.
Suggested patch shape
-def _query_key(query: str) -> str:
- return hashlib.sha256(query.encode("utf-8")).hexdigest()
+def _query_key(query: str, *, model: str, model_version: str, dim: int) -> str:
+ scoped = f"{model}:{model_version}:{dim}:{query}"
+ return hashlib.sha256(scoped.encode("utf-8")).hexdigest()-def cache_query_vec(
- kb_dir: Path, *, query: str, vec: np.ndarray, max_entries: int = 1024,
+def cache_query_vec(
+ kb_dir: Path, *, query: str, vec: np.ndarray,
+ model: str, model_version: str, dim: int, max_entries: int = 1024,
) -> None:
- h = _query_key(query)
+ h = _query_key(query, model=model, model_version=model_version, dim=dim)-def lookup_query_vec(kb_dir: Path, *, query: str) -> np.ndarray | None:
- h = _query_key(query)
+def lookup_query_vec(
+ kb_dir: Path, *, query: str, model: str, model_version: str, dim: int
+) -> np.ndarray | None:
+ h = _query_key(query, model=model, model_version=model_version, dim=dim)Also applies to: 20-23, 46-48
🤖 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` around lines 15 - 16, The cache key function
_query_key currently hashes only the query string which allows stale vectors to
be reused across different embedding models/versions/dimensions; change
_query_key to incorporate embedding model metadata (e.g., model name,
model_version and embedding_dim) into the key (e.g., concatenate or
JSON-serialize [query, model, version, dim] in a stable order before hashing)
and update every call site that uses _query_key (and the other similar helpers
at the other occurrences) to pass the model metadata coming from the embedding
call path (ensure callers in functions handling embeddings supply the same
model/version/dim values so cached entries are namespaced by model identity).
Member
|
Hey, I want to flag something before this merges — Couple of ways we could handle it. Cleanest is probably to bump a A few smaller things while I'm in here: The
When embeddings are configured but broken (model file missing, network blip during install, whatever), And Oh — and could you add a test that pins the new return shape? Just a quick |
dripsmvcp
force-pushed
the
feat/semantic-search-phase-4-read-integration
branch
2 times, most recently
from
May 20, 2026 13:11
71952d8 to
d7fa544
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vouch/jsonl_server.py (1)
74-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn shape is inconsistent with MCP
kb_searchcontract.
_h_searchreturns a raw hit list, while MCP now returns{"backend": ..., "hits": [...]}. This breaks the parity objective and creates protocol divergence for clients.Suggested minimal fix
-def _h_search(p: dict) -> list[dict]: +def _h_search(p: dict) -> dict: @@ - return [ - {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used} - for k, i, sn, sc in hits - ] + return { + "backend": used, + "hits": [ + {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used} + 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 74 - 111, _h_search currently returns a list of hit dicts but must follow the MCP kb_search contract by returning an envelope with the chosen backend and the hits array; change the function to return {"backend": used, "hits": [...] } instead of the raw list, keeping the existing per-hit shape (the list comprehension that builds {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used} can be reused but the final return must be the dict envelope). Ensure you still compute and set the local variable used and hits as before (references: function _h_search, variables used and hits) so callers receive {"backend": used, "hits": <list>} consistently.
🧹 Nitpick comments (1)
src/vouch/index_db.py (1)
331-339: ⚡ Quick winSilent failures hamper debugging when embeddings are configured but fail.
Per reviewer feedback: when
get_embedder()fails (missing model, network issues) or imports fail, returning[]silently makes it hard to distinguish "no results" from "search didn't run." Consider adding a debug/audit log line before returning empty.♻️ Suggested improvement
+import logging + +_log = logging.getLogger(__name__) + def search_semantic(...): try: from .embeddings import get_embedder from .embeddings.cache import cache_query_vec, lookup_query_vec except ImportError: + _log.debug("Embeddings module unavailable; semantic search disabled") return [] try: embedder = get_embedder() except KeyError: + _log.debug("No embedder registered; semantic search disabled") 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/index_db.py` around lines 331 - 339, The current import and embedder acquisition blocks (imports of get_embedder, cache_query_vec, lookup_query_vec and the call to get_embedder()) silently return [] on ImportError or KeyError; update these blocks to log an informative debug/error message (including exception details) to the module logger before returning [] so callers can distinguish “no results” from a failed initialization — specifically wrap the import and embedder acquisition in try/except that captures the exception object and calls a logger (e.g., module-level logger) with a descriptive message and exception info prior to returning.
🤖 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 248-271: The cache using _sqlite_vec_loaded with id(conn) can
produce false positives because Python may reuse object ids after a connection
is closed; update _load_sqlite_vec to avoid id-based caching: either remove the
_sqlite_vec_loaded check entirely and always attempt to load the extension
(sqlite_vec.load(conn)) since loading is idempotent, or replace
_sqlite_vec_loaded with a connection-bound identity (e.g., store state on the
sqlite3.Connection object or use a weakref.WeakKeyDictionary/WeakSet keyed by
the actual connection object) so the loaded flag is tied to the live connection
rather than a recycled id; adjust the logic around conn.enable_load_extension
and the finally block to match the chosen approach and still return True only
when sqlite_vec.load succeeded.
- Around line 294-304: The SQL uses the SELECT alias "score" inside the WHERE
clause which SQLite disallows, causing the sqlite3.OperationalError in the
conn.execute call against embedding_index; replace the alias usage by inlining
the expression 1.0 - vec_distance_cosine(vec, ?) into the WHERE clause (or wrap
the SELECT as a subquery/CTE) so the filter becomes e.g. WHERE kind IN (...) AND
(1.0 - vec_distance_cosine(vec, ?)) >= ? and keep the ORDER BY using the
computed expression or alias in an outer query; update the parameters passed to
conn.execute accordingly so vec_distance_cosine gets the query vector twice if
inlined (or once if using a subquery/CTE).
- Around line 310-316: The fallback scoring loop uses _blob_to_vec to get stored
vectors but doesn't normalize them, so scoring q @ vec yields ||vec||*cos(θ)
instead of cosine similarity; update the loop in the function that builds scored
(using symbols _blob_to_vec, q, score, min_score, scored) to normalize vec to
unit length before computing score (or alternatively normalize vectors at write
time inside put_embedding), then compute score = float(q @ normalized_vec) and
apply the same min_score check and sorting.
In `@src/vouch/jsonl_server.py`:
- Around line 100-102: Mypy reports the import-untyped error on the from ...
import ( line, so move the type: ignore to that import statement: replace the
multi-line import of rrf_fuse from .embeddings.fusion with a single-line import
(e.g., from .embeddings.fusion import rrf_fuse # type:
ignore[import-not-found,import-untyped]) so the ignore is on the same line mypy
checks; keep the symbol name rrf_fuse and the same ignore codes.
In `@src/vouch/server.py`:
- Around line 127-129: The mypy ignore is on the symbol line instead of the
import line, so move the "# type: ignore[...]" comment onto the actual import
statement to suppress the import-time errors; specifically change the multi-line
import that brings in rrf_fuse in server.py (and the identical pattern in
jsonl_server.py) to a single-line import with the type ignore on that line
(apply to the import of rrf_fuse in src/vouch/server.py and the same import in
src/vouch/jsonl_server.py).
In `@src/vouch/storage.py`:
- Around line 486-500: The current sequence calling get_embedder(),
embedder.encode(text), and the DB functions (_index_db.put_embedding and
_index_db.set_embedding_meta) can raise and abort the write despite the
“best-effort” guarantee; surround the embedder.encode and the subsequent DB
writes in a single try/except that catches exceptions (e.g., Exception), logs
the failure (including embedder.name/version and the exception), and then
silently return (or continue) so failures do not propagate; keep the initial
get_embedder() KeyError handling as-is but ensure any runtime errors from
embedder.encode or _index_db.* are swallowed and logged to preserve non-blocking
behavior.
---
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 74-111: _h_search currently returns a list of hit dicts but must
follow the MCP kb_search contract by returning an envelope with the chosen
backend and the hits array; change the function to return {"backend": used,
"hits": [...] } instead of the raw list, keeping the existing per-hit shape (the
list comprehension that builds {"kind": k, "id": i, "snippet": sn, "score": sc,
"backend": used} can be reused but the final return must be the dict envelope).
Ensure you still compute and set the local variable used and hits as before
(references: function _h_search, variables used and hits) so callers receive
{"backend": used, "hits": <list>} consistently.
---
Nitpick comments:
In `@src/vouch/index_db.py`:
- Around line 331-339: The current import and embedder acquisition blocks
(imports of get_embedder, cache_query_vec, lookup_query_vec and the call to
get_embedder()) silently return [] on ImportError or KeyError; update these
blocks to log an informative debug/error message (including exception details)
to the module logger before returning [] so callers can distinguish “no results”
from a failed initialization — specifically wrap the import and embedder
acquisition in try/except that captures the exception object and calls a logger
(e.g., module-level logger) with a descriptive message and exception info prior
to returning.
🪄 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: 2d53037a-9058-4775-8897-a277861202fa
📒 Files selected for processing (9)
src/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/conftest.pytests/embeddings/test_search.pytests/embeddings/test_storage.py
Comment on lines
+310
to
+316
| scored: list[tuple[str, str, str, float]] = [] | ||
| for kind, id_, blob, dim in rows: | ||
| vec = _blob_to_vec(blob, dim) | ||
| score = float(q @ vec) | ||
| if score >= min_score: | ||
| scored.append((kind, id_, "", score)) | ||
| scored.sort(key=lambda r: r[3], reverse=True) |
There was a problem hiding this comment.
Fallback scoring doesn't normalize stored vectors, producing incorrect cosine similarity.
The query vector is normalized (line 288), but stored vectors are used directly in the dot product. This computes ||vec|| * cos(θ) rather than cos(θ), biasing scores toward vectors with larger magnitudes.
Either normalize stored vectors at write time (put_embedding) or normalize them here before scoring.
🐛 Proposed fix: normalize in fallback loop
scored: list[tuple[str, str, str, float]] = []
+ qdim = int(q.shape[0])
for kind, id_, blob, dim in rows:
+ if int(dim) != qdim:
+ continue # dimension mismatch guard (from prior review)
vec = _blob_to_vec(blob, dim)
+ vnorm = float(np.linalg.norm(vec))
+ if vnorm > 0:
+ vec = vec / vnorm
score = float(q @ vec)
if score >= min_score:
scored.append((kind, id_, "", score))📝 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
| scored: list[tuple[str, str, str, float]] = [] | |
| for kind, id_, blob, dim in rows: | |
| vec = _blob_to_vec(blob, dim) | |
| score = float(q @ vec) | |
| if score >= min_score: | |
| scored.append((kind, id_, "", score)) | |
| scored.sort(key=lambda r: r[3], reverse=True) | |
| scored: list[tuple[str, str, str, float]] = [] | |
| qdim = int(q.shape[0]) | |
| for kind, id_, blob, dim in rows: | |
| if int(dim) != qdim: | |
| continue # dimension mismatch guard (from prior review) | |
| vec = _blob_to_vec(blob, dim) | |
| vnorm = float(np.linalg.norm(vec)) | |
| if vnorm > 0: | |
| vec = vec / vnorm | |
| score = float(q @ vec) | |
| if score >= min_score: | |
| scored.append((kind, id_, "", score)) | |
| scored.sort(key=lambda r: r[3], reverse=True) |
🤖 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 310 - 316, The fallback scoring loop uses
_blob_to_vec to get stored vectors but doesn't normalize them, so scoring q @
vec yields ||vec||*cos(θ) instead of cosine similarity; update the loop in the
function that builds scored (using symbols _blob_to_vec, q, score, min_score,
scored) to normalize vec to unit length before computing score (or alternatively
normalize vectors at write time inside put_embedding), then compute score =
float(q @ normalized_vec) and apply the same min_score check and sorting.
Comment on lines
+486
to
+500
| try: | ||
| embedder = get_embedder() | ||
| except KeyError: | ||
| return | ||
| vec = embedder.encode(text) | ||
| with _index_db.open_db(self.kb_dir) as conn: | ||
| _index_db.put_embedding( | ||
| conn, kind=kind, id=id, vec=vec, content_hash=h, | ||
| model=embedder.name, model_version=embedder.version, | ||
| dim=embedder.dim, | ||
| ) | ||
| _index_db.set_embedding_meta( | ||
| self.kb_dir, model=embedder.name, | ||
| version=embedder.version, dim=embedder.dim, | ||
| ) |
There was a problem hiding this comment.
Embedding failures can still abort writes despite the “best-effort” contract.
embedder.encode(...) and embedding DB writes are not guarded, so runtime embedding issues can fail put_*/update_* operations even though this path is documented as non-blocking.
Suggested minimal hardening
- vec = embedder.encode(text)
- with _index_db.open_db(self.kb_dir) as conn:
- _index_db.put_embedding(
- conn, kind=kind, id=id, vec=vec, content_hash=h,
- model=embedder.name, model_version=embedder.version,
- dim=embedder.dim,
- )
- _index_db.set_embedding_meta(
- self.kb_dir, model=embedder.name,
- version=embedder.version, dim=embedder.dim,
- )
+ try:
+ vec = embedder.encode(text)
+ with _index_db.open_db(self.kb_dir) as conn:
+ _index_db.put_embedding(
+ conn, kind=kind, id=id, vec=vec, content_hash=h,
+ model=embedder.name, model_version=embedder.version,
+ dim=embedder.dim,
+ )
+ _index_db.set_embedding_meta(
+ self.kb_dir, model=embedder.name,
+ version=embedder.version, dim=embedder.dim,
+ )
+ except Exception:
+ return📝 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
| try: | |
| embedder = get_embedder() | |
| except KeyError: | |
| return | |
| vec = embedder.encode(text) | |
| with _index_db.open_db(self.kb_dir) as conn: | |
| _index_db.put_embedding( | |
| conn, kind=kind, id=id, vec=vec, content_hash=h, | |
| model=embedder.name, model_version=embedder.version, | |
| dim=embedder.dim, | |
| ) | |
| _index_db.set_embedding_meta( | |
| self.kb_dir, model=embedder.name, | |
| version=embedder.version, dim=embedder.dim, | |
| ) | |
| try: | |
| embedder = get_embedder() | |
| except KeyError: | |
| return | |
| try: | |
| vec = embedder.encode(text) | |
| with _index_db.open_db(self.kb_dir) as conn: | |
| _index_db.put_embedding( | |
| conn, kind=kind, id=id, vec=vec, content_hash=h, | |
| model=embedder.name, model_version=embedder.version, | |
| dim=embedder.dim, | |
| ) | |
| _index_db.set_embedding_meta( | |
| self.kb_dir, model=embedder.name, | |
| version=embedder.version, dim=embedder.dim, | |
| ) | |
| except Exception: | |
| 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 486 - 500, The current sequence calling
get_embedder(), embedder.encode(text), and the DB functions
(_index_db.put_embedding and _index_db.set_embedding_meta) can raise and abort
the write despite the “best-effort” guarantee; surround the embedder.encode and
the subsequent DB writes in a single try/except that catches exceptions (e.g.,
Exception), logs the failure (including embedder.name/version and the
exception), and then silently return (or continue) so failures do not propagate;
keep the initial get_embedder() KeyError handling as-is but ensure any runtime
errors from embedder.encode or _index_db.* are swallowed and logged to preserve
non-blocking behavior.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-4-read-integration
branch
from
May 20, 2026 13:16
d7fa544 to
fc8fea2
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-4-read-integration
branch
from
May 20, 2026 13:44
fc8fea2 to
9cfdf78
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vouch/jsonl_server.py (1)
74-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn shape is out of parity with
server.kb_search.
_h_searchstill returns a plain list, while Phase 4 contract is{ "backend": ..., "hits": [...] }. This creates protocol drift between JSONL and MCP/server and breaks callers expecting the new wire format.Suggested fix
-def _h_search(p: dict) -> list[dict]: +def _h_search(p: dict) -> dict: @@ - return [ - {"kind": k, "id": i, "snippet": sn, "score": sc, "backend": used} - for k, i, sn, sc in hits - ] + return { + "backend": used, + "hits": [ + {"kind": k, "id": i, "snippet": sn, "score": sc} + 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 74 - 111, _h_search currently returns a plain list of hit dicts while the Phase 4 contract expects an object with "backend" and "hits" keys (matching server.kb_search). Modify _h_search to return a dict: {"backend": used, "hits": [...]} where "hits" is the list comprehension currently produced (keep the same per-hit shape {"kind","id","snippet","score"}), and ensure any callers expecting the old plain list are updated or remain compatible; locate this change in the _h_search function and update its final return accordingly.
♻️ Duplicate comments (2)
src/vouch/server.py (1)
127-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove type ignore comment to the import line.
This is a duplicate of a past review comment. The
# type: ignore[...]on line 128 doesn't suppress theimport-untypederror that mypy reports on the import statement itself (line 127).🔧 Recommended fix
- from .embeddings.fusion import ( - rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore] - ) + from .embeddings.fusion import rrf_fuse # type: ignore[import-untyped]🤖 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 127 - 129, The type-ignore comment needs to be moved from the following standalone line to the actual import so mypy suppresses the errors on the import statement; update the import of rrf_fuse from .embeddings.fusion (the rrf_fuse import line) to include the appropriate inline comment (e.g., # type: ignore[import-not-found,import-untyped,unused-ignore]) directly on the import line and remove the separate comment line.src/vouch/storage.py (1)
486-509:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBest-effort embedding path can still fail writes.
Despite the docstring, runtime exceptions from
embedder.encode,_index_db.put_embedding,_index_db.set_embedding_meta, orcheck_and_logcan still propagate and abort artifact writes.Suggested hardening
- vec = embedder.encode(text) - with _index_db.open_db(self.kb_dir) as conn: - _index_db.put_embedding( - conn, kind=kind, id=id, vec=vec, content_hash=h, - model=embedder.name, model_version=embedder.version, - dim=embedder.dim, - ) - _index_db.set_embedding_meta( - self.kb_dir, model=embedder.name, - version=embedder.version, dim=embedder.dim, - ) - try: - # NB: dedup module is added in a later phase; ignore missing-stub - # / missing-module noise in CI's [dev]-only mypy run. - from .embeddings.dedup import ( # type: ignore[import-not-found,import-untyped,unused-ignore] - check_and_log, - ) - check_and_log(self.kb_dir, kind=kind, id=id, vec=vec) - except ImportError: - pass + try: + vec = embedder.encode(text) + with _index_db.open_db(self.kb_dir) as conn: + _index_db.put_embedding( + conn, kind=kind, id=id, vec=vec, content_hash=h, + model=embedder.name, model_version=embedder.version, + dim=embedder.dim, + ) + _index_db.set_embedding_meta( + self.kb_dir, model=embedder.name, + version=embedder.version, dim=embedder.dim, + ) + try: + from .embeddings.dedup import check_and_log # type: ignore[import-not-found,import-untyped] + check_and_log(self.kb_dir, kind=kind, id=id, vec=vec) + except ImportError: + pass + except Exception: + 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 486 - 509, The current "best-effort" embedding flow can still raise and abort writes; wrap the entire sequence that uses get_embedder(), embedder.encode(...), _index_db.put_embedding(...), _index_db.set_embedding_meta(...), and the optional check_and_log(...) call in a broad try/except that catches Exception, logs the error (including the exception details and context such as self.kb_dir, kind, id, and embedder name/version if available), and then continues without re-raising so artifact writes are not aborted; keep the existing KeyError early-return for get_embedder() but ensure any runtime failures in encode/DB/meta/check_and_log are handled.
🧹 Nitpick comments (1)
src/vouch/server.py (1)
103-124: ⚡ Quick winClarify or unify
min_scorebehavior across backends.The
min_scoreparameter is only applied when using theembeddingbackend (line 105) but is silently ignored byfts5(line 114) andsubstring(line 123) backends. This inconsistency can confuse callers who expectmin_scoreto apply uniformly.As noted in the PR objectives review feedback, either document that
min_scoreis embedding-only (in the docstring and parameter description) or apply score filtering across all backends that produce scores.📖 Documentation option
def kb_search( query: str, *, limit: int = 10, backend: str = "auto", min_score: float = 0.0, ) -> dict[str, Any]: """Search the KB. backend: "auto" (default, embedding then fts5 then substring), "embedding", "fts5", "substring", or "hybrid". + min_score: minimum similarity score (applies to embedding backend only). """🤖 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 103 - 124, The code only applies min_score to index_db.search_semantic (embedding) but ignores it for index_db.search (fts5) and store.search_substring (substring); update the search branches so that after obtaining hits from index_db.search and store.search_substring you filter hits by the same min_score threshold (e.g., keep hits where hit.score or hit["score"] >= min_score) before calling _to_dicts, and preserve the existing behavior of returning _to_dicts([], backend_name) when backend is explicitly requested and no hits remain; reference the functions index_db.search_semantic, index_db.search, store.search_substring and the helper _to_dicts when making the change.
🤖 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 217-227: The test file paths in the checklist are incorrect;
update each table row to reference the actual locations under the embeddings
subdirectory (e.g. replace tests/test_embeddings_core.py ->
tests/embeddings/test_core.py, tests/test_embeddings_storage.py ->
tests/embeddings/test_storage.py, tests/test_embeddings_search.py ->
tests/embeddings/test_search.py, tests/test_embeddings_fusion.py ->
tests/embeddings/test_fusion.py, tests/test_embeddings_rerank.py ->
tests/embeddings/test_rerank.py, tests/test_embeddings_hyde.py ->
tests/embeddings/test_hyde.py, tests/test_embeddings_dedup.py ->
tests/embeddings/test_dedup.py, tests/test_embeddings_migration.py ->
tests/embeddings/test_migration.py, tests/test_embeddings_eval.py ->
tests/embeddings/test_eval.py, tests/test_embeddings_cli.py ->
tests/embeddings/test_cli.py) so the doc checklist mirrors the repo layout.
In `@src/vouch/index_db.py`:
- Around line 335-343: When import/lookup of the embedder fails in
search_semantic, don't silently return []; instead catch the
ImportError/KeyError and log the exception and contextual info before returning.
Update the try/except blocks around get_embedder and the imports (get_embedder,
cache_query_vec, lookup_query_vec) to emit a diagnostic message (using the
module logger or logging.getLogger(__name__)) that includes the exception text
and a short note that semantic search fell back to returning empty hits, then
return [].
In `@src/vouch/jsonl_server.py`:
- Around line 99-106: The hybrid branch currently does a direct import of
rrf_fuse which can raise ImportError at runtime; wrap the import and fusion call
in a try/except ImportError around the backend_arg == "hybrid" block, and on
ImportError fall back to a graceful alternative (e.g., use
index_db.search_semantic or index_db.search alone, set used to "semantic" or
"fulltext" as appropriate, and log the import failure), otherwise proceed to
call rrf_fuse(emb, fts, limit=limit) and set used="hybrid"; locate the code
around the backend_arg check and symbols rrf_fuse, index_db.search_semantic,
index_db.search and ensure the exception handler does not expose a 500 stack
trace but returns/records a controlled fallback or error.
In `@src/vouch/server.py`:
- Around line 112-120: The try/except around index_db.search silently swallows
errors; update the exception handler in the block that calls
index_db.search(store.kb_dir, query, limit=limit) to log the failure (including
exception details and context like store.kb_dir, backend and query) before
falling back to hits = [] so operators can audit FTS5 errors; use the module
logger (or logger.exception / logger.error(..., exc_info=True)) and keep the
existing return paths that call _to_dicts so behavior doesn't change.
- Around line 126-133: The hybrid branch imports embeddings.fusion.rrf_fuse
unguarded so a missing Phase 5 module will raise ModuleNotFoundError; wrap the
import of rrf_fuse in a try/except ImportError inside the backend == "hybrid"
block (around the current from .embeddings.fusion import rrf_fuse), and if the
import fails raise or return a clear error (e.g., RuntimeError with a message
that the hybrid fusion feature is not available yet) so the call to
index_db.search_semantic / index_db.search and subsequent use of rrf_fuse and
_to_dicts won't crash unexpectedly.
In `@tests/embeddings/test_search.py`:
- Around line 112-114: The test currently treats resp["result"] as a list (old
JSONL wire format); change assertions to validate the Phase 4 dict-shaped
contract: assert that resp["result"] is a dict, that it contains keys "backend"
and "hits", that resp["result"]["backend"] is one of
("embedding","fts5","substring"), and that resp["result"]["hits"] is a list (and
optionally assert hits non-empty or inspect hits[0] shape if required). Update
the assertions around resp["result"] accordingly (look for resp["result"] usage
in the test function) so regressions to the old list format are caught.
---
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 74-111: _h_search currently returns a plain list of hit dicts
while the Phase 4 contract expects an object with "backend" and "hits" keys
(matching server.kb_search). Modify _h_search to return a dict: {"backend":
used, "hits": [...]} where "hits" is the list comprehension currently produced
(keep the same per-hit shape {"kind","id","snippet","score"}), and ensure any
callers expecting the old plain list are updated or remain compatible; locate
this change in the _h_search function and update its final return accordingly.
---
Duplicate comments:
In `@src/vouch/server.py`:
- Around line 127-129: The type-ignore comment needs to be moved from the
following standalone line to the actual import so mypy suppresses the errors on
the import statement; update the import of rrf_fuse from .embeddings.fusion (the
rrf_fuse import line) to include the appropriate inline comment (e.g., # type:
ignore[import-not-found,import-untyped,unused-ignore]) directly on the import
line and remove the separate comment line.
In `@src/vouch/storage.py`:
- Around line 486-509: The current "best-effort" embedding flow can still raise
and abort writes; wrap the entire sequence that uses get_embedder(),
embedder.encode(...), _index_db.put_embedding(...),
_index_db.set_embedding_meta(...), and the optional check_and_log(...) call in a
broad try/except that catches Exception, logs the error (including the exception
details and context such as self.kb_dir, kind, id, and embedder name/version if
available), and then continues without re-raising so artifact writes are not
aborted; keep the existing KeyError early-return for get_embedder() but ensure
any runtime failures in encode/DB/meta/check_and_log are handled.
---
Nitpick comments:
In `@src/vouch/server.py`:
- Around line 103-124: The code only applies min_score to
index_db.search_semantic (embedding) but ignores it for index_db.search (fts5)
and store.search_substring (substring); update the search branches so that after
obtaining hits from index_db.search and store.search_substring you filter hits
by the same min_score threshold (e.g., keep hits where hit.score or hit["score"]
>= min_score) before calling _to_dicts, and preserve the existing behavior of
returning _to_dicts([], backend_name) when backend is explicitly requested and
no hits remain; reference the functions index_db.search_semantic,
index_db.search, store.search_substring and the helper _to_dicts when making the
change.
🪄 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: 9b1443f7-eb3e-4a71-bde8-81bcd005e914
📒 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/bundle.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fastembed_bge.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_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
+217
to
+227
| | `tests/test_embeddings_core.py` | Embedder ABC, registry, content-hash skip, batched encode, lazy load | | ||
| | `tests/test_embeddings_storage.py` | vec0 + sqlite-vec round trip + NumPy fallback parity | | ||
| | `tests/test_embeddings_search.py` | Semantic primary, FTS5 fallback, lexical-disjoint regression | | ||
| | `tests/test_embeddings_fusion.py` | RRF, weighted, normalized fusion strategies correctness | | ||
| | `tests/test_embeddings_rerank.py` | Cross-encoder rerank changes top-K order on a known pair | | ||
| | `tests/test_embeddings_hyde.py` | HyDE expansion improves recall on terse queries | | ||
| | `tests/test_embeddings_dedup.py` | Threshold ledger + audit event | | ||
| | `tests/test_embeddings_migration.py` | Model-version mismatch + backfill flow | | ||
| | `tests/test_embeddings_eval.py` | recall@k / MRR / nDCG correctness on synthetic ground truth | | ||
| | `tests/test_embeddings_cli.py` | CLI flag routing | | ||
|
|
There was a problem hiding this comment.
Test plan paths don’t match the current test directory layout.
The table lists tests/test_embeddings_*.py, but this PR’s files are under tests/embeddings/test_*.py. The doc checklist should mirror actual paths.
📝 Suggested patch
-| `tests/test_embeddings_core.py` | Embedder ABC, registry, content-hash skip, batched encode, lazy load |
-| `tests/test_embeddings_storage.py` | vec0 + sqlite-vec round trip + NumPy fallback parity |
-| `tests/test_embeddings_search.py` | Semantic primary, FTS5 fallback, lexical-disjoint regression |
-| `tests/test_embeddings_fusion.py` | RRF, weighted, normalized fusion strategies correctness |
-| `tests/test_embeddings_rerank.py` | Cross-encoder rerank changes top-K order on a known pair |
-| `tests/test_embeddings_hyde.py` | HyDE expansion improves recall on terse queries |
-| `tests/test_embeddings_dedup.py` | Threshold ledger + audit event |
-| `tests/test_embeddings_migration.py` | Model-version mismatch + backfill flow |
-| `tests/test_embeddings_eval.py` | recall@k / MRR / nDCG correctness on synthetic ground truth |
-| `tests/test_embeddings_cli.py` | CLI flag routing |
+| `tests/embeddings/test_core.py` | Embedder ABC, registry, content-hash skip, batched encode, lazy load |
+| `tests/embeddings/test_storage.py` | vec0 + sqlite-vec round trip + NumPy fallback parity |
+| `tests/embeddings/test_search.py` | Semantic primary, FTS5 fallback, lexical-disjoint regression |
+| `tests/embeddings/test_fusion.py` | RRF, weighted, normalized fusion strategies correctness |
+| `tests/embeddings/test_rerank.py` | Cross-encoder rerank changes top-K order on a known pair |
+| `tests/embeddings/test_hyde.py` | HyDE expansion improves recall on terse queries |
+| `tests/embeddings/test_dedup.py` | Threshold ledger + audit event |
+| `tests/embeddings/test_migration.py` | Model-version mismatch + backfill flow |
+| `tests/embeddings/test_eval.py` | recall@k / MRR / nDCG correctness on synthetic ground truth |
+| `tests/embeddings/test_cli.py` | CLI flag routing |🤖 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 217
- 227, The test file paths in the checklist are incorrect; update each table row
to reference the actual locations under the embeddings subdirectory (e.g.
replace tests/test_embeddings_core.py -> tests/embeddings/test_core.py,
tests/test_embeddings_storage.py -> tests/embeddings/test_storage.py,
tests/test_embeddings_search.py -> tests/embeddings/test_search.py,
tests/test_embeddings_fusion.py -> tests/embeddings/test_fusion.py,
tests/test_embeddings_rerank.py -> tests/embeddings/test_rerank.py,
tests/test_embeddings_hyde.py -> tests/embeddings/test_hyde.py,
tests/test_embeddings_dedup.py -> tests/embeddings/test_dedup.py,
tests/test_embeddings_migration.py -> tests/embeddings/test_migration.py,
tests/test_embeddings_eval.py -> tests/embeddings/test_eval.py,
tests/test_embeddings_cli.py -> tests/embeddings/test_cli.py) so the doc
checklist mirrors the repo layout.
Comment on lines
+335
to
+343
| try: | ||
| from .embeddings import get_embedder | ||
| from .embeddings.cache import cache_query_vec, lookup_query_vec | ||
| except ImportError: | ||
| return [] | ||
| try: | ||
| embedder = get_embedder() | ||
| except KeyError: | ||
| return [] |
There was a problem hiding this comment.
Log semantic-path failures before returning empty hits.
search_semantic returns [] on embedder import/lookup failures with no diagnostics, which makes auto fallback behavior hard to debug in production.
💡 Suggested patch
+import logging
@@
+logger = logging.getLogger(__name__)
@@
def search_semantic(
@@
- except ImportError:
+ except ImportError as exc:
+ logger.warning("Semantic search unavailable; falling back to lexical backend", exc_info=exc)
return []
@@
- except KeyError:
+ except KeyError:
+ logger.warning("No embedder configured; falling back to lexical backend")
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/index_db.py` around lines 335 - 343, When import/lookup of the
embedder fails in search_semantic, don't silently return []; instead catch the
ImportError/KeyError and log the exception and contextual info before returning.
Update the try/except blocks around get_embedder and the imports (get_embedder,
cache_query_vec, lookup_query_vec) to emit a diagnostic message (using the
module logger or logging.getLogger(__name__)) that includes the exception text
and a short note that semantic search fell back to returning empty hits, then
return [].
Comment on lines
+99
to
+106
| if backend_arg == "hybrid": | ||
| from .embeddings.fusion import ( | ||
| rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore] | ||
| ) | ||
| emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2) | ||
| fts = index_db.search(s.kb_dir, q, limit=limit * 2) | ||
| hits = rrf_fuse(emb, fts, limit=limit) | ||
| used = "hybrid" |
There was a problem hiding this comment.
Hybrid backend import is brittle when fusion extras are unavailable.
backend="hybrid" can raise ImportError at runtime if embeddings.fusion isn’t present yet, turning a valid request into an internal error instead of a controlled fallback/error.
Suggested fix
if backend_arg == "hybrid":
- from .embeddings.fusion import (
- rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore]
- )
+ try:
+ from .embeddings.fusion import rrf_fuse # type: ignore[import-not-found,import-untyped]
+ except ImportError as e:
+ raise ValueError("hybrid backend is unavailable in this build") from e
emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2)
fts = index_db.search(s.kb_dir, q, limit=limit * 2)
hits = rrf_fuse(emb, fts, limit=limit)
used = "hybrid"📝 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
| if backend_arg == "hybrid": | |
| from .embeddings.fusion import ( | |
| rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore] | |
| ) | |
| emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2) | |
| fts = index_db.search(s.kb_dir, q, limit=limit * 2) | |
| hits = rrf_fuse(emb, fts, limit=limit) | |
| used = "hybrid" | |
| if backend_arg == "hybrid": | |
| try: | |
| from .embeddings.fusion import rrf_fuse # type: ignore[import-not-found,import-untyped] | |
| except ImportError as e: | |
| raise ValueError("hybrid backend is unavailable in this build") from e | |
| emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2) | |
| fts = index_db.search(s.kb_dir, q, limit=limit * 2) | |
| hits = rrf_fuse(emb, fts, limit=limit) | |
| used = "hybrid" |
🤖 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 - 106, The hybrid branch currently
does a direct import of rrf_fuse which can raise ImportError at runtime; wrap
the import and fusion call in a try/except ImportError around the backend_arg ==
"hybrid" block, and on ImportError fall back to a graceful alternative (e.g.,
use index_db.search_semantic or index_db.search alone, set used to "semantic" or
"fulltext" as appropriate, and log the import failure), otherwise proceed to
call rrf_fuse(emb, fts, limit=limit) and set used="hybrid"; locate the code
around the backend_arg check and symbols rrf_fuse, index_db.search_semantic,
index_db.search and ensure the exception handler does not expose a 500 stack
trace but returns/records a controlled fallback or error.
Comment on lines
+112
to
+120
| if backend in ("auto", "fts5"): | ||
| try: | ||
| hits = index_db.search(store.kb_dir, query, limit=limit) | ||
| except Exception: | ||
| hits = [] | ||
| if hits: | ||
| return _to_dicts(hits, "fts5") | ||
| if backend == "fts5": | ||
| return _to_dicts([], "fts5") |
There was a problem hiding this comment.
Log when falling back from FTS5 errors.
Lines 113-116 silently catch all exceptions from index_db.search and return an empty result, making it difficult to diagnose FTS5 configuration issues, schema problems, or transient failures. As noted in the PR objectives review feedback, operators would benefit from an audit/log line when the fallback occurs due to errors.
📝 Suggested logging addition
if backend in ("auto", "fts5"):
try:
hits = index_db.search(store.kb_dir, query, limit=limit)
- except Exception:
+ except Exception as exc:
+ # Log the fallback to aid debugging FTS5 issues
+ import logging
+ logging.getLogger(__name__).warning(
+ "FTS5 search failed, falling back: %s", exc
+ )
hits = []📝 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
| if backend in ("auto", "fts5"): | |
| try: | |
| hits = index_db.search(store.kb_dir, query, limit=limit) | |
| except Exception: | |
| hits = [] | |
| if hits: | |
| return _to_dicts(hits, "fts5") | |
| if backend == "fts5": | |
| return _to_dicts([], "fts5") | |
| if backend in ("auto", "fts5"): | |
| try: | |
| hits = index_db.search(store.kb_dir, query, limit=limit) | |
| except Exception as exc: | |
| # Log the fallback to aid debugging FTS5 issues | |
| import logging | |
| logging.getLogger(__name__).warning( | |
| "FTS5 search failed, falling back: %s", exc | |
| ) | |
| hits = [] | |
| if hits: | |
| return _to_dicts(hits, "fts5") | |
| if backend == "fts5": | |
| return _to_dicts([], "fts5") |
🤖 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 112 - 120, The try/except around
index_db.search silently swallows errors; update the exception handler in the
block that calls index_db.search(store.kb_dir, query, limit=limit) to log the
failure (including exception details and context like store.kb_dir, backend and
query) before falling back to hits = [] so operators can audit FTS5 errors; use
the module logger (or logger.exception / logger.error(..., exc_info=True)) and
keep the existing return paths that call _to_dicts so behavior doesn't change.
Comment on lines
+126
to
+133
| if backend == "hybrid": | ||
| from .embeddings.fusion import ( | ||
| rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore] | ||
| ) | ||
| emb = index_db.search_semantic(store.kb_dir, query, limit=limit * 2) | ||
| fts = index_db.search(store.kb_dir, query, limit=limit * 2) | ||
| hits = rrf_fuse(emb, fts, limit=limit) | ||
| return _to_dicts(hits, "hybrid") |
There was a problem hiding this comment.
Defensively guard the hybrid fusion import.
The hybrid backend imports embeddings.fusion.rrf_fuse from Phase 5, which may not exist if Phase 5 hasn't been merged. When backend="hybrid" is requested before Phase 5 is available, this will raise ModuleNotFoundError and break the entire request.
As noted in the PR objectives review feedback, wrap this import defensively (as was done in Phase 3 for similar optional imports) to provide a clear error message when the feature isn't available yet.
🛡️ Recommended defensive pattern
if backend == "hybrid":
+ try:
- from .embeddings.fusion import (
- rrf_fuse, # type: ignore[import-not-found,import-untyped,unused-ignore]
- )
+ from .embeddings.fusion import rrf_fuse # type: ignore[import-untyped]
+ except ImportError as exc:
+ raise ValueError(
+ "hybrid backend requires embeddings.fusion module (Phase 5)"
+ ) from exc
emb = index_db.search_semantic(store.kb_dir, query, limit=limit * 2)🤖 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, The hybrid branch imports
embeddings.fusion.rrf_fuse unguarded so a missing Phase 5 module will raise
ModuleNotFoundError; wrap the import of rrf_fuse in a try/except ImportError
inside the backend == "hybrid" block (around the current from .embeddings.fusion
import rrf_fuse), and if the import fails raise or return a clear error (e.g.,
RuntimeError with a message that the hybrid fusion feature is not available yet)
so the call to index_db.search_semantic / index_db.search and subsequent use of
rrf_fuse and _to_dicts won't crash unexpectedly.
Comment on lines
+112
to
+114
| assert resp["result"] | ||
| assert resp["result"][0]["backend"] in ("embedding", "fts5", "substring") | ||
|
|
There was a problem hiding this comment.
Test is pinning the old JSONL wire format.
This assertion treats resp["result"] as a list; Phase 4 contract should be dict-shaped (backend + hits). Please assert the new structure so regressions are caught.
Suggested fix
- assert resp["result"]
- assert resp["result"][0]["backend"] in ("embedding", "fts5", "substring")
+ assert isinstance(resp["result"], dict)
+ assert "backend" in resp["result"]
+ assert isinstance(resp["result"].get("hits"), list)
+ assert resp["result"]["backend"] in ("embedding", "fts5", "substring")📝 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
| assert resp["result"] | |
| assert resp["result"][0]["backend"] in ("embedding", "fts5", "substring") | |
| assert isinstance(resp["result"], dict) | |
| assert "backend" in resp["result"] | |
| assert isinstance(resp["result"].get("hits"), list) | |
| assert resp["result"]["backend"] in ("embedding", "fts5", "substring") |
🤖 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 112 - 114, The test currently
treats resp["result"] as a list (old JSONL wire format); change assertions to
validate the Phase 4 dict-shaped contract: assert that resp["result"] is a dict,
that it contains keys "backend" and "hits", that resp["result"]["backend"] is
one of ("embedding","fts5","substring"), and that resp["result"]["hits"] is a
list (and optionally assert hits non-empty or inspect hits[0] shape if
required). Update the assertions around resp["result"] accordingly (look for
resp["result"] usage in the test function) so regressions to the old list format
are caught.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-4-read-integration
branch
from
May 21, 2026 05:36
be5c1ab to
9e77d51
Compare
There was a problem hiding this comment.
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)
126-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ruff I001 import formatting error and add defensive guard.
The pipeline is failing with ruff I001 (import un-sorted/un-formatted) on line 127. Additionally, as noted in prior review feedback, the import should be wrapped defensively to prevent
ModuleNotFoundErrorwhen Phase 5 isn't available.Simplifying the type ignore and adding the defensive guard addresses both the pipeline failure and the robustness concern:
🔧 Proposed fix
if backend == "hybrid": - from .embeddings.fusion import rrf_fuse # type: ignore[import-not-found,import-untyped,unused-ignore] + try: + from .embeddings.fusion import rrf_fuse # type: ignore[import-untyped] + except ImportError as exc: + raise ValueError( + "hybrid backend requires embeddings.fusion module (Phase 5)" + ) from exc emb = index_db.search_semantic(store.kb_dir, query, limit=limit * 2)🤖 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 - 131, The import of rrf_fuse in the backend == "hybrid" branch needs to be sorted/formatted and wrapped defensively to avoid ModuleNotFoundError: replace the inline from .embeddings.fusion import rrf_fuse with a try/except ImportError that attempts the import and sets rrf_fuse = None (or raises a clear error) so the code can guard; then check rrf_fuse before calling it and handle the missing-fusion case (e.g., fallback or return) before calling index_db.search_semantic, index_db.search, rrf_fuse, and _to_dicts to avoid runtime failures and satisfy ruff import formatting rules.src/vouch/jsonl_server.py (1)
99-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ruff I001 import formatting error and add defensive guard.
Same issue as
server.py: pipeline fails with ruff I001 on line 100, and the import lacks defensive error handling for when the fusion module isn't available.🔧 Proposed fix
if backend_arg == "hybrid": - from .embeddings.fusion import rrf_fuse # type: ignore[import-not-found,import-untyped,unused-ignore] + try: + from .embeddings.fusion import rrf_fuse # type: ignore[import-untyped] + except ImportError as exc: + raise ValueError("hybrid backend requires embeddings.fusion module") from exc emb = index_db.search_semantic(s.kb_dir, q, limit=limit * 2)🤖 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 - 104, Replace the bare inline import of rrf_fuse inside the backend_arg == "hybrid" block with a defensive try/except ImportError: attempt to import .embeddings.fusion.rrf_fuse (symbol: rrf_fuse) and on ImportError either raise a clear RuntimeError (or ValueError) saying the fusion module is unavailable or set a sentinel and fall back to a non-hybrid path; then only call rrf_fuse when the import succeeded (guard around the call to rrf_fuse(emb, fts, limit=limit)) so index_db.search_semantic and index_db.search are not used for hybrid unless rrf_fuse is present.
🤖 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.
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 99-104: Replace the bare inline import of rrf_fuse inside the
backend_arg == "hybrid" block with a defensive try/except ImportError: attempt
to import .embeddings.fusion.rrf_fuse (symbol: rrf_fuse) and on ImportError
either raise a clear RuntimeError (or ValueError) saying the fusion module is
unavailable or set a sentinel and fall back to a non-hybrid path; then only call
rrf_fuse when the import succeeded (guard around the call to rrf_fuse(emb, fts,
limit=limit)) so index_db.search_semantic and index_db.search are not used for
hybrid unless rrf_fuse is present.
In `@src/vouch/server.py`:
- Around line 126-131: The import of rrf_fuse in the backend == "hybrid" branch
needs to be sorted/formatted and wrapped defensively to avoid
ModuleNotFoundError: replace the inline from .embeddings.fusion import rrf_fuse
with a try/except ImportError that attempts the import and sets rrf_fuse = None
(or raises a clear error) so the code can guard; then check rrf_fuse before
calling it and handle the missing-fusion case (e.g., fallback or return) before
calling index_db.search_semantic, index_db.search, rrf_fuse, and _to_dicts to
avoid runtime failures and satisfy ruff import formatting rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a87c25ce-92ca-4528-ad6f-3412f3a3f0a3
📒 Files selected for processing (2)
src/vouch/jsonl_server.pysrc/vouch/server.py
plind-junior
added a commit
that referenced
this pull request
May 22, 2026
…ad-integration feat(embeddings): semantic-primary kb.search and JSONL parity
This was referenced May 23, 2026
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 4: turn semantic retrieval ON.
kb.search(MCP) andkb.search(JSONL) now default to embedding-first, FTS5 fallback, substring last-resort. This is the headline behavior change of the semantic-search feature: any caller passing nobackendargument switches from BM25 to dense retrieval transparently.Tracks Tasks 14-16 in the plan.
Commits
910191dindex_db.search_semantic(kb_dir, query, limit, kinds, min_score)— query-cached wrapper that encodes the query (skipping re-encode via the LRU cache from Phase 2) then dispatches tosearch_embedding775a711server.kb_searchbecomes the dispatcher:backend="auto"(default) tries embedding → fts5 → substring; explicitbackend="embedding"|"fts5"|"substring"|"hybrid"is also supportede8b7f02jsonl_server._h_searchmirrors the MCP dispatch 1:1 — same param names, same return shapeDesign notes
Auto-fallback is silent and ordered.
autoreturns the first non-empty result, tagging the response with thebackendthat produced it. Explicitbackend=...constrains to a single path. Thehybridbranch importsembeddings.fusion.rrf_fuse— that module lands in Phase 5; until then, the import is only reached when callers explicitly request hybrid.Query embedding cache is wired up.
search_semanticcallslookup_query_vecfirst, encodes only on miss, thencache_query_vec. Repeated queries skip the encode entirely.Behavior change to note
kb_search's signature changed from(query, limit)(positional) to(query, *, limit, backend, min_score)(keyword-only after the query). Any external caller passinglimitpositionally needs to switch to keyword. No in-tree callers are affected (verified by the existing test suite).Test Plan
.venv/bin/pytest tests/embeddings -v→ 30 passed, 4 deselected (3 new search-integration tests).venv/bin/pytest --ignore=tests/test_sessions.py -q→ 93 passed.venv/bin/python -m ruff check src/vouch tests/embeddings→ cleanWhat's NOT in this PR
embeddings.fusion).build_context_packstill uses FTS5 — Phase 7.vouch searchCLI still uses the old flag surface — Phase 8.Summary by CodeRabbit
New Features
Documentation
Tests
Chores