Recall payload projection (2.1): stop shipping the vector back - #186
Merged
Conversation
Every entity, fact and preference vector search returned the whole node,
including a 384-dim embedding -- roughly 3 KB per item and ~130 KB per
turn moved across the wire, deserialized, and discarded. Nothing on the
recall path reads it; the similarity was computed inside the index. A
measured prototype showed -91% on the entity transaction and -21% on the
whole turn with every quality guard flat.
That prototype was reverted because an unconditional projection is
unsafe here, and confirming why was the first thing worth doing: the TCK
conformance bridge serialises embeddings on three search endpoints --
/search_messages, /search_entities, /search_preferences -- and would
silently begin emitting null. Hence the original demand for validation
against the full 178-case run.
Making it opt-in removes that requirement rather than deferring it. A
consumer that re-uses recalled vectors does not enable the option, so it
is unaffected by construction rather than by a test run that passed
once. The default is off and the default query is byte-identical.
Three details that would each have been a silent failure:
The projection happens at the final return only. The recency re-ranker
reads node.last_accessed_at and node.created_at in its WITH clauses, so
stripping earlier would have disabled D1 and quietly reverted ordering
to semantic-only rather than shrinking anything.
It projects `.*` rather than an allow-list of properties. An explicit
list silently drops any property added later, and a mapper reading a
missing key produces a null field, not an error.
The mappers are split into dictionary and INode overloads sharing one
body, following Neo4jMessageRepository. `node {.*, embedding: NULL}` is
a MAP, not a Node -- the documented trap here -- and two independent
mapper bodies would eventually map the same stored row differently
depending on which query fetched it.
Verified against a live database, not just in the Cypher text: field-
for-field equivalence between projected and unprojected reads, scores
unchanged, and owner isolation intact -- a payload change that widened
visibility would be the worst possible way to save 3 KB.
4,370 unit (+11) and 357 integration (+5) green. Release 0 warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “recall payload projection” to avoid returning stored embedding vectors on entity/fact/preference vector recall, reducing wire/deserialization overhead while keeping default behavior unchanged for compatibility (e.g., the TCK bridge).
Changes:
- Introduces
MemoryOptions.OmitEmbeddingsFromRecall(defaultfalse) and wires it through Neo4j entity/fact/preference vector recall. - Updates shared Cypher tail (
VectorRerank.Finish) to optionally projectembeddingtoNULLat the finalRETURN, and updates repositories to map projected results (MAP) via property dictionaries. - Adds unit + integration tests to pin query-shape behavior and validate projected-vs-unprojected equivalence against Neo4j.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AgentMemory.Tests.Unit/Queries/ScopedVectorSearchQueryTests.cs | Adjusts delegate construction due to updated SearchByVector signatures. |
| tests/AgentMemory.Tests.Unit/Queries/RecallPayloadProjectionTests.cs | New unit tests asserting default/off behavior and projection shape when enabled. |
| tests/AgentMemory.Tests.Integration/Repositories/RecallPayloadProjectionIntegrationTests.cs | New Neo4j integration coverage validating projected payload mapping equivalence (entity + fact). |
| src/AgentMemory.Neo4j/Repositories/Neo4jPreferenceRepository.cs | Threads option into vector recall and adds MAP/dictionary mapping branch. |
| src/AgentMemory.Neo4j/Repositories/Neo4jFactRepository.cs | Threads option into vector recall and adds MAP/dictionary mapping branch. |
| src/AgentMemory.Neo4j/Repositories/Neo4jEntityRepository.cs | Threads option into vector recall and adds MAP/dictionary mapping branch. |
| src/AgentMemory.Neo4j/Queries/VectorRerank.cs | Adds optional final-return projection (node {.*, embedding: NULL} AS node) shared by entity/fact/preference searches. |
| src/AgentMemory.Neo4j/Queries/PreferenceQueries.cs | Adds omitEmbedding optional parameter and forwards to VectorRerank.Finish. |
| src/AgentMemory.Neo4j/Queries/FactQueries.cs | Adds omitEmbedding optional parameter and forwards to VectorRerank.Finish. |
| src/AgentMemory.Neo4j/Queries/EntityQueries.cs | Adds omitEmbedding optional parameter and forwards to VectorRerank.Finish. |
| src/AgentMemory.Abstractions/Options/MemoryOptions.cs | Adds and documents OmitEmbeddingsFromRecall. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+219
to
+223
| // Payload projection: nothing on the recall path reads the vector back, and it is ~3 KB an item. | ||
| bool omitEmbedding = _omitEmbeddingsFromRecall; | ||
| async Task<List<(Entity, double)>> QueryAsync(int width, CancellationToken ct) | ||
| { | ||
| var cypher = EntityQueries.SearchByVector(hasOwner, includeShared, width, recencyRerank); | ||
| var cypher = EntityQueries.SearchByVector( |
Comment on lines
391
to
394
| var cypher = FactQueries.SearchByVector( | ||
| hasOwner, includeShared, width, recencyRerank, currentValidTime); | ||
| hasOwner, includeShared, width, recencyRerank, currentValidTime, | ||
| omitEmbedding: _omitEmbeddingsFromRecall); | ||
| return await _tx.ReadAsync(async runner => |
Comment on lines
+236
to
238
| var cypher = PreferenceQueries.SearchByVector( | ||
| hasOwner, includeShared, width, recencyRerank, _omitEmbeddingsFromRecall); | ||
| var parameters = new Dictionary<string, object?> |
Comment on lines
+192
to
+196
| /// <b>Opt-in, and that is a correctness decision rather than caution.</b> Recalled memories come | ||
| /// back with <c>Embedding = null</c> when this is on. Anything that re-uses a recalled vector — | ||
| /// notably the TCK conformance bridge, which serialises embeddings on three search endpoints — | ||
| /// must leave it off. Making it a setting the caller enables means such a consumer is unaffected | ||
| /// <i>by construction</i>, rather than by remembering to check. |
Comment on lines
+117
to
+121
| [Fact] | ||
| public async Task FactsProjectEquivalentlyToo() | ||
| { | ||
| await Facts(false).UpsertAsync(new Fact | ||
| { |
This was referenced Aug 27, 2026
Open
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
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.
Every entity, fact and preference vector search returned the whole node including its 384-dim embedding — ~3 KB/item, ~130 KB/turn moved, deserialized and discarded. Nothing on the recall path reads it. Measured prototype: −91% entity transaction, −21% whole turn, quality guards flat.
Why the prototype was reverted, and why this one isn't. Confirmed the trap by inspection: the TCK bridge serialises embeddings on
/search_messages,/search_entities,/search_preferences, so an unconditional projection emits null there — hence the original demand for a 178-case TCK run. Making it opt-in removes that requirement rather than deferring it: the bridge simply never enables it, so it's unaffected by construction.MemoryOptions.OmitEmbeddingsFromRecall, default false, honoured by the three searches sharingVectorRerank.Finish— no partially-respected setting.Three details that would each have failed silently:
node.last_accessed_atin its WITH clauses; stripping earlier would have disabled D1 and reverted ordering to semantic-only rather than shrinking anything..*, not an allow-list. An explicit list silently drops properties added later, and a mapper reading a missing key yields a null field, not an error.node {.*, embedding: NULL}is a MAP, not a Node — the documented trap. Two mapper bodies would eventually map the same row differently depending on which query fetched it.Verified against live Neo4j, not just the Cypher text: field-for-field equivalence, unchanged scores, and owner isolation intact — widening visibility would be the worst possible way to save 3 KB.
4,370 unit (+11) and 357 integration (+5) green. Release 0 warnings.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PgDgctPpbTziBNE2RT8VdE