feat(agent-memory): Phase 1 — remember() write path#248
Conversation
ffc0fd3 to
cecbb45
Compare
fe9da3d to
b433d60
Compare
KIvanow
left a comment
There was a problem hiding this comment.
One thing I think needs fixing (let me know if it's already addressed in the follow-up PRs).
importance is caller input but written unvalidated. buildMemoryRecord does String(options.importance ?? 0.5) with no [0, 1] clamp and no finiteness check, even though the type implies 0..1 and Phase 2 multiplies it straight into the composite score (weights.importance * importance). The failure chain is quiet: a caller passing importance: NaN/Infinity/out-of-range gets it stored verbatim into a NUMERIC-indexed field, then at recall parseFloat yields NaN, the composite score becomes NaN, and Phase 2's Number.isFinite guard silently drops the item. So a bad importance value makes the memory unrecallable with no error, or for a large finite value lets importance dominate ranking. Since this is a caller-supplied value at a system boundary, worth clamping (or validating and throwing) at write time rather than trusting it.
- MemoryStore.remember(): embed content, HSET {name}:mem:{id} hash with
content, encoded vector, scope fields, importance (default 0.5), tags csv,
source, created_at/last_accessed_at, access_count=0; returns the id
- Validate embedding dimension on first write; mismatch throws
- Extract pure buildMemoryRecord() and unit-test it standalone
- Export memory types (EmbedFn, MemoryScope, RememberOptions, MemoryStoreOptions)
… write A caller-supplied importance was stored verbatim, so NaN/Infinity or an out-of-range value silently poisoned recall: the composite score became NaN and the memory was dropped, or a large value dominated ranking. Reject it at the write boundary, matching the package's other input guards.
6bc85ee to
a60f871
Compare
Agent Memory — Phase 1:
remember()write pathStacked on #247 (Phase 0) — base is the Phase 0 branch.
What's new
MemoryStore.remember(content, options?)→ embeds content (once), HSETs the{name}:mem:{id}hash withcontent, encodedvector, scope fields (threadId/agentId/namespace),importance(default 0.5),tags(CSV),source,created_at/last_accessed_at,access_count=0; returns the id.buildMemoryRecord()extracted and unit-tested standalone.EmbedFn,MemoryScope,RememberOptions,MemoryStoreOptions.Tests
9 unit tests (mocked client + deterministic
fakeEmbed);tsc --noEmit+ prettier clean.Review-driven changes
,), so a comma inside a tag would silently break Phase 2 filtering.tagswhen empty (consistent with the other optional fields) rather than writing''.RememberOptionsto only implemented fields — removedttl/metadataso the API doesn't advertise options Phase 1 doesn't honor (they return with their phases:ttl→ Phase 5).Deferred
Next
Phase 2 (
recall()ranking — composite score pure-math first, then KNN) stacks on this.Note
Low Risk
Additive write-only API with input validation and mocked tests; no auth or recall path yet, though bad embedFn dimensions or Valkey failures would surface at runtime.
Overview
Implements the Phase 1 long-term memory write path:
MemoryStoreis wired with a Valkey client, storename, andembedFn, andremember(content, options?)embeds text once, persists a{name}:mem:{uuid}hash viaHSET, and returns the new id.Record shape is built by a pure
buildMemoryRecord()helper:content, float32-encodedvector, defaultimportance0.5 (validated to [0, 1]), timestamps,access_count0, and optional scope/metadata (threadId,agentId,namespace,source, comma-joinedtags). Empty optionals are omitted; tags with commas are rejected so CSV/TAG indexing stays safe. The store tracks embedding dimension on first write and throws on later mismatches (same idea as semantic-cache).Public types
EmbedFn,MemoryStoreClient,RememberOptions, andMemoryStoreOptionsare exported from the package entrypoint. Unit tests coverremember(mock client) andbuildMemoryRecordvalidation/defaults.Reviewed by Cursor Bugbot for commit a60f871. Bugbot is set up for automated code reviews on this repo. Configure here.