feat: share and compress Lumen indexes across worktrees - #183
Conversation
📝 WalkthroughWalkthroughThe PR introduces repository-scoped shared SQLite collections with profile-based identity, deduplicated vectors, project-aware locking, legacy migration, cleanup, and expanded status metrics. It also bundles sqlite-vec locally and documents the new storage lifecycle. ChangesShared repository index storage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
43dae50 to
d080095
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/clean.go (1)
97-140: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore the required interactive output path.
Lines 101 and 134-138 write directly to injected streams. The
cleancommand must send progress throughtui.Progresson stderr. It must print completion summaries withfmt.Printf. It must print errors withfmt.Fprintf(os.Stderr, ...).As per coding guidelines, interactive
cleancommands must usetui.Progressfor progress,fmt.Printffor summaries, andfmt.Fprintf(os.Stderr, ...)for errors.🤖 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 `@cmd/clean.go` around lines 97 - 140, Update cleanIndexes to use tui.Progress on stderr for progress output instead of the injected stderr writer, fmt.Printf for completion summaries, and fmt.Fprintf(os.Stderr, ...) for errors. Preserve the existing cleanup counts, summary text, and error propagation while removing direct output through the injected stdout/stderr streams.Source: Coding guidelines
🧹 Nitpick comments (18)
internal/store/store.go (1)
130-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the legacy fallback in the
NewCollectiondoc comment.
openCollectionreturns a legacy store throughopenStorewhen the target file still uses the per-worktree schema (internal/store/shared.go, lines 74-79). In that cases.sharedis false,UseProjectis a no-op, andNewCollectionreturns a store that is not a shared collection. The current doc comment states only that the function opens a repository-scoped shared collection.State the fallback in the comment so callers know they must check
IsShared()before they rely on project membership.🤖 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 `@internal/store/store.go` around lines 130 - 149, Update the NewCollection doc comment to document that legacy per-worktree databases may return a non-shared store, where project selection is unavailable; instruct callers to check IsShared() before relying on project membership.internal/config/config.go (1)
70-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider resolving symlinks for the non-Git identity.
git.CommonDirresolves symlinks before returning (internal/git/worktree.go,filepath.EvalSymlinks). The non-Git branch only appliesfilepath.Absandfilepath.Clean. A non-Git project reached through a symlinked path therefore maps to a different collection than the same project reached through the real path. On macOS this happens for/varversus/private/var.Applying
filepath.EvalSymlinkswith a fallback to the cleaned path makes both branches consistent.♻️ Proposed change
if abs, err := filepath.Abs(projectPath); err == nil { identity = filepath.Clean(abs) + if resolved, err := filepath.EvalSymlinks(identity); err == nil { + identity = resolved + } }Note that this changes the on-disk identity for existing non-Git collections, so it should land together with the
IndexVersionbump in this PR rather than later.🤖 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 `@internal/config/config.go` around lines 70 - 82, Update DBPathForProjectProfileBase’s non-Git identity resolution to apply filepath.EvalSymlinks after obtaining the absolute cleaned project path, retaining the cleaned path when resolution fails. Keep Git identity handling unchanged, and coordinate this identity change with the required IndexVersion bump.internal/config/config_test.go (1)
127-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the three subtests to a table.
The three subtests differ only in the environment value and the expected outcome. A table makes the added cases (for example an empty string, mixed case, or a leading space) a one-line change.
♻️ Proposed change
func TestVectorStorageConfiguration(t *testing.T) { - t.Run("defaults to int8", func(t *testing.T) { - t.Setenv("LUMEN_VECTOR_STORAGE", "") - cfg, err := NewConfigService("") - if err != nil { - t.Fatal(err) - } - if got := cfg.VectorStorage(); got != "int8" { - t.Fatalf("VectorStorage() = %q, want int8", got) - } - }) - t.Run("accepts float32 override", func(t *testing.T) { - t.Setenv("LUMEN_VECTOR_STORAGE", "FLOAT32") - cfg, err := NewConfigService("") - if err != nil { - t.Fatal(err) - } - if got := cfg.VectorStorage(); got != "float32" { - t.Fatalf("VectorStorage() = %q, want float32", got) - } - }) - t.Run("rejects unknown storage", func(t *testing.T) { - t.Setenv("LUMEN_VECTOR_STORAGE", "float16") - if _, err := NewConfigService(""); err == nil { - t.Fatal("expected invalid vector storage to fail validation") - } - }) + for _, tc := range []struct { + name string + env string + want string + wantErr bool + }{ + {name: "defaults to int8", env: "", want: "int8"}, + {name: "accepts float32 override", env: "FLOAT32", want: "float32"}, + {name: "accepts lowercase float32", env: "float32", want: "float32"}, + {name: "rejects unknown storage", env: "float16", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("LUMEN_VECTOR_STORAGE", tc.env) + cfg, err := NewConfigService("") + if tc.wantErr { + if err == nil { + t.Fatal("expected invalid vector storage to fail validation") + } + return + } + if err != nil { + t.Fatal(err) + } + if got := cfg.VectorStorage(); got != tc.want { + t.Fatalf("VectorStorage() = %q, want %q", got, tc.want) + } + }) + } }As per coding guidelines: "Use table-driven tests for multiple test cases".
🤖 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 `@internal/config/config_test.go` around lines 127 - 154, The three subtests in TestVectorStorageConfiguration should be consolidated into a table-driven test. Define cases containing the environment value, expected VectorStorage result, and expected error state, then iterate with t.Run while preserving the existing validation for defaults, accepted values, and rejected values.Source: Coding guidelines
internal/store/shared.go (4)
361-374: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the per-chunk existence queries with one batched query.
MissingChunkInputsexecutes oneQueryRowper chunk. Indexing calls it for every file, and embedding batches hold up to 256 chunks, so a full index run issues one SQLite round trip per chunk. A single statement with a boundINlist, executed in slices, returns the same information with a bounded number of statements.♻️ Sketch
func (s *Store) MissingChunkInputs(chunks []chunker.Chunk) ([]int, error) { - missing := make([]int, 0, len(chunks)) - for i, c := range chunks { - h := embeddingInputHash(c) - var exists bool - if err := s.reader().QueryRow(`SELECT EXISTS(SELECT 1 FROM vector_keys WHERE input_hash = ?)`, h[:]).Scan(&exists); err != nil { - return nil, err - } - if !exists { - missing = append(missing, i) - } - } - return missing, nil + hashes := make([][]byte, len(chunks)) + for i, c := range chunks { + h := embeddingInputHash(c) + hashes[i] = h[:] + } + present, err := s.existingInputHashes(hashes) // one query per slice of hashes + if err != nil { + return nil, err + } + missing := make([]int, 0, len(chunks)) + for i, h := range hashes { + if _, ok := present[string(h)]; !ok { + missing = append(missing, i) + } + } + return missing, nil }🤖 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 `@internal/store/shared.go` around lines 361 - 374, Update Store.MissingChunkInputs to replace the per-chunk QueryRow existence checks with batched SQLite queries using bound IN-list parameters, processing chunks in bounded slices. Map returned input hashes back to chunk indices and preserve the existing missing-index result and error behavior.
862-886: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
deferfor the probe connection.
CleanupCollectionAtopensdbat line 863 and then closes it manually on four separate paths. If a new early return is added later, the connection leaks. A single deferred close covers every path.♻️ Proposed change
db, err := sql.Open("sqlite3", dbPath) if err != nil { return CleanupStats{}, false, err } + defer func() { _ = db.Close() }() var shared bool if err := db.QueryRow(`SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'collection_meta')`).Scan(&shared); err != nil { - _ = db.Close() return CleanupStats{}, false, err } if !shared { - _ = db.Close() return CleanupStats{}, false, nil }Note that
openCollectionat line 887 opens its own connections, so the probe connection must be closed before that call to avoid holding two writers. Keep an explicitdb.Close()immediately before line 887 and make the deferred close idempotent, or move the probe into a small helper function that returns(dimensions, storage, shared, error)and closes its own handle.As per coding guidelines: "Always defer cleanup for acquired resources, including database and file handles; prefer defer over manual cleanup."
🤖 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 `@internal/store/shared.go` around lines 862 - 886, Update CleanupCollectionAt to defer cleanup immediately after opening the probe database, replacing the repeated manual closes on early-return paths. Preserve an explicit db.Close call immediately before openCollection, and make the deferred cleanup safe to run again so the probe connection is closed before opening another connection.Source: Coding guidelines
651-661: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the adaptive candidate escalation.
The loop doubles
candidatesuntil the project-local result set is provably complete orcandidatesreachestotalVectors. Each iteration runs a full KNN pass over the collection-widevec_vectorstable. For a small project inside a large repository collection, the terminating iteration approaches a full scan, and the search does roughlylog2(totalVectors/32)passes before it gets there. This runs on the interactive search path.The correctness is right and the test covers the sparse case. For operations, consider a cap on total candidates with a documented degradation, plus a metric for the iteration count so a pathological collection is visible before users report slow searches.
🤖 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 `@internal/store/shared.go` around lines 651 - 661, Bound the adaptive candidate escalation in the search flow around searchSharedCandidates so interactive searches do not repeatedly approach totalVectors; introduce a documented maximum candidate budget and stop escalating once it is reached, returning the best available results with the specified degraded-completeness behavior. Track and expose the number of escalation iterations through the project’s existing metrics mechanism so pathological searches are observable.
229-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle
stampSharedAccess; it is not cheap on the repeat path.The doc comment states that calling
UseProjectrepeatedly for the same path is cheap. Every call reachesstampSharedAccess, including the fast path at lines 229-232. For a file-backed collection,stampSharedAccessopens a new SQLite connection, sets a pragma, executes anUPDATEagainstprojects, and closes the connection. Each call therefore takes the collection write lock and competes with a concurrent indexer transaction.
last_accessed_atonly drives day-granularity cleanup (CleanupStaleProjectscompares against a cutoff measured in days), so a per-call write is not needed. Stamp at most once per interval perStore.♻️ Proposed change
+// lastStamp guards redundant lifecycle writes; last_accessed_at only feeds +// day-granularity cleanup. +const accessStampInterval = 5 * time.Minute + func (s *Store) stampSharedAccess() { + if !s.lastStampedAt.IsZero() && time.Since(s.lastStampedAt) < accessStampInterval { + return + } + s.lastStampedAt = time.Now() if s.dsn == "" || s.dsn == ":memory:" {Note that
lastStampedAtneeds the same synchronization decision asprojectID, and it must reset whenUseProjectselects a different project.🤖 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 `@internal/store/shared.go` around lines 229 - 273, Throttle stampSharedAccess to perform the lifecycle UPDATE at most once per configured interval per Store, tracking the last stamp time alongside projectID with the same synchronization. Reset lastStampedAt whenever UseProject selects a different project, while preserving the existing fast-path and best-effort stamping behavior.internal/store/shared_test.go (2)
102-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the profile guards.
Two new guards protect against opening a collection with the wrong profile, and neither has a test:
NewCollectionrejects avectorStorageother thanint8orfloat32(internal/store/store.go, lines 135-137).createCollectionSchemareturns "collection profile mismatch" whenschema_version,vec_dimensions, orvector_storagediffer from the stored values (internal/store/shared.go, line 191).The second guard is the one that stops a dimension mismatch from corrupting
vec_vectors. A test that creates a collection with 4 dimensions and then reopens the same path with 8 dimensions, expecting an error, locks that behavior in.I can write both tests if that is useful.
🤖 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 `@internal/store/shared_test.go` around lines 102 - 119, Add tests covering both profile guards: verify NewCollection rejects a vectorStorage value other than int8 or float32, and verify reopening an existing collection created with 4 dimensions using 8 dimensions returns the “collection profile mismatch” error. Add these cases alongside TestSharedCollectionFloat32Override, reusing a persistent collection path for the reopen scenario and asserting the expected errors.
331-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefer the store closes in this test.
sharedis closed at line 345 and eachlegacystore at line 362, but neither usesdefer. Anyt.Fatalbetween the open and the close leaves the SQLite handle open, andt.TempDircleanup then reports a failure that hides the real assertion.The explicit
shared.Close()beforeos.Statis required so the WAL is checkpointed, so keep it and add an idempotent deferred close as a safety net.♻️ Proposed change
shared, err := NewCollection(sharedPath, dimensions, "int8", projectA) if err != nil { t.Fatal(err) } + closed := false + defer func() { + if !closed { + _ = shared.Close() + } + }() if _, err := shared.StoreFileRevision("bulk.go", "ff", chunks, vectorMap); err != nil { t.Fatal(err) } @@ if err := shared.Close(); err != nil { t.Fatal(err) } + closed = trueApply the same pattern to the
legacystore inside the loop.As per coding guidelines: "Always defer cleanup for acquired resources, including database and file handles; prefer defer over manual cleanup."
🤖 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 `@internal/store/shared_test.go` around lines 331 - 370, Defer cleanup immediately after creating each store in the test: add an idempotent deferred close for shared and each legacy store, while retaining the explicit shared.Close() before size/stat checks so WAL checkpointing still occurs. Apply the same deferred-close safety net inside the legacy loop without removing its existing explicit close.Source: Coding guidelines
internal/config/service.go (1)
443-447: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize
vector_storagecase for the YAML source too.
applyEnvOverrideslowercasesLUMEN_VECTOR_STORAGE, but the YAML config file value reachesvalidate()unchanged. A config file withvector_storage: Int8therefore fails startup, while the equivalent environment variable succeeds. Normalizing insideVectorStorage()removes the asymmetry and keeps the profile key inDBPathForProjectProfileBasecanonical.♻️ Proposed change
func (s *ConfigService) VectorStorage() string { s.mu.RLock() defer s.mu.RUnlock() - return s.k.String("vector_storage") + return strings.ToLower(strings.TrimSpace(s.k.String("vector_storage"))) }🤖 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 `@internal/config/service.go` around lines 443 - 447, Update ConfigService.VectorStorage to normalize the configured vector_storage value to lowercase before returning it, so YAML and environment values share canonical handling. Keep validate and DBPathForProjectProfileBase using the normalized result, preserving acceptance of int8 and float32.internal/index/index.go (3)
600-609: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the merkle tree before acquiring the project lease.
IsFreshacquires the project lease, then callsmerkle.BuildTree. The walk needs no store access and costs seconds on large projects. While the lease is held, a project switch blocks. Becausesync.RWMutexblocks new readers once a writer waits, same-project readers stall too.
Index(Line 190) andEnsureFresh(Line 266) already build the tree outside the lock for this reason. Apply the same order here.♻️ Proposed reordering
func (idx *Indexer) IsFresh(projectDir string) (bool, error) { + // Build tree outside the lock: it is read-only and can be slow for large projects. + curTree, err := merkle.BuildTree(projectDir, makeSkip(projectDir)) + if err != nil { + return false, fmt.Errorf("build merkle tree: %w", err) + } releaseProject, err := idx.lockProject(projectDir) if err != nil { return false, err } defer releaseProject() - curTree, err := merkle.BuildTree(projectDir, makeSkip(projectDir)) - if err != nil { - return false, fmt.Errorf("build merkle tree: %w", err) - }🤖 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 `@internal/index/index.go` around lines 600 - 609, Update IsFresh to call merkle.BuildTree before acquiring the project lease, returning any tree-building error before invoking idx.lockProject. Keep the existing releaseProject defer and freshness logic unchanged after the lease is acquired, matching the ordering used by Index and EnsureFresh.
680-711: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the project-selection retry loop.
lockProjectretries without a limit. Each iteration switchesidx.projectPathunder the exclusive lock, then reacquires the read lock. If callers alternate between two projects, another waiter can switch the membership back before this goroutine reacquires the read lease, so the loop repeats. There is no retry bound and no fairness guarantee, so a caller can spin under sustained alternation.The current code is correct. Consider adding a retry counter with a logged warning, or perform the store selection and the lease acquisition under one exclusive-to-shared handoff, so the pathological case is observable.
🤖 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 `@internal/index/index.go` around lines 680 - 711, Bound retries in Indexer.lockProject so sustained project alternation cannot spin indefinitely. Add a retry counter and log a warning when the bound is reached, while preserving the existing project-selection and shared-lease behavior; use the existing logging mechanism and return an appropriate error or fallback at exhaustion.
580-592: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winScope
LastIndexedAtto an explicit project.
LastIndexedAttakes only the read lease. It does not select a project. It therefore returnslast_indexed_atfor whichever membership is currently active in the shared store. Every other accessor in this file (Search,Status,IsFresh) callslockProjectfirst.Today the callers in
cmd/stdio.gouse anIndexerwhose active project matches the effective root, so the value is correct. The asymmetry is still a trap: a future caller that shares oneIndexeracross projects would read another project's timestamp and skip a required reindex. Consider adding aprojectDirparameter and routing throughlockProject.🤖 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 `@internal/index/index.go` around lines 580 - 592, Update Indexer.LastIndexedAt to accept an explicit projectDir and call lockProject before reading metadata, matching Search, Status, and IsFresh. Preserve the existing timestamp parsing and failure behavior, and update all callers such as the stdio flow to pass the effective project root.cmd/stdio_test.go (1)
1107-1113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClose the indexer, and cover the project-lock branch.
Two points in this test:
idxis never closed. The sibling tests uset.Cleanup(func() { _ = idx.Close() })(Lines 1244, 1278, 1381). This test leaks the SQLite handle for the lifetime of the test binary. The repository guidelines require deferred cleanup for database handles.The subprocess holds the lock at
indexlock.LockPathForDB(dbPath), which is the collection lock.ensureIndexednow skips onIsHeld(collection) || IsHeld(project)(cmd/stdio.goLine 798). The project-lock half of that new condition has no test. Add a case that holdsindexlock.LockPathForProject(dbPath, effectiveRoot)instead, and assert the same skip. That branch is the one the per-project locking model depends on.💚 Proposed cleanup addition
idx, effectiveRoot, _, err := ic.getOrCreate(projectPath, "") if err != nil { t.Fatalf("getOrCreate: %v", err) } + t.Cleanup(func() { _ = idx.Close() })🤖 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 `@cmd/stdio_test.go` around lines 1107 - 1113, Close idx via t.Cleanup immediately after getOrCreate succeeds, matching sibling tests. Extend the lock-skipping test around ensureIndexed to also acquire indexlock.LockPathForProject(dbPath, effectiveRoot) and assert the same skip behavior, while retaining the existing collection-lock coverage.Source: Coding guidelines
cmd/index.go (1)
238-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not couple the error check to the logger being present.
Line 240 discards the
PrepareLegacyMigrationerror wheneverloggeris nil. The error is then invisible. The repository guidelines require an explicit_ = errwhen an error is ignored on purpose.Separate the two conditions so the intent is explicit at the call site.
♻️ Proposed fix
if projectPath != "" { legacyPath := config.LegacyDBPathForProject(projectPath, emb.ModelName()) - if err := idx.PrepareLegacyMigration(projectPath, legacyPath); err != nil && logger != nil { - logger.Warn("legacy index migration unavailable; rebuilding missing vectors", "path", legacyPath, "error", err) + // Migration is best effort: a failure only means vectors get re-embedded. + if err := idx.PrepareLegacyMigration(projectPath, legacyPath); err != nil { + if logger != nil { + logger.Warn("legacy index migration unavailable; rebuilding missing vectors", "path", legacyPath, "error", err) + } + _ = err } }🤖 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 `@cmd/index.go` around lines 238 - 243, Update the migration handling around PrepareLegacyMigration so its error is checked independently of logger availability. When migration preparation fails, warn only if logger is non-nil; otherwise explicitly discard the error with _ = err to make the intentional suppression clear.Source: Coding guidelines
internal/index/index_concurrency_test.go (1)
396-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concurrent writer to cover the exclusive membership switch.
The test runs 40 concurrent readers (
SearchandStatus). Both take the shared lease path inlockProject. The exclusive branch runs only when the requested project differs from the active project, which the alternating readers do trigger.The scenario that is not covered is a reader racing an active
Indexon the other project.Indexholdsidx.muand a lease for its whole run, so readers of the other project block on the exclusive switch. Add one goroutine that callsidx.IndexonprojectAwhile the readers run, and keep the same membership assertions. That covers the write path and would surface a lease leak in the corruption-recovery branch.🤖 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 `@internal/index/index_concurrency_test.go` around lines 396 - 427, Add a concurrent writer goroutine alongside the existing reader goroutines that invokes idx.Index for projectA while the shared Search and Status operations execute. Ensure it participates in the same start signal and WaitGroup, propagates any indexing error through errs, and preserves the existing membership assertions for all readers.internal/index/migrate_test.go (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestructure the migration test into a table-driven test with realistic inputs. Both findings in this file share one root cause:
TestLegacyMigrationReusesUnchangedVectorscovers a single happy-path case built on an unrealistic chunking budget, so it cannot detect the two failure modes that matter formigrate.go.
internal/index/migrate_test.go#L25-L31: replacemaxChunkTokens0with512sosplitOversizedChunksandmergeUndersizedChunksactually run on both the migration path and the indexing path.internal/index/migrate_test.go#L57-L72: add a second case in which the file content changed after the legacy index was written; assert that the embedder is called and that the legacy database still exists, which pins thefinishLegacyMigrationdeletion guard.The repository guidelines require table-driven tests for multiple cases and red/green TDD for features and bug fixes.
🤖 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 `@internal/index/migrate_test.go` around lines 25 - 31, Restructure TestLegacyMigrationReusesUnchangedVectors as a table-driven test covering unchanged and changed file content; in internal/index/migrate_test.go lines 25-31, pass maxChunkTokens as 512 so splitOversizedChunks and mergeUndersizedChunks execute on both paths, and in lines 57-72 add the changed-content case asserting the embedder is called and the legacy database remains.Source: Coding guidelines
internal/index/migrate.go (1)
34-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMove
query_onlyonto every legacy SQLite connection.
sql.Openreturns the driver connection pool, andPRAGMA query_only=ONonly affects the one connection executed bydb Exec. The followingdb.Querycalls can use cached connections where this pragma was never applied. Set a read-only mode on the DSN instead, and keep each legacy connection read-only for the whole migration.🤖 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 `@internal/index/migrate.go` around lines 34 - 41, Update the legacy database setup around sql.Open in the migration flow to apply SQLite read-only mode through the DSN, ensuring every pooled connection remains read-only throughout the migration. Remove the connection-specific PRAGMA query_only execution while preserving the existing error handling and deferred close 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 @.gitattributes:
- Line 6: Update the .gitattributes rule for internal/sqlitevec/sqlite-vec.c to
use the -text attribute, preserving linguist-vendored and the existing
whitespace setting, so Git does not convert line endings for this vendored
source.
In `@cmd/clean.go`:
- Around line 252-264: Update runDailyCleanup to create a background logger via
newDebugLogger(), replace io.Discard cleanup streams with the logger, and report
skipped maintenance and cleanup failures through slog. Ensure the log file
returned by newDebugLogger() is closed when non-nil, while preserving the
existing cleanup and stamp-file flow.
- Around line 147-150: Update the lock-acquisition handling around
indexlock.TryAcquire in the cleanup flow: return a wrapped lockErr immediately
when acquisition fails, and only report an active indexer and return the
existing success result when lock is nil with no error. Preserve the normal
cleanup path for a successfully acquired lock and keep the error as the final
return value.
In `@cmd/stdio.go`:
- Line 101: Align the JSON tag for DeduplicationRate with the corresponding
internal/index.StatusInfo field, choosing one consistent term and applying it to
both output definitions before finalizing the MCP contract.
- Around line 173-178: Make indexerCache configuration handling consistent
between dbPath and getOrCreate: if ic.cfg is required, remove the nil fallback
in dbPath and rely exclusively on configuredDBPath; otherwise, add nil handling
in getOrCreate before calling MaxChunkTokens or VectorStorage, avoiding any
nil-receiver panic.
- Around line 534-542: Move the synchronous PrepareLegacyMigration call out of
the getOrCreate cache-lock critical section. Keep indexer creation and cache
insertion protected by ic.mu, release the lock before invoking
PrepareLegacyMigration, and preserve the existing warning behavior on migration
failure; ensure concurrent cache operations and Close are not blocked by
migration work.
- Around line 865-870: Update the project-lock handling around
indexlock.TryAcquire in the reindex flow to log a warning when lockErr is
non-nil, matching the existing collection-lock branch’s logging behavior. Keep
contention or a nil projectLock as skipped without treating it as an acquisition
error, and preserve the existing result and release flow.
In `@docs/INDEX_STORAGE.md`:
- Around line 161-166: Update the --days 0 guidance in docs/INDEX_STORAGE.md
lines 161-166 and skills/reindex/SKILL.md lines 22-23 to state that it removes
all eligible cached indexes while preserving indexes with active locks; no code
change is required in cmd/clean.go.
In `@internal/index/migrate.go`:
- Around line 44-99: In the migration flow’s legacy vector and file row loops,
check rows.Err() and fileRows.Err() immediately after iteration completes and
before closing/returning success. Propagate any iteration error, while
preserving the existing scan and close-error handling.
In `@internal/index/shared.go`:
- Around line 139-170: Refactor the embedding flow around the per-file
missing-chunk loop and embedBatchSize so pending chunks accumulate across files
and idx.emb.Embed is called only when the shared batch reaches 256 or at the
final flush. Preserve each chunk’s file/position association when storing
vectors, and record file revisions only after the batch covering them has been
stored to retain crash consistency.
- Around line 184-197: Update the metadata persistence block in the indexing
function so all metadata keys except root_hash are written in a deterministic
order, wrapping any SetMeta failure with the metadata key; write root_hash last
and wrap its failure consistently. Preserve the existing metadata values and
return stats alongside errors.
- Around line 133-138: Update the shared-vector handling around
MissingChunkInputs so a vector disappearing after the check is treated as
recoverable rather than immediately returning an error. Before StoreFileRevision
proceeds, provide the embedded fallback or retry/reindex the affected input when
its vector_key is no longer available, ensuring indexing completes and root_hash
remains readable.
In `@internal/sqlitevec/lib.go`:
- Around line 15-23: Update Auto to return an error based on the
sqlite3_auto_extension status, converting any non-SQLITE_OK result into a Go
error while preserving successful registration. Update every Auto caller,
including the e2e CLI and store initialization flows, to check and propagate or
handle that error. Make Cancel’s ignored status explicit by returning it or
assigning it to _ for intentional best-effort cleanup.
- Around line 5-8: Update the cgo setup surrounding the import C preamble and
the bundled sqlite-vec sources so they use the SQLite headers and sources from
github.com/mattn/go-sqlite3 rather than requiring a system sqlite3.h;
alternatively remove SQLITE_CORE and switch the extension to sqlite3ext.h with
the corresponding initialization path, while preserving compatibility with the
linked driver ABI.
In `@internal/store/hybrid_cte_test.go`:
- Line 23: In the test setup before sql.Open creates the database, call
sqlite_vec.Auto() to register the vec0 extension and handle any returned error
before proceeding. Keep the existing schema creation unchanged so vec0 is
available when it is created.
In `@internal/store/shared.go`:
- Around line 583-614: Change gcUnreferencedTx and its per-file callers
StoreFileRevision, AttachExistingFileRevision, and removeProjectFile so they no
longer perform collection-wide orphan scans on every file operation. Prefer
restricting cleanup to transaction-affected IDs; otherwise move the full sweep
into CleanupStaleProjects and the existing daily cleanup path while preserving
orphan deletion behavior.
- Around line 809-824: After the rows.Next scan loop, check rows.Err() and
return CleanupStats{} with that error when iteration failed, before closing rows
or proceeding with stale. Keep the existing rows.Scan and rows.Close error
handling unchanged, using the loop’s rows symbol.
- Around line 549-554: The database lookup in insertSharedChunks must
distinguish sql.ErrNoRows from other query failures: wrap or return a dedicated
missing-project-file error that identifies path and project membership, while
preserving existing propagation for genuine database errors.
In `@internal/store/store.go`:
- Around line 214-219: Update internal/store/store.go lines 214-219 in the open
path to capture and immediately handle the checkTableExists error by closing db
and returning a wrapped error; also wrap the vector_storage Scan error before
returning it. Update internal/store/shared.go lines 74-79 to capture both
checkTableExists errors, close db, and return wrapped errors before
createCollectionSchema runs; intentionally ignored errors must use explicit _ =
err.
- Around line 86-97: Add a project-selection concurrency contract to Store by
documenting that UseProject and all project-scoped operations require
caller-side serialization, or by adding a sync.RWMutex to protect projectID and
projectPath and the projects lookup. Ensure shared.go consistently follows the
chosen synchronization approach so concurrent project selection cannot mix
results or trigger races.
---
Outside diff comments:
In `@cmd/clean.go`:
- Around line 97-140: Update cleanIndexes to use tui.Progress on stderr for
progress output instead of the injected stderr writer, fmt.Printf for completion
summaries, and fmt.Fprintf(os.Stderr, ...) for errors. Preserve the existing
cleanup counts, summary text, and error propagation while removing direct output
through the injected stdout/stderr streams.
---
Nitpick comments:
In `@cmd/index.go`:
- Around line 238-243: Update the migration handling around
PrepareLegacyMigration so its error is checked independently of logger
availability. When migration preparation fails, warn only if logger is non-nil;
otherwise explicitly discard the error with _ = err to make the intentional
suppression clear.
In `@cmd/stdio_test.go`:
- Around line 1107-1113: Close idx via t.Cleanup immediately after getOrCreate
succeeds, matching sibling tests. Extend the lock-skipping test around
ensureIndexed to also acquire indexlock.LockPathForProject(dbPath,
effectiveRoot) and assert the same skip behavior, while retaining the existing
collection-lock coverage.
In `@internal/config/config_test.go`:
- Around line 127-154: The three subtests in TestVectorStorageConfiguration
should be consolidated into a table-driven test. Define cases containing the
environment value, expected VectorStorage result, and expected error state, then
iterate with t.Run while preserving the existing validation for defaults,
accepted values, and rejected values.
In `@internal/config/config.go`:
- Around line 70-82: Update DBPathForProjectProfileBase’s non-Git identity
resolution to apply filepath.EvalSymlinks after obtaining the absolute cleaned
project path, retaining the cleaned path when resolution fails. Keep Git
identity handling unchanged, and coordinate this identity change with the
required IndexVersion bump.
In `@internal/config/service.go`:
- Around line 443-447: Update ConfigService.VectorStorage to normalize the
configured vector_storage value to lowercase before returning it, so YAML and
environment values share canonical handling. Keep validate and
DBPathForProjectProfileBase using the normalized result, preserving acceptance
of int8 and float32.
In `@internal/index/index_concurrency_test.go`:
- Around line 396-427: Add a concurrent writer goroutine alongside the existing
reader goroutines that invokes idx.Index for projectA while the shared Search
and Status operations execute. Ensure it participates in the same start signal
and WaitGroup, propagates any indexing error through errs, and preserves the
existing membership assertions for all readers.
In `@internal/index/index.go`:
- Around line 600-609: Update IsFresh to call merkle.BuildTree before acquiring
the project lease, returning any tree-building error before invoking
idx.lockProject. Keep the existing releaseProject defer and freshness logic
unchanged after the lease is acquired, matching the ordering used by Index and
EnsureFresh.
- Around line 680-711: Bound retries in Indexer.lockProject so sustained project
alternation cannot spin indefinitely. Add a retry counter and log a warning when
the bound is reached, while preserving the existing project-selection and
shared-lease behavior; use the existing logging mechanism and return an
appropriate error or fallback at exhaustion.
- Around line 580-592: Update Indexer.LastIndexedAt to accept an explicit
projectDir and call lockProject before reading metadata, matching Search,
Status, and IsFresh. Preserve the existing timestamp parsing and failure
behavior, and update all callers such as the stdio flow to pass the effective
project root.
In `@internal/index/migrate_test.go`:
- Around line 25-31: Restructure TestLegacyMigrationReusesUnchangedVectors as a
table-driven test covering unchanged and changed file content; in
internal/index/migrate_test.go lines 25-31, pass maxChunkTokens as 512 so
splitOversizedChunks and mergeUndersizedChunks execute on both paths, and in
lines 57-72 add the changed-content case asserting the embedder is called and
the legacy database remains.
In `@internal/index/migrate.go`:
- Around line 34-41: Update the legacy database setup around sql.Open in the
migration flow to apply SQLite read-only mode through the DSN, ensuring every
pooled connection remains read-only throughout the migration. Remove the
connection-specific PRAGMA query_only execution while preserving the existing
error handling and deferred close behavior.
In `@internal/store/shared_test.go`:
- Around line 102-119: Add tests covering both profile guards: verify
NewCollection rejects a vectorStorage value other than int8 or float32, and
verify reopening an existing collection created with 4 dimensions using 8
dimensions returns the “collection profile mismatch” error. Add these cases
alongside TestSharedCollectionFloat32Override, reusing a persistent collection
path for the reopen scenario and asserting the expected errors.
- Around line 331-370: Defer cleanup immediately after creating each store in
the test: add an idempotent deferred close for shared and each legacy store,
while retaining the explicit shared.Close() before size/stat checks so WAL
checkpointing still occurs. Apply the same deferred-close safety net inside the
legacy loop without removing its existing explicit close.
In `@internal/store/shared.go`:
- Around line 361-374: Update Store.MissingChunkInputs to replace the per-chunk
QueryRow existence checks with batched SQLite queries using bound IN-list
parameters, processing chunks in bounded slices. Map returned input hashes back
to chunk indices and preserve the existing missing-index result and error
behavior.
- Around line 862-886: Update CleanupCollectionAt to defer cleanup immediately
after opening the probe database, replacing the repeated manual closes on
early-return paths. Preserve an explicit db.Close call immediately before
openCollection, and make the deferred cleanup safe to run again so the probe
connection is closed before opening another connection.
- Around line 651-661: Bound the adaptive candidate escalation in the search
flow around searchSharedCandidates so interactive searches do not repeatedly
approach totalVectors; introduce a documented maximum candidate budget and stop
escalating once it is reached, returning the best available results with the
specified degraded-completeness behavior. Track and expose the number of
escalation iterations through the project’s existing metrics mechanism so
pathological searches are observable.
- Around line 229-273: Throttle stampSharedAccess to perform the lifecycle
UPDATE at most once per configured interval per Store, tracking the last stamp
time alongside projectID with the same synchronization. Reset lastStampedAt
whenever UseProject selects a different project, while preserving the existing
fast-path and best-effort stamping behavior.
In `@internal/store/store.go`:
- Around line 130-149: Update the NewCollection doc comment to document that
legacy per-worktree databases may return a non-shared store, where project
selection is unavailable; instruct callers to check IsShared() before relying on
project membership.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5795ff8-f4c1-44cc-aa89-07c5aa0d12af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
.gitattributesCLAUDE.mdREADME.mdcmd/clean.gocmd/clean_test.gocmd/hook.gocmd/index.gocmd/index_test.gocmd/search.gocmd/stdio.gocmd/stdio_test.godocs/INDEX_STORAGE.mde2e_cli_test.gogo.modinternal/config/config.gointernal/config/config_test.gointernal/config/service.gointernal/config/version.gointernal/index/index.gointernal/index/index_concurrency_test.gointernal/index/migrate.gointernal/index/migrate_test.gointernal/index/shared.gointernal/indexlock/lock.gointernal/indexlock/lock_test.gointernal/sqlitevec/LICENSE-APACHEinternal/sqlitevec/LICENSE-MITinternal/sqlitevec/lib.gointernal/sqlitevec/lib_test.gointernal/sqlitevec/sqlite-vec.cinternal/sqlitevec/sqlite-vec.hinternal/store/hybrid_cte_test.gointernal/store/shared.gointernal/store/shared_test.gointernal/store/store.goskills/doctor/SKILL.mdskills/reindex/SKILL.md
| func runDailyCleanup(dataDir string, now time.Time) { | ||
| stampPath := filepath.Join(dataDir, ".last-cleanup") | ||
| if info, err := os.Stat(stampPath); err == nil && now.Sub(info.ModTime()) < dailyCleanupInterval { | ||
| return | ||
| } | ||
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | ||
| return | ||
| } | ||
| if err := cleanIndexes(io.Discard, io.Discard, dataDir, defaultCleanDays, now); err != nil { | ||
| return | ||
| } | ||
| _ = os.WriteFile(stampPath, []byte(now.UTC().Format(time.RFC3339)), 0o600) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the background logger for daily cleanup.
runDailyCleanup is an MCP-startup path. It discards all cleanup output and errors. Create a logger with newDebugLogger(). Log cleanup failures and skipped maintenance through slog. Close the returned log file when it is present.
As per coding guidelines, background and MCP execution must use slog with newDebugLogger() and must not mix interactive output into the background path.
🤖 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 `@cmd/clean.go` around lines 252 - 264, Update runDailyCleanup to create a
background logger via newDebugLogger(), replace io.Discard cleanup streams with
the logger, and report skipped maintenance and cleanup failures through slog.
Ensure the log file returned by newDebugLogger() is closed when non-nil, while
preserving the existing cleanup and stamp-file flow.
Source: Coding guidelines
caf66a7 to
0256cd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
internal/store/shared_test.go (1)
406-412: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip the size-ratio fixture in short mode.
This test writes 1000 vectors of 768 dimensions into three SQLite databases, including two full float32 legacy indexes. It runs on every
go test ./.... Atesting.Short()guard keeps the default developer loop fast while CI still runs the full assertion.♻️ Proposed refactor
func TestSharedInt8StorageAtMostTwentyPercentOfSeparateFloat32(t *testing.T) { + if testing.Short() { + t.Skip("storage size fixture writes three multi-megabyte databases") + } const ( dimensions = 768 chunkCount = 1000 )🤖 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 `@internal/store/shared_test.go` around lines 406 - 412, Update TestSharedInt8StorageAtMostTwentyPercentOfSeparateFloat32 to return immediately when testing.Short() is enabled, before creating temporary directories or writing fixture data; preserve the existing full size-ratio assertion for normal and CI test runs.internal/index/seed.go (2)
136-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefer the database close and wrap the errors.
setSeedProjectPathclosesdbmanually on five error paths. The repository guidelines require deferred cleanup of database resources and error wrapping instead of bare returns. Adeferremoves the risk that a future early return leaks the handle.♻️ Proposed refactor
-func setSeedProjectPath(ctx context.Context, dbPath, projectPath string) error { +func setSeedProjectPath(ctx context.Context, dbPath, projectPath string) (err error) { db, err := sql.Open("sqlite3", sqliteFileDSN(dbPath, "rw")) if err != nil { - return err + return fmt.Errorf("open seed snapshot: %w", err) } + defer func() { err = errors.Join(err, db.Close()) }() var shared bool if err := db.QueryRowContext(ctx, `SELECT EXISTS(SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'collection_meta')`, ).Scan(&shared); err != nil { - _ = db.Close() - return err + return fmt.Errorf("detect shared schema: %w", err) } if shared { if err := setSharedSeedProjectPath(ctx, db, projectPath); err != nil { - _ = db.Close() return err } } else if _, err := db.ExecContext(ctx, `INSERT INTO project_meta (key, value) VALUES ('project_path', ?) ON CONFLICT(key) DO UPDATE SET value = excluded.value`, projectPath, ); err != nil { - _ = db.Close() - return err + return fmt.Errorf("stamp project path: %w", err) } if _, err := db.ExecContext(ctx, "PRAGMA wal_checkpoint(TRUNCATE)"); err != nil { - _ = db.Close() - return err + return fmt.Errorf("checkpoint seed snapshot: %w", err) } - return db.Close() + return nil }As per coding guidelines: "Always defer cleanup of database and file resources, such as
defer Close()" and "Use proper error types and wrapping instead of generic error strings".🤖 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 `@internal/index/seed.go` around lines 136 - 167, Update setSeedProjectPath to defer closing db immediately after opening it, then remove the manual db.Close calls from error paths. Wrap errors returned from database queries, updates, checkpoint execution, and closing with contextual information while preserving the existing control flow and cleanup behavior.Source: Coding guidelines
81-92: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueScope the donor completeness check to one project.
In a shared collection,
project_metaholds oneroot_hashrow per project. This query has noproject_idfilter and noORDER BY, so SQLite returns an arbitrary row.setSharedSeedProjectPathlater selects a specific donor withvalue <> ''ordered bylast_accessed_at. The two checks can disagree: an emptyroot_hashfrom an unrelated in-progress project makes the seeder return(false, nil)even though a complete donor exists.Use the same selection predicate in both places.
♻️ Proposed change
- var rootHash sql.NullString - if err := db.QueryRowContext(ctx, "SELECT value FROM project_meta WHERE key = 'root_hash'").Scan(&rootHash); err != nil && !errors.Is(err, sql.ErrNoRows) { + var rootHash sql.NullString + if err := db.QueryRowContext(ctx, + "SELECT value FROM project_meta WHERE key = 'root_hash' AND value <> '' LIMIT 1", + ).Scan(&rootHash); err != nil && !errors.Is(err, sql.ErrNoRows) {🤖 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 `@internal/index/seed.go` around lines 81 - 92, Update the root_hash query in the donor completeness check to scope results to the target project and use the same non-empty value predicate and last_accessed_at ordering as setSharedSeedProjectPath. Keep the existing handling for missing or empty results, while ensuring the selected row represents the specific complete donor rather than an arbitrary project.cmd/stdio.go (1)
899-903: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the legacy migration scan once per indexer.
prepareMigrationFuncruns on every background reindex.PrepareLegacyMigration(internal/index/migrate.goLines 25-100) reads all legacy vectors, then reads and re-chunks every legacy file whose hash still matches disk.finishLegacyMigrationremoves the legacy database only after a reindex actually stores data. IfEnsureFreshreports the index as fresh, the legacy database stays on disk and the full scan repeats at every TTL expiry.Track the preparation per indexer and skip it after the first attempt.
♻️ Proposed refactor
Add a guard field to
indexerCachekeyed by project, for examplemigrationPrepared map[string]bool, and:- legacyPath := config.LegacyDBPathForProject(projectDir, modelName) - if err := prepareMigrationFunc(idx, projectDir, legacyPath); err != nil { - ic.logger().Warn("legacy index migration unavailable; rebuilding missing vectors", "path", legacyPath, "error", err) - } + if ic.markMigrationPrepared(reindexKey) { + legacyPath := config.LegacyDBPathForProject(projectDir, modelName) + if err := prepareMigrationFunc(idx, projectDir, legacyPath); err != nil { + ic.logger().Warn("legacy index migration unavailable; rebuilding missing vectors", "path", legacyPath, "error", err) + } + }
markMigrationPreparedreturns true the first time it sees the key.🤖 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 `@cmd/stdio.go` around lines 899 - 903, Track legacy migration preparation once per indexer by adding a project-keyed migrationPrepared guard to indexerCache and a markMigrationPrepared helper that returns true only on the first attempt. Use this guard around the prepareMigrationFunc call so subsequent background reindexes skip the legacy migration scan while preserving the existing warning behavior for the initial attempt.
🤖 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 `@cmd/stdio_test.go`:
- Around line 1419-1435: Replace the t.Fatalf calls inside the background
callbacks prepareMigrationFunc and ensureFreshFunc with t.Errorf, then return
immediately after reporting each failed assertion. Keep the existing validation
and expected-call checks intact while ensuring failures do not terminate the
non-test reindex goroutine.
In `@cmd/stdio.go`:
- Line 1477: Update the startup flow around runDailyCleanup so cleanup executes
asynchronously and does not delay MCP server startup or the first response.
Launch the cleanup in a goroutine before or after server.Run begins, while
preserving its existing arguments and daily-stamp behavior.
In `@e2e_cli_test.go`:
- Line 157: Update the t.Cleanup callback around db.Close to explicitly assign
the returned error to the blank identifier, preserving the existing cleanup
behavior while complying with the ignored-error guideline.
In `@internal/config/config.go`:
- Around line 50-58: Normalize the model name to its canonical registry value
before constructing the profile key in the profile-building logic around
ModelDimensions. Reuse the same alias resolution represented by
models.ModelAliases and models.KnownModels so aliases and canonical names
produce identical paths, then add a test asserting equal paths for both inputs.
In `@internal/index/index_test.go`:
- Around line 72-100: Move TestIndexerLastIndexedAtIsProjectScoped out of the
raw string fixture in TestIndexer_IndexAndSearch and place it after that test’s
closing brace as a separate top-level Go function. Keep the test body unchanged
so it executes normally and the main.go fixture retains only its intended
functions.
In `@internal/store/shared.go`:
- Around line 196-209: Make createCollectionSchema’s concurrent first-open setup
atomic: in internal/store/shared.go lines 196-209, replace the
ErrNoRows-then-INSERT flow with INSERT OR IGNORE followed by re-reading the
value so the existing != value comparison determines the result; in lines
211-227, remove the checkTableExists pre-check and create vec_vectors with
CREATE VIRTUAL TABLE IF NOT EXISTS.
- Around line 495-503: Update the vector_keys lookup in the transaction loop
handling vectors in shared.go so a missing row is converted to
store.ErrVectorVanished, while preserving other query errors unchanged. Ensure
the refresh path through internal/index/shared.go can recognize and retry this
sentinel consistently with the insert path.
---
Nitpick comments:
In `@cmd/stdio.go`:
- Around line 899-903: Track legacy migration preparation once per indexer by
adding a project-keyed migrationPrepared guard to indexerCache and a
markMigrationPrepared helper that returns true only on the first attempt. Use
this guard around the prepareMigrationFunc call so subsequent background
reindexes skip the legacy migration scan while preserving the existing warning
behavior for the initial attempt.
In `@internal/index/seed.go`:
- Around line 136-167: Update setSeedProjectPath to defer closing db immediately
after opening it, then remove the manual db.Close calls from error paths. Wrap
errors returned from database queries, updates, checkpoint execution, and
closing with contextual information while preserving the existing control flow
and cleanup behavior.
- Around line 81-92: Update the root_hash query in the donor completeness check
to scope results to the target project and use the same non-empty value
predicate and last_accessed_at ordering as setSharedSeedProjectPath. Keep the
existing handling for missing or empty results, while ensuring the selected row
represents the specific complete donor rather than an arbitrary project.
In `@internal/store/shared_test.go`:
- Around line 406-412: Update
TestSharedInt8StorageAtMostTwentyPercentOfSeparateFloat32 to return immediately
when testing.Short() is enabled, before creating temporary directories or
writing fixture data; preserve the existing full size-ratio assertion for normal
and CI test runs.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c808c17b-8561-4d90-a5c4-845ed11e79b0
📒 Files selected for processing (28)
.gitattributescmd/clean.gocmd/clean_test.gocmd/index.gocmd/stdio.gocmd/stdio_test.godocs/INDEX_STORAGE.mde2e_cli_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/service.gointernal/index/index.gointernal/index/index_concurrency_test.gointernal/index/index_test.gointernal/index/migrate.gointernal/index/migrate_test.gointernal/index/seed.gointernal/index/seed_test.gointernal/index/shared.gointernal/index/shared_batch_test.gointernal/indexlock/lock.gointernal/sqlitevec/lib.gointernal/sqlitevec/lib_test.gointernal/store/shared.gointernal/store/shared_test.gointernal/store/store.gointernal/store/store_test.goskills/reindex/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (13)
- .gitattributes
- internal/config/service.go
- internal/sqlitevec/lib.go
- internal/index/index_concurrency_test.go
- internal/indexlock/lock.go
- cmd/index.go
- cmd/clean.go
- internal/index/migrate.go
- internal/config/config_test.go
- docs/INDEX_STORAGE.md
- skills/reindex/SKILL.md
- internal/sqlitevec/lib_test.go
- internal/index/index.go
| prepareMigrationFunc = func(_ *index.Indexer, gotProject, _ string) error { | ||
| prepareCalls++ | ||
| if gotProject != projectDir { | ||
| t.Fatalf("project = %q, want %q", gotProject, projectDir) | ||
| } | ||
| return nil | ||
| } | ||
| ic := &indexerCache{ | ||
| embedder: &stubEmbedder{}, | ||
| cfg: cfg, | ||
| log: discardLog, | ||
| ensureFreshFunc: func(_ context.Context, _ *index.Indexer, _ string, _ index.ProgressFunc) (bool, index.Stats, error) { | ||
| if prepareCalls != 1 { | ||
| t.Fatalf("PrepareLegacyMigration calls before EnsureFresh = %d, want 1", prepareCalls) | ||
| } | ||
| return false, index.Stats{}, nil | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not call t.Fatalf from a non-test goroutine.
prepareMigrationFunc and ensureFreshFunc run on the background reindex goroutine started by ensureIndexed. t.Fatalf calls runtime.Goexit on the calling goroutine. From a non-test goroutine it stops that goroutine only. The test then blocks on done or reports a confusing failure. Use t.Errorf and return.
🐛 Proposed fix
prepareMigrationFunc = func(_ *index.Indexer, gotProject, _ string) error {
prepareCalls++
if gotProject != projectDir {
- t.Fatalf("project = %q, want %q", gotProject, projectDir)
+ t.Errorf("project = %q, want %q", gotProject, projectDir)
}
return nil
}
@@
ensureFreshFunc: func(_ context.Context, _ *index.Indexer, _ string, _ index.ProgressFunc) (bool, index.Stats, error) {
if prepareCalls != 1 {
- t.Fatalf("PrepareLegacyMigration calls before EnsureFresh = %d, want 1", prepareCalls)
+ t.Errorf("PrepareLegacyMigration calls before EnsureFresh = %d, want 1", prepareCalls)
}
return false, index.Stats{}, nil
},📝 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.
| prepareMigrationFunc = func(_ *index.Indexer, gotProject, _ string) error { | |
| prepareCalls++ | |
| if gotProject != projectDir { | |
| t.Fatalf("project = %q, want %q", gotProject, projectDir) | |
| } | |
| return nil | |
| } | |
| ic := &indexerCache{ | |
| embedder: &stubEmbedder{}, | |
| cfg: cfg, | |
| log: discardLog, | |
| ensureFreshFunc: func(_ context.Context, _ *index.Indexer, _ string, _ index.ProgressFunc) (bool, index.Stats, error) { | |
| if prepareCalls != 1 { | |
| t.Fatalf("PrepareLegacyMigration calls before EnsureFresh = %d, want 1", prepareCalls) | |
| } | |
| return false, index.Stats{}, nil | |
| }, | |
| prepareMigrationFunc = func(_ *index.Indexer, gotProject, _ string) error { | |
| prepareCalls++ | |
| if gotProject != projectDir { | |
| t.Errorf("project = %q, want %q", gotProject, projectDir) | |
| } | |
| return nil | |
| } | |
| ic := &indexerCache{ | |
| embedder: &stubEmbedder{}, | |
| cfg: cfg, | |
| log: discardLog, | |
| ensureFreshFunc: func(_ context.Context, _ *index.Indexer, _ string, _ index.ProgressFunc) (bool, index.Stats, error) { | |
| if prepareCalls != 1 { | |
| t.Errorf("PrepareLegacyMigration calls before EnsureFresh = %d, want 1", prepareCalls) | |
| } | |
| return false, index.Stats{}, nil | |
| }, |
🤖 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 `@cmd/stdio_test.go` around lines 1419 - 1435, Replace the t.Fatalf calls
inside the background callbacks prepareMigrationFunc and ensureFreshFunc with
t.Errorf, then return immediately after reporting each failed assertion. Keep
the existing validation and expected-call checks intact while ensuring failures
do not terminate the non-test reindex goroutine.
| "backend", cfg.Servers()[0].Backend, | ||
| "freshness_ttl", cfg.FreshnessTTL().String(), | ||
| ) | ||
| runDailyCleanup(filepath.Join(config.XDGDataDir(), "lumen"), time.Now(), logger) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not block stdio startup with the cleanup pass.
runDailyCleanup runs before the MCP server starts. Cleanup walks every index directory, opens the SQLite collections, and can reclaim space. On a large data directory this delays the first MCP response and can trip a client startup timeout. The .last-cleanup stamp limits the cost to once per day, but that first run of the day pays the whole cost.
Start the cleanup in a goroutine, or run it after server.Run begins.
🐛 Proposed fix
- runDailyCleanup(filepath.Join(config.XDGDataDir(), "lumen"), time.Now(), logger)
+ go runDailyCleanup(filepath.Join(config.XDGDataDir(), "lumen"), time.Now(), logger)📝 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.
| runDailyCleanup(filepath.Join(config.XDGDataDir(), "lumen"), time.Now(), logger) | |
| go runDailyCleanup(filepath.Join(config.XDGDataDir(), "lumen"), time.Now(), logger) |
🤖 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 `@cmd/stdio.go` at line 1477, Update the startup flow around runDailyCleanup so
cleanup executes asynchronously and does not delay MCP server startup or the
first response. Launch the cleanup in a goroutine before or after server.Run
begins, while preserving its existing arguments and daily-stamp behavior.
| if err != nil { | ||
| t.Fatalf("open index db: %v", err) | ||
| } | ||
| t.Cleanup(func() { db.Close() }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assign the ignored Close error explicitly.
db.Close() returns an error that this cleanup discards implicitly. The coding guidelines require an explicit _ = err form.
🛡️ Proposed fix
- t.Cleanup(func() { db.Close() })
+ t.Cleanup(func() { _ = db.Close() })As per coding guidelines: "When intentionally ignoring an error, explicitly assign it to _ = err".
📝 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.
| t.Cleanup(func() { db.Close() }) | |
| t.Cleanup(func() { _ = db.Close() }) |
🤖 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 `@e2e_cli_test.go` at line 157, Update the t.Cleanup callback around db.Close
to explicitly assign the returned error to the blank identifier, preserving the
existing cleanup behavior while complying with the ignored-error guideline.
Source: Coding guidelines
| // ModelDimensions resolves dimensions for a model in the built-in registry. | ||
| func ModelDimensions(model string) (int, bool) { | ||
| canonical := model | ||
| if resolved, ok := models.ModelAliases[model]; ok { | ||
| canonical = resolved | ||
| } | ||
| spec, ok := models.KnownModels[canonical] | ||
| return spec.Dims, ok | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use the canonical model name in the profile key.
Lines 52-57 resolve an alias for dimension lookup, but lines 85-87 retain the unnormalized model value. An alias and its canonical model therefore produce separate shared collections despite resolving to the same registered model. Normalize the model name before building the profile string. Add a test that asserts equal paths for an alias and its canonical model.
Proposed fix
+func canonicalModel(model string) string {
+ if resolved, ok := models.ModelAliases[model]; ok {
+ return resolved
+ }
+ return model
+}
+
func ModelDimensions(model string) (int, bool) {
- canonical := model
- if resolved, ok := models.ModelAliases[model]; ok {
- canonical = resolved
- }
- spec, ok := models.KnownModels[canonical]
+ spec, ok := models.KnownModels[canonicalModel(model)]
return spec.Dims, ok
}
- profile := identity + "\x00" + scope + "\x00" + model + "\x00" +
+ profile := identity + "\x00" + scope + "\x00" + canonicalModel(model) + "\x00" +Also applies to: 85-87
🤖 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 `@internal/config/config.go` around lines 50 - 58, Normalize the model name to
its canonical registry value before constructing the profile key in the
profile-building logic around ModelDimensions. Reuse the same alias resolution
represented by models.ModelAliases and models.KnownModels so aliases and
canonical names produce identical paths, then add a test asserting equal paths
for both inputs.
| func TestIndexerLastIndexedAtIsProjectScoped(t *testing.T) { | ||
| projectA, projectB := t.TempDir(), t.TempDir() | ||
| idx, err := NewIndexerForProject(":memory:", &mockEmbedder{dims: 4, model: "test-model"}, 512, "int8", projectA) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer func() { _ = idx.Close() }() | ||
| timeA := time.Now().Add(-time.Hour).UTC().Truncate(time.Second) | ||
| timeB := time.Now().UTC().Truncate(time.Second) | ||
| if err := idx.store.SetMeta("last_indexed_at", timeA.Format(time.RFC3339)); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| release, err := idx.lockProject(projectB) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := idx.store.SetMeta("last_indexed_at", timeB.Format(time.RFC3339)); err != nil { | ||
| release() | ||
| t.Fatal(err) | ||
| } | ||
| release() | ||
| if got, ok := idx.LastIndexedAt(projectA); !ok || !got.Equal(timeA) { | ||
| t.Fatalf("project A LastIndexedAt = %v, %v; want %v, true", got, ok, timeA) | ||
| } | ||
| if got, ok := idx.LastIndexedAt(projectB); !ok || !got.Equal(timeB) { | ||
| t.Fatalf("project B LastIndexedAt = %v, %v; want %v, true", got, ok, timeB) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Move the new test out of the raw string literal.
TestIndexer_IndexAndSearch opens a raw string literal at Line 63 and closes it at Line 105. Lines 72-100 sit inside that literal. The Go compiler treats the new test as fixture text, so TestIndexerLastIndexedAtIsProjectScoped never runs. The main.go fixture also receives the test source instead of the intended two functions.
Move the function after the closing } of TestIndexer_IndexAndSearch.
🐛 Proposed fix
// Hello prints a greeting.
func Hello(name string) {
fmt.Println("hello", name)
}
-func TestIndexerLastIndexedAtIsProjectScoped(t *testing.T) {
- projectA, projectB := t.TempDir(), t.TempDir()
- idx, err := NewIndexerForProject(":memory:", &mockEmbedder{dims: 4, model: "test-model"}, 512, "int8", projectA)
- if err != nil {
- t.Fatal(err)
- }
- defer func() { _ = idx.Close() }()
- timeA := time.Now().Add(-time.Hour).UTC().Truncate(time.Second)
- timeB := time.Now().UTC().Truncate(time.Second)
- if err := idx.store.SetMeta("last_indexed_at", timeA.Format(time.RFC3339)); err != nil {
- t.Fatal(err)
- }
- release, err := idx.lockProject(projectB)
- if err != nil {
- t.Fatal(err)
- }
- if err := idx.store.SetMeta("last_indexed_at", timeB.Format(time.RFC3339)); err != nil {
- release()
- t.Fatal(err)
- }
- release()
- if got, ok := idx.LastIndexedAt(projectA); !ok || !got.Equal(timeA) {
- t.Fatalf("project A LastIndexedAt = %v, %v; want %v, true", got, ok, timeA)
- }
- if got, ok := idx.LastIndexedAt(projectB); !ok || !got.Equal(timeB) {
- t.Fatalf("project B LastIndexedAt = %v, %v; want %v, true", got, ok, timeB)
- }
-}
-
// Goodbye prints a farewell.
func Goodbye(name string) {
fmt.Println("bye", name)
}
`)
+
+ emb := &mockEmbedder{dims: 4, model: "test-model"}
+ // ... rest of TestIndexer_IndexAndSearch unchanged ...Then add the test as a separate top-level function:
func TestIndexerLastIndexedAtIsProjectScoped(t *testing.T) {
projectA, projectB := t.TempDir(), t.TempDir()
idx, err := NewIndexerForProject(":memory:", &mockEmbedder{dims: 4, model: "test-model"}, 512, "int8", projectA)
if err != nil {
t.Fatal(err)
}
defer func() { _ = idx.Close() }()
// ... body unchanged ...
}🤖 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 `@internal/index/index_test.go` around lines 72 - 100, Move
TestIndexerLastIndexedAtIsProjectScoped out of the raw string fixture in
TestIndexer_IndexAndSearch and place it after that test’s closing brace as a
separate top-level Go function. Keep the test body unchanged so it executes
normally and the main.go fixture retains only its intended functions.
| for key, value := range want { | ||
| var existing string | ||
| err := db.QueryRow(`SELECT value FROM collection_meta WHERE key = ?`, key).Scan(&existing) | ||
| switch { | ||
| case err == sql.ErrNoRows: | ||
| if _, err := db.Exec(`INSERT INTO collection_meta(key, value) VALUES (?, ?)`, key, value); err != nil { | ||
| return err | ||
| } | ||
| case err != nil: | ||
| return err | ||
| case existing != value: | ||
| return fmt.Errorf("collection profile mismatch for %s: stored %q, requested %q", key, existing, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make first-open schema setup idempotent for concurrent worktrees. createCollectionSchema uses check-then-write sequences that are not atomic. This PR lets several worktrees open one collection at the same time, so on a brand-new collection the losing process fails with a low-level UNIQUE or "table already exists" error instead of the intended profile comparison or a clean success.
internal/store/shared.go#L196-L209: replace theErrNoRows-then-INSERTsequence withINSERT OR IGNOREfollowed by a re-read, so theexisting != valuebranch always decides the outcome.internal/store/shared.go#L211-L227: drop thecheckTableExistspre-check and useCREATE VIRTUAL TABLE IF NOT EXISTS vec_vectors USING vec0(...).
📍 Affects 1 file
internal/store/shared.go#L196-L209(this comment)internal/store/shared.go#L211-L227
🤖 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 `@internal/store/shared.go` around lines 196 - 209, Make
createCollectionSchema’s concurrent first-open setup atomic: in
internal/store/shared.go lines 196-209, replace the ErrNoRows-then-INSERT flow
with INSERT OR IGNORE followed by re-reading the value so the existing != value
comparison determines the result; in lines 211-227, remove the checkTableExists
pre-check and create vec_vectors with CREATE VIRTUAL TABLE IF NOT EXISTS.
| for position, vec := range vectors { | ||
| if position < 0 || position >= len(chunks) { | ||
| return false, fmt.Errorf("vector position %d out of range", position) | ||
| } | ||
| h := embeddingInputHash(chunks[position]) | ||
| var vectorID int64 | ||
| if err := tx.QueryRow(`SELECT id FROM vector_keys WHERE input_hash = ?`, h[:]).Scan(&vectorID); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=go -C 6 'ErrVectorVanished'Repository: ory/lumen
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the shared store segment plus all relevant vector/error-related identifiers without executing repository code.
fd -a 'shared\.go$' . | sed 's#^\./##'
printf '\n--- internal/store/shared.go 400-530 ---\n'
sed -n '400,530p' internal/store/shared.go
printf '\n--- vector-related declarations/usages ---\n'
rg -n --type=go -C 4 'vector_keys|embeddingInputHash|ErrVectorVanished|VectorVanished|reindex|force' .Repository: ory/lumen
Length of output: 50365
Map vanished shared vectors to store.ErrVectorVanished during refresh.
internal/index/shared.go:126 retries storeFile(file) only on store.ErrVectorVanished, but this branch now returns a raw lookup error when vector_keys is missing. A concurrent GC can make the same missing vector_key behavior different in the insert and refresh paths, so unify it at the lookup.
🤖 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 `@internal/store/shared.go` around lines 495 - 503, Update the vector_keys
lookup in the transaction loop handling vectors in shared.go so a missing row is
converted to store.ErrVectorVanished, while preserving other query errors
unchanged. Ensure the refresh path through internal/index/shared.go can
recognize and retry this sentinel consistently with the insert path.
1 participant
Summary
This replaces per-worktree databases with repository-scoped, content-addressed collections that deduplicate file revisions and filepath-aware vectors while preserving project-local exact KNN search and concurrent indexing. It makes int8 vectors the default with a float32 override, adds profile-aware paths, lazy legacy-vector migration, project-aware garbage collection and daily cleanup, locking, and expanded
index_statusstorage metrics. It also vendors sqlite-vec v0.1.9 behind an internal wrapper and updates the README, architecture notes, storage lifecycle guide, and doctor/reindex guidance.Verification
make build-local,make test,make lint, andgit diff --checkall pass.Summary by CodeRabbit
New Features
LUMEN_VECTOR_STORAGE(int8orfloat32).Bug Fixes
Documentation