feat(embeddings): CLI sweep, MCP/JSONL parity, integration test, docs#44
Merged
plind-junior merged 15 commits intoMay 25, 2026
Merged
Conversation
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds embedding-aware search operations to the knowledge base server. It introduces CLI maintenance commands, server tools for embeddings (reindex, dedup scan, eval, stats), updates the context pack to return a dictionary with optional explain field showing per-hit backend labels, and adds graceful fallback when embedding dependencies are unavailable. ChangesEmbedding Semantic Search Operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
clickdependency declarations.Lines 21 and 22 both declare
clickwith different version constraints (>=8.4.0,<9vs>=8.1,<9). This appears to be a merge artifact. Keep only the intended constraint.Proposed fix
dependencies = [ "pydantic>=2.13.4,<3", - "click>=8.4.0,<9", "click>=8.1,<9", "pyyaml>=6,<7", "mcp>=1.0,<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 `@pyproject.toml` around lines 21 - 22, The dependencies list contains duplicate "click" entries with conflicting version specs ("click>=8.4.0,<9" and "click>=8.1,<9"); remove the unintended duplicate so only the correct constraint remains (e.g., keep "click>=8.4.0,<9" or whichever constraint the project intends) and update the dependencies section to have a single "click" entry.
🧹 Nitpick comments (5)
src/vouch/embeddings/migration.py (1)
38-80: 💤 Low valueOverly broad
AttributeErrorexception handling.Catching
AttributeErrorwill swallow any attribute access error within the loop body (e.g., typos inc.text,p.title, etc.), not just missinglist_*methods. Consider catching more narrowly or checkinghasattr(store, 'list_claims')before iterating.Example narrower approach
- try: - for c in store.list_claims(): - store._embed_and_store(kind="claim", id=c.id, text=c.text) - touched += 1 - except AttributeError: - pass + if hasattr(store, 'list_claims'): + for c in store.list_claims(): + store._embed_and_store(kind="claim", id=c.id, text=c.text) + touched += 1🤖 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/migration.py` around lines 38 - 80, The code is catching AttributeError too broadly around each processing loop (e.g., in the blocks using store.list_claims, list_pages, list_sources, list_entities, list_relations, list_evidence and calls to store._embed_and_store), which can hide bugs like typos in properties (c.text, p.title, etc.); fix by guarding each loop with a check like hasattr(store, "list_claims") (or using getattr(store, "list_claims", None) and skip if None) before iterating, or narrow the try/except to only wrap the call to the listing method itself so that errors inside the loop surface; update each block referencing store.list_claims, store.list_pages, store.list_sources, store.list_entities, store.list_relations, and store.list_evidence accordingly.tests/embeddings/test_storage.py (1)
138-153: 💤 Low valueTest name is misleading.
test_search_works_under_both_backendsonly runs the search once with whichever backend is available (sqlite-vec or fallback), not both. Consider renaming totest_search_works_with_available_backendor adding explicit backend toggling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/embeddings/test_storage.py` around lines 138 - 153, The test named test_search_works_under_both_backends is misleading because it only exercises whichever backend is active; either rename the test to something like test_search_works_with_available_backend or modify it to explicitly run the same logic against both backends by parameterizing or looping over backends (e.g., toggling sqlite-vec vs fallback) so the calls to index_db.open_db, index_db.put_embedding and index_db.search_embedding are executed for each backend; update the test name and/or add a simple backend loop to ensure both backends are actually tested.src/vouch/server.py (1)
112-120: ⚡ Quick winBare
except Exceptionswallows unexpected errors.Catching all exceptions masks programming errors and makes debugging harder. Consider catching only the expected FTS5 errors.
Proposed fix
if backend in ("auto", "fts5"): try: hits = index_db.search(store.kb_dir, query, limit=limit) - except Exception: + except (sqlite3.OperationalError, sqlite3.DatabaseError): hits = []You'll need to add
import sqlite3at the top of the file.🤖 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 currently swallows all exceptions; change it to catch only the expected SQLite FTS errors (e.g., catch sqlite3.Error or sqlite3.OperationalError) so programming errors still surface, and add the required import sqlite3 at the top of the file; keep the same fallback behavior (set hits = [] on sqlite errors) and leave the rest of the logic using backend, index_db.search, and _to_dicts unchanged.tests/test_context.py (2)
63-76: 💤 Low valueRemove redundant imports and registration.
The
autouse=Truefixture at line 17-19 already registersMockEmbedderforDEFAULT_MODEL_NAME. The duplicate imports andregister()call inside this test are unnecessary.Proposed cleanup
def test_build_context_pack_uses_semantic_default(tmp_path: Path) -> None: - from tests.embeddings._fakes import MockEmbedder - from vouch.context import build_context_pack - from vouch.embeddings import register - from vouch.embeddings.base import DEFAULT_MODEL_NAME - from vouch.models import Claim - from vouch.storage import KBStore - - register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) store = KBStore.init(tmp_path) src = store.put_source(b"e") store.put_claim(Claim(id="c1", text="exact query string", evidence=[src.id])) pack = build_context_pack(store, query="exact query string", limit=5) assert any(item["id"] == "c1" for item in pack.get("items", []))The necessary imports are already at the module level.
🤖 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/test_context.py` around lines 63 - 76, The test test_build_context_pack_uses_semantic_default contains redundant imports and an extra register(DEFAULT_MODEL_NAME, ...) call; remove the local imports of MockEmbedder, register, and DEFAULT_MODEL_NAME and delete the register(...) invocation so the test relies on the autouse fixture that already registers MockEmbedder for DEFAULT_MODEL_NAME; keep the rest of the test (store init, put_source/put_claim, build_context_pack invocation and assertion) unchanged.
79-95: 💤 Low valueSame redundant imports and registration as above.
This test also duplicates imports and the
register()call that the autouse fixture already handles.🤖 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/test_context.py` around lines 79 - 95, Remove the redundant imports and registration in test_build_context_pack_explain_flag_returns_score_breakdown: delete the explicit imports of MockEmbedder, register, and DEFAULT_MODEL_NAME and remove the register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(...)) call so the autouse fixture handles embedder registration; keep the rest of the test (KBStore usage, put_source/put_claim, build_context_pack and assertions) unchanged and reference the test function name and the register/MockEmbedder/DEFAULT_MODEL_NAME symbols to locate the lines to remove.
🤖 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/retrieval.md`:
- Around line 121-123: The earlier “not embeddings (yet)” section conflicts with
the later “Semantic retrieval” section; update that older paragraph (the one
containing the phrase "not embeddings (yet)") to reflect that semantic
retrieval/embeddings are now supported and either remove the "not embeddings
(yet)" wording or replace it with a short sentence that points readers to the
"Semantic retrieval" section and the embeddings.md link; ensure the headings
"Semantic retrieval" and the reference to embeddings.md remain consistent so
users see one unified retrieval story.
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 33-47: Add a language tag to the fenced code block that lists the
"src/vouch/embeddings/" file tree: change the opening ``` to ```text (or another
language), so the block becomes a labeled fenced code block (e.g., ```text) to
satisfy markdownlint MD040; locate the fenced block containing the
"src/vouch/embeddings/" listing and update its opening backticks accordingly.
In `@src/vouch/capabilities.py`:
- Around line 56-59: The capabilities() function currently returns only lexical
retrieval modes ("fts5", "substring") despite adding embedding-related
capability strings ("kb.reindex_embeddings", "kb.dedup_scan",
"kb.eval_embeddings", "kb.embeddings_stats"); update capabilities() to detect
the presence of these embedding capability names and include the appropriate
semantic retrieval modes (e.g., "embeddings" or "hybrid") in the returned list
so clients reading kb.capabilities can correctly detect semantic/hybrid support;
refer to the capabilities() function and the newly added capability names when
implementing the conditional inclusion.
In `@src/vouch/cli.py`:
- Around line 636-657: The reindex command validates a specific embedder with
get_embedder(model) but doesn't pass the selected model/embedder to
backfill_embeddings, so backfill runs with the default; fix by retrieving the
embedder or model name in reindex (via get_embedder(model) or keeping model) and
pass that through to backfill_embeddings(store, force=force, model=<selected>)
(or the embedding instance if backfill_embeddings accepts it) so the requested
model is actually used; update the reindex function to forward the validated
model/embedder to backfill_embeddings similar to kb_reindex_embeddings.
In `@src/vouch/context.py`:
- Around line 65-67: The code in context.py that returns e.name or
e.description[:200] can raise TypeError when e.description is None; update the
logic after store.get_entity(artifact_id) (variable e) to safely handle a
missing description by using a safe fallback before slicing (e.g., treat
e.description as an empty string when None) so the expression becomes equivalent
to e.name or (e.description or "")[:200]; keep the existing short-circuit
behavior and no-op if e is None handled elsewhere.
In `@src/vouch/embeddings/base.py`:
- Line 17: CI type-check fails because NumPy is imported in the embeddings
modules but not present in dev dependencies; either add "numpy" to dev
dependencies (so pip install -e '.[dev]' provides it) or add mypy overrides to
ignore missing NumPy imports for the affected modules (e.g., add mypy config
sections such as [tool.mypy-src.vouch.embeddings.base],
[tool.mypy-src.vouch.embeddings.st_mpnet],
[tool.mypy-src.vouch.embeddings.st_minilm],
[tool.mypy-src.vouch.embeddings.dedup], [tool.mypy-src.vouch.embeddings.cache],
[tool.mypy-src.vouch.embeddings.fastembed_bge] with ignore_missing_imports =
true) to stop type-check errors for imports in base.py, st_mpnet.py,
st_minilm.py, dedup.py, cache.py, and fastembed_bge.py.
In `@src/vouch/embeddings/cache.py`:
- Line 10: The unconditional top-level "import numpy as np" in
src/vouch/embeddings/cache.py causes import-time failures when the optional
embeddings extras aren't installed; remove the global import and instead perform
a local import of numpy inside the functions that actually need it (wrap each
local import in try/except ImportError), then either raise a clear RuntimeError
explaining that embeddings extras are required or implement graceful degradation
behavior. Update references to "np" in functions within cache.py so they use the
locally imported symbol, and ensure any public functions in cache.py document or
raise the informative error when numpy is unavailable.
In `@src/vouch/embeddings/hyde.py`:
- Around line 20-27: The function expand_query_with_llm should catch errors from
the LLM call so failures degrade to the original query: wrap the call to
llm(prompt) in a try/except (catching Exception), on success return
llm(prompt).strip() or query as before, and on exception return the original
query; reference the expand_query_with_llm function and the llm(prompt)
invocation (and the prompt variable) when making the change.
In `@src/vouch/embeddings/migration.py`:
- Line 34: The backfill_embeddings function currently declares but doesn't use
the force parameter; update backfill_embeddings to make force=True bypass the
existing content-hash check so embeddings are recomputed and written regardless
of stored hashes. Concretely: inside backfill_embeddings (and the loop that
checks whether to skip a document via its stored content_hash), add a
conditional so that when force is True you skip the "if content_hash_matches:
continue" branch and always call the embedding computation and upsert functions
(e.g., compute_embedding, upsert_embeddings or whatever the function uses); if
you prefer to remove the parameter instead, delete force from the signature and
any callers. Also update the function docstring/comments to reflect the behavior
change.
In `@src/vouch/embeddings/rerank.py`:
- Around line 44-47: The list comprehension that builds reranked uses zip(hits,
scores, strict=False) which silently truncates when scores and hits lengths
differ; update the reranker logic in rerank.py to detect a length mismatch
between hits and scores (check len(hits) vs len(scores)) and either raise an
explicit error or log a clear warning before proceeding, or replace zip(...,
strict=False) with zip(..., strict=True) if running on Python 3.10+; ensure you
reference the reranked variable and the inputs hits and scores so the code fails
loudly instead of dropping candidates silently.
In `@src/vouch/embeddings/st_minilm.py`:
- Line 11: Add a mypy type-ignore to the numpy import so type-checking won't
fail when optional deps aren't installed: modify the `import numpy as np`
statement in `st_minilm.py` to include `# type: ignore[import-not-found]`,
mirroring the existing pattern used for `sentence_transformers` (the import of
`SentenceTransformer`) to suppress the missing-import error.
In `@src/vouch/index_db.py`:
- Around line 186-192: Mypy is failing due to missing numpy stubs: update the
two dynamic imports of numpy inside the helper functions by adding a mypy ignore
for the import-not-found error (i.e., add "# type: ignore[import-not-found]" to
the "import numpy as np" lines in the functions _vec_to_blob and _blob_to_vec),
and apply the same change to the analogous import in the search_embedding helper
at the other location mentioned so mypy no longer flags numpy as missing while
keeping numpy as an optional extras dependency.
In `@src/vouch/jsonl_server.py`:
- Around line 99-104: The hybrid branch (when backend_arg == "hybrid") calls
index_db.search_semantic and index_db.search without guarding the FTS leg, so an
exception from index_db.search can abort the whole request; wrap the FTS call
(index_db.search) in a try/except and on failure fall back to a safe value
(e.g., empty hits or skip fusion) before calling rrf_fuse so rrf_fuse(emb, fts,
limit=limit) always gets valid inputs; update local variables (fts, hits, used)
accordingly and preserve the existing behavior of returning hybrid when both
legs succeed.
- Around line 425-433: _h_eval_embeddings ignores caller-selected metrics;
modify it to extract the metric(s) from the incoming dict p (e.g.,
p.get("metric") or p.get("metrics")) and pass that value into the evaluate(...)
call so evaluate receives the requested metric selection. Ensure you convert or
normalize the incoming value to the expected evaluate parameter type (string or
list) before forwarding, and keep existing parameters kb_dir=_store().kb_dir,
queries_file=Path(p["queries_path"]), and k=int(p.get("k", 10)).
In `@src/vouch/server.py`:
- Around line 584-595: The kb_reindex_embeddings function validates a provided
model by calling get_embedder(model) but never uses that embedder when invoking
backfill_embeddings, so the model parameter is ineffective; fix this by either
removing the model parameter or (preferred) updating kb_reindex_embeddings to
pass the chosen embedder (or model name) into backfill_embeddings so it
re-encodes with the requested model—locate kb_reindex_embeddings, the
get_embedder call, and backfill_embeddings and modify the call/signature so
backfill_embeddings receives and uses the validated model/embedder, and ensure
the returned "model" in the dict reflects _current_model_name() or the specified
model accordingly.
In `@tests/embeddings/test_core.py`:
- Around line 56-60: The test_registry_round_trip mutates the global registry by
calling register("test-adapter", ...) which can leak state between tests; update
test_registry_round_trip to use a unique adapter key (e.g., append a UUID or
timestamp) when calling register or ensure the registry is restored after the
test (unregister or snapshot/restore) so other tests are unaffected; locate the
calls to register and get_embedder in test_registry_round_trip and change the
adapter name generation or add teardown logic to remove the registered
"test-adapter" entry (or restore previous registry state) so the mutation is
isolated.
---
Outside diff comments:
In `@pyproject.toml`:
- Around line 21-22: The dependencies list contains duplicate "click" entries
with conflicting version specs ("click>=8.4.0,<9" and "click>=8.1,<9"); remove
the unintended duplicate so only the correct constraint remains (e.g., keep
"click>=8.4.0,<9" or whichever constraint the project intends) and update the
dependencies section to have a single "click" entry.
---
Nitpick comments:
In `@src/vouch/embeddings/migration.py`:
- Around line 38-80: The code is catching AttributeError too broadly around each
processing loop (e.g., in the blocks using store.list_claims, list_pages,
list_sources, list_entities, list_relations, list_evidence and calls to
store._embed_and_store), which can hide bugs like typos in properties (c.text,
p.title, etc.); fix by guarding each loop with a check like hasattr(store,
"list_claims") (or using getattr(store, "list_claims", None) and skip if None)
before iterating, or narrow the try/except to only wrap the call to the listing
method itself so that errors inside the loop surface; update each block
referencing store.list_claims, store.list_pages, store.list_sources,
store.list_entities, store.list_relations, and store.list_evidence accordingly.
In `@src/vouch/server.py`:
- Around line 112-120: The try/except around index_db.search currently swallows
all exceptions; change it to catch only the expected SQLite FTS errors (e.g.,
catch sqlite3.Error or sqlite3.OperationalError) so programming errors still
surface, and add the required import sqlite3 at the top of the file; keep the
same fallback behavior (set hits = [] on sqlite errors) and leave the rest of
the logic using backend, index_db.search, and _to_dicts unchanged.
In `@tests/embeddings/test_storage.py`:
- Around line 138-153: The test named test_search_works_under_both_backends is
misleading because it only exercises whichever backend is active; either rename
the test to something like test_search_works_with_available_backend or modify it
to explicitly run the same logic against both backends by parameterizing or
looping over backends (e.g., toggling sqlite-vec vs fallback) so the calls to
index_db.open_db, index_db.put_embedding and index_db.search_embedding are
executed for each backend; update the test name and/or add a simple backend loop
to ensure both backends are actually tested.
In `@tests/test_context.py`:
- Around line 63-76: The test test_build_context_pack_uses_semantic_default
contains redundant imports and an extra register(DEFAULT_MODEL_NAME, ...) call;
remove the local imports of MockEmbedder, register, and DEFAULT_MODEL_NAME and
delete the register(...) invocation so the test relies on the autouse fixture
that already registers MockEmbedder for DEFAULT_MODEL_NAME; keep the rest of the
test (store init, put_source/put_claim, build_context_pack invocation and
assertion) unchanged.
- Around line 79-95: Remove the redundant imports and registration in
test_build_context_pack_explain_flag_returns_score_breakdown: delete the
explicit imports of MockEmbedder, register, and DEFAULT_MODEL_NAME and remove
the register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(...)) call so the autouse
fixture handles embedder registration; keep the rest of the test (KBStore usage,
put_source/put_claim, build_context_pack and assertions) unchanged and reference
the test function name and the register/MockEmbedder/DEFAULT_MODEL_NAME symbols
to locate the lines to remove.
🪄 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: 1e861749-deb9-4cbb-8e80-6a55e62291e0
📒 Files selected for processing (40)
docs/README.mddocs/embeddings.mddocs/retrieval.mddocs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/dedup.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/hyde.pysrc/vouch/embeddings/migration.pysrc/vouch/embeddings/rerank.pysrc/vouch/embeddings/scorer.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pysrc/vouch/health.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/test_cli.pytests/embeddings/test_core.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.pytests/embeddings/test_integration.pytests/embeddings/test_migration.pytests/embeddings/test_rerank.pytests/embeddings/test_scorer.pytests/embeddings/test_search.pytests/embeddings/test_storage.pytests/test_context.py
Comment on lines
+121
to
+123
| ## Semantic retrieval | ||
|
|
||
| See [embeddings.md](./embeddings.md). |
There was a problem hiding this comment.
Resolve the doc contradiction with the earlier “not embeddings (yet)” section.
Lines 121-123 correctly point to semantic retrieval docs, but earlier text still states embeddings are future/planned. Please update that older section so users see one consistent retrieval story.
🤖 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/retrieval.md` around lines 121 - 123, The earlier “not embeddings (yet)”
section conflicts with the later “Semantic retrieval” section; update that older
paragraph (the one containing the phrase "not embeddings (yet)") to reflect that
semantic retrieval/embeddings are now supported and either remove the "not
embeddings (yet)" wording or replace it with a short sentence that points
readers to the "Semantic retrieval" section and the embeddings.md link; ensure
the headings "Semantic retrieval" and the reference to embeddings.md remain
consistent so users see one unified retrieval story.
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 tag to the fenced block at Line 33.
The code fence is unlabeled, which triggers markdownlint MD040. Add a language (for example text) to keep docs lint clean.
Proposed fix
-```
+```text
src/vouch/embeddings/
__init__.py # public API: encode, search, register
...
-```
+```📝 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
| ``` | |
| 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 | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md` around lines 33
- 47, Add a language tag to the fenced code block that lists the
"src/vouch/embeddings/" file tree: change the opening ``` to ```text (or another
language), so the block becomes a labeled fenced code block (e.g., ```text) to
satisfy markdownlint MD040; locate the fenced block containing the
"src/vouch/embeddings/" listing and update its opening backticks accordingly.
Comment on lines
+636
to
+657
| @cli.command() | ||
| @click.option("--embeddings/--no-embeddings", default=False, | ||
| help="Rebuild the embedding index in addition to FTS5.") | ||
| @click.option("--backfill/--no-backfill", default=False, | ||
| help="Re-encode every artifact under the current model.") | ||
| @click.option("--force/--no-force", default=False, | ||
| help="Re-encode even if content hash unchanged.") | ||
| @click.option("--model", default=None, | ||
| help="Adapter name; defaults to the registered default.") | ||
| def reindex(embeddings: bool, backfill: bool, force: bool, model: str | None) -> None: | ||
| """Rebuild derived indexes from on-disk artifacts.""" | ||
| store = _load_store() | ||
| health.rebuild_index(store) | ||
| if embeddings or backfill: | ||
| from .embeddings.migration import backfill_embeddings | ||
| if model: | ||
| from .embeddings import get_embedder | ||
| get_embedder(model) | ||
| n = backfill_embeddings(store, force=force) | ||
| click.echo(f"reindex: embeddings backfilled = {n}") | ||
| else: | ||
| click.echo("reindex: FTS5 rebuilt") |
There was a problem hiding this comment.
Same issue: model parameter validated but not passed to backfill.
Like kb_reindex_embeddings in server.py, the model parameter is validated via get_embedder(model) but the result is discarded. The backfill will use the default model regardless of what was specified.
Proposed fix
def reindex(embeddings: bool, backfill: bool, force: bool, model: str | None) -> None:
"""Rebuild derived indexes from on-disk artifacts."""
store = _load_store()
health.rebuild_index(store)
if embeddings or backfill:
from .embeddings.migration import backfill_embeddings
+ embedder = None
if model:
from .embeddings import get_embedder
- get_embedder(model)
- n = backfill_embeddings(store, force=force)
+ embedder = get_embedder(model)
+ n = backfill_embeddings(store, force=force, embedder=embedder)
click.echo(f"reindex: embeddings backfilled = {n}")📝 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
| @cli.command() | |
| @click.option("--embeddings/--no-embeddings", default=False, | |
| help="Rebuild the embedding index in addition to FTS5.") | |
| @click.option("--backfill/--no-backfill", default=False, | |
| help="Re-encode every artifact under the current model.") | |
| @click.option("--force/--no-force", default=False, | |
| help="Re-encode even if content hash unchanged.") | |
| @click.option("--model", default=None, | |
| help="Adapter name; defaults to the registered default.") | |
| def reindex(embeddings: bool, backfill: bool, force: bool, model: str | None) -> None: | |
| """Rebuild derived indexes from on-disk artifacts.""" | |
| store = _load_store() | |
| health.rebuild_index(store) | |
| if embeddings or backfill: | |
| from .embeddings.migration import backfill_embeddings | |
| if model: | |
| from .embeddings import get_embedder | |
| get_embedder(model) | |
| n = backfill_embeddings(store, force=force) | |
| click.echo(f"reindex: embeddings backfilled = {n}") | |
| else: | |
| click.echo("reindex: FTS5 rebuilt") | |
| `@cli.command`() | |
| `@click.option`("--embeddings/--no-embeddings", default=False, | |
| help="Rebuild the embedding index in addition to FTS5.") | |
| `@click.option`("--backfill/--no-backfill", default=False, | |
| help="Re-encode every artifact under the current model.") | |
| `@click.option`("--force/--no-force", default=False, | |
| help="Re-encode even if content hash unchanged.") | |
| `@click.option`("--model", default=None, | |
| help="Adapter name; defaults to the registered default.") | |
| def reindex(embeddings: bool, backfill: bool, force: bool, model: str | None) -> None: | |
| """Rebuild derived indexes from on-disk artifacts.""" | |
| store = _load_store() | |
| health.rebuild_index(store) | |
| if embeddings or backfill: | |
| from .embeddings.migration import backfill_embeddings | |
| embedder = None | |
| if model: | |
| from .embeddings import get_embedder | |
| embedder = get_embedder(model) | |
| n = backfill_embeddings(store, force=force, embedder=embedder) | |
| click.echo(f"reindex: embeddings backfilled = {n}") | |
| else: | |
| click.echo("reindex: FTS5 rebuilt") |
🤖 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/cli.py` around lines 636 - 657, The reindex command validates a
specific embedder with get_embedder(model) but doesn't pass the selected
model/embedder to backfill_embeddings, so backfill runs with the default; fix by
retrieving the embedder or model name in reindex (via get_embedder(model) or
keeping model) and pass that through to backfill_embeddings(store, force=force,
model=<selected>) (or the embedding instance if backfill_embeddings accepts it)
so the requested model is actually used; update the reindex function to forward
the validated model/embedder to backfill_embeddings similar to
kb_reindex_embeddings.
Comment on lines
+186
to
+192
| import numpy as np | ||
| return np.asarray(vec, dtype=np.float32).tobytes() | ||
|
|
||
|
|
||
| def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def] | ||
| import numpy as np | ||
| return np.frombuffer(blob, dtype=np.float32, count=dim).copy() |
There was a problem hiding this comment.
Address mypy import-not-found for numpy.
The pipeline is failing because mypy cannot find numpy stubs. Since numpy is an optional dependency (part of [embeddings] extras), add a type: ignore comment or ensure numpy-stubs is included in dev dependencies.
Proposed fix
def _vec_to_blob(vec): # type: ignore[no-untyped-def]
- import numpy as np
+ import numpy as np # type: ignore[import-not-found]
return np.asarray(vec, dtype=np.float32).tobytes()
def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def]
- import numpy as np
+ import numpy as np # type: ignore[import-not-found]
return np.frombuffer(blob, dtype=np.float32, count=dim).copy()Apply the same to line 284 in search_embedding.
📝 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
| import numpy as np | |
| return np.asarray(vec, dtype=np.float32).tobytes() | |
| def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def] | |
| import numpy as np | |
| return np.frombuffer(blob, dtype=np.float32, count=dim).copy() | |
| import numpy as np # type: ignore[import-not-found] | |
| return np.asarray(vec, dtype=np.float32).tobytes() | |
| def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def] | |
| import numpy as np # type: ignore[import-not-found] | |
| return np.frombuffer(blob, dtype=np.float32, count=dim).copy() |
🧰 Tools
🪛 GitHub Actions: ci / 1_test (py3.12).txt
[error] 186-186: mypy error: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / 2_test (py3.11).txt
[error] 186-186: mypy error: Cannot find implementation or library stub for module named "numpy". [import-not-found]
🪛 GitHub Actions: ci / 3_test (py3.13).txt
[error] 186-186: mypy: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / test (py3.11)
[error] 186-186: mypy import-not-found: Cannot find implementation or library stub for module named "numpy"
🪛 GitHub Actions: ci / test (py3.12)
[error] 186-186: mypy error: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / test (py3.13)
[error] 186-186: mypy import-not-found: Cannot find implementation or library stub for module named "numpy"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/index_db.py` around lines 186 - 192, Mypy is failing due to missing
numpy stubs: update the two dynamic imports of numpy inside the helper functions
by adding a mypy ignore for the import-not-found error (i.e., add "# type:
ignore[import-not-found]" to the "import numpy as np" lines in the functions
_vec_to_blob and _blob_to_vec), and apply the same change to the analogous
import in the search_embedding helper at the other location mentioned so mypy no
longer flags numpy as missing while keeping numpy as an optional extras
dependency.
Comment on lines
+425
to
+433
| def _h_eval_embeddings(p: dict) -> dict: | ||
| from pathlib import Path | ||
|
|
||
| from .embeddings.scorer import evaluate | ||
| return evaluate( | ||
| kb_dir=_store().kb_dir, | ||
| queries_file=Path(p["queries_path"]), | ||
| k=int(p.get("k", 10)), | ||
| ) |
There was a problem hiding this comment.
kb.eval_embeddings currently ignores requested metric selection.
Lines 429-433 always use default metrics; caller-provided metric choices are not forwarded.
Proposed fix
def _h_eval_embeddings(p: dict) -> dict:
from pathlib import Path
from .embeddings.scorer import evaluate
+ metric_arg = p.get("metric")
+ metrics = (
+ tuple(m.strip() for m in str(metric_arg).split(",") if m.strip())
+ if metric_arg
+ else ("recall@k", "mrr", "ndcg")
+ )
return evaluate(
kb_dir=_store().kb_dir,
queries_file=Path(p["queries_path"]),
k=int(p.get("k", 10)),
+ metrics=metrics,
)🤖 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 425 - 433, _h_eval_embeddings ignores
caller-selected metrics; modify it to extract the metric(s) from the incoming
dict p (e.g., p.get("metric") or p.get("metrics")) and pass that value into the
evaluate(...) call so evaluate receives the requested metric selection. Ensure
you convert or normalize the incoming value to the expected evaluate parameter
type (string or list) before forwarding, and keep existing parameters
kb_dir=_store().kb_dir, queries_file=Path(p["queries_path"]), and
k=int(p.get("k", 10)).
Comment on lines
+56
to
+60
| def test_registry_round_trip() -> None: | ||
| register("test-adapter", lambda: MockEmbedder(dim=4)) | ||
| e = get_embedder("test-adapter") | ||
| assert e.dim == 4 | ||
| assert e.name == "mock" |
There was a problem hiding this comment.
Isolate registry mutation to avoid cross-test leakage.
register("test-adapter", ...) mutates shared global state and can make tests order-dependent if another test uses the same key. Use a unique adapter name (e.g., UUID-suffixed) or restore registry state via fixture/monkeypatch.
🤖 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_core.py` around lines 56 - 60, The
test_registry_round_trip mutates the global registry by calling
register("test-adapter", ...) which can leak state between tests; update
test_registry_round_trip to use a unique adapter key (e.g., append a UUID or
timestamp) when calling register or ensure the registry is restored after the
test (unregister or snapshot/restore) so other tests are unaffected; locate the
calls to register and get_embedder in test_registry_round_trip and change the
adapter name generation or add teardown logic to remove the registered
"test-adapter" entry (or restore previous registry state) so the mutation is
isolated.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-8-cli-docs
branch
from
5fddf2c to
95a3507
Compare
4 tasks
dripsmvcp
force-pushed
the
feat/semantic-search-phase-8-cli-docs
branch
2 times, most recently
from
12b8e6c to
79bbcef
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (7)
src/vouch/capabilities.py (1)
63-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate advertised retrieval modes to include semantic/hybrid support.
capabilities()still reports lexical-only retrieval while embedding methods are now exposed, so clients can incorrectly disable semantic paths.Proposed fix
- retrieval=["fts5", "substring"], + retrieval=["embedding", "hybrid", "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 `@src/vouch/capabilities.py` around lines 63 - 69, The capabilities() function currently advertises only lexical retrieval modes ("fts5", "substring") even though embedding/semantic methods are available; update the Capabilities returned by capabilities() to include semantic/hybrid retrieval types (e.g., "semantic", "hybrid" or whatever canonical names your system uses) in the retrieval list so clients can enable semantic paths. Locate the capabilities() function and the Capabilities(...) construction and add the appropriate semantic tokens to the retrieval field (ensuring consistency with any existing METHOD names in METHODS and any client-facing enum strings).src/vouch/embeddings/migration.py (1)
34-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
forceis currently a no-op.Line 34 exposes a behavioral flag, but it is never used, so callers cannot influence backfill behavior.
src/vouch/embeddings/hyde.py (1)
20-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd failure fallback for LLM expansion.
If
llm(prompt)raises, query expansion currently fails the search path instead of degrading to the original query.Proposed fix
def expand_query_with_llm(query: str, *, llm: Callable[[str], str]) -> str: @@ - return llm(prompt).strip() or query + try: + expanded = llm(prompt).strip() + except Exception: + return query + return expanded or query🤖 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/hyde.py` around lines 20 - 27, The expand_query_with_llm function currently calls llm(prompt) directly and will propagate any exception; wrap the llm invocation in a try/except that catches generic exceptions and returns the original query on failure (preserve the existing behavior of falling back when the LLM returns an empty/whitespace string), e.g. call llm(prompt) inside a try block, strip the result and if truthy return it, otherwise return query, and in the except block return query (optionally log the exception if a logger is available) while keeping the function signature expand_query_with_llm(query: str, *, llm: Callable[[str], str]) -> str unchanged.src/vouch/embeddings/rerank.py (1)
44-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail loudly on reranker score-count mismatch.
zip(..., strict=False)can silently truncate results when the reranker returns fewer scores than hits.Suggested fix
candidates = [h[2] or h[1] for h in hits] scores = reranker.score(query, candidates) + if len(scores) != len(hits): + raise ValueError( + f"reranker returned {len(scores)} scores for {len(hits)} hits" + ) reranked = [ - (kind, id_, snip, score) - for (kind, id_, snip, _orig), score in zip(hits, scores, strict=False) + (kind, id_, snip, scores[i]) + for i, (kind, id_, snip, _orig) in enumerate(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/embeddings/rerank.py` around lines 44 - 47, The comprehension building reranked can silently drop items because it uses zip(..., strict=False); change the logic to detect and fail on a length mismatch between hits and scores (either by using zip(..., strict=True) or by explicitly checking len(hits) vs len(scores) and raising a ValueError with contextual info) so that any mismatch between hits and scores in the rerank step is surfaced; update the code that constructs reranked (referencing the reranked variable and the hits and scores iterables) accordingly.src/vouch/jsonl_server.py (2)
427-435:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward caller-selected metrics in
kb.eval_embeddings.Current handler ignores requested metric selection and always relies on evaluator defaults.
Suggested fix
def _h_eval_embeddings(p: dict) -> dict: from pathlib import Path from .embeddings.scorer import evaluate + metric_arg = p.get("metric") or p.get("metrics") + metrics = ( + tuple(m.strip() for m in str(metric_arg).split(",") if m.strip()) + if metric_arg + else ("recall@k", "mrr", "ndcg") + ) return evaluate( kb_dir=_store().kb_dir, queries_file=Path(p["queries_path"]), k=int(p.get("k", 10)), + metrics=metrics, )🤖 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 427 - 435, The _h_eval_embeddings handler currently ignores caller-selected metrics; update it to forward a metric (or metrics) argument from the incoming params dict `p` to the evaluator by reading e.g. `p.get("metric")` or `p.get("metrics")` and passing that value to the `evaluate` call (alongside existing `kb_dir`, `queries_file` and `k`). Modify the call in `_h_eval_embeddings` so `evaluate(..., k=int(p.get("k", 10)), metric=metric_value)` (or `metrics=...` if the evaluator expects plural) when the param is present, leaving behavior unchanged if the param is absent.
99-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the hybrid FTS leg against runtime failure.
If FTS search raises, the whole
kb.searchrequest becomesinternal_errorinstead of returning hybrid/semantic results.Suggested fix
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) + try: + fts = index_db.search(s.kb_dir, q, limit=limit * 2) + except Exception: + fts = [] 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 - 105, Wrap the FTS leg (index_db.search call) in a try/except so a runtime error from index_db.search(s.kb_dir, q, limit=...) does not abort the whole kb.search; on exception log or record the error and fall back to using only semantic hits (e.g., set fts = [] or None) and call rrf_fuse(emb, fts, limit=limit) or skip fusion and return emb-based results; update the hybrid branch where backend_arg == "hybrid" around index_db.search and rrf_fuse to handle a failed FTS gracefully.src/vouch/index_db.py (1)
186-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPatch
numpyimports for optional-dependency type checking.These imports are still unguarded for mypy in optional extras setups and can reintroduce
import-not-foundCI failures.Suggested fix
def _vec_to_blob(vec): # type: ignore[no-untyped-def] - import numpy as np + import numpy as np # type: ignore[import-not-found] return np.asarray(vec, dtype=np.float32).tobytes() def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def] - import numpy as np + import numpy as np # type: ignore[import-not-found] return np.frombuffer(blob, dtype=np.float32, count=dim).copy() def search_embedding( @@ - import numpy as np + import numpy as np # type: ignore[import-not-found]Also applies to: 284-284
🤖 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 186 - 192, Add a mypy-safe numpy stub import guarded by TYPE_CHECKING and keep the runtime local imports: at the top of the module add "from typing import TYPE_CHECKING" and "if TYPE_CHECKING: import numpy as np" so type checkers know about numpy types without requiring numpy at runtime; leave the existing in-function "import numpy as np" inside _vec_to_blob and _blob_to_vec (and the similar helper at the other location around line 284) so optional-dependency CI won't fail while still providing types for static analysis.
🧹 Nitpick comments (2)
src/vouch/storage.py (1)
466-509: Embedding on every write may add noticeable latency.
_embed_and_storeruns synchronously on every artifact persist (claims, pages, entities, etc.). For large text or slow embedding models, this could significantly degrade write throughput. Consider whether embedding should be deferred or batched for bulk operations.This is an architectural observation rather than a blocking issue since failures are swallowed.
🤖 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 466 - 509, The _embed_and_store function currently performs synchronous embedding on every write which can add latency; change the flow so writes enqueue embedding work instead of doing it inline: modify callers of _embed_and_store to add an async/deferred path (e.g., push a job into a new in-process queue or external task queue) and implement a background worker that calls the existing _embed_and_store logic (reusing get_embedder, index_db.put_embedding, index_db.set_embedding_meta, and dedup.check_and_log) to perform encoding and DB writes; alternatively add a configuration flag or parameter to _embed_and_store to allow immediate versus deferred behavior and provide a batch-processing routine that consumes queued items and calls the same embed/store code to handle large texts/models without blocking the write path.src/vouch/server.py (1)
611-622: 💤 Low valueRemove redundant
Pathimport.
Pathis already imported at the module level (line 15). The local import on line 614 is unnecessary.Proposed fix
`@mcp.tool`() def kb_eval_embeddings(*, queries_path: str, k: int = 10) -> dict[str, Any]: """Run retrieval eval over a JSONL queries file.""" - from pathlib import Path - from .embeddings.scorer import evaluate store = _store() return evaluate(🤖 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 611 - 622, The local import "from pathlib import Path" inside the kb_eval_embeddings function is redundant because Path is already imported at module level; remove that local import statement in the kb_eval_embeddings function and keep using Path when constructing Path(queries_path) so the function (kb_eval_embeddings) relies on the module-level Path import.
🤖 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/context.py`:
- Around line 83-84: The annotated return type is incorrect: the function
currently declares -> ContextPack | dict[str, Any] but always returns
pack.model_dump() (a dict). Change the signature to -> dict[str, Any] and update
any other functions in the same block (around the pack variable and the
model_dump() calls at/near the 151-160 region) to use dict[str, Any] as the
return annotation so it matches the actual returned value; ensure symbols
referenced include pack and model_dump() for locating the code to change.
In `@src/vouch/embeddings/migration.py`:
- Around line 38-80: The current migration wraps entire loops in try/except
AttributeError which hides real bugs when accessing item attributes (e.g.,
p.body, r.relation) and silently skips work; change each block to only guard the
call to the iterator (e.g., store.list_claims(), store.list_pages(),
store.list_sources(), store.list_entities(), store.list_relations(),
store.list_evidence()) with a try/except AttributeError, and then iterate
normally so attribute errors on items surface; additionally, inside each loop
use safe access (getattr with defaults or explicit checks) when constructing
texts for store._embed_and_store (references: store._embed_and_store,
list_claims/list_pages/list_sources/list_entities/list_relations/list_evidence)
and log or re-raise unexpected AttributeErrors instead of swallowing them.
In `@src/vouch/embeddings/scorer.py`:
- Around line 56-59: The loop reading queries_file calls json.loads(line)
directly which will raise on blank lines; modify the reader in the function that
iterates queries_file (the block using queries_file.open(), the loop that
assigns row = json.loads(line) and q = row["query"]) to skip
blank/whitespace-only lines by checking line.strip() and continuing when empty
before calling json.loads.
In `@src/vouch/health.py`:
- Around line 169-180: The mismatch audit currently only catches ImportError but
detect_mismatch(store.kb_dir) and audit.log_event(...) can raise runtime errors
and should not abort rebuild_index(); wrap the detect_mismatch call and the
audit.log_event call in a broad try/except Exception that prevents exceptions
from propagating (optionally log at debug level) so any I/O/DB/audit write
failures are swallowed and rebuild_index() continues; target the block using
detect_mismatch, audit.log_event and store.kb_dir and ensure only ImportError vs
other Exceptions are handled separately.
In `@src/vouch/index_db.py`:
- Around line 295-300: The SQL references the SELECT alias "score" in the WHERE
clause which SQLite disallows; change the query to compute score in an inner
subquery and filter on score in the outer WHERE (or move the score predicate
into a WHERE that repeats the expression). Specifically, replace the current
query string that selects "1.0 - vec_distance_cosine(vec, ?) AS score" and then
uses "WHERE ... AND score >= ?" with a subquery like SELECT kind, id, score FROM
(SELECT kind, id, 1.0 - vec_distance_cosine(vec, ?) AS score FROM
embedding_index WHERE kind IN (...) ) WHERE score >= ? ORDER BY score DESC LIMIT
? and pass the same parameters (q.tobytes(), *kinds, min_score, limit); update
the SQL variable in the function that builds this query in src/vouch/index_db.py
accordingly.
- Around line 311-315: The fallback loop in the search routine computes score =
float(q @ vec) where q is unit-normalized but vec (from _blob_to_vec) is not, so
normalize vec before dot to preserve cosine semantics: compute vec_norm =
np.linalg.norm(vec) (or similar) and if vec_norm > 0 use vec_unit = vec /
vec_norm else skip or treat score as 0, then compute score = float(q @ vec_unit)
and compare against min_score; update the scored append to use the normalized
score and leave the rest unchanged (references: _blob_to_vec, q, vec, score,
min_score, scored).
In `@src/vouch/server.py`:
- Around line 78-135: The hybrid branch in kb_search ignores the min_score:
modify the hybrid path so index_db.search_semantic is called with the min_score
parameter (i.e. change the emb = index_db.search_semantic(store.kb_dir, query,
limit=limit * 2) call inside the backend == "hybrid" block to include
min_score=min_score) and ensure rrf_fuse still receives the two hit lists;
optionally, after fusion you can filter the returned hits by score >= min_score
before returning via _to_dicts to guarantee the threshold is enforced.
In `@tests/test_context.py`:
- Around line 9-20: Move the MockEmbedder import into the autouse fixture and
gate it with pytest.importorskip("numpy"): inside the _mock_embedder() fixture,
call pytest.importorskip("numpy") first, then import MockEmbedder from
tests.embeddings._fakes and register it via register(DEFAULT_MODEL_NAME, lambda:
MockEmbedder(dim=8)); keep the fixture name _mock_embedder, the register call,
and DEFAULT_MODEL_NAME unchanged so tests collect when numpy is absent and skip
when the fixture is actually needed.
---
Duplicate comments:
In `@src/vouch/capabilities.py`:
- Around line 63-69: The capabilities() function currently advertises only
lexical retrieval modes ("fts5", "substring") even though embedding/semantic
methods are available; update the Capabilities returned by capabilities() to
include semantic/hybrid retrieval types (e.g., "semantic", "hybrid" or whatever
canonical names your system uses) in the retrieval list so clients can enable
semantic paths. Locate the capabilities() function and the Capabilities(...)
construction and add the appropriate semantic tokens to the retrieval field
(ensuring consistency with any existing METHOD names in METHODS and any
client-facing enum strings).
In `@src/vouch/embeddings/hyde.py`:
- Around line 20-27: The expand_query_with_llm function currently calls
llm(prompt) directly and will propagate any exception; wrap the llm invocation
in a try/except that catches generic exceptions and returns the original query
on failure (preserve the existing behavior of falling back when the LLM returns
an empty/whitespace string), e.g. call llm(prompt) inside a try block, strip the
result and if truthy return it, otherwise return query, and in the except block
return query (optionally log the exception if a logger is available) while
keeping the function signature expand_query_with_llm(query: str, *, llm:
Callable[[str], str]) -> str unchanged.
In `@src/vouch/embeddings/rerank.py`:
- Around line 44-47: The comprehension building reranked can silently drop items
because it uses zip(..., strict=False); change the logic to detect and fail on a
length mismatch between hits and scores (either by using zip(..., strict=True)
or by explicitly checking len(hits) vs len(scores) and raising a ValueError with
contextual info) so that any mismatch between hits and scores in the rerank step
is surfaced; update the code that constructs reranked (referencing the reranked
variable and the hits and scores iterables) accordingly.
In `@src/vouch/index_db.py`:
- Around line 186-192: Add a mypy-safe numpy stub import guarded by
TYPE_CHECKING and keep the runtime local imports: at the top of the module add
"from typing import TYPE_CHECKING" and "if TYPE_CHECKING: import numpy as np" so
type checkers know about numpy types without requiring numpy at runtime; leave
the existing in-function "import numpy as np" inside _vec_to_blob and
_blob_to_vec (and the similar helper at the other location around line 284) so
optional-dependency CI won't fail while still providing types for static
analysis.
In `@src/vouch/jsonl_server.py`:
- Around line 427-435: The _h_eval_embeddings handler currently ignores
caller-selected metrics; update it to forward a metric (or metrics) argument
from the incoming params dict `p` to the evaluator by reading e.g.
`p.get("metric")` or `p.get("metrics")` and passing that value to the `evaluate`
call (alongside existing `kb_dir`, `queries_file` and `k`). Modify the call in
`_h_eval_embeddings` so `evaluate(..., k=int(p.get("k", 10)),
metric=metric_value)` (or `metrics=...` if the evaluator expects plural) when
the param is present, leaving behavior unchanged if the param is absent.
- Around line 99-105: Wrap the FTS leg (index_db.search call) in a try/except so
a runtime error from index_db.search(s.kb_dir, q, limit=...) does not abort the
whole kb.search; on exception log or record the error and fall back to using
only semantic hits (e.g., set fts = [] or None) and call rrf_fuse(emb, fts,
limit=limit) or skip fusion and return emb-based results; update the hybrid
branch where backend_arg == "hybrid" around index_db.search and rrf_fuse to
handle a failed FTS gracefully.
---
Nitpick comments:
In `@src/vouch/server.py`:
- Around line 611-622: The local import "from pathlib import Path" inside the
kb_eval_embeddings function is redundant because Path is already imported at
module level; remove that local import statement in the kb_eval_embeddings
function and keep using Path when constructing Path(queries_path) so the
function (kb_eval_embeddings) relies on the module-level Path import.
In `@src/vouch/storage.py`:
- Around line 466-509: The _embed_and_store function currently performs
synchronous embedding on every write which can add latency; change the flow so
writes enqueue embedding work instead of doing it inline: modify callers of
_embed_and_store to add an async/deferred path (e.g., push a job into a new
in-process queue or external task queue) and implement a background worker that
calls the existing _embed_and_store logic (reusing get_embedder,
index_db.put_embedding, index_db.set_embedding_meta, and dedup.check_and_log) to
perform encoding and DB writes; alternatively add a configuration flag or
parameter to _embed_and_store to allow immediate versus deferred behavior and
provide a batch-processing routine that consumes queued items and calls the same
embed/store code to handle large texts/models without blocking the write path.
🪄 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: c165dc51-e8ae-4530-9ce2-41608ecb7a53
📒 Files selected for processing (32)
docs/README.mddocs/embeddings.mddocs/retrieval.mdpyproject.tomlsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/dedup.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/hyde.pysrc/vouch/embeddings/migration.pysrc/vouch/embeddings/rerank.pysrc/vouch/embeddings/scorer.pysrc/vouch/health.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/conftest.pytests/embeddings/test_cli.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.pytests/embeddings/test_integration.pytests/embeddings/test_migration.pytests/embeddings/test_rerank.pytests/embeddings/test_scorer.pytests/embeddings/test_search.pytests/embeddings/test_storage.pytests/test_context.py
✅ Files skipped from review due to trivial changes (4)
- tests/embeddings/conftest.py
- docs/embeddings.md
- docs/README.md
- docs/retrieval.md
Comment on lines
+83
to
+84
| explain: bool = False, | ||
| ) -> ContextPack | dict[str, Any]: |
There was a problem hiding this comment.
Return type annotation doesn't match actual return.
The function signature says -> ContextPack | dict[str, Any] but line 152 always calls pack.model_dump() which returns dict, never a ContextPack object. The annotation should be -> dict[str, Any].
Proposed fix
explain: bool = False,
-) -> ContextPack | dict[str, Any]:
+) -> dict[str, Any]:Also applies to: 151-160
🤖 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/context.py` around lines 83 - 84, The annotated return type is
incorrect: the function currently declares -> ContextPack | dict[str, Any] but
always returns pack.model_dump() (a dict). Change the signature to -> dict[str,
Any] and update any other functions in the same block (around the pack variable
and the model_dump() calls at/near the 151-160 region) to use dict[str, Any] as
the return annotation so it matches the actual returned value; ensure symbols
referenced include pack and model_dump() for locating the code to change.
Comment on lines
+169
to
+180
| try: | ||
| from . import audit | ||
| from .embeddings.migration import detect_mismatch | ||
| m = detect_mismatch(store.kb_dir) | ||
| if m is not None: | ||
| audit.log_event( | ||
| store.kb_dir, event="embedding.model_mismatch", | ||
| actor="vouch-health", | ||
| object_ids=[], data=m, | ||
| ) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Keep mismatch auditing strictly non-fatal.
Line 172–178 can still raise runtime errors (I/O/DB/audit write), which would fail rebuild_index() even though this check is intended as best-effort.
Proposed fix
- try:
- from . import audit
- from .embeddings.migration import detect_mismatch
- m = detect_mismatch(store.kb_dir)
- if m is not None:
- audit.log_event(
- store.kb_dir, event="embedding.model_mismatch",
- actor="vouch-health",
- object_ids=[], data=m,
- )
- except ImportError:
- pass
+ try:
+ from . import audit
+ from .embeddings.migration import detect_mismatch
+ except ImportError:
+ pass
+ else:
+ try:
+ m = detect_mismatch(store.kb_dir)
+ if m is not None:
+ audit.log_event(
+ store.kb_dir,
+ event="embedding.model_mismatch",
+ actor="vouch-health",
+ object_ids=[],
+ data=m,
+ )
+ except Exception:
+ pass🤖 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/health.py` around lines 169 - 180, The mismatch audit currently
only catches ImportError but detect_mismatch(store.kb_dir) and
audit.log_event(...) can raise runtime errors and should not abort
rebuild_index(); wrap the detect_mismatch call and the audit.log_event call in a
broad try/except Exception that prevents exceptions from propagating (optionally
log at debug level) so any I/O/DB/audit write failures are swallowed and
rebuild_index() continues; target the block using detect_mismatch,
audit.log_event and store.kb_dir and ensure only ImportError vs other Exceptions
are handled separately.
7e21643 to
daa6582
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
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-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake JSONL
kb.searchmatch the MCP contract.This handler still returns a bare hit list and silently treats unknown backends as empty results. MCP now returns a top-level
{"backend", "hits"}object and rejects invalid backend names, so the two transports are no longer interchangeable for the same client code.🤖 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 - 109, The _h_search handler currently returns a bare list of hit dicts and silently treats unknown backend values as no-results; update _h_search to validate the backend parameter against the allowed set ("auto", "embedding", "fts5", "substring", "hybrid") and raise/return an error for invalid names, execute the chosen search paths using index_db.search_semantic, index_db.search, s.search_substring and fusion via rrf_fuse as before, then return a top-level object {"backend": used, "hits": [...] } where "hits" is the list of {"kind","id","snippet","score"} dicts; ensure the "used" value reflects the actual backend used (e.g., "embedding","fts5","substring","hybrid") and not silently default unknown backends to empty results.
♻️ Duplicate comments (11)
docs/retrieval.md (1)
6-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOutdated sections still contradict the new semantic retrieval section.
Lines 6-7 and 86-95 still state that embeddings are not yet available or planned for the future, but the new "Semantic retrieval" section at lines 121-123 documents that embeddings are now supported. Update lines 6-7 and the entire "Why FTS5, not embeddings (yet)" section to reflect that semantic retrieval is available.
Also applies to: 86-95, 121-123
🤖 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/retrieval.md` around lines 6 - 7, The docs contain an outdated claim ("For *why* we picked FTS5 over embeddings (today), see ...") and a section titled "Why FTS5, not embeddings (yet)" that contradicts the "Semantic retrieval" section which now documents embeddings support; update those areas by removing the "not yet" wording, rewriting the opening sentence and that section to state that semantic retrieval/embeddings are supported, briefly describe tradeoffs between FTS5 and embeddings and when to use each, and update or remove the ROADMAP link so cross-references are accurate; specifically edit the sentence that currently references FTS5-over-embeddings and the section header "Why FTS5, not embeddings (yet)" and any repeated text in the other affected paragraphs to match the "Semantic retrieval" content.src/vouch/embeddings/scorer.py (1)
56-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip blank JSONL lines before decoding.
Line 58 decodes every raw line; empty/whitespace-only lines will raise
JSONDecodeErrorand stop the whole eval run.Proposed fix
with queries_file.open() as f: for line in f: + if not line.strip(): + continue row = json.loads(line) q = row["query"]🤖 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/scorer.py` around lines 56 - 59, When reading the JSONL in scorer.py (the block using queries_file.open(), iterating over line, and calling json.loads(line) to produce row and q), skip any blank or whitespace-only lines before calling json.loads to avoid JSONDecodeError; i.e., strip the line and continue the loop if it's empty so only non-empty lines are passed into json.loads.src/vouch/embeddings/migration.py (2)
34-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
forceis exposed but currently does nothing.Line 34 declares
force, but the function never branches on it. That makes--forcebehavior ineffective for reindex/backfill flows.🤖 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/migration.py` around lines 34 - 35, The backfill_embeddings function accepts a force flag but never uses it; update backfill_embeddings to branch on the force parameter when deciding whether to skip artifacts that already have embeddings — e.g., in the loop that checks existing embeddings (the logic that currently skips or counts artifacts as already-encoded), add an if force: ... path to re-encode regardless (or otherwise bypass the "already has embedding" check), ensure the touched count increments for re-encoded items, and preserve existing behavior when force is False; reference the backfill_embeddings function and the existing per-artifact embedding-check/encode logic to locate where to add this conditional.
38-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid catching
AttributeErroraround whole loops.These
try/except AttributeErrorblocks also swallow real item-shape bugs (for example missing fields on list entries), causing silent partial backfills.Safer pattern (apply per artifact type)
- try: - for p in store.list_pages(): - store._embed_and_store(kind="page", id=p.id, text=f"{p.title}\n\n{p.body}") - touched += 1 - except AttributeError: - pass + list_pages = getattr(store, "list_pages", None) + if callable(list_pages): + for p in list_pages(): + store._embed_and_store(kind="page", id=p.id, text=f"{p.title}\n\n{p.body}") + touched += 1🤖 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/migration.py` around lines 38 - 80, The current migration wraps each entire loop in try/except AttributeError which hides bugs; change each block that uses store.list_claims(), store.list_pages(), store.list_sources(), store.list_entities(), store.list_relations(), and store.list_evidence() to first check the presence of the listing method (e.g. hasattr(store, "list_claims")) and skip if missing, then iterate and call store._embed_and_store for each item but catch and handle exceptions per-item (catch AttributeError or other exceptions around accessing item fields or the single _embed_and_store call), log the bad item and continue so one malformed entry doesn't silence the whole artifact type. Ensure logs reference the kind and item id/name to aid debugging.src/vouch/health.py (1)
169-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep embedding mismatch auditing strictly non-fatal.
Line 172 and Line 174 can still raise runtime errors; only
ImportErroris caught now. That can abortrebuild_index()even though this check is best-effort.Proposed fix
- try: - from . import audit - from .embeddings.migration import detect_mismatch - m = detect_mismatch(store.kb_dir) - if m is not None: - audit.log_event( - store.kb_dir, event="embedding.model_mismatch", - actor="vouch-health", - object_ids=[], data=m, - ) - except ImportError: - pass + try: + from . import audit + from .embeddings.migration import detect_mismatch + except ImportError: + pass + else: + try: + m = detect_mismatch(store.kb_dir) + if m is not None: + audit.log_event( + store.kb_dir, event="embedding.model_mismatch", + actor="vouch-health", + object_ids=[], data=m, + ) + except Exception: + pass🤖 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/health.py` around lines 169 - 180, The embedding mismatch check around detect_mismatch(store.kb_dir) currently only catches ImportError and can still raise runtime exceptions that abort rebuild_index(); change the handler to catch all exceptions (except Exception as e) around the entire block that imports audit and calls detect_mismatch/audit.log_event so any runtime error is swallowed (non-fatal) and does not propagate to rebuild_index(); optionally emit a non-fatal log message containing e to aid debugging while keeping the operation best-effort.src/vouch/embeddings/rerank.py (1)
43-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail loudly on score-count mismatch before building reranked output.
Line 46 uses
zip(..., strict=False), which can silently truncate results whenlen(scores) != len(hits).Proposed fix
candidates = [h[2] or h[1] for h in hits] scores = reranker.score(query, candidates) + if len(scores) != len(hits): + raise ValueError( + f"reranker returned {len(scores)} scores for {len(hits)} hits" + ) reranked = [ - (kind, id_, snip, score) - for (kind, id_, snip, _orig), score in zip(hits, scores, strict=False) + (kind, id_, snip, scores[i]) + for i, (kind, id_, snip, _orig) in enumerate(hits) ]#!/bin/bash # Verify that rerank guards score/hit cardinality before composing reranked output. rg -n -C2 'scores = reranker\.score|zip\(hits, scores, strict=False\)|len\(scores\) != len\(hits\)' src/vouch/embeddings/rerank.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/rerank.py` around lines 43 - 47, The current code creates reranked by zipping hits and scores with strict=False which silently truncates if len(scores) != len(hits); update the logic around reranker.score and the reranked list comprehension to explicitly check that len(scores) == len(hits) (or raise a clear exception) before building reranked, and then use a deterministic zip (or zip with strict=True) to pair (kind, id_, snip, _orig) from hits with each score; reference the symbols reranker.score, scores, hits, and the reranked list comprehension when making the change.src/vouch/index_db.py (2)
185-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuppress the optional
numpyimports for mypy.CI is already failing on Lines 186, 191, and 284 with
import-not-found. Sincenumpyis optional here, these dynamic imports need the same mypy suppression the earlier review requested.Suggested fix
def _vec_to_blob(vec): # type: ignore[no-untyped-def] - import numpy as np + import numpy as np # type: ignore[import-not-found] return np.asarray(vec, dtype=np.float32).tobytes() def _blob_to_vec(blob: bytes, dim: int): # type: ignore[no-untyped-def] - import numpy as np + import numpy as np # type: ignore[import-not-found] return np.frombuffer(blob, dtype=np.float32, count=dim).copy() def search_embedding( @@ - import numpy as np + import numpy as np # type: ignore[import-not-found]Also applies to: 284-284
🤖 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 185 - 192, The dynamic, optional numpy imports inside _vec_to_blob and _blob_to_vec are raising mypy import-not-found errors; update the inline import statements to suppress mypy's import-not-found check (e.g., add type: ignore[import-not-found] to the `import numpy as np` lines in functions _vec_to_blob and _blob_to_vec), and apply the same suppression to the other dynamic numpy import referenced at the other occurrence (around line 284) so CI stops failing for optional numpy usage.
315-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize stored vectors in the Python fallback.
Line 317 computes
q @ vecafter normalizing onlyq. Whensqlite-vecis unavailable, larger-magnitude stored vectors get inflated scores, so ranking andmin_scorefiltering diverge from the cosine-based SQL path.Suggested fix
scored: list[tuple[str, str, str, float]] = [] for kind, id_, blob, dim in rows: vec = _blob_to_vec(blob, dim) - score = float(q @ vec) + 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))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/index_db.py` around lines 315 - 319, The fallback loop that computes score using q @ vec must normalize the stored vector as well as q to match the cosine-based SQL path: inside the loop over (kind, id_, blob, dim) where _blob_to_vec(blob, dim) produces vec, compute vec's norm and divide vec by its norm (handling zero-norm safely) before doing score = float(q @ vec) so ranking and min_score filtering behave the same as when sqlite-vec is available.src/vouch/cli.py (1)
649-654:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
--modelthrough to the backfill call.
get_embedder(model)only validates the adapter.backfill_embeddings()still runs with the default model, sovouch reindex --model ...does not honor the selected embedder.Suggested fix
if embeddings or backfill: from .embeddings.migration import backfill_embeddings + embedder = None if model: from .embeddings import get_embedder - get_embedder(model) - n = backfill_embeddings(store, force=force) + embedder = get_embedder(model) + n = backfill_embeddings(store, force=force, embedder=embedder) click.echo(f"reindex: embeddings backfilled = {n}")🤖 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/cli.py` around lines 649 - 654, The CLI currently calls get_embedder(model) only to validate the adapter but then calls backfill_embeddings(store, force=force) which ignores the chosen model; update the call to backfill_embeddings to pass the model through (e.g., backfill_embeddings(store, model=model, force=force)) so that the selected embedder is honored, keeping the existing get_embedder(model) validation call in place; adjust the backfill_embeddings invocation signature usage in src/vouch/cli.py accordingly.src/vouch/capabilities.py (1)
63-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdvertise the new semantic retrieval modes in
kb.capabilities.
METHODSnow exposes the embedding tool surface, butretrievalstill reports only lexical modes. Clients that feature-detect viakb.capabilitieswill miss"embedding"/"hybrid"support and never opt into the new backend.🤖 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/capabilities.py` around lines 63 - 69, The capabilities() function currently advertises only lexical retrieval modes; update its retrieval field to include the new semantic modes by adding "embedding" and "hybrid" to the list returned by Capabilities (the function name capabilities(), the Capabilities constructor and the retrieval list literal are the targets); ensure the retrieval list becomes something like ["fts5", "substring", "embedding", "hybrid"] so clients that feature-detect via kb.capabilities will see the embedding/hybrid support exposed by METHODS.src/vouch/jsonl_server.py (1)
99-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep hybrid JSONL search resilient and threshold-aware.
The hybrid branch still skips
min_scoreand letsindex_db.search()raise even though the non-hybrid path already treats FTS failures as recoverable. That makesbackend="hybrid"behave differently from the documented parameter set and fail harder than the MCP path.🤖 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, The hybrid branch currently calls index_db.search_semantic(...) and index_db.search(...) then rrf_fuse(...) but does not apply the min_score filter nor catch failures from index_db.search; update the hybrid branch (the block using rrf_fuse, emb, fts, index_db.search_semantic, index_db.search, and variable used) to mirror the non-hybrid logic: wrap the index_db.search(...) call in the same try/except used elsewhere so FTS failures are recovered from, and filter the fused hits by min_score (or apply the same score-thresholding post-fuse) before assigning hits and setting used="hybrid". Ensure the behavior matches the documented parameters and the MCP/non-hybrid path.
🤖 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/cli.py`:
- Around line 511-514: When backend == "hybrid" ensure the min-score cutoff is
applied before fusion: pass the configured min_score/cutoff into
index_db.search_semantic (or filter its results) so embedding hits below the
threshold are excluded prior to calling rrf_fuse; update the hybrid branch where
search_semantic(store.kb_dir, q, limit=...) and index_db.search(...) are called
so that search_semantic receives the same min_score/cutoff parameter (or apply a
post-call filter on the emb list) before calling rrf_fuse(emb, fts,
limit=limit).
- Around line 616-625: The code silently falls back to k=10 when parsing
malformed "recall@..." metrics; instead validate and reject bad values: in the
loop that iterates over metrics (the block that sets k from m.split("@", 1)[1])
replace the contextlib.suppress(ValueError) with explicit parsing that
raises/returns a clear error on invalid integers (e.g., raise
argparse.ArgumentTypeError or call sys.exit with an informative message) so
malformed inputs like "recall@foo" are rejected rather than converted into
recall@10; update any callers that rely on k to handle the validation error
accordingly (refer to the variables/identifiers metrics, canonical, k).
In `@src/vouch/embeddings/cache.py`:
- Around line 27-31: The INSERT OR REPLACE SQL in the cache module currently
adds "+ 1" to hit_count, causing writes to count as hits; change the VALUES
clause to initialize hit_count from COALESCE((SELECT hit_count FROM
query_embedding_cache WHERE query_hash=?), 0) without adding +1, and move any
increment logic so hit_count is incremented only in the cache retrieval/lookup
path (where you read a cached entry), not in the upsert; reference the
query_embedding_cache table and the INSERT OR REPLACE statement in
src/vouch/embeddings/cache.py when making this change.
In `@src/vouch/embeddings/dedup.py`:
- Line 9: The top-level import of numpy in src/vouch/embeddings/dedup.py causes
ImportError at runtime for user-invoked paths (kb_dedup_scan, _h_dedup_scan, and
the CLI dedup command); change that to a guarded/local import: remove the
module-level "import numpy as np" and instead import numpy inside the functions
that need it (or wrap "from .embeddings.dedup import scan_all" calls in
try/except ImportError), and on ImportError raise a clear RuntimeError
instructing the user to install the optional extras (e.g. "please install
vouch[embeddings]"). Update the symbols kb_dedup_scan, _h_dedup_scan, and the
CLI dedup invocation to use the guarded import so missing numpy fails with the
actionable message rather than an unhandled ImportError.
In `@src/vouch/index_db.py`:
- Around line 40-67: reset() currently only clears FTS tables but leaves derived
embedding state; update the reset() function to also purge all embedding-derived
tables by executing DELETE (or DROP+recreate) for embedding_index,
query_embedding_cache, embedding_dupes and any embedding_* metadata table(s) so
no stale embeddings remain after a rebuild — locate the reset() function in
index_db.py and add SQL statements to clear those tables (e.g., DELETE FROM
embedding_index; DELETE FROM query_embedding_cache; DELETE FROM embedding_dupes;
and similarly remove any embedding_* metadata).
In `@src/vouch/jsonl_server.py`:
- Around line 408-411: The JSONL handler _h_reindex_embeddings is dropping
parity-critical inputs and returning an incomplete response; update
_h_reindex_embeddings to forward model and backfill/backfill-related flags from
the incoming dict p into the call to backfill_embeddings (e.g., pass
model=p.get("model") and backfill/backfill-specific kwargs) and then return the
full response shape that backfill_embeddings provides (not just {"touched": n})
so clients get the same outputs as the MCP path; use the existing _store() and
backfill_embeddings(...) symbols to locate and adapt the call and response
handling.
- Around line 436-451: The JSONL handler _h_embeddings_stats currently omits
embedding metadata fields that MCP returns; update _h_embeddings_stats to
include the full payload by reading model_version and dim from
index_db.get_embedding_meta(store.kb_dir) (the existing meta variable) and
returning them alongside "model", "counts", and "query_cache" (coerce dim to int
if needed), so the return dict mirrors the MCP/kb.embeddings_stats shape used
elsewhere (refer to _h_embeddings_stats, index_db.get_embedding_meta, and
embeddings.cache.query_cache_stats).
In `@src/vouch/server.py`:
- Around line 126-131: The hybrid branch currently calls index_db.search(...)
unguarded which can raise if FTS is unavailable; update the hybrid path so it
mirrors the auto/fts5 logic: call index_db.search_semantic(store.kb_dir, query,
limit=limit*2) into emb as before, then attempt index_db.search(store.kb_dir,
query, limit=limit*2) inside a safe guard (e.g., try/except or check for None)
so that if the FTS leg fails you pass an empty list (or None handled by
rrf_fuse) into rrf_fuse(emb, fts, limit=limit) and still return _to_dicts(hits,
"hybrid") instead of raising an internal error; reference rrf_fuse,
index_db.search_semantic, index_db.search, and _to_dicts when making the change.
- Around line 609-620: The MCP tool kb_eval_embeddings currently calls
evaluate() with defaults and doesn't allow selecting the evaluation metric;
update the kb_eval_embeddings signature to accept a metric parameter (e.g.,
metric: str | None = None or metric: Literal["mrr","ndcg"] = "mrr"), document it
in the docstring, and pass that metric through to
embeddings.scorer.evaluate(kb_dir=..., queries_file=Path(queries_path), k=k,
metric=metric) so callers can request mrr/ndcg-only evaluation; ensure the
parameter name matches the evaluate() argument and keep imports and the _store()
usage unchanged.
In `@src/vouch/storage.py`:
- Around line 468-509: The embed/store path in _embed_and_store currently only
catches ImportError/KeyError but lets runtime errors from embedder.encode,
_index_db.put_embedding, _index_db.set_embedding_meta, and check_and_log
propagate; wrap the embedding and DB write/check sequence (everything after
get_embedder() — i.e., embedder.encode(...), _index_db.put_embedding(...),
_index_db.set_embedding_meta(...), and check_and_log(...)) in a broad try/except
Exception that logs the error and returns, so embedding failures are truly
best-effort and won't cause callers of put_source/put_claim/etc. to fail;
reference the _embed_and_store function and the symbols embedder.encode,
_index_db.put_embedding, _index_db.set_embedding_meta, and check_and_log when
making the change.
---
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 74-109: The _h_search handler currently returns a bare list of hit
dicts and silently treats unknown backend values as no-results; update _h_search
to validate the backend parameter against the allowed set ("auto", "embedding",
"fts5", "substring", "hybrid") and raise/return an error for invalid names,
execute the chosen search paths using index_db.search_semantic, index_db.search,
s.search_substring and fusion via rrf_fuse as before, then return a top-level
object {"backend": used, "hits": [...] } where "hits" is the list of
{"kind","id","snippet","score"} dicts; ensure the "used" value reflects the
actual backend used (e.g., "embedding","fts5","substring","hybrid") and not
silently default unknown backends to empty results.
---
Duplicate comments:
In `@docs/retrieval.md`:
- Around line 6-7: The docs contain an outdated claim ("For *why* we picked FTS5
over embeddings (today), see ...") and a section titled "Why FTS5, not
embeddings (yet)" that contradicts the "Semantic retrieval" section which now
documents embeddings support; update those areas by removing the "not yet"
wording, rewriting the opening sentence and that section to state that semantic
retrieval/embeddings are supported, briefly describe tradeoffs between FTS5 and
embeddings and when to use each, and update or remove the ROADMAP link so
cross-references are accurate; specifically edit the sentence that currently
references FTS5-over-embeddings and the section header "Why FTS5, not embeddings
(yet)" and any repeated text in the other affected paragraphs to match the
"Semantic retrieval" content.
In `@src/vouch/capabilities.py`:
- Around line 63-69: The capabilities() function currently advertises only
lexical retrieval modes; update its retrieval field to include the new semantic
modes by adding "embedding" and "hybrid" to the list returned by Capabilities
(the function name capabilities(), the Capabilities constructor and the
retrieval list literal are the targets); ensure the retrieval list becomes
something like ["fts5", "substring", "embedding", "hybrid"] so clients that
feature-detect via kb.capabilities will see the embedding/hybrid support exposed
by METHODS.
In `@src/vouch/cli.py`:
- Around line 649-654: The CLI currently calls get_embedder(model) only to
validate the adapter but then calls backfill_embeddings(store, force=force)
which ignores the chosen model; update the call to backfill_embeddings to pass
the model through (e.g., backfill_embeddings(store, model=model, force=force))
so that the selected embedder is honored, keeping the existing
get_embedder(model) validation call in place; adjust the backfill_embeddings
invocation signature usage in src/vouch/cli.py accordingly.
In `@src/vouch/embeddings/migration.py`:
- Around line 34-35: The backfill_embeddings function accepts a force flag but
never uses it; update backfill_embeddings to branch on the force parameter when
deciding whether to skip artifacts that already have embeddings — e.g., in the
loop that checks existing embeddings (the logic that currently skips or counts
artifacts as already-encoded), add an if force: ... path to re-encode regardless
(or otherwise bypass the "already has embedding" check), ensure the touched
count increments for re-encoded items, and preserve existing behavior when force
is False; reference the backfill_embeddings function and the existing
per-artifact embedding-check/encode logic to locate where to add this
conditional.
- Around line 38-80: The current migration wraps each entire loop in try/except
AttributeError which hides bugs; change each block that uses
store.list_claims(), store.list_pages(), store.list_sources(),
store.list_entities(), store.list_relations(), and store.list_evidence() to
first check the presence of the listing method (e.g. hasattr(store,
"list_claims")) and skip if missing, then iterate and call
store._embed_and_store for each item but catch and handle exceptions per-item
(catch AttributeError or other exceptions around accessing item fields or the
single _embed_and_store call), log the bad item and continue so one malformed
entry doesn't silence the whole artifact type. Ensure logs reference the kind
and item id/name to aid debugging.
In `@src/vouch/embeddings/rerank.py`:
- Around line 43-47: The current code creates reranked by zipping hits and
scores with strict=False which silently truncates if len(scores) != len(hits);
update the logic around reranker.score and the reranked list comprehension to
explicitly check that len(scores) == len(hits) (or raise a clear exception)
before building reranked, and then use a deterministic zip (or zip with
strict=True) to pair (kind, id_, snip, _orig) from hits with each score;
reference the symbols reranker.score, scores, hits, and the reranked list
comprehension when making the change.
In `@src/vouch/embeddings/scorer.py`:
- Around line 56-59: When reading the JSONL in scorer.py (the block using
queries_file.open(), iterating over line, and calling json.loads(line) to
produce row and q), skip any blank or whitespace-only lines before calling
json.loads to avoid JSONDecodeError; i.e., strip the line and continue the loop
if it's empty so only non-empty lines are passed into json.loads.
In `@src/vouch/health.py`:
- Around line 169-180: The embedding mismatch check around
detect_mismatch(store.kb_dir) currently only catches ImportError and can still
raise runtime exceptions that abort rebuild_index(); change the handler to catch
all exceptions (except Exception as e) around the entire block that imports
audit and calls detect_mismatch/audit.log_event so any runtime error is
swallowed (non-fatal) and does not propagate to rebuild_index(); optionally emit
a non-fatal log message containing e to aid debugging while keeping the
operation best-effort.
In `@src/vouch/index_db.py`:
- Around line 185-192: The dynamic, optional numpy imports inside _vec_to_blob
and _blob_to_vec are raising mypy import-not-found errors; update the inline
import statements to suppress mypy's import-not-found check (e.g., add type:
ignore[import-not-found] to the `import numpy as np` lines in functions
_vec_to_blob and _blob_to_vec), and apply the same suppression to the other
dynamic numpy import referenced at the other occurrence (around line 284) so CI
stops failing for optional numpy usage.
- Around line 315-319: The fallback loop that computes score using q @ vec must
normalize the stored vector as well as q to match the cosine-based SQL path:
inside the loop over (kind, id_, blob, dim) where _blob_to_vec(blob, dim)
produces vec, compute vec's norm and divide vec by its norm (handling zero-norm
safely) before doing score = float(q @ vec) so ranking and min_score filtering
behave the same as when sqlite-vec is available.
In `@src/vouch/jsonl_server.py`:
- Around line 99-104: The hybrid branch currently calls
index_db.search_semantic(...) and index_db.search(...) then rrf_fuse(...) but
does not apply the min_score filter nor catch failures from index_db.search;
update the hybrid branch (the block using rrf_fuse, emb, fts,
index_db.search_semantic, index_db.search, and variable used) to mirror the
non-hybrid logic: wrap the index_db.search(...) call in the same try/except used
elsewhere so FTS failures are recovered from, and filter the fused hits by
min_score (or apply the same score-thresholding post-fuse) before assigning hits
and setting used="hybrid". Ensure the behavior matches the documented parameters
and the MCP/non-hybrid path.
🪄 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: 9b23621c-7b8b-4a1b-93c8-c220b4c268fd
📒 Files selected for processing (30)
docs/README.mddocs/embeddings.mddocs/retrieval.mdsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/dedup.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/hyde.pysrc/vouch/embeddings/migration.pysrc/vouch/embeddings/rerank.pysrc/vouch/embeddings/scorer.pysrc/vouch/health.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/test_cli.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.pytests/embeddings/test_integration.pytests/embeddings/test_migration.pytests/embeddings/test_rerank.pytests/embeddings/test_scorer.pytests/embeddings/test_search.pytests/embeddings/test_storage.pytests/test_context.py
✅ Files skipped from review due to trivial changes (3)
- src/vouch/embeddings/hyde.py
- docs/embeddings.md
- docs/README.md
Comment on lines
+511
to
+514
| if backend == "hybrid": | ||
| emb = index_db.search_semantic(store.kb_dir, q, limit=limit * 2) | ||
| fts = index_db.search(store.kb_dir, q, limit=limit * 2) | ||
| hits = rrf_fuse(emb, fts, limit=limit) |
There was a problem hiding this comment.
Apply --min-score in the hybrid branch.
search_semantic() gets the cutoff for auto/embedding, but not for hybrid. vouch search --backend hybrid --min-score ... will still fuse sub-threshold embedding hits, so the flag is silently ineffective on that backend.
Suggested fix
- emb = index_db.search_semantic(store.kb_dir, q, limit=limit * 2)
+ emb = index_db.search_semantic(
+ store.kb_dir, q, limit=limit * 2, min_score=min_score,
+ )
fts = index_db.search(store.kb_dir, q, limit=limit * 2)📝 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 == "hybrid": | |
| emb = index_db.search_semantic(store.kb_dir, q, limit=limit * 2) | |
| fts = index_db.search(store.kb_dir, q, limit=limit * 2) | |
| hits = rrf_fuse(emb, fts, limit=limit) | |
| if backend == "hybrid": | |
| emb = index_db.search_semantic( | |
| store.kb_dir, q, limit=limit * 2, min_score=min_score, | |
| ) | |
| fts = index_db.search(store.kb_dir, q, limit=limit * 2) | |
| hits = rrf_fuse(emb, fts, limit=limit) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/cli.py` around lines 511 - 514, When backend == "hybrid" ensure the
min-score cutoff is applied before fusion: pass the configured min_score/cutoff
into index_db.search_semantic (or filter its results) so embedding hits below
the threshold are excluded prior to calling rrf_fuse; update the hybrid branch
where search_semantic(store.kb_dir, q, limit=...) and index_db.search(...) are
called so that search_semantic receives the same min_score/cutoff parameter (or
apply a post-call filter on the emb list) before calling rrf_fuse(emb, fts,
limit=limit).
Comment on lines
+616
to
+625
| metrics = tuple(m.strip() for m in metric.split(",")) | ||
| canonical = tuple( | ||
| "recall@k" if m.startswith("recall@") else m for m in metrics | ||
| ) | ||
| import contextlib | ||
| k = 10 | ||
| for m in metrics: | ||
| if m.startswith("recall@"): | ||
| with contextlib.suppress(ValueError): | ||
| k = int(m.split("@", 1)[1]) |
There was a problem hiding this comment.
Reject malformed recall@... metrics instead of defaulting to 10.
The suppress(ValueError) path turns --metric recall@foo into recall@10 without telling the caller, which makes the reported score wrong.
Suggested fix
- import contextlib
k = 10
for m in metrics:
if m.startswith("recall@"):
- with contextlib.suppress(ValueError):
- k = int(m.split("@", 1)[1])
+ try:
+ k = int(m.split("@", 1)[1])
+ except ValueError as e:
+ raise click.BadParameter(
+ f"invalid recall metric: {m}; expected recall@<int>",
+ param_hint="--metric",
+ ) from e🤖 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/cli.py` around lines 616 - 625, The code silently falls back to
k=10 when parsing malformed "recall@..." metrics; instead validate and reject
bad values: in the loop that iterates over metrics (the block that sets k from
m.split("@", 1)[1]) replace the contextlib.suppress(ValueError) with explicit
parsing that raises/returns a clear error on invalid integers (e.g., raise
argparse.ArgumentTypeError or call sys.exit with an informative message) so
malformed inputs like "recall@foo" are rejected rather than converted into
recall@10; update any callers that rely on k to handle the validation error
accordingly (refer to the variables/identifiers metrics, canonical, k).
Comment on lines
+27
to
+31
| "INSERT OR REPLACE INTO query_embedding_cache " | ||
| "(query_hash, vec, hit_count, last_used_at) " | ||
| "VALUES (?, ?, COALESCE(" | ||
| " (SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0" | ||
| ") + 1, ?)", |
There was a problem hiding this comment.
hit_count is incremented on write, not just on cache hit.
At Lines 29-31, a cache insert/update bumps hit_count, which makes stats report writes as hits. Initialize from existing value without adding + 1; keep increments in lookup path only.
Suggested fix
conn.execute(
"INSERT OR REPLACE INTO query_embedding_cache "
"(query_hash, vec, hit_count, last_used_at) "
- "VALUES (?, ?, COALESCE("
- " (SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0"
- ") + 1, ?)",
+ "VALUES (?, ?, COALESCE("
+ " (SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0"
+ "), ?)",
(h, blob, h, now),
)📝 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
| "INSERT OR REPLACE INTO query_embedding_cache " | |
| "(query_hash, vec, hit_count, last_used_at) " | |
| "VALUES (?, ?, COALESCE(" | |
| " (SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0" | |
| ") + 1, ?)", | |
| "INSERT OR REPLACE INTO query_embedding_cache " | |
| "(query_hash, vec, hit_count, last_used_at) " | |
| "VALUES (?, ?, COALESCE(" | |
| " (SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0" | |
| "), ?)", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/embeddings/cache.py` around lines 27 - 31, The INSERT OR REPLACE
SQL in the cache module currently adds "+ 1" to hit_count, causing writes to
count as hits; change the VALUES clause to initialize hit_count from
COALESCE((SELECT hit_count FROM query_embedding_cache WHERE query_hash=?), 0)
without adding +1, and move any increment logic so hit_count is incremented only
in the cache retrieval/lookup path (where you read a cached entry), not in the
upsert; reference the query_embedding_cache table and the INSERT OR REPLACE
statement in src/vouch/embeddings/cache.py when making this change.
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| import numpy as np |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the target file around the numpy import
echo "=== dedup.py (relevant section) ==="
if [ -f "src/vouch/embeddings/dedup.py" ]; then
nl -ba src/vouch/embeddings/dedup.py | sed -n '1,120p'
else
echo "Missing: src/vouch/embeddings/dedup.py"
fi
# Search for other numpy imports and any optional-dependency patterns
echo
echo "=== All numpy imports ==="
rg -n "import numpy|from numpy" -S src || true
# Check pyproject/setup for extras mentioning embeddings/numpy
echo
echo "=== Dependency/extras config ==="
ls -la
if [ -f "pyproject.toml" ]; then
rg -n "\\[project\\]|\\[tool\\.poetry\\]|optional-dependencies|extras|embeddings|numpy" pyproject.toml || true
sed -n '1,200p' pyproject.toml || true
fi
if [ -f "setup.cfg" ]; then
rg -n "extras_require|embeddings|numpy" setup.cfg || true
fi
if [ -f "setup.py" ]; then
rg -n "extras_require|embeddings|numpy" setup.py || true
fi
# Find references to 'embeddings' extras in docs/README
echo
echo "=== README/docs references ==="
rg -n "vouch\\[embeddings\\]|embeddings extra|optional dependencies.*embeddings|pip install .*\\[embeddings\\]" -S . || trueRepository: vouchdev/vouch
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== dedup.py (relevant section) ==="
if [ -f "src/vouch/embeddings/dedup.py" ]; then
sed -n '1,140p' src/vouch/embeddings/dedup.py
else
echo "Missing: src/vouch/embeddings/dedup.py"
fi
echo
echo "=== All numpy imports ==="
rg -n "import numpy|from numpy" -S src || true
echo
echo "=== Dependency/extras config ==="
ls -la
if [ -f "pyproject.toml" ]; then
echo "--- pyproject.toml matches ---"
rg -n "\\[project\\]|\\[tool\\.poetry\\]|optional-dependencies|extras|embeddings|numpy" pyproject.toml || true
echo "--- pyproject.toml top excerpt ---"
sed -n '1,200p' pyproject.toml || true
fi
if [ -f "setup.cfg" ]; then
echo "--- setup.cfg matches ---"
rg -n "extras_require|embeddings|numpy" setup.cfg || true
fi
if [ -f "setup.py" ]; then
echo "--- setup.py matches ---"
rg -n "extras_require|embeddings|numpy" setup.py || true
fi
echo
echo "=== README/docs references ==="
rg -n "vouch\\[embeddings\\]|embeddings extra|optional dependencies.*embeddings|pip install .*\\[embeddings\\]" -S . || trueRepository: vouchdev/vouch
Length of output: 9987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Imports of dedup module ==="
rg -n "embeddings\\.dedup|from \\.dedup import|import .*dedup" -S src || true
echo
echo "=== src/vouch/embeddings/__init__.py ==="
sed -n '1,200p' src/vouch/embeddings/__init__.py || true
echo
echo "=== Check embeddings package entry points ==="
ls -ლა src/vouch/embeddings || true
echo
echo "=== numpy usage / try/except patterns in embeddings stack ==="
rg -n "try:.*import numpy|except ImportError.*numpy|require_numpy|optional|embeddings extras" -S src/vouch/embeddings || true
echo
echo "=== Files importing embedding-stack modules ==="
rg -n "vouch\\.embeddings|from vouch\\.embeddings|import vouch\\.embeddings" -S src || trueRepository: vouchdev/vouch
Length of output: 1919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/vouch/storage.py src/vouch/server.py src/vouch/jsonl_server.py src/vouch/cli.py; do
echo "===== $f (context around dedup import) ====="
case "$f" in
src/vouch/storage.py) sed -n '460,540p' "$f" ;;
src/vouch/server.py) sed -n '560,660p' "$f" ;;
src/vouch/jsonl_server.py) sed -n '380,470p' "$f" ;;
src/vouch/cli.py) sed -n '520,620p' "$f" ;;
esac
echo
done
echo "===== src/vouch/server.py imports near top (first 80 lines) ====="
sed -n '1,120p' src/vouch/server.py
echo
echo "===== src/vouch/cli.py imports near top (first 120 lines) ====="
sed -n '1,140p' src/vouch/cli.pyRepository: vouchdev/vouch
Length of output: 20540
Guard dedup’s optional numpy dependency for the dedup tool paths.
src/vouch/embeddings/dedup.py imports numpy at module load, but storage._embed_and_store() already wraps the dedup import (check_and_log) in try/except ImportError, so missing extras won’t break that hook. The unguarded imports happen in the user-invoked paths: kb_dedup_scan (server), _h_dedup_scan (jsonl_server), and vouch cli dedup (cli.py), which will raise an ImportError/trace when numpy isn’t installed. Add a guard/local import (or catch ImportError around from .embeddings.dedup import scan_all) to fail gracefully with an actionable “install vouch[embeddings]” message.
🤖 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/dedup.py` at line 9, The top-level import of numpy in
src/vouch/embeddings/dedup.py causes ImportError at runtime for user-invoked
paths (kb_dedup_scan, _h_dedup_scan, and the CLI dedup command); change that to
a guarded/local import: remove the module-level "import numpy as np" and instead
import numpy inside the functions that need it (or wrap "from .embeddings.dedup
import scan_all" calls in try/except ImportError), and on ImportError raise a
clear RuntimeError instructing the user to install the optional extras (e.g.
"please install vouch[embeddings]"). Update the symbols kb_dedup_scan,
_h_dedup_scan, and the CLI dedup invocation to use the guarded import so missing
numpy fails with the actionable message rather than an unhandled ImportError.
Comment on lines
+408
to
+411
| def _h_reindex_embeddings(p: dict) -> dict: | ||
| from .embeddings.migration import backfill_embeddings | ||
| n = backfill_embeddings(_store(), force=bool(p.get("force", False))) | ||
| return {"touched": n} |
There was a problem hiding this comment.
kb.reindex_embeddings loses parity-critical inputs and outputs.
The JSONL handler ignores the model/backfill knobs and only returns {"touched": ...}. Clients using JSONL cannot request the same reindex behavior as MCP and cannot rely on the same response shape.
🤖 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 408 - 411, The JSONL handler
_h_reindex_embeddings is dropping parity-critical inputs and returning an
incomplete response; update _h_reindex_embeddings to forward model and
backfill/backfill-related flags from the incoming dict p into the call to
backfill_embeddings (e.g., pass model=p.get("model") and
backfill/backfill-specific kwargs) and then return the full response shape that
backfill_embeddings provides (not just {"touched": n}) so clients get the same
outputs as the MCP path; use the existing _store() and backfill_embeddings(...)
symbols to locate and adapt the call and response handling.
Comment on lines
+436
to
+451
| def _h_embeddings_stats(_: dict) -> dict: | ||
| from . import index_db | ||
| from .embeddings.cache import query_cache_stats | ||
| store = _store() | ||
| meta = index_db.get_embedding_meta(store.kb_dir) | ||
| with index_db.open_db(store.kb_dir) as conn: | ||
| counts = { | ||
| k: int(n) for k, n in conn.execute( | ||
| "SELECT kind, COUNT(*) FROM embedding_index GROUP BY kind" | ||
| ) | ||
| } | ||
| return { | ||
| "model": meta.get("embedding_model"), | ||
| "counts": counts, | ||
| "query_cache": query_cache_stats(store.kb_dir), | ||
| } |
There was a problem hiding this comment.
Return the full embeddings-stats payload over JSONL too.
MCP includes model_version and dim, but the JSONL handler drops both fields. That breaks the stated “identical return shapes across transports” guarantee for kb.embeddings_stats.
🤖 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 436 - 451, The JSONL handler
_h_embeddings_stats currently omits embedding metadata fields that MCP returns;
update _h_embeddings_stats to include the full payload by reading model_version
and dim from index_db.get_embedding_meta(store.kb_dir) (the existing meta
variable) and returning them alongside "model", "counts", and "query_cache"
(coerce dim to int if needed), so the return dict mirrors the
MCP/kb.embeddings_stats shape used elsewhere (refer to _h_embeddings_stats,
index_db.get_embedding_meta, and embeddings.cache.query_cache_stats).
Comment on lines
+609
to
+620
| @mcp.tool() | ||
| def kb_eval_embeddings(*, queries_path: str, k: int = 10) -> dict[str, Any]: | ||
| """Run retrieval eval over a JSONL queries file.""" | ||
| from pathlib import Path | ||
|
|
||
| from .embeddings.scorer import evaluate | ||
| store = _store() | ||
| return evaluate( | ||
| kb_dir=store.kb_dir, | ||
| queries_file=Path(queries_path), | ||
| k=k, | ||
| ) |
There was a problem hiding this comment.
Expose the metric selector on the MCP eval tool.
The PR surface promises kb_eval_embeddings parity with the CLI/JSONL eval flow, but this handler hardcodes evaluate() defaults and gives callers no way to request a specific metric. That makes mrr/ndcg-only evaluation impossible over MCP.
🤖 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 609 - 620, The MCP tool kb_eval_embeddings
currently calls evaluate() with defaults and doesn't allow selecting the
evaluation metric; update the kb_eval_embeddings signature to accept a metric
parameter (e.g., metric: str | None = None or metric: Literal["mrr","ndcg"] =
"mrr"), document it in the docstring, and pass that metric through to
embeddings.scorer.evaluate(kb_dir=..., queries_file=Path(queries_path), k=k,
metric=metric) so callers can request mrr/ndcg-only evaluation; ensure the
parameter name matches the evaluate() argument and keep imports and the _store()
usage unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (17)
src/vouch/health.py (1)
169-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep mismatch auditing strictly non-fatal.
Line 172 and Line 174 can still raise non-import runtime exceptions, which would fail
rebuild_index()even though this check is intended to be best-effort.Proposed minimal fix
- try: - from . import audit - from .embeddings.migration import detect_mismatch - m = detect_mismatch(store.kb_dir) - if m is not None: - audit.log_event( - store.kb_dir, event="embedding.model_mismatch", - actor="vouch-health", - object_ids=[], data=m, - ) - except ImportError: - pass + try: + from . import audit + from .embeddings.migration import detect_mismatch + except ImportError: + pass + else: + try: + m = detect_mismatch(store.kb_dir) + if m is not None: + audit.log_event( + store.kb_dir, + event="embedding.model_mismatch", + actor="vouch-health", + object_ids=[], + data=m, + ) + except Exception: + pass🤖 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/health.py` around lines 169 - 180, The mismatch detection/auditing block (detect_mismatch and audit.log_event using store.kb_dir) must be strictly non-fatal: replace the current try/except ImportError guard with a broader try/except that catches all runtime exceptions (e.g., Exception) around the calls to detect_mismatch and audit.log_event, swallow the exception (or log it at debug/trace) and do not re-raise so rebuild_index() continues even if detection or logging fails.src/vouch/jsonl_server.py (2)
99-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the hybrid FTS leg so one backend failure doesn't abort the call.
Line 104 is unprotected and can bubble an exception as
internal_error; hybrid should degrade gracefully like the auto path.Proposed fix
if backend_arg == "hybrid": from .embeddings.fusion import ( # type: ignore[import-not-found,import-untyped,unused-ignore] 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) + try: + fts = index_db.search(s.kb_dir, q, limit=limit * 2) + except Exception: + fts = [] 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 calls index_db.search_semantic and index_db.search and then rrf_fuse without error handling, so any exception can bubble up as an internal_error; wrap the hybrid FTS leg (the calls to index_db.search_semantic, index_db.search and rrf_fuse) in a try/except that mirrors the auto path’s graceful degradation: on exception log the error, fall back to the non-hybrid result(s) or an empty hit set, set used appropriately (e.g., "hybrid_fallback" or "vector"/"fts"), and ensure the function returns a valid hits value instead of raising. Reference the symbols backend_arg, index_db.search_semantic, index_db.search, and rrf_fuse when locating the code to modify.
410-453:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJSONL embedding handlers still miss parity-critical params/fields.
Line 412 drops
model/backfillinputs, Line 431 ignores requested metric selection, and Lines 449-453 omit embedding metadata fields expected for transport parity.🤖 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 410 - 453, _h_reindex_embeddings, _h_dedup_scan, and _h_embeddings_stats are missing parity-critical params/fields: pass the requested model and backfill flag from p into backfill_embeddings in _h_reindex_embeddings (use p.get("model") and p.get("backfill")/force mapping), allow metric selection from p (e.g., p.get("metric")) into scan_all in _h_dedup_scan instead of hardcoding behavior, and extend the return of _h_embeddings_stats to include full embedding metadata from index_db.get_embedding_meta (embedding_model, embedding_dimensions, embedding_metric, any backfill/index_version fields) alongside counts and query_cache_stats so transport consumers receive the expected fields; use the existing symbols backfill_embeddings, scan_all, index_db.get_embedding_meta, and query_cache_stats to locate and update the calls and returned dicts.src/vouch/capabilities.py (1)
56-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdvertised embedding methods should be reflected in retrieval modes.
Line 67 still reports lexical-only retrieval, so capability consumers can mis-detect semantic/hybrid support despite the newly advertised embedding methods.
Proposed fix
def capabilities() -> Capabilities: return Capabilities( version=__version__, methods=METHODS, - retrieval=["fts5", "substring"], + retrieval=["embedding", "hybrid", "fts5", "substring"], review_gated=True, transports=["mcp", "jsonl"], )Also applies to: 63-68
🤖 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/capabilities.py` around lines 56 - 59, The capabilities list advertises embedding-related methods ("kb.reindex_embeddings", "kb.dedup_scan", "kb.eval_embeddings", "kb.embeddings_stats") but the retrieval description still states "lexical-only", which misleads capability consumers; update the retrieval mode reported (the retrieval description string/field in the same capabilities structure—search for the string "lexical-only" or the retrieval_modes/ retrieval field in this file) to reflect semantic or hybrid support (either change the static text to include "semantic"/"hybrid" or compute it dynamically when embedding capabilities are present) so the advertised embedding methods and the retrieval mode stay consistent.src/vouch/embeddings/hyde.py (1)
20-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle LLM failures with a safe fallback.
llm(prompt)exceptions currently bubble and can fail query expansion; fall back to the original query on provider errors.Proposed fix
def expand_query_with_llm(query: str, *, llm: Callable[[str], str]) -> str: @@ - return llm(prompt).strip() or query + try: + expanded = llm(prompt).strip() + except Exception: + return query + return expanded or query🤖 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/hyde.py` around lines 20 - 27, The expand_query_with_llm function currently lets exceptions from llm(prompt) propagate; wrap the llm call in a try/except around the call to llm(prompt) inside expand_query_with_llm (catch Exception) and on any error return the original query as a safe fallback, otherwise return llm(prompt).strip() as before; reference the function name expand_query_with_llm and the llm(prompt) invocation when making the change.src/vouch/context.py (1)
83-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn annotation should match the actual return value.
Line 84 includes
ContextPackin the return type, but this function always returns adict(pack.model_dump()+ fields).Proposed fix
def build_context_pack( @@ - explain: bool = False, -) -> ContextPack | dict[str, Any]: + explain: bool = False, +) -> dict[str, Any]:🤖 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/context.py` around lines 83 - 84, The return annotation on the function that currently reads "ContextPack | dict[str, Any]" is incorrect because the function always returns a dict (it builds the result from pack.model_dump() plus extra fields). Update the function signature to return only dict[str, Any] (remove ContextPack) and ensure any references to the function's return type (e.g., callers or type hints) expect a dict; locate the function by the symbol "pack.model_dump" usage and the current return annotation to find the exact spot to change.src/vouch/embeddings/dedup.py (1)
9-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional
numpyimport for dedup entry paths.Importing
numpyat module load can fail before dedup commands return a helpful install hint. Prefer guarded/local import and raise a clear runtime message (pip install vouch[embeddings]).Proposed fix pattern
-import numpy as np +try: + import numpy as np +except ImportError: # optional dependency + np = None # type: ignore[assignment] @@ +def _require_numpy() -> None: + if np is None: + raise RuntimeError( + "Embeddings dedup requires optional dependencies. " + "Install with: pip install vouch[embeddings]" + ) @@ def check_and_log( @@ ) -> tuple[str, float] | None: + _require_numpy() @@ def scan_all( @@ ) -> list[dict[str, Any]]: + _require_numpy()Run this to confirm current import/call-site behavior and error handling in entry paths:
#!/bin/bash set -euo pipefail echo "== dedup module top ==" nl -ba src/vouch/embeddings/dedup.py | sed -n '1,80p' echo echo "== dedup call sites in user-facing surfaces ==" rg -n -C2 'scan_all\(|list_duplicates\(|check_and_log\(|embeddings\.dedup' \ src/vouch/cli.py src/vouch/server.py src/vouch/jsonl_server.py echo echo "== existing ImportError/RuntimeError handling around dedup calls ==" rg -n -C2 'ImportError|RuntimeError|vouch\[embeddings\]' \ src/vouch/cli.py src/vouch/server.py src/vouch/jsonl_server.pyAlso applies to: 56-88
🤖 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/dedup.py` around lines 9 - 10, The top-level numpy import in src/vouch/embeddings/dedup.py should be guarded so importing the module (for non-embeddings entry paths) doesn't raise; move the numpy import into the functions that require it (e.g., inside functions referenced by scan_all, list_duplicates, and any helper that uses np) or wrap it in a try/except ImportError and re-raise a clear RuntimeError instructing the user to "pip install vouch[embeddings]"; update all uses in the module to rely on the local import/variable and ensure the error message includes that installation hint and the original ImportError details for debugging.src/vouch/cli.py (3)
511-513:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
--min-scorein the hybrid semantic leg.Hybrid currently ignores the configured score cutoff for embedding hits.
Proposed fix
- emb = index_db.search_semantic(store.kb_dir, q, limit=limit * 2) + emb = index_db.search_semantic( + store.kb_dir, q, limit=limit * 2, min_score=min_score + )🤖 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/cli.py` around lines 511 - 513, In the hybrid branch, apply the --min-score cutoff to the semantic hits returned by index_db.search_semantic: either pass the min_score argument into search_semantic (if it supports a score threshold) or filter the returned emb list (variable emb) to only keep items with score >= min_score before merging with fts; update the hybrid branch where backend == "hybrid" so the emb results respect min_score (refer to index_db.search_semantic, emb, fts, and the min_score CLI variable).
651-655:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--modelis validated but not used for backfill.Backfill still runs with default embedder, so the selected model is ineffective.
Proposed fix sketch
if embeddings or backfill: from .embeddings.migration import backfill_embeddings + embedder = None if model: from .embeddings import get_embedder - get_embedder(model) - n = backfill_embeddings(store, force=force) + embedder = get_embedder(model) + n = backfill_embeddings(store, force=force, embedder=embedder)🤖 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/cli.py` around lines 651 - 655, The CLI validates --model by calling get_embedder(model) but never uses that embedder for backfill, so backfill_embeddings(store, force=force) still uses the default embedder; fix by obtaining the embedder and passing it into the backfill routine (e.g., embedder = get_embedder(model) and call backfill_embeddings(store, embedder=embedder, force=force)) or update backfill_embeddings to accept a model_name parameter and forward it to the embedder; change references to backfill_embeddings in this file to pass the embedder/model so the selected model is actually used.
620-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed
recall@...metrics instead of silently defaulting to 10.
--metric recall@fooshould fail fast; current path reports incorrect results.Proposed fix
- import contextlib k = 10 for m in metrics: if m.startswith("recall@"): - with contextlib.suppress(ValueError): - k = int(m.split("@", 1)[1]) + try: + k = int(m.split("@", 1)[1]) + except ValueError as e: + raise click.BadParameter( + f"invalid recall metric: {m}; expected recall@<int>", + param_hint="--metric", + ) from e🤖 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/cli.py` around lines 620 - 625, The loop that parses "recall@..." metrics silently swallows malformed values using contextlib.suppress, causing invalid inputs to default to k=10; replace the suppress with explicit validation: for each m in metrics if m.startswith("recall@") attempt to parse int(m.split("@",1)[1]) in a try/except and on failure raise a clear error (e.g., raise ValueError or call the CLI parser's error) mentioning the invalid metric string m so the CLI fails fast instead of falling back to k=10; update the parsing code around variables metrics, m, and k and remove contextlib.suppress.src/vouch/server.py (3)
611-622:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose metric selection on
kb_eval_embeddingsfor CLI/MCP parity.This tool currently hardcodes scorer defaults and can’t run metric-specific evals.
🤖 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 611 - 622, The CLI tool kb_eval_embeddings currently uses scorer defaults and should accept a metric parameter; add a new keyword argument metric: str | None = None to kb_eval_embeddings (update the signature and docstring) and pass metric=metric into the call to evaluate (from .embeddings.scorer.evaluate) so callers via MCP/CLI can request metric-specific evaluations; ensure type hinting and default preserve current behavior when metric is None.
130-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden hybrid search: honor
min_scoreand guard FTS failures.Hybrid currently ignores the semantic cutoff and can fail hard if FTS errors.
Proposed fix
- emb = index_db.search_semantic(store.kb_dir, query, limit=limit * 2) - fts = index_db.search(store.kb_dir, query, limit=limit * 2) + emb = index_db.search_semantic( + store.kb_dir, query, limit=limit * 2, min_score=min_score + ) + try: + fts = index_db.search(store.kb_dir, query, limit=limit * 2) + except Exception: + fts = [] hits = rrf_fuse(emb, fts, limit=limit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/server.py` around lines 130 - 132, Filter semantic results by the configured min_score and protect against FTS errors before fusion: when calling index_db.search_semantic (emb) apply a filter to drop any result with score < min_score (still pass up to limit*2 entries), wrap the index_db.search (fts) call in a try/except and fall back to an empty list on failure, then call rrf_fuse(emb_filtered, fts_safe, limit=limit) (referencing emb, fts, index_db.search_semantic, index_db.search, rrf_fuse, min_score, limit, store.kb_dir).
593-597:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
modelparameter is validated but not applied during backfill.
kb_reindex_embeddings(model=...)still backfills with default embedder.🤖 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 593 - 597, The code validates a model but doesn't apply it to the backfill; calling get_embedder(model) alone doesn't change how backfill_embeddings runs. Update usage and signature so the selected model is actually used: call get_embedder(model) and pass the model (or the embedder) into backfill_embeddings (e.g., backfill_embeddings(store, force=force, model=model) or backfill_embeddings(store, force=force, embedder=returned_embedder)) and update backfill_embeddings' definition to accept and use that parameter; also ensure _current_model_name() reflects the applied model if needed.src/vouch/embeddings/scorer.py (1)
56-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip blank JSONL lines before decoding.
json.loads(line)will fail on empty/whitespace-only lines and makeevalbrittle for hand-edited files.Proposed fix
with queries_file.open() as f: for line in f: + if not line.strip(): + continue row = json.loads(line)🤖 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/scorer.py` around lines 56 - 59, In the loop that reads queries_file (the block starting with "with queries_file.open() as f:"), skip any empty or whitespace-only lines before calling json.loads to avoid JSONDecodeError; e.g., check if line.strip() is falsy and continue, then proceed to parse into row and extract q (variables: line, row, q) so hand-edited JSONL with blank lines is ignored.src/vouch/embeddings/migration.py (2)
38-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid broad
AttributeErrorhandlers around entire loops.This can hide genuine migration failures in item field access and silently skip work.
🤖 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/migration.py` around lines 38 - 80, The loops over store.list_claims/list_pages/list_sources/list_entities/list_relations/list_evidence currently wrap the entire iteration in a broad AttributeError which can hide real bugs; instead, remove the outer try/except and perform per-item defensive checks (use hasattr/getattr with defaults or try/except around the single access/_embed_and_store call) inside each loop so failures for one item don't skip the whole collection; specifically update each block that calls store._embed_and_store (and accesses fields like c.text, p.title, p.body, s.title, s.locator, e.name, e.description, r.source, r.relation.value, r.target, ev.quote) to validate or safely access those attributes and handle AttributeError locally.
34-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
forceis currently a no-op in backfill logic.This breaks the expected
--forcebehavior for re-encoding unchanged artifacts.🤖 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/migration.py` around lines 34 - 35, The backfill_embeddings function currently ignores the force parameter so unchanged artifacts are never re-encoded; update backfill_embeddings to honor force by altering the artifact-processing loop to bypass the "skip if embedding exists" branch when force is True (or by treating existing embeddings as stale when force is True), ensure the method that checks for existing embeddings (the local variable/condition inside backfill_embeddings that decides "already has embedding") uses force to force re-encoding, and make sure the returned count still reflects artifacts actually re-encoded/touched.src/vouch/embeddings/rerank.py (1)
43-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail loudly when reranker score count mismatches hit count.
Current
zip(..., strict=False)can silently truncate results.Proposed fix
candidates = [h[2] or h[1] for h in hits] scores = reranker.score(query, candidates) + if len(scores) != len(hits): + raise ValueError( + f"reranker returned {len(scores)} scores for {len(hits)} hits" + ) reranked = [ - (kind, id_, snip, score) - for (kind, id_, snip, _orig), score in zip(hits, scores, strict=False) + (kind, id_, snip, scores[i]) + for i, (kind, id_, snip, _orig) in enumerate(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/embeddings/rerank.py` around lines 43 - 47, The current reranking uses zip(hits, scores, strict=False) which can silently drop entries when counts differ; update the logic in rerank.py around reranker.score, scores, and the list comprehension building reranked so it fails loudly on length mismatch — either use zip(..., strict=True) or explicitly compare len(hits) and len(scores) after calling reranker.score and raise a clear ValueError including both lengths and contextual info (query/candidate count) before building reranked.
🧹 Nitpick comments (1)
tests/test_context.py (1)
67-80: ⚡ Quick winStrengthen the semantic-default assertion.
This test currently verifies result presence, which can still pass via fallback retrieval. Assert the selected backend is semantic to lock the intent.
Proposed test tweak
def test_build_context_pack_uses_semantic_default(tmp_path: Path) -> None: @@ pack = build_context_pack(store, query="exact query string", limit=5) assert any(item["id"] == "c1" for item in pack.get("items", [])) + assert pack.get("backend") == "embedding"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_context.py` around lines 67 - 80, Update the test_build_context_pack_uses_semantic_default to assert that the pack explicitly indicates the semantic backend was used (not just that the expected item is present); after calling build_context_pack(registering DEFAULT_MODEL_NAME with MockEmbedder and creating the store), add an assertion that the returned pack's backend marker (e.g., pack.get("backend") or pack.get("strategy")) equals "semantic" (or the codebase's canonical semantic identifier), while still keeping the existing assertion that "c1" appears in pack["items"]; reference build_context_pack and DEFAULT_MODEL_NAME to locate the test and ensure the new assertion matches the pack's actual metadata key.
🤖 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/cli.py`:
- Line 548: The call _emit_json(pack) is serializing the ContextPack model by
stringifying it (via default=str) instead of emitting structured JSON fields;
change the call so _emit_json receives a plain serializable dict (e.g.,
pack.dict() or dataclasses.asdict(pack) / manual mapping of ContextPack fields)
and update _emit_json if necessary to use json.dump(s) without default=str;
specifically locate the ContextPack instance and replace passing the model
object with its serializable representation (ContextPack.dict() or equivalent)
so JSON output contains structured fields rather than a stringified model.
---
Duplicate comments:
In `@src/vouch/capabilities.py`:
- Around line 56-59: The capabilities list advertises embedding-related methods
("kb.reindex_embeddings", "kb.dedup_scan", "kb.eval_embeddings",
"kb.embeddings_stats") but the retrieval description still states
"lexical-only", which misleads capability consumers; update the retrieval mode
reported (the retrieval description string/field in the same capabilities
structure—search for the string "lexical-only" or the retrieval_modes/ retrieval
field in this file) to reflect semantic or hybrid support (either change the
static text to include "semantic"/"hybrid" or compute it dynamically when
embedding capabilities are present) so the advertised embedding methods and the
retrieval mode stay consistent.
In `@src/vouch/cli.py`:
- Around line 511-513: In the hybrid branch, apply the --min-score cutoff to the
semantic hits returned by index_db.search_semantic: either pass the min_score
argument into search_semantic (if it supports a score threshold) or filter the
returned emb list (variable emb) to only keep items with score >= min_score
before merging with fts; update the hybrid branch where backend == "hybrid" so
the emb results respect min_score (refer to index_db.search_semantic, emb, fts,
and the min_score CLI variable).
- Around line 651-655: The CLI validates --model by calling get_embedder(model)
but never uses that embedder for backfill, so backfill_embeddings(store,
force=force) still uses the default embedder; fix by obtaining the embedder and
passing it into the backfill routine (e.g., embedder = get_embedder(model) and
call backfill_embeddings(store, embedder=embedder, force=force)) or update
backfill_embeddings to accept a model_name parameter and forward it to the
embedder; change references to backfill_embeddings in this file to pass the
embedder/model so the selected model is actually used.
- Around line 620-625: The loop that parses "recall@..." metrics silently
swallows malformed values using contextlib.suppress, causing invalid inputs to
default to k=10; replace the suppress with explicit validation: for each m in
metrics if m.startswith("recall@") attempt to parse int(m.split("@",1)[1]) in a
try/except and on failure raise a clear error (e.g., raise ValueError or call
the CLI parser's error) mentioning the invalid metric string m so the CLI fails
fast instead of falling back to k=10; update the parsing code around variables
metrics, m, and k and remove contextlib.suppress.
In `@src/vouch/context.py`:
- Around line 83-84: The return annotation on the function that currently reads
"ContextPack | dict[str, Any]" is incorrect because the function always returns
a dict (it builds the result from pack.model_dump() plus extra fields). Update
the function signature to return only dict[str, Any] (remove ContextPack) and
ensure any references to the function's return type (e.g., callers or type
hints) expect a dict; locate the function by the symbol "pack.model_dump" usage
and the current return annotation to find the exact spot to change.
In `@src/vouch/embeddings/dedup.py`:
- Around line 9-10: The top-level numpy import in src/vouch/embeddings/dedup.py
should be guarded so importing the module (for non-embeddings entry paths)
doesn't raise; move the numpy import into the functions that require it (e.g.,
inside functions referenced by scan_all, list_duplicates, and any helper that
uses np) or wrap it in a try/except ImportError and re-raise a clear
RuntimeError instructing the user to "pip install vouch[embeddings]"; update all
uses in the module to rely on the local import/variable and ensure the error
message includes that installation hint and the original ImportError details for
debugging.
In `@src/vouch/embeddings/hyde.py`:
- Around line 20-27: The expand_query_with_llm function currently lets
exceptions from llm(prompt) propagate; wrap the llm call in a try/except around
the call to llm(prompt) inside expand_query_with_llm (catch Exception) and on
any error return the original query as a safe fallback, otherwise return
llm(prompt).strip() as before; reference the function name expand_query_with_llm
and the llm(prompt) invocation when making the change.
In `@src/vouch/embeddings/migration.py`:
- Around line 38-80: The loops over
store.list_claims/list_pages/list_sources/list_entities/list_relations/list_evidence
currently wrap the entire iteration in a broad AttributeError which can hide
real bugs; instead, remove the outer try/except and perform per-item defensive
checks (use hasattr/getattr with defaults or try/except around the single
access/_embed_and_store call) inside each loop so failures for one item don't
skip the whole collection; specifically update each block that calls
store._embed_and_store (and accesses fields like c.text, p.title, p.body,
s.title, s.locator, e.name, e.description, r.source, r.relation.value, r.target,
ev.quote) to validate or safely access those attributes and handle
AttributeError locally.
- Around line 34-35: The backfill_embeddings function currently ignores the
force parameter so unchanged artifacts are never re-encoded; update
backfill_embeddings to honor force by altering the artifact-processing loop to
bypass the "skip if embedding exists" branch when force is True (or by treating
existing embeddings as stale when force is True), ensure the method that checks
for existing embeddings (the local variable/condition inside backfill_embeddings
that decides "already has embedding") uses force to force re-encoding, and make
sure the returned count still reflects artifacts actually re-encoded/touched.
In `@src/vouch/embeddings/rerank.py`:
- Around line 43-47: The current reranking uses zip(hits, scores, strict=False)
which can silently drop entries when counts differ; update the logic in
rerank.py around reranker.score, scores, and the list comprehension building
reranked so it fails loudly on length mismatch — either use zip(...,
strict=True) or explicitly compare len(hits) and len(scores) after calling
reranker.score and raise a clear ValueError including both lengths and
contextual info (query/candidate count) before building reranked.
In `@src/vouch/embeddings/scorer.py`:
- Around line 56-59: In the loop that reads queries_file (the block starting
with "with queries_file.open() as f:"), skip any empty or whitespace-only lines
before calling json.loads to avoid JSONDecodeError; e.g., check if line.strip()
is falsy and continue, then proceed to parse into row and extract q (variables:
line, row, q) so hand-edited JSONL with blank lines is ignored.
In `@src/vouch/health.py`:
- Around line 169-180: The mismatch detection/auditing block (detect_mismatch
and audit.log_event using store.kb_dir) must be strictly non-fatal: replace the
current try/except ImportError guard with a broader try/except that catches all
runtime exceptions (e.g., Exception) around the calls to detect_mismatch and
audit.log_event, swallow the exception (or log it at debug/trace) and do not
re-raise so rebuild_index() continues even if detection or logging fails.
In `@src/vouch/jsonl_server.py`:
- Around line 99-106: The hybrid branch currently calls index_db.search_semantic
and index_db.search and then rrf_fuse without error handling, so any exception
can bubble up as an internal_error; wrap the hybrid FTS leg (the calls to
index_db.search_semantic, index_db.search and rrf_fuse) in a try/except that
mirrors the auto path’s graceful degradation: on exception log the error, fall
back to the non-hybrid result(s) or an empty hit set, set used appropriately
(e.g., "hybrid_fallback" or "vector"/"fts"), and ensure the function returns a
valid hits value instead of raising. Reference the symbols backend_arg,
index_db.search_semantic, index_db.search, and rrf_fuse when locating the code
to modify.
- Around line 410-453: _h_reindex_embeddings, _h_dedup_scan, and
_h_embeddings_stats are missing parity-critical params/fields: pass the
requested model and backfill flag from p into backfill_embeddings in
_h_reindex_embeddings (use p.get("model") and p.get("backfill")/force mapping),
allow metric selection from p (e.g., p.get("metric")) into scan_all in
_h_dedup_scan instead of hardcoding behavior, and extend the return of
_h_embeddings_stats to include full embedding metadata from
index_db.get_embedding_meta (embedding_model, embedding_dimensions,
embedding_metric, any backfill/index_version fields) alongside counts and
query_cache_stats so transport consumers receive the expected fields; use the
existing symbols backfill_embeddings, scan_all, index_db.get_embedding_meta, and
query_cache_stats to locate and update the calls and returned dicts.
In `@src/vouch/server.py`:
- Around line 611-622: The CLI tool kb_eval_embeddings currently uses scorer
defaults and should accept a metric parameter; add a new keyword argument
metric: str | None = None to kb_eval_embeddings (update the signature and
docstring) and pass metric=metric into the call to evaluate (from
.embeddings.scorer.evaluate) so callers via MCP/CLI can request metric-specific
evaluations; ensure type hinting and default preserve current behavior when
metric is None.
- Around line 130-132: Filter semantic results by the configured min_score and
protect against FTS errors before fusion: when calling index_db.search_semantic
(emb) apply a filter to drop any result with score < min_score (still pass up to
limit*2 entries), wrap the index_db.search (fts) call in a try/except and fall
back to an empty list on failure, then call rrf_fuse(emb_filtered, fts_safe,
limit=limit) (referencing emb, fts, index_db.search_semantic, index_db.search,
rrf_fuse, min_score, limit, store.kb_dir).
- Around line 593-597: The code validates a model but doesn't apply it to the
backfill; calling get_embedder(model) alone doesn't change how
backfill_embeddings runs. Update usage and signature so the selected model is
actually used: call get_embedder(model) and pass the model (or the embedder)
into backfill_embeddings (e.g., backfill_embeddings(store, force=force,
model=model) or backfill_embeddings(store, force=force,
embedder=returned_embedder)) and update backfill_embeddings' definition to
accept and use that parameter; also ensure _current_model_name() reflects the
applied model if needed.
---
Nitpick comments:
In `@tests/test_context.py`:
- Around line 67-80: Update the test_build_context_pack_uses_semantic_default to
assert that the pack explicitly indicates the semantic backend was used (not
just that the expected item is present); after calling
build_context_pack(registering DEFAULT_MODEL_NAME with MockEmbedder and creating
the store), add an assertion that the returned pack's backend marker (e.g.,
pack.get("backend") or pack.get("strategy")) equals "semantic" (or the
codebase's canonical semantic identifier), while still keeping the existing
assertion that "c1" appears in pack["items"]; reference build_context_pack and
DEFAULT_MODEL_NAME to locate the test and ensure the new assertion matches the
pack's actual metadata key.
🪄 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: 8ff7c4a0-b0f8-4bfc-b8ef-39b3471ae630
📒 Files selected for processing (26)
docs/README.mddocs/embeddings.mddocs/retrieval.mdsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/dedup.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/hyde.pysrc/vouch/embeddings/migration.pysrc/vouch/embeddings/rerank.pysrc/vouch/embeddings/scorer.pysrc/vouch/health.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/embeddings/test_cli.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.pytests/embeddings/test_integration.pytests/embeddings/test_migration.pytests/embeddings/test_rerank.pytests/embeddings/test_scorer.pytests/embeddings/test_search.pytests/test_context.py
✅ Files skipped from review due to trivial changes (3)
- docs/README.md
- docs/embeddings.md
- docs/retrieval.md
| min_items=min_items, require_citations=require_citations, | ||
| ) | ||
| _emit_json(pack.model_dump(mode="json")) | ||
| _emit_json(pack) |
There was a problem hiding this comment.
Serialize ContextPack as structured JSON, not via default=str.
Passing pack directly risks stringifying the model instead of emitting JSON fields.
Proposed fix
- _emit_json(pack)
+ _emit_json(pack.model_dump(mode="json") if hasattr(pack, "model_dump") else pack)📝 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
| _emit_json(pack) | |
| _emit_json(pack.model_dump(mode="json") if hasattr(pack, "model_dump") else pack) |
🤖 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/cli.py` at line 548, The call _emit_json(pack) is serializing the
ContextPack model by stringifying it (via default=str) instead of emitting
structured JSON fields; change the call so _emit_json receives a plain
serializable dict (e.g., pack.dict() or dataclasses.asdict(pack) / manual
mapping of ContextPack fields) and update _emit_json if necessary to use
json.dump(s) without default=str; specifically locate the ContextPack instance
and replace passing the model object with its serializable representation
(ContextPack.dict() or equivalent) so JSON output contains structured fields
rather than a stringified model.
This was referenced May 21, 2026
Closed
14 tasks
Collaborator
|
Fix the conflict or close this PR in 12hr |
Collaborator
|
Fix conflict |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mypy on Phase 7's _enrich_summary flagged `e.description[:200]` because `Entity.description` is `str | None` and `None` isn't indexable. Same fallback pattern as the page branch above: `(value or "")[:200]`.
Per CodeRabbit Critical on PR vouchdev#43: tests/test_context.py imports `tests.embeddings._fakes.MockEmbedder` at module top, and _fakes.py imports numpy. CI's base `[dev]` install lacks numpy, so test collection of test_context.py fails before any test runs. Move the MockEmbedder import inside the `_mock_embedder` autouse fixture and guard with `pytest.importorskip("numpy")`. Tests that depend on the fixture skip cleanly when numpy is unavailable; existing context tests that don't actually need it (none currently, since the fixture is autouse) still skip with a clear reason rather than crash collection.
dripsmvcp
force-pushed
the
feat/semantic-search-phase-8-cli-docs
branch
from
daa6582 to
5d07655
Compare
3 tasks
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 8 makes the feature usable. Adds CLI flags + new commands for everything built in earlier phases, mirrors them in MCP and JSONL, wires the model-mismatch warning into
health.rebuild_index, adds the headline end-to-end integration test, and ships user-facing docs.Tracks Tasks 24-32 in the plan.
Commits
e2a5ad7vouch search--semantic,--backend,--top-k,--min-score,--rerank,--hyde,--explain5afdaf7vouch reindex--embeddings,--backfill,--force,--model NAME2077c23vouch dedup--threshold,--dry-run— scans embeddings for cross-artifact near-duplicates33ef4bfvouch eval embedding--queries PATH(JSONL),--metric recall@K,mrr,ndcgabeeed4vouch embeddings stats32d04e4kb_reindex_embeddings,kb_dedup_scan,kb_eval_embeddings,kb_embeddings_stats— identical params across transports9d96dc5health.rebuild_indexembedding.model_mismatchaudit event whenindex_meta.embedding_modeldiffers from the registered default0f2b35f@pytest.mark.integration) asserting a lexically-disjoint query finds the semantically-matching claim under the realall-mpnet-base-v2model — the headline acceptance criterion5c03701docs/embeddings.md+ cross-link indocs/retrieval.md+ index entry indocs/README.md5fddf2cstyle:ruff cleanup + adds the 4 new JSONL methods tocapabilities.METHODS(test_capabilities.pyenforces parity between handlers and the capabilities dict)Design notes
Python identifier vs CLI name. The
vouch eval embeddingcommand group is registered with Click asname="eval"but the Python function iseval_groupto avoid shadowing the builtin and to keep static analysers / security linters quiet. Same for the embeddings group.MCP/JSONL parity is structural. Every JSONL
kb.<name>method has a matchingkb_<name>MCP tool with identical param names and return shape. The new methods are also registered insrc/vouch/capabilities.py::METHODSbecausetests/test_capabilities.pyasserts the two dicts are in sync.Mismatch detection is non-fatal.
health.rebuild_indexcallsdetect_mismatchafter the FTS5 rebuild and, if a mismatch is found, emits aembedding.model_mismatchaudit event with both stored and current model identities. The KB continues to operate; the operator gets a clear signal that avouch reindex --embeddings --backfillis in order.Acceptance criterion test (from the spec)
test_semantic_search_finds_lexically_disjoint_claim(Task 31, integration-marked) populates a KB with two claims —"login flow uses session cookies signed by the API"and"the sun is large and hot"— then queries"how do we authenticate users". With the realall-mpnet-base-v2model loaded, the auth claim must rank first. This is the headline criterion from the spec; it runs underpytest -m integrationafterpip install vouch[embeddings].Schema / API deviations from plan body
KBStore.discover(kb)in the plan body — the actual API isdiscover_root(start) -> Pathfromvouch.storage. Test adapted.from pathlib import Pathfor type annotations (not in the plan snippet).capabilities.py::METHODSupdated to match the 4 new JSONL handlers (a hard requirement of the existingtest_capabilities.pyparity test — not flagged in the plan but caught by the test).Test Plan
.venv/bin/pytest tests/embeddings -v→ 62 passed, 5 deselected (5 integration-marked end-to-end tests excluded by default).venv/bin/pytest --ignore=tests/test_sessions.py -q→ 127 passed.venv/bin/python -m ruff check src/vouch tests/embeddings→ clean.venv/bin/pytest -m integration -v— runs in CI withpip install vouch[embeddings]; verifies the headline acceptance criterionDocumentation
docs/embeddings.md(new) — user guide covering install, default behavior, all CLI surfaces, maintenance commandsdocs/retrieval.md— appended cross-linkdocs/README.md— added one line under existing entriesSummary by CodeRabbit
New Features
Documentation
Tests