Fix: vouch index to repopulate embedding_index for semantic search - #120
Closed
james3773 wants to merge 1 commit into
Closed
Fix: vouch index to repopulate embedding_index for semantic search#120james3773 wants to merge 1 commit into
vouch index to repopulate embedding_index for semantic search#120james3773 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR addresses two independent improvements: fixing embedding index reset/rebuild to correctly populate semantic search (issue ChangesEmbedding Index Repair (Issue
Session Crystallization Idempotency
Sequence Diagram(s)sequenceDiagram
participant Client
participant rebuild_index
participant reset
participant backfill_embeddings
participant embedding_index
Client->>rebuild_index: vouch index (rebuild_index)
rebuild_index->>reset: reset(kb_dir)
reset->>embedding_index: DELETE stale embeddings
rebuild_index->>backfill_embeddings: backfill_embeddings(store, force=True)
backfill_embeddings->>embedding_index: put_embedding + set_embedding_meta
embedding_index-->>Client: semantic search now works
sequenceDiagram
participant Client
participant crystallize
participant _approved_artifact_ids_for_session
participant proposals
participant update_page
participant summary_page
Client->>crystallize: crystallize(session_id)
crystallize->>_approved_artifact_ids_for_session: scan session proposals
_approved_artifact_ids_for_session->>proposals: filter by APPROVED status + kind
proposals-->>_approved_artifact_ids_for_session: return approved ids
crystallize->>crystallize: attempt approve() for pending
crystallize->>update_page: update_page(summary) if ids non-empty
update_page->>summary_page: overwrite with YAML + claims
summary_page-->>Client: idempotent page id returned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 1
🤖 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/sessions.py`:
- Around line 110-116: Replace the get-then-put existence check with an
update-first flow: call store.update_page(page) and if it raises
ArtifactNotFoundError or a deserialization ValueError (from corrupted
frontmatter), fall back to store.put_page(page) so a malformed or missing page
is overwritten idempotently; remove the store.get_page(page.id) call and handle
both ArtifactNotFoundError and ValueError around store.update_page(page)
(referencing store.update_page, store.put_page, ArtifactNotFoundError, and
page.id).
🪄 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: 42476b52-d522-4115-97c2-59786a95ae48
📒 Files selected for processing (6)
src/vouch/health.pysrc/vouch/index_db.pysrc/vouch/sessions.pysrc/vouch/storage.pytests/embeddings/test_migration.pytests/test_sessions.py
Comment on lines
+110
to
116
| try: | ||
| store.get_page(page.id) | ||
| except ArtifactNotFoundError: | ||
| store.put_page(page) | ||
| else: | ||
| store.update_page(page) | ||
| with index_db.open_db(store.kb_dir) as conn: |
There was a problem hiding this comment.
Use update-first flow to keep crystallize recoverable on malformed existing summary pages.
Line 111 calls store.get_page(page.id) as an existence check; that deserializes frontmatter and can raise ValueError for a corrupted file, aborting crystallize instead of overwriting it idempotently.
Suggested change
- try:
- store.get_page(page.id)
- except ArtifactNotFoundError:
- store.put_page(page)
- else:
- store.update_page(page)
+ try:
+ store.update_page(page)
+ except ArtifactNotFoundError:
+ store.put_page(page)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| store.get_page(page.id) | |
| except ArtifactNotFoundError: | |
| store.put_page(page) | |
| else: | |
| store.update_page(page) | |
| with index_db.open_db(store.kb_dir) as conn: | |
| try: | |
| store.update_page(page) | |
| except ArtifactNotFoundError: | |
| store.put_page(page) | |
| with index_db.open_db(store.kb_dir) as conn: |
🤖 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/sessions.py` around lines 110 - 116, Replace the get-then-put
existence check with an update-first flow: call store.update_page(page) and if
it raises ArtifactNotFoundError or a deserialization ValueError (from corrupted
frontmatter), fall back to store.put_page(page) so a malformed or missing page
is overwritten idempotently; remove the store.get_page(page.id) call and handle
both ArtifactNotFoundError and ValueError around store.update_page(page)
(referencing store.update_page, store.put_page, ArtifactNotFoundError, and
page.id).
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.
Related issues
Closes #119
What changed
health._rebuild_embeddings()now delegates tobackfill_embeddings(store, force=True)sovouch indexrepublishes vectors intoembedding_indexwith model metadata (same path as_embed_and_store), instead of the unused legacyembeddingstable viaindex_embedding().index_db.reset()also clears the legacyembeddingstable. Added regression testtest_rebuild_index_restores_semantic_embeddings.Why
rebuild_index()callsreset(), which wipesembedding_index— the tablesearch_semanticand embedding-backedkb.searchread. The old_rebuild_embeddings()repopulated only the legacyembeddingstable, so semantic search often returned nothing aftervouch indexand silently fell back to FTS5/substring. Anyone recoveringstate.dbor running bulk reindex lost embedding retrieval without an obvious error.What might break
vouch indexwith embeddings installed, semantic / embedding search should return hits again instead of empty results.reset()now also deletes rows from the legacyembeddingstable (orphaned vectors that search never used).backfill_embeddings(claims, pages, sources, entities, relations, evidence), not only claims/pages/entities as before.Not a breaking change for typical workflows. Existing
.vouch/directories need no migration; runvouch indexonce to rebuild correctly.VEP
Not required — internal index rebuild fix; no new CLI/MCP/JSONL surface or schema contract.
Tests
make checkpasses locally (lint + mypy + pytest)test_rebuild_index_restores_semantic_embeddingsintests/embeddings/test_migration.py)CHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
Bug Fixes
Tests