fix: resolve 6 issues — performance, scheduling, TTL, plugin#78
Merged
fix: resolve 6 issues — performance, scheduling, TTL, plugin#78
Conversation
…30 #36 #63 #75) Performance: - auto-forget.ts (#29): concept-bucketed contradiction detection with pre-tokenized cache, O(n * avg_bucket) instead of O(n^2) - consolidate.ts (#28): cap LLM calls at 10, process largest concept groups first - vector-index.ts (#30): bounded min-heap search replaces full sort, early skip for low-scoring entries Scheduling (#36): - index.ts: setInterval auto-forget (1h default) and consolidation (2h, opt-in via CONSOLIDATION_ENABLED=true) - Configurable via AUTO_FORGET_INTERVAL_MS, CONSOLIDATION_INTERVAL_MS - Timers use .unref() to not prevent process exit Memory TTL (#63): - remember.ts: optional ttlDays parameter auto-sets forgetAfter Plugin (#75): - plugin.json: version 0.2.0 -> 0.6.0 Closes #28 #29 #30 #36 #63 #75
📝 WalkthroughWalkthroughThis pull request bumps the plugin version from 0.2.0 to 0.6.0 and introduces several functional improvements: a token-based similarity algorithm for memory contradiction detection, rate-limiting for LLM consolidation calls, TTL support for memories, automatic periodic timers for auto-forget and consolidation, and optimized vector search using minScore tracking. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (5)
src/index.ts (2)
297-305: Same suggestion: log consolidation errors.♻️ Proposed improvement
const consolidationTimer = setInterval(async () => { try { await sdk.trigger("mem::consolidate-pipeline", {}); - } catch {} + } catch (err) { + console.warn("[agentmemory] Consolidation error:", err); + } }, consolidationIntervalMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 297 - 305, The consolidation interval swallowes errors silently; update the setInterval handler (where process.env.CONSOLIDATION_ENABLED is checked and consolidationTimer is created) to catch and log any exception from sdk.trigger("mem::consolidate-pipeline", {}) (use console.error or the existing logger) with a clear context message like "Auto-consolidation failed" and include the error object/details so failures are visible for debugging.
288-295: Consider logging errors from scheduled tasks.The empty
catch {}blocks silently swallow all errors, making it difficult to diagnose issues in production. Even with non-fatal error handling, logging the error provides valuable debugging information.♻️ Proposed improvement
const autoForgetTimer = setInterval(async () => { try { await sdk.trigger("mem::auto-forget", { dryRun: false }); - } catch {} + } catch (err) { + console.warn("[agentmemory] Auto-forget error:", err); + } }, autoForgetIntervalMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 288 - 295, The scheduled task for auto-forget currently swallows all errors in an empty catch; change the catch for the setInterval callback that calls sdk.trigger("mem::auto-forget") (related to autoForgetTimer and autoForgetIntervalMs) to capture the thrown error and log it (e.g., console.error or the existing logger) with context including autoForgetIntervalMs and the "mem::auto-forget" operation so failures are visible while preserving non-fatal behavior.src/state/vector-index.ts (1)
54-58: Optional: Consider binary heap for better worst-case performance.The current approach re-sorts the entire buffer on each replacement, which is O(k log k) per replacement. For large datasets with many high-scoring items, a binary min-heap would reduce this to O(log k) per replacement.
For the default limit of 20, the current implementation is acceptable, but if larger limits are anticipated, a heap-based approach would scale better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/vector-index.ts` around lines 54 - 58, The current replacement logic for the top-k buffer (using results, minScore, score, obsId and entry.sessionId) re-sorts the results array on each insertion which is inefficient for larger k; replace the sort-based approach with a binary min-heap (or a standard library priority queue) that stores entries by score, push new items and if heap size > limit pop the min, and update minScore from heap.peek() so each replacement becomes O(log k) instead of re-sorting the entire results array.src/functions/auto-forget.ts (1)
4-4: Unused import:jaccardSimilarity.The
jaccardSimilarityfunction is imported but no longer used after switching to the token-based similarity calculation. Consider removing this unused import.♻️ Suggested fix
-import { KV, jaccardSimilarity } from "../state/schema.js"; +import { KV } from "../state/schema.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/auto-forget.ts` at line 4, Remove the unused jaccardSimilarity import from the import statement so only KV is imported (update the import line that currently reads "import { KV, jaccardSimilarity } from \"../state/schema.js\";" to only import KV), and verify there are no remaining references to jaccardSimilarity elsewhere in the file (delete any leftover references if found).src/functions/remember.ts (1)
83-85: Consider using a named constant for the magic number.The value
86400000(milliseconds per day) is already defined asMS_PER_DAYinsrc/functions/auto-forget.ts. Consider extracting this constant to a shared location or importing it for consistency.♻️ Suggested improvement
+const MS_PER_DAY = 24 * 60 * 60 * 1000; + // ... in the function body: if (data.ttlDays && typeof data.ttlDays === "number" && data.ttlDays > 0) { - memory.forgetAfter = new Date(Date.now() + data.ttlDays * 86400000).toISOString(); + memory.forgetAfter = new Date(Date.now() + data.ttlDays * MS_PER_DAY).toISOString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/remember.ts` around lines 83 - 85, Replace the magic number 86400000 used to compute memory.forgetAfter in the remember handler (the block that checks data.ttlDays and sets memory.forgetAfter) with the shared MS_PER_DAY constant; either import MS_PER_DAY from the module that defines it (auto-forget's export) or move MS_PER_DAY to a small shared constants module and import it into both remember.ts and auto-forget.ts, then use MS_PER_DAY in the Date calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 284-285: The environment-parsed intervals (autoForgetIntervalMs
and consolidationIntervalMs) use parseInt directly and may become NaN, which
breaks setInterval; validate the parsed values after parseInt (e.g., use
Number.isFinite or !Number.isNaN) and ensure they are positive integers, falling
back to the safe defaults 3600000 and 7200000 (or clamp to a minimum like
1000ms) before passing to setInterval; update the code that assigns
autoForgetIntervalMs and consolidationIntervalMs so invalid or non-positive
results are replaced with the default constants and document that these
validated variables are what setInterval receives.
---
Nitpick comments:
In `@src/functions/auto-forget.ts`:
- Line 4: Remove the unused jaccardSimilarity import from the import statement
so only KV is imported (update the import line that currently reads "import {
KV, jaccardSimilarity } from \"../state/schema.js\";" to only import KV), and
verify there are no remaining references to jaccardSimilarity elsewhere in the
file (delete any leftover references if found).
In `@src/functions/remember.ts`:
- Around line 83-85: Replace the magic number 86400000 used to compute
memory.forgetAfter in the remember handler (the block that checks data.ttlDays
and sets memory.forgetAfter) with the shared MS_PER_DAY constant; either import
MS_PER_DAY from the module that defines it (auto-forget's export) or move
MS_PER_DAY to a small shared constants module and import it into both
remember.ts and auto-forget.ts, then use MS_PER_DAY in the Date calculation.
In `@src/index.ts`:
- Around line 297-305: The consolidation interval swallowes errors silently;
update the setInterval handler (where process.env.CONSOLIDATION_ENABLED is
checked and consolidationTimer is created) to catch and log any exception from
sdk.trigger("mem::consolidate-pipeline", {}) (use console.error or the existing
logger) with a clear context message like "Auto-consolidation failed" and
include the error object/details so failures are visible for debugging.
- Around line 288-295: The scheduled task for auto-forget currently swallows all
errors in an empty catch; change the catch for the setInterval callback that
calls sdk.trigger("mem::auto-forget") (related to autoForgetTimer and
autoForgetIntervalMs) to capture the thrown error and log it (e.g.,
console.error or the existing logger) with context including
autoForgetIntervalMs and the "mem::auto-forget" operation so failures are
visible while preserving non-fatal behavior.
In `@src/state/vector-index.ts`:
- Around line 54-58: The current replacement logic for the top-k buffer (using
results, minScore, score, obsId and entry.sessionId) re-sorts the results array
on each insertion which is inefficient for larger k; replace the sort-based
approach with a binary min-heap (or a standard library priority queue) that
stores entries by score, push new items and if heap size > limit pop the min,
and update minScore from heap.peek() so each replacement becomes O(log k)
instead of re-sorting the entire results array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3356b648-879c-4101-b808-e30976363679
📒 Files selected for processing (6)
plugin/.claude-plugin/plugin.jsonsrc/functions/auto-forget.tssrc/functions/consolidate.tssrc/functions/remember.tssrc/index.tssrc/state/vector-index.ts
Comment on lines
+284
to
+285
| const autoForgetIntervalMs = parseInt(process.env.AUTO_FORGET_INTERVAL_MS || "3600000", 10); | ||
| const consolidationIntervalMs = parseInt(process.env.CONSOLIDATION_INTERVAL_MS || "7200000", 10); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify behavior of setInterval with NaN in Node.js
node -e "
const t = setInterval(() => {
console.log('fired at', Date.now());
clearInterval(t);
}, NaN);
console.log('interval set with NaN');
"Repository: rohitg00/agentmemory
Length of output: 277
🏁 Script executed:
# Check the actual code at lines 284-295 in src/index.ts
sed -n '284,295p' src/index.tsRepository: rohitg00/agentmemory
Length of output: 620
🏁 Script executed:
# Check if there's any validation of these environment variables elsewhere
rg "AUTO_FORGET_INTERVAL_MS|CONSOLIDATION_INTERVAL_MS" -A 2 -B 2Repository: rohitg00/agentmemory
Length of output: 396
🏁 Script executed:
# Check for any input validation patterns in the file
rg "isNaN|Number\.is" src/index.tsRepository: rohitg00/agentmemory
Length of output: 46
Guard against invalid interval values.
If an environment variable contains a non-numeric value, parseInt returns NaN. When passed to setInterval, Node.js issues a TimeoutNaNWarning and coerces the timeout to 1ms, causing the callback to fire immediately and repeatedly, which could overload the system.
🛡️ Proposed fix
-const autoForgetIntervalMs = parseInt(process.env.AUTO_FORGET_INTERVAL_MS || "3600000", 10);
-const consolidationIntervalMs = parseInt(process.env.CONSOLIDATION_INTERVAL_MS || "7200000", 10);
+const autoForgetIntervalMs = parseInt(process.env.AUTO_FORGET_INTERVAL_MS || "3600000", 10) || 3600000;
+const consolidationIntervalMs = parseInt(process.env.CONSOLIDATION_INTERVAL_MS || "7200000", 10) || 7200000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const autoForgetIntervalMs = parseInt(process.env.AUTO_FORGET_INTERVAL_MS || "3600000", 10); | |
| const consolidationIntervalMs = parseInt(process.env.CONSOLIDATION_INTERVAL_MS || "7200000", 10); | |
| const autoForgetIntervalMs = parseInt(process.env.AUTO_FORGET_INTERVAL_MS || "3600000", 10) || 3600000; | |
| const consolidationIntervalMs = parseInt(process.env.CONSOLIDATION_INTERVAL_MS || "7200000", 10) || 7200000; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 284 - 285, The environment-parsed intervals
(autoForgetIntervalMs and consolidationIntervalMs) use parseInt directly and may
become NaN, which breaks setInterval; validate the parsed values after parseInt
(e.g., use Number.isFinite or !Number.isNaN) and ensure they are positive
integers, falling back to the safe defaults 3600000 and 7200000 (or clamp to a
minimum like 1000ms) before passing to setInterval; update the code that assigns
autoForgetIntervalMs and consolidationIntervalMs so invalid or non-positive
results are replaced with the default constants and document that these
validated variables are what setInterval receives.
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
1 participant
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
Resolves 6 open issues covering performance, scheduling, TTL, and plugin fixes.
Test plan
AUTO_FORGET_ENABLED=truemem::rememberwithttlDays: 7setsforgetAfterCloses #28 #29 #30 #36 #63 #75
Summary by CodeRabbit
New Features
Chores