refactor(memory): decouple memory presenter#1873
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe memory presenter is restructured into layered core, infra, and services modules behind a single facade. It adds runtime context, vector-store and embedding orchestration, write/retrieval/maintenance/persona/conflict/management services, updated architecture rules, and test updates for the new module boundaries. ChangesMemory Presenter Layering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WriteCoordinator
participant MemoryRowMutations
participant RetrievalService
participant EmbeddingPipeline
participant VectorStoreManager
WriteCoordinator->>MemoryRowMutations: insertMemory / applyContentUpdate
WriteCoordinator->>EmbeddingPipeline: triggerEmbedding(agentId)
RetrievalService->>VectorStoreManager: getVectorStore(dimensions)
RetrievalService->>EmbeddingPipeline: warmVectorStore / backfillEmbeddings
RetrievalService-->>WriteCoordinator: fused FTS+vector results
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
scripts/architecture-guard.mjs (1)
259-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider table-driven rules to reduce branch duplication.
The four layer branches (root/core/infra/services) repeat a similar shape (compute
allowed, push violation, continue). A small declarative rule table ({layer, allowedLayers, allowedRootModules, message}) driven by a shared loop would reduce duplication and make adding a fifth layer or adjusting an allowlist less error-prone.🤖 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 `@scripts/architecture-guard.mjs` around lines 259 - 352, Refactor checkMemoryPresenterLayerImports to use a table-driven rules structure instead of four separate layer branches. The current root/core/infra/services blocks in memoryPresenterLayer and isMemoryPresenterFacade follow the same pattern of computing an allowed condition and pushing a violation; consolidate that logic into a shared loop with per-layer config for allowed imported layers, root-module allowlists, and violation messages. Keep the existing behavior and symbols like isAllowedMemoryPresenterCoreRootModule, isAllowedMemoryPresenterRuntimeRootModule, and MEMORY_PRESENTER_SERVICE_LEAF_MODULES, but make the decision path data-driven so future layer changes are easier and less repetitive.test/main/presenter/memoryPresenter.test.ts (1)
1452-1532: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWhitebox reliance on private internal maps.
The new test casts
presenter as unknown as {...}to reach into six private fields (vectorStoreWarmups,backfilling,vectorStores,vectorStoreIdentities,vectorStoreReady, plusvectorStoreLocksin the earlier test) to assert lifecycle/cleanup behavior. This tightly couples the test suite to internal field names, so any future rename of these maps silently breaks tests with unclear failures. Given the PR's stated goal of "regression coverage focused on cleanup and vector cache convergence," this trade-off seems deliberate, but consider exposing a minimal test-only introspection hook (or asserting viacreateVectorStore/closecall counts alone) to reduce coupling to internal structure. Given the PR's explicit goal here, no action required now.🤖 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/memoryPresenter.test.ts` around lines 1452 - 1532, The test is reaching into MemoryPresenter’s private lifecycle maps via a cast, which makes it brittle against internal renames. Update the cleanup regression test around cleanupDeletedAgentResources and recall to avoid asserting on vectorStoreWarmups, backfilling, vectorStores, vectorStoreIdentities, and vectorStoreReady directly; prefer validating behavior through public outcomes and createVectorStore/close call counts, or add a small test-only introspection helper on MemoryPresenter if internal state must be observed.src/main/presenter/memoryPresenter/core/candidates.ts (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested ternary for
kindderivation is hard to scan.Consider extracting into a small named helper (e.g.
deriveKind(category, categoryWasProvided, candidateKind)) to make the branching explicit and easier to test in isolation.♻️ Proposed refactor
+function deriveKind( + category: AgentMemoryCategory | null, + categoryWasProvided: boolean, + candidateKind: MemoryCandidate['kind'] +): 'episodic' | 'semantic' { + if (category !== null) return category === 'task_outcome' ? 'episodic' : 'semantic' + if (categoryWasProvided) return 'semantic' + return candidateKind === 'episodic' || candidateKind === 'semantic' ? candidateKind : 'semantic' +} + export function normalizeMemoryCandidate( candidate: MemoryCandidate ): NormalizedMemoryCandidate | null { ... - const kind = - category !== null - ? category === 'task_outcome' - ? 'episodic' - : 'semantic' - : categoryWasProvided - ? 'semantic' - : candidate.kind === 'episodic' || candidate.kind === 'semantic' - ? candidate.kind - : 'semantic' + const kind = deriveKind(category, categoryWasProvided, candidate.kind)🤖 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/core/candidates.ts` around lines 20 - 29, The `kind` derivation in `candidates.ts` is using a nested ternary that is hard to read and maintain. Extract the branching logic into a small named helper such as `deriveKind(category, categoryWasProvided, candidateKind)` near the current `kind` calculation in `candidates.ts`, then have the existing code call that helper so the rules for `task_outcome`, provided categories, and fallback candidate kinds are explicit and easier to test.
🤖 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/presenter/memoryPresenter/context.ts`:
- Around line 59-61: The canContinueAgentMemoryTask guard currently ignores the
disposed state, so long-running embedding/reindex/backfill loops can keep
running after dispose() has been called. Update the canContinueAgentMemoryTask
method in context.ts to include the same !this.disposed check used by the
read/write guards, alongside isManagedAgent(agentId) and isEnabled(agentId), so
the runtime stops continuing memory tasks once disposed.
In `@src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts`:
- Around line 120-145: The vector-write path in embeddingPipeline.ts is still
letting stale rows be marked as errors because the later loop uses the original
records instead of the revalidated live set. Update the logic around the live
filtering and the return from the write path so that only rows that remain
pending/embeddable at write time are tracked for outcome handling, and ensure
the error-marking branch consumes the same live list used by store.upsert and
the written Set. Apply the same fix in the corresponding duplicate flow
mentioned in the comment so both paths consistently avoid marking rows that
changed state during embedding generation.
- Around line 212-219: The requeue/reindex flow in embeddingPipeline’s
runReindex currently calls repository.requeueForEmbedding() before waiting on
the active embeddingDrains promise, which can let an in-flight drain update rows
after they’ve been requeued and cause drainUntilExhausted() to skip them. Fix
this by awaiting any existing drain from this.embeddingDrains for the agentId
before calling requeueForEmbedding(), keeping the rest of the runReindex logic
and the force/requeued checks intact.
In `@src/main/presenter/memoryPresenter/infra/vectorStoreManager.ts`:
- Around line 166-175: The shutdown path in closeAllStores() is clearing
vectorStoreLocks too early, which can let queued withAgentLock() work resume and
repopulate stores after cleanup. Update VectorStoreManager.closeAllStores() to
drain or await the per-agent lock chains before clearing the maps, or perform
each close through the same withAgentLock() flow so no pending task can reopen a
store. Keep the existing store-close loop in sync with vectorStores,
vectorStoreIdentities, vectorStoreReady, and vectorStoreLocks so shutdown fully
quiesces the manager.
In `@src/main/presenter/memoryPresenter/services/conflictService.ts`:
- Around line 146-152: The conflict-resolution flow in conflictService’s
decision handling is discarding merged content for UPDATE and SUPERSEDE outcomes
by mapping them to keep_challenger. Update the logic around the outcome
selection and the subsequent resolveConflict call so that when decision.decision
is UPDATE or SUPERSEDE, the mergedContent from the decision is preserved and
written back through the conflict resolution path instead of being ignored. Use
the existing resolveConflict method and related decision handling in
conflictService to locate the change.
- Around line 55-63: In resolveConflict(), the mutation path currently only
checks isManagedAgent() before calling applyConflictResolution() and syncing
memory; add the same write-permission guard used by the other write path and
return false when the agent is not allowed to write. Recheck permission after
assertSafeAgentId() and before applyConflictResolution(), using the existing
context guard method on this.ctx so the conflict resolution cannot mutate
repository state without write access.
In `@src/main/presenter/memoryPresenter/services/maintenanceService.ts`:
- Around line 155-156: The fire-and-forget warmVectorStore call in
maintenanceService should not be left with an unhandled rejection. Update the
code around warmVectorStore and warmEmbeddingConnection in MaintenanceService so
the background prewarm Promise has an explicit rejection handler, logging or
otherwise handling failures without affecting the main flow. Keep the existing
non-blocking behavior, but ensure the warmVectorStore call cannot produce an
unhandled promise rejection.
- Around line 331-334: The duplicate consolidation flow in maintenanceService’s
active row selection is too broad because listByAgent(agentId) only excludes
persona rows, so working, archived, conflicted, and superseded rows can still be
merged. Tighten the filter in the active selection logic to only include truly
eligible rows for consolidation, using the existing repository row fields/status
flags in this method before sorting by created_at. Keep the change localized
around the active query in maintenanceService so downstream merge decisions only
see active, non-working rows.
In `@src/main/presenter/memoryPresenter/services/managementService.ts`:
- Around line 278-288: The clear path in managementService.clearMemories()
removes rows and resets the agent store, but it never refreshes working memory.
Update the clear flow to call syncWorkingMemoryAfterMutation(agentId) after the
mutation completes, alongside the existing emitChanged/reset logic, so cached
working-memory state is invalidated just like other mutation methods in
ManagementService.
In `@src/main/presenter/memoryPresenter/services/retrievalService.ts`:
- Around line 163-165: The background warmup in retrievalService’s isWarm path
can still trigger an unhandled rejection because warmVectorStore is
fired-and-forgotten. Update the retrievalService flow to explicitly handle the
promise from this.ports.warmVectorStore(agentId, currentEmbedding, { delayOpen:
true }) by attaching a catch handler or wrapping it in async error handling, and
log or otherwise absorb failures so the background warmup does not escape as an
unhandled rejection. Keep warmEmbeddingConnection behavior unchanged.
---
Nitpick comments:
In `@scripts/architecture-guard.mjs`:
- Around line 259-352: Refactor checkMemoryPresenterLayerImports to use a
table-driven rules structure instead of four separate layer branches. The
current root/core/infra/services blocks in memoryPresenterLayer and
isMemoryPresenterFacade follow the same pattern of computing an allowed
condition and pushing a violation; consolidate that logic into a shared loop
with per-layer config for allowed imported layers, root-module allowlists, and
violation messages. Keep the existing behavior and symbols like
isAllowedMemoryPresenterCoreRootModule,
isAllowedMemoryPresenterRuntimeRootModule, and
MEMORY_PRESENTER_SERVICE_LEAF_MODULES, but make the decision path data-driven so
future layer changes are easier and less repetitive.
In `@src/main/presenter/memoryPresenter/core/candidates.ts`:
- Around line 20-29: The `kind` derivation in `candidates.ts` is using a nested
ternary that is hard to read and maintain. Extract the branching logic into a
small named helper such as `deriveKind(category, categoryWasProvided,
candidateKind)` near the current `kind` calculation in `candidates.ts`, then
have the existing code call that helper so the rules for `task_outcome`,
provided categories, and fallback candidate kinds are explicit and easier to
test.
In `@test/main/presenter/memoryPresenter.test.ts`:
- Around line 1452-1532: The test is reaching into MemoryPresenter’s private
lifecycle maps via a cast, which makes it brittle against internal renames.
Update the cleanup regression test around cleanupDeletedAgentResources and
recall to avoid asserting on vectorStoreWarmups, backfilling, vectorStores,
vectorStoreIdentities, and vectorStoreReady directly; prefer validating behavior
through public outcomes and createVectorStore/close call counts, or add a small
test-only introspection helper on MemoryPresenter if internal state must be
observed.
🪄 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
Run ID: 8e31c3d9-cfb3-4bed-ae4e-86fb2e4aec3b
📒 Files selected for processing (40)
docs/architecture/agent-memory-system/spec.mdscripts/architecture-guard.mjssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/memoryPresenter/context.tssrc/main/presenter/memoryPresenter/core/candidates.tssrc/main/presenter/memoryPresenter/core/decision.tssrc/main/presenter/memoryPresenter/core/extraction.tssrc/main/presenter/memoryPresenter/core/injectionPort.tssrc/main/presenter/memoryPresenter/core/lifecycle.tssrc/main/presenter/memoryPresenter/core/recallKeyword.tssrc/main/presenter/memoryPresenter/core/scoring.tssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/infra/memoryVectorStore.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/injection.tssrc/main/presenter/memoryPresenter/lifecycleConstants.tssrc/main/presenter/memoryPresenter/ports.tssrc/main/presenter/memoryPresenter/runtimeConstants.tssrc/main/presenter/memoryPresenter/services/conflictService.tssrc/main/presenter/memoryPresenter/services/maintenanceService.tssrc/main/presenter/memoryPresenter/services/managementService.tssrc/main/presenter/memoryPresenter/services/personaService.tssrc/main/presenter/memoryPresenter/services/reflectionService.tssrc/main/presenter/memoryPresenter/services/retrievalService.tssrc/main/presenter/memoryPresenter/services/rowMutations.tssrc/main/presenter/memoryPresenter/services/workingMemoryService.tssrc/main/presenter/memoryPresenter/services/writeCoordinator.tssrc/main/presenter/memoryPresenter/types.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memoryDecision.test.tstest/main/presenter/memoryExtraction.test.tstest/main/presenter/memoryInjectionPort.test.tstest/main/presenter/memoryLifecycle.test.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryRetrieval.eval.test.tstest/main/presenter/memoryVectorStore.test.tstest/main/presenter/recallKeyword.test.tstest/main/scripts/architectureGuard.test.ts
💤 Files with no reviewable changes (1)
- src/main/presenter/memoryPresenter/lifecycleConstants.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 `@test/main/presenter/memoryPresenter.test.ts`:
- Around line 2649-2651: The test around presenter.reindexEmbeddings should
assert the reindex has not started until the active embedding drain finishes,
since it can currently pass even if the call resets or requeues too early. In
memoryPresenter.test.ts, use the reindexEmbeddings('a', true) promise and the
resolvers queue to add a pre-release check before releasing the first drain,
verifying no second resolver/start has been scheduled yet; then release the
first resolver and continue the existing assertions. This tightens the wait
contract without changing the implementation under test.
🪄 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
Run ID: 33bb8070-31db-48f9-be8a-0dbecc7c16db
📒 Files selected for processing (11)
src/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/memoryPresenter/context.tssrc/main/presenter/memoryPresenter/core/candidates.tssrc/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/services/conflictService.tssrc/main/presenter/memoryPresenter/services/maintenanceService.tssrc/main/presenter/memoryPresenter/services/managementService.tssrc/main/presenter/memoryPresenter/services/retrievalService.tssrc/main/presenter/sqlitePresenter/tables/configTables.tstest/main/presenter/memoryPresenter.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/main/presenter/sqlitePresenter/tables/configTables.ts
- src/main/presenter/agentSessionPresenter/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/presenter/memoryPresenter/core/candidates.ts
- src/main/presenter/memoryPresenter/context.ts
- src/main/presenter/memoryPresenter/services/conflictService.ts
- src/main/presenter/memoryPresenter/infra/vectorStoreManager.ts
- src/main/presenter/memoryPresenter/services/retrievalService.ts
- src/main/presenter/memoryPresenter/services/maintenanceService.ts
- src/main/presenter/memoryPresenter/infra/embeddingPipeline.ts
- src/main/presenter/memoryPresenter/services/managementService.ts
Collaborator
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split MemoryPresenter internals into layered context, core, infra, and service modules while preserving the public facade and runtime behavior.
Add narrow ports, vector store lock boundaries, lifecycle cleanup contracts, architecture guards, and focused regression coverage for cleanup and vector cache convergence.
Summary by CodeRabbit