feat(memory): evolve agent memory architecture - #2036
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a large "Agent Memory Evolution" architecture change: temporal claim metadata, scope-aware applicability, exact-forgetting via tombstones, durable derivation lineage with bounded dirty-work consolidation, a directive plane (create/approve/reject/delete standing directives), and a budgeted context allocator. Changes span storage schema/migrations, memory services, DeepChat agent runtime integration, renderer UI, i18n, and extensive tests. ChangesAgent Memory Evolution
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
- validate batched decisions through scope-aware revalidation - model an active winner in sibling retirement performance coverage - refresh the renderer boundary baseline for directive notifications
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/contracts/routes/memory.routes.ts (1)
121-144: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the temporal metadata invariants in response DTOs.
These schemas accept atemporal rows with temporal fields, non-atemporal rows without
temporalConfidence, and reversed intervals. That violates the documented shared contract and can expose malformed temporal state to management and retrieval clients.Proposed fix
+function enforceProjectedTemporalInvariant( + value: { + temporalKind: (typeof AGENT_MEMORY_TEMPORAL_KINDS)[number] + validFrom: number | null + validUntil: number | null + temporalConfidence: number | null + temporalPrecision: (typeof AGENT_MEMORY_TEMPORAL_PRECISIONS)[number] | null + temporalTimeZone: string | null + }, + context: z.RefinementCtx +): void { + if (value.temporalKind === 'atemporal') { + if ( + value.validFrom !== null || + value.validUntil !== null || + value.temporalConfidence !== null || + value.temporalPrecision !== null || + value.temporalTimeZone !== null + ) { + context.addIssue({ code: 'custom', message: 'atemporal memories cannot carry temporal metadata' }) + } + } else if ( + value.temporalConfidence === null || + (value.validFrom !== null && value.validUntil !== null && value.validFrom >= value.validUntil) + ) { + context.addIssue({ code: 'custom', message: 'invalid temporal metadata' }) + } +}🤖 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/shared/contracts/routes/memory.routes.ts` around lines 121 - 144, Update enforceProjectedScopeInvariant, which is applied to both MemoryItemSchema and MemorySearchResultSchema, to validate temporal metadata: atemporal rows must not carry temporal fields, non-atemporal rows must provide temporalConfidence, and any populated validFrom/validUntil interval must not be reversed. Preserve the existing projected-scope checks while ensuring both response DTOs reject these malformed states.
🧹 Nitpick comments (23)
src/renderer/settings/components/MemoryInboxBar.vue (1)
322-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared directive approve/reject state machine into a composable.
setDirectivePending/approveDirective/rejectDirectivehere duplicatesetPending/transitioninMemoryDirectivesPanel.vue(same revision-guard, pending-set, rejected-reason, reload-on-error pattern). A shareduseDirectiveActions()composable would keep the two panels from drifting.🤖 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/renderer/settings/components/MemoryInboxBar.vue` around lines 322 - 385, Extract the shared directive action state machine from setDirectivePending, approveDirective, and rejectDirective into a useDirectiveActions composable, then update MemoryInboxBar.vue and MemoryDirectivesPanel.vue to use it. Preserve the existing revision guard, pending-ID tracking, rejected-reason notification, stale-agent protection, and reload-on-error behavior in both panels.src/renderer/src/components/chat/MemoryTurnDialog.vue (1)
94-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffPrefer an interpolated message over label + value concatenation.
{{ t('...overhead') }} {{ n }} · {{ t('...unused') }} {{ n }}hardcodes ordering and the·separator in the template, which is awkward for RTL locales (fa-IR,he-IL) and for languages that place the value first. A single key such ast('chat.memory.turn.overheadSummary', { overhead, unused })would let each locale control layout.🤖 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/renderer/src/components/chat/MemoryTurnDialog.vue` around lines 94 - 97, Replace the label-and-value concatenation in the MemoryTurnDialog template with one interpolated translation using a key such as chat.memory.turn.overheadSummary and named overhead and unused values. Add the corresponding locale message entries so translators control ordering and separators, while preserving the displayed token values.Source: Coding guidelines
src/main/memory/core/workingProjection.ts (2)
216-229: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant temporal recomputation.
resolveNextRefreshAtre-derivestemporalMetadataFromRowfor every row althoughtoCandidatealready computed it; pass the computed metadata through instead.🤖 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/memory/core/workingProjection.ts` around lines 216 - 229, Update resolveNextRefreshAt to accept and use the temporal metadata already computed by toCandidate, rather than calling temporalMetadataFromRow for each row. Adjust the caller and input shape as needed while preserving selection of the earliest future validFrom or validUntil boundary.
269-288: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winQuadratic re-render during budget selection.
Each accepted/rejected candidate re-renders the entire projection (
renderSections(next)) and re-runsestimateTokensover the full string, so cost grows as O(N²) in candidate count and string size. With a section-label overhead table you can track the incremental token cost per line instead, and only render once at the end.🤖 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/memory/core/workingProjection.ts` around lines 269 - 288, The budget-selection loop in working projection repeatedly calls renderSections and estimateTokens for every candidate, causing quadratic work. Replace the per-candidate full rendering in the selection logic with incremental token accounting using the section-label overhead table, accept candidates only when their incremental line cost stays within budget, and render the final selected projection once after selection.src/main/memory/core/scoring.ts (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicated
../domain/typestype imports. Two separateimport typestatements target the same module.🤖 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/memory/core/scoring.ts` around lines 20 - 29, Merge the two type-only imports from ../domain/types in scoring.ts into a single import declaration containing MemoryScope, AgentMemoryKind, MemoryTemporalMetadata, and MemoryTemporalPolicyResult; leave all other imports unchanged.src/main/memory/domain/directives.ts (2)
128-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead guard.
assertBoundedTextalready throws on empty input, so line 134 can never trigger.🤖 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/memory/domain/directives.ts` around lines 128 - 135, Remove the redundant empty-topic guard after the `normalizedTopic = assertBoundedText(...)` call in the directive normalization flow. Keep the existing `assertBoundedText` validation and normalization behavior unchanged.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
node:protocol for consistency.
src/main/memory/core/tombstone.tsimportsnode:crypto; this module uses the bare specifier.♻️ Proposed change
-import { createHash } from 'crypto' +import { createHash } from 'node:crypto'🤖 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/memory/domain/directives.ts` at line 1, Update the crypto import in the directives module to use the `node:crypto` specifier, matching the convention used by the nearby tombstone module and preserving the existing createHash usage.src/main/memory/core/temporal.ts (1)
376-388: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDouble normalization on the retrieval hot path.
Callers such as
fuseinsrc/main/memory/core/scoring.ts(lines 212-217) already buildtemporalviatemporalMetadataFromRow, which normalizes;evaluateMemoryTemporalPolicythen normalizes again per candidate (including timezone canonicalization). Consider trusting an already-canonicalMemoryTemporalMetadatahere, or exposing aevaluateNormalizedMemoryTemporalPolicyvariant for hot paths.🤖 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/memory/core/temporal.ts` around lines 376 - 388, The retrieval path redundantly normalizes temporal metadata in evaluateMemoryTemporalPolicy. Add or use a normalized-policy evaluation path, such as evaluateNormalizedMemoryTemporalPolicy, and update callers like fuse that already use temporalMetadataFromRow to call it, while preserving normalization for untrusted direct callers.src/main/memory/index.ts (1)
615-641: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer explicit parameter/return types over
Parameters<DirectiveService[...]>on the public facade.These are public
MemoryServicemethods consumed across the route/DTO boundary; deriving their signatures fromDirectiveServiceinternals makes the contract opaque at call sites and lets an internal signature change silently reshape the public API. Naming the shared directive input/result types directly would keep the facade self-describing.🤖 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/memory/index.ts` around lines 615 - 641, The public MemoryService directive facade methods listDirectives, createDirective, and createDirectiveResult should use explicit shared directive input, result, options, source, and return types instead of Parameters<DirectiveService[...]>. Import the existing shared types and annotate each method’s parameters and return value directly, preserving the current delegation behavior and public API semantics.src/main/memory/core/injectionPort.ts (1)
388-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate recall ordering/rendering with
assembleMemorySection.
buildRecallCandidatesrepeats thekind !== 'working'filter, the non-episodic/episodic ordering, and thecontent + temporalAnnotationrendering already present at Lines 326-338. Two copies of this policy will drift (e.g. a future annotation format change applied in one place only).♻️ Suggested direction
- const recalled = payload.memories.filter((memory) => memory.kind !== 'working') - const ordered = [ - ...recalled.filter((memory) => memory.kind !== 'episodic'), - ...recalled.filter((memory) => memory.kind === 'episodic') - ] - const lines: string[] = [] + const lines: string[] = []and have
assembleMemorySectioniteratebuildRecallCandidates(payload.memories), reusingcandidate.lineinstead of re-rendering.🤖 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/memory/core/injectionPort.ts` around lines 388 - 402, Deduplicate recall filtering, ordering, and rendering by making assembleMemorySection consume buildRecallCandidates(payload.memories) and append each candidate.line directly. Remove the duplicate memory-kind selection and content/temporalAnnotation rendering logic from assembleMemorySection, preserving the existing candidate order and sanitized output.src/main/memory/services/writeCoordinator.ts (1)
470-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancellation inside the directive-suggestion loop skips the extraction audit.
Returning here bypasses
writeExtractionAuditeven though claims from this batch were already applied above, so a cancelled run leaves nomemory/extractaudit row. Every other fence check in this method returns after the audit is written. Prefer breaking out of the loop and letting the existing post-audit fence check (Line 491) handle the cancellation.♻️ Proposed change
if (!batch.failed) { for (const suggestion of parsed.directiveSuggestions) { - if (!this.ctx.canContinueOperation(operationFence)) { - extractionOutcome = 'cancelled' - return { ok: false } - } + if (!this.ctx.canContinueOperation(operationFence)) break this.ports.suggestDirective(input.agentId, suggestion) } }🤖 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/memory/services/writeCoordinator.ts` around lines 470 - 482, Update the directive-suggestion loop in the extraction method to stop iterating when canContinueOperation returns false without returning immediately; preserve the cancelled outcome, break the loop, and allow writeExtractionAudit to run before the existing post-audit fence check handles cancellation.src/main/memory/routes.ts (1)
288-308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the unused directive methods from
MemoryRouteService.
createDirectiveandapproveDirective(row-or-null variants) are declared but never called — the handlers usecreateDirectiveResult/approveDirectiveResult. Keeping both forces every implementation and test double to provide two shapes per operation, and the pairing is already inconsistent (rejectDirectivehas no*Resulttwin).♻️ Proposed change
- createDirective( - agentId: string, - input: MemoryDirectiveInput, - source?: 'explicit_user' | 'manual' - ): AgentMemoryDirectiveRow | null createDirectiveResult( agentId: string, input: MemoryDirectiveInput, source?: 'explicit_user' | 'manual' ): MemoryDirectiveCommandResult - approveDirective(agentId: string, directiveId: string): AgentMemoryDirectiveRow | null approveDirectiveResult(agentId: string, directiveId: string): MemoryDirectiveCommandResult🤖 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/memory/routes.ts` around lines 288 - 308, Remove the unused createDirective and approveDirective row-or-null methods from the MemoryRouteService interface, leaving createDirectiveResult and approveDirectiveResult as the supported operations. Update any implementations and test doubles to stop providing these obsolete methods while preserving the existing rejectDirective contract.src/main/memory/data/tables/agentMemory.ts (1)
962-988: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap the scope-trigger swap in a transaction like
ensureTemporalArtifactsdoes.
AGENT_MEMORY_SCOPE_TRIGGER_DROP_SQLandAGENT_MEMORY_SCOPE_TRIGGER_SQLrun as two separateexeccalls, so a failure in the second leaves the database with no scope-validation triggers until the next startup repair.ensureTemporalArtifacts(Line 939) already usesthis.db.transaction(...)for the same drop/recreate pattern.♻️ Proposed change
- this.db.exec(AGENT_MEMORY_SCOPE_TRIGGER_DROP_SQL) - this.db.exec(AGENT_MEMORY_SCOPE_TRIGGER_SQL) + this.db.transaction(() => { + this.db.exec(AGENT_MEMORY_SCOPE_TRIGGER_DROP_SQL) + this.db.exec(AGENT_MEMORY_SCOPE_TRIGGER_SQL) + })()🤖 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/memory/data/tables/agentMemory.ts` around lines 962 - 988, Update ensureScopeArtifacts to wrap the scope trigger drop-and-recreate operations in a single this.db.transaction(...) call, matching ensureTemporalArtifacts. Ensure AGENT_MEMORY_SCOPE_TRIGGER_DROP_SQL and AGENT_MEMORY_SCOPE_TRIGGER_SQL execute within that transaction so a failure preserves the existing triggers.src/main/memory/services/rowMutations.ts (1)
135-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftProvenance key and insert scope are derived from two independent sources.
buildClaimInsertInputtakesprovenanceKeyfrom the caller but derives the persisted scope frominput.options.scope. Nothing enforces that the caller built that key with the same scope, so a mismatched caller silently persists a row whosescope_type/scope_iddisagrees with its provenance key — which then breaksresolveProvenance's new scope-equality check (Line 126) for that row forever. The call sites in this file are correct today; the contract is what's fragile.Consider taking
scopeas an explicit field and building the key inside the helper viabuildScopedMemoryProvenanceKey, so the pair cannot diverge.🤖 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/memory/services/rowMutations.ts` around lines 135 - 164, Update buildClaimInsertInput to accept an explicit scope rather than a caller-provided provenanceKey, normalize that scope once, and build the provenance key inside the helper with buildScopedMemoryProvenanceKey using the same normalized scope. Persist that identical scope in the insert input, and update all callers in this file to pass scope data instead of constructing or supplying the key.src/main/memory/services/workingMemoryService.ts (1)
286-300: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsult the freshness cache before hitting the repository.
readWorkingMemorycallsflushWorkingMemoryIfDirtyon every read, which now routes the clean case here.resolveWorkingRow(twogetByProvenanceKeylookups) runs before the freshness check even when the cached projection is still valid, so every clean read pays for lookups it then discards.♻️ Proposed reorder
private refreshWorkingMemoryAtTemporalBoundary(agentId: string): void { if (!this.ctx.canReadAgentMemory(agentId)) return - const existing = this.resolveWorkingRow(agentId) - if (!existing) return const freshness = this.workingProjectionFreshness.get(agentId) const now = this.ctx.now() if ( freshness && now >= freshness.builtAt && (freshness.nextRefreshAt === null || now < freshness.nextRefreshAt) ) { return } + if (!this.resolveWorkingRow(agentId)) return this.refreshWorkingMemory(agentId, { preserveOrphanedExisting: true }) }🤖 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/memory/services/workingMemoryService.ts` around lines 286 - 300, Reorder refreshWorkingMemoryAtTemporalBoundary so it evaluates the workingProjectionFreshness entry and current time before calling resolveWorkingRow. Return immediately when the cached projection is still valid, then perform the existing resolveWorkingRow lookup and refresh behavior only when a refresh is due, while preserving the canReadAgentMemory guard.test/main/memory/memoryNativeMigration.test.ts (1)
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
dropV48DerivedArtifactsacross two migration test files. Both files define a byte-identical helper because there is no shared home for native SQLite migration test helpers; when the v48 dirty/derivation artifact set changes, both copies must be updated in lockstep.
test/main/memory/memoryNativeMigration.test.ts#L68-L76: move this definition into the shared native SQLite test harness (alongsidenativeSqliteDescribeIf) and import it here.test/main/memory/agentMemoryTable.test.ts#L258-L267: delete the local copy and import the shared helper.🤖 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/memory/memoryNativeMigration.test.ts` around lines 68 - 76, The helper dropV48DerivedArtifacts is duplicated across two migration test files. Move its single definition into the shared native SQLite test harness alongside nativeSqliteDescribeIf, import it in test/main/memory/memoryNativeMigration.test.ts#L68-L76, and delete the local copy while importing the shared helper in test/main/memory/agentMemoryTable.test.ts#L258-L267.test/main/memory/retrievalService.test.ts (1)
386-396: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
vi.waitForon an already-true condition can't detect a late deletion.The vector is present before the recall, so the polling assertion resolves on the first tick — the test passes even if inline cleanup never ran, and it would still pass if cleanup deleted
sessionTwoIdshortly after. Prefer awaiting the cleanup completion seam (or flushing pending work) and then asserting presence once.🤖 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/memory/retrievalService.test.ts` around lines 386 - 396, Replace the vi.waitFor polling around the sessionTwoId vector assertion with an await of the cleanup completion seam or an equivalent flush of pending inline work after presenter.recall. Then assert store.vectors.has(sessionTwoId) once, preserving the existing repository metadata assertions.test/main/memory/maintenanceService.test.ts (1)
200-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion tolerates a missing row.
row?.superseded_by === nullis false forundefined, so this passes even if00-sourceor99-neighborwere deleted rather than superseded. Asserting the surviving id directly (e.g.expect(repo.getById('99-neighbor')?.superseded_by).toBeNull()plus the source'ssuperseded_by) pins the intended merge direction.🤖 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/memory/maintenanceService.test.ts` around lines 200 - 206, Replace the aggregate filtered-length assertion with direct assertions on both records returned by repo.getById: verify 99-neighbor remains present with superseded_by null and 00-source has the expected superseded_by value pointing to the neighbor, preserving the intended merge direction.test/main/memory/memoryAdd.test.ts (1)
330-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
it.eachfor the UPDATE/SUPERSEDE matrix.The manual
forloop overcasescollapses two independent scenarios into one test, so a failure doesn't identify which decision kind regressed.🤖 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/memory/memoryAdd.test.ts` around lines 330 - 378, Refactor the test around “keeps decision rewrites atomic when they would recreate a forgotten claim” to use it.each with separate UPDATE and SUPERSEDE cases. Move each case’s decision and candidate values into the parameterized test table so failures identify the specific decision kind, while preserving the existing setup and assertions.test/renderer/api/clients.test.ts (1)
985-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead conditional in shared mock case.
routeName === 'memory.rejectDirective'can never be true inside thiscase 'memory.createDirective': case 'memory.approveDirective':block — reject has its own separate case below. Harmless (status still resolves correctly to'active'), but it's leftover copy-paste logic that could confuse future readers of this mock.🧹 Proposed cleanup
- return { - action: 'applied', - directive: { - id: - typeof payload?.directiveId === 'string' - ? payload.directiveId - : 'directive-created', - agentId: payload?.agentId ?? 'agent-1', - kind: 'instruction', - status: routeName === 'memory.rejectDirective' ? 'rejected' : 'active', - source: 'manual', - content: 'Be concise.', - topic: null, - createdAt: 1_000, - updatedAt: 2_000 - } - } + return { + action: 'applied', + directive: { + id: + typeof payload?.directiveId === 'string' + ? payload.directiveId + : 'directive-created', + agentId: payload?.agentId ?? 'agent-1', + kind: 'instruction', + status: 'active', + source: 'manual', + content: 'Be concise.', + topic: null, + createdAt: 1_000, + updatedAt: 2_000 + } + }🤖 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/renderer/api/clients.test.ts` around lines 985 - 1003, Remove the unreachable routeName === 'memory.rejectDirective' conditional from the shared memory.createDirective/memory.approveDirective mock case and set the directive status directly to 'active'. Leave the separate rejectDirective case unchanged.test/main/routes/memoryDto.test.ts (1)
932-940: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the accepting boundary case for directive content length.
Only the over-limit case is asserted, so a regression that lowers the effective cap below
AGENT_MEMORY_DIRECTIVE_CONTENT_MAX_CHARSwould still pass. Thememory.addblock above tests both sides of the limit; mirroring that here keeps the code-point counting behavior pinned.♻️ Suggested addition
+ expect( + memoryCreateDirectiveRoute.input.safeParse({ + agentId: 'deepchat', + directive: { + kind: 'instruction', + content: '😀'.repeat(AGENT_MEMORY_DIRECTIVE_CONTENT_MAX_CHARS) + } + }).success + ).toBe(true) expect( memoryCreateDirectiveRoute.input.safeParse({ agentId: 'deepchat', directive: { kind: 'instruction', content: '😀'.repeat(AGENT_MEMORY_DIRECTIVE_CONTENT_MAX_CHARS + 1) } }).success ).toBe(false)🤖 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/routes/memoryDto.test.ts` around lines 932 - 940, Add an accepting boundary assertion in the memoryCreateDirectiveRoute validation tests for directive content consisting of AGENT_MEMORY_DIRECTIVE_CONTENT_MAX_CHARS code points, alongside the existing over-limit case. Preserve the same agentId and instruction payload structure so both sides of the limit and code-point counting are verified.test/renderer/components/MemoryInboxBar.test.ts (1)
117-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared directive draft limit constant instead of the literal
64.
AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNTalready defines64; using it here (and for thedirectiveCapacityDescriptionmax value) keeps this test aligned with the shared configuration.♻️ Suggested change
+import { AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNT } from '../../../src/shared/types/agent-memory' @@ expect(memoryClient.listDirectives).toHaveBeenCalledWith('deepchat', { statuses: ['draft'], - limit: 64 + limit: AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNT }) @@ - description: 'settings.memory.redesign.directiveCapacityDescription:{"max":64}' + description: `settings.memory.redesign.directiveCapacityDescription:${JSON.stringify({ + max: AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNT + })}`Also applies to: 174-178
🤖 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/renderer/components/MemoryInboxBar.test.ts` around lines 117 - 120, Replace the hardcoded directive limit value 64 in the MemoryInboxBar test expectations, including the directiveCapacityDescription max value, with the shared AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNT constant so both assertions follow the shared configuration.src/main/agent/deepchat/runtime/contextBuilder.ts (1)
1372-1408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated drop-memory/drop-directive/overflow logic across three functions.
buildCacheAwareContextWithMetadata,buildCacheAwareResumeContextWithMetadata, andfitCacheAwareMessagesToContextWindoweach repeat the same "drop memory if over budget → drop directives if still over physical budget → throw overflow" sequence, only differing in how the active-turn message is rebuilt. Also notefitCacheAwareMessagesToContextWindowhardcodesphysicalInputBudget = contextLength - 1(Line 1790) instead of reusingresolvePhysicalInputBudget(contextLength, extraReserveTokens)used by the other two functions — worth confirming these stay equivalent (this function has noextraReserveTokensoption) so future changes to the shared formula don't silently diverge here.Consider extracting a shared helper (e.g.
dropUntilWithinPhysicalBudget(activeTurn, context, physicalBudget, rebuild)) parameterized by the rebuild strategy, to avoid triplicated budget-fitting logic.Also applies to: 1511-1544, 1792-1830
🤖 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/agent/deepchat/runtime/contextBuilder.ts` around lines 1372 - 1408, Extract the repeated memory/directives removal and overflow handling from buildCacheAwareContextWithMetadata, buildCacheAwareResumeContextWithMetadata, and fitCacheAwareMessagesToContextWindow into one helper parameterized by the active-turn message rebuild strategy. Update all three callers to use it, preserving their existing budget checks and overflow behavior; have fitCacheAwareMessagesToContextWindow obtain its physical budget through resolvePhysicalInputBudget(contextLength, extraReserveTokens) with the appropriate no-extra-reserve value instead of hardcoding contextLength - 1.
🤖 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.
Inline comments:
In `@src/main/memory/core/tombstone.ts`:
- Around line 26-47: Update buildMemoryTombstoneIdentities so content tombstone
hashes include the normalized scope alongside agentId and normalized content, or
reuse the scoped provenance key for both identity kinds. Ensure identical
content in different scopes produces distinct content identities while
preserving provenance identity behavior.
In `@src/main/memory/services/maintenanceService.ts`:
- Around line 517-554: Defer dirty seeds when embedding readiness is transient
instead of settling them: in the dimensions === null branch, replace
settleDirtySeeds with deferDirtySeeds using the repository directly. In the
current-row validation, separate the embedding-readiness failure from the
terminal decision_revision mismatch; defer the seed for non-ready/transient rows
and settle only revision-mismatched or otherwise terminal seeds, preserving
retry semantics through deferSeed where applicable.
In `@src/renderer/src/i18n/da-DK/chat.json`:
- Around line 512-518: Translate the newly added memory-turn budget labels
(allocation, directive, persona, working, queryRecall, overhead, and unused) and
directive-management UI strings in src/renderer/src/i18n/da-DK/chat.json:512-518
and settings.json:3117-3156 into Danish; apply the equivalent translations in
src/renderer/src/i18n/de-DE/chat.json:512-518 and settings.json:3108-3147 for
German, src/renderer/src/i18n/ko-KR/chat.json:512-518 and
settings.json:3117-3156 for Korean,
src/renderer/src/i18n/ms-MY/chat.json:512-518 and settings.json:3108-3147 for
Malay, src/renderer/src/i18n/pl-PL/chat.json:512-518 and settings.json:3108-3147
for Polish, and src/renderer/src/i18n/pt-BR/chat.json:512-518 and
settings.json:3117-3156 for Brazilian Portuguese, preserving all existing
translation keys and JSON structure.
In `@src/renderer/src/i18n/es-ES/chat.json`:
- Around line 512-518: Translate the newly added memory-budget labels
(allocation, directive, persona, working, queryRecall, overhead, and unused) in
src/renderer/src/i18n/es-ES/chat.json lines 512-518, fa-IR/chat.json lines
512-518, ru-RU/chat.json lines 512-518, tr-TR/chat.json lines 512-518, and
vi-VN/chat.json lines 512-518. Translate the added directive-management strings
in src/renderer/src/i18n/es-ES/settings.json lines 3107-3147,
ru-RU/settings.json lines 3116-3156, tr-TR/settings.json lines 3107-3147, and
vi-VN/settings.json lines 3107-3147, preserving the existing keys and JSON
structure.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 3117-3156: Translate the new memory UI strings instead of leaving
English fallbacks: translate the budget-label entries in
src/renderer/src/i18n/fr-FR/chat.json (512-518), he-IL/chat.json (512-518),
id-ID/chat.json (512-518), it-IT/chat.json (512-518), and ja-JP/chat.json
(512-518) into their respective languages; translate the directive-management
entries keyed by tabDirectives through directiveDeleteDescription in
src/renderer/src/i18n/fa-IR/settings.json (3117-3156), fr-FR/settings.json
(3117-3156), he-IL/settings.json (3117-3156), id-ID/settings.json (3108-3147),
it-IT/settings.json (3108-3147), and ja-JP/settings.json (3117-3156) into
Persian, French, Hebrew, Indonesian, Italian, and Japanese respectively,
preserving placeholders and JSON structure.
In `@src/shared/contracts/routes/memory.routes.ts`:
- Around line 213-223: Update MemoryDirectiveItemSchema to enforce the same
kind/topic invariant as the input union: suppress_topic items must have a null
topic, while instruction items must have a non-null topic. Preserve the
remaining item fields and validation rules.
In `@test/renderer/components/chat/MemoryTurnDialog.test.ts`:
- Around line 183-197: Update the test setup around makeTurn so the allocation
is provided through the helper’s manifest/nested allocation overrides instead of
assigning next.manifest.allocation afterward. Preserve the existing allocation
values, then assign the fully constructed result to memoryActivity.selectedTurn.
---
Outside diff comments:
In `@src/shared/contracts/routes/memory.routes.ts`:
- Around line 121-144: Update enforceProjectedScopeInvariant, which is applied
to both MemoryItemSchema and MemorySearchResultSchema, to validate temporal
metadata: atemporal rows must not carry temporal fields, non-atemporal rows must
provide temporalConfidence, and any populated validFrom/validUntil interval must
not be reversed. Preserve the existing projected-scope checks while ensuring
both response DTOs reject these malformed states.
---
Nitpick comments:
In `@src/main/agent/deepchat/runtime/contextBuilder.ts`:
- Around line 1372-1408: Extract the repeated memory/directives removal and
overflow handling from buildCacheAwareContextWithMetadata,
buildCacheAwareResumeContextWithMetadata, and
fitCacheAwareMessagesToContextWindow into one helper parameterized by the
active-turn message rebuild strategy. Update all three callers to use it,
preserving their existing budget checks and overflow behavior; have
fitCacheAwareMessagesToContextWindow obtain its physical budget through
resolvePhysicalInputBudget(contextLength, extraReserveTokens) with the
appropriate no-extra-reserve value instead of hardcoding contextLength - 1.
In `@src/main/memory/core/injectionPort.ts`:
- Around line 388-402: Deduplicate recall filtering, ordering, and rendering by
making assembleMemorySection consume buildRecallCandidates(payload.memories) and
append each candidate.line directly. Remove the duplicate memory-kind selection
and content/temporalAnnotation rendering logic from assembleMemorySection,
preserving the existing candidate order and sanitized output.
In `@src/main/memory/core/scoring.ts`:
- Around line 20-29: Merge the two type-only imports from ../domain/types in
scoring.ts into a single import declaration containing MemoryScope,
AgentMemoryKind, MemoryTemporalMetadata, and MemoryTemporalPolicyResult; leave
all other imports unchanged.
In `@src/main/memory/core/temporal.ts`:
- Around line 376-388: The retrieval path redundantly normalizes temporal
metadata in evaluateMemoryTemporalPolicy. Add or use a normalized-policy
evaluation path, such as evaluateNormalizedMemoryTemporalPolicy, and update
callers like fuse that already use temporalMetadataFromRow to call it, while
preserving normalization for untrusted direct callers.
In `@src/main/memory/core/workingProjection.ts`:
- Around line 216-229: Update resolveNextRefreshAt to accept and use the
temporal metadata already computed by toCandidate, rather than calling
temporalMetadataFromRow for each row. Adjust the caller and input shape as
needed while preserving selection of the earliest future validFrom or validUntil
boundary.
- Around line 269-288: The budget-selection loop in working projection
repeatedly calls renderSections and estimateTokens for every candidate, causing
quadratic work. Replace the per-candidate full rendering in the selection logic
with incremental token accounting using the section-label overhead table, accept
candidates only when their incremental line cost stays within budget, and render
the final selected projection once after selection.
In `@src/main/memory/data/tables/agentMemory.ts`:
- Around line 962-988: Update ensureScopeArtifacts to wrap the scope trigger
drop-and-recreate operations in a single this.db.transaction(...) call, matching
ensureTemporalArtifacts. Ensure AGENT_MEMORY_SCOPE_TRIGGER_DROP_SQL and
AGENT_MEMORY_SCOPE_TRIGGER_SQL execute within that transaction so a failure
preserves the existing triggers.
In `@src/main/memory/domain/directives.ts`:
- Around line 128-135: Remove the redundant empty-topic guard after the
`normalizedTopic = assertBoundedText(...)` call in the directive normalization
flow. Keep the existing `assertBoundedText` validation and normalization
behavior unchanged.
- Line 1: Update the crypto import in the directives module to use the
`node:crypto` specifier, matching the convention used by the nearby tombstone
module and preserving the existing createHash usage.
In `@src/main/memory/index.ts`:
- Around line 615-641: The public MemoryService directive facade methods
listDirectives, createDirective, and createDirectiveResult should use explicit
shared directive input, result, options, source, and return types instead of
Parameters<DirectiveService[...]>. Import the existing shared types and annotate
each method’s parameters and return value directly, preserving the current
delegation behavior and public API semantics.
In `@src/main/memory/routes.ts`:
- Around line 288-308: Remove the unused createDirective and approveDirective
row-or-null methods from the MemoryRouteService interface, leaving
createDirectiveResult and approveDirectiveResult as the supported operations.
Update any implementations and test doubles to stop providing these obsolete
methods while preserving the existing rejectDirective contract.
In `@src/main/memory/services/rowMutations.ts`:
- Around line 135-164: Update buildClaimInsertInput to accept an explicit scope
rather than a caller-provided provenanceKey, normalize that scope once, and
build the provenance key inside the helper with buildScopedMemoryProvenanceKey
using the same normalized scope. Persist that identical scope in the insert
input, and update all callers in this file to pass scope data instead of
constructing or supplying the key.
In `@src/main/memory/services/workingMemoryService.ts`:
- Around line 286-300: Reorder refreshWorkingMemoryAtTemporalBoundary so it
evaluates the workingProjectionFreshness entry and current time before calling
resolveWorkingRow. Return immediately when the cached projection is still valid,
then perform the existing resolveWorkingRow lookup and refresh behavior only
when a refresh is due, while preserving the canReadAgentMemory guard.
In `@src/main/memory/services/writeCoordinator.ts`:
- Around line 470-482: Update the directive-suggestion loop in the extraction
method to stop iterating when canContinueOperation returns false without
returning immediately; preserve the cancelled outcome, break the loop, and allow
writeExtractionAudit to run before the existing post-audit fence check handles
cancellation.
In `@src/renderer/settings/components/MemoryInboxBar.vue`:
- Around line 322-385: Extract the shared directive action state machine from
setDirectivePending, approveDirective, and rejectDirective into a
useDirectiveActions composable, then update MemoryInboxBar.vue and
MemoryDirectivesPanel.vue to use it. Preserve the existing revision guard,
pending-ID tracking, rejected-reason notification, stale-agent protection, and
reload-on-error behavior in both panels.
In `@src/renderer/src/components/chat/MemoryTurnDialog.vue`:
- Around line 94-97: Replace the label-and-value concatenation in the
MemoryTurnDialog template with one interpolated translation using a key such as
chat.memory.turn.overheadSummary and named overhead and unused values. Add the
corresponding locale message entries so translators control ordering and
separators, while preserving the displayed token values.
In `@test/main/memory/maintenanceService.test.ts`:
- Around line 200-206: Replace the aggregate filtered-length assertion with
direct assertions on both records returned by repo.getById: verify 99-neighbor
remains present with superseded_by null and 00-source has the expected
superseded_by value pointing to the neighbor, preserving the intended merge
direction.
In `@test/main/memory/memoryAdd.test.ts`:
- Around line 330-378: Refactor the test around “keeps decision rewrites atomic
when they would recreate a forgotten claim” to use it.each with separate UPDATE
and SUPERSEDE cases. Move each case’s decision and candidate values into the
parameterized test table so failures identify the specific decision kind, while
preserving the existing setup and assertions.
In `@test/main/memory/memoryNativeMigration.test.ts`:
- Around line 68-76: The helper dropV48DerivedArtifacts is duplicated across two
migration test files. Move its single definition into the shared native SQLite
test harness alongside nativeSqliteDescribeIf, import it in
test/main/memory/memoryNativeMigration.test.ts#L68-L76, and delete the local
copy while importing the shared helper in
test/main/memory/agentMemoryTable.test.ts#L258-L267.
In `@test/main/memory/retrievalService.test.ts`:
- Around line 386-396: Replace the vi.waitFor polling around the sessionTwoId
vector assertion with an await of the cleanup completion seam or an equivalent
flush of pending inline work after presenter.recall. Then assert
store.vectors.has(sessionTwoId) once, preserving the existing repository
metadata assertions.
In `@test/main/routes/memoryDto.test.ts`:
- Around line 932-940: Add an accepting boundary assertion in the
memoryCreateDirectiveRoute validation tests for directive content consisting of
AGENT_MEMORY_DIRECTIVE_CONTENT_MAX_CHARS code points, alongside the existing
over-limit case. Preserve the same agentId and instruction payload structure so
both sides of the limit and code-point counting are verified.
In `@test/renderer/api/clients.test.ts`:
- Around line 985-1003: Remove the unreachable routeName ===
'memory.rejectDirective' conditional from the shared
memory.createDirective/memory.approveDirective mock case and set the directive
status directly to 'active'. Leave the separate rejectDirective case unchanged.
In `@test/renderer/components/MemoryInboxBar.test.ts`:
- Around line 117-120: Replace the hardcoded directive limit value 64 in the
MemoryInboxBar test expectations, including the directiveCapacityDescription max
value, with the shared AGENT_MEMORY_ACTIVE_DIRECTIVE_MAX_COUNT constant so both
assertions follow the shared configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fc022c6-a870-4fe0-9874-52f1048b355a
📒 Files selected for processing (168)
docs/architecture/agent-memory-evolution/plan.mddocs/architecture/agent-memory-evolution/spec.mddocs/architecture/agent-memory-evolution/tasks.mddocs/architecture/memory-system.mdsrc/main/agent/deepchat/deepChatAgentRepository.tssrc/main/agent/deepchat/memory/memoryPromptContributor.tssrc/main/agent/deepchat/memory/memoryRuntimeCoordinator.tssrc/main/agent/deepchat/runtime/contextBuilder.tssrc/main/agent/deepchat/runtime/contextContributions.tssrc/main/agent/deepchat/runtime/promptAssemblyService.tssrc/main/agent/repository/index.tssrc/main/app/composition.tssrc/main/data/schemaCatalog.tssrc/main/data/sqliteCopyExclusions.tssrc/main/memory/context.tssrc/main/memory/core/batchDecision.tssrc/main/memory/core/candidates.tssrc/main/memory/core/contributionBudget.tssrc/main/memory/core/decision.tssrc/main/memory/core/directiveContribution.tssrc/main/memory/core/directivePolicy.tssrc/main/memory/core/extraction.tssrc/main/memory/core/injectionPort.tssrc/main/memory/core/scope.tssrc/main/memory/core/scoring.tssrc/main/memory/core/temporal.tssrc/main/memory/core/tombstone.tssrc/main/memory/core/workingProjection.tssrc/main/memory/data/database.tssrc/main/memory/data/tables/agentMemory.tssrc/main/memory/data/tables/agentMemoryDirective.tssrc/main/memory/domain/clock.tssrc/main/memory/domain/directives.tssrc/main/memory/domain/types.tssrc/main/memory/index.tssrc/main/memory/injection.tssrc/main/memory/ports.tssrc/main/memory/routes.tssrc/main/memory/runtimeConstants.tssrc/main/memory/services/conflictService.tssrc/main/memory/services/directiveService.tssrc/main/memory/services/maintenanceService.tssrc/main/memory/services/managementService.tssrc/main/memory/services/personaService.tssrc/main/memory/services/reflectionService.tssrc/main/memory/services/retrievalService.tssrc/main/memory/services/rowMutations.tssrc/main/memory/services/workingMemoryService.tssrc/main/memory/services/writeCoordinator.tssrc/main/memory/types.tssrc/main/sync/dataImporter.tssrc/main/tape/application/viewReplayService.tssrc/main/tape/domain/replay.tssrc/main/tape/domain/viewManifest.tssrc/main/tape/ports/capabilities.tssrc/main/tool/agentTools/agentMemoryTools.tssrc/main/tool/runtimePorts.tssrc/renderer/api/MemoryClient.tssrc/renderer/settings/components/MemoryDirectivesPanel.vuesrc/renderer/settings/components/MemoryInboxBar.vuesrc/renderer/settings/components/MemorySettings.vuesrc/renderer/settings/components/memoryRedesignUtils.tssrc/renderer/src/components/chat/MemoryTurnDialog.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/events/memory.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/memory.routes.tssrc/shared/types/agent-memory.tssrc/shared/types/tape-view-manifest.tssrc/types/i18n.d.tstest/fixtures/memory/behavior-v1.jsontest/main/agent/deepchat/deepChatAgentRepository.test.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/loop/contextCoordinator.test.tstest/main/agent/deepchat/memory/memoryRuntimeCoordinator.test.tstest/main/agent/deepchat/runtime/contextBuilder.test.tstest/main/agent/deepchat/runtime/promptAssemblyService.test.tstest/main/app/databaseSecurity.test.tstest/main/memory/agentMemoryDirectiveTable.test.tstest/main/memory/agentMemoryTable.test.tstest/main/memory/conflictService.test.tstest/main/memory/contributionBudget.test.tstest/main/memory/directiveContribution.test.tstest/main/memory/directivePolicy.test.tstest/main/memory/directiveService.test.tstest/main/memory/embeddingDiagnostics.test.tstest/main/memory/lifecycleRegression.test.tstest/main/memory/maintenanceService.test.tstest/main/memory/managementService.test.tstest/main/memory/memory-persona-eval.test.tstest/main/memory/memoryAdd.test.tstest/main/memory/memoryBatchDecision.test.tstest/main/memory/memoryBehavior.eval.test.tstest/main/memory/memoryDecision.test.tstest/main/memory/memoryEmbeddingScale.test.tstest/main/memory/memoryExtraction.test.tstest/main/memory/memoryInjectionPort.test.tstest/main/memory/memoryLifecycle.test.tstest/main/memory/memoryNativeMigration.test.tstest/main/memory/memoryRuntimeContext.test.tstest/main/memory/memoryScope.test.tstest/main/memory/memorySearch.test.tstest/main/memory/memoryService.test.tstest/main/memory/memoryServiceHarness.test.tstest/main/memory/memoryTemporal.test.tstest/main/memory/memoryUpdate.test.tstest/main/memory/memoryUpdateNative.test.tstest/main/memory/personaService.test.tstest/main/memory/repositoryHarness.test.tstest/main/memory/retrievalDiagnostics.test.tstest/main/memory/retrievalService.test.tstest/main/memory/serviceTestSupport.tstest/main/memory/support/memoryFakes.tstest/main/memory/support/memoryServiceTestAdapter.tstest/main/memory/workingMemoryService.test.tstest/main/memory/workingProjection.test.tstest/main/memory/writeCoordinator.test.tstest/main/routes/dispatcher.test.tstest/main/routes/memoryDto.test.tstest/main/session/data/tapeViewManifest.test.tstest/main/session/data/tapeViewReplay.test.tstest/main/tool/agentTools/agentMemoryTools.test.tstest/memory-test-scope.jsontest/renderer/api/clients.test.tstest/renderer/components/MemoryDiagnosticsPanel.test.tstest/renderer/components/MemoryDirectivesPanel.test.tstest/renderer/components/MemoryInboxBar.test.tstest/renderer/components/MemorySettings.test.tstest/renderer/components/chat/MemoryTurnDialog.test.ts
Keep legacy agent tombstones, scope narrow content identities, and derive provenance from scope. Defer transient consolidation work and enforce response and cancellation audit contracts.
Use exact incremental token accounting and normalized temporal fast paths. Localize directive and allocation UI, and strengthen asynchronous and native test coverage.
Contributor
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 (2)
src/main/memory/data/tables/agentMemory.ts (2)
2690-2695: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winPreserve the persisted scope when checking tombstones.
Tombstone identities include scope, but these checks omit it and therefore hash the default agent scope. A forgotten user/project/session claim can be restored, revived, activated, or recreated through an edit.
src/main/memory/data/tables/agentMemory.ts#L2690-L2695: passscope: memoryScopeFromRow(before).src/main/memory/data/tables/agentMemory.ts#L2781-L2786: passscope: memoryScopeFromRow(before).src/main/memory/data/tables/agentMemory.ts#L2838-L2844: passscope: memoryScopeFromRow(before).src/main/memory/data/tables/agentMemory.ts#L3390-L3395: passscope: memoryScopeFromRow(before).🤖 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/memory/data/tables/agentMemory.ts` around lines 2690 - 2695, Preserve each claim’s persisted scope when checking tombstones. In the four hasTombstoneForClaim call sites in src/main/memory/data/tables/agentMemory.ts at lines 2690-2695, 2781-2786, 2838-2844, and 3390-3395, add scope: memoryScopeFromRow(before) alongside the existing claim identity fields.
1545-1550: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBackfill legacy user scopes before making scope columns authoritative.
Existing rows with
user_scopereceivescope_type = 'agent'andscope_id = NULL. That combination passes the new constraints, so formerly user-scoped claims become agent-wide after upgrade and may be recalled for other users. Migrate valid legacyuser_scopevalues toscope_type = 'user'andscope_id = user_scopebefore provisioning scope artifacts; include migration coverage for legacy user-scoped rows.🤖 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/memory/data/tables/agentMemory.ts` around lines 1545 - 1550, Update the schema migration returned for AGENT_MEMORY_SCOPE_SCHEMA_VERSION to backfill existing rows with valid user_scope values to scope_type = 'user' and scope_id = user_scope before creating or relying on the new scope index/artifacts. Preserve the new column constraints and ensure legacy user-scoped rows are covered by migration tests so they cannot remain agent-wide.
🤖 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/memory/data/tables/agentMemory.ts`:
- Around line 2690-2695: Preserve each claim’s persisted scope when checking
tombstones. In the four hasTombstoneForClaim call sites in
src/main/memory/data/tables/agentMemory.ts at lines 2690-2695, 2781-2786,
2838-2844, and 3390-3395, add scope: memoryScopeFromRow(before) alongside the
existing claim identity fields.
- Around line 1545-1550: Update the schema migration returned for
AGENT_MEMORY_SCOPE_SCHEMA_VERSION to backfill existing rows with valid
user_scope values to scope_type = 'user' and scope_id = user_scope before
creating or relying on the new scope index/artifacts. Preserve the new column
constraints and ensure legacy user-scoped rows are covered by migration tests so
they cannot remain agent-wide.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c5e4095-2ef8-442d-81cf-5e1eb87ff0a6
📒 Files selected for processing (73)
docs/architecture/agent-memory-evolution/spec.mddocs/architecture/agent-memory-evolution/tasks.mdsrc/main/memory/core/injectionPort.tssrc/main/memory/core/scoring.tssrc/main/memory/core/temporal.tssrc/main/memory/core/tombstone.tssrc/main/memory/core/workingProjection.tssrc/main/memory/data/tables/agentMemory.tssrc/main/memory/domain/directives.tssrc/main/memory/index.tssrc/main/memory/ports.tssrc/main/memory/routes.tssrc/main/memory/services/conflictService.tssrc/main/memory/services/directiveService.tssrc/main/memory/services/maintenanceService.tssrc/main/memory/services/retrievalService.tssrc/main/memory/services/rowMutations.tssrc/main/memory/services/workingMemoryService.tssrc/main/memory/services/writeCoordinator.tssrc/main/sync/dataImporter.tssrc/renderer/settings/components/MemoryInboxBar.vuesrc/renderer/src/components/chat/MemoryTurnDialog.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/shared/contracts/routes/memory.routes.tstest/main/memory/agentMemoryTable.test.tstest/main/memory/maintenanceService.test.tstest/main/memory/memoryAdd.test.tstest/main/memory/memoryExtraction.test.tstest/main/memory/memoryNativeMigration.test.tstest/main/memory/retrievalService.test.tstest/main/memory/serviceTestSupport.tstest/main/memory/support/memoryFakes.tstest/main/memory/workingProjection.test.tstest/main/nativeSqliteHarness.tstest/main/routes/memoryDto.test.tstest/renderer/api/clients.test.tstest/renderer/components/MemoryInboxBar.test.tstest/renderer/components/chat/MemoryTurnDialog.test.ts
💤 Files with no reviewable changes (1)
- src/main/memory/ports.ts
🚧 Files skipped from review as they are similar to previous changes (60)
- src/renderer/src/i18n/pl-PL/chat.json
- src/renderer/src/i18n/de-DE/chat.json
- src/renderer/src/i18n/zh-CN/chat.json
- src/renderer/src/i18n/vi-VN/chat.json
- src/renderer/src/i18n/ms-MY/chat.json
- src/renderer/src/i18n/en-US/chat.json
- src/renderer/src/i18n/fa-IR/chat.json
- src/renderer/src/i18n/ja-JP/chat.json
- src/renderer/src/i18n/fr-FR/chat.json
- src/renderer/src/components/chat/MemoryTurnDialog.vue
- src/renderer/src/i18n/id-ID/chat.json
- src/renderer/src/i18n/ru-RU/chat.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/it-IT/chat.json
- src/renderer/src/i18n/zh-HK/chat.json
- src/renderer/src/i18n/zh-TW/chat.json
- src/renderer/src/i18n/tr-TR/settings.json
- src/renderer/src/i18n/es-ES/settings.json
- src/renderer/src/i18n/id-ID/settings.json
- src/renderer/src/i18n/de-DE/settings.json
- src/renderer/src/i18n/he-IL/chat.json
- src/renderer/src/i18n/vi-VN/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- src/renderer/src/i18n/pt-BR/chat.json
- src/renderer/src/i18n/ms-MY/settings.json
- src/renderer/src/i18n/es-ES/chat.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- test/renderer/components/chat/MemoryTurnDialog.test.ts
- src/main/memory/core/temporal.ts
- test/renderer/api/clients.test.ts
- src/renderer/src/i18n/it-IT/settings.json
- src/renderer/src/i18n/pl-PL/settings.json
- src/renderer/src/i18n/da-DK/chat.json
- src/renderer/settings/components/MemoryInboxBar.vue
- test/main/memory/memoryAdd.test.ts
- src/main/memory/core/scoring.ts
- src/renderer/src/i18n/pt-BR/settings.json
- test/main/routes/memoryDto.test.ts
- test/main/memory/serviceTestSupport.ts
- src/main/memory/services/workingMemoryService.ts
- test/main/memory/memoryExtraction.test.ts
- src/main/sync/dataImporter.ts
- src/main/memory/services/directiveService.ts
- test/main/memory/retrievalService.test.ts
- src/main/memory/core/workingProjection.ts
- test/main/memory/memoryNativeMigration.test.ts
- src/main/memory/services/conflictService.ts
- src/main/memory/core/injectionPort.ts
- src/main/memory/services/rowMutations.ts
- src/shared/contracts/routes/memory.routes.ts
- src/main/memory/routes.ts
- src/main/memory/services/retrievalService.ts
- src/main/memory/index.ts
- src/main/memory/services/maintenanceService.ts
- src/main/memory/services/writeCoordinator.ts
- test/main/memory/support/memoryFakes.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/memory/services/managementService.ts (1)
185-198: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueResume serializes all pending clears.
await this.enqueueMemoryClear(job.agentId)inside the loop runs each agent's clear to completion before starting the next. With several pending jobs this stretches startup recovery; the per-agent dedupe map already makes concurrent execution safe.♻️ Optional: kick off all jobs and settle together
- for (const job of jobs) { - if (!isSafeAgentId(job.agentId)) { - logger.error(`[Memory] refusing to resume clear job with invalid agent id: ${job.agentId}`) - continue - } - try { - await this.enqueueMemoryClear(job.agentId) - } catch (error) { - logger.error(`[Memory] failed to resume clear job for ${job.agentId}: ${String(error)}`) - } - } + const resumed = jobs.flatMap((job) => { + if (!isSafeAgentId(job.agentId)) { + logger.error(`[Memory] refusing to resume clear job with invalid agent id: ${job.agentId}`) + return [] + } + return [ + this.enqueueMemoryClear(job.agentId).catch((error) => { + logger.error(`[Memory] failed to resume clear job for ${job.agentId}: ${String(error)}`) + }) + ] + }) + await Promise.all(resumed)🤖 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/memory/services/managementService.ts` around lines 185 - 198, Update resumePendingMemoryClears to start all valid enqueueMemoryClear jobs without awaiting each one inside the loop, then await them collectively so pending clears recover concurrently while preserving per-job error logging and invalid-agent filtering.src/main/memory/services/retrievalService.ts (1)
958-1067: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the termination invariant of the refill loop.
The
while (true)is bounded only implicitly: each non-breaking iteration must strictly raisecandidateLimitorvectorCandidateLimit, andnextMemoryRetrievalCandidateLimitclamps atMEMORY_RETRIEVAL_MAX_CANDIDATES, so thenext > currentguards eventually fail. That reasoning is load-bearing and easy to break in a later edit (e.g. ifrefillVectorCandidatesever stopped assigningvectorCandidateLimit = limit). A short comment stating the invariant would protect it.📝 Suggested comment
+ // Bounded loop: every non-breaking iteration strictly increases candidateLimit and/or + // vectorCandidateLimit, both clamped by MEMORY_RETRIEVAL_MAX_CANDIDATES, so the + // `next > current` guards terminate it. Keep refill paths monotonic. while (true) {🤖 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/memory/services/retrievalService.ts` around lines 958 - 1067, Add a concise comment immediately before the refill loop around the while (true) block documenting that every non-breaking iteration must strictly increase candidateLimit or vectorCandidateLimit, and that nextMemoryRetrievalCandidateLimit is capped by MEMORY_RETRIEVAL_MAX_CANDIDATES so the next > current guards eventually terminate the loop. Mention that refillVectorCandidates must preserve this assignment invariant.
🤖 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.
Inline comments:
In `@src/main/memory/data/tables/agentMemory.ts`:
- Around line 173-193: The agent_memory write methods exposed by
MemoryRepositoryPort—recordAccess, recordAccessBatch, setConfidence, and
setLastConsolidatedAt—must honor pending clear jobs. Route these updates through
the existing context gate used by recordInjectionAccess/recordAccessBatch, or
make them no-op when isMemoryClearPending(agentId) is true, while preserving
normal writes when no clear is pending.
---
Nitpick comments:
In `@src/main/memory/services/managementService.ts`:
- Around line 185-198: Update resumePendingMemoryClears to start all valid
enqueueMemoryClear jobs without awaiting each one inside the loop, then await
them collectively so pending clears recover concurrently while preserving
per-job error logging and invalid-agent filtering.
In `@src/main/memory/services/retrievalService.ts`:
- Around line 958-1067: Add a concise comment immediately before the refill loop
around the while (true) block documenting that every non-breaking iteration must
strictly increase candidateLimit or vectorCandidateLimit, and that
nextMemoryRetrievalCandidateLimit is capped by MEMORY_RETRIEVAL_MAX_CANDIDATES
so the next > current guards eventually terminate the loop. Mention that
refillVectorCandidates must preserve this assignment invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7991623-994a-47c7-a457-0c150e62f246
📒 Files selected for processing (60)
docs/architecture/agent-memory-evolution/plan.mddocs/architecture/agent-memory-evolution/spec.mddocs/architecture/agent-memory-evolution/tasks.mddocs/architecture/memory-system.mdsrc/main/memory/context.tssrc/main/memory/core/directiveContribution.tssrc/main/memory/core/directivePolicy.tssrc/main/memory/core/retrievalBudget.tssrc/main/memory/data/tables/agentMemory.tssrc/main/memory/domain/directives.tssrc/main/memory/domain/types.tssrc/main/memory/index.tssrc/main/memory/ports.tssrc/main/memory/services/conflictService.tssrc/main/memory/services/directiveService.tssrc/main/memory/services/maintenanceService.tssrc/main/memory/services/managementService.tssrc/main/memory/services/personaService.tssrc/main/memory/services/retrievalService.tssrc/main/memory/services/writeCoordinator.tssrc/main/memory/types.tssrc/main/tool/agentTools/agentMemoryTools.tssrc/renderer/settings/components/MemoryDirectivesPanel.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/lib/memoryDirectiveTopic.tssrc/shared/types/agent-memory.tstest/main/memory/agentMemoryTable.test.tstest/main/memory/directiveContribution.test.tstest/main/memory/directivePolicy.test.tstest/main/memory/directiveService.test.tstest/main/memory/managementService.test.tstest/main/memory/memoryAdd.test.tstest/main/memory/memoryNativeMigration.test.tstest/main/memory/retrievalService.test.tstest/main/memory/support/memoryFakes.tstest/main/performance/memory/clearScale.perf.tstest/main/performance/memory/workloadBounds.perf.tstest/main/tool/agentTools/agentMemoryTools.test.tstest/memory-test-scope.jsontest/renderer/components/MemoryDiagnosticsPanel.test.tstest/renderer/components/MemoryDirectivesPanel.test.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- test/main/memory/directivePolicy.test.ts
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- test/main/memory/directiveContribution.test.ts
- src/renderer/src/i18n/de-DE/settings.json
- src/renderer/src/i18n/pl-PL/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- test/memory-test-scope.json
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- src/renderer/src/i18n/it-IT/settings.json
- test/main/memory/directiveService.test.ts
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- src/renderer/src/i18n/zh-CN/settings.json
- src/shared/types/agent-memory.ts
- src/renderer/src/i18n/tr-TR/settings.json
- src/main/memory/context.ts
- src/main/memory/domain/directives.ts
- src/renderer/src/i18n/ms-MY/settings.json
- src/main/memory/core/directivePolicy.ts
- src/main/memory/services/directiveService.ts
- src/main/memory/services/conflictService.ts
- src/renderer/settings/components/MemoryDirectivesPanel.vue
- test/renderer/components/MemoryDirectivesPanel.test.ts
- docs/architecture/agent-memory-evolution/spec.md
- docs/architecture/agent-memory-evolution/plan.md
- src/main/memory/types.ts
- src/main/memory/ports.ts
- test/main/memory/retrievalService.test.ts
- src/main/memory/services/maintenanceService.ts
- test/main/memory/memoryNativeMigration.test.ts
- src/main/memory/domain/types.ts
- src/main/memory/index.ts
- test/main/memory/support/memoryFakes.ts
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
Evolve Agent Memory around authoritative atomic claims, temporal correctness, rebuildable projections, incremental consolidation, and an explicit user-controlled directive plane.
This improves memory continuity, preference adherence, time-aware correctness, correction handling, observability, and long-term maintainability.
What changed
Summary by CodeRabbit
New Features
Improvements
Documentation