feat: add stale embedding reindex mode - #423
Conversation
📝 WalkthroughWalkthroughAdds a ChangesStale re-embed feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing this because the branch is based on Steve-too's fork baseline, which still contains the embedding reindex surface that #310 targets, while current upstream main has diverged and no longer contains that surface. That makes the PR conflict-heavy and not a clean merge candidate as opened. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/vouch/embeddings/migration.py (1)
69-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip-decision logic is duplicated between
migration.pyandstorage.py::_embed_and_store.
should_embedhere recomputescontent_hash(text)and callsindex_db.get_embeddingto compare hash/model, butstore._embed_and_store(when called withforce=force_run=False) performs the exact same lookup + comparison again internally before actually re-encoding. This means every non-skipped, non-forced artifact triggers two separateget_embeddingDB round-trips and two hash computations, and the two skip conditions can silently diverge over time since they're maintained independently.Consider extracting the "is this artifact stale?" predicate into a single shared helper (e.g. in
embeddings/base.py) used by bothbackfill_embeddingsand_embed_and_store, or pass the already-fetchedexisting/live_hashinto_embed_and_storeto avoid recomputation.🤖 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 69 - 95, The skip/staleness check is duplicated between backfill_embeddings in migration.py and store._embed_and_store in storage.py, causing redundant DB lookups and hash recomputation. Refactor the “should this artifact be embedded?” logic into a shared helper (for example in embeddings/base.py) or have backfill_embeddings pass the already-fetched existing record and live_hash into _embed_and_store so both paths use the same predicate. Update both backfill_embeddings and _embed_and_store to call the shared logic and keep the force_run behavior consistent.
🤖 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.
Nitpick comments:
In `@src/vouch/embeddings/migration.py`:
- Around line 69-95: The skip/staleness check is duplicated between
backfill_embeddings in migration.py and store._embed_and_store in storage.py,
causing redundant DB lookups and hash recomputation. Refactor the “should this
artifact be embedded?” logic into a shared helper (for example in
embeddings/base.py) or have backfill_embeddings pass the already-fetched
existing record and live_hash into _embed_and_store so both paths use the same
predicate. Update both backfill_embeddings and _embed_and_store to call the
shared logic and keep the force_run behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de909106-84c8-437e-b167-6a830a656a0a
📒 Files selected for processing (6)
src/vouch/cli.pysrc/vouch/embeddings/migration.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/embeddings/test_migration.pytests/embeddings/test_search.py
Closes #310.
Summary
stalesupport tobackfill_embeddingswith scanned/reembedded/skipped counts--stalethrough CLI, MCP, and JSONL reindex surfacesforce+staletogether and fall back to full re-embed on model mismatchTests
PYTHONPATH=src python3 -m pytest tests/embeddings/test_migration.py tests/embeddings/test_search.py::test_mcp_kb_reindex_embeddings tests/embeddings/test_search.py::test_jsonl_kb_reindex_embeddings tests/embeddings/test_cli.py::test_reindex_embeddings_backfills -qSummary by CodeRabbit
New Features
Bug Fixes