diff --git a/docs/architecture/agent-memory-test-seams/plan.md b/docs/architecture/agent-memory-test-seams/plan.md new file mode 100644 index 000000000..6f4afc148 --- /dev/null +++ b/docs/architecture/agent-memory-test-seams/plan.md @@ -0,0 +1,25 @@ +# Agent Memory Test Seams Plan + +## Implementation Approach + +- Add a small test-local helper in `memoryPresenter.test.ts` that exposes service-level runtime + states through `getMutableRuntimeStateForTests()`. +- Replace facade-level runtime casts with helper calls against `embedding`, `vectorStore`, + `maintenance`, `reflection`, `persona`, and `workingMemory`. +- Replace facade wrapper calls with direct service calls: + - `embedding.warmEmbeddingConnection(...)` + - `vectorStore.clearReady(...)` +- Delete the `MemoryPresenter` private compat accessor method, private runtime getters, and private + wrapper methods. + +## Compatibility + +No production behavior, persisted data, public route DTO, IPC contract, or memory runtime behavior +changes. The only intended contract change is test-only: facade private runtime shims are no longer +available. + +## Test Strategy + +- Run `memoryPresenter.test.ts` to verify the moved seams preserve existing coverage. +- Run node typecheck to catch private-member cast type drift. +- Run format, i18n, lint, and diff whitespace checks before handoff. diff --git a/docs/architecture/agent-memory-test-seams/spec.md b/docs/architecture/agent-memory-test-seams/spec.md new file mode 100644 index 000000000..9ae7fe6cc --- /dev/null +++ b/docs/architecture/agent-memory-test-seams/spec.md @@ -0,0 +1,32 @@ +# Agent Memory Test Seams Spec + +## User Need + +Agent memory tests should keep their lifecycle and race coverage without forcing `MemoryPresenter` +to retain facade-level private accessors that exist only for tests. + +## Goal + +Remove the legacy `MemoryPresenter` runtime accessor shims and migrate affected tests to the +existing service-level `getMutableRuntimeStateForTests()` seams. + +## Acceptance Criteria + +- `MemoryPresenter` no longer contains `retainRuntimeCompatAccessorsForTests()` or private runtime + getter wrappers. +- `memoryPresenter.test.ts` no longer casts the presenter facade to read `vectorStoreReady`, + `embeddingDrains`, `personaLocks`, or related compat getters. +- Existing lifecycle, cleanup, cold-path, cooldown, and dispose tests remain present and pass. +- Production memory behavior and public APIs are unchanged. + +## Constraints + +- Do not create a GitHub issue, branch, or commit. +- Keep this as a local refactor; do not rewrite tests into broader behavior-only harnesses. +- Preserve the existing service-level test seams for this change. + +## Non-Goals + +- Do not address native SQLite environment setup. +- Do not remove valuable lifecycle tests. +- Do not broaden memory service APIs or export new production types only for tests. diff --git a/docs/architecture/agent-memory-test-seams/tasks.md b/docs/architecture/agent-memory-test-seams/tasks.md new file mode 100644 index 000000000..a248d1539 --- /dev/null +++ b/docs/architecture/agent-memory-test-seams/tasks.md @@ -0,0 +1,7 @@ +# Agent Memory Test Seams Tasks + +- [x] Create local architecture SDD artifacts without GitHub issue sync. +- [x] Add service-level runtime helper to `memoryPresenter.test.ts`. +- [x] Migrate facade runtime accessor probes to service-level seams. +- [x] Remove `MemoryPresenter` facade test shims. +- [x] Run targeted tests and validation commands through `mise exec -- pnpm ...`. diff --git a/src/main/presenter/memoryPresenter/index.ts b/src/main/presenter/memoryPresenter/index.ts index 5f90c1ca6..d1c3618a5 100644 --- a/src/main/presenter/memoryPresenter/index.ts +++ b/src/main/presenter/memoryPresenter/index.ts @@ -7,7 +7,7 @@ import { type MemoryInjectionResult, type MemoryRuntimePort } from './injection' -import { isSafeAgentId, type AgentMemoryRow, type IMemoryVectorStore } from './types' +import { isSafeAgentId, type AgentMemoryRow } from './types' import type { MemoryCandidate, MemoryConflictPair, @@ -165,31 +165,6 @@ export class MemoryPresenter implements MemoryRuntimePort { triggerEmbedding: (agentId) => this.embedding.processPendingEmbeddings(agentId), clearConsolidationCooldown: (agentId) => this.maintenance.clearCooldown(agentId) }) - - this.retainRuntimeCompatAccessorsForTests() - } - - // Legacy facade-oracle tests intentionally probe these private runtime accessors via casts. - // Keep this method until those tests move to explicit service-level test helpers. - private retainRuntimeCompatAccessorsForTests(): void { - void this.vectorStoreReady - void this.vectorStores - void this.vectorStoreIdentities - void this.vectorStoreLocks - void this.vectorStoreWarmups - void this.vectorStoreDimensionFailures - void this.embeddingWarmups - void this.embeddingDrains - void this.reindexing - void this.backfilling - void this.consolidationTimers - void this.lastConsolidationAt - void this.reflectionAttemptWatermark - void this.personaAttemptWatermark - void this.personaLocks - void this.workingRefreshInFlight - void this.warmEmbeddingConnection - void this.clearVectorStoreReady } startBackgroundMaintenance(): void { @@ -204,81 +179,6 @@ export class MemoryPresenter implements MemoryRuntimePort { this.maintenance.warmActiveAgents() } - private get vectorStoreReady(): Map { - return this.vectorStore.getMutableRuntimeStateForTests().vectorStoreReady - } - - private get vectorStores(): Map> { - return this.vectorStore.getMutableRuntimeStateForTests().vectorStores - } - - private get vectorStoreIdentities(): Map { - return this.vectorStore.getMutableRuntimeStateForTests().vectorStoreIdentities - } - - private get vectorStoreLocks(): Map> { - return this.vectorStore.getMutableRuntimeStateForTests().vectorStoreLocks - } - - private get vectorStoreWarmups(): Map> { - return this.embedding.getMutableRuntimeStateForTests().vectorStoreWarmups - } - - private get vectorStoreDimensionFailures(): Map { - return this.embedding.getMutableRuntimeStateForTests().vectorStoreDimensionFailures - } - - private get embeddingWarmups(): Map> { - return this.embedding.getMutableRuntimeStateForTests().embeddingWarmups - } - - private get embeddingDrains(): Map> { - return this.embedding.getMutableRuntimeStateForTests().embeddingDrains - } - - private get reindexing(): Map> { - return this.embedding.getMutableRuntimeStateForTests().reindexing - } - - private get backfilling(): Map> { - return this.embedding.getMutableRuntimeStateForTests().backfilling - } - - private get consolidationTimers(): Map { - return this.maintenance.getMutableRuntimeStateForTests().consolidationTimers - } - - private get lastConsolidationAt(): Map { - return this.maintenance.getMutableRuntimeStateForTests().lastConsolidationAt - } - - private get reflectionAttemptWatermark(): Map { - return this.reflection.getMutableRuntimeStateForTests().reflectionAttemptWatermark - } - - private get personaAttemptWatermark(): Map { - return this.persona.getMutableRuntimeStateForTests().personaAttemptWatermark - } - - private get personaLocks(): Map> { - return this.persona.getMutableRuntimeStateForTests().personaLocks - } - - private get workingRefreshInFlight(): Set { - return this.workingMemory.getMutableRuntimeStateForTests().workingRefreshInFlight - } - - private warmEmbeddingConnection( - agentId: string, - embedding: { providerId: string; modelId: string } - ): void { - this.embedding.warmEmbeddingConnection(agentId, embedding) - } - - private clearVectorStoreReady(agentId: string): void { - this.vectorStore.clearReady(agentId) - } - isEnabled(agentId: string): boolean { return this.runtime.isEnabled(agentId) } diff --git a/test/main/presenter/memoryPresenter.test.ts b/test/main/presenter/memoryPresenter.test.ts index 53e07653a..84c2f5b96 100644 --- a/test/main/presenter/memoryPresenter.test.ts +++ b/test/main/presenter/memoryPresenter.test.ts @@ -54,6 +54,74 @@ async function flushMicrotasks(cycles = 3): Promise { } } +type MemoryPresenterRuntimeTestSeams = { + embedding: { + warmEmbeddingConnection( + agentId: string, + embedding: { providerId: string; modelId: string } + ): void + getMutableRuntimeStateForTests(): { + embeddingWarmups: Map> + vectorStoreWarmups: Map> + vectorStoreDimensionFailures: Map + embeddingDrains: Map> + reindexing: Map> + backfilling: Map> + errorRetryAt: Map + errorRetryAfterId: Map + orphanVectorReconciles: Map> + orphanVectorReconciled: Set + orphanVectorReconcileRetryAt: Map + } + } + vectorStore: { + clearReady(agentId: string): void + getMutableRuntimeStateForTests(): { + vectorStores: Map> + vectorStoreIdentities: Map + vectorStoreReady: Map + vectorStoreLocks: Map> + } + } + maintenance: { + getMutableRuntimeStateForTests(): { + consolidationTimers: Map + lastConsolidationAt: Map + } + } + reflection: { + getMutableRuntimeStateForTests(): { reflectionAttemptWatermark: Map } + } + persona: { + getMutableRuntimeStateForTests(): { + personaAttemptWatermark: Map + personaLocks: Map> + } + } + workingMemory: { + getMutableRuntimeStateForTests(): { workingRefreshInFlight: Set } + } +} + +function memoryRuntimeForTests(presenter: MemoryPresenter) { + const internals = presenter as unknown as MemoryPresenterRuntimeTestSeams + return { + embeddingService: internals.embedding, + vectorStoreService: internals.vectorStore, + ...internals.embedding.getMutableRuntimeStateForTests(), + ...internals.vectorStore.getMutableRuntimeStateForTests(), + ...internals.maintenance.getMutableRuntimeStateForTests(), + ...internals.reflection.getMutableRuntimeStateForTests(), + ...internals.persona.getMutableRuntimeStateForTests(), + ...internals.workingMemory.getMutableRuntimeStateForTests(), + warmEmbeddingConnection: ( + agentId: string, + embedding: { providerId: string; modelId: string } + ) => internals.embedding.warmEmbeddingConnection(agentId, embedding), + clearVectorStoreReady: (agentId: string) => internals.vectorStore.clearReady(agentId) + } +} + describe('memory repository fakes', () => { it('matches AgentMemoryTable list limit lower-clamp behavior without an upper cap', () => { const repo = new FakeRepository() @@ -1369,21 +1437,7 @@ describe('MemoryPresenter management', () => { embeddingDim: textToVector('').length, embeddingModel: 'p:m' }) - const internals = presenter as unknown as { - lastConsolidationAt: Map - reflectionAttemptWatermark: Map - personaAttemptWatermark: Map - consolidationTimers: Map - personaLocks: Map> - workingRefreshInFlight: Set - reindexing: Map> - backfilling: Map> - embeddingDrains: Map> - vectorStores: Map> - vectorStoreIdentities: Map - vectorStoreReady: Map - vectorStoreLocks: Map> - } + const internals = memoryRuntimeForTests(presenter) await presenter.recall('a', 'redis') await waitForMemoryCondition( () => internals.vectorStoreReady.has('a'), @@ -1448,9 +1502,7 @@ describe('MemoryPresenter management', () => { const [id] = presenter.writeMemoriesSync([{ kind: 'semantic', content: 'redis' }], { agentId: 'a' }) - const internals = presenter as unknown as { - embeddingDrains: Map> - } + const internals = memoryRuntimeForTests(presenter) const drain = presenter.processPendingEmbeddings('a') await new Promise((resolve) => setTimeout(resolve, 0)) @@ -1494,13 +1546,7 @@ describe('MemoryPresenter management', () => { createVectorStore: async () => new FakeVectorStore(), resetVectorStore: async () => undefined }) - const internals = presenter as unknown as { - warmEmbeddingConnection: ( - agentId: string, - embedding: { providerId: string; modelId: string } - ) => void - embeddingWarmups: Map> - } + const internals = memoryRuntimeForTests(presenter) internals.warmEmbeddingConnection('a', { providerId: 'p', modelId: 'm' }) await Promise.resolve() @@ -1563,13 +1609,7 @@ describe('MemoryPresenter management', () => { createVectorStore, resetVectorStore: async () => undefined }) - const internals = presenter as unknown as { - vectorStoreWarmups: Map> - backfilling: Map> - vectorStores: Map> - vectorStoreIdentities: Map - vectorStoreReady: Map - } + const internals = memoryRuntimeForTests(presenter) await presenter.recall('a', 'redis') await waitForMemoryCondition(() => resolveDimensions !== undefined) @@ -1633,9 +1673,7 @@ describe('MemoryPresenter management', () => { createdAt: 2000 + index }) } - const internals = presenter as unknown as { - personaLocks: Map> - } + const internals = memoryRuntimeForTests(presenter) const persona = presenter.maybeEvolvePersona('a', { providerId: 'p', modelId: 'm' }) await new Promise((resolve) => setTimeout(resolve, 0)) @@ -1713,6 +1751,7 @@ describe('MemoryPresenter management', () => { embeddingDim: 4, embeddingModel: 'p:m' }) + const internals = memoryRuntimeForTests(presenter) const first = await presenter.recall('a', 'redis') const second = await presenter.recall('a', 'redis') @@ -1727,10 +1766,7 @@ describe('MemoryPresenter management', () => { const querySpy = vi.spyOn(store, 'query') resolveCreate() await waitForMemoryCondition( - () => - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ), + () => internals.vectorStoreReady.has('a'), 'vector store did not become ready' ) @@ -1769,6 +1805,7 @@ describe('MemoryPresenter management', () => { embeddingDim: 4, embeddingModel: 'p:m' }) + const internals = memoryRuntimeForTests(presenter) const cold = await presenter.searchMemories('a', 'redis') expect(cold.map((hit) => hit.row.id)).toEqual(['m1']) @@ -1780,10 +1817,7 @@ describe('MemoryPresenter management', () => { const querySpy = vi.spyOn(store, 'query') resolveCreate() await waitForMemoryCondition( - () => - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ), + () => internals.vectorStoreReady.has('a'), 'vector store did not become ready' ) @@ -1949,18 +1983,12 @@ describe('MemoryPresenter management', () => { agentId: 'a' } ) + const internals = memoryRuntimeForTests(presenter) await presenter.processPendingEmbeddings('a') - expect( - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ) - ).toBe(true) + expect(internals.vectorStoreReady.has('a')).toBe(true) blockQueryEmbedding = true - const clearReadySpy = vi.spyOn( - presenter as unknown as { clearVectorStoreReady: (agentId: string) => void }, - 'clearVectorStoreReady' - ) + const clearReadySpy = vi.spyOn(internals.vectorStoreService, 'clearReady') const backfillSpy = vi.spyOn(presenter, 'backfillEmbeddings') const reindexSpy = vi.spyOn(presenter, 'reindexEmbeddings') const recall = presenter.recall('a', 'Could you explain the redis setup again?') @@ -1972,11 +2000,7 @@ describe('MemoryPresenter management', () => { expect(clearReadySpy).not.toHaveBeenCalled() expect(backfillSpy).not.toHaveBeenCalled() expect(reindexSpy).not.toHaveBeenCalled() - expect( - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ) - ).toBe(true) + expect(internals.vectorStoreReady.has('a')).toBe(true) } finally { vi.useRealTimers() } @@ -2173,10 +2197,7 @@ describe('MemoryPresenter management', () => { createVectorStore, resetVectorStore: async () => undefined }) - const internals = presenter as unknown as { - vectorStoreWarmups: Map> - vectorStoreDimensionFailures: Map - } + const internals = memoryRuntimeForTests(presenter) repo.insert({ id: 'm1', agentId: 'a', @@ -2279,6 +2300,7 @@ describe('MemoryPresenter management', () => { embeddingDim: 4, embeddingModel: 'p:m' }) + const internals = memoryRuntimeForTests(presenter) const outcome = await presenter.rememberMemory( { kind: 'semantic', content: newContent }, @@ -2298,10 +2320,7 @@ describe('MemoryPresenter management', () => { expect(createVectorStore).toHaveBeenCalledTimes(1) resolveCreate() await waitForMemoryCondition( - () => - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ), + () => internals.vectorStoreReady.has('a'), 'vector store did not become ready' ) }) @@ -3216,22 +3235,13 @@ describe('MemoryPresenter.processPendingEmbeddings (batch + fairness)', () => { }) const requeueSpy = vi.spyOn(repo, 'requeueForEmbedding') requeueSpy.mockReturnValueOnce(0) - const embeddingState = ( - presenter as unknown as { - embedding: { - getMutableRuntimeStateForTests(): { - errorRetryAt: Map - errorRetryAfterId: Map - } - } - } - ).embedding.getMutableRuntimeStateForTests() + const internals = memoryRuntimeForTests(presenter) await presenter.processPendingEmbeddings('a') expect(requeueSpy).toHaveBeenCalledTimes(1) - expect(embeddingState.errorRetryAt.has('a')).toBe(true) - expect(embeddingState.errorRetryAfterId.get('a')).toBe('err-01') + expect(internals.errorRetryAt.has('a')).toBe(true) + expect(internals.errorRetryAfterId.get('a')).toBe('err-01') await presenter.processPendingEmbeddings('a') @@ -3249,23 +3259,14 @@ describe('MemoryPresenter.processPendingEmbeddings (batch + fairness)', () => { status: 'error' }) } - const embeddingState = ( - presenter as unknown as { - embedding: { - getMutableRuntimeStateForTests(): { - errorRetryAt: Map - errorRetryAfterId: Map - } - } - } - ).embedding.getMutableRuntimeStateForTests() + const internals = memoryRuntimeForTests(presenter) await presenter.processPendingEmbeddings('a') expect(repo.getById('err-49')?.status).toBe('embedded') expect(repo.getById('err-50')?.status).toBe('error') - expect(embeddingState.errorRetryAfterId.get('a')).toBe('err-49') + expect(internals.errorRetryAfterId.get('a')).toBe('err-49') - embeddingState.errorRetryAt.set('a', 0) + internals.errorRetryAt.set('a', 0) await presenter.processPendingEmbeddings('a') expect(repo.getById('err-50')?.status).toBe('embedded') @@ -3297,21 +3298,13 @@ describe('MemoryPresenter.processPendingEmbeddings (batch + fairness)', () => { content: 'bad', status: 'pending_embedding' }) - const embeddingState = ( - presenter as unknown as { - embedding: { - getMutableRuntimeStateForTests(): { - errorRetryAt: Map - } - } - } - ).embedding.getMutableRuntimeStateForTests() + const internals = memoryRuntimeForTests(presenter) await presenter.processPendingEmbeddings('a') expect(repo.getById('ok')?.status).toBe('embedded') expect(repo.getById('bad')?.status).toBe('error') - expect(embeddingState.errorRetryAt.has('a')).toBe(true) + expect(internals.errorRetryAt.has('a')).toBe(true) }) }) @@ -3874,36 +3867,22 @@ describe('MemoryPresenter embedding reindex (T5, AC-3.x)', () => { store.vectors.set('orphan-0001', textToVector('orphan redis')) const deleteSpy = vi.spyOn(store, 'deleteByMemoryIds') deleteSpy.mockRejectedValueOnce(new Error('delete failed')) - const embeddingState = ( - presenter as unknown as { - embedding: { - getMutableRuntimeStateForTests(): { - orphanVectorReconcileRetryAt: Map - } - } - } - ).embedding.getMutableRuntimeStateForTests() + const internals = memoryRuntimeForTests(presenter) await presenter.recall('a', 'redis') await waitForMemoryCondition(() => deleteSpy.mock.calls.length === 1) expect(store.vectors.has('orphan-0001')).toBe(true) - expect( - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has('a') - ).toBe(true) + expect(internals.vectorStoreReady.has('a')).toBe(true) - ;( - presenter as unknown as { clearVectorStoreReady(agentId: string): void } - ).clearVectorStoreReady('a') + internals.clearVectorStoreReady('a') await presenter.recall('a', 'redis') await flushMicrotasks() expect(deleteSpy).toHaveBeenCalledTimes(1) - for (const key of embeddingState.orphanVectorReconcileRetryAt.keys()) { - embeddingState.orphanVectorReconcileRetryAt.set(key, 0) + for (const key of internals.orphanVectorReconcileRetryAt.keys()) { + internals.orphanVectorReconcileRetryAt.set(key, 0) } - ;( - presenter as unknown as { clearVectorStoreReady(agentId: string): void } - ).clearVectorStoreReady('a') + internals.clearVectorStoreReady('a') await presenter.recall('a', 'redis') await waitForMemoryCondition( () => deleteSpy.mock.calls.length >= 2 && !store.vectors.has('orphan-0001'), @@ -3920,20 +3899,11 @@ describe('MemoryPresenter embedding reindex (T5, AC-3.x)', () => { releaseList = () => resolve([]) }) ) - const embeddingState = ( - presenter as unknown as { - embedding: { - getMutableRuntimeStateForTests(): { - orphanVectorReconciles: Map> - orphanVectorReconciled: Set - } - } - } - ).embedding.getMutableRuntimeStateForTests() + const internals = memoryRuntimeForTests(presenter) await presenter.recall('a', 'redis') await waitForMemoryCondition( - () => embeddingState.orphanVectorReconciles.size === 1 && typeof releaseList === 'function', + () => internals.orphanVectorReconciles.size === 1 && typeof releaseList === 'function', 'orphan reconcile did not enter in-flight tracking' ) @@ -3948,8 +3918,8 @@ describe('MemoryPresenter embedding reindex (T5, AC-3.x)', () => { await cleanup expect(cleanupDone).toBe(true) - expect(embeddingState.orphanVectorReconciles.size).toBe(0) - expect(embeddingState.orphanVectorReconciled.size).toBe(0) + expect(internals.orphanVectorReconciles.size).toBe(0) + expect(internals.orphanVectorReconciled.size).toBe(0) }) it('reindexEmbeddings re-queues, rebuilds the store, and re-embeds with the new fingerprint', async () => { @@ -4347,13 +4317,7 @@ describe('MemoryPresenter dispose lifecycle (C4, AC-4.1)', () => { createVectorStore: async () => new FakeVectorStore(), resetVectorStore: async () => undefined }) - const internals = presenter as unknown as { - warmEmbeddingConnection: ( - agentId: string, - embedding: { providerId: string; modelId: string } - ) => void - embeddingWarmups: Map> - } + const internals = memoryRuntimeForTests(presenter) internals.warmEmbeddingConnection('a', { providerId: 'p', modelId: 'm' }) await Promise.resolve() @@ -6791,11 +6755,9 @@ describe('MemoryPresenter lifecycle revival (SDD-8)', () => { }) await presenter.recall('a', 'redis') + const internals = memoryRuntimeForTests(presenter) await waitForMemoryCondition( - () => - (presenter as unknown as { vectorStoreReady: Map }).vectorStoreReady.has( - 'a' - ), + () => internals.vectorStoreReady.has('a'), 'vector store did not become ready' )