Repository navigation
feat(sdk): Integrate mem0 for better context - #154
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a pluggable memory provider abstraction and integrates a new Mem0-backed provider. Updates Neurolink to use MemoryProvider via a factory, adjusts utils to pass userId/currentInput, extends types, adds dependencies, tests, and documentation. Also updates .gitignore. No breaking changes to existing APIs beyond added fields/methods. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant NL as NeuroLink
participant PF as MemoryProviderFactory
participant MP as MemoryProvider
participant AI as LLM Provider
participant VS as Mem0 Vector Store
User->>NL: generate(text, context{sessionId,userId})
NL->>PF: createWithValidation(memoryConfig)
PF-->>NL: MemoryProvider (in-memory or Mem0)
NL->>MP: initialize()
alt Mem0 provider
NL->>MP: buildContextMessages(sessionId, userId, currentInput)
MP->>VS: search(currentInput, userId)
alt search ok
VS-->>MP: relevant memories
MP-->>NL: context messages (memories + session)
else search fails/timeout
MP-->>NL: session-only context
end
else In-memory provider
MP-->>NL: session-only context
end
NL->>AI: generate(prompt + context)
AI-->>NL: response
NL->>MP: storeConversationTurn(sessionId, userId, input, response)
MP-->>NL: ack (non-throwing on error)
NL-->>User: response
sequenceDiagram
autonumber
participant App as App Code
participant PF as MemoryProviderFactory
participant IM as InMemoryManager
participant MM as Mem0MemoryManager
App->>PF: createWithValidation({provider:"mem0", mem0:{...}})
alt mem0 config valid and mem0ai available
PF-->>App: Mem0MemoryManager
else invalid/mem0ai missing
PF-->>App: error or fallback guidance
end
App->>PF: createWithValidation({provider:"in-memory"})
PF-->>App: ConversationMemoryManager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
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 |
9fb86b3 to
126c7e1
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/neurolink.ts (1)
200-211: Conflicting “tool:*” event payloads emitted twice.Each tool event is emitted twice with different payload shapes (object vs positional). This will break listeners expecting a single, consistent schema.
Apply:
// Emit tool end event (NeuroLink format - enhanced with result/error) this.emitter.emit("tool:end", { toolName, responseTime: Date.now() - startTime, success, timestamp: Date.now(), result: result, // Enhanced: include actual result error: error, // Enhanced: include error if present }); - - // ADD: Bedrock-compatible tool:end event (positional parameters) - this.emitter.emit("tool:end", toolName, success ? result : error);- // ADD: Bedrock-compatible tool:start event (positional parameters) - this.emitter.emit("tool:start", toolName, params); + // Single canonical payload only (avoid duplicate shapes)If Bedrock-compat events are required, emit under distinct event names (e.g., “bedrock:tool:start|end”).
Also applies to: 3245-3253
🧹 Nitpick comments (33)
.gitignore (1)
30-38: Broaden secret/cert ignore patternsConsider adding a few common artifacts to avoid accidental commits.
Apply this diff:
# Security Files *.key *.pem *.p12 *.pfx *.crt *.cer +*.csr +*.der +*.jks certificates/ private/ +id_rsa +id_ecdsa +id_ed25519package.json (2)
276-278: sqlite3 in onlyBuiltDependencies may cause CI/build painBuilding sqlite3 from source often fails on Alpine/macOS ARM without toolchains. If Mem0 uses sqlite only optionally, prefer optionalDependencies or ensure prebuilds.
Options:
- Move sqlite3 to optionalDependencies with install notes.
- Document required toolchain for CI images (python, make, gcc).
283-287: Overrides verified — no conflicts & no vulnerabilities
All pinned overrides for tmp, axios, devalue and undici don’t clash with any top-level deps (none installed) andpnpm audit --prodreports no issues. Add a periodic production audit to CI if not already configured.docs/memory/mem0-usage-examples.md (1)
213-217: Return the actual sessionId usedYou generate a fallback sessionId when none is provided, but the response echoes the request’s sessionId variable. Return the effective sessionId.
- res.json({ - response: result.content, - sessionId: sessionId, - success: true, - }); + const effectiveSessionId = sessionId || `session_${userId}_${Date.now()}`; + res.json({ + response: result.content, + sessionId: effectiveSessionId, + success: true, + });src/lib/types/conversationTypes.ts (1)
121-128: Comment drift in ConversationMemoryStatsThe “simplified for pure in-memory” note is outdated now that provider can be "mem0".
- * Statistics about conversation memory usage (simplified for pure in-memory storage) + * Statistics about conversation memory usage (provider-agnostic)src/lib/core/conversationMemoryManager.ts (1)
119-124: Interface conformance: buildContextMessages signatureImplement the full MemoryProvider signature to avoid future variance pitfalls.
- buildContextMessages(sessionId: string): ChatMessage[] { - // For in-memory provider, we ignore currentInput and userId + buildContextMessages( + sessionId: string, + _userId?: string, + _currentInput?: string, + ): ChatMessage[] { + // For in-memory provider, we ignore currentInput and userIdtest/memory/memoryIntegration.test.ts (1)
298-307: Brittle exact error string matchPrefer regex to avoid false negatives if wording changes.
- }).toThrow("Mem0 configuration is required"); + }).toThrow(/Mem0 configuration is required/i);src/lib/core/memoryProvider.ts (1)
39-44: Unify return type to Promise for simpler implementors/consumersReturning only Promise<ChatMessage[]> keeps implementations consistent and avoids union gymnastics in downstream code. Current callers already await the result.
Apply:
buildContextMessages( sessionId: string, userId?: string, currentInput?: string, - ): ChatMessage[] | Promise<ChatMessage[]>; + ): Promise<ChatMessage[]>;test/memory/mem0MemoryManager.test.ts (4)
62-64: Destroy manager in afterEach to avoid leaking the cleanup intervalPrevents open-handle warnings/flakiness.
afterEach(() => { vi.clearAllMocks(); + // Ensure periodic cleanup timer is cleared between tests + manager?.destroy(); });
182-199: Stub search to keep the access-time test deterministicAvoids incidental failures if search behavior changes.
const now = Date.now(); const oldTime = now - 10000; // 10 seconds ago + mockMemory.search.mockResolvedValue({ results: [] });
373-376: Don’t mutate shared testConfig after construction; set the manager’s config directlyRelies less on object identity and is clearer about what’s under test.
- // Override maxTurnsPerSession for testing - const originalConfig = { ...testConfig }; - testConfig.maxTurnsPerSession = 2; // Only 2 turns = 4 messages max + // Override maxTurnsPerSession for testing (avoid mutating shared testConfig) + // @ts-ignore - accessing private property for testing + const originalTurns = manager["config"].maxTurnsPerSession; + // @ts-ignore + manager["config"].maxTurnsPerSession = 2; // Only 2 turns = 4 messages max- // Restore original config - Object.assign(testConfig, originalConfig); + // Restore original value + // @ts-ignore + manager["config"].maxTurnsPerSession = originalTurns;Also applies to: 409-411
8-15: Optional: remove redundant vi.mock("mem0ai")You’re directly injecting manager["memory"]; the module mock isn’t used in these tests.
src/lib/utils/conversationMemoryUtils.ts (2)
45-47: Use prompt as a fallback when input.text is absentImproves context-building on clients that pass prompt only.
- const currentInput = options.input?.text; // Extract current input + const currentInput = options.input?.text ?? options.prompt; // Fallback to prompt
108-113: Fix promptLength logging to match the value storedYou store originalPrompt || prompt, but log only prompt length.
logger.debug("Conversation turn stored", { sessionId, userId, - promptLength: originalOptions.prompt?.length || 0, + promptLength: + (originalOptions.originalPrompt ?? originalOptions.prompt ?? "").length, responseLength: result.content.length, });src/lib/core/mem0MemoryManager.ts (3)
35-35: PreferReturnType<typeof setInterval>for portability.NodeJS.Timeout typing can cause friction in non-Node bundlers.
- private cleanupTimer?: NodeJS.Timeout; + private cleanupTimer?: ReturnType<typeof setInterval>;
275-283: Expose search threshold from config
Mem0’ssearchsupports athresholdoption—include it viamem0Config.search.thresholdfor consistent filtering.- this.memory!.search(currentInput, { + this.memory!.search(currentInput, { userId: searchUserId, limit: this.mem0Config.search?.maxResults || 3, + ...(typeof this.mem0Config.search?.threshold === "number" && { + threshold: this.mem0Config.search!.threshold, + }), }),
65-69: Avoid hardcoding the Mem0 version. The OSSMemoryconstructor (new Memory(config?: MemoryConfig)) defaults to version"v1.1"(also recognizes"v1.0";"v2"is only for platform APIs). Hardcoding"v1.1"risks drift if defaults change—either omit the explicit version to use the built-in default or surface it via configuration with a sensible fallback.src/lib/neurolink.ts (2)
4528-4541: getConversationHistory returns cache-only for Mem0.This calls buildContextMessages(sessionId) without userId/currentInput, so Mem0 returns only the local session cache, not persisted semantic memories. The JSDoc (“complete conversation history”) is misleading.
Confirm intended behavior. If you want full cached session history, rename doc to “session history (cache)”. If you want persisted history, extend the MemoryProvider with a dedicated getSession/getHistory API for Mem0.
Pass userId from a context source or add an overload that accepts it.
431-438: Creation via factory is good; consider surfacing provider type in logs/state.Minor: include provider type (e.g., “mem0” vs “in-memory”) in final log to simplify telemetry.
No code change required if already present elsewhere.
docs/memory/mem0-implementation-complete.md (1)
53-86: Doc claims match code (good), but add a note on Mem0-only deletion limits.You correctly state cache cleanup and timeouts. Add a short note that Mem0 doesn’t support per-session deletion (only local cache cleared), to set expectations.
I can add a short “Limitations” section.
src/lib/core/memoryProviderFactory.ts (13)
16-27: Honor config.enabled = false (avoid accidental init/work)If callers still invoke
create(...)whileenabled === false, we may still construct a real provider (and later initialize/use it). Either short‑circuit to a no‑op provider here or ensure upstream never callscreatewhen disabled.Would you like me to add a tiny in-file NoopMemoryProvider and early-return it when
!config.enabled?
24-39: Tidy up stray comments and keep errors consistentRemove TODO-ish comments and keep errors concise.
case "mem0": if (!config.mem0) { - // aren't we checking this in validate? throw new Error( 'Mem0 configuration is required when provider is "mem0"', ); } return new Mem0MemoryManager(config);
45-63: Broaden validation: provider constant + basic numeric guardsCentralize supported providers and validate core numeric fields.
+const SUPPORTED_PROVIDERS = ["in-memory", "mem0"] as const; +type SupportedProvider = (typeof SUPPORTED_PROVIDERS)[number]; static validateConfig(config: ConversationMemoryConfig): { @@ - const provider = config.provider || "in-memory"; + const provider = config.provider || "in-memory"; @@ - if (!["in-memory", "mem0"].includes(provider)) { + if (!(SUPPORTED_PROVIDERS as readonly string[]).includes(provider)) { errors.push( `Invalid memory provider: ${provider}. Must be 'in-memory' or 'mem0'.`, ); } + + // Basic numeric validations + if ( + config.maxSessions != null && + (!Number.isInteger(config.maxSessions) || config.maxSessions <= 0) + ) { + errors.push("maxSessions must be a positive integer"); + } + if ( + config.maxTurnsPerSession != null && + (!Number.isInteger(config.maxTurnsPerSession) || + config.maxTurnsPerSession <= 0) + ) { + errors.push("maxTurnsPerSession must be a positive integer"); + } + if (config.enableSummarization) { + if (!config.summarizationProvider) { + errors.push( + "summarizationProvider is required when enableSummarization is true", + ); + } + if (!config.summarizationModel) { + errors.push( + "summarizationModel is required when enableSummarization is true", + ); + } + }
79-89: Vector store allowlist constant + dimension sanity checkUse a shared constant for types; also validate
dimensionsif present.+const SUPPORTED_VECTOR_STORES = ["qdrant", "pinecone", "weaviate", "chroma"] as const; @@ - if ( - !["qdrant", "pinecone", "weaviate", "chroma"].includes( - mem0Config.vectorStore.type, - ) - ) { + if ( + !(SUPPORTED_VECTOR_STORES as readonly string[]).includes( + mem0Config.vectorStore.type, + ) + ) { // do we need to pass? errors.push( `Invalid vectorStore type: ${mem0Config.vectorStore.type}`, ); } + if ( + typeof mem0Config.vectorStore.dimensions === "number" && + (!Number.isInteger(mem0Config.vectorStore.dimensions) || + mem0Config.vectorStore.dimensions <= 0) + ) { + errors.push( + "Mem0 vectorStore.dimensions must be a positive integer", + ); + }
91-109: Remove inline notes and validate search/storage/embeddings fieldsPrune comments; add light checks for common misconfigs.
- if (!mem0Config.llmProvider) { - // do we need to pass? - errors.push("Mem0 llmProvider is required"); - } + if (!mem0Config.llmProvider) { + errors.push("Mem0 llmProvider is required"); + } @@ - if (!mem0Config.embeddings) { - // do we need to pass? - errors.push("Mem0 embeddings configuration is required"); - } else { + if (!mem0Config.embeddings) { + errors.push("Mem0 embeddings configuration is required"); + } else { if (!mem0Config.embeddings.provider) { errors.push("Mem0 embeddings.provider is required"); } if (!mem0Config.embeddings.model) { errors.push("Mem0 embeddings.model is required"); } } + + // Optional but common: search + storage validation + if ( + mem0Config.search?.threshold != null && + (mem0Config.search.threshold < 0 || + mem0Config.search.threshold > 1) + ) { + errors.push("Mem0 search.threshold must be between 0 and 1"); + } + if ( + mem0Config.search?.maxResults != null && + (!Number.isInteger(mem0Config.search.maxResults) || + mem0Config.search.maxResults <= 0) + ) { + errors.push("Mem0 search.maxResults must be a positive integer"); + } + if ( + mem0Config.storage?.historyDbPath != null && + typeof mem0Config.storage.historyDbPath !== "string" + ) { + errors.push("Mem0 storage.historyDbPath must be a string"); + }
118-133: Don’t log full config on validation errorAvoid leaking config details; log errors only.
- const errorMessage = `Memory provider configuration invalid: ${validation.errors.join(", ")}`; - logger.error(errorMessage, { config }); + const errorMessage = `Memory provider configuration invalid: ${validation.errors.join(", ")}`; + logger.error(errorMessage, { errors: validation.errors });
138-147: Type reuse for provider paramUse the same
SupportedProvideralias to avoid drift.- static getDefaultConfig( - provider: "in-memory" | "mem0", - ): Partial<ConversationMemoryConfig> { + static getDefaultConfig( + provider: SupportedProvider, + ): Partial<ConversationMemoryConfig> {
155-179: Default mem0 config: call out embedding/model/dimension couplingDims (1536) are tied to
text-embedding-3-small. If the default model changes, this silently breaks Qdrant schema. Consider deriving dims from a small provider→model→dims map in validation.I can add a minimal map for OpenAI models and validate
vectorStore.dimensionsmatches the chosen embeddings model.
189-206: Make availability guidance package-manager agnostic and capture errorSmall UX improvement.
case "mem0": try { // Try to import mem0ai to check if it's installed await import("mem0ai"); return { available: true }; - } catch { + } catch (err) { return { available: false, - reason: "mem0ai package not installed. Run: npm install mem0ai", + reason: + "mem0ai package not installed. Install with: npm i mem0ai (or yarn/pnpm equivalent).", }; }
189-214: Type reuse for provider param (availability)Mirror the earlier alias to keep signatures aligned.
- static async checkProviderAvailability( - provider: "in-memory" | "mem0", + static async checkProviderAvailability( + provider: SupportedProvider, ): Promise<{ available: boolean; reason?: string }> {
1-215: Tests to add (follow-up)
- validateConfig: passes when disabled; fails for each individual mem0 missing field; numeric guards; threshold bounds.
- getDefaultConfig: emits consistent dims/model; round-trip validate on both providers.
- checkProviderAvailability: mem0 path when module present/absent (mock dynamic import).
I can scaffold these unit tests with Vitest/Jest if you want.
12-34: Optional: collapse magic strings into top-level constantsYou already use literals in multiple places. Centralizing reduces drift.
+const ERROR_UNKNOWN_PROVIDER = (p: string) => `Unknown memory provider: ${p}`; +const ERROR_MEM0_REQUIRED = 'Mem0 configuration is required when provider is "mem0"'; @@ - throw new Error(`Unknown memory provider: ${provider}`); + throw new Error(ERROR_UNKNOWN_PROVIDER(provider));Also applies to: 79-109
24-39: Non-blocking: defensive default for future providersIf more providers are added later, consider an exhaustive switch with
neverto catch unhandled cases at compile time.I can wire a helper like
assertNever(x: never): neverand use it in thedefaultbranch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.gitignore(1 hunks)docs/memory/mem0-implementation-complete.md(1 hunks)docs/memory/mem0-manual-testing-guide.md(1 hunks)docs/memory/mem0-usage-examples.md(1 hunks)package.json(2 hunks)src/lib/core/conversationMemoryManager.ts(4 hunks)src/lib/core/mem0MemoryManager.ts(1 hunks)src/lib/core/memoryProvider.ts(1 hunks)src/lib/core/memoryProviderFactory.ts(1 hunks)src/lib/neurolink.ts(16 hunks)src/lib/types/conversationTypes.ts(4 hunks)src/lib/utils/conversationMemoryUtils.ts(4 hunks)test/memory/mem0MemoryManager.test.ts(1 hunks)test/memory/memoryIntegration.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
test/memory/mem0MemoryManager.test.ts (2)
src/lib/core/mem0MemoryManager.ts (1)
Mem0MemoryManager(17-496)src/lib/types/conversationTypes.ts (1)
ConversationMemoryConfig(54-84)
src/lib/core/memoryProviderFactory.ts (4)
src/lib/types/conversationTypes.ts (1)
ConversationMemoryConfig(54-84)src/lib/core/memoryProvider.ts (1)
MemoryProvider(15-64)src/lib/utils/logger.ts (1)
logger(341-380)src/lib/core/mem0MemoryManager.ts (1)
Mem0MemoryManager(17-496)
src/lib/core/memoryProvider.ts (1)
src/lib/types/conversationTypes.ts (2)
ChatMessage(133-139)ConversationMemoryStats(121-128)
test/memory/memoryIntegration.test.ts (2)
src/lib/neurolink.ts (2)
neurolink(4982-4982)NeuroLink(155-4979)src/lib/types/conversationTypes.ts (1)
ConversationMemoryConfig(54-84)
src/lib/utils/conversationMemoryUtils.ts (4)
src/lib/core/memoryProvider.ts (1)
MemoryProvider(15-64)src/lib/core/types.ts (1)
TextGenerationOptions(227-263)src/lib/types/conversationTypes.ts (1)
ChatMessage(133-139)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/core/mem0MemoryManager.ts (3)
src/lib/core/memoryProvider.ts (1)
MemoryProvider(15-64)src/lib/types/conversationTypes.ts (5)
ConversationMemoryConfig(54-84)Mem0Config(14-49)ChatMessage(133-139)ConversationMemoryError(177-191)ConversationMemoryStats(121-128)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/core/conversationMemoryManager.ts (1)
src/lib/core/memoryProvider.ts (1)
MemoryProvider(15-64)
src/lib/neurolink.ts (5)
src/lib/core/memoryProvider.ts (1)
MemoryProvider(15-64)src/lib/core/memoryProviderFactory.ts (1)
MemoryProviderFactory(12-215)src/lib/core/conversationMemoryManager.ts (1)
storeConversationTurn(58-113)src/lib/core/mem0MemoryManager.ts (1)
storeConversationTurn(126-172)src/lib/utils/conversationMemoryUtils.ts (1)
storeConversationTurn(82-121)
🔇 Additional comments (7)
src/lib/core/conversationMemoryManager.ts (1)
20-21: Ignore circular import warning: no cycle detected TheconversationMemoryManager.tsmodule isn’t imported byneurolink.js(or any of its dependents), so this direct import doesn’t introduce a circular dependency.Likely an incorrect or invalid review comment.
test/memory/memoryIntegration.test.ts (1)
14-31: Factory mock completenessGood addition of setupToolExecutor to match the provider surface.
test/memory/mem0MemoryManager.test.ts (1)
16-24: Good test coverage and clear expectationsCovers cold start, ID consistency, error/timeout fallbacks, TTL/LRU, per-session limits, stats, and destroy. Nice.
Also applies to: 66-171, 202-281, 283-368, 370-411, 414-463
src/lib/core/mem0MemoryManager.ts (1)
246-253: No Mem0 close/shutdown required
The mem0ai OSS Memory API does not offer aclose()orshutdown()method—only data-clearing calls (reset/delete/delete_all). Your existingdestroyimplementation (clearing the timer and session cache) is sufficient.src/lib/neurolink.ts (1)
1296-1303: Generated verification scripts to inspectgenerate()andgenerateTextInternal()for duplicate event emits. Please review the output to confirm whether both functions emitresponse:start/response:endand we should relocate those events as suggested.docs/memory/mem0-implementation-complete.md (1)
120-129: Please share the exactnew Memory(…)constructor call and its option parameters from your code (including surrounding imports) so I can confirm which mem0ai version fully supports that API.src/lib/core/memoryProviderFactory.ts (1)
12-23: Solid factory shape and public APIClass design and method set are cohesive. Logging on creation is helpful.
fccccfc to
a41725c
Compare
a41725c to
d8da87d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR integrates mem0 memory provider to enhance conversation context management with semantic memory capabilities. The integration provides cross-session memory storage and retrieval using vector embeddings, enabling more contextual and personalized AI conversations.
- Added mem0ai dependency and configuration support for semantic memory
- Implemented memory retrieval before generation and background storage after responses
- Created comprehensive test suite demonstrating memory isolation and streaming functionality
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/types/conversation.ts | Added mem0 configuration types and enabled flag |
| src/lib/neurolink.ts | Integrated mem0 memory retrieval/storage in generate and stream methods |
| src/lib/memory/mem0Initializer.ts | Created mem0 initialization module with fallback implementation |
| scripts/examples/real-memory-test.js | Added comprehensive test suite for memory functionality |
| package.json | Added mem0ai dependency and updated package versions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
d8da87d to
0feae56
Compare
d698e6d to
bbb4440
Compare
338ceab to
e19f1da
Compare
|
aa3211b to
5ed67c4
Compare
cmd-err
changed the title
5ed67c4 to
7ce99c2
Compare
cmd-err
changed the title
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Before:
After:
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation
Chores