feat: Mem0 feature parity — smart evolution, memory hierarchy, search enhancements, graph memory#4
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ts, files) Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…servation updates Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…tion Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…peline Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…l tools Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…g pipeline Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…rompt Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add entities, entity_relations, entity_observations tables with FTS5 search, sync triggers, CHECK constraints, and unique indexes. Add entityExtractionEnabled config option (default: false, env: OPEN_MEM_ENTITY_EXTRACTION). Update existing migration test assertions for new migration count. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…pipeline Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds AI-driven conflict evaluation and entity extraction, user-level cross-project memory with FTS, entity graph support, graph-augmented search and reranking, observation supersede tracking (migrations v8–v9), and wires these into the queue, search orchestrator, servers, tools, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueueProcessor
participant Embedding as EmbeddingModel
participant ObservationRepo
participant ConflictEval as ConflictEvaluator
participant DB as Database
User->>QueueProcessor: enqueue observation
QueueProcessor->>Embedding: generate embedding
QueueProcessor->>ObservationRepo: find similar candidates
alt similarity > bandHigh
QueueProcessor->>DB: mark as duplicate / skip creation
else similarity in gray zone
QueueProcessor->>ConflictEval: evaluate(newObs, candidates)
alt outcome == "update"
ConflictEval-->>QueueProcessor: { outcome: update, supersedesId }
QueueProcessor->>DB: create new observation
QueueProcessor->>ObservationRepo: supersede(oldId, newId)
else outcome == "duplicate"
ConflictEval-->>QueueProcessor: { outcome: duplicate }
QueueProcessor->>DB: skip creation
else outcome == "new_fact"
ConflictEval-->>QueueProcessor: { outcome: new_fact }
QueueProcessor->>DB: create new observation
end
else
QueueProcessor->>DB: create new observation
end
sequenceDiagram
participant User
participant QueueProcessor
participant EntityExtractor
participant LLM as LanguageModel
participant EntityRepo
participant DB as Database
User->>QueueProcessor: enqueue observation
QueueProcessor->>EntityExtractor: extract(observation)
EntityExtractor->>LLM: send extraction prompt
LLM-->>EntityExtractor: XML extraction response
EntityExtractor->>EntityRepo: upsert entities & create relations
EntityRepo->>DB: persist entities/relations/links
EntityExtractor-->>QueueProcessor: ParsedEntityExtraction
QueueProcessor->>DB: continue processing (link observation, etc.)
sequenceDiagram
participant Client
participant SearchOrch as SearchOrchestrator
participant Hybrid as HybridSearch
participant EntityRepo
participant UserMemory
participant Reranker
participant Results
Client->>SearchOrch: search(query, options)
SearchOrch->>Hybrid: run FTS + vector
Hybrid-->>SearchOrch: baseResults
alt entityRepo available
SearchOrch->>EntityRepo: traverse related entities
EntityRepo-->>SearchOrch: related observation IDs
SearchOrch->>SearchOrch: merge graph results (source=project)
end
alt userMemory enabled
SearchOrch->>UserMemory: search user observations
UserMemory-->>SearchOrch: userResults (source=user)
SearchOrch->>SearchOrch: merge & dedupe results
end
alt reranker available
SearchOrch->>Reranker: rerank(query, mergedResults, limit)
Reranker-->>Results: rerankedResults
else
SearchOrch-->>Results: mergedResults
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryImplements comprehensive Mem0 feature parity across 4 tracks with 7,661 lines added. All features are opt-in via config flags (defaulting to Track A — Smart Memory Evolution: Introduced LLM-based conflict resolution that evaluates new observations against similar existing ones using a two-threshold similarity system ( Track B — Memory Hierarchy: User-level cross-project memory database at Track C — Search Enhancements: Dual-mode reranking with LLM-based primary and heuristic fallback. Advanced filters for importance range, date range, concepts, and files. Graceful degradation ensures search succeeds even when AI services fail. Track D — Graph Memory: Entity-relationship extraction creates SQLite-backed knowledge graph. BFS traversal with depth cap (max 2) and visited node limit (100) prevents memory explosion in dense graphs (addresses PR feedback). Graph-augmented search discovers related observations through entity relationships, expanding search coverage beyond direct keyword matches. Architecture: All new subsystems integrate cleanly into existing plugin lifecycle with proper initialization, feature flag checks, and error handling. The 201 new tests provide solid coverage of core scenarios (790 total tests passing). Entity extraction prompt relaxed from "clearly mentioned" to include "strongly implied" relationships per PR feedback to improve knowledge graph coverage. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Hook as Tool Capture Hook
participant Queue as Queue Processor
participant AI as AI Services
participant DB as Databases
participant Search as Search Orchestrator
Note over User,Search: Observation Processing Pipeline
User->>Hook: Tool Output Event
Hook->>Queue: enqueue(toolOutput)
Queue->>DB: store pending message
Note over Queue,AI: Batch Processing (30s intervals)
Queue->>Queue: processBatch()
Queue->>AI: compress(toolOutput)
AI-->>Queue: parsed observation
alt Conflict Resolution Enabled
Queue->>AI: generate embedding
AI-->>Queue: embedding vector
Queue->>DB: findSimilar(embedding)
DB-->>Queue: similar observations
alt Gray Zone Detected
Queue->>AI: ConflictEvaluator.evaluate()
AI-->>Queue: duplicate/update/new_fact
alt Outcome: update
Queue->>DB: create observation
DB-->>Queue: created
Queue->>DB: supersede(oldId, newId)
end
alt Outcome: duplicate
Queue->>Queue: skipObservation = true
end
end
end
alt Not Skipped
Queue->>DB: create observation
DB-->>Queue: created observation
alt Entity Extraction Enabled
Queue->>AI: EntityExtractor.extract()
AI-->>Queue: entities & relations
Queue->>DB: upsertEntity(), createRelation()
end
end
Note over User,Search: Search & Retrieval
User->>Search: search(query)
alt Hybrid Search Strategy
Search->>DB: FTS5 search
DB-->>Search: text results
Search->>AI: generate query embedding
AI-->>Search: embedding
Search->>DB: vector search
DB-->>Search: semantic results
Search->>Search: merge results
end
alt Graph Augmentation Enabled
Search->>DB: findByName(entities)
DB-->>Search: entity nodes
Search->>DB: traverseRelations(depth=1)
DB-->>Search: related entity IDs
Search->>DB: getObservationsForEntity()
DB-->>Search: related observations
Search->>Search: merge graph results
end
alt User Memory Enabled
Search->>DB: UserMemoryDB.search()
DB-->>Search: user observations
Search->>Search: deduplicate & merge
end
alt Reranking Enabled
Search->>AI: LLMReranker.rerank()
AI-->>Search: relevance-ordered indices
end
Search-->>User: final ranked results
|
| if (conflictSupersedesId) { | ||
| try { | ||
| this.observationRepo.supersede(conflictSupersedesId, created.id); | ||
| console.log( | ||
| `[open-mem] Superseded observation ${conflictSupersedesId} with ${created.id}`, | ||
| ); | ||
| } catch { | ||
| // Supersede failure must not block observation creation | ||
| } | ||
| } |
There was a problem hiding this comment.
Supersede operation happens after observation is created. If supersede() fails silently (line 290), the new observation remains but the old one isn't marked as superseded, leaving duplicate active observations.
| if (conflictSupersedesId) { | |
| try { | |
| this.observationRepo.supersede(conflictSupersedesId, created.id); | |
| console.log( | |
| `[open-mem] Superseded observation ${conflictSupersedesId} with ${created.id}`, | |
| ); | |
| } catch { | |
| // Supersede failure must not block observation creation | |
| } | |
| } | |
| if (conflictSupersedesId) { | |
| try { | |
| this.observationRepo.supersede(conflictSupersedesId, created.id); | |
| console.log( | |
| `[open-mem] Superseded observation ${conflictSupersedesId} with ${created.id}`, | |
| ); | |
| } catch (error) { | |
| // Log and consider rollback or marking the new observation | |
| console.error(`[open-mem] Failed to supersede ${conflictSupersedesId}:`, error); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/queue/processor.ts
Line: 284:293
Comment:
Supersede operation happens after observation is created. If `supersede()` fails silently (line 290), the new observation remains but the old one isn't marked as superseded, leaving duplicate active observations.
```suggestion
if (conflictSupersedesId) {
try {
this.observationRepo.supersede(conflictSupersedesId, created.id);
console.log(
`[open-mem] Superseded observation ${conflictSupersedesId} with ${created.id}`,
);
} catch (error) {
// Log and consider rollback or marking the new observation
console.error(`[open-mem] Failed to supersede ${conflictSupersedesId}:`, error);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| const maxDepth = Math.min(depth, 2); // Cap at 2 to prevent explosion | ||
| const visited = new Set<string>(); |
There was a problem hiding this comment.
BFS traversal caps depth at 2, but this cap happens during traversal. With a dense graph, the queue could still grow large before hitting the cap. Consider adding a max visited nodes limit (e.g., 100) to prevent memory issues in pathological cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/db/entities.ts
Line: 183:184
Comment:
BFS traversal caps depth at 2, but this cap happens during traversal. With a dense graph, the queue could still grow large before hitting the cap. Consider adding a max visited nodes limit (e.g., 100) to prevent memory issues in pathological cases.
How can I resolve this? If you propose a fix, please make it concise.| const seenIds = new Set(projectResults.map((r) => r.observation.id)); | ||
| const dedupedUserResults = userResults.filter((r) => !seenIds.has(r.observation.id)); | ||
| return [...projectResults, ...dedupedUserResults].slice(0, limit); |
There was a problem hiding this comment.
User memory results are merged after project results without considering similarity-based deduplication. Two observations with identical content but different IDs (one from project, one from user DB) could both appear in results. Consider computing similarity for cross-database deduplication.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/search/orchestrator.ts
Line: 282:284
Comment:
User memory results are merged after project results without considering similarity-based deduplication. Two observations with identical content but different IDs (one from project, one from user DB) could both appear in results. Consider computing similarity for cross-database deduplication.
How can I resolve this? If you propose a fix, please make it concise.| function resolveUserDbPath(dbPath: string): string { | ||
| if (dbPath.startsWith("~/")) { | ||
| const home = process.env.HOME || process.env.USERPROFILE || ""; | ||
| return `${home}${dbPath.slice(1)}`; | ||
| } | ||
| return dbPath; | ||
| } |
There was a problem hiding this comment.
User DB path resolution uses HOME or USERPROFILE but doesn't validate the path exists or is writable. If the directory doesn't exist (e.g., ~/.config/open-mem/), database creation will fail.
Add directory creation in constructor:
| function resolveUserDbPath(dbPath: string): string { | |
| if (dbPath.startsWith("~/")) { | |
| const home = process.env.HOME || process.env.USERPROFILE || ""; | |
| return `${home}${dbPath.slice(1)}`; | |
| } | |
| return dbPath; | |
| } | |
| function resolveUserDbPath(dbPath: string): string { | |
| if (dbPath.startsWith("~/")) { | |
| const home = process.env.HOME || process.env.USERPROFILE || ""; | |
| const resolved = `${home}${dbPath.slice(1)}`; | |
| const dir = resolved.substring(0, resolved.lastIndexOf("/")); | |
| require("node:fs").mkdirSync(dir, { recursive: true }); | |
| return resolved; | |
| } | |
| return dbPath; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/db/user-memory.ts
Line: 307:313
Comment:
User DB path resolution uses `HOME` or `USERPROFILE` but doesn't validate the path exists or is writable. If the directory doesn't exist (e.g., `~/.config/open-mem/`), database creation will fail.
Add directory creation in constructor:
```suggestion
function resolveUserDbPath(dbPath: string): string {
if (dbPath.startsWith("~/")) {
const home = process.env.HOME || process.env.USERPROFILE || "";
const resolved = `${home}${dbPath.slice(1)}`;
const dir = resolved.substring(0, resolved.lastIndexOf("/"));
require("node:fs").mkdirSync(dir, { recursive: true });
return resolved;
}
return dbPath;
}
```
How can I resolve this? If you propose a fix, please make it concise.| Only extract entities that are clearly mentioned. Do not infer. | ||
| Respond with EXACTLY this XML format: |
There was a problem hiding this comment.
Instruction says Only extract entities that are clearly mentioned. Do not infer. This conservative approach may miss implicit relationships present in code. For example, "React hooks" doesn't explicitly state "React uses hooks" but the relationship is semantically clear. Consider relaxing this constraint for better knowledge graph coverage.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ai/prompts.ts
Line: 226:227
Comment:
Instruction says `Only extract entities that are clearly mentioned. Do not infer.` This conservative approach may miss implicit relationships present in code. For example, "React hooks" doesn't explicitly state "React uses hooks" but the relationship is semantically clear. Consider relaxing this constraint for better knowledge graph coverage.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/db/observations.ts (1)
265-301:⚠️ Potential issue | 🟠 Major
searchByConceptandsearchByFiledo not filter out superseded observations — inconsistent withsearch().The main
search()method (line 204),getIndex()(line 176),getWithEmbeddings()(line 323), andfindSimilar()(line 354) all exclude superseded records withAND o.superseded_by IS NULL, but these two methods don't. Superseded observations will appear in concept and file-based searches.🐛 Proposed fix
searchByConcept(concept: string, limit = 10, projectPath?: string): Observation[] { const hasProjectPath = !!projectPath; const sql = `SELECT o.* FROM observations o JOIN observations_fts fts ON o._rowid = fts.rowid ${hasProjectPath ? "JOIN sessions s ON o.session_id = s.id" : ""} - WHERE observations_fts MATCH ? + WHERE observations_fts MATCH ? AND o.superseded_by IS NULL ${hasProjectPath ? "AND s.project_path = ?" : ""} ORDER BY rank LIMIT ?`;searchByFile(filePath: string, limit = 10, projectPath?: string): Observation[] { const hasProjectPath = !!projectPath; const sql = `SELECT o.* FROM observations o JOIN observations_fts fts ON o._rowid = fts.rowid ${hasProjectPath ? "JOIN sessions s ON o.session_id = s.id" : ""} - WHERE observations_fts MATCH ? + WHERE observations_fts MATCH ? AND o.superseded_by IS NULL ${hasProjectPath ? "AND s.project_path = ?" : ""} ORDER BY rank LIMIT ?`;tests/context/injection.test.ts (1)
36-46:⚠️ Potential issue | 🟡 Minor
makeIndexEntryis missing requireddiscoveryTokensandimportancefields.The
ObservationIndexinterface requires all 8 fields, but this helper only provides 6 defaults. The issue goes undetected because test files are excluded from TypeScript compilation (seetsconfig.json), but the function violates the interface contract. Add defaults for both fields or update the function to accept them via overrides.Additionally,
makeConfigandmakeMockReposhelpers are duplicated across two describe blocks (lines ~251–260 and ~395–412). Extract them to module scope to avoid maintenance burden.src/index.ts (1)
266-276:⚠️ Potential issue | 🟡 Minor
process.on("beforeExit", cleanup)won't fire ifprocess.exit()is called elsewhere.This is a well-known Node.js gotcha —
beforeExitonly fires when the event loop drains naturally, not on explicitprocess.exit()calls. The MCP server'srl.on("close")handler callsprocess.exit(0), which would bypass this cleanup. If the MCP server and plugin entry point run in the same process, the user-memory DB and main DB may not be closed cleanly.Consider additionally registering on
exit(synchronous only) or restructuring the MCP server to not callprocess.exit().
🤖 Fix all issues with AI agents
In `@src/ai/parser.ts`:
- Around line 205-212: parseConflictEvaluationResponse can return a
ConflictEvaluation with outcome === "update" but no supersedesId when the
<supersedes> tag is missing; modify parseConflictEvaluationResponse so that when
outcome === "update" and the parsed supersedes value is missing/empty it returns
null instead of { outcome: "update", reason }, otherwise set result.supersedesId
= supersedes and return the result; reference symbols:
parseConflictEvaluationResponse, ConflictEvaluation, outcome, supersedes,
supersedesId.
In `@src/config.ts`:
- Around line 133-136: The config parse currently assigns Number.parseFloat(...)
directly to env.conflictSimilarityBandLow and env.conflictSimilarityBandHigh
which can become NaN for non-numeric env values; update the handling around the
parse calls (where OPEN_MEM_CONFLICT_BAND_LOW/HIGH are read) to parse into a
local variable, validate with Number.isNaN, and only assign to
env.conflictSimilarityBandLow and env.conflictSimilarityBandHigh if the parsed
value is a valid number (otherwise either leave the existing default or throw a
clear config error); reference the parse operation and the env fields
(OPEN_MEM_CONFLICT_BAND_LOW, OPEN_MEM_CONFLICT_BAND_HIGH,
env.conflictSimilarityBandLow, env.conflictSimilarityBandHigh) when making the
change.
In `@src/db/user-memory.ts`:
- Around line 307-312: The function resolveUserDbPath currently falls back to an
empty string when neither HOME nor USERPROFILE is set, causing "~/" to resolve
to a root path; update resolveUserDbPath to detect when home is missing and
throw a clear error (or return a controlled failure) instead of returning a path
prefixed with "", so when dbPath startsWith("~/") get the home from
process.env.HOME || process.env.USERPROFILE, and if that value is falsy throw a
descriptive Error (e.g., "Cannot resolve user home directory; HOME/USERPROFILE
not set") to prevent writing to an unintended location.
- Around line 208-235: The search method currently passes raw user input into
the FTS5 MATCH clause and can throw on malformed queries; wrap the DB call in a
try/catch inside the search(query) function (the block that calls
this.db.all<UserObservationSearchRow>(...), processes rows with mapRow, and
returns rank) and on any error return an empty array (optionally log the error)
so syntax errors in FTS5 queries are swallowed and handled consistently with
EntityRepository.findByName.
In `@src/search/hybrid.ts`:
- Around line 59-80: safelyRunFts is calling observations.search without
forwarding the required project scoping field; update the call inside
safelyRunFts (the observations.search invocation) to include projectPath:
options.projectPath so the SearchQuery passed to ObservationRepository.search is
project-scoped, preserving the existing try/catch and all other option mappings
(type, limit, importanceMin, importanceMax, createdAfter, createdBefore,
concepts, files).
In `@src/search/orchestrator.ts`:
- Around line 302-322: The functions passesAdvancedFilters (in
src/search/orchestrator.ts) and passesFilters (in src/search/hybrid.ts)
duplicate identical filter logic; extract the shared logic into a single utility
(e.g., create a new passesFilters in src/search/filters.ts or similar) that
accepts (obs: Observation, filters: ObservationFilterOptions) and move the
type/interface for the filter options there, then update both callers to import
and use the shared passesFilters (remove the local passesAdvancedFilters) so
both modules reference the same implementation and types.
In `@src/search/reranker.ts`:
- Around line 49-90: The rerank method currently truncates results into
candidates = results.slice(0, this.maxCandidates) which loses items beyond
maxCandidates; update rerank so after calling applyReranking(candidates,
indices, limit) you append the overflow items =
results.slice(this.maxCandidates) (or the tail needed to satisfy limit) to the
reranked list; ensure you only pass the candidate slice into
parseRerankingResponse/applyReranking but then merge the remaining original
results in their original order and respect the requested limit when returning
the final array.
In `@src/servers/mcp-server.ts`:
- Around line 150-153: The close handler for rl (rl.on("close", ...)) awaits
Promise.all(this.pendingOps) but if that promise rejects the async callback
becomes an unhandled rejection and process.exit(0) may never run; wrap the await
in a try/catch/finally (or append .catch and a .finally) so rejections from
Promise.all(this.pendingOps) are caught and logged (include context like
"pending ops on close"), and ensure process.exit(0) is always called in the
finally block to guarantee shutdown; refer to rl.on("close", ...) and
Promise.all(this.pendingOps) when making the change.
- Around line 508-519: The fallback branch is missing project scoping: when
this.searchOrchestrator is falsy the code calls this.observations.search({
query, type, limit }) but omits projectPath so results span all projects; update
the fallback call to include projectPath: this.projectPath (i.e.
this.observations.search({ query, type, limit, projectPath: this.projectPath }))
so both branches (searchOrchestrator.search and observations.search)
consistently scope by project; confirm SearchQuery handling remains compatible
with optional projectPath.
🧹 Nitpick comments (23)
src/db/schema.ts (1)
271-347: NoON DELETE CASCADEon entity graph FKs — orphan rows possible on observation deletion.
entity_relations.observation_idandentity_observations.observation_idreferenceobservations(id), but withoutON DELETE CASCADE. Deleting an observation (viadelete()ordeleteOlderThan()) will leave dangling rows in these tables. The same applies to entity deletions and the junction/relation tables.This is consistent with the rest of the schema (no cascades anywhere), but the entity graph tables are more heavily cross-referenced. Consider adding cleanup logic in the retention/delete paths, or adding cascades in a follow-up migration.
src/db/observations.ts (2)
536-542:supersede()doesn't verify that either observation ID exists.If called with invalid IDs, the UPDATE silently succeeds (affects 0 rows). Consider returning a boolean or checking the target observation exists, to surface bugs in the conflict resolution pipeline early.
16-22: LIKE-based filtering on JSON-encoded arrays may produce false positives.The
conceptscolumn stores data like["error-handling","testing"]. A LIKE%test%search would match both"testing"and any concept containing "test" as a substring. Same applies tofiles. This is acceptable for now but worth documenting as a known limitation.Also applies to: 236-252
src/context/builder.ts (1)
228-258: Duplicated budget-selection logic betweenbuildUserContextSectionandbuildUserCompactContext.Lines 234–242 and 269–277 are identical. Consider extracting a small helper:
♻️ Proposed refactor
+function selectByBudget(entries: ObservationIndex[], maxTokens: number): ObservationIndex[] { + let budget = maxTokens; + const included: ObservationIndex[] = []; + for (const entry of entries) { + const tokens = entry.tokenCount || estimateTokens(entry.title); + if (budget - tokens < 0) break; + included.push(entry); + budget -= tokens; + } + return included; +} + export function buildUserContextSection( userIndex: ObservationIndex[], maxTokens: number, ): string { if (userIndex.length === 0) return ""; - - let budget = maxTokens; - const included: ObservationIndex[] = []; - - for (const entry of userIndex) { - const tokens = entry.tokenCount || estimateTokens(entry.title); - if (budget - tokens < 0) break; - included.push(entry); - budget -= tokens; - } - + const included = selectByBudget(userIndex, maxTokens); if (included.length === 0) return ""; // ... rest unchangedAlso applies to: 263-288
src/ai/prompts.ts (1)
184-190: Duplicate/orphaned "Reranking Prompt" section header.Lines 188–189 contain a "Reranking Prompt" section comment with no associated code, then the actual
buildRerankingPromptfunction appears at line 245 under a second identical header. Remove the stale one.🧹 Proposed cleanup
-// ----------------------------------------------------------------------------- -// Reranking Prompt -// ----------------------------------------------------------------------------- - // ----------------------------------------------------------------------------- // Entity Extraction Prompt // -----------------------------------------------------------------------------src/ai/entity-extractor.ts (1)
94-112:isRetryableandsleepare duplicated verbatim inconflict-evaluator.ts.Both this file and
conflict-evaluator.tscontain identicalisRetryableandsleepimplementations (and share the same constructor/retry pattern). Consider extracting these into a shared utility (e.g.,src/ai/retry.ts) to reduce duplication and ensure consistent retry behaviour if the logic evolves.tests/ai/entity-extractor.test.ts (1)
15-20:withMockGeneratehelper duplicated across test files.This helper is identical in
tests/ai/conflict-evaluator.test.ts. Consider extracting it to a shared test utility if more AI component tests follow the same pattern.src/ai/conflict-evaluator.ts (1)
17-22:ConflictEvaluatorConfigis identical toEntityExtractorConfig.Both config interfaces have the exact same fields (
provider,apiKey,model,rateLimitingEnabled). Consider defining a sharedAiComponentConfig(or similar) base type to reduce drift if new fields are added to one but not the other.tests/context/injection.test.ts (1)
384-412:makeConfigandmakeMockReposare duplicated within this file.These helpers (lines 385–412) are identical to those defined earlier (lines 159–186). Consider hoisting them to the top-level scope of the test file to avoid duplication.
src/tools/search.ts (1)
17-22: Consider validatingimportance_min ≤ importance_maxand ISO 8601 date format.
importance_minandimportance_maxcan currently be set such thatmin > max, which would silently return no results. Similarly,afterandbeforeaccept arbitrary strings with no date format validation, which could produce confusing errors downstream.💡 Proposed validation refinements
-const searchArgsSchema = z.object({ - query: z.string().describe("Search query (supports keywords, phrases, file paths)"), - type: z - .enum(["decision", "bugfix", "feature", "refactor", "discovery", "change"]) - .optional() - .describe("Filter by observation type"), - limit: z.number().min(1).max(50).default(10).describe("Maximum number of results"), - importance_min: z.number().min(1).max(5).optional().describe("Minimum importance (1-5)"), - importance_max: z.number().min(1).max(5).optional().describe("Maximum importance (1-5)"), - after: z.string().optional().describe("Only observations after this date (ISO 8601)"), - before: z.string().optional().describe("Only observations before this date (ISO 8601)"), - concepts: z.array(z.string()).optional().describe("Filter by concepts"), - files: z.array(z.string()).optional().describe("Filter by file paths"), -}); +const searchArgsSchema = z.object({ + query: z.string().describe("Search query (supports keywords, phrases, file paths)"), + type: z + .enum(["decision", "bugfix", "feature", "refactor", "discovery", "change"]) + .optional() + .describe("Filter by observation type"), + limit: z.number().min(1).max(50).default(10).describe("Maximum number of results"), + importance_min: z.number().min(1).max(5).optional().describe("Minimum importance (1-5)"), + importance_max: z.number().min(1).max(5).optional().describe("Maximum importance (1-5)"), + after: z.string().datetime({ offset: true }).optional().describe("Only observations after this date (ISO 8601)"), + before: z.string().datetime({ offset: true }).optional().describe("Only observations before this date (ISO 8601)"), + concepts: z.array(z.string()).optional().describe("Filter by concepts"), + files: z.array(z.string()).optional().describe("Filter by file paths"), +}).refine( + (d) => d.importance_min == null || d.importance_max == null || d.importance_min <= d.importance_max, + { message: "importance_min must be ≤ importance_max" }, +);tests/search/reranking.test.ts (2)
165-176: Dead code:dummyModelis unused.Lines 167–169 define a
dummyModeltype alias that is never referenced. This can be removed for clarity.🧹 Proposed cleanup
function makeLLMReranker() { - // Use a dummy language model — we override _generate anyway - const dummyModel = {} as Parameters<typeof LLMReranker.prototype.rerank>[0] extends string - ? never - : never; return new LLMReranker({} as any, { rerankingMaxCandidates: 20, provider: "anthropic", model: "test-model", rateLimitingEnabled: false, }); }
384-411: Integration test asserts result count but not the expected reranked order.The mock reverses results and the comment on Line 408 says "the order should be flipped," but the assertion on Lines 409–410 only checks
ids.length. This makes the test pass even if the reranker wasn't actually applied. Consider asserting the expected ordering.💡 Proposed stronger assertion
// The mock reverses, so the order should be flipped from FTS5 default const ids = results.map((r) => r.observation.title); - expect(ids.length).toBe(2); + expect(ids).toEqual(["Authentication JWT tokens", "Database connection pooling"]);src/search/graph.ts (2)
21-32: Nested entity lookups may become a hot path for verbose queries.For a query with W words, this performs up to
(W + W-1)FTS lookups (single words + bigrams), then for each matched entity runs BFS traversal + observation fetches. Short common words (2–3 chars) passing the>= 2filter can cause noisy FTS matches, amplifying the loop.Consider deduplicating observation IDs earlier, capping
entityNamescount, or filtering out common stop-words to keep this bounded.
57-69: Simplify single-word push loop.🧹 Minor simplification
function extractEntityCandidates(query: string): string[] { const words = query.split(/\s+/).filter((w) => w.length >= 2); const candidates: string[] = []; - - for (const word of words) { - candidates.push(word); - } + candidates.push(...words); for (let i = 0; i < words.length - 1; i++) { candidates.push(`${words[i]} ${words[i + 1]}`); } return candidates; }tests/tools/user-memory-tools.test.ts (1)
47-55: ReusecleanupTestDbfor user DB cleanup.The manual cleanup loop on lines 51-55 duplicates the logic already in
cleanupTestDb. You can simplify:♻️ Proposed simplification
afterEach(() => { db.close(); cleanupTestDb(dbPath); userMemoryDb.close(); - for (const suffix of ["", "-wal", "-shm"]) { - try { - unlinkSync(userDbPath + suffix); - } catch {} - } + cleanupTestDb(userDbPath); });tests/queue/conflict-resolution.test.ts (2)
55-69:createSequentialEmbeddingModelappears unused.This helper is defined but never called in the test file. Consider removing it to reduce dead code, or add it when tests that need sequential embeddings are implemented.
80-108: Mocking via internal_generateproperty is brittle.The mock helpers (
mockEvaluatorResponse,mockEvaluatorFailure,mockEvaluatorInvalidResponse,mockCompressorReturning) all override an internal_generatemethod via type assertion. If the internal implementation ofConflictEvaluatororObservationCompressorrenames or restructures this method, all these tests will silently break (pass vacuously or fail with confusing errors). Consider exposing a test-friendly injection point or using a proper mock/spy framework.src/mcp.ts (1)
57-79: Embeddings are gated oncompressionEnabledintentionally, but consider decoupling if users need vector search independently.The
embeddingModelis indeed used for vector search inSearchOrchestrator, but also for deduplication and conflict resolution inQueueProcessor. The coupling tocompressionEnabledappears intentional—when compression is enabled, observations are embedded for dedup/conflict resolution purposes, and those same embeddings enable vector search.However, your concern about flexibility is valid: users who want vector search but not compression (or vice versa) would need separate config flags. Currently, disabling compression disables embeddings entirely, which disables both vector search and conflict resolution.
The system does gracefully degrade—search falls back to FTS5 when
embeddingModelis null—but adding an independentembeddingsEnabledflag (orvectorSearchEnabled) would give users finer control over this feature cluster without forcing the coupling.src/queue/processor.ts (2)
52-64: Consider a configuration/dependency object to reduce constructor parameter count.The constructor now accepts 11 positional parameters. This makes call sites fragile — it's easy to swap nullable arguments. A single deps/options object would improve readability and prevent positional errors.
♻️ Example: group collaborators into a typed options object
+interface QueueProcessorDeps { + compressor: ObservationCompressor; + summarizer: SessionSummarizer; + pendingRepo: PendingMessageRepository; + observationRepo: ObservationRepository; + sessionRepo: SessionRepository; + summaryRepo: SummaryRepository; + embeddingModel?: EmbeddingModel | null; + conflictEvaluator?: ConflictEvaluator | null; + entityExtractor?: EntityExtractor | null; + entityRepo?: EntityRepository | null; +} + export class QueueProcessor { constructor( private config: QueueProcessorConfig, - private compressor: ObservationCompressor, - private summarizer: SessionSummarizer, - ... + private deps: QueueProcessorDeps, ) {}
125-247: Indentation mismatch with surrounding code inside the sametryblock.Lines 125–247 (the new dedup/conflict-resolution block) are indented one level less than the original code at lines 118–123 and 268–282, even though they all live inside the same
tryblock starting at line 117. This doesn't break functionality but reduces readability.src/search/reranker.ts (1)
192-219:isRetryableandsleepare duplicated across multiple AI modules.These same helpers exist in
src/ai/conflict-evaluator.tsandsrc/ai/entity-extractor.ts. Consider extracting them into a shared utility (e.g.,src/ai/retry-utils.ts) to reduce duplication.src/db/entities.ts (1)
114-123: Silent catch swallows all DB errors increateRelation.The bare
catchon line 121 returnsnullfor any error, not just constraint violations. A transient DB error (e.g., disk full, locked DB) would be silently ignored and treated as "relation already exists."Consider narrowing the catch to only expected constraint errors, or at minimum logging the error:
Proposed fix
try { this.db.run( `INSERT OR IGNORE INTO entity_relations (id, source_entity_id, target_entity_id, relationship, observation_id, created_at) VALUES (?, ?, ?, ?, ?, ?)`, [id, sourceEntityId, targetEntityId, relationship, observationId, now], ); - } catch { + } catch (err) { + console.warn(`[open-mem] Failed to create entity relation: ${err}`); return null; }src/index.ts (1)
158-158:entityRepois unconditionally passed toSearchOrchestrator, enabling graph-augmented search even when entity extraction is disabled.
EntityRepositoryis always created (line 158) and always passed toSearchOrchestrator(line 292). The orchestrator will then callgraphAugmentedSearchon every query (orchestrator.ts line 73). When entity extraction is disabled, the entity tables will be empty, so this results in unnecessary DB queries on every search.Consider guarding it:
Proposed fix
const searchOrchestrator = new SearchOrchestrator( observationRepo, embeddingModel, db.hasVectorExtension, reranker, userObservationRepo, - entityRepo, + config.entityExtractionEnabled ? entityRepo : null, );Also applies to: 286-293
…duplication, and docstrings - Improved error handling: supersede try/catch, FTS try/catch, close handler, NaN guards - Fixed project scoping: projectPath in FTS search and fallback search - Added BFS max visited nodes limit to prevent runaway graph traversal - Parser returns null for update without supersedes target - Reranker preserves overflow items instead of discarding them - Extracted shared filters utility (src/search/filters.ts) - Content-based cross-DB deduplication in user-memory - Relaxed entity extraction prompt for better recall - Added JSDoc docstrings across all new files Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/observations.ts (1)
266-302:⚠️ Potential issue | 🟠 Major
searchByConceptandsearchByFiledon't exclude superseded observations.The main
search(),getIndex(),getWithEmbeddings(), andfindSimilar()methods all filter withsuperseded_by IS NULL, but these two specialized search methods do not. This means superseded (stale/contradictory) observations can still surface through concept or file searches.Proposed fix
searchByConcept(concept: string, limit = 10, projectPath?: string): Observation[] { const hasProjectPath = !!projectPath; const sql = `SELECT o.* FROM observations o JOIN observations_fts fts ON o._rowid = fts.rowid ${hasProjectPath ? "JOIN sessions s ON o.session_id = s.id" : ""} WHERE observations_fts MATCH ? + AND o.superseded_by IS NULL ${hasProjectPath ? "AND s.project_path = ?" : ""} ORDER BY rank LIMIT ?`;Apply the same fix to
searchByFile.
🤖 Fix all issues with AI agents
In `@src/db/schema.ts`:
- Around line 294-320: The foreign keys for observation_id in the
entity_relations and entity_observations tables currently reference
observations(id) without cascade behavior, which causes delete failures; update
the CREATE TABLE statements for entity_relations (foreign key clause referencing
observation_id) and entity_observations (foreign key clause referencing
observation_id) to include ON DELETE CASCADE on the reference to
observations(id) so that when an observation is deleted its related rows in
entity_relations and entity_observations are automatically removed.
🧹 Nitpick comments (12)
src/context/builder.ts (1)
240-245:||treatstokenCount === 0as falsy — prefer??for the fallback.On lines 241 and 276,
entry.tokenCount || estimateTokens(entry.title)will fall through to the estimate iftokenCountis0. While a zero token count is unlikely in practice, using??(nullish coalescing) is more semantically correct — it only falls back fornull/undefined.Proposed fix
- const tokens = entry.tokenCount || estimateTokens(entry.title); + const tokens = entry.tokenCount ?? estimateTokens(entry.title);Apply the same change on line 276.
src/db/observations.ts (1)
537-544:supersededoesn't verify that either observation ID exists.If called with a non-existent
observationId, the UPDATE silently affects zero rows. Consider returning a boolean or checkingchangesto let callers know if the operation succeeded — especially since a no-op supersede could mask bugs in conflict resolution logic.src/db/user-memory.ts (1)
321-334: Use a top-levelimportinstead of inlinerequire("node:fs").The file uses ESM
importsyntax throughout but switches torequireon line 331. This works in Bun but is inconsistent and non-standard in an ESM module. Past review feedback on the HOME guard has been addressed correctly.Proposed fix
Add to the top-level imports:
import { mkdirSync } from "node:fs";Then update the function:
function resolveUserDbPath(dbPath: string): string { if (dbPath.startsWith("~/")) { const home = process.env.HOME || process.env.USERPROFILE || ""; if (!home) { throw new Error( "Cannot resolve user DB path: HOME environment variable is not set", ); } const resolved = `${home}${dbPath.slice(1)}`; const dir = resolved.substring(0, resolved.lastIndexOf("/")); - require("node:fs").mkdirSync(dir, { recursive: true }); + mkdirSync(dir, { recursive: true }); return resolved; } return dbPath; }src/search/hybrid.ts (1)
103-143:passesFiltersdoes not enforce project scoping in native vector search.
runNativeVectorSearchretrieves candidates viagetVecEmbeddingMatcheswhich is not project-scoped, andpassesFilters(fromfilters.ts) does not checkprojectPath. This means native vector results could include observations from other projects — unlike the FTS path (project-scoped) and the JS fallback path (which usesgetWithEmbeddings(projectPath, ...)).This is pre-existing behavior, but now that
optionsis threaded through, it would be straightforward to add a project-path check topassesFiltersor filter inline here for consistency.src/ai/prompts.ts (2)
186-192: Duplicate/misplaced section header — "Reranking Prompt" appears twice.Lines 187–188 contain a "Reranking Prompt" section header, but this block is between the conflict evaluation and entity extraction sections. The actual reranking prompt and its correct header are at Lines 244–246. This first occurrence appears to be a leftover.
Proposed fix
-// ----------------------------------------------------------------------------- -// Reranking Prompt -// ----------------------------------------------------------------------------- - // ----------------------------------------------------------------------------- // Entity Extraction Prompt // -----------------------------------------------------------------------------
209-242: Potential XML injection from user-controlled observation data.Fields like
obs.title,obs.narrative,obs.facts, and file paths are interpolated directly into the XML prompt. If any contain XML-like strings (e.g.,</facts>in a fact), it could confuse the LLM's output parsing. This is low-severity since the LLM response (not the prompt) is what gets parsed, but escaping special characters (<,>,&) would make the prompts more robust.src/ai/entity-extractor.ts (1)
105-123:isRetryableandsleepare duplicated betweenentity-extractor.tsandconflict-evaluator.ts.Both files contain identical
isRetryable()andsleep()implementations. Consider extracting these into a shared utility (e.g.,src/ai/retry.ts) to keep them in sync.src/search/orchestrator.ts (1)
265-283: Advanced filters are not forwarded to user memory search.The
searchUserMemorymethod only passesqueryandlimittouserObservationRepo.search(). Filters likeimportanceMin,createdAfter,concepts, etc. fromoptionsare silently ignored for user memory results. If this is intentional (simpler user-memory interface), consider documenting it. If not, theUserObservationRepository.searchAPI would need extending.src/queue/processor.ts (2)
125-247: Conflict resolution logic is sound but deeply nested.The gray-zone approach with
bandLow/bandHighthresholds and LLM evaluation is well-designed. Fast-path skip for exact duplicates abovebandHigh, LLM evaluation for the gray zone, and graceful fallback on evaluator failure are all correct. The non-null assertion onthis.conflictEvaluator!(Line 187) is safe given theconflictEnabledguard.However, this block reaches ~6 levels of nesting (for → try → if → if → if → try). Consider extracting the conflict resolution logic into a private method (e.g.,
evaluateConflicts(...)) that returns{ skip: boolean; supersedesId: string | null }to reduce cognitive complexity.
284-294: Inconsistent indentation in the supersede try/catch block.The
} catch (error) {at Line 290 appears to be at a different indentation level than its correspondingtry {at Line 285. While not a functional issue (JS ignores whitespace), it harms readability in an already deeply nested context.src/ai/conflict-evaluator.ts (1)
96-113:isRetryableandsleepare duplicated acrossconflict-evaluator.tsandreranker.ts.Both files define identical
isRetryableandsleephelper functions. Consider extracting them into a shared utility (e.g.,src/ai/retry-utils.ts) to reduce duplication.src/ai/parser.ts (1)
228-237:EntityTypeis independently defined in bothparser.tsandentities.ts.The
EntityTypeunion is duplicated here (line 229) and insrc/db/entities.ts(line 13). If one is updated without the other, they'll silently drift. Consider having one canonical definition and importing it in the other module.For example,
entities.tscould import fromparser.ts(or both from a shared types file):-export type EntityType = - | "technology" - | "library" - | "pattern" - | "concept" - | "file" - | "person" - | "project" - | "other"; +export type { EntityType } from "../ai/parser";
|
@greptile review |
Add per-folder promise-based lock to serialize concurrent writes and clean up temp files on failure. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/update.ts (1)
6-16:⚠️ Potential issue | 🟡 MinorTitle description says "max 80 chars" but no
.max(80)constraint is enforced.Line 8 describes
"Updated title (max 80 chars)"but the schema only specifiesz.string().optional(). If this is a real constraint, add.max(80). If it's just guidance for the LLM agent, it's fine as-is, but the mismatch could confuse future developers.🔧 Proposed fix (if the constraint is real)
- title: z.string().optional().describe("Updated title (max 80 chars)"), + title: z.string().max(80).optional().describe("Updated title (max 80 chars)"),src/context/progressive.ts (1)
38-70:⚠️ Potential issue | 🟡 Minor
fullObservationsbypass the token budget andtotalTokensunderreports.While
fullObservationsare count-limited (viaconfig.contextFullObservationCount, default 3) before being passed tobuildProgressiveContext, they are not deducted frombudget. This means:
totalTokensonly reflects summaries + filtered index entries, omitting the token cost offullObservations.- If the token budget is tight, some of the pre-selected full observations may not appear in the returned
observationIndex(which is budget-filtered), creating a mismatch.If this separation is intentional, document that the caller must account for
fullObservationstokens separately. Otherwise, their token costs should be deducted frombudgetand included intotalTokens.
🤖 Fix all issues with AI agents
In `@src/db/observations.ts`:
- Around line 243-259: The current LIKE `%...%` on o.concepts / o.files_read /
o.files_modified (built in the query generation using query.concepts,
query.files and escapeLike) can produce substring false-positives (e.g. "test"
matches "testing"); change the matching to require JSON-array element boundaries
by searching for the quoted element (escape the value with escapeLike and wrap
with '"…"' in the pattern) or, preferably, use native JSON operators if
supported (e.g. JSON_CONTAINS / json_each) instead of simple LIKE. Concretely,
update the clauses built in the block that maps query.concepts and query.files
so the pushed params are like `%\"${escapedValue}\"%` (or replace the whole LIKE
logic with JSON_CONTAINS(o.concepts, json_quote(value)) / equivalent) while
still using escapeLike for safe LIKE escaping; keep references to escapeLike,
query.concepts, query.files, o.concepts, o.files_read and o.files_modified when
making the change.
In `@src/db/schema.ts`:
- Around line 261-271: The migration "add-conflict-resolution-columns" adds
observations.superseded_by without a foreign-key; decide whether to prevent
dangling references and, if so, modify the migration to add a FK constraint on
superseded_by referencing observations(id) with ON DELETE SET NULL (or
alternatively add a cleanup trigger) so that when the superseding observation is
deleted the superseded_by value is cleared; update the migration in the version
8 block (name: "add-conflict-resolution-columns") to implement the chosen
behavior and ensure any index/DDL ordering respects the FK change.
🧹 Nitpick comments (10)
src/ai/provider.ts (2)
12-12:| stringrenders the string-literal union ineffective for type checking.The union
"anthropic" | "bedrock" | "openai" | "google" | stringcollapses tostringin TypeScript, so the compiler won't flag typos like"opneai". If the intent is to allow arbitrary providers while still offering autocomplete, use thestring & {}idiom:♻️ Suggested change
-export type ProviderType = "anthropic" | "bedrock" | "openai" | "google" | string; +export type ProviderType = "anthropic" | "bedrock" | "openai" | "google" | (string & {});
85-111:createEmbeddingModelignoresconfig.modeland hardcodes embedding model names.The caller passes a
ModelConfigthat includes amodelfield, but this function never uses it — it always selects a hardcoded embedding model per provider (e.g.,"text-embedding-004"for Google,"text-embedding-3-small"for OpenAI). This is surprising becausecreateModeldoes respectconfig.model.If this is intentional (embedding models are provider-fixed), consider either:
- Adding an
embeddingModelfield toModelConfig(or a separate config type) so callers can override it, or- Documenting clearly on the function that
config.modelis unused and the embedding model is provider-determined.At minimum, the current signature is misleading — callers might expect
config.modelto control which embedding model is created.src/daemon/worker.ts (1)
43-56: Minor: errors swallowed silently in the polling loop.The
catchblock on line 51 discards all errors. While the comment explains the intent (polling errors must not crash the loop), consider logging atdebuglevel so operational issues are diagnosable. Same applies to the.catch(() => {})inhandleMessage(line 89).src/servers/http-server.ts (1)
75-86:redactConfigmay over-redact non-secret fields likeapiVersion.The heuristic
lowerKey.includes("api")will redact any string field whose key contains "api" — including fields likeapiVersionorapiEndpointthat aren't secrets. This is a conservative default (safe), but could confuse dashboard users seeing***REDACTED***for non-sensitive config. Consider matching more precisely (e.g.,lowerKey.includes("apikey") || lowerKey === "apikey" || lowerKey.includes("secret") || lowerKey.includes("token")).src/queue/processor.ts (3)
286-296: Inconsistent indentation in the supersede try/catch block.The
} catchat Line 292 is indented at a different level than thetryat Line 287, making the block structure hard to follow. Not a runtime issue, but it hurts readability in an already deeply nested method.🔧 Proposed fix
if (conflictSupersedesId) { try { this.observationRepo.supersede(conflictSupersedesId, created.id); console.log( `[open-mem] Superseded observation ${conflictSupersedesId} with ${created.id}`, ); - } catch (error) { - // Supersede failure must not block observation creation - console.error(`[open-mem] Failed to supersede ${conflictSupersedesId}:`, error); - } + } catch (error) { + // Supersede failure must not block observation creation + console.error(`[open-mem] Failed to supersede ${conflictSupersedesId}:`, error); + } }
127-330: Consider extracting the dedup/conflict and entity extraction logic into private methods.
processBatchis now ~220 lines with up to 7 levels of nesting in the conflict resolution path. Extracting these into dedicated private methods (e.g.,resolveConflicts(observation, dedupEmbedding)andextractEntities(created)) would significantly improve readability and testability without changing behavior.
54-66: Constructor has grown to 11 parameters.Consider grouping the AI-related dependencies into a single object (e.g.,
aiDeps: { conflictEvaluator, entityExtractor, entityRepo }) to keep the constructor manageable as new features are added.src/db/observations.ts (1)
555-562:supersededoesn't verify thatobservationIdornewObservationIdexist.If called with invalid IDs, the UPDATE silently affects zero rows. Consider returning a boolean or checking
changescount so the caller knows whether the supersession actually took place, rather than silently succeeding.🔧 Proposed fix
- supersede(observationId: string, newObservationId: string): void { + supersede(observationId: string, newObservationId: string): boolean { const now = new Date().toISOString(); this.db.run( "UPDATE observations SET superseded_by = ?, superseded_at = ? WHERE id = ?", [newObservationId, now, observationId], ); + const row = this.db.get<{ id: string }>( + "SELECT id FROM observations WHERE id = ? AND superseded_by = ?", + [observationId, newObservationId], + ); + return !!row; }src/index.ts (2)
143-176: Conditional AI services follow a consistent pattern — minor note onentityRepo.
ConflictEvaluatorandEntityExtractorare correctly gated behind their respective config flags + API key availability, matching theembeddingModelpattern above.
entityRepo(line 162) is instantiated unconditionally even whenentityExtractionEnabledisfalse. Since it's a lightweight DB wrapper this is harmless, but for consistency with the other conditional components you could guard it or lazily create it. Not blocking.
349-357: Consider whether new types should be part of the public API.The re-export surface hasn't been expanded despite adding significant new subsystems (entities, user memory, conflict evaluation). If consumers need to interact with these features programmatically (e.g., configuring
entityExtractionEnabledoruserMemoryEnabled), the corresponding config types or at least the feature flag names may need to be exported.
| if (query.concepts && query.concepts.length > 0) { | ||
| const conceptClauses = query.concepts.map(() => "o.concepts LIKE ? ESCAPE '\\'"); | ||
| sql += ` AND (${conceptClauses.join(" OR ")})`; | ||
| for (const c of query.concepts) { | ||
| params.push(`%${escapeLike(c)}%`); | ||
| } | ||
| } | ||
| if (query.files && query.files.length > 0) { | ||
| const fileClauses = query.files.map( | ||
| () => "(o.files_read LIKE ? ESCAPE '\\' OR o.files_modified LIKE ? ESCAPE '\\')", | ||
| ); | ||
| sql += ` AND (${fileClauses.join(" OR ")})`; | ||
| for (const f of query.files) { | ||
| const escaped = `%${escapeLike(f)}%`; | ||
| params.push(escaped, escaped); | ||
| } | ||
| } |
There was a problem hiding this comment.
Concept/file LIKE filters may produce false-positive matches.
The LIKE %concept% pattern matches substrings within the JSON array (e.g., searching for "test" matches "testing"). This is a known trade-off for simplicity vs. JSON parsing, but worth noting for users who might expect exact-match semantics on the concepts and files filter arrays.
🤖 Prompt for AI Agents
In `@src/db/observations.ts` around lines 243 - 259, The current LIKE `%...%` on
o.concepts / o.files_read / o.files_modified (built in the query generation
using query.concepts, query.files and escapeLike) can produce substring
false-positives (e.g. "test" matches "testing"); change the matching to require
JSON-array element boundaries by searching for the quoted element (escape the
value with escapeLike and wrap with '"…"' in the pattern) or, preferably, use
native JSON operators if supported (e.g. JSON_CONTAINS / json_each) instead of
simple LIKE. Concretely, update the clauses built in the block that maps
query.concepts and query.files so the pushed params are like
`%\"${escapedValue}\"%` (or replace the whole LIKE logic with
JSON_CONTAINS(o.concepts, json_quote(value)) / equivalent) while still using
escapeLike for safe LIKE escaping; keep references to escapeLike,
query.concepts, query.files, o.concepts, o.files_read and o.files_modified when
making the change.
…eanup trigger Replace LIKE substring matching with json_each() for exact concept element matching and per-element file path matching. Add trg_clear_superseded_by trigger to v8 migration to prevent dangling superseded_by references on observation deletion. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
Implements 18 tasks across 4 tracks from the mem0-feature-parity plan, bringing open-mem to feature parity with Mem0's advanced memory capabilities. All features are opt-in via config flags (defaulting to
false).~/.config/open-mem/with merged search results and separate token budgets for context injectionChanges
New Modules
src/ai/conflict-evaluator.tssrc/ai/entity-extractor.tssrc/db/user-memory.tssrc/db/entities.tssrc/search/reranker.tssrc/search/graph.tsModified Modules
src/db/schema.tssrc/db/observations.tssrc/queue/processor.tssrc/search/orchestrator.tssrc/search/hybrid.tssrc/tools/save.ts,search.ts,recall.tssrc/context/builder.tssrc/hooks/context-inject.ts,compaction.tssrc/config.tssrc/types.tssrc/index.ts,src/mcp.ts,src/daemon.tsNew Test Suites (14 files, +201 tests)
tests/ai/conflict-evaluator.test.ts— Conflict evaluation scenariostests/ai/entity-extractor.test.ts— Entity/relationship extractiontests/db/user-memory.test.ts— User-level DB operationstests/db/entities.test.ts— Entity repository CRUD + traversaltests/db/migration-v8.test.ts— Schema migration v8tests/db/migration-v9.test.ts— Schema migration v9tests/search/reranking.test.ts— Reranker pipelinetests/search/graph-search.test.ts— Graph-augmented searchtests/tools/advanced-filters.test.ts— Filter operatorstests/tools/user-memory-tools.test.ts— User memory tool integrationtests/context/hierarchy.test.ts— Hierarchical context injectiontests/queue/conflict-resolution.test.ts— End-to-end conflict resolutionConfiguration
All new features are opt-in and default to
false:Verification
bun x tsc --noEmit)Summary by CodeRabbit
New Features
Behavioral Changes
Tests
Documentation