fix(memory): improve agent memory reliability#1892
Conversation
📝 WalkthroughWalkthroughThis PR adds per-turn memory injection access accounting in the runtime presenter, a keyword-stats TTL cache in retrieval invalidated via a new agent-mutation callback, structured call/failure metrics for maintenance passes with a consolidation failure cooldown, revised provenance-hit and extraction failure semantics, and a ChangesMemory Injection Access Accounting
Keyword Stats Caching
Maintenance Failure Cooldown
Write Coordinator Semantics
Audit memory_ref_id and Archive Filter
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRuntimePresenter
participant MemoryRuntimePort
participant MemoryPresenter
participant Repository
AgentRuntimePresenter->>AgentRuntimePresenter: assemble system prompt, select memories for turn
AgentRuntimePresenter->>MemoryRuntimePort: recordInjectionAccess(agentId, memoryIds, accessedAt)
MemoryRuntimePort->>MemoryPresenter: recordInjectionAccess(agentId, memoryIds, accessedAt)
MemoryPresenter->>MemoryPresenter: check canReadAgentMemory, dedupe IDs
MemoryPresenter->>Repository: listByIds(memoryIds)
Repository-->>MemoryPresenter: owned rows
MemoryPresenter->>Repository: recordAccessBatch(ownedIds, accessedAt)
sequenceDiagram
participant MaintenanceService
participant ReflectionService
participant PersonaService
participant ConflictService
participant AuditLog
MaintenanceService->>MaintenanceService: check lastConsolidationFailureAt cooldown
alt within cooldown
MaintenanceService->>MaintenanceService: run cheap maintenance only
else not in cooldown
MaintenanceService->>ConflictService: runChallengeResolutionPass()
ConflictService-->>MaintenanceService: {touched, calls, failures}
MaintenanceService->>ReflectionService: runMaintenanceReflectionPass()
ReflectionService-->>MaintenanceService: {result, calls, failures}
MaintenanceService->>PersonaService: runMaintenancePersonaPass()
PersonaService-->>MaintenanceService: {result, calls, failures}
MaintenanceService->>MaintenanceService: didAllAttemptedLlmCallsFail()
alt all LLM calls failed
MaintenanceService->>MaintenanceService: stamp lastConsolidationFailureAt, rollback
MaintenanceService->>AuditLog: write memory/maintenance_llm failed
else success
MaintenanceService->>AuditLog: write memory/maintenance_llm completed
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/memoryPresenter/services/conflictService.ts (1)
160-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
runChallengeResolutionPasscan silently drop partial call/failure stats on exception.Unlike
runMaintenanceReflectionPass/runMaintenancePersonaPass, which wrap their entire body in try/catch to always return a structured result, here only thegenerateTextcall is guarded. IflistConflictsorresolveConflict(DB transaction) throws mid-loop, the function rejects instead of returning{touched, calls, failures}. The caller inmaintenanceService.tsthen skipsaddLlmStatsfor this step entirely, so any calls/failures already accumulated are lost from the aggregate used bydidAllAttemptedLlmCallsFail— undermining the new failure-cooldown's accuracy and potentially letting the whole consolidation pass be marked "completed" despite an actual error.🛡️ Proposed fix: guard the whole pass like reflection/persona services
async runChallengeResolutionPass( agentId: string, model: MemoryModelRef ): Promise<MemoryMaintenanceStepResult> { let touched = false let calls = 0 let failures = 0 - for (const pair of this.listConflicts(agentId)) { - const promptCandidate = normalizeMemoryCandidate({ - kind: pair.challenger.kind === 'episodic' ? 'episodic' : 'semantic', - category: pair.challenger.category, - content: pair.challenger.content, - importance: pair.challenger.importance - }) - if (!promptCandidate) continue - const prompt = buildDecisionPrompt(promptCandidate, [{ content: pair.target.content }]) - let decision: MemoryDecision = ADD_DECISION - try { - calls += 1 - const raw = await this.ctx.deps.generateText(model.providerId, model.modelId, prompt) - decision = parseDecision(raw, 1) - } catch (error) { - failures += 1 - logger.warn(`[Memory] challenge decision failed: ${String(error)}`) - continue - } - if (!this.ctx.canWriteAgentMemory(agentId)) break - const outcome: MemoryConflictResolution = - decision.decision === 'NOOP' - ? 'keep_target' - : decision.decision === 'UPDATE' || decision.decision === 'SUPERSEDE' - ? 'keep_challenger' - : 'keep_both' - const mergedContent = - decision.decision === 'UPDATE' || decision.decision === 'SUPERSEDE' - ? decision.mergedContent - : null - if ( - await this.resolveConflict(agentId, pair.challenger.id, outcome, 'scheduler', model, { - mergedContent - }) - ) { - touched = true - } + try { + for (const pair of this.listConflicts(agentId)) { + const promptCandidate = normalizeMemoryCandidate({ + kind: pair.challenger.kind === 'episodic' ? 'episodic' : 'semantic', + category: pair.challenger.category, + content: pair.challenger.content, + importance: pair.challenger.importance + }) + if (!promptCandidate) continue + const prompt = buildDecisionPrompt(promptCandidate, [{ content: pair.target.content }]) + let decision: MemoryDecision = ADD_DECISION + try { + calls += 1 + const raw = await this.ctx.deps.generateText(model.providerId, model.modelId, prompt) + decision = parseDecision(raw, 1) + } catch (error) { + failures += 1 + logger.warn(`[Memory] challenge decision failed: ${String(error)}`) + continue + } + if (!this.ctx.canWriteAgentMemory(agentId)) break + const outcome: MemoryConflictResolution = + decision.decision === 'NOOP' + ? 'keep_target' + : decision.decision === 'UPDATE' || decision.decision === 'SUPERSEDE' + ? 'keep_challenger' + : 'keep_both' + const mergedContent = + decision.decision === 'UPDATE' || decision.decision === 'SUPERSEDE' + ? decision.mergedContent + : null + if ( + await this.resolveConflict(agentId, pair.challenger.id, outcome, 'scheduler', model, { + mergedContent + }) + ) { + touched = true + } + } + } catch (error) { + logger.warn(`[Memory] challenge resolution pass aborted for ${agentId}: ${String(error)}`) } return { touched, calls, failures } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/memoryPresenter/services/conflictService.ts` around lines 160 - 206, The issue is that runChallengeResolutionPass only catches errors around generateText, so exceptions from listConflicts or resolveConflict can abort the pass and lose the accumulated calls/failures result. Update conflictService.ts so runChallengeResolutionPass is wrapped in the same kind of top-level try/catch used by runMaintenanceReflectionPass and runMaintenancePersonaPass, and always return a structured MemoryMaintenanceStepResult with the partial counts even when an unexpected error occurs. Keep the existing per-call try/catch for generateText, but ensure any failure in the full loop still logs and resolves to { touched, calls, failures }.
🧹 Nitpick comments (5)
test/main/presenter/fakes/memoryFakes.ts (1)
786-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake's
memory_ref_idderivation diverges from the realderiveMemoryRefId/extractSingleMemoryRefcontract for empty/whitespacememoryId.The real implementation (
agentMemoryAudit.ts) trims the value and treats an empty/whitespace-only string as absent, falling through toinputRefs. This fake only checkstypeof === 'string', so an empty-stringoutputRefs.memoryIdwould setmemory_ref_idto''instead of falling back toinputRefs.memoryId(ornull), unlike production behavior. Low risk today, but it silently diverges from the upstream contract this fake is meant to mirror.🔧 Suggested fix to mirror the real trimming/fallback behavior
+ memory_ref_id: (() => { + const output = typeof input.outputRefs?.memoryId === 'string' ? input.outputRefs.memoryId.trim() : '' + if (output) return output + const inputRef = typeof input.inputRefs?.memoryId === 'string' ? input.inputRefs.memoryId.trim() : '' + return inputRef || null + })(), - memory_ref_id: - typeof input.outputRefs?.memoryId === 'string' - ? input.outputRefs.memoryId - : typeof input.inputRefs?.memoryId === 'string' - ? input.inputRefs.memoryId - : null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/fakes/memoryFakes.ts` around lines 786 - 791, The `memory_ref_id` fallback logic in `memoryFakes.ts` does not match the real `deriveMemoryRefId`/`extractSingleMemoryRef` behavior for empty or whitespace-only `memoryId` values. Update the fake’s `memory_ref_id` derivation to mirror the production contract by trimming `outputRefs.memoryId` and `inputRefs.memoryId`, treating blank strings as absent, and only selecting a value when the trimmed result is non-empty before falling back to the next source or `null`.src/main/presenter/sqlitePresenter/tables/agentMemory.ts (1)
1220-1232: 🧹 Nitpick | 🔵 Trivial
access_count = 0filter and docstring update are consistent withlistArchiveCandidateLifecycleRows/countArchiveCandidates.Worth flagging as an operational consideration: with the PR's new per-turn injection access accounting,
access_countnow increments whenever a memory is selected for injection (not just direct recall), so any memory that's ever been surfaced once will permanently fail thisaccess_count = 0filter and become non-archivable regardless of subsequent staleness/decay. This is likely intentional (never archive something that was actually used), but combined with a long-lived agent this could let the working set grow unbounded over time since a large share of memories will accumulate at least one injection hit. Worth confirming this tradeoff is intended and that there's a separate reclamation path (e.g., decay-based demotion) for "accessed-once-then-stale" memories.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/sqlitePresenter/tables/agentMemory.ts` around lines 1220 - 1232, The archive-candidate selection in listArchiveCandidates now excludes any row with access_count > 0, which means the new per-turn injection access accounting can make once-used memories permanently ineligible for archiving. Review whether this is the intended policy; if not, relax the access_count = 0 constraint or add a separate age/decay-based path so “accessed-once-then-stale” memories can still be reclaimed while keeping the behavior in listArchiveCandidateLifecycleRows and countArchiveCandidates consistent.src/main/presenter/memoryPresenter/services/maintenanceService.ts (1)
422-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate counters in
mergeNearDuplicates.Local
calls/touchedmirrorresult.calls/result.touched, always updated together (e.g. lines 473-474, 518-519). The finalresult.touched = result.touched || touchedat line 536 is a no-op since both are already equal. Consider dropping the local shadows and usingresult.calls/result.toucheddirectly for the loop-bound check and mutations.Also applies to: 473-474, 518-519, 536-536
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/memoryPresenter/services/maintenanceService.ts` around lines 422 - 427, In mergeNearDuplicates, remove the redundant local calls and touched counters that shadow result.calls and result.touched. Update the loop-bound check and all mutations to use result.calls and result.touched directly, and delete the final result.touched = result.touched || touched assignment since it is redundant. Keep the existing logic around merged, inputTokens, and lastScanned unchanged.test/main/presenter/memorySessionExtractionLock.test.ts (1)
121-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest exercises a local reimplementation, not the production cursor logic.
consumeSpanhere is a standalone closure that reimplements the "advance only onok:true" rule; it doesn't call into the actual extraction/cursor code (e.g.,runMemoryExtractioninagentRuntimePresenter/index.ts). A regression in the real implementation wouldn't be caught by this test. The equivalent behavior is already covered against real code inagentRuntimePresenter.test.ts("keeps the cursor unchanged when extraction returns ok:false").If this file has infrastructure to invoke the real per-session lock/extraction path, prefer wiring this assertion to it instead of a local reimplementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/memorySessionExtractionLock.test.ts` around lines 121 - 134, This test is only checking a local `consumeSpan` closure instead of the real extraction/cursor path. Replace the standalone reimplementation with an assertion against the production flow, ideally through `runMemoryExtraction` or the existing per-session lock/extraction helper used by `agentRuntimePresenter/index.ts`, so the test verifies the actual cursor advancement behavior on `ok:true` and `ok:false` results.src/main/presenter/agentRuntimePresenter/index.ts (1)
2466-2537: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-session prune/clear scans the entire flat map, not just the session's entries.
memoryInjectionAccessByTurnis a single flatMapfor all sessions, keyed bysessionId\u0000messageId. BothpruneMemoryInjectionAccessForSession(called up to twice perrecordMemoryInjectionAccessinvocation) andclearMemoryInjectionAccessForSessioniterate every entry in the map and filter by string-prefix match, so cost scales with the total number of tracked turns across all active sessions (up to 128 × concurrent-session-count), not just the current session. With many concurrent memory-enabled sessions this becomes a per-turn O(N) hot-path cost.Consider nesting by session (
Map<sessionId, Map<messageId, entry>>) so prune/clear only touch the current session's sub-map.♻️ Sketch of nested-map refactor
- private readonly memoryInjectionAccessByTurn = new Map<string, MemoryInjectionAccessTurnEntry>() + private readonly memoryInjectionAccessBySession = new Map< + string, + Map<string, MemoryInjectionAccessTurnEntry> + >()Then
pruneMemoryInjectionAccessForSession/clearMemoryInjectionAccessForSessionoperate only onmemoryInjectionAccessBySession.get(sessionId).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 2466 - 2537, The per-session cleanup in recordMemoryInjectionAccess is scanning the entire flat memoryInjectionAccessByTurn map, which makes pruneMemoryInjectionAccessForSession and clearMemoryInjectionAccessForSession scale with all sessions instead of just one. Refactor the storage in AgentRuntimePresenter to a nested Map keyed by sessionId, then messageId, and update memoryInjectionAccessKey usage so pruneMemoryInjectionAccessForSession and clearMemoryInjectionAccessForSession only iterate the current session’s sub-map. Keep the existing dedupe/tracking behavior in recordMemoryInjectionAccess and the touchedAt/TTL logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/presenter/memoryPresenter/services/conflictService.ts`:
- Around line 160-206: The issue is that runChallengeResolutionPass only catches
errors around generateText, so exceptions from listConflicts or resolveConflict
can abort the pass and lose the accumulated calls/failures result. Update
conflictService.ts so runChallengeResolutionPass is wrapped in the same kind of
top-level try/catch used by runMaintenanceReflectionPass and
runMaintenancePersonaPass, and always return a structured
MemoryMaintenanceStepResult with the partial counts even when an unexpected
error occurs. Keep the existing per-call try/catch for generateText, but ensure
any failure in the full loop still logs and resolves to { touched, calls,
failures }.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 2466-2537: The per-session cleanup in recordMemoryInjectionAccess
is scanning the entire flat memoryInjectionAccessByTurn map, which makes
pruneMemoryInjectionAccessForSession and clearMemoryInjectionAccessForSession
scale with all sessions instead of just one. Refactor the storage in
AgentRuntimePresenter to a nested Map keyed by sessionId, then messageId, and
update memoryInjectionAccessKey usage so pruneMemoryInjectionAccessForSession
and clearMemoryInjectionAccessForSession only iterate the current session’s
sub-map. Keep the existing dedupe/tracking behavior in
recordMemoryInjectionAccess and the touchedAt/TTL logic unchanged.
In `@src/main/presenter/memoryPresenter/services/maintenanceService.ts`:
- Around line 422-427: In mergeNearDuplicates, remove the redundant local calls
and touched counters that shadow result.calls and result.touched. Update the
loop-bound check and all mutations to use result.calls and result.touched
directly, and delete the final result.touched = result.touched || touched
assignment since it is redundant. Keep the existing logic around merged,
inputTokens, and lastScanned unchanged.
In `@src/main/presenter/sqlitePresenter/tables/agentMemory.ts`:
- Around line 1220-1232: The archive-candidate selection in
listArchiveCandidates now excludes any row with access_count > 0, which means
the new per-turn injection access accounting can make once-used memories
permanently ineligible for archiving. Review whether this is the intended
policy; if not, relax the access_count = 0 constraint or add a separate
age/decay-based path so “accessed-once-then-stale” memories can still be
reclaimed while keeping the behavior in listArchiveCandidateLifecycleRows and
countArchiveCandidates consistent.
In `@test/main/presenter/fakes/memoryFakes.ts`:
- Around line 786-791: The `memory_ref_id` fallback logic in `memoryFakes.ts`
does not match the real `deriveMemoryRefId`/`extractSingleMemoryRef` behavior
for empty or whitespace-only `memoryId` values. Update the fake’s
`memory_ref_id` derivation to mirror the production contract by trimming
`outputRefs.memoryId` and `inputRefs.memoryId`, treating blank strings as
absent, and only selecting a value when the trimmed result is non-empty before
falling back to the next source or `null`.
In `@test/main/presenter/memorySessionExtractionLock.test.ts`:
- Around line 121-134: This test is only checking a local `consumeSpan` closure
instead of the real extraction/cursor path. Replace the standalone
reimplementation with an assertion against the production flow, ideally through
`runMemoryExtraction` or the existing per-session lock/extraction helper used by
`agentRuntimePresenter/index.ts`, so the test verifies the actual cursor
advancement behavior on `ok:true` and `ok:false` results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3be57d80-eec6-405a-acb7-df5e0f9ba500
📒 Files selected for processing (23)
src/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/memoryPresenter/context.tssrc/main/presenter/memoryPresenter/core/injectionPort.tssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/runtimeConstants.tssrc/main/presenter/memoryPresenter/services/conflictService.tssrc/main/presenter/memoryPresenter/services/maintenanceService.tssrc/main/presenter/memoryPresenter/services/personaService.tssrc/main/presenter/memoryPresenter/services/reflectionService.tssrc/main/presenter/memoryPresenter/services/retrievalService.tssrc/main/presenter/memoryPresenter/services/writeCoordinator.tssrc/main/presenter/memoryPresenter/types.tssrc/main/presenter/sqlitePresenter/schemaCatalog.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tssrc/main/presenter/sqlitePresenter/tables/agentMemoryAudit.tstest/main/presenter/agentMemoryTable.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memoryAdd.test.tstest/main/presenter/memoryExtraction.test.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memorySessionExtractionLock.test.ts
1 participant
Summary by CodeRabbit
New Features
Bug Fixes