Feat/semantic search#37
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an embeddings subsystem: a design spec, a pluggable Embedder framework and registry, three adapter implementations (MPNet, MiniLM, fastembed BGE), optional extras in pyproject, unit and integration tests, and test fixtures/mocks. ChangesEmbeddings Framework & Adapters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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: 3
🧹 Nitpick comments (1)
src/vouch/embeddings/__init__.py (1)
20-33: ⚡ Quick winNarrow the import guard to avoid masking real module failures.
Lines 20-33 swallow all
ImportError, which can hide genuine adapter module errors (syntax errors, missing local imports, or typos) and fail later with less actionable errors. The optional dependencies are only imported inside adapter class__init__methods, so gating on dependency availability before importing the modules is cleaner.Consider using
importlib.util.find_spec()to check if optional dependencies exist before importing:Proposed refactor
+import importlib.util + # Auto-register the default adapter if sentence-transformers is installed. -try: - from . import st_mpnet # noqa: F401 -except ImportError: - pass - -try: - from . import st_minilm # noqa: F401 -except ImportError: - pass - -try: - from . import fastembed_bge # noqa: F401 -except ImportError: - pass +if importlib.util.find_spec("sentence_transformers") is not None: + from . import st_mpnet # noqa: F401 + from . import st_minilm # noqa: F401 + +if importlib.util.find_spec("fastembed") is not None: + from . import fastembed_bge # noqa: F401🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vouch/embeddings/__init__.py` around lines 20 - 33, The current try/except blocks around importing st_mpnet, st_minilm, and fastembed_bge swallow all ImportErrors and can mask real errors; instead, check for the optional dependency's presence with importlib.util.find_spec() (or catch ModuleNotFoundError and verify the missing name) before importing each adapter module so only genuinely missing optional deps are skipped. Update the import guards for the modules named st_mpnet, st_minilm, and fastembed_bge to first call importlib.util.find_spec('<module_name>') (or validate ModuleNotFoundError.name equals the adapter package) and only then import the module; this keeps other ImportError subclasses (e.g., syntax or local import errors inside the adapter) from being silenced and ties into the adapter classes' __init__ behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 33-47: The fenced code block listing module files (contains lines
like "__init__.py", "base.py", "st_mpnet.py", "fastembed_bge.py", "cache.py",
"rerank.py", "hyde.py", "dedup.py", "fusion.py", "eval.py", "migration.py") is
missing a language identifier causing markdownlint MD040; update the opening
fence from ``` to include a language tag (for example ```text) so the block
becomes a proper fenced code block with a language identifier.
In `@src/vouch/embeddings/base.py`:
- Line 55: The code currently sets key = name or DEFAULT_MODEL_NAME which treats
empty string as default; change the logic to only fall back when name is None by
replacing that expression with an explicit None check (e.g., if name is None
then set key to DEFAULT_MODEL_NAME else set key to name) so DEFAULT_MODEL_NAME
is not used for empty/invalid caller input; update any surrounding docstring or
callers of the variable if needed and keep the symbols key and
DEFAULT_MODEL_NAME as the references to modify.
In `@tests/embeddings/_fakes.py`:
- Around line 32-41: The current loop decodes 4-byte chunks as IEEE754 floats
(struct.unpack("<f", ...)) which yields huge/Inf values causing norm overflow;
replace that decoding with a stable deterministic mapping: interpret each 4-byte
chunk as an unsigned 32-bit int (e.g., via int.from_bytes or numpy uint32),
convert to a float in a bounded range like [-1.0, 1.0] by scaling (value /
2**32)*2 - 1, store into out using a float64 accumulator (ensure out is
float64), then compute norm with float64 and perform normalization only if norm
> 0. Reference symbols to change: the loop that produces h and writes into out
(variables h, seed, out, i, dim) and the norm calculation (variable norm) —
replace struct.unpack("<f", h[j:j+4])[0] with the uint32->scaled-float mapping
and ensure dtype is float64 before dividing.
---
Nitpick comments:
In `@src/vouch/embeddings/__init__.py`:
- Around line 20-33: The current try/except blocks around importing st_mpnet,
st_minilm, and fastembed_bge swallow all ImportErrors and can mask real errors;
instead, check for the optional dependency's presence with
importlib.util.find_spec() (or catch ModuleNotFoundError and verify the missing
name) before importing each adapter module so only genuinely missing optional
deps are skipped. Update the import guards for the modules named st_mpnet,
st_minilm, and fastembed_bge to first call
importlib.util.find_spec('<module_name>') (or validate ModuleNotFoundError.name
equals the adapter package) and only then import the module; this keeps other
ImportError subclasses (e.g., syntax or local import errors inside the adapter)
from being silenced and ties into the adapter classes' __init__ behavior.
🪄 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: cadbdf0e-dc5d-41b9-8f45-1d9be8553876
📒 Files selected for processing (12)
docs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/test_core.pytests/embeddings/test_integration.py
Comment on lines
+33
to
+47
| ``` | ||
| src/vouch/embeddings/ | ||
| __init__.py # public API: encode, search, register | ||
| base.py # Embedder ABC + adapter registry | ||
| st_mpnet.py # default impl (sentence-transformers all-mpnet-base-v2) | ||
| st_minilm.py # alternative impl | ||
| fastembed_bge.py # alternative impl (no-torch path via fastembed) | ||
| cache.py # query embedding LRU + content-hash skip cache | ||
| rerank.py # cross-encoder reranker (ms-marco-MiniLM-L6-v2) | ||
| hyde.py # Hypothetical Document Embedding query expansion | ||
| dedup.py # cosine-threshold duplicate detection at ingest | ||
| fusion.py # RRF, weighted-sum, normalized-cosine fusion strategies | ||
| eval.py # recall@k / MRR / nDCG harness | ||
| migration.py # model-identity check + backfill orchestration | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
At Line 33, the code fence is missing a language tag, which triggers markdownlint MD040.
Proposed fix
-```
+```text
src/vouch/embeddings/
__init__.py # public API: encode, search, register
base.py # Embedder ABC + adapter registry
st_mpnet.py # default impl (sentence-transformers all-mpnet-base-v2)
@@
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 module files (contains lines like
"init.py", "base.py", "st_mpnet.py", "fastembed_bge.py", "cache.py",
"rerank.py", "hyde.py", "dedup.py", "fusion.py", "eval.py", "migration.py") is
missing a language identifier causing markdownlint MD040; update the opening
fence fromto include a language tag (for exampletext) so the block
becomes a proper fenced code block with a language identifier.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| def get_embedder(name: str | None = None) -> Embedder: | ||
| """Resolve an Embedder by adapter name. None -> DEFAULT_MODEL_NAME.""" | ||
| key = name or DEFAULT_MODEL_NAME |
There was a problem hiding this comment.
Handle default resolution with an explicit None check.
Line 55 currently treats "" as default, which can mask invalid caller config. Only None should trigger fallback.
💡 Proposed fix
- key = name or DEFAULT_MODEL_NAME
+ key = DEFAULT_MODEL_NAME if name is None else name📝 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
| key = name or DEFAULT_MODEL_NAME | |
| key = DEFAULT_MODEL_NAME if name is None else name |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/embeddings/base.py` at line 55, The code currently sets key = name
or DEFAULT_MODEL_NAME which treats empty string as default; change the logic to
only fall back when name is None by replacing that expression with an explicit
None check (e.g., if name is None then set key to DEFAULT_MODEL_NAME else set
key to name) so DEFAULT_MODEL_NAME is not used for empty/invalid caller input;
update any surrounding docstring or callers of the variable if needed and keep
the symbols key and DEFAULT_MODEL_NAME as the references to modify.
4 tasks
Collaborator
|
Looks great as a foundation — nothing user-visible changes here so it's a safe merge. Nice job keeping the heavy imports lazy so A few small things, none blocking:
Otherwise good to go! CC: fix CI |
Approved design for feat/semantic-search. Embedding (sentence- transformers all-mpnet-base-v2) becomes the primary search backend with FTS5 as fallback. Synchronous-at-write indexing across all six artifact types (claim, page, source, entity, relation, evidence). Maximally functional scope (~3000 LOC): pluggable model adapter registry, sqlite-vec ANN with NumPy fallback, cross-encoder rerank, HyDE query expansion, ingest-time duplicate detection, model-identity migration, recall/MRR/nDCG eval harness, and full CLI/MCP/JSONL parity. The writing-plans step will turn the rollout order in section 16 into concrete phased tasks.
32-task TDD plan for the semantic-search feature: foundation, storage, write hooks across all six artifact types, semantic-primary search integration in MCP/JSONL/CLI, RRF fusion + hybrid, cross-encoder rerank, HyDE expansion, ingest-time duplicate detection, model-identity migration, recall/MRR/nDCG scorer, end-to-end integration test, and user docs. Each task: failing test, minimal implementation, passing test, commit. MockEmbedder test double keeps the unit suite fast; the real model is exercised only under @pytest.mark.integration. The evaluation module is named scorer.py rather than eval.py to avoid shadowing the Python builtin and to keep static analysers quiet; the user-facing CLI subcommand remains `vouch eval embedding` (the Click group is registered under the name "eval", with the Python identifier eval_group).
CI ran ruff which flagged SIM105 on the three try/except ImportError guard blocks in src/vouch/embeddings/__init__.py. Replace each with `contextlib.suppress(ImportError)` -- same semantics, satisfies ruff. Also add mypy overrides for numpy / sqlite_vec / sentence_transformers / fastembed so the type check passes in the base [dev] CI install where those optional extras aren't present. The embedding code paths that import these libraries are only reached when the extras are installed at runtime; for the static type check the missing stubs are noise.
CI runs `pip install -e '.[dev]'` which deliberately excludes the
optional `[embeddings]` extras. tests/embeddings/test_*.py modules
import numpy at top level, so pytest collection fails with
ModuleNotFoundError before any test can be deselected.
Add tests/embeddings/conftest.py with `pytest.importorskip("numpy")`
so the entire embeddings test directory skips gracefully when numpy
is absent. Once `pip install vouch[embeddings]` (or numpy itself) is
present, the skip is a no-op and the tests run normally.
`_validate_content` in bundle.py uses the path's first directory component to look up a Pydantic validator. For sources, both `sources/<sha>/meta.yaml` (the Source model) and `sources/<sha>/content` (raw opaque bytes) hit the same "sources" key, so the validator was being run on the raw content bytes and failing with "1 validation error for Source". Add an early return for any non-meta.yaml path under `sources/` so opaque content bytes are not Pydantic-validated. Only the Source metadata file is checked, which matches the original intent of the PR vouchdev#13 validation. Unblocks three pre-existing test_bundle.py failures inherited from upstream main.
dripsmvcp
force-pushed
the
feat/semantic-search
branch
from
dfc7878 to
2e3ef76
Compare
Per CodeRabbit on PR vouchdev#37: decoding sha256 chunks as raw float32 (via struct.unpack("<f", ...)) produced NaN/Inf for ~1 in 2^7 chunks because most 4-byte patterns aren't finite IEEE754 floats. Norm overflow then made unit normalization unreliable and broke cosine similarity asserts in downstream phases (the dedup test in Phase 6 already had to work around this with a custom _HashEmbedder). Decode each chunk as an unsigned 32-bit integer, scale to [-1.0, 1.0], accumulate in float64 for exact norm, and cast to float32 on return. Deterministic, finite, well-behaved.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
docs/superpowers/specs/2026-05-20-semantic-search-design.md (1)
1-7: 💤 Low valueConsider adding a staleness notice to the header.
As noted in the PR comments, detailed implementation plans can age quickly as code evolves. Consider adding a brief note after the Status field indicating that this design is a snapshot and that the actual code and related specs are authoritative once implementation is complete.
📋 Suggested addition
**Date:** 2026-05-20 **Status:** Approved (design) +**Note:** This design represents the approved plan as of 2026-05-20. Once implementation is complete, the actual code and inline documentation are authoritative. **Branch:** `feat/semantic-search` **Tracks issue:** (to be filed)🤖 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 1 - 7, Add a short staleness notice directly after the "Status:" field in the document "Semantic Search Design — vouch" (i.e., the header block containing Date and Status) stating that this document is a snapshot and that the implementation and repository code/specs are authoritative; update the header near the "Status" line to include a one-sentence note such as "Note: this design is a snapshot — implementation, code, and updated specs in the repository are authoritative." Ensure the new text is concise and clearly adjacent to the Status field so readers see it immediately.
🤖 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`:
- Line 154: The document mentions an embedding_stale flag but doesn’t define
where it lives; update the spec to state that embedding_stale is a boolean
column added to the embedding_index table (or to the per-embedding rows if you
prefer finer granularity) named embedding_stale DEFAULT true, and explain the
lifecycle: set embedding_stale = true when an embedding is created with a
model_id that does not match the current index.model_id (i.e., during model
mismatch detection in the kb.search/model mismatch flow), clear embedding_stale
= false only when the embedding is re-indexed/updated by the reindex job, and
ensure kb.search checks embedding_stale to decide whether to include embeddings
in vector-search results or fall back to FTS5; reference the embedding_index
schema and kb.search/model_id checks so readers can locate the change.
In `@src/vouch/embeddings/__init__.py`:
- Around line 24-31: Replace the broad ImportError suppression with
ModuleNotFoundError for the optional adapter imports in
src/vouch/embeddings/__init__.py: change contextlib.suppress(ImportError) to
contextlib.suppress(ModuleNotFoundError) for the import blocks importing
st_mpnet, st_minilm, and fastembed_bge so only missing-package errors are
silenced and other import issues surface.
In `@src/vouch/embeddings/base.py`:
- Around line 33-36: Change the empty-sequence check in encode_batch to an
explicit length comparison: replace the truthiness check ("if not texts") with
"if len(texts) == 0" so the method explicitly handles zero-length Sequence
inputs when returning np.zeros((0, self.dim), dtype=np.float32); this updates
the encode_batch method in the class defined in this file.
In `@src/vouch/embeddings/fastembed_bge.py`:
- Around line 35-37: The empty-batch check in encode_batch currently uses
truthiness on the Sequence[str] parameter `texts`, which can be unsafe; change
the condition to an explicit length check (`len(texts) == 0`) in `encode_batch`
and return the same np.zeros((0, self.dim), dtype=np.float32) when true so empty
sequences are handled reliably for all Sequence implementations.
In `@src/vouch/embeddings/st_minilm.py`:
- Around line 31-33: The guard in encode_batch uses ambiguous truthiness on
texts; replace "if not texts:" with an explicit length check such as "if
len(texts) == 0:" (or "if not len(texts):") to avoid errors for array-like or
custom sequence objects that don't support boolean evaluation, and keep the
returned shape using self.dim unchanged.
In `@src/vouch/embeddings/st_mpnet.py`:
- Around line 36-38: In encode_batch, avoid using the truthiness check "if not
texts" because some Sequence types (e.g., numpy arrays) raise ValueError;
replace it with an explicit length check like "if len(texts) == 0" in the
encode_batch method to safely detect empty inputs while leaving the returned
np.zeros((0, self.dim), dtype=np.float32) behavior unchanged.
In `@tests/embeddings/test_core.py`:
- Around line 56-59: The test test_registry_round_trip uses a fixed global
registry key "test-adapter" which can cause cross-test coupling; change the test
to register and retrieve using a unique per-test key (e.g., generate a UUID or
include the test name) when calling register(...) and get_embedder(...), or
alternatively clear/restore the global registry before/after the test; update
the calls to register("test-adapter", lambda: MockEmbedder(dim=4)) and e =
get_embedder("test-adapter") to use the unique key so the MockEmbedder lookup is
isolated.
In `@tests/embeddings/test_integration.py`:
- Around line 14-43: Add pytest.importorskip("sentence_transformers") at the top
of the sentence_transformers-dependent integration tests so they skip gracefully
when the optional dependency is absent; specifically, in
test_st_mpnet_loads_and_encodes, test_st_mpnet_semantic_disjoint, and
test_st_minilm_loads_and_encodes wrap or begin each test (or the module) with
pytest.importorskip("sentence_transformers") before importing
vouch.embeddings.st_mpnet.STMpnetEmbedder and
vouch.embeddings.st_minilm.STMinilmEmbedder so the tests no longer hard-fail
when sentence_transformers isn't installed.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 1-7: Add a short staleness notice directly after the "Status:"
field in the document "Semantic Search Design — vouch" (i.e., the header block
containing Date and Status) stating that this document is a snapshot and that
the implementation and repository code/specs are authoritative; update the
header near the "Status" line to include a one-sentence note such as "Note: this
design is a snapshot — implementation, code, and updated specs in the repository
are authoritative." Ensure the new text is concise and clearly adjacent to the
Status field so readers see it immediately.
🪄 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: a4592557-e630-42f9-b92e-47bc991ed59b
📒 Files selected for processing (14)
docs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlsrc/vouch/bundle.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/conftest.pytests/embeddings/test_core.pytests/embeddings/test_integration.py
✅ Files skipped from review due to trivial changes (1)
- tests/embeddings/init.py
|
|
||
| - Read `index_meta.embedding_model`. If absent (legacy KB) or mismatched with the current adapter: | ||
| - Emit `embedding.model_mismatch` audit event. | ||
| - `kb.search` still works via FTS5 fallback path; embedding results carry `embedding_stale: true` until reindex. |
There was a problem hiding this comment.
Clarify the embedding_stale flag storage.
Line 154 mentions that "embedding results carry embedding_stale: true", but this field is not defined in the embedding_index schema (lines 61-71) or any other table. Specify where this flag is stored and how it is set/checked during the model mismatch flow.
🤖 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` at line 154, The
document mentions an embedding_stale flag but doesn’t define where it lives;
update the spec to state that embedding_stale is a boolean column added to the
embedding_index table (or to the per-embedding rows if you prefer finer
granularity) named embedding_stale DEFAULT true, and explain the lifecycle: set
embedding_stale = true when an embedding is created with a model_id that does
not match the current index.model_id (i.e., during model mismatch detection in
the kb.search/model mismatch flow), clear embedding_stale = false only when the
embedding is re-indexed/updated by the reindex job, and ensure kb.search checks
embedding_stale to decide whether to include embeddings in vector-search results
or fall back to FTS5; reference the embedding_index schema and
kb.search/model_id checks so readers can locate the change.
Comment on lines
+24
to
+31
| with contextlib.suppress(ImportError): | ||
| from . import st_mpnet # noqa: F401 | ||
|
|
||
| with contextlib.suppress(ImportError): | ||
| from . import st_minilm # noqa: F401 | ||
|
|
||
| with contextlib.suppress(ImportError): | ||
| from . import fastembed_bge # noqa: F401 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/vouch/embeddings/__init__.pyRepository: vouchdev/vouch
Length of output: 1446
🏁 Script executed:
python3 << 'EOF'
import sys
# Check the exception hierarchy
print("Python version:", sys.version_info)
print("\nException hierarchy:")
print(f"ModuleNotFoundError MRO: {ModuleNotFoundError.__mro__}")
print(f"ImportError MRO: {ImportError.__mro__}")
print(f"\nIs ModuleNotFoundError a subclass of ImportError? {issubclass(ModuleNotFoundError, ImportError)}")
# Test what exception is raised when importing non-existent module
print("\n--- Test 1: Non-existent module ---")
try:
import nonexistent_module_xyz
except Exception as e:
print(f"Exception type: {type(e).__name__}")
print(f"Exception message: {e}")
# Test what exception is raised with nested import
print("\n--- Test 2: Nested import of non-existent module ---")
try:
from . import nonexistent
except Exception as e:
print(f"Exception type: {type(e).__name__}")
print(f"Is ModuleNotFoundError? {isinstance(e, ModuleNotFoundError)}")
print(f"Is ImportError? {isinstance(e, ImportError)}")
EOFRepository: vouchdev/vouch
Length of output: 744
🏁 Script executed:
rg "contextlib\.suppress\(ImportError\)" --type py -B 2 -A 2Repository: vouchdev/vouch
Length of output: 822
🏁 Script executed:
fd -e py -path "*/embeddings/st_mpnet*" -o -path "*/embeddings/st_minilm*" -o -path "*/embeddings/fastembed_bge*"Repository: vouchdev/vouch
Length of output: 227
🏁 Script executed:
find . -type f -name "st_mpnet.py" -o -name "st_minilm.py" -o -name "fastembed_bge.py"Repository: vouchdev/vouch
Length of output: 169
🏁 Script executed:
fd st_mpnet st_minilm fastembed_bge -e pyRepository: vouchdev/vouch
Length of output: 215
🏁 Script executed:
python3 << 'EOF'
import contextlib
import sys
# Test relative import behavior
print("--- Simulating relative import of missing module ---")
# When a relative import fails because the module doesn't exist:
# from . import st_mpnet would raise ImportError, not ModuleNotFoundError
# Let's also check what happens if a module exists but has import errors
# For example, if st_mpnet.py imports a missing dependency
import tempfile
import os
with tempfile.TemporaryDirectory() as tmpdir:
# Create a module that has an import error
bad_module_path = os.path.join(tmpdir, 'bad_module.py')
with open(bad_module_path, 'w') as f:
f.write('import nonexistent_dependency\n')
sys.path.insert(0, tmpdir)
try:
import bad_module
except Exception as e:
print(f"Type when importing module with missing dependency: {type(e).__name__}")
print(f"Message: {e}")
print(f"Is ModuleNotFoundError? {isinstance(e, ModuleNotFoundError)}")
print(f"Is ImportError? {isinstance(e, ImportError)}")
finally:
sys.path.pop(0)
EOFRepository: vouchdev/vouch
Length of output: 283
🏁 Script executed:
cat -n src/vouch/embeddings/st_mpnet.py | head -40Repository: vouchdev/vouch
Length of output: 1512
🏁 Script executed:
cat -n src/vouch/embeddings/st_minilm.py | head -40Repository: vouchdev/vouch
Length of output: 1533
🏁 Script executed:
cat -n src/vouch/embeddings/fastembed_bge.py | head -40Repository: vouchdev/vouch
Length of output: 1542
🏁 Script executed:
python3 << 'EOF'
import contextlib
# Verify that ModuleNotFoundError is caught by suppress(ModuleNotFoundError)
# and that it handles the lazy import case
print("Test 1: Module doesn't exist")
with contextlib.suppress(ModuleNotFoundError):
from . import nonexistent_module
print("✓ ModuleNotFoundError suppressed successfully")
print("\nTest 2: Verify ModuleNotFoundError inheritance")
print(f"ModuleNotFoundError.__bases__ = {ModuleNotFoundError.__bases__}")
print(f"ImportError in ModuleNotFoundError MRO = {ImportError in ModuleNotFoundError.__mro__}")
print("\nTest 3: ImportError that is NOT ModuleNotFoundError")
# This would represent a real bug (not caught by suppress(ModuleNotFoundError))
class FakeImportError(ImportError):
pass
try:
with contextlib.suppress(ModuleNotFoundError):
raise FakeImportError("Real import bug")
except ImportError as e:
print(f"✓ Other ImportError not suppressed: {type(e).__name__}")
EOFRepository: vouchdev/vouch
Length of output: 246
Suppress only ModuleNotFoundError for optional adapter imports to prevent masking genuine bugs.
Suppressing all ImportError silently hides real module issues (syntax errors, circular imports, etc.) while leaving the registry partially initialized. Since optional dependencies are lazily loaded and ModuleNotFoundError is raised when packages aren't installed, replace ImportError with ModuleNotFoundError to catch only intentional failures and surface genuine import problems.
Proposed fix
-with contextlib.suppress(ImportError):
+with contextlib.suppress(ModuleNotFoundError):
from . import st_mpnet # noqa: F401
-with contextlib.suppress(ImportError):
+with contextlib.suppress(ModuleNotFoundError):
from . import st_minilm # noqa: F401
-with contextlib.suppress(ImportError):
+with contextlib.suppress(ModuleNotFoundError):
from . import fastembed_bge # noqa: F401🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/embeddings/__init__.py` around lines 24 - 31, Replace the broad
ImportError suppression with ModuleNotFoundError for the optional adapter
imports in src/vouch/embeddings/__init__.py: change
contextlib.suppress(ImportError) to contextlib.suppress(ModuleNotFoundError) for
the import blocks importing st_mpnet, st_minilm, and fastembed_bge so only
missing-package errors are silenced and other import issues surface.
Comment on lines
+35
to
+37
| def encode_batch(self, texts: Sequence[str]) -> np.ndarray: | ||
| if not texts: | ||
| return np.zeros((0, self.dim), dtype=np.float32) |
There was a problem hiding this comment.
Use len(texts) == 0 for empty-batch detection.
Line 36 currently relies on truthiness, which is not safe for all Sequence[str] inputs.
Suggested fix
- if not texts:
+ if len(texts) == 0:
return np.zeros((0, self.dim), dtype=np.float32)🤖 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/fastembed_bge.py` around lines 35 - 37, The empty-batch
check in encode_batch currently uses truthiness on the Sequence[str] parameter
`texts`, which can be unsafe; change the condition to an explicit length check
(`len(texts) == 0`) in `encode_batch` and return the same np.zeros((0,
self.dim), dtype=np.float32) when true so empty sequences are handled reliably
for all Sequence implementations.
Comment on lines
+31
to
+33
| def encode_batch(self, texts: Sequence[str]) -> np.ndarray: | ||
| if not texts: | ||
| return np.zeros((0, self.dim), dtype=np.float32) |
There was a problem hiding this comment.
Avoid ambiguous truthiness for texts.
Line 32 should use an explicit length check to avoid ambiguous truth-value errors with array-like sequences.
Suggested fix
- if not texts:
+ if len(texts) == 0:
return np.zeros((0, self.dim), dtype=np.float32)📝 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
| def encode_batch(self, texts: Sequence[str]) -> np.ndarray: | |
| if not texts: | |
| return np.zeros((0, self.dim), dtype=np.float32) | |
| def encode_batch(self, texts: Sequence[str]) -> np.ndarray: | |
| if len(texts) == 0: | |
| return np.zeros((0, self.dim), dtype=np.float32) |
🤖 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/st_minilm.py` around lines 31 - 33, The guard in
encode_batch uses ambiguous truthiness on texts; replace "if not texts:" with an
explicit length check such as "if len(texts) == 0:" (or "if not len(texts):") to
avoid errors for array-like or custom sequence objects that don't support
boolean evaluation, and keep the returned shape using self.dim unchanged.
Comment on lines
+36
to
+38
| def encode_batch(self, texts: Sequence[str]) -> np.ndarray: | ||
| if not texts: | ||
| return np.zeros((0, self.dim), dtype=np.float32) |
There was a problem hiding this comment.
Use a length check for empty Sequence inputs.
Line 37 uses if not texts, which can raise ValueError for some valid sequence types (notably np.ndarray). Prefer an explicit length check.
Suggested fix
- if not texts:
+ if len(texts) == 0:
return np.zeros((0, self.dim), dtype=np.float32)🤖 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/st_mpnet.py` around lines 36 - 38, In encode_batch,
avoid using the truthiness check "if not texts" because some Sequence types
(e.g., numpy arrays) raise ValueError; replace it with an explicit length check
like "if len(texts) == 0" in the encode_batch method to safely detect empty
inputs while leaving the returned np.zeros((0, self.dim), dtype=np.float32)
behavior unchanged.
Comment on lines
+56
to
+59
| def test_registry_round_trip() -> None: | ||
| register("test-adapter", lambda: MockEmbedder(dim=4)) | ||
| e = get_embedder("test-adapter") | ||
| assert e.dim == 4 |
There was a problem hiding this comment.
Avoid fixed global registry key in test.
Line 57 uses a shared key in global state. Use a unique per-test key to prevent cross-test coupling.
Proposed fix
+from uuid import uuid4
@@
def test_registry_round_trip() -> None:
- register("test-adapter", lambda: MockEmbedder(dim=4))
- e = get_embedder("test-adapter")
+ key = f"test-adapter-{uuid4().hex}"
+ register(key, lambda: MockEmbedder(dim=4))
+ e = get_embedder(key)
assert e.dim == 4
assert e.name == "mock"🤖 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 - 59, The test
test_registry_round_trip uses a fixed global registry key "test-adapter" which
can cause cross-test coupling; change the test to register and retrieve using a
unique per-test key (e.g., generate a UUID or include the test name) when
calling register(...) and get_embedder(...), or alternatively clear/restore the
global registry before/after the test; update the calls to
register("test-adapter", lambda: MockEmbedder(dim=4)) and e =
get_embedder("test-adapter") to use the unique key so the MockEmbedder lookup is
isolated.
Comment on lines
+14
to
+43
| def test_st_mpnet_loads_and_encodes() -> None: | ||
| from vouch.embeddings.st_mpnet import STMpnetEmbedder | ||
| e = STMpnetEmbedder() | ||
| vec = e.encode("hello world") | ||
| assert isinstance(vec, np.ndarray) | ||
| assert vec.shape == (768,) | ||
| assert vec.dtype == np.float32 | ||
| assert abs(float(np.linalg.norm(vec)) - 1.0) < 1e-3 | ||
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_st_mpnet_semantic_disjoint() -> None: | ||
| """Semantic similarity > lexical-only baseline.""" | ||
| from vouch.embeddings.st_mpnet import STMpnetEmbedder | ||
| e = STMpnetEmbedder() | ||
| q = e.encode("how do users authenticate") | ||
| a = e.encode("login flow uses session cookies signed by the API") | ||
| b = e.encode("the sun is large") | ||
| sim_a = float(q @ a) | ||
| sim_b = float(q @ b) | ||
| assert sim_a > sim_b | ||
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_st_minilm_loads_and_encodes() -> None: | ||
| from vouch.embeddings.st_minilm import STMinilmEmbedder | ||
| e = STMinilmEmbedder() | ||
| vec = e.encode("hello world") | ||
| assert vec.shape == (384,) | ||
| assert vec.dtype == np.float32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check importorskip coverage in integration tests:"
rg -n 'importorskip\(".*"\)' tests/embeddings/test_integration.py
echo
echo "Check integration marker/addopts config in pyproject.toml:"
rg -n -C2 'addopts|integration|markers' pyproject.tomlRepository: vouchdev/vouch
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check STMpnetEmbedder and STMinilmEmbedder implementations ==="
fd 'st_mpnet.py|st_minilm.py' --type f | head -5
echo
echo "=== Check if sentence_transformers is imported in those modules ==="
rg -n 'sentence_transformers|from vouch\.embeddings' vouch/embeddings/ --type py
echo
echo "=== Check project dependencies for optional packages ==="
rg -n -A5 -B1 'dependencies|optional|extras' pyproject.toml | head -40Repository: vouchdev/vouch
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check STMpnetEmbedder implementation ==="
cat -n src/vouch/embeddings/st_mpnet.py
echo
echo "=== Check STMinilmEmbedder implementation ==="
cat -n src/vouch/embeddings/st_minilm.py
echo
echo "=== Check project dependencies structure ==="
cat -n pyproject.toml | head -60Repository: vouchdev/vouch
Length of output: 5323
Add pytest.importorskip("sentence_transformers") guards to sentence_transformers integration tests for consistency.
Lines 14–43 hard-fail when sentence_transformers is missing, while line 48 already uses pytest.importorskip for fastembed. Both are optional dependencies specified in [project.optional-dependencies], so the test suite should skip gracefully when either is unavailable rather than failing.
Suggested fix
`@pytest.mark.integration`
def test_st_mpnet_loads_and_encodes() -> None:
+ pytest.importorskip("sentence_transformers")
from vouch.embeddings.st_mpnet import STMpnetEmbedder
@@
`@pytest.mark.integration`
def test_st_mpnet_semantic_disjoint() -> None:
"""Semantic similarity > lexical-only baseline."""
+ pytest.importorskip("sentence_transformers")
from vouch.embeddings.st_mpnet import STMpnetEmbedder
@@
`@pytest.mark.integration`
def test_st_minilm_loads_and_encodes() -> None:
+ pytest.importorskip("sentence_transformers")
from vouch.embeddings.st_minilm import STMinilmEmbedder🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/embeddings/test_integration.py` around lines 14 - 43, Add
pytest.importorskip("sentence_transformers") at the top of the
sentence_transformers-dependent integration tests so they skip gracefully when
the optional dependency is absent; specifically, in
test_st_mpnet_loads_and_encodes, test_st_mpnet_semantic_disjoint, and
test_st_minilm_loads_and_encodes wrap or begin each test (or the module) with
pytest.importorskip("sentence_transformers") before importing
vouch.embeddings.st_mpnet.STMpnetEmbedder and
vouch.embeddings.st_minilm.STMinilmEmbedder so the tests no longer hard-fail
when sentence_transformers isn't installed.
Collaborator
|
LGTM |
This was referenced May 21, 2026
plind-junior
pushed a commit
that referenced
this pull request
May 22, 2026
Per CodeRabbit on PR #37: decoding sha256 chunks as raw float32 (via struct.unpack("<f", ...)) produced NaN/Inf for ~1 in 2^7 chunks because most 4-byte patterns aren't finite IEEE754 floats. Norm overflow then made unit normalization unreliable and broke cosine similarity asserts in downstream phases (the dedup test in Phase 6 already had to work around this with a custom _HashEmbedder). Decode each chunk as an unsigned 32-bit integer, scale to [-1.0, 1.0], accumulate in float64 for exact norm, and cast to float32 on return. Deterministic, finite, well-behaved.
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 1 of the semantic search feature: foundation layer only. Adds the
vouch.embeddingspackage with theEmbedderABC, adapter registry, content hashing, and three pluggable model adapters (sentence-transformers/all-mpnet-base-v2,sentence-transformers/all-MiniLM-L6-v2,fastembed/bge-small-en-v1.5). No user-visible behavior change yet — nothing inKBStore,kb.search,build_context_pack, or the CLI touches this package on this PR. Subsequent phases will wire it in.Tracks the design at
docs/superpowers/specs/2026-05-20-semantic-search-design.mdand the full 32-task plan atdocs/superpowers/plans/2026-05-20-semantic-search.md.What's in this PR
src/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pyEmbedderABC,register/get_embedderfactory registry,content_hashsrc/vouch/embeddings/st_mpnet.pysentence-transformers/all-mpnet-base-v2(768-dim)src/vouch/embeddings/st_minilm.pysentence-transformers/all-MiniLM-L6-v2(384-dim)src/vouch/embeddings/fastembed_bge.pyfastembed/bge-small-en-v1.5(384-dim)tests/embeddings/_fakes.pyMockEmbedderdeterministic test double (sha256-derived float32)tests/embeddings/test_core.pytests/embeddings/test_integration.py@pytest.mark.integrationthat load real modelspyproject.tomlembeddings,embeddings-fast,rerank;integrationpytest markerDesign notes
Lazy load. Each real adapter defers
from sentence_transformers import …(orfrom fastembed import …) to its__init__, so the basevouchinstall stays light —import vouchdoesn't pull torch/onnxruntime.Guarded auto-register.
embeddings/__init__.pywraps each adapter import intry / except ImportError: pass. Ifpip install vouch[embeddings]was run, the adapter registers itself; otherwise it silently isn't there.get_embedder(name)raises a clearKeyError: unknown embedderlisting what's available.Per-instance dim on MockEmbedder.
Embedder.dim: ClassVar[int]is the type contract for production adapters (e.g., mpnet setsdim = 768);MockEmbedderdeliberately overrides per-instance via__init__(dim=…)so tests can use any width (dim=4,dim=8, etc.) without subclassing. The registry's_REGISTRY[key]()factory call respects either pattern.Pytest marker.
addopts = "-q -m 'not integration'"deselects the real-model tests by default. Run them withpytest -m integrationafterpip install vouch[embeddings].Test Plan
.venv/bin/pytest tests/embeddings -v→ 8 passed, 4 deselected.venv/bin/python -m ruff check src/vouch/embeddings tests/embeddings→ cleanpip install vouch[embeddings](not exercised here — covered when wiring lands in Phase 2)Output of the test run:
What's NOT in this PR (deferred to follow-ups)
embedding_indextable,search_embedding, sqlite-vec + NumPy fallback) — Phase 2 (Tasks 7-10)KBStore.put_*— Phase 3kb_search/build_context_pack/vouch searchCLI — Phase 4These are scoped in the plan and will land as follow-up PRs.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
Bug Fixes