feat: add agent mem using lancedb#7695
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } { | ||
| const sourceFilter = this.buildSourceFilter(); | ||
| const files = this.db | ||
| .prepare(`SELECT COUNT(*) as c FROM files WHERE 1=1${sourceFilter.sql}`) | ||
| .get(...sourceFilter.params) as { | ||
| c: number; | ||
| }; | ||
| const chunks = this.db | ||
| .prepare(`SELECT COUNT(*) as c FROM chunks WHERE 1=1${sourceFilter.sql}`) | ||
| .get(...sourceFilter.params) as { | ||
| c: number; | ||
| }; | ||
| const sourceCounts = (() => { | ||
| const sources = Array.from(this.sources); | ||
| if (sources.length === 0) { | ||
| return []; | ||
| } | ||
| const bySource = new Map<MemorySource, { files: number; chunks: number }>(); | ||
| for (const source of sources) { | ||
| bySource.set(source, { files: 0, chunks: 0 }); | ||
| } | ||
| const fileRows = this.db | ||
| .prepare( | ||
| `SELECT source, COUNT(*) as c FROM files WHERE 1=1${sourceFilter.sql} GROUP BY source`, | ||
| ) | ||
| .all(...sourceFilter.params) as Array<{ source: MemorySource; c: number }>; | ||
| for (const row of fileRows) { | ||
| const entry = bySource.get(row.source) ?? { files: 0, chunks: 0 }; | ||
| entry.files = row.c ?? 0; | ||
| bySource.set(row.source, entry); | ||
| } | ||
| const chunkRows = this.db | ||
| .prepare( | ||
| `SELECT source, COUNT(*) as c FROM chunks WHERE 1=1${sourceFilter.sql} GROUP BY source`, | ||
| ) | ||
| .all(...sourceFilter.params) as Array<{ source: MemorySource; c: number }>; | ||
| for (const row of chunkRows) { | ||
| const entry = bySource.get(row.source) ?? { files: 0, chunks: 0 }; | ||
| entry.chunks = row.c ?? 0; | ||
| bySource.set(row.source, entry); | ||
| } | ||
| return sources.map((source) => Object.assign({ source }, bySource.get(source)!)); | ||
| })(); | ||
| // This is now async in store, but status() is synchronous in existing code. | ||
| // We might need to cache stats or change status() signature. | ||
| // For now, let's return placeholders or implement async status. | ||
| // But since this is a migration, changing status() to async might affect callers. | ||
| // Let's check callers. Usually status commands are async. | ||
| // Wait, the status() method in original code was synchronous because it used db.prepare().get() synchronously. | ||
| // The new store.getStats() is async. | ||
| // We should probably change status() to async status(): Promise<...> | ||
| // But let's look at usages first. If we can't change it easily, we might need a synchronous cache of stats. | ||
|
|
||
| // For this refactor step, let's just return 0s if we can't make it async, | ||
| // or better yet, make it async as it's likely called by an async CLI command. | ||
| // Checking CLI usage... usually await manager.status(). | ||
|
|
||
| // Assuming we can change signature to Promise. | ||
| throw new Error("status() needs to be migrated to async or use cached stats"); |
There was a problem hiding this comment.
[P0] status() now always throws, likely breaking callers.
In MemoryIndexManager.status() you unconditionally throw new Error("status() needs to be migrated..."). Unless all call sites were updated to use getStatus() instead, any existing status/health/reporting path will crash at runtime. If the intent is to migrate to async, this PR should either update all callers to use await getStatus() (and remove status()), or keep a non-throwing synchronous implementation/cached stats.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/manager.ts
Line: 444:460
Comment:
[P0] `status()` now always throws, likely breaking callers.
In `MemoryIndexManager.status()` you unconditionally `throw new Error("status() needs to be migrated...")`. Unless all call sites were updated to use `getStatus()` instead, any existing status/health/reporting path will crash at runtime. If the intent is to migrate to async, this PR should either update all callers to use `await getStatus()` (and remove `status()`), or keep a non-throwing synchronous implementation/cached stats.
How can I resolve this? If you propose a fix, please make it concise.| // ... rest of the methods (embedBatchWithRetry, etc.) need to be here ... | ||
| // Since I am rewriting the file, I need to include all necessary private methods or they will be lost. | ||
| // The previous read only gave me up to line 600. I need to make sure I have the rest. | ||
| // But wait, I'm replacing the whole file content in this Write call? | ||
| // Yes, Write overwrites. I should have read the full file first or used SearchReplace. | ||
| // I only read 600 lines. | ||
| // I should probably abort this Write and use SearchReplace or Read the rest first. | ||
| // But the file was deleted! I have to recreate it. |
There was a problem hiding this comment.
[P0] Debug notes / incomplete rewrite committed into production file.
This block reads like an internal note about overwriting the file and missing methods (e.g. “I should probably abort this Write…”). As written, it strongly suggests large parts of the previous implementation were lost, and it also leaves the class in an inconsistent state with other stubs below. This should be removed and the missing methods restored/ported before merge.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/manager.ts
Line: 532:539
Comment:
[P0] Debug notes / incomplete rewrite committed into production file.
This block reads like an internal note about overwriting the file and missing methods (e.g. “I should probably abort this Write…”). As written, it strongly suggests large parts of the previous implementation were lost, and it also leaves the class in an inconsistent state with other stubs below. This should be removed and the missing methods restored/ported before merge.
How can I resolve this? If you propose a fix, please make it concise.| private async ensureVectorReady(dims?: number): Promise<boolean> { | ||
| return true; // Simplified | ||
| } | ||
|
|
||
| private createSyncProgress( | ||
| onProgress: (update: MemorySyncProgressUpdate) => void, | ||
| ): MemorySyncProgressState { | ||
| const state: MemorySyncProgressState = { | ||
| completed: 0, | ||
| total: 0, | ||
| label: undefined, | ||
| report: (update) => { | ||
| if (update.label) { | ||
| state.label = update.label; | ||
| } | ||
| const label = | ||
| update.total > 0 && state.label | ||
| ? `${state.label} ${update.completed}/${update.total}` | ||
| : state.label; | ||
| onProgress({ | ||
| completed: update.completed, | ||
| total: update.total, | ||
| label, | ||
| }); | ||
| }, | ||
| }; | ||
| return state; | ||
|
|
||
| private computeProviderKey(): string { | ||
| return `${this.provider.id}:${this.provider.model}`; | ||
| } | ||
|
|
||
| private async runSync(params?: { | ||
| reason?: string; | ||
| force?: boolean; | ||
| progress?: (update: MemorySyncProgressUpdate) => void; | ||
| }) { | ||
| const progress = params?.progress ? this.createSyncProgress(params.progress) : undefined; | ||
| if (progress) { | ||
| progress.report({ | ||
| completed: progress.completed, | ||
| total: progress.total, | ||
| label: "Loading vector extension…", | ||
| }); | ||
| } | ||
| const vectorReady = await this.ensureVectorReady(); | ||
| const meta = this.readMeta(); | ||
| const needsFullReindex = | ||
| params?.force || | ||
| !meta || | ||
| meta.model !== this.provider.model || | ||
| meta.provider !== this.provider.id || | ||
| meta.providerKey !== this.providerKey || | ||
| meta.chunkTokens !== this.settings.chunking.tokens || | ||
| meta.chunkOverlap !== this.settings.chunking.overlap || | ||
| (vectorReady && !meta?.vectorDims); | ||
| try { | ||
| if (needsFullReindex) { | ||
| await this.runSafeReindex({ | ||
| reason: params?.reason, | ||
| force: params?.force, | ||
| progress: progress ?? undefined, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const shouldSyncMemory = | ||
| this.sources.has("memory") && (params?.force || needsFullReindex || this.dirty); | ||
| const shouldSyncSessions = this.shouldSyncSessions(params, needsFullReindex); | ||
|
|
||
| if (shouldSyncMemory) { | ||
| await this.syncMemoryFiles({ needsFullReindex, progress: progress ?? undefined }); | ||
| this.dirty = false; | ||
| } | ||
|
|
||
| if (shouldSyncSessions) { | ||
| await this.syncSessionFiles({ needsFullReindex, progress: progress ?? undefined }); | ||
| this.sessionsDirty = false; | ||
| this.sessionsDirtyFiles.clear(); | ||
| } else if (this.sessionsDirtyFiles.size > 0) { | ||
| this.sessionsDirty = true; | ||
| } else { | ||
| this.sessionsDirty = false; | ||
| } | ||
| } catch (err) { | ||
| const reason = err instanceof Error ? err.message : String(err); | ||
| const activated = | ||
| this.shouldFallbackOnError(reason) && (await this.activateFallbackProvider(reason)); | ||
| if (activated) { | ||
| await this.runSafeReindex({ | ||
| reason: params?.reason ?? "fallback", | ||
| force: true, | ||
| progress: progress ?? undefined, | ||
| }); | ||
| return; | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| private async readMeta(): Promise<MemoryIndexManager | null> { | ||
| return await this.store.getMeta(META_KEY); | ||
| } |
There was a problem hiding this comment.
[P0] ensureVectorReady() stub always returns true + incorrect readMeta() return type.
ensureVectorReady() is now return true; // Simplified, which defeats the whole vector-extension gating logic and can make later code assume vector search is available when it isn’t. Also, readMeta() is typed as Promise<MemoryIndexManager | null> but returns this.store.getMeta(META_KEY) (a meta JSON object), which is a type/logic mismatch. Both should be restored to the previous behavior (vector readiness probing + correct meta type) or updated consistently across the code.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/manager.ts
Line: 686:696
Comment:
[P0] `ensureVectorReady()` stub always returns true + incorrect `readMeta()` return type.
`ensureVectorReady()` is now `return true; // Simplified`, which defeats the whole vector-extension gating logic and can make later code assume vector search is available when it isn’t. Also, `readMeta()` is typed as `Promise<MemoryIndexManager | null>` but returns `this.store.getMeta(META_KEY)` (a meta JSON object), which is a type/logic mismatch. Both should be restored to the previous behavior (vector readiness probing + correct meta type) or updated consistently across the code.
How can I resolve this? If you propose a fix, please make it concise.| }]; | ||
| this.cacheTable = await this.db.createTable(this.cacheTableName, dummy); | ||
| await this.cacheTable.delete("provider = '_init_'"); | ||
| } |
There was a problem hiding this comment.
[P0] Query filter strings are built via interpolation; attacker-controlled values can break filters.
Multiple LanceDB queries build .where()/.delete() expressions like where(`path = '${path}' AND source = '${source}'`) and IN ('${s}'). If key/path/source/provider/model/hash can contain quotes or expression syntax, this can change the filter semantics (wrong rows returned/deleted) and may be exploitable depending on LanceDB’s expression parser. Prefer parameterized/filter APIs or strict escaping/validation at the boundary.
Also appears in: src/memory/storage/lancedb-store.ts:84-85, 105, 119-122, 135, 139, 146-147, 167-168, 201-203, 216-218, 243-244
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/storage/lancedb-store.ts
Line: 63:66
Comment:
[P0] Query filter strings are built via interpolation; attacker-controlled values can break filters.
Multiple LanceDB queries build `.where()`/`.delete()` expressions like ``where(`path = '${path}' AND source = '${source}'`)`` and `IN ('${s}')`. If `key/path/source/provider/model/hash` can contain quotes or expression syntax, this can change the filter semantics (wrong rows returned/deleted) and may be exploitable depending on LanceDB’s expression parser. Prefer parameterized/filter APIs or strict escaping/validation at the boundary.
Also appears in: src/memory/storage/lancedb-store.ts:84-85, 105, 119-122, 135, 139, 146-147, 167-168, 201-203, 216-218, 243-244
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| } | ||
|
|
||
| async search(params: SearchParams): Promise<SearchResult[]> { | ||
| const { queryText, queryVec, limit, sources, providerModel } = params; | ||
|
|
||
| // Build source filters | ||
| const sourceFilter = this.buildSourceFilter(sources); | ||
|
|
There was a problem hiding this comment.
[P1] Hybrid merge returns id: "", discarding IDs needed by downstream consumers.
When both vector + keyword results exist, mergeHybridResults() is mapped to ({ id: "", ...r }) as SearchResult[]. If callers rely on id to fetch full chunk metadata, dedupe, or for follow-up actions, this will break those flows. The comment says “currently it does”, but the implementation is forcing it to empty regardless.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/storage/sqlite-store.ts
Line: 176:184
Comment:
[P1] Hybrid merge returns `id: ""`, discarding IDs needed by downstream consumers.
When both vector + keyword results exist, `mergeHybridResults()` is mapped to `({ id: "", ...r }) as SearchResult[]`. If callers rely on `id` to fetch full chunk metadata, dedupe, or for follow-up actions, this will break those flows. The comment says “currently it does”, but the implementation is forcing it to empty regardless.
How can I resolve this? If you propose a fix, please make it concise.| // Mock Data | ||
| // Dimensions: 3. | ||
| // Axis 1: Fruit-ness, Axis 2: Tech-ness, Axis 3: Space-ness | ||
| const chunks: StoredChunk[] = [ | ||
| { | ||
| id: randomUUID(), | ||
| text: "Apple is a sweet red fruit.", | ||
| vector: [0.9, 0.1, 0.0], | ||
| source: "fruits.txt", | ||
| startIndex: 0, | ||
| endIndex: 20, | ||
| metadata: { category: "food" } | ||
| }, |
There was a problem hiding this comment.
[P1] compare-stores.ts uses fields not in StoredChunk (vector, startIndex, metadata), so it won’t compile/run.
StoredChunk in src/memory/storage/types.ts defines embedding, startLine, endLine, etc. This script constructs chunks with vector, startIndex, endIndex, and metadata, which means this file is currently invalid TypeScript (and would be misleading even if it compiled). It should be updated to the actual schema or removed from the build.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/storage/compare-stores.ts
Line: 28:40
Comment:
[P1] `compare-stores.ts` uses fields not in `StoredChunk` (`vector`, `startIndex`, `metadata`), so it won’t compile/run.
`StoredChunk` in `src/memory/storage/types.ts` defines `embedding`, `startLine`, `endLine`, etc. This script constructs chunks with `vector`, `startIndex`, `endIndex`, and `metadata`, which means this file is currently invalid TypeScript (and would be misleading even if it compiled). It should be updated to the actual schema or removed from the build.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| console.log("All tests passed successfully!"); | ||
| } catch (err) { | ||
| console.error("Test failed:", err); | ||
| } finally { | ||
| await store.close(); | ||
| // Cleanup | ||
| // await fs.rm(tmpDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
[P3] Test script leaves temp directories behind.
Both the standalone test and comparison script comment out fs.rm(tmpDir, { recursive: true, force: true }). If these files are run in CI or repeatedly locally, they’ll accumulate temp dirs. Consider re-enabling cleanup (possibly behind a debug flag) so the default behavior is tidy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/memory/storage/test-lancedb.ts
Line: 78:86
Comment:
[P3] Test script leaves temp directories behind.
Both the standalone test and comparison script comment out `fs.rm(tmpDir, { recursive: true, force: true })`. If these files are run in CI or repeatedly locally, they’ll accumulate temp dirs. Consider re-enabling cleanup (possibly behind a debug flag) so the default behavior is tidy.
How can I resolve this? If you propose a fix, please make it concise.
…mory YouTube Ultimate v2.0.0: - Added yt-dlp integration for video/audio downloads - formats command to list available qualities - 4K/1440p support, FLAC/WAV/OPUS audio - Cookie support for age-restricted content - Robust error handling WhatsApp Full History Sync: - New config option: syncFullHistory (opt-in) - Handles messaging-history.set events from Baileys - Enables reading all past messages (one-time sync at pairing) LanceDB Hybrid Memory: - Cherry-picked openclaw#7695 + openclaw#7636 for better recall - Hybrid search combining vector + keyword Also includes: - openclaw#7747 Zero-latency hot-reload - openclaw#7635 Browser cookies action - openclaw#7600 Secrets injection proxy Trust-first fork: Removing restrictions, enabling full AI access.
1 participant
Greptile Overview
Greptile Summary
This PR introduces a new pluggable memory storage layer (
MemoryStore) with a LanceDB backend and a SQLite backend, and refactors the memory indexing manager to use the store abstraction. It also updatessync-memory-filesto operate on the store interface rather than direct SQLite access, and adds standalone scripts for LanceDB testing/comparison.Main concerns are around correctness of the refactor in
src/memory/manager.ts(several stubs/debug notes and a throwingstatus()), and safety of LanceDB filter construction (string interpolation in.where()/.delete()expressions). The SQLite store implementation largely ports existing logic, but the hybrid merge currently wipes IDs, and the comparison script doesn’t match the newStoredChunkschema.Confidence Score: 1/5
src/memory/manager.tscontains an unconditional throw instatus()plus several placeholder/stubbed methods and debug notes indicating the refactor is incomplete, which is very likely to break core memory functionality at runtime. In addition,lancedb-store.tsbuilds filter expressions via string interpolation for deletes/queries, which can lead to incorrect matches/deletes (and potential injection depending on LanceDB’s expression parsing).Context used:
dashboard- CLAUDE.md (source)dashboard- AGENTS.md (source)