feat(embeddings): semantic-default build_context_pack with --explain#43
Merged
plind-junior merged 4 commits intoMay 25, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors context pack serialization and enriches hit retrieval with backend tracking. Hit retrieval now labels each result with its retrieval backend (embedding, FTS5, substring), summaries are enriched from stored artifacts when missing, and context packs optionally include per-hit metadata. JSON serialization is deferred to the presentation layer, and search dispatch now accepts explicit backend routing parameters. ChangesBackend tracking and context pack enrichment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/vouch/embeddings/dedup.py (1)
74-76: 💤 Low valueDot product assumes unit vectors for cosine similarity.
vecs[k1] @ vecs[k2]equals cosine similarity only when vectors are L2-normalized. If the embedder produces non-normalized vectors, scores will be incorrect. Consider adding a note or normalizing explicitly if this invariant isn't guaranteed upstream.🤖 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 74 - 76, The code treats vecs[k1] @ vecs[k2] as cosine similarity but that only holds for L2-normalized vectors; update the logic around the line where cos = float(vecs[k1] @ vecs[k2]) to compute true cosine similarity by either L2-normalizing vectors (e.g., normalize every vector in vecs once before the pairwise loop) or replace the dot with dot/(norm(v1)*norm(v2)) using the same vecs, and keep references to vecs, k1, k2 and the cos variable so the change is applied exactly where the pairwise similarity is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 213-227: The heading "13. Test plan (~900 LOC across 9 files)" is
out of sync with the table which lists 10 test files (notably
`tests/test_embeddings_cli.py`); update the heading to the correct file count
(change "9 files" to "10 files") or remove the extraneous table row if
`tests/test_embeddings_cli.py` should not be included so the heading and table
match. Ensure you edit the heading text "13. Test plan (~900 LOC across 9
files)" and/or the table entry for `tests/test_embeddings_cli.py` accordingly.
- Around line 33-47: The fenced code block listing the embeddings module files
triggers markdownlint MD040 because it has no language; update the opening fence
from ``` to ```text (or another appropriate language token) so the block becomes
```text and keep the existing contents unchanged; this change applies to the
code block containing the "src/vouch/embeddings/" listing in the
semantic-search-design.md document.
In `@pyproject.toml`:
- Around line 36-49: The dev extras in pyproject.toml are missing numpy, causing
mypy import-not-found for the unconditional "import numpy" in
src/vouch/embeddings/*.py; update the pyproject.toml dev extras to include the
same numpy spec used in the embeddings extras (e.g., "numpy>=1.26,<3") so that
CI installs numpy with pip install -e '.[dev]' and mypy type-checking succeeds.
In `@src/vouch/context.py`:
- Around line 65-67: The code path handling kind == "entity" calls
store.get_entity(artifact_id) and returns e.name or e.description[:200], which
can raise TypeError if e.description is None; update this branch in
src/vouch/context.py (the logic around kind == "entity", variable e from
store.get_entity) to guard against None by using a safe fallback before slicing
(e.g., use an empty string or coalesce e.description) so the expression becomes
something like e.name or (e.description or "")[:200]; ensure the change
preserves the existing short-description behavior.
In `@src/vouch/embeddings/dedup.py`:
- Line 9: CI mypy fails on the np.ndarray annotation in dedup.py (see symbol
np.ndarray used in deduplicate_embeddings or similar) because numpy type stubs
are missing; add "types-numpy" to the dev-dependencies in pyproject.toml
(matching the existing pattern for "types-pyyaml") so mypy can resolve numpy
types during the python -m mypy src run.
In `@src/vouch/embeddings/rerank.py`:
- Around line 43-47: The reranker currently builds reranked using zip(...,
strict=False) which can silently drop entries when the returned scores length
differs from hits; update the code around reranker.score(...) and the
construction of reranked to explicitly validate that len(scores) == len(hits)
and raise a clear exception (include context like query and counts) if they
differ, then proceed to build reranked (or you may switch zip to strict=True
after the length check) so mismatches fail fast; reference the
variables/function names scores, hits, reranked, and reranker.score.
In `@src/vouch/embeddings/scorer.py`:
- Around line 54-67: Validate the incoming metrics list against the supported
set before initializing totals: check that each entry in metrics is one of
{"recall@k","mrr","ndcg"} and raise a clear error (e.g. ValueError) if any
unknown metric is present, rather than silently initializing totals for
unsupported names; then proceed to build totals = {m:0.0 for m in metrics} and
the loop using recall_at_k, mrr, ndcg as before (references: variables metrics,
totals, queries_file, index_db.search_semantic, recall_at_k, mrr, ndcg, and
parameter k).
In `@src/vouch/index_db.py`:
- Around line 185-192: mypy is failing because numpy is an optional dependency;
update static type checking to ignore missing numpy stubs by adding a mypy
override for the "numpy" module (ignore_missing_imports = true) in your
pyproject.toml so functions like _vec_to_blob and _blob_to_vec no longer trigger
import-not-found errors during CI; alternatively, if you prefer a local change,
add a per-file import ignore for numpy (e.g., add a # type:
ignore[import-not-found] on the numpy import lines) to silence mypy.
- Around line 248-271: The cache uses id(conn) in _sqlite_vec_loaded so a new
sqlite3.Connection can reuse a freed object's id and incorrectly skip loading;
update _load_sqlite_vec to mark the connection itself after successful load
(e.g., set an attribute on conn like conn._sqlite_vec_loaded = True) or use a
WeakSet instead of _sqlite_vec_loaded, check that attribute/WeakSet membership
on the actual conn object rather than id(conn), and ensure you still disable
extension loading in the finally block and add the connection to the chosen
cache mechanism only after a successful sqlite_vec.load(conn).
- Around line 294-301: The SQL references the SELECT alias "score" in the WHERE
and ORDER BY clauses (in the conn.execute call that assigns rows), which SQLite
disallows; update the query to either (a) repeat the full expression 1.0 -
vec_distance_cosine(vec, ?) where "score" is used (i.e., use WHERE kind IN (...)
AND 1.0 - vec_distance_cosine(vec, ?) >= ? and ORDER BY 1.0 -
vec_distance_cosine(vec, ?) DESC) or (b) wrap the current SELECT as a subquery
(SELECT * FROM (SELECT ..., 1.0 - vec_distance_cosine(vec, ?) AS score FROM
embedding_index WHERE kind IN (...)) WHERE score >= ? ORDER BY score DESC LIMIT
?) so that the alias can be filtered; apply the change in the conn.execute call
that passes (q.tobytes(), *kinds, min_score, limit) and ensure placeholders and
parameter order remain correct.
In `@tests/embeddings/_fakes.py`:
- Around line 31-42: The encode implementation in MockEmbedder produces
non-finite floats by unpacking arbitrary SHA-256 bits into IEEE float32; to fix,
ensure any unpacked value assigned to out is finite (use np.isfinite) and
replace NaN/Inf with a stable fallback (e.g., 0.0) before incrementing i, then
continue with the existing normalization (guarding against zero norm). Update
the loop that fills out (where struct.unpack("<f", h[j:j + 4])[0] is used) to
check the value for finiteness and substitute 0.0 for non-finite values so tests
comparing arrays deterministically (np.array_equal) won't fail.
---
Nitpick comments:
In `@src/vouch/embeddings/dedup.py`:
- Around line 74-76: The code treats vecs[k1] @ vecs[k2] as cosine similarity
but that only holds for L2-normalized vectors; update the logic around the line
where cos = float(vecs[k1] @ vecs[k2]) to compute true cosine similarity by
either L2-normalizing vectors (e.g., normalize every vector in vecs once before
the pairwise loop) or replace the dot with dot/(norm(v1)*norm(v2)) using the
same vecs, and keep references to vecs, k1, k2 and the cos variable so the
change is applied exactly where the pairwise similarity is computed.
🪄 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: 1ef83ceb-2469-4873-8aee-e01c26561b11
📒 Files selected for processing (34)
docs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/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/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/test_core.pytests/embeddings/test_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
+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.
Specify a language for the fenced code block.
This block triggers markdownlint MD040 and will keep docs lint noisy.
Suggested patch
-```
+```text
src/vouch/embeddings/
__init__.py # public API: encode, search, register
base.py # Embedder ABC + adapter registry
st_mpnet.py # default impl (sentence-transformers all-mpnet-base-v2)
@@
eval.py # recall@k / MRR / nDCG harness
migration.py # model-identity check + backfill orchestration</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/superpowers/specs/2026-05-20-semantic-search-design.md around lines 33
- 47, The fenced code block listing the embeddings module files triggers
markdownlint MD040 because it has no language; update the opening fence fromtotext (or another appropriate language token) so the block becomes ```text
and keep the existing contents unchanged; this change applies to the code block
containing the "src/vouch/embeddings/" listing in the semantic-search-design.md
document.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+213
to
+227
| ## 13. Test plan (~900 LOC across 9 files) | ||
|
|
||
| | File | Covers | | ||
| |---|---| | ||
| | `tests/test_embeddings_core.py` | Embedder ABC, registry, content-hash skip, batched encode, lazy load | | ||
| | `tests/test_embeddings_storage.py` | vec0 + sqlite-vec round trip + NumPy fallback parity | | ||
| | `tests/test_embeddings_search.py` | Semantic primary, FTS5 fallback, lexical-disjoint regression | | ||
| | `tests/test_embeddings_fusion.py` | RRF, weighted, normalized fusion strategies correctness | | ||
| | `tests/test_embeddings_rerank.py` | Cross-encoder rerank changes top-K order on a known pair | | ||
| | `tests/test_embeddings_hyde.py` | HyDE expansion improves recall on terse queries | | ||
| | `tests/test_embeddings_dedup.py` | Threshold ledger + audit event | | ||
| | `tests/test_embeddings_migration.py` | Model-version mismatch + backfill flow | | ||
| | `tests/test_embeddings_eval.py` | recall@k / MRR / nDCG correctness on synthetic ground truth | | ||
| | `tests/test_embeddings_cli.py` | CLI flag routing | | ||
|
|
There was a problem hiding this comment.
Fix the test-plan count mismatch.
The heading says “9 files” but the table currently lists 10 files (including tests/test_embeddings_cli.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 `@docs/superpowers/specs/2026-05-20-semantic-search-design.md` around lines 213
- 227, The heading "13. Test plan (~900 LOC across 9 files)" is out of sync with
the table which lists 10 test files (notably `tests/test_embeddings_cli.py`);
update the heading to the correct file count (change "9 files" to "10 files") or
remove the extraneous table row if `tests/test_embeddings_cli.py` should not be
included so the heading and table match. Ensure you edit the heading text "13.
Test plan (~900 LOC across 9 files)" and/or the table entry for
`tests/test_embeddings_cli.py` accordingly.
| 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
# Check if types-numpy is already in dev dependencies
rg -n "types-numpy" pyproject.toml setup.cfg setup.py requirements*.txt 2>/dev/null || echo "types-numpy not found in dependencies"
# Check mypy configuration for numpy handling
rg -n -A5 '\[tool\.mypy\]' pyproject.toml 2>/dev/null
rg -n 'ignore_missing_imports' pyproject.toml mypy.ini .mypy.ini setup.cfg 2>/dev/null || echo "No ignore_missing_imports found"Repository: vouchdev/vouch
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check if pyproject.toml exists and show its full mypy configuration
if [ -f "pyproject.toml" ]; then
echo "=== pyproject.toml exists ==="
cat pyproject.toml | grep -A 20 "\[tool\.mypy\]" || echo "No [tool.mypy] section"
fi
# Check for mypy.ini or setup.cfg
echo ""
echo "=== Checking for other config files ==="
ls -la mypy.ini setup.cfg .mypy.ini 2>&1 | grep -v "cannot access"
# Check if numpy is imported elsewhere and how many files use it
echo ""
echo "=== Files importing numpy ==="
rg -l "import numpy" --type py
# Check if there are any existing type ignore comments for numpy
echo ""
echo "=== Existing type ignore comments ==="
rg -n "type: ignore" src/vouch/embeddings/dedup.py 2>/dev/null || echo "No type ignore comments in dedup.py"Repository: vouchdev/vouch
Length of output: 632
🏁 Script executed:
#!/bin/bash
# Search for CI/CD configuration and mypy invocation
echo "=== GitHub Actions workflows ==="
find .github/workflows -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -5
echo ""
echo "=== tox.ini ==="
if [ -f "tox.ini" ]; then
cat tox.ini | grep -A 10 "mypy\|tool.mypy" || echo "No mypy in tox.ini"
fi
echo ""
echo "=== Makefile ==="
if [ -f "Makefile" ]; then
cat Makefile | grep -B2 -A5 "mypy" || echo "No mypy in Makefile"
fi
echo ""
echo "=== pytest.ini or pyproject.toml pytest config ==="
if [ -f "pytest.ini" ]; then
cat pytest.ini
else
echo "No pytest.ini"
fi
echo ""
echo "=== Search for mypy in config files ==="
rg -l "mypy" . --type yaml --type toml --type ini 2>/dev/null | head -10
echo ""
echo "=== Check if mypy is in dependencies ==="
rg "mypy" pyproject.toml setup.py setup.cfg requirements*.txt 2>/dev/nullRepository: vouchdev/vouch
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Check the CI workflow to see if mypy is required
echo "=== GitHub Actions CI workflow ==="
cat .github/workflows/ci.yml | grep -B2 -A5 "mypy\|type\|make check" || echo "Checking full workflow..."
echo ""
echo "=== Full CI workflow ==="
cat .github/workflows/ci.ymlRepository: vouchdev/vouch
Length of output: 1671
🏁 Script executed:
#!/bin/bash
# Check the actual mypy default behavior by looking at what numpy stubs would provide
echo "=== Checking dev dependencies in pyproject.toml ==="
rg -A 20 "dev.*=" pyproject.toml | head -40
echo ""
echo "=== Check numpy import usage in dedup.py ==="
cat -n src/vouch/embeddings/dedup.py | head -20
echo ""
echo "=== Check if there are any comments about mypy or numpy in the file ==="
rg -n "mypy|type.*ignore|numpy" src/vouch/embeddings/dedup.pyRepository: vouchdev/vouch
Length of output: 1205
Add types-numpy to dev dependencies to fix mypy type checking.
The CI pipeline explicitly runs python -m mypy src which will fail when encountering the np.ndarray type annotation in dedup.py (line 18) without numpy type stubs. Add types-numpy to the dev dependencies in pyproject.toml, consistent with the existing types-pyyaml pattern.
🧰 Tools
🪛 GitHub Actions: ci / 1_test (py3.11).txt
[error] 9-9: mypy: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / 2_test (py3.12).txt
[error] 9-9: mypy error: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / 3_test (py3.13).txt
[error] 9-9: mypy: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / test (py3.11)
[error] 9-9: mypy error: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / test (py3.12)
[error] 9-9: mypy: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🪛 GitHub Actions: ci / test (py3.13)
[error] 9-9: mypy: Cannot find implementation or library stub for module named "numpy" [import-not-found]
🤖 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, CI mypy fails on the np.ndarray
annotation in dedup.py (see symbol np.ndarray used in deduplicate_embeddings or
similar) because numpy type stubs are missing; add "types-numpy" to the
dev-dependencies in pyproject.toml (matching the existing pattern for
"types-pyyaml") so mypy can resolve numpy types during the python -m mypy src
run.
Comment on lines
+248
to
+271
| _sqlite_vec_loaded: set[int] = set() | ||
|
|
||
|
|
||
| def _load_sqlite_vec(conn: sqlite3.Connection) -> bool: | ||
| """Best-effort load of the sqlite-vec extension.""" | ||
| if id(conn) in _sqlite_vec_loaded: | ||
| return True | ||
| try: | ||
| conn.enable_load_extension(True) | ||
| except (AttributeError, sqlite3.OperationalError): | ||
| return False | ||
| try: | ||
| import sqlite_vec # type: ignore[import-not-found] | ||
| except ImportError: | ||
| return False | ||
| try: | ||
| sqlite_vec.load(conn) | ||
| except sqlite3.OperationalError: | ||
| return False | ||
| finally: | ||
| with suppress(sqlite3.OperationalError): | ||
| conn.enable_load_extension(False) | ||
| _sqlite_vec_loaded.add(id(conn)) | ||
| return True |
There was a problem hiding this comment.
Cache key id(conn) can be reused after connection is garbage collected.
Python reuses object IDs after the original object is freed. If a connection is closed and a new one happens to receive the same memory address (and thus same id()), this function will incorrectly return True without loading the extension on the new connection.
Consider using a WeakSet or attaching a marker attribute to the connection:
Proposed fix using connection attribute
-_sqlite_vec_loaded: set[int] = set()
+_SQLITE_VEC_LOADED_ATTR = "_vouch_sqlite_vec_loaded"
def _load_sqlite_vec(conn: sqlite3.Connection) -> bool:
"""Best-effort load of the sqlite-vec extension."""
- if id(conn) in _sqlite_vec_loaded:
+ if getattr(conn, _SQLITE_VEC_LOADED_ATTR, False):
return True
try:
conn.enable_load_extension(True)
except (AttributeError, sqlite3.OperationalError):
return False
try:
import sqlite_vec # type: ignore[import-not-found]
except ImportError:
return False
try:
sqlite_vec.load(conn)
except sqlite3.OperationalError:
return False
finally:
with suppress(sqlite3.OperationalError):
conn.enable_load_extension(False)
- _sqlite_vec_loaded.add(id(conn))
+ setattr(conn, _SQLITE_VEC_LOADED_ATTR, True)
return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/index_db.py` around lines 248 - 271, The cache uses id(conn) in
_sqlite_vec_loaded so a new sqlite3.Connection can reuse a freed object's id and
incorrectly skip loading; update _load_sqlite_vec to mark the connection itself
after successful load (e.g., set an attribute on conn like
conn._sqlite_vec_loaded = True) or use a WeakSet instead of _sqlite_vec_loaded,
check that attribute/WeakSet membership on the actual conn object rather than
id(conn), and ensure you still disable extension loading in the finally block
and add the connection to the chosen cache mechanism only after a successful
sqlite_vec.load(conn).
dripsmvcp
force-pushed
the
feat/semantic-search-phase-7-context-pack
branch
2 times, most recently
from
5e45e42 to
28da722
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vouch/server.py (1)
139-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
explainonkb_context.
build_context_pack()now supportsexplain, but this MCP tool never accepts or forwards it, so agents cannot request the new per-hit breakdown through the server API.🤖 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 139 - 150, The kb_context function currently doesn't accept the new explain option from build_context_pack; update kb_context signature to include an explain: bool = False parameter (or bool | None if you prefer tri-state) and forward it to build_context_pack by adding explain=explain to the call; ensure the docstring mentions the explain argument and update any type hints so callers and linters accept the new parameter (reference kb_context, build_context_pack, and _store).src/vouch/jsonl_server.py (1)
115-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
kb.contextdrops theexplainrequest flag.
_h_contextdoes not forwardp.get("explain"), so clients cannot request score breakdowns via JSONL even when they send it.Proposed fix
def _h_context(p: dict) -> dict: return build_context_pack( # type: ignore[return-value] _store(), query=p["task"], limit=int(p.get("limit", 10)), + explain=bool(p.get("explain", False)), max_chars=p.get("max_chars"), min_items=int(p.get("min_items", 0)), require_citations=bool(p.get("require_citations", False)), fail_on_warnings=bool(p.get("fail_on_warnings", False)), fail_on_budget_truncation=bool(p.get("fail_on_budget_truncation", False)), )🤖 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 115 - 124, The handler _h_context is dropping the "explain" request flag when calling build_context_pack; update the call in _h_context to forward the explain parameter from p (e.g., pass explain=p.get("explain") or explain=bool(p.get("explain", False))) so clients can request score breakdowns; ensure the build_context_pack invocation includes the explain argument alongside query, limit, max_chars, etc., and preserve existing boolean conversion semantics used for other flags like require_citations and fail_on_warnings.
♻️ Duplicate comments (1)
src/vouch/embeddings/rerank.py (1)
43-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast if the reranker returns the wrong number of scores.
zip(..., strict=False)will silently truncate here, so a buggy reranker can drop hits without any signal. Validatelen(scores) == len(hits)before buildingreranked, then switch tostrict=True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/rerank.py` around lines 43 - 47, The code currently zips hits with scores using zip(..., strict=False) which can silently truncate if reranker.score returns the wrong number of scores; update the logic in rerank.py around reranker.score(query, candidates) to validate that len(scores) == len(hits) immediately after calling reranker.score and raise/return an explicit error (or log and raise an Exception) if they differ, then change the subsequent list comprehension that builds reranked to use zip(hits, scores, strict=True) to ensure mismatches cannot be silently ignored (refer to reranker.score, hits, scores, and reranked in your change).
🧹 Nitpick comments (3)
tests/test_context.py (2)
63-76: ⚡ Quick winRemove redundant embedder registration.
This test re-imports and re-registers
MockEmbeddereven though theautouse=Truefixture at line 17 already registers it before every test. The redundant registration is unnecessary and adds maintenance overhead.♻️ Simplify by relying on the autouse fixture
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]))🤖 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 redundantly re-registers the MockEmbedder; remove the lines that import MockEmbedder and call register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) so the test relies on the autouse fixture that already registers the embedder; keep the rest of the test (store init, put_source/put_claim, call to build_context_pack, and the assertion) unchanged.
79-95: ⚡ Quick winRemove redundant embedder registration.
Same issue as the previous test: the
autouse=Truefixture already registersMockEmbedder, so re-registering it here is unnecessary.♻️ Simplify by relying on the autouse fixture
def test_build_context_pack_explain_flag_returns_score_breakdown( 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="hello", evidence=[src.id]))🤖 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, The test test_build_context_pack_explain_flag_returns_score_breakdown re-registers the embedder redundantly; remove the explicit call to register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) from the test (and drop the now-unused import(s) MockEmbedder and/or register if they become unused) so the test relies on the autouse fixture's MockEmbedder registration instead; keep the rest of the test (store setup, build_context_pack call, and assertions) unchanged.src/vouch/embeddings/fusion.py (1)
22-36: ⚡ Quick winPre-compute list concatenation to avoid repeated allocations.
Both
rrf_fuse(line 34) andweighted_fuse(line 70) call_coalesce_snippet(h, a + b)inside a loop that iterates overa + b. This creates a new concatenated list on each iteration, resulting in O(N²) list operations. Pre-compute the merged list once before the loop.⚡ Proposed fix to pre-compute concatenation
For
rrf_fuse:def rrf_fuse(a: Hits, b: Hits, *, limit: int = 10, k: int = 60) -> Hits: """Reciprocal Rank Fusion: score = sum(1 / (k + rank)) across lists.""" scores: dict[tuple[str, str], float] = {} for lst in (a, b): for rank, h in enumerate(lst, start=1): scores[_key(h)] = scores.get(_key(h), 0.0) + 1.0 / (k + rank) out: Hits = [] seen: set[tuple[str, str]] = set() - for h in a + b: + merged = a + b + for h in merged: if _key(h) in seen: continue seen.add(_key(h)) - out.append((h[0], h[1], _coalesce_snippet(h, a + b), scores[_key(h)])) + out.append((h[0], h[1], _coalesce_snippet(h, merged), scores[_key(h)])) out.sort(key=lambda h: h[3], reverse=True) return out[:limit]For
weighted_fuse:def weighted_fuse( a: Hits, b: Hits, *, w_a: float = 0.5, w_b: float = 0.5, limit: int = 10, ) -> Hits: """Min-max normalize each list, then score = w_a * a_score + w_b * b_score.""" a_norm = _minmax([h[3] for h in a]) b_norm = _minmax([h[3] for h in b]) a_score = {_key(h): a_norm[i] for i, h in enumerate(a)} b_score = {_key(h): b_norm[i] for i, h in enumerate(b)} merged: dict[tuple[str, str], float] = {} for key in {*a_score, *b_score}: merged[key] = w_a * a_score.get(key, 0.0) + w_b * b_score.get(key, 0.0) out: Hits = [] seen: set[tuple[str, str]] = set() - for h in a + b: + merged_hits = a + b + for h in merged_hits: if _key(h) in seen: continue seen.add(_key(h)) - out.append((h[0], h[1], _coalesce_snippet(h, a + b), merged[_key(h)])) + out.append((h[0], h[1], _coalesce_snippet(h, merged_hits), merged[_key(h)])) out.sort(key=lambda h: h[3], reverse=True) return out[:limit]Also applies to: 48-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/fusion.py` around lines 22 - 36, The loop in rrf_fuse and weighted_fuse repeatedly computes a + b when calling _coalesce_snippet and iterating, causing repeated allocations; fix by precomputing merged = a + b once (use this merged list for the iteration and for the _coalesce_snippet calls and when computing scores/lookups) and replace all occurrences of a + b in those functions with merged so out/seen and score accesses use the precomputed merged list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/embeddings/migration.py`:
- Around line 22-23: The comparison currently only checks current.name against
stored which lets upgrades that change version or dim slip by; update the
mismatch detection in the migration logic to also compare current.version and
current.dim (or equivalent attributes) against the stored object's version and
dim and only return None when name, version, and dim all match; otherwise treat
it as a mismatch so the migration/backfill path runs (look for the variables
current, stored, and attributes version/dim in
src/vouch/embeddings/migration.py).
In `@src/vouch/index_db.py`:
- Around line 311-314: The loop that computes score uses an unnormalized dot
product (score = q @ vec) which causes magnitude to skew ranking; change the
calculation in the rows loop (after calling _blob_to_vec) to normalize the
candidate vector (and guard against zero norm) before computing the dot with q,
or alternatively ensure vectors are normalized at write time; update the scoring
expression that uses q and vec (and any related min_score comparisons) so it
uses the normalized vec to preserve cosine semantics.
In `@src/vouch/jsonl_server.py`:
- Around line 99-105: The hybrid branch currently calls
index_db.search_semantic(s.kb_dir, q, limit=limit * 2) without honoring a
min_score filter; update the hybrid flow (the block guarded by backend_arg ==
"hybrid") to pass the caller's min_score into index_db.search_semantic (and
propagate any default/None if not provided) so semantic hits are filtered before
rrf_fuse is called; ensure you still call index_db.search(s.kb_dir, q,
limit=limit * 2) for lexical hits and then call rrf_fuse(emb, fts, limit=limit)
as before.
In `@src/vouch/server.py`:
- Around line 126-133: When backend == "hybrid" handle failures from the FTS
call so a failing index_db.search doesn't abort the whole branch: wrap the
index_db.search(store.kb_dir, query, limit=...) call in a try/except, and on
exception log or ignore the error and set fts to an empty list (or None) so you
can call rrf_fuse with only emb results (or have rrf_fuse skip FTS when fts is
empty), then return _to_dicts(hits, "hybrid") as before; update the hybrid
branch around index_db.search, rrf_fuse, and hits to degrade gracefully to
embedding-only results.
In `@src/vouch/storage.py`:
- Around line 483-485: The current skip logic in _embed_and_store uses only the
content hash (existing = _index_db.get_embedding(self.kb_dir, kind=kind, id=id);
if existing is not None and existing[1] == h: return) which prevents rewrites
when the embedder/model changes; update _embed_and_store (and the call path from
backfill_embeddings) to either accept/thread a force boolean or expand the
stored metadata check to include the embedder model identifier, model version,
and vector dimension from existing (e.g., existing metadata fields) and only
skip when both the content hash and model/version/dim match; ensure
backfill_embeddings can pass force=True to force re-embedding and that
_index_db.get_embedding accessors and the storage schema are consulted/updated
so you can compare model/version/dim before returning early.
- Around line 486-509: The embedding steps after persisting the file must not
raise — wrap the call to embedder.encode(...) and all downstream calls
(_index_db.put_embedding, _index_db.set_embedding_meta, and dedup.check_and_log)
in try/except so any exception is caught, logged, and suppressed; if encode
fails, skip calling put_embedding/set_embedding_meta/check_and_log entirely
(ensure vec is only used when encode succeeded), and do not re-raise errors from
those functions so the write path remains successful. Use existing symbols to
locate changes: get_embedder, embedder.encode, _index_db.put_embedding,
_index_db.set_embedding_meta, check_and_log, self.kb_dir, kind, id, vec, and h.
In `@tests/test_context.py`:
- Line 9: The module-level import of MockEmbedder causes pytest collection
failures when numpy/embeddings extras are missing; remove the top-level "from
tests.embeddings._fakes import MockEmbedder" and instead import MockEmbedder
lazily inside the _mock_embedder fixture function so the import happens at test
runtime (within the _mock_embedder fixture) and will be skipped when the
embeddings tests are skipped; update the _mock_embedder fixture to perform "from
tests.embeddings._fakes import MockEmbedder" at its start and use that local
symbol.
---
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 115-124: The handler _h_context is dropping the "explain" request
flag when calling build_context_pack; update the call in _h_context to forward
the explain parameter from p (e.g., pass explain=p.get("explain") or
explain=bool(p.get("explain", False))) so clients can request score breakdowns;
ensure the build_context_pack invocation includes the explain argument alongside
query, limit, max_chars, etc., and preserve existing boolean conversion
semantics used for other flags like require_citations and fail_on_warnings.
In `@src/vouch/server.py`:
- Around line 139-150: The kb_context function currently doesn't accept the new
explain option from build_context_pack; update kb_context signature to include
an explain: bool = False parameter (or bool | None if you prefer tri-state) and
forward it to build_context_pack by adding explain=explain to the call; ensure
the docstring mentions the explain argument and update any type hints so callers
and linters accept the new parameter (reference kb_context, build_context_pack,
and _store).
---
Duplicate comments:
In `@src/vouch/embeddings/rerank.py`:
- Around line 43-47: The code currently zips hits with scores using zip(...,
strict=False) which can silently truncate if reranker.score returns the wrong
number of scores; update the logic in rerank.py around reranker.score(query,
candidates) to validate that len(scores) == len(hits) immediately after calling
reranker.score and raise/return an explicit error (or log and raise an
Exception) if they differ, then change the subsequent list comprehension that
builds reranked to use zip(hits, scores, strict=True) to ensure mismatches
cannot be silently ignored (refer to reranker.score, hits, scores, and reranked
in your change).
---
Nitpick comments:
In `@src/vouch/embeddings/fusion.py`:
- Around line 22-36: The loop in rrf_fuse and weighted_fuse repeatedly computes
a + b when calling _coalesce_snippet and iterating, causing repeated
allocations; fix by precomputing merged = a + b once (use this merged list for
the iteration and for the _coalesce_snippet calls and when computing
scores/lookups) and replace all occurrences of a + b in those functions with
merged so out/seen and score accesses use the precomputed merged list.
In `@tests/test_context.py`:
- Around line 63-76: The test test_build_context_pack_uses_semantic_default
redundantly re-registers the MockEmbedder; remove the lines that import
MockEmbedder and call register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8))
so the test relies on the autouse fixture that already registers the embedder;
keep the rest of the test (store init, put_source/put_claim, call to
build_context_pack, and the assertion) unchanged.
- Around line 79-95: The test
test_build_context_pack_explain_flag_returns_score_breakdown re-registers the
embedder redundantly; remove the explicit call to register(DEFAULT_MODEL_NAME,
lambda: MockEmbedder(dim=8)) from the test (and drop the now-unused import(s)
MockEmbedder and/or register if they become unused) so the test relies on the
autouse fixture's MockEmbedder registration instead; keep the rest of the test
(store setup, build_context_pack call, and assertions) unchanged.
🪄 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: 07811c98-ba2f-4c35-9d66-5ca730bf72f0
📒 Files selected for processing (25)
pyproject.tomlsrc/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/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/conftest.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.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
+22
to
+23
| if current.name == stored: | ||
| return None |
There was a problem hiding this comment.
Compare version and dimension too when detecting mismatches.
This only checks the adapter name. A model upgrade that keeps the same name but changes version or dim will currently return None, so migration/backfill won't trigger even though the stored vectors are stale or incompatible.
🤖 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 22 - 23, The comparison
currently only checks current.name against stored which lets upgrades that
change version or dim slip by; update the mismatch detection in the migration
logic to also compare current.version and current.dim (or equivalent attributes)
against the stored object's version and dim and only return None when name,
version, and dim all match; otherwise treat it as a mismatch so the
migration/backfill path runs (look for the variables current, stored, and
attributes version/dim in src/vouch/embeddings/migration.py).
80ced1d to
75092e7
Compare
dripsmvcp
added a commit
to dripsmvcp/vouch
that referenced
this pull request
May 20, 2026
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-7-context-pack
branch
from
75092e7 to
45115d2
Compare
dripsmvcp
added a commit
to dripsmvcp/vouch
that referenced
this pull request
May 21, 2026
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-7-context-pack
branch
from
45115d2 to
eb0a02c
Compare
dripsmvcp
added a commit
to dripsmvcp/vouch
that referenced
this pull request
May 21, 2026
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-7-context-pack
branch
from
eb0a02c to
838b702
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/vouch/embeddings/migration.py (1)
22-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare model version/dimension in mismatch detection.
This currently treats matching adapter names as compatible even if
versionordimchanged, so stale/incompatible vectors may not be backfilled.Suggested fix
- if current.name == stored: + if ( + current.name == stored + and current.version == meta.get("embedding_model_version") + and current.dim == meta.get("embedding_dim") + ): return None🤖 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 22 - 23, The check that currently returns None when adapters match by name only (if current.name == stored) should be expanded to also verify model version and embedding dimension; update the mismatch detection in migration logic (the function handling adapter compatibility in src/vouch/embeddings/migration.py) to compare current.version and current.dim against the stored counterpart (e.g., stored.version and stored.dim or stored["version"]/stored["dim"] depending on stored shape) and only return None when name, version, and dim all match; if any of those differ, treat it as incompatible so vectors will be backfilled.src/vouch/embeddings/scorer.py (1)
54-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
metricsinput and reject unsupported names.Unknown metric names currently return silent zeros, which can mask typos and invalidate evaluation output.
Suggested fix
def evaluate( @@ ) -> dict[str, float]: """Run a metric sweep over a JSONL queries file.""" from .. import index_db + supported = {"recall@k", "mrr", "ndcg"} + unknown = set(metrics) - supported + if unknown: + raise ValueError(f"Unsupported metrics: {sorted(unknown)}") totals = {m: 0.0 for m in 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/embeddings/scorer.py` around lines 54 - 67, The code silently accepts unknown metric names in the metrics iterable (used to build totals and drive calls to recall_at_k, mrr, ndcg), so add validation at the start of the scoring routine in scorer.py: define the allowed set {"recall@k","mrr","ndcg"}, compute unsupported = set(metrics) - allowed, and if non-empty raise a ValueError listing the unsupported names; then initialize totals only for supported metrics (e.g. totals = {m:0.0 for m in metrics if m in allowed}) so you don't silently accumulate zeros for misspelled/unsupported metrics.
🧹 Nitpick comments (4)
src/vouch/embeddings/hyde.py (1)
15-17: ⚡ Quick winNormalize query consistently before branching.
query.strip()is used for the length check and template path, but the pass-through path returns the untrimmed string. This can create avoidable embedding/cache divergence for whitespace-only differences.Proposed diff
def expand_query_template(query: str, *, min_chars: int = 20) -> str: """Pad short queries with HyDE template; pass through long ones.""" - if len(query.strip()) >= min_chars: - return query - return DEFAULT_TEMPLATE.format(query=query.strip()) + q = query.strip() + if len(q) >= min_chars: + return q + return DEFAULT_TEMPLATE.format(query=q)🤖 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 15 - 17, Normalize the query once before branching: compute a stripped version of query (e.g., q = query.strip()) and use q for the length check and for the return value so both the pass-through and templated paths return the trimmed string; update the code that currently calls query.strip() twice and returns the original query to instead use the single normalized variable when returning directly or formatting DEFAULT_TEMPLATE, referencing the existing DEFAULT_TEMPLATE and min_chars checks.tests/embeddings/test_migration.py (1)
54-55: ⚡ Quick winStrengthen the backfill assertion to cover both inserted claims.
assert n >= 2plus a singlec1check can still pass ifc2is missed. Add an explicit assertion forc2so this test actually verifies both claim re-encodes.Suggested test hardening
n = backfill_embeddings(store) assert n >= 2 assert index_db.get_embedding(store.kb_dir, kind="claim", id="c1") is not None + assert index_db.get_embedding(store.kb_dir, kind="claim", id="c2") is not None🤖 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_migration.py` around lines 54 - 55, The test currently asserts a count n >= 2 and validates only index_db.get_embedding(store.kb_dir, kind="claim", id="c1"), which can miss a failed backfill for the second claim; update the test to also assert that index_db.get_embedding(store.kb_dir, kind="claim", id="c2") is not None (i.e., explicitly check both "c1" and "c2") so both inserted claims are verified as re-encoded.src/vouch/context.py (1)
155-159: 💤 Low valueExplain block uses
hits[0][4]instead of per-hit_bevariable.The loop already unpacks the backend into
_bebut then ignores it, usinghits[0][4]instead. While this works because_retrievereturns the same backend for all hits, it's confusing and will break if that assumption changes.♻️ Clearer implementation
if explain: result["explain"] = [ - {"kind": k, "id": i, "score": sc, "backend": hits[0][4] if hits else "none"} - for k, i, _sn, sc, _be in hits + {"kind": k, "id": i, "score": sc, "backend": be} + for k, i, _sn, sc, be in hits ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/context.py` around lines 155 - 159, The explain list comprehension is using hits[0][4] instead of the per-item backend variable unpacked as _be; change the list comprehension that builds result["explain"] to use the unpacked backend (_be) for each hit (or rename _be to backend for clarity) so the entry becomes {"kind": k, "id": i, "score": sc, "backend": _be} and no longer relies on hits[0][4].tests/test_context.py (1)
16-23: ⚡ Quick winAvoid skipping unrelated context tests via autouse
importorskip.Right now, missing numpy skips the entire module, including non-embedding quality checks. Prefer a no-op fixture when numpy is absent, and skip only embedding-specific tests.
🔧 Minimal restructuring
`@pytest.fixture`(autouse=True) def _mock_embedder() -> None: - pytest.importorskip("numpy") - from tests.embeddings._fakes import MockEmbedder - register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) + try: + import numpy # noqa: F401 + except Exception: + return + from tests.embeddings._fakes import MockEmbedder + register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8))def test_build_context_pack_uses_semantic_default(tmp_path: Path) -> None: + pytest.importorskip("numpy") from tests.embeddings._fakes import MockEmbedder ... def test_build_context_pack_explain_flag_returns_score_breakdown( tmp_path: Path, ) -> None: + pytest.importorskip("numpy") from tests.embeddings._fakes import MockEmbedder ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_context.py` around lines 16 - 23, The autouse fixture _mock_embedder currently calls pytest.importorskip("numpy") which cancels the whole module when numpy is missing; change it to conditionally import numpy (try/except ImportError) inside the _mock_embedder fixture so that when numpy is present you import tests.embeddings._fakes.MockEmbedder and call register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)), and when numpy is absent the fixture becomes a no-op (e.g., simply return/yield None) so unrelated tests in the module are not skipped; keep references to the same fixture name _mock_embedder, register, and MockEmbedder so the lookup in embedding tests remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/embeddings/migration.py`:
- Around line 38-80: The code currently wraps each entire backfill loop in a
try/except AttributeError which can hide per-item attribute issues or real bugs;
instead, call each iterator (store.list_claims, list_pages, list_sources,
list_entities, list_relations, list_evidence) without a broad AttributeError
catch and handle missing attributes per-item (use getattr(..., "") or check for
required attributes like id, text/title/body/name/description/locator/quote
before calling store._embed_and_store), or limit the try/except to only the call
that may not exist (e.g., around store.list_* to detect a missing method) and
catch/log exceptions from store._embed_and_store per item while still
incrementing touched only on success.
In `@src/vouch/jsonl_server.py`:
- Line 100: The import block in jsonl_server.py is unsorted (ruff I001) around
the line importing rrf_fuse; reorder the import statements to follow ruff/PEP8
grouping and alphabetical ordering (standard library, third-party, then
local/package imports) and adjust any # type: ignore comments to remain on the
specific import if still needed; specifically locate the line "from
.embeddings.fusion import rrf_fuse" and place it in the local/package imports
group in alphabetical order with the other local imports, mirroring the fix used
in server.py so the import block is consistently sorted and CI passes.
In `@src/vouch/server.py`:
- Line 127: The import of rrf_fuse from .embeddings.fusion is triggering ruff
I001 due to import sorting; move "from .embeddings.fusion import rrf_fuse" to
the top-level imports in server.py alongside the other imports so it follows
normal import ordering, or if you need it lazy, keep it as the sole statement in
its conditional block and append a "# noqa: I001" comment to silence the linter.
In `@tests/embeddings/test_dedup.py`:
- Around line 34-37: The autouse fixture _register_default mutates the global
embedding registry by calling register(DEFAULT_MODEL_NAME, _HashEmbedder)
without restoring prior state; update the fixture to save the previous
registration for DEFAULT_MODEL_NAME (or detect if none existed), yield to run
the test, and then restore the saved registration (or unregister if there was
none) after the test completes so subsequent tests run against the original
registry state; reference the fixture name _register_default, the register call,
DEFAULT_MODEL_NAME and _HashEmbedder when making the change.
In `@tests/embeddings/test_search.py`:
- Line 113: The current assertion permits fallback backends and should instead
confirm semantic-default embedding retrieval; update the assertion that checks
resp["result"][0]["backend"] to assert equality to "embedding" (and do the same
for the similar check at the other occurrence) so the test fails if results
regress to "fts5" or "substring"; locate the checks using the resp variable in
tests/embeddings/test_search.py and replace the in-tuple membership test with a
direct equality assertion against "embedding".
In `@tests/test_context.py`:
- Around line 67-81: The test test_build_context_pack_uses_semantic_default is
too weak—change it to assert that semantic retrieval actually wins and that
returned explain rows are consistently structured: after calling
build_context_pack(store, query="exact query string", limit=5) verify the
top-ranked item (pack["items"][0]) has id "c1" (not just present anywhere), and
assert each item in pack["items"] contains an "explain" dictionary with the
expected keys/shape (e.g., score/distance and method fields) and non-null
numeric score; use these checks around build_context_pack and pack.get("items")
to catch routing/regression issues.
---
Duplicate comments:
In `@src/vouch/embeddings/migration.py`:
- Around line 22-23: The check that currently returns None when adapters match
by name only (if current.name == stored) should be expanded to also verify model
version and embedding dimension; update the mismatch detection in migration
logic (the function handling adapter compatibility in
src/vouch/embeddings/migration.py) to compare current.version and current.dim
against the stored counterpart (e.g., stored.version and stored.dim or
stored["version"]/stored["dim"] depending on stored shape) and only return None
when name, version, and dim all match; if any of those differ, treat it as
incompatible so vectors will be backfilled.
In `@src/vouch/embeddings/scorer.py`:
- Around line 54-67: The code silently accepts unknown metric names in the
metrics iterable (used to build totals and drive calls to recall_at_k, mrr,
ndcg), so add validation at the start of the scoring routine in scorer.py:
define the allowed set {"recall@k","mrr","ndcg"}, compute unsupported =
set(metrics) - allowed, and if non-empty raise a ValueError listing the
unsupported names; then initialize totals only for supported metrics (e.g.
totals = {m:0.0 for m in metrics if m in allowed}) so you don't silently
accumulate zeros for misspelled/unsupported metrics.
---
Nitpick comments:
In `@src/vouch/context.py`:
- Around line 155-159: The explain list comprehension is using hits[0][4]
instead of the per-item backend variable unpacked as _be; change the list
comprehension that builds result["explain"] to use the unpacked backend (_be)
for each hit (or rename _be to backend for clarity) so the entry becomes
{"kind": k, "id": i, "score": sc, "backend": _be} and no longer relies on
hits[0][4].
In `@src/vouch/embeddings/hyde.py`:
- Around line 15-17: Normalize the query once before branching: compute a
stripped version of query (e.g., q = query.strip()) and use q for the length
check and for the return value so both the pass-through and templated paths
return the trimmed string; update the code that currently calls query.strip()
twice and returns the original query to instead use the single normalized
variable when returning directly or formatting DEFAULT_TEMPLATE, referencing the
existing DEFAULT_TEMPLATE and min_chars checks.
In `@tests/embeddings/test_migration.py`:
- Around line 54-55: The test currently asserts a count n >= 2 and validates
only index_db.get_embedding(store.kb_dir, kind="claim", id="c1"), which can miss
a failed backfill for the second claim; update the test to also assert that
index_db.get_embedding(store.kb_dir, kind="claim", id="c2") is not None (i.e.,
explicitly check both "c1" and "c2") so both inserted claims are verified as
re-encoded.
In `@tests/test_context.py`:
- Around line 16-23: The autouse fixture _mock_embedder currently calls
pytest.importorskip("numpy") which cancels the whole module when numpy is
missing; change it to conditionally import numpy (try/except ImportError) inside
the _mock_embedder fixture so that when numpy is present you import
tests.embeddings._fakes.MockEmbedder and call register(DEFAULT_MODEL_NAME,
lambda: MockEmbedder(dim=8)), and when numpy is absent the fixture becomes a
no-op (e.g., simply return/yield None) so unrelated tests in the module are not
skipped; keep references to the same fixture name _mock_embedder, register, and
MockEmbedder so the lookup in embedding tests remains intact.
🪄 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: 810772f4-d534-4bc5-b182-58eeb04f892f
📒 Files selected for processing (23)
src/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/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pysrc/vouch/storage.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.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
+34
to
+37
| @pytest.fixture(autouse=True) | ||
| def _register_default() -> None: | ||
| register(DEFAULT_MODEL_NAME, _HashEmbedder) | ||
|
|
There was a problem hiding this comment.
Restore registry state after the autouse fixture runs.
_register_default mutates shared global embedding registration on Line 36 without cleanup, so later tests may run with _HashEmbedder unintentionally. Please add teardown to restore the previous registration (or isolate via monkeypatch) to keep test order independence.
🤖 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_dedup.py` around lines 34 - 37, The autouse fixture
_register_default mutates the global embedding registry by calling
register(DEFAULT_MODEL_NAME, _HashEmbedder) without restoring prior state;
update the fixture to save the previous registration for DEFAULT_MODEL_NAME (or
detect if none existed), yield to run the test, and then restore the saved
registration (or unregister if there was none) after the test completes so
subsequent tests run against the original registry state; reference the fixture
name _register_default, the register call, DEFAULT_MODEL_NAME and _HashEmbedder
when making the change.
| }) | ||
| assert resp["ok"] is True | ||
| assert resp["result"] | ||
| assert resp["result"][0]["backend"] in ("embedding", "fts5", "substring") |
There was a problem hiding this comment.
Tighten backend assertions to actually validate semantic-default behavior.
These checks currently pass even if embedding retrieval silently regresses to fallback paths.
🔧 Proposed test tightening
- assert resp["result"][0]["backend"] in ("embedding", "fts5", "substring")
+ assert resp["result"][0]["backend"] == "embedding"
...
- assert result["backend"] in ("embedding", "fts5", "substring")
+ assert result["backend"] == "embedding"Also applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/embeddings/test_search.py` at line 113, The current assertion permits
fallback backends and should instead confirm semantic-default embedding
retrieval; update the assertion that checks resp["result"][0]["backend"] to
assert equality to "embedding" (and do the same for the similar check at the
other occurrence) so the test fails if results regress to "fts5" or "substring";
locate the checks using the resp variable in tests/embeddings/test_search.py and
replace the in-tuple membership test with a direct equality assertion against
"embedding".
Comment on lines
+67
to
+81
| 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", [])) | ||
|
|
There was a problem hiding this comment.
Semantic/explain tests are too weak to catch routing regressions.
These tests currently check presence, not that semantic retrieval actually won or that explain rows are consistently structured.
🔧 Suggested assertion upgrades
- pack = build_context_pack(store, query="exact query string", limit=5)
+ pack = build_context_pack(store, query="exact query string", limit=5, explain=True)
assert any(item["id"] == "c1" for item in pack.get("items", []))
+ assert any(row.get("backend") == "embedding" for row in pack.get("explain", []))
...
pack = build_context_pack(store, query="hello", limit=5, explain=True)
assert "explain" in pack
- assert any("backend" in row for row in pack["explain"])
+ assert pack["explain"]
+ assert all("backend" in row for row in pack["explain"])Also applies to: 83-99
🤖 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 - 81, The test
test_build_context_pack_uses_semantic_default is too weak—change it to assert
that semantic retrieval actually wins and that returned explain rows are
consistently structured: after calling build_context_pack(store, query="exact
query string", limit=5) verify the top-ranked item (pack["items"][0]) has id
"c1" (not just present anywhere), and assert each item in pack["items"] contains
an "explain" dictionary with the expected keys/shape (e.g., score/distance and
method fields) and non-null numeric score; use these checks around
build_context_pack and pack.get("items") to catch routing/regression issues.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/vouch/embeddings/scorer.py (1)
54-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
metricsnames before computing totals.Line 54 accepts arbitrary metric keys, but Lines 62–67 only update known metrics. Unsupported names silently return
0.0, which can invalidate evaluation reports.Suggested fix
def evaluate( @@ ) -> dict[str, float]: """Run a metric sweep over a JSONL queries file.""" from .. import index_db + supported = {"recall@k", "mrr", "ndcg"} + unknown = set(metrics) - supported + if unknown: + raise ValueError(f"Unsupported metrics: {sorted(unknown)}") totals = {m: 0.0 for m in 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/embeddings/scorer.py` around lines 54 - 67, The code builds totals = {m: 0.0 for m in metrics} but only handles known metric keys ("recall@k", "mrr", "ndcg"), so add validation of the metrics list before the loop: check the provided metrics (variable metrics) against the supported set {"recall@k","mrr","ndcg"} and either raise a ValueError listing unsupported names or silently remove them (prefer raising for visibility); then proceed to compute totals using recall_at_k, mrr, ndcg as already implemented (functions recall_at_k, mrr, ndcg and variables hits, rel, k remain unchanged). Ensure the error message names the offending metric(s) so callers can fix their input.
🧹 Nitpick comments (1)
tests/test_context.py (1)
16-24: ⚡ Quick winScope the numpy-dependent fixture to embedding-specific tests only.
Line 21 in an
autouse=Truefixture skips the entire module when numpy is missing, including non-embedding context tests. This reduces baseline coverage unnecessarily.Suggested refactor
-@pytest.fixture(autouse=True) -def _mock_embedder() -> None: +@pytest.fixture +def _mock_embedder() -> None: @@ register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8))-def test_build_context_pack_uses_semantic_default(tmp_path: Path) -> None: +def test_build_context_pack_uses_semantic_default( + tmp_path: Path, _mock_embedder: None +) -> None: @@ -def test_build_context_pack_explain_flag_returns_score_breakdown( - tmp_path: Path, +def test_build_context_pack_explain_flag_returns_score_breakdown( + tmp_path: Path, _mock_embedder: None ) -> None:🤖 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 16 - 24, The fixture _mock_embedder currently uses autouse=True which causes the whole module to be skipped when numpy is missing; change it so the numpy check and MockEmbedder registration only run for tests that opt into embedding behavior: remove autouse=True from the pytest.fixture decorator (make it a normal fixture, e.g., `@pytest.fixture`), keep the pytest.importorskip("numpy") and the register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) lines, and update embedding tests to opt-in either by adding `@pytest.mark.usefixtures`("_mock_embedder") or by requesting the _mock_embedder fixture in their function signature so only those tests are skipped when numpy is absent.
🤖 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.
Duplicate comments:
In `@src/vouch/embeddings/scorer.py`:
- Around line 54-67: The code builds totals = {m: 0.0 for m in metrics} but only
handles known metric keys ("recall@k", "mrr", "ndcg"), so add validation of the
metrics list before the loop: check the provided metrics (variable metrics)
against the supported set {"recall@k","mrr","ndcg"} and either raise a
ValueError listing unsupported names or silently remove them (prefer raising for
visibility); then proceed to compute totals using recall_at_k, mrr, ndcg as
already implemented (functions recall_at_k, mrr, ndcg and variables hits, rel, k
remain unchanged). Ensure the error message names the offending metric(s) so
callers can fix their input.
---
Nitpick comments:
In `@tests/test_context.py`:
- Around line 16-24: The fixture _mock_embedder currently uses autouse=True
which causes the whole module to be skipped when numpy is missing; change it so
the numpy check and MockEmbedder registration only run for tests that opt into
embedding behavior: remove autouse=True from the pytest.fixture decorator (make
it a normal fixture, e.g., `@pytest.fixture`), keep the
pytest.importorskip("numpy") and the register(DEFAULT_MODEL_NAME, lambda:
MockEmbedder(dim=8)) lines, and update embedding tests to opt-in either by
adding `@pytest.mark.usefixtures`("_mock_embedder") or by requesting the
_mock_embedder fixture in their function signature so only those tests are
skipped when numpy is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fde6adb-f79f-4eec-ae06-fce3c0feaee2
📒 Files selected for processing (18)
src/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/jsonl_server.pysrc/vouch/server.pytests/embeddings/test_dedup.pytests/embeddings/test_fusion.pytests/embeddings/test_hyde.pytests/embeddings/test_migration.pytests/embeddings/test_rerank.pytests/embeddings/test_scorer.pytests/test_context.py
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-7-context-pack
branch
from
35269ec to
1cf58c7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vouch/server.py (1)
147-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose and forward
explainin MCPkb_context.The MCP tool signature omits
explain, so callers can’t request score/backend breakdowns despite this PR adding that mode.💡 Proposed fix
def kb_context( task: str, limit: int = 10, max_chars: int | None = None, min_items: int = 0, require_citations: bool = False, + explain: bool = False, ) -> dict[str, Any]: """Build a ContextPack ready to inject into an agent prompt.""" return build_context_pack( # type: ignore[return-value] _store(), query=task, limit=limit, max_chars=max_chars, min_items=min_items, require_citations=require_citations, + explain=explain, )🤖 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 147 - 158, Add an explain parameter to kb_context and forward it to build_context_pack: update the kb_context signature to include explain: bool = False (or the same type used by build_context_pack), then pass explain=explain in the build_context_pack call so callers can request score/backend breakdowns; ensure the function docstring and type hints reflect the new parameter and keep the existing parameters (task, limit, max_chars, min_items, require_citations) unchanged.src/vouch/jsonl_server.py (1)
129-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
explaininkb.contextJSONL handler.
_h_contextcurrently dropsparams.explain, so JSONL callers cannot activate explain mode even though this PR introduces it.💡 Proposed fix
def _h_context(p: dict) -> dict: return build_context_pack( # type: ignore[return-value] _store(), query=p["task"], limit=int(p.get("limit", 10)), max_chars=p.get("max_chars"), min_items=int(p.get("min_items", 0)), require_citations=bool(p.get("require_citations", False)), fail_on_warnings=bool(p.get("fail_on_warnings", False)), fail_on_budget_truncation=bool(p.get("fail_on_budget_truncation", False)), + explain=bool(p.get("explain", False)), )🤖 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 129 - 139, _h_context drops the incoming params.explain flag so callers cannot enable explain mode; update _h_context to forward the explain flag into the call to build_context_pack (i.e., pass explain=...) using the value from p (or p.get("params", {}).get("explain") if the handler nests params), converting to a boolean as needed so kb.context JSONL requests can activate explain mode; locate the call to build_context_pack inside _h_context and add the explain parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/vouch/jsonl_server.py`:
- Around line 129-139: _h_context drops the incoming params.explain flag so
callers cannot enable explain mode; update _h_context to forward the explain
flag into the call to build_context_pack (i.e., pass explain=...) using the
value from p (or p.get("params", {}).get("explain") if the handler nests
params), converting to a boolean as needed so kb.context JSONL requests can
activate explain mode; locate the call to build_context_pack inside _h_context
and add the explain parameter.
In `@src/vouch/server.py`:
- Around line 147-158: Add an explain parameter to kb_context and forward it to
build_context_pack: update the kb_context signature to include explain: bool =
False (or the same type used by build_context_pack), then pass explain=explain
in the build_context_pack call so callers can request score/backend breakdowns;
ensure the function docstring and type hints reflect the new parameter and keep
the existing parameters (task, limit, max_chars, min_items, require_citations)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 314b2c4e-5ef2-499f-8510-3d5967e94ba1
📒 Files selected for processing (6)
src/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/base.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/test_context.py
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 7: route
build_context_pack(the agent-facing context assembly) through semantic retrieval by default. Adds anexplain=Truemode that returns a per-result score breakdown alongside the assembled pack.Tracks Task 23 in the plan.
Commit
3228619_retrieve()to dispatch embedding → FTS5 → substring; addexplainparameter; surfacebackendin the returned pack; update three downstream callers (cli.py,server.py,jsonl_server.py) for the new return shapeDesign notes
Refactored
_retrieve()instead of inlining inbuild_context_pack. The existing helper already isolated retrieval; cleaner to update it in place rather than push the dispatch into the assembly function. Hits are now annotated with theirbackendsource for downstream--explainreporting._enrich_summary()(new helper) backfills snippets.search_embeddingreturns""as the snippet (the embedding backend only knows IDs and scores). Without this enrichment, the existingmax_charsbudget logic inbuild_context_packwould over-include items because empty snippets contribute 0 chars._enrich_summaryfalls back to the stored artifact's text (claim.text/page.title/entity.name) when the snippet is empty.Return shape: always a dict.
build_context_packnow returnspack.model_dump()rather than theContextPackmodel. Reason: the new tests use.get("items", [])withoutexplain=Trueand the explain field needs to be addable; mixed Pydantic + dict was ugly. Existing callers incli.py/server.py/jsonl_server.pyalready called.model_dump(mode="json")on the result; that's been simplified now thatbuild_context_packreturns a dict directly.Test Plan
.venv/bin/pytest tests/test_context.py -v→ 5 passed (3 existing updated for dict-style access + 2 new for semantic default and explain).venv/bin/pytest --ignore=tests/test_sessions.py -q→ 115 passed.venv/bin/python -m ruff check src/vouch/context.py tests/test_context.py→ clean (3 import-order auto-fixes in tests)Callers updated
src/vouch/cli.py— drops the redundant.model_dump(mode="json")src/vouch/server.py— samesrc/vouch/jsonl_server.py— sameWhat's NOT in this PR
--explainis plumbed through the API but not yet a CLI flag onvouch search— Phase 8 adds it. The MCP / JSONL tools acceptexplain=Trueimmediately.Summary by CodeRabbit