Claude finished @groksrc's task in 2m 46s —— View job

PR Review: Fix sqlite-vec cleanup during reindex

• Gathering context and reading changed files
• Reviewing code quality & standards
• Reviewing testing & documentation
• Reviewing Basic Memory architecture
• Reviewing security & performance
• Reviewing compatibility
• Final summary

Summary

This PR fixes issue #829 by ensuring sqlite-vec is loaded before any DROP TABLE on a vec0 virtual table, preventing a crash during full reindex. The approach is correct and focused. Below is the checklist evaluation followed by specific findings.

Code Quality & Standards

Testing & Documentation

Basic Memory Architecture

Security & Performance

Compatibility

Findings

Finding 1 — Minor: drop_vector_tables() has no dedicated path test for the "table exists but is not vec0" branch

In sqlite_search_repository.py (new drop_vector_tables method, ~line 626):

if vector_sql and "using vec0" in vector_sql.lower():
    await self._ensure_sqlite_vec_loaded(session)

The mocked test (test_reindex_all_uses_sqlite_vec_aware_drop) patches the whole method, so the internal "using vec0" branch is never exercised by the test suite. If the table exists as a regular table (not vec0), the method skips loading the extension and drops it directly—which is correct behavior but is untested. This is an acceptable trade-off for a regression fix (the critical case is vec0), but a future improvement could add a small unit test for this branch.

Fix this →

Finding 2 — Minor: Redundant local from sqlalchemy import text in _purge_stale_search_rows()

In search_service.py at line 704 (pre-existing, not introduced by this PR but visible in context):

async def _purge_stale_search_rows(self) -> None:
    from basic_memory.repository.sqlite_search_repository import SQLiteSearchRepository
    from sqlalchemy import text   # ← redundant; already imported at module level (line 13)

The module-level import (from sqlalchemy import text at line 13) already covers this. The local import is harmless but adds noise. Since this is pre-existing code that wasn't touched by the PR, it can be addressed in a follow-up.

Finding 3 — Design note: isinstance() coupling is consistent with codebase patterns

reindex_all() now uses isinstance(self.repository, SQLiteSearchRepository) to branch behavior. This pattern already exists in _clear_project_vectors_for_full_reindex() and _purge_stale_search_rows() in the same class, so the PR is consistent. A future improvement could add a drop_vector_tables() abstract method to the base class to avoid the isinstance check, but that's out of scope for this fix.

Overall Assessment

Approve. The fix is correct, minimal, and well-targeted. The core issue (sqlite-vec not loaded before DROP TABLE on a vec0 virtual table) is addressed by the new drop_vector_tables() helper that conditionally loads the extension by inspecting sqlite_master first. The existing delete-embeddings-before-chunks ordering constraint is preserved. Two regression tests cover the key scenarios. No blocking issues found.

Claude finished @groksrc's task in 1m 41s —— View job

PR Review: fix(core): load sqlite-vec before vector table cleanup

• Gathering context and reading changed files
• Reviewing code quality & standards
• Reviewing testing & documentation
• Reviewing Basic Memory architecture
• Reviewing security & performance
• Reviewing compatibility
• Final summary

Summary

This PR correctly fixes issue #829 — sqlite-vec was not loaded before DROP TABLE on a vec0 virtual table during full reindex, causing a crash. The fix adds a drop_vector_tables() helper to SQLiteSearchRepository that inspects sqlite_master first and conditionally loads the extension only when the table is actually a vec0 virtual table.

Code Quality & Standards

Testing & Documentation

Basic Memory Architecture

Design note: The drop_vector_tables() method intentionally bypasses execute_query() and uses db.scoped_session() directly — this is the correct approach since execute_query does not guarantee the sqlite-vec extension is loaded on the connection. The same pattern is used in delete_project_vector_rows() and delete_stale_vector_rows().

Security & Performance

Compatibility

Findings

Finding 1 — Pre-existing / minor: redundant local from sqlalchemy import text in _purge_stale_search_rows()

search_service.py line 704 has a local from sqlalchemy import text import inside _purge_stale_search_rows(), but text is already imported at module level (line 13). This is harmless and pre-existing — not introduced by this PR. Worth a follow-up cleanup.

Overall Assessment

Approve. The fix is correct, minimal, and well-targeted. The core issue (sqlite-vec not loaded before DROP TABLE on a vec0 virtual table) is resolved by the new drop_vector_tables() helper that conditionally loads the extension by inspecting sqlite_master. Three regression tests cover the key scenarios: the new routing in reindex_all, the purge-before-sync ordering, and the non-vec0 plain-table branch. No blocking issues found.