# Greenfield Review: Agent Memory System (PR #193) **Scope:** `@coda/memory` package, server integration (`apps/server/src/ai/memory/`), supporting additions to `@coda/collections` and `@coda/common`, `apps/search` re-export cleanup, Redis prefix migration. **Date:** 2026-04-28 ## Summary The agent memory system is well-architected with clean interface boundaries, pluggable decay strategies, thorough null-object degradation, and solid test coverage. The core data model (Observation → Fact via consolidation) is sound and the integration into the server stream handler is minimally invasive. The main areas for improvement are: (1) the `upsertBatch` implementation issues sequential writes instead of pipelining, (2) the `content` field on new facts is set to the raw subject key rather than a human-readable string, (3) the `MemorySortedStore.rangeByScore` doesn't exploit the AVL tree's structure for range queries, and (4) the stream handler's `onToolExecution` callback has grown into a 40-line memory-emission block that should be extracted. ## Architecture Overview ### Components ``` ┌─────────────────────────────────────────────────────────────────┐ │ apps/server │ │ │ │ stream-handler.ts ──► MemoryService │ │ │ loadMemory() │ │ │ │ emitObservation() │ delegates to │ │ │ ▼ │ │ │ ┌──────────────────┐ │ │ │ │ @coda/memory │ │ │ │ │ consolidate() │ │ │ │ │ DecayStrategy │ │ │ │ └──────┬───────────┘ │ │ │ │ │ │ │ ┌──────┴───────────┐ │ │ │ │ Storage Layer │ │ │ │ │ ObservationLog │──► SortedStore (@coda/common) │ │ │ │ FactRepository │──► HashStore (@coda/common) │ │ │ │ AsyncMap │──► @coda/collections │ │ │ └──────────────────┘ │ │ │ │ │ ├──► prompt-formatter.ts (facts → system prompt section) │ │ ├──► affinity.ts (facts → tool affinity scores) │ │ └──► observation-emitters.ts (tool/entity/query → observations)│ │ │ │ catalog.ts ◄── toolAffinities (search ranking boost) │ └─────────────────────────────────────────────────────────────────┘ ``` ### Key Files | File | Role | | ---------------------------------------------------- | ------------------------------------------------------------- | | `packages/memory/src/types.ts` | Core domain types: `Observation`, `Fact`, category/type enums | | `packages/memory/src/observation-log.ts` | `ObservationLog` interface (append, since, prune) | | `packages/memory/src/fact-repository.ts` | `FactRepository` interface (CRUD for facts) | | `packages/memory/src/consolidate.ts` | Core pipeline: observations → facts with decay | | `packages/memory/src/decay/strategy.ts` | `DecayStrategy` interface | | `packages/memory/src/decay/exponential.ts` | Simple time-based exponential decay | | `packages/memory/src/decay/adaptive.ts` | SM-2 inspired adaptive decay | | `packages/memory/src/redis/redis-observation-log.ts` | SortedStore-backed observation log | | `packages/memory/src/redis/redis-fact-repository.ts` | HashStore-backed fact repository | | `packages/memory/src/null/*` | Null object implementations for graceful degradation | | `apps/server/src/ai/memory/memory-service.ts` | Server wrapper: timeout, fire-and-forget, drain | | `apps/server/src/ai/memory/observation-emitters.ts` | Tool/entity/query → observation factories | | `apps/server/src/ai/memory/prompt-formatter.ts` | Facts → system prompt markdown section | | `apps/server/src/ai/memory/affinity.ts` | Facts → tool affinity score map | | `apps/server/src/routes/stream-handler.ts` | Integration point: load → inject → emit | | `packages/common/src/storage/sorted-store.ts` | `SortedStore` interface + AVL-backed in-memory impl | | `packages/common/src/storage/hash-store.ts` | `HashStore` interface + in-memory impl | | `packages/collections/src/async-map.ts` | `AsyncMap` — async key-value primitive | ### Data Flow **Session start (consolidation):** ``` stream-handler.ts → memoryService.loadMemory(identityId) → consolidate({ observationLog, factRepository, checkpoint, decay }) → checkpoint.get(identityId) // last-consolidated timestamp → observationLog.since(identityId, ckpt) // new observations → collectFactKeys(observations) // dedup to unique fact keys → for each key: reinforce existing or create new fact → for unreinforced facts: decay.apply() → delete if below threshold → factRepository.upsertBatch(activeFacts) → checkpoint.set(identityId, latestTimestamp) → observationLog.prune(identityId, threshold) → formatMemorySection(facts) // markdown for system prompt → extractToolAffinities(facts) // Map → prepend promptSection to systemPrompt → pass toolAffinities to converseWithTools ``` **During session (observation emission):** ``` stream-handler.ts onToolExecution callback → emitToolObservations(toolUses, toolResults, timing) → memoryService.emitObservation() for each (fire-and-forget) → emitEntityObservations(toolName, resultData) → memoryService.emitObservation() for each (fire-and-forget) stream-handler.ts query entry → emitQueryPatternObservation(userQuery) → memoryService.emitObservation() (fire-and-forget) ``` ## What Works Well - **Clean interface boundaries.** `ObservationLog`, `FactRepository`, `DecayStrategy`, `SortedStore`, `HashStore` — all narrow, purpose-built interfaces that depend on nothing else. The Redis implementations are thin wrappers that do JSON serialization and key formatting. Swapping to real Redis sorted sets later requires zero changes to the memory package. - **Null object pattern throughout.** `NullObservationLog`, `NullFactRepository`, the `memoryService?: MemoryService` optional on `AppLocals`, and the `try/catch` in `loadMemory()` all compose into a system that degrades gracefully to stateless behavior when any component is unavailable. No callsite needs to guard against `undefined`. - **Fire-and-forget with drain.** The `inflight` set pattern in `MemoryService` is elegant — observations don't block the request, but the server waits for them on shutdown. This is the right tradeoff for best-effort memory. - **One reinforcement per session per fact.** The `collectFactKeys` deduplication is a smart design decision, well-documented in the code comment. It prevents a burst of identical tool calls from artificially inflating confidence. - **Consolidation idempotency.** Writing the checkpoint last means crash recovery produces bounded error (double-reinforcement) rather than data loss. The 7-day prune buffer reinforces this. - **Minimal integration surface.** The stream handler changes are ~40 lines of memory logic in a well-defined region. The catalog change is a single optional parameter. No existing code paths were modified in ways that could regress behavior. --- ## Findings ### 1. Fact `content` set to raw subject key on creation **Dimension:** Abstractions **Impact:** High **Current state:** When `consolidate()` creates a new fact (line 135 of `consolidate.ts`), `content` is set to `subject`: ```ts const newFact: Fact = { // ... content: subject, // e.g. "tool_affinity:get_account" }; ``` This means the prompt formatter injects strings like `"tool_affinity:get_account"` and `"entity_frequency:account:42"` directly into the system prompt. The user sees `**Tool preferences:** tool_affinity:get_account. tool_affinity:search_snowflake_schema.` — internal key notation, not human-readable text. **Greenfield alternative:** Generate a human-readable `content` string during fact creation. Either: - A `contentForFactKey(key: FactKey, observations: Observation[])` function that produces display text like `"Prefers get_account for lookups"` or `"Frequently accesses account Sony Music (acc-123)"`, drawing from the observation's own `content` field. - Or at minimum, strip the category prefix: `content: subject.slice(key.category.length + 1)` → `"get_account"` instead of `"tool_affinity:get_account"`. **Migration path:** Add a `contentFromFactKey` helper in `consolidate.ts`. Pass the first matching observation to it so it can extract human-readable names from metadata. Low effort, high impact on prompt quality. --- ### 2. `RedisFactRepository.upsertBatch` is sequential, not pipelined **Dimension:** Performance **Impact:** Medium **Current state:** `upsertBatch` (line 27-31 of `redis-fact-repository.ts`) loops over facts and calls `this.hash.set()` one at a time with `await`: ```ts async upsertBatch(identityId: string, facts: Fact[]): Promise { const k = this.key(identityId); for (const fact of facts) { await this.hash.set(k, fact.id, JSON.stringify(fact)); } } ``` With in-memory stores this is fine, but when wired to real Redis this becomes N sequential round-trips. For a user with 10-15 active facts, that's 10-15ms of serial I/O inside the consolidation timeout window (500ms). **Greenfield alternative:** Add a `setMany(key: string, entries: [field: string, value: string][])` method to the `HashStore` interface, mapping to Redis `HMSET`. Then `upsertBatch` becomes a single round-trip. **Migration path:** Extend `HashStore` with `setMany`, add a default implementation that delegates to `set` in a loop (backward compat), override in the real Redis implementation with `HMSET`. Low effort. --- ### 3. Expired fact deletion is also sequential **Dimension:** Performance **Impact:** Low **Current state:** In `consolidate.ts` (lines 167-169), expired facts are deleted one at a time: ```ts for (const factId of expiredIds) { await factRepository.delete(identityId, factId); } ``` Same issue as #2 but lower impact since fact expiry is rare (only happens when facts haven't been reinforced in 30-45 days). **Greenfield alternative:** Add a `deleteBatch(identityId: string, factIds: string[])` method to `FactRepository`, mapping to Redis `HDEL` with multiple fields. **Migration path:** Bundle with the `setMany` change in finding #2. Trivial. --- ### 4. `MemorySortedStore.rangeByScore` does a full tree scan **Dimension:** Performance **Impact:** Medium **Current state:** `ScoredMemberTree.rangeByScore` (line 99-106 of `sorted-store.ts`) iterates from the tree's minimum and skips entries below `min`: ```ts rangeByScore(min: number, max: number, limit?: number): string[] { const result: string[] = []; for (const entry of this.tree.sorted()) { if (entry.score < min) continue; // ← scans past all entries below min if (entry.score > max) break; result.push(entry.member); if (limit != null && result.length >= limit) break; } return result; } ``` This is O(n) even when the matching range is empty. An AVL tree supports O(log n) lower-bound search. Since `sorted()` starts at the minimum, every call to `since(identityId, checkpoint)` scans all observations older than the checkpoint before finding the relevant ones. **Greenfield alternative:** Expose a `from(lowerBound)` method on the AVL tree that starts iteration at the first element ≥ the bound. This makes `rangeByScore` O(log n + k) where k is the result count. **Migration path:** This is purely internal to `MemorySortedStore` and won't affect the `SortedStore` interface. When `@coda/collections` AVL tree gains a `from()` iterator, update the private `ScoredMemberTree` class. Medium effort. Note: this is an in-memory-only concern and won't apply once real Redis is wired. --- ### 5. Stream handler memory emission block should be extracted **Dimension:** Structure **Impact:** Medium **Current state:** The `onToolExecution` callback in `stream-handler.ts` (lines 355-419) contains two distinct responsibilities: persisting tool calls via `StreamPersister` and emitting memory observations. The memory block alone is ~40 lines that parse tool results, build timing maps, and loop over observations. This makes the already-long handler harder to read. **Greenfield alternative:** Extract the memory-emission logic into a function like `emitMemoryForToolExecution(memoryService, identityId, data)` in `observation-emitters.ts` (or a new `memory-integration.ts`). The stream handler would call it as a single line alongside the persister block. **Migration path:** Pure refactor — extract the block from lines 378-418 into `observation-emitters.ts`. No behavior change. Low effort. --- ### 6. Timing data assigns round duration to every tool in the round **Dimension:** Abstractions **Impact:** Low **Current state:** In the stream handler (lines 381-386), every tool in a round gets the same `roundDuration` as its timing: ```ts const roundDuration = data.roundCompletedAt && data.roundStartedAt ? data.roundCompletedAt - data.roundStartedAt : 0; const timing = new Map(); for (const tu of data.toolUses) { timing.set(tu.toolUseId, roundDuration); } ``` If three tools execute in parallel in a 500ms round, each is recorded as taking 500ms. This inflates `durationMs` in observation metadata. **Greenfield alternative:** If per-tool timing isn't available from the orchestrator, either (a) omit `durationMs` entirely (it's informational metadata, not used for any computation), or (b) divide round duration evenly as a rough estimate. **Migration path:** Trivial — change the timing assignment or remove it. The `durationMs` field isn't used by consolidation or affinity extraction, so the impact is purely on observability of the observation data. --- ### 7. Domain pattern matching is first-match-wins **Dimension:** Abstractions **Impact:** Low **Current state:** `emitQueryPatternObservation` (line 150-162 of `observation-emitters.ts`) returns on the first regex match: ```ts for (const [pattern, domain] of DOMAIN_PATTERNS) { if (pattern.test(query)) { return { ... }; } } ``` A query like "Show me royalty revenue for this account" matches only "royalty" because it's first in the array. The "revenue" and "account" domains are missed. Over time this biases `domain_preference` facts toward whichever domain appears first in the pattern list. **Greenfield alternative:** Emit one observation per matching domain, or at minimum return an array. This gives the consolidation pipeline a more accurate signal of multi-domain queries. **Migration path:** Change the return type to `PartialObservation[]` and emit all matches. Update the single callsite in stream-handler to loop. Low effort. --- ### 8. `AsyncCollection.size` dual type creates awkward ergonomics **Dimension:** Abstractions **Impact:** Low **Current state:** `AsyncCollection` declares `size` as `number | Promise`: ```ts readonly size: number | Promise; ``` This means every consumer that reads `size` from a generic `AsyncCollection` must `await` it even for in-memory implementations where it's always synchronous. The `AsyncMap` implementation returns a synchronous `number`, so callers that type-narrow to `AsyncMap` are fine, but generic code working with the interface has to handle both. **Greenfield alternative:** Make `size` consistently async: `size(): Promise`, or drop it from the interface entirely (it's not used by any consumer in this PR). **Migration path:** Since `AsyncCollection` is new in this PR and has exactly one implementor (`AsyncMap`), change now before the interface proliferates. Minimal effort. --- ### 9. `isAsyncCollection` type guard is fragile **Dimension:** Abstractions **Impact:** Low **Current state:** `isAsyncCollection` (lines 27-40 of `async-collection.ts`) checks for the presence of methods by name: ```ts typeof o.entries === "function" && typeof o.keys === "function" && typeof o.values === "function" && typeof o.clear === "function"; ``` Any object with `entries`, `keys`, `values`, and `clear` methods passes the check, including a regular `Map` in certain environments. The check doesn't use `Symbol.toStringTag` or any brand. **Greenfield alternative:** This function has no consumers in this PR. If it's needed, use a symbol brand check (e.g., check `Symbol.toStringTag` matches a known value). If it's not needed, remove it — YAGNI. **Migration path:** Remove or add brand check. Trivial. --- ### 10. In-memory checkpoint (`AsyncMap`) has no TTL or Redis backing **Dimension:** Structure **Impact:** Medium (operational, not architectural) **Current state:** The checkpoint store is `new AsyncMap()` — pure in-memory. On server restart, all checkpoints are lost. The next consolidation re-processes _all_ observations (since `checkpoint.get()` returns `undefined`), potentially double-reinforcing every fact. The architecture doc acknowledges this is temporary. This is explicitly documented as a TODO in `server.ts` and `agent-memory.md`, so flagging it here primarily for completeness and to note the operational impact: in a deployment with frequent restarts (ECS task cycling), every restart re-consolidates. The 7-day prune buffer limits the blast radius, and double-reinforcement is bounded, but it's worth prioritizing the Redis migration for this reason. **Greenfield alternative:** Store checkpoints in the same Redis hash as facts (e.g., a special `__checkpoint` field), or use a dedicated Redis key. The `AsyncMap` interface already matches Redis GET/SET semantics. **Migration path:** Add a `RedisCheckpointStore` implementing `AsyncMap` backed by a `KeyValueStore`. Wire it when Redis sorted set support is added. --- ## Test Assessment **Well-covered:** - Core consolidation pipeline — 14 test cases covering creation, reinforcement, decay, expiry, checkpointing, pruning, deduplication, mixed types. This is the most critical code path and it's thoroughly exercised. - Both decay strategies — `ExponentialDecay` (10 tests) and `AdaptiveDecay` (12 tests) cover apply, reinforce, edge cases (elapsed=0, elapsed<0), ease factor clamping, and property preservation. - Observation emitters — 15 tests across tool observations, entity observations, and query patterns. Good coverage of edge cases (meta-tool filtering, missing results, unrecognized tools, empty queries, long query truncation). - Prompt formatter — 8 tests covering empty input, budget enforcement, ranking, category grouping, category merging (affinity+avoidance). - `MemoryService` — 8 tests covering loadMemory (empty, with facts, timeout, error), emitObservation (append, inflight tracking, error swallowing), and drain. - Redis store implementations — 4 tests each for `RedisObservationLog` and `RedisFactRepository`, exercising CRUD and isolation. - Supporting data structures — `AsyncMap` (11 tests), `MemorySortedStore` (5 tests), `MemoryHashStore` (7 tests). **Missing:** - **No integration test for the full stream-handler memory flow.** The individual pieces are tested but nothing verifies that `loadMemory → prompt injection → converseWithTools → onToolExecution → emitObservation` works end-to-end. Given the stream handler is a critical integration point, a functional test that verifies observations are emitted during a streamed response would catch wiring issues (like the affinity key mismatch that was already caught in review). - **No test for concurrent `emitObservation` + `drain` race conditions.** The inflight-set pattern works, but there's no test for the edge case where `drain()` is called while an `emitObservation` is being added to the set (the timing window between `this.inflight.add(promise)` and the `.finally()` that removes it). - **`observationToFactKey` with unknown observation type.** The `default: return null` branch is untested. While TypeScript exhaustiveness catches this at compile time, a runtime test would guard against deserialized observations with unexpected types. - **`formatMemorySection` with facts that have very long `content` strings.** The budget enforcement test uses a ~110-char string but doesn't test the case where a single fact's content exceeds the entire budget. **Redundant:** - None observed. Test coverage is well-distributed without significant overlap. ## Recommendations ### Immediate (fix now) 1. **Fix fact `content` on creation** — replace `content: subject` with a human-readable string derived from the fact key and observation metadata. The current behavior injects internal key notation into the system prompt. (Finding #1) 2. **Extract memory emission from stream handler** — move the 40-line `onToolExecution` memory block into `observation-emitters.ts` as a single function. Reduces stream handler complexity and improves testability. (Finding #5) ### Next iteration 3. **Add `setMany` to `HashStore`** — enables pipelined Redis writes for `upsertBatch` when real Redis is wired. (Finding #2) 4. **Emit multiple domain observations per query** — change `emitQueryPatternObservation` to return all matching domains, not just the first. (Finding #7) 5. **Add a stream-handler integration test for memory** — verify the end-to-end wiring from request → observation emission → consolidation in a subsequent session. (Test Assessment) ### Improvement (when convenient) 6. **Optimize `MemorySortedStore.rangeByScore`** — use AVL lower-bound search instead of full scan. Only relevant while in-memory stores are in use. (Finding #4) 7. **Simplify `AsyncCollection.size` type** — make it consistently `Promise` or remove from the interface. (Finding #8) 8. **Remove `isAsyncCollection`** — no consumers; if needed later, use symbol-brand check. (Finding #9) 9. **Omit or fix per-tool timing** — the current round-duration-per-tool approach inflates `durationMs`. (Finding #6)