feat(memory): add quality gates and observability - #1947
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
✅ Files skipped from review due to trivial changes (18)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThis PR adds scoped memory test gates, deterministic retrieval evaluation, bounded runtime diagnostics, health-contract and UI support, service-oriented test harnesses, and extensive memory lifecycle, retrieval, embedding, maintenance, and provider coverage. ChangesMemory quality gates and evaluation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryPresenter
participant MemoryServices
participant MemoryDiagnosticsCollector
participant MemoryHealthUI
MemoryPresenter->>MemoryServices: execute memory operations
MemoryServices->>MemoryDiagnosticsCollector: record diagnostics
MemoryPresenter->>MemoryDiagnosticsCollector: snapshot(agentId)
MemoryDiagnosticsCollector-->>MemoryPresenter: MemoryRuntimeDiagnosticsDto
MemoryPresenter->>MemoryHealthUI: return health.runtime
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/main/presenter/memory/conflictService.test.ts (1)
6-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
serviceTestSupportimports into one.
memoryRuntimeForTestsis imported in a separate statement from the same module already imported above; combine into a single import for clarity and to avoid tripping duplicate-import lint rules.♻️ Proposed fix
import { DAY, decisionCalls, embeddingConfig, makeLLMPresenter, + memoryRuntimeForTests, routedLLM, seedConflicted, seedEmbedded } from './serviceTestSupport' - -import { memoryRuntimeForTests } from './serviceTestSupport'🤖 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/memory/conflictService.test.ts` around lines 6 - 16, Combine the two imports from serviceTestSupport in conflictService.test.ts into one import declaration, adding memoryRuntimeForTests to the existing named import list and preserving all current imports.src/main/presenter/agentRuntimePresenter/index.ts (1)
663-667: 🚀 Performance & Scalability | 🔵 TrivialMemory extraction queue tracking looks correct.
Cleanup/observation ordering is sound: entries are removed exactly once (on session destroy or task completion via
finally), andobserveMemoryExtractionQueueis re-invoked after every mutation somemoryPortdiagnostics stay in sync. Deleting from theMapwhile iterating it indestroySessionis safe per the Map iteration spec.One thing worth keeping in view operationally:
observeMemoryExtractionQueuerescans the entire process-wide queue on every enqueue/dequeue. This is fine while extraction stays serialized per session and the queue stays small, but if the number of concurrently-queued sessions grows significantly, this becomes an O(n) hot path called at high frequency.Also applies to: 930-935, 2599-2611, 2627-2635
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 663 - 667, The memoryExtractionQueue observation path performs a full process-wide scan on every mutation, which may become a high-frequency O(n) hot path. Optimize observeMemoryExtractionQueue and its enqueue/dequeue callers to maintain equivalent memoryPort diagnostics without rescanning the entire queue for each change, while preserving the existing cleanup and ordering behavior.test/main/presenter/memory/serviceTestSupport.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTyped internals seam is missing
runChallengeResolutionPass, forcing an unsafeas anycast at its only consumer. TheMemoryPresenterRuntimeTestSeams.conflicttype only declaresrepairConflictIntegrity, so the one test that needs to spy onrunChallengeResolutionPasscan't use the typedmemoryRuntimeForTests()helper and falls back to bypassing type-checking entirely.
test/main/presenter/memory/serviceTestSupport.ts#L56-63: add arunChallengeResolutionPass(...)signature to theconflictmember ofMemoryPresenterRuntimeTestSeams(and expose it viamemoryRuntimeForTests().conflictService).test/main/presenter/memory/maintenanceService.test.ts#L256-263: once typed, replacevi.spyOn((presenter as any).conflict, 'runChallengeResolutionPass')withvi.spyOn(memoryRuntimeForTests(presenter).conflictService, 'runChallengeResolutionPass').🤖 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/memory/serviceTestSupport.ts` at line 1, Add runChallengeResolutionPass(...) to the conflict member of MemoryPresenterRuntimeTestSeams and expose that member through memoryRuntimeForTests().conflictService. In maintenanceService.test.ts, replace the unsafe presenter conflict cast in the vi.spyOn call with memoryRuntimeForTests(presenter).conflictService, preserving the existing method spy behavior.
26-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider Vitest's built-in polling primitive instead of a hand-rolled fixed-window poller.
waitForMemoryConditionpolls at a fixed 20×10ms (~200ms) budget and is reused across every test file in this PR for background-refresh/race assertions. Vitest'svi.waitFor/vi.waitUntilprovide the same retry-until-success semantics with configurableinterval/timeoutper call site, which would let flakier assertions opt into more slack without touching this shared helper.// vitest exposes this natively, e.g.: await vi.waitFor(() => condition(), { timeout: 2000, interval: 10 })🤖 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/memory/serviceTestSupport.ts` around lines 26 - 35, Replace the hand-rolled retry loop in waitForMemoryCondition with Vitest’s built-in polling primitive, preserving the condition check and failure message while allowing callers to configure timeout and interval per assertion. Update all call sites to pass appropriate polling options where needed, especially background-refresh and race assertions.
🤖 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/infra/diagnostics/memoryDiagnosticsCollector.ts`:
- Around line 29-31: Normalize capacity options before they are used by the
memory diagnostics collector: convert maxAgents and sampleCapacity to finite
positive integers, applying appropriate defaults for NaN, zero, negative,
fractional, or infinite values. Update the constructor and the related ring/map
initialization and eviction comparisons to use the normalized capacities,
including the logic referenced around the collector’s options handling.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Line 2939: Update the embeddingBacklog translation to use the established
“plongement” terminology consistently with the nearby embedding translations,
replacing “incorporation” while preserving the intended backlog meaning.
In `@test/renderer/components/MemoryDiagnosticsPanel.test.ts`:
- Around line 147-185: The provider admission summary in providerEventSummary
currently hardcodes user-facing abbreviations such as RL, CAP, D, A, and L.
Update providerEventSummary in MemoryDiagnosticsPanel.vue to obtain each label
through vue-i18n translation keys and compose the summary from those localized
values, preserving the existing counts and ordering.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 663-667: The memoryExtractionQueue observation path performs a
full process-wide scan on every mutation, which may become a high-frequency O(n)
hot path. Optimize observeMemoryExtractionQueue and its enqueue/dequeue callers
to maintain equivalent memoryPort diagnostics without rescanning the entire
queue for each change, while preserving the existing cleanup and ordering
behavior.
In `@test/main/presenter/memory/conflictService.test.ts`:
- Around line 6-16: Combine the two imports from serviceTestSupport in
conflictService.test.ts into one import declaration, adding
memoryRuntimeForTests to the existing named import list and preserving all
current imports.
In `@test/main/presenter/memory/serviceTestSupport.ts`:
- Line 1: Add runChallengeResolutionPass(...) to the conflict member of
MemoryPresenterRuntimeTestSeams and expose that member through
memoryRuntimeForTests().conflictService. In maintenanceService.test.ts, replace
the unsafe presenter conflict cast in the vi.spyOn call with
memoryRuntimeForTests(presenter).conflictService, preserving the existing method
spy behavior.
- Around line 26-35: Replace the hand-rolled retry loop in
waitForMemoryCondition with Vitest’s built-in polling primitive, preserving the
condition check and failure message while allowing callers to configure timeout
and interval per assertion. Update all call sites to pass appropriate polling
options where needed, especially background-refresh and race assertions.
🪄 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: be4fd84f-ee28-4f26-9ffb-cf7a582d4d12
📒 Files selected for processing (98)
.github/workflows/prcheck.ymldocs/architecture/agent-memory-system/spec.mddocs/architecture/agent-memory-test-seams/spec.mddocs/architecture/memory-domain-contract-convergence/spec.mddocs/architecture/memory-domain-contract-convergence/tasks.mddocs/architecture/memory-quality-gates-and-observability/metrics.mddocs/architecture/memory-quality-gates-and-observability/plan.mddocs/architecture/memory-quality-gates-and-observability/spec.mddocs/architecture/memory-quality-gates-and-observability/tasks.mdpackage.jsonscripts/check-memory-test-scope.mjsscripts/generate-memory-retrieval-fixture.mjssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/memoryPresenter/core/injectionPort.tssrc/main/presenter/memoryPresenter/core/maintenanceBudget.tssrc/main/presenter/memoryPresenter/index.tssrc/main/presenter/memoryPresenter/infra/diagnostics/memoryDiagnosticsCollector.tssrc/main/presenter/memoryPresenter/infra/embeddingPipeline.tssrc/main/presenter/memoryPresenter/infra/providerGateway.tssrc/main/presenter/memoryPresenter/infra/vectorStoreManager.tssrc/main/presenter/memoryPresenter/ports.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/workingMemoryService.tssrc/main/presenter/memoryPresenter/services/writeCoordinator.tssrc/main/presenter/sqlitePresenter/tables/agentMemory.tssrc/main/routes/debug/createMockChatSession.tssrc/renderer/settings/components/MemoryDiagnosticsPanel.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/contracts/routes/memory.routes.tssrc/shared/types/agent-memory.tstest/fixtures/memory/retrieval-v1.jsontest/helpers/memoryRetrievalEval.tstest/main/performance/memory/workloadBounds.perf.tstest/main/presenter/agentSessionPresenter/integration.test.tstest/main/presenter/fakes/memoryFakes.tstest/main/presenter/memory-persona-eval.test.tstest/main/presenter/memory/conflictService.test.tstest/main/presenter/memory/embeddingDiagnostics.test.tstest/main/presenter/memory/embeddingPipeline.test.tstest/main/presenter/memory/lifecycleRegression.test.tstest/main/presenter/memory/maintenanceDiagnostics.test.tstest/main/presenter/memory/maintenanceService.test.tstest/main/presenter/memory/managementService.test.tstest/main/presenter/memory/personaService.test.tstest/main/presenter/memory/reflectionService.test.tstest/main/presenter/memory/repositoryHarness.test.tstest/main/presenter/memory/retrievalDiagnostics.test.tstest/main/presenter/memory/retrievalService.test.tstest/main/presenter/memory/serviceHarness.tstest/main/presenter/memory/serviceTestSupport.tstest/main/presenter/memory/vectorStoreManager.test.tstest/main/presenter/memory/workingMemoryService.test.tstest/main/presenter/memory/writeCoordinator.test.tstest/main/presenter/memoryAdd.test.tstest/main/presenter/memoryDiagnosticsCollector.test.tstest/main/presenter/memoryEmbeddingScale.test.tstest/main/presenter/memoryExtraction.test.tstest/main/presenter/memoryLifecycle.test.tstest/main/presenter/memoryMaintenanceBudget.test.tstest/main/presenter/memoryPresenter.test.tstest/main/presenter/memoryProviderGateway.test.tstest/main/presenter/memoryRetrieval.eval.test.tstest/main/presenter/memoryServiceHarness.test.tstest/main/presenter/memorySessionExtractionLock.test.tstest/main/presenter/memoryUpdate.test.tstest/main/routes/memoryDto.test.tstest/main/scripts/memoryTestScope.test.tstest/memory-test-scope.jsontest/renderer/components/MemoryDiagnosticsPanel.test.tstest/renderer/components/SkillsSettingsAgentScope.test.tsvitest.config.memory-eval.tsvitest.config.memory-native.tsvitest.config.memory-perf.tsvitest.config.memory-shared.tsvitest.config.memory.ts
💤 Files with no reviewable changes (3)
- src/main/presenter/memoryPresenter/services/workingMemoryService.ts
- src/main/presenter/memoryPresenter/services/reflectionService.ts
- src/main/presenter/memoryPresenter/services/personaService.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 by CodeRabbit
runtimediagnostics payload (including deterministic empty-state values).