feat: User Memory via mem0 (Issue #24)#59
Conversation
- Add src/modules/memory.js — mem0 REST API client wrapper with
addMemory, searchMemories, getMemories, deleteMemory, deleteAllMemories,
buildMemoryContext, and extractAndStoreMemories
- Add /memory command (view, forget, forget <topic>) for user data control
- Integrate memory into AI pipeline: pre-response context injection and
post-response memory extraction (fire-and-forget)
- Add mem0 health check on startup with graceful fallback (AI works
without mem0)
- Memory scoping: user_id=Discord ID, app_id=bills-bot
- Config: memory.{enabled, maxContextMemories, autoExtract, extractModel}
- 55 new tests (44 memory module + 11 command), all 668 tests passing
- Update .env.example with MEM0_API_URL
Closes #24
|
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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds mem0-based per-user memory with an opt-out system and DB persistence: new env var and config, mem0 integration and health checks, /memory slash command, AI pre/post memory hooks, startup loading of opt-outs, extensive tests, and a new runtime dependency for mem0. Changes
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
tests/modules/events.test.js (1)
400-428: 🧹 Nitpick | 🔵 TrivialTest correctly verifies userId propagation — but coverage is limited to this one case.
This test properly asserts that
message.author.idis forwarded as the 6th argument togenerateResponse. However, most other AI-related test messages (e.g., Lines 189, 211, 233, etc.) omitauthor.idfrom the mock, meaningundefinedis silently passed asuserIdin those paths. Consider addingidto the author mocks in other AI tests for consistency.
🤖 Fix all issues with AI agents
In `@src/commands/memory.js`:
- Around line 154-159: The loop that finds matches and calls deleteMemory
sequentially should be changed to perform deletions in parallel: collect the
found ids by iterating matches and matching against allMemories (using the same
predicate as currently does with allMemories.find), then call deleteMemory for
each id and await Promise.allSettled on that array of promises (instead of
awaiting each delete inside the loop). After Promise.allSettled, update
deletedCount by counting fulfilled results (and only increment for successful
deletions); keep using the existing identifiers deleteMemory, matches,
allMemories, and deletedCount to locate the logic to change.
- Around line 149-160: handleForgetTopic currently pulls allMemories via
getMemories and uses fragile exact text matching (m.memory === match.memory)
which breaks when fields differ or texts duplicate and causes an extra full
fetch; update the logic so searchMemories returns IDs if possible and use those
IDs with deleteMemory directly (preferable), otherwise match using multiple
candidate fields (m.memory || m.text || m.content) and ensure you remove or mark
a matched item from the local allMemories list after using it (e.g., pop/splice
or maintain a map of text->queue of items) so duplicate texts do not repeatedly
match the same memory, and avoid re-fetching all memories on each call. Ensure
you update references in handleForgetTopic to use searchMemories, getMemories,
deleteMemory and the matches array accordingly.
- Around line 88-100: Replace the manual substring truncation with the
splitMessage() utility: import splitMessage and build the full message string
from header + memoryList (remove the truncated variable/substring logic), call
splitMessage(fullMessage) to get chunks under Discord's 2000-char limit, then
editReply with the first chunk and send any remaining chunks via
interaction.followUp in order; update references in this block that use
memoryList, header, truncated to use the splitMessage result instead.
In `@src/modules/memory.js`:
- Around line 44-61: The catch fallback in getMemoryConfig currently defaults
enabled to true which can inadvertently enable memory when getConfig() fails;
update the catch return in getMemoryConfig so enabled is false (while keeping
the other fallbacks like maxContextMemories: DEFAULT_MAX_CONTEXT_MEMORIES,
autoExtract: true, extractModel: null) to ensure memory stays off if
configuration cannot be read; reference getConfig, getMemoryConfig, and
DEFAULT_MAX_CONTEXT_MEMORIES when locating the change.
- Around line 117-126: The current mem0Fetch catch block sets mem0Available =
false on any error which permanently disables memory since checkMem0Health()
only runs at startup; update the logic in the mem0Fetch error handler to avoid
poisoning global state on a single transient error: either remove the
unconditional mem0Available = false assignment so failures just return null, or
implement a guarded disable strategy in mem0Fetch that tracks consecutive
failures (e.g., increment a mem0FailureCount and only set mem0Available = false
after N consecutive failures) and reset the count on success, or schedule
periodic health rechecks (call checkMem0Health or trigger a backoff/retry timer)
to restore mem0Available when the service recovers—use the function/method names
mem0Fetch, mem0Available, mem0FailureCount (create it if needed), and
checkMem0Health to locate and implement the change.
- Around line 106-126: The code currently calls response.json() unguarded which
throws on 204/empty bodies and causes mem0Available to be set false; update the
success path (the block that uses response.ok and calls response.json()) to
first handle empty responses—e.g., if response.status === 204 return null
immediately, or read response.text() and if it's empty return null, otherwise
parse JSON—so that response.json() is only called when there is a body; ensure
the catch block still handles real network errors but does not treat empty-body
JSON parse failures as a mem0 outage (keep the mem0Available=false behavior only
for genuine network/fetch errors).
- Around line 134-174: The health check in checkMem0Health currently calls GET
/v1/memories/ and treats 404 as healthy; change it to call the documented mem0
health endpoints instead by first calling GET `${getMem0Url()}/health` and if
that returns 404 try GET `${getMem0Url()}/` (OSS root) as a fallback, and only
mark mem0Available = true when a request returns a successful status
(response.ok / 200); remove the special-case that treats 404 as healthy. Keep
the existing AbortController/REQUEST_TIMEOUT_MS timeout logic and logging (info,
logWarn) and ensure errors still set mem0Available = false and log the error
message.
In `@tests/commands/memory.test.js`:
- Around line 209-236: The forget tests for the /memory command should also
assert that the interaction deferral occurred; after creating the mock
interaction and before or after calling execute(interaction) in both "should
delete all memories and confirm" and "should show error when deletion fails" add
an assertion that interaction.deferReply was called with { ephemeral: true }
(reference mock helpers createMockInteraction and the execute function and the
deleteAllMemories mock) to match the view tests’ behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.env.exampleconfig.jsonsrc/commands/memory.jssrc/index.jssrc/modules/ai.jssrc/modules/events.jssrc/modules/memory.jstests/commands/memory.test.jstests/index.test.jstests/modules/ai.test.jstests/modules/events.test.jstests/modules/memory.test.js
🧰 Additional context used
📓 Path-based instructions (5)
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/index.test.jstests/modules/events.test.jstests/modules/memory.test.jstests/commands/memory.test.jstests/modules/ai.test.js
.env.example
📄 CodeRabbit inference engine (AGENTS.md)
Update .env.example immediately when adding, removing, or renaming environment variables
Files:
.env.example
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/index.jssrc/commands/memory.jssrc/modules/events.jssrc/modules/ai.jssrc/modules/memory.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/events.jssrc/modules/ai.jssrc/modules/memory.js
🧠 Learnings (1)
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/*.js : Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Applied to files:
src/commands/memory.jstests/commands/memory.test.js
🧬 Code graph analysis (6)
src/index.js (1)
src/modules/memory.js (1)
checkMem0Health(134-174)
src/commands/memory.js (1)
src/modules/memory.js (8)
isMemoryAvailable(67-70)memories(234-234)memories(260-260)memories(321-321)getMemories(247-266)deleteAllMemories(273-289)searchMemories(213-240)deleteMemory(296-309)
src/modules/ai.js (1)
src/modules/memory.js (3)
config(46-46)buildMemoryContext(318-328)extractAndStoreMemories(339-369)
tests/commands/memory.test.js (2)
src/commands/memory.js (7)
subcommand(44-44)topic(60-60)userId(45-45)data(21-37)data(21-37)execute(43-67)memories(78-78)src/modules/memory.js (9)
isMemoryAvailable(67-70)getMemories(247-266)deleteAllMemories(273-289)searchMemories(213-240)deleteMemory(296-309)data(115-115)memories(234-234)memories(260-260)memories(321-321)
tests/modules/ai.test.js (2)
src/modules/memory.js (3)
buildMemoryContext(318-328)config(46-46)extractAndStoreMemories(339-369)src/modules/ai.js (4)
config(35-35)config(50-50)generateResponse(386-478)reply(448-448)
src/modules/memory.js (2)
src/modules/config.js (2)
getConfig(130-132)err(30-30)src/logger.js (2)
debug(209-211)info(216-218)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (12)
src/modules/ai.js (1)
9-9: Clean memory integration with graceful fallback — LGTM.The pre-response memory context injection (Lines 403–414) is properly guarded with try/catch and only activates when
userIdis provided. The post-response extraction (Lines 463–468) correctly uses fire-and-forget with a.catch()safety net. Backward compatibility is preserved withuserId = null.Also applies to: 392-414, 463-468
.env.example (1)
33-34: LGTM.The new
MEM0_API_URLenv var is properly documented with an explanatory comment and sensible default, consistent with howgetMem0Url()insrc/modules/memory.jsresolves it. As per coding guidelines: "Update .env.example immediately when adding, removing, or renaming environment variables."config.json (1)
66-71: LGTM.Memory configuration block is consistent with the defaults in
getMemoryConfig()and the new/memorycommand is appropriately permissioned as"everyone"since it only operates on the invoking user's own data.src/index.js (1)
292-293: LGTM — proper startup ordering.The health check runs after DB and config initialization but before event handlers register, ensuring
mem0Availableis correctly set before any user messages arrive. The function handles its own errors internally, so no wrapping is needed here.src/modules/events.js (1)
126-133: LGTM.Clean one-line change correctly passes
message.author.idas the newuserIdparameter togenerateResponse, enabling memory context for AI responses.tests/index.test.js (1)
145-147: LGTM — memory module mock provides a safe default for existing tests.The
checkMem0Healthmock correctly resolves tofalse, ensuring existing startup tests aren't affected by the new memory integration. One minor note: unlike other module mocks which are centralized in the hoistedmocksobject (making per-test reset easy viaimportIndex), this mock uses an inlinevi.fn(). This is acceptable here since no existing test needs to override the health check result, but if future tests need to simulate a healthy mem0, consider hoisting it for consistency.tests/commands/memory.test.js (1)
1-323: Well-structured test suite with good coverage of all command paths.The tests cover the key scenarios: unavailable state, view (empty/formatted/truncated), forget-all (success/failure), and forget-by-topic (match/no-match/failure/multiple). The
createMockInteractionhelper is clean and reusable.tests/modules/ai.test.js (2)
13-17: LGTM — memory module mock and import are correctly set up.The mock returns safe defaults (empty string for
buildMemoryContext,falseforextractAndStoreMemories), ensuring existing AI tests aren't affected while new memory-specific tests can override behavior as needed.Also applies to: 32-32
235-338: Solid coverage of all memory integration paths ingenerateResponse.The five new tests comprehensively cover:
- Memory context injection into system prompt (with verification of the actual fetch body)
- No injection when
userIdis null- Fire-and-forget extraction after response (correctly using
vi.waitForfor the async assertion)- Graceful fallback when
buildMemoryContextrejects- No extraction when
userIdis omittedThis aligns well with the PR objective of ensuring AI continues operating when mem0 is unreachable.
tests/modules/memory.test.js (3)
1-63: LGTM — test harness is well-structured.Good use of the
_setMem0Availabletest helper to control internal state, and thefetchSpypattern keeps network calls fully deterministic. ThebeforeEachcleanup (clearing env vars, resetting config mock, resetting availability) ensures test isolation.
65-183: Thorough coverage of configuration and health check paths.Notably, testing that a 404 response still marks mem0 as available (line 151–157) is a smart edge case — it correctly handles servers that are running but don't expose a dedicated health endpoint. The disabled-config test (line 175–182) properly verifies no fetch is made at all.
185-569: Comprehensive CRUD and edge-case coverage.The field-variant test for
searchMemories(lines 316–334) is particularly valuable since it validates resilience against different mem0 API response shapes. The timeout/abort test (lines 557–569) ensures the circuit-breaker behavior works correctly when requests time out.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Replace raw HTTP fetch calls with mem0ai SDK (v2.2.2)
- Switch from self-hosted mem0 REST API to hosted platform (api.mem0.ai)
- Enable graph memory (enable_graph: true) on all add/search/getAll calls
- Add graph relation context to buildMemoryContext() for richer AI prompts
- Update searchMemories() to return { memories, relations } with graph data
- Add formatRelations() helper for readable relation context
- Replace MEM0_API_URL env var with MEM0_API_KEY for hosted platform auth
- Update health check to verify API key config and SDK client initialization
- Add _setClient() test helper for SDK mock injection
- Rewrite all tests to mock SDK instead of global fetch
- Update /memory forget command to destructure new searchMemories format
Closes #24
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/modules/memory.test.js`:
- Around line 171-184: The test named "should mark as unavailable when client
creation fails" is misleading because it mocks MemoryClient to return an object
and then expects success; change the test to actually exercise the failure path
by resetting the client with _setClient(null), overriding the mocked
MemoryClient constructor (the default export from 'mem0ai') to throw when
instantiated, then call checkMem0Health() and assert it returns false and that
isMemoryAvailable() is false; alternatively split into two tests: one that
asserts success when the mock returns an instance and a new one that sets
MemoryClient.mockImplementation(() => { throw new Error(...) }) and asserts
failure as described.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.env.examplepackage.jsonsrc/commands/memory.jssrc/modules/memory.jstests/commands/memory.test.jstests/modules/memory.test.js
🧰 Additional context used
📓 Path-based instructions (5)
.env.example
📄 CodeRabbit inference engine (AGENTS.md)
Update .env.example immediately when adding, removing, or renaming environment variables
Files:
.env.example
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/commands/memory.jssrc/modules/memory.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/memory.js
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/commands/memory.test.jstests/modules/memory.test.js
🧠 Learnings (5)
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to .env.example : Update .env.example immediately when adding, removing, or renaming environment variables
Applied to files:
.env.example
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/*.js : Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Applied to files:
src/commands/memory.jstests/commands/memory.test.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/commands/memory.js
📚 Learning: 2025-10-10T15:05:26.145Z
Learnt from: CR
Repo: BillChirico/LUA-Obfuscator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T15:05:26.145Z
Learning: Applies to package.json : Only add new packages when absolutely necessary or explicitly requested
Applied to files:
package.json
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to {package.json,.npmrc,pnpm-lock.yaml} : Use pnpm package manager (pinned to v10.23.0) with `.npmrc` configured for strict peer dependencies and disabled shamefully-hoist
Applied to files:
package.json
🧬 Code graph analysis (3)
src/commands/memory.js (2)
src/index.js (1)
interaction(158-158)src/modules/memory.js (7)
isMemoryAvailable(78-81)memories(197-200)memories(228-228)getMemories(215-239)deleteAllMemories(246-260)searchMemories(176-208)searchMemories(307-307)
src/modules/memory.js (2)
src/modules/config.js (2)
err(30-30)getConfig(130-132)src/logger.js (2)
info(216-218)debug(209-211)
tests/commands/memory.test.js (2)
src/commands/memory.js (7)
subcommand(44-44)topic(60-60)userId(45-45)data(21-37)data(21-37)execute(43-67)memories(78-78)src/modules/memory.js (8)
isMemoryAvailable(78-81)getMemories(215-239)deleteAllMemories(246-260)searchMemories(176-208)searchMemories(307-307)deleteMemory(267-281)memories(197-200)memories(228-228)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
.env.example (1)
33-35: LGTM!The new
MEM0_API_KEYenvironment variable is well-documented with clear comments about its optional nature and where to obtain the key.src/modules/memory.js (2)
1-13: LGTM on the overall module structure and graceful fallback design.The module is well-organized with clear separation of concerns: config, availability, CRUD operations, context building, and extraction. The safe defaults (empty arrays,
false) ensure the AI pipeline continues when mem0 is unavailable. The per-user scoping withapp_idnamespacing is solid.Also applies to: 36-49, 78-81, 144-166, 246-260, 267-281, 288-294, 304-324, 336-367
15-15: No issues found. Themem0ainpm package v2.2.2 exportsMemoryClientas the default export, so the import statement is correct.src/commands/memory.js (1)
21-67: LGTM on command structure and availability guard.The command correctly exports
dataandexecute, properly checksisMemoryAvailable()before proceeding, and uses ephemeral replies throughout to keep memory data private to the user.tests/modules/memory.test.js (1)
1-84: LGTM on overall test coverage and structure.Comprehensive coverage of all module functions with proper state isolation between tests via
beforeEach/afterEachcleanup. Good coverage of edge cases (null client, unavailable state, array vs object response formats, field name variants).Also applies to: 85-126, 128-144, 187-242, 244-349, 351-412, 414-489, 491-521, 523-609, 611-675
tests/commands/memory.test.js (1)
1-333: LGTM on command test coverage.Good coverage of all subcommand paths including success, failure, empty states, truncation, and multi-deletion counting. The mock interaction helper is clean and reusable.
package.json (1)
22-22:⚠️ Potential issue | 🔴 CriticalUse a valid version for
mem0aipackage.The specified version
^2.2.2does not exist on npm. The latest available version is2.1.38. Update the dependency to use a valid version, such as"mem0ai": "^2.1.38"or another existing 2.1.x release.⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxCommunity/Volvox.Website PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-02-04T02:20:09.131Z Learning: Applies to {package.json,.npmrc,pnpm-lock.yaml} : Use pnpm package manager (pinned to v10.23.0) with `.npmrc` configured for strict peer dependencies and disabled shamefully-hoistLearnt from: CR Repo: BillChirico/LUA-Obfuscator PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T15:05:26.145Z Learning: Applies to package.json : Only add new packages when absolutely necessary or explicitly requested
…s in memory command - Use splitMessage() utility instead of manual substring truncation for Discord's 2000-char limit, properly handling multi-byte characters (#1) - Include memory IDs in searchMemories results and use them directly in handleForgetTopic instead of fragile text equality matching (#2) - Parallelize deleteMemory calls with Promise.allSettled instead of sequential for loop (#3) - Verify deferReply is called in all forget test variants (#7)
Replace permanent mem0Available = false on API errors with a cooldown- based recovery mechanism. After RECOVERY_COOLDOWN_MS (60s), the next request is allowed through to check if the service recovered. If it fails again, the cooldown resets. This prevents a single transient network error from permanently disabling the memory system for all users until restart. Resolves review threads #4, #8, #12.
…reation The health check now performs a lightweight search against the mem0 platform to verify the SDK client actually works, rather than just checking that the client object was created. This properly validates connectivity with the hosted mem0 platform. Also fixes misleading test name that asserted success but was named as a failure case. Resolves review threads #5, #11.
Add privacy documentation to the memory module explaining that user messages are sent to the mem0 hosted platform for processing. Update the /memory command description to indicate external storage. Users can view and delete their data via /memory view and /memory forget. Resolves review thread #6.
- Add /memory optout subcommand to toggle memory collection per user - In-memory Set with JSON file persistence (data/optout.json) - extractAndStoreMemories and buildMemoryContext skip opted-out users - Toggle behavior: running again opts back in - Add confirmation prompt on /memory forget (all) - Discord ButtonBuilder with Confirm (Danger) and Cancel buttons - 30-second timeout with auto-cancel - Only the command user can interact with buttons - Add /memory admin view and /memory admin clear subcommands - Requires ManageGuild or Administrator permission - Admin view shows target user's memories with opt-out status - Admin clear includes confirmation prompt before deletion - Comprehensive test coverage for all new features - 17 new optout module tests - 32 total memory command tests (was 12) - 54 total memory module tests (was 52) - All metrics above 80% threshold
|
Claude finished @BillChirico's task in 1m 28s —— View job Review Complete: PR #59 - User Memory via mem0 ✅
Verdict: APPROVED This PR implements a well-architected mem0 integration for persistent user memory. After thorough review of all 17 changed files: Code Quality: Clean ESM modules, proper Winston logging (no console.*), single quotes, semicolons, 2-space indentation — all conventions followed. Architecture: Proper separation of concerns with Error Handling: Intelligent transient vs permanent error classification with auto-recovery cooldown. Graceful fallbacks ensure the bot works when mem0 is unavailable. Testing: Comprehensive test coverage with 890 lines for memory module and 887 lines for command tests. CI passing with 80%+ coverage threshold met. Security: No hardcoded credentials, API key read from env, user opt-out persisted to DB, ephemeral replies for sensitive data, admin permission checks implemented. Documentation: All previous review feedback from coderabbitai, cursor, and claude has been addressed in the latest commits. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR implements a comprehensive mem0 integration for user memory with good test coverage (93%+) and solid architecture. However, there is one critical issue that must be fixed before merging:
🔴 Critical Issue
Opt-out state not loaded on startup - loadOptOuts() is never called during the startup sequence in src/index.js. This means after a bot restart, users who previously opted out will have their messages sent to mem0 for memory extraction, violating their explicit privacy choice. This is a data privacy bug that must be fixed.
🟡 Warnings
- Memory defaults to enabled when config fails - In
src/modules/memory.js, the catch fallback ingetMemoryConfig()defaultsenabled: true. Consider defaulting tofalsefor fail-safe behavior.
🔵 Nitpicks
- Inconsistent
markUnavailable()in delete functions -deleteAllMemoriesanddeleteMemorydon't callmarkUnavailable()on errors, unlike other API functions.
✅ What's Good
- Clean ESM module structure following project conventions
- Proper Winston logging (no console.* usage)
- Comprehensive test coverage with 806 lines of memory tests
- Good error handling with graceful fallbacks
- Proper use of
splitMessage()utility for Discord limits - Well-documented JSDoc comments
- Privacy notice in module header
- Admin commands properly check permissions
- Ephemeral replies for sensitive data
See inline comments for specific locations and suggested fixes.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/commands/memory.js`:
- Around line 153-182: Duplicate truncation/composition logic in handleView and
handleAdminView should be extracted into a small helper to avoid duplication:
create a function (e.g., composeTruncatedContent or formatTruncatedMemoryView)
that accepts headerPrefix (like `🧠 **What I remember about ${username}:**`),
the full body text (memoryList), a truncationNotice string, and the splitMessage
function/max length logic, then returns the final content string; replace the
header/maxBodyLength/splitMessage/content composition in handleView and
handleAdminView with calls to this helper and keep existing calls to
splitMessage, max length computation and truncationNotice behavior preserved.
In `@tests/modules/memory.test.js`:
- Around line 149-187: The test is leaving the Promise from addMemory un-awaited
so assertions inside .then() and the vi.useRealTimers() call can be skipped;
make the test function async and await the addMemory('user123','test') call (or
use await expect(...) pattern) so the expect(isMemoryAvailable()) assertions run
after the awaited call, advance timers, and then verify recovery; also ensure
vi.useRealTimers() runs in a finally block (or after awaiting) to reliably
restore timers. Reference: addMemory, isMemoryAvailable, _setMem0Available,
_setClient, _getRecoveryCooldownMs, vi.useFakeTimers, vi.useRealTimers.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
config.jsonsrc/commands/memory.jssrc/modules/memory.jssrc/modules/optout.jstests/commands/memory.test.jstests/modules/memory.test.jstests/modules/optout.test.js
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/commands/memory.test.jstests/modules/memory.test.jstests/modules/optout.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/modules/optout.jssrc/commands/memory.jssrc/modules/memory.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/optout.jssrc/modules/memory.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.js
🧬 Code graph analysis (4)
tests/commands/memory.test.js (3)
src/commands/memory.js (12)
subcommand(85-85)subcommandGroup(84-84)topic(113-113)userId(86-86)toggleOptOut(128-128)data(37-77)data(37-77)execute(83-120)memories(156-156)memories(335-335)content(175-177)content(353-355)src/modules/memory.js (8)
isMemoryAvailable(111-125)getMemories(286-310)deleteAllMemories(317-331)searchMemories(246-279)searchMemories(379-379)deleteMemory(338-352)memories(267-271)memories(299-299)src/modules/optout.js (4)
toggleOptOut(59-71)isOptedOut(48-50)data(85-85)data(110-110)
src/commands/memory.js (3)
src/modules/optout.js (4)
data(85-85)data(110-110)toggleOptOut(59-71)isOptedOut(48-50)src/modules/memory.js (8)
isMemoryAvailable(111-125)memories(267-271)memories(299-299)getMemories(286-310)deleteAllMemories(317-331)searchMemories(246-279)searchMemories(379-379)deleteMemory(338-352)src/utils/splitMessage.js (1)
splitMessage(24-51)
tests/modules/optout.test.js (1)
src/modules/optout.js (6)
_resetOptouts(38-41)_setOptoutPath(31-33)isOptedOut(48-50)toggleOptOut(59-71)loadOptOuts(77-98)saveOptOuts(103-115)
src/modules/memory.js (4)
src/modules/config.js (2)
err(30-30)getConfig(130-132)src/logger.js (2)
info(216-218)debug(209-211)src/commands/memory.js (4)
userId(86-86)memories(156-156)memories(335-335)username(87-87)src/modules/optout.js (1)
isOptedOut(48-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (21)
config.json (2)
66-70: LGTM!The memory configuration block has sensible defaults. The
enabled: truedefault is safe becausecheckMem0Healthwill gracefully disable memory features ifMEM0_API_KEYis not set.
81-81: LGTM!Granting
memorycommand access to "everyone" is appropriate — users need to view and manage their own data.src/modules/optout.js (1)
1-115: Well-structured opt-out module with clean error handling.Uses
node:protocol for builtins, Winston for logging, graceful fallback on missing/corrupt files, and synchronous file I/O which is acceptable for this infrequent operation. The testing hooks (_setOptoutPath,_resetOptouts) are a clean approach for testability.tests/modules/optout.test.js (1)
1-185: Comprehensive test suite with good edge-case coverage.The tests exercise all public API methods, error paths (corrupt JSON, non-array data, read/write errors), and the testing hooks. The mock setup and cleanup are well-structured.
src/modules/memory.js (7)
22-28: LGTM — clean module structure.The imports follow coding guidelines (ESM, Winston logger,
getConfig), and theAPP_IDconstant properly namespaces all mem0 operations to isolate this bot's data.
50-61: Well-designed cooldown-based recovery pattern.
markUnavailable/markAvailablewith timestamp tracking is a clean improvement over the previous "permanently disabled on first failure" approach. This allows automatic retry after the cooldown period.
162-199: Health check via SDKsearchis a pragmatic approach.Using a lightweight
searchcall with a dummy user validates end-to-end SDK connectivity without needing a dedicated health endpoint. This addresses the prior review concern about raw HTTP calls to undocumented endpoints.
246-279: Defensive handling of both response formats is good.The fallback chain
Array.isArray(result) ? result : result?.results || []correctly handles the SDK returning either a raw array or a{ results, relations }object, which guards against SDK version differences.
317-352: Delete methods intentionally skipmarkUnavailable— reasonable design.Unlike
addMemory/searchMemories/getMemories, the delete methods don't callmarkUnavailable()on failure. This is a sound choice since delete failures can be item-specific (e.g., not found) rather than service-level outages.
408-441:extractAndStoreMemoriescorrectly gates on opt-out and autoExtract config.The three-level guard (availability → opt-out → autoExtract) ensures user privacy preferences and config are respected before sending data to the external mem0 platform.
68-81: MemoryClient constructor and methods are correctly implemented for mem0ai v2.2.2.The constructor
new MemoryClient({ apiKey })and all methods used (add,search,getAll,delete,deleteAll) are documented and correctly implemented in the code. The API surface matches the v2.2.2 specification, including proper option handling (e.g.,enable_graph,user_id,app_id).src/commands/memory.js (4)
37-77: LGTM — well-structured command definition.The command builder correctly defines subcommands (
view,forget,optout) and anadminsubcommand group with its own subcommands (view,clear). Thetopicoption onforgetis properly optional.
83-120: Clean dispatch logic with correct ordering.Admin check → opt-out (bypasses availability gate) → memory availability check → subcommand dispatch. This ensures opt-out always works even when mem0 is down.
255-283: Good use ofPromise.allSettledfor parallel deletions.This addresses the prior review feedback about sequential HTTP deletes. The
matchesWithIdsfilter ensures only memories with valid IDs are attempted, and the settled results are correctly filtered for successful deletions.
290-324: Permission check correctly handlesnullmemberPermissions.Line 293 uses short-circuit evaluation (
interaction.memberPermissions && ...) which safely handles DM contexts or other cases wherememberPermissionsis null. The admin permission check gates access before any memory operations or user lookups occur.tests/commands/memory.test.js (1)
1-887: Thorough and well-organized test suite.The test suite covers all subcommands and edge cases: data export, availability gating, view (empty/populated/truncated), opt-out/opt-in, forget-all confirmation flow (confirm/cancel/timeout/filter), forget-by-topic (match/no-match/failure/parallel), and admin flows (permissions/view/clear/timeout/null-permissions). The
createMockInteractionhelper is clean and flexible.tests/modules/memory.test.js (5)
237-293: LGTM — health check tests are comprehensive.Covers all paths: successful connectivity, missing API key, disabled config, auto-created client failure, and SDK connectivity error.
295-350: LGTM — addMemory tests are thorough.Tests cover unavailable state, correct SDK call parameters, error handling with
markUnavailable, optional metadata, and null client. Good assertion on the exactaddcall shape includingenable_graph: true.
352-457: LGTM — searchMemories tests cover all response format variants.Good coverage of the SDK returning either an array or
{ results, relations }object, the fallback field chain (memory,text,content), custom limit parameter, error handling, and null client.
631-728: LGTM — buildMemoryContext tests validate all combinations.Tests cover unavailable, opted-out (verifying search is not called), full context (memories + relations), empty results, relations-only, and memories-only paths.
730-805: LGTM — extractAndStoreMemories tests cover all guard conditions.Validates unavailable state, opt-out (verifying
addnot called), autoExtract disabled, correct message format with username prefix, SDK error marking unavailable, and null client.
- Add memory_optouts table (user_id TEXT PK, created_at TIMESTAMPTZ) to db.js schema - Rewrite optout.js: remove all filesystem code, use DB pool for persistence - Keep in-memory Set for fast lookups (isOptedOut called on every message) - Best-effort DB persistence: log warning on failure, keep in-memory state - loadOptOuts now async, loads from DB on startup with graceful fallback - toggleOptOut now async, writes INSERT/DELETE to DB - Export _setPool for test injection, keep _resetOptouts for testing - Update memory.js to await async toggleOptOut - Rewrite tests to mock DB pool instead of filesystem - All 714 tests passing, lint clean
There was a problem hiding this comment.
Review Summary
This PR implements a well-architected mem0 integration for user memory with good test coverage and follows most project conventions. However, there is one critical issue that must be fixed before merging:
🔴 Critical Issue
Opt-out state not loaded on startup (src/index.js:293)
loadOptOuts()is never called during the startup sequence- After a bot restart, previously opted-out users' messages will be sent to mem0 for memory extraction
- This is a data privacy violation that silently breaks user consent
🟡 Warnings
Memory defaults to enabled when config fails (src/modules/memory.js:97)
- The catch fallback in
getMemoryConfig()defaultsenabled: true - Consider defaulting to
falsefor fail-safe behavior
🔵 Nitpicks
-
Inconsistent
markUnavailable()in delete functions (src/modules/memory.js:329, 349)deleteAllMemoriesanddeleteMemorydon't callmarkUnavailable()on errors unlike other API functions
-
Test has unawaited promise (
tests/modules/memory.test.js:176)- Assertions inside
.then()callback are not awaited
- Assertions inside
See inline comments for specific locations and suggested fixes.
Opt-out preferences were never loaded from the database on startup, causing all users to appear opted-in after a restart. Now loadOptOuts() is awaited during the startup sequence before the mem0 health check.
Previously the catch fallback in getMemoryConfig() defaulted to enabled: true, meaning a config failure would silently enable memory collection. Now defaults to enabled: false and autoExtract: false so memory is safely disabled if config is unavailable.
deleteAllMemories and deleteMemory were missing markUnavailable() calls in their catch blocks, inconsistent with all other API methods (addMemory, searchMemories, getMemories, extractAndStoreMemories). Now transient failures in delete operations trigger the same cooldown/recovery mechanism.
handleView and handleAdminView had identical truncation logic for fitting memory lists within Discord's 2000-char limit. Extracted into a shared formatMemoryList() helper to reduce duplication.
Test 'should return defaults when getConfig throws' renamed to 'should return safe disabled fallback when getConfig throws' to match the new behavior where config failure disables memory. Updated assertions to expect enabled: false and autoExtract: false.
The auto-recovery test had an unawaited .then() chain which meant assertions inside the callback could silently pass or fail without affecting the test result. Refactored to use async/await with a simpler, more reliable test structure.
There was a problem hiding this comment.
LGTM - All previously identified issues have been addressed in commits 9638b44, 8a9deb9, 3ca854d, and 4c2f297. The mem0 integration follows project conventions, has comprehensive test coverage (CI passing), and properly handles opt-out persistence, config error fallbacks, and transient API failures.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/modules/memory.js`:
- Around line 131-138: The helper _setMem0Available intentionally treats false
as a deterministic test hook by clearing mem0UnavailableSince to 0 (preventing
auto-recovery logic in isMemoryAvailable) rather than recording a timestamp like
markUnavailable(); add a concise inline comment inside _setMem0Available
explaining that behavior and why it differs from markUnavailable (mentioning
mem0UnavailableSince, markAvailable, markUnavailable, and isMemoryAvailable) so
future readers understand it's deliberate for deterministic testing.
In `@tests/modules/memory.test.js`:
- Around line 254-263: The test for checkMem0Health relies on the mocked
MemoryClient constructor returning an object without a search method which is
fragile; update the test so it explicitly sets the auto-created client's shape
instead of relying on the global mock: call _setClient(null) to trigger
auto-creation but then explicitly mock MemoryClient (or the factory used by
checkMem0Health) to return an object lacking search or to have search throw, and
add a one-line comment stating the intent; reference the MemoryClient mock, the
checkMem0Health function, and isMemoryAvailable to locate where to change the
test.
- Around line 150-217: Tests call vi.useFakeTimers() but restore with
vi.useRealTimers() only inside each test, risking leaked fake timers if a test
throws; add a guaranteed cleanup by adding an afterEach in the test suite (e.g.,
inside the describe block that contains the isMemoryAvailable-related tests)
that calls vi.useRealTimers(), and remove the per-test vi.useRealTimers() calls
(or keep them if preferred) so timers are always restored even on failures;
reference the existing tests that call vi.useFakeTimers(), vi.useRealTimers(),
and helper functions like _setMem0Available, addMemory, searchMemories, and
_getRecoveryCooldownMs to locate where to add the afterEach.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/commands/memory.jssrc/db.jssrc/index.jssrc/modules/memory.jssrc/modules/optout.jstests/modules/memory.test.jstests/modules/optout.test.js
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/commands/memory.jssrc/index.jssrc/db.jssrc/modules/optout.jssrc/modules/memory.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/optout.jssrc/modules/memory.js
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/modules/memory.test.jstests/modules/optout.test.js
🧬 Code graph analysis (4)
src/index.js (2)
src/modules/optout.js (1)
loadOptOuts(108-124)src/modules/memory.js (1)
checkMem0Health(162-199)
src/modules/optout.js (2)
src/db.js (2)
pool(12-12)getPool(189-194)src/logger.js (1)
info(216-218)
src/modules/memory.js (3)
src/modules/config.js (1)
getConfig(130-132)src/logger.js (2)
info(216-218)debug(209-211)src/modules/optout.js (2)
result(117-117)isOptedOut(58-60)
tests/modules/optout.test.js (3)
src/modules/optout.js (6)
_resetOptouts(48-51)_setPool(41-43)isOptedOut(58-60)toggleOptOut(69-102)result(117-117)loadOptOuts(108-124)src/logger.js (1)
warn(223-225)src/db.js (1)
getPool(189-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (19)
src/modules/memory.js (6)
1-26: LGTM — clean module structure with proper imports and graceful fallback design.ESM imports, Winston logging, config integration, and opt-out checks are all properly wired. The privacy notice in the module docstring is a good practice.
87-102: Config fallback looks correct now.The catch block properly defaults
enabled: falseandautoExtract: false, while the normal path defaults totrue— addressing the previous review concern.
111-125: Auto-recovery cooldown mechanism is well-designed.The cooldown-based recovery at Line 118 properly re-enables mem0 after
RECOVERY_COOLDOWN_MS, allowing the next operation to test connectivity. This addresses the prior concern about permanent disable on transient failures.
162-199: Health check now uses SDK search — much better than raw HTTP.Using
c.search('health-check', ...)with the SDK validates both authentication and connectivity in one call, addressing the prior concern about using undocumented endpoints.
410-443: No issue here —userMessagereceived byextractAndStoreMemoriesis raw text without any prefix.The message flow is: Discord message → remove bot mention (cleanContent) → pass raw text to
generateResponseasuserMessage→ pass same rawuserMessagetoextractAndStoreMemories(line 465 in ai.js). The function then prepends the username once at line 422 in memory.js. No duplication occurs.Likely an incorrect or invalid review comment.
246-279: mem0 SDK search method returns memory IDs reliably—no action needed.The mem0 SDK's
search()method returns memory IDs in all results (documented format:{"results":[{"id":"mem_123",...}]}). ThehandleForgetTopiccommand already includes a defensive filter (matches.filter((m) => m.id)) to ensure only results with IDs are processed, and the fallback mapping insearchMemoriesis a safe precaution rather than an actual concern.Likely an incorrect or invalid review comment.
src/commands/memory.js (5)
43-50: Good extraction offormatMemoryListhelper — addresses prior duplication concern.Shared truncation logic is now centralized. Both
handleViewandhandleAdminViewuse this helper consistently.
98-135:executecorrectly routes subcommands and checks availability.Opt-out is handled before the availability check (Line 111), which is correct since toggling opt-out shouldn't require mem0 to be up.
261-289: Parallel deletion withPromise.allSettled— good improvement.Using IDs directly from search results and
Promise.allSettledfor parallel deletion addresses both prior review concerns (fragile text matching and sequential deletes).One minor note: if
markUnavailable()is called insidedeleteMemoryon the first failure, subsequent parallel deletes will short-circuit viaisMemoryAvailable()returning false. ThedeletedCountwill undercount. This is acceptable since the system is genuinely degraded, but worth being aware of.
296-330: Admin commands should verify memory availability before fetching the target user.If
isMemoryAvailable()returns false (Line 312), the reply is sent without deferring. Butinteraction.options.getUser('user')at Line 321 is called after. If a user somehow passes the availability check but the target user option is invalid/null, Line 322 (targetUser.id) would throw. However, since the option is.setRequired(true), Discord enforces its presence. This is fine.
196-252: Confirmation buttoncustomIdvalues are static strings — safe due to ephemeral + user filter.The
memory_forget_confirm/memory_forget_cancelIDs don't include user-specific disambiguation, but thefilter: (i) => i.user.id === userId(Line 219) andephemeral: true(Line 213) prevent cross-user interference. This is fine for the current design.tests/modules/memory.test.js (2)
150-170: Auto-recovery test now properly awaits — addresses prior concern about unawaited promises.The test correctly uses
async/awaitwithvi.useFakeTimers()and restores withvi.useRealTimers().
1-791: Comprehensive test suite with good coverage of edge cases.Tests cover availability gating, SDK error handling, field variant mapping, opt-out, auto-recovery, and all CRUD operations. Well-structured with clear test names.
src/db.js (1)
162-168: Schema addition looks correct and well-placed.
CREATE TABLE IF NOT EXISTSis idempotent, theuser_id TEXT PRIMARY KEYmatches theON CONFLICT (user_id) DO NOTHINGupsert inoptout.js, andcreated_atprovides useful audit info.src/index.js (1)
293-298: Startup sequencing is correct — opt-outs load before mem0 health check.Both calls are non-fatal (they log warnings and return gracefully on failure), so the bot starts regardless. The order ensures opt-out state is available before any memory operations begin.
tests/modules/optout.test.js (1)
1-203: Thorough test coverage for opt-out module.Good coverage of toggle flows, DB persistence (success and failure), multi-user independence, and graceful degradation when DB is unavailable. Test isolation via
_resetOptouts()inbeforeEach/afterEachis solid.src/modules/optout.js (3)
28-35:resolvePoolpattern is clean — gracefully handles missing DB.Falls back from injected pool →
getPool()→null, keeping the module functional even without a database.
69-102: Best-effort DB persistence with in-memory-first updates — good design choice.In-memory state is always updated immediately, and DB persistence is a fire-and-forget operation that logs warnings on failure. This means the user gets instant feedback, and persistence is "best-effort." One trade-off: if the DB write fails and the bot restarts, the opt-out state is lost. This is acceptable given the design and documented behavior, but worth noting in operational docs.
108-124:loadOptOutsreplaces the full Set — safe since it's only called at startup.If this were called concurrently with
toggleOptOut, there'd be a race condition where a toggle could be overwritten. Since it's only invoked once during startup (before event handlers are registered), this is fine.
Not every API error should disable the entire memory system. Added isTransientError() helper that distinguishes: - Transient (retryable): network errors (ECONNREFUSED, ETIMEDOUT, etc.), 5xx server errors, 429 rate limits, timeout/fetch-failed messages - Permanent (disable system): 4xx client errors (401/403 auth failures), unknown errors Only permanent errors now call markUnavailable(). Transient errors log a warning and return safe defaults without disabling the system. Updated all 6 API operation catch blocks: addMemory, searchMemories, getMemories, deleteAllMemories, deleteMemory, extractAndStoreMemories. Added 8 tests covering error classification and behavior verification.
Explains why setting false does a hard disable without cooldown (test helper for instant state toggling) while setting true calls markAvailable() to clear cooldown. Production code uses markUnavailable() which enables timed auto-recovery.
Previously, vi.useRealTimers() was called at the end of each test using fake timers. If a test failed mid-way, the call would be skipped and fake timers would leak into subsequent tests. Moved to an afterEach block in the isMemoryAvailable describe block so it always runs.
Replaced fragile test that relied on the auto-created mock client lacking a search method. Now explicitly creates a mock client with a search method that throws ECONNREFUSED, clearly testing the scenario where a client is created but cannot reach the mem0 platform. Also asserts that search was actually called.
Deep Review — PR #59: feat: User Memory via mem0This review covers the final state of the code after all 8 commits. Issues from earlier CodeRabbit review rounds that were already addressed are noted separately at the bottom. All findings below were verified against the actual source on the Bugs1.
2. const lines = relations.map((r) => `- ${r.source} → ${r.relationship} → ${r.target}`);No guard for missing properties. A relation like 3.
4. Fire-and-forget extraction failure globally disables memory The extraction is intentionally fire-and-forget ( 5.
Design Issues6.
7. Circuit breaker doesn't distinguish error types All four call 8. When the cooldown expires, this "is" function calls 9.
10. Username baked into memory content instead of metadata { role: 'user', content: `${username}: ${userMessage}` }mem0 stores 11. No size budget for memory context in system prompt
12. 3 guaranteed strings ( 13. Once created, the client is cached permanently with no TTL or invalidation. If Test Quality14. The helper returns success for every SDK method by default. Tests that forget to override a specific method get an implicit pass, masking regressions if production code changes which methods it calls. Overlap with Existing CodeRabbit Review (already addressed in subsequent commits)The following were flagged by CodeRabbit in earlier review rounds and fixed:
Note: Issue #4 above (fire-and-forget — Generated by Claude Code |
2 participants
Summary
Integrates mem0 into the AI chat module so the bot remembers things about each Discord user across conversations and restarts. Uses the official
mem0aiSDK (v2.2.2) with the hosted mem0 platform and graph memory enabled for entity relationship tracking.When a user shares personal info like "I'm learning Rust" or "I work at Google", the bot stores it in mem0 scoped to their Discord user ID — and builds a knowledge graph of relationships between entities.
Changes
New Files
src/modules/memory.js— mem0 SDK client wrapper (mem0aiv2.2.2) with graph memory enabled on all operationssrc/commands/memory.js—/memory viewand/memory forget [topic]slash commandtests/modules/memory.test.js— 48 tests covering SDK methods, graph relations, error handling, graceful fallbacktests/commands/memory.test.js— 11 tests for the memory commandModified Files
src/modules/ai.js— Pre-response: searches mem0 for relevant memories + graph relations and injects into system prompt. Post-response: fire-and-forget memory extraction with graph building.src/modules/events.js— PassesuserId(Discord author ID) togenerateResponse()src/index.js— Runs mem0 health check on startupconfig.json— Addsmemoryconfig section.env.example— DocumentsMEM0_API_KEYenv var (hosted platform)package.json— Addedmem0aidependencyArchitecture
MEM0_API_KEYwith api.mem0.ai (not self-hosted)enable_graph: trueon all add/search/getAll calls. Relations returned as{ source, relationship, target }and formatted into context (e.g., "Joseph → works as → software engineer")user_id=Discord ID,app_id=bills-bot— isolates from other mem0 consumersmemory.{enabled, maxContextMemories, autoExtract, extractModel}Testing
Checklist
mem0aiSDK (v2.2.2)Closes #24