fix: address mem0 deep review issues from PR #59#63
Merged
BillChirico merged 33 commits intomainfrom Feb 16, 2026
Merged
Conversation
Contributor
|
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. 📥 CommitsReviewing files that changed from the base of the PR and between afbc7c74024a898fb85f2c4085da44615c9d9413 and ab04cc6. 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughImplemented resilience mechanisms for memory operations by introducing timeout protections, recovery workflows, and availability state management. Added auto-recovery logic with cooldown periods, batch deletion for memory cleanup, and safety checks during startup and AI response generation. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
|
Claude finished @BillChirico's task in 1m 19s —— View job Review Complete
All 9 fixes from the deep review are correctly implemented:
Previous review issues addressed:
Approved ✅ |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Deep Review: PR #63Stats: 7 files, +339/-60, 12 new tests (123 total pass ✅) Verdict8 of 9 fixes are complete and correct. One fix (#1: falsy ID filtering) is partially undermined by a pre-existing coercion in Fix Verification
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
BillChirico
dismissed stale reviews from claude[bot] and coderabbitai[bot]
via
9720cf4
BillChirico
dismissed stale reviews from claude[bot] and coderabbitai[bot]
via
afbc7c7
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- 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
- 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
…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
- 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
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.
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.
…ForgetTopic Empty string is falsy in JS, so `m.id` filter was silently dropping memories with falsy-but-valid IDs (e.g. numeric 0). Now uses explicit check: m.id !== undefined && m.id !== null && m.id !== ''
Relations with undefined or null properties were being rendered as 'undefined → works at → Google' in the system prompt. Now filters out incomplete relations before formatting.
A hanging mem0 call would block all AI responses indefinitely. Now uses Promise.race with a 5-second timeout so the bot continues responding even if memory lookup hangs.
…emories Background memory extraction failures were disabling the memory system for all users. Now only logs the error — background failures should not have system-wide side effects. Also passes username as metadata instead of only baking it into the content string.
A hanging mem0 health check would block bot startup indefinitely. Now uses Promise.race with a 10-second timeout — if it times out, logs a warning and continues without memory features.
…emory isMemoryAvailable() was a getter with write side effects (auto-recovery called markAvailable). Now: - isMemoryAvailable() — pure, no side effects, just reads state - checkAndRecoverMemory() — checks availability with cooldown-based auto-recovery (the side-effectful version) All internal memory operations and command handlers use checkAndRecoverMemory() for the recovery behavior.
Was hardcoded to search limit of 10, silently leaving remaining memories. Now searches in batches of 100 and loops until no more results, with a safety cap of 10 iterations (1000 memories max).
Memory context had no size limit — with many memories and relations, it could bloat the system prompt significantly. Now truncates with '...' if the context exceeds 2000 characters.
Username was being prepended to the user message content as 'username: message', polluting stored memories with the username string. Now passes username only as metadata, keeping memory content clean and username available for querying via metadata.
Change m.id || '' to m.id ?? '' in searchMemories and getMemories to preserve falsy-but-valid IDs like 0. The || operator coerces 0 to empty string before consumers see it; ?? only coerces null/undefined. Add tests verifying falsy ID preservation in both functions.
…covery When checkMem0Health times out during startup, the catch block now calls markUnavailable() to set mem0UnavailableSince to Date.now(). This enables checkAndRecoverMemory() to auto-recover after the cooldown period expires. Previously, the timeout left mem0Available=false with mem0UnavailableSince=0, which permanently disabled memory features since the recovery condition requires mem0UnavailableSince > 0. Changes: - Export markUnavailable() from src/modules/memory.js - Import and call markUnavailable() in index.js catch block - Add tests verifying markUnavailable is called on health check failure - Add tests verifying auto-recovery works after markUnavailable Resolves PR #63 review thread PRRT_kwDORICdSM5uwjHN
…ortSignal The Promise.race timeout in startup doesn't cancel the underlying checkMem0Health() call. If the health check takes longer than 10s but eventually succeeds, it calls markAvailable(), silently overriding the markUnavailable() set by the timeout catch block. Fix: pass an AbortController signal to checkMem0Health(). When the timeout fires, it aborts the signal. checkMem0Health checks signal.aborted before calling markAvailable(), preventing the late success from re-enabling memory features after startup logged them as disabled. Adds test verifying checkMem0Health returns false and does not mark available when signal is already aborted.
BillChirico
force-pushed
the
fix/mem0-review-fixes
branch
from
0f763f5 to
ab04cc6
Compare
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.
|
|
||
| const matchesWithIds = matches.filter( | ||
| (m) => m.id !== undefined && m.id !== null && m.id !== '', | ||
| ); |
There was a problem hiding this comment.
Filter null/undefined checks are dead code after upstream coercion
Low Severity
The handleForgetTopic filter checks m.id !== undefined && m.id !== null but these conditions can never trigger because searchMemories already coerces null/undefined IDs to '' via m.id ?? ''. The effective filter is just m.id !== ''. This makes the fix for falsy ID handling (fix #1) appear more robust than it actually is — the null/undefined guards are dead code. Either searchMemories should stop coercing IDs (use m.id directly) to let the downstream filter handle all cases, or the filter should be simplified to match what searchMemories actually produces.
Additional Locations (1)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 9 bugs and design issues identified in the deep review of PR #59.
Changes (one commit per fix)
Bugs Fixed
handleForgetTopicsilently drops results without IDs — empty string is falsy, som.idfilter dropped valid falsy IDs. Now uses explicitm.id !== undefined && m.id !== null && m.id !== ''check.formatRelationswritesundefinedinto system prompt — missingsource/relationship/targetproperties causedundefined → works at → Googlein prompts. Now filters incomplete relations.buildMemoryContexthas no timeout — hanging mem0 blocked all AI responses. Now wrapped withPromise.raceand a 5s timeout.extractAndStoreMemoriescallsmarkUnavailable— background failure for one user disabled memory for all. Now only logs errors, no system-wide side effects.checkMem0Healthblocks startup with no timeout — hanging health check prevented bot from starting. Now wrapped withPromise.raceand 10s timeout.Design Issues Fixed
isMemoryAvailable()was a getter with write side effects — renamed auto-recovery version tocheckAndRecoverMemory(), keptisMemoryAvailable()as a pure side-effect-free getter./memory forget topichardcoded limit of 10 — now loops with batch size 100, up to 10 iterations (1000 memories max), deleting all matches.Testing
Ref: PR #59 review comment