fix(memory): preserve stable memory handles - #1538
Conversation
The auto-recall block injects memory IDs in dash format (e.g. doc-bd5777c...) while the update_memory tool only recognized colon format (doc:abc123). This caused deletion failures for any memory surfaced via automatic recall. Accept both doc:/doc- and rec:/rec- prefixes in the Parse method so agents can use IDs from auto-recall, find_memories, or raw database storage interchangeably.
- get_memories: hydrate via GetMemoriesByResolvedHandlesAsync so the tool no longer resolves every ID twice (once for per-ID errors, once inside GetMemoriesByIdsAsync). Adds an early-out when nothing resolved. - update_memory: reject empty/whitespace new_content instead of silently wiping a document body; point the model at delete:true for removal. - update_memory: give records a specific error when old_text is supplied (they don't support find-and-replace) instead of the generic payload error. - restore the UTF-8 BOM on MemoryTypedId.cs (matches Add-FileHeaders canonical encoding) and add it to the new MemoryTypedIdTests.cs. - add regression tests for the empty-new_content and record old_text guards.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Self-review — walking through the key changes.
Problem (#1530): memory IDs drifted between two shapes. Automatic recall surfaced raw storage IDs (doc-<guid>), but update_memory / get_memories only understood the doc: handle form, so acting on an auto-recalled memory failed. On top of that, every update_memory also enqueued a curation checkpoint that the pipeline later re-applied, clobbering the direct edit.
Approach:
- Split the opaque storage ID (
MemoryStorageId) from the model-facing wire handle (doc:/rec:), and make parsing accept both the handle and the legacy raw forms. - Add one visibility-checked resolver (
ResolveMemoryHandleAsync) that maps any handle to its real stored row, enforces boundary/audience, and fails loudly on genuine ambiguity. - Make
update_memorymutate directly (edit / full-replace / supersede / tombstone) with no checkpoint round-trip, and make recall/get emit typed handles so IDs round-trip. - Skill guidance updated so the agent reuses handles verbatim.
Inline comments below flag the specific lines. Validation: targeted memory tests (99 passing), dotnet slopwatch analyze, and header verify all green.
The storage layer already mints self-describing, unique-per-table primary keys
(doc-{guid} / rec-{guid}). The separate "doc:" / "rec:" wire handle was a second
representation that just wrapped the storage id (surfacing doc:doc-{guid} to the
model), and the strip/re-add reconciliation it required is what forced the
candidate-id expansion and the fail-loud ambiguity branch.
Collapse to one model: the storage id IS the handle.
- MemoryTypedId: drop ToWireValue (all 3 overloads) and CandidateStorageIds;
ToString now returns the storage id. Parse still accepts a legacy doc:/rec:
envelope but keeps the remainder as the exact key (never rewrites it).
- ResolveMemoryHandleAsync: one visibility-scoped lookup on the exact key —
no candidate set, no ambiguity case (a primary key is unique).
- find/get/recall surface the storage id verbatim (no more doc:doc-{guid}).
- ResolvedMemoryHandle.WireValue -> Handle (the storage id verbatim).
- update_memory Id description + netclaw-memory skill: copy the id verbatim.
Tests updated to the canonical model: id forms map to their exact keys (the
old ambiguity test is now a deterministic-resolution test), and recall/get
emit the storage id verbatim. Net -73 lines.
- get_memories: the Ids param description still taught the removed doc:/rec: colon envelope; update it to the verbatim doc-…/rec-… handle contract the rest of the PR standardized on. - SQLiteMemoryStore: remove GetMemoriesByIdsAsync — after get_memories switched to ResolveMemoryHandlesAsync + GetMemoriesByResolvedHandlesAsync it has no remaining callers.
Records are append-only: update_memory supersedes a record by inserting a new
rec-{guid} and leaving the old row physically present, and no read path filtered
superseded rows. So after editing rec-x the model's stable handle still hydrated
the pre-edit row — get_memories("rec-x") returned the OLD payload while the edit
lived under an id the model was never shown. (Confirmed against the live DB: a
superseded record whose old row is still present.)
Resolve record ids through their supersede chain to the head (latest) row at the
single resolution point (ResolveMemoryHandleAsync), so both get_memories and
update_memory act on the current version. Documents edit in place and are
unaffected. Chain walk is a recursive CTE over supersedes_record_id; acyclic, so
the deepest reachable row is the head.
Adds an end-to-end regression test: a record edited twice via its original
handle reads back the latest content (not the pre-edit or first-edit row).
…ty check Addresses the remaining code-review cleanups on the stable-handle change: - get_memories resolved each id on its own SqliteConnection (N+1 opens) and rebuilt allowedAudiences per id. ResolveMemoryHandlesAsync now resolves the whole batch over a single connection with one allowedAudiences set, via a shared connection-scoped core (ResolveHandleOnConnectionAsync); the single ResolveMemoryHandleAsync reuses the same core. - The boundary/audience visibility rule was duplicated between resolution (MemoryIdVisibleAsync) and hydration (two inline checks). Extracted one IsAccessible predicate so they cannot drift. - Documented on MemoryTypedId.Parse that the kind prefix is matched case-insensitively while the storage key is matched verbatim/case-sensitively — a mis-cased key fails loud rather than silently coercing to another row (case-insensitive key matching would be a silent fallback).
Aaronontheweb
left a comment
There was a problem hiding this comment.
Self-review — refreshed for the final state. This supersedes my earlier review, whose inline comments were anchored to code the canonical-collapse has since removed (ToWireValue, the candidate/ambiguity machinery, WireValue); GitHub now shows those as outdated.
Problem (#1530): memory-id formats drifted — automatic recall surfaced raw storage ids (doc-<guid>) while the tools only understood a doc: envelope, so acting on a recalled memory failed. Separately, update_memory enqueued a curation checkpoint that the pipeline later re-applied, clobbering the direct edit.
Final design — one canonical model: the storage id IS the handle.
MemoryStorageIdseparates the opaque storage key from what the model sees; recall/find/get emit it verbatim and the tools accept it verbatim, so an id always round-trips to the exact row.- A single visibility-scoped resolver maps any handle to its row — no candidate expansion, no ambiguity branch (a primary key is unique). Record ids resolve through their supersede chain to the head, so a stable handle keeps returning current content after edits.
update_memorymutates directly (edit / full-replace / supersede / tombstone) with no checkpoint round-trip.
Also folds in the code-review follow-ups: batch resolution over one connection, a single shared visibility predicate, and the record supersede-read fix.
Validation: 228 memory/session unit tests; slopwatch + copyright headers clean; behavioral evals on Spark1 (Qwen3.6-35B) — skill + memory cases green. (Two eval cases initially failed due to a separate harness bug from the #1472 log-stream partition, fixed independently in #1547 — not this change.)
Inline notes walk the key lines.
| // ----------------------------------------------------------------------- | ||
| namespace Netclaw.Actors.Memory; | ||
|
|
||
| public readonly record struct MemoryStorageId(string Value) |
There was a problem hiding this comment.
MemoryStorageId is the core of the redesign — it separates the opaque storage key (the real primary key, doc-{guid} / rec-{guid}) from the model-facing handle. The insight the final design lands on (after a detour through a colon-doc: envelope) is that the storage id already is a good handle: self-describing via its prefix, and unique. So we surface it verbatim and accept it verbatim — there's no second representation left to reconcile.
| /// Returns <see cref="MemoryKind.Unknown"/> with the raw value when the prefix is unrecognized. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The kind prefix is matched case-insensitively (a tolerant envelope), but the storage key |
There was a problem hiding this comment.
Case policy, flagged because it reads like a bug but isn't: the kind prefix is recognized case-insensitively (tolerant envelope), but the storage key that follows is matched verbatim / case-sensitively. Generated ids are always lowercase, so this is deliberate — a mis-cased key fails loud rather than being silently coerced to a different row. Case-insensitive key matching would be a silent fallback.
| }, ct); | ||
| } | ||
|
|
||
| public async Task<IReadOnlyList<ResolvedMemoryHandle>> ResolveMemoryHandlesAsync( |
There was a problem hiding this comment.
get_memories resolves a batch of ids — this resolves the whole batch over one connection (and builds the allowed-audience set once) instead of a connection per id. Each id runs through the shared ResolveHandleOnConnectionAsync core below, which the single-id ResolveMemoryHandleAsync also reuses.
|
|
||
| // Core handle resolution against an already-open connection, so a batch (get_memories) can | ||
| // share a single connection and allowedAudiences set instead of opening a connection per id. | ||
| private static async Task<ResolvedMemoryHandle> ResolveHandleOnConnectionAsync( |
There was a problem hiding this comment.
The resolver core. Parse yields the exact storage key, so this is a single visibility-scoped lookup — found or not — with no candidate expansion and no ambiguity branch (a primary key is unique per table). MemoryIdVisibleAsync enforces boundary/audience before a handle is ever returned: resolution is the authoritative access gate.
| if (parsed.Id.IsEmpty) | ||
| return ResolvedMemoryHandle.Failed(rawId, parsed.Kind, "ID payload is required."); | ||
|
|
||
| // Records are append-only: an edit inserts a new row that supersedes the old one and |
There was a problem hiding this comment.
Records are append-only — an edit supersedes (inserts a new rec-{guid}, leaves the old row in place). Without this, a stable handle the model already holds would keep hydrating the pre-edit row after an edit, so get_memories would return stale content. Resolving record ids through the supersede chain to the head (recursive CTE in ResolveRecordHeadAsync) keeps the handle pointing at current content. Documents edit in place and skip this.
|
|
||
| // Single source of truth for the boundary/audience visibility rule, shared by handle | ||
| // resolution (MemoryIdVisibleAsync) and hydration so the two paths cannot drift. | ||
| private static bool IsAccessible(string itemBoundary, string itemAudience, string boundary, ISet<string> allowedAudiences) |
There was a problem hiding this comment.
One IsAccessible predicate for the boundary/audience rule, shared by resolution (MemoryIdVisibleAsync) and hydration (the two inline checks above). It was duplicated in three places — consolidated so the access rule can't drift between the resolve and hydrate paths.
| var sessionId = string.IsNullOrWhiteSpace(context.SessionId) ? "manual/tool" : context.SessionId!; | ||
| var audience = MemoryPolicyScopeResolver.ResolveAudience(context.Audience, sessionId); | ||
| var boundary = MemoryPolicyScopeResolver.ResolveBoundary(context.Boundary?.Value); | ||
| var resolved = await _store.ResolveMemoryHandleAsync(args.Id, boundary, audience, ct); |
There was a problem hiding this comment.
Biggest behavioral change. The tool resolves the handle, then applies the mutation directly (tombstone / edit / replace / supersede) and logs it. The old path also enqueued an audit curation checkpoint that the pipeline re-processed and clobbered the direct edit — that bug is gone, and the IMemoryCheckpointSink dependency with it (see Program.cs, which drops the constructor arg).
| if (typedId.Kind == MemoryKind.Document) | ||
| if (resolved.Kind == MemoryKind.Document) | ||
| { | ||
| if (args.NewContent is not null) |
There was a problem hiding this comment.
New new_content = full document replacement, mutually exclusive with old_text/new_text. The guard just below rejects empty/whitespace new_content (it used to silently wipe the body) and points the model at delete: true for removal.
| var audience = MemoryPolicyScopeResolver.ResolveAudience(context.Audience, sessionId); | ||
| var boundary = MemoryPolicyScopeResolver.ResolveBoundary(context.Boundary?.Value); | ||
| var entries = await _store.GetMemoriesByIdsAsync(ids, boundary, audience, ct); | ||
| var resolved = await _store.ResolveMemoryHandlesAsync(ids, boundary, audience, ct); |
There was a problem hiding this comment.
get_memories resolves every handle first so it can surface per-ID errors (unknown / not-visible) instead of silently dropping them, then hydrates from the already-resolved handles — one resolution pass, not two.
| foreach (var item in recall.Items) | ||
| { | ||
| sb.AppendLine($"- {item.Title} [{item.Id}] sensitivity={item.Sensitivity} score={item.Score:F2}"); | ||
| sb.AppendLine($"- {item.Title} [{item.Id.Value}] sensitivity={item.Sensitivity} score={item.Score:F2}"); |
There was a problem hiding this comment.
The round-trip's other half: the automatic-recall block prints the storage id verbatim ([doc-…]), so whatever the model copies out of recall parses straight back into update_memory / get_memories with no doc:-envelope rewriting.
Fixes #1530
Supersedes #1532
Summary
Validation
MemoryTypedIdTests|FullyQualifiedNameSqliteMemoryToolsTests|FullyQualifiedNameSQLiteMemoryStoreTests|FullyQualifiedNameSessionMessageAssemblerTests"